Repository navigation
test: increase test coverage to 80% - #95
Merged
Merged
Conversation
Coverage improvement: - Overall: 5% → 11% statements - API clients: 12% → 50% (5 new test files) - Hooks: 3% → 18% (3 new/extended test files) - Components: 0% → partial (4 new test files) New test files: - hooks: useProject (22), useOntologyTree (18), useFilteredTree (+11) - API: projects (20), revisions (14), pullRequests (22), lint (12), suggestions (14) - Components: Button (14), ModeSwitcher (8), ShareButton (15), ProjectCard (13) All 295 tests pass. Closes #94 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 35 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR significantly expands test coverage by adding ~155 comprehensive test files covering hooks, API clients, components, stores, and utilities using Vitest and React Testing Library. Additionally, it updates Node.js to version 24, adds Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes This is a massive test suite addition (~12,000+ lines) with heterogeneous coverage across hooks, API clients, and components spanning multiple domains. While individual test files follow consistent patterns (mocking, rendering, assertion), each requires careful verification that mocks are correct, behavior assertions are meaningful, and edge cases are handled properly. The breadth of changes (155+ test files) and mix of simple and complex test scenarios (from button clicks to complex async flows, WebSocket reconnection, drag-and-drop) necessitate thorough review to ensure coverage quality and test reliability. Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
17 new test files covering all remaining hooks: - useAutoSave (12), useEntityAutoSave (13), useCollaborationStatus (7) - useRemoteSync (9), useSuggestionSession (14), useSuggestionBeacon (9) - useKeyboardShortcuts (14), useGraphData (7), useTreeDragDrop (20) - useCrossReferences (8), useEntityHistory (8), useSimilarEntities (8) - useSemanticSearch (8), useProjectAnalytics (15), useIriLabels (8) - useNotifications (8), useTreeSearch (9) Coverage: hooks 18% → 78% stmts / 81% lines, overall 11% → 22% All 480 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (6)
__tests__/lib/api/pullRequests.test.ts (2)
92-96: Consider adding error path tests for other critical endpoints.Only
listhas an error test. While all endpoints use the sameapiobject which throwsApiErrorconsistently (perlib/api/client.ts), adding error tests for at least one write operation (e.g.,create,merge) would strengthen coverage of error handling in mutation paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/api/pullRequests.test.ts` around lines 92 - 96, Add a test that exercises a write/mutation path (e.g., pullRequestsApi.create or pullRequestsApi.merge) to ensure it throws ApiError on server errors: use the existing mockError(...) helper to simulate a failure (e.g., mockError(403, "Forbidden", "Access denied")), then call the chosen method (for example pullRequestsApi.create with minimal valid input) and assert with await expect(...).rejects.toThrow(ApiError); reuse the same test structure as the existing "throws ApiError on failure" list test to keep consistency.
340-348: Minor: URL assertion missing in update test.Unlike other tests in this file, the
updatetest doesn't verify the URL. Consider adding a URL assertion for consistency.describe("update", () => { it("calls PATCH /github-integration", async () => { mockOk({ id: "gi1" }); await githubIntegrationApi.update("p1", { sync_enabled: true }, "tok"); - const [, options] = mockFetch.mock.calls[0]; + const [url, options] = mockFetch.mock.calls[0]; + expect(url).toContain("/api/v1/projects/p1/github-integration"); expect(options.method).toBe("PATCH"); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/api/pullRequests.test.ts` around lines 340 - 348, The test for githubIntegrationApi.update is missing a URL assertion: after calling githubIntegrationApi.update("p1", { sync_enabled: true }, "tok") add an expectation on mockFetch's first call argument (e.g. const [url] = mockFetch.mock.calls[0]; expect(url).toBe("/github-integration/p1")) so the test verifies the correct endpoint was used; keep the existing method assertion for PATCH and place the URL assertion just before or alongside it.__tests__/components/ui/button.test.tsx (2)
105-110: Consider usinguserEventfor more realistic click simulation.The native
.click()method works, but@testing-library/user-eventprovides more realistic event simulation (proper focus handling, event ordering). This is a minor improvement.♻️ Suggested improvement using userEvent
-import { describe, expect, it, vi } from "vitest"; -import { render, screen } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; import { createRef } from "react"; import { Button } from "@/components/ui/button";// ── onClick ─────────────────────────────────────────────────────── - it("calls onClick handler when clicked", () => { + it("calls onClick handler when clicked", async () => { const handleClick = vi.fn(); render(<Button onClick={handleClick}>Click Me</Button>); - screen.getByRole("button").click(); + await userEvent.click(screen.getByRole("button")); expect(handleClick).toHaveBeenCalledOnce(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/button.test.tsx` around lines 105 - 110, The test uses the DOM .click() method; replace it with `@testing-library/user-event` to simulate a real user: import userEvent from '@testing-library/user-event', make the test async, and replace screen.getByRole("button").click() with await userEvent.click(screen.getByRole("button")); keep the same handleClick mock and assertion (handleClick, Button, and screen.getByRole are the symbols to update).
75-85: Consider adding a test that verifies disabled buttons don't trigger onClick.The disabled tests correctly check the
disabledattribute and styling classes, but there's no behavioral test confirming that clicks on a disabled button don't invoke the handler.🧪 Suggested additional test
it("applies disabled styling classes", () => { render(<Button disabled>Disabled</Button>); const btn = screen.getByRole("button"); expect(btn.className).toContain("disabled:pointer-events-none"); expect(btn.className).toContain("disabled:opacity-50"); }); + + it("does not call onClick when disabled", () => { + const handleClick = vi.fn(); + render(<Button disabled onClick={handleClick}>Disabled</Button>); + screen.getByRole("button").click(); + expect(handleClick).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/button.test.tsx` around lines 75 - 85, Add a behavioral test to ensure a disabled Button doesn't call its click handler: in the same test file create a test (e.g., "does not call onClick when disabled") that renders <Button disabled onClick={mockFn}> with mockFn = jest.fn(), use fireEvent.click or userEvent.click on screen.getByRole("button"), and assert mockFn was not called; reference the Button component and existing tests that use screen.getByRole("button") to locate where to add this new test.__tests__/components/projects/project-card.test.tsx (1)
106-115: Strengthen date assertions to actually validate updated-vs-created fallback.Both tests only assert
Updated.*2024, so the fallback test can pass without proving the component pickedcreated_atwhenupdated_atis absent.♻️ Suggested tightening
+import { formatDate } from "@/lib/utils"; @@ it("shows the updated date", () => { - render(<ProjectCard project={makeProject()} />); - // formatDate produces locale-specific output but will contain "2024" - expect(screen.getByText(/Updated.*2024/)).toBeDefined(); + const project = makeProject({ + created_at: "2024-06-01T12:00:00Z", + updated_at: "2024-06-15T12:00:00Z", + }); + render(<ProjectCard project={project} />); + expect(screen.getByText(`Updated ${formatDate(project.updated_at!)}`)).toBeDefined(); }); @@ it("falls back to created_at when updated_at is absent", () => { - render(<ProjectCard project={makeProject({ updated_at: undefined })} />); - expect(screen.getByText(/Updated.*2024/)).toBeDefined(); + const project = makeProject({ + created_at: "2024-06-01T12:00:00Z", + updated_at: undefined, + }); + render(<ProjectCard project={project} />); + expect(screen.getByText(`Updated ${formatDate(project.created_at)}`)).toBeDefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/projects/project-card.test.tsx` around lines 106 - 115, The test asserts only /Updated.*2024/ so the fallback case doesn't prove ProjectCard used created_at when updated_at is missing; update the tests to use makeProject with explicit distinct created_at and updated_at values and assert the rendered "Updated" text matches the expected date string (or formatted output) for each case: for the first test pass an explicit updated_at and assert that exact formatted updated_at appears, and for the fallback test omit updated_at but set created_at to a different known date and assert that the rendered "Updated" text equals that created_at value; reference ProjectCard, makeProject, updated_at and created_at to locate the changes.__tests__/components/editor/ModeSwitcher.test.tsx (1)
45-46: Add@testing-library/user-eventto dependencies and useuserEvent.click()for interaction tests.Direct DOM
.click()skips parts of the real user interaction flow.userEventmodels actual user behavior (triggering the proper event sequence), improving test fidelity. Interactions should beawaited to ensure events have settled before assertions.♻️ Proposed update
import { describe, expect, it, vi, beforeEach } from "vitest"; import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; @@ - it("calls setEditorMode with 'developer' when Developer button is clicked", () => { + it("calls setEditorMode with 'developer' when Developer button is clicked", async () => { render(<ModeSwitcher />); - screen.getByRole("button", { name: "Developer" }).click(); + const user = userEvent.setup(); + await user.click(screen.getByRole("button", { name: "Developer" })); expect(mockSetEditorMode).toHaveBeenCalledWith("developer"); }); @@ - it("calls setEditorMode with 'standard' when Standard button is clicked", () => { + it("calls setEditorMode with 'standard' when Standard button is clicked", async () => { mockEditorMode = "developer"; render(<ModeSwitcher />); - screen.getByRole("button", { name: "Standard" }).click(); + const user = userEvent.setup(); + await user.click(screen.getByRole("button", { name: "Standard" })); expect(mockSetEditorMode).toHaveBeenCalledWith("standard"); });Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ModeSwitcher.test.tsx` around lines 45 - 46, Add '@testing-library/user-event' as a dev dependency, import userEvent from '@testing-library/user-event' in ModeSwitcher.test.tsx, and replace direct DOM .click() calls (e.g., screen.getByRole("button", { name: "Developer" }).click()) with awaited userEvent.click(...) calls awaited via await userEvent.click(screen.getByRole(...)); update the same pattern for the other interaction at lines 52-53 and ensure assertions still reference mockSetEditorMode toHaveBeenCalledWith("developer") after awaiting the interaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/editor/ShareButton.test.tsx`:
- Around line 17-24: The test mutates the global navigator.clipboard in
beforeEach (setting writeTextMock) but never restores it, causing cross-test
leakage; capture the original clipboard (e.g., const originalClipboard =
navigator.clipboard) before overwriting in beforeEach, then restore it in an
afterEach by reassigning navigator.clipboard back to originalClipboard, and keep
existing mock clearing (mockSuccess.mockClear(), mockError.mockClear()) so each
test runs with the original navigator.clipboard state.
- Around line 110-115: The clipboard assertions are too loose—enforce a single
clipboard call and verify both conditions on the same copied string: assert
writeTextMock was called exactly once (use writeTextMock
toHaveBeenCalledTimes(1)), then grab the first call's argument
(writeTextMock.mock.calls[0][0]) and assert that this exact string contains
"/projects/proj-1" and does not contain "classIri"; update the assertions in
ShareButton.test.tsx to use writeTextMock.mock.calls[0][0] for the contains /
not-contains checks so both apply to the same call.
In `@__tests__/lib/api/lint.test.ts`:
- Around line 169-176: The test's assertion for subject_iri is too weak; update
the "passes subject_iri filter" test that calls lintApi.getIssues to assert the
full encoded query parameter rather than just "subject_iri=http". Inspect
mockFetch.mock.calls[0][0] (the captured URL), parse or decode its query string
and assert it equals the full value "http://example.org/Class1" (or assert the
URL contains the properly encoded form of that full value) so encoding/encoding
regressions are caught; refer to lintApi.getIssues and mockFetch.mock.calls in
the test to locate where to change the expectation.
- Around line 52-56: The test currently only checks the error class; update the
"throws ApiError on 403" case to also assert the HTTP status on the thrown
ApiError (e.g., assert error.status === 403 or error.metadata.status === 403) so
unrelated ApiError instances won't pass; use the same lintApi.triggerLint("p1",
"tok") call and add a second assertion such as await
expect(lintApi.triggerLint("p1","tok")).rejects.toMatchObject({ status: 403 })
or capture the error with try/catch and assert the status property on the caught
ApiError.
In `@__tests__/lib/api/projects.test.ts`:
- Around line 87-94: The test currently only asserts that projectApi.list()
rejects with ApiError but doesn't verify retries; update the test around
projectApi.list() to also assert the mock fetch was called three times (or equal
to the number of mocked 500 responses) so the retry contract is enforced—after
setting up the three mockError(500, ...) responses, add a call-count assertion
against the fetch mock (e.g., mockFetch or the global fetch mock used in tests)
to ensure projectApi.list() invoked fetch three times during retries.
- Around line 178-194: The test currently only asserts that options.body is a
FormData; update it to verify the multipart fields created by projectApi.import
by reading the FormData entries from the captured request (from
mockFetch.mock.calls[0][1].body) and asserting it contains the expected keys and
values for "file" (with filename/type), "is_public" (stringified boolean), and
"name" (when provided); ensure you reference projectApi.import in the test and
assert the exact entries rather than just the FormData instance so any dropped
field will fail the test.
In `@__tests__/lib/api/suggestions.test.ts`:
- Around line 131-148: The test currently only checks that navigator.sendBeacon
was called with a Blob; update the test for suggestionsApi.beacon to read and
parse the Blob payload and assert the JSON contains the expected session_id and
content keys (e.g., session_id === "s1" and content === "turtle content"), and
also switch the token passed into suggestionsApi.beacon to one containing
reserved characters (e.g., "beacon tok+/%") to assert the URL includes a
properly encoded token; locate the test's mockBeacon and the blob extracted from
mockBeacon.mock.calls[0] to implement these assertions.
In `@__tests__/lib/hooks/useOntologyTree.test.ts`:
- Around line 401-427: The test currently only checks selectedIri and that
getClassAncestors was called; update it to also assert that the ancestor path
was actually expanded and the target attached by inspecting the hook's tree
state after calling useOntologyTree(). Specifically, after
result.current.navigateToNode("urn:Target"), assert that the tree contains a
node for "urn:Root" and that this node is marked expanded (check the hook's
expanded state property such as expandedNodeIds or expandedIris) and that the
"urn:Target" node appears as a child of "urn:Root" (check result.current.tree,
result.current.nodes, or the hook's node map). Use the actual state property
names in useOntologyTree, and include assertions for both the ancestor being
expanded and the target being present under it.
---
Nitpick comments:
In `@__tests__/components/editor/ModeSwitcher.test.tsx`:
- Around line 45-46: Add '@testing-library/user-event' as a dev dependency,
import userEvent from '@testing-library/user-event' in ModeSwitcher.test.tsx,
and replace direct DOM .click() calls (e.g., screen.getByRole("button", { name:
"Developer" }).click()) with awaited userEvent.click(...) calls awaited via
await userEvent.click(screen.getByRole(...)); update the same pattern for the
other interaction at lines 52-53 and ensure assertions still reference
mockSetEditorMode toHaveBeenCalledWith("developer") after awaiting the
interaction.
In `@__tests__/components/projects/project-card.test.tsx`:
- Around line 106-115: The test asserts only /Updated.*2024/ so the fallback
case doesn't prove ProjectCard used created_at when updated_at is missing;
update the tests to use makeProject with explicit distinct created_at and
updated_at values and assert the rendered "Updated" text matches the expected
date string (or formatted output) for each case: for the first test pass an
explicit updated_at and assert that exact formatted updated_at appears, and for
the fallback test omit updated_at but set created_at to a different known date
and assert that the rendered "Updated" text equals that created_at value;
reference ProjectCard, makeProject, updated_at and created_at to locate the
changes.
In `@__tests__/components/ui/button.test.tsx`:
- Around line 105-110: The test uses the DOM .click() method; replace it with
`@testing-library/user-event` to simulate a real user: import userEvent from
'@testing-library/user-event', make the test async, and replace
screen.getByRole("button").click() with await
userEvent.click(screen.getByRole("button")); keep the same handleClick mock and
assertion (handleClick, Button, and screen.getByRole are the symbols to update).
- Around line 75-85: Add a behavioral test to ensure a disabled Button doesn't
call its click handler: in the same test file create a test (e.g., "does not
call onClick when disabled") that renders <Button disabled onClick={mockFn}>
with mockFn = jest.fn(), use fireEvent.click or userEvent.click on
screen.getByRole("button"), and assert mockFn was not called; reference the
Button component and existing tests that use screen.getByRole("button") to
locate where to add this new test.
In `@__tests__/lib/api/pullRequests.test.ts`:
- Around line 92-96: Add a test that exercises a write/mutation path (e.g.,
pullRequestsApi.create or pullRequestsApi.merge) to ensure it throws ApiError on
server errors: use the existing mockError(...) helper to simulate a failure
(e.g., mockError(403, "Forbidden", "Access denied")), then call the chosen
method (for example pullRequestsApi.create with minimal valid input) and assert
with await expect(...).rejects.toThrow(ApiError); reuse the same test structure
as the existing "throws ApiError on failure" list test to keep consistency.
- Around line 340-348: The test for githubIntegrationApi.update is missing a URL
assertion: after calling githubIntegrationApi.update("p1", { sync_enabled: true
}, "tok") add an expectation on mockFetch's first call argument (e.g. const
[url] = mockFetch.mock.calls[0]; expect(url).toBe("/github-integration/p1")) so
the test verifies the correct endpoint was used; keep the existing method
assertion for PATCH and place the URL assertion just before or alongside it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe2abc22-95aa-4d37-bcd6-f9ff14f85a48
📒 Files selected for processing (12)
__tests__/components/editor/ModeSwitcher.test.tsx__tests__/components/editor/ShareButton.test.tsx__tests__/components/projects/project-card.test.tsx__tests__/components/ui/button.test.tsx__tests__/lib/api/lint.test.ts__tests__/lib/api/projects.test.ts__tests__/lib/api/pullRequests.test.ts__tests__/lib/api/revisions.test.ts__tests__/lib/api/suggestions.test.ts__tests__/lib/hooks/useFilteredTree.test.ts__tests__/lib/hooks/useOntologyTree.test.ts__tests__/lib/hooks/useProject.test.ts
47 of 49 tasks
- Save and restore navigator.clipboard in afterEach to prevent cross-test leakage - Assert writeTextMock called exactly once and verify the copied URL on the same call argument Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Verify thrown ApiError has status === 403, not just instance check - Assert full encoded subject_iri value instead of partial match Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Verify fetch is called 3 times during 500 error retries - Assert FormData contains correct file, is_public, and name fields Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Parse the Blob sent to sendBeacon and verify session_id/content - Use token with reserved chars to verify URL encoding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verify the tree state after navigation: root node is expanded, target node appears as a child with correct label. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Replace .click() with userEvent.click() in ModeSwitcher and Button tests - Add test verifying disabled Button does not call onClick handler - Install @testing-library/user-event as dev dependency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use different years for updated_at and created_at so the fallback test proves ProjectCard used created_at, not just any date. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add test for pullRequestsApi.create throwing ApiError with status 403 - Assert correct URL in githubIntegrationApi.update test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused imports (waitFor, act, afterEach) - Add eslint-disable for react/display-name on test wrapper components - Fix CrossReferencesResponse test data types (ReferenceContext, source_type) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Update CI setup-node action default from Node 22 to Node 24 - Add .nvmrc pinning Node 24 for fnm/nvm local development - Regenerate package-lock.json with Node 24's npm to fix npm ci sync error (missing @swc/helpers) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (19)
__tests__/lib/hooks/useEntityHistory.test.ts (1)
140-146: Add a no-call assertion for the empty-string disabled case.This test checks
isFetching, but it should also assertgetEntityHistoryis not called (as you already do in the other disabled tests) to prevent regressions.Suggested tweak
it("is disabled when entityIri is empty string", () => { const { result } = renderHook( () => useEntityHistory("proj-1", "", "token"), { wrapper: createWrapper() }, ); expect(result.current.isFetching).toBe(false); + expect(mockedGetEntityHistory).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useEntityHistory.test.ts` around lines 140 - 146, The test for useEntityHistory when entityIri is an empty string should also assert that getEntityHistory was not invoked; update the test that renders useEntityHistory("proj-1", "", "token") to include a no-call assertion (e.g., expect(getEntityHistory).not.toHaveBeenCalled()) alongside the existing expect(result.current.isFetching).toBe(false) so the disabled-case prevents any calls to getEntityHistory.__tests__/lib/hooks/useKeyboardShortcuts.test.ts (1)
49-68: Assert cleanup with the exact listener reference.
expect.any(Function)can still pass if a different handler is removed. Capture the callback passed toaddEventListenerand assertremoveEventListenerreceives that same function.Suggested tightening
it("cleans up event listener on unmount", () => { + const addSpy = vi.spyOn(document, "addEventListener"); const removeSpy = vi.spyOn(document, "removeEventListener"); const action = vi.fn(); const shortcuts: ShortcutDefinition[] = [ @@ const { unmount } = renderHook(() => useKeyboardShortcuts(shortcuts)); unmount(); - expect(removeSpy).toHaveBeenCalledWith("keydown", expect.any(Function)); + const addedKeydownCall = addSpy.mock.calls.find((c) => c[0] === "keydown"); + const addedHandler = addedKeydownCall?.[1]; + expect(addedHandler).toBeTypeOf("function"); + expect(removeSpy).toHaveBeenCalledWith("keydown", addedHandler); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useKeyboardShortcuts.test.ts` around lines 49 - 68, The test currently asserts removeEventListener was called with expect.any(Function); instead capture the exact handler passed to addEventListener and assert removeEventListener was called with the same reference: spy on document.addEventListener (e.g., addSpy) to capture its second argument (the handler) when renderHook(() => useKeyboardShortcuts(shortcuts)) runs, unmount(), then assert removeEventListener was invoked with "keydown" and that exact captured handler (the same function reference) for tight verification of useKeyboardShortcuts cleanup.__tests__/lib/hooks/useProjectAnalytics.test.ts (1)
67-120: Consider adding "works without accessToken" test for consistency.The
useHotEntitiesanduseContributorstest suites include a test case verifying the hook works without anaccessToken(lines 167-176 and 224-233). Adding a similar test here would maintain consistency across all three hook suites.🧪 Suggested additional test case
it("works without accessToken", async () => { mockedGetActivity.mockResolvedValue(mockActivity); const { result } = renderHook( () => useProjectActivity("proj-1"), { wrapper: createWrapper() }, ); await waitFor(() => expect(result.current.isSuccess).toBe(true)); expect(mockedGetActivity).toHaveBeenCalledWith("proj-1", undefined, 30); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useProjectAnalytics.test.ts` around lines 67 - 120, Add a new test in the useProjectActivity suite that verifies the hook works without an accessToken: call renderHook(() => useProjectActivity("proj-1")) and mock mockedGetActivity to resolve mockActivity, await isSuccess via waitFor, and assert mockedGetActivity was called with ("proj-1", undefined, 30). Locate the test block for useProjectActivity (tests referencing mockedGetActivity and mockActivity) and add this case similar to the existing "uses default days parameter of 30" test.__tests__/lib/hooks/useCollaborationStatus.test.ts (1)
65-68: Consider extracting the connection delay to a shared constant.The tests are coupled to the hook's hardcoded 100ms delay (
setTimeout(connect, 100)in the hook). If this delay changes in the implementation, tests may fail unexpectedly or silently pass while testing incorrect behavior.Extract the delay to a constant in the hook (e.g.,
export const WS_CONNECTION_DELAY_MS = 100) and import it in tests for resilience.Example approach
In the hook file:
export const WS_CONNECTION_DELAY_MS = 100; // ... const timeout = setTimeout(connect, WS_CONNECTION_DELAY_MS);In the test file:
import { WS_CONNECTION_DELAY_MS } from "@/lib/hooks/useCollaborationStatus"; // ... vi.advanceTimersByTime(WS_CONNECTION_DELAY_MS);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useCollaborationStatus.test.ts` around lines 65 - 68, Extract the hardcoded 100ms delay used in useCollaborationStatus into an exported constant (e.g., export const WS_CONNECTION_DELAY_MS = 100) and replace the inline setTimeout(connect, 100) with setTimeout(connect, WS_CONNECTION_DELAY_MS); then update the test to import WS_CONNECTION_DELAY_MS from the hook and use vi.advanceTimersByTime(WS_CONNECTION_DELAY_MS) so tests remain in sync if the delay changes.__tests__/lib/hooks/useAutoSave.test.ts (1)
7-33: Consider extracting shared draft-store test utilities.This file repeats the same in-memory draft-store mock pattern and key construction used in the companion hook test. A shared helper would reduce duplication and keep both suites aligned when draft semantics evolve.
Also applies to: 120-123, 236-237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useAutoSave.test.ts` around lines 7 - 33, Extract the in-memory draft-store mock into a shared test helper and replace the inline mock in this file: move mockDrafts, stableSetDraft, stableClearDraft, stableGetDraft and the vi.mock(...) that returns draftKey and useDraftStore into a reusable utility (e.g., a setupDraftStoreMock helper) and import it in both hook tests; update tests to call the helper so both suites use the same mock implementation and teardown (ensuring the beforeEach cleanup remains part of the shared helper or is invoked by the tests).__tests__/lib/hooks/useEntityAutoSave.test.ts (1)
97-99: Reduce duplicated draft-key literals to avoid drift.The repeated
"proj-1:main:http://example.org/..."literals are easy to desync from test setup. Consider a tiny local key helper for readability and maintainability.♻️ Suggested refactor
+const keyFor = (iri: string) => `proj-1:main:${iri}`; ... -const key = "proj-1:main:http://example.org/myProp"; +const key = keyFor("http://example.org/myProp");Also applies to: 155-156, 204-205, 236-237, 245-246, 261-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useEntityAutoSave.test.ts` around lines 97 - 99, Multiple tests repeatedly inline the same draft-key literal like "proj-1:main:http://example.org/..." which risks drift; add a small local helper (e.g., const makeDraftKey = (proj = 'proj-1', branch = 'main', prop = 'http://example.org/myProp') => `${proj}:${branch}:${prop}`) at the top of the test suite and replace the duplicated literals (the key variable and other assertions referencing mockDrafts[key]) to call makeDraftKey() or pass specific parts so all assertions use the same derived key; update usages around where mockDrafts is asserted (the existing key const in these tests and the other occurrences mentioned) to call the helper for consistency and maintainability.__tests__/lib/hooks/useTreeSearch.test.ts (2)
122-129: Manual delay for negative assertion could be brittle.The 400ms delay assumes the debounce interval is less than 400ms. If the debounce timing changes, this test could either become slow or produce false negatives.
Using fake timers (
vi.useFakeTimers()) would make this deterministic and faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useTreeSearch.test.ts` around lines 122 - 129, The test uses a real 400ms setTimeout to assert no debounced search ran, which is brittle; replace the manual delay with fake timers (e.g., vi.useFakeTimers()) and advance the clock deterministically (vi.advanceTimersByTime) inside the act wrapper to simulate the debounce window, then restore timers (vi.useRealTimers) and assert that mockedSearchEntities was not called and result.current.searchResults is null; look for the test block using act(...) with the 400ms Promise and update it to use vi.useFakeTimers()/vi.advanceTimersByTime instead.
100-107: Timing-dependent assertion may be fragile.Line 101 asserts
isSearchingistrueimmediately aftersetSearchQuery, assuming the hook synchronously sets this flag before the debounced API call. If the hook implementation changes to setisSearchingonly when the debounce fires, this assertion will fail.Consider using
vi.useFakeTimers()withvi.advanceTimersByTime()for more deterministic debounce testing, which would also speed up the test execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useTreeSearch.test.ts` around lines 100 - 107, The test relies on timing by immediately asserting result.current.isSearching is true after calling setSearchQuery, which is fragile; update the test to use fake timers (vi.useFakeTimers()) and advance the debounce period deterministically (vi.advanceTimersByTime(300) or the debounce duration) before asserting state and awaiting async resolution so the debounce behavior is tested reliably; specifically modify the test around setSearchQuery, isSearching and waitFor to install fake timers, advance timers to trigger the debounced call, then restore real timers and assert result.current.isSearching and result.current.searchResults accordingly.__tests__/lib/hooks/useOntologyTree.test.ts (1)
68-77: Minor: consider using fake timers instead of real delay.The test uses a real 50ms delay to verify no API call occurs. While this works, using
vi.useFakeTimers()would make this deterministic and faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useOntologyTree.test.ts` around lines 68 - 77, The test "does not fetch when projectId is empty" uses a real 50ms delay; replace that with fake timers by calling vi.useFakeTimers() before rendering useOntologyTree({ projectId: "" }), then advance time with vi.advanceTimersByTime(50) to let any pending timers run and assert mockedGetRootClasses was not called and isLoading is false, and finally restore timers with vi.useRealTimers(); ensure references to useOntologyTree and mockedGetRootClasses remain the same so the change is localized.__tests__/lib/hooks/useRemoteSync.test.ts (3)
61-65: Emptyact()is unusual; consider using a small delay or fake timers.An empty
await act(async () => {})doesn't guarantee async effects have completed. If the intent is to flush promises, usingawait vi.waitFor(() => {})or fake timers would be more explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useRemoteSync.test.ts` around lines 61 - 65, The test contains an empty await act(async () => {}) which doesn't reliably flush microtasks; replace that no-op with an explicit flush such as using vi.waitFor(() => {}) or advancing fake timers (vi.useFakeTimers()/vi.runAllTimers()) to ensure async effects complete before the assertion; update the test around the empty act call in __tests__/lib/hooks/useRemoteSync.test.ts (the section containing the await act(...) and the expect(mockedGetConfig).not.toHaveBeenCalled()) to use vi.waitFor or fake timers to deterministically wait for pending async work.
138-165: Polling test relies on real timers, may be slow or flaky.This test waits up to 5 seconds for real polling to complete (the hook polls every 2000ms per
JOB_POLL_INTERVAL). Using fake timers would make this test faster and more deterministic.♻️ Suggested improvement with fake timers
+ it("triggerCheck sets isChecking and calls API", async () => { + vi.useFakeTimers(); + mockedGetConfig.mockResolvedValue(makeConfig()); mockedGetHistory.mockResolvedValue({ items: [], total: 0 }); mockedTriggerCheck.mockResolvedValue({ job_id: "job-1", message: "ok", status: "pending" }); - // The polling will call getJobStatus - mock it to complete immediately mockedGetJobStatus.mockResolvedValue({ job_id: "job-1", status: "complete", result: {}, error: null, }); const { result } = renderHook(() => useRemoteSync({ projectId: "proj-1", accessToken: "token" }), ); - await waitFor(() => expect(result.current.isLoading).toBe(false)); + await vi.runOnlyPendingTimersAsync(); + expect(result.current.isLoading).toBe(false); - // Trigger check await act(async () => { await result.current.triggerCheck(); }); expect(mockedTriggerCheck).toHaveBeenCalledWith("proj-1", "token"); - // Wait for polling to complete (setInterval runs with real timers) - await waitFor(() => expect(result.current.isChecking).toBe(false), { timeout: 5000 }); + // Advance past polling interval + await vi.advanceTimersByTimeAsync(2000); + expect(result.current.isChecking).toBe(false); + + vi.useRealTimers(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useRemoteSync.test.ts` around lines 138 - 165, The test for useRemoteSync relies on real timers which makes it slow and flaky; switch the test to use Jest fake timers around the polling section (call jest.useFakeTimers() before invoking triggerCheck and jest.useRealTimers() in finally), advance timers by the JOB_POLL_INTERVAL (import or reference JOB_POLL_INTERVAL) using jest.advanceTimersByTime(...) or jest.runOnlyPendingTimers() to drive the polling loop so mockedGetJobStatus resolves immediately, and then assert isChecking is false; ensure you still call mockedTriggerCheck/mock resolves as before and restore real timers after the assertion.
49-51: Module-levelbeforeEach— consider moving insidedescribe.Same pattern as
useGraphData.test.ts. PlacingbeforeEachinside thedescribeblock is more conventional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useRemoteSync.test.ts` around lines 49 - 51, Move the module-level beforeEach into the relevant describe block: remove the top-level beforeEach(() => { vi.clearAllMocks(); }); and add the same beforeEach inside the describe that wraps the useRemoteSync tests so vi.clearAllMocks() runs per-suite; ensure you place it alongside other setup in the describe so scope and mock-reset behavior match useGraphData.test.ts.__tests__/lib/hooks/useGraphData.test.ts (2)
49-56: Module-levelbeforeEachis unconventional.The
beforeEachat line 49 is outside thedescribeblock. While this works, placing it inside thedescribeblock is more conventional and makes the test setup scope clearer.♻️ Suggested change
describe("useGraphData", () => { + beforeEach(() => { + vi.clearAllMocks(); + mockedBuildGraph.mockReturnValue({ + nodes: [{ id: "http://example.org/A", label: "A", nodeType: "focus" }], + edges: [], + }); + mockedGetClassAncestors.mockResolvedValue({ nodes: [] }); + }); + it("returns null graphData when focusIri is null", () => {And remove the module-level
beforeEach(lines 49-56).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useGraphData.test.ts` around lines 49 - 56, The module-level beforeEach that calls vi.clearAllMocks() and stubs mockedBuildGraph and mockedGetClassAncestors should be moved inside the test suite's describe block to narrow its scope; remove the top-level beforeEach (the block referencing mockedBuildGraph and mockedGetClassAncestors) and add the same setup inside the describe that contains the tests so the mocks are reset and return values are initialized within that describe's scope.
107-126: Consider asserting specific IRI arguments for parent fetch.The test verifies
getClassDetailwas called twice but doesn't assert the specific arguments for the parent fetch. Adding an assertion for the second call would strengthen the test.💡 Suggested assertion
await waitFor(() => expect(result.current.isLoading).toBe(false)); expect(mockedGetClassDetail).toHaveBeenCalledTimes(2); + expect(mockedGetClassDetail).toHaveBeenNthCalledWith( + 2, + "proj-1", + "http://example.org/B", + "token", + undefined, + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useGraphData.test.ts` around lines 107 - 126, The test currently only checks mockedGetClassDetail call count; add an assertion that the second call received the expected parent IRI by asserting mockedGetClassDetail.mock.calls[1][0] (or using toHaveBeenNthCalledWith) equals "http://example.org/B". Update the "fetches parent nodes at depth 1" test after the waitFor assertion to explicitly check the argument used for the parent fetch (referencing mockedGetClassDetail and the useGraphData invocation) so the test verifies the correct IRI was requested for the parent node.__tests__/lib/hooks/useSuggestionSession.test.ts (1)
242-265: Assert theonErrorcallback in submit failure path.Line 249 defines
onError, but this test only checks local hook state. Add callback assertion to keep error-contract coverage consistent with other failure tests.♻️ Suggested assertion addition
it("submitSession handles errors", async () => { @@ expect(result.current.status).toBe("error"); expect(result.current.error).toBe("Submit failed"); + expect(onError).toHaveBeenCalledWith("Submit failed"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useSuggestionSession.test.ts` around lines 242 - 265, The test for submitSession error path is missing an assertion that the provided onError callback is invoked; update the "submitSession handles errors" test (which uses useSuggestionSession, mockedSubmit, startSession and submitSession) to verify that the onError mock was called (and optionally called with the error or expected args) after calling result.current.submitSession(), in addition to the existing checks of result.current.status and result.current.error.__tests__/lib/hooks/useSuggestionBeacon.test.ts (1)
188-215: Test title over-claims “without re-registering listeners”.Line 188’s case verifies latest ref values, but it does not assert listener registration counts remain unchanged after
rerender. Add call-count checks so the assertion matches the title.♻️ Suggested test hardening
it("uses latest ref values without re-registering listeners", () => { + const addDocSpy = vi.spyOn(document, "addEventListener"); + const addWinSpy = vi.spyOn(window, "addEventListener"); + const { rerender } = renderHook( ({ sessionId, getCurrentContent }) => useSuggestionBeacon({ @@ }, ); + const initialVisibilityRegistrations = addDocSpy.mock.calls.filter( + (c) => c[0] === "visibilitychange", + ).length; + const initialBeforeUnloadRegistrations = addWinSpy.mock.calls.filter( + (c) => c[0] === "beforeunload", + ).length; + // Update to new values rerender({ sessionId: "sess-2", getCurrentContent: () => "new content", }); + expect( + addDocSpy.mock.calls.filter((c) => c[0] === "visibilitychange"), + ).toHaveLength(initialVisibilityRegistrations); + expect( + addWinSpy.mock.calls.filter((c) => c[0] === "beforeunload"), + ).toHaveLength(initialBeforeUnloadRegistrations); + window.dispatchEvent(new Event("beforeunload")); expect(mockedBeacon).toHaveBeenCalledWith("proj-1", "sess-2", "new content", "sess-2"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useSuggestionBeacon.test.ts` around lines 188 - 215, The test title claims "without re-registering listeners" but currently only checks that latest ref values are used; update the test to also assert listener registration counts remain unchanged: before the initial render capture call counts/spies for window.addEventListener and window.removeEventListener (or the module-level listener-mocking helpers used in this test suite), then after rerender assert those spies' call counts did not increase, and still only one listener was registered; keep the existing checks for mockedBeacon and refer to useSuggestionBeacon, renderHook/rerender, window.dispatchEvent and mockedBeacon when adding these call-count assertions.__tests__/lib/hooks/useNotifications.test.ts (1)
91-94: Replace timing sleeps with deterministic waitsUsing
setTimeout(10)in async hook tests is brittle and can flake in CI. PreferwaitForon observable state/API-call conditions.Example refactor pattern
-// Give a tick for the effect to run -await act(async () => { - await new Promise((r) => setTimeout(r, 10)); -}); +await waitFor(() => { + expect(result.current.isLoading).toBe(false); +});-await act(async () => { - window.dispatchEvent(new Event(NOTIFICATIONS_CHANGED_EVENT)); - await new Promise((r) => setTimeout(r, 10)); -}); +await act(async () => { + window.dispatchEvent(new Event(NOTIFICATIONS_CHANGED_EVENT)); +}); +await waitFor(() => { + expect(mockedList.mock.calls.length).toBeGreaterThan(callCount); +});Also applies to: 134-137, 147-150, 161-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useNotifications.test.ts` around lines 91 - 94, Replace the brittle setTimeout-based sleeps inside the tests (the await act(async () => await new Promise(r => setTimeout(r, 10))) invocations) with deterministic waits using waitFor from your testing library; specifically, for each occurrence (around the act calls at the shown ranges) remove the setTimeout promise and instead call waitFor(() => <assertion on observable state or API-call effect>), targeting a concrete condition exposed by the hook (e.g., a state flag, return value, or a mocked API call count) so the test waits on observable behavior rather than an arbitrary timeout.__tests__/lib/hooks/useIriLabels.test.ts (1)
14-15: Prefervi.mocked(...)overReturnType<typeof vi.fn>casts.Lines 14–15 lose the original method signatures with the current cast, which weakens type-safety for call assertions and mock return values.
vi.mocked()preserves the original function types while properly enabling mock helpers like.mockResolvedValue()and.toHaveBeenCalled().Suggested refactor
-const mockedGetClassDetail = projectOntologyApi.getClassDetail as ReturnType<typeof vi.fn>; -const mockedSearchEntities = projectOntologyApi.searchEntities as ReturnType<typeof vi.fn>; +const mockedGetClassDetail = vi.mocked(projectOntologyApi.getClassDetail); +const mockedSearchEntities = vi.mocked(projectOntologyApi.searchEntities);This pattern appears 36+ times across the test suite and should be updated consistently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useIriLabels.test.ts` around lines 14 - 15, The tests cast mocks using ReturnType<typeof vi.fn>, which strips original signatures; replace those casts by using vi.mocked(...) to preserve types and enable proper mock helpers: change the declarations for mockedGetClassDetail and mockedSearchEntities to use vi.mocked(projectOntologyApi.getClassDetail) and vi.mocked(projectOntologyApi.searchEntities) respectively so call assertions and .mockResolvedValue/.toHaveBeenCalled keep correct typings; apply the same vi.mocked(...) refactor to the other similar mock variables across the test suite.__tests__/lib/hooks/useTreeDragDrop.test.ts (1)
37-51: Prefer per-test fixture factories over shared references.
BASE_TREEandBASE_OPTIONSare shared objects. UsingmakeTree()/makeOptions()factories would reduce future coupling and make tests more robust if node data or mock implementations are mutated in any test.Also applies to: 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/hooks/useTreeDragDrop.test.ts` around lines 37 - 51, BASE_TREE and BASE_OPTIONS are shared mutable fixtures; replace them with per-test factory functions (e.g., makeTree() and makeOptions()) that return a fresh tree and options object each time using makeNode(...) for node construction and new mocks (vi.fn(), vi.fn().mockResolvedValue(undefined)) for expandNode/onReparent so tests don’t share references or mutated state; update all test usages to call these factories instead of referencing BASE_TREE/BASE_OPTIONS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/lib/hooks/useIriLabels.test.ts`:
- Around line 84-87: Test currently only asserts getClassDetail wasn't called;
also assert the other API path is untouched by adding an expectation that
searchEntities was not invoked (e.g.
expect(mockedSearchEntities).not.toHaveBeenCalled()) after the existing
expect(mockedGetClassDetail).not.toHaveBeenCalled() check in the "Without
fetching" test in useIriLabels.test.ts so both API mocks (getClassDetail and
searchEntities) are asserted unused.
- Around line 168-171: Replace the hard-coded sleep in the test with
deterministic waiting for the fallback API to finish: remove the setTimeout
block and instead await a waitFor (from `@testing-library/react`) that asserts the
fallback call completed, e.g. await waitFor(() => expect(searchEntities
mock/spy).toHaveBeenCalled()) or await waitFor(() => /* UI update from
getClassDetail fallback is visible */), targeting the searchEntities invocation
that runs when getClassDetail fails; this ensures the test waits for the
asynchronous flow involving getClassDetail and searchEntities to complete
reliably.
In `@__tests__/lib/hooks/useNotifications.test.ts`:
- Around line 142-154: The test currently only checks for graceful failure state
after mockedList rejects with { status: 401 } but doesn't confirm polling
stopped; update the test for "stops polling on 401 error" to assert that
notificationsApi.list (mockedList) is not called again after the failure by
capturing the call count after the initial fetch and then simulating the hook's
polling trigger (e.g., dispatch the same event the hook listens for such as
window.dispatchEvent(new Event('focus')) or advance timers if it uses
setInterval) and asserting mockedList.mock.calls.length did not increase
(remains equal to the initial call count). Locate the test using mockedList and
useNotifications to add the additional simulation and call-count assertion.
In `@__tests__/lib/hooks/useSimilarEntities.test.ts`:
- Around line 138-144: In the "is disabled when entityIri is empty string" test
for useSimilarEntities, add an assertion that the mocked API was not invoked so
the guard prevents any query execution; specifically, after asserting
result.current.isFetching is false, add expect(<the mocked similar-entities API
function used in this test file>).not.toHaveBeenCalled() (i.e., the mock you use
for similar-entities requests) to ensure no network/query function (the API mock
for similar-entities) was called when entityIri is an empty string.
In `@__tests__/lib/hooks/useTreeDragDrop.test.ts`:
- Around line 178-180: The test is calling the async handler without awaiting it
inside async act blocks, which lets promise-driven state updates escape act;
update each occurrence where you call
result.current.handleDragEnd(makeDragEndEvent(...)) inside async act (lines
using act + makeDragEndEvent) to await the call (i.e., await
result.current.handleDragEnd(...)) so that handleDragEnd (which awaits
onReparent(...)) completes within act and avoids flaky assertions and warnings;
apply this change to all four occurrences referencing handleDragEnd, act,
makeDragEndEvent, and onReparent.
---
Nitpick comments:
In `@__tests__/lib/hooks/useAutoSave.test.ts`:
- Around line 7-33: Extract the in-memory draft-store mock into a shared test
helper and replace the inline mock in this file: move mockDrafts,
stableSetDraft, stableClearDraft, stableGetDraft and the vi.mock(...) that
returns draftKey and useDraftStore into a reusable utility (e.g., a
setupDraftStoreMock helper) and import it in both hook tests; update tests to
call the helper so both suites use the same mock implementation and teardown
(ensuring the beforeEach cleanup remains part of the shared helper or is invoked
by the tests).
In `@__tests__/lib/hooks/useCollaborationStatus.test.ts`:
- Around line 65-68: Extract the hardcoded 100ms delay used in
useCollaborationStatus into an exported constant (e.g., export const
WS_CONNECTION_DELAY_MS = 100) and replace the inline setTimeout(connect, 100)
with setTimeout(connect, WS_CONNECTION_DELAY_MS); then update the test to import
WS_CONNECTION_DELAY_MS from the hook and use
vi.advanceTimersByTime(WS_CONNECTION_DELAY_MS) so tests remain in sync if the
delay changes.
In `@__tests__/lib/hooks/useEntityAutoSave.test.ts`:
- Around line 97-99: Multiple tests repeatedly inline the same draft-key literal
like "proj-1:main:http://example.org/..." which risks drift; add a small local
helper (e.g., const makeDraftKey = (proj = 'proj-1', branch = 'main', prop =
'http://example.org/myProp') => `${proj}:${branch}:${prop}`) at the top of the
test suite and replace the duplicated literals (the key variable and other
assertions referencing mockDrafts[key]) to call makeDraftKey() or pass specific
parts so all assertions use the same derived key; update usages around where
mockDrafts is asserted (the existing key const in these tests and the other
occurrences mentioned) to call the helper for consistency and maintainability.
In `@__tests__/lib/hooks/useEntityHistory.test.ts`:
- Around line 140-146: The test for useEntityHistory when entityIri is an empty
string should also assert that getEntityHistory was not invoked; update the test
that renders useEntityHistory("proj-1", "", "token") to include a no-call
assertion (e.g., expect(getEntityHistory).not.toHaveBeenCalled()) alongside the
existing expect(result.current.isFetching).toBe(false) so the disabled-case
prevents any calls to getEntityHistory.
In `@__tests__/lib/hooks/useGraphData.test.ts`:
- Around line 49-56: The module-level beforeEach that calls vi.clearAllMocks()
and stubs mockedBuildGraph and mockedGetClassAncestors should be moved inside
the test suite's describe block to narrow its scope; remove the top-level
beforeEach (the block referencing mockedBuildGraph and mockedGetClassAncestors)
and add the same setup inside the describe that contains the tests so the mocks
are reset and return values are initialized within that describe's scope.
- Around line 107-126: The test currently only checks mockedGetClassDetail call
count; add an assertion that the second call received the expected parent IRI by
asserting mockedGetClassDetail.mock.calls[1][0] (or using
toHaveBeenNthCalledWith) equals "http://example.org/B". Update the "fetches
parent nodes at depth 1" test after the waitFor assertion to explicitly check
the argument used for the parent fetch (referencing mockedGetClassDetail and the
useGraphData invocation) so the test verifies the correct IRI was requested for
the parent node.
In `@__tests__/lib/hooks/useIriLabels.test.ts`:
- Around line 14-15: The tests cast mocks using ReturnType<typeof vi.fn>, which
strips original signatures; replace those casts by using vi.mocked(...) to
preserve types and enable proper mock helpers: change the declarations for
mockedGetClassDetail and mockedSearchEntities to use
vi.mocked(projectOntologyApi.getClassDetail) and
vi.mocked(projectOntologyApi.searchEntities) respectively so call assertions and
.mockResolvedValue/.toHaveBeenCalled keep correct typings; apply the same
vi.mocked(...) refactor to the other similar mock variables across the test
suite.
In `@__tests__/lib/hooks/useKeyboardShortcuts.test.ts`:
- Around line 49-68: The test currently asserts removeEventListener was called
with expect.any(Function); instead capture the exact handler passed to
addEventListener and assert removeEventListener was called with the same
reference: spy on document.addEventListener (e.g., addSpy) to capture its second
argument (the handler) when renderHook(() => useKeyboardShortcuts(shortcuts))
runs, unmount(), then assert removeEventListener was invoked with "keydown" and
that exact captured handler (the same function reference) for tight verification
of useKeyboardShortcuts cleanup.
In `@__tests__/lib/hooks/useNotifications.test.ts`:
- Around line 91-94: Replace the brittle setTimeout-based sleeps inside the
tests (the await act(async () => await new Promise(r => setTimeout(r, 10)))
invocations) with deterministic waits using waitFor from your testing library;
specifically, for each occurrence (around the act calls at the shown ranges)
remove the setTimeout promise and instead call waitFor(() => <assertion on
observable state or API-call effect>), targeting a concrete condition exposed by
the hook (e.g., a state flag, return value, or a mocked API call count) so the
test waits on observable behavior rather than an arbitrary timeout.
In `@__tests__/lib/hooks/useOntologyTree.test.ts`:
- Around line 68-77: The test "does not fetch when projectId is empty" uses a
real 50ms delay; replace that with fake timers by calling vi.useFakeTimers()
before rendering useOntologyTree({ projectId: "" }), then advance time with
vi.advanceTimersByTime(50) to let any pending timers run and assert
mockedGetRootClasses was not called and isLoading is false, and finally restore
timers with vi.useRealTimers(); ensure references to useOntologyTree and
mockedGetRootClasses remain the same so the change is localized.
In `@__tests__/lib/hooks/useProjectAnalytics.test.ts`:
- Around line 67-120: Add a new test in the useProjectActivity suite that
verifies the hook works without an accessToken: call renderHook(() =>
useProjectActivity("proj-1")) and mock mockedGetActivity to resolve
mockActivity, await isSuccess via waitFor, and assert mockedGetActivity was
called with ("proj-1", undefined, 30). Locate the test block for
useProjectActivity (tests referencing mockedGetActivity and mockActivity) and
add this case similar to the existing "uses default days parameter of 30" test.
In `@__tests__/lib/hooks/useRemoteSync.test.ts`:
- Around line 61-65: The test contains an empty await act(async () => {}) which
doesn't reliably flush microtasks; replace that no-op with an explicit flush
such as using vi.waitFor(() => {}) or advancing fake timers
(vi.useFakeTimers()/vi.runAllTimers()) to ensure async effects complete before
the assertion; update the test around the empty act call in
__tests__/lib/hooks/useRemoteSync.test.ts (the section containing the await
act(...) and the expect(mockedGetConfig).not.toHaveBeenCalled()) to use
vi.waitFor or fake timers to deterministically wait for pending async work.
- Around line 138-165: The test for useRemoteSync relies on real timers which
makes it slow and flaky; switch the test to use Jest fake timers around the
polling section (call jest.useFakeTimers() before invoking triggerCheck and
jest.useRealTimers() in finally), advance timers by the JOB_POLL_INTERVAL
(import or reference JOB_POLL_INTERVAL) using jest.advanceTimersByTime(...) or
jest.runOnlyPendingTimers() to drive the polling loop so mockedGetJobStatus
resolves immediately, and then assert isChecking is false; ensure you still call
mockedTriggerCheck/mock resolves as before and restore real timers after the
assertion.
- Around line 49-51: Move the module-level beforeEach into the relevant describe
block: remove the top-level beforeEach(() => { vi.clearAllMocks(); }); and add
the same beforeEach inside the describe that wraps the useRemoteSync tests so
vi.clearAllMocks() runs per-suite; ensure you place it alongside other setup in
the describe so scope and mock-reset behavior match useGraphData.test.ts.
In `@__tests__/lib/hooks/useSuggestionBeacon.test.ts`:
- Around line 188-215: The test title claims "without re-registering listeners"
but currently only checks that latest ref values are used; update the test to
also assert listener registration counts remain unchanged: before the initial
render capture call counts/spies for window.addEventListener and
window.removeEventListener (or the module-level listener-mocking helpers used in
this test suite), then after rerender assert those spies' call counts did not
increase, and still only one listener was registered; keep the existing checks
for mockedBeacon and refer to useSuggestionBeacon, renderHook/rerender,
window.dispatchEvent and mockedBeacon when adding these call-count assertions.
In `@__tests__/lib/hooks/useSuggestionSession.test.ts`:
- Around line 242-265: The test for submitSession error path is missing an
assertion that the provided onError callback is invoked; update the
"submitSession handles errors" test (which uses useSuggestionSession,
mockedSubmit, startSession and submitSession) to verify that the onError mock
was called (and optionally called with the error or expected args) after calling
result.current.submitSession(), in addition to the existing checks of
result.current.status and result.current.error.
In `@__tests__/lib/hooks/useTreeDragDrop.test.ts`:
- Around line 37-51: BASE_TREE and BASE_OPTIONS are shared mutable fixtures;
replace them with per-test factory functions (e.g., makeTree() and
makeOptions()) that return a fresh tree and options object each time using
makeNode(...) for node construction and new mocks (vi.fn(),
vi.fn().mockResolvedValue(undefined)) for expandNode/onReparent so tests don’t
share references or mutated state; update all test usages to call these
factories instead of referencing BASE_TREE/BASE_OPTIONS.
In `@__tests__/lib/hooks/useTreeSearch.test.ts`:
- Around line 122-129: The test uses a real 400ms setTimeout to assert no
debounced search ran, which is brittle; replace the manual delay with fake
timers (e.g., vi.useFakeTimers()) and advance the clock deterministically
(vi.advanceTimersByTime) inside the act wrapper to simulate the debounce window,
then restore timers (vi.useRealTimers) and assert that mockedSearchEntities was
not called and result.current.searchResults is null; look for the test block
using act(...) with the 400ms Promise and update it to use
vi.useFakeTimers()/vi.advanceTimersByTime instead.
- Around line 100-107: The test relies on timing by immediately asserting
result.current.isSearching is true after calling setSearchQuery, which is
fragile; update the test to use fake timers (vi.useFakeTimers()) and advance the
debounce period deterministically (vi.advanceTimersByTime(300) or the debounce
duration) before asserting state and awaiting async resolution so the debounce
behavior is tested reliably; specifically modify the test around setSearchQuery,
isSearching and waitFor to install fake timers, advance timers to trigger the
debounced call, then restore real timers and assert result.current.isSearching
and result.current.searchResults accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3635a856-99bd-4eb8-8ece-6ad67a7c3c96
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
__tests__/components/editor/ModeSwitcher.test.tsx__tests__/components/editor/ShareButton.test.tsx__tests__/components/projects/project-card.test.tsx__tests__/components/ui/button.test.tsx__tests__/lib/api/lint.test.ts__tests__/lib/api/projects.test.ts__tests__/lib/api/pullRequests.test.ts__tests__/lib/api/suggestions.test.ts__tests__/lib/hooks/useAutoSave.test.ts__tests__/lib/hooks/useCollaborationStatus.test.ts__tests__/lib/hooks/useCrossReferences.test.ts__tests__/lib/hooks/useEntityAutoSave.test.ts__tests__/lib/hooks/useEntityHistory.test.ts__tests__/lib/hooks/useFilteredTree.test.ts__tests__/lib/hooks/useGraphData.test.ts__tests__/lib/hooks/useIriLabels.test.ts__tests__/lib/hooks/useKeyboardShortcuts.test.ts__tests__/lib/hooks/useNotifications.test.ts__tests__/lib/hooks/useOntologyTree.test.ts__tests__/lib/hooks/useProject.test.ts__tests__/lib/hooks/useProjectAnalytics.test.ts__tests__/lib/hooks/useRemoteSync.test.ts__tests__/lib/hooks/useSemanticSearch.test.ts__tests__/lib/hooks/useSimilarEntities.test.ts__tests__/lib/hooks/useSuggestionBeacon.test.ts__tests__/lib/hooks/useSuggestionSession.test.ts__tests__/lib/hooks/useTreeDragDrop.test.ts__tests__/lib/hooks/useTreeSearch.test.tspackage.json
✅ Files skipped from review due to trivial changes (4)
- package.json
- tests/lib/api/pullRequests.test.ts
- tests/lib/hooks/useFilteredTree.test.ts
- tests/lib/api/projects.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/components/ui/button.test.tsx
- tests/lib/api/lint.test.ts
- tests/components/editor/ModeSwitcher.test.tsx
- tests/components/editor/ShareButton.test.tsx
- tests/lib/api/suggestions.test.ts
- tests/components/projects/project-card.test.tsx
- tests/lib/hooks/useProject.test.ts
…ith waitFor - Assert searchEntities not called when all IRIs have labelHints - Replace setTimeout with waitFor on searchEntities for fallback test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dispatch a polling event after the 401 and verify the API mock call count does not increase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ensures promise-driven state updates from onReparent complete within act, preventing flaky assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Coverage improvements: - Ontology: 11% → 90% stmts / 93% lines (7 new test files + fixtures) - Components: 14 new test files covering UI, editor, PR, revision, projects - Overall: 22% → 32% statements Ontology tests (133 tests): - turtleUtils (58), turtleBlockParser (17), turtleClassUpdater (13) - turtlePropertyUpdater (14), turtleIndividualUpdater (15) - annotationProperties (16), shared fixtures Component tests (155 tests): - UI: dialog (9), confirm-dialog (10), context-menu (7), ConnectionStatus (12), file-upload (11), tooltip (8) - Editor: LanguageFlag (10), CommitMessageDialog (10), AutoSaveAffordanceBar (14), SuggestionStatusBar (15), RemoteSyncIndicator (10) - PR: PRListItem (17) - Revision: BranchBadge (10) - Projects: member-list (12) All 818 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
__tests__/components/ui/context-menu.test.tsx (4)
58-70: Multiple tests only assert the Trigger exists, not the feature under test.Tests for
ContextMenuLabel(line 58),ContextMenuSeparator(line 72), and "multiple items" (line 99) all share the same pattern: they set up a complex menu structure but only verifyscreen.getByText("Trigger"). This doesn't confirm the label, separator, or items actually render.Consider using Radix's
forceMountprop onContextMenuContentto bypass portal/visibility issues, allowing direct assertions:<ContextMenuContent forceMount> <ContextMenuLabel>Actions</ContextMenuLabel> ... </ContextMenuContent>Then you can assert:
expect(screen.getByText("Actions")).toBeInTheDocument();Also applies to: 72-84, 99-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/context-menu.test.tsx` around lines 58 - 70, The tests render a full ContextMenu but only assert the Trigger exists, so update each test (the ones exercising ContextMenuLabel, ContextMenuSeparator, and multiple ContextMenuItem cases) to mount the content and assert the actual elements: use ContextMenuContent with the forceMount prop to force render the menu DOM, then assert the label text via ContextMenuLabel (e.g., expect screen.getByText("Actions") or similar), assert the separator element for ContextMenuSeparator, and assert multiple items by querying their texts instead of only checking the Trigger.
42-56: Test has no assertions after triggering the context menu.The test invokes
rightClick()but doesn't assert anything afterward. The comment acknowledges JSDOM/portal limitations, but the test should at least verify the interaction doesn't throw, or usewaitForto attempt finding the menu items. As written, this test only verifies the component mounts—not that the right-click interaction works.Consider using
@testing-library/user-eventfor more realistic interactions, or add a minimal assertion:rightClick(screen.getByText("Trigger")); - // Radix portals content - items may or may not be visible based on jsdom support - // We test the component renders without errors + // Even if portal content isn't visible in JSDOM, verify no errors thrown + expect(screen.getByText("Trigger")).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/context-menu.test.tsx` around lines 42 - 56, The test calls rightClick on the ContextMenu trigger but has no assertions; update the test for ContextMenu/ContextMenuTrigger/ContextMenuContent/ContextMenuItem so it actually verifies the interaction: either wrap the rightClick call in an assertion that it doesn't throw (e.g., expect(() => rightClick(screen.getByText("Trigger"))).not.toThrow()), or perform a realistic interaction using `@testing-library/user-event` (userEvent.click with {button:2}) and then use waitFor to assert that an item like "Copy" or "Paste" is present (waitFor(() => expect(screen.getByText("Copy")).toBeInTheDocument())). Ensure the test imports user-event or waitFor as needed and uses the existing component symbols (ContextMenu, ContextMenuTrigger, ContextMenuContent, ContextMenuItem).
27-27: PrefertoBeInTheDocument()overtoBeDefined()/toBeNull()for DOM assertions.React Testing Library's jest-dom matchers (
.toBeInTheDocument(),.not.toBeInTheDocument()) are more semantically appropriate for DOM presence checks and provide better error messages. Using.toBeDefined()on a DOM element works but is less idiomatic.-expect(screen.getByText("Right-click me")).toBeDefined(); +expect(screen.getByText("Right-click me")).toBeInTheDocument(); -expect(screen.queryByText("Edit")).toBeNull(); +expect(screen.queryByText("Edit")).not.toBeInTheDocument();Also applies to: 39-39, 69-69, 83-83, 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/context-menu.test.tsx` at line 27, Replace DOM assertions that use .toBeDefined()/.toBeNull() with jest-dom matchers .toBeInTheDocument()/.not.toBeInTheDocument(): update assertions that call screen.getByText("Right-click me") (and other getByText/queryByText uses at the indicated locations) to use .toBeInTheDocument() for presence and .not.toBeInTheDocument() for absence; ensure the test environment has jest-dom matchers available (import '@testing-library/jest-dom' in your test setup if not already) so .toBeInTheDocument() resolves.
86-97: Test title promises verification of destructive styling but makes no such assertion.The test is named "applies destructive styling class to destructive items" but only asserts
containeris defined. This gives false confidence—the test will pass even if thedestructiveprop is completely broken.Either rename the test to reflect what it actually verifies, or add a meaningful assertion:
🔧 Proposed fix to actually verify destructive styling
it("applies destructive styling class to destructive items", () => { - // We can test the component renders with the destructive prop - const { container } = render( + render( <ContextMenu> <ContextMenuTrigger>Trigger</ContextMenuTrigger> - <ContextMenuContent> + <ContextMenuContent forceMount> <ContextMenuItem destructive>Delete</ContextMenuItem> </ContextMenuContent> </ContextMenu> ); - expect(container).toBeDefined(); + const deleteItem = screen.getByText("Delete"); + expect(deleteItem.closest('[data-destructive]') || deleteItem.className).toMatch(/destructive|red|danger/i); });Alternatively, if JSDOM limitations prevent this, rename the test to "renders ContextMenuItem with destructive prop without crashing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/context-menu.test.tsx` around lines 86 - 97, The test title promises verification of destructive styling but only checks container existence; update the test for ContextMenu/ContextMenuItem by either asserting the rendered "Delete" item has the destructive styling (e.g., locate the item via getByText("Delete") or querySelector on ContextMenuItem and assert it contains the expected destructive CSS class or attribute used by your component) or, if visual/JSDOM limitations prevent class checks, rename the test to "renders ContextMenuItem with destructive prop without crashing" so the title matches the actual assertion; reference ContextMenu, ContextMenuTrigger, ContextMenuContent, and ContextMenuItem when making the change.__tests__/lib/ontology/entityDetailExtractors.test.ts (1)
71-177: Optional: add explicit non-null guard beforedetail!dereferences.Adding
expect(detail).not.toBeNull()in tests that currently go straight todetail!will improve failure readability if nullability regresses.Also applies to: 225-299
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/ontology/entityDetailExtractors.test.ts` around lines 71 - 177, Several tests call extractPropertyDetail(...) and immediately dereference detail with detail! which yields unhelpful errors if it becomes null; add an explicit non-null guard by inserting expect(detail).not.toBeNull() before any dereference in each affected it(...) block (e.g., the tests asserting deprecated, non-deprecated, domain/range, subPropertyOf, inverseOf, equivalentProperty, propertyDisjointWith, seeAlso, isDefinedBy, definitions, null inverseOf case, and the characteristics test) so failures show a clear null assertion message; update the same pattern in the other group referenced (lines ~225-299) to match.__tests__/lib/ontology/turtleClassUpdater.test.ts (1)
66-68: Block slicing via first.is brittle.Lines 67, 86, and 177 use
indexOf(".", dogBlockStart), which can truncate early if the block contains periods in literals/IRIs. Consider a block-end scanner (or shared helper) that respects Turtle syntax.Also applies to: 85-87, 176-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/ontology/turtleClassUpdater.test.ts` around lines 66 - 68, The test is brittle because it finds the block end with result.indexOf(".", dogBlockStart) which can hit periods inside literals/IRIs; replace these slices (variables dogBlockStart, dogBlockEnd, dogBlock and the other similar slices) with a small helper that scans from the start index to find the terminating Turtle statement period while skipping characters inside quotes ("...","'''...'''","'''...'''"), angle-brackets (<...>), and escaped quotes—use that helper in place of the direct indexOf calls so the tests extract full Turtle statements safely.__tests__/lib/ontology/turtleIndividualUpdater.test.ts (1)
241-244: Use a safer block-boundary strategy thanindexOf(".").At Lines 243 and 264, first-dot slicing can mis-detect the end of a Turtle block when periods appear in content. A syntax-aware helper will make these assertions more reliable.
Also applies to: 262-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/lib/ontology/turtleIndividualUpdater.test.ts` around lines 241 - 244, The test currently extracts Turtle statement blocks using result.slice with result.indexOf("ex:fido") and the first subsequent "." which can break when periods appear inside literals or IRIs; replace this brittle approach by introducing a small syntax-aware helper (e.g., findStatementEnd(text: string, startIdx: number)) used by the fidoBlock extraction and the other similar slice at the other assertion: have the helper scan forward from the predicate/subject start and ignore periods inside quoted strings (and escaped quotes) and inside <...> IRIs, returning the index of the true statement terminator dot so you can slice safely; update the test to call findStatementEnd(result, result.indexOf("ex:fido")) (and the other occurrence) instead of using indexOf(".").__tests__/components/projects/member-list.test.tsx (1)
103-111: Refactor action-menu tests to avoid brittle global button selection.Tests use
getAllByRole("button")with positionalbuttons[0]access (lines 123, 131, 139, 168, 196, 224), coupling test logic to unrelated button count and order. Instead, locate specific member rows by name (e.g.,screen.getByText("Bob Jones")), then query the action button within that row context usingwithin(). This approach isolates member-scoped logic and makes tests resilient to unrelated DOM changes.Examples of current pattern
const buttons = screen.getAllByRole("button"); await userEvent.click(buttons[0]);Also applies to: 113-119, 121-127, 129-134, 136-143, 168-170, 196-198, 224-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/projects/member-list.test.tsx` around lines 103 - 111, Tests in member-list.test.tsx are brittle because they query global buttons via getAllByRole("button") and index into buttons[], causing failures when unrelated buttons change; update each test that uses this pattern (including the cases around the "does not show action menu..." spec and the other specs referencing buttons at lines noted) to instead locate the specific member row by text (e.g., screen.getByText("Bob Jones") or the member's display name) and then use within(memberRow) to query/click the action button (use within(...) and within(...).getByRole("button") or queryByRole) so the action-menu interactions are scoped to the member row and no longer depend on global button order. Ensure all references to buttons[...] are replaced with this scoped lookup in tests that interact with MemberList action menus.__tests__/components/editor/CommitMessageDialog.test.tsx (1)
73-78: Test name should reflect disabled-button behavior, not error rendering.Line 73 says “shows error”, but the assertion at Line 77 verifies
disabled === true.Suggested rename
- it("shows error when commit message is only whitespace", async () => { + it("disables Save & Commit when commit message is only whitespace", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/CommitMessageDialog.test.tsx` around lines 73 - 78, Rename the test whose description is "shows error when commit message is only whitespace" to accurately reflect the assertion that the submit button is disabled; update the it(...) string to something like "disables submit button when commit message is only whitespace" so the test name matches the checked behavior in the test for CommitMessageDialog (look for the it(...) block that renders CommitMessageDialog with defaultMessage=" " and asserts (submitBtn as HTMLButtonElement).disabled).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/editor/LanguageFlag.test.tsx`:
- Around line 65-69: The test "renders without crashing for unknown lang codes"
currently uses container.querySelector("span") and asserts
expect(span).toBeDefined(), which falsely passes when span is null; update the
assertion to assert non-null/exists (e.g., expect(span).not.toBeNull() or, if
jest-dom matchers are available, expect(span).toBeInTheDocument()) or prefer a
more specific query from `@testing-library` like getByText/getByRole to locate the
element rendered by the LanguageFlag component and assert its presence so the
test will fail if the component doesn't render a span.
In `@__tests__/components/editor/RemoteSyncIndicator.test.tsx`:
- Around line 20-27: The mocked ApiError class signature is wrong: update the
mock of ApiError so its constructor matches the production shape (status,
statusText, message), assign this.status = status and this.statusText =
statusText and call super(message) so tests that construct new ApiError(404,
"Not Found", "Not found") see the correct status and statusText; locate the
ApiError mock definition in the test file and adjust the constructor parameters
and property assignments accordingly.
In `@__tests__/components/pr/PRListItem.test.tsx`:
- Around line 131-137: The test "does not show commits section when
commits_ahead is 0" can miss phrases like "0 commits ahead"; update the
assertions in the test for PRListItem (using makePR and projectId) to query for
the full phrase or a regex (e.g., matching "0 commits ahead" or /\bcommits?
ahead\b/) instead of exact "commit"/"commits" so the test fails if any "commits
ahead" text is rendered; replace the two queryByText checks with a single
regex-based query expecting null.
- Around line 168-180: The test for PRListItem incorrectly only asserts presence
of the "merged" status text; update the test to also assert the rendered
merged_at date by deriving a deterministic fragment from the mergedAt value used
in the fixture (e.g., "Jan 15, 2025" or "2025-01-15") and using
screen.getByText/new RegExp to verify that PRListItem (component PRListItem,
test helper makePR) actually displays the merged date alongside the status.
In `@__tests__/components/revision/BranchBadge.test.tsx`:
- Around line 26-116: The tests mutate shared mockBranchContext which leaks
state between tests; fix by resetting mockBranchContext to a known default in a
beforeEach or afterEach hook in BranchBadge.test.tsx (e.g., initialize
mockBranchContext.branches and mockBranchContext.isFeatureBranch to their
default values before each test) so individual tests no longer rely on manual
"Restore" lines; update any tests referencing mockBranchContext to assume the
fresh default instead of mutating global state.
- Around line 5-16: The mock factory for vi.mock currently references the
external variable mockBranchContext (and returns useBranch => mockBranchContext)
which will break due to hoisting; change the mock to use vi.hoisted so the
factory creates and returns the mock object inside the hoisted callback (e.g.,
replace vi.mock(..., () => ({ useBranch: () => mockBranchContext })) with
vi.hoisted(() => ({ useBranch: () => ({ currentBranch: "feature/add-class",
defaultBranch: "main", branches: [{ name: "feature/add-class", commits_ahead: 3,
commits_behind: 0 }], isFeatureBranch: true }) }))) so the mocked useBranch is
self-contained and does not reference the top-level mockBranchContext variable
at module-evaluation time.
In `@__tests__/components/ui/confirm-dialog.test.tsx`:
- Around line 113-115: The test wraps a click in act() without awaiting the
async userEvent.click, causing a race with the subsequent waitFor; update the
submitting-state test so you await an async act and await userEvent.click inside
it (i.e., use await act(async () => { await
userEvent.click(screen.getByRole("button", { name: "Confirm" })) })), ensuring
the click interaction occurs within act() before the following waitFor()
assertion.
In `@__tests__/components/ui/dialog.test.tsx`:
- Around line 121-136: The test "closes dialog when close button is clicked"
swallows promise rejection and lacks a real post-close assertion; update the
test around the userEvent.click call to assert the dialog content is removed by
either using await waitForElementToBeRemoved(() => screen.getByText("Close")) or
asserting expect(screen.queryByText("Close")).toBeNull() after the click;
reference the Dialog component, DialogContent/DialogTitle, userEvent.click and
screen.getByText/screen.queryByText or waitForElementToBeRemoved to locate and
update the assertion.
In `@__tests__/components/ui/file-upload.test.tsx`:
- Around line 29-31: The assertion using toBeDefined() on the result of
container.querySelector("input[type='file']") is inadequate because
querySelector can return null; replace it with a strict existence check such as
expect(input).not.toBeNull() or expect(input).toBeInstanceOf(HTMLInputElement)
(or use React Testing Library's expect(screen.getByRole('textbox' or
'file'))/toBeInTheDocument()) so the test fails when the file input is missing;
update the assertions around the variable named input (from
container.querySelector(...)) and keep the subsequent accept check using
input.accept after confirming input is non-null.
In `@__tests__/components/ui/tooltip.test.tsx`:
- Around line 33-43: The test "shows tooltip content on hover" currently only
asserts the trigger remains, which doesn't verify tooltip behavior; update the
test in __tests__/components/ui/tooltip.test.tsx so after calling
userEvent.hover(screen.getByText("Hover target")) it first attempts to assert
the tooltip content appears (e.g., await screen.findByText("Visible now") or
similar) but fall back to asserting the trigger is still present if the tooltip
cannot render in jsdom—use the Tooltip component, renderWithProvider,
userEvent.hover, and screen.getByText/screen.findByText to implement this
conditional/assertion sequence.
In `@__tests__/lib/ontology/turtleBlockParser.test.ts`:
- Around line 281-285: The test is brittle because it assumes order by checking
triples![0].predicate; update the assertion to find the triple with predicate
"http://www.w3.org/1999/02/22-rdf-syntax-ns#type" instead of indexing into
triples. Use parseBlockTriples(...) result (triples) and assert that
some/triples.find(...) returns a non-null triple whose .predicate equals the RDF
type URI, keeping other assertions unchanged.
---
Nitpick comments:
In `@__tests__/components/editor/CommitMessageDialog.test.tsx`:
- Around line 73-78: Rename the test whose description is "shows error when
commit message is only whitespace" to accurately reflect the assertion that the
submit button is disabled; update the it(...) string to something like "disables
submit button when commit message is only whitespace" so the test name matches
the checked behavior in the test for CommitMessageDialog (look for the it(...)
block that renders CommitMessageDialog with defaultMessage=" " and asserts
(submitBtn as HTMLButtonElement).disabled).
In `@__tests__/components/projects/member-list.test.tsx`:
- Around line 103-111: Tests in member-list.test.tsx are brittle because they
query global buttons via getAllByRole("button") and index into buttons[],
causing failures when unrelated buttons change; update each test that uses this
pattern (including the cases around the "does not show action menu..." spec and
the other specs referencing buttons at lines noted) to instead locate the
specific member row by text (e.g., screen.getByText("Bob Jones") or the member's
display name) and then use within(memberRow) to query/click the action button
(use within(...) and within(...).getByRole("button") or queryByRole) so the
action-menu interactions are scoped to the member row and no longer depend on
global button order. Ensure all references to buttons[...] are replaced with
this scoped lookup in tests that interact with MemberList action menus.
In `@__tests__/components/ui/context-menu.test.tsx`:
- Around line 58-70: The tests render a full ContextMenu but only assert the
Trigger exists, so update each test (the ones exercising ContextMenuLabel,
ContextMenuSeparator, and multiple ContextMenuItem cases) to mount the content
and assert the actual elements: use ContextMenuContent with the forceMount prop
to force render the menu DOM, then assert the label text via ContextMenuLabel
(e.g., expect screen.getByText("Actions") or similar), assert the separator
element for ContextMenuSeparator, and assert multiple items by querying their
texts instead of only checking the Trigger.
- Around line 42-56: The test calls rightClick on the ContextMenu trigger but
has no assertions; update the test for
ContextMenu/ContextMenuTrigger/ContextMenuContent/ContextMenuItem so it actually
verifies the interaction: either wrap the rightClick call in an assertion that
it doesn't throw (e.g., expect(() =>
rightClick(screen.getByText("Trigger"))).not.toThrow()), or perform a realistic
interaction using `@testing-library/user-event` (userEvent.click with {button:2})
and then use waitFor to assert that an item like "Copy" or "Paste" is present
(waitFor(() => expect(screen.getByText("Copy")).toBeInTheDocument())). Ensure
the test imports user-event or waitFor as needed and uses the existing component
symbols (ContextMenu, ContextMenuTrigger, ContextMenuContent, ContextMenuItem).
- Line 27: Replace DOM assertions that use .toBeDefined()/.toBeNull() with
jest-dom matchers .toBeInTheDocument()/.not.toBeInTheDocument(): update
assertions that call screen.getByText("Right-click me") (and other
getByText/queryByText uses at the indicated locations) to use
.toBeInTheDocument() for presence and .not.toBeInTheDocument() for absence;
ensure the test environment has jest-dom matchers available (import
'@testing-library/jest-dom' in your test setup if not already) so
.toBeInTheDocument() resolves.
- Around line 86-97: The test title promises verification of destructive styling
but only checks container existence; update the test for
ContextMenu/ContextMenuItem by either asserting the rendered "Delete" item has
the destructive styling (e.g., locate the item via getByText("Delete") or
querySelector on ContextMenuItem and assert it contains the expected destructive
CSS class or attribute used by your component) or, if visual/JSDOM limitations
prevent class checks, rename the test to "renders ContextMenuItem with
destructive prop without crashing" so the title matches the actual assertion;
reference ContextMenu, ContextMenuTrigger, ContextMenuContent, and
ContextMenuItem when making the change.
In `@__tests__/lib/ontology/entityDetailExtractors.test.ts`:
- Around line 71-177: Several tests call extractPropertyDetail(...) and
immediately dereference detail with detail! which yields unhelpful errors if it
becomes null; add an explicit non-null guard by inserting
expect(detail).not.toBeNull() before any dereference in each affected it(...)
block (e.g., the tests asserting deprecated, non-deprecated, domain/range,
subPropertyOf, inverseOf, equivalentProperty, propertyDisjointWith, seeAlso,
isDefinedBy, definitions, null inverseOf case, and the characteristics test) so
failures show a clear null assertion message; update the same pattern in the
other group referenced (lines ~225-299) to match.
In `@__tests__/lib/ontology/turtleClassUpdater.test.ts`:
- Around line 66-68: The test is brittle because it finds the block end with
result.indexOf(".", dogBlockStart) which can hit periods inside literals/IRIs;
replace these slices (variables dogBlockStart, dogBlockEnd, dogBlock and the
other similar slices) with a small helper that scans from the start index to
find the terminating Turtle statement period while skipping characters inside
quotes ("...","'''...'''","'''...'''"), angle-brackets (<...>), and escaped
quotes—use that helper in place of the direct indexOf calls so the tests extract
full Turtle statements safely.
In `@__tests__/lib/ontology/turtleIndividualUpdater.test.ts`:
- Around line 241-244: The test currently extracts Turtle statement blocks using
result.slice with result.indexOf("ex:fido") and the first subsequent "." which
can break when periods appear inside literals or IRIs; replace this brittle
approach by introducing a small syntax-aware helper (e.g.,
findStatementEnd(text: string, startIdx: number)) used by the fidoBlock
extraction and the other similar slice at the other assertion: have the helper
scan forward from the predicate/subject start and ignore periods inside quoted
strings (and escaped quotes) and inside <...> IRIs, returning the index of the
true statement terminator dot so you can slice safely; update the test to call
findStatementEnd(result, result.indexOf("ex:fido")) (and the other occurrence)
instead of using indexOf(".").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5077bcb7-98d3-493d-99cb-2ae02d5c3bf4
📒 Files selected for processing (22)
__tests__/components/editor/AutoSaveAffordanceBar.test.tsx__tests__/components/editor/CommitMessageDialog.test.tsx__tests__/components/editor/LanguageFlag.test.tsx__tests__/components/editor/RemoteSyncIndicator.test.tsx__tests__/components/editor/SuggestionStatusBar.test.tsx__tests__/components/pr/PRListItem.test.tsx__tests__/components/projects/member-list.test.tsx__tests__/components/revision/BranchBadge.test.tsx__tests__/components/ui/ConnectionStatus.test.tsx__tests__/components/ui/confirm-dialog.test.tsx__tests__/components/ui/context-menu.test.tsx__tests__/components/ui/dialog.test.tsx__tests__/components/ui/file-upload.test.tsx__tests__/components/ui/tooltip.test.tsx__tests__/lib/ontology/annotationProperties.test.ts__tests__/lib/ontology/entityDetailExtractors.test.ts__tests__/lib/ontology/fixtures.ts__tests__/lib/ontology/turtleBlockParser.test.ts__tests__/lib/ontology/turtleClassUpdater.test.ts__tests__/lib/ontology/turtleIndividualUpdater.test.ts__tests__/lib/ontology/turtlePropertyUpdater.test.ts__tests__/lib/ontology/turtleUtils.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/lib/ontology/annotationProperties.test.ts
Comment on lines
+5
to
+16
| const mockBranchContext = { | ||
| currentBranch: "feature/add-class", | ||
| defaultBranch: "main", | ||
| branches: [ | ||
| { name: "feature/add-class", commits_ahead: 3, commits_behind: 0 }, | ||
| ], | ||
| isFeatureBranch: true, | ||
| }; | ||
|
|
||
| vi.mock("@/lib/context/BranchContext", () => ({ | ||
| useBranch: () => mockBranchContext, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest, does vi.mock()hoisting require external variables used in the mock factory to be wrapped withvi.hoisted() to avoid TDZ issues?
💡 Result:
Yes—because vi.mock() is hoisted to the top of the file, its factory runs before other top-level code, so referencing external (top-level) variables from inside the factory can hit the temporal dead zone (TDZ) / “not initialized yet” problem. Vitest explicitly warns that you cannot use variables inside the factory that are defined outside the factory. [1]
To safely use shared values in a hoisted mock factory, define them via vi.hoisted(() => ...) (declared before the vi.mock) and reference the returned value inside the factory. [1]
If you don’t want hoisting (and want to freely reference top-level variables), use vi.doMock() instead, since it is not hoisted (but it only affects subsequent imports). [1]
Sources: [1] (vitest.dev)
Citations:
vi.mock() factory cannot safely reference external variables due to hoisting—use vi.hoisted() to avoid runtime TDZ errors.
Vitest hoists vi.mock() calls to the top of the module, causing its factory to execute before other top-level code. Vitest's documentation explicitly prohibits referencing external variables inside the factory. The current code captures mockBranchContext defined outside, which will fail at runtime.
Proposed fix
-import { describe, expect, it, vi } from "vitest";
+import { describe, expect, it, vi } from "vitest";
import { render, screen } from "@testing-library/react";
-// Mock useBranch context
-const mockBranchContext = {
- currentBranch: "feature/add-class",
- defaultBranch: "main",
- branches: [
- { name: "feature/add-class", commits_ahead: 3, commits_behind: 0 },
- ],
- isFeatureBranch: true,
-};
+const { mockBranchContext } = vi.hoisted(() => ({
+ mockBranchContext: {
+ currentBranch: "feature/add-class",
+ defaultBranch: "main",
+ branches: [
+ { name: "feature/add-class", commits_ahead: 3, commits_behind: 0 },
+ ],
+ isFeatureBranch: true,
+ },
+}));
vi.mock("@/lib/context/BranchContext", () => ({
useBranch: () => mockBranchContext,
}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/components/revision/BranchBadge.test.tsx` around lines 5 - 16, The
mock factory for vi.mock currently references the external variable
mockBranchContext (and returns useBranch => mockBranchContext) which will break
due to hoisting; change the mock to use vi.hoisted so the factory creates and
returns the mock object inside the hoisted callback (e.g., replace vi.mock(...,
() => ({ useBranch: () => mockBranchContext })) with vi.hoisted(() => ({
useBranch: () => ({ currentBranch: "feature/add-class", defaultBranch: "main",
branches: [{ name: "feature/add-class", commits_ahead: 3, commits_behind: 0 }],
isFeatureBranch: true }) }))) so the mocked useBranch is self-contained and does
not reference the top-level mockBranchContext variable at module-evaluation
time.
- LanguageFlag: use not.toBeNull() instead of toBeDefined() for querySelector - RemoteSyncIndicator: fix ApiError mock constructor to match (status, statusText, message) - PRListItem: use regex for commits-ahead absence, assert merged date text - BranchBadge: add beforeEach to reset mock state, remove manual restore lines - confirm-dialog: await userEvent.click inside async act - dialog: assert dialog content removed after close click - file-upload: use not.toBeNull() for file input assertion - tooltip: attempt to find tooltip content, fallback to trigger presence - turtleBlockParser: use find() instead of index for rdf:type triple - CommitMessageDialog: rename test to match assertion (disabled button) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
__tests__/components/ui/dialog.test.tsx (1)
121-133:⚠️ Potential issue | 🟡 MinorMake the close-flow assertion async and target the actual close control.
At Line 131,
getByText("Close")clicks the sr-only text node, and Line 133 asserts removal synchronously. This can be flaky for dialog close/unmount timing; prefer clicking by role/name and waiting for removal.✅ Suggested fix
-import { render, screen } from "@testing-library/react"; +import { render, screen, waitFor } from "@testing-library/react"; ... - await userEvent.click(screen.getByText("Close")); - // After closing, dialog content should be removed from DOM - expect(screen.queryByText("Closeable")).toBeNull(); + await userEvent.click(screen.getByRole("button", { name: /close/i })); + await waitFor(() => { + expect(screen.queryByText("Closeable")).toBeNull(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/dialog.test.tsx` around lines 121 - 133, The test currently clicks the SR-only "Close" text and asserts synchronously; update the close flow to click the actual close control and wait for the dialog to unmount. In the "closes dialog when close button is clicked" test, replace the click target with the close button found by role/name (e.g., getByRole('button', { name: /close/i })) and make the post-close assertion asynchronous by waiting for the dialog content to be removed (e.g., using waitForElementToBeRemoved or waitFor on screen.queryByText("Closeable")) so the assertion waits for unmount before checking null.
🧹 Nitpick comments (2)
__tests__/components/pr/PRListItem.test.tsx (1)
167-182: Merged date assertion addresses prior feedback; minor locale caveat.The test now correctly validates that the merged date is rendered (not just the badge). The assertion
/merged Jan 15/assumesformatDate()produces US-English month names. If CI runs with a different locale, this could flake.If
formatDateusesIntl.DateTimeFormat, consider either:
- Ensuring tests run with a consistent locale (e.g.,
LC_ALL=en_US.UTF-8), or- Using a more flexible regex like
/merged.*15/if the month format varies.This is low-risk since most CI environments default to en-US.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/pr/PRListItem.test.tsx` around lines 167 - 182, The test for merged PRs assumes US month names; update the assertion to avoid locale flakiness by matching only the stable parts of the rendered text: change the check that currently uses /merged Jan 15/ to a more flexible match (for example matching "merged" plus the day or using a regex like /merged.*15/), or derive the expected string by calling the same formatting helper (formatDate) used by PRListItem and assert against that; locate the test around PRListItem, makePR and the screen.getByText call to implement the change.__tests__/components/ui/dialog.test.tsx (1)
25-25: Prefer concrete RTL assertions overtoBeDefined()for DOM checks.
getBy*already throws when not found, sotoBeDefined()is mostly redundant. UsequeryBy*+toBeNull/not.toBeNull(ortoBeInTheDocumentif configured) to make intent explicit.Also applies to: 49-50, 65-65, 77-77, 91-91, 106-106, 118-118, 130-130, 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/ui/dialog.test.tsx` at line 25, Replace usages of expect(screen.getByText(...)).toBeDefined() in the dialog tests with explicit DOM assertions: either use expect(screen.getByText("...")).toBeInTheDocument() if jest-dom is configured, or use expect(screen.queryByText("...")).not.toBeNull()/toBeNull() to express presence/absence; update each occurrence (e.g., the assertions that call screen.getByText("Open"), screen.getByText("Hello"), etc.) so they use toBeInTheDocument or queryByText + null checks rather than toBeDefined().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/pr/PRListItem.test.tsx`:
- Around line 131-136: The test is ineffective because it looks for "commits
ahead" which PRListItem never renders; update the assertion to check for the
actual rendered text or element: render <PRListItem pr={makePR({ commits_ahead:
0 })} ... /> and assert that the "commit" label (as produced by PRListItem,
e.g., the <p> containing "commit" or "commits" based on pr.commits_ahead) is not
present — use screen.queryByText(/commit(s)?/) or queryByText(/commit/) or
queryByRole/queryByTestId for the commits element to ensure it returns null when
commits_ahead is 0; keep references to PRListItem and makePR when updating the
test.
---
Duplicate comments:
In `@__tests__/components/ui/dialog.test.tsx`:
- Around line 121-133: The test currently clicks the SR-only "Close" text and
asserts synchronously; update the close flow to click the actual close control
and wait for the dialog to unmount. In the "closes dialog when close button is
clicked" test, replace the click target with the close button found by role/name
(e.g., getByRole('button', { name: /close/i })) and make the post-close
assertion asynchronous by waiting for the dialog content to be removed (e.g.,
using waitForElementToBeRemoved or waitFor on screen.queryByText("Closeable"))
so the assertion waits for unmount before checking null.
---
Nitpick comments:
In `@__tests__/components/pr/PRListItem.test.tsx`:
- Around line 167-182: The test for merged PRs assumes US month names; update
the assertion to avoid locale flakiness by matching only the stable parts of the
rendered text: change the check that currently uses /merged Jan 15/ to a more
flexible match (for example matching "merged" plus the day or using a regex like
/merged.*15/), or derive the expected string by calling the same formatting
helper (formatDate) used by PRListItem and assert against that; locate the test
around PRListItem, makePR and the screen.getByText call to implement the change.
In `@__tests__/components/ui/dialog.test.tsx`:
- Line 25: Replace usages of expect(screen.getByText(...)).toBeDefined() in the
dialog tests with explicit DOM assertions: either use
expect(screen.getByText("...")).toBeInTheDocument() if jest-dom is configured,
or use expect(screen.queryByText("...")).not.toBeNull()/toBeNull() to express
presence/absence; update each occurrence (e.g., the assertions that call
screen.getByText("Open"), screen.getByText("Hello"), etc.) so they use
toBeInTheDocument or queryByText + null checks rather than toBeDefined().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 354ba03d-ec30-4cd3-bf1b-2f57b4573f6e
📒 Files selected for processing (10)
__tests__/components/editor/CommitMessageDialog.test.tsx__tests__/components/editor/LanguageFlag.test.tsx__tests__/components/editor/RemoteSyncIndicator.test.tsx__tests__/components/pr/PRListItem.test.tsx__tests__/components/revision/BranchBadge.test.tsx__tests__/components/ui/confirm-dialog.test.tsx__tests__/components/ui/dialog.test.tsx__tests__/components/ui/file-upload.test.tsx__tests__/components/ui/tooltip.test.tsx__tests__/lib/ontology/turtleBlockParser.test.ts
✅ Files skipped from review due to trivial changes (3)
- tests/components/revision/BranchBadge.test.tsx
- tests/components/editor/LanguageFlag.test.tsx
- tests/lib/ontology/turtleBlockParser.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/components/ui/tooltip.test.tsx
- tests/components/ui/confirm-dialog.test.tsx
- tests/components/editor/CommitMessageDialog.test.tsx
- tests/components/editor/RemoteSyncIndicator.test.tsx
- tests/components/ui/file-upload.test.tsx
Comment on lines
+131
to
+136
| it("does not show commits section when commits_ahead is 0", () => { | ||
| render( | ||
| <PRListItem pr={makePR({ commits_ahead: 0 })} projectId="proj-1" /> | ||
| ); | ||
| expect(screen.queryByText(/commits?\s+ahead/)).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Test assertion will never match — ineffective test.
The regex /commits?\s+ahead/ will never find a match because the component doesn't render "ahead" text. Per PRListItem.tsx lines 158-163, it only renders:
<p className="text-xs text-slate-400">
commit{pr.commits_ahead > 1 ? "s" : ""}
</p>This test passes vacuously and would not catch a regression if the section were incorrectly rendered with commits_ahead: 0.
Proposed fix
it("does not show commits section when commits_ahead is 0", () => {
render(
<PRListItem pr={makePR({ commits_ahead: 0 })} projectId="proj-1" />
);
- expect(screen.queryByText(/commits?\s+ahead/)).toBeNull();
+ expect(screen.queryByText(/^commits?$/)).toBeNull();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/components/pr/PRListItem.test.tsx` around lines 131 - 136, The test
is ineffective because it looks for "commits ahead" which PRListItem never
renders; update the assertion to check for the actual rendered text or element:
render <PRListItem pr={makePR({ commits_ahead: 0 })} ... /> and assert that the
"commit" label (as produced by PRListItem, e.g., the <p> containing "commit" or
"commits" based on pr.commits_ahead) is not present — use
screen.queryByText(/commit(s)?/) or queryByText(/commit/) or
queryByRole/queryByTestId for the commits element to ensure it returns null when
commits_ahead is 0; keep references to PRListItem and makePR when updating the
test.
…alog close - PRListItem: revert to exact text match for "commit"/"commits" absence (the regex /commits?\s+ahead/ never matched the actual rendered text) - PRListItem: use locale-safe /merged.*15/ instead of /merged Jan 15/ - Dialog: use getByRole for close button, waitFor for async unmount Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Exclude type-only files, barrel re-exports, Web Worker, LSP client, and SVG icon from coverage scope (reduces denominator) - Add mockFetch helper with standardized ok/error/empty responses - Add renderWithProviders helper with QueryClient wrapper Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…components Review fixes: - ClassDetailPanel: expose AutoSaveAffordanceBar cancel/save callbacks, rename misleading "no fallback" test title - OntologySourceEditor: wrap all Worker stub tests in try/finally to ensure restore on failure, verify subject IRI renders for diag click - PropertyDetailPanel: replace conditional callback invocations with explicit existence assertions so missing handlers fail the test - StandardEditorLayout: fix localStorage polyfill to return void, use const for capturedOnReparent - TreeNodeContextMenu: prefix unused separatorCount with _ Coverage additions from parallel agents: - TreeNodeContextMenu: 14% → 90%+ (new test file) - StandardEditorLayout: 53% → 80%+ (tab switching, DnD, detail panels) - PropertyAssertionSection: 40% → 80%+ (new test file) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
__tests__/components/editor/ClassDetailPanel.test.tsx (2)
1658-1665:⚠️ Potential issue | 🟡 MinorThe “no fallback provided” branch is still untested.
This case still passes
selectedNodeFallback, so it only checks a mismatched fallback, not theundefinedpath named in the title. Drop the prop here or rename the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 1658 - 1665, The test titled "returns null when classDetail is null and no fallback provided" wrongly supplies selectedNodeFallback, so update the test for component ClassDetailPanel to actually exercise the undefined-fallback branch: remove the selectedNodeFallback prop from the render call (or alternatively rename the test to reflect a mismatched fallback); ensure mockGetClassDetail.mockRejectedValue(new Error("404 Class not found")) remains and assert that the rendered output is null when selectedNodeFallback is not provided.
832-873:⚠️ Potential issue | 🟠 MajorThe cancel-path test still never invokes cancel.
This body still rerenders with a new
classIriand checksmockFlushToGit, so it covers navigation flush, notmockDiscardDraftor exit from edit mode. The stub already exposesdata-testid="cancel-edit"; click that and assert the discard path, or rename the test to the behavior it actually covers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 832 - 873, The test "exits edit mode and discards draft when cancel is invoked" currently only rerenders with a new classIri and asserts mockFlushToGit, so change it to actually exercise the cancel path by clicking the cancel button exposed by the stub (select element with data-testid="cancel-edit") after entering edit mode and then assert that mockDiscardDraft was called and that the edit input is gone; alternatively, if you intend to test navigation-flush behavior keep the rerender/flush assertions but rename the test to reflect navigation flush instead of cancel, updating references to ClassDetailPanel, mockDiscardDraft, mockFlushToGit and the data-testid="cancel-edit" accordingly.__tests__/components/editor/OntologySourceEditor.test.tsx (2)
644-660:⚠️ Potential issue | 🟠 Major
insertAtEndis still unverified here.This case creates local model mocks, but never connects them to the
TurtleEditorstub or assertsexecuteEditswas called. As written, it only proves the method doesn't throw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/OntologySourceEditor.test.tsx` around lines 644 - 660, The test creates local mocks (_executeEditsMock, _getValueMock, _getLineCountMock) but never wires them into the TurtleEditor stub or asserts behavior; update the test to inject these mocks into the TurtleEditor used by OntologySourceEditor (so the editor model returned by the TurtleEditor stub exposes executeEdits = _executeEditsMock, getValue = _getValueMock, getLineCount = _getLineCountMock), call ref.current!.insertAtEnd("\n# new entity") as before, and add an assertion that _executeEditsMock was called with the expected edits (verifying insertAtEnd triggered executeEdits on the model).
1069-1131:⚠️ Potential issue | 🟠 MajorThe navigation callback is still never exercised.
The test renders the problem item and stops there. Without clicking it and asserting
onNavigateToClass("http://example.org/ontology#Person"), this remains a false positive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/OntologySourceEditor.test.tsx` around lines 1069 - 1131, The test currently only renders the issue but never triggers the navigation; after rendering and waiting for the issue text ("Navigate test" or "Person") to appear, simulate a user click on that rendered problem item (use the element returned by screen.getByText for "Navigate test" or "Person") and then assert that the mock onNavigateToClass was called with "http://example.org/ontology#Person"; update the test around the render of <OntologySourceEditor ... onNavigateToClass={onNavigateToClass} /> to perform the click and expect(onNavigateToClass).toHaveBeenCalledWith("http://example.org/ontology#Person"). Ensure this uses the existing DiagWorker and mockGetIssues setup so the element is clickable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 174-179: The assertion in the "shows a loading spinner while
fetching" test is using toBeDefined() on
container.querySelector(".animate-spin") which can pass when the spinner is
missing; update the expectation in that test (the one that mocks
mockGetClassDetail to never resolve and renders ClassDetailPanel) to use
expect(container.querySelector(".animate-spin")).not.toBeNull() instead so the
querySelector check fails when no element is found.
In `@__tests__/components/editor/OntologySourceEditor.test.tsx`:
- Around line 761-823: The test "clicking a problem issue calls scrollToLine
with the issue position" currently only checks rendered text and must assert an
editor-side effect; modify the TurtleEditor mock used by the component to expose
and spy on revealLineInCenter (or setPosition) and, in the test, after clicking
the issue button assert that revealLineInCenter (or setPosition) was called with
the expected line/column (e.g., line 4, col 1, length 39) — restore the original
Worker afterward as before.
In `@__tests__/components/editor/PropertyDetailPanel.test.tsx`:
- Around line 694-705: The test for PropertyDetailPanel is asserting
mockFlushToGit immediately after rerender, which is race-prone because
flushToGit is triggered by an effect on propertyIri; change the assertion to
wait for the effect by wrapping the expect(mockFlushToGit).toHaveBeenCalled()
inside a waitFor(...) and ensure waitFor is imported from `@testing-library/react`
so the test waits for the flush triggered by the propertyIri change before
asserting.
- Around line 626-639: The test "exits edit mode and calls discardDraft when
cancel flow is triggered" currently only enters edit mode; modify it to trigger
the cancel flow by invoking the captured onCancel callback from the edit
controls (or clicking the cancel control if the stub exposes one) and then
assert that mockDiscardDraft was called and that the component exited edit mode
(e.g., auto-save-bar is gone). Locate the test around PropertyDetailPanel render
with DEFAULT_PROPS and onUpdateProperty, grab the onCancel passed into the
editable child (or find the cancel button) and call it, then
expect(mockDiscardDraft).toHaveBeenCalled() and
expect(screen.queryByTestId("auto-save-bar")).toBeNull().
- Around line 49-52: The mock for useEditorModeStore uses the wrong key —
provide the real state shape by exposing editorMode instead of mode so selectors
that read state.editorMode see correct values; update the mocked return object
in the useEditorModeStore mock (currently referencing editorModeOverrides) to
include editorMode: { mode: "standard", continuousEditing: false,
...editorModeOverrides } (preserving editorModeOverrides usage) so tests reflect
production state.
In `@__tests__/components/editor/standard/PropertyAssertionSection.test.tsx`:
- Around line 39-41: The beforeEach currently calls vi.clearAllMocks() which
only clears call history; update the setup to also reset the mock implementation
for searchMock by calling searchMock.mockReset() (or searchMock.mockRestore() if
you prefer restoring the original implementation) inside the beforeEach block
after vi.clearAllMocks(), so each test gets a fresh mock implementation and
previous mockResolvedValue/mockImplementation/mockRejectedValue do not leak.
In `@__tests__/components/editor/standard/StandardEditorLayout.test.tsx`:
- Around line 841-860: The test doesn't exercise OntologyGraph navigation
because the next/dynamic mock only renders a static ontology-graph div; update
the mock for OntologyGraph used by StandardEditorLayout so it forwards and
preserves props (including onNavigateToClass), then in the test
(StandardEditorLayout.test.tsx) after switching to "Graph" obtain the rendered
mock component and call its onNavigateToClass prop (or simulate the user action
that triggers it) to assert that the navigateToNode mock was invoked; reference
the OntologyGraph component, its onNavigateToClass prop, the
StandardEditorLayout render, and the navigateToNode mock when making the change.
---
Duplicate comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 1658-1665: The test titled "returns null when classDetail is null
and no fallback provided" wrongly supplies selectedNodeFallback, so update the
test for component ClassDetailPanel to actually exercise the undefined-fallback
branch: remove the selectedNodeFallback prop from the render call (or
alternatively rename the test to reflect a mismatched fallback); ensure
mockGetClassDetail.mockRejectedValue(new Error("404 Class not found")) remains
and assert that the rendered output is null when selectedNodeFallback is not
provided.
- Around line 832-873: The test "exits edit mode and discards draft when cancel
is invoked" currently only rerenders with a new classIri and asserts
mockFlushToGit, so change it to actually exercise the cancel path by clicking
the cancel button exposed by the stub (select element with
data-testid="cancel-edit") after entering edit mode and then assert that
mockDiscardDraft was called and that the edit input is gone; alternatively, if
you intend to test navigation-flush behavior keep the rerender/flush assertions
but rename the test to reflect navigation flush instead of cancel, updating
references to ClassDetailPanel, mockDiscardDraft, mockFlushToGit and the
data-testid="cancel-edit" accordingly.
In `@__tests__/components/editor/OntologySourceEditor.test.tsx`:
- Around line 644-660: The test creates local mocks (_executeEditsMock,
_getValueMock, _getLineCountMock) but never wires them into the TurtleEditor
stub or asserts behavior; update the test to inject these mocks into the
TurtleEditor used by OntologySourceEditor (so the editor model returned by the
TurtleEditor stub exposes executeEdits = _executeEditsMock, getValue =
_getValueMock, getLineCount = _getLineCountMock), call
ref.current!.insertAtEnd("\n# new entity") as before, and add an assertion that
_executeEditsMock was called with the expected edits (verifying insertAtEnd
triggered executeEdits on the model).
- Around line 1069-1131: The test currently only renders the issue but never
triggers the navigation; after rendering and waiting for the issue text
("Navigate test" or "Person") to appear, simulate a user click on that rendered
problem item (use the element returned by screen.getByText for "Navigate test"
or "Person") and then assert that the mock onNavigateToClass was called with
"http://example.org/ontology#Person"; update the test around the render of
<OntologySourceEditor ... onNavigateToClass={onNavigateToClass} /> to perform
the click and
expect(onNavigateToClass).toHaveBeenCalledWith("http://example.org/ontology#Person").
Ensure this uses the existing DiagWorker and mockGetIssues setup so the element
is clickable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df06ddbd-cd12-4815-a5d7-cfdb2313ce37
📒 Files selected for processing (6)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/OntologySourceEditor.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx__tests__/components/editor/TreeNodeContextMenu.test.tsx__tests__/components/editor/standard/PropertyAssertionSection.test.tsx__tests__/components/editor/standard/StandardEditorLayout.test.tsx
Comment on lines
+761
to
+823
| it("clicking a problem issue calls scrollToLine with the issue position", async () => { | ||
| // Set up worker to return positions for the issue | ||
| const OrigWorker = globalThis.Worker; | ||
| class PositionWorker { | ||
| onmessage: ((e: MessageEvent) => void) | null = null; | ||
| onerror: ((e: ErrorEvent) => void) | null = null; | ||
| postMessage() { | ||
| setTimeout(() => { | ||
| this.onmessage?.({ | ||
| data: { | ||
| diagnostics: [{ | ||
| issueId: "i1", | ||
| startLineNumber: 4, | ||
| startColumn: 1, | ||
| endLineNumber: 4, | ||
| endColumn: 40, | ||
| message: "[ERR1] Missing label", | ||
| severity: "error", | ||
| }], | ||
| positions: [["i1", { line: 4, col: 1, len: 39 }]], | ||
| iriIndex: [], | ||
| iriLabels: [], | ||
| stats: { linesProcessed: 5, irisIndexed: 1, localNamesIndexed: 1, issuesMatched: 1, timeMs: 5 }, | ||
| }, | ||
| } as unknown as MessageEvent); | ||
| }, 0); | ||
| } | ||
| terminate() { /* no-op */ } | ||
| } | ||
| vi.stubGlobal("Worker", PositionWorker); | ||
|
|
||
| mockGetIssues.mockResolvedValue({ | ||
| items: [ | ||
| { | ||
| id: "i1", run_id: "r1", project_id: "proj-1", issue_type: "error", | ||
| rule_id: "ERR1", message: "Missing label", | ||
| subject_iri: "http://example.org/A", | ||
| details: null, created_at: "2024-01-01T00:00:00Z", resolved_at: null, | ||
| }, | ||
| ], | ||
| total: 1, | ||
| skip: 0, | ||
| limit: 200, | ||
| }); | ||
|
|
||
| try { | ||
| const user = userEvent.setup(); | ||
| render(<OntologySourceEditor {...DEFAULT_PROPS} />); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Missing label")).toBeDefined(); | ||
| }); | ||
|
|
||
| const issueButton = screen.getByText("Missing label").closest("button")!; | ||
| await user.click(issueButton); | ||
|
|
||
| await waitFor(() => { | ||
| expect(screen.getByText("Ln 4")).toBeDefined(); | ||
| }); | ||
| } finally { | ||
| vi.stubGlobal("Worker", OrigWorker); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The problem-click test doesn't assert an editor-side effect.
After the click, it only checks for rendered problem text/line metadata. That doesn't prove the click handler scrolled the editor. Expose revealLineInCenter or setPosition from the TurtleEditor mock and assert one of them after clicking the issue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/components/editor/OntologySourceEditor.test.tsx` around lines 761 -
823, The test "clicking a problem issue calls scrollToLine with the issue
position" currently only checks rendered text and must assert an editor-side
effect; modify the TurtleEditor mock used by the component to expose and spy on
revealLineInCenter (or setPosition) and, in the test, after clicking the issue
button assert that revealLineInCenter (or setPosition) was called with the
expected line/column (e.g., line 4, col 1, length 39) — restore the original
Worker afterward as before.
Comment on lines
+841
to
+860
| it("hides graph and navigates when OntologyGraph onNavigateToClass fires", () => { | ||
| const navigateToNode = vi.fn().mockResolvedValue(undefined); | ||
| // We need the OntologyGraph mock to expose onNavigateToClass | ||
| render( | ||
| <StandardEditorLayout | ||
| {...defaultProps({ | ||
| nodes: sampleNodes, | ||
| selectedIri: "http://example.org/ClassA", | ||
| navigateToNode, | ||
| })} | ||
| /> | ||
| ); | ||
| // Switch to graph | ||
| fireEvent.click(screen.getByText("Graph")); | ||
| expect(screen.getByTestId("ontology-graph")).toBeDefined(); | ||
|
|
||
| // Click Back to Details | ||
| fireEvent.click(screen.getByText("Back to Details")); | ||
| // Should show class detail panel again | ||
| expect(screen.getByTestId("class-detail-panel")).toBeDefined(); |
There was a problem hiding this comment.
This test never exercises OntologyGraph navigation.
The current next/dynamic mock only renders a bare ontology-graph div, so this case just re-tests the Back button path already covered above. A regression in StandardEditorLayout’s onNavigateToClass → navigateToNode wiring would still pass here. Preserve the graph props in the mock and invoke onNavigateToClass from the test so navigateToNode is actually asserted.
Suggested test shape
+let _graphProps: Record<string, unknown> | null = null;
vi.mock("next/dynamic", () => ({
__esModule: true,
- default: () => () => <div data-testid="ontology-graph" />,
+ default: () => (props: Record<string, unknown>) => {
+ _graphProps = props;
+ return (
+ <button
+ data-testid="ontology-graph"
+ onClick={() =>
+ (props.onNavigateToClass as ((iri: string) => void) | undefined)?.(
+ "http://example.org/Target",
+ )}
+ />
+ );
+ },
}));
...
it("hides graph and navigates when OntologyGraph onNavigateToClass fires", () => {
const navigateToNode = vi.fn().mockResolvedValue(undefined);
render(
<StandardEditorLayout
{...defaultProps({
nodes: sampleNodes,
selectedIri: "http://example.org/ClassA",
navigateToNode,
})}
/>
);
fireEvent.click(screen.getByText("Graph"));
- expect(screen.getByTestId("ontology-graph")).toBeDefined();
-
- // Click Back to Details
- fireEvent.click(screen.getByText("Back to Details"));
- // Should show class detail panel again
+ fireEvent.click(screen.getByTestId("ontology-graph"));
+ expect(navigateToNode).toHaveBeenCalledWith("http://example.org/Target");
expect(screen.getByTestId("class-detail-panel")).toBeDefined();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@__tests__/components/editor/standard/StandardEditorLayout.test.tsx` around
lines 841 - 860, The test doesn't exercise OntologyGraph navigation because the
next/dynamic mock only renders a static ontology-graph div; update the mock for
OntologyGraph used by StandardEditorLayout so it forwards and preserves props
(including onNavigateToClass), then in the test (StandardEditorLayout.test.tsx)
after switching to "Graph" obtain the rendered mock component and call its
onNavigateToClass prop (or simulate the user action that triggers it) to assert
that the navigateToNode mock was invoked; reference the OntologyGraph component,
its onNavigateToClass prop, the StandardEditorLayout render, and the
navigateToNode mock when making the change.
- ClassDetailPanel: use not.toBeNull() for spinner assertion, split cancel test to actually click cancel button and verify discardDraft, rename misleading fallback test title - PropertyDetailPanel: fix editorModeStore mock key to editorMode, wrap flushToGit assertion in waitFor, complete cancel test to trigger onCancel and assert discardDraft - PropertyAssertionSection: reset searchMock in beforeEach to prevent mock implementation leakage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
__tests__/components/editor/PropertyDetailPanel.test.tsx (1)
626-649:⚠️ Potential issue | 🟡 MinorAssert edit-mode exit after cancel, not just
discardDraft().Both cancel tests stop at
mockDiscardDraft, socancelEditModecan regress and leave the toolbar open while the suite stays green. Please verify the UI exits edit mode as well, e.g.auto-save-bardisappears orEdit Itembecomes visible again.Also applies to: 1159-1175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/PropertyDetailPanel.test.tsx` around lines 626 - 649, The test only asserts mockDiscardDraft was called but doesn't confirm edit mode exited; update the test for PropertyDetailPanel to also assert the UI leaves edit mode after invoking the capturedAutoSaveBarProps.onCancel: after calling onCancel(), waitFor that the "auto-save-bar" test id is no longer in the document or that the "Edit Item" button/text is visible again, and keep the existing expect(mockDiscardDraft).toHaveBeenCalled() so both the discardDraft call and the UI state change (edit mode exit) are verified; apply the same change to the other cancel test that uses capturedAutoSaveBarProps and mockDiscardDraft.__tests__/components/editor/ClassDetailPanel.test.tsx (1)
832-859:⚠️ Potential issue | 🟡 MinorThe cancel-path test still doesn't prove edit mode closed.
This only asserts
mockDiscardDraft. A brokenonCancelthat discards the draft but leaves the panel in edit mode would still pass. Please also assert that the edit UI closes, e.g. the auto-save bar disappears orEdit Itemreturns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 832 - 859, The test "exits edit mode and discards draft when cancel is invoked" currently only asserts mockDiscardDraft was called; update it to also assert that the edit UI closed by checking for a visible "Edit Item" button (or that the AutoSaveAffordanceBar is gone) after clicking the cancel button; locate the test in ClassDetailPanel.test.tsx (the it block with onUpdateClass and mockDiscardDraft) and add a waitFor/assertion such as expect(screen.getByText("Edit Item")).not.toBeNull() or expect(screen.queryByTestId("auto-save-affordance-bar")).toBeNull() to prove edit mode exited.
🧹 Nitpick comments (1)
__tests__/components/editor/standard/PropertyAssertionSection.test.tsx (1)
1028-1033: Use fake timers instead of fixed timeout.The fixed 500ms
setTimeoutmakes this test slow and potentially flaky. Since the debounce is 300ms, use Vitest's fake timers for deterministic, instant execution.Suggested refactor using fake timers
it("does not search entities when assertionType is data", async () => { + vi.useFakeTimers(); + render( <PropertyAssertionSection {...baseProps} assertionType="data" isEditing={true} onAdd={vi.fn()} /> ); await selectProperty("label", "http://example.org/ontology#label", "lab"); // Clear mock call count after property selection searchMock.mockClear(); // The data value input does not trigger entity search const valueInput = screen.getByPlaceholderText("Enter value..."); fireEvent.change(valueInput, { target: { value: "some value" } }); - // Wait a bit to ensure no search is triggered - await new Promise((resolve) => setTimeout(resolve, 500)); + // Advance past debounce period + await vi.advanceTimersByTimeAsync(500); // searchEntities should not be called for data type values expect(searchMock).not.toHaveBeenCalled(); + + vi.useRealTimers(); });Note: If
selectPropertyuses real async operations internally, you may need to wrap it withvi.useRealTimers()or usevi.advanceTimersByTimeAsync()within the helper as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/standard/PropertyAssertionSection.test.tsx` around lines 1028 - 1033, Replace the fixed 500ms setTimeout with Vitest fake timers: call vi.useFakeTimers() before triggering the debounced behavior (the action that ultimately calls searchMock — e.g., selectProperty or the input event used in this test), then advance timers by the debounce duration using vi.advanceTimersByTimeAsync(300) (or slightly above, e.g., 350ms) and assert expect(searchMock).not.toHaveBeenCalled(); finally restore real timers with vi.useRealTimers(). Ensure any helper that performs real async work is wrapped with vi.useRealTimers()/reverted as needed so the fake timer advance reliably triggers the debounce.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/editor/PropertyDetailPanel.test.tsx`:
- Around line 1218-1240: The test "passes comment callbacks to AnnotationRow"
only calls the capturedAnnotationRowProps callbacks (onValueChange,
onLangChange) but never asserts any effect, so regressions to no-op handlers
would still pass; update the test for PropertyDetailPanel to provide a
verifiable postcondition by using the onUpdateProperty mock (or observing
rendered output) to assert the change: after calling
commentRow!.onValueChange("Updated comment") and commentRow!.onLangChange("fr"),
verify that onUpdateProperty was called with the expected payload (or that the
DOM shows the updated comment text/language), and similarly add assertions for
the other tests referenced (the blocks around lines 1244-1261, 1425-1440,
1444-1474) to confirm the callbacks actually propagate changes rather than
simply being callable.
---
Duplicate comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 832-859: The test "exits edit mode and discards draft when cancel
is invoked" currently only asserts mockDiscardDraft was called; update it to
also assert that the edit UI closed by checking for a visible "Edit Item" button
(or that the AutoSaveAffordanceBar is gone) after clicking the cancel button;
locate the test in ClassDetailPanel.test.tsx (the it block with onUpdateClass
and mockDiscardDraft) and add a waitFor/assertion such as
expect(screen.getByText("Edit Item")).not.toBeNull() or
expect(screen.queryByTestId("auto-save-affordance-bar")).toBeNull() to prove
edit mode exited.
In `@__tests__/components/editor/PropertyDetailPanel.test.tsx`:
- Around line 626-649: The test only asserts mockDiscardDraft was called but
doesn't confirm edit mode exited; update the test for PropertyDetailPanel to
also assert the UI leaves edit mode after invoking the
capturedAutoSaveBarProps.onCancel: after calling onCancel(), waitFor that the
"auto-save-bar" test id is no longer in the document or that the "Edit Item"
button/text is visible again, and keep the existing
expect(mockDiscardDraft).toHaveBeenCalled() so both the discardDraft call and
the UI state change (edit mode exit) are verified; apply the same change to the
other cancel test that uses capturedAutoSaveBarProps and mockDiscardDraft.
---
Nitpick comments:
In `@__tests__/components/editor/standard/PropertyAssertionSection.test.tsx`:
- Around line 1028-1033: Replace the fixed 500ms setTimeout with Vitest fake
timers: call vi.useFakeTimers() before triggering the debounced behavior (the
action that ultimately calls searchMock — e.g., selectProperty or the input
event used in this test), then advance timers by the debounce duration using
vi.advanceTimersByTimeAsync(300) (or slightly above, e.g., 350ms) and assert
expect(searchMock).not.toHaveBeenCalled(); finally restore real timers with
vi.useRealTimers(). Ensure any helper that performs real async work is wrapped
with vi.useRealTimers()/reverted as needed so the fake timer advance reliably
triggers the debounce.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b6c6f4b-2051-4755-9ef8-799827097a62
📒 Files selected for processing (3)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx__tests__/components/editor/standard/PropertyAssertionSection.test.tsx
…ions - ClassDetailPanel: assert "Edit Item" visible after cancel to verify edit mode actually exited - PropertyDetailPanel: assert "Edit Item" visible after cancel, verify AnnotationRow callbacks propagate updated values/languages to re-rendered props instead of asserting triggerSave (which only fires on remove) - Rename duplicate fallback test title Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
Coverage Report (Updated)
Coverage by directory
Progress from starting point
|
Detail panels (53 new tests): - ClassDetailPanel: 78.5% → 85%+ (draft restoration, annotation callbacks, relationship callbacks, continuous editing, target label resolution fallback) - PropertyDetailPanel: 78% → 85%+ (annotation adder, relationship save, manual save failure, cancel re-entry prevention) - IndividualDetailPanel: 79% → 85%+ (assertion callbacks, annotation adder, data property lang flags, save failure handling) Shared editor components (38 new tests): - EntityTree: 75% → 97% (keyboard nav, focus, disabled state) - EntityTreeNode: 77% → 100% (double-click, drag, context menu, depth) - NormalizationDiffViewer: 62.5% → 100% (mount, dispose, language reg) - EntityHistoryTab: 76% → 100% (time formatting, collapse, fallbacks) Overall line coverage: 87.8% → 89.4% Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (6)
__tests__/components/editor/IndividualDetailPanel.test.tsx (3)
1261-1268:⚠️ Potential issue | 🟠 MajorDon't hide missing
onRemovewiring behind conditionals.Each block only exercises the assertion path if
onRemoveexists, so a missing callback leaves the test green. Assert the handler exists first, then invoke it.Suggested tightening
- if (firstCommentRow && firstCommentRow.onRemove) { - const onRemove = firstCommentRow.onRemove as () => void; - onRemove(); - await waitFor(() => { - expect(mockTriggerSave).toHaveBeenCalled(); - }); - } + expect(firstCommentRow).toBeDefined(); + expect(firstCommentRow!.onRemove).toBeDefined(); + const onRemove = firstCommentRow!.onRemove as () => void; + onRemove(); + await waitFor(() => { + expect(mockTriggerSave).toHaveBeenCalled(); + });Also applies to: 1296-1303, 1561-1568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/IndividualDetailPanel.test.tsx` around lines 1261 - 1268, The test currently guards invocation of the remove handler with a conditional, which can hide missing wiring; instead assert the handler exists then call it: for the block using commentRows[0] (firstCommentRow) assert expect(firstCommentRow).toBeDefined() and expect(firstCommentRow.onRemove).toBeDefined(), cast onRemove = firstCommentRow.onRemove as () => void, invoke onRemove(), then await waitFor(() => expect(mockTriggerSave).toHaveBeenCalled()); apply the same change to the other similar blocks that use commentRows (the ones around the onRemove usages referenced in the comment).
165-175:⚠️ Potential issue | 🟠 MajorUse
not.toBeNull()for the spinner query.
querySelector()returnsnullon a miss, andtoBeDefined()still passes fornull, so this loading-state test can stay green when the spinner disappears.Suggested fix
- expect(container.querySelector(".animate-spin")).toBeDefined(); + expect(container.querySelector(".animate-spin")).not.toBeNull();#!/bin/bash set -euo pipefail sed -n '165,175p' __tests__/components/editor/IndividualDetailPanel.test.tsx node <<'EOF' const value = null; console.log("querySelector miss returns null:", value === null); console.log("expect(null).toBeDefined() would pass:", value !== undefined); console.log("expect(null).not.toBeNull() would pass:", value !== null); EOF🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/IndividualDetailPanel.test.tsx` around lines 165 - 175, The assertion in the "shows a loading spinner when sourceContent is empty" test uses expect(container.querySelector(".animate-spin")).toBeDefined(), which can falsely pass when querySelector returns null; update the assertion to use expect(container.querySelector(".animate-spin")).not.toBeNull() so a missing spinner correctly fails—locate the test in IndividualDetailPanel.test.tsx (the it("shows a loading spinner when sourceContent is empty") block) and replace the toBeDefined() call with not.toBeNull().
1173-1192:⚠️ Potential issue | 🟠 MajorThese callback tests still pass with no-op handlers.
They invoke
onValueChange/onLangChangeand stop, so the suite never proves the panel rerendered with updatedAnnotationRowprops. Reuse the postcondition pattern already used in the sibling detail-panel tests and assert the new value/lang appears incapturedAnnotationRowProps.Also applies to: 1196-1213, 1502-1531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/IndividualDetailPanel.test.tsx` around lines 1173 - 1192, The test currently calls commentRow!.onValueChange and onLangChange but doesn't assert postconditions, so it passes even if handlers are no-ops; update the test for IndividualDetailPanel to follow the existing postcondition pattern used in sibling detail-panel tests by invoking commentRow!.onValueChange("Updated comment") and commentRow!.onLangChange("fr") and then re-checking capturedAnnotationRowProps for the comment property's entry (the same lookup using propertyIri == "http://www.w3.org/2000/01/rdf-schema#comment") to assert the AnnotationRow props now contain the updated value and lang; use the same helper/assertion style already used elsewhere in the file so the test fails if the panel does not actually pass updated props through.__tests__/components/editor/ClassDetailPanel.test.tsx (2)
2030-2052:⚠️ Potential issue | 🟠 MajorAssert a postcondition after
onAddGroup().This test stops right after invoking the callback, so a no-op handler still passes. Check that
RelationshipSectionrerenders with one more group, or thatcapturedRelationshipSectionProps.groups.lengthincreases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 2030 - 2052, The test currently invokes the captured onAddGroup callback but doesn't assert any postcondition; update the test to verify the effect of calling onAddGroup by asserting that capturedRelationshipSectionProps.groups length increases (or that RelationshipSection rerenders with one more group) after calling onAddGroup; locate the callback via capturedRelationshipSectionProps!.onAddGroup and then assert either capturedRelationshipSectionProps!.groups.length has incremented or that the rendered output from RelationshipSection shows the new group (e.g., by checking for expected new-group text or count) to ensure the handler is not a no-op.
51-54:⚠️ Potential issue | 🟠 MajorMirror the real editor-mode store shape in this mock.
This mock still exposes
mode, so any selector readingstate.editorModewill seeundefinedand the suite can pass against state the component never gets in production. Apply the same fix to the identical mock in__tests__/components/editor/IndividualDetailPanel.test.tsx.Suggested fix
vi.mock("@/lib/stores/editorModeStore", () => ({ useEditorModeStore: (selector: (s: Record<string, unknown>) => unknown) => - selector({ mode: "standard", continuousEditing: false, ...editorModeOverrides }), + selector({ + editorMode: "standard", + continuousEditing: false, + ...editorModeOverrides, + }), }));#!/bin/bash set -euo pipefail echo "== editorModeStore definition ==" fd 'editorModeStore.ts' -x sed -n '1,200p' {} echo echo "== ClassDetailPanel store usage ==" fd 'ClassDetailPanel.tsx' -x rg -n -C2 'useEditorModeStore|editorMode|mode' {} echo echo "== IndividualDetailPanel store usage ==" fd 'IndividualDetailPanel.tsx' -x rg -n -C2 'useEditorModeStore|editorMode|mode' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 51 - 54, The test mock for useEditorModeStore returns an object with top-level keys like mode and continuousEditing instead of matching the real store shape (which exposes state under editorMode), so selectors reading state.editorMode get undefined; update the mock in the ClassDetailPanel test (and mirror the change in IndividualDetailPanel.test) so useEditorModeStore(selector) passes an object shaped like the real store (e.g., { editorMode: { mode: "standard", continuousEditing: false, ...editorModeOverrides }, ... } ) ensuring selectors that access state.editorMode and other real keys behave the same as in production.__tests__/components/editor/PropertyDetailPanel.test.tsx (1)
1455-1470:⚠️ Potential issue | 🟠 Major
onAddGroup()still needs a postcondition.This test ends immediately after calling the callback, so it passes even if the handler is disconnected. Assert that
RelationshipSectionrerenders with an extra group, or thatgroups.lengthincreases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/PropertyDetailPanel.test.tsx` around lines 1455 - 1470, The test calls the capturedRelationshipSectionProps.onAddGroup but lacks a postcondition; after invoking onAddGroup() in the PropertyDetailPanel test, assert that RelationshipSection actually rerenders with an additional group by capturing the groups length before and after or by querying the DOM for the rendered group items and expecting the count to increase; use capturedRelationshipSectionProps (or screen queries) to get the initial groups length, call onAddGroup(), await any necessary re-render, then assert groups.length increased or that an extra group element is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 2030-2052: The test currently invokes the captured onAddGroup
callback but doesn't assert any postcondition; update the test to verify the
effect of calling onAddGroup by asserting that
capturedRelationshipSectionProps.groups length increases (or that
RelationshipSection rerenders with one more group) after calling onAddGroup;
locate the callback via capturedRelationshipSectionProps!.onAddGroup and then
assert either capturedRelationshipSectionProps!.groups.length has incremented or
that the rendered output from RelationshipSection shows the new group (e.g., by
checking for expected new-group text or count) to ensure the handler is not a
no-op.
- Around line 51-54: The test mock for useEditorModeStore returns an object with
top-level keys like mode and continuousEditing instead of matching the real
store shape (which exposes state under editorMode), so selectors reading
state.editorMode get undefined; update the mock in the ClassDetailPanel test
(and mirror the change in IndividualDetailPanel.test) so
useEditorModeStore(selector) passes an object shaped like the real store (e.g.,
{ editorMode: { mode: "standard", continuousEditing: false,
...editorModeOverrides }, ... } ) ensuring selectors that access
state.editorMode and other real keys behave the same as in production.
In `@__tests__/components/editor/IndividualDetailPanel.test.tsx`:
- Around line 1261-1268: The test currently guards invocation of the remove
handler with a conditional, which can hide missing wiring; instead assert the
handler exists then call it: for the block using commentRows[0]
(firstCommentRow) assert expect(firstCommentRow).toBeDefined() and
expect(firstCommentRow.onRemove).toBeDefined(), cast onRemove =
firstCommentRow.onRemove as () => void, invoke onRemove(), then await waitFor(()
=> expect(mockTriggerSave).toHaveBeenCalled()); apply the same change to the
other similar blocks that use commentRows (the ones around the onRemove usages
referenced in the comment).
- Around line 165-175: The assertion in the "shows a loading spinner when
sourceContent is empty" test uses
expect(container.querySelector(".animate-spin")).toBeDefined(), which can
falsely pass when querySelector returns null; update the assertion to use
expect(container.querySelector(".animate-spin")).not.toBeNull() so a missing
spinner correctly fails—locate the test in IndividualDetailPanel.test.tsx (the
it("shows a loading spinner when sourceContent is empty") block) and replace the
toBeDefined() call with not.toBeNull().
- Around line 1173-1192: The test currently calls commentRow!.onValueChange and
onLangChange but doesn't assert postconditions, so it passes even if handlers
are no-ops; update the test for IndividualDetailPanel to follow the existing
postcondition pattern used in sibling detail-panel tests by invoking
commentRow!.onValueChange("Updated comment") and commentRow!.onLangChange("fr")
and then re-checking capturedAnnotationRowProps for the comment property's entry
(the same lookup using propertyIri ==
"http://www.w3.org/2000/01/rdf-schema#comment") to assert the AnnotationRow
props now contain the updated value and lang; use the same helper/assertion
style already used elsewhere in the file so the test fails if the panel does not
actually pass updated props through.
In `@__tests__/components/editor/PropertyDetailPanel.test.tsx`:
- Around line 1455-1470: The test calls the
capturedRelationshipSectionProps.onAddGroup but lacks a postcondition; after
invoking onAddGroup() in the PropertyDetailPanel test, assert that
RelationshipSection actually rerenders with an additional group by capturing the
groups length before and after or by querying the DOM for the rendered group
items and expecting the count to increase; use capturedRelationshipSectionProps
(or screen queries) to get the initial groups length, call onAddGroup(), await
any necessary re-render, then assert groups.length increased or that an extra
group element is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1efbd24f-6125-41db-83a3-7fd3c4489a23
📒 Files selected for processing (7)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/EntityHistoryTab.test.tsx__tests__/components/editor/IndividualDetailPanel.test.tsx__tests__/components/editor/NormalizationDiffViewer.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx__tests__/components/editor/shared/EntityTree.test.tsx__tests__/components/editor/shared/EntityTreeNode.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/components/editor/EntityHistoryTab.test.tsx
- tests/components/editor/NormalizationDiffViewer.test.tsx
- tests/components/editor/shared/EntityTree.test.tsx
- tests/components/editor/shared/EntityTreeNode.test.tsx
- ClassDetailPanel/IndividualDetailPanel: fix editorModeStore mock key to editorMode (matching real store shape) - ClassDetailPanel/PropertyDetailPanel: assert onAddGroup postcondition by verifying groups length increases after calling the callback - IndividualDetailPanel: use not.toBeNull() for spinner assertion, replace conditional onRemove guards with explicit existence assertions, add postcondition assertions for comment value/lang callbacks Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
__tests__/components/editor/ClassDetailPanel.test.tsx (1)
2521-2534:⚠️ Potential issue | 🟡 MinorRename this test or change the setup; it currently documents the opposite branch.
The title says “no matching fallback”, but Line 2528 passes a fallback whose IRI matches
classIri, and the assertion expects the unsaved fallback card. That makes this case misleading to read and easy to mistake for missing branch coverage.✏️ Suggested rename
-it("returns null when classDetail is null and no matching fallback", async () => { +it("shows the unsaved fallback card when the fallback IRI matches", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 2521 - 2534, The test titled "returns null when classDetail is null and no matching fallback" is misconfigured because it supplies a selectedNodeFallback whose iri matches the component's classIri and then asserts the fallback card is shown; either rename the test to reflect the "matching fallback" case or change the fallback. Fix by updating the it(...) description to something like "shows fallback card when classDetail is null and matching fallback provided" (or change selectedNodeFallback.iri to a non-matching value to exercise the no-matching-fallback branch), referencing the mockGetClassDetail rejection, ClassDetailPanel props selectedNodeFallback, and the expectation that the unsaved fallback card text (/has not been saved yet/) is rendered.
🧹 Nitpick comments (3)
__tests__/components/editor/IndividualDetailPanel.test.tsx (1)
613-624: Use async assertions for effect-driven edit-mode entry to reduce flakiness.Lines 613 and 628 assert immediately after render, but edit-mode entry is triggered by
useEffect. These should await UI stabilization.Proposed fix
- it("auto-enters edit mode when continuousEditing is true", () => { + it("auto-enters edit mode when continuousEditing is true", async () => { editorModeOverrides = { continuousEditing: true }; const onUpdateIndividual = vi.fn(); render( <IndividualDetailPanel {...DEFAULT_PROPS} canEdit={true} onUpdateIndividual={onUpdateIndividual} /> ); - expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); + await waitFor(() => { + expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); + }); }); - it("auto-enters edit mode when restoredDraft is available", () => { + it("auto-enters edit mode when restoredDraft is available", async () => { autoSaveOverrides = { restoredDraft: { entityType: "individual", labels: [{ value: "Restored Name", lang: "en" }], comments: [{ value: "Restored Comment", lang: "en" }], definitions: [], typeIris: ["http://example.org/ontology#Person"], sameAsIris: [], differentFromIris: [], objectPropertyAssertions: [], dataPropertyAssertions: [], annotations: [], relationships: [], deprecated: false, updatedAt: Date.now(), }, }; const onUpdateIndividual = vi.fn(); render( <IndividualDetailPanel {...DEFAULT_PROPS} canEdit={true} onUpdateIndividual={onUpdateIndividual} /> ); - expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); - expect(mockClearRestoredDraft).toHaveBeenCalled(); + await waitFor(() => { + expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); + expect(mockClearRestoredDraft).toHaveBeenCalled(); + }); });Also applies to: 628-656
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/IndividualDetailPanel.test.tsx` around lines 613 - 624, The tests assert synchronously right after render but the component enters edit mode inside a useEffect, causing flakiness; update the tests (e.g., the "auto-enters edit mode when continuousEditing is true" case and the other affected cases) to use async assertions—either await screen.findByTestId("auto-save-bar") or wrap the expectation in await waitFor(() => expect(...).not.toBeNull())—so the test waits for the effect-driven UI change from the IndividualDetailPanel to stabilize before asserting.__tests__/components/editor/ClassDetailPanel.test.tsx (2)
116-136: Type the class-detail fixture against the real API contract.
makeClassDetail()currently acceptsRecord<string, unknown>, so the suite can drift away fromOWLClassDetailwithout the compiler noticing. Line 1406 already relies onannotations: null, even thoughlib/api/client.ts:418-432definesannotationsas an array. That makes contract regressions easy to miss and invalid payload tests hard to distinguish from normal cases.♻️ Suggested tightening
-import { projectOntologyApi } from "@/lib/api/client"; +import { projectOntologyApi, type OWLClassDetail } from "@/lib/api/client"; ... -const mockGetClassDetail = projectOntologyApi.getClassDetail as Mock; +const mockGetClassDetail = vi.mocked(projectOntologyApi.getClassDetail); ... -function makeClassDetail(overrides: Record<string, unknown> = {}) { - return { +function makeClassDetail( + overrides: Partial<OWLClassDetail> = {} +): OWLClassDetail { + return { iri: "http://example.org/ontology#Person", labels: [{ value: "Person", lang: "en" }], comments: [{ value: "A human being", lang: "en" }], @@ is_defined: true, annotations: [], ...overrides, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 116 - 136, The fixture factory makeClassDetail should be typed to the real contract OWLClassDetail and use correct field types: change its parameter from Record<string, unknown> to Partial<OWLClassDetail> and return an OWLClassDetail object; ensure default fields match the API (e.g., annotations: [] not null, equivalent_iris/disjoint_iris types match OWLClassDetail) and spread overrides last so tests can override valid properties; update any tests that assumed annotations null to use an explicit invalid-case fixture instead.
164-176: Reset shared mock implementations here, not just call history.Using
vi.clearAllMocks()in a file with lots ofmockResolvedValueOnce/mockRejectedValueOnceleaves this suite more order-dependent than it needs to be. If any one-off implementation goes unconsumed, the next test inherits it. Reset the shared mocks inbeforeEach()before reapplying defaults.🔁 Suggested reset pattern
beforeEach(() => { - vi.clearAllMocks(); + mockGetClassDetail.mockReset(); + mockGetIssues.mockReset(); + mockSearchEntities.mockReset(); + mockTriggerSave.mockReset(); + mockFlushToGit.mockReset(); + mockDiscardDraft.mockReset(); + mockClearRestoredDraft.mockReset(); mockGetClassDetail.mockResolvedValue(makeClassDetail()); mockGetIssues.mockResolvedValue({ items: [] }); mockSearchEntities.mockResolvedValue({ results: [] });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/ClassDetailPanel.test.tsx` around lines 164 - 176, Replace the single call to vi.clearAllMocks() in the beforeEach block with resetting mock implementations so one-off mockResolvedValueOnce / mockRejectedValueOnce don't leak between tests; either call vi.resetAllMocks() or explicitly reset the shared mocks (e.g., mockGetClassDetail.mockReset(), mockGetIssues.mockReset(), mockSearchEntities.mockReset(), mockFlushToGit.mockReset(), mockEditStateRef.current = null) before reapplying the default mockResolvedValue(...) calls so the subsequent mockResolvedValue defaults are always applied to a clean mock state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/editor/IndividualDetailPanel.test.tsx`:
- Around line 1499-1513: The test invokes the RelationshipSection callback
onAddGroup but never asserts outcomes; after calling onAddGroup() in the
"invokes addRelationshipGroup via RelationshipSection" test, assert the expected
postcondition such as that the onUpdateIndividual mock was called or that
capturedRelationshipSectionProps reflects the new group; update the test to call
onAddGroup() then expect(onUpdateIndividual).toHaveBeenCalled() (or another
appropriate assertion against capturedRelationshipSectionProps) so the test
verifies the side effect from IndividualDetailPanel/RelationshipSection.
- Around line 135-145: The beforeEach currently calls vi.clearAllMocks() which
only clears call history but leaves mutated implementations (e.g.,
mockFlushToGit set to mockResolvedValue(false) in a later test) leaking between
tests; replace vi.clearAllMocks() with vi.resetAllMocks() (or
vi.restoreAllMocks() if using spies) and then re-establish the expected default
mock implementations inside the beforeEach—e.g., call
mockExtractIndividualDetail.mockReturnValue(makeIndividualDetail()) and
explicitly reset mockFlushToGit to its intended default
(mockFlushToGit.mockResolvedValue(<defaultValue>)) so each test starts with
fresh implementations.
---
Duplicate comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 2521-2534: The test titled "returns null when classDetail is null
and no matching fallback" is misconfigured because it supplies a
selectedNodeFallback whose iri matches the component's classIri and then asserts
the fallback card is shown; either rename the test to reflect the "matching
fallback" case or change the fallback. Fix by updating the it(...) description
to something like "shows fallback card when classDetail is null and matching
fallback provided" (or change selectedNodeFallback.iri to a non-matching value
to exercise the no-matching-fallback branch), referencing the mockGetClassDetail
rejection, ClassDetailPanel props selectedNodeFallback, and the expectation that
the unsaved fallback card text (/has not been saved yet/) is rendered.
---
Nitpick comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 116-136: The fixture factory makeClassDetail should be typed to
the real contract OWLClassDetail and use correct field types: change its
parameter from Record<string, unknown> to Partial<OWLClassDetail> and return an
OWLClassDetail object; ensure default fields match the API (e.g., annotations:
[] not null, equivalent_iris/disjoint_iris types match OWLClassDetail) and
spread overrides last so tests can override valid properties; update any tests
that assumed annotations null to use an explicit invalid-case fixture instead.
- Around line 164-176: Replace the single call to vi.clearAllMocks() in the
beforeEach block with resetting mock implementations so one-off
mockResolvedValueOnce / mockRejectedValueOnce don't leak between tests; either
call vi.resetAllMocks() or explicitly reset the shared mocks (e.g.,
mockGetClassDetail.mockReset(), mockGetIssues.mockReset(),
mockSearchEntities.mockReset(), mockFlushToGit.mockReset(),
mockEditStateRef.current = null) before reapplying the default
mockResolvedValue(...) calls so the subsequent mockResolvedValue defaults are
always applied to a clean mock state.
In `@__tests__/components/editor/IndividualDetailPanel.test.tsx`:
- Around line 613-624: The tests assert synchronously right after render but the
component enters edit mode inside a useEffect, causing flakiness; update the
tests (e.g., the "auto-enters edit mode when continuousEditing is true" case and
the other affected cases) to use async assertions—either await
screen.findByTestId("auto-save-bar") or wrap the expectation in await waitFor(()
=> expect(...).not.toBeNull())—so the test waits for the effect-driven UI change
from the IndividualDetailPanel to stabilize before asserting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76039413-baf4-4821-be0c-707c88ec59e9
📒 Files selected for processing (3)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/IndividualDetailPanel.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/components/editor/PropertyDetailPanel.test.tsx
- IndividualDetailPanel: assert onAddGroup postcondition (groups length), wrap effect-driven edit mode assertions in waitFor, fix editorMode key - ClassDetailPanel: rename misleading fallback test to reflect matching fallback behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The coverage plan has been fully executed — line coverage reached 89.4%, well above the 80% target. The plan file is no longer needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
__tests__/components/editor/IndividualDetailPanel.test.tsx (3)
664-676:⚠️ Potential issue | 🟡 MinorWait for the IRI-change flush before asserting.
This expectation runs immediately after
rerender(). Ifcomponents/editor/IndividualDetailPanel.tsxfiresflushToGitfrom an effect onindividualIri, this is race-prone and can flake. Wrap the assertion inwaitFor(...), like the property-panel suite already does.Suggested fix
rerender( <IndividualDetailPanel {...DEFAULT_PROPS} individualIri="http://example.org/ontology#JaneDoe" canEdit={false} /> ); - expect(mockFlushToGit).toHaveBeenCalled(); + await waitFor(() => { + expect(mockFlushToGit).toHaveBeenCalled(); + });Verify whether the flush is effect-driven in
components/editor/IndividualDetailPanel.tsx; if it is, the assertion should stay async.#!/bin/bash fd -i 'IndividualDetailPanel.tsx' --exec sed -n '1,260p' {} \; | rg -n -C3 'useEffect|flushToGit|individualIri'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/IndividualDetailPanel.test.tsx` around lines 664 - 676, The test currently asserts mockFlushToGit immediately after rerender, which is race-prone if IndividualDetailPanel triggers flushToGit from a useEffect watching individualIri; update the test to wait for the side-effect by wrapping the expectation in waitFor (importing waitFor from `@testing-library/react` if needed) so you call waitFor(() => expect(mockFlushToGit).toHaveBeenCalled()) after rerender; alternatively, confirm in IndividualDetailPanel (search for useEffect and flushToGit) that the call is synchronous and keep the original assertion only if it is not effect-driven.
1215-1232:⚠️ Potential issue | 🟠 MajorAdd a real postcondition to these callback tests.
Both blocks invoke
onValueChange/onLangChangebut never assert that the updated props reappear on the next render. A no-op handler would still pass here, unlike the adjacent comment test that checks the rerenderedAnnotationRowprops.Suggested fix
const onValueChange = defRow!.onValueChange as (v: string) => void; onValueChange("Updated definition"); + + await waitFor(() => { + const updatedRow = capturedAnnotationRowProps.find( + (p) => + p.propertyIri === "http://www.w3.org/2004/02/skos/core#definition" && + p.value === "Updated definition" + ); + expect(updatedRow).toBeDefined(); + });const onValueChange = annRow!.onValueChange as (v: string) => void; onValueChange("Updated Pref Label"); const onLangChange = annRow!.onLangChange as (l: string) => void; onLangChange("de"); + + await waitFor(() => { + expect( + capturedAnnotationRowProps.find( + (p) => + p.propertyIri === "http://www.w3.org/2004/02/skos/core#prefLabel" && + p.value === "Updated Pref Label" + ) + ).toBeDefined(); + expect( + capturedAnnotationRowProps.find( + (p) => + p.propertyIri === "http://www.w3.org/2004/02/skos/core#prefLabel" && + p.lang === "de" + ) + ).toBeDefined(); + });Also applies to: 1527-1556
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/IndividualDetailPanel.test.tsx` around lines 1215 - 1232, The test calls the definition/label callback handlers but lacks a postcondition to verify the update; after invoking defRow.onValueChange or onLangChange, assert that onUpdateIndividual was called with the expected updated value (or language) and re-render the component (or trigger user interactions) and then locate the updated AnnotationRow in capturedAnnotationRowProps to assert its value/lang reflects the change; update both test blocks (lines around the defRow check and the similar block at 1527-1556) to call the handler, expect onUpdateIndividual to have been called with the new payload, and then re-query capturedAnnotationRowProps for the same propertyIri and assert the new value/lang is present.
135-145:⚠️ Potential issue | 🟠 MajorReset
mockFlushToGitbetween tests.This reintroduces the same leak pattern: Line 1691 mutates a shared mock to resolve
false, but thebeforeEachon Lines 135-145 only clears call history. Later tests then start from the wrong save state.Suggested fix
beforeEach(() => { vi.clearAllMocks(); + mockFlushToGit.mockReset(); + mockFlushToGit.mockResolvedValue(true); autoSaveOverrides = {}; editorModeOverrides = {}; capturedAnnotationRowProps = []; capturedRelationshipSectionProps = null; capturedPropertyAssertionProps = []; capturedAutoSaveBarProps = null; capturedInlineAnnotationAdderProps = null; mockExtractIndividualDetail.mockReturnValue(makeIndividualDetail()); });- mockFlushToGit.mockResolvedValue(false); + mockFlushToGit.mockResolvedValueOnce(false);In Vitest, does `vi.clearAllMocks()` reset mock implementations set with `mockResolvedValue(...)`, or only call history? What is the recommended way to restore a shared mock's default implementation between tests?Also applies to: 1688-1707
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/components/editor/IndividualDetailPanel.test.tsx` around lines 135 - 145, The shared mock mockFlushToGit is being mutated in tests and vi.clearAllMocks() only clears call history, not implementations; update the beforeEach to restore the mock implementation between tests by either replacing vi.clearAllMocks() with vi.resetAllMocks()/vi.restoreAllMocks() or explicitly resetting mockFlushToGit to its default with mockFlushToGit.mockResolvedValue(<originalDefault>) in the beforeEach so tests don't inherit a mutated resolve value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/components/editor/PropertyDetailPanel.test.tsx`:
- Around line 143-152: The test suite leaks mockFlushToGit's implementation
across tests; in the beforeEach block (where vi.clearAllMocks() is called) reset
or restore mockFlushToGit so later tests don't inherit the
mockResolvedValue(false) set in a specific test. Add a call such as
mockFlushToGit.mockReset() or mockFlushToGit.mockRestore() in the beforeEach and
optionally set a safe default with mockFlushToGit.mockResolvedValue(true) (or
remove default implementation) so tests remain order-independent; reference the
mock name mockFlushToGit and the existing beforeEach setup where
autoSaveOverrides/editorModeOverrides are initialized.
---
Duplicate comments:
In `@__tests__/components/editor/IndividualDetailPanel.test.tsx`:
- Around line 664-676: The test currently asserts mockFlushToGit immediately
after rerender, which is race-prone if IndividualDetailPanel triggers flushToGit
from a useEffect watching individualIri; update the test to wait for the
side-effect by wrapping the expectation in waitFor (importing waitFor from
`@testing-library/react` if needed) so you call waitFor(() =>
expect(mockFlushToGit).toHaveBeenCalled()) after rerender; alternatively,
confirm in IndividualDetailPanel (search for useEffect and flushToGit) that the
call is synchronous and keep the original assertion only if it is not
effect-driven.
- Around line 1215-1232: The test calls the definition/label callback handlers
but lacks a postcondition to verify the update; after invoking
defRow.onValueChange or onLangChange, assert that onUpdateIndividual was called
with the expected updated value (or language) and re-render the component (or
trigger user interactions) and then locate the updated AnnotationRow in
capturedAnnotationRowProps to assert its value/lang reflects the change; update
both test blocks (lines around the defRow check and the similar block at
1527-1556) to call the handler, expect onUpdateIndividual to have been called
with the new payload, and then re-query capturedAnnotationRowProps for the same
propertyIri and assert the new value/lang is present.
- Around line 135-145: The shared mock mockFlushToGit is being mutated in tests
and vi.clearAllMocks() only clears call history, not implementations; update the
beforeEach to restore the mock implementation between tests by either replacing
vi.clearAllMocks() with vi.resetAllMocks()/vi.restoreAllMocks() or explicitly
resetting mockFlushToGit to its default with
mockFlushToGit.mockResolvedValue(<originalDefault>) in the beforeEach so tests
don't inherit a mutated resolve value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85f4efc4-6e40-4f9a-a0ce-3788374bcf1e
📒 Files selected for processing (3)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/IndividualDetailPanel.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx
✅ Files skipped from review due to trivial changes (1)
- tests/components/editor/ClassDetailPanel.test.tsx
- PropertyDetailPanel/IndividualDetailPanel: reset mockFlushToGit to mockResolvedValue(true) in beforeEach to prevent implementation leakage between tests - IndividualDetailPanel: wrap flushToGit assertion in waitFor for effect-driven IRI change, add value/lang postcondition assertions for definition and custom annotation callback tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Apr 10, 2026
This was referenced Apr 21, 2026
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #94
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores