Conversation
Contributor
📝 WalkthroughWalkthroughThe pull request converts the synchronous 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. ✨ Finishing Touches
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts (1)
6-9: Duplicated path definitions between module constants and getter functions.Module-level constants (
ZSH_PROFILE,ZSH_RC,ZSH_LOGIN,BASH_RCFILE) and exported getters (getZshProfilePath(), etc.) both compute the same paths viapath.join. Consider having the getters return the constants, or vice versa, to eliminate the duplication.♻️ Example: reuse constants in getters
export function getZshProfilePath(): string { - return path.join(ZSH_DIR, ".zprofile"); + return ZSH_PROFILE; } export function getZshRcPath(): string { - return path.join(ZSH_DIR, ".zshrc"); + return ZSH_RC; } export function getZshLoginPath(): string { - return path.join(ZSH_DIR, ".zlogin"); + return ZSH_LOGIN; } export function getBashRcfilePath(): string { - return path.join(BASH_DIR, "rcfile"); + return BASH_RCFILE; }Also applies to: 42-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts` around lines 6 - 9, The module defines path constants (ZSH_PROFILE, ZSH_RC, ZSH_LOGIN, BASH_RCFILE) and duplicates the same path.join logic inside exported getters (getZshProfilePath, getZshRcPath, getZshLoginPath, getBashRcFilePath); remove the duplication by having each getter simply return the corresponding constant (e.g., have getZshProfilePath() return ZSH_PROFILE, getZshRcPath() return ZSH_RC, getZshLoginPath() return ZSH_LOGIN, getBashRcFilePath() return BASH_RCFILE) so path construction happens in one place and update any callers if necessary.apps/desktop/src/main/lib/agent-setup/ensure-agent-hooks.ts (1)
180-180: Synchronous I/O inside async flow.
cleanupGlobalOpenCodePlugin()usesfs.existsSync,fs.readFileSync, andfs.unlinkSync(peragent-wrappers.ts:165-183). This blocks the event loop during an otherwise fully async startup. It's low-risk given the small file size and one-time nature, but consider converting it to async for consistency with the rest of the flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/agent-setup/ensure-agent-hooks.ts` at line 180, cleanupGlobalOpenCodePlugin currently performs synchronous fs operations (existsSync, readFileSync, unlinkSync); convert it to an async function using fs.promises (or async fs methods): replace existsSync with fs.promises.access/stat with try/catch, readFileSync with fs.promises.readFile, and unlinkSync with fs.promises.unlink, preserve the same behavior and error handling, and export/declare it async; then update its caller in ensure-agent-hooks to await cleanupGlobalOpenCodePlugin() (and mark the caller async if needed) so the startup flow remains fully asynchronous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts`:
- Around line 110-129: The tests fail with TypeError because the module
containing getShellEnv and getShellArgs is being mocked or imported incorrectly
so those named exports are replaced with non-callable values; locate the
mock/import for the shell-wrappers module in this test file (or any __mocks__
file) and change it to use the real implementation (e.g., replace the mock with
jest.mock('.../shell-wrappers', () => jest.requireActual('.../shell-wrappers'))
or use jest.unmock('.../shell-wrappers')), and ensure the test imports the named
exports (getShellEnv, getShellArgs) as actual functions from shell-wrappers.ts
rather than via a mocked object so the functions are callable again.
- Around line 26-48: The tests fail because the dynamic await
import("./shell-wrappers") happens before Bun's mock for "./paths" takes effect
(so path getters point at real dirs and throw ENOENT) and because named exports
may not be correctly read from the imported module; fix by moving/adding the
mock.module("./paths", ...) call so it runs before the dynamic import, ensure
the mock returns the same named exports used by shell-wrappers (e.g.
TEST_ZSH_DIR/TEST_BASH_DIR-backed implementations for getZshProfilePath,
getZshRcPath, getZshLoginPath, getBashRcfilePath), then import the module and
destructure its named exports (or reference them off the module object) to avoid
TypeError (e.g. const shell = await import("./shell-wrappers"); const {
getShellEnv, getShellArgs, getBashRcfileContent, getZshProfileContent,
getZshRcContent, getZshLoginContent } = shell), and add a quick diagnostic
console.log of the returned paths from getZshProfilePath()/getBashRcfilePath()
if the failures persist.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/agent-setup/ensure-agent-hooks.ts`:
- Line 180: cleanupGlobalOpenCodePlugin currently performs synchronous fs
operations (existsSync, readFileSync, unlinkSync); convert it to an async
function using fs.promises (or async fs methods): replace existsSync with
fs.promises.access/stat with try/catch, readFileSync with fs.promises.readFile,
and unlinkSync with fs.promises.unlink, preserve the same behavior and error
handling, and export/declare it async; then update its caller in
ensure-agent-hooks to await cleanupGlobalOpenCodePlugin() (and mark the caller
async if needed) so the startup flow remains fully asynchronous.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.ts`:
- Around line 6-9: The module defines path constants (ZSH_PROFILE, ZSH_RC,
ZSH_LOGIN, BASH_RCFILE) and duplicates the same path.join logic inside exported
getters (getZshProfilePath, getZshRcPath, getZshLoginPath, getBashRcFilePath);
remove the duplication by having each getter simply return the corresponding
constant (e.g., have getZshProfilePath() return ZSH_PROFILE, getZshRcPath()
return ZSH_RC, getZshLoginPath() return ZSH_LOGIN, getBashRcFilePath() return
BASH_RCFILE) so path construction happens in one place and update any callers if
necessary.
Comment on lines
+26
to
+48
| const { | ||
| getBashRcfileContent, | ||
| getBashRcfilePath, | ||
| getCommandShellArgs, | ||
| getShellArgs, | ||
| getShellEnv, | ||
| getZshLoginContent, | ||
| getZshLoginPath, | ||
| getZshProfileContent, | ||
| getZshProfilePath, | ||
| getZshRcContent, | ||
| getZshRcPath, | ||
| } = await import("./shell-wrappers"); | ||
|
|
||
| function writeZshWrappers(): void { | ||
| writeFileSync(getZshProfilePath(), getZshProfileContent(), { mode: 0o644 }); | ||
| writeFileSync(getZshRcPath(), getZshRcContent(), { mode: 0o644 }); | ||
| writeFileSync(getZshLoginPath(), getZshLoginContent(), { mode: 0o644 }); | ||
| } | ||
|
|
||
| function writeBashWrapper(): void { | ||
| writeFileSync(getBashRcfilePath(), getBashRcfileContent(), { mode: 0o644 }); | ||
| } |
Contributor
There was a problem hiding this comment.
Tests are failing in CI — ENOENT and missing function errors.
The pipeline shows five failures in this file:
ENOENTatwriteZshWrappers(line 41) from tests at lines 62 and 96ENOENTatwriteBashWrapper(line 47) from test at line 84TypeError: getShellEnv is not a functionat line 111TypeError: getShellArgs is not a functionat line 122
The ENOENT errors suggest that the path getters (getZshProfilePath(), etc.) are not returning the mocked TEST_ZSH_DIR/TEST_BASH_DIR paths, so writeFileSync targets directories that don't exist. The "not a function" errors suggest the dynamic await import("./shell-wrappers") is returning an incomplete module, possibly because the mock.module("./paths", ...) isn't intercepting the import inside shell-wrappers.ts as expected.
You may need to verify that Bun's mock.module properly intercepts transitive dependency imports when using await import(...). As a diagnostic step, try logging the actual paths returned by the getters and the resolved module keys.
#!/bin/bash
# Check if there's a Bun version constraint and how mock.module is used in other test files
rg -n 'mock\.module' --type=ts -g '*.test.*' -C2 | head -80🧰 Tools
🪛 GitHub Check: Test
[failure] 41-41: error: ENOENT: no such file or directory
at writeZshWrappers (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:41:2)
at <anonymous> (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:96:3)
[failure] 47-47: error: ENOENT: no such file or directory
at writeBashWrapper (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:47:2)
at <anonymous> (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:84:3)
[failure] 41-41: error: ENOENT: no such file or directory
at writeZshWrappers (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:41:2)
at <anonymous> (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:62:3)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts` around lines 26
- 48, The tests fail because the dynamic await import("./shell-wrappers")
happens before Bun's mock for "./paths" takes effect (so path getters point at
real dirs and throw ENOENT) and because named exports may not be correctly read
from the imported module; fix by moving/adding the mock.module("./paths", ...)
call so it runs before the dynamic import, ensure the mock returns the same
named exports used by shell-wrappers (e.g. TEST_ZSH_DIR/TEST_BASH_DIR-backed
implementations for getZshProfilePath, getZshRcPath, getZshLoginPath,
getBashRcfilePath), then import the module and destructure its named exports (or
reference them off the module object) to avoid TypeError (e.g. const shell =
await import("./shell-wrappers"); const { getShellEnv, getShellArgs,
getBashRcfileContent, getZshProfileContent, getZshRcContent, getZshLoginContent
} = shell), and add a quick diagnostic console.log of the returned paths from
getZshProfilePath()/getBashRcfilePath() if the failures persist.
Comment on lines
+110
to
+129
| it("only injects zsh wrapper env when wrapper files exist", () => { | ||
| expect(getShellEnv("/bin/zsh")).toEqual({}); | ||
|
|
||
| writeZshWrappers(); | ||
|
|
||
| expect(getShellEnv("/bin/zsh")).toEqual({ | ||
| SUPERSET_ORIG_ZDOTDIR: process.env.ZDOTDIR || homedir(), | ||
| ZDOTDIR: TEST_ZSH_DIR, | ||
| }); | ||
| }); | ||
|
|
||
| it("falls back to login bash when wrapper is missing", () => { | ||
| expect(getShellArgs("/bin/bash")).toEqual(["-l"]); | ||
|
|
||
| writeBashWrapper(); | ||
| expect(getShellArgs("/bin/bash")).toEqual([ | ||
| "--rcfile", | ||
| path.join(TEST_BASH_DIR, "rcfile"), | ||
| ]); | ||
| }); |
Contributor
There was a problem hiding this comment.
New tests for getShellEnv and getShellArgs are failing.
Both getShellEnv and getShellArgs are valid exports from shell-wrappers.ts, but CI reports TypeError: ... is not a function. This likely stems from the same mock/import issue affecting the whole file. Once the root cause of the mock resolution is fixed, these tests should work.
🧰 Tools
🪛 GitHub Check: Test
[failure] 122-122: TypeError: getShellArgs is not a function. (In 'getShellArgs("/bin/bash")'
at <anonymous> (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:122:10)
[failure] 111-111: TypeError: getShellEnv is not a function. (In 'getShellEnv("/bin/zsh")'
at <anonymous> (/home/runner/work/superset/superset/apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts:111:10)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/lib/agent-setup/shell-wrappers.test.ts` around lines
110 - 129, The tests fail with TypeError because the module containing
getShellEnv and getShellArgs is being mocked or imported incorrectly so those
named exports are replaced with non-callable values; locate the mock/import for
the shell-wrappers module in this test file (or any __mocks__ file) and change
it to use the real implementation (e.g., replace the mock with
jest.mock('.../shell-wrappers', () => jest.requireActual('.../shell-wrappers'))
or use jest.unmock('.../shell-wrappers')), and ensure the test imports the named
exports (getShellEnv, getShellArgs) as actual functions from shell-wrappers.ts
rather than via a mocked object so the functions are callable again.
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.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
Performance
Improvements
Tests