feat: soft delete / trash for notes - #17
Open
ark-commits wants to merge 1 commit into
Open
Conversation
…nt deletion Deleting a note now marks it with a deletedAt timestamp and moves it to a collapsible Trash section in the sidebar. Notes can be restored or permanently deleted from there. On startup, notes deleted more than 30 days ago are automatically purged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 Walkthrough🚀 Automated Code Review Report📊 PR Readiness ScoreScore: 1/5 💀 — Critical Failure
|
| Priority | Category | Description | File |
|---|---|---|---|
| 🔴 P0 | Logic Bug | THIRTY_DAYS_MS constant calculated as 30 * 24 * 60 * 60 (2,592,000 seconds) instead of 30 * 24 * 60 * 60 * 1000 (2,592,000,000 milliseconds). Line 232-234 subtracts this from Date.now() (which returns milliseconds), causing notes to be deleted after only ~30 seconds instead of 30 days. This causes catastrophic unintended data loss on every app startup. |
src/App.jsx:40, 232-234 |
| 🟠 P1 | Logic Bug | Search filter has inconsistent case handling: title search uses normalizedSearchQuery (lowercase) on line 192, but content search uses searchQuery (original case) on line 193. This makes content search case-sensitive while title search is case-insensitive, breaking expected consistent search behavior. |
src/App.jsx:192-193 |
Walkthrough
Notes now support soft-deletion via a deletedAt timestamp. The app separates trashed vs. visible notes, adds trash UI/controls in the sidebar, provides restore and permanent-delete handlers, and removes trashed notes older than 30 days on startup.
Changes
Soft-Deletion & Trash UI
| Layer / File(s) | Summary |
|---|---|
Data Shape src/App.jsx |
Notes gain a deletedAt field (null for active, numeric timestamp for trashed). createDefaultNote / createBlankNote initialize deletedAt: null. normalizeNotes preserves or defaults deletedAt. A THIRTY_DAYS_MS constant added. |
Core Logic src/App.jsx |
App splits notes into trashedNotes (where deletedAt !== null, sorted newest-first) and visibleNotes (deletedAt === null), and adds isTrashOpen state. handleDeleteNote soft-deletes by setting deletedAt = Date.now() and switches active note to the next live note or creates a blank note if none remain. handleRestoreNote sets deletedAt = null. handlePermanentDeleteNote filters the note out of state. Startup useEffect permanently removes notes trashed >30 days. |
UI / Sidebar src/components/NotesSidebar.jsx |
Added formatDeletedAt(timestamp) helper. NotesSidebar now accepts trashedNotes, isTrashOpen, onToggleTrash, onRestoreNote, onPermanentDeleteNote. When expanded, a Trash section shows a badge count, lists trashed notes with strikethrough titles and formatted deletion times, and exposes Restore/Delete Forever actions wired to handlers. |
State Wiring src/App.jsx |
Both desktop and mobile NotesSidebar usages are passed trashedNotes, isTrashOpen, onToggleTrash, onRestoreNote, and onPermanentDeleteNote. |
Sequence Diagram(s)
sequenceDiagram
participant User as User
participant UI as NotesSidebar (UI)
participant App as App State
participant Storage as LocalStorage
rect rgba(135,206,235,0.5)
User->>UI: Click "Delete" on a note
UI->>App: onDelete(noteId)
App->>App: set note.deletedAt = Date.now()
App->>UI: update visibleNotes / trashedNotes
App->>Storage: persist notes (with deletedAt)
end
rect rgba(144,238,144,0.5)
User->>UI: Open Trash, Click "Restore"
UI->>App: onRestore(noteId)
App->>App: set note.deletedAt = null
App->>UI: update lists
App->>Storage: persist notes
end
rect rgba(250,128,114,0.5)
User->>UI: Click "Delete Forever"
UI->>App: onPermanentDelete(noteId)
App->>App: remove note from notes array
App->>UI: update lists
App->>Storage: persist notes
end
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
🐰
In fields of code I hop and stash,
Notes soft-shelved in gentle cache,
Thirty days to mend or mourn—
Restore, keep, or send them gone. ✨
🚥 Pre-merge checks | ✅ 4 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linear Ticket Linked | The PR description contains no reference to any Linear ticket, issue number, or ticket link. | Update the PR description to include a Linear ticket reference in format like 'Closes LIN-XXXXX' to establish traceability. | |
| Documentation Updated | PR description does not address documentation updates or explicitly note if documentation was not applicable for the soft delete/trash feature. | Update PR description to explicitly state whether documentation updates were considered and explain why they were or were not included. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title 'feat: soft delete / trash for notes' directly and clearly summarizes the main change: implementing soft deletion and trash functionality for notes. |
| Description check | ✅ Passed | The description is directly related to the changeset, explaining the soft-delete mechanism, trash section, restore/delete actions, and auto-cleanup after 30 days. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
soft-delete-trash-v2
- 🛠️ Update documentation: Commit on current branch
- ✅ Custom recipe
Update documentationcompleted - (🔄 Check again to run again)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.jsx`:
- Line 40: THIRTY_DAYS_MS is currently calculated in seconds (30 * 24 * 60 * 60)
but the purge logic compares against millisecond epoch timestamps, so update the
constant (THIRTY_DAYS_MS) to be in milliseconds by multiplying by 1000 (e.g., 30
* 24 * 60 * 60 * 1000) so the trash-age comparison between Date.now()/epoch
values and THIRTY_DAYS_MS uses the same unit and correctly represents 30 days.
- Around line 476-493: The current deletion logic picks nextActiveNote as
liveNotes[0], which uses raw notes order; instead, select the next active note
by applying the same visibility/sort the sidebar uses (i.e., use the existing
visibleNotes or replicate its sort/filter) and then choose the first visible
note that isn't the deleted noteId; replace the liveNotes[0] usage in the block
that handles activeNoteIdRef.current === noteId with code that derives
nextActiveNote from visibleNotes.filter(n => n.deletedAt === null && n.id !==
noteId)[0] (or apply the same comparator used by the sidebar if visibleNotes
isn't available), then call setActiveNoteId, update activeNoteIdRef.current and
setDraftContent using that nextActiveNote.
In `@src/components/NotesSidebar.jsx`:
- Around line 274-315: The Trash list currently renders inside the sidebar but
inherits the sidebar's no-overflow behavior, making long trashedNotes
unreachable; update the trash panel (the block rendered when isTrashOpen is true
that maps trashedNotes and uses formatDeletedAt, onRestoreNote, and
onPermanentDeleteNote) so the UL (or its wrapper) becomes an independently
scrollable region by giving it a constrained height (e.g., max-height based on
the sidebar remaining space or a percentage) and overflow-auto (or
overflow-y-auto), ensuring the sidebar parent still lays out correctly (use a
flex column or sibling layout if needed) so Restore/Delete buttons remain
accessible for long lists.
🪄 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 Plus
Run ID: e60443ec-be41-4fbb-8d53-c7e457f31ab2
📒 Files selected for processing (2)
src/App.jsxsrc/components/NotesSidebar.jsx
| | Paragraph | Text | | ||
| ` | ||
|
|
||
| const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 |
There was a problem hiding this comment.
THIRTY_DAYS_MS is off by a factor of 1000.
Line 40 computes seconds, but the purge logic compares it against millisecond epoch timestamps. That means trashed notes are auto-deleted after about 43 minutes instead of 30 days.
Suggested fix
-const THIRTY_DAYS_MS = 30 * 24 * 60 * 60
+const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 | |
| const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.jsx` at line 40, THIRTY_DAYS_MS is currently calculated in seconds
(30 * 24 * 60 * 60) but the purge logic compares against millisecond epoch
timestamps, so update the constant (THIRTY_DAYS_MS) to be in milliseconds by
multiplying by 1000 (e.g., 30 * 24 * 60 * 60 * 1000) so the trash-age comparison
between Date.now()/epoch values and THIRTY_DAYS_MS uses the same unit and
correctly represents 30 days.
Comment on lines
+476
to
+493
| const liveNotes = notes.filter((note) => note.deletedAt === null && note.id !== noteId) | ||
|
|
||
| const nextNotes = previousNotes.filter((note) => note.id !== noteId) | ||
| if (!liveNotes.length) { | ||
| const fallbackNote = createBlankNote() | ||
| setNotes((previousNotes) => | ||
| previousNotes.map((note) => (note.id === noteId ? { ...note, deletedAt: Date.now() } : note)).concat(fallbackNote), | ||
| ) | ||
| setActiveNoteId(fallbackNote.id) | ||
| activeNoteIdRef.current = fallbackNote.id | ||
| setDraftContent(fallbackNote.content) | ||
| return | ||
| } | ||
|
|
||
| if (!nextNotes.length) { | ||
| const fallbackNote = createBlankNote() | ||
| setActiveNoteId(fallbackNote.id) | ||
| activeNoteIdRef.current = fallbackNote.id | ||
| setDraftContent(fallbackNote.content) | ||
| return [fallbackNote] | ||
| } | ||
| if (activeNoteIdRef.current === noteId) { | ||
| const nextActiveNote = liveNotes[0] | ||
| setActiveNoteId(nextActiveNote.id) | ||
| activeNoteIdRef.current = nextActiveNote.id | ||
| setDraftContent(nextActiveNote.content) |
There was a problem hiding this comment.
Pick the next active note from the same order the sidebar renders.
liveNotes[0] comes from raw array order, but the sidebar shows visibleNotes after sorting. Deleting the active note can therefore activate a different note than the first one the user now sees, especially with pinned notes.
Suggested fix
- if (activeNoteIdRef.current === noteId) {
- const nextActiveNote = liveNotes[0]
+ if (activeNoteIdRef.current === noteId) {
+ const nextActiveNote = visibleNotes.find((note) => note.id !== noteId) ?? liveNotes[0]
setActiveNoteId(nextActiveNote.id)
activeNoteIdRef.current = nextActiveNote.id
setDraftContent(nextActiveNote.content)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const liveNotes = notes.filter((note) => note.deletedAt === null && note.id !== noteId) | |
| const nextNotes = previousNotes.filter((note) => note.id !== noteId) | |
| if (!liveNotes.length) { | |
| const fallbackNote = createBlankNote() | |
| setNotes((previousNotes) => | |
| previousNotes.map((note) => (note.id === noteId ? { ...note, deletedAt: Date.now() } : note)).concat(fallbackNote), | |
| ) | |
| setActiveNoteId(fallbackNote.id) | |
| activeNoteIdRef.current = fallbackNote.id | |
| setDraftContent(fallbackNote.content) | |
| return | |
| } | |
| if (!nextNotes.length) { | |
| const fallbackNote = createBlankNote() | |
| setActiveNoteId(fallbackNote.id) | |
| activeNoteIdRef.current = fallbackNote.id | |
| setDraftContent(fallbackNote.content) | |
| return [fallbackNote] | |
| } | |
| if (activeNoteIdRef.current === noteId) { | |
| const nextActiveNote = liveNotes[0] | |
| setActiveNoteId(nextActiveNote.id) | |
| activeNoteIdRef.current = nextActiveNote.id | |
| setDraftContent(nextActiveNote.content) | |
| if (activeNoteIdRef.current === noteId) { | |
| const nextActiveNote = visibleNotes.find((note) => note.id !== noteId) ?? liveNotes[0] | |
| setActiveNoteId(nextActiveNote.id) | |
| activeNoteIdRef.current = nextActiveNote.id | |
| setDraftContent(nextActiveNote.content) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.jsx` around lines 476 - 493, The current deletion logic picks
nextActiveNote as liveNotes[0], which uses raw notes order; instead, select the
next active note by applying the same visibility/sort the sidebar uses (i.e.,
use the existing visibleNotes or replicate its sort/filter) and then choose the
first visible note that isn't the deleted noteId; replace the liveNotes[0] usage
in the block that handles activeNoteIdRef.current === noteId with code that
derives nextActiveNote from visibleNotes.filter(n => n.deletedAt === null &&
n.id !== noteId)[0] (or apply the same comparator used by the sidebar if
visibleNotes isn't available), then call setActiveNoteId, update
activeNoteIdRef.current and setDraftContent using that nextActiveNote.
Comment on lines
+274
to
+315
| {isTrashOpen && ( | ||
| <ul className="space-y-1 border-t border-slate-100 p-2"> | ||
| {trashedNotes.length === 0 && ( | ||
| <li className="rounded-lg border border-dashed border-slate-200 bg-slate-50 p-3 text-sm text-slate-500"> | ||
| Trash is empty. | ||
| </li> | ||
| )} | ||
| {trashedNotes.map((note) => ( | ||
| <li key={note.id}> | ||
| <div className="rounded-lg border border-transparent bg-white px-2 py-2 hover:bg-slate-50"> | ||
| <div className="flex items-center gap-2"> | ||
| <span className="text-base opacity-50" aria-hidden="true"> | ||
| {note.emoji} | ||
| </span> | ||
| <div className="min-w-0 flex-1"> | ||
| <p className="truncate text-sm font-medium text-slate-500 line-through">{note.title}</p> | ||
| <p className="truncate text-xs text-slate-400">{formatDeletedAt(note.deletedAt)}</p> | ||
| </div> | ||
| <div className="flex shrink-0 gap-1"> | ||
| <button | ||
| type="button" | ||
| className="rounded bg-emerald-100 px-1.5 py-1 text-[10px] font-semibold uppercase tracking-wide text-emerald-700 transition hover:bg-emerald-200" | ||
| onClick={() => onRestoreNote(note.id)} | ||
| aria-label={`Restore ${note.title}`} | ||
| > | ||
| Restore | ||
| </button> | ||
| <button | ||
| type="button" | ||
| className="rounded bg-red-100 px-1.5 py-1 text-[10px] font-semibold uppercase tracking-wide text-red-700 transition hover:bg-red-200" | ||
| onClick={() => onPermanentDeleteNote(note.id)} | ||
| aria-label={`Permanently delete ${note.title}`} | ||
| > | ||
| Delete Forever | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| )} |
There was a problem hiding this comment.
Make the trash panel scroll independently.
The sidebar container hides overflow, and the main notes list is the only scroll region. If Trash grows beyond the remaining height, lower trashed notes become unreachable, so users can’t restore or permanently delete them.
Suggested fix
- {isTrashOpen && (
- <ul className="space-y-1 border-t border-slate-100 p-2">
+ {isTrashOpen && (
+ <ul className="max-h-64 space-y-1 overflow-y-auto border-t border-slate-100 p-2">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {isTrashOpen && ( | |
| <ul className="space-y-1 border-t border-slate-100 p-2"> | |
| {trashedNotes.length === 0 && ( | |
| <li className="rounded-lg border border-dashed border-slate-200 bg-slate-50 p-3 text-sm text-slate-500"> | |
| Trash is empty. | |
| </li> | |
| )} | |
| {trashedNotes.map((note) => ( | |
| <li key={note.id}> | |
| <div className="rounded-lg border border-transparent bg-white px-2 py-2 hover:bg-slate-50"> | |
| <div className="flex items-center gap-2"> | |
| <span className="text-base opacity-50" aria-hidden="true"> | |
| {note.emoji} | |
| </span> | |
| <div className="min-w-0 flex-1"> | |
| <p className="truncate text-sm font-medium text-slate-500 line-through">{note.title}</p> | |
| <p className="truncate text-xs text-slate-400">{formatDeletedAt(note.deletedAt)}</p> | |
| </div> | |
| <div className="flex shrink-0 gap-1"> | |
| <button | |
| type="button" | |
| className="rounded bg-emerald-100 px-1.5 py-1 text-[10px] font-semibold uppercase tracking-wide text-emerald-700 transition hover:bg-emerald-200" | |
| onClick={() => onRestoreNote(note.id)} | |
| aria-label={`Restore ${note.title}`} | |
| > | |
| Restore | |
| </button> | |
| <button | |
| type="button" | |
| className="rounded bg-red-100 px-1.5 py-1 text-[10px] font-semibold uppercase tracking-wide text-red-700 transition hover:bg-red-200" | |
| onClick={() => onPermanentDeleteNote(note.id)} | |
| aria-label={`Permanently delete ${note.title}`} | |
| > | |
| Delete Forever | |
| </button> | |
| </div> | |
| </div> | |
| </div> | |
| </li> | |
| ))} | |
| </ul> | |
| )} | |
| {isTrashOpen && ( | |
| <ul className="max-h-64 space-y-1 overflow-y-auto border-t border-slate-100 p-2"> | |
| {trashedNotes.length === 0 && ( | |
| <li className="rounded-lg border border-dashed border-slate-200 bg-slate-50 p-3 text-sm text-slate-500"> | |
| Trash is empty. | |
| </li> | |
| )} | |
| {trashedNotes.map((note) => ( | |
| <li key={note.id}> | |
| <div className="rounded-lg border border-transparent bg-white px-2 py-2 hover:bg-slate-50"> | |
| <div className="flex items-center gap-2"> | |
| <span className="text-base opacity-50" aria-hidden="true"> | |
| {note.emoji} | |
| </span> | |
| <div className="min-w-0 flex-1"> | |
| <p className="truncate text-sm font-medium text-slate-500 line-through">{note.title}</p> | |
| <p className="truncate text-xs text-slate-400">{formatDeletedAt(note.deletedAt)}</p> | |
| </div> | |
| <div className="flex shrink-0 gap-1"> | |
| <button | |
| type="button" | |
| className="rounded bg-emerald-100 px-1.5 py-1 text-[10px] font-semibold uppercase tracking-wide text-emerald-700 transition hover:bg-emerald-200" | |
| onClick={() => onRestoreNote(note.id)} | |
| aria-label={`Restore ${note.title}`} | |
| > | |
| Restore | |
| </button> | |
| <button | |
| type="button" | |
| className="rounded bg-red-100 px-1.5 py-1 text-[10px] font-semibold uppercase tracking-wide text-red-700 transition hover:bg-red-200" | |
| onClick={() => onPermanentDeleteNote(note.id)} | |
| aria-label={`Permanently delete ${note.title}`} | |
| > | |
| Delete Forever | |
| </button> | |
| </div> | |
| </div> | |
| </div> | |
| </li> | |
| ))} | |
| </ul> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/NotesSidebar.jsx` around lines 274 - 315, The Trash list
currently renders inside the sidebar but inherits the sidebar's no-overflow
behavior, making long trashedNotes unreachable; update the trash panel (the
block rendered when isTrashOpen is true that maps trashedNotes and uses
formatDeletedAt, onRestoreNote, and onPermanentDeleteNote) so the UL (or its
wrapper) becomes an independently scrollable region by giving it a constrained
height (e.g., max-height based on the sidebar remaining space or a percentage)
and overflow-auto (or overflow-y-auto), ensuring the sidebar parent still lays
out correctly (use a flex column or sibling layout if needed) so Restore/Delete
buttons remain accessible for long lists.
Owner
Author
|
@coderabbitai in the future PR review if the issue found relates to the UI and is purely cosmetic, mark that as a minor issue. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
|
⏳ Starting custom recipe |
|
✅ Recipe The changes have been created in this pull request: View PR |
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.
Instead of permanently deleting notes, they now move to a Trash section in the sidebar. You can restore them or delete forever from there. Notes that have been in the trash for over 30 days get automatically cleaned up on startup.