Skip to content

bug: event table can rank on the previous view's metric after a client-side view switch #137

Description

@alchemydc

Raised by Copilot's review of #136. Splitting it out rather than fixing it there: it predates that PR and its premise deserves browser confirmation.

Symptom

Navigate from a class view (or the new raw view) to the PAX view of the same event — e.g. /events/<slug>/SS → /events/<slug>/pax — without a full page load. The rank column can read out of order (1, 5, 2, 9…), because the rows are sorted on one metric while the rank pills are computed on the other.

Mechanism

In apps/web/src/app/events/[slug]/leaderboard-table.tsx:

const [sorting, setSorting] = useState<SortingState>([
  { id: paxView ? "bestPaxMs" : "bestRawMs", desc: false },
]);

useState runs its initializer once per mount. Both event views are served by the same [class] route segment, so an App Router transition between them re-renders LeaderboardTable in the same tree position with new props — sorting survives, initialized from the previous view's paxView.

Meanwhile rankByRow / deltaByRow recompute correctly from the current paxView (they're in a useMemo keyed on it), so the two desync. Both directions misbehave:

Transition sorting after Effect
class/raw → pax bestRawMs Column exists in the PAX view, so rows really do sort by raw time while ranks read PAX
pax → class/raw bestPaxMs That column is conditional on paxMetric and no longer exists, so the sort is a no-op and rows fall back to buildLeaderboard's PAX-ascending order

Not a regression from #133

origin/main has the identical initializer, and class ↔ PAX navigation already existed there. #133 adds the raw view, which is a third way into the same transition — and, since it's the "All" landing pill, a more likely one — but it did not introduce the defect.

Before fixing

Confirm the premise in a browser. The whole thing rests on client state surviving an App Router transition between two values of the same dynamic segment. That is the expected behavior (React reconciles the same component type at the same tree position; Next does not key page segments by param), but it should be observed rather than assumed — reproduce on a preview deploy with an event that has both a heterogeneous class and PAX standings on, watching the rank column.

Fix options

  1. useEffect reset — setSorting([{ id: paxView ? "bestPaxMs" : "bestRawMs", desc: false }]) when paxView changes. Simple, but discards a user's deliberate sort on any view switch and renders once with the stale order first.
  2. Remount key upstream — key the component on the resolved view in event-class-page-view.tsx (key={navActive}). Kills the whole class of stale-client-state bugs in this component at once, at the cost of a full remount per view switch.

(2) is probably right given navActive is already computed and threaded, but it's a call worth making with the browser evidence in hand.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions