Skip to content

feat(canvas): Quill charts + dashboard-aware gen-UI agent#2536

Open
adamleithp wants to merge 47 commits into
mainfrom
feat/canvas-quill
Open

feat(canvas): Quill charts + dashboard-aware gen-UI agent#2536
adamleithp wants to merge 47 commits into
mainfrom
feat/canvas-quill

Conversation

@adamleithp

@adamleithp adamleithp commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Important

Stacked on feat/canvas (#2522) — merge that first.
This branch was cut from feat/canvas and depends on its canvas / gen-UI / Channels (project-bluebird) work; the changes here only make sense on top of it. Review #2522 first, merge it, then this PR should be rebased onto (or re-targeted at) the updated main. The base is currently main only so the full context is visible — do not merge this ahead of #2522.

What

Wires @posthog/quill-charts into the canvas gen-UI renderer and makes the chat agent anchored to the dashboard being edited.

Charts

  • Bump @posthog/quill 0.3.0-beta.1beta.14 and add @posthog/quill-charts@beta.14.
    • beta.14 quill tokens.css supplies the --data-color-* / --color-graph-* vars the charts theme from (with dark-mode scope) — beta.1 defined none, so charts rendered with the light fallback palette.
    • beta.14 ships a broken @base-ui/react: "catalog:" dep → pinned via a pnpm override. quill-charts excluded from minimumReleaseAge (published today).
  • New genui components: LineChart, BarChart, Sparkline (catalog + bodies + registry, both view & edit renderers).
    • Charts fill a flex-column wrapper with a definite height (their root is flex:1 1 0, so a plain block collapses them). Sparkline self-sizes via its height prop.
    • Theme via useChartTheme() (built-in fallback palette, dark-aware).

Agent

  • Prepend the current dashboard's title to the prompt every turn so the agent stays anchored to the open board (the agent session freezes its system prompt at session start). The chat UI still shows the clean user message.
  • New system-prompt rules: append-only by default (never replace/remove unless explicitly asked), build only from the catalog (Quill components/charts) unless told otherwise, prefer charts for time-bucketed metrics, and end every message with "Meep".

Test plan

  • pnpm --filter code typecheck ✅, biome ✅ (also via pre-commit).
  • Manual: build a dashboard with a time-series → renders a themed LineChart that respects dark mode; agent ends replies with "Meep" and appends rather than replaces.

Known follow-ups (not in this PR)

  • Append-only on reloaded boards: a reopened saved dashboard starts state.spec = {} in the canvas-gen main service while the UI shows the hydrated spec — so the agent's first append builds from empty and can wipe the visible board on next render. Real fix is seeding state.spec from the saved spec at session start.
  • Stat {$state}-binding crash: a binding/object Stat.value throws "Objects are not valid as a React child" → stuck "Rendering…". EditRenderer doesn't resolve {$state} bindings.
  • Chart tooltip surface vars (--color-bg-surface-*, --color-text-primary-inverse) aren't defined in any beta.14 quill CSS → tooltip bg/text fall back to chart defaults.

🤖 Generated with Claude Code

adamleithp and others added 30 commits June 8, 2026 11:18
Wrap the app in a left app rail (square Quill icon-lg buttons) switching
between a new Home space (/ hello-world scene + its own sidenav) and the
existing Code app (/code). Rail reserves macOS traffic-light space and is a
titlebar drag region.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Give the Home sidenav Quill folder collapsibles with a placeholder nav
(Features, Resources). The Features > Website item opens a new blank /website
canvas route. Centralize Home-space detection in canvas/spaces.ts so the whole
space shares its chrome.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add @json-render/core + @json-render/react. The /website canvas now renders an
agent-built, data-driven UI on the left with a chat panel hugging the right.

- Main CanvasGenService reuses AgentService (PostHog MCP auto-enabled) to run an
  ephemeral __preview__ session per thread with a json-render system prompt and
  bypassPermissions, forwards ACP session updates through @json-render/core's
  mixed-stream parser to assemble the Spec, and streams typed events over tRPC.
- AgentService gains systemPromptOverride to replace the coding-agent prompt for
  non-coding surfaces (keeps only project scoping).
- Renderer: shared json-render catalog (Page/Grid/Card/Stat/Table/BarList/…) +
  Radix registry, a thin canvasChatStore, a scoped subscription registrar, and
  the chat/composer UI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The renderer could OOM/crash while streaming a generated spec: partial or
malformed mid-stream frames were rendered and could recurse infinitely.

- Main only emits a spec once its root element actually exists (no partial
  frames).
- Wrap CanvasRenderer in an ErrorBoundary keyed on the spec, so a bad frame is
  caught and rendering recovers on the next valid frame.
- Only render when isNonEmptySpec(spec).
- Make the canvas subscription idempotent per thread (guards StrictMode
  double-mounts from stacking IPC listeners).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an Inbox rail item (above Code) that opens a full-screen /inbox route
reusing InboxView, without the code app chrome (header/sidebar/space-switcher).
The existing /code/inbox route is unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a numeric Quill destructive badge to the Inbox rail button, mirroring the
code sidebar's actionable-report count. Extract the count into a shared
useInboxSignalCount hook that reuses the sidebar's query cache (no extra
polling).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expand the Website space with a breadcrumb topbar and sub-navigation:
- /website is now a layout (breadcrumb topbar: Website > <page>) with children:
  index (canvas), new (TaskInput reused via onTaskCreated), settings
  (placeholder), and tasks/$taskId (TaskDetail reused).
- New tasks created from /website/new route into the Website space at
  /website/tasks/$id (not /code) and are tracked in a persisted websiteTasksStore.
- HomeSidebar gains a Website section: Canvas, New task, Settings, plus the
  list of created tasks to return to.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Render HomeSidebar nav items as Quill Button (variant default, size sm,
full-width left-aligned), expressing active via data-selected. Active logic
unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the Website "Canvas" entry with "Dashboards": /website redirects to the
default dashboard at /website/dashboards/$dashboardId, which renders mock
website-data dashboards (traffic, acquisition, engagement, conversion,
performance). The breadcrumb second crumb is a Quill combobox to switch the
active dashboard by name (Website > [dashboard]).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an Edit toggle (Quill outline button, ml-auto, data-selected) to the
dashboards breadcrumb bar. When active, the dashboard swaps its tiles for the
gen-UI canvas + side chat input for that dashboard. Make the canvas chat store
multi-thread (one thread per dashboard) so each dashboard keeps its own chat and
generated canvas.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Disable base-ui combobox filtering (filter={null}) so the dashboard dropdown
always lists every dashboard instead of collapsing to the selected one.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the mock dashboards with real, file-backed ones: a main DashboardsService
stores each dashboard as JSON (a json-render spec) under <appData>/dashboards.

- Dashboards list, combobox, and sidebar count now come from the saved files.
- A dashboard renders its saved spec read-only; Edit drops into the gen-UI
  canvas + chat for that dashboard's thread.
- Topbar Save (enabled when the generated spec differs from saved) persists it;
  Save as fork copies the current spec into a new dashboard.
- Empty state + sidebar "New dashboard" create a blank dashboard and open it in
  edit mode. Saved dashboards survive a refresh.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wrap create+navigate in try/catch with a toast + log so a failed New dashboard
action (e.g. backend not reachable) no longer silently does nothing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clicking Save now opens a Quill dialog to confirm/enter the dashboard name
before persisting. The dashboard name in the breadcrumb (edit mode) is also
inline-editable: hovering shows a faint border, clicking swaps to a Quill input
sized to match the text (no layout shift), committing on Enter/blur. Save is
disabled while renaming and re-enabled on blur.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Revert to the dropdown switcher in edit mode and a direct Save (no name dialog,
no click-to-rename input).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a Quill button group (refresh | gear) to the dashboard topbar. Refresh
refetches the dashboard data; the gear dropdown selects Static (manual refresh)
or a Polling interval (10s, 10min). While polling, the main button counts down
("Refreshing in XX"). Polling pauses in edit mode so the data stays put.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tching

Wrap the Polling label and interval items in DropdownMenuGroup so the
Menu.GroupLabel has its required Menu.Group context (fixes the
MenuGroupRootContext error). Spin the refresh icon (motion-safe:animate-spin)
while the dashboard data is fetching.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The canvas nav rail and Home/Website/Inbox spaces mount in one place
(__root.tsx). Gate that mount on a new project-bluebird flag: when off, the
app is the pre-canvas code-only shell. Stranded users on a canvas-only path
(cold-boot restore, stale deep link) are sent to /code once flags resolve, so
flagged users aren't bounced off /website during the load window.

- New PROJECT_BLUEBIRD_FLAG constant; defaults on in dev so local canvas work
  isn't hidden behind a flag PostHog doesn't serve locally.
- New useFeatureFlagsLoaded hook to defer the redirect until a flag value is
  trustworthy rather than acting on the false-before-load default.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- /website index is now a 3-wide card grid of live dashboard previews
  (CanvasRenderer scaled to a thumbnail) instead of redirecting to the first
  dashboard. Clicking a card opens the full dashboard.
- Each card has a hover-revealed "..." menu (outline) with a destructive
  Delete, rendered as a sibling of the card link so it never navigates.
- New dashboards.delete endpoint + deleteDashboard mutation (ENOENT treated as
  success).
- HomeSidebar nav items now actually highlight the active route: Quill's Button
  doesn't style data-selected, so consume it via a Tailwind utility (mirrors
  SidebarItem) and gate the attr on `active || undefined`.
- Dashboard breadcrumbs are now Website > Dashboards [> {name}], with the
  middle crumb linking back to the index.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Editing a saved dashboard rendered WebsiteCanvas, which only shows the chat
thread's spec. A freshly opened board has no thread yet, so the canvas fell
back to EMPTY_THREAD (spec: null) and looked wiped. Seed the thread from the
saved dashboard spec when entering edit, via a new ensureSpec action that only
hydrates an empty thread (never clobbers a live stream or in-session edits).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ettings)

Replace the placeholder Website/Features/Resources nav with server-backed
channels (desktop file-system folders). Each channel gets its own dashboards
(channel-scoped, file-backed), tasks, and settings, routed under
/website/$channelId. Adds channel create/delete, a Slack-style create modal,
orphan-dashboard migration into the first channel, and breadcrumb/nav polish.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Edit mode:
- Inline-edit static text (titles/labels) and drag-and-drop reorder via a
  key-aware EditRenderer over shared presentational bodies; an editable.ts
  "interpreter" gates which props are editable (literals only); data values
  show a locked "from query" hint. Cancel discards unsaved edits.

Refreshable data:
- Each Stat's HogQL is stored in spec.state.queries; a new DashboardQueryService
  runs them via the PostHog query endpoint and dashboards.refresh patches fresh
  values into the file (whole-board + per-card via ViewRenderer). Stat numbers
  are formatted at render.

Hardening:
- Canvas agent runs with a disallowedTools denylist (no file/shell/network) so
  bypassPermissions can't write files or run commands.
- Refresh is hidden in edit mode so a Save can't clobber refreshed values.

Plus CANVAS_MVP.md progress update.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Remove the Slack-like nav rail and Home/Inbox top-level spaces. The code
sidebar gains a [ Tasks | Channels ] tab bar (below the command center, gated
behind project-bluebird): Tasks is the existing list; Channels renders the
channel list (extracted to ChannelsList). Selecting a channel opens its
dashboards under /website/$channelId, now rendered inside the code chrome.

- Delete CanvasNav, HomeSidebar, spaces.ts, the top-level /inbox route.
- __root reverts to the code-only chrome; / redirects to /code.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When project-bluebird is on, the Tasks/Channels TabsList now sits outside
the scroll container so it stays put while the list scrolls, and the gap
below Command Center is tightened.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…stem

The canvas harness now always emits a top-level h1 Heading as the dashboard
title, and that h1 is the dashboard's name: saving syncs it to the file name.

Dashboards move from the local JSON store to the PostHog desktop file system —
each dashboard is a `dashboard`-typed row nested under its channel folder, with
the json-render spec in `meta.spec`. Renaming the h1 PATCHes the row's path, so
names stay in sync with the backend (the same surface that owns channel names).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Breadcrumbs (channel / Dashboards / name) and the dashboard controls now render
in the window title bar via the header store instead of a second standalone bar,
reclaiming ~40px of vertical space in the channel space. Adds the Dashboards
crumb, muted-foreground link color, and consistent vertical alignment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
adamleithp and others added 17 commits June 8, 2026 11:19
…AGENTS.md

A dashboard's own name is its H1, so the breadcrumb stops at "Dashboards" (still
a link back to the grid) instead of repeating the name. Restores right padding
on the header content row. Documents the breadcrumb + naming + storage patterns
in a canvas-feature AGENTS.md — notably: a page is not its own crumb, its H1 is.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A single dashboard shows "#channel / Dashboards" (Dashboards links back to the
grid); the dashboards grid itself shows only "#channel" since its own h1 is the
title. A page is never a crumb of itself.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Removes the adoptOrphans no-op (main service, tRPC procedure, renderer hook, and
its ChannelsList call) left over from the local-file store. Documents the desktop
file-system row's `meta` payload as a typed `DashboardFileMeta` zod schema, and
notes the last-write-wins tradeoff on `meta.spec` at the refresh write + in
AGENTS.md.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tabs

Brings back the Slack-like app rail (gated behind project-bluebird) switching
three top-level spaces: Code (the task app, untouched), Inbox (full-screen), and
Channels (the website space with its channel-list sidebar + dashboards). Adds the
top-level /inbox route and the rail/space branching in __root, with a redirect
guard sending stranded rail-only paths to /code when the flag is off.

Reverts the code sidebar changes: the Tasks/Channels tab bar is gone, the sidebar
is the plain task list again. The Channels space gets its own chrome, so
WebsiteLayout renders its own breadcrumb bar instead of the global header store.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The channel-list sidebar started flush at the top while the outlet's breadcrumb
bar pushed its column down. Add a matching h-10 "Channels" bar above the list so
both columns start on the same line, like /code's header sits above the sidebar.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the small top "+" icon button with a full-width outline "New channel"
button pinned to the bottom of the channels nav; the channel list scrolls above
it. ChannelsList now owns its own scroll so the footer stays put.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drops the dnd-kit sortable/drag-handle reordering from the dashboard edit mode
and the moveChild store action. Inline text editing and the hover frame stay; the
drag affordance is gone.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The rail is now Code + Channels. Drops the Inbox rail item (and its signal-count
badge) and the now-dead top-level /inbox route + space branch in __root. The
in-code inbox (/code/inbox via the code sidebar) is unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…t menu

- Breadcrumb: a faded # icon before the channel name.
- Channel "..." menu: fit-content width, plus a new "Rename channel" item that
  opens a modal (PATCHes the desktop FS folder path via a new
  renameDesktopFileSystemChannel client method + renameChannel mutation).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ChannelGridLink renders a quill Button (default variant) that navigates, so the
channel / Dashboards crumbs get the standard button hover instead of a bare link.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Top bar now holds only the breadcrumbs. A second bar below carries a (dead)
Filter button on the left and the dashboard controls (refresh/edit/save) on the
right. The leaf crumb is now a disabled quill button so it matches the clickable
crumbs' shape instead of being bare text.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Top bar: breadcrumbs + primary actions on the right — New dashboard on the
  grid, Edit/Cancel/Save/Save-as-fork on a dashboard.
- Toolbar below: a (now outline) Filter on the left, just Refresh on the right.
- Drop the in-page "Dashboards" h1; it's now the current (disabled) breadcrumb on
  the grid. New-dashboard header row removed from the grid page.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The toolbar (Filter + refresh) now renders only on the dashboards grid and a
single dashboard, not on the channel's tasks/settings views.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a nested "Sessions" folder collapsible inside each channel (same styling +
child indent) holding New task, the channel's task list, and five dummy status
filters (Backlog/Todo/Needs Review/Done/Cancelled) with status icons. NavButton
gains an optional leading icon. Settings moves below the group.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wire @posthog/quill-charts into the canvas gen-UI renderer and make the
chat agent anchored to the dashboard being edited.

- deps: bump @posthog/quill 0.3.0-beta.1 -> beta.14 (so its tokens.css
  supplies the --data-color-*/--color-graph-* vars the charts theme from,
  with dark-mode scope) and add @posthog/quill-charts beta.14. beta.14
  ships a broken `@base-ui/react: "catalog:"` dep, so pin it via a
  pnpm override; exclude quill-charts from minimumReleaseAge.
- genui: add LineChart, BarChart and Sparkline components (catalog +
  bodies + registry). Charts fill a flex-column wrapper with a definite
  height (their root is `flex:1 1 0`); Sparkline self-sizes via `height`.
  Theme comes from useChartTheme() with a built-in fallback palette.
- agent: prepend the current dashboard's title to the prompt every turn
  so the agent stays anchored to the open board (its system prompt is
  frozen at session start). The chat UI still shows the clean message.
- agent rules: append-only by default (never replace/remove unless asked),
  build only from the catalog (Quill components/charts) unless told
  otherwise, prefer charts for time-bucketed metrics, and end every
  message with "Meep".

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
apps/code/src/renderer/features/canvas/subscriptions.ts:22-44
**`active` set not cleared on subscription error — thread permanently unrecoverable on same page**

When `onError` fires the subscription is dead, but `active.has(threadId)` still returns `true`. Any subsequent call to `registerCanvasSubscription` (e.g. a React remount triggered by an error boundary) returns the no-op `() => {}` guard. Because the backend's `generate()` mutation **awaits the full agent run** and emits `done` via this subscription, if the subscription dies mid-generation the mutation still resolves successfully on the backend, `isStreaming` is set to `true` in the store by `send()`, and `finish()` is never called — leaving the thread permanently stuck in streaming state without a way to recover short of navigating away.

Adding `active.delete(threadId)` inside `onError` would let tRPC's built-in reconnect logic (or a remount) establish a fresh subscription.

### Issue 2 of 3
apps/code/src/main/services/dashboards/service.ts:53-68
`listAll()` does a sequential waterfall of HTTP requests — up to `MAX_PAGES` (50) round-trips — on every call to `list(channelId)`, loading every file-system row for the whole project before filtering client-side. Even for small projects this fires at least one unfiltered API call per channel navigation. If the API supports filtering by type or path prefix, a single filtered request would be far cheaper; if not, caching the full result across the hook's 5 s `staleTime` at the tRPC layer would at least avoid the repeat scans.

```suggestion
  private async listAll(type?: string): Promise<FsEntry[]> {
    const all: FsEntry[] = [];
    // Filter by type on the first request when the API supports it.
    let suffix = type ? `?type=${encodeURIComponent(type)}` : "";
    for (let i = 0; i < MAX_PAGES; i++) {
      const res = await this.fsFetch(suffix);
      if (!res.ok) throw new Error(`Failed to list dashboards (${res.status})`);
      const page = (await res.json()) as {
        next: string | null;
        results: FsEntry[];
      };
      all.push(...page.results);
      if (!page.next) return all;
      suffix = new URL(page.next).search; // carries the pagination offset
    }
    return all;
  }
```

### Issue 3 of 3
apps/code/src/renderer/features/canvas/genui/bodies.tsx:107-130
**Superfluous `ctx` prop on data-only body components**

`GridBody`, `TableBody`, `BarListBody`, `LineChartBody`, `BarChartBody`, and `SparklineBody` all declare `ctx: BodyCtx` in their prop type but never destructure or use it. The pattern is intentional for components that have editable text (e.g. `CardBody`, `StatBody`), but for pure-data or structural components with no string props to wrap, the declaration is dead weight. This affects readability when adding new body components — it's not clear whether the omission of `ctx` usage is deliberate or a forgotten implementation.

Reviews (1): Last reviewed commit: "feat(canvas): Quill charts + dashboard-a..." | Re-trigger Greptile

Comment on lines +22 to +44
const store = useCanvasChatStore.getState();
switch (event.type) {
case "prose":
store.appendProse(threadId, event.text);
break;
case "spec":
store.setSpec(threadId, event.spec as unknown as Spec);
break;
case "tool":
store.noteTool(threadId, event.toolName, event.status);
break;
case "done":
store.finish(threadId);
break;
case "error":
store.fail(threadId, event.message);
break;
case "started":
break;
}
},
onError: (error) => {
log.error("Canvas subscription error", { error });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 active set not cleared on subscription error — thread permanently unrecoverable on same page

When onError fires the subscription is dead, but active.has(threadId) still returns true. Any subsequent call to registerCanvasSubscription (e.g. a React remount triggered by an error boundary) returns the no-op () => {} guard. Because the backend's generate() mutation awaits the full agent run and emits done via this subscription, if the subscription dies mid-generation the mutation still resolves successfully on the backend, isStreaming is set to true in the store by send(), and finish() is never called — leaving the thread permanently stuck in streaming state without a way to recover short of navigating away.

Adding active.delete(threadId) inside onError would let tRPC's built-in reconnect logic (or a remount) establish a fresh subscription.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/canvas/subscriptions.ts
Line: 22-44

Comment:
**`active` set not cleared on subscription error — thread permanently unrecoverable on same page**

When `onError` fires the subscription is dead, but `active.has(threadId)` still returns `true`. Any subsequent call to `registerCanvasSubscription` (e.g. a React remount triggered by an error boundary) returns the no-op `() => {}` guard. Because the backend's `generate()` mutation **awaits the full agent run** and emits `done` via this subscription, if the subscription dies mid-generation the mutation still resolves successfully on the backend, `isStreaming` is set to `true` in the store by `send()`, and `finish()` is never called — leaving the thread permanently stuck in streaming state without a way to recover short of navigating away.

Adding `active.delete(threadId)` inside `onError` would let tRPC's built-in reconnect logic (or a remount) establish a fresh subscription.

How can I resolve this? If you propose a fix, please make it concise.

Comment on lines +53 to +68
private async listAll(): Promise<FsEntry[]> {
const all: FsEntry[] = [];
let suffix = "";
for (let i = 0; i < MAX_PAGES; i++) {
const res = await this.fsFetch(suffix);
if (!res.ok) throw new Error(`Failed to list dashboards (${res.status})`);
const page = (await res.json()) as {
next: string | null;
results: FsEntry[];
};
all.push(...page.results);
if (!page.next) return all;
suffix = new URL(page.next).search; // carries the pagination offset
}
return all;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 listAll() does a sequential waterfall of HTTP requests — up to MAX_PAGES (50) round-trips — on every call to list(channelId), loading every file-system row for the whole project before filtering client-side. Even for small projects this fires at least one unfiltered API call per channel navigation. If the API supports filtering by type or path prefix, a single filtered request would be far cheaper; if not, caching the full result across the hook's 5 s staleTime at the tRPC layer would at least avoid the repeat scans.

Suggested change
private async listAll(): Promise<FsEntry[]> {
const all: FsEntry[] = [];
let suffix = "";
for (let i = 0; i < MAX_PAGES; i++) {
const res = await this.fsFetch(suffix);
if (!res.ok) throw new Error(`Failed to list dashboards (${res.status})`);
const page = (await res.json()) as {
next: string | null;
results: FsEntry[];
};
all.push(...page.results);
if (!page.next) return all;
suffix = new URL(page.next).search; // carries the pagination offset
}
return all;
}
private async listAll(type?: string): Promise<FsEntry[]> {
const all: FsEntry[] = [];
// Filter by type on the first request when the API supports it.
let suffix = type ? `?type=${encodeURIComponent(type)}` : "";
for (let i = 0; i < MAX_PAGES; i++) {
const res = await this.fsFetch(suffix);
if (!res.ok) throw new Error(`Failed to list dashboards (${res.status})`);
const page = (await res.json()) as {
next: string | null;
results: FsEntry[];
};
all.push(...page.results);
if (!page.next) return all;
suffix = new URL(page.next).search; // carries the pagination offset
}
return all;
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/dashboards/service.ts
Line: 53-68

Comment:
`listAll()` does a sequential waterfall of HTTP requests — up to `MAX_PAGES` (50) round-trips — on every call to `list(channelId)`, loading every file-system row for the whole project before filtering client-side. Even for small projects this fires at least one unfiltered API call per channel navigation. If the API supports filtering by type or path prefix, a single filtered request would be far cheaper; if not, caching the full result across the hook's 5 s `staleTime` at the tRPC layer would at least avoid the repeat scans.

```suggestion
  private async listAll(type?: string): Promise<FsEntry[]> {
    const all: FsEntry[] = [];
    // Filter by type on the first request when the API supports it.
    let suffix = type ? `?type=${encodeURIComponent(type)}` : "";
    for (let i = 0; i < MAX_PAGES; i++) {
      const res = await this.fsFetch(suffix);
      if (!res.ok) throw new Error(`Failed to list dashboards (${res.status})`);
      const page = (await res.json()) as {
        next: string | null;
        results: FsEntry[];
      };
      all.push(...page.results);
      if (!page.next) return all;
      suffix = new URL(page.next).search; // carries the pagination offset
    }
    return all;
  }
```

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +107 to +130
children?: ReactNode;
ctx: BodyCtx;
}) {
return (
<Grid columns={String(props.columns ?? 2)} gap="3" width="auto">
{children}
</Grid>
);
}

export function CardBody({
props,
children,
ctx,
}: {
props: CardProps;
children?: ReactNode;
ctx: BodyCtx;
}) {
return (
<Box className="rounded-lg border border-gray-6 bg-gray-1 p-4">
{props.title && (
<Text size="2" weight="bold" className="mb-2 block text-gray-12">
{ctx.text("/title", props.title)}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Superfluous ctx prop on data-only body components

GridBody, TableBody, BarListBody, LineChartBody, BarChartBody, and SparklineBody all declare ctx: BodyCtx in their prop type but never destructure or use it. The pattern is intentional for components that have editable text (e.g. CardBody, StatBody), but for pure-data or structural components with no string props to wrap, the declaration is dead weight. This affects readability when adding new body components — it's not clear whether the omission of ctx usage is deliberate or a forgotten implementation.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/canvas/genui/bodies.tsx
Line: 107-130

Comment:
**Superfluous `ctx` prop on data-only body components**

`GridBody`, `TableBody`, `BarListBody`, `LineChartBody`, `BarChartBody`, and `SparklineBody` all declare `ctx: BodyCtx` in their prop type but never destructure or use it. The pattern is intentional for components that have editable text (e.g. `CardBody`, `StatBody`), but for pure-data or structural components with no string props to wrap, the declaration is dead weight. This affects readability when adding new body components — it's not clear whether the omission of `ctx` usage is deliberate or a forgotten implementation.

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant