Repository navigation
UN-4345 [FEAT] Frontend v1 E2E harness and the frontend-v1-e2e group - #2355
Merged
Merged
Conversation
Adds tests/e2e/ui-v1: a self-contained Playwright suite (own config, lock, Biome, strict TS) with login/org, seed, v1Preview and distribution tag fixtures, and five-state, accessibility, theme-matrix, redirect and old-version helpers. P0-T03 dependencies go through contract/registry.ts (stubbed; built ahead of P0-T03). Adds the frontend-v1-e2e rig group and the CI install steps for it. tests/e2e/ui and the ui group are untouched.
$host drops the port, so Django's CSRF same-origin check compared the browser's Origin (http://localhost:3000) with http://localhost and rejected every unsafe request the SPA made. The org handshake failed and browser-driven specs never got past the session loader.
- five-states: don't await open() while the loading response is held, so an open that waits for networkidle (app.goto) cannot deadlock; hand unstubbed rest to route.fallback() so a spec's own routes still apply. - seed: a row created but not resolvable to a delete URL fails teardown. - old-version: assert v0 rendered only after the SPA has mounted. - Self-tests for the impact threshold, the networkidle open, the pass-through path and that a custom setTheme replaces the default.
Deepak-Kesavan
marked this pull request as ready for review
October 26, 2026 04:52
Contributor
 [Medium impact] The PR appears safe to merge; no new actionable issue was found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… clear the Sonar findings loginAuth0 now starts where the Login button does (/api/v1/login on the app origin, with the app as Referer so the callback returns there), bounds every IdP wait and gives the session fixture its own budget, so a failed sign-in skips or errors with its reason instead of a generic timeout. Sonar: drop the redundant RoutePattern alias, keep the stub registry from being a provably-empty literal, String.raw and \w in the regex helpers, no nested template, reverse cleanups in place with CSRF headers read once, accept only string or number ids, explicit assertions in the a11y self-tests, and NOSONAR on the sequential test.step loops.
…-tests Cloud CI exports UNSTRACT_RIG_EXTRA_MANIFESTS for the whole job, so load_groups() merged groups.cloud.yaml into the synthetic manifests the test_cli tests patch in. The first cloud requires_platform group (frontend-v1-e2e-cloud, which must depend on e2e-smoke) then failed validation against manifests that never define e2e-smoke. Clear the variable per test; the overlay tests set it themselves.
…its id The frontend's /:orgName segment is organization.name from the /organization/<id>/set response (useSessionValid), which differs from the id on cloud. The session now carries both: orgId for API calls, orgName for app.goto and the registry route checks. A harness self-test pins id != name. Also note that the P0-T01 scaffold renders no v1 root yet, so the marker stays assumed.
jaseemjaskp
reviewed
Nov 4, 2026
jaseemjaskp
left a comment
jaseemjaskp
left a comment
Contributor
There was a problem hiding this comment.
A few small things from a review pass over the harness. Overall it looks solid: seed teardown, the held-response release in the five-states helper, the $http_host fix and the CI caching all checked out.
…a cookie preview value The role-credentials skip message only upper-cased the role, while the lookup also maps non-alphanumerics to underscores, so for "org-admin" it named a variable that is never read. Both now use roleEnvKey(). A cookie preview value is URI-encoded JSON, since raw JSON is not a valid RFC 6265 cookie-value; storage overrides keep plain JSON. The README and the V1_PREVIEW contract note that the app must decode it. Also corrects the Session.orgId doc to the /api/v1/unstract/<orgId>/ prefix.
Deepak-Kesavan
added
the
main-only
…the run's The self-test assumed OSS, so it failed in the cloud and on-prem runs it is tagged for. otherDistributionsGrep takes the distribution to build for, and the test checks oss, cloud and onprem explicitly.
Deepak-Kesavan
requested review from
jaseemjaskp,
muhammad-ali-e and
ritwik-g
November 12, 2026 04:32
…re it skips Sonar S2699 flagged the placeholder as a test without assertions. It now checks there is no previewable page, the condition it skips on, so a guard that drifts from the data fails instead of skipping.
ritwik-g
approved these changes
Nov 12, 2026
ritwik-g
left a comment
ritwik-g
left a comment
Contributor
There was a problem hiding this comment.
Thanks Deepak, this is a thorough turn-around. I checked each fix against the code at 35e59a3 (the latest commit since only adds a premise check to the preview placeholder):
- the theme switch now lands on the real
.v1root; - the registry self-tests fail on an empty list;
- untagged and mistyped tests fail instead of vanishing;
- a missing role fails under the rig;
- lint and typecheck run in CI;
- the group gates;
- traces stay out of CI.
The e2e run at this head shows frontend-v1-e2e at 45 passed, 1 skipped (the preview render, until a page is declared but not registered).
Two small new comments inline, neither blocking.
Contributor
Author
|
@ritwik-g @jaseemjaskp all your review points are addressed, and each thread has a reply naming its fix commit:
CI is green, Greptile is 5/5 on c89de0b and Sonar has 0 open issues. Re-review requested. @muhammad-ali-e, adding you for review too. |
…ip; one theme init script per page - The harness no longer seeds unstract.v1.lastUser. The round trip opens the app root (a v0 route, where the session loads) and waits for the app to record the user, which proves the session reaches the version switch. The preference is written once with page.evaluate, so the reload checks the app's own persistence. - defaultSetTheme disposes of the previous theme init script before adding the next: Playwright leaves the order of several undefined.
…nd does A pattern ending in /* kept a literal asterisk, so a splat redirect was checked against the wrong URL. params['*'] now fills it with its slashes kept and each segment encoded, and omitted means empty.
jaseemjaskp
approved these changes
Nov 12, 2026
vishnuszipstack
left a comment
vishnuszipstack
left a comment
Contributor
There was a problem hiding this comment.
Review of PR 2355 — UN-4345 Frontend v1 E2E harness and the frontend-v1-e2e group
Mode: INITIAL. Reviewed head 8533666a2a, merge base d0489de6d7ff332a46bb0db18fb9ab7639597a11, base freshened (base_fetch: ok), 36 files, 2845 lines (matches GitHub). Rubric: unstract plugin v2.1.80.
Signals: lenses 4, 7, 11, 13, 14 raised (auth fixtures, a contract read from the frontend registry, a CI job and a gating group, a new lockfile, tests).
boundary-sweep: 3 sinks traced, clean (the sign-in error message names role, backend URL and mode only; CI traces are off so passwords are not recorded; axe results hold page markup).
Security deep-dive: degraded — the find and filter passes ran in one agent, one after the other, with no separate filter sub-tasks. Nothing survived: no pull_request_target, no untrusted ${{ }} in a run step, no committed session state.
Verdict — APPROVE WITH NITS
Summary — Critical: 0 · High: 0 · Medium: 8 · Low: 11 · Lenses run: 19/19 (2 not covered, 6 N/A; see checklist).
The harness reads the frontend rather than restating it: the preview key, the per-user old-version key, the last-user key, the theme key, PAGES, DEMO_PAGES and TOGGLE_PAGES all come from frontend/src/v1. The only restated strings (data-ui-version="v1", data-theme) match ThemeProvider.tsx:45 and are pinned by positive assertions. Static checks pass: tsc --noEmit exit 0, biome ci . 29 files clean, playwright test --list 47 tests in 9 files (46 under onprem, as the tag gate intends), python -m tests.rig validate OK with 29 groups. No spec ran: there was no browser and no compose stack here, so everything about runtime behaviour below is read from the code.
Most of the Mediums are about the template this sets. Twenty-four page tasks will copy these helpers, so a vacuous pass or a misleading error in a helper shows up again in every page spec.
Findings
Posted inline on the diff; index:
-
- [Medium] [Lens 7] —
REGISTRYrebuilds the app's live set by hand and leaves out its distribution filter —tests/e2e/ui-v1/contract/registry.ts:50
- [Medium] [Lens 7] —
-
- [Medium] [Lens 2, 14] — The harness type-checks frontend files under stricter flags than the frontend uses, and that check sits in front of the whole e2e tier —
tests/e2e/ui-v1/tsconfig.json:9
- [Medium] [Lens 2, 14] — The harness type-checks frontend files under stricter flags than the frontend uses, and that check sits in front of the whole e2e tier —
-
- [Medium] [Lens 3] — The preview self-test hard-codes
UN19as undeclared, so it breaks the day a page task declares UN19 —tests/e2e/ui-v1/specs/harness/preview.spec.ts:41
- [Medium] [Lens 3] — The preview self-test hard-codes
-
- [Medium] [Lens 3, 10] — Every sign-in failure returns
nullwith its cause dropped, and in auto mode an OSS failure is reported asmode=auth0—tests/e2e/ui-v1/fixtures/auth.ts:274
- [Medium] [Lens 3, 10] — Every sign-in failure returns
-
- [Medium] [Lens 3] — Route checks skip any page whose routes need ids, on a promise ("the page spec covers them") that nothing checks —
tests/e2e/ui-v1/specs/harness/registry-routes.spec.ts:99
- [Medium] [Lens 3] — Route checks skip any page whose routes need ids, on a promise ("the page spec covers them") that nothing checks —
-
- [Medium] [Lens 13] — The "untagged test fails" rule is tested only as a pure function, never through the fixture that enforces it —
tests/e2e/ui-v1/specs/harness/tags.spec.ts:31
- [Medium] [Lens 13] — The "untagged test fails" rule is tested only as a pure function, never through the fixture that enforces it —
-
- [Medium] [Lens 8, 13] — Can the tolerated
networkidlewaits ever time out before the test does? —tests/e2e/ui-v1/fixtures/auth.ts:270
- [Medium] [Lens 8, 13] — Can the tolerated
-
- [Medium] [Lens 11, 17] — "Gating" holds only in OSS CI: on a runner without Node the group exits 5 and reads green, and tests/README.md still says Node groups are optional —
tests/groups.yaml:368
- [Medium] [Lens 11, 17] — "Gating" holds only in OSS CI: on a runner without Node the group exits 5 and reads green, and tests/README.md still says Node groups are optional —
-
- [Low] [Lens 3] —
UNSTRACT_E2E_A11Y_IMPACTaccepts prototype keys and silently ignores typos —tests/e2e/ui-v1/helpers/a11y.ts:46
- [Low] [Lens 3] —
-
- [Low] [Lens 3] —
expectFiveStatesandforEachThemepass having checked nothing when given an empty list —tests/e2e/ui-v1/helpers/five-states.ts:198
- [Low] [Lens 3] —
-
- [Low] [Lens 13] —
retries: 1in CI reports a test that passed on retry as a plain pass —tests/e2e/ui-v1/playwright.config.ts:47
- [Low] [Lens 13] —
-
- [Low] [Lens 15] — The "premise check" in the preview placeholder can never fail —
tests/e2e/ui-v1/specs/harness/preview.spec.ts:67
- [Low] [Lens 15] — The "premise check" in the preview placeholder can never fail —
-
- [Low] [Lens 13, 2] — The
fillPatterntest claims parity with the frontend that it does not have, andparamNamesis a second copy with different rules —tests/e2e/ui-v1/specs/harness/registry-routes.spec.ts:47
- [Low] [Lens 13, 2] — The
-
- [Low] [Lens 7] —
PreviewMechanismoffers cookie and sessionStorage variants the app never reads —tests/e2e/ui-v1/contract/registry.ts:78
- [Low] [Lens 7] —
-
- [Low] [Lens 7] —
Session.roleechoes the requested role and is never checked against the signed-in user —tests/e2e/ui-v1/fixtures/auth.ts:290
- [Low] [Lens 7] —
-
- [Low] [Lens 7] —
tags.tsrestates the distribution list instead of importingDISTRIBUTIONS, and treats""differently from the app —tests/e2e/ui-v1/fixtures/tags.ts:35
- [Low] [Lens 7] —
-
- [Low] [Lens 13] — A wall-clock assertion in the five-states self-test —
tests/e2e/ui-v1/specs/harness/five-states.spec.ts:89
- [Low] [Lens 13] — A wall-clock assertion in the five-states self-test —
-
- [Low] [Lens 16] — Several comments say something slightly different from the code —
tests/e2e/ui-v1/fixtures/tags.ts:2
- [Low] [Lens 16] — Several comments say something slightly different from the code —
-
- [Low] [Lens 9] — The demo spec repeats two of the registry-driven browser checks —
tests/e2e/ui-v1/specs/P0-T09.demo.spec.ts:54
- [Low] [Lens 9] — The demo spec repeats two of the registry-driven browser checks —
Lens checklist
- Spec & intent — See findings #8 (and the description, under Unanchored findings)
- Architectural fit & precedent — See findings #2, #13
- Correctness & edge cases — See findings #3, #4, #5, #9, #10
- Security — Not covered — single-pass scan, filters unavailable, surface read directly: the CI job uses
pull_requestwithcontents: read, the one interpolated output is an actions/cache flag, traces are off, and no session state is written - Data integrity & migrations — N/A — seeds and tears down test orgs only; no schema or migration
- Concurrency — Clean (
workers: 1; per-test seeds with teardown; storage keys are per browser context) - API & contract compatibility — See findings #1, #14, #15, #16
- Reliability & resilience — See findings #7
- Performance & cost — See findings #19
- Observability — See findings #4
- Operational safety — See findings #8
- LLM/agent-specific — N/A — no model, prompt or agent path
- Testing — See findings #6, #7, #11, #13, #17
- Dependencies & build — See findings #2 (the new lockfile pins @playwright/test 1.62.1, axe-core, biome and typescript from registry.npmjs.org;
--frozen-lockfile --ignore-scriptsin CI) - Code quality — See findings #12 (biome ci on the suite: 29 files clean)
- Doc & comment accuracy — See findings #18
- Cross-document rule consistency — Not covered — merge-result sweep unavailable: git 2.34 here, the script needs 2.38 (tests/README.md checked by hand: finding #8)
- On-prem deployment experience — N/A — test harness and CI only; no chart, values or on-prem build path
- Scaling & workload lifecycle — N/A — no worker or scaling config
Assessed by me rather than an agent: lenses 4, 5, 6, 8, 11, 12, 14, 15 (pinned gate), 17, 18, 19, and the cross-check of finding #8 against the rig and the cloud workflow. No lifecycle agent was dispatched: the diff has no React components.
Open questions for the author
- Is making
frontend-v1-e2egating intended in this PR? If so, the description should say so, and should say how cloud CI is meant to treat it until unstract-cloud#1866's leg gets Node. - Should the harness compile frontend files under its own stricter flags, or should
frontend/src/v1/tsconfig.jsonadopt them?
Assumptions made
- No spec was run: no browser and no compose stack were available. The description's reported local pass was not reproduced.
cost: agents=7 wall=10:04 lenses-examined=13 of 19 files=36 lines=2845
- REGISTRY and UNREGISTERED_PAGES narrow to the run's distribution, as live.ts and activeEntries do; the distribution comes from the frontend's DISTRIBUTIONS, and an empty value reads as oss. - tsconfig matches frontend/src/v1's strictness flags, so the frontend files tsc pulls in are never checked more strictly than typecheck:v1. - Sign-in failures keep their step and cause; in auto mode both the oss and the auth0 failure are reported. - Registry checks test each page's org-only mappings, and skip naming the params only when none can be filled; the redirect and round-trip helpers take an only filter. - Untagged tests are proven failed through the gate fixture (test.fail). - Tolerated networkidle waits are capped at 10s (settle). - parseImpact rejects unknown and prototype keys; expectFiveStates and forEachTheme refuse empty lists; failOnFlakyTests in CI. - paramNames comes from the frontend; an omitted splat leaves no trailing slash; the preview mechanism is the localStorage one only. - Self-tests pick an undeclared key at run time, check the timeout by message, and cross-check the preview placeholder against raw data.
…cument the gating group - Install, lint and typecheck of tests/e2e/ui-v1 move ahead of the multi-minute image build, so a type error fails in seconds. - tests/README.md lists frontend-v1-e2e as gating (not optional, unlike frontend and ui) with its local install steps; the ui-v1 README and the group comment say the same, and that it is gating in OSS CI. - Comment fixes: the nginx comment quotes the loader text exactly.
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
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.
What
Adds the frontend v1 E2E harness (P0-T09, UN-4345) and the
frontend-v1-e2erig group, so every v1 page task can test itself in a real browser and report apart from v0. It runs on the P0-T03 route registry (#2433).Why
The page definition of done needs E2E, five states, Light/Dark/Unstract and a clean accessibility scan for every page, reported under frontend v1 and never mixed into v0 results (constraint 12, D10).
How
tests/e2e/ui-v1/: a self-contained suite, the same pattern astests/e2e/ui. It has its ownpackage.json+bun.lock,playwright.config.ts,tsconfig.json(the same strictness flags asfrontend/src/v1) andbiome.json. It is all TypeScript and imports nothing fromtests/e2e/ui/.contract/registry.tsreads the frontend's own registry data:registry/types.ts,entries.ts,validate.ts,patterns.tsandlib/oldVersion.ts, which import nothing Vite-specific.REGISTRYis what the E2E image renders as v1 in this run's distribution: registered pages plus P0-T03's demo pages.contract/v1-ui.tsholds the DOM markers (the v1 root and itsdata-theme) and the theme keys fromlib/theme.ts.session/app/role: login as a seeded user of a role (OSS mock-login, or auth0's hosted form) plus org selection. A failed sign-in names the step and cause for each mode tried.seed: rows created through the org-scoped API and deleted after the test. A leak fails the test.v1Preview: renders a declared but unregistered page through P0-T03'sunstract.v1.previewoverride.expectFiveStates: rest, loading, empty, error and no-permission, by intercepting the screen's API.expectAccessible:@axe-core/playwright, WCAG 2.0/2.1 A+AA.forEachTheme: writesunstract.v1.themeand asserts the v1 root renders each theme.expectV0RedirectsToV1expectOldVersionRoundTrip: for toggle pages. It waits for the app to record the signed-in user, then checks the per-user preference on, off and across a reload.specs/P0-T09.demo.spec.ts, the worked example:expectFiveStatesis proven by its self-tests onsupport/state-fixture.ts.frontend-v1-e2eintests/groups.yaml(requires_platform,depends_on: [e2e-smoke]). It gates the build in OSS CI. Theuigroup is untouched.ci-test.yaml, e2e job):VITE_V1_DEV_ROUTES=true(from UN-4271 [FEAT] Frontend v1 version switch and route registry #2433).bun install --frozen-lockfile, and the suite's lint and typecheck run before the image build.reports/frontend-v1-e2e/.tests/e2e/ui-v1, exact pins):@axe-core/playwright4.13.0,@playwright/test1.62.1 (same asui),@biomejs/biome2.3.13 andtypescript7.0.2 (both match frontend's lock),@types/node22.20.3.Also fixed: the e2e frontend proxy rejected every SPA write
tests/compose/frontend-nginx.test.confforwardedHost $host, which drops the port. Django's CSRF same-origin check then rejected every unsafe request from the SPA, so the org handshake got a 403 and the app sat on "Please wait while we prepare your session". It now forwards$http_host.Known gaps (not in this PR)
PersistentLoginsits inside the v0 tree), so the old-version preference can't apply there yet. This belongs to P0-T06, which mounts the session in the v1 tree. The round trip starts from a v0 page for this reason.frontend-v1-e2e-cloud) reads as skipped there until that e2e leg installs the suite and builds the frontend withVITE_V1_DEV_ROUTES=trueandVITE_DISTRIBUTION=cloud.STATE_TEST_IDSand a "Selecting elements" README section follow the v1data-testidconvention when the P1 core components land.serious; P8-T02 owns the final one.Can this PR break any functionality?
No app code changes. Three shared changes:
frontend-v1-e2egates the e2e job, so a red run fails CI. It needs itsnode_modulesand a browser; CI installs both, and locally seetests/e2e/ui-v1/README.md.Host, which only lets through requests that were wrongly rejected.tests/e2e/uiand theuigroup are unchanged.Related
How to test
frontend-v1-e2ereports as its own gating group.Checklist
tsc --noEmitclean intests/e2e/ui-v1, also in CI