Conversation
WalkthroughThe changes refactor project discovery and session indexing in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@server/projects.js`:
- Around line 395-398: The early-exit in the loop that checks "if (sessions.size
>= targetSessionCount && !truncatedReadDetected) { break; }" can make total and
hasMore under-report because remaining files aren't scanned; update the logic in
the function that computes total/hasMore (referencing sessions,
targetSessionCount, truncatedReadDetected, total, hasMore, scannedFiles,
filesWithStats.length) to either document that total is a lower bound when the
loop breaks early or explicitly set hasMore = true whenever scannedFiles <
filesWithStats.length (and truncatedReadDetected is false) so consumers know
more sessions may exist; ensure this change is applied where total and hasMore
are assigned after the loop.
🧹 Nitpick comments (5)
server/projects.js (5)
356-363: Avoid repeatedDateconstruction in a hot inner loop.In a PR focused on performance, constructing two
new Date()objects per entry with a timestamp is wasteful. ISO 8601 strings are lexicographically comparable, so a direct string comparison suffices here.♻️ Proposed fix
if (entry.timestamp) { const timestamp = new Date(entry.timestamp); if (!Number.isNaN(timestamp.getTime())) { const currentTime = timestamp.toISOString(); - if (!session.lastActivity || new Date(currentTime) > new Date(session.lastActivity)) { + if (!session.lastActivity || currentTime > session.lastActivity) { session.lastActivity = currentTime; } } }
234-256: Duplicated system-message detection logic.
isSystemUserMessage/isSystemAssistantMessage(Lines 234-256) duplicate the inline checks inparseJsonlSessions(Lines 1062-1102). Consider reusing the extracted helpers inparseJsonlSessionsto keep the filter predicates in a single place.Also applies to: 1062-1102
446-462: Triplicated recursive JSONL file finder.
findJsonlFilesRecursiveduplicates identical inline functions ingetCodexSessions(Line 1543) anddeleteCodexSession(Line 1914). Consider reusing this top-level helper in both places.
760-813:getSessionsPreview+getCursorSessionsare sequential per project.Each project awaits
getSessionsPreview(I/O-heavy tail reads) sequentially. For users with many projects, consider batching with bounded concurrency (e.g.,Promise.allover small chunks) to overlap I/O. Not a blocker given the currentPROJECT_PREVIEW_MAX_FILES = 12cap, but worth considering if startup remains slow for large project counts.
1136-1143: Dead debug code inparseJsonlSessions.The
Math.random() < 0.01branch (Line 1140) and the emptyif (shouldFilter)block (Lines 1137-1138) appear to be leftover debug scaffolding with no actual logging statements. This predates the current PR but is worth cleaning up.♻️ Suggested cleanup
const filteredSessions = allSessions.filter(session => { - const shouldFilter = session.summary.startsWith('{ "'); - if (shouldFilter) { - } - // Log a sample of summaries to debug - if (Math.random() < 0.01) { // Log 1% of sessions - } - return !shouldFilter; + return !session.summary.startsWith('{ "'); });
| if (sessions.size >= targetSessionCount && !truncatedReadDetected) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Early-exit condition may under-report total and hasMore.
When sessions.size >= targetSessionCount and no truncation occurred, the loop breaks before scanning remaining files. This means total (Line 419) reflects only sessions found so far, and hasMore (Line 422) could be false even though un-scanned files may contain additional sessions. This is probably acceptable for a preview, but the consumer should be aware that total is a lower bound when not all files are scanned.
Consider documenting this or always setting hasMore = true when scannedFiles < filesWithStats.length.
♻️ Suggested adjustment
- const scannedAllFiles = scannedFiles >= filesWithStats.length && !truncatedReadDetected;
- const hasMore = scannedAllFiles ? (offset + limit < total) : true;
+ const scannedAllFiles = scannedFiles >= filesWithStats.length && !truncatedReadDetected;
+ const hasMore = !scannedAllFiles || (offset + limit < total);(This is functionally equivalent to the current code—just confirming the intent is correct and making it slightly more readable.)
🤖 Prompt for AI Agents
In `@server/projects.js` around lines 395 - 398, The early-exit in the loop that
checks "if (sessions.size >= targetSessionCount && !truncatedReadDetected) {
break; }" can make total and hasMore under-report because remaining files aren't
scanned; update the logic in the function that computes total/hasMore
(referencing sessions, targetSessionCount, truncatedReadDetected, total,
hasMore, scannedFiles, filesWithStats.length) to either document that total is a
lower bound when the loop breaks early or explicitly set hasMore = true whenever
scannedFiles < filesWithStats.length (and truncatedReadDetected is false) so
consumers know more sessions may exist; ensure this change is applied where
total and hasMore are assigned after the loop.
GizAI
changed the title
1 participant
Superseded by #378:
#378