Skip to content

Commit cd7984c

Browse files
authored
Merge pull request #1297 from Open-Source-Legal/claude/add-tests-issue-1280-8RzXd
Add comprehensive component tests for ModernDocumentItem and DocumentRelationshipModal
2 parents 297d9cb + c55766e commit cd7984c

9 files changed

Lines changed: 1793 additions & 78 deletions

CHANGELOG.md

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

2929
### Fixed
3030

31+
- **`DocumentRelationshipModal` "Create label" button never appeared** (Issue #1280, `frontend/src/components/documents/DocumentRelationshipModal.tsx`): `@os-legal/ui`'s Dropdown only fires `onSearchChange` in `searchable="async"` mode — with `searchable="local"` the parent's `labelSearchTerm` state never updated, so the `labelSearchTerm`-gated "Create label: ..." empty-state button was permanently hidden. Switched the Dropdown to `async`, moved option filtering into a `useMemo(filteredRelationshipLabels, [relationLabels, hasCorpus, labelSearchTerm])`, and added the missing dep. Now typing a novel label name surfaces the Create button as designed.
3132
- **`RelationGroup.updateForAnnotationDeletion` pre-filter length check** (Issue #1288): `frontend/src/components/annotator/types/annotations.ts:49-50` computed `nowSourceEmpty` / `nowTargetEmpty` from the pre-filter `this.sourceIds` / `this.targetIds`, so the "now empty" conditions were identical to the "before" conditions and the pruning branches that return `undefined` were dead code. Deleting the sole source or sole target of a relation left the relation orphaned, pointing at a deleted annotation id. Fixed by reading from the post-filter `newSourceIds` / `newTargetIds`. Called from `PdfAnnotations.undoAnnotation()`, so undo now properly drops any relation whose last source or target was the popped annotation. New regression tests under `RelationGroup > .updateForAnnotationDeletion()` cover all four pruning branches plus the survive-with-updated-ids and unchanged cases, and the existing `undoAnnotation` test that pinned the wrong behaviour was corrected.
3233
- Additionally fixed a surviving-relation identity drop in the same method: the post-prune return previously constructed `new RelationGroup(newSourceIds, newTargetIds, this.label)` without forwarding `this.id` / `this.structural`, so relations that merely lost a member were silently reassigned a fresh uuid and had their `structural` flag cleared. Both fields are now preserved, and the survival tests assert `updated!.id === rel.id` (plus `structural === true` where applicable) to pin the behaviour.
3334

@@ -37,6 +38,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3738

3839
### Added
3940

41+
- **Component test coverage for `ModernDocumentItem` and `DocumentRelationshipModal`** (Issue #1280): Added ~1,600 lines of Playwright CT coverage for two high-traffic document components that were lacking happy-path tests.
42+
- `frontend/tests/ModernDocumentItem.ct.tsx` — New 750-line suite covering card/list view rendering (thumbnails, badges, metadata), selection state, permission-gated action buttons (CAN_UPDATE / read-only), action behaviors (view, edit, remove, open, download), backend-locked processing state, context menu actions, and version-history / relationship badges.
43+
- `frontend/tests/DocumentRelationshipModal.ct.tsx` — Extended suite now covers state transitions (moves / removes across columns), Add Source/Target search flow, label picker with pre-populated labels, label change, RELATIONSHIP and NOTES submit mutations, mutation failure handling, inline create-label flow, `SMART_LABEL_SEARCH_OR_CREATE`, the missing-corpus error state, and the useMemo filter bodies (`availableDocuments`, `filteredRelationshipLabels`).
44+
- `frontend/tests/DocumentRelationshipModalTestWrapper.tsx` — Added `makeMockRelationLabel` helper, `extraMocks` prop for per-test Apollo mocks, `relationLabels` / `withoutLabelset` / `corpusIdOverride` props, and a DRY `buildDocumentsMock()` factory (replaces duplicated mock blocks).
45+
- `frontend/tests/utils/ReactiveVarObserver.tsx` — New helper that exposes Apollo reactive-var state via DOM data attributes so Playwright assertions can observe cross-process (browser-side) state without reaching into the Node fixture.
46+
- Added workarounds for DndContext pointer-event interception (`clickViaReact` / `openContextMenu` using React 18 `__reactProps$` fiber access) with explicit upgrade-risk comments.
4047
- **Route component and ExtractDetail view test coverage** (Issue #1285): Closed coverage gaps for zero-coverage route wrappers and the 7%-covered `ExtractDetail.tsx` orchestrator.
4148
- `frontend/src/components/routes/__tests__/ExtractDetailRoute.test.tsx` — 6 vitest specs covering missing-ID, reactive-var reuse, loading, error, not-found, and success paths for the slug-resolving extract route.
4249
- `frontend/src/components/routes/__tests__/ExtractLandingRoute.test.tsx` — 4 specs for the legacy `/e/:user/:extract` route, including the redirect to `/extracts/:id` when the reactive var is populated.

CLAUDE.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -561,7 +561,8 @@ Run manually: `pre-commit run --all-files`
561561
13. **Writing sync agent tools**: All agent tools must be async. The `PydanticAIToolWrapper` accepts sync functions but calls them directly (no thread pool) — sync Django ORM calls will raise `SynchronousOnlyOperation`. Use the `a`-prefixed async versions in `core_tools.py`.
562562
14. **PydanticAI `system_prompt` silently dropped**: When creating `PydanticAIAgent`, use `instructions=` NOT `system_prompt=`. The `system_prompt` parameter is only included when `message_history` is `None`, but OpenContracts' `chat()` persists a HUMAN message before calling pydantic-ai's `run()`, so history is always non-empty. This causes the system prompt to be silently dropped. See `docs/architecture/llms/README.md` for full details.
563563
15. **Apollo cache `keyArgs` must use field argument names, not variable names**: In `cache.ts`, `relayStylePagination(["corpus", "name_Contains"])` uses the GraphQL **field argument** names (e.g., `extracts(corpus: $id, name_Contains: $text)`), NOT the variable names (`$id`, `$text`). Mismatched keyArgs silently fail to isolate cache entries, causing queries with different filters to share stale cached results.
564-
16. **Corrupted Docker iptables chains** (RARE): If you see `Chain 'DOCKER-ISOLATION-STAGE-2' does not exist` errors, Docker's iptables chains have been corrupted during docker cycling. Run this nuclear fix:
564+
16. **Playwright CT split-import rule**: In `frontend/tests/*.ct.tsx`, keep JSX-component imports in their own `import { Wrapper } from "./Wrapper"` statement, separate from any helper/constant imports from the same file. Playwright CT's babel transform only rewrites component references into `importRefs` when every specifier in the statement is a JSX component; mixing a component with helpers leaves the component unrewritten and `mount()` throws. Do not let auto-import organisers merge them.
565+
17. **Corrupted Docker iptables chains** (RARE): If you see `Chain 'DOCKER-ISOLATION-STAGE-2' does not exist` errors, Docker's iptables chains have been corrupted during docker cycling. Run this nuclear fix:
565566
```bash
566567
sudo systemctl stop docker && sudo systemctl stop docker.socket && sudo ip link delete docker0 2>/dev/null || true && sudo iptables -t nat -F && sudo iptables -t nat -X && sudo iptables -t filter -F && sudo iptables -t filter -X 2>/dev/null || true && sudo iptables -t mangle -F && sudo iptables -t mangle -X && sudo iptables -t filter -N INPUT 2>/dev/null || true && sudo iptables -t filter -N FORWARD 2>/dev/null || true && sudo iptables -t filter -N OUTPUT 2>/dev/null || true && sudo iptables -P INPUT ACCEPT && sudo iptables -P FORWARD ACCEPT && sudo iptables -P OUTPUT ACCEPT && sudo systemctl start docker
567568
```
-7.98 KB
Loading

frontend/src/components/documents/DocumentRelationshipModal.tsx

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,39 @@ import {
4646
import { ErrorMessage, WarningMessage } from "../widgets/feedback";
4747
import { OS_LEGAL_COLORS } from "../../assets/configurations/osLegalStyles";
4848

49+
// ============================================================================
50+
// HELPERS
51+
// ============================================================================
52+
53+
type LabelLike = {
54+
id: string;
55+
text?: string | null;
56+
labelType?: string | null;
57+
};
58+
59+
/**
60+
* Filter a list of labels down to relationship-typed labels that match
61+
* `searchTerm`. When `hasCorpus` is false, returns an empty array (relationship
62+
* labels only make sense inside a corpus). An empty `searchTerm` returns all
63+
* relationship labels unfiltered. Case-insensitive substring match on `text`.
64+
*/
65+
export function filterRelationshipLabels<T extends LabelLike>(
66+
labels: T[] | null | undefined,
67+
searchTerm: string,
68+
hasCorpus: boolean
69+
): T[] {
70+
if (!hasCorpus) return [];
71+
const relationshipOnly =
72+
labels?.filter(
73+
(label) => label.labelType === LabelType.RelationshipLabel
74+
) || [];
75+
if (!searchTerm) return relationshipOnly;
76+
const needle = searchTerm.toLowerCase();
77+
return relationshipOnly.filter((label) =>
78+
(label.text ?? "").toLowerCase().includes(needle)
79+
);
80+
}
81+
4982
// ============================================================================
5083
// TYPES
5184
// ============================================================================
@@ -396,18 +429,11 @@ export const DocumentRelationshipModal: React.FC<
396429
return allDocuments.filter((doc) => !usedIds.has(doc.id));
397430
}, [allDocuments, sourceIds, targetIds]);
398431

399-
// Get all relationship labels (Dropdown's search prop handles filtering)
400-
const filteredRelationshipLabels = useMemo(() => {
401-
if (!hasCorpus) {
402-
return [];
403-
}
404-
405-
return (
406-
relationLabels?.filter(
407-
(label) => label.labelType === LabelType.RelationshipLabel
408-
) || []
409-
);
410-
}, [relationLabels, hasCorpus]);
432+
// Manual filter keeps `labelSearchTerm` in sync — @os-legal/ui's Dropdown only fires `onSearchChange` in `async` mode.
433+
const filteredRelationshipLabels = useMemo(
434+
() => filterRelationshipLabels(relationLabels, labelSearchTerm, hasCorpus),
435+
[relationLabels, hasCorpus, labelSearchTerm]
436+
);
411437

412438
// Get selected label info
413439
const selectedLabel = useMemo(() => {
@@ -986,7 +1012,7 @@ export const DocumentRelationshipModal: React.FC<
9861012
mode="select"
9871013
placeholder="Search or type to create..."
9881014
fluid
989-
searchable="local"
1015+
searchable="async"
9901016
options={filteredRelationshipLabels.map((label) => ({
9911017
value: label.id,
9921018
label: label.text || "",
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
import { describe, it, expect } from "vitest";
2+
import { filterRelationshipLabels } from "../DocumentRelationshipModal";
3+
import { LabelType } from "../../../types/graphql-api";
4+
5+
const makeLabel = (
6+
overrides: Partial<{
7+
id: string;
8+
text: string | null;
9+
labelType: string;
10+
}> = {}
11+
) => ({
12+
id: "l-1",
13+
text: "references",
14+
labelType: LabelType.RelationshipLabel,
15+
...overrides,
16+
});
17+
18+
describe("filterRelationshipLabels", () => {
19+
it("returns empty list when hasCorpus is false", () => {
20+
const labels = [makeLabel({ id: "l-1", text: "references" })];
21+
expect(filterRelationshipLabels(labels, "", false)).toEqual([]);
22+
});
23+
24+
it("returns empty list for null/undefined labels", () => {
25+
expect(filterRelationshipLabels(null, "", true)).toEqual([]);
26+
expect(filterRelationshipLabels(undefined, "", true)).toEqual([]);
27+
});
28+
29+
it("keeps only RelationshipLabel typed labels", () => {
30+
const labels = [
31+
makeLabel({ id: "l-1", text: "references" }),
32+
makeLabel({ id: "l-2", text: "header", labelType: LabelType.TokenLabel }),
33+
makeLabel({ id: "l-3", text: "amends" }),
34+
];
35+
const out = filterRelationshipLabels(labels, "", true);
36+
expect(out.map((l) => l.id)).toEqual(["l-1", "l-3"]);
37+
});
38+
39+
it("returns all relationship labels unfiltered when searchTerm is empty", () => {
40+
const labels = [
41+
makeLabel({ id: "l-1", text: "references" }),
42+
makeLabel({ id: "l-2", text: "amends" }),
43+
];
44+
expect(filterRelationshipLabels(labels, "", true)).toHaveLength(2);
45+
});
46+
47+
it("filters by case-insensitive substring match on text", () => {
48+
const labels = [
49+
makeLabel({ id: "l-1", text: "References" }),
50+
makeLabel({ id: "l-2", text: "amends" }),
51+
makeLabel({ id: "l-3", text: "supersedes" }),
52+
];
53+
const out = filterRelationshipLabels(labels, "REF", true);
54+
expect(out.map((l) => l.id)).toEqual(["l-1"]);
55+
});
56+
57+
it("treats null text as empty string (does not match any needle)", () => {
58+
const labels = [
59+
makeLabel({ id: "l-1", text: null }),
60+
makeLabel({ id: "l-2", text: "references" }),
61+
];
62+
const out = filterRelationshipLabels(labels, "ref", true);
63+
expect(out.map((l) => l.id)).toEqual(["l-2"]);
64+
});
65+
66+
it("returns empty array when no relationship labels match needle", () => {
67+
const labels = [makeLabel({ id: "l-1", text: "references" })];
68+
expect(filterRelationshipLabels(labels, "zzz", true)).toEqual([]);
69+
});
70+
});

0 commit comments

Comments
 (0)