Repository navigation
perf: short-circuit no-op session refreshes via fingerprint match - #187
Conversation
Fixes #186. Long sessions ran at ~10fps after v0.4.13 because PR #183 shrank the refresh debounce ceiling from 60s to 5s. The shorter window exposed an existing architectural flaw: every `getSessionDetail` IPC returned the full payload, with the renderer-side fingerprint check happening *after* the expensive serialization. Frequent refreshes paid near-real-refresh cost even when nothing on disk had changed. Move the fingerprint check to the IPC boundary. The renderer caches the opaque file-state fingerprint (mtimeMs+size) returned by main and passes it back on the next call. When it matches, the handler short-circuits to a tiny `{ unchanged: true }` sentinel before cache lookup or parsing. The renderer detects the sentinel and exits without transforming or re-rendering — bounded to one stat() + a sub-100B IPC payload. With no-op cost effectively eliminated, the adaptive debounce can be restored to sane values (1s ceiling for >500 groups, 500ms for >200, 300ms for >100), preserving the streaming-responsiveness intent of PR #183 without the perf cost. Backwards compatible: knownFingerprint is optional, so initial loads and export still receive a full SessionDetail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a fingerprint-based short-circuiting mechanism for session detail fetching to optimize performance by skipping IPC serialization and re-renders when the session file is unchanged. Key changes include the introduction of a SessionDetailResponse union type, fingerprint tracking in the renderer store, and updates to the IPC handler. Feedback highlights an opportunity to use the new isSessionDetailUnchanged type guard for better consistency in the main process and identifies a potential memory leak where module-level fingerprint maps are never cleared.
| // below is purely for type safety. | ||
| const detail = await handleGetSessionDetail(_event, projectId, sessionId); | ||
| if (!detail) { | ||
| if (!detail || 'unchanged' in detail) { |
There was a problem hiding this comment.
Consider using the newly introduced isSessionDetailUnchanged type guard here for consistency and better type safety, rather than checking for the 'unchanged' property manually. Note that you will need to import it from @shared/utils/sessionDetailResponse.
| * on every no-op refresh. Distinct from `sessionChunkFingerprint`, which is | ||
| * a renderer-local content fingerprint used as a second-line guard. | ||
| */ | ||
| const sessionFileFingerprint = new Map<string, string>(); |
There was a problem hiding this comment.
The sessionFileFingerprint map (along with sessionChunkFingerprint, sessionRefreshGeneration, etc.) is a module-level constant that is never cleared. In a long-running application session where a user opens and closes many different projects and sessions, these maps will grow indefinitely, leading to a memory leak. Consider cleaning up these entries in cleanupTabSessionData when no other tabs are viewing the associated session.
There was a problem hiding this comment.
Pull request overview
This PR addresses the post-v0.4.13 performance regression by moving the “unchanged session” fingerprint check to the main-process IPC boundary, allowing getSessionDetail to return a tiny sentinel response when the underlying session file hasn’t changed—avoiding expensive serialization + renderer transforms on no-op refreshes.
Changes:
- Add a
knownFingerprintparam togetSessionDetailand return{ unchanged: true, fingerprint }when the file-state fingerprint matches. - Cache and round-trip the file fingerprint in the renderer to enable the short-circuit fast path; restore more aggressive adaptive refresh debounce ceilings.
- Introduce shared IPC-contract helpers + tests to keep the sentinel discriminator consistent.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/shared/utils/sessionDetailResponse.test.ts | Adds IPC contract tests for identifying the “unchanged” sentinel response. |
| src/shared/utils/sessionDetailResponse.ts | Adds shared type-guard helper for the getSessionDetail response union. |
| src/shared/types/api.ts | Updates ElectronAPI.getSessionDetail signature + return type to the new discriminated union. |
| src/renderer/store/slices/sessionDetailSlice.ts | Caches file fingerprints and short-circuits refresh work when main returns the sentinel. |
| src/renderer/store/index.ts | Restores tighter adaptive debounce ceilings now that no-op refreshes are cheap. |
| src/renderer/components/layout/MoreMenu.tsx | Updates export flow to handle SessionDetailResponse vs sentinel. |
| src/preload/index.ts | Plumbs optional knownFingerprint through the preload IPC bridge. |
| src/main/types/chunks.ts | Adds fingerprint to SessionDetail and introduces SessionDetailUnchanged/SessionDetailResponse. |
| src/main/ipc/sessions.ts | Implements the IPC short-circuit based on file-state fingerprint match and includes fingerprint in full responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| !!response && | ||
| typeof response === 'object' && | ||
| 'unchanged' in response && | ||
| (response as { unchanged: unknown }).unchanged === true | ||
| ); |
There was a problem hiding this comment.
isSessionDetailUnchanged currently only checks for the literal unchanged: true flag. Since the type guard is used to narrow callers to SessionDetailUnchanged (and the docs/tests imply fingerprint is guaranteed), it should also validate that fingerprint exists and is a string; otherwise callers may get an unsound narrowing if a malformed IPC payload ever omits/changes fingerprint.
| // No knownFingerprint passed, so the response is always SessionDetail | null | ||
| // at runtime. Narrow defensively for type safety. | ||
| void api | ||
| .getSessionDetail(projectId, sessionId) | ||
| .then((detail) => { | ||
| if (detail) triggerDownload(detail, format); | ||
| .then((response) => { | ||
| if (response && !('unchanged' in response)) { | ||
| triggerDownload(response, format); | ||
| } |
There was a problem hiding this comment.
This export path is manually checking !('unchanged' in response) instead of using the shared isSessionDetailUnchanged helper. Since the whole point of the helper/test is to centralize the IPC discriminator, this duplicates the contract in a second place and can drift if the sentinel shape ever changes. Consider using isSessionDetailUnchanged(response) here as well (and keep the existing null guard).
📝 WalkthroughWalkthroughThis PR implements session-detail fingerprinting to optimize refresh behavior. It adds file-state change detection via optional Changes
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/components/layout/MoreMenu.tsx (1)
81-88: Consider using the shared type guard for consistency.The inline check
!('unchanged' in response)works correctly, but other places in this PR (e.g.,sessionDetailSlice.ts) use the sharedisSessionDetailUnchanged()type guard. Using the shared utility would be slightly more consistent:+import { isSessionDetailUnchanged } from '@shared/utils/sessionDetailResponse'; // ... .then((response) => { - if (response && !('unchanged' in response)) { + if (response && !isSessionDetailUnchanged(response)) { triggerDownload(response, format); } })That said, the current approach is valid defense-in-depth since no
knownFingerprintis passed, so the sentinel cannot be returned at runtime. The comment accurately explains this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/layout/MoreMenu.tsx` around lines 81 - 88, Replace the inline sentinel check with the shared type guard for consistency: in the promise handler for api.getSessionDetail(projectId, sessionId) (where response is checked before calling triggerDownload), use the isSessionDetailUnchanged(response) guard instead of "!('unchanged' in response)" to determine if response is a SessionDetail; keep the existing null/undefined defensive check and ensure you import or reference isSessionDetailUnchanged from the same module used elsewhere (e.g., sessionDetailSlice.ts).src/main/ipc/sessions.ts (1)
389-393: Prefer a named type guard for the unchanged sentinel.
'unchanged' in detailis inline runtime type checking. Per repo rules, this should be anisXxxguard so the narrowing stays explicit and reusable.As per coding guidelines, "Use TypeScript type guards with isXxx naming convention for runtime type checking".Minimal refactor
+function isUnchangedSessionDetailResponse( + detail: SessionDetailResponse | null +): detail is Extract<SessionDetailResponse, { unchanged: true }> { + return detail !== null && typeof detail === 'object' && 'unchanged' in detail; +} + async function handleGetWaterfallData( _event: IpcMainInvokeEvent, projectId: string, sessionId: string ): Promise<WaterfallData | null> { try { @@ - if (!detail || 'unchanged' in detail) { + if (!detail || isUnchangedSessionDetailResponse(detail)) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/sessions.ts` around lines 389 - 393, The inline runtime check "'unchanged' in detail" should be replaced with a named TypeScript type guard to follow repo conventions; add an exported guard function (e.g. isSessionDetailUnchanged or isUnchangedDetail) that accepts the handleGetSessionDetail return type and returns boolean by checking the 'unchanged' sentinel, then use that guard in the conditional (replace "'unchanged' in detail" with isSessionDetailUnchanged(detail)) so the narrowing is explicit and reusable across code that inspects the detail variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/ipc/sessions.ts`:
- Around line 389-393: The inline runtime check "'unchanged' in detail" should
be replaced with a named TypeScript type guard to follow repo conventions; add
an exported guard function (e.g. isSessionDetailUnchanged or isUnchangedDetail)
that accepts the handleGetSessionDetail return type and returns boolean by
checking the 'unchanged' sentinel, then use that guard in the conditional
(replace "'unchanged' in detail" with isSessionDetailUnchanged(detail)) so the
narrowing is explicit and reusable across code that inspects the detail
variable.
In `@src/renderer/components/layout/MoreMenu.tsx`:
- Around line 81-88: Replace the inline sentinel check with the shared type
guard for consistency: in the promise handler for
api.getSessionDetail(projectId, sessionId) (where response is checked before
calling triggerDownload), use the isSessionDetailUnchanged(response) guard
instead of "!('unchanged' in response)" to determine if response is a
SessionDetail; keep the existing null/undefined defensive check and ensure you
import or reference isSessionDetailUnchanged from the same module used elsewhere
(e.g., sessionDetailSlice.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5f07907-65fc-4510-925a-8ae7c04dcf89
📒 Files selected for processing (9)
src/main/ipc/sessions.tssrc/main/types/chunks.tssrc/preload/index.tssrc/renderer/components/layout/MoreMenu.tsxsrc/renderer/store/index.tssrc/renderer/store/slices/sessionDetailSlice.tssrc/shared/types/api.tssrc/shared/utils/sessionDetailResponse.tstest/shared/utils/sessionDetailResponse.test.ts
Fixes #186. Long sessions ran at ~10fps after v0.4.13 because PR #183 shrank the refresh debounce ceiling from 60s to 5s. The shorter window exposed an existing architectural flaw: every
getSessionDetailIPC returned the full payload, with the renderer-side fingerprint check happening after the expensive serialization. Frequent refreshes paid near-real-refresh cost even when nothing on disk had changed.Move the fingerprint check to the IPC boundary. The renderer caches the opaque file-state fingerprint (mtimeMs+size) returned by main and passes it back on the next call. When it matches, the handler short-circuits to a tiny
{ unchanged: true }sentinel before cache lookup or parsing. The renderer detects the sentinel and exits without transforming or re-rendering — bounded to one stat() + a sub-100B IPC payload.With no-op cost effectively eliminated, the adaptive debounce can be restored to sane values (1s ceiling for >500 groups, 500ms for >200, 300ms for >100), preserving the streaming-responsiveness intent of PR #183 without the perf cost.
Backwards compatible: knownFingerprint is optional, so initial loads and export still receive a full SessionDetail.
Summary by CodeRabbit
Performance Improvements
Bug Fixes