Feat: import files from disk#9
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a notes import feature: new authenticated POST /notes/imports route, backend handler and validators to create-and-update notes from uploaded Markdown, frontend file-picker UI and hook to send markdown to the new endpoint, plus tests covering import and validation flows. Changes
Sequence DiagramsequenceDiagram
actor User
participant Options as "Options Component"
participant Hook as "useNotes Hook"
participant API as "POST /notes/imports (Fetch)"
participant Handler as "ImportNotesHandler"
participant Service as "Note Service"
participant DB as "Database"
User->>Options: selects markdown file
Options->>Hook: onImportNotes(file)
Hook->>API: POST {title, content} (credentials included)
API->>Handler: route to ImportNotesHandler
Handler->>Handler: validate method, content-type, path, auth
Handler->>Service: CreateNote(userID, title)
Service->>DB: insert note -> noteID
Handler->>Handler: parse markdown to editor content
Handler->>Service: UpdateNoteById(noteID, content)
Service->>DB: update content
Handler-->>API: respond (202 / error)
Hook->>Hook: refresh notes list
Hook-->>User: update UI (activate imported note)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (2)
frontend/src/hooks/useNotes.ts (1)
108-113: Add lightweight file validation before reading content.A small type/size guard will prevent accidental large/binary imports from freezing the UI path.
Suggested fix
+const MAX_IMPORT_BYTES = 2 * 1024 * 1024; + async function importNotes(file: File, getToken: GetToken): Promise<void> { + const isMarkdown = /\.md$/i.test(file.name) || file.type === "text/markdown"; + if (!isMarkdown) { + throw new Error("only markdown files are supported"); + } + if (file.size > MAX_IMPORT_BYTES) { + throw new Error("file too large to import"); + } + const url = BASEURL + "/notes/imports"; const headers = await authHeaders(getToken); const markdown = await file.text();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useNotes.ts` around lines 108 - 113, The importNotes function should perform a lightweight validation on the incoming File before calling file.text() to avoid large/binary reads; add checks at the top of importNotes (before const markdown = await file.text()) to verify file.size is under a reasonable limit (e.g. 1MB) and that file.type is a text/ type or the filename endsWith .md (case-insensitive); if validation fails, throw or return a clear error so callers can handle it; keep references to importNotes, file.text, parseMarkdownContent, GetToken and BASEURL when updating the function.frontend/src/components/Options.tsx (1)
26-31: Restrict file picker to markdown files.Adding
acceptimproves UX and reduces invalid selections beforeonImportNotesruns.Suggested fix
<input ref={fileInputRef} type="file" + accept=".md,text/markdown" className="hidden" onChange={handleFileChange} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Options.tsx` around lines 26 - 31, The file input in the Options component should restrict selectable files to Markdown to improve UX: update the <input> element (the one using fileInputRef and invoking handleFileChange) to include an accept attribute like ".md,.markdown,text/markdown" so the picker only shows Markdown files; ensure handleFileChange and any callers such as onImportNotes continue to validate file type if needed but the primary change is adding the accept prop to the input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/internal/api/handler/handler.go`:
- Around line 100-111: The current flow calls db.Service.CreateNote before
validating the incoming payload and uses request.UpdateNoteparser which can
panic on parse errors, and then ignores the error returned by
db.Service.UpdateNodeById; change the flow so you first parse/validate the
request payload with request.UpdateNoteparser and return an HTTP error on
parse/validation failure (avoid panics), then call db.Service.CreateNote only
after validation; when calling db.Service.UpdateNodeById check its returned
error and if it fails, delete/rollback the created note (using a delete/remove
method on db.Service or a provided rollback) and return an appropriate HTTP
error instead of success; only write the success response
(response.WriteResponse) after UpdateNodeById succeeds.
In `@backend/internal/api/validator/validator.go`:
- Around line 144-148: The Content-Type check in ImportNotesValidator rejects
valid headers like "application/json; charset=utf-8"; change the strict equality
to parse the header with mime.ParseMediaType(r.Header.Get("Content-Type")) and
compare only the returned mediaType to "application/json", returning the same
http.StatusUnsupportedMediaType on mismatch; mirror the identical fix in
UpdateNoteValidator so both validators accept valid JSON content-type
parameters.
---
Nitpick comments:
In `@frontend/src/components/Options.tsx`:
- Around line 26-31: The file input in the Options component should restrict
selectable files to Markdown to improve UX: update the <input> element (the one
using fileInputRef and invoking handleFileChange) to include an accept attribute
like ".md,.markdown,text/markdown" so the picker only shows Markdown files;
ensure handleFileChange and any callers such as onImportNotes continue to
validate file type if needed but the primary change is adding the accept prop to
the input.
In `@frontend/src/hooks/useNotes.ts`:
- Around line 108-113: The importNotes function should perform a lightweight
validation on the incoming File before calling file.text() to avoid large/binary
reads; add checks at the top of importNotes (before const markdown = await
file.text()) to verify file.size is under a reasonable limit (e.g. 1MB) and that
file.type is a text/ type or the filename endsWith .md (case-insensitive); if
validation fails, throw or return a clear error so callers can handle it; keep
references to importNotes, file.text, parseMarkdownContent, GetToken and BASEURL
when updating the function.
🪄 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: b5dec886-a299-4d67-96a0-db5aab54c4b4
📒 Files selected for processing (8)
backend/cmd/server/main.gobackend/internal/api/handler/handler.gobackend/internal/api/validator/validator.gofrontend/src/Puffnote.tsxfrontend/src/components/Editor.tsxfrontend/src/components/Options.tsxfrontend/src/editor.tsfrontend/src/hooks/useNotes.ts
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)
backend/internal/api/handler/handler_test.go (1)
202-218:⚠️ Potential issue | 🟡 MinorMissing assertion on response status code.
The test verifies mock expectations but does not assert the HTTP status code. Add an assertion to confirm the handler returns the expected status on successful deletion.
🔧 Proposed fix
handler.DelNoteHandler(w, req) + assert.Equal(t, http.StatusOK, w.Code) mockService.AssertExpectations(t) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/api/handler/handler_test.go` around lines 202 - 218, The test TestDelNoteHandler calls Handler.DelNoteHandler and verifies mock expectations but lacks an assertion on the HTTP response status; update TestDelNoteHandler to assert the recorder's status code (e.g., check w.Result().StatusCode or w.Code) equals the expected success code (use http.StatusNoContent or the status your DelNoteHandler returns) after calling handler.DelNoteHandler; keep the MockService.DelNoteById expectation and add this single assertion to validate the handler's HTTP response.
🧹 Nitpick comments (1)
backend/internal/api/validator/validator_test.go (1)
125-153: Tests cover content-type validation but miss method and path validation.The
ImportNotesValidatorimplementation (per context snippet) also validates HTTP method and path format (/notes/imports). Consider adding tests for these scenarios to improve coverage:
- Invalid HTTP method (e.g.,
GETinstead ofPOST)- Invalid path (e.g.,
/notes/invalidor/wrong/imports)📝 Suggested additional tests
func TestImportNotesValidator_InvalidMethod(t *testing.T) { body := strings.NewReader(`{"title":"test","content":{}}`) req := httptest.NewRequest(http.MethodGet, "/notes/imports", body) req.Header.Set("Content-Type", "application/json") err := ImportNotesValidator(req) assert.NotNil(t, err) } func TestImportNotesValidator_InvalidPath(t *testing.T) { body := strings.NewReader(`{"title":"test","content":{}}`) req := httptest.NewRequest(http.MethodPost, "/notes/invalid", body) req.Header.Set("Content-Type", "application/json") err := ImportNotesValidator(req) assert.NotNil(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/internal/api/validator/validator_test.go` around lines 125 - 153, Add unit tests to cover HTTP method and path validation for ImportNotesValidator: create a test that sends a GET request to "/notes/imports" (using httptest.NewRequest with http.MethodGet) and asserts the returned error is non-nil, and another test that sends a POST to an incorrect path such as "/notes/invalid" and asserts a non-nil error; use the same JSON body and Content-Type header setup as existing tests so the failures come from ImportNotesValidator's method/path checks rather than content-type parsing.
🤖 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 `@backend/internal/api/handler/handler_test.go`:
- Around line 202-218: The test TestDelNoteHandler calls Handler.DelNoteHandler
and verifies mock expectations but lacks an assertion on the HTTP response
status; update TestDelNoteHandler to assert the recorder's status code (e.g.,
check w.Result().StatusCode or w.Code) equals the expected success code (use
http.StatusNoContent or the status your DelNoteHandler returns) after calling
handler.DelNoteHandler; keep the MockService.DelNoteById expectation and add
this single assertion to validate the handler's HTTP response.
---
Nitpick comments:
In `@backend/internal/api/validator/validator_test.go`:
- Around line 125-153: Add unit tests to cover HTTP method and path validation
for ImportNotesValidator: create a test that sends a GET request to
"/notes/imports" (using httptest.NewRequest with http.MethodGet) and asserts the
returned error is non-nil, and another test that sends a POST to an incorrect
path such as "/notes/invalid" and asserts a non-nil error; use the same JSON
body and Content-Type header setup as existing tests so the failures come from
ImportNotesValidator's method/path checks rather than content-type parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f85d908-2a3e-4c7c-8ec8-a7485b061a95
📒 Files selected for processing (4)
backend/internal/api/handler/handler.gobackend/internal/api/handler/handler_test.gobackend/internal/api/validator/validator.gobackend/internal/api/validator/validator_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/internal/api/validator/validator.go
- backend/internal/api/handler/handler.go
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
1 participant
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 by CodeRabbit
New Features
Improvements
Bug Fixes
Tests