feat: complete axios notes category refactoring#238
Open
SrujanTag wants to merge 4 commits intoiiitl:mainfrom
Open
feat: complete axios notes category refactoring#238SrujanTag wants to merge 4 commits intoiiitl:mainfrom
SrujanTag wants to merge 4 commits intoiiitl:mainfrom
Conversation
Contributor
|
@SrujanTag is attempting to deploy a commit to the mrimmortal09's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds an "axios" notes category alongside "academic": schema/types, API handlers, forms, listing filters, and UI components now accept and validate category-specific fields (academic: facultyName/year/semester/term; axios: wing/targetAudience/presenterName) with conditional validation and rendering. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant API as Notes API
participant DB as MongoDB
User->>Browser: Open upload/edit form
Browser->>Browser: User selects category (academic/axios)
Browser->>Browser: Render category-specific fields
User->>Browser: Fill fields + attach file
Browser->>Browser: Client-side validation (category-dependent)
Browser->>API: POST /api/notes (form + file)
activate API
API->>API: Parse multipart, check file presence
alt category == academic
API->>API: Validate facultyName, year, semester, term
API->>DB: Create note with academic fields
else category == axios
API->>API: Validate wing against enum, parse presenter/targetAudience
API->>DB: Create note with axios fields
end
DB-->>API: Persisted note
API-->>Browser: 200 OK or 4xx validation error
deactivate API
Browser-->>User: Show success or validation error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
app/notes/edit/[id]/page.tsx (3)
8-8:⚠️ Potential issue | 🟡 MinorRemove unused import
Textarea.Pipeline flagged this as unused.
Proposed fix
-import { Textarea } from '@/components/ui/textarea'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/notes/edit/`[id]/page.tsx at line 8, The import Textarea is unused in the page component; remove the unused symbol from the import line (the import that currently reads "import { Textarea } from '@/components/ui/textarea'") or delete that entire import if nothing else is being imported from that module so the file no longer includes the unused Textarea symbol.
20-20:⚠️ Potential issue | 🟡 MinorRemove unused import
BookOpen.Pipeline flagged this as unused.
Proposed fix
import { Save, FileText, Calendar, - BookOpen, GraduationCap, CheckCircle, AlertCircle, Loader2, } from 'lucide-react'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/notes/edit/`[id]/page.tsx at line 20, Remove the unused import symbol BookOpen from the import list at the top of page.tsx; locate the import statement that includes "BookOpen" (alongside other icons/components) and delete just that identifier so the file no longer imports the unused symbol.
51-61:⚠️ Potential issue | 🟡 MinorAdd missing dependency or stabilize
fetchNoteDetails.Pipeline flagged that
useEffectis missingfetchNoteDetailsin its dependency array. SincefetchNoteDetailsis defined inside the component and referencessessionandunwrappedParams.id, adding it directly would cause infinite re-renders.Wrap
fetchNoteDetailsinuseCallbackwith appropriate dependencies, or move the function inside theuseEffect.Option: Move function inside useEffect
useEffect(() => { if (status === 'unauthenticated') { router.push('/auth/signin?callbackUrl=/notes') return } if (status === 'authenticated') { - fetchNoteDetails() + const fetchNoteDetails = async () => { + try { + setIsLoading(true) + setError(null) + // ... rest of fetchNoteDetails implementation + } catch (err) { + // ... error handling + } finally { + setIsLoading(false) + } + } + fetchNoteDetails() } - }, [status, unwrappedParams.id, router]) + }, [status, unwrappedParams.id, router, session])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/notes/edit/`[id]/page.tsx around lines 51 - 61, The effect depends on a locally declared function fetchNoteDetails but doesn't include it, causing a lint warning; fix by stabilizing fetchNoteDetails: either move the fetchNoteDetails implementation inside the useEffect (so useEffect only depends on status, unwrappedParams.id, router) or wrap fetchNoteDetails in useCallback with the correct dependencies (session and unwrappedParams.id) and then include fetchNoteDetails in the useEffect dependency array; update the useEffect dependency list accordingly to prevent infinite re-renders (useCallback ensures a stable identity) and ensure fetchNoteDetails references are to the memoized function.app/notes/page.tsx (1)
114-117:⚠️ Potential issue | 🟡 MinorFix ESLint warning by wrapping
fetchNotesinuseCallbackor restructuring.The eslint-disable directive is reported as unused, and the exhaustive-deps warning persists. The
fetchNotesfunction should be wrapped inuseCallbackand added to the dependency array, or the fetch logic should be moved inside the effect.Option 1: Move fetch logic inside useEffect
- // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => { - fetchNotes() + const fetchNotes = async () => { + try { + setIsLoading(true) + setError(null) + + const PAGE_SIZE = 100 + let page = 1 + let hasNextPage = true + const collected: TypeNote[] = [] + + while (hasNextPage) { + const response = await fetch( + `/api/notes?limit=${PAGE_SIZE}&page=${page}` + ) + const data = await response.json() + if (!response.ok) + throw new Error(data.message || 'Failed to fetch notes') + + const pageNotes: TypeNote[] = (data.notes.notes as RawNote[]).map( + transformNote + ) + collected.push(...pageNotes) + + hasNextPage = data.notes.hasNextPage ?? false + page += 1 + } + + setAllNotes(collected) + } catch (err) { + setError(err instanceof Error ? err.message : 'Failed to load notes') + } finally { + setIsLoading(false) + } + } + fetchNotes() }, [])Option 2: Use useCallback for fetchNotes
- const fetchNotes = async () => { + const fetchNotes = useCallback(async () => { // ... existing implementation - } + }, []) - // eslint-disable-next-line react-hooks/exhaustive-deps useEffect(() => { fetchNotes() - }, []) + }, [fetchNotes])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/notes/page.tsx` around lines 114 - 117, The ESLint warning comes from calling fetchNotes inside useEffect with an empty deps array and silencing exhaustive-deps; remove the eslint-disable and either (A) move the fetch logic into the effect body (put the code currently in fetchNotes directly inside the useEffect and call it there) or (B) wrap fetchNotes in useCallback (import useCallback, define fetchNotes = useCallback(async () => { ... }, [anyDependencies]) ) and then include fetchNotes in the useEffect dependency array so useEffect(() => { fetchNotes() }, [fetchNotes]); pick one approach and remove the eslint-disable comment.
🧹 Nitpick comments (4)
app/api/notes/route.ts (1)
524-524: Consider validatingtargetAudienceagainst allowed values.
wingis validated againstvalidWings, buttargetAudienceis set without validation. An invalid value would fail at the schema level, but returning an explicit 400 error would be more consistent.Proposed fix
+ if (targetAudience) { + const validAudiences = ['1st Year', '2nd Year', '3rd Year', '4th Year', 'All'] + if (!validAudiences.includes(targetAudience)) { + return NextResponse.json({ message: 'Invalid target audience' }, { status: 400 }) + } + updateData.targetAudience = targetAudience + } - if (targetAudience) updateData.targetAudience = targetAudience🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/notes/route.ts` at line 524, The code sets updateData.targetAudience without validating it, unlike wing which checks validWings; add a validation step for targetAudience before assigning to updateData so invalid values return a 400. Specifically, introduce or use a validTargetAudiences array (analogous to validWings) and, in the same block where you check targetAudience (around the if (targetAudience) updateData.targetAudience = targetAudience), validate that targetAudience is in validTargetAudiences and call the same error-handling path used for wing (return a 400 with a clear message) if it is not; only assign to updateData.targetAudience when the value passes validation.components/notes/note-card.tsx (1)
64-64: Consider usingisAxiosconsistently throughout the component.The
isAxiosvariable is defined but only used for the border styling (line 69). Lines 79, 114, and 154 repeatnote.category === 'axios'instead.Proposed refactor for consistency
- {note.category === 'axios' ? ( + {isAxios ? (Apply similarly at lines 114 and 154.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/notes/note-card.tsx` at line 64, The component defines const isAxios = note.category === 'axios' but then re-evaluates note.category === 'axios' in multiple places; replace those repeated checks by using isAxios consistently across the component (e.g., where border styling currently uses isAxios and in the other JSX/logic locations that compare note.category === 'axios') so all conditional rendering/props use the single isAxios variable (update any ternaries, className conditions and event-handler conditionals to reference isAxios).model/note.ts (1)
125-140: The comment mentions axios notes requirewing, but validation is not enforced.The comment on line 126 states "Axios notes require wing", but the pre-validate hook only validates academic note fields. If you want schema-level enforcement of
wingfor axios notes, add the validation here. Otherwise, the requirement is only enforced at the API layer (app/api/notes/route.tsPOST handler).Proposed fix to enforce wing requirement for axios notes
NoteSchema.pre('validate', function (next) { if (this.category === 'academic') { if (!this.year) { this.invalidate('year', 'Year is required for academic notes') } if (!this.semester) { this.invalidate('semester', 'Semester is required for academic notes') } if (!this.term) { this.invalidate('term', 'Exam type is required for academic notes') } + } else if (this.category === 'axios') { + if (!this.wing) { + this.invalidate('wing', 'Wing is required for axios notes') + } } next() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/note.ts` around lines 125 - 140, The pre-validate hook on NoteSchema (NoteSchema.pre('validate', function (next) { ... })) enforces fields for academic notes but does not validate that axios-category notes have a wing; add a conditional branch checking if this.category === 'axios' and call this.invalidate('wing', 'Wing is required for axios notes') when this.wing is missing so the schema-level validation mirrors the API-layer check in app/api/notes/route.ts.app/upload-notes/page.tsx (1)
489-502: Consider extracting wing options to a shared constant.The wing values are hardcoded here and in the backend (
app/api/notes/route.ts). While they match currently, extracting them to a shared constant (e.g., in@/types/note.tsor a constants file) would reduce the risk of drift and simplify maintenance.Example shared constant
// In a shared constants file, e.g., lib/constants.ts export const AXIOS_WINGS = [ 'ML', 'Web3', 'Web', 'FOSS', 'InfoSec', 'Design', 'App', 'CP', ] as const; export type AxiosWing = (typeof AXIOS_WINGS)[number];Then import and use in both frontend and backend.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/upload-notes/page.tsx` around lines 489 - 502, The wing options are duplicated and should be centralized: create a shared constant (e.g., export AXIOS_WINGS and its string union type AxiosWing from a shared module such as "@/types/note" or "lib/constants") and replace the inline array in upload-notes/page.tsx (the map that renders <SelectItem> items) and the backend array in app/api/notes/route.ts to import and use AXIOS_WINGS and the AxiosWing type; ensure the frontend uses the imported AXIOS_WINGS for mapping and the backend validates/uses the same constant so the two cannot drift.
🤖 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/api/notes/route.ts`:
- Around line 69-80: The route handler currently treats facultyName as required
by pushing it into the missingFields array when empty; update the validation in
app/api/notes/route.ts to match the UI by removing the facultyName check (remove
or skip the line that inspects facultyName and pushes 'facultyName' into
missingFields) so facultyName is optional, ensuring only subject, year,
semester, and term are enforced; keep the rest of the missingFields logic and
error response intact.
In `@app/notes/edit/`[id]/page.tsx:
- Around line 283-476: The edit form is missing an editable Subject field even
though formData.subject is validated and sent in the payload; add a Subject
input above the category-specific conditional so users can edit it. Insert a
labeled Input (id="subject") bound to value={formData.subject} and onChange
calling handleInputChange('subject', e.target.value) (same pattern as
presenterName/facultyName), and ensure its label/placeholder matches other
fields so validation that checks formData.subject will work when submitting.
In `@app/notes/page.tsx`:
- Around line 156-158: The code is casting missing properties (wing,
targetAudience, presenterName) to any when mapping into your note shape; update
the RawNote type to include these optional properties (wing?: string,
targetAudience?: string, presenterName?: string) if the API can return them, or
remove those three fields from the transformation in page.tsx (the mapping that
uses note.wing, note.targetAudience, note.presenterName) so you no longer rely
on non-existent properties; ensure changes touch the RawNote type definition and
the mapping logic that constructs the note object.
---
Outside diff comments:
In `@app/notes/edit/`[id]/page.tsx:
- Line 8: The import Textarea is unused in the page component; remove the unused
symbol from the import line (the import that currently reads "import { Textarea
} from '@/components/ui/textarea'") or delete that entire import if nothing else
is being imported from that module so the file no longer includes the unused
Textarea symbol.
- Line 20: Remove the unused import symbol BookOpen from the import list at the
top of page.tsx; locate the import statement that includes "BookOpen" (alongside
other icons/components) and delete just that identifier so the file no longer
imports the unused symbol.
- Around line 51-61: The effect depends on a locally declared function
fetchNoteDetails but doesn't include it, causing a lint warning; fix by
stabilizing fetchNoteDetails: either move the fetchNoteDetails implementation
inside the useEffect (so useEffect only depends on status, unwrappedParams.id,
router) or wrap fetchNoteDetails in useCallback with the correct dependencies
(session and unwrappedParams.id) and then include fetchNoteDetails in the
useEffect dependency array; update the useEffect dependency list accordingly to
prevent infinite re-renders (useCallback ensures a stable identity) and ensure
fetchNoteDetails references are to the memoized function.
In `@app/notes/page.tsx`:
- Around line 114-117: The ESLint warning comes from calling fetchNotes inside
useEffect with an empty deps array and silencing exhaustive-deps; remove the
eslint-disable and either (A) move the fetch logic into the effect body (put the
code currently in fetchNotes directly inside the useEffect and call it there) or
(B) wrap fetchNotes in useCallback (import useCallback, define fetchNotes =
useCallback(async () => { ... }, [anyDependencies]) ) and then include
fetchNotes in the useEffect dependency array so useEffect(() => { fetchNotes()
}, [fetchNotes]); pick one approach and remove the eslint-disable comment.
---
Nitpick comments:
In `@app/api/notes/route.ts`:
- Line 524: The code sets updateData.targetAudience without validating it,
unlike wing which checks validWings; add a validation step for targetAudience
before assigning to updateData so invalid values return a 400. Specifically,
introduce or use a validTargetAudiences array (analogous to validWings) and, in
the same block where you check targetAudience (around the if (targetAudience)
updateData.targetAudience = targetAudience), validate that targetAudience is in
validTargetAudiences and call the same error-handling path used for wing (return
a 400 with a clear message) if it is not; only assign to
updateData.targetAudience when the value passes validation.
In `@app/upload-notes/page.tsx`:
- Around line 489-502: The wing options are duplicated and should be
centralized: create a shared constant (e.g., export AXIOS_WINGS and its string
union type AxiosWing from a shared module such as "@/types/note" or
"lib/constants") and replace the inline array in upload-notes/page.tsx (the map
that renders <SelectItem> items) and the backend array in app/api/notes/route.ts
to import and use AXIOS_WINGS and the AxiosWing type; ensure the frontend uses
the imported AXIOS_WINGS for mapping and the backend validates/uses the same
constant so the two cannot drift.
In `@components/notes/note-card.tsx`:
- Line 64: The component defines const isAxios = note.category === 'axios' but
then re-evaluates note.category === 'axios' in multiple places; replace those
repeated checks by using isAxios consistently across the component (e.g., where
border styling currently uses isAxios and in the other JSX/logic locations that
compare note.category === 'axios') so all conditional rendering/props use the
single isAxios variable (update any ternaries, className conditions and
event-handler conditionals to reference isAxios).
In `@model/note.ts`:
- Around line 125-140: The pre-validate hook on NoteSchema
(NoteSchema.pre('validate', function (next) { ... })) enforces fields for
academic notes but does not validate that axios-category notes have a wing; add
a conditional branch checking if this.category === 'axios' and call
this.invalidate('wing', 'Wing is required for axios notes') when this.wing is
missing so the schema-level validation mirrors the API-layer check in
app/api/notes/route.ts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 997d4316-efdb-4861-acf1-a47a2d417f15
📒 Files selected for processing (18)
app/api/chat/messages/route.tsapp/api/notes/route.tsapp/layout.jsxapp/notes/edit/[id]/page.tsxapp/notes/page.tsxapp/quick-reads/page.tsxapp/upload-notes/page.tsxapp/upload-papers/page.tsxcomponents/chat/ChatWidget.tsxcomponents/notes/note-card.tsxcomponents/theme-toggler.tsxhooks/useChatMessages.tshooks/useSemesterAutofill.tslib/eventEmitter.tsmiddleware.tsmodel/Message.tsmodel/note.tstypes/note.d.ts
💤 Files with no reviewable changes (5)
- hooks/useChatMessages.ts
- model/Message.ts
- components/chat/ChatWidget.tsx
- app/api/chat/messages/route.ts
- lib/eventEmitter.ts
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.
Resolves #236
Description
Live Demo (if any)
Note for Maintainer
Checkout
Summary by CodeRabbit
New Features
Bug Fixes / Improvements