Skip to content

web: rank-informed surfaces -- post-log compare prompt + top dishes on home - #78

Merged
TSVRN9 merged 3 commits into
mainfrom
feat/rank-surfaces
Aug 19, 2026
Merged

TSVRN9 merged 3 commits into
mainfrom
feat/rank-surfaces

Conversation

@TSVRN9

@TSVRN9 TSVRN9 commented Aug 19, 2026 •

Copy link
Copy Markdown
Owner

Closes #67

Summary

  • shared/src/ranking.ts: distinctLoggedDishes (extracts the distinct umass-menu dishes a user has logged, in first-seen order) and pickPostLogComparisonPair (the "when to offer, which pair" logic) -- reuses applyComparison's dish identity (dishName+hallTid) and RankedDish.comparisonCount, the same signal /rank's own picker prefers under-compared dishes with. Red-first unit coverage in shared/src/ranking.test.ts (9 new tests -- see the correction note below).
  • web/src/routes/halls/[slug]/+page.svelte: after logging a dish, offers a dismissible, non-modal one-tap comparison prompt against the least-compared previously-logged dish, if a valid pair exists. Choosing a winner writes both Elo tracks (RankedDish + RankedFood) via IndexedDbRankingStorage, same as /rank's own choose().
  • web/src/routes/+page.svelte: "Your Top Dishes" module -- top 3 device-local ranked dishes (rankDishes(...).slice(0, 3)), empty state (gated on both directions to avoid an SSR/pre-hydration loading flash) linking to /rank via "Start ranking" (not "Compare dishes" -- avoids colliding with the existing quick-link's accessible name).
  • web/e2e/rank-surfaces.spec.ts: 9 e2e tests covering both surfaces, including a trial-click actionability check proving the comparison prompt never intercepts the header nav or a Log button.

Review fix (blocking): prompt placement ceiling

The comparison prompt originally rendered in normal document flow, directly under the date nav (see "Known simplification" below for why plain flow was chosen over the alternatives tried at the time). On a realistic-length menu that ceiling is real, not theoretical: logging dish #35 on a ~40-dish menu left the prompt ~4800px above the viewport (toBeInViewport() ratio 0) -- offered only in the DOM, not to the user, which breaks #67 AC2 ("offer a prompt"). The original 2-dish e2e fixture couldn't see this; all 8 specs passed vacuously regardless of placement.

Fixed by moving the prompt to a bottom-anchored, viewport-fixed position, stacked with the existing bottom-fixed status toast via a flex-col-reverse wrapper so whichever of them is showing lands at the very bottom and the other stacks above it -- they can both be visible at once right after a second log, and this ordering keeps them from ever overlapping regardless of either one's height (no hardcoded pixel gap). The wrapper is pointer-events-none so it never blocks clicks on the page underneath; each child re-enables pointer-events-auto for its own bounds.

Red-first: rank-surfaces.spec.ts gained a ~40-dish long-menu fixture and two new specs --

  1. logging a dish far down the list asserts the prompt toBeInViewport() (confirmed red against the pre-fix document-flow version: viewport ratio 0);
  2. a { trial: true } actionability check run at the actually-scrolled position (not scroll 0 -- a real click auto-scrolls its target into view first, which is exactly how the original fixed-position attempt passed every spec in this file while intercepting the header nav, caught only by rank.spec.ts:80, a different file), plus a geometry assertion that the prompt's bottom edge never extends past the toast's top edge. That geometry assertion was itself mutation-verified: temporarily forcing the prompt back to fixed bottom-5 left-1/2 (overlapping the toast) reproduced a red failure (Expected: <= 665, Received: 700); reverted immediately after confirming.

rank.spec.ts:47/:80 (the pre-existing interception canaries) and the full suite were re-run after the change -- all green (see Verification below).

The now-inaccurate ponytail: comment describing plain document flow was removed; the new bottom-anchored comment documents a smaller, real residual ceiling instead (narrow-viewport overlap with the last dish row's Log button -- untested, since this repo's Playwright config only runs a Desktop Chrome project).

Data residency

Both surfaces are strictly device-local (IndexedDB via IndexedDbRankingStorage) -- zero server calls, no auth gate, per CLAUDE.md's data residency table. Previously "verified" by a dedicated page.route("**/api/**") guard test in this file; that test has been deleted, not rewritten -- it's the same pattern home-dashboard.spec.ts's own comment documents as retired repo-wide (a fetch(..., { mode: "no-cors" }) call sails past a route-glob guard undetected). home-dashboard.spec.ts's page.on("request") guard already covers /, which both surfaces render on/link from, so this is deletion of a redundant, weaker check, not a coverage loss.

Declared, deliberate gap: chooseCompare in halls/[slug]/+page.svelte does not call syncDiningHallRanks, unlike /rank's own choose(). Per ADR 0001's Consequences clause, new code reacting to a Pairwise Comparison must touch both Elo tracks or justify touching only one -- this does update both tracks, but is a third reactor to the comparison event and doesn't sync. Adding that sync call would put a Supabase network call on the logging surface, beyond what #67 asks for (and its own constraint is zero server calls here). A signed-in user who only ever compares via this prompt won't sync their favorite dining halls until they also visit /rank. Flagging as a follow-up rather than silently fixing or silently omitting.

Minor cleanup taken in the same pass

loggedDishKey in shared/src/ranking.ts now delegates to the module-private dishKey instead of re-literalizing the ${dishName}::${hallTid} format a third time in the same file.

Correction: the "24 new tests" claim in this PR and in #67 was wrong

The original red-first log (posted on #67) reported import-level red -- SyntaxError: does not provide an export named 'distinctLoggedDishes' -- which fails the whole test file at load time, not per-assertion. Counting "65 total after, 41 before the failing import" as "24 new" double-counted the file's 16 pre-existing tests (which also couldn't run while the import failed). The real number of new tests added for distinctLoggedDishes/pickPostLogComparisonPair is 9 (verified directly: grep -c '^test(' shared/src/ranking.test.ts before this PR's other tests were touched shows 25 total in that file, 16 pre-existing + 9 new). Posting this correction on #67 as well.

Verification

  • pnpm --filter @udine/shared test -- 65 passed
  • CI=1 PORT=4316 npx playwright test (full suite) -- 66 passed (up from the 59 reported at the original PR -- the delta is mostly origin/main's web: social activity feed (pings + sightings) #75 social-feed merge, not this branch; this branch's own net e2e change is 8 → 9 in rank-surfaces.spec.ts, +2 new specs -1 deleted residency guard)
  • pnpm --filter web check -- 0 errors, 0 warnings
  • pnpm --filter web build -- succeeded
  • cd mobile && npx tsc --noEmit -- clean (shared/ changed)
  • pnpm --filter mobile test -- 35 passed (shared/ changed)

Full red/green log and per-criterion evidence posted on #67; this review-fix's red/green evidence is in this PR body above and mirrored in a new #67 comment.

Test plan

  • Shared pair-selection logic unit-tested red-first (import-level red: does not provide an export named 'distinctLoggedDishes')
  • Post-log prompt appears/dismisses correctly (e2e, red evidence pasted on web: rank-informed surfaces (post-log compare prompt, top dishes) #67); logging flow specs unweakened (halls-menu.spec.ts unmodified, still green)
  • Post-log prompt is actually reachable on a realistic-length menu, not just offered in the DOM (blocking review finding -- see "Review fix" above; red-first, mutation-verified)
  • Top-dishes module on home with empty state
  • Full suite green, check clean, shared tests green (local CI lanes -- remote CI is disabled per CLAUDE.md)

…n home (#67)

Adds pickPostLogComparisonPair + distinctLoggedDishes to @udine/shared,
reusing applyComparison's dish identity and RankedDish.comparisonCount
(the same signal /rank's own picker uses) instead of a parallel notion of
"pairable". Wires it into halls/[slug]: after logging a dish, a
dismissible, non-blocking comparison prompt offers to compare it against
the least-compared previously-logged dish, if one exists. Adds a "Your Top
Dishes" module to the home dashboard: top 3 device-local ranked dishes,
empty state when nothing's been compared yet, linking to /rank.

Both surfaces are strictly device-local (IndexedDB via
IndexedDbRankingStorage) -- zero server calls, no auth gate, per CLAUDE.md's
data residency table. Deliberately does not call syncDiningHallRanks from
the new prompt (see the comment on chooseCompare) -- declared as a known
follow-up rather than silently added, since that would put a network call
on the logging surface.
TSVRN9 added 2 commits August 19, 2026 14:44
…67)

Blocking review finding: the prompt rendered in document flow directly
under the date nav, so on a realistic-length menu it sat far above the
viewport after logging a dish deep in the list (toBeInViewport ratio 0,
top -4803px on a ~40-dish fixture) -- offered only in the DOM, not to
the user, breaking #67 AC2. The 2-dish e2e fixture couldn't see this;
all 8 specs passed vacuously regardless of placement.

Fixed with a bottom-anchored, viewport-fixed placement, stacked with
the existing bottom-fixed status toast via a flex-col-reverse wrapper
so the two never overlap regardless of either one's height (no
hardcoded pixel gap), and can't intercept each other or the page
underneath (pointer-events-none wrapper, pointer-events-auto children).

rank-surfaces.spec.ts: added a ~40-dish long-menu fixture and two
specs -- (a) toBeInViewport() after logging far down the list (red
against the pre-fix code: viewport ratio 0), (b) a { trial: true }
actionability check run at the actually-scrolled position (not scroll
0) plus a non-overlap geometry assertion, mutation-verified by
temporarily re-overlapping the prompt with the toast and confirming
red. rank.spec.ts:47/:80 (the pre-existing interception canaries) and
the full suite re-run green after the change.

Also, per the same review pass:
- Deleted the page.route("**/api/**") residency guard test in this
  file -- the retired pattern home-dashboard.spec.ts's own comment
  documents (a no-cors cross-origin fetch sails past it); that file's
  page.on("request") guard already covers "/".
- shared/src/ranking.ts: loggedDishKey now delegates to the
  module-private dishKey instead of re-literalizing the
  ${dishName}::${hallTid} format a third time in the same file.
@TSVRN9

TSVRN9 commented Aug 19, 2026

Copy link
Copy Markdown
Owner Author

Addressed the blocking review finding: the post-log comparison prompt rendered in document flow directly under the date nav, so on a realistic-length menu it sat far above the viewport after logging a dish deep in the list (probe confirmed: toBeInViewport() ratio 0, top -4803px, logging dish #35 on a ~40-dish fixture). Offered only in the DOM, not to the user -- breaks #67 AC2. The original 2-dish e2e fixture couldn't catch this; all 8 specs passed vacuously regardless of placement.

Fix (web/src/routes/halls/[slug]/+page.svelte): bottom-anchored, viewport-fixed placement, stacked with the existing bottom-fixed status toast via a flex-col-reverse wrapper -- whichever of the two is showing lands at the very bottom, the other stacks above it, and they can never overlap regardless of either one's height (no hardcoded pixel gap). Wrapper is pointer-events-none; each child re-enables pointer-events-auto for its own bounds, so the stack never blocks clicks on the page underneath.

Red-first (web/e2e/rank-surfaces.spec.ts): added a ~40-dish long-menu fixture (synthetic dish names/nutrition run through the real parseCategoryItems) and two specs:

  1. Logging a dish far down the list asserts toBeInViewport() -- ran this against the pre-fix code first and got the exact red the reviewer's probe describes (viewport ratio 0).
  2. A { trial: true } actionability check run at the actually-scrolled position (not scroll 0 -- a real .click() auto-scrolls its target into view, which is exactly how the earlier fixed top-* attempt passed every spec in this file while intercepting the header nav, caught only by rank.spec.ts:80, a different file), plus a geometry assertion that the prompt's bottom edge never extends past the toast's top edge. Mutation-verified that geometry assertion: temporarily forced the prompt back to fixed bottom-5 left-1/2 (overlapping the toast) and got a real failure (Expected: <= 665, Received: 700); reverted immediately after confirming red.

rank.spec.ts:47/:80 (the pre-existing interception canaries) and the full suite were re-run after the change -- all green.

Also from the same review pass:

  • Deleted the page.route("**/api/**") residency-guard test in rank-surfaces.spec.ts (not rewritten) -- retired pattern, per home-dashboard.spec.ts's own comment (a no-cors cross-origin fetch sails past a route-glob guard); that file's page.on("request") guard already covers /.
  • Corrected the "24 new tests" claim in this PR body and on web: rank-informed surfaces (post-log compare prompt, top dishes) #67 -- the real number is 9; posting a separate correction comment on web: rank-informed surfaces (post-log compare prompt, top dishes) #67.
  • shared/src/ranking.ts: loggedDishKey now delegates to the module-private dishKey instead of re-literalizing the ${dishName}::${hallTid} format a third time in the file. Shared tests re-run (65 passed), mobile tsc --noEmit clean, pnpm --filter mobile test 35 passed.
  • Updated/removed the now-inaccurate ponytail: comment describing plain document flow; the new comment names the smaller residual ceiling (narrow-viewport overlap with the last dish row's Log button -- untested, this repo's Playwright config only runs a Desktop Chrome project).

Merged origin/main in first (#75 social feed, #79 README) -- no conflicts.

Verification (all local, per CLAUDE.md's "CI is local-only"):

  • pnpm --filter @udine/shared test -- 65 passed
  • CI=1 PORT=4316 npx playwright test (full suite) -- 66 passed
  • pnpm --filter web check -- 0 errors, 0 warnings
  • pnpm --filter web build -- succeeded
  • cd mobile && npx tsc --noEmit -- clean
  • pnpm --filter mobile test -- 35 passed

Not merging -- pushed to feat/rank-surfaces for review.

@TSVRN9
TSVRN9 merged commit da2a097 into main Aug 19, 2026
@TSVRN9
TSVRN9 deleted the feat/rank-surfaces branch August 19, 2026 19:12
TSVRN9 added a commit that referenced this pull request Aug 19, 2026
…m post-log compares (#84)

* web: close the compare-prompt cluster (#80, #81, #82)

- #81: elementFromPoint probe in rank-surfaces.spec.ts proves the last
  dish row's Log button, not the pointer-events-none wrapper, receives
  the click at desktop viewport -- the trial-click gap PR #78's review
  found. Replaced onboarding.spec.ts's retired page.route("**/api/**")
  residency guard with the page.on("request") pattern.
- #82: bottom-anchored spacer (sm:hidden, sized to the toast/prompt
  stack's own bind:clientHeight + the wrapper's bottom-5 offset) keeps
  the last dish row's Log button reachable while the prompt is up at
  narrow viewports, without touching desktop geometry or adding a new
  component.
- #80: chooseCompare() now fire-and-forget syncs favorite dining halls
  via syncDiningHallRanks when signed in, mirroring /rank's choose() --
  supersedes #67's stricter zero-server-calls constraint for this
  surface now that #67 is closed. New e2e coverage asserts the signed-in
  sync fires and, as a real residency guard (page.on("request"), not
  route-glob counting), that a signed-out compare makes no
  Supabase/cross-origin request.

All three fixes are red-first: each new/changed spec was run against
the pre-fix code and confirmed failing before the corresponding
implementation change, then confirmed passing after.

* e2e: settle the toast before the narrow-viewport probe (review finding on #84); document the toast-shrink transient
TSVRN9 added a commit that referenced this pull request Aug 19, 2026
…sheet, nutrition label (#91)

Rebuilds halls/[slug] per the v2 canvas (wave 3 of epic #87):

- shared: parseCategoryItems now also captures the data-*-dv attributes
  (additive optional fields on NutritionFacts) — including the
  data-cholesterol_dv underscore quirk the real feed uses. web is
  unaffected: no existing NutritionFacts constructor sets the new fields,
  and no web spec asserts exact nutrition-object shape.
- shared: openFoodFacts.ts gains searchProducts (cgi/search.pl, fields=
  slimmed) for the sheet's "Add something else" row — the issue named
  "shared's existing openFoodFacts client" as SEARCH-capable, but only
  lookupBarcode existed; extracted the nutriments→NutritionFacts mapper
  so both share the same per-serving/100g-fallback logic. Barcode
  scanning stays out of scope (native dep).
- mobile/src/lib/plate.ts: pure in-memory plate math (add/increment,
  stepper set, totals via shared's computeDailyTotals, one LogEntry per
  plate row with servings = that row's count — matches how today.tsx
  already displays "Dish × N" and how rank.tsx dedupes its comparison
  pool by dish key).
- mobile/src/lib/nutritionLabel.ts: FDA-label row order/formatting
  (DV% column, blank vs. no-DV-column distinction).
- mobile/src/components/{NutritionLabel,PlateBar,PlateSheet}.tsx: RN
  Modals (not routed screens — MenuItem doesn't need to survive
  router search-param serialization, and neither needs a back-stack
  entry), each with a render smoke test.
- mobile/src/app/halls/[slug].tsx: dish rows get a +/stepper and an
  info affordance; bottom bar appears once the plate is non-empty and
  opens the expanded sheet; the dish list's bottom padding tracks the
  bar's measured height (occlusion-bug class from PR #78/#84) and
  collapses to 0 once the plate empties, even with a stale measured
  height still in state. _layout.tsx untouched — no new routes.

Closes #91
TSVRN9 added a commit that referenced this pull request Aug 20, 2026
…sion, %DV absent/blank, OFF estimate marker

Addresses all four REQUEST-CHANGES findings from the PR #106 review:

1. mobile/src/lib/hallMenu.test.tsx: red-first coverage for halls/[slug].tsx's
   plate wiring, which previously had zero tests. Individually confirmed each
   of the reviewer's three simultaneous mutations now fails at least one test:
   (a) PlateBar mounted unconditionally, (b) setPlate([]) deleted from
   logPlate, (c) the occlusion padding call replaced with a constant 0.
   Also covers: LOG writes one addEntry per plate row with servings = that
   row's count; the plate is retained (not cleared) on a partial addEntry
   failure; the SectionList's contentContainer paddingBottom tracks the
   bar's measured height.

2. halls/[slug].tsx: the LOG-failure banner is now anchored above the plate
   bar's measured height (same listBottomPadding reuse as the list's own
   padding), instead of sitting in-flow underneath the still-mounted,
   opaque bar -- the same occlusion-bug class as PR #78/#84, this time on
   the failure path the previous round added without a layout fix.

3. shared/src/umassDining.ts: getAttr previously returned "" for both an
   absent -dv attribute and a present-but-blank one, so dv() collapsed both
   to null -- meaning a mutated/misspelled attribute name (e.g. the
   cholesterol_dv underscore -> hyphen) was indistinguishable from a
   legitimately blank one. getAttrRaw now distinguishes absent (undefined)
   from present-but-blank (""), implementing the undefined/null contract
   NutritionFacts's *Dv fields already documented. Existing lookupBarcode
   output is untouched (dv() is only used by the new *Dv fields).

4. shared/src/openFoodFacts.ts's searchProducts docstring claimed a
   "marks servingSize as an estimate" mitigation that no UI actually read.
   Added mobile/src/lib/plate.ts's isEstimatedServing (reads the "per 100g"
   marker) and wired it into PlateSheet's search-result row and plate-item
   row, so a per-100g fallback shows "est. per 100g" instead of silently
   presenting 100g numbers as one serving. Docstring updated to point at
   the actual consumer.

Verification: shared node --test 117/117 (was 116 -- new absent/blank
%DV test), mobile tsc --noEmit clean, mobile jest 20 suites / 111 tests
(was 15/91 -- new hallMenu.test.tsx, isEstimatedServing cases, and the
estimate-marker PlateSheet cases). All four findings' new/changed tests
individually mutation-verified (implementation reverted or a targeted
mutation applied, confirmed red, restored, confirmed green).
TSVRN9 added a commit that referenced this pull request Aug 20, 2026
…sheet, nutrition label (#91)

Rebuilds halls/[slug] per the v2 canvas (wave 3 of epic #87):

- shared: parseCategoryItems now also captures the data-*-dv attributes
  (additive optional fields on NutritionFacts) — including the
  data-cholesterol_dv underscore quirk the real feed uses. web is
  unaffected: no existing NutritionFacts constructor sets the new fields,
  and no web spec asserts exact nutrition-object shape.
- shared: openFoodFacts.ts gains searchProducts (cgi/search.pl, fields=
  slimmed) for the sheet's "Add something else" row — the issue named
  "shared's existing openFoodFacts client" as SEARCH-capable, but only
  lookupBarcode existed; extracted the nutriments→NutritionFacts mapper
  so both share the same per-serving/100g-fallback logic. Barcode
  scanning stays out of scope (native dep).
- mobile/src/lib/plate.ts: pure in-memory plate math (add/increment,
  stepper set, totals via shared's computeDailyTotals, one LogEntry per
  plate row with servings = that row's count — matches how today.tsx
  already displays "Dish × N" and how rank.tsx dedupes its comparison
  pool by dish key).
- mobile/src/lib/nutritionLabel.ts: FDA-label row order/formatting
  (DV% column, blank vs. no-DV-column distinction).
- mobile/src/components/{NutritionLabel,PlateBar,PlateSheet}.tsx: RN
  Modals (not routed screens — MenuItem doesn't need to survive
  router search-param serialization, and neither needs a back-stack
  entry), each with a render smoke test.
- mobile/src/app/halls/[slug].tsx: dish rows get a +/stepper and an
  info affordance; bottom bar appears once the plate is non-empty and
  opens the expanded sheet; the dish list's bottom padding tracks the
  bar's measured height (occlusion-bug class from PR #78/#84) and
  collapses to 0 once the plate empties, even with a stale measured
  height still in state. _layout.tsx untouched — no new routes.

Closes #91
TSVRN9 added a commit that referenced this pull request Aug 20, 2026
…sion, %DV absent/blank, OFF estimate marker

Addresses all four REQUEST-CHANGES findings from the PR #106 review:

1. mobile/src/lib/hallMenu.test.tsx: red-first coverage for halls/[slug].tsx's
   plate wiring, which previously had zero tests. Individually confirmed each
   of the reviewer's three simultaneous mutations now fails at least one test:
   (a) PlateBar mounted unconditionally, (b) setPlate([]) deleted from
   logPlate, (c) the occlusion padding call replaced with a constant 0.
   Also covers: LOG writes one addEntry per plate row with servings = that
   row's count; the plate is retained (not cleared) on a partial addEntry
   failure; the SectionList's contentContainer paddingBottom tracks the
   bar's measured height.

2. halls/[slug].tsx: the LOG-failure banner is now anchored above the plate
   bar's measured height (same listBottomPadding reuse as the list's own
   padding), instead of sitting in-flow underneath the still-mounted,
   opaque bar -- the same occlusion-bug class as PR #78/#84, this time on
   the failure path the previous round added without a layout fix.

3. shared/src/umassDining.ts: getAttr previously returned "" for both an
   absent -dv attribute and a present-but-blank one, so dv() collapsed both
   to null -- meaning a mutated/misspelled attribute name (e.g. the
   cholesterol_dv underscore -> hyphen) was indistinguishable from a
   legitimately blank one. getAttrRaw now distinguishes absent (undefined)
   from present-but-blank (""), implementing the undefined/null contract
   NutritionFacts's *Dv fields already documented. Existing lookupBarcode
   output is untouched (dv() is only used by the new *Dv fields).

4. shared/src/openFoodFacts.ts's searchProducts docstring claimed a
   "marks servingSize as an estimate" mitigation that no UI actually read.
   Added mobile/src/lib/plate.ts's isEstimatedServing (reads the "per 100g"
   marker) and wired it into PlateSheet's search-result row and plate-item
   row, so a per-100g fallback shows "est. per 100g" instead of silently
   presenting 100g numbers as one serving. Docstring updated to point at
   the actual consumer.

Verification: shared node --test 117/117 (was 116 -- new absent/blank
%DV test), mobile tsc --noEmit clean, mobile jest 20 suites / 111 tests
(was 15/91 -- new hallMenu.test.tsx, isEstimatedServing cases, and the
estimate-marker PlateSheet cases). All four findings' new/changed tests
individually mutation-verified (implementation reverted or a targeted
mutation applied, confirmed red, restored, confirmed green).
TSVRN9 added a commit that referenced this pull request Aug 20, 2026
…sheet, nutrition label (#91)

Rebuilds halls/[slug] per the v2 canvas (wave 3 of epic #87):

- shared: parseCategoryItems now also captures the data-*-dv attributes
  (additive optional fields on NutritionFacts) — including the
  data-cholesterol_dv underscore quirk the real feed uses. web is
  unaffected: no existing NutritionFacts constructor sets the new fields,
  and no web spec asserts exact nutrition-object shape.
- shared: openFoodFacts.ts gains searchProducts (cgi/search.pl, fields=
  slimmed) for the sheet's "Add something else" row — the issue named
  "shared's existing openFoodFacts client" as SEARCH-capable, but only
  lookupBarcode existed; extracted the nutriments→NutritionFacts mapper
  so both share the same per-serving/100g-fallback logic. Barcode
  scanning stays out of scope (native dep).
- mobile/src/lib/plate.ts: pure in-memory plate math (add/increment,
  stepper set, totals via shared's computeDailyTotals, one LogEntry per
  plate row with servings = that row's count — matches how today.tsx
  already displays "Dish × N" and how rank.tsx dedupes its comparison
  pool by dish key).
- mobile/src/lib/nutritionLabel.ts: FDA-label row order/formatting
  (DV% column, blank vs. no-DV-column distinction).
- mobile/src/components/{NutritionLabel,PlateBar,PlateSheet}.tsx: RN
  Modals (not routed screens — MenuItem doesn't need to survive
  router search-param serialization, and neither needs a back-stack
  entry), each with a render smoke test.
- mobile/src/app/halls/[slug].tsx: dish rows get a +/stepper and an
  info affordance; bottom bar appears once the plate is non-empty and
  opens the expanded sheet; the dish list's bottom padding tracks the
  bar's measured height (occlusion-bug class from PR #78/#84) and
  collapses to 0 once the plate empties, even with a stale measured
  height still in state. _layout.tsx untouched — no new routes.

Closes #91
TSVRN9 added a commit that referenced this pull request Aug 20, 2026
…sion, %DV absent/blank, OFF estimate marker

Addresses all four REQUEST-CHANGES findings from the PR #106 review:

1. mobile/src/lib/hallMenu.test.tsx: red-first coverage for halls/[slug].tsx's
   plate wiring, which previously had zero tests. Individually confirmed each
   of the reviewer's three simultaneous mutations now fails at least one test:
   (a) PlateBar mounted unconditionally, (b) setPlate([]) deleted from
   logPlate, (c) the occlusion padding call replaced with a constant 0.
   Also covers: LOG writes one addEntry per plate row with servings = that
   row's count; the plate is retained (not cleared) on a partial addEntry
   failure; the SectionList's contentContainer paddingBottom tracks the
   bar's measured height.

2. halls/[slug].tsx: the LOG-failure banner is now anchored above the plate
   bar's measured height (same listBottomPadding reuse as the list's own
   padding), instead of sitting in-flow underneath the still-mounted,
   opaque bar -- the same occlusion-bug class as PR #78/#84, this time on
   the failure path the previous round added without a layout fix.

3. shared/src/umassDining.ts: getAttr previously returned "" for both an
   absent -dv attribute and a present-but-blank one, so dv() collapsed both
   to null -- meaning a mutated/misspelled attribute name (e.g. the
   cholesterol_dv underscore -> hyphen) was indistinguishable from a
   legitimately blank one. getAttrRaw now distinguishes absent (undefined)
   from present-but-blank (""), implementing the undefined/null contract
   NutritionFacts's *Dv fields already documented. Existing lookupBarcode
   output is untouched (dv() is only used by the new *Dv fields).

4. shared/src/openFoodFacts.ts's searchProducts docstring claimed a
   "marks servingSize as an estimate" mitigation that no UI actually read.
   Added mobile/src/lib/plate.ts's isEstimatedServing (reads the "per 100g"
   marker) and wired it into PlateSheet's search-result row and plate-item
   row, so a per-100g fallback shows "est. per 100g" instead of silently
   presenting 100g numbers as one serving. Docstring updated to point at
   the actual consumer.

Verification: shared node --test 117/117 (was 116 -- new absent/blank
%DV test), mobile tsc --noEmit clean, mobile jest 20 suites / 111 tests
(was 15/91 -- new hallMenu.test.tsx, isEstimatedServing cases, and the
estimate-marker PlateSheet cases). All four findings' new/changed tests
individually mutation-verified (implementation reverted or a targeted
mutation applied, confirmed red, restored, confirmed green).
TSVRN9 added a commit that referenced this pull request Aug 20, 2026
… OFF search (#91) (#112)

* mobile: hall menu v2 — plate selection, bottom summary bar, expanded sheet, nutrition label (#91)

Rebuilds halls/[slug] per the v2 canvas (wave 3 of epic #87):

- shared: parseCategoryItems now also captures the data-*-dv attributes
  (additive optional fields on NutritionFacts) — including the
  data-cholesterol_dv underscore quirk the real feed uses. web is
  unaffected: no existing NutritionFacts constructor sets the new fields,
  and no web spec asserts exact nutrition-object shape.
- shared: openFoodFacts.ts gains searchProducts (cgi/search.pl, fields=
  slimmed) for the sheet's "Add something else" row — the issue named
  "shared's existing openFoodFacts client" as SEARCH-capable, but only
  lookupBarcode existed; extracted the nutriments→NutritionFacts mapper
  so both share the same per-serving/100g-fallback logic. Barcode
  scanning stays out of scope (native dep).
- mobile/src/lib/plate.ts: pure in-memory plate math (add/increment,
  stepper set, totals via shared's computeDailyTotals, one LogEntry per
  plate row with servings = that row's count — matches how today.tsx
  already displays "Dish × N" and how rank.tsx dedupes its comparison
  pool by dish key).
- mobile/src/lib/nutritionLabel.ts: FDA-label row order/formatting
  (DV% column, blank vs. no-DV-column distinction).
- mobile/src/components/{NutritionLabel,PlateBar,PlateSheet}.tsx: RN
  Modals (not routed screens — MenuItem doesn't need to survive
  router search-param serialization, and neither needs a back-stack
  entry), each with a render smoke test.
- mobile/src/app/halls/[slug].tsx: dish rows get a +/stepper and an
  info affordance; bottom bar appears once the plate is non-empty and
  opens the expanded sheet; the dish list's bottom padding tracks the
  bar's measured height (occlusion-bug class from PR #78/#84) and
  collapses to 0 once the plate empties, even with a stale measured
  height still in state. _layout.tsx untouched — no new routes.

Closes #91

* mobile: don't clear the plate or lose data on a partial logPlate failure (#91 review pass)

logPlate looped storage.addEntry across the plate's entries with no error
handling; a failure partway through would still clear the plate and
report success. Now catches, leaves the plate/sheet untouched, and
surfaces the failure via the existing banner -- with a ponytail note on
the known retry-duplicates-committed-rows ceiling (toLogEntries mints
fresh ids per call, so a retry doesn't replace what already committed).

* mobile: PR #106 review fixes — hallMenu wiring coverage, banner occlusion, %DV absent/blank, OFF estimate marker

Addresses all four REQUEST-CHANGES findings from the PR #106 review:

1. mobile/src/lib/hallMenu.test.tsx: red-first coverage for halls/[slug].tsx's
   plate wiring, which previously had zero tests. Individually confirmed each
   of the reviewer's three simultaneous mutations now fails at least one test:
   (a) PlateBar mounted unconditionally, (b) setPlate([]) deleted from
   logPlate, (c) the occlusion padding call replaced with a constant 0.
   Also covers: LOG writes one addEntry per plate row with servings = that
   row's count; the plate is retained (not cleared) on a partial addEntry
   failure; the SectionList's contentContainer paddingBottom tracks the
   bar's measured height.

2. halls/[slug].tsx: the LOG-failure banner is now anchored above the plate
   bar's measured height (same listBottomPadding reuse as the list's own
   padding), instead of sitting in-flow underneath the still-mounted,
   opaque bar -- the same occlusion-bug class as PR #78/#84, this time on
   the failure path the previous round added without a layout fix.

3. shared/src/umassDining.ts: getAttr previously returned "" for both an
   absent -dv attribute and a present-but-blank one, so dv() collapsed both
   to null -- meaning a mutated/misspelled attribute name (e.g. the
   cholesterol_dv underscore -> hyphen) was indistinguishable from a
   legitimately blank one. getAttrRaw now distinguishes absent (undefined)
   from present-but-blank (""), implementing the undefined/null contract
   NutritionFacts's *Dv fields already documented. Existing lookupBarcode
   output is untouched (dv() is only used by the new *Dv fields).

4. shared/src/openFoodFacts.ts's searchProducts docstring claimed a
   "marks servingSize as an estimate" mitigation that no UI actually read.
   Added mobile/src/lib/plate.ts's isEstimatedServing (reads the "per 100g"
   marker) and wired it into PlateSheet's search-result row and plate-item
   row, so a per-100g fallback shows "est. per 100g" instead of silently
   presenting 100g numbers as one serving. Docstring updated to point at
   the actual consumer.

Verification: shared node --test 117/117 (was 116 -- new absent/blank
%DV test), mobile tsc --noEmit clean, mobile jest 20 suites / 111 tests
(was 15/91 -- new hallMenu.test.tsx, isEstimatedServing cases, and the
estimate-marker PlateSheet cases). All four findings' new/changed tests
individually mutation-verified (implementation reverted or a targeted
mutation applied, confirmed red, restored, confirmed green).

* mobile: auto-dismiss the logged banner and make its occlusion clearance banner-aware

Device-pass finding on mobile/plate-logging @ e4ee0d5: after LOG succeeds
the plate empties, so listBottomPadding(barHeight, false) = 0 for both the
list padding and the banner anchor, and `logged` was never cleared -- the
success banner permanently covered the last menu row until the plate was
repopulated, and its text was clipped by the gesture-nav edge (no
safe-area inset).

- Auto-dismiss: a useEffect keyed on `logged` clears it via setTimeout
  after a few seconds (4s), cleaning up on unmount/re-trigger. Retires the
  pre-existing "banner never dismisses" nit from the #106 review too.
- Occlusion: the SectionList's bottom padding now adds the banner's own
  measured height (via onLayout, same pattern as the bar) on top of the
  existing bar-aware padding, only while `logged` is set. The banner
  itself now also pads for the bottom safe-area inset (useSafeAreaInsets,
  same as PlateBar) so it isn't clipped by gesture nav when there's no bar
  underneath it to already clear that space.

Both fixes are red-first in hallMenu.test.tsx: a fake-timers test proves
the banner is gone after the timeout (fails without the effect), and two
padding tests prove the list's paddingBottom includes the banner's
measured height alone and stacked on top of the bar's clearance (fails if
the padding sum is reverted to bar-only). All three individually
mutation-verified (mutated the fix away, confirmed red, restored).

Also promoted jest.useFakeTimers()/useRealTimers() to a file-wide
beforeEach/afterEach: logPlate's success and failure paths both now
schedule a real setTimeout, and no test in this file ever unmounts its
renderer, so a real timer landing ~4s after a test finished (well past
teardown) was crashing the whole suite ("window.dispatchEvent is not a
function") instead of just failing one test -- caught by running the full
mobile jest suite, not just this file in isolation.

mobile jest: 20 suites / 114 tests (was 111). mobile tsc --noEmit clean.
shared untouched.
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.

web: rank-informed surfaces (post-log compare prompt, top dishes)

1 participant