Found while checking #133 live: double-clicking a bed only selected it — in Playwright, through the Chrome extension, and by hand. The hint in the toolkit and the plan's aria-label both promise "double-click a bed to plant it".
Why:track() calls setPointerCapture on the SVG root for every press (since the Organic rebuild, 52b2c09). Pointer capture retargets the compatibility click/dblclick events to the capturing element, so the onDoubleClick on each object's <g> never fires. It has been dead since #124; nobody noticed because "Plant this" in the inspector (and Plants mode on the phone) get you there too.
Fix: detect the double press in objDown itself — two presses on the same object within 400 ms and 12 px — which capture can't retarget. The second press focuses the bed and starts no drag, so its pointerup has nothing to select into Plot. A plop in an unfocused bed already delegates to objDown, so double-clicking a plant focuses its bed. The dead onDoubleClick prop is gone, with a comment saying why it can't come back.
tsc, vitest and the build pass; the behaviour is confirmed on the live build after merge (the extension's double_click produced two real presses, which is exactly what selects-only today and focuses with this).
Found while checking #133 live: double-clicking a bed only selected it — in Playwright, through the Chrome extension, and by hand. The hint in the toolkit and the plan's aria-label both promise "double-click a bed to plant it".
**Why:** `track()` calls `setPointerCapture` on the SVG root for every press (since the Organic rebuild, 52b2c09). Pointer capture retargets the compatibility `click`/`dblclick` events to the capturing element, so the `onDoubleClick` on each object's `<g>` never fires. It has been dead since #124; nobody noticed because "Plant this" in the inspector (and Plants mode on the phone) get you there too.
**Fix:** detect the double press in `objDown` itself — two presses on the same object within 400 ms and 12 px — which capture can't retarget. The second press focuses the bed and starts no drag, so its pointerup has nothing to select into Plot. A plop in an unfocused bed already delegates to `objDown`, so double-clicking a plant focuses its bed. The dead `onDoubleClick` prop is gone, with a comment saying why it can't come back.
`tsc`, vitest and the build pass; the behaviour is confirmed on the live build after merge (the extension's `double_click` produced two real presses, which is exactly what selects-only today and focuses with this).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
"Double-click a bed to plant it" has done nothing since the Organic rebuild:
track() captures the pointer on the SVG root, and pointer capture retargets
the compatibility click/dblclick events to the root, so the onDoubleClick
handler on each object's <g> never fired. A double-click only selected.
The double press is now detected in objDown itself — two presses on the
same object within 400 ms and 12 px — which capture cannot retarget. The
second press focuses the bed and starts no drag, so its pointerup has
nothing to select into Plot; a plop in an unfocused bed already delegates
to objDown, so double-clicking a plant focuses its bed too.
Co-Authored-By: Claude Fable 5 <[email protected]>
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
4/4 reviewers finished · updated 2026-08-23 07:21:35Z
#### `claude-code/opus` · claude-code — ✅ 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
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ 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
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 2 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** — 2 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡lastPress useRef declared far from the component's other refs (lines 87-97), breaking the established grouping pattern
maintainability · flagged by 3 models
lastPress ref declared out-of-group (web/src/editor/Canvas.tsx:338): All other useRef hooks are declared together at lines 87–97. lastPress is inserted ~250 lines later, next to the function that uses it. A reader scanning for "which refs does this component own?" will miss it. It should sit with its peers at the top of the hook block — the proximity to objDown doesn't add anything because refs don't have render-time side-effects.
🪰 Gadfly · advisory
🟡 **lastPress useRef declared far from the component's other refs (lines 87-97), breaking the established grouping pattern**
_maintainability · flagged by 3 models_
- **`lastPress` ref declared out-of-group** (`web/src/editor/Canvas.tsx:338`): All other `useRef` hooks are declared together at lines 87–97. `lastPress` is inserted ~250 lines later, next to the function that uses it. A reader scanning for "which refs does this component own?" will miss it. It should sit with its peers at the top of the hook block — the proximity to `objDown` doesn't add anything because refs don't have render-time side-effects.
<sub>🪰 Gadfly · advisory</sub>
web/src/editor/Canvas.tsx:357 — lastPress is updated for every pointer button, so non-primary clicks (right-click, middle-click) can participate in or complete a double-click. Verified by reading objDown: the ref is written unconditionally before the e.button check, and the if (prev && …) block that fires focusObject does not inspect e.button. Native dblclick fires only for the primary button; right-clicking a bed twice within 400 ms should not focus it. Fix: gate the `last…
🪰 Gadfly · advisory
🟠 **Double-click detection accepts non-primary pointer buttons, unlike native dblclick**
_correctness, error-handling · flagged by 2 models_
- **`web/src/editor/Canvas.tsx:357` — `lastPress` is updated for every pointer button, so non-primary clicks (right-click, middle-click) can participate in or complete a double-click.** Verified by reading `objDown`: the ref is written unconditionally before the `e.button` check, and the `if (prev && …)` block that fires `focusObject` does not inspect `e.button`. Native `dblclick` fires only for the primary button; right-clicking a bed twice within 400 ms should not focus it. Fix: gate the `last…
<sub>🪰 Gadfly · advisory</sub>
This is a purely client-side interaction change — double-click detection for focusing a bed in the SVG canvas editor. Reviewed through the Security lens:
No untrusted data reaches any sink. The new code reads only performance.now(), e.clientX/Y, and the object's numeric id — all local browser/editor values. They feed Math.hypot/timestamp comparisons and focusObject(o); none is used in a query, path, template, URL, or dangerouslySetInnerHTML. No injection surface. (Verified by reading Canvas.tsx:355-364.)
No authz bypass.focusObject (verified at Canvas.tsx:171-192) only mutates local editor UI state (setFocus/setSel/setTab/camera) — it grants no privileges. The actual mutation path (placePlop, Canvas.tsx:301-315) still gates on latest.current.canEdit, and per the project's architecture the real permission enforcement lives server-side in internal/service. Double-clicking merely arms the plant palette; it cannot plant in a bed the user can't edit.
o.plantable guard on the double-press branch matches the removed onDoubleClick gate, so no code path became reachable that wasn't already reachable via the inspector's "Plant this". No insecure default introduced.
No secrets, deserialization, SSRF, or new input-validation gaps — there is no network or data-parsing code in this diff.
Nothing in my lane is materially wrong here.
🎯 Correctness — No material issues found
I've verified the change against the checked-out code. Let me walk through my correctness analysis.
What I checked:
Traced the full double-click sequence through objDown (Canvas.tsx:342), onCanvasUp (459), track/setPointerCapture (261), and focusObject (171).
Confirmed the plop-delegation path (plopDown line 381 → objDown(o)(e)) so double-clicking a plant records the bed's id and focuses the bed, as the PR claims.
Confirmed dim (line 571 = focusId != null && o.id !== focusId) is exactly the early-return guard at line 354, so dropping !dim from the removed onDoubleClick loses nothing — dimmed siblings still return early.
Confirmed no other onDoubleClick/dblclick handlers exist (only the aria-label copy at line 557).
down1: prev is null/stale → records lastPress, falls through, sets drag='obj'.
up1: d.t==='obj', not moved → selects object into Plot, clears drag.
down2: prev.id===o.id, within 400 ms / 12 px → double detected; nulls lastPress and drag, calls focusObject (plantable) → focus + toolkit tab. End state correct.
up2: drag is null → returns early, nothing selected. Matches the comment.
Edge cases that hold up:
lastPress reset to null on a detected double prevents a triple-click re-triggering.
id-match requirement blocks cross-object false positives (press A then B never fires a double).
performance.now() is monotonic; Math.hypot on clientX/Y in screen px with a 12 px tolerance is standard.
Non-plantable object double-click: second press nulls drag, skips focus, returns → object stays selected from click 1. No regression.
Pointer capture is auto-released on pointerup; pts.current.delete runs before the early return in onCanvasUp — no leak.
The thresholds (400 ms, 12 px) are ordinary double-click tolerances, not domain conversion factors, so there's no magic-number derivation to redo here.
One nuance I considered but do not think is a bug: in read-only mode (canEdit false), the second press still calls focusObject, which flips the rail to the toolkit (plant) tab. But the original onDoubleClick had no canEdit guard either, so this matches the pre-existing intended design (the handler was merely dead due to capture), and planting itself is still blocked in placePlop. Not a correctness regression.
VERDICT: No material issues found
The double-press detection is logically sound: the state machine (record → compare id/time/distance → reset) is correct, the pointerup path is properly neutralized, the plop delegation preserves the "double-click a plant focuses its bed" behavior, and the removed onDoubleClick was provably equivalent to the retained early-return guard. Nothing in the correctness lens is materially wrong.
🧹 Code cleanliness & maintainability — Minor issues
The finding is confirmed: module-level tuning constants live at lines 38–41 (WHEEL_SENSITIVITY, ANIM_MS, REFIT_THRESHOLD_PX, FT), while the new DOUBLE_PRESS_MS/DOUBLE_PRESS_PX sit in the component body at 339–340. The only other in-component ALL_CAPS names (GW/GH at 114–115) are derived from garden props, so they legitimately stay inside — they don't undermine the convention claim. The lastPress ref correctly remains in-component. Finding holds.
VERDICT: Minor issues
web/src/editor/Canvas.tsx:339-340 — tuning constants placed inside the component body, against the file's own convention.DOUBLE_PRESS_MS and DOUBLE_PRESS_PX are plain magic-number tuning constants, but they're declared inside the Canvas component function (recreated every render). Every other constant of this kind in the file lives at module top — WHEEL_SENSITIVITY, ANIM_MS, REFIT_THRESHOLD_PX, FT (lines 38–41). The only other in-component ALL_CAPS names (GW/GH) are values derived from garden props, so they must stay local; these two are compile-time constants that belong beside ANIM_MS et al. The lastPress ref correctly stays in the component. Trivial, but it's exactly the "doesn't follow the pattern the surrounding code uses" case.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the Performance lens by reading web/src/editor/Canvas.tsx:332–372 at the checked-out state.
The change adds double-press detection inside objDown, which is a discrete pointer-down event handler, not a hot path:
The added work per press is O(1): one ref read, one performance.now(), one small object allocation ({ id, at, x, y }), and one Math.hypot. This runs only on a user-initiated pointerdown on an object — not inside a render loop, animation frame, or per-plop iteration — so the per-event cost is negligible.
lastPress is a single-slot useRef holding one small object at a time (reset to null on the second press). No unbounded growth, no accumulation, no collection retained across events.
No new loops, queries, copies of large structures, or blocking calls are introduced. The removed onDoubleClick prop slightly reduces per-object render props on the <g>, if anything a marginal win.
The two module-ish constants (DOUBLE_PRESS_MS, DOUBLE_PRESS_PX) are re-bound each render but are primitive literals — immaterial.
Nothing in this diff regresses efficiency in a way I can justify flagging.
🧯 Error handling & edge cases — No material issues found
I've verified the change against the actual code. Let me confirm my understanding of the unhappy paths:
Dimmed siblings (line 354): st.focusId != null && o.id !== st.focusId exactly matches the dim computation at line 571, so the old !dim guard on onDoubleClick is preserved — dimmed objects return before reaching the double-press block.
armedKind/armedPlant paths return before the double-press block, so a double-press while armed places plops as before and never focuses.
Second-press pointerup (line 466-467): drag.current was cleared, so onCanvasUp hits if (!d) return — nothing spurious selects into Plot, as the comment claims.
track() (line 261) wraps setPointerCapture in try/catch and capture releases implicitly on pointerup; pts is deleted on up. No leaked capture across the two presses.
Non-plantable double-press: if (o.plantable) guards the focusObject call; the object simply stays selected from the first press. No crash, no dangling state.
Review
VERDICT: No material issues found
Through the error-handling & edge-cases lens this change is clean. I checked the paths I'd expect to break:
No swallowed errors introduced; focusObject and track already handle their own failure modes (setPointerCapture is in a try/catch, focusObject no-ops when svgRef.current is null).
No missing cleanup: lastPress.current and drag.current are both reset on the double-press branch (lines 361–362), and stale lastPress from an earlier press is defended by the same-id + <400ms + <12px triple guard, so a false-positive double from two unrelated presses is effectively impossible.
Boundary/nil cases hold: prev is null-checked before use; performance.now() is monotonic (no overflow/clock-skew concern); Math.hypot on clientX/Y can't NaN here.
Behavioral parity with the removed onDoubleClick={o.plantable && !dim ? ...} prop is preserved — both the !dim guard (via the early return at line 354) and the o.plantable guard (line 363) survive. The prop ran regardless of canEdit, and so does the new path, so no read-only-viewer regression is introduced by this diff.
One non-lens observation, verified while tracing but not something I'm raising as a finding: a read-only viewer can trigger focusObject (which flips the rail to the plant palette) by double-clicking a plantable bed — but that was already true of the removed onDoubleClick, so it's neither new nor within this lens.
This change is pure client-side pointer-event bookkeeping with no security-relevant data flows.
lastPress.current stores only numeric fields derived from pointer events (id: number, at: number, x, y). The id is a server-issued integer primary key, not user-controlled input. The ref is useRef-scoped to the component instance — no global state, no cross-session leak.
focusObject(o) (verified by reading Canvas.tsx) makes zero network requests and performs only local Zustand store mutations (focus ID, selection, tab, camera). No untrusted content is rendered.
o.plantable guard is checked before calling focusObject, consistent with the surrounding code.
performance.now() and Math.hypot are browser-internal, not attacker-controlled.
Removal of onDoubleClick eliminates an event handler that was already dead (capture retargets it to the root); no security surface change.
Nothing in this diff touches authn/authz, network calls, serialization, template rendering, or any tainted-input-to-sink path.
🎯 Correctness — Minor issues
I've confirmed the relevant code sections. Here's what I verified:
onCanvasDown (lines 319–330): no reference to lastPress anywhere in the function — CONFIRMED.
lastPress.current is written only inside objDown at lines 357 and 361 — CONFIRMED.
dim definition at line 571: const dim = focusId != null && o.id !== focusId — matches the draft's claim exactly — CONFIRMED.
The scenario is structurally valid: a pointerdown on canvas between two pointerdowns on the same object does not clear lastPress, so the second object pointerdown can match the first and trigger focusObject spuriously.
The finding survives verification.
Minor issues
The double-press detection logic is sound overall — the !dim equivalence is confirmed (dim is exactly focusId != null && o.id !== focusId, which the existing objDown guard already enforces), pointer capture releases automatically on pointerup, and the 400 ms / 12 px constants are reasonable. One correctness gap:
lastPress.current is not cleared on non-object interactions (web/src/editor/Canvas.tsx, line 354 — the assignment site)
lastPress.current is only written inside objDown. Clicking on the canvas background fires canvasDown, which does not touch lastPress. As a result a rapid sequence click-bed-A → click-canvas → click-bed-A (all three within 400 ms) has prev.id === o.id and now − prev.at < 400 hold on the third press, and focusObject fires even though the user clicked away in between. The browser-native dblclick resets the window when the click lands on a different target; this implementation does not replicate that.
In practice 400 ms for three distinct pointer events is tight enough that this will be rare, but it is a real divergence from the expected "double-click" contract. The fix is straightforward: set lastPress.current = null in canvasDown (and wherever else the user clearly begins a new interaction context, e.g. when a pan drag starts).
🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed by the actual code:
lastPress is declared at line 338 — well outside the useRef group at lines 87–97.
DOUBLE_PRESS_MS and DOUBLE_PRESS_PX are at lines 339–340 inside the component body, while the established module-level constants (WHEEL_SENSITIVITY, ANIM_MS, REFIT_THRESHOLD_PX, FT) live at lines 38–41.
Minor issues
lastPress ref declared out-of-group (web/src/editor/Canvas.tsx:338): All other useRef hooks are declared together at lines 87–97. lastPress is inserted ~250 lines later, next to the function that uses it. A reader scanning for "which refs does this component own?" will miss it. It should sit with its peers at the top of the hook block — the proximity to objDown doesn't add anything because refs don't have render-time side-effects.
DOUBLE_PRESS_MS / DOUBLE_PRESS_PX declared inside the component (web/src/editor/Canvas.tsx:339): The file's established pattern puts config literals at module level (WHEEL_SENSITIVITY, ANIM_MS, REFIT_THRESHOLD_PX, FT at lines 38–41). These two constants are recreated as fresh bindings on every render and are buried inside the component body where they're less discoverable. They should join the module-level block; the only argument for keeping them inside (co-location) is undercut by the fact that ANIM_MS and REFIT_THRESHOLD_PX are equally "local" in concept and still live at module level.
No other cleanliness issues in scope — the removal of the dead onDoubleClick prop is clean, and the comment explaining why it can't come back is appropriate.
⚡ Performance — No material issues found
Based on my exploration, I can now complete the performance review.
No material issues found
The change adds logic only to the objDown event handler — a user-interaction path, not a render loop. Walking through each addition:
lastPress ref — a single useRef allocation at mount; the object written on press ({ id, at, x, y }) is created once per pointer-down, user-triggered, nowhere near the hot render path.
performance.now() — negligible cost, called only on pointer-down.
DOUBLE_PRESS_MS / DOUBLE_PRESS_PX inside the component body — they're primitive number bindings, no heap allocation. Worth knowing: the component already recreates far heavier things (handler closures, memoized arrays) on every render; two number bindings aren't a meaningful addition. Hoisting them to module scope would be cleaner style but the cost difference is immeasurable.
Math.hypot(…) — O(1), pointer-down only.
The existing render-time cost (curried objDown(o) called per object inside .map(), producing a new closure per object per render) predates this PR and is untouched by it. No new work lands on the render path.
🧯 Error handling & edge cases — No material issues found
I now have everything I need. Let me verify the full sequence: track() adds the pointer to pts.current, the double-press path sets drag.current = null and returns, then onCanvasUp fires because the pointer is captured on the SVG root.
onCanvasUp at line 459:
pts.current.delete(e.pointerId)// ← cleans up pts.current ✓
...constd=drag.currentdrag.current=nullif(!d)return// ← exits cleanly when drag is null ✓
Both concerns check out: pts.current is cleaned up by the normal up handler, and drag.current = null is explicitly guarded.
Verdict: No material issues found
The double-press detection path is error-safe under the error-handling lens:
pts.current cleanup — track(e) always runs before the double-press check, adding the pointer to pts.current. The early return in objDown does not skip cleanup because cleanup happens in onCanvasUp via pts.current.delete(e.pointerId) (line 460), which fires on every pointer-up regardless of drag state. Verified by reading onCanvasUp.
drag.current = null on second press — onCanvasUp reads const d = drag.current and immediately guards if (!d) return (line 467), so the up event for the second press exits cleanly with no spurious selection or mutation. Verified by reading lines 459–505.
Non-plantable object double-press — The second press for a non-plantable object takes the if (o.plantable) focusObject(o) branch silently (does nothing, returns). This matches the old onDoubleClick={o.plantable && !dim ? … : undefined} guard, so behavior is equivalent.
First-press up interaction — The first press's pointerup does select-into-Plot (setSel + setTab('plot')), but the second press's objDown immediately calls focusObject which overrides to setTab('toolkit'). These are synchronous state updates within the same event loop; no observable flicker.
performance.now() usage — Standard, never throws, never returns null.
Through the security lens, this change has no authn/authz, injection, SSRF, deserialization, secret-handling, or untrusted-input surface. The only inputs are local pointer coordinates (e.clientX/e.clientY) and performance.now(), all used solely for in-browser double-press timing/distance gating that calls focusObject(o) on an already-loaded EditorObject. I verified the surrounding objDown path (Canvas.tsx:342–372): the new block sits after the armedKind/armedPlant early returns, so it cannot be reached while a plant/kind is armed, and it only acts on o already resolved from local state — no tainted or cross-origin data flows into it. Nothing to flag.
🎯 Correctness — No material issues found
I have verified the relevant code paths. Let me finalize the review.
The diff correctly identifies and fixes the setPointerCapture retargeting issue. The double-press detection logic in objDown is sound for the plantable bed case. However, I found one correctness regression introduced by the removal of onDoubleClick.
Finding: Removed non-plantable double-click focus — The old onDoubleClick={o.plantable && !dim ? () => focusObject(o) : undefined} only focused plantable beds, so non-plantable object double-click is unchanged. But the new code path's behavior on the first press of a double-click is the concern. Let me re-examine.
Let me re-read the drag flow carefully to confirm whether the first press of a now-recognized double-click correctly leaves selection intact.
The flow for a double-click on a plantable bed (desktop, canEdit, no armed plant, no existing focus):
First press: objDown → lastPress set → not a double → drag.current = { t: 'obj', ... }. On pointerup (onCanvasUp): d.t === 'obj', !d.moved → setSel({type:'object', id}) and setTab('plot'). drag.current cleared.
Second press: objDown → prev matches within 400ms/12px → lastPress.current = null, drag.current = null, focusObject(o) → focuses bed, selects object, sets tab to toolkit, animates camera.
This matches the description: first press selects, second focuses/plants. Previously the first press selected and the second selected again (no focus). So the fix works as intended for plantable beds.
I've verified the change is correct for its stated goal. No material correctness issues in my lens.
No material issues found.
The double-press detection (Canvas.tsx:355-365) is correctly placed after the armed-plant/armed-kind early returns and the dimmed-sibling inert check, so it only fires for an editable, non-dimmed object — matching the old onDoubleClick={o.plantable && !dim ? ...} guard.
The lastPress.current = null + drag.current = null on the second press correctly suppresses drag and prevents the matching pointerup from re-selecting into the Plot tab, since onCanvasUp returns early when d is null (Canvas.tsx:467). Verified by reading onCanvasUp.
lastPress is only ever written in objDown (verified by grepping the whole src tree for lastPress/DOUBLE_PRESS), so there is no stale-state reset issue across renders. The ref persists correctly; the only minor effect is that an aborted second press (e.g. a long pause or movement >12px) leaves lastPress pointing at the prior press, but the next qualifying press still evaluates against the most recent press since it's overwritten before the check — actually, it's overwritten unconditionally on line 357 before the comparison, which is the correct ordering for double-press detection.
The threshold values (400ms, 12px) are reasonable for a manual double-click and are within the same order as the drag threshold (thresh = 3px desktop / 7px touch); they don't collide because the double-press check uses client-space deltas while drag uses world-space deltas, and the double-press short-circuits before a drag is even started.
The non-plantable case is unchanged: if (o.plantable) focusObject(o) guards focus to plantable beds only, exactly as the removed onDoubleClick did.
I confirmed by reading track (Canvas.tsx:261), focusObject (Canvas.tsx:171), objDown and the surrounding handlers (Canvas.tsx:342-405), onCanvasUp (Canvas.tsx:459-505), and the render of each object <g> (Canvas.tsx:578-593), and by grepping for lastPress/onDoubleClick across web/src.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/editor/Canvas.tsx:339-340 — DOUBLE_PRESS_MS / DOUBLE_PRESS_PX are declared inside the component body, but they're pure compile-time tuning constants with no per-render dependency. Every other such tuning constant in this file lives at module top-level (WHEEL_SENSITIVITY, ANIM_MS, REFIT_THRESHOLD_PX, FT at lines 38-41). The in-body uppercase consts that do exist (GW, GH, s at lines 114-116) are derived from props/state each render, so they have a reason to be local; these two don't. Declaring them in-body (a) recreates two bindings every render for no reason, and (b) breaks the file's own convention for where magic-number thresholds live, making them harder to spot when someone comes looking to tune double-click sensitivity. Suggested fix: hoist both to the module-level block beside WHEEL_SENSITIVITY.
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, the change is clean:
objDown runs once per pointer press (not a hot loop), so the added performance.now() call and Math.hypot arithmetic are negligible — no concern.
lastPress is a single useRef holding one small record; it's overwritten on each press and cleared on a double, so there's no unbounded growth or accumulation.
The two new constants (DOUBLE_PRESS_MS, DOUBLE_PRESS_PX) are primitive numbers recreated per render, but they're cheap literals and objDown already allocates a closure per object per render (pre-existing), so this adds nothing measurable. Moving them to module scope would be a micro-optimization, not worth flagging.
The diff actually removes the onDoubleClick handler, slightly reducing per-render closure allocation.
No new allocations on any hot path, no loops, no queries, no blocking calls.
I verified objDown's placement and the ref declaration by reading Canvas.tsx:325-385; nothing in the surrounding code introduces a repeated/allocation-heavy path that the new logic feeds into.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling & edge-cases lens, I traced every unhappy path the diff introduces and found the new logic to be robust. Key verifications (all by reading web/src/editor/Canvas.tsx):
Dangling drag on second press: The double-press branch sets drag.current = null (line 362). I confirmed the pointerup handler (onCanvasUp, lines 465-467) treats d === null as a no-op early-return, so a swallowed second press leaves no half-finished drag/select. Even if the first press's pointerup was missed, onPointerCancel is routed to the same handler (line 553), which clears drag.current and pts.current.
Pointer capture on the swallowed press: track(e) (line 344) runs before the double-press branch, so the second press still calls setPointerCapture (wrapped in try/catch at 264-268) and adds to pts.current. The pointerup handler deletes the pointer id (line 460), so no leak. This matches pre-existing behavior — capture was always set in track for every press.
Rolling lastPress window: lastPress.current is overwritten on every qualifying press (line 357) before the match check, and reset to null on a match (line 361). Pressing a different object, or pressing past the 400ms/12px window, simply re-bases the window — no stale state. The dimmed-sibling early return (line 354) correctly runs before lastPress is touched, so inert objects can't poison the detector.
Pinch / armed placement: The double-press branch sits after the pts.current.size === 2 → pinchStart() early return (line 345) and the armedKind/armedPlant returns (348-352), so it can't fire mid-pinch or while placing. Correct.
Non-plantable objects: The if (o.plantable) focusObject(o) guard (line 363) means a non-plantable second press returns early with no drag and no focus. The first press already selected the object via its pointerup (lines 473-477), so selection isn't lost. The old onDoubleClick likewise did nothing for non-plantable objects — no regression.
Read-only (canEdit false): The double-press branch (line 358) executes before the canEdit guard (line 366), so double-clicking a plantable bed focuses it even in read-only mode. I verified the old onDoubleClick={o.plantable && !dim ? () => focusObject(o) : undefined} had nocanEdit guard either, so this preserves the prior behavior rather than introducing it. Not a new defect.
No swallowed errors, missing cleanup, panics, nil/boundary, or overflow concerns are introduced by this diff.
Reviewed the double-click-to-plant fix through the security lens. The change replaces a broken onDoubleClick prop with manual double-press detection inside objDown using a lastPress ref. Verified the following:
No injection surfaces: lastPress stores only numeric IDs, timestamps, and pointer coordinates; no user strings are evaluated or rendered.
No authn/authz gap: the dim guard (focusId != null && o.id !== st.focusId) still applies via the early return above the new double-press logic, so dimmed objects remain inert.
No SSRF or deserialization: no network requests or structured data parsing are introduced.
Pointer-capture interaction is safe: setPointerCapture retargets compatibility click/dblclick events but does not block pointerdown dispatch, which is why the onPointerDown-based detection works correctly.
The lastPress ref is single-valued and scoped to the component; no memory-exhaustion or race-condition concern.
No security-relevant defects identified.
🎯 Correctness — No material issues found
No material issues found
I examined the double-click detection logic through the correctness lens, tracing the full pointer event flow and state transitions. The implementation is sound:
objDown records each press in lastPress and checks the next press against the 400 ms / 12 px thresholds before any drag starts. On a detected double-press it nulls drag.current, so onCanvasUp correctly returns early and cannot overwrite the focus with a single-click selection. Verified by reading objDown (lines 342–372) and onCanvasUp (lines 459–505).
The st.focusId != null && o.id !== st.focusId guard correctly keeps dimmed siblings inert; the double-click check sits after it, so double-clicking a dimmed sibling is correctly ignored.
Delegation via plopDown → objDown(o) when st.focusId !== p.objectId means double-clicking a plop in an unfocused bed focuses the bed, as intended.
The unconditional lastPress.current = { ... } before the if (prev && ...) check is safe because prev is read first; resetting lastPress.current = null inside the true branch prevents triple-click false positives.
track(e) (which captures the pointer on the SVG root) is called before the double-click check, but that does not affect correctness because the detection is based on successive pointerdown events, not retargeted dblclick.
No logic bugs or semantic correctness issues introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
The lastPress ref allocates one small object per object press, but this is negligible (not in a hot loop, bounded to one entry, and far below any perceptible threshold).
Removing the onDoubleClick prop from every <g> actually reduces the number of React-managed event listeners per rendered object by one, which is a tiny net improvement.
The focusObject call on double-click triggers store updates and a camera animation, but that is intentional and only happens on the rare double-click gesture, not on normal interactions.
🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/editor/Canvas.tsx:318 — onCanvasDown never invalidates lastPress, so a canvas click between two bed clicks does not break the double-click chain. Verified by reading the file: lastPress is only mutated inside objDown, and onCanvasDown (the empty-canvas handler) does not touch it. Sequence: click bed A → click empty canvas → click bed A again within 400 ms/12 px → false double-click triggers focusObject(A) instead of a normal select/drag. Fix: add lastPress.current = null at the top of onCanvasDown.
web/src/editor/Canvas.tsx:357 — lastPress is updated for every pointer button, so non-primary clicks (right-click, middle-click) can participate in or complete a double-click. Verified by reading objDown: the ref is written unconditionally before the e.button check, and the if (prev && …) block that fires focusObject does not inspect e.button. Native dblclick fires only for the primary button; right-clicking a bed twice within 400 ms should not focus it. Fix: gate the lastPress.current = {…} assignment and the double-click check with e.button === 0 (or e.buttons === 1).
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 3 findings (2 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | lastPress useRef declared far from the component's other refs (lines 87-97), breaking the established grouping pattern | `web/src/editor/Canvas.tsx:338` | 3/4 | maintainability |
| 🟠 | Double-click detection accepts non-primary pointer buttons, unlike native dblclick | `web/src/editor/Canvas.tsx:354` | 2/4 | correctness, error-handling |
<details><summary>1 single-model finding (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Canvas clicks do not invalidate lastPress, allowing false double-clicks across empty-space interactions | `web/src/editor/Canvas.tsx:318` | kimi-k2.6:cloud | error-handling |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</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 is a purely client-side interaction change — double-click detection for focusing a bed in the SVG canvas editor. Reviewed through the Security lens:
- **No untrusted data reaches any sink.** The new code reads only `performance.now()`, `e.clientX/Y`, and the object's numeric `id` — all local browser/editor values. They feed `Math.hypot`/timestamp comparisons and `focusObject(o)`; none is used in a query, path, template, URL, or `dangerouslySetInnerHTML`. No injection surface. (Verified by reading `Canvas.tsx:355-364`.)
- **No authz bypass.** `focusObject` (verified at `Canvas.tsx:171-192`) only mutates local editor UI state (`setFocus`/`setSel`/`setTab`/`camera`) — it grants no privileges. The actual mutation path (`placePlop`, `Canvas.tsx:301-315`) still gates on `latest.current.canEdit`, and per the project's architecture the real permission enforcement lives server-side in `internal/service`. Double-clicking merely arms the plant palette; it cannot plant in a bed the user can't edit.
- **`o.plantable` guard** on the double-press branch matches the removed `onDoubleClick` gate, so no code path became reachable that wasn't already reachable via the inspector's "Plant this". No insecure default introduced.
- No secrets, deserialization, SSRF, or new input-validation gaps — there is no network or data-parsing code in this diff.
Nothing in my lane is materially wrong here.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified the change against the checked-out code. Let me walk through my correctness analysis.
**What I checked:**
- Traced the full double-click sequence through `objDown` (Canvas.tsx:342), `onCanvasUp` (459), `track`/`setPointerCapture` (261), and `focusObject` (171).
- Confirmed the plop-delegation path (`plopDown` line 381 → `objDown(o)(e)`) so double-clicking a plant records the *bed's* id and focuses the bed, as the PR claims.
- Confirmed `dim` (line 571 = `focusId != null && o.id !== focusId`) is exactly the early-return guard at line 354, so dropping `!dim` from the removed `onDoubleClick` loses nothing — dimmed siblings still return early.
- Confirmed no other `onDoubleClick`/`dblclick` handlers exist (only the aria-label copy at line 557).
**Sequence trace (real double-click: down1, up1, down2, up2):**
- down1: `prev` is null/stale → records `lastPress`, falls through, sets `drag='obj'`.
- up1: `d.t==='obj'`, not moved → selects object into Plot, clears drag.
- down2: `prev.id===o.id`, within 400 ms / 12 px → double detected; nulls `lastPress` and `drag`, calls `focusObject` (plantable) → focus + `toolkit` tab. End state correct.
- up2: `drag` is null → returns early, nothing selected. Matches the comment.
**Edge cases that hold up:**
- `lastPress` reset to null on a detected double prevents a triple-click re-triggering.
- id-match requirement blocks cross-object false positives (press A then B never fires a double).
- `performance.now()` is monotonic; `Math.hypot` on `clientX/Y` in screen px with a 12 px tolerance is standard.
- Non-plantable object double-click: second press nulls drag, skips focus, returns → object stays selected from click 1. No regression.
- Pointer capture is auto-released on pointerup; `pts.current.delete` runs before the early return in `onCanvasUp` — no leak.
The thresholds (400 ms, 12 px) are ordinary double-click tolerances, not domain conversion factors, so there's no magic-number derivation to redo here.
One nuance I considered but do not think is a bug: in read-only mode (`canEdit` false), the second press still calls `focusObject`, which flips the rail to the `toolkit` (plant) tab. But the original `onDoubleClick` had no `canEdit` guard either, so this matches the pre-existing intended design (the handler was merely dead due to capture), and planting itself is still blocked in `placePlop`. Not a correctness regression.
## VERDICT: No material issues found
The double-press detection is logically sound: the state machine (record → compare id/time/distance → reset) is correct, the pointerup path is properly neutralized, the plop delegation preserves the "double-click a plant focuses its bed" behavior, and the removed `onDoubleClick` was provably equivalent to the retained early-return guard. Nothing in the correctness lens is materially wrong.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
The finding is confirmed: module-level tuning constants live at lines 38–41 (`WHEEL_SENSITIVITY`, `ANIM_MS`, `REFIT_THRESHOLD_PX`, `FT`), while the new `DOUBLE_PRESS_MS`/`DOUBLE_PRESS_PX` sit in the component body at 339–340. The only other in-component ALL_CAPS names (`GW`/`GH` at 114–115) are derived from garden props, so they legitimately stay inside — they don't undermine the convention claim. The `lastPress` ref correctly remains in-component. Finding holds.
## VERDICT: Minor issues
- **`web/src/editor/Canvas.tsx:339-340` — tuning constants placed inside the component body, against the file's own convention.** `DOUBLE_PRESS_MS` and `DOUBLE_PRESS_PX` are plain magic-number tuning constants, but they're declared inside the `Canvas` component function (recreated every render). Every other constant of this kind in the file lives at module top — `WHEEL_SENSITIVITY`, `ANIM_MS`, `REFIT_THRESHOLD_PX`, `FT` (lines 38–41). The only other in-component ALL_CAPS names (`GW`/`GH`) are values derived from garden props, so they must stay local; these two are compile-time constants that belong beside `ANIM_MS` et al. The `lastPress` ref correctly stays in the component. Trivial, but it's exactly the "doesn't follow the pattern the surrounding code uses" case.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the Performance lens by reading `web/src/editor/Canvas.tsx:332–372` at the checked-out state.
The change adds double-press detection inside `objDown`, which is a discrete pointer-down event handler, not a hot path:
- The added work per press is O(1): one ref read, one `performance.now()`, one small object allocation (`{ id, at, x, y }`), and one `Math.hypot`. This runs only on a user-initiated `pointerdown` on an object — not inside a render loop, animation frame, or per-plop iteration — so the per-event cost is negligible.
- `lastPress` is a single-slot `useRef` holding one small object at a time (reset to `null` on the second press). No unbounded growth, no accumulation, no collection retained across events.
- No new loops, queries, copies of large structures, or blocking calls are introduced. The removed `onDoubleClick` prop slightly reduces per-object render props on the `<g>`, if anything a marginal win.
- The two module-ish constants (`DOUBLE_PRESS_MS`, `DOUBLE_PRESS_PX`) are re-bound each render but are primitive literals — immaterial.
Nothing in this diff regresses efficiency in a way I can justify flagging.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've verified the change against the actual code. Let me confirm my understanding of the unhappy paths:
- **Dimmed siblings** (line 354): `st.focusId != null && o.id !== st.focusId` exactly matches the `dim` computation at line 571, so the old `!dim` guard on `onDoubleClick` is preserved — dimmed objects `return` before reaching the double-press block.
- **`armedKind`/`armedPlant`** paths return before the double-press block, so a double-press while armed places plops as before and never focuses.
- **Second-press pointerup** (line 466-467): `drag.current` was cleared, so `onCanvasUp` hits `if (!d) return` — nothing spurious selects into Plot, as the comment claims.
- **`track()`** (line 261) wraps `setPointerCapture` in try/catch and capture releases implicitly on pointerup; `pts` is deleted on up. No leaked capture across the two presses.
- **Non-plantable double-press**: `if (o.plantable)` guards the `focusObject` call; the object simply stays selected from the first press. No crash, no dangling state.
## Review
**VERDICT: No material issues found**
Through the error-handling & edge-cases lens this change is clean. I checked the paths I'd expect to break:
- No swallowed errors introduced; `focusObject` and `track` already handle their own failure modes (`setPointerCapture` is in a try/catch, `focusObject` no-ops when `svgRef.current` is null).
- No missing cleanup: `lastPress.current` and `drag.current` are both reset on the double-press branch (lines 361–362), and stale `lastPress` from an earlier press is defended by the same-`id` + `<400ms` + `<12px` triple guard, so a false-positive double from two unrelated presses is effectively impossible.
- Boundary/nil cases hold: `prev` is null-checked before use; `performance.now()` is monotonic (no overflow/clock-skew concern); `Math.hypot` on clientX/Y can't NaN here.
- Behavioral parity with the removed `onDoubleClick={o.plantable && !dim ? ...}` prop is preserved — both the `!dim` guard (via the early `return` at line 354) and the `o.plantable` guard (line 363) survive. The prop ran regardless of `canEdit`, and so does the new path, so no read-only-viewer regression is introduced by this diff.
One non-lens observation, verified while tracing but not something I'm raising as a finding: a read-only viewer can trigger `focusObject` (which flips the rail to the plant palette) by double-clicking a plantable bed — but that was already true of the removed `onDoubleClick`, so it's neither new nor within this lens.
</details>
</details>
<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>
**No material issues found.**
This change is pure client-side pointer-event bookkeeping with no security-relevant data flows.
- **`lastPress.current`** stores only numeric fields derived from pointer events (`id: number`, `at: number`, `x`, `y`). The `id` is a server-issued integer primary key, not user-controlled input. The ref is `useRef`-scoped to the component instance — no global state, no cross-session leak.
- **`focusObject(o)`** (verified by reading `Canvas.tsx`) makes zero network requests and performs only local Zustand store mutations (focus ID, selection, tab, camera). No untrusted content is rendered.
- **`o.plantable`** guard is checked before calling `focusObject`, consistent with the surrounding code.
- **`performance.now()`** and `Math.hypot` are browser-internal, not attacker-controlled.
- Removal of `onDoubleClick` eliminates an event handler that was already dead (capture retargets it to the root); no security surface change.
Nothing in this diff touches authn/authz, network calls, serialization, template rendering, or any tainted-input-to-sink path.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
I've confirmed the relevant code sections. Here's what I verified:
1. `onCanvasDown` (lines 319–330): no reference to `lastPress` anywhere in the function — CONFIRMED.
2. `lastPress.current` is written only inside `objDown` at lines 357 and 361 — CONFIRMED.
3. `dim` definition at line 571: `const dim = focusId != null && o.id !== focusId` — matches the draft's claim exactly — CONFIRMED.
4. The scenario is structurally valid: a pointerdown on canvas between two pointerdowns on the same object does not clear `lastPress`, so the second object pointerdown can match the first and trigger `focusObject` spuriously.
The finding survives verification.
---
**Minor issues**
The double-press detection logic is sound overall — the `!dim` equivalence is confirmed (`dim` is exactly `focusId != null && o.id !== focusId`, which the existing `objDown` guard already enforces), pointer capture releases automatically on `pointerup`, and the 400 ms / 12 px constants are reasonable. One correctness gap:
- **`lastPress.current` is not cleared on non-object interactions** (`web/src/editor/Canvas.tsx`, line 354 — the assignment site)
`lastPress.current` is only written inside `objDown`. Clicking on the canvas background fires `canvasDown`, which does not touch `lastPress`. As a result a rapid sequence **click-bed-A → click-canvas → click-bed-A** (all three within 400 ms) has `prev.id === o.id` and `now − prev.at < 400` hold on the third press, and `focusObject` fires even though the user clicked away in between. The browser-native `dblclick` resets the window when the click lands on a different target; this implementation does not replicate that.
In practice 400 ms for three distinct pointer events is tight enough that this will be rare, but it is a real divergence from the expected "double-click" contract. The fix is straightforward: set `lastPress.current = null` in `canvasDown` (and wherever else the user clearly begins a new interaction context, e.g. when a pan drag starts).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings are confirmed by the actual code:
1. `lastPress` is declared at line 338 — well outside the `useRef` group at lines 87–97.
2. `DOUBLE_PRESS_MS` and `DOUBLE_PRESS_PX` are at lines 339–340 inside the component body, while the established module-level constants (`WHEEL_SENSITIVITY`, `ANIM_MS`, `REFIT_THRESHOLD_PX`, `FT`) live at lines 38–41.
---
**Minor issues**
- **`lastPress` ref declared out-of-group** (`web/src/editor/Canvas.tsx:338`): All other `useRef` hooks are declared together at lines 87–97. `lastPress` is inserted ~250 lines later, next to the function that uses it. A reader scanning for "which refs does this component own?" will miss it. It should sit with its peers at the top of the hook block — the proximity to `objDown` doesn't add anything because refs don't have render-time side-effects.
- **`DOUBLE_PRESS_MS` / `DOUBLE_PRESS_PX` declared inside the component** (`web/src/editor/Canvas.tsx:339`): The file's established pattern puts config literals at module level (`WHEEL_SENSITIVITY`, `ANIM_MS`, `REFIT_THRESHOLD_PX`, `FT` at lines 38–41). These two constants are recreated as fresh bindings on every render and are buried inside the component body where they're less discoverable. They should join the module-level block; the only argument for keeping them inside (co-location) is undercut by the fact that `ANIM_MS` and `REFIT_THRESHOLD_PX` are equally "local" in concept and still live at module level.
No other cleanliness issues in scope — the removal of the dead `onDoubleClick` prop is clean, and the comment explaining why it can't come back is appropriate.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Based on my exploration, I can now complete the performance review.
---
**No material issues found**
The change adds logic only to the `objDown` event handler — a user-interaction path, not a render loop. Walking through each addition:
- **`lastPress` ref** — a single `useRef` allocation at mount; the object written on press (`{ id, at, x, y }`) is created once per pointer-down, user-triggered, nowhere near the hot render path.
- **`performance.now()`** — negligible cost, called only on pointer-down.
- **`DOUBLE_PRESS_MS` / `DOUBLE_PRESS_PX` inside the component body** — they're primitive number bindings, no heap allocation. Worth knowing: the component already recreates far heavier things (handler closures, memoized arrays) on every render; two number bindings aren't a meaningful addition. Hoisting them to module scope would be cleaner style but the cost difference is immeasurable.
- **`Math.hypot(…)`** — O(1), pointer-down only.
The existing render-time cost (curried `objDown(o)` called per object inside `.map()`, producing a new closure per object per render) predates this PR and is untouched by it. No new work lands on the render path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I now have everything I need. Let me verify the full sequence: `track()` adds the pointer to `pts.current`, the double-press path sets `drag.current = null` and returns, then `onCanvasUp` fires because the pointer is captured on the SVG root.
**`onCanvasUp` at line 459:**
```ts
pts.current.delete(e.pointerId) // ← cleans up pts.current ✓
...
const d = drag.current
drag.current = null
if (!d) return // ← exits cleanly when drag is null ✓
```
Both concerns check out: `pts.current` is cleaned up by the normal up handler, and `drag.current = null` is explicitly guarded.
---
**Verdict: No material issues found**
The double-press detection path is error-safe under the error-handling lens:
- **`pts.current` cleanup** — `track(e)` always runs before the double-press check, adding the pointer to `pts.current`. The early `return` in `objDown` does not skip cleanup because cleanup happens in `onCanvasUp` via `pts.current.delete(e.pointerId)` (line 460), which fires on every pointer-up regardless of drag state. Verified by reading `onCanvasUp`.
- **`drag.current = null` on second press** — `onCanvasUp` reads `const d = drag.current` and immediately guards `if (!d) return` (line 467), so the up event for the second press exits cleanly with no spurious selection or mutation. Verified by reading lines 459–505.
- **Non-plantable object double-press** — The second press for a non-plantable object takes the `if (o.plantable) focusObject(o)` branch silently (does nothing, returns). This matches the old `onDoubleClick={o.plantable && !dim ? … : undefined}` guard, so behavior is equivalent.
- **First-press up interaction** — The first press's `pointerup` does select-into-Plot (`setSel` + `setTab('plot')`), but the second press's `objDown` immediately calls `focusObject` which overrides to `setTab('toolkit')`. These are synchronous state updates within the same event loop; no observable flicker.
- **`performance.now()` usage** — Standard, never throws, never returns null.
</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, this change has no authn/authz, injection, SSRF, deserialization, secret-handling, or untrusted-input surface. The only inputs are local pointer coordinates (`e.clientX`/`e.clientY`) and `performance.now()`, all used solely for in-browser double-press timing/distance gating that calls `focusObject(o)` on an already-loaded `EditorObject`. I verified the surrounding `objDown` path (Canvas.tsx:342–372): the new block sits after the `armedKind`/`armedPlant` early returns, so it cannot be reached while a plant/kind is armed, and it only acts on `o` already resolved from local state — no tainted or cross-origin data flows into it. Nothing to flag.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I have verified the relevant code paths. Let me finalize the review.
The diff correctly identifies and fixes the `setPointerCapture` retargeting issue. The double-press detection logic in `objDown` is sound for the plantable bed case. However, I found one correctness regression introduced by the removal of `onDoubleClick`.
**Finding: Removed non-plantable double-click focus** — The old `onDoubleClick={o.plantable && !dim ? () => focusObject(o) : undefined}` only focused plantable beds, so non-plantable object double-click is unchanged. But the new code path's behavior on the *first* press of a double-click is the concern. Let me re-examine.
Let me re-read the drag flow carefully to confirm whether the first press of a now-recognized double-click correctly leaves selection intact.
The flow for a double-click on a plantable bed (desktop, canEdit, no armed plant, no existing focus):
1. First press: `objDown` → `lastPress` set → not a double → `drag.current = { t: 'obj', ... }`. On pointerup (`onCanvasUp`): `d.t === 'obj'`, `!d.moved` → `setSel({type:'object', id})` and `setTab('plot')`. `drag.current` cleared.
2. Second press: `objDown` → `prev` matches within 400ms/12px → `lastPress.current = null`, `drag.current = null`, `focusObject(o)` → focuses bed, selects object, sets tab to toolkit, animates camera.
This matches the description: first press selects, second focuses/plants. Previously the first press selected and the second selected again (no focus). So the fix works as intended for plantable beds.
I've verified the change is correct for its stated goal. No material correctness issues in my lens.
**No material issues found.**
- The double-press detection (`Canvas.tsx:355-365`) is correctly placed after the armed-plant/armed-kind early returns and the dimmed-sibling inert check, so it only fires for an editable, non-dimmed object — matching the old `onDoubleClick={o.plantable && !dim ? ...}` guard.
- The `lastPress.current = null` + `drag.current = null` on the second press correctly suppresses drag and prevents the matching pointerup from re-selecting into the Plot tab, since `onCanvasUp` returns early when `d` is null (`Canvas.tsx:467`). Verified by reading `onCanvasUp`.
- `lastPress` is only ever written in `objDown` (verified by grepping the whole `src` tree for `lastPress`/`DOUBLE_PRESS`), so there is no stale-state reset issue across renders. The ref persists correctly; the only minor effect is that an aborted second press (e.g. a long pause or movement >12px) leaves `lastPress` pointing at the prior press, but the next qualifying press still evaluates against the *most recent* press since it's overwritten before the check — actually, it's overwritten unconditionally on line 357 before the comparison, which is the correct ordering for double-press detection.
- The threshold values (400ms, 12px) are reasonable for a manual double-click and are within the same order as the drag threshold (`thresh` = 3px desktop / 7px touch); they don't collide because the double-press check uses client-space deltas while drag uses world-space deltas, and the double-press short-circuits before a drag is even started.
- The non-plantable case is unchanged: `if (o.plantable) focusObject(o)` guards focus to plantable beds only, exactly as the removed `onDoubleClick` did.
I confirmed by reading `track` (`Canvas.tsx:261`), `focusObject` (`Canvas.tsx:171`), `objDown` and the surrounding handlers (`Canvas.tsx:342-405`), `onCanvasUp` (`Canvas.tsx:459-505`), and the render of each object `<g>` (`Canvas.tsx:578-593`), and by grepping for `lastPress`/`onDoubleClick` across `web/src`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
VERDICT: Minor issues
- `web/src/editor/Canvas.tsx:339-340` — `DOUBLE_PRESS_MS` / `DOUBLE_PRESS_PX` are declared inside the component body, but they're pure compile-time tuning constants with no per-render dependency. Every other such tuning constant in this file lives at module top-level (`WHEEL_SENSITIVITY`, `ANIM_MS`, `REFIT_THRESHOLD_PX`, `FT` at lines 38-41). The in-body uppercase consts that do exist (`GW`, `GH`, `s` at lines 114-116) are derived from props/state each render, so they have a reason to be local; these two don't. Declaring them in-body (a) recreates two bindings every render for no reason, and (b) breaks the file's own convention for where magic-number thresholds live, making them harder to spot when someone comes looking to tune double-click sensitivity. Suggested fix: hoist both to the module-level block beside `WHEEL_SENSITIVITY`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Through the performance lens, the change is clean:
- `objDown` runs once per pointer press (not a hot loop), so the added `performance.now()` call and `Math.hypot` arithmetic are negligible — no concern.
- `lastPress` is a single `useRef` holding one small record; it's overwritten on each press and cleared on a double, so there's no unbounded growth or accumulation.
- The two new constants (`DOUBLE_PRESS_MS`, `DOUBLE_PRESS_PX`) are primitive numbers recreated per render, but they're cheap literals and `objDown` already allocates a closure per object per render (pre-existing), so this adds nothing measurable. Moving them to module scope would be a micro-optimization, not worth flagging.
- The diff actually *removes* the `onDoubleClick` handler, slightly reducing per-render closure allocation.
- No new allocations on any hot path, no loops, no queries, no blocking calls.
I verified `objDown`'s placement and the ref declaration by reading `Canvas.tsx:325-385`; nothing in the surrounding code introduces a repeated/allocation-heavy path that the new logic feeds into.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## VERDICT: No material issues found
Through the error-handling & edge-cases lens, I traced every unhappy path the diff introduces and found the new logic to be robust. Key verifications (all by reading `web/src/editor/Canvas.tsx`):
- **Dangling drag on second press**: The double-press branch sets `drag.current = null` (line 362). I confirmed the pointerup handler (`onCanvasUp`, lines 465-467) treats `d === null` as a no-op early-return, so a swallowed second press leaves no half-finished drag/select. Even if the first press's pointerup was missed, `onPointerCancel` is routed to the same handler (line 553), which clears `drag.current` and `pts.current`.
- **Pointer capture on the swallowed press**: `track(e)` (line 344) runs before the double-press branch, so the second press still calls `setPointerCapture` (wrapped in try/catch at 264-268) and adds to `pts.current`. The pointerup handler deletes the pointer id (line 460), so no leak. This matches pre-existing behavior — capture was always set in `track` for every press.
- **Rolling `lastPress` window**: `lastPress.current` is overwritten on every qualifying press (line 357) before the match check, and reset to `null` on a match (line 361). Pressing a different object, or pressing past the 400ms/12px window, simply re-bases the window — no stale state. The dimmed-sibling early return (line 354) correctly runs before `lastPress` is touched, so inert objects can't poison the detector.
- **Pinch / armed placement**: The double-press branch sits after the `pts.current.size === 2` → `pinchStart()` early return (line 345) and the `armedKind`/`armedPlant` returns (348-352), so it can't fire mid-pinch or while placing. Correct.
- **Non-plantable objects**: The `if (o.plantable) focusObject(o)` guard (line 363) means a non-plantable second press returns early with no drag and no focus. The first press already selected the object via its pointerup (lines 473-477), so selection isn't lost. The old `onDoubleClick` likewise did nothing for non-plantable objects — no regression.
- **Read-only (`canEdit` false)**: The double-press branch (line 358) executes before the `canEdit` guard (line 366), so double-clicking a plantable bed focuses it even in read-only mode. I verified the old `onDoubleClick={o.plantable && !dim ? () => focusObject(o) : undefined}` had **no** `canEdit` guard either, so this preserves the prior behavior rather than introducing it. Not a new defect.
No swallowed errors, missing cleanup, panics, nil/boundary, or overflow concerns are introduced by this diff.
</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>
**No material issues found**
Reviewed the double-click-to-plant fix through the security lens. The change replaces a broken `onDoubleClick` prop with manual double-press detection inside `objDown` using a `lastPress` ref. Verified the following:
- No injection surfaces: `lastPress` stores only numeric IDs, timestamps, and pointer coordinates; no user strings are evaluated or rendered.
- No authn/authz gap: the `dim` guard (`focusId != null && o.id !== st.focusId`) still applies via the early return above the new double-press logic, so dimmed objects remain inert.
- No SSRF or deserialization: no network requests or structured data parsing are introduced.
- Pointer-capture interaction is safe: `setPointerCapture` retargets compatibility `click`/`dblclick` events but does not block `pointerdown` dispatch, which is why the `onPointerDown`-based detection works correctly.
- The `lastPress` ref is single-valued and scoped to the component; no memory-exhaustion or race-condition concern.
No security-relevant defects identified.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I examined the double-click detection logic through the correctness lens, tracing the full pointer event flow and state transitions. The implementation is sound:
- `objDown` records each press in `lastPress` and checks the next press against the 400 ms / 12 px thresholds before any drag starts. On a detected double-press it nulls `drag.current`, so `onCanvasUp` correctly returns early and cannot overwrite the focus with a single-click selection. Verified by reading `objDown` (lines 342–372) and `onCanvasUp` (lines 459–505).
- The `st.focusId != null && o.id !== st.focusId` guard correctly keeps dimmed siblings inert; the double-click check sits after it, so double-clicking a dimmed sibling is correctly ignored.
- Delegation via `plopDown` → `objDown(o)` when `st.focusId !== p.objectId` means double-clicking a plop in an unfocused bed focuses the bed, as intended.
- The unconditional `lastPress.current = { ... }` before the `if (prev && ...)` check is safe because `prev` is read first; resetting `lastPress.current = null` inside the true branch prevents triple-click false positives.
- `track(e)` (which captures the pointer on the SVG root) is called before the double-click check, but that does not affect correctness because the detection is based on successive `pointerdown` events, not retargeted `dblclick`.
No logic bugs or semantic correctness issues introduced.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
- The `lastPress` ref allocates one small object per object press, but this is negligible (not in a hot loop, bounded to one entry, and far below any perceptible threshold).
- Removing the `onDoubleClick` prop from every `<g>` actually reduces the number of React-managed event listeners per rendered object by one, which is a tiny net improvement.
- The `focusObject` call on double-click triggers store updates and a camera animation, but that is intentional and only happens on the rare double-click gesture, not on normal interactions.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`web/src/editor/Canvas.tsx:318` — `onCanvasDown` never invalidates `lastPress`, so a canvas click between two bed clicks does not break the double-click chain.** Verified by reading the file: `lastPress` is only mutated inside `objDown`, and `onCanvasDown` (the empty-canvas handler) does not touch it. Sequence: click bed A → click empty canvas → click bed A again within 400 ms/12 px → false double-click triggers `focusObject(A)` instead of a normal select/drag. Fix: add `lastPress.current = null` at the top of `onCanvasDown`.
- **`web/src/editor/Canvas.tsx:357` — `lastPress` is updated for every pointer button, so non-primary clicks (right-click, middle-click) can participate in or complete a double-click.** Verified by reading `objDown`: the ref is written unconditionally before the `e.button` check, and the `if (prev && …)` block that fires `focusObject` does not inspect `e.button`. Native `dblclick` fires only for the primary button; right-clicking a bed twice within 400 ms should not focus it. Fix: gate the `lastPress.current = {…}` assignment and the double-click check with `e.button === 0` (or `e.buttons === 1`).
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
lastPress sits with the component's other refs and its thresholds at module
scope with the other tuning constants; only a primary-button (or finger)
press counts, like a native dblclick; a press on empty ground clears the
pending half, so bed → ground → bed within 400 ms is not a double-click.
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit 38011db639 into main2026-08-23 07:26:50 +00:00
steve
deleted branch fix/double-click-focus2026-08-23 07:26:50 +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.
Found while checking #133 live: double-clicking a bed only selected it — in Playwright, through the Chrome extension, and by hand. The hint in the toolkit and the plan's aria-label both promise "double-click a bed to plant it".
Why:
track()callssetPointerCaptureon the SVG root for every press (since the Organic rebuild,52b2c09). Pointer capture retargets the compatibilityclick/dblclickevents to the capturing element, so theonDoubleClickon each object's<g>never fires. It has been dead since #124; nobody noticed because "Plant this" in the inspector (and Plants mode on the phone) get you there too.Fix: detect the double press in
objDownitself — two presses on the same object within 400 ms and 12 px — which capture can't retarget. The second press focuses the bed and starts no drag, so its pointerup has nothing to select into Plot. A plop in an unfocused bed already delegates toobjDown, so double-clicking a plant focuses its bed. The deadonDoubleClickprop is gone, with a comment saying why it can't come back.tsc, vitest and the build pass; the behaviour is confirmed on the live build after merge (the extension'sdouble_clickproduced two real presses, which is exactly what selects-only today and focuses with this).🤖 Generated with Claude Code
🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-23 07:21:35Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 2 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -332,0 +335,4 @@// and dblclick events to the root, so a dblclick handler on the object// never fires — which is why "double-click a bed to plant it" did nothing// from the Organic rebuild until this was noticed (2026-08-23).const lastPress = useRef<{ id: number; at: number; x: number; y: number } | null>(null)🟡 lastPress useRef declared far from the component's other refs (lines 87-97), breaking the established grouping pattern
maintainability · flagged by 3 models
lastPressref declared out-of-group (web/src/editor/Canvas.tsx:338): All otheruseRefhooks are declared together at lines 87–97.lastPressis inserted ~250 lines later, next to the function that uses it. A reader scanning for "which refs does this component own?" will miss it. It should sit with its peers at the top of the hook block — the proximity toobjDowndoesn't add anything because refs don't have render-time side-effects.🪰 Gadfly · advisory
@@ -342,6 +352,17 @@ export const Canvas = forwardRef<}// Dimmed siblings stay inert inside a focused bed.if (st.focusId != null && o.id !== st.focusId) returnconst prev = lastPress.current🟠 Double-click detection accepts non-primary pointer buttons, unlike native dblclick
correctness, error-handling · flagged by 2 models
web/src/editor/Canvas.tsx:357—lastPressis updated for every pointer button, so non-primary clicks (right-click, middle-click) can participate in or complete a double-click. Verified by readingobjDown: the ref is written unconditionally before thee.buttoncheck, and theif (prev && …)block that firesfocusObjectdoes not inspecte.button. Nativedblclickfires only for the primary button; right-clicking a bed twice within 400 ms should not focus it. Fix: gate the `last…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 3 findings (2 with multi-model agreement)
web/src/editor/Canvas.tsx:338web/src/editor/Canvas.tsx:3541 single-model finding (lower confidence)
web/src/editor/Canvas.tsx:318Per-model detail
claude-code/opus (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 is a purely client-side interaction change — double-click detection for focusing a bed in the SVG canvas editor. Reviewed through the Security lens:
performance.now(),e.clientX/Y, and the object's numericid— all local browser/editor values. They feedMath.hypot/timestamp comparisons andfocusObject(o); none is used in a query, path, template, URL, ordangerouslySetInnerHTML. No injection surface. (Verified by readingCanvas.tsx:355-364.)focusObject(verified atCanvas.tsx:171-192) only mutates local editor UI state (setFocus/setSel/setTab/camera) — it grants no privileges. The actual mutation path (placePlop,Canvas.tsx:301-315) still gates onlatest.current.canEdit, and per the project's architecture the real permission enforcement lives server-side ininternal/service. Double-clicking merely arms the plant palette; it cannot plant in a bed the user can't edit.o.plantableguard on the double-press branch matches the removedonDoubleClickgate, so no code path became reachable that wasn't already reachable via the inspector's "Plant this". No insecure default introduced.Nothing in my lane is materially wrong here.
🎯 Correctness — No material issues found
I've verified the change against the checked-out code. Let me walk through my correctness analysis.
What I checked:
objDown(Canvas.tsx:342),onCanvasUp(459),track/setPointerCapture(261), andfocusObject(171).plopDownline 381 →objDown(o)(e)) so double-clicking a plant records the bed's id and focuses the bed, as the PR claims.dim(line 571 =focusId != null && o.id !== focusId) is exactly the early-return guard at line 354, so dropping!dimfrom the removedonDoubleClickloses nothing — dimmed siblings still return early.onDoubleClick/dblclickhandlers exist (only the aria-label copy at line 557).Sequence trace (real double-click: down1, up1, down2, up2):
previs null/stale → recordslastPress, falls through, setsdrag='obj'.d.t==='obj', not moved → selects object into Plot, clears drag.prev.id===o.id, within 400 ms / 12 px → double detected; nullslastPressanddrag, callsfocusObject(plantable) → focus +toolkittab. End state correct.dragis null → returns early, nothing selected. Matches the comment.Edge cases that hold up:
lastPressreset to null on a detected double prevents a triple-click re-triggering.performance.now()is monotonic;Math.hypotonclientX/Yin screen px with a 12 px tolerance is standard.pts.current.deleteruns before the early return inonCanvasUp— no leak.The thresholds (400 ms, 12 px) are ordinary double-click tolerances, not domain conversion factors, so there's no magic-number derivation to redo here.
One nuance I considered but do not think is a bug: in read-only mode (
canEditfalse), the second press still callsfocusObject, which flips the rail to thetoolkit(plant) tab. But the originalonDoubleClickhad nocanEditguard either, so this matches the pre-existing intended design (the handler was merely dead due to capture), and planting itself is still blocked inplacePlop. Not a correctness regression.VERDICT: No material issues found
The double-press detection is logically sound: the state machine (record → compare id/time/distance → reset) is correct, the pointerup path is properly neutralized, the plop delegation preserves the "double-click a plant focuses its bed" behavior, and the removed
onDoubleClickwas provably equivalent to the retained early-return guard. Nothing in the correctness lens is materially wrong.🧹 Code cleanliness & maintainability — Minor issues
The finding is confirmed: module-level tuning constants live at lines 38–41 (
WHEEL_SENSITIVITY,ANIM_MS,REFIT_THRESHOLD_PX,FT), while the newDOUBLE_PRESS_MS/DOUBLE_PRESS_PXsit in the component body at 339–340. The only other in-component ALL_CAPS names (GW/GHat 114–115) are derived from garden props, so they legitimately stay inside — they don't undermine the convention claim. ThelastPressref correctly remains in-component. Finding holds.VERDICT: Minor issues
web/src/editor/Canvas.tsx:339-340— tuning constants placed inside the component body, against the file's own convention.DOUBLE_PRESS_MSandDOUBLE_PRESS_PXare plain magic-number tuning constants, but they're declared inside theCanvascomponent function (recreated every render). Every other constant of this kind in the file lives at module top —WHEEL_SENSITIVITY,ANIM_MS,REFIT_THRESHOLD_PX,FT(lines 38–41). The only other in-component ALL_CAPS names (GW/GH) are values derived from garden props, so they must stay local; these two are compile-time constants that belong besideANIM_MSet al. ThelastPressref correctly stays in the component. Trivial, but it's exactly the "doesn't follow the pattern the surrounding code uses" case.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the Performance lens by reading
web/src/editor/Canvas.tsx:332–372at the checked-out state.The change adds double-press detection inside
objDown, which is a discrete pointer-down event handler, not a hot path:performance.now(), one small object allocation ({ id, at, x, y }), and oneMath.hypot. This runs only on a user-initiatedpointerdownon an object — not inside a render loop, animation frame, or per-plop iteration — so the per-event cost is negligible.lastPressis a single-slotuseRefholding one small object at a time (reset tonullon the second press). No unbounded growth, no accumulation, no collection retained across events.onDoubleClickprop slightly reduces per-object render props on the<g>, if anything a marginal win.DOUBLE_PRESS_MS,DOUBLE_PRESS_PX) are re-bound each render but are primitive literals — immaterial.Nothing in this diff regresses efficiency in a way I can justify flagging.
🧯 Error handling & edge cases — No material issues found
I've verified the change against the actual code. Let me confirm my understanding of the unhappy paths:
st.focusId != null && o.id !== st.focusIdexactly matches thedimcomputation at line 571, so the old!dimguard ononDoubleClickis preserved — dimmed objectsreturnbefore reaching the double-press block.armedKind/armedPlantpaths return before the double-press block, so a double-press while armed places plops as before and never focuses.drag.currentwas cleared, soonCanvasUphitsif (!d) return— nothing spurious selects into Plot, as the comment claims.track()(line 261) wrapssetPointerCapturein try/catch and capture releases implicitly on pointerup;ptsis deleted on up. No leaked capture across the two presses.if (o.plantable)guards thefocusObjectcall; the object simply stays selected from the first press. No crash, no dangling state.Review
VERDICT: No material issues found
Through the error-handling & edge-cases lens this change is clean. I checked the paths I'd expect to break:
focusObjectandtrackalready handle their own failure modes (setPointerCaptureis in a try/catch,focusObjectno-ops whensvgRef.currentis null).lastPress.currentanddrag.currentare both reset on the double-press branch (lines 361–362), and stalelastPressfrom an earlier press is defended by the same-id+<400ms+<12pxtriple guard, so a false-positive double from two unrelated presses is effectively impossible.previs null-checked before use;performance.now()is monotonic (no overflow/clock-skew concern);Math.hypoton clientX/Y can't NaN here.onDoubleClick={o.plantable && !dim ? ...}prop is preserved — both the!dimguard (via the earlyreturnat line 354) and theo.plantableguard (line 363) survive. The prop ran regardless ofcanEdit, and so does the new path, so no read-only-viewer regression is introduced by this diff.One non-lens observation, verified while tracing but not something I'm raising as a finding: a read-only viewer can trigger
focusObject(which flips the rail to the plant palette) by double-clicking a plantable bed — but that was already true of the removedonDoubleClick, so it's neither new nor within this lens.claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
This change is pure client-side pointer-event bookkeeping with no security-relevant data flows.
lastPress.currentstores only numeric fields derived from pointer events (id: number,at: number,x,y). Theidis a server-issued integer primary key, not user-controlled input. The ref isuseRef-scoped to the component instance — no global state, no cross-session leak.focusObject(o)(verified by readingCanvas.tsx) makes zero network requests and performs only local Zustand store mutations (focus ID, selection, tab, camera). No untrusted content is rendered.o.plantableguard is checked before callingfocusObject, consistent with the surrounding code.performance.now()andMath.hypotare browser-internal, not attacker-controlled.onDoubleClickeliminates an event handler that was already dead (capture retargets it to the root); no security surface change.Nothing in this diff touches authn/authz, network calls, serialization, template rendering, or any tainted-input-to-sink path.
🎯 Correctness — Minor issues
I've confirmed the relevant code sections. Here's what I verified:
onCanvasDown(lines 319–330): no reference tolastPressanywhere in the function — CONFIRMED.lastPress.currentis written only insideobjDownat lines 357 and 361 — CONFIRMED.dimdefinition at line 571:const dim = focusId != null && o.id !== focusId— matches the draft's claim exactly — CONFIRMED.lastPress, so the second object pointerdown can match the first and triggerfocusObjectspuriously.The finding survives verification.
Minor issues
The double-press detection logic is sound overall — the
!dimequivalence is confirmed (dimis exactlyfocusId != null && o.id !== focusId, which the existingobjDownguard already enforces), pointer capture releases automatically onpointerup, and the 400 ms / 12 px constants are reasonable. One correctness gap:lastPress.currentis not cleared on non-object interactions (web/src/editor/Canvas.tsx, line 354 — the assignment site)lastPress.currentis only written insideobjDown. Clicking on the canvas background firescanvasDown, which does not touchlastPress. As a result a rapid sequence click-bed-A → click-canvas → click-bed-A (all three within 400 ms) hasprev.id === o.idandnow − prev.at < 400hold on the third press, andfocusObjectfires even though the user clicked away in between. The browser-nativedblclickresets the window when the click lands on a different target; this implementation does not replicate that.In practice 400 ms for three distinct pointer events is tight enough that this will be rare, but it is a real divergence from the expected "double-click" contract. The fix is straightforward: set
lastPress.current = nullincanvasDown(and wherever else the user clearly begins a new interaction context, e.g. when a pan drag starts).🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed by the actual code:
lastPressis declared at line 338 — well outside theuseRefgroup at lines 87–97.DOUBLE_PRESS_MSandDOUBLE_PRESS_PXare at lines 339–340 inside the component body, while the established module-level constants (WHEEL_SENSITIVITY,ANIM_MS,REFIT_THRESHOLD_PX,FT) live at lines 38–41.Minor issues
lastPressref declared out-of-group (web/src/editor/Canvas.tsx:338): All otheruseRefhooks are declared together at lines 87–97.lastPressis inserted ~250 lines later, next to the function that uses it. A reader scanning for "which refs does this component own?" will miss it. It should sit with its peers at the top of the hook block — the proximity toobjDowndoesn't add anything because refs don't have render-time side-effects.DOUBLE_PRESS_MS/DOUBLE_PRESS_PXdeclared inside the component (web/src/editor/Canvas.tsx:339): The file's established pattern puts config literals at module level (WHEEL_SENSITIVITY,ANIM_MS,REFIT_THRESHOLD_PX,FTat lines 38–41). These two constants are recreated as fresh bindings on every render and are buried inside the component body where they're less discoverable. They should join the module-level block; the only argument for keeping them inside (co-location) is undercut by the fact thatANIM_MSandREFIT_THRESHOLD_PXare equally "local" in concept and still live at module level.No other cleanliness issues in scope — the removal of the dead
onDoubleClickprop is clean, and the comment explaining why it can't come back is appropriate.⚡ Performance — No material issues found
Based on my exploration, I can now complete the performance review.
No material issues found
The change adds logic only to the
objDownevent handler — a user-interaction path, not a render loop. Walking through each addition:lastPressref — a singleuseRefallocation at mount; the object written on press ({ id, at, x, y }) is created once per pointer-down, user-triggered, nowhere near the hot render path.performance.now()— negligible cost, called only on pointer-down.DOUBLE_PRESS_MS/DOUBLE_PRESS_PXinside the component body — they're primitive number bindings, no heap allocation. Worth knowing: the component already recreates far heavier things (handler closures, memoized arrays) on every render; two number bindings aren't a meaningful addition. Hoisting them to module scope would be cleaner style but the cost difference is immeasurable.Math.hypot(…)— O(1), pointer-down only.The existing render-time cost (curried
objDown(o)called per object inside.map(), producing a new closure per object per render) predates this PR and is untouched by it. No new work lands on the render path.🧯 Error handling & edge cases — No material issues found
I now have everything I need. Let me verify the full sequence:
track()adds the pointer topts.current, the double-press path setsdrag.current = nulland returns, thenonCanvasUpfires because the pointer is captured on the SVG root.onCanvasUpat line 459:Both concerns check out:
pts.currentis cleaned up by the normal up handler, anddrag.current = nullis explicitly guarded.Verdict: No material issues found
The double-press detection path is error-safe under the error-handling lens:
pts.currentcleanup —track(e)always runs before the double-press check, adding the pointer topts.current. The earlyreturninobjDowndoes not skip cleanup because cleanup happens inonCanvasUpviapts.current.delete(e.pointerId)(line 460), which fires on every pointer-up regardless of drag state. Verified by readingonCanvasUp.drag.current = nullon second press —onCanvasUpreadsconst d = drag.currentand immediately guardsif (!d) return(line 467), so the up event for the second press exits cleanly with no spurious selection or mutation. Verified by reading lines 459–505.Non-plantable object double-press — The second press for a non-plantable object takes the
if (o.plantable) focusObject(o)branch silently (does nothing, returns). This matches the oldonDoubleClick={o.plantable && !dim ? … : undefined}guard, so behavior is equivalent.First-press up interaction — The first press's
pointerupdoes select-into-Plot (setSel+setTab('plot')), but the second press'sobjDownimmediately callsfocusObjectwhich overrides tosetTab('toolkit'). These are synchronous state updates within the same event loop; no observable flicker.performance.now()usage — Standard, never throws, never returns null.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, this change has no authn/authz, injection, SSRF, deserialization, secret-handling, or untrusted-input surface. The only inputs are local pointer coordinates (
e.clientX/e.clientY) andperformance.now(), all used solely for in-browser double-press timing/distance gating that callsfocusObject(o)on an already-loadedEditorObject. I verified the surroundingobjDownpath (Canvas.tsx:342–372): the new block sits after thearmedKind/armedPlantearly returns, so it cannot be reached while a plant/kind is armed, and it only acts onoalready resolved from local state — no tainted or cross-origin data flows into it. Nothing to flag.🎯 Correctness — No material issues found
I have verified the relevant code paths. Let me finalize the review.
The diff correctly identifies and fixes the
setPointerCaptureretargeting issue. The double-press detection logic inobjDownis sound for the plantable bed case. However, I found one correctness regression introduced by the removal ofonDoubleClick.Finding: Removed non-plantable double-click focus — The old
onDoubleClick={o.plantable && !dim ? () => focusObject(o) : undefined}only focused plantable beds, so non-plantable object double-click is unchanged. But the new code path's behavior on the first press of a double-click is the concern. Let me re-examine.Let me re-read the drag flow carefully to confirm whether the first press of a now-recognized double-click correctly leaves selection intact.
The flow for a double-click on a plantable bed (desktop, canEdit, no armed plant, no existing focus):
objDown→lastPressset → not a double →drag.current = { t: 'obj', ... }. On pointerup (onCanvasUp):d.t === 'obj',!d.moved→setSel({type:'object', id})andsetTab('plot').drag.currentcleared.objDown→prevmatches within 400ms/12px →lastPress.current = null,drag.current = null,focusObject(o)→ focuses bed, selects object, sets tab to toolkit, animates camera.This matches the description: first press selects, second focuses/plants. Previously the first press selected and the second selected again (no focus). So the fix works as intended for plantable beds.
I've verified the change is correct for its stated goal. No material correctness issues in my lens.
No material issues found.
Canvas.tsx:355-365) is correctly placed after the armed-plant/armed-kind early returns and the dimmed-sibling inert check, so it only fires for an editable, non-dimmed object — matching the oldonDoubleClick={o.plantable && !dim ? ...}guard.lastPress.current = null+drag.current = nullon the second press correctly suppresses drag and prevents the matching pointerup from re-selecting into the Plot tab, sinceonCanvasUpreturns early whendis null (Canvas.tsx:467). Verified by readingonCanvasUp.lastPressis only ever written inobjDown(verified by grepping the wholesrctree forlastPress/DOUBLE_PRESS), so there is no stale-state reset issue across renders. The ref persists correctly; the only minor effect is that an aborted second press (e.g. a long pause or movement >12px) leaveslastPresspointing at the prior press, but the next qualifying press still evaluates against the most recent press since it's overwritten before the check — actually, it's overwritten unconditionally on line 357 before the comparison, which is the correct ordering for double-press detection.thresh= 3px desktop / 7px touch); they don't collide because the double-press check uses client-space deltas while drag uses world-space deltas, and the double-press short-circuits before a drag is even started.if (o.plantable) focusObject(o)guards focus to plantable beds only, exactly as the removedonDoubleClickdid.I confirmed by reading
track(Canvas.tsx:261),focusObject(Canvas.tsx:171),objDownand the surrounding handlers (Canvas.tsx:342-405),onCanvasUp(Canvas.tsx:459-505), and the render of each object<g>(Canvas.tsx:578-593), and by grepping forlastPress/onDoubleClickacrossweb/src.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
web/src/editor/Canvas.tsx:339-340—DOUBLE_PRESS_MS/DOUBLE_PRESS_PXare declared inside the component body, but they're pure compile-time tuning constants with no per-render dependency. Every other such tuning constant in this file lives at module top-level (WHEEL_SENSITIVITY,ANIM_MS,REFIT_THRESHOLD_PX,FTat lines 38-41). The in-body uppercase consts that do exist (GW,GH,sat lines 114-116) are derived from props/state each render, so they have a reason to be local; these two don't. Declaring them in-body (a) recreates two bindings every render for no reason, and (b) breaks the file's own convention for where magic-number thresholds live, making them harder to spot when someone comes looking to tune double-click sensitivity. Suggested fix: hoist both to the module-level block besideWHEEL_SENSITIVITY.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, the change is clean:
objDownruns once per pointer press (not a hot loop), so the addedperformance.now()call andMath.hypotarithmetic are negligible — no concern.lastPressis a singleuseRefholding one small record; it's overwritten on each press and cleared on a double, so there's no unbounded growth or accumulation.DOUBLE_PRESS_MS,DOUBLE_PRESS_PX) are primitive numbers recreated per render, but they're cheap literals andobjDownalready allocates a closure per object per render (pre-existing), so this adds nothing measurable. Moving them to module scope would be a micro-optimization, not worth flagging.onDoubleClickhandler, slightly reducing per-render closure allocation.I verified
objDown's placement and the ref declaration by readingCanvas.tsx:325-385; nothing in the surrounding code introduces a repeated/allocation-heavy path that the new logic feeds into.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling & edge-cases lens, I traced every unhappy path the diff introduces and found the new logic to be robust. Key verifications (all by reading
web/src/editor/Canvas.tsx):drag.current = null(line 362). I confirmed the pointerup handler (onCanvasUp, lines 465-467) treatsd === nullas a no-op early-return, so a swallowed second press leaves no half-finished drag/select. Even if the first press's pointerup was missed,onPointerCancelis routed to the same handler (line 553), which clearsdrag.currentandpts.current.track(e)(line 344) runs before the double-press branch, so the second press still callssetPointerCapture(wrapped in try/catch at 264-268) and adds topts.current. The pointerup handler deletes the pointer id (line 460), so no leak. This matches pre-existing behavior — capture was always set intrackfor every press.lastPresswindow:lastPress.currentis overwritten on every qualifying press (line 357) before the match check, and reset tonullon a match (line 361). Pressing a different object, or pressing past the 400ms/12px window, simply re-bases the window — no stale state. The dimmed-sibling early return (line 354) correctly runs beforelastPressis touched, so inert objects can't poison the detector.pts.current.size === 2→pinchStart()early return (line 345) and thearmedKind/armedPlantreturns (348-352), so it can't fire mid-pinch or while placing. Correct.if (o.plantable) focusObject(o)guard (line 363) means a non-plantable second press returns early with no drag and no focus. The first press already selected the object via its pointerup (lines 473-477), so selection isn't lost. The oldonDoubleClicklikewise did nothing for non-plantable objects — no regression.canEditfalse): The double-press branch (line 358) executes before thecanEditguard (line 366), so double-clicking a plantable bed focuses it even in read-only mode. I verified the oldonDoubleClick={o.plantable && !dim ? () => focusObject(o) : undefined}had nocanEditguard either, so this preserves the prior behavior rather than introducing it. Not a new defect.No swallowed errors, missing cleanup, panics, nil/boundary, or overflow concerns are introduced by this diff.
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Reviewed the double-click-to-plant fix through the security lens. The change replaces a broken
onDoubleClickprop with manual double-press detection insideobjDownusing alastPressref. Verified the following:lastPressstores only numeric IDs, timestamps, and pointer coordinates; no user strings are evaluated or rendered.dimguard (focusId != null && o.id !== st.focusId) still applies via the early return above the new double-press logic, so dimmed objects remain inert.setPointerCaptureretargets compatibilityclick/dblclickevents but does not blockpointerdowndispatch, which is why theonPointerDown-based detection works correctly.lastPressref is single-valued and scoped to the component; no memory-exhaustion or race-condition concern.No security-relevant defects identified.
🎯 Correctness — No material issues found
No material issues found
I examined the double-click detection logic through the correctness lens, tracing the full pointer event flow and state transitions. The implementation is sound:
objDownrecords each press inlastPressand checks the next press against the 400 ms / 12 px thresholds before any drag starts. On a detected double-press it nullsdrag.current, soonCanvasUpcorrectly returns early and cannot overwrite the focus with a single-click selection. Verified by readingobjDown(lines 342–372) andonCanvasUp(lines 459–505).st.focusId != null && o.id !== st.focusIdguard correctly keeps dimmed siblings inert; the double-click check sits after it, so double-clicking a dimmed sibling is correctly ignored.plopDown→objDown(o)whenst.focusId !== p.objectIdmeans double-clicking a plop in an unfocused bed focuses the bed, as intended.lastPress.current = { ... }before theif (prev && ...)check is safe becauseprevis read first; resettinglastPress.current = nullinside the true branch prevents triple-click false positives.track(e)(which captures the pointer on the SVG root) is called before the double-click check, but that does not affect correctness because the detection is based on successivepointerdownevents, not retargeteddblclick.No logic bugs or semantic correctness issues introduced.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
lastPressref allocates one small object per object press, but this is negligible (not in a hot loop, bounded to one entry, and far below any perceptible threshold).onDoubleClickprop from every<g>actually reduces the number of React-managed event listeners per rendered object by one, which is a tiny net improvement.focusObjectcall on double-click triggers store updates and a camera animation, but that is intentional and only happens on the rare double-click gesture, not on normal interactions.🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/editor/Canvas.tsx:318—onCanvasDownnever invalidateslastPress, so a canvas click between two bed clicks does not break the double-click chain. Verified by reading the file:lastPressis only mutated insideobjDown, andonCanvasDown(the empty-canvas handler) does not touch it. Sequence: click bed A → click empty canvas → click bed A again within 400 ms/12 px → false double-click triggersfocusObject(A)instead of a normal select/drag. Fix: addlastPress.current = nullat the top ofonCanvasDown.web/src/editor/Canvas.tsx:357—lastPressis updated for every pointer button, so non-primary clicks (right-click, middle-click) can participate in or complete a double-click. Verified by readingobjDown: the ref is written unconditionally before thee.buttoncheck, and theif (prev && …)block that firesfocusObjectdoes not inspecte.button. Nativedblclickfires only for the primary button; right-clicking a bed twice within 400 ms should not focus it. Fix: gate thelastPress.current = {…}assignment and the double-click check withe.button === 0(ore.buttons === 1).Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.