fix: save org/app state on resume, enforce access checks, remove redundant prompts#569
Merged
WcaleNieWolny merged 5 commits intomainfrom Mar 24, 2026
Merged
fix: save org/app state on resume, enforce access checks, remove redundant prompts#569WcaleNieWolny merged 5 commits intomainfrom
WcaleNieWolny merged 5 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces the old resume mechanism with Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Init as initApp
participant Resume as tryResumeOnboarding
participant Local as LocalTmpState
participant DB as Supabase
participant AddApp as addAppStep
User->>Init: start init (apikey)
Init->>Resume: tryResumeOnboarding(apikey)
Resume->>Local: read tmp onboarding state
Local-->>Resume: { step_done, orgId, orgName, appId? }
alt valid state (orgId && step_done)
Resume->>User: prompt ["Yes, continue","No, start over"]
User-->>Resume: choice
alt continue
Resume->>DB: rpc('get_orgs_v7')
DB-->>Resume: orgs
Resume->>Resume: find org by gid === orgId
alt org found and access ok
Resume-->>Init: { stepDone, orgId, orgName, appId? }
else
Resume-->>Init: undefined
end
else start over
Resume-->>Init: undefined
end
else
Resume-->>Init: undefined
end
alt resume returned
Init->>Init: set globalOrgId/globalOrgName
Init->>Init: if appId present -> set globalAppId
Init->>Init: stepToSkip = stepDone
else fallback
Init->>Init: interactive org selection, stepToSkip = 0
end
alt running add-app step
Init->>AddApp: addAppStep(...)
AddApp-->>Init: { appId }
Init->>Init: set globalAppId = appId
Init->>Local: markStepDone(... includes appId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (2)
src/init/command.ts (2)
2347-2350: Minor:orgNamecould be undefined for legacy state files.If a user resumes from a state file created before this PR (which wouldn't have
orgNamesaved), the warning message will display "undefined" for the org name. Consider using a fallback.🔧 Suggested improvement
- pLog.warn(`Previously used organization "${resumed.orgName}" is no longer available. Please select a new one.`) + pLog.warn(`Previously used organization "${resumed.orgName || resumed.orgId}" is no longer available. Please select a new one.`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/init/command.ts` around lines 2347 - 2350, The warning message uses resumed.orgName which may be undefined for legacy state files; update the pLog.warn call in the else branch (where resumed and organization selection happens) to use a safe fallback (e.g., resumed.orgName || '<previous organization>' or similar) so the log never prints "undefined"; keep the rest of the flow (calling selectOrganizationForInit and resetting stepToSkip) unchanged.
434-436: Minor: Falsy check onstep_donecould reject step 0.The condition
!step_donewill be true ifstep_doneis0. While the current code never writesstep_done: 0to the state file (steps start at 1), this could be confusing if the behavior changes later. Consider using an explicit type check.🔧 Suggested improvement
- if (!orgId || !step_done) + if (!orgId || typeof step_done !== 'number' || step_done < 1) return undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/init/command.ts` around lines 434 - 436, The current destructure and guard uses a falsy check (!step_done) which will incorrectly reject a valid numeric 0; change the guard to explicitly check for null/undefined instead (e.g., test step_done == null or typeof step_done === 'undefined') while keeping the same orgId null/undefined check for orgId; update the conditional that follows the JSON.parse(rawData) destructure (referencing step_done, orgId, orgName, pathToPackageJson, channelName, platform, delta, currentVersion) so it only returns undefined when step_done or orgId are actually missing, not merely falsy.
🤖 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 2339-2351: The RPC call supabase.rpc('get_orgs_v7') in the resumed
branch doesn't check the returned error, so allOrganizations can be undefined on
API failure and the code wrongly warns the org is missing; update the resumed
path to inspect the RPC response error (like selectOrganizationForInit does),
log pLog.error with the RPC error when present, and only proceed to find
savedOrg when data exists—if an error occurred, surface the error and prompt
retry or fall back to selectOrganizationForInit (affecting variables
allOrganizations, savedOrg, organization, and stepToSkip).
---
Nitpick comments:
In `@src/init/command.ts`:
- Around line 2347-2350: The warning message uses resumed.orgName which may be
undefined for legacy state files; update the pLog.warn call in the else branch
(where resumed and organization selection happens) to use a safe fallback (e.g.,
resumed.orgName || '<previous organization>' or similar) so the log never prints
"undefined"; keep the rest of the flow (calling selectOrganizationForInit and
resetting stepToSkip) unchanged.
- Around line 434-436: The current destructure and guard uses a falsy check
(!step_done) which will incorrectly reject a valid numeric 0; change the guard
to explicitly check for null/undefined instead (e.g., test step_done == null or
typeof step_done === 'undefined') while keeping the same orgId null/undefined
check for orgId; update the conditional that follows the JSON.parse(rawData)
destructure (referencing step_done, orgId, orgName, pathToPackageJson,
channelName, platform, delta, currentVersion) so it only returns undefined when
step_done or orgId are actually missing, not merely falsy.
![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: 7a6d850bf4
ℹ️ 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".
src/init/command.ts
Outdated
Comment on lines
+2342
to
+2345
| const savedOrg = allOrganizations?.find(org => org.gid === resumed.orgId) | ||
| if (savedOrg) { | ||
| organization = savedOrg | ||
| pLog.info(`Using organization "${savedOrg.name}"`) |
There was a problem hiding this comment.
This branch accepts savedOrg from get_orgs_v7 and assigns it directly, which skips the admin-role and 2FA checks enforced in selectOrganizationForInit (normalizeRole filtering and enforcing_2fa guard). If a user’s permissions changed since the saved state (e.g., no longer admin, or 2FA required but not enabled), resume will continue with an org they can’t onboard and fail later instead of falling back to org selection as intended.
Useful? React with 👍 / 👎.
WcaleNieWolny
force-pushed
the
fix/save-org-on-resume
branch
from
7a6d850 to
e391b04
Compare
Previously the org selection prompt appeared every time, even when resuming. Now markStepDone persists orgId/orgName in the temp file, and tryResumeOnboarding reads it back — asking "resume?" before org selection. If the saved org is no longer available, falls back to the normal org picker and resets progress.
Old temp files without orgId are now explicitly flagged with a warning instead of silently discarding the saved state.
WcaleNieWolny
force-pushed
the
fix/save-org-on-resume
branch
from
e391b04 to
6d47ff2
Compare
Without this, resuming onboarding uses the appId from capacitor.config instead of the one created during step 1, causing subsequent steps to reference the wrong app.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/init/command.ts (1)
453-469:⚠️ Potential issue | 🟠 MajorValidate the saved org before hydrating the resume state.
Lines 453-468 restore
globalPathToPackageJson/globalChannelName/ related state before the org has been validated, while Lines 2348-2356 only check whether the savedgidexists. That leaves three bad paths here: an RPC failure gets reported as “org no longer available”, a downgraded or non-2FA-eligible org bypasses the same guards asselectOrganizationForInit(), and the fallbackstepToSkip = 0path can still carry stale resume state into the fresh flow. Only apply the saved globals afterget_orgs_v7succeeds and the org passes the normal admin/2FA checks; otherwise clear or rewrite the tmp state before falling back.Also applies to: 2341-2358
🧹 Nitpick comments (1)
src/init/command.ts (1)
474-476: UseformatError(err)in this CLI error path.Interpolating
errdirectly here will often degrade to[object Object]and makes the resume failure harder to understand.Proposed fix
catch (err) { - pLog.error(`Cannot read which steps have been completed, error:\n${err}`) + pLog.error(`Cannot read which steps have been completed, error:\n${formatError(err)}`) pLog.warn('Onboarding will continue but please report it to the capgo team!') return undefined }As per coding guidelines, "For user-visible error messages, format errors with
formatError(...)instead of dumping raw exceptions when possible".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/init/command.ts` around lines 474 - 476, Replace the raw error interpolation in the catch block that calls pLog.error with a formatted error using formatError(err); specifically update the pLog.error invocation (the one currently doing pLog.error(`Cannot read which steps have been completed, error:\n${err}`)) to pass formatError(err) so the message becomes the same descriptive text combined with formatError(err), ensuring you import or reference formatError where used.
🤖 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/command.ts`:
- Around line 474-476: Replace the raw error interpolation in the catch block
that calls pLog.error with a formatted error using formatError(err);
specifically update the pLog.error invocation (the one currently doing
pLog.error(`Cannot read which steps have been completed, error:\n${err}`)) to
pass formatError(err) so the message becomes the same descriptive text combined
with formatError(err), ensuring you import or reference formatError where used.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/init/command.ts (1)
431-485: Consider refactoring to reduce cognitive complexity.SonarCloud flags this function with cognitive complexity 18 (limit 15). While the logic is correct, extracting the state restoration (lines 457-474) into a helper would improve readability and comply with complexity thresholds.
♻️ Suggested refactor to extract state restoration
+function restoreGlobalStateFromResume( + pathToPackageJson?: string, + channelName?: string, + platform?: string, + delta?: boolean, + currentVersion?: string, + savedAppId?: string, +) { + if (pathToPackageJson) + globalPathToPackageJson = pathToPackageJson + if (channelName) + globalChannelName = channelName + if (platform === 'ios' || platform === 'android') + globalPlatform = platform + if (typeof delta === 'boolean') + globalDelta = delta + if (typeof currentVersion === 'string' && currentVersion.length > 0) + globalCurrentVersion = currentVersion + if (savedAppId) + globalAppId = savedAppId +} + async function tryResumeOnboarding(apikey: string): Promise<ResumeResult | undefined> { try { const rawData = readFileSync(getTmpObjectPath(), 'utf-8') if (!rawData || rawData.length === 0) return undefined const { step_done, orgId, orgName, appId: savedAppId, pathToPackageJson, channelName, platform, delta, currentVersion } = JSON.parse(rawData) if (!orgId || !step_done) { pLog.warn('⚠️ Found previous onboarding progress, but it was saved in an older format.') pLog.info(' Starting fresh. Your previous progress cannot be resumed.') return undefined } pLog.info(formatInitResumeMessage(step_done, initOnboardingSteps.length)) if (orgName) { pLog.info(` Organization: ${orgName}`) } const resumeChoice = await pSelect({ message: 'Would you like to continue from where you left off?', options: [ { value: 'yes', label: '✅ Yes, continue' }, { value: 'no', label: '❌ No, start over' }, ], }) await cancelCommand(resumeChoice, orgId, apikey) if (resumeChoice === 'yes') { - if (pathToPackageJson) { - globalPathToPackageJson = pathToPackageJson - } - if (channelName) { - globalChannelName = channelName - } - if (platform === 'ios' || platform === 'android') { - globalPlatform = platform - } - if (typeof delta === 'boolean') { - globalDelta = delta - } - if (typeof currentVersion === 'string' && currentVersion.length > 0) { - globalCurrentVersion = currentVersion - } - if (savedAppId) { - globalAppId = savedAppId - } + restoreGlobalStateFromResume(pathToPackageJson, channelName, platform, delta, currentVersion, savedAppId) return { stepDone: step_done, orgId, orgName, appId: savedAppId } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/init/command.ts` around lines 431 - 485, The tryResumeOnboarding function has a dense state-restoration block (setting globalPathToPackageJson, globalChannelName, globalPlatform, globalDelta, globalCurrentVersion, globalAppId from pathToPackageJson, channelName, platform, delta, currentVersion, savedAppId) that increases cognitive complexity; extract that logic into a small helper function (e.g., restoreOnboardingState(parsed: {pathToPackageJson?, channelName?, platform?, delta?, currentVersion?, appId?}) or restoreOnboardingStateFromSaved) which performs the conditional assignments and returns any needed values, then call this helper from tryResumeOnboarding (replace the inline assignments with a single call) so tryResumeOnboarding becomes simpler and under the complexity threshold.
🤖 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/command.ts`:
- Around line 431-485: The tryResumeOnboarding function has a dense
state-restoration block (setting globalPathToPackageJson, globalChannelName,
globalPlatform, globalDelta, globalCurrentVersion, globalAppId from
pathToPackageJson, channelName, platform, delta, currentVersion, savedAppId)
that increases cognitive complexity; extract that logic into a small helper
function (e.g., restoreOnboardingState(parsed: {pathToPackageJson?,
channelName?, platform?, delta?, currentVersion?, appId?}) or
restoreOnboardingStateFromSaved) which performs the conditional assignments and
returns any needed values, then call this helper from tryResumeOnboarding
(replace the inline assignments with a single call) so tryResumeOnboarding
becomes simpler and under the complexity threshold.
- Remove "Create channel X for Y in Capgo?" confirmation — user already picked the channel name, no need to ask again - Delete temp file when user chooses "No, start over" on the resume prompt so they aren't asked again on the next run - Fix "Done" → "done" in channel success message
- Validate saved org has admin/super_admin role before auto-resuming (permissions may have changed since state was saved) - Check enforcing_2fa guard on resume just like selectOrganizationForInit does - Handle RPC error from get_orgs_v7 in the resume path instead of silently treating API failures as "org no longer available" - Fall back to org selection with stepToSkip=0 in all failure cases
WcaleNieWolny
changed the title
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
orgId,orgName, andappIdto the temp file so resume doesn't fall back to defaults or re-ask org selectionget_orgs_v7errors instead of silently treating API failures as "org no longer available"orgIdshow a warning instead of silently starting freshTest plan
Summary by CodeRabbit
New Features
Bug Fixes