From 5b09d20749298f5bb12bea5abfc7e9e30ed3ed6a Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 01:20:50 +0800 Subject: [PATCH 01/13] =?UTF-8?q?feat(sessions):=20pin=20browse=20modes=20?= =?UTF-8?q?=E2=80=94=20recency=20order,=20ungroup,=20only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three complaints about the pinned zone, one header. - Order: lastTimestamp DESC instead of pinnedAt ASC. The old order buried the session touched five minutes ago under months-old pins; the hover-suppression from PR #136 already absorbs the layout movement that ordering was avoiding. - Collapse now UNGROUPS instead of hiding: pins fall back to their chronological slot with the star. That is the "everything in time order" mode, and it removes a state where browsing could make a pinned session invisible. Pins outside the loaded window are appended to the timeline so no mode drops them, and a pinned session is never folded away as a minor session. - New "only" chip on the header: list and search scoped to pins. Both toggles share one line because vertical space is scarce in a menu-bar popup; the header stays rendered on an empty result so the mode is never a trap. Docs: plan doc 4.4 rewritten to what shipped (it still described dual placement and pinnedAt desc), 8's settled questions closed, and 4.5/4.6/6 record the 2026-08-20 measurements behind the next two PRs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- CHANGELOG.md | 8 ++ README.md | 11 ++- docs/session-finding-plan.md | 146 +++++++++++++++++++++++---- package.json | 2 +- src/switcher-ui.tsx | 187 +++++++++++++++++++++++++++++------ 5 files changed, 303 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fd8191..0b63143 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 1.0.85 + +- Feat: three ways to browse pinned sessions, from the same `๐Ÿ“Œ Pinned (N)` header + - **Pinned zone now sorts by recency** like every other list (it used to sort by when you pinned, which buried the session you touched five minutes ago beneath months-old pins) + - **Collapsing the zone now ungroups instead of hiding**: pinned sessions fall back to their normal chronological position, still marked โ˜… โ€” that gives you a pure "everything in time order" list, and no browsing state can make a pinned session invisible any more + - **New `only` toggle** on the right of the header: show โ€” and search โ€” pinned sessions only + - A pinned session is never folded away into the "minor sessions" group, and pins older than the loaded list stay visible in every mode + ## 1.0.84 - Feat: session pins & manual hide โ€” session-finding Batch 1 PR-2 (plan: `docs/session-finding-plan.md` ยง4.4) diff --git a/README.md b/README.md index 5895900..61db21c 100644 --- a/README.md +++ b/README.md @@ -24,9 +24,16 @@ CodeV can list, search, and resume Claude Code sessions. Press `โŒƒ+โŒ˜+R` to op Search covers **every session and every user prompt you ever typed** (not just the ~100 most recent sessions shown in the list) plus titles, branches, PR links, and last AI replies. When a match sits in the middle of a conversation, the row shows a `โŒ• #N โ€ฆ` snippet with the surrounding context. Closed one-shot sessions (โ‰ค2 messages, untitled, no PR) fold into an expandable "minor sessions" row to keep the list scannable. -**Pin** the sessions you keep coming back to (hover ๐Ÿ“Œ on a row, or `โŒ˜D` on the selected row): they **move into** a collapsible **๐Ÿ“Œ Pinned** zone at the top (no duplicate left in the timeline; search still shows everything) โ€” works even for old sessions found via deep search. **Hide** one-offs you never want in the main flow (hover โŠ˜, or `โ‡งโŒ˜D`): they move into the minor-sessions fold, stay searchable, and can be unhidden from inside the fold (they carry a persistent โŠ˜ marker there). Pins and hides live in `~/.config/codev/session-marks.json`, shared across accounts. +**Pin** the sessions you keep coming back to (hover ๐Ÿ“Œ on a row, or `โŒ˜D` on the selected row): they **move into** a **๐Ÿ“Œ Pinned** zone at the top, ordered by recency like the rest of the list โ€” works even for old sessions found via deep search. **Hide** one-offs you never want in the main flow (hover โŠ˜, or `โ‡งโŒ˜D`): they move into the minor-sessions fold, stay searchable, and can be unhidden from inside the fold (they carry a persistent โŠ˜ marker there). Pins and hides live in `~/.config/codev/session-marks.json`, shared across accounts. -Keyboard semantics worth knowing: the shortcuts act on the **selected row** (the one with the blue left border โ€” hovering selects), and require an explicit selection. `โŒ˜D` = pin/unpin toggle; `โ‡งโŒ˜D` = hide (on a pinned row this unpins *and* folds in one step โ€” pin and hide are mutually exclusive). Collapsing the zone is a mouse action on the `๐Ÿ“Œ Pinned (N)` header; when the last pin is removed the zone disappears entirely (that's normal, not a collapse). +The `๐Ÿ“Œ Pinned (N)` header carries two independent toggles: + +| Click | Effect | +|---|---| +| the `โ–พ ๐Ÿ“Œ Pinned (N)` label | **Group / ungroup.** Ungrouped (`โ–ธ`), pinned sessions drop back into their normal chronological position with a โ˜… instead of sitting in a block at the top โ€” use it when you want one list purely in time order. Nothing is ever hidden either way. | +| the `only` chip on the right | **Pinned only.** The list โ€” and the search box โ€” is scoped to pinned sessions. Click again to leave. | + +Keyboard semantics worth knowing: the shortcuts act on the **selected row** (the one with the blue left border โ€” hovering selects), and require an explicit selection. `โŒ˜D` = pin/unpin toggle; `โ‡งโŒ˜D` = hide (on a pinned row this unpins *and* folds in one step โ€” pin and hide are mutually exclusive). When the last pin is removed the header disappears entirely (that's normal, not a collapse). A pinned session is never folded away as a "minor session", whatever its message count. **Simple rule**: when running multiple sessions in the same project directory at the same time, give each running session a name. Closed sessions don't need names โ€” they won't cause issues. diff --git a/docs/session-finding-plan.md b/docs/session-finding-plan.md index 37320a4..3b8b127 100644 --- a/docs/session-finding-plan.md +++ b/docs/session-finding-plan.md @@ -1,8 +1,12 @@ # Session-Finding Improvement Plan (search / browse / pins / preview) -> **Status: decided; Batch 1 in flight** (finalized 2026-07-12 via a brainstorm session). +> **Status: Batch 1 shipped; Batch 2/3 not started** (finalized 2026-07-12 via a brainstorm session). > PR-1 "search & noise" (ยง4.1โ€“ยง4.3: B2 highlight + A1/B1 full search + C1 folding) = **PR #132 (merged)**; -> PR-2 "pins" (ยง4.4 D1, incl. C1's manual hide) = **PR #136**; Batch 2/3 not started. +> PR-2 "pins" (ยง4.4 D1, incl. C1's manual hide) = **PR #136 (merged)**, plus a browse-modes +> follow-up (ยง4.4: recency ordering ยท ungroup ยท pinned-only scope). +> **Live review 2026-08-20** re-measured the remaining pain and produced a follow-up queue โ€” +> row readability (ยง4.5), PR-reference canonicalization (ยง4.6), a frecency list (ยง6) โ€” recorded +> below with the measurements that justify each. > This document is the cross-session / cross-model implementation reference: every "decision" > below was confirmed point-by-point with the user โ€” do not re-open decided options; > implementation details (ยง4โ€“ยง6) may adapt to what you find. @@ -122,19 +126,35 @@ Pure CSS in `switcher-ui.tsx`. Groundwork in `docs/pin-feature-handoff.md` (its ยง5 [REC] is the base; deviations below are multi-account-era updates). -**UI spec (proposed to the user):** +**UI spec โ€” as shipped** (PR #136 + the browse-modes follow-up; supersedes the original +proposal, which is kept below as the rejected-options record): - Hovering a session row reveals a ๐Ÿ“Œ button on the right; click toggles; pinned rows show a persistent small โ˜…. -- Top of the Sessions list: a collapsible "๐Ÿ“Œ Pinned (N)" section, expanded by default; rows - fully reuse the existing session row (status dot / badges / PR / title all intact). -- A pinned session **also** stays in its chronological position (with the โ˜…) โ€” the section is - a shortcut, not a move (Notion favorites behave the same). -- While searching: the pinned section hides; results are one unified list (matching pinned - rows keep the โ˜…). -- Unpin: hover-`x` on the pinned row (the recent-projects list already has this pattern) or - click ๐Ÿ“Œ again. -- One-line empty-state hint. -- v1 ordering: pinnedAt desc; named groups are v2 (schema keeps a `group?` field now). +- Top of the Sessions list: a "๐Ÿ“Œ Pinned (N)" header, rows fully reuse the existing session + row (status dot / badges / PR / title all intact). +- **Zone-only, not dual placement**: pinning MOVES the session into the zone; the timeline + keeps no duplicate. (User verdict during PR #136 live testing: the duplicate was more noise + than signal. The original proposal โ€” and Notion's favorites โ€” kept both.) +- **Ordering: `lastTimestamp` DESC** (recency), `pinnedAt` DESC as the tie-break for + unresolved placeholder rows. PR #136 shipped `pinnedAt` ASC so a new pin appended at the + zone bottom rather than reshuffling rows under the cursor; that traded away the ordering + every other list uses, and the hover-suppression in `suppressHoverSelection()` already + absorbs the layout movement it was avoiding. +- **Two independent toggles on the header** (both persisted in localStorage): + - label / `โ–พโ–ธ` = **group / ungroup**. Ungrouped, pins fall back to their chronological slot + with the โ˜… โ€” the "everything in time order" browsing mode. It replaces the earlier + collapse semantics, where collapsing removed pins from the zone *and* the timeline at + once, i.e. browsing could make a pinned session invisible. + - `only` chip = **scope**: list and search are restricted to pinned sessions. + - Both toggles live on one line because vertical space is the scarce resource in a + menu-bar popup. +- While searching: the zone does not group; results are one unified list (matching pinned + rows keep the โ˜…). With `only` on, the search is scoped to pins. +- A pinned session is never folded into the minor-sessions group regardless of its stats + (pinning is an explicit "keep this"); pins outside the loaded window are appended to the + timeline when ungrouped, so no mode can drop them. +- Unpin: click ๐Ÿ“Œ again, or `โŒ˜D` on the selected row. +- Named groups are v2 (the schema already keeps a `group?` field). **Store (deviation from the handoff [REC], justified: multi-account era + hidden list too):** - Single file `~/.config/codev/session-marks.json` (one per machine, cross-account; @@ -154,6 +174,93 @@ multi-account-era updates). - Edge cases (v1 ignores them; re-pin manually if hit): explicit `--fork-session`, cross-account copy-fork (issue #128). +### 4.5 Row readability: a matched row must show *why* it matched (2026-08-20 review) + +The user's report โ€” "a long title still filters correctly, but I see no highlight" โ€” has three +independent causes, all confirmed in code, plus one finding that turned out to be **bigger than +the reported symptom**. + +**Measured on the user's machine (2026-08-20), 125 unique custom titles:** + +| Measurement | Value | +|---|---| +| Title length | median **44** chars, max **165** | +| Longer than the UI's 35-char hard slice | **64%** | +| Chain-style (`A -> B > C`, newest step at the END) | **38%** | +| **Titles sharing their first 35 chars with another title** | **48/125 = 38%**, in 13 groups | +| Largest such group | **8 sessions** all rendering as `fred-ff nextjs backend and mcp arch` | +| First prompts longer than the 50-char slice (`both` display mode) | **39%** (of 522 sessions) | +| Last prompts longer than the 40-char slice | **42%** | +| Titled sessions whose title never appears as a `/rename` prompt | **27/78 = 35%** | + +โ‡’ The title column โ€” the user's primary identification signal, since they hand-title every +non-throwaway session โ€” **is visually ambiguous for 38% of sessions even without searching**. +Fixing the highlight is necessary; fixing identifiability is worth more. + +**Causes:** + +- **R1 โ€” hard slice.** `customTitles[id].slice(0, 35)` is applied *before* `Highlighter`, while + `filterSessionsLocally` matches the *full* title โ‡’ a match past char 35 filters the row in and + highlights nothing. +- **R2 โ€” CSS clip.** Row line 1 is `nowrap + overflow:hidden + ellipsis` with all right-hand + badges `flexShrink: 0`; the branch renders after the title and is the first casualty. (Branch + is not hard-sliced โ€” same visible effect, different mechanism.) +- **R3 โ€” snippet suppression hole.** The `โŒ•` line is suppressed when the match is in prompt #0 + or the last prompt, on the grounds that the row already displays those โ€” but it displays only + their first 50/40 chars. A match past the slice is then suppressed *and* invisible. +- **R4 โ€” the two search paths carry different fields.** Main-side deep search sees + `projectName + project + all prompts` and is the only path that returns a snippet; the + renderer filter sees the enrichment fields (title / branch / PR / AI reply) but returns only + a boolean. A title-only match therefore produces no snippet at all โ€” which matters for the + 35% of titled sessions with no `/rename` prompt to fall back on. + +**Fixes (one PR):** + +- **T1 โ€” middle-ellipsis title** (`head โ€ฆ tail`) instead of a head-only slice, plus the full + title on the `title=` attribute. Fixes the 38% ambiguity, and needs no search to pay off. +- **T2 โ€” match-aware window**: when searching, if the first match falls outside the visible + window, move the window to the match (reuse the pure `extractSnippet()` in + `session-search.ts`). Apply to title / first / last / assistant lines. **This subsumes a fix + for R3** โ€” once the visible line scrolls to the match, the suppression is correct again. +- **T3 โ€” branch**: same windowing, and consider moving the branch to line 2 so it stops + competing with the title for line 1. + +**Naming guidance given to the user** (their titles are a running log, and every list UI +truncates from the right): put the discriminator in the first ~20 chars, drop the leading +project name (the row already prints it), and prefer `- ` over an +ever-growing `A > B > C` chain. After T1 the constraint relaxes to "head 15 + tail 15 must be +unique", so chaining becomes viable again if each appended step stays short. + +### 4.6 PR-reference canonicalization: `#123` โ‡„ the full URL (2026-08-20 review) + +Searching a session's *own* PR works either way today, because `filterSessionsLocally` builds +the badge haystack as `PR # `. Searching **user prompts** does not: the haystack is the +raw prompt text, so the query has to use the same form the prompt happened to use. + +**Measured**: 2,506 (session, PR-number) pairs mentioned across all prompts โ€” + +| Forms present in the prompts | Pairs | Consequence today | +|---|---|---| +| both `#N` and the URL | 486 (**19.4%**) | either query finds it | +| URL only | 960 (**38.3%**) | searching `#N` misses it | +| `#N` only | 1,060 (**42.3%**) | searching the URL misses it | + +โ‡’ **80.6% of PR mentions are reachable by only one of the two forms.** + +**Fix**: a pure term parser in `session-search.ts`, applied to *both* the main-side deep search +and the renderer filter (one comparison, two callers โ€” not two implementations). A query word +recognized as a PR reference (`https://github.com/o/r/pull/N`, `#N`, `o/r#N`) matches if any +equivalent form appears in the target (`#N`, `/pull/N`, `/issues/N`, `o/r#N`). **Require a +delimiter โ€” never match a bare number** (`#1598` must not hit `15980`). + +Cost: a query with no PR reference takes exactly today's path (one `String.includes` per word); +a PR term costs at most 3 `includes` instead of 1, cheapest form first. Full-corpus search is +5โ€“20ms behind a 180ms debounce, so this stays imperceptible โ€” but measure and record the timing +in the PR. + +Free follow-on: the same parser gives the ยง6 `B4` filters (`is:pinned`, `has:pr`, `pr:1598`, +`project:`, `branch:`, `after:`). + ## 5. Batch 2 โ€” structural investments ### 5.1 C4: preview / detail (v1 card โ†’ v2 pane) @@ -205,7 +312,8 @@ multi-account-era updates). | Item | Content | Note | |---|---|---| | D3 `/pin` | Custom slash command: leverages Claude Code's slash **autocomplete** (answers the user's dislike of `!` having none); the command runs `codev pin`; sessionId from the **`CLAUDE_CODE_SESSION_ID` env var** ([FACT], ยง7); accepts one LLM turn (user OK'd). Args possible: `/pin as "โ€ฆ"` | UI pin remains primary | -| B4 filters | `project:` `branch:` `account:` `has:pr` `msgs:>10` `after:` chips | | +| B4 filters | `project:` `branch:` `account:` `has:pr` `msgs:>10` `after:` `is:pinned` chips | **Promoted** (2026-08-20): the ยง4.6 term parser does most of the work, and `is:pinned` overlaps the pinned-only scope | +| D4 "Frequent" list | A frecency scope (`distinct active days ร— log(1+prompts) ร— exp(-age/14d)`) alongside the pinned scope. **Derive it from `history.jsonl`, which is already loaded** โ€” no click instrumentation, so no weeks-long cold start | **Measured 2026-08-20, and the numbers argue for modest expectations**: only 3 of the frecency top-10 are outside the recency top-20, so most of it is a re-ordering of rows you can already see. It does **not** subsume pins โ€” of the user's 7 real pins, 3 rank in the frecency top-10 but three others rank #26/#63/#72, because a pin is often exactly the *low*-activity session you refuse to lose. The two intents are complementary: frecency = "I keep coming back", pin = "I decided this matters" | | A4-lite | "Generate title" button in the preview (haiku, writes a custom title) | No batch auto-summarizing | | C3 chain collapse | **Essentially defunct** (2026-07-12): normal resumes reuse the sessionId (ยง4.4) โ€” no generation chains exist; only meaningful if `--fork-session` / copy-fork become common | Kept for the record | @@ -248,9 +356,11 @@ multi-account-era updates). ## 8. Open questions (decide during implementation) -- "Pinned section AND chronological position both show the row" is the proposed default; the - user hasn't given a final verdict (switch to section-only if they object). -- `session-marks.json` single file vs. the handoff's `~/.claude/codev-status/pinned.json`: - this doc leans to the former (multi-account + hidden list); revisit at implementation time. +- ~~"Pinned section AND chronological position both show the row"~~ โ€” **settled**: dual + placement was rejected by the user during PR #136 live testing (zone-only). The browse-modes + follow-up then gave the chronological view back as an explicit *ungroup* toggle rather than + as a duplicate row; see ยง4.4. +- ~~`session-marks.json` single file vs. the handoff's `~/.claude/codev-status/pinned.json`~~ โ€” + **settled**: single cross-account file at `~/.config/codev/session-marks.json` (shipped). - Whether C4 v1 card and v2 pane ship together: judge by effort at the time. - Whether FTS indexes thinking blocks: v1 no (size/noise), keep a flag. diff --git a/package.json b/package.json index 986878e..61fc688 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "CodeV", "productName": "CodeV", - "version": "1.0.84", + "version": "1.0.85", "description": "Quick switcher for VS Code, Cursor, and Claude Code sessions", "repository": { "type": "git", diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index 97ecf33..8f0d440 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -46,12 +46,35 @@ const MINOR_FOLD_BAR_STYLE = { flexShrink: 0, } as const; -// Header row of the pinned zone at the top of the Sessions list. +// Header row of the pinned zone at the top of the Sessions list. It carries +// two independent toggles on one line โ€” the label groups/ungroups the zone, +// the "only" chip scopes browsing and search to pins โ€” so neither costs +// vertical space, which is the scarce resource in a menu-bar popup. const PINNED_HEADER_STYLE = { padding: '6px 10px 2px 24px', color: '#c9a227', fontSize: '12px', + display: 'flex', + alignItems: 'center', + justifyContent: 'space-between', + gap: '8px', +} as const; + +const PINNED_ONLY_CHIP_STYLE = { + fontSize: '10px', + borderRadius: '3px', + padding: '1px 6px', cursor: 'pointer', + border: '1px solid #6b5a1e', + color: '#c9a227', + backgroundColor: 'transparent', +} as const; + +const PINNED_ONLY_CHIP_ACTIVE_STYLE = { + ...PINNED_ONLY_CHIP_STYLE, + border: '1px solid #c9a227', + color: '#1e1e1e', + backgroundColor: '#c9a227', } as const; // Global styles for the switcher UI (moved from index.css) @@ -415,6 +438,16 @@ function SwitcherApp() { return false; } }); + // Browse/search SCOPE: while on, only pinned sessions are listed and the + // search box searches inside them. Independent of the collapse toggle, which + // only decides whether pins are grouped at the top or left in time order. + const [pinnedOnly, setPinnedOnly] = useState(() => { + try { + return localStorage.getItem('codev-pinned-only') === '1'; + } catch { + return false; + } + }); // Pinned sessions living outside the loaded list (fetched by id) const [extraPinnedSessions, setExtraPinnedSessions] = useState([]); const extraPinnedKeyRef = useRef(''); @@ -533,14 +566,30 @@ function SwitcherApp() { // A user-hidden session is forced into the fold regardless of its stats. const isSearchingSessions = sessionSearchValue.trim().length > 0; const hiddenSet = new Set(sessionMarks.hidden); + const hasPins = Object.keys(sessionMarks.pins).length > 0; + // Two independent browse states, both driven from the zone header: + // - "only" scopes the list (and the search) to pinned sessions. + // - collapsing the zone UNGROUPS rather than hides: pins fall back to their + // chronological slot, still marked โ˜…. That is the "everything in time + // order" mode, and it means no browsing state can make a pin invisible + // (the old collapse dropped them from the zone AND the timeline at once). + const pinnedOnlyActive = pinnedOnly && hasPins; + const groupPinned = + !isSearchingSessions && !pinnedOnlyActive && !pinnedCollapsed; const majorSessions: any[] = []; const minorSessions: any[] = []; for (const s of sessions) { - // Pinned sessions live in the zone ONLY (user verdict: the timeline - // duplicate was more noise than signal). Search still shows everything. - if (!isSearchingSessions && sessionMarks.pins[s.sessionId]) continue; + const isPinned = !!sessionMarks.pins[s.sessionId]; + if (pinnedOnlyActive && !isPinned) continue; + // Lifted into the zone โ€” no second copy in the timeline (user verdict: + // the duplicate was more noise than signal). + if (groupPinned && isPinned) continue; const minor = !isSearchingSessions && + // An ungrouped pin must never fold into the minor group: pinning is an + // explicit "keep this", and a pinned session can still be a short + // untitled one that the junk predicate would happily fold away. + !isPinned && (hiddenSet.has(s.sessionId) || (activeDetectionReady && isMinorSession(s, !!customTitles[s.sessionId], !!prLinks[s.sessionId]))); @@ -567,10 +616,7 @@ function SwitcherApp() { pinnedById.set(s.sessionId, s); } } - // pinnedAt ASC: a new pin APPENDS at the zone bottom instead of reshuffling - // the existing zone rows โ€” less layout movement under the cursor. const pinnedRows = Object.entries(sessionMarks.pins) - .sort(([, a], [, b]) => (a.pinnedAt || '').localeCompare(b.pinnedAt || '')) .map(([id, info]) => { // Fall back to a placeholder built from the pin record itself: a pin // can be momentarily (VS Code sessions are absent from history.jsonl @@ -593,17 +639,59 @@ function SwitcherApp() { return { ...s, __pinnedRow: true, + __pinnedAt: info.pinnedAt || '', isActive: s.sessionId in activeStateRef.current || s.isActive, activePid: activeStateRef.current[s.sessionId] ?? s.activePid, }; - }); - const showPinnedZone = !isSearchingSessions && pinnedRows.length > 0; - const visiblePinnedRows = showPinnedZone && !pinnedCollapsed ? pinnedRows : []; - - const displayedSessions = [ - ...visiblePinnedRows, - ...(minorsExpanded ? [...majorSessions, ...minorSessions] : majorSessions), - ]; + }) + // Recency first, like every other list in the app. The previous pinnedAt + // ASC order (chosen so a new pin appended at the bottom instead of + // reshuffling rows under the cursor) buried the session touched five + // minutes ago beneath months-old pins; the hover-suppression added in + // PR #136 already absorbs the layout movement that ordering was avoiding. + // Unresolved placeholders carry lastTimestamp 0 and sink to the bottom, + // where pinnedAt DESC puts the newest pin first among them. + .sort( + (a, b) => + (b.lastTimestamp || 0) - (a.lastTimestamp || 0) || + (b.__pinnedAt || '').localeCompare(a.__pinnedAt || ''), + ); + const showPinnedZone = groupPinned && pinnedRows.length > 0; + const visiblePinnedRows = showPinnedZone ? pinnedRows : []; + // The header owns both toggles, so it must stay reachable in every state + // that has pins โ€” including during a search, where turning "only" on is how + // you scope the results, and including an empty result set, which would + // otherwise trap the user inside pinned-only mode with no way back. + const canGroupPins = !isSearchingSessions && !pinnedOnlyActive; + + // A pin older than the loaded window exists ONLY in pinnedRows (resolved by + // id, with a placeholder when even that fails). The zone renders it while + // grouping is on; once ungrouped it would vanish from both places, so append + // it to the timeline instead. The loaded list is the top-N by recency, so + // anything outside it is older than every row already there and belongs at + // the bottom. + const timelineIds = new Set(); + for (const s of majorSessions) timelineIds.add(s.sessionId); + for (const s of minorSessions) timelineIds.add(s.sessionId); + const ungroupedPins = + !groupPinned && !pinnedOnlyActive && !isSearchingSessions + ? pinnedRows.filter((s: any) => !timelineIds.has(s.sessionId)) + : []; + const timelineRows = [...majorSessions, ...ungroupedPins]; + + const displayedSessions = + pinnedOnlyActive && !isSearchingSessions + ? // Same reason: scope to the resolved pin set rather than filtering + // `sessions`, which would silently drop the out-of-window ones. + pinnedRows + : [ + ...visiblePinnedRows, + ...(minorsExpanded + ? [...timelineRows, ...minorSessions] + : timelineRows), + ]; + // Row index the expanded minor-group header sits above. + const minorFoldHeaderIndex = visiblePinnedRows.length + timelineRows.length; // Pin/hide moves rows under a STATIONARY cursor; Chromium then re-hit-tests // and fires mouseenter on whatever row slid under the mouse, teleporting the @@ -668,6 +756,16 @@ function SwitcherApp() { // same as the minors fold collapse does. setSelectedSessionIndex(0); }; + const togglePinnedOnly = () => { + setPinnedOnly((prev) => { + const next = !prev; + try { + localStorage.setItem('codev-pinned-only', next ? '1' : '0'); + } catch {} + return next; + }); + setSelectedSessionIndex(0); + }; // Pinning/hiding inserts or removes rows above the selection, shifting every // index โ€” without re-anchoring, the next โŒ˜D would act on an unintended row. @@ -1640,35 +1738,64 @@ function SwitcherApp() {
- {sessions.length === 0 ? ( -
- {sessionSearchValue ? 'โš ๏ธ No matching sessions found' : '๐Ÿค– No Claude Code sessions found'} -
- ) : (<> - {showPinnedZone && ( -
0 && ( +
+ e.preventDefault()} - onClick={togglePinnedCollapsed} + onClick={canGroupPins ? togglePinnedCollapsed : undefined} onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { + if (canGroupPins && (e.key === 'Enter' || e.key === ' ')) { e.preventDefault(); togglePinnedCollapsed(); } }} - style={PINNED_HEADER_STYLE} + style={{ cursor: canGroupPins ? 'pointer' : 'default' }} > - {pinnedCollapsed ? 'โ–ธ' : 'โ–พ'} ๐Ÿ“Œ Pinned ({pinnedRows.length}) -
- )} + {canGroupPins ? (pinnedCollapsed ? 'โ–ธ ' : 'โ–พ ') : ''}๐Ÿ“Œ Pinned ({pinnedRows.length}) + + e.preventDefault()} + onClick={togglePinnedOnly} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + togglePinnedOnly(); + } + }} + style={pinnedOnlyActive ? PINNED_ONLY_CHIP_ACTIVE_STYLE : PINNED_ONLY_CHIP_STYLE} + > + only + +
+ )} + {displayedSessions.length === 0 && minorSessions.length === 0 ? ( +
+ {pinnedOnlyActive + ? 'โš ๏ธ No pinned session matches โ€” click "only" above to leave pinned-only' + : sessionSearchValue ? 'โš ๏ธ No matching sessions found' : '๐Ÿค– No Claude Code sessions found'} +
+ ) : (<> {displayedSessions.map((session, index) => ( {visiblePinnedRows.length > 0 && index === visiblePinnedRows.length && (
)} - {minorsExpanded && minorSessions.length > 0 && index === visiblePinnedRows.length + majorSessions.length && ( + {minorsExpanded && minorSessions.length > 0 && index === minorFoldHeaderIndex && (
Date: Thu, 20 Aug 2026 01:26:29 +0800 Subject: [PATCH 02/13] refactor(sessions): extract the list view as a pure, tested module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Which rows appear, in which group, in which order was ~130 lines inline in a 2000-line component, so the only way to check it was to run the app โ€” and it is exactly where the bugs have been (PR #136 needed five rounds of live testing, all list/index interactions). Now three independent browse states multiply together, so the matrix earned a test. src/session-list-view.ts is pure (no fs, electron or React) and moved verbatim; 10 tests cover grouped / ungrouped / pinned-only / pinned-only+search plus two invariants that no type check can reach: an out-of-window pin appears exactly once in EVERY mode, and a pinned session is never folded away as a minor session. Mutation-verified rather than assumed โ€” reverting each guard in turn (pin-guard in the minor predicate, the out-of-window append, the recency order, the pinned-only filter) turns the suite red, and restoring it turns it green. 63 tests pass. Also fixes a type annotation the extraction surfaced: sessions carry epoch-ms numbers, but formatRelativeTime declared `string` โ€” it only survived because every caller went through an `any` row. No behavior change (new Date takes both). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/session-list-view.test.ts | 141 ++++++++++++++++++++ src/session-list-view.ts | 235 ++++++++++++++++++++++++++++++++++ src/switcher-ui.tsx | 174 +++++++------------------ 3 files changed, 425 insertions(+), 125 deletions(-) create mode 100644 src/session-list-view.test.ts create mode 100644 src/session-list-view.ts diff --git a/src/session-list-view.test.ts b/src/session-list-view.test.ts new file mode 100644 index 0000000..bcaea19 --- /dev/null +++ b/src/session-list-view.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, it } from 'vitest'; + +import { buildSessionListView, BuildListViewArgs } from './session-list-view'; + +// Recency-sorted, like the real list. `old` sits outside the loaded window on +// purpose (see `outOfWindowPin`), and `junk` satisfies the minor predicate. +const recent = { sessionId: 'recent', lastTimestamp: 500, messageCount: 40 }; +const middle = { sessionId: 'middle', lastTimestamp: 300, messageCount: 20 }; +const junk = { sessionId: 'junk', lastTimestamp: 200, messageCount: 1 }; +const outOfWindowPin = { sessionId: 'old', lastTimestamp: 10, messageCount: 90 }; + +const at = (iso: string) => ({ pinnedAt: iso, cwd: '/tmp/proj' }); + +const build = (over: Partial = {}) => + buildSessionListView({ + sessions: [recent, middle, junk], + allSessions: [recent, middle, junk], + extraPinnedSessions: [], + pins: {}, + hidden: [], + activePids: {}, + hasCustomTitle: () => false, + hasPrLink: () => false, + isSearching: false, + pinnedOnly: false, + pinnedCollapsed: false, + minorsExpanded: false, + activeDetectionReady: true, + ...over, + }); + +const ids = (rows: { sessionId: string }[]) => rows.map((r) => r.sessionId); + +describe('buildSessionListView โ€” grouping', () => { + it('grouped: pins render in the zone and leave no duplicate in the timeline', () => { + const v = build({ pins: { middle: at('2026-01-01T00:00:00Z') } }); + expect(ids(v.visiblePinnedRows)).toEqual(['middle']); + expect(ids(v.majorSessions)).toEqual(['recent']); // junk folded + expect(ids(v.displayedSessions)).toEqual(['middle', 'recent']); + expect(v.groupPinned).toBe(true); + }); + + it('ungrouped: pins fall back into the timeline instead of disappearing', () => { + const v = build({ + pins: { middle: at('2026-01-01T00:00:00Z') }, + pinnedCollapsed: true, + }); + expect(v.visiblePinnedRows).toEqual([]); + // Still exactly once, and back in its chronological slot. + expect(ids(v.displayedSessions)).toEqual(['recent', 'middle']); + expect(v.groupPinned).toBe(false); + }); + + it('orders the zone by recency, newest pin first among unresolved rows', () => { + const v = build({ + pins: { + middle: at('2026-01-01T00:00:00Z'), // pinned first, but older activity + recent: at('2026-06-01T00:00:00Z'), + ghostA: at('2026-02-01T00:00:00Z'), // unresolvable -> lastTimestamp 0 + ghostB: at('2026-03-01T00:00:00Z'), + }, + }); + expect(ids(v.pinnedRows)).toEqual(['recent', 'middle', 'ghostB', 'ghostA']); + // An unresolved pin still renders as a row, with no misleading msg count. + expect(v.pinnedRows[3].messageCount).toBeUndefined(); + }); +}); + +describe('buildSessionListView โ€” pinned-only scope', () => { + it('drops every non-pinned row while browsing', () => { + const v = build({ + pins: { middle: at('2026-01-01T00:00:00Z') }, + pinnedOnly: true, + }); + expect(ids(v.displayedSessions)).toEqual(['middle']); + expect(v.pinnedOnlyActive).toBe(true); + expect(v.canGroupPins).toBe(false); // grouping is meaningless in this scope + }); + + it('scopes search results to pins too', () => { + const v = build({ + sessions: [recent, middle], // as if both matched the query + pins: { middle: at('2026-01-01T00:00:00Z') }, + pinnedOnly: true, + isSearching: true, + }); + expect(ids(v.displayedSessions)).toEqual(['middle']); + }); + + it('is inert with no pins, so the scope can never blank the list', () => { + const v = build({ pinnedOnly: true }); + expect(v.pinnedOnlyActive).toBe(false); + expect(ids(v.displayedSessions)).toEqual(['recent', 'middle']); + }); +}); + +describe('buildSessionListView โ€” invariants across the whole matrix', () => { + const pins = { old: at('2026-01-01T00:00:00Z') }; + const args = { + pins, + // `old` is outside the loaded list; only the by-id fetch resolves it. + extraPinnedSessions: [outOfWindowPin], + }; + + it('a pin outside the loaded window appears exactly once in every mode', () => { + const modes: Partial[] = [ + {}, + { pinnedCollapsed: true }, + { pinnedOnly: true }, + { minorsExpanded: true }, + { pinnedCollapsed: true, minorsExpanded: true }, + ]; + for (const mode of modes) { + const shown = ids(build({ ...args, ...mode }).displayedSessions); + expect(shown.filter((id) => id === 'old')).toEqual(['old']); + } + }); + + it('never folds a pinned session away as a minor session', () => { + // `junk` would fold on its own stats (1 msg, untitled, no PR, closed). + const v = build({ pins: { junk: at('2026-01-01T00:00:00Z') }, pinnedCollapsed: true }); + expect(ids(v.minorSessions)).toEqual([]); + expect(ids(v.displayedSessions)).toContain('junk'); + }); + + it('places the minor-fold header directly after the last timeline row', () => { + const v = build({ ...args, pinnedCollapsed: true, minorsExpanded: true }); + // recent, middle, then the out-of-window pin, then the fold header. + expect(ids(v.displayedSessions)).toEqual(['recent', 'middle', 'old', 'junk']); + expect(v.minorFoldHeaderIndex).toBe(3); + expect(v.displayedSessions[v.minorFoldHeaderIndex].sessionId).toEqual( + v.minorSessions[0].sessionId, + ); + }); + + it('counts explicitly hidden rows separately from junk-predicate ones', () => { + const v = build({ hidden: ['middle'] }); + expect(ids(v.minorSessions)).toEqual(['middle', 'junk']); + expect(v.hiddenMinorCount).toBe(1); + }); +}); diff --git a/src/session-list-view.ts b/src/session-list-view.ts new file mode 100644 index 0000000..c77ab6a --- /dev/null +++ b/src/session-list-view.ts @@ -0,0 +1,235 @@ +/** + * Pure composition of the Sessions list (no fs, no electron, no React) so the + * browse-state matrix is unit-testable. + * + * Everything the Sessions tab renders is one function of state: which rows + * appear, in which group, in which order. That decision used to live inline in + * `switcher-ui.tsx`, where the only way to check it was to run the app โ€” and + * it is exactly where the bugs have been (PR #136 needed five rounds of live + * testing, all of them list/index interactions). + * + * The three browse states are independent, which is what makes the matrix + * worth testing rather than eyeballing: + * + * - `isSearching` โ€” search shows everything, ungrouped. + * - `pinnedOnly` โ€” scope: non-pinned rows drop out, search included. + * - `pinnedCollapsed` โ€” grouping: collapsed UNGROUPS (pins fall back to their + * chronological slot with a โ˜…) rather than hiding, so no combination of + * states can make a pinned session invisible. + * + * Used by: `switcher-ui.tsx`. + */ + +import { isMinorSession } from './session-search'; + +/** A session row. Deliberately loose โ€” the renderer's rows carry enrichment. */ +export interface ListViewSession { + sessionId: string; + project?: string; + projectName?: string; + lastTimestamp?: number; + messageCount?: number; + isActive?: boolean; + activePid?: number; + accountLabel?: string; + // Rows also carry renderer-side enrichment this module never reads; `any` + // keeps the consuming JSX compiling exactly as it did when rows were `any[]`. + [key: string]: any; +} + +export interface PinRecord { + pinnedAt?: string; + cwd?: string; + accountLabel?: string; +} + +export interface BuildListViewArgs { + /** The list to render: the full timeline while browsing, results while searching. Recency-sorted. */ + sessions: ListViewSession[]; + /** Every loaded session โ€” the first source used to resolve a pin to a real row. */ + allSessions: ListViewSession[]; + /** Pins resolved by id from the main-side cache because they fall outside `allSessions`. */ + extraPinnedSessions: ListViewSession[]; + pins: Record; + hidden: string[]; + /** sessionId -> pid for sessions detected as running. */ + activePids: Record; + /** Renderer-side enrichment the junk predicate needs. */ + hasCustomTitle: (sessionId: string) => boolean; + hasPrLink: (sessionId: string) => boolean; + isSearching: boolean; + pinnedOnly: boolean; + pinnedCollapsed: boolean; + minorsExpanded: boolean; + /** Folding waits for the first active-session detection (never fold a just-started session). */ + activeDetectionReady: boolean; +} + +export interface ListView { + /** Every pin as a row, recency-ordered โ€” the zone's content, and the pinned-only list. */ + pinnedRows: ListViewSession[]; + /** The subset actually rendered as a zone (empty unless grouping is on). */ + visiblePinnedRows: ListViewSession[]; + majorSessions: ListViewSession[]; + minorSessions: ListViewSession[]; + /** Flattened render order; row indexes elsewhere in the UI are indexes into this. */ + displayedSessions: ListViewSession[]; + /** Index the expanded minor-group header sits above. */ + minorFoldHeaderIndex: number; + /** How many folded rows got there by an explicit hide rather than the junk predicate. */ + hiddenMinorCount: number; + pinnedOnlyActive: boolean; + groupPinned: boolean; + /** Grouping is meaningless while searching or scoped to pins โ€” the header hides its arrow. */ + canGroupPins: boolean; +} + +/** Resolve one pin to a real row, or synthesize a placeholder from the pin record. */ +const resolvePinnedRow = ( + id: string, + info: PinRecord, + pinnedById: Map, + activePids: Record, +): ListViewSession => { + // A pin can be momentarily unresolvable (VS Code sessions are absent from + // history.jsonl until the closed-scan merges them in) or permanently so + // (transcript cleaned up). Without the placeholder the zone count flaps on + // every tab switch. + const s = pinnedById.get(id) ?? { + sessionId: id, + project: info.cwd || '', + projectName: + (info.cwd || '').split('/').filter(Boolean).pop() || id.slice(0, 8), + firstUserMessage: '', + lastUserMessage: '', + lastTimestamp: 0, + // undefined, not 0: the row renders 'โ€ฆ msgs' instead of a misleading + // '0 msgs' while the session is unresolved (or permanently gone). + messageCount: undefined, + isActive: false, + accountLabel: info.accountLabel, + }; + return { + ...s, + __pinnedRow: true, + __pinnedAt: info.pinnedAt || '', + isActive: s.sessionId in activePids || s.isActive, + activePid: activePids[s.sessionId] ?? s.activePid, + }; +}; + +export const buildSessionListView = ({ + sessions, + allSessions, + extraPinnedSessions, + pins, + hidden, + activePids, + hasCustomTitle, + hasPrLink, + isSearching, + pinnedOnly, + pinnedCollapsed, + minorsExpanded, + activeDetectionReady, +}: BuildListViewArgs): ListView => { + const hiddenSet = new Set(hidden); + const hasPins = Object.keys(pins).length > 0; + const pinnedOnlyActive = pinnedOnly && hasPins; + const groupPinned = !isSearching && !pinnedOnlyActive && !pinnedCollapsed; + const canGroupPins = !isSearching && !pinnedOnlyActive; + + // C1: fold minor (junk) sessions while browsing; searching shows everything. + // Minors keep their recency order but render below the fold row at the end. + // A user-hidden session is forced into the fold regardless of its stats. + const majorSessions: ListViewSession[] = []; + const minorSessions: ListViewSession[] = []; + for (const s of sessions) { + const isPinned = !!pins[s.sessionId]; + if (pinnedOnlyActive && !isPinned) continue; + // Lifted into the zone โ€” no second copy in the timeline (user verdict: + // the duplicate was more noise than signal). + if (groupPinned && isPinned) continue; + const minor = + !isSearching && + // An ungrouped pin must never fold into the minor group: pinning is an + // explicit "keep this", and a pinned session can still be a short + // untitled one that the junk predicate would happily fold away. + !isPinned && + (hiddenSet.has(s.sessionId) || + (activeDetectionReady && + isMinorSession(s, hasCustomTitle(s.sessionId), hasPrLink(s.sessionId)))); + (minor ? minorSessions : majorSessions).push(s); + } + // Manually hidden sessions may be titled/long โ€” keep the fold label honest. + const hiddenMinorCount = minorSessions.filter((s) => + hiddenSet.has(s.sessionId), + ).length; + + // Pinned rows come from the loaded list when available, else from the by-id + // fetch, else a placeholder. + const pinnedById = new Map(); + for (const s of allSessions) { + if (pins[s.sessionId]) pinnedById.set(s.sessionId, s); + } + for (const s of extraPinnedSessions) { + if (pins[s.sessionId] && !pinnedById.has(s.sessionId)) { + pinnedById.set(s.sessionId, s); + } + } + const pinnedRows = Object.entries(pins) + .map(([id, info]) => resolvePinnedRow(id, info, pinnedById, activePids)) + // Recency first, like every other list in the app. The previous pinnedAt + // ASC order (chosen so a new pin appended at the bottom instead of + // reshuffling rows under the cursor) buried the session touched five + // minutes ago beneath months-old pins; the hover-suppression added in + // PR #136 already absorbs the layout movement that ordering was avoiding. + // Unresolved placeholders carry lastTimestamp 0 and sink to the bottom, + // where pinnedAt DESC puts the newest pin first among them. + .sort( + (a, b) => + (b.lastTimestamp || 0) - (a.lastTimestamp || 0) || + String(b.__pinnedAt || '').localeCompare(String(a.__pinnedAt || '')), + ); + + const visiblePinnedRows = groupPinned && pinnedRows.length > 0 ? pinnedRows : []; + + // A pin older than the loaded window exists ONLY in pinnedRows. The zone + // renders it while grouping is on; once ungrouped it would vanish from both + // places, so append it to the timeline instead. The loaded list is the top-N + // by recency, so anything outside it is older than every row already there + // and belongs at the bottom. + const timelineIds = new Set(); + for (const s of majorSessions) timelineIds.add(s.sessionId); + for (const s of minorSessions) timelineIds.add(s.sessionId); + const ungroupedPins = + !groupPinned && !pinnedOnlyActive && !isSearching + ? pinnedRows.filter((s) => !timelineIds.has(s.sessionId)) + : []; + const timelineRows = [...majorSessions, ...ungroupedPins]; + + const displayedSessions = + pinnedOnlyActive && !isSearching + ? // Same reason: scope to the resolved pin set rather than filtering + // `sessions`, which would silently drop the out-of-window ones. + pinnedRows + : [ + ...visiblePinnedRows, + ...(minorsExpanded + ? [...timelineRows, ...minorSessions] + : timelineRows), + ]; + + return { + pinnedRows, + visiblePinnedRows, + majorSessions, + minorSessions, + displayedSessions, + minorFoldHeaderIndex: visiblePinnedRows.length + timelineRows.length, + hiddenMinorCount, + pinnedOnlyActive, + groupPinned, + canGroupPins, + }; +}; diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index 8f0d440..0940a23 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -5,7 +5,7 @@ import Highlighter from 'react-highlight-words'; import Select, { components, OptionProps } from 'react-select'; import { HoverButton } from './HoverButton'; import PopupDefaultExample from './popup'; -import { isMinorSession } from './session-search'; +import { buildSessionListView } from './session-list-view'; import TerminalTab from './terminal-tab'; type SwitcherMode = 'projects' | 'sessions' | 'terminal'; @@ -319,7 +319,9 @@ const OptionUI = ( }; /** Format relative time for session display */ -const formatRelativeTime = (timestamp: string): string => { +// Sessions carry epoch-ms numbers; the old `string` annotation only survived +// because every caller went through an `any` row. `new Date()` takes both. +const formatRelativeTime = (timestamp: number | string): string => { if (!timestamp) return ''; const diff = Date.now() - new Date(timestamp).getTime(); const minutes = Math.floor(diff / 60000); @@ -561,138 +563,42 @@ function SwitcherApp() { }, 180); }; - // C1: fold minor (junk) sessions while browsing; searching shows everything. - // Minors keep their recency order but render below the fold row at the end. - // A user-hidden session is forced into the fold regardless of its stats. const isSearchingSessions = sessionSearchValue.trim().length > 0; const hiddenSet = new Set(sessionMarks.hidden); const hasPins = Object.keys(sessionMarks.pins).length > 0; - // Two independent browse states, both driven from the zone header: - // - "only" scopes the list (and the search) to pinned sessions. - // - collapsing the zone UNGROUPS rather than hides: pins fall back to their - // chronological slot, still marked โ˜…. That is the "everything in time - // order" mode, and it means no browsing state can make a pin invisible - // (the old collapse dropped them from the zone AND the timeline at once). - const pinnedOnlyActive = pinnedOnly && hasPins; - const groupPinned = - !isSearchingSessions && !pinnedOnlyActive && !pinnedCollapsed; - const majorSessions: any[] = []; - const minorSessions: any[] = []; - for (const s of sessions) { - const isPinned = !!sessionMarks.pins[s.sessionId]; - if (pinnedOnlyActive && !isPinned) continue; - // Lifted into the zone โ€” no second copy in the timeline (user verdict: - // the duplicate was more noise than signal). - if (groupPinned && isPinned) continue; - const minor = - !isSearchingSessions && - // An ungrouped pin must never fold into the minor group: pinning is an - // explicit "keep this", and a pinned session can still be a short - // untitled one that the junk predicate would happily fold away. - !isPinned && - (hiddenSet.has(s.sessionId) || - (activeDetectionReady && - isMinorSession(s, !!customTitles[s.sessionId], !!prLinks[s.sessionId]))); - (minor ? minorSessions : majorSessions).push(s); - } + // Which rows appear, in which group, in which order โ€” one pure function so + // the browse-state matrix (search x pinned-only x grouped) is testable + // without running the app. See src/session-list-view.ts. + const { + pinnedRows, + visiblePinnedRows, + minorSessions, + displayedSessions, + minorFoldHeaderIndex, + hiddenMinorCount, + pinnedOnlyActive, + canGroupPins, + } = buildSessionListView({ + sessions, + allSessions, + extraPinnedSessions, + pins: sessionMarks.pins, + hidden: sessionMarks.hidden, + activePids: activeStateRef.current, + hasCustomTitle: (id: string) => !!customTitles[id], + hasPrLink: (id: string) => !!prLinks[id], + isSearching: isSearchingSessions, + pinnedOnly, + pinnedCollapsed, + minorsExpanded, + activeDetectionReady, + }); // Manually hidden sessions may be titled/long โ€” keep the fold label honest. - const hiddenMinorCount = minorSessions.filter((s: any) => - hiddenSet.has(s.sessionId), - ).length; const minorFoldSuffix = hiddenMinorCount > 0 ? `(โ‰ค2 msgs, untitled ยท ${hiddenMinorCount} hidden)` : '(โ‰ค2 msgs, untitled)'; - // Pinned zone (PR-2): rows come from the loaded list when available, else - // from the by-id fetch. Zone-only model: pinning MOVES the session here - // (the timeline keeps no duplicate; search mode still shows everything). - const pinnedById = new Map(); - for (const s of allSessions) { - if (sessionMarks.pins[s.sessionId]) pinnedById.set(s.sessionId, s); - } - for (const s of extraPinnedSessions) { - if (sessionMarks.pins[s.sessionId] && !pinnedById.has(s.sessionId)) { - pinnedById.set(s.sessionId, s); - } - } - const pinnedRows = Object.entries(sessionMarks.pins) - .map(([id, info]) => { - // Fall back to a placeholder built from the pin record itself: a pin - // can be momentarily (VS Code sessions are absent from history.jsonl - // until the closed-scan merges them in) or permanently unresolvable โ€” - // without this the zone count flaps on every tab switch. - const s = pinnedById.get(id) ?? { - sessionId: id, - project: info.cwd || '', - projectName: - (info.cwd || '').split('/').filter(Boolean).pop() || id.slice(0, 8), - firstUserMessage: '', - lastUserMessage: '', - lastTimestamp: 0, - // undefined, not 0: the row renders 'โ€ฆ msgs' instead of a misleading - // '0 msgs' while the session is unresolved (or permanently gone) - messageCount: undefined, - isActive: false, - accountLabel: info.accountLabel, - }; - return { - ...s, - __pinnedRow: true, - __pinnedAt: info.pinnedAt || '', - isActive: s.sessionId in activeStateRef.current || s.isActive, - activePid: activeStateRef.current[s.sessionId] ?? s.activePid, - }; - }) - // Recency first, like every other list in the app. The previous pinnedAt - // ASC order (chosen so a new pin appended at the bottom instead of - // reshuffling rows under the cursor) buried the session touched five - // minutes ago beneath months-old pins; the hover-suppression added in - // PR #136 already absorbs the layout movement that ordering was avoiding. - // Unresolved placeholders carry lastTimestamp 0 and sink to the bottom, - // where pinnedAt DESC puts the newest pin first among them. - .sort( - (a, b) => - (b.lastTimestamp || 0) - (a.lastTimestamp || 0) || - (b.__pinnedAt || '').localeCompare(a.__pinnedAt || ''), - ); - const showPinnedZone = groupPinned && pinnedRows.length > 0; - const visiblePinnedRows = showPinnedZone ? pinnedRows : []; - // The header owns both toggles, so it must stay reachable in every state - // that has pins โ€” including during a search, where turning "only" on is how - // you scope the results, and including an empty result set, which would - // otherwise trap the user inside pinned-only mode with no way back. - const canGroupPins = !isSearchingSessions && !pinnedOnlyActive; - - // A pin older than the loaded window exists ONLY in pinnedRows (resolved by - // id, with a placeholder when even that fails). The zone renders it while - // grouping is on; once ungrouped it would vanish from both places, so append - // it to the timeline instead. The loaded list is the top-N by recency, so - // anything outside it is older than every row already there and belongs at - // the bottom. - const timelineIds = new Set(); - for (const s of majorSessions) timelineIds.add(s.sessionId); - for (const s of minorSessions) timelineIds.add(s.sessionId); - const ungroupedPins = - !groupPinned && !pinnedOnlyActive && !isSearchingSessions - ? pinnedRows.filter((s: any) => !timelineIds.has(s.sessionId)) - : []; - const timelineRows = [...majorSessions, ...ungroupedPins]; - - const displayedSessions = - pinnedOnlyActive && !isSearchingSessions - ? // Same reason: scope to the resolved pin set rather than filtering - // `sessions`, which would silently drop the out-of-window ones. - pinnedRows - : [ - ...visiblePinnedRows, - ...(minorsExpanded - ? [...timelineRows, ...minorSessions] - : timelineRows), - ]; - // Row index the expanded minor-group header sits above. - const minorFoldHeaderIndex = visiblePinnedRows.length + timelineRows.length; - // Pin/hide moves rows under a STATIONARY cursor; Chromium then re-hit-tests // and fires mouseenter on whatever row slid under the mouse, teleporting the // selection (and overriding the re-anchor below) โ€” the same phenomenon the @@ -756,6 +662,24 @@ function SwitcherApp() { // same as the minors fold collapse does. setSelectedSessionIndex(0); }; + // Removing the last pin drops out of pinned-only implicitly (the scope needs + // pins to mean anything), so clear the STORED preference too โ€” otherwise the + // next pin, possibly weeks later, silently collapses the list to that one + // row. Keyed off a real non-empty โ†’ empty transition, because at mount the + // marks are empty until the IPC load lands and a plain `!hasPins` check + // would wipe a legitimately stored preference. + const hadPinsRef = useRef(false); + useEffect(() => { + if (hasPins) { + hadPinsRef.current = true; + return; + } + if (!hadPinsRef.current || !pinnedOnly) return; + setPinnedOnly(false); + try { + localStorage.setItem('codev-pinned-only', '0'); + } catch {} + }, [hasPins, pinnedOnly]); const togglePinnedOnly = () => { setPinnedOnly((prev) => { const next = !prev; From 77e9ae63c9477c0e81fd995bd7c661cf85f21a70 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 01:59:29 +0800 Subject: [PATCH 03/13] fix(sessions): PR #137 review round 1 (CodeRabbit + cubic) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers independently found the same real gap, so it leads. - Search missed out-of-window pins (CR major + cubic P2). Such a pin lives only in the by-id fetch, and its title/branch/PR are renderer enrichment the main-side prompt search cannot see โ€” so a query for its name found nothing, and in pinned-only mode that read as "no pinned session matches" for a pin visible one keystroke earlier. applySearchFilter now widens its candidate set with the new pure mergeSessionsById, from a ref (this runs in a debounced timeout where React state reads are stale) and only while a query is live, so browsing is byte-identical. Mutation-verified: neutering the merge turns the suite red. 66 tests. - The header reported the unscoped session count next to a pin-filtered list (cubic P2). Scoped modes now report what is on screen. - ListViewSession declared every field the renderer reads and the index signature is `unknown` instead of `any` (CR major); tsc confirms nothing was missed. - Formatting (CR minor): the two new files are fully prettier'd; in switcher-ui.tsx only the line breaks I introduced are fixed. The rest of that file's report is the pre-existing whole-file indentation drift, and reflowing it is what this repo forbids. Not changed: the "future-dated 2026-08-20" finding. CodeRabbit reads UTC; the measurements were taken on the user's machine at UTC+8, where it is already the 20th (commit stamps carry +0800). Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- CHANGELOG.md | 2 ++ src/session-list-view.test.ts | 53 ++++++++++++++++++++++++++++++++--- src/session-list-view.ts | 47 +++++++++++++++++++++++++++---- src/switcher-ui.tsx | 45 ++++++++++++++++++++++++----- 4 files changed, 130 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b63143..2fc56c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ - **Collapsing the zone now ungroups instead of hiding**: pinned sessions fall back to their normal chronological position, still marked โ˜… โ€” that gives you a pure "everything in time order" list, and no browsing state can make a pinned session invisible any more - **New `only` toggle** on the right of the header: show โ€” and search โ€” pinned sessions only - A pinned session is never folded away into the "minor sessions" group, and pins older than the loaded list stay visible in every mode + - Searching now also matches the title / branch / PR link of pinned sessions older than the loaded list โ€” previously such a pin was visible while browsing but reported as "no match" the moment you searched for its name + - The `N sessions` count next to the search box reports the scoped total while `only` is on, instead of the unfiltered one ## 1.0.84 diff --git a/src/session-list-view.test.ts b/src/session-list-view.test.ts index bcaea19..324f4f7 100644 --- a/src/session-list-view.test.ts +++ b/src/session-list-view.test.ts @@ -1,13 +1,21 @@ import { describe, expect, it } from 'vitest'; -import { buildSessionListView, BuildListViewArgs } from './session-list-view'; +import { + buildSessionListView, + BuildListViewArgs, + mergeSessionsById, +} from './session-list-view'; // Recency-sorted, like the real list. `old` sits outside the loaded window on // purpose (see `outOfWindowPin`), and `junk` satisfies the minor predicate. const recent = { sessionId: 'recent', lastTimestamp: 500, messageCount: 40 }; const middle = { sessionId: 'middle', lastTimestamp: 300, messageCount: 20 }; const junk = { sessionId: 'junk', lastTimestamp: 200, messageCount: 1 }; -const outOfWindowPin = { sessionId: 'old', lastTimestamp: 10, messageCount: 90 }; +const outOfWindowPin = { + sessionId: 'old', + lastTimestamp: 10, + messageCount: 90, +}; const at = (iso: string) => ({ pinnedAt: iso, cwd: '/tmp/proj' }); @@ -118,7 +126,10 @@ describe('buildSessionListView โ€” invariants across the whole matrix', () => { it('never folds a pinned session away as a minor session', () => { // `junk` would fold on its own stats (1 msg, untitled, no PR, closed). - const v = build({ pins: { junk: at('2026-01-01T00:00:00Z') }, pinnedCollapsed: true }); + const v = build({ + pins: { junk: at('2026-01-01T00:00:00Z') }, + pinnedCollapsed: true, + }); expect(ids(v.minorSessions)).toEqual([]); expect(ids(v.displayedSessions)).toContain('junk'); }); @@ -126,7 +137,12 @@ describe('buildSessionListView โ€” invariants across the whole matrix', () => { it('places the minor-fold header directly after the last timeline row', () => { const v = build({ ...args, pinnedCollapsed: true, minorsExpanded: true }); // recent, middle, then the out-of-window pin, then the fold header. - expect(ids(v.displayedSessions)).toEqual(['recent', 'middle', 'old', 'junk']); + expect(ids(v.displayedSessions)).toEqual([ + 'recent', + 'middle', + 'old', + 'junk', + ]); expect(v.minorFoldHeaderIndex).toBe(3); expect(v.displayedSessions[v.minorFoldHeaderIndex].sessionId).toEqual( v.minorSessions[0].sessionId, @@ -138,4 +154,33 @@ describe('buildSessionListView โ€” invariants across the whole matrix', () => { expect(ids(v.minorSessions)).toEqual(['middle', 'junk']); expect(v.hiddenMinorCount).toBe(1); }); + + it('shows an out-of-window pin that only the widened search set could match', () => { + // The renderer widens its search candidates with mergeSessionsById, so a + // hit on such a pin's title/branch arrives here inside `sessions`. + const v = build({ + ...args, + sessions: [outOfWindowPin], + isSearching: true, + pinnedOnly: true, + }); + expect(ids(v.displayedSessions)).toEqual(['old']); + }); +}); + +describe('mergeSessionsById', () => { + it('appends only rows the primary list does not already have', () => { + const merged = mergeSessionsById( + [recent, middle], + [middle, outOfWindowPin], + ); + expect(ids(merged)).toEqual(['recent', 'middle', 'old']); + }); + + it('returns the primary list unchanged when there is nothing to add', () => { + const primary = [recent, middle]; + // Identity, not just equality: a keystroke must not churn array identity. + expect(mergeSessionsById(primary, [])).toBe(primary); + expect(mergeSessionsById(primary, [recent])).toBe(primary); + }); }); diff --git a/src/session-list-view.ts b/src/session-list-view.ts index c77ab6a..121a7e5 100644 --- a/src/session-list-view.ts +++ b/src/session-list-view.ts @@ -22,19 +22,29 @@ import { isMinorSession } from './session-search'; -/** A session row. Deliberately loose โ€” the renderer's rows carry enrichment. */ +/** + * A session row as the Sessions tab renders it. Every field the renderer reads + * is declared; the index signature is `unknown` so anything undeclared has to + * be narrowed at the use site rather than silently typed as `any`. + */ export interface ListViewSession { sessionId: string; project?: string; projectName?: string; + firstUserMessage?: string; + lastUserMessage?: string; lastTimestamp?: number; messageCount?: number; isActive?: boolean; activePid?: number; accountLabel?: string; - // Rows also carry renderer-side enrichment this module never reads; `any` - // keeps the consuming JSX compiling exactly as it did when rows were `any[]`. - [key: string]: any; + accountIsAnchor?: boolean; + /** `claude-vscode` for sessions launched from the VS Code extension. */ + entrypoint?: string; + /** Set on rows lifted out of the pin store (zone rows and placeholders). */ + __pinnedRow?: boolean; + __pinnedAt?: string; + [key: string]: unknown; } export interface PinRecord { @@ -84,6 +94,26 @@ export interface ListView { canGroupPins: boolean; } +/** + * Union two row lists by sessionId, first list wins. + * + * Used to widen the search candidate set. A pin outside the loaded window + * exists only in the by-id fetch, and its title / branch / PR link live in + * renderer-side enrichment that the main-side prompt search cannot see โ€” so + * without this, a query matching only those fields finds nothing even though + * the very same pin is visible while browsing. Returns `primary` unchanged + * when there is nothing to add, so a keystroke does not churn array identity. + */ +export const mergeSessionsById = ( + primary: ListViewSession[], + extra: ListViewSession[], +): ListViewSession[] => { + if (extra.length === 0) return primary; + const seen = new Set(primary.map((s) => s.sessionId)); + const added = extra.filter((s) => !seen.has(s.sessionId)); + return added.length === 0 ? primary : [...primary, ...added]; +}; + /** Resolve one pin to a real row, or synthesize a placeholder from the pin record. */ const resolvePinnedRow = ( id: string, @@ -158,7 +188,11 @@ export const buildSessionListView = ({ !isPinned && (hiddenSet.has(s.sessionId) || (activeDetectionReady && - isMinorSession(s, hasCustomTitle(s.sessionId), hasPrLink(s.sessionId)))); + isMinorSession( + s, + hasCustomTitle(s.sessionId), + hasPrLink(s.sessionId), + ))); (minor ? minorSessions : majorSessions).push(s); } // Manually hidden sessions may be titled/long โ€” keep the fold label honest. @@ -192,7 +226,8 @@ export const buildSessionListView = ({ String(b.__pinnedAt || '').localeCompare(String(a.__pinnedAt || '')), ); - const visiblePinnedRows = groupPinned && pinnedRows.length > 0 ? pinnedRows : []; + const visiblePinnedRows = + groupPinned && pinnedRows.length > 0 ? pinnedRows : []; // A pin older than the loaded window exists ONLY in pinnedRows. The zone // renders it while grouping is on; once ungrouped it would vanish from both diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index 0940a23..d84bd62 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -5,7 +5,7 @@ import Highlighter from 'react-highlight-words'; import Select, { components, OptionProps } from 'react-select'; import { HoverButton } from './HoverButton'; import PopupDefaultExample from './popup'; -import { buildSessionListView } from './session-list-view'; +import { buildSessionListView, mergeSessionsById } from './session-list-view'; import TerminalTab from './terminal-tab'; type SwitcherMode = 'projects' | 'sessions' | 'terminal'; @@ -452,6 +452,11 @@ function SwitcherApp() { }); // Pinned sessions living outside the loaded list (fetched by id) const [extraPinnedSessions, setExtraPinnedSessions] = useState([]); + // Mirrored into a ref: applySearchFilter runs from a debounced timeout and + // from setState updaters, where reading React state gives the value captured + // when the callback was created (the stale-closure trap this file has been + // bitten by twice โ€” see sessionSearchRef2). + const extraPinnedSessionsRef = useRef([]); const extraPinnedKeyRef = useRef(''); // Keep the selection on the same session after pin/hide reshuffles the list const reanchorSelectionRef = useRef(null); @@ -500,7 +505,18 @@ function SwitcherApp() { // results (issue #131). Deep matches outside the loaded list are appended, // then everything re-sorts into the usual recency order. const applySearchFilter = (allItems: any[], query: string) => { - const base = filterSessionsLocally(allItems, query); + // Pins outside the loaded window are only in the by-id fetch, and their + // title / branch / PR live in renderer enrichment the main-side prompt + // search cannot see โ€” without widening the candidate set here, searching + // for such a pin's title finds nothing, and in pinned-only mode that reads + // as "no pinned session matches" for a pin that is visible while browsing. + // Only while a query is live: this runs on every keystroke including the + // one that empties the box, and widening the browse list is not this + // function's job. + const candidates = query.trim() + ? mergeSessionsById(allItems, extraPinnedSessionsRef.current) + : allItems; + const base = filterSessionsLocally(candidates, query); if (!query.trim() || deepMatchesRef.current.length === 0) return base; const seen = new Set(base.map((s: any) => s.sessionId)); const extra = deepMatchesRef.current @@ -743,6 +759,7 @@ function SwitcherApp() { if (key === extraPinnedKeyRef.current) return; extraPinnedKeyRef.current = key; if (missing.length === 0) { + extraPinnedSessionsRef.current = []; setExtraPinnedSessions([]); return; } @@ -750,6 +767,7 @@ function SwitcherApp() { // Drop stale responses (a newer pin set superseded this request) if (extraPinnedKeyRef.current !== key) return; const found = result || []; + extraPinnedSessionsRef.current = found; setExtraPinnedSessions(found); if (found.length === 0) return; window.electronAPI.loadSessionEnrichment(found).then((enrichment) => { @@ -1658,7 +1676,9 @@ function SwitcherApp() { }} /> - {sessions.length} sessions + {/* Scoped modes must report what is on screen โ€” an unscoped + count next to a pin-filtered list reads as a bug. */} + {pinnedOnlyActive ? displayedSessions.length : sessions.length} sessions
@@ -1675,7 +1695,10 @@ function SwitcherApp() { onMouseDown={(e) => e.preventDefault()} onClick={canGroupPins ? togglePinnedCollapsed : undefined} onKeyDown={(e) => { - if (canGroupPins && (e.key === 'Enter' || e.key === ' ')) { + if ( + canGroupPins && + (e.key === 'Enter' || e.key === ' ') + ) { e.preventDefault(); togglePinnedCollapsed(); } @@ -1701,7 +1724,11 @@ function SwitcherApp() { togglePinnedOnly(); } }} - style={pinnedOnlyActive ? PINNED_ONLY_CHIP_ACTIVE_STYLE : PINNED_ONLY_CHIP_STYLE} + style={ + pinnedOnlyActive + ? PINNED_ONLY_CHIP_ACTIVE_STYLE + : PINNED_ONLY_CHIP_STYLE + } > only @@ -1711,7 +1738,9 @@ function SwitcherApp() {
{pinnedOnlyActive ? 'โš ๏ธ No pinned session matches โ€” click "only" above to leave pinned-only' - : sessionSearchValue ? 'โš ๏ธ No matching sessions found' : '๐Ÿค– No Claude Code sessions found'} + : sessionSearchValue + ? 'โš ๏ธ No matching sessions found' + : '๐Ÿค– No Claude Code sessions found'}
) : (<> {displayedSessions.map((session, index) => ( @@ -1719,7 +1748,9 @@ function SwitcherApp() { {visiblePinnedRows.length > 0 && index === visiblePinnedRows.length && (
)} - {minorsExpanded && minorSessions.length > 0 && index === minorFoldHeaderIndex && ( + {minorsExpanded && + minorSessions.length > 0 && + index === minorFoldHeaderIndex && (
Date: Thu, 20 Aug 2026 02:04:32 +0800 Subject: [PATCH 04/13] fix(sessions): persist header toggles from effects, not updaters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit nitpick, found in the review BODY rather than as a thread โ€” the thread query reported zero unresolved while this was outstanding, which is the "one source when there are two" trap. A state updater must be pure; React may call it more than once. Both header toggles wrote localStorage inside setState. Persistence now lives in one effect per key, and the toggles just compute the next value. Fixing it also removed a duplicated rule: two places were writing codev-pinned-only (the toggle and the last-pin-removed reset), so they could drift about what is stored. The reset now only sets state and the effect follows it. togglePinnedCollapsed had the same impurity from PR #136. Fixed alongside rather than left as the odd one out โ€” three lines, same function family, and a half-fixed pair is its own smell. tsc clean, 66 tests pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/switcher-ui.tsx | 46 +++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index d84bd62..54fa422 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -666,24 +666,35 @@ function SwitcherApp() { .catch(() => {}); } }; + // Persist the two header toggles from effects, never from inside a state + // updater: an updater must be pure (React is free to call it more than once), + // and one effect owning each key means no second writer can disagree with it + // about what is stored โ€” the reset below just sets state and this follows. + useEffect(() => { + try { + localStorage.setItem( + 'codev-pinned-collapsed', + pinnedCollapsed ? '1' : '0', + ); + } catch {} + }, [pinnedCollapsed]); + useEffect(() => { + try { + localStorage.setItem('codev-pinned-only', pinnedOnly ? '1' : '0'); + } catch {} + }, [pinnedOnly]); const togglePinnedCollapsed = () => { - setPinnedCollapsed((prev) => { - const next = !prev; - try { - localStorage.setItem('codev-pinned-collapsed', next ? '1' : '0'); - } catch {} - return next; - }); + setPinnedCollapsed((prev) => !prev); // The list just changed length โ€” snap the selection back to the top, // same as the minors fold collapse does. setSelectedSessionIndex(0); }; // Removing the last pin drops out of pinned-only implicitly (the scope needs - // pins to mean anything), so clear the STORED preference too โ€” otherwise the - // next pin, possibly weeks later, silently collapses the list to that one - // row. Keyed off a real non-empty โ†’ empty transition, because at mount the - // marks are empty until the IPC load lands and a plain `!hasPins` check - // would wipe a legitimately stored preference. + // pins to mean anything), so clear the preference too โ€” otherwise the next + // pin, possibly weeks later, silently collapses the list to that one row. + // Keyed off a real non-empty โ†’ empty transition, because at mount the marks + // are empty until the IPC load lands and a plain `!hasPins` check would wipe + // a legitimately stored preference. const hadPinsRef = useRef(false); useEffect(() => { if (hasPins) { @@ -692,18 +703,9 @@ function SwitcherApp() { } if (!hadPinsRef.current || !pinnedOnly) return; setPinnedOnly(false); - try { - localStorage.setItem('codev-pinned-only', '0'); - } catch {} }, [hasPins, pinnedOnly]); const togglePinnedOnly = () => { - setPinnedOnly((prev) => { - const next = !prev; - try { - localStorage.setItem('codev-pinned-only', next ? '1' : '0'); - } catch {} - return next; - }); + setPinnedOnly((prev) => !prev); setSelectedSessionIndex(0); }; From 5b21a58a06a391e07981a36272b096b305accde6 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 03:36:43 +0800 Subject: [PATCH 05/13] fix(sessions): PR #137 review round 2 (CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings: three inline threads plus two more that live only in the review body and never became threads. - A pre-existing bug, one word (thread, major). The VS Code closed-scan refresh re-filtered with filterSessionsLocally on a list that already held deep-search hits โ€” those matched on middle prompts, which the local haystack does not contain, so every prompt-only result vanished the moment a scan landed mid-search. From PR #132; this PR never touched the line. - Search results now recompute when late data arrives (thread, major). `sessions` is a materialized filter, so the by-id pin fetch and the title/branch/PR/reply enrichment were invisible to a query already typed. An effect reapplies the live query, returning the previous array when the row sequence is unchanged so the polled dependencies cannot re-render the list every tick. - Stale pinned-only survived a restart (review body, major). The old guard watched for a non-empty -> empty transition, which only covers losing the last pin while running; if the pins were already gone at launch the preference stayed on and the next pin silently collapsed the list. Gating on "the marks actually loaded" covers both and removes the ref the old version needed. - Empty catch blocks now say why they are empty (thread, minor). - extraPinnedSessions and its ref use ListViewSession instead of any[] (review body, trivial), so the contracts they feed can actually reject malformed rows. Not changed: the Prettier thread. The remaining report on that file is the pre-existing whole-file indentation drift, unchanged since round 1 โ€” my own changed lines are clean. tsc clean, 66 tests pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/switcher-ui.tsx | 95 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 75 insertions(+), 20 deletions(-) diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index 54fa422..199a2e8 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -5,7 +5,11 @@ import Highlighter from 'react-highlight-words'; import Select, { components, OptionProps } from 'react-select'; import { HoverButton } from './HoverButton'; import PopupDefaultExample from './popup'; -import { buildSessionListView, mergeSessionsById } from './session-list-view'; +import { + buildSessionListView, + ListViewSession, + mergeSessionsById, +} from './session-list-view'; import TerminalTab from './terminal-tab'; type SwitcherMode = 'projects' | 'sessions' | 'terminal'; @@ -433,6 +437,9 @@ function SwitcherApp() { pins: Record; hidden: string[]; }>({ pins: {}, hidden: [] }); + // False until the first getSessionMarks() response lands. Before that an + // empty pin set means "not known yet", not "no pins". + const [marksLoaded, setMarksLoaded] = useState(false); const [pinnedCollapsed, setPinnedCollapsed] = useState(() => { try { return localStorage.getItem('codev-pinned-collapsed') === '1'; @@ -451,12 +458,14 @@ function SwitcherApp() { } }); // Pinned sessions living outside the loaded list (fetched by id) - const [extraPinnedSessions, setExtraPinnedSessions] = useState([]); + const [extraPinnedSessions, setExtraPinnedSessions] = useState< + ListViewSession[] + >([]); // Mirrored into a ref: applySearchFilter runs from a debounced timeout and // from setState updaters, where reading React state gives the value captured // when the callback was created (the stale-closure trap this file has been // bitten by twice โ€” see sessionSearchRef2). - const extraPinnedSessionsRef = useRef([]); + const extraPinnedSessionsRef = useRef([]); const extraPinnedKeyRef = useRef(''); // Keep the selection on the same session after pin/hide reshuffles the list const reanchorSelectionRef = useRef(null); @@ -579,6 +588,38 @@ function SwitcherApp() { }, 180); }; + // `sessions` is a materialized filter result, so anything that arrives after + // the query was typed is invisible to it: the by-id pin fetch, and the + // title / branch / PR / last-reply enrichment those rows are matched on. + // Without this, a row that matches only on a late-arriving field stays + // missing until the next keystroke โ€” and in pinned-only mode that reads as + // "no pinned session matches" for a pin you can see while browsing. + useEffect(() => { + const search = sessionSearchRef2.current; + if (!search.trim()) return; + setSessions((prev: any[]) => { + const next = applySearchFilter(allSessionsRef.current, search); + // Identical rows in identical order โ†’ keep the old array. Some of the + // deps below are refreshed by polling, and a fresh array every tick + // would re-render the list for nothing. + if ( + next.length === prev.length && + next.every((s: any, i: number) => s.sessionId === prev[i].sessionId) + ) { + return prev; + } + return next; + }); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ + customTitles, + branches, + prLinks, + assistantResponses, + terminalApps, + extraPinnedSessions, + ]); + const isSearchingSessions = sessionSearchValue.trim().length > 0; const hiddenSet = new Set(sessionMarks.hidden); const hasPins = Object.keys(sessionMarks.pins).length > 0; @@ -676,12 +717,18 @@ function SwitcherApp() { 'codev-pinned-collapsed', pinnedCollapsed ? '1' : '0', ); - } catch {} + } catch { + // Best effort: a browsing preference is not worth failing a render over + // (localStorage throws when the quota is full or storage is blocked). + // The in-memory state stays correct; only the next launch forgets it. + } }, [pinnedCollapsed]); useEffect(() => { try { localStorage.setItem('codev-pinned-only', pinnedOnly ? '1' : '0'); - } catch {} + } catch { + // Best effort, same as above. + } }, [pinnedOnly]); const togglePinnedCollapsed = () => { setPinnedCollapsed((prev) => !prev); @@ -689,21 +736,21 @@ function SwitcherApp() { // same as the minors fold collapse does. setSelectedSessionIndex(0); }; - // Removing the last pin drops out of pinned-only implicitly (the scope needs - // pins to mean anything), so clear the preference too โ€” otherwise the next - // pin, possibly weeks later, silently collapses the list to that one row. - // Keyed off a real non-empty โ†’ empty transition, because at mount the marks - // are empty until the IPC load lands and a plain `!hasPins` check would wipe - // a legitimately stored preference. - const hadPinsRef = useRef(false); + // Pinned-only needs pins to mean anything, so an empty pin set must clear the + // stored preference โ€” otherwise the next pin, possibly weeks later, silently + // collapses the list to that one row. + // + // Gated on the marks having actually loaded, not on having seen pins earlier + // in this run: at mount the pin set is empty simply because the IPC load has + // not landed, and an ungated `!hasPins` check would wipe a legitimately + // stored preference. Watching for a non-empty โ†’ empty transition instead + // would only cover the case where the last pin is removed while the app is + // running, and miss the one where it is already gone at launch (a reset or + // hand-edited marks file) โ€” which is the same footgun, one restart later. useEffect(() => { - if (hasPins) { - hadPinsRef.current = true; - return; - } - if (!hadPinsRef.current || !pinnedOnly) return; + if (!marksLoaded || hasPins || !pinnedOnly) return; setPinnedOnly(false); - }, [hasPins, pinnedOnly]); + }, [marksLoaded, hasPins, pinnedOnly]); const togglePinnedOnly = () => { setPinnedOnly((prev) => !prev); setSelectedSessionIndex(0); @@ -741,7 +788,10 @@ function SwitcherApp() { .then((m: any) => { if (m) setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); }) - .catch(() => {}); + .catch(() => {}) + // Marked loaded either way: a failed read is still a completed attempt, + // and leaving the flag false would disable the pinned-only reset forever. + .finally(() => setMarksLoaded(true)); const unsubscribe = window.electronAPI.onSessionMarksUpdated( (_event: any, m: any) => { if (m) { @@ -1095,7 +1145,12 @@ function SwitcherApp() { setSessions((prev: any[]) => { const updated = updateSessions(prev); const search = sessionSearchRef2.current; - return search.trim() ? filterSessionsLocally(updated, search) : updated; + // applySearchFilter, not filterSessionsLocally: the list being + // re-filtered here already contains deep-search hits, which + // matched on middle prompts that the local filter's haystack + // does not contain โ€” running the local filter alone drops every + // prompt-only match the moment a closed-VS-Code scan lands. + return search.trim() ? applySearchFilter(updated, search) : updated; }); }); }, 300); From 6fc723ebd7cf5492a627c22674df51297f69cb12 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 04:03:05 +0800 Subject: [PATCH 06/13] fix(sessions): a failed marks read is unknown, not empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubic P2 on 5b21a58, and it is right: the reasoning in that commit was backwards. marksLoaded was set from .finally(), so a rejected getSessionMarks() made the empty pin set look authoritative and the pinned-only reset cleared a valid stored preference โ€” the exact damage the reset was added to prevent, now triggered by a transient IPC or filesystem failure instead of by a stale preference. "A failed read is still a completed attempt" was true and irrelevant: the guard needs to know the pin set is EMPTY, and a failure only tells you it is UNKNOWN. Not clearing is the safe direction, so the flag is now set only by a real response โ€” and also by an fs.watch push, which is a real read of the store, so a store that becomes readable later still promotes the flag. tsc clean, 66 tests pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/switcher-ui.tsx | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index 199a2e8..9b8595b 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -786,17 +786,25 @@ function SwitcherApp() { window.electronAPI .getSessionMarks() .then((m: any) => { - if (m) setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); + if (!m) return; + setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); + // Only a real response makes the pin set authoritative. A rejection โ€” + // or a nullish payload โ€” leaves it UNKNOWN, not empty, and the + // pinned-only reset must never act on unknown: it would wipe a valid + // stored preference on a transient IPC or filesystem failure, which is + // the exact damage that reset exists to prevent. Not clearing is the + // safe direction; the watcher below promotes the flag if the store + // becomes readable later. + setMarksLoaded(true); }) - .catch(() => {}) - // Marked loaded either way: a failed read is still a completed attempt, - // and leaving the flag false would disable the pinned-only reset forever. - .finally(() => setMarksLoaded(true)); + .catch(() => {}); const unsubscribe = window.electronAPI.onSessionMarksUpdated( (_event: any, m: any) => { if (m) { suppressHoverSelection(); setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); + // A push from the main-side fs.watch is a real read of the store. + setMarksLoaded(true); } }, ); From a5e2ff00d4de1125552140efa5aa4e8ea6ea543d Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 04:16:11 +0800 Subject: [PATCH 07/13] fix(sessions): PR #137 review round 4 (cubic + CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A failed marks read is now distinguishable from an empty store (cubic P2). Last round fixed the renderer half; the main side still swallowed every error into emptyMarks(), so the IPC resolved successfully with an empty pin set and the renderer could not tell "no pins" from "could not read". readMarksFileResult now reports `known`, treating a MISSING file as authoritative (first run really has no marks) and anything else โ€” permissions, IO, malformed JSON โ€” as unknown. The get handler ships the flag; the renderer only promotes marksLoaded on an authoritative read. The watcher stops broadcasting unknown reads entirely rather than annotating them: announcing "the marks are now empty" because the file would not parse pushes every listener into acting on state that is still intact on disk. - No stale filter closure can overwrite fresher results (CodeRabbit major). The deep-search callback filtered with an applySearchFilter captured ~180ms plus one IPC round-trip earlier, closing over that render's enrichment maps; if enrichment landed in between, its write replaced a correct list with a stale one and nothing recomputed. It now bumps a revision and the refresh effect โ€” the single filtering site, running from a current render โ€” owns it. - Removed an eslint-disable for react-hooks/exhaustive-deps, a rule this repo does not configure, so it was itself an ESLint error. - Typed the list pipeline (sessions, allSessions and their refs, deepMatchesRef) as ListViewSession[]. Tests: 68 pass. The two new guards are mutation-verified โ€” dropping the ENOENT distinction fails 2, broadcasting unknown reads fails 1, restoring both goes green. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/electron-api.d.ts | 2 ++ src/main.ts | 6 +++- src/session-marks.test.ts | 27 ++++++++++++++++- src/session-marks.ts | 42 ++++++++++++++++++++++---- src/session-marks.watch.test.ts | 53 ++++++++++++++++++++++++++++++++- src/switcher-ui.tsx | 40 ++++++++++++++++--------- 6 files changed, 148 insertions(+), 22 deletions(-) diff --git a/src/electron-api.d.ts b/src/electron-api.d.ts index d850788..61348e9 100644 --- a/src/electron-api.d.ts +++ b/src/electron-api.d.ts @@ -136,6 +136,8 @@ interface IElectronAPI { { pinnedAt: string; cwd: string; accountLabel?: string; group: string | null } >; hidden: string[]; + /** False when the store exists but could not be read โ€” the pin set is unknown, not empty. */ + known: boolean; }>; pinSession: ( sessionId: string, diff --git a/src/main.ts b/src/main.ts index f1b19cd..2512567 100644 --- a/src/main.ts +++ b/src/main.ts @@ -34,6 +34,7 @@ import { } from './claude-session-utility'; import { readSessionMarks, + readSessionMarksResult, watchSessionMarks, withHidden, withoutHidden, @@ -2384,7 +2385,10 @@ const ensureMarksWatcher = () => { ipcMain.handle('get-session-marks', () => { ensureMarksWatcher(); - return readSessionMarks(); + // `known` travels with the marks: an unreadable store yields empty marks + // that must not be mistaken for "this user has no pins" (see MarksRead). + const read = readSessionMarksResult(); + return { ...read.marks, known: read.known }; }); ipcMain.handle('pin-session', (_event, sessionId: string, info: { cwd?: string; accountLabel?: string }) => { diff --git a/src/session-marks.test.ts b/src/session-marks.test.ts index 4c6a388..66eb4bb 100644 --- a/src/session-marks.test.ts +++ b/src/session-marks.test.ts @@ -7,6 +7,7 @@ import { emptyMarks, normalizeMarks, readMarksFile, + readMarksFileResult, withHidden, withoutHidden, withoutPin, @@ -115,5 +116,29 @@ describe('marks file roundtrip', () => { fs.writeFileSync(file, '{not json'); expect(readMarksFile(file)).toEqual(emptyMarks()); }); -}); + // Both cases above yield empty marks, but they do not mean the same thing: + // "no store yet" is a fact, "could not parse the store" is ignorance. A + // caller that clears user state when the pin set looks empty must be able + // to tell them apart. + it('reports a missing file as authoritative and an unreadable one as unknown', () => { + const file = path.join(dir, 'session-marks.json'); + expect(readMarksFileResult(file)).toEqual({ + marks: emptyMarks(), + known: true, + }); + + fs.writeFileSync(file, '{not json'); + expect(readMarksFileResult(file)).toEqual({ + marks: emptyMarks(), + known: false, + }); + + const marks = withPin(emptyMarks(), 'abc', { + pinnedAt: '2026-07-14T01:02:03Z', + cwd: '/repo', + }); + writeMarksFile(file, marks); + expect(readMarksFileResult(file)).toEqual({ marks, known: true }); + }); +}); diff --git a/src/session-marks.ts b/src/session-marks.ts index 2d21704..734c1fd 100644 --- a/src/session-marks.ts +++ b/src/session-marks.ts @@ -128,14 +128,37 @@ export const withoutHidden = ( const MARKS_FILENAME = 'session-marks.json'; -export const readMarksFile = (filePath: string): SessionMarks => { +/** + * A read plus whether its result is authoritative. + * + * `known: false` means the pin set is UNKNOWN, not empty. Callers that act on + * emptiness โ€” clearing a stored browse preference, broadcasting a change โ€” + * must not act on an unknown read, or a transient filesystem failure destroys + * user state that is still perfectly intact on disk. + */ +export interface MarksRead { + marks: SessionMarks; + known: boolean; +} + +export const readMarksFileResult = (filePath: string): MarksRead => { try { - return normalizeMarks(JSON.parse(fs.readFileSync(filePath, 'utf-8'))); - } catch { - return emptyMarks(); + return { + marks: normalizeMarks(JSON.parse(fs.readFileSync(filePath, 'utf-8'))), + known: true, + }; + } catch (err) { + // A missing file IS authoritative: no store yet means no marks yet, which + // is simply the first run. Anything else โ€” permissions, IO, malformed + // JSON โ€” leaves the real contents unknown. + const code = (err as NodeJS.ErrnoException | undefined)?.code; + return { marks: emptyMarks(), known: code === 'ENOENT' }; } }; +export const readMarksFile = (filePath: string): SessionMarks => + readMarksFileResult(filePath).marks; + export const writeMarksFile = (filePath: string, marks: SessionMarks): void => { fs.mkdirSync(path.dirname(filePath), { recursive: true }); // temp + rename so a crash mid-write can't corrupt the store @@ -150,6 +173,9 @@ const defaultMarksPath = (): string => export const readSessionMarks = (): SessionMarks => readMarksFile(defaultMarksPath()); +export const readSessionMarksResult = (): MarksRead => + readMarksFileResult(defaultMarksPath()); + export const writeSessionMarks = (marks: SessionMarks): void => writeMarksFile(defaultMarksPath(), marks); @@ -174,7 +200,13 @@ export const watchMarksFile = ( if (changed && changed !== filename) return; if (debounceTimer) clearTimeout(debounceTimer); debounceTimer = setTimeout(() => { - onChange(readMarksFile(filePath)); + const read = readMarksFileResult(filePath); + // Never broadcast an unknown read. Announcing "the marks are now empty" + // because the file could not be parsed would push every listener into + // acting on state that is still intact on disk; staying silent leaves + // them on the last thing actually seen. + if (!read.known) return; + onChange(read.marks); }, 50); }); diff --git a/src/session-marks.watch.test.ts b/src/session-marks.watch.test.ts index ec8708f..2ef8ae5 100644 --- a/src/session-marks.watch.test.ts +++ b/src/session-marks.watch.test.ts @@ -1,4 +1,7 @@ -import { describe, expect, it, vi } from 'vitest'; +import * as realFs from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; // Mock fs BEFORE the module under test imports it: fs.watch is replaced by a // controllable fake so the watcher 'error' path can be driven deterministically. @@ -20,6 +23,7 @@ vi.mock('fs', async (importOriginal) => { }; }); +import * as fs from 'fs'; import { watchMarksFile } from './session-marks'; describe('watchMarksFile error recovery', () => { @@ -37,3 +41,50 @@ describe('watchMarksFile error recovery', () => { expect(onChange).not.toHaveBeenCalled(); }); }); + +describe('watchMarksFile broadcast', () => { + let dir: string; + + afterEach(() => { + vi.useRealTimers(); + if (dir) realFs.rmSync(dir, { recursive: true, force: true }); + }); + + // Fire the fs.watch callback the module registered, then run the debounce. + const fireChange = (file: string) => { + const calls = (fs.watch as unknown as { mock: { calls: any[][] } }).mock + .calls; + const listener = calls[calls.length - 1][2]; + listener('change', path.basename(file)); + vi.advanceTimersByTime(60); + }; + + it('broadcasts a readable store and stays silent on an unreadable one', () => { + vi.useFakeTimers(); + dir = realFs.mkdtempSync(path.join(os.tmpdir(), 'codev-marks-watch-')); + const file = path.join(dir, 'session-marks.json'); + const onChange = vi.fn(); + + realFs.writeFileSync( + file, + JSON.stringify({ version: 1, pins: {}, hidden: ['a'] }), + ); + watchMarksFile(file, onChange); + fireChange(file); + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange.mock.calls[0][0].hidden).toEqual(['a']); + + // An unparseable store means the marks are UNKNOWN, not empty. Announcing + // empty here would push every listener into acting on state that is still + // intact on disk โ€” the pinned-only reset would wipe a valid preference. + realFs.writeFileSync(file, '{not json'); + fireChange(file); + expect(onChange).toHaveBeenCalledTimes(1); + + // A deletion IS authoritative: the store really is gone. + realFs.rmSync(file); + fireChange(file); + expect(onChange).toHaveBeenCalledTimes(2); + expect(onChange.mock.calls[1][0].hidden).toEqual([]); + }); +}); diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index 9b8595b..c5b2771 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -417,8 +417,8 @@ function SwitcherApp() { ); const [projectBranches, setProjectBranches] = useState>({}); const [activeIDEFolders, setActiveIDEFolders] = useState>(new Set()); - const [allSessions, setAllSessions] = useState([]); - const [sessions, setSessions] = useState([]); + const [allSessions, setAllSessions] = useState([]); + const [sessions, setSessions] = useState([]); const [selectedSessionIndex, setSelectedSessionIndex] = useState(-1); const [sessionDisplayMode, setSessionDisplayMode] = useState('first'); const [customTitles, setCustomTitles] = useState>({}); @@ -472,12 +472,15 @@ function SwitcherApp() { const hoverSuppressTokenRef = useRef(0); const modeRef = useRef(initialMode); const activeStateRef = useRef>({}); - const allSessionsRef = useRef([]); + const allSessionsRef = useRef([]); const lastAssistantFetchRef = useRef>({}); const sessionSearchRef2 = useRef(''); // tracks current search value for use in closures const deepSearchTimerRef = useRef | null>(null); const deepSearchSeqRef = useRef(0); - const deepMatchesRef = useRef([]); // latest main-side full-prompt matches + // Bumped when a deep-search response lands, so the single filtering site + // (the refresh effect) reruns with current state instead of a stale closure. + const [deepSearchRev, setDeepSearchRev] = useState(0); + const deepMatchesRef = useRef([]); // latest main-side full-prompt matches // Set true when a session is opened; on the next window show, clear the search so // returning to Sessions shows the full list. Toggling away without selecting keeps it. const clearSessionSearchOnShowRef = useRef(false); @@ -558,7 +561,12 @@ function SwitcherApp() { if (seq !== deepSearchSeqRef.current || sessionSearchRef2.current !== query) return; deepMatchesRef.current = res?.sessions || []; setSearchSnippets(res?.snippets || {}); - setSessions(applySearchFilter(allSessionsRef.current, query)); + // Bump a revision instead of filtering here. This callback was created + // ~180ms + one IPC round-trip ago and closes over the enrichment maps of + // THAT render, so filtering now would overwrite fresher results with a + // stale view. The refresh effect below owns the filtering; it runs from + // a current render, and this is one of its dependencies. + setDeepSearchRev((r) => r + 1); // Lazy-enrich deep matches that aren't in the loaded list. Bounded by // the deep-search result cap (100), same magnitude as the initial load. const loaded = new Set( @@ -610,8 +618,8 @@ function SwitcherApp() { } return next; }); - // eslint-disable-next-line react-hooks/exhaustive-deps }, [ + deepSearchRev, customTitles, branches, prLinks, @@ -788,13 +796,15 @@ function SwitcherApp() { .then((m: any) => { if (!m) return; setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); - // Only a real response makes the pin set authoritative. A rejection โ€” - // or a nullish payload โ€” leaves it UNKNOWN, not empty, and the - // pinned-only reset must never act on unknown: it would wipe a valid - // stored preference on a transient IPC or filesystem failure, which is - // the exact damage that reset exists to prevent. Not clearing is the - // safe direction; the watcher below promotes the flag if the store - // becomes readable later. + // Only an AUTHORITATIVE read makes the pin set trustworthy. A + // rejection, a nullish payload, or `known: false` (the store exists + // but could not be parsed โ€” main returns empty marks either way) all + // leave it UNKNOWN rather than empty, and the pinned-only reset must + // never act on unknown: it would wipe a valid stored preference on a + // transient filesystem failure, which is the exact damage that reset + // exists to prevent. Not clearing is the safe direction; the watcher + // below promotes the flag if the store becomes readable later. + if (m.known === false) return; setMarksLoaded(true); }) .catch(() => {}); @@ -803,7 +813,9 @@ function SwitcherApp() { if (m) { suppressHoverSelection(); setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); - // A push from the main-side fs.watch is a real read of the store. + // A push is a real read: the main-side watcher drops unknown reads + // rather than broadcasting them, so arriving here means the store + // was parsed successfully. setMarksLoaded(true); } }, From 500d823df2ef13453115346bd06d244eb286cfde Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 04:17:37 +0800 Subject: [PATCH 08/13] docs: record the review-loop facts this PR paid for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things cost real time on PR #137 and are not obvious from the tooling: - A green "CodeRabbit pass" prints identically whether it reviewed or was rate limited; only the status description says which. cubic instead embeds the reviewed SHA in its review body. - Findings land in three places and only threads have an unresolved count. CodeRabbit's Nitpick and Outside-diff blocks live in the review body, so reviewThreads reported zero unresolved twice while a real finding โ€” once a Major โ€” was still outstanding. - Requiring a bot review before merging is a judgment, not a rule anything here states. Records the default (wait for both) and the escape hatch when the rate limit stops making the wait informative. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- CLAUDE.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index da12fe9..95dccbb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,6 +13,18 @@ - After 3 back-to-back reviews, limited to 4 reviews per hour - Each push triggers an incremental review โ€” rapid pushes (4+ within an hour) will queue or delay reviews - **Practical rule:** wait for all reviewers to finish, address all comments, then push fixes together (one or more commits is fine, but aim for a single push). This conserves review quota and avoids triggering redundant review cycles. +- **A green `CodeRabbit pass` does not mean it reviewed anything.** `gh pr checks` prints the same green tick whether CodeRabbit reviewed and found nothing or never ran. The distinction is only in the status description: + + ```bash + gh api repos/grimmerk/codev/commits//statuses \ + --jq '.[] | select(.context=="CodeRabbit") | "\(.state) โ€” \(.description)"' + # "success โ€” Review completed" โ† actually reviewed + # "success โ€” Review rate limited" โ† never ran; the tick is meaningless + ``` + + cubic states it differently: its review body embeds the commit SHA it reviewed (``), so that is the thing to check rather than the tick. +- **Findings arrive in three places, and only one of them has an unresolved count.** Inline review comments become threads (`reviewThreads`, resolvable); plain comments are issue comments; and CodeRabbit puts `๐Ÿงน Nitpick` and `โš ๏ธ Outside diff range` blocks in the **review body** (`gh api repos/.../pulls//reviews --jq '.[].body'`), which is neither. Measured on PR #137: `reviewThreads` reported zero unresolved twice while a real finding โ€” once a Major โ€” sat in a review body. A review-body finding has no thread, so answer it with `gh pr comment` quoting it. +- **Whether a bot must review before merging is a judgment, not a rule.** Nothing here requires it, and CodeRabbit's free-plan rate limit can withhold a review indefinitely (it was limited on 4 of 6 heads in one evening on PR #137, largely self-inflicted by pushing four times). Default: wait for both. Escape hatch, when the wait stops being informative โ€” if two attempts on the *current* head both come back rate-limited (the automatic run from the push, then one `@coderabbitai review` posted at least ~30 minutes after the rate-limit timestamp), treat CodeRabbit as unavailable and judge on cubic plus CI. Say so explicitly rather than reporting that both reviewers passed. ## Build Commands From 66fd6fada9dc289b0f75ecc4185a965319bcbe00 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 04:51:04 +0800 Subject: [PATCH 09/13] fix(sessions): never overwrite a marks store we could not read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubic P1 + P2 on 500d823, and the P1 is real data loss that predates this PR โ€” the `known` flag added last round is only what made it detectable. Every marks mutation is read-modify-write over the whole file, and readSessionMarks() degrades an unreadable store to empty marks. So a single Cmd+D against a corrupt or unreadable session-marks.json wrote "empty plus this one change" over it, erasing every other pin and hidden id on disk. Present since PR #136. Fixed by collapsing all four read-modify-write handlers onto one mutateMarksFile(), which refuses to write when the read was not authoritative and returns known:false so the IPC can report it. A missing file still writes: ENOENT is authoritative, so the first-ever pin creates the store as before. Four copies of read-modify-write were four chances to forget the guard, which is why the callers collapse rather than each growing a check. Also (P2): the renderer applied the empty payload to state before checking `known`, so a transiently unreadable store hid valid pins from the UI. The guard now runs first. Tests: 71 pass, +3 for mutateMarksFile โ€” applies and persists on a readable store, still creates the store on the first-ever mutation, and on an unreadable one leaves the file BYTE-IDENTICAL rather than merely skipping the change. Mutation-verified: removing the refusal turns that last test red. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/main.ts | 56 +++++++++++++++++++++++++++++---------- src/session-marks.test.ts | 45 +++++++++++++++++++++++++++++++ src/session-marks.ts | 30 +++++++++++++++++++++ src/switcher-ui.tsx | 5 +++- 4 files changed, 121 insertions(+), 15 deletions(-) diff --git a/src/main.ts b/src/main.ts index 2512567..2a7088e 100644 --- a/src/main.ts +++ b/src/main.ts @@ -33,14 +33,13 @@ import { getSessionsByIds, } from './claude-session-utility'; import { - readSessionMarks, + mutateSessionMarks, readSessionMarksResult, watchSessionMarks, withHidden, withoutHidden, withoutPin, withPin, - writeSessionMarks, } from './session-marks'; import { installHooks, @@ -2395,12 +2394,23 @@ ipcMain.handle('pin-session', (_event, sessionId: string, info: { cwd?: string; if (!sessionId || typeof sessionId !== 'string') { return { ok: false, error: 'invalid sessionId' }; } - const marks = withPin(readSessionMarks(), sessionId, { - pinnedAt: new Date().toISOString(), - cwd: typeof info?.cwd === 'string' ? info.cwd : '', - accountLabel: typeof info?.accountLabel === 'string' ? info.accountLabel : undefined, - }); - writeSessionMarks(marks); + const res = mutateSessionMarks((prev) => + withPin(prev, sessionId, { + pinnedAt: new Date().toISOString(), + cwd: typeof info?.cwd === 'string' ? info.cwd : '', + accountLabel: + typeof info?.accountLabel === 'string' + ? info.accountLabel + : undefined, + }), + ); + if (!res.known) { + // The store exists but could not be read. Writing now would overwrite it + // with an empty set plus this one change, erasing every other mark. + console.warn('[session-marks] pin refused: store unreadable', sessionId); + return { ok: false, error: 'marks store unreadable' }; + } + const marks = res.marks; // Audit trail for the lost-pin reports (visible in Console.app / stdout) console.log('[session-marks] pin', sessionId, 'pins:', Object.keys(marks.pins).length); return { ok: true, marks }; @@ -2410,8 +2420,14 @@ ipcMain.handle('unpin-session', (_event, sessionId: string) => { if (!sessionId || typeof sessionId !== 'string') { return { ok: false, error: 'invalid sessionId' }; } - const marks = withoutPin(readSessionMarks(), sessionId); - writeSessionMarks(marks); + const res = mutateSessionMarks((prev) => withoutPin(prev, sessionId)); + if (!res.known) { + // The store exists but could not be read. Writing now would overwrite it + // with an empty set plus this one change, erasing every other mark. + console.warn('[session-marks] unpin refused: store unreadable', sessionId); + return { ok: false, error: 'marks store unreadable' }; + } + const marks = res.marks; console.log('[session-marks] unpin', sessionId, 'pins:', Object.keys(marks.pins).length); return { ok: true, marks }; }); @@ -2420,8 +2436,14 @@ ipcMain.handle('hide-session', (_event, sessionId: string) => { if (!sessionId || typeof sessionId !== 'string') { return { ok: false, error: 'invalid sessionId' }; } - const marks = withHidden(readSessionMarks(), sessionId); - writeSessionMarks(marks); + const res = mutateSessionMarks((prev) => withHidden(prev, sessionId)); + if (!res.known) { + // The store exists but could not be read. Writing now would overwrite it + // with an empty set plus this one change, erasing every other mark. + console.warn('[session-marks] hide refused: store unreadable', sessionId); + return { ok: false, error: 'marks store unreadable' }; + } + const marks = res.marks; console.log('[session-marks] hide', sessionId, 'pins:', Object.keys(marks.pins).length, 'hidden:', marks.hidden.length); return { ok: true, marks }; }); @@ -2430,8 +2452,14 @@ ipcMain.handle('unhide-session', (_event, sessionId: string) => { if (!sessionId || typeof sessionId !== 'string') { return { ok: false, error: 'invalid sessionId' }; } - const marks = withoutHidden(readSessionMarks(), sessionId); - writeSessionMarks(marks); + const res = mutateSessionMarks((prev) => withoutHidden(prev, sessionId)); + if (!res.known) { + // The store exists but could not be read. Writing now would overwrite it + // with an empty set plus this one change, erasing every other mark. + console.warn('[session-marks] unhide refused: store unreadable', sessionId); + return { ok: false, error: 'marks store unreadable' }; + } + const marks = res.marks; console.log('[session-marks] unhide', sessionId, 'hidden:', marks.hidden.length); return { ok: true, marks }; }); diff --git a/src/session-marks.test.ts b/src/session-marks.test.ts index 66eb4bb..3ae067c 100644 --- a/src/session-marks.test.ts +++ b/src/session-marks.test.ts @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { emptyMarks, normalizeMarks, + mutateMarksFile, readMarksFile, readMarksFileResult, withHidden, @@ -141,4 +142,48 @@ describe('marks file roundtrip', () => { writeMarksFile(file, marks); expect(readMarksFileResult(file)).toEqual({ marks, known: true }); }); + + // Every mutation is read-modify-write over the whole file, so a read that + // degrades to empty marks turns the next pin into a full overwrite. + describe('mutateMarksFile', () => { + it('applies and persists the mutation on a readable store', () => { + const file = path.join(dir, 'session-marks.json'); + writeMarksFile(file, withHidden(emptyMarks(), 'junk-1')); + + const res = mutateMarksFile(file, (prev) => + withPin(prev, 'abc', { + pinnedAt: '2026-07-14T01:02:03Z', + cwd: '/repo', + }), + ); + + expect(res.known).toBe(true); + expect(Object.keys(res.marks.pins)).toEqual(['abc']); + expect(readMarksFile(file).hidden).toEqual(['junk-1']); + }); + + it('creates the store on the first-ever mutation (ENOENT is authoritative)', () => { + const file = path.join(dir, 'nested', 'session-marks.json'); + const res = mutateMarksFile(file, (prev) => withHidden(prev, 'junk-1')); + expect(res.known).toBe(true); + expect(readMarksFile(file).hidden).toEqual(['junk-1']); + }); + + it('refuses to write when the store is unreadable, leaving it byte-identical', () => { + const file = path.join(dir, 'session-marks.json'); + const corrupt = "{not json โ€” but somebody's real pins are in here"; + fs.writeFileSync(file, corrupt); + + const res = mutateMarksFile(file, (prev) => + withPin(prev, 'abc', { + pinnedAt: '2026-07-14T01:02:03Z', + cwd: '/repo', + }), + ); + + expect(res.known).toBe(false); + // Not "the mutation was skipped" โ€” nothing was written at all. + expect(fs.readFileSync(file, 'utf-8')).toBe(corrupt); + }); + }); }); diff --git a/src/session-marks.ts b/src/session-marks.ts index 734c1fd..17ab7e2 100644 --- a/src/session-marks.ts +++ b/src/session-marks.ts @@ -176,6 +176,36 @@ export const readSessionMarks = (): SessionMarks => export const readSessionMarksResult = (): MarksRead => readMarksFileResult(defaultMarksPath()); +/** + * Read-modify-write that REFUSES to write when the store could not be read. + * + * Every mutation here is read-modify-write over the whole file, so a read that + * silently degrades to empty marks turns the next pin or hide into a full + * overwrite: one keystroke against an unreadable store would erase every other + * pin and hidden id on disk. A missing file is still fine โ€” ENOENT is + * authoritative (see MarksRead), so the first-ever pin creates the store as + * usual. + * + * Returns the resulting marks with `known: true` when the write happened, or + * the unknown read (`known: false`) when it was refused and nothing was + * touched. Collapsing the four callers onto this one path is deliberate: four + * copies of read-modify-write are four chances to forget the guard. + */ +export const mutateMarksFile = ( + filePath: string, + mutate: (marks: SessionMarks) => SessionMarks, +): MarksRead => { + const read = readMarksFileResult(filePath); + if (!read.known) return read; + const next = mutate(read.marks); + writeMarksFile(filePath, next); + return { marks: next, known: true }; +}; + +export const mutateSessionMarks = ( + mutate: (marks: SessionMarks) => SessionMarks, +): MarksRead => mutateMarksFile(defaultMarksPath(), mutate); + export const writeSessionMarks = (marks: SessionMarks): void => writeMarksFile(defaultMarksPath(), marks); diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index c5b2771..6afa760 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -795,6 +795,10 @@ function SwitcherApp() { .getSessionMarks() .then((m: any) => { if (!m) return; + // Guard BEFORE touching state: an unknown read carries empty marks, + // and applying them would hide valid pins from the UI until something + // else re-reads the store. + if (m.known === false) return; setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); // Only an AUTHORITATIVE read makes the pin set trustworthy. A // rejection, a nullish payload, or `known: false` (the store exists @@ -804,7 +808,6 @@ function SwitcherApp() { // transient filesystem failure, which is the exact damage that reset // exists to prevent. Not clearing is the safe direction; the watcher // below promotes the flag if the store becomes readable later. - if (m.known === false) return; setMarksLoaded(true); }) .catch(() => {}); From ec98789b8ad1a3e0df25300123f0f2f105431092 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 06:30:27 +0800 Subject: [PATCH 10/13] fix(sessions): a store that parses is not yet a store we understand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit round on 66fd6fa. Two of five findings were real, and the first is the same data-loss class as cubic's P1, one level deeper. - Schema-invalid stores were treated as authoritative (major). normalizeMarks is deliberately forgiving so a partly-corrupt store still renders, but `known: true` was set after any successful JSON.parse โ€” so `[]`, a `pins: []`, or a version-2 file written by a future build would parse, normalize to empty, be declared authoritative, and then be OVERWRITTEN by the next pin. The version case is the worst: the data is perfectly good and only this build is too old to read it. readMarksFileResult now checks the on-disk shape before claiming authority. - An in-flight initial read could roll back a newer watcher push (major). getSessionMarks() and onSessionMarksUpdated had no ordering guard, so a slow initial response could apply an older snapshot over a newer one โ€” and an emptier snapshot then clears and persists pinnedOnly. A watcher push is newer by definition (the store changed after we asked), so it now wins. - Trailing commas on the two setSessionMarks literals, which this repo's own style guide asks for. - CLAUDE.md: the status-check snippet used ``, which bash reads as redirection, so the documented command could not be pasted. Also corrected a claim in that file this PR itself disproved โ€” cubic's review body is not a reliable coverage signal, because it does not always post one; the check-run output is. Tests: 73 pass, +2 for the schema check. Mutation-verified: bypassing it turns the schema test red. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- CLAUDE.md | 13 +++++++++++-- src/session-marks.test.ts | 36 ++++++++++++++++++++++++++++++++++++ src/session-marks.ts | 34 ++++++++++++++++++++++++++++++---- src/switcher-ui.tsx | 18 ++++++++++++++++-- 4 files changed, 93 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 95dccbb..f15118b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,13 +16,22 @@ - **A green `CodeRabbit pass` does not mean it reviewed anything.** `gh pr checks` prints the same green tick whether CodeRabbit reviewed and found nothing or never ran. The distinction is only in the status description: ```bash - gh api repos/grimmerk/codev/commits//statuses \ + SHA=$(gh pr view --json headRefOid --jq .headRefOid) + gh api repos/grimmerk/codev/commits/"$SHA"/statuses \ --jq '.[] | select(.context=="CodeRabbit") | "\(.state) โ€” \(.description)"' # "success โ€” Review completed" โ† actually reviewed # "success โ€” Review rate limited" โ† never ran; the tick is meaningless ``` - cubic states it differently: its review body embeds the commit SHA it reviewed (``), so that is the thing to check rather than the tick. + **cubic needs a different source, and its review body is not it** โ€” it does not always post one. On PR #137 it reviewed a head, found nothing, and posted no body at all, so a body-based check reported "not reviewed yet". Read the check-run output, which is always present and states the counts: + + ```bash + gh api repos/grimmerk/codev/commits/"$SHA"/check-runs \ + --jq '.check_runs[] | select(.name|test("cubic")) | "\(.conclusion) โ€” \(.output.summary)"' + # "success โ€” AI review completed with 1 review. 0 issues found across 4 files" + ``` + + When it *does* post a body, that body embeds the SHA it reviewed (``), which is useful corroboration but not a substitute. - **Findings arrive in three places, and only one of them has an unresolved count.** Inline review comments become threads (`reviewThreads`, resolvable); plain comments are issue comments; and CodeRabbit puts `๐Ÿงน Nitpick` and `โš ๏ธ Outside diff range` blocks in the **review body** (`gh api repos/.../pulls//reviews --jq '.[].body'`), which is neither. Measured on PR #137: `reviewThreads` reported zero unresolved twice while a real finding โ€” once a Major โ€” sat in a review body. A review-body finding has no thread, so answer it with `gh pr comment` quoting it. - **Whether a bot must review before merging is a judgment, not a rule.** Nothing here requires it, and CodeRabbit's free-plan rate limit can withhold a review indefinitely (it was limited on 4 of 6 heads in one evening on PR #137, largely self-inflicted by pushing four times). Default: wait for both. Escape hatch, when the wait stops being informative โ€” if two attempts on the *current* head both come back rate-limited (the automatic run from the push, then one `@coderabbitai review` posted at least ~30 minutes after the rate-limit timestamp), treat CodeRabbit as unavailable and judge on cubic plus CI. Say so explicitly rather than reporting that both reviewers passed. diff --git a/src/session-marks.test.ts b/src/session-marks.test.ts index 3ae067c..793d84a 100644 --- a/src/session-marks.test.ts +++ b/src/session-marks.test.ts @@ -6,6 +6,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { emptyMarks, normalizeMarks, + isKnownMarksShape, mutateMarksFile, readMarksFile, readMarksFileResult, @@ -143,6 +144,41 @@ describe('marks file roundtrip', () => { expect(readMarksFileResult(file)).toEqual({ marks, known: true }); }); + // normalizeMarks is deliberately forgiving so a partly-corrupt store still + // renders. That is right for display and wrong for authority: a file that + // parses but is not our schema must not be declared authoritative, or the + // next pin overwrites it. + it('treats a parseable but schema-invalid store as unknown', () => { + const file = path.join(dir, 'session-marks.json'); + const cases: [string, string][] = [ + ['[]', 'an array is not a marks object'], + [ + '{"version":2,"pins":{},"hidden":[]}', + 'a newer format this build cannot read', + ], + ['{"pins":[]}', 'pins must be an object'], + ['{"hidden":{}}', 'hidden must be an array'], + ]; + for (const [json, why] of cases) { + fs.writeFileSync(file, json); + expect(readMarksFileResult(file).known, why).toBe(false); + // and therefore untouchable + expect(mutateMarksFile(file, (p) => withHidden(p, 'x')).known).toBe( + false, + ); + expect(fs.readFileSync(file, 'utf-8')).toBe(json); + } + // A store we do understand stays writable. + writeMarksFile(file, emptyMarks()); + expect(readMarksFileResult(file).known).toBe(true); + }); + + it('accepts our own shape, including a bare object', () => { + expect(isKnownMarksShape({ version: 1, pins: {}, hidden: [] })).toBe(true); + expect(isKnownMarksShape({})).toBe(true); + expect(isKnownMarksShape(null)).toBe(false); + }); + // Every mutation is read-modify-write over the whole file, so a read that // degrades to empty marks turns the next pin into a full overwrite. describe('mutateMarksFile', () => { diff --git a/src/session-marks.ts b/src/session-marks.ts index 17ab7e2..11994c5 100644 --- a/src/session-marks.ts +++ b/src/session-marks.ts @@ -141,12 +141,38 @@ export interface MarksRead { known: boolean; } +/** The only store version this build understands. */ +const SUPPORTED_VERSION = 1; + +/** + * Does a parsed value look like a store we understand? + * + * `normalizeMarks` is deliberately forgiving โ€” it coerces anything into valid + * v1 marks so a partly-corrupt store still renders. That is right for display + * and wrong for authority: without this check a `[]`, a `pins: []`, or a + * version-2 file written by a future build would parse, normalize to empty, + * be declared authoritative, and then be OVERWRITTEN by the next pin. Rejecting + * an unsupported version matters most โ€” that is the case where the data is + * perfectly good and only this build is too old to read it. + */ +export const isKnownMarksShape = (raw: unknown): boolean => { + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) return false; + const o = raw as Record; + if (o.version !== undefined && o.version !== SUPPORTED_VERSION) return false; + if ( + o.pins !== undefined && + (typeof o.pins !== 'object' || o.pins === null || Array.isArray(o.pins)) + ) { + return false; + } + if (o.hidden !== undefined && !Array.isArray(o.hidden)) return false; + return true; +}; + export const readMarksFileResult = (filePath: string): MarksRead => { try { - return { - marks: normalizeMarks(JSON.parse(fs.readFileSync(filePath, 'utf-8'))), - known: true, - }; + const raw = JSON.parse(fs.readFileSync(filePath, 'utf-8')); + return { marks: normalizeMarks(raw), known: isKnownMarksShape(raw) }; } catch (err) { // A missing file IS authoritative: no store yet means no marks yet, which // is simply the first run. Anything else โ€” permissions, IO, malformed diff --git a/src/switcher-ui.tsx b/src/switcher-ui.tsx index 6afa760..b272b6e 100644 --- a/src/switcher-ui.tsx +++ b/src/switcher-ui.tsx @@ -440,6 +440,8 @@ function SwitcherApp() { // False until the first getSessionMarks() response lands. Before that an // empty pin set means "not known yet", not "no pins". const [marksLoaded, setMarksLoaded] = useState(false); + // Set by the first fs.watch push; makes the initial read's response stale. + const marksPushSeenRef = useRef(false); const [pinnedCollapsed, setPinnedCollapsed] = useState(() => { try { return localStorage.getItem('codev-pinned-collapsed') === '1'; @@ -795,11 +797,19 @@ function SwitcherApp() { .getSessionMarks() .then((m: any) => { if (!m) return; + // A watcher push that has already landed is newer than this response by + // definition โ€” the store changed after we asked. Applying the in-flight + // snapshot on top of it would roll the pin set back, and an emptier + // snapshot would then clear and PERSIST pinnedOnly. + if (marksPushSeenRef.current) return; // Guard BEFORE touching state: an unknown read carries empty marks, // and applying them would hide valid pins from the UI until something // else re-reads the store. if (m.known === false) return; - setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); + setSessionMarks({ + pins: m.pins || {}, + hidden: m.hidden || [], + }); // Only an AUTHORITATIVE read makes the pin set trustworthy. A // rejection, a nullish payload, or `known: false` (the store exists // but could not be parsed โ€” main returns empty marks either way) all @@ -814,8 +824,12 @@ function SwitcherApp() { const unsubscribe = window.electronAPI.onSessionMarksUpdated( (_event: any, m: any) => { if (m) { + marksPushSeenRef.current = true; suppressHoverSelection(); - setSessionMarks({ pins: m.pins || {}, hidden: m.hidden || [] }); + setSessionMarks({ + pins: m.pins || {}, + hidden: m.hidden || [], + }); // A push is a real read: the main-side watcher drops unknown reads // rather than broadcasting them, so arriving here means the store // was parsed successfully. From 691c30b87f8027c5b322ad7ffb1ad50f0bf39c95 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 06:58:45 +0800 Subject: [PATCH 11/13] fix(sessions): derive read authority from the normalizer, not beside it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubic P2 on ec98789. Third and narrowest instance of the same family: normalizeMarks is forgiving, and whatever it silently drops gets erased for real by the next read-modify-write. round 5 read failed -> empty -> overwrite (cubic P1) round 6 parsed, wrong schema -> empty -> overwrite (CodeRabbit) round 7 schema fine, an ENTRY dropped -> overwrite (this) `pins: {"abc": "garbage"}` passed the envelope check, normalized to nothing, and was declared authoritative. Fixed by deriving the check instead of extending it. Re-listing the normalizer's per-entry rules in the validator would have been a second copy of one rule, and the next time its tolerance changed the two would drift โ€” which is how this bug existed in the first place. isAuthoritativeRead now compares entry COUNTS against the normalized result, so it follows normalizeMarks automatically. Deduplication is still authoritative (deduping is not dropping); a non-string or empty id is not. Tests: 74 pass, +1 covering four lossy shapes plus the dedupe case. Both halves mutation-verified โ€” removing either count comparison turns it red. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/session-marks.test.ts | 37 +++++++++++++++++++++--- src/session-marks.ts | 60 +++++++++++++++++++++++++++++---------- 2 files changed, 78 insertions(+), 19 deletions(-) diff --git a/src/session-marks.test.ts b/src/session-marks.test.ts index 793d84a..c1f5460 100644 --- a/src/session-marks.test.ts +++ b/src/session-marks.test.ts @@ -6,7 +6,7 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest'; import { emptyMarks, normalizeMarks, - isKnownMarksShape, + isAuthoritativeRead, mutateMarksFile, readMarksFile, readMarksFileResult, @@ -174,9 +174,38 @@ describe('marks file roundtrip', () => { }); it('accepts our own shape, including a bare object', () => { - expect(isKnownMarksShape({ version: 1, pins: {}, hidden: [] })).toBe(true); - expect(isKnownMarksShape({})).toBe(true); - expect(isKnownMarksShape(null)).toBe(false); + const empty = emptyMarks(); + expect( + isAuthoritativeRead({ version: 1, pins: {}, hidden: [] }, empty), + ).toBe(true); + expect(isAuthoritativeRead({}, empty)).toBe(true); + expect(isAuthoritativeRead(null, empty)).toBe(false); + }); + + // The envelope can be ours while individual entries are not: normalizeMarks + // drops those silently, and the next read-modify-write would erase them. + it('rejects a read whose entries do not survive normalization', () => { + const file = path.join(dir, 'session-marks.json'); + const lossy: [string, string][] = [ + [ + '{"version":1,"pins":{"abc":"garbage"},"hidden":[]}', + 'a pin entry that is not an object', + ], + ['{"version":1,"pins":{"abc":null},"hidden":[]}', 'a null pin entry'], + ['{"version":1,"pins":{},"hidden":["ok",42]}', 'a non-string hidden id'], + ['{"version":1,"pins":{},"hidden":[""]}', 'an empty hidden id'], + ]; + for (const [json, why] of lossy) { + fs.writeFileSync(file, json); + expect(readMarksFileResult(file).known, why).toBe(false); + expect(mutateMarksFile(file, (p) => withHidden(p, 'x')).known).toBe( + false, + ); + expect(fs.readFileSync(file, 'utf-8')).toBe(json); + } + // Duplicates are deduped, not dropped โ€” still authoritative. + fs.writeFileSync(file, '{"version":1,"pins":{},"hidden":["a","a"]}'); + expect(readMarksFileResult(file).known).toBe(true); }); // Every mutation is read-modify-write over the whole file, so a read that diff --git a/src/session-marks.ts b/src/session-marks.ts index 11994c5..f479b3c 100644 --- a/src/session-marks.ts +++ b/src/session-marks.ts @@ -145,34 +145,64 @@ export interface MarksRead { const SUPPORTED_VERSION = 1; /** - * Does a parsed value look like a store we understand? + * Is a parsed value a store we may treat as AUTHORITATIVE? * - * `normalizeMarks` is deliberately forgiving โ€” it coerces anything into valid - * v1 marks so a partly-corrupt store still renders. That is right for display - * and wrong for authority: without this check a `[]`, a `pins: []`, or a - * version-2 file written by a future build would parse, normalize to empty, - * be declared authoritative, and then be OVERWRITTEN by the next pin. Rejecting - * an unsupported version matters most โ€” that is the case where the data is - * perfectly good and only this build is too old to read it. + * `normalizeMarks` is forgiving by design โ€” it coerces anything into valid v1 + * marks so a partly-corrupt store still renders. That is right for display and + * wrong for authority: whatever it silently drops would be erased for real by + * the next read-modify-write. + * + * Two independent ways to lose data, so two checks: + * + * 1. The envelope has to be ours. A `[]`, a `pins: []`, or a version this build + * does not know would normalize to empty and then be overwritten. The + * version case matters most โ€” there the data is perfectly good and only this + * build is too old to read it. + * 2. Normalization has to be LOSSLESS. An envelope can be fine while individual + * entries are not (`pins: {"abc": "garbage"}`), and those are dropped + * silently. This compares counts against the normalized result rather than + * re-listing the normalizer's rules: one definition, so widening its + * tolerance cannot leave a second copy behind to drift. */ -export const isKnownMarksShape = (raw: unknown): boolean => { +export const isAuthoritativeRead = ( + raw: unknown, + marks: SessionMarks, +): boolean => { if (!raw || typeof raw !== 'object' || Array.isArray(raw)) return false; const o = raw as Record; if (o.version !== undefined && o.version !== SUPPORTED_VERSION) return false; - if ( - o.pins !== undefined && - (typeof o.pins !== 'object' || o.pins === null || Array.isArray(o.pins)) - ) { + + const pins = o.pins; + if (pins !== undefined) { + if (typeof pins !== 'object' || pins === null || Array.isArray(pins)) { + return false; + } + if (Object.keys(pins).length !== Object.keys(marks.pins).length) { + return false; + } + } else if (Object.keys(marks.pins).length !== 0) { return false; } - if (o.hidden !== undefined && !Array.isArray(o.hidden)) return false; + + const hidden = o.hidden; + if (hidden !== undefined) { + if (!Array.isArray(hidden)) return false; + // The normalizer dedupes, so compare against the deduped input; a + // non-string or empty id survives the Set but not the filter, which is + // exactly the drop this check exists to catch. + if (new Set(hidden).size !== marks.hidden.length) return false; + } else if (marks.hidden.length !== 0) { + return false; + } + return true; }; export const readMarksFileResult = (filePath: string): MarksRead => { try { const raw = JSON.parse(fs.readFileSync(filePath, 'utf-8')); - return { marks: normalizeMarks(raw), known: isKnownMarksShape(raw) }; + const marks = normalizeMarks(raw); + return { marks, known: isAuthoritativeRead(raw, marks) }; } catch (err) { // A missing file IS authoritative: no store yet means no marks yet, which // is simply the first run. Anything else โ€” permissions, IO, malformed From a076d76b0d4b8d644f96b6d044bbd4f5e50524a8 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 07:29:16 +0800 Subject: [PATCH 12/13] fix(sessions): authority means normalization changed nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth consecutive round of findings in this one predicate, and both reviewers independently landed on the same one this time, so the answer is a different shape of check rather than another condition. round 5 the read failed -> empty -> overwrite round 6 parsed, wrong envelope -> empty -> overwrite round 7 envelope fine, entry DROPPED -> overwrite round 8 entry kept, FIELDS coerced -> overwrite (both bots) plus: an unknown top-level field, silently deleted Each round I enumerated one more way normalizeMarks can be forgiving, and each round review found a narrower one โ€” because "all the ways a forgiving function can be forgiving" is not a list anyone can finish. So the predicate collapses to the invariant it was always approximating: a read is authoritative only if normalizing it changes NOTHING. One canonical-JSON comparison, key order ignored. It has no narrower case left to miss, it tracks normalizeMarks automatically instead of restating its rules beside it, and it is shorter than what it replaces. Deliberately stricter than before: a bare {}, duplicate hidden ids, or a field a future build added are all now refused rather than silently rewritten. Refusing costs one pin action; rewriting costs the user's data. Tests: 75 pass. The load-bearing one asserts a store THIS build wrote round-trips as authoritative โ€” if strict authority ever rejected our own output, pins would quietly stop persisting. Ten rewrite shapes and a key-order case cover the rest. Mutation- verified: forcing authority true fails 2, dropping key sorting fails 1. Also fixes a doc block cubic caught: the cubic check-runs snippet referenced $SHA defined only in the preceding block, so copied on its own it queried commits//check-runs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- CLAUDE.md | 1 + src/session-marks.test.ts | 79 +++++++++++++++++++++++++++++---------- src/session-marks.ts | 69 +++++++++++++--------------------- 3 files changed, 88 insertions(+), 61 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f15118b..5477a48 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,6 +26,7 @@ **cubic needs a different source, and its review body is not it** โ€” it does not always post one. On PR #137 it reviewed a head, found nothing, and posted no body at all, so a body-based check reported "not reviewed yet". Read the check-run output, which is always present and states the counts: ```bash + SHA=$(gh pr view --json headRefOid --jq .headRefOid) gh api repos/grimmerk/codev/commits/"$SHA"/check-runs \ --jq '.check_runs[] | select(.name|test("cubic")) | "\(.conclusion) โ€” \(.output.summary)"' # "success โ€” AI review completed with 1 review. 0 issues found across 4 files" diff --git a/src/session-marks.test.ts b/src/session-marks.test.ts index c1f5460..f51d8e3 100644 --- a/src/session-marks.test.ts +++ b/src/session-marks.test.ts @@ -173,39 +173,80 @@ describe('marks file roundtrip', () => { expect(readMarksFileResult(file).known).toBe(true); }); - it('accepts our own shape, including a bare object', () => { - const empty = emptyMarks(); - expect( - isAuthoritativeRead({ version: 1, pins: {}, hidden: [] }, empty), - ).toBe(true); - expect(isAuthoritativeRead({}, empty)).toBe(true); - expect(isAuthoritativeRead(null, empty)).toBe(false); + // The guard that matters most: if strict authority ever rejected our OWN + // output, pins would silently stop persisting. Round-tripping must hold. + it('treats a store this build wrote as authoritative', () => { + const file = path.join(dir, 'session-marks.json'); + const marks = withHidden( + withPin(emptyMarks(), 'abc', { + pinnedAt: '2026-07-14T01:02:03Z', + cwd: '/repo', + accountLabel: 'work', + }), + 'junk-1', + ); + writeMarksFile(file, marks); + const read = readMarksFileResult(file); + expect(read.known).toBe(true); + expect(read.marks).toEqual(marks); + expect(isAuthoritativeRead(marks, marks)).toBe(true); }); - // The envelope can be ours while individual entries are not: normalizeMarks - // drops those silently, and the next read-modify-write would erase them. - it('rejects a read whose entries do not survive normalization', () => { + // Authority is "normalization is a no-op". Enumerating the ways a forgiving + // normalizer can differ is not a list anyone can finish โ€” four review rounds + // each found one more โ€” so the check compares the whole result instead. + it('refuses any store this build would rewrite', () => { const file = path.join(dir, 'session-marks.json'); - const lossy: [string, string][] = [ + const rewritten: [string, string][] = [ + ['[]', 'an array is not a marks object'], + ['{}', 'missing envelope โ€” we would add version/pins/hidden'], + [ + '{"version":2,"pins":{},"hidden":[]}', + 'a newer format this build cannot read', + ], + ['{"version":1,"pins":[],"hidden":[]}', 'pins must be an object'], + ['{"version":1,"pins":{},"hidden":{}}', 'hidden must be an array'], [ '{"version":1,"pins":{"abc":"garbage"},"hidden":[]}', - 'a pin entry that is not an object', + 'a pin entry that is dropped', + ], + [ + '{"version":1,"pins":{"abc":{"pinnedAt":42,"cwd":7,"group":null}},"hidden":[]}', + 'a pin entry whose FIELDS are coerced', ], - ['{"version":1,"pins":{"abc":null},"hidden":[]}', 'a null pin entry'], ['{"version":1,"pins":{},"hidden":["ok",42]}', 'a non-string hidden id'], - ['{"version":1,"pins":{},"hidden":[""]}', 'an empty hidden id'], + [ + '{"version":1,"pins":{},"hidden":["a","a"]}', + 'duplicates we would collapse', + ], + [ + '{"version":1,"pins":{},"hidden":[],"groups":{"x":1}}', + 'an unknown field a future build added', + ], ]; - for (const [json, why] of lossy) { + for (const [json, why] of rewritten) { fs.writeFileSync(file, json); expect(readMarksFileResult(file).known, why).toBe(false); expect(mutateMarksFile(file, (p) => withHidden(p, 'x')).known).toBe( false, ); - expect(fs.readFileSync(file, 'utf-8')).toBe(json); + expect(fs.readFileSync(file, 'utf-8'), why).toBe(json); } - // Duplicates are deduped, not dropped โ€” still authoritative. - fs.writeFileSync(file, '{"version":1,"pins":{},"hidden":["a","a"]}'); - expect(readMarksFileResult(file).known).toBe(true); + }); + + it('ignores key order, which is not a rewrite', () => { + const marks = withPin(emptyMarks(), 'abc', { + pinnedAt: '2026-07-14T01:02:03Z', + cwd: '/repo', + }); + const reordered = { + hidden: [], + pins: { + abc: { group: null, cwd: '/repo', pinnedAt: '2026-07-14T01:02:03Z' }, + }, + version: 1, + }; + expect(isAuthoritativeRead(reordered, marks)).toBe(true); }); // Every mutation is read-modify-write over the whole file, so a read that diff --git a/src/session-marks.ts b/src/session-marks.ts index f479b3c..1978797 100644 --- a/src/session-marks.ts +++ b/src/session-marks.ts @@ -144,59 +144,44 @@ export interface MarksRead { /** The only store version this build understands. */ const SUPPORTED_VERSION = 1; +/** Stable JSON โ€” object keys sorted, so key ORDER can never fake a difference. */ +const canonical = (value: unknown): string => + JSON.stringify(value, (_key, val) => + val && typeof val === 'object' && !Array.isArray(val) + ? Object.fromEntries( + Object.entries(val as Record).sort(([x], [y]) => + x < y ? -1 : x > y ? 1 : 0, + ), + ) + : val, + ); + /** * Is a parsed value a store we may treat as AUTHORITATIVE? * * `normalizeMarks` is forgiving by design โ€” it coerces anything into valid v1 * marks so a partly-corrupt store still renders. That is right for display and - * wrong for authority: whatever it silently drops would be erased for real by - * the next read-modify-write. + * wrong for authority: whatever it changes would be written back for real by + * the next read-modify-write, erasing the original. * - * Two independent ways to lose data, so two checks: + * So the invariant is simply **normalization must be a no-op**. Earlier + * versions of this check enumerated the ways normalization can differ โ€” the + * envelope, then dropped entries, then coerced fields, then unknown top-level + * keys โ€” and each round of review found one more that the enumeration missed, + * because "all the ways a forgiving function can be forgiving" is not a list + * anyone can finish. Comparing the whole normalized result against the input + * has no narrower case left to miss, and it tracks `normalizeMarks` + * automatically instead of restating its rules beside it. * - * 1. The envelope has to be ours. A `[]`, a `pins: []`, or a version this build - * does not know would normalize to empty and then be overwritten. The - * version case matters most โ€” there the data is perfectly good and only this - * build is too old to read it. - * 2. Normalization has to be LOSSLESS. An envelope can be fine while individual - * entries are not (`pins: {"abc": "garbage"}`), and those are dropped - * silently. This compares counts against the normalized result rather than - * re-listing the normalizer's rules: one definition, so widening its - * tolerance cannot leave a second copy behind to drift. + * Deliberately strict: a store this build would rewrite in ANY way โ€” including + * one carrying a field a future version added, or a bare `{}` โ€” is refused + * rather than silently rewritten. Refusing costs a lost pin action; rewriting + * costs the user's data. */ export const isAuthoritativeRead = ( raw: unknown, marks: SessionMarks, -): boolean => { - if (!raw || typeof raw !== 'object' || Array.isArray(raw)) return false; - const o = raw as Record; - if (o.version !== undefined && o.version !== SUPPORTED_VERSION) return false; - - const pins = o.pins; - if (pins !== undefined) { - if (typeof pins !== 'object' || pins === null || Array.isArray(pins)) { - return false; - } - if (Object.keys(pins).length !== Object.keys(marks.pins).length) { - return false; - } - } else if (Object.keys(marks.pins).length !== 0) { - return false; - } - - const hidden = o.hidden; - if (hidden !== undefined) { - if (!Array.isArray(hidden)) return false; - // The normalizer dedupes, so compare against the deduped input; a - // non-string or empty id survives the Set but not the filter, which is - // exactly the drop this check exists to catch. - if (new Set(hidden).size !== marks.hidden.length) return false; - } else if (marks.hidden.length !== 0) { - return false; - } - - return true; -}; +): boolean => canonical(raw) === canonical(marks); export const readMarksFileResult = (filePath: string): MarksRead => { try { From 1a583b8e5bab86cb3103233813ae684bf01c8d90 Mon Sep 17 00:00:00 2001 From: Grimmer Kang Date: Thu, 20 Aug 2026 07:57:34 +0800 Subject: [PATCH 13/13] test(sessions): drop an assertion that could not fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cubic P3 x2, both about the tests rather than the predicate โ€” the authority check itself drew no findings this round, which is the first time in five rounds. - Removed `isAuthoritativeRead(marks, marks)`. It passes the same object reference twice, so it reduces to canonical(x) === canonical(x) and is unconditionally true for any implementation. I had described that test as the load-bearing one, which made the line worse than useless: it read as the guard while proving nothing. The real guard is the two lines above it โ€” write to disk, read back, and require known=true โ€” and those do fail if strict authority ever rejects our own output. Mutation-verified after removal: forcing authority false still fails 5 tests. - Removed `treats a parseable but schema-invalid store as unknown`. Every shape it covered is a strict subset of `refuses any store this build would rewrite`, which replaced it. 74 tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvkXq7nfQokaTTnbNP8LhD --- src/session-marks.test.ts | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/src/session-marks.test.ts b/src/session-marks.test.ts index f51d8e3..beb5b65 100644 --- a/src/session-marks.test.ts +++ b/src/session-marks.test.ts @@ -148,31 +148,6 @@ describe('marks file roundtrip', () => { // renders. That is right for display and wrong for authority: a file that // parses but is not our schema must not be declared authoritative, or the // next pin overwrites it. - it('treats a parseable but schema-invalid store as unknown', () => { - const file = path.join(dir, 'session-marks.json'); - const cases: [string, string][] = [ - ['[]', 'an array is not a marks object'], - [ - '{"version":2,"pins":{},"hidden":[]}', - 'a newer format this build cannot read', - ], - ['{"pins":[]}', 'pins must be an object'], - ['{"hidden":{}}', 'hidden must be an array'], - ]; - for (const [json, why] of cases) { - fs.writeFileSync(file, json); - expect(readMarksFileResult(file).known, why).toBe(false); - // and therefore untouchable - expect(mutateMarksFile(file, (p) => withHidden(p, 'x')).known).toBe( - false, - ); - expect(fs.readFileSync(file, 'utf-8')).toBe(json); - } - // A store we do understand stays writable. - writeMarksFile(file, emptyMarks()); - expect(readMarksFileResult(file).known).toBe(true); - }); - // The guard that matters most: if strict authority ever rejected our OWN // output, pins would silently stop persisting. Round-tripping must hold. it('treats a store this build wrote as authoritative', () => { @@ -189,7 +164,6 @@ describe('marks file roundtrip', () => { const read = readMarksFileResult(file); expect(read.known).toBe(true); expect(read.marks).toEqual(marks); - expect(isAuthoritativeRead(marks, marks)).toBe(true); }); // Authority is "normalization is a no-op". Enumerating the ways a forgiving