deck-plans: fix blanks-first + lift compareWithEmptyLast helper#64
Merged
Conversation
Fixes #63. /deck-plan exhibited the same blanks-first defect on first load that PR #62 fixed for /vehicle-type. Lift the empty-last comparator into a generic src/utils/compareWithEmptyLast.ts and route all three call sites (vehicle-types, deck-plans, search-side useDataViewTableLogic) through it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Simplify pass on PR feedback: - Drop the duplicate getDeckPlanSortValue in deckPlanViewConfig.ts; import from the new module instead. - Replace inline deckPlanSortVal in VehicleTypeDetails.tsx with the shared getDeckPlanSortValue + OrderBy from useDeckPlans. - Convert switch default arms in both *SortValue.ts files to an exhaustive `never` check so adding a new OrderBy column is a compile error rather than silently falling through to '' (which would re-introduce the blanks-last bug for the new column). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Browser-level coverage for the blanks-first defect: stub the DeckPlans GraphQL query with 5 named + 5 null-name rows, visit /deck-plan, and assert page 1 lists Plan Alpha → Plan Echo before any null-name rows. A second test toggles to desc and confirms blanks stay last (not flipped to the top). - e2e-tests/fixtures/deck-plans-mock.json: mixed-name fixture payload - e2e-tests/no-auth/autosys-helpers.ts: interceptDeckPlansQuery helper - e2e-tests/no-auth/deck-plan-name-sort.spec.ts: the spec Both tests pass on Chromium and Firefox. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
NOTE: This is a client-side workaround. The real bug is in Sobek's GraphQL serialization, which emits empty NeTEx Name elements as whitespace strings (e.g. "\n ") rather than null or "". File a Sobek-side issue to fix the source; once that lands, drop the `v.trim() === ''` branch from compareWithEmptyLast and revert the e2e fixture to plain nulls. Why this commit: until Sobek changes, the previous predicate (`v === '' || v == null`) let whitespace strings through as "real" values; ASCII '\n' (10) and ' ' (32) are < 'a' (97), so blank-name rows still dominated page 1 of /deck-plan even after PR #62/#63's empty-last logic. Fixture-driven unit + e2e tests passed because they used null, not whitespace. Patch: add `isEmpty` predicate that trims string values before the emptiness check. 0 remains a real numeric value (vehicle-types `length: 0` unaffected). Verified live against `npm run local`: page 0 now shows named rows (ids 9 "aa" + 10 "zz") first, whitespace-name rows parked at the end. - Whitespace regression case added to deckPlanSortValue.test.ts (77/77). - e2e fixture mixes null + whitespace + tab/newline strings so the Playwright spec exercises the predicate (4/4 green). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dynnamitt
requested review from
GunnarAtEgde and
Copilot
and removed request for
Copilot
- src/utils/compareWithEmptyLast.ts: export isEmpty so future call sites inherit the same Sobek-aware trim semantics; hoist `dir` constant to drop one ternary per branch. - e2e-tests/no-auth/autosys-helpers.ts: extract interceptGraphQLQuery + loadFixture helpers; both interceptVehicleTypesQuery and interceptDeckPlansQuery become one-liners. - src/data/vehicle-types/vehicleTypeSortValue.test.ts: mirror the whitespace regression case from deckPlanSortValue.test.ts — same comparator, same Sobek backend, same exposure to entur/sobek#121. - e2e-tests/no-auth/deck-plan-name-sort.spec.ts: drop redundant waitForLoadState('networkidle') — the toHaveAttribute assertion on total-entries already gates render. 78/78 unit, 24/24 e2e (Chromium + Firefox). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 11, 2026
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.
Summary
/deck-planno longer shows blank Name column on first load — rows missing an optional NeTEx Name park at the end regardless of asc/desc, just like vehicle-types post-vehicle-types: blanks-last sort + drop deckPlan.description #62.src/utils/compareWithEmptyLast.tsand routes all three call sites through it:vehicleTypeSortValue(refactor, behaviour-preserving),useDeckPlans(bug fix),useDataViewTableLogic(search-side path now matches default-view ordering).deckPlanSortValue.test.tsmirroringvehicleTypeSortValue.test.tsas a regression spec.Test plan
npm run test— 76/76 pass (6 new deck-plan cases)npm run build— greennpm run lint— 0 errors (only pre-existing warnings unrelated to this change)npm run check— Prettier clean/deck-plan: first load shows populated names, blank rows at the end; toggling Name sort asc↔desc keeps blanks at the end/vehicle-type: no regression🤖 Generated with Claude Code