Improve chat tools and metadata#896
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR normalizes chat tag/thread data, expands chat playground API-target and server-tool handling, adds queued prompts and bulk thread actions, updates message metadata rendering, and forwards custom base URLs through chat proxy routes while attaching timing metadata to terminal stream frames. ChangesChat data normalization and tool config
Chat playground shell and multi-thread actions
Conversation flow and composer controls
Gateway proxy and stream timing
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ChatPlayground
participant ChatHeader
participant ChatSidebar
participant ChatDeleteDialog
participant ChatTagsDialog
ChatPlayground->>ChatPlayground: infer apiTarget / resolve baseUrl
ChatPlayground->>ChatHeader: pass apiTarget and onApiTargetChange
ChatPlayground->>ChatSidebar: pass bulk action callbacks
ChatSidebar->>ChatPlayground: request delete/edit selected threads
ChatPlayground->>ChatDeleteDialog: open with count
ChatPlayground->>ChatTagsDialog: open with threads selection
sequenceDiagram
participant ChatConversation
participant ChatConversationComposer
participant onSend
ChatConversation->>ChatConversationComposer: pass queuedPrompts and callbacks
ChatConversationComposer->>ChatConversation: submit prompt or queue edit/reorder
ChatConversation->>ChatConversation: drain queued prompt for active thread
ChatConversation->>onSend: sendPrompt(payload)
sequenceDiagram
participant ChatRoute
participant gatewayProxy as gatewayProxy.ts
participant Gateway
ChatRoute->>gatewayProxy: proxyGatewayPost/Get(baseUrl)
gatewayProxy->>gatewayProxy: resolveGatewayBaseUrl(requestedBaseUrl)
gatewayProxy->>Gateway: proxy request to selected upstream
Gateway-->>gatewayProxy: response / stream
gatewayProxy-->>ChatRoute: proxied result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/lib/indexeddb/chats.ts (1)
131-158: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSolid sanitization logic; consider validating
colorformat.The trim/dedup/default-color logic matches the documented test contract exactly. One optional improvement:
coloris only checked for non-emptiness, not for a valid CSS color shape (e.g., hex). Malformed imported values (e.g.,"javascript:x", overly long strings) would pass through unchanged and could produce visually broken tag swatches downstream.♻️ Optional: constrain color to a hex pattern
+const HEX_COLOR_PATTERN = /^#(?:[0-9a-f]{3}|[0-9a-f]{6})$/i; + export function normalizeChatTags(value: unknown): ChatTag[] { ... - const color = - typeof candidate.color === "string" && candidate.color.trim() - ? candidate.color.trim() - : DEFAULT_CHAT_TAG_COLOR; + const trimmedColor = + typeof candidate.color === "string" ? candidate.color.trim() : ""; + const color = HEX_COLOR_PATTERN.test(trimmedColor) + ? trimmedColor + : DEFAULT_CHAT_TAG_COLOR;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/indexeddb/chats.ts` around lines 131 - 158, normalizeChatTags currently accepts any non-empty string for ChatTag.color, so malformed imported values can slip through. Update normalizeChatTags in chats.ts to validate the color field more strictly before constructing each ChatTag, using the existing DEFAULT_CHAT_TAG_COLOR fallback when the value does not match the intended CSS/hex format. Keep the current id/name trimming and dedup behavior intact while tightening the color handling inside the normalizeChatTags loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/src/lib/indexeddb/chats.ts`:
- Around line 131-158: normalizeChatTags currently accepts any non-empty string
for ChatTag.color, so malformed imported values can slip through. Update
normalizeChatTags in chats.ts to validate the color field more strictly before
constructing each ChatTag, using the existing DEFAULT_CHAT_TAG_COLOR fallback
when the value does not match the intended CSS/hex format. Keep the current
id/name trimming and dedup behavior intact while tightening the color handling
inside the normalizeChatTags loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27aaa2a8-f704-492b-a4bd-3917ae948075
📒 Files selected for processing (3)
apps/web/src/components/(chat)/ChatPlayground.tsxapps/web/src/lib/indexeddb/chats.test.tsapps/web/src/lib/indexeddb/chats.ts
DanielButler1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/web/src/app/api/chat/_shared/gatewayProxy.ts (1)
32-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail-open fallback makes the missing-config branch unreachable
resolveGatewayBaseUrl()always returns either the requested URL,configuredGatewayBaseUrl, orPUBLIC_GATEWAY_BASE_URL, soproxyGatewayPost()/proxyGatewayGet()can no longer hitbuildGatewayMissingConfigResponse(). If misconfigured deployments should still fail closed, restore thenullpath; otherwise remove the dead guard and document the fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/chat/_shared/gatewayProxy.ts` around lines 32 - 36, `resolveGatewayBaseUrl()` in `gatewayProxy.ts` currently falls back to `PUBLIC_GATEWAY_BASE_URL`, which makes the missing-config branch in `proxyGatewayPost()` and `proxyGatewayGet()` unreachable. Decide whether misconfigured deployments should fail closed or fail open: if fail closed, restore a null/absent path so `buildGatewayMissingConfigResponse()` can still be returned; if fail open, remove the dead guard and update the gateway base URL logic/documentation to reflect the fallback.apps/web/src/app/api/chat/video/route.ts (1)
86-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
baseUrlthrough video GET proxying
apps/web/src/app/api/chat/video/route.ts: both GET branches drop the override, so a video job created against a custom gateway can’t be listed or polled from this route. PassbaseUrlintoproxyGatewayGethere too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/chat/video/route.ts` around lines 86 - 119, The GET handler in video/route.ts is dropping the custom gateway override when proxying both the list and poll branches. Update the GET logic to forward baseUrl into proxyGatewayGet alongside the existing path from resolveVideoListPath and resolveVideoPollPath, so requests created against a custom gateway continue to resolve correctly.apps/web/src/app/api/chat/audio/route.ts (1)
50-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread
baseUrlthrough music pollingapps/web/src/app/api/chat/audio/route.ts:50-82Polling here ignores the gateway override used byPOST, so music jobs created on a custom/dev gateway will be polled against the default backend and can miss or 404. PassbaseUrlin the poll URL and forward it toproxyGatewayGet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/chat/audio/route.ts` around lines 50 - 82, The music polling GET handler currently ignores the gateway override used by POST, so polling can hit the wrong backend. Update the GET path in audio route’s GET handler to read the same baseUrl query param and thread it into both the polling URL built by resolveMusicPollPath and the proxyGatewayGet call. Keep the existing resourceId/action validation in place, but ensure the resolved poll request uses the caller-provided baseUrl when present.apps/web/src/components/(chat)/playground/chat-playground-core.ts (1)
467-507: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClamp advisor temperature too
buildAdvisorDefinitionforwardsadvisor.temperatureunchanged, while the sibling subagent path clamps it to[0, 2]. Normalizing here avoids inconsistent tool payloads and rejected requests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/`(chat)/playground/chat-playground-core.ts around lines 467 - 507, The buildAdvisorDefinition helper currently forwards advisor.temperature without normalization, unlike the sibling subagent path. Update buildAdvisorDefinition to clamp the temperature value to the accepted [0, 2] range before assigning it to parameters.temperature, and keep the existing checks in place for advisor, advisor.temperature, and the withParameters("ai-stats:advisor", ...) call so the advisor payload stays consistent.
🧹 Nitpick comments (4)
apps/web/src/components/(chat)/ChatConversationComposer.tsx (1)
517-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate helper — extract
getReadableTextColorto a shared module.This function is identical to
getReadableTextColorinapps/web/src/components/(chat)/ChatConversationMessages.tsx(lines 517-532). Extract it into a shared, named export and import it in both files to avoid divergence.As per coding guidelines: "Prefer named exports for shared utilities in TypeScript".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/`(chat)/ChatConversationComposer.tsx around lines 517 - 532, The `getReadableTextColor` helper in `ChatConversationComposer` is duplicated in `ChatConversationMessages`, so extract it into a shared utility module as a named export and update both components to import and use that shared function. Keep the implementation in one place only, and remove the local duplicate so the `getReadableTextColor` behavior stays consistent across both chat components.Source: Coding guidelines
apps/web/src/app/api/chat/_shared/gatewayProxy.ts (1)
223-249: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSecurity-sensitive allow-list logic lacks dedicated tests.
resolveGatewayBaseUrl/isDevelopmentLocalGatewayBaseUrlgate which upstream host a client-suppliedbaseUrlcan reach (SSRF-relevant). Given the PR's stated validation only coverschats.test.ts, this new allow-list has no unit coverage for the match/no-match/dev-local branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/chat/_shared/gatewayProxy.ts` around lines 223 - 249, Add dedicated unit tests for the allow-list logic in isDevelopmentLocalGatewayBaseUrl and resolveGatewayBaseUrl, since these functions control which client-supplied baseUrl is accepted. Cover the exact match branch, the configuredGatewayBaseUrl and PUBLIC_GATEWAY_BASE_URL fallback behavior, and the development-local acceptance/rejection branches for localhost/127.0.0.1, protocol, port, and pathname. Place the tests near the gatewayProxy coverage so the new SSRF-sensitive rules are exercised independently of chats.test.ts.apps/web/src/lib/indexeddb/chats.ts (1)
50-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared
reasoningEffortunion to avoid triple duplication.The literal union
"none" | "minimal" | "low" | "medium" | "high" | "xhigh" | "max"is now repeated verbatim inChatAdvisorServerToolConfig,ChatSubagentServerToolConfig, andChatModelSettings. Any future addition/removal of an effort level requires updating three places in lockstep, which is easy to miss.♻️ Suggested consolidation
+export type ChatReasoningEffort = + | "none" + | "minimal" + | "low" + | "medium" + | "high" + | "xhigh" + | "max"; + export type ChatAdvisorServerToolConfig = { ... - reasoningEffort?: "none" | "minimal" | "low" | "medium" | "high" | "xhigh" | "max"; + reasoningEffort?: ChatReasoningEffort; }; export type ChatSubagentServerToolConfig = { ... - reasoningEffort?: "none" | "minimal" | "low" | "medium" | "high" | "xhigh" | "max"; + reasoningEffort?: ChatReasoningEffort; };Also applies to: 61-68, 136-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/indexeddb/chats.ts` around lines 50 - 59, The repeated reasoningEffort literal union in ChatAdvisorServerToolConfig, ChatSubagentServerToolConfig, and ChatModelSettings should be consolidated into a shared type alias or exported union so updates happen in one place. Define a single reusable symbol near the existing chat settings types in chats.ts, then replace each inline union with that shared type while preserving the same allowed values.apps/web/src/components/(chat)/playground/chat-playground-core.ts (1)
19-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify
SUPPORTED_CHAT_SERVER_TOOLSstays in sync withChatServerToolType.This set is manually kept parallel to
ChatServerToolTypedefined inapps/web/src/lib/indexeddb/chats.ts. They currently match, but nothing enforces that at compile time — adding a new tool literal to the type without updating this set would silently disable the tool fornormalizeServerToolsfiltering, rather than raising a type/lint error.♻️ Suggested compile-time guard
-const SUPPORTED_CHAT_SERVER_TOOLS = new Set<ChatServerToolType>([ - "gateway:datetime", - ... -]); +const SUPPORTED_CHAT_SERVER_TOOLS_LIST = [ + "gateway:datetime", + "ai-stats:web_search", + "ai-stats:web_fetch", + "ai-stats:advisor", + "ai-stats:image_generation", + "ai-stats:fusion", + "ai-stats:subagent", +] as const satisfies readonly ChatServerToolType[]; +// TS will error if a ChatServerToolType member is missing above (via exhaustiveness check elsewhere) or extra +const SUPPORTED_CHAT_SERVER_TOOLS = new Set<ChatServerToolType>( + SUPPORTED_CHAT_SERVER_TOOLS_LIST, +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/`(chat)/playground/chat-playground-core.ts around lines 19 - 41, `SUPPORTED_CHAT_SERVER_TOOLS` is manually duplicating `ChatServerToolType`, so new tool literals can be added to the type in `chats.ts` without being accepted by `normalizeServerTools`. Add a compile-time guard in `chat-playground-core.ts` that ties `SUPPORTED_CHAT_SERVER_TOOLS` to `ChatServerToolType` (for example via a type-level exhaustiveness check or mapped assertion) so any mismatch fails TypeScript compilation. Keep the guard near `SUPPORTED_CHAT_SERVER_TOOLS` and `normalizeServerTools` so the sync requirement is enforced automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/pipeline/after/stream.ts`:
- Around line 297-318: The terminal message_stop frame is dropping timing
because timingUsage falls back to null instead of preserving the last computed
usage. Update the stream timing logic in the after/stream flow so the
message_stop path uses latestStreamUsageRaw when next.usage,
next.response.usage, and next.message.usage are absent, while keeping the
existing matchedTimingFrame and attachStreamTimingMeta behavior intact.
In `@apps/web/src/components/`(chat)/ChatConversation.tsx:
- Around line 477-511: The queue-drain lock in ChatConversation’s useEffect can
stay stuck if sendPrompt/onSend rejects a prompt before isSending changes, which
prevents later queued prompts from draining. Update the send path around
sendPrompt(nextPrompt) so the lock is cleared when the send is not accepted, or
have sendPrompt return an explicit acceptance result and only remove the
prompt/set queue state when accepted. Use queueDrainInFlightRef, sendPrompt, and
the queuedPrompts/activeThreadId effect as the main touchpoints.
---
Outside diff comments:
In `@apps/web/src/app/api/chat/_shared/gatewayProxy.ts`:
- Around line 32-36: `resolveGatewayBaseUrl()` in `gatewayProxy.ts` currently
falls back to `PUBLIC_GATEWAY_BASE_URL`, which makes the missing-config branch
in `proxyGatewayPost()` and `proxyGatewayGet()` unreachable. Decide whether
misconfigured deployments should fail closed or fail open: if fail closed,
restore a null/absent path so `buildGatewayMissingConfigResponse()` can still be
returned; if fail open, remove the dead guard and update the gateway base URL
logic/documentation to reflect the fallback.
In `@apps/web/src/app/api/chat/audio/route.ts`:
- Around line 50-82: The music polling GET handler currently ignores the gateway
override used by POST, so polling can hit the wrong backend. Update the GET path
in audio route’s GET handler to read the same baseUrl query param and thread it
into both the polling URL built by resolveMusicPollPath and the proxyGatewayGet
call. Keep the existing resourceId/action validation in place, but ensure the
resolved poll request uses the caller-provided baseUrl when present.
In `@apps/web/src/app/api/chat/video/route.ts`:
- Around line 86-119: The GET handler in video/route.ts is dropping the custom
gateway override when proxying both the list and poll branches. Update the GET
logic to forward baseUrl into proxyGatewayGet alongside the existing path from
resolveVideoListPath and resolveVideoPollPath, so requests created against a
custom gateway continue to resolve correctly.
In `@apps/web/src/components/`(chat)/playground/chat-playground-core.ts:
- Around line 467-507: The buildAdvisorDefinition helper currently forwards
advisor.temperature without normalization, unlike the sibling subagent path.
Update buildAdvisorDefinition to clamp the temperature value to the accepted [0,
2] range before assigning it to parameters.temperature, and keep the existing
checks in place for advisor, advisor.temperature, and the
withParameters("ai-stats:advisor", ...) call so the advisor payload stays
consistent.
---
Nitpick comments:
In `@apps/web/src/app/api/chat/_shared/gatewayProxy.ts`:
- Around line 223-249: Add dedicated unit tests for the allow-list logic in
isDevelopmentLocalGatewayBaseUrl and resolveGatewayBaseUrl, since these
functions control which client-supplied baseUrl is accepted. Cover the exact
match branch, the configuredGatewayBaseUrl and PUBLIC_GATEWAY_BASE_URL fallback
behavior, and the development-local acceptance/rejection branches for
localhost/127.0.0.1, protocol, port, and pathname. Place the tests near the
gatewayProxy coverage so the new SSRF-sensitive rules are exercised
independently of chats.test.ts.
In `@apps/web/src/components/`(chat)/ChatConversationComposer.tsx:
- Around line 517-532: The `getReadableTextColor` helper in
`ChatConversationComposer` is duplicated in `ChatConversationMessages`, so
extract it into a shared utility module as a named export and update both
components to import and use that shared function. Keep the implementation in
one place only, and remove the local duplicate so the `getReadableTextColor`
behavior stays consistent across both chat components.
In `@apps/web/src/components/`(chat)/playground/chat-playground-core.ts:
- Around line 19-41: `SUPPORTED_CHAT_SERVER_TOOLS` is manually duplicating
`ChatServerToolType`, so new tool literals can be added to the type in
`chats.ts` without being accepted by `normalizeServerTools`. Add a compile-time
guard in `chat-playground-core.ts` that ties `SUPPORTED_CHAT_SERVER_TOOLS` to
`ChatServerToolType` (for example via a type-level exhaustiveness check or
mapped assertion) so any mismatch fails TypeScript compilation. Keep the guard
near `SUPPORTED_CHAT_SERVER_TOOLS` and `normalizeServerTools` so the sync
requirement is enforced automatically.
In `@apps/web/src/lib/indexeddb/chats.ts`:
- Around line 50-59: The repeated reasoningEffort literal union in
ChatAdvisorServerToolConfig, ChatSubagentServerToolConfig, and ChatModelSettings
should be consolidated into a shared type alias or exported union so updates
happen in one place. Define a single reusable symbol near the existing chat
settings types in chats.ts, then replace each inline union with that shared type
while preserving the same allowed values.
🪄 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 Plus
Run ID: bd08e5b4-623e-44ed-8f53-12b3d4da4aaa
📒 Files selected for processing (26)
apps/api/src/pipeline/after/stream.tsapps/web/src/app/api/chat/_shared/gatewayProxy.tsapps/web/src/app/api/chat/audio/route.tsapps/web/src/app/api/chat/embeddings/route.tsapps/web/src/app/api/chat/image/route.tsapps/web/src/app/api/chat/moderation/route.tsapps/web/src/app/api/chat/text/route.tsapps/web/src/app/api/chat/video/route.tsapps/web/src/components/(chat)/ChatConversation.tsxapps/web/src/components/(chat)/ChatConversationComposer.tsxapps/web/src/components/(chat)/ChatConversationMessages.tsxapps/web/src/components/(chat)/ChatDeleteDialog.tsxapps/web/src/components/(chat)/ChatHeader.tsxapps/web/src/components/(chat)/ChatMessageFooters.tsxapps/web/src/components/(chat)/ChatMessageMarkers.tsxapps/web/src/components/(chat)/ChatPlayground.tsxapps/web/src/components/(chat)/ChatShortcutReference.tsxapps/web/src/components/(chat)/ChatSidebar.tsxapps/web/src/components/(chat)/ChatTagsDialog.tsxapps/web/src/components/(chat)/chatConversationHelpers.tsapps/web/src/components/(chat)/playground/chat-playground-core.test.tsapps/web/src/components/(chat)/playground/chat-playground-core.tsapps/web/src/components/(chat)/playground/use-grouped-chat-threads.tsapps/web/src/components/ai-elements/reasoning.tsxapps/web/src/lib/config/featureLabels.tsapps/web/src/lib/indexeddb/chats.ts
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/lib/config/featureLabels.ts
1 participant
Summary
Validation
Created with Codex
Summary by CodeRabbit
New Features
Bug Fixes
Tests