Repository navigation
Keep up to three graphs resident with right-only page switching - #142
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a9bf82f88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let configured=if List.mem graph_id ids then ids else ids @ [graph_id] in | ||
| Effect.bind (send_manager (Graph_service.Configure_resident_graphs configured)) | ||
| ~f:(fun ()->send_manager (Graph_service.Focus_resident_graph graph_id)))) |
There was a problem hiding this comment.
When a resident graph fails to open, the “Retry local open” button still dispatches Select_graph (journal_manager_view.ml), but this handler now only configures the existing membership and focuses the existing session. Static tracing shows that configure_residents skips already admitted scopes and Visible_graph_changed only restores their state, so neither operation retries mirror inspection or attachment. The button therefore leaves the graph in the same failed state. Distinguish retrying a failed graph from focusing a healthy resident and restart the failed graph's open operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed this remained valid at 6313330. Fixed in 1d61aeb: Application selection now uses the actual Retry_graph_open recovery state to dispatch the existing Select_graph reopen command, while healthy residents retain the focus path. It covers both Local_restore_failed and a current Graph_failed worker with an available local mirror, and closes the picker on retry.
The mounted public-hook tests inspect the real Client_command without the fixture's Focus-to-Select conversion. Core tests verify the renewed lifetime stays admitted and advances to attachment; catalog persistence completion remains accounted for. Final full build/regression passed (1,114 tests plus 87 PASS).
| let session_for_effect core (scope : effect_scope) = | ||
| List.find_opt (fun session -> | ||
| let s=session.saved_identity in | ||
| scope.graph_generation=Some s.graph_generation && scope.account_generation=Some s.account.account_generation) | ||
| (remember_current core).resident_sessions |
There was a problem hiding this comment.
When recovery recreates an earlier resident's scope, static tracing shows that select_graph_transition still increments that session's own generation rather than allocating above resident_next_generation. For example, recovering A at generation 1 while B remains at generation 2 gives both graphs generation 2. This lookup then routes completions using only account and graph generations, so B's runner completion can be delivered to A and discarded without completing B's pending work. Apply the global generation allocator to recovery transitions as well as newly configured residents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed this remained valid at 6313330. Fixed in 1d61aeb: every new graph lifetime allocates above the global resident generation, and both recovery paths re-register the retained session under its fresh scope. Terminal callbacks use exact retained owners/tickets; stale business notifications stay inert. Allocator, session ownership, public Core regressions.
Regressions cover online and authentication recovery, a different graph's in-flight key completion, stale mirror/outbox events, and asset/socket retirement. Final full build and regression passed: 1,114 tests plus 87 PASS checks.
| let state=match event with Sync_event (Sync.Resident_graphs_changed _) -> {state with resident_mode=true} | _->state in | ||
| let known=state.resident_mode in |
There was a problem hiding this comment.
On a cold launch with nonempty saved resident membership, Sync.step restores the sessions internally while handling the catalog-load completion; no Resident_graphs_changed event reaches this worker, so resident_mode remains false. Static tracing shows that translate_sync consequently issues the restored graphs' mirror inspections without loading their graph sessions, and the final restore_visible_graph loads a fresh session with pending=[], losing those inspection tickets. Their completions are then ignored, leaving startup stuck instead of opening the most recently used graph. Enable resident mode before translating the restored sync transition, not only on explicit configuration events.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed this remained valid at 6313330. Fixed in 1d61aeb: translate_sync enables resident mode from the new Sync snapshot before translating any restored graph effects.
The public worker regression restores cached memberships of 1, 2 and 3 graphs from a new worker via catalog completion, without an extra configuration event. It completes inspections/attachments in reverse order, verifies every DB opens, preserves the selected graph and checks scoped reads use the matching DB. Final full build/regression passed (1,114 tests plus 87 PASS). Actual native GUI startup remains a separate pending acceptance check.
| | Visible_graph_changed id when List.mem id (resident_graphs core) -> | ||
| let core=remember_current core in | ||
| let next=restore_visible core (Some id) in | ||
| let next={next with cached_selected_graph=Some id} in |
There was a problem hiding this comment.
When focusing a resident whose saved presentation_generation is lower than the native host's current generation, this restores the older startup scope unchanged. Static tracing shows that JournalApplicationPlatform.request rejects timeline/online requests unless scope >= startupScope, so the host cannot adopt that graph's scope. Native authentication and online-cancellation events subsequently carry the host's newer scope, which apply_authenticated_user and Online_cancelled reject against the focused graph's older scope. Authentication recovery or cancellation therefore stops working in that graph. Keep the account-level native presentation scope monotonic and synchronize it on focus without replacing the retained graph runtime.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed this remained valid at 6313330. Fixed in 1d61aeb: restoring a session keeps the shared native presentation generation monotonic without changing retained graph scopes, keys or DB identity. Account-wide cancellation publishes the final visible state once, so its emitted ticket matches the final reducer state.
Focus/cancellation regressions and publication checks cover retained lifetimes, stale/fresh native tickets and all visible positions with 1/2/3 residents. Final full build/regression passed (1,114 tests plus 87 PASS). This is reducer/native-contract evidence; real Simulator gesture recording is still pending.
Keep up to three graph sessions resident within one account. Sync/DB runtimes, requests, subscriptions, assets, navigation and Capture drafts retain their graph and lifecycle ownership; account authentication and platform services remain shared. Hidden graph updates continue syncing and defer presentation work until focus.
Switch graph contains three ordered resident slots, with explicit replacement at capacity. Closing a slot preserves its local mirror; membership and order survive restart, and signing out retires the account's resident sessions. Timeline uses whole-page, right-only cyclic switching with cancellation and vertical-scroll priority, without an added graph-name bar or pagination UI. Native List offsets restore independently for each graph.
Follow-up fixes cover recovery-generation collisions, cold resident restoration, native presentation ownership, local-open retry, and graph-open publications arriving before membership admission. Focus now requests its explicit graph state and retains a scope-fenced retry across Worker backpressure. The two application ordering/capacity regressions were observed RED before the fixes and GREEN afterward.
Validation:
e25364344136c8c4084835ac39b7fdc5568988be. CI 40829563588 succeeded: 25 successful steps and two intentional cache-save skips.dune build @all app/native_embed.exe.oanddune runtest --forcepassed: 21 Alcotest suites / 1,117 tests, plus 87 other PASS checks (353 sync and 155 application tests). The final test-only navigation correction also passed Swift/XCTest typechecking.Recordings are unedited H.264, 1170×2532, using synthetic data only:
Journal-resident-graphs-01-right-swipe-scroll.mp4libfile_38060c1972ac819188096679dde3707bJournal-resident-graphs-02-slots-replacement-restart.mp4libfile_a5901ded98e48191b0a59b26b7e813efJournal-resident-graphs-03-draft-isolation.mp4libfile_56b21ee370b88191bce4f6d0268d2ac7Journal-resident-graphs-04-saving-switch.mp4libfile_f1ef59fc41b88191a37399b5ff72fd2bJournal-resident-graphs-05-hidden-sync.mp4libfile_bd8874596f94819185e249d483068525Journal-resident-graphs-06-rapid-swipes.mp4libfile_05a17e5d501c81918f174d1bea1005e1Journal-resident-graphs-07-signout.mp4libfile_b7c22f0b5d388191b9a6c08e081ef433All seven recordings are saved in the user's ChatGPT Library (228,383,362 bytes total). Every create result succeeded, with the returned filename and byte count matching its local recording; Library identity metadata was persisted on all seven local files. Local video frames were inspected. Library's MP4 read returns file metadata rather than video frames, so no remote frame-by-frame or byte-download comparison is claimed.
Native evidence uses production UI/reducers through the public test dependency-injection entrypoint, memory-only synthetic secrets and isolated local graph copies. The Saving test adds an eight-second fixture-only persistence delay; it proves the gesture starts during Saving and the eventual focus/save ownership, not that focus commits before persistence finishes. Native production source is
13f7da110ee3abb32eeb457b36cb1bb1398772b9; the final commit changes only the reusable acceptance script, with identical production bytes. Executable SHA256:cb8d2fd3035ecd000797c4f8ebe08c5cc8596c3eba47da55cf39414a4b4bcc0b. Host-restamped Simulator builds remain experimental (cross_abi_validated=false); this is not real-device ABI or phone-installation acceptance.Formal non-Draft PR as requested. Not approved, merged or released.