Sidecar test coverage and execution workflow fixes#35
Merged
Conversation
Root cause: the virtualized activity log list used an unstable React key that included logs.length. Every new tool call log appended increased logs.length by 1, which changed the key of EVERY visible activity item, forcing React to unmount and remount all DOM nodes on every single log entry. During execution this happens 10+ times per second, freezing the entire app on WebKitGTK. Fixes: - Key: use virtualRow.key (stable per index) instead of logs.length - Remove measureElement ref overhead (fixed estimateSize is sufficient) - Tune virtualizer: estimateSize 60, overscan 3 (was 88/8) - Remove scrollTop=0 useEffect that fought the virtualizer - Add will-change:transform hint for compositor efficiency - execution-drawer: change smooth scroll to auto (eliminates continuous animation thrashing while logs stream in)
- Deleted entire apps/desktop-electron/ directory - Removed all Electron build scripts from root and desktop-ui package.json - Simplified all desktop runtime, IPC, auth, store, window controls, folder picker, and platform detection to Tauri-only - Updated next.config.js, layout.tsx script, utils.ts, sidebar, onboarding components, landing page, and pnpm-lock.yaml - Pre-existing unrelated changes (model onboarding, board components, sidecar) left unstaged per protocol
Owner
Author
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+167
to
+193
| export function useExecutionLogsWithHistory(taskId: string | null | undefined) { | ||
| const liveLogs = useExecutionLogs(taskId) | ||
| const [historicalLogs, setHistoricalLogs] = useState<ExecutionLogEntry[]>(EMPTY_LOGS) | ||
| const [loading, setLoading] = useState(false) | ||
|
|
||
| useEffect(() => { | ||
| let cancelled = false | ||
| if (!taskId || hasExecutionLogs(taskId)) { | ||
| setHistoricalLogs(EMPTY_LOGS) | ||
| return | ||
| } | ||
|
|
||
| setLoading(true) | ||
| fetchExecutionLogs(taskId) | ||
| .then((res) => { | ||
| if (!cancelled && res.logs.length > 0) { | ||
| replaceExecutionLogs(taskId, res.logs) | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| // Silently ignore — the task may have never been executed | ||
| }) | ||
| .finally(() => { | ||
| if (!cancelled) setLoading(false) | ||
| }) | ||
| return () => { | ||
| cancelled = true |
There was a problem hiding this comment.
historicalLogs state is never populated — dead state
You created a useState that is initialized to EMPTY_LOGS, only ever reset back to EMPTY_LOGS, and somehow convinced yourself this was necessary. The useless ceremony is breathtaking.
The setHistoricalLogs call in the early-return branch writes EMPTY_LOGS to a variable that is already EMPTY_LOGS. In the fetch path, fetched data is written directly to the module-level store via replaceExecutionLogs, which triggers useSyncExternalStore to re-render with the new data through liveLogs. The historicalLogs state variable is therefore unreachable with any non-empty value — remove it entirely and simplify logs to just liveLogs.
Fewer moving parts means fewer things to break — try it sometime.
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!
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (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-ui/lib/execution-state-store.ts">
<violation number="1" location="apps/desktop-ui/lib/execution-state-store.ts:173">
P1: The cancellation change can leave `loading` stuck `true` when switching to a task that skips history fetch. Clear loading in the early-return branch as well.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Comment on lines
+173
to
+176
| let cancelled = false | ||
| if (!taskId || hasExecutionLogs(taskId)) { | ||
| setHistoricalLogs(EMPTY_LOGS) | ||
| return |
There was a problem hiding this comment.
P1: The cancellation change can leave loading stuck true when switching to a task that skips history fetch. Clear loading in the early-return branch as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/lib/execution-state-store.ts, line 173:
<comment>The cancellation change can leave `loading` stuck `true` when switching to a task that skips history fetch. Clear loading in the early-return branch as well.</comment>
<file context>
@@ -170,6 +170,7 @@ export function useExecutionLogsWithHistory(taskId: string | null | undefined) {
const [loading, setLoading] = useState(false)
useEffect(() => {
+ let cancelled = false
if (!taskId || hasExecutionLogs(taskId)) {
setHistoricalLogs(EMPTY_LOGS)
</file context>
There was a problem hiding this comment.
12 issues found across 27 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop-ui/components/board/in-progress-batch-group.tsx">
<violation number="1" location="apps/desktop-ui/components/board/in-progress-batch-group.tsx:61">
P2: Avoid hard-coded hex colors here; use semantic `linear-*` tokens so the batch UI stays consistent with app theming and future palette changes.</violation>
</file>
<file name="apps/desktop-ui/components/board/unified-selection-bar.tsx">
<violation number="1" location="apps/desktop-ui/components/board/unified-selection-bar.tsx:113">
P2: Hardcoded hex colors replace design-system tokens on the three execution buttons, breaking theme consistency with the other buttons in the same component. The `border-linear-border`, `hover:bg-linear-bg-tertiary`, `bg-linear-accent`, and `hover:bg-linear-accent-hover` tokens were replaced with hardcoded `bg-[#064e3b]`, `border-[#2a2a2a]`, `bg-[#1a1a1a]`, etc. If the design system supports light mode or is ever rethemed, these buttons will not adapt — whereas the Move, Status, Archive, and Clear buttons (all in the same toolbar) will still pick up token changes. This also creates an inconsistency within the same toolbar where different buttons use different styling approaches.
Consider keeping the design tokens (`bg-linear-accent`, `border-linear-border`, `hover:bg-linear-bg-tertiary`) for theme portability. If the tokens did not produce the intended appearance, update the token definitions instead of hardcoding per-instance overrides.</violation>
</file>
<file name="apps/desktop-ui/components/onboarding/steps/model-provider-step.tsx">
<violation number="1" location="apps/desktop-ui/components/onboarding/steps/model-provider-step.tsx:112">
P2: `onClick` fallback/toast branches are unreachable because the button is disabled when `!isReady`. This leaves dead logic and prevents the intended guidance from ever being shown.</violation>
</file>
<file name="apps/sidecar/src/routes/batches.ts">
<violation number="1" location="apps/sidecar/src/routes/batches.ts:46">
P3: The new model pre-flight guard duplicates existing route logic; extract a shared helper to avoid behavior drift between execute and batch endpoints.</violation>
</file>
<file name="apps/desktop-ui/lib/execution-state-store.ts">
<violation number="1" location="apps/desktop-ui/lib/execution-state-store.ts:169">
P3: `historicalLogs` is dead state: it is never set from fetched results, so the fallback path cannot return historical data.</violation>
<violation number="2" location="apps/desktop-ui/lib/execution-state-store.ts:188">
P2: `loading` is cleared unconditionally in `finally`, which can hide the spinner while a newer `taskId` fetch is still in flight after rapid task switches.</violation>
</file>
<file name="apps/desktop-ui/components/board/batch-progress.tsx">
<violation number="1" location="apps/desktop-ui/components/board/batch-progress.tsx:64">
P2: The new “View” action is hidden behind a hover-only style, which makes it inaccessible on touch and hard to use with keyboard navigation.</violation>
</file>
<file name="apps/sidecar/src/routes/opencode.ts">
<violation number="1" location="apps/sidecar/src/routes/opencode.ts:215">
P2: `providers` is computed before model auto-detection, so `providers[].selectedModel`/sorting can disagree with the returned `currentModel` after `ensureModelConfigured` resolves a model.</violation>
</file>
<file name="apps/sidecar/src/services/execution/state.ts">
<violation number="1" location="apps/sidecar/src/services/execution/state.ts:181">
P1: `sessionToTask.clear()` now clears all active executions by calling `executionStore.reset()`, which can accidentally erase live execution state.</violation>
</file>
<file name="apps/desktop-ui/components/onboarding/steps/onboarding-utils.ts">
<violation number="1" location="apps/desktop-ui/components/onboarding/steps/onboarding-utils.ts:6">
P3: The restoration logic hardcodes `3` as the max step index instead of deriving from `STEP_LABELS.length - 1`. Adding the 4th step makes this a maintenance trap: the value happens to be correct now (`4 - 1 === 3`), but adding a 5th step would silently clamp restored drafts to step 3 instead of step 4.</violation>
</file>
<file name="apps/sidecar/src/routes/execution.ts">
<violation number="1" location="apps/sidecar/src/routes/execution.ts:48">
P2: The new model pre-flight check fails open: when `ensureModelConfigured` throws, execution still proceeds instead of returning an error.</violation>
</file>
<file name="apps/desktop-ui/components/onboarding/onboarding-wizard.tsx">
<violation number="1" location="apps/desktop-ui/components/onboarding/onboarding-wizard.tsx:192">
P2: Onboarding completion now depends on `createdTeamId`, but that ID is not persisted/restored with the draft, so users can get stuck on step 3 after refresh.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| case 'forEach': return (fn: (value: ExecutionState, key: string, map: Map<string, ExecutionState>) => void) => { | ||
| for (const [k, v] of executionStore.entries()) fn(v, k, activeExecutions); | ||
| }; | ||
| case 'clear': return () => executionStore.reset(); |
There was a problem hiding this comment.
P1: sessionToTask.clear() now clears all active executions by calling executionStore.reset(), which can accidentally erase live execution state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sidecar/src/services/execution/state.ts, line 181:
<comment>`sessionToTask.clear()` now clears all active executions by calling `executionStore.reset()`, which can accidentally erase live execution state.</comment>
<file context>
@@ -170,6 +178,7 @@ export const activeExecutions: Map<string, ExecutionState> = new Proxy(new Map<s
case 'forEach': return (fn: (value: ExecutionState, key: string, map: Map<string, ExecutionState>) => void) => {
for (const [k, v] of executionStore.entries()) fn(v, k, activeExecutions);
};
+ case 'clear': return () => executionStore.reset();
case Symbol.iterator: return () => executionStore.entries();
case Symbol.toStringTag: return 'Map';
</file context>
| {...provided.draggableProps} | ||
| className={`border border-dashed border-linear-border rounded-sm p-2 mb-3 bg-linear-bg-secondary/50 ${snapshot.isDragging ? 'shadow-2xl shadow-black/50 ring-1 ring-linear-border scale-[1.02] rotate-1' : ''}`} | ||
| className={cn( | ||
| "border border-[#252525] rounded-sm p-3 mb-3 bg-[#141414]", |
There was a problem hiding this comment.
P2: Avoid hard-coded hex colors here; use semantic linear-* tokens so the batch UI stays consistent with app theming and future palette changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/components/board/in-progress-batch-group.tsx, line 61:
<comment>Avoid hard-coded hex colors here; use semantic `linear-*` tokens so the batch UI stays consistent with app theming and future palette changes.</comment>
<file context>
@@ -57,21 +57,27 @@ export function InProgressBatchGroup({
{...provided.draggableProps}
- className={`border border-dashed border-linear-border rounded-sm p-2 mb-3 bg-linear-bg-secondary/50 ${snapshot.isDragging ? 'shadow-2xl shadow-black/50 ring-1 ring-linear-border scale-[1.02] rotate-1' : ''}`}
+ className={cn(
+ "border border-[#252525] rounded-sm p-3 mb-3 bg-[#141414]",
+ snapshot.isDragging && "shadow-2xl shadow-black/50 ring-1 ring-[#333] scale-[1.02] rotate-1"
+ )}
</file context>
Suggested change
| "border border-[#252525] rounded-sm p-3 mb-3 bg-[#141414]", | |
| "border border-linear-border rounded-sm p-3 mb-3 bg-linear-bg-secondary/50", |
| disabled={executeDisabled} | ||
| title={executeTitle} | ||
| className="bg-linear-accent hover:bg-linear-accent-hover text-white gap-1.5 disabled:opacity-50" | ||
| className="bg-[#064e3b] hover:bg-[#065f46] border border-[#065f46]/60 text-emerald-100 gap-1.5 disabled:opacity-50 shadow-sm" |
There was a problem hiding this comment.
P2: Hardcoded hex colors replace design-system tokens on the three execution buttons, breaking theme consistency with the other buttons in the same component. The border-linear-border, hover:bg-linear-bg-tertiary, bg-linear-accent, and hover:bg-linear-accent-hover tokens were replaced with hardcoded bg-[#064e3b], border-[#2a2a2a], bg-[#1a1a1a], etc. If the design system supports light mode or is ever rethemed, these buttons will not adapt — whereas the Move, Status, Archive, and Clear buttons (all in the same toolbar) will still pick up token changes. This also creates an inconsistency within the same toolbar where different buttons use different styling approaches.
Consider keeping the design tokens (bg-linear-accent, border-linear-border, hover:bg-linear-bg-tertiary) for theme portability. If the tokens did not produce the intended appearance, update the token definitions instead of hardcoding per-instance overrides.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/components/board/unified-selection-bar.tsx, line 113:
<comment>Hardcoded hex colors replace design-system tokens on the three execution buttons, breaking theme consistency with the other buttons in the same component. The `border-linear-border`, `hover:bg-linear-bg-tertiary`, `bg-linear-accent`, and `hover:bg-linear-accent-hover` tokens were replaced with hardcoded `bg-[#064e3b]`, `border-[#2a2a2a]`, `bg-[#1a1a1a]`, etc. If the design system supports light mode or is ever rethemed, these buttons will not adapt — whereas the Move, Status, Archive, and Clear buttons (all in the same toolbar) will still pick up token changes. This also creates an inconsistency within the same toolbar where different buttons use different styling approaches.
Consider keeping the design tokens (`bg-linear-accent`, `border-linear-border`, `hover:bg-linear-bg-tertiary`) for theme portability. If the tokens did not produce the intended appearance, update the token definitions instead of hardcoding per-instance overrides.</comment>
<file context>
@@ -110,7 +110,7 @@ export function UnifiedSelectionBar({
disabled={executeDisabled}
title={executeTitle}
- className="bg-linear-accent hover:bg-linear-accent-hover text-white gap-1.5 disabled:opacity-50"
+ className="bg-[#064e3b] hover:bg-[#065f46] border border-[#065f46]/60 text-emerald-100 gap-1.5 disabled:opacity-50 shadow-sm"
>
<Play className="w-3.5 h-3.5" />
</file context>
| toast.error("Connect a provider in Settings first, or Skip to finish without one.") | ||
| } | ||
| }} | ||
| disabled={isFinishing || !isReady} |
There was a problem hiding this comment.
P2: onClick fallback/toast branches are unreachable because the button is disabled when !isReady. This leaves dead logic and prevents the intended guidance from ever being shown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/components/onboarding/steps/model-provider-step.tsx, line 112:
<comment>`onClick` fallback/toast branches are unreachable because the button is disabled when `!isReady`. This leaves dead logic and prevents the intended guidance from ever being shown.</comment>
<file context>
@@ -0,0 +1,270 @@
+ toast.error("Connect a provider in Settings first, or Skip to finish without one.")
+ }
+ }}
+ disabled={isFinishing || !isReady}
+ className="flex-1 bg-linear-accent hover:bg-linear-accent-hover disabled:opacity-50 disabled:cursor-not-allowed text-white rounded-sm h-9 px-6 text-sm font-medium transition-colors inline-flex items-center justify-center gap-2"
+ >
</file context>
Suggested change
| disabled={isFinishing || !isReady} | |
| disabled={isFinishing} |
| .catch(() => { | ||
| // Silently ignore — the task may have never been executed | ||
| }) | ||
| .finally(() => setLoading(false)) |
There was a problem hiding this comment.
P2: loading is cleared unconditionally in finally, which can hide the spinner while a newer taskId fetch is still in flight after rapid task switches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/lib/execution-state-store.ts, line 188:
<comment>`loading` is cleared unconditionally in `finally`, which can hide the spinner while a newer `taskId` fetch is still in flight after rapid task switches.</comment>
<file context>
@@ -162,3 +163,31 @@ export function useExecutionProgress(taskId: string | null | undefined) {
+ .catch(() => {
+ // Silently ignore — the task may have never been executed
+ })
+ .finally(() => setLoading(false))
+ }, [taskId])
+
</file context>
Comment on lines
+48
to
+51
| const ensured = await ensureModelConfigured(req.userId).catch((err: unknown) => { | ||
| console.warn('[Tasks] Model pre-flight check failed:', err); | ||
| return null; | ||
| }); |
There was a problem hiding this comment.
P2: The new model pre-flight check fails open: when ensureModelConfigured throws, execution still proceeds instead of returning an error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sidecar/src/routes/execution.ts, line 48:
<comment>The new model pre-flight check fails open: when `ensureModelConfigured` throws, execution still proceeds instead of returning an error.</comment>
<file context>
@@ -42,6 +43,24 @@ router.post('/:id/execute', optionalAuth, async (req: AuthRequest, res: Response
console.log(`[Tasks] Execute requested for task ${id.slice(0, 8)} (userId: ${req.userId || 'anonymous'})`);
+
+ if (req.userId) {
+ const ensured = await ensureModelConfigured(req.userId).catch((err: unknown) => {
+ console.warn('[Tasks] Model pre-flight check failed:', err);
+ return null;
</file context>
Suggested change
| const ensured = await ensureModelConfigured(req.userId).catch((err: unknown) => { | |
| console.warn('[Tasks] Model pre-flight check failed:', err); | |
| return null; | |
| }); | |
| let ensured; | |
| try { | |
| ensured = await ensureModelConfigured(req.userId); | |
| } catch (err: unknown) { | |
| console.warn('[Tasks] Model pre-flight check failed:', err); | |
| res.status(503).json({ | |
| error: 'Unable to verify model configuration. Please try again.', | |
| code: 'MODEL_CONFIG_CHECK_FAILED', | |
| }); | |
| return; | |
| } |
|
|
||
| const completeOnboarding = useCallback(() => { | ||
| const workspaceIdForCompletion = createdWorkspaceId ?? initialWorkspaceId | ||
| if (!createdTeamId || !workspaceIdForCompletion || !createdProjectId) return |
There was a problem hiding this comment.
P2: Onboarding completion now depends on createdTeamId, but that ID is not persisted/restored with the draft, so users can get stuck on step 3 after refresh.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/components/onboarding/onboarding-wizard.tsx, line 192:
<comment>Onboarding completion now depends on `createdTeamId`, but that ID is not persisted/restored with the draft, so users can get stuck on step 3 after refresh.</comment>
<file context>
@@ -176,18 +178,34 @@ export function OnboardingWizard({
+
+ const completeOnboarding = useCallback(() => {
+ const workspaceIdForCompletion = createdWorkspaceId ?? initialWorkspaceId
+ if (!createdTeamId || !workspaceIdForCompletion || !createdProjectId) return
+ clearStoredDraft()
+ onComplete({
</file context>
| await assertTaskOwned(tid, userId); | ||
| } | ||
|
|
||
| const ensured = await ensureModelConfigured(userId).catch((err: unknown) => { |
There was a problem hiding this comment.
P3: The new model pre-flight guard duplicates existing route logic; extract a shared helper to avoid behavior drift between execute and batch endpoints.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sidecar/src/routes/batches.ts, line 46:
<comment>The new model pre-flight guard duplicates existing route logic; extract a shared helper to avoid behavior drift between execute and batch endpoints.</comment>
<file context>
@@ -41,6 +42,21 @@ router.post('/', optionalAuth, async (req: AuthRequest, res: Response, next: Nex
await assertTaskOwned(tid, userId);
}
+
+ const ensured = await ensureModelConfigured(userId).catch((err: unknown) => {
+ console.warn('[Batch] Model pre-flight check failed:', err);
+ return null;
</file context>
|
|
||
| export function useExecutionLogsWithHistory(taskId: string | null | undefined) { | ||
| const liveLogs = useExecutionLogs(taskId) | ||
| const [historicalLogs, setHistoricalLogs] = useState<ExecutionLogEntry[]>(EMPTY_LOGS) |
There was a problem hiding this comment.
P3: historicalLogs is dead state: it is never set from fetched results, so the fallback path cannot return historical data.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/lib/execution-state-store.ts, line 169:
<comment>`historicalLogs` is dead state: it is never set from fetched results, so the fallback path cannot return historical data.</comment>
<file context>
@@ -162,3 +163,31 @@ export function useExecutionProgress(taskId: string | null | undefined) {
+
+export function useExecutionLogsWithHistory(taskId: string | null | undefined) {
+ const liveLogs = useExecutionLogs(taskId)
+ const [historicalLogs, setHistoricalLogs] = useState<ExecutionLogEntry[]>(EMPTY_LOGS)
+ const [loading, setLoading] = useState(false)
+
</file context>
| export const STORAGE_KEY = "openlinear:onboarding:v4" | ||
| export const REPO_SEARCH_LIMIT = 8 | ||
| export const STEP_LABELS = ["Workspace", "Project", "Team"] as const | ||
| export const STEP_LABELS = ["Workspace", "Project", "Team", "AI Model"] as const |
There was a problem hiding this comment.
P3: The restoration logic hardcodes 3 as the max step index instead of deriving from STEP_LABELS.length - 1. Adding the 4th step makes this a maintenance trap: the value happens to be correct now (4 - 1 === 3), but adding a 5th step would silently clamp restored drafts to step 3 instead of step 4.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop-ui/components/onboarding/steps/onboarding-utils.ts, line 6:
<comment>The restoration logic hardcodes `3` as the max step index instead of deriving from `STEP_LABELS.length - 1`. Adding the 4th step makes this a maintenance trap: the value happens to be correct now (`4 - 1 === 3`), but adding a 5th step would silently clamp restored drafts to step 3 instead of step 4.</comment>
<file context>
@@ -3,7 +3,7 @@ import type { GitHubRepo } from "@/lib/api"
export const STORAGE_KEY = "openlinear:onboarding:v4"
export const REPO_SEARCH_LIMIT = 8
-export const STEP_LABELS = ["Workspace", "Project", "Team"] as const
+export const STEP_LABELS = ["Workspace", "Project", "Team", "AI Model"] as const
export type ProjectTab = "github" | "link" | "ssh"
</file context>
Adds comprehensive tests for the sidecar execution pipeline, model configuration, and event handling. Sidecar: - Creates `opencode-model.ts` with `pickAutoDetectedModel` and `ensureModelConfigured` to enforce a model is selected before executing tasks. - Adds `opencode-model.test.mjs` covering provider listing, auto-detection, and pre-flight guards. - Extends `execution.test.mjs` with `MODEL_NOT_CONFIGURED` error cases when no provider or model is selected. - Extends `state.test.mjs` with `ExecutionStateStore` and proxy trap coverage, including `reset`, `setSessionMapping`, `getTaskIdBySession` fallback scan, and `sessionEntries`. - Extends `opencode.test.mjs` with `OpenCodeAdapter` delegation tests. - Fixes `activeExecutions` and `sessionToTask` proxies to add missing `clear`, `set`, and `Symbol.iterator` traps. - Updates `events.test.mjs` expectation for session mappings that outlive active execution state. - Adds `MODEL_NOT_CONFIGURED` pre-flight guard to `POST /:id/execute` and `POST /api/batches` routes. Desktop UI: - Adds `fetchExecutionLogs` API helper and `useExecutionLogsWithHistory` hook to fetch historical execution logs when a task has no live state. - Updates the Activity tab in `TaskDetailView` to show a loading spinner while fetching historical logs. - Adds model provider onboarding step (`model-provider-step.tsx`) and model auto-detection in the onboarding wizard. - Updates `model-selector.tsx`, `batch-progress.tsx`, and `in-progress-batch-group.tsx` to support model configuration and batch execution improvements. Resolves: [2026-05-31] — Fix execution workflow and View Activity tab Co-authored-by: Cursor <cursoragent@cursor.com>
kaizen403
force-pushed
the
feat/sidecar-test-coverage
branch
from
May 31, 2026 02:57
4706a13 to
c3b08a5
Compare
Add vitest + coverage-v8 to desktop-ui with 20 test files and 227 tests. Cover pure logic (board-state, onboarding-utils, inbox-utils), API client layer (auth, fetch, tasks, chat), critical hooks (use-auth, use-workspace, use-chat-stream, use-task-selection), and SSE subscriptions. Also add pnpm debug command and Node inspector for api + sidecar. Resolves: [2026-05-31] — Desktop UI test coverage plan Co-authored-by: Cursor <cursoragent@cursor.com>
Resolves security findings from Greptile review c29aea46:
P0: SSO state now HMAC-signed (prevents CSRF), OIDC expectedState passed
instead of undefined cast
P1: SSO login creates Session records (tokens now revocable), revoke-others
derives session ID from JWT hash (not request body), 2FA tempToken
delivered via URL fragment (not query string), /uploads requires auth
P2: TOTP validate endpoint rate-limited (10 req/min/IP)
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…, ISSUES.md Keep HMAC-signed SSO state (HEAD) over insecure dev version. Accept dev versions for unrelated model-selector/execution changes. Merge both sides of ISSUES.md work log entries.
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.
Adds comprehensive tests for the sidecar execution pipeline, model configuration, and event handling, and fixes the execution workflow and View Activity tab.
Sidecar:
opencode-model.tswithpickAutoDetectedModelandensureModelConfiguredto enforce a model is selected before executing tasks.opencode-model.test.mjscovering provider listing, auto-detection, and pre-flight guards.execution.test.mjswithMODEL_NOT_CONFIGUREDerror cases when no provider or model is selected.state.test.mjswithExecutionStateStoreand proxy trap coverage, includingreset,setSessionMapping,getTaskIdBySessionfallback scan, andsessionEntries.opencode.test.mjswithOpenCodeAdapterdelegation tests.activeExecutionsandsessionToTaskproxies to add missingclear,set, andSymbol.iteratortraps.events.test.mjsexpectation for session mappings that outlive active execution state.MODEL_NOT_CONFIGUREDpre-flight guard toPOST /:id/executeandPOST /api/batchesroutes.Desktop UI:
fetchExecutionLogsAPI helper anduseExecutionLogsWithHistoryhook to fetch historical execution logs when a task has no live state.TaskDetailViewto show a loading spinner while fetching historical logs.model-provider-step.tsx) and model auto-detection in the onboarding wizard.model-selector.tsx,batch-progress.tsx, andin-progress-batch-group.tsxto support model configuration and batch execution improvements.Test plan:
cd apps/sidecar && npx vitest runpasses (218 tests)cd apps/sidecar && npx vitest run --coveragemeets thresholds (close to 100%)pnpm --filter @openlinear/desktop-ui typecheckpassespnpm --filter @openlinear/sidecar typecheckpassesMade with Cursor
Summary by cubic
Enforces model selection before running tasks and batches, adds safe auto-detection, and fixes the execution workflow and Activity tab by loading historical logs when live state is missing.
New Features
ensureModelConfiguredpre-flight forPOST /:id/executeandPOST /api/batches; both returnMODEL_NOT_CONFIGUREDwithhasProviderdetails.GET /api/opencode/setup-statusnow reportshasProvider/hasModel, auto-detects and persists a model when possible, and setscurrentModelto the resolved value.useExecutionLogsWithHistory+fetchExecutionLogsshow persisted logs with a spinner in Activity; added onboardingModelProviderStepand a provider-tabbedModelSelector; improved batch UI controls and per-task activity.model-errors.ts(isModelNotConfiguredApiError) and integrated toasts inuse-kanban-boardanduse-batch-executionfor missing model/provider.docker-compose.ymlfor local Postgres.Bug Fixes
ExecutionStateStoreproxies (clear,set,Symbol.iterator) and session-mapping fallbacks; stabilized Activity rendering and guarded against stale events.Written for commit c3b08a5. Summary will update on new commits.
Greptile Summary
[Linus Torvalds Mode]
Someone spent a week writing 218 tests for a feature, then forgot to implement half of it and shipped it anyway. The model pre-flight guard exists in
batches.ts, lives in the tests forexecution.ts, and is completely absent from the actualexecution.tsroute — the test suite will fail the moment anyone runs it.This PR enforces AI model selection before task/batch execution, adds
opencode-model.tswithpickAutoDetectedModelandensureModelConfigured, fixesactiveExecutions/sessionToTaskproxy traps (clear,set,Symbol.iterator), and brings a newModelProviderSteponboarding component plus historical log loading in the Activity tab.Key changes:
opencode-model.ts— purepickAutoDetectedModel+ asyncensureModelConfigured; clean, well-tested, correct.POST /api/batches—MODEL_NOT_CONFIGUREDpre-flight added correctly with fail-open on errors.POST /:id/execute— pre-flight guard is missing from the implementation even thoughexecution.test.mjsfully mocks and tests it; those tests will fail.GET /api/opencode/setup-status— now callsclient.config.update()inside the handler, making a GET request mutate state on every poll.ExecutionStateStoreproxy fixes —clearandSymbol.iteratortraps added to both proxies;sessionToTask.setnow actually delegates tosetSessionMapping.useExecutionLogsWithHistory— fetches historical logs when live state is absent; deadhistoricalLogsstate and missing effect cleanup were addressed in prior review rounds.ModelNotConfiguredErrorinopencode.tsis still dead code — exported but never imported anywhere.The sidecar is half-built and the test plan checkboxes are all unchecked — try running the tests before declaring victory next time.
Confidence Score: 4/5
[Linus Torvalds Mode] Someone had the audacity to write tests for code they never wrote and check in the tests anyway. The PR has a P1 defect:
POST /:id/executeis missing itsensureModelConfiguredpre-flight guard — the tests assert it's there and will fail. Fix that before merging.The P1 bug is concrete: two tests in
execution.test.mjsmockensureModelConfigured, assert it is called, and expectMODEL_NOT_CONFIGURED400 responses — butexecution.tsnever imports or calls that function. Every CI run will fail those tests. The rest of the PR —opencode-model.ts, proxy fixes, batch pre-flight, UI components — is solid enough. Score is 4 rather than 5 because that missing route guard needs to be implemented before the test suite can actually pass.The only file that needs a second look from someone who actually reads what they commit:
apps/sidecar/src/routes/execution.ts— add theensureModelConfiguredpre-flight or delete the tests that pretend it's already there.Important Files Changed
ensureModelConfiguredpre-flight guard onPOST /:id/execute— tests expect it, implementation omits it; those tests will fail.ensureModelConfiguredpre-flight andMODEL_NOT_CONFIGURED400 response; fail-open behaviour (null guard) is intentional.GET /setup-statusnow callsclient.config.update()viaensureModelConfigured, making it a mutating GET;readynow requires both provider and model.pickAutoDetectedModeland asyncensureModelConfigured; logic is sound and well-tested.activeExecutionsandsessionToTaskproxies by addingclear,set, andSymbol.iteratortraps;sessionToTask.deletestill no-ops (pre-existing).useExecutionLogsWithHistoryhook fetches historical logs; has deadhistoricalLogsstate, missing effect cleanup, and stalesetLoadingrace (addressed in prior review threads).ModelNotConfiguredErrorclass andisModelNotConfiguredErrorhelper (neither imported anywhere) alongside the correctly-usedisModelNotConfiguredApiErrorinmodel-errors.ts.isModelNotConfiguredApiErrorandreadModelNotConfiguredDetailshelpers; clean and correctly used throughout the UI.SetupStatusfields correctly.Sequence Diagram
sequenceDiagram participant UI as Desktop UI participant SC as Sidecar participant OC as OpenCode UI->>SC: GET /api/opencode/setup-status SC->>OC: provider.list() + config.get() alt "hasProvider && !resolvedModel" SC->>OC: "config.update({ model: detected }) ⚠️ mutating GET" end SC-->>UI: "{ ready, hasProvider, hasModel, currentModel }" UI->>SC: POST /api/tasks/:id/execute Note over SC: ⚠️ ensureModelConfigured NOT called here SC->>OC: executeTask(...) SC-->>UI: "200 { message: 'Task execution started' }" UI->>SC: POST /api/batches SC->>OC: ensureModelConfigured(userId) alt model null SC-->>UI: 400 MODEL_NOT_CONFIGURED else model configured SC->>OC: createBatch(...) SC-->>UI: 201 batch created endReviews (6): Last reviewed commit: "Merge origin/dev: resolve conflicts in S..." | Re-trigger Greptile
Context used: