Native app: apply session_row frames instead of waiting for the poll - #341
kentdebruin wants to merge 16 commits into
Conversation
The server now expects a sidebar socket to send sessions_subscribe and answers most metadata changes with session_row or session_row_removed (7e7ff4d, 29bbe68). The native app decoded neither and sat on its 5s poll for every archive, rename, model switch and run boundary. ServerEvent decodes session_row (the list projection, same tolerant Session model as the poll), session_row_removed (`id`) and sessions_invalidated; malformed frames stay .ignored. OS1Socket sends sessions_subscribe and decodes row frames off the main actor, matched by a type prefix. PresenceStore subscribes the active account's socket after every hello and on account switch, using the live list's query (?archived=exclude), and forwards that account's frames plus a `subscribed` event to observers. SessionsListViewModel keys pending frames by session, flushes them after 120ms, and merges off the main actor with applyingRowUpdates: the poll's filters (no desk rows, a locally archived row stays hidden, a locally restored one reads as live), pending creates retire when their own row arrives and otherwise stay in front, recency order is kept, and a blocked row resurfaces its hide. A publish that lands mid-merge requeues the frames instead of overwriting it. sessions_invalidated and a reconnect re-read the list. The poll remains as the fallback; OS1_SESSIONS_POLL_SECONDS lengthens it for a build under test. Verified on tella-mac-node: OS1 and OS1Mac build, 70 tests pass (13 new list-update, 3 new decoder). Against os.tella.dev with the poll at 600s, archiving a session over REST dropped its row from the Mac app within 5s and restoring it brought the row back. Co-authored-by: Kent de Bruin <[email protected]>
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · quality 3/5 · risk lowSafe once the P1 below is fixed. A concurrent fallback poll can undo the live update this PR is intended to preserve. 🟢 Risk low · recovery in minutes · secrets or config 1 inline comment below. Reviewed 🔁 1 finding → owning session · fix round 1/6 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![open-session-os-tella-dev[bot]](https://avatars.githubusercontent.com/u/65127121?s=60&v=4){kind=small}
| // The list moved under the merge (a poll landed, a local archive). | ||
| // That publish is newer for everything but these rows, so replay | ||
| // them onto it rather than publishing this stale base over it. | ||
| guard revision == sessionsRevision else { |
There was a problem hiding this comment.
🔴 P1 — A stale poll can overwrite a newer row frame
This revision check protects only the case where a poll publishes while the row merge is detached. The reverse ordering remains unsafe: a poll can receive an old response, then a session_row with a newer title can flush and call setSessions, and finally the poll's detached preparation/grouping can finish and unconditionally publish its old response in refresh(). The live change then disappears for up to the next 5-second poll, or much longer with the documented verification interval. Capture sessionsRevision when the poll starts and re-check it immediately before publishing the poll result; discard or retry the response if another list mutation landed meanwhile.
There was a problem hiding this comment.
Fixed in 7f9660e. refresh() now captures sessionsRevision before its request goes out. Each row flush that publishes while a poll is in flight records its frames (newest per id, tagged with the revision that carries them); the poll replays the frames newer than its start revision over the response (polled, off the main actor), then re-checks the revision right before publishing. If a flush landed during the detached passes it reruns them over the moved list (at most twice), and a response is discarded once a later-started poll has already published. Tests: testAPollReplaysNewerFramesOverItsResponse, testAPollReplayKeepsTheLocalOverlays, testReplayPicksTheFramesAppliedAfterThePollStarted, testAPollWithNothingToReplayIsThePreparedList. OS1Mac test suite (1062) and the OS1 simulator build pass.
A poll's response was built before any row frame applied while the request was out, but refresh() published it unconditionally, so a rename or archive that a frame had already moved past reappeared until the next poll. Capture the list revision when the request goes out, record the frames each flush applies while a poll is in flight, replay the newer ones over the response off the main actor, and re-check the revision before publishing. A flush that lands during the detached passes reruns them over the moved list; a response is discarded once a later-started poll has published. Co-authored-by: Kent de Bruin <[email protected]>
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · quality 3/5 · risk lowSafe once the P1 below is fixed. The new replay logic protects loaded lists, but initial-load frames are still discarded. 🟢 Risk low · recovery in minutes · secrets or config, large diff 1 inline comment below. Reviewed 🔁 1 finding → owning session · fix round 2/6 |
|
|
||
| private func flushRowUpdates() async { | ||
| // A row that arrives before the first list would publish a one-row | ||
| // list; the poll's first answer carries it anyway. |
There was a problem hiding this comment.
🔴 P1 — The stale-poll fix still drops frames during initial loading
The new replay mechanism only sees updates that pass this guard. On launch, the initial sessions request can fetch session X, then another client archives X while workspace loading/grouping is still running. The resulting session_row_removed flushes while hasLoaded is false and is deleted here; the initial response then publishes X because neither sessionsRevision nor appliedRowUpdates records the newer removal. X remains incorrectly visible until the next poll. Preserve and replay frames received while the initial refresh is in flight instead of clearing them.
There was a problem hiding this comment.
Fixed in fa627ae. Before the first list has loaded, flushRowUpdates no longer clears the frames. When a refresh is in flight it bumps sessionsRevision (nothing published, but the list's truth moved) and records them through the same recordAppliedRowUpdates the post-load path uses, so the first response replays them via polled, and a flush that lands mid-pass reruns the detached passes over the recorded frames. With no request out they are still dropped, since the first poll's answer carries them. The record/replay/prune path is the one exercised by the round-1 tests (testAPollReplaysNewerFramesOverItsResponse etc.); OS1Mac suite (1062) and the OS1 simulator build pass.
A frame that flushed before the first list had loaded was dropped, so an archive or rename made elsewhere while the initial request was out could not be replayed over that request's response, and the row it had already moved past was published until the next poll. When a refresh is in flight, record those frames for its replay instead; with no request out, the first poll's answer carries them anyway. Co-authored-by: Kent de Bruin <[email protected]>
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · quality 3/5 · risk highSafe once the P1 below is fixed. The initial-loading race fix still loses frames when the bounded replay attempts are exhausted. 🔴 Risk high · recovery in days · secrets or config, large diff 1 inline comment below. Reviewed 🔁 1 finding → owning session · fix round 3/6 |
| guard sequence > publishedRefreshSequence else { break } | ||
| if snapshotRevision != sessionsRevision { | ||
| if attempt < 2 { continue } | ||
| break |
There was a problem hiding this comment.
🔴 P1 — The retry cap still drops frames during initial loading
The new replay fixes ordinary initial-load races, but only while one of these three attempts reaches a stable revision. With the documented 5.5k-row list, three 120ms-coalesced frame batches can land across the detached filtering/grouping passes. The third revision mismatch takes this break; refresh() then sets hasLoaded = true, and its defer prunes the recorded frames even though no initial list was published. The UI remains empty, or is rebuilt one subsequent row frame at a time, until the next five-second poll. Do not complete the initial load after exhausting this retry budget: keep retrying until a stable initial publication, or retain the replay and immediately start another refresh while hasLoaded remains false.
There was a problem hiding this comment.
Fixed in a0c4717. The rerun budget now applies only once a list is on screen. While hasLoaded is false the first response reruns its detached passes over the moved list, with the recorded frames in the replay, until a pass reaches a stable revision and publishes; it no longer breaks out, so the initial load cannot complete with an empty screen or prune the frames it needed. After the first list the cap of two reruns stays, since a later poll covers a list that will not hold still. OS1Mac suite (1062) and the OS1 simulator build pass.
The poll reran its detached passes at most twice when a row flush published under them, then gave up and left the rest to the next poll. Before the first list that is the wrong trade: giving up marked the load done with an empty screen and pruned the frames it needed, so a 5.5k-row list under a burst of frames came back one row at a time until the next poll. Rerun until the first response lands; keep the cap once a list is on screen. Co-authored-by: Kent de Bruin <[email protected]>
🤖 OS review · request changes · quality 3/5 · risk lowSafe once the P1 below is fixed. The first-load replay issue is addressed, but stale detached attempts can still persist an incorrect hide removal. 🟢 Risk low · recovery in minutes · secrets or config, large diff 1 inline comment below. Reviewed |
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · quality 3/5 · risk lowSafe once the P1 below is fixed. The first-load replay issue is addressed, but stale detached attempts can still persist an incorrect hide removal. 🟢 Risk low · recovery in minutes · secrets or config, large diff 1 inline comment below. Reviewed 🔁 1 finding → owning session · fix round 4/6 |
| // question, and the entry is consumed when it does — so a hide can | ||
| // never swallow work that needs you. Consuming it here (not in the | ||
| // row filter) keeps the mutation out of view body evaluation. | ||
| HideStore.shared.clear(polled.resurfacedHideKeys) |
There was a problem hiding this comment.
🔴 P1 — A stale poll attempt can permanently clear a sidebar hide
polled is computed from a snapshot, but its resurfaced hide keys are cleared before the revision checks at lines 1370-1375. For example, a poll sees hidden session X in needsInput; while the detached pass runs, a newer session_row reports that X no longer needs input and advances sessionsRevision. This line still removes X's hide and HideStore.clear persists that removal to the server, after which the stale list attempt is discarded. Defer this mutation until the attempt's sequence and revision have been accepted. For the no-publish path, validate the revision before clearing and breaking.
There was a problem hiding this comment.
Fixed in 48ee5d9: poll-pass side effects now run only after the connection, refresh sequence, and sessions revision are accepted. The no-publish path performs the same validation before clearing resurfaced hides, and the legacy archive index is deferred with that clear.
The server now expects a sidebar socket to send sessions_subscribe and answers most metadata changes with session_row or session_row_removed (7e7ff4d, 29bbe68). The native app decoded neither and sat on its 5s poll for every archive, rename, model switch and run boundary. ServerEvent decodes session_row (the list projection, same tolerant Session model as the poll), session_row_removed (`id`) and sessions_invalidated; malformed frames stay .ignored. OS1Socket sends sessions_subscribe and decodes row frames off the main actor, matched by a type prefix. PresenceStore subscribes the active account's socket after every hello and on account switch, using the live list's query (?archived=exclude), and forwards that account's frames plus a `subscribed` event to observers. SessionsListViewModel keys pending frames by session, flushes them after 120ms, and merges off the main actor with applyingRowUpdates: the poll's filters (no desk rows, a locally archived row stays hidden, a locally restored one reads as live), pending creates retire when their own row arrives and otherwise stay in front, recency order is kept, and a blocked row resurfaces its hide. A publish that lands mid-merge requeues the frames instead of overwriting it. sessions_invalidated and a reconnect re-read the list. The poll remains as the fallback; OS1_SESSIONS_POLL_SECONDS lengthens it for a build under test. Verified on tella-mac-node: OS1 and OS1Mac build, 70 tests pass (13 new list-update, 3 new decoder). Against os.tella.dev with the poll at 600s, archiving a session over REST dropped its row from the Mac app within 5s and restoring it brought the row back. Co-authored-by: Kent de Bruin <[email protected]>
A poll's response was built before any row frame applied while the request was out, but refresh() published it unconditionally, so a rename or archive that a frame had already moved past reappeared until the next poll. Capture the list revision when the request goes out, record the frames each flush applies while a poll is in flight, replay the newer ones over the response off the main actor, and re-check the revision before publishing. A flush that lands during the detached passes reruns them over the moved list; a response is discarded once a later-started poll has published. Co-authored-by: Kent de Bruin <[email protected]>
A frame that flushed before the first list had loaded was dropped, so an archive or rename made elsewhere while the initial request was out could not be replayed over that request's response, and the row it had already moved past was published until the next poll. When a refresh is in flight, record those frames for its replay instead; with no request out, the first poll's answer carries them anyway. Co-authored-by: Kent de Bruin <[email protected]>
The poll reran its detached passes at most twice when a row flush published under them, then gave up and left the rest to the next poll. Before the first list that is the wrong trade: giving up marked the load done with an empty screen and pruned the frames it needed, so a 5.5k-row list under a burst of frames came back one row at a time until the next poll. Rerun until the first response lands; keep the cap once a list is on screen. Co-authored-by: Kent de Bruin <[email protected]>
…rames' into report-handle-live-session-row-frames Co-authored-by: Kent de Bruin <[email protected]>
…ssion-row-frames Co-authored-by: Kent de Bruin <[email protected]>
Also repair the latest main-line lane assertion so the required repository check type-checks after syncing the PR branch. Co-authored-by: Kent de Bruin <[email protected]>
…ssion-row-frames Co-authored-by: Kent de Bruin <[email protected]>
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · quality 3/5Safe once the P1 below is fixed. The latest change addresses the hide race, but a no-op newer poll still leaves older polls eligible to publish. 1 inline comment below. Reviewed 🔁 1 finding → owning session · fix round 5/6 |
| // The list already says what the response says; the pass | ||
| // was accepted at the current revision, so what it found | ||
| // to consume still holds. | ||
| accept(polled) |
There was a problem hiding this comment.
🔴 P1 — A no-op newer poll does not supersede an older poll
publishedRefreshSequence advances only when a poll assigns the list. For example, while the socket is disconnected, poll 1 can snapshot title Old; the server then restores title New, the socket reconnects, and its .subscribed event starts poll 2. Poll 2 sees New, matching the currently displayed list, so this branch accepts it without advancing the sequence. If delayed poll 1 then completes, it still passes sequence > publishedRefreshSequence and publishes Old until the next fallback poll. Mark every accepted poll, including a no-op, as published before breaking.
| accept(polled) | |
| publishedRefreshSequence = sequence | |
| accept(polled) |
There was a problem hiding this comment.
Fixed in 9c07463: every poll accepted at a stable revision now advances publishedRefreshSequence, even when its rows already match the displayed list, so an older in-flight poll cannot publish afterward.
…ssion-row-frames Co-authored-by: Kent de Bruin <[email protected]>
Resolve the actor-service assertion with the authoritative main-line version. Co-authored-by: Kent de Bruin <[email protected]>
Advance the accepted refresh watermark even when the newer poll matches the displayed list, preventing an older in-flight response from publishing afterward. Co-authored-by: Kent de Bruin <[email protected]>
🕙 Outdated review — superseded by a newer review below🤖 OS review · request changes · quality 3/5Safe once the remaining P1 is fixed. The no-op poll ordering fix addresses the latest finding, but replay still preserves stale hide-clearing decisions. 1 inline comment below. Reviewed 🔁 1 finding → owning session · fix round 6/6 |
| return ( | ||
| applied.sessions, | ||
| prepared.archived, | ||
| Array(Set(prepared.resurfacedHideKeys).union(applied.resurfacedHideKeys)) |
There was a problem hiding this comment.
🔴 P1 — Recompute resurfaced hides after replaying newer rows
The earlier stale-hide finding remains reproducible despite moving side effects behind acceptance. A poll response contains hidden session X with waitingForInput: true; while preparation runs, a newer row with waitingForInput: false lands. The revision check correctly retries, but prepared(all, ...) again marks X's hide for removal from the old response. Replay updates X to its current state, yet this union retains the stale hide key. The accepted pass then calls HideStore.clear, permanently saving the unwanted removal. Compute resurfaced hide keys from the final replayed active list, rather than unioning decisions from the superseded response. Add a regression test covering a blocked poll row replaced by a non-blocked replay row.
There was a problem hiding this comment.
Fixed in f56683e: poll hide removals are recomputed from the final replayed active rows, rather than unioned with decisions from the stale response. Regression coverage checks a blocked response replaced by a non-blocked row or removal, plus other blocked rows that must still resurface the shared workspace. Verified bun run check, all 1,087 OS1Mac tests, and the OS1 simulator build.
Co-authored-by: Kent de Bruin <[email protected]>
🤖 OS review · request changes · quality 3/5Safe once the P1 below is fixed. Recomputing hides after replay fixes the previous example, but no-op frames still bypass replay and can permanently clear a hide. 1 inline comment below. Reviewed 🔁 Still not merge-ready after 6 handed-off fix round(s) — over to humans. (The |
| } | ||
| return | ||
| } | ||
| guard applied.changed, let grouped else { return } |
There was a problem hiding this comment.
🔴 P1 — Keep no-op frames in the in-flight poll replay
The stale-hide fix remains incomplete because this guard discards frames before recording them when they do not change the displayed list. For example, another client restores an archived, blocked session X in hidden workspace ws-1; a poll captures X, then that client archives X again while the poll awaits workspace metadata. The server's 250ms coalescer can send only the removal. Since X is absent from the displayed list, drop reports no change and this guard discards the removal. The poll consequently publishes X and permanently clears workspace:ws-1 through accept, even though X is already archived. The next poll removes X but cannot restore the persisted hide. Record accepted no-op frames too, advancing the replay revision while requests are in flight so detached poll passes retry. Add a regression covering an unknown-row removal arriving during a loaded-list refresh, rather than supplying the replay directly.
1 participant
Brings the native Swift client up to the session-list wire behavior shipped in 7e7ff4d and 29bbe68. The server expects a sidebar socket to send
sessions_subscribeand answers most metadata changes withsession_row/session_row_removed; at HEAD the native app ignored both and waited for its 5s poll.Before / after
sessions_subscribewith?archived=excludeafter every hello and on account switchsession_row.ignoredsession_row_removed.ignoredsessions_invalidated.ignoredChanges
ServerEvent:.sessionRow(Session),.sessionRowRemoved(sessionId:),.sessionsInvalidated; malformed frames stay.ignored.OS1Socket:subscribeSessions(query:); row frames take the detached decode path (type-prefix match).PresenceStore: subscribes the active account's socket after hello and instart()(account switch / foreground), forwards that account's frames plus asubscribedevent to observers.SessionsListViewModel:applyingRowUpdatesis a pure off-main merge that keeps the poll's filters, local archive/restore overlays, optimistic placeholders (retire on own row, otherwise stay in front), recency order and hide resurfacing. A publish landing mid-merge requeues the frames. Poll stays as fallback;OS1_SESSIONS_POLL_SECONDSlengthens it for verification.Verification
xcodegen generate, OS1 (iOS Simulator) and OS1Mac build;ServerEventTests,SessionRowUpdateTests(new, 13),PresenceStoreTests,ArchivedSliceTests,SessionsListLensTests: 70 pass.OS1_SESSIONS_POLL_SECONDS=600: archiving a session over REST removed its row from the Mac app within 5s (1,028 -> 1,027), restoring it brought the row back (-> 1,028). Screenshots in the session.Started by Kent de Bruin in this OS session