Conversation
Right panel was unmounting CodeTab via two routes — `<Show
when={!collapsed()}>` in RightPanelLayout and a `match()` swap of
sibling components in RightPanel — both of which discarded CodeTab's
selectedPath signal and Pierre's internal tree expansion on every
collapse or Inspector switch.
Drop the inner `<Show>` (Resizable already shrinks the panel to zero
width via `sizes=[1,0]`, no unmount needed) and replace the `match()`
with both children always rendered, visibility toggled by Tailwind
`hidden`. TAB_KINDS / TAB_LABEL records still give compile-time
exhaustiveness over RightPanelTabKind.
Adds two regression scenarios + the `right panel tab "{kind}"` step.
…817) Pierre's row-click handler synchronously calls `controller.closeSearch()` after firing selection (verified at @pierre/trees/dist/render/ FileTreeView.js around the row-click plan, where `closeSearch: isSearchOpen` is hardcoded with no opt-out). When the host drives search externally via the wrapper's `searchQuery` prop, every click would clear Pierre's internal filter while the host's signal still held the query — input shows "alp", tree shows everything. Re-apply the host's query on the next microtask inside `onSelectionChange`, after Pierre's clear has run. Keeps the wrapper push-only (no need to read Pierre's internal search state). Assumes Pierre's row handler stays synchronous. Adds a regression scenario plus the `type into filter` and `filter input should contain` step definitions.
Surfaced when CodeTab stopped unmounting on right-panel tab toggle: the reset effect at CodeTab.tsx (`on([repoPath, view], ...)`) was firing on every \`preferences\` cell tick, not just on actual repo/view changes. SolidJS \`on(...)\` runs the callback whenever the underlying signals emit — without an equality gate on the projected value, an unrelated preference update (or a snapshot replay) re-ran the callback and the unconditional \`setSelectedPath(null)\` wiped the user's selection on every tick. Pre-fix this was masked because tab-switch tore CodeTab down entirely; once mounted state survives toggles, the effect leaked. Memoize the (repoPath, view) tuple as a string key so the reset effect fires only on real value transitions. Also harden two related sites: - \`handleSelect\` ignores \`null\` from Pierre — Pierre fires \`onSelectionChange([])\` from \`resetPaths\` and tear-down, not just user deselect, and the Code tab has no UX affordance for explicit deselect (user switches selection by clicking another file). - The membership-check effect treats an empty \`treePaths\` as "tree not yet loaded" rather than "selected file is missing", so a stream resubscribe (which briefly drops \`status()\` to undefined) doesn't null \`selectedPath\`. Update the \`right panel should not be visible\` step to assert effective collapse via bounding-box width — after #818 the panel stays in the DOM at ~0 width when collapsed (a 1px \`border-l\` is all that remains, so Playwright's \`state: "hidden"\` would still see it as visible).
Followup to the previous commit. The earlier guard treated any empty \`treePaths\` as "tree loading" — but two existing scenarios assert that a truly-empty tree (post-commit clearing local changes; post-rm deleting the browsed file) DOES clear the stale \`selectedPath\` and shows the "Select a file…" hint. Distinguish loading from genuinely empty by checking the relevant stream's \`pending()\` accessor: the gitStatus / fsListAll subscription exposes whether it is mid-resubscribe. If pending, hold the selection through the transient empty; once the stream has delivered (\`!pending\`) an empty paths set is authoritative and we null normally.
|
| Step | Status | Duration | Verification |
|---|---|---|---|
| sync | ✓ | 1s | git fetch ok; forge=github |
| research | ✓ | 13s | Talk-mode plan: drop <Show> at RightPanelLayout.tsx:69; replace match() at RightPanel.tsx:81-90 with hidden-toggle divs (#818); queueMicrotask re-apply in solid-pierre FileTree onSelectionChange after row click (#817). Pierre source verified at FileTreeView.js:1738 + fileTreeRowClickPlan.js:11. |
| branch | ✓ | 11s | On dull-type, 0 ahead of origin/master |
| implement | ✓ | 2m 8s | Both fixes + 3 regression scenarios + 3 step defs |
| check | ✓ | 54s | just check clean; 48 pre-existing warnings, none in changed files |
| fmt | ✓ | 9s | just fmt clean — 0 files reformatted |
| commit | ✓ | 2m 10s | Two commits per the plan: a9544488 (#818), 488d8665 (#817) |
| test | ✓ | 33m 54s | 244/244 e2e pass after 4 followup commits (visibility-step, memo gate, handleSelect ignore-null, membership-check pending-gate) |
| create-pr | ✓ | 1m 3s | Draft PR #826 |
| ci | ✓ | 7m 45s | 14/15 contexts pass; e2e@aarch64-darwin flakes scattered across 3 retries — different scenarios each time, all unrelated to right-panel/code-tab. Filed at #320. |
| Total | 48m 41s |
Slowest step: test (33m 54s — three full e2e runs to debug the cascading #818 followups, plus the final clean run)
Optimization suggestions
- Talk-mode reviewers should run on a draft diff, not just a sketch. The four Code browser doesn't remember state #818 followup commits all surfaced as test failures, not as design-phase findings. A post-implement
hickey/lowypass before the first push would have caught the unmemoedon([repoPath, view], …)cascading on every preference tick. - The
defer: true+ reactive-on-array pattern is a recurring footgun.on([a, b], …)re-fires on every upstream signal emit regardless of value change — three CodeTab effects had this shape and only one needed fixing this round, but the others (and any new ones) carry latent risk. A repo-wideGrepforon(\[followed by an audit could be a fast standalone PR. - e2e suite dominates wall time at 34m for one feature change. A
--feature code-tab.featurefilter onjust test-quickwould have made the failure-iteration loop ~3× faster; the bottleneck was running the full 244-scenario suite each retry. Worth ajust test-quick-feature <name>recipe.
Workflow completed at 2026-05-04 (UTC).
Swaps the #817 fix from `onSelectionChange` to a DOM `click` listener on the FileTree wrapper's container, matching @srid's parallel PR #824. The original `onSelectionChange` hook missed the re-click case: Pierre's `#applySelection` short-circuits its `selectionVersion` increment when the new selection equals the current one, so the callback never fires on re-click of an already-selected row — but `controller.closeSearch()` still runs on every row click, so the filter would be wiped with no recovery path. The DOM-event hook fires regardless of selection-version state. Three calls to Pierre's `setSearch` now route through a single `applySearchQuery` helper that owns normalization (`""` → `null`, Pierre's contract for "no filter") and `onError` routing. Also folds in reviewer feedback (hickey + lowy on the post-implement diff): - `aria-hidden` on the right-panel root (collapsed) and on each tab wrapper (inactive) — makes the keep-mounted contract legible to both future readers and assistive tech. - Top-of-block invariant comment in CodeTab naming the selection-stability invariant the three guards collectively defend ("preserve selection across non-genuine transitions"); each guard documents a different churn source. - `resetKey` separator note: `::` is collision-safe given current `view()` enum + `repoPath()` semantics. - Strengthened comment on the row-click handler explicitly naming the two Pierre invariants the microtask-after-click pattern relies on (synchronous handler; `data-item-path` attribute on rows). - Re-click assertion added to `Filter survives clicking a filtered result` — the case the previous implementation missed. Approach credit: PR #824 by @srid (Codex). The DOM-listener structure, `applySearchQuery` helper, and re-click test scenario are adapted from that PR.
Hickey/Lowy AnalysisPost-implement review pass on the diff (originally skipped under
Hickey rationale
On CodeTab's three guards: "They are not the same invariant from different angles; they are three distinct noise sources arriving via three different channels (upstream signal ticking, stream lifecycle, Pierre API semantics). Collapsing them into one would couple those channels in a single place, which is worse than the current distribution. The distribution is correct here. What makes them feel fragmented is that none of the three individual locations states the governing invariant." On Lowy rationale
Structural observation worth recording: keep-mounted is a pattern in this codebase, not a one-off — every reactive component that maintains subscriptions across visibility changes will face the same pressure (guards against churn, pending gates, null filters). Worth naming in architecture notes for future component design. Deeper structural gap neither PR closes: "Both PRs are host-level patches for Pierre's unconditional Comparison with #824PR #824 ships the same #817 fix (which we've now adopted) but doesn't address #818 (state loss on panel toggle / Inspector switch). This PR carries both fixes plus the three #818 follow-ups (memoized reset key, ignore-null in |
1 participant
Two long-standing irritations in the right-panel Code browser, fixed together. Switching the right panel to Inspector or collapsing it lost the selected file and tree expansion (#818); typing a filter and clicking a result cleared the filter (#817). The fixes are independent but ship as one because the regressions surface in the same component.
Two bugs, two root causes
<Show when={!collapsed()}>andmatch(activeTab())both unmounted CodeTab, discarding its local state<Show>(Resizable already shrinks to zero width viasizes=[1,0]) and replacematch()with both children rendered + TailwindhiddentogglehandleRowClicksynchronously callscontroller.closeSearch()after firing selection (closeSearch: isSearchOpenis hardcoded infileTreeRowClickPlan.jswith no opt-out)@kolu/solid-pierre'sFileTreewrapper, re-applyprops.searchQueryon the next microtask insideonSelectionChange— runs after Pierre's clear, keeps the wrapper push-only#818 had a tail of its own
Once CodeTab stopped unmounting, three latent issues in CodeTab.tsx surfaced and had to be fixed in this PR:
on([repoPath, view], …)re-runs its callback whenever any underlying signal emits — and thepreferencescell ticks for unrelated updates too. Memoize the projected(repoPath, view)tuple as a string key so the effect fires only on real value transitions.handleSelectmistook Pierre'snullfor user deselect. Pierre firesonSelectionChange([])fromresetPathsand on tear-down, not just user intent. The Code tab has no UX for explicit deselect (user switches by clicking another file), so ignore null events.gitStatus/fsListAllto resubscribe (input function returns a fresh object literal each cycle), briefly droppingtreePaths()to[]. Gate the "selection no longer in tree" check on the relevant stream'spending()accessor — once the stream has delivered, an empty paths set is genuinely authoritative (post-commit, post-rm).The ordering matters: the reset-effect memo alone fixes the obvious case; the latter two harden CodeTab against transient empty states that the keep-mounted approach now exposes to the membership and selection paths.
Test surface
Three regression scenarios in
code-tab.feature:Selected file survives panel collapse and reopen— covers Code browser doesn't remember state #818's collapse pathSelected file survives Inspector tab switch— covers Code browser doesn't remember state #818's tab-switch path (this one drove all four follow-up commits)Filter survives clicking a filtered result— covers Clicking resets search results in Code browser #817Plus four new step definitions (
right panel tab \"{kind}\",type \"{value}\" into the Code tab filter,the Code tab filter input should contain \"{value}\", and a correctedthe right panel should not be visiblethat asserts effective collapse via bounding-box width — the panel now stays in DOM at ~1px width when collapsed, since aborder-lsurvivessizes=[1,0]).Try it locally
```sh
nix run github:juspay/kolu/dull-type
```
Closes #817. Closes #818.
Generated by `/do` on Claude Code (model `claude-opus-4-7`).