fix(target): preserve app context across bundle and publish - #74
fix(target): preserve app context across bundle and publish#74sunnylqm wants to merge 14 commits into
Conversation
|
Warning Review limit reachedNext included review available in 12 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughBundle publishing now carries explicit ChangesTarget context propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change preserves the selected app and configuration through bundling, publishing, version creation, and native-package binding. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CLI
participant normalizeBundleOptions
participant createAppTargetResolver
participant createPublishBundleRequest
participant PublishAPI
participant versionCommands
CLI->>normalizeBundleOptions: provide appId and config
normalizeBundleOptions->>createAppTargetResolver: resolve target context
createAppTargetResolver-->>createPublishBundleRequest: return appId and configPath
createPublishBundleRequest->>PublishAPI: submit bundle publish request
PublishAPI-->>CLI: return version ID
CLI->>versionCommands: update version with appId
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
全部评审意见已逐项落实:
最新提交: GitHub Actions run #175 全部通过:
CodeRabbit 对功能变更的最近一次完整增量评审结论为 No actionable comments、Merge Risk: Minimal;当前无未解决 review thread,最新 commit status 亦为 success。 |
|
✅ Action performedReview finished.
|
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
…for both brands Follow-up to the review of #74. - keep `update.json` as the selected-app file for cresc too: the cresc docs and the client SDK read that name, and `cresc.config.json` was never wired in, so switching the default would have broken every existing cresc project - replace the nine hand-rolled `options.appId || getSelectedApp(...)` blocks in bundle/versions/package with one `resolveAppId()` helper - bundle: resolve the app before any side effect (.gitignore edits, plugin probes) so a named bundle without a selected app fails immediately; a bundle-only run only tolerates a missing selection (typed AppNotSelectedError) and reports malformed configs instead of swallowing them; drop the dead `config` forwarding and the three-way cached target - SDK: `BundleOptions.appId/config` and `provider.getSelectedApp(platform, config)` so programmatic callers get the same single-app guarantee - messages: parse/mismatch errors name the file (or `--appId`) actually used - tests: exercise bundleCommands.bundle end to end (Hermes base + publish get the same app, fail-fast, bundle-only fallback, dev bundles) and the default file, instead of the removed wrapper helpers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCaS35vZG4DCtmM24MYaVR
…for both brands Follow-up to the review of #74. - keep `update.json` as the selected-app file for cresc too: the cresc docs and the client SDK read that name, and `cresc.config.json` was never wired in, so switching the default would have broken every existing cresc project - replace the nine hand-rolled `options.appId || getSelectedApp(...)` blocks in bundle/versions/package with one `resolveAppId()` helper - bundle: resolve the app before any side effect (.gitignore edits, plugin probes) so a named bundle without a selected app fails immediately; a bundle-only run only tolerates a missing selection (typed AppNotSelectedError) and reports malformed configs instead of swallowing them; drop the dead `config` forwarding and the three-way cached target - SDK: `BundleOptions.appId/config` and `provider.getSelectedApp(platform, config)` so programmatic callers get the same single-app guarantee - messages: parse/mismatch errors name the file (or `--appId`) actually used - tests: exercise bundleCommands.bundle end to end (Hermes base + publish get the same app, fail-fast, bundle-only fallback, dev bundles) and the default file, instead of the removed wrapper helpers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCaS35vZG4DCtmM24MYaVR
de0cdc6 to
ac3ede8
Compare
…for both brands Follow-up to the review of #74. - keep `update.json` as the selected-app file for cresc too: the cresc docs and the client SDK read that name, and `cresc.config.json` was never wired in, so switching the default would have broken every existing cresc project - replace the nine hand-rolled `options.appId || getSelectedApp(...)` blocks in bundle/versions/package with one `resolveAppId()` helper - bundle: resolve the app before any side effect (.gitignore edits, plugin probes) so a named bundle without a selected app fails immediately; a bundle-only run only tolerates a missing selection (typed AppNotSelectedError) and reports malformed configs instead of swallowing them; drop the dead `config` forwarding and the three-way cached target - SDK: `BundleOptions.appId/config` and `provider.getSelectedApp(platform, config)` so programmatic callers get the same single-app guarantee - messages: parse/mismatch errors name the file (or `--appId`) actually used - tests: exercise bundleCommands.bundle end to end (Hermes base + publish get the same app, fail-fast, bundle-only fallback, dev bundles) and the default file, instead of the removed wrapper helpers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCaS35vZG4DCtmM24MYaVR
ac3ede8 to
7e89a29
Compare
Summary
resolveAppId()), instead of each step re-reading the selected appappIdfrombundleintopublish, and from version creation into native-package binding--configwhen selecting or creating an appupdate.jsonas the selected-app file for both brands (see below)BundleOptions.appId/config,provider.getSelectedApp(platform, config)Problem
A single publish command could operate on more than one app context:
bundle --appId ... --name ...used the explicit app for the Hermes base lookup, but did not forward it topublishpublish --appId ...created the OTA version under that app, then the internal binding step re-read the default selected appcreateApp --config ...selected the newly created app in the default config rather than the requested fileprovider.bundle()) had no way to pin the app at allIn a multi-app project this could create a version for one app and attempt to bind it against another app's native package, or compile a Hermes delta against one app's base and upload it to another.
Changes
resolveAppId({ appId, config, platform })inapp.ts: explicit--appIdwins, otherwise the app selected for the platform in--config(defaultupdate.json). Replaces nine hand-rolled resolution blocks inbundle.ts,versions.ts,package.ts.bundle: the app is resolved before any side effect (.gitignoreedits, plugin probes, Metro/Hermes). A named bundle with no selected app fails immediately; a bundle-only run tolerates only a missing selection (typedAppNotSelectedError, full-bytecode fallback) and reports a malformed config instead of swallowing it. The same app id feeds the Hermes base lookup andpublish.publishpasses the resolvedappIdinto bothupdatecalls (binding, deps warning).createApp --configselects the new app in that file.--appId) instead of a hard-codedupdate.json.Why
update.jsonstays the default for crescupdateJsoninconstants.tssaidcresc.config.jsonbut was never used: every read/write inapp.tshard-codedupdate.json, the cresc docs (getting-started,integration,cli) documentupdate.json, and the client SDK doesimport _updateConfig from './update.json'. Switching the default would have broken every existing cresc project with no migration path, so the constant is nowupdate.jsonfor both brands.Tests
tests/target-context.test.tsdrivesbundleCommands.bundleend to end with the runner/pack/publish mocked: one config read shared by the Hermes base lookup andpublish, default file, explicit--appId, fail-fast on missing selection for named bundles, bundle-only fallback, malformed config, dev bundlesresolveAppIdunit cases (default file, explicit config, explicit id, platform validation, missing/mismatched/malformed config)versions.test.ts: explicitappIdis kept through upload, version creation, and bindingconstants.test.ts:updateJsonisupdate.jsonfor crescNot changed here:
--appIdis still not cross-checked against the app's platform on the client (pre-existing forpublish --appId; the server validates).