feat(flights): a section selector bar for the flight editor - #23
Conversation
A flight is one long Form — route, schedule, details, crew, passengers, the forms for each airport, actions — and reaching the forms means scrolling past everything else. Compact width now carries the same scroll-spy bar the flyfun-weather briefing uses: it highlights the section nearest the top and jumps to one on tap. The weather app's `ScrollSpyScroll` could not be reused as-is. It is built on `.scrollTargetLayout()` / `ScrollPosition` / `onScrollTargetVisibilityChange`, which are ScrollView-only, and this view is a `List`-backed `Form`. So the jump goes through `ScrollViewReader` (which addresses rows a lazy List has not materialised yet) and the highlight through `onScrollVisibilityChange`, a per-view modifier that does fire inside a List. Anchors sit on each section's header, so a jump lands with the title in view; the four collapsible sections expand before they are scrolled to. `hasForms` is now the single predicate for whether an airport's form section renders, read by both the section and the nav, so a pill can never point at a section that is not there. Wide layout and macOS are untouched — they already split the flight across two columns, and they need their own pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code ReviewBug:
private func switchToFlight(_ newFlight: Flight) {
// Reset cached state for the new flight
formDetails = [:]
notifications = [:]
extraFieldValues = [:]
flight = newFlight
}Since these three actions reuse the same Everything else — the One thing worth double-checking on a simulator/device per the PR's own note: |
… events Review caught that `switchToFlight` — Create Return Flight, Next Leg, Duplicate — swaps the flight under the same `FlightEditView` instance and resets every other per-flight cache, but not the set of seen section ids, so the pill bar could highlight a section from the previous flight. Clearing the set alongside the others does not actually fix it: `onScrollVisibilityChange` only fires on a *change*, so headers already on screen would never re-report and the bar would sit on "Route" until the next scroll. The set was event truth, and events cannot be replayed. Replaced with `FlightSectionSpy`, which holds each header's offset from the top of the `Form` and picks the deepest one that has reached the top edge. Offsets describe where headers actually are, so a flight swap needs no reset at all. It also fixes the second thing review flagged: a "topmost visible header" rule moves the highlight to the next section as its header creeps in from the bottom, whereas holding the last header that passed the top keeps it on a section taller than the screen. Offsets are measured against the form's own named coordinate space rather than `.scrollView`, so this does not rest on that space resolving inside a `List`, and `onDisappear` drops headers the list recycles. Only `active` is observable — offsets churn every scroll frame and must not re-evaluate the view. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed in 02f6154. The Both findings share that root cause, so the mechanism is replaced rather than patched.
Two implementation notes: offsets are measured against the form's own named coordinate space, not
Build green, 95/95 tests passing. Live behaviour still pending a simulator run. |
Code ReviewReviewed One issue found: Unlocalized pill labels for the form-anchor pills ( anchors.append((id: formAnchorID(direction: "arrival"), title: "\(flight.destinationICAO) arr"))
...
anchors.append((id: formAnchorID(direction: "departure"), title: "\(flight.originICAO) dep"))The other four pill labels (Route/Schedule/Details/Crew/Passengers/Actions) are built with No other high-confidence issues — logic for tracking gating ( |
Tapping a pill did nothing and the highlight named the wrong section — at the top it said Schedule, then held there until Passengers came into view and flipped to Details. One cause behind both. The anchors were on the `Section` headers, and in a `List` a header is not a row: `ScrollViewProxy.scrollTo` cannot address it, and it does not take part in row geometry. On top of that, the offsets were measured in a `.named(_:)` coordinate space declared on the `Form`, which does not resolve from inside a `List`'s row hosts — so every anchor reported the same number, "deepest" became whichever entry the dictionary happened to yield, and the answer only moved when rows materialised. Each section now opens with a zero-height `FlightSectionAnchor` row. Rows are real scroll targets, so the jump works, and the anchor sits exactly at the section boundary, so the highlight means what it says. Offsets are measured in `.global`, which resolves everywhere. Anchor positions are also paired with the scroll offset they were taken at and corrected by how far the list has scrolled since. Scrolling moves rows 1:1 with the content offset, so this stays exact even if a row's geometry callback does not re-fire mid-scroll; the offset comes from `onScrollGeometryChange` on the form, a container-level modifier that does report continuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code ReviewWent through the scroll-spy implementation ( Two minor, non-blocking points:
Nothing here blocks merging. |
…to one Taps never landed and the highlight named the wrong section, and the zero-height anchor rows drew a stray separator at the top of every card. The pattern was ported onto the wrong substrate. flyfun-weather's scroll-spy rail rests on `.scrollTargetLayout()`, `ScrollPosition` and `onScrollTargetVisibilityChange` — all `ScrollView`-only — because the briefing content is a `ScrollView`. The flight editor is a `List`-backed `Form`, which offers neither an addressable scroll target nor a usable position signal: section headers are not rows, a `.named(_:)` coordinate space declared on the `Form` does not resolve inside row hosts, and a row cannot be zero-height. Each workaround bought a new artifact. So this ports the rail's *other* mode instead. The bar now selects: a pill renders that section alone, "All" restores the full list. The pill is the state, so there is nothing to keep in sync and nothing to measure — no `ScrollViewReader`, no geometry, no anchor rows. The `Form` body is byte for byte what it was before this branch. Picking a section also expands it, since a lone collapsed DisclosureGroup would be a title and nothing else. A selection naming a section the flight does not have falls back to showing everything, because `switchToFlight` keeps the same view instance. Wide layout and macOS are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed the diff ( No bugs found. The core logic holds up:
Two minor, non-blocking observations:
Approving — nothing here blocks the merge. |
Focusing a section late in the bar scrolled the bar along with it, so "All" ended up several swipes off to the left — the way back was the hardest thing to reach. "All" now sits outside the horizontal scroller, pinned at the leading edge behind a divider, so it never scrolls away. Tapping the section you are already in also returns to the full list, so there are two routes back and neither depends on where the bar happens to be scrolled. Returning to the full list parked on the section you were reading would be nicer still, but that needs a scroll-to-section, which is exactly what `Form`/`List` would not give us and why this is a selector at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
roznet
changed the title
Code ReviewReviewed the diff against 1. PR description no longer matches the implementation. The description says the jump is done via 2. Minor: section ids ( 🤖 Generated with Claude Code |
The doc keeps an explicit `Views/` file tree and feature status list, and every other view file is in it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Code ReviewReviewed the diff ( No high-confidence issues found:
Approving. |
1 participant
Compact width gets a pill bar above the flight
Form. Tap a section and it shows alone; All restores the full list.All · Route · Schedule · Details · Crew · Passengers ·
<ICAO>arr ·<ICAO>dep · ActionsWhy a selector and not a scroll-spy
The first cut tried to port flyfun-weather's briefing scroll-spy (
SectionSpyBar/ScrollSpyScroll) and failed three times. That rail rests on.scrollTargetLayout(),ScrollPositionandonScrollTargetVisibilityChange— all ScrollView-only — because the briefing content is aScrollView.FlightEditViewis aList-backedForm, which gives neither an addressable scroll target nor a usable position signal:ScrollViewProxy.scrollTocan't address them and they take no part in row geometry, so taps were dead and the highlight frozen..named(_:)coordinate space declared on theFormdoesn't resolve insideList's separately-hosted row contexts — every anchor reported the same number, so "topmost" became whichever entry the dictionary yielded.Listrow can't be zero-height — injected anchor rows drew a stray separator at the top of every card.So this ports the weather web rail's other mode instead: focus mode (
enterFocus/exitFocusinsidebar-layout.ts). The pill is the state, so there's nothing to keep in sync and nothing to measure — noScrollViewReader, no geometry, no anchor rows. TheFormbody is byte-for-byte what it was before this branch.Details
hasForms(airport:direction:)is the single predicate shared byformIDs(the pills) andformSection(the render), so a pill can never select an empty section.DisclosureGroupwould be a title and nothing else.effectiveSelectionfalls back to All when the selection names a section this flight doesn't have:switchToFlightreuses the same view instance, so the previous flight's arrival-forms pill can survive the swap.Returning to the full list parked on the section you were reading would be nicer, but needs scroll-to-section — the capability
Form/Listwouldn't give us, and the reason this is a selector at all.Scope
Wide layout and macOS untouched —
shows(_:)is a no-op atsizeClass == .regular. They already split the flight across two columns and need their own pass.Verification
xcodebuild build— greenxcodebuild test— 95 tests, 95 passed, 0 failed (count read back from the result bundle)"All", with de/es/fr; the other six labels already existed inLocalizable.xcstringsDeferred
navSections/expandSection/compactLayout; worth an enum if this grows.designs/ios-app.mddoesn't listFlightSectionNav.swiftyet.🤖 Generated with Claude Code