Skip to content

Commit 37a40ae

Browse files
authored
Merge pull request #1495 from Open-Source-Legal/claude/fix-annotation-deep-links-ppMBa
Fix annotation deep-links from corpus-home Table of Contents
2 parents f2472cb + c3ff9e9 commit 37a40ae

4 files changed

Lines changed: 197 additions & 2 deletions

File tree

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3131

3232
### Fixed
3333

34+
- **Annotation deep-links from the corpus-home Table of Contents silently no-op'd** (`frontend/src/components/corpuses/DocumentAnnotationIndex.tsx`, `frontend/src/components/knowledge_base/document/document_kb/RightPanelContent.tsx`): clicking a structural section in the corpus-home document index (e.g. "Subchapter I. Formation, p. 2") was supposed to open the document with the annotation pre-selected and scrolled into view. Instead it appeared to do nothing. Root cause: `DocumentAnnotationIndex` overloaded a single `embedded` prop with two semantics — visual layout ("render without an outer container") *and* click routing ("we are already on the document page, just rewrite `?ann=`"). The corpus-home call site (`DocumentTableOfContents.tsx:919`) needed the visual flavor but absolutely was *not* on a document page, so `handleSectionClick` took the wrong branch and wrote `?ann=<id>` onto the corpus URL — no navigation, no scroll. Fix splits the prop: `embedded` is now purely visual, and a new explicit `onDocumentPage` prop controls click routing. The single call site that's actually on a document page (`RightPanelContent.tsx`) opts in. Regression test in `frontend/src/components/corpuses/__tests__/DocumentAnnotationIndex.test.tsx` pins the new contract: a click from a corpus URL must produce a string-form `navigate("/d/.../doc?ann=<id>")` (full deep link), while a click from a document URL with `onDocumentPage` produces `navigate({ search: "...ann=<id>..." }, { replace: true })`.
35+
3436
- **Zip importer reported `success: True` even when sidecars failed (silent annotation loss)** (`opencontractserver/tasks/import_tasks.py:421`, `:1411`): `_read_sidecar` raises `ValueError` when a sidecar exceeds `ZIP_MAX_SIDECAR_SIZE_BYTES`; malformed JSON, schema failures, and missing labels for sidecar-declared annotations all bump `annotation_sidecars_errored` and append to `errors`. The success determinations only checked `files_errored` (`import_zip_with_folder_structure`) or the user-cap message (`process_documents_zip`), so callers observed `success: True, completed: True` while annotations were silently dropped — exactly the silent data-loss path called out in PR #1489 review feedback. `import_zip_with_folder_structure` now requires `annotation_sidecars_errored == 0` in addition to the existing `files_errored == 0` and user-cap check; `process_documents_zip` now requires `error_files == 0` in addition to the user-cap check. `relationship_errors` is intentionally not folded in — those are surfaced separately via `relationships_skipped` + `relationship_errors` and the documents themselves are imported correctly. Two tests in `test_sidecar_import.py` (`test_skip_pipeline_without_labels_json`) and a new regression test (`test_sidecar_error_drops_overall_success_flag`) lock down the new contract.
3537

3638
- **`_create_analysis_notification` was reading a non-existent field on `Analyzer`** (Issue #1471, `opencontractserver/analyzer/views.py:50`). The notification builder for completed/failed analyses was reading `analysis.analyzer.analyzer_id`, but `Analyzer` has no such field — its primary key is `id` (a `CharField(max_length=1024, primary_key=True)`). On a real ORM instance every successful or failed remote-analyzer notification raised `AttributeError` inside the builder before the `Notification` row was written. Production tests in `opencontractserver/tests/test_job_notifications.py` masked the bug by mocking the entire `Analysis` instance with `MagicMock()` and setting `analysis.analyzer.analyzer_id = "test-analyzer"` (MagicMock auto-creates any attribute on access). Fix replaces the bad attribute with `analysis.analyzer.id`. A follow-up integration test using a real `Analyzer` instance is tracked in the issue.

frontend/src/components/corpuses/DocumentAnnotationIndex.tsx

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,14 @@ interface DocumentAnnotationIndexProps {
8484
maxDepth?: number;
8585
/** When true, renders without outer container (for embedding in tabs) */
8686
embedded?: boolean;
87+
/**
88+
* When true, the component is being rendered inside the document page
89+
* itself, so a section click should only update the `?ann=` selection on
90+
* the current URL. When false (the default), a click navigates to the
91+
* document URL with the selection preset — required for corpus-level
92+
* tables of contents that deep-link into a document.
93+
*/
94+
onDocumentPage?: boolean;
8795
/** Case-insensitive filter applied to section titles and descriptions */
8896
filterQuery?: string;
8997
}
@@ -407,6 +415,7 @@ export const DocumentAnnotationIndex: React.FC<
407415
corpusId,
408416
maxDepth = DOCUMENT_ANNOTATION_INDEX_MAX_DEPTH,
409417
embedded = false,
418+
onDocumentPage = false,
410419
filterQuery,
411420
}) => {
412421
const navigate = useNavigate();
@@ -624,9 +633,8 @@ export const DocumentAnnotationIndex: React.FC<
624633
}
625634
}, [filterQuery, allNodeIds]);
626635

627-
// Handle section click - select annotation via URL params (routing mantra)
628636
const handleSectionClick = (node: SectionNode) => {
629-
if (embedded) {
637+
if (onDocumentPage) {
630638
// Already on the document page — update ?ann= to select the annotation.
631639
// CentralRouteManager will sync selectedAnnotationIds, triggering scroll.
632640
updateAnnotationSelectionParams(location, navigate, {
Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
/**
2+
* Regression tests for DocumentAnnotationIndex deep-linking behavior.
3+
*
4+
* Bug context:
5+
* The corpus home page renders a Table of Contents whose leaves are
6+
* structural annotations (e.g. "Subchapter I. Formation, p. 2"). Clicking
7+
* one is supposed to deep-link into the corresponding document with the
8+
* annotation pre-selected.
9+
*
10+
* The component originally overloaded a single `embedded` prop for two
11+
* meanings: "render without an outer container" AND "we are already on
12+
* the document page". The corpus-home call site needed the visual flavor
13+
* but absolutely was NOT on a document page. The click handler took the
14+
* wrong branch and rewrote `?ann=<id>` onto the corpus URL, so nothing
15+
* happened. These tests pin the new contract: visual layout (`embedded`)
16+
* and click routing (`onDocumentPage`) are independent.
17+
*/
18+
import React from "react";
19+
import { render, screen, waitFor } from "@testing-library/react";
20+
import userEvent from "@testing-library/user-event";
21+
import { MemoryRouter } from "react-router-dom";
22+
import { MockedProvider } from "@apollo/client/testing";
23+
import { describe, it, expect, vi, beforeEach } from "vitest";
24+
25+
import { DocumentAnnotationIndex } from "../DocumentAnnotationIndex";
26+
import {
27+
GET_DOCUMENT_ANNOTATION_INDEX,
28+
GetDocumentAnnotationIndexInput,
29+
} from "../../../graphql/queries";
30+
import {
31+
DOCUMENT_ANNOTATION_INDEX_LIMIT,
32+
OC_SECTION_LABEL,
33+
} from "../../../assets/configurations/constants";
34+
import { openedCorpus } from "../../../graphql/cache";
35+
36+
// Mock react-router-dom's useNavigate so we can assert on navigation intent.
37+
const mockNavigate = vi.fn();
38+
vi.mock("react-router-dom", async () => {
39+
const actual = await vi.importActual<typeof import("react-router-dom")>(
40+
"react-router-dom"
41+
);
42+
return {
43+
...actual,
44+
useNavigate: () => mockNavigate,
45+
};
46+
});
47+
48+
const DOC_ID = "doc-1";
49+
const DOC_SLUG = "test-document";
50+
const CORPUS_ID = "corpus-1";
51+
const SECTION_ID = "ann-section-1";
52+
const SECTION_TITLE = "Subchapter I. Formation";
53+
54+
const corpusForVar = {
55+
id: CORPUS_ID,
56+
slug: "test-corpus",
57+
creator: { id: "u1", slug: "test-user" },
58+
} as Parameters<typeof openedCorpus>[0];
59+
60+
const variables: GetDocumentAnnotationIndexInput = {
61+
documentId: DOC_ID,
62+
corpusId: CORPUS_ID,
63+
labelText: OC_SECTION_LABEL,
64+
first: DOCUMENT_ANNOTATION_INDEX_LIMIT,
65+
};
66+
67+
const indexMock = {
68+
request: { query: GET_DOCUMENT_ANNOTATION_INDEX, variables },
69+
result: {
70+
data: {
71+
annotations: {
72+
totalCount: 1,
73+
edges: [
74+
{
75+
node: {
76+
id: SECTION_ID,
77+
rawText: SECTION_TITLE,
78+
longDescription: null,
79+
page: 2,
80+
parent: null,
81+
},
82+
},
83+
],
84+
},
85+
},
86+
},
87+
};
88+
89+
const renderIndex = (props: {
90+
onDocumentPage?: boolean;
91+
initialEntry: string;
92+
}) => {
93+
return render(
94+
<MockedProvider mocks={[indexMock]} addTypename={false}>
95+
<MemoryRouter initialEntries={[props.initialEntry]}>
96+
<DocumentAnnotationIndex
97+
documentId={DOC_ID}
98+
documentSlug={DOC_SLUG}
99+
corpusId={CORPUS_ID}
100+
embedded
101+
onDocumentPage={props.onDocumentPage}
102+
/>
103+
</MemoryRouter>
104+
</MockedProvider>
105+
);
106+
};
107+
108+
describe("DocumentAnnotationIndex — section click deep-link routing", () => {
109+
beforeEach(() => {
110+
mockNavigate.mockReset();
111+
openedCorpus(corpusForVar);
112+
});
113+
114+
it("navigates to the document URL with ?ann=<id> when clicked from the corpus home (default)", async () => {
115+
renderIndex({ initialEntry: "/c/test-user/test-corpus" });
116+
117+
// Wait for the section to render (data resolved)
118+
const item = await screen.findByRole("treeitem", {
119+
name: new RegExp(SECTION_TITLE, "i"),
120+
});
121+
122+
await userEvent.click(item);
123+
124+
// Should have triggered a full navigation to the document page.
125+
// navigate(targetPath) is called with a string starting with /d/...
126+
await waitFor(() => {
127+
expect(mockNavigate).toHaveBeenCalledTimes(1);
128+
});
129+
130+
const navArg = mockNavigate.mock.calls[0][0];
131+
expect(typeof navArg).toBe("string");
132+
expect(navArg).toMatch(/^\/d\/test-user\/test-corpus\/test-document/);
133+
expect(navArg).toContain(`ann=${SECTION_ID}`);
134+
});
135+
136+
it("only updates ?ann= on the current URL when onDocumentPage is true", async () => {
137+
renderIndex({
138+
onDocumentPage: true,
139+
initialEntry: "/d/test-user/test-corpus/test-document",
140+
});
141+
142+
const item = await screen.findByRole("treeitem", {
143+
name: new RegExp(SECTION_TITLE, "i"),
144+
});
145+
146+
await userEvent.click(item);
147+
148+
await waitFor(() => {
149+
expect(mockNavigate).toHaveBeenCalledTimes(1);
150+
});
151+
152+
// Search-only navigation: navigate({ search: "..." }, { replace: true })
153+
const [navArg, navOpts] = mockNavigate.mock.calls[0];
154+
expect(navArg).toEqual({
155+
search: expect.stringContaining(`ann=${SECTION_ID}`),
156+
});
157+
expect(navOpts).toEqual({ replace: true });
158+
});
159+
160+
it("does NOT update ?ann= on the current URL when on the corpus home (regression: embedded must not imply onDocumentPage)", async () => {
161+
renderIndex({
162+
// embedded=true is set by renderIndex, but onDocumentPage is unset
163+
initialEntry: "/c/test-user/test-corpus",
164+
});
165+
166+
const item = await screen.findByRole("treeitem", {
167+
name: new RegExp(SECTION_TITLE, "i"),
168+
});
169+
170+
await userEvent.click(item);
171+
172+
await waitFor(() => {
173+
expect(mockNavigate).toHaveBeenCalledTimes(1);
174+
});
175+
176+
// The single navigate call must be a full URL string (not a {search}
177+
// object), proving we did NOT take the "already on the document page"
178+
// branch that originally swallowed the deep link.
179+
const navArg = mockNavigate.mock.calls[0][0];
180+
expect(typeof navArg).toBe("string");
181+
expect(navArg).not.toMatch(/^\/c\//); // must leave the corpus URL
182+
expect(navArg).toMatch(/^\/d\//);
183+
});
184+
});

frontend/src/components/knowledge_base/document/document_kb/RightPanelContent.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,7 @@ export const RightPanelContent: React.FC<RightPanelContentProps> = ({
166166
documentId={documentId}
167167
corpusId={corpusId}
168168
embedded
169+
onDocumentPage
169170
/>
170171
</ScrollableFillPanel>
171172
</FlexColumnPanel>

0 commit comments

Comments
 (0)