Skip to content

Conversation

@zerob13
Copy link
Collaborator

@zerob13 zerob13 commented Nov 23, 2025

Summary by CodeRabbit

  • Chores
    • Improved temporary directory management for operations with dedicated storage location and enhanced error handling with fallback mechanisms and logging.

✏️ Tip: You can customize this high-level summary in your review settings.

@zerob13 zerob13 marked this pull request as ready for review November 23, 2025 13:17
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 23, 2025

Walkthrough

This PR modifies the ACP initialization helper to introduce dedicated temporary directory management. A new private helper function obtains or creates a temporary directory under Electron's userData path, with fallback to process.cwd() if creation fails. The PTY working directory is updated to use this computed temporary directory instead of directly using the process working directory.

Changes

Cohort / File(s) Summary
ACP initialization & temporary directory management
src/main/presenter/configPresenter/acpInitHelper.ts
Adds private getAcpTempDir() helper to manage ACP temporary directory under Electron userData path with fallback error handling; imports app from electron; integrates temp directory into PTY session lifecycle by routing working directory to the new helper function instead of process.cwd(); adjusts spawn and session initialization to use computed workDir.

Sequence Diagram

sequenceDiagram
    participant Init as ACP Initialization
    participant Helper as getAcpTempDir()
    participant FS as Electron userData / FS
    participant PTY as PTY Session
    
    Init->>Helper: Request ACP temp directory
    Helper->>FS: Check/create directory under userData
    alt Directory creation succeeds
        FS-->>Helper: Directory path
    else Creation fails
        FS-->>Helper: Return process.cwd()
    end
    Helper-->>Init: Return workDir
    Init->>PTY: Initialize with workDir
    PTY->>PTY: Set cwd to workDir
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Review the new getAcpTempDir() helper logic for proper error handling and fallback behavior
  • Verify electron app.getPath() integration and imports
  • Confirm PTY session initialization correctly uses the computed workDir across spawn and cwd assignments

Poem

🐰 A cozy warren for ACP to roam,
In userData's safe and sheltered home,
When paths grow tangled or troubles appear,
We hop back to cwd, never fear!
Temp directories tidy, the workspace is clean. 🌰

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: acp init dir no permission' directly addresses the core issue being resolved in the changeset—fixing permission problems with the ACP initialization directory by introducing a dedicated temporary directory management system.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bugfix/acp-init-no-permission

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f22d00 and b9b0fe0.

📒 Files selected for processing (1)
  • src/main/presenter/configPresenter/acpInitHelper.ts (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/presenter/configPresenter/acpInitHelper.ts (1)
test/mocks/electron.ts (2)
  • app (2-10)
  • shell (25-27)
⏰ 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). (5)
  • GitHub Check: build-mac (arm64)
  • GitHub Check: build-mac (x64)
  • GitHub Check: build-linux (x64)
  • GitHub Check: build-windows (x64)
  • GitHub Check: build-check (x64)
🔇 Additional comments (2)
src/main/presenter/configPresenter/acpInitHelper.ts (2)

5-5: LGTM!

The import of app from Electron is necessary to access the userData path for the temporary directory.


344-345: LGTM!

The temporary directory is correctly integrated into the PTY session lifecycle. The working directory is computed once via getAcpTempDir() and consistently used for both logging and as the cwd parameter when spawning the PTY process.

Also applies to: 367-367, 375-375

@zerob13 zerob13 merged commit 9fd63cf into dev Nov 23, 2025
6 checks passed
@zerob13 zerob13 deleted the bugfix/acp-init-no-permission branch November 23, 2025 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants