The arrow-key nudge handler existed but only ever acted on a pointer selection, and nothing could select without a mouse — so the feature was unusable by exactly the keyboard users it's for. This gives the canvas a keyboard path in, and fixes the Modal focus trap every destructive confirmation goes through.
Canvas
<svg role="application"> + an aria-label describing the controls + a <title> naming the garden. A screen reader now meets an interactive canvas, not an empty graphic.
Each object <g> is a focusable role="button" with an aria-label (name + kind) and aria-pressed. Enter/Space selects it — the missing step — which makes the existing arrow-key nudge reachable.
A :focus-visible CSS rule draws a dashed accent ring on keyboard focus (and not on a mouse click — the whole point of :focus-visible). CSS rather than React state because onFocus on an SVG <g> is unreliable, and a CSS rule cleanly overrides the shape's inline stroke.
Closes #84 (the scoped first slice). Part of #86.
The arrow-key nudge handler existed but only ever acted on a **pointer** selection, and nothing could select without a mouse — so the feature was unusable by exactly the keyboard users it's for. This gives the canvas a keyboard path in, and fixes the Modal focus trap every destructive confirmation goes through.
## Canvas
- `<svg role="application">` + an aria-label describing the controls + a `<title>` naming the garden. A screen reader now meets an interactive canvas, not an empty graphic.
- Each object `<g>` is a focusable `role="button"` with an aria-label (name + kind) and `aria-pressed`. **Enter/Space selects it** — the missing step — which makes the existing arrow-key nudge reachable.
- A `:focus-visible` CSS rule draws a dashed accent ring on **keyboard** focus (and not on a mouse click — the whole point of `:focus-visible`). CSS rather than React state because `onFocus` on an SVG `<g>` is unreliable, and a CSS rule cleanly overrides the shape's inline `stroke`.
## Modal
Blast radius: `DeleteGarden` / `ClearBed` / `DeletePlant` / `DeleteSeedLot` / `Share`.
- Tab is trapped inside the dialog and wraps at the ends, instead of walking out into the page behind the backdrop.
- On close, focus returns to the element that **opened** the dialog, not `<body>`.
## Verified live (real keyboard, built binary)
Not just tsc — driven with actual key events against the running app:
| Check | Result |
|--|--|
| Tab focuses an object (SVG `<g tabindex>` takes focus) | ✅ `activeElement === g` |
| Enter selects | ✅ `aria-pressed` false→true |
| Focus ring on keyboard focus | ✅ real Tab → `:focus-visible` matched, computed `stroke-dasharray "5px, 4px"` |
| Dialog traps focus | ✅ 5 real Tabs stayed inside |
| Escape closes + restores focus | ✅ dialog closed, `activeElement === opener` |
(The `:focus-visible` ring can't be triggered by scripted/mouse focus by design, so it's confirmed with genuine Tab presses.)
## Not done here (noted follow-ups)
- Object **dimensions** in the aria-label — needs the garden's unit context this component doesn't hold.
- Roving-tabindex between plops inside a focused bed.
- `EditorRail` tablist semantics (the third item #84 mentions).
`tsc --noEmit` and 87 vitest tests pass.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The arrow-key nudge handler existed but only ever acted on a POINTER
selection, and nothing could select without a mouse — so the feature was
unusable by exactly the keyboard users it's for. This is the scoped first
slice: give the canvas a keyboard path in, and fix the Modal focus trap that
every destructive confirmation goes through.
Canvas:
- The <svg> gets role="application" + an aria-label describing the controls,
and a <title> naming the garden — a screen reader now announces an
interactive canvas rather than an empty graphic.
- Each object <g> is a focusable role="button" with an aria-label (name +
kind) and aria-pressed reflecting selection. Enter/Space selects it — the
step that was missing — which makes the existing arrow-key nudge reachable.
- A :focus-visible CSS rule draws a dashed accent ring on keyboard focus (and
NOT on a mouse click, which is the point of :focus-visible). CSS rather than
React state because onFocus on an SVG <g> is unreliable, and a CSS rule
cleanly overrides the shape's inline stroke.
Modal (blast radius: DeleteGarden/ClearBed/DeletePlant/DeleteSeedLot/Share):
- Tab is trapped inside the dialog and wraps at the ends, instead of walking
out into the page behind the backdrop.
- On close, focus returns to the element that opened the dialog rather than
landing on <body>.
Verified live against the built binary with real keyboard input: Tab focuses
an object (SVG <g tabindex> genuinely takes focus), Enter flips aria-pressed
false→true, the focus-visible dash renders (computed stroke-dasharray "5px,
4px"), the dialog traps focus through 5 Tabs, and Escape closes it and
restores focus to the opener.
Follow-ups noted, not done here: object dimensions in the aria-label (needs the
garden's unit context this component doesn't hold), roving-tabindex between
plops inside a focused bed, and the EditorRail tablist semantics.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 03:28:48Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟠focusable() selector includes input[type="hidden"], breaking tab trap boundary when hidden inputs are at modal edges
correctness, maintainability · flagged by 2 models
web/src/components/ui/Modal.tsx:40 — The focusable() selector uses input:not([disabled]), which includes input[type="hidden"]. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (active === first / active === last) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button…
🪰 Gadfly · advisory
🟠 **focusable() selector includes input[type="hidden"], breaking tab trap boundary when hidden inputs are at modal edges**
_correctness, maintainability · flagged by 2 models_
- **`web/src/components/ui/Modal.tsx:40`** — The `focusable()` selector uses `input:not([disabled])`, which includes `input[type="hidden"]`. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (`active === first` / `active === last`) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button…
<sub>🪰 Gadfly · advisory</sub>
🟠Focus trap doesn't handle the focused element being removed from the DOM (e.g. ShareGardenModal's remove-share button), letting Tab escape the dialog
correctness · flagged by 1 model
web/src/components/ui/Modal.tsx:49 — The Tab-trap checks active === first/last/card but not the case where the previously-focused element was removed from the DOM (focus reverts to <body> per browser default). ShareGardenModal.tsx:117-127's per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press finds document.activeElement === body, which matches none of the checked branches — no preventDefault() fires…
🪰 Gadfly · advisory
🟠 **Focus trap doesn't handle the focused element being removed from the DOM (e.g. ShareGardenModal's remove-share button), letting Tab escape the dialog**
_correctness · flagged by 1 model_
- `web/src/components/ui/Modal.tsx:49` — The Tab-trap checks `active === first/last/card` but not the case where the previously-focused element was removed from the DOM (focus reverts to `<body>` per browser default). `ShareGardenModal.tsx:117-127`'s per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press finds `document.activeElement === body`, which matches none of the checked branches — no `preventDefault()` fires…
<sub>🪰 Gadfly · advisory</sub>
🟡Focus trap falls through (Tab escapes) when activeElement is outside {first, last, card} — external/stolen focus isn't pulled back in
correctness, error-handling · flagged by 2 models
web/src/components/ui/Modal.tsx:61-68 — the Tab-wrap logic only fires when active === first (Shift), active === card (Shift), or active === last (non-Shift). If document.activeElement is none of those — e.g. focus escaped to <body> or an element behind the backdrop because something stole focus after mount, or focus landed on a [tabindex="-1"] child that isn't the card — neither branch executes, preventDefault is not called, and Tab walks out of the dialog. The comment claims i…
🪰 Gadfly · advisory
🟡 **Focus trap falls through (Tab escapes) when activeElement is outside {first, last, card} — external/stolen focus isn't pulled back in**
_correctness, error-handling · flagged by 2 models_
- `web/src/components/ui/Modal.tsx:61-68` — the Tab-wrap logic only fires when `active === first` (Shift), `active === card` (Shift), or `active === last` (non-Shift). If `document.activeElement` is none of those — e.g. focus escaped to `<body>` or an element behind the backdrop because something stole focus after mount, or focus landed on a `[tabindex="-1"]` child that isn't the card — neither branch executes, `preventDefault` is not called, and Tab walks out of the dialog. The comment claims i…
<sub>🪰 Gadfly · advisory</sub>
🟠Focus restore on modal close doesn't check if the opener element is still in the DOM, silently failing for the exact delete/clear flows the PR targets
error-handling · flagged by 2 models
web/src/components/ui/Modal.tsx:73 — opener is captured once at mount (line 33) as document.activeElement and the cleanup calls opener?.focus?.(). The ref is never re-resolved, and there is no isConnected guard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out), .focus() on a detached node is a silent no-op and focus falls to <body> — exa…
🪰 Gadfly · advisory
🟠 **Focus restore on modal close doesn't check if the opener element is still in the DOM, silently failing for the exact delete/clear flows the PR targets**
_error-handling · flagged by 2 models_
- `web/src/components/ui/Modal.tsx:73` — `opener` is captured once at mount (line 33) as `document.activeElement` and the cleanup calls `opener?.focus?.()`. The ref is never re-resolved, and there is no `isConnected` guard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out), `.focus()` on a detached node is a silent no-op and focus falls to `<body>` — exa…
<sub>🪰 Gadfly · advisory</sub>
correctness, maintainability · flagged by 4 models
web/src/editor/ObjectShape.tsx:79 — The accessible label builds the kind display string with object.kind.replace(/_/g, ' '), duplicating a concern that kindDef() in ./kinds.ts already solves. kindDef returns curated, Title Case labels (e.g. "Grow bag", "In-ground") while the regex produces lowercase, unhyphenated output ("grow bag", "in ground"). This creates an inconsistency with the rest of the UI (e.g. JournalPanel uses objectDisplayName, which respects kindDef l…
🪰 Gadfly · advisory
🟠 **Ad-hoc kind string formatting duplicates existing kindDef labels**
_correctness, maintainability · flagged by 4 models_
- **`web/src/editor/ObjectShape.tsx:79`** — The accessible label builds the kind display string with `object.kind.replace(/_/g, ' ')`, duplicating a concern that `kindDef()` in `./kinds.ts` already solves. `kindDef` returns curated, Title Case labels (e.g. `"Grow bag"`, `"In-ground"`) while the regex produces lowercase, unhyphenated output (`"grow bag"`, `"in ground"`). This creates an inconsistency with the rest of the UI (e.g. `JournalPanel` uses `objectDisplayName`, which respects `kindDef` l…
<sub>🪰 Gadfly · advisory</sub>
🟠aria-pressed misused on non-toggle role="button"; object selection is not a toggle action
correctness · flagged by 2 models
web/src/editor/ObjectShape.tsx:96 — aria-pressed={selected} on role="button" advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (handleKey) always calls onSelect(object.id); there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. Fix: remove aria-pressed. I…
🪰 Gadfly · advisory
🟠 **aria-pressed misused on non-toggle role="button"; object selection is not a toggle action**
_correctness · flagged by 2 models_
- **`web/src/editor/ObjectShape.tsx:96`** — `aria-pressed={selected}` on `role="button"` advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (`handleKey`) always calls `onSelect(object.id)`; there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. **Fix:** remove `aria-pressed`. I…
<sub>🪰 Gadfly · advisory</sub>
This diff is confined to frontend accessibility (ARIA attributes, SVG role/tabIndex, CSS :focus-visible, and a Tab-focus trap in a modal). I checked the specific spots most likely to hide a security issue for this kind of change:
garden.name, objectDisplayName(object), and object.kind are all interpolated into JSX aria-label attributes and an SVG <title> text node (web/src/editor/GardenCanvas.tsx:242, :244, web/src/editor/ObjectShape.tsx:79). These go through React's normal attribute/text rendering, which escapes them — no dangerouslySetInnerHTML or raw DOM writes exist anywhere in web/src (verified via grep), so there's no XSS vector even if a garden/object name contained markup.
The Modal's new focus-trap (web/src/components/ui/Modal.tsx:37-42) scopes querySelectorAll to card (the dialog root), so a stray focusable element elsewhere on the page can't be pulled into the trap, and the opener restored on close is whatever document.activeElement already was in the same document at mount time — not attacker-influenceable.
No new network calls, no new data persistence, no server-side code touched — this is pure client-side DOM/ARIA/CSS wiring.
Nothing here touches authn/authz, injection surfaces, untrusted-data handling, or secrets.
🎯 Correctness — Minor issues
Both findings confirmed against the actual source.
Finding 1 (ObjectShape.tsx:79): Confirmed. kindDef/objectDisplayName are imported and objectDisplayName is used for the name half of the label, but the kind half uses object.kind.replace(/_/g, ' ') instead of kindDef(object.kind)?.label. kinds.ts:18-24 shows real labels differ: in_ground → "In-ground" (label uses a hyphen, regex produces "in ground"), and all others are title-cased ("Grow bag" vs regex's "grow bag").
Finding 2 (Modal.tsx:44-69): Confirmed. The trap logic only checks active === first, active === last, or active === card. ShareGardenModal.tsx:117-127 renders a "Remove" button inside the trapped dialog whose onClick removes that share from shares.data, unmounting the focused button. Per standard browser behavior, focus then falls back to <body>. On the next Tab, document.activeElement is body, matching none of the three checked values, so neither branch fires preventDefault(), and the browser's default Tab handling takes over — breaking the trap.
VERDICT: Minor issues
web/src/editor/ObjectShape.tsx:79 — The accessible label derives the kind text with object.kind.replace(/_/g, ' ') instead of the canonical kindDef(kind)?.label (already available via the kindDef/objectDisplayName import one line above). This produces wrong/inconsistent kind names for a screen-reader user: in_ground → "in ground" instead of the app's real label "In-ground", and every other kind loses its title case ("grow bag" vs "Grow bag", "tree" vs "Tree", etc.). Fix: use kindDef(object.kind)?.label ?? object.kind.
web/src/components/ui/Modal.tsx:49 — The Tab-trap checks active === first/last/card but not the case where the previously-focused element was removed from the DOM (focus reverts to <body> per browser default). ShareGardenModal.tsx:117-127's per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press finds document.activeElement === body, which matches none of the checked branches — no preventDefault() fires, and the browser's default Tab handling walks focus out of the dialog, defeating the trap. Fix: also listen for focusin on document (or treat "active not contained in card" as the fallback condition) and refocus the card/first item.
🧹 Code cleanliness & maintainability — Minor issues
Confirmed — Inspector.tsx:124 and Palette.tsx:45 both use kindDef(kind)?.label as the canonical human-readable kind label, which is exactly what the draft claims. The finding holds against the actual code.
VERDICT: Minor issues
web/src/editor/ObjectShape.tsx:79 — the new accessible label hand-formats the kind via object.kind.replace(/_/g, ' '), producing raw slugs like "in ground" / "grow bag". But kinds.ts (already imported one line above for objectDisplayName) exports kindDef(kind)?.label, the canonical human label used elsewhere in the editor (Inspector.tsx:124, Palette.tsx:45 both use kindDef(...)?.label ?? ... — see web/src/editor/kinds.ts:18-25 for the actual label strings, e.g. "In-ground", "Grow bag"). This reinvents formatting that already exists in the very module being imported from, producing a slightly worse/inconsistent string for screen reader users than the rest of the UI uses. Fix: `${objectDisplayName(object)}, ${kindDef(object.kind)?.label ?? object.kind}` (import kindDef alongside objectDisplayName).
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, quadratic behavior).
Modal.tsx:37-42 — focusable() runs a querySelectorAll scoped to the dialog card on every Tab keypress. Verified the modal's card subtree is a small confirmation dialog (a handful of controls at most, per the PR's blast-radius list: DeleteGarden/ClearBed/DeletePlant/DeleteSeedLot/Share). Re-querying per keypress on a bounded, tiny DOM subtree is not a meaningful cost.
ObjectShape.tsx:79 — label (string concat + objectDisplayName + replace) is recomputed on every render of every object. Read the component: it's wrapped in React.memo (ObjectShape.tsx:39), so this only runs when an object's own props actually change, not on every canvas pan/zoom/select. The regex replace is O(string length) on a short kind name — negligible even at garden-scale object counts.
GardenCanvas.tsx:238-244 — role, aria-label (template string), and <title> added to the SVG root are computed once per canvas render, not per-object; no loop involvement.
styles/index.css additions are static CSS rules, no runtime cost.
No N+1 patterns, no new work inside rendered.map(...) (GardenCanvas.tsx:259-263, unchanged by this diff), no new unbounded state, no blocking calls introduced. This PR is attribute/CSS/event-wiring only — nothing here changes asymptotic behavior or adds hot-path cost.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/components/ui/Modal.tsx:71-74 — The cleanup's opener?.focus?.() doesn't check whether opener is still attached to the document before restoring focus. Verified end-to-end: DeleteGardenModal (web/src/components/gardens/DeleteGardenModal.tsx:16-17) calls onClose() right after the delete mutation succeeds; useDeleteGarden (web/src/lib/gardens.ts:133-138) invalidates the gardens query on success, which refetches the list and eventually unmounts the GardenCard row — including the "Delete" <button> (web/src/components/gardens/GardenCard.tsx:63-65) that was document.activeElement when the dialog opened (i.e., opener). If that button is detached from the DOM by the time Modal's cleanup runs, .focus() on it is a silent no-op (focusing steps require the element be connected), and focus falls back to <body> — the "loses their place" failure the new code's own comment (line 32) says it's preventing. ClearBedModal (web/src/editor/ClearBedModal.tsx:37) follows the identical trigger-button-in-a-list → mutate → onClose pattern and is equally exposed. The code has no isConnected guard at all, so this is a real gap regardless of exact race timing in any one flow — suggest checking opener?.isConnected and falling back to a stable container (e.g. the list's own heading, or document.body) when the opener is gone.
Everything else in the diff — the Tab-trap boundary logic in Modal.tsx (verified: correctly handles empty-focusable-list, single-item, and Shift+Tab-from-card cases), and the Enter/Space handler added in ObjectShape.tsx:65-71 — checks out against the source and introduces no new unhandled-error paths.
Through the security lens, I checked for injection, untrusted-data handling, and authz concerns:
aria-label / <title> construction (ObjectShape.tsx:79, GardenCanvas.tsx): built from objectDisplayName(object), object.kind, and garden.name — all user-authored garden data. These flow into React JSX attributes/text (aria-label={label}, <title>{garden.name}</title>), which React escapes by default. No dangerouslySetInnerHTML, no attribute-breakout vector. object.kind is run through .replace(/_/g, ' ') but still lands in an escaped attribute. No XSS path.
Modal focus trap (Modal.tsx:37-42): querySelectorAll uses a static selector literal — no user/external input taints the selector, so no selector-injection / DOM-based issue. document.activeElement capture and opener?.focus?.() cleanup operate on already-focused DOM elements, no untrusted data.
No new network calls, deserialization, secret handling, or authz boundaries touched by this change — it's purely client-side a11y/UI.
I verified objectDisplayName returns plain string fields (kinds.ts:32-34) and confirmed the render sites use normal JSX escaping.
🎯 Correctness — Minor issues
Both findings verified against the actual source.
Finding 1 — Confirmed. ObjectShape.tsx:79 builds the label as ${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}. objectDisplayName (kinds.ts:32-34) returns o.name || kindDef(o.kind)?.label || 'Object', but the second segment is the raw object.kind slug with underscores replaced — not kindDef(object.kind)?.label. Reading the full OBJECT_KINDS table (kinds.ts:17-25): the kind slugs are bed, grow_bag, container, in_ground, tree, path, structure, with labels Bed, Grow bag, Container, etc. So a named bed reads "North Bed, bed" and an unnamed one reads "Bed, bed" (redundant) — never the "North Bed, raised bed" the comment promises. The friendly label is readily available via kindDef(object.kind)?.label. Upgrading confidence to high since I directly verified the label table.
Finding 2 — Confirmed. Modal.tsx:62-68: the Tab handler only wraps when active === first, active === last, or active === card. If the currently-focused control becomes :disabled mid-mutation (excluded by the :not([disabled]) selector in focusable()), the browser drops activeElement to <body>. A subsequent Tab/Shift+Tab then has active === body, matches neither branch, so preventDefault is never called and focus walks out behind the fixed inset-0 backdrop. Narrow (requires a focused control disabling itself while busy), but real.
The draft's third bullet explicitly states "Not a bug, no change required" and was not included in the findings JSON, so it correctly does not appear as a finding.
VERDICT: Minor issues
web/src/editor/ObjectShape.tsx:79 — accessible name's second segment is the raw kind slug, not the friendly label. The label is ${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}. objectDisplayName (kinds.ts:32-34) resolves to the name or kindDef(kind)?.label, but the second segment uses the bare slug (object.kind) with underscores swapped for spaces. Verified against OBJECT_KINDS (kinds.ts:17-25): slugs are bed, grow_bag, in_ground, etc., labels are Bed, Grow bag, In-ground. So a named bed reads "North Bed, bed" (redundant) and an unnamed one reads "Bed, bed" — not the "North Bed, raised bed" the inline comment and PR description claim. kindDef(object.kind)?.label is already available and would produce the documented behavior; alternatively drop the second segment when the display name already came from the label. Minor a11y labeling regression; low user impact.
web/src/components/ui/Modal.tsx:62-68 — focus trap can leak when the focused control disables itself during busy. The wrap branches only match active === first, active === last, or active === card. focusable() (Modal.tsx:37-42) excludes :disabled controls, so if the focused button becomes disabled mid-mutation the browser moves activeElement to <body>; a following Tab/Shift+Tab then matches no branch, preventDefault is not called, and focus escapes behind the fixed inset-0 z-50 backdrop — the exact failure the trap exists to prevent. Narrow (only when a focused control disables itself during a pending mutation), but real. Suggested fix: in the else fallback, when active is not one of items and not card, redirect to first (Tab) / last (Shift+Tab).
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
Finding 1 — Accessible name bypasses the existing kindDef().label abstraction and produces a redundant name for unnamed objects (web/src/editor/ObjectShape.tsx:79)
Two related cleanliness problems, both confirmed against the actual source:
Duplicated humanization logic.web/src/editor/kinds.ts already curates a capitalized kind label map (KindDef.label: "Grow bag", "In-ground", "Raised bed"-style values), and objectDisplayName (kinds.ts:32-33) itself falls back to kindDef(o.kind)?.label. This line instead re-derives a human label from the raw object.kind string with .replace(/_/g, ' '), yielding lowercase, un-hyphenated forms ("grow bag", "in ground") that disagree with the curated labels shown everywhere else. The "humanize a kind" concern now lives in two places with two different outputs.
Redundant accessible name for unnamed objects.objectDisplayName returns o.name || kindDef(o.kind)?.label || 'Object', so an unnamed grow_bag gets objectDisplayName = "Grow bag" (line 33), and the full label becomes "Grow bag, grow bag" — the kind is stated twice in the screen-reader name. Unnamed objects are the common case right after placement.
Suggested fix — reuse the existing abstraction for the kind suffix and drop the redundant part when the display name is already the kind label:
Everything else in the diff is clean: the Modal focus-trap is self-contained and follows the existing ref-based effect pattern; the focusable() selector is a standard a11y query; the CSS :focus-visible rule is narrowly scoped and well-commented; the object-shape class name is reasonable.
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, I checked the two hot-path-relevant spots:
Modal focusable() on each Tab (web/src/components/ui/Modal.tsx:37-42,53): querySelectorAll + Array.from runs on every Tab keydown. Tab is an infrequent, user-paced event and the dialog contains only a handful of controls, so the cost is negligible. No caching needed.
Per-render string/regex in ObjectShape (web/src/editor/ObjectShape.tsx:79): object.kind.replace(/_/g, ' ') and the template string allocate per render, but ObjectShape is wrapped in memo, so this only runs when an object's props actually change — not on every canvas frame/parent re-render. Not material.
The canvas change adds a tabIndex={0} per object (linear in object count), but that's tab-stop semantics, not a runtime cost, and is bounded by the garden's object list. No N+1, no unbounded growth, no per-frame work introduced. Clean from a performance standpoint.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
I verified both findings against the actual checked-out web/src/components/ui/Modal.tsx. The diff matches the source, and both edge cases hold up.
web/src/components/ui/Modal.tsx:73 — opener is captured once at mount (line 33) as document.activeElement and the cleanup calls opener?.focus?.(). The ref is never re-resolved, and there is no isConnected guard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out), .focus() on a detached node is a silent no-op and focus falls to <body> — exactly the loss the PR set out to fix. Suggested fix: guard with opener?.isConnected && opener.focus() and, when not connected, fall back to focusing card (or a documented restore target).
web/src/components/ui/Modal.tsx:61-68 — the Tab-wrap logic only fires when active === first (Shift), active === card (Shift), or active === last (non-Shift). If document.activeElement is none of those — e.g. focus escaped to <body> or an element behind the backdrop because something stole focus after mount, or focus landed on a [tabindex="-1"] child that isn't the card — neither branch executes, preventDefault is not called, and Tab walks out of the dialog. The comment claims it "pull[s] a stray focus … back to a real control," but it only covers the card stray case, not an arbitrary external activeElement. Suggested fix: when active is not within items (and not the card), explicitly refocus first (forward) or last (Shift) instead of falling through.
Both are edge cases rather than crashes; the happy paths (including empty-items and Escape-while-busy) are handled correctly.
I examined the diff through the Security lens and found no authn/authz gaps, injection vectors, secret leakage, or unsafe handling of untrusted data.
XSS via aria-label / <title>: The user-editable garden.name and object.name flow into React JSX props (aria-label, <title>, aria-pressed). React's normal attribute escaping applies; there is no dangerouslySetInnerHTML or raw HTML insertion.
DOM injection via querySelectorAll: The focus-trap selector in Modal.tsx is a hardcoded string with no user-controlled interpolation.
Kind string injection:object.kind.replace(/_/g, ' ') operates on a closed enum (OBJECT_KINDS); the regex is safe and the output is used only as a React text prop.
Focus restoration (opener?.focus?.()): No security boundary is violated; this is an accessibility affordance, not an authentication/authorization mechanism.
🎯 Correctness — Minor issues
Verdict: Minor issues
web/src/components/ui/Modal.tsx:40 — The focusable() selector uses input:not([disabled]), which includes input[type="hidden"]. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (active === first / active === last) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button will not be prevented, and focus walks out behind the backdrop. Fix: change the selector clause to input:not([disabled]):not([type="hidden"]).
web/src/editor/ObjectShape.tsx:96 — aria-pressed={selected} on role="button" advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (handleKey) always calls onSelect(object.id); there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. Fix: remove aria-pressed. If the selectable state must be exposed, use aria-selected with an appropriate role (e.g. option inside a listbox, or gridcell) rather than misusing a toggle button state.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/ObjectShape.tsx:79 — The accessible label builds the kind display string with object.kind.replace(/_/g, ' '), duplicating a concern that kindDef() in ./kinds.ts already solves. kindDef returns curated, Title Case labels (e.g. "Grow bag", "In-ground") while the regex produces lowercase, unhyphenated output ("grow bag", "in ground"). This creates an inconsistency with the rest of the UI (e.g. JournalPanel uses objectDisplayName, which respects kindDef labels) and means new kinds or label edits in KindDef won't propagate here. Fix: Import kindDef and use kindDef(object.kind)?.label ?? object.kind for the second half of the label.
No other material maintainability issues found.
⚡ Performance — No material issues found
No material issues found
The changes are accessibility-focused and don't introduce material performance regressions. Specifically:
Modal.tsx:37-42 — focusable() runs querySelectorAll on every Tab keypress to build the trap list. This is O(n) on the modal's DOM subtree, but typical modals here have < 20 focusable controls, so the cost per tab is negligible. Not worth caching.
ObjectShape.tsx:79 — object.kind.replace(/_/g, ' ') and objectDisplayName(object) run on each render, but they're trivial string ops on small inputs and the component is already wrapped in memo, so it only re-renders when props change.
SVG focusables — Adding tabIndex={0} to every garden object is an accessibility/UX pattern, not a rendering performance concern; the DOM node count is unchanged from before (the <g> elements already existed).
No allocations in hot loops, no N+1 patterns, no missing pagination, and no work moved onto critical paths.
🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the diff through the error-handling & edge-cases lens and could not identify a material problem introduced by these changes.
What I checked:
Modal focus trap (Modal.tsx) — The focusable() query re-runs on every Tab keydown, so dynamically added/removed controls are picked up. The items.length === 0 branch prevents default and pulls focus back to the card. Shift+Tab from the card itself (which gets initial focus via tabIndex={-1}) is explicitly wrapped to last. Forward Tab from the card is not explicitly trapped, but standard browser behavior moves focus from a tabindex="-1" container to its first focusable descendant, so the trap is intact in practice (and the author verified this with real keyboard tests). The opener?.focus?.() cleanup safely restores focus on unmount; the optional chaining handles detached elements and SVG <g> openers gracefully.
Object keyboard selection (ObjectShape.tsx) — handleKey defensively calls preventDefault() + stopPropagation() for Enter/Space, preventing scroll or bubbled actions. aria-pressed receives a boolean that React stringifies correctly. The label construction uses objectDisplayName, which has a safe fallback chain (name → kind label → "Object"). tabIndex={0} is applied unconditionally; there is no null-guard needed because the prop type guarantees object is present.
Canvas accessibility (GardenCanvas.tsx) — garden.name is interpolated into aria-label and <title>; JSX escaping prevents injection issues. No runtime null risk because garden is a required prop.
CSS focus ring (index.css) — The :focus-visible rule is progressive enhancement; the outline: none removal is acceptable because the replacement is confirmed working in the browsers the author tested, and the native fallback (default outline on SVG <g>) was unreliable anyway.
I also verified that the global arrow-key nudge handler in GardenEditorPage is not newly broken by this PR—it already existed and its document.activeElement whitelist simply doesn’t cover SVG <g> elements, which is correct because we want nudging to work when an object has focus. No new crash path or swallowed error is introduced.
The aria-label is built from objectDisplayName(object) and object.kind. The kind comes from a controlled set (the API validates against an enum). Let me check whether kind is user-controlled and could inject anything, though aria-labels are text-only (no HTML injection risk in React).
React escapes aria-label text content, so no HTML/script injection vector there.
Now let me check the security angle of the Modal's document.activeElement opener capture and focus restoration. The opener is captured on mount and refocused on cleanup. Is there a scenario where the opener element gets removed from the DOM (e.g. the trigger button unmounts), leaving a stale ref? opener?.focus?.() uses optional chaining — if opener was detached, .focus() on a detached element is a no-op (no throw). Not a security issue, just UX.
Let me consider whether there's any authz/authn relevance. This is a frontend-only a11y change. No new API surfaces, no input handling changes that touch the backend. The onSelect(object.id) call passes an object id — let me verify the nudge handler that consumes selection does permission checking via the service layer (per CLAUDE.md the service is the seam). Verdict: No material issues found
This is a frontend-only accessibility change. Through the security lens I checked:
No new tainted data flows. The aria-labels are built from objectDisplayName(object) + object.kind (ObjectShape.tsx:79) and garden.name (GardenCanvas.tsx:242). All are rendered as React text content / attributes — React escapes them, so there is no HTML/script injection vector. object.kind is constrained to a fixed set (the store has a CHECK constraint; the service validates), and object.name/garden.name are display-only strings here.
No injection in the focus-trap selector.Modal.tsx:40 uses a static string literal for querySelectorAll — no user input is interpolated into the selector.
No authz/authn surface touched.onSelect(object.id) only sets selectedId in the client store; the actual mutations (nudge/move/resize) flow through the existing service layer, which is the permission seam per the architecture. Selection alone is not an authorization boundary.
Opener focus restoration (Modal.tsx:33,73) captures document.activeElement on mount and refocuses it on cleanup. No credential/secret exposure; the worst case is a no-op .focus() on a detached element, which is harmless.
🎯 Correctness — No material issues found
I'll verify the diff against the actual checked-out code, focusing on correctness.
Now let me check the arrow-key nudge handler that the PR description references, and verify the objectDisplayName/label logic edge cases.
The arrow-key nudge handler the PR claims "already exists" — let me find it to verify it actually acts on the selection made by Enter/Space.
The nudge handler reads s.selectedId from the store. Let me verify onSelect actually sets selectedId so Enter → nudge chain works.
Good — onSelect is select, which sets selectedId. Enter → onSelect → selectedId set → arrow keys act on it. The chain works.
Now let me check the Escape handler interaction: the canvas Escape peels layers (line 163-175), and there's a new onKeyDown on object <g> that only handles Enter/Space (stopPropagation on those). But does Escape from a focused SVG element get caught by the window handler correctly? Yes — handleKey doesn't touch Escape, so it bubbles to window. Fine.
Let me check the Modal focus-trap edge: when the dialog opens, card.focus() is called but document.activeElement is captured before that — good. But what if the opener is inside an element that gets unmounted while the modal is open (common in React with conditional rendering)? Then opener.focus() on a detached element silently does nothing → focus lands on <body>. Let me check a couple of the call sites to see if the trigger button survives.
Now let me check the callers of these modals to see whether the opener button stays mounted while the modal is open. The key concern: if the trigger is conditionally rendered and unmounts when modalOpen becomes true, the restore-focus fails. Let me check one caller pattern.
The trigger buttons (GardenCard, the clear-bed button in the editor) stay mounted while the modal is open — the modal is rendered as a sibling, not replacing the trigger. So opener.focus() will work in these cases. Good.
Now let me verify one important correctness concern with the focus trap: the focusable() selector includes [tabindex]:not([tabindex="-1"]), which will match the card itself (it has tabIndex={-1}) — no, it excludes -1. But the object <g> elements with tabIndex={0} are not inside the modal, so fine. Let me check whether the trap correctly handles the case where focus is on the card (tabIndex=-1) and there ARE focusable items.
Looking at the logic: if active === card and shift+tab → wrap to last. But if active === card and plain Tab (no shift), the condition !e.shiftKey && active === last is false, so no preventDefault — the browser moves focus to the first focusable naturally. That's correct behavior.
Let me check the GardenCanvas <title> element placement — in SVG, <title> should be the first child of the element it labels.
Now let me look at a potential correctness concern with the nudge handler's input-guard. The window-level nudge handler (line 207-208) bails if activeElement is an INPUT/TEXTAREA/SELECT. But now a focused SVG <g> is activeElement when the user presses arrow keys. That's not in the bail list, so the nudge proceeds — which is exactly what we want. Good.
But wait — consider the reverse: a user is typing in an input field (e.g. the Inspector's name input) and presses Enter/Space. The object's onKeyDown is on the <g>, so it won't fire when focus is in an input. Fine. And the nudge handler bails on inputs. Fine.
Now, one concern: the new handleKey calls e.stopPropagation() for Enter/Space. The nudge handler is on window (capture not used, so bubble phase). Enter/Space on a focused <g> → handleKey fires, stopPropagation prevents it reaching window. But the nudge handler only cares about arrow keys, not Enter/Space, so stopPropagation is harmless there. However, the Escape handler (line 163-175) is also on window. If a user presses Escape while a <g> is focused, handleKey does nothing (no match), Escape bubbles to window, and the peel-back runs — deselecting. That's reasonable.
Let me double-check the aria-pressed semantics. aria-pressed on a role="button" indicates toggle state. Here selected maps to pressed. The button represents the object; "pressed" = selected. This is a slightly loose use of aria-pressed (the object isn't really a toggle), but it's a reasonable mapping and not a correctness bug. Not reporting.
Let me verify the CSS :focus-visible rule actually overrides the inline stroke on the shape. CSS specificity: .object-shape:focus-visible :is(rect, ellipse) is a class + pseudo-class + descendant selector. Inline stroke attribute is a presentation attribute, which has lower specificity than any CSS rule. So the CSS wins. Verified correct.
Now let me check one thing about the focus trap: the onKey is on document (bubble). If a Modal is open AND the GardenCanvas is behind it, and the user presses Tab while focus is on a canvas <g> that somehow... no, the modal is an overlay, the canvas isn't focusable through the backdrop. Not a concern.
Let me check whether multiple Modals can be open at once (nested), which would break the single keydown listener assumption.
No nested modals in the current code — each page renders at most one. The single-listener assumption holds.
Now let me look at the SeedLotModal which is a form modal (not in the PR's blast radius list but uses Modal) — does it have a form with inputs that the trap now affects? The trap wraps Tab among inputs. That's normal dialog behavior and correct.
Now, one correctness concern with the focus trap and forms: when the form has a submit button and the user is on the last input, Tab moves to the first item (wrapping). That's correct. But what about the case where active is neither first, last, nor card (e.g. focus is on a middle input) — no preventDefault, browser handles Tab normally. Correct.
Let me verify the objectDisplayName returns something sensible for the label. For an unnamed object: o.name || kindDef(o.kind)?.label || 'Object'. Then label = "Object, bed" for an unnamed bed? No — kindDef('bed')?.label = 'Bed', so it'd be "Bed, bed". That's redundant but not a correctness bug. For a named object "North Bed" of kind raised_bed... wait, is raised_bed a kind? Let me check the kinds. From kinds.ts, the kinds are: bed, grow_bag, container, in_ground, tree, path, structure. There's no raised_bed. The PR comment says "North Bed, raised bed" — but the kind is bed, so it'd be "North Bed, bed". The comment example is slightly off but the code is correct relative to the actual kinds. Minor comment inaccuracy, not a bug.
Let me check the types to confirm object.kind and the EditorObject shape.
Now let me check the CSS file to verify the rule placement (inside @layer base?) and the --color-accent-strong token exists.
Token exists in both light and dark. Good.
Now one more correctness concern: stroke-dasharray: 5 4 as a CSS value on an SVG shape with vector-effect="non-scaling-stroke". The dash values are in user units (cm in the world <g> since the parent transform scales). Without non-scaling-stroke, dashes would scale with the viewport. With non-scaling-stroke, the stroke width is in screen px but stroke-dasharray is still in user units... actually the dash array behavior with non-scaling-stroke is browser-dependent and can render unevenly at different zoom levels. This is a visual fidelity concern, not a correctness bug in the keyboard-reachability feature itself. I won't report it as material.
Let me verify the most important claim of the PR: that Enter/Space on a focused object actually results in the arrow-key nudge working. I've confirmed:
onSelect = select from the store (GardenCanvas line 261)
select sets selectedId (store line 86)
The nudge handler checks s.selectedId != null (line 212) and acts on it
The nudge handler bails only if activeElement is INPUT/TEXTAREA/SELECT — a focused <g> passes through
The chain is sound.
Now let me check one subtle issue: when the user tabs to an object and presses Enter, handleKey calls e.stopPropagation(). But does the window-level nudge keydown listener also listen for Enter? No — it only checks DIRS[e.key] (arrows). So no conflict. And the Escape handler only checks e.key === 'Escape'. No conflict.
But there's a real concern: after Enter selects the object, focus remains on the <g>. The user presses arrow keys. The nudge handler fires on window, reads s.selectedId, moves the object. Good. But e.preventDefault() is called in the nudge handler (line 215). Arrow keys on a focused element might also trigger scrolling — preventDefault stops that. Fine.
One more: does pressing Space on a focused <g> scroll the page? handleKey calls e.preventDefault() for Space, so no. Good.
Let me verify there are no existing tests that this change might break and that the claimed tests pass.
No Modal/object-shape tests exist, so no test breakage from the behavior change. The claimed 87 vitest tests are the lib tests.
I've thoroughly verified the change through the correctness lens. Let me summarize.
VERDICT: No material issues found
I verified the core correctness chain the PR depends on:
Enter/Space → nudge chain works: ObjectShape.onSelect is wired to the store's select (GardenCanvas.tsx:261), which sets selectedId (store.ts:86). The arrow-key nudge handler (GardenEditorPage.tsx:201-222) reads s.selectedId and acts on it, and its input-guard (tagName === INPUT/TEXTAREA/SELECT, line 207-208) does NOT bail on a focused SVG <g>, so a keyboard-selected object is nudgable. The PR's central claim holds.
handleKey stopPropagation is safe: it only stops Enter/Space; the window-level handlers only care about Escape and arrow keys respectively, so no listener is starved of events it needs. Escape from a focused <g> correctly bubbles to the peel-back handler.
Focus trap logic is correct: the wrap conditions (shift+tab on first/card → last; plain tab on last → first) and the empty-dialog fallback are sound. Middle-item Tab is left to the browser. The focusable() selector excludes disabled controls and tabindex="-1" (so the card itself, tabIndex={-1}, is correctly excluded from the wrap set but still handled as a stray-focus origin).
Opener restore works for the listed blast radius: all five named modals (DeleteGarden, ClearBed, DeletePlant, DeleteSeedLot, Share) are rendered as siblings of their trigger buttons, which stay mounted while the modal is open, so opener.focus() resolves to a live element. No nested-modals case exists in the current code, so the single document keydown listener is unambiguous.
:focus-visible CSS overrides inline stroke: presentation attributes have lower specificity than any CSS rule, so .object-shape:focus-visible :is(rect, ellipse) { stroke: ... } wins over the inline stroke={stroke}. The --color-accent-strong token exists in both light and dark themes (index.css:13, index.css:56).
objectDisplayName import resolves and the label derivation is correct for the actual kinds (bed, grow_bag, etc. — there is no raised_bed kind, so the comment example "North Bed, raised bed" is slightly inaccurate, but the runtime string "North Bed, bed" is correct). This is a comment inaccuracy, not a behavior bug.
No correctness defects found in my lane.
Final review complete.
VERDICT: No material issues found
I verified the change through the correctness lens, checking the actual checked-out code rather than the diff in isolation:
Enter/Space → arrow-nudge chain (the PR's central claim): confirmed end-to-end. ObjectShape's onSelect is wired to the store's select (GardenCanvas.tsx:261 → store.ts:86 sets selectedId). The nudge handler (GardenEditorPage.tsx:201-222) acts on s.selectedId, and its input-guard (line 207-208, bails only on INPUT/TEXTAREA/SELECT) does not reject a focused SVG <g>. So a keyboard-selected object is nudgable — the previously-unreachable feature now works.
handleKey stopPropagation is safe: it only intercepts Enter/Space; the two window-level keydown listeners care only about Escape (GardenEditorPage.tsx:164) and arrow keys (:201) respectively. No listener is starved. Escape from a focused <g> bubbles correctly to the peel-back handler.
Focus trap logic is sound: wrap conditions handle shift+tab on first/card → last and plain tab on last → first; middle-item Tab is left to the browser; empty-dialog falls back to the card. The focusable() selector correctly excludes tabindex="-1" (so the card, tabIndex={-1}, is excluded from the wrap set but still handled as a stray-focus origin) and disabled controls.
Opener restore works for the stated blast radius: all five named modals are rendered as siblings of their trigger buttons (e.g. GardensPage.tsx:55-73, GardenEditorPage.tsx:527-534), which remain mounted while the modal is open, so opener.focus() resolves to a live element. No nested-modal pattern exists in the current code, so the single document keydown listener is unambiguous.
:focus-visible CSS overrides inline stroke: SVG presentation attributes have lower specificity than any CSS rule, so .object-shape:focus-visible :is(rect, ellipse) { stroke: ... } wins over the inline stroke={stroke}. The --color-accent-strong token exists in both light (index.css:13) and dark (index.css:56) themes.
objectDisplayName import resolves (kinds.ts:32); the label derivation is correct for the actual kinds (bed, grow_bag, …). Minor: the code comment's example "North Bed, raised bed" is inaccurate (there is no raised_bed kind; the real output is "North Bed, bed"), but this is a comment nit, not a behavior bug — not material.
No correctness defects in my lane.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/editor/ObjectShape.tsx:79 — the accessible-name suffix rebuilds the kind label with object.kind.replace(/_/g, ' '), duplicating the canonical kind→human-label mapping that already lives in kinds.ts (KindDef.label, surfaced via kindDef()). The two diverge in practice: in_ground renders here as "in ground" while kindDef('in_ground').label is "In-ground" (hyphen, capitalized); grow_bag renders as "grow bag" vs "Grow bag". Every other call site (Inspector.tsx:124, Palette.tsx:45) reads kindDef(...)?.label; this is the only one that re-derives it. Suggested fix: const label = \${objectDisplayName(object)}, ${kindDef(object.kind)?.label ?? object.kind.replace(/_/g, ' ')}`— use the single source of truth, keep the replace only as a fallback for unknown kinds. (Verified by readingkinds.ts:18-24` and the other call sites via grep.)
web/src/components/ui/Modal.tsx:40 — the focusable-element selector is a long inline string literal re-evaluated by querySelectorAll on every Tab keypress. Hoisting it to a module-level constant would make the intent ("the dialog's focusable controls") named and self-documenting, and matches how the rest of the file keeps its keydown logic readable. Trivial, but it's the kind of magic string that quietly rots (e.g. someone adds a [contenteditable] and has to find it here).
No other cleanliness concerns. The CSS-vs-React choice for the focus ring is well commented, the ObjectShape changes follow the surrounding component structure, and the Modal focus-trap reads cleanly.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed the diff through the performance lens. Verified against the checked-out code.
Modal.tsx:37-42 — focusable() calls querySelectorAll on each Tab keydown. This runs only inside a modal dialog (a handful of elements, triggered by an occasional Tab press), not on any hot path. Negligible.
ObjectShape.tsx:79 — object.kind.replace(/_/g, ' ') and the label template run on each render, but the component is memo'd (line 39), so re-renders only happen when an object's props actually change, not on every pan/zoom. No per-frame cost.
ObjectShape.tsx:88-98 — new props/attributes added (role, tabIndex, aria-label, aria-pressed, className, onKeyDown) are constant-ish per render; no new allocations on a hot path.
styles/index.css.object-shape:focus-visible :is(rect, ellipse) — a descendant-selector CSS rule; browsers match this cheaply and it only applies under :focus-visible (at most one element). No concern.
No efficiency regressions, unbounded growth, or hot-path work introduced.
🧯 Error handling & edge cases — No material issues found
I'll review the diff through the error-handling & edge-cases lens, focusing on the unhappy paths introduced.
Let me check how the modal handles the busy case and the cleanup ordering issue, and verify the Space key handling interaction with scrolling.
Now let me examine the key edge cases. The Space key handling is important — when a focusable SVG <g> has tabIndex={0}, pressing Space normally triggers a click on buttons but <g> is not a native button. The handler calls e.preventDefault() which prevents page scroll. Good. But there's a subtle issue: the handleKey only handles Enter/Space for selection, but arrow-key nudge is mentioned as the follow-on. Let me check where arrow keys are handled.
Now let me understand a key edge case in the Modal focus trap. The onKey handler for the Modal is attached to document. The nudge handler in GardenEditorPage is attached to window. Both fire on Tab... actually nudge only handles arrows. But the Modal's keydown on document and GardenEditor's keydown on window — when a Modal is open over the editor, do they conflict? The nudge handler returns early on non-arrow keys, so no Tab conflict. But there's a more important edge case.
Let me look at the focus restoration ordering issue — the cleanup runs opener?.focus?.() but what if opener was removed from the DOM (e.g., the trigger button unmounted)? Also, let me check the active === card case in the Tab trap.
Now let me examine the key edge case in the Modal focus trap. The critical concern is the busy state interaction with Escape and the Tab trap. When busy is true, Escape is blocked (good), but Tab trapping still works. Let me check one important edge case: the ShareGardenModal has a list that can be empty or populated dynamically — the focusable() list is recomputed each Tab press, so that's fine.
But there's a real edge case: the focusable() selector includes [tabindex]:not([tabindex="-1"]). The card itself has tabIndex={-1}, so it won't match. But what about the case where focus is on an element NOT in the items list and NOT the card — e.g., focus somehow escaped to the backdrop or body? Let me think about this.
If active is neither first, last, nor card, then neither branch fires and Tab proceeds normally — which could walk out of the dialog. This is a real gap but it's an edge case that's hard to hit in practice since focus starts on card.
Let me check a more concrete issue: the Space key in handleKey. When e.key === ' ' (Space) on a role="button" element, the native semantics would also fire a click. But since this is an SVG <g> with role="button" and tabIndex={0}, browsers may synthesize a click on Space for ARIA buttons. Let me verify whether e.preventDefault() in onKeyDown prevents the synthetic click.
Now let me think about the actual edge cases in my lens. The most important one: the Modal cleanup restores focus to opener unconditionally, even when the modal closed because the opener itself was destroyed. For example, in DeleteGardenModal, after successful deletion, onClose() is called. The opener (the "Delete" menu button that opened the modal) may have unmounted because the garden row was removed from the list. Calling .focus() on a detached element is a no-op (no error), so focus falls to <body>. That's the same problem the PR is trying to fix, but it's pre-existing behavior in a new edge case. Not a regression — the old code also left focus on <body>. So this is acceptable.
Let me check a more concrete issue: the opener capture happens at mount time. document.activeElement at the moment the effect runs — is it guaranteed to be the opener? React effects run after render/paint. If the modal is opened by a button click, the button is still activeElement when the effect runs (focus hasn't moved yet, since card?.focus() happens right after). This is correct.
But there's a subtle issue: if the modal is opened programmatically (not from a focused element), document.activeElement could be <body> or the modal's trigger might not be focused. Then on close, focus goes to <body> — which is the same as before, no regression.
Let me look at one more edge case: the items.length === 0 branch in the Tab trap. If there are no focusable elements, it preventDefaults and focuses the card. But the card has tabIndex={-1}, so it CAN receive focus via .focus(). Good. But this means Tab is completely trapped with no way out except Escape — which is the correct behavior for a modal.
Now let me check the most important error-handling concern: what happens when cardRef.current is null (the card variable is null)? The focusable() function uses card?.querySelectorAll(...) which returns undefined, and Array.from(undefined ?? []) → Array.from([]) → []. So items.length === 0, it calls card?.focus() which is a no-op. No crash. Good defensive handling.
Let me check one final thing — the Escape handler change. Previously: if (e.key === 'Escape' && !busyRef.current) onCloseRef.current(). Now it adds return after. The return is important — without it, an Escape would fall through to the Tab check, but since e.key === 'Escape' !== 'Tab', the if (e.key !== 'Tab') return would catch it anyway. So the new return is redundant but harmless. No issue.
Let me verify the Space key preventDefault doesn't break scrolling in the canvas context.
Now I have a clear picture. Let me verify one final concern about the Space key on SVG <g> with role="button". The handleKey calls e.preventDefault() and e.stopPropagation() then onSelect. For a native <button>, Space fires click on keyup, and preventDefault on keydown prevents it. But for an SVG <g> with ARIA role="button", browsers do NOT synthesize click events — there's no native button behavior on <g>. So onSelect in handleKey is the only path. The e.preventDefault() prevents page scroll on Space, which is the right behavior. No double-fire issue.
One subtle edge case worth checking: the handleKey fires onSelect on BOTH keydown for Enter and Space. For Enter, native buttons fire click on keydown (for buttons it's actually keyup but preventDefault on keydown stops it). For a <g>, there's no synthesis, so firing on keydown is fine and responsive. No issue.
Let me check whether the arrow-key nudge handler in GardenEditorPage has a conflict now that objects are focusable. When an object <g> has focus and the user presses an arrow key, the nudge handler on window fires. But it checks s.selectedId — the object must be selected (not just focused). So pressing arrows on a focused-but-not-selected object does nothing (the if (s.selectedId != null) branch doesn't execute, and selectedPlantingId is also null). The arrow key then does its default action — which for a role="button" element is nothing (no scroll since it's an SVG). This is fine, but let me verify there's no scroll-the-page issue.
Actually, arrows on a focusable SVG element inside role="application" — the application role tells AT to pass keystrokes through. Arrow keys on a focused element inside role="application" would not scroll the page by default in most browsers since the SVG fills the container. This is acceptable.
I've completed my investigation. The diff is clean from an error-handling/edge-case perspective. The defensive null-handling in the Modal (card?.focus(), Array.from(... ?? [])) is correct. The items.length === 0 branch is handled. The Space key preventDefault correctly prevents page scroll. No panics, no swallowed errors, no missing cleanup.
VERDICT: No material issues found
Modal focus trap (Modal.tsx:54-58): The items.length === 0 branch is handled — card?.focus() is a safe no-op if cardRef.current is null, and Array.from(... ?? []) guards the null querySelectorAll result. Verified by reading the full effect.
Opener capture (Modal.tsx:33): document.activeElement is cast to HTMLElement | null and the cleanup uses opener?.focus?.() (optional chaining + optional call), so a null or non-HTMLElement opener can't crash. If the opener was unmounted (e.g. the garden row removed after delete), .focus() on a detached element is a silent no-op — not a regression (old behavior also left focus on <body>). Verified.
Space key (ObjectShape.tsx:65-71): e.preventDefault() stops the page-scroll default; SVG <g> has no native button semantics so there's no synthesized click to double-fire. Verified there is no onClick on ObjectShape that could conflict.
Tab trap mid-list gap (Modal.tsx:62-68): if document.activeElement is neither first, last, nor card (focus somehow escaped to the backdrop), neither branch fires and Tab is not trapped. This is a theoretical gap but not reachable from the normal flow (focus starts on card and the trap keeps it inside), so it's not a material defect.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
The diff is entirely frontend accessibility work: adding role, aria-label, tabIndex, keyboard handlers, CSS focus rings, and a modal focus trap. From a security lens, there is no new attack surface introduced.
No authn/authz changes.
No new injection points: garden.name and object.name are inserted via React JSX into aria-label and <title>, which are properly escaped by React's runtime. They were already rendered as DOM text before this change.
No SSRF, deserialization, secret leakage, or insecure defaults.
The keyboard handler in ObjectShape only responds to Enter/Space and calls the existing onSelect with a numeric id.
The Modal focus trap uses standard querySelectorAll selectors and preventDefault on Tab for wrapping — expected, correct behavior.
🎯 Correctness — Minor issues
Minor issues
web/src/editor/ObjectShape.tsx:96 — aria-pressed={selected} on a non-toggle button. aria-pressed is specifically for toggle buttons that cycle state on activation. Clicking an already-selected object does not deselect it — select(object.id) keeps it selected. A screen-reader user hearing "pressed" will expect a second activation to unpress, which doesn't happen. Fix: Remove aria-pressed. The selection state is already communicated by the selection-stroke colour change and can be added to the aria-label if needed (e.g. aria-label={selected ? label + ', selected' : label}). If a programmatic selection indicator is desired, use aria-selected inside a container with a supporting role (listbox, grid, etc.) instead of misapplying aria-pressed.
web/src/components/ui/Modal.tsx:62 — Forward Tab from the initially-focused card container is not explicitly trapped.
The code handles active === card only for Shift+Tab (e.shiftKey), wrapping to last. If the card itself has focus (which it does on mount via card?.focus()) and the user presses Tab (forward), the handler does not intercept it. In practice most browsers move focus into the first child control, but this relies on native behaviour rather than an explicit guard. If the modal is rendered in DOM order before other focusable page elements, a forward Tab from the card could walk out into the page. Fix: Add active === card to the forward-Tab branch: else if (!e.shiftKey && (active === last || active === card)) { e.preventDefault(); first.focus() }.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The changes are clean and well-structured from a maintainability standpoint:
Modal.tsx keeps the focus-trap logic in a single, readable effect with a clear focusable() helper. Comments explain the DOM-order query and the focus-return behavior.
ObjectShape.tsx adds a concise handleKey handler and a neatly composed aria-label without bloating the component. The CSS-over-inline-stroke explanation in the comment is accurate and useful.
index.css scopes the new rule to .object-shape:focus-visible :is(rect, ellipse) — a tidy, low-specificity selector that won't leak elsewhere.
No duplicated code, no confusing names, no functions that have grown too long, and no deviations from the patterns already in use.
⚡ Performance — No material issues found
No material issues found
The changes are lightweight accessibility additions with no material performance regressions. Specifically:
Modal focus trap: focusable() does a small querySelectorAll on every Tab keydown, but the selector scope is a single dialog card with typically 2–3 controls. This is negligible and not inside a hot loop.
ObjectShape aria-label computation: objectDisplayName() plus a trivial replace run per object render. The component is memo'd and the parent opacity wrapper already causes re-renders on focus changes — the added work is O(1) per object and dwarfed by the SVG render itself.
New keyboard handlers: onKeyDown on focused SVG <g> elements is handled via React's synthetic event delegation; no extra DOM listeners are attached.
No allocation or quadratic behavior introduced: The diff does not add new arrays, maps, or nested loops over collections inside render paths.
🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the PR through the Error handling & edge cases lens. Here is what I verified and why it is clean:
I read the full file. The focusable() query re-runs on every Tab keydown, so dynamically added/removed controls are handled. The items.length === 0 branch correctly traps Tab when the dialog has no interactive children. The opener capture and restoration (opener?.focus?.()) is safe: if the opener element has been removed from the DOM, focus() silently does nothing rather than throwing. The cleanup order (remove listener, then restore focus) is correct.
I verified the objectDisplayName utility in kinds.ts. The label string interpolation is safe: object.kind is typed string, and .replace() on an empty or unknown string does not throw. The handleKey only intercepts Enter and Space; it does not block arrow keys, which bubble up to the existing window-level nudge handler as intended. The component does not crash on a missing name — objectDisplayName falls back through kindDef to the literal "Object".
I read the nudge handler effect. It checks document.activeElement.tagName against INPUT, TEXTAREA, and SELECT. When an SVG <g> is focused, tagName is "G", so the nudge handler proceeds normally. There is no conflict with the new keyboard focus path; the nudge works exactly when an object is selected, regardless of whether the selection came from pointer or keyboard.
CSS focus indicator (web/src/styles/index.css)
The .object-shape:focus-visible :is(rect, ellipse) rule overrides inline presentation attributes via specificity, and outline: none is paired with the :focus-visible rule so keyboard users get a visible indicator while mouse users do not. This is the intended accessibility pattern.
No swallowed errors, missing cleanups, nil-pointer risks, off-by-one conditions, integer overflows, or unhandled edge cases were introduced by this diff.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 6 findings (5 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | Ad-hoc kind string formatting duplicates existing kindDef labels | `web/src/editor/ObjectShape.tsx:79` | 4/5 | correctness, maintainability |
| 🟠 | focusable() selector includes input[type="hidden"], breaking tab trap boundary when hidden inputs are at modal edges | `web/src/components/ui/Modal.tsx:40` | 2/5 | correctness, maintainability |
| 🟠 | Focus restore on modal close doesn't check if the opener element is still in the DOM, silently failing for the exact delete/clear flows the PR targets | `web/src/components/ui/Modal.tsx:73` | 2/5 | error-handling |
| 🟠 | aria-pressed misused on non-toggle role="button"; object selection is not a toggle action | `web/src/editor/ObjectShape.tsx:96` | 2/5 | correctness |
| 🟡 | Focus trap falls through (Tab escapes) when activeElement is outside {first, last, card} — external/stolen focus isn't pulled back in | `web/src/components/ui/Modal.tsx:61` | 2/5 | correctness, error-handling |
<details><summary>1 single-model finding (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Focus trap doesn't handle the focused element being removed from the DOM (e.g. ShareGardenModal's remove-share button), letting Tab escape the dialog | `web/src/components/ui/Modal.tsx:49` | claude-code/sonnet | correctness |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**VERDICT: No material issues found**
This diff is confined to frontend accessibility (ARIA attributes, SVG `role`/`tabIndex`, CSS `:focus-visible`, and a Tab-focus trap in a modal). I checked the specific spots most likely to hide a security issue for this kind of change:
- `garden.name`, `objectDisplayName(object)`, and `object.kind` are all interpolated into JSX `aria-label` attributes and an SVG `<title>` text node (`web/src/editor/GardenCanvas.tsx:242`, `:244`, `web/src/editor/ObjectShape.tsx:79`). These go through React's normal attribute/text rendering, which escapes them — no `dangerouslySetInnerHTML` or raw DOM writes exist anywhere in `web/src` (verified via grep), so there's no XSS vector even if a garden/object name contained markup.
- The Modal's new focus-trap (`web/src/components/ui/Modal.tsx:37-42`) scopes `querySelectorAll` to `card` (the dialog root), so a stray focusable element elsewhere on the page can't be pulled into the trap, and the `opener` restored on close is whatever `document.activeElement` already was in the same document at mount time — not attacker-influenceable.
- No new network calls, no new data persistence, no server-side code touched — this is pure client-side DOM/ARIA/CSS wiring.
Nothing here touches authn/authz, injection surfaces, untrusted-data handling, or secrets.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings confirmed against the actual source.
**Finding 1** (`ObjectShape.tsx:79`): Confirmed. `kindDef`/`objectDisplayName` are imported and `objectDisplayName` is used for the name half of the label, but the kind half uses `object.kind.replace(/_/g, ' ')` instead of `kindDef(object.kind)?.label`. `kinds.ts:18-24` shows real labels differ: `in_ground` → `"In-ground"` (label uses a hyphen, regex produces `"in ground"`), and all others are title-cased (`"Grow bag"` vs regex's `"grow bag"`).
**Finding 2** (`Modal.tsx:44-69`): Confirmed. The trap logic only checks `active === first`, `active === last`, or `active === card`. `ShareGardenModal.tsx:117-127` renders a "Remove" button inside the trapped dialog whose `onClick` removes that share from `shares.data`, unmounting the focused button. Per standard browser behavior, focus then falls back to `<body>`. On the next Tab, `document.activeElement` is `body`, matching none of the three checked values, so neither branch fires `preventDefault()`, and the browser's default Tab handling takes over — breaking the trap.
VERDICT: Minor issues
- `web/src/editor/ObjectShape.tsx:79` — The accessible label derives the kind text with `object.kind.replace(/_/g, ' ')` instead of the canonical `kindDef(kind)?.label` (already available via the `kindDef`/`objectDisplayName` import one line above). This produces wrong/inconsistent kind names for a screen-reader user: `in_ground` → `"in ground"` instead of the app's real label `"In-ground"`, and every other kind loses its title case (`"grow bag"` vs `"Grow bag"`, `"tree"` vs `"Tree"`, etc.). Fix: use `kindDef(object.kind)?.label ?? object.kind`.
- `web/src/components/ui/Modal.tsx:49` — The Tab-trap checks `active === first/last/card` but not the case where the previously-focused element was removed from the DOM (focus reverts to `<body>` per browser default). `ShareGardenModal.tsx:117-127`'s per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press finds `document.activeElement === body`, which matches none of the checked branches — no `preventDefault()` fires, and the browser's default Tab handling walks focus out of the dialog, defeating the trap. Fix: also listen for `focusin` on `document` (or treat "active not contained in card" as the fallback condition) and refocus the card/first item.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Confirmed — `Inspector.tsx:124` and `Palette.tsx:45` both use `kindDef(kind)?.label` as the canonical human-readable kind label, which is exactly what the draft claims. The finding holds against the actual code.
**VERDICT: Minor issues**
- `web/src/editor/ObjectShape.tsx:79` — the new accessible label hand-formats the kind via `object.kind.replace(/_/g, ' ')`, producing raw slugs like `"in ground"` / `"grow bag"`. But `kinds.ts` (already imported one line above for `objectDisplayName`) exports `kindDef(kind)?.label`, the canonical human label used elsewhere in the editor (`Inspector.tsx:124`, `Palette.tsx:45` both use `kindDef(...)?.label ?? ...` — see `web/src/editor/kinds.ts:18-25` for the actual label strings, e.g. `"In-ground"`, `"Grow bag"`). This reinvents formatting that already exists in the very module being imported from, producing a slightly worse/inconsistent string for screen reader users than the rest of the UI uses. Fix: `` `${objectDisplayName(object)}, ${kindDef(object.kind)?.label ?? object.kind}` `` (import `kindDef` alongside `objectDisplayName`).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, quadratic behavior).
- `Modal.tsx:37-42` — `focusable()` runs a `querySelectorAll` scoped to the dialog card on every `Tab` keypress. Verified the modal's `card` subtree is a small confirmation dialog (a handful of controls at most, per the PR's blast-radius list: `DeleteGarden`/`ClearBed`/`DeletePlant`/`DeleteSeedLot`/`Share`). Re-querying per keypress on a bounded, tiny DOM subtree is not a meaningful cost.
- `ObjectShape.tsx:79` — `label` (string concat + `objectDisplayName` + `replace`) is recomputed on every render of every object. Read the component: it's wrapped in `React.memo` (`ObjectShape.tsx:39`), so this only runs when an object's own props actually change, not on every canvas pan/zoom/select. The regex `replace` is O(string length) on a short kind name — negligible even at garden-scale object counts.
- `GardenCanvas.tsx:238-244` — `role`, `aria-label` (template string), and `<title>` added to the SVG root are computed once per canvas render, not per-object; no loop involvement.
- `styles/index.css` additions are static CSS rules, no runtime cost.
No N+1 patterns, no new work inside `rendered.map(...)` (`GardenCanvas.tsx:259-263`, unchanged by this diff), no new unbounded state, no blocking calls introduced. This PR is attribute/CSS/event-wiring only — nothing here changes asymptotic behavior or adds hot-path cost.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
## Verdict: Minor issues
- **`web/src/components/ui/Modal.tsx:71-74`** — The cleanup's `opener?.focus?.()` doesn't check whether `opener` is still attached to the document before restoring focus. Verified end-to-end: `DeleteGardenModal` (`web/src/components/gardens/DeleteGardenModal.tsx:16-17`) calls `onClose()` right after the delete mutation succeeds; `useDeleteGarden` (`web/src/lib/gardens.ts:133-138`) invalidates the gardens query on success, which refetches the list and eventually unmounts the `GardenCard` row — including the "Delete" `<button>` (`web/src/components/gardens/GardenCard.tsx:63-65`) that was `document.activeElement` when the dialog opened (i.e., `opener`). If that button is detached from the DOM by the time Modal's cleanup runs, `.focus()` on it is a silent no-op (focusing steps require the element be connected), and focus falls back to `<body>` — the "loses their place" failure the new code's own comment (line 32) says it's preventing. `ClearBedModal` (`web/src/editor/ClearBedModal.tsx:37`) follows the identical trigger-button-in-a-list → mutate → onClose pattern and is equally exposed. The code has no `isConnected` guard at all, so this is a real gap regardless of exact race timing in any one flow — suggest checking `opener?.isConnected` and falling back to a stable container (e.g. the list's own heading, or `document.body`) when the opener is gone.
Everything else in the diff — the Tab-trap boundary logic in `Modal.tsx` (verified: correctly handles empty-focusable-list, single-item, and Shift+Tab-from-card cases), and the `Enter`/`Space` handler added in `ObjectShape.tsx:65-71` — checks out against the source and introduces no new unhandled-error paths.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the security lens, I checked for injection, untrusted-data handling, and authz concerns:
- **`aria-label` / `<title>` construction** (`ObjectShape.tsx:79`, `GardenCanvas.tsx`): built from `objectDisplayName(object)`, `object.kind`, and `garden.name` — all user-authored garden data. These flow into React JSX attributes/text (`aria-label={label}`, `<title>{garden.name}</title>`), which React escapes by default. No `dangerouslySetInnerHTML`, no attribute-breakout vector. `object.kind` is run through `.replace(/_/g, ' ')` but still lands in an escaped attribute. No XSS path.
- **Modal focus trap** (`Modal.tsx:37-42`): `querySelectorAll` uses a static selector literal — no user/external input taints the selector, so no selector-injection / DOM-based issue. `document.activeElement` capture and `opener?.focus?.()` cleanup operate on already-focused DOM elements, no untrusted data.
- **No new network calls, deserialization, secret handling, or authz boundaries** touched by this change — it's purely client-side a11y/UI.
I verified `objectDisplayName` returns plain string fields (`kinds.ts:32-34`) and confirmed the render sites use normal JSX escaping.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Both findings verified against the actual source.
**Finding 1** — Confirmed. `ObjectShape.tsx:79` builds the label as `${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}`. `objectDisplayName` (`kinds.ts:32-34`) returns `o.name || kindDef(o.kind)?.label || 'Object'`, but the second segment is the raw `object.kind` slug with underscores replaced — not `kindDef(object.kind)?.label`. Reading the full `OBJECT_KINDS` table (`kinds.ts:17-25`): the kind slugs are `bed`, `grow_bag`, `container`, `in_ground`, `tree`, `path`, `structure`, with labels `Bed`, `Grow bag`, `Container`, etc. So a named bed reads `"North Bed, bed"` and an unnamed one reads `"Bed, bed"` (redundant) — never the `"North Bed, raised bed"` the comment promises. The friendly label is readily available via `kindDef(object.kind)?.label`. Upgrading confidence to high since I directly verified the label table.
**Finding 2** — Confirmed. `Modal.tsx:62-68`: the Tab handler only wraps when `active === first`, `active === last`, or `active === card`. If the currently-focused control becomes `:disabled` mid-mutation (excluded by the `:not([disabled])` selector in `focusable()`), the browser drops `activeElement` to `<body>`. A subsequent Tab/Shift+Tab then has `active === body`, matches neither branch, so `preventDefault` is never called and focus walks out behind the `fixed inset-0` backdrop. Narrow (requires a focused control disabling itself while busy), but real.
The draft's third bullet explicitly states "Not a bug, no change required" and was not included in the findings JSON, so it correctly does not appear as a finding.
**VERDICT: Minor issues**
- **`web/src/editor/ObjectShape.tsx:79` — accessible name's second segment is the raw kind slug, not the friendly label.** The label is `${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}`. `objectDisplayName` (`kinds.ts:32-34`) resolves to the name or `kindDef(kind)?.label`, but the second segment uses the bare slug (`object.kind`) with underscores swapped for spaces. Verified against `OBJECT_KINDS` (`kinds.ts:17-25`): slugs are `bed`, `grow_bag`, `in_ground`, etc., labels are `Bed`, `Grow bag`, `In-ground`. So a named bed reads `"North Bed, bed"` (redundant) and an unnamed one reads `"Bed, bed"` — not the `"North Bed, raised bed"` the inline comment and PR description claim. `kindDef(object.kind)?.label` is already available and would produce the documented behavior; alternatively drop the second segment when the display name already came from the label. Minor a11y labeling regression; low user impact.
- **`web/src/components/ui/Modal.tsx:62-68` — focus trap can leak when the focused control disables itself during `busy`.** The wrap branches only match `active === first`, `active === last`, or `active === card`. `focusable()` (`Modal.tsx:37-42`) excludes `:disabled` controls, so if the focused button becomes disabled mid-mutation the browser moves `activeElement` to `<body>`; a following Tab/Shift+Tab then matches no branch, `preventDefault` is not called, and focus escapes behind the `fixed inset-0 z-50` backdrop — the exact failure the trap exists to prevent. Narrow (only when a focused control disables itself during a pending mutation), but real. Suggested fix: in the `else` fallback, when `active` is not one of `items` and not `card`, redirect to `first` (Tab) / `last` (Shift+Tab).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## Verdict: Minor issues
**Finding 1 — Accessible name bypasses the existing `kindDef().label` abstraction and produces a redundant name for unnamed objects** (`web/src/editor/ObjectShape.tsx:79`)
The label is built as:
```ts
const label = `${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}`
```
Two related cleanliness problems, both confirmed against the actual source:
1. **Duplicated humanization logic.** `web/src/editor/kinds.ts` already curates a capitalized kind label map (`KindDef.label`: `"Grow bag"`, `"In-ground"`, `"Raised bed"`-style values), and `objectDisplayName` (`kinds.ts:32-33`) itself falls back to `kindDef(o.kind)?.label`. This line instead re-derives a human label from the raw `object.kind` string with `.replace(/_/g, ' ')`, yielding lowercase, un-hyphenated forms (`"grow bag"`, `"in ground"`) that disagree with the curated labels shown everywhere else. The "humanize a kind" concern now lives in two places with two different outputs.
2. **Redundant accessible name for unnamed objects.** `objectDisplayName` returns `o.name || kindDef(o.kind)?.label || 'Object'`, so an unnamed `grow_bag` gets `objectDisplayName = "Grow bag"` (line 33), and the full label becomes `"Grow bag, grow bag"` — the kind is stated twice in the screen-reader name. Unnamed objects are the common case right after placement.
Suggested fix — reuse the existing abstraction for the kind suffix and drop the redundant part when the display name is already the kind label:
```ts
const kindLabel = kindDef(object.kind)?.label ?? object.kind.replace(/_/g, ' ')
const name = objectDisplayName(object)
const label = name === kindLabel ? kindLabel : `${name}, ${kindLabel}`
```
Everything else in the diff is clean: the Modal focus-trap is self-contained and follows the existing ref-based effect pattern; the `focusable()` selector is a standard a11y query; the CSS `:focus-visible` rule is narrowly scoped and well-commented; the `object-shape` class name is reasonable.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
Through the performance lens, I checked the two hot-path-relevant spots:
- **Modal `focusable()` on each Tab** (`web/src/components/ui/Modal.tsx:37-42,53`): `querySelectorAll` + `Array.from` runs on every Tab keydown. Tab is an infrequent, user-paced event and the dialog contains only a handful of controls, so the cost is negligible. No caching needed.
- **Per-render string/regex in `ObjectShape`** (`web/src/editor/ObjectShape.tsx:79`): `object.kind.replace(/_/g, ' ')` and the template string allocate per render, but `ObjectShape` is wrapped in `memo`, so this only runs when an object's props actually change — not on every canvas frame/parent re-render. Not material.
The canvas change adds a `tabIndex={0}` per object (linear in object count), but that's tab-stop semantics, not a runtime cost, and is bounded by the garden's object list. No N+1, no unbounded growth, no per-frame work introduced. Clean from a performance standpoint.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Verdict: Minor issues
I verified both findings against the actual checked-out `web/src/components/ui/Modal.tsx`. The diff matches the source, and both edge cases hold up.
- `web/src/components/ui/Modal.tsx:73` — `opener` is captured once at mount (line 33) as `document.activeElement` and the cleanup calls `opener?.focus?.()`. The ref is never re-resolved, and there is no `isConnected` guard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out), `.focus()` on a detached node is a silent no-op and focus falls to `<body>` — exactly the loss the PR set out to fix. Suggested fix: guard with `opener?.isConnected && opener.focus()` and, when not connected, fall back to focusing `card` (or a documented restore target).
- `web/src/components/ui/Modal.tsx:61-68` — the Tab-wrap logic only fires when `active === first` (Shift), `active === card` (Shift), or `active === last` (non-Shift). If `document.activeElement` is none of those — e.g. focus escaped to `<body>` or an element behind the backdrop because something stole focus after mount, or focus landed on a `[tabindex="-1"]` child that isn't the card — neither branch executes, `preventDefault` is not called, and Tab walks out of the dialog. The comment claims it "pull[s] a stray focus … back to a real control," but it only covers the `card` stray case, not an arbitrary external `activeElement`. Suggested fix: when `active` is not within `items` (and not the card), explicitly refocus `first` (forward) or `last` (Shift) instead of falling through.
Both are edge cases rather than crashes; the happy paths (including empty-`items` and Escape-while-busy) are handled correctly.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict:** No material issues found
I examined the diff through the Security lens and found no authn/authz gaps, injection vectors, secret leakage, or unsafe handling of untrusted data.
- **XSS via `aria-label` / `<title>`:** The user-editable `garden.name` and `object.name` flow into React JSX props (`aria-label`, `<title>`, `aria-pressed`). React's normal attribute escaping applies; there is no `dangerouslySetInnerHTML` or raw HTML insertion.
- **DOM injection via `querySelectorAll`:** The focus-trap selector in `Modal.tsx` is a hardcoded string with no user-controlled interpolation.
- **Kind string injection:** `object.kind.replace(/_/g, ' ')` operates on a closed enum (`OBJECT_KINDS`); the regex is safe and the output is used only as a React text prop.
- **Focus restoration (`opener?.focus?.()`):** No security boundary is violated; this is an accessibility affordance, not an authentication/authorization mechanism.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Verdict:** Minor issues
- **`web/src/components/ui/Modal.tsx:40`** — The `focusable()` selector uses `input:not([disabled])`, which includes `input[type="hidden"]`. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (`active === first` / `active === last`) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button will not be prevented, and focus walks out behind the backdrop. **Fix:** change the selector clause to `input:not([disabled]):not([type="hidden"])`.
- **`web/src/editor/ObjectShape.tsx:96`** — `aria-pressed={selected}` on `role="button"` advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (`handleKey`) always calls `onSelect(object.id)`; there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. **Fix:** remove `aria-pressed`. If the selectable state must be exposed, use `aria-selected` with an appropriate role (e.g. `option` inside a `listbox`, or `gridcell`) rather than misusing a toggle button state.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/ObjectShape.tsx:79`** — The accessible label builds the kind display string with `object.kind.replace(/_/g, ' ')`, duplicating a concern that `kindDef()` in `./kinds.ts` already solves. `kindDef` returns curated, Title Case labels (e.g. `"Grow bag"`, `"In-ground"`) while the regex produces lowercase, unhyphenated output (`"grow bag"`, `"in ground"`). This creates an inconsistency with the rest of the UI (e.g. `JournalPanel` uses `objectDisplayName`, which respects `kindDef` labels) and means new kinds or label edits in `KindDef` won't propagate here.
**Fix:** Import `kindDef` and use `kindDef(object.kind)?.label ?? object.kind` for the second half of the label.
No other material maintainability issues found.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The changes are accessibility-focused and don't introduce material performance regressions. Specifically:
- **`Modal.tsx:37-42`** — `focusable()` runs `querySelectorAll` on every `Tab` keypress to build the trap list. This is O(n) on the modal's DOM subtree, but typical modals here have < 20 focusable controls, so the cost per tab is negligible. Not worth caching.
- **`ObjectShape.tsx:79`** — `object.kind.replace(/_/g, ' ')` and `objectDisplayName(object)` run on each render, but they're trivial string ops on small inputs and the component is already wrapped in `memo`, so it only re-renders when props change.
- **SVG focusables** — Adding `tabIndex={0}` to every garden object is an accessibility/UX pattern, not a rendering performance concern; the DOM node count is unchanged from before (the `<g>` elements already existed).
No allocations in hot loops, no N+1 patterns, no missing pagination, and no work moved onto critical paths.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
I scrutinized the diff through the error-handling & edge-cases lens and could not identify a material problem introduced by these changes.
What I checked:
- **Modal focus trap (`Modal.tsx`)** — The `focusable()` query re-runs on every `Tab` keydown, so dynamically added/removed controls are picked up. The `items.length === 0` branch prevents default and pulls focus back to the card. `Shift+Tab` from the card itself (which gets initial focus via `tabIndex={-1}`) is explicitly wrapped to `last`. Forward `Tab` from the card is *not* explicitly trapped, but standard browser behavior moves focus from a `tabindex="-1"` container to its first focusable descendant, so the trap is intact in practice (and the author verified this with real keyboard tests). The `opener?.focus?.()` cleanup safely restores focus on unmount; the optional chaining handles detached elements and SVG `<g>` openers gracefully.
- **Object keyboard selection (`ObjectShape.tsx`)** — `handleKey` defensively calls `preventDefault()` + `stopPropagation()` for `Enter`/`Space`, preventing scroll or bubbled actions. `aria-pressed` receives a boolean that React stringifies correctly. The `label` construction uses `objectDisplayName`, which has a safe fallback chain (`name → kind label → "Object"`). `tabIndex={0}` is applied unconditionally; there is no null-guard needed because the prop type guarantees `object` is present.
- **Canvas accessibility (`GardenCanvas.tsx`)** — `garden.name` is interpolated into `aria-label` and `<title>`; JSX escaping prevents injection issues. No runtime null risk because `garden` is a required prop.
- **CSS focus ring (`index.css`)** — The `:focus-visible` rule is progressive enhancement; the `outline: none` removal is acceptable because the replacement is confirmed working in the browsers the author tested, and the native fallback (default outline on SVG `<g>`) was unreliable anyway.
I also verified that the global arrow-key nudge handler in `GardenEditorPage` is not newly broken by this PR—it already existed and its `document.activeElement` whitelist simply doesn’t cover SVG `<g>` elements, which is correct because we *want* nudging to work when an object has focus. No new crash path or swallowed error is introduced.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
The aria-label is built from `objectDisplayName(object)` and `object.kind`. The kind comes from a controlled set (the API validates against an enum). Let me check whether kind is user-controlled and could inject anything, though aria-labels are text-only (no HTML injection risk in React).
React escapes aria-label text content, so no HTML/script injection vector there.
Now let me check the security angle of the Modal's `document.activeElement` opener capture and focus restoration. The opener is captured on mount and refocused on cleanup. Is there a scenario where the opener element gets removed from the DOM (e.g. the trigger button unmounts), leaving a stale ref? `opener?.focus?.()` uses optional chaining — if opener was detached, `.focus()` on a detached element is a no-op (no throw). Not a security issue, just UX.
Let me consider whether there's any authz/authn relevance. This is a frontend-only a11y change. No new API surfaces, no input handling changes that touch the backend. The `onSelect(object.id)` call passes an object id — let me verify the nudge handler that consumes selection does permission checking via the service layer (per CLAUDE.md the service is the seam).
**Verdict: No material issues found**
This is a frontend-only accessibility change. Through the security lens I checked:
- **No new tainted data flows.** The aria-labels are built from `objectDisplayName(object)` + `object.kind` (`ObjectShape.tsx:79`) and `garden.name` (`GardenCanvas.tsx:242`). All are rendered as React text content / attributes — React escapes them, so there is no HTML/script injection vector. `object.kind` is constrained to a fixed set (the store has a CHECK constraint; the service validates), and `object.name`/`garden.name` are display-only strings here.
- **No injection in the focus-trap selector.** `Modal.tsx:40` uses a static string literal for `querySelectorAll` — no user input is interpolated into the selector.
- **No authz/authn surface touched.** `onSelect(object.id)` only sets `selectedId` in the client store; the actual mutations (nudge/move/resize) flow through the existing service layer, which is the permission seam per the architecture. Selection alone is not an authorization boundary.
- **Opener focus restoration** (`Modal.tsx:33,73`) captures `document.activeElement` on mount and refocuses it on cleanup. No credential/secret exposure; the worst case is a no-op `.focus()` on a detached element, which is harmless.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I'll verify the diff against the actual checked-out code, focusing on correctness.
Now let me check the arrow-key nudge handler that the PR description references, and verify the `objectDisplayName`/label logic edge cases.
The arrow-key nudge handler the PR claims "already exists" — let me find it to verify it actually acts on the selection made by Enter/Space.
The nudge handler reads `s.selectedId` from the store. Let me verify `onSelect` actually sets `selectedId` so Enter → nudge chain works.
Good — `onSelect` is `select`, which sets `selectedId`. Enter → `onSelect` → `selectedId` set → arrow keys act on it. The chain works.
Now let me check the Escape handler interaction: the canvas Escape peels layers (line 163-175), and there's a new `onKeyDown` on object `<g>` that only handles Enter/Space (stopPropagation on those). But does Escape from a focused SVG element get caught by the window handler correctly? Yes — handleKey doesn't touch Escape, so it bubbles to window. Fine.
Let me check the Modal focus-trap edge: when the dialog opens, `card.focus()` is called but `document.activeElement` is captured *before* that — good. But what if the opener is inside an element that gets unmounted while the modal is open (common in React with conditional rendering)? Then `opener.focus()` on a detached element silently does nothing → focus lands on `<body>`. Let me check a couple of the call sites to see if the trigger button survives.
Now let me check the callers of these modals to see whether the opener button stays mounted while the modal is open. The key concern: if the trigger is conditionally rendered and unmounts when `modalOpen` becomes true, the restore-focus fails. Let me check one caller pattern.
The trigger buttons (GardenCard, the clear-bed button in the editor) stay mounted while the modal is open — the modal is rendered as a sibling, not replacing the trigger. So `opener.focus()` will work in these cases. Good.
Now let me verify one important correctness concern with the focus trap: the `focusable()` selector includes `[tabindex]:not([tabindex="-1"])`, which will match the card itself (it has `tabIndex={-1}`) — no, it excludes `-1`. But the object `<g>` elements with `tabIndex={0}` are not inside the modal, so fine. Let me check whether the trap correctly handles the case where focus is on the card (tabIndex=-1) and there ARE focusable items.
Looking at the logic: if `active === card` and shift+tab → wrap to last. But if `active === card` and plain Tab (no shift), the condition `!e.shiftKey && active === last` is false, so no preventDefault — the browser moves focus to the first focusable naturally. That's correct behavior.
Let me check the GardenCanvas `<title>` element placement — in SVG, `<title>` should be the first child of the element it labels.
Now let me look at a potential correctness concern with the nudge handler's input-guard. The window-level nudge handler (line 207-208) bails if `activeElement` is an INPUT/TEXTAREA/SELECT. But now a focused SVG `<g>` is `activeElement` when the user presses arrow keys. That's not in the bail list, so the nudge proceeds — which is exactly what we want. Good.
But wait — consider the reverse: a user is typing in an input field (e.g. the Inspector's name input) and presses Enter/Space. The object's `onKeyDown` is on the `<g>`, so it won't fire when focus is in an input. Fine. And the nudge handler bails on inputs. Fine.
Now, one concern: the new `handleKey` calls `e.stopPropagation()` for Enter/Space. The nudge handler is on `window` (capture not used, so bubble phase). Enter/Space on a focused `<g>` → `handleKey` fires, stopPropagation prevents it reaching window. But the nudge handler only cares about arrow keys, not Enter/Space, so stopPropagation is harmless there. However, the Escape handler (line 163-175) is also on `window`. If a user presses Escape while a `<g>` is focused, `handleKey` does nothing (no match), Escape bubbles to window, and the peel-back runs — deselecting. That's reasonable.
Let me double-check the `aria-pressed` semantics. `aria-pressed` on a `role="button"` indicates toggle state. Here `selected` maps to pressed. The button represents the object; "pressed" = selected. This is a slightly loose use of `aria-pressed` (the object isn't really a toggle), but it's a reasonable mapping and not a correctness bug. Not reporting.
Let me verify the CSS `:focus-visible` rule actually overrides the inline `stroke` on the shape. CSS specificity: `.object-shape:focus-visible :is(rect, ellipse)` is a class + pseudo-class + descendant selector. Inline `stroke` attribute is a presentation attribute, which has lower specificity than any CSS rule. So the CSS wins. Verified correct.
Now let me check one thing about the focus trap: the `onKey` is on `document` (bubble). If a Modal is open AND the GardenCanvas is behind it, and the user presses Tab while focus is on a canvas `<g>` that somehow... no, the modal is an overlay, the canvas isn't focusable through the backdrop. Not a concern.
Let me check whether multiple Modals can be open at once (nested), which would break the single `keydown` listener assumption.
No nested modals in the current code — each page renders at most one. The single-listener assumption holds.
Now let me look at the SeedLotModal which is a form modal (not in the PR's blast radius list but uses Modal) — does it have a form with inputs that the trap now affects? The trap wraps Tab among inputs. That's normal dialog behavior and correct.
Now, one correctness concern with the focus trap and forms: when the form has a submit button and the user is on the last input, Tab moves to the first item (wrapping). That's correct. But what about the case where `active` is neither `first`, `last`, nor `card` (e.g. focus is on a middle input) — no preventDefault, browser handles Tab normally. Correct.
Let me verify the `objectDisplayName` returns something sensible for the label. For an unnamed object: `o.name || kindDef(o.kind)?.label || 'Object'`. Then label = `"Object, bed"` for an unnamed bed? No — `kindDef('bed')?.label` = `'Bed'`, so it'd be `"Bed, bed"`. That's redundant but not a correctness bug. For a named object `"North Bed"` of kind `raised_bed`... wait, is `raised_bed` a kind? Let me check the kinds. From kinds.ts, the kinds are: `bed`, `grow_bag`, `container`, `in_ground`, `tree`, `path`, `structure`. There's no `raised_bed`. The PR comment says `"North Bed, raised bed"` — but the kind is `bed`, so it'd be `"North Bed, bed"`. The comment example is slightly off but the code is correct relative to the actual kinds. Minor comment inaccuracy, not a bug.
Let me check the types to confirm `object.kind` and the EditorObject shape.
Now let me check the CSS file to verify the rule placement (inside `@layer base`?) and the `--color-accent-strong` token exists.
Token exists in both light and dark. Good.
Now one more correctness concern: `stroke-dasharray: 5 4` as a CSS value on an SVG shape with `vector-effect="non-scaling-stroke"`. The dash values are in user units (cm in the world `<g>` since the parent transform scales). Without non-scaling-stroke, dashes would scale with the viewport. With `non-scaling-stroke`, the stroke width is in screen px but `stroke-dasharray` is still in user units... actually the dash array behavior with non-scaling-stroke is browser-dependent and can render unevenly at different zoom levels. This is a visual fidelity concern, not a correctness bug in the keyboard-reachability feature itself. I won't report it as material.
Let me verify the most important claim of the PR: that Enter/Space on a focused object actually results in the arrow-key nudge working. I've confirmed:
1. `onSelect` = `select` from the store (GardenCanvas line 261)
2. `select` sets `selectedId` (store line 86)
3. The nudge handler checks `s.selectedId != null` (line 212) and acts on it
4. The nudge handler bails only if `activeElement` is INPUT/TEXTAREA/SELECT — a focused `<g>` passes through
The chain is sound.
Now let me check one subtle issue: when the user tabs to an object and presses Enter, `handleKey` calls `e.stopPropagation()`. But does the window-level nudge keydown listener also listen for Enter? No — it only checks `DIRS[e.key]` (arrows). So no conflict. And the Escape handler only checks `e.key === 'Escape'`. No conflict.
But there's a real concern: after Enter selects the object, focus remains on the `<g>`. The user presses arrow keys. The nudge handler fires on `window`, reads `s.selectedId`, moves the object. Good. But `e.preventDefault()` is called in the nudge handler (line 215). Arrow keys on a focused element might also trigger scrolling — preventDefault stops that. Fine.
One more: does pressing Space on a focused `<g>` scroll the page? `handleKey` calls `e.preventDefault()` for Space, so no. Good.
Let me verify there are no existing tests that this change might break and that the claimed tests pass.
No Modal/object-shape tests exist, so no test breakage from the behavior change. The claimed 87 vitest tests are the lib tests.
I've thoroughly verified the change through the correctness lens. Let me summarize.
**VERDICT: No material issues found**
I verified the core correctness chain the PR depends on:
- **Enter/Space → nudge chain works**: `ObjectShape.onSelect` is wired to the store's `select` (`GardenCanvas.tsx:261`), which sets `selectedId` (`store.ts:86`). The arrow-key nudge handler (`GardenEditorPage.tsx:201-222`) reads `s.selectedId` and acts on it, and its input-guard (`tagName === INPUT/TEXTAREA/SELECT`, line 207-208) does NOT bail on a focused SVG `<g>`, so a keyboard-selected object is nudgable. The PR's central claim holds.
- **`handleKey` stopPropagation is safe**: it only stops Enter/Space; the window-level handlers only care about Escape and arrow keys respectively, so no listener is starved of events it needs. Escape from a focused `<g>` correctly bubbles to the peel-back handler.
- **Focus trap logic is correct**: the wrap conditions (`shift+tab` on first/card → last; plain `tab` on last → first) and the empty-dialog fallback are sound. Middle-item Tab is left to the browser. The `focusable()` selector excludes disabled controls and `tabindex="-1"` (so the card itself, `tabIndex={-1}`, is correctly excluded from the wrap set but still handled as a stray-focus origin).
- **Opener restore works for the listed blast radius**: all five named modals (`DeleteGarden`, `ClearBed`, `DeletePlant`, `DeleteSeedLot`, `Share`) are rendered as siblings of their trigger buttons, which stay mounted while the modal is open, so `opener.focus()` resolves to a live element. No nested-modals case exists in the current code, so the single `document` keydown listener is unambiguous.
- **`:focus-visible` CSS overrides inline `stroke`**: presentation attributes have lower specificity than any CSS rule, so `.object-shape:focus-visible :is(rect, ellipse) { stroke: ... }` wins over the inline `stroke={stroke}`. The `--color-accent-strong` token exists in both light and dark themes (`index.css:13`, `index.css:56`).
- **`objectDisplayName` import resolves** and the label derivation is correct for the actual kinds (`bed`, `grow_bag`, etc. — there is no `raised_bed` kind, so the comment example `"North Bed, raised bed"` is slightly inaccurate, but the runtime string `"North Bed, bed"` is correct). This is a comment inaccuracy, not a behavior bug.
No correctness defects found in my lane.
Final review complete.
## VERDICT: No material issues found
I verified the change through the correctness lens, checking the actual checked-out code rather than the diff in isolation:
- **Enter/Space → arrow-nudge chain (the PR's central claim)**: confirmed end-to-end. `ObjectShape`'s `onSelect` is wired to the store's `select` (`GardenCanvas.tsx:261` → `store.ts:86` sets `selectedId`). The nudge handler (`GardenEditorPage.tsx:201-222`) acts on `s.selectedId`, and its input-guard (line 207-208, bails only on `INPUT`/`TEXTAREA`/`SELECT`) does **not** reject a focused SVG `<g>`. So a keyboard-selected object is nudgable — the previously-unreachable feature now works.
- **`handleKey` stopPropagation is safe**: it only intercepts Enter/Space; the two `window`-level keydown listeners care only about Escape (`GardenEditorPage.tsx:164`) and arrow keys (`:201`) respectively. No listener is starved. Escape from a focused `<g>` bubbles correctly to the peel-back handler.
- **Focus trap logic is sound**: wrap conditions handle `shift+tab` on first/card → last and plain `tab` on last → first; middle-item Tab is left to the browser; empty-dialog falls back to the card. The `focusable()` selector correctly excludes `tabindex="-1"` (so the card, `tabIndex={-1}`, is excluded from the wrap set but still handled as a stray-focus origin) and disabled controls.
- **Opener restore works for the stated blast radius**: all five named modals are rendered as siblings of their trigger buttons (e.g. `GardensPage.tsx:55-73`, `GardenEditorPage.tsx:527-534`), which remain mounted while the modal is open, so `opener.focus()` resolves to a live element. No nested-modal pattern exists in the current code, so the single `document` keydown listener is unambiguous.
- **`:focus-visible` CSS overrides inline `stroke`**: SVG presentation attributes have lower specificity than any CSS rule, so `.object-shape:focus-visible :is(rect, ellipse) { stroke: ... }` wins over the inline `stroke={stroke}`. The `--color-accent-strong` token exists in both light (`index.css:13`) and dark (`index.css:56`) themes.
- **`objectDisplayName` import resolves** (`kinds.ts:32`); the label derivation is correct for the actual kinds (`bed`, `grow_bag`, …). Minor: the code comment's example `"North Bed, raised bed"` is inaccurate (there is no `raised_bed` kind; the real output is `"North Bed, bed"`), but this is a comment nit, not a behavior bug — not material.
No correctness defects in my lane.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
- **`web/src/editor/ObjectShape.tsx:79`** — the accessible-name suffix rebuilds the kind label with `object.kind.replace(/_/g, ' ')`, duplicating the canonical kind→human-label mapping that already lives in `kinds.ts` (`KindDef.label`, surfaced via `kindDef()`). The two diverge in practice: `in_ground` renders here as `"in ground"` while `kindDef('in_ground').label` is `"In-ground"` (hyphen, capitalized); `grow_bag` renders as `"grow bag"` vs `"Grow bag"`. Every other call site (`Inspector.tsx:124`, `Palette.tsx:45`) reads `kindDef(...)?.label`; this is the only one that re-derives it. Suggested fix: `const label = \`${objectDisplayName(object)}, ${kindDef(object.kind)?.label ?? object.kind.replace(/_/g, ' ')}\`` — use the single source of truth, keep the replace only as a fallback for unknown kinds. (Verified by reading `kinds.ts:18-24` and the other call sites via grep.)
- **`web/src/components/ui/Modal.tsx:40`** — the focusable-element selector is a long inline string literal re-evaluated by `querySelectorAll` on every Tab keypress. Hoisting it to a module-level constant would make the intent ("the dialog's focusable controls") named and self-documenting, and matches how the rest of the file keeps its keydown logic readable. Trivial, but it's the kind of magic string that quietly rots (e.g. someone adds a `[contenteditable]` and has to find it here).
No other cleanliness concerns. The CSS-vs-React choice for the focus ring is well commented, the `ObjectShape` changes follow the surrounding component structure, and the Modal focus-trap reads cleanly.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed the diff through the performance lens. Verified against the checked-out code.
- `Modal.tsx:37-42` — `focusable()` calls `querySelectorAll` on each Tab keydown. This runs only inside a modal dialog (a handful of elements, triggered by an occasional Tab press), not on any hot path. Negligible.
- `ObjectShape.tsx:79` — `object.kind.replace(/_/g, ' ')` and the `label` template run on each render, but the component is `memo`'d (line 39), so re-renders only happen when an object's props actually change, not on every pan/zoom. No per-frame cost.
- `ObjectShape.tsx:88-98` — new props/attributes added (`role`, `tabIndex`, `aria-label`, `aria-pressed`, `className`, `onKeyDown`) are constant-ish per render; no new allocations on a hot path.
- `styles/index.css` `.object-shape:focus-visible :is(rect, ellipse)` — a descendant-selector CSS rule; browsers match this cheaply and it only applies under `:focus-visible` (at most one element). No concern.
No efficiency regressions, unbounded growth, or hot-path work introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I'll review the diff through the error-handling & edge-cases lens, focusing on the unhappy paths introduced.
Let me check how the modal handles the `busy` case and the cleanup ordering issue, and verify the `Space` key handling interaction with scrolling.
Now let me examine the key edge cases. The `Space` key handling is important — when a focusable SVG `<g>` has `tabIndex={0}`, pressing Space normally triggers a click on buttons but `<g>` is not a native button. The handler calls `e.preventDefault()` which prevents page scroll. Good. But there's a subtle issue: the `handleKey` only handles Enter/Space for selection, but arrow-key nudge is mentioned as the follow-on. Let me check where arrow keys are handled.
Now let me understand a key edge case in the Modal focus trap. The `onKey` handler for the Modal is attached to `document`. The nudge handler in GardenEditorPage is attached to `window`. Both fire on Tab... actually nudge only handles arrows. But the Modal's `keydown` on `document` and GardenEditor's `keydown` on `window` — when a Modal is open over the editor, do they conflict? The nudge handler returns early on non-arrow keys, so no Tab conflict. But there's a more important edge case.
Let me look at the focus restoration ordering issue — the cleanup runs `opener?.focus?.()` but what if `opener` was removed from the DOM (e.g., the trigger button unmounted)? Also, let me check the `active === card` case in the Tab trap.
Now let me examine the key edge case in the Modal focus trap. The critical concern is the `busy` state interaction with Escape and the Tab trap. When `busy` is true, Escape is blocked (good), but Tab trapping still works. Let me check one important edge case: the ShareGardenModal has a list that can be empty or populated dynamically — the `focusable()` list is recomputed each Tab press, so that's fine.
But there's a real edge case: the `focusable()` selector includes `[tabindex]:not([tabindex="-1"])`. The card itself has `tabIndex={-1}`, so it won't match. But what about the case where focus is on an element NOT in the `items` list and NOT the card — e.g., focus somehow escaped to the backdrop or body? Let me think about this.
If `active` is neither `first`, `last`, nor `card`, then neither branch fires and Tab proceeds normally — which could walk out of the dialog. This is a real gap but it's an edge case that's hard to hit in practice since focus starts on `card`.
Let me check a more concrete issue: the `Space` key in `handleKey`. When `e.key === ' '` (Space) on a `role="button"` element, the native semantics would also fire a click. But since this is an SVG `<g>` with `role="button"` and `tabIndex={0}`, browsers may synthesize a click on Space for ARIA buttons. Let me verify whether `e.preventDefault()` in `onKeyDown` prevents the synthetic click.
Now let me think about the actual edge cases in my lens. The most important one: **the Modal cleanup restores focus to `opener` unconditionally, even when the modal closed because the opener itself was destroyed**. For example, in `DeleteGardenModal`, after successful deletion, `onClose()` is called. The opener (the "Delete" menu button that opened the modal) may have unmounted because the garden row was removed from the list. Calling `.focus()` on a detached element is a no-op (no error), so focus falls to `<body>`. That's the same problem the PR is trying to fix, but it's pre-existing behavior in a new edge case. Not a regression — the old code also left focus on `<body>`. So this is acceptable.
Let me check a more concrete issue: the `opener` capture happens at mount time. `document.activeElement` at the moment the effect runs — is it guaranteed to be the opener? React effects run after render/paint. If the modal is opened by a button click, the button is still `activeElement` when the effect runs (focus hasn't moved yet, since `card?.focus()` happens right after). This is correct.
But there's a subtle issue: **if the modal is opened programmatically** (not from a focused element), `document.activeElement` could be `<body>` or the modal's trigger might not be focused. Then on close, focus goes to `<body>` — which is the same as before, no regression.
Let me look at one more edge case: the `items.length === 0` branch in the Tab trap. If there are no focusable elements, it `preventDefault`s and focuses the card. But the card has `tabIndex={-1}`, so it CAN receive focus via `.focus()`. Good. But this means Tab is completely trapped with no way out except Escape — which is the correct behavior for a modal.
Now let me check the most important error-handling concern: **what happens when `cardRef.current` is null** (the `card` variable is null)? The `focusable()` function uses `card?.querySelectorAll(...)` which returns `undefined`, and `Array.from(undefined ?? [])` → `Array.from([])` → `[]`. So `items.length === 0`, it calls `card?.focus()` which is a no-op. No crash. Good defensive handling.
Let me check one final thing — the Escape handler change. Previously: `if (e.key === 'Escape' && !busyRef.current) onCloseRef.current()`. Now it adds `return` after. The `return` is important — without it, an Escape would fall through to the Tab check, but since `e.key === 'Escape'` !== `'Tab'`, the `if (e.key !== 'Tab') return` would catch it anyway. So the new `return` is redundant but harmless. No issue.
Let me verify the `Space` key preventDefault doesn't break scrolling in the canvas context.
Now I have a clear picture. Let me verify one final concern about the `Space` key on SVG `<g>` with `role="button"`. The `handleKey` calls `e.preventDefault()` and `e.stopPropagation()` then `onSelect`. For a native `<button>`, Space fires `click` on `keyup`, and `preventDefault` on `keydown` prevents it. But for an SVG `<g>` with ARIA `role="button"`, browsers do NOT synthesize click events — there's no native button behavior on `<g>`. So `onSelect` in `handleKey` is the only path. The `e.preventDefault()` prevents page scroll on Space, which is the right behavior. No double-fire issue.
One subtle edge case worth checking: the `handleKey` fires `onSelect` on BOTH `keydown` for Enter and Space. For Enter, native buttons fire click on `keydown` (for buttons it's actually keyup but preventDefault on keydown stops it). For a `<g>`, there's no synthesis, so firing on keydown is fine and responsive. No issue.
Let me check whether the arrow-key nudge handler in GardenEditorPage has a conflict now that objects are focusable. When an object `<g>` has focus and the user presses an arrow key, the nudge handler on `window` fires. But it checks `s.selectedId` — the object must be *selected* (not just focused). So pressing arrows on a focused-but-not-selected object does nothing (the `if (s.selectedId != null)` branch doesn't execute, and `selectedPlantingId` is also null). The arrow key then does its default action — which for a `role="button"` element is nothing (no scroll since it's an SVG). This is fine, but let me verify there's no scroll-the-page issue.
Actually, arrows on a focusable SVG element inside `role="application"` — the `application` role tells AT to pass keystrokes through. Arrow keys on a focused element inside `role="application"` would not scroll the page by default in most browsers since the SVG fills the container. This is acceptable.
I've completed my investigation. The diff is clean from an error-handling/edge-case perspective. The defensive null-handling in the Modal (`card?.focus()`, `Array.from(... ?? [])`) is correct. The `items.length === 0` branch is handled. The Space key `preventDefault` correctly prevents page scroll. No panics, no swallowed errors, no missing cleanup.
VERDICT: No material issues found
- **Modal focus trap (`Modal.tsx:54-58`)**: The `items.length === 0` branch is handled — `card?.focus()` is a safe no-op if `cardRef.current` is null, and `Array.from(... ?? [])` guards the null `querySelectorAll` result. Verified by reading the full effect.
- **Opener capture (`Modal.tsx:33`)**: `document.activeElement` is cast to `HTMLElement | null` and the cleanup uses `opener?.focus?.()` (optional chaining + optional call), so a null or non-HTMLElement opener can't crash. If the opener was unmounted (e.g. the garden row removed after delete), `.focus()` on a detached element is a silent no-op — not a regression (old behavior also left focus on `<body>`). Verified.
- **Space key (`ObjectShape.tsx:65-71`)**: `e.preventDefault()` stops the page-scroll default; SVG `<g>` has no native button semantics so there's no synthesized click to double-fire. Verified there is no `onClick` on `ObjectShape` that could conflict.
- **Tab trap mid-list gap (`Modal.tsx:62-68`)**: if `document.activeElement` is neither `first`, `last`, nor `card` (focus somehow escaped to the backdrop), neither branch fires and Tab is not trapped. This is a theoretical gap but not reachable from the normal flow (focus starts on `card` and the trap keeps it inside), so it's not a material defect.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
The diff is entirely frontend accessibility work: adding `role`, `aria-label`, `tabIndex`, keyboard handlers, CSS focus rings, and a modal focus trap. From a security lens, there is no new attack surface introduced.
- No authn/authz changes.
- No new injection points: `garden.name` and `object.name` are inserted via React JSX into `aria-label` and `<title>`, which are properly escaped by React's runtime. They were already rendered as DOM text before this change.
- No SSRF, deserialization, secret leakage, or insecure defaults.
- The keyboard handler in `ObjectShape` only responds to `Enter`/`Space` and calls the existing `onSelect` with a numeric `id`.
- The Modal focus trap uses standard `querySelectorAll` selectors and `preventDefault` on Tab for wrapping — expected, correct behavior.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Minor issues**
* `web/src/editor/ObjectShape.tsx:96` — `aria-pressed={selected}` on a non-toggle button.
`aria-pressed` is specifically for **toggle** buttons that cycle state on activation. Clicking an already-selected object does **not** deselect it — `select(object.id)` keeps it selected. A screen-reader user hearing "pressed" will expect a second activation to unpress, which doesn't happen.
**Fix:** Remove `aria-pressed`. The selection state is already communicated by the selection-stroke colour change and can be added to the `aria-label` if needed (e.g. `aria-label={selected ? label + ', selected' : label}`). If a programmatic selection indicator is desired, use `aria-selected` inside a container with a supporting role (`listbox`, `grid`, etc.) instead of misapplying `aria-pressed`.
* `web/src/components/ui/Modal.tsx:62` — Forward Tab from the initially-focused card container is not explicitly trapped.
The code handles `active === card` only for **Shift+Tab** (`e.shiftKey`), wrapping to `last`. If the card itself has focus (which it does on mount via `card?.focus()`) and the user presses **Tab** (forward), the handler does not intercept it. In practice most browsers move focus into the first child control, but this relies on native behaviour rather than an explicit guard. If the modal is rendered in DOM order before other focusable page elements, a forward Tab from the card could walk out into the page.
**Fix:** Add `active === card` to the forward-Tab branch: `else if (!e.shiftKey && (active === last || active === card)) { e.preventDefault(); first.focus() }`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found.
The changes are clean and well-structured from a maintainability standpoint:
- **Modal.tsx** keeps the focus-trap logic in a single, readable effect with a clear `focusable()` helper. Comments explain the DOM-order query and the focus-return behavior.
- **ObjectShape.tsx** adds a concise `handleKey` handler and a neatly composed `aria-label` without bloating the component. The CSS-over-inline-stroke explanation in the comment is accurate and useful.
- **index.css** scopes the new rule to `.object-shape:focus-visible :is(rect, ellipse)` — a tidy, low-specificity selector that won't leak elsewhere.
- No duplicated code, no confusing names, no functions that have grown too long, and no deviations from the patterns already in use.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
The changes are lightweight accessibility additions with no material performance regressions. Specifically:
- **Modal focus trap**: `focusable()` does a small `querySelectorAll` on every `Tab` keydown, but the selector scope is a single dialog card with typically 2–3 controls. This is negligible and not inside a hot loop.
- **ObjectShape aria-label computation**: `objectDisplayName()` plus a trivial `replace` run per object render. The component is memo'd and the parent opacity wrapper already causes re-renders on focus changes — the added work is O(1) per object and dwarfed by the SVG render itself.
- **New keyboard handlers**: `onKeyDown` on focused SVG `<g>` elements is handled via React's synthetic event delegation; no extra DOM listeners are attached.
- **No allocation or quadratic behavior introduced**: The diff does not add new arrays, maps, or nested loops over collections inside render paths.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
I scrutinized the PR through the **Error handling & edge cases** lens. Here is what I verified and why it is clean:
**Modal focus trap (`web/src/components/ui/Modal.tsx`)**
- I read the full file. The `focusable()` query re-runs on every `Tab` keydown, so dynamically added/removed controls are handled. The `items.length === 0` branch correctly traps Tab when the dialog has no interactive children. The `opener` capture and restoration (`opener?.focus?.()`) is safe: if the opener element has been removed from the DOM, `focus()` silently does nothing rather than throwing. The cleanup order (remove listener, then restore focus) is correct.
**Canvas keyboard path (`web/src/editor/ObjectShape.tsx`)**
- I verified the `objectDisplayName` utility in `kinds.ts`. The `label` string interpolation is safe: `object.kind` is typed `string`, and `.replace()` on an empty or unknown string does not throw. The `handleKey` only intercepts `Enter` and `Space`; it does not block arrow keys, which bubble up to the existing window-level nudge handler as intended. The component does not crash on a missing `name` — `objectDisplayName` falls back through `kindDef` to the literal `"Object"`.
**Nudge handler interaction (`web/src/pages/GardenEditorPage.tsx`)**
- I read the nudge handler effect. It checks `document.activeElement.tagName` against `INPUT`, `TEXTAREA`, and `SELECT`. When an SVG `<g>` is focused, `tagName` is `"G"`, so the nudge handler proceeds normally. There is no conflict with the new keyboard focus path; the nudge works exactly when an object is selected, regardless of whether the selection came from pointer or keyboard.
**CSS focus indicator (`web/src/styles/index.css`)**
- The `.object-shape:focus-visible :is(rect, ellipse)` rule overrides inline presentation attributes via specificity, and `outline: none` is paired with the `:focus-visible` rule so keyboard users get a visible indicator while mouse users do not. This is the intended accessibility pattern.
No swallowed errors, missing cleanups, nil-pointer risks, off-by-one conditions, integer overflows, or unhandled edge cases were introduced by this diff.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- Object aria-label uses kindDef().label (the canonical "In-ground") instead of
an ad-hoc kind.replace() that produced "in ground" and diverged from the UI.
4+ models flagged this.
- aria-current, not aria-pressed, for the selected object — selection isn't a
toggle, which is what aria-pressed means; aria-current marks the active item.
- Modal focus trap made robust: if focus is NOT inside the dialog (fell to
<body> because the focused control was removed — ShareGardenModal's
remove-share button — or disabled while busy, or externally stolen), Tab now
pulls it back in instead of escaping. The previous branches only handled
focus being exactly at a known boundary.
- Focus restore checks opener.isConnected before calling focus(): the delete/
clear flows this targets often remove the element that opened the dialog, and
a disconnected node's focus() silently no-ops.
- Hoisted the focusable-element selector to a module constant, and excluded
input[type="hidden"] (it matched input:not([disabled]) and, at a boundary,
broke the wrap).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
steve
merged commit e3d8e01e5b into main2026-07-22 03:34:08 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #84 (the scoped first slice). Part of #86.
The arrow-key nudge handler existed but only ever acted on a pointer selection, and nothing could select without a mouse — so the feature was unusable by exactly the keyboard users it's for. This gives the canvas a keyboard path in, and fixes the Modal focus trap every destructive confirmation goes through.
Canvas
<svg role="application">+ an aria-label describing the controls + a<title>naming the garden. A screen reader now meets an interactive canvas, not an empty graphic.<g>is a focusablerole="button"with an aria-label (name + kind) andaria-pressed. Enter/Space selects it — the missing step — which makes the existing arrow-key nudge reachable.:focus-visibleCSS rule draws a dashed accent ring on keyboard focus (and not on a mouse click — the whole point of:focus-visible). CSS rather than React state becauseonFocuson an SVG<g>is unreliable, and a CSS rule cleanly overrides the shape's inlinestroke.Modal
Blast radius:
DeleteGarden/ClearBed/DeletePlant/DeleteSeedLot/Share.<body>.Verified live (real keyboard, built binary)
Not just tsc — driven with actual key events against the running app:
<g tabindex>takes focus)activeElement === garia-pressedfalse→true:focus-visiblematched, computedstroke-dasharray "5px, 4px"activeElement === opener(The
:focus-visiblering can't be triggered by scripted/mouse focus by design, so it's confirmed with genuine Tab presses.)Not done here (noted follow-ups)
EditorRailtablist semantics (the third item #84 mentions).tsc --noEmitand 87 vitest tests pass.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 03:28:48Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -31,0 +37,4 @@const focusable = () =>Array.from(card?.querySelectorAll<HTMLElement>('a[href], button:not([disabled]), textarea:not([disabled]), input:not([disabled]), select:not([disabled]), [tabindex]:not([tabindex="-1"])',🟠 focusable() selector includes input[type="hidden"], breaking tab trap boundary when hidden inputs are at modal edges
correctness, maintainability · flagged by 2 models
web/src/components/ui/Modal.tsx:40— Thefocusable()selector usesinput:not([disabled]), which includesinput[type="hidden"]. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (active === first/active === last) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button…🪰 Gadfly · advisory
@@ -33,0 +46,4 @@onCloseRef.current()return}if (e.key !== 'Tab') return🟠 Focus trap doesn't handle the focused element being removed from the DOM (e.g. ShareGardenModal's remove-share button), letting Tab escape the dialog
correctness · flagged by 1 model
web/src/components/ui/Modal.tsx:49— The Tab-trap checksactive === first/last/cardbut not the case where the previously-focused element was removed from the DOM (focus reverts to<body>per browser default).ShareGardenModal.tsx:117-127's per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press findsdocument.activeElement === body, which matches none of the checked branches — nopreventDefault()fires…🪰 Gadfly · advisory
@@ -33,0 +58,4 @@}const first = items[0]const last = items[items.length - 1]const active = document.activeElement🟡 Focus trap falls through (Tab escapes) when activeElement is outside {first, last, card} — external/stolen focus isn't pulled back in
correctness, error-handling · flagged by 2 models
web/src/components/ui/Modal.tsx:61-68— the Tab-wrap logic only fires whenactive === first(Shift),active === card(Shift), oractive === last(non-Shift). Ifdocument.activeElementis none of those — e.g. focus escaped to<body>or an element behind the backdrop because something stole focus after mount, or focus landed on a[tabindex="-1"]child that isn't the card — neither branch executes,preventDefaultis not called, and Tab walks out of the dialog. The comment claims i…🪰 Gadfly · advisory
@@ -35,1 +71,3 @@return () => document.removeEventListener('keydown', onKey)return () => {document.removeEventListener('keydown', onKey)opener?.focus?.()🟠 Focus restore on modal close doesn't check if the opener element is still in the DOM, silently failing for the exact delete/clear flows the PR targets
error-handling · flagged by 2 models
web/src/components/ui/Modal.tsx:73—openeris captured once at mount (line 33) asdocument.activeElementand the cleanup callsopener?.focus?.(). The ref is never re-resolved, and there is noisConnectedguard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out),.focus()on a detached node is a silent no-op and focus falls to<body>— exa…🪰 Gadfly · advisory
@@ -63,0 +76,4 @@// A concise accessible name: the object's label plus its kind, e.g.// "North Bed, raised bed". The dimensions aren't included — they need the// garden's unit context this component doesn't hold — so they're a follow-up.const label = `${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}`🟠 Ad-hoc kind string formatting duplicates existing kindDef labels
correctness, maintainability · flagged by 4 models
web/src/editor/ObjectShape.tsx:79— The accessible label builds the kind display string withobject.kind.replace(/_/g, ' '), duplicating a concern thatkindDef()in./kinds.tsalready solves.kindDefreturns curated, Title Case labels (e.g."Grow bag","In-ground") while the regex produces lowercase, unhyphenated output ("grow bag","in ground"). This creates an inconsistency with the rest of the UI (e.g.JournalPanelusesobjectDisplayName, which respectskindDefl…🪰 Gadfly · advisory
@@ -65,0 +93,4 @@role="button"tabIndex={0}aria-label={label}aria-pressed={selected}🟠 aria-pressed misused on non-toggle role="button"; object selection is not a toggle action
correctness · flagged by 2 models
web/src/editor/ObjectShape.tsx:96—aria-pressed={selected}onrole="button"advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (handleKey) always callsonSelect(object.id); there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. Fix: removearia-pressed. I…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 6 findings (5 with multi-model agreement)
web/src/editor/ObjectShape.tsx:79web/src/components/ui/Modal.tsx:40web/src/components/ui/Modal.tsx:73web/src/editor/ObjectShape.tsx:96web/src/components/ui/Modal.tsx:611 single-model finding (lower confidence)
web/src/components/ui/Modal.tsx:49Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
This diff is confined to frontend accessibility (ARIA attributes, SVG
role/tabIndex, CSS:focus-visible, and a Tab-focus trap in a modal). I checked the specific spots most likely to hide a security issue for this kind of change:garden.name,objectDisplayName(object), andobject.kindare all interpolated into JSXaria-labelattributes and an SVG<title>text node (web/src/editor/GardenCanvas.tsx:242,:244,web/src/editor/ObjectShape.tsx:79). These go through React's normal attribute/text rendering, which escapes them — nodangerouslySetInnerHTMLor raw DOM writes exist anywhere inweb/src(verified via grep), so there's no XSS vector even if a garden/object name contained markup.web/src/components/ui/Modal.tsx:37-42) scopesquerySelectorAlltocard(the dialog root), so a stray focusable element elsewhere on the page can't be pulled into the trap, and theopenerrestored on close is whateverdocument.activeElementalready was in the same document at mount time — not attacker-influenceable.Nothing here touches authn/authz, injection surfaces, untrusted-data handling, or secrets.
🎯 Correctness — Minor issues
Both findings confirmed against the actual source.
Finding 1 (
ObjectShape.tsx:79): Confirmed.kindDef/objectDisplayNameare imported andobjectDisplayNameis used for the name half of the label, but the kind half usesobject.kind.replace(/_/g, ' ')instead ofkindDef(object.kind)?.label.kinds.ts:18-24shows real labels differ:in_ground→"In-ground"(label uses a hyphen, regex produces"in ground"), and all others are title-cased ("Grow bag"vs regex's"grow bag").Finding 2 (
Modal.tsx:44-69): Confirmed. The trap logic only checksactive === first,active === last, oractive === card.ShareGardenModal.tsx:117-127renders a "Remove" button inside the trapped dialog whoseonClickremoves that share fromshares.data, unmounting the focused button. Per standard browser behavior, focus then falls back to<body>. On the next Tab,document.activeElementisbody, matching none of the three checked values, so neither branch firespreventDefault(), and the browser's default Tab handling takes over — breaking the trap.VERDICT: Minor issues
web/src/editor/ObjectShape.tsx:79— The accessible label derives the kind text withobject.kind.replace(/_/g, ' ')instead of the canonicalkindDef(kind)?.label(already available via thekindDef/objectDisplayNameimport one line above). This produces wrong/inconsistent kind names for a screen-reader user:in_ground→"in ground"instead of the app's real label"In-ground", and every other kind loses its title case ("grow bag"vs"Grow bag","tree"vs"Tree", etc.). Fix: usekindDef(object.kind)?.label ?? object.kind.web/src/components/ui/Modal.tsx:49— The Tab-trap checksactive === first/last/cardbut not the case where the previously-focused element was removed from the DOM (focus reverts to<body>per browser default).ShareGardenModal.tsx:117-127's per-share "✕ Remove" button lives inside the trapped dialog; activating it via keyboard removes it from the list, so the next Tab press findsdocument.activeElement === body, which matches none of the checked branches — nopreventDefault()fires, and the browser's default Tab handling walks focus out of the dialog, defeating the trap. Fix: also listen forfocusinondocument(or treat "active not contained in card" as the fallback condition) and refocus the card/first item.🧹 Code cleanliness & maintainability — Minor issues
Confirmed —
Inspector.tsx:124andPalette.tsx:45both usekindDef(kind)?.labelas the canonical human-readable kind label, which is exactly what the draft claims. The finding holds against the actual code.VERDICT: Minor issues
web/src/editor/ObjectShape.tsx:79— the new accessible label hand-formats the kind viaobject.kind.replace(/_/g, ' '), producing raw slugs like"in ground"/"grow bag". Butkinds.ts(already imported one line above forobjectDisplayName) exportskindDef(kind)?.label, the canonical human label used elsewhere in the editor (Inspector.tsx:124,Palette.tsx:45both usekindDef(...)?.label ?? ...— seeweb/src/editor/kinds.ts:18-25for the actual label strings, e.g."In-ground","Grow bag"). This reinvents formatting that already exists in the very module being imported from, producing a slightly worse/inconsistent string for screen reader users than the rest of the UI uses. Fix:`${objectDisplayName(object)}, ${kindDef(object.kind)?.label ?? object.kind}`(importkindDefalongsideobjectDisplayName).⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, quadratic behavior).
Modal.tsx:37-42—focusable()runs aquerySelectorAllscoped to the dialog card on everyTabkeypress. Verified the modal'scardsubtree is a small confirmation dialog (a handful of controls at most, per the PR's blast-radius list:DeleteGarden/ClearBed/DeletePlant/DeleteSeedLot/Share). Re-querying per keypress on a bounded, tiny DOM subtree is not a meaningful cost.ObjectShape.tsx:79—label(string concat +objectDisplayName+replace) is recomputed on every render of every object. Read the component: it's wrapped inReact.memo(ObjectShape.tsx:39), so this only runs when an object's own props actually change, not on every canvas pan/zoom/select. The regexreplaceis O(string length) on a short kind name — negligible even at garden-scale object counts.GardenCanvas.tsx:238-244—role,aria-label(template string), and<title>added to the SVG root are computed once per canvas render, not per-object; no loop involvement.styles/index.cssadditions are static CSS rules, no runtime cost.No N+1 patterns, no new work inside
rendered.map(...)(GardenCanvas.tsx:259-263, unchanged by this diff), no new unbounded state, no blocking calls introduced. This PR is attribute/CSS/event-wiring only — nothing here changes asymptotic behavior or adds hot-path cost.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
web/src/components/ui/Modal.tsx:71-74— The cleanup'sopener?.focus?.()doesn't check whetheropeneris still attached to the document before restoring focus. Verified end-to-end:DeleteGardenModal(web/src/components/gardens/DeleteGardenModal.tsx:16-17) callsonClose()right after the delete mutation succeeds;useDeleteGarden(web/src/lib/gardens.ts:133-138) invalidates the gardens query on success, which refetches the list and eventually unmounts theGardenCardrow — including the "Delete"<button>(web/src/components/gardens/GardenCard.tsx:63-65) that wasdocument.activeElementwhen the dialog opened (i.e.,opener). If that button is detached from the DOM by the time Modal's cleanup runs,.focus()on it is a silent no-op (focusing steps require the element be connected), and focus falls back to<body>— the "loses their place" failure the new code's own comment (line 32) says it's preventing.ClearBedModal(web/src/editor/ClearBedModal.tsx:37) follows the identical trigger-button-in-a-list → mutate → onClose pattern and is equally exposed. The code has noisConnectedguard at all, so this is a real gap regardless of exact race timing in any one flow — suggest checkingopener?.isConnectedand falling back to a stable container (e.g. the list's own heading, ordocument.body) when the opener is gone.Everything else in the diff — the Tab-trap boundary logic in
Modal.tsx(verified: correctly handles empty-focusable-list, single-item, and Shift+Tab-from-card cases), and theEnter/Spacehandler added inObjectShape.tsx:65-71— checks out against the source and introduces no new unhandled-error paths.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
Through the security lens, I checked for injection, untrusted-data handling, and authz concerns:
aria-label/<title>construction (ObjectShape.tsx:79,GardenCanvas.tsx): built fromobjectDisplayName(object),object.kind, andgarden.name— all user-authored garden data. These flow into React JSX attributes/text (aria-label={label},<title>{garden.name}</title>), which React escapes by default. NodangerouslySetInnerHTML, no attribute-breakout vector.object.kindis run through.replace(/_/g, ' ')but still lands in an escaped attribute. No XSS path.Modal.tsx:37-42):querySelectorAlluses a static selector literal — no user/external input taints the selector, so no selector-injection / DOM-based issue.document.activeElementcapture andopener?.focus?.()cleanup operate on already-focused DOM elements, no untrusted data.I verified
objectDisplayNamereturns plain string fields (kinds.ts:32-34) and confirmed the render sites use normal JSX escaping.🎯 Correctness — Minor issues
Both findings verified against the actual source.
Finding 1 — Confirmed.
ObjectShape.tsx:79builds the label as${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}.objectDisplayName(kinds.ts:32-34) returnso.name || kindDef(o.kind)?.label || 'Object', but the second segment is the rawobject.kindslug with underscores replaced — notkindDef(object.kind)?.label. Reading the fullOBJECT_KINDStable (kinds.ts:17-25): the kind slugs arebed,grow_bag,container,in_ground,tree,path,structure, with labelsBed,Grow bag,Container, etc. So a named bed reads"North Bed, bed"and an unnamed one reads"Bed, bed"(redundant) — never the"North Bed, raised bed"the comment promises. The friendly label is readily available viakindDef(object.kind)?.label. Upgrading confidence to high since I directly verified the label table.Finding 2 — Confirmed.
Modal.tsx:62-68: the Tab handler only wraps whenactive === first,active === last, oractive === card. If the currently-focused control becomes:disabledmid-mutation (excluded by the:not([disabled])selector infocusable()), the browser dropsactiveElementto<body>. A subsequent Tab/Shift+Tab then hasactive === body, matches neither branch, sopreventDefaultis never called and focus walks out behind thefixed inset-0backdrop. Narrow (requires a focused control disabling itself while busy), but real.The draft's third bullet explicitly states "Not a bug, no change required" and was not included in the findings JSON, so it correctly does not appear as a finding.
VERDICT: Minor issues
web/src/editor/ObjectShape.tsx:79— accessible name's second segment is the raw kind slug, not the friendly label. The label is${objectDisplayName(object)}, ${object.kind.replace(/_/g, ' ')}.objectDisplayName(kinds.ts:32-34) resolves to the name orkindDef(kind)?.label, but the second segment uses the bare slug (object.kind) with underscores swapped for spaces. Verified againstOBJECT_KINDS(kinds.ts:17-25): slugs arebed,grow_bag,in_ground, etc., labels areBed,Grow bag,In-ground. So a named bed reads"North Bed, bed"(redundant) and an unnamed one reads"Bed, bed"— not the"North Bed, raised bed"the inline comment and PR description claim.kindDef(object.kind)?.labelis already available and would produce the documented behavior; alternatively drop the second segment when the display name already came from the label. Minor a11y labeling regression; low user impact.web/src/components/ui/Modal.tsx:62-68— focus trap can leak when the focused control disables itself duringbusy. The wrap branches only matchactive === first,active === last, oractive === card.focusable()(Modal.tsx:37-42) excludes:disabledcontrols, so if the focused button becomes disabled mid-mutation the browser movesactiveElementto<body>; a following Tab/Shift+Tab then matches no branch,preventDefaultis not called, and focus escapes behind thefixed inset-0 z-50backdrop — the exact failure the trap exists to prevent. Narrow (only when a focused control disables itself during a pending mutation), but real. Suggested fix: in theelsefallback, whenactiveis not one ofitemsand notcard, redirect tofirst(Tab) /last(Shift+Tab).🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
Finding 1 — Accessible name bypasses the existing
kindDef().labelabstraction and produces a redundant name for unnamed objects (web/src/editor/ObjectShape.tsx:79)The label is built as:
Two related cleanliness problems, both confirmed against the actual source:
Duplicated humanization logic.
web/src/editor/kinds.tsalready curates a capitalized kind label map (KindDef.label:"Grow bag","In-ground","Raised bed"-style values), andobjectDisplayName(kinds.ts:32-33) itself falls back tokindDef(o.kind)?.label. This line instead re-derives a human label from the rawobject.kindstring with.replace(/_/g, ' '), yielding lowercase, un-hyphenated forms ("grow bag","in ground") that disagree with the curated labels shown everywhere else. The "humanize a kind" concern now lives in two places with two different outputs.Redundant accessible name for unnamed objects.
objectDisplayNamereturnso.name || kindDef(o.kind)?.label || 'Object', so an unnamedgrow_baggetsobjectDisplayName = "Grow bag"(line 33), and the full label becomes"Grow bag, grow bag"— the kind is stated twice in the screen-reader name. Unnamed objects are the common case right after placement.Suggested fix — reuse the existing abstraction for the kind suffix and drop the redundant part when the display name is already the kind label:
Everything else in the diff is clean: the Modal focus-trap is self-contained and follows the existing ref-based effect pattern; the
focusable()selector is a standard a11y query; the CSS:focus-visiblerule is narrowly scoped and well-commented; theobject-shapeclass name is reasonable.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, I checked the two hot-path-relevant spots:
focusable()on each Tab (web/src/components/ui/Modal.tsx:37-42,53):querySelectorAll+Array.fromruns on every Tab keydown. Tab is an infrequent, user-paced event and the dialog contains only a handful of controls, so the cost is negligible. No caching needed.ObjectShape(web/src/editor/ObjectShape.tsx:79):object.kind.replace(/_/g, ' ')and the template string allocate per render, butObjectShapeis wrapped inmemo, so this only runs when an object's props actually change — not on every canvas frame/parent re-render. Not material.The canvas change adds a
tabIndex={0}per object (linear in object count), but that's tab-stop semantics, not a runtime cost, and is bounded by the garden's object list. No N+1, no unbounded growth, no per-frame work introduced. Clean from a performance standpoint.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
I verified both findings against the actual checked-out
web/src/components/ui/Modal.tsx. The diff matches the source, and both edge cases hold up.web/src/components/ui/Modal.tsx:73—openeris captured once at mount (line 33) asdocument.activeElementand the cleanup callsopener?.focus?.(). The ref is never re-resolved, and there is noisConnectedguard or fallback target. If the opener was removed from the DOM by the time the dialog closes (e.g. the trigger's parent conditionally re-rendered, or a list item behind the backdrop was swapped out),.focus()on a detached node is a silent no-op and focus falls to<body>— exactly the loss the PR set out to fix. Suggested fix: guard withopener?.isConnected && opener.focus()and, when not connected, fall back to focusingcard(or a documented restore target).web/src/components/ui/Modal.tsx:61-68— the Tab-wrap logic only fires whenactive === first(Shift),active === card(Shift), oractive === last(non-Shift). Ifdocument.activeElementis none of those — e.g. focus escaped to<body>or an element behind the backdrop because something stole focus after mount, or focus landed on a[tabindex="-1"]child that isn't the card — neither branch executes,preventDefaultis not called, and Tab walks out of the dialog. The comment claims it "pull[s] a stray focus … back to a real control," but it only covers thecardstray case, not an arbitrary externalactiveElement. Suggested fix: whenactiveis not withinitems(and not the card), explicitly refocusfirst(forward) orlast(Shift) instead of falling through.Both are edge cases rather than crashes; the happy paths (including empty-
itemsand Escape-while-busy) are handled correctly.kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I examined the diff through the Security lens and found no authn/authz gaps, injection vectors, secret leakage, or unsafe handling of untrusted data.
aria-label/<title>: The user-editablegarden.nameandobject.nameflow into React JSX props (aria-label,<title>,aria-pressed). React's normal attribute escaping applies; there is nodangerouslySetInnerHTMLor raw HTML insertion.querySelectorAll: The focus-trap selector inModal.tsxis a hardcoded string with no user-controlled interpolation.object.kind.replace(/_/g, ' ')operates on a closed enum (OBJECT_KINDS); the regex is safe and the output is used only as a React text prop.opener?.focus?.()): No security boundary is violated; this is an accessibility affordance, not an authentication/authorization mechanism.🎯 Correctness — Minor issues
Verdict: Minor issues
web/src/components/ui/Modal.tsx:40— Thefocusable()selector usesinput:not([disabled]), which includesinput[type="hidden"]. Hidden inputs are not keyboard-focusable. If a modal ever places a hidden input at the start or end of its content (e.g., a form with a CSRF token), the focus trap’s boundary checks (active === first/active === last) will compare against the hidden input instead of the real last tabbable control. Tab (or Shift+Tab) from the actual last visible button will not be prevented, and focus walks out behind the backdrop. Fix: change the selector clause toinput:not([disabled]):not([type="hidden"]).web/src/editor/ObjectShape.tsx:96—aria-pressed={selected}onrole="button"advertises a toggle button to screen readers (pressed / not pressed). The keyboard action (handleKey) always callsonSelect(object.id); there is no path that deselects the object when Enter/Space is pressed again on an already-selected item, so the toggle semantics are misleading. A screen reader user will expect the state to flip on second activation, which it does not. Fix: removearia-pressed. If the selectable state must be exposed, usearia-selectedwith an appropriate role (e.g.optioninside alistbox, orgridcell) rather than misusing a toggle button state.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
web/src/editor/ObjectShape.tsx:79— The accessible label builds the kind display string withobject.kind.replace(/_/g, ' '), duplicating a concern thatkindDef()in./kinds.tsalready solves.kindDefreturns curated, Title Case labels (e.g."Grow bag","In-ground") while the regex produces lowercase, unhyphenated output ("grow bag","in ground"). This creates an inconsistency with the rest of the UI (e.g.JournalPanelusesobjectDisplayName, which respectskindDeflabels) and means new kinds or label edits inKindDefwon't propagate here.Fix: Import
kindDefand usekindDef(object.kind)?.label ?? object.kindfor the second half of the label.No other material maintainability issues found.
⚡ Performance — No material issues found
No material issues found
The changes are accessibility-focused and don't introduce material performance regressions. Specifically:
Modal.tsx:37-42—focusable()runsquerySelectorAllon everyTabkeypress to build the trap list. This is O(n) on the modal's DOM subtree, but typical modals here have < 20 focusable controls, so the cost per tab is negligible. Not worth caching.ObjectShape.tsx:79—object.kind.replace(/_/g, ' ')andobjectDisplayName(object)run on each render, but they're trivial string ops on small inputs and the component is already wrapped inmemo, so it only re-renders when props change.tabIndex={0}to every garden object is an accessibility/UX pattern, not a rendering performance concern; the DOM node count is unchanged from before (the<g>elements already existed).No allocations in hot loops, no N+1 patterns, no missing pagination, and no work moved onto critical paths.
🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the diff through the error-handling & edge-cases lens and could not identify a material problem introduced by these changes.
What I checked:
Modal.tsx) — Thefocusable()query re-runs on everyTabkeydown, so dynamically added/removed controls are picked up. Theitems.length === 0branch prevents default and pulls focus back to the card.Shift+Tabfrom the card itself (which gets initial focus viatabIndex={-1}) is explicitly wrapped tolast. ForwardTabfrom the card is not explicitly trapped, but standard browser behavior moves focus from atabindex="-1"container to its first focusable descendant, so the trap is intact in practice (and the author verified this with real keyboard tests). Theopener?.focus?.()cleanup safely restores focus on unmount; the optional chaining handles detached elements and SVG<g>openers gracefully.ObjectShape.tsx) —handleKeydefensively callspreventDefault()+stopPropagation()forEnter/Space, preventing scroll or bubbled actions.aria-pressedreceives a boolean that React stringifies correctly. Thelabelconstruction usesobjectDisplayName, which has a safe fallback chain (name → kind label → "Object").tabIndex={0}is applied unconditionally; there is no null-guard needed because the prop type guaranteesobjectis present.GardenCanvas.tsx) —garden.nameis interpolated intoaria-labeland<title>; JSX escaping prevents injection issues. No runtime null risk becausegardenis a required prop.index.css) — The:focus-visiblerule is progressive enhancement; theoutline: noneremoval is acceptable because the replacement is confirmed working in the browsers the author tested, and the native fallback (default outline on SVG<g>) was unreliable anyway.I also verified that the global arrow-key nudge handler in
GardenEditorPageis not newly broken by this PR—it already existed and itsdocument.activeElementwhitelist simply doesn’t cover SVG<g>elements, which is correct because we want nudging to work when an object has focus. No new crash path or swallowed error is introduced.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
The aria-label is built from
objectDisplayName(object)andobject.kind. The kind comes from a controlled set (the API validates against an enum). Let me check whether kind is user-controlled and could inject anything, though aria-labels are text-only (no HTML injection risk in React).React escapes aria-label text content, so no HTML/script injection vector there.
Now let me check the security angle of the Modal's
document.activeElementopener capture and focus restoration. The opener is captured on mount and refocused on cleanup. Is there a scenario where the opener element gets removed from the DOM (e.g. the trigger button unmounts), leaving a stale ref?opener?.focus?.()uses optional chaining — if opener was detached,.focus()on a detached element is a no-op (no throw). Not a security issue, just UX.Let me consider whether there's any authz/authn relevance. This is a frontend-only a11y change. No new API surfaces, no input handling changes that touch the backend. The
onSelect(object.id)call passes an object id — let me verify the nudge handler that consumes selection does permission checking via the service layer (per CLAUDE.md the service is the seam).Verdict: No material issues found
This is a frontend-only accessibility change. Through the security lens I checked:
objectDisplayName(object)+object.kind(ObjectShape.tsx:79) andgarden.name(GardenCanvas.tsx:242). All are rendered as React text content / attributes — React escapes them, so there is no HTML/script injection vector.object.kindis constrained to a fixed set (the store has a CHECK constraint; the service validates), andobject.name/garden.nameare display-only strings here.Modal.tsx:40uses a static string literal forquerySelectorAll— no user input is interpolated into the selector.onSelect(object.id)only setsselectedIdin the client store; the actual mutations (nudge/move/resize) flow through the existing service layer, which is the permission seam per the architecture. Selection alone is not an authorization boundary.Modal.tsx:33,73) capturesdocument.activeElementon mount and refocuses it on cleanup. No credential/secret exposure; the worst case is a no-op.focus()on a detached element, which is harmless.🎯 Correctness — No material issues found
I'll verify the diff against the actual checked-out code, focusing on correctness.
Now let me check the arrow-key nudge handler that the PR description references, and verify the
objectDisplayName/label logic edge cases.The arrow-key nudge handler the PR claims "already exists" — let me find it to verify it actually acts on the selection made by Enter/Space.
The nudge handler reads
s.selectedIdfrom the store. Let me verifyonSelectactually setsselectedIdso Enter → nudge chain works.Good —
onSelectisselect, which setsselectedId. Enter →onSelect→selectedIdset → arrow keys act on it. The chain works.Now let me check the Escape handler interaction: the canvas Escape peels layers (line 163-175), and there's a new
onKeyDownon object<g>that only handles Enter/Space (stopPropagation on those). But does Escape from a focused SVG element get caught by the window handler correctly? Yes — handleKey doesn't touch Escape, so it bubbles to window. Fine.Let me check the Modal focus-trap edge: when the dialog opens,
card.focus()is called butdocument.activeElementis captured before that — good. But what if the opener is inside an element that gets unmounted while the modal is open (common in React with conditional rendering)? Thenopener.focus()on a detached element silently does nothing → focus lands on<body>. Let me check a couple of the call sites to see if the trigger button survives.Now let me check the callers of these modals to see whether the opener button stays mounted while the modal is open. The key concern: if the trigger is conditionally rendered and unmounts when
modalOpenbecomes true, the restore-focus fails. Let me check one caller pattern.The trigger buttons (GardenCard, the clear-bed button in the editor) stay mounted while the modal is open — the modal is rendered as a sibling, not replacing the trigger. So
opener.focus()will work in these cases. Good.Now let me verify one important correctness concern with the focus trap: the
focusable()selector includes[tabindex]:not([tabindex="-1"]), which will match the card itself (it hastabIndex={-1}) — no, it excludes-1. But the object<g>elements withtabIndex={0}are not inside the modal, so fine. Let me check whether the trap correctly handles the case where focus is on the card (tabIndex=-1) and there ARE focusable items.Looking at the logic: if
active === cardand shift+tab → wrap to last. But ifactive === cardand plain Tab (no shift), the condition!e.shiftKey && active === lastis false, so no preventDefault — the browser moves focus to the first focusable naturally. That's correct behavior.Let me check the GardenCanvas
<title>element placement — in SVG,<title>should be the first child of the element it labels.Now let me look at a potential correctness concern with the nudge handler's input-guard. The window-level nudge handler (line 207-208) bails if
activeElementis an INPUT/TEXTAREA/SELECT. But now a focused SVG<g>isactiveElementwhen the user presses arrow keys. That's not in the bail list, so the nudge proceeds — which is exactly what we want. Good.But wait — consider the reverse: a user is typing in an input field (e.g. the Inspector's name input) and presses Enter/Space. The object's
onKeyDownis on the<g>, so it won't fire when focus is in an input. Fine. And the nudge handler bails on inputs. Fine.Now, one concern: the new
handleKeycallse.stopPropagation()for Enter/Space. The nudge handler is onwindow(capture not used, so bubble phase). Enter/Space on a focused<g>→handleKeyfires, stopPropagation prevents it reaching window. But the nudge handler only cares about arrow keys, not Enter/Space, so stopPropagation is harmless there. However, the Escape handler (line 163-175) is also onwindow. If a user presses Escape while a<g>is focused,handleKeydoes nothing (no match), Escape bubbles to window, and the peel-back runs — deselecting. That's reasonable.Let me double-check the
aria-pressedsemantics.aria-pressedon arole="button"indicates toggle state. Hereselectedmaps to pressed. The button represents the object; "pressed" = selected. This is a slightly loose use ofaria-pressed(the object isn't really a toggle), but it's a reasonable mapping and not a correctness bug. Not reporting.Let me verify the CSS
:focus-visiblerule actually overrides the inlinestrokeon the shape. CSS specificity:.object-shape:focus-visible :is(rect, ellipse)is a class + pseudo-class + descendant selector. Inlinestrokeattribute is a presentation attribute, which has lower specificity than any CSS rule. So the CSS wins. Verified correct.Now let me check one thing about the focus trap: the
onKeyis ondocument(bubble). If a Modal is open AND the GardenCanvas is behind it, and the user presses Tab while focus is on a canvas<g>that somehow... no, the modal is an overlay, the canvas isn't focusable through the backdrop. Not a concern.Let me check whether multiple Modals can be open at once (nested), which would break the single
keydownlistener assumption.No nested modals in the current code — each page renders at most one. The single-listener assumption holds.
Now let me look at the SeedLotModal which is a form modal (not in the PR's blast radius list but uses Modal) — does it have a form with inputs that the trap now affects? The trap wraps Tab among inputs. That's normal dialog behavior and correct.
Now, one correctness concern with the focus trap and forms: when the form has a submit button and the user is on the last input, Tab moves to the first item (wrapping). That's correct. But what about the case where
activeis neitherfirst,last, norcard(e.g. focus is on a middle input) — no preventDefault, browser handles Tab normally. Correct.Let me verify the
objectDisplayNamereturns something sensible for the label. For an unnamed object:o.name || kindDef(o.kind)?.label || 'Object'. Then label ="Object, bed"for an unnamed bed? No —kindDef('bed')?.label='Bed', so it'd be"Bed, bed". That's redundant but not a correctness bug. For a named object"North Bed"of kindraised_bed... wait, israised_beda kind? Let me check the kinds. From kinds.ts, the kinds are:bed,grow_bag,container,in_ground,tree,path,structure. There's noraised_bed. The PR comment says"North Bed, raised bed"— but the kind isbed, so it'd be"North Bed, bed". The comment example is slightly off but the code is correct relative to the actual kinds. Minor comment inaccuracy, not a bug.Let me check the types to confirm
object.kindand the EditorObject shape.Now let me check the CSS file to verify the rule placement (inside
@layer base?) and the--color-accent-strongtoken exists.Token exists in both light and dark. Good.
Now one more correctness concern:
stroke-dasharray: 5 4as a CSS value on an SVG shape withvector-effect="non-scaling-stroke". The dash values are in user units (cm in the world<g>since the parent transform scales). Without non-scaling-stroke, dashes would scale with the viewport. Withnon-scaling-stroke, the stroke width is in screen px butstroke-dasharrayis still in user units... actually the dash array behavior with non-scaling-stroke is browser-dependent and can render unevenly at different zoom levels. This is a visual fidelity concern, not a correctness bug in the keyboard-reachability feature itself. I won't report it as material.Let me verify the most important claim of the PR: that Enter/Space on a focused object actually results in the arrow-key nudge working. I've confirmed:
onSelect=selectfrom the store (GardenCanvas line 261)selectsetsselectedId(store line 86)s.selectedId != null(line 212) and acts on itactiveElementis INPUT/TEXTAREA/SELECT — a focused<g>passes throughThe chain is sound.
Now let me check one subtle issue: when the user tabs to an object and presses Enter,
handleKeycallse.stopPropagation(). But does the window-level nudge keydown listener also listen for Enter? No — it only checksDIRS[e.key](arrows). So no conflict. And the Escape handler only checkse.key === 'Escape'. No conflict.But there's a real concern: after Enter selects the object, focus remains on the
<g>. The user presses arrow keys. The nudge handler fires onwindow, readss.selectedId, moves the object. Good. Bute.preventDefault()is called in the nudge handler (line 215). Arrow keys on a focused element might also trigger scrolling — preventDefault stops that. Fine.One more: does pressing Space on a focused
<g>scroll the page?handleKeycallse.preventDefault()for Space, so no. Good.Let me verify there are no existing tests that this change might break and that the claimed tests pass.
No Modal/object-shape tests exist, so no test breakage from the behavior change. The claimed 87 vitest tests are the lib tests.
I've thoroughly verified the change through the correctness lens. Let me summarize.
VERDICT: No material issues found
I verified the core correctness chain the PR depends on:
ObjectShape.onSelectis wired to the store'sselect(GardenCanvas.tsx:261), which setsselectedId(store.ts:86). The arrow-key nudge handler (GardenEditorPage.tsx:201-222) readss.selectedIdand acts on it, and its input-guard (tagName === INPUT/TEXTAREA/SELECT, line 207-208) does NOT bail on a focused SVG<g>, so a keyboard-selected object is nudgable. The PR's central claim holds.handleKeystopPropagation is safe: it only stops Enter/Space; the window-level handlers only care about Escape and arrow keys respectively, so no listener is starved of events it needs. Escape from a focused<g>correctly bubbles to the peel-back handler.shift+tabon first/card → last; plaintabon last → first) and the empty-dialog fallback are sound. Middle-item Tab is left to the browser. Thefocusable()selector excludes disabled controls andtabindex="-1"(so the card itself,tabIndex={-1}, is correctly excluded from the wrap set but still handled as a stray-focus origin).DeleteGarden,ClearBed,DeletePlant,DeleteSeedLot,Share) are rendered as siblings of their trigger buttons, which stay mounted while the modal is open, soopener.focus()resolves to a live element. No nested-modals case exists in the current code, so the singledocumentkeydown listener is unambiguous.:focus-visibleCSS overrides inlinestroke: presentation attributes have lower specificity than any CSS rule, so.object-shape:focus-visible :is(rect, ellipse) { stroke: ... }wins over the inlinestroke={stroke}. The--color-accent-strongtoken exists in both light and dark themes (index.css:13,index.css:56).objectDisplayNameimport resolves and the label derivation is correct for the actual kinds (bed,grow_bag, etc. — there is noraised_bedkind, so the comment example"North Bed, raised bed"is slightly inaccurate, but the runtime string"North Bed, bed"is correct). This is a comment inaccuracy, not a behavior bug.No correctness defects found in my lane.
Final review complete.
VERDICT: No material issues found
I verified the change through the correctness lens, checking the actual checked-out code rather than the diff in isolation:
ObjectShape'sonSelectis wired to the store'sselect(GardenCanvas.tsx:261→store.ts:86setsselectedId). The nudge handler (GardenEditorPage.tsx:201-222) acts ons.selectedId, and its input-guard (line 207-208, bails only onINPUT/TEXTAREA/SELECT) does not reject a focused SVG<g>. So a keyboard-selected object is nudgable — the previously-unreachable feature now works.handleKeystopPropagation is safe: it only intercepts Enter/Space; the twowindow-level keydown listeners care only about Escape (GardenEditorPage.tsx:164) and arrow keys (:201) respectively. No listener is starved. Escape from a focused<g>bubbles correctly to the peel-back handler.shift+tabon first/card → last and plaintabon last → first; middle-item Tab is left to the browser; empty-dialog falls back to the card. Thefocusable()selector correctly excludestabindex="-1"(so the card,tabIndex={-1}, is excluded from the wrap set but still handled as a stray-focus origin) and disabled controls.GardensPage.tsx:55-73,GardenEditorPage.tsx:527-534), which remain mounted while the modal is open, soopener.focus()resolves to a live element. No nested-modal pattern exists in the current code, so the singledocumentkeydown listener is unambiguous.:focus-visibleCSS overrides inlinestroke: SVG presentation attributes have lower specificity than any CSS rule, so.object-shape:focus-visible :is(rect, ellipse) { stroke: ... }wins over the inlinestroke={stroke}. The--color-accent-strongtoken exists in both light (index.css:13) and dark (index.css:56) themes.objectDisplayNameimport resolves (kinds.ts:32); the label derivation is correct for the actual kinds (bed,grow_bag, …). Minor: the code comment's example"North Bed, raised bed"is inaccurate (there is noraised_bedkind; the real output is"North Bed, bed"), but this is a comment nit, not a behavior bug — not material.No correctness defects in my lane.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
web/src/editor/ObjectShape.tsx:79— the accessible-name suffix rebuilds the kind label withobject.kind.replace(/_/g, ' '), duplicating the canonical kind→human-label mapping that already lives inkinds.ts(KindDef.label, surfaced viakindDef()). The two diverge in practice:in_groundrenders here as"in ground"whilekindDef('in_ground').labelis"In-ground"(hyphen, capitalized);grow_bagrenders as"grow bag"vs"Grow bag". Every other call site (Inspector.tsx:124,Palette.tsx:45) readskindDef(...)?.label; this is the only one that re-derives it. Suggested fix:const label = \${objectDisplayName(object)}, ${kindDef(object.kind)?.label ?? object.kind.replace(/_/g, ' ')}`— use the single source of truth, keep the replace only as a fallback for unknown kinds. (Verified by readingkinds.ts:18-24` and the other call sites via grep.)web/src/components/ui/Modal.tsx:40— the focusable-element selector is a long inline string literal re-evaluated byquerySelectorAllon every Tab keypress. Hoisting it to a module-level constant would make the intent ("the dialog's focusable controls") named and self-documenting, and matches how the rest of the file keeps its keydown logic readable. Trivial, but it's the kind of magic string that quietly rots (e.g. someone adds a[contenteditable]and has to find it here).No other cleanliness concerns. The CSS-vs-React choice for the focus ring is well commented, the
ObjectShapechanges follow the surrounding component structure, and the Modal focus-trap reads cleanly.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed the diff through the performance lens. Verified against the checked-out code.
Modal.tsx:37-42—focusable()callsquerySelectorAllon each Tab keydown. This runs only inside a modal dialog (a handful of elements, triggered by an occasional Tab press), not on any hot path. Negligible.ObjectShape.tsx:79—object.kind.replace(/_/g, ' ')and thelabeltemplate run on each render, but the component ismemo'd (line 39), so re-renders only happen when an object's props actually change, not on every pan/zoom. No per-frame cost.ObjectShape.tsx:88-98— new props/attributes added (role,tabIndex,aria-label,aria-pressed,className,onKeyDown) are constant-ish per render; no new allocations on a hot path.styles/index.css.object-shape:focus-visible :is(rect, ellipse)— a descendant-selector CSS rule; browsers match this cheaply and it only applies under:focus-visible(at most one element). No concern.No efficiency regressions, unbounded growth, or hot-path work introduced.
🧯 Error handling & edge cases — No material issues found
I'll review the diff through the error-handling & edge-cases lens, focusing on the unhappy paths introduced.
Let me check how the modal handles the
busycase and the cleanup ordering issue, and verify theSpacekey handling interaction with scrolling.Now let me examine the key edge cases. The
Spacekey handling is important — when a focusable SVG<g>hastabIndex={0}, pressing Space normally triggers a click on buttons but<g>is not a native button. The handler callse.preventDefault()which prevents page scroll. Good. But there's a subtle issue: thehandleKeyonly handles Enter/Space for selection, but arrow-key nudge is mentioned as the follow-on. Let me check where arrow keys are handled.Now let me understand a key edge case in the Modal focus trap. The
onKeyhandler for the Modal is attached todocument. The nudge handler in GardenEditorPage is attached towindow. Both fire on Tab... actually nudge only handles arrows. But the Modal'skeydownondocumentand GardenEditor'skeydownonwindow— when a Modal is open over the editor, do they conflict? The nudge handler returns early on non-arrow keys, so no Tab conflict. But there's a more important edge case.Let me look at the focus restoration ordering issue — the cleanup runs
opener?.focus?.()but what ifopenerwas removed from the DOM (e.g., the trigger button unmounted)? Also, let me check theactive === cardcase in the Tab trap.Now let me examine the key edge case in the Modal focus trap. The critical concern is the
busystate interaction with Escape and the Tab trap. Whenbusyis true, Escape is blocked (good), but Tab trapping still works. Let me check one important edge case: the ShareGardenModal has a list that can be empty or populated dynamically — thefocusable()list is recomputed each Tab press, so that's fine.But there's a real edge case: the
focusable()selector includes[tabindex]:not([tabindex="-1"]). The card itself hastabIndex={-1}, so it won't match. But what about the case where focus is on an element NOT in theitemslist and NOT the card — e.g., focus somehow escaped to the backdrop or body? Let me think about this.If
activeis neitherfirst,last, norcard, then neither branch fires and Tab proceeds normally — which could walk out of the dialog. This is a real gap but it's an edge case that's hard to hit in practice since focus starts oncard.Let me check a more concrete issue: the
Spacekey inhandleKey. Whene.key === ' '(Space) on arole="button"element, the native semantics would also fire a click. But since this is an SVG<g>withrole="button"andtabIndex={0}, browsers may synthesize a click on Space for ARIA buttons. Let me verify whethere.preventDefault()inonKeyDownprevents the synthetic click.Now let me think about the actual edge cases in my lens. The most important one: the Modal cleanup restores focus to
openerunconditionally, even when the modal closed because the opener itself was destroyed. For example, inDeleteGardenModal, after successful deletion,onClose()is called. The opener (the "Delete" menu button that opened the modal) may have unmounted because the garden row was removed from the list. Calling.focus()on a detached element is a no-op (no error), so focus falls to<body>. That's the same problem the PR is trying to fix, but it's pre-existing behavior in a new edge case. Not a regression — the old code also left focus on<body>. So this is acceptable.Let me check a more concrete issue: the
openercapture happens at mount time.document.activeElementat the moment the effect runs — is it guaranteed to be the opener? React effects run after render/paint. If the modal is opened by a button click, the button is stillactiveElementwhen the effect runs (focus hasn't moved yet, sincecard?.focus()happens right after). This is correct.But there's a subtle issue: if the modal is opened programmatically (not from a focused element),
document.activeElementcould be<body>or the modal's trigger might not be focused. Then on close, focus goes to<body>— which is the same as before, no regression.Let me look at one more edge case: the
items.length === 0branch in the Tab trap. If there are no focusable elements, itpreventDefaults and focuses the card. But the card hastabIndex={-1}, so it CAN receive focus via.focus(). Good. But this means Tab is completely trapped with no way out except Escape — which is the correct behavior for a modal.Now let me check the most important error-handling concern: what happens when
cardRef.currentis null (thecardvariable is null)? Thefocusable()function usescard?.querySelectorAll(...)which returnsundefined, andArray.from(undefined ?? [])→Array.from([])→[]. Soitems.length === 0, it callscard?.focus()which is a no-op. No crash. Good defensive handling.Let me check one final thing — the Escape handler change. Previously:
if (e.key === 'Escape' && !busyRef.current) onCloseRef.current(). Now it addsreturnafter. Thereturnis important — without it, an Escape would fall through to the Tab check, but sincee.key === 'Escape'!=='Tab', theif (e.key !== 'Tab') returnwould catch it anyway. So the newreturnis redundant but harmless. No issue.Let me verify the
Spacekey preventDefault doesn't break scrolling in the canvas context.Now I have a clear picture. Let me verify one final concern about the
Spacekey on SVG<g>withrole="button". ThehandleKeycallse.preventDefault()ande.stopPropagation()thenonSelect. For a native<button>, Space firesclickonkeyup, andpreventDefaultonkeydownprevents it. But for an SVG<g>with ARIArole="button", browsers do NOT synthesize click events — there's no native button behavior on<g>. SoonSelectinhandleKeyis the only path. Thee.preventDefault()prevents page scroll on Space, which is the right behavior. No double-fire issue.One subtle edge case worth checking: the
handleKeyfiresonSelecton BOTHkeydownfor Enter and Space. For Enter, native buttons fire click onkeydown(for buttons it's actually keyup but preventDefault on keydown stops it). For a<g>, there's no synthesis, so firing on keydown is fine and responsive. No issue.Let me check whether the arrow-key nudge handler in GardenEditorPage has a conflict now that objects are focusable. When an object
<g>has focus and the user presses an arrow key, the nudge handler onwindowfires. But it checkss.selectedId— the object must be selected (not just focused). So pressing arrows on a focused-but-not-selected object does nothing (theif (s.selectedId != null)branch doesn't execute, andselectedPlantingIdis also null). The arrow key then does its default action — which for arole="button"element is nothing (no scroll since it's an SVG). This is fine, but let me verify there's no scroll-the-page issue.Actually, arrows on a focusable SVG element inside
role="application"— theapplicationrole tells AT to pass keystrokes through. Arrow keys on a focused element insiderole="application"would not scroll the page by default in most browsers since the SVG fills the container. This is acceptable.I've completed my investigation. The diff is clean from an error-handling/edge-case perspective. The defensive null-handling in the Modal (
card?.focus(),Array.from(... ?? [])) is correct. Theitems.length === 0branch is handled. The Space keypreventDefaultcorrectly prevents page scroll. No panics, no swallowed errors, no missing cleanup.VERDICT: No material issues found
Modal.tsx:54-58): Theitems.length === 0branch is handled —card?.focus()is a safe no-op ifcardRef.currentis null, andArray.from(... ?? [])guards the nullquerySelectorAllresult. Verified by reading the full effect.Modal.tsx:33):document.activeElementis cast toHTMLElement | nulland the cleanup usesopener?.focus?.()(optional chaining + optional call), so a null or non-HTMLElement opener can't crash. If the opener was unmounted (e.g. the garden row removed after delete),.focus()on a detached element is a silent no-op — not a regression (old behavior also left focus on<body>). Verified.ObjectShape.tsx:65-71):e.preventDefault()stops the page-scroll default; SVG<g>has no native button semantics so there's no synthesized click to double-fire. Verified there is noonClickonObjectShapethat could conflict.Modal.tsx:62-68): ifdocument.activeElementis neitherfirst,last, norcard(focus somehow escaped to the backdrop), neither branch fires and Tab is not trapped. This is a theoretical gap but not reachable from the normal flow (focus starts oncardand the trap keeps it inside), so it's not a material defect.opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
The diff is entirely frontend accessibility work: adding
role,aria-label,tabIndex, keyboard handlers, CSS focus rings, and a modal focus trap. From a security lens, there is no new attack surface introduced.garden.nameandobject.nameare inserted via React JSX intoaria-labeland<title>, which are properly escaped by React's runtime. They were already rendered as DOM text before this change.ObjectShapeonly responds toEnter/Spaceand calls the existingonSelectwith a numericid.querySelectorAllselectors andpreventDefaulton Tab for wrapping — expected, correct behavior.🎯 Correctness — Minor issues
Minor issues
web/src/editor/ObjectShape.tsx:96—aria-pressed={selected}on a non-toggle button.aria-pressedis specifically for toggle buttons that cycle state on activation. Clicking an already-selected object does not deselect it —select(object.id)keeps it selected. A screen-reader user hearing "pressed" will expect a second activation to unpress, which doesn't happen.Fix: Remove
aria-pressed. The selection state is already communicated by the selection-stroke colour change and can be added to thearia-labelif needed (e.g.aria-label={selected ? label + ', selected' : label}). If a programmatic selection indicator is desired, usearia-selectedinside a container with a supporting role (listbox,grid, etc.) instead of misapplyingaria-pressed.web/src/components/ui/Modal.tsx:62— Forward Tab from the initially-focused card container is not explicitly trapped.The code handles
active === cardonly for Shift+Tab (e.shiftKey), wrapping tolast. If the card itself has focus (which it does on mount viacard?.focus()) and the user presses Tab (forward), the handler does not intercept it. In practice most browsers move focus into the first child control, but this relies on native behaviour rather than an explicit guard. If the modal is rendered in DOM order before other focusable page elements, a forward Tab from the card could walk out into the page.Fix: Add
active === cardto the forward-Tab branch:else if (!e.shiftKey && (active === last || active === card)) { e.preventDefault(); first.focus() }.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The changes are clean and well-structured from a maintainability standpoint:
focusable()helper. Comments explain the DOM-order query and the focus-return behavior.handleKeyhandler and a neatly composedaria-labelwithout bloating the component. The CSS-over-inline-stroke explanation in the comment is accurate and useful..object-shape:focus-visible :is(rect, ellipse)— a tidy, low-specificity selector that won't leak elsewhere.⚡ Performance — No material issues found
No material issues found
The changes are lightweight accessibility additions with no material performance regressions. Specifically:
focusable()does a smallquerySelectorAllon everyTabkeydown, but the selector scope is a single dialog card with typically 2–3 controls. This is negligible and not inside a hot loop.objectDisplayName()plus a trivialreplacerun per object render. The component is memo'd and the parent opacity wrapper already causes re-renders on focus changes — the added work is O(1) per object and dwarfed by the SVG render itself.onKeyDownon focused SVG<g>elements is handled via React's synthetic event delegation; no extra DOM listeners are attached.🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the PR through the Error handling & edge cases lens. Here is what I verified and why it is clean:
Modal focus trap (
web/src/components/ui/Modal.tsx)focusable()query re-runs on everyTabkeydown, so dynamically added/removed controls are handled. Theitems.length === 0branch correctly traps Tab when the dialog has no interactive children. Theopenercapture and restoration (opener?.focus?.()) is safe: if the opener element has been removed from the DOM,focus()silently does nothing rather than throwing. The cleanup order (remove listener, then restore focus) is correct.Canvas keyboard path (
web/src/editor/ObjectShape.tsx)objectDisplayNameutility inkinds.ts. Thelabelstring interpolation is safe:object.kindis typedstring, and.replace()on an empty or unknown string does not throw. ThehandleKeyonly interceptsEnterandSpace; it does not block arrow keys, which bubble up to the existing window-level nudge handler as intended. The component does not crash on a missingname—objectDisplayNamefalls back throughkindDefto the literal"Object".Nudge handler interaction (
web/src/pages/GardenEditorPage.tsx)document.activeElement.tagNameagainstINPUT,TEXTAREA, andSELECT. When an SVG<g>is focused,tagNameis"G", so the nudge handler proceeds normally. There is no conflict with the new keyboard focus path; the nudge works exactly when an object is selected, regardless of whether the selection came from pointer or keyboard.CSS focus indicator (
web/src/styles/index.css).object-shape:focus-visible :is(rect, ellipse)rule overrides inline presentation attributes via specificity, andoutline: noneis paired with the:focus-visiblerule so keyboard users get a visible indicator while mouse users do not. This is the intended accessibility pattern.No swallowed errors, missing cleanups, nil-pointer risks, off-by-one conditions, integer overflows, or unhandled edge cases were introduced by this diff.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.