AI Chat improvements - #518
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds chat management (pin/tag/search/rename), parent-chat linking, follow-up suggestions and title generation, chat feedback, assistant/chart sanitisation and anomaly detection, new data tools, SSE follow-up emission, speech-recognition hook, chart export/segmentation, UI strings, and DB migrations for pins/tags/parent_chat_id. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AiController
participant AiService
participant AiChatService
participant DB
Client->>AiController: POST messages / open SSE
AiController->>AiService: stream/relay messages to model
AiService-->>AiController: streaming deltas / tool calls
AiController->>AiChatService: persist/append messages & metadata
AiChatService->>DB: insert/update ai_chat rows
alt Stream finishes without error and assistant has content
AiController->>AiService: generateFollowUps(firstUserMessage, project, abortSignal)
AiService-->>AiController: follow-up suggestions
AiController->>Client: SSE event "followUps" with suggestions
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Blaumaus
marked this pull request as ready for review
April 21, 2026 19:07
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/apps/cloud/src/ai/ai.service.ts (2)
1544-1548:⚠️ Potential issue | 🔴 CriticalScope goal lookups to the current project.
Line 1545 fetches by
goalIdonly. A user who can access project A but knows a goal ID from project B can make the tool reveal that goal’s metadata and apply its value against project A’s data. Includeproject: { id: pid }in the lookup.Proposed adjustment
if (goalId) { - const goal = await this.goalService.findOne({ where: { id: goalId } }) + const goal = await this.goalService.findOne({ + where: { id: goalId, project: { id: pid } }, + }) if (!goal) { return { error: 'Goal not found' } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/ai.service.ts` around lines 1544 - 1548, The lookup currently calls this.goalService.findOne({ where: { id: goalId } }) which can return goals from other projects; change the query used in goalService.findOne to scope by project by adding project: { id: pid } to the where clause (e.g., where: { id: goalId, project: { id: pid } }) so the goal lookup is restricted to the current project's id (pid) before returning or applying its metadata.
1156-1168:⚠️ Potential issue | 🟠 MajorApply filters for errors and CAPTCHA queries.
getDataaccepts filters for all analytics-style datasets, but Lines 1156-1168 routeerrorsandcaptchawithout passingfilters, and those methods don’t add filter conditions. Questions like “errors on /checkout” or “CAPTCHA challenges from Germany” will return unfiltered project-wide data.Also applies to: 1469-1519, 1709-1796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/ai.service.ts` around lines 1156 - 1168, The getData routing for errors and captcha is not forwarding the incoming filters and the target methods (getErrorsData, getCaptchaData) don't apply filter conditions; update the calls in getData to pass the filters argument through to getErrorsData and getCaptchaData, then modify getErrorsData and getCaptchaData to accept a filters parameter and apply those filter conditions to their queries (e.g., narrow by path, country, or other filter keys) so they return filtered results; apply the same change pattern to the other similar routing sites mentioned (the other getData branches that route to errors/captcha) so all three call sites forward filters and the callee methods enforce them.backend/apps/cloud/src/ai/ai.controller.ts (1)
893-913:⚠️ Potential issue | 🟡 MinorReturn
parentChatIdafter branching.The branched chat is created with
parentChatId, but the response omits it, so clients cannot immediately render provenance without refetching the chat.Proposed fix
return { id: branchedChat.id, name: branchedChat.name, messages: branchedChat.messages, + parentChatId: branchedChat.parentChatId, created: branchedChat.created, updated: branchedChat.updated, branched: true, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/ai.controller.ts` around lines 893 - 913, The response after creating a branched chat (via this.aiChatService.create producing branchedChat) omits the parentChatId so clients can't render provenance; update the returned object from the branch handler to include parentChatId: branchedChat.parentChatId (or the original parent id if stored elsewhere) alongside id, name, messages, created, updated, and branched so clients receive the provenance immediately.
🧹 Nitpick comments (2)
backend/migrations/mysql/2026_04_21_ai_chat_parent_chat.sql (1)
1-4: Consider whetherparent_chat_idshould be a real FK toai_chat(id).The column is kept as a plain
varchar(36)with no foreign-key constraint, so deleting a parent chat silently leaves children pointing at a non-existent id.findParentSummaryinai-chat.service.tsalready handlesnull, so you probably want either:
- a FK with
ON DELETE SET NULLto auto-cleanup dangling refs, or- an explicit documented decision to preserve the string id for provenance even after the parent is gone.
If the second is intentional, this is just a note — no change needed. Otherwise adding the constraint here is much cheaper than reconciling orphans later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/migrations/mysql/2026_04_21_ai_chat_parent_chat.sql` around lines 1 - 4, The new parent_chat_id column on table ai_chat is currently a plain varchar(36) and should either be a real foreign key to ai_chat(id) or explicitly documented as intentionally non-FK; to fix this, modify the migration to add a foreign-key constraint on ai_chat(parent_chat_id) referencing ai_chat(id) with ON DELETE SET NULL (ensure parent_chat_id type matches id), or leave the column but add a comment in the migration/DDL and in ai-chat.service.ts near findParentSummary explaining the provenance decision; update the ALTER TABLE to add the CONSTRAINT name (and keep or recreate the idx_ai_chat_parent_chat_id index) so deletes of a parent auto-null children rather than leaving dangling ids.backend/migrations/mysql/2026_04_21_ai_chat_pin_tags.sql (1)
1-5: Composite index on (pinned,updated) matches the query — LGTM.One minor follow-up:
tagsis a TypeORMsimple-arraystored astext, which means tag filtering inlistByProjectwill rely onLIKE '%tag%'and cannot use any index. If tag filtering becomes hot, consider normalising into a join table (ai_chat_tag) later. No change required for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/migrations/mysql/2026_04_21_ai_chat_pin_tags.sql` around lines 1 - 5, The new `tags` text column (used by TypeORM simple-array) cannot be indexed so `listByProject` filtering will use LIKE and be slow if hot; to fix when needed, normalize tags into a join table `ai_chat_tag` (columns: `id`, `ai_chat_id` FK to `ai_chat`, `tag` with an index) and migrate existing `tags` values into that table, then update the `listByProject` query/Repository method to JOIN/WHERE on `ai_chat_tag.tag` (and add an index on `ai_chat_tag.tag`) while keeping the `idx_ai_chat_pinned_updated` composite index for pinned/updated queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/ai/ai-chat.service.ts`:
- Around line 145-158: The tag aggregation currently uses a Set<string> that
treats "Bug" and "bug" as different values; change to dedupe case-insensitively
by using a Map keyed by the lowercase tag and storing a canonical display value
(e.g., first-seen original) so duplicates across chats collapse. In the method
that processes rows (the block creating "set" from "rows" in listTagsByProject),
compute key = part.toLowerCase(), if the key is not present add map.set(key,
part) and skip otherwise; finally return Array.from(map.values()).sort((a,b) =>
a.localeCompare(b, undefined, { sensitivity: 'base' })). This preserves a
preferred casing while ensuring case-insensitive deduplication.
In `@backend/apps/cloud/src/ai/ai.controller.ts`:
- Around line 698-716: The background title job can overwrite a user rename
because generateChatTitle().then(...) calls aiChatService.update(chat.id, {
name: title }) unconditionally; change this to a conditional update by first
loading the latest chat (via aiChatService.findById or equivalent) and only
calling aiChatService.update when the current chat.name is still empty/unchanged
from the original (or implement an atomic update method like
aiChatService.updateIfNameEquals(chat.id, expectedName, { name: title })). Use
aiService.generateChatTitle, aiChatService.update/findById (or add
updateIfNameEquals) and the original chat.id/createChatDto.name to perform the
check so the background job does not clobber a user-provided name.
- Around line 671-680: The controller currently ignores an invalid
createChatDto.parentChatId and proceeds with parentChatId=null; change this so
when createChatDto.parentChatId is provided but
this.aiChatService.findParentSummary(createChatDto.parentChatId, pid) returns
falsy, the handler throws an HTTP error (e.g., throw new
BadRequestException('Invalid parentChatId') or NotFoundException) instead of
continuing; locate the block using parentChatId, createChatDto.parentChatId, and
this.aiChatService.findParentSummary to implement the guard and error throw so
clients cannot silently create unbranched chats for bad IDs.
- Around line 407-416: The follow-up generator is still running after
Promise.race times out; create an AbortController in the caller and use it to
cancel the slow task: wrap the aiService.generateFollowUps call with an
AbortController, pass its signal to generateFollowUps (and ensure
generateFollowUps forwards the abortSignal into its generateText invocation),
and on the timeout branch call controller.abort() so the OpenRouter request is
cancelled instead of continuing to consume quota (refer to
FOLLOW_UPS_TIMEOUT_MS, generateFollowUps, aiService.generateFollowUps,
generateText, AbortController/abortSignal).
In `@backend/apps/cloud/src/ai/dto/chat.dto.ts`:
- Around line 34-38: The tool call payload allows unbounded nested JSON via the
args?: unknown property (and the similar field around lines 79-90), so add
validation and/or redaction: replace args?: unknown with a constrained DTO or
validator (e.g., use IsOptional + IsObject({ nullable: true }) + a maximum
size/shape check or a custom validator that enforces allowed keys/depth/length),
or store only a redacted/trimmed string summary of args (e.g., serialize and
truncate) before persisting; update the DTO(s) referencing args (the
toolCalls.args property and the similar field at 79-90) and add unit tests to
ensure overly large or deeply nested payloads are rejected or truncated.
In `@backend/apps/cloud/src/ai/entity/ai-chat.entity.ts`:
- Around line 59-61: The parentChatId column on the AiChat entity is a plain
string so deleting a parent AiChat can orphan children; change the model to
either add a TypeORM self-relation (e.g., add a `@ManyToOne`(() => AiChat, a =>
a.children, { nullable: true, onDelete: 'SET NULL' }) parent: AiChat | null with
a corresponding `@OneToMany` children property) or update the migration
(2026_04_21_ai_chat_parent_chat.sql) to add a foreign key on parent_chat_id
referencing ai_chat(id) with ON DELETE SET NULL so findParentSummary and the UI
won't lose context when parents are removed.
- Around line 45-47: sanitiseTags() currently returns [] which TypeORM
serializes to an empty string and later deserializes to ['']; change
sanitiseTags() so that after trimming/filtering it returns null when the
resulting array is empty (i.e. if sanitizedTags.length === 0 return null),
leaving non-empty arrays unchanged; ensure callers (e.g. updateMeta() and any
references to chat.tags) continue to handle string[] | null and that the entity
property tags remains nullable so empty tag lists persist as null in the DB.
In `@web/app/hooks/useSpeechRecognition.ts`:
- Around line 52-53: The useState initializer for isSupported uses
getSpeechRecognition() which runs on the server and freezes isSupported as false
after hydration; change the initial state to false and in useEffect call
getSpeechRecognition() and setIsSupported(true) if available. Update the hook
function useSpeechRecognition to initialize isSupported with useState(false) and
add a client-only effect (useEffect) that checks getSpeechRecognition() and
calls setIsSupported accordingly so the mic button appears when supported.
In `@web/app/pages/Project/tabs/AskAI/AIChart.tsx`:
- Around line 540-569: The component AIChart initializes displayType from
chart.chartType once, causing stale state when a new chart prop arrives; add a
useEffect that watches chart.chartType (and possibly chart.data shape) and calls
setDisplayType(chart.chartType) to reset displayType whenever the incoming chart
type changes so subsequent compatibility checks (used by compatibleTypes and the
logic around the existing displayType checks) won't produce a blank chart.
In `@web/app/pages/Project/tabs/AskAI/contentSegments.ts`:
- Around line 60-70: The catch currently pushes a { kind: 'chart', chart: null,
pending: true } even when a balanced JSON slice (jsonString from
content.substring(startIndex, endIndex + 1)) was parsed but invalid; change the
logic in the try/catch around JSON.parse so that on parse failure you check
endIndex: if endIndex === -1 keep the pending chart placeholder (pending true,
chart null) but otherwise treat the slice as plain text and push { kind: 'text',
text: jsonString } instead of a permanent pending chart; update the push
locations that reference jsonString, JSON.parse, and segments to implement this
behavior.
In `@web/app/pages/Project/tabs/AskAI/exportHelpers.ts`:
- Around line 65-68: When exporting chart JSON in the block that builds the
Markdown (the segment.chart branch that calls getChartLabel and pushes to out
with out.push), compute the maximum run of consecutive backticks in the JSON
string and generate a code fence that is one backtick longer than that run; then
use that dynamic fence instead of a fixed "```" so the closing fence cannot be
prematurely terminated by content in JSON. Implement this by creating json =
JSON.stringify(segment.chart, null, 2), scanning json for the longest sequence
of '`' characters, building fence = '`'.repeat(maxRun + 1), and using
`${fence}json\n${json}\n${fence}` in the out.push call alongside the existing
label via getChartLabel.
In `@web/app/routes/projects`.$id.tsx:
- Around line 1266-1271: The current catch for JSON.parse of tagsRaw silently
sets body.tags = [], which can wipe tags on malformed input; instead, when
tagsRaw is provided but invalid, either return a 400 Bad Request (so the caller
fixes the payload) or do not mutate body.tags (omit the property) so no
destructive update occurs. Update the try/catch around JSON.parse(tagsRaw) to,
on error, throw or return a validation response indicating malformed tagsRaw (or
simply leave body.tags undefined) rather than assigning an empty array;
reference the tagsRaw variable and the body.tags assignment in the handler that
processes the request to locate and change the behavior.
---
Outside diff comments:
In `@backend/apps/cloud/src/ai/ai.controller.ts`:
- Around line 893-913: The response after creating a branched chat (via
this.aiChatService.create producing branchedChat) omits the parentChatId so
clients can't render provenance; update the returned object from the branch
handler to include parentChatId: branchedChat.parentChatId (or the original
parent id if stored elsewhere) alongside id, name, messages, created, updated,
and branched so clients receive the provenance immediately.
In `@backend/apps/cloud/src/ai/ai.service.ts`:
- Around line 1544-1548: The lookup currently calls this.goalService.findOne({
where: { id: goalId } }) which can return goals from other projects; change the
query used in goalService.findOne to scope by project by adding project: { id:
pid } to the where clause (e.g., where: { id: goalId, project: { id: pid } }) so
the goal lookup is restricted to the current project's id (pid) before returning
or applying its metadata.
- Around line 1156-1168: The getData routing for errors and captcha is not
forwarding the incoming filters and the target methods (getErrorsData,
getCaptchaData) don't apply filter conditions; update the calls in getData to
pass the filters argument through to getErrorsData and getCaptchaData, then
modify getErrorsData and getCaptchaData to accept a filters parameter and apply
those filter conditions to their queries (e.g., narrow by path, country, or
other filter keys) so they return filtered results; apply the same change
pattern to the other similar routing sites mentioned (the other getData branches
that route to errors/captcha) so all three call sites forward filters and the
callee methods enforce them.
---
Nitpick comments:
In `@backend/migrations/mysql/2026_04_21_ai_chat_parent_chat.sql`:
- Around line 1-4: The new parent_chat_id column on table ai_chat is currently a
plain varchar(36) and should either be a real foreign key to ai_chat(id) or
explicitly documented as intentionally non-FK; to fix this, modify the migration
to add a foreign-key constraint on ai_chat(parent_chat_id) referencing
ai_chat(id) with ON DELETE SET NULL (ensure parent_chat_id type matches id), or
leave the column but add a comment in the migration/DDL and in
ai-chat.service.ts near findParentSummary explaining the provenance decision;
update the ALTER TABLE to add the CONSTRAINT name (and keep or recreate the
idx_ai_chat_parent_chat_id index) so deletes of a parent auto-null children
rather than leaving dangling ids.
In `@backend/migrations/mysql/2026_04_21_ai_chat_pin_tags.sql`:
- Around line 1-5: The new `tags` text column (used by TypeORM simple-array)
cannot be indexed so `listByProject` filtering will use LIKE and be slow if hot;
to fix when needed, normalize tags into a join table `ai_chat_tag` (columns:
`id`, `ai_chat_id` FK to `ai_chat`, `tag` with an index) and migrate existing
`tags` values into that table, then update the `listByProject` query/Repository
method to JOIN/WHERE on `ai_chat_tag.tag` (and add an index on
`ai_chat_tag.tag`) while keeping the `idx_ai_chat_pinned_updated` composite
index for pinned/updated queries.
🪄 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: 08d5915c-f870-4a1d-a572-567982cc8a94
⛔ Files ignored due to path filters (3)
docs/public/img/analytics-dashboard/ask-ai-chart-toolbar.pngis excluded by!**/*.pngdocs/public/img/analytics-dashboard/ask-ai-history.pngis excluded by!**/*.pngdocs/public/img/analytics-dashboard/ask-ai.pngis excluded by!**/*.png
📒 Files selected for processing (20)
backend/apps/cloud/src/ai/ai-chat.service.tsbackend/apps/cloud/src/ai/ai.controller.tsbackend/apps/cloud/src/ai/ai.module.tsbackend/apps/cloud/src/ai/ai.service.tsbackend/apps/cloud/src/ai/dto/chat.dto.tsbackend/apps/cloud/src/ai/entity/ai-chat.entity.tsbackend/migrations/mysql/2026_04_21_ai_chat_parent_chat.sqlbackend/migrations/mysql/2026_04_21_ai_chat_pin_tags.sqldocs/content/docs/analytics-dashboard/ask-ai.mdxweb/app/api/index.tsweb/app/hooks/useSpeechRecognition.tsweb/app/pages/Dashboard/Dashboard.tsxweb/app/pages/Project/tabs/AskAI/AIChart.tsxweb/app/pages/Project/tabs/AskAI/AskAIView.tsxweb/app/pages/Project/tabs/AskAI/contentSegments.tsweb/app/pages/Project/tabs/AskAI/exportHelpers.tsweb/app/pages/Project/tabs/AskAI/toolFormatters.tsweb/app/routes/projects.$id.tsxweb/app/styles/ProjectViewStyle.cssweb/public/locales/en.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/apps/cloud/src/ai/ai.controller.ts (2)
495-511:isLimitModeback-compat is hard to read; consider documenting or dropping it.The five-condition gate to decide whether
query.limitwins overquery.takeis easy to get wrong when the DTO evolves (e.g. adding another filter silently flips clients that still sendlimit). Either:
- drop
limitif no live clients still use it and lettakebe the single knob, or- extract a small helper (
resolvePageSize(query)) and add a comment pointing to the deprecation plan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/ai.controller.ts` around lines 495 - 511, The compound isLimitMode logic that decides when query.limit overrides query.take is fragile—extract it into a small helper like resolvePageSize(query) that returns the numeric take and encapsulates the five-condition gate, then call that helper where take is computed before invoking aiChatService.listByProject; also add a brief comment in resolvePageSize pointing to the deprecation plan for query.limit (or remove support entirely if no clients remain) so future DTO changes won't silently flip behavior.
407-440: Optional: also abort follow-up generation when the client disconnects.The 5 s timeout aborts the OpenRouter call, but if the client closes the SSE connection during follow-up generation we keep awaiting until timeout (up to 5 s of wasted quota/CPU per dropped request). Since you already track
clientClosedviares.on('close', …), you can hook it into the sameAbortController.♻️ Proposed tweak
- let clientClosed = false - res.on('close', () => { - clientClosed = true - }) + let clientClosed = false + let onClientClose: (() => void) | null = null + res.on('close', () => { + clientClosed = true + onClientClose?.() + }) ... - if (!clientClosed && !streamErrored && assistantText.trim().length > 0) { + if (!clientClosed && !streamErrored && assistantText.trim().length > 0) { const FOLLOW_UPS_TIMEOUT_MS = 5_000 const controller = new AbortController() + onClientClose = () => controller.abort() const timeoutHandle = setTimeout( () => controller.abort(), FOLLOW_UPS_TIMEOUT_MS, ) ... } finally { clearTimeout(timeoutHandle) + onClientClose = null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/ai.controller.ts` around lines 407 - 440, The follow-up generation uses an AbortController ("controller") with a timeout but doesn't abort when the SSE client disconnects ("clientClosed"), so generateFollowUps may continue consuming resources; hook the SSE close handler (the res.on('close' ...) that sets clientClosed) to also call controller.abort() (or register a listener that aborts controller.signal) before awaiting this.aiService.generateFollowUps, ensuring the AbortController created in this block is aborted immediately when the client disconnects and that any cleanup (e.g., removing the listener) occurs in the finally block alongside clearTimeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/cloud/src/ai/ai-chat.service.ts`:
- Around line 87-111: The search path currently interpolates raw user input into
term = `%${search.trim()}%` and performs CAST(chat.messages AS CHAR) LIKE :term
which both fails to escape SQL LIKE metacharacters and inefficiently scans JSON;
update the code around baseQuery(), applyTagAndPinned(...) and
orderAndPaginate(...) so you first escape '%' and '_' in search.trim() (and
backslashes) before building term, use a parameterized LIKE with an explicit
ESCAPE '\\' clause (e.g. "LIKE :term ESCAPE '\\\\'") to prevent wildcard
injection, and remove or replace the CAST(chat.messages AS CHAR) LIKE fallback
by either matching only message text (via a new search_text column maintained on
save or using JSON_SEARCH/JSON_EXTRACT to target message content) so queries can
be indexed and avoid full JSON scans; ensure changes reference the term
creation, the contentQb where CAST(chat.messages AS CHAR) LIKE :term is used,
and keep applyTagAndPinned(baseQuery()) usage intact.
In `@backend/apps/cloud/src/ai/ai.controller.ts`:
- Around line 800-844: The endpoint submitChatFeedback currently only logs
analytics via trackCustom and doesn't persist ratings/comments nor enforce
stricter access; add persistence and tighten access: implement a feedback
persistence path (e.g., create an AiChatFeedback entity/table and repository or
add aiChatService.recordFeedback(chatId, feedbackDto, uid) that saves {chatId,
messageIndex, rating, comment, userId, timestamp}) and call it from
submitChatFeedback after verifyProjectAccess; additionally decide enforcement
for access by either changing the route auth to require authentication or adding
an owner check (use uid and aiChatService.verifyProjectAccess/
projectService.allowedToView or a new aiChatService.isChatOwner(chatId, uid))
and reject unauthenticated/non-owner submissions accordingly.
---
Nitpick comments:
In `@backend/apps/cloud/src/ai/ai.controller.ts`:
- Around line 495-511: The compound isLimitMode logic that decides when
query.limit overrides query.take is fragile—extract it into a small helper like
resolvePageSize(query) that returns the numeric take and encapsulates the
five-condition gate, then call that helper where take is computed before
invoking aiChatService.listByProject; also add a brief comment in
resolvePageSize pointing to the deprecation plan for query.limit (or remove
support entirely if no clients remain) so future DTO changes won't silently flip
behavior.
- Around line 407-440: The follow-up generation uses an AbortController
("controller") with a timeout but doesn't abort when the SSE client disconnects
("clientClosed"), so generateFollowUps may continue consuming resources; hook
the SSE close handler (the res.on('close' ...) that sets clientClosed) to also
call controller.abort() (or register a listener that aborts controller.signal)
before awaiting this.aiService.generateFollowUps, ensuring the AbortController
created in this block is aborted immediately when the client disconnects and
that any cleanup (e.g., removing the listener) occurs in the finally block
alongside clearTimeout.
🪄 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: f5453ebc-0b5d-4c66-840c-02a65f244238
📒 Files selected for processing (10)
backend/apps/cloud/src/ai/ai-chat.service.tsbackend/apps/cloud/src/ai/ai.controller.tsbackend/apps/cloud/src/ai/ai.service.tsbackend/apps/cloud/src/ai/dto/chat.dto.tsbackend/apps/cloud/src/ai/entity/ai-chat.entity.tsweb/app/hooks/useSpeechRecognition.tsweb/app/pages/Project/tabs/AskAI/AIChart.tsxweb/app/pages/Project/tabs/AskAI/contentSegments.tsweb/app/pages/Project/tabs/AskAI/exportHelpers.tsweb/app/routes/projects.$id.tsx
✅ Files skipped from review due to trivial changes (1)
- web/app/hooks/useSpeechRecognition.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- web/app/pages/Project/tabs/AskAI/contentSegments.ts
- web/app/pages/Project/tabs/AskAI/AIChart.tsx
- web/app/pages/Project/tabs/AskAI/exportHelpers.ts
- web/app/routes/projects.$id.tsx
- backend/apps/cloud/src/ai/ai.service.ts
- backend/apps/cloud/src/ai/dto/chat.dto.ts
There was a problem hiding this comment.
🧹 Nitpick comments (5)
backend/apps/cloud/src/ai/dto/chat.dto.ts (4)
283-291: Emptysearchstring will 400 instead of being ignored.
@IsOptional()in class-validator only short-circuits onnull/undefined, so?search=(empty) reaches@MinLength(2)and fails validation. The frontend inweb/app/routes/projects.$id.tsxalready guards withlength >= 2, but any other caller sending an empty string will get a 400. Consider coercing empty-string toundefinedvia@Transform(similar toparseOptionalBool) for consistency.♻️ Suggested change
`@IsOptional`() + `@Transform`(({ value }) => (typeof value === 'string' && value.trim() === '' ? undefined : value)) `@IsString`() `@MinLength`(2) `@MaxLength`(100) search?: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/dto/chat.dto.ts` around lines 283 - 291, The search property currently uses `@IsOptional`() but an empty string still triggers `@MinLength`(2); update the DTO to coerce empty-string to undefined by adding a `@Transform` that returns undefined for '' (similar to parseOptionalBool usage), import Transform from class-transformer, and keep the existing `@IsOptional`(), `@IsString`(), `@MinLength`(2), `@MaxLength`(100) decorators so that ?search= is treated as omitted rather than causing a 400.
362-369: Optional: capmessageIndexupper bound.
messageIndexis>= 0but unbounded at the top, so a caller can send e.g.Number.MAX_SAFE_INTEGER. Assuming the controller dereferenceschat.messages[messageIndex]for context, an out-of-range index is harmless, but adding@Max(MAX_MESSAGES_PER_CHAT - 1)(or similar) mirrors the persisted message cap and fails fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/dto/chat.dto.ts` around lines 362 - 369, The messageIndex DTO validator currently only enforces a minimum but not an upper bound; add a Max constraint to the messageIndex property (e.g., `@Max`(MAX_MESSAGES_PER_CHAT - 1)) so callers cannot submit excessively large indexes. Update chat.dto.ts to import and reference the shared MAX_MESSAGES_PER_CHAT constant (or the appropriate config constant) and add `@Max`(...) above the messageIndex field alongside `@Min`(0) and `@IsInt`() so validation fails fast when an index exceeds the persisted message cap.
239-246:parseOptionalBoolsilently drops invalid values.For inputs like
pinned=yesororderByPinned=1a,parseOptionalBoolreturnsundefined, which@IsOptional()then treats as "not supplied" — the filter is silently ignored instead of returning a 400. If callers rely onpinned=foofailing loudly, consider throwing or returning the raw value so@IsBoolean()rejects it. Not blocking if permissive parsing is intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/dto/chat.dto.ts` around lines 239 - 246, parseOptionalBool currently returns undefined for invalid non-empty inputs (e.g., "yes", "1a"), which makes `@IsOptional` treat them as absent; change parseOptionalBool so it does not silently drop invalid non-empty values — instead, after handling valid true/false cases, return the original input (or throw a clear Error) so class-validator's `@IsBoolean/`@IsOptional flow will produce a 400; update the function parseOptionalBool to return the raw value for invalid strings (or explicitly throw) rather than undefined.
205-213: Consider validatingparentChatIdas a UUID.
MaxLength(36)constrains length but accepts any string format, whilefindParentSummaryperforms a database lookup. Using@IsUUID()would enforce UUID format validation at the DTO layer, returning a cleaner 400 validation error instead of a downstream "not found" error.♻️ Suggested change
`@IsOptional`() - `@IsString`() - `@MaxLength`(36) + `@IsUUID`() parentChatId?: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/dto/chat.dto.ts` around lines 205 - 213, The parentChatId DTO property currently uses `@MaxLength`(36) which allows non-UUID strings; replace the length check with a UUID validator by removing `@MaxLength`(36) and adding `@IsUUID`() (keep `@IsOptional`()) on the parentChatId field in chat.dto.ts and update imports to include IsUUID from class-validator; this ensures parentChatId is validated as a UUID before reaching findParentSummary.web/app/routes/projects.$id.tsx (1)
1353-1385: Rating cast bypasses client-side validation.
formData.get('rating')?.toString() as 'good' | 'bad'lies to the type system — ifratingis missing or anything other than'good'/'bad', this still compiles as a valid union. The backendFeedbackDtowill reject it (so no correctness bug), but you will round-trip a bad request to the server instead of failing fast. Consider validating before issuing the fetch:♻️ Suggested change
- const chatId = formData.get('chatId')?.toString() - const rating = formData.get('rating')?.toString() as 'good' | 'bad' + const chatId = formData.get('chatId')?.toString() + const ratingRaw = formData.get('rating')?.toString() + if (ratingRaw !== 'good' && ratingRaw !== 'bad') { + return data<ProjectViewActionData>( + { intent, error: 'Invalid rating' }, + { status: 400 }, + ) + } + const rating = ratingRaw const messageIndex = formData.get('messageIndex')?.toString() const comment = formData.get('comment')?.toString() const body: Record<string, unknown> = { rating }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/app/routes/projects`.$id.tsx around lines 1353 - 1385, The code in the 'submit-ai-chat-feedback' action silently casts rating with formData.get(... ) as 'good' | 'bad', which can send invalid values to serverFetch; validate rating locally first by checking the extracted rating string strictly equals 'good' or 'bad' (use the local variable rating or rawRating) and if not return a data<ProjectViewActionData> error response (400) instead of issuing serverFetch; only build the body and call serverFetch when the rating passes this explicit check so we fail fast and avoid round-tripping invalid input to the FeedbackDto validation on the backend.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/apps/cloud/src/ai/dto/chat.dto.ts`:
- Around line 283-291: The search property currently uses `@IsOptional`() but an
empty string still triggers `@MinLength`(2); update the DTO to coerce empty-string
to undefined by adding a `@Transform` that returns undefined for '' (similar to
parseOptionalBool usage), import Transform from class-transformer, and keep the
existing `@IsOptional`(), `@IsString`(), `@MinLength`(2), `@MaxLength`(100) decorators
so that ?search= is treated as omitted rather than causing a 400.
- Around line 362-369: The messageIndex DTO validator currently only enforces a
minimum but not an upper bound; add a Max constraint to the messageIndex
property (e.g., `@Max`(MAX_MESSAGES_PER_CHAT - 1)) so callers cannot submit
excessively large indexes. Update chat.dto.ts to import and reference the shared
MAX_MESSAGES_PER_CHAT constant (or the appropriate config constant) and add
`@Max`(...) above the messageIndex field alongside `@Min`(0) and `@IsInt`() so
validation fails fast when an index exceeds the persisted message cap.
- Around line 239-246: parseOptionalBool currently returns undefined for invalid
non-empty inputs (e.g., "yes", "1a"), which makes `@IsOptional` treat them as
absent; change parseOptionalBool so it does not silently drop invalid non-empty
values — instead, after handling valid true/false cases, return the original
input (or throw a clear Error) so class-validator's `@IsBoolean/`@IsOptional flow
will produce a 400; update the function parseOptionalBool to return the raw
value for invalid strings (or explicitly throw) rather than undefined.
- Around line 205-213: The parentChatId DTO property currently uses
`@MaxLength`(36) which allows non-UUID strings; replace the length check with a
UUID validator by removing `@MaxLength`(36) and adding `@IsUUID`() (keep
`@IsOptional`()) on the parentChatId field in chat.dto.ts and update imports to
include IsUUID from class-validator; this ensures parentChatId is validated as a
UUID before reaching findParentSummary.
In `@web/app/routes/projects`.$id.tsx:
- Around line 1353-1385: The code in the 'submit-ai-chat-feedback' action
silently casts rating with formData.get(... ) as 'good' | 'bad', which can send
invalid values to serverFetch; validate rating locally first by checking the
extracted rating string strictly equals 'good' or 'bad' (use the local variable
rating or rawRating) and if not return a data<ProjectViewActionData> error
response (400) instead of issuing serverFetch; only build the body and call
serverFetch when the rating passes this explicit check so we fail fast and avoid
round-tripping invalid input to the FeedbackDto validation on the backend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bc2c20b-76c0-44f4-88c6-19c181945402
📒 Files selected for processing (5)
backend/apps/cloud/src/ai/ai-chat.service.tsbackend/apps/cloud/src/ai/ai.controller.tsbackend/apps/cloud/src/ai/dto/chat.dto.tsweb/app/pages/Project/tabs/AskAI/AskAIView.tsxweb/app/routes/projects.$id.tsx
✅ Files skipped from review due to trivial changes (1)
- backend/apps/cloud/src/ai/ai-chat.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/cloud/src/ai/ai.controller.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/apps/cloud/src/ai/ai.controller.ts (1)
640-642: Optional: fold parent lookup into the main chat query.
verifyProjectAccessalready fetches the chat; this adds a second round-trip perGET /:pid/chats/:chatIdjust to resolve{ id, name }of the parent. A single self-join (or adding parent as a@ManyToOne(() => AiChat)relation with aselect: ['id', 'name']load option) would halve the query count on a hot read path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/cloud/src/ai/ai.controller.ts` around lines 640 - 642, The current code does a second DB round-trip by calling aiChatService.findParentSummary(chat.parentChatId, pid) after verifyProjectAccess has already loaded the chat; refactor to include the parent summary in the initial chat query instead: add a self-relation on the AiChat entity (e.g. `@ManyToOne`(() => AiChat, { nullable: true, select: ['id','name'] }) parentChat) or modify the repository/query used by verifyProjectAccess (or the underlying AiChatRepository method) to perform a left join/self-join and select parent id and name, then return that parent summary as parentChat so ai.controller.ts no longer calls aiChatService.findParentSummary and the GET /:pid/chats/:chatId handler uses the already-loaded parentChat field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/apps/cloud/src/ai/ai.controller.ts`:
- Around line 640-642: The current code does a second DB round-trip by calling
aiChatService.findParentSummary(chat.parentChatId, pid) after
verifyProjectAccess has already loaded the chat; refactor to include the parent
summary in the initial chat query instead: add a self-relation on the AiChat
entity (e.g. `@ManyToOne`(() => AiChat, { nullable: true, select: ['id','name'] })
parentChat) or modify the repository/query used by verifyProjectAccess (or the
underlying AiChatRepository method) to perform a left join/self-join and select
parent id and name, then return that parent summary as parentChat so
ai.controller.ts no longer calls aiChatService.findParentSummary and the GET
/:pid/chats/:chatId handler uses the already-loaded parentChat field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 547f392f-f3f0-4413-8cd2-4bfe4b630b73
📒 Files selected for processing (2)
backend/apps/cloud/src/ai/ai-chat.service.tsbackend/apps/cloud/src/ai/ai.controller.ts
This was referenced Apr 26, 2026
6 tasks
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.
Changes
If applicable, please describe what changes were made in this pull request.
Community Edition support
Database migrations
Documentation
Summary by CodeRabbit
New Features
Documentation