frontend: keep features modal visible after scrolled row clicks

[shiv-tyagi]

Clicking a feature name after scrolling the list scrolled the overflow-hidden dialog off-screen, so the modal looked blank. Rows are now role="checkbox" controls instead of a <label> around a hidden checkbox, which stops the browser from scrolling that control into view.

Made with Cursor

[Copilot]

🟡 Changes recommended

Correct the mixed accessibility state so it reflects the control’s actual checked state.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates feature rows so clicking after scrolling no longer moves the modal off-screen.

Changes:

• Replaces hidden-checkbox labels with role="checkbox" row controls.
• Adds Enter and Space keyboard activation.

File summaries

File	Summary
frontend/src/components/FeaturesModal.tsx	Implements the updated feature-row interaction model.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟢 Approval recommended

No unresolved review issues were identified.

Review details

• Files reviewed: 1/1 changed files
• Comments generated: 0 new
• Review effort level: Lite

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-14)

Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting.

Reviewed at head 0aedae691b. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_15_AIReview/devcall_pr_reviews.html#prCustomBuild-258

Verdict: COMMENT — the fix works, with one small keyboard regression. Both reviewers drove the real FeaturesModal in headless Chromium:

• At the parent, clicking a feature after scrolling the list scrolls the dialog about 3000 px up, leaving the modal blank. At head it stays put. Tabbing through the rows had the same bug at the parent and is also fixed.
• Dependency auto-select, the "Disable anyway" warning, category and toggle-all, search, and the ids passed to onDone behave the same as before.
• Nothing else read the removed <input>. tsc and lint pass.

Issue

• frontend/src/components/FeaturesModal.tsx:481-486: holding Space or Enter skips the "Disable anyway" confirmation.
  • onKeyDown calls onToggle on every keydown, auto-repeats included. Take a checked feature with checked dependents:
    • The first keydown only sets pendingUncheck.
    • The first auto-repeat calls toggle() again. That confirms (:69-77), so the feature and all its dependents are disabled before the key is released.
  • At the parent, the native checkbox toggled once, on keyup, so a held key just left the warning showing.
  • Reproduced by both reviewers with Chromium key events.
  • Suggested fix, tested (restores warning-only behaviour): keep e.preventDefault(), but only call onToggle when !e.repeat.

Notes

• FeaturesModal.tsx:478: aria-label={feature.name} shrinks the checkbox's accessible name to just the feature name. With the <label> it was the whole row: name, category, default on/off and description. Pointing aria-describedby at the description <p> (:520) would keep that context. Checked in Chromium's accessibility tree; not tried with a real screen reader.
• :482: Enter now toggles as well as Space. A native checkbox toggles on Space only, but CategoryCard in the same file already handles Enter, so this is consistent. It only matters because of the repeat issue above.

[shiv-tyagi]

Fixed the issue highlighted in the AI review.

About the notes,

1. It is okay. The app is anyways not optimised to work with screen readers.
2. The issue is fixed, this is consistent with the category card so we should be good.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-15)

Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting.

Re-reviewed at head 0547f77761 (you were last told 0aedae691b); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_15_AIReview/devcall_pr_reviews.html#prCustomBuild-258

The held-key confirmation bypass is fixed. One small side effect of where the guard sits: a held Space now scrolls the list. Verdict: COMMENT (unchanged), one-line fix.

Previous round

• ✅ Held Space/Enter skipping "Disable anyway": with a checked feature that has dependents, a held key now leaves the warning up, and a separate second press confirms. Checked in headless Chromium with the real component; the bypass still reproduces at the previous head.
• ✅ Accessible name and Enter toggling: agreed, both fine for the reasons you give.

Issue

• frontend/src/components/FeaturesModal.tsx:482-485: !e.repeat is in the same if as e.preventDefault(), so repeated Space keydowns get the browser default and scroll the feature list (12 repeats scrolled ~6,000 px in Chromium). Once the pending row leaves view, its IntersectionObserver (:440-450) cancels the warning, so it just disappears. Nothing gets disabled, but a held Space on the old native checkbox didn't scroll.
• Fix: always preventDefault() for Space/Enter and guard only the toggle:

  if (e.key === ' ' || e.key === 'Enter') {
      e.preventDefault();
      if (!e.repeat) onToggle(feature.id);
  }

  Checked in Chromium: the warning stays and the list doesn't scroll.

Also checked: npm ci, tsc --noEmit and lint pass; clicking a row after scrolling keeps the modal visible; nothing else changed between the two heads.

[tridge]

Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting.

Re-reviewed at head ae429a2566 (you were last told 0547f77761); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_15_1401/devcall_pr_reviews.html#prCustomBuild-258

The held-Space scrolling is fixed. No blockers. Verdict: COMMENT → APPROVE.

• ✅ frontend/src/components/FeaturesModal.tsx:482-485: preventDefault() now runs for every Space/Enter keydown and only the toggle skips repeats. Checked in headless Chromium with the real component: one keydown plus 12 repeats of Space or Enter leaves the list unscrolled and the warning up. With the previous version of the file swapped back in, Space scrolls ~5,100 px and the warning disappears.
• ✅ The held-key confirmation fix still holds: a held key toggles once, and a separate press confirms.

Also checked: nothing inside the row is focusable, so the unconditional preventDefault() can't swallow a nested control's keys. npm ci, tsc --noEmit and lint pass; CI 3/3 passing.