fix: open the note editor from Write Note - #1286
Conversation
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 5/5
frontend/src/app/(dashboard)/notebooks/components/NotesColumn.test.tsxonly partially exercises the editor lifecycle, so a regression when reopening the same or another note afteronOpenChange(false)could go undetected; add an explicit reopen assertion to cover that path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/app/(dashboard)/notebooks/components/NotesColumn.test.tsx">
<violation number="1" location="frontend/src/app/(dashboard)/notebooks/components/NotesColumn.test.tsx:100">
P3: The edit path is only exercised once. Test 3 clicks a note row (edit mode), closes the editor, then clicks Write Note (create mode) — it never re-opens the same/different note row after the `onOpenChange(false)` cleanup (`setEditingNote(undefined)`). Since the final `data-note-id` of '' would pass even if the close handler did not clear the selected note (Write Note's `handleOpenEditor()` also clears it), this test does not actually pin down the close-cleanup regression the PR claims to preserve. Add a step that re-clicks a note row after closing and asserts `data-note-id` is the note id again, so a regression in the edit path after cleanup is caught.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| fireEvent.click(screen.getByText(existingNote.title!)) | ||
| expect(editor).toHaveAttribute('data-open', 'true') | ||
| expect(editor).toHaveAttribute('data-note-id', existingNote.id) |
There was a problem hiding this comment.
P3: The edit path is only exercised once. Test 3 clicks a note row (edit mode), closes the editor, then clicks Write Note (create mode) — it never re-opens the same/different note row after the onOpenChange(false) cleanup (setEditingNote(undefined)). Since the final data-note-id of '' would pass even if the close handler did not clear the selected note (Write Note's handleOpenEditor() also clears it), this test does not actually pin down the close-cleanup regression the PR claims to preserve. Add a step that re-clicks a note row after closing and asserts data-note-id is the note id again, so a regression in the edit path after cleanup is caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/app/(dashboard)/notebooks/components/NotesColumn.test.tsx, line 100:
<comment>The edit path is only exercised once. Test 3 clicks a note row (edit mode), closes the editor, then clicks Write Note (create mode) — it never re-opens the same/different note row after the `onOpenChange(false)` cleanup (`setEditingNote(undefined)`). Since the final `data-note-id` of '' would pass even if the close handler did not clear the selected note (Write Note's `handleOpenEditor()` also clears it), this test does not actually pin down the close-cleanup regression the PR claims to preserve. Add a step that re-clicks a note row after closing and asserts `data-note-id` is the note id again, so a regression in the edit path after cleanup is caught.</comment>
<file context>
@@ -0,0 +1,108 @@
+
+ fireEvent.click(screen.getByText(existingNote.title!))
+ expect(editor).toHaveAttribute('data-open', 'true')
+ expect(editor).toHaveAttribute('data-note-id', existingNote.id)
+
+ fireEvent.click(screen.getByText('close editor'))
</file context>
Description
Replace the coupled create/edit open-state calculation in
NotesColumnwith one explicit editor-open state and one optional selected note, following the controlled editor pattern used byTransformationsList. Route both the Write Note button and note-row editing through a small production handler that clears or sets the selected note before opening the existingNoteEditorDialog, and mirroronOpenChangedirectly while clearing selection on close. Add a focusedNotesColumninteraction test that mocks the existing editor boundary and verifies the create path receives the current notebook ID with no selected note, opens on click, closes cleanly, and can be opened again; also preserve the edit path so the state cleanup does not regress row editing.The notebook Notes column renders a Write Note button, but issue #1252 reports that clicking it produces no modal, console error, or network request. The production entry point is
NotesColumn, which currently derives the editor's open state from bothshowAddDialogandeditingNoteand translates the dialog callback back into those states. The same repository already has a simpler working controlled-dialog pattern inTransformationsList, while the Notes column has no interaction test covering its create-note entry point. The note API andNoteEditorDialogcreation behavior are already present, so the fix should stay within the column's dialog wiring.Fixes #1252
Related Issue
Fixes #1252
Type of Change
How Has This Been Tested?
With notes loaded, click Write Note and verify the existing note editor becomes open in create mode with the supplied notebook ID and no note selected.
With an empty notes list, click the same header action and verify create mode opens, proving the behavior is independent of whether a source or note already exists.
Close the create dialog through its
onOpenChange(false)callback, then click Write Note again and verify it reopens without stale edit state.Click an existing note row and verify the editor opens with that note; close it and then open Write Note to verify create mode does not retain the previously selected note.
Design Alignment
Which design principles does this PR support? (See VISION.md)
Explanation:
Checklist
Not applicable to this change.
Code Quality
Testing
make rufforruff check . --fixNot run: no test command resolved in this workspace, so nothing was executed to pass.
make lintoruv run python -m mypy .Not run: no test command resolved in this workspace, so nothing was executed to pass.
onOpenChange(false)callback, then click Write Note again and verify it reopens without stale edit state.Documentation
/docs(if applicable)Not run: no test command resolved in this workspace, so nothing was executed to pass.
Database Changes
/migrations)Not run: no test command resolved in this workspace, so nothing was executed to pass.
Breaking Changes
Screenshots (if applicable)
Additional Context
Nothing beyond what is described above.
Pre-Submission Verification
Before submitting, please verify:
Thank you for contributing to Open Notebook! 🎉
AI was used for assistance.