feat: wire note-links API into graph page for cross-reference edges #85

Merged
forgejo_admin merged 2 commits from 74-wire-note-links-graph into main 2026-03-28 05:28:45 +00:00

Summary

  • Wire the existing getNoteLinks API client into the graph page to fetch and display cross-reference edges (dashed accent-colored lines) alongside parent-child edges (solid lines)
  • Previously the graph only showed parent_slug relationships; now it also shows note-links from the /notes/{slug}/links endpoint
  • Uses batched concurrent requests (10 at a time) with a progress indicator during loading

Changes

  • src/routes/graph/+page.svelte: Added fetchAllNoteLinks() 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 footer

Test Plan

  • npm run build passes with no errors
  • Load /graph page -- dashed reference edges appear alongside solid parent edges
  • Stats footer shows "(N parent, M reference)" breakdown
  • Filter pills work correctly with mixed edge types
  • Loading progress updates during link fetching

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes (1 file changed)
  • Commit messages are descriptive
  • Closes #74
  • forgejo_admin/pal-e-app#74 -- port graph page from playground
  • pal-e-docs -- project this work belongs to
## Summary - Wire the existing `getNoteLinks` API client into the graph page to fetch and display cross-reference edges (dashed accent-colored lines) alongside parent-child edges (solid lines) - Previously the graph only showed parent_slug relationships; now it also shows note-links from the `/notes/{slug}/links` endpoint - Uses batched concurrent requests (10 at a time) with a progress indicator during loading ## Changes - `src/routes/graph/+page.svelte`: Added `fetchAllNoteLinks()` 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 footer ## Test Plan - [x] `npm run build` passes with no errors - [ ] Load `/graph` page -- dashed reference edges appear alongside solid parent edges - [ ] Stats footer shows "(N parent, M reference)" breakdown - [ ] Filter pills work correctly with mixed edge types - [ ] Loading progress updates during link fetching ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes (1 file changed) - [x] Commit messages are descriptive ## Related Notes - Closes #74 - `forgejo_admin/pal-e-app#74` -- port graph page from playground - `pal-e-docs` -- project this work belongs to
feat: wire note-links API into graph page for cross-reference edges
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
23ae01284c
The 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>
Author
Owner

QA Review -- PR #85

Summary

Single-file change (60+/4-) wiring getNoteLinks into the graph page to display cross-reference edges alongside parent-child edges. Clean, focused scope matching issue #74.

Findings

No blockers.

Nits (non-blocking):

  1. 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/edges endpoint on pal-e-api would eliminate this).

  2. Potential parent/reference edge overlap -- If note A has parent_slug: B AND 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.

  3. 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 $derived that computes both counts. Not a performance issue at graph scale.

Checklist

  • Diff matches PR description
  • No secrets or credentials
  • No unrelated file changes
  • Build verified (npm run build passes per PR)
  • getNoteLinks already exists in api-client -- no new API surface
  • Deduplication logic is correct (outgoing-only + Set key)
  • Error handling via Promise.allSettled is appropriate
  • Loading progress indicator provides good UX during batch fetch

VERDICT: APPROVE

## QA Review -- PR #85 ### Summary Single-file change (60+/4-) wiring `getNoteLinks` into the graph page to display cross-reference edges alongside parent-child edges. Clean, focused scope matching issue #74. ### Findings **No blockers.** **Nits (non-blocking):** 1. **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/edges` endpoint on pal-e-api would eliminate this). 2. **Potential parent/reference edge overlap** -- If note A has `parent_slug: B` AND 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. 3. **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 `$derived` that computes both counts. Not a performance issue at graph scale. ### Checklist - [x] Diff matches PR description - [x] No secrets or credentials - [x] No unrelated file changes - [x] Build verified (`npm run build` passes per PR) - [x] `getNoteLinks` already exists in api-client -- no new API surface - [x] Deduplication logic is correct (outgoing-only + Set key) - [x] Error handling via `Promise.allSettled` is appropriate - [x] Loading progress indicator provides good UX during batch fetch VERDICT: APPROVE
Author
Owner

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. Adds fetchAllNoteLinks() with concurrency-controlled batch fetching (10 at a time via Promise.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 (NoteLink with direction: 'incoming' | 'outgoing') is well-established -- already used identically in src/routes/notes/[slug]/+page.svelte. The outgoing-only filter plus edgeSet dedup correctly prevents duplicate edges. The slugSet guard correctly skips links to notes not in the current graph.

Force layout integration: Reference edges are appended to graphEdges before runSimulation is called, so they participate in the attraction forces. This is correct -- reference-linked nodes will cluster together visually.

Graceful degradation: Promise.allSettled with silent skip on rejected results is reasonable. If individual /notes/{slug}/links calls fail, the graph renders without those edges rather than crashing entirely.

BLOCKERS

1. Edge key collision in Svelte {#each} block

The template keys edges with edge.source + '-' + edge.target:

{#each filteredEdges as edge (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.type

This is a correctness bug that silently drops edges, which qualifies as a blocker.

NITS

  1. CONCURRENCY = 10 is 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 a const inside the function, which is acceptable, but a module-level BATCH_SIZE or LINK_FETCH_CONCURRENCY would be clearer.

  2. Stats footer double-scans filteredEdges. The template computes .filter(e => e.type === 'parent').length and .filter(e => e.type === 'reference').length separately, scanning the array twice. At current scale this is fine. If the graph grows, consider a $derived that computes both counts in a single pass.

  3. Silent partial failure. When Promise.allSettled rejects 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 a console.warn.

  4. 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

  • Branch named after issue: 74-wire-note-links-graph references issue #74
  • PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
  • Related section references issue: Closes #74, forgejo_admin/pal-e-app#74
  • Related references plan slug: References pal-e-docs project but no specific plan slug
  • No secrets committed
  • No unnecessary file changes: 1 file changed, tightly scoped
  • Commit messages are descriptive

PROCESS OBSERVATIONS

  • Change failure risk: Low-medium. The edge key collision bug is a rendering correctness issue, not a crash. The fix is a 1-line change.
  • Scope: Tightly scoped to a single file with a clear purpose. No scope creep.
  • API surface: Uses the existing getNoteLinks client function with no modifications to the API layer. Clean integration.
  • Performance consideration: For a knowledge base with 500 notes, this fires up to 50 batched API calls (500/10). On a slow connection this could take several seconds. The progress indicator addresses this well.

VERDICT: NOT APPROVED

One blocker: edge key collision in the {#each} block. Fix by including edge.type in the key string. After that fix, this PR is ready to merge.

## 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`. Adds `fetchAllNoteLinks()` with concurrency-controlled batch fetching (10 at a time via `Promise.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 (`NoteLink` with `direction: 'incoming' | 'outgoing'`) is well-established -- already used identically in `src/routes/notes/[slug]/+page.svelte`. The outgoing-only filter plus `edgeSet` dedup correctly prevents duplicate edges. The `slugSet` guard correctly skips links to notes not in the current graph. **Force layout integration**: Reference edges are appended to `graphEdges` before `runSimulation` is called, so they participate in the attraction forces. This is correct -- reference-linked nodes will cluster together visually. **Graceful degradation**: `Promise.allSettled` with silent skip on rejected results is reasonable. If individual `/notes/{slug}/links` calls fail, the graph renders without those edges rather than crashing entirely. ### BLOCKERS **1. Edge key collision in Svelte `{#each}` block** The template keys edges with `edge.source + '-' + edge.target`: ```svelte {#each filteredEdges as edge (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.type` This is a correctness bug that silently drops edges, which qualifies as a blocker. ### NITS 1. **`CONCURRENCY = 10` is 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 a `const` inside the function, which is acceptable, but a module-level `BATCH_SIZE` or `LINK_FETCH_CONCURRENCY` would be clearer. 2. **Stats footer double-scans `filteredEdges`.** The template computes `.filter(e => e.type === 'parent').length` and `.filter(e => e.type === 'reference').length` separately, scanning the array twice. At current scale this is fine. If the graph grows, consider a `$derived` that computes both counts in a single pass. 3. **Silent partial failure.** When `Promise.allSettled` rejects 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 a `console.warn`. 4. **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 - [x] Branch named after issue: `74-wire-note-links-graph` references issue #74 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related section references issue: `Closes #74`, `forgejo_admin/pal-e-app#74` - [ ] Related references plan slug: References `pal-e-docs` project but no specific plan slug - [x] No secrets committed - [x] No unnecessary file changes: 1 file changed, tightly scoped - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Change failure risk**: Low-medium. The edge key collision bug is a rendering correctness issue, not a crash. The fix is a 1-line change. - **Scope**: Tightly scoped to a single file with a clear purpose. No scope creep. - **API surface**: Uses the existing `getNoteLinks` client function with no modifications to the API layer. Clean integration. - **Performance consideration**: For a knowledge base with 500 notes, this fires up to 50 batched API calls (500/10). On a slow connection this could take several seconds. The progress indicator addresses this well. ### VERDICT: NOT APPROVED One blocker: edge key collision in the `{#each}` block. Fix by including `edge.type` in the key string. After that fix, this PR is ready to merge.
fix: include edge type in graph {#each} key to prevent collision
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline failed
d05aaeba3d
When two nodes are connected by both a parent edge and a reference
edge, the key `source-target` is identical for both. Svelte deduplicates
keyed each blocks, silently dropping one edge. Adding edge.type to the
key ensures both edges render.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Author
Owner

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: d05aaeb

## 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: d05aaeb
Author
Owner

PR #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 includes edge.type:

{#each filteredEdges as edge (edge.source + '-' + edge.target + '-' + edge.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.type is a discriminated union ('parent' | 'reference') defined in the GraphEdge interface (line 28), so the composite key is now unique for all valid edge combinations.

API integration: getNoteLinks is already exported from $lib/api-client.ts (line 321) with proper encodeURIComponent on the slug parameter. The NoteLink interface (line 97) confirms direction: 'incoming' | 'outgoing' exists, matching the link.direction !== 'outgoing' filter in fetchAllNoteLinks.

Concurrency control: Batch size of 10 with Promise.allSettled is 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 edgeSet with "source->target" keys combined with the direction === '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 dashed var(--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 .svelte component 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 in e2e/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

  1. 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 $derived computed property would be cleaner and avoid re-filtering on every render cycle. Minor.

  2. 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 /graph and 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.

  3. slugSet computed twice: buildGraph creates its own slugSet (line 135) even though fetchAllNoteLinks already computed the same set (line 90). Passing the set as a parameter would avoid the redundant allocation. Cosmetic.

  4. 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

  • Branch named after issue: 74-wire-note-links-graph references issue #74
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related section references forgejo_admin/pal-e-app#74 and pal-e-docs project
  • Related section does not reference a plan slug (no plan slug apparent for this work)
  • No secrets committed (1 file changed, pure UI logic)
  • No unnecessary file changes (single file: src/routes/graph/+page.svelte)
  • Commit messages are descriptive

PROCESS OBSERVATIONS

  • Change failure risk: Low. This is additive visualization logic with graceful error handling (Promise.allSettled + silent skip). Worst case on API failure is the graph renders without reference edges, same as before.
  • Deployment frequency: Single-file change, no infrastructure or config changes. Clean merge path.
  • Documentation gap: The graph page has zero automated test coverage across two PRs now (#78 and #85). Recommend tracking this as a follow-up issue before the test debt compounds further.

VERDICT: APPROVED

## PR #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 includes `edge.type`: ``` {#each filteredEdges as edge (edge.source + '-' + edge.target + '-' + edge.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.type` is a discriminated union (`'parent' | 'reference'`) defined in the `GraphEdge` interface (line 28), so the composite key is now unique for all valid edge combinations. **API integration**: `getNoteLinks` is already exported from `$lib/api-client.ts` (line 321) with proper `encodeURIComponent` on the slug parameter. The `NoteLink` interface (line 97) confirms `direction: 'incoming' | 'outgoing'` exists, matching the `link.direction !== 'outgoing'` filter in `fetchAllNoteLinks`. **Concurrency control**: Batch size of 10 with `Promise.allSettled` is 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 `edgeSet` with `"source->target"` keys combined with the `direction === '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 dashed `var(--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 `.svelte` component 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 in `e2e/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 1. **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 `$derived` computed property would be cleaner and avoid re-filtering on every render cycle. Minor. 2. **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 `/graph` and 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. 3. **`slugSet` computed twice**: `buildGraph` creates its own `slugSet` (line 135) even though `fetchAllNoteLinks` already computed the same set (line 90). Passing the set as a parameter would avoid the redundant allocation. Cosmetic. 4. **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 - [x] Branch named after issue: `74-wire-note-links-graph` references issue #74 - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related section references `forgejo_admin/pal-e-app#74` and `pal-e-docs` project - [ ] Related section does not reference a plan slug (no plan slug apparent for this work) - [x] No secrets committed (1 file changed, pure UI logic) - [x] No unnecessary file changes (single file: `src/routes/graph/+page.svelte`) - [x] Commit messages are descriptive ### PROCESS OBSERVATIONS - **Change failure risk**: Low. This is additive visualization logic with graceful error handling (`Promise.allSettled` + silent skip). Worst case on API failure is the graph renders without reference edges, same as before. - **Deployment frequency**: Single-file change, no infrastructure or config changes. Clean merge path. - **Documentation gap**: The graph page has zero automated test coverage across two PRs now (#78 and #85). Recommend tracking this as a follow-up issue before the test debt compounds further. ### VERDICT: APPROVED
forgejo_admin deleted branch 74-wire-note-links-graph 2026-03-28 05:28:45 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/pal-e-docs-app!85
No description provided.