feat: preserve entity selection across viewer/editor modes#104
Merged
damienriehl merged 23 commits intodevfrom May 2, 2026
Merged
feat: preserve entity selection across viewer/editor modes#104damienriehl merged 23 commits intodevfrom
damienriehl merged 23 commits intodevfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (6)
📝 WalkthroughWalkthroughUnified selection and viewer/editor switching: adds selection URL utilities and a session selection store, replaces continuousEditing with preferEditMode, auto-enters edit mode in detail panels, moves flush-to-git to unmount, removes the ContinuousEditingToggle, updates routing/UI to preserve selection, and updates/adds tests accordingly. (50 words) ChangesStore & Preferences
Selection & Navigation
Detail Panels & Autosave
Removal & Misc
Sequence Diagram(s)sequenceDiagram
participant User
participant Viewer
participant Router
participant Editor
participant SelectionStore
participant Layout
participant DetailPanel
participant AutoSave
User->>Viewer: Click "Editor" (link includes selection query)
Viewer->>Router: Navigate to /projects/:id/editor?{selection}
Router->>Editor: Render editor page with searchParams
Editor->>SelectionStore: readSelectionFromSearchParams -> selection
SelectionStore-->>Editor: selection {iri,type}
Editor->>Layout: apply selection (navigateToNode for class or entityNavigationRef for others)
Layout->>SelectionStore: setSelection(iri,type)
Layout->>DetailPanel: mount detail panel keyed by iri
DetailPanel->>DetailPanel: auto-enter edit mode -> render AutoSave
User->>Editor: Click "Viewer" (switcher)
Editor->>Router: Navigate to /projects/:id?{selection}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 34 minutes and 20 seconds.Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
60 tasks
JohnRDOrazio
force-pushed
the
feat/preserve-entity-selection
branch
2 times, most recently
from
0b3edc5 to
c6e8bdf
Compare
Pass classIri query param when navigating between viewer and editor. Replace "Back" link with "Close Editor" button that preserves selection. Remove ContinuousEditingToggle — being in the editor now implies edit mode for the selected entity. Simplify ClassDetailPanel, PropertyDetailPanel, and IndividualDetailPanel edit state by auto-entering edit mode when onUpdate handler is provided (editor context). Closes #98 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JohnRDOrazio
force-pushed
the
feat/preserve-entity-selection
branch
from
c6e8bdf to
66a0b4c
Compare
JohnRDOrazio
added
enhancement
5 tasks
Restore the persisted preference that was previously named "continuousEditing" under the new name "preferEditMode" with the semantics issue #98 calls for: "open ontology in edit mode by default" rather than the redundant per-entity toggle the old name implied. - Re-add preferEditMode + setPreferEditMode to editorModeStore (default false). - Replace the unconditional !!onUpdate* auto-enter trigger in ClassDetailPanel, PropertyDetailPanel, and IndividualDetailPanel with a check against preferEditMode, so users opt in to skipping the "Edit Item" click. - Surface the preference as a toggle in app/settings (User Settings → Editor Preferences) using the Pencil icon. - Drop a stray blank line left where the ContinuousEditingToggle import was removed from app/projects/[id]/editor/page.tsx. - Update detail panel test mocks to default preferEditMode: true (matching the editor-context behavior they exercise) and add explicit gating tests that assert no auto-enter when preferEditMode is off. - Cover the new field/setter in editorModeStore tests. Closes the items 4 and 5 gap from issue #98 that the original PR #104 had deferred. PR #152 is being closed as a duplicate of this one. Co-Authored-By: Claude Opus 4.7 (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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/stores/editorModeStore.ts (1)
44-65:⚠️ Potential issue | 🟠 MajorPersisted preference migration is missing.
At Lines 44-65, the storage key is unchanged but the field was renamed. Existing persisted
continuousEditingvalues won’t map topreferEditMode, so prior user preference is reset.🔧 Suggested patch
persist( (set) => ({ @@ { name: "ontokit-editor-preferences", + version: 2, + migrate: (persistedState, version) => { + if (!persistedState || typeof persistedState !== "object") return persistedState; + const state = persistedState as { + preferEditMode?: boolean; + continuousEditing?: boolean; + }; + if (version < 2) { + return { + ...state, + preferEditMode: state.preferEditMode ?? state.continuousEditing ?? false, + }; + } + return state; + }, onRehydrateStorage: () => (state) => { if (state) { applyThemeToDOM(state.theme); } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/stores/editorModeStore.ts` around lines 44 - 65, The rehydration callback in onRehydrateStorage must migrate the old persisted key continuousEditing to the new preferEditMode so users don't lose their setting; update the onRehydrateStorage handler (the state parameter inside onRehydrateStorage) to check for state.continuousEditing and, if present and preferEditMode is undefined, set state.preferEditMode = state.continuousEditing (and optionally remove state.continuousEditing) before calling applyThemeToDOM(state.theme), referencing the existing onRehydrateStorage function, preferEditMode and continuousEditing property names to locate where to apply this migration.
🧹 Nitpick comments (2)
__tests__/components/editor/ClassDetailPanel.test.tsx (1)
432-433: Extract a shared helper for repeated auto-save-bar waits.The same
waitFor(getByTestId("auto-save-bar"))pattern is repeated many times; extracting one helper will reduce noise and maintenance overhead.♻️ Suggested refactor
+const waitForAutoSaveBar = async () => { + await waitFor(() => { + expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); + }); +}; - await waitFor(() => { - expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); - }); + await waitForAutoSaveBar();🤖 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 432 - 433, Extract a reusable helper for the repeated waitFor(getByTestId("auto-save-bar")) pattern: add a function (e.g. waitForAutoSaveBar or waitForAutoSaveBarToAppear) that wraps waitFor(() => getByTestId("auto-save-bar")) or screen.getByTestId inside the test file or shared test utils, then replace every occurrence in ClassDetailPanel.test.tsx with a call to that helper to reduce duplication and improve readability; ensure the helper uses the same import sources (waitFor, screen/getByTestId) so existing tests continue to work.__tests__/components/editor/PropertyDetailPanel.test.tsx (1)
689-700: Wrap auto-save-bar assertions inwaitForfor test robustness.Lines 699 and 1908 use synchronous
getByTestId, which may race with effect-driven UI state. While the current pattern is not technically inconsistent with the rest of the file (all auto-save-bar assertions use synchronous calls), wrapping these inwaitForwould improve reliability:Suggested refactor
- expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); + await waitFor(() => { + expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); + });Also applies to: 1908
🤖 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 689 - 700, The test "auto-enters edit mode when preferEditMode is on (editor context)" uses a synchronous assertion on the "auto-save-bar" which can race with effect-driven UI updates; update the assertion to wait for the element using React Testing Library's waitFor (wrap the expect(screen.getByTestId("auto-save-bar"))... inside waitFor) so the test waits for the auto-save bar to appear, and apply the same change to the other occurrence around line 1908 to improve reliability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/editor/page.tsx:
- Line 825: The Close Editor link currently builds its href using only
selectedIri which can be empty; update the href construction to fall back to
classIriParam when selectedIri is falsy (i.e., use selectedIri ||
classIriParam), ensuring you still encode the value with encodeURIComponent and
only append the ?classIri=... query when that fallback value exists; change the
template that constructs href (the expression using projectId and selectedIri)
to use the fallback so the Close Editor preserves the last selected class from
the URL.
In `@app/projects/`[id]/page.tsx:
- Line 261: The Link currently builds its href only from selectedIri so if the
page was opened with a classIri query param but selectedIri is not yet hydrated
the selection is lost; change the href generation in the Link (the JSX using
Link, projectId and selectedIri) to fall back to the existing URL query param
named classIri (read from the current location/searchParams or router.query)
when selectedIri is falsy, and still encodeURIComponent the value, so the
resulting URL uses selectedIri || existingClassIri for the ?classIri= query
parameter.
---
Outside diff comments:
In `@lib/stores/editorModeStore.ts`:
- Around line 44-65: The rehydration callback in onRehydrateStorage must migrate
the old persisted key continuousEditing to the new preferEditMode so users don't
lose their setting; update the onRehydrateStorage handler (the state parameter
inside onRehydrateStorage) to check for state.continuousEditing and, if present
and preferEditMode is undefined, set state.preferEditMode =
state.continuousEditing (and optionally remove state.continuousEditing) before
calling applyThemeToDOM(state.theme), referencing the existing
onRehydrateStorage function, preferEditMode and continuousEditing property names
to locate where to apply this migration.
---
Nitpick comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 432-433: Extract a reusable helper for the repeated
waitFor(getByTestId("auto-save-bar")) pattern: add a function (e.g.
waitForAutoSaveBar or waitForAutoSaveBarToAppear) that wraps waitFor(() =>
getByTestId("auto-save-bar")) or screen.getByTestId inside the test file or
shared test utils, then replace every occurrence in ClassDetailPanel.test.tsx
with a call to that helper to reduce duplication and improve readability; ensure
the helper uses the same import sources (waitFor, screen/getByTestId) so
existing tests continue to work.
In `@__tests__/components/editor/PropertyDetailPanel.test.tsx`:
- Around line 689-700: The test "auto-enters edit mode when preferEditMode is on
(editor context)" uses a synchronous assertion on the "auto-save-bar" which can
race with effect-driven UI updates; update the assertion to wait for the element
using React Testing Library's waitFor (wrap the
expect(screen.getByTestId("auto-save-bar"))... inside waitFor) so the test waits
for the auto-save bar to appear, and apply the same change to the other
occurrence around line 1908 to improve reliability.
🪄 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: a89f5da0-7c90-4c60-b0b0-54d01d8a9c5d
📒 Files selected for processing (13)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/ContinuousEditingToggle.test.tsx__tests__/components/editor/IndividualDetailPanel.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx__tests__/lib/stores/editorModeStore.test.tsapp/projects/[id]/editor/page.tsxapp/projects/[id]/page.tsxapp/settings/page.tsxcomponents/editor/ClassDetailPanel.tsxcomponents/editor/ContinuousEditingToggle.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsxlib/stores/editorModeStore.ts
💤 Files with no reviewable changes (2)
- tests/components/editor/ContinuousEditingToggle.test.tsx
- components/editor/ContinuousEditingToggle.tsx
Resolve test conflicts in IndividualDetailPanel.test.tsx and
PropertyDetailPanel.test.tsx by adopting dev's LanguagePicker assertion
pattern (getAllByLabelText("Language tag") + selectOptions) under the new
auto-enter behavior (waitFor auto-save-bar instead of clicking "Edit Item").
Address CodeRabbit feedback on PR #104:
- Open Editor link in viewer (app/projects/[id]/page.tsx) and Close Editor
link in editor (app/projects/[id]/editor/page.tsx) now fall back to the
current classIri search param when selectedIri is unhydrated, so the
selection survives the round-trip even when the tree hasn't finished
loading.
- Wrap synchronous expect(getByTestId("auto-save-bar")) calls in waitFor
for the preferEditMode auto-enter test and the cancel-then-restore tests
in PropertyDetailPanel and IndividualDetailPanel — auto-enter is
effect-driven and could race.
Lint cleanup: remove 119 unused `const user = userEvent.setup();`
declarations across the three detail-panel test files. They were left
over when the original PR removed the manual "Edit Item" click steps.
Skip the migration for the old continuousEditing localStorage key —
ontokit-web has no public release yet, so no real users would be carrying
the key, and zustand persist will simply use the new default.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
__tests__/components/editor/ClassDetailPanel.test.tsx (2)
1714-1746:⚠️ Potential issue | 🟠 MajorThese tests are asserting the wrong editor contract.
preferEditModeshould decide whether the app opens the viewer or the editor by default. Once this panel is already rendered in the editor, it should still enter edit mode. Keeping this expectation will mask the regression inClassDetailPanel.Suggested adjustment
-it("does not auto-enter edit mode when preferEditMode is off", async () => { +it("still auto-enters edit mode when preferEditMode is off and the editor is open", async () => { editorModeOverrides = { preferEditMode: false }; const onUpdateClass = vi.fn(); render( <ClassDetailPanel {...DEFAULT_PROPS} canEdit={true} onUpdateClass={onUpdateClass} /> ); await waitFor(() => { - expect(screen.getByText("Edit Item")).not.toBeNull(); + expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); }); - expect(screen.queryByTestId("auto-save-bar")).toBeNull(); });🤖 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 1714 - 1746, The tests for ClassDetailPanel assert the wrong contract for preferEditMode: adjust the two specs so preferEditMode controls whether the component initially mounts in viewer vs editor, but once the panel is rendered in an editor context it should always enter edit mode; update the first test to ensure auto-editing occurs when the component is mounted in editor context (e.g., by setting canEdit=true and asserting auto-save/bar appears), and change the second test to assert that preferEditMode:false causes the component to mount in viewer mode by default (assert presence of "Edit Item" without auto-save bar) only when the panel is not already in an editor context—use the existing test variables editorModeOverrides, canEdit, and onUpdateClass and the ClassDetailPanel component to locate and modify the expectations accordingly.
584-596:⚠️ Potential issue | 🟡 MinorThis spec no longer tests the click path it names.
It now passes solely because edit mode auto-enters. That leaves the manual
"Edit Item"path untested, even though users can still reach it after cancel or from a non-auto-enter state.Suggested test shape
it("enters edit mode when Edit Item button is clicked", async () => { + const user = userEvent.setup(); + editorModeOverrides = { preferEditMode: false }; const onUpdateClass = vi.fn(); render( <ClassDetailPanel {...DEFAULT_PROPS} canEdit={true} onUpdateClass={onUpdateClass} /> ); + await waitFor(() => { + expect(screen.getByText("Edit Item")).not.toBeNull(); + }); + await user.click(screen.getByText("Edit Item")); + await waitFor(() => { expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); });🤖 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 584 - 596, The test "enters edit mode when Edit Item button is clicked" currently passes because edit mode auto-enters; update it to actually exercise the click path by rendering ClassDetailPanel (use the existing DEFAULT_PROPS and onUpdateClass mock) in a non-auto-enter state or cancel the auto-entered mode first, then simulate a user click on the "Edit Item" control (e.g., fireEvent.click or userEvent.click targeting the button by role/text) and assert that edit mode appears (e.g., presence of the editor form or specific test-id instead of relying on auto-save-bar). Ensure you reference the existing render call, screen queries (like getByRole/getByText/getByTestId), and onUpdateClass mock when adding the click and assertions.__tests__/components/editor/IndividualDetailPanel.test.tsx (2)
642-672:⚠️ Potential issue | 🟠 Major
preferEditMode: falseshould not make the editor panel read-only.This test bakes in the opposite of the feature goal. The preference should control which route opens first; once
IndividualDetailPanelis rendered in editor context, it should still auto-enter edit mode.🤖 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 642 - 672, The test "does not auto-enter edit mode when preferEditMode is off" is asserting the wrong behavior: preferEditMode only controls which route opens first, but when IndividualDetailPanel is rendered in editor context it should still auto-enter edit mode; update this test (and/or the variable usage of editorModeOverrides) so that with editor context and editorModeOverrides = { preferEditMode: false } the component still auto-enters edit mode—replace the expect(screen.getByText("Edit Item")).not.toBeNull() and expect(...queryByTestId("auto-save-bar")).toBeNull() assertions with an assertion that the auto-save bar is present (e.g., expect(screen.getByTestId("auto-save-bar")).not.toBeNull()) referencing IndividualDetailPanel, editorModeOverrides, and preferEditMode so the test reflects the intended behavior.
383-396:⚠️ Potential issue | 🟡 MinorThis no longer verifies the
"Edit Item"interaction.The test succeeds as soon as auto-entry happens, so it won't catch a broken manual edit button anymore. Please keep one spec that disables auto-entry and actually clicks
"Edit Item".🤖 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 383 - 396, The test "enters edit mode when Edit Item is clicked" no longer verifies clicking the Edit button because auto-entry immediately opens the editor; update or add a spec that disables auto-entry when rendering IndividualDetailPanel (pass a prop to turn off auto-entry, e.g., include autoEntryEnabled: false or disableAutoEntry on the component props when calling render with DEFAULT_PROPS) then simulate the user click on the "Edit Item" control (use fireEvent.click or userEvent.click on screen.getByText(/Edit Item/i) or its test id) and assert the panel entered edit mode (for example by checking for the presence of the edit form fields or the auto-save bar after the click) while keeping onUpdateIndividual as a spy to ensure no premature saves.__tests__/components/editor/PropertyDetailPanel.test.tsx (2)
689-720:⚠️ Potential issue | 🟠 Major
preferEditModegating here contradicts the editor-mode contract.These tests lock in behavior where editor context does not auto-enter edit mode when
preferEditModeis false. That conflicts with the PR objective that being in editor implies edit mode.Suggested test expectation update
- // ── Auto-enter edit mode gated by preferEditMode ── + // ── Editor context always auto-enters edit mode ── - it("does not auto-enter edit mode when preferEditMode is off", async () => { + it("still auto-enters edit mode when preferEditMode is off (already in editor)", async () => { editorModeOverrides = { preferEditMode: false }; const onUpdateProperty = vi.fn(); render( <PropertyDetailPanel {...DEFAULT_PROPS} canEdit={true} onUpdateProperty={onUpdateProperty} /> ); - await waitFor(() => { - expect(screen.getByText("Edit Item")).not.toBeNull(); - }); - expect(screen.queryByTestId("auto-save-bar")).toBeNull(); + await waitFor(() => { + expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); + }); });🤖 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 689 - 720, The tests in PropertyDetailPanel.test.tsx assert that editorModeOverrides.preferEditMode=false prevents auto-entering edit mode, which contradicts the editor-mode contract that being in editor context should always auto-enter edit mode; update the failing test cases: in the "does not auto-enter edit mode when preferEditMode is off" test remove or change the expectation that edit mode is disabled (references: PropertyDetailPanel, editorModeOverrides, preferEditMode) so the test instead asserts the component does auto-enter edit mode (e.g., check for the auto-save bar or other edit-mode indicators) even when preferEditMode is false, and keep the other assertions consistent with editor context behavior.
398-412:⚠️ Potential issue | 🟡 MinorTest intent and implementation diverged (manual entry path is no longer tested).
The test says “when Edit Item is clicked” but no click occurs now, so this duplicates auto-entry coverage instead of validating the manual edit trigger path.
Suggested fix to keep real click-path coverage
it("enters edit mode when Edit Item is clicked", async () => { + const user = userEvent.setup(); + editorModeOverrides = { preferEditMode: false }; const onUpdateProperty = vi.fn(); render( <PropertyDetailPanel {...DEFAULT_PROPS} canEdit={true} onUpdateProperty={onUpdateProperty} /> ); + await waitFor(() => { + expect(screen.getByText("Edit Item")).not.toBeNull(); + }); + await user.click(screen.getByText("Edit Item")); await waitFor(() => { expect(screen.getByTestId("auto-save-bar")).not.toBeNull(); }); - // Auto-save bar should appear - expect(screen.getByTestId("auto-save-bar")).toBeDefined(); });🤖 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 398 - 412, The test "enters edit mode when Edit Item is clicked" no longer simulates the user click so it only asserts auto-entry; update the test for PropertyDetailPanel to actually trigger the manual edit path: render PropertyDetailPanel with canEdit={true} and onUpdateProperty=vi.fn(), locate and click the "Edit Item" control (e.g., screen.getByText / getByRole for the Edit Item button) and then waitFor/assert that the auto-save bar (screen.getByTestId("auto-save-bar")) appears; remove or replace the redundant direct existence assertion so the test verifies the click-to-edit behavior rather than duplicate auto-entry coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/editor/ClassDetailPanel.tsx`:
- Line 122: The panel currently prevents auto-entering edit mode by checking
preferEditMode from useEditorModeStore; instead, when the component is rendered
with an onUpdateClass prop it should always enter edit mode. In
ClassDetailPanel, remove or stop using the preferEditMode check (the const
preferEditMode = useEditorModeStore((s) => s.preferEditMode) and any
conditionals that gate auto-entry on it) and change the logic that triggers edit
mode to rely solely on the presence of onUpdateClass (e.g., if (onUpdateClass) {
/* set edit mode / call editor entry */ }) so users viewing this editor-backed
panel are editable regardless of their preferEditMode setting; apply the same
change to the other block referenced around the 224-230 area.
In `@components/editor/PropertyDetailPanel.tsx`:
- Line 129: The panel currently uses preferEditMode from useEditorModeStore to
decide whether to enable editing inside PropertyDetailPanel; instead remove that
gating so being inside the editor always enables edit mode when editing is
available. Replace usages of preferEditMode in PropertyDetailPanel with a
runtime capability check (e.g., prop or state like editingAvailable / canEdit)
and automatically set the editor mode to edit (call the editor mode setter from
useEditorModeStore to true) whenever editing is available; do the same
replacement for the other occurrences that mirror lines 313-316. Use the
existing symbols preferEditMode, useEditorModeStore and PropertyDetailPanel to
locate and update the logic.
---
Outside diff comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 1714-1746: The tests for ClassDetailPanel assert the wrong
contract for preferEditMode: adjust the two specs so preferEditMode controls
whether the component initially mounts in viewer vs editor, but once the panel
is rendered in an editor context it should always enter edit mode; update the
first test to ensure auto-editing occurs when the component is mounted in editor
context (e.g., by setting canEdit=true and asserting auto-save/bar appears), and
change the second test to assert that preferEditMode:false causes the component
to mount in viewer mode by default (assert presence of "Edit Item" without
auto-save bar) only when the panel is not already in an editor context—use the
existing test variables editorModeOverrides, canEdit, and onUpdateClass and the
ClassDetailPanel component to locate and modify the expectations accordingly.
- Around line 584-596: The test "enters edit mode when Edit Item button is
clicked" currently passes because edit mode auto-enters; update it to actually
exercise the click path by rendering ClassDetailPanel (use the existing
DEFAULT_PROPS and onUpdateClass mock) in a non-auto-enter state or cancel the
auto-entered mode first, then simulate a user click on the "Edit Item" control
(e.g., fireEvent.click or userEvent.click targeting the button by role/text) and
assert that edit mode appears (e.g., presence of the editor form or specific
test-id instead of relying on auto-save-bar). Ensure you reference the existing
render call, screen queries (like getByRole/getByText/getByTestId), and
onUpdateClass mock when adding the click and assertions.
In `@__tests__/components/editor/IndividualDetailPanel.test.tsx`:
- Around line 642-672: The test "does not auto-enter edit mode when
preferEditMode is off" is asserting the wrong behavior: preferEditMode only
controls which route opens first, but when IndividualDetailPanel is rendered in
editor context it should still auto-enter edit mode; update this test (and/or
the variable usage of editorModeOverrides) so that with editor context and
editorModeOverrides = { preferEditMode: false } the component still auto-enters
edit mode—replace the expect(screen.getByText("Edit Item")).not.toBeNull() and
expect(...queryByTestId("auto-save-bar")).toBeNull() assertions with an
assertion that the auto-save bar is present (e.g.,
expect(screen.getByTestId("auto-save-bar")).not.toBeNull()) referencing
IndividualDetailPanel, editorModeOverrides, and preferEditMode so the test
reflects the intended behavior.
- Around line 383-396: The test "enters edit mode when Edit Item is clicked" no
longer verifies clicking the Edit button because auto-entry immediately opens
the editor; update or add a spec that disables auto-entry when rendering
IndividualDetailPanel (pass a prop to turn off auto-entry, e.g., include
autoEntryEnabled: false or disableAutoEntry on the component props when calling
render with DEFAULT_PROPS) then simulate the user click on the "Edit Item"
control (use fireEvent.click or userEvent.click on screen.getByText(/Edit
Item/i) or its test id) and assert the panel entered edit mode (for example by
checking for the presence of the edit form fields or the auto-save bar after the
click) while keeping onUpdateIndividual as a spy to ensure no premature saves.
In `@__tests__/components/editor/PropertyDetailPanel.test.tsx`:
- Around line 689-720: The tests in PropertyDetailPanel.test.tsx assert that
editorModeOverrides.preferEditMode=false prevents auto-entering edit mode, which
contradicts the editor-mode contract that being in editor context should always
auto-enter edit mode; update the failing test cases: in the "does not auto-enter
edit mode when preferEditMode is off" test remove or change the expectation that
edit mode is disabled (references: PropertyDetailPanel, editorModeOverrides,
preferEditMode) so the test instead asserts the component does auto-enter edit
mode (e.g., check for the auto-save bar or other edit-mode indicators) even when
preferEditMode is false, and keep the other assertions consistent with editor
context behavior.
- Around line 398-412: The test "enters edit mode when Edit Item is clicked" no
longer simulates the user click so it only asserts auto-entry; update the test
for PropertyDetailPanel to actually trigger the manual edit path: render
PropertyDetailPanel with canEdit={true} and onUpdateProperty=vi.fn(), locate and
click the "Edit Item" control (e.g., screen.getByText / getByRole for the Edit
Item button) and then waitFor/assert that the auto-save bar
(screen.getByTestId("auto-save-bar")) appears; remove or replace the redundant
direct existence assertion so the test verifies the click-to-edit behavior
rather than duplicate auto-entry coverage.
🪄 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: f36c7778-2617-4375-bb92-17f8d71c68a4
📒 Files selected for processing (8)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/IndividualDetailPanel.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsxapp/projects/[id]/editor/page.tsxapp/projects/[id]/page.tsxcomponents/editor/ClassDetailPanel.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- app/projects/[id]/page.tsx
- components/editor/IndividualDetailPanel.tsx
- app/projects/[id]/editor/page.tsx
The three tests named "enters edit mode when Edit Item is clicked" never
actually clicked anything — they relied on the auto-enter behavior the
mock default (preferEditMode: true) provides, so the manual click path
was uncovered.
Set preferEditMode: false in each, assert the panel mounts read-only
("Edit Item" present, no auto-save bar), then click Edit Item and assert
the auto-save bar appears. This covers the click-to-edit flow that the
preferEditMode gating now makes possible.
The remaining CodeRabbit findings in this batch (proposing to remove the
preferEditMode gating entirely so editor context always auto-enters) are
intentionally not applied — they would reverse the issue #98 item 4
design we just shipped, where the preference controls whether entities
open in edit mode by default.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Addressing the two actionable findings from the latest CodeRabbit review: Fix 1 — Fix 2 — Both changes applied in commit R-Hart80/ontokit-web@194b293 on branch // Before (both buttons)
href={`...${selectedIri ? `?classIri=${encodeURIComponent(selectedIri)}` : ""}`}
// After
href={`...${(selectedIri || classIriParam) ? `?classIri=${encodeURIComponent(selectedIri || classIriParam!)}` : ""}`}This ensures the entity selection is preserved even when the page is opened via a Happy to open a follow-up PR targeting this branch if that's easier to review. |
Per the design clarification: when the user is in the editor (vs the
read-only viewer), the panel is unconditionally in edit mode. There is
no longer a separate "Edit Item" affordance to enter edit mode — that
button was a holdover from the three-state world (viewer / editor-but-
read-only / editor-editing) that we collapsed to two states (viewer /
editor-always-editing).
Detail panels (Class, Property, Individual):
- Drop the preferEditMode read inside the panels. preferEditMode is now
purely a routing-level preference (which page to default to when
opening a project), not a per-entity gate.
- Drop cancelledIriRef. With unconditional auto-entry there is nothing
to suppress — Cancel just discards the in-progress draft and re-inits
from server state, then stays in edit mode (auto-save bar visible).
- Manual save no longer calls setIsEditing(false); it just flushes.
- Remove the Edit Item button block, the canEnterEdit local, and the
now-unused Pencil/isSuggestionMode references in ClassDetailPanel.
Settings copy: update the Prefer Edit Mode description to reflect the
routing semantics ("opens a project straight to the editor / viewer
first") rather than the old per-entity meaning.
Tests updated:
- Remove "does not auto-enter edit mode when preferEditMode is off"
(gating no longer exists).
- Remove "enters edit mode when Edit Item is clicked" (no button).
- Rename "hides Edit Item button while in edit mode" to "never renders
an Edit Item button in editor context".
- Rewrite cancel tests: assert the auto-save bar stays visible after
cancel instead of expecting a return to read-only "Edit Item".
- Drop the redundant per-panel `preferEditMode: true` mock default.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
Thank you for the evaluation, I'll keep it in mind. I'm still actively working on this PR to clean a lot of things up and get them aligned. In the very first version v0.1.0 there were only classes, no properties or individuals. In the second iteration v0.2.0 from @damienriehl, properties and individuals trees were added, but weren't wired up with the navigation / selection. So I'm currently fixing that. |
…rams Replaces the standalone "Open Editor" / "Close Editor" buttons with a grouped Viewer|Editor toggle that lives next to the existing Standard|Developer ModeSwitcher in both pages. The toggle's active segment renders as a non-link span; the inactive segment is a Link. Selection round-trip is now type-aware. The single ?classIri= query param is replaced with one of three mutually-exclusive keys: - ?classIri=<encoded> for class entities - ?propertyIri=<encoded> for properties - ?individualIri=<encoded> for individuals This addresses OWL punning, where the same IRI can refer to a class and an individual simultaneously — a bookmarked link to the Person class and one to the Person individual are now unambiguous. A new lib/utils/selectionUrl helper centralizes: - readSelectionFromSearchParams: reads any of the three keys with priority class > property > individual (matching tree order). - buildSelectionQuery: builds a leading-? query string from a typed selection. The viewer page no longer renders its own "Open Editor" button. The "Sign in to edit" affordance for unauthenticated users is preserved. The editor page no longer renders its "Close Editor" link or the preceding divider. Wiring of selectedIri → entity-type at the link-builder side is the next phase (selection store needs to surface the active entity's type); for now the switcher carries through whichever ?<type>Iri= param the URL already has, which preserves today's behavior end-to-end. Tests: 16 new (URL helper round-trip and priority order; switcher active-segment rendering, link hrefs, and IRI carry-through for all three types). Full suite: 2705 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/editor/IndividualDetailPanel.tsx (1)
269-294:⚠️ Potential issue | 🟠 MajorGate restored-draft entry on
onUpdateIndividual.The restored-draft branch runs before the
onUpdateIndividualcheck, socanEdit={true}plus a missing updater still flips the panel into edit mode. In that stateuseEntityAutoSavehasonFlush: undefined, which gives users an editable UI with no persistence path.Suggested fix
useEffect(() => { if (isEditing || editInitializedRef.current) return; - if (!canEdit || !detail) return; + if (!canEdit || !onUpdateIndividual || !detail) return; if (restoredDraft && restoredDraft.entityType === "individual" && individualIri) { const d = restoredDraft as IndividualDraftEntry; setEditLabels(d.labels); @@ clearRestoredDraft(); return; } - if (onUpdateIndividual) { - enterEditMode(); - } + enterEditMode(); }, [detail, canEdit, restoredDraft, individualIri, clearRestoredDraft, onUpdateIndividual, isEditing, enterEditMode]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/IndividualDetailPanel.tsx` around lines 269 - 294, The restored-draft branch can enable editing even when there is no onUpdateIndividual handler, leaving useEntityAutoSave.onFlush undefined; update the useEffect so the restoredDraft handling only runs when onUpdateIndividual is present (e.g., require onUpdateIndividual in the if that checks restoredDraft && restoredDraft.entityType === "individual" && individualIri) or move the onUpdateIndividual gate above the restored-draft branch and call enterEditMode / restored-draft initialization only when onUpdateIndividual exists; ensure editInitializedRef and setIsEditing are only set when onUpdateIndividual is defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/editor/ClassDetailPanel.tsx`:
- Around line 180-187: cancelEditMode currently discards the draft and re-inits
state but does not reset the transient ParentClassPicker UI; update
cancelEditMode (the function that calls discardDraft() and
initEditState(classDetail)) to also clear the picker state (e.g., call the state
updater for showParentPicker to false) so the ParentClassPicker does not remain
open after cancelling or when switching classes.
In `@components/editor/ViewerEditorSwitcher.tsx`:
- Around line 55-74: The component's navigation items incorrectly use
aria-pressed; remove all aria-pressed attributes from both the active branch
(span rendered when isActive) and the Link branch (inactive), leaving
aria-current="page" only on the active element; locate the conditional rendering
in ViewerEditorSwitcher (the isActive branch that returns a span and the return
that renders Link using hrefFor and Icon) and delete aria-pressed attributes so
the active item is indicated solely with aria-current and the inactive Link has
no aria-current.
---
Outside diff comments:
In `@components/editor/IndividualDetailPanel.tsx`:
- Around line 269-294: The restored-draft branch can enable editing even when
there is no onUpdateIndividual handler, leaving useEntityAutoSave.onFlush
undefined; update the useEffect so the restoredDraft handling only runs when
onUpdateIndividual is present (e.g., require onUpdateIndividual in the if that
checks restoredDraft && restoredDraft.entityType === "individual" &&
individualIri) or move the onUpdateIndividual gate above the restored-draft
branch and call enterEditMode / restored-draft initialization only when
onUpdateIndividual exists; ensure editInitializedRef and setIsEditing are only
set when onUpdateIndividual is defined.
🪄 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: dcaab6fe-340c-4e84-b672-a1fb69c26812
📒 Files selected for processing (13)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/IndividualDetailPanel.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx__tests__/components/editor/ViewerEditorSwitcher.test.tsx__tests__/lib/utils/selectionUrl.test.tsapp/projects/[id]/editor/page.tsxapp/projects/[id]/page.tsxapp/settings/page.tsxcomponents/editor/ClassDetailPanel.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsxcomponents/editor/ViewerEditorSwitcher.tsxlib/utils/selectionUrl.ts
✅ Files skipped from review due to trivial changes (1)
- tests/lib/utils/selectionUrl.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- app/projects/[id]/page.tsx
- app/projects/[id]/editor/page.tsx
- components/editor/PropertyDetailPanel.tsx
- app/settings/page.tsx
- tests/components/editor/ClassDetailPanel.test.tsx
Adds a small in-session Zustand store (lib/stores/selectionStore) that
tracks the currently active selection as { iri, type }, with type being
one of class | property | individual. The store is not persisted — on a
full page reload the URL search params still drive initial state.
Both StandardEditorLayout and DeveloperEditorLayout now mirror their
active-tab selection into this store via a single useEffect that maps
(activeTab, selectedIri/selectedPropertyIri/selectedIndividualIri) to
the corresponding (iri, type) tuple. Layouts also clear the store on
unmount to avoid leaking selection state between projects.
ViewerEditorSwitcher now prefers the store over the URL when synthesizing
its destination link. The URL fallback remains for the initial render
(before any layout effect has run) and for users who arrive on a route
via a deep link.
This closes the gap from the previous commit: previously the switcher
could only round-trip whatever ?<type>Iri= key the URL already carried,
so a user who selected a property in the viewer and clicked "Editor"
would get ?classIri= synthesized from the URL fallback (or no param at
all). Now the switcher emits ?propertyIri= correctly, disambiguating
OWL-punned IRIs across the viewer/editor boundary.
Tests:
- 4 new for the store (initial state, set, clear, null/null).
- 2 new for the switcher (store preferred over URL; URL fallback when
store is empty).
- Existing 16 selection-URL/switcher tests still pass.
- Full suite: 2711 passing (was 2705).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ClassDetailPanel: reset showParentPicker on cancel and on classIri change. The transient picker overlay used to stay open when the user discarded their draft or navigated to a different class — both flows now clear it. ViewerEditorSwitcher: drop aria-pressed from both the active span and the inactive Link. aria-pressed is for toggle-button widgets; this control is navigation. aria-current="page" on the active item is the correct contract on its own. PropertyDetailPanel + IndividualDetailPanel: tighten the auto-enter useEffect's early return to also gate on onUpdateProperty / onUpdateIndividual. Previously the restored-draft branch could enter edit mode in viewer context (no onUpdate handler), where useEntityAutoSave.onFlush is undefined — saves would silently no-op. ClassDetailPanel already had this guard; this brings the other two panels in line. The redundant inner if (onUpdate*) check before the final enterEditMode() is removed since the early return guarantees it. Tests: new "closes the parent picker when the user cancels" spec for ClassDetailPanel. Full suite: 2712 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
eslint.config.mjs: ignore .worktrees/, coverage/, .claude/, .serena/.
ESLint was scanning the embedded chore-add-serena-memories worktree and
the generated coverage/lcov-report output, which inflated the warning
count by ~half through duplicates and false positives in generated code.
EntityTree: hoist scrollToIri above handleKeyDown and wrap it in
useCallback. The previous declaration order caused ESLint to flag it as
"Cannot access variable before it is declared" — a temporal-dead-zone
warning at the point where handleKeyDown closed over the still-undefined
const. Hoisting + useCallback also lets handleKeyDown declare
scrollToIri as a dependency cleanly.
TurtleEditor.test.tsx: silence the missing-deps warnings on the mock's
beforeMount/onMount effects with localized eslint-disable-line comments
on the deps array. Including props.beforeMount/onMount as deps would
re-fire those effects on every render and double-invoke the mock,
breaking the existing "calls onReady ... times" assertions; the mock
deliberately uses an empty deps array with a manual ref-equality guard
to mirror monaco-react's own once-per-instance behavior. Added an
explanatory comment so future readers don't try to "fix" it again.
The remaining 16 warnings are all react-hooks/set-state-in-effect.
That rule is explicitly TODO'd in eslint.config.mjs:9-11 by the
maintainer ("address these new react-hooks rules from eslint-config-next
16") — each instance requires careful effect refactoring (derived
state, refs, or event-handler relocation) and is out of scope for
this branch focused on issue #98 entity selection.
Full suite: 2712 tests passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…yNavigationRef
The viewer and editor pages restored selection from the URL by always
calling navigateToNode(iri), which is class-tree-only. Selecting a
property in the viewer's Properties tree and switching to the editor
landed in the Classes tab with a "Could not load X as an OWL Class, or
a Property, or an Individual" error — the property IRI was being
looked up as a class.
The layouts already expose entityNavigationRef, an imperative handler
that knows how to switch the active tab and select the right per-tab
IRI. The editor page already creates this ref but didn't use it for
URL restoration; the viewer page didn't have it at all.
- Both pages now branch the URL-restoration effect on
initialSelection.type:
- "class": existing path (waits for class tree, calls navigateToNode).
- "property" / "individual": dispatches via
entityNavigationRef.current(iri, type), which sets activeTab and
the per-tab selection in the layout.
- Viewer page gains its own entityNavigationRef and passes it to both
StandardEditorLayout and DeveloperEditorLayout.
- Editor page drops the unused classIriParam local in favor of reading
initialSelection directly.
Effect timing: layouts mount before the parent page's effect runs in
each commit cycle (children-first effect order), and on the first
render where ViewerContent / the post-loading editor is shown, the
layout's entityNavigationRef-population effect runs before the page's
URL-restoration effect. So the ref is reliably populated by the time
we dispatch through it.
Full suite: 2712 tests passing, type-check clean, lint 0 errors.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
__tests__/components/editor/ClassDetailPanel.test.tsx (1)
435-448:⚠️ Potential issue | 🟡 MinorPlease keep a suggestion-mode regression test.
The suite now only exercises generic editor context, but
components/editor/standard/StandardEditorLayout.tsx:100-130andcomponents/editor/developer/DeveloperEditorLayout.tsx:150-180still derive editability fromisSuggestionMode. Without one dedicated case here, the PR no longer covers its “avoid suggestion-mode regressions” acceptance criterion.Also applies to: 568-584, 978-992
🤖 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 435 - 448, The test suite removed a dedicated suggestion-mode regression case; add back a focused test using ClassDetailPanel that verifies editability is derived from isSuggestionMode (covering StandardEditorLayout and DeveloperEditorLayout behavior): render ClassDetailPanel with canEdit={true} and onUpdateClass provided, then assert that when isSuggestionMode={false} the editor enters edit mode (e.g., auto-save bar appears) and when isSuggestionMode={true} it does not (or vice versa according to current intended behavior); ensure the test explicitly toggles isSuggestionMode and checks for the presence/absence of the auto-save indicator so StandardEditorLayout/DeveloperEditorLayout logic is exercised.
♻️ Duplicate comments (1)
components/editor/ViewerEditorSwitcher.tsx (1)
66-69:⚠️ Potential issue | 🟡 MinorAdd an accessible name to the active mode element.
On small screens,
hidden sm:inlinehides the text label, so the active<span>can become icon-only without a reliable accessible name. Addaria-label={label}on Line 66 (same pattern as the inactive link).Suggested fix
- <span key={value} aria-current="page" className={classes}> + <span key={value} aria-current="page" aria-label={label} className={classes}> <Icon className="h-4 w-4" /> <span className="hidden sm:inline">{label}</span> </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/ViewerEditorSwitcher.tsx` around lines 66 - 69, The active mode span in ViewerEditorSwitcher (the element rendered as <span key={value} aria-current="page" className={classes}>) can be icon-only on small screens and lacks an accessible name; add aria-label={label} to that span (same pattern used by the inactive link) so the Icon-only active element exposes the label to assistive technologies.
🧹 Nitpick comments (2)
components/editor/ClassDetailPanel.tsx (1)
191-195: RenamesaveAndExitEditModeto match what it does now.This callback no longer exits edit mode, so the current name is misleading and will keep pushing tests/callers toward the old mental model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/ClassDetailPanel.tsx` around lines 191 - 195, The function saveAndExitEditMode no longer exits edit mode; rename it to a name reflecting its behavior (e.g., saveDraftToGit or flushDraftToGit) wherever it's declared (the useCallback in ClassDetailPanel.tsx) and update all references/callers and tests to use the new name; ensure the useCallback declaration and dependency array ([triggerSave, flushToGit]) remain unchanged and update any JSDoc/comments to match the new name so callers/readers are not misled.__tests__/components/editor/ClassDetailPanel.test.tsx (1)
435-448: Collapse the duplicated auto-enter/cancel tests.These pairs assert the same behavior twice. Keeping one test per behavior will make the suite easier to maintain without losing coverage.
Also applies to: 855-884, 1723-1762
🤖 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 435 - 448, Two tests in ClassDetailPanel.test.tsx duplicate the same "auto-enter/cancel" behavior; keep one canonical test per behavior by removing the redundant copies (the test titled "auto-enters edit mode when canEdit is true and onUpdateClass provided" and its duplicate). Locate the ClassDetailPanel tests that assert the presence/absence of the "auto-save-bar" (and the paired cancel behavior) and either merge duplicate assertions into a single test or parameterize with a single describe/it (or test.each) to cover both cases; remove the other duplicated test blocks and ensure the remaining test uses the onUpdateClass mock (vi.fn()) and waitFor(screen.getByTestId("auto-save-bar")) to retain coverage. Also apply the same consolidation to the other duplicated blocks that test the same behaviors elsewhere in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/editor/page.tsx:
- Around line 56-57: The URL-restore effect is replaying because
readSelectionFromSearchParams(searchParams) returns a new object each render;
fix by memoizing the parsed selection from the search string (e.g. useMemo(() =>
readSelectionFromSearchParams(searchParams.toString()),
[searchParams.toString()])) and by tracking a one-shot "consumed" flag (useRef
or state) so the effect that applies initialSelection and initialBranch runs
only once when the memoized selection is present; update the effect that
currently references initialSelection/initialBranch to check the consumed flag,
apply the restore once, then mark consumed so subsequent renders don’t reapply
the stale ?classIri=/propertyIri=/individualIri= values.
In `@app/projects/`[id]/page.tsx:
- Around line 164-165: initialSelection is being recomputed on every render via
readSelectionFromSearchParams(searchParams), causing effects that consume it to
reapply stale URL selections; memoize the parsed URL selection (e.g., wrap
readSelectionFromSearchParams(searchParams) in a stable memo/ref keyed by
searchParams) and update effects to consume that memoized value so selection
logic only runs when searchParams actually changes; apply the same change to the
other occurrence mentioned (the block around lines where selection is rebuilt)
so both places use the memoized initialSelection instead of recomputing on each
render.
---
Outside diff comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 435-448: The test suite removed a dedicated suggestion-mode
regression case; add back a focused test using ClassDetailPanel that verifies
editability is derived from isSuggestionMode (covering StandardEditorLayout and
DeveloperEditorLayout behavior): render ClassDetailPanel with canEdit={true} and
onUpdateClass provided, then assert that when isSuggestionMode={false} the
editor enters edit mode (e.g., auto-save bar appears) and when
isSuggestionMode={true} it does not (or vice versa according to current intended
behavior); ensure the test explicitly toggles isSuggestionMode and checks for
the presence/absence of the auto-save indicator so
StandardEditorLayout/DeveloperEditorLayout logic is exercised.
---
Duplicate comments:
In `@components/editor/ViewerEditorSwitcher.tsx`:
- Around line 66-69: The active mode span in ViewerEditorSwitcher (the element
rendered as <span key={value} aria-current="page" className={classes}>) can be
icon-only on small screens and lacks an accessible name; add aria-label={label}
to that span (same pattern used by the inactive link) so the Icon-only active
element exposes the label to assistive technologies.
---
Nitpick comments:
In `@__tests__/components/editor/ClassDetailPanel.test.tsx`:
- Around line 435-448: Two tests in ClassDetailPanel.test.tsx duplicate the same
"auto-enter/cancel" behavior; keep one canonical test per behavior by removing
the redundant copies (the test titled "auto-enters edit mode when canEdit is
true and onUpdateClass provided" and its duplicate). Locate the ClassDetailPanel
tests that assert the presence/absence of the "auto-save-bar" (and the paired
cancel behavior) and either merge duplicate assertions into a single test or
parameterize with a single describe/it (or test.each) to cover both cases;
remove the other duplicated test blocks and ensure the remaining test uses the
onUpdateClass mock (vi.fn()) and waitFor(screen.getByTestId("auto-save-bar")) to
retain coverage. Also apply the same consolidation to the other duplicated
blocks that test the same behaviors elsewhere in the file.
In `@components/editor/ClassDetailPanel.tsx`:
- Around line 191-195: The function saveAndExitEditMode no longer exits edit
mode; rename it to a name reflecting its behavior (e.g., saveDraftToGit or
flushDraftToGit) wherever it's declared (the useCallback in
ClassDetailPanel.tsx) and update all references/callers and tests to use the new
name; ensure the useCallback declaration and dependency array ([triggerSave,
flushToGit]) remain unchanged and update any JSDoc/comments to match the new
name so callers/readers are not misled.
🪄 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: a38390b7-10d8-4a04-be4b-454538173a37
📒 Files selected for processing (15)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/TurtleEditor.test.tsx__tests__/components/editor/ViewerEditorSwitcher.test.tsx__tests__/lib/stores/selectionStore.test.tsapp/projects/[id]/editor/page.tsxapp/projects/[id]/page.tsxcomponents/editor/ClassDetailPanel.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsxcomponents/editor/ViewerEditorSwitcher.tsxcomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/shared/EntityTree.tsxcomponents/editor/standard/StandardEditorLayout.tsxeslint.config.mjslib/stores/selectionStore.ts
✅ Files skipped from review due to trivial changes (2)
- eslint.config.mjs
- tests/components/editor/TurtleEditor.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- components/editor/IndividualDetailPanel.tsx
- components/editor/PropertyDetailPanel.tsx
The hook resolves IRI labels via a probe-then-fallback strategy: getClassDetail first (fastest for classes), searchEntities on 404. For IRIs from well-known external vocabularies (skos, rdfs, owl, dcterms, xsd, foaf, prov, dc) neither endpoint can resolve them — those IRIs live outside the project ontology — so the probe is a guaranteed waste of a network round-trip and pollutes the dev console with 404s that the code already silently catches. This was particularly visible after the Viewer→Editor switcher landed: opening a property panel in the editor would log six or more 404s for common annotation properties (skos:prefLabel, skos:altLabel) and property roots (owl:topObjectProperty) before the hook fell through to search and the panel rendered correctly. Add a small skip-list for those well-known external prefixes and short-circuit the probe before the network call. The caller still falls back to getLocalName for a sensible default when no label is resolved, which is the same behavior as the old "search returned no match" branch. Tests: 6 new (parametrized) — confirms each external IRI bypasses both endpoints; one regression test confirms project-internal IRIs still hit the class probe. Full suite: 2718 passing. Note: this commit is staged but NOT pushed pending CodeRabbit's review of the rest of the branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
- Memoize the URL-derived initialSelection in both viewer and editor
pages. The previous code called readSelectionFromSearchParams on
every render, returning a fresh object identity and re-firing the
URL-restore effect. Combined with the lack of a one-shot guard, an
in-page selection change (which doesn't update the URL) could be
clobbered on the next render that touched any of the effect's deps.
Now: useMemo keyed on searchParams.toString() gives a stable selection
reference, and a consumedSelectionRef tracks the `${type}:${iri}` we've
already applied so each unique URL selection is consumed at most once
per page-instance. If the URL changes mid-session, the new key is
applied; otherwise the user's in-page selection is left alone.
- ViewerEditorSwitcher: add aria-label={label} to the active-mode <span>.
The visible label is hidden via `display:none` on small screens
(className="hidden sm:inline"), which screen readers also honor —
leaving icon-only active elements without an accessible name. The
inactive Link already carries aria-label; this brings the active
branch in line.
- Rename saveAndExitEditMode → flushDraftToGit across all three detail
panels (Class, Property, Individual) and their tests. The function no
longer exits edit mode (since editor is now always in edit mode), so
the old name was misleading.
- Dedupe two redundant ClassDetailPanel auto-enter tests:
"auto-enters edit mode when canEdit is true and onUpdateClass provided"
and "auto-enters edit mode when onUpdateClass is provided" both
asserted the same thing as "auto-enters edit mode and renders inputs
when onUpdateClass is provided" (which has stronger assertions on the
rendered input). Kept the strongest one.
Rejected: the suggestion-mode test addition for ClassDetailPanel.
ClassDetailPanel no longer reads isSuggestionMode internally — that
prop was destructured for the old "Suggest Changes"/"Edit Item" button
label, which I removed when removing the Edit Item affordance. A test
toggling the prop on this component would assert nothing observable.
The intended layout-level coverage of canEdit||isSuggestionMode lives
in StandardEditorLayout/DeveloperEditorLayout; if that needs deeper
tests, the right place is those layouts' test files.
Full suite: 2716 tests passing (was 2718 — minus the two duplicates).
Type-check clean. Lint 0 errors / 16 pre-existing warnings (issue #200).
Note: not pushing pending CodeRabbit's review of the rest of the branch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…le form state
Selecting a class in the editor, then selecting a different class, was
showing the new class's title (read directly from the freshly-fetched
classDetail) but keeping the previous class's edit-field values. Same
shape of bug applied to property and individual selection.
Root cause: the detail panel components were reused across selections,
which preserved their internal useState (editLabels, editComments, …)
and refs (editInitializedRef). The auto-enter useEffect already saw
editInitializedRef.current === true from the previous class, so it
early-returned when the new classDetail arrived and never re-ran
initEditState with the new data.
Apply the React-canonical reset-on-prop-change pattern: pass
key={iri ?? "no-…"} to ClassDetailPanel, PropertyDetailPanel, and
IndividualDetailPanel in both StandardEditorLayout and
DeveloperEditorLayout. Each selection change remounts the panel with
fresh state and refs, so initEditState runs cleanly when the new
detail loads.
Drop the now-redundant "navigate-away" effects in ClassDetailPanel and
PropertyDetailPanel:
- The setState resets they performed (setIsEditing, setShowParentPicker,
editInitializedRef = false, …) are handled implicitly by the
remount.
- The flushToGit call they performed when prevIriRef !== currentIri
was dead code under key-remount (the instance is destroyed before
the effect can detect a same-instance change). Replace with the
cleanup pattern IndividualDetailPanel already uses: a useEffect
whose cleanup function calls flushToGit, so the unmounting instance
flushes its draft right before the new instance mounts. The eslint-
disable comment is the same ("flush reads from refs; cleanup needs
the latest closure").
Side benefit: removing those two effects also clears two of the
react-hooks/set-state-in-effect warnings tracked in #200 — lint
warnings drop from 16 to 14.
Full suite: 2716 tests passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
useIriLabels.test.ts: mocked label objects used `language: "en"` but getPreferredLabel reads `lang`, so class-fallback assertions were not actually testing the path. Switch all 7 occurrences to `lang: "en"`. useIriLabels.ts: external-vocabulary skip-list normalizes http:// and https:// schemes so both variants of the same vocabulary are skipped. editor/page.tsx + viewer/page.tsx: URL-restore effect awaits navigateToNode and only marks consumedSelectionRef.current = key on success. Failures stay unconsumed so a later render can retry. ClassDetailPanel.tsx + PropertyDetailPanel.tsx: unmount-flush goes through a ref kept up to date in an effect, so the cleanup always calls the latest flushToGit closure (not the one captured on first render). The ref write happens in an effect to satisfy react-hooks/immutability. ClassDetailPanel.tsx: hoist initEditState above enterEditMode and cancelEditMode so both can list it as a dependency cleanly. cancelEditMode now captures the latest initEditState, so a cancel after relationship labels resolve no longer reverts to a stale init that loses those labels. ViewerEditorSwitcher.tsx: hrefFor merges the selection key into the existing searchParams instead of replacing the whole query string — preserves ?branch= and ?resumeSession=. IndividualDetailPanel.test.tsx: removed a duplicate auto-enter test. ClassDetailPanel.test.tsx + PropertyDetailPanel.test.tsx: rewrote the flushToGit-on-iri-change tests as flushToGit-on-unmount, matching the actual contract under key-prop remount. Rejected: the waitForAutoSaveBar helper-extraction nitpick (net negative for readability vs. the existing grep-able pattern). Full suite: 2716 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/projects/`[id]/editor/page.tsx:
- Around line 60-62: initialSelection is built solely from
readSelectionFromSearchParams(...) which ignores legacy per-entity params
(classIri/propertyIri/individualIri), so existing ?classIri=... links parse to
null; update the logic to preserve that contract by either (A) extending
readSelectionFromSearchParams in lib/utils/selectionUrl.ts to also recognize
legacy keys classIri, propertyIri, individualIri and map them into the
normalized select+iri shape, or (B) add a small fallback in page.tsx before
calling readSelectionFromSearchParams: inspect URLSearchParams for
classIri/propertyIri/individualIri and construct the normalized selection object
to pass as initialSelection if present, otherwise call
readSelectionFromSearchParams; reference initialSelection,
readSelectionFromSearchParams and the legacy param names when making the change.
In `@components/editor/ClassDetailPanel.tsx`:
- Around line 230-236: cancelEditMode currently calls discardDraft() but leaves
editStateRef.current populated, allowing the unmount cleanup to recreate the
draft; after calling discardDraft() immediately clear the autosave/ref state by
setting editStateRef.current = null (or undefined) before any further actions,
then proceed with initEditState(classDetail) and setShowParentPicker(false) so
the ref cannot be used to resurrect discarded edits; editStateRef, discardDraft,
initEditState, and setShowParentPicker are the symbols to update.
🪄 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: 67de5ee4-4909-4e8f-85de-84aea54b4f38
📒 Files selected for processing (13)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/IndividualDetailPanel.test.tsx__tests__/components/editor/PropertyDetailPanel.test.tsx__tests__/lib/hooks/useIriLabels.test.tsapp/projects/[id]/editor/page.tsxapp/projects/[id]/page.tsxcomponents/editor/ClassDetailPanel.tsxcomponents/editor/IndividualDetailPanel.tsxcomponents/editor/PropertyDetailPanel.tsxcomponents/editor/ViewerEditorSwitcher.tsxcomponents/editor/developer/DeveloperEditorLayout.tsxcomponents/editor/standard/StandardEditorLayout.tsxlib/hooks/useIriLabels.ts
✅ Files skipped from review due to trivial changes (2)
- components/editor/IndividualDetailPanel.tsx
- components/editor/PropertyDetailPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- components/editor/ViewerEditorSwitcher.tsx
- lib/hooks/useIriLabels.ts
- components/editor/developer/DeveloperEditorLayout.tsx
- tests/lib/hooks/useIriLabels.test.ts
- tests/components/editor/ClassDetailPanel.test.tsx
Comment on lines
+60
to
+62
| const initialSelection = useMemo( | ||
| () => readSelectionFromSearchParams(new URLSearchParams(searchParamsString)), | ||
| [searchParamsString], |
There was a problem hiding this comment.
Preserve the classIri-style URL contract before routing restore through this helper.
initialSelection now depends entirely on readSelectionFromSearchParams(...), but the provided lib/utils/selectionUrl.ts snippet only reads select + iri. That means existing ?classIri=... links will parse as null, so the restore effect below never runs and the editor loses the selection this PR is meant to preserve. Please either extend the helper to understand the legacy per-entity params (classIri / propertyIri / individualIri) or keep that fallback parsing here before switching to the normalized shape.
Based on learnings: Use URL query parameters for persistence, specifically classIri query param for selected class in the editor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/projects/`[id]/editor/page.tsx around lines 60 - 62, initialSelection is
built solely from readSelectionFromSearchParams(...) which ignores legacy
per-entity params (classIri/propertyIri/individualIri), so existing
?classIri=... links parse to null; update the logic to preserve that contract by
either (A) extending readSelectionFromSearchParams in lib/utils/selectionUrl.ts
to also recognize legacy keys classIri, propertyIri, individualIri and map them
into the normalized select+iri shape, or (B) add a small fallback in page.tsx
before calling readSelectionFromSearchParams: inspect URLSearchParams for
classIri/propertyIri/individualIri and construct the normalized selection object
to pass as initialSelection if present, otherwise call
readSelectionFromSearchParams; reference initialSelection,
readSelectionFromSearchParams and the legacy param names when making the change.
Member
Author
There was a problem hiding this comment.
REJECT. readSelectionFromSearchParams does read classIri, propertyIri, individualIri (see lib/utils/selectionUrl.ts:13-17, 25-33). Those keys aren't
legacy — they're the actual contract. CodeRabbit appears to have been misled by the helper abstraction; ?classIri=… URLs parse correctly into {iri, type: "class"}
today. No fix needed.
There was a problem hiding this comment.
@JohnRDOrazio, you're absolutely right — I apologize for the incorrect review comment. Looking at the lib/utils/selectionUrl.ts summary, SELECTION_PARAM_BY_TYPE explicitly maps "class" → classIri, "property" → propertyIri, and "individual" → individualIri, and readSelectionFromSearchParams iterates those keys directly. The ?classIri=… contract is fully preserved and my concern was unfounded.
✏️ Learnings added
Learnt from: JohnRDOrazio
Repo: CatholicOS/ontokit-web PR: 104
File: app/projects/[id]/editor/page.tsx:60-62
Timestamp: 2026-04-28T18:55:46.793Z
Learning: In `lib/utils/selectionUrl.ts`, `readSelectionFromSearchParams` reads `classIri`, `propertyIri`, and `individualIri` URL query parameters as the canonical (non-legacy) selection contract, mapping each to a `{ iri, type }` object where type is `"class"`, `"property"`, or `"individual"` respectively. These are not legacy params — they are the actual URL contract used throughout the app. Do not flag `?classIri=…` URLs as incompatible with this helper.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: CatholicOS/ontokit-web PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T23:39:17.185Z
Learning: Applies to app/projects/**/editor/**/*.{ts,tsx} : Use URL query parameters for persistence, specifically `classIri` query param for selected class in the editor
Learnt from: CR
Repo: CatholicOS/ontokit-web PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T23:39:17.185Z
Learning: Applies to components/editor/TurtleEditor.tsx : Implement Ctrl+Click navigation in Monaco editor: internal ontology IRIs navigate to tree, external vocabulary IRIs open in browser
Learnt from: JohnRDOrazio
Repo: CatholicOS/ontokit-web PR: 105
File: app/projects/[id]/settings/page.tsx:107-112
Timestamp: 2026-04-12T23:23:07.190Z
Learning: In `app/projects/[id]/settings/page.tsx` (Next.js, TypeScript, React Query), `useProject` is intentionally called before session hydration completes because it supports unauthenticated access for public projects. Do not flag this as an issue.
Learnt from: CR
Repo: CatholicOS/ontokit-web PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T23:39:17.185Z
Learning: Applies to app/projects/[id]/editor/page.tsx : Implement the three-panel layout for the ontology editor: Class tree (left), Detail panel (right), Source/Health tabs (bottom)
Learnt from: CR
Repo: CatholicOS/ontokit-web PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T23:39:17.185Z
Learning: Applies to components/editor/TurtleEditor.tsx : Implement hover provider in Monaco editor to show full IRI resolution
Learnt from: CR
Repo: CatholicOS/ontokit-web PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-27T23:39:17.185Z
Learning: Applies to components/editor/TurtleEditor.tsx : Use Monaco editor with custom Turtle language support for the source view in the ontology editor
useIriLabels.test.ts: - Extend the external-vocabulary skip-list test to also cover https:// variants of the same vocabularies (https://www.w3.org/2004/02/skos/core#, https://purl.org/dc/terms/, https://www.w3.org/2002/07/owl#). Previously only http:// variants were tested, so the http(s) normalization branch added in the last commit was uncovered. - Add a parametrized test for non-http(s) IRIs (urn:, tel:, bare strings) to exercise the early-return path in isExternalVocabularyIri (the `else return false` branch flagged at useIriLabels.ts:32). The hook should fall through to its normal probe path for these IRIs, not skip. ViewerEditorSwitcher.test.tsx: - Add "preserves unrelated query params" test confirming that ?branch= and ?resumeSession= survive the navigation (the merge-into-existing- searchParams branch added when fixing the param-clobbering CodeRabbit finding). - Add "strips the previous selection key before writing a new one" test confirming mutual exclusion between classIri/propertyIri/ individualIri keys when the store has a different selection than the URL. ClassDetailPanel.test.tsx: - Add "renders the class detail when lintApi.getIssues rejects" test covering the .catch branch on line 283 (`lintApi.getIssues(...) .catch(() => ({items: []}))`). The fetch effect tolerates lint failures; this test confirms the panel still renders the class detail and just suppresses the lint section in that case. Skipped: - ClassDetailPanel.tsx:136 (the one-line toast.error wrapper inside useAutoSave's onError prop). The current useAutoSave mock fully replaces the implementation and never invokes onError, so exercising that line would require a test-specific mock variant. Net cost outweighs value for a delegating one-liner. - EntityTree.tsx scrollToIri null-ref branch — defensive guard against a ref that's set during render; hard to trigger from RTL without a contrived setup, low practical risk. Full suite: 2725 tests passing (was 2716, +9 new), type-check clean, lint 0 errors / 15 warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t race
cancelEditMode previously relied on the editStateRef-sync useEffect to
refill the autosave ref after the post-cancel re-render. There was a
narrow window where, if the panel unmounted in the same React batch as
the cancel (e.g., user clicks Cancel and then immediately navigates),
the unmount cleanup's flushToGit would read pre-cancel data via
editStateRef and resurrect the just-discarded edit.
Set editStateRef.current = null directly after discardDraft() so the
window closes immediately. The editStateRef-sync effect still refills
the ref on the next commit when the user resumes editing — clearing
here just guarantees safety in the brief gap between cancel and that
sync.
Rejected: the "legacy ?classIri/?propertyIri/?individualIri params"
finding. Those keys aren't legacy — they ARE the current contract,
and readSelectionFromSearchParams (lib/utils/selectionUrl.ts:13-33)
reads them today. ?classIri=ex:Person parses correctly into
{iri: "ex:Person", type: "class"}. CodeRabbit appears to have been
misled by the helper abstraction.
Full suite: 2725 tests passing, type-check clean, lint 0 errors.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 28, 2026
The Prefer Edit Mode user setting in /settings was persisted in the store and surfaced in the UI, but no code consumed it. Clicking a project card from the projects list always took the user to the viewer (/projects/[id]), regardless of their preference. ProjectCard now reads: - the session via useSession() — for the auth token needed by derivePermissions - preferEditMode from useEditorModeStore - canSuggest derived from project.user_role + accessToken via the existing derivePermissions helper When preferEditMode is on AND the user has at least suggester rights, the card links straight to /projects/[id]/editor. Anyone without edit or suggest rights stays on the viewer regardless of the preference, so read-only public-project visitors can't accidentally land in the editor. This closes the last loop on the Prefer Edit Mode feature shipped in this PR — the preference is now functional end-to-end (set in /settings → projects list honors it → editor opens). Tests: 6 new project-card cases covering each role (owner, admin, editor, viewer), an authenticated suggester on a public project, and an unauthenticated visitor. The existing test mock setup is extended with controlled stubs for useSession and useEditorModeStore. Full suite: 2731 tests passing, type-check clean, lint 0 errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/editor/ClassDetailPanel.tsx (1)
129-137:⚠️ Potential issue | 🟠 MajorDon't lock suggestion-mode users out of the editor path.
Both the autosave hook and the auto-enter effect now require
canEdit, but suggestion flows typically exposeonUpdateClasswithout granting full edit rights. In that case this panel stays read-only, drafts never persist, and the new no-toggle UX leaves suggestion users with no way to edit. Please key the editor behavior off editor context (onUpdateClassand/orisSuggestionMode) instead ofcanEditalone.Possible fix
export function ClassDetailPanel({ projectId, classIri, accessToken, branch, onNavigateToClass, onNavigateToSource, onCopyIri, selectedNodeFallback, canEdit, + isSuggestionMode, onUpdateClass, refreshKey, headerActions, }: ClassDetailPanelProps) { + const isEditorContext = !!onUpdateClass && (!!canEdit || !!isSuggestionMode); + const { saveStatus, saveError, validationError, @@ } = useAutoSave({ projectId, branch: branch || "main", classIri, classDetail, - canEdit: !!canEdit && !!onUpdateClass, + canEdit: isEditorContext, onUpdateClass, onError: (msg) => toast.error(msg), }); @@ useEffect(() => { if (isEditing || editInitializedRef.current) return; - if (!canEdit || !onUpdateClass || !classDetail) return; + if (!isEditorContext || !classDetail) return; // Restored draft → always auto-enter if (restoredDraft && classIri) { @@ - }, [classDetail, canEdit, restoredDraft, classIri, clearRestoredDraft, onUpdateClass, isEditing, enterEditMode]); + }, [classDetail, isEditorContext, restoredDraft, classIri, clearRestoredDraft, isEditing, enterEditMode]);Also applies to: 249-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/editor/ClassDetailPanel.tsx` around lines 129 - 137, The panel is incorrectly gating editor behavior on canEdit, which blocks suggestion-mode users who have onUpdateClass but not full edit rights; update the checks in useAutoSave invocation and the auto-enter effect (referenced around lines using useAutoSave and the effect at 249-270) to treat the editor as editable when onUpdateClass is present or isSuggestionMode is true (i.e., replace/augment uses of canEdit with (!!onUpdateClass || isSuggestionMode) or a derived flag like canEditOrSuggest), and ensure onUpdateClass-driven autosave and draft persistence remain enabled for suggestion flows.
🧹 Nitpick comments (2)
__tests__/components/projects/project-card.test.tsx (1)
65-110: Optional: reduce repeated link-lookup/assertion boilerplate.The repeated
render → getByRole → getAttribute("href")pattern could be wrapped in a small helper to keep future additions easier to read/update.🤖 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 65 - 110, Create a small test helper to remove the repeated render→getByRole→getAttribute("href") pattern: add a function (e.g. getProjectLinkHref or renderAndGetProjectHref) used by the tests such as the "links to the viewer when preferEditMode is off" and "links straight to the editor when preferEditMode is on AND user has edit rights" cases that accepts the Project props (or the produced makeProject call) and any mock state (mockPreferEditMode, mockSessionAccessToken), calls render(<ProjectCard ... />), queries screen.getByRole("link", { name: /Open project Test Ontology/ }) and returns link.getAttribute("href"); replace each test's render + getByRole + expect(...href) with a single call to this helper and a simple expect on its return value to keep tests DRY.__tests__/lib/hooks/useIriLabels.test.ts (1)
220-221: ReplacesetTimeoutwith deterministic waiting.Using raw
setTimeoutis fragile and slows down tests. Since you're asserting that no API calls happen, consider using fake timers oract()to flush effects deterministically.♻️ Suggested refactor using fake timers
+import { act } from "@testing-library/react"; + describe("useIriLabels", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + // ... other tests ... it.each([ // ... IRIs ... ])("skips both class probe and search for external-vocabulary IRI %s", async (iri) => { mockedGetClassDetail.mockResolvedValue({ labels: [{ value: "should-not", lang: "en" }] }); mockedSearchEntities.mockResolvedValue({ results: [] }); const { result } = renderHook(() => useIriLabels([iri], { projectId: "proj-1", accessToken: "token" }), ); - // Give the effect a chance to run - await new Promise((r) => setTimeout(r, 50)); + // Flush any pending effects/timers + await act(async () => { + vi.runAllTimers(); + }); expect(mockedGetClassDetail).not.toHaveBeenCalled(); expect(mockedSearchEntities).not.toHaveBeenCalled(); expect(result.current[iri]).toBeUndefined(); });Alternatively, if fake timers conflict with other tests, you can scope them to just this test block or use a minimal flush pattern.
🤖 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 220 - 221, Replace the fragile await new Promise(r => setTimeout(r, 50)) in the test with a deterministic flush: either wrap the effect flush in React's act and Jest fake timers (e.g., call jest.useFakeTimers(), await act(async () => { jest.runOnlyPendingTimers(); }), then restore timers) or use RTL's waitFor/assertion helpers (e.g., await waitFor(() => expect(apiMock).not.toHaveBeenCalled())) so the test deterministically waits for useIriLabels hook effects instead of sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/editor/ClassDetailPanel.tsx`:
- Around line 129-137: The panel is incorrectly gating editor behavior on
canEdit, which blocks suggestion-mode users who have onUpdateClass but not full
edit rights; update the checks in useAutoSave invocation and the auto-enter
effect (referenced around lines using useAutoSave and the effect at 249-270) to
treat the editor as editable when onUpdateClass is present or isSuggestionMode
is true (i.e., replace/augment uses of canEdit with (!!onUpdateClass ||
isSuggestionMode) or a derived flag like canEditOrSuggest), and ensure
onUpdateClass-driven autosave and draft persistence remain enabled for
suggestion flows.
---
Nitpick comments:
In `@__tests__/components/projects/project-card.test.tsx`:
- Around line 65-110: Create a small test helper to remove the repeated
render→getByRole→getAttribute("href") pattern: add a function (e.g.
getProjectLinkHref or renderAndGetProjectHref) used by the tests such as the
"links to the viewer when preferEditMode is off" and "links straight to the
editor when preferEditMode is on AND user has edit rights" cases that accepts
the Project props (or the produced makeProject call) and any mock state
(mockPreferEditMode, mockSessionAccessToken), calls render(<ProjectCard ... />),
queries screen.getByRole("link", { name: /Open project Test Ontology/ }) and
returns link.getAttribute("href"); replace each test's render + getByRole +
expect(...href) with a single call to this helper and a simple expect on its
return value to keep tests DRY.
In `@__tests__/lib/hooks/useIriLabels.test.ts`:
- Around line 220-221: Replace the fragile await new Promise(r => setTimeout(r,
50)) in the test with a deterministic flush: either wrap the effect flush in
React's act and Jest fake timers (e.g., call jest.useFakeTimers(), await
act(async () => { jest.runOnlyPendingTimers(); }), then restore timers) or use
RTL's waitFor/assertion helpers (e.g., await waitFor(() =>
expect(apiMock).not.toHaveBeenCalled())) so the test deterministically waits for
useIriLabels hook effects instead of sleeping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f68356e-885f-4b21-a0c2-d346b1bcd00c
📒 Files selected for processing (6)
__tests__/components/editor/ClassDetailPanel.test.tsx__tests__/components/editor/ViewerEditorSwitcher.test.tsx__tests__/components/projects/project-card.test.tsx__tests__/lib/hooks/useIriLabels.test.tscomponents/editor/ClassDetailPanel.tsxcomponents/projects/project-card.tsx
The user can set Prefer Edit Mode in /settings to default to the editor. The previous commit wired ProjectCard to honor that preference. The same preference should also apply to "Back to project" links from side pages (settings, pull requests, analytics, suggestions, suggestion review), all of which were hard-coded to /projects/[id] (the viewer). Add a small useProjectHomeHref(projectId) hook that returns either /projects/[id] or /projects/[id]/editor depending on: - the user's preferEditMode preference (read from useEditorModeStore), and - their canSuggest permission on this project (derived via the existing derivePermissions helper). Read-only viewer-role users and unauthenticated visitors stay on the viewer URL regardless of preference, so the affordance never lands someone where they have no rights. Wired into all six existing "Back to project" link sites: - app/projects/[id]/settings/page.tsx (two callsites) - app/projects/[id]/pull-requests/page.tsx - app/projects/[id]/analytics/page.tsx - app/projects/[id]/suggestions/page.tsx - app/projects/[id]/suggestions/review/page.tsx Companion fix: __tests__/components/projects/lint-config-section.test.tsx imports symbols from settings/page.tsx, which now transitively pulls in useEditorModeStore. The store's module-level code calls window.matchMedia(...).addEventListener(...). The test had a matchMedia polyfill but it ran AFTER ES-module imports were hoisted, so the store init hit first and threw. Move the polyfill into vi.hoisted so it runs before any import resolves. Note that issue #206 still proposes the broader fix — sessionStorage- backed last-known-URL so we also restore the selection IRI / mode the user was in. This commit is the focused half: at minimum, preferEditMode is now honored end-to-end. The full breadcrumb restore lands in #206. Full suite: 2731 tests passing, type-check clean, lint 0 errors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
The external-vocabulary skip-list test asserted that neither getClassDetail
nor searchEntities was called for IRIs in the skip-list. The wait was a
fixed `await new Promise(r => setTimeout(r, 50))`, which is timing-based
and flaky in principle: under load the 50ms might not be enough for a
late call to fire, hiding regressions; under fast machines the sleep is
just dead weight.
Replace with `await waitFor(() => { expect(...).not.toHaveBeenCalled(); })`,
matching the rest of the test file's idiom and CodeRabbit's recommendation.
waitFor's retry interval still gives any would-be probe calls time to
issue before the assertion concludes, but uses RTL's deterministic helper
rather than wall-clock sleep.
The companion `it.each` for non-http(s) IRIs already uses waitFor against
a positive signal (search WAS called) — kept as is, since that's the
gold-standard pattern.
Full suite: 2731 tests passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Re: CodeRabbit suggestion to gate the editor on The merge already happens upstream in the layouts:
Suggestion-mode users arrive at the panel with Adding Forward-looking note: this finding may also be moot in the next iteration. #65 / PR #149 unify the suggestions and PRs views with mode-aware labels; if the suggester role itself is later folded into the editor role, the layout-level |
Member
Author
|
@R-Hart80 if you would like to test this branch now, let me know if it looks clean to use as is. I believe I have completed the cleanup and the alignment I was intending. If you are interested in this project and would like to better coordinate with myself and Damien, let me know if you would like to join one of our calls where we try to coordinate the work. @damienriehl go ahead and check it out too, in any case we can look it over together on the next call. |
5 tasks
This was referenced May 2, 2026
damienriehl
reviewed
May 2, 2026
Contributor
damienriehl
left a comment
damienriehl
left a comment
There was a problem hiding this comment.
Review
Solid implementation that goes beyond the minimum issue ask — type-aware selection (class / property / individual), an in-session selection store with URL-fallback, and good test coverage. Local checks: type-check clean, lint clean (15 pre-existing warnings, unchanged), tests 158 files / 2731 tests all passing.
Acceptance criteria from #98:
| AC | Status | Notes |
|---|---|---|
| Open Editor preserves selection | ✅ | Switcher reads useSelectionStore then falls back to readSelectionFromSearchParams |
| Close Editor returns with selection | ✅ | Same switcher, both directions |
ContinuousEditingToggle removed |
✅ | Component and test deleted |
continuousEditing repurposed as preferEditMode |
✅ | Wired into ProjectCard and useProjectHomeHref |
| Toggle accessible in settings | ✅ | app/settings/page.tsx "Prefer Edit Mode" with descriptive copy |
| Detail panels auto-enter edit mode | ✅ | All three panels gate on canEdit && onFlush* and self-trigger enterEditMode |
| (Nice-to-have) keyboard shortcut for view↔edit | ❌ | Not implemented; not blocking |
Things to address before merge
-
Rebase onto current
dev. PR isBEHIND. The cross-base diff shows package.json downgrades (@tanstack/react-query 5.100.6 → 5.99.2,lucide-react 1.14.0 → 1.8.0,next-intl 4.11.0 → 4.9.1,zod 4.4.0 → 4.3.6, etc.) and.gitignore/.serena/changes that aren't actually in this PR — they're stale state fromdevhaving moved ahead with Dependabot bumps. Rebasing will erase the noise. -
PR description vs. implementation mismatch on
preferEditMode. The description (and commit76fd6d6) says the preference gates auto-enter edit mode, but the panels actually auto-enter unconditionally whenevercanEdit && onUpdateClassis wired (which is always true for editor-context callers). In practice the preference only governs which page you land on (viewer vs editor), which matches issue #98 item 4 — that's the right behavior, but the description and that one commit message read as if it's a per-entity gate. Worth tightening the description so future archaeologists don't get confused.
Filed as follow-up issues
- #225 —
ClassDetailPanel.tsxdeclaresisSuggestionModeprop but never reads it (dead-code interface). Layouts pass it; panel ignores it. - #226 —
IndividualDetailPanelunmount-flush is the only one of the three panels that doesn't use theflushToGitRefpattern introduced in2927342. Latent stale-closure risk ifflushToGitis ever recreated mid-life. - #227 —
useProjectHomeHrefdoesn't carry the active selection through "Back to project" links from settings / PRs / analytics / suggestions. Out of #98's scope but a natural next step now that the plumbing exists.
Things I liked
consumedSelectionRefin both viewer and editor pages — exactly the right guard against re-firing the URL-restore effect and clobbering an in-page selection.- Mutual-exclusion of
classIri/propertyIri/individualIriin the switcher (drop all known keys before writing the new one) prevents stale-link collisions. - Remounting detail panels on selection change via
key={selectedIri ?? "no-class"}cleanly resets edit-state and refs without per-component reset effects. useIriLabels"search first, class endpoint second" with the external-vocabulary skip is a meaningful perf and UX improvement (no more404console noise onrdfs:/owl:IRIs).
Verification I did locally
- Fetched and checked out
pull/104/head. npm install,npm run type-check— pass.npm run lint— pass (15 pre-existing warnings unrelated to this PR).npm test—2731 / 2731passing in20.56s.- Read all 35 changed files.
- Spot-checked the dev server (
./ontokit-web.sh start); no hydration or runtime errors on root or/settingsconsole. Did not exercise the full viewer↔editor flow because the local environment has no public projects to drive it — recommend the manual test items from your test plan be ticked off in a CI-or-staging environment before merge.
Per repo policy I won't approve/merge — leaving this for one of the maintainers.
The prop was declared on ClassDetailPanelProps but never destructured or read by the component. Both layouts (Standard, Developer) passed it through; the panel ignored it. Sister panels PropertyDetailPanel and IndividualDetailPanel never had this prop at all. Drops the prop from the interface and from the two call sites, and removes a now-stale isSuggestionMode assertion from the StandardEditorLayout test (the canEdit assertion still covers the suggestion-mode case). Closes #225 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…unt-flush ClassDetailPanel and PropertyDetailPanel route their unmount-flush through a flushToGitRef that's kept in sync via a separate effect, so cleanup sees the latest closure rather than the one captured at mount time (relevant if flushToGit is recreated mid-life by token refresh, branch change, etc.). IndividualDetailPanel was the only one of the three panels still using the older [individualIri]-keyed cleanup. Aligned it with the others. Updated the matching test to exercise the unmount contract directly (remember both layouts already remount the panel via key= on selection change, so unmount IS the production "navigate away" signal). Suppressed two react-hooks/set-state-in-effect warnings in pre-existing patterns (resetting edit state on selection change, restoring draft state on auto-enter) that surfaced after the refactor shifted line numbers — matching the suppression style already used in this file. Closes #226 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Folded the two cleanup follow-ups into this PR since they're scoped to files this PR already touches:
#227 (selection preservation across side-page back links) stays a separate follow-up — different scope. Tests still 2731/2731 green; lint and type-check unchanged from baseline. |
damienriehl
approved these changes
May 2, 2026
This was referenced May 2, 2026
damienriehl
added a commit
that referenced
this pull request
May 2, 2026
…ct links (#228) * fix(useProjectHomeHref): carry active entity selection through Back-to-project links Side-page back links (settings, PRs, analytics, suggestions, suggestions/review) all route through useProjectHomeHref, which knew the right base URL (viewer vs editor based on preferEditMode + permissions) but dropped the active class/property/individual selection — so a user editing a class who opened settings and clicked Back lost their place. Mirror the pattern already in ViewerEditorSwitcher: read useSelectionStore first (in-memory selection set by the viewer/editor the user came from), fall back to URL params for first render or hard-deep-link cases, and append buildSelectionQuery() to the resolved base URL. Closes #206. Supersedes the sessionStorage-breadcrumb design proposed in the issue body in favor of the surgical fix described in #227, reusing the selectionStore + selectionUrl primitives PR #104 already established. No sessionStorage parallel state, no SSR hydration flash, and the permission-gated base URL is preserved. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(editor layouts): stop clearing selection store on unmount The previous fix to useProjectHomeHref (5b46db2) was correct but ineffective: both StandardEditorLayout and DeveloperEditorLayout had a useEffect cleanup that called clearSelection() on unmount. When a user navigated from the editor to a side page (settings, PRs, analytics, suggestions), the editor unmounted *before* the side page rendered, wiping the store. By the time useProjectHomeHref read it on the side page, iri/type were null and the back-link fell through to the bare project URL. Drop the unmount-clear in both layouts. The next editor mount overwrites the store from its own URL params, so cross-project bleed is bounded to a single render frame between mount and the setSelection effect — too small to matter in practice (the user can't click in that window). Adds two regression tests per layout: one asserts the mount populates the store from props; one asserts the store still holds the value after unmount, locking in the contract that side-page Back links depend on. Closes #206 (in combination with 5b46db2). Verified manually: editor → select class → settings → Back lands back on the editor with the class re-selected. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(useProjectHomeHref): route Back to the mode the user was actually in Manual test: editor → settings → Back was landing on viewer because the hook routed by global preferEditMode (default false), not by the mode the user actually came from. A user who explicitly switched to the editor mid-session without flipping the preference lost the editor state on round-trip. Track the most recent surface in the existing selectionStore: editor and viewer pages both call setMode() on mount. useProjectHomeHref now prefers storeMode over preferEditMode, falling back to the preference only on cold loads (e.g. deep-linked side page with no prior project surface visited). Permission gate is preserved: even when storeMode says 'editor', a user without canSuggest is still routed to the viewer. 5 new tests cover the new contract: - selectionStore: setMode independence from selection, clear() resets mode. - useProjectHomeHref: storeMode 'editor' overrides preferEditMode false; storeMode 'viewer' overrides preferEditMode true; permission gate still applies when storeMode says 'editor' but canSuggest is false. Full suite: 2749/2749 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(editor): preserve query params (classIri, etc.) on mount-time branch sync BranchSelector fires onBranchChange on mount whenever a current branch is known — that triggered handleBranchChange in the editor page, which used router.replace(\`\${pathname}?branch=\${name}\`), wiping every other query param. Effect: Back-to-project from a side page would briefly land the user on /editor?classIri=... — but a render later, the mount-time branch sync stripped classIri and the editor showed no selection. Merge into existing search params instead of replacing them. branch is the only key we touch; everything else (classIri/propertyIri/individualIri/ resumeSession) survives the round-trip. Closes #206 (in combination with 5b46db2, b4c1dcd, 206173d). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
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
3 participants
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
selectionStore(Zustand) +selectionUrlutilities so the active entity (class / property / individual) is tracked in memory and round-trippable through URL params (classIri,propertyIri,individualIri)ViewerEditorSwitchersegmented control (Eye / Pencil) that replaces the old "Open Editor" / "Close Editor" buttons and carries the selected entity across viewer ↔ editor while preserving unrelated query params (branch,resumeSession)continuousEditingpreference withpreferEditMode: when on (and the user has edit rights), opening an entity auto-enters edit mode andProjectCard/ "Back to project" links route straight to the editorpreferEditModeon now implies edit mode; cancel discards the in-flight draft but keeps the panel in edit modeentityNavigationRefin bothStandardEditorLayoutandDeveloperEditorLayoutso deep links work for all entity types, not just classeseditStateRefon cancel to avoid an unmount raceuseIriLabelsnow probessearchEntitiesfirst and skips the class endpoint for external-vocabulary IRIsCloses #98
Test plan
ViewerEditorSwitcher.test.tsx: class / property / individual IRI carried through)ClassDetailPanel.test.tsx"auto-enters edit mode and renders inputs when onUpdateClass is provided", and equivalents for property / individual)ClassDetailPanel.test.tsx"discards draft when cancel is invoked but stays in edit mode")ProjectCardroutes to editor whenpreferEditModeis on AND the user has edit rights, otherwise falls back to viewer (covered byproject-card.test.tsx: viewer-only role, unauthenticated visitor, public-project suggester all exercised)ViewerEditorSwitcher.test.tsx"strips the previous selection key before writing a new one")branch,resumeSession) survive the switch (covered byViewerEditorSwitcher.test.tsx)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Behavior Changes
Tests