[codex] Rewrite pnpm SPM node_modules paths#2003
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated pnpm path-rewriting logic in the build process to normalize excessive relative path references ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 8 minutes and 23 seconds. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 255e201960
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/test/test-build-zip-filter.mjs (1)
453-494:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd coverage for a nested
ios.pathlayout.This regression only exercises the flat
ios/case, so it won’t catch the path-depth bug above. A second fixture with a deeperios.path(for exampleapps/native/ios) would verify thatPackage.swiftkeeps the correct number of../segments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/test/test-build-zip-filter.mjs` around lines 453 - 494, Add a second test case in the same file that exercises a nested ios.path (e.g., 'apps/native/ios') to catch the path-depth bug: create a project fixture with the deeper ios layout, write the same Package.swift and plugin files under that nested ios path, call zipDirectory with ios.path set to the deeper path, then assert the zip contains the workspace plugin files (using entries.includes checks) and that zip.readAsText for the generated CapApp-SPM/Package.swift uses the correct number of ../ segments for the deeper layout; update assertions to match the expected relative path (more ../ segments) so the test fails until the path-depth bug is fixed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/build/request.ts`:
- Around line 1089-1096: The current ios-path rewrite unconditionally reduces
any run of four-or-more "../" to exactly "../../../", which breaks deeply nested
ios layouts; in the platform === 'ios' block that mutates rewritten, change the
replace call to use a replacement function (not a fixed string) that counts how
many "../" segments were matched and reduces them to at most three but preserves
fewer if the original needed fewer — i.e. compute the match length, decide the
number of "../" to keep (min(originalCount, 3)), and return that repeated
segment plus the captured "ios/" or "node_modules/" suffix so nested ios paths
that legitimately need more hops aren't truncated incorrectly. Ensure you update
the call that references rewritten.replace and keep the same regex but provide a
callback replacement.
---
Outside diff comments:
In `@cli/test/test-build-zip-filter.mjs`:
- Around line 453-494: Add a second test case in the same file that exercises a
nested ios.path (e.g., 'apps/native/ios') to catch the path-depth bug: create a
project fixture with the deeper ios layout, write the same Package.swift and
plugin files under that nested ios path, call zipDirectory with ios.path set to
the deeper path, then assert the zip contains the workspace plugin files (using
entries.includes checks) and that zip.readAsText for the generated
CapApp-SPM/Package.swift uses the correct number of ../ segments for the deeper
layout; update assertions to match the expected relative path (more ../
segments) so the test fails until the path-depth bug is fixed.
🪄 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
Run ID: c6e86a0b-aaef-49c3-b699-16860e781a86
📒 Files selected for processing (2)
cli/src/build/request.tscli/test/test-build-zip-filter.mjs
|
1 participant
Summary (AI generated)
Motivation (AI generated)
The native build archive already included the pnpm package contents, but Package.swift could still point at ../../../../../node_modules/... after .pnpm path flattening, so Xcode looked for packages outside the uploaded layout.
Business Impact (AI generated)
This unblocks native iOS cloud builds for pnpm workspace customers using Capacitor SPM dependencies, reducing build failures for monorepo apps.
Test Plan (AI generated)
Generated with AI
Summary by CodeRabbit
Bug Fixes
Tests