perf: speed up project/session scanning I/O#378
Conversation
WalkthroughAdds JSONL tail reading, recursive discovery and Codex session indexing on the server; exposes new session/index helpers. Frontend gains hydration-aware project/session merging, URL-based one-time hydration attempts, Sidebar hydration callbacks, and new client API helpers for cursor/codex sessions. Changes
Sequence Diagram(s)sequenceDiagram
participant Frontend
participant Server
participant FS as FileSystem
participant CodexIndex
Frontend->>Server: GET /api/projects
Server->>CodexIndex: buildCodexSessionsIndex(limitPerProject)
CodexIndex->>FS: findJsonlFilesRecursive(projectPaths)
loop per file
CodexIndex->>FS: readJsonlTailLines(filePath, maxBytes, maxLines)
FS-->>CodexIndex: tail lines
CodexIndex-->>CodexIndex: parse/aggregate session previews
end
CodexIndex-->>Server: codexSessionsIndex
Server-->>Frontend: projects payload (with preview/index hints)
Frontend->>Server: POST /api/projects/:id/hydrate (onProjectHydrate)
Server->>CodexIndex: getCodexSessionsForProjectFromIndex(projectPath, limit)
CodexIndex-->>Server: sessions list
Server-->>Frontend: hydrated session data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/projects.js (3)
1229-1244:⚠️ Potential issue | 🔴 CriticalClearing a display name deletes the entire project config, including
manuallyAddedandoriginalPath.When
newDisplayNameis empty,delete config[projectName](line 1234) removes the whole config object. For manually-added projects this wipesmanuallyAddedandoriginalPath, effectively unregistering them. Only thedisplayNamekey should be removed.🐛 Proposed fix
async function renameProject(projectName, newDisplayName) { const config = await loadProjectConfig(); if (!newDisplayName || newDisplayName.trim() === '') { - // Remove custom name if empty, will fall back to auto-generated - delete config[projectName]; + // Remove custom name if empty, will fall back to auto-generated + if (config[projectName]) { + delete config[projectName].displayName; + } } else { - // Set custom display name - config[projectName] = { - displayName: newDisplayName.trim() - }; + // Set custom display name, preserving other config + config[projectName] = { + ...config[projectName], + displayName: newDisplayName.trim() + }; } await saveProjectConfig(config); return true; }
1400-1412:⚠️ Potential issue | 🟡 Minor
addProjectManuallyreturns a project object missingcodexSessions,taskmaster, andsessionMetafields.
getProjectsnow returns project objects withcodexSessions,taskmaster, andsessionMetaproperties. The object returned byaddProjectManually(used when a user adds a project via UI) lacks these fields, which can causeundefinedproperty access in frontend components that render the response directly.🛡️ Proposed fix — align return shape
return { name: projectName, path: absolutePath, fullPath: absolutePath, displayName: displayName || await generateDisplayName(projectName, absolutePath), isManuallyAdded: true, sessions: [], - cursorSessions: [] + cursorSessions: [], + codexSessions: [], + sessionMeta: { hasMore: false, total: 0 }, + taskmaster: { + hasTaskmaster: false, + hasEssentialFiles: false, + metadata: null, + status: 'not-configured' + } };
1450-1511:⚠️ Potential issue | 🟠 MajorSQLite connection is leaked when a query throws before
db.close().If any of the queries between
open(line 1450) anddb.close()(line 1485) throw, execution jumps to the outercatchat line 1509 without closing the database. Over many sessions, this leaks file descriptors.🐛 Proposed fix — wrap in try/finally
// Open SQLite database const db = await open({ filename: storeDbPath, driver: sqlite3.Database, mode: sqlite3.OPEN_READONLY }); - - // Get metadata from meta table - const metaRows = await db.all(` - SELECT key, value FROM meta - `); - - // Parse metadata - ... - - // Get message count - const messageCountResult = await db.get(` - SELECT COUNT(*) as count FROM blobs - `); - - await db.close(); + + try { + // Get metadata from meta table + const metaRows = await db.all(` + SELECT key, value FROM meta + `); + + // ... (parse metadata, get message count — unchanged) ... + + const messageCountResult = await db.get(` + SELECT COUNT(*) as count FROM blobs + `); + + // Extract session info & push to sessions array here + // (move the push logic inside this try block) + } finally { + await db.close(); + }
🤖 Fix all issues with AI agents
In `@server/projects.js`:
- Around line 375-392: getSessionsPreview currently increments
session.messageCount only for 'user' and 'assistant' roles which differs from
parseJsonlSessions (which increments for every entry with a sessionId), causing
inconsistent counts; update getSessionsPreview (or alternatively rename the
preview field) so the semantics match parseJsonlSessions: either increment
session.messageCount for every entry with a sessionId (including system/summary
entries) to mirror parseJsonlSessions, or rename session.messageCount in
getSessionsPreview to something like userAssistantCount and update consumers;
refer to getSessionsPreview, parseJsonlSessions, and the
session.messageCount/userAssistantCount identifiers when making the change.
- Line 758: The promise returned by buildCodexSessionsIndex is not guarded, so
codexSessionsIndexPromise can reject and abort getProjects; wrap the call site
by attaching a .catch(...) to codexSessionsIndexPromise (where const
codexSessionsIndexPromise = buildCodexSessionsIndex(5)) so it always resolves to
a safe fallback (e.g., null or an empty index) on error; ensure downstream
awaits (the places that await codexSessionsIndexPromise in getProjects) handle
that fallback value gracefully.
🧹 Nitpick comments (6)
server/projects.js (6)
356-364: RedundantDateobject allocations in the inner loop.On every JSONL entry with a timestamp, two
new Date()objects are created for comparison (line 360). Since both values are ISO 8601 strings, a simple string comparison (currentTime > session.lastActivity) is sufficient and avoids allocation churn across thousands of lines.♻️ Suggested diff
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; } } }
446-462: Duplicate recursive JSONL file finders can be consolidated.
findJsonlFilesRecursiveduplicates the localfindJsonlFilesclosures insidegetCodexSessions(line 1543) anddeleteCodexSession(line 1914). Consider replacing those inline closures with calls to this shared utility to reduce duplication.
508-513:lastActivityis already aDate; wrapping it again is redundant.At line 492,
lastActivityis assigned as aDateobject. The sort comparator at line 510 wraps it innew Date(...)again unnecessarily.♻️ Suggested diff
for (const [cwd, sessions] of sessionsByCwd.entries()) { const sortedSessions = sessions - .sort((a, b) => new Date(b.lastActivity) - new Date(a.lastActivity)) + .sort((a, b) => b.lastActivity - a.lastActivity) .slice(0, limitPerProject); sessionsByCwd.set(cwd, sortedSessions); }
518-537: PreferMap.getfor O(1) lookup before falling back to iteration.Both keys and the lookup value go through
normalizeCodexPath, so a directMap.get(normalizedProjectPath)will match in the common case. The current implementation iterates the entire index for every project, turning the overall complexity to O(projects × index entries).♻️ Suggested diff
function getCodexSessionsForProjectFromIndex(codexSessionsIndex, projectPath, limit = 5) { const normalizedProjectPath = normalizeCodexPath(projectPath); if (!normalizedProjectPath || codexSessionsIndex.size === 0) { return []; } - const matchedSessions = []; - for (const [sessionCwd, sessions] of codexSessionsIndex.entries()) { - try { - if (sessionCwd === normalizedProjectPath || path.relative(sessionCwd, normalizedProjectPath) === '') { - matchedSessions.push(...sessions); - } - } catch (_) { - // Ignore cross-device / invalid relative path comparisons - } - } + // Fast path: direct map lookup + const direct = codexSessionsIndex.get(normalizedProjectPath); + if (direct) { + const sorted = [...direct].sort((a, b) => b.lastActivity - a.lastActivity); + return limit > 0 ? sorted.slice(0, limit) : sorted; + } - matchedSessions.sort((a, b) => new Date(b.lastActivity) - new Date(a.lastActivity)); + // Slow path: fallback for normalization edge cases + const matchedSessions = []; + for (const [sessionCwd, sessions] of codexSessionsIndex.entries()) { + try { + if (path.relative(sessionCwd, normalizedProjectPath) === '') { + matchedSessions.push(...sessions); + } + } catch (_) {} + } + matchedSessions.sort((a, b) => b.lastActivity - a.lastActivity); return limit > 0 ? matchedSessions.slice(0, limit) : matchedSessions; }
760-813: Sequential per-project I/O limits the benefit of the tail-read optimization.Each project in the loop sequentially calls
getSessionsPreview(which does tail I/O),generateDisplayName(which may readpackage.json),getCursorSessions(SQLite), anddetectTaskMasterFolder(filesystem). For repositories with many projects, this serialization still accumulates significant latency.Consider batching projects in small parallel groups (e.g.,
Promise.allover chunks of 4–6 projects) to exploit I/O concurrency without overwhelming the filesystem.
1134-1143: Dead debug blocks left behind inparseJsonlSessions.Lines 1137–1138 and 1140–1141 are empty conditional blocks (the
console.logcalls were likely stripped). These are noise and can be removed.♻️ 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('{ "'); });
|
Addressed CodeRabbit's actionable issues in commit c19c810 (on top of 602df0b baseline):
Validation run:
@coderabbitai please re-review the latest commit. |
|
✅ I appreciate the validation run with typecheck and build. I'll review the latest commit now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/App.jsx (1)
526-549:⚠️ Potential issue | 🟠 MajorStale closure: merge computed outside
setProjectscallback.
mergedFreshProjectsat line 531 is computed from theprojectsclosure value, but thesetProjectsupdater (line 534) receives the real latestprevProjects. If a concurrent update (e.g., from WebSocket or hydration) changesprojectsbetween line 531 and the updater execution, the merge is based on stale data, potentially dropping hydration state.Move the merge inside the functional updater to use the latest state:
🐛 Proposed fix
const handleSidebarRefresh = async () => { try { const response = await api.projects(); const freshProjects = await response.json(); - const mergedFreshProjects = mergeHydratedProjectData(projects, freshProjects); setProjects(prevProjects => { + const mergedFreshProjects = mergeHydratedProjectData(prevProjects, freshProjects); const hasChanges = mergedFreshProjects.some((newProject, index) => { // ... }) || mergedFreshProjects.length !== prevProjects.length; return hasChanges ? mergedFreshProjects : prevProjects; }); if (selectedProject) { + // Re-compute merge for selected project lookup + const mergedFreshProjects = mergeHydratedProjectData(projects, freshProjects); const refreshedProject = mergedFreshProjects.find(p => p.name === selectedProject.name); // ...
🤖 Fix all issues with AI agents
In `@server/projects.js`:
- Around line 799-831: The manual project builder is setting
sessionMeta.hasMore: true for entries added from manualProjects which causes the
UI to attempt loading non-existent sessions; change the default to
sessionMeta.hasMore: false (or remove the hardcoded hasMore and let the existing
hydration logic update it) in the project object created inside the
manualProjects loop (the block that calls emitProjectProgress and
generateDisplayName), so cursorSessions/codexSessions/sessions hydration can
determine the real value.
- Around line 758-764: The code currently calls getSessionsPreview(...) and
getSessions(...) in parallel for each project, doubling disk I/O because
getSessions still parses full JSONL via parseJsonlSessions; change this to avoid
calling getSessions at startup: use the metadata already computed by
getSessionsPreview (it returns hasMore/total) to populate hasMore/total for the
project (referencing getSessionsPreview and parseJsonlSessions), and remove or
defer the getSessions(...) call so full parsing only runs when the user expands
a project (i.e., call getSessions inside the expansion handler instead of in the
projects loop where directories and emitProjectProgress are used).
- Around line 208-215: normalizeCodexPath currently returns '' for
non-string/falsy inputs but falls through to path.normalize for strings, which
turns an input like '\\\\?\\' into '.' and breaks callers that do falsy checks
(e.g., where normalizedCwd and normalizedProjectPath are used). Change
normalizeCodexPath so that after stripping a leading '\\\\?\\' you explicitly
return '' when cleanedPath === '' (preserving the original falsy semantic for
inputs that become empty), otherwise return path.normalize(cleanedPath); keep
the initial non-string/falsy guard and the unique function name
normalizeCodexPath to locate the change.
- Around line 518-537: The two functions buildCodexSessionsIndex and
getCodexSessionsForProjectFromIndex are dead code (not exported or referenced)
and should be removed; delete their full definitions from the module (remove the
function declarations and any associated unused imports/variables they rely on,
e.g., normalizeCodexPath and any local use of codexSessionsIndex) and run the
test/lint step to ensure no remaining references; if these are intended for
future use, instead add a clear TODO comment and export them from the module so
callers can use them.
- Around line 446-462: The recursive file-walk functions are following symlinks
and can loop; update findJsonlFilesRecursive and the two inline findJsonlFiles
used by getCodexSessions and deleteCodexSession to skip symbolic links by
checking !entry.isSymbolicLink() before recursing into directories (i.e., only
recurse when entry.isDirectory() && !entry.isSymbolicLink()); also consider
extracting a single shared utility (e.g., findJsonlFiles) and replacing the
three implementations with that to ensure consistent symlink protection across
callers.
In `@src/App.jsx`:
- Line 322: The effect is re-triggering infinitely because projects is in the
dependency array and setProjects(…) creates a new array reference (via
mergeHydratedProjectData.map), so update the effect to stop depending on
projects/mergeHydratedProjectData: either (preferred) use a functional state
update like setProjects(prev => mergeHydratedProjectData(prev, ...)) and remove
projects and mergeHydratedProjectData from the dependency array, or implement a
last-processed message id/ref (e.g., lastProcessedMessageIdRef) to skip
reprocessing the same latestMessage; also refactor any isUpdateAdditive and
selectedProject updates to read from refs or the functional updater so they no
longer require projects in the deps, leaving only stable deps (latestMessage,
selectedSession, activeSessions, etc.) in the array.
In `@src/components/Sidebar.jsx`:
- Around line 1063-1067: Replace the hardcoded English pluralization in the
Sidebar JSX (the IIFE that calls getProjectSessionCountInfo(project)) with the
i18n t() call so translations handle singular/plural forms; call
t('project.session.count', { label: countInfo.label, count: countInfo.numeric })
(or your app's pluralization key) instead of manually appending 's', and add
corresponding keys to your locale files (e.g., "project.session.count" with
plural forms like "project.session.count" and "project.session.count_plural") so
non-English locales receive correct pluralization.
🧹 Nitpick comments (5)
server/projects.js (1)
414-422:legacyTotalCaplogic is overly complex and theMath.maxis a no-op.
Math.max(limit + offset, (limit + offset) * 2)always equals(limit + offset) * 2sincelimitandoffsetare non-negative. The surrounding heuristic (lines 416-420) to inflateeffectiveTotalwhen a truncated read is detected is fragile and difficult to reason about — it can report more sessions than actually exist.Consider simplifying or adding a brief comment explaining the exact contract this total provides to consumers.
src/App.jsx (2)
142-169: Deeply nested ternaries in merge logic harm readability.Lines 160-166 use triple-nested ternaries that are difficult to follow. Consider extracting a small helper function (e.g.,
pickSessions(hydrated, current, incoming)) to make the merge intent explicit.♻️ Example helper extraction
+ const pickMerged = (isHydrated, currentHydrated, currentVal, incomingVal) => + isHydrated + ? (currentHydrated ? currentVal : (incomingVal || [])) + : (incomingVal || []); + return { ...incomingProject, - sessions: sessionsHydrated ? (currentProject.sessionsHydrated ? currentProject.sessions : (incomingProject.sessions || [])) : (incomingProject.sessions || []), - sessionMeta: sessionsHydrated ? (currentProject.sessionsHydrated ? currentProject.sessionMeta : incomingProject.sessionMeta) : incomingProject.sessionMeta, + sessions: pickMerged(sessionsHydrated, currentProject.sessionsHydrated, currentProject.sessions, incomingProject.sessions), + sessionMeta: pickMerged(sessionsHydrated, currentProject.sessionsHydrated, currentProject.sessionMeta, incomingProject.sessionMeta), sessionsHydrated, - cursorSessions: cursorSessionsHydrated ? (currentProject.cursorSessionsHydrated ? currentProject.cursorSessions : (incomingProject.cursorSessions || [])) : (incomingProject.cursorSessions || []), + cursorSessions: pickMerged(cursorSessionsHydrated, currentProject.cursorSessionsHydrated, currentProject.cursorSessions, incomingProject.cursorSessions), cursorSessionsHydrated, - codexSessions: codexSessionsHydrated ? (currentProject.codexSessionsHydrated ? currentProject.codexSessions : (incomingProject.codexSessions || [])) : (incomingProject.codexSessions || []), + codexSessions: pickMerged(codexSessionsHydrated, currentProject.codexSessionsHydrated, currentProject.codexSessions, incomingProject.codexSessions), codexSessionsHydrated };
406-447: Lazy cursor session resolution iterates all projects sequentially — consider scoping the search.When a deep-linked
sessionIdisn't found in already-loaded data, this code sequentially callsapi.cursorSessions(...)for every project (line 413-445). With many projects, this can be slow and generates N API requests. Consider limiting the search to the most likely project (e.g., from URL context) or parallelizing withPromise.allSettled.src/components/Sidebar.jsx (2)
217-294: Loading spinner clears before cursor/codex hydration completes.
setLoadingSessionsis reset tofalsein thefinallyblock (line 278), but the cursor/codexPromise.allSettledat line 281 hasn't resolved yet. Users see the loading indicator disappear, then cursor/codex sessions appear shortly after with no visual feedback.Consider either:
- Moving the
finallyblock into thePromise.allSettled.then()callback, or- Using a separate loading indicator for cursor/codex sessions.
143-150: MissinghydrateProjectSessionsin useEffect dependency array.The effect calls
hydrateProjectSessions(line 147) but it's not listed in the dependency array (line 150:[selectedSession, selectedProject, initialSessionsLoaded]). While this works becausehydrateProjectSessionsis redefined on every render (not wrapped inuseCallback), the exhaustive-deps lint rule would flag this. SincehydrateProjectSessionsdepends onloadingSessions,projects, andonProjectHydrate, adding it to deps could cause unwanted re-triggers. Consider wrapping it inuseCallbackor using a ref.
server/projects.js
Outdated
| async function findJsonlFilesRecursive(dir) { | ||
| const files = []; | ||
| try { | ||
| const entries = await fs.readdir(dir, { withFileTypes: true }); | ||
| for (const entry of entries) { | ||
| const fullPath = path.join(dir, entry.name); | ||
| if (entry.isDirectory()) { | ||
| files.push(...await findJsonlFilesRecursive(fullPath)); | ||
| } else if (entry.name.endsWith('.jsonl')) { | ||
| files.push(fullPath); | ||
| } | ||
| } | ||
| } catch (_) { | ||
| // Skip directories we cannot read | ||
| } | ||
| return files; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the exact code at the mentioned lines
wc -l server/projects.js
echo "---"
# Get the findJsonlFilesRecursive function
sed -n '446,462p' server/projects.jsRepository: siteboon/claudecodeui
Length of output: 543
🏁 Script executed:
#!/bin/bash
# Check lines 1512 and 1883 for similar patterns
echo "=== Around line 1512 ==="
sed -n '1505,1525p' server/projects.js
echo ""
echo "=== Around line 1883 ==="
sed -n '1876,1896p' server/projects.jsRepository: siteboon/claudecodeui
Length of output: 1100
🏁 Script executed:
#!/bin/bash
# Search for all isDirectory() calls to understand the pattern
rg -n 'isDirectory' server/projects.jsRepository: siteboon/claudecodeui
Length of output: 330
🏁 Script executed:
#!/bin/bash
# Look for how findJsonlFilesRecursive is called and what dir parameter is passed
rg -B3 -A3 'findJsonlFilesRecursive' server/projects.js | head -60Repository: siteboon/claudecodeui
Length of output: 638
🏁 Script executed:
#!/bin/bash
# Get full function context for the isDirectory calls at 1532 and 1903
echo "=== Function containing line 1532 ==="
sed -n '1517,1560p' server/projects.js
echo ""
echo "=== Function containing line 1903 ==="
sed -n '1892,1935p' server/projects.jsRepository: siteboon/claudecodeui
Length of output: 3068
🏁 Script executed:
#!/bin/bash
# Check if there are inline findJsonlFiles functions or recursive patterns
rg -B5 -A10 'function.*find.*jsonl|const.*find.*jsonl.*=' server/projects.js | head -80Repository: siteboon/claudecodeui
Length of output: 47
Unbounded recursion without symlink protection in three locations.
findJsonlFilesRecursive (line 446) and two inline findJsonlFiles functions in getCodexSessions (line 1532) and deleteCodexSession (line 1903) follow directories without checking for symbolic links. A symlink cycle under ~/.codex/sessions would cause infinite recursion and stack overflow. Add !entry.isSymbolicLink() checks to skip symlinks in all three functions.
🛡️ Proposed fix
Change the directory check in all three recursive functions from:
- if (entry.isDirectory()) {
+ if (entry.isDirectory() && !entry.isSymbolicLink()) {This prevents following symlink cycles while still recursing into actual directories.
Consider unifying the three findJsonlFiles implementations into a single reusable function to reduce code duplication and ensure consistent symlink handling across all callers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function findJsonlFilesRecursive(dir) { | |
| const files = []; | |
| try { | |
| const entries = await fs.readdir(dir, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const fullPath = path.join(dir, entry.name); | |
| if (entry.isDirectory()) { | |
| files.push(...await findJsonlFilesRecursive(fullPath)); | |
| } else if (entry.name.endsWith('.jsonl')) { | |
| files.push(fullPath); | |
| } | |
| } | |
| } catch (_) { | |
| // Skip directories we cannot read | |
| } | |
| return files; | |
| } | |
| async function findJsonlFilesRecursive(dir) { | |
| const files = []; | |
| try { | |
| const entries = await fs.readdir(dir, { withFileTypes: true }); | |
| for (const entry of entries) { | |
| const fullPath = path.join(dir, entry.name); | |
| if (entry.isDirectory() && !entry.isSymbolicLink()) { | |
| files.push(...await findJsonlFilesRecursive(fullPath)); | |
| } else if (entry.name.endsWith('.jsonl')) { | |
| files.push(fullPath); | |
| } | |
| } | |
| } catch (_) { | |
| // Skip directories we cannot read | |
| } | |
| return files; | |
| } |
🤖 Prompt for AI Agents
In `@server/projects.js` around lines 446 - 462, The recursive file-walk functions
are following symlinks and can loop; update findJsonlFilesRecursive and the two
inline findJsonlFiles used by getCodexSessions and deleteCodexSession to skip
symbolic links by checking !entry.isSymbolicLink() before recursing into
directories (i.e., only recurse when entry.isDirectory() &&
!entry.isSymbolicLink()); also consider extracting a single shared utility
(e.g., findJsonlFiles) and replacing the three implementations with that to
ensure consistent symlink protection across callers.
|
Follow-up fixes pushed in
Validation:
@coderabbitai please re-review the latest commit. |
|
✅ Double-scan eliminated - Removed eager I appreciate the validation run with typecheck and build. I'll review the latest commit now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/projects.js (1)
1559-1575:⚠️ Potential issue | 🟡 Minor
findSessionFileingetCodexSessionMessagesis missing the symlink guard added elsewhere.The two
findJsonlFileshelpers (lines 1427 and 1798) were updated to skip symlinks, butfindSessionFileat line 1564 still recurses unconditionally intoentry.isDirectory(). A symlink cycle under~/.codex/sessionswould cause a stack overflow here.🛡️ Proposed fix
- if (entry.isDirectory()) { + if (entry.isDirectory() && !entry.isSymbolicLink()) {
🤖 Fix all issues with AI agents
In `@src/components/Sidebar.jsx`:
- Around line 1208-1211: The desktop view currently renders the raw numeric
label from getProjectSessionCountInfo(project).label; update the JSX to use the
i18n key like the mobile view by calling t('projects.sessionCountLabel', {
count: getProjectSessionCountInfo(project).count ||
parseInt(getProjectSessionCountInfo(project).label, 10) }) (or pass the
appropriate numeric value returned by getProjectSessionCountInfo) instead of
rendering .label directly so desktop uses the same localized/pluralized string
as mobile; keep the same surrounding element (the div with className "text-xs
text-muted-foreground") and use the existing t function imported in the
component.
🧹 Nitpick comments (5)
server/projects.js (2)
380-383:truncatedReadDetectedis file-scoped but used as a project-wide flag — may scan more files than necessary.Once any file triggers a truncated read, the early-exit condition on line 380 can never be satisfied, forcing a scan of every remaining JSONL file even if enough sessions were already discovered. For projects with many files where the newest (largest) file is the only truncated one, this eliminates the I/O savings the PR aims for.
Consider tracking truncation per-file and only keeping the project-wide flag for the
hasMoreheuristic, or allowing the early exit whensessions.size >= targetSessionCountregardless of truncation:- if (sessions.size >= targetSessionCount && !truncatedReadDetected) { + if (sessions.size >= targetSessionCount) { break; }The
hasMorecomputation (lines 408–410) can still usetruncatedReadDetectedto signal incompleteness.
655-691: Sequentialawait getSessionsPreviewin theforloop serializes I/O across all project directories.Each project's preview is fetched one at a time. For users with many projects, this serialization adds up. Consider batching with bounded concurrency (e.g.,
Promise.allover small chunks) to leverage parallel filesystem I/O.src/components/Sidebar.jsx (1)
217-294:hydrateProjectSessions— overall structure is sound; note the fire-and-forget pattern for cursor/codex.The cursor and codex promises (lines 227–241) are started in parallel with the Claude fetch, but their results are consumed after the
finallyblock clearsloadingSessions. This means the loading spinner disappears before cursor/codex data arrives. If that's the intended UX (show Claude sessions immediately, then silently backfill cursor/codex), this is fine. If the spinner should cover all three providers, move thePromise.allSettledinto thetryblock before thefinally.src/App.jsx (2)
163-173: Nested ternaries inmergeHydratedProjectDataare correct but hard to follow.The logic for preserving hydrated data is sound — once a provider's data is hydrated, the hydrated source wins. However, three levels of nested ternaries repeated for each field make this difficult to reason about during future maintenance.
Consider extracting a small helper:
♻️ Suggested readability improvement
+ const pickHydrated = (field, hydratedFlag) => { + const isHydrated = Boolean(incomingProject[hydratedFlag] || currentProject[hydratedFlag]); + if (!isHydrated) return incomingProject[field] || []; + return currentProject[hydratedFlag] ? currentProject[field] : (incomingProject[field] || []); + }; + return { ...incomingProject, - sessions: sessionsHydrated ? (currentProject.sessionsHydrated ? currentProject.sessions : (incomingProject.sessions || [])) : (incomingProject.sessions || []), - sessionMeta: sessionsHydrated ? (currentProject.sessionsHydrated ? currentProject.sessionMeta : incomingProject.sessionMeta) : incomingProject.sessionMeta, + sessions: pickHydrated('sessions', 'sessionsHydrated'), + sessionMeta: sessionsHydrated + ? (currentProject.sessionsHydrated ? currentProject.sessionMeta : incomingProject.sessionMeta) + : incomingProject.sessionMeta, sessionsHydrated, - cursorSessions: cursorSessionsHydrated ? (currentProject.cursorSessionsHydrated ? currentProject.cursorSessions : (incomingProject.cursorSessions || [])) : (incomingProject.cursorSessions || []), + cursorSessions: pickHydrated('cursorSessions', 'cursorSessionsHydrated'), cursorSessionsHydrated, - codexSessions: codexSessionsHydrated ? (currentProject.codexSessionsHydrated ? currentProject.codexSessions : (incomingProject.codexSessions || [])) : (incomingProject.codexSessions || []), + codexSessions: pickHydrated('codexSessions', 'codexSessionsHydrated'), codexSessionsHydrated };
265-285: RedundantmergeHydratedProjectDatacall whenhasActiveSession && isAdditiveUpdate.When the active-session branch (line 267) determines the update is additive and falls through, line 283 recomputes the same merge that was already done at line 270. Consider reusing the result:
♻️ Suggested fix
if (hasActiveSession) { const currentProjects = projectsRef.current; const updatedProjects = mergeHydratedProjectData(currentProjects, latestMessage.projects); const isAdditiveUpdate = isUpdateAdditive(currentProjects, updatedProjects, selectedProject, selectedSession); if (!isAdditiveUpdate) { return; } - // Continue with additive updates below + // Additive update — reuse the already-merged projects + projectsRef.current = updatedProjects; + setProjects(updatedProjects); + } else { + const updatedProjects = mergeHydratedProjectData(projectsRef.current, latestMessage.projects); + projectsRef.current = updatedProjects; + setProjects(updatedProjects); } - - const updatedProjects = mergeHydratedProjectData(projectsRef.current, latestMessage.projects); - projectsRef.current = updatedProjects; - setProjects(updatedProjects);
| <div className="text-xs text-muted-foreground"> | ||
| {(() => { | ||
| const sessionCount = getAllSessions(project).length; | ||
| const hasMore = project.sessionMeta?.hasMore !== false; | ||
| return hasMore && sessionCount >= 5 ? `${sessionCount}+` : sessionCount; | ||
| return getProjectSessionCountInfo(project).label; | ||
| })()} |
There was a problem hiding this comment.
Desktop session count doesn't use the i18n sessionCountLabel key.
The mobile view (line 1066) renders the localized t('projects.sessionCountLabel', ...) string (e.g., "5 sessions"), but the desktop view here renders only the raw numeric label (e.g., "5+") without the localized noun. This means non-English users will see a bare number on desktop but a translated string on mobile.
If the compact display is intentional, consider this a nit. Otherwise, apply the same t() call here.
🤖 Prompt for AI Agents
In `@src/components/Sidebar.jsx` around lines 1208 - 1211, The desktop view
currently renders the raw numeric label from
getProjectSessionCountInfo(project).label; update the JSX to use the i18n key
like the mobile view by calling t('projects.sessionCountLabel', { count:
getProjectSessionCountInfo(project).count ||
parseInt(getProjectSessionCountInfo(project).label, 10) }) (or pass the
appropriate numeric value returned by getProjectSessionCountInfo) instead of
rendering .label directly so desktop uses the same localized/pluralized string
as mobile; keep the same surrounding element (the div with className "text-xs
text-muted-foreground") and use the existing t function imported in the
component.
|
Great work on this performance optimization! 🚀 I've been using Claude Code UI with multiple projects and noticed the scanning can be slow sometimes. This PR looks like it addresses exactly that pain point. A few thoughts:
Looking forward to seeing this merged. I'm also tracking #379 (slow response with many MCP servers) — these two improvements together would make a big difference for power users. Happy to help test if needed! |
|
Wow! 🔥 This PR seems to hit two birds with one stone:
Really excited for this to land! Let me know if you need help verifying the Chinese translations or testing performance. |
2 participants
Summary
Why
~/.claude/projectsJSONL files were fully read on each refresh, causing slow project loadingNotes
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
Localization