[codex] fix onboarding updater sync recovery#596
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOnboarding now enforces and verifies an explicit Changes
Sequence DiagramsequenceDiagram
participant User
participant Init as Init Workflow
participant State as Updater State Checker
participant PkgMgr as Package Manager
participant BuildSync as Build/Sync
User->>Init: start onboarding
Init->>Init: select & remember package.json
Init->>State: getUpdaterInstallState(packageJsonPath)
alt updater ready
State->>Init: ready
Init->>BuildSync: run build/sync
else updater not ready
State->>Init: details (missing declaration / not resolvable)
Init->>User: show remediation command(s)
User->>PkgMgr: run install or fix files
User->>Init: type "ready"
Init->>State: re-check updater state
alt ready after fix
State->>Init: ready
Init->>BuildSync: run build/sync
else still not ready
Init->>User: prompt manual fix or additional retry
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
riderx
force-pushed
the
codex/onboarding-updater-sync-guard
branch
from
8d8fa2e to
5da5414
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/init/command.ts (1)
1604-1696: SplitgetAssistedDependenciesinto smaller path-selection helpers.This function now mixes auto-detection, native picker fallback, tree navigation, manual entry, validation, and state mutation in one body, which makes the onboarding recovery path hard to change safely. Extracting the picker/tree/manual branches behind a
resolvePackageJsonPath()-style helper would bring the complexity back under control. As per coding guidelines, "Prefer small reusable helpers for parsing and normalization logic instead of repeating ad hoc parsing inside command bodies."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/init/command.ts` around lines 1604 - 1696, getAssistedDependencies is too large and mixes auto-detection, native file picker, tree selector, and manual entry plus validation and state mutation; extract the interactive selection logic into a new helper (e.g., resolvePackageJsonPath) and keep getAssistedDependencies focused on orchestration. Specifically, create resolvePackageJsonPath() that encapsulates the branches that call canUseFilePicker(), openPackageJsonPicker(), the tree selector loop using pSelect(), and the manual pText() path (including their validation and pIsCancel handling), return a normalized selected path or null, and update getAssistedDependencies to call resolvePackageJsonPath(), call rememberPackageJsonPath(selectedPath) where appropriate, and then reuse getAllPackagesDependencies(undefined, selectedPath) — keep existing helper names (rememberPackageJsonPath, openPackageJsonPicker, pConfirm, pSelect, pText, canUseFilePicker, getAllPackagesDependencies) so callers remain identifiable.
🤖 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/init/command.ts`:
- Around line 2521-2537: After waitForReadyConfirmation returns, do not proceed
to mark the step completed; instead force the outer loop to re-run the
build/sync by returning 'retry'. Concretely, change the branch after
waitForReadyConfirmation (where buildAndSyncCommand is referenced) so it returns
'retry' immediately rather than calling validateIosUpdaterSync,
handleBrokenIosSync, or promoteEncryptionSummaryToEnabled; keep
validateIosUpdaterSync, handleBrokenIosSync, promoteEncryptionSummaryToEnabled,
platform, pm.runner, cwd(), and globalPathToPackageJson available for the
subsequent retry flow.
---
Nitpick comments:
In `@src/init/command.ts`:
- Around line 1604-1696: getAssistedDependencies is too large and mixes
auto-detection, native file picker, tree selector, and manual entry plus
validation and state mutation; extract the interactive selection logic into a
new helper (e.g., resolvePackageJsonPath) and keep getAssistedDependencies
focused on orchestration. Specifically, create resolvePackageJsonPath() that
encapsulates the branches that call canUseFilePicker(), openPackageJsonPicker(),
the tree selector loop using pSelect(), and the manual pText() path (including
their validation and pIsCancel handling), return a normalized selected path or
null, and update getAssistedDependencies to call resolvePackageJsonPath(), call
rememberPackageJsonPath(selectedPath) where appropriate, and then reuse
getAllPackagesDependencies(undefined, selectedPath) — keep existing helper names
(rememberPackageJsonPath, openPackageJsonPicker, pConfirm, pSelect, pText,
canUseFilePicker, getAllPackagesDependencies) so callers remain identifiable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22c9d84b-3c9a-4ef4-9e03-2bd067139d80
📒 Files selected for processing (4)
skills/usage/SKILL.mdsrc/init/command.tssrc/init/updater.tstest/test-onboarding-recovery.mjs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5da5414883
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
riderx
force-pushed
the
codex/onboarding-updater-sync-guard
branch
from
5da5414 to
b8efbe8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/init/command.ts`:
- Line 1761: The inline regex in the destructuring assignment "const [command,
...args] = pm.installCommand.split(/\s+/).filter(Boolean)" should be hoisted to
module scope to satisfy the e18e/prefer-static-regex lint rule; add a top-level
constant (e.g. SPLIT_WHITESPACE_REGEX = /\s+/) and replace the inline /\s+/ with
that constant when calling pm.installCommand.split(...), leaving the rest of the
expression intact.
- Around line 1684-1701: The current getAssistedDependencies always calls
rememberPackageJsonPath(root) and returns dependencies for root, which
overwrites a valid globalPathToPackageJson and reverts to the repo root; change
the flow so if packageJsonPath (computed from globalPathToPackageJson ?? root)
points to a valid nested app (i.e. globalPathToPackageJson is set and produced
non-empty/adequate dependencies), call rememberPackageJsonPath(packageJsonPath)
and return dependencies for packageJsonPath instead of always using root; ensure
uses of getAllPackagesDependencies, packageJsonPath, root,
globalPathToPackageJson, rememberPackageJsonPath, and resolvePackageJsonPath are
updated accordingly so the previously selected package.json is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69079b94-da50-46a1-a4b3-31363654abe8
📒 Files selected for processing (4)
skills/usage/SKILL.mdsrc/init/command.tssrc/init/updater.tstest/test-onboarding-recovery.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/usage/SKILL.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8efbe82d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
riderx
force-pushed
the
codex/onboarding-updater-sync-guard
branch
from
b8efbe8 to
0cc78a6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/init/updater.ts (1)
44-46: UseObject.hasOwn()instead ofhasOwnProperty.The static analysis correctly identifies that the modern
Object.hasOwn()should be preferred overObject.prototype.hasOwnProperty.call().Suggested fix
- if (!dependencies || !Object.prototype.hasOwnProperty.call(dependencies, packageName)) + if (!dependencies || !Object.hasOwn(dependencies, packageName))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/init/updater.ts` around lines 44 - 46, Replace the legacy hasOwnProperty pattern with Object.hasOwn: in src/init/updater.ts where you check dependencies and packageName (variables packageJson, section, dependencies, packageName), change the guard that currently uses Object.prototype.hasOwnProperty.call(dependencies, packageName) to use Object.hasOwn(dependencies, packageName) so the condition becomes a check for missing dependencies or missing package via Object.hasOwn and then continue.
🤖 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/init/updater.ts`:
- Around line 44-46: Replace the legacy hasOwnProperty pattern with
Object.hasOwn: in src/init/updater.ts where you check dependencies and
packageName (variables packageJson, section, dependencies, packageName), change
the guard that currently uses Object.prototype.hasOwnProperty.call(dependencies,
packageName) to use Object.hasOwn(dependencies, packageName) so the condition
becomes a check for missing dependencies or missing package via Object.hasOwn
and then continue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 984368f3-adb2-4b63-9739-1b2da6f5fb4a
📒 Files selected for processing (4)
skills/usage/SKILL.mdsrc/init/command.tssrc/init/updater.tstest/test-onboarding-recovery.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/usage/SKILL.md
riderx
force-pushed
the
codex/onboarding-updater-sync-guard
branch
from
0cc78a6 to
38ffc24
Compare
riderx
force-pushed
the
codex/onboarding-updater-sync-guard
branch
from
38ffc24 to
fe86925
Compare
|
1 participant
Summary
@capgo/capacitor-updateris declared in the selectedpackage.jsonand resolvable fromnode_modulesreadyconfirmation and re-check when automatic updater install, manual install, build, or sync needs recoverycap sync, preserve iOS sync validation, and avoid marking the updater step complete just from selectingpackage.jsonRoot Cause
The onboarding flow trusted
getInstalledVersion(...), which can succeed from stale native project files or existingnode_modulesstate without proving the selectedpackage.jsondeclares@capgo/capacitor-updater. Manual install paths also continued without waiting for the user to finish and without re-checking.Validation
bun run lintbun run buildbun run test:mcpbun run test:bundlebun run test:onboarding-recoverySummary by CodeRabbit
New Features
Documentation
Tests