feat: add AI chatbot feature with configuration, access control, and attachment management#165
Merged
Conversation
…attachment management - Implemented loadAiConfig to resolve AI configurations based on organization and purpose. - Created requireChatbotAccess to enforce authentication and feature flag checks for chatbot endpoints. - Developed chatbotAttachments for managing file uploads and in-memory storage of attachments. - Added chatbotSources to extract structured sources from chatbot tool results. - Introduced featureFlags utility to manage feature flag resolution on the server. - Defined shared types for chatbot functionality, including messages, attachments, and sources. - Established a feature flag registry for controlling the availability of the new AI chatbot experience.
|
🚅 Deployed to the reqcore-pr-165 environment in applirank
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds a chatbot assistant feature, multi-config AI provider support, feature-flag infrastructure (client + server + docs), persistent chatbot data model and APIs, UI components/composables for chat, and migrations/schema changes to support multiple AI configs and chatbot persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Client
participant API as Server/API
participant DB as Database
participant LLM
participant Tools as ToolExecutor
User->>Browser: Open chat, submit message
Browser->>API: POST /api/chatbot/chat (conversation, message, overrides)
API->>DB: Load conversation, aiConfig, agent, attachments
API->>DB: Insert user message, update conversation metadata
API->>LLM: streamText(messages, aiConfig, agent, tools)
loop streaming
LLM->>API: SSE text delta / reasoning / tool request
API->>Browser: SSE chunk
alt tool call
API->>Tools: execute tool with org/scope
Tools->>DB: query jobs/applications/candidates
DB-->>Tools: results
Tools->>API: tool result
API->>LLM: tool result (for continuation)
end
end
LLM->>API: finish event (usage)
API->>DB: persist assistant message, sources, toolCalls
Browser->>User: render final message, sources panel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 40 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical 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 (1)
server/api/ai-config/[id]/test-connection.post.ts (1)
51-62:⚠️ Potential issue | 🟠 MajorAvoid returning raw provider error details to clients.
On Line 61, relaying
${message}can expose upstream/internal details. Return a generic client-safe error and keep full diagnostics in server logs.Suggested change
catch (err: any) { const message = err?.data?.statusMessage ?? err?.message ?? 'Unknown error' if (typeof message === 'string' && message.includes('decrypt')) { throw createError({ statusCode: 422, statusMessage: 'Failed to decrypt API key. If you recently rotated BETTER_AUTH_SECRET, re-enter the API key for this configuration.', }) } + console.error('AI test connection failed', { orgId, configId: id, error: message }) throw createError({ statusCode: 422, - statusMessage: `Connection test failed: ${message}`, + statusMessage: 'Connection test failed. Please verify provider, model, base URL, and API key.', }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/ai-config/`[id]/test-connection.post.ts around lines 51 - 62, The catch block in server/api/ai-config/[id]/test-connection.post.ts exposes upstream/internal error details by interpolating `message` into the client-facing createError response; change it to log the full `message` on the server (use your logger or console.error) and return a generic, client-safe statusMessage (e.g., "Connection test failed") while preserving the existing decrypt-specific 422 message behavior for `message.includes('decrypt')`; locate the catch block that defines `const message = err?.data?.statusMessage ?? err?.message ?? 'Unknown error'` and update the second createError to omit `message` from the returned `statusMessage` and ensure the full `message` is recorded in server logs.
🟠 Major comments (24)
app/layouts/dashboard.vue-30-30 (1)
30-30:⚠️ Potential issue | 🟠 MajorRemove the scroll container in full-bleed mode.
The base class still applies
overflow-y-auto, so theoverflow-hiddenbranch can keep a vertical scrollbar and won't fully opt out of the standard layout.🔧 Suggested fix
- <main :class="['relative flex-1 min-h-0 overflow-y-auto', isFullbleed ? 'overflow-hidden' : 'px-4 py-6 sm:px-6 lg:px-8 lg:py-8']"> + <main + :class="[ + 'relative flex-1 min-h-0', + isFullbleed + ? 'overflow-hidden' + : 'overflow-y-auto px-4 py-6 sm:px-6 lg:px-8 lg:py-8', + ]" + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/layouts/dashboard.vue` at line 30, The main element currently always includes "overflow-y-auto" which prevents true full-bleed; change the class binding on the <main> (the element using :class with isFullbleed) to only include "overflow-y-auto" when isFullbleed is false (e.g., move "overflow-y-auto" into the conditional branch that runs when !isFullbleed) so that when isFullbleed is true the class list includes "overflow-hidden" and not "overflow-y-auto".server/api/applications/[id]/analyze.post.ts-31-33 (1)
31-33:⚠️ Potential issue | 🟠 MajorDon't swallow malformed request bodies.
Catching
readBody()errors here turns a bad request into a default-config analysis run, so malformed payloads can silently use the wrong AI configuration instead of failing fast.♻️ Proposed fix
- const body = await readBody(event).catch(() => null) - const parsedBody = body ? bodySchema.parse(body) : null + const body = await readBody(event) + const parsedBody = body ? bodySchema.parse(body) : null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/applications/`[id]/analyze.post.ts around lines 31 - 33, The code currently swallows readBody() errors by using await readBody(event).catch(() => null) which turns malformed request bodies into a silent default run; remove the .catch and instead call await readBody(event) directly (or use bodySchema.safeParse) and return a 400 Bad Request when readBody or bodySchema parsing fails; specifically update the logic around readBody(event) and bodySchema.parse/bodySchema.safeParse in this handler so that readBody errors and schema validation failures propagate as a client error rather than falling back to null/default config.app/components/ScoreBreakdown.vue-39-50 (1)
39-50:⚠️ Potential issue | 🟠 MajorMake the config picker org-aware.
/api/ai-configis org-scoped (filters byactiveOrganizationId), but this fetch is cached under a static key andselectedAiConfigIdis never reset when the org changes. If a user selects a config in one org, switches to another org, and runs analysis, the component will send the previous org's config ID to the API.Include the org id in the cache key or watch
activeOrgand reset invalid selections.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ScoreBreakdown.vue` around lines 39 - 50, The fetch for AI configs is cached under a static key and selectedAiConfigId is not reset on org change, causing cross-org leakage; update the useFetch key to include the current organization id (e.g. `'ai-configs-analysis-picker-' + activeOrgId`) or derive key from a computed that includes activeOrg, and add a watcher on activeOrg (or activeOrganizationId) to clear or validate selectedAiConfigId (the ref selectedAiConfigId) when the org changes so any previously selected id is reset if it doesn't exist in aiConfigOptions; adjust defaultAnalysisConfig/computed usage accordingly to avoid stale selections.server/api/ai-config/[id]/test-connection.post.ts-24-25 (1)
24-25:⚠️ Potential issue | 🟠 MajorUse write-level permission for connection tests.
On Line 24,
scoring: ['read']allows read-only users to trigger external provider calls (cost/egress side effects). This endpoint should require the same permission level as config management actions.Suggested change
- const session = await requirePermission(event, { scoring: ['read'] }) + const session = await requirePermission(event, { scoring: ['create'] })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/ai-config/`[id]/test-connection.post.ts around lines 24 - 25, The endpoint uses requirePermission(event, { scoring: ['read'] }) which allows read-only users to trigger external provider calls; change the permission to require write-level access by calling requirePermission(event, { scoring: ['write'] }) (or the equivalent write permission set used in your auth model) so that test-connection.post.ts enforces the same write-level permission as config management actions before proceeding with orgId/session usage.server/api/ai-config/[id].delete.ts-22-45 (1)
22-45:⚠️ Potential issue | 🟠 MajorMove existence check into the transaction and verify deletion.
Current flow does a pre-read (Line 22) outside the transaction, then deletes later (Line 29). Under concurrent requests, this can return success for an already-deleted row and run default-promotion based on stale state.
Suggested transactional pattern
- const existing = await db.query.aiConfig.findFirst({ - where: and(eq(aiConfig.id, id), eq(aiConfig.organizationId, orgId)), - columns: { id: true, isDefaultChatbot: true, isDefaultAnalysis: true }, - }) - if (!existing) throw createError({ statusCode: 404, statusMessage: 'AI configuration not found.' }) - await db.transaction(async (tx) => { - await tx.delete(aiConfig).where(and(eq(aiConfig.id, id), eq(aiConfig.organizationId, orgId))) + const existing = await tx.query.aiConfig.findFirst({ + where: and(eq(aiConfig.id, id), eq(aiConfig.organizationId, orgId)), + columns: { id: true, isDefaultChatbot: true, isDefaultAnalysis: true }, + }) + if (!existing) throw createError({ statusCode: 404, statusMessage: 'AI configuration not found.' }) + + const deleted = await tx + .delete(aiConfig) + .where(and(eq(aiConfig.id, id), eq(aiConfig.organizationId, orgId))) + .returning({ id: aiConfig.id }) + if (deleted.length === 0) throw createError({ statusCode: 404, statusMessage: 'AI configuration not found.' }) // Promote a successor for any default slot we just vacated. if (existing.isDefaultChatbot || existing.isDefaultAnalysis) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/ai-config/`[id].delete.ts around lines 22 - 45, The pre-read of the aiConfig (existing) happens outside db.transaction which can race; move the existence check and state read into the same transaction so you read the current row, then perform the delete and verify it affected a row (throw 404 if no rows deleted). Specifically, inside db.transaction use tx.query.aiConfig.findFirst/forUpdate (or an equivalent select) to load the target row (replace the outer existing), then call tx.delete(aiConfig).where(and(eq(aiConfig.id, id), eq(aiConfig.organizationId, orgId))) and ensure the delete returned/affected a row before proceeding; use the loaded row's isDefaultChatbot/isDefaultAnalysis to decide promotion and then tx.update(aiConfig).set(promote).where(eq(aiConfig.id, successor.id)) as before so all reads/changes occur atomically within the same transaction.server/api/ai-analysis/stats.get.ts-23-42 (1)
23-42:⚠️ Potential issue | 🟠 MajorMake pricing resolution deterministic across multiple configs.
Without an explicit
orderBy, map assignment (Line 36) and fallbackpricingConfigs[0](Line 41) depend on DB row order. If the org has multiple configs for the same provider/model, pricing can flip unpredictably.Suggested fix
const pricingConfigs = await db.query.aiConfig.findMany({ where: eq(aiConfig.organizationId, orgId), columns: { provider: true, model: true, inputPricePer1m: true, outputPricePer1m: true, isDefaultAnalysis: true, + createdAt: true, }, + orderBy: (t, { desc }) => [desc(t.isDefaultAnalysis), desc(t.createdAt)], }) const pricingByModel = new Map<string, { inputPricePer1m: number | null, outputPricePer1m: number | null }>() for (const c of pricingConfigs) { - pricingByModel.set(`${c.provider}::${c.model}`, { - inputPricePer1m: c.inputPricePer1m != null ? Number(c.inputPricePer1m) : null, - outputPricePer1m: c.outputPricePer1m != null ? Number(c.outputPricePer1m) : null, - }) + const key = `${c.provider}::${c.model}` + if (!pricingByModel.has(key)) { + pricingByModel.set(key, { + inputPricePer1m: c.inputPricePer1m != null ? Number(c.inputPricePer1m) : null, + outputPricePer1m: c.outputPricePer1m != null ? Number(c.outputPricePer1m) : null, + }) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/ai-analysis/stats.get.ts` around lines 23 - 42, Make pricing resolution deterministic by adding an explicit order to the db.query.aiConfig.findMany call and making map assignment deterministic: update the query (db.query.aiConfig.findMany) to include an orderBy that prefers default analysis entries and a stable tie-breaker (e.g., order by provider, model, isDefaultAnalysis desc, createdAt or id asc), and when populating pricingByModel only set a key if it does not already exist (so the first row according to the new order wins). Also derive defaultAnalysisConfig by taking the first element of the ordered pricingConfigs (pricingConfigs[0]) so the chosen default is deterministic.server/api/chatbot/conversations/index.get.ts-20-23 (1)
20-23:⚠️ Potential issue | 🟠 Major
lastMessageAtordering is inverted for NULL rows.With nullable timestamps,
DESCcan placeNULLrows ahead of active conversations. This breaks the “most-recent” intent. UseNULLS LAST(or equivalent coalescing) forlastMessageAt.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/conversations/index.get.ts` around lines 20 - 23, The ordering on chatbotConversation.lastMessageAt currently uses desc(chatbotConversation.lastMessageAt) which can place NULLs before recent rows; change the ordering to ensure NULLs come last (e.g., use NULLS LAST via the query builder or wrap with coalesce to a very old timestamp) so pinned and most-recent conversations sort correctly; update the orderBy clause that references desc(chatbotConversation.pinned), desc(chatbotConversation.lastMessageAt), desc(chatbotConversation.createdAt) to use a NULLS LAST variant (or coalesce(chatbotConversation.lastMessageAt, <minDate>)) for chatbotConversation.lastMessageAt.server/api/ai-config/[id].patch.ts-37-63 (1)
37-63:⚠️ Potential issue | 🟠 MajorAvoid non-null assertion on update result.
updated!on Line 62 can crash if the row disappears between existence check and update. Add a guard and return a controlled 404.Proposed fix
const [updated] = await db.update(aiConfig) .set(updates) .where(and(eq(aiConfig.id, id), eq(aiConfig.organizationId, orgId))) .returning({ id: aiConfig.id, @@ apiKeyEncrypted: aiConfig.apiKeyEncrypted, }) + + if (!updated) { + throw createError({ statusCode: 404, statusMessage: 'AI configuration not found.' }) + } @@ - const { apiKeyEncrypted, ...rest } = updated! + const { apiKeyEncrypted, ...rest } = updated🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/ai-config/`[id].patch.ts around lines 37 - 63, The code uses a non-null assertion on updated (updated!) after the db.update(...).set(...).where(...).returning(...) call which can throw if the row vanishes between checks; instead check whether updated is undefined and if so return a controlled 404 response (or throw a not-found error) before destructuring; update the handler around the db.update result (variable updated) to guard, log/record the not-found condition if desired, and only destructure apiKeyEncrypted from updated when it is present.server/api/ai-config/index.post.ts-23-27 (1)
23-27:⚠️ Potential issue | 🟠 MajorHandle concurrent default-selection conflicts explicitly.
Lines 23-27 compute
isFirstbefore the transaction. Under concurrent creates, two requests can both attempt default assignment; one will likely fail on unique default indexes and bubble as a generic 500. Handle this as a controlled conflict response (and deriveisFirstinside the transaction).💡 Suggested hardening
- const existingCount = await db.$count(aiConfig, eq(aiConfig.organizationId, orgId)) - const isFirst = existingCount === 0 - const isDefaultChatbot = isFirst || body.isDefaultChatbot === true - const isDefaultAnalysis = isFirst || body.isDefaultAnalysis === true - - const created = await db.transaction(async (tx) => { + let created: { + id: string + name: string + provider: string + model: string + baseUrl: string | null + maxTokens: number + isDefaultChatbot: boolean + isDefaultAnalysis: boolean + } + try { + created = await db.transaction(async (tx) => { + const existingCount = await tx.$count(aiConfig, eq(aiConfig.organizationId, orgId)) + const isFirst = existingCount === 0 + const isDefaultChatbot = isFirst || body.isDefaultChatbot === true + const isDefaultAnalysis = isFirst || body.isDefaultAnalysis === true + if (isDefaultChatbot) { await tx.update(aiConfig) .set({ isDefaultChatbot: false }) .where(eq(aiConfig.organizationId, orgId)) } if (isDefaultAnalysis) { await tx.update(aiConfig) .set({ isDefaultAnalysis: false }) .where(eq(aiConfig.organizationId, orgId)) - } + } - const [row] = await tx.insert(aiConfig) + const [row] = await tx.insert(aiConfig) .values({ organizationId: orgId, name: body.name, @@ isDefaultChatbot, isDefaultAnalysis, }) .returning({ id: aiConfig.id, name: aiConfig.name, @@ isDefaultAnalysis: aiConfig.isDefaultAnalysis, }) - return row! - }) + return row! + }) + } catch (error: any) { + if (error?.code === '23505') { + throw createError({ + statusCode: 409, + statusMessage: 'Default AI configuration changed concurrently. Please retry.', + }) + } + throw error + }Also applies to: 28-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/ai-config/index.post.ts` around lines 23 - 27, Compute and check "isFirst" inside the same DB transaction that creates the aiConfig instead of before it: move the await db.$count(aiConfig, eq(aiConfig.organizationId, orgId)) into the transaction that also performs the create so isFirst is derived atomically; use those transactional values to set isDefaultChatbot and isDefaultAnalysis. Additionally catch the DB unique-constraint error raised when two requests race to set a default (from the create/update inside the transaction) and translate it into a controlled conflict response (e.g., 409) rather than a generic 500, so callers receive a clear conflict for concurrent default-selection; update error handling around the transaction to detect that DB constraint error and return the conflict.app/pages/dashboard/settings/ai/[id].vue-52-55 (1)
52-55:⚠️ Potential issue | 🟠 MajorLoading gate can deadlock on provider-fetch errors.
Line 62 requires
providers.valueto be truthy. If/api/ai-config/providersfails, this page can remain in loading state indefinitely.Suggested fix (explicit default + status-only readiness)
-const { data: providers, status: providersStatus } = useFetch<Record<string, ProviderInfo>>('/api/ai-config/providers', { +const { data: providers, status: providersStatus } = useFetch<Record<string, ProviderInfo>>('/api/ai-config/providers', { key: 'ai-providers', headers: useRequestHeaders(['cookie']), + default: () => ({}), }) @@ -const isReady = computed(() => - configsStatus.value !== 'pending' && providersStatus.value !== 'pending' && providers.value, -) +const isReady = computed(() => + configsStatus.value !== 'pending' && providersStatus.value !== 'pending', +)Also applies to: 61-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/settings/ai/`[id].vue around lines 52 - 55, The page can hang waiting for providers because the template gates on providers.value truthiness; change the useFetch call for providers to provide an explicit default (e.g., empty object) and make loading/readiness checks use providersStatus (or check providersStatus === 'success') instead of providers.value truthiness. Concretely, update the useFetch invocation for providers to include a default/initial value (so providers is never undefined) and replace any conditional like `if (!providers.value)` or template gates that rely on providers.value with checks against `providersStatus` (or `providersStatus === 'success'`) so failures won't keep the page stuck loading.app/components/ConversationItem.vue-81-86 (1)
81-86:⚠️ Potential issue | 🟠 MajorAction trigger is hover-only; add keyboard-visible state.
Line 85 makes the menu button visible only on hover. Keyboard users need a focus-visible equivalent to access actions reliably.
Suggested accessibility-safe visibility classes
- <span class="whitespace-nowrap text-[10px] text-surface-400 dark:text-surface-500 group-hover:invisible"> + <span class="whitespace-nowrap text-[10px] text-surface-400 dark:text-surface-500 group-hover:invisible group-focus-within:invisible"> {{ relativeTime }} </span> <button - class="absolute right-0 size-6 flex items-center justify-center rounded text-surface-500 hover:bg-surface-200 dark:hover:bg-surface-700 invisible group-hover:visible cursor-pointer border-0 bg-transparent" + class="absolute right-0 size-6 flex items-center justify-center rounded text-surface-500 hover:bg-surface-200 dark:hover:bg-surface-700 invisible group-hover:visible group-focus-within:visible focus-visible:visible cursor-pointer border-0 bg-transparent" title="Actions" `@click.stop`="(e) => emit('toggleMenu', e)" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ConversationItem.vue` around lines 81 - 86, The menu button is only exposed on hover in ConversationItem.vue (the button with classes including "invisible group-hover:visible"), which prevents keyboard users from accessing it; update the button's visibility rules to also become visible when focused (e.g., add focus-visible:visible and focus:visible or focus-within:visible utility classes) and ensure it is keyboard-focusable (has no tabindex="-1" and uses a semantic button element) so the action menu can be triggered by keyboard navigation.app/components/ChatbotAgentManagerModal.vue-67-72 (1)
67-72:⚠️ Potential issue | 🟠 MajorAdd temperature validation and normalization to prevent empty string submission.
When the temperature input is empty or contains invalid text, Vue 3's
v-model.numberassigns an empty string""instead ofnull. ThecanSavecheck currently doesn't validate this field, so an empty string gets sent to the server, failing Zod validation which expectsz.number().min(0).max(2).optional().nullable()(lines 16 in POST and line 15 in PATCH endpoints). Add avalidTemperaturecomputed property to checkdraft.value.temperatureis eithernullor a finite number between 0 and 2, and normalize the payload to sendnullinstead of empty strings.Suggested guard + normalization
const promptTooLong = computed(() => draft.value.systemPrompt.length > CHATBOT_AGENT_PROMPT_MAX) +const validTemperature = computed(() => { + const t = draft.value.temperature + return t === null || (typeof t === 'number' && Number.isFinite(t) && t >= 0 && t <= 2) +}) const canSave = computed(() => !saving.value && draft.value.name.trim().length > 0 && draft.value.systemPrompt.trim().length > 0 - && !promptTooLong.value, + && !promptTooLong.value + && validTemperature.value, ) @@ - const payload = { + const t = draft.value.temperature + const normalizedTemperature = + typeof t === 'number' && Number.isFinite(t) ? t : null + + const payload = { name: draft.value.name.trim(), description: draft.value.description.trim() || null, icon: draft.value.icon.trim() || null, systemPrompt: draft.value.systemPrompt, - temperature: draft.value.temperature, + temperature: normalizedTemperature, isDefault: draft.value.isDefault, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ChatbotAgentManagerModal.vue` around lines 67 - 72, Add a computed validTemperature that returns true only if draft.value.temperature is null or a finite number between 0 and 2, include validTemperature in the canSave computed so saving is blocked for invalid/empty temperature, and normalize draft.value.temperature to null (not empty string) when building the POST/PATCH payload (i.e., convert "" -> null) so the server receives a nullable number matching the Zod schema; reference canSave and draft.value.temperature when implementing these changes.server/utils/schemas/scoring.ts-17-17 (1)
17-17:⚠️ Potential issue | 🟠 MajorValidate trimmed
name, not raw input.With
.min(1)...trim(), whitespace-only values pass validation and become empty after transform. Put.trim()before length constraints.✅ Proposed fix
- name: z.string().min(1).max(80).trim(), + name: z.string().trim().min(1).max(80), ... - name: z.string().min(1).max(80).trim().optional(), + name: z.string().trim().min(1).max(80).optional(),Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/schemas/scoring.ts` at line 17, The schema currently applies trim after length checks for the "name" field (and the other string at line 31), allowing whitespace-only values to pass; move the trim call to run before the min and max checks so the string is trimmed prior to length validation (i.e., apply trim first for the name field and the other offending string validator in the same schema).app/components/JobSubNavActions.vue-66-77 (1)
66-77:⚠️ Potential issue | 🟠 Major
isDeletingis not reset on successful delete path.The success path never clears
isDeleting, which can leave the UI stuck if navigation does not occur immediately.✅ Proposed fix
async function handleDelete() { isDeleting.value = true try { track('job_deleted', { job_id: props.jobId }) await deleteJob() + showDeleteConfirm.value = false } catch (err: any) { if (handlePreviewReadOnlyError(err)) return toast.error('Failed to delete job', { message: err.data?.statusMessage, statusCode: err.data?.statusCode }) - isDeleting.value = false showDeleteConfirm.value = false + } finally { + isDeleting.value = false } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/JobSubNavActions.vue` around lines 66 - 77, The success path in handleDelete leaves isDeleting.value true, which can freeze the UI; after awaiting deleteJob() (in function handleDelete) set isDeleting.value = false (and optionally reset showDeleteConfirm.value = false) before any navigation or return so the UI state is cleared even if navigation is delayed; keep the existing error handling (handlePreviewReadOnlyError, toast) unchanged.server/utils/ai/chatTools.ts-43-55 (1)
43-55:⚠️ Potential issue | 🟠 MajorFail closed when job scope is malformed (
kind: "job"withoutjobId).Current checks only enforce restrictions when
scope.jobIdexists, which can degrade to org-wide access if malformed scope slips through.🔒 Proposed fail-closed guard
function assertJobInScope(scope: ChatbotScope, jobId: string) { - if (scope.kind === 'job' && scope.jobId && scope.jobId !== jobId) { - throw new Error(`Job ${jobId} is outside the active scope.`) + if (scope.kind === 'job') { + if (!scope.jobId) throw new Error('Invalid job scope: missing jobId.') + if (scope.jobId !== jobId) { + throw new Error(`Job ${jobId} is outside the active scope.`) + } } } function jobScopeFilter(orgId: string, scope: ChatbotScope) { const base = eq(job.organizationId, orgId) - if (scope.kind === 'job' && scope.jobId) { - return and(base, eq(job.id, scope.jobId)) + if (scope.kind === 'job') { + if (!scope.jobId) throw new Error('Invalid job scope: missing jobId.') + return and(base, eq(job.id, scope.jobId)) } return base }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/utils/ai/chatTools.ts` around lines 43 - 55, Both functions currently assume that when scope.kind === 'job' a scope.jobId may be present and only narrow access when it exists; you must make them fail-closed when a malformed job scope (kind: "job" without jobId) is encountered. Update assertJobInScope to throw immediately if scope.kind === 'job' && !scope.jobId, and keep the existing mismatch check (throw if scope.jobId exists and differs from jobId). Update jobScopeFilter to similarly throw if scope.kind === 'job' && !scope.jobId rather than falling back to org-wide filter; otherwise preserve current behavior (return and(base, eq(job.id, scope.jobId)) when jobId present, else base).app/composables/useFeatureFlag.ts-65-67 (1)
65-67:⚠️ Potential issue | 🟠 MajorAdd cleanup for the PostHog feature-flag listener subscription.
posthog.onFeatureFlags(apply)registers a callback that is never unsubscribed. Each time this composable is instantiated, a new listener is added without removing the previous one, causing accumulated subscriptions and duplicate updates on re-mounts.PostHog's
onFeatureFlagsreturns an unsubscribe function. UseonScopeDisposeto ensure it is called when the composable scope is disposed:🔧 Cleanup pattern
apply() // immediate read in case flags are already cached - posthog.onFeatureFlags(apply) // re-evaluates after fetch / identify / reload + const unsubscribe = posthog.onFeatureFlags(apply) // re-evaluates after fetch / identify / reload + if (typeof unsubscribe === 'function') { + onScopeDispose(unsubscribe) + }Import
onScopeDisposefrom Vue if not already available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/composables/useFeatureFlag.ts` around lines 65 - 67, The posthog.onFeatureFlags(apply) listener is never unsubscribed causing duplicate callbacks; update the useFeatureFlag composable to capture the unsubscribe function returned by posthog.onFeatureFlags when registering (posthog.onFeatureFlags(apply)) and call that unsubscribe inside Vue's onScopeDispose so the listener is removed when the composable scope is destroyed; import onScopeDispose if missing and ensure apply remains registered immediately (apply()) but the subscription is cleaned up via the unsubscribe in onScopeDispose.server/api/chatbot/chat.post.ts-346-364 (1)
346-364:⚠️ Potential issue | 🟠 MajorDon't stream raw internal error text back to the client.
Both tool errors and top-level stream errors forward the original exception message to the UI. That can leak provider, tool, or database internals. Log the raw error server-side, but emit a generic user-facing message on the stream.
💡 Suggested fix
case 'tool-error': { const existing = toolCallById.get(part.toolCallId) const errMsg = part.error instanceof Error ? part.error.message : String(part.error) if (existing) { existing.output = { error: errMsg } existing.status = 'error' } writeEvent(controller, { type: 'tool-error', id: part.toolCallId, - error: errMsg, + error: 'Tool execution failed.', }) break } case 'error': writeEvent(controller, { type: 'error', - error: part.error instanceof Error ? part.error.message : String(part.error), + error: 'The assistant hit an internal error. Please try again.', }) breakAlso applies to: 383-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/chat.post.ts` around lines 346 - 364, The code currently streams raw error messages for tool errors and top-level errors (see handling around tool-error and error cases using toolCallById, existing, writeEvent, controller, and part), which can leak internals; instead, log the full error on the server (e.g., using your existing logger) and send a sanitized generic message to the client via writeEvent (for tool-error send a generic "Tool failed" or similar alongside the toolCallId and set status 'error', for error send a generic "An internal error occurred"); ensure you still update existing.output/status as before but replace the client-facing error text with the generic string while recording the real part.error in server logs.server/database/schema/app.ts-819-823 (1)
819-823:⚠️ Potential issue | 🟠 MajorAdd tenant-scoped foreign keys for the chatbot tables.
These FKs only validate raw IDs. A conversation can point at another user's folder/agent, and a message can carry
organizationId/userIdvalues that don't match itsconversationId. In a multi-tenant feature marked as private-per-user, that integrity should be enforced in the schema, not only in the API layer.Also applies to: 846-863
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/database/schema/app.ts` around lines 819 - 823, The schema currently creates single-column FKs (folderId, agentId, aiConfigId) that only validate raw IDs (chatbotFolder, chatbotAgent, aiConfig) which allows cross-tenant references; modify those constraints to be tenant-scoped by adding composite foreign keys that include the owning tenant/org column (e.g., reference (organizationId, folder_id) -> chatbotFolder(organization_id, id) and similarly for agent and aiConfig), and apply the same pattern to the conversation and message-related tables so conversation.organizationId and message.organizationId/userId are validated against the referenced conversation/owner rows (use composite references like (organizationId, conversation_id) and (organizationId, user_id) where appropriate); update the FK definitions for the symbols folderId, agentId, aiConfigId, chatbotFolder, chatbotAgent, aiConfig, conversation and message to enforce organization-scoped integrity.app/components/AiConfigForm.vue-182-205 (1)
182-205:⚠️ Potential issue | 🟠 Major
Test connectionis testing the persisted config, not the current form.After changing provider/model/base URL/API key, this still calls the saved-config endpoint with no draft payload, so the result can say "works" for settings the user has already changed. Either disable the button until the draft is saved, or add a test endpoint that accepts the unsaved form values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/AiConfigForm.vue` around lines 182 - 205, The handleTest function currently tests the persisted config by calling /api/ai-config/:id/test-connection, so changes in the current form (draft) aren’t validated; update handleTest to POST the current form/draft payload instead of only calling the saved-config endpoint when there are unsaved edits (use the reactive form object such as the local draft/config form value you have in the component), or call a new endpoint like POST /api/ai-config/test-connection that accepts a JSON body with provider/model/baseUrl/apiKey; keep existing behavior for pure "edit saved" flows (props.config path) but when isEdit is true and there are unsaved changes send the draft JSON body, include headers (useRequestHeaders(['cookie']) and Content-Type application/json), and continue to set isTesting/testResult and toast messages the same way.server/api/chatbot/chat.post.ts-184-193 (1)
184-193:⚠️ Potential issue | 🟠 MajorFail when any referenced attachment has expired, not only all of them.
Right now a request with 3 attachment IDs still succeeds if only 1 resolves. That silently drops the missing files from the model context and from the persisted snapshot. Compare the resolved count against the requested IDs and return
410unless all referenced attachments are still available.💡 Suggested fix
- if (attachmentIds.length > 0 && attachmentRecords.length === 0) { + if (attachmentRecords.length !== attachmentIds.length) { throw createError({ statusCode: 410, statusMessage: 'Attachments expired. Please re-upload your files.', }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/chat.post.ts` around lines 184 - 193, The code currently treats attachments as present if getChatbotAttachments returns any records; instead ensure you await getChatbotAttachments(session.user.id, attachmentIds), then compare attachmentRecords.length to attachmentIds.length and throw the 410 createError unless they are equal (so any missing attachment triggers the error). Update the block that defines attachmentRecords and the following conditional to use the awaited result and strict count comparison.server/api/chatbot/chat.post.ts-179-182 (1)
179-182:⚠️ Potential issue | 🟠 MajorRequire the final message to be the active user turn.
This picks the most recent user message anywhere in the array, not necessarily the last message in the request. If the payload ends with an assistant message, you'll persist an older user turn again and stream against mismatched history. Reject requests unless the final entry is a user message.
💡 Suggested fix
- const lastUser = [...body.messages].reverse().find((m) => m.role === 'user') - if (!lastUser) { - throw createError({ statusCode: 400, statusMessage: 'No user message in request.' }) - } + const lastUser = body.messages[body.messages.length - 1] + if (!lastUser || lastUser.role !== 'user') { + throw createError({ statusCode: 400, statusMessage: 'Last message must be a user message.' }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/chat.post.ts` around lines 179 - 182, The code currently finds the most recent user message via reverse().find() (lastUser) which can return an earlier user turn even if the request ends with an assistant message; change the logic to require the final message in body.messages to be a user turn: check body.messages is non-empty, inspect the last element (e.g., const last = body.messages[body.messages.length-1]) and if last.role !== 'user' throw createError({ statusCode: 400, statusMessage: 'Final message must be a user turn.' }); remove the reverse().find() usage and only accept requests whose last message is role === 'user'.app/components/AiConfigForm.vue-150-158 (1)
150-158:⚠️ Potential issue | 🟠 MajorClear
baseUrlwhen switching an existing config back to a standard provider.In edit mode this omits
baseUrlentirely unless the provider is custom, so a previously saved custom endpoint can survive a switch toopenaiand keep routing requests to the stale URL. Sendnullon PATCH when the selected provider no longer uses a base URL.💡 Suggested fix
- if (isCustomProvider.value) body.baseUrl = form.value.baseUrl + if (isCustomProvider.value) body.baseUrl = form.value.baseUrl.trim() + else if (isEdit.value) body.baseUrl = null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/AiConfigForm.vue` around lines 150 - 158, The PATCH request currently only sets body.baseUrl when isCustomProvider.value is true, leaving a previous custom URL intact when switching back to a standard provider; update the code that builds body (used in the isEdit / props.config PATCH to `/api/ai-config/${props.config.id}`) so that if isCustomProvider.value is false you explicitly set body.baseUrl = null (instead of omitting it), while still assigning body.baseUrl = form.value.baseUrl when isCustomProvider.value is true and keeping body.apiKey = form.value.apiKey as before.server/database/schema/app.ts-773-791 (1)
773-791:⚠️ Potential issue | 🟠 MajorEnforce a single default agent per user at the database level.
isDefaultis described as the preselected agent, but nothing stops multiple rows from beingtruefor the same(organizationId, userId). Add a partial unique index so this invariant survives concurrent writes and API bugs.💡 Suggested fix
}, (t) => ([ index('chatbot_agent_org_user_idx').on(t.organizationId, t.userId), + uniqueIndex('chatbot_agent_default_org_user_idx') + .on(t.organizationId, t.userId) + .where(sql`${t.isDefault} = true`), ]))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/database/schema/app.ts` around lines 773 - 791, The chatbotAgent table allows multiple rows with isDefault=true per (organizationId, userId); add a partial unique index to enforce at most one default per user: create a unique index on (organization_id, user_id) where is_default = true (e.g., name it chatbot_agent_org_user_default_idx) alongside the existing index in the pgTable/schema definition (referencing chatbotAgent, isDefault, organizationId, userId) and ensure the change is applied via a DB migration so concurrent writes are prevented at the database level.app/pages/dashboard/chatbot/[[id]].vue-127-140 (1)
127-140:⚠️ Potential issue | 🟠 MajorDon't clear the composer before
send()succeeds.Pressing Enter can submit an empty draft, and clearing
draftbefore awaitingsend()drops the user's message on failures. Guard the empty case here and only reset the textarea after a successful send.💡 Suggested fix
async function handleSubmit() { if (isStreaming.value) return const content = draft.value - draft.value = '' - await nextTick(autoResize) + if (!content.trim() && pendingAttachments.value.length === 0) return await send(content) + draft.value = '' + await nextTick(autoResize) await nextTick(scrollToBottom) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/chatbot/`[[id]].vue around lines 127 - 140, handleSubmit currently clears draft.value before awaiting send(), which allows submission of empty messages and loses the message on send failure; change handleSubmit to first validate draft.value is non-empty, then call send(content) and only after send resolves successfully set draft.value = '' and run nextTick(autoResize) and nextTick(scrollToBottom); also ensure onKeyDown still prevents default and calls handleSubmit, and keep the isStreaming check in handleSubmit to avoid concurrent submits.
🟡 Minor comments (7)
server/api/chatbot/upload.post.ts-36-39 (1)
36-39:⚠️ Potential issue | 🟡 MinorReject multiple
fileparts explicitly.The handler currently takes the first matching part and silently discards the rest, which makes the "single file upload" contract ambiguous for clients.
🧩 Suggested fix
const form = await readMultipartFormData(event) - const filePart = form?.find((p) => p.name === 'file') - if (!filePart?.data || !filePart.filename) { + const fileParts = (form ?? []).filter((p) => p.name === 'file' && p.data && p.filename) + if (fileParts.length !== 1) { + throw createError({ statusCode: 400, statusMessage: 'Exactly one file is required' }) + } + const filePart = fileParts[0]! + if (!filePart.data || !filePart.filename) { throw createError({ statusCode: 400, statusMessage: 'No file provided' }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/upload.post.ts` around lines 36 - 39, The handler currently selects the first matching part and ignores others; update the upload logic (using readMultipartFormData, form and filePart) to explicitly detect and reject multiple parts named "file" by counting form.filter(p => p.name === 'file') and throwing a 400 error (e.g., "Multiple files provided; only one file allowed") if the count is > 1, while retaining the existing validation that a single filePart has .data and .filename before proceeding.server/api/chatbot/agents/index.get.ts-25-35 (1)
25-35:⚠️ Potential issue | 🟡 MinorPreserve zero-valued temperatures.
This truthiness check turns
0intonull, so a deterministic agent can round-trip as “unset.” Use a nullish check instead.Suggested fix
- temperature: r.temperature ? Number(r.temperature) : null, + temperature: r.temperature == null ? null : Number(r.temperature),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/agents/index.get.ts` around lines 25 - 35, Replace the truthy check that converts zero to null in the agents mapping: change the temperature line in the rows.map callback from "temperature: r.temperature ? Number(r.temperature) : null" to a nullish check that preserves 0, e.g. "temperature: r.temperature != null ? Number(r.temperature) : null" so zero-valued temperatures remain numeric while still mapping undefined/null to null.app/components/ChatbotSourcesPanel.vue-66-71 (1)
66-71:⚠️ Potential issue | 🟡 MinorAdd an accessible name to the close button.
The icon-only button has no
aria-label, so screen readers won’t announce its purpose.Proposed fix
<button + aria-label="Close sources panel" class="inline-flex size-7 items-center justify-center rounded text-surface-500 hover:bg-surface-200 dark:hover:bg-surface-800 cursor-pointer border-0 bg-transparent" `@click`="emit('close')" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ChatbotSourcesPanel.vue` around lines 66 - 71, The close button in ChatbotSourcesPanel.vue is icon-only and lacks an accessible name; update the button element (the one calling emit('close') and rendering the <X /> icon) to include an accessible label by adding an aria-label (e.g., aria-label="Close") or aria-labelledby referencing a hidden/visually-hidden text element, ensuring screen readers announce its purpose while keeping the visual icon unchanged.server/api/chatbot/folders/[id].patch.ts-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorTrim before validating non-empty folder names.
On Line 8,
z.string().min(1).trim()can accept whitespace-only input and then persist''after trim. Put.trim()before.min(1).Proposed fix
- name: z.string().min(1).max(80).trim().optional(), + name: z.string().trim().min(1).max(80).optional(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/folders/`[id].patch.ts at line 8, The zod schema for the folder "name" currently calls .min(1) before .trim(), so inputs of only whitespace can pass validation then be persisted as empty strings; update the schema call order on the "name" field (the z.string(...).min(...).max(...).trim().optional() expression) to apply .trim() before .min(1) (e.g., z.string().trim().min(1).max(80).optional()) so trimming happens prior to the non-empty check.app/components/ChatbotSidebar.vue-136-145 (1)
136-145:⚠️ Potential issue | 🟡 MinorUse floor-based bucketing for relative time.
Line 139 (and derived calculations) uses
Math.round, which makes timestamps appear older too early. UseMath.floorto avoid premature jumps between units.💡 Suggested fix
- const mins = Math.round(diff / 60_000) + const mins = Math.floor(diff / 60_000) if (mins < 1) return 'just now' if (mins < 60) return `${mins}m` - const hrs = Math.round(mins / 60) + const hrs = Math.floor(mins / 60) if (hrs < 24) return `${hrs}h` - const days = Math.round(hrs / 24) + const days = Math.floor(hrs / 24)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/ChatbotSidebar.vue` around lines 136 - 145, The relativeTime function is using Math.round which causes premature unit jumps; update the calculations in relativeTime (function relativeTime(ms: number | null)) to use Math.floor for mins, hrs, and days bucketing (i.e., compute mins = Math.floor(diff / 60_000), hrs = Math.floor(mins / 60), days = Math.floor(hrs / 24)) and keep the same early-return behavior for null/0 and each unit threshold so timestamps progress only when the full unit has elapsed.server/api/chatbot/folders/index.post.ts-10-12 (1)
10-12:⚠️ Potential issue | 🟡 MinorPrevent whitespace-only folder names by reordering string checks.
Line 11 chains
.min(1)before.trim(), which allows whitespace-only input to pass. In Zod v4, validators like.min()execute before transforms like.trim(), so" "(length 1) satisfies.min(1)but then becomes""after trimming. Reorder to.trim().min(1).max(80)to trim first, then validate length.Suggested fix
- name: z.string().min(1).max(80).trim(), + name: z.string().trim().min(1).max(80),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/api/chatbot/folders/index.post.ts` around lines 10 - 12, The name field in the Zod schema (bodySchema inside z.object) applies .min(1) before .trim(), allowing whitespace-only names to pass; change the validator chain on the name property to trim first then validate (e.g., use .trim().min(1).max(80)) so whitespace is removed before length checks; update the name schema declaration accordingly.app/pages/dashboard/chatbot/[[id]].vue-20-21 (1)
20-21:⚠️ Potential issue | 🟡 MinorGate the loaders, not just the markup.
loadAll()and the top-level/api/jobsfetch still run when the feature flag is off, so flagged-off users still hit chatbot-related loading paths on page load. Short-circuit the data loading as well, or move the flag check into route middleware.Also applies to: 83-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/dashboard/chatbot/`[[id]].vue around lines 20 - 21, The feature flag check (useFeatureFlagEnabled) currently only hides markup but still allows data loading—prevent loadAll() and the top-level fetch('/api/jobs') from running when enabled is false by short-circuiting the data-loading logic (e.g., in the component setup or created hook) so those calls are skipped when useFeatureFlagEnabled('chatbot-experience') is false; alternatively move the gate into route middleware so loadAll() and the API fetch never execute for flagged-off users (also apply the same change to the code block around loadAll() at the 83-93 section).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89ed0b05-6dc6-493f-b98f-c0186ce4ad26
⛔ Files ignored due to path filters (4)
public/reqcore-emoji-128-transparent.pngis excluded by!**/*.pngpublic/reqcore-emoji-128.pngis excluded by!**/*.pngpublic/reqcore-emoji-256-transparent.pngis excluded by!**/*.pngpublic/reqcore-emoji-256.pngis excluded by!**/*.png
📒 Files selected for processing (74)
.env.exampleSELF-HOSTING.mdapp/components/AiConfigForm.vueapp/components/AppTopBar.vueapp/components/ChatbotAgentManagerModal.vueapp/components/ChatbotAgentPicker.vueapp/components/ChatbotModelPicker.vueapp/components/ChatbotSidebar.vueapp/components/ChatbotSourcesPanel.vueapp/components/ConversationItem.vueapp/components/JobSubNavActions.vueapp/components/MarkdownDescription.vueapp/components/OrgSwitcher.vueapp/components/ScoreBreakdown.vueapp/composables/useChatbot.tsapp/composables/useFeatureFlag.tsapp/layouts/dashboard.vueapp/pages/dashboard/chatbot/[[id]].vueapp/pages/dashboard/jobs/[id]/ai-analysis.vueapp/pages/dashboard/jobs/[id]/application-form.vueapp/pages/dashboard/jobs/[id]/candidates.vueapp/pages/dashboard/jobs/[id]/index.vueapp/pages/dashboard/jobs/[id]/settings.vueapp/pages/dashboard/jobs/new.vueapp/pages/dashboard/settings/ai.vueapp/pages/dashboard/settings/ai/[id].vueapp/pages/dashboard/settings/ai/index.vueapp/pages/dashboard/settings/ai/new.vueapp/types/router.d.tsnuxt.config.tspublic/reqcore-emoji-128-transparent.webppublic/reqcore-emoji-256-transparent.webpserver/api/ai-analysis/stats.get.tsserver/api/ai-config/[id].delete.tsserver/api/ai-config/[id].get.tsserver/api/ai-config/[id].patch.tsserver/api/ai-config/[id]/set-default.post.tsserver/api/ai-config/[id]/test-connection.post.tsserver/api/ai-config/generate-criteria.post.tsserver/api/ai-config/index.get.tsserver/api/ai-config/index.post.tsserver/api/applications/[id]/analyze.post.tsserver/api/chatbot/agents/[id].delete.tsserver/api/chatbot/agents/[id].patch.tsserver/api/chatbot/agents/index.get.tsserver/api/chatbot/agents/index.post.tsserver/api/chatbot/chat.post.tsserver/api/chatbot/conversations/[id].delete.tsserver/api/chatbot/conversations/[id].get.tsserver/api/chatbot/conversations/[id].patch.tsserver/api/chatbot/conversations/index.get.tsserver/api/chatbot/conversations/index.post.tsserver/api/chatbot/folders/[id].delete.tsserver/api/chatbot/folders/[id].patch.tsserver/api/chatbot/folders/index.get.tsserver/api/chatbot/folders/index.post.tsserver/api/chatbot/upload.post.tsserver/database/migrations/0025_chatbot_persistence.sqlserver/database/migrations/0026_multi_ai_configs.sqlserver/database/migrations/meta/0025_snapshot.jsonserver/database/migrations/meta/_journal.jsonserver/database/schema/app.tsserver/utils/ai/autoScore.tsserver/utils/ai/chatTools.tsserver/utils/ai/loadConfig.tsserver/utils/ai/provider.tsserver/utils/chatbotAccess.tsserver/utils/chatbotAttachments.tsserver/utils/chatbotSources.tsserver/utils/featureFlags.tsserver/utils/posthog.tsserver/utils/schemas/scoring.tsshared/chatbot.tsshared/feature-flags.ts
💤 Files with no reviewable changes (1)
- app/pages/dashboard/settings/ai.vue
Comment on lines
+32
to
+67
| // Enforce per-user cap. | ||
| const [{ value: existing } = { value: 0 }] = await db | ||
| .select({ value: count() }) | ||
| .from(chatbotAgent) | ||
| .where(and( | ||
| eq(chatbotAgent.organizationId, orgId), | ||
| eq(chatbotAgent.userId, userId), | ||
| )) | ||
|
|
||
| if (existing >= CHATBOT_AGENT_MAX_PER_USER) { | ||
| throw createError({ | ||
| statusCode: 422, | ||
| statusMessage: `Agent limit reached (${CHATBOT_AGENT_MAX_PER_USER}). Delete an agent before adding another.`, | ||
| }) | ||
| } | ||
|
|
||
| // If marking this agent as default, unset any previous default. | ||
| if (body.isDefault) { | ||
| await db.update(chatbotAgent) | ||
| .set({ isDefault: false, updatedAt: new Date() }) | ||
| .where(and( | ||
| eq(chatbotAgent.organizationId, orgId), | ||
| eq(chatbotAgent.userId, userId), | ||
| )) | ||
| } | ||
|
|
||
| const [created] = await db.insert(chatbotAgent).values({ | ||
| organizationId: orgId, | ||
| userId, | ||
| name: body.name, | ||
| description: body.description ?? null, | ||
| icon: body.icon ?? null, | ||
| systemPrompt: body.systemPrompt, | ||
| temperature: typeof body.temperature === 'number' ? String(body.temperature) : null, | ||
| isDefault: body.isDefault === true, | ||
| }).returning() |
There was a problem hiding this comment.
Create flow has concurrency holes (cap + default uniqueness).
Line 33 (count), Line 50 (clear defaults), and Line 58 (insert) are non-atomic. Concurrent requests can exceed CHATBOT_AGENT_MAX_PER_USER and can also produce multiple defaults.
Suggested direction (atomic write path)
- // Enforce per-user cap.
- const [{ value: existing } = { value: 0 }] = await db
- .select({ value: count() })
- .from(chatbotAgent)
- .where(and(
- eq(chatbotAgent.organizationId, orgId),
- eq(chatbotAgent.userId, userId),
- ))
-
- if (existing >= CHATBOT_AGENT_MAX_PER_USER) {
- throw createError({
- statusCode: 422,
- statusMessage: `Agent limit reached (${CHATBOT_AGENT_MAX_PER_USER}). Delete an agent before adding another.`,
- })
- }
-
- // If marking this agent as default, unset any previous default.
- if (body.isDefault) {
- await db.update(chatbotAgent)
- .set({ isDefault: false, updatedAt: new Date() })
- .where(and(
- eq(chatbotAgent.organizationId, orgId),
- eq(chatbotAgent.userId, userId),
- ))
- }
-
- const [created] = await db.insert(chatbotAgent).values({
+ const [created] = await db.transaction(async (tx) => {
+ const [{ value: existing } = { value: 0 }] = await tx
+ .select({ value: count() })
+ .from(chatbotAgent)
+ .where(and(
+ eq(chatbotAgent.organizationId, orgId),
+ eq(chatbotAgent.userId, userId),
+ ))
+
+ if (existing >= CHATBOT_AGENT_MAX_PER_USER) {
+ throw createError({
+ statusCode: 422,
+ statusMessage: `Agent limit reached (${CHATBOT_AGENT_MAX_PER_USER}). Delete an agent before adding another.`,
+ })
+ }
+
+ if (body.isDefault) {
+ await tx.update(chatbotAgent)
+ .set({ isDefault: false, updatedAt: new Date() })
+ .where(and(
+ eq(chatbotAgent.organizationId, orgId),
+ eq(chatbotAgent.userId, userId),
+ ))
+ }
+
+ return tx.insert(chatbotAgent).values({
organizationId: orgId,
userId,
name: body.name,
description: body.description ?? null,
icon: body.icon ?? null,
systemPrompt: body.systemPrompt,
temperature: typeof body.temperature === 'number' ? String(body.temperature) : null,
isDefault: body.isDefault === true,
- }).returning()
+ }).returning()
+ })Also add a DB-level uniqueness guard for (organizationId, userId) where isDefault=true so invariants hold even under races.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/chatbot/agents/index.post.ts` around lines 32 - 67, Wrap the
read/count, the “clear defaults” update, and the insert into a single DB
transaction to make the flow atomic: begin a transaction via your DB client
(db.transaction), within it perform a row-level lock on the user's agents (e.g.,
SELECT ... FOR UPDATE or an equivalent via chatbotAgent where
organizationId/userId) before re-counting, then enforce the
CHATBOT_AGENT_MAX_PER_USER check, run the update that clears previous defaults,
perform the insert, and commit; additionally add a DB-level uniqueness guard (a
unique partial index/constraint) on (organizationId, userId) where isDefault =
true so only one default can exist even under races. Ensure you reference
chatbotAgent, count(), CHATBOT_AGENT_MAX_PER_USER and body.isDefault in the
transaction, and add a migration that creates the partial unique index to
enforce uniqueness at the DB layer.
Comment on lines
+83
to
+87
| ALTER TABLE "chatbot_conversation" ADD CONSTRAINT "chatbot_conversation_folder_id_chatbot_folder_id_fk" FOREIGN KEY ("folder_id") REFERENCES "public"."chatbot_folder"("id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "chatbot_conversation" ADD CONSTRAINT "chatbot_conversation_agent_id_chatbot_agent_id_fk" FOREIGN KEY ("agent_id") REFERENCES "public"."chatbot_agent"("id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "chatbot_message" ADD CONSTRAINT "chatbot_message_conversation_id_chatbot_conversation_id_fk" FOREIGN KEY ("conversation_id") REFERENCES "public"."chatbot_conversation"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "chatbot_message" ADD CONSTRAINT "chatbot_message_organization_id_organization_id_fk" FOREIGN KEY ("organization_id") REFERENCES "public"."organization"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "chatbot_message" ADD CONSTRAINT "chatbot_message_user_id_user_id_fk" FOREIGN KEY ("user_id") REFERENCES "public"."user"("id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint |
There was a problem hiding this comment.
Foreign keys don’t enforce tenant/user ownership consistency.
These FKs validate only referenced id, not matching organization_id/user_id. That permits cross-tenant linkage if a foreign id is supplied. Enforce org/user consistency at the DB layer (composite constraints or equivalent integrity checks).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/database/migrations/0025_chatbot_persistence.sql` around lines 83 -
87, The foreign keys added (constraints
chatbot_conversation_folder_id_chatbot_folder_id_fk,
chatbot_conversation_agent_id_chatbot_agent_id_fk,
chatbot_message_conversation_id_chatbot_conversation_id_fk,
chatbot_message_organization_id_organization_id_fk,
chatbot_message_user_id_user_id_fk) only validate referenced ids and do not
ensure organization/user tenant consistency; update the schema so FK checks
include tenant columns (e.g., replace or add composite FKs that reference (id,
organization_id) or (id, user_id) on chatbot_folder, chatbot_agent,
chatbot_conversation, organization and user tables respectively) or implement
equivalent DB-side enforcement (triggers or CHECK USING subqueries) that ensures
referenced rows have matching organization_id/user_id, and drop the old
single-column FKs once the composite/triggers are in place to prevent
cross-tenant links.
…ing PR title linting and release verification
…elated logic for attachment management
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
SELF-HOSTING.md (1)
94-96: Avoid wildcardcdto keep extraction deterministic.At Line 95,
cd reqcore-*can break if multiple matching directories already exist.Suggested fix
curl -fsSL -o reqcore.tar.gz https://github.com/reqcore-inc/reqcore/releases/latest/download/reqcore-$(curl -fsSL https://api.github.com/repos/reqcore-inc/reqcore/releases/latest | grep tag_name | cut -d '"' -f 4 | sed 's/^v//').tar.gz -tar -xzf reqcore.tar.gz && cd reqcore-* +bundle_dir="$(tar -tzf reqcore.tar.gz | head -1 | cut -d/ -f1)" +tar -xzf reqcore.tar.gz +cd "$bundle_dir"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@SELF-HOSTING.md` around lines 94 - 96, Replace the non-deterministic wildcard change "cd reqcore-*" by programmatically determining the single extracted directory from the downloaded archive (reqcore.tar.gz) and cd into that exact directory; specifically, after downloading/extracting the archive (reqcore.tar.gz) use a reliable method to obtain the top-level folder name created by tar (e.g., list the archive contents or inspect the extracted entries) and then cd into that captured directory name instead of using the glob "reqcore-*"..github/workflows/release-verification.yml (1)
57-60: Validate exact pinned tag after replacement.At Line 57 and Line 143,
sedcan succeed without actually replacing anything. Current validation at Line 60 does not assert the expected tag value.Suggested fix
- name: Pin compose file to the released image tag run: | set -euo pipefail + expected="ghcr.io/reqcore-inc/reqcore:${{ steps.tag.outputs.version }}" sed -i \ "s|ghcr.io/reqcore-inc/reqcore:latest|ghcr.io/reqcore-inc/reqcore:${{ steps.tag.outputs.version }}|" \ docker-compose.production.yml - grep "ghcr.io/reqcore-inc/reqcore" docker-compose.production.yml + grep -q "$expected" docker-compose.production.yml \ + || { echo "❌ Failed to pin compose image to $expected"; exit 1; }sed \ "s|ghcr.io/reqcore-inc/reqcore:latest|ghcr.io/reqcore-inc/reqcore:${{ steps.tag.outputs.version }}|" \ docker-compose.production.yml \ > "bundle/reqcore-${{ steps.tag.outputs.version }}/docker-compose.production.yml" + grep -q "ghcr.io/reqcore-inc/reqcore:${{ steps.tag.outputs.version }}" \ + "bundle/reqcore-${{ steps.tag.outputs.version }}/docker-compose.production.yml" \ + || { echo "❌ Bundle compose file was not pinned correctly"; exit 1; }Also applies to: 143-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-verification.yml around lines 57 - 60, The sed replacement may run without changing anything, so after running the sed that replaces ghcr.io/reqcore-inc/reqcore:latest with ghcr.io/reqcore-inc/reqcore:${{ steps.tag.outputs.version }} you must assert the exact pinned tag exists in docker-compose.production.yml (use a command that checks for the precise string ghcr.io/reqcore-inc/reqcore:${{ steps.tag.outputs.version }} and fail the step if not found); do the same exact verification for the second replacement block later in the file (the other sed/grep area) to ensure the substitution actually occurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/docker-publish.yml:
- Around line 104-108: TAGS loop currently uses `${tag%%:*}` which breaks on
registry ports and signs duplicates; instead derive the image reference with
`${tag%:*}` (removes only the final :tag) and deduplicate before signing (e.g.,
collect images into a set via `declare -A seen` or pipe through `sort -u`) so
you iterate unique images and call `cosign sign --yes "${image}@${DIGEST}"`
exactly once per image; update the loop that references TAGS/tag/image/DIGEST
and the cosign sign invocation accordingly.
In @.github/workflows/pr-title-lint.yml:
- Around line 33-45: The PR title lint allows the "revert" type in the "types"
list but .github/release-please-config.json has no matching changelog section;
either remove "revert" from the types allowlist in the workflow (the multiline
"types:" block) or add a corresponding mapping for "revert" in
.github/release-please-config.json so release-please can categorize revert
PRs—update whichever file you choose and keep the symbol "revert" and the
"types" block consistent with release-please config.
- Around line 17-31: The workflow uses pull_request_target which grants
base-repo context unnecessarily; update the trigger in the `on:` block to
`pull_request` (keeping the existing types: [opened, edited, synchronize,
reopened]) so the lint job runs in the PR fork context, and keep the existing
step that uses amannn/action-semantic-pull-request@v6 and GITHUB_TOKEN
unchanged.
In @.github/workflows/release-verification.yml:
- Line 38: The workflow's overall timeout (timeout-minutes) is set to 20, which
exactly equals the worst-case wait loop duration (60 × 20s) and leaves no time
for setup/start/assert steps; increase timeout-minutes (the workflow-level
timeout) to a larger value (e.g., 30) so there is breathing room, or
alternatively reduce the polling iterations/delay in the wait loop (the
polling/wait loop referenced around lines 67-74) so the sum of max polling time
plus setup/start/assert fits under the timeout; update every occurrence of
timeout-minutes and ensure the wait loop/polling configuration and
workflow-level timeout remain consistent.
- Around line 100-106: The "Assert migrations + S3 bucket ready" step currently
does one-time greps for the strings "Database migrations applied successfully"
and 'S3 bucket "reqcore" is ready" which can cause flaky failures; change each
one-time grep into a retry loop with a sensible timeout (e.g., total wait and
sleep intervals) that repeatedly runs docker compose -f
docker-compose.production.yml logs app | grep -q "<message>" until success or
timeout, and on timeout echo the error message and dump the logs before exiting
non-zero; target the step named "Assert migrations + S3 bucket ready" and the
exact messages "Database migrations applied successfully" and 'S3 bucket
"reqcore" is ready' when implementing the retries.
---
Nitpick comments:
In @.github/workflows/release-verification.yml:
- Around line 57-60: The sed replacement may run without changing anything, so
after running the sed that replaces ghcr.io/reqcore-inc/reqcore:latest with
ghcr.io/reqcore-inc/reqcore:${{ steps.tag.outputs.version }} you must assert the
exact pinned tag exists in docker-compose.production.yml (use a command that
checks for the precise string ghcr.io/reqcore-inc/reqcore:${{
steps.tag.outputs.version }} and fail the step if not found); do the same exact
verification for the second replacement block later in the file (the other
sed/grep area) to ensure the substitution actually occurred.
In `@SELF-HOSTING.md`:
- Around line 94-96: Replace the non-deterministic wildcard change "cd
reqcore-*" by programmatically determining the single extracted directory from
the downloaded archive (reqcore.tar.gz) and cd into that exact directory;
specifically, after downloading/extracting the archive (reqcore.tar.gz) use a
reliable method to obtain the top-level folder name created by tar (e.g., list
the archive contents or inspect the extracted entries) and then cd into that
captured directory name instead of using the glob "reqcore-*".
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d69cfa4d-82d9-4f69-b966-2cb7808e1eaf
📒 Files selected for processing (7)
.github/pull_request_template.md.github/workflows/dependabot-automerge.yml.github/workflows/docker-publish.yml.github/workflows/pr-title-lint.yml.github/workflows/release-verification.yml.vscode/tasks.jsonSELF-HOSTING.md
✅ Files skipped from review due to trivial changes (2)
- .github/pull_request_template.md
- .vscode/tasks.json
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
Type of change
Validation
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit
New Features
Documentation