Make Python the canonical browser projector - #87
Conversation
…er-projection # Conflicts: # plugins/leaf/skills/leaf/scripts/leaf/http.py # plugins/leaf/skills/leaf/scripts/leaf/projection.py
# Conflicts: # plugins/leaf/skills/leaf/assets/leaf.js
leaf-agent
left a comment
There was a problem hiding this comment.
Moving the fold into Python reads well — browser_state keyed on (revision, through_seq), the JS side reduced to a DOM adapter, and /api/view projecting an older base at the sequence the DOM has already applied. The docs are updated in step, and the three new server tests pin the parts that matter (same snapshot as the accepted POST, refused future boundary, only the shown and next revisions projected). Three things I'd want your call on before this lands.
The static site loses reload persistence, and I don't think the change requires it. sessionStorage held the raw event log — the authority — not a projection. The thing this PR set out to de-duplicate is the projector, and docs/session.js reimplements that anyway in demoProjection/demoThreads/demoAsks. Since state() calls demoBrowser() fresh on every read, seeding events from the mirror would flow through the new path unchanged. What it costs is the one place a reader can see Leaf's headline claim without a checkout: test_a_decision_holds_across_a_reload — "the log is the state here as much as anywhere" — is now test_a_static_demo_decision_resets_on_reload asserting the opposite, and the module header's "a board that takes a drag and holds it across a reload" is edited out. If there's a reason the seed can't come back, it'd be worth saying so in the comment; as written the rationale ("browser storage would turn this into a second implementation of Leaf's persistence and version rules") describes the projector rather than the mirror.
Relatedly, that file's header still says the process "answers three paths". There are four now, and /api/view falls through the shim to realFetch, so a static host 404s it. Unreachable today because scripts/site.py publishes only versions/v1.html per example, but the first example that publishes a second version gets "Couldn't load vN" on the Δ control instead of a comparison.
canUndoAction dropped a branch its own comment still describes — see the inline note.
/api/view is the one route outside the _answer fault boundary — also inline.
Nothing on the Python/JS agreement itself: coverage reconstructing terminal entries for events state_projection skips, the within={}/floors={} conversation scope standing in for the old chrome ? [] : retractedIds(...), and unanswered_awaiting feeding answeredContext all line up with what the JS was computing.
…er-projection # Conflicts: # plugins/leaf/skills/leaf/CLAUDE.md
|
Addressed the review on the current head:
Local evidence on the final merged tree is
|
|
Both inline findings are closed, and the server-side undo fact is the stronger version of what I asked for — the browser gets the answer rather than the log to re-fold it from. I checked The static exhibit's visit scope reads as settled — you've named it a user-approved boundary, and the reload-reset test now states it as the claim rather than leaving it as an absence, so I won't press it further. Local evidence on |
leaf-agent
left a comment
There was a problem hiding this comment.
The restored prior-winner route reads right: _restores_desired's three branches are exactly "the projection without this event still has that coordinate" — another writer already holds it, a live report stands under it, or an earlier surviving action does — which is what the removed stateProjection(revision, e.id).desired.has(coordinate) computed, and the docs/session.js mirror agrees with it. The /api/view SystemExit conversion and the two new server tests pin the parts that were open.
One thing the same commit changed in passing, inline: the undo_error filter moved off the published page's containment onto the view's.
Move page-seated message and thread reconciliation into its own owner while preserving panel composition and shared reply wiring.
Move the modifier-aim and design-mode corpus into its own test module, retaining all collection and fixture behavior and updating the focused Linux example.
Move browser-gate, render-check, installed-command, and shot journeys into their own test module while retaining collection and fixture behavior.
…er-projection # Conflicts: # plugins/leaf/skills/leaf/assets/runtime/conversation/model.js
leaf-agent
left a comment
There was a problem hiding this comment.
I approved this on 37a670d while ci / test was still running; it has since gone red, so I'm dismissing that approval. One failure, and I don't think it's a flake.
tests/test_render_startup.py::test_an_export_carries_runtime_data_as_a_labelled_snapshot times out at exporting.py's wait_for_probe(page, "dataApplied", ...) — line 77, not the logApplied wait below it. It failed the same way on 04990ad before the merge (run 33151667045) and on 37a670d after it (run 33152724973), while main is green on ca25e69, 410b565 and bb51604. Two runs, same assertion, main clean — that reads as the branch rather than the "ran before the page was ready" class, and a 30s timeout against the test's deliberate 750ms settle and 0.5s page_state delay is a wait that never arrives rather than one that arrives late.
The mechanism is that a throw in applyState now leaves data-lf-data-revision unstamped forever. notifyDataSubscribers is the only writer of that attribute, and after this PR nothing reaches it once an application faults:
applyStatecallsacceptData(state.data)first, thennotifyChangedData()only on its early returns or at the end ofapply(). A throw in between skips the stamp.- The
catchthat restorespriorEvents/priorBrowser/priorViewends inthrow error, so the fault propagates tostate-feed.js, which setsreadAnswered = falseand callsreportPageError. tick()— the one unconditionalnotifyDataSubscribers()— is reachable only fromreadNothing(), which fires when a read brought no state, not when a read succeeded and its application threw.- The
setIntervalheartbeat is gated onreadAnswered, which that same failure path just cleared.
So every later poll re-reads, throws identically, and the probe can only run out the clock.
This PR adds two throws into exactly that window, both in state-application.js: the Number.isInteger(eventSeq) guard ("state browser must name its log sequence") and, inside apply(), the runtime.view basis check ("state browser has no matching revision view"). Either one firing produces the observed timeout. I couldn't tell which from CI — the run captures no browser console, so reportPageError's text isn't in the log — and that's the piece worth having before choosing a fix: a repro that prints the page error would say whether the views map is genuinely missing the shown revision on this export path, or whether through_seq is absent on some response.
Worth deciding as part of that: whether a state the page refuses should be able to strand the data stamp at all. The comment above acceptData says events and data are "independent authorities" and that data is accepted before the event gate precisely so an older event tail can't hold back newer data — but the new guards are event-shaped and now gate the data stamp too, which is the coupling that comment sets out to avoid.
Happy to open a fix PR if you'd like, though the choice between "stamp data before the guards can fire" and "make the guards not reachable here" looks like yours to make rather than mine to guess.
What I checked on the rest of the range
Everything since my last review on 02ea9a6 reads clean:
d1a7afbcloses theundo_errorcontainment thread the right way._browser_undo_candidatesnow takeswithin(the active document) anddocument_within(the view's) separately,browser_statepassesactive_within, within, floors, andactive_within = enclosing_of(active_spk)offdocuments[active_revision]agrees with the door'sactive_enclosing, which isenclosing_ids(latest_revision(...))—enclosing_ofandenclosing_idsare documented as the same containment reading. The new test stubsundo_errorand asserts it saw only the active map.663c982is a byte-identical extraction: I diffed the moved block against theconversationMessageNode/conversationThreadNode/renderConversationstext inreconcile.jsat02ea9a6and it matches exactly. All fourteen injected names are bound before line 282 (setChildrenandshowThreadare hoisted declarations; the rest come from thecreateThreadModel/createConversationMessages/createRepliesdestructures above), and the factory shape matchesmessages.js/placement.js/work-lines.js.22e467fand04990adare faithful splits: identicaldefsets across the three files before and after, differing only in per-file imports and docstrings, no unused imports, all three carrypytestmark = pytest.mark.nightly, and thescripts/linux-suite.shand_typos.tomlpointers were both updated to names that exist.- The
37a670dmerge resolves one conflict, inconversation/model.js, and resolves it correctly — it takes main's sharedsameAnchorfromanchors.jsover this branch's localJSON.stringify(left, Object.keys(left ?? {}).sort())copy, which is the asymmetry main's version was written to fix. No hand-written content: the combined diff has no++lines and no lines dropped from both parents.
CI failed — tests/test_render_startup.py::test_an_export_carries_runtime_data_as_a_labelled_snapshot times out on the dataApplied probe; analysis in the follow-up review.
|
The export failure is fixed in This PR changed The formerly failing test now passes locally, as do all 41 startup cases and the broadened 225-case startup/server/undo/reaction/conversation/static slice on the final merged tree. Pre-commit also passes after formatting the latest
|
leaf-agent
left a comment
There was a problem hiding this comment.
Your route-level diagnosis is right and mine was wrong — the throw was on the Python side of the seam, not in state-application.js. GET /api/state reaches self.page_state(revision) with a positional argument on every read, so the one-parameter stub raised before any state left the server; the guards I pointed at were never reached. Confirmed on CI as well as locally: test_an_export_carries_runtime_data_as_a_labelled_snapshot PASSED on 2d1ad9f in run 33155162347. I checked the other page_state seam (test_interact_server.py's own_state) — it already carries and forwards view_revision, so this was the only stale one.
The remaining ci / test red is not this PR's. The run above is 1 failed, 1374 passed, and the failure is tests/test_render_navigation.py::test_a_label_press_keeps_the_controls_keyboard_standing — a file this PR doesn't touch. It fails on main independently, on four consecutive commits since db1100e4 and green on bb516048 before it, and #88 already fixes it. This PR inherits it through its base.
2d1ad9f is the same fix as open PR #89 — the same one-line isort reorder in validation.py, down to the blob hashes (024e849..03441fd). Worth picking one: #89 is the dedicated fix and unblocks lint on main now, where this PR carries it incidentally and will land later. If #89 goes first, the commit here becomes a no-op on merge rather than a conflict, so nothing breaks either way — it's just two PRs claiming one change.
I'm not approving only because ci / test is terminally red on this head. Both remaining reds have their own PRs open and neither is yours; once #88 lands and this branch picks it up, I'd expect green.
|
One thing my review of this head didn't cover, because it isn't yours: the server named this fault in every single response, and nothing in the browser reads it. That's why it cost a route-level diagnostic instead of showing up in the first failure.
So the one stale signature turned into a 30-second This predates the branch — that |
Carry the one-line stabilization from #88 so the inherited navigation test observes the open panel after its transition completes.
…er-projection # Conflicts: # plugins/leaf/skills/leaf/scripts/leaf/validation.py
Leaf previously reconstructed durable page state in both Python and browser JavaScript, which let winner, retraction, ask, conversation, update, and undo rules drift. This change makes Python produce the browser-ready projection for an exact document revision and log sequence. JavaScript consumes that projection, maps it to the DOM, and overlays only unresolved local gestures.
/api/statereturns the shown and active revision views, accepted event responses retain the revision shown by the tab, and/api/viewprojects a historical comparison at the sequence the DOM has applied. The static documentation host keeps a small in-memory emulator because it has no Python process; that path resets on reload and is not used by served Leaf pages. Its shim now answers/api/viewas well as the other runtime routes.Local evidence on the current head:
pre-commit run --all-files; 225 startup, server, undo, reaction, conversation, and static-site cases. Before the finalmainrefreshes, the full branch passed 661 tests with 1 skip.