Default Account Flow - Continue #3384

danblackadder · 2025-09-30T14:54:36Z

Motivation

We are updating the account flow to handle default accounts with a single button for "Continue" and altering the way we handle the multiple account option

Changes

Update Checkbox to correctly handle a disabled state on hover
Add Toggle.Svelte
Update create within CreateAccount.svelte to handle a default option
Add EditAccount.svelte view which is currently hidden behind the feature flag
Add editAccount to accounts route
Update text and components across the auth flow hidden behind the feature flag
Get defaultAccount added to account/page.ts

Tests

All testing has been done by eye on Windows with Chrome - e2e to be updated once feature flag is to be switched off

danblackadder · 2025-09-30T14:55:11Z

src/frontend/src/lib/components/ui/Checkbox.svelte

    "hover:bg-bg-primary_hover",
    "has-checked:bg-bg-brand-solid has-checked:hover:bg-bg-brand-solid_hover has-checked:border-none",
-    "has-disabled:border-border-disabled has-disabled:bg-bg-disabled_subtle has-disabled:text-fg-disabled_subtle",
+    "has-disabled:border-border-disabled has-disabled:bg-bg-disabled_subtle has-disabled:hover:bg-bg-disabled_subtle has-disabled:text-fg-disabled_subtle has-disabled:cursor-not-allowed",


The checkbox still presented itself as clickable even though it was disabled...

danblackadder · 2025-09-30T15:13:18Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/account/+page.svelte

+  let isEditAccountDialogVisible = $state<boolean>(false);
  let isAuthorizing = $state(false);
  let tooltipAnchorRef = $state<HTMLElement>();
+  let accountToEdit = $state<AccountInfo | undefined>(undefined);


Separated this from the isEditAccountDialogVisible as will be the same account we want to then delete if we press the "Delete account" button from within the edit dialog

graphite-app · 2025-09-30T16:19:19Z

src/frontend/src/lib/components/ui/Toggle.svelte

+    type="button"
+    aria-labelledby={label ? "toggle" : undefined}
+    class={`bg-bg-tertiary relative flex h-5 w-9 flex-row items-center rounded-full p-0.5 ${toggled ? "justify-end" : "justify-start"}`}
+    onclick={() => onClick(!toggled)}


In Svelte, the native DOM onclick attribute should be replaced with Svelte's event directive syntax on:click. The Svelte syntax properly binds component methods and ensures proper event handling within the component lifecycle.

on:click={() => onClick(!toggled)}

This change applies to both button elements in this component that currently use onclick.

Suggested change

onclick={() => onClick(!toggled)}

on:click={() => onClick(!toggled)}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Copilot

Pull Request Overview

This PR implements a default account flow for the authorization process with feature flag-controlled updates. The primary change introduces a simplified "Continue" button that uses a default account, replacing the previous multiple account selection approach.

Updated authorization flow to support default account selection with a single "Continue" button
Added toggle functionality to enable/disable multiple accounts view
Introduced account editing capabilities for renaming and setting default accounts

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
continue/+page.svelte	Added default account flow with "Continue" button and multiple accounts toggle
account/+page.ts	Added defaultAccount retrieval to page load function
account/+page.svelte	Added account editing functionality and multiple accounts toggle
EditAccount.svelte	New component for editing account names and default settings
CreateAccount.svelte	Updated to support setting accounts as default during creation
Toggle.svelte	New toggle component for multiple accounts feature
Checkbox.svelte	Fixed disabled state styling to prevent hover effects

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-30T17:57:49Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/continue/+page.svelte

+          name: defaultAccount.name[0],
+          accountNumber: defaultAccount.account_number[0],
+        });
+        await authorizationStore.authorize(defaultAccount.account_number[0]);


Potential undefined access when defaultAccount.name or defaultAccount.account_number arrays are empty. Consider adding null checks or providing fallback values.

Suggested change

name: defaultAccount.name[0],

accountNumber: defaultAccount.account_number[0],

});

await authorizationStore.authorize(defaultAccount.account_number[0]);

name: defaultAccount.name?.[0] ?? "",

accountNumber: defaultAccount.account_number?.[0] ?? "",

});

await authorizationStore.authorize(defaultAccount.account_number?.[0] ?? "");

Copilot · 2025-09-30T17:57:49Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/account/+page.svelte

+          )
+          .then(throwCanisterError);
+      }
+      accounts = [...accounts, account];


This line adds the edited account to the accounts array instead of updating the existing account. This will create duplicate entries. Should update the existing account in the array instead.

Suggested change

accounts = [...accounts, account];

accounts = accounts.map((a) =>

JSON.stringify(a.account_number) === JSON.stringify(account.account_number) ? account : a

);

Copilot · 2025-09-30T17:57:49Z

src/frontend/src/lib/components/ui/Toggle.svelte

+<label class="flex flex-row items-center gap-2">
+  <button
+    type="button"
+    aria-labelledby={label ? "toggle" : undefined}


The aria-labelledby references 'toggle' but the actual id is 'toggle-label' (line 22). This will break accessibility as the label association won't work correctly.

Copilot · 2025-09-30T17:57:50Z

src/frontend/src/lib/components/ui/Toggle.svelte

+  <input
+    type="checkbox"
+    role="switch"
+    aria-labelledby={label ? "toggle" : undefined}


The aria-labelledby references 'toggle' but the actual id is 'toggle-label' (line 22). This will break accessibility as the label association won't work correctly.

Copilot · 2025-09-30T17:57:50Z

src/frontend/src/lib/components/ui/Toggle.svelte

+    aria-labelledby={label ? "toggle" : undefined}
+    checked={toggled}
+    {disabled}
+    onchange={() => onClick(!toggled)}


The hidden checkbox's onchange handler calls onClick(!toggled), but the button's onclick already does the same. This could cause the toggle to be called twice when using keyboard navigation, resulting in no state change.

Suggested change

onchange={() => onClick(!toggled)}

lmuntaner

Looking good to me!

Can you add some screenshots to better see the changes? Thanks!

lmuntaner · 2025-10-01T08:13:30Z

src/frontend/src/lib/components/ui/Toggle.svelte

+    <div class="size-4 rounded-full bg-white"></div>
+  </button>
+  {#if label}
+    <span id="toggle-label" class="text-text-secondary text-sm font-medium"


Why is the id needed?

lmuntaner · 2025-10-01T08:20:28Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/continue/+page.svelte

        <li class="contents">
          <ButtonCard
-            onclick={() => handleContinueAs(account)}
+            onclick={() => handleContinueAs()}


nitpick onclick={handleContinueAs}?

lmuntaner · 2025-10-01T08:20:51Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/continue/+page.svelte

-            onclick={() => handleContinueAs(account)}
+            onclick={() => handleContinueAs()}
            disabled={loading}
+            class="!justify-center"


Why do you need !justify-center?

lmuntaner · 2025-10-01T08:23:40Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/account/+page.svelte

    ),
  );

+  let toggled = $state<boolean>(true);


toggled what? Maybe toggledMultipleAccounts would be more descriptive?

lmuntaner · 2025-10-01T08:24:22Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/account/+page.svelte

+    try {
+      // TODO: fix account.account_number = []
+      // currently when requesting to update_account against a primary account
+      // as there is no account number is causes this to panic


What panics? The backend?

lmuntaner · 2025-10-01T08:24:50Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/account/+page.svelte

+        )
+        .then(throwCanisterError);
+      if (isDefault) {
+        await $authenticatedStore.actor


Could we do the requests in parallel?

lmuntaner · 2025-10-01T08:27:08Z

src/frontend/src/routes/(new-styling)/authorize/(panel)/account/+page.svelte

+                    variant="tertiary"
+                    class="no-card-hover"
+                    onclick={(event) => {
+                      event.stopPropagation();


Why is event.stopPropagation(); needed?

Default account flow

16d581b

danblackadder commented Sep 30, 2025

View reviewed changes

danblackadder added 7 commits September 30, 2025 15:55

Merge remote-tracking branch 'origin/main' into dan/default-account

26fd1d2

Delete empty delete file

926851c

Update import to

4803b34

Make create use feature flag

c3b534a

Close Edit modal on error

fd103f9

Put fedCM back

83f92eb

Remove unused relative

c17a306

danblackadder changed the title ~~Default account flow~~ Default Account Flow - Continue Sep 30, 2025

danblackadder added 2 commits September 30, 2025 16:11

Remove unused import

511abac

Remove unused import

ab6218c

danblackadder commented Sep 30, 2025

View reviewed changes

danblackadder requested review from sea-snake, aterga and lmuntaner September 30, 2025 15:24

danblackadder marked this pull request as ready for review September 30, 2025 16:17

graphite-app bot reviewed Sep 30, 2025

View reviewed changes

aterga requested a review from Copilot September 30, 2025 17:56

Copilot AI reviewed Sep 30, 2025

View reviewed changes

lmuntaner reviewed Oct 1, 2025

View reviewed changes

	onclick={() => onClick(!toggled)}
	on:click={() => onClick(!toggled)}

-      accounts = [...accounts, account];
+      accounts = accounts.map((a) =>
+        JSON.stringify(a.account_number) === JSON.stringify(account.account_number) ? account : a
+      );

Default Account Flow - Continue #3384

Are you sure you want to change the base?

Default Account Flow - Continue #3384

Uh oh!

Conversation

danblackadder commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

graphite-app bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

lmuntaner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danblackadder commented Sep 30, 2025 •

edited

Loading