Skip to content

feat: migrate shopping list to new format with checked state#318

Merged
dubadub merged 8 commits intomainfrom
feat/shopping-list-new-format
Apr 13, 2026
Merged

feat: migrate shopping list to new format with checked state#318
dubadub merged 8 commits intomainfrom
feat/shopping-list-new-format

Conversation

@dubadub
Copy link
Copy Markdown
Member

@dubadub dubadub commented Apr 10, 2026

Summary

  • Rewrite ShoppingListStore to use the new .shopping-list format (cooklang-rs parser) instead of the old tab-delimited .shopping_list.txt
  • Add .shopping-checked file for server-side persistent ingredient check state, replacing localStorage
  • Auto-migrate from legacy format on first load (old file renamed to .bak)
  • Add API endpoints: POST /check, POST /uncheck, GET /checked, POST /compact
  • Compact checked log automatically on recipe removal to prune stale entries

Depends on: cooklang/cooklang-rs#(the checked file support PR)

Test plan

  • Start server with existing .shopping_list.txt — verify auto-migration creates .shopping-list and renames old file
  • Add/remove recipes via UI — verify .shopping-list file updates correctly
  • Check/uncheck ingredients — verify .shopping-checked file receives + name/- name entries
  • Reload page — verify checked state persists from server (not localStorage)
  • Remove a recipe — verify .shopping-checked is compacted (stale entries removed)
  • Clear list — verify both files are cleaned up

dubadub added 2 commits April 10, 2026 19:59
Replaces the old tab-delimited `.shopping_list.txt` with the new
`.shopping-list` format from proposal 0016 and adds `.shopping-checked`
for persistent ingredient check state:

- Rewrite ShoppingListStore to use cooklang-rs shopping_list parser
- Auto-migrate from legacy format on first load (renames old file to .bak)
- Add check/uncheck/checked/compact API endpoints
- Replace localStorage-based checked state with server-side persistence
- Compact checked log on recipe removal to prune stale entries
- Add per-recipe reference checkboxes so users control which sub-recipes
  get included when adding to the shopping list (all checked by default).
- Add menus as a single plan entry with nested recipes (and their
  sub-refs as grandchildren) instead of one top-level entry per recipe.
- Expose name as a derived, read-only field: recipe_display_name(path)
  is computed on load and on add, so any client-supplied name is no
  longer silently discarded.
- Fix migration bypass: add(), add_menu(), and remove() now call
  migrate_if_needed() before touching .shopping-list, so the legacy
  .shopping_list.txt can't be orphaned by a write before the first load.
- Fix compact() race: acquire an exclusive advisory file lock (fs2) for
  the read-modify-write so concurrent check()/uncheck() appends can't
  be lost. check()/uncheck() also lock while appending.
- Ignore .shopping-list, .shopping-checked, and .shopping_list.txt.bak
  in .gitignore.
- Flag the ../cooklang-rs path dependency with a TODO referencing
  cooklang/cooklang-rs#93 until that PR is released.
- Refresh recipe snapshot tests for the new upstream parser output
  (full unit names, scalable flag).
@cooklang cooklang deleted a comment from claude bot Apr 12, 2026
dubadub added 2 commits April 12, 2026 17:41
The `path = "../cooklang-rs"` dep only works locally. Point at the
upstream branch feat/shopping-list-checked-support until
cooklang/cooklang-rs#93 is merged and released.
`store.remove()` was calling `compact()`, which delegated to
`cooklang::shopping_list::compact_checked(entries, &ShoppingList)`.
That upstream helper only kept entries whose names appeared as
`Ingredient` items in the list — but our on-disk `.shopping-list`
persists recipe *references* only. Every checked entry was therefore
considered stale and dropped, so removing any recipe cleared the
whole checked file.

Pair with the cooklang-rs contract change: `compact_checked` now
accepts the user-visible ingredient names directly. Introduce
`aggregate_current_ingredient_names()` which walks the stored list
and expands each reference via `extract_ingredients` (honoring
`included_references` and per-recipe scale). Both the `remove` and
explicit `compact` endpoints feed those names into `store.compact()`.
Remove failures in aggregation degrade to a warn log so a recipe
removal can't fail on a render-time parse error.

Also indent nested sub-recipes under their parent recipe card so
Pizza Dough visibly nests under Neapolitan Pizza (border-l accent).
@cooklang cooklang deleted a comment from claude bot Apr 12, 2026
@cooklang cooklang deleted a comment from claude bot Apr 12, 2026
dubadub added 2 commits April 12, 2026 20:43
…ndling

Security (high):
- Escape every untrusted interpolation (recipe names, paths, ref names,
  ingredient names, category names) in shopping_list.html. A recipe file
  called `<img src=x onerror=alert(1)>.cook` is a legal filename and
  previously executed script via `innerHTML`.
- Move inline `onclick="removeRecipe('${path}')"` and
  `onchange="toggleItem('${itemId}', ..., '${name}')"` to delegated
  handlers keyed off `data-*` attributes. Script-attribute context no
  longer receives user-controlled data at all, so a single-quote break
  in a path/name can't escape.

Correctness:
- `aggregate_current_ingredient_names` now propagates parse errors via
  `with_context(..)?` instead of `let _ =`. On remove, the handler still
  treats compact as best-effort — aggregation failure warns and skips
  compaction, so a broken recipe can never wipe the checked log.
- `compact()` now stages to `.shopping-checked.tmp` and renames into
  place. A crash between truncate and write previously left the file
  zero-length; rename is atomic on POSIX so the original is preserved
  until the new content is fully durable.

Refactor:
- Extract `to_multiplier(scale)` — was duplicated three times across
  `add`, `add_menu` (outer), and `add_menu` (inner).

Dependency:
- Repoint `cooklang` git dep from the now-merged
  `feat/shopping-list-checked-support` branch to `main` (picks up
  0.18.5-dev with #93 merged plus the typed `ShoppingListError` follow-
  up). To swap to a crates.io version once the next release lands.
0.18.5 landed on crates.io with cooklang/cooklang-rs#93 merged,
including the name-based `compact_checked` API plus the typed
`ShoppingListError` follow-up. Repoint to the stable version and
drop the branch-based git dep — also collapses the duplicate
cooklang copies in the workspace (cooklang-find, cooklang-reports,
cooklang-language-server all resolve 0.18.5 now), addressing the
review concern about multiple cooklang versions compiling.
@cooklang cooklang deleted a comment from claude bot Apr 12, 2026
@cooklang cooklang deleted a comment from claude bot Apr 12, 2026
File-level flock doesn't prevent two Axum tasks in the same process from
racing on `.shopping-checked` — the kernel treats them as one lock owner,
so a concurrent compact could still truncate between an append's open and
write. Replace flock with a `tokio::sync::Mutex<()>` on AppState held by
every check/uncheck/compact handler (including the post-remove compact).

Drops the fs2 dependency and simplifies `compact()` to a plain read +
atomic temp-file rename.

Also refreshes 5 snapshot fixtures to match cooklang 0.18.5 output.
@cooklang cooklang deleted a comment from claude bot Apr 12, 2026
…encoding

- `clear_shopping_list` now acquires `checked_log_lock` so a concurrent
  check/uncheck can't recreate `.shopping-checked` between our remove_file
  and the caller's view of a cleared list.

- `aggregate_current_ingredient_names` now resets `seen` per top-level
  shopping-list entry. The list may legitimately contain the same recipe
  multiple times (e.g. duplicates from legacy format); sharing one `seen`
  across all entries misreported the second occurrence as a circular
  dependency, skipped compaction, and let stale checks accumulate.

- Replaced `encodeURI(path)` in shopping_list.html with a segment-wise
  `encodeRecipePath` helper that uses `encodeURIComponent` per segment.
  `encodeURI` leaves `#`, `?`, `&`, `=` alone (they're valid URI
  delimiters), which breaks recipe filenames that contain them.

- `compact()` now removes the `.shopping-checked.tmp` file on write or
  rename failure so crashes don't leave dangling temp files.
@cooklang cooklang deleted a comment from claude bot Apr 12, 2026
@claude
Copy link
Copy Markdown

claude bot commented Apr 12, 2026

Code Review

Overall this is a solid and well-thought-out migration. The design decisions (append-only checked log, atomic compact via rename, in-process mutex over file locks) are well documented and correct. A few things worth addressing:


Bug: to_multiplier duplicated in migrate_if_needed

shopping_list_store.rs defines to_multiplier(scale: f64) but migrate_if_needed re-implements the same logic inline:

let multiplier = if (scale - 1.0).abs() < f64::EPSILON {
    None
} else {
    Some(scale)
};

This should just be to_multiplier(scale). If the epsilon threshold ever changes, only one place gets updated.


Bug: clearList no longer resets in-memory checkedItems

The old code wrapped clearList to also clear localStorage and the in-memory set. The new code removes that wrapper. While a subsequent loadShoppingList() call will repopulate checkedItems from the (now empty) server response, any checked checkboxes currently visible on screen won't have their visual state reset until the list reloads. If clearList calls loadShoppingList() immediately after clearing (verify this), the issue resolves itself — but it's fragile. It would be safer to explicitly checkedItems.clear() at the top of clearList.


Minor: Menu detection by file extension is brittle

api_items_from_list uses r.path.ends_with(".menu") to distinguish plan entries from recipe entries. This works as long as the .menu extension is the only convention, but since this is also how the format writer determines nesting structure, consider whether a future format version could break this. Not blocking, just worth a comment.


Minor: migrate_if_needed called on every mutation

load(), add(), add_menu(), and remove() each call migrate_if_needed() which does a fs::metadata stat on .shopping_list.txt and .shopping-list on every invocation. For a local tool this is fine, but once migration completes (the legacy file is renamed to .bak), the stat is a no-op forever. A lightweight flag or a check like !self.legacy_path.exists() short-circuits quickly — but this is acceptable as-is.


Good additions worth calling out

  • escHtml + encodeRecipePath: The old template was vulnerable to XSS from recipe filenames containing <, >, ", ', and URL-breaking characters. All dynamic interpolations are now escaped. Nice.
  • Event delegation over inline onclick: onclick="removeRecipe('${path}')" with unescaped paths was a latent script-injection risk. The data-action pattern is the right fix.
  • Atomic compact via rename: Writing to a temp file, fsync-ing, then renaming is exactly right. The crash-safety comment is appreciated.
  • checked_log_lock comment: The explanation of why flock is insufficient for same-process serialization is correct and saves the next reader from rediscovering it.
  • Per-entry seen reset in aggregate_current_ingredient_names: The comment explaining why seen must be reset per top-level entry (duplicate entries from legacy format) is a good catch.

Nit: resolve_recipe_references is single-use

resolve_recipe_references is only called from add_menu_to_shopping_list. Since it's not reused and is short, consider inlining it or at least moving it immediately above its caller so the reader doesn't have to scroll.


Test plan gap

The manual test plan doesn't cover: what happens when .shopping-list is present but the cooklang parser returns an error (e.g. malformed file). load_list would propagate the error up through load() and surface as a 500. Worth verifying the UX for this case.

@dubadub dubadub merged commit 7700032 into main Apr 13, 2026
6 checks passed
@dubadub dubadub deleted the feat/shopping-list-new-format branch April 13, 2026 09:27
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.

1 participant