feat(frontend): redesign session detail page — remove sidebar, add explorer panel and shared layout#939
Merged
Gkrumbach07 merged 2 commits intomainfrom Mar 17, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFrontend: large UI refactor adding ProjectLayout, collapsible SessionsSidebar, explorer (ExplorerPanel, FilesTab, ContextTab), tabbed content (ContentTabs), FileViewer, session settings, new session creation pages/components/hooks and many tests. Backend: CreateSession now triggers async display-name generation when InitialPrompt exists and DisplayName is empty. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ProjectLayout
participant SessionPage
participant ExplorerPanel
participant FileViewer
participant Chat
participant SettingsModal
User->>ProjectLayout: Navigate to /projects/[name]/sessions/[sessionName]
ProjectLayout->>SessionPage: render with sidebar and controls
SessionPage->>ExplorerPanel: init useExplorerState and register handlers
User->>ExplorerPanel: Open Files tab and click file
ExplorerPanel->>SessionPage: onFileOpen(filePath)
SessionPage->>FileViewer: open file tab and fetch content
FileViewer->>FileViewer: fetch file and highlight code
User->>SessionPage: Switch to Chat tab (ContentTabs)
SessionPage->>Chat: render chat UI and input toolbar (workflowSlot, repo/upload actions)
User->>SessionPage: Click Settings
SessionPage->>SettingsModal: onOpenSettings()
SettingsModal->>SettingsModal: load session details / MCP / integrations
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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 unit tests (beta)
📝 Coding Plan
Comment |
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
4 times, most recently
from
2123bd9 to
a4948b5
Compare
…plorer panel and shared layout Remove the left accordion sidebar from the session detail page and redistribute its contents into more natural UI surfaces: - Sessions sidebar (workspace nav + recents) in a shared Next.js layout - Right-side Explorer panel with Files + Context tabs (repos/uploads separated) - Settings modal (Session/MCP/Integrations tabs) via kebab menu - Workflow selector in the chat input toolbar - File tab system with syntax-highlighted viewer - New session page with runner/model selector and repo support - Split nav header (branding in sidebar, actions in content header) - Chat input restyled with unified bordered container RHOAIENG-53563 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
from
a4948b5 to
2e488f9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx (1)
1061-1121: 🧹 Nitpick | 🔵 TrivialConsider extracting the initial prompt logic to reduce complexity.
The
streamMessagesmemo now includes logic to prepend a synthetic user message forinitialPrompt. While functional, this adds cognitive load to an already complex memo. Consider extracting the initial prompt handling to a separate helper or early-return pattern for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx around lines 1061 - 1121, Extract the initialPrompt detection and synthetic message creation out of the streamMessages useMemo: create a small helper (e.g., getSyntheticInitialUserMessage or computeInitialPromptMessage) that takes session?.spec?.initialPrompt, session?.metadata?.creationTimestamp and committedStreamMessages and returns either the synthetic user_message object or null by checking the same hasInitialUserMessage logic currently inline; then simplify streamMessages to just concat the returned synthetic message (if any), committedStreamMessages and the agent streaming blocks. Ensure you wire the helper's return into the useMemo dependencies instead of repeating the initialPrompt logic inside streamMessages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/sessions.go`:
- Around line 900-902: The code is currently doing a direct assertion on
created.Object["spec"] which can panic if the shape is unexpected; change the
block around created.Object["spec"] so it safely type-checks (use the two-value
assertion pattern) and only call ExtractSessionContext and
GenerateDisplayNameAsync when the assertion succeeds; if the spec is missing or
not the expected map[string]interface{} pass a nil/empty context to
ExtractSessionContext or skip it and still call GenerateDisplayNameAsync as a
best-effort asynchronous task (do not return or panic on failure). Ensure you
reference created.Object["spec"], ExtractSessionContext,
GenerateDisplayNameAsync, project, name and req.InitialPrompt when applying the
guard.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx:
- Around line 6-10: The mock button components used in the test (notably the
RunnerModelSelector mock that renders the button with
data-testid="runner-model-selector" and any similar mock button at the other
location) are missing an explicit type attribute and may default to submit in
form contexts; update those mock/button JSX to include type="button" on the
<button> elements so they never trigger form submits (preserve existing onClick
handlers and data-testid values when adding the attribute).
- Around line 61-63: The test currently assumes the submit button is the last
DOM button by using screen.getAllByRole('button') and indexing the last element;
change this to a stable selector for the submit control (e.g., use
screen.getByRole('button', { name: /submit/i }) or add a data-testid like
"submit-button" and use screen.getByTestId('submit-button')) in the
new-session-view.test.tsx so the assertion targets the specific Submit button
rather than DOM order; update the expect to assert the disabled state on that
element.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/__tests__/runner-model-selector.test.tsx:
- Line 25: The mock function mockUseRunnerTypes declares an unused parameter
(_projectName) which triggers `@typescript-eslint/no-unused-vars`; remove the
unused parameter by changing the stub to no-args (e.g., vi.fn(() => ({ data:
mockRunnerTypes }))) so mockUseRunnerTypes no longer defines or references
_projectName.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsx:
- Around line 78-81: The test currently finds the close button by index using
screen.getAllByRole('button') and taking the last element (in the
explorer-panel.test.tsx test), which is brittle; replace that positional lookup
with an accessible query that matches role plus accessible name (e.g., use
screen.getByRole('button', { name: /close/i }) or getByLabelText/getByTitle as
appropriate) and then call fireEvent.click on that result; update any references
to closeButton in the test accordingly so the selector targets the button by its
accessible label instead of array position.
- Around line 35-37: The test fixture uses the wrong prop name: replace the
stale onAddContext mock with the correct onAddRepository mock used by the
component (update the object keys in the test setup where onAddContext appears),
and adjust any related expectations or calls in explorer-panel.test.tsx to
reference onAddRepository instead so the Context tab action is wired to the real
integration path.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/context-tab.tsx:
- Around line 127-136: The button rendering the expand/collapse control (the
element using isExpanded, toggleExpanded and rendering ChevronDown/ChevronRight)
lacks an explicit type and will default to type="submit", which can trigger form
submissions; update that <button> to include type="button" to prevent unintended
form submission when this component is used inside a form.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsx:
- Around line 76-99: The two tab buttons that call onTabChange("files") and
onTabChange("context") (in explorer-panel.tsx) need an explicit type attribute
to avoid accidental form submission; add type="button" to each <button> element
(the ones rendering FolderOpen and Link and invoking onTabChange) so they behave
as non-submitting buttons.
- Around line 42-43: The ExplorerPanel component is destructuring a `visible`
prop that isn't used; remove `visible` from the ExplorerPanel parameter and from
any associated props/type definition (e.g., Props or interface used to type the
component) so the prop is no longer expected, and update any call sites that
pass `visible` to stop sending it (or, if you intended to use it, implement
conditional rendering inside ExplorerPanel using `visible` to show/hide the
panel); ensure exports and types remain consistent after removal.
- Around line 29-39: ExplorerPanelProps declares onUploadFile twice causing a
TypeScript conflict; rename the Context-tab upload prop to a unique name (e.g.,
onUploadContextFile or onUploadRepositoryFile) in the interface and update any
corresponding destructuring/uses in ExplorerPanel and its Context tab code
(search for ExplorerPanelProps, onUploadFile, onAddRepository, uploadedFiles,
onRemoveRepository) so the original onUploadFile remains for the main upload
action and the new name is used for the context/upload-in-repositories flow.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/files-tab.tsx:
- Around line 76-84: The handler rebuilds the file path from node.name causing
wrong targets for nodes whose path !== name; update handleSelect to use the
node.path property when opening files (call onFileOpen(node.path) for file
nodes) instead of composing a path from selectedDirectory/currentSubPath and
node.name; keep onFileOrFolderSelect(node) unchanged for non-file nodes.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/file-viewer.tsx:
- Around line 92-96: The download logic and button disable currently treat empty
string as falsy so zero-byte files can’t be downloaded; update the check in
handleDownload to only return when content is null or undefined (e.g., if
(content === null || content === undefined) return;) and change the viewer’s
download button disabled condition to use the same null/undefined check (not a
general falsy check) so empty-string content is allowed and
triggerDownload(content, fileName, "text/plain") can run for zero-byte files;
refer to handleDownload, content, triggerDownload, and filePath when making the
changes.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/session-settings-modal.tsx:
- Around line 56-62: The hardcoded integrationsTotal (currently set to 4) can
drift from the actual list in IntegrationsPanel; replace this magic number by
deriving the total from a single source of truth—either import a shared constant
or the integrations list used by IntegrationsPanel and compute integrationsTotal
= integrationsList.length, and ensure integrationsConnected continues to filter
the same list (use the same keys used by integrationsStatus like
github/gitlab/jira/google) so both the total and connected calculations come
from the same exported symbol (e.g., integrationsList or INTEGRATIONS_COUNT)
rather than a hardcoded literal.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx:
- Around line 227-245: Replace the non-semantic div acting as a button with a
native button: in the sessions-sidebar component where the element is created
(keyed by session.metadata.uid and calling
handleNavigate(session.metadata.name)), change the <div role="button"> wrapper
to a <button type="button">, keep the onClick handler that calls handleNavigate,
retain className and key, and remove the manual tabIndex and onKeyDown
Enter/Space emulation so native keyboard behavior and semantics are used.
- Around line 161-170: The JSX currently nests a Button inside a Link (creating
<a><button>…), which is invalid; update the Workspaces and the other sidebar nav
instance (the block using Button, Link, ChevronLeft and the similar block at
177-189) to use Button's asChild prop and move Link inside the Button so the
Link becomes the rendered element (e.g., <Button asChild> <Link
href="…">…</Link> </Button>), preserving classes and children (ChevronLeft and
text) to avoid nested interactive elements and keep keyboard/focus behavior
correct.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsx:
- Around line 32-37: The test's skeleton detection is brittle because it queries
'[aria-hidden="true"]'; update the test and component so the skeletons are
detected via a stable identifier: add a data-testid (e.g.,
data-testid="card-skeleton") to the CardSkeleton component and change the test
in integrations-panel.test.tsx to assert on
container.querySelectorAll('[data-testid="card-skeleton"]') or use
getAllByTestId, keeping the existing mockUseIntegrationsStatus return ({ data:
null, isPending: true }) and the IntegrationsPanel render intact so the test
reliably targets CardSkeleton elements.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/__tests__/mcp-servers-panel.test.tsx:
- Around line 59-88: The test for McpServersPanel advances fake timers but never
asserts the post-timeout UI; update the test that uses vi.useFakeTimers() and
mockUseMcpStatus to wrap the timeout advance in act() (import act from
'@testing-library/react') and then assert the "No MCP servers" message is
rendered (e.g., expect(screen.getByText(/No MCP servers/i)).toBeInTheDocument())
after vi.advanceTimersByTime(16000), and finally restore timers with
vi.useRealTimers().
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/integrations-panel.tsx:
- Around line 18-21: Add a clarifying inline comment above the githubConfigured
assignment to explain the API shape difference: note that
integrationsStatus?.github?.active is an optional string enum ('app' | 'pat')
indicating which GitHub auth method is active (not a boolean), whereas other
providers use a boolean connected field; update the line setting
githubConfigured (and mention integrationsStatus and the active field) so future
maintainers understand why active != null is used instead of connected ?? false.
- Around line 102-113: The TooltipProvider is being created inside each
IntegrationCard, which is inefficient; hoist a single TooltipProvider to wrap
the whole list in IntegrationsPanel and remove the per-card TooltipProvider
usage in IntegrationCard. Specifically, wrap the list rendering in
IntegrationsPanel with one TooltipProvider (so all Tooltip, TooltipTrigger,
TooltipContent instances use that provider) and update IntegrationCard to render
Tooltip/TooltipTrigger/TooltipContent directly without creating its own
TooltipProvider.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/workflow-selector.tsx:
- Around line 217-226: The button rendered in the WorkflowItem (in
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/workflow-selector.tsx)
lacks an explicit type, which defaults to "submit" and can trigger unintended
form submissions; update the JSX for the button returned by the component (the
element with props onClick, disabled, className and conditional classes
selected/disabled) to include type="button" so it only acts as a normal button
and not a form submitter.
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1655-1662: The ExplorerPanel props list includes onUploadFile
twice which triggers a duplicate-prop lint/build error; remove the redundant
onUploadFile prop occurrence so only one onUploadFile={handleOpenUploadModal}
remains (locate the props block where onUploadFile is passed and drop the
duplicate entry referencing handleOpenUploadModal).
In `@components/frontend/src/components/chat/__tests__/ChatInputBox.test.tsx`:
- Around line 88-92: The test's fragile selector uses
btn.className.includes("rounded-full"); update the test in ChatInputBox.test.tsx
to query the send button by accessible name instead (use
screen.getByRole("button", { name: /send/i }) or similar) and assert it's
present, and if the ChatInputBox component lacks an accessible label, add
aria-label="Send" (or equivalent accessible text) to the send button element so
the test and accessibility are robust.
In `@components/frontend/src/components/chat/ChatInputBox.tsx`:
- Around line 593-606: The Upload File menu currently only calls
onUploadFile?.() leaving the hidden file input and handleFileSelect unreachable
if the prop isn't provided; update the DropdownMenuItem onClick handler to
fallback to programmatically opening the local picker by calling
fileInputRef.current?.click() when onUploadFile is not provided (e.g.,
onClick={() => onUploadFile ? onUploadFile() : fileInputRef.current?.click()});
ensure fileInputRef and handleFileSelect remain used as the fallback path so the
hidden <input> can handle selected files.
- Around line 674-678: The <button> element that calls handleSendAsNew (inside
the ChatInputBox component) lacks an explicit type, so it defaults to "submit"
and can unintentionally submit a parent form; update the button to include
type="button" to prevent form submission when clicked and satisfy the lint rule
while keeping existing props (onClick={handleSendAsNew}, disabled={!hasContent
|| isSending}, className, etc.).
In `@e2e/cypress/e2e/sessions.cy.ts`:
- Line 967: The condition checking for the modal title uses
$modal.find(':contains("Add Repository")').length > 1 which only passes when
there are more than one matches; change it to > 0 (or >= 1) so the URL input
handling runs when the modal is present. Update the conditional around the
$modal.find(':contains("Add Repository")').length check in sessions.cy.ts so the
block that fills and asserts the repository URL executes when at least one "Add
Repository" element exists.
---
Outside diff comments:
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1061-1121: Extract the initialPrompt detection and synthetic
message creation out of the streamMessages useMemo: create a small helper (e.g.,
getSyntheticInitialUserMessage or computeInitialPromptMessage) that takes
session?.spec?.initialPrompt, session?.metadata?.creationTimestamp and
committedStreamMessages and returns either the synthetic user_message object or
null by checking the same hasInitialUserMessage logic currently inline; then
simplify streamMessages to just concat the returned synthetic message (if any),
committedStreamMessages and the agent streaming blocks. Ensure you wire the
helper's return into the useMemo dependencies instead of repeating the
initialPrompt logic inside streamMessages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f97c2c2b-3bd5-42b6-8463-54d6041d1869
📒 Files selected for processing (57)
components/backend/handlers/sessions.gocomponents/frontend/src/app/layout.tsxcomponents/frontend/src/app/page.tsxcomponents/frontend/src/app/projects/[name]/keys/page.tsxcomponents/frontend/src/app/projects/[name]/layout.tsxcomponents/frontend/src/app/projects/[name]/new/page.tsxcomponents/frontend/src/app/projects/[name]/page.tsxcomponents/frontend/src/app/projects/[name]/permissions/page.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/page.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/content-tabs.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/file-viewer.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/runner-model-selector.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/session-settings-modal.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/sessions-sidebar.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/workflow-selector.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/content-tabs.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/context-tab.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/context-tab.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/files-tab.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/file-viewer.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/modals/add-context-modal.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/new-session-view.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/runner-model-selector.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/session-settings-modal.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/mcp-servers-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/session-details.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/card-skeleton.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/integrations-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/mcp-servers-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/session-details.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/workflow-selector.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-explorer-state.test.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-file-tabs.test.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-workflow-selection.test.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-explorer-state.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-workflow-selection.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/lib/types.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/session-header.tsxcomponents/frontend/src/app/projects/[name]/sessions/page.tsxcomponents/frontend/src/app/projects/[name]/settings/page.tsxcomponents/frontend/src/components/chat/ChatInputBox.tsxcomponents/frontend/src/components/chat/__tests__/ChatInputBox.test.tsxcomponents/frontend/src/components/navigation-wrapper.tsxcomponents/frontend/src/components/navigation.tsxcomponents/frontend/src/components/session/MessagesTab.tsxcomponents/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsxcomponents/frontend/src/components/workspace-sections/sessions-section.tsxcomponents/frontend/src/components/workspace-sections/sharing-section.tsxcomponents/frontend/src/hooks/use-resize-panel.tse2e/cypress/e2e/sessions.cy.ts
💤 Files with no reviewable changes (2)
- components/frontend/src/components/workspace-sections/sharing-section.tsx
- components/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsx
...rc/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx
Show resolved
Hide resolved
...rc/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx
Show resolved
Hide resolved
...p/projects/[name]/sessions/[sessionName]/components/__tests__/runner-model-selector.test.tsx
Outdated
Show resolved
Hide resolved
...projects/[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsx
Outdated
Show resolved
Hide resolved
components/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+593
to
+606
| <DropdownMenuItem onClick={() => onUploadFile?.()}> | ||
| <Upload className="w-4 h-4 mr-2" /> | ||
| Upload File | ||
| </DropdownMenuItem> | ||
| </DropdownMenuContent> | ||
| </DropdownMenu> | ||
| <input | ||
| ref={fileInputRef} | ||
| type="file" | ||
| multiple | ||
| accept="image/*,.pdf,.txt,.md,.json,.csv,.xml,.yaml,.yml,.log,.py,.js,.ts,.go,.java,.rs,.rb,.sh,.html,.css" | ||
| className="hidden" | ||
| onChange={handleFileSelect} | ||
| /> |
There was a problem hiding this comment.
Upload menu action is not wired to the local file picker path
At Line 593, Upload File only invokes onUploadFile?.(). If that prop is not provided, the action is a no-op, while Lines 599-606 keep a hidden file input + handleFileSelect flow that is otherwise unreachable.
Suggested fix
- <DropdownMenuItem onClick={() => onUploadFile?.()}>
+ <DropdownMenuItem
+ onClick={() => {
+ if (onUploadFile) {
+ onUploadFile();
+ } else {
+ fileInputRef.current?.click();
+ }
+ }}
+ >
<Upload className="w-4 h-4 mr-2" />
Upload File
</DropdownMenuItem>As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/components/chat/ChatInputBox.tsx` around lines 593 -
606, The Upload File menu currently only calls onUploadFile?.() leaving the
hidden file input and handleFileSelect unreachable if the prop isn't provided;
update the DropdownMenuItem onClick handler to fallback to programmatically
opening the local picker by calling fileInputRef.current?.click() when
onUploadFile is not provided (e.g., onClick={() => onUploadFile ? onUploadFile()
: fileInputRef.current?.click()}); ensure fileInputRef and handleFileSelect
remain used as the fallback path so the hidden <input> can handle selected
files.
Contributor
Author
There was a problem hiding this comment.
Good catch — will address in follow-up.
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/cypress/e2e/sessions.cy.ts (1)
85-92:⚠️ Potential issue | 🔴 CriticalSession bootstrap still uses a stale navigation path, which matches the current CI timeout.
The setup waits for and clicks
new-session-btnafter visiting/projects/${workspaceSlug}. With the new routing model, this flow can miss that button entirely and fail the suite before tests begin.Proposed fix
- cy.intercept('GET', '**/api/projects/*/agentic-sessions*').as('sessionsList') - cy.then(() => cy.visit(`/projects/${workspaceSlug}`)) - cy.wait('@sessionsList', { timeout: 15000 }) - cy.get('[data-testid="new-session-btn"]').should('be.visible').click() + cy.then(() => cy.visit(`/projects/${workspaceSlug}/new`)) cy.get('[data-testid="create-session-submit"]', { timeout: 10000 }) .should('not.be.disabled').click()As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` around lines 85 - 92, The test clicks the stale new-session-btn after visiting `/projects/${workspaceSlug}`, which can fail with the new routing; update the flow in sessions.cy.ts to navigate directly to the sessions route (e.g., cy.visit(`/projects/${workspaceSlug}/sessions`) or the equivalent route used by the app), then wait for the '@sessionsList' network alias and for the create session UI to be ready before interacting with '[data-testid="new-session-btn"]' and '[data-testid="create-session-submit"]' (ensure you wait for visibility/enabled states), and finally assert the URL with the existing regex match; this ensures the test doesn't depend on the old navigation path and stabilizes the clicks on new-session-btn and create-session-submit.
♻️ Duplicate comments (14)
e2e/cypress/e2e/sessions.cy.ts (1)
967-967:⚠️ Potential issue | 🟠 MajorModal presence gate is still incorrect (
> 1skips valid single-match modal cases).This still bypasses URL-input checks when exactly one "Add Repository" node is present.
Proposed fix
- if ($modal.find(':contains("Add Repository")').length > 1) { + if ($modal.find(':contains("Add Repository")').length > 0) {As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` at line 967, The modal presence gate is wrong because it checks $modal.find(':contains("Add Repository")').length > 1, which skips the valid single-match case; change this condition to detect any match (e.g., use .length > 0 or .length >= 1) and, to avoid false positives from nested text nodes, consider filtering for an exact text match (e.g., filter by el.innerText.trim() === "Add Repository") on the $modal.find(...) result so the URL-input checks run when exactly one "Add Repository" node is present.components/backend/handlers/sessions.go (1)
900-902:⚠️ Potential issue | 🟠 MajorGuard
created.Object["spec"]assertion to prevent post-create panics.Line 901 performs a direct type assertion that can panic if
specis missing or has an unexpected type. This would cause a 500 error after the session is already created, potentially leading to duplicate creates on client retry.This file already uses
unstructured.NestedMapelsewhere for safe access (e.g., lines 447-448, 1304).Proposed fix
- if strings.TrimSpace(req.InitialPrompt) != "" && strings.TrimSpace(req.DisplayName) == "" { - sessionCtx := ExtractSessionContext(created.Object["spec"].(map[string]interface{})) - GenerateDisplayNameAsync(project, name, req.InitialPrompt, sessionCtx) - } + if strings.TrimSpace(req.InitialPrompt) != "" && strings.TrimSpace(req.DisplayName) == "" { + if specMap, found, err := unstructured.NestedMap(created.Object, "spec"); err == nil && found { + sessionCtx := ExtractSessionContext(specMap) + GenerateDisplayNameAsync(project, name, req.InitialPrompt, sessionCtx) + } else { + log.Printf("CreateSession: skipping display name generation for %s/%s (spec access failed)", project, name) + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/sessions.go` around lines 900 - 902, The direct type assertion created.Object["spec"].(map[string]interface{}) can panic if spec is missing or wrong type—replace it with a safe lookup using unstructured.NestedMap(created.Object, "spec") (as used elsewhere) and handle the false case before calling ExtractSessionContext and GenerateDisplayNameAsync; specifically, retrieve spec via unstructured.NestedMap, return/skip display name generation if lookup fails, and pass the safely obtained map to ExtractSessionContext to avoid post-create panics.components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/context-tab.tsx (1)
127-130:⚠️ Potential issue | 🟡 MinorSet
type="button"on the branch-toggle button.The control still defaults to submit behavior if rendered inside a form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/context-tab.tsx around lines 127 - 130, The branch-toggle <button> currently relies on default submit behavior which can trigger form submits when this component is inside a form; update the button element that calls toggleExpanded (the element with onClick={toggleExpanded} and className="h-4 w-4 ...") to explicitly include type="button" to prevent it from acting as a submit control.components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/mcp-servers-panel.test.tsx (1)
2-3:⚠️ Potential issue | 🟠 MajorTimeout-path test is incomplete and does not assert the target UI.
The test advances fake timers but never verifies that the empty-state message is rendered, so the timeout branch is not actually tested.
Proposed fix
-import { render, screen } from '@testing-library/react'; +import { render, screen, act } from '@testing-library/react'; ... it('renders "No MCP servers" message when empty and timed out', () => { vi.useFakeTimers(); mockUseMcpStatus.mockReturnValue({ data: { servers: [], totalCount: 0 }, isPending: false, }); - render(<McpServersPanel {...defaultProps} sessionPhase="Running" />); - // Advance past the 15s timeout - vi.advanceTimersByTime(16000); - // After timeout, the component should re-render showing the empty message - // But we need to trigger a re-render... The useEffect sets state after timeout. - // With fake timers, act() should handle it: - vi.useRealTimers(); + try { + render(<McpServersPanel {...defaultProps} sessionPhase="Running" />); + act(() => { + vi.advanceTimersByTime(16000); + }); + expect( + screen.getByText('No MCP servers available for this session.') + ).toBeDefined(); + } finally { + vi.useRealTimers(); + } });As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
Also applies to: 59-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/__tests__/mcp-servers-panel.test.tsx around lines 2 - 3, The timeout-path test in mcp-servers-panel.test.tsx for the McpServersPanel component advances fake timers but never asserts the UI outcome; update the test to run the timers to completion (using jest.runAllTimers() or advanceTimersByTime inside act) and then assert the empty-state message is rendered (e.g., use screen.findByText or screen.getByText for the specific empty-state string rendered by McpServersPanel), and ensure timers are restored/cleared after the test.components/frontend/src/components/chat/ChatInputBox.tsx (2)
669-675:⚠️ Potential issue | 🟠 MajorAdd
type="button"to prevent unintended form submissionThe "Send as new" button at line 669 lacks an explicit
typeattribute. Native buttons default totype="submit", which can trigger parent form submission unexpectedly.Suggested fix
<button + type="button" onClick={handleSendAsNew} disabled={!hasContent || isSending} className="text-xs text-muted-foreground hover:text-foreground disabled:opacity-50" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/chat/ChatInputBox.tsx` around lines 669 - 675, The "Send as new" button in ChatInputBox (the button using onClick={handleSendAsNew}) lacks an explicit type and so defaults to type="submit", which can trigger parent form submission; update that button element to include type="button" to prevent unintended form submission while preserving the existing onClick, disabled, and className props.
588-601:⚠️ Potential issue | 🟠 MajorUpload File menu action doesn't fall back to local file picker
The
Upload Filedropdown item only invokesonUploadFile?.(). If that prop is not provided, the action is a no-op, while the hidden file input (lines 594-601) andhandleFileSelectremain unreachable.Suggested fix
- <DropdownMenuItem onClick={() => onUploadFile?.()}> + <DropdownMenuItem + onClick={() => { + if (onUploadFile) { + onUploadFile(); + } else { + fileInputRef.current?.click(); + } + }} + > <Upload className="w-4 h-4 mr-2" /> Upload File </DropdownMenuItem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/chat/ChatInputBox.tsx` around lines 588 - 601, The DropdownMenuItem click currently calls onUploadFile?.(), which is a no-op when the prop is missing and prevents using the hidden input; change the click handler for the "Upload File" item (DropdownMenuItem) to call onUploadFile if present, otherwise trigger a click on the hidden file input via fileInputRef (e.g., fileInputRef.current?.click()), so the existing handleFileSelect remains reachable; ensure fileInputRef is properly forwarded/defined and that the handler preserves existing behavior when onUploadFile exists.components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/files-tab.tsx (1)
76-85:⚠️ Potential issue | 🟠 MajorFile path construction uses
node.nameinstead ofnode.pathThe
handleSelectfunction reconstructs the file path usingnode.name(line 79-80), butfileNodesmapping at line 68 preserves the fullpathfrom the originalWorkspaceItem. For files in subdirectories wherepathdiffers fromname, this will open the wrong file.Suggested fix
const handleSelect = (node: FileTreeNode) => { if (node.type === "file" && onFileOpen) { - const fullPath = currentSubPath - ? `${selectedDirectory.path}/${currentSubPath}/${node.name}` - : `${selectedDirectory.path}/${node.name}`; - onFileOpen(fullPath); + onFileOpen(node.path); } else { onFileOrFolderSelect(node); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/files-tab.tsx around lines 76 - 85, handleSelect builds file paths using node.name which can differ from the original WorkspaceItem.path; use the node.path preserved in fileNodes instead of reconstructing from node.name. Update the handleSelect logic (function handleSelect, type FileTreeNode) to compute fullPath by combining selectedDirectory.path and currentSubPath with node.path (or using node.path directly if it is already absolute/relative as expected) and then call onFileOpen(fullPath); keep onFileOrFolderSelect(node) behavior for non-file nodes.components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx (3)
26-28:⚠️ Potential issue | 🟠 MajorMock button needs explicit
type="button"The mock
WorkflowSelectorbutton at line 27 lacks atypeattribute.Suggested fix
vi.mock('../workflow-selector', () => ({ - WorkflowSelector: () => <button data-testid="workflow-selector">No workflow</button>, + WorkflowSelector: () => <button type="button" data-testid="workflow-selector">No workflow</button>, }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx around lines 26 - 28, The mock implementation of WorkflowSelector used in the test renders a button without an explicit type, which can submit forms unexpectedly; update the mock (the WorkflowSelector export in the test) to render a button with type="button" (i.e., change the mock component that returns <button data-testid="workflow-selector">... to include type="button") so it won't act as a submit button during tests.
59-64: 🧹 Nitpick | 🔵 TrivialSubmit button assertion uses fragile positional lookup
Finding the submit button by taking the last element from
getAllByRole('button')will break if UI changes add more buttons. Use an accessible selector instead.Suggested improvement
Add
aria-label="Submit"to the submit button inNewSessionView, then:it('send button is disabled when textarea is empty', () => { render(<NewSessionView {...defaultProps} />); - const allButtons = screen.getAllByRole('button'); - const lastButton = allButtons[allButtons.length - 1]; - expect(lastButton.hasAttribute('disabled')).toBe(true); + const submitButton = screen.getByRole('button', { name: /submit/i }); + expect(submitButton).toBeDisabled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx around lines 59 - 64, The test in new-session-view.test.tsx uses a fragile positional lookup to find the submit button; update the NewSessionView component to add an accessible name (e.g., add aria-label="Submit" on the submit button in NewSessionView) and change the test (in the same file) to locate the button by role and accessible name (e.g., getByRole('button', { name: /submit/i })) and assert it is disabled, so the lookup targets the Submit control explicitly rather than relying on button order.
5-12:⚠️ Potential issue | 🟠 MajorMock button needs explicit
type="button"The mock
RunnerModelSelectorbutton at line 7 lacks atypeattribute, which can cause issues if rendered in a form context and fails the lint rule.Suggested fix
vi.mock('../runner-model-selector', () => ({ RunnerModelSelector: ({ onSelect }: { onSelect: (r: string, m: string) => void }) => ( - <button data-testid="runner-model-selector" onClick={() => onSelect('claude-agent-sdk', 'claude-sonnet-4-5')}> + <button type="button" data-testid="runner-model-selector" onClick={() => onSelect('claude-agent-sdk', 'claude-sonnet-4-5')}> claude-agent-sdk · Claude Sonnet 4.5 </button> ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx around lines 5 - 12, The mocked RunnerModelSelector component renders a button without an explicit type which can cause form-submit behavior and lint failures; update the mock for RunnerModelSelector in the test to set the button's type to "button" (i.e., add type="button" on the button element used in the RunnerModelSelector mock) so it won't act as a submit control when rendered inside forms.components/frontend/src/components/chat/__tests__/ChatInputBox.test.tsx (1)
88-92: 🧹 Nitpick | 🔵 TrivialFragile selector: prefer accessible query over CSS class matching
The test queries the Send button using
btn.className.includes("rounded-full"), coupling the test to styling implementation. If the button has anaria-label, use that for a more robust selector.Suggested improvement
Add
aria-label="Send"to the send button inChatInputBox.tsx, then update the test:- const sendBtn = screen.getAllByRole("button").find( - (btn) => !btn.hasAttribute("disabled") && btn.className.includes("rounded-full") - ); + const sendBtn = screen.getByRole("button", { name: /send/i }); expect(sendBtn).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/chat/__tests__/ChatInputBox.test.tsx` around lines 88 - 92, The test is using a fragile CSS-class-based selector to find the Send button; update the implementation and test to use an accessible label instead: add aria-label="Send" (or similar) to the circular send button in ChatInputBox (the component that renders the send button) and change the test in ChatInputBox.test.tsx to query the button by role with name (e.g., getByRole("button", { name: /send/i }) or equivalent) rather than filtering by className on sendBtn.components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsx (2)
75-83: 🧹 Nitpick | 🔵 TrivialClose button assertion uses fragile positional lookup
Selecting the close button by array index (
buttons[buttons.length - 1]) will break if the header layout changes. Use an accessible selector.Suggested improvement
The close
Buttoninexplorer-panel.tsxshould have an accessible label. Then update the test:it('calls onClose when close button clicked', () => { render(<ExplorerPanel {...defaultProps} />); - // The close button is the one with the X icon, last button in the header - const buttons = screen.getAllByRole('button'); - // Close button is the last button in the tab header area - const closeButton = buttons[buttons.length - 1]; + const closeButton = screen.getByRole('button', { name: /close/i }); fireEvent.click(closeButton); expect(defaultProps.onClose).toHaveBeenCalledTimes(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsx around lines 75 - 83, The test uses a positional lookup for the close button which is fragile; update the Close Button in explorer-panel.tsx (the Button inside the ExplorerPanel component) to include an accessible label/aria-label (e.g., "Close" or similar), then change the test in explorer-panel.test.tsx to select the button by accessible name (e.g., getByRole('button', { name: /close/i }) or getByLabelText) and assert onClose is called; reference the ExplorerPanel component and the close Button when making the changes.
32-38:⚠️ Potential issue | 🟠 MajorTest fixture uses wrong prop name
onAddContextThe component expects
onAddRepository(perexplorer-panel.tsxline 63), but the test usesonAddContext. This reduces test fidelity for the Context tab action wiring.Suggested fix
// Context tab props repositories: [], uploadedFiles: [], - onAddContext: vi.fn(), + onAddRepository: vi.fn(), onRemoveRepository: vi.fn(), onRemoveFile: vi.fn(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsx around lines 32 - 38, The test fixture in explorer-panel.test.tsx is passing the wrong prop name: it uses onAddContext but the ExplorerPanel component expects onAddRepository (see explorer-panel.tsx prop usage). Update the props object in the test to replace onAddContext: vi.fn() with onAddRepository: vi.fn(), and adjust any test references/assertions that call or spy on the old name so they target onAddRepository instead.components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsx (1)
73-96:⚠️ Potential issue | 🟡 MinorAdd
type="button"to tab buttons.Both tab buttons lack an explicit
typeattribute. Without it, buttons default totype="submit"which can cause unintended form submission if the panel is ever rendered inside a form.Proposed fix
<button + type="button" onClick={() => onTabChange("files")} className={cn(<button + type="button" onClick={() => onTabChange("context")} className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsx around lines 73 - 96, The two tab buttons in explorer-panel.tsx (the button elements that call onTabChange("files") and onTabChange("context") and render FolderOpen and Link icons) are missing an explicit type and default to "submit"; update both button elements to include type="button" to prevent accidental form submissions when this panel is nested in a form, ensuring the onTabChange handler behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/frontend/src/app/projects/`[name]/scheduled-sessions/page.tsx:
- Around line 1-10: Remove the client boundary and convert ScheduledSessionsPage
into a Server Component that accepts params; delete the 'use client' directive
and the useParams import, change the default export to an async function
ScheduledSessionsPage({ params }) and derive projectName from params.name, and
pass that projectName into the existing SchedulesSection client component so the
client boundary remains only inside SchedulesSection; keep the existing guard
for missing projectName (return null) if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/context-tab.tsx:
- Around line 127-136: Icon-only action buttons in context-tab.tsx (e.g., the
expand/collapse button that calls toggleExpanded and uses isExpanded, plus the
remove repository/remove file buttons) lack accessible names; add explicit
aria-label attributes that describe the action (e.g., "Expand branch", "Collapse
branch", "Remove repository", "Remove file") on each button and ensure the label
toggles appropriately when state changes (use isExpanded to switch
expand/collapse label). Also include a visually hidden title or
aria-pressed/aria-expanded if appropriate for stateful controls to improve
screen reader semantics.
- Around line 96-123: The component uses unstable index-based keys and derived
names (repoName, file.name) which break reconciliation and state tracking;
change keys and state identifiers to stable unique properties: use repo.url (or
repo.path) instead of `repo-${idx}`/repoName for keys, removingRepo, and
expandedRepos, and use file.path instead of `file-${idx}`/file.name when
rendering files and tracking expanded/removed state (update usages in
repositories.map, toggleExpanded, setExpandedRepos, and any references around
lines where file keys/state are set such as where `file-${idx}` is used).
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/new-session-view.tsx:
- Around line 62-71: The effect in the component (useEffect) is re-running on
user selection because selectedRunner is in the dependency array; change the
dependency array to only watch runnerTypes so the effect only validates when
runnerTypes change. Update the effect that currently references runnerTypes and
selectedRunner (which calls setSelectedRunner and setSelectedModel with
getDefaultModel) to remove selectedRunner from the dependency list and keep
runnerTypes only, ensuring the same logic runs only when runnerTypes are
loaded/updated.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/session-settings-modal.tsx:
- Around line 55-62: The badge shows "0/4" while useIntegrationsStatus is
unresolved because integrationsStatus is undefined; modify the logic that
computes integrationsConnected and the badge render to gate on the query state
from useIntegrationsStatus (e.g., useIntegrationsStatus().isPending / .isError /
.isLoading or check integrationsStatus presence) so you only compute
integrationsConnected when integrationsStatus exists, otherwise render a
neutral/placeholder state; update the expressions referencing integrationsStatus
(the integrationsConnected array and the place that uses integrationsTotal) to
short-circuit until the status is available.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/integrations-panel.tsx:
- Around line 16-21: The integration-status logic treats a failed/no-data load
as "disconnected" because githubConfigured, gitlabConfigured, jiraConfigured,
and googleConfigured are derived directly from integrationsStatus; change the
checks to detect when the hook has errored without any cached data
(useIntegrationsStatus's isError and integrationsStatus === undefined) and
return an "unknown"/neutral state instead of false—i.e., update the expressions
around integrationsStatus (used in githubConfigured, gitlabConfigured,
jiraConfigured, googleConfigured and the UI branches further down) to explicitly
handle the isError && !integrationsStatus case and avoid rendering "Not
connected" or counting them as disconnected.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/mcp-servers-panel.tsx:
- Around line 45-48: The current showPlaceholders logic forces placeholders for
any non-running session because it includes !isRunning; remove that clause so
non-running sessions can show the real empty/data state. Update the
showPlaceholders assignment (referencing showPlaceholders, isRunning,
mcpPending, mcpServers, placeholderTimedOut) to something like: show
placeholders only when mcpPending is true or when mcpServers.length === 0 and
!placeholderTimedOut, thereby dropping the !isRunning check.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/session-details.tsx:
- Around line 35-42: The icon-only edit button lacks an accessible label; update
the Button (the one rendering <Pencil /> and using onEditName) to include a
stable aria-label or accessible name such as "Edit session name" (e.g.,
aria-label or an explicit visually-hidden text) so screen readers can announce
it, and update the corresponding test to query that label (e.g.,
getByRole/getByLabelText "Edit session name") instead of counting buttons.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/hooks/use-workflow-selection.ts:
- Around line 23-35: filteredWorkflows filters and lowercases
workflow.description but WorkflowConfig marks description as optional, which can
cause a TypeError; update the filter in useMemo (the filteredWorkflows
computation using ootbWorkflows and search) to safely handle missing
descriptions by defaulting to an empty string (e.g., use workflow.description ??
'' or optional chaining with a fallback) before calling toLowerCase(), leaving
the rest of the logic (name check and sort by a.name.localeCompare) unchanged.
In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/page.tsx:
- Around line 1634-1637: The resize handle div using explorerResize.onMouseDown
lacks keyboard and ARIA support; update the element used for the handle (or the
useResizePanel implementation) to expose and hook a keyboard-accessible action
(e.g., call adjustWidth from useResizePanel on onKeyDown/Enter/Arrow keys) and
add appropriate ARIA attributes (role="separator", aria-orientation="vertical"
and aria-valuenow/aria-valuemin/aria-valuemax if applicable) so keyboard users
can resize, or if you intentionally don't want it interactive via keyboard mark
the element aria-hidden="true" and ensure an alternative control exists; ensure
references to explorerResize, useResizePanel, and adjustWidth are used to locate
and wire up the changes.
In `@components/frontend/src/components/workspace-sections/sessions-section.tsx`:
- Around line 217-222: The href for the Link in the New Session button uses raw
projectName and can break with special characters; update the href template to
URL-encode projectName (use encodeURIComponent(projectName)) so the Link
receives a safe path (the JSX element using Link and the surrounding Button with
data-testid="new-session-btn" should pass the encoded value). Ensure the encoded
value is used wherever projectName is interpolated for routes to match the
page.tsx approach.
In `@components/frontend/src/hooks/use-resize-panel.ts`:
- Around line 16-17: The code currently writes to localStorage on every
mousemove by calling the useLocalStorage setter (setWidth) and also trusts the
stored width raw on read; fix this by keeping the live dragging width in
component state (e.g., a local state like dragWidth or reuse width for in-memory
during drag) and only call setWidth once in the mouseup handler, not inside
mousemove; when initializing/reading the persisted value (the value returned by
useLocalStorage/storageKey/defaultWidth) clamp it to the allowed bounds (min/max
width constants or a clamp helper) so it never renders outside bounds before a
drag; ensure mousemove updates only the in-memory state (and respects clamping)
and mouseup persists the clamped value with setWidth and clears isDragging.
In `@e2e/cypress/e2e/sessions.cy.ts`:
- Around line 959-962: The selector using $body.find('button:contains("Add
Repository"), button:contains("Add")') is too broad and can match unrelated
"Add" buttons; update the DOM query used in the sessions.cy.ts test (the addBtn
lookup) to target a more specific, stable hook (e.g., exact text "Add
Repository", a data-testid/data-cy attribute, an aria-label, or scope the search
to the explorer panel container) so the test clicks the intended control
deterministically; apply the same change to the other occurrence mentioned
(around the addBtn usage at the later lines) so both lookups use the same
precise selector or test id.
- Around line 1985-1991: Replace the silent cy.log fallbacks so tests fail when
required UI is missing: locate the block that logs "Context tab not found" and
the branch that logs "Empty context state displayed correctly" (search for those
exact strings) and replace those cy.log calls with explicit test failures (e.g.,
throw new Error(...) or use Cypress/Chai assertions like expect(...).to.exist or
assert.fail(...)) so the spec fails when the Context tab or expected content is
absent rather than quietly logging and passing.
- Around line 137-143: The test uses a brittle text match
cy.contains('Workspaces').click({ force: true }) which can hit non-link
elements; replace this with a deterministic selector for the sidebar link (e.g.,
a data attribute like data-cy or an explicit role/link query) and click that
instead, then assert navigation via cy.url(). Specifically, update the test in
sessions.cy.ts to target the sidebar workspaces link using a unique selector
(for example the element identified by data-cy="sidebar-workspaces" or using
cy.findByRole('link', { name: /Workspaces/ })) in place of
cy.contains('Workspaces'), keep the visibility checks for 'Workspaces' and
'Sessions' but use the deterministic selector for the .click() action so the
navigation assertion remains reliable.
- Around line 305-320: The workflow selector check currently uses a
non-assertive guard (if (workflowBtn.length)) which allows the step to silently
pass; replace this with an explicit assertion and failure path so the test fails
when the control is missing: locate the control via the workflowBtn lookup (or
prefer cy.contains/cy.get selectors), assert its existence using a Cypress
assertion (e.g., should('exist') / should('have.length.greaterThan', 0) or
expect on wrapped element), then click it and assert the popover appears before
attempting to click "Fix a bug" (assert the popover contains "Fix a bug" and
then click); avoid swallowing absence with plain if checks in sessions.cy.ts and
ensure failures surface in CI.
---
Outside diff comments:
In `@e2e/cypress/e2e/sessions.cy.ts`:
- Around line 85-92: The test clicks the stale new-session-btn after visiting
`/projects/${workspaceSlug}`, which can fail with the new routing; update the
flow in sessions.cy.ts to navigate directly to the sessions route (e.g.,
cy.visit(`/projects/${workspaceSlug}/sessions`) or the equivalent route used by
the app), then wait for the '@sessionsList' network alias and for the create
session UI to be ready before interacting with '[data-testid="new-session-btn"]'
and '[data-testid="create-session-submit"]' (ensure you wait for
visibility/enabled states), and finally assert the URL with the existing regex
match; this ensures the test doesn't depend on the old navigation path and
stabilizes the clicks on new-session-btn and create-session-submit.
---
Duplicate comments:
In `@components/backend/handlers/sessions.go`:
- Around line 900-902: The direct type assertion
created.Object["spec"].(map[string]interface{}) can panic if spec is missing or
wrong type—replace it with a safe lookup using
unstructured.NestedMap(created.Object, "spec") (as used elsewhere) and handle
the false case before calling ExtractSessionContext and
GenerateDisplayNameAsync; specifically, retrieve spec via
unstructured.NestedMap, return/skip display name generation if lookup fails, and
pass the safely obtained map to ExtractSessionContext to avoid post-create
panics.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsx:
- Around line 26-28: The mock implementation of WorkflowSelector used in the
test renders a button without an explicit type, which can submit forms
unexpectedly; update the mock (the WorkflowSelector export in the test) to
render a button with type="button" (i.e., change the mock component that returns
<button data-testid="workflow-selector">... to include type="button") so it
won't act as a submit button during tests.
- Around line 59-64: The test in new-session-view.test.tsx uses a fragile
positional lookup to find the submit button; update the NewSessionView component
to add an accessible name (e.g., add aria-label="Submit" on the submit button in
NewSessionView) and change the test (in the same file) to locate the button by
role and accessible name (e.g., getByRole('button', { name: /submit/i })) and
assert it is disabled, so the lookup targets the Submit control explicitly
rather than relying on button order.
- Around line 5-12: The mocked RunnerModelSelector component renders a button
without an explicit type which can cause form-submit behavior and lint failures;
update the mock for RunnerModelSelector in the test to set the button's type to
"button" (i.e., add type="button" on the button element used in the
RunnerModelSelector mock) so it won't act as a submit control when rendered
inside forms.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsx:
- Around line 75-83: The test uses a positional lookup for the close button
which is fragile; update the Close Button in explorer-panel.tsx (the Button
inside the ExplorerPanel component) to include an accessible label/aria-label
(e.g., "Close" or similar), then change the test in explorer-panel.test.tsx to
select the button by accessible name (e.g., getByRole('button', { name: /close/i
}) or getByLabelText) and assert onClose is called; reference the ExplorerPanel
component and the close Button when making the changes.
- Around line 32-38: The test fixture in explorer-panel.test.tsx is passing the
wrong prop name: it uses onAddContext but the ExplorerPanel component expects
onAddRepository (see explorer-panel.tsx prop usage). Update the props object in
the test to replace onAddContext: vi.fn() with onAddRepository: vi.fn(), and
adjust any test references/assertions that call or spy on the old name so they
target onAddRepository instead.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/context-tab.tsx:
- Around line 127-130: The branch-toggle <button> currently relies on default
submit behavior which can trigger form submits when this component is inside a
form; update the button element that calls toggleExpanded (the element with
onClick={toggleExpanded} and className="h-4 w-4 ...") to explicitly include
type="button" to prevent it from acting as a submit control.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsx:
- Around line 73-96: The two tab buttons in explorer-panel.tsx (the button
elements that call onTabChange("files") and onTabChange("context") and render
FolderOpen and Link icons) are missing an explicit type and default to "submit";
update both button elements to include type="button" to prevent accidental form
submissions when this panel is nested in a form, ensuring the onTabChange
handler behavior remains unchanged.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/files-tab.tsx:
- Around line 76-85: handleSelect builds file paths using node.name which can
differ from the original WorkspaceItem.path; use the node.path preserved in
fileNodes instead of reconstructing from node.name. Update the handleSelect
logic (function handleSelect, type FileTreeNode) to compute fullPath by
combining selectedDirectory.path and currentSubPath with node.path (or using
node.path directly if it is already absolute/relative as expected) and then call
onFileOpen(fullPath); keep onFileOrFolderSelect(node) behavior for non-file
nodes.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/__tests__/mcp-servers-panel.test.tsx:
- Around line 2-3: The timeout-path test in mcp-servers-panel.test.tsx for the
McpServersPanel component advances fake timers but never asserts the UI outcome;
update the test to run the timers to completion (using jest.runAllTimers() or
advanceTimersByTime inside act) and then assert the empty-state message is
rendered (e.g., use screen.findByText or screen.getByText for the specific
empty-state string rendered by McpServersPanel), and ensure timers are
restored/cleared after the test.
In `@components/frontend/src/components/chat/__tests__/ChatInputBox.test.tsx`:
- Around line 88-92: The test is using a fragile CSS-class-based selector to
find the Send button; update the implementation and test to use an accessible
label instead: add aria-label="Send" (or similar) to the circular send button in
ChatInputBox (the component that renders the send button) and change the test in
ChatInputBox.test.tsx to query the button by role with name (e.g.,
getByRole("button", { name: /send/i }) or equivalent) rather than filtering by
className on sendBtn.
In `@components/frontend/src/components/chat/ChatInputBox.tsx`:
- Around line 669-675: The "Send as new" button in ChatInputBox (the button
using onClick={handleSendAsNew}) lacks an explicit type and so defaults to
type="submit", which can trigger parent form submission; update that button
element to include type="button" to prevent unintended form submission while
preserving the existing onClick, disabled, and className props.
- Around line 588-601: The DropdownMenuItem click currently calls
onUploadFile?.(), which is a no-op when the prop is missing and prevents using
the hidden input; change the click handler for the "Upload File" item
(DropdownMenuItem) to call onUploadFile if present, otherwise trigger a click on
the hidden file input via fileInputRef (e.g., fileInputRef.current?.click()), so
the existing handleFileSelect remains reachable; ensure fileInputRef is properly
forwarded/defined and that the handler preserves existing behavior when
onUploadFile exists.
In `@e2e/cypress/e2e/sessions.cy.ts`:
- Line 967: The modal presence gate is wrong because it checks
$modal.find(':contains("Add Repository")').length > 1, which skips the valid
single-match case; change this condition to detect any match (e.g., use .length
> 0 or .length >= 1) and, to avoid false positives from nested text nodes,
consider filtering for an exact text match (e.g., filter by el.innerText.trim()
=== "Add Repository") on the $modal.find(...) result so the URL-input checks run
when exactly one "Add Repository" node is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0340b7c3-7ca1-4425-b88d-ef732e4b8676
📒 Files selected for processing (57)
components/backend/handlers/sessions.gocomponents/frontend/src/app/layout.tsxcomponents/frontend/src/app/page.tsxcomponents/frontend/src/app/projects/[name]/keys/page.tsxcomponents/frontend/src/app/projects/[name]/layout.tsxcomponents/frontend/src/app/projects/[name]/new/page.tsxcomponents/frontend/src/app/projects/[name]/page.tsxcomponents/frontend/src/app/projects/[name]/permissions/page.tsxcomponents/frontend/src/app/projects/[name]/scheduled-sessions/page.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/content-tabs.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/file-viewer.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/new-session-view.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/runner-model-selector.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/session-settings-modal.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/sessions-sidebar.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/__tests__/workflow-selector.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/content-tabs.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/context-tab.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/__tests__/explorer-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/context-tab.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/explorer-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/files-tab.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/file-viewer.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/modals/add-context-modal.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/new-session-view.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/runner-model-selector.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/session-settings-modal.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/mcp-servers-panel.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/session-details.test.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/card-skeleton.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/integrations-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/mcp-servers-panel.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/session-details.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/workflow-selector.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-explorer-state.test.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-file-tabs.test.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/__tests__/use-workflow-selection.test.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-explorer-state.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-file-tabs.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/hooks/use-workflow-selection.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/lib/types.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/page.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/session-header.tsxcomponents/frontend/src/app/projects/[name]/sessions/page.tsxcomponents/frontend/src/app/projects/[name]/settings/page.tsxcomponents/frontend/src/components/chat/ChatInputBox.tsxcomponents/frontend/src/components/chat/__tests__/ChatInputBox.test.tsxcomponents/frontend/src/components/navigation-wrapper.tsxcomponents/frontend/src/components/navigation.tsxcomponents/frontend/src/components/session/MessagesTab.tsxcomponents/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsxcomponents/frontend/src/components/workspace-sections/sessions-section.tsxcomponents/frontend/src/components/workspace-sections/sharing-section.tsxcomponents/frontend/src/hooks/use-resize-panel.tse2e/cypress/e2e/sessions.cy.ts
💤 Files with no reviewable changes (2)
- components/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsx
- components/frontend/src/components/workspace-sections/sharing-section.tsx
Comment on lines
+1
to
+10
| 'use client'; | ||
|
|
||
| import { useParams } from 'next/navigation'; | ||
| import { SchedulesSection } from '@/components/workspace-sections/scheduled-sessions-tab'; | ||
|
|
||
| export default function ScheduledSessionsPage() { | ||
| const params = useParams(); | ||
| const projectName = params?.name as string; | ||
|
|
||
| if (!projectName) return null; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
PAGE='components/frontend/src/app/projects/[name]/scheduled-sessions/page.tsx'
echo "== Next.js version =="
rg -n --no-heading '"next"\s*:\s*"' package.json
echo
echo "== Client-only usage in page =="
rg -n --no-heading '\buseParams\s*\(|\buse[A-Z][A-Za-z0-9_]*\s*\(' "$PAGE"
echo
echo "== Locate SchedulesSection implementation and inspect boundary =="
fd 'scheduled-sessions-tab.tsx' components/frontend/src --exec sh -c '
for f in "$@"; do
echo "-- $f --"
sed -n "1,120p" "$f"
done
' sh
echo
echo "== Compare sibling project pages that already use params props =="
rg -n --no-heading "export default (async )?function .*Page\\s*\\(\\{\\s*params" components/frontend/src/app/projects -g 'page.tsx'Repository: ambient-code/platform
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Repository structure =="
ls -la | head -20
echo
echo "== Finding package.json files =="
fd 'package.json' . -type f | head -20
echo
echo "== Finding the target file =="
fd 'scheduled-sessions' . -type f | grep -E '\.(tsx?|jsx?)$'Repository: ambient-code/platform
Length of output: 1511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Finding package.json files =="
fd 'package.json' --type f
echo
echo "== Finding target page file =="
fd 'page.tsx' --path '*scheduled-sessions*'
echo
echo "== Directory listing of components =="
ls -la components/ | head -20Repository: ambient-code/platform
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
echo "== Next.js version from frontend package.json =="
rg '"next"' components/frontend/package.json -A 1 -B 1
echo
echo "== Find scheduled-sessions page =="
find components/frontend -name 'page.tsx' | grep scheduled-sessions
echo
echo "== Look for SchedulesSection component =="
find components/frontend -name '*scheduled-sessions-tab*'
echo
echo "== Check sibling pages for params pattern =="
rg 'export default.*Page.*\(.*params' components/frontend/src/app/projects --type tsxRepository: ambient-code/platform
Length of output: 732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Target page.tsx content =="
cat -n components/frontend/src/app/projects/[name]/scheduled-sessions/page.tsx
echo
echo "== SchedulesSection implementation (first 50 lines) =="
head -50 components/frontend/src/components/workspace-sections/scheduled-sessions-tab.tsx
echo
echo "== Check sibling pages in /projects for params usage =="
find components/frontend/src/app/projects -name 'page.tsx' -exec grep -l "params" {} \;
echo
echo "== Example: read one sibling page using params =="
find components/frontend/src/app/projects -name 'page.tsx' ! -path '*scheduled-sessions*' | head -1 | xargs cat -nRepository: ambient-code/platform
Length of output: 19187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check all project pages for server component pattern (no 'use client') =="
find components/frontend/src/app/projects -name 'page.tsx' | while read file; do
if ! grep -q "'use client'" "$file"; then
echo "Server component page found: $file"
head -15 "$file"
echo "---"
fi
done
echo
echo "== Search for any page.tsx that receives params prop =="
rg "function.*Page.*\{.*params" components/frontend/src/app/projects -A 3
echo
echo "== Check nested scheduled session page for comparison =="
cat -n components/frontend/src/app/projects/[name]/scheduled-sessions/[scheduledSessionName]/page.tsx | head -20Repository: ambient-code/platform
Length of output: 2182
Consider converting to a server component using async params, aligning with the parent route pattern.
This page can be a Server Component receiving params as a prop, which keeps the client boundary at SchedulesSection and aligns with the established pattern in the parent route ([name]/page.tsx). While all sibling detail pages currently use 'use client' with useParams(), converting to async params improves consistency within the route tree.
♻️ Suggested refactor
-'use client';
-
-import { useParams } from 'next/navigation';
import { SchedulesSection } from '@/components/workspace-sections/scheduled-sessions-tab';
-export default function ScheduledSessionsPage() {
- const params = useParams();
- const projectName = params?.name as string;
-
- if (!projectName) return null;
+type PageProps = {
+ params: Promise<{ name: string }>;
+};
+
+export default async function ScheduledSessionsPage({ params }: PageProps) {
+ const { name: projectName } = await params;
return (
<div className="h-full overflow-auto p-6">
<SchedulesSection projectName={projectName} />
</div>
);
}📝 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
| 'use client'; | |
| import { useParams } from 'next/navigation'; | |
| import { SchedulesSection } from '@/components/workspace-sections/scheduled-sessions-tab'; | |
| export default function ScheduledSessionsPage() { | |
| const params = useParams(); | |
| const projectName = params?.name as string; | |
| if (!projectName) return null; | |
| import { SchedulesSection } from '@/components/workspace-sections/scheduled-sessions-tab'; | |
| type PageProps = { | |
| params: Promise<{ name: string }>; | |
| }; | |
| export default async function ScheduledSessionsPage({ params }: PageProps) { | |
| const { name: projectName } = await params; | |
| return ( | |
| <div className="h-full overflow-auto p-6"> | |
| <SchedulesSection projectName={projectName} /> | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/app/projects/`[name]/scheduled-sessions/page.tsx
around lines 1 - 10, Remove the client boundary and convert
ScheduledSessionsPage into a Server Component that accepts params; delete the
'use client' directive and the useParams import, change the default export to an
async function ScheduledSessionsPage({ params }) and derive projectName from
params.name, and pass that projectName into the existing SchedulesSection client
component so the client boundary remains only inside SchedulesSection; keep the
existing guard for missing projectName (return null) if needed.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — will address in follow-up cleanup.
.../frontend/src/app/projects/[name]/sessions/[sessionName]/components/explorer/context-tab.tsx
Show resolved
Hide resolved
Comment on lines
+127
to
+136
| <button | ||
| onClick={toggleExpanded} | ||
| className="h-4 w-4 text-muted-foreground flex-shrink-0 hover:text-foreground cursor-pointer" | ||
| > | ||
| {isExpanded ? ( | ||
| <ChevronDown className="h-4 w-4" /> | ||
| ) : ( | ||
| <ChevronRight className="h-4 w-4" /> | ||
| )} | ||
| </button> |
There was a problem hiding this comment.
Add accessible names to icon-only action buttons.
These controls need explicit aria-label values (expand/collapse repo branches, remove repository, remove file) so assistive tech can identify their actions.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
Also applies to: 180-196, 285-297
🧰 Tools
🪛 Biome (2.4.7)
[error] 127-130: Provide an explicit type prop for the button element.
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/explorer/context-tab.tsx
around lines 127 - 136, Icon-only action buttons in context-tab.tsx (e.g., the
expand/collapse button that calls toggleExpanded and uses isExpanded, plus the
remove repository/remove file buttons) lack accessible names; add explicit
aria-label attributes that describe the action (e.g., "Expand branch", "Collapse
branch", "Remove repository", "Remove file") on each button and ensure the label
toggles appropriately when state changes (use isExpanded to switch
expand/collapse label). Also include a visually hidden title or
aria-pressed/aria-expanded if appropriate for stateful controls to improve
screen reader semantics.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — will address in follow-up cleanup.
Contributor
Author
There was a problem hiding this comment.
Fixed in latest push.
Comment on lines
+62
to
+71
| useEffect(() => { | ||
| if (runnerTypes && runnerTypes.length > 0) { | ||
| const isCurrentAvailable = runnerTypes.some((r) => r.id === selectedRunner); | ||
| if (!isCurrentAvailable) { | ||
| const firstRunner = runnerTypes[0].id; | ||
| setSelectedRunner(firstRunner); | ||
| setSelectedModel(getDefaultModel(firstRunner)); | ||
| } | ||
| } | ||
| }, [runnerTypes, selectedRunner]); |
There was a problem hiding this comment.
Unnecessary re-renders from useEffect dependency.
Including selectedRunner in the dependency array causes this effect to run whenever the user manually selects a different runner, even though the effect only needs to validate against newly loaded runnerTypes. This could reset user selection unexpectedly if the component re-renders after a manual selection.
🔧 Proposed fix
useEffect(() => {
if (runnerTypes && runnerTypes.length > 0) {
- const isCurrentAvailable = runnerTypes.some((r) => r.id === selectedRunner);
+ const isCurrentAvailable = runnerTypes.some((r) => r.id === DEFAULT_RUNNER_TYPE_ID);
if (!isCurrentAvailable) {
const firstRunner = runnerTypes[0].id;
setSelectedRunner(firstRunner);
setSelectedModel(getDefaultModel(firstRunner));
}
}
- }, [runnerTypes, selectedRunner]);
+ }, [runnerTypes]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/new-session-view.tsx
around lines 62 - 71, The effect in the component (useEffect) is re-running on
user selection because selectedRunner is in the dependency array; change the
dependency array to only watch runnerTypes so the effect only validates when
runnerTypes change. Update the effect that currently references runnerTypes and
selectedRunner (which calls setSelectedRunner and setSelectedModel with
getDefaultModel) to remove selectedRunner from the dependency list and keep
runnerTypes only, ensuring the same logic runs only when runnerTypes are
loaded/updated.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — will address in follow-up cleanup.
Comment on lines
+55
to
+62
| const { data: integrationsStatus } = useIntegrationsStatus(); | ||
| const integrationsTotal = 4; | ||
| const integrationsConnected = [ | ||
| integrationsStatus?.github?.active != null, | ||
| integrationsStatus?.gitlab?.connected, | ||
| integrationsStatus?.jira?.connected, | ||
| integrationsStatus?.google?.connected, | ||
| ].filter(Boolean).length; |
There was a problem hiding this comment.
Keep the integrations badge out of the false 0/4 state.
Before this query resolves—and on a cold error path—integrationsStatus is undefined, so the sidebar badge immediately renders 0/4 even though the real state is still unknown. Gate the badge on isPending / isError and keep it neutral until you actually have status data.
🛠️ Suggested fix
- const { data: integrationsStatus } = useIntegrationsStatus();
+ const {
+ data: integrationsStatus,
+ isPending: isIntegrationsPending,
+ isError: isIntegrationsError,
+ } = useIntegrationsStatus();
@@
{
id: "integrations",
label: "Integrations",
icon: Link2,
- badge: `${integrationsConnected}/${integrationsTotal}`,
+ badge:
+ isIntegrationsPending || (isIntegrationsError && !integrationsStatus)
+ ? "—"
+ : `${integrationsConnected}/${integrationsTotal}`,
},Also applies to: 77-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/session-settings-modal.tsx
around lines 55 - 62, The badge shows "0/4" while useIntegrationsStatus is
unresolved because integrationsStatus is undefined; modify the logic that
computes integrationsConnected and the badge render to gate on the query state
from useIntegrationsStatus (e.g., useIntegrationsStatus().isPending / .isError /
.isLoading or check integrationsStatus presence) so you only compute
integrationsConnected when integrationsStatus exists, otherwise render a
neutral/placeholder state; update the expressions referencing integrationsStatus
(the integrationsConnected array and the place that uses integrationsTotal) to
short-circuit until the status is available.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — will address in follow-up cleanup.
Comment on lines
+16
to
+17
| const [width, setWidth] = useLocalStorage(storageKey, defaultWidth); | ||
| const [isDragging, setIsDragging] = useState(false); |
There was a problem hiding this comment.
Persist the panel width after the drag, not on every mousemove.
Here setWidth is the useLocalStorage setter, so calling it inside mousemove turns the resize loop into synchronous localStorage I/O on the main thread. Line 16 also trusts the stored value as-is, which means a stale width can render the panel outside the current min/max bounds until the next drag. Keep live drag width in memory, clamp the stored value on read, and write it back once on mouseup.
Also applies to: 42-46, 49-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/hooks/use-resize-panel.ts` around lines 16 - 17, The
code currently writes to localStorage on every mousemove by calling the
useLocalStorage setter (setWidth) and also trusts the stored width raw on read;
fix this by keeping the live dragging width in component state (e.g., a local
state like dragWidth or reuse width for in-memory during drag) and only call
setWidth once in the mouseup handler, not inside mousemove; when
initializing/reading the persisted value (the value returned by
useLocalStorage/storageKey/defaultWidth) clamp it to the allowed bounds (min/max
width constants or a clamp helper) so it never renders outside bounds before a
drag; ensure mousemove updates only the in-memory state (and respects clamping)
and mouseup persists the clamped value with setWidth and clears isDragging.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — will address in follow-up cleanup.
Comment on lines
+305
to
+320
| // Step 9: Test workflow selector on running session (now in chat input toolbar) | ||
| cy.get('body').then(($body) => { | ||
| // Workflow selector is a button in the chat input toolbar | ||
| const workflowBtn = $body.find('button:contains("No workflow"), button:contains("workflow")') | ||
| if (workflowBtn.length) { | ||
| cy.wrap(workflowBtn.first()).click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('body').then(($popover) => { | ||
| if ($popover.find(':contains("Fix a bug")').length) { | ||
| cy.contains(/Fix a bug/i).click({ force: true }) | ||
| } else { | ||
| cy.get('body').type('{esc}') | ||
| } | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Workflow-selector step can silently pass without verifying the control exists.
The if (workflowBtn.length) guard makes this step non-assertive; regressions in workflow selector rendering won’t fail the test.
Proposed fix
- cy.get('body').then(($body) => {
- // Workflow selector is a button in the chat input toolbar
- const workflowBtn = $body.find('button:contains("No workflow"), button:contains("workflow")')
- if (workflowBtn.length) {
- cy.wrap(workflowBtn.first()).click({ force: true })
+ cy.contains('button', /workflow/i, { timeout: 10000 })
+ .should('be.visible')
+ .click({ force: true })
cy.wait(500)
cy.get('body').then(($popover) => {
if ($popover.find(':contains("Fix a bug")').length) {
cy.contains(/Fix a bug/i).click({ force: true })
} else {
cy.get('body').type('{esc}')
}
})
- }
- })
+ As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/cypress/e2e/sessions.cy.ts` around lines 305 - 320, The workflow selector
check currently uses a non-assertive guard (if (workflowBtn.length)) which
allows the step to silently pass; replace this with an explicit assertion and
failure path so the test fails when the control is missing: locate the control
via the workflowBtn lookup (or prefer cy.contains/cy.get selectors), assert its
existence using a Cypress assertion (e.g., should('exist') /
should('have.length.greaterThan', 0) or expect on wrapped element), then click
it and assert the popover appears before attempting to click "Fix a bug" (assert
the popover contains "Fix a bug" and then click); avoid swallowing absence with
plain if checks in sessions.cy.ts and ensure failures surface in CI.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — will address in follow-up cleanup.
Comment on lines
+1985
to
1991
| } else if ($panel.find(':contains("No repositories added")').length) { | ||
| cy.log('Empty context state displayed correctly') | ||
| // Even in empty state, there is an "Add Context" button | ||
| const emptyAddBtn = $expanded.find('button:contains("Add Context")') | ||
| if (emptyAddBtn.length) { | ||
| cy.wrap(emptyAddBtn.first()).click({ force: true }) | ||
| cy.wait(500) | ||
| cy.get('body').type('{esc}') | ||
| cy.wait(300) | ||
| } | ||
| } | ||
| }) | ||
| } else { | ||
| cy.log('Context accordion trigger not found') | ||
| cy.log('Context tab not found') | ||
| } |
There was a problem hiding this comment.
Explorer tab tests should fail when required tabs/content are missing.
Current cy.log(...) fallbacks make these tests pass even when Context/Files panel behavior regresses.
Proposed fix
- } else {
- cy.log('Context tab not found')
- }
+ } else {
+ throw new Error('Context tab not found in Explorer panel')
+ }
...
- } else {
- cy.log('No file tree nodes found (files may be empty for pending session)')
- }
+ } else {
+ throw new Error('Files tab opened but no file tree nodes were rendered')
+ }
...
- } else {
- cy.log('Files tab not found')
- }
+ } else {
+ throw new Error('Files tab not found in Explorer panel')
+ }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
Also applies to: 2011-2016
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/cypress/e2e/sessions.cy.ts` around lines 1985 - 1991, Replace the silent
cy.log fallbacks so tests fail when required UI is missing: locate the block
that logs "Context tab not found" and the branch that logs "Empty context state
displayed correctly" (search for those exact strings) and replace those cy.log
calls with explicit test failures (e.g., throw new Error(...) or use
Cypress/Chai assertions like expect(...).to.exist or assert.fail(...)) so the
spec fails when the Context tab or expected content is absent rather than
quietly logging and passing.
Contributor
Author
There was a problem hiding this comment.
Acknowledged — will address in follow-up cleanup.
Contributor
Review Queue Status
Action needed: Fix CI failures
|
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
from
020bf34 to
7a61155
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e/cypress/e2e/sessions.cy.ts (2)
388-426:⚠️ Potential issue | 🟠 MajorSession header menu test silently passes if menu button isn't found.
The entire test body is wrapped in
if (menuBtn.length), meaning if the MoreVertical menu button is missing (e.g., due to a rendering regression), the test passes without executing any assertions. This masks potential UI regressions.Proposed fix
cy.get('button').filter(':visible').then(($buttons) => { const menuBtn = $buttons.filter((_, el) => el.querySelector('svg.lucide-more-vertical') !== null) - if (menuBtn.length) { + expect(menuBtn.length, 'Session header menu button should exist').to.be.greaterThan(0) cy.wrap(menuBtn.first()).click({ force: true }) // ... rest of interactions - } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` around lines 388 - 426, The test currently wraps all interactions in an if (menuBtn.length) guard so it silently passes if the MoreVertical menu button is missing; instead assert that the menu button exists (e.g., verify menuBtn.length > 0 or use a Cypress should/assert) immediately after computing menuBtn in sessions.cy.ts, so the test fails fast on missing UI, then proceed with the existing clicks/flows (references: the menuBtn variable and the chained cy.wrap(menuBtn.first()) interactions for View details, Export chat, Edit name, and Clone).
840-887: 🧹 Nitpick | 🔵 TrivialFeedback button tests should assert expected state rather than silently logging.
When neither
thumbsUpnorthumbsDownbuttons are found, the test logs and passes. If feedback buttons are expected on messages (even in pending sessions with agent responses), this masks regressions.Consider adding a comment clarifying why the fallback is acceptable, or restructuring the test to run only when feedback buttons are expected to exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` around lines 840 - 887, The test currently swallows a missing-feedback-buttons case by calling cy.log when neither thumbsUp nor thumbsDown are found (selectors using aria-label "This response was helpful"/"not helpful"), which masks regressions; update the test to either assert that at least one feedback button exists (e.g., expect(thumbsUp.length || thumbsDown.length).to.be.gt(0)) when feedback is expected, or explicitly guard/run this block only for sessions known to include feedback buttons and add a clarifying comment explaining the conditional; keep references to the existing symbols/selectors (thumbsUp, thumbsDown, the "Share feedback" modal title and feedback-comment/textarea) and ensure the test fails when buttons are expected but missing instead of silently passing.
♻️ Duplicate comments (2)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsx (2)
240-273:⚠️ Potential issue | 🟡 MinorUse a native button for clickable recent-session rows.
Line 240 uses
div role="button"plus manual keyboard emulation. Replace it with<button type="button">to get native semantics and keyboard behavior.Proposed fix
- <div + <button + type="button" role="button" - tabIndex={0} onClick={() => handleNavigate(session.metadata.name) } - onKeyDown={(e) => { - if (e.key === "Enter" || e.key === " ") { - e.preventDefault(); - handleNavigate(session.metadata.name); - } - }} className={cn( "w-full flex items-center gap-2 px-2 py-2 rounded-md text-left text-sm transition-colors cursor-pointer", "hover:bg-accent hover:text-accent-foreground", isActive && "bg-accent text-accent-foreground font-medium" )} > @@ - </div> + </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx around lines 240 - 273, The recent-session row currently uses a div with role="button" and manual keyboard handling; replace that div with a native <button type="button"> to get built-in keyboard and semantics. Update the element where handleNavigate(session.metadata.name) is attached (the current div that also sets tabIndex, onClick and onKeyDown) to be a button, remove tabIndex and the onKeyDown keyboard emulation, keep the onClick calling handleNavigate, preserve the same className, AgentStatusIndicator usage, and the content (name, createdAt, isActive styling) so behavior and styles remain identical but with native accessibility.
164-173:⚠️ Potential issue | 🟠 MajorFix invalid interactive nesting in sidebar navigation.
Line 164 and Line 180 currently render
Link > Button(<a><button>...</button></a>), which is invalid and can break keyboard/focus behavior in primary navigation. UseButton asChildand renderLinkinside the button.Proposed fix
- <Link href="/projects" className="block"> - <Button - variant="ghost" - size="sm" - className="w-full justify-start text-muted-foreground hover:text-foreground" - > - <ChevronLeft className="w-4 h-4 mr-2" /> - Workspaces - </Button> - </Link> + <Button + asChild + variant="ghost" + size="sm" + className="w-full justify-start text-muted-foreground hover:text-foreground" + > + <Link href="/projects"> + <ChevronLeft className="w-4 h-4 mr-2" /> + Workspaces + </Link> + </Button> @@ - <Link key={item.label} href={item.href} className="block"> - <Button - variant="ghost" - size="sm" - className={cn( - "w-full justify-start", - isActive && "bg-accent text-accent-foreground font-medium" - )} - > - <Icon className="w-4 h-4 mr-2" /> - {item.label} - </Button> - </Link> + <Button + asChild + key={item.label} + variant="ghost" + size="sm" + className={cn( + "w-full justify-start", + isActive && "bg-accent text-accent-foreground font-medium" + )} + > + <Link href={item.href}> + <Icon className="w-4 h-4 mr-2" /> + {item.label} + </Link> + </Button>Also applies to: 180-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx around lines 164 - 173, The current markup nests a <Button> inside a <Link> (Link > Button) which creates invalid interactive nesting; update both occurrences (the Workspaces and the other sidebar link) to use the Button component's asChild prop and render the Link inside the Button instead (i.e., <Button asChild> <Link href="...">...</Link> </Button>), keeping the existing props/classes (variant="ghost", size="sm", className="w-full justify-start ...") and the ChevronLeft icon usage unchanged so keyboard/focus behavior is correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@e2e/cypress/e2e/sessions.cy.ts`:
- Around line 388-426: The test currently wraps all interactions in an if
(menuBtn.length) guard so it silently passes if the MoreVertical menu button is
missing; instead assert that the menu button exists (e.g., verify menuBtn.length
> 0 or use a Cypress should/assert) immediately after computing menuBtn in
sessions.cy.ts, so the test fails fast on missing UI, then proceed with the
existing clicks/flows (references: the menuBtn variable and the chained
cy.wrap(menuBtn.first()) interactions for View details, Export chat, Edit name,
and Clone).
- Around line 840-887: The test currently swallows a missing-feedback-buttons
case by calling cy.log when neither thumbsUp nor thumbsDown are found (selectors
using aria-label "This response was helpful"/"not helpful"), which masks
regressions; update the test to either assert that at least one feedback button
exists (e.g., expect(thumbsUp.length || thumbsDown.length).to.be.gt(0)) when
feedback is expected, or explicitly guard/run this block only for sessions known
to include feedback buttons and add a clarifying comment explaining the
conditional; keep references to the existing symbols/selectors (thumbsUp,
thumbsDown, the "Share feedback" modal title and feedback-comment/textarea) and
ensure the test fails when buttons are expected but missing instead of silently
passing.
---
Duplicate comments:
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx:
- Around line 240-273: The recent-session row currently uses a div with
role="button" and manual keyboard handling; replace that div with a native
<button type="button"> to get built-in keyboard and semantics. Update the
element where handleNavigate(session.metadata.name) is attached (the current div
that also sets tabIndex, onClick and onKeyDown) to be a button, remove tabIndex
and the onKeyDown keyboard emulation, keep the onClick calling handleNavigate,
preserve the same className, AgentStatusIndicator usage, and the content (name,
createdAt, isActive styling) so behavior and styles remain identical but with
native accessibility.
- Around line 164-173: The current markup nests a <Button> inside a <Link> (Link
> Button) which creates invalid interactive nesting; update both occurrences
(the Workspaces and the other sidebar link) to use the Button component's
asChild prop and render the Link inside the Button instead (i.e., <Button
asChild> <Link href="...">...</Link> </Button>), keeping the existing
props/classes (variant="ghost", size="sm", className="w-full justify-start ...")
and the ChevronLeft icon usage unchanged so keyboard/focus behavior is correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c82b84f-7d68-49cf-854b-4c79076e3978
📒 Files selected for processing (2)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsxe2e/cypress/e2e/sessions.cy.ts
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
4 times, most recently
from
1306bea to
a8e4ecc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsx (2)
240-252:⚠️ Potential issue | 🟡 MinorUse a native button for recents row trigger.
Line 240-Line 252 still emulates button behavior with
div role="button"+ keyboard handlers. This was already flagged in prior review; using<button type="button">is the correct semantic control and removes manual key handling.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx around lines 240 - 252, Replace the non-semantic clickable div in the recents row with a native <button type="button">: in the component (sessions-sidebar.tsx) where the div uses role="button", tabIndex, onClick, and onKeyDown to call handleNavigate(session.metadata.name) and className={cn(...)}; change that element to a button, keep the onClick={() => handleNavigate(session.metadata.name)} and the className={cn(...)} prop, and remove role, tabIndex and the manual onKeyDown keyboard handling since a native button provides correct semantics and keyboard behavior automatically.
164-173:⚠️ Potential issue | 🟠 MajorAvoid nested interactive elements in sidebar navigation.
These blocks still render
LinkwrappingButton(<a><button>...), which is invalid and can break keyboard/focus behavior. This was already flagged in prior review and is still present.♻️ Suggested fix
- <Link href="/projects" className="block"> - <Button - variant="ghost" - size="sm" - className="w-full justify-start text-muted-foreground hover:text-foreground" - > - <ChevronLeft className="w-4 h-4 mr-2" /> - Workspaces - </Button> - </Link> + <Button + asChild + variant="ghost" + size="sm" + className="w-full justify-start text-muted-foreground hover:text-foreground" + > + <Link href="/projects"> + <ChevronLeft className="w-4 h-4 mr-2" /> + Workspaces + </Link> + </Button> @@ - <Link key={item.label} href={item.href} className="block"> - <Button - variant="ghost" - size="sm" - className={cn( - "w-full justify-start", - isActive && "bg-accent text-accent-foreground font-medium" - )} - > - <Icon className="w-4 h-4 mr-2" /> - {item.label} - </Button> - </Link> + <Button + asChild + key={item.label} + variant="ghost" + size="sm" + className={cn( + "w-full justify-start", + isActive && "bg-accent text-accent-foreground font-medium" + )} + > + <Link href={item.href}> + <Icon className="w-4 h-4 mr-2" /> + {item.label} + </Link> + </Button>As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 180-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx around lines 164 - 173, The Link currently wraps a Button (Link > Button > ChevronLeft) which creates nested interactive elements; replace the pattern so only a single interactive element is rendered — either render Link as the interactive element and apply the Button styling/classes to it (move className, variant/size styles to the Link and keep ChevronLeft and text inside), or update the Button to render an anchor (e.g., support an href/asChild pattern) and use Button with href instead of wrapping it in Link; locate the block containing Link, Button and ChevronLeft in the sessions-sidebar component and implement one of these approaches consistently (also fix the similar block around the 180-192 region).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx:
- Around line 62-65: The sidebar always requests 100 sessions via
useSessionsPaginated(collapsed ? "" : projectName, { limit: 100 }) which is
overkill for the visible recents list and causes extra network and render work;
change to request a small initial page (e.g., limit: 10) and implement
incremental loading: set the initial call in the SessionsSidebar component to
useSessionsPaginated(..., { limit: 10 }), surface a "Load more" or auto-fetch
handler that calls the returned refetch/nextPage when the user requests more,
and ensure collapsed/expanded logic (collapsed, projectName) still triggers the
correct small fetch so periodic refetches only refresh the small page until more
is explicitly requested.
---
Duplicate comments:
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/sessions-sidebar.tsx:
- Around line 240-252: Replace the non-semantic clickable div in the recents row
with a native <button type="button">: in the component (sessions-sidebar.tsx)
where the div uses role="button", tabIndex, onClick, and onKeyDown to call
handleNavigate(session.metadata.name) and className={cn(...)}; change that
element to a button, keep the onClick={() =>
handleNavigate(session.metadata.name)} and the className={cn(...)} prop, and
remove role, tabIndex and the manual onKeyDown keyboard handling since a native
button provides correct semantics and keyboard behavior automatically.
- Around line 164-173: The Link currently wraps a Button (Link > Button >
ChevronLeft) which creates nested interactive elements; replace the pattern so
only a single interactive element is rendered — either render Link as the
interactive element and apply the Button styling/classes to it (move className,
variant/size styles to the Link and keep ChevronLeft and text inside), or update
the Button to render an anchor (e.g., support an href/asChild pattern) and use
Button with href instead of wrapping it in Link; locate the block containing
Link, Button and ChevronLeft in the sessions-sidebar component and implement one
of these approaches consistently (also fix the similar block around the 180-192
region).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d1e96f2-fb24-4cbb-8fc8-c650a559cf10
📒 Files selected for processing (2)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsxe2e/cypress/e2e/sessions.cy.ts
...ents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (5)
e2e/cypress/e2e/sessions.cy.ts (5)
953-953:⚠️ Potential issue | 🟠 MajorFix modal-title condition (
> 1skips valid single-match cases).This check should pass when at least one
"Add Repository"match exists; otherwise URL input assertions are skipped.Proposed fix
- if ($modal.find(':contains("Add Repository")').length > 1) { + if ($modal.find(':contains("Add Repository")').length > 0) {As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` at line 953, The modal-title check currently uses $modal.find(':contains("Add Repository")').length > 1 which skips valid single-match cases; update the condition in the sessions.cy.ts test to assert presence when there is at least one match (use .length > 0 or .length >= 1) so the URL input assertions run for a single "Add Repository" modal; locate the check using the $modal variable and the ':contains("Add Repository")' selector and replace the comparison accordingly.
946-947:⚠️ Potential issue | 🟠 Major
button:contains("Add")is too broad and makes modal flow nondeterministic.Including generic
"Add"can click unrelated controls; scope to the intended action only.Proposed fix
- const addBtn = $body.find('button:contains("Add Repository"), button:contains("Add")') + const addBtn = $body.find('button:contains("Add Repository")') ... - const addBtn = $panel.find('button:contains("Add Repository"), button:contains("Add")') + const addBtn = $panel.find('button:contains("Add Repository")')As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
Also applies to: 1954-1955
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` around lines 946 - 947, The selector for addBtn is too broad: $body.find('button:contains("Add Repository"), button:contains("Add")') — remove the generic button:contains("Add") and scope the selector to the intended control (e.g., use the specific text "Add Repository", limit the search to the modal container, or prefer a test-specific attribute like data-testid or aria-label). Update the $body.find call that assigns addBtn so it only matches the repository-add button (refer to the addBtn variable and the $body.find(...) invocation) to make the modal flow deterministic.
291-306:⚠️ Potential issue | 🟠 MajorMake workflow-selector validation assertive (don’t silently skip).
The
if (workflowBtn.length)guard allows this step to pass even when the workflow selector is missing.Proposed fix
- cy.get('body').then(($body) => { - // Workflow selector is a button in the chat input toolbar - const workflowBtn = $body.find('button:contains("No workflow"), button:contains("workflow")') - if (workflowBtn.length) { - cy.wrap(workflowBtn.first()).click({ force: true }) - cy.wait(500) - cy.get('body').then(($popover) => { - if ($popover.find(':contains("Fix a bug")').length) { - cy.contains(/Fix a bug/i).click({ force: true }) - } else { - cy.get('body').type('{esc}') - } - }) - } - }) + cy.contains('button', /workflow/i, { timeout: 10000 }) + .should('be.visible') + .click({ force: true }) + cy.contains(/Fix a bug/i, { timeout: 5000 }).should('be.visible').click({ force: true })As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` around lines 291 - 306, The current test silently skips when the workflow selector is missing because of the if (workflowBtn.length) guard; replace this non-assertive guard with explicit Cypress assertions so the test fails if the workflow button or the "Fix a bug" option are not present. Specifically, stop using the manual workflowBtn length check and instead use assertive selectors (e.g., cy.get('button:contains("No workflow"), button:contains("workflow")').should('exist') and then .first().click(...)), and after opening the popover assert cy.contains(/Fix a bug/i).should('exist') before clicking it (otherwise fail the test) — keep the click/escape behavior but make existence checks use should('exist') on the same selectors referenced above.
1971-1977:⚠️ Potential issue | 🟠 MajorReplace log-only fallbacks with explicit failures in Explorer tab checks.
cy.log(...)here masks regressions (missing tabs/content) and allows false-positive test passes.Proposed fix
- } else { - cy.log('Context tab not found') - } + } else { + throw new Error('Context tab not found in Explorer panel') + } ... - } else { - cy.log('No file tree nodes found (files may be empty for pending session)') - } + } else { + throw new Error('Files tab opened but no file tree nodes were rendered') + } ... - } else { - cy.log('Files tab not found') - } + } else { + throw new Error('Files tab not found in Explorer panel') + }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
Also applies to: 1997-2002
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` around lines 1971 - 1977, Replace the silent cy.log fallbacks used in the Explorer tab checks with explicit test failures and assertions: where the code currently calls cy.log('Context tab not found') or cy.log('Empty context state displayed correctly'), assert the tab/panel exists and the empty-state text is visible (e.g., use cy.contains(..., 'Explorer')/cy.get(...).should('exist') and cy.get(...).contains('No repositories added').should('be.visible') or throw an Error with a clear message). Update both occurrences (the block testing the Explorer/Context tab near the cy.log calls and the duplicate at the other occurrence around lines 1997-2002) so missing tabs or content cause failing assertions instead of logging.
125-131:⚠️ Potential issue | 🟠 MajorUse a deterministic selector for Workspaces navigation click.
cy.contains('Workspaces').click({ force: true })is ambiguous and can hit non-navigation elements, making this test flaky.Proposed fix
- cy.contains('Workspaces').click({ force: true }) + cy.get('a[href="/projects"]').first().should('be.visible').click()As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cypress/e2e/sessions.cy.ts` around lines 125 - 131, The test "should display sidebar navigation and navigate" uses an ambiguous cy.contains('Workspaces').click({ force: true }) which can target non-navigation elements; change it to a deterministic selector (e.g., a data attribute or explicit anchor selector) such as replacing that line with a stable query like cy.get('[data-cy="nav-workspaces"]').should('be.visible').click({ force: true }) or cy.get('a[href="/projects"]').should('be.visible').click({ force: true }); update the test to assert visibility on that deterministic selector before clicking to avoid flakiness and keep the rest of the URL assertion (cy.url().should('include', '/projects')) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@e2e/cypress/e2e/sessions.cy.ts`:
- Line 953: The modal-title check currently uses $modal.find(':contains("Add
Repository")').length > 1 which skips valid single-match cases; update the
condition in the sessions.cy.ts test to assert presence when there is at least
one match (use .length > 0 or .length >= 1) so the URL input assertions run for
a single "Add Repository" modal; locate the check using the $modal variable and
the ':contains("Add Repository")' selector and replace the comparison
accordingly.
- Around line 946-947: The selector for addBtn is too broad:
$body.find('button:contains("Add Repository"), button:contains("Add")') — remove
the generic button:contains("Add") and scope the selector to the intended
control (e.g., use the specific text "Add Repository", limit the search to the
modal container, or prefer a test-specific attribute like data-testid or
aria-label). Update the $body.find call that assigns addBtn so it only matches
the repository-add button (refer to the addBtn variable and the $body.find(...)
invocation) to make the modal flow deterministic.
- Around line 291-306: The current test silently skips when the workflow
selector is missing because of the if (workflowBtn.length) guard; replace this
non-assertive guard with explicit Cypress assertions so the test fails if the
workflow button or the "Fix a bug" option are not present. Specifically, stop
using the manual workflowBtn length check and instead use assertive selectors
(e.g., cy.get('button:contains("No workflow"),
button:contains("workflow")').should('exist') and then .first().click(...)), and
after opening the popover assert cy.contains(/Fix a bug/i).should('exist')
before clicking it (otherwise fail the test) — keep the click/escape behavior
but make existence checks use should('exist') on the same selectors referenced
above.
- Around line 1971-1977: Replace the silent cy.log fallbacks used in the
Explorer tab checks with explicit test failures and assertions: where the code
currently calls cy.log('Context tab not found') or cy.log('Empty context state
displayed correctly'), assert the tab/panel exists and the empty-state text is
visible (e.g., use cy.contains(..., 'Explorer')/cy.get(...).should('exist') and
cy.get(...).contains('No repositories added').should('be.visible') or throw an
Error with a clear message). Update both occurrences (the block testing the
Explorer/Context tab near the cy.log calls and the duplicate at the other
occurrence around lines 1997-2002) so missing tabs or content cause failing
assertions instead of logging.
- Around line 125-131: The test "should display sidebar navigation and navigate"
uses an ambiguous cy.contains('Workspaces').click({ force: true }) which can
target non-navigation elements; change it to a deterministic selector (e.g., a
data attribute or explicit anchor selector) such as replacing that line with a
stable query like
cy.get('[data-cy="nav-workspaces"]').should('be.visible').click({ force: true })
or cy.get('a[href="/projects"]').should('be.visible').click({ force: true });
update the test to assert visibility on that deterministic selector before
clicking to avoid flakiness and keep the rest of the URL assertion
(cy.url().should('include', '/projects')) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57a91be0-9d1c-4b46-bcc9-d31d0fc830c8
📒 Files selected for processing (2)
components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/sessions-sidebar.tsxe2e/cypress/e2e/sessions.cy.ts
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
5 times, most recently
from
1aa3cef to
777c863
Compare
Contributor
Author
CodeRabbit Review — Addressed ItemsThe following issues flagged by CodeRabbit have been fixed in the latest push:
|
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
6 times, most recently
from
f26df08 to
3444cba
Compare
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
2 times, most recently
from
451be9d to
2ee27c8
Compare
Replace session phase dots with AgentStatusIndicator in the sidebar recents list, showing working/idle/needs-input/error states. Add HoverCard tooltips (model, time, prompt) matching the sessions list page. Add manual refresh button to the Recents header. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Gkrumbach07
force-pushed
the
feat/remove-sidepanel
branch
from
2ee27c8 to
b33011b
Compare
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
Key Changes
Layout Architecture
app/projects/[name]/layout.tsx) with persistent sidebar across all workspace routesNew Components (20 new files)
sessions-sidebar.tsx— workspace nav + recentscontent-tabs.tsx— Chat tab + closable file tabsfile-viewer.tsx— syntax-highlighted code viewer with line numbersexplorer/explorer-panel.tsx,files-tab.tsx,context-tab.tsxsession-settings-modal.tsx+ settings sub-componentsworkflow-selector.tsx— dropdown in chat input toolbarnew-session-view.tsx— ChatGPT-style empty state with runner/model selectorrunner-model-selector.tsx— cascading runner type + model dropdownnavigation-wrapper.tsx— hides global nav on project pagesRestyled Chat Input
+dropdown with "Add Repository" and "Upload File" actionsRoute Changes
/projects/[name]→ redirects to/new/projects/[name]/new— new session page/projects/[name]/sessions— sessions list (renders directly, no redirect loop)/projects/[name]/scheduled-sessions— schedules page (new index route)Backend
initialPromptis provided without explicitdisplayNameTest plan
/newpage with runner/model/workflow/repo selectionResolves: RHOAIENG-53563
🤖 Generated with Claude Code