-
Notifications
You must be signed in to change notification settings - Fork 1
Navigate backlinks to their exact referring block (closes #83) #257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
607ad06
Add coverage for the exact-block navigate/reveal/highlight contract (…
brylie 198b2f4
Implement exact-block backlink navigation (closes #83)
brylie 8094b74
Fix a real cross-document deep-link race CodeRabbit found on PR #257
brylie cb8f3ed
Strengthen the stale-block-id deep-link test (CodeRabbit review)
brylie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| <script lang="ts"> | ||
| import { resolve } from '$app/paths'; | ||
| import type { Backlink } from '$lib/data/links'; | ||
| import Icon from './Icon.svelte'; | ||
|
|
||
| // Issue #83: navigates to the exact referring block, not just the | ||
| // referring Document. `onJumpTo` mirrors SyncedBlockUsage.svelte's own | ||
| // same-document short-circuit — a link whose sourceDocumentId is the | ||
| // Document already open gets an in-place scroll/focus/highlight via the | ||
| // page's navigateToBlock instead of a full navigation (which would be a | ||
| // no-op page reload landing on the same #block-<id> hash regardless, just | ||
| // slower and losing in-progress state elsewhere on the page). A | ||
| // cross-document link is left as a real <a href>, so browser Back and | ||
| // keyboard Enter/Space activation both keep working for free. | ||
| let { | ||
| spaceId, | ||
| currentDocumentId, | ||
| backlinks, | ||
| onJumpTo | ||
| }: { | ||
| spaceId: string; | ||
| currentDocumentId: string; | ||
| backlinks: Backlink[]; | ||
| onJumpTo: (documentId: string, recordId: string) => void; | ||
| } = $props(); | ||
| </script> | ||
|
|
||
| <section | ||
| class="mt-5 rounded-lg border border-border bg-surface/50 p-3" | ||
| aria-labelledby="backlinks-heading" | ||
| > | ||
| <div class="flex items-center gap-2 text-xs font-semibold tracking-wider text-muted uppercase"> | ||
| <Icon name="link" size={15} class="text-accent" /> | ||
| <h2 id="backlinks-heading">Backlinks</h2> | ||
| <span class="normal-case">{backlinks.length}</span> | ||
| </div> | ||
| {#if backlinks.length > 0} | ||
| <ul class="mt-2 space-y-2"> | ||
| {#each backlinks as backlink, index (`${backlink.sourceRecordId}-${index}`)} | ||
| <li class="min-w-0 text-sm"> | ||
| <a | ||
| href="{resolve('/space/[spaceId]/doc/[id]', { | ||
| spaceId, | ||
| id: backlink.sourceDocumentId | ||
| })}#block-{backlink.sourceRecordId}" | ||
| onclick={(e) => { | ||
| if (backlink.sourceDocumentId === currentDocumentId) { | ||
| e.preventDefault(); | ||
| onJumpTo(backlink.sourceDocumentId, backlink.sourceRecordId); | ||
| } | ||
| }} | ||
| class="font-medium text-fg underline underline-offset-2 transition-colors hover:text-accent" | ||
| > | ||
| {backlink.sourceDocumentTitle || 'Untitled Document'} | ||
| </a> | ||
| <p class="mt-0.5 truncate text-xs text-muted" title={backlink.context}> | ||
| {backlink.context} | ||
| </p> | ||
| </li> | ||
| {/each} | ||
| </ul> | ||
| {:else} | ||
| <p class="mt-2 text-xs text-muted italic">No pages link here yet.</p> | ||
| {/if} | ||
| </section> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,173 @@ | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
| import { render, screen } from '@testing-library/svelte'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import type { ComponentProps } from 'svelte'; | ||
| import type { Backlink } from '$lib/data/links'; | ||
| import SyncedBlockUsage from './SyncedBlockUsage.svelte'; | ||
|
|
||
| function instance(overrides: Partial<Backlink> = {}): Backlink { | ||
| return { | ||
| sourceDocumentId: 'doc-source', | ||
| sourceDocumentTitle: 'Source Doc', | ||
| sourceRecordId: 'block-1', | ||
| context: 'Some context text', | ||
| ...overrides | ||
| }; | ||
| } | ||
|
|
||
| function baseProps( | ||
| overrides: Partial<ComponentProps<typeof SyncedBlockUsage>> = {} | ||
| ): ComponentProps<typeof SyncedBlockUsage> { | ||
| return { | ||
| spaceId: 'space-1', | ||
| currentDocumentId: 'doc-current', | ||
| instances: [instance()], | ||
| onJumpTo: vi.fn(), | ||
| ...overrides | ||
| }; | ||
| } | ||
|
|
||
| // This is the one live surface using the navigate-to-`#block-<id>`-and-reveal | ||
| // mechanism issue #83 asks for — the general page_link/wiki-link Backlinks | ||
| // panel that issue describes was removed (#120) pending a shard-aware | ||
| // reverse index (#81/#70), so this is where that navigation contract is | ||
| // actually exercised today. | ||
| describe('SyncedBlockUsage (#153, #83)', () => { | ||
| it('lists each usage as a real link to its exact source-block fragment', async () => { | ||
| const user = userEvent.setup(); | ||
| render(SyncedBlockUsage, baseProps()); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 1 place' })); | ||
|
|
||
| const link = screen.getByRole('menuitem', { name: /Source Doc/ }); | ||
| expect(link).toHaveAttribute('href', expect.stringContaining('/space/space-1/doc/doc-source')); | ||
| expect(link.getAttribute('href')).toMatch(/#block-block-1$/); | ||
| }); | ||
|
|
||
| it('jumps in place and suppresses navigation when the usage is in the current document', async () => { | ||
| const user = userEvent.setup(); | ||
| const props = baseProps({ | ||
| currentDocumentId: 'doc-source', | ||
| instances: [instance({ sourceDocumentId: 'doc-source' })] | ||
| }); | ||
| render(SyncedBlockUsage, props); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 1 place' })); | ||
| await user.click(screen.getByRole('menuitem', { name: /Source Doc/ })); | ||
|
|
||
| expect(props.onJumpTo).toHaveBeenCalledExactlyOnceWith('doc-source', 'block-1'); | ||
| }); | ||
|
|
||
| // A real <a href> (not a JS-only handler) for a cross-document usage is | ||
| // what keeps browser Back and keyboard Enter/Space activation working for | ||
| // free — asserting onJumpTo is never called here is how this test proves | ||
| // the click was left to normal navigation instead of being intercepted. | ||
| it('does not intercept navigation when the usage is in a different document', async () => { | ||
| const user = userEvent.setup(); | ||
| const props = baseProps({ | ||
| currentDocumentId: 'doc-current', | ||
| instances: [instance({ sourceDocumentId: 'doc-other' })] | ||
| }); | ||
| render(SyncedBlockUsage, props); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 1 place' })); | ||
| await user.click(screen.getByRole('menuitem', { name: /Source Doc/ })); | ||
|
|
||
| expect(props.onJumpTo).not.toHaveBeenCalled(); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| }); | ||
|
|
||
| // The pointer-click test above only proves the onclick handler's own | ||
| // preventDefault branch is skipped for a cross-document usage — it | ||
| // doesn't rule out some *other* handler (e.g. a keydown listener) doing | ||
| // the same thing only for keyboard activation. Enter-on-a-focused-anchor | ||
| // dispatches a real click event the same way a browser does, so this | ||
| // listens for that event directly to confirm nothing prevented it. | ||
| it('does not intercept keyboard (Enter) activation when the usage is in a different document', async () => { | ||
| const user = userEvent.setup(); | ||
| const props = baseProps({ | ||
| currentDocumentId: 'doc-current', | ||
| instances: [instance({ sourceDocumentId: 'doc-other' })] | ||
| }); | ||
| render(SyncedBlockUsage, props); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 1 place' })); | ||
| const link = screen.getByRole('menuitem', { name: /Source Doc/ }); | ||
| let clickEvent: MouseEvent | undefined; | ||
| link.addEventListener('click', (e) => { | ||
| clickEvent = e as MouseEvent; | ||
| }); | ||
|
|
||
| link.focus(); | ||
| await user.keyboard('{Enter}'); | ||
|
|
||
| expect(clickEvent).toBeDefined(); | ||
| expect(clickEvent?.defaultPrevented).toBe(false); | ||
| expect(props.onJumpTo).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('shows a placeholder instead of a link list when there are no other usages', async () => { | ||
| const user = userEvent.setup(); | ||
| render(SyncedBlockUsage, baseProps({ instances: [] })); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 0 places' })); | ||
|
|
||
| expect(screen.getByText('No other locations yet.')).toBeInTheDocument(); | ||
| expect(screen.queryByRole('menuitem')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('closes on Escape', async () => { | ||
| const user = userEvent.setup(); | ||
| render(SyncedBlockUsage, baseProps()); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 1 place' })); | ||
| expect(screen.getByRole('menu')).toBeInTheDocument(); | ||
|
|
||
| await user.keyboard('{Escape}'); | ||
| expect(screen.queryByRole('menu')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('ArrowDown/ArrowUp move roving focus among usage links, wrapping at each end', async () => { | ||
| const user = userEvent.setup(); | ||
| const props = baseProps({ | ||
| instances: [ | ||
| instance({ sourceRecordId: 'block-1', sourceDocumentTitle: 'First Doc' }), | ||
| instance({ sourceRecordId: 'block-2', sourceDocumentTitle: 'Second Doc' }) | ||
| ] | ||
| }); | ||
| render(SyncedBlockUsage, props); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 2 places' })); | ||
| const first = screen.getByRole('menuitem', { name: /First Doc/ }); | ||
| const second = screen.getByRole('menuitem', { name: /Second Doc/ }); | ||
| expect(first).toHaveFocus(); | ||
|
|
||
| await user.keyboard('{ArrowDown}'); | ||
| expect(second).toHaveFocus(); | ||
|
|
||
| await user.keyboard('{ArrowDown}'); | ||
| expect(first).toHaveFocus(); | ||
|
|
||
| await user.keyboard('{ArrowUp}'); | ||
| expect(second).toHaveFocus(); | ||
| }); | ||
|
|
||
| it('offers Detach only when onDetach is provided, and calls it', async () => { | ||
| const user = userEvent.setup(); | ||
| const props = baseProps({ onDetach: vi.fn() }); | ||
| render(SyncedBlockUsage, props); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 1 place' })); | ||
| await user.click(screen.getByRole('menuitem', { name: 'Detach to independent copy' })); | ||
|
|
||
| expect(props.onDetach).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| it('does not offer Detach when onDetach is absent', async () => { | ||
| const user = userEvent.setup(); | ||
| render(SyncedBlockUsage, baseProps()); | ||
|
|
||
| await user.click(screen.getByRole('button', { name: 'Used in 1 place' })); | ||
|
|
||
| expect(screen.queryByRole('menuitem', { name: 'Detach to independent copy' })).toBeNull(); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.