Cycle between identical products so each is picked once - #37
Conversation
rankProducts ranked products by waste but never broke ties by variety. When an ingredient had several equivalent products (same pack size and shelf life, differing only by store link, e.g. two pizza flavors) and multiple packs were needed, all packs were loaded onto the single top-ranked product, so the same one was recommended repeatedly. Now equivalent products are cycled: their packs are spread one-of-each in a deterministic round-robin (earlier products absorb the remainder). Total cost is unchanged because equivalent products share pack size and therefore waste. Two products are equivalent when every buying and consumption field matches (itemsPerPack, quantityPerItem, unit, both shelf lives, canBeFrozen); only the store link (variant) may differ. Price is not modeled on Product, so it cannot be part of the key. The waste-based sort is now stable (ties keep input order) so both the ranking and the cycle are deterministic. Non-equivalent products keep the existing waste-based ranking untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
guplem
left a comment
There was a problem hiding this comment.
Reviewed by an automated pass. The change is correct and matches the issue's acceptance criteria; one thing the human reviewer should know before merge.
What is done well
- The equivalence key is well chosen and clearly documented: it groups on every field that changes how a product is bought and consumed (pack shape, both shelf lives, can-be-frozen) and excludes the store link on purpose, so two flavors of the same pack size are cycled. Price is not a
Productfield, and the comment says to add it to the key if it ever is. - The distribution is deterministic (stable sort, then round-robin with the remainder to earlier products) and leaves total cost and waste unchanged. Strong test coverage: one-of-each, remainder split (2,1,1), single-unit no-op, link-only variants cycled, and non-equivalent products (different size / different shelf life) kept on waste ranking.
Key point for the reviewer (Suggestion, not blocking)
- The cycle is applied to the recommendation object, which is exactly what the issue's acceptance criteria ask for. But the on-screen "Buy N" count and the copied list do not read
ProductRecommendation.packsNeeded; they recompute packs per product viaproduct.packsNeeded(...)(shopping_ingredient.dart:194-197,shopping_product_row.dart:181). So the cycled distribution is not yet visible to the user. Making it visible means reworking the "or"-alternatives display and the multi-trip copy path (ADR 0014 area), a larger change kept out of this issue's scope. Good candidate for a follow-up issue. - Minor side effect: the owned-field pack auto-fill uses the top recommendation's
packsNeeded(shopping_ingredient.dart:230), which is now the cycled (smaller) share. Low impact, worth a glance.
Verdict: approved pending human review. CI analyze-and-test gate must be green before merge.
rankProducts already spreads equivalent products one-of-each (e.g. one of each pizza flavor), but the shopping card and the copied list ignored that split: both recomputed the full solo pack count per product. Three identical products each showed "3 packs" instead of "1 pack" one-of-each. Now the equivalent split is visible: - Card: equivalent products (same buying traits, different store link) render as a combined "buy one of each" joined by "and"; each shows its cycled share. Non-equivalent products keep the "or" alternatives and their solo counts. - Copy: distributes the packs one-of-each across equivalent products so each shows its share instead of the full count; a zero-share variant is dropped. - Auto-fill: summed the whole equivalence group, so it fills the full amount needed again instead of a single (smaller) cycled share. Equivalence is defined once: productEquivalenceKey and distributeEquivalentPacks are now public in waste_optimizer and used by rankProducts, the card, and the copy. The card and copy distribute from the still-needed amount so the counts reflect owned stock (recommendation.packsNeeded is based on the full desired amount and ignores owned). The copy line builder is extracted to a pure buildIngredientCopyLines function so it can be unit-tested without providers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
An equivalence group (e.g. two pizza flavors of the same size) that needed just 1 pack was skipped by the cycle guard, so every member kept packsNeeded = 1. Auto-fill sums the group, so it filled the group size (2) instead of the true total (1), over-buying. Now a group of 2+ members always distributes its total via distributeEquivalentPacks whenever at least 1 pack is needed: distributeEquivalentPacks(1, 2) = [1, 0]. The shares sum to the true total, so auto-fill is correct with no change to _autoFillOwned. That split can now give a member a 0 share. The card used to still render it as a full row joined by "and", showing "Buy 1 pack and Covered". The copied list already drops 0-share variants; the card now matches: it skips a 0-share member in a combined group and emits no dangling "and" divider. A lone surviving member renders as a normal single row. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
guplem
left a comment
There was a problem hiding this comment.
Re-review after surfacing the cycle in the UI and copy (the earlier "key point for the reviewer"). The cycle is now visible to the user, and a regression found during this pass was fixed on the branch. Ready for a human check.
What changed since the first review
- The one-of-each cycle now shows in both places: the card presents equivalent products as a combined "buy one of each" (joined by "and") using each product's cycled share, and the copied list lists one-of-each instead of the full solo count for every equivalent product. Non-equivalent products keep the "or" alternatives with their solo counts.
- Equivalence is now defined once (
productEquivalenceKey) and reused by the optimizer, the card, and the copy. The round-robin split is a shareddistributeEquivalentPackshelper (verified: sums to the total, remainder to earliest, no negatives).
Found and fixed on this branch
- (was Required) The owned-field auto-fill over-counted when an equivalent group needed only 1 pack total: the cycle skipped the 1-pack case, leaving each member at 1, so auto-fill summed to the group size (2) instead of 1. Fixed by always distributing (1 pack across 2 equivalents -> [1, 0]), so the shares sum to the true total. Also stopped the card rendering a stray "... and Covered" row for a zero-share variant (now matches the copy, which drops it). New tests cover both, driven through
rankProducts. Full suite green (640 tests).
Verified correct: grouping is order-independent, owned stock still lowers grouped counts, non-equivalent products unchanged, distribution math sound.
Known minor (not fixed, immaterial): in multi-trip mode the card distributes the whole-menu total while the copy distributes per trip, so the extra pack can land on a different equivalent variant. Same total; since equivalent products are interchangeable by definition, which one gets the extra does not matter.
Verdict: approved pending human review. CI analyze-and-test gate must be green before merge. Note: overlaps with #42 (mixed-pack display) - reconcile when both land.
Reconcile the equivalent-product cycling of this branch with the shopping work that landed on main (under-buy, copy-matches-page, freeze note). Three files conflicted; all resolved to keep BOTH sides: - waste_optimizer.dart: rankProducts now composes both features. Each product first gets its recommendation including the under-buy reduction (#29, _considerBuyingOnePackLess), then rankProducts cycles equivalent products (#27, _cycleEquivalentProducts) at the end, spreading the resulting group total one-of-each. distributeEquivalentPacks/productEquivalenceKey stay the single source of truth. (auto-merged, verified) - shopping_ingredient.dart: the card keeps all three additions together: the freeze-on-arrival note (#28), the under-buy chip wiring (#29), and the equivalent-group presentation of _buildProductRows (#27). The conflict only tangled the shared closing braces of _separatorDivider and _buildFreezeNote; both methods are now complete. - shopping_page.dart: the copy keeps #34's per-trip split (distributeRemainingAcrossTrips, computeRemainingQuantities) and #27's per-product one-of-each equivalence distribution in buildIngredientCopyLines, applied within each trip's amounts. (auto-merged, verified) Test files (waste_optimizer_test.dart, shopping_ingredient_test.dart): both branches added new test groups at the same spot; git tangled them on shared context lines. Reconstructed so every group is complete and present. No test expectation was changed. flutter analyze: no issues. flutter test: 665 pass. Format check: clean (only generated .g.dart files differ, which the check excludes).
guplem
left a comment
There was a problem hiding this comment.
Rebased onto the updated main (which now has #34, #29, #28) and reconciled. CI is green and the PR is mergeable.
Reconciliation (verified)
rankProductsnow composes both features in sequence: #29's per-product under-buy reduction runs first (keeping full-buy waste for fair ranking), then #27's_cycleEquivalentProductsspreads each equivalence group's (already-reduced) total one-of-each. Equivalent products get identical reductions, so cycling the reduced total is correct.distributeEquivalentPacks/productEquivalenceKeystay the single source of truth.- Copy: #34's per-trip split (
distributeRemainingAcrossTrips+computeRemainingQuantities) runs first, thenbuildIngredientCopyLinesdistributes each trip's equivalent products one-of-each. They nest cleanly. - Card keeps the equivalent-group presentation (#27), the freeze note (#28), and the under-buy chip (#29) together.
- No test expectations changed. Full suite passes together: 665 tests.
Known minor (documented, not fixed): copyWithPacksNeeded (used by cycling) does not carry the underBuy/shortfall fields, so an equivalent group that also qualifies for under-buy loses the under-buy chip on its cycled shares. This is a rare intersection, all tests pass, and forcing the flag onto each cycled share could show a confusing per-share shortfall. Flagging for awareness; can be a follow-up if you want the warning on cycled groups.
Verdict: approved pending human review. CI green and mergeable. Reminder: overlaps with #42 (multi-product card/copy presentation) - reconcile the two displays if you merge both.
Bring the per-product owned quantity branch (#40, issue #24) up to date with main after PR #37 (issue #27, "buy one of each" for equivalent products) merged. The only conflict was the product-row section of shopping_ingredient.dart. Both features change how product rows render: - #40 adds a per-product "Owned" input on each row and hides the header owned input when per-product inputs show (usesPerProductOwnedInputs). - #37 groups equivalent products into a combined "buy one of each" block joined by "and", with cycled one-of-each pack shares. Resolved by keeping #37's _buildProductRows grouping and passing each member's owned count and onProductOwnedChanged callback into its ShoppingProductRow, keyed by the product's index in Ingredient.products. Combined equivalent members now each show their own owned input. The header-fallback owned input still shows only when no per-product input renders. Freeze note and under-buy chip preserved. shopping_page.dart, waste_optimizer.dart, and the new copy test merged cleanly: the copy path keeps #37's one-of-each equivalence distribution (buildIngredientCopyLines) and #40's owned subtraction (_ownedStockFor / computeRemainingQuantities feed the remaining amount the copy distributes). Test fix: #37's two auto-fill tests exercised the header owned input's auto-fill button. Under #40, matching-unit products replace that header with per-product inputs, so the button no longer renders in that setup. Reframed both tests to the header-fallback scenario (recipe unit the products do not use) so the auto-fill summing logic is still covered; the summed expectations (3.0 and 1.0) are unchanged. Also added #40's new required params to #37's _pumpProducts helper. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reconcile the mixed-pack combination solver (#42 / issue #26) with the newly merged equivalent-product cycle (#37 / issue #27). Both features change the shopping card and the copied list, so compose them instead of dropping either. waste_optimizer.dart: auto-merged clean. Keeps both #37's cycling machinery (productEquivalenceKey, distributeEquivalentPacks, _cycleEquivalentProducts) and #42's recommendCombination solver. Shared helpers (ProductRecommendation, _NormalizedEvent, _normalizeEvents) stay single-definition. Copy (shopping_page.dart, buildIngredientCopyLines): #42's combination is now the primary output. The waste-minimal pack mix is picked first, its packs are summed per equivalence key, and where a key covers 2+ equivalent variants (same pack size, e.g. two pizza flavors) they are spread one-of-each via distributeEquivalentPacks. Different pack sizes are different keys, so #26's size mix and #27's variety spread never collide. Still passes empty events, so each trip's amount matches the page (#34). Card (shopping_ingredient.dart): keeps both #42's "Best value" banner (_buildCombinationBanner) and #37's equivalent-group rows (_buildProductRows), plus the freeze note (#28) and under-buy chip (#29). The banner shows the whole-menu mix; the rows show per-product solo counts per trip, so their numbers can differ for the same ingredient (a human can later fold them into one display). Tests: kept all of #37's cycle tests and #42's solver tests. Updated one copy expectation: "non-equivalent products each keep their full solo count" no longer holds, because the copy now shows the waste-minimal mix (#26), not every product. Reworked it to assert the mix lists each distinct pack size with its own count. ADR 0018: added a line describing how the copy composes with the #27 cycle. flutter analyze: no issues. flutter test: 698 passed. format check: clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Closes #27
When several products of one ingredient are truly identical, the optimizer used to keep recommending the same one. Now it spreads equivalent products one-of-each, and that spread is shown to the user in both the card and the copied list.
Product, so it cannot be part of the key yet; a code comment records this.rankProductsdistributes the packs one-of-each across equivalent products (e.g. 3 identical products needing 4 packs -> 2,1,1). Total cost and waste are unchanged.productEquivalenceKey), used by the optimizer, the card, and the copy.Note for the reviewer
This "buy some of A and some of B" presentation overlaps with #42 (the mixed-pack combination display). The two are complementary in intent (this one spreads identical products for variety; #42 mixes different pack sizes to cut waste), but they touch the same card and copy code. When both land they should be reconciled into one presentation. Landing order does not matter for correctness; expect a merge conflict in
shopping_ingredient.dart/shopping_page.dart/waste_optimizer.dart.Test plan
flutter analyzepassesflutter test test/passes (640 tests)🤖 Generated with Claude Code