saddlepaddle/new panes#2801
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces a new pane-layout workspace system supporting multiple roots and panes per workspace, migrates workspace state persistence from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (13)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/components/PaneViewerEmptyState/PaneViewerEmptyState.tsx (1)
8-10: Consider de-coupling from legacyWorkspaceViewmodule paths.Line 8–Line 10 pull UI assets/components from an older feature area. Moving these into a shared or local
PaneViewer-adjacent module will reduce cross-feature coupling.As per coding guidelines, "Co-locate dependencies (utils, hooks, constants, config, tests, stories) next to the file using them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/components/PaneViewerEmptyState/PaneViewerEmptyState.tsx around lines 8 - 10, PaneViewerEmptyState imports UI assets/components (supersetEmptyStateWordmark and EmptyTabActionButton) from the legacy WorkspaceView module, creating unwanted cross-feature coupling; move or duplicate these dependencies into a PaneViewer-adjacent/shared UI module (e.g., a local assets/ and components/ folder next to PaneViewerEmptyState) and update the imports in PaneViewerEmptyState.tsx to reference the new local/shared paths so the file no longer depends on renderer/screens/main/components/WorkspaceView.packages/db/drizzle/0029_use_v2_workspace_id.sql (1)
1-2: Add an index forchat_sessions.v2_workspace_idwith this migration.The FK is correct, but it won’t index the column automatically. Adding an index here avoids workspace-filter query regressions as data grows.
📈 Proposed migration addition
ALTER TABLE "chat_sessions" ADD COLUMN "v2_workspace_id" uuid;--> statement-breakpoint ALTER TABLE "chat_sessions" ADD CONSTRAINT "chat_sessions_v2_workspace_id_v2_workspaces_id_fk" FOREIGN KEY ("v2_workspace_id") REFERENCES "public"."v2_workspaces"("id") ON DELETE set null ON UPDATE no action; +CREATE INDEX "chat_sessions_v2_workspace_id_idx" ON "chat_sessions" ("v2_workspace_id");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/drizzle/0029_use_v2_workspace_id.sql` around lines 1 - 2, The migration adds the v2_workspace_id column and FK on chat_sessions but omits an index; update the migration (the ALTER TABLE "chat_sessions" ADD COLUMN "v2_workspace_id" uuid; and related FK statement "chat_sessions_v2_workspace_id_v2_workspaces_id_fk") to also create an index on chat_sessions.v2_workspace_id (e.g., CREATE INDEX on that column) so workspace-filter queries remain performant as data grows.packages/db/src/schema/schema.ts (1)
648-650: Consider adding an index onv2WorkspaceIdfor query performance.If chat sessions will be frequently queried by
v2WorkspaceId(e.g., loading sessions for a workspace), an index would improve lookup performance. The existing indexes only coverorganizationId,createdBy, andlastActiveAt.💡 Suggested index addition
(table) => [ index("chat_sessions_org_idx").on(table.organizationId), index("chat_sessions_created_by_idx").on(table.createdBy), index("chat_sessions_last_active_idx").on(table.lastActiveAt), + index("chat_sessions_v2_workspace_id_idx").on(table.v2WorkspaceId), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schema/schema.ts` around lines 648 - 650, Add a database index for v2WorkspaceId to improve query performance when filtering by workspace; locate the table definition that contains the v2WorkspaceId column (the column symbol is v2WorkspaceId and it references v2Workspaces.id) and add an index for that column alongside the existing indexes (similar to the existing indexes on organizationId, createdBy, lastActiveAt) so queries that filter by v2WorkspaceId will use the index.packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootView/PaneRootView.tsx (1)
13-15: Consider extracting inline import type.The inline
import("../../../../../types").PaneGroupNode<TPaneData>works but is verbose. Consider importingPaneGroupNodeat the top with the other type imports for consistency.♻️ Suggested refactor
import type { PaneWorkspaceStore } from "../../../../../core/store"; -import type { PaneRootState } from "../../../../../types"; +import type { PaneGroupNode, PaneRootState } from "../../../../../types"; import type { PaneRegistry, PaneRendererContext } from "../../../../types"; import { PaneNodeView } from "../PaneNodeView"; interface PaneRootViewProps<TPaneData> { store: StoreApi<PaneWorkspaceStore<TPaneData>>; root: PaneRootState<TPaneData> | null; registry: PaneRegistry<TPaneData>; onAddPane?: (args: { store: StoreApi<PaneWorkspaceStore<TPaneData>>; root: PaneRootState<TPaneData>; - group: import("../../../../../types").PaneGroupNode<TPaneData>; + group: PaneGroupNode<TPaneData>; }) => void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootView/PaneRootView.tsx` around lines 13 - 15, Extract the inline type import by adding a top-level type import for PaneGroupNode and replace the inline usage in the callback signature: import { PaneGroupNode } from "…/types" (matching existing import style with other types), then update the parameter type from import("../../../../../types").PaneGroupNode<TPaneData> to PaneGroupNode<TPaneData> in the function where PaneRootState<TPaneData> and group are declared (the callback in PaneRootView.tsx referencing root: PaneRootState<TPaneData>; group: ...).packages/pane-layout/src/react/components/PaneWorkspace/components/PaneNodeView/PaneNodeView.tsx (1)
13-17: Same inline import pattern as PaneRootView.Consider extracting the inline
import("../../../../../types").PaneGroupNode<TPaneData>to a top-level import for consistency with the rest of the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneNodeView/PaneNodeView.tsx` around lines 13 - 17, The onAddPane prop type currently uses an inline dynamic import for PaneGroupNode; extract import("../../../../../types").PaneGroupNode<TPaneData> to a top-level import (e.g., import { PaneGroupNode } from '...') and update the onAddPane signature to reference the imported PaneGroupNode type instead of the inline import so the file (PaneNodeView.tsx) matches the rest of the top-level imports; keep the existing identifiers PaneWorkspaceStore, PaneRootState, TPaneData and the onAddPane prop name intact.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/PaneViewer.tsx (1)
199-204: Consider addingsandboxattribute to the preview iframe.The iframe renders
data.urlwithout restrictions. While this is likely localhost for preview, adding asandboxattribute with appropriate permissions would provide defense-in-depth:<iframe className="h-full w-full border-0 bg-background" + sandbox="allow-scripts allow-same-origin allow-forms" src={data.url} title={pane.titleOverride ?? "Browser"} />If full iframe capabilities are required for the preview to function correctly, this can be skipped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/PaneViewer.tsx around lines 199 - 204, The iframe rendering in the PaneViewer component currently embeds data.url without sandboxing; update the <iframe> JSX inside PaneViewer (where src={data.url} and title={pane.titleOverride ?? "Browser"}) to include a sandbox attribute restricting capabilities (e.g., sandbox="allow-forms allow-scripts" or a stricter set) appropriate for preview needs, or conditionally add/omit sandbox based on the pane config if full capabilities are required; ensure the chosen sandbox token list is documented in a comment near the iframe for future reviewers.apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts (1)
15-15: Consider adding basic runtime validation forpaneLayout.
z.custom<PaneWorkspaceState<unknown>>()provides type-level inference but performs no runtime validation. If malformed data is persisted (e.g., from a bug or migration), it will pass validation and potentially cause runtime errors elsewhere.If full recursive validation is too complex, consider at minimum validating the top-level shape:
💡 Optional refinement
-const paneWorkspaceStateSchema = z.custom<PaneWorkspaceState<unknown>>(); +const paneWorkspaceStateSchema = z.custom<PaneWorkspaceState<unknown>>((val) => { + return ( + val !== null && + typeof val === "object" && + "roots" in val && + Array.isArray((val as { roots: unknown }).roots) + ); +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts` at line 15, The current paneWorkspaceStateSchema uses z.custom<PaneWorkspaceState<unknown>>() which skips runtime validation; replace it with a schema that at minimum validates the top-level shape of PaneWorkspaceState (e.g., required keys like paneLayout and any id/version fields) by creating a z.object for PaneWorkspaceState with paneLayout validated as a non-empty object or specific union/record and other top-level properties typed (use z.record, z.string, z.number, z.optional as appropriate) so malformed persisted data fails validation; update the symbol paneWorkspaceStateSchema to use that z.object-based schema and keep the generic PaneWorkspaceState<unknown> type inference by using .parse/.safeParse where data is loaded.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts (1)
51-56: Type assertion forpaneLayoutassumes correct persisted data shape.The cast
as PaneWorkspaceState<PaneViewerData> | undefinedassumes the persisted data matches the expected type. Combined with thez.custom()schema that performs no runtime validation, malformed persisted data could cause runtime errors.This is consistent with the schema design decision but worth noting. If persistence corruption becomes an issue, consider adding shape validation when reading (defensive parsing).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts around lines 51 - 56, The persistedPaneLayout currently casts localWorkspaceState?.paneLayout to PaneWorkspaceState<PaneViewerData> which assumes the stored shape is valid; change the read in useV2WorkspacePaneLayout to perform defensive runtime validation (e.g., use the existing z schema or a shape-check helper to safeParse/safeValidate localWorkspaceState?.paneLayout) and if validation fails fall back to EMPTY_PANE_LAYOUT, removing the unchecked type assertion; reference persistedPaneLayout, localWorkspaceState, PaneWorkspaceState, PaneViewerData, EMPTY_PANE_LAYOUT and update the useV2WorkspacePaneLayout hook to accept only the validated object.packages/pane-layout/src/core/store/store.ts (1)
479-500: Inconsistent indentation insetPaneDatamethod.The
setPaneDatamethod has inconsistent indentation that breaks the visual pattern of other methods.🔧 Suggested fix
- setPaneData: (args) => { - set((state) => { - const paneLocation = findPaneLocation(state.state, args.paneId); - if (!paneLocation) return state; - - return { - state: { - ...state.state, - roots: state.state.roots.map((root) => - root.id === paneLocation.rootId - ? updateGroupNode(root, paneLocation.groupId, (currentGroup) => ({ - ...currentGroup, - panes: currentGroup.panes.map((pane) => - pane.id === args.paneId ? { ...pane, data: args.data } : pane, - ), - })) - : root, - ), - }, - }; - }); - }, + setPaneData: (args) => { + set((state) => { + const paneLocation = findPaneLocation(state.state, args.paneId); + if (!paneLocation) return state; + + return { + state: { + ...state.state, + roots: state.state.roots.map((root) => + root.id === paneLocation.rootId + ? updateGroupNode(root, paneLocation.groupId, (currentGroup) => ({ + ...currentGroup, + panes: currentGroup.panes.map((pane) => + pane.id === args.paneId ? { ...pane, data: args.data } : pane, + ), + })) + : root, + ), + }, + }; + }); + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pane-layout/src/core/store/store.ts` around lines 479 - 500, The setPaneData setter has inconsistent indentation/brace alignment that breaks the visual pattern; reformat the block inside setPaneData (the callback passed to set) so braces and ternary mapping align with other setters: locate setPaneData, the call to findPaneLocation, and the roots.map that uses updateGroupNode, and adjust indentation/line breaks so the early return, the returned object { state: { ... } }, and nested maps/conditionals follow the same indentation style as neighboring methods for readability.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/ChatPaneInterface.tsx (1)
732-744: Inconsistent indentation in dependency array.The dependency array has inconsistent indentation formatting which reduces readability.
🔧 Suggested fix
}, [ activeModel?.id, - captureChatEvent, - clearRuntimeError, - commands, - getOrCreateSession, - initialLaunchConfig, - sendMessageToSession, - sessionId, - setRuntimeErrorMessage, + captureChatEvent, + clearRuntimeError, + commands, + getOrCreateSession, + initialLaunchConfig, + sendMessageToSession, + sessionId, + setRuntimeErrorMessage, onUserMessageSubmitted, thinkingLevel, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/ChatPaneInterface.tsx around lines 732 - 744, The dependency array in ChatPaneInterface's effect hook is inconsistently indented (see the array containing activeModel?.id, captureChatEvent, clearRuntimeError, commands, getOrCreateSession, initialLaunchConfig, sendMessageToSession, sessionId, setRuntimeErrorMessage, onUserMessageSubmitted, thinkingLevel); reformat the array so all items are aligned consistently (one per line or single-line) and indentation matches the surrounding code style used in ChatPaneInterface/ChatPaneInterface.tsx to improve readability.packages/trpc/src/router/chat/chat.ts (1)
43-73: Consider validatingv2WorkspaceIdexists before insertion.The
createSessionmutation insertsv2WorkspaceIdwithout verifying the workspace exists. If the workspace ID is invalid, this could create orphaned session records. Also,onConflictDoNothingsilently succeeds on duplicates—consider whether the caller should be informed when a session already exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/chat/chat.ts` around lines 43 - 73, The createSession mutation inserts input.v2WorkspaceId into chatSessions without verifying the workspace exists and silently ignores duplicates via onConflictDoNothing; before inserting (in createSession) query the workspace store (e.g., lookup v2WorkspaceId in your workspaces table/model) and throw a TRPCError (BAD_REQUEST or NOT_FOUND) if not found, and instead of blindly using onConflictDoNothing either check for an existing session by id first and return a clear error/previous sessionId or change the insert flow to detect conflict and return an appropriate TRPCError (or a structured response indicating the session already exists) so callers are informed of duplicates.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/old/page.tsx (1)
205-227: Indentation inconsistency in conditional rendering block.The conditional rendering block has inconsistent indentation that makes the code harder to read. The closing
}on line 227 doesn't align with the opening structure.🔧 Suggested fix for consistent indentation
- {activeView === "chat" ? ( - <WorkspaceChat - onSessionIdChange={() => {}} - sessionId={null} - workspaceId={workspaceId} - /> - ) : activeView === "files" ? ( - <WorkspaceFiles + {activeView === "chat" ? ( + <WorkspaceChat + onSessionIdChange={() => {}} + sessionId={null} + workspaceId={workspaceId} + /> + ) : activeView === "files" ? ( + <WorkspaceFiles + onSelectFile={handleSelectFile} + selectedFilePath={selectedFilePath} + workspaceId={workspaceId} + /> + ) : ( + <div className="flex h-full w-full flex-col gap-6 overflow-y-auto p-6"> + <WorkspaceTerminal workspaceId={workspaceId} /> + <div className="space-y-4"> + <Section title="health.info" query={healthQuery} /> + <Section title="github.getUser" query={githubUserQuery} /> + <Section title="workspace.gitStatus" query={gitStatusQuery} /> + </div> + </div> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/old/page.tsx around lines 205 - 227, The JSX conditional rendering around activeView is mis-indented; reformat the ternary branches for consistent indentation so each branch (the WorkspaceChat block, WorkspaceFiles block, and the fallback div containing WorkspaceTerminal and Section components) lines up visually with the opening "{activeView === \"chat\" ? (" expression, and ensure the final closing ")}" matches that same indentation level; update the blocks referencing WorkspaceChat, WorkspaceFiles (handleSelectFile, selectedFilePath), WorkspaceTerminal, and the Sections (healthQuery, githubUserQuery, gitStatusQuery) so their opening and closing tags and braces are consistently indented.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/pane-viewer.model.ts (1)
42-159: Add co-located tests for the pane factory helpers.Please add
pane-viewer.model.test.tsnext to this file to lock downkind,titleOverride, defaultpinned, anddatapayload shaping for each factory.As per coding guidelines,
**/*.{ts,tsx}: Co-locate tests with their implementation files (e.g.,Component.test.tsxnext toComponent.tsx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/pane-viewer.model.ts around lines 42 - 159, Add a co-located test file named pane-viewer.model.test.ts next to pane-viewer.model.ts that imports the factory functions (createFilePane, createTerminalPane, createBrowserPane, createChatPane, createDevtoolsPane) and asserts for each factory that the returned PaneState has the expected kind string, the provided title is set as titleOverride, the pinned default (or provided) value is correct, and the data payload shape/fields (filePath, mode, hasChanges, language; sessionKey, cwd, launchMode, command; url, mode; sessionId; targetPaneId, targetTitle) match exactly what the factory should produce. Ensure tests cover both default pinned behavior and when pinned is explicitly passed so the shape and defaults are locked down.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/pane-viewer.model.ts:
- Around line 42-56: createFilePane currently leaves the pinned parameter
undefined; make it default to true to match other creators (createTerminalPane,
createBrowserPane, createChatPane, createDevtoolsPane) so file panes are pinned
when callers (e.g., PaneViewer) omit it. Fix by adding a default in the function
parameter destructuring (pinned = true) and keep the pinned?: boolean type so
callers remain compatible; verify createFilePane and any callers continue to
compile.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/PaneViewer.tsx:
- Around line 171-238: The useMemo for paneRegistry incorrectly includes unused
workspaceName and omits mention of the stable store.getState call; update the
dependency array on the useMemo that creates paneRegistry (the one rendering
chat/terminal/file/browser/devtools) to remove workspaceName and keep only
stable deps (e.g., [workspaceId]) — do not add store.getState to the array since
store is created once and getState is stable; alternatively, if workspaceName
was intended to be used in the memo body, either use it inside renderers (e.g.,
in the chat renderPane) or remove the prop/comment it until needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceChat/hooks/useWorkspaceChatController/useWorkspaceChatController.ts:
- Around line 109-138: getOrCreateSession can race with the live query that
populates sessions: when a provided sessionId isn’t yet in the sessions array
the code calls createSessionRecord which can create duplicates and cause UI
inconsistency; fix by marking the session as "in-flight" and/or performing an
optimistic update to the sessions cache after createSessionRecord. Concretely,
inside getOrCreateSession (and in the branch where sessionId exists but
sessions.some(...) is false and in the new-session branch) add a ref or set
(e.g., inFlightSessionIds) to track session IDs being created to prevent
duplicate creates, and after successful createSessionRecord update the sessions
query result (either push the new session into the sessions state/cache or
trigger the sessions query to refresh) and invoke onSessionIdChange so the UI
immediately reflects the new session.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceChat/WorkspaceChat.tsx:
- Around line 42-51: The isFocused prop is hardcoded to true on
WorkspaceChatInterface; change it to derive focus from the workspace/pane active
state and pass that boolean instead. Locate where WorkspaceChatInterface is
rendered and compute a boolean (e.g., const isFocused = activePaneId === 'chat'
|| activePane === `chat:${workspaceId}` or via the existing
useActivePane/getActivePane selector from context), then replace isFocused with
isFocused={isFocused}; ensure you import/use the correct selector/hook that
provides the current active pane. Use the WorkspaceChatInterface render site and
related symbols (getOrCreateSession, handleNewChat, sessionId, workspaceId,
organizationId, cwd) when making the change so only the isFocused value is
replaced.
In `@apps/electric-proxy/src/electric.ts`:
- Around line 34-46: The code constructs new URL(...) for upstream without
validating env.ELECTRIC_CLOUD_URL or env.ELECTRIC_URL, causing runtime
TypeError; before creating the URL in the hasSourceCredentials branch and the
fallback branch, validate that env.ELECTRIC_CLOUD_URL (when hasSourceCredentials
is true) or env.ELECTRIC_URL (when false) is a non-empty string and a valid URL,
and throw or log a clear error if missing/invalid; update the logic around
hasSourceCredentials and upstream so URL construction only happens after
validation (refer to the hasSourceCredentials variable, upstream assignment, and
env.ELECTRIC_CLOUD_URL / env.ELECTRIC_URL names to locate the fix).
In `@packages/pane-layout/src/core/store/store.ts`:
- Around line 781-784: The set() callbacks in resizeSplit and equalizeSplit call
updateNodeAtPath and throw an Error when node.type !== "split", which can leave
the Zustand store inconsistent; instead, validate before mutating or avoid
throwing inside set: use findNodePathBySplitId/getState (or inspect currentRoot)
to confirm the node exists and is type "split" before calling set(), and if the
check fails, return without calling set() (or inside the set callback return the
unchanged state/currentRoot rather than throwing). Update references:
resizeSplit, equalizeSplit, findNodePathBySplitId, updateNodeAtPath so no
exceptions are thrown inside Zustand set callbacks.
In `@packages/pane-layout/src/core/store/utils/utils.ts`:
- Around line 47-59: The getNodeAtPath function uses a non-null assertion
current.children[index]! without bounds checks; update getNodeAtPath to validate
that current.type === "split" and that index is a valid integer within 0 <=
index < current.children.length before accessing current.children[index], and
throw a clear, descriptive Error (e.g., "Invalid path: index X out of bounds for
split node") when the check fails so corrupted or stale paths produce
deterministic errors.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneGroup/PaneGroup.tsx`:
- Around line 92-108: The blur handler can race with the Escape key so pressing
Escape may still trigger saveEdit; modify the component to track a cancellation
flag (e.g., a ref like cancelledRef) that on Escape (in the onKeyDown branch
that calls setIsEditing(false)) sets cancelledRef.current = true, and update the
onBlur handler to check cancelledRef.current: if true, reset it to false and
bail out (do not call saveEdit), otherwise call saveEdit normally; reference the
Input component's onKeyDown handler, saveEdit, setIsEditing, and the onBlur
handler to locate where to add and check the cancelled flag.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/components/RootRenameInput/RootRenameInput.tsx`:
- Around line 34-45: The onBlur handler in RootRenameInput is causing a
double-submit after onKeyDown handles Enter (and causing cancel to be negated by
a subsequent blur); fix by introducing a temporary flag (e.g., ignoreNextBlur or
blurSuppressed) scoped to the component and toggle it inside the onKeyDown
handler when you handle Enter or Escape (set it true before calling
onSubmit/onCancel), then modify the onBlur handler to check that flag and, if
true, clear the flag and return without calling onSubmit; ensure the flag is
reset in both branches so normal blurs still submit and reference the
RootRenameInput component and its onKeyDown/onBlur/onSubmit/onCancel handlers
when applying the change.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneSplitHandle/PaneSplitHandle.tsx`:
- Around line 15-24: The equalize action is only bound to the mouse via
onDoubleClick on the button (aria-label "Equalize split") so keyboard users
can't trigger it; add an onKeyDown handler on the same button (the element using
onDoubleClick in PaneSplitHandle) that listens for Enter and Space, calls the
same onDoubleClick/equalize handler, and prevents default for Space to avoid
scrolling, ensuring keyboard activation mirrors the double-click behavior for
accessibility.
In `@packages/trpc/src/router/chat/chat.ts`:
- Around line 75-114: The two mutations updateSession and updateTitle duplicate
title-updating behavior but use different authorization checks; make their logic
consistent by requiring the same org-scoped authorization in both places: fetch
organizationId from ctx.session.session.activeOrganizationId and include
eq(chatSessions.organizationId, organizationId) alongside
eq(chatSessions.createdBy, ctx.session.user.id) in the WHERE clause of the
update in updateTitle (or conversely, if you prefer org-less behavior, remove
the organizationId check from updateSession and centralize the shared logic into
a single helper used by both mutations); ensure the
db.update(chatSessions).set(...).where(...) call in both updateSession and
updateTitle uses the exact same predicate set and update fields, or deprecate
one mutation and route callers to the canonical method.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/components/PaneViewerEmptyState/PaneViewerEmptyState.tsx:
- Around line 8-10: PaneViewerEmptyState imports UI assets/components
(supersetEmptyStateWordmark and EmptyTabActionButton) from the legacy
WorkspaceView module, creating unwanted cross-feature coupling; move or
duplicate these dependencies into a PaneViewer-adjacent/shared UI module (e.g.,
a local assets/ and components/ folder next to PaneViewerEmptyState) and update
the imports in PaneViewerEmptyState.tsx to reference the new local/shared paths
so the file no longer depends on renderer/screens/main/components/WorkspaceView.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:
- Around line 51-56: The persistedPaneLayout currently casts
localWorkspaceState?.paneLayout to PaneWorkspaceState<PaneViewerData> which
assumes the stored shape is valid; change the read in useV2WorkspacePaneLayout
to perform defensive runtime validation (e.g., use the existing z schema or a
shape-check helper to safeParse/safeValidate localWorkspaceState?.paneLayout)
and if validation fails fall back to EMPTY_PANE_LAYOUT, removing the unchecked
type assertion; reference persistedPaneLayout, localWorkspaceState,
PaneWorkspaceState, PaneViewerData, EMPTY_PANE_LAYOUT and update the
useV2WorkspacePaneLayout hook to accept only the validated object.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/pane-viewer.model.ts:
- Around line 42-159: Add a co-located test file named pane-viewer.model.test.ts
next to pane-viewer.model.ts that imports the factory functions (createFilePane,
createTerminalPane, createBrowserPane, createChatPane, createDevtoolsPane) and
asserts for each factory that the returned PaneState has the expected kind
string, the provided title is set as titleOverride, the pinned default (or
provided) value is correct, and the data payload shape/fields (filePath, mode,
hasChanges, language; sessionKey, cwd, launchMode, command; url, mode;
sessionId; targetPaneId, targetTitle) match exactly what the factory should
produce. Ensure tests cover both default pinned behavior and when pinned is
explicitly passed so the shape and defaults are locked down.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/PaneViewer.tsx:
- Around line 199-204: The iframe rendering in the PaneViewer component
currently embeds data.url without sandboxing; update the <iframe> JSX inside
PaneViewer (where src={data.url} and title={pane.titleOverride ?? "Browser"}) to
include a sandbox attribute restricting capabilities (e.g., sandbox="allow-forms
allow-scripts" or a stricter set) appropriate for preview needs, or
conditionally add/omit sandbox based on the pane config if full capabilities are
required; ensure the chosen sandbox token list is documented in a comment near
the iframe for future reviewers.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/ChatPaneInterface.tsx:
- Around line 732-744: The dependency array in ChatPaneInterface's effect hook
is inconsistently indented (see the array containing activeModel?.id,
captureChatEvent, clearRuntimeError, commands, getOrCreateSession,
initialLaunchConfig, sendMessageToSession, sessionId, setRuntimeErrorMessage,
onUserMessageSubmitted, thinkingLevel); reformat the array so all items are
aligned consistently (one per line or single-line) and indentation matches the
surrounding code style used in ChatPaneInterface/ChatPaneInterface.tsx to
improve readability.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/old/page.tsx:
- Around line 205-227: The JSX conditional rendering around activeView is
mis-indented; reformat the ternary branches for consistent indentation so each
branch (the WorkspaceChat block, WorkspaceFiles block, and the fallback div
containing WorkspaceTerminal and Section components) lines up visually with the
opening "{activeView === \"chat\" ? (" expression, and ensure the final closing
")}" matches that same indentation level; update the blocks referencing
WorkspaceChat, WorkspaceFiles (handleSelectFile, selectedFilePath),
WorkspaceTerminal, and the Sections (healthQuery, githubUserQuery,
gitStatusQuery) so their opening and closing tags and braces are consistently
indented.
In
`@apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts`:
- Line 15: The current paneWorkspaceStateSchema uses
z.custom<PaneWorkspaceState<unknown>>() which skips runtime validation; replace
it with a schema that at minimum validates the top-level shape of
PaneWorkspaceState (e.g., required keys like paneLayout and any id/version
fields) by creating a z.object for PaneWorkspaceState with paneLayout validated
as a non-empty object or specific union/record and other top-level properties
typed (use z.record, z.string, z.number, z.optional as appropriate) so malformed
persisted data fails validation; update the symbol paneWorkspaceStateSchema to
use that z.object-based schema and keep the generic PaneWorkspaceState<unknown>
type inference by using .parse/.safeParse where data is loaded.
In `@packages/db/drizzle/0029_use_v2_workspace_id.sql`:
- Around line 1-2: The migration adds the v2_workspace_id column and FK on
chat_sessions but omits an index; update the migration (the ALTER TABLE
"chat_sessions" ADD COLUMN "v2_workspace_id" uuid; and related FK statement
"chat_sessions_v2_workspace_id_v2_workspaces_id_fk") to also create an index on
chat_sessions.v2_workspace_id (e.g., CREATE INDEX on that column) so
workspace-filter queries remain performant as data grows.
In `@packages/db/src/schema/schema.ts`:
- Around line 648-650: Add a database index for v2WorkspaceId to improve query
performance when filtering by workspace; locate the table definition that
contains the v2WorkspaceId column (the column symbol is v2WorkspaceId and it
references v2Workspaces.id) and add an index for that column alongside the
existing indexes (similar to the existing indexes on organizationId, createdBy,
lastActiveAt) so queries that filter by v2WorkspaceId will use the index.
In `@packages/pane-layout/src/core/store/store.ts`:
- Around line 479-500: The setPaneData setter has inconsistent indentation/brace
alignment that breaks the visual pattern; reformat the block inside setPaneData
(the callback passed to set) so braces and ternary mapping align with other
setters: locate setPaneData, the call to findPaneLocation, and the roots.map
that uses updateGroupNode, and adjust indentation/line breaks so the early
return, the returned object { state: { ... } }, and nested maps/conditionals
follow the same indentation style as neighboring methods for readability.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneNodeView/PaneNodeView.tsx`:
- Around line 13-17: The onAddPane prop type currently uses an inline dynamic
import for PaneGroupNode; extract
import("../../../../../types").PaneGroupNode<TPaneData> to a top-level import
(e.g., import { PaneGroupNode } from '...') and update the onAddPane signature
to reference the imported PaneGroupNode type instead of the inline import so the
file (PaneNodeView.tsx) matches the rest of the top-level imports; keep the
existing identifiers PaneWorkspaceStore, PaneRootState, TPaneData and the
onAddPane prop name intact.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootView/PaneRootView.tsx`:
- Around line 13-15: Extract the inline type import by adding a top-level type
import for PaneGroupNode and replace the inline usage in the callback signature:
import { PaneGroupNode } from "…/types" (matching existing import style with
other types), then update the parameter type from
import("../../../../../types").PaneGroupNode<TPaneData> to
PaneGroupNode<TPaneData> in the function where PaneRootState<TPaneData> and
group are declared (the callback in PaneRootView.tsx referencing root:
PaneRootState<TPaneData>; group: ...).
In `@packages/trpc/src/router/chat/chat.ts`:
- Around line 43-73: The createSession mutation inserts input.v2WorkspaceId into
chatSessions without verifying the workspace exists and silently ignores
duplicates via onConflictDoNothing; before inserting (in createSession) query
the workspace store (e.g., lookup v2WorkspaceId in your workspaces table/model)
and throw a TRPCError (BAD_REQUEST or NOT_FOUND) if not found, and instead of
blindly using onConflictDoNothing either check for an existing session by id
first and return a clear error/previous sessionId or change the insert flow to
detect conflict and return an appropriate TRPCError (or a structured response
indicating the session already exists) so callers are informed of duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0f2585b-8a46-4187-846e-327ce71b463b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (74)
.superset/lib/setup/steps.shapps/desktop/package.jsonapps/desktop/plans/20260322-1545-v2-workspace-panes.mdapps/desktop/plans/20260322-2305-pane-layout-api-and-test-plan.mdapps/desktop/plans/20260322-2335-pane-layout-model-comparison.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/PaneViewer.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/components/PaneViewerEmptyState/PaneViewerEmptyState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/components/PaneViewerEmptyState/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/pane-viewer.model.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/WorkspaceChat.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/SessionSelector/SessionSelector.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/ChatPaneInterface.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/components/ChatMessageList/ChatMessageList.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/hooks/useSlashCommandExecutor/useSlashCommandExecutor.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/utils/sendMessage/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/utils/sendMessage/sendMessage.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/utils/sendMessage/sendMessage.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/hooks/useWorkspaceChatController/useWorkspaceChatController.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceNotFoundState/WorkspaceNotFoundState.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceNotFoundState/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/old/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/CollectionsProvider.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.tsapps/electric-proxy/.dev.vars.exampleapps/electric-proxy/src/electric.tsapps/electric-proxy/src/types.tspackages/db/drizzle/0029_use_v2_workspace_id.sqlpackages/db/drizzle/meta/0029_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/relations.tspackages/db/src/schema/schema.tspackages/pane-layout/package.jsonpackages/pane-layout/src/core/store/index.tspackages/pane-layout/src/core/store/store.test.tspackages/pane-layout/src/core/store/store.tspackages/pane-layout/src/core/store/utils/index.tspackages/pane-layout/src/core/store/utils/utils.tspackages/pane-layout/src/index.tspackages/pane-layout/src/react/components/PaneWorkspace/PaneWorkspace.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneContent/PaneContent.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneContent/index.tspackages/pane-layout/src/react/components/PaneWorkspace/components/PaneGroup/PaneGroup.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneGroup/index.tspackages/pane-layout/src/react/components/PaneWorkspace/components/PaneNodeView/PaneNodeView.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneNodeView/index.tspackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/PaneRootTabs.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/PaneRootTabItem.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/components/RootRenameInput/RootRenameInput.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/components/RootRenameInput/index.tspackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/index.tspackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/index.tspackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootView/PaneRootView.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootView/index.tspackages/pane-layout/src/react/components/PaneWorkspace/components/PaneSplitHandle/PaneSplitHandle.tsxpackages/pane-layout/src/react/components/PaneWorkspace/components/PaneSplitHandle/index.tspackages/pane-layout/src/react/components/PaneWorkspace/index.tspackages/pane-layout/src/react/components/index.tspackages/pane-layout/src/react/hooks/index.tspackages/pane-layout/src/react/hooks/usePaneWorkspaceStore/index.tspackages/pane-layout/src/react/hooks/usePaneWorkspaceStore/usePaneWorkspaceStore.tspackages/pane-layout/src/react/index.tspackages/pane-layout/src/react/types.tspackages/pane-layout/src/types.tspackages/pane-layout/tsconfig.jsonpackages/trpc/src/router/chat/chat.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/utils/sendMessage/sendMessage.ts
| export function createFilePane({ | ||
| title, | ||
| filePath, | ||
| mode, | ||
| hasChanges, | ||
| language, | ||
| pinned, | ||
| }: { | ||
| title: string; | ||
| filePath: string; | ||
| mode: FilePaneData["mode"]; | ||
| hasChanges: boolean; | ||
| language?: string; | ||
| pinned?: boolean; | ||
| }): PaneState<PaneViewerData> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
model_file=$(fd -p 'pane-viewer.model.ts$' | head -n1)
echo "Inspecting defaults in: $model_file"
nl -ba "$model_file" | sed -n '42,160p'
echo
echo "Factory call sites (check whether pinned is usually passed explicitly):"
rg -nP --type=ts --type=tsx -C2 '\bcreate(File|Terminal|Browser|Chat|Devtools)Pane\s*\('Repository: superset-sh/superset
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
model_file=$(fd -p 'pane-viewer.model.ts$' | head -n1)
echo "=== File: $model_file ==="
echo
# Read the file with line numbers
echo "=== Reading lines 40-160 with line numbers ==="
cat -n "$model_file" | sed -n '40,160p'
echo
echo "=== Searching for all createXyzPane function definitions ==="
rg -nP 'export function create\w+Pane\s*\(' "$model_file"Repository: superset-sh/superset
Length of output: 3108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for createFilePane call sites ==="
rg -nP 'createFilePane\s*\(' --type=ts --type=tsx -B2 -A5
echo
echo "=== Count of createFilePane calls ==="
rg -c 'createFilePane\s*\(' --type=ts --type=tsx || echo "0"Repository: superset-sh/superset
Length of output: 156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for createFilePane call sites ==="
rg -nP 'createFilePane\s*\(' --type=ts -B2 -A5
echo
echo "=== Count of createFilePane calls ==="
rg -c 'createFilePane\s*\(' --type=ts || echo "0"Repository: superset-sh/superset
Length of output: 2753
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Looking for createPane function definition ==="
rg -nP 'function createPane\s*\(' --type=ts -A20
echo
echo "=== Full PaneViewer.tsx call context ==="
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/PaneViewer.tsx | sed -n '73,85p'Repository: superset-sh/superset
Length of output: 1972
🏁 Script executed:
#!/bin/bash
set -euo pipefixo
model_file=$(fd -p 'pane-viewer.model.ts$' | head -n1)
viewer_file=$(fd -p 'PaneViewer.tsx$' | grep "PaneViewer" | head -n1)
echo "=== Looking for createPane function definition in model file ==="
grep -n "function createPane\|const createPane" "$model_file" -A15
echo
echo "=== Full PaneViewer.tsx call context ==="
cat -n "$viewer_file" | sed -n '73,85p'Repository: superset-sh/superset
Length of output: 117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
model_file=$(fd -p 'pane-viewer.model.ts$' | head -n1)
viewer_file=$(fd -p 'PaneViewer.tsx$' | grep "PaneViewer" | head -n1)
echo "=== Looking for createPane function definition in model file ==="
grep -n "function createPane\|const createPane" "$model_file" -A15
echo
echo "=== Full PaneViewer.tsx call context ==="
cat -n "$viewer_file" | sed -n '73,85p'Repository: superset-sh/superset
Length of output: 128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
model_file=$(fd -p 'pane-viewer.model.ts$' | head -n1)
viewer_file=$(fd -p 'PaneViewer.tsx$' | grep "PaneViewer" | head -n1)
echo "=== Entire pane-viewer.model.ts file ==="
wc -l "$model_file"
cat -n "$model_file"
echo
echo "=== Full PaneViewer.tsx call context ==="
cat -n "$viewer_file" | sed -n '70,90p'Repository: superset-sh/superset
Length of output: 4457
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking `@superset/pane-layout` package ==="
fd -p '@superset/pane-layout' -type d | head -5
echo
echo "=== Looking for createPane definition/types ==="
find . -path './node_modules/@superset/pane-layout' -name '*.d.ts' -o -name '*.ts' 2>/dev/null | head -10
echo
echo "=== Searching for any references to file pane pinning behavior ==="
rg -i 'file.*pane.*pin|pin.*file.*pane' --type=ts -B2 -A2 | head -40
echo
echo "=== Check if there are tests that show expected behavior ==="
fd -p 'pane-viewer.*test' --type fRepository: superset-sh/superset
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking `@superset/pane-layout` package ==="
fd -p '@superset/pane-layout' -type d | head -5
echo
echo "=== Looking for pane-layout types/definitions ==="
find . -path '*@superset/pane-layout*' \( -name '*.d.ts' -o -name '*.ts' \) 2>/dev/null | head -20
echo
echo "=== Searching for any references to file pane pinning behavior ==="
rg -i 'file.*pane.*pin|pin.*file.*pane' --type=ts -B2 -A2 | head -50
echo
echo "=== Check if there are tests for pane-viewer.model ==="
fd -p 'pane-viewer.*test'Repository: superset-sh/superset
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Looking for pane-layout package ==="
find . -type d -name '@superset' 2>/dev/null | head -5
echo
echo "=== Searching for pinning behavior documentation or tests ==="
rg 'pinned' --type ts -B1 -A1 | grep -i 'file\|pane' | head -30
echo
echo "=== Check for any design docs or comments about file pane defaults ==="
rg 'file.*pane|FilePaneData' --type ts -B2 -A2 | head -40Repository: superset-sh/superset
Length of output: 4488
Add default pinned: true to createFilePane for consistency with other pane creators.
createFilePane (line 42-56) lacks a default value for the pinned parameter, while createTerminalPane, createBrowserPane, createChatPane, and createDevtoolsPane all default pinned to true. The call site in PaneViewer.tsx (line 75-80) does not provide an explicit pinned value, meaning file panes will receive undefined for pinning while all other pane types default to pinned behavior. This creates inconsistent pane persistence across pane types and should be normalized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/PaneViewer/pane-viewer.model.ts
around lines 42 - 56, createFilePane currently leaves the pinned parameter
undefined; make it default to true to match other creators (createTerminalPane,
createBrowserPane, createChatPane, createDevtoolsPane) so file panes are pinned
when callers (e.g., PaneViewer) omit it. Fix by adding a default in the function
parameter destructuring (pinned = true) and keep the pinned?: boolean type so
callers remain compatible; verify createFilePane and any callers continue to
compile.
| const getOrCreateSession = useCallback(async (): Promise<string> => { | ||
| if (!organizationId) { | ||
| throw new Error("No active organization selected"); | ||
| } | ||
|
|
||
| if (sessionId) { | ||
| if (sessions.some((item) => item.id === sessionId)) { | ||
| return sessionId; | ||
| } | ||
|
|
||
| await createSessionRecord({ | ||
| sessionId, | ||
| organizationId, | ||
| workspaceId, | ||
| v2WorkspaceId: workspaceId, | ||
| }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } finally { | ||
| setIsSessionInitializing(false); | ||
| return sessionId; | ||
| } | ||
| }, [hasCurrentSessionRecord, organizationId, sessionId, workspaceId]); | ||
|
|
||
| useEffect(() => { | ||
| if (sessionId || sessions.length > 0 || !organizationId) return; | ||
| if (legacySessionBootstrapRef.current) return; | ||
| legacySessionBootstrapRef.current = true; | ||
|
|
||
| void handleNewChat() | ||
| .catch(() => {}) | ||
| .finally(() => { | ||
| legacySessionBootstrapRef.current = false; | ||
| }); | ||
| }, [handleNewChat, organizationId, sessionId, sessions.length]); | ||
| const nextSessionId = crypto.randomUUID(); | ||
| await createSessionRecord({ | ||
| sessionId: nextSessionId, | ||
| v2WorkspaceId: workspaceId, | ||
| }); | ||
| onSessionIdChange(nextSessionId); | ||
| posthog.capture("chat_session_created", { | ||
| workspace_id: workspaceId, | ||
| session_id: nextSessionId, | ||
| organization_id: organizationId, | ||
| }); | ||
| return nextSessionId; | ||
| }, [onSessionIdChange, organizationId, sessionId, sessions, workspaceId]); |
There was a problem hiding this comment.
Potential race condition in getOrCreateSession.
If sessionId is provided but not found in the local sessions list (line 115-117), the code creates a new session record. However, the sessions list comes from a live query that may not have synced the latest data. This could lead to:
- Duplicate creation attempts (handled by
onConflictDoNothingon the server) - Brief UI inconsistency where a session appears created but isn't in the list yet
Consider adding optimistic updates to the sessions query after creation, or using a ref to track in-flight creations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceChat/hooks/useWorkspaceChatController/useWorkspaceChatController.ts
around lines 109 - 138, getOrCreateSession can race with the live query that
populates sessions: when a provided sessionId isn’t yet in the sessions array
the code calls createSessionRecord which can create duplicates and cause UI
inconsistency; fix by marking the session as "in-flight" and/or performing an
optimistic update to the sessions cache after createSessionRecord. Concretely,
inside getOrCreateSession (and in the branch where sessionId exists but
sessions.some(...) is false and in the new-session branch) add a ref or set
(e.g., inFlightSessionIds) to track session IDs being created to prevent
duplicate creates, and after successful createSessionRecord update the sessions
query result (either push the new session into the sessions state/cache or
trigger the sessions query to refresh) and invoke onSessionIdChange so the UI
immediately reflects the new session.
| <WorkspaceChatInterface | ||
| getOrCreateSession={getOrCreateSession} | ||
| initialLaunchConfig={null} | ||
| isFocused | ||
| onResetSession={handleNewChat} | ||
| sessionId={sessionId} | ||
| initialLaunchConfig={launchConfig} | ||
| workspaceId={workspaceId} | ||
| organizationId={organizationId} | ||
| cwd={workspacePath} | ||
| isFocused | ||
| isSessionReady={hasCurrentSessionRecord} | ||
| ensureSessionReady={ensureCurrentSessionRecord} | ||
| onStartFreshSession={handleStartFreshSession} | ||
| onConsumeLaunchConfig={consumeLaunchConfig} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how isFocused is used in WorkspaceChatInterface and whether it affects behavior
rg -n -A5 'isFocused' apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/\$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/Repository: superset-sh/superset
Length of output: 27422
Derive isFocused from active pane state in multi-pane layout.
The isFocused prop is hardcoded to true and controls hotkey enablement and search auto-close behavior downstream. In the multi-pane workspace layout, this should reflect whether the chat pane is actually focused, not always be enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceChat/WorkspaceChat.tsx
around lines 42 - 51, The isFocused prop is hardcoded to true on
WorkspaceChatInterface; change it to derive focus from the workspace/pane active
state and pass that boolean instead. Locate where WorkspaceChatInterface is
rendered and compute a boolean (e.g., const isFocused = activePaneId === 'chat'
|| activePane === `chat:${workspaceId}` or via the existing
useActivePane/getActivePane selector from context), then replace isFocused with
isFocused={isFocused}; ensure you import/use the correct selector/hook that
provides the current active pane. Use the WorkspaceChatInterface render site and
related symbols (getOrCreateSession, handleNewChat, sessionId, workspaceId,
organizationId, cwd) when making the change so only the isFocused value is
replaced.
| const hasSourceCredentials = | ||
| Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET); | ||
|
|
||
| const upstream = hasSourceCredentials | ||
| ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL) | ||
| : new URL(env.ELECTRIC_URL ?? ""); | ||
|
|
||
| if (hasSourceCredentials) { | ||
| upstream.searchParams.set("source_id", env.ELECTRIC_SOURCE_ID ?? ""); | ||
| upstream.searchParams.set("secret", env.ELECTRIC_SOURCE_SECRET ?? ""); | ||
| } else { | ||
| upstream.searchParams.set("secret", env.ELECTRIC_SECRET ?? ""); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the JavaScript URL API, what happens for new URL("/v1/shape", undefined)andnew URL("")? Please cite MDN/spec behavior.
💡 Result:
For new URL("/v1/shape", undefined): Throws a TypeError because "/v1/shape" is a relative URL (root-relative path) and requires a valid base URL to resolve, but undefined is not a valid base. The constructor treats the second argument as undefined (optional parameter missing or explicitly undefined), and since the first argument is relative and lacks a scheme, it fails validation per WHATWG URL spec and MDN docs. For new URL(""): Throws a TypeError because the empty string "" is not a valid absolute URL and has no base provided to resolve it as relative. MDN explicitly shows equivalent examples throwing TypeError: new URL("/en-US/docs") (no base, relative) and new URL("/en-US/docs", "") (invalid empty base). The spec link in MDN is to WHATWG URL#dom-url-url, where the constructor applies the URL parser, failing for relative inputs without usable base.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/URL/URL
- 2: https://developer.cdn.mozilla.net/en-US/docs/Web/API/URL/URL
- 3: https://github.com/mdn/content/blob/main/files/en-us/web/api/url/url/index.md
- 4: https://url.spec.whatwg.org/
🏁 Script executed:
# Read the specific file and lines in question
head -n 50 apps/electric-proxy/src/electric.ts | tail -n +30Repository: superset-sh/superset
Length of output: 745
🏁 Script executed:
# Check the Env type definition for these variables
grep -A 20 "ELECTRIC_CLOUD_URL\|ELECTRIC_URL" apps/electric-proxy/src/types.ts | head -n 40Repository: superset-sh/superset
Length of output: 210
🏁 Script executed:
# Search for calls to buildUpstreamUrl
rg "buildUpstreamUrl" apps/electric-proxy/src/ -A 2 -B 2Repository: superset-sh/superset
Length of output: 962
Guard required env URLs before constructing URL objects.
Lines 38–39 throw TypeError at runtime if ELECTRIC_CLOUD_URL or ELECTRIC_URL are unset. The ?? operator on line 39 coalesces undefined to "", which also fails. Add explicit validation before new URL(...) calls.
Proposed fix
export function buildUpstreamUrl(
clientUrl: URL,
tableName: string,
whereClause: WhereClause,
env: Env,
): URL {
const hasSourceCredentials =
Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET);
- const upstream = hasSourceCredentials
- ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL)
- : new URL(env.ELECTRIC_URL ?? "");
+ let upstream: URL;
+ if (hasSourceCredentials) {
+ if (!env.ELECTRIC_CLOUD_URL) {
+ throw new Error("ELECTRIC_CLOUD_URL is required when source credentials are configured");
+ }
+ upstream = new URL("/v1/shape", env.ELECTRIC_CLOUD_URL);
+ } else {
+ if (!env.ELECTRIC_URL) {
+ throw new Error("ELECTRIC_URL is required when source credentials are not configured");
+ }
+ upstream = new URL(env.ELECTRIC_URL);
+ }
if (hasSourceCredentials) {
upstream.searchParams.set("source_id", env.ELECTRIC_SOURCE_ID ?? "");
upstream.searchParams.set("secret", env.ELECTRIC_SOURCE_SECRET ?? "");
} else {
upstream.searchParams.set("secret", env.ELECTRIC_SECRET ?? "");
}📝 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.
| const hasSourceCredentials = | |
| Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET); | |
| const upstream = hasSourceCredentials | |
| ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL) | |
| : new URL(env.ELECTRIC_URL ?? ""); | |
| if (hasSourceCredentials) { | |
| upstream.searchParams.set("source_id", env.ELECTRIC_SOURCE_ID ?? ""); | |
| upstream.searchParams.set("secret", env.ELECTRIC_SOURCE_SECRET ?? ""); | |
| } else { | |
| upstream.searchParams.set("secret", env.ELECTRIC_SECRET ?? ""); | |
| } | |
| const hasSourceCredentials = | |
| Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET); | |
| let upstream: URL; | |
| if (hasSourceCredentials) { | |
| if (!env.ELECTRIC_CLOUD_URL) { | |
| throw new Error("ELECTRIC_CLOUD_URL is required when source credentials are configured"); | |
| } | |
| upstream = new URL("/v1/shape", env.ELECTRIC_CLOUD_URL); | |
| } else { | |
| if (!env.ELECTRIC_URL) { | |
| throw new Error("ELECTRIC_URL is required when source credentials are not configured"); | |
| } | |
| upstream = new URL(env.ELECTRIC_URL); | |
| } | |
| if (hasSourceCredentials) { | |
| upstream.searchParams.set("source_id", env.ELECTRIC_SOURCE_ID ?? ""); | |
| upstream.searchParams.set("secret", env.ELECTRIC_SOURCE_SECRET ?? ""); | |
| } else { | |
| upstream.searchParams.set("secret", env.ELECTRIC_SECRET ?? ""); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/electric-proxy/src/electric.ts` around lines 34 - 46, The code
constructs new URL(...) for upstream without validating env.ELECTRIC_CLOUD_URL
or env.ELECTRIC_URL, causing runtime TypeError; before creating the URL in the
hasSourceCredentials branch and the fallback branch, validate that
env.ELECTRIC_CLOUD_URL (when hasSourceCredentials is true) or env.ELECTRIC_URL
(when false) is a non-empty string and a valid URL, and throw or log a clear
error if missing/invalid; update the logic around hasSourceCredentials and
upstream so URL construction only happens after validation (refer to the
hasSourceCredentials variable, upstream assignment, and env.ELECTRIC_CLOUD_URL /
env.ELECTRIC_URL names to locate the fix).
| <Input | ||
| autoFocus | ||
| className="h-7" | ||
| onBlur={saveEdit} | ||
| onChange={(event) => setEditValue(event.target.value)} | ||
| onKeyDown={(event) => { | ||
| if (event.key === "Enter") { | ||
| event.preventDefault(); | ||
| saveEdit(); | ||
| } | ||
| if (event.key === "Escape") { | ||
| event.preventDefault(); | ||
| setIsEditing(false); | ||
| } | ||
| }} | ||
| value={editValue} | ||
| /> |
There was a problem hiding this comment.
Potential race condition: Escape key may not cancel the edit.
When user presses Escape, setIsEditing(false) is called, but the Input unmounting can trigger onBlur which calls saveEdit(). The order of execution may cause the edit to be saved instead of cancelled.
Consider tracking whether the edit was cancelled:
🐛 Proposed fix
function PaneGroupItem<TPaneData>({
...
}: PaneGroupItemProps<TPaneData>) {
const [isEditing, setIsEditing] = useState(false);
const [editValue, setEditValue] = useState("");
+ const isCancelledRef = useRef(false);
const startEditing = () => {
setEditValue(typeof title === "string" ? title : pane.id);
+ isCancelledRef.current = false;
setIsEditing(true);
};
const saveEdit = () => {
+ if (isCancelledRef.current) return;
const nextTitle = editValue.trim();
// ... rest of save logic
};
+ const cancelEdit = () => {
+ isCancelledRef.current = true;
+ setIsEditing(false);
+ };
// In onKeyDown:
if (event.key === "Escape") {
event.preventDefault();
- setIsEditing(false);
+ cancelEdit();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneGroup/PaneGroup.tsx`
around lines 92 - 108, The blur handler can race with the Escape key so pressing
Escape may still trigger saveEdit; modify the component to track a cancellation
flag (e.g., a ref like cancelledRef) that on Escape (in the onKeyDown branch
that calls setIsEditing(false)) sets cancelledRef.current = true, and update the
onBlur handler to check cancelledRef.current: if true, reset it to false and
bail out (do not call saveEdit), otherwise call saveEdit normally; reference the
Input component's onKeyDown handler, saveEdit, setIsEditing, and the onBlur
handler to locate where to add and check the cancelled flag.
| onBlur={onSubmit} | ||
| onChange={(event) => onChange(event.target.value)} | ||
| onClick={(event) => event.stopPropagation()} | ||
| onKeyDown={(event) => { | ||
| event.stopPropagation(); | ||
| if (event.key === "Enter") { | ||
| event.preventDefault(); | ||
| onSubmit(); | ||
| } else if (event.key === "Escape") { | ||
| event.preventDefault(); | ||
| onCancel(); | ||
| } |
There was a problem hiding this comment.
Prevent blur from re-submitting after Enter/Escape handling.
Line 34 plus Line 39–44 can double-fire submit on Enter, and Escape can be followed by blur-submit, negating cancel.
Suggested fix
import { useEffect, useRef } from "react";
@@
export function RootRenameInput({
@@
}: RootRenameInputProps) {
const inputRef = useRef<HTMLInputElement>(null);
+ const skipBlurSubmitRef = useRef(false);
@@
<input
@@
- onBlur={onSubmit}
+ onBlur={() => {
+ if (skipBlurSubmitRef.current) {
+ skipBlurSubmitRef.current = false;
+ return;
+ }
+ onSubmit();
+ }}
@@
onKeyDown={(event) => {
event.stopPropagation();
if (event.key === "Enter") {
event.preventDefault();
+ skipBlurSubmitRef.current = true;
onSubmit();
} else if (event.key === "Escape") {
event.preventDefault();
+ skipBlurSubmitRef.current = true;
onCancel();
}
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/components/RootRenameInput/RootRenameInput.tsx`
around lines 34 - 45, The onBlur handler in RootRenameInput is causing a
double-submit after onKeyDown handles Enter (and causing cancel to be negated by
a subsequent blur); fix by introducing a temporary flag (e.g., ignoreNextBlur or
blurSuppressed) scoped to the component and toggle it inside the onKeyDown
handler when you handle Enter or Escape (set it true before calling
onSubmit/onCancel), then modify the onBlur handler to check that flag and, if
true, clear the flag and return without calling onSubmit; ensure the flag is
reset in both branches so normal blurs still submit and reference the
RootRenameInput component and its onKeyDown/onBlur/onSubmit/onCancel handlers
when applying the change.
| <button | ||
| aria-label="Equalize split" | ||
| className={cn( | ||
| "group bg-border focus-visible:ring-ring relative flex shrink-0 items-center justify-center transition-colors hover:bg-border/90 focus-visible:ring-1 focus-visible:ring-offset-1 focus-visible:outline-hidden", | ||
| isHorizontal | ||
| ? "h-full w-px cursor-col-resize after:absolute after:inset-y-0 after:left-1/2 after:w-1 after:-translate-x-1/2" | ||
| : "h-px w-full cursor-row-resize after:absolute after:inset-x-0 after:top-1/2 after:h-1 after:-translate-y-1/2", | ||
| )} | ||
| onDoubleClick={onDoubleClick} | ||
| type="button" |
There was a problem hiding this comment.
Add keyboard activation for the equalize action.
On Line 23, the action is mouse-only (onDoubleClick). Keyboard users can focus the button but cannot trigger equalize.
Suggested fix
<button
aria-label="Equalize split"
@@
onDoubleClick={onDoubleClick}
+ onKeyDown={(event) => {
+ if (event.key === "Enter" || event.key === " ") {
+ event.preventDefault();
+ onDoubleClick();
+ }
+ }}
type="button"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/pane-layout/src/react/components/PaneWorkspace/components/PaneSplitHandle/PaneSplitHandle.tsx`
around lines 15 - 24, The equalize action is only bound to the mouse via
onDoubleClick on the button (aria-label "Equalize split") so keyboard users
can't trigger it; add an onKeyDown handler on the same button (the element using
onDoubleClick in PaneSplitHandle) that listens for Enter and Space, calls the
same onDoubleClick/equalize handler, and prevents default for Space to avoid
scrolling, ensuring keyboard activation mirrors the double-click behavior for
accessibility.
| updateSession: protectedProcedure | ||
| .input( | ||
| z.object({ | ||
| sessionId: z.uuid(), | ||
| title: z.string().optional(), | ||
| }), | ||
| ) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const organizationId = ctx.session.session.activeOrganizationId; | ||
|
|
||
| if (!organizationId) { | ||
| throw new TRPCError({ | ||
| code: "FORBIDDEN", | ||
| message: "No active organization selected", | ||
| }); | ||
| } | ||
|
|
||
| const updates: Partial<typeof chatSessions.$inferInsert> = {}; | ||
| if (input.title !== undefined) { | ||
| updates.title = input.title; | ||
| } | ||
|
|
||
| if (Object.keys(updates).length === 0) { | ||
| return { updated: false }; | ||
| } | ||
|
|
||
| const [updated] = await db | ||
| .update(chatSessions) | ||
| .set(updates) | ||
| .where( | ||
| and( | ||
| eq(chatSessions.id, input.sessionId), | ||
| eq(chatSessions.organizationId, organizationId), | ||
| eq(chatSessions.createdBy, ctx.session.user.id), | ||
| ), | ||
| ) | ||
| .returning({ id: chatSessions.id }); | ||
|
|
||
| return { updated: !!updated }; | ||
| }), |
There was a problem hiding this comment.
Potential duplicate functionality: updateSession vs updateTitle.
Both mutations update the session title, but with different authorization scopes:
updateSession(lines 101-111): checksorganizationId+createdByupdateTitle(lines 174-186): only checkscreatedBy
This inconsistency could lead to authorization bypass if clients use updateTitle instead of updateSession. Consider deprecating one or aligning their authorization logic.
Also applies to: 174-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/trpc/src/router/chat/chat.ts` around lines 75 - 114, The two
mutations updateSession and updateTitle duplicate title-updating behavior but
use different authorization checks; make their logic consistent by requiring the
same org-scoped authorization in both places: fetch organizationId from
ctx.session.session.activeOrganizationId and include
eq(chatSessions.organizationId, organizationId) alongside
eq(chatSessions.createdBy, ctx.session.user.id) in the WHERE clause of the
update in updateTitle (or conversely, if you prefer org-less behavior, remove
the organizationId check from updateSession and centralize the shared logic into
a single helper used by both mutations); ensure the
db.update(chatSessions).set(...).where(...) call in both updateSession and
updateTitle uses the exact same predicate set and update fields, or deprecate
one mutation and route callers to the canonical method.
There was a problem hiding this comment.
11 issues found across 75 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/hooks/useSlashCommandExecutor/useSlashCommandExecutor.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/hooks/useSlashCommandExecutor/useSlashCommandExecutor.ts:60">
P2: Wrap `onResetSession()` in a try/catch here so a rejection doesn’t escape the callback and can surface a clear error instead of an unhandled promise rejection.
(Based on your team's feedback about handling async call rejections with try/catch.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/pane-layout/src/core/store/store.ts">
<violation number="1" location="packages/pane-layout/src/core/store/store.ts:688">
P2: `movePane` does not preserve the active pane on same-group reorders when `select` is false, and can leave a non-empty group with `activePaneId: null`.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts:20">
P2: Adding required `sidebarState`/`paneLayout` fields to the persisted workspace schema will invalidate existing `v2-workspace-local-state` entries. Add a migration or defaults so old rows still parse instead of being rejected.</violation>
</file>
<file name=".superset/lib/setup/steps.sh">
<violation number="1" location=".superset/lib/setup/steps.sh:481">
P1: Bypassing the Caddy HTTPS proxy reinstates the browser 6-connection limit for local SSE streams, causing streams to stall when multiple panes are open. Keep the URLs pointed to the Caddy proxy, and update the `Caddyfile` (below) to `reverse_proxy localhost:{$WRANGLER_PORT}`.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/PaneViewer.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/PaneViewer.tsx:40">
P2: Use a cross-platform basename extraction here; splitting on "/" fails for Windows paths so pane titles can show the full path.
(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts:59">
P2: Remove the `get()` check before `update()`. The preceding `ensureWorkspaceInSidebar` call synchronously guarantees the record exists, making this an unreachable condition and an unnecessary read.\n\n(Based on your team's feedback about refactoring read-then-update patterns into direct updates.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/electric-proxy/src/electric.ts">
<violation number="1" location="apps/electric-proxy/src/electric.ts:37">
P1: Validate required Electric URL env vars before constructing `URL`; the current fallback can throw `TypeError: Invalid URL` at runtime when vars are missing.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/WorkspaceChat.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/WorkspaceChat.tsx:45">
P2: Do not hardcode `isFocused` to true here; wire it to pane focus/active state so chat hotkeys and focus-driven behavior are only enabled for the focused pane.</violation>
</file>
<file name="packages/pane-layout/src/react/components/PaneWorkspace/components/PaneGroup/PaneGroup.tsx">
<violation number="1" location="packages/pane-layout/src/react/components/PaneWorkspace/components/PaneGroup/PaneGroup.tsx:95">
P2: Escape should cancel rename without persisting edits; currently blur still calls `saveEdit`, which can save a canceled value.</violation>
</file>
<file name="packages/pane-layout/src/core/store/utils/utils.ts">
<violation number="1" location="packages/pane-layout/src/core/store/utils/utils.ts:56">
P3: Add an index bounds check before reading `children[index]` so invalid paths fail with a clear error instead of an undefined-property crash.</violation>
</file>
<file name="packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/components/RootRenameInput/RootRenameInput.tsx">
<violation number="1" location="packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/components/RootRenameInput/RootRenameInput.tsx:34">
P2: Do not submit unconditionally on blur; Enter/Escape key handlers currently race with blur and can cause double-submit or submit-after-cancel.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const upstream = hasSourceCredentials | ||
| ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL) | ||
| : new URL(env.ELECTRIC_URL ?? ""); |
There was a problem hiding this comment.
P1: Validate required Electric URL env vars before constructing URL; the current fallback can throw TypeError: Invalid URL at runtime when vars are missing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/electric.ts, line 37:
<comment>Validate required Electric URL env vars before constructing `URL`; the current fallback can throw `TypeError: Invalid URL` at runtime when vars are missing.</comment>
<file context>
@@ -31,9 +31,19 @@ export function buildUpstreamUrl(
+ const hasSourceCredentials =
+ Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET);
+
+ const upstream = hasSourceCredentials
+ ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL)
+ : new URL(env.ELECTRIC_URL ?? "");
</file context>
| const upstream = hasSourceCredentials | |
| ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL) | |
| : new URL(env.ELECTRIC_URL ?? ""); | |
| let upstream: URL; | |
| if (hasSourceCredentials) { | |
| if (!env.ELECTRIC_CLOUD_URL) { | |
| throw new Error("ELECTRIC_CLOUD_URL is required when source credentials are configured"); | |
| } | |
| upstream = new URL("/v1/shape", env.ELECTRIC_CLOUD_URL); | |
| } else { | |
| if (!env.ELECTRIC_URL) { | |
| throw new Error("ELECTRIC_URL is required when source credentials are not configured"); | |
| } | |
| upstream = new URL(env.ELECTRIC_URL); | |
| } |
| await onResetSession(); | ||
| toast.success( | ||
| text === "/clear" | ||
| ? "Context cleared in a new chat session" | ||
| : "Started a new chat session", | ||
| ); |
There was a problem hiding this comment.
P2: Wrap onResetSession() in a try/catch here so a rejection doesn’t escape the callback and can surface a clear error instead of an unhandled promise rejection.
(Based on your team's feedback about handling async call rejections with try/catch.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/components/WorkspaceChatInterface/hooks/useSlashCommandExecutor/useSlashCommandExecutor.ts, line 60:
<comment>Wrap `onResetSession()` in a try/catch here so a rejection doesn’t escape the callback and can surface a clear error instead of an unhandled promise rejection.
(Based on your team's feedback about handling async call rejections with try/catch.) </comment>
<file context>
@@ -61,16 +57,12 @@ export function useSlashCommandExecutor({
- } else if (startResult.errorMessage) {
- toast.error(startResult.errorMessage);
- }
+ await onResetSession();
+ toast.success(
+ text === "/clear"
</file context>
| await onResetSession(); | |
| toast.success( | |
| text === "/clear" | |
| ? "Context cleared in a new chat session" | |
| : "Started a new chat session", | |
| ); | |
| try { | |
| await onResetSession(); | |
| toast.success( | |
| text === "/clear" | |
| ? "Context cleared in a new chat session" | |
| : "Started a new chat session", | |
| ); | |
| } catch (error) { | |
| console.warn("[chat] Failed to reset chat session", error); | |
| const resetError = "Failed to start a new chat session"; | |
| onSetErrorMessage(resetError); | |
| toast.error(resetError); | |
| } |
| createdAt: persistedDateSchema, | ||
| tabOrder: z.number().int().default(0), | ||
| sectionId: z.string().uuid().nullable().default(null), | ||
| sidebarState: z.object({ |
There was a problem hiding this comment.
P2: Adding required sidebarState/paneLayout fields to the persisted workspace schema will invalidate existing v2-workspace-local-state entries. Add a migration or defaults so old rows still parse instead of being rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts, line 20:
<comment>Adding required `sidebarState`/`paneLayout` fields to the persisted workspace schema will invalidate existing `v2-workspace-local-state` entries. Add a migration or defaults so old rows still parse instead of being rejected.</comment>
<file context>
@@ -11,12 +12,17 @@ export const dashboardSidebarProjectSchema = z.object({
createdAt: persistedDateSchema,
- tabOrder: z.number().int().default(0),
- sectionId: z.string().uuid().nullable().default(null),
+ sidebarState: z.object({
+ projectId: z.string().uuid(),
+ tabOrder: z.number().int().default(0),
</file context>
| ); | ||
|
|
||
| useEffect(() => { | ||
| ensureWorkspaceInSidebar(workspaceId, projectId); |
There was a problem hiding this comment.
P2: Remove the get() check before update(). The preceding ensureWorkspaceInSidebar call synchronously guarantees the record exists, making this an unreachable condition and an unnecessary read.\n\n
(Based on your team's feedback about refactoring read-then-update patterns into direct updates.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/PaneViewer/hooks/useV2WorkspacePaneLayout/useV2WorkspacePaneLayout.ts, line 59:
<comment>Remove the `get()` check before `update()`. The preceding `ensureWorkspaceInSidebar` call synchronously guarantees the record exists, making this an unreachable condition and an unnecessary read.\n\n
(Based on your team's feedback about refactoring read-then-update patterns into direct updates.) </comment>
<file context>
@@ -0,0 +1,99 @@
+ );
+
+ useEffect(() => {
+ ensureWorkspaceInSidebar(workspaceId, projectId);
+ }, [ensureWorkspaceInSidebar, projectId, workspaceId]);
+
</file context>
| <WorkspaceChatInterface | ||
| getOrCreateSession={getOrCreateSession} | ||
| initialLaunchConfig={null} | ||
| isFocused |
There was a problem hiding this comment.
P2: Do not hardcode isFocused to true here; wire it to pane focus/active state so chat hotkeys and focus-driven behavior are only enabled for the focused pane.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceChat/WorkspaceChat.tsx, line 45:
<comment>Do not hardcode `isFocused` to true here; wire it to pane focus/active state so chat hotkeys and focus-driven behavior are only enabled for the focused pane.</comment>
<file context>
@@ -43,16 +40,14 @@ export function WorkspaceChat({
<WorkspaceChatInterface
+ getOrCreateSession={getOrCreateSession}
+ initialLaunchConfig={null}
+ isFocused
+ onResetSession={handleNewChat}
sessionId={sessionId}
</file context>
| <Input | ||
| autoFocus | ||
| className="h-7" | ||
| onBlur={saveEdit} |
There was a problem hiding this comment.
P2: Escape should cancel rename without persisting edits; currently blur still calls saveEdit, which can save a canceled value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pane-layout/src/react/components/PaneWorkspace/components/PaneGroup/PaneGroup.tsx, line 95:
<comment>Escape should cancel rename without persisting edits; currently blur still calls `saveEdit`, which can save a canceled value.</comment>
<file context>
@@ -0,0 +1,276 @@
+ <Input
+ autoFocus
+ className="h-7"
+ onBlur={saveEdit}
+ onChange={(event) => setEditValue(event.target.value)}
+ onKeyDown={(event) => {
</file context>
| ref={inputRef} | ||
| className={className} | ||
| maxLength={maxLength} | ||
| onBlur={onSubmit} |
There was a problem hiding this comment.
P2: Do not submit unconditionally on blur; Enter/Escape key handlers currently race with blur and can cause double-submit or submit-after-cancel.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/components/PaneRootTabItem/components/RootRenameInput/RootRenameInput.tsx, line 34:
<comment>Do not submit unconditionally on blur; Enter/Escape key handlers currently race with blur and can cause double-submit or submit-after-cancel.</comment>
<file context>
@@ -0,0 +1,52 @@
+ ref={inputRef}
+ className={className}
+ maxLength={maxLength}
+ onBlur={onSubmit}
+ onChange={(event) => onChange(event.target.value)}
+ onClick={(event) => event.stopPropagation()}
</file context>
There was a problem hiding this comment.
1 issue found across 29 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/PaneRootTabs.tsx">
<violation number="1" location="packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/PaneRootTabs.tsx:122">
P2: Removing `roots` from this effect dependency can leave overflow state stale when the root list changes (especially from empty to non-empty). Re-run the effect when `roots` updates.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| useEffect(() => { | ||
| requestAnimationFrame(updateOverflow); | ||
| }, [updateOverflow]); |
There was a problem hiding this comment.
P2: Removing roots from this effect dependency can leave overflow state stale when the root list changes (especially from empty to non-empty). Re-run the effect when roots updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pane-layout/src/react/components/PaneWorkspace/components/PaneRootTabs/PaneRootTabs.tsx, line 122:
<comment>Removing `roots` from this effect dependency can leave overflow state stale when the root list changes (especially from empty to non-empty). Re-run the effect when `roots` updates.</comment>
<file context>
@@ -121,7 +119,7 @@ export function PaneRootTabs<TPaneData>({
useEffect(() => {
requestAnimationFrame(updateOverflow);
- }, [roots, updateOverflow]);
+ }, [updateOverflow]);
if (roots.length === 0) {
</file context>
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
There was a problem hiding this comment.
3 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".superset/lib/setup/steps.sh">
<violation number="1" location=".superset/lib/setup/steps.sh:482">
P2: Client Electric URLs now require Caddy unconditionally, but Caddy is still optional in dependency checks, which can produce a working setup script with broken Electric connectivity.</violation>
</file>
<file name="apps/electric-proxy/src/types.ts">
<violation number="1" location="apps/electric-proxy/src/types.ts:3">
P2: `ELECTRIC_SHAPE_URL` is typed optional even though it is required at runtime, which can cause request-time crashes on missing env config.</violation>
</file>
<file name="apps/electric-proxy/src/electric.ts">
<violation number="1" location="apps/electric-proxy/src/electric.ts:37">
P1: Avoid constructing `URL` from `""` when `ELECTRIC_SHAPE_URL` is unset; this can throw `Invalid URL` and break all proxy requests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const hasSourceCredentials = | ||
| Boolean(env.ELECTRIC_SOURCE_ID) && Boolean(env.ELECTRIC_SOURCE_SECRET); | ||
|
|
||
| const upstream = new URL(env.ELECTRIC_SHAPE_URL ?? ""); |
There was a problem hiding this comment.
P1: Avoid constructing URL from "" when ELECTRIC_SHAPE_URL is unset; this can throw Invalid URL and break all proxy requests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/electric.ts, line 37:
<comment>Avoid constructing `URL` from `""` when `ELECTRIC_SHAPE_URL` is unset; this can throw `Invalid URL` and break all proxy requests.</comment>
<file context>
@@ -34,9 +34,7 @@ export function buildUpstreamUrl(
- const upstream = hasSourceCredentials
- ? new URL("/v1/shape", env.ELECTRIC_CLOUD_URL)
- : new URL(env.ELECTRIC_URL ?? "");
+ const upstream = new URL(env.ELECTRIC_SHAPE_URL ?? "");
if (hasSourceCredentials) {
</file context>
| const upstream = new URL(env.ELECTRIC_SHAPE_URL ?? ""); | |
| const upstreamShapeUrl = env.ELECTRIC_SHAPE_URL; | |
| if (!upstreamShapeUrl) { | |
| throw new Error("Missing ELECTRIC_SHAPE_URL"); | |
| } | |
| const upstream = new URL(upstreamShapeUrl); |
| write_env_var "NEXT_PUBLIC_ELECTRIC_URL" "https://localhost:$CADDY_ELECTRIC_PORT/api/electric" | ||
| write_env_var "NEXT_PUBLIC_ELECTRIC_PROXY_URL" "https://localhost:$CADDY_ELECTRIC_PORT/api/electric" | ||
| echo "# Caddy HTTPS proxy for HTTP/2 (avoids browser 6-connection limit with Electric SSE streams)" | ||
| write_env_var "NEXT_PUBLIC_ELECTRIC_URL" "https://localhost:$CADDY_ELECTRIC_PORT" |
There was a problem hiding this comment.
P2: Client Electric URLs now require Caddy unconditionally, but Caddy is still optional in dependency checks, which can produce a working setup script with broken Electric connectivity.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .superset/lib/setup/steps.sh, line 482:
<comment>Client Electric URLs now require Caddy unconditionally, but Caddy is still optional in dependency checks, which can produce a working setup script with broken Electric connectivity.</comment>
<file context>
@@ -478,18 +478,18 @@ step_write_env() {
- write_env_var "NEXT_PUBLIC_ELECTRIC_URL" "http://localhost:$WRANGLER_PORT"
- write_env_var "NEXT_PUBLIC_ELECTRIC_PROXY_URL" "http://localhost:$WRANGLER_PORT"
+ echo "# Caddy HTTPS proxy for HTTP/2 (avoids browser 6-connection limit with Electric SSE streams)"
+ write_env_var "NEXT_PUBLIC_ELECTRIC_URL" "https://localhost:$CADDY_ELECTRIC_PORT"
+ write_env_var "NEXT_PUBLIC_ELECTRIC_PROXY_URL" "https://localhost:$CADDY_ELECTRIC_PORT"
} >> .env
</file context>
| ELECTRIC_CLOUD_URL: string; | ||
| ELECTRIC_SOURCE_ID: string; | ||
| ELECTRIC_SOURCE_SECRET: string; | ||
| ELECTRIC_SHAPE_URL?: string; |
There was a problem hiding this comment.
P2: ELECTRIC_SHAPE_URL is typed optional even though it is required at runtime, which can cause request-time crashes on missing env config.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/electric-proxy/src/types.ts, line 3:
<comment>`ELECTRIC_SHAPE_URL` is typed optional even though it is required at runtime, which can cause request-time crashes on missing env config.</comment>
<file context>
@@ -1,8 +1,7 @@
export interface Env {
AUTH_URL: string;
- ELECTRIC_URL?: string;
+ ELECTRIC_SHAPE_URL?: string;
ELECTRIC_SECRET?: string;
- ELECTRIC_CLOUD_URL?: string;
</file context>
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Introduces a VS Code–style multi-pane layout for v2 workspaces using
@superset/pane-layout, with per-workspace persisted layouts and initial Chat, Files, Terminal, and Browser panes. Also refactors v2 Chat to workspace-scoped TRPC sessions and updates Electric proxy/client env to use a local shape endpoint by default.New Features
@superset/pane-layoutpackage: shared engine + React components (roots, groups, tabs, splits) with tests.PaneViewerin v2 workspaces anduseV2WorkspacePaneLayoutpersisting tov2WorkspaceLocalState.paneLayout; empty and not-found states.v2WorkspaceLocalState.sidebarState(replacesv2SidebarWorkspaces).chat.createSession/chat.updateSessionTRPC, on-demand session creation, and cleaner slash command handling.ELECTRIC_SECRETandELECTRIC_SHAPE_URL; client vars point to the Caddy root; improved Browser webview drag passthrough.Migration
chat_sessions.v2_workspace_idand relations.ELECTRIC_SHAPE_URL+ELECTRIC_SECRET(falls back toELECTRIC_SOURCE_ID/ELECTRIC_SOURCE_SECRETwhen set).NEXT_PUBLIC_ELECTRIC_URLandNEXT_PUBLIC_ELECTRIC_PROXY_URLto the Caddy root (no/api/electricsuffix).@superset/pane-layout.Written for commit a3d43ba. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements