refactor(release): lean releaseTrain modules - #372
Conversation
|
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 54 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
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 |
codeant-ai
Bot
added
the
size:L
PR Summary by QodoRefactor release-train modules with shared error handling and pure helpers
AI Description
Diagram
High-Level Assessment
Files changed (23)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTo customize comments, go to the Qodo configuration screen, or learn more in the docs. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (!planInput.noOta && planInput.hasCloudStorage) { | ||
| ota = platforms.map((platform) => ({ | ||
| platform, | ||
| channel: input.channel, | ||
| runtimeVersion: input.runtimeVersion, | ||
| channel: planInput.channel, | ||
| runtimeVersion: planInput.runtimeVersion, | ||
| })); | ||
| } |
There was a problem hiding this comment.
Suggestion: OTA followers are created for every declared platform whenever cloud storage is configured, before native submission succeeds. If startTrain records a native submission failure, the corresponding OTA remains pending, while advanceTrain only publishes it when that native car is exactly released; because failed native cars are terminal and never transition to released, the train remains permanently in progress and watch mode cannot settle. Do not create the follower until the native leg is successfully submitted, or mark/cancel the follower when its native leg fails. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ OTA-enabled trains remain permanently in progress after submission failure.
- ❌ `release-train status --watch` can poll indefinitely.
- ⚠️ Failed native legs leave orphaned pending OTA followers.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/releaseTrain/engine.ts
**Line:** 72:78
**Comment:**
*Incomplete Implementation: OTA followers are created for every declared platform whenever cloud storage is configured, before native submission succeeds. If `startTrain` records a native submission failure, the corresponding OTA remains pending, while `advanceTrain` only publishes it when that native car is exactly `released`; because failed native cars are terminal and never transition to released, the train remains permanently in progress and watch mode cannot settle. Do not create the follower until the native leg is successfully submitted, or mark/cancel the follower when its native leg fails.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| const releaseTrainOptions: { | ||
| app?: string; | ||
| profile: string; | ||
| platform?: string; | ||
| ota: boolean; | ||
| hold?: boolean; | ||
| channel: string; | ||
| runtimeVersion?: string; | ||
| watch?: boolean; | ||
| json?: boolean; | ||
| } = {}; | ||
| if (commandOptions.app !== undefined) optionalOptions.app = commandOptions.app; | ||
| if (commandOptions.platform !== undefined) optionalOptions.platform = commandOptions.platform; | ||
| if (commandOptions.hold !== undefined) optionalOptions.hold = commandOptions.hold; | ||
| if (commandOptions.runtimeVersion !== undefined) | ||
| optionalOptions.runtimeVersion = commandOptions.runtimeVersion; | ||
| if (commandOptions.watch !== undefined) optionalOptions.watch = commandOptions.watch; | ||
| if (commandOptions.json !== undefined) optionalOptions.json = commandOptions.json; | ||
| const releaseTrainOptions = { | ||
| env: string[]; | ||
| includeLocal: boolean; | ||
| } = { | ||
| profile: commandOptions.profile, | ||
| ota: commandOptions.ota, | ||
| channel: commandOptions.channel, | ||
| env: commandOptions.env, | ||
| includeLocal: commandOptions.includeLocal, | ||
| ...optionalOptions, | ||
| }; |
There was a problem hiding this comment.
Suggestion: The CLI registers the shared --print-env option through addEnvFlags, but this newly constructed options object omits printEnv. As a result, invoking release-train --print-env silently discards the user's request and executes the train normally instead of printing the resolved environment or rejecting the unsupported option. Preserve the flag if release-train supports it, or stop registering it for this command. [api mismatch]
Severity Level: Major ⚠️
- ❌ `release-train --print-env` runs the train command unexpectedly.
- ⚠️ CI/debug environment inspection output is unavailable.
- ⚠️ Users may trigger status, startup, or store API work accidentally.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/cli/commands/releaseTrain.ts
**Line:** 46:64
**Comment:**
*Api Mismatch: The CLI registers the shared `--print-env` option through `addEnvFlags`, but this newly constructed options object omits `printEnv`. As a result, invoking `release-train --print-env` silently discards the user's request and executes the train normally instead of printing the resolved environment or rejecting the unsupported option. Preserve the flag if release-train supports it, or stop registering it for this command.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let versionCandidate = appDescriptor.version; | ||
| if (versionCandidate === undefined) { | ||
| const storeVersion = yield* attemptTransport( | ||
| 'read App Store version', | ||
| ascClient.getLatestMarketingVersion(bundleId), | ||
| ); | ||
| if (storeVersion !== null) versionString = storeVersion; | ||
| if (storeVersion !== null) versionCandidate = storeVersion; | ||
| } | ||
| if (versionString === undefined) { | ||
| const marketingVersion = usableMarketingVersion(versionCandidate); | ||
| if (marketingVersion === null) { |
There was a problem hiding this comment.
Suggestion: A configured empty version never falls back to App Store Connect because the lookup is guarded only by versionCandidate === undefined. usableMarketingVersion then rejects the empty value and the train fails even when getLatestMarketingVersion could provide a valid version. Treat an empty configured version as absent before performing the fallback lookup. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ iOS release-train startup fails with an empty configured version.
- ⚠️ Valid App Store marketing-version fallback is skipped.
- ⚠️ Developers receive a failure despite an available store version.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/releaseTrain/builder.ts
**Line:** 244:253
**Comment:**
*Incorrect Condition Logic: A configured empty version never falls back to App Store Connect because the lookup is guarded only by `versionCandidate === undefined`. `usableMarketingVersion` then rejects the empty value and the train fails even when `getLatestMarketingVersion` could provide a valid version. Treat an empty configured version as absent before performing the fallback lookup.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core/releaseTrain/record.ts">
<violation number="1" location="src/core/releaseTrain/record.ts:73">
P3: For non-Error `cause` values, `errorMessage` returns `String(cause)` ("null", "undefined", "[object Object]"), which replaces the previous informative `${operation} failed.` fallback. Consider falling back to the operation label whenever the shared message yields a non-descriptive value, so users still see which operation failed.</violation>
</file>
<file name="src/core/releaseTrain/command.ts">
<violation number="1" location="src/core/releaseTrain/command.ts:97">
P3: The new `trainFailure` derives its message via the shared `errorMessage(cause)` for any non-Error cause. Because `errorMessage` falls back to `String(cause)`, a non-Error cause (a rejected object, or an undefined/null cause) now surfaces as literal 'undefined', 'null', or '[object Object]' in the release-train failure channel instead of the friendlier `${operation} failed.` fallback that the old code produced. The `message.length === 0` guard only catches empty strings, not these stringified placeholders. Consider guarding on a truthy/non-generic result, e.g. only use `errorMessage(cause)` when it yields a meaningful message and otherwise fall back to '${operation} failed.'</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let message = errorMessage(cause); | ||
| if (message.length === 0) message = `${operation} failed.`; |
There was a problem hiding this comment.
P3: For non-Error cause values, errorMessage returns String(cause) ("null", "undefined", "[object Object]"), which replaces the previous informative ${operation} failed. fallback. Consider falling back to the operation label whenever the shared message yields a non-descriptive value, so users still see which operation failed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/releaseTrain/record.ts, line 73:
<comment>For non-Error `cause` values, `errorMessage` returns `String(cause)` ("null", "undefined", "[object Object]"), which replaces the previous informative `${operation} failed.` fallback. Consider falling back to the operation label whenever the shared message yields a non-descriptive value, so users still see which operation failed.</comment>
<file context>
@@ -62,21 +63,25 @@ export const makeTrainRecordFailure = Data.tagged<TrainRecordFailure>('TrainReco
const recordFailure = (operation: string, cause: unknown): TrainRecordFailure => {
- let message = `${operation} failed.`;
- if (cause instanceof Error) message = cause.message;
+ let message = errorMessage(cause);
+ if (message.length === 0) message = `${operation} failed.`;
return makeTrainRecordFailure({ operation, message, cause });
</file context>
| let message = errorMessage(cause); | |
| if (message.length === 0) message = `${operation} failed.`; | |
| let message = errorMessage(cause); | |
| if (message.length === 0 || message === 'null' || message === 'undefined' || message === '[object Object]') | |
| message = `${operation} failed.`; |
| let message = fallbackMessage; | ||
| if (message === undefined && cause instanceof Error) message = cause.message; | ||
| if (message === undefined) message = `${operation} failed.`; | ||
| if (message === undefined) message = errorMessage(cause); |
There was a problem hiding this comment.
P3: The new trainFailure derives its message via the shared errorMessage(cause) for any non-Error cause. Because errorMessage falls back to String(cause), a non-Error cause (a rejected object, or an undefined/null cause) now surfaces as literal 'undefined', 'null', or '[object Object]' in the release-train failure channel instead of the friendlier ${operation} failed. fallback that the old code produced. The message.length === 0 guard only catches empty strings, not these stringified placeholders. Consider guarding on a truthy/non-generic result, e.g. only use errorMessage(cause) when it yields a meaningful message and otherwise fall back to '${operation} failed.'
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/releaseTrain/command.ts, line 97:
<comment>The new `trainFailure` derives its message via the shared `errorMessage(cause)` for any non-Error cause. Because `errorMessage` falls back to `String(cause)`, a non-Error cause (a rejected object, or an undefined/null cause) now surfaces as literal 'undefined', 'null', or '[object Object]' in the release-train failure channel instead of the friendlier `${operation} failed.` fallback that the old code produced. The `message.length === 0` guard only catches empty strings, not these stringified placeholders. Consider guarding on a truthy/non-generic result, e.g. only use `errorMessage(cause)` when it yields a meaningful message and otherwise fall back to '${operation} failed.'</comment>
<file context>
@@ -87,60 +88,62 @@ type PreparedTrain = Readonly<{
let message = fallbackMessage;
- if (message === undefined && cause instanceof Error) message = cause.message;
- if (message === undefined) message = `${operation} failed.`;
+ if (message === undefined) message = errorMessage(cause);
+ if (message.length === 0) message = `${operation} failed.`;
return makeReleaseTrainCommandFailure({ operation, message, cause });
</file context>
1302f07 to
3d35f7f
Compare
Share errorMessage, extract pure release-gate / presentation / marketing- version helpers, rename resolve stems (planTrainCars, createTrainRuntime, loadTargetTrain), and keep car guards. Add colocated business tests for gate, slug, status detail, and train-record helpers; refresh docs badges. Fixes #350
3d35f7f to
cf11321
Compare
1 participant
Summary
src/core/releaseTrain/*and the thin CLI facade while keeping train car guards.errorMessage, extract pure release-gate / presentation / marketing-version / live-train helpers, rename resolve/build stems (planTrainCars,createTrainRuntime,loadTargetTrain).Before / after
instanceof Error/ String(cause)errorMessageadvanceTrainisReleaseGateOpen+isNativeFailure/isNativeApprovedOrReleased(tested)buildTrainRuntime,resolveTrainCars,resolveTargetcreateTrainRuntime,planTrainCars,loadTargetTraincarStatusLinecarStatusDetail/trainAppSlugGate
All six green. Unit: 2096 passing.
Notes
Fixes #350
Summary by cubic
Leaned the releaseTrain modules by extracting pure helpers and unifying error handling with
errorMessage, with no behavior changes. Adds focused tests and refreshes docs/test badges. Fixes #350.buildTrainRuntime→createTrainRuntime,resolveTrainCars→planTrainCars,resolveTarget→loadTargetTrain,ResolveCarsInput→TrainCarPlanInput.isReleaseGateOpen,isNativeFailure,isNativeApprovedOrReleased; presentationcarStatusDetail,trainAppSlug; marketing-versionusableMarketingVersion,marketingVersionMissingMessage; store-buildsisUsableIosStoreBuild,isStoredAndroidBuild; recordsisLiveTrain,safeTrainId.errorMessagefor clearer messages.Written for commit cf11321. Summary will update on new commits.