feat: wire note-links API into graph page for cross-reference edges #85
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
forgejo_admin/pal-e-docs-app!85
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "74-wire-note-links-graph"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
getNoteLinksAPI client into the graph page to fetch and display cross-reference edges (dashed accent-colored lines) alongside parent-child edges (solid lines)/notes/{slug}/linksendpointChanges
src/routes/graph/+page.svelte: AddedfetchAllNoteLinks()function with concurrency-controlled batch fetching, deduplication via outgoing-only filter, reference edge merging into force layout, progress status updates, and edge type breakdown in stats footerTest Plan
npm run buildpasses with no errors/graphpage -- dashed reference edges appear alongside solid parent edgesReview Checklist
Related Notes
forgejo_admin/pal-e-app#74-- port graph page from playgroundpal-e-docs-- project this work belongs toThe graph previously only showed parent-child relationships from parent_slug. Now fetches note-links via the /notes/{slug}/links endpoint to display cross-reference edges (dashed lines) alongside parent edges (solid lines). Uses batched concurrent requests (10 at a time) with progress indicator. Closes #74 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>QA Review -- PR #85
Summary
Single-file change (60+/4-) wiring
getNoteLinksinto the graph page to display cross-reference edges alongside parent-child edges. Clean, focused scope matching issue #74.Findings
No blockers.
Nits (non-blocking):
N+1 request pattern -- For 500 notes at concurrency 10, this issues 500 HTTP requests in 50 sequential batches. Acceptable given no bulk endpoint exists, but worth noting as a future optimization candidate (bulk
/graph/edgesendpoint on pal-e-api would eliminate this).Potential parent/reference edge overlap -- If note A has
parent_slug: BAND a note-link to B, both a solid parent edge and a dashed reference edge render between the same pair. This is arguably informative (shows both relationship types), but could be deduplicated if the visual is noisy.Stats footer filter calls --
filteredEdges.filter(...)is called twice in the template (once for parent count, once for reference count). Minor -- could be a single$derivedthat computes both counts. Not a performance issue at graph scale.Checklist
npm run buildpasses per PR)getNoteLinksalready exists in api-client -- no new API surfacePromise.allSettledis appropriateVERDICT: APPROVE
PR #85 Review
DOMAIN REVIEW
Tech stack: SvelteKit 2 / Svelte 5 (runes mode) / TypeScript / client-side API fetching
Single-file change (+60/-4) to
src/routes/graph/+page.svelte. AddsfetchAllNoteLinks()with concurrency-controlled batch fetching (10 at a time viaPromise.allSettled), outgoing-only deduplication, reference edge merging into the force layout, progress status updates, and an edge type breakdown in the stats footer.Correctness: The API contract (
NoteLinkwithdirection: 'incoming' | 'outgoing') is well-established -- already used identically insrc/routes/notes/[slug]/+page.svelte. The outgoing-only filter plusedgeSetdedup correctly prevents duplicate edges. TheslugSetguard correctly skips links to notes not in the current graph.Force layout integration: Reference edges are appended to
graphEdgesbeforerunSimulationis called, so they participate in the attraction forces. This is correct -- reference-linked nodes will cluster together visually.Graceful degradation:
Promise.allSettledwith silent skip on rejected results is reasonable. If individual/notes/{slug}/linkscalls fail, the graph renders without those edges rather than crashing entirely.BLOCKERS
1. Edge key collision in Svelte
{#each}blockThe template keys edges with
edge.source + '-' + edge.target:If note A has a parent-child relationship to note B AND a cross-reference link to note B, both edges share the key
"A-B". Svelte deduplicates by key in keyed each blocks, so one edge will be silently dropped from rendering.Fix: Include the edge type in the key:
edge.source + '-' + edge.target + '-' + edge.typeThis is a correctness bug that silently drops edges, which qualifies as a blocker.
NITS
CONCURRENCY = 10is a magic number. Consider extracting to a named constant at module scope with a comment explaining why 10 (e.g., "avoid overwhelming the API with 500 concurrent requests"). Currently it is aconstinside the function, which is acceptable, but a module-levelBATCH_SIZEorLINK_FETCH_CONCURRENCYwould be clearer.Stats footer double-scans
filteredEdges. The template computes.filter(e => e.type === 'parent').lengthand.filter(e => e.type === 'reference').lengthseparately, scanning the array twice. At current scale this is fine. If the graph grows, consider a$derivedthat computes both counts in a single pass.Silent partial failure. When
Promise.allSettledrejects individual note-link fetches, there is no user-visible indication. Consider adding a subtle "(N notes had missing link data)" note if any results were rejected, or at minimum aconsole.warn.Test Plan items unchecked. The PR body shows 4 manual test items with 3 unchecked. These represent the acceptance criteria from issue #74's scope. Ideally, at least the stats footer breakdown and dashed edge rendering should have E2E coverage -- though this is an inherited gap from the graph page being merged without tests (PR #78).
SOP COMPLIANCE
74-wire-note-links-graphreferences issue #74Closes #74,forgejo_admin/pal-e-app#74pal-e-docsproject but no specific plan slugPROCESS OBSERVATIONS
getNoteLinksclient function with no modifications to the API layer. Clean integration.VERDICT: NOT APPROVED
One blocker: edge key collision in the
{#each}block. Fix by includingedge.typein the key string. After that fix, this PR is ready to merge.QA Blocker Fix
Fixed the edge key collision in the
{#each}block (line 322).Before:
edge.source + '-' + edge.target-- two edges between the same node pair (one parent, one reference) produce the same key, causing Svelte to silently drop one.After:
edge.source + '-' + edge.target + '-' + edge.type-- unique key per edge type, both edges render correctly.Commit:
d05aaebPR #85 Re-Review
Re-review after fix commit addressing the edge key collision blocker from the first review.
DOMAIN REVIEW
Stack: SvelteKit / TypeScript / SVG (client-side graph visualization)
Blocker fix verified: The
{#each}key on line 322 now includesedge.type:Previously the key was
edge.source + '-' + edge.target, which caused Svelte to silently drop duplicate source-target edges that differed only by type (parent vs reference). The fix is correct --edge.typeis a discriminated union ('parent' | 'reference') defined in theGraphEdgeinterface (line 28), so the composite key is now unique for all valid edge combinations.API integration:
getNoteLinksis already exported from$lib/api-client.ts(line 321) with properencodeURIComponenton the slug parameter. TheNoteLinkinterface (line 97) confirmsdirection: 'incoming' | 'outgoing'exists, matching thelink.direction !== 'outgoing'filter infetchAllNoteLinks.Concurrency control: Batch size of 10 with
Promise.allSettledis appropriate -- failed individual requests are silently skipped rather than crashing the whole graph. This is the right tradeoff for a visualization page.Deduplication logic: The
edgeSetwith"source->target"keys combined with thedirection === 'outgoing'filter correctly prevents duplicate reference edges. Since the API returns both incoming and outgoing from each note's perspective, filtering to outgoing-only means each cross-reference is counted exactly once.Edge rendering differentiation: Parent edges get solid
var(--color-border)lines at width 2 / opacity 0.7. Reference edges get dashedvar(--color-accent)lines at width 1 / opacity 0.35. Good visual hierarchy. Legend at bottom of SVG documents both styles.Accessibility: Nodes have
role="link",tabindex="0", and keyboard event handlers for Enter/Space (lines 342-349). The<title>element on each node (line 390) provides screen reader text. Filter pills are native<button>elements. No new accessibility regressions introduced.BLOCKERS
None. The previous blocker (edge key collision) is resolved.
Test coverage assessment: This PR adds 61 lines of new functionality to a single
.sveltecomponent with no accompanying tests. However, the existing test baseline for the graph page was already zero (the graph page was ported in PR #78 without tests, and the only graph reference in tests is a nav link visibility check ine2e/home.spec.ts). The new code is purely additive visualization logic (fetch + merge + display) with no auth, security, or data mutation paths. The test plan in the PR body notes manual verification items. Given this is a visualization enhancement on an already-untested page, I am not flagging this as a BLOCKER -- but the graph page as a whole needs test coverage (see nits).NITS
Stats footer double-filter: Lines 465-466 call
filteredEdges.filter()twice in the template -- once for parent count, once for reference count. For typical graph sizes this is negligible, but a$derivedcomputed property would be cleaner and avoid re-filtering on every render cycle. Minor.Graph page has zero test coverage: Neither this PR nor the original graph port (PR #78) added any tests. A single E2E test that loads
/graphand asserts the SVG renders with edges would cover the happy path. This is not a blocker for this PR specifically since the gap predates it, but it should be tracked as a follow-up issue.slugSetcomputed twice:buildGraphcreates its ownslugSet(line 135) even thoughfetchAllNoteLinksalready computed the same set (line 90). Passing the set as a parameter would avoid the redundant allocation. Cosmetic.Magic number:
CONCURRENCY = 10(line 94) is defined as a const inside the function, which is fine. No issue, just noting it is appropriately scoped.SOP COMPLIANCE
74-wire-note-links-graphreferences issue #74forgejo_admin/pal-e-app#74andpal-e-docsprojectsrc/routes/graph/+page.svelte)PROCESS OBSERVATIONS
Promise.allSettled+ silent skip). Worst case on API failure is the graph renders without reference edges, same as before.VERDICT: APPROVED