feat: hash-driven element activation for deep-linking#86
Conversation
Synchronize URL hash fragment with open/close state of <dialog>, [popover], and <details> elements. When the hash matches an element's ID the element is activated; when it deactivates the hash is cleared. Supports invoker buttons, <a href="#id"> links, and Back/Forward via popstate. Also extends the invoker polyfill with popover commands and adds a morphdom guard to preserve open dialog/popover top-layer state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ReviewOverall this is well-structured with good tests. Two issues worth addressing: Bug: missing try/catch in
|
There was a problem hiding this comment.
Pull request overview
Adds hash-driven “deep link” activation so URL fragments can open/close <dialog>, [popover], and <details> elements and stay in sync with browser history, integrating this behavior into LiveTemplate’s navigation and DOM morphing lifecycle.
Changes:
- Introduces
dom/hash-link.tsto synchronizelocation.hashwith activatable element open/close state usinghistory.pushState/replaceState, plus lifecycle wiring inlivetemplate-client.ts. - Extends the invoker polyfill to support popover commands (
show-popover,hide-popover,toggle-popover) and adds/updates unit tests. - Updates link interception to treat same-page hash links as “activations” when the hash points at an activatable element, and adds a morphdom guard to preserve top-layer state for open dialogs/popovers.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
dom/hash-link.ts |
New module implementing hash↔UI synchronization for dialogs/popovers/details. |
dom/invoker-polyfill.ts |
Adds popover command handling to the invoker polyfill. |
dom/link-interceptor.ts |
Intercepts same-page hash links to activatable targets and activates them. |
livetemplate-client.ts |
Wires hash-link setup/teardown/opening into client lifecycle; adds morphdom guard for top-layer elements. |
tests/hash-link.test.ts |
New unit tests for hash-link module behaviors and lifecycle idempotency. |
tests/invoker-polyfill.test.ts |
Adds tests for popover command support and command/target mismatches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function handleToggle(e: Event): void { | ||
| const el = e.target; | ||
| if (!(el instanceof Element)) return; | ||
| if (!el.id) return; | ||
|
|
||
| const handler = findHandler(el); | ||
| if (!handler) return; | ||
|
|
||
| if (handler.isOpen(el)) { | ||
| if (location.hash === "#" + el.id) return; | ||
| history.pushState(null, "", "#" + el.id); | ||
| } else { | ||
| if (location.hash !== "#" + el.id) return; | ||
| history.replaceState(null, "", location.pathname + location.search); | ||
| } | ||
| } | ||
|
|
||
| function handlePopstate(): void { | ||
| const id = location.hash.slice(1); | ||
|
|
||
| document.querySelectorAll("dialog[open]").forEach((el) => { | ||
| if (el.id !== id) (el as HTMLDialogElement).close(); | ||
| }); | ||
| document | ||
| .querySelectorAll("[popover]") | ||
| .forEach((el) => { | ||
| if ( | ||
| el instanceof HTMLElement && | ||
| el.matches(":popover-open") && | ||
| el.id !== id | ||
| ) { | ||
| el.hidePopover(); | ||
| } | ||
| }); | ||
|
|
||
| if (id) { | ||
| const el = document.getElementById(id); | ||
| if (el) { | ||
| const handler = findHandler(el); | ||
| if (handler && !handler.isOpen(el)) handler.open(el); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new hash-link behavior includes popover synchronization via toggle/popstate (push hash on open, clear on close, and close non-matching open popovers on back/forward), but the added unit tests only cover invoker-button clicks for popovers. Add tests that simulate popover toggle events and popstate-driven popover closes to ensure this synchronization works (and stays working) across changes.
| if (command === "show-popover") { | ||
| target.showPopover(); | ||
| } else if (command === "hide-popover") { | ||
| target.hidePopover(); | ||
| } else if (command === "toggle-popover") { | ||
| target.togglePopover(); |
There was a problem hiding this comment.
Popover command handling calls showPopover()/hidePopover()/togglePopover() without checking that those methods exist. On browsers that lack the Popover API but still have elements with a popover attribute in the DOM, this will throw and break all delegated clicks. Add a typeof (target as any).showPopover === "function" (etc.) guard (and optionally an open-state check) before invoking these methods, otherwise no-op like the dialog branch does.
| if (command === "show-popover") { | |
| target.showPopover(); | |
| } else if (command === "hide-popover") { | |
| target.hidePopover(); | |
| } else if (command === "toggle-popover") { | |
| target.togglePopover(); | |
| const popoverTarget = target as HTMLElement & { | |
| showPopover?: () => void; | |
| hidePopover?: () => void; | |
| togglePopover?: () => void; | |
| }; | |
| const isOpen = target.matches(":popover-open"); | |
| if (command === "show-popover") { | |
| if (typeof popoverTarget.showPopover === "function" && !isOpen) { | |
| popoverTarget.showPopover(); | |
| } | |
| } else if (command === "hide-popover") { | |
| if (typeof popoverTarget.hidePopover === "function" && isOpen) { | |
| popoverTarget.hidePopover(); | |
| } | |
| } else if (command === "toggle-popover") { | |
| if (typeof popoverTarget.togglePopover === "function") { | |
| popoverTarget.togglePopover(); | |
| } |
| { | ||
| matches: (el) => | ||
| el instanceof HTMLElement && el.hasAttribute("popover"), | ||
| isOpen: (el) => { | ||
| try { | ||
| return (el as HTMLElement).matches(":popover-open"); | ||
| } catch { | ||
| return false; | ||
| } | ||
| }, | ||
| open: (el) => (el as HTMLElement).showPopover(), | ||
| close: (el) => (el as HTMLElement).hidePopover(), | ||
| }, |
There was a problem hiding this comment.
The popover handler assumes the Popover API exists whenever an element has a popover attribute, and unconditionally calls showPopover()/hidePopover(). If showPopover/hidePopover are undefined (older browsers, partial polyfills), this will throw at runtime. Consider tightening matches (e.g., require the methods to be functions) and making open/close no-op safely when the API isn’t available.
| document | ||
| .querySelectorAll("[popover]") | ||
| .forEach((el) => { | ||
| if ( | ||
| el instanceof HTMLElement && | ||
| el.matches(":popover-open") && | ||
| el.id !== id | ||
| ) { | ||
| el.hidePopover(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
handlePopstate uses el.matches(":popover-open") without a try/catch. In environments where the :popover-open selector is unsupported, matches() throws a SyntaxError and breaks popstate handling entirely. Use the same safe wrapper approach as isOpen (or centralize it) before calling matches, and similarly guard hidePopover() if Popover API may be missing.
| /** @internal Test-only — do not call from production code. */ | ||
| export function teardownHashLink(): void { | ||
| if (!installed) return; | ||
| installed = false; | ||
|
|
||
| document.removeEventListener("click", handleClick); | ||
| document.removeEventListener("close", handleClose, true); | ||
| document.removeEventListener("toggle", handleToggle, true); | ||
| window.removeEventListener("popstate", handlePopstate); |
There was a problem hiding this comment.
teardownHashLink is documented as “Test-only — do not call from production code”, but this PR wires it into the production disconnect lifecycle. Either adjust the function’s visibility/contract (and comment) to reflect production use, or avoid calling it from LiveTemplateClient so the “test-only” promise remains true.
| if (target.pathname === window.location.pathname && target.hash) { | ||
| const hashId = target.hash.slice(1); | ||
| if (hashId && isHashLinkTarget(hashId)) { | ||
| e.preventDefault(); | ||
| activateHashTarget(hashId); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
The hash-link interception checks only pathname equality. If a link changes the query string but also has a hash (e.g. /items?page=2#dlg from /items?page=1), this branch returns early and bypasses LiveTemplate navigation entirely, causing a full-page navigation instead of the intended SPA/WS navigate path. Consider requiring both pathname and search to match the current URL before treating it as a hash-only activation case.
| !(toEl as Element).hasAttribute('data-lvt-force-update') && ( | ||
| (fromEl instanceof HTMLDialogElement && fromEl.hasAttribute('open')) || | ||
| (fromEl instanceof HTMLElement && fromEl.hasAttribute('popover') && safeMatchesPopoverOpen(fromEl)) |
There was a problem hiding this comment.
New code uses single quotes for attribute names ('data-lvt-force-update', 'open', 'popover') while the surrounding code in this method uses double quotes. For consistency with the rest of the file (and the repo’s predominant style), switch these to double quotes.
| !(toEl as Element).hasAttribute('data-lvt-force-update') && ( | |
| (fromEl instanceof HTMLDialogElement && fromEl.hasAttribute('open')) || | |
| (fromEl instanceof HTMLElement && fromEl.hasAttribute('popover') && safeMatchesPopoverOpen(fromEl)) | |
| !(toEl as Element).hasAttribute("data-lvt-force-update") && ( | |
| (fromEl instanceof HTMLDialogElement && fromEl.hasAttribute("open")) || | |
| (fromEl instanceof HTMLElement && fromEl.hasAttribute("popover") && safeMatchesPopoverOpen(fromEl)) |
- Refactor handlePopstate to use handlers array, fixing missing try/catch for :popover-open and adding details close on back/forward - Guard popover API calls (showPopover/hidePopover/togglePopover) for browsers without native support in hash-link and invoker-polyfill - Check both pathname and search in link-interceptor hash-only path - Fix teardownHashLink JSDoc (used in production disconnect()) - Normalize quote style in morphdom preservation guard - Add tests for popover toggle events and popstate details close Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ReviewOverall: Solid feature with good test coverage and thoughtful self-guarding design. A few things worth addressing: Bug:
|
- Extract safeMatchesPopoverOpen to hash-link.ts as shared export, removing the duplicate local definition in livetemplate-client.ts - Add comment explaining why openFromHash() is called after first render (setupHashLink's internal call fires before server content) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code ReviewOverall this is a clean, well-tested implementation. A few things worth checking before merge: Potential bug:
|
- Details elements are independent (not top-layer), so popstate should not close all open details when the hash changes — only dialogs and popovers participate in the close-all sweep - Mark safeMatchesPopoverOpen as @internal to prevent consumer reliance - Update tests to reflect details-are-independent semantics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ReviewBug / UX inconsistency —
// hash-link.ts:119
document.querySelectorAll("dialog, [popover]").forEach((el) => {The test Module-level singleton ( If two
It's marked Overall: the core logic is sound, the test coverage is thorough, and the |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ReviewGood implementation overall. The design is clean, event guards are correct, and test coverage is thorough. A few things to flag: Undocumented The morphdom skip ( Behavioral change in link-interceptor worth documenting Removing the Minor: redundant
Minor: Marked No blocking issues. The state machine, event ordering, and self-guarding close handler are solid. |
The server stopped emitting the "sm" (StaticsMap) field on TreeNode in v0.8.0 when fingerprint-based statics tracking replaced the per-path ClientStructureRegistry (#86). The client kept the read path and the parameter chain alive as a no-op fallback, but the underlying data has not flowed since v0.8.0 — every reachable codepath now passes undefined. Removed: - types.ts: staticsMap?: Record<string, string[]> field on TargetedRangeOp. - state/tree-renderer.ts: - staticsMap?: ... field on RangeStateEntry interface. - sm: existing.sm / value.sm / rangeStructure.sm reads (5 sites). - sm: ... writes into treeState and rangeState (3 sites). - The dead `_sk` lookup branch inside renderRangeItem. - staticsMap?: ... parameter from renderRangeItem and renderItemsWithStatics. - state/range-dom-applier.ts: - staticsMap parameter from RenderItemFn type and 6 internal helpers (apply, applyUpdateRow, applyInsertAfter, applyAppend, applyPrepend, renderItemsAtomic, renderAndParse). - Destructuring at apply() entry point. - livetemplate-client.ts: dropped sm arg in the renderItem callback wiring. - tests/range-dom-applier.test.ts: updated test renderItem callback to match the new 4-arg signature. No tests referenced StaticsMap, _sk, or "sm" — confirming the path was unreachable from any runtime scenario before this commit. All 529 tests pass; tsc --noEmit clean. Closes #17. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The server stopped emitting the "sm" (StaticsMap) field on TreeNode in v0.8.0 when fingerprint-based statics tracking replaced the per-path ClientStructureRegistry (#86). The client kept the read path and the parameter chain alive as a no-op fallback, but the underlying data has not flowed since v0.8.0 — every reachable codepath now passes undefined. Removed: - types.ts: staticsMap?: Record<string, string[]> field on TargetedRangeOp. - state/tree-renderer.ts: - staticsMap?: ... field on RangeStateEntry interface. - sm: existing.sm / value.sm / rangeStructure.sm reads (5 sites). - sm: ... writes into treeState and rangeState (3 sites). - The dead `_sk` lookup branch inside renderRangeItem. - staticsMap?: ... parameter from renderRangeItem and renderItemsWithStatics. - state/range-dom-applier.ts: - staticsMap parameter from RenderItemFn type and 6 internal helpers (apply, applyUpdateRow, applyInsertAfter, applyAppend, applyPrepend, renderItemsAtomic, renderAndParse). - Destructuring at apply() entry point. - livetemplate-client.ts: dropped sm arg in the renderItem callback wiring. - tests/range-dom-applier.test.ts: updated test renderItem callback to match the new 4-arg signature. No tests referenced StaticsMap, _sk, or "sm" — confirming the path was unreachable from any runtime scenario before this commit. All 529 tests pass; tsc --noEmit clean. Closes #17. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
dom/hash-link.ts— generic framework module that synchronizes URL hash fragments with open/close state of<dialog>,[popover], and<details>elements. Useshistory.pushState(notlocation.hash) to avoidhashchangedouble-activation errors. Self-guarding close handler naturally preventsreplaceStateduringpopstate-triggered closes without boolean flags.dom/invoker-polyfill.ts) to handleshow-popover,hide-popover, andtoggle-popovercommands on[popover]elements.dom/link-interceptor.ts) to intercept<a href="#id">clicks when the target is an activatable element, while respectingshouldSkipchecks (target="_blank",download,lvt-nav:no-intercept).livetemplate-client.tsto preserve open<dialog>and[popover]elements (browser top-layer state has no DOM representation).setupHashLink/teardownHashLink/openFromHashinto connect/disconnect/first-render lifecycle.Test plan
tests/hash-link.test.tscovering openFromHash, isHashLinkTarget, activateHashTarget, invoker button clicks, close/toggle events, popstate reconciliation, teardown idempotency, and setup idempotencytests/invoker-polyfill.test.tsfor popover commands#new-session-dialogopens dialog, reload persists, Back closes, Escape clears hash./scripts/e2e-remote.sh🤖 Generated with Claude Code