feat: show each character's current location on the account and members pages - #162
Merged
Conversation
A many-character members drawer pushes .scroller--tall past its height cap and gains a vertical scrollbar; on a classic-scrollbar platform that takes width out of the region mid-render, tipping an already near-full table into horizontal scroll as a side effect of nothing the user did. scrollbar-gutter: stable reserves that width unconditionally, so crossing the vertical-overflow threshold no longer changes the content box width. This container's headless Chromium renders scrollbars as zero-width overlays regardless of host OS or --disable-features=OverlayScrollbar (confirmed against both the drawer and a trivial always-overflowing probe element), so the horizontal-overflow fix itself is not verified by automated test here and needs confirmation in a real classic-scrollbar browser. What drawer-scroll.spec.ts does verify: a many-character drawer crosses the region's vertical-overflow threshold and a few-character one doesn't (real geometry, independent of scrollbar rendering), and that scrollbar-gutter: stable is the computed value on the scroll container. Applying the gutter to .scroller--tall shifted the true rightmost scrollLeft on that class's other users (admin accounts, audit) by a consistent, measured 10px versus the naive scrollWidth - clientWidth figure those tests' pin assertions relied on -- accounted for via TALL_SCROLLER_GUTTER_SLOP in e2e/geometry.ts rather than loosened silently.
The hardcoded 10px constant contradicted its own doc comment (which wrongly described scrollbar-gutter's reservation as inflating scrollWidth rather than shrinking clientWidth) and encoded this container's incidental scrollbar width rather than anything platform-invariant. pinGeometry now measures offsetWidth - clientWidth on the scroll region in the same evaluate() call and returns it as gutterWidth; admin.spec.ts and audit.spec.ts derive their tolerance from that measured value instead. The real figure (12px: ~10px from .scroller's `scrollbar-width: thin` plus 2px of border) closes the assertions cleanly with no residual slop. drawer-scroll.spec.ts's disclaimer no longer overstates what this environment can't see: scrollbar-gutter's reservation is genuinely verified here (offsetWidth - clientWidth > 0, now asserted directly); only the original bug's own mechanism -- an unreserved, classically-rendered scrollbar narrowing a box -- can't be reproduced under headless Chromium's overlay scrollbars.
esi-location.read_location.v1 (required), esi-universe.read_structures.v1 and esi-location.read_online.v1 (both optional) ship in one re-auth event, because the re-auth cost is per rollout and not per scope. The consent copy lands with them rather than after: without it every member's consent screen falls through to describeScope's fallback and says authGD cannot explain what it is asking for.
Adds five nullable location columns to character (system/station/structure ids, online flag, checked-at timestamp) and a universe_name cache table for resolved EVE names, keyed by id with a kind enum for refresh policy. Nothing reads or writes these yet -- persistence layer only.
resolveUniverseName's initial SELECT was outside the try/catch, so a DB read failure (connection drop, timeout) rejected instead of falling through to ESI. Wrap the read so it degrades to "no cached candidate," matching the function's never-throws contract. Add a test that forces the read to reject via a Proxy over the test db, and a comment recording that isCacheFresh is checked against input.kind rather than cached.kind because id alone is the primary key and EVE id ranges never collide across kinds.
Covers the two gaps a reviewer found in the initial location job: a two-character run where only one fails (proving the per-character try/catch isn't hoistable without breaking updated-count), and the needs_reauth branch's CAS on the refresh-token blob, both the match and the concurrent-rotation-miss cases. Also references LOCATION_SCOPES_OPTIONAL by index at both scope gates instead of re-typing the literals, and moves a test fixture's structure id out of the solar-system id range.
User-approved deviation from the plan's reference implementation: previously a throwing getOnline/getStructureName reached the outer catch and could mark a character needs_reauth on the strength of an optional scope, or abandon the write entirely. Wrap both optional reads in .catch(() => null) so a failure degrades the corresponding detail exactly like the scope being absent — the required getLocation read is untouched and still aborts the write on failure.
getLocatableCharacters has no ORDER BY (by design; adding one is out of scope for this job), so the two-character isolation test's teeth depended on which row Postgres happened to return first. Run the scenario with the failing character seeded first and then second, so the assertion holds regardless of physical row order — repo has form on this exact class of bug (3ba6405).
The location job persisted `tokenStatus: "needs_reauth"` under a CAS and left no durable record of it, unlike the equivalent state change in `token-health`. Mirror that job's pattern: - `.returning()` on the CAS, so a zero-row update (the row rotated or was reclaimed underneath) drops the stale decision and counts as a transient failure rather than being silently swallowed as `failed`. - Audit only on the TRANSITION into needs_reauth. A ~15-minute job would otherwise write ~96 identical rows a day per broken character. The `details` are deliberately NOT token-health's `missingScopes`: nothing was computed against config here. The stored `scopes` column still claims the grant and ESI refused the read anyway, so the payload names the scope whose read 403'd. That means one action with two payload shapes, so the audit summariser declares both — an undeclared key renders as `+N more`, which would report a hidden field that is not actually hidden. The catch block's "write NO location columns, not even locationCheckedAt" behaviour is unchanged; `errors.push` moved above the branching so the early-returning CAS-miss path still contributes to the error summary.
… four comments No behaviour change. - `src/jobs/location.ts`: delete the `.catch(() => null)` on the structure name lookup and the comment justifying it. `resolveUniverseName` is contractually non-throwing and the call already sits inside the optional-scope gate, so the guard's only live effect would be laundering a genuine TypeError into a silent `namesUnresolved++`. All three `resolveUniverseName` call sites are now unwrapped and consistent. - `e2e/sync.spec.ts`: adding `location` to the housekeeping group made that list render three items, but the assertion still said two — the one thing the per-task reviews could not see, since each saw only its own diff. Also assert `location` is reachable inside its group's list, which is the test's own stated purpose. - Four comments the branch invalidated: the `OPEN_WINDOW_SCOPE` line references in `login/page.tsx` (both re-derived), the "most frequent job runs every 30 minutes / three missed ticks" premise in `core/health.ts` (location now ticks every 15, so 90 minutes is six) and its restatement in `docs/ops.md`, and the "CONTACTS column's caption" in `account/page.tsx`, which named a column merged into STATUS. `STALE_AFTER_MS` is unchanged — it is hand-picked, not derived.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (45)
Comment |
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.
Show each character's current location on the account page and in the admin members drawer, as a second line under the character's name:
J123456 — Home Astrahus.Also carries two smaller fixes the same brainstorm turned up: the members-drawer scroll defect, and the account manifest's column crowding at 10+ characters.
What this adds
A
locationjob on a ~15-minute cron reads each character's position from ESI and writes five nullable columns oncharacter. Auniverse_namecache table resolves system/station/structure ids to names so the job doesn't re-ask ESI for facts that never change. The view layer formats one line per character; a singleCharacterLocationcomponent renders it on both pages, so they cannot drift into two spellings of the same fact.Three new ESI scopes.
esi-location.read_location.v1is required;esi-universe.read_structures.v1andesi-location.read_online.v1are optional — a character missing an optional scope still gets a location line with that detail omitted. Scopes are env-driven viaEVE_SSO_SCOPES, so the change lands in.env.example,playwright.config.tsanddocs/ops.mdrather than in application source.Decisions a reviewer should know
locationAsOfis the OLDEST reading, not the newest — deliberately unlikemapObservedAtsitting right beside it on the admin page.mapObservedAtreduces newest-wins because the ACL observation is one job run; location has a per-character clock, and a failed read does not advancelocationCheckedAt. Understating freshness is the safe direction. The two labels look adjacent and derive oppositely; that's intentional.On any read failure the job writes nothing for that character — not even
locationCheckedAt. Keeping the last known values and the last successful timestamp is what makes the "as of" label honest about staleness instead of the row silently blanking.A
needs_reauthverdict can only follow from the REQUIRED scope's own failure. Both optional reads degrade to null exactly as if the scope were absent. This was a deliberate deviation from the plan's reference implementation, which would have let an optional-scope 403 write a persistedneeds_reauth.Location data is current-value-only. No history table, no per-reading rows.
The
needs_reauthwrite is audited, and only on the transition. The CAS uses.returning(); a zero-row update means the row rotated underneath, so the stale decision is dropped and counted transient rather than written. The audit row fires only when the character was not alreadyneeds_reauth— without that guard a ~15-minute job writes ~96 identical rows a day per broken character. The payload is{ scope, detectedBy }, deliberately not token-health'smissingScopes: nothing was computed against config here, the storedscopescolumn still claims the grant, and ESI refused the read anyway.token.needs_reauththerefore has two writers with two payload shapes, sosummarize.tsdeclares both — an undeclared key renders as+N more, which would tell an admin two fields were withheld when nothing was.Account manifest: 6 columns → 4
Token, standings and map state merge into one STATUS cell. Three accessibility contracts moved with the merge and are worth checking: each of the three merged entries keeps a visible label (without the column headers, two of them read literally "ok");
aria-describedbymoved onto the merged<td>and appears exactly once; and the re-authorize control stays an<a>styled as a button, since it navigates.Members drawer scroll
scrollbar-gutter: stableon the tall drawer region. This fix is not verified by any automated test in this repo — headless Chromium renders scrollbars as zero-width overlays, so the original horizontal-overflow symptom cannot be reproduced in CI. It was applied on the instrument-independent measurement that many-character rows cross the vertical-overflow threshold (956 > 718) while few-character rows do not (530 == 530). Needs one confirmation in a real classic-scrollbar browser.Deploy note
For one window after deploy, most characters will read
needs_reauthand the new job will readmissingon the admin sync page. Both self-resolve — the first is members re-consenting to the new scopes, the second is the job's first tick.SYNC_MODE=liveremains a required secret.Migration
0009is purely additive: one CREATE TYPE, one CREATE TABLE, five nullable ADD COLUMNs. No already-applied migration is edited. Nothing touchesTOKEN_ENCRYPTION_KEYhandling or the OAuth state flow.Verification
/universe/structures/{id}/cannot be exercised faithfully against a mock, so the structure resolver needs one live check: a character docked in a player structure should render the real citadel name, and one docked where the corp has no access should incrementnamesUnresolvedwhile leaving the job statusok— a name failure must never turn the job amber.Known follow-up
src/jobs/contacts.tswrites the sameneeds_reauthCAS without an audit row. This PR fixes that in the location job; contacts is left alone deliberately, as a separate change.