[Paul-AUB]

fix(Account): address review feedback after #1545

🤔 What

• Keep the account deletion guidance visible when the account request fails.
• Close the dialog after the contact link is clicked and show a new-tab icon.
• Extend the Cypress scenario to cover the failed account request and dialog closing.
• Include nested Cypress specs in Firefox CI and share one production build across all browser jobs.

Follow-up to #1545 and its inline review comments. Issue #1543 was closed by that merge.

🤷‍♂️ Why

The deletion instructions do not depend on account data, so an API failure should not hide them. The contact link needs a clear external-navigation cue and should leave the original page ready when the user returns. Firefox jobs should include future nested specs without rebuilding the app for every spec.

🔍 How

The deletion section now renders outside the account-data guard. Its existing external AppLink closes the dialog on click and displays OpenInNewIcon. The E2E workflow discovers specs recursively, builds the app once, uploads dist, then restores it in each browser job.

🔗 Related API PR

None.

🧪 Testing

• Full ESLint check: node node_modules/eslint/bin/eslint.js . (0 errors).
• Prettier check on modified files with --bracket-same-line.
• Parsed .github/workflows/test.yml with the installed yaml package.
• git diff --check.
• Cypress tests on Chrome and Firefox in CI.
• Shared production build in CI.

📸 Previews

No new screenshots. The visible change is the external-link icon on the contact button.

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1546.westeurope.azurestaticapps.net

[ClemRz]

Code review focused on correctness. Three findings below (one medium, two low), plus a small style nit. The rest of the change looks sound: the widened find matches Cypress's default specPattern, the artifact path matches outDir: 'dist', and moving the build out of the cypress-io/github-action step loses no environment (the old step had no env: block).

[ClemRz]

retention-days: 1 breaks "Re-run failed jobs".

A Firefox shard flakes, and the maintainer comes back the next morning and clicks Re-run failed jobs. GitHub does not re-run e2e-build because it succeeded, so every e2e job fails at Restore built app with Unable to find any artifacts for the associated workflow — a failure with no relation to the code under test, and no way to recover other than pushing an empty commit or re-running the whole workflow.

Before this PR the e2e job rebuilt from source via build: yarn build, so a next-day re-run just worked.

Suggest raising the retention to a few days, which costs very little for a dist this size:

          retention-days: 7

[Paul-AUB]

Good catch, fixed. Bumped to retention-days: 7 and added a comment on the field explaining why (so nobody trims it back without thinking about the re-run flow).

[ClemRz]

The guard was deleted outright rather than narrowed to the error case.

The stated intent is that an API failure should not hide the deletion instructions, but isPending is also true during the ordinary initial fetch. So a user opening /ui/account on a slow connection now sees the three grey skeleton rectangles immediately followed by a fully rendered "Account deletion" card — loading placeholders and live content side by side. With useAccount's retry: 1 and the default 1000 ms retryDelay, the failure path shows the same mixed state for a second or so before the error Alert appears.

Keeping the loading guard satisfies the review comment without changing the loading state:

      {!isAccountLoading && <AccountDeletionSection />}

Minor, related: the comment just above ("Rendered even when the account fetch failed…") is scoped to OfflineDataSection; this new line now inherits it implicitly. Worth extending the comment to cover both, or moving it.

[Paul-AUB]

Fixed. Guard is now {!isAccountLoading && <AccountDeletionSection />} so the card doesn't render next to the skeletons on slow connections, and the error path still shows it because isAccountLoading (i.e. isPending) flips to false once the last retry rejects.

I also split the comment above into two blocks — one for OfflineDataSection (still rendered on error), one for AccountDeletionSection (rendered on error but hidden while pending) — so the intent stays anchored to each line.

[ClemRz]

Stripping href means the test never exercises the interaction the new onClick introduces.

The change under test makes one click both close the dialog and follow the link. removeAttr('href') removes the second half, so the assertion only proves the dialog closes when the anchor is inert — which is not the risky case. Removing a clicked <a> from the DOM inside its own click handler cancels the navigation in Blink and Gecko, so "closes the dialog" and "opens the contact form" are in genuine tension here.

This is currently safe only by accident: StandardDialog uses MUI Dialog's default Fade exit (~195 ms) and grottoTheme.js has no transitionDuration override, so the anchor stays connected well past the browser's default action. A future transitionDuration={0}, a custom TransitionComponent, or a theme-level transition override would silently break the button — and this test would still pass.

Stubbing the navigation instead keeps the href intact and covers both halves:

cy.window().then(win => cy.stub(win, 'open').as('openContact'));
cy.get('[data-testid="account-deletion-contact-link"]').click();
cy.get('[role="dialog"]').should('not.exist');

Secondary point: removeAttr mutates a live React node, so any re-render of the open dialog between the invoke and the click restores the attribute. That makes the step nondeterministic rather than merely incomplete.

[Paul-AUB]

Fixed. Dropped removeAttr('href') and now stub window.open in onBeforeLoad:

onBeforeLoad: win => {
  win.localStorage.setItem('selectedLanguage', 'en');
  cy.stub(win, 'open').as('winOpen');
}

The anchor keeps its real href/target, no live React node gets mutated, and the click exercises the same path a user would trigger.

[ClemRz]

Nit: the sibling external-link button in this same file (line 306) uses endIcon={<OpenInNewIcon fontSize="small" />}. Worth matching so the two external-link cues render at the same size.

[Paul-AUB]

Fixed — <OpenInNewIcon fontSize="small" /> now, matches the sibling at line 306.

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1546.westeurope.azurestaticapps.net

[urien]

Pour moi cela fonctionne et c'est clair pour les utilisateurs

[ClemRz]

Re-reviewed after f0146c3. Three of the four earlier comments are correctly fixed: retention-days: 7 with the reasoning recorded, the !isAccountLoading guard (consistent with the sibling checks at 1428/1447/1453), and the icon size.

The Cypress one needs another pass — the stub can't intercept this click, so the test changed shape without gaining the coverage. Details inline, plus three smaller things noticed on the new lines.

[ClemRz]

This stub cannot intercept the click, so the comment above it is not accurate.

AppLink renders an external href as a plain anchor — <a href target="_blank" rel="noopener noreferrer"> (src/components/common/AppLink.jsx:74-84). Native anchor activation never goes through window.open, and Cypress doesn't patch window.open or strip target either. So line 44's .click() still spawns a real window and still fetches https://en.wikicaves.org/contact from CI on every run — inside the per-spec Firefox runner, the browser the workflow comment calls out as prone to reconnect failures. @winOpen is also never asserted, so nothing will catch the drift.

Dropping removeAttr('href') was the right move, but the regression this test should guard is still uncovered: whether closing the dialog cancels the navigation of the anchor being unmounted. It stays safe only by accident — MUI Dialog's default ~195 ms Fade exit keeps the anchor connected past the default action, and a transitionDuration={0} or a custom TransitionComponent would break the button with this test still green.

One caveat if you reach for invoke('removeAttr', 'target') instead: with target gone the click navigates the AUT itself to an external origin, which trips Cypress's cross-origin error and invalidates the [role="dialog"] assertion that follows. Recording the default action and then cancelling it keeps the test on-origin while covering both halves:

cy.get('[data-testid="account-deletion-contact-link"]').then($a => {
  $a[0].addEventListener('click', e => {
    cy.wrap(e.defaultPrevented).as('navBlocked');
    e.preventDefault();
  });
});
cy.get('[data-testid="account-deletion-contact-link"]').click();
cy.get('@navBlocked').should('eq', false);
cy.get('[role="dialog"]').should('not.exist');

The simplest honest alternative is to delete the stub and its comment and let the test cover dialog dismissal only — that is what it actually verifies today.

[Paul-AUB]

✅ Fixed in f4b49ea: removed the ineffective window.open stub. The test records the native anchor click before cancelling navigation, asserts that default navigation was not already blocked, and checks that the dialog closes.

[ClemRz]

isPending is also true for a paused query, so the scenario this PR is about is still not covered.

networkMode: 'offlineFirst' (src/conf/queryClient.js:88) means the first request always leaves, and only the retries pause. An offline user whose /account request fails therefore settles at status: 'pending', fetchStatus: 'paused' — the page stays on the three skeletons indefinitely, with neither the error Alert nor the deletion card. That is exactly the "the account request failed, keep the deletion guidance visible" case the guard was supposed to preserve, reintroduced on the offline path.

!isLoading (isPending && isFetching) renders the card in that state while still hiding it during the ordinary in-flight fetch, which is the behaviour the comment describes. An explicit fetchStatus !== 'paused' check reads more plainly if you prefer it spelled out.

[Paul-AUB]

✅ Fixed in f4b49ea: the loading guard now uses isLoading, so paused offline retries no longer hide the deletion card behind skeletons. Added an E2E case that fails the initial account request, goes offline, and checks that the card is visible without the error alert.

[ClemRz]

Small efficiency point on the new job: this installs the full root dependency tree purely to produce dist, and cypress@^15 is a web-app devDependency — so its postinstall downloads the (hundreds of MB) browser binary in the one job that never runs Cypress. setup-node's cache: 'yarn' doesn't cover ~/.cache/Cypress, so it's paid on every run and eats into the shared-build speedup this PR is going for.

      - name: Install dependencies
        run: yarn
        env:
          CYPRESS_INSTALL_BINARY: 0

[Paul-AUB]

✅ Fixed in f4b49ea: the e2e-build install sets CYPRESS_INSTALL_BINARY=0; only the browser-running jobs need the Cypress binary.

[ClemRz]

Possible flake here — flagging as unverified, since I didn't measure the rendered layout.

The 503 raises a global error toast (src/conf/queryClient.js:41-61), and SnackbarProvider is anchored bottom-center with the default 5 s auto-hide (src/pages/ApplicationShell.jsx:219). With retry: 1 the toast lands roughly a second after cy.wait('@getAccount'), so it is on screen exactly when this click fires. In the failure state the page is short — error Alert, OfflineDataSection, deletion card — so this button sits low enough that the toast may overlap it, which Cypress reports as "element is covered by another element".

Whether it actually overlaps at 1000×660 depends on heights I haven't measured, so this may well be fine as-is. Dismissing the toast (or asserting it, then waiting it out) before the click makes the test independent of how tall the page happens to be.

[Paul-AUB]

✅ Fixed in f4b49ea: the 503 test waits for the error snackbar to disappear before clicking the deletion button, avoiding an overlap regardless of page height.

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1546.westeurope.azurestaticapps.net

[ClemRz]

Re-reviewed after f4b49ea. CYPRESS_INSTALL_BINARY: 0 and the isPending → isLoading swap are both correct — I checked the one risk in the rename, that a disabled query is also isPending && !isFetching, but AuthChecker only renders its child when isAuth is true, so AccountPage never mounts in that state. Removing the window.open stub also fixes the real problem there: CI no longer fires a request at en.wikicaves.org on every run.

The two replacement assertions don't hold up though — both pass unconditionally, so they read as coverage without providing any. One of them is the pattern I suggested last round; the correction is mine, not yours. Details inline, plus a race in the new offline test.

Non-blocking observation: in the paused state the page now renders no skeletons, no error Alert and no account sections — just the offline-data and deletion cards, which the new test asserts. It meets the goal of this PR, but the user sees their account info silently missing with no explanation. An "you appear to be offline" affordance would close that gap; reasonable to leave out of scope here.

[ClemRz]

This assertion can never fail — my suggested pattern was wrong, apologies for the detour.

Two independent reasons:

1. Ordering. React 19 delegates events to the root container (createRoot(document.getElementById('root')), src/index.jsx:9). A listener attached to the anchor itself fires in the target phase, before the event bubbles up to #root where React's onClick={() => setIsDialogOpen(false)} runs. So defaultPrevented is sampled before the dialog close could have had any effect.
2. Wrong signal. The regression is the browser skipping the default action because the anchor was disconnected mid-dispatch. That never sets defaultPrevented — it stays false in both the healthy and the broken case.

So expect(wasNavigationBlocked).to.eq(false) holds whether or not the button works.

What discriminates is observing state after React's handler and checking connectedness instead. document sits above #root, so its bubble listener runs last:

let anchorStillConnected;
cy.document().then(doc => {
  doc.addEventListener(
    'click',
    event => {
      anchorStillConnected = event.target.isConnected;
      event.preventDefault();
    },
    { once: true }
  );
});
cy.get('[data-testid="account-deletion-contact-link"]').click();
cy.then(() => expect(anchorStillConnected).to.eq(true));
cy.get('[role="dialog"]').should('not.exist');

If that feels like more machinery than the risk warrants, dropping the check entirely is also defensible — just don't leave an assertion that implies the navigation is covered when it isn't.

[Paul-AUB]

Fixed in a8b2e10. The document-level click listener now checks that the clicked link is still connected after React handles the click, then prevents external navigation. The test also verifies that the dialog closes.

[ClemRz]

#notistack-snackbar never exists in this app, so this line is a no-op.

That id comes from notistack's default MaterialDesignContent, but every variant — error included — is overridden with AppSnackbar (src/pages/ApplicationShell.jsx:107-113), and AppSnackbar renders a bare SnackbarContent with only ref, className and style (src/components/common/AppSnackbar.jsx:24). No id is ever emitted.

The selector therefore matches nothing, should('not.exist') is satisfied on the first attempt, and the 10 s timeout never engages — so the toast is still on screen when the click below fires, and the overlap risk is exactly what it was before. Worse than leaving it out, since the line implies the problem was handled.

.notistack-SnackbarContainer is the real hook. Adding a data-testid to AppSnackbar would be sturdier — nothing else in the suite targets snackbars yet, so there's no convention to match.

[Paul-AUB]

Fixed in a8b2e10. AppSnackbar now exposes a stable data-testid. The test first confirms the error toast appears, then waits for that actual toast to disappear before clicking the deletion button.

[ClemRz]

This test has to win a ~1 s race, and nothing pins the ordering.

retry: 1 with the default 1000 ms retryDelay schedules the second attempt about a second after the first 503. This block has to get onLine set and the offline event dispatched inside that window. On a slow runner the retry fires first, the intercept returns 503 again, the query settles to error, the Alert renders — and line 97's cy.contains('An error occurred. Please try again.').should('not.exist') fails. A flake that will be read as a real regression.

Setting it in onBeforeLoad removes the race entirely. networkMode: 'offlineFirst' still issues the first request, so cy.wait('@getAccount') keeps working and the retry is paused from the start — no event dispatch needed:

cy.visit('/account', {
  onBeforeLoad: win => {
    win.localStorage.setItem('selectedLanguage', 'en');
    Object.defineProperty(win.navigator, 'onLine', {
      configurable: true,
      value: false
    });
  }
});

[Paul-AUB]

Fixed in a8b2e10. The offline state is set in onBeforeLoad, before the first account request and its retry. The delayed offline event was removed, so the test no longer races the retry timer.

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1546.westeurope.azurestaticapps.net

[github-actions]

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1546.westeurope.azurestaticapps.net