feat(mobile): support custom meal types - #2063
Conversation
Assisted-by: Open WebUI
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe mobile app adds meal type management and API operations. Meal summaries and food-entry flows now use configured meal types and canonical IDs. Custom, hidden, deleted, and historical meal types retain distinct labels and grouping. ChangesMeal identity and grouping
Meal type settings and navigation
Diary summaries and meal details
Canonical IDs in food-entry flows
ID-based meal copying
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FoodSettings
participant MealTypeSettings
participant MealTypeAPI
participant Diary
participant FoodSummary
participant MealTypeDetail
FoodSettings->>MealTypeSettings: Open Meal Types
MealTypeSettings->>MealTypeAPI: Create, update, reorder, or delete meal types
MealTypeAPI-->>MealTypeSettings: Return mutation result
Diary->>FoodSummary: Provide entries and meal type definitions
FoodSummary->>MealTypeDetail: Open detail with canonical meal type ID
MealTypeDetail->>MealTypeDetail: Resolve identity and filter entries
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Validation ResultsChange Detection
✅ All checks passed. Thank you! |
- Add default_time editing (create + edit) with HH:MM validation and null clearing - Replace hardcoded sort_order 99 with numeric sort order field (create + edit) - Match web/backend contract: system types keep visibility/quick-log/default_time (per-user overrides), while name/sort_order stay locked for system types - Add dedicated MealTypeSettingsScreen tests (14 cases) Assisted-by: Open WebUI
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (2)
SparkyFitnessMobile/__tests__/utils/mealNutrition.test.ts (1)
92-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for fallback group metadata.
The test asserts only
mealTypeIdand entry count. Add assertions forisSystem,user_id, andsortOrderon fallback groups.MealSectionderives the label and icon from those fields, so a regression there is not caught today. This also pins the label contract discussed onSparkyFitnessMobile/src/utils/mealNutrition.tslines 180-191.🧪 Suggested assertions
const otherGroup = groups.find((g) => g.mealTypeId === null); expect(otherGroup).toBeDefined(); expect(otherGroup!.entries).toHaveLength(1); + expect(otherGroup!.isSystem).toBe(false); + expect(otherGroup!.sortOrder).toBe(9999); + expect(otherGroup!.name).toBe('completely-unknown');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/utils/mealNutrition.test.ts` around lines 92 - 102, Extend the unmatched-entry test for groupFoodEntriesByMealType to assert the fallback group’s metadata: verify isSystem and user_id match the fallback contract and sortOrder has the expected value. Keep the existing mealTypeId and entry-count assertions, using the fallback fields that MealSection relies on for its label and icon.SparkyFitnessMobile/src/utils/mealNutrition.ts (1)
8-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
MealEntryGroupsexport. RetainMealTypeKey, which is used byRootStackParamList.MealTypeDetail.mealType.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/utils/mealNutrition.ts` around lines 8 - 10, Remove the unused exported MealEntryGroups type from mealNutrition.ts, while retaining MealTypeKey for RootStackParamList.MealTypeDetail.mealType.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 314-320: Update the test “does not open the rename/reorder edit
modal for system meal types” to press the rendered “breakfast” system row before
checking the modal state, then assert that “Edit Meal Type” remains absent.
In `@SparkyFitnessMobile/src/components/CopyMealSheet.tsx`:
- Around line 100-114: Preserve canonical meal type IDs throughout the copy
flow: update handleCopy and the onCopy payload to include source and target meal
type IDs, then extend the corresponding copy API/request and /food-entries/copy
endpoint to use those IDs for selection. Ensure duplicate meal-type names cannot
cause the wrong source or target type to be copied.
In `@SparkyFitnessMobile/src/components/FoodSummary.tsx`:
- Line 137: The meal-type resolution must preserve historical IDs instead of
merging unknown IDs by name. In
SparkyFitnessMobile/src/components/FoodSummary.tsx#L137-L137, update
groupFoodEntriesByMealType to use name matching only when entry.meal_type_id is
absent and create an ID-based historical fallback when the ID is unknown. In
SparkyFitnessMobile/src/screens/EditLoggedMealScreen.tsx#L433-L435, when
meal.meal_type_id exists without a resolved definition, use its literal
historical label rather than getMealTypeDisplayLabelForName. Add coverage for a
deleted custom breakfast alongside an active system breakfast.
- Line 157: Update the key expression in the FoodSummary meal-group rendering to
use a stable key derived from the fallback group name whenever mealTypeId is
absent, while retaining mealTypeId for identified groups. Add a regression test
covering two legacy groups with different names and null mealTypeId values,
verifying they render as distinct sections.
In `@SparkyFitnessMobile/src/screens/FoodEntryViewScreen.tsx`:
- Around line 1136-1139: Update the view-mode label rendering near
getMealTypeDisplayLabelForName to first find the meal type in mealTypes by
entry.meal_type_id and use its display label or literal name; only call
getMealTypeDisplayLabelForName when that ID is unavailable.
In `@SparkyFitnessMobile/src/screens/MealTypeDetailScreen.tsx`:
- Around line 181-194: Update the Add Food action’s navigation parameters so it
forwards mealTypeId only when resolvedType exists; otherwise omit it or pass
undefined, allowing FoodEntryAddScreen to choose a visible default meal type.
Keep the existing date parameter and navigation behavior unchanged.
In `@SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx`:
- Around line 297-315: Add distinct accessibilityLabel props to both Switch
controls in the meal-type row: label the visibility switch as “Visible” plus
mt.name and the quick-log switch as “Quick log” plus mt.name, while preserving
their existing behavior and styling.
- Around line 239-264: Move the DefaultTimeInput component outside
MealTypeSettingsScreen so its identity remains stable across parent renders,
passing saveDefaultTime through props and preserving the existing
synchronization and blur behavior. Update MealTypeSettingsScreen to provide that
callback, and add a regression test confirming text entered before onBlur is
retained when a mutation or query refresh rerenders the parent.
- Around line 387-391: Add an onRequestClose handler to both Modal components in
MealTypeSettingsScreen, using each modal’s corresponding state setter to dismiss
that modal when the Android hardware back button is pressed. Preserve the
existing visibility and backdrop-dismiss behavior.
In `@SparkyFitnessMobile/src/utils/mealNutrition.ts`:
- Around line 180-191: Update fallback-group label resolution in MealSection so
groups marked isSystem: false use getHistoricalMealTypeLabel(group.name) and
preserve the literal historical name, rather than passing user_id: null to
getMealTypeDisplayLabel. Keep normal system-group label resolution unchanged.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/utils/mealNutrition.test.ts`:
- Around line 92-102: Extend the unmatched-entry test for
groupFoodEntriesByMealType to assert the fallback group’s metadata: verify
isSystem and user_id match the fallback contract and sortOrder has the expected
value. Keep the existing mealTypeId and entry-count assertions, using the
fallback fields that MealSection relies on for its label and icon.
In `@SparkyFitnessMobile/src/utils/mealNutrition.ts`:
- Around line 8-10: Remove the unused exported MealEntryGroups type from
mealNutrition.ts, while retaining MealTypeKey for
RootStackParamList.MealTypeDetail.mealType.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 906e7eb1-5c3f-4f2a-8971-8d474187b74d
📒 Files selected for processing (24)
SparkyFitnessMobile/App.tsxSparkyFitnessMobile/__tests__/components/CopyMealSheet.test.tsxSparkyFitnessMobile/__tests__/components/FoodSummary.test.tsxSparkyFitnessMobile/__tests__/screens/FoodEntryAddScreen.test.tsxSparkyFitnessMobile/__tests__/screens/FoodSearchScreen.test.tsxSparkyFitnessMobile/__tests__/screens/MealTypeDetailScreen.test.tsxSparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/utils/mealNutrition.test.tsSparkyFitnessMobile/src/components/CopyMealSheet.tsxSparkyFitnessMobile/src/components/FoodSummary.tsxSparkyFitnessMobile/src/navigation/safeScreens.tsxSparkyFitnessMobile/src/screens/DiaryScreen.tsxSparkyFitnessMobile/src/screens/EditLoggedMealScreen.tsxSparkyFitnessMobile/src/screens/FoodEntryAddScreen.tsxSparkyFitnessMobile/src/screens/FoodEntryViewScreen.tsxSparkyFitnessMobile/src/screens/FoodPhotoLogEntryScreen.tsxSparkyFitnessMobile/src/screens/FoodSearchScreen.tsxSparkyFitnessMobile/src/screens/FoodSettingsScreen.tsxSparkyFitnessMobile/src/screens/MealTypeDetailScreen.tsxSparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsxSparkyFitnessMobile/src/screens/foodForm/CreateFoodMode.tsxSparkyFitnessMobile/src/services/api/mealTypesApi.tsSparkyFitnessMobile/src/types/navigation.tsSparkyFitnessMobile/src/utils/mealNutrition.ts
Correction round for the custom meal types PR: - groupFoodEntriesByMealType is now id-first: an entry whose meal_type_id exists but does not resolve (deleted/hidden custom type) lands in an id-based historical fallback group and is NEVER rematched by name to an active type. Name-based matching runs only for entries without an id. A deleted custom 'breakfast' stays separate from the active system Breakfast. - getMealGroupLabel branches on isSystem, so fallback groups keep their literal snapshotted name (a deleted custom 'breakfast' renders as 'breakfast', never the translated system 'Breakfast'). FoodSummary uses it for section labels. - getFoodEntryMealTypeLabel resolves labels by canonical id first and is used by FoodEntryViewScreen and EditLoggedMealScreen; unknown historical ids preserve the literal label instead of name-rematching. - FoodSummary section keys are collision-resistant: identified groups use meal:<id>, no-id historical groups use historical:<name>. - MealTypeDetailScreen forwards only resolvedType?.id to FoodSearch so a hidden/deleted type is never preselected for a new entry. - CopyMealSheet: the upstream /food-entries/copy endpoint is name-only (verified), so a copy whose selected source or target raw name maps to more than one meal type id is now blocked with a clear error instead of silently copying to/from the wrong type. - DefaultTimeInput moved out of MealTypeSettingsScreen to module scope (stable identity; unblurred text survives parent re-renders) and receives mealType/onSave via props. Both Modal instances gained onRequestClose, and the visibility/quick-log switches carry distinct accessible labels (Visible/Quick log + meal type name). - Tests: system-row press regression, switch a11y, onRequestClose, DefaultTimeInput re-render, deleted-custom vs system-breakfast grouping, historical label literality, distinct no-id fallback groups, id-first entry labels, and ambiguous copy blocking (source and target). Removed the unused MealEntryGroups export. Assisted-by: Open WebUI
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
SparkyFitnessMobile/src/components/FoodSummary.tsx (1)
54-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRestrict calorie targets to system meal groups.
getMealPercentage(group.name, goals)runs for custom and historical groups. A custom group namedbreakfastcan therefore receive the systembreakfasttarget. This breaks the required system/custom separation.Return
0for non-system groups and includegroup.isSystemin the memo dependencies.Proposed fix
const targetCalories = React.useMemo(() => { - if (!goals || !calorieGoal) return 0; + if (!group.isSystem || !goals || !calorieGoal) return 0; const percentage = getMealPercentage(group.name, goals); return Math.round((calorieGoal * percentage) / 100); - }, [goals, calorieGoal, group.name]); + }, [goals, calorieGoal, group.isSystem, group.name]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/components/FoodSummary.tsx` around lines 54 - 64, The calorie target calculation in FoodSummary must return 0 for non-system groups before calling getMealPercentage, preventing custom or historical groups from receiving system targets. Update the React.useMemo dependencies to include group.isSystem while preserving the existing system-group calculation.
🧹 Nitpick comments (1)
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx (1)
351-353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the new
anyescapes.Lines 353, 379, and 388 bypass TypeScript checking. Use the complete
MealTypefixture and let.find()infer the test-instance type.Proposed fix
- .mockResolvedValue({ ...systemMealTypes[0], default_time: '09:15' } as any); + .mockResolvedValue({ ...systemMealTypes[0]!, default_time: '09:15' }); ... - const addModal = UNSAFE_getAllByType(Modal).find((m: any) => m.props.visible === true); + const addModal = UNSAFE_getAllByType(Modal).find((m) => m.props.visible === true); ... - const editModal = UNSAFE_getAllByType(Modal).find((m: any) => m.props.visible === true); + const editModal = UNSAFE_getAllByType(Modal).find((m) => m.props.visible === true);As per coding guidelines, never use
anyin edited TypeScript code.Also applies to: 373-389
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx` around lines 351 - 353, Remove the `as any` casts from the `updateMealType` mock and the related lines in the test. Use a complete `MealType` fixture for `mockResolvedValue`, and let `.find()` infer the test instance type without explicit `any` escapes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@SparkyFitnessMobile/src/components/FoodSummary.tsx`:
- Around line 54-64: The calorie target calculation in FoodSummary must return 0
for non-system groups before calling getMealPercentage, preventing custom or
historical groups from receiving system targets. Update the React.useMemo
dependencies to include group.isSystem while preserving the existing
system-group calculation.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 351-353: Remove the `as any` casts from the `updateMealType` mock
and the related lines in the test. Use a complete `MealType` fixture for
`mockResolvedValue`, and let `.find()` infer the test instance type without
explicit `any` escapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f7e178a-a086-44fd-ad29-a5aa38142af0
📒 Files selected for processing (10)
SparkyFitnessMobile/__tests__/components/CopyMealSheet.test.tsxSparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/utils/mealNutrition.test.tsSparkyFitnessMobile/src/components/CopyMealSheet.tsxSparkyFitnessMobile/src/components/FoodSummary.tsxSparkyFitnessMobile/src/screens/EditLoggedMealScreen.tsxSparkyFitnessMobile/src/screens/FoodEntryViewScreen.tsxSparkyFitnessMobile/src/screens/MealTypeDetailScreen.tsxSparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsxSparkyFitnessMobile/src/utils/mealNutrition.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- SparkyFitnessMobile/src/screens/EditLoggedMealScreen.tsx
- SparkyFitnessMobile/src/screens/MealTypeDetailScreen.tsx
- SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
Maintainer (apedley) UI feedback on the meal type settings screen: - Raw numeric sort_order UI removed entirely (no Order input, no order values in rows). Custom ordering is now drag-and-drop via the drag handle, reusing the existing WorkoutReorderList gesture pattern (Gesture.Pan + computeReorderTargetIndex); a move persists sequential sort_order (100, 110, 120, ... — web convention, above the locked system 10/20/30/40 range) through updateMealType, writing only the types whose value actually changed. Accessible Move up/Move down actions on the handle cover screen readers. System rows are not draggable. - Create now auto-assigns the new custom type to the end of the custom list (max custom sort_order + 10) — no user-visible order number. - HH:MM text input replaced with the existing wheel time picker (react-native-ui-datepicker mode=time, the same pattern FastingEditSheet uses); each row has a compact time cell opening the shared MealTypeTimePickerSheet; Clear returns null. - Duplicate Suggested Meal Times editor removed from FoodSettingsScreen — MealTypeSettings is the single owner of default_time. - Add/Edit modals consolidated into one parameterized MealTypeFormSheet (existing BottomSheetModal pattern from CopyMealSheet) — name, time, visibility, quick-log; no sort_order; system rows cannot edit name. - Iconography: system rows use the canonical MEAL_CONFIG icon; the generic snack icon is only the neutral fallback. Custom rows keep a neutral icon (ownership rule). - Typography: no more text-[10px] (only text-xs and up). 'Quick' label renamed to 'Quick log'. Trash icon uses --color-icon-danger token instead of hardcoded #EF4444. - Long-name rows keep every action (edit/delete/time/reorder) via flex-shrink + numberOfLines; covered by a dedicated layout test. Assisted-by: Open WebUI
- mealNutrition: single MEAL_CONFIG source of truth (removed the parallel SYSTEM_LABELS map and the duplicate getMealTypeIcon switch); getMealTypeSystemLabel is now internal and derives from MEAL_CONFIG. Removed dead exports (filterFoodEntriesByMealType, getFoodEntryMealTypeKey, getMealTypeDisplayLabelForName) and their tests. Removed the redundant MealGroup.user_id — getMealGroupLabel branches on isSystem only. - Blank historical entries (meal_type_id null, meal_type '') normalize to the synthetic Other bucket; the summary shows 'Other' and tapping it lists the actual blank-name entries (compat fix + regression). - targetCalories is now SYSTEM-ONLY: a custom type named breakfast (or a historical group) never inherits the system Breakfast target calories (FoodSummary and MealTypeDetail; functional blocker). - FoodSummary: dropped the redundant visibleGroups filter (groups are only created when they have entries) and extracted one shared empty-state renderer. - CopyMealSheet title resolves the source by canonical meal type ID (active definition wins; unresolved/historical id keeps its literal snapshotted name); never picks the first same-named item. The ambiguous-name copy block is unchanged. - mealTypeId now survives MealTypeDetail → FoodSearch → FoodScan (barcode → FoodEntryAdd, photo → FoodPhotoFlow) → FoodPhotoLogEntry (preselected meal type); photo params carry it through Improve → EstimateReview → LogEntry. Stale hidden/deleted ids are never forwarded (resolvedType guard). Assisted-by: Open WebUI
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx (2)
140-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hardcoded placeholder color with a theme variable.
placeholderTextColor="#9CA3AF"bypasses the theme. The component already reads--color-text-mutedintotextMuted. Use that value so the placeholder follows light and dark themes.♻️ Proposed refactor
- placeholderTextColor="`#9CA3AF`" + placeholderTextColor={textMuted}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx` around lines 140 - 149, Update the TextInput in MealTypeFormSheet to use the existing textMuted theme value for placeholderTextColor instead of the hardcoded "`#9CA3AF`", ensuring the placeholder follows light and dark themes.
108-115: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueStabilize the fallback date passed to
DateTimePicker.When
values.defaultTimeis empty, this expression returns a newDateon every render. Thedateprop identity then changes on each keystroke in the Name field. Memoize the value so the wheel does not re-seed while the user types.♻️ Proposed refactor
- const timeValue = values.defaultTime - ? (() => { - const d = new Date(); - const [h, m] = values.defaultTime.split(':').map(Number); - d.setHours(h, m, 0, 0); - return d; - })() - : new Date(); + const timeValue = useMemo(() => { + const d = new Date(); + if (values.defaultTime) { + const [h, m] = values.defaultTime.split(':').map(Number); + d.setHours(h, m, 0, 0); + } + return d; + }, [values.defaultTime]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx` around lines 108 - 115, Memoize the date value used by the DateTimePicker around the values.defaultTime conversion so an empty defaultTime does not create a new Date on every render. Preserve the existing parsed-time behavior and only stabilize the fallback date identity, using the component’s existing React memoization patterns.SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx (1)
34-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClear
pendingwhen the sheet is dismissed.
presentstores the caller'sonSelectcallback in state. NoonDismisshandler clears it. If the user closes the sheet with the backdrop or a swipe, the stale callback and value stay mounted until the nextpresent. AddonDismiss={() => setPending(null)}on theBottomSheetModalto release the reference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx` around lines 34 - 45, Clear the pending selection when the modal closes by adding an onDismiss handler to the BottomSheetModal that sets pending to null. Keep the existing present and dismiss behavior unchanged.SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx (3)
231-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the comment with the assertion.
The comment states the test asserts the clear affordance. The test only presses the time cell and asserts that no update fires. Update the comment, or extend the test to drive the picker callback and assert the resulting
updateMealTypepayload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx` around lines 231 - 245, The test comment around the `cell` press does not match the assertion. Update it to describe that pressing the time cell opens the picker without triggering `updateMealType`, or extend the test to invoke the picker callback and assert the expected HH:MM payload through `updateMealType`.
154-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the visibility and quick-log fields in the create payload.
The add form collects
isVisibleandshowInQuickLog. This test asserts onlyname,sort_order, anddefault_time, so it does not detect that the create payload drops both switches. See the related comment onSparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsxLines 234-279.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx` around lines 154 - 175, The createMealType payload assertion in the “creates a custom meal type…” test must also verify the form’s isVisible and showInQuickLog values. Set or retain the expected switch states in the rendered form, then include both fields in the expect.objectContaining assertion so dropped visibility or quick-log values are detected.
51-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe sheet mock renders children unconditionally.
BottomSheetModalis replaced by aViewthat always renders its children. The sheet content is therefore present in the tree beforepresentAddorpresentEditruns. Tests that press "Create meal type" or "Save meal type" pass even if the screen never presents the sheet. Consider mockingpresent/dismisswith visibility state so the tests verify the presentation wiring too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx` around lines 51 - 61, The `@gorhom/bottom-sheet` mock in MealTypeSettingsScreen tests should not render BottomSheetModal children by default. Add mock presentation state so present makes the modal content visible and dismiss hides it, allowing create and edit tests to verify presentAdd/presentEdit wiring while preserving the existing BottomSheetScrollView and BottomSheetView stubs.SparkyFitnessMobile/src/components/FoodSummary.tsx (1)
59-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLook up
MEAL_CONFIGonce.The expression calls
group.name.toLowerCase()and indexesMEAL_CONFIGtwice. Read the entry into a local first.♻️ Proposed refactor
- const icon = - group.isSystem && MEAL_CONFIG[group.name.toLowerCase()]?.icon - ? MEAL_CONFIG[group.name.toLowerCase()].icon - : 'meal-snack'; + const systemConfig = group.isSystem + ? MEAL_CONFIG[group.name.toLowerCase()] + : undefined; + const icon = systemConfig?.icon ?? 'meal-snack';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/components/FoodSummary.tsx` around lines 59 - 64, Update the icon selection logic near the `icon` declaration to compute the lowercased group name and retrieve the corresponding `MEAL_CONFIG` entry once in local variables, then reuse that entry for the system-icon check and value. Preserve the existing `'meal-snack'` fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeDetailScreen.test.tsx`:
- Around line 39-58: Update the useScreenHeader mock factory to replace the
explicit any annotations on config and item with a narrow type containing the
optional right header action(s), accessibilityLabel, and onPress fields.
Preserve the existing normalization and Pressable rendering behavior, and do not
introduce any in the edited TypeScript code.
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 177-182: Update the test case around the unused findByText query
to await the loaded meal-type list before pressing “Add meal type” and asserting
the create button’s disabled state. Use the existing findByText result as the
synchronization point so all resulting state updates complete within the test.
In `@SparkyFitnessMobile/src/screens/FoodPhotoImproveScreen.tsx`:
- Line 122: Preserve mealTypeId when recovery navigation replaces FoodScan:
update both replacement-navigation calls around the image-removal and
recoverable-estimate-error flows to include the existing mealTypeId from
route.params in their parameters, keeping subsequent meal logging associated
with the originating meal type.
In `@SparkyFitnessMobile/src/screens/FoodPhotoLogEntryScreen.tsx`:
- Around line 108-115: Update the preselection logic near selectedMealTypeId to
wait until mealTypes is available, use initialMealTypeId only when it matches an
entry in the selectable mealTypes list, and otherwise fall back to
defaultMealTypeId. Preserve the existing self-limiting selection guard and
ensure stale originating IDs cannot reach the submission path.
In `@SparkyFitnessMobile/src/screens/FoodScanScreen.tsx`:
- Line 77: Update both `FoodPhotoIntro` navigation calls near the existing `{
date }` parameters to also pass the current `mealTypeId`, preserving it through
the manual-entry flow into `FoodSearch`.
In `@SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx`:
- Around line 353-359: Update the drag-gap calculation near activeDragIndex so
target is derived from the live drag translation rather than
activeDragIndex.value; keep active as the original dragged index, and use the
resulting target in the existing shift conditions so rows between the original
and destination indices receive the appropriate offset.
- Around line 321-369: Extract the logic from renderCustomRow into a dedicated
React row component that owns the useAnimatedStyle hook and receives
activeDragIndex, panY, strides, offsets, moveCustomType, and relevant edit,
picker, delete, and mutation callbacks as props. Update orderedCustomTypes.map
to render this component instead of invoking renderCustomRow, preserving the
existing drag, reorder, and row interaction behavior.
- Around line 234-279: Update the create path in handleFormSave to include
values.isVisible as is_visible and values.showInQuickLog as show_in_quick_log in
the createMutation payload. Widen the createMutation mutationFn parameter type
to accept these fields, while preserving the existing create behavior and
relying on createMealType’s supported partial payload.
- Around line 160-180: Update persistCustomOrder to await each updateMutation
request sequentially rather than using fire-and-forget mutate calls, and remove
per-operation invalidation callbacks. After all operations complete, call
invalidate exactly once so customOrderOverride is not cleared by intermediate
refetches.
- Around line 331-339: Update the `.onEnd` callback to invoke `moveCustomType`
via `runOnJS` when from is valid and differs from to, preserving the existing
indices and conditional behavior while ensuring React state updates and
mutations execute on the JS thread.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 231-245: The test comment around the `cell` press does not match
the assertion. Update it to describe that pressing the time cell opens the
picker without triggering `updateMealType`, or extend the test to invoke the
picker callback and assert the expected HH:MM payload through `updateMealType`.
- Around line 154-175: The createMealType payload assertion in the “creates a
custom meal type…” test must also verify the form’s isVisible and showInQuickLog
values. Set or retain the expected switch states in the rendered form, then
include both fields in the expect.objectContaining assertion so dropped
visibility or quick-log values are detected.
- Around line 51-61: The `@gorhom/bottom-sheet` mock in MealTypeSettingsScreen
tests should not render BottomSheetModal children by default. Add mock
presentation state so present makes the modal content visible and dismiss hides
it, allowing create and edit tests to verify presentAdd/presentEdit wiring while
preserving the existing BottomSheetScrollView and BottomSheetView stubs.
In `@SparkyFitnessMobile/src/components/FoodSummary.tsx`:
- Around line 59-64: Update the icon selection logic near the `icon` declaration
to compute the lowercased group name and retrieve the corresponding
`MEAL_CONFIG` entry once in local variables, then reuse that entry for the
system-icon check and value. Preserve the existing `'meal-snack'` fallback.
In `@SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx`:
- Around line 140-149: Update the TextInput in MealTypeFormSheet to use the
existing textMuted theme value for placeholderTextColor instead of the hardcoded
"`#9CA3AF`", ensuring the placeholder follows light and dark themes.
- Around line 108-115: Memoize the date value used by the DateTimePicker around
the values.defaultTime conversion so an empty defaultTime does not create a new
Date on every render. Preserve the existing parsed-time behavior and only
stabilize the fallback date identity, using the component’s existing React
memoization patterns.
In `@SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx`:
- Around line 34-45: Clear the pending selection when the modal closes by adding
an onDismiss handler to the BottomSheetModal that sets pending to null. Keep the
existing present and dismiss behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 562081ab-1b6e-42a5-8a8b-a6c4a44545c0
📒 Files selected for processing (22)
SparkyFitnessMobile/__tests__/components/CopyMealSheet.test.tsxSparkyFitnessMobile/__tests__/components/FoodSummary.test.tsxSparkyFitnessMobile/__tests__/screens/FoodSearchScreen.test.tsxSparkyFitnessMobile/__tests__/screens/FoodSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/screens/MealTypeDetailScreen.test.tsxSparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/utils/mealNutrition.test.tsSparkyFitnessMobile/src/components/CopyMealSheet.tsxSparkyFitnessMobile/src/components/FoodSummary.tsxSparkyFitnessMobile/src/components/MealTypeFormSheet.tsxSparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsxSparkyFitnessMobile/src/screens/FoodPhotoEstimateReviewScreen.tsxSparkyFitnessMobile/src/screens/FoodPhotoImproveScreen.tsxSparkyFitnessMobile/src/screens/FoodPhotoIntroScreen.tsxSparkyFitnessMobile/src/screens/FoodPhotoLogEntryScreen.tsxSparkyFitnessMobile/src/screens/FoodScanScreen.tsxSparkyFitnessMobile/src/screens/FoodSearchScreen.tsxSparkyFitnessMobile/src/screens/FoodSettingsScreen.tsxSparkyFitnessMobile/src/screens/MealTypeDetailScreen.tsxSparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsxSparkyFitnessMobile/src/types/navigation.tsSparkyFitnessMobile/src/utils/mealNutrition.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- SparkyFitnessMobile/src/screens/FoodSearchScreen.tsx
- SparkyFitnessMobile/tests/components/FoodSummary.test.tsx
- SparkyFitnessMobile/src/screens/MealTypeDetailScreen.tsx
- SparkyFitnessMobile/src/components/CopyMealSheet.tsx
- SparkyFitnessMobile/tests/utils/mealNutrition.test.ts
- SparkyFitnessMobile/src/utils/mealNutrition.ts
- MealTypeSettingsScreen: the custom-row drag row is now a real React component (CustomMealTypeRow) instead of a render helper calling useAnimatedStyle — hooks are no longer invoked inside a plain function during a map (rules-of-hooks). The SharedValue prop types import the reanimated type directly instead of Animated.SharedValue. - Escaped the apostrophe in the system-row hint text (JSX entity). - MealTypeSettingsScreen.test.tsx / mealNutrition.test.ts: removed unused imports and destructured queries flagged by the CI lint pass (eslint, not the cached expo lint). Assisted-by: Open WebUI
|
Thanks for the detailed UI/product feedback. The redesign is pushed ( Ordering
Time picker
Duplication
Code cleanup
UI details
Testing honesty
|
- MealTypeSettingsScreen drag rows: onMove is dispatched through runOnJS from the gesture worklet (worklet→JS boundary); the row gap target is now derived from the live pan translation via computeReorderTargetIndex instead of re-reading activeDragIndex (which always equalled active, so neighbours never opened a gap). - Reorder persistence is sequential (mutateAsync per row) with a single invalidate at the end, so the order override is not reset per row and the displayed order cannot briefly snap back. - Create payload now includes is_visible and show_in_quick_log (the form sheet offers both toggles in add mode). - FoodPhotoLogEntry preselection validates the originating meal type id against the selectable list — a stale/hidden/deleted id falls back to the default instead of being submitted for a new entry. - FoodPhotoImprove recovery navigation (remove image / retry after estimate error) keeps mealTypeId on both FoodScan replacements; FoodScan forwards mealTypeId on both FoodPhotoIntro navigations (including the photo-mode button path) and its effect dependency. - MealTypeDetail test mock types the header-action shape instead of any; MealTypeSettings test awaits the loaded list before asserting the disabled create button. Assisted-by: Open WebUI
Final SPLIT-2 correction round (maintainer product decision + blockers): Visibility (product decision): - Mobile Meal Type Settings no longer exposes any Visibility UI (system rows, custom rows, Add/Edit form). Diary sections are data-driven and appear when logged entries exist. Backend is_visible is preserved: the edit payload omits is_visible entirely (backend COALESCE keeps it) and creation relies on the verified backend default is_visible = true. show_in_quick_log is the only per-user toggle besides default time. Reorder persistence (blocker): - persistCustomOrder now uses a DEDICATED path: direct updateMealType calls (NOT the generic updateMutation which invalidated+toasted per row). Success: optimistic order updates immediately, deterministic sequential sort_orders written only for changed rows, no refetch between rows, then setQueryData + exactly ONE invalidate. Failure: one reorder-specific error, single log, single invalidate, optimistic override cleared so the UI reconciles with server truth. Component + picker hardening: - CustomMealTypeRow moved to MODULE SCOPE (stable component identity — no remounts on every parent render for gesture-driven rows); DnD worklet keeps runOnJS(onMove) at the worklet→JS boundary and derives the gap target from live translation. - MealTypeTimePickerSheet: onDismiss clears pending value + callback ref (dismiss without Save/Clear never invokes the callback); picker Date memoized. MealTypeFormSheet: picker Date memoized so typing the name does not reseed the wheel; placeholder uses the theme textMuted token instead of hardcoded #9CA3AF. - FoodSummary: single canonical MEAL_CONFIG lookup (systemConfig read once, icon + label reuse); ownership rules preserved. Tests: - Controllable bottom-sheet mock: children render only while presented (real presentation semantics; Add-after-Edit does not retain values). - Reorder: Alpha 100 / Beta 110 → Beta 100 / Alpha 110 with correct ids, payloads and exactly ONE invalidate; partial-failure case verifies one error, reconciliation and single refetch. - Time picker: real Save (default_time 'HH:MM'), Clear (null), dismiss without action (no update). - Create: no is_visible in payload, auto end-of-list sort_order, quick log follow-up update; Edit: payload excludes sort_order and is_visible. - Visibility UI absent; quick-log toggles via semantic labels; system rows: icon, quick-log, time, no drag/edit/delete/Visibility. Assisted-by: Open WebUI
|
@coderabbitai review Fresh review requested for |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx (1)
351-389: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winGuard against overlapping reorder runs.
moveCustomTypecallspersistCustomOrderwithout waiting for a previous run. If the user drags twice quickly, two runs overlap. Each run capturescustomTypesfrom its own closure, so the second run computesopsfrom data that does not include the first run's in-flight writes. When the first run finishes, it callssetQueryData, clearscustomOrderOverride, and invalidates. The UI can then show the intermediate order while the second run is still writing.The final
invalidate()corrects the state, so the impact is a transient wrong order. Add a run token to drop stale completions.♻️ Proposed guard
+ const reorderRunRef = useRef(0); const persistCustomOrder = useCallback( async (orderedIds: string[]) => { + const runId = ++reorderRunRef.current; const byId = new Map(customTypes.map((mt) => [mt.id, mt]));); - setCustomOrderOverride(null); - invalidate(); + if (runId !== reorderRunRef.current) return; + setCustomOrderOverride(null); + invalidate(); } catch (err) { addLog(`Failed to persist meal type order: ${(err as Error).message}`, 'ERROR'); Toast.show({ type: 'error', text1: 'Failed to reorder meal types' }); // Reconcile with server truth: drop the optimistic order and refetch. - setCustomOrderOverride(null); - invalidate(); + if (runId === reorderRunRef.current) { + setCustomOrderOverride(null); + } + invalidate(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx` around lines 351 - 389, Update persistCustomOrder to use a monotonically increasing run token shared across invocations, and capture the token when each run starts. Before applying success or failure completion effects—setQueryData, clearing customOrderOverride, logging/toasting as applicable, and invalidate—verify the token is still current; stale runs must exit without changing UI or cache state.SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx (1)
11-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the sheet dismissal handle per instance.
mockSheetState.forceSetis a single global slot. The screen mounts twoBottomSheetModalinstances: the form sheet and the time picker sheet. Each instance overwritesforceSetin its own effect, so the slot points at whichever sheet re-rendered last. The current tests pass only because the sheet under test is the one that toggledshownmost recently.Store one setter per sheet so a test dismisses the intended sheet. For example, key the registry by the rendered content or by a
testIDprop, and exposedismissSheet(key).Also note that
mockSheetState.presentedis assigned but no assertion reads it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx` around lines 11 - 46, Update the BottomSheetModal mock and its test control state so dismissal setters are stored per mounted sheet instance rather than in the single global mockSheetState.forceSet slot. Key each setter using the sheet’s rendered content or testID, expose a dismissSheet(key) helper, and update callers to dismiss the intended form or time-picker sheet; remove the unused mockSheetState.presented state and its assignments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 206-211: Update the dinner icon assertion in the test case
“renders the canonical system icons from MEAL_CONFIG (no parallel map)” to await
findByTestId before passing its resolved element to expect, ensuring the icon
lookup is actually verified and any rejection is handled.
- Around line 524-527: Update the destructive-button handling in the alert test
to assert that the destructive button exists before using it, then assert that
its optional onPress callback is defined before invoking it. Preserve the
existing deleteSpy expectation after the callback runs.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 11-46: Update the BottomSheetModal mock and its test control state
so dismissal setters are stored per mounted sheet instance rather than in the
single global mockSheetState.forceSet slot. Key each setter using the sheet’s
rendered content or testID, expose a dismissSheet(key) helper, and update
callers to dismiss the intended form or time-picker sheet; remove the unused
mockSheetState.presented state and its assignments.
In `@SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx`:
- Around line 351-389: Update persistCustomOrder to use a monotonically
increasing run token shared across invocations, and capture the token when each
run starts. Before applying success or failure completion effects—setQueryData,
clearing customOrderOverride, logging/toasting as applicable, and
invalidate—verify the token is still current; stale runs must exit without
changing UI or cache state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f76b492-1b70-45c5-9a85-341bf5c29aa9
📒 Files selected for processing (12)
SparkyFitnessMobile/__tests__/components/FoodSummary.test.tsxSparkyFitnessMobile/__tests__/screens/MealTypeDetailScreen.test.tsxSparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/utils/mealNutrition.test.tsSparkyFitnessMobile/src/components/FoodSummary.tsxSparkyFitnessMobile/src/components/MealTypeFormSheet.tsxSparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsxSparkyFitnessMobile/src/screens/FoodPhotoImproveScreen.tsxSparkyFitnessMobile/src/screens/FoodPhotoLogEntryScreen.tsxSparkyFitnessMobile/src/screens/FoodScanScreen.tsxSparkyFitnessMobile/src/screens/FoodSettingsScreen.tsxSparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- SparkyFitnessMobile/tests/screens/MealTypeDetailScreen.test.tsx
- SparkyFitnessMobile/src/screens/FoodPhotoImproveScreen.tsx
- SparkyFitnessMobile/src/screens/FoodPhotoLogEntryScreen.tsx
- SparkyFitnessMobile/src/components/FoodSummary.tsx
- SparkyFitnessMobile/src/screens/FoodScanScreen.tsx
- SparkyFitnessMobile/tests/utils/mealNutrition.test.ts
- SparkyFitnessMobile/src/screens/FoodSettingsScreen.tsx
- SparkyFitnessMobile/tests/components/FoodSummary.test.tsx
… design) Implement the final maintainer design from the text-only spec (PR CodeWithCJ#2063): Ordering model: - ONE unified list on Meal Types: system anchors (Breakfast 10, Lunch 20, Dinner 30, Snacks 40) are FIXED; custom types live in the integer slots between anchors (11-19, 21-29, 31-39), max 9 per gap. No separate System/Custom sections, no raw sort_order shown anywhere. - New mealTypeSlots helper: gap mapping (incl. legacy values), unified list building, gap-aware move (same-gap and cross-gap), minimal sort_order writes (sequential slots, system anchors never written), capacity rejection (a 10th custom in a gap is refused with one concise toast, no partial writes). - Create: auto end-of-list slot in d_s (first free slot, 31+); if d_s is full the create is rejected with a clear toast; no more 100/110/120. - Custom rows are draggable via the existing WorkoutReorderList gesture pattern (Gesture.Pan + computeReorderTargetIndex + runOnJS at the worklet→JS boundary, gap derived from live translation); system rows show the canonical FILLED MEAL_CONFIG icon and are not draggable. - Accessibility: Move up / Move down actions use the same ordering algorithm (cross-anchor moves allowed, capacity applies). Race hardening: - Reorder persistence is SERIALIZED through a promise chain; an older persistence sequence can never overwrite a newer accepted visual order (newest-desired-order wins deterministically). Direct updateMealType writes (no generic mutation toasts/invalidations per row), exactly ONE invalidate on success, and on failure one reorder-specific error + override reset + refetch. Deferred-promise regression test covers the old-reorder-cannot-overwrite case. UI: - Rows: leading icon (system) / drag handle (custom), primary name label, clearly tappable right-side time element (large target + chevron, not a tiny glyph). Row tap opens the edit sheet; time tap opens the large time-picker sheet; drag starts only on the handle. - Edit sheet: Name (custom editable / system display-only), Visibility (real backend is_visible, themed Switch — no platform green), Quick log (backend show_in_quick_log), Default time row opening the LARGE wheel sheet, Delete (custom only, danger token, confirmation). Edit payload preserves sort_order and never touches order. - Create sheet: name + Visibility + Quick log + the actual large time wheel inline (one creation experience), NO Delete; create is one logical operation (base POST then per-user follow-ups reconciled, no half-configured success). - Time picker sheet: dominant wheel (scale requirement), Save commits HH:MM, Clear commits null, dismiss without action makes NO change (pending state cleared), picker Date memoized (no reseed on unrelated renders). Form sheet placeholder uses the theme textMuted token. Assisted-by: Open WebUI
Replace the unused loop variable in resolveCustomTargetGap with an index loop; behavior unchanged. Assisted-by: Open WebUI
|
Thanks for the detailed text specification — the final design is implemented and pushed ( Ordering model (the core fix)
Rows / interaction
Sheets
Testing honesty
|
|
@coderabbitai review Fresh review requested for |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx (2)
281-297: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert both writes unconditionally.
The expectations are wrapped in
if (aWrite)andif (bWrite). The test passes when neitheranorbis written, because onlywrites.length >= 1is enforced. The test then cannot detect a regression in the coalescing logic it is named for. Wait for both writes, then assert the slot ranges.💚 Proposed change
await waitFor(() => { - const writes = updateSpy.mock.calls.map((c) => c[0]); - expect(writes.length).toBeGreaterThanOrEqual(1); - // Deterministic final state: B before Lunch (b_l), A after Lunch (l_d). - const aWrite = updateSpy.mock.calls.find((c) => c[0] === 'a'); - const bWrite = updateSpy.mock.calls.find((c) => c[0] === 'b'); - if (aWrite) { - const s = (aWrite[1] as any).sort_order; - expect(s).toBeGreaterThanOrEqual(21); - expect(s).toBeLessThanOrEqual(29); - } - if (bWrite) { - const s = (bWrite[1] as any).sort_order; - expect(s).toBeGreaterThanOrEqual(11); - expect(s).toBeLessThanOrEqual(19); - } + // Deterministic final state: B before Lunch (b_l), A after Lunch (l_d). + const aWrite = updateSpy.mock.calls.find((c) => c[0] === 'a'); + const bWrite = updateSpy.mock.calls.find((c) => c[0] === 'b'); + expect(aWrite).toBeDefined(); + expect(bWrite).toBeDefined(); + const aSort = (aWrite![1] as any).sort_order; + const bSort = (bWrite![1] as any).sort_order; + expect(aSort).toBeGreaterThanOrEqual(21); + expect(aSort).toBeLessThanOrEqual(29); + expect(bSort).toBeGreaterThanOrEqual(11); + expect(bSort).toBeLessThanOrEqual(19); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx` around lines 281 - 297, Update the coalescing test around updateSpy so it waits until both the a and b calls are present, then assert both writes unconditionally. Remove the conditional guards around aWrite and bWrite while preserving their existing sort_order range checks and the current write-count validation.
189-194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the raw-order assertion so times cannot match.
queryAllByText(/\b(11|21|31|100|110)\b/)also matches displayed times. A fixture withdefault_time: '11:00'makes this test fail even though no sort order is exposed. Match a label form instead, for example/\b(sort[_ ]?order|Order)\b/i, or assert on the specific row text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx` around lines 189 - 194, Update the raw-order assertion in the “never exposes raw sort_order / Order numbers” test to avoid matching displayed meal times such as “11:00”. Scope the check to an order label or specific row text, using the existing rendered screen queries, while preserving the assertion that raw sort-order information is not exposed.SparkyFitnessMobile/src/utils/mealTypeSlots.ts (2)
104-107: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueGuard the slot count against the gap capacity.
slotsForGapreturns values outside the gap whencountexceedsMAX_CUSTOM_PER_GAP. For exampleslotsForGap('b_l', 10)returns a final value of20, which is the Lunch anchor. Callers validate capacity today, so this is defensive only. Add a guard so a future caller cannot write an anchor value.♻️ Proposed hardening
export function slotsForGap(gap: MealGapKey, count: number): number[] { - const [first] = GAP_SLOT_RANGE[gap]; - return Array.from({ length: count }, (_, i) => first + i); + const [first, last] = GAP_SLOT_RANGE[gap]; + const capped = Math.min(count, last - first + 1); + return Array.from({ length: capped }, (_, i) => first + i); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/utils/mealTypeSlots.ts` around lines 104 - 107, Update slotsForGap to cap the requested count at the gap’s available custom-slot capacity, MAX_CUSTOM_PER_GAP, before generating values. Preserve the existing sequence starting at GAP_SLOT_RANGE[gap] and prevent any returned slot from reaching the gap’s anchor.
120-147: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid mutating the input gaps.
moveCustomTypeBetweenGapssplicescurrentGaps[fromGap]andcurrentGaps[toGap]in place. The returned object is a new reference, but it holds the same array references, so the caller's previous state is also modified. A caller that passes React state loses the pre-move value and gets no identity change on the arrays. The rejection path restores the element, which shows the intent is non-mutating.Copy the arrays before you modify them.
♻️ Proposed refactor
const src = currentGaps[fromGap]; if (fromIndex < 0 || fromIndex >= src.length) return null; - const [moved] = src.splice(fromIndex, 1); - - let dst = currentGaps[toGap]; - if (toGap === fromGap) { - // Same gap: insertion index is relative to the list AFTER the removal. - const clamped = Math.min(Math.max(toIndex, 0), dst.length); - dst.splice(clamped, 0, moved); - return { ...currentGaps }; - } - if (dst.length >= MAX_CUSTOM_PER_GAP) { - // Restore the removed element before returning null so the caller's - // optimistic state stays consistent if it decides to keep it. - src.splice(fromIndex, 0, moved); - return null; - } - const clamped = Math.min(Math.max(toIndex, 0), dst.length); - dst.splice(clamped, 0, moved); - return { ...currentGaps }; + const nextSrc = src.slice(); + const [moved] = nextSrc.splice(fromIndex, 1); + + if (toGap === fromGap) { + // Same gap: insertion index is relative to the list AFTER the removal. + const clamped = Math.min(Math.max(toIndex, 0), nextSrc.length); + nextSrc.splice(clamped, 0, moved); + return { ...currentGaps, [fromGap]: nextSrc }; + } + const nextDst = currentGaps[toGap].slice(); + if (nextDst.length >= MAX_CUSTOM_PER_GAP) return null; + const clamped = Math.min(Math.max(toIndex, 0), nextDst.length); + nextDst.splice(clamped, 0, moved); + return { ...currentGaps, [fromGap]: nextSrc, [toGap]: nextDst };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/utils/mealTypeSlots.ts` around lines 120 - 147, Update moveCustomTypeBetweenGaps to clone the source and destination gap arrays before any splice operations, while preserving the existing same-gap insertion, capacity rejection, restoration behavior, and return contract. Ensure currentGaps and all of its original arrays remain unchanged on both success and rejection paths.SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx (2)
456-469: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the slot constants and remove the duplicate
setIsCreating(false).
gapFirstre-declares the first slot of each gap.mealTypeSlots.tsalready exportsGAP_SLOT_RANGEandslotsForGap. Duplicating the numbers here lets the two definitions drift. The capacity branch also callssetIsCreating(false)twice.♻️ Proposed refactor
if (current[targetGap].length >= MAX_CUSTOM_PER_GAP) { setIsCreating(false); Toast.show({ type: 'error', text1: `No more meal types can be placed ${GAP_USER_LABEL[targetGap]}.`, }); - setIsCreating(false); return; } - const gapFirst: Record<MealGapKey, number> = { b_l: 11, l_d: 21, d_s: 31 }; - const nextSort = gapFirst[targetGap] + current[targetGap].length; + const nextSort = GAP_SLOT_RANGE[targetGap][0] + current[targetGap].length;Add
GAP_SLOT_RANGEto the import from../utils/mealTypeSlots.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx` around lines 456 - 469, Update the meal-type creation flow to import and reuse GAP_SLOT_RANGE or slotsForGap from mealTypeSlots instead of declaring the local gapFirst constants, deriving nextSort from the shared slot definitions. Remove the duplicate setIsCreating(false) call in the capacity branch while preserving its existing early return behavior.
292-302: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMemoize
strideswithoffsets.
stridesis rebuilt on every render, so its identity changes even when the row count does not. The array is passed to everyCustomMealTypeRowand captured by the drag worklet, which forces a new worklet closure per render. Memoize it onunifiedRows.lengthlikeoffsets.♻️ Proposed refactor
- const strides = unifiedRows.map(() => ROW_HEIGHT + ROW_GAP); + const strides = useMemo( + () => unifiedRows.map(() => ROW_HEIGHT + ROW_GAP), + // eslint-disable-next-line react-hooks/exhaustive-deps + [unifiedRows.length], + ); const offsets = useMemo(() => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx` around lines 292 - 302, Memoize the strides calculation alongside offsets using unifiedRows.length as its dependency, so the strides array retains a stable identity when the row count is unchanged. Update the strides declaration near offsets without changing the existing stride values or offset computation.SparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.ts (1)
154-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the comment and cover an interior target index.
The comment on Line 164 states the result is the
d_sgap at index 0, but the assertion expects{ gap: 'l_d', index: 2 }.d_sis empty, so it never appears in the flattened list.No case covers a target that falls inside a gap at a non-zero position, for example
targetIndex = 2withl_d: ['l2','l3']. That gap hides the defect reported onSparkyFitnessMobile/src/utils/mealTypeSlots.tsLines 235-252. If you keep the function, add that case. If you delete the function, delete this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.ts` around lines 154 - 166, Correct the targetIndex 3 test comment to describe the l_d gap at index 2, since the empty d_s gap is not flattened. Add an assertion for targetIndex 2 that resolves to the l_d gap at its non-zero interior position, covering the defect in resolveCustomTargetGap; if that function is removed, remove this test instead.SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx (1)
297-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared save labels.
Import
SAVE_LABELandSAVING_LABELfrom../hooks/useScreenHeader. Use them for both create and edit button text to keep form labels consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx` around lines 297 - 317, Import SAVE_LABEL and SAVING_LABEL from ../hooks/useScreenHeader in MealTypeFormSheet, then replace the create and edit button text expressions with these shared labels while preserving the existing isSaving conditional behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 438-443: Fix the dismissal test by exercising the mocked
BottomSheetModal’s actual dismissal path rather than sending a “dismiss” event
to the Save button. Extend the BottomSheetModal mock with a backdrop testID that
invokes the same imperative dismiss behavior, press that backdrop in the test,
and replace the raw setTimeout wait with act or waitFor before asserting
updateSpy was not called.
In `@SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx`:
- Around line 264-272: Update the time picker initialization in the
TouchableOpacity onPress handler to pass the current form value, such as the
form’s defaultTime field, to present instead of
toHourMinute(editingMt.default_time). Preserve the existing null fallback and
setValues callback so reopening the picker reflects the unsaved selection.
In `@SparkyFitnessMobile/src/utils/mealTypeSlots.ts`:
- Around line 235-252: The function resolveCustomTargetGap returns index 0 for
interior targets; update it to calculate and return the position within the
selected gap, or remove the function if it has no production callers. In
SparkyFitnessMobile/src/utils/mealTypeSlots.ts lines 235-252, preserve boundary
behavior; in SparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.ts lines
154-166, correct the comment to match the { gap: 'l_d', index: 2 } assertion and
add coverage for targetIndex 2 with l_d containing ['l2','l3'], or delete the
test if the function is removed.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx`:
- Around line 281-297: Update the coalescing test around updateSpy so it waits
until both the a and b calls are present, then assert both writes
unconditionally. Remove the conditional guards around aWrite and bWrite while
preserving their existing sort_order range checks and the current write-count
validation.
- Around line 189-194: Update the raw-order assertion in the “never exposes raw
sort_order / Order numbers” test to avoid matching displayed meal times such as
“11:00”. Scope the check to an order label or specific row text, using the
existing rendered screen queries, while preserving the assertion that raw
sort-order information is not exposed.
In `@SparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.ts`:
- Around line 154-166: Correct the targetIndex 3 test comment to describe the
l_d gap at index 2, since the empty d_s gap is not flattened. Add an assertion
for targetIndex 2 that resolves to the l_d gap at its non-zero interior
position, covering the defect in resolveCustomTargetGap; if that function is
removed, remove this test instead.
In `@SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx`:
- Around line 297-317: Import SAVE_LABEL and SAVING_LABEL from
../hooks/useScreenHeader in MealTypeFormSheet, then replace the create and edit
button text expressions with these shared labels while preserving the existing
isSaving conditional behavior.
In `@SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx`:
- Around line 456-469: Update the meal-type creation flow to import and reuse
GAP_SLOT_RANGE or slotsForGap from mealTypeSlots instead of declaring the local
gapFirst constants, deriving nextSort from the shared slot definitions. Remove
the duplicate setIsCreating(false) call in the capacity branch while preserving
its existing early return behavior.
- Around line 292-302: Memoize the strides calculation alongside offsets using
unifiedRows.length as its dependency, so the strides array retains a stable
identity when the row count is unchanged. Update the strides declaration near
offsets without changing the existing stride values or offset computation.
In `@SparkyFitnessMobile/src/utils/mealTypeSlots.ts`:
- Around line 104-107: Update slotsForGap to cap the requested count at the
gap’s available custom-slot capacity, MAX_CUSTOM_PER_GAP, before generating
values. Preserve the existing sequence starting at GAP_SLOT_RANGE[gap] and
prevent any returned slot from reaching the gap’s anchor.
- Around line 120-147: Update moveCustomTypeBetweenGaps to clone the source and
destination gap arrays before any splice operations, while preserving the
existing same-gap insertion, capacity rejection, restoration behavior, and
return contract. Ensure currentGaps and all of its original arrays remain
unchanged on both success and rejection paths.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2006e33f-c5f5-4790-ae43-357b406e1249
📒 Files selected for processing (6)
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.tsSparkyFitnessMobile/src/components/MealTypeFormSheet.tsxSparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsxSparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsxSparkyFitnessMobile/src/utils/mealTypeSlots.ts
- Removed resolveCustomTargetGap (no production caller; its in-gap index was always 0 for interior targets, contradicting its contract) plus its unit test and import. - MealTypeFormSheet edit row seeds the LARGE time picker from the CURRENT form value (values.defaultTime) so an unsaved selection survives reopening; dropped the now-unused editingMt state. - Time picker dismiss test now presses the mocked sheet backdrop (the real dismissal path) instead of firing a fake 'dismiss' event on the Save button, and uses act() instead of a raw setTimeout. Assisted-by: Open WebUI
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="732647fa"
echo '=== Commit metadata and changed files ==='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
echo '=== Focused AST outlines ==='
for file in \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/mealTypeSlots.ts \
SparkyFitnessMobile/src/hooks/useMealTypes.ts \
SparkyFitnessMobile/src/hooks/queryKeys.ts \
SparkyFitnessMobile/src/services/api/mealTypesApi.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.ts
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
done
echo '=== Commit diff ==='
git show --no-ext-diff --format= --find-renames "$commit" -- \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/mealTypeSlots.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.ts
echo '=== Mutation-token and reorder implementation ==='
rg -n -C8 \
'fieldOwnerRef|token|onMutate|onSuccess|onError|onSettled|setQueryData|latestDesiredOrderRef|workerRunningRef|generation|override|customTypesRef|persist|reorder' \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/mealTypeSlots.ts \
-g '*.ts' -g '*.tsx'
echo '=== Numbered source slices ==='
nl -ba SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx | sed -n '180,740p'
nl -ba SparkyFitnessMobile/src/utils/mealTypeSlots.ts | sed -n '1,380p'
echo '=== Focused concurrency tests ==='
rg -n -C12 \
'same.value|token|owner|deferred|concurr|rollback|optimistic|stale|rapid|generation|reorder|failure|snap' \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.ts \
-g '*.test.ts' -g '*.test.tsx'
echo '=== Query and API contracts ==='
sed -n '1,180p' SparkyFitnessMobile/src/hooks/useMealTypes.ts
sed -n '1,140p' SparkyFitnessMobile/src/hooks/queryKeys.ts
sed -n '1,260p' SparkyFitnessMobile/src/services/api/mealTypesApi.tsLength of output: 81287 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== MealTypeSettingsScreen: imports, rows, mutation, and reorder ==='
sed -n '1,230p' SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
sed -n '230,620p' SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
sed -n '620,860p' SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
echo '=== Meal type query, API, and type contracts ==='
sed -n '1,220p' SparkyFitnessMobile/src/hooks/useMealTypes.ts
sed -n '1,180p' SparkyFitnessMobile/src/hooks/queryKeys.ts
sed -n '1,280p' SparkyFitnessMobile/src/services/api/mealTypesApi.ts
sed -n '1,180p' SparkyFitnessMobile/src/types/mealTypes.ts
echo '=== Meal type slot model ==='
sed -n '1,360p' SparkyFitnessMobile/src/utils/mealTypeSlots.ts
echo '=== Test setup and all new concurrency tests ==='
sed -n '1,470p' SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
sed -n '470,1120p' SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== All direct mealTypesQueryKey cache writers and useMealTypes consumers ==='
rg -n -C5 \
'mealTypesQueryKey|setQueryData<MealType|useMealTypes\s*\(' \
SparkyFitnessMobile/src SparkyFitnessMobile/__tests__ \
-g '*.ts' -g '*.tsx'
echo '=== Current commit source diff with stable line labels ==='
git diff --unified=80 732647fa^ 732647fa -- \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxLength of output: 187469
I found no new correctness issues. The per-field token prevents an older completion from modifying a field after a newer mutation owns it. This covers the same-value update case from the previous P1. The reorder worker keeps the newer optimistic order while an older persistence request completes. The generation checks and absolute slot writes preserve the newest accepted order. The added deferred tests cover the reported races. You are interacting with an AI system. |
Independent review blocker: per-field tokens protect the React Query cache from stale callbacks but NOT the server — PUT /api/meal-types/:id has no client version token, so an older request could commit after a newer one and a later refetch would surface the stale value. Network execution ordering: - updateRequestQueueRef (Map<mealTypeId, Promise>) serializes the actual PUTs PER record in user-initiation order: a request waits for the previous one on the same id, so the newest user intent is always the last server write. Different record IDs may still run concurrently. The chain survives failures (run.catch) so a later request still runs. - The mutation token is now allocated SYNCHRONOUSLY at the mutate boundary (before cancelQueries), so token order == user action order. - onSettled invalidates ONLY after the record's update queue drains (pendingUpdatesRef gating) — an intermediate completion never refetches over a newer optimistic mutation. Optimistic UI is unchanged: B's switch/time still updates the cache immediately while A's PUT is pending; only the network write waits. Tests (deferred, mutate fake serverState ONLY when a request executes): - REAL visibility server-write race: A→false, B→true; serialized PUTs give server writes exactly false then true; final server/cache/UI true. (Fails against the old parallel-request implementation.) - REAL default-time server-write race: 16:00, A→17:30, B→18:45; final 18:45 everywhere. - Two failures (A→false fail, B→true fail): cache/UI reconcile to the original server value (true); no stranded optimistic value. - Same-value visibility/time races updated for serialization (B's PUT waits for A; updateCalls stays 1 until A settles). - Different-record concurrency (Breakfast/Lunch) still green — queues are independent per id. Reorder worker tests untouched and green. Full Jest 286 suites / 4819 tests, 0 failed. Assisted-by: Open WebUI
|
@coderabbitai review Fresh review requested for |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="180b0638"
echo '=== Commit metadata and changed files ==='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
echo '=== Focused AST outlines ==='
for file in \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/services/api/mealTypesApi.ts \
SparkyFitnessMobile/src/hooks/useMealTypes.ts \
SparkyFitnessMobile/src/hooks/queryKeys.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
done
echo '=== Target diff ==='
git diff --unified=80 "${commit}^" "$commit" -- \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== Update mutation and queue implementation ==='
nl -ba SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx | sed -n '180,410p'
echo '=== All update mutation call sites ==='
rg -n -C6 'updateMutation\.(mutate|mutateAsync)|updateMealType\(' \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src -g '*.ts' -g '*.tsx'
echo '=== Query key and useMealTypes contract ==='
sed -n '1,180p' SparkyFitnessMobile/src/hooks/queryKeys.ts
sed -n '1,220p' SparkyFitnessMobile/src/hooks/useMealTypes.ts
echo '=== Relevant deferred and queue regression tests ==='
rg -n -C12 \
'serial|queue|deferred|visibility false|17:30|18:45|same.value|two.failure|independent|invalidate|pending' \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== Test harness and API mocks ==='
sed -n '1,420p' SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxLength of output: 41071
You are interacting with an AI system. |
CodeRabbit P1 (5231725071) — reserve the per-record request order before
awaiting cancelQueries.
Reservation model:
- New mutateMealType(id, data, options?) wrapper is the ONLY entry point for
row Visibility, row default time, and Edit Save. At the SYNCHRONOUS
user-action boundary it: (1) allocates the mutation token; (2) reserves a
per-record queue SLOT (UpdateReservation { predecessor, done, resolveDone }
— B sees A's done immediately, so PUT order equals user-initiation order
regardless of cancelQueries/network timing); (3) increments the per-record
pending counter; (4) reserves field ownership; (5) captures rollback
metadata from the CURRENT cache; (6) invokes the TanStack mutation with
those internal vars.
- mutationFn waits for the reserved predecessor slot, performs the PUT, and
releases the slot in a finally (a failed PUT never blocks the next one).
The PUT does NOT start at reservation time.
- onMutate (after cancelQueries) applies the optimistic cache write ONLY for
fields this mutation still owns — a delayed older onMutate cannot overwrite
a newer optimistic value (ownership was reserved synchronously).
- onSettled releases the slot on EVERY settling path (including onMutate
failure where mutationFn never ran) so the per-record queue can never
deadlock, and still gates the authoritative invalidate until the record's
queue drains.
Regression tests (all fail against 180b063's mutationFn-reservation):
- delayed cancelQueries visibility: A→false, B→true; cancel(A) pending,
cancel(B) resolves first; PUT order ['false','true']; final
server/cache/Switch = true; A's delayed onMutate cannot overwrite B's
optimistic true.
- delayed cancelQueries time: A 17:30, B 18:45; PUT order ['17:30','18:45'];
final server/cache/rendered = 18:45.
- queue failure: A PUT fails, B reserved after A still executes (no
deadlock); callLog ['false','true'].
Existing concurrency regressions stay green: same-value, same-field
different values, different fields, different-record queues, A-success/
B-fail, A-fail/B-success, both-fail, plus all reorder worker tests.
Full Jest 286 suites / 4822 tests, 0 failed.
Assisted-by: Open WebUI
|
@coderabbitai review Fresh review requested for |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="8bc4386d"
echo '=== Commit metadata and changed files ==='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
echo '=== Focused source outlines ==='
for file in \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/hooks/useMealTypes.ts \
SparkyFitnessMobile/src/hooks/queryKeys.ts \
SparkyFitnessMobile/src/services/api/mealTypesApi.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
done
echo '=== Focused commit diff ==='
git diff --unified=100 "${commit}^" "$commit" -- \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== Reservation, mutation lifecycle, and cache implementation ==='
rg -n -C10 \
'mutateMealType|UpdateReservation|updateRequestQueueRef|reserve|predecessor|resolveDone|done|onMutate|mutationFn|onSuccess|onError|onSettled|cancelQueries|invalidateQueries|pendingUpdatesRef|fieldOwnerRef' \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
echo '=== Numbered implementation context ==='
nl -ba SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx | sed -n '180,460p'
echo '=== All update entry points and direct API calls ==='
rg -n -C6 \
'mutateMealType\s*\(|updateMutation\.(mutate|mutateAsync)|updateMealType\s*\(' \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src -g '*.ts' -g '*.tsx'
echo '=== New queue and delayed-cancel regression tests ==='
rg -n -C14 \
'delayed|cancelQueries|reservation|queue|predecessor|failure|deadlock|serial|PUT order|initiation|same.value|invalidate' \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== Query and API contracts ==='
sed -n '1,220p' SparkyFitnessMobile/src/hooks/useMealTypes.ts
sed -n '1,160p' SparkyFitnessMobile/src/hooks/queryKeys.ts
sed -n '1,280p' SparkyFitnessMobile/src/services/api/mealTypesApi.tsLength of output: 65625 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Complete generic-update implementation ==='
sed -n '245,505p' SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
echo '=== Generic-update call sites and direct update API call sites ==='
rg -n -C5 \
'mutateMealType\s*\(|updateMutation\.(mutate|mutateAsync)|updateMealType\s*\(' \
SparkyFitnessMobile/src -g '*.ts' -g '*.tsx'
echo '=== Query-key and visible-meal consumer contract ==='
sed -n '1,220p' SparkyFitnessMobile/src/hooks/useMealTypes.ts
sed -n '1,160p' SparkyFitnessMobile/src/hooks/queryKeys.ts
echo '=== Test setup and new regression-test bodies ==='
sed -n '1,260p' SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
sed -n '1240,1470p' SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== React Query package version and mutation lifecycle patterns ==='
sed -n '/"`@tanstack`\/react-query"/p' SparkyFitnessMobile/package.json
rg -n -C4 \
'useMutation<|onMutate:|onSettled:' \
SparkyFitnessMobile/src -g '*.ts' -g '*.tsx' | head -360Length of output: 67545
I found no new correctness issues.
The field-token checks prevent a delayed older optimistic write from replacing newer state. The reservation release path prevents a failed request from blocking the next request. The new tests cover delayed cancellation for visibility and default time. They also cover queue release after a failed request. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx`:
- Around line 1-4: Update the imports used by the MealTypeTimeWheel component to
provide the React type referenced by React.FC<MealTypeTimeWheelProps>, either by
importing React or by importing FC and using FC directly. Keep the component’s
existing props and behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a663627-3ee0-43bf-bac9-f353104aa1e5
📒 Files selected for processing (7)
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsxSparkyFitnessMobile/__tests__/utils/mealTypeSlots.test.tsSparkyFitnessMobile/src/components/MealTypeFormSheet.tsxSparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsxSparkyFitnessMobile/src/components/MealTypeTimeWheel.tsxSparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsxSparkyFitnessMobile/src/utils/mealTypeSlots.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx
- SparkyFitnessMobile/tests/utils/mealTypeSlots.test.ts
- SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
Pre-device-build cleanup (maintainer mockups require human-facing labels): - System meal types (user_id === null) now display their canonical MEAL_CONFIG labels (Breakfast/Lunch/Dinner/Snacks) via the existing getMealTypeDisplayLabel helper — the raw lowercase backend identifiers are never exposed in the main unified list, row accessibility labels (Edit/Visible/Default time for), the system edit sheet display-only name, and the Quick log accessibility label. Backend name is never mutated; persistence/payloads unchanged. - Custom meal types stay literal: a custom type named 'breakfast' renders 'breakfast' (user_id != null branch of the same helper). - MealTypeTimeWheel: narrow type import (React.FC -> type FC). Regression tests: - system labels: Breakfast/Snacks canonical + accessibility labels use canonical display; raw 'breakfast'/'snacks' absent. - custom collision: custom 'breakfast' renders literal, system Breakfast stays canonical; accessibility literal for custom, canonical for system. Full Jest 286 suites / 4824 tests, 0 failed. Assisted-by: Open WebUI
|
@coderabbitai review Fresh review requested for |
|
Thanks again for the mockups and detailed direction. I’ve done one final pre-device-pass cleanup on the Meal Types implementation, including making sure the built-in system meal types use their canonical display labels ( I’ve also started building a fresh mobile version from this PR for a physical-device pass. I’ll attach updated screenshots shortly once the build is installed and I’ve checked the unified drag list, fixed anchors, Visibility controls, edit sheet, and the larger time picker against your mockups. I’m leaving the mobile tested checkbox unchecked until that device pass is complete. |
Physical Android pass on PR CodeWithCJ#2063 found two interaction bugs that the automated suite could not catch; both are corrected here using the app's existing, device-proven patterns. BUG A — Default Time wheel was invisible on device: - Root cause: MealTypeTimeWheel wrapped react-native-ui-datepicker in `<View style={{ transform: [{ scale: 1.8 }] }}>`. The library's wheel is an Animated.FlatList driven with the native driver; inside a scaled ancestor it rendered completely blank on physical Android (the sheet opened, title + Clear/Save visible, picker area empty). - Fix: removed the transform-scale hack entirely. The wheel now renders the picker DIRECTLY with the library's supported sizing API (containerHeight={220} — the full five 44pt rows, same device-proven sizing as the app's TimeSheet) and the picker-specific style keys (time_selector_label / time_label / time_selected_indicator), 28pt labels. The same shared MealTypeTimeWheel is used by the dedicated Default Time sheet AND the inline Create flow, so both surfaces are fixed by the one change. - Reuse: MealTypeTimeWheel now imports the SAME DateType→Date→HH:MM conversion helpers from TimeSheet (dateTypeToDate / timeStringToDate / dateToTimeString exported; behavior identical, existing callers unchanged) — no second drifting conversion implementation. BUG B — dragging moved only the active row; siblings did not shift: - Root cause: CustomMealTypeRow returned `translateY: 0, scale: 1` for every non-active row and system rows were plain static Views; the drop target was computed only in onEnd, so there was no live preview. - Fix (preview layer only; persistence/worker architecture untouched): - NEW shared pure worklet helper computeReorderPreviewShift(rowIndex, activeIndex, targetIndex, stride) → -stride/0/+stride, extracted into WorkoutReorderList and now used by BOTH lists (one algorithm). - LIVE UI-thread targetIndex = useDerivedValue(...) recomputed from panY every frame (WorkoutReorderList pattern); the gesture's onEnd reads the same value so the committed destination always matches the previewed gap. - NEW shared useMealTypeRowDragPreviewStyle hook drives BOTH row kinds: active row floats (translate + 1.02 scale + lift shadow); every other row — custom siblings AND system anchors — springs one stride toward the origin while between active and target (damping 44 / stiffness 960, same spring as WorkoutReorderList). The list closes the old gap and opens the new one naturally; no stationary hole at the source. - System rows are now Animated shells that may VISUALLY shift as passive siblings, but have NO GestureDetector, NO drag handle and NO adjustable actions — they can never become active or be persisted; system sort_order (10/20/30/40) is still never written (doPersist only ever writes custom records; existing full-gap rejection + worker coalescing + post-render commit handoff unchanged). - Commit handoff preserved: committingTranslate + pendingDragResetRef keep the preview frozen until the new unifiedRows render — no snap-back, no one-frame jump. New tests (fail against the previous implementation): - MealTypeTimeWheel.test.tsx: picker rendered directly with supported sizing props (timePicker/initialView/hideHeader/use12Hours/ containerHeight=220), no transform scale on the wrapper, 17:30 seeds correctly, unset seeds current visible time, Date→HH:MM conversion, empty payload ignored, picker-specific style keys. - reorderDrag.test.ts: exhaustive computeReorderPreviewShift coverage (downward/upward/anchor-crossing/stride) and computeReorderTargetIndex live-target progression (down, up, cross-anchor insertion, cross-gap reverse) — the pure helpers the components use in useAnimatedStyle. - MealTypeSettingsScreen.test.tsx: dedicated sheet + Create wheel pass containerHeight/date/use12Hours; unset Save commits visible time; wheel change → pending → Save commits changed HH:MM; Create wheel change updates submitted default_time; backdrop dismiss never commits and clears pending; preview-hook harness (active float, sibling shift down/ up, anchor passive shift, commit handoff no-snap, idle); system rows have no drag handle/reorder actions; accessibility Move up/down uses the same reorder semantics. Validation: - Targeted + regression suites (MealTypeSettingsScreen, mealTypeSlots, FoodSummary, mealNutrition, CopyMealSheet, MealTypeDetail, FoodSearch, FoodSettings, concurrency, WorkoutReorderList + workout reorder tests): all green (WorkoutReorderList refactor verified by its own suite). - Full Jest 288 suites / 4853 tests, 0 failed (was 286/4824). - tsc --noEmit clean, expo lint --max-warnings 0 clean, git diff --check clean. Assisted-by: Open WebUI
|
Physical Android testing found two interaction issues that weren't caught by the automated coverage: the Default Time bottom sheet opened with an empty picker area, and drag-and-drop moved only the active custom row without shifting neighboring rows to preview the insertion gap. I've corrected both using the app's existing interaction patterns (
New coverage: I'll build and re-test a fresh APK from this HEAD before marking the mobile checkbox complete or attaching final screenshots. |
|
@coderabbitai review Fresh review requested for |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="e108e224"
echo '=== Commit metadata and changed files ==='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
echo '=== Focused AST outlines ==='
for file in \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/reorderDrag.ts \
SparkyFitnessMobile/src/components/TimeSheet.tsx \
SparkyFitnessMobile/src/utils/mealTypeSlots.ts \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
SparkyFitnessMobile/__tests__/utils/reorderDrag.test.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
done
echo '=== Commit diff ==='
git diff --unified=100 "${commit}^" "$commit" -- \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/reorderDrag.ts \
SparkyFitnessMobile/src/components/TimeSheet.tsx \
SparkyFitnessMobile/src/utils/mealTypeSlots.ts \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
SparkyFitnessMobile/__tests__/utils/reorderDrag.test.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== Time wheel and established TimeSheet usage ==='
rg -n -C8 \
'MealTypeTimeWheel|containerHeight|DateType|react-native-ui-datepicker|onChange|onValueChange|transform|scale' \
SparkyFitnessMobile/src -g '*.ts' -g '*.tsx' | head -700
echo '=== Drag preview and persistence paths ==='
rg -n -C10 \
'computeReorderPreviewShift|useMealTypeRowDragPreviewStyle|targetIndex|useDerivedValue|useAnimatedStyle|handleDrop|persist|latestDesiredOrderRef|workerRunningRef|updateMealType' \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils \
-g '*.ts' -g '*.tsx'
echo '=== Numbered implementation and test slices ==='
nl -ba SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx | sed -n '1,300p'
nl -ba SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx | sed -n '1,900p'
if [ -f SparkyFitnessMobile/src/utils/reorderDrag.ts ]; then
nl -ba SparkyFitnessMobile/src/utils/reorderDrag.ts | sed -n '1,420p'
fi
sed -n '1,420p' SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx 2>/dev/null || true
sed -n '1,520p' SparkyFitnessMobile/__tests__/utils/reorderDrag.test.ts 2>/dev/null || trueLength of output: 239371 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== WorkoutReorderList complete implementation ==='
sed -n '1,520p' SparkyFitnessMobile/src/components/WorkoutReorderList.tsx
echo '=== Meal type drag target, move, and post-render handoff ==='
sed -n '650,920p' SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
echo '=== Meal type slot derivation and target constraints ==='
sed -n '1,300p' SparkyFitnessMobile/src/utils/mealTypeSlots.ts
echo '=== Exact helper references and definitions ==='
rg -n -C12 \
'export function computeReorderTargetIndex|export function computeReorderPreviewShift|function computeReorderTargetIndex|function computeReorderPreviewShift|moveCustomType|pendingDragResetRef|deriveGapsFromUnified' \
SparkyFitnessMobile/src/components/WorkoutReorderList.tsx \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/mealTypeSlots.ts
echo '=== Diff for the shared reorder helper only ==='
git diff --unified=70 e108e224^ e108e224 -- \
SparkyFitnessMobile/src/components/WorkoutReorderList.tsx \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx
echo '=== Focused tests for helper and screen drag behavior ==='
sed -n '1,260p' SparkyFitnessMobile/__tests__/utils/reorderDrag.test.ts
rg -n -C10 \
'drag|preview|targetIndex|full gap|No more meal types|handoff|accessibilityAction' \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/components/WorkoutReorderList.test.tsx \
SparkyFitnessMobile/__tests__ \
-g '*.test.ts' -g '*.test.tsx' | head -800Length of output: 203557
You are interacting with an AI system. |
…fix test) CI Mobile Tests lint step flagged the unused updateSpy in the new Create wheel test; assert the quick-log follow-up update instead, which also strengthens the coverage (create payload default_time from the wheel + follow-up show_in_quick_log:false for the new type). ESLint (direct) clean, screen suite 51/51 green. Assisted-by: Open WebUI
|
@coderabbitai review HEAD moved to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="e108e224"
head="b7935b00"
echo '=== Commits since the previous device-bugfix review ==='
git log --oneline "${base}..${head}"
echo '=== Final HEAD metadata and changed files ==='
git show --no-ext-diff --format=fuller --stat "${head}"
git diff-tree --no-commit-id --name-status -r "${head}"
echo '=== Full unreviewed diff ==='
git diff --unified=100 "${base}" "${head}" -- \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/reorderDrag.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx
echo '=== Drag rejection, reset, and handoff paths ==='
rg -n -C10 \
'full gap|No more meal types|pendingDragResetRef|committingTranslate|activeDragIndex|panY|onEnd|onMove|moveCustomType|handleDrop|reset' \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/reorderDrag.ts \
-g '*.ts' -g '*.tsx'
echo '=== Numbered focused implementation ==='
nl -ba SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx | sed -n '600,940p'
echo '=== Relevant regression tests ==='
rg -n -C12 \
'full gap|No more meal types|drag|preview|idle|reset|Create|quick.log|wheel' \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
-g '*.test.ts' -g '*.test.tsx'Length of output: 47144
You are interacting with an AI system. |
CodeRabbit P1 on the device-bugfix HEAD: the gesture's onEnd freezes the drag preview (committingTranslate = panY, activeDragIndex stays) BEFORE moveCustomType runs; when the destination gap is full, moveCustomType toasts and returned early WITHOUT arming the post-render reset, so the dropped row (and now, with sibling-shift preview, every shifted row) stayed stuck translated forever. Fix: - New exported resetMealTypeDragPreview(activeDragIndex, panY, committingTranslate) clears the three shared values (identity preview). - moveCustomType's full-gap rejection path (and the defensive anchor-bound guard) call it, so a rejected drop springs the preview back to idle. - The accepted-move post-render commit handoff now uses the same helper (identical assignments, one implementation). Regression test 'full-gap drop rejection releases the frozen drag preview': screen-level toast + no-write assertions, then the exact frozen-preview style (translateY 129) -> resetMealTypeDragPreview -> idle identity style. fireEvent runs BEFORE the hook-harness renders (multiple RTL render() calls disconnect earlier trees from event dispatch). Full Jest 288 suites / 4854 tests, tsc/eslint/diff-check clean. Assisted-by: Open WebUI
|
@coderabbitai review HEAD moved to |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="2c201410"
previous="b7935b00"
echo '=== Commit metadata and changed files ==='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
echo '=== Unreviewed diff ==='
git diff --unified=100 "$previous" "$commit" -- \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/reorderDrag.ts \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/utils/reorderDrag.test.ts
echo '=== Drag implementation outline ==='
ast-grep outline SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
--items all --view expanded
echo '=== Rejection and reset paths ==='
rg -n -C12 \
'resetMealTypeDragPreview|No more meal types|full gap|pendingDragResetRef|committingTranslate|activeDragIndex|panY|moveCustomType|onEnd|onMove' \
SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx \
SparkyFitnessMobile/src/utils/reorderDrag.ts \
-g '*.ts' -g '*.tsx'
echo '=== Numbered implementation context ==='
nl -ba SparkyFitnessMobile/src/screens/MealTypeSettingsScreen.tsx | sed -n '680,940p'
echo '=== Targeted regression tests ==='
rg -n -C12 \
'full gap|No more meal types|frozen|reset|idle|preview|no-write|drag' \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx \
SparkyFitnessMobile/__tests__/utils/reorderDrag.test.ts \
-g '*.test.ts' -g '*.test.tsx'Length of output: 85234
The regression test verifies the error toast, no reorder write, the frozen preview state, and the reset to the idle transform. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
SECOND physical Android pass (APK from 2c20141): DnD sibling movement is confirmed working, but the Default Time bottom sheet STILL opened with an empty region where the hour/minute wheels must be — so the earlier transform-scale diagnosis was falsified by the device re-test. Actual root cause, from inspecting the installed react-native-ui-datepicker 3.1.2 source (not props/Jest mocks): - The time view is a horizontal ScrollView (scrollEnabled:false) around a `timePickerContainer` HARDCODED to width/height 150 (CONTAINER_HEIGHT/2), holding two `flex:1` wheel columns. The vertical wheel is an `Animated.FlatList` and the selected indicator is `width:'100%'` — every horizontal pixel is inherited from the PARENT width. If the parent does not stretch the picker, the columns collapse and the wheel is invisible. - `containerHeight` is applied only as `height` on the outer Calendar wrapper around the active view; it does NOT size the time box (which is the fixed 150 from the CONTAINER_HEIGHT enum). The previous fix leaned on it as "the mechanism that sizes the wheel" — that comment was wrong. Why it stayed blank after the transform removal: MealTypeTimeWheel still wrapped the picker in `<View style={{ height, alignItems:'center', justifyContent:'center' }}>`. `alignItems:'center'` stops the picker root from stretching, so the wheel columns (flex:1 children inheriting parent width) collapse to ~0 width → empty region on device. Fix (wheel layout only — DnD untouched): - MealTypeTimeWheel now owns an explicit FULL-WIDTH contract on its root: `width:'100%'`, `alignSelf:'stretch'`, own fixed height; `alignItems: 'center'`/`justifyContent:'center'` removed. It also passes `style={{ width:'100%', alignSelf:'stretch' }}` to the picker so the Calendar root stretches — the same effective layout as the app's device-proven TimeSheet (picker rendered directly under a full-width BottomSheetView). - 24-hour presentation: `use12Hours` removed (hours 00–23 / minutes 00–59, no AM/PM third column) per the maintainer mockup. Persisted value stays canonical HH:MM. - Dedicated sheet renders the shared wheel DIRECTLY under BottomSheetView; the redundant fixed-height `<View style={{ height: TIME_WHEEL_WRAPPER_HEIGHT }}>` wrapper is removed — MealTypeTimeWheel is the single owner of its own dimensions (mirrors TimeSheet's structure). - Stale comments corrected: no more "scaled 1.8× in a 280pt wrapper" and no claim that containerHeight sizes the wheel. Tests strengthened (the old "picker exists + containerHeight + timePicker" coverage passed while the device was still blank): - MealTypeTimeWheel.test.tsx: full-width stretch root (width '100%', alignSelf 'stretch'), no alignItems:'center' shrink wrapper, no transform, picker style width '100%', use12Hours falsy, 17:30 + 23:59 seeding, unset current time, hour + minute change, empty payload ignored, 28pt style keys. - MealTypeSettingsScreen.test.tsx: dedicated sheet + Create assert the full-width/24h contract; dedicated sheet asserts no redundant height wrapper around the shared wheel. - DnD tests (reorderDrag, WorkoutReorderList) untouched and green. Validation: targeted + regression suites green; full Jest 288 suites / 4859 tests, 0 failed; tsc --noEmit clean; eslint --max-warnings 0 clean; git diff --check clean. Assisted-by: Open WebUI
|
@coderabbitai review Fresh review requested for A fresh APK from Fix (wheel layout only): shared |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current HEAD and recent commits ==='
git log --oneline -12
git show --no-ext-diff --format=fuller --stat HEAD
git diff-tree --no-commit-id --name-status -r HEAD
echo '=== Focused outlines ==='
for file in \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx \
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx \
SparkyFitnessMobile/src/components/TimeSheet.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimePickerSheet.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeFormSheet.test.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
done
echo '=== Current-head focused diff ==='
git diff --unified=100 HEAD^ HEAD -- \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx \
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx \
SparkyFitnessMobile/src/components/TimeSheet.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimePickerSheet.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeFormSheet.test.tsx
echo '=== Wheel implementation and call sites ==='
nl -ba SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx | sed -n '1,340p'
nl -ba SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx | sed -n '1,360p'
nl -ba SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx | sed -n '1,520p'
rg -n -C6 \
'MealTypeTimeWheel|react-native-ui-datepicker|containerHeight|timePicker|is24Hour|DateType|defaultTime' \
SparkyFitnessMobile/src SparkyFitnessMobile/__tests__ \
-g '*.ts' -g '*.tsx' | head -1000
echo '=== Focused tests ==='
for file in \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimePickerSheet.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeFormSheet.test.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
sed -n '1,620p' "$file"
fi
done
echo '=== Package version and installed declaration/source availability ==='
rg -n -C3 'react-native-ui-datepicker' SparkyFitnessMobile/package.json SparkyFitnessMobile -g 'package.json' -g '*.ts' -g '*.tsx' | head -220
find SparkyFitnessMobile -path '*react-native-ui-datepicker*' -maxdepth 5 -type f 2>/dev/null | head -80Length of output: 30632 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Complete current diff ==='
git diff --unified=120 HEAD^ HEAD -- \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== Shared wheel ==='
sed -n '1,280p' SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx
echo '=== Dedicated sheet ==='
sed -n '1,360p' SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx
echo '=== Create form and established TimeSheet ==='
sed -n '1,620p' SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx
sed -n '1,440p' SparkyFitnessMobile/src/components/TimeSheet.tsx
echo '=== All wheel usage and surrounding render contexts ==='
rg -n -C12 '<MealTypeTimeWheel|TIME_WHEEL_|MealTypeTimePickerSheet|timePicker|use12Hours' \
SparkyFitnessMobile/src SparkyFitnessMobile/__tests__ \
-g '*.ts' -g '*.tsx' | head -1200
echo '=== Targeted test files ==='
sed -n '1,400p' SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx
sed -n '1,2200p' SparkyFitnessMobile/__tests__/screens/MealTypeSettingsScreen.test.tsx
echo '=== Dependency source or declarations, if checked out ==='
fd -HI 'react-native-ui-datepicker' . -t d -t f | head -100
fd -HI 'DateTimePicker.*' . -t f | rg 'react-native-ui-datepicker|datepicker' | head -100
echo '=== Full working tree state ==='
git status --shortLength of output: 250243
I found no new correctness issues.
The dedicated sheet and the Create flow use the same shared wheel. The sheet no longer adds a second height wrapper. The wheel keeps canonical The added tests cover the layout contract, 24-hour configuration, seeding, changes, and both wheel surfaces. Device rendering remains dependent on the planned APK re-test. You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="eb86c947"
previous="2c201410"
echo '=== Commit metadata and changed files ==='
git show --no-ext-diff --format=fuller --stat "$commit"
git diff-tree --no-commit-id --name-status -r "$commit"
echo '=== Unreviewed diff ==='
git diff --unified=100 "$previous" "$commit" -- \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx \
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx \
SparkyFitnessMobile/src/components/TimeSheet.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimePickerSheet.test.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeFormSheet.test.tsx
echo '=== Focused outlines ==='
for file in \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx \
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx \
SparkyFitnessMobile/src/components/TimeSheet.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
done
echo '=== Full shared-wheel and consumer implementations ==='
sed -n '1,320p' SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx
sed -n '1,360p' SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx
sed -n '1,460p' SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx
sed -n '1,340p' SparkyFitnessMobile/src/components/TimeSheet.tsx
echo '=== All shared wheel call sites and picker props ==='
rg -n -C7 \
'MealTypeTimeWheel|DateTimePicker|containerHeight|use12Hours|style=\{|styles=\{' \
SparkyFitnessMobile/src SparkyFitnessMobile/__tests__ \
-g '*.ts' -g '*.tsx'
echo '=== Dependency version and installed picker declarations/source ==='
rg -n -C3 '"react-native-ui-datepicker"' SparkyFitnessMobile/package.json SparkyFitnessMobile/package-lock.json SparkyFitnessMobile/yarn.lock 2>/dev/null || true
fd -HI 'react-native-ui-datepicker' SparkyFitnessMobile/node_modules 2>/dev/null | head -30 || true
fd -HI . SparkyFitnessMobile/node_modules/react-native-ui-datepicker 2>/dev/null | head -80 || true
rg -n -C5 \
'containerHeight|use12Hours|timePickerContainer|timePicker' \
SparkyFitnessMobile/node_modules/react-native-ui-datepicker 2>/dev/null | head -500 || true
echo '=== Targeted tests ==='
sed -n '1,420p' SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx 2>/dev/null || true
sed -n '1,440p' SparkyFitnessMobile/__tests__/components/MealTypeTimePickerSheet.test.tsx 2>/dev/null || true
sed -n '1,520p' SparkyFitnessMobile/__tests__/components/MealTypeFormSheet.test.tsx 2>/dev/null || trueLength of output: 203248 🏁 Script executed: #!/bin/bash
set -euo pipefail
commit="eb86c947"
previous="2c201410"
echo '=== Exact changed-file diff ==='
git diff --unified=120 "$previous" "$commit" -- \
SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx \
SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx \
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx
echo '=== Current shared wheel ==='
nl -ba SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx | sed -n '1,280p'
echo '=== Current dedicated sheet consumer ==='
nl -ba SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx | sed -n '1,360p'
echo '=== Current inline-create consumer sections ==='
rg -n -C12 'MealTypeTimeWheel|defaultTime|default_time|TIME_WHEEL' \
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx
echo '=== Established TimeSheet picker layout ==='
rg -n -C16 'DateTimePicker|containerHeight|timePicker|style=\{|styles=\{' \
SparkyFitnessMobile/src/components/TimeSheet.tsx
echo '=== Installed picker source and declaration files ==='
fd -HI -t f . SparkyFitnessMobile/node_modules/react-native-ui-datepicker 2>/dev/null | \
rg '(index|types|d\.ts|Time|time)' | head -100
rg -n -C8 \
'interface.*DateTimePicker|type.*DateTimePicker|containerHeight|use12Hours|timePickerContainer|time_label|time_selected_indicator' \
SparkyFitnessMobile/node_modules/react-native-ui-datepicker 2>/dev/null | head -600
echo '=== Test and mock contract ==='
rg -n -C8 \
'react-native-ui-datepicker|date-picker|DateTimePicker' \
SparkyFitnessMobile/jest.setup.js \
SparkyFitnessMobile/__tests__/components/MealTypeTimeWheel.test.tsx \
-g '*.js' -g '*.ts' -g '*.tsx'Length of output: 23244 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Inline-create wheel consumer ==='
rg -n -C18 'MealTypeTimeWheel|defaultTime|default_time' \
SparkyFitnessMobile/src/components/MealTypeFormSheet.tsx
echo '=== Dedicated sheet current implementation ==='
sed -n '1,260p' SparkyFitnessMobile/src/components/MealTypeTimePickerSheet.tsx
echo '=== Shared wheel current implementation ==='
sed -n '1,260p' SparkyFitnessMobile/src/components/MealTypeTimeWheel.tsx
echo '=== Date picker mocks ==='
rg -n -C10 \
'react-native-ui-datepicker|date-picker|DateTimePicker' \
SparkyFitnessMobile \
-g 'jest.setup.js' -g '*.test.ts' -g '*.test.tsx' -g '*.js' | head -450
echo '=== Other direct date-picker layouts and all shared-wheel wrappers ==='
rg -n -C12 \
'<DateTimePicker|<MealTypeTimeWheel|TIME_WHEEL_WRAPPER_HEIGHT|alignItems:\s*.\x27center\x27|alignItems:\s*"center"' \
SparkyFitnessMobile/src \
-g '*.tsx'
echo '=== Component-specific test coverage ==='
fd -t f -a 'MealType*Time*' SparkyFitnessMobile/__tests__ -x sh -c \
'echo "--- $1 ---"; sed -n "1,420p" "$1"' sh {}Length of output: 111632
The change also preserves canonical The Jest mock verifies the prop contract only. A fresh Android build remains necessary to verify the native picker rendering for this exact commit. You are interacting with an AI system. |
|
@apedley I used your mockups and create a new version of custom meals. Screenshots from real device attached into PR description. |







Description
What problem does this PR solve?
The mobile app lacked full support for the backend's custom meal types (create/edit/delete, per-user quick-log and default-time suggestions). This PR implements the maintainer's final design: one unified meal-type list with fixed system anchors and drag-and-drop custom ordering between them, using the app's established patterns.
How did you implement the solution?
WorkoutReorderList. Crossed rows spring aside as the target changes; system meal types can participate only as passive visual siblings during the preview and remain fixed/non-draggable anchors whose sort orders are never rewritten. Accessible Move up/down use the same algorithm; destination is clamped between the anchors and a drop into a full gap is rejected with one concise toast — no partial writes.is_visible, using the app's themedcomponents/ui/Switch(no default iOS green, no hardcoded tints); semantic labelVisible <Name>; toggling persistsis_visible. No duplicate Visibility in the edit/create sheets (mockup placement wins). Hidden types stay excluded from NEW-entry selection while historical Diary entries remain readable.Not setorHH:MM) with a large invisible hit target — no nested pill/card, no timer icon, no chevron; the row stays one clean settings row.TimeSheet/react-native-ui-datepickerlayout pattern and renders the picker directly with the library-supportedcontainerHeight={220}rather than transform scaling. The same visible wheel is used in the dedicated Default Time sheet and inline Create flow, with readable 28pt time labels. Save commits canonicalHH:MM, Clear commitsnull, and dismissing without Save/Clear makes no change.is_visibleso a plain name/time/quick-log edit never overwrites server visibility state.MEAL_CONFIG(single lookup);SYSTEM_LABELS, the duplicate icon switch, dead helpers (filterFoodEntriesByMealType,getFoodEntryMealTypeKey,getMealTypeDisplayLabelForName) and redundantMealGroup.user_idare gone.mealTypeIdsurvives MealTypeDetail → search → barcode/photo → entry.Linked Issue: Closes #1981 · Closes #2062 · Related to #1923 · Related to #1979
How to Test
cd SparkyFitnessMobile && pnpm installnpx jest --runInBand __tests__/screens/MealTypeSettingsScreen.test.tsx __tests__/utils/mealTypeSlots.test.ts __tests__/components/FoodSummary.test.tsx __tests__/utils/mealNutrition.test.ts __tests__/components/CopyMealSheet.test.tsx __tests__/screens/MealTypeDetailScreen.test.tsx __tests__/screens/FoodSearchScreen.test.tsx __tests__/screens/FoodSettingsScreen.test.tsxNot setor an existing time and confirm the Default Time sheet shows a visible scrollable wheel (Save persists the selected time, Clear removes it, dismiss without Save/Clear leaves it unchanged); in Create Meal Type confirm the inline wheel is visible and the selected time is saved on Create.PR Type
Checklist
All PRs:
New features only:
Mobile changes (
SparkyFitnessMobile/):4cb5d759/2c201410lineage) found the invisible Default Time wheel and missing sibling movement during drag preview; the DnD sibling shift is now PHYSICALLY VERIFIED working from the APK built at2c201410. Pass 2 found the Default Time wheel STILL invisible at2c201410, which was fixed ineb86c947. The checkbox remains unchecked until a fresh APK fromeb86c947is installed and the picker is re-verified on the device.)Screenshots
Screenshots
Notes for Reviewers
tsc --noEmit,expo lint --max-warnings 0,git diff --check, CI).WorkoutReorderListgesture infrastructure (no new dependency); the time picker reuses the app's existingreact-native-ui-datepickertime-wheel mechanism.WorkoutReorderListsibling-shift pattern and is PHYSICALLY VERIFIED working. Pass 2 showed the picker was STILL blank — root cause found in the library source: the wheel columns are flex:1 children that inherit all width from the parent, so the shared wheel's center-shrink wrapper collapsed them. The wheel now owns a full-width stretch contract (mirrors the app's device-provenTimeSheet) and uses 24-hour presentation. A fresh Android APK re-test (eb86c947) is still pending before the device checkbox and screenshots are finalized.Summary by CodeRabbit
New Features
Bug Fixes