feat(mobile): support custom measurement entries - #2061
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 custom measurement APIs, React Query hooks, typed form utilities, editable Daily measurement controls, diary integration, manual-entry filtering, health-sync category handling, and comprehensive tests. ChangesCustom measurement workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MeasurementsAddScreen
participant useCustomMeasurements
participant measurementsApi
participant QueryCache
User->>MeasurementsAddScreen: edit custom measurement
MeasurementsAddScreen->>useCustomMeasurements: save or delete entry
useCustomMeasurements->>measurementsApi: send mutation request
measurementsApi-->>useCustomMeasurements: return mutation result
useCustomMeasurements->>QueryCache: invalidate date query and refresh health-sync cache
QueryCache-->>MeasurementsAddScreen: provide updated custom data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
|
Assisted-by: Open WebUI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
SparkyFitnessMobile/__tests__/components/MeasurementsSummary.test.tsx (1)
102-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the mock component props.
Lines 106 and 169 introduce
anyin new function parameters. Define the minimal prop types instead.Proposed fix
- default: ({ title }: any) => ( + default: ({ title }: { title?: string }) => ( ... - default: ({ name }: any) => <View testID={`icon-${name}`} />, + default: ({ name }: { name: string }) => <View testID={`icon-${name}`} />,Also applies to: 165-170
🤖 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__/components/MeasurementsSummary.test.tsx` around lines 102 - 112, Replace the any-typed mock component parameters in the MeasurementsSummary tests with minimal explicit prop types, covering the rendered children and any props the mock uses. Update both mock component definitions around the affected tests while preserving their existing behavior.Source: Coding guidelines
SparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.ts (1)
843-884: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a Daily tombstone case to this suite.
Both tests in this block use
numericCat('Hourly'), so they exercisesyncMultiEntryonly. No test covers a tombstoned single-entry category, which routes throughsyncSingleEntry. That gap hides the missing resurrection guard reported onSparkyFitnessMobile/src/utils/customMeasurementsForm.tslines 224-236.Add a case that passes
numericCat('Daily')withdeleted: [{ entryId: 'e1' }]and a server response that still returnse1. Assert thatrowsis empty and that the tombstone survives.🤖 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/customMeasurementsForm.test.ts` around lines 843 - 884, Add a Daily-category tombstone resurrection test in the syncCustomForm suite using numericCat('Daily'), a deleted e1 tombstone, and a server response still containing e1; assert the resulting rows remain empty and the deleted list retains e1, covering the syncSingleEntry path.SparkyFitnessMobile/__tests__/hooks/useCustomMeasurements.test.ts (1)
135-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding failure-path coverage for both mutations.
Both tests cover the success path only. The
onErrorhandlers inuseSaveCustomMeasurementanduseDeleteCustomMeasurementlog throughaddLogand are never exercised. Add one rejected-promise test per mutation. Assert thatmutateAsyncrejects and that no invalidation runs for the date key. This protects the partial-failure handling thatMeasurementsAddScreendepends on.🤖 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__/hooks/useCustomMeasurements.test.ts` around lines 135 - 188, Add rejected-promise tests for both useSaveCustomMeasurement and useDeleteCustomMeasurement. Mock each mutation API to reject, assert mutateAsync rejects, verify addLog is called through the onError path, and confirm invalidateQueries is not called for the affected date key.SparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsx (5)
648-661: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact hour rather than any number.
The test presses
hour-plus-new-1once and then assertsexpect.any(Number). That passes for every hour value, including a value the stepper never produced. The stepper starts at the current local hour and increments with wraparound, so the expected value is(new Date().getHours() + 1) % 24. Pin the clock and assert the exact result.💚 Proposed assertion
test('hourly category adds a row and saves with the chosen hour', async () => { setCustomCategories([customCategory({ id: 'c1', frequency: 'Hourly' })]); + jest.useFakeTimers().setSystemTime(new Date(2024, 5, 15, 9, 30)); const screen = renderScreen(); fireEvent.press(screen.getByTestId('add-custom-c1')); fireEvent.press(screen.getByTestId('hour-plus-new-1')); fireEvent.changeText(screen.getByTestId('custom-input-new-1'), '10'); await pressSave(screen); const payload = savedCustomPayload(); expect(payload.category_id).toBe('c1'); expect(payload.value).toBe(10); - expect(payload.entry_hour).toEqual(expect.any(Number)); + expect(payload.entry_hour).toBe(10); + jest.useRealTimers(); });Confirm that fake timers do not interfere with the
await actflush used bypressSave.🤖 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/MeasurementsAddScreen.test.tsx` around lines 648 - 661, Update the hourly category test around pressSave to pin the system clock, then assert payload.entry_hour equals (new Date().getHours() + 1) % 24 after pressing hour-plus-new-1 once. Ensure the clock setup and cleanup do not interfere with the await act flush performed by pressSave, and restore the real clock after the test.
444-477: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the duplicated mock setup into a shared helper.
This
beforeEachrepeats the custom-hook and upsert mock configuration from the first describe block at lines 228-252. The two copies must stay in sync by hand. A shared helper removes the duplication.♻️ Proposed refactor
+const setupDefaultMocks = () => { + mockUseUpsertCheckIn.mockReturnValue({ + mutate, + mutateAsync, + isPending: false, + } as unknown as ReturnType<typeof useUpsertCheckIn>); + setCustomCategories([]); + setCustomEntries([]); + mockUseSaveCustomMeasurement.mockReturnValue({ + mutate: jest.fn(), + mutateAsync: jest.fn().mockResolvedValue(undefined), + isPending: false, + } as unknown as ReturnType<typeof useSaveCustomMeasurement>); + mockUseDeleteCustomMeasurement.mockReturnValue({ + mutate: jest.fn(), + mutateAsync: jest.fn().mockResolvedValue(undefined), + isPending: false, + } as unknown as ReturnType<typeof useDeleteCustomMeasurement>); +};Then call
setupDefaultMocks()from bothbeforeEachblocks.🤖 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/MeasurementsAddScreen.test.tsx` around lines 444 - 477, Extract the duplicated custom-hook and upsert mock configuration from the two describe blocks into a shared setupDefaultMocks() helper. Replace the repeated setup in both beforeEach blocks with calls to setupDefaultMocks(), while keeping each block’s distinct state and Alert setup unchanged.
495-537: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo tests do not assert the behavior their names describe.
Line 495: the name states "in API order", but the test only checks that both labels exist. It passes if the screen reverses the order.
Line 523: the name states "prefills boolean true and false distinctly", but the test only counts the
YesandNolabels. It never checks which option is selected. The tri-state contract —trueselects Yes,falseselects No,''selects nothing — stays untested. That contract is the main reasonCustomBooleanControlexists.💚 Proposed assertions
test('renders custom categories in API order with literal names and units', () => { setCustomCategories([ customCategory({ id: 'c1', name: 'Stres', display_name: null, measurement_type: 'mmHg' }), customCategory({ id: 'c2', name: 'Energy', display_name: 'Energy Level', measurement_type: '' }), ]); const screen = renderScreen(); expect(screen.getByText('Custom Measurements')).toBeTruthy(); // User names stay literal — never translated or reordered. expect(screen.getByText('Stres (mmHg)')).toBeTruthy(); expect(screen.getByText('Energy Level')).toBeTruthy(); + const labels = screen + .getAllByText(/Stres \(mmHg\)|Energy Level/) + .map((node) => node.props.children.join?.('') ?? node.props.children); + expect(labels[0]).toContain('Stres'); }); @@ const screen = renderScreen(); - // Both yes/no pairs render; the false entry is a real saved value. - expect(screen.getAllByText('Yes')).toHaveLength(2); - expect(screen.getAllByText('No')).toHaveLength(2); + // c1 holds 'true' -> Yes selected; c2 holds 'false' -> No selected. + const yesOptions = screen.getAllByText('Yes'); + const noOptions = screen.getAllByText('No'); + expect(yesOptions).toHaveLength(2); + expect(noOptions).toHaveLength(2); + expect(yesOptions[0].parent?.parent?.props.accessibilityState.selected).toBe(true); + expect(noOptions[0].parent?.parent?.props.accessibilityState.selected).toBe(false); + expect(yesOptions[1].parent?.parent?.props.accessibilityState.selected).toBe(false); + expect(noOptions[1].parent?.parent?.props.accessibilityState.selected).toBe(true); });The
parent?.parenttraversal matches the pattern already used at line 716. Consider atestIDon the optionTouchableOpacityinCustomBooleanControlto make these lookups less brittle.🤖 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/MeasurementsAddScreen.test.tsx` around lines 495 - 537, Strengthen the tests named “renders custom categories in API order with literal names and units” and “prefills boolean true and false distinctly via tri-state control.” Assert the rendered custom category labels in their API order rather than only checking presence, and verify the boolean controls’ selected state so true selects Yes, false selects No, and empty values select neither. Add stable option testIDs in CustomBooleanControl if needed, following the existing parent traversal pattern.
119-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
any[]parameters with the custom measurement types.Lines 119 and 128 declare
any[]parameters on two new helper functions. The coding guidelines prohibitanyin new code. This PR addsSparkyFitnessMobile/src/types/customMeasurements.ts, which exports the matching contracts, so a typed parameter is available at no cost.The retrieved learning about
as anycovers partial hook return values used as focused fixtures. These are parameter annotations on new helpers, so the exemption does not apply.♻️ Proposed fix
+import type { + CustomCategory, + CustomMeasurementEntry, +} from '../../src/types/customMeasurements'; + -const setCustomCategories = (categories: any[]) => { +const setCustomCategories = (categories: Partial<CustomCategory>[]) => { mockUseCustomCategories.mockReturnValue({ data: categories, isLoading: false, isError: false, refetch: jest.fn(), } as unknown as ReturnType<typeof useCustomCategories>); }; -const setCustomEntries = (entries: any[]) => { +const setCustomEntries = (entries: Partial<CustomMeasurementEntry>[]) => { mockUseCustomMeasurementsByDate.mockReturnValue({ data: entries, isLoading: false, isError: false, refetch: jest.fn(), } as unknown as ReturnType<typeof useCustomMeasurementsByDate>); };Confirm the exported type names in
SparkyFitnessMobile/src/types/customMeasurements.tsbefore applying.As per coding guidelines: "Never use
anyor disable@typescript-eslint/no-explicit-anyin new functions or edited code; define explicit TypeScript types or import schemas from@workspace/shared."🤖 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/MeasurementsAddScreen.test.tsx` around lines 119 - 135, Replace the any[] parameters in setCustomCategories and setCustomEntries with the corresponding exported custom measurement types from src/types/customMeasurements.ts. Confirm and import the exact type names, keeping the existing mock hook return setup unchanged.Sources: Coding guidelines, Learnings
585-646: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReuse
confirmClearAlert()in both custom-clear tests. This removes duplicated alert-confirmation logic.🤖 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/MeasurementsAddScreen.test.tsx` around lines 585 - 646, Extract the repeated alert-button lookup and confirmation act flow into the existing confirmClearAlert() helper, then call it from both “clears an existing custom value through delete after confirmation” and “deleting one category does not affect another category value” tests. Preserve each test’s existing assertions and behavior.SparkyFitnessMobile/__tests__/services/customMeasurementsApi.test.ts (1)
98-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge the two duplicate POST tests and assert the full body.
Both tests carry the same name and the same assertions. Duplicate test names make Jest failure output ambiguous. Neither test verifies
valueorentry_date, although the name claims it. Parse the serialized body and assert every field.♻️ Proposed consolidation
describe('saveCustomMeasurement', () => { - test('sends POST with category_id, value, entry_date', async () => { + test('sends POST with category_id, value, entry_date', async () => { const savedEntry = { id: 'entry-1', category_id: 'cat-1', value: '75', entry_date: '2024-06-15', }; mockFetch.mockResolvedValue({ ok: true, json: () => Promise.resolve(savedEntry) }); const result = await saveCustomMeasurement({ category_id: 'cat-1', value: 75, entry_date: '2024-06-15', }); + expect(result.id).toBe('entry-1'); expect(result.value).toBe('75'); - expect(mockFetch).toHaveBeenCalledWith( - 'https://example.com/api/measurements/custom-entries', - expect.objectContaining({ - method: 'POST', - body: expect.stringContaining('"category_id":"cat-1"'), - }), - ); - }); - - test('sends POST with category_id, value, entry_date', async () => { - mockFetch.mockResolvedValue({ ok: true, json: () => Promise.resolve({ id: 'e1', category_id: 'cat-1', value: '75', entry_date: '2024-06-15' }) }); - - const result = await saveCustomMeasurement({ category_id: 'cat-1', value: 75, entry_date: '2024-06-15' }); - - expect(result.id).toBe('e1'); - expect(mockFetch).toHaveBeenCalledWith( - 'https://example.com/api/measurements/custom-entries', - expect.objectContaining({ method: 'POST', body: expect.stringContaining('"category_id":"cat-1"') }), - ); + const [url, init] = mockFetch.mock.calls[0]; + expect(url).toBe('https://example.com/api/measurements/custom-entries'); + expect(init.method).toBe('POST'); + expect(JSON.parse(init.body)).toMatchObject({ + category_id: 'cat-1', + value: 75, + entry_date: '2024-06-15', + }); }); });🤖 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__/services/customMeasurementsApi.test.ts` around lines 98 - 132, Merge the duplicate tests in the saveCustomMeasurement describe block into one uniquely named test. Parse the serialized request body passed to mockFetch and assert category_id, value, and entry_date, while preserving the result assertions needed for the saved response.SparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsx (1)
152-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the ref write out of the render phase.
Line 154 writes
customFormRef.currentduring render. React 19 can discard a render in concurrent mode or double-invoke it in Strict Mode. A render-phase ref write is not idiomatic and can store a value from a render that React throws away. The reads occur in effects and callbacks after commit, so current behavior converges. Consider writing the ref in an effect that is declared before the reconciliation effect, so effect ordering keeps the ref fresh.♻️ Proposed refactor
const [customForm, setCustomForm] = useState<CustomFormState>({}); const customFormRef = useRef<CustomFormState>({}); - customFormRef.current = customForm; + useEffect(() => { + customFormRef.current = customForm; + }, [customForm]);Verify that the reconciliation effect still reads the latest committed form after this change.
🤖 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/MeasurementsAddScreen.tsx` around lines 152 - 161, Move the customFormRef.current assignment out of render and into a useEffect declared before the reconciliation effect, updating it whenever customForm changes. Ensure reconciliation and callbacks read the latest committed custom form state while preserving existing dirty-row behavior.
🤖 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__/components/MeasurementsSummary.test.tsx`:
- Around line 21-24: Update the MeasurementsSummary test fixtures to satisfy the
CheckInMeasurement prop type: include an entry_date value in the empty-object
test, and pass undefined for custom measurements in all three custom-only tests.
Keep the existing assertions and test behavior unchanged.
In `@SparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.ts`:
- Around line 505-514: Remove the timezone-dependent
iso.startsWith('2026-01-05') assertion from the entryTimestampFor test; retain
the local Date getter assertions for year, month, day, and hour, matching the
approach used by the sibling test.
In `@SparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsx`:
- Around line 654-669: Update isSaveDisabled in MeasurementsAddScreen so
isCustomDataError no longer disables saving standard measurements; retain
loading and mutation guards. Preserve the custom-section error state and its
retry affordance, allowing standard-only saves while custom data is unavailable.
- Around line 607-622: Update the save flow around the partial-failure handler
to track each operation’s success and clear only dirty keys belonging to
completed operations; preserve unsucceeded form values, dirty fields, and custom
rows so refetch plus a second save retries only pending work without duplicating
successful inserts. Extend the save variant of CustomOp with a rowKey in
customMeasurementsForm.ts, and use it to identify succeeded custom operations.
- Around line 746-812: Add accessible names and button roles to the icon-only
TouchableOpacity controls identified by testIDs hour-minus, hour-plus, and
delete-custom. Include the current category label in each accessibilityLabel,
clearly distinguishing decrease hour, increase hour, and delete actions, with
the delete label indicating its destructive purpose.
In `@SparkyFitnessMobile/src/utils/customMeasurementsForm.ts`:
- Around line 224-236: The single-entry synchronization path must honor
tombstoned server entries like syncMultiEntry. In
SparkyFitnessMobile/src/utils/customMeasurementsForm.ts lines 224-236, update
syncSingleEntry to skip server entries whose id appears in prev.deleted; in
SparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.ts lines
843-884, add a numericCat('Daily') case with tombstone e1 and a server response
containing e1, asserting rows is empty and the tombstone remains.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/components/MeasurementsSummary.test.tsx`:
- Around line 102-112: Replace the any-typed mock component parameters in the
MeasurementsSummary tests with minimal explicit prop types, covering the
rendered children and any props the mock uses. Update both mock component
definitions around the affected tests while preserving their existing behavior.
In `@SparkyFitnessMobile/__tests__/hooks/useCustomMeasurements.test.ts`:
- Around line 135-188: Add rejected-promise tests for both
useSaveCustomMeasurement and useDeleteCustomMeasurement. Mock each mutation API
to reject, assert mutateAsync rejects, verify addLog is called through the
onError path, and confirm invalidateQueries is not called for the affected date
key.
In `@SparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsx`:
- Around line 648-661: Update the hourly category test around pressSave to pin
the system clock, then assert payload.entry_hour equals (new Date().getHours() +
1) % 24 after pressing hour-plus-new-1 once. Ensure the clock setup and cleanup
do not interfere with the await act flush performed by pressSave, and restore
the real clock after the test.
- Around line 444-477: Extract the duplicated custom-hook and upsert mock
configuration from the two describe blocks into a shared setupDefaultMocks()
helper. Replace the repeated setup in both beforeEach blocks with calls to
setupDefaultMocks(), while keeping each block’s distinct state and Alert setup
unchanged.
- Around line 495-537: Strengthen the tests named “renders custom categories in
API order with literal names and units” and “prefills boolean true and false
distinctly via tri-state control.” Assert the rendered custom category labels in
their API order rather than only checking presence, and verify the boolean
controls’ selected state so true selects Yes, false selects No, and empty values
select neither. Add stable option testIDs in CustomBooleanControl if needed,
following the existing parent traversal pattern.
- Around line 119-135: Replace the any[] parameters in setCustomCategories and
setCustomEntries with the corresponding exported custom measurement types from
src/types/customMeasurements.ts. Confirm and import the exact type names,
keeping the existing mock hook return setup unchanged.
- Around line 585-646: Extract the repeated alert-button lookup and confirmation
act flow into the existing confirmClearAlert() helper, then call it from both
“clears an existing custom value through delete after confirmation” and
“deleting one category does not affect another category value” tests. Preserve
each test’s existing assertions and behavior.
In `@SparkyFitnessMobile/__tests__/services/customMeasurementsApi.test.ts`:
- Around line 98-132: Merge the duplicate tests in the saveCustomMeasurement
describe block into one uniquely named test. Parse the serialized request body
passed to mockFetch and assert category_id, value, and entry_date, while
preserving the result assertions needed for the saved response.
In `@SparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.ts`:
- Around line 843-884: Add a Daily-category tombstone resurrection test in the
syncCustomForm suite using numericCat('Daily'), a deleted e1 tombstone, and a
server response still containing e1; assert the resulting rows remain empty and
the deleted list retains e1, covering the syncSingleEntry path.
In `@SparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsx`:
- Around line 152-161: Move the customFormRef.current assignment out of render
and into a useEffect declared before the reconciliation effect, updating it
whenever customForm changes. Ensure reconciliation and callbacks read the latest
committed custom form state while preserving existing dirty-row behavior.
🪄 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: 54d5ca67-ee64-46a0-853e-96248f055613
📒 Files selected for processing (21)
SparkyFitnessMobile/__tests__/components/MeasurementsSummary.test.tsxSparkyFitnessMobile/__tests__/hooks/useCustomMeasurements.test.tsSparkyFitnessMobile/__tests__/screens/DiaryScreen.test.tsxSparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsxSparkyFitnessMobile/__tests__/services/customMeasurementsApi.test.tsSparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.tsSparkyFitnessMobile/src/components/CustomBooleanControl.tsxSparkyFitnessMobile/src/components/MeasurementsSummary.tsxSparkyFitnessMobile/src/hooks/index.tsSparkyFitnessMobile/src/hooks/queryKeys.tsSparkyFitnessMobile/src/hooks/useCustomMeasurements.tsSparkyFitnessMobile/src/hooks/useCustomNutrients.tsSparkyFitnessMobile/src/hooks/useDailySummary.tsSparkyFitnessMobile/src/hooks/useMeasurements.tsSparkyFitnessMobile/src/hooks/useNutrientDisplayPreferences.tsSparkyFitnessMobile/src/hooks/useUpsertCheckIn.tsSparkyFitnessMobile/src/screens/DiaryScreen.tsxSparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsxSparkyFitnessMobile/src/services/api/measurementsApi.tsSparkyFitnessMobile/src/types/customMeasurements.tsSparkyFitnessMobile/src/utils/customMeasurementsForm.ts
Correction round for the custom measurements PR: - Partial save failures no longer discard unsaved input. Each CustomOp carries its local rowKey; operations that reach the server are removed from the pending/dirty set, failed and not-yet-attempted rows stay dirty so the refetch preserves typed values and a retry submits only the remaining work. Successful All/Unlimited inserts are never re-sent, so retries cannot duplicate them. Standard fields keep their dirty markers until the upsert actually persists. - isSaveDisabled no longer includes isCustomDataError: a custom categories/entries fetch failure no longer blocks saving weight, steps, or other standard measurements (retry affordance kept). - syncSingleEntry now honors tombstones like syncMultiEntry: a Daily id deleted locally is never resurrected by a stale server response, and the marker survives until the server stops returning the id. - Icon-only row controls (hour minus/plus, delete) get accessible names and roles using the category label. - Move customFormRef.current out of the render body into an effect. - Test fixture fixes: MeasurementsSummary uses valid CheckInMeasurement shapes; the timestamp assertion is timezone-independent (local getters only); buildCustomOps expectations now pin rowKey for a complete operation body; hourly save asserts the exact chosen hour via faked system time; boolean tri-state selection and API render order are asserted explicitly. Assisted-by: Open WebUI
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
SparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsx (2)
1090-1096: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the footer busy state with the header busy state.
The header uses
isSaving, which covers all three mutations.FooterSaveBarusesupsertMutation.isPendingonly. During a custom-only save the footer button is disabled but shows no busy label.♻️ Proposed change
<FooterSaveBar onPress={handleSave} disabled={isSaveDisabled} - busy={upsertMutation.isPending} + busy={isSaving} />🤖 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/MeasurementsAddScreen.tsx` around lines 1090 - 1096, Update the FooterSaveBar in the MeasurementsAddScreen render to use the same isSaving state as the header for its busy prop, ensuring saves handled by all three mutations display the busy label consistently.
589-651: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
standardPersistedcan never betrueat line 649.
upsertMutation.mutateAsyncruns only whencustomSucceededistrue. If it resolves,customSucceededstaystrueand the block at lines 632-636 returns. If it rejects,standardPersistedstaysfalse. The branch at lines 649-651 is therefore unreachable, and thestandardPersistedvariable is dead state.Remove the variable and the branch, or restructure the flow so a successful standard save clears
dirtyFieldsRefeven when a later step fails.♻️ Proposed simplification
const remainingDirtyCustom = new Set(dirtyCustomKeysRef.current); - let standardPersisted = false; let customSucceeded = true; @@ if (customSucceeded && hasAnyField) { try { await upsertMutation.mutateAsync(payload); - standardPersisted = true; } catch { customSucceeded = false; } } @@ dirtyCustomKeysRef.current = remainingDirtyCustom; - if (standardPersisted) { - dirtyFieldsRef.current = new Set(); - }🤖 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/MeasurementsAddScreen.tsx` around lines 589 - 651, Remove the unused standardPersisted state from the save flow around the custom operation loop and upsertMutation call. Eliminate its assignment and the trailing dirtyFieldsRef clearing branch, preserving the existing behavior where the function returns immediately after a fully successful standard save and retains standard dirty markers on partial failure.SparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsx (2)
739-777: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a partial-failure case that includes a delete operation.
The suite covers save operations only. The screen also issues delete operations from
catForm.deleted. A case where a delete succeeds and a later save fails would show whether the retry re-sends the same delete. That path is the one flagged inSparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsxat lines 593-621.🤖 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/MeasurementsAddScreen.test.tsx` around lines 739 - 777, Add a test alongside the existing partial custom failure case that populates catForm.deleted, makes the delete operation succeed, then makes a later custom save fail. Retry the save and assert the successful delete is invoked only once while the failed save is retried, unsaved rows remain pending after the first attempt, and navigation completes after retry.
546-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a role/testID query over
.parent?.parenttraversal.The assertions walk two levels up from the text node to reach the touchable. A markup change inside
CustomBooleanControlbreaks these four assertions without any behavior change. Query the option byaccessibilityRoleand accessible name, or add a testID per option inCustomBooleanControl.🤖 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/MeasurementsAddScreen.test.tsx` around lines 546 - 556, Replace the four `.parent?.parent` traversals in the tri-state assertions with stable role-and-accessible-name queries for each Yes/No option, or use dedicated per-option testIDs exposed by CustomBooleanControl. Keep the existing selected-state expectations for both categories unchanged.
🤖 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/screens/MeasurementsAddScreen.tsx`:
- Around line 593-621: The custom deletion lifecycle is incomplete: successful
deletes must clear their tombstones and remain protected during reconciliation.
In MeasurementsAddScreen.tsx lines 593-621, update the delete-success branch
alongside remainingDirtyCustom.delete to remove the matching entryId tombstone
from customForm; in lines 397-412, ensure syncCustomForm consults
current.deleted during reconciliation, or mark the deleted row key dirty there
so refetches cannot restore it. Use the existing custom form and syncCustomForm
state paths, and preserve pending tombstones and dirty entries when deletion
fails.
---
Nitpick comments:
In `@SparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsx`:
- Around line 739-777: Add a test alongside the existing partial custom failure
case that populates catForm.deleted, makes the delete operation succeed, then
makes a later custom save fail. Retry the save and assert the successful delete
is invoked only once while the failed save is retried, unsaved rows remain
pending after the first attempt, and navigation completes after retry.
- Around line 546-556: Replace the four `.parent?.parent` traversals in the
tri-state assertions with stable role-and-accessible-name queries for each
Yes/No option, or use dedicated per-option testIDs exposed by
CustomBooleanControl. Keep the existing selected-state expectations for both
categories unchanged.
In `@SparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsx`:
- Around line 1090-1096: Update the FooterSaveBar in the MeasurementsAddScreen
render to use the same isSaving state as the header for its busy prop, ensuring
saves handled by all three mutations display the busy label consistently.
- Around line 589-651: Remove the unused standardPersisted state from the save
flow around the custom operation loop and upsertMutation call. Eliminate its
assignment and the trailing dirtyFieldsRef clearing branch, preserving the
existing behavior where the function returns immediately after a fully
successful standard save and retains standard dirty markers on partial failure.
🪄 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: 0a001d94-d7bf-4e11-949d-eb595162ecba
📒 Files selected for processing (5)
SparkyFitnessMobile/__tests__/components/MeasurementsSummary.test.tsxSparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsxSparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.tsSparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsxSparkyFitnessMobile/src/utils/customMeasurementsForm.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- SparkyFitnessMobile/tests/utils/customMeasurementsForm.test.ts
- SparkyFitnessMobile/tests/components/MeasurementsSummary.test.tsx
- SparkyFitnessMobile/src/utils/customMeasurementsForm.ts
CodeRabbit follow-up on the custom measurements partial-failure fix: a delete that reached the server must be removed from the pending set AND its tombstone dropped from the custom form, otherwise a retry after a later partial failure re-sends the DELETE for an already-removed entry id. Reconciliation already honors tombstones (multi-entry guard plus the new single-entry guard), so dropping the marker after a confirmed delete completes the lifecycle. Regression test: delete + failing save -> retry sends the save but not the delete. Assisted-by: Open WebUI
apedley
left a comment
apedley
left a comment
There was a problem hiding this comment.
Need a scope cut here:
- Every auto created category is daily. Habits are daily. And hand made categories are mostly daily. So the hourly/all/unlimited stuff (hour steppers, hour conflict slot sets,insert only read only rows, and most of the tombstone code) will see next to no traffic.
Drop the hourly/all/unlimited stuff. If you want to pull it out into a separate PR that's fine but it needs to be more of a feature than something tacked on to the daily log.
- Anyone syncing health data gets 5 pages of fields on the add measurements screen. Any synced metric without a dedicated handler gets stored as a custom measurement. But it gets worse: editing one preserves the source so the manually entered value is overwritten next sync. Garmin users with mood data will even get a raw JSON blob displayed.
Filter everything:
- Diary tiles should only show when the source === 'manual'
- On the add screen, hide categories whose names matches the known health sync list. Exact name matching only hurts someone who literally typed "HRV_SDNN_min" as a category name, and their category is already receiving synced entries anyway since ingestion matches by name - renaming it un-hides it. The real fix is server side (something like a has_manual_entries flag on the categories response) so if you'd like to also add that here or in another PR feel free
--
Are you testing yourself or having AI do it? Some of this stuff might have been caught if it wasn't tested in isolation. Especially the add screen flood.
| refetchNutrientPrefs, | ||
| ]); | ||
|
|
||
| const isRefreshing = |
There was a problem hiding this comment.
ORs from 5 queries so any cache invalidation while diary is up (swipe deleting rows for example) flashes the pull to refresh spinner. Check how dashboard screen does it
There was a problem hiding this comment.
Fixed. Diary pull-to-refresh now uses Promise.allSettled over the five queries: a failing custom refetch no longer prevents the remaining refetches from completing nor throws from the refresh handler, and the spinner still tears down in finally. Regression test added (Test G) verifying one rejected custom refetch -> other refetches still run, no throw, refreshing returns to false.
There was a problem hiding this comment.
You're right — my previous reply addressed failure isolation, not the spinner behavior you were pointing out. The RefreshControl is now driven only by the local user-initiated refreshing state, matching DashboardScreen; background query invalidations/refetches no longer show the pull-to-refresh spinner. I kept Promise.allSettled only for error isolation inside an actual manual refresh and added regression coverage for both cases:
- Test E: all five queries reporting background
isRefetching(cache invalidation while the diary is up) →RefreshControl.props.refreshing === false. - Test E2: an actual pull shows
refreshing === truewhile the queries are pending andfalseafter settlement. - Test G: one rejected refetch still lets the others run and never throws; spinner returns to false.
I also removed the PR1-added isRefetching passthroughs from useDailySummary, useMeasurements, useCustomNutrients and useNutrientDisplayPreferences — they existed only to build the aggregate spinner and have no other consumers — so the shared hook APIs stay minimal.
| text1: 'Save failed', | ||
| text2: 'Could not save measurements. Please try again.', | ||
| }); | ||
| // Standalone callers (e.g. VitalsCard) rely on this toast. Multi-mutation |
There was a problem hiding this comment.
VitalsCard is not a thing on mobile. I think it may have been in the past
There was a problem hiding this comment.
Fixed. Removed the stale 'VitalsCard' reference from the useUpsertCheckIn onError comment — the comment now just says standalone callers rely on the toast while multi-mutation flows pass showErrorToast:false.
| labels: { yes: string; no: string; clear: string }; | ||
| } | ||
|
|
||
| const CustomBooleanControl: React.FC<CustomBooleanControlProps> = ({ |
There was a problem hiding this comment.
This is really a nullable boolean picker. A name like YesNoClearControl saves the next reader the "why does a boolean have three states" question.
There was a problem hiding this comment.
Fixed. CustomBooleanControl renamed to YesNoClearControl (file, component, and the import in MeasurementsAddScreen). It is a nullable/clearable boolean picker, so the name now answers the 'why three states' question.
| export { useFastingTimer } from './useFastingTimer'; | ||
| export type { FastTimerValues } from './useFastingTimer'; | ||
| export { useCustomCategories, useCustomMeasurementsByDate, useSaveCustomMeasurement, useDeleteCustomMeasurement } from './useCustomMeasurements'; | ||
| export { customCategoriesQueryKey, customMeasurementsByDateQueryKey } from './queryKeys'; |
There was a problem hiding this comment.
add it to the existing import for this file
There was a problem hiding this comment.
Fixed. customCategoriesQueryKey and customMeasurementsByDateQueryKey are now merged into the existing ./queryKeys import block instead of a standalone re-export line.
Maintainer (apedley) CHANGES_REQUESTED scope cut and health-sync fixes: Scope cut — remove Hourly / All / Unlimited from the mobile daily-log: - Drop hour steppers, hour conflict detection, multi-entry ordering, insert-only read-only rows, entryTimestampFor, and multi-entry tombstone machinery. customMeasurementsForm is now a small Daily-specific model (one editable manual row per category) and the add screen exposes only Daily frequency. Backend/web support untouched. Health-sync flood fixes: - Diary tiles (MeasurementsSummary) show ONLY manual custom entries (source === 'manual' or legacy null); synced entries never render. - Add screen filters categories BEFORE presentation: eligible = frequency 'Daily' AND name not in the centralized AUTO_HEALTH_SYNC_CUSTOM_CATEGORY_NAMES set (exact, case-sensitive — mirrors server cat.name === categoryName). The set is derived from DEFAULT_UNITS_BY_HEALTH_TYPE keys (incl. _min/_max/_avg variants), the Garmin 'Raw Stress Data' JSON category, and Oura/Fitbit/Polar/ Withings/Google hard-coded categoryNames. - Synced entries never become editable manual state (no prefill, no preserved synced source); every save posts source 'manual'. - A renamed former-sync category becomes visible again. - Garmin raw JSON category is filtered out (no special parsing needed). - Integrated regression: 100-category health-heavy account does not flood the add screen; manual + renamed categories still editable. Reliability: - Diary pull-to-refresh uses Promise.allSettled (one failing refetch no longer blocks the others nor throws); spinner still tears down. - Retained prior guarantees: custom fetch failure does not block standard saves, partial save failure preserves input, retries skip succeeded rows, zero/false/comma-decimal remain valid, dirty values survive refetch, one coherent mutation-pending state for header + footer. Inline maintainer nits: - CustomBooleanControl renamed to YesNoClearControl. - hooks/index.ts merges the custom query keys into the existing queryKeys import block. - Removed the stale VitalsCard reference in useUpsertCheckIn comment. - Consolidated the duplicate customMeasurementsApi POST tests into one asserting the complete request body. Tests: Daily types (numeric/text/boolean/zero/false/comma), visibility filters, synced-source handling, Diary manual-only, health-heavy integration, dirty/refetch/partial-failure reliability, and the Diary allSettled refresh. Hourly/All/Unlimited unit tests removed. Assisted-by: Open WebUI
There was a problem hiding this comment.
Thanks for the thorough review — all points addressed in 37e4821a.
Scope cut (Hourly / All / Unlimited removed):
- Dropped hour steppers, hour-conflict detection, multi-entry ordering, insert-only read-only rows,
entryTimestampFor, and the multi-entry tombstone machinery.customMeasurementsFormis now a small Daily-specific model (one editable manual row per category) and the add screen exposes onlyDailyfrequency. Backend/web support untouched; those frequencies are simply not surfaced as editable fields here.
Health-sync flood fixed (both filters):
- Diary tiles (
MeasurementsSummary) now show only entries withsource === 'manual'(legacy null treated as manual). Synced entries never render as tiles. - Add screen filters before presentation: eligible =
frequency === 'Daily'AND name not in a centralizedAUTO_HEALTH_SYNC_CUSTOM_CATEGORY_NAMESset. Matching is exact + case-sensitive, mirroring servercat.name === categoryName. The set was derived by auditing the repository:DEFAULT_UNITS_BY_HEALTH_TYPEkeys (incl._min/_max/_avgvariants), the GarminRaw Stress DataJSON category, and the hard-codedcategoryNamevalues in Oura/Fitbit/Polar/Withings/Google processors. Renaming a category away from the canonical name re-shows it (verified by test). - Synced entries are never prefilled/edited as manual state and a synced source is never preserved into a manual save — every save from this screen posts
source: 'manual'. Garmin raw JSON categories are filtered out (no UI parsing needed). The 5-pages-of-fields flood is covered by a 100-category integration test.
Testing honesty: all validation here is automated (Jest, tsc, lint, CI).
Then when everythink is green I built Android apk and test this on real device.
Also fixed the four inline comments (Diary allSettled refresh, VitalsCard comment, YesNoClearControl rename, hooks/index query-keys import).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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__/components/MeasurementsSummary.test.tsx`:
- Around line 238-274: Update the “manual entry with value 0 still appears” and
“manual boolean false still appears” tests to assert the rendered value text in
addition to the tile labels. Verify the UI displays 0 for the numeric entry and
false for the boolean entry, so formatting cannot silently drop either value.
In `@SparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsx`:
- Around line 930-935: Correct the fixture-count comment above healthNames to
match the array’s actual auto-generated category count. In the integrated
health-heavy account test, replace the fixed custom-input-health-98 and
custom-input-health-99 checks with an index-independent assertion that no input
whose identifier matches the health-* pattern is rendered.
In `@SparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.ts`:
- Around line 13-19: Remove the undeclared measurement_type property from the
dailyCat fixture while preserving all fields required by CustomCategoryMeta and
its existing dataType behavior.
🪄 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: a006b920-de6c-4149-a806-c96a05c771a3
📒 Files selected for processing (13)
SparkyFitnessMobile/__tests__/components/MeasurementsSummary.test.tsxSparkyFitnessMobile/__tests__/screens/DiaryScreen.test.tsxSparkyFitnessMobile/__tests__/screens/MeasurementsAddScreen.test.tsxSparkyFitnessMobile/__tests__/services/customMeasurementsApi.test.tsSparkyFitnessMobile/__tests__/utils/customMeasurementsForm.test.tsSparkyFitnessMobile/src/components/MeasurementsSummary.tsxSparkyFitnessMobile/src/components/YesNoClearControl.tsxSparkyFitnessMobile/src/hooks/index.tsSparkyFitnessMobile/src/hooks/useUpsertCheckIn.tsSparkyFitnessMobile/src/screens/DiaryScreen.tsxSparkyFitnessMobile/src/screens/MeasurementsAddScreen.tsxSparkyFitnessMobile/src/utils/autoHealthSyncCategories.tsSparkyFitnessMobile/src/utils/customMeasurementsForm.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- SparkyFitnessMobile/src/hooks/useUpsertCheckIn.ts
- SparkyFitnessMobile/src/hooks/index.ts
- SparkyFitnessMobile/tests/screens/DiaryScreen.test.tsx
- SparkyFitnessMobile/tests/services/customMeasurementsApi.test.ts
- SparkyFitnessMobile/src/screens/DiaryScreen.tsx
- MeasurementsSummary: the 'value 0' and 'boolean false' Diary tests now
also assert the rendered value text ('0', 'false') so formatting cannot
silently drop either real value.
- MeasurementsAddScreen integrated health-heavy test: corrected the
fixture-count comment (100+) and replaced fixed index checks with an
index-independent assertion that no input whose testID matches the
health-* pattern renders.
- customMeasurementsForm: removed the undeclared measurement_type field
from the dailyCat fixture (kept every field required by
CustomCategoryMeta).
Assisted-by: Open WebUI
Follow-up to the maintainer's CHANGES_REQUESTED review: Diary pull-to-refresh spinner: - The RefreshControl is now driven ONLY by the local user-initiated 'refreshing' state, matching DashboardScreen. Background React Query refetches/cache invalidations (e.g. swipe-deleting a row) no longer turn on the pull-to-refresh spinner. - Removed the PR1-added isRefetching passthroughs from useDailySummary, useMeasurements, useCustomNutrients and useNutrientDisplayPreferences (they existed only to build the aggregate spinner and have no other consumers). DiaryScreen no longer destructures them. - Kept Promise.allSettled inside the manual onRefresh for error isolation; one failing refetch neither blocks the others nor throws. - Tests: Test E asserts all five queries reporting background isRefetching keep the spinner off; Test E2 asserts a manual pull shows the spinner while pending and clears after settlement; Test G retains rejected-query isolation. Strict manual source contract: - isManualSource now returns source === 'manual' ONLY. The DB column is source VARCHAR(50) NOT NULL DEFAULT 'manual', so null/undefined are NOT manual; removed the 'legacy null' exception everywhere. - Diary hasAnyMeasurement/manualCustomMeasurements, MeasurementsSummary (defense-in-depth) and syncCustomForm all use the single isManualSource predicate; no inline source filters remain. - Tests: null and undefined sources never prefill the Daily manual editor and never create a Diary tile; synced (garmin/healthkit) sources stay excluded; a user-entered value still saves a fresh operation with source 'manual'. Cleanup: - Removed the dead standardPersisted state (the only true assignment always preceded the full-success return; the later branch was unreachable). Partial-failure behavior is unchanged. Assisted-by: Open WebUI
I built release apk and test in on my android phone. I didn't add health sync in this testing app and that was my mistake. Edit. Tested. I attach screenshots to PR description. |
|
Everything looks really good which makes me sorry I have to walk back part of this.. I had assumed the collision space was hard to type names like HRV_SDNN_min. Something like "weight" or "Blood Pressure" we can't filter out. Let's do this: Keep the name list, but matched categories with no manual entry collapse into a "More categories" section behind one tap instead of disappearing. That way it can ship and I can figure out a long term solution for server side. |
Maintainer apedley requirement (5228882952): the exact health-sync name
list must stop being an exclusion rule. Matched Daily categories with no
manual entry for the selected date now collapse under a one-tap 'More
categories' section instead of disappearing; matched categories that
already have a manual entry stay in the main custom list.
Architecture (form model vs presentation):
- FORM MODEL is now dailyCustomCategories = every frequency==='Daily'
category (health-sync names included), feeding syncCustomForm,
buildCustomOps, delete-confirmation lookup, reconciliation, and dirty
preservation. A value typed inside the collapsed section is always part
of the form regardless of expansion state or refetch.
- manualCategoryIds is built from customMeasurements via the strict
isManualSource helper (synced/null/undefined never count) for the
CURRENT selected date only (temporary mobile heuristic; server-side
has_manual_entries remains future work per maintainer).
- PRESENTATION partitions dailyCustomCategories: primary = not a known
health-sync name OR has a manual entry today; more = known health-sync
name AND no manual entry today. Both preserve API ordering.
- 'More categories' uses the existing FoodForm 'Show more nutrients'
pattern (Button variant=ghost, self-start, py-0 px-0, accent text,
lazy render via showMoreCategories && map). accessibilityRole=button,
accessibilityState={{ expanded }}, label 'More categories'.
Behavior preserved: manual-only saves (source 'manual'), synced values
never prefilled, synced entries never deleted by manual saves, Hourly /
All / Unlimited absent everywhere, dirty values survive collapse/re-expand
and background refetch, partial-failure retry and tombstone lifecycle
unchanged, Diary strict manual-only contract unchanged.
Tests: updated the old 'hides health-sync categories' expectation and the
health-heavy account test; added A-G + K-L regressions (collapsed
weight, manual Blood Pressure promoted+prefilled, synced-only behind
More not prefilled, null-source stays hidden, save-from-More uses manual
source, dirty survives collapse/re-expand, dirty survives refetch, no
toggle when nothing hidden, accessibility expanded state).
Full Jest 286 suites / 4817 tests, 0 failed.
Assisted-by: Open WebUI
|
@coderabbitai review Fresh review requested for |
|
Thanks — that makes sense. I kept the exact health-sync name list, but changed it from an exclusion rule into a presentation heuristic: matched Daily categories with no manual entry for the selected date now stay accessible under a one-tap This keeps health-heavy accounts compact without hiding legitimate collisions such as I also kept the UI intentionally close to the existing Measurements form — I’ll build and test a fresh APK from this updated HEAD before treating the device validation/screenshots as final. |
|
✅ Action performedReview finished.
|
|
@apedley I attach new screenshots here. This is what you had in mind with sync categories? Screenshots
|
3 participants
Description
What problem does this PR solve?
The mobile app lacked support for the backend's existing custom-measurement API. This PR adds manual Daily custom-measurement editing on the Measurements Add screen and manual-only Diary tiles.
How did you implement the solution?
MeasurementsAddScreen: one editable manual row per Daily category — numeric, text, boolean (YesNoClearControl), zero,false, comma decimal1,5; create/save, edit, clear/delete; dirty-state and refetch protection; partial-failure handling; single coherent mutation-pending state for header + footer.source === 'manual'(strict contract — the DB column isNOT NULL DEFAULT 'manual', so null/undefined are not manual); the Add screen filters categories before presentation (frequency === 'Daily'AND name not in the centralizedAUTO_HEALTH_SYNC_CUSTOM_CATEGORY_NAMESset, exact/case-sensitive matching mirroring server ingestion). Synced entries never become editable manual state; every save postssource: 'manual'. Renamed former-sync categories become visible again. Garmin raw JSON categories are filtered out.DiaryScreen: manual-only presentation; pull-to-refresh spinner is driven only by the local user-initiatedrefreshingstate (DashboardScreen pattern) — background query invalidations never flash the spinner;Promise.allSettledkeeps refresh error-isolated.useUpsertCheckIn: optionalshowErrorToast: falsefor the orchestrating screen.Linked Issue: Related to #1923 · Related to #1979
How to Test
cd SparkyFitnessMobile && pnpm installnpx jest --runInBand __tests__/utils/customMeasurementsForm.test.ts __tests__/services/customMeasurementsApi.test.ts __tests__/screens/MeasurementsAddScreen.test.tsx __tests__/components/MeasurementsSummary.test.tsx __tests__/screens/DiaryScreen.test.tsxHRV_SDNN_min,Resting Heart Rate, GarminRaw Stress Dataabsent), enter numeric/text/boolean values, save, restart, confirm persisted; Diary shows only manual custom tiles.PR Type
Checklist
All PRs:
New features only:
Mobile changes (
SparkyFitnessMobile/):Screenshots
Before scope cut
After
has_manual_entriesflag on the categories response is the suggested long-term fix; happy to add it in a follow-up PR.More categories (health-sync names)
Daily-only scope remains. The known health-sync name list remains, but it is now a presentation heuristic, not an exclusion rule: matched Daily categories with no manual entry for the selected date are collapsed under a one-tap More categories section (lightweight Show-more pattern, lazy-rendered), while matched categories that already have a manual entry stay visible in the main custom list. This keeps integration-heavy accounts compact while keeping legitimate collisions (
weight,Blood Pressure) accessible. Exact/case-sensitive matching remains; renamed categories remain primary; every manual save usessource: 'manual'and synced-only values are never prefilled. Long-term server-side categorization remains future work.Summary by CodeRabbit