Skip to content

Fix phone Back after a closed reader, and open desktop chapters with the reader - #507

Merged
Deicyde merged 8 commits into
mainfrom
fix/442-phone-back-and-reader-first
Oct 31, 2026
Merged

Deicyde merged 8 commits into
mainfrom
fix/442-phone-back-and-reader-first

Conversation

@Deicyde

@Deicyde Deicyde commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #442. It merged with two regressions against its last reviewed head, e2a7f39. A readiness check found them, and they were confirmed on main afterwards (#442 comment). Both are still on main 91490d7, after #444 (run 40986902670).

Problem

  • Phone Back fails.
    • Steps, at 720 px or narrower: open a result, close the reader, open a result whose map shares no card with the first, then press Back.
    • What happens: the explorer shows "Could not open this view" and loses the saved Back/Forward state.
    • Why: setSheet re-centres the selection before refresh(true) recomputes the active cards. So geometry() frames the previous route's cards, both sceneFrame() calls return null, and the comparison throws.
  • Desktop chapters open map-first.
    • Since 8a4780f, two kinds of chapter open with the reader collapsed to its 48 px handle and zoom-out stopped at 0.9: chapters with more than 12 statements and at least one without a local link, and chapters with three linked outside topics.
    • That is most real chapters: 4 of 5 in zeta(7), and 7 of 7 in Hilbert-Smith.
    • 2c3c6b8 changed the two tests that caught this, so that they click the handle first.

Change

  • setSheet calls calculateActive() before it centres the selection on a closing phone reader.
  • A new ui$coarseOverview flag is true only for authored-area overviews and for scopes over 120 nodes.
    • It decides the desktop reader default and the bp-dag-scope-overview class that collapses the reader.
    • It decides the reader state after a selection is cleared or hidden by the status filter.
    • It decides the handle's collapsed state and its × label, and the 0.9 zoom-out floor.
    • ui$scopeOverview still drives group navigation and phone map-first.
    • This follows Jack's decision: desktop chapter pages open with the reader showing the chapter's text, and map-first stays for real overviews.
  • Bundle budget: 144,000 to 145,000 bytes, approved by Jack for this fix. Main's bundle was 143,993 bytes when this PR opened, and this PR's was 144,080. After Remove size caps on published data #512 it is 143,570.
    • All five places that pin the limit are raised: scripts/build-explorer.mjs, dag_viewer.viewer_script, two in tests/test_explorer_asset_bundle.py, and tests/test_graph_view_rendering.py.
    • So are the "144 KB" mentions in CHANGELOG.md, CONTRIBUTING.md and docs/explorer-design.md.
  • geometry(), fit(), sceneFrame and the layout are untouched.
  • No command, flag, JSON key or schema changes.

Tests

  • New tests/test_integrated_phone_back_browser.py (WebKit, 390×844):
    • picks two results from dependencies/index.json whose maps share no card;
    • opens the second from search, and by hash;
    • after Back, checks that no error shows, the first result's reader is still closed, and the history snapshot is not null.
  • New tests/test_integrated_reader_first_browser.py (WebKit, 1440×900): a hub chapter with three linked chapters, and a 14-statement chapter with one unlinked statement. Each must open with:
    • a reader at least 340 px wide, with the text visible;
    • no overview class;
    • zoom-out going below 0.9.
  • Restored the two assertions in tests/test_dag_viewer_browser.py that 2c3c6b8 loosened.
  • Both new files run in the WebKit job and in make test-browser.
  • The tests catch the bugs.
    • On main's explorer code, all six targeted tests fail, each with its regression's symptom (run 40995680060).
    • With one fix removed at a time, only that fix's tests fail: phone, reader.
  • Full suite at 0b450e9 on scratch CI: every job passes, with WebKit at 207 passed (run 40999985907).

Notes

  • Focus after a map-card or breadcrumb click. On a shelf or context chapter, the reader now opens, but keyboard focus goes to the map's first card, because mapRequested still reads ui$scopeOverview. At e2a7f39 focus went to the reading heading.
  • Untested halves of the flag. No test covers the oversized-scope half of ui$coarseOverview, or the 0.9 floor on authored-area overviews. tests/test_scoped_overview_browser.py covers map-first for authored areas.

Landing

Review follow-ups not in this PR

The review at b422ab6 found nothing blocking. Its notes are left out to keep this fix small:

  • Navigation-only pages (Jack's call). 39 Atlas pages with no prose and no statements now open a reader that only lists child links. One of them, Data Structures, is left with just its title. Treating such pages as overviews would keep them map-first.
  • Keyboard focus. After a map-card or breadcrumb arrival on a desktop shelf or context chapter, focus goes to the map. Changing mapRequested to read (sheetLayout() ? ui$scopeOverview : ui$coarseOverview) would focus the reading heading.
  • Oversized scope with no local links. It stays map-first with no cards. Adding && overview.nodes.length to the flag would open its reader.
  • Untested map-first half. No test covers scopes over 120 nodes, or the 0.9 floor on authored-area overviews.
  • Phone Back test precondition. The test should confirm in the browser that its two maps share no card.

Posted by PR swarm: 320-329 Reviewer

…selection

On a phone, applyRoute calls setSheet before refresh(true). When Back
returns to a result whose reader was closed, setSheet closes the reader
and centres the selection while ui$active still holds the previous
route's cards. geometry() then frames cards that are not on the new map,
both sceneFrame() calls return null, and the explorer shows "Could not
open this view" and drops the history snapshot. setSheet now calls
calculateActive() before centerIfHidden(), so it frames the current
projection's cards; geometry() is unchanged.

The new WebKit test opens a result at 390 px, closes its reader, opens a
result whose map shares no card with it (from search and by hash), and
checks that Back keeps the reader closed, shows no error and keeps a
history snapshot. It runs in the WebKit job.

The fix takes the bundle to 144,000 bytes, so the explorer budget in the
build script, publication and tests rises from 144,000 to 145,000 bytes.
Jack approved this raise for this fix on 2026-10-29.
…rviews

Since 8a4780f, updateProjection sets ui$scopeOverview for native
shelves and boundary context cards as well as for authored-area
overviews, so on desktop most chapters opened with only the 48 px
reading handle, and zoom-out stopped at 0.9. A new ui$coarseOverview
flag is true only for authored-area overviews and for scopes coarsened
because they exceed 120 nodes. It now drives the desktop reader choice
in applyRoute, the bp-dag-scope-overview class, clearSelection,
syncSheetAccessibility, onStatusChange and the zoom-out floor in
zoomAt. ui$scopeOverview still drives group navigation and phone
map-first.

The two assertions that 2c3c6b8 loosened in test_dag_viewer_browser.py
are back in their e2a7f39 form: the reader is at least 340 px wide on
load, and a map-card click leaves it open, without clicking the handle.
The Node harnesses declare the new variable. A new WebKit test opens a
hub chapter with three linked chapters and a 14-statement chapter with
one unlinked statement at 1440x900, and checks the reader width, the
chapter text, the missing overview class and zoom-out below 0.9. It
runs in the WebKit job.
One Fixed entry under Unreleased covers the desktop reader default and the
phone Back failure. The Explorer delivery entry now names the 145 KB
budget that the phone fix raised.
CONTRIBUTING.md and docs/explorer-design.md still said the explorer
bundle stays below 144 KB. The phone fix raised the enforced limit to
145,000 bytes and the bundle is now 144,080 bytes, so both lines now
give the 145 KB budget that the changelog and the checks use.
The phone Back and reader-first WebKit tests were added only to the
WebKit job in tests.yml, so make test-browser, which CONTRIBUTING.md
says runs the browser tests as CI does, skipped both. The Makefile now
lists the same nine files as the WebKit job.
@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Oct 30, 2026
@Deicyde

Deicyde commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Review at b422ab6: nothing blocks merge.
This review was run by the session that opened #507, at Jack's request; the checks were done by separate review agents that did not write the change.

Blockers: none. One finding was filed as a blocker and downgraded to a note after a second agent re-checked it (note 1). Separately, main has moved and GitHub now reports a conflict; the last paragraph says what the rebase needs.

Notes. Line numbers are at b422ab6. blueprint-dag.js and blueprint-navigation.js are in autoform_cli/assets/.

  1. Atlas "Data Structures" now opens on desktop with a reader that has nothing to read. blueprint-dag.js:1671 (the default is decided at 1539-1540; declutterReader is called at 1750 and defined at blueprint-navigation.js:642). On the Atlas wiki preview at eceb8bfd, roadmap/ComputerScience/CSLibExt/lean/data-structures/ has a shelf of 29 unlinked topics, no authored areas and under 120 nodes. So ui$coarseOverview is false and the reader opens. Its README is a title plus three h2 sections that only list its 60 child topics. The Topics outline (113 entries, limit 120) represents all 60, so declutterReader hides all three sections. The reader keeps only the title, the previous/next links and a collapsed list of other topics. Main opened this page map-first with the 0.9 floor. The second agent re-rendered the subtree at the head and confirmed all of this, but downgraded it for three reasons. The page is neither an authored-area overview nor an oversized scope, so the head follows Jack's decision. e2a7f39 opened the same reader. Main shows it after a Topics-outline click. The first agent's sweep found no other empty reader among the 409 pages whose default changed. The current wiki head, 618fdc90, has the same README (now under .../part-01/lean/part-01/data-structures). Its 42.1 MB index could not load before Keep large libraries' search index loadable #508, which is now on main, so the rebased head will show this page on the current wiki too. Fix, if Jack wants such pages map-first: keep a flag from navigation.readerIsOverview(dom$readingBody) (blueprint-navigation.js:710), computed after renderNavigation(), and include it where updateProjection sets ui$coarseOverview and the class. Or have declutterReader keep the lists when hiding them would leave only the title. Either way, add a WebKit case: a topic whose README is only h2 child-index lists, plus one unlinked statement.

  2. Keyboard focus still goes to the map on desktop chapters that now open with the reader. blueprint-dag.js:1537 (focus is set at 1558-1559). mapRequested still reads ui$scopeOverview. Reach a desktop shelf or context chapter with mapFirst, either through a breadcrumb other than Home (1958) or through activateNode on a scope record (3100). The second covers map cards, boundary cards, search results, shelf links and inventory rows. The reader opens, but focus goes to the first map card. A shelves-only chapter draws no cards, so focus is dropped: it stays on body, or on the breadcrumb nav after a crumb. Main focuses the same targets, so this is not a regression against main. At e2a7f39 these chapters focused the reading heading. The PR body lists it. Fix, at 1537: replace ui$scopeOverview with (sheetLayout() ? ui$scopeOverview : ui$coarseOverview). A Node probe of the patched applyRoute gave these results:

    • Shelf, context and shelves-only chapters focus the reading h1.
    • Coarse overviews and topic-card chapters keep map focus.
    • Phones and fresh loads are unchanged.

    The patch was not run in WebKit. Add a WebKit assertion to tests/test_integrated_reader_first_browser.py that a breadcrumb or map-card arrival at hub and shelf focuses .bp-reading-body h1.

  3. A chapter over 120 nodes with no local links stays map-first with no map. blueprint-dag.js:1671. The originalNodes > MAX_MAP_NODES clause sets the flag even when structuralScopeProjection draws no cards. The page then shows only the "Not on the map" shelves, the chapter text collapses to the 48 px handle, and a mapFirst arrival has no card to focus. The 1,000-statement "flat" fixture of test_oversized_flat_scope_defaults_to_bounded_native_shelves is such a page. Main does the same, and it fits the literal "oversized-scope coarsening" wording. At e2a7f39 this chapter opened with the reader. Fix, if Jack wants the reader there: ui$coarseOverview = Boolean(overview && (!overview._scopeOverview.$structural || overview._scopeOverview.originalNodes > MAX_MAP_NODES && overview.nodes.length));

  4. 38 more changed Atlas pages open a reader that only lists child links. blueprint-dag.js:1671. Of the 198 Atlas pages whose default changed, 38 besides Data Structures have no prose and no statements:

    • 15 collections, for example roadmap/Analysis/MathlibExt (26 links);
    • 6 transparent parts, for example roadmap/Analysis/MathlibExt/part-01 (20 links);
    • 17 topic indexes, for example .../part-01/complex (24 links).

    declutterReader skips collection and transparent owners and keeps lists with anchored items, so these lists stay visible and repeat the Topics outline. Main opened them map-first. This matches the rule as written, so no change is needed unless Jack wants navigation-only pages treated as overviews. In that case, widen note 1's flag, since readerIsOverview refuses collection and transparent owners and anchored items.

  5. No test covers the half of the flag that keeps map-first. blueprint-dag.js:1671 (the originalNodes clause) and 2588 (the floor). The new tests cover only chapters that switch to the reader. Nothing asserts that a scope over 120 nodes keeps the class, the collapsed reader and the floor. Nothing asserts that an authored-area overview keeps the floor on zoom-out either: tests/test_scoped_overview_browser.py:584 checks the scale after a fit. The flat test's reader check (tests/test_dag_viewer_browser.py:1198) uses to_contain_text, which passes while the reader is collapsed. Removing the clause or the floor would leave CI green. The PR body says so. Fix:

    • Add a chapter of 121 or more linked statements to the reader-first fixture. Use linked statements so that the test does not pin note 3. Assert the class, a 48 px reader, and a scale of at least 0.9 after four Zoom out clicks.
    • In tests/test_scoped_overview_controls.py, next to the test at line 228, assert that ui$coarseOverview is true and that zoomAt(.8, ...) from 0.9 stays at 0.9. Then delete the areas, call updateProjection(), and assert that the flag is false and the scale drops below 0.9. Both halves pass at the head in a Node probe.
  6. The phone Back test takes its "no shared card" precondition from a Python copy of neighborhood(). tests/test_integrated_phone_back_browser.py:50. The crash needs every framing card of B to be missing from A's scene: sceneFrame returns null only for an empty box list (blueprint-dag.js:1175), and .contained is read at 2246. _disjoint_results derives that precondition from dependencies/index.json, which matches neighborhood() (1601) today. Suppose neighborhood() later draws a card that both maps share. Then both cases pass even with the calculateActive() call at 3143 removed. Fix: before page.go_back(), read the drawn card ids on A's and B's maps with page.eval_on_selector_all('.bp-dag-node[data-autoform-node-id]', 'els => els.map(e => e.dataset.autoformNodeId)'), and assert that the two sets are disjoint.

What was checked and holds. The agents ran the unmodified updateProjection, applyRoute, setSheet and zoomAt from main 91490d7 and from the head in Node, over each project's real index and page graphs. Each page was run as a desktop load, on a phone and as a map-card arrival. On desktop, every page whose default changed went from a collapsed reader with the 0.9 floor to reader-first with free zoom, and no phone reader default changed. The changed pages were 0 of 5 in the bundled example, 4 of 8 in zeta(7) (its four context-card chapters), 5 of 67 in Hatcher, 202 of 244 in Hartshorne and 198 of 2,053 in Atlas; Hartshorne's include the landing page, whose hero main hid behind the 48 px handle, and Atlas's seven authored-area or over-120-node scopes stay map-first with the floor. The early calculateActive() in setSheet is guarded by ui$activeDirty, which updateProjection and applyRoute set, so it builds the new route's set and refresh(true) keeps it. Same-route handle and Map-button calls do nothing new, restoreState skips the branch, and #444's sheet states are untouched. ui$coarseOverview implies ui$scopeOverview, and the phone branch is equivalent to main's. The reads switched to the new flag match the CSS, and the ui$scopeOverview reads left in place are group navigation. On phones, shelf and context maps also lose the 0.9 floor, as at e2a7f39. Every Node harness that evaluates a reader of the new flag declares it. The 145,000-byte limit is in all five code pins and three docs, with no 144 left. The bundle is 144,080 bytes (main 143,993), and its banner matches the source. The two restored assertions match e2a7f39. No command, flag, JSON key or schema changes. Not run locally: pytest, Lean or WebKit, because load was around 300, swap was over 11 GB, and local WebKit does not work on this Mac. Hatcher and Hartshorne were rendered from copies without their source_units: coverage lines, a format this renderer rejects. The Atlas numbers come from the Oct 22 preview at eceb8bfd, because a head re-render stalled. The PR body's Hilbert-Smith counts were not rechecked.

CI and merge state. At b422ab6, run 41002603872 is 9/9 green: eight jobs and the CLA check. WebKit has 207 passed and 1 skipped, against 203 and the same skip on main (run 40981081987); the four extra passes are the two new files. The guard runs show that the tests catch both bugs. On main's explorer code all six targeted tests fail (run 40995680060). With one fix removed, only that fix's tests fail (phone, reader). Main has since moved 48 commits to 8155f23, and GitHub reports #507 as conflicting, so the body's "Merges cleanly" line is out of date. A dry-run merge conflicts only in two files:

A scratch rebuild of the merged sources gives 144,080 bytes, under the limit. Rebase, run make build-explorer, and merge once CI passes on the new head.

Posted by PR swarm: 320-329 Reviewer

…d-reader-first

# Conflicts:
#	CHANGELOG.md
#	autoform_cli/assets/blueprint-explorer.min.js
@Deicyde

Deicyde commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Head check at 898d31f: merge once CI passes. CI has passed. 898d31f merges main 8155f23 into b422ab6, the head reviewed above.

Posted by PR swarm: 320-329 Reviewer

…d-reader-first

# Conflicts:
#	CHANGELOG.md
#	autoform_cli/assets/blueprint-explorer.min.js
@Deicyde

Deicyde commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Head check at 13db62e: ready to merge. It merges main after #512, which conflicted in two places. CHANGELOG.md takes #512's "No size caps on published data" entry, which replaced #508's, and keeps this PR's entry. The explorer bundle is rebuilt from the merged sources at 143,570 bytes, and all five pins of the 145,000-byte limit survive. No source or test line of this PR changed. CI passes 9/9 (run 41166327902), and the head merges cleanly with current main.

Posted by PR swarm: 320-329 Reviewer

@Deicyde
Deicyde merged commit 59b826b into main Oct 31, 2026
9 checks passed
@Deicyde
Deicyde deleted the fix/442-phone-back-and-reader-first branch October 31, 2026 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant