Skip to content

Follow-ups from #556: refresh-gate override choke point, move atomicity, and iOS edit-form polish #558

Description

@roznet

Leftovers from #556 (#552 / #544). The two Importants from the final review round
were fixed directly on main (690281f8, 34b8d095); these are what remains.

1. apply_params_change_override is opt-in at six call sites

packs.py (×3 incl. /freshness), agent.py, refresh_resume.py, scheduler.py.
All six are correct today, but two of them only became correct via dedicated
follow-up commits during #556 — the gap reappeared twice, which is the signal
that a new decide_refresh caller will reintroduce it.

The obvious fix doesn't work as stated. Folding the override into
decide_refresh as the default would silently undo the deliberate scheduler
exclusion (scheduler.py, triggered_by != "resume"): an edit is already
followed by a client-driven refresh, so forcing a full run per edited flight in
the routine cycle just buys a second one. So this needs an explicit opt-out
parameter, or a single boundary the client-facing paths pass through and the
scheduler doesn't — a design change, not a mechanical refactor.

2. Move: artifacts are deleted before the transaction commits

flights.py — the subscriber re-insert loop runs between delete_flight
(which _rmtrees pack artifact directories) and db.flush(). If the loop
throws, the DB rolls back and the old flight/pack rows return, but the artifact
directories are already gone — restored rows pointing at missing files. Low
probability (the snapshotted subscriber rows come from valid data), but the
function's docstring frames the move as atomic and this is the one part that
isn't. Pre-existing ordering, newly relevant now that code runs between the two.

3. Test coverage gaps

  • agent.py's _freshness_dict is wired into the override but has no test;
    it mirrors two endpoints that do, so it can drift silently.
  • FixtureBriefingRepository.moveFlight synthesises the moved FlightResponse
    with rawRoute always nil, unlike the real server's three-way rule the fixture
    otherwise mirrors. Latent today; a future raw-route-across-move test would get
    a false pass from the fixture rather than exercising client logic.

4. iOS polish

  • loadPackCount resolves via a .task, so a pilot who edits and taps Save
    before it lands sees the "no briefings" copy in the Move note, understating
    what Move discards. Needs a completion guard, or the note deferring until the
    count is known.
  • applyResidualEdits's post-move PATCH retries on any error including a 422,
    which a second identical attempt can never fix. Costs a round-trip before the
    (accurate) message appears.
  • waypointsText isn't seeded from raw_route, so the route field shows the
    resolved waypoints rather than the pilot's Field-15 string. A real remaining
    parity gap with the web, deliberately deferred from iOS flight edit: Move/Duplicate, dismiss on save, and three server bugs behind it #556 because it changes
    what the pilot sees on every edit.

5. Run /sync-ios-web

FlightDuration.swift is a new hand-copied surface mirroring
web/ts/utils/duration.ts. I verified it by hand (options, ceiling, epsilon,
ceil-to-quarter, label format all match), but the pairing should be registered
with the sync tooling rather than resting on a PR-description note.

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