Repository navigation
Tracknotes: a Resupply screen of its own - #238
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
eamon-b
left a comment
eamon-b
left a comment
There was a problem hiding this comment.
Review of #238 at f659003 (diff against feat/resupply-planner)
Summary
Resupply planning moves from a section at the foot of Plan, with its checkbox modal, to a stack screen of its own. Each turn-off is now one decision (a radio list, or a switch for a single place), with legs in between, a zero-days stepper, and a Resupply entry row near the top of Plan. "Plan resupply here" on a waypoint is now the same group decision.
The core claims hold up:
- Edits are built at apply time.
chooseResupplyEditor,zeroDaysStepEditorandwaypointResupplyEditorall compute from the documentapplyhands them.applyis serialised per trail (writeChains), so two quick taps build on each other. The legacy fallback is read inside the editor too, so a tap made just after "Clear choices" does not bring the old selection back. - Every write goes through
plans-store.apply, so it is synced through the outbox. Hiker inputs stay inplan-inputs-store. - No calculator is forked. All figures come from
@lib/resupply-planand all wording from@lib/resupply-display. - The removed modules have no references left (
ResupplyCard,ResupplySelectList,resupply-toggle,toggleResupplyStop,setResupplyStopSelected). Theuse-planned-resupplyread hooks behind the map rings, elevation marks, the List "Planned" chip and the DistanceStrip "Next resupply" are unchanged. - The
resupplyroute is still registered as a plainStack.Screen. The deep link resolves to the same file. - SOBO works. The screen and Plan both use
usePlanRoute(plan).trail(alternates spliced in, direction applied). The section params are route km on that same trail, and the groups are sorted in walking order.
Checks:
npm run check:mobilepassed locally: 171 suites and 2156 tests; typecheck clean; lint 0 errors and the 3 existing warnings.- Root
npx tsc --noEmitis clean. - CI is green on f659003, and the PR is MERGEABLE/CLEAN.
- Overlap with other open PRs: #236 (this PR's base) also edits
mobile/src/features/plan/__tests__/ResupplyCard.test.tsx, which this PR deletes. That is expected with a stacked PR, but if #236 changes that file again, the rebase will hit a modify/delete conflict. #237 touches no mobile files. - No emulator was attached, so nothing was checked on a device.
Findings (most serious first)
1. "Clear choices" leaves zero days behind, and they are still counted but can no longer be seen or changed.
mobile/src/features/plan/use-planned-resupply.ts:108-111 (resetResupplyStops) clears resupplyStops, but resupplyZeroDays is left alone. After the reset, every multi-place group is undecided. For an undecided stop, computeResupplyLegs counts the largest zero-days value among its ticked options (stopZeroDays; src/lib/resupply-plan.ts test at line ~1099 shows {b1: 0.5, b2: 2} giving 2). plannerRows (resupply-planner.ts:146) shows 0 for an undecided group, and ResupplyPlanner.tsx:347 shows the stepper only when one place is chosen.
Failure scenario:
- At Monarch Pass, choose Salida and add 2 zero days.
- Tap Clear choices.
- The header still reads "Days (incl. 2 days off)" and the leg into Monarch Pass still carries the 2 days. No row shows them, and no stepper can remove them.
The same happens when choosing Salida with zero days, then switching to Poncha Springs, then clearing: the hidden Salida days come back into the totals. They also sync to the web.
Fix options:
- clear
resupplyZeroDaysin the reset; - or prune the entry for any option that is no longer the chosen one when
chooseResupplyEditorruns; - or at least show the inherited figure on undecided groups.
2. In multi-place groups, the option details and the "More" button can't be reached with a screen reader.
At ResupplyPlanner.tsx:389-403, Radio is a Pressable (accessible by default) with accessibilityLabel={spokenLabel}, and it wraps OptionFacts, which holds its own "More"/"Less" Pressable (lines 423-433). An accessible parent becomes a single focus element: its label replaces the children's text, and the nested button is not separately focusable (always on iOS; on Android TalkBack merges it too).
Failure scenario: a VoiceOver user at Monarch Pass hears "Salida, radio button" and nothing else. They miss "Full resupply · 22.5 km hitch · Accepts boxes", the description, and the expand control, which are the facts the decision depends on. Single-place groups don't have this problem, because OptionFacts sits outside the Switch there.
Fix: put the facts in accessibilityHint or build them into the label, and move "More" outside the radio's Pressable.
3. Several controls share the same spoken label on a 70-group screen.
ResupplyPlanner.tsx:353/362: "Fewer zero days" / "More zero days" are identical on every chosen stop.:340: "None – carry through" is identical in every group.
Navigating by control, the hiker can't tell which turn-off they are changing. Fix: add the place or turn-off name, for example "More zero days at Salida" and "Carry through Monarch Pass".
4. React.memo on the rows has no effect, so every tap re-renders the whole list.
plannerRows (resupply-planner.ts:96-150) builds new row and names objects on every call, and it is re-run whenever stored, zeroDays or legs change, which is every edit. GroupRow (memoised on row) and LegRow (on leg/names) therefore always re-render. On the CDT (about 70 groups plus about 70 legs, with FlatList's default window covering most of them) one radio tap re-renders every mounted row and recomputes optionFacts for each option. That is probably tolerable on a recent phone, but the file header says rows are memoised when in practice they are not.
Fix: reuse unchanged row objects (key them and compare choice/zeroDays/leg fields), or memoise GroupRow on primitive props.
5. With an invalid section, the Resupply row and the screen behind it disagree.
If the hiker sets the start at or after the end on Plan, the row reads "Choose where to resupply" (resupplyEntryText: hasData is false, plan.tsx:509-513), even when a plan is already made. Tapping it opens the screen without params (validSection is false), which shows the whole trail with real figures. The screen's "Choose a start before the end on the Plan screen." caption (ResupplyPlanner.tsx:133) can never show, because the screen always falls back to the whole trail.
Fix: have the row say the section is invalid in that case.
6. Single-place groups read differently in two places before any plan is made (low).
With no selection stored, a single-place group is chosen on the resupply screen (the switch is on). The waypoint detail, through isSelected/plannedResupplyIds(null), says "Plan resupply here" for the same place. Tapping it makes the default selection explicit; nothing is lost. This is consistent with the shared semantics, but it may confuse someone moving between the two screens.
7. Leftover code (nit).
saveResupplyStops(use-planned-resupply.ts:95) no longer has any caller.useStoredResupplyStopsstill returnsstoredand its doc comment ("a write has to start from the list the hiker is looking at") now describes the opposite of what the editors do.
Verdict
The structure is sound, and the edit-race and sync claims check out. Fix #1 before merging: it silently inflates the hiker's day totals with zero days they can't see or remove. #2 and #3 are accessibility regressions worth fixing in the same pass. #4–#7 can follow. Nothing has been run on a device yet.
🤖 Generated with Claude Code
f659003 to
74f132e
Compare
Review of #236 and #238: - A section that starts at a town no longer walks out of it again: the leg arriving there, in the section before, counted the side trip both ways, so two sections meeting at a town added up to more than the whole trail (0-50 + 50-100 was 109 km against 106). Only a stop at the trail's own start, which no leg arrives at, has its walk out on the leg leaving. - With a camp plan, a leg's arrival comes from that plan alone: its rest nights are already in its dates, and adding the resupply zero days too counted the same rest twice. The day number is now the calendar day the date falls on (walking days plus earlier rest days), so day and date agree. Zero days still count in the summary's total days. - Only a stop decided on one place takes zero days; an undecided one takes none rather than the most any of its places holds. - Zero days follow the selection: setResupplyStops with an explicit list, and so chooseResupplyOption, drops those of options no longer ticked (carrying through included); absent keeps them. resetResupplyPlan clears the selection and the zero days together. - chooseResupplyOption writes nothing when the group already reads that way, so a lone place chosen under the every-option default keeps the default. - The privacy policy lists the zero days a plan holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Clear choices resets through resetResupplyPlan, so the zero days go with the selection; choosing another place or carrying through drops the left-out places' zero days in the shared editor. - A multi-place group's radio speaks the place's full name with its facts as the hint; the facts and More/Less sit beside the radio, each its own focusable element, instead of inside one Pressable label. - Repeated controls name their place or turn-off: "Fewer/More zero days at Salida", "None - carry through at Monarch Pass (US 50)". - Rows are memoised by value (sameGroupRow, sameLegRow), since plannerRows rebuilds every row object on each edit; a tap now re-renders the changed rows only. - An invalid Plan section: the Resupply row is worked out for the whole trail and says "Whole trail · ...", which is what the screen shows. - The waypoint detail reads the group's choice with resupplyGroupChoice, so a lone place under the every-option default shows as planned with Remove, as the planner does; a place in an undecided group picks. - saveResupplyStops is gone, and useStoredResupplyStops says what it is for now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
74f132e to
09e8ce1
Compare
Update on the review (
|
Review of #236 and #238: - A section that starts at a town no longer walks out of it again: the leg arriving there, in the section before, counted the side trip both ways, so two sections meeting at a town added up to more than the whole trail (0-50 + 50-100 was 109 km against 106). Only a stop at the trail's own start, which no leg arrives at, has its walk out on the leg leaving. - With a camp plan, a leg's arrival comes from that plan alone: its rest nights are already in its dates, and adding the resupply zero days too counted the same rest twice. The day number is now the calendar day the date falls on (walking days plus earlier rest days), so day and date agree. Zero days still count in the summary's total days. - Only a stop decided on one place takes zero days; an undecided one takes none rather than the most any of its places holds. - Zero days follow the selection: setResupplyStops with an explicit list, and so chooseResupplyOption, drops those of options no longer ticked (carrying through included); absent keeps them. resetResupplyPlan clears the selection and the zero days together. - chooseResupplyOption writes nothing when the group already reads that way, so a lone place chosen under the every-option default keeps the default. - The privacy policy lists the zero days a plan holds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The resupply picker was a modal of checkboxes behind a "Choose stops"
link at the foot of the Plan screen, and the legs a card under it; the
owner could not find either. The planner is now a regular stack screen
(guide/[trailId]/resupply, title "Resupply"), laid out like the planning
spreadsheet: a summary on top (summariseResupplyPlan), the hiker's inputs
in one editable line, then start -> leg -> stop -> ... -> end, each leg
just above the stop it arrives at.
- A decision group is a radio list plus "None - carry through"
(chooseResupplyOption); a lone place is a Yes/No switch; a group with
more than one place still ticked says "Not decided yet".
- Each option shows storeLabel, its access or "On trail", accepts boxes
and the description's first sentence, the rest behind More.
- Each leg shows walked km with legSideTripText, rides, climb, days, food
and a long-carry flag. No arrival days, and no camp plan is passed to
computeResupplyLegs, so the plan's rest days and the zero days are
never counted twice.
- A chosen stop has a half-day zero-days stepper (setResupplyZeroDays,
through plans-store.apply), passed to the legs as zeroDays.
- Hours/day, pace, food per day and long-carry days are editable from
the screen (plan-inputs-store, in the existing sheet and cards).
The Plan screen's Resupply section becomes a labelled row near the top
("5 stops · longest 7 days · 2 to decide") that opens the screen with
the section as route params; the waypoint detail's "Plan resupply here"
toggle gains a link to it. ResupplyCard, ResupplySelectList and the
checkbox toggle are removed with their tests.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With every option ticked by default, "Plan resupply here" on a town at a turn-off with several towns added a tick, leaving the group undecided on the resupply screen. It now makes the same decision the screen does (waypointResupplyEditor): planning picks this place in its group (chooseResupplyOption), removing the group's chosen place carries the group through, and removing a tick from a group still undecided takes only that tick out. A lone place stays Yes/No. The edit reads the document when it runs, with the device-local selection as the base only while the document has none. setResupplyStopSelected had no caller left and goes with its tests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Group headers use resupplyGroupName and the rows inside a turn-off resupplyOptionShortName, so "Salida (access: Monarch Pass (US 50))" under "Monarch Pass (US 50)" reads "Salida" (the full name stays the accessibility label). Each leg row carries legEndpointNames, so a leg into a group still undecided ends at "Monarch Pass (US 50) (not decided)" rather than every place joined, and its side trip is "off trail and back" rather than to a list of towns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
groupHeading titles a group of one by the place's short name ("Chama") with
"turn-off at Cumbres Pass (CO 17)" underneath, from the group label; the full
name stays the accessibility label. A decision group is still headed by its
turn-off.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Clear choices resets through resetResupplyPlan, so the zero days go with the selection; choosing another place or carrying through drops the left-out places' zero days in the shared editor. - A multi-place group's radio speaks the place's full name with its facts as the hint; the facts and More/Less sit beside the radio, each its own focusable element, instead of inside one Pressable label. - Repeated controls name their place or turn-off: "Fewer/More zero days at Salida", "None - carry through at Monarch Pass (US 50)". - Rows are memoised by value (sameGroupRow, sameLegRow), since plannerRows rebuilds every row object on each edit; a tap now re-renders the changed rows only. - An invalid Plan section: the Resupply row is worked out for the whole trail and says "Whole trail · ...", which is what the screen shows. - The waypoint detail reads the group's choice with resupplyGroupChoice, so a lone place under the every-option default shows as planned with Remove, as the planner does; a place in an undecided group picks. - saveResupplyStops is gone, and useStoredResupplyStops says what it is for now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
09e8ce1 to
83c6df7
Compare
Stacked on #236 (shared lib). Merge #236 first; GitHub will then retarget this to
main.Tracknotes already had resupply planning, but it was hard to find. It sat at the bottom of the Plan screen, below Stops, and you reached its checkbox modal from there. This PR gives resupply planning its own screen.
Changes
app/guide/[trailId]/resupply.tsxis now a regular stack screen titled "Resupply", not a modal. The deep link still works.ResupplyEntryRow), reading e.g. "5 stops · longest 7 days · 2 to decide".PlanSettingsSheetwith hours/pace and food/long-carry; the values stay inplan-inputs-store.setResupplyZeroDaysthroughplans-store.apply.chooseResupplyOption, replacing any other town chosen there. Removing the chosen place makes the turn-off "carry through".ResupplyCard,ResupplySelectList,resupply-toggle.tsand the unusedtoggleResupplyStop/setResupplyStopSelected.Verified
npm run check:mobile: 171 suites and 2156 tests pass; typecheck clean; lint shows 0 errors and the 3 existing warnings.Not verified on a device
Nobody has run it on a device yet: no emulator or Metro was attached.
🤖 Generated with Claude Code