Feat/UI fixes#232
Merged
Merged
Conversation
Made-with: Cursor
Made-with: Cursor
…pdown, composer layout fixes - Add Search action to prompt builder (workspace/web/deep research) - Group Flashcards and Quiz under Learn dropdown in composer - Fix chat collapse to unmaximize when maximized - Show collapse button when maximized - Restructure thread composer layout, improve scroll-to-bottom Made-with: Cursor
…s, breadcrumb chevrons Made-with: Cursor
…wport, test mocks - Include userName in createEvent for activity/history - Remove PDF viewport gap - Fix workspace-worker edit test mock types Made-with: Cursor
…ments - Route Claude models exclusively through Vertex AI (no Anthropic) - Add Claude Haiku 4.5 to model selector - Add provider timeouts (30s BYOK) for vertex/google/anthropic/openai - Add user tracking for Gateway analytics - Log resolved provider in chat API Made-with: Cursor
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (113)
📝 WalkthroughWalkthroughReconfigures chat routing and provider selection (Claude prefixing and provider fallbacks), adds a "search" action to the prompt builder, adjusts UI behaviors and iconography, threads userName into workspace events, removes the large HeroAnimation component, and updates tooltip/kbd/provider usages and various small UX tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as Next API Route
participant Gateway
participant Provider
participant Worker as WorkspaceWorker
participant DB
Client->>Route: POST /api/chat (modelId, payload, headers)
Route->>Gateway: prepare request (apply Claude prefixing, providerOptions, timeouts, metadata headers)
Gateway->>Provider: route request (Claude -> anthropic/Vertex or Gemini fallback)
Provider-->>Gateway: stream response (with providerMetadata)
Gateway-->>Route: stream -> Route attaches http-referer/title headers, forwards stream to client
Route->>Route: on stream end, read providerMetadata and log resolved provider
Route->>Worker: append workspace event (message/event) with userName included
Worker->>DB: INSERT append_workspace_event (payload includes userName)
DB-->>Worker: ack
Worker-->>Route: event append ack
Route-->>Client: final response / stream finished
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| import { useAui } from "@assistant-ui/react"; | ||
| import { focusComposerInput } from "@/lib/utils/composer-utils"; | ||
| import { Brain, Play, ChevronUp, ChevronDown, ChevronRight, X, Circle, CircleDot, ArrowUpIcon, ArrowLeft, CheckCircle2, Folder as FolderIcon } from "lucide-react"; | ||
| import { Brain, Play, ChevronUp, ChevronDown, ChevronRight, X, Circle, CircleDot, ArrowUpIcon, ArrowLeft, CheckCircle2, Folder as FolderIcon, Search, FolderSearch, Globe, Sparkles } from "lucide-react"; |
| import { useState, useEffect, useCallback, useRef, memo, useLayoutEffect } from 'react'; | ||
| import { createPortal } from "react-dom"; | ||
| import { X, RotateCcw, Maximize, Minimize, ChevronUp, ChevronDown, MoreHorizontal, Expand, Shrink, Camera, Download } from 'lucide-react'; | ||
| import { X, RotateCcw, ChevronUp, ChevronDown, MoreHorizontal, Expand, Shrink, Camera, Download } from 'lucide-react'; |
| import Link from "next/link"; | ||
| import { usePathname, useRouter } from "next/navigation"; | ||
| import { Search, X, ChevronDown, FolderOpen, Plus, Upload, Folder as FolderIcon, Settings, Share2, Play, Brain, File, ImageIcon, Mic, PanelRight } from "lucide-react"; | ||
| import { Search, X, ChevronDown, ChevronRight, FolderOpen, Plus, Upload, Folder as FolderIcon, Settings, Share2, Play, Brain, File, ImageIcon, Mic, PanelRight } from "lucide-react"; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/workspace-canvas/MarqueeSelector.tsx">
<violation number="1" location="src/components/workspace-canvas/MarqueeSelector.tsx:129">
P2: Selection is cleared unconditionally before interactive-target guards, causing text/input selections to be removed even when marquee selection should not run.</violation>
</file>
<file name="src/components/assistant-ui/thread.tsx">
<violation number="1" location="src/components/assistant-ui/thread.tsx:430">
P3: This `composerFill` fallback is unreachable with the current `SUGGESTION_ACTIONS` shape, leaving dead conditional logic in the click handler.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| // Clear native text selection when clicking on workspace background | ||
| // (MarqueeSelector captures background clicks before they reach WorkspaceSection) | ||
| window.getSelection()?.removeAllRanges(); |
Contributor
There was a problem hiding this comment.
P2: Selection is cleared unconditionally before interactive-target guards, causing text/input selections to be removed even when marquee selection should not run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/workspace-canvas/MarqueeSelector.tsx, line 129:
<comment>Selection is cleared unconditionally before interactive-target guards, causing text/input selections to be removed even when marquee selection should not run.</comment>
<file context>
@@ -124,6 +124,10 @@ export function MarqueeSelector({
+ // Clear native text selection when clicking on workspace background
+ // (MarqueeSelector captures background clicks before they reach WorkspaceSection)
+ window.getSelection()?.removeAllRanges();
+
// Check if clicking on any interactive or grid element
</file context>
| } else if (suggestedAction.useDialog && "action" in suggestedAction && suggestedAction.action) { | ||
| setDialogAction(suggestedAction.action); | ||
| } else if ("composerFill" in suggestedAction && suggestedAction.composerFill) { | ||
| } else if ("composerFill" in suggestedAction && typeof suggestedAction.composerFill === "string") { |
Contributor
There was a problem hiding this comment.
P3: This composerFill fallback is unreachable with the current SUGGESTION_ACTIONS shape, leaving dead conditional logic in the click handler.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/assistant-ui/thread.tsx, line 430:
<comment>This `composerFill` fallback is unreachable with the current `SUGGESTION_ACTIONS` shape, leaving dead conditional logic in the click handler.</comment>
<file context>
@@ -423,7 +427,7 @@ const ThreadSuggestions: FC<ThreadSuggestionsProps> = ({ items }) => {
} else if (suggestedAction.useDialog && "action" in suggestedAction && suggestedAction.action) {
setDialogAction(suggestedAction.action);
- } else if ("composerFill" in suggestedAction && suggestedAction.composerFill) {
+ } else if ("composerFill" in suggestedAction && typeof suggestedAction.composerFill === "string") {
handleDirectFill(suggestedAction.composerFill);
}
</file context>
Member
Author
|
Tip For best results, initiate chat on the files or code changes.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/workspace-canvas/MarqueeSelector.tsx (1)
127-142:⚠️ Potential issue | 🟡 MinorSelection cleared before interactive element check contradicts stated intent.
The comment states this clears selection "when clicking on workspace background," but
removeAllRanges()executes unconditionally before the interactive element check at lines 131-142. This means selection is cleared even when clicking on cards, buttons, or other interactive elements.While event ordering may mitigate some issues (native DOM listener fires after React synthetic events), consider moving the call after the check to match the stated intent:
Proposed fix to align behavior with comment
// Don't start if clicking on a card or interactive element const target = e.target as HTMLElement; - // Clear native text selection when clicking on workspace background - // (MarqueeSelector captures background clicks before they reach WorkspaceSection) - window.getSelection()?.removeAllRanges(); - // Check if clicking on any interactive or grid element if ( target.closest('article') || // Card itself target.closest('button') || // Any button target.closest('[role="button"]') || // Button role elements target.closest('.react-grid-item') || // Grid item wrapper target.classList.contains('drag-handle') || // Drag handle target.tagName === 'INPUT' || // Input fields target.tagName === 'TEXTAREA' // Text areas ) { return; } + // Clear native text selection when clicking on workspace background + window.getSelection()?.removeAllRanges(); + startMarqueeSelection(e);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/workspace-canvas/MarqueeSelector.tsx` around lines 127 - 142, The selection-clearing call window.getSelection()?.removeAllRanges() is executed unconditionally in MarqueeSelector.tsx before checking interactive elements (the target.closest/... checks), so selections get cleared even when clicking buttons/cards; move the removeAllRanges() call to after the interactive-element guard (i.e., only run it when none of target.closest('article'|'button'|'[role="button"]'|'.react-grid-item') nor tag checks (target.tagName === 'INPUT'/'TEXTAREA') or drag-handle match), or wrap it in the same conditional that verifies the click is on the workspace background so it only clears selection for background clicks.src/components/workspace-canvas/WorkspaceSection.tsx (1)
359-372:⚠️ Potential issue | 🟠 MajorSelection cleared before button check breaks highlight feature.
The
removeAllRanges()call at line 361 executes before the interactive element check at lines 364-372. When a user selects text and clicks the highlight button:
- mousedown bubbles to WorkspaceSection
removeAllRanges()clears the selection- Code checks for button and returns early
- Later,
addHighlight()callswindow.getSelection()and finds no selectionPer
SelectableText.tsx:263-270,addHighlightlogs "no selection available" and returns early when the selection is empty.Proposed fix to preserve selection for button clicks
const handleWorkspaceMouseDown = (event: React.MouseEvent<HTMLDivElement>) => { const target = event.target as HTMLElement; - // Clear native text selection when clicking anywhere in the workspace - // (Background clicks are handled by MarqueeSelector; this fires for card clicks etc.) - window.getSelection()?.removeAllRanges(); - // Don't blur if clicking directly on an input/textarea or button if ( target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.tagName === 'BUTTON' || target.closest('button') || target.closest('[role="button"]') ) { return; } + + // Clear native text selection when clicking on non-interactive workspace areas + window.getSelection()?.removeAllRanges();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/workspace-canvas/WorkspaceSection.tsx` around lines 359 - 372, The selection is being cleared before the early-return that protects interactive elements in the WorkspaceSection mousedown handler, which causes addHighlight (in SelectableText) to see an empty selection; fix it by moving or guarding the window.getSelection()?.removeAllRanges() call so it only runs when the click is NOT on an input/textarea/button (or inside a button/role="button") — i.e., perform the target.tagName/target.closest checks first and return early for interactive elements, and only then call removeAllRanges(); update the mousedown handler in WorkspaceSection accordingly.src/components/assistant-ui/PromptBuilderDialog.tsx (1)
191-200:⚠️ Potential issue | 🟠 MajorSearch should start from a clean card-selection state.
searchis the only action here without a workspace-context picker, but this dialog still seeds fromselectedCardIds. If the user already has cards selected, the existing prefill effect will populate the simple input with"these items"and enable Send before they enter a real query. The submit path then only skipsselectMultipleCards, so the search flow still inherits stale selection state instead of starting clean.Also applies to: 300-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/assistant-ui/PromptBuilderDialog.tsx` around lines 191 - 200, When the dialog opens for a search flow, the component still inherits previous selectedCardIds and pre-fills the simple input ("these items"); clear that stale selection and input so search starts clean: in the open-useEffect (the block that currently calls setCount, setNoteTemplate, setSearchSource) also call setSelectedCardIds([]) and setSimpleInput("") whenever search is the intended action (or unconditionally if this dialog is exclusively search), and ensure the submit path that skips selectMultipleCards no longer inherits prior selection; also apply the same change for the similar init block around the 300-317 region so both entry points reset selectedCardIds and simpleInput.
🧹 Nitpick comments (3)
src/lib/ai/workers/__tests__/workspace-worker.edit.test.ts (1)
22-22: Add one assertion for the newuserNamethreading.These variadic mocks make the suite tolerant of the extra arg, but they do not verify it. Right now a regression that stops passing
userNametocreateEventwould still pass because the test session has no name/email and the mock ignores arg 4. Please add a case that seedssession.user.name(or email) and assertsmockCreateEventreceives that fourth argument.Example assertion to add
mockGetSession.mockResolvedValue({ user: { id: "user-1", name: "Alice", email: "alice@example.com" }, }); await workspaceWorker("edit", { workspaceId: "ws-1", itemId: "quiz-1", itemType: "quiz", itemName: "Quiz 1", oldString: " ]\n}", newString: " ]\n}", }); expect(mockCreateEvent).toHaveBeenCalledWith( expect.any(String), expect.anything(), "user-1", "Alice" );Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/ai/workers/__tests__/workspace-worker.edit.test.ts` at line 22, The test uses variadic mocks so it doesn't verify the new userName threading; update the tests that use mockGetSession and mockCreateEvent (in workspace-worker.edit.test.ts) to seed mockGetSession.mockResolvedValue with a session.user containing id and name (or email) and add an assertion after calling workspaceWorker("edit", ...) that mockCreateEvent was called with the expected fourth argument (e.g., expect(mockCreateEvent).toHaveBeenCalledWith(expect.any(String), expect.anything(), "user-1", "Alice")); apply the same added assertion for the other occurrence referenced in the file so regressions that drop passing userName are caught.src/app/api/chat/route.ts (2)
399-405: Log the resolved provider insideonFinish.AI SDK already exposes final-step
providerMetadatainonFinish, andresult.providerMetadatais just another promise that resolves when the stream finishes. Since you already have anonFinishhandler, this detached promise adds a second completion path for the same log and has no rejection handling. Moving the log intoonFinishkeeps it inside the stream lifecycle. (ai-sdk.dev)Suggested fix
- onFinish: ({ usage, finishReason }) => { + onFinish: ({ usage, finishReason, providerMetadata }) => { const usageInfo = { inputTokens: usage?.inputTokens, outputTokens: usage?.outputTokens, totalTokens: usage?.totalTokens, cachedInputTokens: usage?.cachedInputTokens, // Standard property reasoningTokens: usage?.reasoningTokens, // Note: Extended provider-specific properties might not be available consistently via Gateway finishReason, }; logger.info("📊 [CHAT-API] Final Token Usage:", usageInfo); + + const provider = + (providerMetadata as any)?.gateway?.routing?.resolvedProvider ?? + (providerMetadata as any)?.gateway?.routing?.finalProvider; + if (provider) { + logger.info("🔍 [CHAT-API] Gateway resolved provider:", provider); + } }, @@ - // Log which provider the Gateway actually used (resolves when stream completes) - void Promise.resolve((result as any).providerMetadata).then((meta: any) => { - const provider = meta?.gateway?.routing?.resolvedProvider ?? meta?.gateway?.routing?.finalProvider; - if (provider) { - logger.info("🔍 [CHAT-API] Gateway resolved provider:", provider); - } - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/chat/route.ts` around lines 399 - 405, The detached Promise.resolve((result as any).providerMetadata) logging creates a second completion path without rejection handling—remove that detached promise and instead log the resolved provider inside the existing onFinish handler: access result.providerMetadata (or the meta parameter passed into onFinish), extract meta?.gateway?.routing?.resolvedProvider ?? meta?.gateway?.routing?.finalProvider into a provider variable, and call logger.info("🔍 [CHAT-API] Gateway resolved provider:", provider) only when provider is present; ensure you handle providerMetadata resolution errors inside onFinish rather than leaving an unhandled promise.
350-362: Use the request origin instead of a production fallback.If
NEXT_PUBLIC_APP_URLis unset, preview/local/self-hosted environments will advertisehttps://thinkex.appas the outbound referer. Deriving this fromreq.urlkeeps provider-side attribution accurate.Suggested fix
- const appUrl = process.env.NEXT_PUBLIC_APP_URL || "https://thinkex.app"; + const appUrl = process.env.NEXT_PUBLIC_APP_URL ?? new URL(req.url).origin;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/chat/route.ts` around lines 350 - 362, The code currently sets appUrl from NEXT_PUBLIC_APP_URL and passes it into streamText headers as "http-referer"; change this to derive the origin from the incoming request URL (use the request object's URL/origin) and only fallback to NEXT_PUBLIC_APP_URL or an empty string if the origin cannot be derived so preview/local/self-hosted environments advertise the correct referer; update the appUrl assignment and the headers passed into streamText (the appUrl variable and the "http-referer" header in the streamText(...) call) to use the computed request origin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/api/chat/route.ts`:
- Around line 233-235: The code currently prefixes any modelId that contains
"claude" unless it already starts with "anthropic/", which incorrectly mangles
qualified slugs like "vertex/claude-sonnet-4.5"; change the condition to only
auto-prefix unqualified Claude IDs by checking that the id contains "claude" and
has no namespace slash (e.g., replace the branch using
modelId.includes("claude") && !modelId.startsWith("anthropic/") with a check
like modelId.includes("claude") && !modelId.includes("/") so only bare ids
(e.g., "claude-sonnet-4.1") become "anthropic/claude-sonnet-4.1"); update the
branch around the modelId variable where this rewrite occurs.
In `@src/components/ui/tooltip.tsx`:
- Around line 21-29: The Tooltip component incorrectly creates a TooltipProvider
per instance (wrapping TooltipProvider around TooltipPrimitive.Root), which
overrides any outer/global provider settings; change Tooltip to stop
instantiating TooltipProvider and instead pass delayDuration directly into
TooltipPrimitive.Root (i.e., accept delayDuration prop and forward it to
TooltipPrimitive.Root), leaving global TooltipProvider management to callers so
sidebar.tsx and CollaboratorAvatars.tsx can control the shared delay behavior.
In `@src/components/workspace-canvas/WorkspaceHeader.tsx`:
- Around line 564-565: The ChevronRight icons used as breadcrumb separators in
WorkspaceHeader.tsx are decorative and should be hidden from assistive tech;
update each ChevronRight instance (e.g., the ones near the DropdownMenu and
other separator occurrences) to include an accessibility attribute such as
aria-hidden="true" (or role="presentation") so screen readers ignore them,
leaving only the real breadcrumb link text announced.
In `@src/lib/ai/workers/workspace-worker.ts`:
- Line 268: The code falls back to session.user.email when building userName
which can persist PII into workspace_events.user_name; change the assignment for
userName to only use a non-sensitive public display field (e.g.
session.user.displayName) or session.user.name and do not fall back to
session.user.email so that userName is undefined when no public name exists.
Update the expression that sets userName (currently using session.user.name ||
session.user.email) to only use a safe public field or session.user.name (e.g.
session.user.displayName || session.user.name || undefined) and remove any use
of session.user.email so workspace_events.user_name never stores raw emails.
---
Outside diff comments:
In `@src/components/assistant-ui/PromptBuilderDialog.tsx`:
- Around line 191-200: When the dialog opens for a search flow, the component
still inherits previous selectedCardIds and pre-fills the simple input ("these
items"); clear that stale selection and input so search starts clean: in the
open-useEffect (the block that currently calls setCount, setNoteTemplate,
setSearchSource) also call setSelectedCardIds([]) and setSimpleInput("")
whenever search is the intended action (or unconditionally if this dialog is
exclusively search), and ensure the submit path that skips selectMultipleCards
no longer inherits prior selection; also apply the same change for the similar
init block around the 300-317 region so both entry points reset selectedCardIds
and simpleInput.
In `@src/components/workspace-canvas/MarqueeSelector.tsx`:
- Around line 127-142: The selection-clearing call
window.getSelection()?.removeAllRanges() is executed unconditionally in
MarqueeSelector.tsx before checking interactive elements (the target.closest/...
checks), so selections get cleared even when clicking buttons/cards; move the
removeAllRanges() call to after the interactive-element guard (i.e., only run it
when none of
target.closest('article'|'button'|'[role="button"]'|'.react-grid-item') nor tag
checks (target.tagName === 'INPUT'/'TEXTAREA') or drag-handle match), or wrap it
in the same conditional that verifies the click is on the workspace background
so it only clears selection for background clicks.
In `@src/components/workspace-canvas/WorkspaceSection.tsx`:
- Around line 359-372: The selection is being cleared before the early-return
that protects interactive elements in the WorkspaceSection mousedown handler,
which causes addHighlight (in SelectableText) to see an empty selection; fix it
by moving or guarding the window.getSelection()?.removeAllRanges() call so it
only runs when the click is NOT on an input/textarea/button (or inside a
button/role="button") — i.e., perform the target.tagName/target.closest checks
first and return early for interactive elements, and only then call
removeAllRanges(); update the mousedown handler in WorkspaceSection accordingly.
---
Nitpick comments:
In `@src/app/api/chat/route.ts`:
- Around line 399-405: The detached Promise.resolve((result as
any).providerMetadata) logging creates a second completion path without
rejection handling—remove that detached promise and instead log the resolved
provider inside the existing onFinish handler: access result.providerMetadata
(or the meta parameter passed into onFinish), extract
meta?.gateway?.routing?.resolvedProvider ??
meta?.gateway?.routing?.finalProvider into a provider variable, and call
logger.info("🔍 [CHAT-API] Gateway resolved provider:", provider) only when
provider is present; ensure you handle providerMetadata resolution errors inside
onFinish rather than leaving an unhandled promise.
- Around line 350-362: The code currently sets appUrl from NEXT_PUBLIC_APP_URL
and passes it into streamText headers as "http-referer"; change this to derive
the origin from the incoming request URL (use the request object's URL/origin)
and only fallback to NEXT_PUBLIC_APP_URL or an empty string if the origin cannot
be derived so preview/local/self-hosted environments advertise the correct
referer; update the appUrl assignment and the headers passed into streamText
(the appUrl variable and the "http-referer" header in the streamText(...) call)
to use the computed request origin.
In `@src/lib/ai/workers/__tests__/workspace-worker.edit.test.ts`:
- Line 22: The test uses variadic mocks so it doesn't verify the new userName
threading; update the tests that use mockGetSession and mockCreateEvent (in
workspace-worker.edit.test.ts) to seed mockGetSession.mockResolvedValue with a
session.user containing id and name (or email) and add an assertion after
calling workspaceWorker("edit", ...) that mockCreateEvent was called with the
expected fourth argument (e.g.,
expect(mockCreateEvent).toHaveBeenCalledWith(expect.any(String),
expect.anything(), "user-1", "Alice")); apply the same added assertion for the
other occurrence referenced in the file so regressions that drop passing
userName are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef0b72c3-8090-463c-9280-99c58154a04e
📒 Files selected for processing (17)
.gitignoresrc/app/api/chat/route.tssrc/components/assistant-ui/AssistantPanel.tsxsrc/components/assistant-ui/PromptBuilderDialog.tsxsrc/components/assistant-ui/thread.tsxsrc/components/chat/AppChatHeader.tsxsrc/components/landing/HeroAnimation.tsxsrc/components/pdf/AppPdfViewer.tsxsrc/components/pdf/PdfPanelHeader.tsxsrc/components/ui/tooltip.tsxsrc/components/workspace-canvas/ItemPanelContent.tsxsrc/components/workspace-canvas/MarqueeSelector.tsxsrc/components/workspace-canvas/WorkspaceHeader.tsxsrc/components/workspace-canvas/WorkspaceSection.tsxsrc/components/workspace-canvas/YouTubePlayerControls.tsxsrc/lib/ai/workers/__tests__/workspace-worker.edit.test.tssrc/lib/ai/workers/workspace-worker.ts
💤 Files with no reviewable changes (2)
- src/components/landing/HeroAnimation.tsx
- .gitignore
Comment on lines
+233
to
+235
| // Auto-prefix with anthropic/ if it looks like a Claude model and lacks prefix | ||
| if (modelId.includes("claude") && !modelId.startsWith("anthropic/")) { | ||
| modelId = `anthropic/${modelId}`; |
There was a problem hiding this comment.
Only auto-prefix unqualified Claude IDs.
This branch rewrites any model ID containing claude unless it already starts with anthropic/, so a valid qualified slug like vertex/claude-sonnet-4.5 would become anthropic/vertex/claude-sonnet-4.5. The current UI already sends anthropic/... values from src/components/assistant-ui/thread.tsx:122-148, but this API route should not mangle other callers.
Suggested fix
- if (modelId.includes("claude") && !modelId.startsWith("anthropic/")) {
+ if (!modelId.includes("/") && modelId.startsWith("claude")) {
modelId = `anthropic/${modelId}`;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/chat/route.ts` around lines 233 - 235, The code currently
prefixes any modelId that contains "claude" unless it already starts with
"anthropic/", which incorrectly mangles qualified slugs like
"vertex/claude-sonnet-4.5"; change the condition to only auto-prefix unqualified
Claude IDs by checking that the id contains "claude" and has no namespace slash
(e.g., replace the branch using modelId.includes("claude") &&
!modelId.startsWith("anthropic/") with a check like modelId.includes("claude")
&& !modelId.includes("/") so only bare ids (e.g., "claude-sonnet-4.1") become
"anthropic/claude-sonnet-4.1"); update the branch around the modelId variable
where this rewrite occurs.
Comment on lines
+564
to
565
| <ChevronRight className="h-3.5 w-3.5 text-sidebar-foreground/50 mx-1 shrink-0" /> | ||
| <DropdownMenu> |
There was a problem hiding this comment.
Hide breadcrumb separators from assistive tech.
These ChevronRight icons are decorative. Without aria-hidden, they can be announced as extra graphics between breadcrumb items.
Accessibility fix
- <ChevronRight className="h-3.5 w-3.5 text-sidebar-foreground/50 mx-1 shrink-0" />
+ <ChevronRight aria-hidden="true" className="h-3.5 w-3.5 text-sidebar-foreground/50 mx-1 shrink-0" />Also applies to: 629-630, 652-653, 697-698, 725-725
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/workspace-canvas/WorkspaceHeader.tsx` around lines 564 - 565,
The ChevronRight icons used as breadcrumb separators in WorkspaceHeader.tsx are
decorative and should be hidden from assistive tech; update each ChevronRight
instance (e.g., the ones near the DropdownMenu and other separator occurrences)
to include an accessibility attribute such as aria-hidden="true" (or
role="presentation") so screen readers ignore them, leaving only the real
breadcrumb link text announced.
| throw new Error("User not authenticated"); | ||
| } | ||
| const userId = session.user.id; | ||
| const userName = session.user.name || session.user.email || undefined; |
There was a problem hiding this comment.
Avoid storing raw email addresses as event display names.
Falling back from session.user.name to session.user.email will persist email addresses into workspace_events.user_name, which can leak PII to collaborators and any UI consuming event history. Prefer a non-sensitive public display field here, and leave userName unset when no display name exists.
Suggested fix
- const userName = session.user.name || session.user.email || undefined;
+ const userName = session.user.name?.trim() || undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const userName = session.user.name || session.user.email || undefined; | |
| const userName = session.user.name?.trim() || undefined; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/ai/workers/workspace-worker.ts` at line 268, The code falls back to
session.user.email when building userName which can persist PII into
workspace_events.user_name; change the assignment for userName to only use a
non-sensitive public display field (e.g. session.user.displayName) or
session.user.name and do not fall back to session.user.email so that userName is
undefined when no public name exists. Update the expression that sets userName
(currently using session.user.name || session.user.email) to only use a safe
public field or session.user.name (e.g. session.user.displayName ||
session.user.name || undefined) and remove any use of session.user.email so
workspace_events.user_name never stores raw emails.
Contributor
There was a problem hiding this comment.
4 issues found across 115 files (changes from recent commits).
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/components/ui/tooltip.tsx">
<violation number="1" location="src/components/ui/tooltip.tsx:28">
P2: `delayDuration` now falls back to Radix’s 700ms default when callers omit it, because the Tooltip component no longer wraps `TooltipProvider`. This regresses the intended 300ms delay. Consider applying the local default when passing the prop.</violation>
</file>
<file name=".agents/skills/shadcn/evals/evals.json">
<violation number="1" location=".agents/skills/shadcn/evals/evals.json:7">
P2: Eval 1 has contradictory acceptance criteria (`ToggleGroup` vs required `Switch`), which can lead to inconsistent or incorrect grading.</violation>
</file>
<file name=".agents/skills/shadcn/rules/forms.md">
<violation number="1" location=".agents/skills/shadcn/rules/forms.md:41">
P3: The ToggleGroup option-count guidance is inconsistent (2–7 vs 2–5), which makes the rule ambiguous.</violation>
</file>
<file name=".agents/skills/shadcn/rules/icons.md">
<violation number="1" location=".agents/skills/shadcn/rules/icons.md:94">
P2: The "correct" example hardcodes `lucide-react`, which conflicts with this document’s rule to use the configured `iconLibrary` and avoid assuming lucide.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| </TooltipProvider> | ||
| <TooltipPrimitive.Root | ||
| data-slot="tooltip" | ||
| delayDuration={delayDuration} |
Contributor
There was a problem hiding this comment.
P2: delayDuration now falls back to Radix’s 700ms default when callers omit it, because the Tooltip component no longer wraps TooltipProvider. This regresses the intended 300ms delay. Consider applying the local default when passing the prop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/components/ui/tooltip.tsx, line 28:
<comment>`delayDuration` now falls back to Radix’s 700ms default when callers omit it, because the Tooltip component no longer wraps `TooltipProvider`. This regresses the intended 300ms delay. Consider applying the local default when passing the prop.</comment>
<file context>
@@ -21,27 +21,25 @@ function TooltipProvider({
- </TooltipProvider>
+ <TooltipPrimitive.Root
+ data-slot="tooltip"
+ delayDuration={delayDuration}
+ {...props}
+ />
</file context>
Suggested change
| delayDuration={delayDuration} | |
| delayDuration={delayDuration ?? 300} |
| { | ||
| "id": 1, | ||
| "prompt": "I'm building a Next.js app with shadcn/ui (base-nova preset, lucide icons). Create a settings form component with fields for: full name, email address, and notification preferences (email, SMS, push notifications as toggle options). Add validation states for required fields.", | ||
| "expected_output": "A React component using FieldGroup, Field, ToggleGroup, data-invalid/aria-invalid validation, gap-* spacing, and semantic colors.", |
Contributor
There was a problem hiding this comment.
P2: Eval 1 has contradictory acceptance criteria (ToggleGroup vs required Switch), which can lead to inconsistent or incorrect grading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/skills/shadcn/evals/evals.json, line 7:
<comment>Eval 1 has contradictory acceptance criteria (`ToggleGroup` vs required `Switch`), which can lead to inconsistent or incorrect grading.</comment>
<file context>
@@ -0,0 +1,47 @@
+ {
+ "id": 1,
+ "prompt": "I'm building a Next.js app with shadcn/ui (base-nova preset, lucide icons). Create a settings form component with fields for: full name, email address, and notification preferences (email, SMS, push notifications as toggle options). Add validation states for required fields.",
+ "expected_output": "A React component using FieldGroup, Field, ToggleGroup, data-invalid/aria-invalid validation, gap-* spacing, and semantic colors.",
+ "files": [],
+ "expectations": [
</file context>
|
|
||
| ```tsx | ||
| // Import from the project's configured iconLibrary (e.g. lucide-react, @tabler/icons-react). | ||
| import { CheckIcon } from "lucide-react" |
Contributor
There was a problem hiding this comment.
P2: The "correct" example hardcodes lucide-react, which conflicts with this document’s rule to use the configured iconLibrary and avoid assuming lucide.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/skills/shadcn/rules/icons.md, line 94:
<comment>The "correct" example hardcodes `lucide-react`, which conflicts with this document’s rule to use the configured `iconLibrary` and avoid assuming lucide.</comment>
<file context>
@@ -0,0 +1,101 @@
+
+```tsx
+// Import from the project's configured iconLibrary (e.g. lucide-react, @tabler/icons-react).
+import { CheckIcon } from "lucide-react"
+
+function StatusBadge({ icon: Icon }: { icon: React.ComponentType }) {
</file context>
| - Native HTML select (no JS) → `native-select` | ||
| - Boolean toggle → `Switch` (for settings) or `Checkbox` (for forms) | ||
| - Single choice from few options → `RadioGroup` | ||
| - Toggle between 2–5 options → `ToggleGroup` + `ToggleGroupItem` |
Contributor
There was a problem hiding this comment.
P3: The ToggleGroup option-count guidance is inconsistent (2–7 vs 2–5), which makes the rule ambiguous.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/skills/shadcn/rules/forms.md, line 41:
<comment>The ToggleGroup option-count guidance is inconsistent (2–7 vs 2–5), which makes the rule ambiguous.</comment>
<file context>
@@ -0,0 +1,192 @@
+- Native HTML select (no JS) → `native-select`
+- Boolean toggle → `Switch` (for settings) or `Checkbox` (for forms)
+- Single choice from few options → `RadioGroup`
+- Toggle between 2–5 options → `ToggleGroup` + `ToggleGroupItem`
+- OTP/verification code → `InputOTP`
+- Multi-line text → `Textarea`
</file context>
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 by CodeRabbit
New Features
Bug Fixes
UI/Style
Other Changes