Pin named layouts to the sidebar - #592
Conversation
srid
left a comment
There was a problem hiding this comment.
Reviewed against pinning-layouts.md. The four ruled decisions are all honoured: always-named layout pins with an empty name refused, the chord gone everywhere (key table, help table, both palette handlers, the ⌘⇧P hint), a whole-workspace push with Back restoring the previous workspace, and pages-only storage with a round-trip unit test. pin_layout.feature covers all eleven scenarios the brief listed plus two more, and the old chord-driven step now goes through the palette. Docs are updated in the same PR. Good work.
Verdict: changes requested (posted as a comment, since GitHub refuses a formal request-changes on one's own PR) for one deviation from the brief and one gap, with a few smaller notes inline:
- The brief said not to touch the page grammar for
/s/, and this PR does (routes.ts). Details inline, including the import cycle it introduces and what it does to a vault with a top-levels/directory. - A layout row on
Pins.olai's own page is not drawn as a place. The brief's storage section anddocs/format.mdboth say a title that names a place is drawn as that place wherever it appears.NodeTitleonly ever builds page targets, so[Planning](/s/…)renders as a plain markdown link there, and clicking it falls through to a full browser navigation. Either draw it with the shared face (which already knows the layout kind) or say in the docs that the rule stops at the shelf, and add a scenario alongside "Pins.olai opened as an outline reads like an outline".
Smaller: ⌘/Ctrl-click on a layout pin is swallowed instead of left to the browser; a doc sentence about re-choosing the command has no scenario behind it after the old one was deleted; and the new docs/format.md paragraph sits above the section's lede.
| const { pathname, search, fragment } = parts | ||
| // A workspace ending in a filename must not be mistaken for that file. | ||
| // Workspace grammar belongs above page routes, including plugin claims. | ||
| if (pathname.startsWith(WORKSPACE_PREFIX)) return null |
There was a problem hiding this comment.
The brief was explicit here: "Do not add a page claim for /s/. routeIn/routeOf keep returning what they return; a workspace is a level above a route and stays that way." This line makes the page grammar aware of the workspace prefix, so the layering is now inverted.
Two consequences worth weighing:
- A vault with a top-level
s/directory: every file under it now parses as no page at all. ItsLinkhrefs failrouteIn, a pin to it stops being a pin, and a pane segments%2Fnotes.olaidecodes to the front page. Before this PR only the lone address bar was confused by such a path (the/s/intercept inworkspaceOf), so this widens an existing edge into every link and pin. routes.tsnow imports values fromworkspace.ts, andworkspace.tsalready importsHOME_ROUTEandhrefOfPlainfromroutes.ts. It runs because the uses are inside functions, but it is a cycle the brief's layering was there to avoid.
The reason given in the PR body is that a workspace address ending in a filename would otherwise read as a page pin. That is a pins-side ordering question: try layoutIn before addressIn in pinOf and the grammar can stay as it was. If there is a case that ordering does not cover, say which in the comment here.
| * cannot be read names nothing, and the address means what an unrecognised one | ||
| * means. | ||
| */ | ||
| import { layoutHref, layoutIn, WORKSPACE_PREFIX, type Workspace } from "./workspace.ts" |
There was a problem hiding this comment.
This is the other half of the cycle noted below. If layoutIn/layoutHref stay on Routing, the import can be broken by moving WORKSPACE_PREFIX and the two functions' pure parts here rather than importing them from the module that imports this one. Alternatively put them on Router only, which is what the brief suggested as the fallback.
| <span class="flex min-w-0 flex-1 items-center gap-1.5"> | ||
| <Face | ||
| route={route()} | ||
| target={{ kind: "page", route: route() }} |
There was a problem hiding this comment.
Always kind: "page", and the addressIn above it returns undefined for a /s/ title, so a layout row opened in Pins.olai itself falls through to TitleHtml and draws as an ordinary markdown link. The docs added in this PR say the split mark is the shared face's business and docs/format.md says a title that names a place is drawn as that place wherever it appears. Either resolve a layout target here too, or narrow the doc claim to the shelf. Either way a scenario on Pins.olai's own page is missing.
| <a href={href()} class={ROW} data-testid={TESTID.pinLink} | ||
| aria-current={props.current ? "page" : undefined} title={props.pin.bare} | ||
| onClick={(event) => { | ||
| event.preventDefault() |
There was a problem hiding this comment.
preventDefault() unconditionally, before checking the modifiers. The page branch goes through Link, which returns early when ours(event) is false so ⌘/Ctrl-click and middle-click stay the browser's (a new tab, since the href is real). The brief only asked for Alt and Shift to behave like a plain click. Suggest: if metaKey || ctrlKey || button !== 0, return without preventing, so a layout pin can still be opened in a new tab.
| href={href()} | ||
| onClick={(event) => { | ||
| if (props.target.kind !== "layout") return | ||
| event.preventDefault() |
There was a problem hiding this comment.
Same as in Pin.tsx: this swallows ⌘/Ctrl-click for a pressable layout name. Check the modifiers before preventDefault().
| And I pin the layout | ||
| And I name the pin "Planning" | ||
| Then the pinned shelf holds "/s/house.olai/garden.olai" | ||
| When I pin the layout |
There was a problem hiding this comment.
The step clicks whichever of Pin this layout… / Unpin this layout is present, so nothing asserts the label flips to Unpin this layout while standing in a pinned layout. One Then the palette offers "Unpin this layout" before this line would pin the UX the brief describes. Same for the current-ness rule "regardless of widths or focus": the unit test covers it, but no scenario opens the layout at different widths and checks the shelf lights up.
| - **Escape writes nothing at all.** The question comes before the pin, so backing out of it backs out of the whole gesture. Backing out is not a mode, either: the palette command works again. | ||
|
|
||
| **A question owns the modal while it is up.** `⌘⇧P` pressed again over its own question does nothing at all — the question that press would ask is already on screen, and asking it a second time would hand the box back its opening words over the name you are half-way through typing. It is the same rule the caret, Tab and Escape already keep there: a question is answered or backed out of, and nothing pressed elsewhere becomes its answer or writes past it. | ||
| **A question owns the modal while it is up.** Choosing a pin command again preserves the name being typed. Answer the question or press Escape to back out. |
There was a problem hiding this comment.
"Choosing a pin command again preserves the name being typed." The scenario that backed the old version of this sentence was deleted and nothing replaced it. The brief asked for the ⌘K equivalent or to fold it into another scenario. If the palette cannot offer the row while a question is up, this sentence is describing nothing and should go; if it can, add the scenario.
|
|
||
| ## Pins | ||
|
|
||
| A pin title may also be a workspace address: `/s/` followed by percent-encoded page addresses, one per pane (without each page's leading slash). For example, `[Orchestrating](/s/%23abc/orchestrator%2Flanes.olai)`. The app always writes layout pins with a supplied name and saves pages only, with no `?w=` widths or `?f=` focus. Readers tolerate those parameters and the workspace codec's `?a=` and `?t=` extensions; following a pin restores equal widths and first-pane focus. Bare workspace titles written by hand remain pins, deriving pane names and using an unresolved node's address when necessary. |
There was a problem hiding this comment.
This paragraph lands above the section's lede ("A pin is an ordinary node…") and before the grammar it extends ("The title is the address this app would spell for that page…"). Move it to right after that grammar paragraph and drop the extra blank line, so the section still reads top-down: what a pin is, which file, what a title may be.
|
Addressed the review in 1153e16 and 9f8a2e1:
Validation: all 75 targeted browser scenarios passed. A fresh full |
srid
left a comment
There was a problem hiding this comment.
Re-reviewed at 9f8a2e1 against pinning-layouts.md and my earlier review. Verdict: approve (posted as a comment, since GitHub refuses a formal approval on one's own PR). CI is green on this head, the PR is still unmerged, and the brief's delivery rule is intact.
Resolved
/s/reservation in the page grammar — reverted.routes.tsis back to master, the workspace codec composes above it asWorkspaceRouting, andtargetIntries the workspace reading before the page reading. The import cycle is gone (routes.tsno longer importsworkspace.ts). A unit test and an e2e scenario both prove a real file unders/still resolves as a page and as a pane segment.- Layout rows on
Pins.olai's own page —NodeTitlenow resolves a target rather than a route, so a layout title draws the split mark there too, a named one opens the workspace in place without a reload, and a bare one opens the editor on its address. Covered by "Layout titles on the Pins outline share the face and navigate without reloading". - ⌘/Ctrl-click and middle-click swallowed — both handlers now return before
preventDefault()on those gestures. The new-tab scenario outline covers shelf and outline surfaces with both gestures. - "Unpin this layout" label never asserted — asserted in the undo scenario and in the new width/focus scenario, which also proves current-ness ignores
?w=and?f=. - Doc sentence with no scenario behind it — rewritten to what the palette actually does (the question replaces the command list) and backed by "The naming question replaces the command list until cancelled".
docs/format.mdparagraph placement — moved below the address grammar it extends; stray blank line gone.
New, optional
packages/plugins/outlines/src/browser/routing.tsstill exportsaddressInandtitleFacewrappers, andpackages/plugins/markdown/src/browser/routing.tshas the same pair. After this PR nothing outside tests calls either. Fine to leave for a follow-up, but they are now two title readers beside the one that knows about layouts, which is the shape thetargetIncomment argues against.event.button !== 0inside aclickhandler is harmless but moot: browsers deliver middle-click asauxclick, soclicknever sees button 1. The ⌘/Ctrl guard is what does the work. No change needed.
A sidebar pin can now restore an entire workspace. In a split, ⌘K offers “Pin this layout…” and asks for a required name; following the split-marked shelf entry replaces all panes in one history push. Back restores the previous workspace. Layouts save only ordered pages and reopen with equal widths and the first pane focused.
Navigation owns layout parsing, normalization, and whole-workspace opening through its declared service. Workspace routing composes above the unchanged page grammar, preserving files under
s/and avoiding a routes/workspace import cycle. The shared title resolver recognizes layouts before pages; both the shelf and outline titles use the shared layout face. Ctrl/⌘-click and middle-click retain native browser new-tab behavior.Pins retains ownership of shelf subscriptions, naming, writes, and the status line used by shelf controls. Live pane names come from navigation's existing owner-bound reports; service ownership and withdrawal lifetimes are preserved. Layouts use the existing named-address format. The pin keyboard chord is removed; page pinning remains in the palette and row menu.
Coverage includes storage, resize/focus normalization and current indicators, Back, replacing another split, modifier clicks, naming refusal and cancellation, rename/unpin/undo, missing and trashed targets, live agent-written pins, focused-page pinning, phone tabs, outline layout titles, browser new-tab gestures, and real files under
s/. User and plugin documentation is updated.The refactor separates three independently changing decisions:
savedLayoutowns normalization independently of the URL codec; the declared staticlayout-presscontract owns browser gesture policy for both shelf and outline anchors; and pins keeps status/timer lifetime instatus.ts, with writes and undo using the existing held edit capability inwrites.ts. No service owner or withdrawal order changes. Added coverage checks nested layouts, drag click suppression, already-consumed gestures, and normalization without a URL round trip.Validation: full
just cipassed on02dbe2cbbusingODU_HOSTS=/home/srid/.config/odu/hosts-lane-graph.json(run0mu02tw58-n1nzx191). All unit tests, typechecks, build/dependency and Cordis audits, and all 1,715 browser scenarios passed.