refactor(store): readonly-domain-types (stack 5/12, re-split #307) - #378
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 · |
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
PR Summary by QodoRefactor store domain consumers for readonly types
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
| const report = yield* reconcileApp(client, reconcileInput); | ||
| report.actions.push(...(yield* reconcileAssetActions(client, job, dryRun, allowDestructive))); | ||
| return { job, report }; | ||
| const assetActions = yield* reconcileAssetActions(client, job, dryRun, allowDestructive); |
There was a problem hiding this comment.
Suggestion: If reconcileApp fails with a catalog precondition or API error, this call is never reached and the surrounding catchAll returns only { error }. Consequently, screenshots and previews are neither reconciled nor reported, even though reconcileAssetActions is explicitly designed to be an isolated pass whose failures must not discard other work. Run the asset pass independently of catalog reconciliation and combine both outcomes. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Sync reports only catalog errors, omitting asset actions.
- ⚠️ Asset planning is skipped after catalog API failures.
- ❌ Apply cannot process assets from failed catalog plans.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/core/store/syncRun.ts
**Line:** 190:190
**Comment:**
*Incomplete Implementation: If `reconcileApp` fails with a catalog precondition or API error, this call is never reached and the surrounding `catchAll` returns only `{ error }`. Consequently, screenshots and previews are neither reconciled nor reported, even though `reconcileAssetActions` is explicitly designed to be an isolated pass whose failures must not discard other work. Run the asset pass independently of catalog reconciliation and combine both outcomes.
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
Code Review by Qodo
1. ReconcileContext outside core/types.ts
|
| /** Mutable state for one reconciliation pass. */ | ||
| export type ReconcileContext = { | ||
| actions: PlannedAction[]; | ||
| actions: MutableDeep<PlannedAction>[]; | ||
| dryRun: boolean; |
There was a problem hiding this comment.
1. reconcilecontext outside core/types.ts 📘 Rule violation ⌂ Architecture
This PR modifies exported domain-shape types (ReconcileContext, SyncAppReport.actions) in feature/logic modules instead of centralizing them in src/core/types.ts as required. This makes shared shapes harder to reuse consistently and violates the required type-centralization policy.
Agent Prompt
## Issue description
The PR changes exported domain-shape types in non-type modules (e.g., `src/core/store/reconcile.ts`, `src/core/store/syncRun.ts`) instead of defining them in `src/core/types.ts`.
## Issue Context
Compliance requires new/modified domain shapes to live in `src/core/types.ts` (not in feature/logic modules).
## Fix Focus Areas
- src/core/store/reconcile.ts[6-10]
- src/core/store/syncRun.ts[37-42]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| const mergeAppleLocale = ( | ||
| existingLocale: AppleLocaleInfo | undefined, | ||
| listingDraft: DraftListing, | ||
| ): AppleLocaleInfo => { |
There was a problem hiding this comment.
2. Manual locale merge drift 🐞 Bug ⚙ Maintainability
mergeAppleLocale manually copies each AppleLocaleInfo field, so any future additions to AppleLocaleInfo that exist in persisted configs will be silently dropped when a locale is updated via applyDraft. This is a forward-compatibility data-loss risk compared to the prior generic spread merge behavior.
Agent Prompt
### Issue description
`mergeAppleLocale` enumerates `AppleLocaleInfo` fields one-by-one. If `AppleLocaleInfo` gains new optional fields later (and existing configs contain them), calling `applyDraft` for a locale will rebuild that locale object without the new fields, effectively deleting them.
### Issue Context
The old approach of spreading the existing locale preserved all enumerable runtime properties. The new approach requires manual synchronization with `AppleLocaleInfo`.
### Fix Focus Areas
- src/core/listing/apply.ts[139-169]
### Suggested fix
Refactor `mergeAppleLocale` to preserve all existing locale properties via a spread, and then only special-case `keywords` to ensure it is cloned into a new mutable array.
For example:
- Start with `const mergedLocale: MutableDeep<AppleLocaleInfo> = { ...(existingLocale ?? {}) }` (preserves any future fields).
- If `existingLocale?.keywords` exists, set `mergedLocale.keywords = [...existingLocale.keywords]`.
- Overlay the draft fields (`title`, `subtitle`, `description`, `promotionalText`, and `keywords`), cloning `keywords` from the draft when present.
This keeps the runtime “preserve unknown fields” behavior while still avoiding aliasing the keywords array.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Superseded by land of tip stack #386 (same 12 domain commits). |
Stack 5/12 of re-split HOLD #307
Domain:
storeBase:
refactor/types/readonly-stack-04-providersFull green tip:
refactor/foundation/readonly-types-fullLand stack in order. Intermediate PRs may not typecheck alone.
Summary by cubic
Make store domain types readonly and add
MutableDeepwhere local mutation is needed, improving immutability and type safety across listing, offers, Game Center, Play, and reconcile flows. No functional changes expected; merges now avoid mutating shared arrays.readonly(screenshots, offers, privacy, availability, team roles, sync jobs, etc.).MutableDeepfor constructing mutable payloads (listings, planned actions, Play releases/products/subscriptions, ASC requests).mergeAppleLocaleto apply drafts safely and copykeywordsinto a new array.actionsare mutable handles;summarizenow acceptsreadonly.Written for commit 7edfce6. Summary will update on new commits.