fix: align session sidebar recency with file updates#193
fix: align session sidebar recency with file updates#193Arshgill01 wants to merge 6 commits intomatt1398:mainfrom
Conversation
![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 introduces an updatedAt field to sessions, enabling the sidebar to sort and group sessions by their most recent activity. It also updates the notification listener to refresh the sidebar on any session change or addition. Review feedback recommends marking updatedAt as optional in the Session interface to accommodate legacy data and ensuring that test helpers provide numeric timestamps instead of strings to avoid runtime errors during sorting.
I am having trouble creating individual review comments. Click here to see my feedback.
src/main/types/domain.ts (93)
Since the PR notes mention avoiding cache-clearing logic and the renderer code uses nullish coalescing (updatedAt ?? createdAt) to handle potentially missing data, it would be safer to mark updatedAt as optional in the interface. This accurately reflects that sessions loaded from older persistent caches may not have this field yet, preventing potential type-safety issues if other parts of the code assume it is always present.
updatedAt?: number;
test/renderer/utils/dateGrouping.test.ts (13-14)
The Session type (defined in src/main/types/domain.ts) expects createdAt and updatedAt to be numbers (Unix timestamps in milliseconds), but this test helper is providing ISO strings. While new Date() in the grouping utility can handle both, the sorting logic in sessionSlice.ts performs subtraction, which will result in NaN at runtime if strings are passed. The mock data should use numbers to match the domain model and ensure test reliability.
createdAt: createdAt.getTime(),
updatedAt: (updatedAt ?? createdAt).getTime(),
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an optional session-level ChangesSession Update Timestamp
Possibly related PRs
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/renderer/utils/dateGrouping.test.ts (1)
16-16: ⚡ Quick win
createSessionalways populatesupdatedAt, leaving theundefinedfallback indateGrouping.tsuntested.
(updatedAt ?? createdAt).getTime()meanssession.updatedAtis always a number — the realSession.updatedAtis optional. As a result,dateGrouping.ts'supdatedAt ?? createdAtbranch is only exercised via equal values, never withupdatedAt === undefined.Changing to
updatedAt?.getTime()is backward-compatible: all existing call-sites omitupdatedAt, so they'd getundefinedand fall through tocreatedAtin the grouping logic — same bucket, same assertions. The new test explicitly passesupdatedToday, so it still produces a definedupdatedAtand continues to assertToday.♻️ Proposed fix
- updatedAt: (updatedAt ?? createdAt).getTime(), + updatedAt: updatedAt?.getTime(),🤖 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 `@test/renderer/utils/dateGrouping.test.ts` at line 16, The test helper createSession currently sets updatedAt to (updatedAt ?? createdAt).getTime(), which always produces a numeric updatedAt and prevents exercising the dateGrouping.ts branch where updatedAt is undefined; change the helper to use updatedAt?.getTime() so updatedAt remains undefined when not passed, allowing dateGrouping's (updatedAt ?? createdAt) fallback to be tested; keep any tests that need a defined updatedAt (e.g., passing updatedToday) unchanged so they still assert "Today".
🤖 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.
Nitpick comments:
In `@test/renderer/utils/dateGrouping.test.ts`:
- Line 16: The test helper createSession currently sets updatedAt to (updatedAt
?? createdAt).getTime(), which always produces a numeric updatedAt and prevents
exercising the dateGrouping.ts branch where updatedAt is undefined; change the
helper to use updatedAt?.getTime() so updatedAt remains undefined when not
passed, allowing dateGrouping's (updatedAt ?? createdAt) fallback to be tested;
keep any tests that need a defined updatedAt (e.g., passing updatedToday)
unchanged so they still assert "Today".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcfb942c-e6b0-4f0f-9b25-6f65c9bed073
📒 Files selected for processing (4)
src/main/types/domain.tssrc/main/utils/jsonl.tstest/main/services/discovery/ProjectScanner.updatedAt.test.tstest/renderer/utils/dateGrouping.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/renderer/utils/dateGrouping.test.ts (1)
120-129: ⚡ Quick winConsider adding a test for when
updatedAtis older thancreatedAt.The new test validates the primary happy path (updatedAt is newer). However, there's no test covering the inverse — when
updatedAtis provided but is older thancreatedAt. Given the implementation usesupdatedAt ?? createdAt, a stale/incorrect mtime would silently move a session to an older group. A test like the one below would document whether that behavior is intentional:📝 Suggested additional test case
+ it('should use updatedAt even when it is older than createdAt', () => { + const createdToday = new Date('2024-01-15T10:00:00Z'); // Today + const updatedOld = new Date('2024-01-01T10:00:00Z'); // Older + const sessions = [createSession('1', createdToday, updatedOld)]; + + const result = groupSessionsByDate(sessions); + + // Documents that updatedAt is unconditionally preferred when present + expect(result.Today).toHaveLength(0); + expect(result.Older).toHaveLength(1); + });🤖 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 `@test/renderer/utils/dateGrouping.test.ts` around lines 120 - 129, Add a test to cover the case where updatedAt is present but older than createdAt to document and verify behavior of groupSessionsByDate; specifically, using createSession to build a session with createdAt newer than updatedAt, call groupSessionsByDate(sessions) and assert the session falls into the group determined by createdAt (e.g., Today) and not moved to Older, so we can ensure updatedAt being older doesn't incorrectly override createdAt.
🤖 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.
Nitpick comments:
In `@test/renderer/utils/dateGrouping.test.ts`:
- Around line 120-129: Add a test to cover the case where updatedAt is present
but older than createdAt to document and verify behavior of groupSessionsByDate;
specifically, using createSession to build a session with createdAt newer than
updatedAt, call groupSessionsByDate(sessions) and assert the session falls into
the group determined by createdAt (e.g., Today) and not moved to Older, so we
can ensure updatedAt being older doesn't incorrectly override createdAt.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fba6d9db-c5b2-4ae0-9b73-a19e211cb905
📒 Files selected for processing (1)
test/renderer/utils/dateGrouping.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/utils/dateGrouping.ts (1)
31-33: ⚡ Quick winUpdate the function JSDoc to match updatedAt-aware grouping.
The implementation now groups by the most recent timestamp (
max(updatedAt, createdAt)), but the comment still says grouping is based oncreatedAtonly.Proposed doc patch
- * Sessions are categorized based on their createdAt timestamp: - * - Today: Sessions created today - * - Yesterday: Sessions created yesterday - * - Previous 7 Days: Sessions created 2-7 days ago - * - Older: Sessions created more than 7 days ago + * Sessions are categorized based on their most recent timestamp + * (max(updatedAt, createdAt), with createdAt fallback): + * - Today: Sessions updated/created today + * - Yesterday: Sessions updated/created yesterday + * - Previous 7 Days: Sessions updated/created 2-7 days ago + * - Older: Sessions updated/created more than 7 days ago🤖 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/renderer/utils/dateGrouping.ts` around lines 31 - 33, Update the function JSDoc to reflect that grouping now uses the most recent timestamp between updatedAt and createdAt (i.e., timestamp is computed via Math.max(session.updatedAt ?? session.createdAt, session.createdAt)), rather than only createdAt; mention that the variable timestamp and resulting sessionDate are derived from the later of updatedAt and createdAt so groups won't move backwards in time.
🤖 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.
Nitpick comments:
In `@src/renderer/utils/dateGrouping.ts`:
- Around line 31-33: Update the function JSDoc to reflect that grouping now uses
the most recent timestamp between updatedAt and createdAt (i.e., timestamp is
computed via Math.max(session.updatedAt ?? session.createdAt,
session.createdAt)), rather than only createdAt; mention that the variable
timestamp and resulting sessionDate are derived from the later of updatedAt and
createdAt so groups won't move backwards in time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90950b2c-77f6-49e3-821f-a310c6b8c3c7
📒 Files selected for processing (5)
src/renderer/components/sidebar/SessionItem.tsxsrc/renderer/store/slices/sessionSlice.tssrc/renderer/utils/dateGrouping.tstest-debug.tstest/renderer/utils/dateGrouping.test.ts
1 participant
Summary
Fixes #181 by aligning sidebar session recency with the actual JSONL file update time.
Previously, project recency and sidebar session recency used different timestamps:
.jsonlfile mtime.createdAt, which represents the session’s first prompt / creation time.That meant a long-lived session started weeks ago but updated today could make the project appear fresh while the session still appeared old in the sidebar.
Changes
Session.updatedAtwhile preservingcreatedAt.updatedAtfrom the session JSONL file mtime.updatedAt ?? createdAt.updatedAt ?? createdAt.updatedAt ?? createdAt.Validation
pnpm typecheck— passpnpm test— pass, 49 files / 696 testspnpm lint— passpnpm build— passNotes
This intentionally avoids cache-clearing logic. The bug was caused by a timestamp mismatch between project recency and sidebar session recency, not by stale parsed session caches. Sidebar refreshes are still coalesced through the existing project refresh scheduler, so repeated JSONL writes do not trigger immediate one-refresh-per-event behavior.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests