Begin integration of unit conversion + optimisation to BeerXML import flow #91
Conversation
WalkthroughAdds BeerXML unit-detection and a user unit-choice conversion step (UI + API), converts recipe metrics to an offline calculator, centralizes Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImportScreen as ImportBeerXMLScreen
participant Modal as UnitConversionChoiceModal
participant BeerXML as BeerXMLService
participant API as ApiService
participant Matching as IngredientMatching
User->>ImportScreen: select BeerXML file
ImportScreen->>BeerXML: parse file
BeerXML-->>ImportScreen: parsed recipes
ImportScreen->>BeerXML: detectRecipeUnitSystem(recipe)
BeerXML-->>ImportScreen: detectedSystem
ImportScreen->>Modal: show UnitConversionChoiceModal (if applicable)
alt user chooses conversion
User->>Modal: select target system
Modal-->>ImportScreen: onChooseMetric/onChooseImperial
ImportScreen->>BeerXML: convertRecipeUnits(recipe, targetSystem, normalize)
BeerXML->>API: POST /beerxml/convert-recipe
API-->>BeerXML: converted recipe + warnings
BeerXML-->>ImportScreen: converted recipe (or original on fallback)
ImportScreen->>Matching: navigate with convertedRecipe
else import as-is
ImportScreen->>Matching: navigate with originalRecipe
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/services/beerxml/BeerXMLService.ts (1)
400-499: Unit detection + conversion flow is solid; consider explicitly setting unit_system after conversionThe unit detection logic (counting recognized metric/imperial batch and ingredient units and returning
"mixed"for ambiguous cases) is straightforward, andconvertRecipeUnits’ use of the AIunit_conversionworkflow plus a graceful fallback to the original recipe on failure is a good safety net.One improvement to make the contract more explicit: if the AI endpoint does not set
unit_system, callers might see a stale or undefined unit system even after successful conversion. You can guarantee consistency by forcingunit_systemon the merged recipe:// Merge converted data back into original recipe structure // Use nullish coalescing to preserve falsy values like 0 or empty string return { ...recipe, ...convertedRecipe, + unit_system: convertedRecipe.unit_system ?? targetUnitSystem, ingredients: convertedRecipe.ingredients ?? recipe.ingredients, batch_size: convertedRecipe.batch_size ?? recipe.batch_size, batch_size_unit: convertedRecipe.batch_size_unit ?? recipe.batch_size_unit, mash_temperature: convertedRecipe.mash_temperature ?? recipe.mash_temperature, mash_temp_unit: convertedRecipe.mash_temp_unit ?? recipe.mash_temp_unit, };src/components/beerxml/UnitConversionChoiceModal.tsx (1)
1-244: Clean implementation of UnitConversionChoiceModal with minor optional UX tweaksThe modal component is well-structured: it consumes theme colors, clearly explains the unit mismatch, exposes focused callbacks (
onConvert,onImportAsIs,onCancel), and correctly disables actions whileisConverting(with a loading indicator).Two small optional refinements you might consider:
- Prevent closing the modal via
onCancelwhileisConverting(e.g., have the parent ignoreonCancelduring conversion) so users can’t accidentally dismiss it mid-request.- For slightly friendlier copy, map
recipeUnitSystem/userUnitSystemto display labels like “Metric”/“Imperial” instead of the raw lowercase literals before rendering them.app/(modals)/(beerxml)/importBeerXML.tsx (2)
153-204: LGTM!The conversion flow is well-structured with proper error handling. The Alert on conversion failure provides a good user experience by offering a fallback option.
Note: Line 198 uses a non-null assertion (
importState.selectedRecipe!). While the early return on lines 154-156 guards against this, consider capturing the recipe in a local variable at the start of the function for better safety:const handleConvertAndImport = async () => { const recipe = importState.selectedRecipe; if (!recipe) { return; } // ... use recipe instead of importState.selectedRecipe throughoutThis is optional but improves clarity and prevents potential issues if state changes.
488-504: LGTM with a type safety note.The conditional rendering correctly ensures the modal only displays when there's a valid recipe unit system. The guard on line 489 prevents rendering when
recipeUnitSystemis null (i.e., for "mixed" unit systems).Optional improvement: TypeScript's control flow analysis may not narrow the type inside JSX after the
&&operator. Consider using a more explicit pattern for better type safety:{importState.recipeUnitSystem && importState.showUnitConversionChoice && ( <UnitConversionChoiceModal visible={true} recipeUnitSystem={importState.recipeUnitSystem as UnitSystem} // ... rest of props /> )}Or extract to a variable before JSX:
const recipeUnits = importState.recipeUnitSystem; // Then in JSX: {recipeUnits && ( <UnitConversionChoiceModal visible={importState.showUnitConversionChoice} recipeUnitSystem={recipeUnits} // ... /> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importBeerXML.tsx(8 hunks)package.json(1 hunks)src/components/beerxml/UnitConversionChoiceModal.tsx(1 hunks)src/services/beerxml/BeerXMLService.ts(3 hunks)src/styles/components/beerxml/unitConversionModalStyles.ts(1 hunks)src/types/ai.ts(1 hunks)tests/app/(modals)/(beerxml)/importBeerXML.test.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/types/ai.ts (1)
src/types/recipe.ts (1)
Recipe(94-133)
src/components/beerxml/UnitConversionChoiceModal.tsx (4)
src/types/common.ts (1)
UnitSystem(27-27)src/contexts/ThemeContext.tsx (1)
useTheme(109-115)src/styles/components/beerxml/unitConversionModalStyles.ts (1)
unitConversionModalStyles(9-89)src/styles/common/colors.ts (1)
colors(85-85)
src/services/beerxml/BeerXMLService.ts (2)
src/types/recipe.ts (1)
Recipe(94-133)src/types/common.ts (1)
UnitSystem(27-27)
tests/app/(modals)/(beerxml)/importBeerXML.test.tsx (2)
app/(modals)/(beerxml)/importBeerXML.tsx (1)
ImportBeerXMLScreen(53-507)src/constants/testIDs.ts (1)
TEST_IDS(145-280)
app/(modals)/(beerxml)/importBeerXML.tsx (4)
src/services/beerxml/BeerXMLService.ts (1)
BeerXMLRecipe(12-15)src/types/common.ts (1)
UnitSystem(27-27)src/contexts/UnitContext.tsx (1)
useUnits(119-125)src/components/beerxml/UnitConversionChoiceModal.tsx (1)
UnitConversionChoiceModal(78-244)
🔇 Additional comments (19)
android/app/build.gradle (1)
95-96: Android version bump aligns with other configs
versionCode 192andversionName "3.3.0"are consistent with the rest of the PR’s versioning and are appropriate for a new release build.package.json (1)
4-4: package.json version synchronized to 3.3.0The package version matches the native and Expo versions, keeping build/version scripts consistent.
app.json (1)
6-61: Expo version, Android versionCode, and runtimeVersion kept in syncUpdating
version,android.versionCode(192), andruntimeVersionall to 3.3.0 keeps native builds and Expo Updates runtime boundaries aligned. This looks coherent with the Gradle and package.json changes.android/app/src/main/res/values/strings.xml (1)
4-4: expo_runtime_version string aligned with runtimeVersionThe
expo_runtime_versionresource now matches the 3.3.0 runtime version used in app.json, keeping native resources consistent with the OTA runtime.src/types/ai.ts (1)
24-26: Allowing partial recipes in AIAnalysisRequest fits unit_conversion needsWidening
complete_recipetoRecipe | Partial<Recipe>cleanly supports the unit_conversion workflow while remaining backward compatible with existing callers that still send fullRecipeobjects. The updated doc comment makes the exception explicit.src/services/beerxml/BeerXMLService.ts (1)
3-15: BeerXMLRecipe interface specialization looks appropriateExporting
BeerXMLRecipeasPartial<Recipe>with requiredingredientsand optionalmetadataaccurately reflects parsed BeerXML payloads and gives downstream consumers a strong, reusable type for this service.src/styles/components/beerxml/unitConversionModalStyles.ts (1)
1-89: UnitConversion modal styles are well-structuredThe style sheet cleanly separates overlay, content, text, and button styling, with appropriate touch targets (
minHeight: 48) and shadow/elevation for the dialog. It pairs well with theme-driven colors from the component.tests/app/(modals)/(beerxml)/importBeerXML.test.tsx (4)
13-21: LGTM!The extended color properties align with the component's theme requirements.
26-45: LGTM!Comprehensive UnitContext mock covering all methods. The default "imperial" unit system provides a consistent baseline for testing.
57-58: LGTM!The mock implementations provide sensible defaults while allowing individual tests to override behavior as needed.
63-65: LGTM!Mocking the modal to return null is appropriate for testing the integration logic without rendering the actual modal UI.
app/(modals)/(beerxml)/importBeerXML.tsx (8)
28-36: LGTM!The new imports are necessary for the unit conversion feature and follow the project's import conventions.
56-56: LGTM!Correct usage of the
useUnitshook to access the user's preferred unit system.
65-67: LGTM!The new state fields are initialized with appropriate default values.
209-214: LGTM!The handler correctly closes the modal and proceeds with the original recipe unchanged.
219-235: LGTM!The optional parameter pattern is excellent—it supports both the conversion flow (passing the converted recipe) and the direct import flow (using the state recipe), maintaining backward compatibility with existing call sites.
248-250: LGTM!The reset function correctly clears all unit-related state fields.
403-403: LGTM!The call site correctly leverages the fallback logic added to
proceedToIngredientMatching.
122-137: Both BeerXMLService methods are properly implemented and handle edge cases correctly.The
detectRecipeUnitSystemmethod (lines 404-444) correctly detects unit systems, handles whitespace withtrim(), and returns "mixed" when systems are balanced or unknown. TheconvertRecipeUnitsmethod (lines 450-500) properly calls the AI endpoint and gracefully falls back to the original recipe on conversion failures. The code in importBeerXML.tsx correctly setsrecipeUnitSystemtonullwhen "mixed" is detected and only shows the conversion modal for true mismatches, which aligns with the service implementation.
…ially seeing a stale or undefined unit system even after successful conversion - Removed Unused "unit_conversion" Step - Improved Type Safety in handleConvertAndImport - Renamed Misleading Test Names
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/services/beerxml/BeerXMLService.ts (1)
400-444: LGTM: Unit detection logic is sound.The method correctly identifies the predominant unit system by counting metric and imperial units across batch size and ingredients. The trimming and lowercase normalization handles input variations well.
One edge case: when both
metricCountandimperialCountare 0 (no recognizable units), the method returns"mixed"(line 443). This is reasonable behavior—it treats recipes without clear unit information as ambiguous—but consider adding a brief comment documenting this case for future maintainers.Optional: add a clarifying comment at line 443:
- return "mixed"; // Equal or unknown + return "mixed"; // Equal counts, unknown units, or no detectable unitstests/app/(modals)/(beerxml)/importBeerXML.test.tsx (1)
467-719: Test coverage addresses past review feedback.The test suite correctly verifies the unit detection logic, and the test names (lines 610, 670) were updated per past review comments to accurately reflect that they test detection, not the full conversion flow.
The
UnitConversionChoiceModalmock returnsnull(line 64), sohandleConvertAndImportandhandleImportAsIsare never invoked. This means the actual conversion logic, error handling, and navigation after user choice aren't tested here. While the current tests provide good coverage of the detection logic, consider adding integration tests in the future that unmock the modal or directly invoke the conversion handlers to verify the complete flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importBeerXML.tsx(9 hunks)package.json(1 hunks)src/services/beerxml/BeerXMLService.ts(3 hunks)tests/app/(modals)/(beerxml)/importBeerXML.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (2)
app/(modals)/(beerxml)/importBeerXML.tsx (4)
src/services/beerxml/BeerXMLService.ts (1)
BeerXMLRecipe(12-15)src/types/common.ts (1)
UnitSystem(27-27)src/contexts/UnitContext.tsx (1)
useUnits(119-125)src/components/beerxml/UnitConversionChoiceModal.tsx (1)
UnitConversionChoiceModal(78-244)
src/services/beerxml/BeerXMLService.ts (2)
src/types/recipe.ts (1)
Recipe(94-133)src/types/common.ts (1)
UnitSystem(27-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (9)
android/app/build.gradle (2)
95-96: Verify the versionCode increment of 2.The versionCode was bumped from 191 to 193, skipping 192. This is unusual; typically each release increments by 1. Please clarify whether this is intentional (e.g., beta/internal releases allocated 192) or if this should be 192 instead.
95-96: Semantic versioning bump looks appropriate for a feature release.The versionName change from "3.2.6" to "3.3.1" correctly follows semantic versioning conventions for a feature release (minor version bump with patch reset), aligning with the PR's unit conversion feature addition.
android/app/src/main/res/values/strings.xml (1)
4-4: Runtime version consistent with app config.The expo_runtime_version string resource is correctly updated to match the version changes in app.json and build.gradle.
app.json (1)
6-6: Version updates are consistent across all configuration files.The version in app.json (line 6), versionCode (line 19), runtimeVersion (line 61), and package.json are all correctly updated to 3.3.1, matching the build.gradle changes. These updates follow proper semantic versioning for a feature release.
Also applies to: 19-19, 61-61
src/services/beerxml/BeerXMLService.ts (1)
446-501: LGTM: Conversion with graceful fallback.The method correctly invokes the AI unit conversion workflow and merges results using nullish coalescing (lines 483-494), which preserves falsy values like
0. The fallback to the original recipe on error (lines 495-500) ensures the import flow isn't blocked, and the logging at lines 496-498 aids debugging.app/(modals)/(beerxml)/importBeerXML.tsx (4)
48-68: LGTM: State structure supports the conversion flow.The three new state fields (
showUnitConversionChoice,recipeUnitSystem,isConverting) cleanly manage the unit conversion workflow. Initialization is correct, withrecipeUnitSystemstarting asnullto ensure the modal doesn't render on mount.
123-138: LGTM: Unit detection logic is well-integrated.The detection happens at the right point (after parsing), and the logic correctly handles the three cases:
- Store
nullfor"mixed"(line 135) so the modal won't render (line 490 checks truthiness)- Show the modal only when units don't match AND the system isn't
"mixed"(line 137)This ensures users aren't prompted to convert recipes with inconsistent or unknown units, which is the right UX decision.
150-215: LGTM: Conversion handlers are well-structured.Both handlers are implemented correctly:
handleConvertAndImport(lines 153-205): defensive null check, managesisConvertingstate, converts via service, updates state, and proceeds to matching. The error handling (lines 176-203) gracefully offers import-as-is as a fallback, which prevents blocking the user flow.handleImportAsIs(lines 210-215): simply bypasses conversion and proceeds directly to matching.The separation of concerns is clean, and the fallback UX is user-friendly.
489-505: LGTM: Modal rendering is correctly wired.The conditional rendering at line 490 ensures the modal only appears when
recipeUnitSystemis truthy (i.e., notnullfor"mixed"units). All props are correctly bound to state and handlers, and theonCancelcallback (lines 498-503) properly hides the modal without proceeding, allowing the user to import manually via the "Import Recipe" button.
…e temp recipe to display immediately and update to server returned recipe when available - Add delete brew session functionality from dashboard (previously was a "coming soon" no-op) - Switch recipe metric calculation to always use offline calculations - Improve styling - Add TemperatureUnit union type to common types and update references throughout app to use - Update tests and increase coverage
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (14)
app/(tabs)/index.tsx (1)
218-223: Inconsistent query key patterns and magic string.Two observations:
- Line 221 uses
QUERY_KEYS.DASHBOARDdirectly while line 220 uses spread[...QUERY_KEYS.RECIPES]. ThecloneMutation(lines 248-249) consistently uses spread for both.- Line 222 uses a magic string
["userRecipes"]instead of aQUERY_KEYSconstant.Consider aligning the patterns and adding
userRecipestoQUERY_KEYSinqueryClient.tsfor consistency.onSuccess: () => { // Invalidate both recipes and dashboard queries to immediately update UI - queryClient.invalidateQueries({ queryKey: [...QUERY_KEYS.RECIPES] }); - queryClient.invalidateQueries({ queryKey: QUERY_KEYS.DASHBOARD }); - queryClient.invalidateQueries({ queryKey: ["userRecipes"] }); + queryClient.invalidateQueries({ queryKey: QUERY_KEYS.RECIPES }); + queryClient.invalidateQueries({ queryKey: QUERY_KEYS.DASHBOARD }); + queryClient.invalidateQueries({ queryKey: QUERY_KEYS.USER_RECIPES }); // Add USER_RECIPES to QUERY_KEYS },tests/utils/deviceUtils.test.ts (2)
25-40: Make global.crypto restoration symmetric with original definitionYou currently store only
global.crypto’s value and then redefine the property with fixed{ writable: true, configurable: true }, which can diverge from the original descriptor (or create the property when it didn’t exist). For robustness, capture and restore the full property descriptor instead of just the value.For example:
- let originalCrypto: any; + let originalCryptoDescriptor: PropertyDescriptor | undefined; beforeEach(() => { - jest.clearAllMocks(); - originalCrypto = global.crypto; + jest.clearAllMocks(); + originalCryptoDescriptor = Object.getOwnPropertyDescriptor(global, "crypto"); }); afterEach(() => { - // Restore original crypto - Object.defineProperty(global, "crypto", { - value: originalCrypto, - writable: true, - configurable: true, - }); + // Restore original crypto + if (originalCryptoDescriptor) { + Object.defineProperty(global, "crypto", originalCryptoDescriptor); + } else { + // If there was no crypto originally, remove the test override + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (global as any).crypto; + } });
186-202: Add coverage for Android/iOS branches in getPlatformCurrent tests only verify the fallback behavior (
"web"for unknown or null OS) but not the primary"Android"and"iOS"branches implemented ingetPlatform(see src/utils/deviceUtils.ts). Adding explicit tests will lock in the intended mapping.For example:
describe("getPlatform", () => { + it("should return 'android' when osName is 'Android'", () => { + mockDevice.osName = "Android"; + + const result = getPlatform(); + + expect(result).toBe("android"); + }); + + it("should return 'ios' when osName is 'iOS'", () => { + mockDevice.osName = "iOS"; + + const result = getPlatform(); + + expect(result).toBe("ios"); + }); + it("should return 'web' for unknown OS (fallback)", () => { mockDevice.osName = "Windows";app/(modals)/(recipes)/editRecipe.tsx (1)
419-440: Consider extracting the metrics fallback logic to reduce duplication.The metrics assignment pattern is repeated five times with identical structure. This could be refactored into a helper function to improve maintainability.
Consider this refactor:
+ const getMetricOrFallback = (metricKey: keyof RecipeMetrics, estimatedKey: keyof Recipe) => { + return metricsData && Number.isFinite(metricsData[metricKey]) + ? metricsData[metricKey] + : existingRecipe?.[estimatedKey]; + }; + // Include estimated metrics - use newly calculated metrics if available and finite, // otherwise preserve existing recipe metrics to avoid resetting them - estimated_og: - metricsData && Number.isFinite(metricsData.og) - ? metricsData.og - : existingRecipe?.estimated_og, - estimated_fg: - metricsData && Number.isFinite(metricsData.fg) - ? metricsData.fg - : existingRecipe?.estimated_fg, - estimated_abv: - metricsData && Number.isFinite(metricsData.abv) - ? metricsData.abv - : existingRecipe?.estimated_abv, - estimated_ibu: - metricsData && Number.isFinite(metricsData.ibu) - ? metricsData.ibu - : existingRecipe?.estimated_ibu, - estimated_srm: - metricsData && Number.isFinite(metricsData.srm) - ? metricsData.srm - : existingRecipe?.estimated_srm, + estimated_og: getMetricOrFallback('og', 'estimated_og'), + estimated_fg: getMetricOrFallback('fg', 'estimated_fg'), + estimated_abv: getMetricOrFallback('abv', 'estimated_abv'), + estimated_ibu: getMetricOrFallback('ibu', 'estimated_ibu'), + estimated_srm: getMetricOrFallback('srm', 'estimated_srm'),app/(modals)/(beerxml)/importReview.tsx (3)
83-83: Query key with object reference may trigger unnecessary re-fetches.Including
recipeDatadirectly in the query key causes re-fetches whenever the object reference changes, even if content is identical. Consider serializing to a stable string or extracting only the relevant fields.- queryKey: ["recipeMetrics", "beerxml-import-offline", recipeData], + queryKey: [ + "recipeMetrics", + "beerxml-import-offline", + recipeData?.batch_size, + recipeData?.batch_size_unit, + recipeData?.efficiency, + recipeData?.boil_time, + JSON.stringify(recipeData?.ingredients?.map((i: any) => i.ingredient_id)), + ],
100-116: Extract duplicated unit-system derivation logic.The mash_temp_unit and default temperature logic is duplicated between the metrics input (lines 105-115) and recipe payload (lines 176-186). Extract to a helper to ensure consistency.
// Helper at top of file const deriveUnitDefaults = (batchSizeUnit: string | undefined) => { const isMetric = String(batchSizeUnit).toLowerCase() === "l"; return { isMetric, tempUnit: isMetric ? "C" : "F" as TemperatureUnit, defaultMashTemp: isMetric ? 67 : 152, unitSystem: isMetric ? "metric" : "imperial" as const, }; };Also applies to: 168-186
534-573: Truthy checks for metrics may hide zero values.Using
{calculatedMetrics.og ? ... : null}hides the row when the value is0. While OG/FG/ABV/IBU/SRM are unlikely to be exactly 0 in valid recipes, consider using explicit!= nullchecks for robustness.- {calculatedMetrics.og ? ( + {calculatedMetrics.og != null ? (tests/src/hooks/useRecipeMetrics.test.ts (1)
386-389: Query key indices are correct but consider using destructuring or constants to improve maintainability.The test assertions correctly verify indices [1–4] against the implementation (queryKey at lines 40–50 of useRecipeMetrics.ts), where:
- [1] = "offline-first"
- [2] = batch_size
- [3] = batch_size_unit
- [4] = efficiency
However, relying on positional indices makes tests brittle when the queryKey structure evolves. Consider extracting indices into named constants or destructuring the queryKey in assertions to make the relationship between test and implementation explicit and less error-prone.
tests/app/(modals)/(beerxml)/importReview.test.tsx (6)
10-27: Context and appearance mocks are consistent but could be shared.These mocks look coherent and minimal, and they unblock
ImportReviewScreenfrom real environment dependencies. Consider lifting them into a shared test helper (e.g.contextMocksin@/tests/testUtils) to avoid drift and duplication across test files.Also applies to: 120-153
100-118: Auth state helper is useful; ensure deterministic reset between tests.
mockAuthState+setMockAuthStatemake it easy to override auth for specific tests. To avoid subtle cross-test leakage whensetMockAuthStateis called with partial overrides, consider keeping abaseAuthStateconstant and resettingmockAuthStatefrom that base in a top-levelbeforeEachor insidesetMockAuthState.
155-170: DuplicateresetCounterscall in firstbeforeEach.
testUtils.resetCounters()is invoked twice; this is harmless but noisy and could confuse future readers. Dropping one call keeps setup clearer.beforeEach(() => { jest.clearAllMocks(); - testUtils.resetCounters(); - testUtils.resetCounters(); + testUtils.resetCounters(); setMockAuthState({ user: { id: "test-user",
188-191: RepeatedbeforeEachsetup can be centralized.Several suites repeat the same pattern (
jest.clearAllMocks()+testUtils.resetCounters()). If there’s no suite-specific variation, consider a top-levelbeforeEach(or a small helper) to keep setup DRY and reduce the chance of suites drifting apart.Also applies to: 220-223, 300-303, 378-381
272-297:coerceIngredientTimetest only asserts non-crash, not behavior.This test currently just ensures the screen renders when given various
timeshapes, which is a valid smoke test. IfcoerceIngredientTimehas specific normalization behavior (e.g., mapping falsy values to a default string), consider adding at least one assertion on the rendered time output to lock that behavior in.
43-89: Consolidate redundant@tanstack/react-querymock or use realQueryClient.A centralized mock for
@tanstack/react-queryalready exists intests/setupTests.jswith a stateful implementation (Map-backed query storage, cache tracking). This file duplicates and overrides that mock with shallowjest.fn()stubs for methods (mount,unmount,defaultOptions, etc.) that the tests don't actually call. Either remove this local mock and rely on the setupTests.js version, or if local test-specific behavior is needed, construct a realQueryClientfromjest.requireActualand only override the methods your tests use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(8 hunks)app/(modals)/(recipes)/editRecipe.tsx(1 hunks)app/(tabs)/index.tsx(3 hunks)package.json(1 hunks)src/hooks/useRecipeMetrics.ts(5 hunks)src/services/brewing/OfflineMetricsCalculator.ts(2 hunks)src/styles/modals/createRecipeStyles.ts(1 hunks)src/types/brewSession.ts(1 hunks)src/types/common.ts(1 hunks)src/types/recipe.ts(2 hunks)tests/app/(modals)/(beerxml)/importReview.test.tsx(13 hunks)tests/services/brewing/OfflineMetricsCalculator.test.ts(1 hunks)tests/src/hooks/useRecipeMetrics.test.ts(12 hunks)tests/src/types/brewSession.test.ts(1 hunks)tests/utils/deviceUtils.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- android/app/build.gradle
- app.json
- package.json
- android/app/src/main/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (8)
src/hooks/useRecipeMetrics.ts (1)
src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(15-372)
tests/utils/deviceUtils.test.ts (2)
src/services/config.ts (1)
STORAGE_KEYS(56-70)src/utils/deviceUtils.ts (1)
getPlatform(68-76)
tests/services/brewing/OfflineMetricsCalculator.test.ts (2)
src/types/recipe.ts (2)
RecipeMetricsInput(155-163)RecipeIngredient(44-76)src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(15-372)
src/types/recipe.ts (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
src/services/brewing/OfflineMetricsCalculator.ts (1)
src/types/recipe.ts (3)
RecipeFormData(136-152)RecipeMetricsInput(155-163)RecipeMetrics(79-91)
tests/src/hooks/useRecipeMetrics.test.ts (3)
src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(15-372)src/types/recipe.ts (1)
RecipeMetrics(79-91)src/hooks/useRecipeMetrics.ts (1)
useRecipeMetrics(17-142)
app/(tabs)/index.tsx (2)
src/services/offlineV2/UserCacheService.ts (1)
deleteBrewSession(1203-1368)src/services/api/queryClient.ts (2)
queryClient(59-89)QUERY_KEYS(100-141)
app/(modals)/(beerxml)/importReview.tsx (7)
src/services/offlineV2/UserCacheService.ts (1)
createRecipe(307-406)src/hooks/offlineV2/useUserData.ts (1)
useRecipes(18-276)src/hooks/offlineV2/index.ts (1)
useRecipes(16-16)src/types/recipe.ts (1)
RecipeMetricsInput(155-163)src/types/common.ts (1)
TemperatureUnit(29-29)src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(15-372)src/services/api/queryClient.ts (2)
queryClient(59-89)QUERY_KEYS(100-141)
🔇 Additional comments (24)
src/styles/modals/createRecipeStyles.ts (1)
1021-1024: BeerXML section padding looks consistent and low‑riskAdding
padding: 16tosectionshould improve readability and align this block with other padded cards/sections in the modal. No functional or layout red flags from this change alone.app/(tabs)/index.tsx (2)
103-108: LGTM!Good use of destructuring rename to avoid using the reserved keyword
deleteas an identifier.
430-442: No changes needed. ThebrewSession.idis already typed asstring(via theIDtype alias defined insrc/types/common.ts), and thedeleteBrewSessionfunction signature at line 1203 ofUserCacheService.tsexpectsid: string. The code is already type-safe. The recipe deletion's use ofString(recipe.id)is similarly redundant sincerecipe.idis also of typeID(string).tests/utils/deviceUtils.test.ts (1)
42-166: Thorough coverage of getDeviceId and UUID generation pathsThe tests around
getDeviceIdand UUID generation (existing ID, new ID viacrypto.randomUUID, expo-crypto fallback, SecureStore read/write failures, and RFC4122 v4 compliance checks) look comprehensive and align well with the expected behaviors, including logging and storage interactions.Also applies to: 204-259
src/types/common.ts (1)
29-29: LGTM!The centralized
TemperatureUnittype provides a single source of truth for temperature units across the codebase, improving type consistency.tests/src/types/brewSession.test.ts (1)
13-13: LGTM!The import restructuring correctly reflects the centralized
TemperatureUnittype definition in common.ts.src/types/brewSession.ts (1)
1-1: LGTM!The import change properly centralizes the
TemperatureUnittype, eliminating redundant type definitions.src/types/recipe.ts (3)
20-20: LGTM!The import correctly consolidates
TemperatureUnitfrom the centralized common types.
147-147: LGTM!The use of the centralized
TemperatureUnittype improves type consistency.
154-163: LGTM!The
RecipeMetricsInputinterface provides a well-defined, minimal data structure for offline metrics calculations. The optionalmash_temperatureandmash_temp_unitfields appropriately reflect that these are not strictly required for all calculation scenarios.src/hooks/useRecipeMetrics.ts (4)
7-11: LGTM!The updated documentation clearly communicates the offline-first approach and instant calculation behavior.
42-42: LGTM!The simplified query key correctly reflects the offline-first calculation approach while maintaining cache key discrimination based on recipe parameters.
76-104: LGTM!The offline-first calculation logic is clean and well-structured. The validation and error handling properly return fallback metrics when calculations fail, ensuring the UI always has usable data.
109-117: LGTM!The cache and retry configuration is perfectly suited for deterministic offline calculations. Setting
staleTime: Infinityand disabling refetch behaviors ensures optimal performance without unnecessary recalculations.tests/services/brewing/OfflineMetricsCalculator.test.ts (1)
1-572: Excellent test coverage!The test suite comprehensively covers both validation and calculation logic with excellent edge case handling:
Validation tests cover all input constraints (batch size, efficiency, boil time, ingredients, fermentables, amounts, alpha acid ranges, attenuation ranges).
Calculation tests cover diverse scenarios including basic calculations, unit conversions, missing yeast (0% attenuation), dry-hop exclusion from IBU, whirlpool/flameout hops, default values for missing properties, rounding precision, other fermentables (sugars/extracts), SRM clamping, and multiple yeast averaging.
The test data and assertions align well with the implementation and brewing formulas.
src/services/brewing/OfflineMetricsCalculator.ts (3)
8-13: LGTM!The addition of
RecipeMetricsInputto the imports enables the expanded type union for calculations.
19-21: LGTM!The union type
RecipeFormData | RecipeMetricsInputprovides flexibility by accepting either the full recipe form data or the minimal metrics input. Both types contain all fields required for calculations, making this a safe and practical extension.
223-226: LGTM!The validation function correctly accepts both input types, ensuring consistent validation regardless of whether the full recipe form or minimal metrics input is provided.
tests/src/hooks/useRecipeMetrics.test.ts (1)
5-21: LGTM - Mock setup correctly aligns with offline-first architecture.The OfflineMetricsCalculator mock properly exposes both
validateRecipeDataandcalculateMetricsstatic methods, matching the public API described in the relevant code snippets.app/(modals)/(beerxml)/importReview.tsx (2)
231-244: Good cache management with offline-first approach.The cache invalidation now correctly handles both online (
QUERY_KEYS.RECIPES) and offline (["userRecipes"],[...QUERY_KEYS.RECIPES, "offline"]) query keys, and primes the detail cache for immediate navigation.
254-259: Usingrouter.replaceprevents back-navigation to import flow.This is the correct behavior post-creation. The user should not be able to navigate back into the import review after the recipe is created.
tests/app/(modals)/(beerxml)/importReview.test.tsx (3)
6-6: Good move to centralize provider wiring viarenderWithProviders.Switching to
renderWithProviderskeeps tests closer to the real app tree and reduces per-test boilerplate; this pattern looks solid here.
172-183: Render assertions look aligned with the screen’s contract.Using
renderWithProvidersconsistently and asserting for user-facing text/test IDs (titles, batch size, metrics, action buttons, section containers) gives good confidence in the Import Review UI without over-coupling to implementation details.Also applies to: 193-269, 294-297, 305-332, 335-337, 345-357, 360-374, 383-411, 428-433
413-433: TemporaryuseQueryoverride is correctly restored.Capturing
originalMockviagetMockImplementation()and restoring it after the test avoids cross-test pollution while still exercising the metrics-present path; this pattern looks safe for the current mock structure.
- Fixed ingredient.time comparison in importReview.tsx - Fixed ineffective test assertion in useRecipeMetrics.test.ts - Fixed deviceUtils test to properly test undefined branch
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/(modals)/(beerxml)/importReview.tsx (1)
110-128: Remove debug console statements as previously requested.Lines 115-118 and 126 still contain
console.warnandconsole.errorcalls that were flagged for removal in a previous review. Replace these withUnifiedLoggercalls at appropriate levels or remove them entirely if they're purely for debugging.Apply this diff:
try { const validation = OfflineMetricsCalculator.validateRecipeData(recipeFormData); if (!validation.isValid) { - console.warn( - "[IMPORT_REVIEW] Invalid recipe data:", - validation.errors - ); + await UnifiedLogger.warn( + "beerxml", + "Invalid recipe data during import review", + validation.errors + ); return null; } const metrics = OfflineMetricsCalculator.calculateMetrics(recipeFormData); return metrics; } catch (error) { - console.error("[IMPORT_REVIEW] Metrics calculation failed:", error); + await UnifiedLogger.error( + "beerxml", + "Metrics calculation failed during import review", + error + ); return null; }
🧹 Nitpick comments (5)
tests/utils/deviceUtils.test.ts (3)
31-45: Make global.crypto mocking/restoration more robustThe current pattern overwrites
global.cryptowith a new descriptor inafterEach, which may change flags likeenumerableandconfigurableor create the property when it didn’t exist. A safer approach is to snapshot and restore the original descriptor (or delete the property if it was absent), which avoids subtle environment differences.You could refactor along these lines:
- let originalCrypto: any; + let originalCryptoDescriptor: PropertyDescriptor | undefined; beforeEach(() => { jest.clearAllMocks(); - originalCrypto = global.crypto; + originalCryptoDescriptor = Object.getOwnPropertyDescriptor(global, "crypto"); }); afterEach(() => { - // Restore original crypto - Object.defineProperty(global, "crypto", { - value: originalCrypto, - writable: true, - configurable: true, - }); + if (originalCryptoDescriptor) { + Object.defineProperty(global, "crypto", originalCryptoDescriptor); + } else { + // If crypto wasn’t originally defined, clean up any test-added value + // eslint-disable-next-line @typescript-eslint/no-explicit-any + delete (global as any).crypto; + } });
47-171: getDeviceId tests give strong coverage; consider asserting write attempt on set failureThe
getDeviceIdsuite nicely covers: existing ID, new ID withcrypto.randomUUID, expo-crypto fallback, and both read/write SecureStore failure paths. This is solid coverage for the main behaviors.As a small enhancement, in the
"SecureStore.setItemAsync failure"test you might also assert thatsetItemAsyncwas actually called (once) before warning, to guarantee you’re not only checking logging:const result = await getDeviceId(); @@ - expect(consoleWarnSpy).toHaveBeenCalled(); + expect(SecureStore.setItemAsync).toHaveBeenCalledTimes(1); + expect(consoleWarnSpy).toHaveBeenCalled();
204-220: Optionally add a positive-path getPlatform test for expected OS valuesThe
getPlatformtests cover fallback behavior for unknown andnullosName, which is good. IfgetPlatformis expected to return a specific value for Android (e.g."android"whenosName === "Android"), consider adding an explicit positive-path test for that case to lock in the intended mapping.tests/src/hooks/useRecipeMetrics.test.ts (1)
248-254: Strengthen assertion to verify no error state.While checking that
result.currentis defined and has properties is better than the previous ineffective assertion, the past review suggested explicitly verifying that the hook did not error. Add an assertion that confirmsisErrorisfalse.Apply this diff:
// Test should complete without errors despite null/undefined id/name fields expect(result.current).toBeDefined(); + expect(result.current.isError).toBe(false); // Verify the hook returns a valid query result object with expected properties expect(result.current).toHaveProperty("data");app/(modals)/(beerxml)/importReview.tsx (1)
96-100: Extract duplicate mash_temp_unit logic to a helper function.The mash temperature unit calculation logic is duplicated in both the metrics query (lines 96-100) and the recipe creation mutation (lines 157-161). Extract this to a helper function to reduce duplication and improve maintainability.
Add this helper at the top of the file after imports:
function deriveMashTempUnit( recipeData: any ): TemperatureUnit { return ( (recipeData.mash_temp_unit as TemperatureUnit) ?? (String(recipeData.batch_size_unit).toLowerCase() === "l" ? "C" : "F") ); }Then use it in both places:
const recipeFormData: RecipeMetricsInput = { batch_size: recipeData.batch_size || 5, batch_size_unit: recipeData.batch_size_unit || "gal", efficiency: recipeData.efficiency || 75, boil_time: recipeData.boil_time || 60, - mash_temp_unit: - (recipeData.mash_temp_unit as TemperatureUnit) ?? - (String(recipeData.batch_size_unit).toLowerCase() === "l" - ? "C" - : "F"), + mash_temp_unit: deriveMashTempUnit(recipeData), mash_temperature: /* ... */, ingredients: recipeData.ingredients, };And similarly in the mutation payload.
Also applies to: 157-161
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/(modals)/(beerxml)/importBeerXML.tsx(10 hunks)app/(modals)/(beerxml)/importReview.tsx(6 hunks)tests/src/hooks/useRecipeMetrics.test.ts(12 hunks)tests/utils/deviceUtils.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/(modals)/(beerxml)/importBeerXML.tsx (5)
src/services/beerxml/BeerXMLService.ts (1)
BeerXMLRecipe(12-15)src/types/common.ts (1)
UnitSystem(27-27)src/contexts/UnitContext.tsx (1)
useUnits(119-125)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/components/beerxml/UnitConversionChoiceModal.tsx (1)
UnitConversionChoiceModal(78-244)
tests/src/hooks/useRecipeMetrics.test.ts (4)
src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(15-372)src/types/recipe.ts (1)
RecipeMetrics(79-91)src/services/api/queryClient.ts (1)
queryClient(59-89)src/hooks/useRecipeMetrics.ts (1)
useRecipeMetrics(17-142)
🔇 Additional comments (17)
tests/utils/deviceUtils.test.ts (2)
173-202: getDeviceName tests correctly cover null/undefined and happy pathThe
getDeviceNametests now exercisenull,undefined, and a valid model name string, andmockModelNameis typed to allowundefined. This fully addresses the previous gap around theundefinedbranch and adds a clear happy-path assertion. No further changes needed here.
222-277: UUID generation tests are thorough and non-flakyThe UUID tests validate both the general RFC4122 format and the specific version/variant bits, and they correctly force the expo-crypto code path by clearing
crypto.randomUUID. Combined with the earlier fallback test, this gives strong confidence in the UUID generation logic without relying on randomness. Looks good.app/(modals)/(beerxml)/importBeerXML.tsx (5)
211-219: LGTM!The import-as-is flow is clean and correctly handles the case where no recipe is selected.
224-240: Good design for flexible recipe parameter.The optional
recipeparameter cleanly supports both the unit-conversion path (passing converted recipe) and the direct import path (using state). The fallback logic is correct.
245-257: LGTM!The reset properly includes all new state fields introduced for the unit conversion workflow.
493-509: LGTM!The modal is properly gated and all required props are correctly wired to state and handlers.
127-142: > Likely an incorrect or invalid review comment.tests/src/hooks/useRecipeMetrics.test.ts (5)
5-11: LGTM!The mock setup correctly provides jest.fn() implementations for both
validateRecipeDataandcalculateMetricsmethods.
144-148: LGTM!The mock return values correctly match the expected API shape of
validateRecipeData(returning{ isValid, errors }) andcalculateMetrics(returningRecipeMetrics).
334-336: LGTM!Good verification that
calculateMetricsreceives the complete recipe data structure.
390-393: LGTM!Query key assertions correctly verify the offline-first structure with expected parameters.
75-395: Good test coverage of offline-first metrics flow.The tests comprehensively cover:
- Query enablement conditions
- Invalid data filtering
- Empty response handling
- Complex ingredient scenarios
- Query key structure
All tests properly mock
OfflineMetricsCalculatorand verify the hook's behavior.app/(modals)/(beerxml)/importReview.tsx (5)
38-40: LGTM!The imports correctly bring in the offline-first dependencies needed for the new flow.
71-71: LGTM!Good use of
UnifiedLoggerfor error logging instead ofconsole.error.
134-135: LGTM!Perfect query configuration for offline-first deterministic calculations:
staleTime: Infinity(calculation never stales) andretry: false(no network retries needed).
208-225: LGTM! Good offline-first pattern.The flow correctly:
- Uses offline-first
createRecipe(creates temp recipe, then syncs to server)- Invalidates all relevant cache keys for consistency
- Prefills the detail cache for immediate navigation without additional fetch
This provides a smooth user experience with immediate feedback.
584-598: LGTM! Properly handles ingredient time coercion.Lines 591-596 correctly address a previous review concern by calling
coerceIngredientTimefirst, then checking if the result is positive before rendering. The IIFE pattern ensures type coercion happens in the right order.
- Extract duplicate mash_temp_unit logic to a helper function - Continue replacing console.error, console.warn and console.log statements with UnifiedLogger equivalents - Update tests to match new UnifiedLogger implementations throughout app - Make global.crypto mocking/restoration more robust
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/services/offlineV2/UserCacheService.ts (1)
198-210: UnifiedLogger methods are async and must always be awaited.The
UnifiedLogger.debug(),info(),warn(), anderror()methods are defined asasyncfunctions returningPromise<void>. Calls withoutawaitwill not wait for the logging operation to complete, potentially causing logs to be lost if the calling function exits before the promise settles.Throughout the file, these methods are called inconsistently—some with
await, others without:
- Lines 198–205:
await UnifiedLogger.debug(...)✓- Lines 207–210:
UnifiedLogger.debug(...)✗- Lines 241–244:
UnifiedLogger.debug(...)✗- Lines 289–302:
await UnifiedLogger.debug(...)✓- Lines 314, 418, 519, 701, 780:
UnifiedLogger.error(...)✗Add
awaitto allUnifiedLoggermethod calls. This is especially critical for error logging, where missed logs hinder debugging.src/contexts/NetworkContext.tsx (2)
308-327: Remove redundant logging calls.Lines 309-313 and 322-326 contain duplicate logging that already occurred on lines 301-307 and 317-321. The structured
UnifiedLogger[failures.length ? "warn" : "debug"]calls already log these events. Adding additionalUnifiedLogger.warncalls creates duplicate log entries.Apply this diff to remove the redundant calls:
void UnifiedLogger[failures.length ? "warn" : "debug"]( "NetworkContext.backgroundRefresh", failures.length ? `Background cache refresh had ${failures.length} failures` : "Background cache refresh completed", { results } ); - if (failures.length > 0) { - UnifiedLogger.warn( - "network", - "Background cache refresh had failures:", - failures - ); - } }) .catch(error => { void UnifiedLogger.error( "NetworkContext.backgroundRefresh", "Background cache refresh failed", { error: error instanceof Error ? error.message : String(error) } ); - UnifiedLogger.warn( - "network", - "Background cache refresh failed:", - error - ); });
368-373: Remove redundant logging call.Line 368-372 duplicates the error logging already performed on lines 360-367 via the structured
UnifiedLogger.errorcall.Apply this diff:
.catch(error => { void UnifiedLogger.error( "NetworkContext.handleStateChange", `Failed to sync pending operations when coming back online: ${error instanceof Error ? error.message : "Unknown error"}`, { error: error instanceof Error ? error.message : "Unknown error", } ); - UnifiedLogger.warn( - "network", - "Background sync of pending operations failed:", - error - ); });app/(modals)/(beerxml)/importReview.tsx (1)
164-174: Fixunit_systemdetection to be case‑insensitive and consistent.Here you check
recipeData.batch_size_unit === "l"(Line 164), but elsewhere you normalize with.toLowerCase()(e.g., inderiveMashTempUnit). If BeerXML parsing ever produces"L"or other metric tokens, this branch will incorrectly classify the recipe as"imperial".Suggest aligning this with the helper’s logic:
- unit_system: (recipeData.batch_size_unit === "l" - ? "metric" - : "imperial") as "metric" | "imperial", + unit_system: (String(recipeData.batch_size_unit).toLowerCase() === "l" + ? "metric" + : "imperial") as "metric" | "imperial",This keeps behavior consistent with
deriveMashTempUnitand avoids mis‑tagging metric recipes.
♻️ Duplicate comments (2)
app/(modals)/(beerxml)/importReview.tsx (2)
72-82: Unify logging: preferUnifiedLoggerover rawconsole.*in this flow.You’ve already wired
UnifiedLoggerfor the JSON parse failure (Line 78), but the rest of this screen still mixes inconsole.warn/console.error(metrics validation, ingredient validation, and mutation error). For consistency and easier observability control, I’d route these throughUnifiedLoggeras well.Example for the metrics block (Lines 121-139):
- const validation = - OfflineMetricsCalculator.validateRecipeData(recipeFormData); - if (!validation.isValid) { - console.warn( - "[IMPORT_REVIEW] Invalid recipe data:", - validation.errors - ); - return null; - } + const validation = + OfflineMetricsCalculator.validateRecipeData(recipeFormData); + if (!validation.isValid) { + UnifiedLogger.warn( + "[IMPORT_REVIEW] Invalid recipe data:", + validation.errors + ); + return null; + } @@ - } catch (error) { - console.error("[IMPORT_REVIEW] Metrics calculation failed:", error); + } catch (error) { + UnifiedLogger.error( + "[IMPORT_REVIEW] Metrics calculation failed:", + error + ); return null; }You can apply the same pattern for the ingredient‑filter errors (Lines 188-198) and the mutation error handler (Lines 261-263) so all logging on this screen flows through the same mechanism. Based on relevant_code_snippets for
UnifiedLogger, this should integrate cleanly.Also applies to: 121-139, 188-198, 261-263
589-605: Time rendering fix now safely handles string/invalid values.Switching to:
{(() => { const time = coerceIngredientTime(ingredient.time); return time !== undefined && time > 0 ? ` • ${time} min` : ""; })()}ensures you always coerce first and only render when the numeric result is positive, which avoids the earlier fragility around string
"0"and other non‑numeric inputs while preserving the existing “hide 0‑minute additions” behavior.
🧹 Nitpick comments (11)
src/services/offlineV2/StartupHydrationService.ts (3)
112-115: Static-data hydration logging is consistent; consider adding structured contextThe static-data logs cover all important branches (no cache vs already cached, background updates, completion) and use a consistent category/message style.
If you want richer diagnostics later, you could pass
ingredientsStats/ record counts as the optionaldataargument toUnifiedLogger.debuginstead of only embedding them in strings, to keep structured fields available to whatever backend you plug in.Also applies to: 120-129, 141-150, 160-163
50-52: Mix ofconsole.*andUnifiedLoggerfor errors/warningsYou now route debug-level messages through
UnifiedLoggerbut still emit errors/warnings viaconsole.error/console.warn. If logs are being centralized off-device, it may be worth gradually moving these toUnifiedLogger.error/.warnfor consistency and to avoid missing important failures in production.Also applies to: 99-103, 132-135, 153-157
178-181: Reset-hydration debug log is helpful; clarify fire-and-forget usage if neededThe reset log is useful for tests and logout flows. Since
UnifiedLogger.debugis async, you’re intentionally not awaiting it everywhere in this class; if your linting or error-handling strategy cares about unhandled rejections, consider either:
- wrapping calls in
void UnifiedLogger.debug(...)to make the fire-and-forget intent explicit, or- documenting that logger failures are intentionally ignored in this service.
Otherwise, behavior here looks fine.
app/(modals)/(recipes)/editRecipe.tsx (1)
388-404: Consider tightening the type/guard logic ingetMetricOrFallbackThe helper correctly prefers finite
metricsDatavalues and otherwise falls back to the existing recipe’sestimated_*fields. To make it a bit more robust (and slightly friendlier to strict TS), you could explicitly guard for numeric values:- const getMetricOrFallback = ( + const getMetricOrFallback = ( metricKey: keyof RecipeMetrics, estimatedKey: keyof Pick< Recipe, | "estimated_og" | "estimated_fg" | "estimated_abv" | "estimated_ibu" | "estimated_srm" > ): number | undefined => { const metricValue = metricsData?.[metricKey]; - if (metricValue !== undefined && Number.isFinite(metricValue)) { - return metricValue as number; - } + if (typeof metricValue === "number" && Number.isFinite(metricValue)) { + return metricValue; + } return existingRecipe?.[estimatedKey] as number | undefined; };This avoids ever treating non-numeric values (e.g. unexpected strings/nulls) as candidates and removes the need for the type assertion.
src/services/offlineV2/UserCacheService.ts (1)
47-50: Consider standardizing the logger category parameter.The category/context parameter passed to
UnifiedLoggervaries throughout the file:
- Some use module-level:
"offline-cache"- Others use method-specific:
"UserCacheService.methodName"Examples:
- Line 47:
UnifiedLogger.warn("offline-cache", ...)- Line 207:
UnifiedLogger.debug("offline-cache", ...)- Line 198:
await UnifiedLogger.debug("UserCacheService.getRecipes", ...)- Line 339:
await UnifiedLogger.info("UserCacheService.createRecipe", ...)While both approaches work, standardizing on one convention would make log filtering and searching more consistent. The method-specific format provides better traceability, while the module-level format provides easier grouping.
Also applies to: 114-117, 207-210, 241-244, 339-353, 442-453
src/services/beerxml/BeerXMLService.ts (2)
490-501: Potential data loss with spread operator ordering.When merging
convertedRecipeinto the original recipe, the spread order (...recipe, ...convertedRecipe) means any field inconvertedRecipewill overriderecipe. Then the explicit assignments (lines 493-500) overrideconvertedRecipewith nullish coalescing fallbacks.This logic is correct, but consider simplifying by only spreading convertedRecipe fields that exist, or documenting the intent more clearly.
454-511: Graceful fallback on conversion failure is appropriate.The
convertRecipeUnitsmethod correctly:
- Falls back to the original recipe on API errors
- Uses nullish coalescing to preserve falsy values like
0- The
unit_conversionworkflow is properly supported and documented in the type definitions (AIAnalysisRequestexplicitly notes this workflow accepts partial recipes)However, consider whether silently returning the original recipe on failure might confuse users who expect converted units. Adding explicit user notification would improve transparency about the fallback behavior.
tests/src/contexts/DeveloperContext.test.tsx (2)
216-220: Assertion usesexpect.any(String)for category - verify consistency.This assertion uses
expect.any(String)for the category, while line 170 uses the specific string"developer". If the production code uses a consistent category, consider making this assertion more specific to catch regressions if the category changes.- expect(UnifiedLogger.error).toHaveBeenCalledWith( - expect.any(String), + expect(UnifiedLogger.error).toHaveBeenCalledWith( + "developer", "Failed to set network simulation mode:", expect.any(Error) );
329-333: Same observation about category consistency.Similar to line 216, this assertion uses
expect.any(String)for category. Consider using the specific category string for consistency with line 170.app/(modals)/(beerxml)/importReview.tsx (2)
29-33: Helper + types for mash temp unit look solid (minor type-nit only).Pulling
TemperatureUnitfrom@src/typesand centralizing the mash temp unit logic inderiveMashTempUnitkeeps metrics and payload creation consistent and easier to reason about.If you find this helper spreading further, consider tightening the parameter type (e.g., a small
BeerXmlRecipeLikeinterface instead ofany) so you get compile‑time safety onmash_temp_unit/batch_size_unitaccesses, but that’s purely optional right now.Also applies to: 38-47
91-99: Coerce numeric fields forRecipeMetricsInputto avoid string/NaN issues.
RecipeMetricsInputdefinesbatch_size,efficiency, andboil_timeas numbers, andOfflineMetricsCalculator.validateRecipeDataexpects numeric input. IfrecipeDatacomes from JSON/BeerXML parsing with numeric fields as strings, the current||defaults will pass strings through to the calculator and may cause validation to fail or metrics to be skipped.You can defensively coerce here to ensure
RecipeMetricsInputis truly numeric:- const recipeFormData: RecipeMetricsInput = { - batch_size: recipeData.batch_size || 5, - batch_size_unit: recipeData.batch_size_unit || "gal", - efficiency: recipeData.efficiency || 75, - boil_time: recipeData.boil_time || 60, + const recipeFormData: RecipeMetricsInput = { + batch_size: Number(recipeData.batch_size) || 5, + batch_size_unit: recipeData.batch_size_unit || "gal", + efficiency: Number(recipeData.efficiency) || 75, + boil_time: coerceIngredientTime(recipeData.boil_time) ?? 60, mash_temp_unit: deriveMashTempUnit(recipeData), mash_temperature: typeof recipeData.mash_temperature === "number" ? recipeData.mash_temperature : String(recipeData.batch_size_unit).toLowerCase() === "l" ? 67 : 152, ingredients: recipeData.ingredients, };This keeps the UI defaults the same but makes the offline metrics path more robust if upstream parsing ever yields
"75"instead of75, etc.Also applies to: 105-119, 145-147
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (27)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importBeerXML.tsx(10 hunks)app/(modals)/(beerxml)/importReview.tsx(6 hunks)app/(modals)/(recipes)/createRecipe.tsx(1 hunks)app/(modals)/(recipes)/editRecipe.tsx(3 hunks)app/(tabs)/index.tsx(4 hunks)package.json(1 hunks)src/contexts/AuthContext.tsx(14 hunks)src/contexts/DeveloperContext.tsx(8 hunks)src/contexts/NetworkContext.tsx(7 hunks)src/contexts/UnitContext.tsx(8 hunks)src/services/NotificationService.ts(14 hunks)src/services/api/apiService.ts(6 hunks)src/services/api/queryClient.ts(1 hunks)src/services/beerxml/BeerXMLService.ts(9 hunks)src/services/offlineV2/StartupHydrationService.ts(9 hunks)src/services/offlineV2/StaticDataService.ts(22 hunks)src/services/offlineV2/UserCacheService.ts(81 hunks)src/services/storageService.ts(12 hunks)tests/app/(modals)/(beerxml)/importReview.test.tsx(11 hunks)tests/src/contexts/DeveloperContext.test.tsx(5 hunks)tests/src/contexts/UnitContext.test.tsx(6 hunks)tests/src/hooks/useRecipeMetrics.test.ts(12 hunks)tests/src/services/NotificationService.test.ts(12 hunks)tests/utils/deviceUtils.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/utils/deviceUtils.test.ts
- package.json
- android/app/src/main/res/values/strings.xml
- app/(modals)/(beerxml)/importBeerXML.tsx
- android/app/build.gradle
🧰 Additional context used
🧬 Code graph analysis (18)
src/services/api/apiService.ts (1)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)
tests/src/contexts/UnitContext.test.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
tests/src/contexts/DeveloperContext.test.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/services/NotificationService.ts (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
tests/src/hooks/useRecipeMetrics.test.ts (3)
src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(15-372)src/types/recipe.ts (1)
RecipeMetrics(79-91)src/hooks/useRecipeMetrics.ts (1)
useRecipeMetrics(17-142)
src/services/offlineV2/StartupHydrationService.ts (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/contexts/NetworkContext.tsx (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
app/(modals)/(recipes)/createRecipe.tsx (1)
src/services/api/queryClient.ts (2)
queryClient(59-89)QUERY_KEYS(100-142)
tests/app/(modals)/(beerxml)/importReview.test.tsx (3)
tests/testUtils.tsx (2)
testUtils(292-368)renderWithProviders(30-85)app/(modals)/(beerxml)/importReview.tsx (1)
ImportReviewScreen(63-649)src/constants/testIDs.ts (1)
TEST_IDS(145-280)
src/services/beerxml/BeerXMLService.ts (3)
src/types/recipe.ts (1)
Recipe(94-133)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/common.ts (1)
UnitSystem(27-27)
src/contexts/UnitContext.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/contexts/AuthContext.tsx (3)
src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/UnifiedLogger.ts (2)
error(315-325)UnifiedLogger(11-428)src/services/logger/DevLogger.ts (1)
error(141-148)
app/(tabs)/index.tsx (2)
src/services/offlineV2/UserCacheService.ts (1)
deleteBrewSession(1244-1413)src/services/api/queryClient.ts (2)
queryClient(59-89)QUERY_KEYS(100-142)
app/(modals)/(recipes)/editRecipe.tsx (1)
src/types/recipe.ts (2)
RecipeMetrics(79-91)Recipe(94-133)
src/contexts/DeveloperContext.tsx (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
tests/src/services/NotificationService.test.ts (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/services/offlineV2/StaticDataService.ts (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
app/(modals)/(beerxml)/importReview.tsx (6)
src/types/common.ts (1)
TemperatureUnit(29-29)src/services/offlineV2/UserCacheService.ts (1)
createRecipe(322-421)src/hooks/offlineV2/useUserData.ts (1)
useRecipes(18-276)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/recipe.ts (1)
RecipeMetricsInput(155-163)src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(15-372)
🔇 Additional comments (46)
app.json (1)
6-6: Version alignment verified across all configuration files.All three version fields in
app.jsonare consistently updated to 3.3.4/196:
version: 3.3.4 ✓versionCode(Android): 196 ✓runtimeVersion: 3.3.4 ✓Cross-file consistency confirmed:
package.json: 3.3.4 ✓android/app/build.gradle: versionCode 196, versionName 3.3.4 ✓The semantic versioning bump (3.2.6 → 3.3.4) is appropriate for the feature additions in this PR. The versionCode increment of +5 (191→196) is confirmed across all config files; however, whether this represents multiple development builds or intentional skipping cannot be determined from configuration alone.
src/services/offlineV2/StartupHydrationService.ts (2)
10-37: UnifiedLogger integration in startup guard + entry log looks correctThe new import and debug calls around the
isHydrating/hasHydratedguard and startup entry read clean and give useful trace points; category and messages are consistent.
46-49: User-data hydration logging is well-placed and informativeThe added debug logs around user-data hydration (start, cache-miss vs cache-hit, and completion) accurately reflect control flow and should make it much easier to reason about offline behavior in the field.
Also applies to: 66-69, 78-88, 94-97
app/(modals)/(recipes)/editRecipe.tsx (2)
19-24: ImportingRecipeMetricsis consistent with new metrics handlingThe added
RecipeMetricsimport is used solely for typing ingetMetricOrFallbackand fits the existing@src/typesmodel cleanly; no issues here.
442-448: Estimated metrics wiring looks correct; double‑checkundefinedserialization behaviorUsing
getMetricOrFallbackto populateestimated_og/fg/abv/ibu/srmensures you:
- Prefer freshly calculated metrics when they’re finite.
- Preserve previously stored
estimated_*values when recalculation fails or yields non‑finite results.One thing to verify is how
updateRecipeV2and the backing API/storage handleundefinedvalues on these fields when both sources are absent: ifundefinedfields are omitted from the payload (typicalJSON.stringifybehavior), this will leave existing values untouched; if they’re explicitly persisted asnullor similar, you might unintentionally clear estimates. If it’s the latter, you may want an explicit step to strip keys whose fallback isundefinedbefore sending.src/services/offlineV2/UserCacheService.ts (1)
1-5159: LGTM! Comprehensive logging migration to UnifiedLogger.The systematic replacement of
console.*calls withUnifiedLogger.*throughout the entire file successfully centralizes the logging strategy without altering any functional behavior. The error objects are properly passed as parameters, and the structured logging format with category parameters will improve log filtering and analysis.The migration approach is sound and aligns with the broader PR objective of adopting
UnifiedLoggeracross the codebase.src/services/offlineV2/StaticDataService.ts (4)
17-17: LGTM!The UnifiedLogger import is correctly added and follows project conventions.
159-163: LGTM!Warning logging replacements are consistent with the error logging pattern and maintain appropriate context.
Also applies to: 216-220, 333-335, 613-616, 624-628
405-408: LGTM!Debug logging replacements correctly maintain the
__DEV__guards to prevent logging in production builds.Also applies to: 418-421, 431-434, 453-456
470-474: Good improvement to error context.The structured error data
{ type: typeof beerStylesData, data: beerStylesData }improves debuggability by including type information alongside the unexpected data.src/contexts/NetworkContext.tsx (1)
169-173: LGTM! Clean migration to UnifiedLogger.The replacement of
console.warnwithUnifiedLogger.warnfor initialization errors is well-executed, with appropriate category tagging and error context.src/contexts/UnitContext.tsx (1)
162-166: LGTM! Consistent UnifiedLogger migration.All console logging has been cleanly replaced with UnifiedLogger calls using the "units" category. Error context is preserved, and the structured logging approach improves observability.
Also applies to: 188-192, 229-233, 262-266, 308-311, 326-326, 427-430
src/services/NotificationService.ts (1)
106-109: LGTM! Clean logging migration.All console logging has been properly replaced with UnifiedLogger using the "notifications" category. Error handling flows remain unchanged, and development-only logging is appropriately gated.
Also applies to: 126-130, 169-173, 234-238, 323-327, 341-345, 368-372, 405-409, 421-425, 439-443, 525-528
tests/src/contexts/UnitContext.test.tsx (1)
26-34: LGTM! Test coverage properly updated.The test suite correctly mocks UnifiedLogger and updates assertions to verify structured logging calls. Mock clearing in
beforeEachprevents test pollution.Also applies to: 90-94, 442-445, 528-531, 987-991, 1056-1060
src/contexts/DeveloperContext.tsx (1)
159-163: LGTM! Consistent developer context logging.All console logging replaced with UnifiedLogger using the "developer" category. Error handling flows remain intact, and structured logging provides better context for debugging.
Also applies to: 227-231, 298-302, 325-329, 351-351, 394-394, 424-424, 492-492
src/services/storageService.ts (1)
96-100: LGTM! Comprehensive storage logging migration.All console logging replaced with UnifiedLogger using the "storage" category. Input validation errors, size limit warnings, and error handling flows are properly preserved with enhanced structured logging.
Also applies to: 351-355, 432-436, 459-459, 467-470, 488-488, 537-540, 548-551, 569-569, 628-632, 649-653, 671-671, 710-710
tests/src/services/NotificationService.test.ts (1)
56-64: LGTM! Complete test coverage for logging migration.Test suite properly mocks UnifiedLogger and verifies all structured logging calls. Mock setup and clearing in
beforeEachensures test isolation.Also applies to: 90-94, 137-140, 178-182, 246-250, 389-393, 417-421, 448-452, 526-530, 553-557, 628-631
src/contexts/AuthContext.tsx (1)
364-373: LGTM! Auth context logging migration complete.All console logging replaced with UnifiedLogger using the "auth" category. Error handling flows are preserved, and background task failures are properly logged without blocking critical paths.
Also applies to: 565-574, 679-679, 698-698, 838-838, 862-862, 875-875, 900-904, 914-914, 933-933, 956-960, 1094-1094, 1108-1108, 1134-1134
src/services/beerxml/BeerXMLService.ts (3)
1-4: LGTM! Import changes are appropriate.The addition of
UnitSystemtype from@src/typesandUnifiedLoggerfrom the logger service aligns with the new unit detection/conversion functionality and the project-wide logging migration.
13-16: LGTM! ExportingBeerXMLRecipeinterface.Making this interface public enables consumers (like the import screen and tests) to use the type for unit conversion workflows.
408-448: Unit detection logic is well-structured.The
detectRecipeUnitSystemmethod correctly:
- Normalizes units with
toLowerCase().trim()- Counts metric vs imperial units from batch size and ingredients
- Returns "mixed" when counts are equal or both zero
One minor observation: the logic returns the predominant system even with a single-unit difference. This seems reasonable for practical use.
app/(modals)/(recipes)/createRecipe.tsx (1)
280-280: LGTM! Consistent use of centralized query keys.Replacing the string literal with
QUERY_KEYS.USER_RECIPESaligns with the existing pattern used for other cache invalidations in this block (lines 281-285). This improves maintainability and type safety.src/services/api/apiService.ts (4)
31-31: LGTM! UnifiedLogger import added.The import path
@/src/services/logger/UnifiedLoggeris consistent with the project's path aliasing conventions.
373-373: LGTM! API error logging migrated to UnifiedLogger.The change from inline console logging to
UnifiedLogger.error("api", ...)centralizes error logging while maintaining the same information payload.
446-477: LGTM! Token management error logging migrated.All three TokenManager methods (
getToken,setToken,removeToken) now useUnifiedLogger.errorconsistently. Error handling behavior is preserved—errors are still thrown after logging insetTokenand silently handled in the others.
508-512: LGTM! Developer mode warning logging migrated.The change from
console.warntoUnifiedLogger.warnmaintains the same behavior while using the unified logging infrastructure.tests/src/contexts/DeveloperContext.test.tsx (2)
30-39: LGTM! UnifiedLogger mock is correctly structured.The mock properly:
- Sets
__esModule: truefor ES module compatibility- Exports as
defaultmatching the import style- Provides all four logging methods as
jest.fn()for assertion
170-174: LGTM! Test assertion updated for UnifiedLogger.warn.The assertion correctly verifies that
UnifiedLogger.warnis called with the expected category ("developer"), message, and error object when storage errors occur during settings loading.src/services/api/queryClient.ts (1)
108-108: LGTM! NewUSER_RECIPESquery key added.The new constant follows the established naming convention and provides a type-safe key for user-specific recipe cache invalidation. This supports the centralized query key usage pattern seen in
createRecipe.tsx.app/(tabs)/index.tsx (4)
107-108: LGTM!The destructuring of
deleteBrewSessionfromuseBrewSessionscorrectly exposes the new deletion capability for use in the context menu handler.
219-222: LGTM!Proper query invalidation for recipe deletion now includes
RECIPES,DASHBOARD, andUSER_RECIPES, ensuring all related cached data is refreshed.
248-249: LGTM!Query invalidation for clone is consistent with the pattern used elsewhere in the file.
416-446: Brew session cache is properly updated via the hook'sloadDatacallback.The original concern about missing query invalidation is based on a false premise. Recipe deletion uses a React Query mutation with explicit
queryClient.invalidateQueries()calls, while brew session deletion uses a different but equally valid pattern: thedeleteSessioncallback fromuseBrewSessionsautomatically callsloadData(false)after deletion, which refetches the data from the cache service and updates the component's local state. This immediately reflects the deletion in the UI without requiring manual query invalidation. Both patterns handle cache updates correctly, just through different mechanisms—recipes use React Query's mutation system, while brew sessions use the hook's internal state management.tests/app/(modals)/(beerxml)/importReview.test.tsx (6)
6-6: LGTM!Correctly imports the centralized test utilities for consistent provider wrapping across tests.
10-27: LGTM!The Appearance and ThemeContext mocks provide the necessary simulation of color scheme behavior and theme data for the tests.
43-45: Good documentation.Helpful comment clarifying that react-query mocking is handled centrally in setupTests.js, avoiding redundant overrides.
56-109: LGTM!Comprehensive context mocks provide controlled test environments for Auth, Network, Developer, Unit, and Calculators contexts. Each mock correctly includes both the hook and the provider component.
112-125: LGTM!Proper test isolation with
testUtils.resetCounters()andsetMockAuthState()inbeforeEachensures each test starts with a clean state.
127-139: LGTM!Tests correctly updated to use
renderWithProvidersfor consistent provider wrapping.tests/src/hooks/useRecipeMetrics.test.ts (6)
5-21: LGTM!Clean migration from API service mocking to
OfflineMetricsCalculatormocking. The mock structure correctly exposesvalidateRecipeDataandcalculateMetricsmethods, andjest.mocked()provides proper TypeScript typing for the mock.
101-101: LGTM!Assertions correctly updated to verify
mockedCalculator.calculateMetricswas not called when the query is disabled.
144-148: LGTM!Proper mock setup pattern for the calculator - validating recipe data first, then calculating metrics.
248-255: Previous review issue addressed.The assertions now properly verify the hook result instead of using the ineffective
expect(() => result.current).not.toThrow()pattern. The new assertions checkresult.current.isError,toHaveProperty("data"), etc., which meaningfully validate the hook's behavior with edge-case inputs.
334-337: LGTM!Good addition to verify the calculator is called with the correct recipe data, ensuring the integration between the hook and calculator is properly tested.
391-402: LGTM!Query key structure verification correctly updated to reflect the new "offline-first" mode, with proper destructuring and individual assertions for each key segment.
app/(modals)/(beerxml)/importReview.tsx (1)
202-211: Ingredient mapping looks correct and time coercion is appropriate.The
IngredientInputmapping correctly:
- Enforces required fields via the preceding
filter.- Normalizes
amountto a number.- Uses
coerceIngredientTimefortimeso downstream consumers get either a non‑negative number orundefined.- Generates a unique
instance_idfor each imported ingredient.This should play nicely with both the offline metrics calculator and the offline recipe store.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/offlineV2/StaticDataService.ts (1)
474-483: AwaitUnifiedLogger.errorbefore throwingOfflineErrorfor unexpected beer styles format.Here you log an unexpected data format and then immediately throw:
UnifiedLogger.error( "offline-static", `[StaticDataService.fetchAndCacheBeerStyles] Unexpected data format:`, { type: typeof beerStylesData, data: beerStylesData } ); throw new OfflineError(...);Every other
UnifiedLogger.errorin this file is awaited, so this one stands out and could drop logs in edge cases before the exception propagates. Suggest aligning with the rest of the file:- UnifiedLogger.error( + await UnifiedLogger.error( "offline-static", `[StaticDataService.fetchAndCacheBeerStyles] Unexpected data format:`, { type: typeof beerStylesData, data: beerStylesData } );This keeps the logging behavior consistent with earlier review feedback on
StaticDataService.To double‑check consistency, you can run:
#!/bin/bash rg "UnifiedLogger.error" src/services/offlineV2/StaticDataService.ts -n -C2
🧹 Nitpick comments (7)
src/services/offlineV2/StaticDataService.ts (1)
433-451: Guard this debug log with__DEV__for consistency and to avoid noisy prod logs.Other
UnifiedLogger.debugcalls infetchAndCacheBeerStylesare wrapped inif (__DEV__), but this one is not:if (Array.isArray(beerStylesData)) { // If it's already an array, process normally UnifiedLogger.debug( "offline-static", `[StaticDataService.fetchAndCacheBeerStyles] Processing array format with ${beerStylesData.length} items` ); beerStylesData.forEach(…); }Unless
UnifiedLogger.debugalready suppresses logs in production, this will emit on every array fetch while others won’t. For consistency and to reduce production noise, consider:if (Array.isArray(beerStylesData)) { // If it's already an array, process normally - UnifiedLogger.debug( - "offline-static", - `[StaticDataService.fetchAndCacheBeerStyles] Processing array format with ${beerStylesData.length} items` - ); + if (__DEV__) { + UnifiedLogger.debug( + "offline-static", + `[StaticDataService.fetchAndCacheBeerStyles] Processing array format with ${beerStylesData.length} items` + ); + } beerStylesData.forEach((item: any) => {src/contexts/NetworkContext.tsx (2)
230-273: De-duplicateNetworkContext.handleStateChangelogs to avoid redundant entriesRight now every state change logs twice with the same category/message/payload:
- Line 231: unconditional
UnifiedLogger.info("NetworkContext.handleStateChange", "Network state change detected", …).- Lines 245-257 / 259-271: conditional
infovsdebugwith the same message and data.This will emit two nearly identical log entries per state change. Suggest logging only once and varying the level based on whether we changed online/offline status, e.g.:
- // Log network state changes - void UnifiedLogger.info( - "NetworkContext.handleStateChange", - "Network state change detected", - { - connected, - reachable, - type, - wasOffline, - isNowOnline, - shouldRefresh, - previousOnlineState: previousOnlineState.current, - } - ); - if (previousOnlineState.current !== isNowOnline) { - void UnifiedLogger.info( - "NetworkContext.handleStateChange", - "Network state change detected", - { - connected, - reachable, - type, - wasOffline, - isNowOnline, - shouldRefresh, - previousOnlineState: previousOnlineState.current, - } - ); - } else { - void UnifiedLogger.debug( - "NetworkContext.handleStateChange", - "Network state change detected", - { - connected, - reachable, - type, - wasOffline, - isNowOnline, - shouldRefresh, - previousOnlineState: previousOnlineState.current, - } - ); - } + // Log network state changes (info when status flips, debug otherwise) + const logMethod = previousOnlineState.current !== isNowOnline ? "info" : "debug"; + void UnifiedLogger[logMethod]( + "NetworkContext.handleStateChange", + "Network state change detected", + { + connected, + reachable, + type, + wasOffline, + isNowOnline, + shouldRefresh, + previousOnlineState: previousOnlineState.current, + } + );
169-173: Normalize UnifiedLogger Promise handling (voidvsawait) in catch blocksIn these catch paths you call
UnifiedLogger.warnwithoutawaitor avoidprefix:
- Lines 169-173: initialization failure.
- Lines 385-386: caching failure.
- Lines 410-414: cache-load failure.
- Line 426: manual refresh failure.
Elsewhere in this file (
info/debug/errorcalls) you consistently usevoid UnifiedLogger.*(...), which keeps lints happy about “floating promises” while intentionally fire‑and‑forgetting logging.For consistency (and to avoid potential
no-floating-promisescomplaints), consider eitherawaiting or prefixing these calls withvoid. Keeping them fire‑and‑forget but consistent with the rest of the file would look like:- UnifiedLogger.warn( + void UnifiedLogger.warn( "network", "Failed to initialize network monitoring:", error ); ... - } catch (error) { - UnifiedLogger.warn("network", "Failed to cache network state:", error); + } catch (error) { + void UnifiedLogger.warn("network", "Failed to cache network state:", error); ... - } catch (error) { - UnifiedLogger.warn( + } catch (error) { + void UnifiedLogger.warn( "network", "Failed to load cached network state:", error ); ... - } catch (error) { - UnifiedLogger.warn("network", "Failed to refresh network state:", error); + } catch (error) { + void UnifiedLogger.warn("network", "Failed to refresh network state:", error);Also applies to: 385-386, 410-415, 425-427
app/(modals)/(beerxml)/importReview.tsx (4)
29-33: HardenderiveMashTempUnitagainst invalid/empty values and DRY unit logic
deriveMashTempUnitcurrently passes through any non‑nullishrecipeData.mash_temp_unit(including""or unexpected values), which can leak an invalidTemperatureUnitinto both metrics and creation payloads. You’re already deriving a sensible default frombatch_size_unit, so it would be safer to normalize/whitelist:
- Only accept
"C"/"F"(and maybe case‑normalize).- For anything else (empty string, unknown symbol), fall back to the batch‑size‑based default.
You could also centralize the
"l" => metriccheck into this helper and reuse it whereunit_systemand default mash temperatures are derived to avoid subtle drift.Also applies to: 38-40, 42-47
75-81: Make JSON parse error logging consistent with other UnifiedLogger callsHere you call:
UnifiedLogger.error("Failed to parse recipe data:", error as string);Elsewhere you use a
(context, message, payload)shape (e.g."import-review", "Metrics calculation failed", error). For consistency and better diagnostics (stack traces, structured payloads), consider using the same signature here and passing the raw error object instead ofas string:UnifiedLogger.error("import-review", "Failed to parse recipe data", error);This keeps logging uniform and avoids losing error metadata.
91-99: Consider enriching the metricsqueryKeyand differentiating validation vs internal errorsThe offline metrics flow looks solid overall, but two behavior points are worth a look:
queryKeygranularity
The key only fingerprints a few scalar fields plus the list ofingredient_ids. Metrics typically also depend on amounts/units; if those change while IDs stay the same, React Query may treat results as cache hits and skip recalculation. If users can revisit this screen after editing ingredient amounts, consider including a fuller fingerprint (e.g.JSON.stringify(recipeData.ingredients), or at least IDs + amounts) in the key.
metricsErroris effectively unused
Both validation failures and calculator exceptions are normalized tonull, so the UI almost always falls into the “No metrics calculated” branch rather than the “Failed to calculate metrics” error state. If you intend to surface genuine calculator failures differently from “no metrics”, you might want to:
- Return
nullfor validation failures, but- Re‑throw (or throw a wrapped error) in the
catchpath sometricsErroris set and the error banner is shown.That would make the UI more explicit about “bad data” vs “unexpected internal failure”.
Also applies to: 105-107, 111-112, 118-119, 121-152
169-174: Centralize ingredient normalization between metrics and creation, and reuse unit logicThe mutation flow and logging look good, but there’s some duplication and drift risk:
- You build
RecipeMetricsInputusing the rawrecipeData.ingredients, while creation uses a filtered + normalizedIngredientInput[](ID presence, required fields, numeric amount, coerced time, generatedinstance_id). That means metrics are computed on a different shape than what actually gets persisted.- You also derive
unit_systemhere using the same"l" => metricheuristic already embedded inderiveMashTempUnit.To reduce divergence over time:
- Consider extracting a small helper, e.g.
normalizeImportedIngredients(recipeData.ingredients): IngredientInput[], that applies the same validation/logging you use in the mutation. You can then:
- Use that array both in
RecipeMetricsInput.ingredientsand in therecipePayload.- Optionally, centralize
unit_systemderivation alongsidederiveMashTempUnitso any future changes in unit detection are applied consistently.This keeps metrics, payload, and logging aligned and makes it easier to evolve import rules in one place.
Also applies to: 193-205, 209-213, 218-228, 232-235, 239-249, 259-262, 279-280
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(8 hunks)package.json(1 hunks)src/contexts/NetworkContext.tsx(4 hunks)src/services/offlineV2/StaticDataService.ts(22 hunks)src/services/offlineV2/UserCacheService.ts(81 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- android/app/build.gradle
- src/services/offlineV2/UserCacheService.ts
- android/app/src/main/res/values/strings.xml
🧰 Additional context used
🧬 Code graph analysis (2)
src/services/offlineV2/StaticDataService.ts (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
src/contexts/NetworkContext.tsx (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
🔇 Additional comments (3)
app.json (1)
6-6: Version metadata updates look good.The version bump from 3.2.6 to 3.3.5, versionCode increment from 191 to 197, and matching runtimeVersion update are consistent and aligned with the feature integration objective. The AI summary confirms related version updates across android/app/build.gradle, strings.xml, and package.json, so cross-file consistency is maintained.
Also applies to: 19-19, 61-61
src/services/offlineV2/StaticDataService.ts (1)
17-17: UnifiedLogger migration in StaticDataService looks consistent and preserves behavior.The switch from
console.*toUnifiedLogger(with structured payloads and preserved error throwing) is coherent across methods, matches the service’s error semantics, and improves observability without changing public APIs.Also applies to: 72-86, 110-124, 172-205, 232-252, 257-313, 320-401, 406-512, 517-573, 578-636
app/(modals)/(beerxml)/importReview.tsx (1)
608-622: Ingredient time rendering now robust and type‑safeThe new ingredient details block correctly funnels
ingredient.timethroughcoerceIngredientTimebefore comparison, and only renders the “• X min” suffix when the coerced value is positive. This resolves the earlier fragility around string times like"0"and non‑numeric values while keeping the UI clean for zero/absent times.
…nt entries - Properly gate UnifiedLogger debug calls in StaticDataService to __DEV__ - Change all UnifiedLogger warning calls in NetworkContext to be fire-and-forget - Make JSON parse error logging consistent with other UnifiedLogger calls - Improve offline metric calculation by respecting provided mash temp and falling back to unit system defaults when not
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/types/recipe.ts (1)
143-143: Inconsistent type usage: UseUnitSysteminstead of literal union.
RecipeFormData.unit_systemuses the literal union"imperial" | "metric"whileRecipe.unit_system(line 102) uses the centralizedUnitSystemtype. This inconsistency can cause type mismatches when converting form data to recipe entities.Apply this diff to align with the centralized type:
- unit_system: "imperial" | "metric"; + unit_system: UnitSystem;
♻️ Duplicate comments (1)
src/services/offlineV2/StaticDataService.ts (1)
159-163: Consider awaitingUnifiedLogger.warn/debugfor consistency (or documenting fire‑and‑forget usage).In these places,
UnifiedLogger.warn/.debugare called withoutawait, while other services and this file’s error paths generally await logger calls. For consistency and to avoid silently dropped logs if the logger ever rejects, you may want toawaitthese as well, or explicitly treat them as intentionally fire‑and‑forget (especially in background flows).Also applies to: 216-220, 337-340, 408-413, 421-426, 435-440, 459-462, 619-622, 630-634
🧹 Nitpick comments (4)
src/hooks/offlineV2/useUserData.ts (1)
62-62: Consider migrating error logging to UnifiedLogger for consistency.While success and info messages use
UnifiedLogger.info, error scenarios still useconsole.errorat multiple locations. For consistent, centralized logging, consider usingUnifiedLogger.error(if available) orUnifiedLogger.infowith an error context.Example for line 62:
- console.error("Error loading recipes:", err); + await UnifiedLogger.error("useRecipes.loadData", "Error loading recipes", { error: err });Apply similar changes to lines 218, 335, 482, and 504 to complete the migration to UnifiedLogger mentioned in the PR summary.
Also applies to: 218-218, 335-335, 482-482, 504-504
src/contexts/NetworkContext.tsx (2)
169-173: Consider usingerrorlevel for initialization failure loggingFailure to initialize network monitoring is fairly serious for an app that depends on connectivity awareness. You already fall back to a safe optimistic state, but it may be worth logging this as
UnifiedLogger.error(and/or using a more specific category like"NetworkContext.initialize") so these issues stand out in observability dashboards instead of being buried among warnings.
358-362: Align log levels for cache/load/refresh failures with their impactYou’re now consistently routing storage and refresh failures through
UnifiedLogger.warn, which is good for centralizing visibility. It may be worth slightly tuning severity by case:
- For silent degradations like failing to cache or load prior network state (358-362, 387-391),
warnis reasonable since the app falls back gracefully.- For
refreshNetworkStatefailures (403-407), you immediately rethrow the error; treating this aserrorinstead ofwarncould better reflect that callers see a hard failure.Not mandatory, but tightening severity here might make production diagnostics clearer.
Also applies to: 387-391, 403-407
app/(modals)/(beerxml)/importReview.tsx (1)
92-155: Offline-first metrics calculation is well-structured.The refactored metrics calculation properly uses
OfflineMetricsCalculatorwith appropriate query configuration (staleTime: Infinityfor deterministic results,retry: falsefor local calculations). The validation flow and error handling withUnifiedLoggerare solid.However, note the temperature unit inference issue flagged in
deriveMashTempUnitapplies here as well (line 117).Additionally, lines 119-120 duplicate the unit-to-temperature inference logic:
recipeData.mash_temperature ?? (String(recipeData.batch_size_unit).toLowerCase() === "l" ? 67 : 152),Consider extracting a
deriveMashTemperaturehelper that returns the default temperature value based on the unit system, similar toderiveMashTempUnit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(8 hunks)package.json(1 hunks)src/contexts/NetworkContext.tsx(5 hunks)src/hooks/offlineV2/useUserData.ts(1 hunks)src/services/brewing/OfflineMetricsCalculator.ts(3 hunks)src/services/offlineV2/StaticDataService.ts(22 hunks)src/types/recipe.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (4)
src/contexts/NetworkContext.tsx (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
src/services/offlineV2/StaticDataService.ts (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
src/services/brewing/OfflineMetricsCalculator.ts (2)
src/types/recipe.ts (3)
RecipeFormData(136-152)RecipeMetricsInput(155-163)RecipeMetrics(79-91)src/utils/recipeUtils.ts (1)
isDryHopIngredient(44-48)
src/types/recipe.ts (1)
src/types/common.ts (2)
UnitSystem(27-27)TemperatureUnit(29-29)
🔇 Additional comments (14)
android/app/build.gradle (1)
95-96: Version metadata is consistent and appropriately bumped.The versionCode (191→198) and versionName ("3.2.6"→"3.3.6") changes align with the Expo config and resource strings, reflecting a coordinated version bump across the platform. The minor version increment (3.2.6→3.3.6) is semantically appropriate for the feature additions (unit conversion, offline metrics workflow, BeerXML enhancements) noted in the PR summary.
Note: The versionCode jump of +7 is larger than typical patch increments. Verify this aligns with your internal build/QA process and branching strategy (e.g., if multiple builds were cut from this branch during development).
app.json (1)
6-6: Expo configuration version updates are consistent and correctly aligned.The version, versionCode, and runtimeVersion fields are all updated consistently (3.3.6 and 198). The runtimeVersion change correctly signals to Expo that a new OTA runtime update is available, ensuring Expo clients will fetch the updated bundle.
Also applies to: 19-19, 61-61
android/app/src/main/res/values/strings.xml (1)
4-4: Resource string correctly aligned with Expo runtimeVersion.The expo_runtime_version string resource is updated to match the runtimeVersion in app.json, ensuring consistency between native Android config and Expo metadata.
src/hooks/offlineV2/useUserData.ts (1)
13-13: No action required. The import change to named import (import { UnifiedLogger }) is valid—the module exports both named and default exports. This import pattern is also the dominant style across the codebase (28 of 33 files), making it consistent with project conventions.src/services/offlineV2/StaticDataService.ts (1)
17-17: Centralized, awaited error logging withUnifiedLoggerlooks good.Importing
UnifiedLoggerand switching the error paths in these catch blocks toawait UnifiedLogger.error(...)standardizes logging, preserves the existingOfflineError/VersionErrorcontracts, and addresses the earlier concern about fire‑and‑forget error logs in this service. No functional issues spotted here.Also applies to: 76-80, 114-118, 176-180, 195-199, 241-245, 293-297, 390-394, 476-480, 503-507, 526-530, 545-549, 568-572
src/contexts/NetworkContext.tsx (1)
230-233: Dynamic log level selection for state changes looks solidUsing
logMethodto switch betweeninfoon status flips anddebugfor non-flip updates keeps a single logging call site and avoids duplication while still distinguishing important transitions from noise. This is a clean pattern and reads clearly.src/types/recipe.ts (2)
20-24: LGTM: Centralized type imports improve consistency.The migration to centralized
TemperatureUnitandUnitSystemtypes improves type safety and maintainability across the codebase. The BatchSizeUnit order adjustment to "l" | "gal" appears to align with a metric-first convention.
154-163: Well-designed minimal interface for metrics calculation.
RecipeMetricsInputprovides a clean separation of concerns by including only the fields required for offline metrics calculations, avoiding unnecessary dependencies on form-specific fields.src/services/brewing/OfflineMetricsCalculator.ts (2)
8-22: LGTM: Union type enables flexible usage.The signature update to accept
RecipeFormData | RecipeMetricsInputis a clean architectural improvement. It allows the calculator to work with minimal data when only metrics are needed, while maintaining backward compatibility with existing code that passes full form data.
157-157: Good refactor: Centralized dry-hop detection.Replacing the inline string check with
isDryHopIngredient(hop)improves maintainability by centralizing the logic and ensuring consistent normalization of hop use types across the codebase.app/(modals)/(beerxml)/importReview.tsx (4)
80-84: LGTM: Standardized logging addresses past feedback.The migration to
UnifiedLoggerproperly addresses the previous review feedback about removing debug console statements. The structured logging with component tags ("import-review") improves observability.
196-220: Robust ingredient validation with detailed logging.The validation filtering properly checks for required fields (
ingredient_id,name,type,unit, validamount) and logs specific errors for debugging. The use ofvoid UnifiedLogger.error(...)correctly handles the async logger without blocking.
235-252: LGTM: Proper offline-first recipe creation with cache management.The mutation correctly uses the offline-first
createRecipehook, invalidates relevant query keys (RECIPES, USER_RECIPES, offline queries, DASHBOARD), and primes the detail cache for immediate access. Therouter.replacenavigation pattern avoids modal stack issues.
618-623: LGTM: Time coercion properly addresses past feedback.The ingredient time display now correctly coerces the value first, then checks if it's defined and greater than zero. This addresses the previous review comment about checking
ingredient.time > 0before coercion, which could misbehave with string values.
…sts and contexts - Updated all instances of temperature units from lowercase (f/c) to uppercase (F/C) in test files for consistency. - Adjusted mock implementations and expected values in unit tests to reflect the casing change. - Ensured that temperature unit handling in conversion functions and contexts aligns with the new standard.
🤖 This commit was automatically generated by GitHub Actions to ensure consistent code formatting across the project.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
src/components/recipes/IngredientEditor/IngredientDetailEditor.tsx (1)
68-72: Type alias swap toUnitSystemis correct; consider future use of_unitSystemSwitching the third parameter of
getContextualIncrements(and its call sites) to the sharedUnitSystemalias removes duplication of the"metric" | "imperial"union and keeps everything consistent withuseUnits. Runtime behavior is unchanged since the parameter remains unused, which the leading underscore makes explicit. If you don’t plan to make increments depend on unit system soon, you could drop the parameter entirely to reduce noise; otherwise this is fine as a forward‑looking hook.Also applies to: 262-270, 775-779, 895-899
src/types/recipe.ts (1)
154-163: Consider whether RecipeMetricsInput should carry unit_system as well
RecipeMetricsInputlooks well‑scoped for metric calculations, and includingbatch_size_unit+ optionalmash_temp_unitis usually enough. If any downstream calculators ever need to distinguish between metric/imperial behavior beyond those field units, consider adding an optionalunit_system: UnitSystemhere now to avoid later signature churn; otherwise this shape is fine as‑is.src/services/offlineV2/LegacyMigrationService.ts (1)
10-11: UnitSystem integration in legacy migration looks consistentUpdating both
migrateLegacyRecipesToV2andconvertLegacyToV2Recipeto takeuserUnitSystem: UnitSystemand derivingbatch_size_unit,mash_temperature, andmash_temp_unitfrom it keeps the migratedRecipeobjects compatible with the new typings. The defaults ("l"/"gal", 67°C/152°F,"C"/"F") are reasonable, but you may want to confirm they match whatever “initial recipe” defaults you use elsewhere so legacy imports behave identically.Also applies to: 26-29, 166-201
tests/app/(modals)/(calculators)/strikeWater.test.tsx (1)
29-41: Strike water modal tests correctly migrated to uppercase temperature unitsUpdating
tempUnitin the calculator state and themockUnitConverter.convertTemperaturelogic to use"F"/"C"keeps these tests in sync with the newTemperatureUnitusage in the screen and service. As written, the mock simply returns the input value for unexpected units; if you ever want tests to fail fast on unit mismatches, you could have the mock throw or log whenfrom/toare not"F"/"C".Also applies to: 124-133, 248-276, 290-402
src/components/recipes/RecipeForm/ParametersForm.tsx (1)
61-76: Mash temperature validation can’t distinguish “empty” from “too low”Right now,
onChangeTextcoerces any falsy input to0:const numValue = parseFloat(text) || 0; handleFieldChange("mash_temperature", numValue);so when the user clears the field, validation sees
0, notNaN, and reports “too low” instead of “required”. Not new to this PR, but it’s more noticeable with stricter unit handling.You could treat an empty string as
undefined/NaNinstead so the"Mash temperature is required"path is used:- onChangeText={text => { - const numValue = parseFloat(text) || 0; - handleFieldChange("mash_temperature", numValue); - }} + onChangeText={text => { + const numValue = + text.trim() === "" ? Number.NaN : parseFloat(text); + handleFieldChange("mash_temperature", numValue); + }}and update
validateField("mash_temperature", value)to useNumber.isNaNonvaluedirectly rather than reparsing a number.Also applies to: 219-229
app/(modals)/(calculators)/strikeWater.tsx (1)
49-57: Strike-water temperature unit migration is consistentUsing
TemperatureUnitplus uppercase"F"/"C"in options, conversions, and helpers is consistent with the shared type and UnitConverter expectations; behavior remains correct.You could tighten
handleTempUnitChange’s signature to acceptTemperatureUnitinstead ofstringto avoid downstream casts, ifUnitTogglecan be generically typed in a follow-up.Also applies to: 216-235, 258-272
app/(modals)/(calculators)/hydrometerCorrection.tsx (1)
46-49: Hydrometer correction screen now consistently uses uppercase temperature unitsThe screen’s options, conversion helper, placeholders, and validation ranges all use
"F"/"C"in a way that matches the updated calculator and tests; behavior remains correct.If you later centralize temperature conversions, consider delegating
convertTemperaturetoUnitConverter.convertTemperatureplustoFixed(1)to avoid duplicating the F↔C formula.Also applies to: 168-179, 208-214, 261-263, 274-276
tests/src/contexts/UnitContext.test.tsx (4)
435-445: Remove unusedconsoleSpyvariable.The
consoleSpyis created on line 435-437 but never used—assertions now checkUnifiedLogger.warninstead. This dead code should be removed.it("should handle unknown unit conversions gracefully", () => { const wrapper = createWrapper("metric"); const { result } = renderHook(() => useUnits(), { wrapper }); - const consoleSpy = jest - .spyOn(console, "warn") - .mockImplementation(() => {}); - const result_conv = result.current.convertUnit(100, "unknown", "kg"); expect(result_conv.value).toBe(100); expect(result_conv.unit).toBe("unknown"); expect(UnifiedLogger.warn).toHaveBeenCalledWith( "units", "No conversion available from unknown to kg" ); });
513-531: Remove unusedconsoleSpyvariable.Same issue as above—
consoleSpyis created but assertions useUnifiedLogger.warn.it("should handle unsupported unit conversions with warning", () => { - const consoleSpy = jest - .spyOn(console, "warn") - .mockImplementation(() => {}); const wrapper = createWrapper("metric"); const { result } = renderHook(() => useUnits(), { wrapper }); // ... });
963-965: Remove unusedconsoleSpyvariable.The spy on
console.warnis unused; assertions targetUnifiedLogger.warn.it("should handle background fetch errors gracefully", async () => { const ApiService = require("@services/api/apiService").default; - const consoleSpy = jest - .spyOn(console, "warn") - .mockImplementation(() => {}); // ... });Also applies to: 987-991
1039-1041: Remove unusedconsoleSpyvariable.The spy on
console.erroris created but never referenced in assertions.it("should handle updateUnitSystem error and revert", async () => { const ApiService = require("@services/api/apiService").default; - const consoleSpy = jest - .spyOn(console, "error") - .mockImplementation(() => {}); // ... });Also applies to: 1056-1060
src/contexts/UnitContext.tsx (1)
326-326: Consider awaiting the logger call before reverting state.
UnifiedLogger.error()returnsPromise<void>. While fire-and-forget is acceptable for non-critical logging, this error log occurs right before reverting state on a failure path. If the log fails silently, debugging production issues becomes harder.} catch (err) { - UnifiedLogger.error("units", "Failed to update unit system:", err); + await UnifiedLogger.error("units", "Failed to update unit system:", err); setError("Failed to save unit preference"); setUnitSystem(previousSystem); // Revert on errorsrc/services/offlineV2/StartupHydrationService.ts (2)
52-52: Incomplete logging migration.Some error paths still use
console.error(line 52) andconsole.warn(lines 100-103) while the rest of the file migrated toUnifiedLogger. Consider completing the migration for consistency.} catch (error) { - console.error(`[StartupHydrationService] Hydration failed:`, error); + await UnifiedLogger.error( + "offline-hydration", + "[StartupHydrationService] Hydration failed:", + error + ); // Don't throw - app should still work even if hydration fails } finally {} catch (error) { - console.warn( - `[StartupHydrationService] User data hydration failed:`, - error - ); + void UnifiedLogger.warn( + "offline-hydration", + "[StartupHydrationService] User data hydration failed:", + error + ); // Continue with static data hydration even if user data fails }Also applies to: 100-103
132-137: Complete the logging migration inhydrateStaticData.Multiple
console.warncalls remain in this method while debug logging usesUnifiedLogger.StaticDataService.updateIngredientsCache().catch(error => { - console.warn( - `[StartupHydrationService] Background ingredients update failed:`, - error - ); + void UnifiedLogger.warn( + "offline-hydration", + "[StartupHydrationService] Background ingredients update failed:", + error + ); });Apply similar changes to lines 153-158 and 166-169.
Also applies to: 153-158, 166-169
src/services/calculators/HydrometerCorrectionCalculator.ts (1)
158-162: DefaultcalibrationTempassumes Fahrenheit; consider documenting or reordering.The default
calibrationTemp = 60is meaningful only whentempUnit = "F". If a caller explicitly passestempUnit = "C"without specifyingcalibrationTemp, they'd get 60°C (converted to 140°F) instead of the intended 15.5°C.Consider either:
- Adding a JSDoc note about this coupling, or
- Using an options object to avoid parameter ordering issues
Example JSDoc clarification:
/** * Get correction table for common temperatures + * @param calibrationTemp - Calibration temperature (default 60°F; provide 15.5 if using Celsius) */ public static getCorrectionTable(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
app/(auth)/resetPassword.tsx(2 hunks)app/(modals)/(brewSessions)/createBrewSession.tsx(2 hunks)app/(modals)/(calculators)/hydrometerCorrection.tsx(5 hunks)app/(modals)/(calculators)/strikeWater.tsx(3 hunks)app/(modals)/(calculators)/unitConverter.tsx(1 hunks)app/(modals)/(recipes)/createRecipe.tsx(4 hunks)app/(modals)/(recipes)/editRecipe.tsx(4 hunks)app/(modals)/(recipes)/ingredientPicker.tsx(2 hunks)src/components/brewSessions/FermentationChart.tsx(2 hunks)src/components/calculators/UnitToggle.tsx(1 hunks)src/components/recipes/BrewingMetrics/BrewingMetricsDisplay.tsx(2 hunks)src/components/recipes/IngredientEditor/IngredientDetailEditor.tsx(2 hunks)src/components/recipes/RecipeForm/ParametersForm.tsx(2 hunks)src/contexts/CalculatorsContext.tsx(6 hunks)src/contexts/UnitContext.tsx(10 hunks)src/hooks/offlineV2/useUserData.ts(6 hunks)src/services/calculators/HydrometerCorrectionCalculator.ts(9 hunks)src/services/calculators/PrimingSugarCalculator.ts(2 hunks)src/services/calculators/StrikeWaterCalculator.ts(8 hunks)src/services/calculators/UnitConverter.ts(4 hunks)src/services/offlineV2/LegacyMigrationService.ts(3 hunks)src/services/offlineV2/StartupHydrationService.ts(9 hunks)src/services/offlineV2/StaticDataService.ts(22 hunks)src/services/offlineV2/UserCacheService.ts(86 hunks)src/types/ai.ts(3 hunks)src/types/api.ts(2 hunks)src/types/brewSession.ts(2 hunks)src/types/recipe.ts(3 hunks)tests/app/(modals)/(calculators)/hydrometerCorrection.test.tsx(32 hunks)tests/app/(modals)/(calculators)/strikeWater.test.tsx(9 hunks)tests/src/components/recipes/RecipeForm/ParametersForm.test.tsx(2 hunks)tests/src/contexts/CalculatorsContext.test.tsx(9 hunks)tests/src/contexts/UnitContext.test.tsx(13 hunks)tests/src/services/calculators/HydrometerCorrectionCalculator.test.ts(22 hunks)tests/src/services/calculators/PrimingSugarCalculator.test.ts(2 hunks)tests/src/services/calculators/StrikeWaterCalculator.test.ts(18 hunks)tests/src/services/calculators/UnitConverter.test.ts(6 hunks)tests/src/types/common.test.ts(1 hunks)tests/testUtils.tsx(2 hunks)tests/utils/unitContextMock.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/src/services/calculators/HydrometerCorrectionCalculator.test.ts
- src/components/calculators/UnitToggle.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- src/hooks/offlineV2/useUserData.ts
- app/(modals)/(recipes)/createRecipe.tsx
- src/types/brewSession.ts
- src/services/offlineV2/UserCacheService.ts
- src/types/ai.ts
🧰 Additional context used
🧬 Code graph analysis (23)
tests/src/services/calculators/UnitConverter.test.ts (1)
src/services/calculators/UnitConverter.ts (1)
UnitConverter(5-224)
src/components/recipes/BrewingMetrics/BrewingMetricsDisplay.tsx (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
src/services/offlineV2/StaticDataService.ts (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
src/types/api.ts (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
tests/src/contexts/UnitContext.test.tsx (2)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/contexts/UnitContext.tsx (1)
useUnits(120-126)
tests/src/components/recipes/RecipeForm/ParametersForm.test.tsx (1)
src/types/common.ts (1)
UnitSystem(27-27)
src/components/brewSessions/FermentationChart.tsx (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
app/(auth)/resetPassword.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/components/recipes/RecipeForm/ParametersForm.tsx (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
app/(modals)/(calculators)/strikeWater.tsx (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
src/services/offlineV2/LegacyMigrationService.ts (1)
src/types/common.ts (1)
UnitSystem(27-27)
tests/src/services/calculators/StrikeWaterCalculator.test.ts (1)
src/services/calculators/StrikeWaterCalculator.ts (1)
StrikeWaterCalculator(21-233)
app/(modals)/(recipes)/editRecipe.tsx (2)
src/types/common.ts (1)
UnitSystem(27-27)src/types/recipe.ts (2)
RecipeMetrics(79-91)Recipe(94-133)
tests/testUtils.tsx (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
tests/src/services/calculators/PrimingSugarCalculator.test.ts (1)
src/services/calculators/PrimingSugarCalculator.ts (1)
PrimingSugarCalculator(16-191)
src/services/calculators/PrimingSugarCalculator.ts (2)
src/types/common.ts (1)
TemperatureUnit(29-29)src/services/calculators/UnitConverter.ts (1)
UnitConverter(5-224)
src/contexts/CalculatorsContext.tsx (1)
src/types/common.ts (2)
TemperatureUnit(29-29)UnitSystem(27-27)
src/types/recipe.ts (1)
src/types/common.ts (2)
UnitSystem(27-27)TemperatureUnit(29-29)
src/services/calculators/HydrometerCorrectionCalculator.ts (2)
src/types/common.ts (1)
TemperatureUnit(29-29)src/services/calculators/UnitConverter.ts (1)
UnitConverter(5-224)
app/(modals)/(brewSessions)/createBrewSession.tsx (1)
src/types/common.ts (1)
TemperatureUnit(29-29)
app/(modals)/(recipes)/ingredientPicker.tsx (1)
src/types/common.ts (1)
UnitSystem(27-27)
src/components/recipes/IngredientEditor/IngredientDetailEditor.tsx (1)
src/types/common.ts (1)
UnitSystem(27-27)
src/contexts/UnitContext.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
🔇 Additional comments (50)
src/types/api.ts (1)
28-28: LGTM! Clean refactoring to centralized type.The change from an inline string literal union to the centralized
TemperatureUnittype improves maintainability and consistency. The types are equivalent ("C" | "F"), ensuring no breaking changes to the API contract.Also applies to: 189-189
app/(auth)/resetPassword.tsx (2)
50-50: LGTM: Import addition aligns with logging standardization.The UnifiedLogger import is correctly added to support the project-wide migration from console-based logging to UnifiedLogger.
192-196: LGTM: Logging migration correctly implemented.The replacement of console-based logging with
UnifiedLogger.erroris correct:
- Parameters match the expected signature (category, message, data)
- Proper use of
awaitsince the method returns a Promise- Error handling flow remains unchanged
- Category naming follows the component.function convention
app/(modals)/(brewSessions)/createBrewSession.tsx (2)
106-107: LGTM! Good type centralization.Migrating from the inline literal type to the centralized
TemperatureUnittype improves consistency and maintainability across the codebase.
62-62: No action required. The import path@/src/typesis correct. Thesrc/types/index.tsfile re-exportsTemperatureUnitfromcommon.ts, making it properly accessible via the@/src/typespath. This follows the standard import pattern established throughout the codebase.src/components/recipes/IngredientEditor/IngredientDetailEditor.tsx (1)
39-44: Centralizing onUnitSystemimport looks goodImporting
UnitSystemfrom@src/typesalongsideRecipeIngredient/IngredientType/IngredientUnitkeeps the component aligned with the shared type definitions and avoids local string unions. No issues here.app/(modals)/(recipes)/ingredientPicker.tsx (1)
51-52: Type standardization aligns with centralized definitions.The import and usage of
UnitSystemfrom@src/typesreplaces the inline literal union type, improving consistency across the codebase.Also applies to: 158-158
tests/testUtils.tsx (1)
10-10: Type alignment with centralizedTemperatureUnitdefinition.The
temperatureUnitfield now uses the sharedTemperatureUnittype, ensuring consistency with the type defined insrc/types/common.ts.Also applies to: 24-24
src/contexts/CalculatorsContext.tsx (2)
314-315: Verify intentional change in default preferences.The default
defaultUnitschanged to"metric"andtemperatureUnitto"C". This changes the initial experience for new users who haven't set preferences. Please confirm this is the intended behavior for the application's target audience.
52-52: Type centralization improves maintainability.The migration to
TemperatureUnitandUnitSystemtypes from../typesensures consistent typing across the codebase and provides a single source of truth for these type definitions.Also applies to: 79-79, 91-91, 173-174
tests/src/contexts/CalculatorsContext.test.tsx (3)
59-59: Tests correctly updated for uppercase temperature units.The test assertions now match the updated default values in the implementation (
"F"instead of"f").Also applies to: 65-65
143-143: Test payloads updated to use consistent uppercase format.Dispatch payloads correctly use uppercase
"C"and"F"to match the newTemperatureUnittype definition.Also applies to: 152-152, 171-171
596-597: Verify default expectations for corrupted state test.This test now expects
"metric"and"C"as fallback defaults when persisted state is corrupted. Ensure this aligns with the intended default behavior for new/reset users.app/(modals)/(recipes)/editRecipe.tsx (3)
19-25: Type standardization withUnitSystemandRecipeMetrics.The imports and function signature now use the centralized
UnitSystemtype, maintaining consistency with other parts of the codebase.Also applies to: 150-150
389-405: Well-designed fallback pattern for metrics preservation.The
getMetricOrFallbackhelper correctly prioritizes newly calculated metrics when finite, falling back to existing recipe values otherwise. This prevents accidental metric loss during updates when calculations are unavailable.
443-449: Improved metrics handling during recipe updates.The explicit field assignments with
getMetricOrFallbackare cleaner than conditional spreading and ensure consistent behavior. The comment clearly documents the intent.src/types/recipe.ts (1)
20-21: Consistent use of shared UnitSystem/TemperatureUnit typesSwitching
unit_systemandmash_temp_unitto use the sharedUnitSystem/TemperatureUnittypes in bothRecipeandRecipeFormDatakeeps the units consistent across the codebase and should make future refactors safer. No issues spotted with these type changes.Also applies to: 102-107, 143-147
src/components/recipes/BrewingMetrics/BrewingMetricsDisplay.tsx (1)
15-27: BrewingMetricsDisplay now correctly typed with TemperatureUnitTyping
mash_temp_unitasTemperatureUnitand importing it from the shared types aligns this component with the rest of the unit system work. The"°${mash_temp_unit || "F"}"fallback keeps behavior stable for missing data. No further changes needed here.Also applies to: 199-205
tests/utils/unitContextMock.ts (1)
26-37: UnitContext mock now aligned with uppercase temperature unitsReturning
"F"forgetPreferredUnit("temperature")and in the non‑weight/volume branch ofconvertForDisplaymatches the newTemperatureUnit = "C" | "F"convention and keeps tests consistent with production behavior. Looks good.Also applies to: 50-63
tests/src/types/common.test.ts (1)
143-160: Pagination test data change is harmlessChanging the sample
dataarray to["a", "b", "C"]doesn’t affect the assertions (they only inspect pagination fields), so this is effectively a no‑op and safe.tests/src/services/calculators/StrikeWaterCalculator.test.ts (1)
35-43: StrikeWaterCalculator tests now correctly exercise TemperatureUnit behaviorThe updated
convertTemperaturemock and the use of"F"/"C"in allcalculateStrikeWater,calculateInfusion, andvalidateInputscalls align these tests with theTemperatureUnittype used in the implementation. The assertions around Fahrenheit/Celsius ranges and error messages still make sense with the new units. No issues found.Also applies to: 50-57, 66-95, 102-118, 180-202, 209-237, 242-255, 300-356, 391-428
src/components/recipes/RecipeForm/ParametersForm.tsx (1)
7-8: Good move to type mash-temp unit as TemperatureUnitImporting
TemperatureUnitand updatinghandleMashTempUnitChangeto accept a typed unit parameter keeps this form aligned with the shared unit definitions, and the conversion logic for F↔C remains correct and side‑effect free. The buttons still pass"F"/"C"explicitly, so everything type‑checks cleanly.Also applies to: 101-115, 235-269
tests/src/services/calculators/PrimingSugarCalculator.test.ts (1)
30-35: Temperature unit updates in tests look correctUppercase
"C"/"F"in the mock andestimateResidualCO2tests align with theTemperatureUnittype and the implementation; no issues spotted.Also applies to: 149-177
app/(modals)/(calculators)/unitConverter.tsx (1)
78-82: Temperature unit option values align with UnitConverterChanging Fahrenheit/Celsius values to
"F"/"C"matchesUnitConverter.getTemperatureUnitsandconvertTemperatureexpectations; Kelvin"k"remains compatible.tests/src/services/calculators/UnitConverter.test.ts (1)
63-75: UnitConverter temperature tests correctly reflect uppercase units and messagesTests now exercise
"F"/"C"/"k"consistently and assert the updated error messages and formatting; this matches the current UnitConverter contract.Also applies to: 88-92, 131-134, 149-151, 175-183, 187-193, 197-198, 261-268
tests/src/components/recipes/RecipeForm/ParametersForm.test.tsx (1)
4-5: UnitSystem typing in tests is now consistent with shared typesUsing the shared
UnitSystemtype formockUnitContext.unitSystemkeeps these tests aligned with the global type definition without changing behavior.Also applies to: 44-47
src/components/brewSessions/FermentationChart.tsx (1)
32-33: FermentationChart props now correctly use the shared TemperatureUnit typeSwitching
temperatureUnitto the sharedTemperatureUnitalias centralizes the allowed values and keeps existing logic (C/F comparisons and fallbacks) intact.Also applies to: 247-250
tests/app/(modals)/(calculators)/hydrometerCorrection.test.tsx (1)
48-50: Hydrometer correction tests correctly assert uppercase temperature unit behaviorThe test suite now drives and verifies
"F"/"C"units throughout state, toggling, conversions, calculator calls, and history payloads, matching the updated screen and service contracts.Also applies to: 134-135, 178-183, 265-283, 288-320, 340-389, 392-399, 414-420, 435-441, 467-472, 435-452, 478-487, 497-503, 517-523, 549-571, 593-594, 602-608, 606-617, 621-627, 640-646, 660-666, 671-674, 688-696, 699-706, 724-729, 743-747
src/services/calculators/UnitConverter.ts (1)
192-194: LGTM!The updated return values correctly reflect the new uppercase convention for Fahrenheit (
"F") and Celsius ("C"), while maintaining lowercase"k"for Kelvin which aligns with the switch case handling.tests/src/contexts/UnitContext.test.tsx (2)
26-34: LGTM!The UnifiedLogger mock is properly configured with all the required methods (error, warn, info, debug), enabling assertions on logging calls throughout the test suite.
1092-1108: LGTM!Temperature conversion tests correctly updated to use uppercase unit tokens (
"F","C"), aligning with the newTemperatureUnittype.src/contexts/UnitContext.tsx (4)
162-166: LGTM!The migration to
UnifiedLogger.warnwith the"units"category provides consistent structured logging for cache corruption scenarios.
349-351: LGTM!The
getPreferredUnitmethod correctly returns uppercase temperature units ("C"/"F") aligned with theTemperatureUnittype.
419-431: LGTM!Temperature conversion branches and the fallback logging correctly use uppercase unit tokens and structured logging via
UnifiedLogger.warn.
629-633: LGTM!The
getCommonUnitsmethod returns uppercase temperature unit values ("C"/"F") with proper labels, maintaining consistency with the rest of the unit system changes.src/services/offlineV2/StaticDataService.ts (4)
76-80: LGTM!The error logging is correctly awaited before throwing the
OfflineError, ensuring the log completes before the exception propagates.
337-340: Fire-and-forget logging is appropriate here.Using
void UnifiedLogger.warn(...)for the authentication warning is acceptable since this is a non-critical informational log during a graceful fallback path (returning empty array for unauthenticated users).
476-480: LGTM!Awaiting the error log before throwing ensures the structured error payload (with type and data) is captured before the exception propagates.
619-634: LGTM!Fire-and-forget pattern (
void UnifiedLogger.warn) is appropriate for background version check failures, which should not block the main flow.src/services/offlineV2/StartupHydrationService.ts (2)
20-23: LGTM!The method signature correctly uses the
UnitSystemtype with"imperial"as the default, aligning with the broader type system changes.
26-29: LGTM!The
UnifiedLogger.debugcalls provide consistent structured logging for hydration progress tracking with the"offline-hydration"category.Also applies to: 34-37, 47-50
src/services/calculators/StrikeWaterCalculator.ts (3)
6-6: LGTM!The
TemperatureUnitimport and updated method signature with"F"default align with the centralized type system changes.Also applies to: 36-36
50-57: LGTM!Temperature conversion logic correctly uses uppercase
"C"and"F"tokens for both input detection andUnitConvertercalls.Also applies to: 78-79
177-182: LGTM!The
validateInputsmethod correctly acceptsTemperatureUnitand validates temperature ranges based on uppercase"F"comparison.Also applies to: 193-211
src/services/calculators/PrimingSugarCalculator.ts (1)
96-103: No action required—all callers already use uppercase temperature units.The type definition
TemperatureUnit = "C" | "F"(insrc/types/common.ts) enforces uppercase only. All existing calls toestimateResidualCO2in the test file use uppercase"F"and"C", so the signature change poses no breaking change risk in the current codebase.src/services/calculators/HydrometerCorrectionCalculator.ts (5)
7-8: Good centralization of theTemperatureUnittype.Importing from
@/src/types/commonensures consistency with other calculators and services using the same type definition.
28-48: LGTM!The
TemperatureUnittype integration is correct. The conditional at line 41 properly checks for"C"and converts to Fahrenheit for the internal calculations.
205-236: LGTM!Validation logic correctly handles both temperature units with appropriate ranges and clear error messages.
84-100: LGTM!The
TemperatureUnittype integration acrosscalculateCorrectionDefault,findCorrectTemperature, andisCorrectionSignificantis consistent and correct. The default values and conditional checks properly use the uppercase"F"and"C"literals matching the type definition.Also applies to: 107-153, 187-200
241-251: No action required—all callers already use uppercaseFandCproperties.The return type uses uppercase keys
FandCthroughout the codebase, and all tests confirm this is the expected interface. No callers are accessing lowercaseforcproperties.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/(modals)/(beerxml)/importReview.tsx (1)
615-620: Consider simplifying the time display logic.The current IIFE pattern correctly addresses the coercion-before-check requirement but is verbose for a simple conditional render.
Apply this diff for a more concise implementation:
- {(() => { - const time = coerceIngredientTime(ingredient.time); - return time !== undefined && time > 0 - ? ` • ${time} min` - : ""; - })()} + {((time) => + time !== undefined && time > 0 ? ` • ${time} min` : "" + )(coerceIngredientTime(ingredient.time))}Or extract to a helper at the module level if used elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/services/calculators/UnitConverter.ts (1)
221-228: LGTM!Using the normalization helper for validation ensures consistent behavior with
convertTemperature. This resolves the previous review concern about validation accepting lowercase while conversion rejected it.
🧹 Nitpick comments (10)
src/services/calculators/UnitConverter.ts (1)
87-111: Consider removing unreachable default cases.After normalization,
fromandtocan only be"F","C", or"K". Thedefaultbranches at lines 97-98 and 109-110 are now unreachable dead code sincenormalizeTemperatureUnitthrows on unknown units before reaching these switches.You could remove the default cases or mark them with a TypeScript
neverassertion for exhaustiveness checking:case "K": celsius = value - 273.15; break; - default: - throw new Error(`Unknown temperature unit: ${fromUnit}`); } // Convert from Celsius to target unit switch (to) { case "F": return celsius * (9 / 5) + 32; case "C": return celsius; case "K": return celsius + 273.15; - default: - throw new Error(`Unknown temperature unit: ${toUnit}`); }src/contexts/UnitContext.tsx (2)
162-166: Inconsistentawaitusage with asyncUnifiedLoggermethods.
UnifiedLogger.warnreturns aPromise<void>, but these calls are not awaited while line 326 usesawait UnifiedLogger.error(...). Fire-and-forget logging is acceptable, but the inconsistency may cause confusion. Consider either:
- Consistently not awaiting all log calls (add
voidprefix for clarity)- Awaiting all calls if log delivery is critical
- UnifiedLogger.warn( + void UnifiedLogger.warn( "units", "Corrupted cached user settings, removing:", parseErr );
325-328: Consider consistent logging pattern.This is the only
UnifiedLoggercall withawait. While ensuring error delivery before state updates is reasonable, it delayssetErrorif logging is slow. For consistency with other fire-and-forget calls in this file, consider:} catch (err) { - await UnifiedLogger.error("units", "Failed to update unit system:", err); + void UnifiedLogger.error("units", "Failed to update unit system:", err); setError("Failed to save unit preference");app/(modals)/(beerxml)/importReview.tsx (4)
131-138: Empty string amount silently becomes 0.
Number("")evaluates to0, soisNaN(Number(""))isfalse. This means an ingredient withamount: ""passes validation and becomes0on line 146. If this is intentional (treat missing amounts as zero), consider adding a comment. If not, add explicit empty-string handling:- if (isNaN(Number(ing.amount))) { + if (ing.amount === "" || ing.amount == null || isNaN(Number(ing.amount))) {
170-174: Missingvoidprefix for consistency.Other UnifiedLogger calls in this file (lines 74, 116, 124, 132, 241, 255, 366) use
void UnifiedLogger.*to explicitly ignore the promise. This call is inconsistent:- UnifiedLogger.error( + void UnifiedLogger.error( "import-review", "Failed to parse recipe data:", error );
317-321: Unsafe double cast bypasses type checking.The
as unknown as Partial<Recipe>cast silences TypeScript but could hide real type mismatches betweenIngredientInput[]andRecipeIngredient[]. Consider defining a properRecipeCreatePayloadtype that acceptsIngredientInput[], or use a type guard/mapper:// In types/recipe.ts export interface RecipeCreatePayload extends Omit<Partial<Recipe>, 'ingredients'> { ingredients: IngredientInput[]; }Then update
createRecipesignature to acceptRecipeCreatePayload.
626-665: Truthiness check may hide valid zero values.Conditions like
{calculatedMetrics.og ? ...}will hide display when value is0. While OG/FG/ABV of 0 are likely invalid, IBU and SRM of 0 are valid (e.g., a recipe with no hops or no colored grains). Consider explicit null checks:- {calculatedMetrics.ibu ? ( + {calculatedMetrics.ibu != null ? (Or accept that zero-IBU/SRM scenarios are rare enough that current behavior is acceptable.
src/services/offlineV2/StartupHydrationService.ts (3)
24-27: Promise.allSettled + swallowed errors means “success” logging even when hydration failsRight now
hydrateOnStartupusesPromise.allSettledand bothhydrateUserData/hydrateStaticDatacatch and swallow their own errors. That combination means:
- The
try/catchhere almost never sees a rejection from those two methods.hasHydratedis set totrueand[StartupHydrationService] Hydration completed successfullyis logged even if one or both sub‑hydrations actually failed.Functionality is still “best effort” and non‑blocking, but this can make debugging real hydration failures harder and prevents a clean way to know if anything actually succeeded.
A cleaner pattern would be either:
- Use
Promise.alland let the sub‑methods throw (logging at the top level), or- Keep
Promise.allSettledbut inspect the results and log a “completed with errors” message (and potentially avoid settinghasHydrated).For example (if you choose to keep errors bubbling up):
- try { - // Hydrate in parallel for better performance - await Promise.allSettled([ - this.hydrateUserData(userId, userUnitSystem), - this.hydrateStaticData(), - ]); - - this.hasHydrated = true; - UnifiedLogger.debug( - "offline-hydration", - `[StartupHydrationService] Hydration completed successfully` - ); - } catch (error) { + try { + // Hydrate in parallel for better performance + await Promise.all([ + this.hydrateUserData(userId, userUnitSystem), + this.hydrateStaticData(), + ]); + + this.hasHydrated = true; + UnifiedLogger.debug( + "offline-hydration", + `[StartupHydrationService] Hydration completed successfully` + ); + } catch (error) { await UnifiedLogger.error( "offline-hydration", "[StartupHydrationService] Hydration failed:", error ); // Don't throw - app should still work even if hydration fails } finally {(You’d then let the sub‑methods throw instead of swallowing errors; see related comments below.)
Also applies to: 34-37, 46-50, 52-56
71-75: hydrateUserData logging is solid; consider letting errors bubble to the top levelThe additional debug logging around user hydration is helpful and low‑noise.
Given the top‑level
hydrateOnStartupis already structured to log failures, you might consider letting this method throw instead of swallowing errors:- private static async hydrateUserData( - userId: string, - userUnitSystem: UnitSystem = "imperial" - ): Promise<void> { - try { + private static async hydrateUserData( + userId: string, + userUnitSystem: UnitSystem = "imperial" + ): Promise<void> { UnifiedLogger.debug( "offline-hydration", `[StartupHydrationService] Hydrating user data...` ); … - UnifiedLogger.debug( + UnifiedLogger.debug( "offline-hydration", `[StartupHydrationService] User data hydration completed` ); - } catch (error) { - void UnifiedLogger.warn( - "offline-hydration", - `[StartupHydrationService] User data hydration failed:`, - error - ); - // Continue with static data hydration even if user data fails - } + // Let errors bubble; top-level hydrateOnStartup will log and decide policy. }That keeps a single place (
hydrateOnStartup) responsible for deciding whether to mark the whole startup hydration as “successful” vs “failed/partial” while still keeping your per‑step debug logs.If you prefer to keep the current “always swallow and warn” behavior, you may want to adjust the top‑level “completed successfully” message to something like “startup hydration attempt completed (see warnings for per‑step failures)” to avoid confusion.
Also applies to: 83-94, 99-102, 104-108
118-121: Static data hydration flow is well‑structured; same note on error swallowing vs top‑level statusThe static hydration logic is clear:
- Debug message at start and end.
- Branching on cached vs non‑cached for ingredients and beer styles.
- Background cache updates with
.catchthat logs viaUnifiedLogger.warn, which is appropriate.The same structural concern as for
hydrateUserDataapplies here: by catching and swallowing all errors,hydrateOnStartupcan’t distinguish “all good” from “static hydration actually failed”, yet it logs a global success and setshasHydrated.If you want top‑level status to track real success/failure, you could:
- Let this method throw and rely on
hydrateOnStartup’stry/catch, or- Keep catching but surface a boolean/enum status to the caller instead of just
void.For example, the minimal “bubble up” change:
- private static async hydrateStaticData(): Promise<void> { - try { + private static async hydrateStaticData(): Promise<void> { UnifiedLogger.debug( "offline-hydration", `[StartupHydrationService] Hydrating static data...` ); … - UnifiedLogger.debug( + UnifiedLogger.debug( "offline-hydration", `[StartupHydrationService] Static data hydration completed` ); - } catch (error) { - void UnifiedLogger.warn( - "offline-hydration", - `[StartupHydrationService] Static data hydration failed:`, - error - ); - } }and then rely on
Promise.all+ the top‑levelcatchto log any failures once.If you prefer the fully best‑effort behavior, consider renaming the top‑level log to something that acknowledges potential partial failures, e.g. “Startup hydration attempt finished; see warnings for details.”
Also applies to: 126-135, 138-142, 148-151, 154-157, 160-165, 168-171, 173-177
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(7 hunks)package.json(1 hunks)src/components/recipes/RecipeForm/ParametersForm.tsx(3 hunks)src/contexts/UnitContext.tsx(10 hunks)src/services/brewing/OfflineMetricsCalculator.ts(10 hunks)src/services/calculators/HydrometerCorrectionCalculator.ts(9 hunks)src/services/calculators/UnitConverter.ts(6 hunks)src/services/offlineV2/StartupHydrationService.ts(8 hunks)src/types/recipe.ts(3 hunks)tests/src/contexts/UnitContext.test.tsx(14 hunks)tests/src/services/calculators/UnitConverter.test.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- android/app/src/main/res/values/strings.xml
- src/services/brewing/OfflineMetricsCalculator.ts
- android/app/build.gradle
- tests/src/contexts/UnitContext.test.tsx
- src/components/recipes/RecipeForm/ParametersForm.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
src/services/offlineV2/StartupHydrationService.ts (2)
src/types/common.ts (1)
UnitSystem(27-27)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/types/recipe.ts (1)
src/types/common.ts (2)
UnitSystem(27-27)TemperatureUnit(29-29)
src/contexts/UnitContext.tsx (2)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)scripts/build-helper.js (1)
err(42-42)
src/services/calculators/HydrometerCorrectionCalculator.ts (2)
src/types/common.ts (1)
TemperatureUnit(29-29)src/services/calculators/UnitConverter.ts (1)
UnitConverter(5-245)
tests/src/services/calculators/UnitConverter.test.ts (1)
src/services/calculators/UnitConverter.ts (1)
UnitConverter(5-245)
app/(modals)/(beerxml)/importReview.tsx (6)
src/types/common.ts (2)
UnitSystem(27-27)TemperatureUnit(29-29)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/recipe.ts (3)
IngredientInput(247-256)RecipeMetricsInput(165-173)Recipe(94-133)src/utils/keyUtils.ts (1)
generateUniqueId(95-114)src/hooks/offlineV2/useUserData.ts (1)
useRecipes(18-284)src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(19-383)
🔇 Additional comments (34)
package.json (1)
4-4: Version bump is consistent across files.The version update to 3.3.7 aligns with app.json and related config files per the PR summary. Since this is a straightforward version update with no dependency or script changes, this looks good.
app.json (2)
6-6: Version strings are consistent across expo config.The version and runtimeVersion fields are both updated to 3.3.7, matching package.json.
Also applies to: 61-61
19-19: Verify the versionCode increment strategy.The versionCode jumps from 191 to 199 (+8), which is larger than a typical single-release increment. If this is a single production release, Android convention is to increment by 1. Confirm whether this is intentional (e.g., reserved codes, intermediate pre-release builds, or alignment with a versioning strategy).
src/types/recipe.ts (4)
20-20: LGTM!Centralizing
TemperatureUnitandUnitSystemimports fromcommon.tseliminates type duplication and ensures consistency across the codebase.
102-106: LGTM!Using the centralized
UnitSystemandTemperatureUnittypes ensures type consistency with other modules using these definitions.
143-147: LGTM!
RecipeFormDatanow aligns with theRecipeinterface by using the same centralized types.
154-173: Well-documented interface with clear design rationale.The JSDoc explaining why
unit_systemis intentionally excluded helps future maintainers understand the design decision. The union type(RecipeIngredient | IngredientInput)[]for ingredients provides flexibility for both existing recipes and imports.tests/src/services/calculators/UnitConverter.test.ts (5)
61-92: LGTM!Tests properly cover case-insensitivity (e.g.,
"celsius","fahrenheit", lowercase"k") and verify the expected uppercase canonical outputs. The mix of cases ensures the normalization logic is thoroughly tested.
130-134: LGTM!Validation tests correctly verify that both canonical (
"F") and alias ("celsius") forms are accepted while invalid units are rejected.
148-151: LGTM!Formatting tests verify that canonical units produce correctly formatted output with degree symbols.
172-198: LGTM!Error handling tests cover both unknown unit errors and Kelvin conversion edge cases. Using lowercase
"k"inputs confirms normalization works correctly for Kelvin.
259-265: LGTM!Tests verify that
getTemperatureUnits()returns the canonical uppercase forms["F", "C", "K"].src/services/calculators/UnitConverter.ts (4)
50-69: Clean normalization helper that centralizes temperature unit handling.This addresses the previous review concern about case-sensitivity inconsistency. Both validation and conversion now use this same normalization logic, ensuring consistent behavior.
77-79: LGTM!Normalizing units at the entry point ensures all downstream logic uses canonical forms consistently.
208-210: LGTM!Returning uppercase canonical units aligns with the normalization behavior and the
TemperatureUnittype definition.
241-244: LGTM!Normalizing before formatting ensures the displayed unit is always in canonical form regardless of input casing.
src/services/calculators/HydrometerCorrectionCalculator.ts (7)
7-7: LGTM!Using the centralized
TemperatureUnittype fromcommon.tsensures type consistency across calculators.
28-48: LGTM!The type change to
TemperatureUnitand updated comparisons to uppercase"C"are consistent with the codebase-wide standardization.
84-100: LGTM!The default parameter
tempUnit: TemperatureUnit = "F"and corresponding logic correctly selects the appropriate calibration temperature based on the unit.
107-121: LGTM!Temperature bounds are correctly set based on the unit system (32-212°F or 0-100°C).
155-173: Good JSDoc update clarifying the default calibration temperature limitation.The documentation correctly warns that the default
calibrationTemp = 60is meaningful only fortempUnit="F". This helps prevent subtle bugs when users switch to Celsius without adjusting the calibration value.Consider whether a runtime check or assertion would be valuable when
tempUnit="C"andcalibrationTemp=60, since 60°C is an unusual calibration temperature (most are ~15.5°C). This could catch accidental misuse.
212-242: LGTM!
validateInputscorrectly validates temperature ranges based on the unit type.
248-257: LGTM!The return type and keys updated from lowercase
{ f, c }to uppercase{ F, C }aligns with theTemperatureUnittype definition.src/contexts/UnitContext.tsx (3)
40-40: LGTM!The import follows the project's path alias conventions and correctly brings in the UnifiedLogger for structured logging.
427-431: Logging approach looks good.The structured logging with category
"units"and descriptive messages improves observability. The fallback behavior (returning original value when no conversion exists) is appropriate.
350-350: This change is not breaking; uppercase temperature units are the established convention.The codebase already expects and uses uppercase
"C"and"F"throughout. Test assertions (lines 233, 242 in UnitContext.test.tsx) verifygetPreferredUnit("temperature")returns uppercase, and all production usage viaUnitConverter.convertTemperature()(found in calculators, strike water, priming sugar, hydrometer correction) passes uppercase units. TheUnitConverter.normalizeTemperatureUnit()function normalizes lowercase inputs to uppercase if needed, making this change safe and consistent with the rest of the system.app/(modals)/(beerxml)/importReview.tsx (5)
29-42: LGTM! Clean imports aligned with offline-first architecture.The imports properly bring in the necessary types (
TemperatureUnit,UnitSystem,RecipeMetricsInput) and the offline-first dependencies (useRecipes,OfflineMetricsCalculator,UnifiedLogger).
48-57: Good centralization of unit system derivation.The explicit
unit_systemcheck before falling back to batch size inference addresses the previous fragility concern by allowing callers to provide an explicit value when available. The fallback to "imperial" for unknown units is a reasonable default.
63-81: Operator precedence bug fixed correctly.The rewrite properly separates validation from fallback logic. The previous implementation
(recipeData.mash_temp_unit ?? recipeData.unit_system === "metric")had incorrect precedence; this version validates the input first, then applies unit-system-appropriate defaults.
187-204: Query key fingerprinting is reasonable for this use case.Including
JSON.stringifyof ingredient details in the query key ensures cache invalidation when ingredients change. For import flows with typical ingredient counts, this is acceptable. For very large recipe imports, consider a hash function instead.
702-706: Time coercion issue fixed with IIFE pattern.The previous fragile pattern
ingredient.time > 0before coercion is now properly addressed by coercing first viacoerceIngredientTime, then checking the result. The IIFE cleanly encapsulates this logic.src/services/offlineV2/StartupHydrationService.ts (3)
187-190: resetHydrationState logging is appropriate and consistentResetting the internal flags and emitting a debug log through
UnifiedLoggerkeeps behavior transparent for tests and logout flows and matches the logging strategy elsewhere in this service. No issues here.
34-37: Be cautious logging raw user identifiers through UnifiedLoggerThe debug log sends
userIdthroughUnifiedLogger, which can forward logs to a dev server or other sinks. IfuserIdis considered PII, this expands its exposure surface versus a pure local console log.Consider either:
- Omitting or hashing/truncating the identifier in logs, or
- Ensuring your logging policy explicitly allows user identifiers to be sent to all logger backends.
10-11: UnitSystem typing and default are correct; call sites properly updatedThe
UnitSystemtype definition ("metric" | "imperial") is sound, and the default of"imperial"is valid. The primary production call site inuseStartupHydration.tscorrectly passes theunitSystemparameter from theUnitContext, which properly manages the type. All test cases verify both default and explicit invocations, and no breaking changes were detected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests/src/services/calculators/UnitConverter.test.ts (1)
127-131: Temperature unit validation test matches the new C/F-only normalizationValidating
"F"and"celsius"(plus a clearly invalid string) accurately reflectsisValidTemperatureUnit’s reliance onnormalizeTemperatureUnit. If you want to harden coverage further, you could optionally add cases for"f"/"c"literals to lock in lowercase alias support as well.src/hooks/offlineV2/useUserData.ts (2)
68-76: Async logger calls in catch blocks may obscure primary errorsIn several catch paths you now
await UnifiedLogger.error(...)before setting state (e.g.,useRecipes.loadData,useRecipes.refresh,useBrewSessions.loadData,useBrewSessions.refresh). If the logger itself throws (network/AsyncStorage issues), the hook will surface that logging failure and may skip updating the intendederrorstate.If you want logging to be best‑effort only, consider either wrapping these awaits in a small
try/catchor switching tovoid UnifiedLogger.error(...)so logging can’t interfere with the main control flow.Also applies to: 215-237, 349-357, 500-505, 526-535
85-95: Drop theas Partial<Recipe>cast when delegating toUserCacheService.createRecipe
useRecipes.createacceptsPartial<Recipe> | RecipeCreatePayload, andUserCacheService.createRecipenow has the same union signature. The cast here:const newRecipe = await UserCacheService.createRecipe({ ...recipe, user_id: userId, } as Partial<Recipe>);is therefore unnecessary and hides the fact that
ingredientsmay beIngredientInput[]when usingRecipeCreatePayload.You can pass the union directly:
const newRecipe = await UserCacheService.createRecipe({ ...recipe, user_id: userId, });This keeps the create‑payload shape visible to the type system and aligns with how
createRecipeis declared.app/(modals)/(beerxml)/importReview.tsx (3)
49-58: Unit/ingredient helpers and metrics payload wiring look good (with a small enhancement)The
deriveUnitSystem/deriveMashTempUnithelpers plusnormalizeImportedIngredientsandRecipeMetricsInputassembly give a clear, single source of truth for both metrics and creation; the validation + logging around bad ingredients is also helpful.One small enhancement: when mapping
instance_id, consider preserving an existing value before generating a new one so IDs stay stable across steps:instance_id: ing.instance_id ?? generateUniqueId("ing"),That keeps any upstream instance identifiers (e.g., from BeerXML matching) intact while still backfilling when missing.
Also applies to: 64-82, 106-158, 220-239
29-44: StandardizeUnifiedLoggerimport aliasHere you import:
import { UnifiedLogger } from "@/src/services/logger/UnifiedLogger";while other modules (e.g.,
src/hooks/offlineV2/useUserData.ts,src/services/offlineV2/UserCacheService.ts) use@services/logger/UnifiedLogger. To avoid accidental duplicate bundles or path‑alias drift, consider standardizing on a single alias for this module across the app.
279-326: SimplifycreateRecipecall and keepRecipeCreatePayloadvisibleInside the mutation,
recipePayloadis aRecipeCreatePayload, anduseRecipes().createalready acceptsPartial<Recipe> | RecipeCreatePayload. The double cast:return await createRecipe(recipePayload as unknown as Partial<Recipe>);is therefore unnecessary and hides the fact that
ingredientsareIngredientInput[].You can simplify and keep the type information intact:
return await createRecipe(recipePayload);This keeps the BeerXML import payload explicit at the UI layer and leaves
UserCacheService.createRecipeas the single place that understands how to turnIngredientInput[]into whatever persistedRecipeshape you need.src/services/offlineV2/UserCacheService.ts (2)
15-31: AlignUnifiedLoggerimport style with its actual exportsThis file still uses a default import:
import UnifiedLogger from "@services/logger/UnifiedLogger";while other updated modules import it as a named export (
{ UnifiedLogger }). IfUnifiedLogger.tshas been converted to named‑only exports, this import will break; if it still exports both default and named, the behaviour is fine but stylistically inconsistent.Worth double‑checking the logger’s export shape and then standardizing on either default or named imports across the codebase to avoid confusion.
195-319: Standardize async logger usage to avoid floating promises and inconsistent failuresThroughout this service there’s a mix of:
await UnifiedLogger.*(...)inside try/catch blocks, and- bare calls like
UnifiedLogger.debug(...)/UnifiedLogger.warn(...)withoutawaitorvoid.Given
UnifiedLoggermethods areasync, the bare calls create floating promises that can yield unhandled rejections if logging fails, while the awaited calls can cause logger failures to affect primary control flow (e.g.,getRecipes,getBrewSessions, sync helpers).It would be cleaner to pick a consistent strategy:
- For logs that must not affect behaviour, explicitly fire‑and‑forget:
void UnifiedLogger.debug(...).- For logs you do want to await, keep
awaitbut consider smalltry/catchblocks so a logging failure doesn’t mask the real offline/cache error.That will make error propagation and linting around logging much clearer.
Also applies to: 878-1013, 3084-3138, 3875-3897, 4343-4346
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(8 hunks)package.json(1 hunks)src/contexts/UnitContext.tsx(10 hunks)src/hooks/offlineV2/useUserData.ts(8 hunks)src/services/calculators/UnitConverter.ts(4 hunks)src/services/offlineV2/UserCacheService.ts(87 hunks)src/types/recipe.ts(3 hunks)tests/src/services/calculators/UnitConverter.test.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- android/app/src/main/res/values/strings.xml
- android/app/build.gradle
- src/contexts/UnitContext.tsx
- src/services/calculators/UnitConverter.ts
🧰 Additional context used
🧬 Code graph analysis (4)
tests/src/services/calculators/UnitConverter.test.ts (1)
src/services/calculators/UnitConverter.ts (1)
UnitConverter(5-226)
src/types/recipe.ts (1)
src/types/common.ts (2)
UnitSystem(27-27)TemperatureUnit(29-29)
src/hooks/offlineV2/useUserData.ts (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/types/recipe.ts (2)
Recipe(94-133)RecipeCreatePayload(181-184)src/services/logger/DevLogger.ts (1)
error(141-148)
src/services/offlineV2/UserCacheService.ts (6)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)src/types/common.ts (1)
UnitSystem(27-27)src/types/offlineV2.ts (1)
OfflineError(267-276)src/types/recipe.ts (2)
Recipe(94-133)RecipeCreatePayload(181-184)
🔇 Additional comments (9)
app.json (1)
6-6: Verify version alignment and clarify the version jump rationale.The version bump is coordinated across multiple fields (version, versionCode, runtimeVersion), which is good practice. However, the jump from 3.2.6 to 3.3.8 is unconventional for semantic versioning—typically, a minor version bump would start at 3.3.0 rather than 3.3.8. Additionally, the versionCode increment from 191 to 200 is significant.
Please verify that:
- All version references across the codebase (package.json, android/app/build.gradle, android/app/src/main/res/values/strings.xml per the AI summary) are consistently updated to 3.3.8.
- The 3.3.8 version jump is intentional (e.g., aligning with a prior versioning scheme) or if this should be 3.3.0.
- The versionCode increment of +9 (191 → 200) aligns with your release versioning strategy.
Also applies to: 19-19, 61-61
tests/src/services/calculators/UnitConverter.test.ts (4)
61-89: Temperature conversion tests now align with canonical "F"/"C" behavior and Kelvin removalUsing uppercase
"F"/"C"in the main conversion tests and asserting the generic"Unsupported temperature unit"message matchesnormalizeTemperatureUnit’s canonicalization and error semantics, and the Kelvin-removal comment documents the new C/F-only surface clearly.
145-148: Temperature formatting expectations are consistent withtoFixed(1)+ normalized unitExpecting
"152.4°F"and"67.2°C"matchesformatTemperature’stemp.toFixed(1)plus°and the normalized"F"/"C"units; this keeps the test tightly coupled to the intended display format.
168-187: Error and same-unit behaviors forconvertTemperatureare well coveredThe tests for invalid
"from"/"to"units throwing"Unsupported temperature unit"and the same-unit"C" → "C"case returning the original value directly mirror the normalization + early-return logic inconvertTemperature, giving good coverage of both failure and no-op paths (with Kelvin explicitly documented as out-of-scope).
248-253:getTemperatureUnitstest correctly constrains the public API to C/F onlyAsserting that the returned units contain
"F"and"C"and have length 2 makes the C/F-only contract explicit and guards against accidentally reintroducing Kelvin or extra units via future changes.src/types/recipe.ts (1)
20-25: Type changes for units/metrics/creation payload look consistentSwitching
unit_system/mash_temp_unittoUnitSystem/TemperatureUnitand introducingRecipeMetricsInputplusRecipeCreatePayloadmatches how the BeerXML import and offline metrics flows are using these shapes. The intersections withRecipeandIngredientInputlook sound; no issues from a type‑safety perspective.Also applies to: 93-107, 136-152, 154-184
app/(modals)/(beerxml)/importReview.tsx (2)
192-274: Offline metrics query + display behaviour is robustThe
useQueryintegration withOfflineMetricsCalculatorlooks solid:
- Inputs are normalized via
normalizeImportedIngredientsandRecipeMetricsInput.- Validation happens first; invalid data logs a warning and returns
nullwithout surfacing an error state.- Real calculation errors are logged and re‑thrown so
metricsErrorcan render the fallback UI.- The query key includes batch/efficiency/boil/mash fields plus an ingredient fingerprint, so recomputation semantics are deterministic.
- The updated
calculatedMetrics.ibu != null/srm != nullchecks ensure zero values render correctly instead of being treated as falsy.No changes needed here.
Also applies to: 598-670
699-711: Time rendering now coerces before comparisonUsing an inline function with
coerceIngredientTime(ingredient.time)before checkingtime > 0fixes the earlier fragileingredient.time > 0check for string values (e.g.,"0","5"), while also handling non‑numeric input gracefully. This is a good robustness improvement.src/services/offlineV2/UserCacheService.ts (1)
195-198:UnitSystemtyping on cache/sync APIs is consistent with the new recipe typesUpdating
getRecipes/getBrewSessionsand the various hydrate/refresh methods to acceptuserUnitSystem: UnitSystem = "imperial"aligns these APIs with the newRecipe.unit_systemandRecipeFormData.unit_systemtypings. Callers likeuseUserDataandLegacyMigrationService.migrateLegacyRecipesToV2can now rely on the shared alias instead of ad‑hoc string unions, without changing runtime behaviour.Also applies to: 878-881, 2654-2657, 2709-2713, 3514-3518, 3733-3736
…ed ingredient management
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
src/hooks/offlineV2/useUserData.ts (5)
62-66: Enrich recipe load error log with contextual fieldsThe
UnifiedLogger.errorcall is fine, but the payload only contains the raw error. Adding structured context likeuserIdandunitSystemwould make these logs much more actionable when debugging multi‑user issues.For example:
- } catch (err) { - await UnifiedLogger.error( - "useRecipes.loadData", - "Error loading recipes:", - err - ); + } catch (err) { + await UnifiedLogger.error( + "useRecipes.loadData", + "Error loading recipes", + { error: err, userId, unitSystem } + );This keeps behavior the same while improving observability.
222-226: Include user + unit context in offline cache failure logThis error path only fires when both refresh and offline cache load fail, so the log will be high‑value. Right now it only logs
cacheError. Consider also logginguserIdForCacheandunitSystemso you can correlate failures by user/unit system in prod.Example:
- } catch (cacheError) { - await UnifiedLogger.error( - "useRecipes.refresh", - "Failed to load offline recipe cache:", - cacheError - ); + } catch (cacheError) { + await UnifiedLogger.error( + "useRecipes.refresh", + "Failed to load offline recipe cache", + { error: cacheError, userId: userIdForCache, unitSystem } + );Semantics remain unchanged.
343-347: Add context to brew session load error logs and consider non‑blocking loggingThe
UnifiedLogger.errorusage is correct, but two refinements could help:
- Include
userIdandunitSystemin the data payload to disambiguate which user/unit configuration failed:
} catch (err) {await UnifiedLogger.error("useBrewSessions.loadData","Error loading brew sessions:",err);
} catch (err) {await UnifiedLogger.error("useBrewSessions.loadData","Error loading brew sessions",{ error: err, userId, unitSystem });
- If
UnifiedLogger.errorcan perform network I/O, you may eventually want to avoid blocking the user on it in this path (e.g.,void UnifiedLogger.error(...);) so that logging failures don’t delay displaying cached data. That’s optional but worth considering for offline‑first UX.
494-498: Avoid blocking refresh fallback on logging and add more structured dataIn the brew‑session
refreshouter catch, logging before falling back to offline cache is good, but:
- Enrich the log with
userIdForCacheandunitSystemso you can see which user/session context failed.- Consider not awaiting the logger in this path so a slow or failing logging backend doesn’t delay the offline fallback in an already‑failing network scenario.
For example:
- } catch (error) { - await UnifiedLogger.error( - "useBrewSessions.refresh", - "Brew sessions refresh failed:", - error - ); + } catch (error) { + void UnifiedLogger.error( + "useBrewSessions.refresh", + "Brew sessions refresh failed", + { error, userId: userIdForCache, unitSystem } + );You still capture the error without coupling UX latency to the logging pipeline.
520-524: Log offline‑cache failure with rich context (user, unit system, prior error)This is the “last resort” path when both server refresh and offline cache load fail, so the log will be critical for debugging. Right now it only logs
cacheError.Consider including:
userIdForCacheunitSystem- The original
errorfrom the outer catch (so you can see both network and cache failure)Example:
- } catch (cacheError) { - await UnifiedLogger.error( - "useBrewSessions.refresh", - "Failed to load offline brew session cache:", - cacheError - ); + } catch (cacheError) { + await UnifiedLogger.error( + "useBrewSessions.refresh", + "Failed to load offline brew session cache", + { + cacheError, + originalError: error, + userId: userIdForCache, + unitSystem, + } + );This keeps behavior identical while making rare failure cases diagnosable.
src/types/recipe.ts (2)
45-47: Clarify invariants betweenidandingredient_idacross flows.Making
RecipeIngredient.idoptional and adding optionalingredient_idsupports both freshly created and fetched ingredients, but some flows (e.g. BeerXML import) now effectively requireingredient_idat runtime while UI types still mark it optional. Consider documenting or tightening this invariant (e.g., via a subtype likeResolvedRecipeIngredientfor post‑matching data) so it’s clearer which paths may legitimately lackingredient_id.Also applies to: 51-52
155-173:RecipeMetricsInputshape aligns with offline metrics use; payload clean‑up is reasonable.Using a minimal
RecipeMetricsInputfocused on batch size, boil/mash, and ingredients (withoutunit_system) matches how the OfflineMetricsCalculator expects data and reduces coupling to the fullRecipemodel. RemovingRecipeCreatePayload/IngredientInputin favor ofPartial<Recipe>+RecipeIngredientsimplifies the public surface and appears consistent with the BeerXML import and create flows.Also applies to: 175-176, 248-249
app/(modals)/(beerxml)/importReview.tsx (3)
192-209: Offline metrics query is well‑structured; consider reusing numeric coercion for boil time.The
useQuerykey includes all relevant numeric fields plus an ingredient fingerprint and usesRecipeMetricsInputwith sensible defaults,deriveUnitSystem, andderiveMashTempUnit, which fits the OfflineMetricsCalculator contract. Validation vs internal‑error handling is also clean.One small consistency tweak: for
boil_time, you currently userecipeData.boil_time || 60, while UI display usescoerceIngredientTime(recipeData.boil_time) ?? 60. To avoid discrepancies (e.g., string/invalid values or negative numbers), consider applyingcoerceIngredientTimehere as well and defaulting when it returnsundefined.Also applies to: 215-239, 241-274
281-325: OfflinecreateRecipepayload and cache invalidation look correct; guard against fully filtered ingredients.Using
normalizeImportedIngredients+deriveUnitSystemto build aPartial<Recipe>foruseRecipes.createis consistent with the metrics path, and the cache invalidation/priming plusrouter.replaceflow matches the offline‑first design. Error logging viaUnifiedLogger.erroris appropriate.Given the normalization filter can drop ingredients, it might be worth:
- Short‑circuiting the mutation (with a user‑facing alert) if
normalizedIngredients.length === 0, or- At least logging a warning before creating a recipe with no ingredients.
That would make it easier to detect edge cases where import/matching failed badly but the recipe is still created “empty”.
Also applies to: 329-340, 349-355, 369-370
553-569: Metrics display handles 0 IBU/SRM correctly; OG/FG/ABV still hide zero values.Switching IBU and SRM checks to
calculatedMetrics.ibu != null/srm != nullensures you now show legitimate0.0values instead of treating them as falsy, which is especially important for very low‑bitterness recipes. OG/FG/ABV continue to use simple truthiness checks; that’s fine in practice since 0 is unrealistic for OG/FG, but if you expect true 0‑ABV imports, you may want to align those checks to!= nullas well.Also applies to: 596-673
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(8 hunks)package.json(1 hunks)src/hooks/offlineV2/useUserData.ts(6 hunks)src/services/brewing/OfflineMetricsCalculator.ts(7 hunks)src/services/offlineV2/UserCacheService.ts(86 hunks)src/types/recipe.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/res/values/strings.xml
- src/services/offlineV2/UserCacheService.ts
- src/services/brewing/OfflineMetricsCalculator.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/types/recipe.ts (1)
src/types/common.ts (3)
ID(2-2)UnitSystem(27-27)TemperatureUnit(29-29)
src/hooks/offlineV2/useUserData.ts (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (8)
android/app/build.gradle (1)
95-96: Verify versionCode increment strategy.The version fields are synchronized correctly with package.json and app.json. However, the versionCode jump from 191 to 201 (+10) is unconventional—Android versioning typically increments by 1 per build. Similarly, the version string jumps from 3.2.6 directly to 3.3.9 instead of the conventional 3.3.0 for a minor version bump.
Please confirm that these versioning decisions are intentional.
package.json (1)
4-4: Version bump synchronized correctly.The package.json version field is correctly updated to "3.3.9" and synchronized with android/app/build.gradle and app.json. No dependencies or build scripts were modified, so this is a clean metadata-only change.
app.json (1)
6-6: Expo version fields properly synchronized.All three Expo configuration version fields—expo.version (line 6), android.versionCode (line 19), and runtimeVersion (line 61)—are correctly synchronized with the corresponding values in android/app/build.gradle and package.json. This ensures consistent versioning across the build system and runtime.
Also applies to: 19-19, 61-61
src/hooks/offlineV2/useUserData.ts (1)
13-13: Named import ofUnifiedLoggerlooks correctSwitching to
import { UnifiedLogger } from "@services/logger/UnifiedLogger";aligns this hook with the named export pattern described forUnifiedLoggerand should avoid default‑import mismatches. No issues here.src/types/recipe.ts (1)
20-25: Centralizing unit types looks consistent and future‑proof.Importing
TemperatureUnit/UnitSystemfrom./commonand wiring them intoRecipe/RecipeFormDataplusBatchSizeUnitis a clean consolidation of unit handling. This keeps Recipe-related types aligned with the new unit‑system helpers and OfflineMetricsCalculator usage.Also applies to: 103-104, 144-145, 107-108
app/(modals)/(beerxml)/importReview.tsx (3)
29-35: Unit helpers and logging/import wiring look solid and fix prior precedence issues.Importing shared types (
Recipe*,UnitSystem,TemperatureUnit), offline hooks, andUnifiedLogger, plus introducingderiveUnitSystemandderiveMashTempUnit, gives a clear, typed path for unit handling. The newderiveMashTempUnitcorrectly normalizes"C"/"F"and falls back based onunitSystem, avoiding the earlier??operator‑precedence bug and making behavior predictable.Also applies to: 40-42, 44-57, 63-81
170-171: JSON parse error handling is safe and logs correctly.Using a lazy
useStateinitializer aroundJSON.parsewith aUnifiedLogger.erroron failure and falling back tonullis appropriate. The earlyif (!recipeData)render branch ensures the rest of the component doesn’t dereference invalid data.Also applies to: 175-183
680-710: Time rendering now correctly normalizes before comparison.The new IIFE using
coerceIngredientTime(ingredient.time)and then checkingtime !== undefined && time > 0fixes the earlier fragility where string times like"0"/"5"relied on implicit coercion. This is a robust pattern for these imported values.
feat: enhance BeerXML import workflow with unit conversion choice - Added unit choice step after parsing BeerXML files - Updated modal to allow users to select metric or imperial units - Implemented normalization for brewing-friendly measurements fix: refactor BeerXMLService to handle unit conversion and normalization - Updated convertRecipeUnits method to return warnings and handle normalization - Adjusted API service to include new conversion endpoint test: update tests for ImportBeerXMLScreen to reflect new unit choice logic - Modified tests to ensure unit choice modal is displayed after parsing - Ensured conversion logic is correctly tested with mocked services style: clean up imports and improve code readability across components
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/app/(modals)/(beerxml)/importBeerXML.test.tsx (1)
461-580: Unit conversion tests don’t yet exercise the behavior their names claimThe new “Unit Conversion Workflow” tests mostly verify that mocks exist and that parsing ran:
"should always show unit choice modal after parsing..."never checks thatUnitConversionChoiceModalwas rendered (or that itsvisibleprop becametrue); it only asserts thatparseBeerXML/importBeerXMLFilewere called, which is already covered earlier."should convert recipe to chosen unit system with normalization"and"should handle conversion errors gracefully"only assert thatconvertRecipeUnitsis defined, but do not trigger any code path that calls it or assert on how its result (or failures) affect navigation/state.To make these tests meaningful, you could either:
- Strengthen them to simulate a user choice by changing the
UnitConversionChoiceModalmock to capture props and invokeonChooseMetric/onChooseImperial, then assert thatconvertRecipeUnitsis called with the expected arguments and that the converted recipe is used; or- Rename them to reflect the narrower coverage if you prefer to keep them as wiring/smoke tests.
🧹 Nitpick comments (5)
src/components/beerxml/UnitConversionChoiceModal.tsx (1)
156-331: Loading state currently shows “Converting...” on both options
isConvertingdrives the loading UI for both the metric and imperial buttons, so once a conversion starts, both buttons render a spinner and “Converting...”, even though only one action is actually in-flight. Consider tracking which target unit was chosen (e.g.,convertingTarget: "metric" | "imperial" | null) in the parent and passing two flags so only the active choice shows a spinner while the other is just disabled.src/services/offlineV2/UserCacheService.ts (2)
194-305: Debug logging builds large payloads in hot paths
getRecipesandgetBrewSessionsbuild detaileditemDetails/returned*arrays and pass them intoUnifiedLogger.debugon every call. With many cached items this can become expensive (object allocation + JSON serialization) even if debug logs are ultimately dropped at runtime. Consider guarding the heavy-detail payloads behind a dev flag or a cheaper check (e.g., only includeitemDetailswhen__DEV__or when an explicit “offline-cache-debug” config is enabled).Also applies to: 874-989
3075-3127: Per-item debug logs may be noisy at scaleIn
getCachedRecipesandgetCachedBrewSessions, every non-matching user record logs a separateUnifiedLogger.debug("offline-cache", ...)entry. With many users or shared caches this can flood logs and slow down cache reads. If you still need this visibility, consider:
- Logging only a sample of mismatches (e.g., first N),
- Aggregating counts by
user_idin a single message, or- Enabling these per-item logs only under
__DEV__/ a dedicated debug flag.Also applies to: 3215-3274
src/contexts/DeveloperContext.tsx (1)
174-235: Consider consolidating duplicate UnifiedLogger calls in error pathsSeveral catch blocks log the same failure twice: once with a detailed category (e.g.,
"DeveloperContext.setNetworkSimulationMode","DeveloperContext.makeQueryCacheStale") and again with the generic"developer"category. This isn’t incorrect, but it can add noise and volume in log backends without much extra signal. If you don’t rely on both categories downstream, consider dropping the secondary"developer"calls or folding that context into the structureddatapayload on the primary log.Also applies to: 239-304, 319-331, 360-397, 403-427, 435-495
src/services/beerxml/BeerXMLService.ts (1)
1-5: Verify UnifiedLogger import alias for consistencyThis file imports the logger as:
import { UnifiedLogger } from "@/src/services/logger/UnifiedLogger";Other modules appear to use
@services/logger/UnifiedLogger. If the@/alias is not configured (or configured differently from@services), this import will fail at build/test time even though types look fine. It would be safer and more consistent to align with the existing alias, e.g.:-import { UnifiedLogger } from "@/src/services/logger/UnifiedLogger"; +import { UnifiedLogger } from "@services/logger/UnifiedLogger";Please confirm which alias is actually configured in your tsconfig/jest/moduleNameMapper setup and adjust accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importBeerXML.tsx(9 hunks)app/(modals)/(brewSessions)/viewBrewSession.tsx(1 hunks)package.json(1 hunks)src/components/beerxml/UnitConversionChoiceModal.tsx(1 hunks)src/components/brewSessions/DryHopTracker.tsx(1 hunks)src/contexts/DeveloperContext.tsx(9 hunks)src/services/api/apiService.ts(8 hunks)src/services/beerxml/BeerXMLService.ts(9 hunks)src/services/config.ts(1 hunks)src/services/offlineV2/UserCacheService.ts(87 hunks)src/styles/components/beerxml/unitConversionModalStyles.ts(1 hunks)src/types/api.ts(3 hunks)tests/app/(modals)/(beerxml)/importBeerXML.test.tsx(5 hunks)tests/src/contexts/DeveloperContext.test.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- android/app/src/main/res/values/strings.xml
- package.json
- src/services/api/apiService.ts
- src/styles/components/beerxml/unitConversionModalStyles.ts
- android/app/build.gradle
🧰 Additional context used
🧬 Code graph analysis (5)
tests/src/contexts/DeveloperContext.test.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/components/beerxml/UnitConversionChoiceModal.tsx (4)
src/types/common.ts (1)
UnitSystem(27-27)src/contexts/ThemeContext.tsx (1)
useTheme(109-115)src/styles/components/beerxml/unitConversionModalStyles.ts (1)
unitConversionModalStyles(9-100)src/styles/common/colors.ts (1)
colors(85-85)
src/types/api.ts (2)
src/types/common.ts (2)
TemperatureUnit(29-29)UnitSystem(27-27)src/types/recipe.ts (1)
Recipe(95-134)
src/services/beerxml/BeerXMLService.ts (3)
src/types/recipe.ts (1)
Recipe(95-134)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/common.ts (1)
UnitSystem(27-27)
src/contexts/DeveloperContext.tsx (3)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
🔇 Additional comments (16)
app.json (1)
6-6: Verify the versionCode increment rationale.The version updates are consistent across the app.json configuration: semantic versioning aligns across version (3.3.10), runtimeVersion (3.3.10), and versionCode (202) per the AI summary. However, the versionCode jump from 191 to 202 is unusually large (+11) compared to typical Android release increments of +1 per build. Please confirm this is intentional rather than a typo.
Also applies to: 19-19, 61-61
app/(modals)/(brewSessions)/viewBrewSession.tsx (1)
26-26: UnifiedLogger named import change looks consistentSwitching to
import { UnifiedLogger } from "@services/logger/UnifiedLogger";matches a move toward named exports while leaving all call sites (UnifiedLogger.error/info/debug) unchanged and type-safe. As long as the logger module now exposes a namedUnifiedLogger, this refactor is good and keeps behavior identical.src/components/brewSessions/DryHopTracker.tsx (1)
35-35: Named import forUnifiedLoggerlooks correct and consistentSwitching to
import { UnifiedLogger } from "@services/logger/UnifiedLogger";is appropriate as long as the logger module now exposesUnifiedLoggeras a named export, and it keeps existingUnifiedLogger.debug/info/errorcalls intact. No further changes needed in this file.src/services/config.ts (1)
163-170: BeerXML CONVERT_RECIPE endpoint wiring looks consistentThe new
ENDPOINTS.BEERXML.CONVERT_RECIPEpath ("/beerxml/convert-recipe") aligns with the BeerXML service’sconvertRecipecall and keeps the BeerXML routes grouped under a single namespace. No issues from a typing or routing perspective.tests/src/contexts/DeveloperContext.test.tsx (1)
14-21: UnifiedLogger test mock is compatible with DeveloperContext usageMocking
UnifiedLoggeras an object withwarn/error/info/debug: jest.fn()matches how the context uses it (includingawaitin some paths), and the assertions on category/message arguments for storage and reset errors are aligned with the implementation. This keeps the tests focused on behavior without depending on the logger internals.Also applies to: 30-38, 157-175, 200-221, 313-333
src/types/api.ts (1)
28-33: Unit/BeerXML API typings are consistent with shared types and service usage
- Switching
mash_temp_unittoTemperatureUnitreuses the shared"C" | "F"union and stays backward compatible at the type level.BeerXMLConvertRecipeRequest/BeerXMLConvertRecipeResponsecorrectly model the payload used inBeerXMLService.convertRecipeUnits(recipe,target_system: UnitSystem, optionalnormalize, and optionalwarningscoming back).No functional issues spotted.
Also applies to: 187-195, 410-420
src/services/beerxml/BeerXMLService.ts (1)
13-16: BeerXML unit detection and conversion behavior looks robust
BeerXMLRecipeextendingPartial<Recipe>with a requiredingredientsarray makes downstream usage (e.g., conversion and matching) safer.detectRecipeUnitSystemnormalizes units (trim + lowercase) and distinguishes metric vs imperial using bothbatch_size_unitand per-ingredientunit, returning"mixed"when ambiguous—good defensive default for callers.convertRecipeUnitsdelegates toApiService.beerxml.convertRecipewith{ recipe, target_system, normalize }, and on failure or missingresponse.data.recipeit logs viaUnifiedLoggerand returns the original recipe rather than blocking import.This is a sensible, failure-tolerant integration of the new backend conversion endpoint.
Also applies to: 404-499
tests/app/(modals)/(beerxml)/importBeerXML.test.tsx (1)
10-25: Mocks for theming, units, and BeerXMLService are aligned with the new flow
useThemeanduseUnitsmocks now expose the colors and unit-related helpers ImportBeerXMLScreen expects.- The BeerXMLService mock provides
importBeerXMLFile,parseBeerXML, and aconvertRecipeUnitsstub that resolves to{ recipe, warnings: [] }, which matches the new service API.UnitConversionChoiceModalis mocked to rendernull, which is fine as long as conversion-specific behavior is tested via state or handler effects rather than actual modal UI.This setup should keep the tests focused on screen logic without pulling in the real modal or service implementations.
Also applies to: 26-45, 51-62, 64-66, 201-241
app/(modals)/(beerxml)/importBeerXML.tsx (8)
28-37: LGTM: Imports are well-organized and necessary.All new imports are properly used throughout the file to support the unit conversion flow integration.
39-50: LGTM: State interface properly extended for unit conversion flow.The
unit_choicestep andisConvertingfield are both used correctly throughout the component to manage the new unit conversion workflow.
113-147: LGTM: Parsing logic correctly integrated with unit conversion flow.The automatic selection of the first recipe and transition to the
unit_choicestep streamlines the user experience. Error logging follows the correct pattern.
149-208: LGTM: Unit conversion handler is well-implemented.The function properly manages loading states, handles both warnings and errors gracefully, and provides clear user feedback. The defensive check for warnings existence (line 172) is good practice.
213-229: LGTM: Navigation function properly supports converted recipe flow.The optional
recipeparameter allows passing the converted recipe while maintaining backward compatibility with existing callers. The logic correctly prioritizes the parameter over state.
480-494: LGTM: Modal integration is properly implemented.The
UnitConversionChoiceModalis correctly wired with appropriate visibility conditions, handlers, and props. The cancel handler provides a good escape path back to file selection.
55-55: LGTM: Unit context integration.The
unitSystemis correctly extracted from the context and used to inform the unit conversion modal about the user's preferred units.
242-242: LGTM: State reset includes conversion flag.The
isConvertingfield is properly reset to maintain consistent state.
- Unify UnifiedLogger imports across app - Fix BrewSession temp id -> server id mapping so that newly created recipes that haven't been synced yet can successfully have brew sessions created from them
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (13)
app/(auth)/resetPassword.tsx (1)
187-201: Good move to centralized logging; consider not awaiting logger in UI flowUsing
UnifiedLogger.error("resetPassword.handleResetPassword", "Password reset failed:", err)in the catch block is correct and matches the logger’s(category, message, data?)signature. This brings the screen in line with the unified logging approach.One optional improvement: you might want to drop the
awaitso the user-facing flow (including the fallback alert) is not coupled to logger latency or transient logging issues, e.g.:- } catch (err: unknown) { - // Error is handled by the context and displayed through error state - await UnifiedLogger.error( + } catch (err: unknown) { + // Error is handled by the context and displayed through error state + UnifiedLogger.error( "resetPassword.handleResetPassword", "Password reset failed:", err );Since
UnifiedLogger.erroris documented as non-throwing, fire-and-forget is typically safe and keeps the UX snappier.src/services/offlineV2/UserCacheService.ts (1)
48-52: Consider standardizing logging surface across the file.The file uses two logging surface patterns inconsistently:
- Generic "offline-cache" surface (lines 49, 115, 141, 322, etc.)
- Method-specific surfaces like "UserCacheService.getRecipes" (line 227)
While functional, standardizing on one approach (preferably method-specific surfaces for better log filtering) would improve maintainability. This could be deferred to a follow-up refactoring task.
Also applies to: 115-118, 141-145, 210-214, 322-322, 423-423
src/services/offlineV2/StartupHydrationService.ts (4)
8-12: Use a type-only import forUnitSystemSince
UnitSystemis a type, prefer a type-only import to avoid pulling it into runtime bundles:-import { UnitSystem } from "@/src/types"; +import type { UnitSystem } from "@/src/types";This keeps the import tree cleaner and matches TS best practices.
20-49: ClarifyhasHydratedsemantics withPromise.allSettledand align logging await behaviorTwo points here:
Promise.allSettled+ unconditionalthis.hasHydrated = truemeans the service will consider hydration “done” even if user data or static data hydration failed (only warnings are logged, and subsequent calls will early-return). If the intent is “only skip future startup hydration when both succeed”, consider inspecting the settled results before flippinghasHydrated:- // Hydrate in parallel for better performance - await Promise.allSettled([ - this.hydrateUserData(userId, userUnitSystem), - this.hydrateStaticData(), - ]); - - this.hasHydrated = true; + // Hydrate in parallel for better performance + const results = await Promise.allSettled([ + this.hydrateUserData(userId, userUnitSystem), + this.hydrateStaticData(), + ]); + + const hasFailure = results.some(r => r.status === "rejected"); + if (!hasFailure) { + this.hasHydrated = true; + }
- In the catch, you
await UnifiedLogger.error(...)while other log calls usevoid UnifiedLogger.warn(...). For consistency (and to avoid making hydration duration depend on logging), you can drop theawaitand follow the same fire-and-forget pattern:- await UnifiedLogger.error( + void UnifiedLogger.error(
54-81: Remove the no-opelsebranch inhydrateUserDataThe
elseblock after theexistingRecipes.length === 0check is empty, which adds noise without behavior:if (existingRecipes.length === 0) { // ... } else { }You can safely drop the
elseand keep only theif+ comment, which will slightly simplify the method.
86-123: Slight naming cleanup for cache stats and confirm failure behavior
getCacheStats()returns both ingredients and beer styles stats, but the variable is namedingredientsStatswhile also used foringredientsStats.beerStyles.cached. Renaming to something likecacheStatswould better reflect its contents:- const ingredientsStats = await StaticDataService.getCacheStats(); - if (!ingredientsStats.ingredients.cached) { + const cacheStats = await StaticDataService.getCacheStats(); + if (!cacheStats.ingredients.cached) { ... - if (!ingredientsStats.beerStyles.cached) { + if (!cacheStats.beerStyles.cached) {Also, static data failures are logged via
UnifiedLogger.warnand then swallowed, which keeps startup resilient but means callers can’t distinguish “never hydrated” from “hydration failed but we moved on”. If that’s intentional, this implementation is fine; if not, you may want to expose or track a separate static-data hydration status.src/services/beerxml/BeerXMLService.ts (2)
129-129: Missingvoidorawaitfor async UnifiedLogger calls.These
UnifiedLogger.errorcalls are missing thevoidprefix orawait. While they work, explicitly marking fire-and-forget promises withvoidsilences ESLint warnings and documents intent.Apply this pattern consistently:
- UnifiedLogger.error("beerxml", "🍺 BeerXML Export - Error:", error); + void UnifiedLogger.error("beerxml", "🍺 BeerXML Export - Error:", error);Also applies to: 159-159, 198-198, 252-252, 276-276
185-188: Missingvoidfor fire-and-forget logger call.This
UnifiedLogger.warncall is async but the result is not awaited or explicitly voided.- UnifiedLogger.warn( + void UnifiedLogger.warn( "beerxml", "🍺 BeerXML Parse - No recipes found in response" );app/(modals)/(beerxml)/importBeerXML.tsx (1)
99-103: Missingvoidorawaitfor async UnifiedLogger calls.These
UnifiedLoggercalls are async but the result is not awaited or explicitly voided. For consistency with other files in this PR that usevoid UnifiedLogger..., add thevoidprefix.- UnifiedLogger.error( + void UnifiedLogger.error( "beerxml", "🍺 BeerXML Import - File selection error:", error );Apply the same pattern to lines 136, 174, and 188.
Also applies to: 136-140, 174-178, 188-192
app/(modals)/(beerxml)/importReview.tsx (2)
246-250: Unnecessaryawaiton logger calls blocks query execution.The
awaitkeywords on lines 246 and 260 unnecessarily block the queryFn while logging completes. Since the log result isn't used and the function either returnsnullor throws afterward, usevoidinstead (as done on line 74) for non-blocking async logging.Apply this diff:
if (!validation.isValid) { - await UnifiedLogger.warn( + void UnifiedLogger.warn( "import-review", "Invalid recipe data for metrics calculation", validation.errors ); return null; } try { const metrics = OfflineMetricsCalculator.calculateMetrics(recipeFormData); return metrics; } catch (error) { - await UnifiedLogger.error( + void UnifiedLogger.error( "import-review", "Unexpected metrics calculation failure", error ); throw error; }Also applies to: 260-264
705-708: Time coercion issue fixed, but consider simplifying the pattern.The IIFE now correctly coerces the time value before checking it, addressing the previous review concern. However, the nested arrow function pattern is somewhat complex.
For better readability, consider this simpler approach:
- {(time => - time !== undefined && time > 0 - ? ` • ${time} min` - : "")(coerceIngredientTime(ingredient.time))} + {(() => { + const time = coerceIngredientTime(ingredient.time); + return time !== undefined && time > 0 ? ` • ${time} min` : ""; + })()}tests/src/services/NotificationService.test.ts (2)
90-95: Fix duplicateerrorclear / missinginfoclear (or drop explicit clears entirely)In
beforeEach, you:// Clear UnifiedLogger mocks jest.mocked(UnifiedLogger.error).mockClear(); jest.mocked(UnifiedLogger.warn).mockClear(); jest.mocked(UnifiedLogger.error).mockClear(); jest.mocked(UnifiedLogger.debug).mockClear();Issues:
UnifiedLogger.erroris cleared twice.UnifiedLogger.infois never cleared.- This is also redundant with the earlier
jest.clearAllMocks()which already resets alljest.fn()s.I’d either rely purely on
jest.clearAllMocks()or fix the typo so the intent matches the code:- // Clear UnifiedLogger mocks - jest.mocked(UnifiedLogger.error).mockClear(); - jest.mocked(UnifiedLogger.warn).mockClear(); - jest.mocked(UnifiedLogger.error).mockClear(); - jest.mocked(UnifiedLogger.debug).mockClear(); + // (Optional) Explicitly clear UnifiedLogger mocks on top of jest.clearAllMocks() + jest.mocked(UnifiedLogger.error).mockClear(); + jest.mocked(UnifiedLogger.warn).mockClear(); + jest.mocked(UnifiedLogger.info).mockClear(); + jest.mocked(UnifiedLogger.debug).mockClear();Given
jest.clearAllMocks()is already present, you could also safely delete this block for less duplication.
618-632: Align test description with log level (debug vs warn) for dev-only hop alert loggingThe test name says
"should log debug information in development"but the assertion checks a warn-level log:expect(UnifiedLogger.warn).toHaveBeenCalledWith( "notifications", expect.stringContaining('⚠️ Skipping hop alert for "Very Late Hop"') );Either behavior is fine, but for clarity and future maintenance it would be good to align them:
- If this really is intended as a warning, consider renaming the test to something like
"should log a warning in development when skipping hop alerts", or- If it’s intended as debug-only noise, consider logging via
UnifiedLogger.debugin the implementation and updating the test accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(auth)/resetPassword.tsx(2 hunks)app/(modals)/(beerxml)/importBeerXML.tsx(9 hunks)app/(modals)/(beerxml)/importReview.tsx(8 hunks)app/(modals)/(brewSessions)/viewBrewSession.tsx(1 hunks)package.json(1 hunks)src/components/banners/StaleDataBanner.tsx(0 hunks)src/components/recipes/AIAnalysisResultsModal.tsx(0 hunks)src/contexts/AuthContext.tsx(15 hunks)src/contexts/NetworkContext.tsx(5 hunks)src/contexts/UnitContext.tsx(10 hunks)src/services/NotificationService.ts(14 hunks)src/services/api/apiService.ts(8 hunks)src/services/beerxml/BeerXMLService.ts(9 hunks)src/services/offlineV2/StartupHydrationService.ts(6 hunks)src/services/offlineV2/StaticDataService.ts(22 hunks)src/services/offlineV2/UserCacheService.ts(67 hunks)src/services/storageService.ts(12 hunks)tests/src/contexts/UnitContext.test.tsx(14 hunks)tests/src/services/NotificationService.test.ts(12 hunks)
💤 Files with no reviewable changes (2)
- src/components/recipes/AIAnalysisResultsModal.tsx
- src/components/banners/StaleDataBanner.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- src/services/NotificationService.ts
- app/(modals)/(brewSessions)/viewBrewSession.tsx
- src/services/storageService.ts
- src/services/offlineV2/StaticDataService.ts
- src/services/api/apiService.ts
- src/contexts/AuthContext.tsx
- package.json
- app.json
🧰 Additional context used
🧬 Code graph analysis (6)
tests/src/services/NotificationService.test.ts (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/services/beerxml/BeerXMLService.ts (2)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/common.ts (1)
UnitSystem(27-27)
tests/src/contexts/UnitContext.test.tsx (2)
src/types/common.ts (1)
UnitSystem(27-27)src/contexts/UnitContext.tsx (2)
UnitProvider(128-692)useUnits(120-126)
app/(auth)/resetPassword.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
src/services/offlineV2/UserCacheService.ts (6)
src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)src/types/common.ts (1)
UnitSystem(27-27)src/types/offlineV2.ts (2)
OfflineError(267-276)STORAGE_KEYS_V2(221-244)src/utils/recipeUtils.ts (1)
isTempId(17-19)src/types/brewSession.ts (1)
BrewSession(45-112)
src/contexts/UnitContext.tsx (1)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
🔇 Additional comments (28)
app/(auth)/resetPassword.tsx (1)
50-50: UnifiedLogger import aligns with centralized logging strategyImporting
UnifiedLoggerfrom@services/logger/UnifiedLoggeris consistent with the project-wide migration away fromconsole.errorand matches the documented API. No issues here.android/app/build.gradle (1)
95-96: Verify the large version jump is intentional.The version bump from
3.2.6to3.3.11is substantial, skipping multiple intermediate minor versions (3.3.0–3.3.10). This is unusual for a single release. While the accompanying versionCode increment of 12 (191→203) is proportionate, please confirm that this versioning reflects the actual scope of changes intended for this release and that it aligns with your release planning.android/app/src/main/res/values/strings.xml (1)
4-4: Version string aligned with build.gradle.The update to
expo_runtime_versionis consistent with the correspondingversionNamebump inbuild.gradle. No concerns with this change.src/services/offlineV2/UserCacheService.ts (4)
17-17: LGTM: Import changes align with standardization.The migration to named export for UnifiedLogger and the introduction of the UnitSystem type improve type safety and standardize the codebase.
Also applies to: 29-29
4059-4107: LGTM: Enhanced pending operations tracking improves observability.The detailed tracking of different ID mapping types (entityIds, brew session recipe refs, parentIds) with separate counters and consolidated logging significantly improves debugging visibility for the temp-ID resolution flow. The logic correctly categorizes and counts each mapping type.
196-196: LGTM with minor note: UnitSystem type improves type safety.The migration from string unions to the UnitSystem type provides better type safety and consistency. Minor observation: default values vary between "metric" (line 196, 2425) and "imperial" (lines 766, 2386, 2986, 3145) across methods. This appears intentional (recipes default to metric, brew sessions to imperial), but ensure this aligns with product requirements.
Also applies to: 766-766, 2386-2386, 2425-2425, 2986-2986, 3145-3145
2696-2698: LGTM: User filtering maintains data isolation.The consistent filtering by user_id in both getCachedRecipes and getCachedBrewSessions ensures proper user data isolation and prevents cross-user data access. This is essential security hygiene for the offline cache.
Also applies to: 2800-2802
src/contexts/UnitContext.tsx (3)
40-40: LGTM - UnifiedLogger integration for warning paths.The migration from
console.warntoUnifiedLogger.warnis correctly implemented with thevoidprefix for fire-and-forget async calls, ensuring non-blocking behavior in the preference loading flow.Also applies to: 162-166, 188-192, 229-233, 262-266
308-311: LGTM - Appropriate async handling for error logging.Line 326 correctly uses
awaitfor the error path inupdateUnitSystemsince it's within an async function with error handling, while line 308 usesvoidfor the non-critical warning path during cache corruption recovery.Also applies to: 326-326
350-350: LGTM - Temperature unit normalization to uppercase.The standardization of temperature units from lowercase (
c/f) to uppercase (C/F) is consistently applied acrossgetPreferredUnit,convertUnit, andgetCommonUnits. This aligns with the uppercased labels (°C/°F) in the UI.Also applies to: 419-421, 427-431, 631-632
tests/src/contexts/UnitContext.test.tsx (3)
45-71: LGTM - Improved test wrapper pattern.Refactoring
createWrapperto return a namedTestWrappercomponent instead of an anonymous function improves debuggability in React DevTools and follows React best practices for component definitions.
233-233: LGTM - Temperature unit expectations updated consistently.All temperature-related test expectations are correctly updated to use uppercase units (
"C","F") matching the implementation changes in UnitContext.tsx.Also applies to: 242-242, 309-311, 742-742, 753-753, 798-800, 1084-1086, 1093-1095
439-442: LGTM - UnifiedLogger assertions correctly verify logging behavior.The test assertions properly verify that
UnifiedLogger.warnandUnifiedLogger.errorare called with the expected category and message parameters.Also applies to: 522-525, 978-982, 1044-1048
src/contexts/NetworkContext.tsx (3)
169-173: LGTM - Consistent UnifiedLogger migration for error paths.All
console.warncalls are correctly replaced withUnifiedLogger.warnusing thevoidprefix for non-blocking async behavior, maintaining the existing error handling flow.Also applies to: 348-352, 377-381, 393-397
230-245: LGTM - Smart dynamic log level selection.The computed
logMethodpattern elegantly reduces log noise by usinginfolevel only when the online status flips, anddebugfor routine state updates. This improves observability without flooding logs.
264-278: LGTM - Conditional logging for background refresh results.The ternary-based log level selection (
failures.length ? "warn" : "debug") appropriately escalates logging only when failures occur.src/services/beerxml/BeerXMLService.ts (3)
408-448: LGTM - Well-designed unit system detection.The
detectRecipeUnitSystemmethod has solid logic:
- Checks batch_size_unit first as a strong signal
- Aggregates ingredient unit counts
- Returns "mixed" for ambiguous cases
- Properly handles whitespace with
.trim()
462-499: LGTM - Robust unit conversion with graceful fallback.The
convertRecipeUnitsmethod correctly:
- Defaults
normalizetotruefor brewing-friendly values- Returns original recipe if API fails (doesn't block import)
- Logs both errors and warnings appropriately
The docstring clearly explains the BeerXML metric specification and normalization rationale.
503-504: LGTM - Singleton pattern for service.Exporting a singleton instance is appropriate for this stateless service class.
app/(modals)/(beerxml)/importBeerXML.tsx (3)
40-40: LGTM - Unit choice step properly integrated.The
"unit_choice"step is now correctly wired into the flow:
- After parsing, the first recipe is auto-selected
- Flow transitions to
unit_choicestepUnitConversionChoiceModalrenders when step is"unit_choice"This addresses the previous review comment about the unused step value.
Also applies to: 124-134, 480-495
150-209: LGTM - Well-structured unit conversion handler.The
handleUnitSystemChoicefunction properly:
- Sets
isConvertingstate for loading UI feedback- Calls the backend conversion with normalization enabled
- Logs warnings if present
- Handles errors gracefully with user feedback via Alert
- Resets to file_selection on failure for clean retry
214-230: LGTM - Flexible recipe parameter for ingredient matching.Making the
recipeparameter optional with fallback toimportState.selectedRecipemaintains backward compatibility while allowing direct passing of the converted recipe.app/(modals)/(beerxml)/importReview.tsx (4)
44-81: LGTM! Unit derivation logic properly refactored.The new
deriveUnitSystemandderiveMashTempUnithelpers centralize and fix the unit detection issues flagged in previous reviews. The operator precedence bug in temperature unit derivation has been resolved, and proper validation with fallbacks is now in place.
100-158: Good normalization strategy addresses previous divergence concern.The centralized
normalizeImportedIngredientsfunction is now consistently used for both metrics calculation (line 216) and recipe creation (line 282), which resolves the previous issue where preview and persisted ingredients could diverge.Minor efficiency note: Normalization runs twice (once for metrics query, once for mutation). Consider memoizing the normalized result if performance becomes a concern.
279-376: LGTM! Solid offline-first mutation implementation.The recipe creation flow now uses the offline V2 hook, which creates a temporary recipe for immediate display before syncing to the server. Cache invalidation strategy appropriately covers all relevant query keys, and priming the detail cache (lines 336-339) ensures smooth navigation.
The use of
router.replace(line 351) maintains consistency with other recipe creation flows.
653-653: LGTM! Proper null checks for zero-valued metrics.The
!= nullchecks correctly distinguish between missing metrics and zero values, ensuring that valid IBU/SRM values of 0 are displayed.Also applies to: 661-661
tests/src/services/NotificationService.test.ts (2)
136-140: UnifiedLogger-based assertions look consistent with the new logging strategyAll the updated expectations on
UnifiedLogger.warn/UnifiedLogger.erroruse a consistent category ("notifications") and message shapes that mirror likely production behavior for:
- Permission denial during initialization (lines 136-140).
- Initialization, scheduling, cancellation, haptics, permission check, and getScheduled error paths (lines 178-182, 246-250, 389-393, 417-421, 448-452, 526-530, 553-557).
- The “very late hop” dev-only debug/warn case (lines 628-631).
This gives good coverage of error and edge paths around notifications and hop alerts with the new logger abstraction. Once the mock path issue is fixed, these expectations should be robust.
Also applies to: 178-182, 246-250, 389-393, 417-421, 448-452, 526-530, 553-557, 628-631
14-14: The import and mock paths forUnifiedLoggerare actually correct and resolve to the same module. Jest'smoduleNameMapperconfiguration maps both@services/logger/UnifiedLogger(import path at line 14) and@/src/services/logger/UnifiedLogger(mock path at line 57) to<rootDir>/src/services/logger/UnifiedLogger. The mock will properly apply to the import, andjest.mocked(UnifiedLogger.*).mockClear()calls will work as expected at runtime.Likely an incorrect or invalid review comment.
- Extracted duplicated temp recipe_id resolution logic into resolveTempRecipeId() helper - Changed isConverting: boolean → convertingTarget: UnitSystem | null so that only the active button shows spinner, other is just disabled - StartupHydrationService Semantics: Only sets hasHydrated = true if both user data AND static data succeed - Fix: BeerXML Preview Divergence: Normalized ingredients computed once with useMemo. Same ingredients used for preview, metrics, and creation
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/offlineV2/UserCacheService.ts (1)
800-812: Remove duplicate error logging.The error is logged twice - once with structured context (awaited) and again immediately after without await. This creates redundant log entries.
await UnifiedLogger.error( "UserCacheService.getBrewSessions", `Error getting brew sessions: ${error instanceof Error ? error.message : "Unknown error"}`, { userId, error: error instanceof Error ? error.message : "Unknown error", } ); - UnifiedLogger.error( - "offline-cache", - "Error getting brew sessions:", - error - ); throw new OfflineError(
♻️ Duplicate comments (1)
app/(modals)/(beerxml)/importBeerXML.tsx (1)
99-103: Addvoidprefix to fire-and-forget UnifiedLogger call.The
UnifiedLogger.errorcall returns a Promise but is not awaited. Addvoidprefix for consistency with the rest of the codebase and to suppress unhandled-promise warnings.- UnifiedLogger.error( + void UnifiedLogger.error( "beerxml", "🍺 BeerXML Import - File selection error:", error );
🧹 Nitpick comments (5)
src/services/offlineV2/StartupHydrationService.ts (1)
81-84: Remove empty conditional block.This
ifblock contains only a comment explaining what happens elsewhere. The comment should be outside the block, and the conditional can be removed entirely since it serves no purpose.- if (existingRecipes.length === 0) { - // The UserCacheService.getRecipes() method will automatically hydrate - // So we don't need to do anything special here - } + // The UserCacheService.getRecipes() method will automatically hydrate + // if no cached recipes are found, so we don't need additional logic here.src/services/offlineV2/UserCacheService.ts (2)
3227-3231: Remove empty debug block.This empty
if (__DEV__)block appears to be leftover from removed debug logging. Clean it up to avoid confusion.// Debug logging for sanitized result - if (__DEV__) { - } - return sanitized; }
2422-2426: Parameter prepared for future use but defaults inconsistent.The
_userUnitSystemparameter is prepared for future unit conversion integration (per PR title). However, the default"metric"differs fromhydrateBrewSessionsFromServerwhich uses"imperial". Consider aligning these defaults for consistency when the feature is implemented.tests/app/(modals)/(beerxml)/importReview.test.tsx (1)
68-109: Remove unusedReactimports in context mocks.Each context mock includes
const React = require("react");but React is never used within those mock implementations since the providers simply pass through children.jest.mock("@contexts/AuthContext", () => { - const React = require("react"); return { useAuth: () => mockAuthState, AuthProvider: ({ children }: { children: React.ReactNode }) => children, }; }); // Mock other context providers jest.mock("@contexts/NetworkContext", () => { - const React = require("react"); return { useNetwork: () => ({ isConnected: true, isInternetReachable: true }), NetworkProvider: ({ children }: { children: React.ReactNode }) => children, }; }); jest.mock("@contexts/DeveloperContext", () => { - const React = require("react"); return { useDeveloper: () => ({ isDeveloperMode: false }), DeveloperProvider: ({ children }: { children: React.ReactNode }) => children, }; }); jest.mock("@contexts/UnitContext", () => { - const React = require("react"); return { useUnits: () => ({ unitSystem: "imperial", setUnitSystem: jest.fn() }), UnitProvider: ({ children }: { children: React.ReactNode }) => children, }; }); jest.mock("@contexts/CalculatorsContext", () => { - const React = require("react"); return { useCalculators: () => ({ state: {}, dispatch: jest.fn() }), CalculatorsProvider: ({ children }: { children: React.ReactNode }) => children, }; });src/services/beerxml/BeerXMLService.ts (1)
462-498: Consider returning warnings on conversion failure.When conversion fails, the catch block returns
warnings: [], losing any warnings that may have been generated before the failure. If there's valuable diagnostic info from a partial conversion, consider preserving it.The current behavior is acceptable as a graceful fallback. If partial warnings are needed, consider tracking them separately before the API call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(auth)/resetPassword.tsx(2 hunks)app/(modals)/(beerxml)/importBeerXML.tsx(9 hunks)app/(modals)/(beerxml)/importReview.tsx(11 hunks)package.json(3 hunks)src/components/beerxml/UnitConversionChoiceModal.tsx(1 hunks)src/services/beerxml/BeerXMLService.ts(9 hunks)src/services/offlineV2/StartupHydrationService.ts(4 hunks)src/services/offlineV2/UserCacheService.ts(68 hunks)tests/app/(modals)/(beerxml)/importReview.test.tsx(11 hunks)tests/src/contexts/UnitContext.test.tsx(14 hunks)tests/src/services/NotificationService.test.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- app/(auth)/resetPassword.tsx
- tests/src/services/NotificationService.test.ts
- tests/src/contexts/UnitContext.test.tsx
- app.json
- src/components/beerxml/UnitConversionChoiceModal.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
src/services/beerxml/BeerXMLService.ts (2)
src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/common.ts (1)
UnitSystem(27-27)
tests/app/(modals)/(beerxml)/importReview.test.tsx (3)
tests/testUtils.tsx (2)
testUtils(293-369)renderWithProviders(31-86)app/(modals)/(beerxml)/importReview.tsx (1)
ImportReviewScreen(160-765)src/constants/testIDs.ts (1)
TEST_IDS(145-280)
app/(modals)/(beerxml)/importReview.tsx (6)
src/types/common.ts (2)
UnitSystem(27-27)TemperatureUnit(29-29)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/recipe.ts (3)
RecipeIngredient(44-77)RecipeMetricsInput(165-173)Recipe(95-134)src/utils/keyUtils.ts (1)
generateUniqueId(95-114)src/services/offlineV2/UserCacheService.ts (1)
createRecipe(241-325)src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(16-373)
app/(modals)/(beerxml)/importBeerXML.tsx (5)
src/services/beerxml/BeerXMLService.ts (2)
BeerXMLRecipe(13-16)parseBeerXML(170-203)src/types/common.ts (1)
UnitSystem(27-27)src/contexts/UnitContext.tsx (1)
useUnits(120-126)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/components/beerxml/UnitConversionChoiceModal.tsx (1)
UnitConversionChoiceModal(79-341)
src/services/offlineV2/UserCacheService.ts (4)
src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)src/types/common.ts (1)
UnitSystem(27-27)src/utils/recipeUtils.ts (1)
isTempId(17-19)
🔇 Additional comments (33)
src/services/offlineV2/StartupHydrationService.ts (3)
10-11: LGTM!Import updates are clean -
UnifiedLoggeras named export andUnitSystemtype import align with the codebase patterns shown in the relevant snippets.
33-54: Good use of Promise.allSettled for resilient parallel hydration.The pattern correctly allows both hydration tasks to complete independently and only marks
hasHydratedas true when both succeed. The detailed logging of partial failures with per-task status will aid debugging.
101-138: LGTM!The static data hydration follows a sensible pattern: check cache status, fetch if missing, or update in background if cached. Background update failures are properly caught and logged without disrupting the main flow.
src/services/offlineV2/UserCacheService.ts (5)
17-30: LGTM!Import updates correctly bring in
UnifiedLoggeras a named export and addUnitSystemto the type imports from@src/types, aligning with the type definition incommon.ts.
3531-3552: Good refactoring - extracted temp recipe_id resolution.The
resolveTempRecipeIdhelper is now properly integrated into the CREATE path, addressing the DRY concern from past reviews. The destructured result is correctly used in the API call and logging.
3664-3704: Good refactoring - consistent temp recipe_id resolution in UPDATE path.The UPDATE path now uses the same
resolveTempRecipeIdhelper as CREATE, eliminating the code duplication flagged in past reviews.
4040-4088: LGTM!The improved logging with separate counters for entity IDs, recipe references, and parent IDs provides excellent visibility into what the ID mapping operation updated. This will significantly aid debugging sync issues.
2696-2710: LGTM!The cache corruption recovery pattern (reset and return empty on parse error) is appropriate for this offline-first architecture. The UnifiedLogger integration maintains consistent logging.
tests/app/(modals)/(beerxml)/importReview.test.tsx (5)
6-6: Good migration torenderWithProviders.Using the centralized
renderWithProvidersutility ensures consistent provider wrapping across tests and aligns with the test utilities pattern.
10-15: LGTM: Appearance mock setup.Mocking
Appearancefor color scheme behavior is appropriate for ensuring consistent test behavior.
112-125: Good test setup with state reset.Using
testUtils.resetCounters()andsetMockAuthStateinbeforeEachensures clean test isolation between runs.
258-300: Ingredient test data properly structured.The updated test data includes all required fields (
ingredient_id,name,type,amount,unit) needed bynormalizeImportedIngredients, ensuring tests accurately reflect the validation logic.
384-404: No action required. TheuseQuerymock insetupTests.jsis created withjest.fn(), which automatically provides all Jest mock function methods includinggetMockImplementation(),mockImplementation(), andmockReturnValue(). The test code is correct and will not fail due to missing mock methods.Likely an incorrect or invalid review comment.
src/services/beerxml/BeerXMLService.ts (3)
3-4: LGTM: Clean imports for new dependencies.Importing
UnitSystemfrom types andUnifiedLoggerfor centralized logging aligns with the codebase patterns.
13-16: Good public API expansion.Exporting
BeerXMLRecipeinterface allows consumers (like the import flow) to properly type recipe data throughout the workflow.
404-448: Unit detection logic is well-structured.The
detectRecipeUnitSystemmethod properly handles:
- Batch size unit detection with trimming
- Ingredient unit aggregation
- "mixed" return for ambiguous cases
app/(modals)/(beerxml)/importBeerXML.tsx (4)
28-37: Good imports for unit conversion workflow.The new imports properly support the unit conversion flow with
useUnitsfor user preference,BeerXMLRecipetype,UnitConversionChoiceModalcomponent, andUnifiedLoggerfor centralized logging.
39-50: State interface properly extended.Adding
unit_choicestep andconvertingTargetstate enables proper tracking of the conversion workflow.
150-209: Unit conversion handler is well-implemented.The
handleUnitSystemChoicefunction properly:
- Sets loading state with
convertingTarget- Calls
BeerXMLService.convertRecipeUnitswith normalization- Logs warnings from conversion
- Handles errors gracefully with user feedback
- Resets state appropriately on both success and failure
One consistency note: add
voidprefix to the UnifiedLogger calls at lines 174 and 188:- UnifiedLogger.warn( + void UnifiedLogger.warn( "beerxml", "🍺 BeerXML Conversion warnings:", warnings ); - UnifiedLogger.error( + void UnifiedLogger.error( "beerxml", "🍺 BeerXML Import - Conversion error:", error );
480-495: Modal integration looks correct.The
UnitConversionChoiceModalis properly integrated with:
- Visibility tied to
unit_choicestep- User preference passed via
unitSystem- Loading state via
convertingTarget- Appropriate callbacks for metric/imperial selection and cancel
app/(modals)/(beerxml)/importReview.tsx (9)
44-81: Well-designed helper functions for unit system derivation.The
deriveUnitSystemandderiveMashTempUnitfunctions properly:
- Handle explicit values with validation
- Normalize input (uppercase for temp units)
- Log warnings for invalid values via UnifiedLogger
- Provide sensible defaults based on unit system
This addresses the past review comments about operator precedence and fragile inference.
100-158: Robust ingredient normalization with validation.The
normalizeImportedIngredientsfunction properly:
- Validates required fields (
ingredient_id,name,type,unit,amount)- Logs errors for invalid ingredients via UnifiedLogger
- Filters out invalid entries
- Generates unique
instance_idfor each valid ingredient- Uses
coerceIngredientTimefor time valuesThis addresses the past review comment about preview vs. persisted ingredients diverging.
184-199: Good memoization of normalized ingredients and filtered count.Using
useMemofornormalizedIngredientsandfilteredOutCountensures consistent data across preview, metrics, and creation while avoiding unnecessary recomputation.
201-285: Solid offline-first metrics calculation implementation.The metrics calculation:
- Uses comprehensive query key including ingredient fingerprint
- Validates data before calculation
- Logs validation failures as warnings (not errors)
- Throws internal errors to set
metricsErrorstate- Uses
staleTime: Infinityandretry: falseappropriately for deterministic local calculations
290-331: Recipe creation properly uses normalized ingredients.The mutation correctly:
- Derives unit system using centralized logic
- Uses
normalizedIngredients(same data as preview)- Includes calculated metrics if available
- Uses offline V2
createRecipefor immediate display
336-346: Comprehensive cache invalidation after recipe creation.Invalidating
RECIPES,USER_RECIPES, offline recipes, andDASHBOARDqueries ensures all views refresh properly. Priming the detail cache withsetQueryDataenables immediate navigation to the new recipe.
510-519: Good UX: Display filtered ingredient count.Showing the count of invalid/filtered ingredients with a warning color helps users understand what will actually be imported, addressing the "what you see is what you get" concern from past reviews.
666-681: Correct null checks for IBU and SRM metrics.Using
!= nullinstead of truthy checks properly handles the case where IBU or SRM is0, which is a valid calculated value.
708-721: Ingredient rendering uses normalized data consistently.Using
normalizedIngredientsfor the ingredient list ensures the UI displays exactly what will be saved. The time display logic is now safe sincecoerceIngredientTimehas already been applied during normalization.android/app/src/main/res/values/strings.xml (1)
4-4: Version update is consistent with coordinated bump across app.The expo_runtime_version string is correctly updated to align with the version bump across build.gradle, package.json, and app.json.
android/app/build.gradle (1)
95-96: Version bump is consistent and properly coordinated.The versionCode and versionName updates are aligned with changes in strings.xml and package.json; no build logic is altered.
package.json (2)
1-119: Implementation files not provided for review.The AI summary indicates this PR adds BeerXML unit-detection, UnitConversionChoiceModal, TemperatureUnit/UnitSystem types, UnifiedLogger integration, and offline metrics migration. However, none of these implementation files are present in this review set—only configuration and version-bump files (strings.xml, build.gradle, package.json) are shown. Ensure that the actual feature implementation files (TypeScript/Java modules for unit conversion, logging, and BeerXML import) are reviewed separately for correctness, test coverage, and alignment with the stated PR objectives.
4-4: Expo 54 is compatible with React 19.1.0 and React Native 0.81.5, but verify platform-specific behavior before merge.Expo SDK 54 is built to use React 19.1.0 with React Native 0.81.x, so the version alignment across expo ~54.0.25, expo-router ~6.0.17, and jest-expo ~54.0.14 is correct. However, there are known issues reported with this combination (e.g., web-specific problems including React.createContext errors and runtime hangs). Test the app's web and native platforms to confirm the known issues don't affect your use case, or verify that patch updates of SDK 54 have addressed them.
…rics - Change default unit system fully to metric throughout app - Add void to async logging calls where appropriate to signal fire-and-forget nature of these
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/src/services/offlineV2/StartupHydrationService.test.ts (1)
470-476: Test comment contradicts expected value.Line 470 states the test "should default to imperial", but the assertion on line 475 expects
"metric". Update the comment to match the new behavior.- // Call without unit system (should default to imperial) + // Call without unit system (should default to metric) await StartupHydrationService.hydrateOnStartup(mockUserId);
🧹 Nitpick comments (5)
src/services/offlineV2/LegacyMigrationService.ts (2)
38-134: Consider migrating to UnifiedLogger.This file still uses
console.logextensively (lines 38, 42, 47, 58, 67, 91, 107, 113, 117, 127). According to the PR's AI summary, the project is standardizing onUnifiedLoggerfor logging. Consider updating these logging statements to align with the project-wide logging strategy.
196-199: Consider using a unit conversion utility.The hardcoded temperature conversions (67°C ≈ 152°F) work as defaults, but using a centralized conversion utility would improve maintainability and consistency, especially since the PR is focused on integrating unit conversion.
If a conversion utility exists or is being added as part of this PR, consider refactoring:
- mash_temperature: userUnitSystem === "metric" ? 67 : 152, // 67°C ≈ 152°F + mash_temperature: userUnitSystem === "metric" ? 67 : convertCelsiusToFahrenheit(67),app/(modals)/(beerxml)/importReview.tsx (1)
100-174: Normalization logic correctly validates and filters ingredients.This addresses the past review concern about preview diverging from persisted data - both now use the same
normalizedIngredientsarray.One consideration: if a BeerXML file has many invalid ingredients,
UnifiedLogger.errorwill be called for each one. Consider batching these into a single warning with a count, or usingwarnlevel since these are data quality issues rather than system errors.- void UnifiedLogger.error( + void UnifiedLogger.warn( "import-review", "Ingredient missing ingredient_id", ing );src/services/offlineV2/UserCacheService.ts (2)
3222-3223: Remove empty debug blocks.Empty
__DEV__blocks at lines 3222-3223, 4283-4284, and 4403-4404 serve no purpose and should be removed to reduce clutter.Apply this diff to remove the empty block at line 3222-3223:
- // Debug logging for sanitized result - if (__DEV__) { - } - return sanitized;Similarly remove the empty blocks at lines 4283-4284 and 4403-4404.
Also applies to: 4283-4284, 4403-4404
3594-3600: Add logging for recipe UPDATE→CREATE conversion.The brew session UPDATE→CREATE conversion at lines 3691-3707 includes detailed logging, but the recipe conversion here lacks equivalent logging. This inconsistency makes debugging more difficult.
Apply this diff to add consistent logging:
if (isTempId) { // Convert UPDATE with temp ID to CREATE operation - + await UnifiedLogger.info( + "UserCacheService.processPendingOperation", + `Converting UPDATE with temp ID ${operation.entityId} to CREATE operation for recipe`, + { + entityId: operation.entityId, + recipeName: operation.data?.name || "Unknown", + } + ); const response = await ApiService.recipes.create(operation.data);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importBeerXML.tsx(9 hunks)app/(modals)/(beerxml)/importReview.tsx(11 hunks)package.json(3 hunks)src/services/beerxml/BeerXMLService.ts(9 hunks)src/services/offlineV2/LegacyMigrationService.ts(3 hunks)src/services/offlineV2/StartupHydrationService.ts(4 hunks)src/services/offlineV2/UserCacheService.ts(68 hunks)tests/app/(modals)/(beerxml)/importReview.test.tsx(11 hunks)tests/src/services/offlineV2/StartupHydrationService.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/res/values/strings.xml
- app.json
- app/(modals)/(beerxml)/importBeerXML.tsx
🧰 Additional context used
🧬 Code graph analysis (6)
tests/app/(modals)/(beerxml)/importReview.test.tsx (1)
tests/testUtils.tsx (2)
testUtils(293-369)renderWithProviders(31-86)
src/services/offlineV2/LegacyMigrationService.ts (1)
src/types/common.ts (1)
UnitSystem(27-27)
src/services/beerxml/BeerXMLService.ts (3)
src/types/recipe.ts (1)
Recipe(95-134)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/common.ts (1)
UnitSystem(27-27)
src/services/offlineV2/StartupHydrationService.ts (2)
src/types/common.ts (1)
UnitSystem(27-27)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)
app/(modals)/(beerxml)/importReview.tsx (4)
src/types/common.ts (2)
UnitSystem(27-27)TemperatureUnit(29-29)src/services/logger/UnifiedLogger.ts (1)
UnifiedLogger(11-428)src/types/recipe.ts (3)
RecipeIngredient(44-77)RecipeMetricsInput(165-173)Recipe(95-134)src/services/brewing/OfflineMetricsCalculator.ts (1)
OfflineMetricsCalculator(16-373)
src/services/offlineV2/UserCacheService.ts (8)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)src/types/common.ts (1)
UnitSystem(27-27)src/types/recipe.ts (1)
Recipe(95-134)src/types/brewSession.ts (1)
BrewSession(45-112)src/types/offlineV2.ts (2)
PendingOperation(175-180)OfflineError(267-276)src/utils/recipeUtils.ts (1)
isTempId(17-19)
🔇 Additional comments (20)
src/services/offlineV2/LegacyMigrationService.ts (3)
10-10: LGTM! Good type centralization.Importing
UnitSystemfrom a centralized location improves maintainability and ensures consistency across the codebase.
166-170: LGTM! Consistent type usage.The parameter type change to
UnitSystemis consistent with the broader refactoring and improves type safety.
26-29: No action required. All callers ofmigrateLegacyRecipesToV2explicitly pass theuserUnitSystemparameter (test cases pass "imperial" or "metric", production code passes aunitSystemvariable). The default parameter value is never used in practice, so changing it from "imperial" to "metric" has no actual impact on runtime behavior.android/app/build.gradle (1)
95-96: Version numbers properly synchronized.Lines 95-96 correctly reflect the version bump to 3.3.13 and versionCode 205. Ensure native Android builds complete successfully after this upgrade, particularly given the Android API level changes introduced in Expo SDK 54.
package.json (2)
4-4: No action required. Version consistency is already maintained across configuration files:app.jsonspecifies both"version"and"runtimeVersion"as"3.3.13",android/app/build.gradlehasversionName "3.3.13", andeas.jsondoes not track application versions (only CLI requirements). All version references are synchronized.Likely an incorrect or invalid review comment.
4-4: No action required — Expo SDK 54 configuration is already properly set up.The project is fully configured for Expo SDK 54 with React 19.1.0 and React Native 0.81.5. Verification confirms:
- New Architecture is enabled (
newArchEnabled: truein app.json)- Edge-to-edge is explicitly configured (
edgeToEdgeEnabled: true)- SafeAreaView is correctly imported from
react-native-safe-area-context(not from react-native)- expo-file-system uses the standard API (~19.0.14), not the deprecated /next API
- JSC is already handled via the community-maintained
jsc-androidpackage in build.gradle- Reanimated v4 is installed and compatible with the enabled New Architecture
- package-lock.json exists and is current
No deprecated APIs are in use, and all breaking changes from SDK 54 are already addressed.
tests/app/(modals)/(beerxml)/importReview.test.tsx (2)
68-104: Context mocks may conflict withrenderWithProviderswrapper.These manual context mocks (
useAuth,useNetwork, etc.) override the hooks, butrenderWithProvidersfromtestUtils.tsxalso wraps the component with actual providers. This works because the mocks take precedence, but it's worth noting:
- The mocks bypass the provider logic entirely
- Changes to provider behavior won't be tested
This is acceptable for unit tests focused on the component's rendering logic, but consider integration tests without mocks for end-to-end flows.
253-294: Test data properly structured for validation.The expanded ingredient objects now include required fields (
ingredient_id,type,amount,unit) that align withnormalizeImportedIngredientsvalidation. This ensures tests accurately reflect real-world import scenarios.src/services/beerxml/BeerXMLService.ts (3)
403-448: Unit detection logic is sound.The
detectRecipeUnitSystemmethod correctly:
- Trims and lowercases unit strings before comparison
- Counts both batch_size_unit and ingredient units
- Handles "mixed" case when counts are equal or both present
- Uses comprehensive unit name aliases (liters, litres, gallons, etc.)
506-508: Singleton export is appropriate.The singleton pattern ensures consistent state across the app for BeerXML operations.
462-503: Graceful degradation on conversion failure is good.The method correctly returns the original recipe if the backend conversion fails, ensuring the import flow isn't blocked. The
voidprefix on UnifiedLogger calls is now consistent with the rest of the file.The
ApiService.beerxml.convertRecipeendpoint has appropriate timeout configuration (15 seconds default), preventing indefinite waits if the API is unresponsive.src/services/offlineV2/StartupHydrationService.ts (2)
33-54: Partial failure handling is well-designed.Using
Promise.allSettledallows both hydration tasks to complete independently, andhasHydratedis only set when both succeed. This ensures:
- A failure in user data doesn't prevent static data hydration
- Partial failures trigger a retry on next startup
- Detailed per-task status is logged for debugging
89-93: Consistent UnifiedLogger usage throughout.All logging now uses
UnifiedLoggerwith appropriate log levels (warnfor recoverable issues,errorfor failures) and thevoidprefix for fire-and-forget semantics.app/(modals)/(beerxml)/importReview.tsx (4)
44-81: Well-designed centralized unit derivation functions.These functions address the past review concerns about fragile unit inference:
deriveUnitSystemrespects explicit values first, then falls back to batch_size_unit inferencederiveMashTempUnitnormalizes input, validates against "C"/"F", and logs invalid values before falling backThe operator precedence bug from the previous review is resolved.
225-301: Offline metrics calculation is well-implemented.Key design decisions:
staleTime: Infinityandretry: falseare correct for deterministic local calculations- Validation failures return
null(graceful, no error state) while internal errors throw (shows error UI)- Query key includes ingredient fingerprint for proper cache invalidation
730-736: Time display logic correctly handles edge cases.The condition
ingredient.time !== undefined && ingredient.time > 0properly:
- Excludes undefined/null times
- Excludes explicit zero times (0 min would be meaningless)
- Shows time only when meaningful
This addresses the past review concern about time coercion ordering.
682-697: Correct null checks for IBU/SRM metrics.Using
!= nullinstead of truthy checks allows valid zero values (e.g., 0 IBU for an unhopped beer) to display properly.src/services/offlineV2/UserCacheService.ts (3)
17-17: LGTM! Clean logger and type imports.The migration to
UnifiedLoggerand addition of theUnitSystemtype import properly support the logging consolidation and unit-system-aware API changes throughout the file.Also applies to: 29-29
3228-3352: LGTM! Well-implemented temp ID resolution.The
resolveTempRecipeIdhelper successfully addresses the duplicate code issue from past reviews and properly validatesrecipe_idbefore type assertions (addressing the type safety concern from previous comments). The method includes:
- Proper validation that
recipe_idexists and is a non-empty string before processing (lines 3254-3273)- Safe type assertion after validation (lines 3280-3282)
- Comprehensive error handling with appropriate
DEPENDENCY_ERRORcodes- Consistent structured logging throughout
4059-4107: LGTM! Improved ID mapping observability.The enhanced tracking with separate counters for
updatedEntityIds,updatedBrewSessionRecipeRefs, andupdatedParentIdsprovides much better visibility into which types of temp→real ID mappings are occurring during sync operations. The detailed logging will significantly aid debugging.
…d in beerxml ImportReview - Fully convert LegacyMigrationService to UnifiedLogger calls instead of direct console logs. Should evaluate whether this service is still necessary anymore - Remove empty __DEV__ blocks in UserCacheService (previously were debug logging but no longer necessary - Minor test comment text fix
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/offlineV2/LegacyMigrationService.ts (1)
199-199: Critical: batch_size_unit and unit_system are overwritten instead of preserved during migration.The migration code (line 33-34) uses the
userUnitSystemparameter to assignbatch_size_unitandunit_system, overwriting the legacy recipe's original values. Meanwhile,batch_sizeis preserved as-is (line 15), creating a unit mismatch.Example of data corruption:
- Legacy recipe:
batch_size: 19,batch_size_unit: "l",unit_system: "metric"- User preference is now
"imperial"- Migration produces:
batch_size: 19,batch_size_unit: "gal",unit_system: "imperial"- Result: 19 liters is silently misinterpreted as 19 gallons (~3.8x too large)
The legacy recipe already has explicit unit context stored. Either preserve both
batch_sizeand its originalunit_system/batch_size_unit, or convert thebatch_sizevalue if applying a different unit system. Currently, changing only the label without converting the value corrupts the data.Tests only validate scenarios where legacy and user unit systems match, missing this bug.
🧹 Nitpick comments (3)
src/services/offlineV2/UserCacheService.ts (1)
2682-2696: Minor inconsistency in error logging pattern.The error at line 2691 passes
edirectly as the third argument, while other error handlers wrap it in an object like{ error: e instanceof Error ? e.message : "Unknown error" }. This works but is slightly inconsistent with the pattern used elsewhere in the file.Consider standardizing:
} catch (e) { - UnifiedLogger.warn( - "offline-cache", - "Corrupt USER_RECIPES cache; resetting", - e - ); + UnifiedLogger.warn( + "offline-cache", + "Corrupt USER_RECIPES cache; resetting", + { error: e instanceof Error ? e.message : "Unknown error" } + );app/(modals)/(beerxml)/importReview.tsx (2)
48-57: Consider logging warnings for unexpected unit system values.The function silently falls back to
batch_size_unit-based logic whenexplicitUnitSystemis defined but invalid (e.g.,"foo"). While this fallback is reasonable, logging a warning would help surface data quality issues during import, similar to howderiveMashTempUnitlogs invalid inputs.Apply this diff to add validation logging:
function deriveUnitSystem( batchSizeUnit: string | undefined, explicitUnitSystem?: string ): UnitSystem { if (explicitUnitSystem === "metric" || explicitUnitSystem === "imperial") { return explicitUnitSystem; } + if (explicitUnitSystem !== undefined) { + void UnifiedLogger.warn( + "import-review", + `Invalid unit_system "${explicitUnitSystem}", deriving from batch_size_unit` + ); + } // Default based on batch size unit return batchSizeUnit?.toLowerCase() === "l" ? "metric" : "imperial"; }
225-242: Refactor queryKey to avoid expensive JSON.stringify.Serializing the entire ingredient array with
JSON.stringifyon every render is expensive and fragile (sensitive to key order, whitespace). Consider using a simpler fingerprint like a hash or concatenated IDs.Apply this diff to use a simpler fingerprint:
queryKey: [ "recipeMetrics", "beerxml-import-offline", recipeData?.batch_size, recipeData?.batch_size_unit, recipeData?.efficiency, recipeData?.boil_time, recipeData?.mash_temperature, recipeData?.mash_temp_unit, - // Include full ingredient fingerprint (ID + amount + unit) for proper cache invalidation - JSON.stringify( - recipeData?.ingredients?.map((i: any) => ({ - id: i.ingredient_id, - amount: i.amount, - unit: i.unit, - })) - ), + // Include ingredient fingerprint for cache invalidation + normalizedIngredients.length, + normalizedIngredients.map(i => `${i.ingredient_id}:${i.amount}:${i.unit}`).join("|"), ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/(modals)/(beerxml)/importReview.tsx(11 hunks)src/services/offlineV2/LegacyMigrationService.ts(11 hunks)src/services/offlineV2/UserCacheService.ts(68 hunks)tests/src/services/offlineV2/StartupHydrationService.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/src/services/offlineV2/StartupHydrationService.test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/offlineV2/LegacyMigrationService.ts (2)
src/types/common.ts (1)
UnitSystem(27-27)src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)
🔇 Additional comments (16)
src/services/offlineV2/UserCacheService.ts (8)
17-30: LGTM on import updates.The named export pattern for
UnifiedLoggerand the addition ofUnitSystemtype import are appropriate changes that improve type safety and logging consistency.
194-197: Default unit system is now consistent.All methods (
getRecipes,getBrewSessions,refreshRecipesFromServer,hydrateRecipesFromServer,hydrateBrewSessionsFromServer,refreshBrewSessionsFromServer) now consistently default to"metric". This addresses the previously flagged inconsistency.
3215-3339: Well-implemented helper addressing previous review feedback.The
resolveTempRecipeIdmethod properly:
- Validates
recipe_idexists and is a non-empty string before processing (lines 3241-3260)- Extracts the duplicate resolution logic from CREATE/UPDATE paths
- Returns a properly typed result with
recipe_id: stringguarantee- Uses appropriate error codes (
DEPENDENCY_ERROR) with correct retryable flagsThis addresses both the DRY concern and the type safety issue flagged in previous reviews.
3540-3561: LGTM on CREATE path integration.The
resolveTempRecipeIdhelper is correctly integrated into the brew session CREATE flow. The resolvedbrewSessionDatawith guaranteed validrecipe_idis properly passed to the API call.
3676-3760: LGTM on UPDATE path integration.The
resolveTempRecipeIdhelper is correctly integrated into the brew session UPDATE flow. Calling it before the temp ID check ensures thatrecipe_idis validated and resolved regardless of whether the session itself has a temp or real ID. The resolved data flows correctly through both the temp-to-create conversion and normal update paths.
4056-4104: Improved observability in ID mapping.The enhanced tracking variables (
updatedEntityIds,updatedBrewSessionRecipeRefs,updatedParentIds) provide better visibility into what's being updated during temp-to-real ID mapping. This will help with debugging sync issues.
3583-3598: LGTM on improved logging for UPDATE-to-CREATE conversion.The enhanced logging provides better context for debugging when a recipe UPDATE with temp ID gets converted to a CREATE operation.
2786-2800: LGTM on getCachedBrewSessions filtering and error handling.The user filtering and error handling with cache reset is correct. The awaited error logging ensures proper async handling in the catch block.
app/(modals)/(beerxml)/importReview.tsx (2)
303-399: LGTM! Recipe creation flow is well-structured.The mutation properly:
- Uses offline-first creation with
useRecipes- Shares
normalizedIngredientswith preview and metrics (ensuring WYSIWYG)- Invalidates appropriate query caches on success
- Primes the detail cache for immediate navigation
- Handles errors with unified logging
528-743: LGTM! UI correctly uses normalized ingredients.The UI properly:
- Displays
normalizedIngredientsinstead of rawrecipeData.ingredients- Shows
filteredOutCountwith warning styling for transparency- Uses explicit
ingredient.time !== undefined && ingredient.time > 0check (past review issue is resolved)- Handles 0 values correctly with
!= nullchecks (lines 682, 690)This ensures "what you see is what you get" between preview and creation.
src/services/offlineV2/LegacyMigrationService.ts (6)
10-10: LGTM!The import additions for
UnitSystemandUnifiedLoggerare appropriate and necessary for the migration enhancements.Also applies to: 14-14
27-30: LGTM!The addition of the
userUnitSystemparameter with a default value of"metric"provides backward compatibility while enabling unit-system-aware migrations.
41-44: LGTM!The migration from console logging to
UnifiedLoggeris consistently implemented throughout the service with appropriate log levels and descriptive namespaces.Also applies to: 47-50, 61-64, 71-74, 96-99, 102-106, 114-118, 124-127, 129-133, 140-144, 175-179, 249-252, 280-283, 285-289
219-220: LGTM!The default mash temperature values (67°C ≈ 152°F) are correctly set based on the unit system and represent reasonable defaults for typical mash temperatures.
218-218: LGTM!Setting the
unit_systemfield ensures that migrated recipes explicitly track their unit system, which is essential for the new unit-aware architecture.
55-58: LGTM!The
userUnitSystemparameter is correctly propagated through the migration pipeline, ensuring consistent unit handling when checking for duplicates and converting recipes.Also applies to: 80-84
…served during migration in LegacyMigrationService - Standardise error logging pattern for UserCacheService - Log warnings for unexpected unit system values in beerxml importReview
jackmisner
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/services/offlineV2/LegacyMigrationService.ts (1)
187-231: LGTM: Unit-aware conversion logic is correct.The conversion properly applies unit system defaults to
batch_size_unit,unit_system,mash_temperature, andmash_temp_unit. The default mash temperatures (67°C / 152°F) are appropriate for typical brewing.Optional: Extract magic numbers to constants.
Consider extracting the default mash temperatures to named constants for improved maintainability:
const DEFAULT_MASH_TEMP_CELSIUS = 67; const DEFAULT_MASH_TEMP_FAHRENHEIT = 152;Then use them in the conversion logic:
mash_temperature: legacyRecipe.mash_temperature || - (userUnitSystem === "metric" ? 67 : 152), // Default mash temp values + (userUnitSystem === "metric" ? DEFAULT_MASH_TEMP_CELSIUS : DEFAULT_MASH_TEMP_FAHRENHEIT),app/(modals)/(beerxml)/importReview.tsx (3)
44-64: AlignderiveUnitSystemdoc comment with actual fallback behaviorThe doc says “fallback to metric”, but the implementation returns
"imperial"for anybatchSizeUnitthat is not"l"(includingundefined). This is a small doc/behavior mismatch that can confuse future maintainers.You can either:
- Update the comment to describe the current behavior (metric for
"l", imperial otherwise), or- Adjust the fallback logic if the intended default truly is metric.
-/** - * Derive unit system from batch_size_unit with fallback to metric - * Centralizes the "l" => metric logic used throughout import - */ +/** + * Derive unit system from batch_size_unit, treating "l" as metric + * and defaulting to imperial otherwise. Centralizes the "l" => metric logic + * used throughout import. + */
112-181: Tighten typing and canonicalization innormalizeImportedIngredientsThe normalization logic is solid and centralizes validation nicely, but a couple of small improvements would make it more robust:
- The function signature uses
any[]; since this is the only gateway intoRecipeIngredient, consider typing the input more strictly (e.g., a BeerXML‑specific DTO type) to catch shape issues earlier.- You rely on exact string matches for
ing.type("grain","hop","yeast","other") for both field mapping and later grouping. If upstream ever passes"Grain"/"Hops"/etc., type‑specific fields won’t be set and those ingredients won’t be grouped as expected.Normalizing
ing.typeonce here would harden this path:- .map( - (ing: any): RecipeIngredient => ({ + .map( + (ing: any): RecipeIngredient => { + const type = String(ing.type).toLowerCase(); + return { // No id - backend generates on creation - ingredient_id: ing.ingredient_id, - name: ing.name, - type: ing.type, + ingredient_id: ing.ingredient_id, + name: ing.name, + type, @@ - ...(ing.type === "grain" && { + ...(type === "grain" && { @@ - ...(ing.type === "hop" && { + ...(type === "hop" && { @@ - ...(ing.type === "yeast" && { + ...(type === "yeast" && { @@ - ...(ing.beerxml_data && { - beerxml_data: ing.beerxml_data, - }), - }) + ...(ing.beerxml_data && { + beerxml_data: ing.beerxml_data, + }), + }; + })
712-741: Use stable keys and normalized time when rendering ingredient rowsThe ingredient rendering looks good and now uses normalized times, but there are two small improvements you can make:
- You already generate an
instance_idfor each ingredient; using the array index askeyis unnecessary and can cause subtle UI issues on insert/remove.- Since
timeis already normalized, the condition is fine; just keep relying on that invariant.Switching to
instance_idfor keys is straightforward:- {ingredients.map((ingredient, index) => ( - <View key={index} style={styles.ingredientItem}> + {ingredients.map(ingredient => ( + <View + key={ingredient.instance_id} + style={styles.ingredientItem} + > @@ - <Text style={styles.ingredientDetails}> - {ingredient.amount || 0} {ingredient.unit || ""} + <Text style={styles.ingredientDetails}> + {ingredient.amount || 0} {ingredient.unit || ""} {ingredient.use && ` • ${ingredient.use}`} {ingredient.time !== undefined && ingredient.time > 0 ? ` • ${ingredient.time} min` : ""} </Text>src/services/offlineV2/UserCacheService.ts (1)
48-51: Consider standardizing await usage for UnifiedLogger calls.There's inconsistent handling of UnifiedLogger promises across the file:
- Some calls use
await(lines 226-233, 308-318)- Some use
void(lines 784, 905)- Some have no await/void (lines 48, 57, 115, 141, 322, 423)
For consistency and to avoid unhandled promise rejections, consider standardizing on one pattern:
- Use
awaitwhere logging errors should be caught in the current try-catch- Use
voidfor fire-and-forget logging where blocking isn't desiredExample standardization:
- UnifiedLogger.warn( - "offline-cache", + void UnifiedLogger.warn( + "offline-cache", `[UserCacheService.getRecipeById] User ID is required for security` );Also applies to: 115-118, 141-145, 322-323, 784-788
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/(modals)/(beerxml)/importReview.tsx(11 hunks)src/services/offlineV2/LegacyMigrationService.ts(12 hunks)src/services/offlineV2/UserCacheService.ts(68 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/services/offlineV2/LegacyMigrationService.ts (4)
src/types/common.ts (1)
UnitSystem(27-27)src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)
src/services/offlineV2/UserCacheService.ts (7)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)src/types/common.ts (1)
UnitSystem(27-27)src/types/brewSession.ts (1)
BrewSession(45-112)src/types/offlineV2.ts (2)
PendingOperation(175-180)OfflineError(267-276)src/utils/recipeUtils.ts (1)
isTempId(17-19)
🔇 Additional comments (10)
src/services/offlineV2/LegacyMigrationService.ts (3)
10-10: LGTM: Type-safe imports added.The addition of
UnitSystemandUnifiedLoggerimports properly supports the centralized type system and structured logging infrastructure.Also applies to: 14-14
27-30: LGTM: Type-safe method signature.The parameter type now uses the centralized
UnitSystemtype, improving type safety and consistency across the codebase. The metric default is appropriate for the migration flow.
41-44: LGTM: Centralized logging implementation.The replacement of console logging with
UnifiedLoggerprovides structured, centralized logging. The fire-and-forget pattern (usingvoid) is appropriate for non-blocking logging calls.Also applies to: 47-50, 61-64, 71-74, 96-99, 102-106, 114-118, 124-127, 129-133, 140-144, 175-179, 255-258, 286-289, 291-295
src/services/offlineV2/UserCacheService.ts (7)
17-17: LGTM! Import changes align with the codebase refactoring.The migration from default to named UnifiedLogger import and the addition of UnitSystem type import are correct and consistent with the PR objectives.
Also applies to: 29-29
194-196: Past review comment appears resolved - defaults are now consistent.The previous concern about inconsistent default unit systems seems to be addressed. All methods with
userUnitSystemparameters now consistently default to"metric"(lines 196, 766, 2972, 3131, 420), which provides uniform behavior across the service.
3215-3339: LGTM! Type safety concern from past review is now properly addressed.The
resolveTempRecipeIdmethod includes proper validation before assertingrecipe_idas a string:
- Lines 3241-3260: Validates that
recipe_idexists, is a non-empty string before processing- Throws a descriptive
OfflineErrorif validation fails- Comprehensive logging at each decision point
- Proper error handling for missing recipes and userId
This implementation addresses the previous type safety concern and provides a clean, reusable helper for resolving temporary recipe IDs in both CREATE and UPDATE paths.
3539-3573: LGTM! Duplicate code from past review successfully eliminated.The CREATE (lines 3540-3546) and UPDATE (lines 3680-3686) paths now both call the extracted
resolveTempRecipeIdhelper method, eliminating the ~64 lines of duplicate logic flagged in the previous review. The consistent usage pattern across both paths improves maintainability and reduces the risk of divergent behavior.Also applies to: 3676-3705
2682-2696: LGTM! Robust error handling for cache corruption.Both
getCachedRecipes(lines 2687-2695) andgetCachedBrewSessions(lines 2791-2799) implement proper defensive error handling:
- Catch JSON parsing errors from potentially corrupted cache
- Log warnings with clear context
- Reset corrupt cache to prevent repeated failures
- Return safe fallback (empty array) to keep app functional
This prevents cache corruption from cascading into application crashes.
Also applies to: 2786-2800
4056-4104: LGTM! Improved observability for ID mapping operations.The enhanced counter tracking (lines 4056-4087) provides detailed visibility into what gets updated during temp-to-real ID mapping:
- Separate counters for entityId, brew session recipe references, and parentId updates
- Conditional save only when updates occur
- Comprehensive logging with breakdown of update types
This greatly improves debugging capabilities for ID resolution issues.
4409-4565: LGTM! Well-designed tempId mapping cache with good security practices.The three tempId mapping methods provide robust navigation compatibility after sync:
- saveTempIdMapping (lines 4409-4450): Stores mappings with 24-hour TTL and proper atomicity via
withKeyQueue- getRealIdFromTempId (lines 4459-4519): Includes important security validation - verifies userId matches before returning real ID (line 4481), preventing cross-user data access
- cleanupExpiredTempIdMappings (lines 4525-4565): Prevents unbounded cache growth
The 24-hour TTL is a reasonable balance between navigation compatibility and cache size. All methods use non-throwing error handling since these are non-critical operations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/services/offlineV2/UserCacheService.ts (2)
194-197: Past review concern addressed - Unit system defaults are now consistent.Both
getRecipesandgetBrewSessionsnow default to"metric", resolving the inconsistency flagged in the previous review. All related methods (refreshRecipesFromServer,hydrateRecipesFromServer,hydrateBrewSessionsFromServer,refreshBrewSessionsFromServer) also consistently default to"metric".Also applies to: 776-779
3239-3363: Past review concerns addressed - Well-structured helper method.This new
resolveTempRecipeIdmethod addresses both past review concerns:
DRY violation resolved: The duplicate temp recipe_id resolution logic has been extracted into this reusable helper, called from both CREATE (line 3566) and UPDATE (line 3706) paths.
Type safety issue fixed: The method now validates that
recipe_idexists and is a non-empty string (lines 3265-3268) before any type assertions, preventing the unsafe cast ofundefinedtostring.The error handling appropriately distinguishes between validation failures (non-recoverable) and dependency issues (recoverable when recipe hasn't synced yet).
🧹 Nitpick comments (4)
app/(modals)/(beerxml)/importReview.tsx (4)
44-67: Unit derivation helpers correctly normalize and validate unitsThe new
deriveUnitSystem/deriveMashTempUnithelpers read cleanly, respect explicit values, and log when encountering invalid inputs before falling back to sensible defaults, which also addresses the earlier precedence bug in the mash temp logic.Longer‑term, consider pushing this unit-system derivation into the BeerXML service (and passing a typed
UnitSystem | undefineddown) so the UI stays free of parsing heuristics and only consumes canonical units from the parser.Also applies to: 69-91
96-108: Ingredient normalization is robust; consider slightly stricter amount coercion
coerceIngredientTimeplusnormalizeImportedIngredientsgive you a solid, centralized normalization path (including logging, ID generation, and type-specific fields) that’s now reused for metrics, preview, and creation.One small edge case: strings that are only whitespace (e.g.
" ") will currently pass theamountvalidation (Number(" ") === 0) and be treated as0. If that should instead be treated as “missing/invalid”, you could add a.trim() === ""check before numeric coercion.Also applies to: 110-185
236-250: Offline metrics query is deterministic and failure modes are well handledThe React Query setup for offline metrics looks solid: deterministic
queryKey,enabledguard,staleTime: Infinity, andretry: falseall match the “pure local calculation” model, and the split between validation failures (returningnull) and hard errors (logged + thrown) gives a clean UX.If you ever allow editing of ingredient properties that affect metrics (e.g. color, alpha_acid) in this screen, you may want to include those fields in the fingerprint portion of the
queryKeyso metrics recompute when those change, not just when id/amount/unit do.Also applies to: 252-263, 271-285, 287-317
672-714: Use null checks (not truthiness) for all metric display guards for consistencyYou’ve already switched IBU and SRM to
!= nullchecks so0still renders; OG/FG/ABV are still guarded with simple truthiness checks.For future robustness and consistency, consider using the same
value != nullpattern for OG/FG/ABV as well, even though 0 is unlikely for gravity and only marginally plausible for ABV.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/(modals)/(beerxml)/importReview.tsx(11 hunks)src/services/offlineV2/UserCacheService.ts(68 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/offlineV2/UserCacheService.ts (7)
src/services/logger/UnifiedLogger.ts (2)
UnifiedLogger(11-428)error(315-325)src/services/logger/Logger.ts (1)
error(239-245)src/services/logger/DevLogger.ts (1)
error(141-148)src/types/common.ts (1)
UnitSystem(27-27)src/types/recipe.ts (1)
Recipe(95-134)src/types/offlineV2.ts (2)
PendingOperation(175-180)OfflineError(267-276)src/utils/recipeUtils.ts (1)
isTempId(17-19)
🔇 Additional comments (7)
src/services/offlineV2/UserCacheService.ts (6)
17-30: LGTM - Import updates align with standardization goals.The switch to named export for
UnifiedLoggerand addition ofUnitSystemtype import supports the PR's goal of centralizing these types and standardizing logging.
3563-3596: LGTM - Clean integration of the extracted helper.The
resolveTempRecipeIdcall properly validates and resolves the recipe_id before the API call. The logging now includeshadTempRecipeIdfor better debugging visibility.
4084-4131: LGTM - Improved observability for ID mapping operations.The detailed breakdown of updated references (entity IDs, brew session recipe references, and parent IDs) provides valuable debugging information for sync-related issues.
48-60: LGTM - Consistent logging pattern.The use of
void UnifiedLogger.*for fire-and-forget logging in non-critical paths versusawait UnifiedLogger.*in main operation flows provides a good balance between observability and performance.
2706-2720: LGTM - Appropriate cache recovery strategy.The cache corruption handling (resetting and returning empty array) is a reasonable fallback that allows the app to continue functioning while re-hydrating from server.
4426-4478: LGTM - Well-designed tempId mapping with security checks.The tempId mapping cache is a good addition for navigation compatibility. Key strengths:
- Security: Stores
userIdand verifies ownership before returning mappings- Memory management: 24-hour TTL prevents unbounded growth
- Graceful degradation: Non-critical errors are logged but don't throw
- Storage key properly defined:
STORAGE_KEYS_V2.TEMP_ID_MAPPINGSis correctly defined insrc/types/offlineV2.tsand consistently used across all three mapping methodsapp/(modals)/(beerxml)/importReview.tsx (1)
215-227: Nice “what you see is what you get” behavior via normalizedIngredientsDriving both the preview and the create payload from
normalizedIngredients, and surfacingfilteredOutCountin the summary, removes the earlier divergence where previewed ingredients could be dropped silently on save.Using
instance_idas the React key also avoids collisions on repeatingingredient_ids, which is important for this import flow.Also applies to: 324-359, 544-550, 725-756
…review, metrics, and create
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/(modals)/(beerxml)/importReview.tsx (2)
44-67: Unit system & mash temp helpers fix prior bugs and centralize logicThe new
deriveUnitSystem/deriveMashTempUnithelpers correctly:
- Prefer an explicit
unit_systemwhen valid, with a warning on invalid values.- Fall back to a deterministic batch-size–based unit system.
- Normalize
mash_temp_unitto"C"/"F"and default by unit system when invalid or missing.This addresses the earlier operator-precedence/unit-system issues and makes behavior much easier to reason about. If you expect additional imperial aliases from BeerXML (e.g.,
"gallon","gals"), you could extend thelowerCasedUnitchecks, but current logic is already sound.Also applies to: 69-91
228-250: Offline metrics integration and numeric normalization are solid; consider aligning batch-size display defaultThe offline metrics path looks robust:
- Query key includes all relevant numeric inputs and an ingredient fingerprint.
enabledgating +staleTime: Infinitymake sense for deterministic local calculation.batch_size,boil_time, mash temp/unit, and ingredients are normalized consistently with what’s used in the create payload.One minor UX nit: when
batch_sizeis missing or invalid, metrics default to19.0, but the “Batch Size” detail row shows"N/A". If you want true WYSIWYG for users, consider reusing this normalized batch size (or its default) in the preview as well so the displayed value matches what metrics are calculated with.Also applies to: 252-285, 287-317
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(11 hunks)package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/build.gradle
- app.json
- package.json
🔇 Additional comments (6)
android/app/src/main/res/values/strings.xml (1)
4-4: Version update aligns with coordinated version bump.The
expo_runtime_versionis correctly updated to3.3.14as part of the broader version bump (versionCode 191 → 206) mentioned in the PR summary. The resource format is valid.Please verify that all version strings across the project (
build.gradle,app.json,package.json, and manifest files) are consistently updated to3.3.14and versionCode206.app/(modals)/(beerxml)/importReview.tsx (5)
15-15: Imports and new dependencies look consistentThe added imports for shared types,
useRecipes,OfflineMetricsCalculator, andUnifiedLoggerare cohesive and align this screen with the new offline-first + centralized logging architecture. No issues spotted here.Also applies to: 29-35, 40-42
197-198: Switch to UnifiedLogger and offline create hook is appropriateUsing
UnifiedLogger.erroron JSON parse failure and the offline-v2useRecipes().createhook keeps logging and data flow consistent with the rest of the app. This is a clean replacement for ad‑hocconsole.*and direct API calls.Also applies to: 202-206
211-227: Using normalized ingredients for both preview and create nicely fixes WYSIWYG issuesNormalizing once via
normalizeImportedIngredientsand then:
- Driving the preview from
normalizedIngredients, and- Using the same array for metrics + the create payload,
ensures the user only sees ingredients that will actually be saved. The
filteredOutCountbanner is a good UX touch to surface discarded items without overwhelming the UI.Also applies to: 554-560, 735-739
708-723: IBU/SRM guards now correctly show legitimate zero valuesSwitching the checks to
calculatedMetrics.ibu != null/srm != nullensures 0 IBU or 0 SRM recipes still render their metrics instead of being treated as falsy. This is the right guard for numeric fields.
735-738: Ingredient list now uses normalized data and properly normalized timesRendering ingredient groups from
normalizedIngredientswithinstance_idkeys and:ingredient.time !== undefined && ingredient.time > 0 ? ` • ${ingredient.time} min` : ""means time comparisons now operate on the already-coerced numeric
time, addressing the earlier “string '0' vs. > 0” issue. This is clean and consistent with the normalization helper.Also applies to: 750-766
…normalization - Fix: Batch-size unit label in preview can diverge from what is actually saved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/(modals)/(beerxml)/importReview.tsx (1)
271-291: Normalize batch size/boil time once and reuse across metrics, creation, and preview.You now normalize
batch_size/boil_timeconsistently for metrics and creation, which is great. The preview still derives its batch-size display fromNumber(recipeData.batch_size), so edge cases like""or"0"can show0.0/N/Awhile metrics and the saved recipe use the 19.0 default.To keep “shown”, “calculated”, and “saved” values perfectly aligned and remove duplication, consider extracting a small helper (or
useMemo) that returnsunitSystem,normalizedBatchSize, andnormalizedBoilTime, and reuse it in:
- the metrics recipeFormData,
- the create payload, and
- the Batch Size/Boil Time preview rows.
This keeps the normalization rules in exactly one place.
- <View style={styles.detailRow}> - <Text style={styles.detailLabel}>Batch Size:</Text> - <Text style={styles.detailValue}> - {(() => { - const n = Number(recipeData.batch_size); - const displayValue = Number.isFinite(n) - ? n.toFixed(1) - : "N/A"; - - const unitSystem = deriveUnitSystem( - recipeData.batch_size_unit, - recipeData.unit_system - ); + <View style={styles.detailRow}> + <Text style={styles.detailLabel}>Batch Size:</Text> + <Text style={styles.detailValue}> + {(() => { + // Example: reuse normalized values computed once above + const displayValue = normalizedBatchSize.toFixed(1); + + const unitSystem = deriveUnitSystem( + recipeData.batch_size_unit, + recipeData.unit_system + ); const rawUnit = String( recipeData.batch_size_unit || "" ).toLowerCase(); @@ - return `${displayValue} ${unitLabel}`; + return `${displayValue} ${unitLabel}`; })()} </Text> </View>Also applies to: 338-347, 615-642
🧹 Nitpick comments (4)
app/(modals)/(beerxml)/importReview.tsx (4)
44-91: Unit derivation helpers look solid; consider case-normalizing explicit unit_system.Centralizing
deriveUnitSystemandderiveMashTempUnitwith logging for invalid inputs is a good improvement and fixes the earlier precedence bug. You might optionally normalizeexplicitUnitSystemto lowercase before comparison so values like"Metric"don’t get treated as invalid if the backend ever changes casing.
115-192: Ingredient normalization is much stricter now and suitable for reuse.The validation pipeline (required fields, missing/invalid/≤0 amounts) plus logging and the inclusion of
instance_idand BeerXML metadata makes this a good single source of truth for both metrics and creation. The only minor nit is thatamountgets parsed twice (once for validation, once in the mapper); if this ever becomes hot, you could reuse the validated numeric value in the mapped object.
733-748: Consider consistent null-checking across all calculated metric fields.IBU and SRM now use
!= nullchecks, which will still render0.0if that ever occurs. OG, FG, and ABV still rely on truthiness checks; for strict consistency, you could switch those to!= nullas well so a true zero value would render instead of disappearing.- {calculatedMetrics.og ? ( + {calculatedMetrics.og != null ? ( @@ - {calculatedMetrics.fg ? ( + {calculatedMetrics.fg != null ? ( @@ - {calculatedMetrics.abv ? ( + {calculatedMetrics.abv != null ? (
761-789: Ingredient list now correctly uses normalized data and stable keys.Driving the per-type sections from
normalizedIngredientswithinstance_idkeys and the newtimehandling (numeric/undefined only) fixes the earlier coercion issues and keeps preview aligned with what will be saved. Theingredient.amount || 0/ingredient.unit || ""fallbacks are now effectively redundant given the stricter normalization, so you could simplify those expressions if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/(modals)/(beerxml)/importReview.tsx(12 hunks)
🔇 Additional comments (5)
app/(modals)/(beerxml)/importReview.tsx (5)
203-224: JSON parse error handling is appropriate and uses UnifiedLogger.Parsing
params.recipeDataonce, capturing failures viaUnifiedLogger.error, and short‑circuiting to an “Invalid Recipe Data” screen is a clean pattern that avoids cascading null checks deeper in the component.
218-233: Good reuse of normalized ingredients and explicit surfaced discard count.Deriving
normalizedIngredientsonce and computingfilteredOutCountgives a nice WYSIWYG guarantee and clearly explains to users why ingredient counts might not match the raw BeerXML.
243-324: Offline metrics calculation is well-guarded and failure modes are distinct.Using validated, normalized inputs for metrics, returning
nullon validation failure (no error state), and reserving the error path for unexpected calculator failures (with logging) gives predictable behavior and clear UX messaging in the “Calculated Metrics” section.
348-381: Creation flow correctly mirrors metrics inputs and uses offline create.The mutation reuses the same normalization logic for batch size/boil time, respects the derived
unitSystemandmash_temp_unit, passesnormalizedIngredients, and then immediately primes the detail cache and list queries. Logging errors viaUnifiedLoggerwhile keeping the user-facing Alert simple is a good balance.Also applies to: 383-395, 425-425
561-567: Nice UX touch surfacing filtered-out ingredients to users.Showing
({filteredOutCount} invalid filtered out)alongside the ingredient count (with warning color) makes the normalization behavior user-visible instead of silently dropping data.
… beerxml flow
- Created Normalization Helper
- Memoized Normalized Values
- Updated All Three Use Cases:
- Metrics Calculation
- Recipe Creation
- Preview Display
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/(modals)/(beerxml)/importReview.tsx (4)
45-68: Unit derivation and normalization helpers look solid; consider hardening explicitunit_systemhandling.The centralization of
deriveUnitSystem,deriveMashTempUnit, andnormalizeRecipeValuesnicely aligns metrics, preview, and persistence — especially the consistent19.0/60defaults and canonicalization of"gal"vs"gallons". The only edge case I’d consider is makingexplicitUnitSystemcomparisons case‑insensitive before logging as invalid, so unexpected values like"IMPERIAL"don’t silently fall back tometric-derived behavior whenbatch_size_unitis missing.Also applies to: 70-92, 145-192
199-276: Ingredient normalization is robust; you can reuse parsed amount and double‑check domain intent for zero amounts.The validation pipeline here (required fields, numeric parsing,
Number.isFinite, andamountNumber <= 0rejection) is a good tightening over the previous behavior, and logging viaUnifiedLoggeris helpful. Since you already computeamountNumber, you could reuse it in the mapping step instead of callingNumber(ing.amount)again, and if the domain ever needs to support explicit zero‑amount ingredients (e.g., placeholders), you may want to relax the<= 0guard to< 0or gate it behind a clearly documented rule.
333-359: Offline metrics query wiring is consistent with normalization flow; query key could optionally use normalized values.The query’s
enabledguard, reuse ofnormalizedValues/normalizedIngredients, and separation between validation failures (returnnull) vs internal errors (log + throw) all look good and should give predictable UX. If you want cache keys to reflect the canonicalized inputs rather than raw BeerXML values (e.g.,"gallons"vs"gal", or""vs0for times), consider keying offnormalizedValuesfields instead of the rawrecipeData.*, but this is more of a cache hygiene improvement than a correctness issue.Also applies to: 362-376, 378-408
807-812: Ingredient rendering now leverages normalized data; minor time/amount display tweaks are optional.Driving the per‑type ingredient lists off
normalizedIngredientswithinstance_idkeys and usingcoerceIngredientTimeoutput for thetimechecks addresses the earlier fragility around rawtimevalues and preview/persistence divergence. If you ever need to show explicit zero‑minute additions, you could swap theingredient.time > 0condition for>= 0with a separate “missing” check, but current behavior (hide zero) is reasonable if that matches your UX intent.Also applies to: 823-839
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
android/app/build.gradle(1 hunks)android/app/src/main/res/values/strings.xml(1 hunks)app.json(3 hunks)app/(modals)/(beerxml)/importReview.tsx(12 hunks)package.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/build.gradle
🔇 Additional comments (6)
app/(modals)/(beerxml)/importReview.tsx (3)
302-329: Nice “what you see is what you get” alignment via normalized values and ingredient counts.Using
normalizedValuesandnormalizedIngredientsfor the recipe details, ingredient count, and filtered‑out count keeps the UI tightly aligned with what will actually be saved and used for metrics. The conditional warning onfilteredOutCountand the preformatteddisplayBatchSize/displayBoilTimestrings make the behavior explicit to users and should avoid the earlier preview vs payload divergences.Also applies to: 633-639, 688-699
413-448: Create mutation correctly reuses normalized data and offline V2 flow.The mutation guards on
normalizedValues, uses the same normalized values and ingredients as metrics/preview, and feeds through calculated metrics when present, which keeps creation behavior consistent. The switch touseRecipes’ offlinecreateplus targeted query invalidations and priming ofQUERY_KEYS.RECIPE(createdRecipe.id)should give a snappy UX with correct cache hydration.Also applies to: 450-452, 457-467, 472-472, 477-482, 497-501, 512-512
781-796: Good fix on IBU/SRM display conditions.Using
!= nullfor the IBU and SRM checks ensures you still render legitimate zero values instead of treating them as “missing,” aligning with how OG/FG/ABV are handled while still avoidingundefined/null.android/app/src/main/res/values/strings.xml (1)
4-4: Version update is consistent with app.json and package.json.This runtime version matches the expo.runtimeVersion and package version bumps across the PR.
app.json (1)
6-6: Verify the versionCode increment (191 → 207).The version field (3.3.15) and runtimeVersion (3.3.15) are consistent across manifests. However, the versionCode jump from 191 to 207 is unusually large for a minor feature release. Confirm this represents the intended build number progression (e.g., intermediate development builds) rather than an accidental merge conflict or duplication.
Also applies to: 19-19, 61-61
package.json (1)
4-4: Version matches app.json and strings.xml.The package version 3.3.15 is consistent across all manifests.
jackmisner
deleted the
integrate-recipe-unit-conversion-flowchart-from-backend
branch
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.