Skip to content

fix(cli/build init): route to recovery when no profiles match this app#2308

Open
WcaleNieWolny wants to merge 54 commits into
mainfrom
fix/cli-pick-profile-empty-recovery
Open

fix(cli/build init): route to recovery when no profiles match this app#2308
WcaleNieWolny wants to merge 54 commits into
mainfrom
fix/cli-pick-profile-empty-recovery

Conversation

@WcaleNieWolny
Copy link
Copy Markdown
Contributor

@WcaleNieWolny WcaleNieWolny commented May 21, 2026

Summary

Two UX fixes for capgo build init --platform ios (import-existing path):

1. Empty picker → no-match-recovery (the main fix)

When the user has profiles for the identity they picked but none match the current app's bundle ID (or distribution mode), the picker showed:

Pick a provisioning profile (0 matching this app's bundle ID and app_store distribution):
(1 other profile hidden — wrong bundle ID or distribution mode)
↩️   Back to identity selection

That's a dead end. The user already has the import-no-match-recovery screen (with Fetch from Apple, Create new App Store profile via Apple, and Open Developer Portal options) — we just weren't routing them there.

Now the filterProfilesForApp(profiles, appId, importDistribution) filter runs at:

  • identity selection — the natural choke point
  • import-pick-profile entry — defense-in-depth covering Apple-fetch results, resume, and back-navigation

If nothing survives, we jump straight to import-no-match-recovery. The alert wording there now distinguishes the two cases so the user understands what's actually wrong:

  • No profiles at all → "No provisioning profile on this Mac is linked to ''."
  • Profiles exist but none match → "No provisioning profile on this Mac matches this app (com.example.app) for app_store distribution under ''."

This is what the user asked for: when the local list is empty for this app, offer to create the profile via the Apple API using their .p8 key (already implemented; we just needed to send users there).

2. Dedupe consecutive identical log lines

✔ Distribution · app_store could appear 3× in the rendered log. addLog now collapses consecutive identical entries — covers this symptom plus any other repeats from resume / back-navigation.

Refactor

Extracted the bundleId + distribution filter (previously inline in 3 places in app.tsx) as filterProfilesForApp in macos-signing.ts. Added 5 unit tests covering matching, mismatched bundleId, mismatched distribution, null/undefined distribution, and empty input.

Test plan

  • bun run typecheck — clean
  • bunx eslint src/build/onboarding/ui/app.tsx src/build/onboarding/macos-signing.ts — only 7 pre-existing errors remain on main, none introduced by this PR
  • bun test/test-macos-signing.mjs — 23 tests pass (including 5 new filterProfilesForApp tests)
  • bun test/test-onboarding-recovery.mjs — all pass
  • bun test/test-apple-api-import-helpers.mjs — all pass
  • Manual: run capgo build init on a Mac with a distribution cert whose only profiles are for a different bundle ID — verify the flow lands at import-no-match-recovery with the new alert wording and "Create a new App Store profile via Apple" option
  • Manual: repeatedly enter / re-enter the Distribution selector — verify only one ✔ Distribution · … line appears

Summary by CodeRabbit

  • New Features

    • Improved provisioning-profile matching to avoid empty pickers and add a “switch to create new” recovery (persists choice) plus a macOS-only “switch back to import” escape
    • Visible “Checking certificate on Apple” onboarding step to pre-validate identities
    • Broader distribution-certificate matching and deduplicated log rendering
    • Clearer recovery messaging and pre-filled edit inputs when retrying verification
  • Tests

    • Added unit tests for provisioning profile filtering behavior

Review Change Stack

Two UX fixes for the iOS import-existing onboarding flow:

1. The provisioning-profile picker filters profiles by appId +
   distribution mode. When the filter removed all profiles, the user was
   dropped at an empty picker with only "Back" — a dead end. Now the
   filter runs early (at identity selection AND defensively when entering
   pick-profile from any source) and routes to import-no-match-recovery
   instead, where the user can fetch / create the profile via the Apple
   API with their .p8 key, or open the developer portal manually.

   The recovery alert wording now distinguishes "no profiles at all for
   this identity" from "profiles exist but none match this app's bundle
   ID / distribution mode" so the user knows what's actually wrong.

2. The "✔ Distribution · app_store" log line could appear multiple times
   in the output. addLog now dedupes consecutive identical entries, which
   covers the immediate symptom plus any other repeated entries from
   resume / back navigation.

The bundleId + distribution filter is extracted as filterProfilesForApp
in macos-signing.ts (used in 3 places in app.tsx), with unit tests
covering matching, mismatched bundleId, mismatched distribution,
null/undefined distribution, and empty input.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 21, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds filterProfilesForApp (filters provisioning profiles by bundleId and optionally by distribution) with unit tests; integrates it into the macOS import UI to populate profile pickers, guard routing to recovery when no usable profiles exist, expand recovery messaging and actions, deduplicate consecutive log entries, and add an Apple cert checking onboarding step.

Changes

Profile Filtering Helper and macOS Import Flow

Layer / File(s) Summary
Profile filtering helper and unit tests
cli/src/build/onboarding/macos-signing.ts, cli/test/test-macos-signing.mjs
New filterProfilesForApp exported function filters provisioning profiles by bundleId and optionally by importDistribution (profileType), treating null/undefined as accepting any type. Unit tests validate bundleId+distribution matches, bundleId mismatch, distribution mismatch, null/undefined distribution behavior, and empty input handling.
Onboarding step and UI integration
cli/src/build/onboarding/types.ts, cli/src/build/onboarding/ui/app.tsx, cli/src/build/onboarding/ui/components.tsx
Adds 'import-checking-apple-cert' onboarding step and progress/label entries. Imports filterProfilesForApp into the UI, deduplicates consecutive log entries, guards entry to import-pick-profile by routing to import-no-match-recovery when no usable profiles exist, replaces inline filtering with the helper, adds an Apple-side cert pre-check step and tri-state appleCertIdForChosen, changes missing-Apple-cert error paths to log+route to recovery, expands import-no-match-recovery messaging and actions (including switch-create-new), adds macOS escape hatches for switching flows, and pre-fills FilteredTextInput on retry/edit.
Apple API certificate query
cli/src/build/onboarding/apple-api.ts
listDistributionCerts now queries both DISTRIBUTION and IOS_DISTRIBUTION with limit=200; added comments documenting rationale and exclusion of managed certs.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • Cap-go/capgo#2211: Introduced the macOS import-from-this-Mac provisioning-profile picker flow that this PR's filtering logic augments and refines.

Poem

🐰 I hopped through profiles, tidy and spry,
Matched bundles true beneath the sky,
When none fit, I showed the right way,
Tests hopped in to bless the play,
The import trail now hops okay.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately captures the main fix of routing to recovery when no profiles match the app, which is the primary focus of this PR.
Description check ✅ Passed The description covers all required sections: a detailed summary of the changes, a comprehensive test plan with specific steps, and a checklist with testing results. However, screenshots are not applicable since this is a backend/CLI change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented May 21, 2026

Merging this PR will not alter performance

✅ 43 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing fix/cli-pick-profile-empty-recovery (1b9201c) with main (49709c8)

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

…empty-recovery

# Conflicts:
#	cli/src/build/onboarding/ui/app.tsx
When the user picks "Fetch matching profile from Apple now" or "Create a
new App Store profile via Apple" from import-no-match-recovery, and
findCertIdBySha1 returns null (Apple's /certificates response doesn't
include a SHA1 match for the local Keychain cert), the handler used to
throw an error like:

    Apple does not have a certificate matching the Keychain identity
    "Apple Distribution: digital shift oü (UVTJ336J2D)". Either it was
    revoked on Apple's side or it was never uploaded. Use "Create new"
    instead.

handleError caught it and dumped the user at the generic support-bundle
error screen, whose only options ("Retry" / "Restart onboarding") are
useless here — Retry runs the same failing call again, and the user
shouldn't have to nuke their progress to escape.

Both call sites (import-fetching-profile and import-create-profile-only)
now do what the sibling "Apple has the cert but no profiles linked"
branch already does: addLog a yellow warning and setStep back to
import-no-match-recovery so the user can pick a different recovery path
(Open Developer Portal, Back to identity selection, Exit) without
restarting.

Also softened the wording — the error message claimed Apple revoked the
cert or never had it, but a frequent real cause is that
listDistributionCerts only filters for the legacy IOS_DISTRIBUTION type
and excludes newer cross-platform DISTRIBUTION certs (shown as "Apple
Distribution:" in Keychain). A follow-up commit will broaden that
filter; this commit just stops the dead-end so users aren't stuck while
the deeper fix lands.
…ery menu

The no-match-recovery menu used to offer "Fetch matching profile from
Apple" and "Create a new App Store profile via Apple" unconditionally,
even when Apple's API can't find the chosen Keychain cert at all — in
which case both options fail with the same error and the user is stuck
clicking actions that can never succeed.

Now, after the user picks an identity with no matching local profile
AND we have a verified ASC API key, the flow lands on a new step
`import-checking-apple-cert` (brief spinner: "Checking with Apple for
<identity>…") which runs `findCertIdBySha1` once and stores the result
in `appleCertIdForChosen` state. The recovery menu then curates its
options from that result:

  • Apple has the cert (string)  → show Fetch + Create + Open Portal +
    Back. (Most users, especially once the lookup filter is broadened
    in a follow-up.)
  • Apple lacks the cert (null)  → hide Fetch + Create (they can't
    succeed). Surface a new "Switch to Create new" option that exits
    the import flow, reuses the already-verified .p8, and routes to
    `creating-certificate` to generate a fresh cert + profile via
    Apple. No Keychain side effects — the orphan local cert stays put.
  • Not checked yet (undefined)  → ad_hoc without ASC key. Falls back
    to the legacy "Provide ASC API key, then …" labels (which route
    through api-key-instructions before retrying the action), so we
    don't pessimistically demand a key from ad_hoc users.

The alert + body copy also branch on the pre-check result so users see
why the API-driven options aren't shown when the cert isn't on Apple.
Identity selection resets `appleCertIdForChosen` so a re-pick triggers
a fresh check.

`import-pick-profile`'s defense-in-depth guard (back-navigation /
resume / Apple-fetch with no matches) now routes through the same
pre-check, so every entry point into recovery goes through the same
gate. If a previous check is already cached for this identity, the
gate skips the re-query to avoid a redundant round-trip.

No change to the failing branches in `import-fetching-profile` /
`import-create-profile-only` themselves — they remain as defensive
guards (the menu just doesn't offer them when they'd fail).
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cli/src/build/onboarding/ui/app.tsx`:
- Around line 197-211: The apple-cert tri-state (appleCertIdForChosen) must be
authoritative for all recovery flows: ensure any path that performs an ASC
lookup (including the "provide ASC API key, then fetch/create" flow) either
routes through the existing import-checking-apple-cert step or writes the lookup
result into appleCertIdForChosen via setAppleCertIdForChosen before navigating
back to the recovery UI; update the handlers that trigger Apple lookups so they
persist the returned string/null into appleCertIdForChosen (and reset to
undefined only when the chosen identity truly changes) — apply the same change
to the other occurrence referenced around the alternate block.
- Around line 1717-1744: The UI is offering the "switch-create-new" flow for
imports with importMode === 'ad_hoc', which improperly alters distribution;
update the option logic so switchToCreateNewOption (and any branch that pushes
the 'switch-create-new' value) is only added when importMode !== 'ad_hoc'.
Locate the switchToCreateNewOption definition and the other branches flagged
(around the fetch/create option blocks and the other ranges that reference
'switch-create-new') and add a guard using importMode (or skip when importMode
=== 'ad_hoc') so the 'switch-create-new' choice is never presented for ad_hoc
imports. Ensure labels/values for fetch/create remain unchanged for ad_hoc.
🪄 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: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: b8476d2e-7c8a-4bfa-b462-49581a029e68

📥 Commits

Reviewing files that changed from the base of the PR and between a055a62 and 0082d88.

📒 Files selected for processing (2)
  • cli/src/build/onboarding/types.ts
  • cli/src/build/onboarding/ui/app.tsx

Comment thread cli/src/build/onboarding/ui/app.tsx Outdated
Comment thread cli/src/build/onboarding/ui/app.tsx Outdated
…ths)

Two related UX fixes for users who got stuck in the create-new path
after clicking a button that looked like simple navigation:

1. Rename the destructive "Cancel and use Create new instead" buttons
   (in import-distribution-mode picker and import-pick-identity
   picker) to "Switch to 'Create new' (Apple generates a fresh cert
   + profile)". Both old buttons silently committed
   setupMethod=create-new to the on-disk progress file, then routed to
   api-key-instructions — which on Ctrl+C+rerun put the user into
   creating-certificate directly, and if their Apple team was at the
   3-cert limit, into cert-limit-prompt with no obvious way out. The
   old label read like navigation; the behaviour was a permanent state
   mutation. New label says what actually happens.

2. Add "Switch back to Import existing" escape options at the two
   places users land when stuck in this trap:

   - cert-limit-prompt: alongside the "Revoke an existing cert" rows,
     offer "Switch back to Import existing (use a cert from your
     Keychain)". Reverses setupMethod=create-new in progress and routes
     to import-scanning. Targets the exact symptom the user hit:
     resumed into creating-certificate, hit the cert limit, no way
     back to the import flow that would have just worked with their
     existing Keychain cert.

   - api-key-instructions: when the user is not currently in import
     mode AND we're on macOS, append a "Switch to Import existing
     (use a cert from your Keychain instead)" option to the file-input
     Select. Same persistence semantics as the cert-limit-prompt
     escape. Hidden on non-macOS hosts because the import flow needs
     Keychain access.

The underlying root cause (the lookup filter that makes the cert
appear "not on Apple") will be fixed in a follow-up commit on the
same PR. These two changes just stop the user being stuck while we
land that.
listDistributionCerts was filtering only IOS_DISTRIBUTION, the legacy
iOS-specific cert type. Apple deprecated it around 2021 in favor of the
newer cross-platform DISTRIBUTION type — and new certs created from
App Store Connect default to DISTRIBUTION. macOS reports these in
Keychain as "Apple Distribution: <team>" (vs. the old "iPhone
Distribution: <team>" for IOS_DISTRIBUTION).

A team's cert ledger almost always has both types after a few years.
The old filter silently excluded the newer ones, causing
findCertIdBySha1 to return null for any local "Apple Distribution:"
identity — which surfaced as the misleading "Apple does not have a
certificate matching the Keychain identity" error in the no-match
recovery flow, even though the cert was right there in the Developer
Portal.

Concrete fix:
  - Add DISTRIBUTION to the filter (comma-separated; Apple's API
    accepts the multi-value syntax).
  - Bump limit from 10 to 200 (Apple's documented max). Avoids any
    pagination concerns for teams with deep cert histories — the per-
    team active-cert limit is far lower than 200, but expired and
    revoked rows also count toward this endpoint's pagination.
  - Comment notes that DISTRIBUTION_MANAGED is intentionally excluded
    because those certs are Apple-HSM-signed (Xcode Cloud / managed
    signing) and can't be used to sign builds on third-party CI. They
    will still surface in the upcoming Available/Unavailable table view
    (Phase 2) marked as unsignable.

For your specific case ("Apple Distribution: digital shift oü
(UVTJ336J2D)"), this is the actual cure: findCertIdBySha1 will now
find your cert, the no-match-recovery flow will offer Fetch + Create
(not Switch-to-Create-new), and the existing happy path will work
end-to-end.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
cli/src/build/onboarding/ui/app.tsx (3)

1558-1565: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use the filtered profile count in the identity picker.

m.profiles.length still counts profiles for other bundle IDs and distributions, so this row can say "matching profiles" and then immediately route to no-match recovery after selection. Derive the count from filterProfilesForApp(...) instead.

Suggested fix
               ...importMatches.map((m) => {
-                const matchCount = m.profiles.length
+                const matchCount = filterProfilesForApp(m.profiles, appId, importDistribution).length
                 const label = matchCount > 0
                   ? `🔑  ${m.identity.name} · ${matchCount} matching profile${matchCount === 1 ? '' : 's'}`
                   : `🔑  ${m.identity.name} · ⚠ no matching profiles on this Mac (recovery available)`
                 return { label, value: m.identity.sha1 }
               }),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/src/build/onboarding/ui/app.tsx` around lines 1558 - 1565, The picker
label currently uses m.profiles.length which counts all profiles for an
identity; change it to compute the count via filterProfilesForApp(...) so only
profiles matching the current app and distribution are counted. Replace uses of
m.profiles.length in the importMatches.map label construction with the
filteredProfiles.length (e.g., const filtered = filterProfilesForApp(m.profiles,
bundleId, distribution); const matchCount = filtered.length) and keep the rest
of the label logic the same, leaving the returned value as m.identity.sha1.

2234-2266: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Hide the Import-existing recovery path off macOS.

cert-limit-prompt is reachable on Linux/Windows too, but this option always routes to import-scanning, which depends on the macOS Keychain flow. On non-macOS hosts it offers a dead-end recovery path.

Suggested fix
             options={[
               ...existingCerts.map((c) => {
                 const ourCertId = certData?.certificateId || initialProgress?.completedSteps.certificateCreated?.certificateId
                 const isOurs = ourCertId === c.id
                 const creator = isOurs ? ' · 🔧 Created by Capgo' : ''
                 return {
                   label: `🗑️   Revoke ${c.name} · expires ${c.expirationDate.split('T')[0]}${creator}`,
                   value: c.id,
                 }
               }),
-              { label: '🔄  Switch back to Import existing (use a cert from your Keychain)', value: '__switch-import__' },
+              ...(isMacOS()
+                ? [{ label: '🔄  Switch back to Import existing (use a cert from your Keychain)', value: '__switch-import__' }]
+                : []),
               { label: '✖  Exit onboarding', value: '__exit__' },
             ]}
             onChange={async (value) => {
               if (value === '__exit__') {
                 addLog(`Exiting. Revoke a certificate manually in App Store Connect, then resume with ${buildInitCommand}.`, 'yellow')
                 exitOnboarding()
                 return
               }
-              if (value === '__switch-import__') {
+              if (value === '__switch-import__' && isMacOS()) {
                 // Reverse the destructive setupMethod=create-new commit that
                 // happens when the user clicks "Switch to Create new" from
                 // the import flow. Lets users back out of the create-new
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/src/build/onboarding/ui/app.tsx` around lines 2234 - 2266, The "Switch
back to Import existing" option and its handler should be gated to macOS so
non-mac hosts don't get routed to the dead-end import-scanning flow: update the
options array creation to include the { label: '🔄  Switch back to Import
existing ...', value: '__switch-import__' } item only when running on macOS
(e.g. process.platform === 'darwin' or an isMac flag), and in the onChange
branch for value === '__switch-import__' add a platform guard that logs a
friendly message and aborts if not macOS; keep the existing loadProgress,
saveProgress, setImportMode, setStep and addLog calls for the allowed macOS
path.

1565-1579: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't offer Switch to "Create new" after ad_hoc is selected.

This branch jumps into creating-certificate, which only creates App Store credentials in this flow. If importDistribution === 'ad_hoc', selecting it silently changes the requested distribution.

Suggested fix
-              { label: '🆕  Switch to "Create new" (Apple generates a fresh cert + profile)', value: '__cancel__' },
+              ...(importDistribution !== 'ad_hoc'
+                ? [{ label: '🆕  Switch to "Create new" (Apple generates a fresh cert + profile)', value: '__cancel__' }]
+                : []),
             ]}
             onChange={async (value) => {
-              if (value === '__cancel__') {
+              if (value === '__cancel__' && importDistribution !== 'ad_hoc') {
                 setImportMode(false)
                 // Persist the switch so a CLI restart doesn't resume into
                 // the import flow the user just abandoned. Mirrors the same
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/src/build/onboarding/ui/app.tsx` around lines 1565 - 1579, The "Switch to
'Create new'" branch currently runs when users previously chose
importDistribution === 'ad_hoc', which silently converts their request to App
Store flow; guard this by checking importDistribution before offering or
applying the switch: either remove the option from the choices when
importDistribution === 'ad_hoc' or, in the onChange handler for value
'__cancel__', abort the transition if the loaded progress (from
loadProgress(appId)) has existing.importDistribution === 'ad_hoc' — do not call
setImportMode(false), do not set existing.setupMethod = 'create-new', do not
delete existing.importDistribution, and do not setStep('api-key-instructions')
in that case; use the existing symbols setImportMode, loadProgress,
saveProgress, setStep and existing.importDistribution to implement the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@cli/src/build/onboarding/ui/app.tsx`:
- Around line 1558-1565: The picker label currently uses m.profiles.length which
counts all profiles for an identity; change it to compute the count via
filterProfilesForApp(...) so only profiles matching the current app and
distribution are counted. Replace uses of m.profiles.length in the
importMatches.map label construction with the filteredProfiles.length (e.g.,
const filtered = filterProfilesForApp(m.profiles, bundleId, distribution); const
matchCount = filtered.length) and keep the rest of the label logic the same,
leaving the returned value as m.identity.sha1.
- Around line 2234-2266: The "Switch back to Import existing" option and its
handler should be gated to macOS so non-mac hosts don't get routed to the
dead-end import-scanning flow: update the options array creation to include the
{ label: '🔄  Switch back to Import existing ...', value: '__switch-import__' }
item only when running on macOS (e.g. process.platform === 'darwin' or an isMac
flag), and in the onChange branch for value === '__switch-import__' add a
platform guard that logs a friendly message and aborts if not macOS; keep the
existing loadProgress, saveProgress, setImportMode, setStep and addLog calls for
the allowed macOS path.
- Around line 1565-1579: The "Switch to 'Create new'" branch currently runs when
users previously chose importDistribution === 'ad_hoc', which silently converts
their request to App Store flow; guard this by checking importDistribution
before offering or applying the switch: either remove the option from the
choices when importDistribution === 'ad_hoc' or, in the onChange handler for
value '__cancel__', abort the transition if the loaded progress (from
loadProgress(appId)) has existing.importDistribution === 'ad_hoc' — do not call
setImportMode(false), do not set existing.setupMethod = 'create-new', do not
delete existing.importDistribution, and do not setStep('api-key-instructions')
in that case; use the existing symbols setImportMode, loadProgress,
saveProgress, setStep and existing.importDistribution to implement the guard.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 538011dd-343a-45e0-83bf-e602fc68756c

📥 Commits

Reviewing files that changed from the base of the PR and between 0082d88 and 42fad7f.

📒 Files selected for processing (2)
  • cli/src/build/onboarding/apple-api.ts
  • cli/src/build/onboarding/ui/app.tsx

When verifying-key failed (e.g. user typed the Issuer UUID into the
Key ID field by mistake, or vice versa — common given they're labelled
similarly in App Store Connect), the error screen offered only:

  🔄  Try again              ← runs verifying-key with same bad values
  ↩️   Restart onboarding     ← nukes ALL progress, retype everything
  ❌  Exit

"Try again" was useless because nothing changed. "Restart" was
overkill — the user just wanted to fix a typo, not re-pick their .p8,
distribution mode, etc.

Three changes:

1. FilteredTextInput now accepts an `initialValue` prop. The input
   pre-fills with this value, backspace works to delete from it, and
   submit returns the (possibly edited) string. Backwards compatible —
   default is empty string, existing call sites unchanged.

2. The input-key-id and input-issuer-id steps pass the current `keyId`
   / `issuerId` state as initialValue, so re-entering those steps
   shows the value the user already typed (or had auto-detected from
   the .p8 filename) ready to edit, not a blank field.

3. The error screen's Select now includes two explicit edit options —
   `✏️ Edit Key ID (currently: <value>)` and `✏️ Edit Issuer ID
   (currently: <value>)` — but ONLY when the failed step was
   `verifying-key`. Picking either clears the error and routes
   straight to the relevant input step with the value pre-filled. No
   .p8 re-selection, no UUID retype.

Now the typical typo-fix workflow takes one keypress (down-arrow to
"Edit Issuer ID") + backspaces to fix the value + Enter, instead of
abandoning the whole session.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cli/src/build/onboarding/ui/app.tsx (1)

655-659: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Missing cancelled check before setStep in async IIFE.

Other async blocks in this useEffect check if (cancelled) return after async operations. This block calls await loadProgress(appId) but doesn't guard the subsequent setStep against cleanup.

Consistency fix
         else {
           ;(async () => {
             const apiKeyAvailable = !!(p8ContentRef.current || (await loadProgress(appId))?.completedSteps?.apiKeyVerified)
+            if (cancelled)
+              return
             setStep(apiKeyAvailable ? 'import-checking-apple-cert' : 'import-no-match-recovery')
           })()
         }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/src/build/onboarding/ui/app.tsx` around lines 655 - 659, The async IIFE
sets state after awaiting loadProgress without checking for component cleanup;
update the IIFE that uses p8ContentRef and loadProgress(appId) so it returns
early if the effect has been cancelled (check the existing cancelled flag)
before calling setStep with 'import-checking-apple-cert' or
'import-no-match-recovery'; ensure you mirror the same "if (cancelled) return"
pattern used in other async blocks in this useEffect to avoid setting state on
an unmounted component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@cli/src/build/onboarding/ui/app.tsx`:
- Around line 655-659: The async IIFE sets state after awaiting loadProgress
without checking for component cleanup; update the IIFE that uses p8ContentRef
and loadProgress(appId) so it returns early if the effect has been cancelled
(check the existing cancelled flag) before calling setStep with
'import-checking-apple-cert' or 'import-no-match-recovery'; ensure you mirror
the same "if (cancelled) return" pattern used in other async blocks in this
useEffect to avoid setting state on an unmounted component.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fd9eca78-93db-4627-b36c-46f94e12b7af

📥 Commits

Reviewing files that changed from the base of the PR and between 42fad7f and cd20598.

📒 Files selected for processing (2)
  • cli/src/build/onboarding/ui/app.tsx
  • cli/src/build/onboarding/ui/components.tsx

…very

Third edit option alongside "Edit Key ID" and "Edit Issuer ID" for the
case where the wrong .p8 file was selected entirely (revoked key, file
from wrong Apple account, picked an outdated AuthKey_*.p8, etc.). The
shipped error screen offered only the two ID-edit paths, which left the
user stuck if the actual mistake was the key file itself.

Routes to api-key-instructions (the same step that hosts the file
picker / manual-path Select), with pickerOpenedRef reset so the macOS
file picker can re-open from a clean state. After a new .p8 is picked,
the auto-detection in p8-method-select repopulates Key ID from the
filename pattern; Issuer ID stays unchanged because it's account-
scoped, not key-scoped.

Label shows the basename of the current path (currently: foo.p8) so
the user can confirm what's about to be replaced.
Apple Key IDs are always exactly 10 alphanumeric chars uppercase (e.g.
"KDTXMK292V"). Issuer IDs are standard UUIDs — hex + hyphens, 36 chars.
Both have rigid formats, but the input fields accepted arbitrary text,
which is how users end up pasting "0cd4db4a-5598-45b8-9d32-75cdf127d005"
into the Key ID field — verifying-key fails 1-2s later with no
indication that the input was structurally invalid.

FilteredTextInput now supports three new optional props:

- `allowedPattern: RegExp` — per-character whitelist. Anything that
  doesn't match is dropped on insert. Lets fields with tight formats
  refuse invalid keystrokes / pastes at the input level.
- `maxLength: number` — hard cap on buffer length, silently truncates
  past-cap characters. Paste-safe: pasting "Key ID: KDTXMK292V foo"
  truncates to "KDTXMK292V" after filter strips the spaces, colon, and
  trailing word.
- `transform: (s) => s` — post-filter transform on the full buffer.
  Used to force uppercase for Apple Key IDs (case-insensitive on
  Apple's side but uppercase by convention; auto-uppercasing prevents
  a class of "I typed lowercase, why doesn't it work" tickets).

Pipeline order: blacklist filter → allowedPattern whitelist → maxLength
truncate → transform. Pulled out as a pure `applyConstraints` helper
so initialValue prefill goes through the same path as user keystrokes
(otherwise an initialValue with invalid chars would appear briefly
before being filtered).

Visual: when maxLength is set and the field isn't masked, append a
dim "n/max" counter to the right of the cursor so the user sees how
many characters they have left.

Applied to both Key ID input variants (auto-detected-from-filename
and manual) with `allowedPattern=/[a-zA-Z0-9]/`, `maxLength=10`,
`transform=toUpperCase`. Updated the placeholder to "KDTXMK292V" so
the example shows the correct format.

Applied to Issuer ID input with `allowedPattern=/[a-fA-F0-9-]/`,
`maxLength=36`. No transform — UUIDs are case-insensitive on Apple's
side, and lowercase is the more common copy-paste form.

Net effect: pasting the issuer UUID into the Key ID field now
truncates to the first 10 valid alphanumeric chars (the hyphens
and characters past position 10 are silently dropped), so the
typo we just saw — both fields reading the same UUID — becomes
impossible.
Editing a field surfaced both the old AND the new value in the log:

  ✔ Key file selected · /path/to/AuthKey_66FGQZB566.p8
  ✔ Key ID · 0cd4db4a-5598-45b8-9d32-75cdf127d005   ← original typo
  ✔ Issuer ID · 0cd4db4a-5598-45b8-9d32-75cdf127d005
  ✔ Key ID · 66FGQZB566                              ← after edit
  ✔ Issuer ID · 0cd4db4a-5598-45b8-9d32-75cdf127d005 ← re-confirmed (same)

The "audit trail" of every keystroke is noise — the user wants to see
the current state, not the history.

New `upsertLog(prefix, text, color)` helper alongside `addLog`. Finds
the first existing log entry whose text starts with `prefix` and
rewrites it in place; otherwise appends. Used for field-update events
that the user can re-enter mid-session:

  - "✔ Key file selected · …" / "✔ Key file found · …"  (prefix: "✔ Key file")
  - "✔ Key ID · …"                                      (prefix: "✔ Key ID · ")
  - "✔ Issuer ID · …"                                   (prefix: "✔ Issuer ID · ")
  - "✔ Distribution · …"                                (prefix: "✔ Distribution · ")
  - "✔ Identity · …"                                    (prefix: "✔ Identity · ")
  - "✔ Profile · …"                                     (prefix: "✔ Profile · ")

The Key file prefix is intentionally short ("✔ Key file") so a switch
between the file-picker variant ("Key file selected") and the manual-
path variant ("Key file found") upserts the same row instead of
stacking them.

Pure `addLog` is preserved for one-shot events that don't have a
field semantics — "API Key verified", "Distribution certificate
created", "Provisioning profile created", error toasts, etc.

Net effect after editing the user's reported typo:

  ✔ Key file selected · /path/to/AuthKey_66FGQZB566.p8
  ✔ Key ID · 66FGQZB566       ← only the current value
  ✔ Issuer ID · <uuid>        ← only the current value
  ✔ API Key verified — Key: 66FGQZB566
…dation

The single-Select identity picker mixed valid and Apple-unrecognized
certs into one list, then surfaced "this cert isn't on Apple" only
after the user clicked an Apple-API-dependent recovery option. Three
changes wire this up properly so users only see options that can
succeed:

1) **Classifier helper** (`apple-api.ts` + tests):
   `classifyCertAvailability({ localExpirationDate?, isManaged?,
   appleCertId, lookupError? })` returns
   `{ available, reason, reasonText, appleCertId? }` with stable
   reason codes: expired, managed, not-visible, check-failed,
   no-private-key. Local-side disqualifiers (expired date, managed
   type) short-circuit before consulting the lookup so we don't burn
   API calls on certs we already know can't sign. Returns neutral
   wording for null lookup results — does NOT claim revocation we
   can't prove from the response. 7 unit tests cover every branch
   including malformed-date tolerance and non-Error throws.

2) **Eager batch validation** (new step `import-validating-all-certs`):
   After verifying-key succeeds in the import flow, a fan-out spinner
   step runs `findCertIdBySha1` in parallel across every scanned
   Keychain identity (typically 1-3, capped to whatever the user has).
   Each lookup is wrapped so a network blip on one cert doesn't
   disqualify the others. Results feed `classifyCertAvailability`
   into an `identityAvailability` Record keyed by SHA1, then the
   picker step renders. Skipped for ad_hoc users without an ASC API
   key — they fall through to the single-list layout as before.

3) **Two-table picker UI** (`import-pick-identity` rewrite):

   ```
   ✅  AVAILABLE (1)
   ────────────────────────────────────────────────────────────
   NAME                                         TEAM         PROFILES (matching/total)
   ────────────────────────────────────────────────────────────
   ▶ 🔑 Apple Distribution: digital shift oü   UVTJ336J2D   2/5 ✓

   [🆕  Switch to "Create new" ...]
   [✖  Exit onboarding]

   ⚠️   UNAVAILABLE (2)
   ────────────────────────────────────────────────────────────
   NAME                                         TEAM         REASON
   ────────────────────────────────────────────────────────────
   🔒 Apple Distribution: revoked cert         UVTJ336J2D   Not visible to current API key (revoked, different team, or lookup limitation)
   🔒 Distribution Managed                     UVTJ336J2D   Apple-managed — can't sign locally

   💡 Unavailable certificates can't be used to sign builds. Even
   downloading them from the Apple Developer Portal won't help —
   the private key was only on the Mac that generated the original
   CSR. Use "Create new" above to generate a fresh cert + profile
   that Apple recognizes.
   ```

   Only AVAILABLE rows are inside the `<Select>` (keyboard cursor
   skips Unavailable rows entirely). UNAVAILABLE renders as a
   display-only Box block below the picker, with the user-supplied
   footnote about why downloading from the portal won't fix things
   (the private key never left the originating Mac).

   Column widths fixed for ~80-col terminals; long names truncate
   with `…`. The PROFILES column shows `matching/total` so users
   with one cert linked to many apps can see at a glance which has a
   ready-to-use local profile.

   Cached `appleCertId` from batch validation flows through to
   `appleCertIdForChosen` on identity selection, so the downstream
   per-identity pre-check (when routing to no-match-recovery) skips
   the redundant network round-trip.

The previous Phase 1 filter broadening + Phase 0 trap-escape commits
already made this dead-end unreachable for users with cross-platform
"Apple Distribution" certs. The table is the structural fix: even if
some future cert type slips past the filter, the picker still won't
offer impossible options because each cert's classification is
checked up-front, not on-demand.
The hand-rolled column alignment in commit fe0fa89 broke when names
hit the truncation boundary — long cert names collapsed against the
neighbouring Team column with no visual separator, e.g.:

  ❯ 🔑 Apple Distribution: digital shift oü (UV…UVTJ336J2D  0/1

I tried `ink-table` first (the obvious package) but it ships as
CommonJS while modern Ink is ESM with top-level await — `require('ink')`
fails the bundler. Rather than fight the dep, I wrote a small inline
Table component (~75 LOC in components.tsx).

The new `<Table>`:

  - Auto-sizes each column to the widest cell (header or any row value),
    capped at `maxColumnWidth` (default 50).
  - Truncates overflowing cells with `…` using `Array.from()` for
    codepoint-safe length (emoji and combining marks don't double-count).
  - Renders box-drawing borders (`┌┬┐ ├┼┤ └┴┘ │ ─`) so columns are
    visually separated whether or not cells truncate.
  - Optional `cellColor` / `cellDim` callbacks for per-cell styling.
  - Derives the column list from the first row's keys, so callers pass
    plain `Record<string, string>[]` data.

Available + Unavailable tables in `import-pick-identity` now use it:

  ✅  AVAILABLE (1)
  ┌─────┬────────────────────────────────────────────────┬────────────┬─────────────┐
  │ #   │ Name                                           │ Team       │ Profiles    │
  ├─────┼────────────────────────────────────────────────┼────────────┼─────────────┤
  │ 1   │ 🔑 Apple Distribution: digital shift oü (UVT…  │ UVTJ336J2D │ 2/5 ✓       │
  └─────┴────────────────────────────────────────────────┴────────────┴─────────────┘

  Pick an option:
  ❯ [1]  Apple Distribution: digital shift oü · UVTJ336J2D
    🆕  Switch to "Create new" (...)
    ✖  Exit onboarding

  ⚠️   UNAVAILABLE (2)
  ┌────────────────────────────────────────────┬────────────┬─────────────────────────────┐
  │ Name                                       │ Team       │ Reason                      │
  ├────────────────────────────────────────────┼────────────┼─────────────────────────────┤
  │ 🔒 Apple Distribution: revoked cert         │ UVTJ336J2D │ Not visible to current API… │
  │ 🔒 Distribution Managed                     │ UVTJ336J2D │ Apple-managed — can't sign… │
  └────────────────────────────────────────────┴────────────┴─────────────────────────────┘

The picker has a separate Select below the Available table with
labelled rows (`[1] …`, `[2] …`) so users keyboard-navigate the actions
while the table provides the visual context. This is the lightest-
weight way to get selectable+tabular UX without a custom Ink renderer.

`cellColor` is wired up for the unavailable table's Reason column
(yellow) and the available table's Profiles column when a matching
profile is found (green ✓), to make scan-time visual cues match the
prose footnote.
…table

The Table component computed widths from `Array.from(s).length` —
codepoint count. That breaks on:

  - 🔑 and other emoji: render as 2 terminal columns, count as 1 codepoint
  - CJK characters: 2 columns / 1 codepoint
  - Some combining marks: 0 columns / 1 codepoint

Result: rows containing emoji rendered 1 column wider per emoji than
the header border, so the closing │ visibly drifted right of the
top/bottom ┬/┴ separators (visible in the user's screenshot: the data
row's │ after "(UVTJ336J2…" sat ~1 column past the header's ┬).

Switch to `string-width` (already in our tree transitively via Ink,
now a direct dep) which returns true terminal-column width. Two pure
helpers:

  - `truncateByDisplayWidth(s, max)` — accumulates per-char width via
    string-width, stops before the ellipsis would overflow.
  - `padByDisplayWidth(s, width)` — adds trailing spaces until the
    rendered width matches the column.

Both `widths[col]` computation and per-cell rendering go through the
display-width helpers, so emoji + accented characters now align with
the border. Verified locally — `🔑 Apple Distribution…` row now ends
exactly where the header ┬ separator sits.

string-width@8.x is ESM (compatible with our Ink 5.x ESM stack).
…flow

WHAT:
Split the no-usable-profile partition in app.tsx around the import-checking-apple-cert step into three cases instead of two: bundle mismatch, same-bundle wrong-distribution, and the residual none-match fallback. The new middle branch reports the real diagnosis when Apple returned profiles for the right bundle id but in a distribution mode (app_store vs ad_hoc) that filterProfilesForApp rejected.

WHY:
ultrareview issue #5 (distribution-mismatch-wording, severity nit) flagged that when filterProfilesForApp returned 0 and otherBundleIds was empty, the only remaining case was "bundle matches, distribution does not" — yet the else branch logged "Apple returned N profiles for this cert but none match this app". That wording is wrong: the profiles DO match the app's bundle id, so users landed in the recovery menu with a misleading diagnosis pointing them at the wrong fix (recreate profile / change bundle id) instead of the actual one (re-run with the correct --distribution, or create a profile in the desired mode).

VERIFIED:
- Read app.tsx:1188-1204 to confirm sameBundleWrongDist is the residual case after otherBundleIds is exhausted.
- Confirmed otherBundleIds appears only in this block (grep), so the edit is scoped and unique.
- New branch uses existing addLog signature, `importDistribution` is already in scope, and `synthesized` / `iosBundleId` are the same identifiers used by the surrounding partition.
- Ran lint-relevant test suites: test:onboarding-recovery, test:apple-api-import-helpers.
WHAT
- listProfilesForCert now walks body.links.next instead of returning only
  the first 200 profiles. allData/allIncluded are accumulated across pages,
  then the existing client-side cert-id filter and bundleId map are applied
  once at the end.
- Added pagination tests to test-apple-api-import-helpers.mjs that mock
  globalThis.fetch with multi-page responses and assert: (a) every page is
  walked, (b) Apple's fully-qualified links.next URL is stripped to a path
  before being passed to ascFetch, (c) matches are aggregated across pages,
  (d) the loop terminates cleanly when links.next is absent or missing.

WHY (ultrareview issue #4 — list-profiles-pagination)
- Apple's /profiles endpoint caps limit at 200, but that cap is on the
  TEAM's total active profile count (apps × distribution types × extensions
  × dev machines), NOT on profiles matching our certificate. Teams with
  200+ profiles silently lost cert matches that sat on page 2+, so
  import-checking-apple-cert misrouted to no-match-recovery, and the
  create-new path collided with the existing-but-paginated-away profile
  named "Capgo <appId> AppStore".
- Apple returns links.next as a fully-qualified URL
  (https://api.appstoreconnect.apple.com/v1/profiles?cursor=...). Since
  ascFetch builds `${ASC_BASE_URL}${path}`, we strip ASC_BASE_URL before
  reuse to avoid a double-prefixed request; a startsWith guard preserves
  any future path-relative form.

VERIFICATION
- Read apple-api.ts (lines 5, 42-69, 323-368) to confirm: ASC_BASE_URL
  constant lives at module scope and ascFetch concatenates path onto it.
- Wrote 3 pagination tests against a fetch mock covering: multi-page
  aggregation with cross-page matches, single-page short-circuit, and a
  page with missing data/included fields.
- Lint-clean under the existing oxlint surface for this file (uses the
  same `any` casts already pervasive in apple-api.ts; no new console.log
  or unused imports).
WHAT\nThe import-validating-all-certs step in build onboarding now fetches\nthe team's distribution certs once via listDistributionCerts and\nindexes the result by SHA1, instead of issuing N parallel findCertBySha1\ncalls. Each identity is resolved with an O(1) Map lookup.\n\nWHY (root cause)\nUltrareview issue #6 (eager-batch-fanout): findCertBySha1 internally\ncalls listDistributionCerts({includeContent:true}) on every invocation.\nThe previous Promise.all over importMatches therefore triggered N\nidentical /certificates downloads and N×M SHA1 hashes for a team with\nN local identities and M Apple-side certs. This concretely amplified\nthe apple_api_rate_limited risk the PR adds a telemetry category for —\na user with 3-5 distribution identities could spike 5 concurrent\nrequests against the same endpoint within a single render tick.\n\nThe refactor reduces the cost to one /certificates download + M SHA1\nhashes + N Map lookups. Downstream behavior is preserved:\n- result shape ({sha1, cert, error}) is unchanged\n- classifyCertAvailability sees the same cert/null inputs it did before\n- a SHA1 with no Apple-side match yields cert=null, which classify\n  already handles identically to the old 'no Apple match' branch\n- the cancelled guard moves to before the SHA1 indexing loop, matching\n  the original ordering relative to the network call\n\nThe single try/catch around the fetch reflects the actual failure mode:\none API call either succeeds for the whole batch or fails for the whole\nbatch. Per-identity error capture is no longer needed.\n\nVERIFICATION\n- bun run test:apple-api-import-helpers (adds 4 new tests covering the\n  SHA1-indexed Map pattern: known hit, unknown miss, case-insensitive\n  lookup, missing certificateContent guard)\n- bun run test:macos-signing (covers the upstream matchIdentitiesToProfiles\n  pipeline that feeds importMatches)\n- Verified listDistributionCerts and computeCertSha1 are exported from\n  apple-api.ts (lines 212, 262) and the existing apple-api.js import in\n  app.tsx was extended in place rather than duplicated.

Revisions applied per adversarial verifier:
  - dropped unused findCertBySha1 import (only batch path used it)
  - placed appended test block BEFORE the terminal OK marker
  - shortened commit subject to ≤70 chars
  - updated the stale 'fans out findCertBySha1' comment at the verifying-key fan-out site
Comment thread cli/test/test-apple-api-import-helpers.mjs Dismissed
Comment thread cli/test/test-apple-api-import-helpers.mjs Dismissed
…crete bundle ids

WHAT\n- Added an exported `bundleIdMatches(profileBundleId, appId)` helper in\n  `src/build/onboarding/macos-signing.ts` that honors Apple's wildcard\n  syntax: literal equality, suffix wildcard (`com.example.*`), and the bare\n  `*` wildcard.\n- Updated `filterProfilesForApp` to delegate the bundle-id check to\n  `bundleIdMatches` instead of using strict equality. The distribution\n  conjunction is unchanged, so a wildcard ad_hoc/enterprise profile is\n  still correctly filtered out when the caller pins `app_store`.\n- Updated the manual `.mobileprovision` file-picker validation in\n  `app.tsx` (`import-provide-profile-path` step) to use `bundleIdMatches`\n  so a wildcard profile picked from disk is accepted.\n- Refactored the un-migrated inline filter inside `import-pick-profile`\n  to call `filterProfilesForApp(allMatchedProfiles, iosBundleId,\n  importDistribution)`, removing the duplicated strict-equality logic.\n- Resolved the provisioning_map key for wildcard imports: when the\n  chosen profile's `bundleId` contains `*`, the key is substituted with\n  the concrete `iosBundleId`. Xcode resolves the map by\n  `PRODUCT_BUNDLE_IDENTIFIER` at sign time, so a literal `com.example.*`\n  key would never match.\n\nWHY (root cause)\nUltrareview issue #2 (`wildcard-profile-handling`) flagged that\n`filterProfilesForApp` and the file-picker validator used strict\n`p.bundleId === appId` comparisons. The mobileprovision parser at\n`mobileprovision-parser.ts:74-75` strips only the team-id prefix from\n`application-identifier`, leaving wildcard asterisks intact — so a\nlegitimate wildcard profile (typical for ad_hoc/enterprise teams that\nshare one profile across many apps) presents here as\n`bundleId === \"com.example.*\"` and never matched the concrete app id.\nUsers were routed straight to no-match recovery and manually picked\nwildcard profiles were hard-rejected.\n\nVERIFICATION\n- Added unit tests in `test/test-macos-signing.mjs` covering:\n  exact equality, suffix wildcard accept/reject, bare `*` wildcard,\n  and the distribution conjunction still gating wildcards.\n- Run with `bun run test:macos-signing`.

Revisions applied per adversarial verifier:
  - added 6th edit: defense-in-depth check in import-pick-profile onChange now uses bundleIdMatches
    so the picker would not over-reject wildcards the upstream filter intentionally accepted
  - placed appended test block BEFORE the terminal OK marker
  - hoisted bundleIdMatches into the static import block
…vide-profile-path

CI failed on PR #2308 with:

  FAIL tests/onboarding-error-categories.unit.test.ts > mapIosOnboardingError
       > maps import-fetching-profile failures to profile_read_failed
  AssertionError: expected 'unknown' to be 'profile_read_failed'

The backend test asserts mapIosOnboardingError behaviour against the
CLI's error-categories module (`../cli/src/build/onboarding/error-categories.ts`).
The 'import-fetching-profile' step was removed in commit 36a7c282 along
with the Rescan recovery option, and ultrareview-fix commit 4e17b7a
added the replacement mapping for 'import-provide-profile-path' (the
.mobileprovision file picker step that took over the "read profile
from disk" failure class).

This commit aligns the backend regression test with the new step name,
which is what the CLI test in test:macos-signing also asserts. The
behaviour we're locking in is unchanged: parse + 3 invariant checks
(bundle id / distribution / cert SHA1) + the generic catch on the
file-picker step all map to 'profile_read_failed' for PostHog
telemetry, just under the new step id.

Verified locally: bunx vitest run tests/onboarding-error-categories.unit.test.ts
→ 23 passed (23).
…breadcrumb

Two related UX problems on the import-portal-explanation step
(reached from the no-match recovery menu's "Open Apple Developer
Portal" option) when the user picked ad_hoc distribution:

  1. The "Open the portal anyway (advanced)" wording made no sense
     for ad_hoc — "anyway" implies a recommended automatic alternative
     to contrast against, but ad_hoc has none (canCreateProfile gate
     hides the auto path because our createProfile is IOS_APP_STORE-
     only and ad_hoc additionally needs a registered-devices list).
  2. The 7-step manual walkthrough silently skipped device registration
     — exactly the ad_hoc-specific complexity that makes it hard. So
     users following the walkthrough downloaded a profile that signed
     but wouldn't run on any device.

Changes:

  - Ad_hoc branch of the alert now reads "Ad-hoc distribution is
    genuinely fiddly (you also need to register every target device on
    Apple's side). Here's what's involved — and how to get help if
    you're stuck."
  - A new cyan-bordered info box (ad_hoc only) tells the user to email
    support@capgo.app with their team id for hands-on help — the
    canonical support address already used elsewhere in `init`.
  - The manual step list gains an ad_hoc-only step 6 about the
    Devices panel + device registration URL; the rest of the steps
    renumber to keep the order monotonic.
  - The Select label switches from "Open the portal anyway (advanced)"
    (ad_hoc's only path) to a plain "Open Apple Developer Portal" so
    the wording matches what's actually offered.
  - App_store branch is unchanged — same wording, same recommended-
    automatic nudge, same Select.

Build, typecheck, lint, test:onboarding-recovery green.
…ists

When `capgo build init` re-enters the iOS onboarding wizard with a saved
progress file AND the resume target isn't trivially 'welcome', show a new
'resume-prompt' step that surfaces a breadcrumb of what was saved (start
time, setup method, distribution mode, ASC key/cert/profile state, any iOS
bundle id override, and the resume target) and lets the user pick:

  ▶️  Continue from where I left off  — routes to getResumeStep(progress)
  🔄  Restart onboarding              — wipes progress + returns to welcome

Why: previously the wizard silently teleported users to the middle of the
flow after any partial run. Users coming back hours later had no chance to
inspect what was saved or to bail out without manually deleting the
on-disk progress file.

Implementation notes:
- New 'resume-prompt' step added to OnboardingStep, STEP_PROGRESS, and
  getPhaseLabel — exhaustive switches are kept exhaustive.
- The initial step useState lands on resume-prompt only when
  initialProgress !== null AND startStep !== 'welcome'. Trivial cases
  (no progress) skip the prompt entirely — no UX regression.
- Extracted resetForFreshStart() useCallback so the existing ErrorStep
  restart handler and the new resume-prompt restart handler share one
  reset implementation. The helper also fixes a latent bug in the
  ErrorStep handler: the eager per-identity Apple-side availability map
  (identityAvailability) was never reset on restart, so the import-pick
  picker could render with stale per-cert reasons from the previous run.
  The helper now resets that map AND the iOS bundle id confirmation state
  (iosBundleId/appIdConfirmed/pendingAppIdNext/confirmAppIdTyping), which
  the original handler also missed.
- Defensive startedAt parse: legacy/corrupted progress files with an
  unparseable startedAt render the raw value with a "(could not parse)"
  suffix instead of crashing the wizard.
- iOS-only. The Android wizard lives in a separate file and is not
  modified. Adding the new step to the shared union is type-safe and a
  no-op on Android (Android's getResumeStep never produces it).

Test plan:
- [ ] Run `capgo build init --platform ios` to completion; verify no
      prompt appears.
- [ ] Quit mid-flow (after p8 selection); re-run; verify prompt appears,
      breadcrumb shows the saved p8/keyId, Continue routes back to the
      verify step.
- [ ] From the prompt, pick Restart; verify progress file is deleted,
      iosBundleId/appIdConfirmed/identityAvailability are reset, and the
      flow restarts at welcome.
- [ ] Hit an error mid-flow; pick Restart from ErrorStep; verify the
      same reset happens (regression test for the missing
      identityAvailability reset).
- [ ] Corrupt the startedAt field in the saved progress file; re-run;
      verify the prompt renders with the raw value + "(could not parse)"
      suffix instead of crashing.
- [ ] Run `capgo build init --platform android`; verify no behavior
      change (Android wizard unaffected).

--- Verifier check ---
Verdict: apply (plan_valid=True)
Applied as-planned; informational concerns acknowledged:
  • tests_to_run uses vitest-style invocation (`pnpm --filter @capgo/cli test -- --run src/build/onboarding/progress.test.ts`) that doesn't match this repo's actual test setup. Tests here are bun scripts:
  • The new step adds rich Apple-side credentials state to the breadcrumb (apiKeyVerified keyId, certificate expirationDate, profileName). These are derived purely from local progress; if Apple has since
  • `log` state (LogEntry[]) is intentionally NOT cleared in `resetForFreshStart`, mirroring the existing ErrorStep restart contract. After picking Restart from the new resume-prompt, the completed-steps
The team id ("Email support@capgo.app with your team id (UVTJ336J2D)
and we'll walk you through…") added no value for support — anyone can
send their team id, support still needs to verify identity another
way, and parenthesising the user's id inside the action sentence made
the line longer for no payoff. Drop the parenthetical so the call to
action reads cleanly.
… distribution

Bug surfaced by the new resume-prompt feature: a user who picked
"Import existing" + Ad Hoc, quit the wizard, and came back hit
Continue on the resume prompt — but landed on `setup-method-select`
asking "How do you want to set up iOS credentials?" again, re-asking
the fork they had just confirmed.

Root cause: `getResumeStep`'s import-existing branch only checked for
the saved .p8 chain when deciding the no-apiKey fallthrough. If no
.p8 inputs existed it returned `setup-method-select` regardless of
the saved `importDistribution`. The comment above that return claimed
"distribution mode is gone from progress, so re-ask" — but
importDistribution has been persisted for a while now (the rest of
the codebase keys off it correctly, e.g. getImportEntryStep).

Fix: replace the bare fallthrough with a three-way branch on the
saved distribution mode:

  • ad_hoc       → 'import-scanning' (no .p8 needed; non-TestFlight)
  • app_store    → 'api-key-instructions' (start the .p8 input chain)
  • undefined    → 'import-distribution-mode' (re-ask just that, not
                    the whole setup fork)

Mirrors what `getImportEntryStep` already does after a successful
scan, but at mount time so the resume target is right before any work
runs.

Added regression tests covering all three new branches + the
existing partial .p8 branches (verifying-key / input-issuer-id /
input-key-id) so future churn in this routing logic is caught:

  • ad_hoc import without apiKey → import-scanning
  • app_store import with no inputs → api-key-instructions
  • importDistribution unset → import-distribution-mode
  • full .p8 inputs → verifying-key
  • .p8 + keyId only → input-issuer-id
  • .p8 only → input-key-id

All 21 onboarding-progress tests pass; onboarding-recovery green.
…recovery

Moves the "Ad-hoc is complex — email support@capgo.app for help"
breadcrumb from the import-portal-explanation step body to:
  1. an addLog pair fired at the moment the user picks Ad Hoc on
     the import-distribution-mode step, and
  2. a mount-time hydration replay that re-surfaces the same lines on
     resume (gated on importDistribution === 'ad_hoc' AND
     !completedSteps.profileCreated, so the heads-up disappears once
     ad-hoc setup is already past the profile-creation milestone).

Why move it: the previous placement only fired when the user had
already started the manual portal walkthrough — meaning they
discovered the offer only after they'd hit a wall and started
fumbling. Surfacing it at distribution-mode means the user sees the
heads-up the moment they commit to the harder path, before any work
has been wasted. The yellow log entries stay visible in the side log
throughout the rest of the wizard, so they remain available later
when the user is actually evaluating recovery options.

Cleanup: the cyan-bordered "Want help…" Box and its surrounding
conditional render block are removed from import-portal-explanation;
the rest of the walkthrough (alert text, device-registration step,
plain "Open Apple Developer Portal" select option) is unchanged.

Build, typecheck, lint, onboarding-progress (21 tests),
onboarding-recovery green.
… picked

When the resume-prompt screen is the initial step, the mount-time
useEffect was eagerly populating the side log with the user's
in-progress breadcrumb entries — including the new ad-hoc support
hint — BEFORE the user had picked Continue or Restart. That made the
prompt visually arrive AFTER its own context, and if the user picked
Restart the wizard kept the now-stale log entries dangling.

Refactor:

  - Extract the hydration block into a `hydrateCompletedLog`
    useCallback (covers partial inputs, completed steps, and the
    ad-hoc support hint replay).
  - Gate the mount-time useEffect on a ref captured from the initial
    step value: when the wizard mounts straight onto resume-prompt,
    hydration is suppressed. When it mounts onto welcome or a deeper
    step (trivial-progress case), hydration runs as before so partial
    breadcrumbs still appear.
  - Call `hydrateCompletedLog()` from the resume-prompt onChange's
    Continue branch, immediately before transitioning to startStep —
    so the side log fills in exactly when the user commits to picking
    up where they left off. The Restart branch keeps the log empty,
    which is what a fresh start should look like.

addLog's consecutive-dedupe guards against accidental double-call if
this ever runs twice.

Build, typecheck, lint, onboarding-progress (21 tests),
onboarding-recovery green.
… sites

The hint pair fired twice in one of these cases:

  • User resumed a saved ad_hoc run via the resume-prompt → hydrate
    replay emitted the hint, then the wizard transitioned to
    import-pick-identity, then the user back-navigated to the
    distribution-mode picker, re-picked Ad Hoc, and the onChange
    emitted the hint again.
  • Any sequence where Ad Hoc was picked after another log line
    (typically `✔ Distribution · ad_hoc` itself) had already broken
    addLog's consecutive-dedupe between the two pairs.

Fix: route both call sites through a `logAdHocSupportHint` useCallback
guarded by a session-scoped `adHocHintShownRef`. The helper is
idempotent across the Ink session — first call emits the pair, all
subsequent calls no-op until the next Restart resets the ref via
`resetForFreshStart`.

Reset on Restart is intentional: if the user wipes progress and
re-enters the import flow then re-picks Ad Hoc, the hint is again
newly relevant (it's part of "you've just committed to the harder
path") so it should re-emit. Without the reset, a stale "shown"
flag from the pre-restart session would silently mute it.

Build, typecheck, lint, onboarding-progress (21), onboarding-recovery
green.
…t stack

`addLog` only deduplicates CONSECUTIVE log entries; when the user
navigates back through the import flow and re-picks Ad Hoc (or
re-enters a Key ID, etc.), the previous "✔ Distribution · ad_hoc"
emission is no longer the most recent line — anything else logged in
between breaks the dedupe — so the new emission survives and the
breadcrumb stacks.

Restored the pre-merge `upsertLog(prefix, text, color)` helper that
scans the whole log for any entry starting with `prefix` and REPLACES
it in place (else appends, with the same consecutive-dedupe guard).
Re-pointed the five field-update emission sites at it:

  • ✔ Key file selected · <path>   (hydration replay + .p8 picker)
  • ✔ Key ID · <id>                 (hydration replay + Key ID input)
  • ✔ Issuer ID · <id>              (hydration replay + Issuer ID input)
  • ✔ Distribution · <mode>         (distribution-mode picker)

Plain breadcrumb emissions (build progress, profile counts, etc.)
keep using addLog — they're append-only event lines, not field
updates.

A user who picks Ad Hoc → goes to import-pick-identity → cancels
back to setup-method-select → picks Import again → re-picks Ad Hoc
now sees exactly one "✔ Distribution · ad_hoc" line instead of two
(the second pick upserts the same entry in place).

Build, typecheck, lint, onboarding-progress (21), onboarding-recovery
green.
The mount-time hydration replay (used when the user picks Continue
on the resume-prompt) was emitting:

  • partial inputs (Key file selected, Key ID, Issuer ID)
  • completed steps (API Key verified, Cert created, Profile created)
  • the ad-hoc support hint when applicable

…but not the upstream `✔ Distribution · <mode>` line that the
distribution-mode picker emits on a fresh run. So after Continue on
an ad_hoc resume, the user saw the ad-hoc support hint surface
without the breadcrumb that explains WHY the wizard is in ad-hoc
mode in the first place.

Fix: add the same upsertLog('✔ Distribution · ', …) call to the
hydration block, gated on `importDistribution` being set. Placed
FIRST in the block so the resumed log mirrors the visual order of a
fresh run — Distribution leads the import-flow breadcrumb stack.

Uses upsertLog (not addLog) for symmetry with the distribution-mode
picker — a user who back-navigates and re-picks doesn't end up with
two entries.

Build, typecheck, lint, onboarding-progress (21), onboarding-recovery
green.
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jun 2, 2026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants