IS-11570 Add browser back/forward navigation to the HAAPI stepper - #266
IS-11570 Add browser back/forward navigation to the HAAPI stepper#266aleixsuau wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds browser Back/Forward integration for the HAAPI stepper by persisting step progress into window.history state and replaying reproducible steps when users navigate the browser history.
Changes:
- Refactors the
HistoryNavigationadapter to provide statelessreplace/push/gooperations and an unsubscribe-basedpopstatelistener API. - Introduces
useHaapiStepperHistoryNavigation(plus a headless mounting component) to sync stepper history ↔ browser history and skip non-reproducible entries during navigation. - Adds unit tests for reproducible-action classification and history navigation behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/haapi-react-app/src/shared/util/browser-apis.ts | Refactors the browser history adapter API to support stepper navigation needs (replace/push/go + unsubscribe). |
| src/haapi-react-app/src/shared/feature/history/useHaapiStepperHistoryNavigation.ts | Implements the core stepper↔browser history synchronization and navigation skipping logic. |
| src/haapi-react-app/src/shared/feature/history/useHaapiStepperHistoryNavigation.spec.ts | Adds unit tests covering recording, skipping, snapping back, and forward-branch truncation. |
| src/haapi-react-app/src/shared/feature/history/reproducible-action.ts | Defines what actions/entries are safe to replay via history navigation. |
| src/haapi-react-app/src/shared/feature/history/reproducible-action.spec.ts | Tests reproducibility classification for links, GET/POST forms, and client operations. |
| src/haapi-react-app/src/shared/feature/history/HaapiStepperHistoryNavigation.tsx | Headless component that mounts the hook inside <HaapiStepper>. |
| src/haapi-react-app/src/App.tsx | Mounts <HaapiStepperHistoryNavigation /> inside the app’s stepper tree. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8388b37 to
695cb32
Compare
| const browserHistoryEntryIndex = { index: nextBrowserHistoryIndex } satisfies BrowserHistoryEntryIndex; | ||
| if (nextBrowserHistoryIndex === 0) { | ||
| // The first step reuses the history entry the app was loaded with instead of pushing a new one. | ||
| browserNavigation.replaceEntry(browserHistoryEntryIndex); |
There was a problem hiding this comment.
R1 ("Back on the initial step must stay in the app, no reload") isn't implemented.
No guard entry is seeded behind the first step. Confirmed live in the browser: exactly one replaceState({index:0}) and zero pushes before it. Pressing Back from the first step destroyed the JS execution context; the next document came back with window.name reset, navType: "navigate", history.state: null and a server-rendered title — a full cross-document reload out of the SPA, exactly as the ticket's R1 root-cause analysis predicts.
The ticket's plan says "implement R1 now"; if it's deferred to a follow-up, worth stating that in the PR description (see also my comment on the test that currently looks like it covers this).
One caveat for fairness: if the login page is opened in a brand-new tab with nothing behind it, Back is a no-op and R1 accidentally holds. The failure needs a prior entry in the tab — i.e. the normal "user clicked Log in on a client app" case.
| ); | ||
| // If we skipped over non-reproducible entries, move the browser onto the step we actually re-opened. | ||
| moveBrowserHistoryTo(destinationBrowserHistoryIndex, currentBrowserHistoryIndex, browserNavigation); | ||
| browserHistoryRef.current = { ...browserHistoryRef.current, index: destinationBrowserHistoryIndex }; |
There was a problem hiding this comment.
Concurrent replays desync the cursor from the displayed step, and Back then navigates forward.
advanceHaapiStepperToTheNextStep calls the void, fire-and-forget nextStep, and the cursor is updated unconditionally here with no loading guard. Two Back presses 30 ms apart (well inside one HAAPI round trip), reproduced twice in the browser:
popstate{1} -> popstate{0} -> push{1} -> push{2}
cursor: {index:2} UI: selector (entry 0)
Both late-resolving replays were mistaken for new steps and appended as forward entries. A single deliberate Back from that state then moved the cursor 2 -> 1 and the UI selector -> login form — i.e. Back moved the user forward. Second run: popstate{1} -> popstate{0} -> push{1}, cursor 1, UI on the login form. go() was never called to correct any of it.
Root cause: isSameStepperHistoryEntry compares only the last stepper entry against the browser entry at the current cursor, so an out-of-order replay looks like a brand-new step. Ignoring popstate while the stepper is loading, and/or tagging replays so their completion is recognised, would close it.
Related but not reproduced: on this same path, if a replay returns a problem step, nextStepAsync early-returns without appending a history entry (HaapiStepper.tsx:326) while the cursor has already moved. I tried to force that with a competing flow in the same session and the replay still succeeded, so I'd treat it as theoretical — but the same fix covers it.
| expect(fake.position()).toBe(0); // snapped back onto link@0 | ||
| }); | ||
|
|
||
| it('keeps the user in the flow when going back from the first step', () => { |
There was a problem hiding this comment.
This test asserts a popstate the browser cannot emit, so the suite reports coverage for the one behaviour that is actually broken.
fake.navigate(-1) clamps pos to -1 and fires the listener with state = null. In a real browser the first step replaces the loaded entry's state ({index:0}), so there is no in-document null-state entry behind it — going back leaves the document entirely (verified: execution context destroyed, full reload). That makes the findReproducibleBrowserHistoryEntryIndex(...) === -1 -> snap-back path unreachable in the back direction.
The path is genuinely reachable going forward — verified in the browser: popstate{2} -> go(-1) -> popstate{1}. Suggest keeping the forward case and renaming or removing this one until a guard entry actually exists.
| entries = [...entries.slice(0, pos + 1), state]; | ||
| pos = entries.length - 1; | ||
| }), | ||
| go: vi.fn((delta: number) => { |
There was a problem hiding this comment.
The fake's go() is synchronous; the real one is async — so the specs can't see a duplicated replay.
Running this hook against the fake as written vs. one that defers move(delta): sync -> nextStep fires twice for a single Back press; async (real-browser behaviour) -> once. The specs don't notice because they assert toHaveBeenCalledWith rather than call counts.
The real browser does behave correctly here (one corrective cycle observed), so this is test fidelity rather than a production bug — but deferring go() and asserting call counts would let the suite detect the re-entrancy Copilot flagged, instead of silently tolerating it.
| go(delta: number): void; | ||
|
|
||
| /** The state attached to the current history entry (null when none was set). */ | ||
| getState(): unknown; |
There was a problem hiding this comment.
getState() has no consumer.
Added to the interface and to BrowserHistoryNavigation, but a grep across ui-kit/src finds only the declaration, the implementation and the test fake — nothing reads it. Dead API surface unless something downstream is planned.
Review summary (IS-11570)Reviewed against the ticket's acceptance criteria, then verified in a browser against a local Identity Server (Vite dev server on :8443 proxying to idsvr :9443, Mechanics are clean: Confirmed working in the browser:
Blocking, in my view: the polling classification and the concurrent-replay desync — both produce user-visible wrong navigation in flows that ship by default — plus R1. Two findings with no line to hang off: 1. No e2e coverage. 2. The ticket's SDK scope isn't in this PR. IS-11570's "In scope", AC5 and Technical Implementation call for Minor: the ticket says form actions are reproducible when |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@urre @luisgoncalves I couldn't find this specific case in Confluence. How should we fix it? |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…not implemented). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>


Jira: https://curity.atlassian.net/browse/IS-11570
Adds
useHaapiStepperHistoryNavigationand mounts<HaapiStepperHistoryNavigation />in the app, wiring the browser Back/Forward buttons to the HAAPI stepper:pushStatebehaviour.Testing
Run a HAAPI flow and use the browser Back/Forward buttons: reproducible steps (GET links / GET forms) are revisited, non-reproducible ones (POST forms, client operations) are skipped.
Unit tests:
npx vitest run src/shared/feature/history.Deferred: R1 (Back on the first step)
R1 ("Back on the initial step must stay in the app, no reload") is not implemented in this PR. The first step reuses the document's load history entry, so pressing Back from it is a cross-document navigation that reloads the app — no
popstatefires, so the hook can't intercept it.Planned follow-up: seed a sentinel history entry behind the first step at mount, so Back lands on it (a same-document
popstate) and snaps forward to the first step — trapping Back inside the flow (confirmed acceptable for the auth flow). Theit.skip('R1: …')test documents this gap so it isn't mistaken for covered.