feat: centralised getEffectiveCwd() + provider helpers with stale session guard#114
Conversation
InbarR
left a comment
There was a problem hiding this comment.
Direction is good - having a single source of truth for the effective CWD makes sense, especially with the idle-session guard.
Two things before merging:
-
The new e2e spec (
effective-cwd-helper.spec.ts) fails on CI because it usesrequire(...)inside a browser context, and browsers don't have CommonJSrequire. Thetypeof getEffectiveCwd === 'function'fallback never runs becauserequirethrows first. Easiest fix is a top-level ES import, or expose the helper onwindowfrom the renderer for e2e access. -
The PR description mentions ~10 call sites being migrated, but only
DiffReview.tsxis converted in this diff. Are the rest planned for a follow-up?
The other Playwright failures look like the same pre-existing base-branch issue I noted on #112, not anything you introduced.
Suggest landing #112 first since this one builds on it, then this rebases cleanly.
Follow-up to #3 (diff panel CWD resolution). Two improvements based on adversarial review feedback: 1. Stale session guard: getEffectiveCwd() only prefers AI session CWD when the session is active (status !== 'idle'). After CLI exit, the session goes idle and the helper falls back to shell CWD — preventing the diff panel from showing the wrong repo. 2. Centralized helper: findSessionById() and getEffectiveCwd() are exported from terminal-store.ts. This eliminates the hardcoded copilotSessions.find() ?? claudeCodeSessions.find() pattern and makes adding a third provider a single-point change. DiffReview.tsx now uses getEffectiveCwd() instead of inline logic. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous spec called require('../src/renderer/state/terminal-store')
inside window.evaluate(). Electron's renderer is a sandboxed browser
context with no CommonJS require, so the call threw and the typeof
fallback never ran, masking real failures and tripping CI.
App.tsx now exposes getEffectiveCwd, findSessionById and
getSessionProvider on window alongside __terminalStore, and the
spec calls the real helper directly. The inline fallback is removed
so the test now actually exercises production code.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the small getSessionProvider() helper next to findSessionById()
in terminal-store, then converts every remaining call site of the
duplicated copilotSessions.find/some + claudeCodeSessions.find/some
pattern across the renderer:
- terminal-store.ts: snapshotPaneForRestore, getStartupCommand,
linker idle-session guard, resumeAllSessions
- StatusBar.tsx: focusedAiSession + terminalListEntries.findStatus
- TabBar.tsx: aiStatus selector
- TerminalPanel.tsx: latestPrompt, latestPromptTime,
sessionStatus, aiProvider selectors
- DiffReview.tsx: agentLabel provider branch
When a third provider is added, only the two helpers in
terminal-store need touching. Net -28 lines, no behaviour change.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7e3847c to
946bdfb
Compare
|
Both points addressed in the latest push: 1. 2. The ~10 call sites are migrated in this PR (not deferred). Also rebased on top of merged #112 (one trivial conflict in |
Summary
Follow-up to #112 — addresses both pieces of @InbarR's review feedback on the original draft, now that #112 has merged.
1. Stale session guard (unchanged from prior revision)
getEffectiveCwd()only prefers the AI session's CWD when the session is active (status !== 'idle'). After the CLI exits and the session goes idle, the helper falls back to the shell's CWD — preventing the diff panel from showing the wrong repo.2. Centralised helpers (now actually applied across the codebase)
Three exported helpers live in
src/renderer/state/terminal-store.ts:findSessionById(copilotSessions, claudeCodeSessions, id)— replaces the duplicatedcs.find(...) ?? ccs.find(...)pattern.getSessionProvider(copilotSessions, claudeCodeSessions, id)— returns'copilot' \| 'claude-code' \| undefinedfor sites that only need the provider tag (replaces.some()duplicates).getEffectiveCwd(terminal, copilotSessions, claudeCodeSessions)— single source of truth forwhere is this terminal working?When a third AI provider is added, only these helpers need updating.
3. e2e test no longer relies on
require()in the rendererThe previous spec used
require('../src/renderer/state/terminal-store')insidewindow.evaluate(). Electron's renderer is a sandboxed browser context with no CommonJSrequire, so the call threw before thetypeoffallback could run and the test silently failed on CI.App.tsxnow exposes__getEffectiveCwd,__findSessionById, and__getSessionProvideronwindowalongside__terminalStore, and the spec calls the real helpers directly. The inline fallback is gone — the test now actually exercises production code.Changes
src/renderer/state/terminal-store.tsfindSessionById,getSessionProvider,getEffectiveCwd; migrated 4 internal sitessrc/renderer/App.tsxwindowfor e2esrc/renderer/components/DiffReview.tsxgetEffectiveCwd+getSessionProvidersrc/renderer/components/StatusBar.tsxfindSessionByIdsrc/renderer/components/TabBar.tsxaiStatusselector migratedsrc/renderer/components/TerminalPanel.tsxlatestPrompt,latestPromptTime,sessionStatus,aiProvider)tests/e2e/effective-cwd-helper.spec.tsrequire()+ inline fallback; calls the real helper viawindowNet -28 lines across renderer code, no behaviour change for the migrated sites (IDs are unique across the provider arrays, so the lookup-order swap is semantically equivalent).
Test coverage
aiSessionId(session removed)Dependencies
Now rebased on top of merged #112. No outstanding chain.