Skip to content

feat(todo): edit in place for todo titles #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
demo3-labworksdev wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(todo): edit in place for todo titles #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e03bba
feat(todo): add edit in place for todo titles (#30)
demo3-labworksdev Apr 7, 2026
293ca80
fix(todo): handle 404 on edit and show user-friendly message
demo3-labworksdev Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 81 additions & 1 deletion src/templates/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,23 @@
<html>
<head>
<title>Todo App</title>
<style>
.editable-title {
cursor: pointer;
border-bottom: 1px dashed #aaa;
}
.editable-title:hover {
background-color: #f0f0f0;
}
.editable-title input {
font-size: inherit;
font-family: inherit;
border: 1px solid #666;
padding: 1px 4px;
width: 100%;
box-sizing: border-box;
}
</style>
</head>
<body>
<h1>Todo App</h1>
Expand All @@ -24,7 +41,7 @@ <h1>Todo App</h1>
{% for todo in todos %}
<tr>
<td>{{ todo.id }}</td>
<td>{{ todo.title }}</td>
<td class="editable-title" data-todo-id="{{ todo.id }}" title="Click to edit">{{ todo.title }}</td>
<td>{{ "Done" if todo.completed else "Open" }}</td>
<td>
<a href="/toggle/{{ todo.id }}">[toggle]</a>
Expand All @@ -36,5 +53,68 @@ <h1>Todo App</h1>
{% else %}
<p>No todos yet.</p>
{% endif %}

<script>
document.querySelectorAll('.editable-title').forEach(function(cell) {
cell.addEventListener('click', function() {
if (cell.querySelector('input')) return;

var originalText = cell.textContent.trim();
var todoId = cell.dataset.todoId;

var input = document.createElement('input');
input.type = 'text';
input.value = originalText;
cell.textContent = '';
cell.appendChild(input);
input.focus();
input.select();

function save() {
var newTitle = input.value.trim();
if (!newTitle || newTitle === originalText) {
cell.textContent = originalText;
return;
}
input.disabled = true;
var formData = new FormData();
formData.append('title', newTitle);
var responseStatus = null;
fetch('/edit/' + todoId, { method: 'PATCH', body: formData, credentials: 'same-origin' })
.then(function(res) {
responseStatus = res.status;
if (res.ok) return res.json();
throw new Error('Save failed');
})
.then(function(data) {
cell.textContent = data.title;
})
.catch(function() {
if (responseStatus === 404) {
alert('This todo no longer exists — it may have been deleted in another tab. Click OK to remove it from the list.');
cell.closest('tr').remove();
return;
}
cell.style.color = 'red';
input.disabled = false;
input.focus();
setTimeout(function() { cell.style.color = ''; }, 2000);
});
}

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;
}
Comment on lines +105 to +112
Copy link
Copy Markdown

@ai-coding-guardrails ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

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

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:

Suggested change
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:

Suggested change
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:

  1. list_changed_files, {'pattern': {'type': 'string', 'value': '**/*.py'}}
  2. list_changed_files, {'pattern': '**/*.py'}
  3. get_file_lines, {'file_path': 'src/todo_app/app.py', 'start_line': 1, 'end_line': 200}
  4. get_file_lines, {'file_path': 'tests/test_app.py', 'start_line': 1, 'end_line': 200}

});

input.addEventListener('blur', save);
});
});
</script>
</body>
</html>
17 changes: 16 additions & 1 deletion src/todo_app/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import sqlite3
from pathlib import Path

from flask import Flask, redirect, render_template, request, url_for
from flask import Flask, jsonify, redirect, render_template, request, url_for

app = Flask(__name__, template_folder=str(Path(__file__).parent.parent / "templates"))
DATABASE = os.environ.get("DATABASE_PATH", "todos.db")
Expand Down Expand Up @@ -63,6 +63,21 @@ def toggle(todo_id: int) -> str:
return redirect(url_for("index"))


@app.route("/edit/<int:todo_id>", methods=["PATCH"])
def edit(todo_id: int) -> str:
"""Edit a todo's title."""
title = request.form.get("title", "").strip()
if not title:
return jsonify({"error": "Title cannot be empty"}), 400
conn = get_db()
cursor = conn.execute("UPDATE todos SET title = ? WHERE id = ?", (title, todo_id))
conn.commit()
conn.close()
Comment on lines +73 to +75
Copy link
Copy Markdown

@ai-coding-guardrails ai-coding-guardrails bot Apr 7, 2026

Choose a reason for hiding this comment

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

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:

Suggested change
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:

  1. get_file_lines, {'file_path': 'src/todo_app/app.py', 'start_line': '1', 'end_line': '50'}
  2. get_file_lines, {'file_path': 'src/todo_app/app.py', 'start_line': '51', 'end_line': '92'}
  3. list_changed_files, {'pattern': ''}
  4. get_file_lines, {'file_path': 'tests/test_app.py', 'start_line': '1', 'end_line': '100'}

if cursor.rowcount == 0:
return jsonify({"error": "Todo not found"}), 404
return jsonify({"title": title})


@app.route("/delete/<int:todo_id>")
def delete(todo_id: int) -> str:
"""Delete a todo."""
Expand Down
17 changes: 17 additions & 0 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
"""Shared pytest fixtures."""

import pytest

import todo_app.app as app_module
from todo_app.app import app, init_db


@pytest.fixture
def client(tmp_path, monkeypatch):
"""Flask test client with a temporary database."""
monkeypatch.setattr(app_module, "DATABASE", str(tmp_path / "test.db"))
app.config["TESTING"] = True
init_db()

with app.test_client() as test_client:
yield test_client
84 changes: 84 additions & 0 deletions tests/test_app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
"""Tests for the Flask todo application routes."""

import pytest


@pytest.mark.unit
def test_index_empty(client):
"""Test index page with no todos."""
response = client.get("/")
assert response.status_code == 200
assert b"No todos yet." in response.data


@pytest.mark.unit
def test_add_todo(client):
"""Test adding a todo."""
response = client.post("/add", data={"title": "Buy milk"}, follow_redirects=True)
assert response.status_code == 200
assert b"Buy milk" in response.data


@pytest.mark.unit
def test_add_todo_empty_title(client):
"""Test that adding a todo with empty title is ignored."""
response = client.post("/add", data={"title": " "}, follow_redirects=True)
assert response.status_code == 200
assert b"No todos yet." in response.data


@pytest.mark.unit
def test_toggle_todo(client):
"""Test toggling a todo's completion status."""
client.post("/add", data={"title": "Test task"})
response = client.get("/toggle/1", follow_redirects=True)
assert response.status_code == 200
assert b"Done" in response.data


@pytest.mark.unit
def test_delete_todo(client):
"""Test deleting a todo."""
client.post("/add", data={"title": "To delete"})
response = client.get("/delete/1", follow_redirects=True)
assert response.status_code == 200
assert b"No todos yet." in response.data


@pytest.mark.unit
def test_edit_todo(client):
"""Test editing a todo title."""
client.post("/add", data={"title": "Original title"})
response = client.patch("/edit/1", data={"title": "Updated title"})
assert response.status_code == 200
data = response.get_json()
assert data["title"] == "Updated title"


@pytest.mark.unit
def test_edit_todo_empty_title(client):
"""Test that editing with an empty title returns 400."""
client.post("/add", data={"title": "Original title"})
response = client.patch("/edit/1", data={"title": " "})
assert response.status_code == 400
data = response.get_json()
assert "error" in data


@pytest.mark.unit
def test_edit_todo_not_found(client):
"""Test that editing a non-existent todo returns 404."""
response = client.patch("/edit/999", data={"title": "Ghost edit"})
assert response.status_code == 404
data = response.get_json()
assert "error" in data


@pytest.mark.unit
def test_edit_todo_persists(client):
"""Test that editing a todo persists the new title."""
client.post("/add", data={"title": "Old title"})
client.patch("/edit/1", data={"title": "New title"})
response = client.get("/")
assert b"New title" in response.data
assert b"Old title" not in response.data
Loading