fix(desktop): prevent [gone] from being stored as workspace branch name#2885
Merged
Conversation
The porcelain status parser incorrectly extracted "[gone]" as the branch name when git reported "No commits yet on BRANCH...origin/BRANCH [gone]". The old code split by space and took the last element; the fix strips the prefix and applies the same tracking-info regex used for normal branches. Also adds syncBranch validation to reject obviously invalid names and a one-time startup repair that reads the real branch from the worktree HEAD file for any existing corrupted records.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes branch/HEAD resolution with getCurrentBranch/getHeadSha, upgrades git status parsing to porcelain v2, tightens branch-name validation in syncBranch, and adds detached-HEAD handling and tests across git, PR, and workspace flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Git as Git CLI
participant GitHub
participant DB
Client->>Server: trigger operation (push/sync/fetch/createPR/merge)
Server->>Git: getCurrentBranch(worktreePath)
Git-->>Server: branch name OR null
alt branch == null (detached / cannot resolve)
Server->>Git: run legacy gh/git commands using worktree cwd
Git-->>Server: command result
Server->>GitHub: (optional) fetch/report via gh CLI
GitHub-->>Server: result
Server->>Client: return success or TRPCError per flow
else branch resolved
Server->>Git: perform git operations (fetch/push, rev-parse HEAD)
Git-->>Server: headSha or error (unborn handled as undefined)
Server->>DB: update workspace.branch / headSha if changed
DB-->>Server: confirmation
Server->>GitHub: fetch PR/status/preview (headSha optional)
GitHub-->>Server: result
Server->>Client: return success
end
sequenceDiagram
participant Caller as getStatusNoLock()
participant Git as Git CLI
participant Parser as parsePorcelainStatusV2
Caller->>Git: git status --porcelain=v2 --branch
Git-->>Caller: stdout (porcelain v2)
Caller->>Parser: parse stdout
Parser->>Parser: read `# branch.*` headers (head, upstream, ab)
Parser->>Parser: parse file records (1,2,u,?,!)
Parser-->>Caller: StatusResult { files, staged, modified, renamed, conflicted, ahead, behind }
Caller-->>Caller: return status to caller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/local-db/index.ts">
<violation number="1" location="apps/desktop/src/main/lib/local-db/index.ts:125">
P2: Don't silently swallow per-worktree repair errors; log a warning with worktree context so failed repairs are 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/local-db/index.ts (1)
109-141: Consider using Drizzle ORM instead of raw SQLite statements.The coding guidelines specify "Use Drizzle ORM for all database operations", but this repair logic uses raw
sqlite.prepare()calls. SincelocalDbis already initialized at this point, the queries could be refactored to use Drizzle for consistency.Additionally, the summary log at line 146 reports
corruptedWorktrees.length, which is the number of worktrees queried, not the number actually repaired (some may have unreadable HEAD files). Consider tracking successful repairs separately.♻️ Suggested refactor using Drizzle ORM
+import { eq } from "drizzle-orm"; // ... at the repair section: - const corruptedWorktrees = sqlite - .prepare( - "SELECT id, path FROM worktrees WHERE branch = '[gone]'", - ) - .all() as { id: string; path: string }[]; + const corruptedWorktrees = localDb + .select({ id: schema.worktrees.id, path: schema.worktrees.path }) + .from(schema.worktrees) + .where(eq(schema.worktrees.branch, "[gone]")) + .all(); + let repairedCount = 0; for (const wt of corruptedWorktrees) { // ... branch resolution logic unchanged ... if (realBranch) { - sqlite - .prepare("UPDATE worktrees SET branch = ? WHERE id = ?") - .run(realBranch, wt.id); - sqlite - .prepare( - "UPDATE workspaces SET branch = ? WHERE worktree_id = ? AND branch = '[gone]'", - ) - .run(realBranch, wt.id); + localDb + .update(schema.worktrees) + .set({ branch: realBranch }) + .where(eq(schema.worktrees.id, wt.id)) + .run(); + localDb + .update(schema.workspaces) + .set({ branch: realBranch }) + .where( + and( + eq(schema.workspaces.worktreeId, wt.id), + eq(schema.workspaces.branch, "[gone]"), + ), + ) + .run(); + repairedCount++; console.log( `[local-db] Repaired branch for worktree ${wt.id}: [gone] → ${realBranch}`, ); } } - if (corruptedWorktrees.length > 0) { + if (repairedCount > 0) { console.log( - `[local-db] Repaired ${corruptedWorktrees.length} corrupted worktree branch(es)`, + `[local-db] Repaired ${repairedCount} corrupted worktree branch(es)`, ); }As per coding guidelines: "Use Drizzle ORM for all database operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/local-db/index.ts` around lines 109 - 141, The code uses raw sqlite.prepare() calls (corruptedWorktrees query and subsequent UPDATEs) instead of the project's Drizzle ORM and also logs corruptedWorktrees.length rather than the number actually repaired; replace the raw SQL in this repair block by using the initialized Drizzle client (localDb / drizzle instance) to query worktrees (instead of sqlite.prepare("SELECT ...")), and perform the two updates via Drizzle update calls targeting the worktrees and workspaces tables (the UPDATEs currently run after finding realBranch), while incrementing a repaired counter when an update runs successfully; finally, change the summary log to report the repaired count (not corruptedWorktrees.length).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/lib/local-db/index.ts`:
- Around line 109-141: The code uses raw sqlite.prepare() calls
(corruptedWorktrees query and subsequent UPDATEs) instead of the project's
Drizzle ORM and also logs corruptedWorktrees.length rather than the number
actually repaired; replace the raw SQL in this repair block by using the
initialized Drizzle client (localDb / drizzle instance) to query worktrees
(instead of sqlite.prepare("SELECT ...")), and perform the two updates via
Drizzle update calls targeting the worktrees and workspaces tables (the UPDATEs
currently run after finding realBranch), while incrementing a repaired counter
when an update runs successfully; finally, change the summary log to report the
repaired count (not corruptedWorktrees.length).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31596822-7761-4a99-9ab0-828b021bf297
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/workspaces/procedures/status.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/main/lib/local-db/index.ts
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
There was a problem hiding this comment.
1 issue found across 8 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/lib/trpc/routers/workspaces/utils/git.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts:296">
P2: `u` records are always unmerged/conflicted in porcelain v2, but the parser only flags conflicts when XY contains `U`. Conflict types like `AA`/`DD` won’t be reported. Mark all `u` entries as conflicted when parsing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)
184-202:⚠️ Potential issue | 🟠 MajorSeparate branch resolution from HEAD hash lookup to repair branches on unborn worktrees.
Line 186 blocks the update on unborn branches:
Promise.all()rejects whengit.revparse(["HEAD"])fails, preventing the branch name (whichgetCurrentBranchName()can resolve via symbolic-ref) from being persisted. WhengetCurrentBranchName()returnsnull, theif (!branch) { continue; }also leaves stale PR linkage unchanged indefinitely. Resolve the branch first, then attemptheadShawith error handling, or explicitly clear PR state when no branch can be resolved.💡 Suggested direction
- const [branch, rawHeadSha] = await Promise.all([ - getCurrentBranchName(git), - git.revparse(["HEAD"]), - ]); - if (!branch) { - continue; - } - const headSha = rawHeadSha.trim(); + const branch = await getCurrentBranchName(git); + const headSha = await git + .revparse(["HEAD"]) + .then((value) => value.trim()) + .catch(() => null); + + if (!branch) { + // Clear any stale PR linkage here if detached/unresolvable + // workspaces should not retain their previous branch state. + continue; + } @@ - .set({ - branch, - headSha, - }) + .set({ + branch, + ...(headSha ? { headSha } : {}), + })🤖 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/pull-requests.ts` around lines 184 - 202, The Promise.all() call mixes branch resolution and rev-parse so a failing git.revparse(["HEAD"]) aborts persisting a resolvable branch; change the logic in the loop to call getCurrentBranchName(git) first and handle its result (if branch is null either clear PR linkage by updating branch/headSha to null via this.db.update(workspaces).set(...) or continue after deciding clear vs skip), then separately attempt to get headSha by calling git.revparse(["HEAD"]) inside a try/catch and set headSha to trimmed value on success or to null/undefined on failure before comparing to workspace.branch/workspace.headSha and performing the update; reference getCurrentBranchName, git.revparse, headSha, workspace.branch, workspace.headSha, and this.db.update(workspaces).set(...) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 184-202: The Promise.all() call mixes branch resolution and
rev-parse so a failing git.revparse(["HEAD"]) aborts persisting a resolvable
branch; change the logic in the loop to call getCurrentBranchName(git) first and
handle its result (if branch is null either clear PR linkage by updating
branch/headSha to null via this.db.update(workspaces).set(...) or continue after
deciding clear vs skip), then separately attempt to get headSha by calling
git.revparse(["HEAD"]) inside a try/catch and set headSha to trimmed value on
success or to null/undefined on failure before comparing to
workspace.branch/workspace.headSha and performing the update; reference
getCurrentBranchName, git.revparse, headSha, workspace.branch,
workspace.headSha, and this.db.update(workspaces).set(...) when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91083718-d52e-424e-9c1a-eac2d6861b86
📒 Files selected for processing (8)
apps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/lib/trpc/routers/changes/security/git-commands.tsapps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.test.tsapps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tspackages/host-service/src/runtime/pull-requests/pull-requests.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/host-service/test/pull-requests.test.ts (1)
1-3: Move this test next topull-requests.tsto match repo test co-location rules.Please place this as a co-located test (for example, alongside
packages/host-service/src/runtime/pull-requests/pull-requests.ts) instead of the sharedtest/folder.As per coding guidelines,
**/*.{ts,tsx}: Co-locate tests with their implementation files (e.g.,Component.test.tsxnext toComponent.tsx).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/test/pull-requests.test.ts` around lines 1 - 3, Move the test file from the shared test folder into the same directory as the implementation file: place packages/host-service/test/pull-requests.test.ts next to the implementation pull-requests.ts (e.g., packages/host-service/src/runtime/pull-requests/pull-requests.test.ts); update the import path for PullRequestRuntimeManager if needed to a relative import that points to pull-requests.ts and ensure the test still imports describe/expect/mock/test from "bun:test".packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)
195-197: Consider lightweight debug logging when branch resolution is skipped.Line 195 silently
continues on unresolved branch; a debug-level log here would improve diagnosis of recurring workspace sync gaps without adding noisy warnings.🤖 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/pull-requests.ts` around lines 195 - 197, When branch resolution yields falsy and the code hits "if (!branch) { continue; }", add a lightweight debug log before the continue so unresolved branches are recorded for diagnostics; use the module's existing logger (e.g., logger.debug or processLogger.debug) to emit a concise message including the PR identifier or workspace id and the resolved branch value (null/undefined) so it remains silent in normal runs but helps trace recurring sync gaps.apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
308-327: Consider adding defensive handling for missing rename source path.For type
2(rename/copy) records, the original path is expected on the next NUL-separated entry. Iffromis undefined (e.g., malformed output), the code falls back to usingpathas the original, which could mask parsing issues.The current fallback at line 315 is reasonable, but consider logging a warning when the expected
fromentry is missing to aid debugging.💡 Optional: Add warning for missing rename source
if (entry.startsWith("2 ")) { const match = entry.match(/^2 (\S{2}) \S+ \S+ \S+ \S+ \S+ \S+ \S+ (.+)$/); const from = entries[i + 1]; if (match) { const xy = match[1] || ".."; const path = match[2]; if (path) { - const originalPath = from || path; + const originalPath = from || path; + if (!from) { + console.warn("[parsePorcelainStatusV2] Missing original path for rename entry:", path); + } renamed.push({ from: originalPath, to: path });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 308 - 327, The rename/copy handling block (when entry.startsWith("2 ")) should defensively detect when the expected source entry `from = entries[i + 1]` is missing and emit a warning before falling back to using `path` as the original; update the block around variables `entry`, `from`, `match`, `path`, `renamed`, and the call to `addFile`/`normalizeStatusCode` to: if `from` is undefined log a concise warning (using the project's logger instance, e.g., logger.warn or processLogger.warn) including the raw `entry` (or `match[0]`) and the computed `path`/index `i`, then proceed with the existing fallback logic that pushes to `renamed` and calls `addFile`. Ensure the warning contains enough context (entry type "2", xy, path, and i) and does not change the current control flow (still increment `i += 2` and continue).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 308-327: The rename/copy handling block (when entry.startsWith("2
")) should defensively detect when the expected source entry `from = entries[i +
1]` is missing and emit a warning before falling back to using `path` as the
original; update the block around variables `entry`, `from`, `match`, `path`,
`renamed`, and the call to `addFile`/`normalizeStatusCode` to: if `from` is
undefined log a concise warning (using the project's logger instance, e.g.,
logger.warn or processLogger.warn) including the raw `entry` (or `match[0]`) and
the computed `path`/index `i`, then proceed with the existing fallback logic
that pushes to `renamed` and calls `addFile`. Ensure the warning contains enough
context (entry type "2", xy, path, and i) and does not change the current
control flow (still increment `i += 2` and continue).
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 195-197: When branch resolution yields falsy and the code hits "if
(!branch) { continue; }", add a lightweight debug log before the continue so
unresolved branches are recorded for diagnostics; use the module's existing
logger (e.g., logger.debug or processLogger.debug) to emit a concise message
including the PR identifier or workspace id and the resolved branch value
(null/undefined) so it remains silent in normal runs but helps trace recurring
sync gaps.
In `@packages/host-service/test/pull-requests.test.ts`:
- Around line 1-3: Move the test file from the shared test folder into the same
directory as the implementation file: place
packages/host-service/test/pull-requests.test.ts next to the implementation
pull-requests.ts (e.g.,
packages/host-service/src/runtime/pull-requests/pull-requests.test.ts); update
the import path for PullRequestRuntimeManager if needed to a relative import
that points to pull-requests.ts and ensure the test still imports
describe/expect/mock/test from "bun:test".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 58a7c77b-ba7d-44ad-a0cd-642bfbbc924c
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.test.tsapps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/test/pull-requests.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts
- apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.test.ts
Contributor
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/runtime/pull-requests/pull-requests.ts">
<violation number="1" location="packages/host-service/src/runtime/pull-requests/pull-requests.ts:48">
P2: Avoid swallowing all `revparse("HEAD")` errors here. Only suppress the expected unborn-HEAD case; rethrow unexpected errors so they are observable and don’t silently null out `headSha`.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.ts:47">
P2: This catch masks all `rev-parse HEAD` errors and always downgrades to `headSha = undefined`. Only the known unborn-HEAD failure should be tolerated; other errors should be rethrown/logged so PR resolution doesn’t silently lose SHA disambiguation.
(Based on your team's feedback about avoiding silent catch blocks.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (1)
314-329: Consider adding a fallback for malformed ordinary change records.The regex for ordinary change records (
1 XY ...) assumes a specific format. If the regex doesn't match, the entry is silently skipped.While unlikely in practice, if git emits a slightly different format in future versions, files could be silently missed. The current behavior (skip and continue) is defensive, but you may want to add debug logging for unmatched entries during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts` around lines 314 - 329, The ordinary change record branch that handles entries starting with "1 " currently ignores malformed lines when the regex in that block doesn't match; update the branch in the same block (where entry is checked with entry.startsWith("1 ") and match is derived) to add a fallback: when match is falsy, emit a debug/processLogger.warn including the raw entry string so malformed records are visible during development, and optionally attempt a conservative parse (e.g., split on spaces and take the last token as path) before continuing; keep existing use of addFile and normalizeStatusCode for properly parsed records.packages/host-service/test/pull-requests.test.ts (1)
1-2: Consider co-locating this test withpull-requests.ts.Placing this test next to
packages/host-service/src/runtime/pull-requests/pull-requests.tswould align with the repository’s test-location rule and reduce cross-folder coupling.As per coding guidelines, "Co-locate tests with their implementation files (e.g.,
Component.test.tsxnext toComponent.tsx)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/test/pull-requests.test.ts` around lines 1 - 2, The test file pull-requests.test.ts should be moved to sit next to the implementation pull-requests.ts and its imports updated accordingly: relocate packages/host-service/test/pull-requests.test.ts to the same directory as pull-requests.ts, update the import of PullRequestRuntimeManager to use a relative path (pointing to "./pull-requests" or the local module name), and adjust any test-runner or export references if necessary so the test imports PullRequestRuntimeManager from the co-located module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/host-service/test/pull-requests.test.ts`:
- Around line 115-124: The test creates a Jest spy with const warnSpy =
spyOn(console, "warn") but never restores it, which can leak into other tests;
fix by restoring the spy after use (e.g., call warnSpy.mockRestore() after the
assertions in this test) or add an afterEach cleanup that calls
warnSpy.mockRestore() when defined. Locate the warnSpy declaration in the test
that calls (manager as unknown as { syncWorkspaceBranches: () => Promise<void>
}).syncWorkspaceBranches() and ensure you call warnSpy.mockRestore() (or use
jest.restoreAllMocks() in afterEach) so console.warn is returned to its original
implementation.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts`:
- Around line 314-329: The ordinary change record branch that handles entries
starting with "1 " currently ignores malformed lines when the regex in that
block doesn't match; update the branch in the same block (where entry is checked
with entry.startsWith("1 ") and match is derived) to add a fallback: when match
is falsy, emit a debug/processLogger.warn including the raw entry string so
malformed records are visible during development, and optionally attempt a
conservative parse (e.g., split on spaces and take the last token as path)
before continuing; keep existing use of addFile and normalizeStatusCode for
properly parsed records.
In `@packages/host-service/test/pull-requests.test.ts`:
- Around line 1-2: The test file pull-requests.test.ts should be moved to sit
next to the implementation pull-requests.ts and its imports updated accordingly:
relocate packages/host-service/test/pull-requests.test.ts to the same directory
as pull-requests.ts, update the import of PullRequestRuntimeManager to use a
relative path (pointing to "./pull-requests" or the local module name), and
adjust any test-runner or export references if necessary so the test imports
PullRequestRuntimeManager from the co-located module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 07dee775-c931-4af0-8e3b-880657c9a6a2
📒 Files selected for processing (7)
apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.test.tsapps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/git.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tspackages/host-service/src/runtime/pull-requests/pull-requests.tspackages/host-service/test/pull-requests.test.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.ts
- packages/host-service/src/runtime/pull-requests/pull-requests.ts
Comment on lines
+115
to
+124
| const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); | ||
|
|
||
| await ( | ||
| manager as unknown as { syncWorkspaceBranches: () => Promise<void> } | ||
| ).syncWorkspaceBranches(); | ||
|
|
||
| expect(setMock).not.toHaveBeenCalled(); | ||
| expect(refreshProjectMock).not.toHaveBeenCalled(); | ||
| expect(warnSpy).toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this file has a console.warn spy without restoration.
rg -n -C2 'spyOn\(console,\s*"warn"\)|mockRestore\(' packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 277
🏁 Script executed:
#!/bin/bash
# Get full context of the test around lines 115-124, plus any cleanup hooks
sed -n '100,150p' packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 838
🏁 Script executed:
#!/bin/bash
# Search for afterEach or afterAll cleanup hooks in the entire test file
rg -n 'afterEach|afterAll' packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/bin/bash
# Find the test function definition that includes line 115
sed -n '75,125p' packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 1515
🏁 Script executed:
#!/bin/bash
# Search for other spyOn calls in the file to see the pattern
rg -n 'spyOn\(' packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Find the test function definition
sed -n '1,80p' packages/host-service/test/pull-requests.test.ts | tail -30Repository: superset-sh/superset
Length of output: 928
🏁 Script executed:
#!/bin/bash
# Check imports and test framework
head -20 packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 807
🏁 Script executed:
#!/bin/bash
# Count number of tests in file
rg -c "it\(|test\(" packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Check Bun test framework documentation pattern by searching for mockRestore in codebase
rg -n 'mockRestore' packages/host-service/test/ --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check if there are other test files in the project and how they handle spyOn
find packages/host-service/test -name "*.test.ts" -o -name "*.test.tsx" | head -5Repository: superset-sh/superset
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check other test files for spyOn patterns
rg -l 'spyOn' packages/ --type ts --type tsx | head -5Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for mockRestore usage in entire codebase
rg -n 'mockRestore' packages/ --type tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
#!/antml:bin/bash
# Find other test files with spyOn to see patterns
rg -l 'spyOn' packages/ --type tsRepository: superset-sh/superset
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check Bun test docs about spy cleanup - search for test cleanup patterns in codebase
rg -n 'afterEach|afterAll|finally.*mockRestore' packages/ --type tsRepository: superset-sh/superset
Length of output: 2305
🏁 Script executed:
#!/bin/bash
# Check what typical afterEach cleanup does in similar test files
sed -n '19,25p' packages/workspace-fs/src/host/service.test.tsRepository: superset-sh/superset
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Check the pull-requests.test.ts file structure - does it have afterEach at all?
rg -n 'describe|afterEach|beforeEach' packages/host-service/test/pull-requests.test.tsRepository: superset-sh/superset
Length of output: 189
Restore console.warn spy to avoid cross-test leakage.
warnSpy is never restored, which can affect the other test in this file. The codebase uses afterEach cleanup patterns elsewhere.
Proposed fix
const warnSpy = spyOn(console, "warn").mockImplementation(() => {});
-
- await (
- manager as unknown as { syncWorkspaceBranches: () => Promise<void> }
- ).syncWorkspaceBranches();
-
- expect(setMock).not.toHaveBeenCalled();
- expect(refreshProjectMock).not.toHaveBeenCalled();
- expect(warnSpy).toHaveBeenCalled();
+ try {
+ await (
+ manager as unknown as { syncWorkspaceBranches: () => Promise<void> }
+ ).syncWorkspaceBranches();
+
+ expect(setMock).not.toHaveBeenCalled();
+ expect(refreshProjectMock).not.toHaveBeenCalled();
+ expect(warnSpy).toHaveBeenCalled();
+ } finally {
+ warnSpy.mockRestore();
+ }📝 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
| const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); | |
| await ( | |
| manager as unknown as { syncWorkspaceBranches: () => Promise<void> } | |
| ).syncWorkspaceBranches(); | |
| expect(setMock).not.toHaveBeenCalled(); | |
| expect(refreshProjectMock).not.toHaveBeenCalled(); | |
| expect(warnSpy).toHaveBeenCalled(); | |
| }); | |
| const warnSpy = spyOn(console, "warn").mockImplementation(() => {}); | |
| try { | |
| await ( | |
| manager as unknown as { syncWorkspaceBranches: () => Promise<void> } | |
| ).syncWorkspaceBranches(); | |
| expect(setMock).not.toHaveBeenCalled(); | |
| expect(refreshProjectMock).not.toHaveBeenCalled(); | |
| expect(warnSpy).toHaveBeenCalled(); | |
| } finally { | |
| warnSpy.mockRestore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/host-service/test/pull-requests.test.ts` around lines 115 - 124, The
test creates a Jest spy with const warnSpy = spyOn(console, "warn") but never
restores it, which can leak into other tests; fix by restoring the spy after use
(e.g., call warnSpy.mockRestore() after the assertions in this test) or add an
afterEach cleanup that calls warnSpy.mockRestore() when defined. Locate the
warnSpy declaration in the test that calls (manager as unknown as {
syncWorkspaceBranches: () => Promise<void> }).syncWorkspaceBranches() and ensure
you call warnSpy.mockRestore() (or use jest.restoreAllMocks() in afterEach) so
console.warn is returned to its original implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
parsePortelainStatusincorrectly extracted[gone]as the branch name when git reported## No commits yet on BRANCH...origin/BRANCH [gone]. The old code split by space and took the last element; the fix strips the prefix and applies the same tracking-info regex used for normal branches.syncBranchmutation now rejects branch names starting with[or containing spaces, preventing git status artifacts from being persisted.[gone]records.Root cause
When a worktree's remote tracking branch is deleted (PR merged) and the worktree is in a "No commits yet" state,
git status --porcelain=v1 -boutputs:The parser split by space and took the last word (
[gone]) instead of parsing the actual branch name (my-branch).useBranchSyncInvalidationthen synced this invalid value into the database.Test plan
[gone]now display the correct branch name after app restartsyncBranchrejects[gone]and other invalid branch namesSummary by cubic
Fixes a bug that stored “[gone]” as the workspace branch when parsing git status for new branches with a deleted remote. Updates the parser and adds stricter branch validation.
Written for commit 0e56023. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests