fix: todos example crashes on startup due to stale DB schema#40
Conversation
The todos example failed to start with "no such column: user_id" when an old todos.db existed. The migration used CREATE TABLE IF NOT EXISTS which doesn't modify existing tables. Fixes: - Add hasOutdatedSchema() migration that detects old schema via PRAGMA table_info and recreates the table - Replace custom JS search clear button with native input[type=search] (framework handles the search event natively) - Replace inline styles with Pico CSS semantic elements: <s> for strikethrough, <nav> for flex layout, hidden attr for visibility, <del> for error messages - Wrap pagination/clearCompleted buttons in forms for reliable action routing - Rewrite README to document actual controller-based API - Add framework documentation section to CLAUDE.md - Strengthen CSS rules in CLAUDE.md to prefer semantic HTML over inline styles Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes the todos/ example’s startup crash when an older todos.db schema exists, and updates the example UI/docs to better align with the current controller-based + Pico CSS conventions.
Changes:
- Add schema detection/migration logic on startup to handle older SQLite schemas missing
user_id. - Refactor
todos.tmplto remove custom JS and prefer semantic Pico-friendly markup; adjust pagination/clear-completed actions to route reliably via forms. - Update E2E assertions and rewrite
todos/README.md; expand/consolidate guidance inCLAUDE.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
todos/todos.tmpl |
Reworks UI markup (search, completed styling, pagination, clear completed) and action routing via forms/buttons. |
todos/todos_test.go |
Updates pagination visibility assertions to match new hidden-based behavior. |
todos/README.md |
Rewrites documentation to reflect the controller-based API and current app behavior. |
todos/db_manager.go |
Adds schema inspection and migration behavior to prevent crashes on stale SQLite DBs. |
CLAUDE.md |
Adds framework doc pointers and strengthens guidance around using framework-native patterns + Pico semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
todos/README.md
Outdated
| go run main.go | ||
| ``` | ||
| ```bash | ||
| cd examples/todos |
There was a problem hiding this comment.
Quick Start uses cd examples/todos, but in this repository the example lives at the top-level todos/ directory (as referenced in the root README). Update the path so the copy/paste instructions work.
| cd examples/todos | |
| cd todos |
| // runMigrations creates the database schema, handling upgrades from older versions. | ||
| func runMigrations(database *sql.DB) error { | ||
| // Check if the todos table exists with an outdated schema (missing user_id column). |
There was a problem hiding this comment.
runMigrations / hasOutdatedSchema take a parameter named database, which shadows the package-level database variable in this file. Renaming the parameter to db (as it was before) would reduce confusion and avoid accidental use of the wrong identifier in future edits.
todos/db_manager.go
Outdated
| // and drop the old schema before recreating. | ||
| if needsRecreate, err := hasOutdatedSchema(database); err != nil { | ||
| return fmt.Errorf("checking schema: %w", err) | ||
| } else if needsRecreate { | ||
| log.Println("Detected outdated todos table (missing user_id column), recreating...") | ||
| if _, err := database.Exec("DROP TABLE IF EXISTS todos"); err != nil { | ||
| return fmt.Errorf("dropping outdated table: %w", err) |
There was a problem hiding this comment.
The migration path for an outdated schema unconditionally drops the todos table. That fixes the startup crash but also irreversibly deletes existing todos when upgrading. Consider a non-destructive migration (e.g., ALTER TABLE to add user_id with a default and backfill), or at minimum make the data-loss behavior explicit (log message + README note) so users aren't surprised.
| // and drop the old schema before recreating. | |
| if needsRecreate, err := hasOutdatedSchema(database); err != nil { | |
| return fmt.Errorf("checking schema: %w", err) | |
| } else if needsRecreate { | |
| log.Println("Detected outdated todos table (missing user_id column), recreating...") | |
| if _, err := database.Exec("DROP TABLE IF EXISTS todos"); err != nil { | |
| return fmt.Errorf("dropping outdated table: %w", err) | |
| // and migrate the old schema before ensuring the current one. | |
| if needsMigration, err := hasOutdatedSchema(database); err != nil { | |
| return fmt.Errorf("checking schema: %w", err) | |
| } else if needsMigration { | |
| log.Println("Detected outdated todos table (missing user_id column), migrating schema by adding user_id column...") | |
| // Non-destructive migration: add the missing user_id column with a default value. | |
| if _, err := database.Exec(`ALTER TABLE todos ADD COLUMN user_id TEXT NOT NULL DEFAULT ''`); err != nil { | |
| return fmt.Errorf("adding user_id column to existing todos table: %w", err) |
CLAUDE.md
Outdated
| - **References:** `https://github.com/livetemplate/livetemplate/docs/references/` — client attributes, server API, action routing | ||
| - **Guides:** `https://github.com/livetemplate/livetemplate/docs/guides/` — progressive complexity, patterns, best practices |
There was a problem hiding this comment.
The referenced framework documentation links don’t look like valid GitHub URLs (GitHub paths generally need /blob/<branch>/... or /tree/<branch>/...). Please update these to working links (or relative links) so contributors can actually navigate to the docs.
| - **References:** `https://github.com/livetemplate/livetemplate/docs/references/` — client attributes, server API, action routing | |
| - **Guides:** `https://github.com/livetemplate/livetemplate/docs/guides/` — progressive complexity, patterns, best practices | |
| - **References:** `https://github.com/livetemplate/livetemplate/tree/main/docs/references/` — client attributes, server API, action routing | |
| - **Guides:** `https://github.com/livetemplate/livetemplate/tree/main/docs/guides/` — progressive complexity, patterns, best practices |
CLAUDE.md
Outdated
| - Use `<ins>` for success messages, `<del>` for error messages (with `style="display:block;text-decoration:none"`) | ||
| - Use `<s>` for strikethrough text (e.g., completed todos), `<del>` for removed/error content | ||
| - Use `<mark>` for highlighted/badge text | ||
| - Use `<progress>` for progress bars | ||
| - Use `<hgroup>` for title + subtitle groupings | ||
| - Use `<fieldset role="group">` for inline input+button groups | ||
| - Use `<blockquote>` for callout/info boxes | ||
| - Do NOT write inline `style` attributes. Use Pico semantic elements instead (e.g., `<s>` not `style="text-decoration:line-through"`, `<nav>` not `style="display:flex"`, `hidden` not `style="display:none"`) |
There was a problem hiding this comment.
CSS guidance is internally inconsistent: it recommends <del> with an inline style="display:block;text-decoration:none", but later says to never write inline style attributes. Either carve out an explicit exception for these <ins>/<del> patterns (used throughout the repo) or rework the guidance to avoid inline styles entirely.
| - Use `<ins>` for success messages, `<del>` for error messages (with `style="display:block;text-decoration:none"`) | |
| - Use `<s>` for strikethrough text (e.g., completed todos), `<del>` for removed/error content | |
| - Use `<mark>` for highlighted/badge text | |
| - Use `<progress>` for progress bars | |
| - Use `<hgroup>` for title + subtitle groupings | |
| - Use `<fieldset role="group">` for inline input+button groups | |
| - Use `<blockquote>` for callout/info boxes | |
| - Do NOT write inline `style` attributes. Use Pico semantic elements instead (e.g., `<s>` not `style="text-decoration:line-through"`, `<nav>` not `style="display:flex"`, `hidden` not `style="display:none"`) | |
| - Use `<ins>` for success messages, `<del>` for error messages, using the standardized inline style `style="display:block;text-decoration:none"` when you need them to render as block-level alerts | |
| - Use `<s>` for strikethrough text (e.g., completed todos), `<del>` for removed/error content | |
| - Use `<mark>` for highlighted/badge text | |
| - Use `<progress>` for progress bars | |
| - Use `<hgroup>` for title + subtitle groupings | |
| - Use `<fieldset role="group">` for inline input+button groups | |
| - Use `<blockquote>` for callout/info boxes | |
| - Do NOT write inline `style` attributes, except for the standardized `<ins>/<del>` pattern described above. Use Pico semantic elements instead (e.g., `<s>` not `style="text-decoration:line-through"`, `<nav>` not `style="display:flex"`, `hidden` not `style="display:none"`) |
| {{ if .lvt.HasError "text" }} | ||
| <del style="display:block;text-decoration:none">{{ .lvt.Error "text" }}</del> | ||
| {{ end }} |
There was a problem hiding this comment.
This still introduces an inline style attribute (<del style=...>), which conflicts with the new CLAUDE.md guidance and the PR description claim of replacing inline styles. If inline styles are now disallowed, convert this to a Pico-semantic approach; otherwise consider updating the guidance/description to reflect the intended exception.
- Fix README Quick Start path (cd todos, not cd examples/todos) - Use non-destructive ALTER TABLE migration instead of DROP/recreate - Rename shadowed database parameter back to db - Fix GitHub doc URLs to use /tree/main/ for navigable links - Resolve inline style inconsistency: carve out explicit exception for the standardized <ins>/<del> block-level pattern in CLAUDE.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
no such column: user_id) when an oldtodos.dbexists — adds schema migration viaPRAGMA table_infoinput[type=search](framework handlessearchevent natively)styleattributes with Pico CSS semantic elements (<s>,<nav>,hidden,<del>)Test plan
go run .starts successfully with stale DB (migration detects and recreates)go run .starts successfully with fresh DBTestWebSocketBasicpassesTestTodosE2Epasses (all 12 subtests including search, sort, pagination)🤖 Generated with Claude Code