Skip to content

fix(preview): re-resolve markdown links when the page tree changes - #1684

Open
zorak1103 wants to merge 4 commits into
perber:mainfrom
zorak1103:fix/flaky-incoming-links
Open

zorak1103 wants to merge 4 commits into
perber:mainfrom
zorak1103:fix/flaky-incoming-links

Conversation

@zorak1103

Copy link
Copy Markdown
Contributor

Related issue

Fixes #1683

What changed

Fix: MarkdownLink re-resolves links when the page tree changes (ui/leafwiki-ui/src/features/preview/MarkdownLink.tsx)

MarkdownLink used to subscribe to the stable getPageByPath function, so a later change to the tree indexes never re-rendered it. MarkdownPreview memoizes the body of plain Markdown, so its own re-render did not reach the link either. A link rendered before the tree had loaded stayed a red "create page" button, and a link to a page that was later moved or deleted stayed clickable. The component now computes the lookup path before its early returns and subscribes to the target node itself: useTreeStore((s) => lookupPath === null ? null : (s.byPath[lookupPath] ?? null)). The selector returns either a stable node reference or null. getPageByPath/getPageById remain only for the wikilink-notfound click handler. Web, protocol-relative, asset, fragment, relative-path, read-only and navigation-state behaviour is unchanged.

Regression tests

  • MarkdownLink.test.tsx: the target appears after render (absolute and relative link); the target disappears after render (editor, and read-only). Store changes only, no rerender, no prop changes.
  • MarkdownPreview.test.tsx: [Target](/target) resolves when the tree arrives later, and unresolves when the target is moved away. This checks across the memoization boundary.
  • All six failed on the old code before the fix.

E2E (e2e/tests/page.spec.ts)

  • The existing test is renamed to move-page-updates-incoming-links-via-refactor-api, because it calls the refactor API, not the dialog. Every step and assertion is kept.
  • New server checkpoints: the saved referrer contains the original link; after apply, the target sits at the root and the referrer contains [target](/target) without the old path. A failed rewrite now fails as such instead of as a missing DOM link.
  • When the browser assertions fail, an incoming-link-evidence.json attachment holds the checkpoint values, the article HTML and the pathname, with no cookies, headers or credentials. The assertion error is rethrown.
  • New incoming-link-recovers-after-late-tree-response: holds back only GET <base>/api/tree (exact pathname) until the referrer has rendered, then releases it and expects the anchor with no navigation. Gates are released and unrouted in finally.

CI (e2e/playwright.config.ts, .github/workflows/e2e.yml)

  • trace: 'on-first-retry' → 'retain-on-failure'. No retries are configured, so the old setting never recorded a trace.
  • New if: failure() step uploads e2e/test-results/ with the already pinned actions/upload-artifact@043fb46… (v7.0.1), retention-days: 1. The test step's exit status is unchanged.
  • Note: traces contain the hard-coded E2E test password from e2e/run.sh and session cookies of the throwaway test server. They contain no real credentials.

No backend, auth, flag, schema or dependency change; no timeout increase, retry or skip.

How I tested it

  • RED before the fix: the six new Vitest tests failed on their post-transition assertions. The new late-tree E2E test failed ("element(s) not found") with the old MarkdownLink.tsx temporarily restored, and passes with the fix.
  • Frontend: npm test passes 727/727. npm run build and npm run lint -- --max-warnings=0 are clean.
  • E2E, local mode (Windows): --grep 'incoming-link' --repeat-each 3 passed 6/6. --grep 'move-page-updates-incoming-links-via-refactor-api' --repeat-each 20 with E2E_MAX_FAILURES=0 passed 20/20. Full one-worker suite: 206 passed, 3 skipped, 0 failed.
  • E2E, Docker mode (LF checkout): full suite 206 passed, 3 skipped, 0 failed.
  • E2E lint and format: eslint and prettier --check are clean on an LF checkout.
  • Artifacts: a deliberately failing probe (reverted, not committed) produced trace.zip, a screenshot, a video and error-context.md under e2e/test-results/.
  • Versions: Node 24.14.1, npm 11.11.0, Go 1.27.2, Playwright 1.63.0 with Chromium.

Known, separate: in one earlier 20-repeat run, the new pre-move checkpoint failed once in 33 executions. The referrer had been saved without the typed link. EditPage.savePage treats a save button that never becomes enabled as "nothing to save" and returns without saving. The old test would also have failed in that case, at its preview poll, so this is a setup flake in the shared helper and not the CI failure from #1683. Why the typed text was missing is not diagnosed. Not changed here; it can be tracked separately.

This PR fixes the late-tree subscription defect it demonstrates. It does not claim that every intermittent failure of this test is gone.

Manual test checklist (still open):

  • Move a page that has incoming links, with link rewriting on. In the viewer, the referrer's link points to the new path and navigates there.
  • Same in the editor's live preview.
  • A link to a missing page: red create-page button when signed in, red text with a tooltip when read-only.
  • Delete or move a target while the referrer is open. After the tree refreshes, the link no longer navigates.
  • Asset links (/assets/...) and external links open in a new tab; same-origin full URLs stay in the current tab.
  • Repeat the viewer checks as a viewer-role or public read-only user.

Checklist

  • I discussed the approach on the linked issue before starting (or this is a trivial fix: typo, docs, obvious one-liner)
  • This PR is focused on a single change
  • New behaviour plugs into an extension point; no new if feature branches in shared code (conventions)
  • I ran npm run format in ui/leafwiki-ui (and in e2e if e2e tests changed)
  • I updated documentation if needed (none needed)
  • For UI changes: I tested it manually in the browser (light/dark mode, mobile width where relevant)
  • AI tools were used for this PR (name them: OpenCode with Anthropic Claude models). I have reviewed and understand every line

@zorak1103
zorak1103 requested a review from perber as a code owner October 15, 2026 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

E2E: intermittently missing incoming link after refactor move

2 participants