Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces a complete workspace management feature to the platform, including new OpenAPI endpoint definitions for workspace CRUD operations, backend domain models with file-based persistence, configuration updates, API handlers, and a comprehensive React-based UI with workspace selection, DAG run Kanban visualization, and cockpit page integration. Additionally, it adds a new DAG code generation tool for the agent and updates navigation menu structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Cockpit UI
participant API as Backend API
participant Store as Workspace Store
participant FS as File System
User->>UI: Select/Create Workspace
UI->>API: POST /workspaces or GET /workspaces
API->>Store: Create(ctx, ws) or List(ctx)
Store->>FS: Write/Read JSON files
FS-->>Store: File I/O complete
Store-->>API: Workspace(s) returned
API-->>UI: WorkspaceResponse(s)
UI->>UI: Update selectedWorkspace state
User->>UI: Select DAG Template
UI->>API: Fetch DAG metadata
API-->>UI: DAG details
UI->>UI: Display DAGPreviewModal
User->>UI: Click Enqueue on DAG
UI->>UI: Inject workspace tag into DAG spec
UI->>API: POST /dag-runs/enqueue with workspace tag
API-->>UI: DAGRun created
UI->>UI: Display run in Kanban board
sequenceDiagram
participant User as User
participant Cockpit as Cockpit Page
participant Kanban as DateKanbanList
participant SSE as SSE Stream
participant API as Backend API
Cockpit->>Kanban: Render with selectedWorkspace
Kanban->>Kanban: useInfiniteKanban → loadedDates
loop For Each Loaded Date
Kanban->>API: Fetch DAG runs for date range
API-->>Kanban: DAG runs
Kanban->>Kanban: useDateKanbanData → group by status
Kanban->>Kanban: Render DateKanbanSection
end
Note over Cockpit: Real-time updates via SSE
Kanban->>SSE: Subscribe to DAG run updates (if isToday)
SSE-->>Kanban: New/updated run
Kanban->>Kanban: Update columns via mutation
Kanban->>Kanban: Re-render Kanban cards
User->>Kanban: Scroll to bottom
Kanban->>Kanban: IntersectionObserver triggers
Kanban->>Kanban: loadNextDate()
Kanban->>API: Fetch previous day runs
API-->>Kanban: Older runs
Kanban->>Kanban: Append to loadedDates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces a cohesive but substantial feature spanning backend domain models, persistence layer, API endpoints, handlers, and an extensive React component tree with custom hooks. While individual changes follow consistent patterns (workspace CRUD, Kanban components, hook definitions), the heterogeneous nature across multiple domains (Go backend, TypeScript/React frontend, OpenAPI definitions, configuration) and the interplay between workspace selection, DAG run visualization, and enqueue mechanics require careful review of control flow integration points, state management patterns, error handling consistency, and permission checks. Possibly related PRs
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
ui/src/features/cockpit/components/TemplateSelector.tsx-76-85 (1)
76-85:⚠️ Potential issue | 🟡 MinorClear cached selection when the selected template is no longer present.
If
filteredDagsno longer containsselectedTemplate,selectedDagstays stale and can display the wrong label.Suggested fix
- const found = filteredDags.find((d) => d.fileName === selectedTemplate); - if (found) { - setSelectedDag(found); - } + const found = filteredDags.find((d) => d.fileName === selectedTemplate); + setSelectedDag(found ?? null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/TemplateSelector.tsx` around lines 76 - 85, The effect in useEffect that syncs selectedTemplate with selectedDag can leave selectedDag stale when filteredDags no longer contains the selectedTemplate; update the effect so that after searching filteredDags.find((d) => d.fileName === selectedTemplate) you explicitly clear the selection when not found (call setSelectedDag(null)) in addition to setting it when found, keeping the same dependency array [selectedTemplate, filteredDags] and referencing the existing symbols selectedTemplate, filteredDags, setSelectedDag, and selectedDag.ui/src/features/cockpit/components/WorkspaceSelector.tsx-58-61 (1)
58-61:⚠️ Potential issue | 🟡 MinorUse explicit compact sizing/padding for the input and select trigger.
These controls currently rely on component defaults, which can render taller than the surrounding compact toolbar controls.
Suggested class adjustments
- className="px-2 text-xs w-40" + className="h-7 w-40 px-2 py-0.5 text-xs bg-background" ... - <SelectTrigger className="text-xs w-40"> + <SelectTrigger className="h-7 w-40 px-2 py-0.5 text-xs bg-background">As per coding guidelines "Use information-dense, compact UI design with minimal whitespace, small form element heights (h-7 or smaller for select boxes), and reduced padding" and "Ensure all form inputs use compact padding (py-0.5 or py-1) and consistent background colors matching related elements".
Also applies to: 82-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/WorkspaceSelector.tsx` around lines 58 - 61, The Input control (ref inputRef) and the corresponding Select trigger should use explicit compact sizing and padding rather than rely on defaults; update the Input's className to include a fixed height (h-7), compact vertical padding (py-0.5), small text (text-xs), tight line-height (leading-none) and existing horizontal padding/width (px-2 w-40) and apply the same class set to the Select trigger element so both controls match in height, padding and background color (use the same bg-... utility or remove bg to inherit) for consistent, information-dense toolbar styling.ui/src/features/cockpit/components/TemplateSelector.tsx-318-339 (1)
318-339:⚠️ Potential issue | 🟡 MinorUse wrapping classes for long DAG names/descriptions in the list.
Current
truncateusage clips content; this list should wrap long strings to avoid losing essential metadata.Suggested class changes
- <span className={cn( - "font-medium text-xs truncate", + <span className={cn( + "font-medium text-xs whitespace-normal break-words", dag.errors?.length > 0 && "text-destructive" )}> ... - <div className="text-[11px] text-muted-foreground truncate"> + <div className="text-[11px] text-muted-foreground whitespace-normal break-words"> {description} </div>As per coding guidelines "Always handle long text in tables and lists with
whitespace-normal break-wordsto prevent layout overflow".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/TemplateSelector.tsx` around lines 318 - 339, The list currently uses "truncate" on the DAG name and description in TemplateSelector (the span with className built via cn for dag.dag.name and the description div), which clips long text; replace those "truncate" uses with wrapping classes "whitespace-normal break-words" (and remove/avoid "truncate") so long DAG names and descriptions wrap instead of being clipped, and ensure the parent container (the surrounding div rendering the name/description) allows wrapping by not forcing no-wrap behavior.ui/src/features/cockpit/components/EmbeddedAgentChat.tsx-82-86 (1)
82-86:⚠️ Potential issue | 🟡 MinorDon’t swallow send failures silently.
sendMessage(...).catch(() => {})drops actionable errors and can leave users without feedback when message send fails.Suggested error handling
- const handleSend = useCallback( - (message: string, dagContexts?: DAGContext[], model?: string, soulId?: string): void => { - setInitialInputValue(null); - sendMessage(message, model, dagContexts, soulId).catch(() => {}); - }, - [sendMessage, setInitialInputValue] - ); + const handleSend = useCallback( + async (message: string, dagContexts?: DAGContext[], model?: string, soulId?: string): Promise<void> => { + setInitialInputValue(null); + try { + await sendMessage(message, model, dagContexts, soulId); + } catch (err) { + setError(err instanceof Error ? err.message : 'Failed to send message'); + } + }, + [sendMessage, setInitialInputValue, setError] + );As per coding guidelines "Use error handling with try/catch for async operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/EmbeddedAgentChat.tsx` around lines 82 - 86, Replace the silent promise swallow in handleSend by making the callback async and wrapping the sendMessage call in try/catch: in the handleSend function (which calls setInitialInputValue and sendMessage), await sendMessage(message, model, dagContexts, soulId) inside a try block, and in the catch block log the error and surface user feedback (e.g., call the app's toast/error handler or set an error state) so failures are visible; ensure you still clear or restore input via setInitialInputValue as appropriate and do not leave the promise unhandled.ui/src/App.tsx-237-237 (1)
237-237:⚠️ Potential issue | 🟡 MinorWrap cockpit route with feature guard or add disabled state handling.
The
/cockpitroute is accessible to all authenticated users, but the menu item is only shown whenconfig.agentEnabledis true. Users can navigate directly to/cockpiteven when the agent feature is disabled.The codebase uses route-level guards for similar feature-gated pages (e.g.,
<DeveloperElement>for/system-status,<LicensedRoute>for/audit-logs). Apply the same pattern here:
- Either wrap the route with a feature guard component
- Or have
CockpitPagegracefully handle the disabled state and show an appropriate message🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/App.tsx` at line 237, The /cockpit route is exposed even when the agent feature is disabled; update the routing so access is gated by the same feature-guard pattern used elsewhere: wrap the Route element for path="/cockpit" (currently rendering CockpitPage) with a feature guard that checks config.agentEnabled (e.g., create/ reuse a guard similar to DeveloperElement or LicensedRoute that returns <Navigate/> or a “feature disabled” fallback when false), or alternatively update CockpitPage to render a graceful disabled-state UI when config.agentEnabled is false; ensure you reference the Route path="/cockpit", CockpitPage, and config.agentEnabled when implementing the guard or the in-component handling.ui/src/features/cockpit/components/KanbanCard.tsx-47-50 (1)
47-50:⚠️ Potential issue | 🟡 MinorSet explicit button type to prevent accidental form submit.
<button>defaults totype="submit". If this card is ever rendered inside a form, clicking it can trigger unintended submission.🔧 Proposed fix
<button + type="button" onClick={onClick} className="w-full text-left p-2 rounded-md border border-border bg-card hover:bg-accent/50 transition-colors cursor-pointer" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/KanbanCard.tsx` around lines 47 - 50, In the KanbanCard component update the JSX button element to include an explicit type attribute (type="button") so clicks don't act as form submissions; locate the <button ... onClick={onClick} className="w-full text-left p-2 rounded-md border border-border bg-card hover:bg-accent/50 transition-colors cursor-pointer"> and add type="button" to it to prevent accidental submits when rendered inside a form.api/v1/api.yaml-9675-9691 (1)
9675-9691:⚠️ Potential issue | 🟡 MinorAdd non-empty validation for workspace
name.
namecurrently permits empty strings in both create and update payloads, which weakens contract validation.🛠️ Suggested schema hardening
CreateWorkspaceRequest: type: object required: - name properties: name: type: string + minLength: 1 description: type: string UpdateWorkspaceRequest: type: object properties: name: type: string + minLength: 1 description: type: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 9675 - 9691, Add non-empty validation to the workspace "name" field by updating both CreateWorkspaceRequest and UpdateWorkspaceRequest: add a constraint like minLength: 1 (or an equivalent pattern) to the name property so empty strings are rejected; keep name required on CreateWorkspaceRequest and ensure UpdateWorkspaceRequest's name, when present, also enforces minLength: 1.
🧹 Nitpick comments (4)
ui/src/menu.tsx (1)
266-270: MissingisExpandedin useEffect dependency array is intentional but should be documented.The missing dependency is intentional to prevent collapsing when manually toggled, but adding a comment would clarify this to future maintainers.
💡 Suggested documentation
React.useEffect(() => { + // Intentionally omitting isExpanded to only auto-expand on route navigation, + // not when user manually toggles the group if (isChildActive && !isExpanded) { setIsExpanded(true); } }, [isChildActive]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/menu.tsx` around lines 266 - 270, Add an inline comment above the React.useEffect that references isChildActive, isExpanded, and setIsExpanded explaining that omitting isExpanded from the dependency array is intentional: the effect should only expand the item when a child becomes active (isChildActive changes) but must not collapse when isExpanded is toggled manually, so only isChildActive is listed as a dependency. Keep the comment concise and mention the behavioral intent to prevent automatic collapsing.ui/src/features/cockpit/components/DAGPreviewModal.tsx (1)
210-213: Use denser modal padding/header spacing for cockpit UI consistency.Current
p-6+mb-4makes this panel visually looser than the compact cockpit patterns.🎨 Suggested refactor
- <div className="p-6 w-full flex flex-col h-full dag-modal-content"> + <div className="p-3 w-full flex flex-col h-full dag-modal-content"> @@ - <div className="flex justify-between items-center mb-4"> + <div className="flex justify-between items-center mb-2">As per coding guidelines "Keep modal headers small and information-dense with minimal padding (p-2 or p-3 instead of p-4 or p-6) and support keyboard navigation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/cockpit/components/DAGPreviewModal.tsx` around lines 210 - 213, Replace the loose modal spacing in DAGPreviewModal by changing the wrapper div's className from "p-6 w-full flex flex-col h-full dag-modal-content" to a denser padding (e.g., "p-3 w-full flex flex-col h-full dag-modal-content") and reduce the toolbar bottom margin from "mb-4" to "mb-2" so the header is visually compact and matches cockpit patterns; after updating classes, verify the toolbar and modal header elements in DAGPreviewModal remain keyboard-focusable (tab/escape handling) to preserve keyboard navigation.api/v1/api.yaml (1)
5962-5967: ExtractworkspaceIdinto a reusable parameter component.
workspaceIdis defined inline three times; centralizing it undercomponents/parameterswill prevent drift.♻️ Suggested refactor
/workspaces/{workspaceId}: get: parameters: - - name: workspaceId - in: path - required: true - schema: - type: string + - $ref: "#/components/parameters/WorkspaceId" - $ref: "#/components/parameters/RemoteNode" ... patch: parameters: - - name: workspaceId - in: path - required: true - schema: - type: string + - $ref: "#/components/parameters/WorkspaceId" - $ref: "#/components/parameters/RemoteNode" ... delete: parameters: - - name: workspaceId - in: path - required: true - schema: - type: string + - $ref: "#/components/parameters/WorkspaceId" - $ref: "#/components/parameters/RemoteNode"parameters: + WorkspaceId: + name: workspaceId + in: path + required: true + description: "The unique identifier of the workspace" + schema: + type: string + minLength: 1Also applies to: 5987-5992, 6024-6029
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 5962 - 5967, The inline workspaceId path parameter is duplicated; extract it into a reusable components/parameters entry named e.g. WorkspaceId (define schema type: string and required:true, in:path) and replace each inline definition (the blocks where name: workspaceId in:path required:true schema:type:string — occurrences near the existing RemoteNode ref) with a $ref: "#/components/parameters/WorkspaceId"; update all three occurrences so they reference the new component to prevent drift.ui/src/api/v1/schema.ts (1)
11660-11840: Consider addingdefaulterror responses for workspace operations for type-safety parity.Most operations in this file expose a
defaulterror response; workspace operations currently do not. Adding them in the OpenAPI source will make client-side error handling more uniform after regeneration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/api/v1/schema.ts` around lines 11660 - 11840, The workspace endpoints (listWorkspaces, createWorkspace, getWorkspace, deleteWorkspace, updateWorkspace) lack a default error response; add a default response entry to each operation's responses using the existing Error schema (i.e. default: { headers: { [name: string]: unknown }, content: { "application/json": components["schemas"]["Error"] } }) so client-side error handling is uniform, then regenerate the OpenAPI clients/spec artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/agent/dag_codegen_test.go`:
- Line 1: The new test file (dag_codegen_test.go, package agent) is missing the
required GPL v3 license header; add the repository-standard header by running
the repository tool or adding the header manually: run `make addlicense` (which
applies the GPL v3 header to source files) or insert the project's GPL v3
license header at the top of dag_codegen_test.go so the file complies with the
repository's license header policy.
In `@internal/cmn/config/loader.go`:
- Line 966: The WorkspacesDir field is only added as a default
({&cfg.Paths.WorkspacesDir, "workspaces"}) but not included in path resolution
or environment binding, causing inconsistent behavior; update loadPathsConfig to
include paths.workspaces_dir in the resolution loop so relative values for
cfg.Paths.WorkspacesDir are resolved the same as other path fields, and add an
env binding entry for "paths.workspaces_dir" (or the existing env naming
convention used for other paths) so WorkspacesDir can be overridden from the
environment; locate references to WorkspacesDir, loadPathsConfig, and the paths
binding map in loader.go to mirror the handling used for the other path fields
and ensure the default, resolution, and env binding are all wired together.
In `@internal/service/frontend/api/v1/workspaces.go`:
- Around line 17-19: The handlers currently check a.workspaceStore == nil but
return inconsistent responses (e.g., ListWorkspaces uses
ListWorkspaces200JSONResponse with empty slice while other handlers return 404
or 503); treat a nil workspaceStore as an infrastructure/service unavailable
error across all endpoints. Update each handler that uses workspaceStore (refer
to checks for workspaceStore and response constructors like
ListWorkspaces200JSONResponse and the other handlers around the ranges noted) to
return a consistent ServiceUnavailable response (use the API’s
ServiceUnavailable response builder and include a short error message) instead
of 200 or 404 when workspaceStore == nil so all paths signal an unavailable
store.
In `@internal/workspace/workspace.go`:
- Line 1: The new file begins with "package workspace" but lacks the required
GPLv3 license header; run the repository tooling to add it (e.g., execute the
prescribed command such as make addlicense) or prepend the standard GPL v3
header to the top of the file so that the file containing package workspace
includes the repository-required license comment block.
In `@ui/src/features/cockpit/components/DAGPreviewModal.tsx`:
- Around line 153-156: The window.open call in DAGPreviewModal.tsx (where url =
`/dags/${fileName}/spec` and you call window.open(url, '_blank')) must include
no-opener and no-referrer to prevent the new page from accessing window.opener;
update the window.open invocation to include the security flags (e.g., pass
"noopener,noreferrer" as the third argument or use an anchor with rel="noopener
noreferrer"), or alternatively set the opened window's opener to null after
opening; change the code around the window.open call in the DAGPreviewModal
component accordingly.
- Around line 164-182: The keydown handler is registered even when the modal is
closed; update the useEffect that defines handleKeyDown to only attach the
listener while the modal is open (e.g., depend on an isOpen prop/state and
early-return if not open), and ensure you still remove the listener when the
modal closes; modify the dependency array to include isOpen and keep references
to shouldIgnoreKeyboardShortcuts, onClose, and handleFullscreenClick so Escape
and 'f' only act when the modal is open.
In `@ui/src/features/cockpit/components/EmbeddedAgentChat.tsx`:
- Around line 113-115: The icon-only sidebar toggle button in
EmbeddedAgentChat.tsx is missing accessible properties; update the button that
uses toggleSidebar and the PanelLeftClose/PanelLeftOpen icons to include
type="button" and an appropriate aria-label (e.g., "Toggle sidebar" or "Open
sidebar" / "Close sidebar" depending on sidebarOpen) so screen readers can
identify the action. Do the same for the error-dismiss action button (the button
that clears/dismisses errors) by adding type="button" and an aria-label like
"Dismiss error" while keeping its existing onClick handler intact.
In `@ui/src/features/cockpit/components/TemplateSelector.tsx`:
- Around line 199-222: The trigger button currently nests another <button>
(clear action) inside it which breaks HTML semantics and accessibility; refactor
TemplateSelector so the clear action is not a nested button: move the clear
control out of the main trigger rendered by handleOpen (or replace the inner
<button> with a non-nested interactive element such as a <span> or <div> with
role="button" and proper keyboard handlers), keep the clear logic using
onSelect('') and setSelectedDag(null) but remove e.stopPropagation(), ensure the
new control has an accessible label (aria-label) and handles onClick and
onKeyDown to support keyboard users, and preserve styles/icons (X) and visual
layout currently tied to selectedDagName.
In `@ui/src/features/cockpit/components/WorkspaceSelector.tsx`:
- Around line 37-65: Race between Enter key and blur causes duplicate onCreate
calls; fix by adding a ref guard (e.g., createdRef or submittingRef) checked
inside handleCreate so subsequent calls are no-ops after the first invocation,
set that ref to true before calling onCreate and before calling
setIsCreating(false), and reset it when opening creation flow; also update
handleKeyDown to set the guard when handling Enter or Escape (for Escape set the
guard to prevent onBlur-triggered create) and include that same guard check in
the Input onBlur handler path; finally adjust SelectTrigger's padding from
"py-2" to a compact value like "py-1" (or "py-0.5") to comply with the compact
form input guideline.
In `@ui/src/features/cockpit/hooks/useCockpitState.ts`:
- Around line 45-52: The deleteWorkspace callback currently ignores errors from
client.DELETE and always calls mutate() and setSelectedWorkspace(''), causing UI
desync; update deleteWorkspace to await client.DELETE inside a try/catch so that
mutate() and setSelectedWorkspace('') are only executed after a successful
response, and handle failures in the catch (e.g., log the error or surface a
user-visible error) while leaving the current selection intact; specifically
modify the deleteWorkspace function that calls
client.DELETE('/workspaces/{workspaceId}', ...), wrapping the request in
try/catch and moving mutate() and setSelectedWorkspace('') into the successful
path.
- Around line 19-30: The useEffect currently references createWorkspace before
it's defined causing a temporal dead zone; move the entire useEffect block so it
sits after the createWorkspace declaration (keep references to autoCreatedRef
and selectedWorkspace as before). Update the deleteWorkspace callback (the async
function that calls client.DELETE) to only clear selection and call mutate when
the DELETE returns no error (check the returned { error } and only run mutate()
and setSelectedWorkspace('') on success). Finally, include selectedWorkspace in
the useEffect dependency array so the effect reacts to selection changes.
In `@ui/src/features/cockpit/hooks/useDateKanbanData.ts`:
- Around line 63-67: sseParams used for the SSE subscription (const sseParams
and useDAGRunsListSSE) omits remoteNode which can cause cross-node updates;
update the useMemo that defines sseParams to include remoteNode in the returned
object and add remoteNode to the dependency array so the memo and SSE
subscription are routed to the same node as the list (mirror how useQuery routes
by node).
In `@ui/src/features/dags/components/common/DAGActions.tsx`:
- Around line 523-525: The await of onEnqueue(params, dagRunId, immediate) can
throw and needs a try/catch around it: wrap the await call in try { await
onEnqueue(...) } catch (err) { if (typeof onEnqueueError === "function") call
onEnqueueError(err, params, dagRunId, immediate); else log the error and surface
a user-friendly message via the app's notification utility (or console.error) so
the rejection is handled and users get feedback; keep the early return behavior
after a successful await. Reference: onEnqueue, params, dagRunId, immediate.
In `@ui/src/menu.tsx`:
- Around line 574-581: The Remote Nodes NavItem is rendered unconditionally;
restrict it to admins by rendering it only when isAdmin is true (use the
existing isAdmin value from useIsAdmin). Locate the NavItem instance for "Remote
Nodes" and wrap it in a conditional check (e.g., if (isAdmin) ... ) so it
matches other permission-guarded items like canAccessSystemStatus/canWrite;
ensure props (to, text, icon, isOpen, onClick, customColor) remain unchanged.
---
Minor comments:
In `@api/v1/api.yaml`:
- Around line 9675-9691: Add non-empty validation to the workspace "name" field
by updating both CreateWorkspaceRequest and UpdateWorkspaceRequest: add a
constraint like minLength: 1 (or an equivalent pattern) to the name property so
empty strings are rejected; keep name required on CreateWorkspaceRequest and
ensure UpdateWorkspaceRequest's name, when present, also enforces minLength: 1.
In `@ui/src/App.tsx`:
- Line 237: The /cockpit route is exposed even when the agent feature is
disabled; update the routing so access is gated by the same feature-guard
pattern used elsewhere: wrap the Route element for path="/cockpit" (currently
rendering CockpitPage) with a feature guard that checks config.agentEnabled
(e.g., create/ reuse a guard similar to DeveloperElement or LicensedRoute that
returns <Navigate/> or a “feature disabled” fallback when false), or
alternatively update CockpitPage to render a graceful disabled-state UI when
config.agentEnabled is false; ensure you reference the Route path="/cockpit",
CockpitPage, and config.agentEnabled when implementing the guard or the
in-component handling.
In `@ui/src/features/cockpit/components/EmbeddedAgentChat.tsx`:
- Around line 82-86: Replace the silent promise swallow in handleSend by making
the callback async and wrapping the sendMessage call in try/catch: in the
handleSend function (which calls setInitialInputValue and sendMessage), await
sendMessage(message, model, dagContexts, soulId) inside a try block, and in the
catch block log the error and surface user feedback (e.g., call the app's
toast/error handler or set an error state) so failures are visible; ensure you
still clear or restore input via setInitialInputValue as appropriate and do not
leave the promise unhandled.
In `@ui/src/features/cockpit/components/KanbanCard.tsx`:
- Around line 47-50: In the KanbanCard component update the JSX button element
to include an explicit type attribute (type="button") so clicks don't act as
form submissions; locate the <button ... onClick={onClick} className="w-full
text-left p-2 rounded-md border border-border bg-card hover:bg-accent/50
transition-colors cursor-pointer"> and add type="button" to it to prevent
accidental submits when rendered inside a form.
In `@ui/src/features/cockpit/components/TemplateSelector.tsx`:
- Around line 76-85: The effect in useEffect that syncs selectedTemplate with
selectedDag can leave selectedDag stale when filteredDags no longer contains the
selectedTemplate; update the effect so that after searching
filteredDags.find((d) => d.fileName === selectedTemplate) you explicitly clear
the selection when not found (call setSelectedDag(null)) in addition to setting
it when found, keeping the same dependency array [selectedTemplate,
filteredDags] and referencing the existing symbols selectedTemplate,
filteredDags, setSelectedDag, and selectedDag.
- Around line 318-339: The list currently uses "truncate" on the DAG name and
description in TemplateSelector (the span with className built via cn for
dag.dag.name and the description div), which clips long text; replace those
"truncate" uses with wrapping classes "whitespace-normal break-words" (and
remove/avoid "truncate") so long DAG names and descriptions wrap instead of
being clipped, and ensure the parent container (the surrounding div rendering
the name/description) allows wrapping by not forcing no-wrap behavior.
In `@ui/src/features/cockpit/components/WorkspaceSelector.tsx`:
- Around line 58-61: The Input control (ref inputRef) and the corresponding
Select trigger should use explicit compact sizing and padding rather than rely
on defaults; update the Input's className to include a fixed height (h-7),
compact vertical padding (py-0.5), small text (text-xs), tight line-height
(leading-none) and existing horizontal padding/width (px-2 w-40) and apply the
same class set to the Select trigger element so both controls match in height,
padding and background color (use the same bg-... utility or remove bg to
inherit) for consistent, information-dense toolbar styling.
---
Nitpick comments:
In `@api/v1/api.yaml`:
- Around line 5962-5967: The inline workspaceId path parameter is duplicated;
extract it into a reusable components/parameters entry named e.g. WorkspaceId
(define schema type: string and required:true, in:path) and replace each inline
definition (the blocks where name: workspaceId in:path required:true
schema:type:string — occurrences near the existing RemoteNode ref) with a $ref:
"#/components/parameters/WorkspaceId"; update all three occurrences so they
reference the new component to prevent drift.
In `@ui/src/api/v1/schema.ts`:
- Around line 11660-11840: The workspace endpoints (listWorkspaces,
createWorkspace, getWorkspace, deleteWorkspace, updateWorkspace) lack a default
error response; add a default response entry to each operation's responses using
the existing Error schema (i.e. default: { headers: { [name: string]: unknown },
content: { "application/json": components["schemas"]["Error"] } }) so
client-side error handling is uniform, then regenerate the OpenAPI clients/spec
artifacts.
In `@ui/src/features/cockpit/components/DAGPreviewModal.tsx`:
- Around line 210-213: Replace the loose modal spacing in DAGPreviewModal by
changing the wrapper div's className from "p-6 w-full flex flex-col h-full
dag-modal-content" to a denser padding (e.g., "p-3 w-full flex flex-col h-full
dag-modal-content") and reduce the toolbar bottom margin from "mb-4" to "mb-2"
so the header is visually compact and matches cockpit patterns; after updating
classes, verify the toolbar and modal header elements in DAGPreviewModal remain
keyboard-focusable (tab/escape handling) to preserve keyboard navigation.
In `@ui/src/menu.tsx`:
- Around line 266-270: Add an inline comment above the React.useEffect that
references isChildActive, isExpanded, and setIsExpanded explaining that omitting
isExpanded from the dependency array is intentional: the effect should only
expand the item when a child becomes active (isChildActive changes) but must not
collapse when isExpanded is toggled manually, so only isChildActive is listed as
a dependency. Keep the comment concise and mention the behavioral intent to
prevent automatic collapsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 845843fc-679e-4fb7-b986-55bbd8c0cd81
⛔ Files ignored due to path filters (1)
ui/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
api/v1/api.gen.goapi/v1/api.yamlinternal/agent/dag_codegen.gointernal/agent/dag_codegen_test.gointernal/cmn/config/config.gointernal/cmn/config/definition.gointernal/cmn/config/loader.gointernal/persis/fileworkspace/store.gointernal/persis/fileworkspace/store_test.gointernal/service/audit/entry.gointernal/service/frontend/api/v1/api.gointernal/service/frontend/api/v1/workspaces.gointernal/service/frontend/server.gointernal/workspace/store.gointernal/workspace/workspace.goui/package.jsonui/src/App.tsxui/src/api/v1/schema.tsui/src/components/ui/date-range-picker.tsxui/src/components/ui/select.tsxui/src/components/ui/tag-combobox.tsxui/src/features/cockpit/components/CockpitToolbar.tsxui/src/features/cockpit/components/DAGPreviewModal.tsxui/src/features/cockpit/components/DateKanbanList.tsxui/src/features/cockpit/components/DateKanbanSection.tsxui/src/features/cockpit/components/EmbeddedAgentChat.tsxui/src/features/cockpit/components/KanbanBoard.tsxui/src/features/cockpit/components/KanbanCard.tsxui/src/features/cockpit/components/KanbanColumn.tsxui/src/features/cockpit/components/TemplateSelector.tsxui/src/features/cockpit/components/WorkspaceSelector.tsxui/src/features/cockpit/hooks/useCockpitState.tsui/src/features/cockpit/hooks/useDateKanbanData.tsui/src/features/cockpit/hooks/useInfiniteKanban.tsui/src/features/dags/components/common/DAGActions.tsxui/src/features/dags/components/dag-details/DAGDetailsContent.tsxui/src/features/dags/components/dag-details/DAGHeader.tsxui/src/menu.tsxui/src/pages/cockpit/index.tsxui/src/pages/dag-runs/index.tsxui/src/pages/queues/index.tsx
yottahmd
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1728 +/- ##
==========================================
- Coverage 69.61% 69.52% -0.09%
==========================================
Files 398 400 +2
Lines 43873 44058 +185
==========================================
+ Hits 30540 30632 +92
- Misses 10879 10941 +62
- Partials 2454 2485 +31
... and 9 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
UI/UX
Dependencies