Repository navigation
Open favorite pages and blocks in shared detail - #84
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: 63015d4cff
ℹ️ 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".
| when Graph.Uuid.to_string page.uuid = block_id && not page.recycled -> | ||
| remember_page_revision t page.uuid revision; | ||
| Hashtbl.replace t.graph_pages block_id page; | ||
| detail_children t ~generation ~page ~root:(page_root_member page revision) ~limit ~after |
There was a problem hiding this comment.
When a favorite page is opened, detail_children projects its direct blocks via block_on_page, which turns block.parent = page.uuid into Journal_model.parent_id = None. Those blocks still expose Delete, but Journal_detail.stage_delete consequently records no parent and undo_delete only reattaches entries having Some parent, so pressing Undo on any direct page child leaves it absent from the detail even though the deletion was cancelled; it returns only after reloading. Preserve the synthetic page root as the deletion owner, or make page-root staging and Undo derive ownership from the detail branch.
Useful? React with 👍 / 👎.
| { uuid = page.uuid; page = page.uuid; parent = page.uuid; order = "page" | ||
| ; title = page.title; created_at_ms = page.created_at_ms | ||
| ; updated_at_ms = page.updated_at_ms; refs = []; tags = page.tags | ||
| ; properties = page.properties } |
There was a problem hiding this comment.
For a page containing an entity-backed property represented by a logseq.property/created-from-property child, the direct child is projected with parent_id=None while this adapter also exposes the page property itself. The detail renderer recognizes and names an owned property value only when its parent is Some root, so this flow renders the value once in the page's property fields and again as a separate generic “Property” row. Preserve the synthetic page-parent relationship or make the property ownership checks page-root-aware.
Useful? React with 👍 / 👎.
| let root = if Graph.Uuid.equal interest.root.block.uuid page.uuid | ||
| then page_root_member page revision else interest.root in | ||
| let interest = { interest with page; root } in |
There was a problem hiding this comment.
When a page detail has more than 128 direct children and a continuation has been loaded, remember_detail_members leaves t.detail_children[root] containing only the most recently returned slice. A later page title or property change reaches this branch and emits Children_reconciled from that last slice; if it was the final slice, its continuation is None, so Journal_detail.reconcile_children replaces the complete child list with only that slice and the earlier children disappear until the detail is reloaded. Retain an authoritative first-page prefix or avoid issuing a full children reconciliation from a continuation-only cache entry.
Useful? React with 👍 / 👎.
| | (V2_missing_page { uuid; _ } | ||
| | V2_present_page {page={uuid;recycled=true;_};_}), Changed_detail_page _ -> |
There was a problem hiding this comment.
If the selected page is recycled after its page lookup completes but before the initial V2_get_children response arrives, this branch emits Block_removed while the route is still Detail_loading without a staged detail, so Journal_routes.stage_delete ignores the removal. The outstanding Detail_children operation still carries the old page/root and later publishes Detail_loaded, causing the recycled page to open successfully. Retire the pending child read or send an owner-specific missing-detail completion that transitions the loading route before a late response can publish.
Useful? React with 👍 / 👎.
| | V2_favorite_page { uuid; _ } | V2_favorite_block { uuid; _ } -> | ||
| let block_id = Logseq_db_types.Graph_types.Uuid.to_string uuid in | ||
| ( open_detail t ~block_id ~request_generation |
There was a problem hiding this comment.
Combining page and block favorites in this case discards the target kind before creating the detail route. Consequently, a slow, missing, or failed page still renders the existing “Loading block,” “Block unavailable,” and “Unable to open block” states in application.ml, and page-copy progress likewise says “block and descendants.” Retain the target kind in the route or use entity-neutral wording so page failures and recovery feedback describe the item the user actually opened.
Useful? React with 👍 / 👎.
d0e2dbc to
77d8be5
Compare
1 participant
Favorite pages and outline blocks now open the shared detail view and return to Favorites. Page roots display their title/properties and children through the existing staged detail loader, with the same loading grace, pagination, depth/cycle guards and asset owners. Page append uses page revisions and exact parent/page confirmation; Copy reuses the existing bounded subtree traversal.
Favorites use compact single-line native list rows with shared mldoc/reference title resolution, document icons for pages and small solid bullets for blocks. The Favorites root omits Capture; page and outline detail retain Capture and Copy. Page roots omit outline-only mutation actions.
Validation:
c49728e). 115 Application, 140 runtime, 45 route/detail and all adaptive tests pass; source boundary is clean. The test-only rebase conflicts preserve both Status A and Favorites coverage. No task changes to protected spec, Dune, locks or Swift.cross_abi_validated=false), so it does not claim persisted graph, native picker/assets or full iOS ABI acceptance. No phone/private graph or real graph/sync-service writes.The new decision document validates.
spec-dev-tool check --allstill reports three unchanged historical document errors.Independent review: the separate
journal-code-reviewreviewer inspected final head77d8be500dc03b8bec36ad1a54cbdff2d903a06dand found no evidence-backed new issue, including the Status/Favorites test integration. This final review was static; local test results and native acceptance limits are listed above.Final full CI passed on
77d8be5: https://github.com/logseq/logseq_journal/actions/runs/38876472440 (all targets/native embed and full regression suite).