test(e2e): make the harness worktree-safe — one source for the port, one for the database - #49
Merged
Merged
Conversation
playwright.config.ts and e2e/helpers.ts each carried an independent copy of the test database URL and of port 3111. Overriding one and not the other seeded one database while the pages read another; a sibling worktree holding 3111 caused Playwright to attach to it and report a pass that never touched the branch under test. e2e/env.ts is now the only place either value is decided, imported by both. The dev server port and a dedicated Postgres container are derived from a hash of the worktree path, so concurrent worktrees cannot collide. Reuse of an existing server is granted only when its cwd and DATABASE_URL prove it belongs to this worktree and this run. CI is unaffected: it sets CI=true, which keeps the historical localhost:5433/authgd_test default and disables provisioning.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughChangesIsolated E2E environment
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/ops.md`:
- Around line 469-479: Update the earlier concurrency warning in docs/ops.md to
remove the claim that npm test and npm run test:e2e share authgd_test; retain or
qualify the warning about concurrent npm test runs if that remains accurate,
consistent with the isolated database behavior described in the e2e section.
In `@e2e/env.ts`:
- Around line 70-75: Update the APP_PORT and DB_PORT configuration in e2e/env.ts
to validate explicit E2E_PORT and E2E_DB_PORT overrides as finite integers
between 1 and 65535; reject invalid values such as non-numeric strings, negative
numbers, zero, fractions, and Infinity by throwing before the runner starts,
while retaining portFor defaults when the variables are unset.
In `@e2e/provision.ts`:
- Around line 38-44: Update docker() to invoke spawnSync with a bounded timeout
so blocked Docker commands cannot prevent ensureTestDatabase() from returning;
when the command times out, return a failed result with stderr describing the
timeout and including the Docker arguments for context, while preserving normal
stdout/stderr handling for completed commands.
In `@e2e/server-guard.ts`:
- Around line 129-139: Update the process handling around the databaseUrl and
dbRecreated checks in the server guard so stop(pid) is called only after
conclusive harness ownership is established, such as validating a PID record
created by the managed E2E server. When ownership cannot be proven, reject the
run with an actionable error instead of sending SIGTERM; preserve the restart
behavior for confirmed-owned processes.
In `@scripts/e2e-clean.ts`:
- Around line 18-25: Update the Docker cleanup result handling around
removed.status and removed.stdout in the e2e cleanup flow: distinguish
successful removal from a confirmed “no such container” response, remove the
migration stamp only in those cases, and throw for other Docker failures so
cleanup cannot silently proceed with a running container.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 62ee3c35-ee4d-4ef5-94d6-b7411e8a5a3e
📒 Files selected for processing (8)
docs/ops.mde2e/env.tse2e/helpers.tse2e/provision.tse2e/server-guard.tspackage.jsonplaywright.config.tsscripts/e2e-clean.ts
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
The prior mobile pass proved there was no horizontal *page* scroll at
320px. It did not prove the admin tables were usable, and they were not:
globals.css had one layout breakpoint and no position:sticky anywhere.
To reach a row's controls you scrolled the region right, which carried
the character name off the left edge — so you pressed a 28px control
that changes someone's tier with nothing on screen saying whose. On a
product whose promise is "derole, don't boot", that is a
wrong-person-deroled bug waiting for a bad night.
Sticky headers on accounts and audit, and a pinned first column on both,
so the identifying value survives the scroll right. The pin needs an
opaque cell, which cannot inherit the translucent row hover; it is
flattened with a measured color-mix that lands within 1/255 of the
neighbouring cells. Not applied to the runs tables: getSyncStatus caps
them at five rows, so they have no vertical scroll range and a sticky
header there could not be tested.
Narrow paths for the two tables that forced the most sideways scroll.
Below 40rem the runs table gives up its 44rem floor and its ISO stamp
for a relative time, and audit does the same: 992px wide with a pin
eating 69% of the region becomes 544px with a pin at 28%, taking forced
scroll at 320px from 706px to 258px. The exact instant stays in a
visually-hidden span in both, so nothing is destroyed to fit. 258px is
the honest floor, not a way station — four legible columns plus a
non-zero Details column exceed a 286px region no matter how it is cut.
Two pre-existing defects surfaced on the way. Audit's Details column
measured 0px at every viewport: table-layout:fixed plus a colgroup
summing 47.75rem left nothing for the one unsized column, so the payload
disclosure was unclickable. And the accounts identity chain used ??,
which walks straight past an empty string, so a blank character name
rendered aria-label="Note for " — a row with no identity at all, in the
column whose entire job is saying whose tier is about to change. Blank
names are now filtered before the pick ("" sorts first and would win it)
and the tier, cryo, auto and note controls all carry the row identity.
Two cascade traps found by reading computed style rather than the
stylesheet: the runs and audit width floors were declared after the
40rem block, so every same-specificity narrow override was silently
void; and audit's colgroup widths were inline styles, which outrank a
media query outright. Both moved.
Every geometry assertion in this change was shown to fail without its
rule before being kept. toBeInViewport is not used for any of them — an
element painted over by a sticky cell still passes it, so occlusion is
measured rect-vs-rect via e2e/geometry.ts. One pre-existing audit
assertion was found to have been silently disabled by this branch's own
visually-hidden span, whose overflow:hidden matched the test's
clipped-cell skip guard; the guard now excludes it.
Superseded by main in the meantime and deliberately not carried over:
an E2E_PORT override for the test harness, which #49 replaced with
per-worktree hashed ports and server-ownership proof.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
The prior mobile pass proved there was no horizontal *page* scroll at
320px. It did not prove the admin tables were usable, and they were not:
globals.css had one layout breakpoint and no position:sticky anywhere.
To reach a row's controls you scrolled the region right, which carried
the character name off the left edge — so you pressed a 28px control
that changes someone's tier with nothing on screen saying whose. On a
product whose promise is "derole, don't boot", that is a
wrong-person-deroled bug waiting for a bad night.
Sticky headers on accounts and audit, and a pinned first column on both,
so the identifying value survives the scroll right. The pin needs an
opaque cell, which cannot inherit the translucent row hover; it is
flattened with a measured color-mix that lands within 1/255 of the
neighbouring cells. Not applied to the runs tables: getSyncStatus caps
them at five rows, so they have no vertical scroll range and a sticky
header there could not be tested.
Narrow paths for the two tables that forced the most sideways scroll.
Below 40rem the runs table gives up its 44rem floor and its ISO stamp
for a relative time, and audit does the same: 992px wide with a pin
eating 69% of the region becomes 544px with a pin at 28%, taking forced
scroll at 320px from 706px to 258px. The exact instant stays in a
visually-hidden span in both, so nothing is destroyed to fit. 258px is
the honest floor, not a way station — four legible columns plus a
non-zero Details column exceed a 286px region no matter how it is cut.
Two pre-existing defects surfaced on the way. Audit's Details column
measured 0px at every viewport: table-layout:fixed plus a colgroup
summing 47.75rem left nothing for the one unsized column, so the payload
disclosure was unclickable. And the accounts identity chain used ??,
which walks straight past an empty string, so a blank character name
rendered aria-label="Note for " — a row with no identity at all, in the
column whose entire job is saying whose tier is about to change. Blank
names are now filtered before the pick ("" sorts first and would win it)
and the tier, cryo, auto and note controls all carry the row identity.
Two cascade traps found by reading computed style rather than the
stylesheet: the runs and audit width floors were declared after the
40rem block, so every same-specificity narrow override was silently
void; and audit's colgroup widths were inline styles, which outrank a
media query outright. Both moved.
Every geometry assertion in this change was shown to fail without its
rule before being kept. toBeInViewport is not used for any of them — an
element painted over by a sticky cell still passes it, so occlusion is
measured rect-vs-rect via e2e/geometry.ts. One pre-existing audit
assertion was found to have been silently disabled by this branch's own
visually-hidden span, whose overflow:hidden matched the test's
clipped-cell skip guard; the guard now excludes it.
Superseded by main in the meantime and deliberately not carried over:
an E2E_PORT override for the test harness, which #49 replaced with
per-worktree hashed ports and server-ownership proof.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
The prior mobile pass proved there was no horizontal *page* scroll at
320px. It did not prove the admin tables were usable, and they were not:
globals.css had one layout breakpoint and no position:sticky anywhere.
To reach a row's controls you scrolled the region right, which carried
the character name off the left edge — so you pressed a 28px control
that changes someone's tier with nothing on screen saying whose. On a
product whose promise is "derole, don't boot", that is a
wrong-person-deroled bug waiting for a bad night.
Sticky headers on accounts and audit, and a pinned first column on both,
so the identifying value survives the scroll right. The pin needs an
opaque cell, which cannot inherit the translucent row hover; it is
flattened with a measured color-mix that lands within 1/255 of the
neighbouring cells. Not applied to the runs tables: getSyncStatus caps
them at five rows, so they have no vertical scroll range and a sticky
header there could not be tested.
Narrow paths for the two tables that forced the most sideways scroll.
Below 40rem the runs table gives up its 44rem floor and its ISO stamp
for a relative time, and audit does the same: 992px wide with a pin
eating 69% of the region becomes 544px with a pin at 28%, taking forced
scroll at 320px from 706px to 258px. The exact instant stays in a
visually-hidden span in both, so nothing is destroyed to fit. 258px is
the honest floor, not a way station — four legible columns plus a
non-zero Details column exceed a 286px region no matter how it is cut.
Two pre-existing defects surfaced on the way. Audit's Details column
measured 0px at every viewport: table-layout:fixed plus a colgroup
summing 47.75rem left nothing for the one unsized column, so the payload
disclosure was unclickable. And the accounts identity chain used ??,
which walks straight past an empty string, so a blank character name
rendered aria-label="Note for " — a row with no identity at all, in the
column whose entire job is saying whose tier is about to change. Blank
names are now filtered before the pick ("" sorts first and would win it)
and the tier, cryo, auto and note controls all carry the row identity.
Two cascade traps found by reading computed style rather than the
stylesheet: the runs and audit width floors were declared after the
40rem block, so every same-specificity narrow override was silently
void; and audit's colgroup widths were inline styles, which outrank a
media query outright. Both moved.
Every geometry assertion in this change was shown to fail without its
rule before being kept. toBeInViewport is not used for any of them — an
element painted over by a sticky cell still passes it, so occlusion is
measured rect-vs-rect via e2e/geometry.ts. One pre-existing audit
assertion was found to have been silently disabled by this branch's own
visually-hidden span, whose overflow:hidden matched the test's
clipped-cell skip guard; the guard now excludes it.
Superseded by main in the meantime and deliberately not carried over:
an E2E_PORT override for the test harness, which #49 replaced with
per-worktree hashed ports and server-ownership proof.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
The prior mobile pass proved there was no horizontal *page* scroll at
320px. It did not prove the admin tables were usable, and they were not:
globals.css had one layout breakpoint and no position:sticky anywhere.
To reach a row's controls you scrolled the region right, which carried
the character name off the left edge — so you pressed a 28px control
that changes someone's tier with nothing on screen saying whose. On a
product whose promise is "derole, don't boot", that is a
wrong-person-deroled bug waiting for a bad night.
Sticky headers on accounts and audit, and a pinned first column on both,
so the identifying value survives the scroll right. The pin needs an
opaque cell, which cannot inherit the translucent row hover; it is
flattened with a measured color-mix that lands within 1/255 of the
neighbouring cells. Not applied to the runs tables: getSyncStatus caps
them at five rows, so they have no vertical scroll range and a sticky
header there could not be tested.
Narrow paths for the two tables that forced the most sideways scroll.
Below 40rem the runs table gives up its 44rem floor and its ISO stamp
for a relative time, and audit does the same: 992px wide with a pin
eating 69% of the region becomes 544px with a pin at 28%, taking forced
scroll at 320px from 706px to 258px. The exact instant stays in a
visually-hidden span in both, so nothing is destroyed to fit. 258px is
the honest floor, not a way station — four legible columns plus a
non-zero Details column exceed a 286px region no matter how it is cut.
Two pre-existing defects surfaced on the way. Audit's Details column
measured 0px at every viewport: table-layout:fixed plus a colgroup
summing 47.75rem left nothing for the one unsized column, so the payload
disclosure was unclickable. And the accounts identity chain used ??,
which walks straight past an empty string, so a blank character name
rendered aria-label="Note for " — a row with no identity at all, in the
column whose entire job is saying whose tier is about to change. Blank
names are now filtered before the pick ("" sorts first and would win it)
and the tier, cryo, auto and note controls all carry the row identity.
Two cascade traps found by reading computed style rather than the
stylesheet: the runs and audit width floors were declared after the
40rem block, so every same-specificity narrow override was silently
void; and audit's colgroup widths were inline styles, which outrank a
media query outright. Both moved.
Every geometry assertion in this change was shown to fail without its
rule before being kept. toBeInViewport is not used for any of them — an
element painted over by a sticky cell still passes it, so occlusion is
measured rect-vs-rect via e2e/geometry.ts. One pre-existing audit
assertion was found to have been silently disabled by this branch's own
visually-hidden span, whose overflow:hidden matched the test's
clipped-cell skip guard; the guard now excludes it.
Superseded by main in the meantime and deliberately not carried over:
an E2E_PORT override for the test harness, which #49 replaced with
per-worktree hashed ports and server-ownership proof.
guarzo
added a commit
that referenced
this pull request
Aug 4, 2026
The prior mobile pass proved there was no horizontal *page* scroll at
320px. It did not prove the admin tables were usable, and they were not:
globals.css had one layout breakpoint and no position:sticky anywhere.
To reach a row's controls you scrolled the region right, which carried
the character name off the left edge — so you pressed a 28px control
that changes someone's tier with nothing on screen saying whose. On a
product whose promise is "derole, don't boot", that is a
wrong-person-deroled bug waiting for a bad night.
Sticky headers on accounts and audit, and a pinned first column on both,
so the identifying value survives the scroll right. The pin needs an
opaque cell, which cannot inherit the translucent row hover; it is
flattened with a measured color-mix that lands within 1/255 of the
neighbouring cells. Not applied to the runs tables: getSyncStatus caps
them at five rows, so they have no vertical scroll range and a sticky
header there could not be tested.
Narrow paths for the two tables that forced the most sideways scroll.
Below 40rem the runs table gives up its 44rem floor and its ISO stamp
for a relative time, and audit does the same: 992px wide with a pin
eating 69% of the region becomes 544px with a pin at 28%, taking forced
scroll at 320px from 706px to 258px. The exact instant stays in a
visually-hidden span in both, so nothing is destroyed to fit. 258px is
the honest floor, not a way station — four legible columns plus a
non-zero Details column exceed a 286px region no matter how it is cut.
Two pre-existing defects surfaced on the way. Audit's Details column
measured 0px at every viewport: table-layout:fixed plus a colgroup
summing 47.75rem left nothing for the one unsized column, so the payload
disclosure was unclickable. And the accounts identity chain used ??,
which walks straight past an empty string, so a blank character name
rendered aria-label="Note for " — a row with no identity at all, in the
column whose entire job is saying whose tier is about to change. Blank
names are now filtered before the pick ("" sorts first and would win it)
and the tier, cryo, auto and note controls all carry the row identity.
Two cascade traps found by reading computed style rather than the
stylesheet: the runs and audit width floors were declared after the
40rem block, so every same-specificity narrow override was silently
void; and audit's colgroup widths were inline styles, which outrank a
media query outright. Both moved.
Every geometry assertion in this change was shown to fail without its
rule before being kept. toBeInViewport is not used for any of them — an
element painted over by a sticky cell still passes it, so occlusion is
measured rect-vs-rect via e2e/geometry.ts. One pre-existing audit
assertion was found to have been silently disabled by this branch's own
visually-hidden span, whose overflow:hidden matched the test's
clipped-cell skip guard; the guard now excludes it.
Superseded by main in the meantime and deliberately not carried over:
an E2E_PORT override for the test harness, which #49 replaced with
per-worktree hashed ports and server-ownership proof.
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
1 participant
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.
The e2e harness had two hardcoded shared resources. In a repo routinely worked from several concurrent worktrees, both collide.
Port 3111 +
reuseExistingServer: !process.env.CI. If a sibling worktree held 3111, Playwright silently attached to it and the suite exercised that worktree's code against that worktree's database — and passed. Reproduced on this branch: a fully green10 passed (11.3s)from a run that never touched its own code.Two independent
DATABASE_URLdeclarations.playwright.config.tsset the server's;e2e/helpers.tshad its own hardcoded default for seeding. Override one and not the other and the tests seed one database while the pages read another — every assertion fails on missing content.Port isolation alone does not fix the second one:
resetDb()TRUNCATEs on everybeforeEach, so two worktrees on different ports still wipe each other's rows in a sharedauthgd_test.What changed
e2e/env.ts— the single source. Hashes the worktree's absolute path into a dev-server port and a database port (stable within a worktree, distinct between worktrees), and derives the database URL. Imported by bothplaywright.config.tsande2e/helpers.ts, so configuring one without the other is unrepresentable.E2E_PORT,E2E_DB_PORT,TEST_DATABASE_URLoverride.e2e/provision.ts— starts a per-worktree Postgres container and migrates it. Runs at config load, notglobalSetup: Playwright orderswebServerstartup before global setup.e2e/server-guard.ts— resolvesreuseExistingServer. Reuse is granted only when the process holding the port is proved to belong to this worktree and to be reading this run's database. Anything else is a hard error naming the override, never a silent attach.scripts/e2e-clean.ts/npm run test:e2e:clean— remove this worktree's container.docs/ops.md— replaces the superseded manualCREATE DATABASE authgd_test_mineworkaround.npm run test:e2eis unchanged and remains the single entry point.Decisions
reuseExistingServer: conditional, not flat. Once the port is per-worktree a collision is unlikely, but hashes collide and people setE2E_PORTby hand — and the failure mode is a false green, which nothing prompts you to investigate. So reuse is earned, not assumed: the guard reads/proc/<pid>/cwdand/proc/<pid>/environand restarts the server if it predates a recreated database or points at a different one. Under CI it staysfalse, the same answer!process.env.CIgave.Auto-provision over a documented manual step. A manual step is exactly what
docs/ops.mdalready had, and its existence is why the bug survived — a setup instruction that can be skipped will be. The cost is a Docker dependency; when Docker is absent the error namesTEST_DATABASE_URLas the way out. The container is kept warm between runs rather than torn down: a throwaway would payinitdbplus a full migration every time, and would strand a reused dev server against a database that no longer exists.CI
No change needed, and none made.
ci.ymlstands up a Postgres service on host 5433 and deliberately sets no override; GitHub Actions setsCI=trueautomatically.e2e/env.tskeeps that exact default behind anIS_CIbranch — per-worktree isolation arrives by override, never by changing the shared default.Verification
Two worktrees, concurrent suites:
A 0.5s sampler across the run saw exactly two distinct tuples over 44 samples, with 21 iterations where both servers were live simultaneously:
Guard, with worktree B forced onto A's port while A's server was up:
Cold run after
npm run test:e2e:clean(container fully removed, reprovisioned and migrated):10 passed (12.6s).npm run typecheckclean.npm run lint— 0 errors, 4 pre-existingno-img-elementwarnings.npm run format:check— "All matched files use Prettier code style!"Scope
No application code and no test assertions changed.
tests/helpers/db.tscarries a third copy of the 5433 default for the unit suite; left alone deliberately as out of scope for this harness change.Summary by CodeRabbit
New Features
npm run test:e2e:cleanto remove test containers and reset migration state.Documentation