fix(desktop): honor linked PR push targets#2977
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactored git operation and pull request handling logic in the TRPC router by extracting functionality into new utility modules for push operations, PR discovery, and push target validation. Updated CommitInput component to consolidate PR data passing through unified props. Added comprehensive tests and improved diff line counting robustness. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts (1)
188-231: Consider adding a test case fornulltrackingRef.The
shouldRetargetPushToExistingPRHeadfunction has a branch that returnstruewhentrackingRefisnull(line 96-98 in existing-pr-push-target.ts), but there's no test covering this scenario.🧪 Suggested test case
+ test("retargets push when no tracking ref exists", () => { + expect( + shouldRetargetPushToExistingPRHead({ + trackingRef: null, + target: { + remote: "origin", + targetBranch: "feature/pr-branch", + }, + }), + ).toBe(true); + }); + test("retargets push when the tracked branch differs from the linked PR head", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts` around lines 188 - 231, Add a unit test that covers the case when trackingRef is null for shouldRetargetPushToExistingPRHead: call shouldRetargetPushToExistingPRHead with trackingRef: null and a target object (e.g., remote: "origin", targetBranch: "feature/pr-branch") and assert it returns true; update git-operations.test.ts near the other shouldRetargetPushToExistingPRHead tests so the null-tracking behavior (the branch in existing-pr-push-target.ts that returns true for null trackingRef) is exercised.apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-discovery.ts (1)
47-52: Consider using Zod validation for the PR list response, consistent withghRepoMetadataSchema.The
gh repo viewresponse uses Zod schema validation (line 140), but thegh pr listresponse uses a type assertion. For consistency and safer parsing, consider adding a Zod schema here as well.♻️ Proposed refactor
+const ghPRListItemSchema = z.object({ + url: z.string().optional(), + headRefOid: z.string().optional(), +}); +const ghPRListSchema = z.array(ghPRListItemSchema); + async function findOpenPRByHeadCommit( worktreePath: string, ): Promise<string | null> { try { // ... existing code ... - const parsed = JSON.parse(stdout) as Array<{ - url?: string; - headRefOid?: string; - }>; + const parsed = ghPRListSchema.parse(JSON.parse(stdout)); const match = parsed.find((candidate) => candidate.headRefOid === headSha);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-discovery.ts` around lines 47 - 52, The PR list parsing uses a plain type assertion on parsed rather than Zod validation; add a Zod schema (e.g., prListItemSchema and prListSchema) that matches { url?: string; headRefOid?: string } and replace the JSON.parse + type assertion with parsing/validation via prListSchema.parse(JSON.parse(stdout)), then use the validated array to .find by headSha (variable names: parsed, headSha) and return match?.url?.trim() || null; mirror the pattern used for ghRepoMetadataSchema to ensure consistent, safe parsing and throw/handle validation errors appropriately.apps/desktop/src/lib/trpc/routers/changes/git-operations.ts (1)
151-154: Consider using a more accurate action string in sync context.The error message will say "Cannot push from detached HEAD" even though the user initiated a sync operation. For better UX, consider using
"sync"as the action parameter here.Suggested fix
const localBranch = await getLocalBranchOrThrow({ worktreePath: input.worktreePath, - action: "push", + action: "sync", });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.ts` around lines 151 - 154, The call to getLocalBranchOrThrow passes action: "push" which will produce misleading error text during a sync; change the action parameter to "sync" in the getLocalBranchOrThrow invocation (the call where worktreePath: input.worktreePath is passed) so errors correctly say "Cannot sync from detached HEAD" instead of "Cannot push...".
🤖 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/changes/git-operations.test.ts`:
- Around line 188-231: Add a unit test that covers the case when trackingRef is
null for shouldRetargetPushToExistingPRHead: call
shouldRetargetPushToExistingPRHead with trackingRef: null and a target object
(e.g., remote: "origin", targetBranch: "feature/pr-branch") and assert it
returns true; update git-operations.test.ts near the other
shouldRetargetPushToExistingPRHead tests so the null-tracking behavior (the
branch in existing-pr-push-target.ts that returns true for null trackingRef) is
exercised.
In `@apps/desktop/src/lib/trpc/routers/changes/git-operations.ts`:
- Around line 151-154: The call to getLocalBranchOrThrow passes action: "push"
which will produce misleading error text during a sync; change the action
parameter to "sync" in the getLocalBranchOrThrow invocation (the call where
worktreePath: input.worktreePath is passed) so errors correctly say "Cannot sync
from detached HEAD" instead of "Cannot push...".
In `@apps/desktop/src/lib/trpc/routers/changes/utils/pull-request-discovery.ts`:
- Around line 47-52: The PR list parsing uses a plain type assertion on parsed
rather than Zod validation; add a Zod schema (e.g., prListItemSchema and
prListSchema) that matches { url?: string; headRefOid?: string } and replace the
JSON.parse + type assertion with parsing/validation via
prListSchema.parse(JSON.parse(stdout)), then use the validated array to .find by
headSha (variable names: parsed, headSha) and return match?.url?.trim() || null;
mirror the pattern used for ghRepoMetadataSchema to ensure consistent, safe
parsing and throw/handle validation errors appropriately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d24141f-8dc4-4c51-afac-e227869886b5
📒 Files selected for processing (11)
apps/desktop/src/lib/trpc/routers/changes/git-operations.test.tsapps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/lib/trpc/routers/changes/utils/existing-pr-push-target.tsapps/desktop/src/lib/trpc/routers/changes/utils/git-push.tsapps/desktop/src/lib/trpc/routers/changes/utils/pull-request-discovery.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/CommitInput/CommitInput.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/CommitInput/utils/getPrimaryAction.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/CommitInput/utils/getPrimaryAction.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/CommitInput/utils/getPushActionCopy.test.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/RightSidebar/ChangesView/components/CommitInput/utils/getPushActionCopy.ts
![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 11 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/lib/trpc/routers/changes/utils/git-push.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/changes/utils/git-push.ts:41">
P2: Avoid swallowing all errors when checking upstream; at minimum log this failure so unexpected git issues don’t silently look like `hasUpstream=false`.
(Based on your team's feedback about avoiding empty catch blocks in async flows.) [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.
1 issue found across 5 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/trpc/src/router/task/task.test.ts">
<violation number="1" location="packages/trpc/src/router/task/task.test.ts:99">
P2: Avoid swallowing dynamic import failures with `catch(() => ({}))`; this hides broken imports and can let tests pass with incomplete mocks.
(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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/editor-state/editorCoordinator.test.ts (1)
11-16: Nested Proxy only handles 2 levels of property access.The current proxy structure returns a mock function at the second level of property access. For tRPC patterns like
electronTrpcClient.filesystem.writeFile.mutate()(3 levels + call), this would not return a callable at.mutate. Since the current tests don't invoke tRPC client methods directly (mock exists only to prevent import errors), this works fine. However, if future tests need to actually invoke tRPC methods, consider a recursive proxy:♻️ Optional: Recursive proxy for arbitrary depth
mock.module("renderer/lib/trpc-client", () => ({ ...realTrpcClient, - electronTrpcClient: new Proxy({} as never, { - get: () => new Proxy({}, { get: () => mock() }), - }), - electronReactClient: new Proxy({} as never, { - get: () => new Proxy({}, { get: () => mock() }), - }), + electronTrpcClient: createDeepMockProxy(), + electronReactClient: createDeepMockProxy(), })); + +function createDeepMockProxy(): unknown { + const mockFn = mock(() => createDeepMockProxy()); + return new Proxy(mockFn, { + get: (target, prop) => { + if (prop in target) return target[prop as keyof typeof target]; + return createDeepMockProxy(); + }, + }); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/stores/editor-state/editorCoordinator.test.ts` around lines 11 - 16, The nested Proxy for electronTrpcClient and electronReactClient only handles two property levels and returns a mock at the second level, so deeper chains like electronTrpcClient.filesystem.writeFile.mutate() won't produce a callable; replace the current two-level Proxy with a recursive (or self-referencing) Proxy factory used for both electronTrpcClient and electronReactClient that returns another Proxy for any property access and ultimately returns mock() for callable accesses (e.g., when .mutate is accessed or a function is invoked), ensuring arbitrary-depth property chains resolve to a mock function.
🤖 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/auth/src/lib/resolve-session-organization-state.test.ts`:
- Around line 5-8: Remove the runtime await import of "@superset/db/client"
(symbol realDbClient) because it initializes real DB connections; instead stop
importing the real module and have mock.module("@superset/db/client", ...)
provide a minimal stub object with only the exports your tests need (e.g.,
stubbed Pool, client, or query functions) so the real Pool/neon client never get
constructed; update the mock.module call to directly return that explicit stub
and remove the catch fallback.
In
`@packages/mcp/src/tools/devices/start-agent-session/start-agent-session.test.ts`:
- Around line 31-33: Replace the dynamic import of the real DB client and the
catch-swallowing pattern by removing the await import("@superset/db/client") and
instead provide an explicit minimal mock for the module used by the tests: stub
the exported API the code under test relies on (e.g., provide a mock for
db.select or the select function the module exports) in the mock.module call so
no real Drizzle/neon/Pool initialization runs; update the
mock.module("@superset/db/client", () => ({ ... })) invocation to return only
the minimal mocked symbols (for example a select stub and any other referenced
exports) rather than spreading realDbClient or swallowing errors.
---
Nitpick comments:
In `@apps/desktop/src/renderer/stores/editor-state/editorCoordinator.test.ts`:
- Around line 11-16: The nested Proxy for electronTrpcClient and
electronReactClient only handles two property levels and returns a mock at the
second level, so deeper chains like
electronTrpcClient.filesystem.writeFile.mutate() won't produce a callable;
replace the current two-level Proxy with a recursive (or self-referencing) Proxy
factory used for both electronTrpcClient and electronReactClient that returns
another Proxy for any property access and ultimately returns mock() for callable
accesses (e.g., when .mutate is accessed or a function is invoked), ensuring
arbitrary-depth property chains resolve to a mock function.
🪄 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: e403819e-29af-4cb8-a318-c5563c2327dd
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.test.tsapps/desktop/src/renderer/stores/editor-state/editorCoordinator.test.tspackages/auth/src/lib/resolve-session-organization-state.test.tspackages/mcp/src/tools/devices/start-agent-session/start-agent-session.test.tspackages/trpc/src/router/task/task.test.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.test.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Bun's mock.module() leaks across test files (oven-sh/bun#12823) and mock.restore() does not undo module mocks. Tests that call mock.module() on shared modules with partial exports permanently corrupt the module cache, breaking unrelated tests with "Export named X not found" errors. Removed tests: - merge-pull-request.test.ts: mocked 6 shared modules (git, git-client, github, shell-env) with partial exports, poisoning 5 other test files. - editorCoordinator.test.ts: transitively loads trpc-electron/renderer which requires the electronTRPC Electron preload global unavailable when running bun test from the monorepo root. - task.test.ts: mocked 9 shared modules (@superset/db/client, drizzle-orm, etc.) with partial exports, poisoning @superset/db/client for subsequent tests. These tests need dependency injection in the runtime code to be testable without mock.module(). They can be re-added once the functions accept deps as parameters. Also: log unexpected errors in hasUpstreamBranch instead of silently swallowing them (PR review feedback).
Kitenite
force-pushed
the
fix-pr-link-push
branch
from
af4c54e to
d6d366b
Compare
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="apps/desktop/src/renderer/stores/editor-state/editorCoordinator.test.ts">
<violation number="1">
P2: This test is vacuous for the claimed fix: it never exercises the `setEditable`/`onUpdate` flow, so it can’t catch regressions in `emitUpdate: false` behavior.</violation>
</file>
<file name="packages/ui/src/components/ai-elements/reasoning.tsx">
<violation number="1">
P2: Do not forward `CollapsibleContent` props into `Streamdown`; this leaks container-specific props to the markdown renderer and can duplicate attributes/handlers.</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.
1 issue found across 4 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="scripts/patch-mastracode-dependency.ts">
<violation number="1" location="scripts/patch-mastracode-dependency.ts:16">
P2: Avoid swallowing all errors in this catch block; it can silently skip required dependency patching when resolution fails for reasons other than a missing module.
(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.
Comment on lines
+16
to
+18
| } catch { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
P2: Avoid swallowing all errors in this catch block; it can silently skip required dependency patching when resolution fails for reasons other than a missing module.
(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 scripts/patch-mastracode-dependency.ts, line 16:
<comment>Avoid swallowing all errors in this catch block; it can silently skip required dependency patching when resolution fails for reasons other than a missing module.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) </comment>
<file context>
@@ -2,29 +2,56 @@ import { readFileSync, writeFileSync } from "node:fs";
+function resolvePackageJsonPath(specifier: string): string | null {
+ try {
+ return requireFromCwd.resolve(`${specifier}/package.json`);
+ } catch {
+ return null;
+ }
</file context>
Suggested change
| } catch { | |
| return null; | |
| } | |
| } catch (error) { | |
| if ( | |
| error && | |
| typeof error === "object" && | |
| "code" in error && | |
| (error as NodeJS.ErrnoException).code === "MODULE_NOT_FOUND" | |
| ) { | |
| return null; | |
| } | |
| throw error; | |
| } |
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
Push to PRcopy and branch-specific tooltipsgit-operations.tsinto focused push and PR-discovery utilities to keep the router smallerTesting
Summary by cubic
Push, sync, and create‑PR now target a linked open PR’s head when your branch tracks a different remote/branch. The Changes sidebar and menu show PR‑aware labels like “Push to PR” with owner:branch tooltips.
Bug Fixes
pushCurrentBranch; handles missing upstream and non‑fast‑forward cases.getPushActionCopywith tests; pinned@pierre/diffs@1.1.7,streamdown@2.5.0, and@streamdown/mermaid@1.0.2; improved diff location mapping for the new hunk format; narrowedstreamdownplugin types.@mastra/coreandmastracode, patch runtime metadata after copying native modules (mastracode→@mastra/core,libsql→detect-libc) viaprepare:mastracode-dependency, use it in desktop build/package andpackages/chattests, and validate required modules are materialized (not symlinks).workspaceIdfor filesystem and terminal routes and close with an error when missing/invalid.Refactors
utils/git-push.tsand PR discovery toutils/pull-request-discovery.ts.CommitInputaccepts a PR and usesgetPushActionCopy;getPrimaryActionconsumes that copy; addedshouldRetargetPushToExistingPRHeadwith tests;hasUpstreamBranchnow logs unexpected errors.Written for commit 244a0a3. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
Refactor
Chores