feat(feedback): inline nudge instead of an auto-opening modal, cooldown instead of a lifetime cap - #923
Closed
Samhit21 wants to merge 1 commit into
Closed
feat(feedback): inline nudge instead of an auto-opening modal, cooldown instead of a lifetime cap#923Samhit21 wants to merge 1 commit into
Samhit21 wants to merge 1 commit into
Conversation
…wn instead of a lifetime cap (#912) #900 replaced the always-visible inline panel with a milestone-triggered dialog, which fixed the nagging this issue was filed about and overshot into near-silence. Two changes, both from the research pass on #912. **The milestone raises a nudge, not the dialog.** `Dialog` is a native `showModal()`, so opening it automatically took keyboard focus off whatever the user was doing and announced itself to a screen reader as an interruption — for an invitation, the wrong instrument. NN/g's *User-Feedback Requests* endorses asking after task completion and keeping an always-available way in, both of which #900 got right, while explicitly discouraging modal popups for the ask itself. A résumé export now shows `FeedbackNudge`: inline, dismissible, `role="status"`, the star row and one line. Picking a star opens the dialog on that rating's branch — user-initiated, which is when a modal is right — so nobody is asked for the same star twice. **Cooldown instead of a lifetime cap.** The automatic ask was gated on `seenCount > 0` — shown once ever, never again — and `openDialog` incremented that same counter, so a user who opened the dialog from the ambient button out of curiosity permanently disabled being asked. `ocv_feedback_prompted_at` now records only unprompted asks, and only the nudge writes it; the window is 14 days. `ocv_feedback_submitted` is unchanged as the permanent kill switch, since someone who already wrote in should not be asked again. Neither `ocv_feedback_dialog_seen` nor the retired panel's `ocv_feedback_seen` is read. Every returning tester holds a value under both, and honouring them would keep the lifetime cap alive for exactly the people this is for — so those browsers are eligible once, then fall under the cooldown. That single extra ask is the intended cost of loosening the cap. The cooldown fails OPEN: an unreadable timestamp asks rather than staying silent, because a corrupt key that locks someone out of ever being asked is this issue's bug reached by another route. Scope: the two changes recommended in the issue writeup. Which milestone should earn the ask (export is the rarest success moment in the funnel) and per-parse vs per-browser memory both need a product call and are left for follow-ups.
Collaborator
Author
|
Closing for now — reopening the scope discussion on #912 first, per that issue's own instruction to agree the plan before any code. The branch is kept, so this is reopenable as-is if the plan lands here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the two changes recommended in the research pass on #912 (writeup).
Context: the issue's premise was stale
#912 was written against
FeedbackPanel.tsx, which no longer exists — #900/#903 retired it after the Aug 27 sync. Three of the issue's five questions were already answered by that change (no longer always-visible, already branches by rating, already a bare star row that expands). What was left is what this PR does.1. The milestone raises a nudge, not the dialog
Dialogis a nativeshowModal(). Opening it automatically took keyboard focus off whatever the user was doing and announced itself to a screen reader as an interruption — the wrong instrument for an invitation. This is also the answer to the issue's accessibility question, and the answer had got worse with #900, not better: the old concern was a panel appearing below the fold, the new one was a focus trap arriving uninvited.NN/g's User-Feedback Requests endorses asking after task completion and keeping an always-available way in — both of which #900 got right — while explicitly discouraging modal popups for the ask itself.
So a résumé export now shows
FeedbackNudge: inline, dismissible,role="status"(polite, so it waits behind whatever the reader was saying about the export), the star row plus one line. Picking a star opens the dialog on that rating's branch via a newinitialRatingprop, so nobody is asked for the same star twice.Side effect worth noting: this nearly retires the two-moment
notifyResumeExported/notifyExportCloseddance. A non-modal nudge can't stack onExportDialog, so the split now exists only so the nudge doesn't appear behind it, unseen.2. Cooldown instead of a lifetime cap
The automatic ask was gated on
seenCount > 0— shown once, ever. AndopenDialog()incremented that same counter, so a user who clicked the ambient[★ Feedback]button out of curiosity permanently disabled the automatic ask.ocv_feedback_prompted_atrecords only unprompted asks, written when the nudge becomes visible rather than when it's answered — a nudge scrolled past is still an ask received, and re-asking tomorrow because it was ignored is the nagging this exists to prevent.Migration is a deliberate one-time re-ask. Neither
ocv_feedback_dialog_seen(#900) norocv_feedback_seen(the retired panel) is read. Every returning tester holds a value under both, and honouring them would keep the lifetime cap alive for exactly the people this change is for — so those browsers become eligible once, then fall under the cooldown like everyone else.The cooldown fails open. An unreadable timestamp asks rather than staying silent: a corrupt key that locks someone out of ever being asked is this issue's own bug reached by another route.
Deliberately not in scope
parseKey, but "is a new résumé a new experience to rate" is a decision, not a refactor.feedback_submittedevent shape, which Feedback session UX: research + propose flow for the always-visible feedback widget #912 puts out of scope; flagged in the writeup as wanting its own issue.Verification
npm run verifygreen: 384 test files, 6464 passed, 10 skipped, build clean,fallow audit --base origin/mainexit 0.19 new tests. The ones with teeth:
dialogand has noaria-modal— if that ever regresses, this PR has been undone.One fallow note:
App.tsx'sAppfunction trips the complexity threshold at 787 lines. That's pre-existing debt (CLAUDE.md's known-debt list); this PR adds 8 lines to it and doesn't touch the shape. The gate passes.