Repository navigation
feat: Add Per-Project Settings and AI PR/Commit Generation Tools - #64
Merged
Merged
Conversation
…, push, log, diff, stash)
- Introduce `GitView` component for visualizing and managing git status (staged, unstaged, untracked). - Add `git` tab to `AgentView` to provide direct access to repository management. - Update `registerGitHandlers` and Electron preload to support status tracking for untracked files. - Integrate AI-powered commit message generation via the `ai:generateCommitMessage` IPC channel. - Enable viewing commit logs and basic repository interactions within the agent interface.
- Added `diffFile` IPC handler to the electron preload script for fetching detailed per-file diff statistics and content. - Updated `GitView` component to support expanding file rows to reveal inline diff content. - Implemented `handleToggleFile` to fetch and cache diff data on demand, including loading states and visual indicators for additions and deletions. - Refactored `FileRow` to support expansion, loading spinner, and diff statistics display.
Added the ability to create pull requests directly from the GitView component. - Exposed a `createPr` bridge method in the electron preload. - Implemented a new UI form in `GitView` to input PR title and description. - Integrated error handling and success feedback with a link to open the newly created PR. - conditionally show "Create PR" button when the branch is ahead of the remote.
- Updated `GitStatus` interface in preload and GitView to include `hasUpstream` and `defaultBranch` information. - Relocated the "Push" button to the header area to sit alongside other git action indicators. - Added logic to show the "Push" button only when there are ahead commits. - Added conditional logic to show the PR form button only when the branch has a set upstream and is not the default branch. - Removed the redundant Push button from the commit action footer.
- Handle untracked files in `git:diffFile` by falling back to `--no-index` against `/dev/null` if `git diff HEAD` returns no changes. - Reset file diff cache and collapse all expanded files when staging, unstaging, or committing to ensure UI consistency. - Update `handleToggleFile` to correctly fetch and cache diffs upon expansion. - Improve PR creation flow by adding an automatic push before PR submission and updating the UI logic for push/PR buttons. - Export `UnifiedFile` component for external use.
Update the system prompt in `tools.ts` to better guide the AI on managing context IDs. - Clarify that workspace, project, and column IDs are stable during the session. - Specify that `get_active_context` should be called once at the start of a session or after a project change. - Remove the redundant instruction to call `get_active_context` before every write operation to streamline tool usage.
- Added extensive diagnostic logging to git status and diff operations in the electron IPC handler to track file states and command outputs. - Updated `GitView` to distinguish file tracking in the UI using a unique key (`section:path`) to prevent states from clobbering when a file is both staged and unstaged. - Implemented auto-expansion for the "Staged" section when no unstaged or untracked changes are present. - Improved error handling and logging for git diff operations in the UI.
- Remove excessive `console.log` statements throughout the electron IPC handlers and GitView component. - Switch from `trim()` to `trimEnd()` when processing git output to preserve leading indentation, which is often important for diff formatting. - Clean up git status parsing logic. - Simplify error handling and remove unused debug data from git handler return objects.
Update the IPC git status handler to correctly parse file paths returned by `git status --porcelain`. This change adds support for: 1. Extracting the destination path from staged rename entries (e.g., "old -> new"). 2. Stripping wrapping double quotes which Git uses when files contain special characters or spaces.
- Added `ai:generatePrDescription` IPC handler to automate PR title and markdown description generation based on a git diff. - Introduced `git:diffBranch` IPC handler to retrieve the diff between the current branch and a base branch (defaulting to `origin/main`). - Updated `GitView` component with a "Generate Description" button that fetches the branch diff and uses the AI service to suggest PR content, populating the existing PR title and body fields.
|
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:
📝 WalkthroughWalkthroughThe PR adds persisted project settings, a project settings screen, Git IPC and UI support for repository actions and AI-assisted text generation, and folder fields plus updated active-context prompt guidance. ChangesProject settings storage
Git workspace and AI actions
Context metadata and session instructions
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Add eslint-disable-next-line directives to suppress state updates triggered within useEffect hooks in GitView.tsx. This clarifies that the initial refresh and automatic UI section expansion are intentional behaviors for the component lifecycle.
- Introduce `git:prStatus` IPC handler to fetch current branch PR details via the `gh` CLI. - Update `preload.ts` to expose `prStatus` to the renderer. - Update `GitView.tsx` to automatically fetch and display the PR status if a PR exists for the current branch. - Add a "View PR" button to the git UI, allowing users to quickly open existing PRs in their browser. - Ensure PR status is refreshed after push and PR creation operations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
electron/ipc/ai-handlers.ts (1)
73-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated config-resolution logic into a shared helper.
Lines 73-93 and 117-137 are near-identical, and both duplicate the resolution block in
ai:generatePrd(lines 40-60). The only difference isagentConfigvsaiConfig. Consolidating into one helper (returning the resolved{ baseUrl, model, apiKey, isLocal }or anerr) reduces drift risk when defaults like"gpt-4o-mini"or"https://api.openai.com"change.♻️ Sketch of a shared resolver
function resolveLlmConfig( reqConfig: { baseUrl?: string; model?: string; apiKey?: string } | undefined, cacheKey: "aiConfig" | "agentConfig", ): { ok: true; config: LLMConfig } | { ok: false; error: { error: string } } { let cfg = reqConfig; if (!cfg?.apiKey) { const cached = getCachedConfig()[cacheKey]; if (cached?.apiKey) { cfg = { ...cfg, baseUrl: cfg?.baseUrl || cached.baseUrl || "", model: cfg?.model || cached.model || "", apiKey: cached.apiKey, }; } } const baseUrl = normaliseBaseUrl(cfg?.baseUrl || "https://api.openai.com"); const model = cfg?.model || "gpt-4o-mini"; const apiKey = cfg?.apiKey || ""; if (!apiKey && !isLocalEndpoint(baseUrl)) { return { ok: false, error: err("AI is not configured. Add an API key in Settings.") }; } return { ok: true, config: { baseUrl, model, apiKey } }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/ai-handlers.ts` around lines 73 - 93, The config-resolution logic in the AI IPC handlers is duplicated across the ai:generatePrd path and the nearby handler using reqConfig, getCachedConfig(), normaliseBaseUrl(), and isLocalEndpoint(). Extract this into a shared helper (for example a resolver that returns the resolved baseUrl, model, apiKey, and isLocal or an err result) and have both handlers call it with the appropriate cache key (agentConfig vs aiConfig). Keep the shared default values and API-key validation in one place so changes to the fallback model or base URL only need to be made once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/git.ts`:
- Around line 222-244: The stat calculation in the git diff path is using the
HEAD-based numstat result even when the file is untracked, so the returned
added/deleted counts stay at zero while the actual diff is generated from the
untracked-file fallback. Update the logic in the git diff handling around the
statSafe/diff computation to derive stats from the same source as the final
diff, using the untracked-file diff output when the HEAD diff is empty, so the
returned stat object matches the diff contents for untracked files.
- Around line 298-305: The git:stash IPC handler only relies on the TypeScript
union in registerIpcHandle, which does not protect the runtime boundary. In the
git.ts handler, validate the incoming action value against the explicit
allowlist used by the stash flow ("push", "pop", and "list") before calling
git(["stash", action], cwd) or git(["stash", "list"], cwd), and reject any other
payload early inside the handle callback.
- Around line 90-94: In the git status logic inside the `gitSafe`/`rev-list`
handling, the `ahead` and `behind` values are assigned in the wrong order for
`branch@{upstream}...HEAD`; `--left-right` reports the left side first, so swap
the destructuring in the `electron/ipc/git.ts` flow where `aheadBehind.stdout`
is parsed and returned. Keep the rest of the return shape in the same
`defaultBranch`/`hasUpstream`/`staged` logic, but ensure the parsed values map
correctly to `behind` first and `ahead` second before building the result
object.
- Around line 15-29: Update assertWithinCodeDirectory in git.ts to authorize
against real filesystem locations instead of only path.resolve results. Resolve
both cwd and each registered code_directory with fs.realpathSync (or
realpathSync.native) before the equality/startsWith check, then compare those
canonical paths so symlinked directories cannot bypass the containment check.
- Around line 32-50: The git helper functions in git and gitSafe currently use
synchronous subprocess execution, which can block the Electron main process
during IPC operations. Refactor the shared command-running path to use async
process execution with spawn or execFile, then update the handlers in handle()
to await those helpers instead of calling spawnSync directly. Keep the same
behavior and return shapes for git, gitSafe, and the related
status/commit/push/PR flows while making the operations non-blocking.
- Around line 217-244: The git:diffFile IPC handler in registerIpcHandle should
validate the renderer-supplied filePath before the --no-index fallback is used,
because an empty HEAD diff can currently cause arbitrary absolute or ../ paths
to be diffed against /dev/null. Add a path guard in the handle() flow to reject
paths outside the repo/code directory before calling gitSafe for either diff
path, and keep the path separator -- before file operands in all git invocations
within this handler.
In `@src/components/agent/AgentView.tsx`:
- Around line 285-290: The Git mobile branch in AgentView is unreachable because
the mobile tab selector and center-pane visibility logic never expose a Git
option, so the mobileTab === "git" gating on the GitView wrapper is dead code.
Update the mobile tab UI and the mobile center container logic in AgentView so
Git can actually be selected on mobile, or remove the mobileTab "git" handling
from the GitView and related tab state if Git is meant to remain desktop-only.
In `@src/components/agent/GitView.tsx`:
- Around line 345-368: The button styles in GitView use the raw Tailwind color
token text-white on the Push, View PR, and Create PR actions, which violates the
CSS-variable-only color guideline. Update the hover text color in this button
group to use the project’s on-accent CSS variable token instead, and keep the
change localized to the GitView button markup so the existing accent styling
remains intact.
- Around line 697-704: The stage/unstage button in GitView uses broken alpha
concatenation for its background color, so the tint is ignored when actionColor
is a CSS variable. Update the button styling in the relevant JSX block in
GitView to use a proper alpha-safe CSS token mix instead of `${actionColor}15`,
following the project guideline to use color-mix(in srgb, var(--token) X%,
transparent) for translucent variants. Keep the existing actionColor-driven
styling for the text/icon and only replace the backgroundColor expression.
- Around line 716-721: The status badge in GitView uses an invalid alpha suffix
on the value returned by statusColor(status), which can produce a broken
backgroundColor when the color is a CSS variable. Update the span styling in
GitView to generate the tinted background through color-mix(in srgb,
var(--token) X%, transparent) instead of concatenating "10" onto the color
string, while keeping statusColor(status) for the foreground color.
In `@src/components/settings/ProjectSettings.tsx`:
- Around line 40-41: “Reset to defaults” and cleared fields are not persisted
because ProjectSettings.save/reset and the field update paths only send shallow
patches, which cannot remove keys. Update ProjectSettings.tsx so `reset()`,
`save()`, and the `update(field, undefined)` flow in `update`/`save` send an
explicit full settings replacement (or explicit null/sentinel values) instead of
relying on `{ ...existing, ...patch }`, and make sure the optimistic
`updateProject(...)` state matches the exact payload sent through
`window.electron?.project.updateSettings`.
- Around line 18-22: The form state in ProjectSettings is initialized from
activeProject.projectSettings only once, so it can become stale when the active
project changes. Update the ProjectSettings flow to resync settings whenever
activeProject.id changes, either by remounting the component with a key tied to
the active project in SettingsView or by adding an effect in ProjectSettings
that reinitializes the settings state from the current activeProject. Ensure
save() always uses the current project’s settings rather than the previously
edited one.
---
Nitpick comments:
In `@electron/ipc/ai-handlers.ts`:
- Around line 73-93: The config-resolution logic in the AI IPC handlers is
duplicated across the ai:generatePrd path and the nearby handler using
reqConfig, getCachedConfig(), normaliseBaseUrl(), and isLocalEndpoint(). Extract
this into a shared helper (for example a resolver that returns the resolved
baseUrl, model, apiKey, and isLocal or an err result) and have both handlers
call it with the appropriate cache key (agentConfig vs aiConfig). Keep the
shared default values and API-key validation in one place so changes to the
fallback model or base URL only need to be made once.
🪄 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 Plus
Run ID: 0990727b-acdd-41d2-b648-d4f50261c744
📒 Files selected for processing (20)
electron/db/queries.tselectron/db/schema.tselectron/ipc/ai-handlers.tselectron/ipc/chat-executor.tselectron/ipc/db-handlers.tselectron/ipc/git.tselectron/lib/pi-agent-prompt.tselectron/lib/tools.tselectron/main.tselectron/mcp/tools/notes.tselectron/preload.tselectron/shared/db-mappers.tselectron/shared/read-tools-pure.tssrc/components/agent/AgentView.tsxsrc/components/agent/DiffFile.tsxsrc/components/agent/GitView.tsxsrc/components/settings/ProjectSettings.tsxsrc/components/settings/settings-view.tsxsrc/store/slices/workspace.tssrc/types/index.ts
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
… validation - Introduce `resolveConfig` helper in `ai-handlers.ts` to centralize LLM configuration resolution and validation, eliminating duplicated logic across IPC handlers. - Refactor `ai:generatePrd`, `ai:generateCommitMessage`, and `ai:generatePRDescription` to utilize the new shared `resolveConfig` function. - Update `assertWithinCodeDirectory` in `git.ts` to use `fs.realpathSync` for more reliable path comparison against configured project directories. - Switch from `spawnSync` to `execFile` in the Git IPC module for improved process handling.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/git.ts (1)
83-118: 🎯 Functional Correctness | 🟠 MajorParse
git statuswith-z.--porcelainquotes special pathnames, so stripping the outer quotes still leaves escaped sequences like\"and\303\244in the returned paths. Those paths can fail later stage/diff calls. Switch togit status --porcelain=v1 -zand parse the NUL-delimited records, including rename pairs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/git.ts` around lines 83 - 118, The git status parsing in git:status still relies on quoted porcelain output, so special pathnames remain escaped and can break later git operations. Update the parsing logic in the git:status handler to use git status --porcelain=v1 -z and consume NUL-delimited records instead of splitting on newlines and stripping quotes. Be sure to handle rename entries correctly when building the staged, unstaged, and untracked path lists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/ai-handlers.ts`:
- Around line 36-39: The local-endpoint check in isLocalEndpoint is too
permissive because it relies on substring matching, which can incorrectly treat
hostnames like localhost.example.com as local and bypass the API-key gate in the
ai-handlers flow. Update the shared helper to parse the URL and compare the
hostname exactly against the allowed local hosts such as localhost, 127.0.0.1,
0.0.0.0, and optionally ::1, then keep the existing api-key enforcement in the
handler using that stricter result.
- Around line 21-30: The cached endpoint settings are only merged in the
ai-handlers request path when cached.apiKey exists, which skips
local/OpenAI-compatible configs that do not need a key. Update the logic in the
request config merge block to always apply cached baseUrl and model from
getCachedConfig() for the relevant cacheKey, then separately validate whether
the resolved endpoint actually requires an apiKey before falling back to the
default OpenAI URL or “AI is not configured.”
In `@electron/ipc/git.ts`:
- Around line 205-208: The git:log IPC handler currently trusts the runtime
count value, so a large or non-finite request can generate excessive output.
Update the registerIpcHandle("git:log", ...) handler in git.ts to validate and
clamp count at the IPC boundary before it reaches git(["log", ...]); keep the
default behavior but force the value into a small safe range so handle and git
log invocation only ever use bounded --max-count values.
- Around line 52-79: The git helper functions git() and gitSafe() currently rely
on execFile defaults, so large outputs from git diff/git log can hit the buffer
limit and fail or truncate. Update both helpers to pass an explicit higher
maxBuffer in the execFile options, and in git() detect buffer-related failures
and reject with a clear “diff too large” style error instead of a generic git
failure. Keep the changes centered on the git() and gitSafe() helpers so all
callers inherit the new behavior.
- Around line 151-158: The git staging flow in registerIpcHandle("git:stage")
only validates cwd, so the files pathspecs can still escape the authorized
directory; update the staging logic to reject any files entries containing
traversal or absolute paths before calling git(["add", ...]) and make the
all/autoStage path operate on the current directory scope by using a
cwd-restricted pathspec such as "." rather than broad add/reset targets. Keep
the fix centered in the git:stage handler and the related git add/reset helpers
so all staging operations remain confined to the approved code directory.
---
Outside diff comments:
In `@electron/ipc/git.ts`:
- Around line 83-118: The git status parsing in git:status still relies on
quoted porcelain output, so special pathnames remain escaped and can break later
git operations. Update the parsing logic in the git:status handler to use git
status --porcelain=v1 -z and consume NUL-delimited records instead of splitting
on newlines and stripping quotes. Be sure to handle rename entries correctly
when building the staged, unstaged, and untracked path lists.
🪄 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 Plus
Run ID: 5d7e766f-2675-4447-9cb0-19b2ba735455
📒 Files selected for processing (6)
electron/ipc/ai-handlers.tselectron/ipc/git.tssrc/components/agent/AgentView.tsxsrc/components/agent/GitView.tsxsrc/components/settings/ProjectSettings.tsxsrc/components/settings/settings-view.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/settings/settings-view.tsx
- src/components/settings/ProjectSettings.tsx
- src/components/agent/AgentView.tsx
- src/components/agent/GitView.tsx
- Simplify `resolveConfig` logic in `ai-handlers.ts` to explicitly merge cached configuration values. - Increase `maxBuffer` for git operations to 20MB to handle large command outputs, with added handling for buffer overflow errors. - Robustly parse git status using `-z` and NUL-delimited output to correctly handle renamed files and special characters. - Implement path validation for git add/reset operations to prevent directory traversal attacks. - Use `--` delimiters for git command arguments to prevent ambiguity with pathspecs. - Clamp the `git:log` count to prevent excessive resource usage.
Owner
Author
|
@coderabbitai what do you think of this implementation did we miss anything obvious |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== outlines =="
ast-grep outline electron/ipc/git.ts --view expanded || true
ast-grep outline electron/ipc/ai-handlers.ts --view expanded || true
ast-grep outline src/components/settings/ProjectSettings.tsx --view expanded || true
ast-grep outline electron/db/queries.ts --view expanded || true
ast-grep outline electron/shared/db-mappers.ts --view expanded || true
ast-grep outline electron/preload.ts --view expanded || true
ast-grep outline src/types/index.ts --view expanded || true
echo
echo "== electron/ipc/ai-handlers.ts =="
sed -n '1,260p' electron/ipc/ai-handlers.ts
echo
echo "== electron/ipc/git.ts (1) =="
sed -n '1,260p' electron/ipc/git.ts
echo
echo "== electron/ipc/git.ts (2) =="
sed -n '261,520p' electron/ipc/git.ts
echo
echo "== src/components/settings/ProjectSettings.tsx =="
sed -n '1,260p' src/components/settings/ProjectSettings.tsx
echo
echo "== electron/db/queries.ts relevant =="
sed -n '1,240p' electron/db/queries.ts
echo
echo "== electron/shared/db-mappers.ts =="
sed -n '1,220p' electron/shared/db-mappers.ts
echo
echo "== electron/preload.ts relevant =="
sed -n '1,260p' electron/preload.ts
echo
echo "== src/types/index.ts =="
sed -n '1,220p' src/types/index.ts
echo
echo "== search for projectSettings usages =="
rg -n -C2 '\bprojectSettings\b|\bupdateProjectSettings\b|\bupdateSettings\b' electron srcLength of output: 91470
The one thing that does look like an obvious bug to me is the project settings reset/boolean flow:
That means “Reset to defaults” doesn’t really clear settings cleanly, and I’d probably fix that by:
Two smaller follow-ups I’d keep in mind:
So: nothing glaringly unsafe jumped out at me, but I would definitely tighten the |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/git.ts (1)
165-166: 🗄️ Data Integrity & Integration | 🟠 MajorReject option-like branch names before checkout.
execFileavoids shell injection, butgit checkoutstill treats values like--forceas flags. Validatebranchwithgit check-ref-format --branchand reject names starting with-before calling Git.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/git.ts` around lines 165 - 166, The checkout flow in git.ts still allows option-like branch names to be passed into git checkout, so update the branch validation in the checkout path before the git(args, cwd) call. In the branch-handling logic around create ? ["checkout", "-b", branch] : ["checkout", branch], reject names that start with "-" and validate branch using git check-ref-format --branch before invoking Git, so only safe branch refs reach the checkout command.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/git.ts`:
- Around line 120-124: The git status parser in git.ts is using the wrong path
for rename/copy records: for R/C entries, the destination path is already in
entry.slice(3), while the next NUL-separated field is the source path. Update
the parsing logic in the git status handling path so the record keeps the
destination path instead of overwriting filePath with the following field, and
verify the R/C branch in the parser uses the correct field ordering.
- Around line 64-69: The maxBuffer error handling in git command execution is
matching the wrong Node error code, so oversized diffs can be missed. Update the
error check in the git IPC logic around the maxBuffer branch to use the actual
Node code ERR_CHILD_PROCESS_STDIO_MAXBUFFER and keep a broader fallback for
related buffer-exceeded messages; make sure the rejection path in the git
command handler still surfaces the same user-facing oversized diff error.
- Around line 52-56: The path validation in isSafePathspec only blocks absolute
paths and parent traversal, so Git pathspec magic like :(top)file or :/file
still slips through and can escape the intended repo-relative restriction.
Tighten isSafePathspec to reject any file argument starting with Git pathspec
magic syntax, and make sure git:diffFile applies the same validation to its
filePath input before passing it to Git.
---
Outside diff comments:
In `@electron/ipc/git.ts`:
- Around line 165-166: The checkout flow in git.ts still allows option-like
branch names to be passed into git checkout, so update the branch validation in
the checkout path before the git(args, cwd) call. In the branch-handling logic
around create ? ["checkout", "-b", branch] : ["checkout", branch], reject names
that start with "-" and validate branch using git check-ref-format --branch
before invoking Git, so only safe branch refs reach the checkout command.
🪄 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 Plus
Run ID: 4f382bd6-831e-46a0-bf7d-2adbf99af486
📒 Files selected for processing (3)
electron/ipc/ai-handlers.tselectron/ipc/git.tselectron/lib/llm.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/ipc/ai-handlers.ts
- Block pathspecs starting with ":" in `isSafePathspec` to prevent command injection. - Update `ERR_CHILD_PROCESS_STDIO_MAXBUFFER` error code handling to ensure compatibility with Node.js buffer limits. - Fix logic error in `registerGitHandlers` when parsing rename/copy status codes by correctly skipping the source path rather than overwriting the target path. - Add branch name validation in `git:checkout` using `git check-ref-format` to prevent malicious command execution through crafted branch names. - Enforce `isSafePathspec` checks in `git:diffFile` to prevent directory traversal attacks.
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)
electron/ipc/git.ts (1)
184-189: 🗄️ Data Integrity & Integration | 🟠 MajorTreat per-file Git inputs as literal pathspecs
isSafePathspec()blocks traversal and:magic, butgit add/reset/diffstill interpret*,?, and[]in filenames as patterns. That can stage, reset, or diff the wrong file for names likesrc/[id].tsx. Use Git’s literal pathspec mode for these per-file calls.Proposed fix
- await git(["add", "--", ...files], cwd); + await git(["--literal-pathspecs", "add", "--", ...files], cwd); @@ - await git(["reset", "HEAD", "--", ...files], cwd); + await git(["--literal-pathspecs", "reset", "HEAD", "--", ...files], cwd); @@ - statStdout = (await gitSafe(["diff", "--cached", "--numstat", "--", filePath], cwd)).stdout; - diff = (await gitSafe(["diff", "--cached", "--unified=10", "--", filePath], cwd)).stdout; + statStdout = (await gitSafe(["--literal-pathspecs", "diff", "--cached", "--numstat", "--", filePath], cwd)).stdout; + diff = (await gitSafe(["--literal-pathspecs", "diff", "--cached", "--unified=10", "--", filePath], cwd)).stdout; @@ - const r = await gitSafe(["diff", "HEAD", "--unified=10", "--", filePath], cwd); + const r = await gitSafe(["--literal-pathspecs", "diff", "HEAD", "--unified=10", "--", filePath], cwd); @@ - statStdout = (await gitSafe(["diff", "HEAD", "--numstat", "--", filePath], cwd)).stdout; + statStdout = (await gitSafe(["--literal-pathspecs", "diff", "HEAD", "--numstat", "--", filePath], cwd)).stdout; @@ - const untracked = await gitSafe(["diff", "--no-index", "--unified=10", "--", "/dev/null", filePath], cwd); + const untracked = await gitSafe(["--literal-pathspecs", "diff", "--no-index", "--unified=10", "--", "/dev/null", filePath], cwd); @@ - statStdout = (await gitSafe(["diff", "--no-index", "--numstat", "--", "/dev/null", filePath], cwd)).stdout; + statStdout = (await gitSafe(["--literal-pathspecs", "diff", "--no-index", "--numstat", "--", "/dev/null", filePath], cwd)).stdout;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/git.ts` around lines 184 - 189, The per-file Git operations in git.ts are still treating file names as pattern pathspecs, so names containing wildcard characters can match unintended files. Update the git add/reset/diff call sites to use Git’s literal pathspec mode for these per-file inputs, and keep the existing isSafePathspec() validation as a separate safety check. Use the git() helper and the per-file loops around it as the main places to adjust so the filenames are passed as literal pathspecs rather than interpreted patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@electron/ipc/git.ts`:
- Around line 184-189: The per-file Git operations in git.ts are still treating
file names as pattern pathspecs, so names containing wildcard characters can
match unintended files. Update the git add/reset/diff call sites to use Git’s
literal pathspec mode for these per-file inputs, and keep the existing
isSafePathspec() validation as a separate safety check. Use the git() helper and
the per-file loops around it as the main places to adjust so the filenames are
passed as literal pathspecs rather than interpreted patterns.
Owner
Author
|
@coderabbitai I have addressed ideas and follow ups. |
|
Tip For best results, initiate chat on the files or code changes.
|
Owner
Author
|
@coderabbitai please re-review. Made a few more changes and some fixes. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/git.ts (1)
277-280: 🔒 Security & Privacy | 🟠 MajorValidate
baseBranchbefore building either diff revspec.
A crafted value like--output=/tmp/xcan still reach the fallback as the first revision argument and be parsed as a Git option after theorigin/lookup fails. Reuse the checkout branch validation here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/git.ts` around lines 277 - 280, Validate baseBranch before constructing either git diff revspec in gitSafe usage within the diff fallback flow, since a crafted value can still be interpreted as an option when origin/ lookup fails. Reuse the existing checkout branch validation used elsewhere in electron/ipc/git.ts before calling gitSafe for both the initial diff and the fallback diff, and ensure only approved branch names are interpolated into the revision arguments.Source: Linters/SAST tools
♻️ Duplicate comments (1)
src/components/agent/ProjectSettingsModal.tsx (1)
24-24: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKey the section by active project to avoid cross-project saves.
ProjectSettingsSectionstill initializes its local form state from the current project only once, so rendering it here without a project-tied key reintroduces the stale-form bug on the modal path. IfactiveProjectIdchanges while this dialog stays open, the form can keep the previous project's values andsave()will write them to the newly active project instead.Suggested direction
+import { useCairnStore } from "`@/store`"; +import { useShallow } from "zustand/react/shallow"; + export function ProjectSettingsModal({ open, onClose }: ProjectSettingsModalProps) { + const { activeProjectId } = useCairnStore(useShallow((s) => ({ activeProjectId: s.activeProjectId }))); return ( <Dialog open={open} onOpenChange={(v) => { if (!v) onClose(); }}> <DialogContent size="lg" className="overflow-y-auto max-h-[85vh] p-5"> @@ </DialogHeader> <div className="pt-4"> - <ProjectSettingsSection showHeader={false} /> + <ProjectSettingsSection key={activeProjectId ?? "no-project"} showHeader={false} /> </div> </DialogContent> </Dialog> ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/agent/ProjectSettingsModal.tsx` at line 24, `ProjectSettingsSection` in `ProjectSettingsModal` is mounted without being tied to the current project, so its local form state can persist across `activeProjectId` changes. Update the `ProjectSettingsSection` usage to include a project-specific key based on the active project identifier so the section remounts when the project changes. This should be done at the `ProjectSettingsModal` render site and should use the active project symbol that drives the modal state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/ai-handlers.ts`:
- Around line 40-44: The cleanOutput helper in ai-handlers.ts only strips fences
when the response starts with a code block, so replies with leading prose can
leave the closing fence behind and break the parsed secondary field. Update
cleanOutput to detect and extract the first fenced block anywhere in the text,
and only fall back to the existing startsWith("```") path if no fenced block is
found. Make sure the regex handling covers both SUBJECT/BODY and
TITLE/DESCRIPTION style responses so the commit body and PR description are
fully cleaned.
In `@src/components/agent/AgentView.tsx`:
- Around line 203-209: The icon-only settings buttons in AgentView need an
explicit accessible name because relying on title is insufficient for screen
readers. Update both button instances in AgentView so the button that opens
project settings includes an aria-label with the same “Project Settings” text,
keeping the existing title if desired; make the same change for the second
settings button near the other occurrence in the component.
In `@src/components/graph/MatrixCanvas.tsx`:
- Around line 99-102: The stale cell-index issue in MatrixCanvas causes pinned
or hovered selections to point at the wrong tag pair after activeTags changes.
Update the selection state in MatrixCanvas so pinnedCell and hoveredCell are
cleared when activeTags is filtered/reordered, or refactor the selection to
store tag IDs instead of r/c indices. Make sure the panelItems lookup always
uses the current tags from activeTags and never reuses outdated row/column
positions.
- Around line 232-234: The dense-layout value hiding in MatrixCanvas should not
rely only on hover, since that blocks touch and keyboard users from seeing
counts and leaves diagonal cells inaccessible. Update the showValue logic in the
cell rendering path to also reveal values for the pinned/focused cell and/or
when the pointer is non-hover, and apply the same accessibility-friendly rule in
the related dense-layout rendering blocks. Use the existing MatrixCanvas cell
state and hover/pin/focus symbols to keep the behavior consistent across those
sections.
In `@src/components/notes/NoteMarkdownPreview.tsx`:
- Line 68: The path parsing in NoteMarkdownPreview’s image handling uses
decodeURIComponent on cleanSrc, which can throw for literal percent signs or
malformed escapes and break preview rendering. Update the decoding logic in the
affected code paths (including the matching spot near the same relParts usage)
to decode safely with a guard or try/catch, and fall back to the original raw
path when decoding fails.
- Around line 52-55: The src/components/notes/NoteMarkdownPreview.tsx image
source handling in the resolution logic currently treats file: URLs as safe and
assigns them directly, bypassing the IPC-backed local file read path. Update the
srcset/src resolution flow in the NoteMarkdownPreview component so file: sources
are either rejected or parsed and routed through the same agent:readFileBase64
path used for local files, preserving the assertWithinCodeDirectory containment
check.
- Around line 100-109: Reset and cancel in-flight image loading in
NoteMarkdownPreview when src, filePath, or projectRoot changes. Update the
effect that calls window.electron.agent.readFileBase64 so stale promises cannot
overwrite newer state, and clear resolvedSrc before starting each new local read
so old images do not remain visible. Use the existing state/effect logic around
setResolvedSrc, setError, and the readFileBase64 promise chain to locate the
fix.
- Line 118: The Tailwind arbitrary color classes in NoteMarkdownPreview are
using unescaped whitespace, which breaks parsing in v4. Update the span’s border
and background utility values to the underscore form for the color-mix
expressions so Tailwind treats them as single classes, and keep the rest of the
inline-flex styling unchanged.
In `@src/components/settings/ProjectSettings.tsx`:
- Around line 31-42: The ProjectSettings useEffect that probes
.github/PULL_REQUEST_TEMPLATE.md can apply a stale readFile() result after
activeProject changes, so update the effect to ignore outdated async
completions. Use the activeProject-dependent effect around
window.electron.agent.readFile(templatePath) to capture the current project
identity, and before calling setRepoTemplateExists verify the result still
belongs to the latest activeProject; otherwise discard it. Keep the existing
fallback in the else branch, and make sure the repoTemplateExists state only
reflects the current project so the banner and useRepoPrTemplate toggle stay in
sync.
In `@src/store/slices/notes.ts`:
- Around line 212-223: The generatePrd flow in notes.ts is swallowing the
successful IPC result because the awaited window.electron.ai.generatePrd call is
not returned. Update the generatePrd action so it forwards the promise result
from the generatePrd invocation on success, while keeping the existing try/catch
behavior in place for errors.
---
Outside diff comments:
In `@electron/ipc/git.ts`:
- Around line 277-280: Validate baseBranch before constructing either git diff
revspec in gitSafe usage within the diff fallback flow, since a crafted value
can still be interpreted as an option when origin/ lookup fails. Reuse the
existing checkout branch validation used elsewhere in electron/ipc/git.ts before
calling gitSafe for both the initial diff and the fallback diff, and ensure only
approved branch names are interpolated into the revision arguments.
---
Duplicate comments:
In `@src/components/agent/ProjectSettingsModal.tsx`:
- Line 24: `ProjectSettingsSection` in `ProjectSettingsModal` is mounted without
being tied to the current project, so its local form state can persist across
`activeProjectId` changes. Update the `ProjectSettingsSection` usage to include
a project-specific key based on the active project identifier so the section
remounts when the project changes. This should be done at the
`ProjectSettingsModal` render site and should use the active project symbol that
drives the modal state.
🪄 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 Plus
Run ID: bf0df6ae-c545-4f05-871c-8cda79ffd72b
📒 Files selected for processing (15)
changelogs/v2.2.5.mdelectron/db/queries.tselectron/ipc/ai-handlers.tselectron/ipc/git.tselectron/preload.tssrc/components/agent/AgentEditor.tsxsrc/components/agent/AgentView.tsxsrc/components/agent/GitView.tsxsrc/components/agent/ProjectSettingsModal.tsxsrc/components/graph/MatrixCanvas.tsxsrc/components/notes/NoteMarkdownPreview.tsxsrc/components/settings/ProjectSettings.tsxsrc/components/settings/settings-view.tsxsrc/store/slices/notes.tssrc/types/index.ts
✅ Files skipped from review due to trivial changes (1)
- changelogs/v2.2.5.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/types/index.ts
- src/components/agent/GitView.tsx
- electron/db/queries.ts
- electron/preload.ts
- ai-handlers: Add regex to extract fenced code blocks containing specific commit/PR instruction keywords. - git: Validate base branch names before git diff operations for improved security. - AgentView: Apply missing `aria-label` to project settings buttons for better accessibility. - ProjectSettingsModal: Add key prop to re-render settings sections on project switch. - MatrixCanvas: Clear selection on tag changes, enable clicking diagonal cells, and improve value visibility logic. - NoteMarkdownPreview: Add URI decoding safety to image path resolution and fix race conditions in image loading effects.
Removed redundant `eslint-disable-next-line react-hooks/set-state-in-effect` comments across `MatrixCanvas` and `NoteMarkdownPreview` components. These suppressions were no longer serving a purpose, and cleaning them up improves code readability and maintainability.
Prevent the push button from being interactable when the local branch is up-to-date with the remote (i.e., when 'ahead' is 0). This improves UX by avoiding unnecessary Git operations when there is nothing to push.
Owner
Author
|
@coderabbitai what do you think are we good to merge? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== changed files =="
git diff --name-only origin/HEAD...HEAD || true
echo
echo "== outlines =="
ast-grep outline src/components/settings/ProjectSettings.tsx --view expanded || true
ast-grep outline electron/db/queries.ts --view expanded || true
ast-grep outline electron/ipc/ai-handlers.ts --view expanded || true
ast-grep outline electron/preload.ts --view expanded || true
ast-grep outline src/store/slices/notes.ts --view expanded || true
ast-grep outline electron/ipc/git.ts --view expanded || true
echo
echo "== src/components/settings/ProjectSettings.tsx =="
sed -n '1,260p' src/components/settings/ProjectSettings.tsx
echo
echo "== electron/db/queries.ts =="
sed -n '1,260p' electron/db/queries.ts
echo
echo "== electron/ipc/ai-handlers.ts =="
sed -n '1,320p' electron/ipc/ai-handlers.ts
echo
echo "== electron/preload.ts =="
sed -n '1,240p' electron/preload.ts
echo
echo "== src/store/slices/notes.ts =="
sed -n '1,220p' src/store/slices/notes.ts
echo
echo "== targeted searches =="
rg -n -C2 'autoStageOnCommit|useRepoPrTemplate|project_settings|generatePrd|parseLLMResponse|cleanOutput|TITLE:|DESCRIPTION:|SUBJECT:|BODY:' electron srcLength of output: 75752 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== changed files =="
git diff --name-only origin/HEAD...HEAD || true
echo
echo "== outlines =="
ast-grep outline src/components/settings/ProjectSettings.tsx --view expanded || true
ast-grep outline electron/db/queries.ts --view expanded || true
ast-grep outline electron/ipc/ai-handlers.ts --view expanded || true
ast-grep outline electron/preload.ts --view expanded || true
ast-grep outline src/store/slices/notes.ts --view expanded || true
ast-grep outline electron/ipc/git.ts --view expanded || true
echo
echo "== src/components/settings/ProjectSettings.tsx =="
sed -n '1,260p' src/components/settings/ProjectSettings.tsx
echo
echo "== electron/db/queries.ts =="
sed -n '1,260p' electron/db/queries.ts
echo
echo "== electron/ipc/ai-handlers.ts =="
sed -n '1,320p' electron/ipc/ai-handlers.ts
echo
echo "== electron/preload.ts =="
sed -n '1,240p' electron/preload.ts
echo
echo "== src/store/slices/notes.ts =="
sed -n '1,220p' src/store/slices/notes.ts
echo
echo "== targeted searches =="
rg -n -C2 'autoStageOnCommit|useRepoPrTemplate|project_settings|generatePrd|parseLLMResponse|cleanOutput|TITLE:|DESCRIPTION:|SUBJECT:|BODY:' electron srcLength of output: 75752
The main things I was worried about earlier now look addressed:
So: I don’t see any obvious merge blockers from this pass, and I’d be comfortable merging it. |
7 of 13 tasks
This was referenced Jul 25, 2026
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 of Changes
This PR introduces a
project_settingsJSON column to theprojectstable and adds new IPC handlers to automate the generation of git commit messages and pull request descriptions using AI.Context/Why
Key Changes
project_settingscolumn (TEXT, defaults to'{}') in theprojectstable.updateProjectSettingshelper inqueries.tsto manage incremental updates to project configurations.ai:generateCommitMessage: Generates conventional commit-compliant messages from diffs using a custom system prompt.ai:generatePrDescription: Generates structured markdown PR descriptions (Summary, Context, Key Changes) from diffs.chat-executor.tsto includefolderinfo when listing recent notes for improved tool output clarity.Summary by CodeRabbit
New Features
Bug Fixes
Other