GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||
GeezerVison is showing github.com — fix(ios): let the app's xcconfig win over a plugin's, and warn on discarded settings by edusperoni · Pull Request #6091 · NativeScript/nativescript-cli Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesXCConfig merge behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant XcconfigService
participant RubyMerge
participant Logger
XcconfigService->>RubyMerge: merge destination and source xcconfig files
RubyMerge-->>XcconfigService: return merged output with conflict JSON
XcconfigService->>Logger: log warnings for differing settings
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
test/xcconfig-service.ts (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct coverage for conflict reporting.
Use
LoggerStubto assert: differing values emit one warning, equivalent values emit none, and malformed marker JSON only emits a trace without rejecting the merge.🤖 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 `@test/xcconfig-service.ts` at line 22, Add direct tests around the conflict-reporting merge flow using the registered LoggerStub: verify differing values produce exactly one warning, equivalent values produce no warnings, and malformed marker JSON produces only a trace while allowing the merge to complete successfully.
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 `@lib/services/xcconfig-service.ts`:
- Around line 93-98: Remove the unconditional App_Resources attribution from the
warning assembled in the conflicts loop of the xcconfig merge logic. Keep the
existing retained-value and higher-precedence message so plugin-vs-plugin
conflicts accurately reflect dependency-order precedence without claiming the
app won.
- Around line 46-68: Update the XCConfig merge execution around mergeScript and
$childProcess.exec to use execFile with separate Ruby arguments instead of
shell-interpolating a ruby -e command. Pass the merge script and
destination/source paths as arguments, read the paths from Ruby’s ARGV, and
remove the apostrophe-based path interpolation so command substitution cannot
execute.
---
Nitpick comments:
In `@test/xcconfig-service.ts`:
- Line 22: Add direct tests around the conflict-reporting merge flow using the
registered LoggerStub: verify differing values produce exactly one warning,
equivalent values produce no warnings, and malformed marker JSON produces only a
trace while allowing the merge to complete successfully.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c89f567-321c-4776-9de2-4183c3b6187f
lib/services/ios-project-service.tslib/services/xcconfig-service.tstest/ios-project-service.tstest/xcconfig-service.ts
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
The merge ran through a shell-interpolated `ruby -e "..."` with only apostrophes escaped, so a project or node_modules path containing a command substitution was executed by the shell before ruby started. A directory named `a$(hostname)b` is enough to reproduce it. execFile passes the script and both paths as arguments, which removes the shell from the path entirely and makes the escaping unnecessary. Also corrects the conflict warning: it claimed the app's xcconfig had won, which is wrong for a plugin-vs-plugin conflict, where the earlier plugin wins by dependency order. It now states the ordering instead of naming a winner.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.