Refactor provider/session architecture to be DB-driven, modular, and sessionId-first across backend and frontend#715
Conversation
… clarity and consistency
- Implemented githubTokensDb for managing GitHub tokens with CRUD operations. - Created otificationPreferencesDb to handle user notification preferences. - Added projectsDb for project path management and related operations. - Introduced pushSubscriptionsDb for managing browser push subscriptions. - Developed scanStateDb to track the last scanned timestamp. - Established sessionsDb for session management with CRUD functionalities. - Created userDb for user management, including authentication and onboarding. - Implemented �apidKeysDb for storing and managing VAPID keys. feat(database): define schema for new database tables - Added SQL schema definitions for users, API keys, user credentials, notification preferences, VAPID keys, push subscriptions, projects, sessions, scan state, and app configuration. - Included necessary indexes for performance optimization. refactor(shared): enhance type definitions and utility functions - Updated shared types and interfaces for improved clarity and consistency. - Added new types for credential management and provider-specific operations. - Refined utility functions for better error handling and message normalization.
Why: - /api/projects is a hot path (initial load, sidebar refresh, websocket sync). - Scanning .taskmaster for every project on each call added avoidable fs I/O and payload size. - TaskMaster metadata is only needed after selecting a specific project. - Moving it to a project-scoped endpoint makes loading cost match user intent. - The UI now hydrates TaskMaster state on selection and keeps it across refresh events. - This prevents status flicker/regression while still removing global scan overhead. - Selection fetches are sequence-guarded to block stale async responses on fast switching. - isManuallyAdded was removed from responses to keep the public project contract minimal. - Project dumps now use incrementing snapshot files to preserve history for debugging. What changed: - Added GET /api/projects/:projectName/taskmaster and getProjectTaskMaster(). - Removed TaskMaster detection from bulk getProjects(). - Added api.projectTaskmaster(...) plus selection-time hydration in frontend contexts. - Merged cached taskmaster values into refreshed project lists for continuity. - Removed isManuallyAdded from manual project payloads.
…db.js and schema.js files
…er-derived name GET /api/projects used to scan ~/.claude/projects/ on every request, derive each project's identity from the encoded folder name, and re-parse JSONL files to build session lists. Using the folder-derived name as the project identifier leaked the Claude CLI's on-disk encoding into every API route, forced every downstream endpoint to re-resolve a real path via JSONL 'cwd' inspection, and made the project list endpoint O(projects x sessions) on disk I/O. This change switches the entire API surface to identify projects by the stable primary key from the 'projects' table and drives the listing straight from the DB: - Add projectsDb.getProjectPathById as the canonical projectId -> path resolver so routes no longer need to touch the filesystem to figure out where a project lives. - Rewrite getProjects so it reads the project list from the 'projects' table and the per-project session list from the 'sessions' table (one SELECT per project). No filesystem scanning happens for this endpoint anymore, which removes the dependency on ~/.claude/projects existing, on Cursor's MD5-hashed chat folders being discoverable, and on Codex's JSONL history being on disk. Per the migration spec each session now exposes 'summary' sourced from sessions.custom_name, 'messageCount' = 0 (message counting is not implemented), and sessionMeta.hasMore is pinned to false since this endpoint doesn't drive session pagination. - Introduce id-based wrappers (getSessionsById, renameProjectById, deleteSessionById, deleteProjectById, getProjectTaskMasterById) so every caller can pass projectId and resolve the real path through the DB. renameProjectById also writes to projects.custom_project_name so the DB-driven getProjects response reflects renames immediately; it keeps project-config.json in sync for any legacy reader that still consults the JSON file. - Migrate every /api/projects/:projectName route in server/index.js, server/routes/taskmaster.js, and server/routes/messages.js to :projectId, and change server/routes/git.js so the 'project' query/body parameter carries a projectId that is resolved through the DB before any git command runs. TaskMaster WebSocket broadcasts emit 'projectId' for the same reason so the frontend can match notifications against its current selection without another lookup. - Delete helpers that existed only to feed the old getProjects path (getCursorSessions, getGeminiCliSessions, getProjectTaskMaster) along with their unused imports (better-sqlite3's Database, applyCustomSessionNames). The legacy folder-name helpers (getSessions, renameProject, deleteSession, deleteProject, extractProjectDirectory) are kept as internal implementation details of the id-based wrappers and of destructive cleanup / conversation search, but they are no longer re-exported. - searchConversations still walks JSONL to produce match snippets (that data doesn't live in the DB), but it now includes the resolved projectId in each result so the sidebar can cross-reference hits with its already loaded project list without a second round-trip. Frontend migration: - Project.name is replaced by Project.projectId in src/types/app.ts, and ProjectSession.__projectName becomes __projectId so session tagging and sidebar state keys stay aligned with the backend identifier. Settings continues to use SettingsProject.name for legacy consumers, but it is populated from projectId by normalizeProjectForSettings. - All places that previously indexed per-project state by project.name (sidebar expanded/starred/loading/deletingProjects sets, additionalSessions map, projectHasMoreOverrides, starredProjects localStorage, command history and draft-input localStorage, TaskMaster caches) now key on projectId so state survives display-name edits and is consistent across the app. - src/utils/api.js renames every endpoint parameter to projectId, the unified messages endpoint takes projectId in its query string, and useSessionStore forwards projectId on fetchFromServer / fetchMore / refreshFromServer. Git panel, file tree, code editor, PRD editor, plugins context, MCP server flows and TaskMaster hooks are all updated to pass projectId. - DEFAULT_PROJECT_FOR_EMPTY_SHELL is updated to carry a 'default' projectId sentinel so the empty-shell placeholder still satisfies the Project contract. Bug fix bundled in: - sessionsDb.setName no longer bumps updated_at when a row already exists. Renaming is a label change, not activity, so there is no reason for it to reset 'last activity' in the sidebar. It also no longer relies on SQLite's CURRENT_TIMESTAMP, which stores a naive 'YYYY-MM-DD HH:MM:SS' value that JavaScript parses as local time and caused renamed sessions to appear shifted backwards by the client's UTC offset. When an INSERT actually happens it now writes ISO-8601 UTC with a 'Z' suffix. - buildSessionsByProviderFromDb normalizes any legacy naive timestamps in the sessions table to ISO-8601 UTC on the way out so rows written before this change also render correctly on the client. Other cleanup: - Removed the filesystem-first project-discovery comment block at the top of server/projects.js and replaced it with a short note that describes the new DB-driven flow and lists the few remaining filesystem-dependent helpers (message reads, search, destructive delete, manual project registration). - server/modules/providers/index.ts is added as a small barrel so the providers module exposes a stable public surface. Made-with: Cursor
In addition, for projects_updated websocket response, send the sessionId instead
Restructure project creation, listing, GitHub clone progress, and TaskMaster details behind a dedicated TypeScript module under server/modules/projects/, and align the client wizard with a single path-based flow. Server / routing - Remove server/routes/projects.js and mount server/modules/projects/ projects.routes.ts at /api/projects (still behind authenticateToken). - Drop duplicate handlers from server/index.js for GET /api/projects and GET /api/projects/:projectId/taskmaster; those live on the new router. - Import WORKSPACES_ROOT and validateWorkspacePath from shared utils in index.js instead of the deleted projects route module. Projects router (projects.routes.ts) - GET /: list projects with sessions (existing snapshot behavior). - POST /create-project: validate body, reject legacy workspaceType and mixed clone fields, delegate to createProject service, return distinct success copy when an archived path is reactivated. - GET /clone-progress: Server-Sent Events for clone progress/complete/error; requires authenticated user id for token resolution; wires startCloneProject. - GET /:projectId/taskmaster: delegates to getProjectTaskMaster. Services (new) - project-management.service.ts: path validation, workspace directory creation, persistence via projectsDb.createProjectPath, mapping to API project shape; surfaces AppError for validation, conflict, and not-found cases; optional dependency injection for tests. - project-clone.service.ts: validates workspace, resolves GitHub auth (stored token or inline token), runs git clone with progress callbacks, registers project via createProject on success; sanitizes errors and supports cancellation; injectable dependencies for tests. - projects-has-taskmaster.service.ts: moves TaskMaster detection and normalization out of server/projects.js; resolve-by-id and public getProjectTaskMaster with structured AppError responses. Persistence and shared types - projectsDb.createProjectPath now returns CreateProjectPathResult (created | reactivated_archived | active_conflict) using INSERT … ON CONFLICT with selective update when the row is archived; normalizes display name from path or custom name; repository row typing moves to shared ProjectRepositoryRow. - getProjectPaths() returns only non-archived rows (isArchived = 0). - shared/types.ts: ProjectRepositoryRow, CreateProjectPathResult/outcome, WorkspacePathValidationResult. - shared/utils.ts: WORKSPACES_ROOT, forbidden path lists, validateWorkspacePath, asyncHandler for Express async routes. Legacy cleanup - server/projects.js: remove detectTaskMasterFolder, normalizeTaskMasterInfo, and getProjectTaskMasterById (logic lives in the new service). - server/routes/agent.js: register external API project paths with projectsDb.createProjectPath instead of addProjectManually try/catch; treat active_conflict as an existing registration and continue. Tests - Add Node test suites for project-management, project-clone, and projects-has-taskmaster services; update projects.service test import for renamed projects-with-sessions-fetch.service.ts. Rename - projects.service.ts → projects-with-sessions-fetch.service.ts; re-export from modules/projects/index.ts. Client (project creation wizard) - Remove StepTypeSelection and workspaceType from form state and types; wizard is two steps (configure path/GitHub auth, then review). - createWorkspaceRequest → createProjectRequest; clone vs create-only inferred from githubUrl (pathUtils / isCloneWorkflow). - Adjust step indices, WizardProgress, StepConfiguration/Review, WorkspacePathField, and src/utils/api.js as needed for the new API. Docs - Minor websocket README touch-up. Net: ~1.6k insertions / ~0.9k deletions across 29 files; behavior is centralized in typed services with explicit HTTP errors and test seams.
The websocket projects effect in useProjectsState could re-handle the same latestMessage after local state writes triggered re-renders. Under bursty websocket traffic, this created an update feedback cycle that surfaced as 'Maximum update depth exceeded', often from Sidebar. What changed: - Added lastHandledMessageRef so each latestMessage object is handled once. - Added an early return guard when the current message was already handled. - Made projects updates idempotent by comparing previous and merged payloads before calling setProjects. Result: - Breaks the effect -> state update -> effect re-entry cycle. - Reduces redundant renders during rapid projects_updated traffic while preserving normal project/session synchronization.
|
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:
📝 WalkthroughWalkthroughMigrates project/session identity from filesystem names to DB-backed projectId; replaces legacy monolithic DB with modular connection, schema, repositories, and migrations; adds per-provider session synchronizers, filesystem watcher, WebSocket server/services (chat/shell/plugin proxy), project services/routes, search, and widespread frontend projectId updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Browser Client
participant HTTP as HTTP Server
participant WSS as WebSocketServer
participant SyncSvc as SessionSynchronizerService
participant Provider as ProviderSync (Claude/Codex/...)
participant DB as SQLite DB
participant FS as File System
Client->>HTTP: HTTP request /api or ws upgrade
HTTP->>WSS: createWebSocketServer(attached to HTTP)
WSS->>HTTP: verifyWebSocketClient(auth)
Note over HTTP,SyncSvc: Startup
HTTP->>SyncSvc: initializeSessionsWatcher()
SyncSvc->>Provider: provider.sessionSynchronizer.synchronize(lastScanAt)
Provider->>FS: scan JSONL/JSON artifact files
Provider->>DB: upsert sessions (sessionsDb.createSession)
FS-->>SyncSvc: chokidar add/change event (filePath)
SyncSvc->>Provider: synchronizeFile(filePath)
Provider->>DB: update session row (createSession / update)
SyncSvc->>HTTP: getProjectsWithSessions(skipSynchronization=true)
HTTP->>WSS: broadcast projects_updated (batched)
Client->>WSS: send chat/shell/plugin messages (sessionId)
WSS->>DB: resolve session by sessionId
WSS->>Provider: route provider-specific command
Provider->>WSS: response messages
WSS-->>Client: deliver messages
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/components/plugins/view/PluginTabContent.tsx (1)
13-42:⚠️ Potential issue | 🟡 MinorPlugin contract change:
project.nameis now an opaque DB id, not a display name.Existing plugins displaying
context.project.nameto users will show the opaqueprojectId(a database-assigned identifier) instead of the human-readable display name. TheProjecttype has bothprojectIdanddisplayNameavailable, anddisplayNameis already used consistently throughout the UI (Shell.tsx, MainContentTitle.tsx).Consider this fix:
Suggested change
type PluginContext = { theme: 'dark' | 'light'; - // Plugin contract historically used `name` for the project identifier; we - // keep that key and populate it from the DB `projectId` so external plugins - // continue to receive a stable opaque id. - project: { name: string; path: string } | null; + project: { id: string; name: string; path: string } | null; session: { id: string; title: string } | null; }; ... project: selectedProject ? { - name: selectedProject.projectId, + id: selectedProject.projectId, + name: selectedProject.displayName, path: selectedProject.fullPath || selectedProject.path || '', } : null,Per Contributing.md, clearly describe what changed and mark this as a
BREAKING CHANGE:in the commit message / PR description if plugins are supported externally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/plugins/view/PluginTabContent.tsx` around lines 13 - 42, The PluginContext currently sets project.name to the opaque DB id (selectedProject.projectId), causing plugins that render context.project.name to show a non-human id; update buildContext so project.name is set to selectedProject.displayName (falling back to fullPath/path/id as appropriate) instead of projectId, and ensure any external plugin consumers are informed by marking the PR/commit as BREAKING CHANGE in the description per Contributing.md.src/components/file-tree/hooks/useFileTreeUpload.ts (1)
91-160:⚠️ Potential issue | 🟡 MinorGuard against missing
selectedProjectinhandleDrop.
handleDropis registered unconditionally on the tree container inFileTree.tsx, but the upload URL usesselectedProject!.projectIdwith a non-null assertion. If a drop is triggered whenselectedProjectisnull(orprojectIdis missing), the request URL becomes/projects/undefined/files/uploadand the server returns an error after the user has already gone through directory traversal/FormData construction.🛡️ Proposed early guard
const handleDrop = useCallback(async (e: React.DragEvent) => { e.preventDefault(); e.stopPropagation(); setIsDragOver(false); + if (!selectedProject?.projectId) { + setDropTarget(null); + showToast('No project selected', 'error'); + return; + } + const targetPath = dropTarget || ''; setOperationLoading(true);And then drop the
!assertion on line 158.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/file-tree/hooks/useFileTreeUpload.ts` around lines 91 - 160, handleDrop uses selectedProject!.projectId with a non-null assertion which can produce a bad upload URL if selectedProject is null; add an early guard at the start of handleDrop (before heavy work like directory traversal and FormData creation) to check selectedProject and selectedProject.projectId, and if missing immediately setOperationLoading(false), setDropTarget(null) and return (optionally surface an error/toast), then remove the `!` non-null assertion from the API call so the code uses the validated selectedProject.projectId.server/routes/codex.js (1)
8-18:⚠️ Potential issue | 🔴 Critical
deleteSessionByIddeletes without filtering by provider — will delete any session with matching ID across all providers.The implementation at
server/modules/database/repositories/sessions.db.ts:172uses onlysession_idin the WHERE clause:DELETE FROM sessions WHERE session_id = ?But the schema defines a composite PRIMARY KEY
(session_id, provider)(line 91, schema.ts). This means session IDs are not globally unique — multiple providers can share the same ID. The current delete will remove all rows with that session ID regardless of provider.In the route,
deleteCodexSession(line 11) is provider-scoped, but the subsequentsessionsDb.deleteSessionById(sessionId)(line 12) will delete the first matching session from any provider. If session IDs collide across providers (e.g., a Codex and Claude session both use IDxyz), this route will delete the wrong provider's data.Fix: Pass the provider name to
deleteSessionByIdand filter by bothsession_id AND provider, or use a separate provider-scoped delete method likedeleteSessionByIdAndProvider(sessionId, provider).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/codex.js` around lines 8 - 18, The route calls sessionsDb.deleteSessionById(sessionId) which only filters by session_id but your DB schema uses a composite primary key (session_id, provider), so that delete can remove rows for other providers; update the route to pass the provider and delete by both keys (either call a new repository method like deleteSessionByIdAndProvider(sessionId, provider) or change deleteSessionById to accept a provider param) and invoke it from the route (e.g., use the same provider scope as deleteCodexSession) so the SQL uses WHERE session_id = ? AND provider = ?.src/components/chat/hooks/useChatComposerState.ts (1)
715-736:⚠️ Potential issue | 🟡 MinorPotential clobber when switching projects (pre-existing race).
Effect on Lines 727–736 depends on
[input, selectedProject], while the restore effect on Lines 715–725 depends on[selectedProject?.projectId]. WhenselectedProjectchanges, the persist effect runs synchronously with the previousinputand writes it under the new project'sdraft_input_${projectId}key, briefly clobbering the new project's saved draft (it is then re-written oncesetInputfrom the restore effect commits). Worst case: if the previous project's input was empty,removeItemdeletes the new project's saved draft before it can be restored.Consider gating the persist effect on a ref tracking the project the current
inputbelongs to (or make the persist effect's dep be[input, selectedProject?.projectId]plus a "ready" flag set only after restore completes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatComposerState.ts` around lines 715 - 736, The persist effect can clobber a newly-selected project's draft because it runs with the old selectedProject while the restore effect is still replaying saved input; fix by introducing a ref/ready flag that tracks which project the current input belongs to (e.g., restoredProjectIdRef or isRestoredRef) and only allow the persist effect to write/remove when that ref matches selectedProject.projectId (set that ref inside the restore effect after calling setInput and updating inputValueRef.current); also change the persist effect's dependency to use selectedProject?.projectId (not the whole object) so it runs predictably for project switches and gates writes until the restore completes.server/routes/git.js (1)
567-571:⚠️ Potential issue | 🟡 MinorStale error message on
/commitafter the projectId migration.Every other route in this file was updated to say "Project id ..." but
/commitstill emits "Project name, commit message, and files are required", even thoughgetActualProjectPath(project)on line 574 now expects a DBprojectId. Inconsistent with the rest of the file and misleading to API consumers.🩹 Suggested fix
if (!project || !message || !files || files.length === 0) { - return res.status(400).json({ error: 'Project name, commit message, and files are required' }); + return res.status(400).json({ error: 'Project id, commit message, and files are required' }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/git.js` around lines 567 - 571, Update the stale error message in the request validation for the /commit handler: when destructuring const { project, message, files } = req.body and checking if (!project || !message || !files || files.length === 0), change the returned JSON error to reference "Project id" (or "projectId") instead of "Project name" to match the rest of this file and the use of getActualProjectPath(project); ensure the error text reads e.g. "Project id, commit message, and files are required" so API consumers see a consistent message.src/components/project-creation-wizard/ProjectCreationWizard.tsx (1)
66-76:⚠️ Potential issue | 🟡 MinorValidation bypass via
onAdvanceToConfirm.
handleNext(Lines 66–76) guards step 1 → 2 by requiring a non-emptyworkspacePath, butonAdvanceToConfirmon Line 178 callssetStep(2)directly without that guard. IfWorkspacePathFieldtriggers this (e.g., on Enter) with an empty/whitespace path, the user lands on the review step in an invalid state and the only signal is the eventual create-time failure. Route the keyboard advance through the same validation path:🛡️ Proposed fix
- onAdvanceToConfirm={() => setStep(2)} + onAdvanceToConfirm={handleNext}Also applies to: 178-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/project-creation-wizard/ProjectCreationWizard.tsx` around lines 66 - 76, handleNext is being bypassed because onAdvanceToConfirm calls setStep(2) directly, allowing an empty/whitespace workspacePath to advance; change calls so keyboard advances run the same validation path. Fix by routing the keyboard/enter handler (onAdvanceToConfirm) to call the existing handleNext (or extract a small validateAndAdvanceToStep2 function used by both handleNext and onAdvanceToConfirm) instead of directly calling setStep(2); ensure the validation uses formState.workspacePath.trim() and sets setError(t('projectWizard.errors.providePath')) on failure so WorkspacePathField cannot advance to step 2 with empty input.server/index.js (1)
1560-1589:⚠️ Potential issue | 🔴 CriticalCritical:
closeSessionsWatcher()is invoked at startup, not on shutdown.
server.listen()returns synchronously (its callback runs later), so the top-levelawait closeSessionsWatcher()at line 1582 fires almost immediately after — racing with (or running before)initializeSessionsWatcher()inside the listen callback. The net effect is that the file watcher is torn down right after it's initialized, and on real shutdown (SIGTERM/SIGINT)shutdownPluginsdoes not close the watcher at all, so chokidar handles can keep the process from exiting cleanly.The teardown call needs to live in the signal handlers next to
stopAllPlugins().🐛 Proposed fix
server.listen(SERVER_PORT, HOST, async () => { // ... existing body ... await initializeSessionsWatcher(); // Start server-side plugin processes for enabled plugins startEnabledPluginServers().catch(err => { console.error('[Plugins] Error during startup:', err.message); }); }); - await closeSessionsWatcher(); // Clean up plugin processes on shutdown const shutdownPlugins = async () => { + await closeSessionsWatcher(); await stopAllPlugins(); process.exit(0); }; process.on('SIGTERM', () => void shutdownPlugins()); process.on('SIGINT', () => void shutdownPlugins());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 1560 - 1589, The call to closeSessionsWatcher() is incorrectly awaited at startup and should be moved into the shutdown flow; remove the top-level await closeSessionsWatcher() and instead ensure the watcher teardown runs inside the shutdown handler alongside stopAllPlugins(). Specifically, in the code around server.listen(...) and the signal handlers, keep initializeSessionsWatcher() inside the listen callback, delete the premature await closeSessionsWatcher(), and update shutdownPlugins (the function invoked on SIGTERM/SIGINT) to await closeSessionsWatcher() and await stopAllPlugins() before exiting so the watcher is properly closed on process termination.server/routes/taskmaster.js (1)
426-448:⚠️ Potential issue | 🔴 CriticalPath traversal:
fileNameis read straight off the URL intopath.join.
req.params.fileNameis concatenated into a filesystem path with no validation. A request toGET /api/taskmaster/prd/<id>/..%2F..%2F..%2Fetc%2Fpasswdresolves outside<projectPath>/.taskmaster/docsand the handler returns the file contents. The siblingPOST /prd/:projectId(Line 356) already enforces a regex; apply the same check (and ideally also resolve + verify the result stays within the docs dir).🛡️ Suggested fix
router.get('/prd/:projectId/:fileName', async (req, res) => { try { const { projectId, fileName } = req.params; + + if (!fileName.match(/^[\w\-. ]+\.(txt|md)$/) || fileName.includes('..')) { + return res.status(400).json({ + error: 'Invalid filename', + message: 'Filename must end with .txt or .md and contain only alphanumeric characters, spaces, dots, and dashes' + }); + } const projectPath = await resolveProjectPathFromId(projectId); if (!projectPath) { return res.status(404).json({ error: 'Project not found', message: `Project "${projectId}" does not exist` }); } - const filePath = path.join(projectPath, '.taskmaster', 'docs', fileName); + const docsDir = path.resolve(projectPath, '.taskmaster', 'docs'); + const filePath = path.resolve(docsDir, fileName); + if (!filePath.startsWith(docsDir + path.sep)) { + return res.status(400).json({ error: 'Invalid filename' }); + }The same validation is missing in
POST /parse-prd/:projectId(Line 815,fileNamefromreq.body) andPOST /apply-template/:projectId(Line 1394,fileNamefromreq.body). Please harden all three.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/taskmaster.js` around lines 426 - 448, The GET handler for '/prd/:projectId/:fileName' currently uses req.params.fileName directly causing path traversal; update it to enforce the same filename regex used in the POST '/prd/:projectId' route (validate fileName against the allowed pattern), then construct the absolute docsDir (path.resolve(projectPath, '.taskmaster', 'docs')) and resolve the requested path (path.resolve(docsDir, fileName)) and verify the resolved path startsWith the docsDir before accessing; if validation or containment fails return a 400/404 and do not read the file. Apply the identical filename validation + resolve-and-verify containment fix to the handlers for POST '/parse-prd/:projectId' (where fileName is from req.body) and POST '/apply-template/:projectId' (fileName from req.body) so all three endpoints use the same safe checks.
🟡 Minor comments (23)
src/i18n/config.js-189-190 (1)
189-190:⚠️ Potential issue | 🟡 MinorUpdate stale inline comment to match
debug: falsebehavior.Line 189 says debug is enabled in development, but Line 190 hard-disables it. Please update the comment to avoid misleading future changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/config.js` around lines 189 - 190, The inline comment above the debug property is stale; update the comment near the debug: false entry in src/i18n/config.js so it accurately states that debug is disabled (or controlled elsewhere) rather than saying debug is enabled in development—modify the comment text adjacent to the debug property to reflect the current behavior.server/modules/providers/list/codex/codex-sessions.provider.ts-254-260 (1)
254-260:⚠️ Potential issue | 🟡 Minor
totalis omitted whenlimitis null, causingfetchHistoryto reporttotal: 0.When called without a limit, this branch returns
{ messages, tokenUsage }and dropstotal/hasMore. Downstream at lines 524–526:const total = Array.isArray(result) ? rawMessages.length : (result.total || 0); const hasMore = Array.isArray(result) ? false : Boolean(result.hasMore);
result.totalisundefined, so the API returnstotal: 0even though the full message list was loaded. Any client usingtotalfor "X messages" UI or further pagination will be broken.🐛 Suggested fix
- return { messages, tokenUsage }; + return { + messages, + total: messages.length, + hasMore: false, + offset, + limit, + tokenUsage, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/codex/codex-sessions.provider.ts` around lines 254 - 260, When the function returns the full message list without a limit it currently returns only { messages, tokenUsage }, omitting total/hasMore which causes downstream code (where result.total and result.hasMore are read) to treat total as 0; update the branch that returns { messages, tokenUsage } to also include total: messages.length and hasMore: false (and keep tokenUsage), so callers reading result.total/result.hasMore get correct values; check identifiers messages, tokenUsage, total, hasMore and the result variable used by fetchHistory/fetch sessions.server/modules/websocket/services/shell-websocket.service.ts-282-356 (1)
282-356:⚠️ Potential issue | 🟡 MinorStale URL announcement state on session reattach.
urlDetectionBufferandannouncedAuthUrlsare captured in the outer connection closure, butonDatais registered once when the PTY is first spawned (Line 282). On reattach (Line 207–234),existingSession.wsis updated to the new socket, yet the originalonDataclosure keeps using the first connection’sannouncedAuthUrlsset — so a client that reconnects mid-flow will never receiveauth_urlevents for URLs already announced to a previous (now closed) socket, and the URL parse buffer keeps growing under the original WS lifetime instead of being scoped to the PTY session.Consider hanging this state off the
PtySessionEntry(e.g.,announcedAuthUrls: Set<string>,urlDetectionBuffer: string) and reading/mutating viasessioninsideonData, then resetting it on reattach so a new client can re-receive pending auth URLs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/websocket/services/shell-websocket.service.ts` around lines 282 - 356, The onData handler registered in shellProcess.onData closes over module-level urlDetectionBuffer and announcedAuthUrls, causing stale URL state across reattachments; move these into the PtySessionEntry (e.g., add announcedAuthUrls: Set<string> and urlDetectionBuffer: string on the session object stored in ptySessionsMap) and update the onData callback to read/mutate session.urlDetectionBuffer and session.announcedAuthUrls instead of the outer variables; also update the reattach logic that replaces existingSession.ws to reset session.urlDetectionBuffer = '' and session.announcedAuthUrls.clear() so a newly attached client can receive auth_url events (refer to ptySessionsMap, shellProcess.onData, ptySessionKey, existingSession, dependencies.extractUrlsFromText and dependencies.shouldAutoOpenUrlFromOutput in your changes).server/shared/utils.ts-160-175 (1)
160-175:⚠️ Potential issue | 🟡 MinorTighten the
/varallowlist to avoid prefix collisions.
normalizedPath.startsWith('/var/tmp')andnormalizedPath.startsWith('/var/folders')will also match siblings like/var/tmpevilor/var/folders-attacker. Containment via theWORKSPACES_ROOTrealpath check at Line 199–208 mitigates this in practice, but the allowlist itself should be precise:🔧 Tighten the prefix check
- if ( - forbiddenPath === '/var' - && (normalizedPath.startsWith('/var/tmp') || normalizedPath.startsWith('/var/folders')) - ) { - continue; - } + if ( + forbiddenPath === '/var' + && ( + normalizedPath === '/var/tmp' + || normalizedPath.startsWith('/var/tmp/') + || normalizedPath === '/var/folders' + || normalizedPath.startsWith('/var/folders/') + ) + ) { + continue; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/utils.ts` around lines 160 - 175, The current allowlist for '/var' uses startsWith('/var/tmp') and startsWith('/var/folders') which can match sibling prefixes; update the conditional in the FORBIDDEN_WORKSPACE_PATHS loop (referencing normalizedPath and forbiddenPath) so it allows only '/var/tmp' and '/var/folders' and their true subdirectories by checking exact equality OR startsWith with a following path.sep (e.g. normalizedPath === '/var/tmp' || normalizedPath.startsWith('/var/tmp' + path.sep) and similarly for '/var/folders'), leaving the rest of the forbidden-path logic and the subsequent WORKSPACES_ROOT realpath checks intact.server/shared/utils.ts-465-500 (1)
465-500:⚠️ Potential issue | 🟡 MinorFallback for
birthtimeon filesystems without reliable creation-time metadata.
fileStat.birthtimeis unreliable on tmpfs (especially with kernel <5.17) and potentially some network mounts, where it can default to epoch (1970-01-01). WithlastScanAtset to a recent date, those files will silently fail thebirthtime > lastScanAtcheck and be skipped on subsequent scans.Consider adding a fallback to
mtime(orMath.max(birthtimeMs, ctimeMs, mtimeMs)) whenbirthtimeMsis unreliably low, so incremental scans remain robust across mixed filesystem types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/shared/utils.ts` around lines 465 - 500, The current comparison in findFilesRecursivelyCreatedAfter uses fileStat.birthtime which can be unreliable on some filesystems; change the check to compute an effective timestamp (e.g., const effectiveMs = Math.max(fileStat.birthtimeMs, fileStat.ctimeMs, fileStat.mtimeMs)) and compare that number to lastScanAt.getTime() (after ensuring lastScanAt is converted to milliseconds), so use effectiveMs > lastScanAtMs when deciding to push fullPath; keep the existing behavior when lastScanAt is null. This uses symbols: function findFilesRecursivelyCreatedAfter, variable fileStat, and properties birthtimeMs/ctimeMs/mtimeMs.server/middleware/auth.js-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorImport path uses
.jsextension but references TypeScript source file—update to.tsfor consistency.The import
../modules/database/index.jsresolves toserver/modules/database/index.tsat runtime because:
- Development:
tsxTypeScript loader handles.js→.tsresolution transparently.- Production: The build step (
tsc) compiles all.tsfiles to.jsindist-server/, so import paths resolve correctly in compiled output.While runtime works, source files should import from
../modules/database/index.tsfor clarity and to match the actual file extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/auth.js` at line 2, Update the import in server/middleware/auth.js to reference the TypeScript source file extension: change the module specifier from '../modules/database/index.js' to '../modules/database/index.ts' so the imported symbols userDb and appConfigDb explicitly come from the .ts source; ensure any other imports in this file follow the same .ts convention for consistency.src/components/sidebar/view/subcomponents/SidebarModals.tsx-114-120 (1)
114-120:⚠️ Potential issue | 🟡 MinorUse a better fallback chain for the project identity in the delete confirmation.
The current code falls back directly to
projectIdwhendisplayNameis unavailable. For a destructive operation, this is poor UX—the raw database ID is not a recognizable label to users. All offullPath,path, andprojectIdare available on theProjecttype and should form the fallback chain instead.♻️ Suggested fallback chain
<span className="font-medium text-foreground"> - {deleteConfirmation.project.displayName || deleteConfirmation.project.projectId} + {deleteConfirmation.project.displayName + || deleteConfirmation.project.fullPath + || deleteConfirmation.project.path + || deleteConfirmation.project.projectId} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/view/subcomponents/SidebarModals.tsx` around lines 114 - 120, The delete confirmation currently shows deleteConfirmation.project.displayName || deleteConfirmation.project.projectId which falls back to a raw DB id; update the fallback to use displayName, then fullPath, then path, then projectId (e.g. displayName || fullPath || path || projectId) and guard with optional chaining in case deleteConfirmation or project is undefined (use deleteConfirmation?.project?....). Change the expression inside SidebarModals.tsx where the project label is rendered so the UI shows the most user-friendly identifier available.server/modules/database/init-db.ts-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorUse
console.errorfor the failure log.The catch branch logs at
console.loglevel even though it's an initialization failure that's about to be re-thrown. Useconsole.errorso the message is routed to stderr and is appropriately prioritized by log aggregators.🛠️ Proposed fix
- console.log('Database initialization failed', { error: message }); + console.error('Database initialization failed', { error: message });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/init-db.ts` at line 14, The catch block in the database initialization flow currently uses console.log to report "Database initialization failed" which should be written to stderr; locate the console.log call in server/modules/database/init-db.ts (the catch branch that logs { error: message }) and replace it with console.error so the failure is properly prioritized by log aggregators and routed to stderr before the error is re-thrown.server/modules/providers/list/claude/claude-sessions.provider.ts-184-195 (1)
184-195:⚠️ Potential issue | 🟡 MinorPagination edge case:
offset > totalreturns wrong slice.
endIndex = total - offsetcan become negative when callers pass an offset beyond the available history.Array.prototype.slice(0, -n)then interprets the negative bound as "all but the last n" instead of "empty", returning a non-empty page that the client did not ask for.Concrete example:
total = 5,offset = 6,limit = 2→
startIndex = max(0, 5 - 6 - 2) = 0,endIndex = -1,slice(0, -1)returns the first four messages even though the requested page is past the end.🛠️ Proposed fix
- const startIndex = Math.max(0, total - offset - limit); - const endIndex = total - offset; - const paginatedMessages = sortedMessages.slice(startIndex, endIndex); - const hasMore = startIndex > 0; + const endIndex = Math.max(0, total - offset); + const startIndex = Math.max(0, endIndex - limit); + const paginatedMessages = sortedMessages.slice(startIndex, endIndex); + const hasMore = startIndex > 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/list/claude/claude-sessions.provider.ts` around lines 184 - 195, The pagination math can produce a negative endIndex (endIndex = total - offset) when offset >= total, causing slice(startIndex, endIndex) to return unintended results; modify the pagination in claude-sessions.provider.ts to handle this edge: if offset >= total return an empty messages array (messages: [], hasMore: false) with the existing total/offset/limit, otherwise compute startIndex/endIndex as before and slice sortedMessages; ensure the logic references startIndex, endIndex, sortedMessages and sets hasMore correctly.server/modules/database/repositories/app-config.ts-45-52 (1)
45-52:⚠️ Potential issue | 🟡 MinorPotential JWT secret race across concurrent boots.
getOrCreateJwtSecret()performs a non-atomic read-then-write. If two processes (e.g., clustered workers, dev server + script, parallel test runs) initialize against the same SQLite file at the same time, both can observe a missing secret, generate distinct values, and the secondset()will overwrite the first viaON CONFLICT DO UPDATE— invalidating any tokens already signed by the first. Since the upsert always overwrites, the safest pattern isINSERT ... ON CONFLICT(key) DO NOTHING, then re-get()the row to read the canonical value:🔒 Proposed fix to make the secret bootstrap idempotent
getOrCreateJwtSecret(): string { - let secret = appConfigDb.get('jwt_secret'); - if (!secret) { - secret = crypto.randomBytes(64).toString('hex'); - appConfigDb.set('jwt_secret', secret); - } - return secret; + const existing = appConfigDb.get('jwt_secret'); + if (existing) { + return existing; + } + const candidate = crypto.randomBytes(64).toString('hex'); + const db = getConnection(); + db.prepare( + 'INSERT INTO app_config (key, value) VALUES (?, ?) ON CONFLICT(key) DO NOTHING' + ).run('jwt_secret', candidate); + return appConfigDb.get('jwt_secret') ?? candidate; + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/repositories/app-config.ts` around lines 45 - 52, getOrCreateJwtSecret currently does a non-atomic read-then-write (appConfigDb.get followed by appConfigDb.set) which can race across concurrent processes and overwrite an existing secret; change the bootstrap to perform an atomic insert-if-not-exists and then re-read the canonical value: attempt to insert the jwt_secret row using a single INSERT ... ON CONFLICT(key) DO NOTHING (or equivalent appConfigDb insert API/transaction) instead of unconditionally calling appConfigDb.set, and after the insert always call appConfigDb.get('jwt_secret') to return the persisted canonical secret; ensure the implementation in getOrCreateJwtSecret avoids updating an existing value and only writes when the row is absent.server/modules/providers/provider.routes.ts-306-329 (1)
306-329:⚠️ Potential issue | 🟡 MinorTighten
limit/offsetvalidation.Two gaps:
Number.parseInt('5abc', 10)returns5, so the trailing-garbage case slips through "valid integer" validation. UseNumber(raw)+Number.isInteger(...)(or a^-?\d+$regex) for strict parsing.- No lower-bound check — negative
limit/offsetwill reach the underlying query. If the query is unbounded by the service layer, this is a minor DoS / unexpected-behavior vector.🛡️ Suggested stricter parsing
- const limit = limitRaw === undefined ? null : Number.parseInt(limitRaw, 10); - const offset = offsetRaw === undefined ? 0 : Number.parseInt(offsetRaw, 10); - - if (limitRaw !== undefined && Number.isNaN(limit)) { - throw new AppError('limit must be a valid integer.', { - code: 'INVALID_QUERY_PARAMETER', - statusCode: 400, - }); - } - - if (offsetRaw !== undefined && Number.isNaN(offset)) { - throw new AppError('offset must be a valid integer.', { - code: 'INVALID_QUERY_PARAMETER', - statusCode: 400, - }); - } + const parseNonNegativeInt = (raw: string | undefined, name: string): number | null => { + if (raw === undefined) return null; + const n = Number(raw); + if (!Number.isInteger(n) || n < 0) { + throw new AppError(`${name} must be a non-negative integer.`, { + code: 'INVALID_QUERY_PARAMETER', + statusCode: 400, + }); + } + return n; + }; + const limit = parseNonNegativeInt(limitRaw, 'limit'); + const offset = parseNonNegativeInt(offsetRaw, 'offset') ?? 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/provider.routes.ts` around lines 306 - 329, The current parsing uses Number.parseInt which allows trailing characters and lacks lower-bound checks; update the parsing of limitRaw/offsetRaw (coming from readOptionalQueryString) to first convert with Number(value) and validate with Number.isInteger(...) (or a /^\-?\d+$/ regex) to reject values like "5abc", and add non-negative checks (limit >= 0 and offset >= 0) before calling sessionsService.fetchHistory; on invalid input throw the same AppError (code 'INVALID_QUERY_PARAMETER', statusCode 400) with a clear message referencing which parameter is invalid.server/modules/database/connection.ts-73-88 (1)
73-88:⚠️ Potential issue | 🟡 MinorMinor: legacy migration may leave WAL/SHM half-copied on error.
If the main
.dbcopy succeeds but-wal/-shmcopy fails, the catch logs a warning and proceeds. With WAL contents not fully migrated, the resulting DB could be missing recent transactions. As a one-time best-effort migration this may be acceptable, but consider rolling back the partial copy (deleting the new.dbso the next start retries) to avoid masking the issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/connection.ts` around lines 73 - 88, The migration currently copies the main DB (fs.copyFileSync(legacyPath, targetPath)) then attempts -wal/-shm copies and only logs on error, risking a partial migration; change this to perform a transactional migration by either copying -wal and -shm before finalizing the new DB or, if you keep the current order, catch any error during the suffix loop and rollback by deleting targetPath and any targetPath + '-wal' / '-shm' files that were created, then rethrow or surface the error instead of silently proceeding; use the existing legacyPath and targetPath variables and the suffix loop to detect which files were created and remove them in the error handling path.server/modules/projects/services/project-delete.service.ts-13-27 (1)
13-27:⚠️ Potential issue | 🟡 MinorMinor:
path.resolve(raw)for relative paths depends on CWD.If
jsonl_pathis ever stored relative (legacy data, future regressions),path.resolve(raw)resolves againstprocess.cwd(), which can differ between writers and this delete path — silently turning into ENOENT and skipping cleanup. Either reject non-absolute values explicitly or resolve them against a known base (e.g., user home / provider root) so failures are visible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/projects/services/project-delete.service.ts` around lines 13 - 27, The loop that builds absolute JSONL paths (using jsonl_path, path.isAbsolute and path.resolve) can mistakenly resolve relative paths against process.cwd(), leading to silent ENOENTs; update the logic in the function that iterates over sessions to either (a) explicitly reject non-absolute jsonl_path values by logging/throwing an error so deletion failures are visible, or (b) resolve relative paths against a deterministic base (e.g., a configured projectRoot/provider root or os.homedir()) instead of path.resolve(process.cwd()), and ensure the error path reports the original value and chosen base; keep the seen deduplication and result.push(absolute) behavior unchanged.server/modules/database/migrations.ts-99-102 (1)
99-102:⚠️ Potential issue | 🟡 Minor
COALESCE(projects.isStarred, excluded.isStarred)is a no-op for non-null columns.
projects.isStarredis declared withDEFAULT 0and is conventionally neverNULL, so the COALESCE always returns the existing value. If the intent is "preserve already-starred state", that's redundant (just writeisStarred = projects.isStarredor omit the column from the SET); if the intent is "OR the two states together so a star in either source wins", you actually wantMAX(projects.isStarred, excluded.isStarred). Same observation applies tocustom_project_nameif it has a NOT NULL constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/migrations.ts` around lines 99 - 102, The ON CONFLICT SET in server/modules/database/migrations.ts is using COALESCE(projects.isStarred, excluded.isStarred) (and similarly for custom_project_name) which is a no-op when projects.isStarred is non-null with DEFAULT 0; change the SET to express the intended semantics: if you want to preserve existing starred state omit updating isStarred or set isStarred = projects.isStarred, and if you want a star to win use an aggregate like MAX(projects.isStarred, excluded.isStarred) instead of COALESCE; apply the same logic to custom_project_name depending on its NULLability (omit update if you want to preserve, or use COALESCE correctly only if NULLs are expected).server/modules/projects/services/project-management.service.ts-116-119 (1)
116-119:⚠️ Potential issue | 🟡 MinorCustom-name fallback is double-applied; persisted name will never be empty.
resolveDisplayNameis invoked twice for the create flow:
- Line 118 normalizes
input.customNameto a non-empty string, falling back tobasename(resolvedProjectPath).mapProjectRowToApiView(line 79) re-applies the same fallback when reading back from DB.Net effect: callers can never persist an empty
custom_project_nameon create — the row will always carry the directory basename. If that's intended, fine; but it diverges fromupdateProjectDisplayName(line 147–150) which explicitly storesnullfor empty input. Consider passing the raw trimmedcustomName(ornull) topersistProjectPathand letting display-name derivation happen on read only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/projects/services/project-management.service.ts` around lines 116 - 119, Currently resolveDisplayName is called before persisting which forces a non-empty name into persistProjectPath; instead pass the raw trimmed input (or null) so display-name fallback is applied only on read. Replace normalizedCustomName usage in the create flow by trimming input.customName and converting empty -> null, then call dependencies.persistProjectPath(resolvedProjectPath, <trimmedOrNull>) and keep resolveDisplayName only in mapProjectRowToApiView (and preserve updateProjectDisplayName behavior that stores null for empty input).server/modules/database/repositories/notification-preferences.ts-59-65 (1)
59-65:⚠️ Potential issue | 🟡 MinorRace on first-read insert can throw on
user_idUNIQUE constraint.If two requests for the same user race on first read, both observe
!row, both attempt the bareINSERT, and the loser will fail theuser_idUNIQUE/PK constraint instead of returning defaults. Make this idempotent withON CONFLICT(user_id) DO NOTHING(orINSERT OR IGNORE).🛠️ Proposed fix
- if (!row) { - const defaults = normalizeNotificationPreferences(DEFAULT_NOTIFICATION_PREFERENCES); - db.prepare( - 'INSERT INTO user_notification_preferences (user_id, preferences_json, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP)' - ).run(userId, JSON.stringify(defaults)); - return defaults; - } + if (!row) { + const defaults = normalizeNotificationPreferences(DEFAULT_NOTIFICATION_PREFERENCES); + db.prepare( + `INSERT INTO user_notification_preferences (user_id, preferences_json, updated_at) + VALUES (?, ?, CURRENT_TIMESTAMP) + ON CONFLICT(user_id) DO NOTHING` + ).run(userId, JSON.stringify(defaults)); + return defaults; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/repositories/notification-preferences.ts` around lines 59 - 65, The INSERT in the no-row branch can race and violate the user_id UNIQUE constraint; change the insertion to be idempotent (use "ON CONFLICT(user_id) DO NOTHING" or "INSERT OR IGNORE" on the user_notification_preferences insert prepared via db.prepare) and after the insert re-query the row (or return the defaults if still absent) so the function (the code around the existing db.prepare(...) and return defaults) never throws on concurrent first-read inserts; keep the same preferences_json and CURRENT_TIMESTAMP behavior and ensure you return the stored row if the conflict prevented inserting.server/modules/database/schema.ts-82-96 (1)
82-96:⚠️ Potential issue | 🟡 MinorComposite PK
(session_id, provider)vs. session_id-only lookups elsewhere.The PK allows two rows to share a
session_idacross different providers, but the new service layer (sessions.service.ts) and several callers query/delete strictly bysession_id(and the indexidx_session_ids_lookupis also single-column). In practice provider UUIDs won't collide, but the schema doesn't guarantee it — any future provider that picks short or sequential ids would silently corrupt cross-provider lookups. Consider either:
- Making
session_iditselfUNIQUE(dropproviderfrom the PK), if a session id is meant to be globally unique, or- Keeping the composite PK and updating the lookup helpers (and downstream services) to take
(sessionId, provider).Also applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/schema.ts` around lines 82 - 96, The sessions table currently defines a composite PK (session_id, provider) in SESSIONS_TABLE_SCHEMA_SQL while the service layer (sessions.service.ts) and callers/query/index (idx_session_ids_lookup) operate only by session_id; either make session_id globally unique by removing provider from the PRIMARY KEY and adding UNIQUE(session_id) so existing single-column lookups remain correct, or keep the composite PK and update all lookup/delete code in sessions.service.ts plus the idx_session_ids_lookup index to accept and use both (session_id, provider) parameters so every query/delete includes provider; choose one approach and apply it consistently across SESSIONS_TABLE_SCHEMA_SQL, sessions.service.ts, and any index definitions.server/modules/projects/services/project-clone.service.ts-242-248 (1)
242-248:⚠️ Potential issue | 🟡 Minor
lastErroris clobbered by empty/whitespace stderr chunks.Each stderr
dataevent unconditionally overwriteslastError, including whenmessageis an empty string aftertrim(). Git frequently emits trailing newlines as separate chunks, so the meaningful failure (e.g.Authentication failed,Repository not found) gets replaced by'', andresolveCloneFailureMessagethen falls through to the generic'Git clone failed'.🐛 Proposed fix
gitProcess.stderr?.on('data', (data: Buffer | string) => { const message = data.toString().trim(); - lastError = message; if (message) { + lastError = message; handlers.onProgress(message); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/projects/services/project-clone.service.ts` around lines 242 - 248, The stderr data handler currently sets lastError for every chunk, which lets empty/whitespace chunks clobber prior meaningful errors; update the gitProcess.stderr?.on('data', ...) handler so it trims the chunk into message, calls handlers.onProgress(message) when message is non-empty, and only assigns lastError when message is non-empty; this preserves earlier non-empty stderr (used by resolveCloneFailureMessage) and still reports progress via handlers.onProgress.server/modules/projects/services/projects-with-sessions-fetch.service.ts-163-183 (1)
163-183:⚠️ Potential issue | 🟡 MinorSnapshots accumulate forever — no rotation/cleanup.
writeSnapshotis called on everygetProjectsWithSessions(i.e., every/api/projectsrequest). For long-running installations this fills.tmp/project-dumps/indefinitely. The counter also grows without bound (4-digitpadStart→ first overflow at 10000, after which paths still work but ordering breaks).Consider one of:
- Keep only the N most recent snapshots (delete older entries inside
getNextProjectsSnapshotPath).- Make snapshot dumping opt-in via env flag (looks like a debug aid, not a runtime feature).
- Throttle to e.g. once per minute.
Want me to draft a small ring-buffer cleanup that prunes to the last N files?
server/modules/database/repositories/sessions.db.ts-118-131 (1)
118-131:⚠️ Potential issue | 🟡 MinorProvider loaders should disambiguate sessions by provider when available.
createSessionupserts on(session_id, provider), so the samesession_idcan legitimately exist for multiple providers.getSessionByIdpicks the most-recently-updated row regardless of provider, which can return the wrong row if a session id collides across providers (e.g., Cursor sessions). The provider-specific loaders (gemini, claude, codex) all have access to theirPROVIDERconstant at call sites but pass onlysession_idtogetSessionById.Add a
getSessionByIdAndProvider(sessionId, provider)overload and use it in the provider loaders:
gemini-sessions.provider.tsline 11claude-sessions.provider.tsline 110codex-sessions.provider.tsline 65The provider-agnostic
sessionsServicemethods can continue usinggetSessionByIdsince they intentionally resolve the provider from the result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/repositories/sessions.db.ts` around lines 118 - 131, Add a provider-disambiguating lookup by implementing getSessionByIdAndProvider(sessionId: string, provider: string) that queries WHERE session_id = ? AND provider = ? (mirroring the existing getSessionById return type and behavior) and keep the existing provider-agnostic getSessionById for callers that intentionally resolve provider from results; then update the provider-specific loaders to call getSessionByIdAndProvider instead of getSessionById in the gemini, claude, and codex loaders (they each have access to their PROVIDER constant) so the upsert key used by createSession (session_id, provider) is respected and collisions across providers are avoided.server/modules/projects/services/projects-with-sessions-fetch.service.ts-107-127 (1)
107-127:⚠️ Potential issue | 🟡 Minor
messageCountis hard-coded to 0 for every session.The sidebar UI renders a message-count badge only when
messageCount > 0(SidebarSessionItem.tsx lines 103–107, 147–154). With the current implementation always returning 0, the badge will never display. Either populatemessageCountby computing it from session artifacts when synchronizing sessions, or remove the field fromSessionSummaryto prevent misleading consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/projects/services/projects-with-sessions-fetch.service.ts` around lines 107 - 127, The code is hard-coding messageCount to 0 for every session which prevents the message-count badge from ever showing; update the loop in projects-with-sessions-fetch.service.ts to populate messageCount from the actual data source (e.g., use an existing DB column like row.message_count or derive it from session artifact/message arrays if rows include them) and assign that value to the SessionSummary objects pushed into byProvider, or if no reliable source exists remove messageCount from the SessionSummary type and from consumers (e.g., SidebarSessionItem.tsx) to avoid misleading data. Ensure you update the SessionSummary references and the push block (where id/summary/lastActivity are set) to reflect the chosen fix.server/projects.js-16-25 (1)
16-25:⚠️ Potential issue | 🟡 MinorStale references to removed manual-project-registration helper.
The header doc still lists
addProjectManuallyand the~/.claude/project-config.jsonsync as features served by this module, but per the PR summary that helper has been removed and replaced by DB-backed project creation. The bullet should be dropped (or rewritten to point at the new location) so future readers don't go hunting for code that isn't here.📝 Suggested doc cleanup
* - (Project row removal / JSONL cleanup is handled in * `modules/projects/services/project-delete.service.ts`.) - * - Manual project registration (`addProjectManually`) which syncs to - * ~/.claude/project-config.json for backwards compatibility. */Note that
loadProjectConfig/saveProjectConfigare still kept around becauseextractProjectDirectoryandsearchConversationsconsult~/.claude/project-config.json; if the intent was to fully retire manual registration, you may also want to confirm those readers are still required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/projects.js` around lines 16 - 25, Update the module header comment to remove or rewrite the stale bullet about the removed addProjectManually helper and syncing to ~/.claude/project-config.json; instead mention the current DB-backed project creation location (or simply drop the bullet). While editing, keep notes that loadProjectConfig and saveProjectConfig remain for extractProjectDirectory and searchConversations if those still read ~/.claude/project-config.json, but do not re-introduce addProjectManually—reference the functions loadProjectConfig, saveProjectConfig, extractProjectDirectory, and searchConversations so reviewers can verify remaining filesystem readers.server/modules/database/repositories/projects.db.ts-105-112 (1)
105-112:⚠️ Potential issue | 🟡 MinorRemove unused
updateCustomProjectNamemethod or clarify its intended use.This method has zero callers in the codebase. If it's intended as part of the public API, it should either be removed or its implementation corrected—the
INSERT ... ON CONFLICT UPDATEpattern is problematic because it creates project rows for unknown paths (with auto-generated IDs), unlike the correctly-designed sibling methodupdateCustomProjectNameById(lines 114–121) which uses a plainUPDATE. If this method should exist, preferUPDATEand let callers go throughcreateProjectPathfirst when the row doesn't exist. Otherwise, delete it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/database/repositories/projects.db.ts` around lines 105 - 112, The method updateCustomProjectName currently does an INSERT ... ON CONFLICT which creates new project rows with a random UUID for unknown projectPath and has no callers; either remove this unused method or change its behavior to match the intended API: replace the INSERT/ON CONFLICT logic with a plain UPDATE against the projects table (similar to updateCustomProjectNameById) so callers must create rows explicitly via createProjectPath first; update or delete updateCustomProjectName accordingly and ensure any public API surfaces and tests reflect the chosen behavior.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/index.js (3)
1558-1591:⚠️ Potential issue | 🔴 CriticalCritical:
closeSessionsWatcher()runs at startup, not on shutdown — watcher never gets cleaned up.
server.listen(...)returns synchronously and only invokes theasynccallback later when the port is actually bound. As a result,await closeSessionsWatcher()on line 1580 executes immediately afterserver.listenis called — before (and concurrently with) the listen callback that runsinitializeSessionsWatcher(). Net effect:
- At startup,
closeSessionsWatcher()runs against an empty/uninitialized watcher state (best case a no-op, worst case races with initialization).- On
SIGTERM/SIGINT,shutdownPluginsonly callsstopAllPlugins()— the chokidar watcher insideinitializeSessionsWatcheris never closed, so file handles and the polling interval keep running and the process may not exit cleanly.The cleanup belongs inside the signal handlers.
🛠️ Suggested fix
server.listen(SERVER_PORT, HOST, async () => { // ... existing logging ... // Start watching the projects folder for changes await initializeSessionsWatcher(); // Start server-side plugin processes for enabled plugins startEnabledPluginServers().catch(err => { console.error('[Plugins] Error during startup:', err.message); }); }); - await closeSessionsWatcher(); - // Clean up plugin processes on shutdown - const shutdownPlugins = async () => { - await stopAllPlugins(); - process.exit(0); - }; - process.on('SIGTERM', () => void shutdownPlugins()); - process.on('SIGINT', () => void shutdownPlugins()); + // Clean up plugin processes and session watcher on shutdown + const shutdown = async () => { + try { + await closeSessionsWatcher(); + } catch (err) { + console.error('[Shutdown] Error closing sessions watcher:', err); + } + try { + await stopAllPlugins(); + } catch (err) { + console.error('[Shutdown] Error stopping plugins:', err); + } + process.exit(0); + }; + process.on('SIGTERM', () => void shutdown()); + process.on('SIGINT', () => void shutdown());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 1558 - 1591, The call to await closeSessionsWatcher() runs immediately after server.listen instead of on shutdown, racing with initializeSessionsWatcher(); remove that top-level await and instead update shutdownPlugins (the function invoked by process.on('SIGTERM'/'SIGINT')) to await closeSessionsWatcher() along with await stopAllPlugins(), then call process.exit(0). Ensure the server.listen async callback still awaits initializeSessionsWatcher(), and handle errors from shutdownPlugins (e.g., catch/log) to guarantee the watcher is closed before exiting.
313-333:⚠️ Potential issue | 🟡 MinorRemove the legacy
/api/sessions/:sessionId/renameroute since it's no longer used.No frontend clients call this legacy endpoint. The new
PUT /api/providers/sessions/:sessionIdroute (in provider.routes.ts) is the intended replacement and is properly mounted with authentication middleware. Both routes currently invokesessionsDb.updateSessionCustomName()directly, so keeping the legacy route creates duplicate rename logic. Remove it to establish a single source of truth for session rename behavior.Note: Both the legacy and new routes lack per-session ownership enforcement. This is a pre-existing gap across session-scoped actions tracked separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 313 - 333, Remove the legacy route handler for app.put('/api/sessions/:sessionId/rename'): delete the entire route block that uses authenticateToken and calls sessionsDb.updateSessionCustomName(...) so the new single source of truth (PUT /api/providers/sessions/:sessionId in provider.routes.ts) is used instead; ensure any imports or references rendered unused by removing this block (e.g., authenticateToken or route-specific comments) are cleaned up to avoid linter errors.
297-310:⚠️ Potential issue | 🟡 MinorConfirm deletion ordering — DB first is safer for UI consistency.
sessionsDb.deleteSessionById()is synchronous (better-sqlite3 style usingdb.prepare().run()), so the unawaited call on line 303 is correct and not a problem.However, the deletion order is inverted: filesystem first (line 302), then DB (line 303). If
deleteSessionByIdthrows after partially mutating the JSONL file, the DB row is never deleted and the session reappears. Conversely, if the DB call throws, the JSONL is gone but the row lingers as an orphaned entry.Consider reversing the order — delete from DB first (as the source-of-truth for what sessions exist), then best-effort cleanup of disk artifacts. This way, if either operation fails, the DB and UI remain consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 297 - 310, The deletion order should be reversed so the DB (sessionsDb.deleteSessionById) is the primary operation and filesystem cleanup (deleteSessionById(projectId, sessionId)) is best-effort; modify the route handler to call sessionsDb.deleteSessionById(sessionId) first and only if it succeeds attempt await deleteSessionById(projectId, sessionId) inside its own try/catch, logging any disk cleanup errors but still returning success if the DB delete succeeded; ensure that if sessionsDb.deleteSessionById throws the handler returns a 500 and does not attempt disk cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/index.js`:
- Around line 1558-1591: The call to await closeSessionsWatcher() runs
immediately after server.listen instead of on shutdown, racing with
initializeSessionsWatcher(); remove that top-level await and instead update
shutdownPlugins (the function invoked by process.on('SIGTERM'/'SIGINT')) to
await closeSessionsWatcher() along with await stopAllPlugins(), then call
process.exit(0). Ensure the server.listen async callback still awaits
initializeSessionsWatcher(), and handle errors from shutdownPlugins (e.g.,
catch/log) to guarantee the watcher is closed before exiting.
- Around line 313-333: Remove the legacy route handler for
app.put('/api/sessions/:sessionId/rename'): delete the entire route block that
uses authenticateToken and calls sessionsDb.updateSessionCustomName(...) so the
new single source of truth (PUT /api/providers/sessions/:sessionId in
provider.routes.ts) is used instead; ensure any imports or references rendered
unused by removing this block (e.g., authenticateToken or route-specific
comments) are cleaned up to avoid linter errors.
- Around line 297-310: The deletion order should be reversed so the DB
(sessionsDb.deleteSessionById) is the primary operation and filesystem cleanup
(deleteSessionById(projectId, sessionId)) is best-effort; modify the route
handler to call sessionsDb.deleteSessionById(sessionId) first and only if it
succeeds attempt await deleteSessionById(projectId, sessionId) inside its own
try/catch, logging any disk cleanup errors but still returning success if the DB
delete succeeded; ensure that if sessionsDb.deleteSessionById throws the handler
returns a 500 and does not attempt disk cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8cb32021-4e1f-42c8-8ece-d609ec9cf7c2
📒 Files selected for processing (2)
server/index.jsserver/modules/database/repositories/notification-preferences.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/modules/database/repositories/notification-preferences.ts
…plify session metadata UX Why this change was needed: - Project listing had an implicit side effect: every fetch wrote a debug snapshot under `.tmp/project-dumps`. That added unnecessary disk I/O to a hot path, introduced hidden runtime behavior, and created maintenance overhead for code that was not part of product functionality. - Keeping snapshot-specific exports/tests around made the projects module API broader than needed and coupled tests to temporary/debug behavior instead of user-visible behavior. - Codex sessions could remain stuck with a placeholder name (`Untitled Codex Session`) even after a real title became available from newer sync data, which degraded session discoverability in the UI. - Sidebar session rows showed duplicated provider branding and long-form relative times, which added visual noise and reduced scan speed when many sessions are listed. What changed: - Removed temporary projects snapshot dumping from `projects-with-sessions-fetch.service.ts`: - deleted snapshot types/helpers and file-write flow - removed the write call from `getProjectsWithSessions` - Removed snapshot-related surface area from `projects/index.ts`. - Removed the snapshot-focused test `projects.service.test.ts` that only validated removed debug behavior. - Updated `codex-session-synchronizer.provider.ts` to upgrade session names when an existing session still has the placeholder title but a real parsed name is now available. - Updated `SidebarSessionItem.tsx`: - removed duplicate provider logo rendering in each session row - moved age indicator to the right side - made age indicator fade on hover to prioritize action controls - switched to compact relative time format (`<1m`, `Xm`, `Xhr`, `Xd`) for faster list scanning Outcome: - Lower overhead and fewer hidden side effects in project fetches. - Cleaner module boundaries in projects. - Better Codex session naming consistency after sync. - Cleaner sidebar density and clearer hover/action behavior.
…//github.com/siteboon/claudecodeui into refactor/use-database-for-session-managemnet
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/index.js (2)
1511-1545:⚠️ Potential issue | 🔴 Critical
closeSessionsWatcher()runs at startup instead of shutdown — watcher is never closed cleanly.
server.listen(...)returns synchronously and only invokes its callback once the server is bound. So the actual order at runtime is:
server.listen(SERVER_PORT, HOST, asyncCallback)— non-blocking.await closeSessionsWatcher();— runs immediately, before the listen callback fires.- SIGTERM/SIGINT handlers registered.
- (later) listen callback fires →
await initializeSessionsWatcher();So
closeSessionsWatcheris invoked during boot (no-op on a cold start because nothing's been initialized yet), and the SIGTERM/SIGINT handlers (shutdownPlugins) only callstopAllPlugins()— the session watcher is never closed. On graceful shutdown the watcher's pending flush timer and chokidar handles leak.🐛 Suggested fix — move close into the shutdown path
- await closeSessionsWatcher(); // Clean up plugin processes on shutdown - const shutdownPlugins = async () => { - await stopAllPlugins(); - process.exit(0); - }; - process.on('SIGTERM', () => void shutdownPlugins()); - process.on('SIGINT', () => void shutdownPlugins()); + const shutdown = async () => { + try { + await closeSessionsWatcher(); + } catch (error) { + console.error('[ERROR] Failed to close sessions watcher:', error); + } + await stopAllPlugins(); + process.exit(0); + }; + process.on('SIGTERM', () => void shutdown()); + process.on('SIGINT', () => void shutdown());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 1511 - 1545, The closeSessionsWatcher() call is currently awaited at startup instead of during shutdown; remove the standalone await closeSessionsWatcher() after server.listen and instead call and await closeSessionsWatcher() inside the shutdown routine (shutdownPlugins) so the watcher is closed on SIGTERM/SIGINT; update shutdownPlugins to await stopAllPlugins(), await closeSessionsWatcher(), and then exit (and consider also awaiting server.close() if server needs to be closed), and ensure the process.on('SIGTERM'/'SIGINT') handlers invoke this async shutdownPlugins correctly.
297-332:⚠️ Potential issue | 🟡 MinorRemove these legacy session endpoints—they're unreachable, duplicate the provider-aware routes, and the delete path only works for Claude.
src/utils/api.jsalready pointsdeleteSessionandrenameSessionat/api/providers/sessions/:sessionId(handled inprovider.routes.ts). The legacy endpoints are not called anywhere in the codebase:
DELETE /api/projects/:projectId/sessions/:sessionId(Line 297) — callsdeleteSessionByIdfromserver/projects.js, which only deletes Claude JSONL files and fails for Codex/Gemini sessions.PUT /api/sessions/:sessionId/rename(Line 312) — superseded by the provider-awarePUT /api/providers/sessions/:sessionId.Removing them matches the stated intent and eliminates unnecessary maintenance surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 297 - 332, Remove the two legacy Express routes that duplicate provider-aware endpoints: delete the app.delete('/api/projects/:projectId/sessions/:sessionId', ...) handler (which calls deleteSessionById and sessionsDb.deleteSessionById) and the app.put('/api/sessions/:sessionId/rename', ...) handler (which calls sessionsDb.updateSessionCustomName); ensure no other code depends on these routes (API consumer code in src/utils/api.js already targets /api/providers/sessions/:sessionId) and run tests to confirm provider-aware routes in provider.routes.ts handle deletion and renaming for all providers.server/projects.js (1)
65-526:⚠️ Potential issue | 🟡 MinorCode duplication: three utility functions appear to be replicated across modules.
The verification found that
findCodexJsonlFiles,normalizeComparablePath, andisVisibleCodexUserMessageare not actually dead—they are duplicated implementations inserver/modules/providers/services/session-conversations-search.service.tsandserver/modules/providers/list/codex/codex-sessions.provider.ts.Four functions are legitimately unreferenced:
getSessions(line 65)parseJsonlSessions(line 193)buildCodexSessionsIndex(line 452)getCodexSessions(line 504)Consolidate the duplicated utility functions into a shared location (e.g., a utilities module) to maintain a single source of truth and reduce maintenance burden. Remove the unreferenced functions entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/projects.js` around lines 65 - 526, Duplicate utility logic (findCodexJsonlFiles, normalizeComparablePath, isVisibleCodexUserMessage) should be consolidated into a single shared utilities module and the four unreferenced functions (getSessions, parseJsonlSessions, buildCodexSessionsIndex, getCodexSessions) should be removed from this file; to fix, extract the three duplicated functions into a new or existing shared utilities file (export them with the same names), update imports in server/modules/providers/services/session-conversations-search.service.ts, server/modules/providers/list/codex/codex-sessions.provider.ts and this file to import from the shared module, delete the duplicate implementations here, and remove the unused functions getSessions, parseJsonlSessions, buildCodexSessionsIndex, getCodexSessions from server/projects.js (ensuring no remaining references), then run the project linter/tests to validate imports and exports.
🧹 Nitpick comments (7)
server/modules/providers/services/session-conversations-search.service.ts (1)
54-69: Module-levelprojectDirectoryCacheis never invalidated.
projectDirectoryCache(Line 54) lives for the lifetime of the process. If a project's actual cwd is renamed/moved on disk after the first search, subsequent searches will keep using the stale path. Since this is best-effort metadata, not a correctness concern, but worth a TTL or invalidation hook tied tosessionsWatcherevents if those are visible here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/services/session-conversations-search.service.ts` around lines 54 - 69, The module-level Map projectDirectoryCache retains stale paths for the process lifetime, so update extractProjectDirectory to invalidate or refresh entries: implement a TTL (e.g., store timestamps with entries and evict when older than configured TTL) or subscribe to sessionsWatcher events and clear or remove specific project keys when renames/moves are detected; ensure extractProjectDirectory checks the cache age and falls back to recomputing the directory if expired, and add a clearProjectDirectory(projectName) helper and a sessionsWatcher hook to call it when relevant events occur.server/modules/projects/services/projects-with-sessions-fetch.service.ts (1)
192-256: Sequential per-project disk reads on everyGET /api/projectscall.
getProjectsWithSessionsdoes oneawait fs.readFile(packageJson)(and JSON parse) per project sequentially ingenerateDisplayName— for every request, even when no project changed. With many tracked projects this turns the list endpoint into an N-disk-IO loop on each call.Two optional improvements:
- Run the loop with
Promise.allto overlap I/O.- Cache the resolved
displayNameper(project_path, mtime of package.json)since this is rarely changing data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/projects/services/projects-with-sessions-fetch.service.ts` around lines 192 - 256, getProjectsWithSessions currently calls generateDisplayName (which does per-project fs.readFile) sequentially for each project causing N synchronous disk reads; change the implementation to build an array of promises (e.g. map over projectRows) and await Promise.all so disk I/O for display names and sessions (readProjectSessionsPageByPath) runs in parallel, while preserving progress updates; additionally introduce an in-memory cache (Map) keyed by `${project_path}:${packageJsonMtime}` where you check fs.stat(packageJson).mtimeMs before calling generateDisplayName and reuse the cached displayName when the mtime matches (update cache when different), referencing getProjectsWithSessions, generateDisplayName, projectsDb.getProjectPaths and readProjectSessionsPageByPath to locate spots to implement the Promise.all refactor and the caching logic.src/utils/api.js (2)
99-104: Auth token in query string forsearchConversationsUrl.Since this URL is consumed by EventSource (which can't attach an
Authorizationheader), passing the token via?token=...is a known compromise but it does end up in proxy/access logs andRefererheaders if any client follows redirects. If not already done elsewhere, consider:
- Issuing a short-lived single-use token specifically for SSE.
- Or proxying through a same-origin route that reads the cookie (if your auth scheme supports it).
Not a regression introduced by this PR — flagging as a follow-up since the URL surface moved here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/api.js` around lines 99 - 104, The helper searchConversationsUrl currently appends the auth token to the query string (in searchConversationsUrl), which exposes sensitive tokens in logs and referers; update its implementation to stop emitting the long-lived auth token in the URL: either (A) change callers to request a short-lived single-use SSE token from the server (e.g., call an endpoint that returns a one-time token) and have searchConversationsUrl accept and append only that short-lived token, or (B) remove token plumbing entirely and route EventSource connections through a same-origin proxy endpoint that reads the auth cookie/session on the server side and forwards SSE (so searchConversationsUrl no longer adds any token param). Ensure you update places that call searchConversationsUrl to follow the chosen flow and document the new single-use token/proxy contract.
76-156:encodeURIComponentis applied inconsistently across the project/file path API methods.Within this same object,
projectSessions,projectTaskmaster,toggleProjectStar, andunifiedSessionMessagesall run their id throughencodeURIComponent, butrenameProject,deleteSession,renameSession,deleteProject,readFile,readFileBlob,saveFile,getFiles,createFile,renameFile,deleteFile,uploadFiles, and thetaskmaster.*methods interpolate the id directly. Project IDs are likely UUIDs today, but the inconsistency makes future schema changes (or any non-UUID id, including session IDs that already accept._-) a hidden breakage. Recommend encoding everywhere for parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/api.js` around lines 76 - 156, Many API helper methods interpolate project or session IDs without URL-encoding, causing inconsistent behavior; update all calls that build URLs using projectId or sessionId to wrap those identifiers with encodeURIComponent to ensure safety. Specifically, change usages in renameProject, deleteSession, renameSession, deleteProject, readFile, readFileBlob, saveFile, getFiles, createFile, renameFile, deleteFile, uploadFiles and any taskmaster.* methods to use encodeURIComponent(projectId) or encodeURIComponent(sessionId) when constructing the endpoint string (and keep URLSearchParams usage in deleteProject/searchConversationsUrl unchanged); also ensure toggleProjectStar/unifiedSessionMessages style is used as the canonical pattern across authenticatedFetch invocations. Ensure you only encode the ID segments (not entire query strings or already-encoded parameters) so endpoints remain correct.src/components/sidebar/view/subcomponents/SidebarProjectItem.tsx (1)
54-100: LGTM!The migration to
project.projectId-based selection/editing keys is consistent throughout the component, andhasMoreSessionsis correctly derived fromproject.sessionMeta?.hasMorebefore passing it toSidebarProjectSessions.Tiny nit (optional):
getSessionCountDisplayand the inlinetotalSessionCounton line 98 redo the sameNumber(project.sessionMeta?.total ?? sessions.length)computation. You could collapse them into a single locally-scoped value to avoid the duplicated fallback logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/view/subcomponents/SidebarProjectItem.tsx` around lines 54 - 100, The session total computation is duplicated: getSessionCountDisplay and the local totalSessionCount both compute Number(project.sessionMeta?.total ?? sessions.length); remove the duplication by computing this once locally (e.g., in SidebarProjectItem as totalSessionCount) and reuse that value for sessionCountDisplay and sessionCountLabel, and delete or update getSessionCountDisplay so it no longer repeats the fallback logic.src/hooks/useProjectsState.ts (1)
709-725: Avoid capturing state from inside asetProjectsupdater.
mergedProjectForSelectionis assigned from within thesetProjectsupdater (line 717) and then read after to drivesetSelectedProject. Updater functions are expected to be pure — under React 18 StrictMode the updater is invoked twice in development, which works here only because the assignment is idempotent. It's still fragile and obscures intent.A cleaner version computes the merged project once outside the updater, or uses a separate
setSelectedProject(prev => ...)so no closure variable is needed:♻️ Alternative
- let mergedProjectForSelection: Project | null = null; setProjects((previousProjects) => previousProjects.map((candidate) => { if (candidate.projectId !== projectId) { return candidate; } - - const mergedProject = mergeProjectSessionPage(candidate, sessionsPage); - mergedProjectForSelection = mergedProject; - return mergedProject; + return mergeProjectSessionPage(candidate, sessionsPage); }), ); - if (selectedProject?.projectId === projectId && mergedProjectForSelection) { - setSelectedProject(mergedProjectForSelection); + if (selectedProject?.projectId === projectId) { + setSelectedProject((previous) => + previous && previous.projectId === projectId + ? mergeProjectSessionPage(previous, sessionsPage) + : previous, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useProjectsState.ts` around lines 709 - 725, The updater for setProjects assigns mergedProjectForSelection inside the mapping (using mergeProjectSessionPage) and then reads it afterwards to call setSelectedProject, which captures mutable state inside the updater; instead, first locate the target project from projects (or via a find on projects using projectId), compute mergedProject = mergeProjectSessionPage(foundProject, sessionsPage) outside of the setProjects updater, then call setProjects(prev => prev.map(candidate => candidate.projectId === projectId ? mergedProject : candidate)); finally, if selectedProject?.projectId === projectId call setSelectedProject(mergedProject) (or use setSelectedProject(prev => prev?.projectId === projectId ? mergedProject : prev)) so no external variable is assigned from within the setProjects updater.src/components/sidebar/view/subcomponents/SidebarProjectList.tsx (1)
30-37: StaleprojectNameparameter names in prop types after the projectId migration.After the migration, callers now invoke these with
project.projectId(seeSidebarProjectItem.tsxlines 103, 104, 107, and theisProjectStarred(project.projectId)call on line 127 of this file). The prop signatures still describe the argument asprojectName, which is misleading for future maintainers. Worth renaming toprojectIdfor consistency with the new contract.♻️ Proposed naming alignment
- isProjectStarred: (projectName: string) => boolean; + isProjectStarred: (projectId: string) => boolean; onEditingNameChange: (value: string) => void; - onToggleProject: (projectName: string) => void; + onToggleProject: (projectId: string) => void; onProjectSelect: (project: Project) => void; - onToggleStarProject: (projectName: string) => void; + onToggleStarProject: (projectId: string) => void; onStartEditingProject: (project: Project) => void; onCancelEditingProject: () => void; - onSaveProjectName: (projectName: string) => void; + onSaveProjectName: (projectId: string) => void;Apply the equivalent rename to
SidebarProjectItemProps(lines 31, 33, 36) for parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/view/subcomponents/SidebarProjectList.tsx` around lines 30 - 37, Props in SidebarProjectList's type definition still use the parameter name projectName even though callers now pass project.projectId; update the prop parameter names to projectId for all affected callbacks (isProjectStarred, onToggleProject, onToggleStarProject, onSaveProjectName if applicable) and mirror the same rename inside SidebarProjectItemProps (the corresponding props at lines showing those entries) so signatures reflect the new contract; keep types unchanged (string) but rename the parameter identifier to projectId in the prop declarations and any related JSDoc/comments for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/modules/database/repositories/sessions.db.ts`:
- Around line 100-201: Several single-row methods treat session_id as globally
unique while the schema uses (session_id, provider) as the unique key;
updateSessionCustomName, getSessionById, and deleteSessionById must be made
provider-aware to avoid affecting multiple rows. Change the signatures of
updateSessionCustomName(sessionId, customName), getSessionById(sessionId) and
deleteSessionById(sessionId) to accept a provider string, update their SQL to
add "AND provider = ?" to the WHERE clauses, and update all callers (notably
sessionsService.deleteSessionById and sessionsService.renameSessionById which
already have session.provider) to pass the provider; alternatively, if you
prefer a schema change instead, make session_id globally unique and remove
provider from those WHERE clauses consistently across the repository and
callers.
- Around line 60-98: The createSession flow uses normalizeProjectPathForProvider
(in sessionsDb.createSession) which only strips Windows long-path prefixes for
the "codex" provider, causing inconsistent project_path normalization vs
createProject/validateWorkspacePath and agent.js; fix by centralizing
normalization: update normalizeProjectPathForProvider (or create a new
normalizeProjectPath utility) to always strip the Windows "\\\\?\\ " long-path
prefix and normalize/trim trailing slashes for any provider, and call that
shared normalizer from sessionsDb.createSession, projectsDb.createProjectPath
(and replace uses in createProject/validateWorkspacePath/agent.js) so all
inserts/lookup use the exact same canonical path form to avoid duplicate project
rows and orphaned sessions.
In `@server/modules/providers/provider.routes.ts`:
- Around line 338-364: The handler currently converts limit and offset with
Number.parseInt but only checks for NaN, allowing negative values; update the
validation to require non-negative integers (match the behavior of
parseSessionSearchLimit and parseNonNegativeIntQuery) by checking that parsed
limit and offset are >= 0 and throwing an AppError with code
'INVALID_QUERY_PARAMETER' and statusCode 400 when negative; locate the parsing
logic using readOptionalQueryString, the variables limit/offset, and the call to
sessionsService.fetchHistory to apply the new non-negative checks before calling
fetchHistory.
In `@src/components/sidebar/hooks/useSidebarController.ts`:
- Around line 196-222: The effect can trigger duplicate migrations when
onRefresh identity changes mid-flight; fix by making the migration one-shot
using a ref guard and calling the latest onRefresh via a ref: add a
migrationStartedRef (e.g., migrationStartedRef.current) and if true return
early, set it to true before calling
api.migrateLegacyProjectStars(readLegacyStarredProjectIds()), store the latest
onRefresh in an onRefreshRef and call onRefreshRef.current() after the API call,
and ensure clearLegacyStarredProjectIds() runs once (in finally) — reference
readLegacyStarredProjectIds, api.migrateLegacyProjectStars,
onRefresh/onRefreshRef, clearLegacyStarredProjectIds, and migrationStartedRef to
locate the changes.
- Around line 442-465: The TOCTOU bug in loadMoreSessionsForProject lets two
concurrent calls pass the initial loadingMoreProjects.has(projectId) check and
trigger duplicate onLoadMoreSessions requests; fix this by making the membership
check atomic inside the state updater: use setLoadingMoreProjects(prev => { if
(prev.has(projectId)) return prev; const next = new Set(prev);
next.add(projectId); return next; }) and only proceed to call onLoadMoreSessions
when the updater added the project, then remove the project in the finally block
as before; update the loadMoreSessionsForProject closure to rely on the
updater-returned decision rather than the outer loadingMoreProjects variable to
prevent duplicate fetches.
In `@src/hooks/useProjectsState.ts`:
- Around line 606-625: The current setProjects callback inside
handleSessionDelete unconditionally decrements project.sessionMeta.total for
every project; instead, only update the owning project where the session was
actually removed: inside the prevProjects.map used by setProjects, compute the
new filtered arrays (sessions, cursorSessions, codexSessions, geminiSessions)
and compare their total loaded count to the original project (or check if any
filtered length is less than the original); if no array changed, return the
original project unchanged; if at least one array lost an item
(sessionIdToDelete found), build updatedProject and set sessionMeta.total to
Math.max(0, Number(project.sessionMeta?.total ?? 0) - 1) and recompute hasMore
using countLoadedProjectSessions(updatedProject). This ensures only the owning
project’s sessionMeta and hasMore are modified.
---
Outside diff comments:
In `@server/index.js`:
- Around line 1511-1545: The closeSessionsWatcher() call is currently awaited at
startup instead of during shutdown; remove the standalone await
closeSessionsWatcher() after server.listen and instead call and await
closeSessionsWatcher() inside the shutdown routine (shutdownPlugins) so the
watcher is closed on SIGTERM/SIGINT; update shutdownPlugins to await
stopAllPlugins(), await closeSessionsWatcher(), and then exit (and consider also
awaiting server.close() if server needs to be closed), and ensure the
process.on('SIGTERM'/'SIGINT') handlers invoke this async shutdownPlugins
correctly.
- Around line 297-332: Remove the two legacy Express routes that duplicate
provider-aware endpoints: delete the
app.delete('/api/projects/:projectId/sessions/:sessionId', ...) handler (which
calls deleteSessionById and sessionsDb.deleteSessionById) and the
app.put('/api/sessions/:sessionId/rename', ...) handler (which calls
sessionsDb.updateSessionCustomName); ensure no other code depends on these
routes (API consumer code in src/utils/api.js already targets
/api/providers/sessions/:sessionId) and run tests to confirm provider-aware
routes in provider.routes.ts handle deletion and renaming for all providers.
In `@server/projects.js`:
- Around line 65-526: Duplicate utility logic (findCodexJsonlFiles,
normalizeComparablePath, isVisibleCodexUserMessage) should be consolidated into
a single shared utilities module and the four unreferenced functions
(getSessions, parseJsonlSessions, buildCodexSessionsIndex, getCodexSessions)
should be removed from this file; to fix, extract the three duplicated functions
into a new or existing shared utilities file (export them with the same names),
update imports in
server/modules/providers/services/session-conversations-search.service.ts,
server/modules/providers/list/codex/codex-sessions.provider.ts and this file to
import from the shared module, delete the duplicate implementations here, and
remove the unused functions getSessions, parseJsonlSessions,
buildCodexSessionsIndex, getCodexSessions from server/projects.js (ensuring no
remaining references), then run the project linter/tests to validate imports and
exports.
---
Nitpick comments:
In `@server/modules/projects/services/projects-with-sessions-fetch.service.ts`:
- Around line 192-256: getProjectsWithSessions currently calls
generateDisplayName (which does per-project fs.readFile) sequentially for each
project causing N synchronous disk reads; change the implementation to build an
array of promises (e.g. map over projectRows) and await Promise.all so disk I/O
for display names and sessions (readProjectSessionsPageByPath) runs in parallel,
while preserving progress updates; additionally introduce an in-memory cache
(Map) keyed by `${project_path}:${packageJsonMtime}` where you check
fs.stat(packageJson).mtimeMs before calling generateDisplayName and reuse the
cached displayName when the mtime matches (update cache when different),
referencing getProjectsWithSessions, generateDisplayName,
projectsDb.getProjectPaths and readProjectSessionsPageByPath to locate spots to
implement the Promise.all refactor and the caching logic.
In `@server/modules/providers/services/session-conversations-search.service.ts`:
- Around line 54-69: The module-level Map projectDirectoryCache retains stale
paths for the process lifetime, so update extractProjectDirectory to invalidate
or refresh entries: implement a TTL (e.g., store timestamps with entries and
evict when older than configured TTL) or subscribe to sessionsWatcher events and
clear or remove specific project keys when renames/moves are detected; ensure
extractProjectDirectory checks the cache age and falls back to recomputing the
directory if expired, and add a clearProjectDirectory(projectName) helper and a
sessionsWatcher hook to call it when relevant events occur.
In `@src/components/sidebar/view/subcomponents/SidebarProjectItem.tsx`:
- Around line 54-100: The session total computation is duplicated:
getSessionCountDisplay and the local totalSessionCount both compute
Number(project.sessionMeta?.total ?? sessions.length); remove the duplication by
computing this once locally (e.g., in SidebarProjectItem as totalSessionCount)
and reuse that value for sessionCountDisplay and sessionCountLabel, and delete
or update getSessionCountDisplay so it no longer repeats the fallback logic.
In `@src/components/sidebar/view/subcomponents/SidebarProjectList.tsx`:
- Around line 30-37: Props in SidebarProjectList's type definition still use the
parameter name projectName even though callers now pass project.projectId;
update the prop parameter names to projectId for all affected callbacks
(isProjectStarred, onToggleProject, onToggleStarProject, onSaveProjectName if
applicable) and mirror the same rename inside SidebarProjectItemProps (the
corresponding props at lines showing those entries) so signatures reflect the
new contract; keep types unchanged (string) but rename the parameter identifier
to projectId in the prop declarations and any related JSDoc/comments for
clarity.
In `@src/hooks/useProjectsState.ts`:
- Around line 709-725: The updater for setProjects assigns
mergedProjectForSelection inside the mapping (using mergeProjectSessionPage) and
then reads it afterwards to call setSelectedProject, which captures mutable
state inside the updater; instead, first locate the target project from projects
(or via a find on projects using projectId), compute mergedProject =
mergeProjectSessionPage(foundProject, sessionsPage) outside of the setProjects
updater, then call setProjects(prev => prev.map(candidate => candidate.projectId
=== projectId ? mergedProject : candidate)); finally, if
selectedProject?.projectId === projectId call setSelectedProject(mergedProject)
(or use setSelectedProject(prev => prev?.projectId === projectId ? mergedProject
: prev)) so no external variable is assigned from within the setProjects
updater.
In `@src/utils/api.js`:
- Around line 99-104: The helper searchConversationsUrl currently appends the
auth token to the query string (in searchConversationsUrl), which exposes
sensitive tokens in logs and referers; update its implementation to stop
emitting the long-lived auth token in the URL: either (A) change callers to
request a short-lived single-use SSE token from the server (e.g., call an
endpoint that returns a one-time token) and have searchConversationsUrl accept
and append only that short-lived token, or (B) remove token plumbing entirely
and route EventSource connections through a same-origin proxy endpoint that
reads the auth cookie/session on the server side and forwards SSE (so
searchConversationsUrl no longer adds any token param). Ensure you update places
that call searchConversationsUrl to follow the chosen flow and document the new
single-use token/proxy contract.
- Around line 76-156: Many API helper methods interpolate project or session IDs
without URL-encoding, causing inconsistent behavior; update all calls that build
URLs using projectId or sessionId to wrap those identifiers with
encodeURIComponent to ensure safety. Specifically, change usages in
renameProject, deleteSession, renameSession, deleteProject, readFile,
readFileBlob, saveFile, getFiles, createFile, renameFile, deleteFile,
uploadFiles and any taskmaster.* methods to use encodeURIComponent(projectId) or
encodeURIComponent(sessionId) when constructing the endpoint string (and keep
URLSearchParams usage in deleteProject/searchConversationsUrl unchanged); also
ensure toggleProjectStar/unifiedSessionMessages style is used as the canonical
pattern across authenticatedFetch invocations. Ensure you only encode the ID
segments (not entire query strings or already-encoded parameters) so endpoints
remain correct.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d97b5f8-29fd-4293-97c8-d8cff20bc6a7
📒 Files selected for processing (18)
server/index.jsserver/modules/database/repositories/sessions.db.tsserver/modules/projects/index.tsserver/modules/projects/projects.routes.tsserver/modules/projects/services/projects-with-sessions-fetch.service.tsserver/modules/providers/list/codex/codex-session-synchronizer.provider.tsserver/modules/providers/provider.routes.tsserver/modules/providers/services/session-conversations-search.service.tsserver/projects.jssrc/components/sidebar/hooks/useSidebarController.tssrc/components/sidebar/types/types.tssrc/components/sidebar/view/Sidebar.tsxsrc/components/sidebar/view/subcomponents/SidebarProjectItem.tsxsrc/components/sidebar/view/subcomponents/SidebarProjectList.tsxsrc/components/sidebar/view/subcomponents/SidebarProjectSessions.tsxsrc/components/sidebar/view/subcomponents/SidebarSessionItem.tsxsrc/hooks/useProjectsState.tssrc/utils/api.js
✅ Files skipped from review due to trivial changes (1)
- server/modules/projects/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- server/modules/providers/list/codex/codex-session-synchronizer.provider.ts
- src/components/sidebar/view/subcomponents/SidebarSessionItem.tsx
- src/components/sidebar/view/subcomponents/SidebarProjectSessions.tsx
- src/components/sidebar/view/Sidebar.tsx
- src/components/sidebar/types/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/sidebar/hooks/useSidebarController.ts (1)
280-284: Unreachable sequence guard right after the increment.const seq = ++searchSeqRef.current; if (seq !== searchSeqRef.current) { return; }
seqis assigned the post-increment value ofsearchSeqRef.current, soseq !== searchSeqRef.currentis alwaysfalseat this point and the early return can never fire. Looks like a leftover from a refactor — the real staleness checks already live inside each SSE event handler (if (seq !== searchSeqRef.current) { es.close(); return; }). Safe to drop these three lines to avoid confusion; if the intent was to gate against a re-render that bumped the seq before reaching here, the check would need to live after anawait/state read, not immediately after the assignment.♻️ Suggested cleanup
setIsSearching(true); const seq = ++searchSeqRef.current; - if (seq !== searchSeqRef.current) { - return; - } - const url = api.searchConversationsUrl(query);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/sidebar/hooks/useSidebarController.ts` around lines 280 - 284, Remove the unreachable post-increment guard by deleting the three lines that set const seq = ++searchSeqRef.current; and immediately check if (seq !== searchSeqRef.current) { return; } inside useSidebarController; the correct staleness checks already occur in the SSE handlers (e.g., the existing if (seq !== searchSeqRef.current) { es.close(); return; } blocks), so keep those and just remove the redundant increment-and-check to avoid confusion while preserving the shared searchSeqRef logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/modules/providers/services/session-conversations-search.service.ts`:
- Around line 176-190: The matchesQuery function incorrectly returns true for
every text when words.length === 1; change the logic so a single-word query
actually checks the text rather than short-circuiting to true: in matchesQuery
(referencing requireExactPhrase, phraseRegex, words, and allWordsMatch) keep the
phraseRegex.test(text) check, but for the single-word case call and return
allWordsMatch(text.toLowerCase()) instead of returning true immediately (e.g.,
if (phraseRegex.test(text)) return true; if (words.length === 1) return
allWordsMatch(...);), ensuring the final fallback to allWordsMatch remains
reachable.
---
Nitpick comments:
In `@src/components/sidebar/hooks/useSidebarController.ts`:
- Around line 280-284: Remove the unreachable post-increment guard by deleting
the three lines that set const seq = ++searchSeqRef.current; and immediately
check if (seq !== searchSeqRef.current) { return; } inside useSidebarController;
the correct staleness checks already occur in the SSE handlers (e.g., the
existing if (seq !== searchSeqRef.current) { es.close(); return; } blocks), so
keep those and just remove the redundant increment-and-check to avoid confusion
while preserving the shared searchSeqRef logic.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d0dd0d6-8fcd-43f6-b311-cb7304f6f6b5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonserver/modules/providers/services/session-conversations-search.service.tssrc/components/sidebar/hooks/useSidebarController.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/modules/providers/services/session-conversations-search.service.ts (1)
199-204:⚠️ Potential issue | 🔴 CriticalSingle‑word queries still bypass actual text matching.
The condition
phraseRegex.test(text) || words.length === 1short‑circuits totruefor every text whenever the query is a single word, somatchesQueryreturnstrueregardless of content andallWordsMatchbecomes unreachable. Net effect: every single‑word search returns the firstMAX_MATCHES_PER_SESSIONmessages of every ripgrep‑filtered session, withhighlightsproduced bybuildSnippetfor unrelated text.This was raised on a prior commit and is still present.
🐛 Proposed fix
- if (phraseRegex.test(text) || words.length === 1) { - return true; - } - - return allWordsMatch(text.toLowerCase()); + if (phraseRegex.test(text)) { + return true; + } + + if (words.length === 1) { + return false; + } + + return allWordsMatch(text.toLowerCase());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/services/session-conversations-search.service.ts` around lines 199 - 204, The matchesQuery predicate incorrectly short‑circuits when words.length === 1, causing any single‑word query to always return true; update matchesQuery so it only returns true immediately when phraseRegex.test(text) is true (i.e., a quoted phrase match), and for single‑word queries call the existing allWordsMatch(text.toLowerCase()) logic instead of bypassing it; ensure phraseRegex, words, and allWordsMatch are used as intended so single‑word queries actually filter content (prevent unrelated highlights produced by buildSnippet and avoid returning irrelevant messages counted against MAX_MATCHES_PER_SESSION).
🧹 Nitpick comments (1)
server/modules/providers/services/session-conversations-search.service.ts (1)
891-897: Inconsistent fallback summary across providers.
parseCodexSessionMatchesuseslatestUserMessageText(last user message seen) for the summary, whileparseGeminiSessionMatchesusesfirstUserText(first user message). Claude’s fallback similarly ends up tracking the last user message viastate.fallbackUserText. Picking the first user prompt is generally a better summary heuristic for a "what was this session about" label; either way, aligning the three providers on the same convention will give the sidebar a more predictable look.Also applies to: 966-972
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/modules/providers/services/session-conversations-search.service.ts` around lines 891 - 897, The summary fallback is inconsistent: parseCodexSessionMatches currently uses latestUserMessageText but parseGeminiSessionMatches and Claude use first-user heuristics; change parseCodexSessionMatches to use the same first-user heuristic (replace latestUserMessageText with firstUserText) and update the other referenced fallback (around the 966-972 area) so all three providers use the same symbol/logic; ensure you also standardize state.fallbackUserText usage in Claude-related code to mirror firstUserText so the toSummaryText calls across parseCodexSessionMatches, parseGeminiSessionMatches, and the Claude fallback all use the first user message as the summary source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/modules/providers/services/session-conversations-search.service.ts`:
- Around line 533-561: The current pre-filter uses normalizedQuery (single ASCII
spaces) with runRipgrepFilesWithMatches when requireExactPhrase is true, which
can drop files that the in-memory phraseRegex/matchesQuery would accept (because
phraseRegex uses \s+). Change the logic in the requireExactPhrase branch to fall
back to a per-word intersection: run ripgrep per individual word (same as the
non-phrase branch) and intersect the resulting matched path sets instead of
searching for the normalizedQuery as a whole; keep the strict
phraseRegex/matchesQuery check later to filter candidates. Update references
around normalizedQuery, requireExactPhrase, runRipgrepFilesWithMatches, and
matchedForPhrase to implement the per-word chunked workers and set intersection
approach so the ripgrep pre-filter remains an over-approximation.
---
Duplicate comments:
In `@server/modules/providers/services/session-conversations-search.service.ts`:
- Around line 199-204: The matchesQuery predicate incorrectly short‑circuits
when words.length === 1, causing any single‑word query to always return true;
update matchesQuery so it only returns true immediately when
phraseRegex.test(text) is true (i.e., a quoted phrase match), and for
single‑word queries call the existing allWordsMatch(text.toLowerCase()) logic
instead of bypassing it; ensure phraseRegex, words, and allWordsMatch are used
as intended so single‑word queries actually filter content (prevent unrelated
highlights produced by buildSnippet and avoid returning irrelevant messages
counted against MAX_MATCHES_PER_SESSION).
---
Nitpick comments:
In `@server/modules/providers/services/session-conversations-search.service.ts`:
- Around line 891-897: The summary fallback is inconsistent:
parseCodexSessionMatches currently uses latestUserMessageText but
parseGeminiSessionMatches and Claude use first-user heuristics; change
parseCodexSessionMatches to use the same first-user heuristic (replace
latestUserMessageText with firstUserText) and update the other referenced
fallback (around the 966-972 area) so all three providers use the same
symbol/logic; ensure you also standardize state.fallbackUserText usage in
Claude-related code to mirror firstUserText so the toSummaryText calls across
parseCodexSessionMatches, parseGeminiSessionMatches, and the Claude fallback all
use the first user message as the summary source.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe120a7b-a975-48f8-bf2e-e9d707cb8f3e
📒 Files selected for processing (1)
server/modules/providers/services/session-conversations-search.service.ts
… and harden conflict detection
Why
- Legacy installs can have a sessions table shape that predates provider/custom_name columns. Running migrateLegacySessionNames first caused its INSERT OR REPLACE INTO sessions (...) to target columns that may not exist and fail during startup migration.
- Some upgraded databases had projects.project_id as plain TEXT instead of a real PRIMARY KEY. That breaks assumptions used by id-based lookups and can allow invalid/duplicate identity semantics over time.
- projectsDb.createProjectPath inferred outcomes from
ow.isArchived, but the upsert path always returns the post-update row with isArchived=0, so archived-reactivation and fresh-create could be misclassified.
- git clone accepted user-controlled URLs directly in argv position, so inputs beginning with - could be interpreted as options instead of a repository argument.
What
- Added
ebuildProjectsTableWithPrimaryKeySchema in migrations: detect table shape via getTableInfo('projects'), verify project_id has pk=1, and rebuild when missing.
- Rebuild flow now creates a canonical projects__new table (project_id TEXT PRIMARY KEY), copies rows with transformation, backfills empty ids via SQLITE_UUID_SQL, deduplicates conflicting ids/paths, then swaps tables inside a transaction.
- Replaced the prior �ddColumnToTableIfNotExists(...) + UPDATE project_id sequence with PK-aware detection/rebuild logic so legacy DBs converge to the required schema.
- Reordered migration sequence to run
ebuildSessionsTableWithProjectSchema before migrateLegacySessionNames, ensuring sessions is normalized before legacy session_names merge writes execute.
- Updated projectsDb.createProjectPath to generate an �ttemptedId before insert, pass it into the prepared statement, and classify outcomes by comparing returned
ow.project_id to �ttemptedId (created vs
eactivated_archived), with no-row remaining �ctive_conflict.
- Hardened clone execution by inserting -- before clone URL in git argv and rejecting normalized GitHub URLs that start with - in startCloneProject.
Tests
- Added integration coverage for projectsDb.createProjectPath branches: fresh insert, archived reactivation, and active conflict.
- Added clone service test for option-prefixed githubUrl rejection (INVALID_GITHUB_URL).
…n synchronization results
Codex history normalization was downgrading reasoning into plain assistant text because of branch ordering, not because the raw data was missing. Why this mattered: - Codex reasoning JSONL entries are intentionally mapped to history items with type thinking, but they also carry message.role assistant. - normalizeHistoryEntry evaluated the assistant-role branch before the thinking branch. - As a result, reasoning content matched the assistant-text path first and was emitted as kind text instead of kind thinking. - This collapses semantic intent, so UI and downstream features that rely on thinking blocks (separate rendering, filtering, and interpretation of model thought process vs final answer) receive the wrong message kind. What changed: - Prioritized thinking detection (raw.type === thinking or raw.isReasoning) before role-based assistant normalization. - Kept a non-empty content guard for thinking payloads to avoid emitting empty artifacts. Impact: - Reasoning entries from persisted Codex JSONL now remain thinking blocks end-to-end. - Regular assistant text normalization behavior remains unchanged.
3 participants
Context / Why this was needed
This branch addresses a structural issue in
main: session and project behavior was split across legacy route files, provider logic, and filesystem scanning paths, which made behavior inconsistent and hard to evolve safely. The biggest pain points were:projects.js, route files, provider code), so provider-specific parsing was coupled to unrelated modules.getName,applyCustomSessionNames, etc.) increased ambiguity and made data ownership unclear.server/index.jsand old routes carried too much business logic, slowing down changes and increasing regression risk.What changed (and why)
Database access was centralized into typed repositories under
server/modules/database/*, and legacy DB filesserver/database/db.jsandserver/database/schema.jswere removed. This was needed to establish one typed persistence layer instead of ad-hoc SQL usage spread across the app.Session metadata became the canonical lookup source via
server/modules/database/repositories/sessions.db.ts. A newdeleteSessionByIdwas added there, rename now usesupdateSessionCustomName, and legacy name helpers were removed. This was needed so all session operations are keyed bysessionIdagainst one authoritative table.Provider-specific history readers were moved into provider session modules:
server/modules/providers/list/claude/claude-sessions.provider.tsserver/modules/providers/list/codex/codex-sessions.provider.tsserver/modules/providers/list/gemini/gemini-sessions.provider.tsThis was needed to keep parsing logic with the provider that owns that format and stop leaking provider internals into generic modules.
For file path resolution, providers now use
sessionsDb.getSessionById(sessionId)andjsonl_pathdirectly rather than directory search heuristics. This was needed to remove fragile filesystem discovery and ensure deterministic message loading.Session message fetching was moved to provider routes:
GET /api/providers/sessions/:sessionId/messagesinserver/modules/providers/provider.routes.tsserver/routes/messages.jswas removedThis was needed to make provider session APIs live in one module and enforce “route parses input, service executes logic.”
sessionId:DELETE /api/providers/sessions/:sessionIdPUT /api/providers/sessions/:sessionIdBoth delegate to
server/modules/providers/services/sessions.service.ts. This was needed to remove provider-specific routing requirements from the client and unify session mutation behavior.sessions.service.tswas simplified sofetchHistoryno longer requires a provider argument, andFetchHistoryOptionsno longer includesprojectName(seeserver/shared/types.ts). Provider/project context is now resolved server-side from DB metadata. This was needed to eliminate client-side coupling to backend storage details.Session deletion behavior was normalized by removing
deleteSessionArtifactsand adding optionaldeletedFromDiskbehavior todeleteSessionById, usingremoveFileIfExistswhen requested. This was needed to keep DB deletion always available while making disk cleanup explicit and safe.Notification naming lookup in
server/services/notification-orchestrator.jswas updated to usegetSessionNamefromsessions.db.ts. This was needed after removing legacy naming helpers and to keep notification title resolution aligned with the new session repository contract.Project, provider, and websocket logic was modularized into:
server/modules/projects/*server/modules/providers/*server/modules/websocket/*This was needed to reduce
server/index.jscomplexity, isolate responsibilities, and make incremental changes lower-risk.src/utils/api.js,src/stores/useSessionStore.ts, and project/session state consumers. This was needed so UI state no longer sends provider/project identifiers for history fetches and uses the unified backend contract.Net effect
This PR shifts the system from filesystem-heuristic, route-fragmented session management to a DB-indexed, provider-owned, sessionId-driven architecture. The change reduces ambiguity in data ownership, removes duplicated flows, and creates a safer base for future session features (pagination, deletion semantics, watcher updates, and provider expansion) without reintroducing cross-module coupling.
Summary by CodeRabbit
New Features
Improvements
Refactor