feat(fe): auth disambiguation dialog primitives#3966
Merged
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to 50510c9. Security Overview
Detected Code Changes
|
MRmarioruci
force-pushed
the
feat/auth-disambiguation-primitives
branch
2 times, most recently
from
6550428 to
241bb22
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR lays groundwork for upcoming auth-flow alignment work by introducing new auth disambiguation dialog components, extracting the “continue on another device” UI into a standalone view, and improving UX around user-canceled OpenID sign-ins.
Changes:
- Add new auth disambiguation dialog components (NotConnected / AlreadyLinked / SwitchAccessMethod) plus supporting copy in
en.po. - Refactor
AuthWizardto use a dedicatedContinueOnAnotherDeviceViewand adjust dialog rendering to avoid conflicting overlays. - Show an informational toast when an OpenID sign-in is canceled by the user.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/frontend/src/routes/(new-styling)/manage/(authenticated)/+layout.svelte | Adjusts mobile sidebar auto-close behavior to only trigger on pathname changes. |
| src/frontend/src/lib/locales/en.po | Adds new English strings for auth disambiguation dialogs and OIDC cancel toast. |
| src/frontend/src/lib/components/wizards/auth/views/ContinueOnAnotherDeviceView.svelte | New wrapper view around RegisterAccessMethodWizard. |
| src/frontend/src/lib/components/wizards/auth/index.ts | Exports newly added auth dialog/view primitives. |
| src/frontend/src/lib/components/wizards/auth/dialogs/SwitchAccessMethodDialog.test.ts | Adds unit tests for dialog label/badge formatting logic (documentation references need correction). |
| src/frontend/src/lib/components/wizards/auth/dialogs/SwitchAccessMethodDialog.svelte | New “switch access method” dialog UI. |
| src/frontend/src/lib/components/wizards/auth/dialogs/IdentityNotConnectedDialog.test.ts | Adds unit tests describing parent prop contract (documentation references need correction). |
| src/frontend/src/lib/components/wizards/auth/dialogs/IdentityNotConnectedDialog.svelte | New “identity not connected” dialog UI. |
| src/frontend/src/lib/components/wizards/auth/dialogs/IdentityAlreadyLinkedDialog.test.ts | Adds unit tests describing parent prop contract (documentation references need correction). |
| src/frontend/src/lib/components/wizards/auth/dialogs/IdentityAlreadyLinkedDialog.svelte | New “already linked” dialog UI. |
| src/frontend/src/lib/components/wizards/auth/AuthWizard.test.ts | Adds tests for isOpenIdCancelError behavior used by the new cancel toast. |
| src/frontend/src/lib/components/wizards/auth/AuthWizard.svelte | Refactors continue-on-another-device flow and adds OIDC-cancel toast. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…y + OIDC-cancel toast - Extract ContinueOnAnotherDeviceView from AuthWizard into a pure view - Restructure render so non-modal overlays (ContinueOnAnotherDevice, MigrationWizard) sit on top of the picker instead of replacing it - Show a toast when OIDC sign-in is canceled
…-cancel detection
When AuthWizard is rendered outside a host dialog and an overlay
(ContinueOnAnotherDevice or MigrationWizard) is triggered while the
picker has already advanced past chooseMethod, the view-content Dialog
and the overlay Dialog rendered simultaneously, breaking
`getByRole('dialog')` strict mode and intercepting pointer events on
mobile viewports.
The /manage layout's afterNavigate hook closed the mobile sidebar on every navigation event, including SvelteKit's same-URL navigations (clicking a nav link for the current page, or the replaceState goto inside handleOtherDeviceConfirmed that strips the ?activate= query param after a cross-device confirmation). On mobile that yanked the menu shut mid-click and made the next nav-link click time out because the link was translated off-screen. Guard the auto-close on actual route changes only.
Both IdentityAlreadyLinkedDialog and IdentityNotConnectedDialog now accept a loading boolean. When set, the primary action button is disabled and renders a ProgressRing — giving callers a way to indicate the in-flight state of the resume/signIn callback they triggered.
…igation After a same-pathname guard landed in daf5cb1, /manage → /manage/<sub> nav-link clicks on mobile still raced against Playwright's auto-wait: the click target moved off-screen via the menu close animation before the click event finished dispatching, so the actionability retry loop saw the element outside the viewport and timed out. Defer the close to the next task so the click event completes and the new page begins rendering before the sidebar starts sliding out. The visual effect for the user is identical.
- SwitchAccessMethodDialog: key the from/to {#each} by index, not the
method object, so when fromMethod and toMethod happen to be the same
reference Svelte doesn't crash on a duplicate key.
- IdentityNotConnectedDialog: the disclosure <summary> stripped the
default focus outline without a focus-visible alternative; restore
keyboard focus visibility via a focus-visible ring.
- Three dialog .test.ts files: clarify that the referenced Playwright
spec (authorize/auth-disambiguation.spec.ts) lives in the stacked
routes PR — the primitive lands here ahead of being wired up.
MRmarioruci
force-pushed
the
feat/auth-disambiguation-primitives
branch
from
5145b06 to
1a8e3f6
Compare
The mobile shard of the cli.spec tests was timing out on the Settings
link click. `waitForURL("/manage")` resolves when the URL flips,
but on mobile the manage layout's first paint can land a tick later.
`isVisible()` does not auto-wait — when it ran on the unrendered
header it returned a stale false, the conditional skipped the menu
open, and the Settings click then timed out trying to reach a link
inside the still-closed (`translate-x-[-100%]`) sidebar.
Wait for a stable sidebar element (the Settings link itself) before
probing visibility. Three callsites share the same race; all three
get the same guard.
…ion-primitives # Conflicts: # src/frontend/tests/e2e-playwright/routes/cli.spec.ts
sea-snake
reviewed
Jun 4, 2026
sea-snake
reviewed
Jun 4, 2026
sea-snake
reviewed
Jun 4, 2026
sea-snake
reviewed
Jun 4, 2026
sea-snake
reviewed
Jun 4, 2026
- Move dialog primitives from dialogs/ into views/ and drop the Dialog suffix - Remove dialog component tests (they covered copied logic, not the components) - Drop en.po hunk (translations land via separate chore PR) - Revert setTimeout-deferred mobile sidebar close; pathname-change guard stays - Restore cli.spec.ts helper to main version (PR #3975)
sea-snake
reviewed
Jun 4, 2026
sea-snake
reviewed
Jun 4, 2026
sea-snake
reviewed
Jun 4, 2026
sea-snake
reviewed
Jun 4, 2026
The wizard already returns "cancelled" on isOpenIdCancelError, which surfaces the inline cancellation tooltip above the provider button — the additional info toast notified the user twice and conflicted with dialog backdrops. Per sea-snake review.
- Relocate the six isOpenIdCancelError test cases from AuthWizard.test.ts into openID.test.ts (where the function lives) and delete the AuthWizard test file, which never imported AuthWizard. - Drop the pathname-comparison guard in /manage's afterNavigate; the mobile sidebar should close on any navigation, since users can't interact with the page while it's open.
After removing the cancellation toasts, the `t` import is no longer referenced — flagged by ESLint's no-unused-vars rule.
MRmarioruci
added a commit
that referenced
this pull request
Jun 4, 2026
- Drop the en.po changes — translations land via the bot, not feature PRs (sea-snake review on #3966). - Replace Promise.any([a.waitFor(), b.waitFor()]) in cli.spec.ts:signUp with a single .or().first().waitFor() — same effect, less noise. Comment explains why the explicit waitFor stays (auto-wait covers interactions, not isVisible() probes).
sea-snake
approved these changes
Jun 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Foundation for the upcoming homepage and
/authorize//managealignment work. Introduces reusable dialog primitives for auth disambiguation, extractsContinueOnAnotherDeviceDialoginto its own component, and improves OIDC-cancel UX with a toast. No existing flows are wired to the new dialogs yet — that happens in the stacked PRs.Changes
IdentityNotConnectedDialog,IdentityAlreadyLinkedDialog,SwitchAccessMethodDialogcomponentsContinueOnAnotherDeviceDialogfromAuthWizardinto a standalone component