Skip to content

Rework flight date/time entry: instant storage, picker widget, drop the duplicate FPL parser - #18

Open
roznet wants to merge 4 commits into
mainfrom
claude/datetime-widget-upgrade-review-qdf46d
Open

Rework flight date/time entry: instant storage, picker widget, drop the duplicate FPL parser#18
roznet wants to merge 4 commits into
mainfrom
claude/datetime-widget-upgrade-review-qdf46d

Conversation

@roznet

@roznet roznet commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Closes #17. Depends on roznet/flyfun-common#9 (ZonedWallClock), so the FlyFunCommon package pin needs bumping before this builds.

Three commits, each independently reviewable.

1. Use RZFlight's ICAO flight plan parser (6cc39e9)

Models/ICAOFlightPlanParser.swift reimplemented RZFlight.ICAOFlightPlanParser in 217 lines against the library's 613, with no tests where RZFlight has RZFlightFPLTests. It also returned the weaker representation, a day plus a separate "HH:mm" string, rather than the composed UTC instant RZFlight already builds from field 13 and DOF/.

That forced the caller to add two "HH:mm" strings by hand to get the arrival. addTime() wrapped at 24h without touching the date, so any leg whose EET/ crossed midnight landed a day early. RZFlight derives the arrival from EET/ on the instant, so the bug has nowhere to live.

Both import paths now converge on applyRoute(). The pasted plan and the weather import already carried the same RZFlight.Route, so the duplicated origin/destination/time/aircraft mapping collapses into one.

2. Store departure and arrival as absolute instants (69e0662)

departureDateTime / arrivalDateTime become settable, and setting one writes the day and the UTC time of day together, so an edit crossing midnight moves the day.

departureInstant / arrivalInstant are added as the representation to move to, but are deliberately not authoritative yet, and are currently write-only. The legacy pair stays authoritative and stays dual-written, because CloudKit syncs this store between devices on different app versions and an older build writes only the pair; a release that trusted the instants would silently drop that device's edits. backfillScheduleInstants() runs each launch and re-derives any instant that has drifted, which is how an older device's edit is picked up. A later release flips the precedence, the one after drops the pair.

The one thing worth arguing about in this PR: I originally planned to re-pin departureDate to UTC, and that was wrong. Every existing row was written as device-local midnight by a DatePicker, and older builds still read it that way. Normalising it would have shifted stored dates by a day for every user west of Greenwich and broken what those builds display. It keeps its device-local meaning until the pair is dropped entirely. This is called out in designs/ios-app.md so the next person does not undo it.

Also fixes the inconsistency from #17: the API date is now formatted in UTC from the resolved instant, so it always names the same day as the ..._time_utc sent beside it. Previously departureDateTime read the stored day in UTC while the API mapping read it device-locally, and the two disagreed for any flight near midnight.

Times are range-checked when parsed, because the old widget wrote partial input into the model on every keystroke and values like "1" exist in the store.

3. Replace the text field with a picker (442602c)

FlightDateTimeField gives a date picker plus hour, minute and timezone pickers, matching the weather app's departure picker. It binds to the instant and keeps the zone as view state, deriving everything through ZonedWallClock per render.

AirportTimezoneCache becomes observable instead of callback-based. The old callback was dropped on three paths: the airport not being in the database yet, a geocoder failure, and a resolution already in flight for the same ICAO. Each left the picker empty with no way to recover, and the first fired routinely, because the airport database loads in the background at launch so an early lookup simply missed. Resolution now waits for that load, and every observer sees the result.

Zone selection goes through resolvedTimeZoneId, so the picker can no longer select a zone that has fallen out of its own list and silently fall back to GMT while rendering blank.

Behaviour changes to sign off on

  1. Minutes step in fives rather than being free text. A pilot can no longer file 14:37. I judged 5 the right trade for scanability against a 60-entry menu, but it is a one-line change (minuteStep is a parameter) if you want 1.
  2. A new flight now starts with a departure time (next whole hour UTC) rather than blank, because a picker always shows something. Previously a flight could be created with no time, which silently excluded it from the connecting-leg and return-flight payloads. I think the new behaviour is better, but it does mean a time the pilot did not type can reach a customs form, so it is yours to veto.
  3. createReturnFlight / createNextLeg deliberately still copy only the day and leave the time unset, as before. duplicateFlight copies the schedule verbatim, instants included, so it stays an exact copy rather than acquiring a time the original never had.

Not done here

Nothing is compiled or run. No Swift toolchain in this environment (Linux, no swiftc/xcodebuild), so none of this has been built, and the flyfun-common tests have not been run either. Given this touches a CloudKit-backed schema, please build and exercise the create, edit, paste-FPL and weather-import paths before merging. The Xcode project uses PBXFileSystemSynchronizedRootGroup, so the added and deleted Swift files need no project.pbxproj change.

The timezone data source is unchanged. Moving airport timezones into the bundled airports.db would let AirportTimezoneCache be deleted outright and would populate the picker on first paint with no geocoder at all. euro_aip already has the schema-migration machinery (migrate_schema has been walked three times) and a commented-out TIMEZONE field definition. It is worth doing, but nothing here is blocked on it, and the DST bug that originally motivated it is fixed by commit 3 regardless.

flyfun-weather is untouched. Migrating its DepartureTimeModel onto ZonedWallClock is behaviour-neutral and is the natural follow-up; until then the logic exists in both places.


Generated by Claude Code

Brice Rosenzweig and others added 4 commits September 14, 2026 12:49
…ocal copy

Models/ICAOFlightPlanParser.swift reimplemented RZFlight's parser in 217 lines
against the library's 613, with no tests where RZFlight has RZFlightFPLTests.
It also returned the weaker representation: a calendar day plus a separate
"HH:mm" string, rather than the composed UTC instant RZFlight already builds
from field 13 and DOF/.

That forced the caller to add two "HH:mm" strings by hand to derive the
arrival. addTime() wrapped at 24h without touching the date, so any leg whose
EET crossed midnight landed a day early. RZFlight derives arrival from EET/ on
the instant, so the bug has nowhere to live.

Both import paths now converge on applyRoute(): the pasted flight plan and the
weather-app import already carried the same RZFlight.Route type, so the
duplicated origin/destination/time/aircraft mapping collapses into one.

Behaviour preserved for a plan carrying DOF/ but no usable field 13 time,
where the route yields no instant: the day is still applied so the pilot only
has to fill in the time.

Refs #17

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDSfbp4tjwMcKj9mMtf9ME
Flight stored its schedule as a calendar day plus a separate "HH:mm" UTC
string. That pair cannot express a time edit that crosses midnight, and it
leaves an unanswerable question at every call site: which timezone is the
stored day read in? Two readers already answered it differently.
departureDateTime read the day with a UTC calendar while the API mapping
formatted it with a device-local DateFormatter, so the two disagreed by a day
for any flight stored near midnight.

departureInstant / arrivalInstant are added as the representation to move to,
and departureDateTime / arrivalDateTime become settable. Setting one writes
the day and the UTC time of day together, so an edit crossing midnight now
moves the day instead of wrapping within it.

The legacy pair stays authoritative and stays dual-written. CloudKit syncs
this store between devices that may be on different app versions, and a build
without the instants writes only the pair, so a release that trusted the
instants would silently drop edits made on an older device. The instants are
populated now so a later release can flip the precedence and then drop the
pair. backfillScheduleInstants runs each launch and re-derives any instant
that has drifted, which is how an older device's edit gets picked up.

departureDate keeps its existing meaning, midnight in the device's own
timezone, rather than being re-pinned to UTC. Every existing row was written
that way by a device-timezone DatePicker, and older builds still read it that
way; normalising it would have shifted stored dates by a day for users west of
Greenwich and broken what those builds display.

The API date is now formatted in UTC from the resolved instant, so it always
names the same day as the ..._time_utc sent beside it.

Times are also range-checked when parsed. The widget being replaced wrote
whatever the pilot had typed so far into the model on every keystroke, so
values like "1" exist in the store and must read as "no time entered" rather
than compose into a nonsense moment.

Also drops previousDepartureDate, which was assigned in onAppear and never
read.

Refs #17

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDSfbp4tjwMcKj9mMtf9ME
TimeEntryView was an unlabelled, borderless TextField with an empty
placeholder next to a timezone menu. Nothing indicated where or what to type,
it wrote whatever had been typed so far into the model on every keystroke, and
its conversion was wrong twice over: secondsFromGMT() with no date argument
used today's offset rather than the flight date's, and the result was wrapped
modulo 1440 so a local time on the far side of UTC midnight kept the wrong
day.

FlightDateTimeField replaces it with a date picker plus hour, minute and
timezone pickers, matching the layout of the weather app's departure picker.
It binds to the absolute instant and keeps the display zone as view state,
deriving everything through FlyFunCommon's ZonedWallClock on each render. So
switching zone re-displays the same moment, an edit crossing midnight moves
the day, and DST resolves against the flight's own date.

AirportTimezoneCache becomes observable instead of callback-based. The
callback dropped its completion on three paths: the airport not being in the
database yet, a geocoder failure, and a resolution already in flight for the
same ICAO. Each left the picker with nothing and no way to recover, and the
first fired routinely because the airport database loads in the background at
launch, so an early lookup simply missed. Resolution now waits for that load,
and every observer sees the result because the cache is observed rather than
notified.

Zone selection goes through ZonedWallClock.resolvedTimeZoneId, which keeps the
selection present in the options. The old widget could select a zone that had
fallen out of the list and silently fall back to GMT while rendering blank.

Behaviour change worth calling out: minutes now step in fives and a new flight
starts with a departure time rather than a blank one, because a picker always
shows something. Previously a flight could be created with no time at all,
which silently excluded it from the connecting-leg and return-flight payloads.

Refs #17

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDSfbp4tjwMcKj9mMtf9ME
…ime field

Records why the legacy date + UTC-time pair is still authoritative during the
migration window, and the trap for whoever touches this next: departureDate
must keep its device-local meaning while the pair is live, because re-pinning
it to UTC shifts stored dates by a day for users west of Greenwich and breaks
what older builds display.

Also notes that ICAO flight plan parsing belongs to RZFlight, since the
duplicate this change removed is exactly the mistake to not repeat.

Refs #17

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GDSfbp4tjwMcKj9mMtf9ME
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Reviewed the three commits plus the design-doc update. No bugs found.

Spot-checked the parts most likely to hide a timezone/off-by-one bug:

  • Flight.composeSchedule / localDay(ofUTCDayIn:) round-trip correctly (day read in device-local calendar, time-of-day in UTC, verified the getter/setter pair is a true inverse).
  • alignUTCDay(of:to:) correctly preserves the instant's own UTC time-of-day while moving only the day.
  • autoSyncArrivalDate / the NewFlightFlow departure onChange now key off the composed instant rather than the raw departureDate, so an edit that crosses midnight (via date or time/zone picker) correctly carries the arrival day with it — this closes the exact bug class called out in the PR description.
  • AirportTimezoneCache's move to @Observable/@MainActor with resolve() guarding on pending closes the three dropped-callback paths described; the preload/resolve interplay still matches the pre-existing (already-accepted) sequential-with-delay pattern.
  • API payload date formatting (dateFmt, UTC-pinned) now sourced from the resolved instant everywhere, matching designs/api.md's YYYY-MM-DD contract and fixing the local/UTC mismatch from Rework the flight date/time entry widget: instant storage, shared wall-clock model, drop the duplicate FPL parser #17.
  • designs/ios-app.md accurately reflects the new dual-write/backfill scheme and explicitly documents the device-local-midnight trap for departureDate, which is the right place to pin that down.

No CLAUDE.md exists at the repo root to check against. Nothing else stood out as a logic error, duplication, or architecture deviation. Approving.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rework the flight date/time entry widget: instant storage, shared wall-clock model, drop the duplicate FPL parser

1 participant