feat(sessions): Batch 1 PR-2 — pinned zone + manual hide#136
Conversation
- Pinned zone at the top (collapsible, remembered); pin via hover icon or Cmd+D; pinned rows also keep their chronological spot with a gold star marker - Works for deep-search matches beyond the loaded 100 (fetched by id via new get-sessions-by-ids IPC + lazy enrichment) - Hide (hover icon or Shift+Cmd+D) forces a session into the minor fold; reversible from inside the fold; pin/hide are mutually exclusive (pinning unhides, hiding unpins) - Store: ~/.config/codev/session-marks.json, fs.watch push to the renderer, atomic temp+rename writes; pure transitions unit-tested (7 new tests, 52 total) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persisted session pin/hide marks, Electron IPC and preload APIs, bulk session lookup, and a sessions switcher UI with pinned and minor-session zones, keyboard controls, file watching, tests, and release documentation. ChangesSession marks
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SessionsView
participant MainProcess
participant MarksFile
User->>SessionsView: pin or hide selected session
SessionsView->>MainProcess: invoke session mark action
MainProcess->>MarksFile: read, transform, and write marks
MarksFile-->>MainProcess: filesystem update
MainProcess-->>SessionsView: session-marks-updated
SessionsView->>MainProcess: fetch missing pinned sessions by ID
MainProcess-->>SessionsView: ClaudeSession records
SessionsView-->>User: render pinned and minor-session zones
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/switcher-ui.tsx">
<violation number="1" location="src/switcher-ui.tsx:584">
P3: Same unhandled-rejection concern: `window.electronAPI.pinSession(...).then(applyMarksResult)` is missing a `.catch()` handler, as is `toggleHide`'s calls to `unhideSession` and `hideSession`. Adding a `.catch(() => {})` on all four IPC call sites would suppress the unhandled rejection if the main process encounters an error.</violation>
<violation number="2" location="src/switcher-ui.tsx:610">
P2: The `useEffect` that subscribes to `onSessionMarksUpdated` never cleans up the listener on unmount. If the component remounts (StrictMode in dev, hot reload, or any React reconciliation that tears down and rebuilds this tree), a new subscription is added each time without removing the old one, resulting in duplicate callbacks and wasted state updates. Consider storing the unsubscription function returned by `ipcRenderer.on`/`ipcRenderer.removeListener` and calling it in the effect's cleanup function.</violation>
<violation number="3" location="src/switcher-ui.tsx:628">
P2: Rapidly pinning multiple sessions that are outside the loaded list can leave some of them missing from the pinned zone. An earlier `getSessionsByIds` response can overwrite the newer response, and the key ref then suppresses the retry. Consider discarding a response unless its request key still matches `extraPinnedKeyRef.current`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Marks listener returns an unsubscribe; effect cleans up on unmount - Stale getSessionsByIds responses discarded (key check) so rapid pins outside the loaded list can't overwrite newer results - .catch on all marks IPC call sites (no unhandled rejections) - isDestroyed() guard before webContents.send in the marks watcher - Pinned-zone collapse resets selectedSessionIndex (same as minors) - Fold label stays honest when it contains manually hidden sessions (adds 'N hidden' to the descriptor) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Pinning inserts a zone row above the selection, shifting every index while selectedSessionIndex stayed put — the next Cmd+D then acted on an unintended row (user-reported: phantom pins, toggles that seemed to do nothing). After any pin/hide toggle the selection re-anchors to the same session's timeline copy by sessionId. Hidden rows now carry a persistent dim no-entry marker (was only discoverable by hovering inside the fold). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/switcher-ui.tsx (3)
617-628: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove the
localStorage.setItemside effect out of the state updater.
setPinnedCollapsed((prev) => {...localStorage.setItem(...)...})performs a side effect inside a functional updater. React may invoke updaters more than once, so this write can fire redundantly.♻️ Proposed refactor
const togglePinnedCollapsed = () => { - setPinnedCollapsed((prev) => { - const next = !prev; - try { - localStorage.setItem('codev-pinned-collapsed', next ? '1' : '0'); - } catch {} - return next; - }); + setPinnedCollapsed((prev) => { + const next = !prev; + return next; + }); + try { + localStorage.setItem('codev-pinned-collapsed', pinnedCollapsed ? '0' : '1'); + } catch {} setSelectedSessionIndex(0); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 617 - 628, Move the localStorage persistence out of the functional updater in togglePinnedCollapsed. Compute the next pinned-collapsed value from current state through the existing state transition, then persist it separately while preserving the existing key, value format, error handling, and selection reset behavior.Source: Linters/SAST tools
589-616: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer async/await for
togglePin/toggleHide, matching the file's own convention.These use
.then()/.catch()chains, whereas the equivalent one-shot IPC handlers already in this file (openAccountPickerForLaunch, lines 333-353) use async/await with try/catch. As per coding guidelines,src/**/*.{ts,tsx}should "Use async/await for asynchronous operations."♻️ Proposed refactor
- const togglePin = (session: any) => { + const togglePin = async (session: any) => { reanchorSelectionRef.current = session.sessionId; - if (sessionMarks.pins[session.sessionId]) { - window.electronAPI - .unpinSession(session.sessionId) - .then(applyMarksResult) - .catch(() => {}); - } else { - window.electronAPI - .pinSession(session.sessionId, { cwd: session.project, accountLabel: session.accountLabel }) - .then(applyMarksResult) - .catch(() => {}); - } + try { + const r = sessionMarks.pins[session.sessionId] + ? await window.electronAPI.unpinSession(session.sessionId) + : await window.electronAPI.pinSession(session.sessionId, { + cwd: session.project, + accountLabel: session.accountLabel, + }); + applyMarksResult(r); + } catch {} };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 589 - 616, Refactor togglePin and toggleHide to async functions using await for the pin/unpin and hide/unhide IPC calls, replacing their then/catch chains with try/catch handling while preserving reanchorSelectionRef assignment and applyMarksResult invocation.Source: Coding guidelines
671-705: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun the project formatter over the new code — dozens of prettier/ESLint errors across all four files. Static analysis reports consistent "prettier/prettier" spacing/wrapping/trailing-comma errors throughout the new session-marks code; the root cause is the same everywhere (new code wasn't formatted), so a single
prettier --write(oreslint --fix) pass resolves all of them.
src/switcher-ui.tsx#L671-L705: reformat this effect plus the other flagged ranges in the same file (408, 542, 577, 586, 598, 1561-1729, 1897).src/electron-api.d.ts#L131-L149: reformat the newIElectronAPIsession-marks type block (lines 136, 144-146).src/main.ts#L2342-L2404: reformat the new IPC handler block, especiallypin-session(lines 2365-2376).src/preload.ts#L79-L91: reformat the newelectronAPIbridge methods (lines 81-85, 90).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 671 - 705, Run the project formatter or ESLint autofix over the new session-marks code. Reformat the effect around the session-loading useEffect in src/switcher-ui.tsx and the other flagged ranges there (408, 542, 577, 586, 598, 1561-1729, 1897); reformat the IElectronAPI session-marks block in src/electron-api.d.ts (131-149), the IPC handlers in src/main.ts (2342-2404), and the electronAPI bridge methods in src/preload.ts (79-91).Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/session-marks.ts`:
- Around line 163-184: Add an error listener to the watcher created in
watchSessionMarks so fs.watch failures are handled without an unhandled
exception; preserve the existing change callback, debounce behavior, and cleanup
returned by the function.
- Around line 50-51: Reformat the pinnedAt expression in the session-mark
mapping and the corresponding block around the second reported location using
the project’s Prettier configuration, keeping behavior unchanged and ensuring
both sections comply with the 80-column limit.
In `@src/switcher-ui.tsx`:
- Around line 1561-1569: Update the ⇧⌘D branch in the keyboard handler around
selectedSessionIndex so toggleHide(s) is only called when the selected session
is not a __pinnedRow, matching the mouse hide-control restriction; preserve
togglePin(s) behavior for non-shift ⌘D.
---
Nitpick comments:
In `@src/switcher-ui.tsx`:
- Around line 617-628: Move the localStorage persistence out of the functional
updater in togglePinnedCollapsed. Compute the next pinned-collapsed value from
current state through the existing state transition, then persist it separately
while preserving the existing key, value format, error handling, and selection
reset behavior.
- Around line 589-616: Refactor togglePin and toggleHide to async functions
using await for the pin/unpin and hide/unhide IPC calls, replacing their
then/catch chains with try/catch handling while preserving reanchorSelectionRef
assignment and applyMarksResult invocation.
- Around line 671-705: Run the project formatter or ESLint autofix over the new
session-marks code. Reformat the effect around the session-loading useEffect in
src/switcher-ui.tsx and the other flagged ranges there (408, 542, 577, 586, 598,
1561-1729, 1897); reformat the IElectronAPI session-marks block in
src/electron-api.d.ts (131-149), the IPC handlers in src/main.ts (2342-2404),
and the electronAPI bridge methods in src/preload.ts (79-91).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81090522-cb66-451c-9b85-8b8637cf23cb
📒 Files selected for processing (11)
CHANGELOG.mdREADME.mddocs/session-finding-plan.mdpackage.jsonsrc/claude-session-utility.tssrc/electron-api.d.tssrc/main.tssrc/preload.tssrc/session-marks.test.tssrc/session-marks.tssrc/switcher-ui.tsx
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- fs.watch gets an error handler (dir gone / watcher limits would otherwise crash the main process) - Re-anchor ref now armed together with the marks state update, so an unrelated render can't consume it before the list reshuffles - Shift+Cmd+D respects the pinned-zone restriction like the mouse UI - session-marks.ts prettier'd (new file) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/switcher-ui.tsx (1)
674-707: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
getSessionsByIdsfailure permanently blocks retry for the same pin set.
extraPinnedKeyRef.currentis set tokeybefore the fetch (line 679), so if the IPC call rejects, the.catch(() => {})on line 706 swallows the error and the guard on line 678 prevents any retry until the missing-ID set changes. A transient main-process error would leave those pinned sessions absent from the zone with no recovery path.Consider resetting
extraPinnedKeyRef.currentin the catch handler so the next render retries:🛡️ Proposed fix
window.electronAPI.getSessionsByIds(missing).then((result: any[]) => { // Drop stale responses (a newer pin set superseded this request) if (extraPinnedKeyRef.current !== key) return; const found = result || []; setExtraPinnedSessions(found); if (found.length === 0) return; window.electronAPI.loadSessionEnrichment(found).then((enrichment) => { if (enrichment.titles && Object.keys(enrichment.titles).length > 0) { setCustomTitles((prev: Record<string, string>) => ({ ...prev, ...enrichment.titles })); } if (enrichment.branches && Object.keys(enrichment.branches).length > 0) { setBranches((prev: Record<string, string>) => ({ ...prev, ...enrichment.branches })); } if (enrichment.prLinks && Object.keys(enrichment.prLinks).length > 0) { setPrLinks((prev) => ({ ...prev, ...enrichment.prLinks })); } }); window.electronAPI.loadLastAssistantResponses(found).then((responses: Record<string, string>) => { if (responses && Object.keys(responses).length > 0) { setAssistantResponses((prev: Record<string, string>) => ({ ...prev, ...responses })); } }); - }).catch(() => {}); + }).catch(() => { + // Allow retry on next render — a transient IPC failure shouldn't + // permanently block pinned-session enrichment. + extraPinnedKeyRef.current = ''; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 674 - 707, Reset extraPinnedKeyRef.current in the getSessionsByIds promise catch handler so a rejected fetch does not permanently suppress retries for the same missing-ID set. Keep the existing stale-response guard and successful loading behavior unchanged, while allowing the next render of this useEffect to retry the request.
🧹 Nitpick comments (1)
src/switcher-ui.tsx (1)
590-590: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrettier formatting violations flagged by static analysis.
Lines 590, 601, and 1569–1574 have prettier/prettier errors. Line 601 also violates the coding guideline for trailing commas — the object
{ cwd: session.project, accountLabel: session.accountLabel }should have a trailing comma and be multi-lined per prettier.Run
npx prettier --write src/switcher-ui.tsxto auto-fix.As per coding guidelines,
src/**/*.{ts,tsx}files should use trailing commas in arrays/objects.Also applies to: 601-601, 1569-1574
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` at line 590, Run Prettier on src/switcher-ui.tsx to correct formatting violations at setSessionMarks and the other flagged sections, including multiline formatting and trailing commas for the object near session.project and session.accountLabel. Preserve the existing behavior.Sources: Coding guidelines, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/switcher-ui.tsx`:
- Around line 674-707: Reset extraPinnedKeyRef.current in the getSessionsByIds
promise catch handler so a rejected fetch does not permanently suppress retries
for the same missing-ID set. Keep the existing stale-response guard and
successful loading behavior unchanged, while allowing the next render of this
useEffect to retry the request.
---
Nitpick comments:
In `@src/switcher-ui.tsx`:
- Line 590: Run Prettier on src/switcher-ui.tsx to correct formatting violations
at setSessionMarks and the other flagged sections, including multiline
formatting and trailing commas for the object near session.project and
session.accountLabel. Preserve the existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c13075b9-947d-48f2-8cad-00206652f70e
📒 Files selected for processing (2)
src/session-marks.tssrc/switcher-ui.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/session-marks.ts
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/session-marks.ts">
<violation number="1" location="src/session-marks.ts:179">
P2: After a watcher error, external edits to `session-marks.json` stop reaching the open Sessions UI until restart. The handler consumes the error but leaves `marksWatcherCleanup` populated, so `ensureMarksWatcher()` will never create a replacement watcher. Consider reporting the failure to the owner and clearing/recreating the directory watcher (with bounded retry/backoff where appropriate) instead of treating the failed watcher as live.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }, 50); | ||
| }); | ||
|
|
||
| watcher.on('error', () => { |
There was a problem hiding this comment.
P2: After a watcher error, external edits to session-marks.json stop reaching the open Sessions UI until restart. The handler consumes the error but leaves marksWatcherCleanup populated, so ensureMarksWatcher() will never create a replacement watcher. Consider reporting the failure to the owner and clearing/recreating the directory watcher (with bounded retry/backoff where appropriate) instead of treating the failed watcher as live.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/session-marks.ts, line 179:
<comment>After a watcher error, external edits to `session-marks.json` stop reaching the open Sessions UI until restart. The handler consumes the error but leaves `marksWatcherCleanup` populated, so `ensureMarksWatcher()` will never create a replacement watcher. Consider reporting the failure to the owner and clearing/recreating the directory watcher (with bounded retry/backoff where appropriate) instead of treating the failed watcher as live.</comment>
<file context>
@@ -177,6 +176,12 @@ export const watchSessionMarks = (
}, 50);
});
+ watcher.on('error', () => {
+ // Swallow watcher errors (dir deleted, permissions changed, OS watcher
+ // limits) — an unhandled 'error' event would crash the main process.
</file context>
Two more root causes from live testing:
- Clicking any pin/hide icon or fold control moved focus off the
search input, silently killing every Cmd+D until a tab switch
refocused it ('shortcut randomly stops working'). All non-input
controls now preventDefault on mousedown so focus never leaves.
- Pinned(N) counted only pins that resolved to a loaded session
object; a pinned VS Code session (absent from history.jsonl until
the closed-scan merges) flapped the count on every tab switch.
Unresolvable pins now render a placeholder row built from the pin
record, so the zone and its count stay stable.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/switcher-ui.tsx">
<violation number="1" location="src/switcher-ui.tsx:581">
P3: The new pinned-row placeholder (used when a pin can't be resolved from `allSessions`/`extraPinnedSessions`) always renders '0 msgs' since `messageCount` defaults to 0 — this can mislead users into thinking a pinned session is empty when it's actually just not yet resolved (or permanently gone). Consider special-casing the placeholder render (e.g. showing '…' or omitting the count) instead of a literal 0.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| firstUserMessage: '', | ||
| lastUserMessage: '', | ||
| lastTimestamp: 0, | ||
| messageCount: 0, |
There was a problem hiding this comment.
P3: The new pinned-row placeholder (used when a pin can't be resolved from allSessions/extraPinnedSessions) always renders '0 msgs' since messageCount defaults to 0 — this can mislead users into thinking a pinned session is empty when it's actually just not yet resolved (or permanently gone). Consider special-casing the placeholder render (e.g. showing '…' or omitting the count) instead of a literal 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/switcher-ui.tsx, line 581:
<comment>The new pinned-row placeholder (used when a pin can't be resolved from `allSessions`/`extraPinnedSessions`) always renders '0 msgs' since `messageCount` defaults to 0 — this can mislead users into thinking a pinned session is empty when it's actually just not yet resolved (or permanently gone). Consider special-casing the placeholder render (e.g. showing '…' or omitting the count) instead of a literal 0.</comment>
<file context>
@@ -565,14 +565,30 @@ function SwitcherApp() {
+ firstUserMessage: '',
+ lastUserMessage: '',
+ lastTimestamp: 0,
+ messageCount: 0,
+ isActive: false,
+ accountLabel: info.accountLabel,
</file context>
After a tab switch selection resets to -1 and Cmd+D silently fell back to row 0 — the first pinned-zone row — so a bare Cmd+D toggled the most recently pinned session (user-reported: Pinned(1)<->(2) oscillation, accidental pin of the top row). Enter keeps its open-top-result default; the mutating shortcut now no-ops without a hovered/arrowed selection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/switcher-ui.tsx (3)
1579-1595: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun Prettier on the keyboard shortcut branch.
ESLint reports
prettier/prettiererrors on Lines [1580]-[1587]; this should be formatted before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 1579 - 1595, Run Prettier on the keyboard shortcut branch around the ⌘D handler in the event-handling logic, applying the repository’s standard formatting to the affected conditional and nested statements without changing its behavior.Source: Linters/SAST tools
1621-1636: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRender the pinned zone when the main session list is empty.
When
sessions.length === 0, the surrounding conditional skips this block—even ifshowPinnedZoneis true andextraPinnedSessionssupplied unresolved pinned rows. Users then cannot see or expand their pinned sessions.- {sessions.length === 0 ? ( + {sessions.length === 0 && !showPinnedZone ? (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 1621 - 1636, Update the surrounding render condition for the pinned-zone block containing togglePinnedCollapsed so it also renders when sessions.length === 0 and showPinnedZone is true. Preserve the existing behavior for non-empty session lists and ensure unresolved extraPinnedSessions remain visible and expandable through pinnedRows.
1738-1759: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake pin/hide controls keyboard-accessible.
These clickable
<span>elements are not focusable and have no button semantics or accessible labels, so keyboard and assistive-technology users cannot operate the new mouse controls. Use<button type="button">witharia-labelinstead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 1738 - 1759, Replace the pin and hide clickable spans in the selected-session controls with type="button" buttons, preserving their click and mouse-down behavior and visual styling. Add descriptive aria-label values that reflect each control’s current action, such as pin/unpin and hide/unhide, while keeping the existing conditional rendering and handlers unchanged.
🧹 Nitpick comments (2)
src/switcher-ui.tsx (2)
418-420: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftUse strict session types for the new pinning state.
extraPinnedSessionsand the pinned-row pipeline are typed withany, which weakens the new by-ID/placeholder contract. Define a sharedSessiontype and use it for fetched sessions, placeholders, and derived pinned rows.As per coding guidelines,
src/**/*.{ts,tsx}requires strict typing.Also applies to: 532-590
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 418 - 420, Replace the any-based typing for extraPinnedSessions and the pinned-row pipeline in the relevant switcher UI logic with a shared strict Session type. Use Session consistently for fetched-by-ID sessions, placeholder entries, state, and derived pinned rows, preserving the existing pinning behavior and avoiding any in these paths.Source: Coding guidelines
609-634: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse async/await for the new IPC flows.
The pin/hide helpers and marks-loading/enrichment effects use Promise chains. Convert these new asynchronous paths to
asyncfunctions withtry/catchfor consistency and clearer error handling.As per coding guidelines, asynchronous operations should use async/await.
Also applies to: 673-723
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/switcher-ui.tsx` around lines 609 - 634, Convert the asynchronous flows in togglePin, toggleHide, and the marks-loading/enrichment effects around the referenced section from Promise .then/.catch chains to async/await. Mark the relevant helpers and effect callbacks async where appropriate, await each IPC operation, and wrap the operations in try/catch while preserving the existing applyMarksResult behavior and error-swallowing semantics.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/switcher-ui.tsx`:
- Around line 1579-1595: Run Prettier on the keyboard shortcut branch around the
⌘D handler in the event-handling logic, applying the repository’s standard
formatting to the affected conditional and nested statements without changing
its behavior.
- Around line 1621-1636: Update the surrounding render condition for the
pinned-zone block containing togglePinnedCollapsed so it also renders when
sessions.length === 0 and showPinnedZone is true. Preserve the existing behavior
for non-empty session lists and ensure unresolved extraPinnedSessions remain
visible and expandable through pinnedRows.
- Around line 1738-1759: Replace the pin and hide clickable spans in the
selected-session controls with type="button" buttons, preserving their click and
mouse-down behavior and visual styling. Add descriptive aria-label values that
reflect each control’s current action, such as pin/unpin and hide/unhide, while
keeping the existing conditional rendering and handlers unchanged.
---
Nitpick comments:
In `@src/switcher-ui.tsx`:
- Around line 418-420: Replace the any-based typing for extraPinnedSessions and
the pinned-row pipeline in the relevant switcher UI logic with a shared strict
Session type. Use Session consistently for fetched-by-ID sessions, placeholder
entries, state, and derived pinned rows, preserving the existing pinning
behavior and avoiding any in these paths.
- Around line 609-634: Convert the asynchronous flows in togglePin, toggleHide,
and the marks-loading/enrichment effects around the referenced section from
Promise .then/.catch chains to async/await. Mark the relevant helpers and effect
callbacks async where appropriate, await each IPC operation, and wrap the
operations in try/catch while preserving the existing applyMarksResult behavior
and error-swallowing semantics.
Root cause of three rounds of pin chaos (wrong sessions getting pinned, counts jumping 1->2->3, a never-touched session surviving restart): pin/hide moves rows under a STATIONARY cursor; Chromium re-hit-tests after layout changes and fires mouseenter on whatever row slid under the mouse, teleporting the selection and overriding the re-anchor — so the next Cmd+D acted on an unrelated session. The window-show code already suppresses exactly this phenomenon with ignoreMouseEnterRef; now every marks-driven layout change gets the same 400ms suppression (token-guarded), and the pinned zone appends new pins at the bottom (pinnedAt asc) instead of reshuffling existing rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The timeline duplicate of a pinned session read as noise, not signal — pinning now MOVES the row into the zone (search still shows everything). The zone rows gain the hide control (unpin + fold in one step, per the pin/hide exclusivity) since the timeline path is gone, and Shift+Cmd+D allows the same. Marks IPC handlers log an audit line for the reported cross-restart pin-loss (not yet reproduced with evidence). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/switcher-ui.tsx">
<violation number="1" location="src/switcher-ui.tsx:541">
P3: The pinnedById/pinnedRows comment still documents dual placement ('keeps its chronological spot below'), but the new `continue` above now removes pinned sessions from the timeline entirely. Update that comment to match the zone-only model so future readers aren't misled about where pinned rows render.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| 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; |
There was a problem hiding this comment.
P3: The pinnedById/pinnedRows comment still documents dual placement ('keeps its chronological spot below'), but the new continue above now removes pinned sessions from the timeline entirely. Update that comment to match the zone-only model so future readers aren't misled about where pinned rows render.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/switcher-ui.tsx, line 541:
<comment>The pinnedById/pinnedRows comment still documents dual placement ('keeps its chronological spot below'), but the new `continue` above now removes pinned sessions from the timeline entirely. Update that comment to match the zone-only model so future readers aren't misled about where pinned rows render.</comment>
<file context>
@@ -536,6 +536,9 @@ function SwitcherApp() {
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 minor =
!isSearchingSessions &&
</file context>
1 participant
Summary
Session-finding Batch 1 PR-2 — pins & manual hide (plan:
docs/session-finding-plan.md§4.4). Follows PR #132; design confirmed with the user (pinned-section model chosen over a separate favorites view — zero mode-switch for the highest-frequency path).📌 Pins
▾ 📌 Pinned (N)), collapsed state remembered (localStorage). Rows fully reuse the existing session row — status dots, badges, PR links, snippets all work.get-sessions-by-idsIPC (reads the full session cache) and lazily enriches them.⊘ Manual hide (C1's missing piece)
Store
~/.config/codev/session-marks.json— single cross-account file next to the accounts registry (JSON on purpose: tiny data, human-debuggable, no SQLite features needed):{ "version": 1, "pins": { "<sessionId>": { "pinnedAt": "…", "cwd": "…", "accountLabel": "…", "group": null } }, "hidden": ["<sessionId>"] }--resume/--continuereuse the id; only--fork-sessionforks), so plain sessionId keying needs no migration logic.groupfield reserved for v2 named groups.Tests
session-marks.test.ts): normalize (garbage/defaults/dedupe), pin↔hide mutual exclusion, idempotent removals, input immutability, tmpdir file roundtrip incl. corrupt-file recovery and no temp leftovers — 52 total pass;tsc --noEmitclean.Out of scope
/pinin-session slash command (Batch 3, plan §6).🤖 On behalf of @grimmerk — generated with Claude Code
Summary by cubic
Adds session pins and manual hide with a collapsible 📌 Pinned zone. Pinned sessions now live only in the zone while browsing (no timeline duplicate), stay searchable, and sync across accounts via
~/.config/codev/session-marks.json.New Features
getSessionsByIds+ lazy enrichment; unresolved pins render placeholders so Pinned(N) stays stable.Bug Fixes
webContents.sendis guarded withisDestroyed();fs.watchhas an error handler.getSessionsByIdsresponses are dropped; all marks IPC calls.catcherrors; audit logs added for pin/unpin/hide.Written for commit 9aa4b7e. Summary will update on new commits.
Summary by CodeRabbit
~/.config/codev/session-marks.jsonand syncs live when the file changes.