fix (desktop): git status to not locking anymore#739
Conversation
|
Warning Rate limit exceeded@AviPeltz has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 28 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new getStatusNoLock function that runs git status with --no-optional-locks and porcelain parsing, replaces git.status() calls with it, updates safety checks and uncommitted-change detection, enhances error handling, and runs a fetch --prune during branch checkout safety validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts:
- Around line 193-207: The code pushes the same path into the modified array
twice when indexStatus === "M" and workingStatus === "M"; change the logic in
the block handling indexStatus and workingStatus (the branches that push to
modified, staged, deleted, conflicted — e.g., where indexStatus is checked and
where workingStatus is checked) to avoid duplicate pushes by checking whether
the path is already present before pushing (or use a temporary Set for
modified/staged/deleted/conflicted and convert back to arrays at the end),
ensuring staged still gets its push for indexStatus === "M" but modified is only
added once.
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (2)
213-231: Consider using empty array forignoredto matchStatusResulttype.The
StatusResulttype from simple-git definesignoredasstring[]. Setting it toundefinedmay cause type mismatches in consuming code that expects an array.♻️ Suggested change
return { not_added, conflicted, created, deleted, - ignored: undefined, + ignored: [], modified,
105-105: Typo in function name: "Portelain" should be "Porcelain".♻️ Suggested fix
-function parsePortelainStatus(stdout: string): StatusResult { +function parsePorcelainStatus(stdout: string): StatusResult {Also update the call site at line 82:
- return parsePortelainStatus(stdout); + return parsePorcelainStatus(stdout);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: For Electron interprocess communication, ALWAYS use tRPC as defined insrc/lib/trpc
Use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary.
For tRPC subscriptions with trpc-electron, ALWAYS use the observable pattern from@trpc/server/observableinstead of async generators, as the library explicitly checksisObservable(result)and throws an error otherwise
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use object parameters for functions with 2+ parameters instead of positional arguments
Functions with 2+ parameters should accept a single params object with named properties for self-documentation and extensibility
Use prefixed console logging with context pattern: [domain/operation] message
Extract magic numbers and hardcoded values to named constants at module top
Use lookup objects/maps instead of repeated if (type === ...) conditionals
Avoid usinganytype - maintain type safety in TypeScript code
Never swallow errors silently - at minimum log them with context
Import from concrete files directly when possible - avoid barrel file abuse that creates circular dependencies
Avoid deep nesting (4+ levels) - use early returns, extract functions, and invert conditions
Use named properties in options objects instead of boolean parameters to avoid boolean blindness
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Drizzle ORM for all database operations - never use raw SQL
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Biome for formatting and linting - run at root level with
bun run lint:fixorbiome check --write
Files:
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git.ts (4)
6-6: LGTM!The type-only import of
StatusResultis correctly added to support the newgetStatusNoLockfunction's return type.
54-98: LGTM!The
getStatusNoLockfunction is well-designed:
- Uses
--no-optional-locksto prevent blocking other git operations- Porcelain v1 with
-zensures reliable parsing of filenames with special characters-Menables rename detection for accurate status- Good error handling with descriptive messages for common failure modes
581-586: LGTM!The update to use
getStatusNoLockaligns with the PR objective to prevent git locking during status checks.
893-949: LGTM!The safety check function now correctly uses
getStatusNoLockto avoid blocking other git operations while checking for uncommitted changes before branch checkout. The separatesimpleGitinstance for the fetch operation is appropriate since it's a distinct, best-effort network operation.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
1 participant
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.