Skip to content

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
mainfrom
feat/feedback-nudge-cooldown-issue-912
Closed

feat(feedback): inline nudge instead of an auto-opening modal, cooldown instead of a lifetime cap#923
Samhit21 wants to merge 1 commit into
mainfrom
feat/feedback-nudge-cooldown-issue-912

Conversation

@Samhit21

Copy link
Copy Markdown
Collaborator

Implements the two changes recommended in the research pass on #912 (writeup).

⚠️ #912 says "don't open a PR yet" — it asks for research first, then a scoped discussion with @s-annam. This PR exists because the work was explicitly requested after the writeup was posted. Treat it as a concrete proposal to react to rather than an agreed scope; the two open product calls below are deliberately not in it.

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

Dialog is a native showModal(). 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 new initialRating prop, so nobody is asked for the same star twice.

Side effect worth noting: this nearly retires the two-moment notifyResumeExported / notifyExportClosed dance. A non-modal nudge can't stack on ExportDialog, 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. And openDialog() incremented that same counter, so a user who clicked the ambient [★ Feedback] button out of curiosity permanently disabled the automatic ask.

before after
Automatic ask once per browser, forever every 14 days
Ambient open burns the automatic trigger untouched
After submitting never auto-asked again unchanged

ocv_feedback_prompted_at records 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) nor ocv_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

  • Which milestone earns the ask. Export is the rarest success moment in the funnel — anyone who parses, reads their score, edits and leaves is never asked. Changing it is a product call, not an implementation detail.
  • Per-parse vs per-browser memory. The issue's question 3, still open. Falls out cheaply if the cooldown key incorporates parseKey, but "is a new résumé a new experience to rate" is a decision, not a refactor.
  • A dismissed rating is lost entirely — pick 2★, close without typing, nothing is recorded. That's the strongest signal we have, discarded from the users least likely to write a paragraph. Fixing it changes the feedback_submitted event 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 verify green: 384 test files, 6464 passed, 10 skipped, build clean, fallow audit --base origin/main exit 0.

19 new tests. The ones with teeth:

  • the milestone raises the nudge and not the dialog;
  • the ambient button does not write the cooldown key;
  • suppressed inside the window and asks again past it — without the second case the first would pass against a permanent lock;
  • both retired keys are ignored and left untouched;
  • an unreadable timestamp asks;
  • the nudge is not a dialog and has no aria-modal — if that ever regresses, this PR has been undone.

One fallow note: App.tsx's App function 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.

…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.
@Samhit21

Copy link
Copy Markdown
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.

@Samhit21 Samhit21 closed this Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant