Skip to content

Conversation

@arii
Copy link
Owner

@arii arii commented Feb 10, 2026

Description

This PR addresses architectural feedback on the ZoneDistribution component and theme management.

Key Changes:

  1. Theme Architecture: Added custom.hrZones to the MUI theme in lib/theme.ts to centralize zone colors, removing hardcoded or repetitive logic in components.
  2. Code Simplification: Refactored ZoneDistribution.tsx to remove verbose comments and simplify the color resolution logic using ZONE_COLOR_MAP and the new theme property.
  3. Test Logic Isolation: Moved the __IS_TEST_ENV__ check to a shared helper isTestEnvironment() in lib/utils.ts to avoid embedding raw test environment checks in production components.
  4. UI Consistency: Fixed percentage formatting in ZoneDistribution to use fixed decimals (e.g., "60.0%") for consistent layout.
  5. Performance: Implemented lazy loading for ZoneDistribution using next/dynamic in SessionDetail and ExperimentalAnalyticsPage.
  6. Type Safety: Updated ZONE_PRIORITY in ZoneDistribution to include all enum members, satisfying strict TypeScript checks.

Verified with unit tests (tests/unit/app/client/experimental/components/ZoneDistribution.test.tsx) and frontend verification via Playwright.

Fixes #

Change Type: 🏗️ Refactoring (code change that neither fixes bug nor adds feature)

PR Scope Checklist

This checklist is mandatory for all PRs.

  • PR has a clear, single purpose: The title and description of the PR clearly state the purpose of the change.
  • All changes relate to the stated objective: The code changes should be directly related to the purpose of the PR.
  • No unrelated cleanup or refactoring: The PR should not contain any changes that are not directly related to the stated objective.
  • Title and description match the actual changes: The title and description should accurately reflect the changes in the PR.
  • Tests cover the specific change scope: The tests should be focused on the changes in the PR and should not include unrelated tests.

Impact Assessment

  • Changes are backward compatible (or breaking changes are documented)
  • Tests are added/updated for new functionality
  • Documentation is updated if needed
  • ADR is created/updated for significant architectural changes
Original PR Body

This PR addresses architectural feedback on the ZoneDistribution component and theme management.

Key Changes:

  1. Theme Architecture: Added custom.hrZones to the MUI theme in lib/theme.ts to centralize zone colors, removing hardcoded or repetitive logic in components.
  2. Code Simplification: Refactored ZoneDistribution.tsx to remove verbose comments and simplify the color resolution logic using ZONE_COLOR_MAP and the new theme property.
  3. Test Logic Isolation: Moved the __IS_TEST_ENV__ check to a shared helper isTestEnvironment() in lib/utils.ts to avoid embedding raw test environment checks in production components.
  4. UI Consistency: Fixed percentage formatting in ZoneDistribution to use fixed decimals (e.g., "60.0%") for consistent layout.
  5. Performance: Implemented lazy loading for ZoneDistribution using next/dynamic in SessionDetail and ExperimentalAnalyticsPage.
  6. Type Safety: Updated ZONE_PRIORITY in ZoneDistribution to include all enum members, satisfying strict TypeScript checks.

Verified with unit tests (tests/unit/app/client/experimental/components/ZoneDistribution.test.tsx) and frontend verification via Playwright.


PR created automatically by Jules for task 6693765866534748708 started by @arii

- Moved HR zone colors to `lib/theme.ts` under `palette.custom.hrZones`.
- Refactored `ZoneDistribution.tsx` to remove verbose comments and simplify color resolution.
- Centralized `isTestEnvironment` check in `lib/utils.ts`.
- Updated `ZoneDistribution.tsx` to use fixed decimal strings for percentages.
- Lazy loaded `ZoneDistribution` in `SessionDetail` and `ExperimentalAnalyticsPage`.
- Updated unit tests to match new formatting and stricter type checking.

Co-authored-by: arii <342438+arii@users.noreply.github.com>
@google-labs-jules
Copy link
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@github-actions github-actions bot changed the title Refactor ZoneDistribution and improve theme architecture feat(medium): Refactor ZoneDistribution and improve theme architecture Feb 10, 2026
@arii
Copy link
Owner Author

arii commented Feb 10, 2026

Reviewed commit: a3682910dedacbf21a065b31dc4a668f6fc3c3e6

This PR introduces several positive changes, primarily focused on refactoring the ZoneDistribution component and enhancing the application's theme architecture. The dynamic import of ZoneDistribution is a good step towards improving initial page load performance by deferring the loading of a client-side chart component.

Strengths

  • Performance Improvement: Dynamically importing ZoneDistribution with ssr: false optimizes client-side rendering and reduces the initial bundle size, aligning with best practices for interactive components.
  • Theme Centralization: The addition of custom.hrZones to lib/theme.ts centralizes HR zone color definitions. This improves consistency and maintainability, directly addressing the DESIGN_GUIDELINES.md requirement for a comprehensive MUI theme.
  • Code Reusability: Extracting the test environment check into a dedicated isTestEnvironment utility function in lib/utils.ts promotes the DRY principle and improves code clarity, as outlined in CODE_REVIEW_GUIDELINES.md.
  • Expanded HR Zones: The update to ZONE_PRIORITY in ZoneDistribution.tsx to include Aerobic, Recovery, and Idle indicates an expansion of the HR zone model, which is a positive functional enhancement.
  • Test Updates: The unit tests for ZoneDistribution have been appropriately updated to reflect the new percentage formatting, ensuring test accuracy and preventing regressions.

Issues and Suggestions

  • HR Zone Definition Inconsistency: There's a notable discrepancy between the DESIGN_GUIDELINES.md and the implementation regarding "Fat Burn" and "Warm Up" zones. The guidelines state "Success (Green #4CAF50): Matches Warm Up zone (formerly Fat Burn)" while also listing "Success (Green #4CAF50): Matches Fat Burn zone". The PR's theme and component logic now treat these as distinct zones with different colors. This needs to be resolved by updating the DESIGN_GUIDELINES.md to reflect the distinct nature of these zones and ensuring the HrZoneName enum (implicitly used) accurately represents all defined HR zones and their relationship to the theme. This is a high-priority technical debt item.
  • Missing Theme Colors for New Zones: The ZoneDistribution.tsx component now includes Aerobic, Recovery, and Idle in its ZONE_PRIORITY mapping. However, lib/theme.ts's custom.hrZones palette does not yet define specific colors for these new zones. They will currently fall back to theme.palette.grey[500], leading to inconsistent visuals. These new zones should be added to the theme for complete integration.
  • Percentage Formatting: The percentage value in ZoneDistribution.tsx is now calculated using toFixed(1) and stored as a string. While functional, it's generally better practice to store numerical values and format them only at the point of display. This allows for easier numerical operations if needed in the future and maintains data type consistency. Consider storing percentage as a number and formatting formattedPercentage as a string for display.

Reviewed at commit: a3682910dedacbf21a065b31dc4a668f6fc3c3e6

@google-labs-jules
Copy link
Contributor

Reviewed commit: a3682910dedacbf21a065b31dc4a668f6fc3c3e6

This PR introduces several positive changes, primarily focused on refactoring the ZoneDistribution component and enhancing the application's theme architecture. The dynamic import of ZoneDistribution is a good step towards improving initial page load performance by deferring the loading of a client-side chart component.

Strengths

  • Performance Improvement: Dynamically importing ZoneDistribution with ssr: false optimizes client-side rendering and reduces the initial bundle size, aligning with best practices for interactive components.
  • Theme Centralization: The addition of custom.hrZones to lib/theme.ts centralizes HR zone color definitions. This improves consistency and maintainability, directly addressing the DESIGN_GUIDELINES.md requirement for a comprehensive MUI theme.
  • Code Reusability: Extracting the test environment check into a dedicated isTestEnvironment utility function in lib/utils.ts promotes the DRY principle and improves code clarity, as outlined in CODE_REVIEW_GUIDELINES.md.
  • Expanded HR Zones: The update to ZONE_PRIORITY in ZoneDistribution.tsx to include Aerobic, Recovery, and Idle indicates an expansion of the HR zone model, which is a positive functional enhancement.
  • Test Updates: The unit tests for ZoneDistribution have been appropriately updated to reflect the new percentage formatting, ensuring test accuracy and preventing regressions.

Issues and Suggestions

  • HR Zone Definition Inconsistency: There's a notable discrepancy between the DESIGN_GUIDELINES.md and the implementation regarding "Fat Burn" and "Warm Up" zones. The guidelines state "Success (Green #4CAF50): Matches Warm Up zone (formerly Fat Burn)" while also listing "Success (Green #4CAF50): Matches Fat Burn zone". The PR's theme and component logic now treat these as distinct zones with different colors. This needs to be resolved by updating the DESIGN_GUIDELINES.md to reflect the distinct nature of these zones and ensuring the HrZoneName enum (implicitly used) accurately represents all defined HR zones and their relationship to the theme. This is a high-priority technical debt item.
  • Missing Theme Colors for New Zones: The ZoneDistribution.tsx component now includes Aerobic, Recovery, and Idle in its ZONE_PRIORITY mapping. However, lib/theme.ts's custom.hrZones palette does not yet define specific colors for these new zones. They will currently fall back to theme.palette.grey[500], leading to inconsistent visuals. These new zones should be added to the theme for complete integration.
  • Percentage Formatting: The percentage value in ZoneDistribution.tsx is now calculated using toFixed(1) and stored as a string. While functional, it's generally better practice to store numerical values and format them only at the point of display. This allows for easier numerical operations if needed in the future and maintains data type consistency. Consider storing percentage as a number and formatting formattedPercentage as a string for display.

Reviewed at commit: a3682910dedacbf21a065b31dc4a668f6fc3c3e6

Thank you for the review! I have addressed your feedback:

  1. HR Zone Definition Inconsistency: I updated docs/DESIGN_GUIDELINES.md to remove the conflicting information and clearly list all zones (Max, Peak, Cardio, Aerobic, Fat Burn, Warm Up, Recovery, Idle) as distinct entities with their own colors.
  2. Missing Theme Colors: I added aerobic, recovery, and idle to lib/theme.ts (and types/mui-theme.d.ts) with distinct colors to match the updated guidelines.
  3. Percentage Formatting: Refactored ZoneDistribution.tsx to store percentage as a number in the data structure and format it as a string only during rendering (JSX).

@arii arii closed this Feb 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant