Skip to content

fix(feedback): make the anonymity toggle read as a control - #406

Open
gongfpp wants to merge 1 commit into
pacifio:0.4.1from
gongfpp:gongfpp/69-feedback-anonymity-affordance
Open

gongfpp wants to merge 1 commit into
pacifio:0.4.1from
gongfpp:gongfpp/69-feedback-anonymity-affordance

Conversation

@gongfpp

@gongfpp gongfpp commented Oct 13, 2026

Copy link
Copy Markdown

What?

The feedback panel's identity row is a switch now, and it looks like one.

Why?

Fixes #69. The row already said which mode was active, but nothing said it could be clicked. The hover was a colour change only, and the explanation lived in a native title that opens after the ~1s OS delay — by which point the row has already read as static text.

How?

  • Affordance. A rounded hover fill (hover:bg-element-hover, the token the app's other interactive rows use) plus a little padding. The padding is cancelled by -mx-1.5 so the text keeps the panel's gutter, and the row's height is set by the h-6 Send button either way — so nothing reflows, including across the toggle.
  • Keyboard focus is the existing app-wide :focus-visible ring from globals.css; no second ring was added.
  • role="switch" + aria-checked. It was an unlabelled control whose entire meaning lived in changing text.
  • A fixed aria-label="Send anonymously", per @uzayer in the thread. Naming it after the visible text would make a screen reader announce "Sending as Jane Doe, switch, off" — the name contradicting the state it just reported.
  • title → Hint (src/ui/tooltip.tsx), so the hint opens on the shared delay and matches every other hint in the app.

Tests

src/features/feedback/components/feedback-panel.test.tsx — three cases: the switch role and its fixed name (and that the name does not follow the visible text), the dropped native title, and that toggling flips aria-checked, the visible label, and the store the submit payload reads. All three fail against the previous component.

Screenshots

Both states, pointer on the row so the hover affordance is in the shot.

Attributed to the account:

Feedback panel with the pointer on the identity row, which reads "Sending as Dev Halvorsen" on a rounded hover fill

Anonymous:

Feedback panel after toggling, reading "Sending anonymously" on the same hover fill

Checklist

Only the things CI can't check for you:

  • Targets the current version branch, unless it's a small standalone fix
  • New behaviour has a test; a bug fix has a test that fails without it
  • You've run the app and used the change in a window

On the last box: this was checked in bun run dev against the mock backend, not in the
dev:app bundle — @uzayer's guidance on #69
says browser screenshots are enough for this one, so the two above are the evidence rather
than a claim I can't make. Happy to rebuild in dev:app if you'd rather see it there.

The identity row already showed which mode was active, but it was styled as
status text: a colour-only hover, and a native `title` that only appeared
after the ~1s OS delay. Nothing said the row could be clicked.

- hover fill plus the app-wide `:focus-visible` ring, so the row reads as
  interactive. The padding is cancelled by a negative margin, so the gutter
  and the row height are unchanged.
- `role="switch"` and `aria-checked` for screen readers, with a FIXED
  `aria-label`. The visible text changes between states, so naming the
  control after it made a screen reader read "Sending as Jane, switch, off".
- the app's own `Hint` tooltip replaces the native `title`, so the hint
  opens on the shared delay and matches every other hint in the app.

Fixes pacifio#69
@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ee62fcd3-6857-45a5-8c41-1db4cc311a82

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@gongfpp

gongfpp commented Oct 14, 2026

Copy link
Copy Markdown
Author

CI follow-up — the check suite here is empty rather than red.

No workflow has actually run against this PR. pull_request: in .github/workflows/ci.yml carries no branch filter, so a PR targeting 0.4.1 does trigger it — but for a first-time contributor's fork, GitHub holds the run until it's approved. Until then the only "check" that shows up is CodeRabbit's, which skips itself on non-default branches. That's the whole story behind unstable; it isn't a signal about this change. 0.4.1 is still at c2a280a, which is this PR's base, so there is nothing to rebase.

Local run, with the toolchain mise.toml pins (bun 1.4.0 / node 24):

$ bunx vitest run src/features/feedback/components/feedback-panel.test.tsx
 ✓ src/features/feedback/components/feedback-panel.test.tsx (3 tests) 130ms

 Test Files  1 passed (1)
      Tests  3 passed (3)

…and, in case anything else here touches React rendering, the whole .tsx population:

$ bunx vitest run <all 43 *.test.tsx files>
 Test Files  43 passed (43)
      Tests  341 passed (341)

One environment note, in case it saves the next person an hour: with NODE_ENV=production exported (this container's default), every .tsx test in the repo fails with TypeError: React.act is not a function, because react's production build exports no act — 54 files go red regardless of their diff. Unsetting it makes them pass. Nothing for this PR to fix, but it looks exactly like a bad change if you hit it cold.

Once the run is approved I'll follow up on anything CI flags. And if you'd rather see the two states rebuilt inside the dev:app bundle instead of dev, say the word and I'll redo the screenshots — bun run dev against the mock backend is what I used, per your note on #69 that browser screenshots are enough here.

@gongfpp

gongfpp commented Oct 14, 2026

Copy link
Copy Markdown
Author

Closing this to keep the queue tidy — there are older unmerged PRs ahead of it and no sign of review capacity here at the moment.

Nothing is wrong with the change: CI never ran (action_required), but the same suite passes locally (341 tests, all 43 .tsx files) with the mise.toml toolchain. The branch stays at gongfpp:gongfpp/69-feedback-anonymity-affordance, so just say the word and I'll reopen it or re-target whatever version you want.

@gongfpp gongfpp closed this Oct 14, 2026
@gongfpp gongfpp reopened this Oct 14, 2026
@gongfpp

gongfpp commented Oct 14, 2026

Copy link
Copy Markdown
Author

Update: I've reopened this and I'm leaving it open — the closing note I left above was premature.

The change itself stands: CI never ran on this PR (action_required), but the same suite passes locally — 341 tests across all 43 .tsx files with the mise.toml toolchain. The branch gongfpp/69-feedback-anonymity-affordance stays available if you want it as-is, and I'm happy to re-target whatever version you prefer or to wait for review capacity.

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