fix: handle pnpm node_modules paths in native build zip#502
Conversation
📝 WalkthroughWalkthroughThe PR normalizes native dependency paths (pnpm/store and node_modules prefixes) for iOS (SPM/CocoaPods) and Android (Gradle) during native dependency discovery, and rewrites pnpm-style paths to standard node_modules references in text-based build files inside the zip before finalizing the archive. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI build flow
participant Parser as Native path parsers (SPM / CocoaPods / Gradle)
participant Rewriter as Zip content rewriter
participant Zip as Zip packaging service
CLI->>Parser: Scan project files for native dependency references
Parser-->>CLI: Emit normalized package paths (trim up to last node_modules/, strip platform suffixes)
CLI->>Zip: Collect files into temporary archive
Zip->>Rewriter: Provide text entries for inspection
Rewriter-->>Zip: Replace pnpm store paths with standard `node_modules/`, collapse ios relative paths
Zip->>CLI: Finalize and return normalized archive
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/build/request.ts (1)
769-787:zip.updateFileAPI confirmed valid — overall rewrite logic is correct.
updateFile(entry, content)is listed in the official adm-zip wiki API, and theupdateFilemethod takes the filename and new content as a Buffer, matching the call on line 777.One minor observation:
rewriteFileInZipsilently returns when the entry isn't found (if (!entry) return). This is correct for legitimately absent files (e.g., Podfile on an SPM project), but it equally silences a wrong-entry-path bug — the zip would be uploaded with unrewritten pnpm paths and CI builds would fail with the same symptom this PR fixes. Adding a verbose-mode log for the no-op case would make that failure path diagnosable:🔍 Suggested verbose logging (optional)
const rewriteFileInZip = (zipEntryPath: string) => { const entry = zip.getEntry(zipEntryPath) - if (!entry) - return + if (!entry) { + if (verbose) + log.info(`[pnpm rewrite] entry not found in zip: ${zipEntryPath}`) + return + } const original = entry.getData().toString('utf-8')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/request.ts` around lines 769 - 787, The rewriteFileInZip helper quietly returns when zip.getEntry(zipEntryPath) yields no entry, which hides cases where an expected file (e.g., `${platformDir}/Podfile`) is missing and pnpm path rewrites never run; update rewriteFileInZip to log a verbose/warn message when entry is falsy (including the attempted zipEntryPath and platform) so CI diagnostics show why pnpmPathPattern rewrites didn’t occur, while preserving the existing behavior of using pnpmPathPattern and calling zip.updateFile(Buffer) when replacements happen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/build/request.ts`:
- Around line 769-787: The rewriteFileInZip helper quietly returns when
zip.getEntry(zipEntryPath) yields no entry, which hides cases where an expected
file (e.g., `${platformDir}/Podfile`) is missing and pnpm path rewrites never
run; update rewriteFileInZip to log a verbose/warn message when entry is falsy
(including the attempted zipEntryPath and platform) so CI diagnostics show why
pnpmPathPattern rewrites didn’t occur, while preserving the existing behavior of
using pnpmPathPattern and calling zip.updateFile(Buffer) when replacements
happen.
The previous commit only rewrote specific files (Podfile, settings.gradle). This was insufficient because pnpm paths also leak into Podfile.lock, Manifest.lock, Pods.xcodeproj/project.pbxproj, and .xcconfig files. Additionally, pod install with pnpm resolves symlinks and bakes the real .pnpm store depth into relative paths in project.pbxproj group entries (e.g. ../../../../../../ios/App/Pods/ instead of ../../../ios/App/Pods/). These don't contain .pnpm text so the first regex misses them, and they resolve to wrong locations on CI runners with different workspace depths. Fix: - Scan ALL text-based zip entries (by extension) instead of hardcoded paths. - Collapse excessive ../ sequences before ios/ in pbxproj to 3 levels.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/build/request.ts (1)
779-781: Two minor clarity issues: variable shadow and redundantPodfileguard.
const basenameat line 780 shadows thebasenamefunction imported fromnode:path— use a distinct name (e.g.,entryBasename).- Including
''intextExtensionsalready admits all extensionless files (includingPodfile), making the explicitbasename !== 'Podfile'guard a dead-code branch. Either drop''from the set and rely solely on thePodfilename check, or drop the name check and document that''intentionally covers all extensionless files.♻️ Proposed cleanup
- const ext = entry.entryName.includes('.') ? `.${entry.entryName.split('.').pop()}` : '' - const basename = entry.entryName.split('/').pop() || '' - if (!textExtensions.has(ext) && basename !== 'Podfile') + const entryBasename = entry.entryName.split('/').pop() || '' + const ext = entryBasename.includes('.') ? `.${entryBasename.split('.').pop()}` : '' + if (!textExtensions.has(ext) && entryBasename !== 'Podfile') continueAnd remove
''fromtextExtensionsso only the explicitPodfilecatch handles extensionless files:const textExtensions = new Set([ - '', '.gradle', ... ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/request.ts` around lines 779 - 781, Rename the local const basename to entryBasename (it currently shadows the imported basename from node:path) and update its usages (e.g., where you check for 'Podfile'); also remove the empty-string entry ('') from the textExtensions set and rely on the explicit basename === 'Podfile' check to allow extensionless Podfile files, updating any comments to document that extensionless files are only permitted via the Podfile name guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/build/request.ts`:
- Around line 772-775: The Set initialization for textExtensions mixes inline
and multi-line items and violates antfu/consistent-list-newline; update the new
Set([...]) expression (symbol: textExtensions) so each string element is on its
own line inside the array literal (one item per line, including the empty string
''), maintaining the same items and commas, so the linter passes.
---
Nitpick comments:
In `@src/build/request.ts`:
- Around line 779-781: Rename the local const basename to entryBasename (it
currently shadows the imported basename from node:path) and update its usages
(e.g., where you check for 'Podfile'); also remove the empty-string entry ('')
from the textExtensions set and rely on the explicit basename === 'Podfile'
check to allow extensionless Podfile files, updating any comments to document
that extensionless files are only permitted via the Podfile name guard.
| const textExtensions = new Set([ | ||
| '', '.gradle', '.swift', '.json', '.lock', '.xml', '.properties', | ||
| '.pbxproj', '.xcconfig', '.plist', '.podspec', '.rb', '.yaml', '.yml', | ||
| ]) |
There was a problem hiding this comment.
ESLint antfu/consistent-list-newline error blocks linting CI.
The array inside new Set([...]) mixes inline and multi-line layout. Each item needs its own line.
🔧 Proposed fix
- const textExtensions = new Set([
- '', '.gradle', '.swift', '.json', '.lock', '.xml', '.properties',
- '.pbxproj', '.xcconfig', '.plist', '.podspec', '.rb', '.yaml', '.yml',
- ])
+ const textExtensions = new Set([
+ '',
+ '.gradle',
+ '.swift',
+ '.json',
+ '.lock',
+ '.xml',
+ '.properties',
+ '.pbxproj',
+ '.xcconfig',
+ '.plist',
+ '.podspec',
+ '.rb',
+ '.yaml',
+ '.yml',
+ ])📝 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.
| const textExtensions = new Set([ | |
| '', '.gradle', '.swift', '.json', '.lock', '.xml', '.properties', | |
| '.pbxproj', '.xcconfig', '.plist', '.podspec', '.rb', '.yaml', '.yml', | |
| ]) | |
| const textExtensions = new Set([ | |
| '', | |
| '.gradle', | |
| '.swift', | |
| '.json', | |
| '.lock', | |
| '.xml', | |
| '.properties', | |
| '.pbxproj', | |
| '.xcconfig', | |
| '.plist', | |
| '.podspec', | |
| '.rb', | |
| '.yaml', | |
| '.yml', | |
| ]) |
🧰 Tools
🪛 ESLint
[error] 773-773: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 773-773: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 773-773: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 773-773: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 773-773: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 773-773: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 774-774: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 774-774: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 774-774: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 774-774: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 774-774: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
[error] 774-774: Should have line breaks between items, in node ArrayExpression
(antfu/consistent-list-newline)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/build/request.ts` around lines 772 - 775, The Set initialization for
textExtensions mixes inline and multi-line items and violates
antfu/consistent-list-newline; update the new Set([...]) expression (symbol:
textExtensions) so each string element is on its own line inside the array
literal (one item per line, including the empty string ''), maintaining the same
items and commas, so the linter passes.
2 participants
Summary
build requestuploaded emptynode_moduleswhen the project used pnpm (or had stale pnpm-generated Capacitor config files), causing CI builds to fail with missing dependency errors. iOS builds also failed due to pnpm paths embedded deep in Pods project files.capacitor.settings.gradle,Podfile, and CocoaPods-generated files (project.pbxproj,.xcconfig,Manifest.lock) contain pnpm store paths likenode_modules/.pnpm/@capacitor+android@8.1.0.../node_modules/@capacitor/android/capacitor. The CLI'sextractNativeDependenciesused these raw.pnpm/...paths as package identifiers, which never matched the actual directory structure during zip traversal — resulting in zeronode_modulesfiles being included. Additionally,pod installwith pnpm resolves symlinks and bakes the real.pnpmstore depth into relative paths inproject.pbxprojgroup entries (e.g.../../../../../../ios/App/Pods/instead of../../../ios/App/Pods/), causing Xcode to look for files at wrong paths on CI.Changes
extractNativeDependencies— For all platforms (Android gradle, iOS Podfile, iOS SPM), normalize captured paths by stripping the.pnpm/.../node_modules/prefix to get the real package name. Also handle@capacitor/android's special/capacitorsuffix.zipDirectory— pnpm text rewrite — Scan ALL text-based zip entries (by file extension) for pnpm store paths and rewrite them to standard flatnode_modules/paths. This covers Podfile, Podfile.lock, Manifest.lock,project.pbxproj,.xcconfigfiles,settings.gradle, etc.zipDirectory— iOS deep relative path fix — Collapse excessive../sequences beforeios/in pbxproj group paths (from 6+ levels back down to 3 levels). These deep relative paths are generated bypod installresolving pnpm symlinks and don't contain.pnpm/text, so the first regex misses them.Testing
build request --platform android→ build succeeded on CIbuild request --platform android→ build succeeded on CIbuild request --platform ios→ build succeeded on CIbuild request --platform ios→ build succeeded on CISummary by CodeRabbit