feat(todo): edit in place for todo titles#88
Conversation
- Click a todo title to enter edit mode - Save on Enter or blur, cancel on Escape - Visual indicator (dashed underline + pointer cursor + hover highlight) - PATCH /edit/<id> endpoint returns JSON - Disables input during save; shows red error indicator on failure - 8 new unit tests, 100% coverage on app.py Known: authorization not yet implemented across all mutation endpoints (toggle, delete, add, edit) - to be addressed in a future issue. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Return 404 when editing a non-existent todo (rowcount check) - Show alert explaining the todo was deleted if a 404 is returned - Remove the stale row from the page after user acknowledges - Add test for editing a non-existent todo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
I've got 2 comments for you to consider
Reviewed with 🤟 by Zenable
| input.addEventListener('keydown', function(e) { | ||
| if (e.key === 'Enter') { | ||
| input.removeEventListener('blur', save); | ||
| save(); | ||
| } else if (e.key === 'Escape') { | ||
| input.removeEventListener('blur', save); | ||
| cell.textContent = originalText; | ||
| } |
There was a problem hiding this comment.
After pressing Enter or Escape, the blur event listener is removed via removeEventListener, but this only works if the exact same function reference is passed. Here save is a named function defined inside the click handler, so the reference is consistent — however, after pressing Escape, cell.textContent = originalText destroys the input (removing it from the DOM), which fires blur before the removeEventListener call on line 110 can take effect (since blur fires synchronously on DOM removal in some browsers). This means save() can be called on Escape, sending an unwanted PATCH request.
Consider setting a flag or removing the blur listener before modifying the DOM:
| input.addEventListener('keydown', function(e) { | |
| if (e.key === 'Enter') { | |
| input.removeEventListener('blur', save); | |
| save(); | |
| } else if (e.key === 'Escape') { | |
| input.removeEventListener('blur', save); | |
| cell.textContent = originalText; | |
| } | |
| input.addEventListener('keydown', function(e) { | |
| if (e.key === 'Enter') { | |
| input.removeEventListener('blur', save); | |
| save(); | |
| } else if (e.key === 'Escape') { | |
| input.removeEventListener('blur', save); | |
| cell.textContent = originalText; | |
| } | |
| }); |
The fix is to call input.removeEventListener('blur', save) before cell.textContent = originalText on the Escape branch — which is already the order shown, but note that in Firefox and some environments blur fires synchronously when the element is removed. A safer approach is to use a boolean guard:
| input.addEventListener('keydown', function(e) { | |
| if (e.key === 'Enter') { | |
| input.removeEventListener('blur', save); | |
| save(); | |
| } else if (e.key === 'Escape') { | |
| input.removeEventListener('blur', save); | |
| cell.textContent = originalText; | |
| } | |
| var saving = false; | |
| function save() { | |
| if (saving) return; | |
| saving = true; | |
| var newTitle = input.value.trim(); | |
| if (!newTitle || newTitle === originalText) { | |
| cell.textContent = originalText; | |
| return; | |
| } |
Why did I show this?
Category: bug
Comment Quality: high
Based on general best practices
Tools used:
list_changed_files,{'pattern': {'type': 'string', 'value': '**/*.py'}}list_changed_files,{'pattern': '**/*.py'}get_file_lines,{'file_path': 'src/todo_app/app.py', 'start_line': 1, 'end_line': 200}get_file_lines,{'file_path': 'tests/test_app.py', 'start_line': 1, 'end_line': 200}
| cursor = conn.execute("UPDATE todos SET title = ? WHERE id = ?", (title, todo_id)) | ||
| conn.commit() | ||
| conn.close() |
There was a problem hiding this comment.
The database is committed before checking cursor.rowcount. While the UPDATE of a non-existent row is a no-op and won't corrupt data, the commit is unnecessary in that case. More importantly, if the commit somehow raises an exception after the execute, the connection is never closed (no try/finally). This pattern exists in other routes too, but the 404 path here makes the ordering more visible — the 404 response is returned after an already-committed (empty) transaction.
Consider checking cursor.rowcount before committing:
| cursor = conn.execute("UPDATE todos SET title = ? WHERE id = ?", (title, todo_id)) | |
| conn.commit() | |
| conn.close() | |
| cursor = conn.execute("UPDATE todos SET title = ? WHERE id = ?", (title, todo_id)) | |
| if cursor.rowcount == 0: | |
| conn.close() | |
| return jsonify({"error": "Todo not found"}), 404 | |
| conn.commit() | |
| conn.close() |
Why did I show this?
Category: mistake
Comment Quality: high
Based on general best practices
Tools used:
get_file_lines,{'file_path': 'src/todo_app/app.py', 'start_line': '1', 'end_line': '50'}get_file_lines,{'file_path': 'src/todo_app/app.py', 'start_line': '51', 'end_line': '92'}list_changed_files,{'pattern': ''}get_file_lines,{'file_path': 'tests/test_app.py', 'start_line': '1', 'end_line': '100'}
Summary
PATCH /edit/<id>endpoint with JSON responseapp.pyKnown Issues
add,toggle,edit,delete) — to be addressed in a future issueTest Plan
task test— all unit tests passCloses #30
🤖 Generated with Claude Code