Implement PR status checks for the v2 workspace sidebar#2527
Merged
Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds WebSocket-based workspace terminal support and a PullRequestRuntimeManager with GitHub integration; persists PR metadata in DB, exposes PR routes via tRPC, and surfaces PR/checks data in the dashboard sidebar and hover cards. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant TerminalComp as WorkspaceTerminal
participant WS as WebSocket
participant HostSvc as Host Service
participant PTY as PTY Shell
User->>Browser: Open workspace page
Browser->>TerminalComp: mount WorkspaceTerminal
TerminalComp->>WS: open ws://host/terminal/:workspaceId?reconnect=key
WS->>HostSvc: websocket upgrade -> register route
HostSvc->>PTY: spawn shell (cwd=worktree)
PTY-->>HostSvc: data / exit events
HostSvc-->>WS: send {type: "data"/"exit"/"error"}
WS-->>TerminalComp: onmessage -> write to xterm
TerminalComp->>WS: send {type: "input", data} or {type: "resize", cols, rows}
WS->>HostSvc: forward to PTY
sequenceDiagram
participant Sidebar as Dashboard Sidebar
participant Hook as useDashboardSidebarData
participant TRPC as tRPC Client
participant HostSvc as Host Service Runtime
participant DB as Database
participant GitHub as GitHub API
Sidebar->>Hook: mount
Hook->>TRPC: query pullRequests.getByWorkspaces(workspaceIds)
TRPC->>HostSvc: ctx.runtime.pullRequests.getPullRequestsByWorkspaces(ids)
HostSvc->>DB: read related PR/workspace rows
DB-->>HostSvc: PR records
alt stale/refresh needed
HostSvc->>GitHub: fetchRepositoryPullRequests(repo)
GitHub-->>HostSvc: PR GraphQL nodes
HostSvc->>DB: upsert pull_requests, update workspace.pull_request_id
end
HostSvc-->>TRPC: return workspace PR snapshots
TRPC-->>Hook: deliver PR mapping
Hook-->>Sidebar: attach pullRequest to workspace and render items
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
7 issues found across 44 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="packages/host-service/src/terminal/terminal.ts">
<violation number="1" location="packages/host-service/src/terminal/terminal.ts:149">
P2: Validate websocket payload shape after `JSON.parse` instead of relying on a type cast; malformed messages can trigger runtime errors in `terminal.write`/`terminal.resize`.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx:129">
P2: Guard the `close` state update so stale sockets from a previous effect run cannot overwrite the status of a newer active connection.</violation>
</file>
<file name="packages/host-service/drizzle/0001_famous_mindworm.sql">
<violation number="1" location="packages/host-service/drizzle/0001_famous_mindworm.sql:33">
P2: The migration defines `workspaces.pull_request_id` without `ON DELETE SET NULL`, so DB behavior does not match the schema and can block pull request row deletions.</violation>
</file>
<file name="packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts">
<violation number="1" location="packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts:109">
P3: Avoid an empty catch here; parse failures are silently dropped and become hard to diagnose.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/host-service/src/db/schema.ts">
<violation number="1" location="packages/host-service/src/db/schema.ts:63">
P2: The PR uniqueness key is missing `projectId`, so rows intended to be project-scoped can be reused and overwritten across projects for the same repo/PR number.</violation>
</file>
<file name="packages/host-service/src/runtime/pull-requests/pull-requests.ts">
<violation number="1" location="packages/host-service/src/runtime/pull-requests/pull-requests.ts:79">
P1: Catch rejections from scheduled background async calls instead of using uncaught fire-and-forget promises.
(Based on your team's feedback about handling async calls with proper await/catch.) [FEEDBACK_USED]</violation>
<violation number="2" location="packages/host-service/src/runtime/pull-requests/pull-requests.ts:297">
P2: Avoid an empty catch path here; log the remote resolution failure before returning so PR sync issues remain diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (this.branchSyncTimer || this.projectRefreshTimer) return; | ||
|
|
||
| this.branchSyncTimer = setInterval(() => { | ||
| void this.syncWorkspaceBranches(); |
Contributor
There was a problem hiding this comment.
P1: Catch rejections from scheduled background async calls instead of using uncaught fire-and-forget promises.
(Based on your team's feedback about handling async calls with proper await/catch.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/runtime/pull-requests/pull-requests.ts, line 79:
<comment>Catch rejections from scheduled background async calls instead of using uncaught fire-and-forget promises.
(Based on your team's feedback about handling async calls with proper await/catch.) </comment>
<file context>
@@ -0,0 +1,406 @@
+ if (this.branchSyncTimer || this.projectRefreshTimer) return;
+
+ this.branchSyncTimer = setInterval(() => {
+ void this.syncWorkspaceBranches();
+ }, BRANCH_SYNC_INTERVAL_MS);
+ this.projectRefreshTimer = setInterval(() => {
</file context>
|
|
||
| let message: TerminalClientMessage; | ||
| try { | ||
| message = JSON.parse(String(event.data)) as TerminalClientMessage; |
Contributor
There was a problem hiding this comment.
P2: Validate websocket payload shape after JSON.parse instead of relying on a type cast; malformed messages can trigger runtime errors in terminal.write/terminal.resize.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/terminal/terminal.ts, line 149:
<comment>Validate websocket payload shape after `JSON.parse` instead of relying on a type cast; malformed messages can trigger runtime errors in `terminal.write`/`terminal.resize`.</comment>
<file context>
@@ -0,0 +1,178 @@
+
+ let message: TerminalClientMessage;
+ try {
+ message = JSON.parse(String(event.data)) as TerminalClientMessage;
+ } catch {
+ sendMessage(ws, {
</file context>
| }); | ||
|
|
||
| socket.addEventListener("close", () => { | ||
| setConnectionState("closed"); |
Contributor
There was a problem hiding this comment.
P2: Guard the close state update so stale sockets from a previous effect run cannot overwrite the status of a newer active connection.
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/WorkspaceTerminal/WorkspaceTerminal.tsx, line 129:
<comment>Guard the `close` state update so stale sockets from a previous effect run cannot overwrite the status of a newer active connection.</comment>
<file context>
@@ -0,0 +1,171 @@
+ });
+
+ socket.addEventListener("close", () => {
+ setConnectionState("closed");
+ });
+
</file context>
| ALTER TABLE `projects` ADD `repo_url` text;--> statement-breakpoint | ||
| ALTER TABLE `projects` ADD `remote_name` text;--> statement-breakpoint | ||
| ALTER TABLE `workspaces` ADD `head_sha` text;--> statement-breakpoint | ||
| ALTER TABLE `workspaces` ADD `pull_request_id` text REFERENCES pull_requests(id);--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
P2: The migration defines workspaces.pull_request_id without ON DELETE SET NULL, so DB behavior does not match the schema and can block pull request row deletions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/drizzle/0001_famous_mindworm.sql, line 33:
<comment>The migration defines `workspaces.pull_request_id` without `ON DELETE SET NULL`, so DB behavior does not match the schema and can block pull request row deletions.</comment>
<file context>
@@ -0,0 +1,34 @@
+ALTER TABLE `projects` ADD `repo_url` text;--> statement-breakpoint
+ALTER TABLE `projects` ADD `remote_name` text;--> statement-breakpoint
+ALTER TABLE `workspaces` ADD `head_sha` text;--> statement-breakpoint
+ALTER TABLE `workspaces` ADD `pull_request_id` text REFERENCES pull_requests(id);--> statement-breakpoint
+CREATE INDEX `workspaces_pull_request_id_idx` ON `workspaces` (`pull_request_id`);
\ No newline at end of file
</file context>
| table.repoName, | ||
| table.headBranch, | ||
| ), | ||
| uniqueIndex("pull_requests_repo_pr_unique").on( |
Contributor
There was a problem hiding this comment.
P2: The PR uniqueness key is missing projectId, so rows intended to be project-scoped can be reused and overwritten across projects for the same repo/PR number.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/db/schema.ts, line 63:
<comment>The PR uniqueness key is missing `projectId`, so rows intended to be project-scoped can be reused and overwritten across projects for the same repo/PR number.</comment>
<file context>
@@ -1,17 +1,74 @@
+ table.repoName,
+ table.headBranch,
+ ),
+ uniqueIndex("pull_requests_repo_pr_unique").on(
+ table.repoProvider,
+ table.repoOwner,
</file context>
| return null; | ||
| } | ||
| remoteUrl = value.trim(); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
P2: Avoid an empty catch path here; log the remote resolution failure before returning so PR sync issues remain diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/runtime/pull-requests/pull-requests.ts, line 297:
<comment>Avoid an empty catch path here; log the remote resolution failure before returning so PR sync issues remain diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,406 @@
+ return null;
+ }
+ remoteUrl = value.trim();
+ } catch {
+ return null;
+ }
</file context>
| typeof item.status === "string" && | ||
| (item.url === null || typeof item.url === "string"), | ||
| ); | ||
| } catch { |
Contributor
There was a problem hiding this comment.
P3: Avoid an empty catch here; parse failures are silently dropped and become hard to diagnose.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts, line 109:
<comment>Avoid an empty catch here; parse failures are silently dropped and become hard to diagnose.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -0,0 +1,150 @@
+ typeof item.status === "string" &&
+ (item.url === null || typeof item.url === "string"),
+ );
+ } catch {
+ return [];
+ }
</file context>
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx (1)
6-6: Prefer alias import over deep relative path.Use the configured alias here to avoid brittle deep traversal imports.
♻️ Suggested import update
-import { useWorkspaceHostUrl } from "../../../providers/WorkspaceTrpcProvider/WorkspaceTrpcProvider"; +import { useWorkspaceHostUrl } from "renderer/routes/_authenticated/_dashboard/v2-workspace/providers/WorkspaceTrpcProvider/WorkspaceTrpcProvider";As per coding guidelines:
apps/desktop/**/*.{ts,tsx}: Use alias as defined intsconfig.jsonwhen possible.🤖 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/WorkspaceTerminal/WorkspaceTerminal.tsx at line 6, The import of useWorkspaceHostUrl in WorkspaceTerminal.tsx uses a brittle deep relative path; replace the relative import with the project's TypeScript path alias (import useWorkspaceHostUrl from the configured alias path for the WorkspaceTrpcProvider) so the module resolves via tsconfig aliases (keep the identifier useWorkspaceHostUrl and update the import statement in WorkspaceTerminal.tsx to use the alias form).packages/host-service/src/app.ts (1)
74-84: Type cast toRecord<string, unknown>bypasses type safety.The context object is cast to
Record<string, unknown>, losing the type guarantees fromHostServiceContext. If the tRPC server'screateContextexpects a specific type, consider properly typing the return or using a type assertion toHostServiceContextinstead.Suggested fix
createContext: async () => - ({ + <HostServiceContext>{ git, github, api, db, runtime, deviceClientId: options?.deviceClientId ?? null, deviceName: options?.deviceName ?? null, - }) as Record<string, unknown>, + },You may need to adjust the trpcServer generic or context type to accept
HostServiceContext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/app.ts` around lines 74 - 84, The current createContext function returns an object and unsafely casts it to Record<string, unknown>, losing HostServiceContext typing; update the createContext return to be typed as HostServiceContext (or adjust the trpcServer generic to accept HostServiceContext) so the compiler enforces the expected fields (git, github, api, db, runtime, deviceClientId, deviceName). Locate the createContext implementation and replace the broad Record<string, unknown> cast with a proper HostServiceContext type assertion or explicit return type, ensuring all context properties match HostServiceContext; if needed, update the trpcServer generics to use HostServiceContext so downstream handlers get correct types.
🤖 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/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`:
- Around line 107-133: The current code builds pull request queries only for
localWorkspaceIds (derived from local device filtering) which causes
remote/cloud rows to always show pullRequest: null even though the host service
can return PR snapshots for arbitrary workspace IDs; update the query to use all
workspace IDs from sidebarWorkspaces (e.g., derive workspaceIds =
sidebarWorkspaces.map(w => w.id).sort()) and change the enabled condition to
check activeHostService !== null and workspaceIds.length > 0, and then, where
rows are populated (the logic around hostType === "local-device"), attach pull
request data by looking up pullRequestData.workspaces by workspace id regardless
of hostType (or explicitly render a “not available for remote/cloud” UI if you
intend to restrict), ensuring you still call
activeHostService?.client.pullRequests.getByWorkspaces.query({ workspaceIds })
so the server can return snapshots for any workspace ids.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx:
- Around line 35-39: The websocketUrl computed in the useMemo for websocketUrl
should incorporate reconnectKey so changes to reconnectKey change the URL itself
rather than only being used to trigger effects; update the useMemo that builds
the URL (currently referencing hostUrl and workspaceId) to append reconnectKey
as a query parameter (or otherwise encode it into the path) and include
reconnectKey in the useMemo dependency list, and then remove reconnectKey from
the effect dependency that currently only uses it to re-trigger reconnection;
refer to the websocketUrl useMemo, reconnectKey, hostUrl, and workspaceId when
making the change.
- Around line 99-104: The effect in WorkspaceTerminal that creates a new
WebSocket currently uses inline listeners (socket.addEventListener("open", ...),
"message", etc.) which are not removed on cleanup; change to named handler
functions (e.g., handleOpen, handleMessage, handleClose, handleError) that call
setConnectionState and sendResize where needed, add them with
socket.addEventListener, and remove them in the effect cleanup via
socket.removeEventListener to avoid stale listeners updating state when
reconnectKey swaps; apply the same pattern for the listeners referenced around
lines 128-140.
In `@packages/host-service/drizzle/0001_famous_mindworm.sql`:
- Line 33: The ALTER TABLE migration adds workspaces.pull_request_id foreign key
without ON DELETE SET NULL, which mismatches the declared schema's pullRequestId
onDelete: "set null"; update the migration statement that creates the foreign
key on the workspaces.pull_request_id column to include ON DELETE SET NULL so
the database FK behavior matches the schema (ensure the migration's ALTER TABLE
`workspaces` ADD `pull_request_id` ... REFERENCES pull_requests(id) is changed
to include ON DELETE SET NULL to align with pullRequestId in schema.ts).
In `@packages/host-service/src/app.ts`:
- Around line 52-57: pullRequestRuntime.start() creates long-running intervals
but there is no teardown; add a proper shutdown path by exposing a
stop()/shutdown() method on PullRequestRuntimeManager (if one doesn't exist)
that clears timers, then keep a reference to the created instance
(pullRequestRuntime) and call pullRequestRuntime.stop()/shutdown() from the
app's shutdown hooks/server close handlers so intervals are cleared during
graceful shutdown or when the service is recreated.
In `@packages/host-service/src/db/schema.ts`:
- Around line 30-33: The pull_requests schema currently mixes repo-global
identity with a single-project FK (projectId) which causes ownership overwrites
and cascade deletes; update the schema so PRs are scoped consistently: either
(preferred) make PR uniqueness and runtime lookups include projectId (i.e.,
ensure the unique/index on {repoProvider, repoFullName, number} includes
projectId and keep the FK) or remove the projectId foreign key and model
project-to-PR ownership separately so PR rows are repo-global; change the
definition around pull_requests (columns projectId, repoProvider, number and any
unique/index definitions) and update any runtime lookup logic that uses
find-by-repo+number to also include projectId if you choose the project-scoped
option.
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 215-224: The current refresh error handler for
performProjectRefresh only logs via console.warn and doesn't persist the failure
to PullRequestWorkspaceSnapshot, so stale PR data remains without an error
signal; update the catch handler(s) for performProjectRefresh (and the analogous
block around lines 364-381) to write the caught error into the snapshot's error
field (e.g., set PullRequestWorkspaceSnapshot.error = error or call the snapshot
update method used elsewhere) and ensure the success path explicitly clears
snapshot.error = null (matching the success behavior), using the same
update/persistence mechanism the codebase uses for snapshots so failures are
durable and the UI can surface them.
- Around line 202-231: The bug is that refreshProject(projectId, true) returns
early when an existing in-flight refresh is present; change refreshProject so
that when an existing promise exists and force is true it still awaits the
existing promise but does NOT return—i.e., replace the early-return behavior in
the existing-in-flight branch: keep awaiting existing for both modes, but only
return if !force; then proceed to create a new refreshPromise by calling
performProjectRefresh (preserving the existing finally logic that deletes from
inFlightProjects and sets nextProjectRefreshAt using
PROJECT_REFRESH_INTERVAL_MS).
In `@packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts`:
- Line 4: The GraphQL query currently hardcodes pullRequests(first: 100) and
statusChecks(first: 50) which truncates results; update the query and
surrounding logic in query.ts to support cursor-based pagination: change the
pullRequests and statusChecks calls to accept variables (e.g., $prAfter,
$statusAfter) instead of fixed first offsets, use PageInfo (endCursor,
hasNextPage) and edges/nodes to iterate, and modify the function that executes
the query (the one constructing/processing the pullRequests response) to loop
fetching pages (passing after cursors) and accumulate all nodes until
hasNextPage is false; ensure the final return includes the full aggregated lists
for pullRequests and statusChecks and surface any partial-data errors if
requests fail mid-pagination.
In
`@packages/host-service/src/runtime/pull-requests/utils/parse-github-remote/parse-github-remote.ts`:
- Line 15: The HTTPS remote regex in parse-github-remote.ts currently rejects
authenticated URLs like "https://user@github.com/owner/repo.git"; update the
regex in that file so it accepts an optional "user@" segment immediately after
the "https://" prefix while preserving the existing named capture groups "owner"
and "name" and the optional ".git" suffix (this will allow
getProjectRepository() to parse authenticated HTTPS remotes instead of returning
null). Ensure the pattern only makes the "user@" part optional and non-greedy so
owner/name captures remain correct, and run/update any unit tests covering
parseGithubRemote or the regex array to include an authenticated-HTTPS example.
In
`@packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts`:
- Around line 10-15: The aggregate-check logic in pull-request-mappers.ts (the
code that reduces CheckStatus values into an overall status around lines 10 and
~64-68) currently treats only "failure" and "pending" as non-success, causing
"skipped" and "cancelled" to be treated as success; update that aggregation so
that "skipped" and "cancelled" are considered non-success (i.e., overall is not
"success" if any check is "failure", "pending", "skipped", or "cancelled").
Locate the reducer/mapper function that consumes the CheckStatus union (search
for the reduction that checks for "failure" or "pending") and add checks for
"skipped" and "cancelled" (or change logic to require all statuses === "success"
for overall success). Also update any related branches/conditionals and tests
that assume skipped/cancelled are successes.
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 147-161: The code unsafely casts JSON.parse(String(event.data)) to
TerminalClientMessage; instead add runtime validation after parsing (do not use
a blind type assertion) to verify the object has a valid "type" and required
fields before using them: when message.type === "input" ensure message.data is a
string before calling terminal.write(message.data); when message.type ===
"resize" ensure message.cols and message.rows are numbers before using them; for
any invalid shape sendMessage(ws, { type: "error", message: "Invalid terminal
message payload" }) and return. Use the existing symbols TerminalClientMessage,
terminal.write, sendMessage and event.data to locate where to insert these
checks.
In `@packages/host-service/src/trpc/router/pull-requests/pull-requests.ts`:
- Around line 4-16: The getByWorkspaces endpoint (inside pullRequestsRouter) is
defined with publicProcedure and calls
ctx.runtime.pullRequests.getPullRequestsByWorkspaces, exposing sensitive
PR/workspace data; change this and all other publicProcedure usages in routers
(e.g., workspace, github, project, git, cloud, health) to use your authenticated
TRPC middleware/protectedProcedure (or add an auth middleware to router) so
requests are validated before calling runtime methods, or alternatively enforce
strict origin/transport restrictions before enabling CORS; ensure the symbol
names to update include pullRequestsRouter, getByWorkspaces, publicProcedure,
router, and any other router exports that currently use publicProcedure.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx:
- Line 6: The import of useWorkspaceHostUrl in WorkspaceTerminal.tsx uses a
brittle deep relative path; replace the relative import with the project's
TypeScript path alias (import useWorkspaceHostUrl from the configured alias path
for the WorkspaceTrpcProvider) so the module resolves via tsconfig aliases (keep
the identifier useWorkspaceHostUrl and update the import statement in
WorkspaceTerminal.tsx to use the alias form).
In `@packages/host-service/src/app.ts`:
- Around line 74-84: The current createContext function returns an object and
unsafely casts it to Record<string, unknown>, losing HostServiceContext typing;
update the createContext return to be typed as HostServiceContext (or adjust the
trpcServer generic to accept HostServiceContext) so the compiler enforces the
expected fields (git, github, api, db, runtime, deviceClientId, deviceName).
Locate the createContext implementation and replace the broad Record<string,
unknown> cast with a proper HostServiceContext type assertion or explicit return
type, ensuring all context properties match HostServiceContext; if needed,
update the trpcServer generics to use HostServiceContext so downstream handlers
get correct types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04e023ea-4520-48c4-897b-d3f8db126812
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
apps/desktop/src/main/host-service/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContent/DashboardSidebarSectionContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/DashboardSidebarWorkspaceHoverCardContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceStatusBadge/DashboardSidebarWorkspaceStatusBadge.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/utils/getWorkspaceRowMocks.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/providers/WorkspaceTrpcProvider/WorkspaceTrpcProvider.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/DashboardNewWorkspaceDraftContext.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/DashboardNewWorkspaceForm.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DashboardNewWorkspaceFormHeader/DashboardNewWorkspaceFormHeader.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/hooks/useDashboardNewWorkspaceProjectSelection/useDashboardNewWorkspaceProjectSelection.tspackages/host-service/drizzle/0001_famous_mindworm.sqlpackages/host-service/drizzle/meta/0001_snapshot.jsonpackages/host-service/drizzle/meta/_journal.jsonpackages/host-service/package.jsonpackages/host-service/src/app.tspackages/host-service/src/db/schema.tspackages/host-service/src/runtime/pull-requests/index.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/src/runtime/pull-requests/utils/github-query/github-query.tspackages/host-service/src/runtime/pull-requests/utils/github-query/index.tspackages/host-service/src/runtime/pull-requests/utils/github-query/query.tspackages/host-service/src/runtime/pull-requests/utils/github-query/types.tspackages/host-service/src/runtime/pull-requests/utils/parse-github-remote/index.tspackages/host-service/src/runtime/pull-requests/utils/parse-github-remote/parse-github-remote.tspackages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/index.tspackages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.tspackages/host-service/src/serve.tspackages/host-service/src/terminal/terminal.tspackages/host-service/src/trpc/context/context.tspackages/host-service/src/trpc/context/index.tspackages/host-service/src/trpc/router/pull-requests/index.tspackages/host-service/src/trpc/router/pull-requests/pull-requests.tspackages/host-service/src/trpc/router/router.tspackages/host-service/src/types.ts
💤 Files with no reviewable changes (3)
- packages/host-service/src/trpc/context/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/utils/getWorkspaceRowMocks.ts
- packages/host-service/src/trpc/context/context.ts
Comment on lines
+107
to
+133
| const localWorkspaceIds = useMemo( | ||
| () => | ||
| sidebarWorkspaces | ||
| .filter( | ||
| (workspace) => | ||
| workspace.deviceType !== "cloud" && | ||
| workspace.deviceClientId === deviceInfo?.deviceId, | ||
| ) | ||
| .map((workspace) => workspace.id) | ||
| .sort(), | ||
| [deviceInfo?.deviceId, sidebarWorkspaces], | ||
| ); | ||
|
|
||
| const { data: pullRequestData } = useQuery({ | ||
| queryKey: [ | ||
| "dashboard-sidebar", | ||
| "pull-requests", | ||
| activeOrganizationId, | ||
| localWorkspaceIds, | ||
| ], | ||
| enabled: activeHostService !== null && localWorkspaceIds.length > 0, | ||
| refetchInterval: 30_000, | ||
| queryFn: () => | ||
| activeHostService?.client.pullRequests.getByWorkspaces.query({ | ||
| workspaceIds: localWorkspaceIds, | ||
| }) ?? Promise.resolve({ workspaces: [] }), | ||
| }); |
Contributor
There was a problem hiding this comment.
This feature is currently local-workspace only.
Lines 107-133 build the query from localWorkspaceIds, and Lines 205-208 only attach the result when hostType === "local-device". packages/host-service/src/runtime/pull-requests/pull-requests.ts (Lines 96-147) already returns PR snapshots by arbitrary workspace ID, so remote-device and cloud rows will always render pullRequest: null even when the host service has data for them. If that restriction is intentional, the UI should make it explicit instead of silently dropping the status.
💡 Minimal fix
- const localWorkspaceIds = useMemo(
- () =>
- sidebarWorkspaces
- .filter(
- (workspace) =>
- workspace.deviceType !== "cloud" &&
- workspace.deviceClientId === deviceInfo?.deviceId,
- )
- .map((workspace) => workspace.id)
- .sort(),
- [deviceInfo?.deviceId, sidebarWorkspaces],
- );
+ const workspaceIds = useMemo(
+ () => sidebarWorkspaces.map((workspace) => workspace.id).sort(),
+ [sidebarWorkspaces],
+ );
const { data: pullRequestData } = useQuery({
queryKey: [
"dashboard-sidebar",
"pull-requests",
activeOrganizationId,
- localWorkspaceIds,
+ workspaceIds,
],
- enabled: activeHostService !== null && localWorkspaceIds.length > 0,
+ enabled: activeHostService !== null && workspaceIds.length > 0,
refetchInterval: 30_000,
queryFn: () =>
activeHostService?.client.pullRequests.getByWorkspaces.query({
- workspaceIds: localWorkspaceIds,
+ workspaceIds,
}) ?? Promise.resolve({ workspaces: [] }),
});
...
- pullRequest:
- hostType === "local-device"
- ? (localPullRequestsByWorkspaceId.get(workspace.id) ?? null)
- : null,
+ pullRequest: localPullRequestsByWorkspaceId.get(workspace.id) ?? null,Also applies to: 205-208
🤖 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/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`
around lines 107 - 133, The current code builds pull request queries only for
localWorkspaceIds (derived from local device filtering) which causes
remote/cloud rows to always show pullRequest: null even though the host service
can return PR snapshots for arbitrary workspace IDs; update the query to use all
workspace IDs from sidebarWorkspaces (e.g., derive workspaceIds =
sidebarWorkspaces.map(w => w.id).sort()) and change the enabled condition to
check activeHostService !== null and workspaceIds.length > 0, and then, where
rows are populated (the logic around hostType === "local-device"), attach pull
request data by looking up pullRequestData.workspaces by workspace id regardless
of hostType (or explicitly render a “not available for remote/cloud” UI if you
intend to restrict), ensuring you still call
activeHostService?.client.pullRequests.getByWorkspaces.query({ workspaceIds })
so the server can return snapshots for any workspace ids.
Comment on lines
+99
to
+104
| socket.addEventListener("open", () => { | ||
| setConnectionState("open"); | ||
| sendResize(); | ||
| }); | ||
|
|
||
| socket.addEventListener("message", (event) => { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "WorkspaceTerminal.tsx" --type fRepository: superset-sh/superset
Length of output: 199
🏁 Script executed:
cat -n "apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx" | head -150 | tail -70Repository: superset-sh/superset
Length of output: 219
🏁 Script executed:
fd "WorkspaceTerminal.tsx" apps/desktop/src -type fRepository: superset-sh/superset
Length of output: 234
🏁 Script executed:
fd "WorkspaceTerminal.tsx" apps/desktop/srcRepository: superset-sh/superset
Length of output: 199
🏁 Script executed:
cat -n 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx' | sed -n '80,150p'Repository: superset-sh/superset
Length of output: 2229
🏁 Script executed:
cat -n 'apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceTerminal/WorkspaceTerminal.tsx' | sed -n '1,100p'Repository: superset-sh/superset
Length of output: 3137
Detach socket listeners in cleanup to prevent stale state updates.
When reconnectKey changes, the effect creates a new socket, but the old socket's event listeners remain attached. If the old socket closes after the new socket opens, its stale listeners will fire and overwrite state (e.g., set connectionState to closed even though the new socket is open). Named event handlers with removeEventListener in the cleanup function are required.
🔧 Suggested cleanup pattern
+ let disposed = false;
- socket.addEventListener("open", () => {
+ const onOpen = () => {
+ if (disposed) return;
setConnectionState("open");
sendResize();
- });
+ };
- socket.addEventListener("message", (event) => {
+ const onMessage = (event: MessageEvent) => {
+ if (disposed) return;
let message: TerminalServerMessage;
try {
message = JSON.parse(String(event.data)) as TerminalServerMessage;
} catch {
terminal.writeln("\r\n[terminal] invalid server payload");
return;
}
@@
terminal.writeln(
`\r\n[terminal] exited with code ${message.exitCode} (signal ${message.signal})`,
);
- });
+ };
- socket.addEventListener("close", () => {
+ const onClose = () => {
+ if (disposed) return;
setConnectionState("closed");
- });
+ };
- socket.addEventListener("error", () => {
+ const onError = () => {
+ if (disposed) return;
terminal.writeln("\r\n[terminal] websocket error");
- });
+ };
+ socket.addEventListener("open", onOpen);
+ socket.addEventListener("message", onMessage);
+ socket.addEventListener("close", onClose);
+ socket.addEventListener("error", onError);
return () => {
+ disposed = true;
+ socket.removeEventListener("open", onOpen);
+ socket.removeEventListener("message", onMessage);
+ socket.removeEventListener("close", onClose);
+ socket.removeEventListener("error", onError);
resizeObserver.disconnect();
onTerminalDataDispose.dispose();
socket.close();
terminal.dispose();
};Also applies to: 128-140
🤖 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/WorkspaceTerminal/WorkspaceTerminal.tsx
around lines 99 - 104, The effect in WorkspaceTerminal that creates a new
WebSocket currently uses inline listeners (socket.addEventListener("open", ...),
"message", etc.) which are not removed on cleanup; change to named handler
functions (e.g., handleOpen, handleMessage, handleClose, handleError) that call
setConnectionState and sendResize where needed, add them with
socket.addEventListener, and remove them in the effect cleanup via
socket.removeEventListener to avoid stale listeners updating state when
reconnectKey swaps; apply the same pattern for the listeners referenced around
lines 128-140.
| ALTER TABLE `projects` ADD `repo_url` text;--> statement-breakpoint | ||
| ALTER TABLE `projects` ADD `remote_name` text;--> statement-breakpoint | ||
| ALTER TABLE `workspaces` ADD `head_sha` text;--> statement-breakpoint | ||
| ALTER TABLE `workspaces` ADD `pull_request_id` text REFERENCES pull_requests(id);--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Migration FK definition:"
rg -n 'ALTER TABLE `workspaces` ADD `pull_request_id`|ON DELETE' packages/host-service/drizzle/0001_famous_mindworm.sql -C2
echo
echo "Drizzle schema FK definition:"
rg -n 'pullRequestId|onDelete' packages/host-service/src/db/schema.ts -C3
echo
echo "Snapshot FK action:"
rg -n 'workspaces_pull_request_id_pull_requests_id_fk|onDelete' packages/host-service/drizzle/meta/0001_snapshot.json -C2Repository: superset-sh/superset
Length of output: 2234
Add ON DELETE SET NULL to the workspaces.pull_request_id foreign key in the migration.
The migration at line 33 creates the FK without ON DELETE SET NULL, but packages/host-service/src/db/schema.ts declares pullRequestId with onDelete: "set null" (lines 82-84), and the snapshot expects "onDelete": "set null" (line 369). This mismatch will cause the database state to diverge from the declared schema, potentially breaking application behavior.
Suggested fix
-ALTER TABLE `workspaces` ADD `pull_request_id` text REFERENCES pull_requests(id);--> statement-breakpoint
+ALTER TABLE `workspaces` ADD `pull_request_id` text REFERENCES pull_requests(id) ON DELETE SET NULL;--> statement-breakpoint📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ALTER TABLE `workspaces` ADD `pull_request_id` text REFERENCES pull_requests(id);--> statement-breakpoint | |
| ALTER TABLE `workspaces` ADD `pull_request_id` text REFERENCES pull_requests(id) ON DELETE SET NULL;--> statement-breakpoint |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/drizzle/0001_famous_mindworm.sql` at line 33, The ALTER
TABLE migration adds workspaces.pull_request_id foreign key without ON DELETE
SET NULL, which mismatches the declared schema's pullRequestId onDelete: "set
null"; update the migration statement that creates the foreign key on the
workspaces.pull_request_id column to include ON DELETE SET NULL so the database
FK behavior matches the schema (ensure the migration's ALTER TABLE `workspaces`
ADD `pull_request_id` ... REFERENCES pull_requests(id) is changed to include ON
DELETE SET NULL to align with pullRequestId in schema.ts).
Comment on lines
+52
to
+57
| const pullRequestRuntime = new PullRequestRuntimeManager({ | ||
| db, | ||
| deviceClientId: options?.deviceClientId, | ||
| deviceName: options?.deviceName, | ||
| git, | ||
| github, | ||
| }); | ||
| pullRequestRuntime.start(); |
Contributor
There was a problem hiding this comment.
Missing teardown for PullRequestRuntimeManager intervals.
pullRequestRuntime.start() sets up interval timers, but there's no corresponding stop() call wired into any shutdown hook. This could lead to resource leaks if the app is recreated or during graceful shutdown. Consider returning a cleanup function or exposing the runtime for external teardown.
Suggested approach
export interface CreateAppResult {
app: Hono;
injectWebSocket: ReturnType<typeof createNodeWebSocket>["injectWebSocket"];
+ shutdown: () => void;
}
// ... later in createApp:
+ const shutdown = () => {
+ pullRequestRuntime.stop();
+ };
- return { app, injectWebSocket };
+ return { app, injectWebSocket, shutdown };Then call shutdown() in the server close handlers in serve.ts and apps/desktop/src/main/host-service/index.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/app.ts` around lines 52 - 57,
pullRequestRuntime.start() creates long-running intervals but there is no
teardown; add a proper shutdown path by exposing a stop()/shutdown() method on
PullRequestRuntimeManager (if one doesn't exist) that clears timers, then keep a
reference to the created instance (pullRequestRuntime) and call
pullRequestRuntime.stop()/shutdown() from the app's shutdown hooks/server close
handlers so intervals are cleared during graceful shutdown or when the service
is recreated.
| export const PULL_REQUESTS_QUERY = ` | ||
| query PullRequestsForSidebar($owner: String!, $repo: String!) { | ||
| repository(owner: $owner, name: $repo) { | ||
| pullRequests(first: 100, states: [OPEN, CLOSED, MERGED], orderBy: { field: UPDATED_AT, direction: DESC }) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/host-service/src/runtime/pull-requests/utils/github-query/query.tsRepository: superset-sh/superset
Length of output: 1044
🏁 Script executed:
rg -t ts -t tsx "PULL_REQUESTS_QUERY" --no-headingRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg "PULL_REQUESTS_QUERY" -A 5 -B 2Repository: superset-sh/superset
Length of output: 2424
🏁 Script executed:
cat -n packages/host-service/src/runtime/pull-requests/utils/github-query/github-query.tsRepository: superset-sh/superset
Length of output: 870
🏁 Script executed:
cat -n packages/host-service/src/runtime/pull-requests/utils/github-query/types.tsRepository: superset-sh/superset
Length of output: 1296
Add pagination support to prevent silent data truncation in large repositories.
The query uses fixed limits (first: 100 for PRs, first: 50 for status checks) without pagination cursors. Repositories exceeding these limits will silently drop relevant pull requests and status checks, with no indication to the caller that data is incomplete.
🔧 Suggested query expansion for pagination support
export const PULL_REQUESTS_QUERY = `
query PullRequestsForSidebar($owner: String!, $repo: String!) {
repository(owner: $owner, name: $repo) {
pullRequests(first: 100, states: [OPEN, CLOSED, MERGED], orderBy: { field: UPDATED_AT, direction: DESC }) {
nodes {
number
title
url
state
isDraft
headRefName
headRefOid
reviewDecision
updatedAt
statusCheckRollup {
contexts(first: 50) {
nodes {
__typename
... on CheckRun {
name
conclusion
detailsUrl
status
}
... on StatusContext {
context
state
targetUrl
}
}
+ pageInfo {
+ hasNextPage
+ endCursor
+ }
}
}
}
+ pageInfo {
+ hasNextPage
+ endCursor
+ }
}
}
}
`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/runtime/pull-requests/utils/github-query/query.ts`
at line 4, The GraphQL query currently hardcodes pullRequests(first: 100) and
statusChecks(first: 50) which truncates results; update the query and
surrounding logic in query.ts to support cursor-based pagination: change the
pullRequests and statusChecks calls to accept variables (e.g., $prAfter,
$statusAfter) instead of fixed first offsets, use PageInfo (endCursor,
hasNextPage) and edges/nodes to iterate, and modify the function that executes
the query (the one constructing/processing the pullRequests response) to loop
fetching pages (passing after cursors) and accumulate all nodes until
hasNextPage is false; ensure the final return includes the full aggregated lists
for pullRequests and statusChecks and surface any partial-data errors if
requests fail mid-pagination.
| const patterns = [ | ||
| /^git@github\.com:(?<owner>[^/]+)\/(?<name>[^/]+?)(?:\.git)?$/, | ||
| /^ssh:\/\/git@github\.com\/(?<owner>[^/]+)\/(?<name>[^/]+?)(?:\.git)?$/, | ||
| /^https:\/\/github\.com\/(?<owner>[^/]+)\/(?<name>[^/]+?)(?:\.git)?\/?$/, |
Contributor
There was a problem hiding this comment.
Authenticated HTTPS remotes are not parsed.
Line 15 rejects URLs like https://user@github.com/owner/repo.git. In packages/host-service/src/runtime/pull-requests/pull-requests.ts (getProjectRepository()), parse failure returns null, so PR sync is skipped.
🔧 Minimal regex fix
- /^https:\/\/github\.com\/(?<owner>[^/]+)\/(?<name>[^/]+?)(?:\.git)?\/?$/,
+ /^https:\/\/(?:[^@/]+@)?github\.com\/(?<owner>[^/]+)\/(?<name>[^/]+?)(?:\.git)?\/?$/,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/host-service/src/runtime/pull-requests/utils/parse-github-remote/parse-github-remote.ts`
at line 15, The HTTPS remote regex in parse-github-remote.ts currently rejects
authenticated URLs like "https://user@github.com/owner/repo.git"; update the
regex in that file so it accepts an optional "user@" segment immediately after
the "https://" prefix while preserving the existing named capture groups "owner"
and "name" and the optional ".git" suffix (this will allow
getProjectRepository() to parse authenticated HTTPS remotes instead of returning
null). Ensure the pattern only makes the "user@" part optional and non-greedy so
owner/name captures remain correct, and run/update any unit tests covering
parseGithubRemote or the regex array to include an authenticated-HTTPS example.
Comment on lines
+10
to
+15
| type CheckStatus = | ||
| | "success" | ||
| | "failure" | ||
| | "pending" | ||
| | "skipped" | ||
| | "cancelled"; |
Contributor
There was a problem hiding this comment.
Cancelled/skipped checks currently collapse to success.
CheckStatus includes cancelled and skipped, but the aggregate only looks for failure and pending. A PR whose only workflow was cancelled will show a green pass in the sidebar.
Also applies to: 64-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/host-service/src/runtime/pull-requests/utils/pull-request-mappers/pull-request-mappers.ts`
around lines 10 - 15, The aggregate-check logic in pull-request-mappers.ts (the
code that reduces CheckStatus values into an overall status around lines 10 and
~64-68) currently treats only "failure" and "pending" as non-success, causing
"skipped" and "cancelled" to be treated as success; update that aggregation so
that "skipped" and "cancelled" are considered non-success (i.e., overall is not
"success" if any check is "failure", "pending", "skipped", or "cancelled").
Locate the reducer/mapper function that consumes the CheckStatus union (search
for the reduction that checks for "failure" or "pending") and add checks for
"skipped" and "cancelled" (or change logic to require all statuses === "success"
for overall success). Also update any related branches/conditionals and tests
that assume skipped/cancelled are successes.
Comment on lines
+147
to
+161
| let message: TerminalClientMessage; | ||
| try { | ||
| message = JSON.parse(String(event.data)) as TerminalClientMessage; | ||
| } catch { | ||
| sendMessage(ws, { | ||
| type: "error", | ||
| message: "Invalid terminal message payload", | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (message.type === "input") { | ||
| terminal.write(message.data); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Unsafe type assertion on parsed WebSocket message.
The JSON.parse result is cast to TerminalClientMessage without runtime validation. A malformed message (e.g., { type: "input" } without data, or { type: "resize" } without cols/rows) would pass the type check but cause runtime errors when accessing properties.
Suggested validation
let message: TerminalClientMessage;
try {
message = JSON.parse(String(event.data)) as TerminalClientMessage;
} catch {
sendMessage(ws, {
type: "error",
message: "Invalid terminal message payload",
});
return;
}
+ if (message.type === "input" && typeof message.data !== "string") {
+ sendMessage(ws, { type: "error", message: "Invalid input message" });
+ return;
+ }
+ if (
+ message.type === "resize" &&
+ (typeof message.cols !== "number" || typeof message.rows !== "number")
+ ) {
+ sendMessage(ws, { type: "error", message: "Invalid resize message" });
+ return;
+ }
if (message.type === "input") {
terminal.write(message.data);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let message: TerminalClientMessage; | |
| try { | |
| message = JSON.parse(String(event.data)) as TerminalClientMessage; | |
| } catch { | |
| sendMessage(ws, { | |
| type: "error", | |
| message: "Invalid terminal message payload", | |
| }); | |
| return; | |
| } | |
| if (message.type === "input") { | |
| terminal.write(message.data); | |
| return; | |
| } | |
| let message: TerminalClientMessage; | |
| try { | |
| message = JSON.parse(String(event.data)) as TerminalClientMessage; | |
| } catch { | |
| sendMessage(ws, { | |
| type: "error", | |
| message: "Invalid terminal message payload", | |
| }); | |
| return; | |
| } | |
| if (message.type === "input" && typeof message.data !== "string") { | |
| sendMessage(ws, { type: "error", message: "Invalid input message" }); | |
| return; | |
| } | |
| if ( | |
| message.type === "resize" && | |
| (typeof message.cols !== "number" || typeof message.rows !== "number") | |
| ) { | |
| sendMessage(ws, { type: "error", message: "Invalid resize message" }); | |
| return; | |
| } | |
| if (message.type === "input") { | |
| terminal.write(message.data); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/terminal/terminal.ts` around lines 147 - 161, The
code unsafely casts JSON.parse(String(event.data)) to TerminalClientMessage;
instead add runtime validation after parsing (do not use a blind type assertion)
to verify the object has a valid "type" and required fields before using them:
when message.type === "input" ensure message.data is a string before calling
terminal.write(message.data); when message.type === "resize" ensure message.cols
and message.rows are numbers before using them; for any invalid shape
sendMessage(ws, { type: "error", message: "Invalid terminal message payload" })
and return. Use the existing symbols TerminalClientMessage, terminal.write,
sendMessage and event.data to locate where to insert these checks.
Comment on lines
+4
to
+16
| export const pullRequestsRouter = router({ | ||
| getByWorkspaces: publicProcedure | ||
| .input( | ||
| z.object({ | ||
| workspaceIds: z.array(z.string()), | ||
| }), | ||
| ) | ||
| .query(async ({ ctx, input }) => { | ||
| const workspaces = await ctx.runtime.pullRequests.getPullRequestsByWorkspaces( | ||
| input.workspaceIds, | ||
| ); | ||
| return { workspaces }; | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packages/host-service/src/trpc/index.ts =="
sed -n '1,120p' packages/host-service/src/trpc/index.ts
echo
echo "== packages/host-service/src/app.ts =="
sed -n '1,220p' packages/host-service/src/app.ts
echo
echo "== packages/host-service/src/serve.ts =="
sed -n '1,220p' packages/host-service/src/serve.ts
echo
echo "== Host-service transport guards / procedure usage =="
rg -n -C2 'publicProcedure|middleware|cors|origin|localhost|127\.0\.0\.1|0\.0\.0\.0|listen\(' packages/host-service/srcRepository: superset-sh/superset
Length of output: 14359
Secure this route and all publicProcedure endpoints — they expose sensitive data without authentication.
The getByWorkspaces procedure uses publicProcedure, which is unauthenticated raw TRPC. While serve.ts defaults to localhost, the PORT environment variable allows binding to any interface (including 0.0.0.0). Additionally, app.ts line 64 enables CORS with cors() without origin restrictions, allowing any website to call these endpoints.
This exposes workspace enumeration and PR metadata (titles, URLs, sync errors) to any reachable client. The same issue affects all other routers (workspace, github, project, git, cloud, health) — all use publicProcedure without authentication.
Solutions:
- Add authentication middleware to TRPC procedures, or
- Lock the transport to localhost with explicit origin checks before CORS is enabled, or
- Move sensitive procedures behind authenticated TRPC middleware.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/src/trpc/router/pull-requests/pull-requests.ts` around
lines 4 - 16, The getByWorkspaces endpoint (inside pullRequestsRouter) is
defined with publicProcedure and calls
ctx.runtime.pullRequests.getPullRequestsByWorkspaces, exposing sensitive
PR/workspace data; change this and all other publicProcedure usages in routers
(e.g., workspace, github, project, git, cloud, health) to use your authenticated
TRPC middleware/protectedProcedure (or add an auth middleware to router) so
requests are validated before calling runtime methods, or alternatively enforce
strict origin/transport restrictions before enabling CORS; ensure the symbol
names to update include pullRequestsRouter, getByWorkspaces, publicProcedure,
router, and any other router exports that currently use publicProcedure.
Contributor
There was a problem hiding this comment.
5 issues found across 35 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="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts:142">
P2: Handle failures in this async hover-refresh callback so rejected host-service calls do not become unhandled promise rejections.
(Based on your team's feedback about handling async calls with proper await and catch.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts:228">
P2: `branchExistsOnRemote` is being set from repository presence instead of an actual remote branch check, which can render broken GitHub branch links for unpushed branches.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx:88">
P2: Handle the async `onWorkspaceHover` call with `await`/`try`-`catch` so hover-triggered refresh failures don’t surface as unhandled promise rejections.
(Based on your team's feedback about handling async calls with proper await and catch.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/components/ChecksList/ChecksList.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/components/ChecksList/ChecksList.tsx:11">
P3: This ChecksList is a near‑copy of the existing WorkspaceSidebar hover card checks list. Consider extracting a shared component/helper so check filtering/toggle behavior stays in sync between v1 and v2.
(Based on your team's feedback about avoiding duplicated business logic in multiple components.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContent/DashboardSidebarSectionContent.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContent/DashboardSidebarSectionContent.tsx:31">
P2: Handle promise rejections from `onWorkspaceHover`. The callback can be async, but the handler ignores its promise, so a rejection becomes an unhandled promise rejection.
(Based on your team's feedback about handling async calls with proper await and catch.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| project.githubOwner && project.githubRepoName | ||
| ? `https://github.com/${project.githubOwner}/${project.githubRepoName}` | ||
| : null, | ||
| branchExistsOnRemote: |
Contributor
There was a problem hiding this comment.
P2: branchExistsOnRemote is being set from repository presence instead of an actual remote branch check, which can render broken GitHub branch links for unpushed branches.
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/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts, line 228:
<comment>`branchExistsOnRemote` is being set from repository presence instead of an actual remote branch check, which can render broken GitHub branch links for unpushed branches.</comment>
<file context>
@@ -206,6 +221,15 @@ export function useDashboardSidebarData() {
+ project.githubOwner && project.githubRepoName
+ ? `https://github.com/${project.githubOwner}/${project.githubRepoName}`
+ : null,
+ branchExistsOnRemote:
+ project.githubOwner !== null && project.githubRepoName !== null,
+ previewUrl: null,
</file context>
Comment on lines
+142
to
+145
| await activeHostService.client.pullRequests.refreshByWorkspaces.mutate({ | ||
| workspaceIds: [workspaceId], | ||
| }); | ||
| await refetchPullRequests(); |
Contributor
There was a problem hiding this comment.
P2: Handle failures in this async hover-refresh callback so rejected host-service calls do not become unhandled promise rejections.
(Based on your team's feedback about handling async calls with proper await and 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/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts, line 142:
<comment>Handle failures in this async hover-refresh callback so rejected host-service calls do not become unhandled promise rejections.
(Based on your team's feedback about handling async calls with proper await and catch.) </comment>
<file context>
@@ -117,21 +118,35 @@ export function useDashboardSidebarData() {
+ return;
+ }
+
+ await activeHostService.client.pullRequests.refreshByWorkspaces.mutate({
+ workspaceIds: [workspaceId],
+ });
</file context>
Suggested change
| await activeHostService.client.pullRequests.refreshByWorkspaces.mutate({ | |
| workspaceIds: [workspaceId], | |
| }); | |
| await refetchPullRequests(); | |
| try { | |
| await activeHostService.client.pullRequests.refreshByWorkspaces.mutate({ | |
| workspaceIds: [workspaceId], | |
| }); | |
| await refetchPullRequests(); | |
| } catch (error) { | |
| console.warn( | |
| "[DashboardSidebar] failed to refresh workspace pull request", | |
| { workspaceId, error }, | |
| ); | |
| } |
| name={workspace.name} | ||
| branch={workspace.branch} | ||
| workspace={workspace} | ||
| onHoverCardOpen={() => onWorkspaceHover(workspace.id)} |
Contributor
There was a problem hiding this comment.
P2: Handle the async onWorkspaceHover call with await/try-catch so hover-triggered refresh failures don’t surface as unhandled promise rejections.
(Based on your team's feedback about handling async calls with proper await and 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/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarCollapsedProjectContent/DashboardSidebarCollapsedProjectContent.tsx, line 88:
<comment>Handle the async `onWorkspaceHover` call with `await`/`try`-`catch` so hover-triggered refresh failures don’t surface as unhandled promise rejections.
(Based on your team's feedback about handling async calls with proper await and catch.) </comment>
<file context>
@@ -83,14 +83,9 @@ export const DashboardSidebarCollapsedProjectContent = forwardRef<
- branch={workspace.branch}
- pullRequest={workspace.pullRequest}
+ workspace={workspace}
+ onHoverCardOpen={() => onWorkspaceHover(workspace.id)}
shortcutLabel={workspaceShortcutLabels.get(workspace.id)}
isCollapsed
</file context>
Suggested change
| onHoverCardOpen={() => onWorkspaceHover(workspace.id)} | |
| onHoverCardOpen={async () => { | |
| try { | |
| await onWorkspaceHover(workspace.id); | |
| } catch (error) { | |
| console.warn( | |
| "[DashboardSidebar] failed to refresh workspace PR data", | |
| error, | |
| ); | |
| } | |
| }} |
| name={workspace.name} | ||
| branch={workspace.branch} | ||
| workspace={workspace} | ||
| onHoverCardOpen={() => onWorkspaceHover(workspace.id)} |
Contributor
There was a problem hiding this comment.
P2: Handle promise rejections from onWorkspaceHover. The callback can be async, but the handler ignores its promise, so a rejection becomes an unhandled promise rejection.
(Based on your team's feedback about handling async calls with proper await and 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/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionContent/DashboardSidebarSectionContent.tsx, line 31:
<comment>Handle promise rejections from `onWorkspaceHover`. The callback can be async, but the handler ignores its promise, so a rejection becomes an unhandled promise rejection.
(Based on your team's feedback about handling async calls with proper await and catch.) </comment>
<file context>
@@ -26,14 +26,9 @@ export function DashboardSidebarSectionContent({
- branch={workspace.branch}
- pullRequest={workspace.pullRequest}
+ workspace={workspace}
+ onHoverCardOpen={() => onWorkspaceHover(workspace.id)}
shortcutLabel={workspaceShortcutLabels.get(workspace.id)}
/>
</file context>
| checks: DashboardSidebarWorkspacePullRequestCheck[]; | ||
| } | ||
|
|
||
| export function ChecksList({ checks }: ChecksListProps) { |
Contributor
There was a problem hiding this comment.
P3: This ChecksList is a near‑copy of the existing WorkspaceSidebar hover card checks list. Consider extracting a shared component/helper so check filtering/toggle behavior stays in sync between v1 and v2.
(Based on your team's feedback about avoiding duplicated business logic in multiple components.)
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/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceHoverCardContent/components/ChecksList/ChecksList.tsx, line 11:
<comment>This ChecksList is a near‑copy of the existing WorkspaceSidebar hover card checks list. Consider extracting a shared component/helper so check filtering/toggle behavior stays in sync between v1 and v2.
(Based on your team's feedback about avoiding duplicated business logic in multiple components.) </comment>
<file context>
@@ -0,0 +1,44 @@
+ checks: DashboardSidebarWorkspacePullRequestCheck[];
+}
+
+export function ChecksList({ checks }: ChecksListProps) {
+ const [expanded, setExpanded] = useState(false);
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by cubic
Adds real GitHub PR status, checks, and review state to the v2 workspace sidebar for local workspaces, powered by a host-service that syncs branches and maps workspaces to PRs. Also adds an in‑app terminal for v2 workspaces over WebSocket.
New Features
pullRequests.getByWorkspacesandpullRequests.refreshByWorkspaces.node-pty) with reconnect; server route mounted via@hono/node-ws.Dependencies
@hono/node-wsandnode-pty.GITHUB_TOKEN/GH_TOKENor git credential manager).Written for commit ff85df0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes