feat: persist AI assistant chat history so conversations can be resumed (JEF-65) - #182
Merged
Merged
Conversation
…ed (JEF-65) The assistant's conversation was entirely ephemeral: assistant.tsx kept it in a plain useState, and sendChatMessage took the whole prior history as a client-supplied argument on every call — a refresh or navigating away lost everything. This adds server-side persistence, scoped per user like every other entity (cascade-deletes with the user, same as LoginEvent/Session). Backend (Clean Architecture layers, following the LoginEvent feature's shape as the closest existing precedent — a simple user-scoped table with no application-level parent): - New Message domain entity + Message table (userId, role, content, createdAt), cascade-deletes on user. - IMessageRepository / DrizzleMessageRepository: create, findAllByUserId, deleteAllByUserId. - GetChatHistoryUseCase and ClearChatHistoryUseCase — new chatHistory query and clearChatHistory mutation. - ChatWithAssistantUseCase: loads history from the repository instead of a client-supplied `history` array, and persists both the user's message and the assistant's reply after a successful response (not on rate-limit/AI_NOT_CONFIGURED failures, so there's no dangling question with no answer). This also let the role-validation check be deleted — history is DB-sourced now, never client-controlled, so the injection concern it guarded against no longer applies. sendChatMessage's `history` argument and the now-unused ChatMessageInput GraphQL type are removed. Frontend: - assistant.tsx gets a route loader (same ensureQueryData pattern as JEF-64) that prefetches chatHistory. The component seeds its local `messages` state synchronously from the already-populated query cache in a useState initializer, rather than useQuery + an effect — avoids a render where messages is briefly empty before the history loads in, and keeps the existing optimistic-append send flow unchanged. - New "Clear" button (confirm() guard, matching the existing window.confirm pattern used for deleting an application) calls clearChatHistory, resets local state, and writes the emptied result back into the query cache so a subsequent navigation away and back doesn't show stale cached history. Verified: typecheck/lint/build clean for both apps. 879/879 API tests passing (new repository/mapper/use-case tests, plus ChatWithAssistantUseCase's tests updated for the new load-from-repo / persist-after-success behavior — including that a rate-limited or unconfigured-AI turn persists nothing). 154/154 web tests passing (new AssistantPage test covering seeded history, sending, and clear+confirm — required adding an Element.scrollIntoView polyfill to the shared jsdom test setup, a pre-existing gap since this page never had a test before). Manually verified end-to-end against a live dev server + real SQLite DB: confirmed a failed send (no LLM key configured) persists nothing, then seeded messages directly in the DB and confirmed via Playwright that the assistant page renders the persisted conversation on load, and that clearing empties both the UI and the database (a page reload after clearing still shows the empty state). Ref: JEF-65
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
WalkthroughThe change adds persisted conversations and messages. The API adds storage, use cases, repositories, GraphQL operations, and ownership checks. The web assistant loads cached history, sends conversation identifiers, and supports confirmed conversation clearing. ChangesPersisted chat
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AssistantPage
participant GraphQL
participant GetChatHistoryUseCase
participant DrizzleMessageRepository
AssistantPage->>GraphQL: request chatHistory(conversationId)
GraphQL->>GetChatHistoryUseCase: execute(userId, conversationId)
GetChatHistoryUseCase->>DrizzleMessageRepository: findAllByConversationId(conversationId)
DrizzleMessageRepository-->>GetChatHistoryUseCase: ordered messages
GetChatHistoryUseCase-->>GraphQL: message history
GraphQL-->>AssistantPage: cached chat history
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployments for this PR: |
Extends the single-thread chat history from the previous commit to support multiple named conversations per user, ChatGPT-sidebar style. - New Conversation entity/table (userId, title, timestamps), cascades to the user like everything else. Message now belongs to a conversationId instead of directly to a userId — a conversation is the actual ownership boundary, messages are just its contents. - IConversationRepository / DrizzleConversationRepository: create, findById, findAllByUserId (newest-updated first, for the sidebar), updateTitle, delete (cascades to its messages at the DB level). - CreateConversationUseCase, ListConversationsUseCase, DeleteConversationUseCase (replaces the old ClearChatHistoryUseCase — clearing is now "delete this conversation" instead of wiping the single thread). GetChatHistoryUseCase and ChatWithAssistantUseCase both now take a conversationId and verify it belongs to the requesting user (NOT_FOUND / FORBIDDEN), matching the existing GetApplicationUseCase ownership-check pattern. - ChatWithAssistantUseCase auto-derives a conversation's title from its first message (truncated to 50 chars) once history is empty — no extra LLM call, just a substring of what the user actually typed. - GraphQL: new `conversations` query and `createConversation` / `deleteConversation` mutations; `chatHistory` and `sendChatMessage` now take a `conversationId` argument. Removed the now-unused `clearChatHistory` mutation. Migration regenerated from scratch (0003_confused_satana.sql) rather than layered as an alter on top of the previous commit's migration — that one was never applied to any real database (only ephemeral CI preview DBs), so there's no shipped state to preserve. Verified: typecheck/lint/build clean, 900/900 API tests passing (new repository/mapper/use-case tests for Conversation, plus ChatWithAssistantUseCase/GetChatHistoryUseCase tests updated for conversation-scoped ownership checks and title derivation). Ref: JEF-65
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/routes/_authenticated/assistant.tsx (1)
109-116: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSynchronise successful sends into the chat-history cache.
handleSendonly updates localmessages; the loader-seeded['chatHistory']cache remains unchanged. A later route entry can therefore reload the old persisted value and omit messages that were already saved. Append both user and assistant messages tochatHistoryaftersend.mutateAsyncsucceeds, and add a regression test that remounts the page after a successful send.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/_authenticated/assistant.tsx` around lines 109 - 116, The handleSend flow must also update the ['chatHistory'] query cache after send.mutateAsync succeeds, appending both the user message and returned assistant message while preserving existing history. Add a regression test that performs a successful send, remounts the page, and verifies the sent messages remain visible.
🧹 Nitpick comments (4)
apps/api/src/infrastructure/db/schema.ts (1)
111-125: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider adding a database-level
CHECKconstraint forrole.Drizzle's
text('role', { enum: [...] })config only infers TypeScript types. It does not add a runtime or database-level constraint. Add acheck()constraint onroleso the database rejects invalid values, independent of the application layer.♻️ Proposed fix to add a `CHECK` constraint
+import { check } from 'drizzle-orm/sqlite-core'; +import { sql } from 'drizzle-orm'; + export const message = sqliteTable( 'Message', { id: text('id').primaryKey(), conversationId: text('conversationId') .notNull() .references(() => conversation.id, { onDelete: 'cascade' }), role: text('role', { enum: ['user', 'assistant'] }).notNull(), content: text('content').notNull(), createdAt: integer('createdAt', { mode: 'timestamp_ms' }) .notNull() .$defaultFn(() => new Date()), }, - (table) => [index('Message_conversationId_idx').on(table.conversationId)], + (table) => [ + index('Message_conversationId_idx').on(table.conversationId), + check('Message_role_check', sql`${table.role} IN ('user', 'assistant')`), + ], );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/infrastructure/db/schema.ts` around lines 111 - 125, Update the message table definition to add a database-level check constraint on the role column, allowing only the existing 'user' and 'assistant' values. Use Drizzle’s check constraint in the table configuration alongside the Message_conversationId_idx definition, while preserving the current TypeScript enum configuration.apps/api/src/infrastructure/db/repositories/DrizzleMessageRepository.ts (1)
27-34: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider a limit or pagination for long conversation histories.
findAllByConversationIdretrieves every message for a conversation with no limit. For long-running conversations, this grows unbounded and may need pagination, especially if this data feeds an LLM context window with token limits.Add a
limit/cursor parameter now, or track this as a follow-up once conversation length becomes a real concern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/infrastructure/db/repositories/DrizzleMessageRepository.ts` around lines 27 - 34, Update findAllByConversationId to accept a limit and pagination cursor, apply them to the ordered Drizzle query, and preserve chronological results while bounding returned conversation history. Update its interface and callers to pass the new parameters or use sensible defaults.apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts (1)
56-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the ownership check.
ChatWithAssistantUseCase,GetChatHistoryUseCaseandDeleteConversationUseCaserepeat the same find-then-verify block. A shared helper, for exampleassertConversationOwnedBy(conversationId, userId), would keep the error codes consistent as more conversation use cases arrive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts` around lines 56 - 62, Extract the repeated conversation lookup and ownership validation from ChatWithAssistantUseCase into a shared assertConversationOwnedBy helper, then reuse it in GetChatHistoryUseCase and DeleteConversationUseCase. Preserve the existing NOT_FOUND and FORBIDDEN error codes and return the validated conversation for callers that need it.apps/api/src/use-cases/conversations/ICreateConversationUseCase.ts (1)
3-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an input object for consistency.
IDeleteConversationUseCaseandIGetChatHistoryUseCaseboth accept a named input object. This contract andIListConversationsUseCaseaccept a bareuserIdstring. A named object, for example{ userId }, keeps the use-case surface uniform and prevents an accidental swap withconversationIdat a call site.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/use-cases/conversations/ICreateConversationUseCase.ts` around lines 3 - 5, Update ICreateConversationUseCase.execute to accept a named input object containing userId instead of a bare string, and propagate this contract change through its implementations and callers while preserving the existing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts`:
- Around line 158-201: Update ChatWithAssistantUseCase.execute so the user
message is persisted via messageRepository.create before resolving the LLM
provider or invoking complete/completeWithTools. Preserve rate-limit behavior by
keeping the limiter check before persistence, and add test coverage verifying
the user message remains stored when provider creation or completion fails.
In `@apps/api/src/http/schema/queries/chatQueries.ts`:
- Around line 29-34: Update the chatHistory resolver around
getChatHistoryUseCase.execute to catch ownership and not-found failures, pass
them through the established fromCodedError mapping, and preserve the existing
messageMapper.toDTO mapping for successful results.
In `@apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts`:
- Around line 90-95: The conversation timestamp is only updated during title
initialization, so later messages leave sidebar ordering stale. Add a touch(id)
method to IConversationRepository and implement it in
DrizzleConversationRepository to update the conversation’s updatedAt to the
current time, then call it from ChatWithAssistantUseCase.execute after the
message writes on every turn.
- Around line 75-88: Update ChatWithAssistantUseCase to persist the user message
before calling complete, while preserving the pre-write history.length === 0
value for first-turn title derivation. Use the provided transactionManager and
transaction-scoped client from getClient to atomically create the assistant
reply and update the conversation title; ensure failures in the provider call
still leave the user message persisted.
- Around line 67-73: Limit the history used to build messages in
ChatWithAssistantUseCase by selecting only the most recent N turns or an
appropriate token-bounded window from the ordered result, while preserving
chronological order for replay. Keep title-update logic based on the full
history length rather than the truncated window.
In `@apps/api/src/use-cases/conversations/CreateConversationUseCase.ts`:
- Around line 13-15: Update CreateConversationUseCase.execute to prevent
unbounded conversation creation: enforce a dedicated conversation rate limiter
before calling conversationRepository.create, or first reuse an existing empty
conversation for the user. Preserve the current generated-id creation path only
when no reusable conversation exists, and ensure the authenticated userId is
used for the limit or lookup.
In `@apps/web/src/__tests__/components/AssistantPage.test.tsx`:
- Around line 68-84: Extend the test around AssistantPage so that after the
successful send it verifies the ['chatHistory'] cache contains both the
submitted user message and the assistant response. Use the existing query client
or remount AssistantPage to confirm the persisted history, while preserving the
current request and rendered-message assertions.
In `@apps/web/src/routes/_authenticated/assistant.tsx`:
- Around line 81-91: Update the send and clear mutation controls around send,
clear, and onClear so sending is disabled while clear.isPending, clearing is
disabled while send.isPending, and onClear returns without acting when
send.isPending. Preserve the existing mutation behavior when neither request is
pending.
- Around line 62-65: Disable SSR for the protected Route configuration by
setting ssr to false, and move session initialization into beforeLoad using
hydrateSession() so it runs client-side before AssistantPage data loading.
Preserve the existing chatHistoryQueryOptions loader after the session has been
hydrated.
---
Outside diff comments:
In `@apps/web/src/routes/_authenticated/assistant.tsx`:
- Around line 109-116: The handleSend flow must also update the ['chatHistory']
query cache after send.mutateAsync succeeds, appending both the user message and
returned assistant message while preserving existing history. Add a regression
test that performs a successful send, remounts the page, and verifies the sent
messages remain visible.
---
Nitpick comments:
In `@apps/api/src/infrastructure/db/repositories/DrizzleMessageRepository.ts`:
- Around line 27-34: Update findAllByConversationId to accept a limit and
pagination cursor, apply them to the ordered Drizzle query, and preserve
chronological results while bounding returned conversation history. Update its
interface and callers to pass the new parameters or use sensible defaults.
In `@apps/api/src/infrastructure/db/schema.ts`:
- Around line 111-125: Update the message table definition to add a
database-level check constraint on the role column, allowing only the existing
'user' and 'assistant' values. Use Drizzle’s check constraint in the table
configuration alongside the Message_conversationId_idx definition, while
preserving the current TypeScript enum configuration.
In `@apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts`:
- Around line 56-62: Extract the repeated conversation lookup and ownership
validation from ChatWithAssistantUseCase into a shared assertConversationOwnedBy
helper, then reuse it in GetChatHistoryUseCase and DeleteConversationUseCase.
Preserve the existing NOT_FOUND and FORBIDDEN error codes and return the
validated conversation for callers that need it.
In `@apps/api/src/use-cases/conversations/ICreateConversationUseCase.ts`:
- Around line 3-5: Update ICreateConversationUseCase.execute to accept a named
input object containing userId instead of a bare string, and propagate this
contract change through its implementations and callers while preserving the
existing behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bc52607-92de-460f-830c-b35ba1d18bda
📒 Files selected for processing (44)
apps/api/drizzle/0003_confused_satana.sqlapps/api/drizzle/meta/0003_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.tsapps/api/src/__tests__/application/chat/GetChatHistoryUseCase.test.tsapps/api/src/__tests__/application/conversations/CreateConversationUseCase.test.tsapps/api/src/__tests__/application/conversations/DeleteConversationUseCase.test.tsapps/api/src/__tests__/application/conversations/ListConversationsUseCase.test.tsapps/api/src/__tests__/helpers/createTestDb.tsapps/api/src/__tests__/helpers/mocks.tsapps/api/src/__tests__/infrastructure/db/repositories/DrizzleConversationRepository.test.tsapps/api/src/__tests__/infrastructure/db/repositories/DrizzleMessageRepository.test.tsapps/api/src/__tests__/interface-adapters/mappers/ConversationMapper.test.tsapps/api/src/__tests__/interface-adapters/mappers/MessageMapper.test.tsapps/api/src/constants.tsapps/api/src/domain/conversation/Conversation.tsapps/api/src/domain/message/Message.tsapps/api/src/http/container.tsapps/api/src/http/schema/index.tsapps/api/src/http/schema/mutations/chatMutations.tsapps/api/src/http/schema/queries/chatQueries.tsapps/api/src/http/schema/types/ConversationType.tsapps/api/src/http/schema/types/MessageType.tsapps/api/src/http/schema/types/inputs/ChatInputs.tsapps/api/src/infrastructure/db/repositories/DrizzleConversationRepository.tsapps/api/src/infrastructure/db/repositories/DrizzleMessageRepository.tsapps/api/src/infrastructure/db/schema.tsapps/api/src/interface-adapters/mappers/ConversationMapper.tsapps/api/src/interface-adapters/mappers/MessageMapper.tsapps/api/src/use-cases/chat/ChatWithAssistantUseCase.tsapps/api/src/use-cases/chat/GetChatHistoryUseCase.tsapps/api/src/use-cases/chat/IGetChatHistoryUseCase.tsapps/api/src/use-cases/conversations/CreateConversationUseCase.tsapps/api/src/use-cases/conversations/DeleteConversationUseCase.tsapps/api/src/use-cases/conversations/ICreateConversationUseCase.tsapps/api/src/use-cases/conversations/IDeleteConversationUseCase.tsapps/api/src/use-cases/conversations/IListConversationsUseCase.tsapps/api/src/use-cases/conversations/ListConversationsUseCase.tsapps/api/src/use-cases/ports/IConversationRepository.tsapps/api/src/use-cases/ports/IMessageRepository.tsapps/web/src/__tests__/components/AssistantPage.test.tsxapps/web/src/__tests__/setup.tsapps/web/src/routeTree.gen.tsapps/web/src/routes/_authenticated/assistant.tsx
💤 Files with no reviewable changes (1)
- apps/api/src/http/schema/types/inputs/ChatInputs.ts
Comment on lines
+158
to
+201
| it("persists the user's message and the assistant's reply after a successful response", async () => { | ||
| const llmProvider = makeToolCallingProvider({ content: 'Hello there!', toolCalls: [] }); | ||
| const messageRepository = makeMessageRepository(); | ||
| const generateId = vi.fn().mockReturnValueOnce('user-msg-id').mockReturnValueOnce('ai-msg-id'); | ||
| const deps = makeDeps({ | ||
| llmProviderFactory: makeLLMProviderFactory({ | ||
| forUser: vi.fn().mockResolvedValue(llmProvider), | ||
| }), | ||
| messageRepository, | ||
| generateId, | ||
| }); | ||
|
|
||
| await new ChatWithAssistantUseCase(deps as never).execute({ | ||
| ...baseInput, | ||
| message: 'hi', | ||
| }); | ||
|
|
||
| expect(messageRepository.create).toHaveBeenNthCalledWith(1, { | ||
| id: 'user-msg-id', | ||
| conversationId: 'conv-1', | ||
| role: 'user', | ||
| content: 'hi', | ||
| }); | ||
| expect(messageRepository.create).toHaveBeenNthCalledWith(2, { | ||
| id: 'ai-msg-id', | ||
| conversationId: 'conv-1', | ||
| role: 'assistant', | ||
| content: 'Hello there!', | ||
| }); | ||
| }); | ||
|
|
||
| it('does not persist anything when rate-limited', async () => { | ||
| const messageRepository = makeMessageRepository(); | ||
| const deps = makeDeps({ | ||
| chatRateLimiter: makeRateLimiter({ consume: vi.fn().mockReturnValue(false) }), | ||
| messageRepository, | ||
| }); | ||
|
|
||
| await new ChatWithAssistantUseCase(deps as never) | ||
| .execute({ ...baseInput, message: 'hi' }) | ||
| .catch(() => {}); | ||
|
|
||
| expect(messageRepository.create).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'ChatWithAssistantUseCase\.(ts|test\.ts)$|ChatWithAssistantUseCase' . | sed 's#^\./##'
echo "== test file outline/section =="
if [ -f apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts ]; then
wc -l apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts
ast-grep outline apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts 2>/dev/null || true
sed -n '1,260p' apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts
fi
echo "== implementation search =="
rg -n "class ChatWithAssistantUseCase|execute\\(|messageRepository\\.create|complete\\(|completeWithTools|AI_NOT_CONFIGURED|llmProviderFactory" apps/api/src -g '*.ts'Repository: mankatcheung/job-finder
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ChatWithAssistantUseCase implementation =="
sed -n '1,180p' apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts
echo "== helpers around makeToolCallingProvider and makeLLMProviderFactory =="
sed -n '1,70p' apps/api/src/__tests__/helpers/mocks.ts
sed -n '420,475p' apps/api/src/__tests__/helpers/mocks.ts
echo "== relevant test cases =="
sed -n '80,220p' apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts
sed -n '320,360p'
echo "== tests that call repository create in chat use-case tests =="
rg -n "messageRepository|conversationRepository|create|updateTitle|hasBeenNthCalledWith|not\\.toHaveBeenCalled" apps/api/src/__tests__/application/chat/ChatWithassistantUseCase.test.ts apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts 2>/dev/null || trueRepository: mankatcheung/job-finder
Length of output: 18105
Persist the user message before calling the LLM.
execute() calls complete() before messageRepository.create, so a rejected llmProviderFactory.forUser() call or completeWithTools() failure prevents the user’s input from being stored. Add coverage for this failure path, and move user-message persistence before complete() so transient LLM provider failures do not drop user input.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/__tests__/application/chat/ChatWithAssistantUseCase.test.ts`
around lines 158 - 201, Update ChatWithAssistantUseCase.execute so the user
message is persisted via messageRepository.create before resolving the LLM
provider or invoking complete/completeWithTools. Preserve rate-limit behavior by
keeping the limiter check before persistence, and add test coverage verifying
the user message remains stored when provider creation or completion fails.
Comment on lines
+29
to
+34
| const { getChatHistoryUseCase, messageMapper } = ctx.diScope.cradle; | ||
| const messages = await getChatHistoryUseCase.execute({ | ||
| userId: ctx.user.sub, | ||
| conversationId: String(args.conversationId), | ||
| }); | ||
| return messages.map((m) => messageMapper.toDTO(m)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Map use-case errors in chatHistory.
Ownership and not-found failures from getChatHistoryUseCase bypass fromCodedError. The resolver can then return an untyped internal GraphQL error instead of the established coded error response.
Proposed fix
import { GraphQLError } from 'graphql';
+import { fromCodedError } from '`#src/http/errors/AppError.js`';
- const messages = await getChatHistoryUseCase.execute({
- userId: ctx.user.sub,
- conversationId: String(args.conversationId),
- });
- return messages.map((m) => messageMapper.toDTO(m));
+ try {
+ const messages = await getChatHistoryUseCase.execute({
+ userId: ctx.user.sub,
+ conversationId: String(args.conversationId),
+ });
+ return messages.map((m) => messageMapper.toDTO(m));
+ } catch (err) {
+ throw fromCodedError(err);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const { getChatHistoryUseCase, messageMapper } = ctx.diScope.cradle; | |
| const messages = await getChatHistoryUseCase.execute({ | |
| userId: ctx.user.sub, | |
| conversationId: String(args.conversationId), | |
| }); | |
| return messages.map((m) => messageMapper.toDTO(m)); | |
| import { GraphQLError } from 'graphql'; | |
| import { fromCodedError } from '`#src/http/errors/AppError.js`'; | |
| const { getChatHistoryUseCase, messageMapper } = ctx.diScope.cradle; | |
| try { | |
| const messages = await getChatHistoryUseCase.execute({ | |
| userId: ctx.user.sub, | |
| conversationId: String(args.conversationId), | |
| }); | |
| return messages.map((m) => messageMapper.toDTO(m)); | |
| } catch (err) { | |
| throw fromCodedError(err); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/http/schema/queries/chatQueries.ts` around lines 29 - 34, Update
the chatHistory resolver around getChatHistoryUseCase.execute to catch ownership
and not-found failures, pass them through the established fromCodedError
mapping, and preserve the existing messageMapper.toDTO mapping for successful
results.
Comment on lines
+67
to
73
| const history = await this.deps.messageRepository.findAllByConversationId(input.conversationId); | ||
|
|
||
| const messages: LLMMessage[] = [ | ||
| { role: 'system', content: SYSTEM_PROMPT }, | ||
| ...input.history.map((m) => ({ role: m.role, content: m.content })), | ||
| ...history.map((m) => ({ role: m.role, content: m.content })), | ||
| { role: 'user', content: input.message }, | ||
| ]; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the message repository implementation and its ordering.
fd -t f 'DrizzleMessageRepository.ts' --exec cat -n
fd -t f 'IMessageRepository.ts' --exec cat -nRepository: mankatcheung/job-finder
Length of output: 2254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the use case around history construction and any relevant constants/config.
use_case_file="$(fd -t f '^ChatWithAssistantUseCase\.ts$' | head -n 1)"
echo "use_case_file=$use_case_file"
wc -l "$use_case_file"
cat -n "$use_case_file" | sed -n '1,140p'
echo "---- search constants ----"
rg -n "MAX_HISTORY|HISTORY|token|context|usage|input\.message|messageRepository\.findAllByConversationId" apps/api/src -SRepository: mankatcheung/job-finder
Length of output: 50379
Cap the chat history before sending the request.
findAllByConversationId already returns rows ordered by createdAt, but ChatWithAssistantUseCase sends every stored turn on each request. Limit the replayed history, such as the last N turns or a token budget, before sending messages to the provider. Keep the title-update check on the full stored length if you apply a window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts` around lines 67 -
73, Limit the history used to build messages in ChatWithAssistantUseCase by
selecting only the most recent N turns or an appropriate token-bounded window
from the ordered result, while preserving chronological order for replay. Keep
title-update logic based on the full history length rather than the truncated
window.
Comment on lines
+75
to
+88
| const reply = await this.complete(messages, input.userId); | ||
|
|
||
| await this.deps.messageRepository.create({ | ||
| id: this.deps.generateId(), | ||
| conversationId: input.conversationId, | ||
| role: 'user', | ||
| content: input.message, | ||
| }); | ||
| await this.deps.messageRepository.create({ | ||
| id: this.deps.generateId(), | ||
| conversationId: input.conversationId, | ||
| role: 'assistant', | ||
| content: reply, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Persist the user message before the provider call, and make the writes atomic.
Two defects exist in this sequence.
- The user message is written only after
completeresolves. If the provider call fails, for example on a network error, a provider rate limit orAI_NOT_CONFIGURED, the user turn is never stored. The web client keeps it in optimistic state, so the message appears to be sent until the page reloads and it is gone. - The two
createcalls and theupdateTitlecall are separate writes. A failure between them leaves a user message with no assistant reply, or a conversation without its derived title.
Write the user message first, then wrap the reply write and the title update in one transaction. The container already provides transactionManager, and getClient resolves the transaction-scoped client.
🐛 Proposed restructure
- const reply = await this.complete(messages, input.userId);
-
- await this.deps.messageRepository.create({
- id: this.deps.generateId(),
- conversationId: input.conversationId,
- role: 'user',
- content: input.message,
- });
- await this.deps.messageRepository.create({
- id: this.deps.generateId(),
- conversationId: input.conversationId,
- role: 'assistant',
- content: reply,
- });
+ await this.deps.messageRepository.create({
+ id: this.deps.generateId(),
+ conversationId: input.conversationId,
+ role: 'user',
+ content: input.message,
+ });
+
+ const reply = await this.complete(messages, input.userId);
+
+ await this.deps.messageRepository.create({
+ id: this.deps.generateId(),
+ conversationId: input.conversationId,
+ role: 'assistant',
+ content: reply,
+ });If you move the user write earlier, keep the history.length === 0 value that you captured before the write, so the title derivation on Line 90 still triggers on the first turn.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts` around lines 75 -
88, Update ChatWithAssistantUseCase to persist the user message before calling
complete, while preserving the pre-write history.length === 0 value for
first-turn title derivation. Use the provided transactionManager and
transaction-scoped client from getClient to atomically create the assistant
reply and update the conversation title; ensure failures in the provider call
still leave the user message persisted.
Comment on lines
+90
to
+95
| if (history.length === 0) { | ||
| await this.deps.conversationRepository.updateTitle( | ||
| input.conversationId, | ||
| this.deriveTitle(input.message), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
updatedAt becomes stale after the first turn.
Conversation.updatedAt uses $onUpdate on the Conversation row, so it only advances when a column of that row changes. This block calls updateTitle only when the conversation was empty. Writing a Message row does not touch the parent. After the first turn, updatedAt never changes again.
IConversationRepository.findAllByUserId documents "Newest-updated first, for the conversation list/sidebar", and DrizzleConversationRepository.findAllByUserId orders by desc(conversation.updatedAt). The sidebar therefore stops reflecting recent activity, and every conversation keeps its creation order.
Add a repository method that touches the conversation on each message write, for example touch(id) setting updatedAt to the current time, and call it in execute after the message writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/use-cases/chat/ChatWithAssistantUseCase.ts` around lines 90 -
95, The conversation timestamp is only updated during title initialization, so
later messages leave sidebar ordering stale. Add a touch(id) method to
IConversationRepository and implement it in DrizzleConversationRepository to
update the conversation’s updatedAt to the current time, then call it from
ChatWithAssistantUseCase.execute after the message writes on every turn.
Comment on lines
+13
to
+15
| async execute(userId: string): Promise<Conversation> { | ||
| return this.deps.conversationRepository.create({ id: this.deps.generateId(), userId }); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the conversation mutations and any rate limiting applied.
fd -t f 'chatMutations.ts' --exec cat -n
rg -nP 'createConversationUseCase|RateLimiter' apps/api/src/http -C2Repository: mankatcheung/job-finder
Length of output: 8022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== use case file =="
fd -t f 'CreateConversationUseCase.ts' --exec cat -n
echo "== rate limiter implementation =="
fd -t f 'RateLimiter.ts|IRateLimiter.ts' --exec cat -n
echo "== repo schema/rows relevant to conversations and messages =="
fd -t f 'Conversations.*|Messages.*' apps/api/src/infrastructure/db apps/api/src -g '*Conversation*' -g '*Message*' -g '*.ts' | sort | sed -n '1,120p'
rg -n "conversations|messages|createConversation" apps/api/src/infrastructure/db apps/api/src/use-cases apps/api/src/http -C 3 | sed -n '1,240p'Repository: mankatcheung/job-finder
Length of output: 16514
Add a rate limit to createConversation.
The authenticated mutation calls createConversationUseCase.execute(ctx.user.sub) without consuming a rate limiter, while SendChatMessage already enforces chatRateLimiter. Add a conversation-creation limiter, or reuse an existing empty conversation before creating another row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/api/src/use-cases/conversations/CreateConversationUseCase.ts` around
lines 13 - 15, Update CreateConversationUseCase.execute to prevent unbounded
conversation creation: enforce a dedicated conversation rate limiter before
calling conversationRepository.create, or first reuse an existing empty
conversation for the user. Preserve the current generated-id creation path only
when no reusable conversation exists, and ensure the authenticated userId is
used for the limit or lookup.
Comment on lines
+68
to
+84
| it('sends a message without a history argument and appends the reply', async () => { | ||
| mockGqlRequest.mockResolvedValue({ sendChatMessage: 'You have 2 active applications.' }); | ||
| render(<AssistantPage />, { wrapper: Wrapper }); | ||
|
|
||
| fireEvent.change(screen.getByPlaceholderText('Ask a question…'), { | ||
| target: { value: 'how many active applications do I have?' }, | ||
| }); | ||
| fireEvent.click(screen.getByRole('button', { name: 'Send' })); | ||
|
|
||
| await waitFor(() => | ||
| expect(screen.getByText('You have 2 active applications.')).toBeInTheDocument(), | ||
| ); | ||
| expect(screen.getByText('how many active applications do I have?')).toBeInTheDocument(); | ||
| expect(mockGqlRequest).toHaveBeenCalledWith(expect.stringContaining('sendChatMessage'), { | ||
| message: 'how many active applications do I have?', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test chat-history cache persistence after sending.
This test verifies local state only. It does not verify the ['chatHistory'] cache after a successful send.
Assert the cache contents or remount AssistantPage after the mutation. This test must show both the user message and assistant response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/__tests__/components/AssistantPage.test.tsx` around lines 68 -
84, Extend the test around AssistantPage so that after the successful send it
verifies the ['chatHistory'] cache contains both the submitted user message and
the assistant response. Use the existing query client or remount AssistantPage
to confirm the persisted history, while preserving the current request and
rendered-message assertions.
Comment on lines
62
to
65
| export const Route = createFileRoute('/_authenticated/assistant')({ | ||
| loader: ({ context: { queryClient } }) => queryClient.ensureQueryData(chatHistoryQueryOptions), | ||
| component: AssistantPage, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'createFileRoute\(|ssr:\s*false|hydrateSession' apps/web/src/routesRepository: mankatcheung/job-finder
Length of output: 18286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== assistant route =="
cat -n apps/web/src/routes/_authenticated/assistant.tsx
echo
echo "== query client/graphql client relevant exports =="
rg -n -C 4 'export const queryClient|new QueryClient|export .*hydrateSession|function hydrateSession|const hydrateSession|from .*queryClient|from .*graphql/client' apps/web/src/lib apps/web/src/graphql apps/web/src -g '*.ts' -g '*.tsx' | sed -n '1,220p'Repository: mankatcheung/job-finder
Length of output: 26541
Disable SSR for this protected route.
This loader calls gqlClient.request(...chatHistory...) before the session is established, and the API cookie is unavailable to the web server. Set ssr: false and resolve the session client-side through beforeLoad/hydrateSession() to avoid a server render failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_authenticated/assistant.tsx` around lines 62 - 65,
Disable SSR for the protected Route configuration by setting ssr to false, and
move session initialization into beforeLoad using hydrateSession() so it runs
client-side before AssistantPage data loading. Preserve the existing
chatHistoryQueryOptions loader after the session has been hydrated.
Source: Coding guidelines
Frontend half of the backend commit above. assistant.tsx now shows a conversation sidebar instead of a single fixed thread: - Route search param (?conversation=id) is the source of truth for which conversation is active, not local component state — so switching conversations, refreshing, and back/forward navigation all just work via the URL. loaderDeps + loader prefetch both the conversations list and (if a conversation is already selected) its history, same ensureQueryData pattern as JEF-64. - No local `messages` state at all: each conversation's history lives under its own `['chatHistory', conversationId]` query key, so switching conversations is just React Query switching which cached query is active — no manual seeding/resetting needed between conversations like a single useState would require. - "New conversation" clears the active selection (a client-side draft state, no API call) rather than eagerly creating a row — a conversation is actually created lazily on first send, so clicking "New" repeatedly without typing anything doesn't litter the sidebar with empty untitled conversations. - Sending with no active conversation: creates one first, optimistic- updates it into the sidebar list, navigates the URL to it, then sends into it — implicit-creation UX matching the backend's lazy-title-on-first-message behavior. - Delete (replacing the old single "Clear" button) is now per- conversation, exposed as a small trash icon on each sidebar row (aria-label="Delete conversation" — icon-only, needed a real accessible name, not just a test hook). Verified: typecheck/lint/build clean. 155/155 web tests passing (AssistantPage.test.tsx rewritten for the new architecture — mocks route search/navigate instead of the old getQueryData-seeding pattern, since messages are now query-driven rather than locally seeded). Manually verified end-to-end against a live dev server + real SQLite DB: seeded two conversations with distinct histories directly in the DB, then confirmed via Playwright that the sidebar lists both newest-first, switching between them shows the correct isolated history for each (not a mix), and deleting the active one removes it from the sidebar, resets to the empty/compose state, and — confirmed via a page reload — the delete persisted server-side rather than only updating local UI state. Ref: JEF-65
…h main Both this branch and the just-merged JEF-53 (refresh token rotation) independently generated a migration numbered 0003. Renumbering this branch's to 0004 so it applies after JEF-53's Session-table changes once merged with main. The 0003 snapshot is dropped here and will be regenerated fresh against main's actual 0003 baseline after merging, so drizzle-kit computes the correct diff instead of a stale one.
…history # Conflicts: # apps/api/drizzle/meta/_journal.json
drizzle-kit generate (run to produce the missing 0004 snapshot after the merge) correctly detected no snapshot existed yet for 0004 and produced a *new* 0005 migration + snapshot instead — since it only had 0003's snapshot to diff against, it recomputed the Conversation/Message CREATE TABLE statements from scratch rather than seeing them as already covered by 0004. That regenerated SQL was byte-identical to 0004's, so 0005 was a straight duplicate. Kept 0004's migration file (already correct), relabeled the newly generated snapshot as 0004's own (its prevId already correctly chains from main's 0003 snapshot), and dropped the redundant 0005 migration + journal entry. Chain is now 0000→0004 with a snapshot for every step.
…aimed 0004 first)
…history # Conflicts: # apps/api/drizzle/meta/_journal.json
…4 renumbered to 0005)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The assistant's conversation was entirely ephemeral:
assistant.tsxkept it in a plainuseState, andsendChatMessagetook the whole prior history as a client-supplied argument on every call — a refresh or navigating away lost everything. This adds server-side persistence, scoped per user like every other entity (cascade-deletes with the user, same asLoginEvent/Session).Backend (Clean Architecture layers, following
LoginEvent's shape as the closest existing precedent — a simple user-scoped table with no application-level parent):Messagedomain entity +Messagetable (userId,role,content,createdAt), cascade-deletes on user.IMessageRepository/DrizzleMessageRepository:create,findAllByUserId,deleteAllByUserId.GetChatHistoryUseCaseandClearChatHistoryUseCase— newchatHistoryquery andclearChatHistorymutation.ChatWithAssistantUseCase: loads history from the repository instead of a client-suppliedhistoryarray, and persists both the user's message and the assistant's reply after a successful response (not on rate-limit/AI_NOT_CONFIGURED failures, so there's no dangling question with no answer). This also let the role-validation check be deleted — history is DB-sourced now, never client-controlled, so the injection concern it guarded against no longer applies.sendChatMessage'shistoryargument and the now-unusedChatMessageInputGraphQL type are removed.Frontend:
assistant.tsxgets a routeloader(sameensureQueryDatapattern as JEF-64) that prefetcheschatHistory. The component seeds its localmessagesstate synchronously from the already-populated query cache in auseStateinitializer, rather thanuseQuery+ an effect — avoids a render wheremessagesis briefly empty before the history loads in, and keeps the existing optimistic-append send flow unchanged.confirm()guard, matching the existingwindow.confirmpattern used for deleting an application) callsclearChatHistory, resets local state, and writes the emptied result back into the query cache so a subsequent navigation away and back doesn't show stale cached history.Ref: Linear JEF-65.
Test plan
pnpm --filter @job-finder/api typecheck/lint/build— cleanpnpm --filter @job-finder/web typecheck/lint/build— cleanpnpm --filter @job-finder/api test— 879/879 passing (new repository/mapper/use-case tests, plusChatWithAssistantUseCase's tests updated for the new load-from-repo / persist-after-success behavior — including that a rate-limited or unconfigured-AI turn persists nothing)pnpm --filter @job-finder/web test— 154/154 passing (newAssistantPagetest covering seeded history, sending, and clear+confirm — required adding anElement.scrollIntoViewpolyfill to the shared jsdom test setup, a pre-existing gap since this page never had a test before)🤖 Generated with Claude Code
https://claude.ai/code/session_01N2PBmsuzPhrmNnfZf6C3BM
Summary by CodeRabbit
New Features
Bug Fixes
Tests