feat(layout): Chat-first UI redesign with artifact panel#130
Open
TinDang97 wants to merge 12 commits into
Open
feat(layout): Chat-first UI redesign with artifact panel#130TinDang97 wants to merge 12 commits into
TinDang97 wants to merge 12 commits into
Conversation
Create infrastructure for chat-first layout redesign behind layout_v2 feature flag (defaults to false — zero visible behavior change): - ArtifactPanelStore: MobX store for tab management (open/close/pin/active) - UIStore: extended with layoutMode, artifactPanelSize, chatColumnSize - ChatFirstShell: layout shell with ConversationSidebar + ResizablePanelGroup - ConversationSidebar: conversation history sidebar - ArtifactPanel + ArtifactTabBar: right panel for structured content - ChatEmptyState: chat-first landing with suggested prompts - WorkspaceFeatureToggles: added layout_v2 toggle
Wire layout_v2 feature flag to swap AppShell → ChatFirstShell: - Workspace layout conditionally renders ChatFirstShell when layout_v2 on - ConversationSidebar: WorkspaceSwitcher, session history, browse shortcuts (Notes/Issues/Projects), SidebarUserControls, collapse toggle - Homepage: renders ChatEmptyState instead of HomepageHub when layout_v2 on - All existing behavior preserved when layout_v2 is off (default)
- ArtifactPanel: auto-transitions layoutMode via MobX reaction (chat-first → chat-artifact when tabs open, reverse when closed) - Expand/collapse button to toggle canvas-first mode - ChatFirstShell: mobile overlay sidebar, expand button when collapsed, responsive auto-collapse on mobile/tablet - ArtifactTabBar: functional tab switching, close, pin indicators
- useRouteArtifact hook: parses pathname to auto-open artifact tabs for notes, issues, projects, members routes - ChatFirstShell: routes artifact pages into ArtifactPanel, chat pages into the main column - ArtifactPanel: accepts children prop for routed content - /chat page: redirects to workspace root when layout_v2 is active
- ChatEmptyState: Compass logo, icon-paired suggested prompts, quick navigation buttons (Notes/Issues/Projects) that route to workspace pages - Prompts: 'Create a new note', 'Show my open issues', 'Start a project plan', 'What should I focus on today?'
- Mobile/tablet: full-screen content, no split panel - Desktop: resizable split when artifact panel active - Sidebar: overlay with backdrop on mobile, inline on desktop - Mobile menu button with backdrop blur - Auto-collapse sidebar on small screens
Critical fixes for chat-first UX:
- ChatFirstShell: owns a persistent ChatView in the chat column,
independent of routing. ChatEmptyState is the emptyStateSlot.
onPromptClick wires to prefillValue for seamless prompt flow.
- Route content (notes/issues) renders inside ArtifactPanel via
{children} when isRouteArtifact is true. ChatView stays in the
chat column during artifact navigation.
- Homepage: renders empty container when layout_v2 on — the shell's
ChatView takes over. HomepageHub still renders when flag is off.
- Mobile: shows ChatView on homepage, route content on artifact pages
- useRouteArtifact: transitions layoutMode to chat-artifact when opening artifact tabs, back to chat-first on homepage navigation. Fixes chicken-and-egg where ArtifactPanel never mounted because layoutMode hadn't changed yet. - ChatFirstShell: persistent ChatView in chat column (independent of routing), ChatEmptyState as emptyStateSlot with prefill wiring - Homepage: empty container when layout_v2 on — shell owns ChatView Verified in browser: split view works (chat left + artifact right), layout transitions between homepage and artifact routes work correctly.
P1 fixes: - ArtifactTabBar: add role="tablist" container, keyboard nav (arrow keys, Home/End), proper tabIndex roving, use <button> for tabs - ChatFirstShell: add aria-hidden to mobile backdrop overlay - ChatFirstShell: fix duplicate id="main-content" in split view P2 fixes: - ArtifactTabBar: increase close button to 20px with hover bg - ConversationSidebar: browse shortcuts h-8 → h-10 (40px touch) - ConversationSidebar: replace width animation with transform (translateX for GPU compositing, no layout reflow) - ChatEmptyState: replace useRouter with Link for quick nav (eliminates unnecessary re-renders from router state) - ConversationSidebar: session buttons get focus-visible ring P3 fixes: - ConversationSidebar: SessionListStore uses useMemo instead of useState — re-creates when pilotSpaceStore becomes available - ChatEmptyState: add overflow-auto + responsive spacing (space-y-6 on small, space-y-10 on sm+) for short viewports - ChatEmptyState: wrap quick nav in <nav> landmark
P0 — First interaction:
- Suggested prompts now auto-send immediately (Claude.ai pattern)
instead of just prefilling the input. Fallback to prefill on error.
- Prefill only applies when input is empty — prevents silently
overwriting user's in-progress text.
P1 — Context preservation:
- ChatView: new `persistentMode` prop skips auto-resume/clear on
context changes. ChatFirstShell passes this to keep conversation
alive across page navigation.
P1 — Error recovery:
- PilotSpaceStreamHandler: detect 401/403 errors and show specific
guidance ("Go to Settings → AI Providers to configure your key")
instead of generic "Unauthorized".
P1 — Issue context:
- Issue detail page now passes issueTitle and issueStatus to
setIssueContext, enabling context-aware AI responses.
From UX design architect review:
- ArtifactTabBar: tab close <span> → <button> with tabIndex={0},
focus ring, keyboard accessible (Enter/Space)
- ConversationSidebar: session <p> → <span> for valid HTML inside
<button>, added aria-label for screen readers
- ChatFirstShell: ARIA live region announces layout transitions
("Artifact panel opened") for screen reader users
- ChatEmptyState redesigned:
- Personalized greeting using userName ("Good morning, Tin.")
- Categorized prompts: Write (2), Plan (2), Review (2) in 2-col grid
- Quick nav only shown when sidebar is collapsed (removes redundancy)
- Left-aligned layout with Compass icon inline (not centered card)
- Capability tagline: "Write notes, plan projects, extract issues,
review code."
10 recommendations implemented:
1.1 Panel slide-in: 200ms ease-out motion animation when artifact
panel opens (transform + opacity, no layout reflow)
1.2 Escape hatch: New Chat button resets to chat-first mode, clears
unpinned tabs, clears conversation
2.3 AI loading skeleton: shimmer skeleton matching ChatEmptyState
layout, 5s timeout shows "check settings" hint with link
2.4 Grouped session history: uses SessionListStore.sessionsGroupedByDate
with sticky Today/Yesterday/weekday headers
2.5 Session context menu: DropdownMenu with Delete action (confirm
dialog), more-options button visible on hover/focus
2.6 Keyboard shortcuts: Cmd+B (sidebar), Cmd+Shift+C (layout toggle),
Cmd+/ (focus chat), Cmd+Shift+N (new chat), Cmd+[/] (back/forward)
3.2 Contextual prompts: ChatEmptyState adapts to artifact context —
note-context (summarize, extract issues), issue-context (subtasks,
similar issues), default (write, plan, review)
4.2 Back/forward navigation: history stack in ArtifactPanelStore,
back/forward buttons in ArtifactTabBar, Cmd+[/] shortcuts
4.3 Sidebar header: workspace name replaces full switcher, search
input for filtering conversations added
7.5 Touch targets: browse shortcuts h-10 → h-11 (44px WCAG 2.2)
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Router as Next.js Router
participant WorkspaceStore
participant UIStore
participant Layout as WorkspaceLayout
participant Shell as Shell (ChatFirstShell / AppShell)
User->>Router: Navigate to workspace
Router->>Layout: Render layout
Layout->>WorkspaceStore: useWorkspaceStore()
WorkspaceStore-->>Layout: Check layout_v2 feature flag
alt layout_v2 enabled
Layout->>Shell: Render ChatFirstShell
Shell->>UIStore: Initialize layout state
Shell->>Shell: hydrate UI from localStorage
Shell->>Router: useRouter hook
User->>Router: Navigate to /workspace/notes
Router->>Shell: Update pathname
Shell->>Shell: useRouteArtifact(true)
Shell->>Shell: parseRouteToArtifact()
Shell->>UIStore: setLayoutMode('chat-artifact')
Shell->>Shell: artifactPanel.openTab()
Shell->>Shell: Render two-panel layout
User->>Shell: Click keyboard shortcut (Cmd+B)
Shell->>Shell: useChatFirstShortcuts
Shell->>UIStore: setLayoutMode() toggle
Shell->>Shell: Re-render with new mode
else layout_v2 disabled
Layout->>Shell: Render AppShell
Shell->>Shell: Render legacy layout
end
sequenceDiagram
participant User
participant ChatView
participant ArtifactPanel
participant ArtifactPanelStore
participant UIStore
User->>ChatView: Click on artifact context prompt
ChatView->>ChatView: onPromptClick()
ChatView->>UIStore: Check prefillValue flow
ChatView->>ChatView: setInputValue() or sendMessage()
alt artifact not yet open
ChatView->>ChatView: Trigger error or empty state
else artifact opens during chat
User->>ArtifactPanel: Artifact tabs trigger
ArtifactPanel->>ArtifactPanelStore: reaction monitors hasOpenTabs
ArtifactPanelStore-->>ArtifactPanel: hasOpenTabs = true
ArtifactPanel->>UIStore: setLayoutMode('chat-artifact')
ArtifactPanel->>ArtifactPanel: Show expand/restore button
ArtifactPanel->>ArtifactTabBar: Render tabs
User->>ArtifactTabBar: Navigate with arrow keys / click tab
ArtifactTabBar->>ArtifactPanelStore: setActiveTab() / goBack() / goForward()
ArtifactPanelStore-->>ArtifactPanel: Update activeTabId
ArtifactPanel->>ArtifactPanel: Display active tab content
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
Review Summary by QodoImplement chat-first UI redesign with artifact panel and layout modes
WalkthroughsDescription• Implements chat-first UI redesign behind layout_v2 feature flag • Adds persistent ChatView with ConversationSidebar and resizable ArtifactPanel • Introduces keyboard shortcuts (Cmd+B, Cmd+Shift+C, Cmd+/, Cmd+Shift+N, Cmd+[/]) • Auto-transitions between layout modes based on artifact panel state • Syncs URL routes to artifact tabs with back/forward navigation history Diagramflowchart LR
A["Feature Flag<br/>layout_v2"] -->|enabled| B["ChatFirstShell"]
A -->|disabled| C["AppShell<br/>legacy"]
B --> D["ConversationSidebar"]
B --> E["ChatView<br/>persistent"]
B --> F["ArtifactPanel"]
E --> G["ChatEmptyState<br/>contextual prompts"]
F --> H["ArtifactTabBar<br/>tab management"]
I["useRouteArtifact"] -->|parse URL| J["Open artifact tab"]
J -->|auto-transition| K["Layout Mode<br/>chat-first/artifact/canvas"]
L["useChatFirstShortcuts"] -->|keyboard| K
M["UIStore"] -->|persist| N["localStorage<br/>layoutMode, sizes"]
File Changes1. frontend/src/components/layout/index.ts
|
Code Review by Qodo
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+72
to
+109
| <button | ||
| key={tab.id} | ||
| type="button" | ||
| role="tab" | ||
| aria-selected={isActive} | ||
| tabIndex={isActive ? 0 : -1} | ||
| className={cn( | ||
| 'group flex items-center gap-1.5 rounded-md px-2.5 py-1.5 text-xs transition-colors', | ||
| 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring', | ||
| isActive | ||
| ? 'bg-accent text-accent-foreground font-medium' | ||
| : 'text-muted-foreground hover:bg-accent/50 hover:text-foreground' | ||
| )} | ||
| onClick={() => artifactPanel.setActiveTab(tab.id)} | ||
| onKeyDown={(e) => handleKeyDown(e, index)} | ||
| > | ||
| <span className="truncate max-w-[120px]">{tab.title}</span> | ||
| {tab.isPinned && <Pin className="h-2.5 w-2.5 text-muted-foreground" />} | ||
| <button | ||
| type="button" | ||
| tabIndex={0} | ||
| aria-label={`Close ${tab.title}`} | ||
| className={cn( | ||
| 'inline-flex items-center justify-center rounded-sm', | ||
| 'h-5 w-5 min-h-[20px] min-w-[20px]', | ||
| 'opacity-0 group-hover:opacity-100 group-focus-within:opacity-100', | ||
| 'focus:opacity-100', | ||
| 'transition-opacity hover:bg-accent-foreground/10', | ||
| 'focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring' | ||
| )} | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| artifactPanel.closeTab(tab.id); | ||
| }} | ||
| > | ||
| <X className="h-3 w-3" /> | ||
| </button> | ||
| </button> |
There was a problem hiding this comment.
1. Nested tab close button 🐞 Bug ≡ Correctness
ArtifactTabBar renders a tab <button> that contains another <button> for closing the tab, which is invalid HTML and can break focus, keyboard navigation, and assistive-tech semantics for the tablist.
Agent Prompt
### Issue description
`ArtifactTabBar` nests a close `<button>` inside the tab `<button role="tab">`, which is invalid HTML and can break focus/keyboard behavior.
### Issue Context
You need two separate interactive controls: the tab activator and the close control.
### Fix Focus Areas
- frontend/src/components/layout/artifact-tab-bar.tsx[68-112]
### Suggested fix approach
- Replace the current structure with a non-interactive wrapper (e.g. `<div className="group ...">`) that contains:
- a single `<button role="tab" ...>` for selecting/activating the tab
- a sibling `<button type="button" ...>` for closing
- Ensure keyboard behavior remains sane:
- Arrow/Home/End should move focus among *tab buttons only*
- Close button should be reachable via Tab (optional) but must not be nested inside the tab button
- Keep `e.stopPropagation()` on the close button if clicking close should not activate the tab.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
frontend/src/app/(workspace)/[workspaceSlug]/chat/page.tsx (1)
27-33: Consider early return to avoid unnecessary rendering during redirect.When
layout_v2is enabled, the redirect effect fires but the component continues executing the remaining effects and renders the ChatView before navigation completes. This causes brief unnecessary work.♻️ Proposed refactor to short-circuit rendering
// Redirect to workspace root when layout_v2 is active (chat IS the homepage) const useV2Layout = workspaceStore.isFeatureEnabled('layout_v2'); useEffect(() => { if (useV2Layout && slug) { router.replace(`/${slug}`); } }, [useV2Layout, slug, router]); + + // Short-circuit rendering when redirect is pending + if (useV2Layout) { + return null; + } // Ensure workspaces are loaded so slug→UUID resolution works🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/`(workspace)/[workspaceSlug]/chat/page.tsx around lines 27 - 33, When layout_v2 is enabled the component still continues rendering and running effects while the redirect effect (useEffect with router.replace) runs; short-circuit the component when workspaceStore.isFeatureEnabled('layout_v2') is true (and slug exists) to avoid rendering ChatView and running other effects. Add an early-return at the top of the page component that triggers or waits for the same redirect (router.replace(`/${slug}`)) and returns null (or a minimal placeholder) so the rest of the component (ChatView render and other effects) does not execute when useV2Layout is true.frontend/src/features/ai/ChatView/ChatView.tsx (1)
191-199: Minor: Unnecessary effect re-runs frominputValuedependency.Adding
inputValueto the dependency array causes this effect to re-execute on every keystroke while the user types. Since thelastPrefillRefguard and!inputValue.trim()check prevent any action wheninputValuehas content, the effect body becomes a no-op but still runs repeatedly.Consider restructuring to avoid the
inputValuedependency:♻️ Proposed refactor to avoid unnecessary re-runs
- const lastPrefillRef = useRef<string | null>(null); + const lastPrefillRef = useRef<{ value: string | null; applied: boolean }>({ value: null, applied: false }); useEffect(() => { - if (prefillValue && prefillValue !== lastPrefillRef.current) { - lastPrefillRef.current = prefillValue; - // Only prefill if user hasn't typed anything - if (!inputValue.trim()) { - setInputValue(prefillValue); - } + if (prefillValue && prefillValue !== lastPrefillRef.current.value) { + lastPrefillRef.current = { value: prefillValue, applied: false }; } - }, [prefillValue, inputValue]); + }, [prefillValue]); + + // Apply prefill once when input is empty + useEffect(() => { + const pending = lastPrefillRef.current; + if (pending.value && !pending.applied && !inputValue.trim()) { + pending.applied = true; + setInputValue(pending.value); + } + }, [inputValue]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/features/ai/ChatView/ChatView.tsx` around lines 191 - 199, The effect watching prefillValue currently includes inputValue in its dependency array causing needless re-runs on each keystroke; remove inputValue from the dependency array and instead use a ref to read the live input value inside the effect (add or reuse an inputValueRef that you update in the input change handler where setInputValue is called). Keep the existing guards (lastPrefillRef and the trimmed-empty check) but change the check to use inputValueRef.current.trim() so the effect only reruns when prefillValue changes and still only sets setInputValue(prefillValue) when the user hasn't typed.frontend/src/stores/UIStore.ts (1)
248-254: Consider extracting size constraints as named constants.The clamping ranges (
[30, 85]for artifact panel,[15, 70]for chat column) are reasonable but could be clearer as named constants for maintainability.💡 Optional: extract constants
+const ARTIFACT_PANEL_SIZE_MIN = 30; +const ARTIFACT_PANEL_SIZE_MAX = 85; +const CHAT_COLUMN_SIZE_MIN = 15; +const CHAT_COLUMN_SIZE_MAX = 70; + setArtifactPanelSize(size: number): void { - this.artifactPanelSize = Math.max(30, Math.min(85, size)); + this.artifactPanelSize = Math.max(ARTIFACT_PANEL_SIZE_MIN, Math.min(ARTIFACT_PANEL_SIZE_MAX, size)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/stores/UIStore.ts` around lines 248 - 254, Extract the hard-coded clamp bounds in setArtifactPanelSize and setChatColumnSize into named constants (e.g., ARTIFACT_PANEL_MIN, ARTIFACT_PANEL_MAX, CHAT_COLUMN_MIN, CHAT_COLUMN_MAX) at the top of the module or class, then replace the literals 30/85 and 15/70 with those constants inside the methods so the clamping logic (this.artifactPanelSize = Math.max(ARTIFACT_PANEL_MIN, Math.min(ARTIFACT_PANEL_MAX, size)); and this.chatColumnSize = Math.max(CHAT_COLUMN_MIN, Math.min(CHAT_COLUMN_MAX, size));) is clearer and easier to maintain.frontend/src/components/layout/conversation-sidebar.tsx (1)
98-106: UUID detection heuristic is fragile but acceptable.The check
workspaceId.includes('-')attempts to distinguish UUIDs from slugs. While this works for most cases, workspace slugs with dashes (e.g., "my-workspace") would trigger false positives. Consider a proper UUID regex if this causes issues, but the current approach is pragmatic for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/layout/conversation-sidebar.tsx` around lines 98 - 106, The current heuristic in the useEffect (checking workspaceId.includes('-')) to decide when to call notificationStore.startPolling is fragile and can misclassify dashed slugs; replace that check with a proper UUID test (e.g., use a UUID regex) so the effect only starts polling for true UUID workspaceId values; update the conditional in the useEffect that references workspaceId, isAuthenticated, and notificationStore (the block that calls notificationStore.startPolling(workspaceId) and notificationStore.stopPolling()) to use the regex-based isUuid check instead of includes('-').frontend/src/hooks/useChatFirstShortcuts.ts (1)
18-72: Consider suppressing shortcuts when focus is in editable elements.Shortcuts like
Cmd+B(toggle sidebar) could conflict with expected behavior when the user is typing in a text input or contenteditable area (whereCmd+Btypically means "bold"). The handler unconditionally callspreventDefault()for all matched keys.Consider adding an early-exit check:
💡 Suggested guard for editable elements
function handleKeyDown(e: KeyboardEvent) { const isMod = e.metaKey || e.ctrlKey; if (!isMod) return; + // Don't intercept shortcuts when user is typing in editable elements + const target = e.target as HTMLElement; + const isEditable = target.tagName === 'INPUT' || + target.tagName === 'TEXTAREA' || + target.isContentEditable; + if (isEditable && ['b', 'B'].includes(e.key)) return; + // Cmd+B: Toggle sidebar🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useChatFirstShortcuts.ts` around lines 18 - 72, The keyboard handler in handleKeyDown should early-return when focus is inside editable elements to avoid overriding native shortcuts: inside the handleKeyDown function (in useChatFirstShortcuts.ts) check the event target (e.target) or document.activeElement for being an input, textarea, select, or having isContentEditable true (and optionally elements with role="textbox"), and if so return without calling preventDefault or executing shortcut actions; implement this guard at the top of handleKeyDown before any shortcut logic so commands like Cmd+B won't interfere while typing.frontend/src/hooks/useRouteArtifact.ts (1)
83-92: Layout mode transitions may override user's explicit choice.When navigating to an artifact route, this hook always sets
layoutModeto'chat-artifact'if currently'chat-first'(Line 84-86). However, if the user was in'canvas-first'mode (expanded artifact), navigating to another artifact will stay incanvas-first— which is good.But when navigating away from an artifact (Lines 87-91), if
layoutModeis'canvas-first', it still transitions to'chat-first', potentially surprising users who prefer the expanded view.Consider whether
'canvas-first'should be preserved or reset on navigation. This is a UX decision rather than a bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useRouteArtifact.ts` around lines 83 - 92, The current hook unconditionally resets uiStore.layoutMode to 'chat-first' when leaving an artifact route, which overrides users who had explicitly chosen 'canvas-first'; update the logic in the route handling (the block using uiStore.layoutMode and uiStore.setLayoutMode) so that on navigation away you only set layoutMode to 'chat-first' when the previous mode was 'chat-artifact' (or when no explicit 'canvas-first' choice exists), and preserve 'canvas-first' if uiStore.layoutMode === 'canvas-first'; ensure the same targeted check is applied when entering an artifact route (only switch to 'chat-artifact' from 'chat-first', not from 'canvas-first') so both transitions respect explicit user choice.frontend/src/components/layout/artifact-panel.tsx (1)
22-34: Potential race between this reaction anduseRouteArtifact's layout transitions.Both this
reactionand theuseRouteArtifacthook modifyuiStore.layoutMode. When a user navigates to an artifact route:
useRouteArtifactcallsartifactPanel.openTab()and setslayoutModeto'chat-artifact'- This
reactionfires whenhasOpenTabschanges and also setslayoutModeWhile both set the same value (
'chat-artifact'), the redundant transitions could cause unnecessary re-renders. More importantly, when closing the last tab:
artifactPanel.closeTab()triggershasOpenTabs→false- This reaction sets
layoutModeto'chat-first'- But if
useRouteArtifactis still on an artifact route, it might try to re-open the tabConsider whether the reaction here is needed given
useRouteArtifactalready handles transitions, or add coordination to prevent fighting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/layout/artifact-panel.tsx` around lines 22 - 34, This reaction is racing with useRouteArtifact which already toggles layoutMode; either remove this reaction entirely or add guards so it never overrides route-driven transitions: update the reaction on artifactPanel.hasOpenTabs to only call uiStore.setLayoutMode when the current route is not an artifact route (use the same route-checking state/useRouteArtifact flag used by useRouteArtifact), e.g. only set layoutMode on open/close if !isArtifactRoute or a new suppressLayoutSync flag on uiStore is false; reference artifactPanel.hasOpenTabs, uiStore.layoutMode, uiStore.setLayoutMode, and useRouteArtifact (and artifactPanel.openTab/closeTab) when implementing the guard or removal.frontend/src/components/layout/chat-first-shell.tsx (1)
60-69: Use a shadcnButtonfor the loading-hint action.Line 60-Line 69 uses a raw
<button>inside a surface that otherwise follows shadcn primitives; this can drift from design tokens/focus behavior.As per coding guidelines, "Use shadcn/ui components and design system tokens."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/layout/chat-first-shell.tsx` around lines 60 - 69, The plain <button> used for "Check AI provider settings" should be replaced with the shadcn Button primitive to preserve design tokens and focus behavior: import and use the Button component (e.g., Button) instead of the raw element, keep the onClick handler that dispatches new CustomEvent('pilot:open-settings', { detail: { tab: 'ai-providers' } }), and map the current styling/intent to the Button props (e.g., variant="link" or appropriate className) so the visual/hover/transition behavior remains consistent with other shadcn/ui components.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/layout/artifact-tab-bar.tsx`:
- Around line 90-108: The close button is nested inside the tab button (invalid
HTML); refactor the tab rendering so the outer clickable element for the tab is
not a <button> that contains the close <button>. Wrap the tab content in a
non-interactive container (e.g., a div with role="tab" / appropriate
aria-selected and tabIndex) and render the close control as a separate <button>
that calls artifactPanel.closeTab(tab.id) on click; ensure the tab activation
logic currently on the parent button is moved to the container’s
onClick/onKeyDown handlers (handle Enter/Space) and preserve the X icon
component usage and all focus/aria attributes for accessibility.
In `@frontend/src/components/layout/chat-first-shell.tsx`:
- Around line 224-242: The ResizablePanel sizes are only used as defaults
(uiStore.chatColumnSize and uiStore.artifactPanelSize) but never written back
after drag; add resize handlers on the ResizablePanelGroup/ResizablePanel to
compute the new percentage sizes and call the uiStore mutators (e.g.,
uiStore.setChatColumnSize(...) and uiStore.setArtifactPanelSize(...) or the
equivalent setters) to persist updated values so the split ratios survive
reloads; ensure you derive percentages from the panel sizes passed to the
onResize/onChange callback and store them as integers/floats matching the
existing uiStore fields.
- Around line 163-165: The live region only announces the "opened" state; change
it to announce both open and close by computing an announcement string from the
artifact panel visibility and rendering that in the aria-live region. In the
ChatFirstShell component, watch showArtifactPanel and isRouteArtifact (e.g.,
with a useEffect or a usePrevious hook) and set a local announcement state like
artifactAnnouncement to "Artifact panel opened" when showArtifactPanel &&
isRouteArtifact and to "Artifact panel closed" when the panel becomes not
visible; render artifactAnnouncement inside the div with aria-live="polite" so
both open and close events are announced to assistive tech.
In `@frontend/src/components/layout/conversation-sidebar.tsx`:
- Around line 213-219: Replace the inline window.confirm call inside
DropdownMenuItem's onSelect with shadcn/ui's AlertDialog: change
DropdownMenuItem (the trigger) to open an AlertDialog (use AlertDialog,
AlertDialogTrigger, AlertDialogContent, AlertDialogHeader, AlertDialogFooter,
AlertDialogAction, AlertDialogCancel), move the confirm prompt into
AlertDialogContent, and call sessionListStore.deleteSession(session.sessionId)
from the AlertDialogAction handler; ensure the AlertDialog has proper
aria-label/heading text, keyboard-focusable buttons (Cancel and Delete), and
uses the project's design tokens/classes for destructive styling so the dialog
is accessible and visually consistent.
- Around line 147-154: The search <input> currently relies only on placeholder
text and lacks an accessible name; update the input (the element with
placeholder="Search conversations..." and className="flex-1 bg-transparent ...")
to provide an accessible name—either add aria-label="Search conversations" or
wire up aria-labelledby to a visible label—so screen readers announce its
purpose (this change does not affect the existing onChange that calls
sessionListStore?.searchSessions).
In `@frontend/src/features/ai/ChatView/ChatEmptyState.tsx`:
- Around line 184-202: The Quick Actions nav builds hrefs using workspaceSlug
which can be empty and produce protocol-relative URLs like "//notes"; update the
Link href construction in ChatEmptyState (where QUICK_ACTIONS are mapped) to
guard against an empty workspaceSlug by conditionally constructing the path—use
`/${workspaceSlug}/${path}` only when workspaceSlug is non-empty, otherwise use
`/${path}` (or otherwise sanitize/join segments) so generated hrefs never start
with a double slash.
- Around line 126-128: The greeting construction in ChatEmptyState.tsx uses
userName.split(' ')[0] which yields an empty string for empty or whitespace-only
userName and produces "Good morning, ."; update the logic that builds greeting
(the greeting constant that uses getGreeting() and userName) to defensively trim
userName and check for emptiness (e.g., const firstName = userName?.trim() ?
userName.trim().split(' ')[0] : undefined) and only include the ", {firstName}."
segment when firstName is non-empty so greetings become "Good morning." for
blank names and "Good morning, Alice." for valid names.
In `@frontend/src/stores/ArtifactPanelStore.ts`:
- Around line 94-105: closeTab currently removes the tab from openTabs and
pinnedTabIds but leaves any references in the navigation history, so
goBack()/goForward() can land on closed IDs; update closeTab (and the same logic
around the close flow at the other block referenced) to also remove the closed
tabId from whatever navigation stacks/state you use (e.g., back/forward stacks
or a single history array) and adjust activeTabId selection as before —
specifically, after this.openTabs.splice(...) and
this.pinnedTabIds.delete(tabId) remove all occurrences of tabId from the history
structures (e.g., this.backStack, this.forwardStack or this.history) so
navigation never returns to a closed tab, and ensure if the activeTabId was set
to a pruned id you re-evaluate it to prev?.id ?? null.
- Around line 107-111: The setActiveTab method currently sets this.activeTabId
directly (in ArtifactPanelStore.setActiveTab) which bypasses the navigation
history and prevents goBack()/goForward() from seeing manual tab switches;
change setActiveTab so that after validating the tab exists
(this.openTabs.some(...)) it calls the existing pushHistory method (or the
appropriate history-pushing helper used elsewhere) with the new tabId instead of
directly assigning this.activeTabId, ensuring pushHistory records the tab change
so goBack/goForward will work with manual tab activations.
---
Nitpick comments:
In `@frontend/src/app/`(workspace)/[workspaceSlug]/chat/page.tsx:
- Around line 27-33: When layout_v2 is enabled the component still continues
rendering and running effects while the redirect effect (useEffect with
router.replace) runs; short-circuit the component when
workspaceStore.isFeatureEnabled('layout_v2') is true (and slug exists) to avoid
rendering ChatView and running other effects. Add an early-return at the top of
the page component that triggers or waits for the same redirect
(router.replace(`/${slug}`)) and returns null (or a minimal placeholder) so the
rest of the component (ChatView render and other effects) does not execute when
useV2Layout is true.
In `@frontend/src/components/layout/artifact-panel.tsx`:
- Around line 22-34: This reaction is racing with useRouteArtifact which already
toggles layoutMode; either remove this reaction entirely or add guards so it
never overrides route-driven transitions: update the reaction on
artifactPanel.hasOpenTabs to only call uiStore.setLayoutMode when the current
route is not an artifact route (use the same route-checking
state/useRouteArtifact flag used by useRouteArtifact), e.g. only set layoutMode
on open/close if !isArtifactRoute or a new suppressLayoutSync flag on uiStore is
false; reference artifactPanel.hasOpenTabs, uiStore.layoutMode,
uiStore.setLayoutMode, and useRouteArtifact (and artifactPanel.openTab/closeTab)
when implementing the guard or removal.
In `@frontend/src/components/layout/chat-first-shell.tsx`:
- Around line 60-69: The plain <button> used for "Check AI provider settings"
should be replaced with the shadcn Button primitive to preserve design tokens
and focus behavior: import and use the Button component (e.g., Button) instead
of the raw element, keep the onClick handler that dispatches new
CustomEvent('pilot:open-settings', { detail: { tab: 'ai-providers' } }), and map
the current styling/intent to the Button props (e.g., variant="link" or
appropriate className) so the visual/hover/transition behavior remains
consistent with other shadcn/ui components.
In `@frontend/src/components/layout/conversation-sidebar.tsx`:
- Around line 98-106: The current heuristic in the useEffect (checking
workspaceId.includes('-')) to decide when to call notificationStore.startPolling
is fragile and can misclassify dashed slugs; replace that check with a proper
UUID test (e.g., use a UUID regex) so the effect only starts polling for true
UUID workspaceId values; update the conditional in the useEffect that references
workspaceId, isAuthenticated, and notificationStore (the block that calls
notificationStore.startPolling(workspaceId) and notificationStore.stopPolling())
to use the regex-based isUuid check instead of includes('-').
In `@frontend/src/features/ai/ChatView/ChatView.tsx`:
- Around line 191-199: The effect watching prefillValue currently includes
inputValue in its dependency array causing needless re-runs on each keystroke;
remove inputValue from the dependency array and instead use a ref to read the
live input value inside the effect (add or reuse an inputValueRef that you
update in the input change handler where setInputValue is called). Keep the
existing guards (lastPrefillRef and the trimmed-empty check) but change the
check to use inputValueRef.current.trim() so the effect only reruns when
prefillValue changes and still only sets setInputValue(prefillValue) when the
user hasn't typed.
In `@frontend/src/hooks/useChatFirstShortcuts.ts`:
- Around line 18-72: The keyboard handler in handleKeyDown should early-return
when focus is inside editable elements to avoid overriding native shortcuts:
inside the handleKeyDown function (in useChatFirstShortcuts.ts) check the event
target (e.target) or document.activeElement for being an input, textarea,
select, or having isContentEditable true (and optionally elements with
role="textbox"), and if so return without calling preventDefault or executing
shortcut actions; implement this guard at the top of handleKeyDown before any
shortcut logic so commands like Cmd+B won't interfere while typing.
In `@frontend/src/hooks/useRouteArtifact.ts`:
- Around line 83-92: The current hook unconditionally resets uiStore.layoutMode
to 'chat-first' when leaving an artifact route, which overrides users who had
explicitly chosen 'canvas-first'; update the logic in the route handling (the
block using uiStore.layoutMode and uiStore.setLayoutMode) so that on navigation
away you only set layoutMode to 'chat-first' when the previous mode was
'chat-artifact' (or when no explicit 'canvas-first' choice exists), and preserve
'canvas-first' if uiStore.layoutMode === 'canvas-first'; ensure the same
targeted check is applied when entering an artifact route (only switch to
'chat-artifact' from 'chat-first', not from 'canvas-first') so both transitions
respect explicit user choice.
In `@frontend/src/stores/UIStore.ts`:
- Around line 248-254: Extract the hard-coded clamp bounds in
setArtifactPanelSize and setChatColumnSize into named constants (e.g.,
ARTIFACT_PANEL_MIN, ARTIFACT_PANEL_MAX, CHAT_COLUMN_MIN, CHAT_COLUMN_MAX) at the
top of the module or class, then replace the literals 30/85 and 15/70 with those
constants inside the methods so the clamping logic (this.artifactPanelSize =
Math.max(ARTIFACT_PANEL_MIN, Math.min(ARTIFACT_PANEL_MAX, size)); and
this.chatColumnSize = Math.max(CHAT_COLUMN_MIN, Math.min(CHAT_COLUMN_MAX,
size));) is clearer and easier to maintain.
🪄 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: 35ef7c9c-640c-4987-b105-4702786e47ad
📒 Files selected for processing (19)
frontend/src/app/(workspace)/[workspaceSlug]/chat/page.tsxfrontend/src/app/(workspace)/[workspaceSlug]/issues/[issueId]/page.tsxfrontend/src/app/(workspace)/[workspaceSlug]/page.tsxfrontend/src/app/(workspace)/layout.tsxfrontend/src/components/layout/artifact-panel.tsxfrontend/src/components/layout/artifact-tab-bar.tsxfrontend/src/components/layout/chat-first-shell.tsxfrontend/src/components/layout/conversation-sidebar.tsxfrontend/src/components/layout/index.tsfrontend/src/features/ai/ChatView/ChatEmptyState.tsxfrontend/src/features/ai/ChatView/ChatView.tsxfrontend/src/hooks/useChatFirstShortcuts.tsfrontend/src/hooks/useRouteArtifact.tsfrontend/src/stores/ArtifactPanelStore.tsfrontend/src/stores/RootStore.tsfrontend/src/stores/UIStore.tsfrontend/src/stores/ai/PilotSpaceStreamHandler.tsfrontend/src/stores/index.tsfrontend/src/types/workspace.ts
Comment on lines
+90
to
+108
| <button | ||
| type="button" | ||
| tabIndex={0} | ||
| aria-label={`Close ${tab.title}`} | ||
| className={cn( | ||
| 'inline-flex items-center justify-center rounded-sm', | ||
| 'h-5 w-5 min-h-[20px] min-w-[20px]', | ||
| 'opacity-0 group-hover:opacity-100 group-focus-within:opacity-100', | ||
| 'focus:opacity-100', | ||
| 'transition-opacity hover:bg-accent-foreground/10', | ||
| 'focus-visible:outline-none focus-visible:ring-1 focus-visible:ring-ring' | ||
| )} | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| artifactPanel.closeTab(tab.id); | ||
| }} | ||
| > | ||
| <X className="h-3 w-3" /> | ||
| </button> |
There was a problem hiding this comment.
Nested interactive elements: button inside button is invalid HTML.
The close button (Lines 90-108) is nested inside the tab button (Lines 72-109). Nested interactive elements are invalid HTML and violate WCAG 2.2 requirements. Screen readers and keyboard navigation will behave unpredictably.
🐛 Proposed fix: restructure tab as a container with separate button and close button
- <button
- key={tab.id}
- type="button"
- role="tab"
- aria-selected={isActive}
- tabIndex={isActive ? 0 : -1}
- className={cn(
- 'group flex items-center gap-1.5 rounded-md px-2.5 py-1.5 text-xs transition-colors',
- 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring',
- isActive
- ? 'bg-accent text-accent-foreground font-medium'
- : 'text-muted-foreground hover:bg-accent/50 hover:text-foreground'
- )}
- onClick={() => artifactPanel.setActiveTab(tab.id)}
- onKeyDown={(e) => handleKeyDown(e, index)}
- >
- <span className="truncate max-w-[120px]">{tab.title}</span>
- {tab.isPinned && <Pin className="h-2.5 w-2.5 text-muted-foreground" />}
- <button
- type="button"
- tabIndex={0}
- aria-label={`Close ${tab.title}`}
+ <div
+ key={tab.id}
+ role="tab"
+ aria-selected={isActive}
+ tabIndex={isActive ? 0 : -1}
+ className={cn(
+ 'group flex items-center gap-1.5 rounded-md text-xs transition-colors',
+ 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring',
+ isActive
+ ? 'bg-accent text-accent-foreground font-medium'
+ : 'text-muted-foreground hover:bg-accent/50 hover:text-foreground'
+ )}
+ onKeyDown={(e) => handleKeyDown(e, index)}
+ onClick={() => artifactPanel.setActiveTab(tab.id)}
+ >
+ <span className="truncate max-w-[120px] px-2.5 py-1.5">{tab.title}</span>
+ {tab.isPinned && <Pin className="h-2.5 w-2.5 text-muted-foreground" />}
+ <button
+ type="button"
+ tabIndex={-1}
+ aria-label={`Close ${tab.title}`}
className={cn(
- 'inline-flex items-center justify-center rounded-sm',
+ 'inline-flex items-center justify-center rounded-sm mr-1',
'h-5 w-5 min-h-[20px] min-w-[20px]',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/layout/artifact-tab-bar.tsx` around lines 90 - 108,
The close button is nested inside the tab button (invalid HTML); refactor the
tab rendering so the outer clickable element for the tab is not a <button> that
contains the close <button>. Wrap the tab content in a non-interactive container
(e.g., a div with role="tab" / appropriate aria-selected and tabIndex) and
render the close control as a separate <button> that calls
artifactPanel.closeTab(tab.id) on click; ensure the tab activation logic
currently on the parent button is moved to the container’s onClick/onKeyDown
handlers (handle Enter/Space) and preserve the X icon component usage and all
focus/aria attributes for accessibility.
Comment on lines
+163
to
+165
| <div aria-live="polite" className="sr-only"> | ||
| {showArtifactPanel && isRouteArtifact ? 'Artifact panel opened' : ''} | ||
| </div> |
There was a problem hiding this comment.
Announce both open and close states in the live region.
Line 164 only announces the “opened” state. Closing the artifact panel is silent for assistive tech users.
As per coding guidelines, "Ensure accessibility is WCAG 2.2 AA baseline. Never use color as sole meaning carrier. Implement keyboard-first interaction patterns."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/layout/chat-first-shell.tsx` around lines 163 - 165,
The live region only announces the "opened" state; change it to announce both
open and close by computing an announcement string from the artifact panel
visibility and rendering that in the aria-live region. In the ChatFirstShell
component, watch showArtifactPanel and isRouteArtifact (e.g., with a useEffect
or a usePrevious hook) and set a local announcement state like
artifactAnnouncement to "Artifact panel opened" when showArtifactPanel &&
isRouteArtifact and to "Artifact panel closed" when the panel becomes not
visible; render artifactAnnouncement inside the div with aria-live="polite" so
both open and close events are announced to assistive tech.
Comment on lines
+224
to
+242
| <ResizablePanelGroup orientation="horizontal" className="flex-1"> | ||
| <ResizablePanel | ||
| id="chat-column" | ||
| defaultSize={`${uiStore.chatColumnSize}%`} | ||
| minSize="15%" | ||
| className="min-w-0" | ||
| > | ||
| <div className="h-full overflow-hidden"> | ||
| {chatViewElement} | ||
| </div> | ||
| </ResizablePanel> | ||
|
|
||
| <ResizableHandle withHandle /> | ||
|
|
||
| <ResizablePanel | ||
| id="artifact-panel" | ||
| defaultSize={`${uiStore.artifactPanelSize}%`} | ||
| minSize="30%" | ||
| className="min-w-0" |
There was a problem hiding this comment.
Resizable panel sizes are read but not persisted after user drag.
Line 227 and Line 240 pull uiStore.chatColumnSize / uiStore.artifactPanelSize as defaults, but there is no resize callback here to write updated sizes back to the store. This can cause split ratios to reset on reload.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/layout/chat-first-shell.tsx` around lines 224 - 242,
The ResizablePanel sizes are only used as defaults (uiStore.chatColumnSize and
uiStore.artifactPanelSize) but never written back after drag; add resize
handlers on the ResizablePanelGroup/ResizablePanel to compute the new percentage
sizes and call the uiStore mutators (e.g., uiStore.setChatColumnSize(...) and
uiStore.setArtifactPanelSize(...) or the equivalent setters) to persist updated
values so the split ratios survive reloads; ensure you derive percentages from
the panel sizes passed to the onResize/onChange callback and store them as
integers/floats matching the existing uiStore fields.
Comment on lines
+147
to
+154
| <input | ||
| type="text" | ||
| placeholder="Search conversations..." | ||
| className="flex-1 bg-transparent text-xs text-sidebar-foreground placeholder:text-muted-foreground/50 outline-none" | ||
| onChange={(e) => { | ||
| sessionListStore?.searchSessions(e.target.value); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Search input lacks accessible name (WCAG 2.2 AA violation).
The search input relies solely on placeholder text. Per WCAG, placeholder is not a substitute for a label. Screen reader users won't hear the input's purpose until they're inside it.
🛡️ Proposed fix: add aria-label
<input
type="text"
placeholder="Search conversations..."
+ aria-label="Search conversations"
className="flex-1 bg-transparent text-xs text-sidebar-foreground placeholder:text-muted-foreground/50 outline-none"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/layout/conversation-sidebar.tsx` around lines 147 -
154, The search <input> currently relies only on placeholder text and lacks an
accessible name; update the input (the element with placeholder="Search
conversations..." and className="flex-1 bg-transparent ...") to provide an
accessible name—either add aria-label="Search conversations" or wire up
aria-labelledby to a visible label—so screen readers announce its purpose (this
change does not affect the existing onChange that calls
sessionListStore?.searchSessions).
Comment on lines
+213
to
+219
| <DropdownMenuItem | ||
| className="text-xs gap-2 text-destructive focus:text-destructive" | ||
| onSelect={() => { | ||
| if (confirm('Delete this conversation?')) { | ||
| sessionListStore?.deleteSession(session.sessionId); | ||
| } | ||
| }} |
There was a problem hiding this comment.
Replace window.confirm with an accessible AlertDialog.
window.confirm() is not keyboard-accessible and doesn't match the app's design system. WCAG 2.2 AA requires accessible confirmation dialogs. Use shadcn/ui's AlertDialog component instead.
As per coding guidelines: "Use shadcn/ui components and design system tokens" and "Ensure accessibility is WCAG 2.2 AA baseline."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/layout/conversation-sidebar.tsx` around lines 213 -
219, Replace the inline window.confirm call inside DropdownMenuItem's onSelect
with shadcn/ui's AlertDialog: change DropdownMenuItem (the trigger) to open an
AlertDialog (use AlertDialog, AlertDialogTrigger, AlertDialogContent,
AlertDialogHeader, AlertDialogFooter, AlertDialogAction, AlertDialogCancel),
move the confirm prompt into AlertDialogContent, and call
sessionListStore.deleteSession(session.sessionId) from the AlertDialogAction
handler; ensure the AlertDialog has proper aria-label/heading text,
keyboard-focusable buttons (Cancel and Delete), and uses the project's design
tokens/classes for destructive styling so the dialog is accessible and visually
consistent.
Comment on lines
+126
to
+128
| const greeting = userName | ||
| ? `${getGreeting()}, ${userName.split(' ')[0]}.` | ||
| : `${getGreeting()}.`; |
There was a problem hiding this comment.
Handle edge case for empty or whitespace-only userName.
If userName is an empty string or contains only whitespace, userName.split(' ')[0] returns "", producing a greeting like "Good morning, ." with a trailing period.
💡 Suggested fix
const greeting = userName
- ? `${getGreeting()}, ${userName.split(' ')[0]}.`
+ ? `${getGreeting()}, ${userName.split(' ')[0] || userName}.`
: `${getGreeting()}.`;Or more defensively:
+const firstName = userName?.trim().split(' ')[0];
const greeting = userName
- ? `${getGreeting()}, ${userName.split(' ')[0]}.`
+ ? firstName ? `${getGreeting()}, ${firstName}.` : `${getGreeting()}.`
: `${getGreeting()}.`;📝 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 greeting = userName | |
| ? `${getGreeting()}, ${userName.split(' ')[0]}.` | |
| : `${getGreeting()}.`; | |
| const greeting = userName | |
| ? `${getGreeting()}, ${userName.split(' ')[0] || userName}.` | |
| : `${getGreeting()}.`; |
Suggested change
| const greeting = userName | |
| ? `${getGreeting()}, ${userName.split(' ')[0]}.` | |
| : `${getGreeting()}.`; | |
| const firstName = userName?.trim().split(' ')[0]; | |
| const greeting = userName | |
| ? firstName ? `${getGreeting()}, ${firstName}.` : `${getGreeting()}.` | |
| : `${getGreeting()}.`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/ai/ChatView/ChatEmptyState.tsx` around lines 126 - 128,
The greeting construction in ChatEmptyState.tsx uses userName.split(' ')[0]
which yields an empty string for empty or whitespace-only userName and produces
"Good morning, ."; update the logic that builds greeting (the greeting constant
that uses getGreeting() and userName) to defensively trim userName and check for
emptiness (e.g., const firstName = userName?.trim() ? userName.trim().split('
')[0] : undefined) and only include the ", {firstName}." segment when firstName
is non-empty so greetings become "Good morning." for blank names and "Good
morning, Alice." for valid names.
Comment on lines
+184
to
+202
| {sidebarCollapsed && !artifactContext && ( | ||
| <nav aria-label="Quick navigation" className="flex items-center justify-center gap-2"> | ||
| {QUICK_ACTIONS.map(({ label, path, icon: Icon }) => ( | ||
| <Link | ||
| key={label} | ||
| href={`/${workspaceSlug}/${path}`} | ||
| className={cn( | ||
| 'flex items-center gap-1.5 rounded-lg px-3 py-1.5', | ||
| 'text-xs font-medium text-muted-foreground', | ||
| 'transition-colors hover:bg-accent hover:text-foreground', | ||
| 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring' | ||
| )} | ||
| > | ||
| <Icon className="h-3.5 w-3.5" /> | ||
| {label} | ||
| </Link> | ||
| ))} | ||
| </nav> | ||
| )} |
There was a problem hiding this comment.
Guard against empty workspaceSlug to prevent invalid URLs.
If workspaceSlug is empty (e.g., when pathname is "/" or user is on a non-workspace route), href={\/${workspaceSlug}/${path}`}produces"//notes"`, which browsers interpret as a protocol-relative URL. This could potentially redirect to an external domain if one exists.
🛡️ Suggested guard
- {sidebarCollapsed && !artifactContext && (
+ {sidebarCollapsed && !artifactContext && workspaceSlug && (
<nav aria-label="Quick navigation" className="flex items-center justify-center gap-2">📝 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
| {sidebarCollapsed && !artifactContext && ( | |
| <nav aria-label="Quick navigation" className="flex items-center justify-center gap-2"> | |
| {QUICK_ACTIONS.map(({ label, path, icon: Icon }) => ( | |
| <Link | |
| key={label} | |
| href={`/${workspaceSlug}/${path}`} | |
| className={cn( | |
| 'flex items-center gap-1.5 rounded-lg px-3 py-1.5', | |
| 'text-xs font-medium text-muted-foreground', | |
| 'transition-colors hover:bg-accent hover:text-foreground', | |
| 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring' | |
| )} | |
| > | |
| <Icon className="h-3.5 w-3.5" /> | |
| {label} | |
| </Link> | |
| ))} | |
| </nav> | |
| )} | |
| {sidebarCollapsed && !artifactContext && workspaceSlug && ( | |
| <nav aria-label="Quick navigation" className="flex items-center justify-center gap-2"> | |
| {QUICK_ACTIONS.map(({ label, path, icon: Icon }) => ( | |
| <Link | |
| key={label} | |
| href={`/${workspaceSlug}/${path}`} | |
| className={cn( | |
| 'flex items-center gap-1.5 rounded-lg px-3 py-1.5', | |
| 'text-xs font-medium text-muted-foreground', | |
| 'transition-colors hover:bg-accent hover:text-foreground', | |
| 'focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring' | |
| )} | |
| > | |
| <Icon className="h-3.5 w-3.5" /> | |
| {label} | |
| </Link> | |
| ))} | |
| </nav> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/features/ai/ChatView/ChatEmptyState.tsx` around lines 184 - 202,
The Quick Actions nav builds hrefs using workspaceSlug which can be empty and
produce protocol-relative URLs like "//notes"; update the Link href construction
in ChatEmptyState (where QUICK_ACTIONS are mapped) to guard against an empty
workspaceSlug by conditionally constructing the path—use
`/${workspaceSlug}/${path}` only when workspaceSlug is non-empty, otherwise use
`/${path}` (or otherwise sanitize/join segments) so generated hrefs never start
with a double slash.
Comment on lines
+94
to
+105
| closeTab(tabId: string): void { | ||
| const index = this.openTabs.findIndex((t) => t.id === tabId); | ||
| if (index === -1) return; | ||
|
|
||
| this.openTabs.splice(index, 1); | ||
| this.pinnedTabIds.delete(tabId); | ||
|
|
||
| if (this.activeTabId === tabId) { | ||
| const prev = this.openTabs[Math.max(0, index - 1)]; | ||
| this.activeTabId = prev?.id ?? null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Closed tabs remain in history, creating dead navigation entries.
After Line 98 and Line 130, history is not pruned. goBack()/goForward() can land on closed-tab IDs, causing apparent no-op navigation.
Proposed fix
+ private pruneHistoryToOpenTabs(): void {
+ const openIds = new Set(this.openTabs.map((t) => t.id));
+ this.history = this.history.filter((id) => openIds.has(id));
+ this.historyIndex = Math.min(this.historyIndex, this.history.length - 1);
+ if (this.history.length === 0) this.historyIndex = -1;
+ }
closeTab(tabId: string): void {
@@
this.openTabs.splice(index, 1);
this.pinnedTabIds.delete(tabId);
+ this.pruneHistoryToOpenTabs();
@@
closeAllUnpinned(): void {
this.openTabs = this.openTabs.filter((t) => this.pinnedTabIds.has(t.id));
+ this.pruneHistoryToOpenTabs();
if (this.activeTabId && !this.openTabs.some((t) => t.id === this.activeTabId)) {
this.activeTabId = this.openTabs[0]?.id ?? null;
}
}Also applies to: 129-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/stores/ArtifactPanelStore.ts` around lines 94 - 105, closeTab
currently removes the tab from openTabs and pinnedTabIds but leaves any
references in the navigation history, so goBack()/goForward() can land on closed
IDs; update closeTab (and the same logic around the close flow at the other
block referenced) to also remove the closed tabId from whatever navigation
stacks/state you use (e.g., back/forward stacks or a single history array) and
adjust activeTabId selection as before — specifically, after
this.openTabs.splice(...) and this.pinnedTabIds.delete(tabId) remove all
occurrences of tabId from the history structures (e.g., this.backStack,
this.forwardStack or this.history) so navigation never returns to a closed tab,
and ensure if the activeTabId was set to a pruned id you re-evaluate it to
prev?.id ?? null.
Comment on lines
+107
to
+111
| setActiveTab(tabId: string): void { | ||
| if (this.openTabs.some((t) => t.id === tabId)) { | ||
| this.activeTabId = tabId; | ||
| } | ||
| } |
There was a problem hiding this comment.
setActiveTab bypasses history, so back/forward misses manual tab switches.
Line 109 updates activeTabId without calling pushHistory, which makes tab-bar activation invisible to goBack() / goForward().
Proposed fix
setActiveTab(tabId: string): void {
- if (this.openTabs.some((t) => t.id === tabId)) {
+ if (this.openTabs.some((t) => t.id === tabId) && this.activeTabId !== tabId) {
this.activeTabId = tabId;
+ this.pushHistory(tabId);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/stores/ArtifactPanelStore.ts` around lines 107 - 111, The
setActiveTab method currently sets this.activeTabId directly (in
ArtifactPanelStore.setActiveTab) which bypasses the navigation history and
prevents goBack()/goForward() from seeing manual tab switches; change
setActiveTab so that after validating the tab exists (this.openTabs.some(...))
it calls the existing pushHistory method (or the appropriate history-pushing
helper used elsewhere) with the new tabId instead of directly assigning
this.activeTabId, ensuring pushHistory records the tab change so
goBack/goForward will work with manual tab activations.
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
Redesigns PilotSpace from a traditional sidebar-nav + page-based layout to a Claude.ai-inspired chat-first model behind the
layout_v2feature flag (defaults to OFF — zero visible change when flag is off).When enabled, chat becomes the primary surface. Notes, issues, and projects appear in a right-side artifact panel alongside the persistent chat. The "Note-First" paradigm is preserved — users can expand any artifact to full width for deep work.
Before → After
What changed
New components (9 files, +1426 lines)
ChatFirstShellAppShell— persistent ChatView + artifact panel + responsive sidebarConversationSidebarArtifactPanelArtifactTabBarChatEmptyStateArtifactPanelStoreuseRouteArtifactuseChatFirstShortcutsAILoadingSkeletonModified files (10 files)
UIStorelayoutMode,artifactPanelSize,chatColumnSize(persisted to localStorage)RootStoreArtifactPanelStorewiring +useArtifactPanelStore()hookstores/index.tsworkspace.tslayout_v2: booleanfeature toggle (defaultfalse)layout/index.tsChatFirstShellexport(workspace)/layout.tsxChatFirstShellvsAppShellbased onlayout_v2[workspaceSlug]/page.tsxlayout_v2on (shell owns ChatView)[workspaceSlug]/chat/page.tsxlayout_v2on[issueId]/page.tsxissueTitle+issueStatustosetIssueContextChatView.tsxpersistentModeprop, auto-send prompts, prefill protectionPilotSpaceStreamHandler.tsLayout modes
Three review rounds incorporated
Keyboard shortcuts (new)
Cmd+BCmd+Shift+CCmd+/Cmd+Shift+NCmd+[Cmd+]Accessibility
tablist+tabroles with keyboard navigation (Arrow, Home, End)aria-live="polite"announces layout transitions for screen readersaria-label<p>inside<button>)How to test
layout_v2: truein workspace feature toggles (or temporarily inDEFAULT_FEATURE_TOGGLESinfrontend/src/types/workspace.ts)layout_v2— should see the original AppShell layout unchangedTest plan
pnpm type-check— 0 errorspnpm lint— 0 errors (22 pre-existing warnings)pnpm test src/stores/__tests__/UIStore.test.ts— 12/12 passSummary by CodeRabbit