Skip to content

Stop the onboarding wizard offering GitHub and stalling after sign-in - #1101

Merged
alexeyzimarev merged 2 commits into
mainfrom
wizard-sso-default-and-advance
Sep 22, 2026
Merged

alexeyzimarev merged 2 commits into
mainfrom
wizard-sso-default-and-advance

Conversation

@alexeyzimarev

@alexeyzimarev alexeyzimarev commented Sep 21, 2026 •

Copy link
Copy Markdown
Member

Closes #1107 — AI-3119

What & why

The first-run wizard preselected GitHub App sign-in for workspace discovery, while kcap setup and kcap login default to single sign-on. Wizard discovery is single sign-on only; a server on GitHub App auth is reached by name or URL, where its own /auth/config picks the flow.

After a committed sign-in the wizard showed "You're signed in. Refreshing…" and stayed on the step. That line, and the refresh-and-close behind it, belong to the re-auth dialog, which hosts the same SignInStepViewModel. The step raises Completed and takes its success detail from its host: the dialog keeps its copy, the wizard advances after the same 1.6 s hold.

Where to look

Completed is withheld while a consent quarantine notice is up and fires on acknowledgement. TryAdvanceFrom checks the wizard's visit counter as well as the step id, so a Back or Next pressed during the hold is not overridden — including Back then Next, which lands on Sign in again.

Verification

  • A_committed_sign_in_moves_the_wizard_on_after_the_success_hold failed without the fix (Timed out waiting for: the move past the sign-in step) and passes with it.
  • Onboarding and re-auth test classes: 130 passed, 0 failed.
  • Full Capacitor.App.Tests.Unit: 2768 of 2769. The one failure, Size_label_and_image_flag, expects 2.3 MB and gets 2,3 MB under a Norwegian locale; it passes under en_US.UTF-8.
  • Not run in the live app: the path needs a real sign-in.

🤖 Generated with Claude Code

The sign-in step has two hosts, so what follows a commit is the host's: the re-auth dialog refreshes and closes, the wizard advances after the same hold. A GitHub App server is still reachable by name or URL, where its own auth config picks the flow.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Use SSO discovery and advance onboarding after sign-in

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Restricts wizard workspace discovery to single sign-on, matching CLI defaults.
• Advances onboarding after committed sign-in without overriding user navigation.
• Preserves re-auth messaging and consent quarantine acknowledgement behavior.
Diagram

graph TD
    Connect["SSO Discovery"] --> SignIn["Sign-in Step"] --> Quarantine{"Notice pending?"}
    Quarantine -->|Yes| Ack["Acknowledge notice"] --> Completed["Completed event"] --> Wizard["Wizard advances"]
    Quarantine -->|No| Completed
    SignIn -->|Satisfied state| Reauth["Re-auth host"] --> Refresh["Refresh and close"]
Loading
High-Level Assessment

The host-owned behavior is the appropriate design because SignInStepViewModel is shared by onboarding and re-auth but each host has a different post-success action. Direct wizard navigation inside the shared view model would couple it to one host, while retaining a provider selector would conflict with the wizard's SSO-only discovery contract.

Files changed (16) +254 / -84

Bug fix (8) +67 / -66
App.axaml.csShare the sign-in success hold with re-auth +1/-4

Share the sign-in success hold with re-auth

• Replaces the app-local re-auth delay with SignInStepViewModel.SuccessHold, keeping wizard and dialog success timing consistent.

src/Capacitor.App/App.axaml.cs

ReauthComposition.csConfigure re-auth-specific success messaging +5/-1

Configure re-auth-specific success messaging

• Passes the refresh message into the shared sign-in view model so only the re-auth dialog promises a refresh after success.

src/Capacitor.App/Services/Onboarding/ReauthComposition.cs

WizardAuthBridges.csRoute all wizard discovery through SSO +3/-3

Route all wizard discovery through SSO

• Routes both discovery and workspace creation through WorkOS instead of honoring a wizard-selected provider.

src/Capacitor.App/Services/Onboarding/WizardAuthBridges.cs

WizardComposition.csAdvance the wizard after committed sign-in +15/-5

Advance the wizard after committed sign-in

• Subscribes to sign-in completion and advances after the shared success hold. Uses guarded navigation so delayed completion cannot override later user navigation.

src/Capacitor.App/Services/Onboarding/WizardComposition.cs

ConnectStepViewModel.csMake workspace discovery SSO-only +6/-28

Make workspace discovery SSO-only

• Removes provider selection state and always stages a provider-neutral discovery intent. Updates documentation to clarify how non-SSO servers remain reachable by name or URL.

src/Capacitor.App/ViewModels/Onboarding/ConnectStepViewModel.cs

OnboardingViewModel.csAdd guarded automatic step advancement +8/-0

Add guarded automatic step advancement

• Adds TryAdvanceFrom to move forward only when the completed step is still current and the wizard remains open.

src/Capacitor.App/ViewModels/Onboarding/OnboardingViewModel.cs

SignInStepViewModel.csExpose host-neutral sign-in completion +29/-18

Expose host-neutral sign-in completion

• Adds a shared success hold, configurable committed detail, and a Completed event. Completion waits until any consent quarantine notice has been acknowledged.

src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs

OnboardingWindow.axamlRemove the discovery provider selector +0/-7

Remove the discovery provider selector

• Removes GitHub and single sign-on radio buttons because wizard workspace discovery now always uses SSO.

src/Capacitor.App/Views/Onboarding/OnboardingWindow.axaml

Refactor (1) +1 / -1
WizardAuthService.csRemove provider data from discovery intent +1/-1

Remove provider data from discovery intent

• Makes ConnectIntent.Discover provider-neutral because wizard discovery now has a single SSO implementation.

src/Capacitor.App/Services/Onboarding/WizardAuthService.cs

Tests (6) +179 / -17
ConnectStepViewModelTests.csTest default provider-neutral discovery +6/-8

Test default provider-neutral discovery

• Replaces provider-selection coverage with assertions that a fresh connect step stages SSO discovery successfully.

test/Capacitor.App.Tests.Unit/ConnectStepViewModelTests.cs

OnboardingViewModelTests.csTest guarded automatic advancement +51/-0

Test guarded automatic advancement

• Verifies TryAdvanceFrom moves to the next step and refuses late advancement after navigation or wizard closure.

test/Capacitor.App.Tests.Unit/OnboardingViewModelTests.cs

ReauthCompositionTests.csVerify re-auth retains refresh messaging +2/-0

Verify re-auth retains refresh messaging

• Confirms the re-auth composition still displays its host-specific refresh detail after successful sign-in.

test/Capacitor.App.Tests.Unit/ReauthCompositionTests.cs

SignInStepViewModelTests.csTest sign-in completion and quarantine gating +54/-4

Test sign-in completion and quarantine gating

• Verifies only committed authentication raises completion, wizard success text remains host-neutral, and quarantine acknowledgement releases completion. Updates discovery expectations to SSO.

test/Capacitor.App.Tests.Unit/SignInStepViewModelTests.cs

WizardAuthServiceTests.csUpdate auth service tests for provider-neutral discovery +1/-1

Update auth service tests for provider-neutral discovery

• Adapts admission-gate coverage to the simplified discovery intent.

test/Capacitor.App.Tests.Unit/WizardAuthServiceTests.cs

WizardStartupTests.csTest delayed wizard advancement end to end +65/-4

Test delayed wizard advancement end to end

• Adds fake-time integration coverage proving committed sign-in advances after the hold without overriding user navigation. Updates startup and proxy tests for provider-neutral discovery.

test/Capacitor.App.Tests.Unit/WizardStartupTests.cs

Documentation (1) +7 / -0
CHANGES.mdDocument SSO discovery and host-owned sign-in completion +7/-0

Document SSO discovery and host-owned sign-in completion

• Explains that wizard discovery is SSO-only and that post-sign-in behavior belongs to each host. Documents quarantine acknowledgement and stale-navigation safeguards.

docs/CHANGES.md

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d0daab00ad

ℹ️ 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".

Comment on lines +105 to +108
internal bool TryAdvanceFrom(WizardStepId id) {
if (_closed || Current.Id != id || _index >= Steps.Count - 1) return false;

return TryGoTo(Steps[_index + 1].Id);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Invalidate the delayed advance after any navigation

If the user presses Back after sign-in succeeds and then returns to Sign-in before the 1.6-second hold expires, Current.Id is SignIn again, so the stale completion timer is accepted and unexpectedly advances to Defaults. This can skip sign-in after the user changes the workspace selection; track a navigation generation or completion token so leaving and returning invalidates the pending advance.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed in 47ef094. OnboardingViewModel.Visit is bumped on every transition; the hold captures it when Completed fires and TryAdvanceFrom refuses on a mismatch. Pinned by TryAdvanceFrom_is_refused_on_a_return_visit_to_the_same_step and the returned: true case of A_committed_sign_in_never_pulls_the_user_off_a_step_they_chose.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-21T16:30:10.299785Z d0daab0 PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@qodo-code-review

qodo-code-review Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Failed consent saves still advance ✗ Dismissed 🐞 Bug ☼ Reliability
Description
AcknowledgeQuarantineAsync clears QuarantineNotice, ignores the boolean result from
AckQuarantineAsync, and invokes Completed whenever sign-in is satisfied. When the app-state
write returns false, the wizard advances despite not persisting the acknowledgement and removes the
in-session opportunity to retry it.
Code

src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs[R514-515]

+
+        if (Satisfied) Completed?.Invoke();
Relevance

●●● Strong

Unconditionally advancing after a failed persistence operation is a concrete reliability bug.

PR-#884
PR-#1069

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The shared acknowledgement helper returns the result of IAppStateStore.UpdateAsync, whose
production implementation explicitly returns false on write failure. The new unconditional
Completed invocation converts that existing failure signal into automatic wizard advancement.

src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs[505-515]
src/Capacitor.App/Services/Onboarding/ConsentFlipCoordinator.cs[127-135]
src/Capacitor.App/Services/AppStateStore.cs[27-53]
src/Capacitor.App/Services/Onboarding/WizardComposition.cs[145-157]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The quarantine acknowledgement path advances after failed persistence because it ignores `AckQuarantineAsync`'s boolean result and clears the notice before saving. This loses the retry UI and records completion without a durable acknowledgement.

## Fix Focus Areas
- src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs[505-515]
- src/Capacitor.App/Services/Onboarding/ConsentFlipCoordinator.cs[132-135]

## Recommended Fix
Capture the result of `AckQuarantineAsync`, clear the notice and raise `Completed` only when it returns true, and retain or restore the notice when persistence returns false or throws so the user can retry.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Returning users are advanced again ✓ Resolved 🐞 Bug ≡ Correctness
Description
AdvanceAfterHoldAsync calls TryAdvanceFrom(SignIn) after its delay, but TryAdvanceFrom checks
only the current step identifier and cannot distinguish the Sign-in visit that scheduled the
callback from a later visit to the same step. If the user leaves Sign-in, goes Back, changes the
connection choice, and returns within the 1.6-second hold, the old completion remains eligible and
advances past Sign-in despite belonging to the previous connection intent.
Code

src/Capacitor.App/ViewModels/Onboarding/OnboardingViewModel.cs[R106-108]

+        if (_closed || Current.Id != id || _index >= Steps.Count - 1) return false;
+
+        return TryGoTo(Steps[_index + 1].Id);
Relevance

●●● Strong

Recent accepted precedent flags stale asynchronous callbacks that remain valid after navigation
changes.

PR-#979

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
A committed sign-in raises Completed and starts an independent fire-and-forget delayed callback,
while navigation can move from Sign-in to Connect and back to the same Sign-in object during that
delay. Because TryAdvanceFrom validates only that the current step has the Sign-in identifier and
OnEnterAsync does not reset the satisfied state, it cannot tell that the callback belongs to an
earlier visit and therefore still permits it to advance.

src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs[405-428]
src/Capacitor.App/Services/Onboarding/WizardComposition.cs[145-158]
src/Capacitor.App/ViewModels/Onboarding/OnboardingViewModel.cs[103-139]
src/Capacitor.App/Services/Onboarding/WizardComposition.cs[142-157]
src/Capacitor.App/ViewModels/Onboarding/OnboardingViewModel.cs[84-108]
src/Capacitor.App/ViewModels/Onboarding/OnboardingViewModel.cs[111-149]
src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs[355-359]
src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs[518-526]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A delayed completion callback must advance only the exact Sign-in visit that produced it. The current check accepts a later visit to the same Sign-in step, so leaving it, changing the connection choice, and returning before the hold expires lets a callback from the previous connection intent advance the wizard without a corresponding sign-in.

## Fix Focus Areas
- src/Capacitor.App/Services/Onboarding/WizardComposition.cs[145-157]
- src/Capacitor.App/ViewModels/Onboarding/OnboardingViewModel.cs[103-109]

## Recommended Fix
Associate each scheduled completion with the current navigation generation or step-entry instance captured when `Completed` fires. Increment or otherwise invalidate that generation whenever navigation leaves the originating Sign-in visit, and require the captured generation to remain unchanged in `TryAdvanceFrom`; retain the existing closed-wizard and current-step guards. Add a test that completes sign-in, navigates Back, changes the connection, returns to Sign-in before the hold expires, and verifies that the old callback does not advance.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. A navigation comment repeats the code ✓ Resolved 📘 Rule violation ⚙ Maintainability
Description
TryAdvanceFrom_is_refused_once_the_user_has_left_that_step_or_closed_the_wizard appends `// the
user got to Defaults first to a NextCommand` call whose surrounding setup and assertions already
state that transition. Because the note adds no non-obvious constraint, later edits must maintain
redundant prose that can drift from the test.
Code

test/Capacitor.App.Tests.Unit/OnboardingViewModelTests.cs[488]

+            await moved.NextCommand.Execute().ToTask(); // the user got to Defaults first
Relevance

●●● Strong

Recent precedents accept removing comments that merely restate test names or assertions.

PR-#831
PR-#703

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The changed line describes the obvious result of executing NextCommand, while PR Compliance ID
2762993 permits comments only for non-obvious, behavior-critical constraints.

Rule 2762993: Restrict comments to documenting non-obvious, behavior‑critical constraints
test/Capacitor.App.Tests.Unit/OnboardingViewModelTests.cs[488-488]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The inline navigation comment merely restates the immediately preceding test action and does not document a behavior-critical constraint.

## Fix Focus Areas
- test/Capacitor.App.Tests.Unit/OnboardingViewModelTests.cs[488-488]

## Recommended Fix
Remove the inline comment while retaining the `NextCommand` call; the test name and assertions already communicate why the navigation occurs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. PR description omits both issue links ✗ Dismissed 📘 Rule violation § Compliance
Description
The PR description opens by saying that both halves of the reference line are absent instead of
providing one closing GitHub reference and one Linear key on the same line. This omission applies
whenever reviewers or release tooling use the description to trace the wizard changes back to their
work items.
Code

docs/CHANGES.md[R1603-1604]

+The wizard's workspace discovery is single sign-on only, matching the CLI's default: a server on
+GitHub App auth is reached by name or URL, where its own `/auth/config` picks the flow.
Relevance

●●● Strong

Explicit compliance rule requires both GitHub and Linear references in the PR description.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2897991 requires a single description line containing both references. The supplied
PR description explicitly states that the GitHub and Linear references are both absent.

Rule 2897991: PR description must contain both GitHub and Linear issue references; PR title must not contain issue IDs

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR description does not contain the required reference line with both a closing GitHub issue reference and a Linear issue key.

## Fix Focus Areas
- docs/CHANGES.md[1603-1604]

## Recommended Fix
Create the relevant work items and add one description line in the form `Closes #123 AI-456`, keeping both references together and leaving the PR title unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View medium (1)
5. Commit subject lacks issue traceability 📘 Rule violation ⚙ Maintainability
Description
The supplied commit subject Stop the onboarding wizard offering GitHub and stalling after sign-in
does not end in (#<digits>) and combines the two outcomes with and. When this commit is merged,
its subject lacks the required GitHub traceability token and single-clause form.
Code

docs/CHANGES.md[R1603-1604]

+The wizard's workspace discovery is single sign-on only, matching the CLI's default: a server on
+GitHub App auth is reached by name or URL, where its own `/auth/config` picks the flow.
Relevance

●●● Strong

Explicit repository compliance rule requires issue-traceable, single-clause commit subjects.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 2897961 requires every commit subject to be a single imperative clause ending with
a GitHub issue reference. The supplied commit subject has no such reference and joins two outcomes
with and.

Rule 2897961: Enforce single-clause imperative commit subject with GitHub issue reference and 80-char limit

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The commit subject lacks the required trailing GitHub issue reference and contains two joined outcomes rather than one imperative clause.

## Fix Focus Areas
- docs/CHANGES.md[1603-1604]

## Recommended Fix
Rewrite the commit subject as one concise imperative clause of at most 80 characters and append the explicit GitHub issue number in `(#123)` form.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 64 rules
✅ Cross-repo context — repo relationships
  Explored: repo: kurrent-io/kcap-server (sha: 13b39a35)
  Explored: repo: kurrent-io/kcap-deployments (sha: cbe40e54)
Review mode: 🧠 Deep: This is a behavior-changing onboarding/auth flow refactor spanning multiple composition, view-model, UI, timing, quarantine, and navigation paths with many independent edit sites and subtle asynchronous edge cases.

Grey Divider

Tip of the day
💡 Did you know, you can route each action level your way: inline, summary, both, or drop

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread docs/CHANGES.md
Comment on lines +1603 to +1604
The wizard's workspace discovery is single sign-on only, matching the CLI's default: a server on
GitHub App auth is reached by name or URL, where its own `/auth/config` picks the flow.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

2. Commit subject lacks issue traceability 📘 Rule violation ⚙ Maintainability

The supplied commit subject Stop the onboarding wizard offering GitHub and stalling after sign-in
does not end in (#<digits>) and combines the two outcomes with and. When this commit is merged,
its subject lacks the required GitHub traceability token and single-clause form.
Agent Prompt
## Issue description
The commit subject lacks the required trailing GitHub issue reference and contains two joined outcomes rather than one imperative clause.

## Fix Focus Areas
- docs/CHANGES.md[1603-1604]

## Recommended Fix
Rewrite the commit subject as one concise imperative clause of at most 80 characters and append the explicit GitHub issue number in `(#123)` form.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The reference goes in once an issue exists (see the sibling thread). The subject is one clause with a compound object; squash-merge appends the PR number.

Comment thread docs/CHANGES.md
Comment thread test/Capacitor.App.Tests.Unit/OnboardingViewModelTests.cs Outdated
Comment thread src/Capacitor.App/ViewModels/Onboarding/SignInStepViewModel.cs
Comment thread src/Capacitor.App/ViewModels/Onboarding/OnboardingViewModel.cs Outdated
The step id alone cannot tell the visit that armed the hold from a later one: Back then Next within the hold lands on Sign in again and the old timer would move the user on.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@alexeyzimarev
alexeyzimarev merged commit 1e3598c into main Sep 22, 2026
8 checks passed
@alexeyzimarev
alexeyzimarev deleted the wizard-sso-default-and-advance branch September 22, 2026 15:57
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.

Onboarding wizard defaults to GitHub sign-in and stalls after signing in

1 participant