Fix annotation deep-links from corpus-home Table of Contents

[JSv4]

Summary

Fixed a bug where clicking structural sections in the corpus-home Table of Contents (e.g., "Subchapter I. Formation, p. 2") would silently fail to navigate to the document. The issue was caused by overloading the embedded prop to mean both "render without outer container" AND "we are already on the document page", causing the click handler to take the wrong routing branch.

Key Changes

• Separated concerns in DocumentAnnotationIndex: Introduced a new onDocumentPage prop to explicitly indicate whether the component is being rendered inside the document page itself, independent of the embedded visual layout prop

  • embedded: Controls visual rendering (with/without outer container)
  • onDocumentPage: Controls click routing behavior (update current URL vs. navigate to document)

• Updated click handler logic: Changed handleSectionClick to use onDocumentPage instead of embedded to determine routing behavior:

  • When onDocumentPage=true: Updates ?ann= parameter on current URL (for in-document selection)
  • When onDocumentPage=false (default): Navigates to the document URL with ?ann= preset (for corpus-level deep-linking)

• Updated call sites:

  • RightPanelContent.tsx: Added onDocumentPage={true} when rendering the index inside the document page

• Added comprehensive regression tests: New test file DocumentAnnotationIndex.test.tsx with three test cases covering:

  • Deep-linking from corpus home (default behavior)
  • In-document annotation selection (onDocumentPage=true)
  • Regression check ensuring embedded no longer implies onDocumentPage

Implementation Details

The root cause was that the corpus-home Table of Contents call site needed the embedded visual styling but was NOT on a document page. The original code conflated these two concerns, causing clicks to attempt updating ?ann= on the corpus URL instead of navigating to the document. The fix makes the routing intent explicit through a dedicated prop while preserving backward compatibility (onDocumentPage defaults to false).

https://claude.ai/code/session_01HFfDk339uN1JjJGv7eqQXr

[claude]

Code Review

Overview

This is a clean, minimal fix for a silent no-op bug in the corpus-home Table of Contents. The root cause — embedded doing double duty as both a visual flag and a routing flag — is accurately diagnosed, and the solution (introducing onDocumentPage) correctly separates those two concerns. Backward compatibility is preserved since onDocumentPage defaults to false.

---

What's Good

• Correct root cause analysis. The corpus-home call site in DocumentTableOfContents.tsx:914 passes embedded for visual reasons but is not on a document page. Before this fix, that was enough to silently swallow the navigation. The new prop makes intent explicit at both call sites.
• Minimal diff. Only three files change in the production layer (component interface, component body, one call site). No collateral edits.
• Regression tests cover the contract precisely. Three test cases pin the exact distinction that caused the bug:
  1. Default (corpus home) → full navigate("/d/...") string
  2. onDocumentPage=true → navigate({ search: "...ann=..." }, { replace: true })
  3. Regression guard: embedded=true + no onDocumentPage still produces a full navigation, not a search-only rewrite
• CHANGELOG entry is thorough, includes file paths and line numbers, and describes both the failure mode and the fix.

---

Issues / Suggestions

Minor: Verbose comment in handleSectionClick

Per CLAUDE.md conventions, comments should only appear when the WHY is non-obvious. The four-line block added above handleSectionClick describes the historical bug, which belongs in the commit message and CHANGELOG rather than the source file. A future reader who doesn't know the bug history will find the function name onDocumentPage self-explanatory. Consider trimming to one line, e.g.:

// `onDocumentPage` is independent of `embedded`; see CHANGELOG for background.
const handleSectionClick = (node: SectionNode) => {

Observation: openedCorpus() in the non-onDocumentPage branch

handleSectionClick calls openedCorpus() (an Apollo reactive variable) in the else branch:

const corpus = openedCorpus();
navigateToRelationshipDocument(
  { id: documentId, title: node.title, slug: documentSlug },
  corpus,
  ...

openedCorpus() can be null if the corpus hasn't been set (e.g., a direct deep-link load). This is a pre-existing issue rather than something this PR introduces, but the new onDocumentPage=false code path is now the default, so any future call site that forgets to set openedCorpus before rendering will silently navigate to a broken URL. Worth tracking in an issue even if not blocking this PR.

Test: Shared mock is reused without duplication for refetches

Each test renders a fresh component that fires the GET_DOCUMENT_ANNOTATION_INDEX query once. The single indexMock in MockedProvider is therefore sufficient. However, if DocumentAnnotationIndex ever adds a refetch on focus/visibility change, all three tests would break simultaneously. Not a blocker — just something to keep in mind if the component evolves.

Test location consistency

The new test lives in frontend/src/components/corpuses/__tests__/ (Vitest unit tests). That is the correct location for a @testing-library/react unit test, as opposed to frontend/tests/ (Playwright component tests). This is fine, but worth noting that the CLAUDE.md section on "Frontend Component Tests" refers to the Playwright path, so reviewers skimming that section might be confused. No action required.

---

Summary

This is a well-scoped, correct fix. The only actionable suggestion is trimming the verbose comment in handleSectionClick; everything else is minor or pre-existing. Ready to merge once any comment-style preference is resolved.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review

Overview

This PR fixes a real bug: clicking structural sections in the corpus-home Table of Contents silently no-op'd because DocumentAnnotationIndex overloaded embedded to mean both "render without a container" and "we are on the document page." The corpus-home call site in DocumentTableOfContents.tsx used embedded for the visual flavor but was not on a document page, causing handleSectionClick to rewrite ?ann= onto the corpus URL instead of navigating to the document.

The fix is clean and minimal.

---

Strengths

• Root cause is precisely identified and fixed. The prop split (embedded = visual layout, onDocumentPage = routing intent) removes the overloaded semantics permanently.
• Backward compatible. onDocumentPage defaults to false, so all existing callers that did not pass it (like DocumentTableOfContents.tsx:914) correctly get the navigate-to-document behavior without any changes.
• Call sites are complete. DocumentTableOfContents.tsx (the broken call site) now implicitly gets the correct default, and RightPanelContent.tsx (the in-document call site) explicitly opts in with onDocumentPage.
• Regression tests are well-structured. Three focused test cases, including a dedicated regression case verifying that embedded=true no longer implies onDocumentPage. Using MemoryRouter + mocked useNavigate is the right approach for routing assertions.
• JSDoc on the new prop accurately describes both behaviors and gives a "why" — exactly the right level of documentation.

---

Minor Issues

1. window.location.pathname vs location.pathname (pre-existing, but worth flagging)

In the else branch of handleSectionClick (~line 646), the code passes window.location.pathname to navigateToRelationshipDocument. The onDocumentPage branch directly above it uses location from useLocation(). Using window.location.pathname will disagree with MemoryRouter state in tests. This is pre-existing code not introduced here, but since this function is being touched it is an easy fix — swap in location.pathname for consistency.

2. openedCorpus reactive var is an implicit test dependency

The tests correctly set openedCorpus(corpusForVar) in beforeEach, but there is no corresponding teardown. If another test module sets openedCorpus to a different value and runs before this suite, the navigation URL assertions could produce unexpected slugs. Consider adding an afterEach that resets it to null.

---

What is Not Needed Here

• No other call sites exist for DocumentAnnotationIndex beyond the two already handled (DocumentTableOfContents.tsx, RightPanelContent.tsx), so the change is complete.
• No security or performance concerns.

---

Summary

Solid, focused bug fix. The prop split is the right design, the tests are correct, and the CHANGELOG entry is thorough. The two items above are minor — the window.location one is pre-existing tech debt, and the openedCorpus teardown is a nice-to-have for test isolation. Neither is a blocker.