Repository navigation
feat(chats): add repository-less chats to the Manager UI and API - #483
chriswritescode-dev wants to merge 6 commits into
Conversation
![lil-worker-cstech[bot]](https://avatars.githubusercontent.com/u/99081550?s=60&v=4){kind=small}
| private toChat(id: string): Chat { | ||
| const directory = path.join(this.root(), id) | ||
| return { | ||
| id, | ||
| directory, | ||
| filesPath: path.relative(getReposPath(), directory), | ||
| createdAt: parseChatIdCreatedAt(id), | ||
| } | ||
| } |
There was a problem hiding this comment.
Files path escapes root
filesPath is path.relative(getReposPath(), directory), which gives ../chats/<id>. The frontend passes this as the file browser basePath and as repoLocalPath. File APIs that resolve paths against the repos root and block traversal will reject it, so Files and file links would break in chats. I can't see the file routes from this diff, so this needs a check against them. No test uses the real path: the frontend tests mock filesPath as /chats/<id>.
| await this.openCodeClient.api.session.remove({ sessionID: sessionId }) | ||
| try { | ||
| const remaining = await this.openCodeClient.api.session.list({ | ||
| directory: chat.directory, | ||
| limit: 1, | ||
| }) | ||
|
|
||
| if (remaining.data.length > 0) { | ||
| return { deleted: true, folderRemoved: false } | ||
| } | ||
|
|
||
| try { | ||
| await this.terminals.removeAll(chat.directory) | ||
| } catch (error) { | ||
| logger.warn(`Failed to remove terminals for chat ${chatId}:`, error) | ||
| } | ||
|
|
||
| await fs.rm(chat.directory, { recursive: true, force: true }) | ||
| return { deleted: true, folderRemoved: true } |
There was a problem hiding this comment.
Delete/create race
After removing the session, the code checks session.list({ limit: 1 }) and then deletes the whole chat folder recursively. Nothing stops another session being created in the same folder in between, for example through the chat page's "New session" or fork. That session would be left pointing at a deleted directory, and any files it wrote would be lost.
| {!remote && ( | ||
| <TabsContent | ||
| value="chats" | ||
| className="mt-0 flex min-h-0 w-full min-w-0 flex-1 flex-col px-0" | ||
| > | ||
| <ChatPickerContent | ||
| open={open} | ||
| activeSessionID={activeSessionID} | ||
| onNavigate={onNavigate} | ||
| onDeleteActiveSession={onDeleteActiveSession} | ||
| searchInputRef={chatsSearchInputRef} | ||
| searchTrailing={chatsTrailing} | ||
| /> | ||
| </TabsContent> | ||
| )} |
There was a problem hiding this comment.
Inconsistent remote check
The Chats tab trigger and the tab-cycling logic use isRemote, but the Chats TabsContent is gated on remote. If the two ever differ, the remote picker could show a Chats tab, or mount chat content for a linked server even though its trigger is hidden. Using one flag for both would avoid that.
| const confirmDelete = useCallback(() => { | ||
| if (!chatToDelete) return | ||
| if (chatToDelete.session.id === activeSessionID) onDeleteActiveSession() | ||
| void deleteChatSession | ||
| .mutateAsync({ | ||
| chatId: chatToDelete.chat.id, | ||
| sessionId: chatToDelete.session.id, | ||
| directory: chatToDelete.session.location.directory, | ||
| }) | ||
| .catch(() => undefined) | ||
| .finally(() => setChatToDelete(null)) | ||
| }, [chatToDelete, activeSessionID, onDeleteActiveSession, deleteChatSession]) |
There was a problem hiding this comment.
Navigates before delete
onDeleteActiveSession() runs, and navigates away from the open chat, before the delete request finishes. If the request fails, for example with a 400 for a mismatched session or a network error, the session still exists but the user has already been sent to /.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| {!remote && ( | ||
| <TabsContent | ||
| value="chats" |
There was a problem hiding this comment.
Suggestion: The Chats tab trigger, the tab cycle and the forced reset to sessions all use isRemote, but the Chats tab content uses the remote prop. If these values differ, the Chats tab can be shown with no content, or the chat list can be mounted for a linked server and query local chat data. Use isRemote here too. [possible bug, importance: 5]
| {!remote && ( | |
| <TabsContent | |
| value="chats" | |
| {!isRemote && ( | |
| <TabsContent | |
| value="chats" |
| const { id, directory } = await this.createFolder(root) | ||
| try { | ||
| const model = input.model ? parseOpenCodeModelRef(input.model) : undefined | ||
| const session = await this.openCodeClient.api.session.create({ |
There was a problem hiding this comment.
Suggestion: An invalid model string is parsed only after the chat folder exists. The parse error then creates a folder, deletes it again, and is probably returned as a server error instead of a client error. Parse the model before creating the folder so bad input fails early and leaves nothing behind. [general, importance: 4]
| const { id, directory } = await this.createFolder(root) | |
| try { | |
| const model = input.model ? parseOpenCodeModelRef(input.model) : undefined | |
| const session = await this.openCodeClient.api.session.create({ | |
| const model = input.model ? parseOpenCodeModelRef(input.model) : undefined | |
| const { id, directory } = await this.createFolder(root) | |
| try { | |
| const session = await this.openCodeClient.api.session.create({ |
User description
Summary
/chats/:chatId/sessions/:sessionIdwith transcript, composer, details, files, terminal and preview, and no repository tools.Change
shared/src/schemas/chats.ts: chat and chat-folder schemas plus the workspace path helper.backend/src/services/chats.ts,backend/src/routes/chats.ts:ChatServiceownschats/<chatId>folders;/api/chatslists paged, gets, creates and deletes chats, removing the folder and its terminals with the last session.backend/src/routes/chat-terminals.ts: chat terminals use the existing terminal stack in the chat folder.backend/src/services/notification.ts: chat sessions resolve to their chat so notification links open the right page.useChatsand the chat API client;ChatsNavGroupin the desktop sidebar;ChatPickerContentand the fourth session-picker tab;SessionDetailchat scope with terminal-target support; activity, builtin commands and More-sheet items hide repository-only actions for chats.Validation
pnpm typecheck(cli, frontend, backend) and sharedtsc --noEmit: cleanpnpm lint: 0 errors (41 pre-existing warnings)pnpm test: CLI 456/456; frontend 324 files / 4160 tests; backend 4475 pass, 3 sandbox failures from the host CPU limit in untouched files; coverage run excluding those files 222 files / 4430 tests, 80% thresholds hold;pnpm buildexit 0Notes
/api/chatsseparately.Checklist
pnpm lintpasses locallypnpm typecheckpasses locallyPR Type
Enhancement, Tests
Description
Add repository-less chats as dated workspace folders
Expose
/api/chatslist, get, create, delete-sessionChat session page with transcript, terminals, files, preview
Sidebar Chats group and fourth session-picker tab
Route notifications, prompts and shortcuts into chats
Cover chats with backend, hook and UI tests
Diagram Walkthrough
File Walkthrough
15 files
Define chat schemas, id pattern and response typesBuild chat session paths and read chat ids from directoriesChatService owns chat folders and session lifecycleAdd paged chat list, get, create and session delete routesChat terminal REST and websocket routes in chat folderResolve chat folders to chat notification linksChat folder pagination, session rows and mutationsAdd chat API client for list, get, create, deleteChats sidebar group with sessions and pagingChat tab content for shared session pickerAdd Chats tab and remote-aware tab cycleChat session scope with terminal target and delete actionResolve local session path for repo or chatParse and build chat session routesSurface chat sessions as active work rows3 files
Add chats workspace directory path helperWire chat service, routes and chats directory bootstrapInclude chats root in sandbox project roots1 files
Extract reusable session picker list and row components1 files
Cover chat folder creation, listing and deletion39 files