Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds two browser-safe stub modules ("debug" no-op logger and a placeholder " ChangesBun CJS Interop Stubs
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Bundler as Vite Build
participant Resolver as Bun Package Resolver
participant Alias as resolve.alias
participant Stub as Local Stub Module
participant Bun as node_modules/.bun
Bundler->>Resolver: request CJS package resolution
Resolver->>Bun: locate hoisted package.json entries
Bun-->>Resolver: candidate entry paths
Resolver-->>Bundler: cjsInteropEntries list
Bundler->>Alias: register aliases for debug, `@spacebot/api-client`, cjsInteropEntries
Alias->>Stub: fall back to stub when real package unavailable
Stub-->>Bundler: resolved module for build/optimizeDeps
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
7d67ebe to
ac96f2a
Compare
Add Vite aliases and stubs for Bun-hoisted packages and the optional @spacebot/api-client dependency so tauri:dev starts without resolve errors.
ac96f2a to
5bfad86
Compare
![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
🧹 Nitpick comments (1)
apps/tauri/vite.config.ts (1)
50-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging when CJS interop packages can't be resolved.
The
catchblock silently returns[], making it hard to debug why a package was skipped (e.g.,.bundirectory missing, package not hoisted). Aconsole.warnwould help developers identify which packages weren't aliased without failing the build.♻️ Proposed fix: add warning log
const cjsInteropEntries = CJS_INTEROP_PACKAGES.flatMap((packageName) => { try { return [{packageName, entry: resolveBunEntry(packageName)}]; - } catch { + } catch (e) { + console.warn(`[vite] Could not resolve CJS interop entry for "${packageName}": ${(e as Error).message}`); return []; } });🤖 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 `@apps/tauri/vite.config.ts` around lines 50 - 56, The CJS interop resolution in cjsInteropEntries is swallowing failures silently, so skipped packages are hard to diagnose. Update the try/catch around resolveBunEntry(packageName) to emit a console.warn in the catch path that includes the packageName and a short reason/context before returning []; keep the existing behavior of skipping the alias, but make the warning visible when a package cannot be resolved.
🤖 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 `@apps/tauri/vite.config.ts`:
- Around line 19-36: `resolveBunEntry` currently assumes `pkgJson.browser` is
always a string, so packages with object-valued `browser` mappings can resolve
to invalid paths and get skipped by the `cjsInteropEntries` flow. Update the
`resolveBunEntry` logic to handle the `browser` field defensively by only using
it when it is a string, otherwise fall back to `main` or `index.js`, and keep
the existing candidate resolution/throw behavior in `resolveBunEntry` and its
callers unchanged.
---
Nitpick comments:
In `@apps/tauri/vite.config.ts`:
- Around line 50-56: The CJS interop resolution in cjsInteropEntries is
swallowing failures silently, so skipped packages are hard to diagnose. Update
the try/catch around resolveBunEntry(packageName) to emit a console.warn in the
catch path that includes the packageName and a short reason/context before
returning []; keep the existing behavior of skipping the alias, but make the
warning visible when a package cannot be resolved.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4613635a-3e53-4b59-8ac7-79f5ecedb3e1
📒 Files selected for processing (3)
apps/tauri/src/stubs/debug.tsapps/tauri/src/stubs/spacebot-api-client.tsapps/tauri/vite.config.ts
| function resolveBunEntry(packageName: string): string { | ||
| const pkgDir = resolveBunPackageDir(packageName); | ||
| const pkgJson = JSON.parse( | ||
| fs.readFileSync(path.join(pkgDir, 'package.json'), 'utf8') | ||
| ) as {main?: string; browser?: string}; | ||
| const entry = pkgJson.browser ?? pkgJson.main ?? 'index.js'; | ||
| const candidates = [ | ||
| path.resolve(pkgDir, entry), | ||
| path.resolve(pkgDir, `${entry}.js`), | ||
| path.resolve(pkgDir, entry, 'index.js'), | ||
| ]; | ||
| for (const candidate of candidates) { | ||
| if (fs.existsSync(candidate)) { | ||
| return candidate; | ||
| } | ||
| } | ||
| throw new Error(`Could not resolve entry for ${packageName}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
resolveBunEntry doesn't handle object-valued browser field.
The browser field in package.json can be either a string or an object (e.g., {"./node.js": "./browser.js"}). The as {main?: string; browser?: string} cast assumes it's always a string. If a package has an object browser field, entry would be an object at runtime, path.resolve would stringify it to "[object Object]", all candidates would fail, and the package would be silently skipped by the cjsInteropEntries try/catch — potentially reintroducing the blank-screen issue this PR fixes.
🛡️ Proposed fix: guard against non-string `browser` field
const pkgJson = JSON.parse(
fs.readFileSync(path.join(pkgDir, 'package.json'), 'utf8')
- ) as {main?: string; browser?: string};
- const entry = pkgJson.browser ?? pkgJson.main ?? 'index.js';
+ ) as {main?: string; browser?: string | Record<string, string>};
+ const browserEntry =
+ typeof pkgJson.browser === 'string' ? pkgJson.browser : undefined;
+ const entry = browserEntry ?? pkgJson.main ?? 'index.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function resolveBunEntry(packageName: string): string { | |
| const pkgDir = resolveBunPackageDir(packageName); | |
| const pkgJson = JSON.parse( | |
| fs.readFileSync(path.join(pkgDir, 'package.json'), 'utf8') | |
| ) as {main?: string; browser?: string}; | |
| const entry = pkgJson.browser ?? pkgJson.main ?? 'index.js'; | |
| const candidates = [ | |
| path.resolve(pkgDir, entry), | |
| path.resolve(pkgDir, `${entry}.js`), | |
| path.resolve(pkgDir, entry, 'index.js'), | |
| ]; | |
| for (const candidate of candidates) { | |
| if (fs.existsSync(candidate)) { | |
| return candidate; | |
| } | |
| } | |
| throw new Error(`Could not resolve entry for ${packageName}`); | |
| } | |
| function resolveBunEntry(packageName: string): string { | |
| const pkgDir = resolveBunPackageDir(packageName); | |
| const pkgJson = JSON.parse( | |
| fs.readFileSync(path.join(pkgDir, 'package.json'), 'utf8') | |
| ) as {main?: string; browser?: string | Record<string, string>}; | |
| const browserEntry = | |
| typeof pkgJson.browser === 'string' ? pkgJson.browser : undefined; | |
| const entry = browserEntry ?? pkgJson.main ?? 'index.js'; | |
| const candidates = [ | |
| path.resolve(pkgDir, entry), | |
| path.resolve(pkgDir, `${entry}.js`), | |
| path.resolve(pkgDir, entry, 'index.js'), | |
| ]; | |
| for (const candidate of candidates) { | |
| if (fs.existsSync(candidate)) { | |
| return candidate; | |
| } | |
| } | |
| throw new Error(`Could not resolve entry for ${packageName}`); | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 21-21: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(pkgDir, 'package.json'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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 `@apps/tauri/vite.config.ts` around lines 19 - 36, `resolveBunEntry` currently
assumes `pkgJson.browser` is always a string, so packages with object-valued
`browser` mappings can resolve to invalid paths and get skipped by the
`cjsInteropEntries` flow. Update the `resolveBunEntry` logic to handle the
`browser` field defensively by only using it when it is a string, otherwise fall
back to `main` or `index.js`, and keep the existing candidate resolution/throw
behavior in `resolveBunEntry` and its callers unchanged.
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| : []), | ||
| { | ||
| find: /^@spacebot\/api-client$/, | ||
| replacement: hasSpacebot |
There was a problem hiding this comment.
[🟡 Medium] [🔵 Bug]
The new fallback only aliases @spacebot/api-client inside Vite, but both @apps/tauri/tsconfig.json and @packages/interface/tsconfig.json still map that module exclusively to ../../../spacebot/packages/api-client/src. @apps/tauri/src/App.tsx imports @sd/interface/Spacebot and @sd/interface/windows/VoiceOverlay, so bun run --filter @sd/tauri typecheck on the intended no-spacebot setup still resolves the Spacebot API imports to a missing external checkout instead of this stub. Mirror the fallback in TypeScript resolution, or provide a local declaration/package path that TypeScript can use when spacebot is absent.
// apps/tauri/vite.config.ts
{
find: /^@spacebot\/api-client$/,
replacement: hasSpacebot
? `${spacebot}/api-client/src`
: spacebotStub,
},This branch has not been deployed
1 participant
Summary
style-to-jsand related deps)optimizeDeps.includeentries for affected packages@spacebot/api-clientwhen the spacebot repo is not cloned locallyTest plan
cd apps/tauri && bun run buildcd apps/tauri && bun run tauri:dev— app mounts React and connects to daemon