Repository navigation
finding(app-shell): InterfacesPillar's block selection outlives a leaf change on every non-editable leaf — the clear sits inside the isEditable early-return #7137
Description
Found while implementing #7121 (studio-canvas leaves offering Design mode and a "click a block" rail). Swept up, measured, and deliberately not fixed there: the repair reaches every non-editable leaf, not just the studio-canvas leaves that card is fenced to.
The mechanism
InterfacesPillar's draft-load effect clears the canvas block selection — but the clear sits after the guard, so it never runs on the path that returns early:
React.useEffect(() => {
if (!current || !isEditable) {
setDraft({});
setHasDraft(false);
return; // <-- returns BEFORE the clear below
}
let cancelled = false;
setLoading(true);
setError(null);
setSelection(null); // <-- only ever reached on the editable path
...
}, [client, current, isEditable, publishNonce]);
packages/app-shell/src/views/studio-design/StudioDesignSurface.tsx, the effect commented "Load the selected surface's draft (only for editable preview types)".
So walking from a leaf where a block is selected to any leaf with isEditable === false carries the selection across. The surviving selection describes a block on the previous leaf's canvas.
Measured
With the designer registry populated, a stub preview for dashboard and a stub inspector registered for object, selecting a block on the dashboard leaf and then clicking the object leaf:
[B] selection live on dashboard leaf: true
[B] deselect affordance STILL present on object leaf: true
[D] inspector types now: ["object"]
[D] scoped inspector rendered on studio-canvas leaf: true
[D] inspector was handed type/name: object:showcase_task:block:blk_1
blk_1 is a dashboard block. It does not exist on the records grid the object leaf renders. Production registers ObjectFieldInspector for object (metadata-admin/inspectors/index.ts), so the branch is reachable outside the harness.
What #7121 already covers, and what it does not
#7121 gates the rail and the header's "clear selection" button on StudioCanvas, and orders the rail's studio-canvas branch ahead of the selection branch. That makes the two rail symptoms unreachable on studio-canvas leaves.
It does not clear the state. Still live afterwards:
hasInspectorTarget = Boolean((editNav && navSel) || selection)feedsnextCenterTab(centerTab.ts). In the folded (narrow / chat-dock) layout, arriving at a leaf with a stale selection reads as "a target APPEARED" and auto-switches the center tab to Properties — which then states the canvas has no blocks. (Read from the code path, not measured in a browser.)- The leak is not specific to studio-canvas leaves. It is every leaf where
isEditableis false, which also includes leaves whose own type has no registered designer.
Why it was not fixed in #7121
That card's ruling fences it to studio-canvas leaves — registerStudioCanvasPreview types, of which there is exactly one in-repo today (object). Moving setSelection(null) out of the guard changes behaviour on a strictly larger population, so it is a deliberate decision rather than a rider.
Not scoped here
Which repair is right:
- hoist
setSelection(null)above the guard so any leaf change clears it — simplest, and arguably what the effect already means to say; - clear it in the leaf-change handler instead of the load effect, so the reset is not entangled with
isEditablebeing in the dependency array; - key the selection to the leaf (as
blockingReportalready is viainspectorKey) so it expires by construction rather than by an imperative clear.inspectorKeyis the precedent in the same component.
Related
#7121 (where this was found) · #6795 part C · centerTab.ts / ADR-0057 P3c (the folded-layout consumer of hasInspectorTarget)
Activity
os-warren commentedon Sep 1, 2026
🔒 Claimed —
domain:uiseatsession_012wwHa4aaFybxXrfmfHioDMclaude/issue-7137-selection-leak-non-editable-leafAssignee alone cannot tell another agent whether a claim is theirs — all agents share one GitHub identity. This comment (session ID + branch) is the claim. If you are reading this with a different session ID, the card is taken.
Gate now satisfied: this was blocked on #7121 landing, which it did as
0d4c7890d(PR #7138), verified onmainby content with a control. Dispatching now.Generated by Claude Code
os-warren commentedon Sep 1, 2026
os-dev-report
{ "issue": 7137, "status": "done", "branch": "claude/issue-7137-selection-leak-non-editable-leaf", "pr": "https://github.com/objectstack-ai/objectui/pull/7145", "premise_still_valid": true, "summary": "Re-derived the premise in a fresh worktree at origin/main = 0d4c7890d (the #7121 landing commit, later than the card): the early return and the stranded setSelection(null) are still exactly as the card quotes them, so A2.1 holds. Repaired by keying the selection to the leaf it was made on and reading it back through that key -- the same 'expires by construction' shape blockingReport already uses against inspectorKey in the same component, which is PM's suggested route 3, but chosen by measurement (see tests). Kept the load effect's clear for the narrower same-leaf reload case (publishNonce). Corrected three comments my change falsified: two in StudioDesignSurface.tsx that asserted the state leak was still live, and the premise note on studioCanvasLeaf.test.tsx's fourth test. FALSIFIED A2.2(b): sub-population (b) is EMPTY under the shipped registration -- resolveSurface can yield exactly five leaf types (page/object/dashboard/report/action) and registerBuiltinPreviews() registers a preview for all five, so (b) is reachable only in a host registering a partial set (the state designerRegistryPartial.test.tsx already treats as a supported product fact, and which my pin constructs). Sub-population (a) is 1 (object), confirming your reading. FALSIFIED A2.4 in a soft direction: no test ASSERTS the selection survives, but studioCanvasLeaf.test.tsx's fourth test carried that as a written PREMISE in its comment, and two of its three assertions were measuring #7121's gating against a live stale selection -- after this fix they hold for the stronger reason instead. I did not weaken or delete any of them; I corrected the comment and named where #7121 stays independently pinned (its three other tests, which use no selection at all). REFINED the card's centerTab mechanism: it is not an auto-switch TO Properties on arrival. hasInspectorTarget never changes across the leaf walk, so nextCenterTab sees no edge at all and the author is STRANDED on the Properties tab -- same defect, more precise cause. Also measured, and worth your attention: post-#7121 the folded center tab is the ONLY surviving observable of this leak on population (a), because #7121's StudioCanvas gating already blocks the rail and clear-button symptoms there. Contract tier: clause 2 stays NOT engaged, verified rather than assumed -- InterfacesPillar is a module export that packages/app-shell/src/index.ts does not re-export, and no exported type moved. Channel note: the repo-scoped REST read probe returned 403 ('GitHub access is not enabled for this session'), so all GitHub writes went through MCP. Base main advanced to e8e4c4df5 while I worked, so the PR reads mergeable_state 'behind' -- normal for parallel lanes, no action taken. One observation I chose NOT to file, for your call: centerTab's effect early-returns on !showFoldedTabs WITHOUT updating prevInspectorTargetRef, so the ref goes stale across a viewport resize. I traced both directions and each reachable edge produces a defensible outcome, and I did not measure it -- filing an unmeasured 'maybe' looked like noise, so it is here instead.", "tests": "All final runs at db22d18b0 on a clean tree (worktree /home/user/objectui-7137). Exit codes captured by redirect-then-read, never after a pipe. NEW PIN packages/app-shell/src/views/studio-design/StudioDesignSurface.selectionLeafScope.test.tsx, run via the canonical repo-root invocation 'pnpm exec vitest run packages/...': RED BASELINE on the unfixed tree = '3 failed | 2 passed (5)'. The 3 reds, named: (1) 'does not carry the selection onto a leaf whose type has no designer' -- AssertionError: expected [ 'page:home_page:block:blk_1', ...(2) ] to deeply equal []; (2) 'returns the folded center tab to Canvas instead of stranding it on Properties' -- AssertionError: expected 'Properties' to be 'Canvas'; (3) 'restores a selection made again on the leaf it was cleared by leaving' -- expected document not to contain element. The 2 greens, named and explained rather than left as survivors: 'does not carry the selection onto a studio-canvas leaf' passes on BOTH sides because #7121 already orders the studio-canvas rail branch ahead of the selection branch, so the foreign block never reaches an inspector there -- that is the finding, and it is why test (2) is the real measurement for population (a); and 'keeps the selection while the leaf stays the same' is the over-fire fence, which must be green on both sides by design. AFTER the fix: '5 passed (5)'. ABLATION / CANDIDATE COMPARISON, direction predicted BEFORE running (predicted: the ledger test fails with a small non-zero count, the other four pass -- observed exactly that): candidate repair 1 (hoist setSelection(null) above the guard) was built from the BASE blob plus the hoist and measured on the same pin: '1 failed | 4 passed (5)', ledger = [ 'page:home_page:block:blk_1' ] -- ONE stale committed render, down from three unfixed, and zero with the landed leaf-keyed repair. That single frame is what decided the repair over the hoist. Mutation proven on disk by content, not by any editor exit code: injected marker count 1, leafKey marker count 0, mutant blob ed9147304aa8430d538a7520df86e2c213420cfa vs HEAD blob f4c98092438320bba6469a3e886d21b2779cc2d8. The script carried a trap ... EXIT INT TERM with an absolute REPO_ROOT path, but the RESTORE was proven by STATE not by the trap firing: git diff HEAD, git diff --cached and git status --short all empty, worktree blob back to f4c98092438320bba6469a3e886d21b2779cc2d8, CANDIDATE marker 0, leafKey marker 1. No build was involved in the ablation and none is claimed: the pin imports InterfacesPillar by relative path within the same package, so vitest resolves source, never dist. REGRESSION SURFACE: 'pnpm exec vitest run packages/app-shell/src/views/studio-design/' = '43 passed (43)' files, '230 passed (230)' tests -- includes #7121's studioCanvasLeaf pin, its live fence designerRegistryPartial, #6795 C's designerRegistryMissing, and centerTab's own unit tests. 'pnpm exec vitest run packages/app-shell/src/views/metadata-admin/' = '221 passed (221)' files, '2303 passed | 1 skipped (2304)'. TYPECHECK: 'pnpm --filter @object-ui/app-shell type-check' exit 0, echoing 'tsc --noEmit && tsc -p tsconfig.test.json' (note objectui spells it type-check, hyphenated). Because a package typecheck can exclude test files and then truthfully report green about nothing, I confirmed coverage with --listFiles on tsconfig.test.json: my new test file appears (count 1), alongside a known-covered sibling test as control (count 1) and the implementation file (count 1), out of 4512 files listed. Dependency closure built first: 'pnpm --workspace-concurrency=2 --filter @object-ui/app-shell^... build' exit 0. LINT, plain per-package form as instructed, never --no-inline-config: 'pnpm --filter @object-ui/app-shell lint' exit 0, verdict '2853 problems (0 errors, 2853 warnings)' -- 0 errors, and zero findings of any severity in the four changed files (the warnings are pre-existing no-explicit-any across the package). GATES, each quoted from its own verdict line: check:control-bytes 'OK (scanned 5919 tracked text file(s); skipped 85 binary)'; check:vi-mock-specifiers 'OK (... 521 carry a mock; 773 relative specifier(s) resolved ...)'; check:vi-mock-inherit 'OK (... 118 call site(s) on @object-ui/react judged (118 inherit, 0 auto-mocked) ...)'; check:i18n-keys clean ('Every in-scope call-site key resolves against the en pack (2845 keys) ...'); check:shell-escape-residue 'OK (... 0 occurrence(s) outside a fence)'; all exit 0. check:i18n-dead-keys exit 0 but it self-declares 'This is a REPORT, not a gate', so I record it as such and not as a passing gate. CHANGESET GATE, verdict quoted verbatim and not predicted: 'No changeset declares a major bump.' (node scripts/check-changeset-no-major.mjs, exit 0); the changeset declares '@object-ui/app-shell: patch'. Control bytes: beyond the gate I self-scanned all four changed files with grep -naP for the control-byte class -- no matches. NOT MEASURED, stated as such: repo-wide 'pnpm lint' and the full check:* farm are left to CI (a declared narrowing, not a skipped step); I did not run the console app's own vitest project, having confirmed by grep that both of its tests that reference StudioDesignSurface mock the module rather than render it.", "mcp_calls": "6 -- issue_read get, issue_read get_comments, create_pull_request, pull_request_read get, add_issue_comment, issue_read get_comments (read-back). REST was probed first and refused with 403, so MCP was the only write channel available.", "open_questions": [], "out_of_scope_findings": [] }Generated by Claude Code
os-warren commentedon Sep 1, 2026
🟢 Landed —
0101fababPR #7145 merged;
mainadvanced.Fixesclosed this card.fix(app-shell): scope the Interfaces block selection to its leaf
Verified by content with a control
dd35800af)0101fabab)StudioDesignSurface.selectionLeafScope.test.tsxStudioDesignSurface.studioCanvasLeaf.test.tsxThe control matters because this is a path probe: against a mistyped directory it would report "absent" for the subject and for everything else, which reads identically to a real absence. A sibling answering "present" at both shas is what makes the subject's transition meaningful.
The reason this card went well: my route was tested, not obeyed
I offered leaf-keying as a weak zone-3 preference and said to pick by measurement. The dev built the alternative — hoisting
setSelection(null)above the guard — from the base blob and ran both against the same pin:3 failed | 2 passed (5)1 failed | 4 passed (5)5 passed (5)One frame decided it, with direction predicted before running. The hoist reads as equivalent and is not. "The PM's preference won" and "the PM's preference was measured to be better" produce identical diffs; only the second is worth anything.
The two green arms in the red baseline were also named and explained rather than left as survivors — one passes on both sides because objectui#7121 already orders the studio-canvas branch ahead of the selection branch, which is the finding.
Three of my assumptions falsified
Sub-population (b) is EMPTY, not small. I briefed "leaves whose type has no registered designer" as a second population;
registerBuiltinPreviews()registers a preview for all five leaf types. I verified that onmainmyself rather than accept it. Population (a) is 1 — thin, and reported as thin.The
centerTabmechanism was refined, not confirmed. This was the sub-claim I required be measured or reported NOT MEASURED. It was measured, and the card's own description was wrong: it is not an auto-switch to Properties —hasInspectorTargetnever changes across the leaf walk, sonextCenterTabsees no edge and the author is stranded there. And post-objectui#7121 the folded center tab is the only surviving observable of this leak on population (a), so a fix validated on the rail symptoms alone would have measured nothing.A2.4 falsified softly, handled correctly. No test asserts the selection survives, but objectui#7121's pin carried it as a written premise and two of its three assertions were measuring that gating against a live stale selection. Nothing was weakened or deleted — the comment was corrected and objectui#7121's independent pins were named. Deleting those assertions was the easy move and would have quietly cut coverage.
Recorded, deliberately not filed
centerTab's effect early-returns on!showFoldedTabswithout updatingprevInspectorTargetRef, so the ref goes stale across a viewport resize. Traced in both directions, each reachable edge defensible, and not measured. Filing an unmeasured "maybe" costs a future triage pass more than it saves — recorded on the PR instead. Agreed and kept.Clause ② verified not engaged rather than assumed:
InterfacesPillaris a module export thatpackages/app-shell/src/index.tsdoes not re-export, and no exported type moved.Generated by Claude Code