feat: Persist and restore active session id (resume in last open sess…#714
feat: Persist and restore active session id (resume in last open sess…#714scriptease wants to merge 1 commit intoRunMaestro:rcfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdded persistent active-session tracking: new IPC handlers and preload APIs for getting/setting Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (React)
participant Hook as useSessionRestoration Hook
participant IPC as Preload / IPC
participant Persist as Persistence Store
participant Store as Zustand Store
Renderer->>Hook: mount -> restore sessions
Hook->>IPC: window.maestro.sessions.getAll()
IPC->>Persist: ipc -> sessions:getAll
Persist-->>IPC: list of sessions
IPC-->>Hook: sessions[]
Hook->>IPC: window.maestro.sessions.getActiveSessionId()
IPC->>Persist: ipc -> sessions:getActiveSessionId
Persist-->>IPC: activeSessionId | ''
IPC-->>Hook: activeSessionId
alt activeSessionId matches restored sessions
Hook->>Store: hydrateActiveSessionId(activeSessionId)
else stale or empty
Hook->>Store: setActiveSessionId(fallbackId)
Store->>IPC: window.maestro.sessions.setActiveSessionId(fallbackId)
IPC->>Persist: ipc -> sessions:setActiveSessionId(fallbackId)
end
sequenceDiagram
participant User as User Action
participant Store as Zustand Store
participant IPC as Preload / IPC
participant Persist as Persistence Store
User->>Store: setActiveSessionId(newId)
Store->>Store: update activeSessionId, reset cyclePosition
Store->>IPC: window.maestro.sessions.setActiveSessionId(newId) (fire-and-forget)
IPC->>Persist: ipc -> sessions:setActiveSessionId(newId)
Persist-->>IPC: void
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR adds persistence for the active session ID so Maestro reopens on the last-used session instead of always defaulting to session Key changes:
Issues found:
Confidence Score: 3/5Functionally correct for the happy path, but two issues in sessionStore.ts should be addressed before merging: a side effect inside a Zustand set callback and unthrottled disk writes on every Cmd+J/K cycle step. The IPC plumbing (new handlers, preload bridge, type declarations) is clean and correct. The startup restoration logic in useSessionRestoration.ts works as intended. The deduction comes from sessionStore.ts: (1) setActiveSessionIdInternal places an IPC call inside the Zustand set() callback, violating the pure-function contract, and (2) because that function drives keyboard cycling, every Cmd+J/K keystroke now triggers a disk write with no throttling. Neither blocks the feature from working, but both are real defects that should be fixed. src/renderer/stores/sessionStore.ts — setActiveSessionIdInternal side-effect placement and unthrottled persistence during cycling Important Files Changed
Sequence DiagramsequenceDiagram
participant R as Renderer (useSessionRestoration)
participant S as sessionStore (Zustand)
participant P as Preload Bridge
participant M as Main Process (persistence.ts)
participant D as maestro-sessions.json
Note over R,D: App startup — restore active session
R->>M: sessions:getAll()
M->>D: sessionsStore.get('sessions', [])
D-->>M: StoredSession[]
M-->>R: savedSessions[]
R->>R: restoreSession() x N
R->>M: sessions:getActiveSessionId()
M->>D: sessionsStore.get('activeSessionId', '')
D-->>M: savedId (string)
M-->>R: savedId
R->>R: targetId = savedId found? savedId : sessions[0].id
R->>S: setActiveSessionId(targetId)
S->>S: set({ activeSessionId, cyclePosition: -1 })
S->>P: setActiveSessionId(targetId)
P->>M: sessions:setActiveSessionId(targetId)
M->>D: sessionsStore.set('activeSessionId', targetId)
Note over R,D: User switches session (explicit click / Cmd+J/K)
R->>S: setActiveSessionId(id) or setActiveSessionIdInternal(id)
S->>S: set({ activeSessionId: id })
S->>P: setActiveSessionId(id)
P->>M: sessions:setActiveSessionId(id)
M->>D: sessionsStore.set('activeSessionId', id)
Reviews (1): Last reviewed commit: "feat: Persist and restore active session..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/renderer/stores/sessionStore.ts
Outdated
| setActiveSessionIdInternal: (v) => { | ||
| set((s) => { | ||
| const newId = resolve(v, s.activeSessionId); | ||
| window.maestro?.sessions?.setActiveSessionId(newId); | ||
| return { activeSessionId: newId }; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
set callback
setActiveSessionIdInternal performs an IPC call (window.maestro?.sessions?.setActiveSessionId) inside the Zustand set((s) => {...}) callback. Zustand expects this function to be a pure state producer — it should only compute and return the next state. Running side effects here can lead to double-invocations in strict/dev modes and makes the intent ambiguous.
setActiveSessionId (the non-internal variant) correctly places the IPC call after the set(...) call. The same pattern should be applied here:
setActiveSessionIdInternal: (v) => {
let newId: string | undefined;
set((s) => {
newId = resolve(v, s.activeSessionId);
return { activeSessionId: newId };
});
if (newId !== undefined) {
window.maestro?.sessions?.setActiveSessionId(newId);
}
},
src/renderer/stores/sessionStore.ts
Outdated
| setActiveSessionIdInternal: (v) => { | ||
| set((s) => { | ||
| const newId = resolve(v, s.activeSessionId); | ||
| window.maestro?.sessions?.setActiveSessionId(newId); | ||
| return { activeSessionId: newId }; | ||
| }); | ||
| }, |
There was a problem hiding this comment.
setActiveSessionIdInternal is the code path used for keyboard-based session cycling (Cmd+J/K). With this change, every cycle step now fires an IPC write to disk. A user holding Cmd+J to scroll through 10 sessions produces 10 writes in rapid succession.
setActiveSessionId (the explicit user-intent path) is the right place for persistence. Consider either:
- Skipping persistence in
setActiveSessionIdInternalentirely — only persist on explicitsetActiveSessionIdcalls (keyboard cycling is transient intent, not a committed "last open session"). - Debouncing the IPC call so only the final resting session ID is written.
Option 1 is simpler and more semantically correct: only write when the user deliberately switches sessions, not while they are mid-cycle.
| const savedActiveSessionId = await window.maestro.sessions.getActiveSessionId(); | ||
| const targetSessionId = | ||
| savedActiveSessionId && restoredSessions.find((s) => s.id === savedActiveSessionId) | ||
| ? savedActiveSessionId | ||
| : restoredSessions[0]?.id; | ||
| if (targetSessionId) { | ||
| setActiveSessionId(targetSessionId); | ||
| } |
There was a problem hiding this comment.
setActiveSessionId persists during initial load, potentially overwriting a valid saved ID
When the saved activeSessionId isn't found in the restored sessions (e.g., that session was deleted), the code falls back to restoredSessions[0]?.id. setActiveSessionId is then called, which immediately fires window.maestro?.sessions?.setActiveSessionId(id) and overwrites the persisted ID with the fallback.
This is correct behavior for the deletion case, but it also means that if sessions haven't fully migrated yet (unlikely but possible), the fallback ID is permanently stored before the next app run can attempt a re-match. A clarifying comment here would help future maintainers understand this intentional overwrite.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/renderer/hooks/session/useSessionRestoration.ts (1)
433-441: Minor redundant IPC write when restoring the same active session ID.When
savedActiveSessionIdis valid, callingsetActiveSessionId(targetSessionId)triggers a write back to IPC (via the store action's side effect), even though we just read that same value. This is harmless but slightly inefficient.If you want to avoid the redundant write, you could use a store-internal setter that skips the IPC call when the value hasn't changed, or conditionally call it only when the ID differs from
savedActiveSessionId.♻️ Optional: skip redundant IPC write
const targetSessionId = savedActiveSessionId && restoredSessions.find((s) => s.id === savedActiveSessionId) ? savedActiveSessionId : restoredSessions[0]?.id; if (targetSessionId) { - setActiveSessionId(targetSessionId); + // Only persist if we're changing from the saved value (avoid redundant IPC write) + if (targetSessionId !== savedActiveSessionId) { + setActiveSessionId(targetSessionId); + } else { + // Just update store state without IPC round-trip + useSessionStore.setState({ activeSessionId: targetSessionId, cyclePosition: -1 }); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/session/useSessionRestoration.ts` around lines 433 - 441, The restore flow in useSessionRestoration.ts reads savedActiveSessionId via window.maestro.sessions.getActiveSessionId() then unconditionally calls setActiveSessionId(targetSessionId), causing a redundant IPC write when the value is unchanged; change the logic to only call setActiveSessionId(targetSessionId) when targetSessionId !== savedActiveSessionId (or use a store-internal setter that skips IPC when unchanged) so that setActiveSessionId is skipped if the active ID is already the saved value.src/__tests__/renderer/hooks/useSessionRestoration.test.ts (1)
137-141: Test mocks are correctly updated.The mock setup aligns with the global test setup. Consider adding a test case that verifies
getActiveSessionIdis called during session restoration and that its value is used when valid, to ensure the new IPC integration is covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/hooks/useSessionRestoration.test.ts` around lines 137 - 141, Add a test in useSessionRestoration.test.ts that asserts the hook calls the mocked IPC method getActiveSessionId and uses its resolved value when it's non-empty: set (window as any).maestro.sessions.getActiveSessionId to return a valid session id, spy on that mock to ensure it was invoked during restoration, and verify the hook selects/sets that session as active (e.g., by asserting setActiveSessionId or the hook's resulting state). Target the useSessionRestoration behavior and the mocks getActiveSessionId and setActiveSessionId so the new IPC integration is covered.src/__tests__/main/ipc/handlers/persistence.test.ts (1)
144-145: Consider adding unit tests for the new handlers' behavior.The registration test is updated correctly, but there are no tests verifying the actual
sessions:getActiveSessionIdandsessions:setActiveSessionIdhandler logic (e.g., default value when unset, persistence after set). Given these handlers are simple get/set operations, this is a minor gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/main/ipc/handlers/persistence.test.ts` around lines 144 - 145, Add unit tests that exercise the actual handler logic for 'sessions:getActiveSessionId' and 'sessions:setActiveSessionId': write one test asserting that invoking or calling the 'sessions:getActiveSessionId' handler returns the expected default (e.g., null/undefined) when nothing has been set, then write a test that calls 'sessions:setActiveSessionId' with a sample ID and asserts a subsequent 'sessions:getActiveSessionId' returns that same ID to verify persistence; place these tests alongside the existing registration test and use the same invocation helper (e.g., ipc.invoke or the registered handler function) and assertions pattern used in the file.src/renderer/stores/sessionStore.ts (1)
201-204: Keep restore hydration separate from user-driven persistence.
useSessionRestorationalready reads the saved id from IPC and then calls this setter (src/renderer/hooks/session/useSessionRestoration.ts:435-445), so startup now does a guaranteed read→write round-trip. It also means an invalid saved id gets rewritten to the fallback first session on launch. A small non-persisting hydrate path would keep restore semantics separate from user-initiated changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/stores/sessionStore.ts` around lines 201 - 204, The setter setActiveSessionId writes to persistent IPC (window.maestro?.sessions?.setActiveSessionId) and is being called by useSessionRestoration during startup, causing an undesired read→write round-trip and overwriting invalid saved ids; create a separate non-persisting hydrate path: add a new store action (e.g., hydrateActiveSessionId or setActiveSessionIdLocal) that only updates the local state (activeSessionId and cyclePosition) without calling window.maestro, leave setActiveSessionId for user-driven changes that must persist, and update useSessionRestoration to call the new non-persisting action instead of setActiveSessionId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/stores/sessionStore.ts`:
- Line 203: The IPC call window.maestro.sessions.setActiveSessionId returns a
Promise and is being invoked without await or error handling (both where it's
called directly and inside the Zustand setter), so move the async persistence
outside the synchronous set((s) => { ... }) callback: call and await
window.maestro.sessions.setActiveSessionId(id) (or handle errors with try/catch)
before calling setActiveSessionIdInternal (or, alternatively, call
setActiveSessionIdInternal only after the await succeeds), and ensure any
invocation in other places also either awaits the promise or catches/reports
errors to avoid unhandled rejections.
---
Nitpick comments:
In `@src/__tests__/main/ipc/handlers/persistence.test.ts`:
- Around line 144-145: Add unit tests that exercise the actual handler logic for
'sessions:getActiveSessionId' and 'sessions:setActiveSessionId': write one test
asserting that invoking or calling the 'sessions:getActiveSessionId' handler
returns the expected default (e.g., null/undefined) when nothing has been set,
then write a test that calls 'sessions:setActiveSessionId' with a sample ID and
asserts a subsequent 'sessions:getActiveSessionId' returns that same ID to
verify persistence; place these tests alongside the existing registration test
and use the same invocation helper (e.g., ipc.invoke or the registered handler
function) and assertions pattern used in the file.
In `@src/__tests__/renderer/hooks/useSessionRestoration.test.ts`:
- Around line 137-141: Add a test in useSessionRestoration.test.ts that asserts
the hook calls the mocked IPC method getActiveSessionId and uses its resolved
value when it's non-empty: set (window as
any).maestro.sessions.getActiveSessionId to return a valid session id, spy on
that mock to ensure it was invoked during restoration, and verify the hook
selects/sets that session as active (e.g., by asserting setActiveSessionId or
the hook's resulting state). Target the useSessionRestoration behavior and the
mocks getActiveSessionId and setActiveSessionId so the new IPC integration is
covered.
In `@src/renderer/hooks/session/useSessionRestoration.ts`:
- Around line 433-441: The restore flow in useSessionRestoration.ts reads
savedActiveSessionId via window.maestro.sessions.getActiveSessionId() then
unconditionally calls setActiveSessionId(targetSessionId), causing a redundant
IPC write when the value is unchanged; change the logic to only call
setActiveSessionId(targetSessionId) when targetSessionId !==
savedActiveSessionId (or use a store-internal setter that skips IPC when
unchanged) so that setActiveSessionId is skipped if the active ID is already the
saved value.
In `@src/renderer/stores/sessionStore.ts`:
- Around line 201-204: The setter setActiveSessionId writes to persistent IPC
(window.maestro?.sessions?.setActiveSessionId) and is being called by
useSessionRestoration during startup, causing an undesired read→write round-trip
and overwriting invalid saved ids; create a separate non-persisting hydrate
path: add a new store action (e.g., hydrateActiveSessionId or
setActiveSessionIdLocal) that only updates the local state (activeSessionId and
cyclePosition) without calling window.maestro, leave setActiveSessionId for
user-driven changes that must persist, and update useSessionRestoration to call
the new non-persisting action instead of setActiveSessionId.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a02d284-2f65-456d-96d0-eca0ac34e733
📒 Files selected for processing (9)
src/__tests__/main/ipc/handlers/persistence.test.tssrc/__tests__/renderer/hooks/useSessionRestoration.test.tssrc/__tests__/setup.tssrc/main/ipc/handlers/persistence.tssrc/main/preload/settings.tssrc/main/stores/types.tssrc/renderer/global.d.tssrc/renderer/hooks/session/useSessionRestoration.tssrc/renderer/stores/sessionStore.ts
| setActiveSessionId: (id) => set({ activeSessionId: id, cyclePosition: -1 }), | ||
| setActiveSessionId: (id) => { | ||
| set({ activeSessionId: id, cyclePosition: -1 }); | ||
| window.maestro?.sessions?.setActiveSessionId(id); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'getActiveSessionId|setActiveSessionId|ipcRenderer\.invoke|ipcMain\.handle' \
src/renderer/global.d.ts \
src/main/preload/settings.ts \
src/main/ipc/handlers/persistence.tsRepository: RunMaestro/Maestro
Length of output: 7348
🏁 Script executed:
cat -n src/renderer/stores/sessionStore.ts | sed -n '195,220p'Repository: RunMaestro/Maestro
Length of output: 876
Add explicit await or error handling to these IPC calls, especially at line 209.
window.maestro.sessions.setActiveSessionId() returns Promise<void> (via ipcRenderer.invoke), but both lines 203 and 209 drop the promise without handling it. This creates unhandled rejection risk if persistence fails.
Line 209 is more problematic: the IPC call sits inside a Zustand state setter callback (set((s) => { ... })), which should be synchronous. Any async operation here will not block state updates and will cause the renderer to reflect the new activeSessionId before the main process confirms persistence. Move the IPC call outside the setter and await it before calling setActiveSessionIdInternal, or use a separate effect hook to handle persistence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/stores/sessionStore.ts` at line 203, The IPC call
window.maestro.sessions.setActiveSessionId returns a Promise and is being
invoked without await or error handling (both where it's called directly and
inside the Zustand setter), so move the async persistence outside the
synchronous set((s) => { ... }) callback: call and await
window.maestro.sessions.setActiveSessionId(id) (or handle errors with try/catch)
before calling setActiveSessionIdInternal (or, alternatively, call
setActiveSessionIdInternal only after the await succeeds), and ensure any
invocation in other places also either awaits the promise or catches/reports
errors to avoid unhandled rejections.
…ion when maestro is launched)
scriptease
force-pushed
the
persist-activesessionid
branch
from
e5d3580 to
5249abb
Compare
2 participants
Before:
Maestro would always start with session [0] from maestro-sessions.json
After:
Persist
activeSessionIdin the maestro-sessions.json and resume the last open session when Maestro launches, ignore when not found.{ "sessions": [...], "activeSessionId": "c97dca4d-172f-471e-880c-7e694c59f9f4" }Summary by CodeRabbit
New Features
Refactor
Tests