Skip to content

Conversation

@YishaiGlasner
Copy link
Contributor

Code Changes

  1. In UserHistoryPanel avoid removing node's _id for being able to delete it (deletion is done by id).
  2. Pass onDeleteNote from UserHistoryPanel via NotesList to NoteListing for triggering setNotes on deletion.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes the delete note functionality on the notes page by ensuring note IDs are preserved through the data flow and properly wired to the deletion handler.

Changes:

  • Modified UserHistoryPanel to include _id when flattening notes data and added onDeleteNote handler to update state after deletion
  • Updated NotesList to receive and pass onDeleteNote callback to individual NoteListing components

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
static/js/UserHistoryPanel.jsx Preserves _id field in flattened notes and implements onDeleteNote handler to filter deleted notes from state
static/js/NoteListing.jsx Passes onDeleteNote prop from NotesList to NoteListing to trigger state updates on deletion

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

notes && notes.length ?
notes.map((item, i) => (
<NoteListing data={item} key={i} />
<NoteListing data={item} key={i} onDeleteNote={() => onDeleteNote(item._id)} />
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using array index as React key is an anti-pattern, especially in a list where items can be deleted. When an item is deleted, the indices shift, which can cause React to incorrectly reuse component instances and lead to bugs or performance issues. Use the note's unique identifier instead.

Suggested change
<NoteListing data={item} key={i} onDeleteNote={() => onDeleteNote(item._id)} />
<NoteListing data={item} key={item._id} onDeleteNote={() => onDeleteNote(item._id)} />

Copilot uses AI. Check for mistakes.
};

const NotesList = ({notes}) => {
const NotesList = ({notes, onDeleteNote}) => {
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NotesList component is missing PropTypes validation. Add PropTypes for both notes and onDeleteNote to ensure proper type checking and documentation of expected props.

Copilot uses AI. Check for mistakes.
</div>
{ menuOpen === 'notes' ?
<NotesList notes={notes} />
<NotesList notes={notes} onDeleteNote={(noteId) => setNotes(notes.filter(n => n._id !== noteId))} />
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onDeleteNote callback will fail if notes is null when a deletion occurs. The filter operation on line 91 doesn't handle the case where notes might be null, which could happen during the loading state. Add a null check before calling filter.

Suggested change
<NotesList notes={notes} onDeleteNote={(noteId) => setNotes(notes.filter(n => n._id !== noteId))} />
<NotesList
notes={notes}
onDeleteNote={(noteId) =>
setNotes(prevNotes => (prevNotes || []).filter(n => n._id !== noteId))
}
/>

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's null, there is nothing to delete.

@mergify
Copy link

mergify bot commented Jan 18, 2026

🧪 CI Insights

Here's what we observed from your CI run for 02ccb25.

🟢 All jobs passed!

But CI Insights is watching 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants