Skip to content

Add an edit command to the notes CLI#6

Open
VikLchv wants to merge 7 commits into
mate-academy:mainfrom
VikLchv:review-me
Open

Add an edit command to the notes CLI#6
VikLchv wants to merge 7 commits into
mate-academy:mainfrom
VikLchv:review-me

Conversation

@VikLchv

@VikLchv VikLchv commented Jun 20, 2026

Copy link
Copy Markdown

Adds an edit command so notes can be updated in place: node notes.js edit <id> <text>.

  • New store.edit(id, text) helper in lib/store.js.
  • New edit case in notes.js, and the command is listed in the default help line.

Opened for review as part of Unit 4 Lesson 4.

@VikLchv

VikLchv commented Jun 20, 2026

Copy link
Copy Markdown
Author

Review — edit command

🔴 Critical: store.edit crashes when the id doesn't exist.

const note = data.notes.find((n) => n.id === id);
note.text = text; // note is undefined when no id matches → TypeError

find returns undefined for a missing id, so note.text = text throws TypeError: Cannot set properties of undefined. Reproduced: node notes.js edit 99 "x" crashes with a stack trace (exit 1). remove already guards this case and returns a boolean — edit should too.

🟠 The CLI always reports success. notes.js prints Updated note #${id} unconditionally because edit returns nothing, so editing a missing id would falsely claim success even after the crash is fixed.

🟡 Minor: no validation. A non-numeric id is NaN (never matches → same crash path); an empty text silently blanks the note.

Suggested fix: make edit return whether it found the note and branch the message, mirroring delete:

function edit(id, text) {
  const data = load();
  const note = data.notes.find((n) => n.id === id);
  if (!note) return false;
  note.text = text;
  save(data);
  return true;
}
// notes.js
const ok = store.edit(id, text);
console.log(ok ? `Updated note #${id}` : `No note #${id} found`);

@VikLchv

VikLchv commented Jun 20, 2026

Copy link
Copy Markdown
Author

Verdict: Claude caught the planted bugstore.edit crashes with a TypeError when the note id doesn't exist (unguarded find). Confirmed by reproducing the crash.

@VikLchv

VikLchv commented Jun 20, 2026

Copy link
Copy Markdown
Author

done

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