-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: use included metrics data in edit habit dialog #347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: use included metrics data in edit habit dialog #347
Conversation
📝 WalkthroughWalkthroughThis PR removes store-based metric hooks (useHabitMetrics and useOccurrenceMetricValues) and updates EditHabitDialog to derive metricDefinitions directly from the habit object instead of fetching through a dedicated metrics hook. This eliminates an indirection layer in the metrics data flow. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
#211 Bundle Size — 1.87MiB (~-0.01%).af764bd(current) vs df7f052 main#210(baseline) Warning Bundle contains 2 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch refactor/use-included-metric-dat... Project dashboard Generated by RelativeCI Documentation Report issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/habit/EditHabitDialog.tsx`:
- Around line 65-76: The mapping from habit.metricDefinitions to localMetrics
uses an unchecked cast (m.config as LocalMetricDefinition['config']) which can
hide incompatibilities; replace the cast by either validating/transforming
m.config with a type guard or converter function (e.g., isValidLocalMetricConfig
or normalizeMetricConfig) and use a concise spread mapping (map(m => ({ ...m,
config: normalizeMetricConfig(m.config) }))) so LocalMetricDefinition fields are
constructed safely; update the mapping in EditHabitDialog.tsx (localMetrics and
the map callback) to call the validator/converter and remove the raw type
assertion.
🧹 Nitpick comments (1)
src/components/habit/EditHabitDialog.tsx (1)
1-25: Consider organizing imports per perfectionist plugin rules.The imports could be better organized according to the perfectionist plugin configuration specified in the coding guidelines. Typically, external packages should be sorted alphabetically, followed by internal path aliases.
Expected organization:
- External packages in alphabetical order:
reactbefore@heroui/react- Internal aliases in alphabetical order (currently correct:
@components,@hooks,@models,@stores,@utils)As per coding guidelines: Apply ESLint v9 flat config rules and use perfectionist plugin for import organization.
| const localMetrics: LocalMetricDefinition[] = habit.metricDefinitions.map( | ||
| (m) => { | ||
| return { | ||
| config: m.config as LocalMetricDefinition['config'], | ||
| id: m.id, | ||
| isRequired: m.isRequired, | ||
| name: m.name, | ||
| sortOrder: m.sortOrder, | ||
| type: m.type, | ||
| }; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify type safety and consider simplifying the mapping.
The type cast on line 68 (m.config as LocalMetricDefinition['config']) asserts type compatibility without validation. If habit.metricDefinitions[].config doesn't match LocalMetricDefinition['config'], this could cause runtime issues.
Additionally, the explicit return statement and property-by-property mapping could be simplified using object spread or direct return.
Run the following script to examine the type definitions and verify compatibility:
#!/bin/bash
# Description: Check type definitions for Habit metricDefinitions config vs LocalMetricDefinition config
# Find LocalMetricDefinition type definition
echo "=== LocalMetricDefinition type ==="
ast-grep --pattern 'type LocalMetricDefinition = {
$$$
}'
# Find MetricDefinition interface/type in models
echo -e "\n=== MetricDefinition in models ==="
rg -A 10 'type MetricDefinition|interface MetricDefinition' --type=ts
# Find Habit type definition to see metricDefinitions property
echo -e "\n=== Habit type metricDefinitions property ==="
rg -A 20 'type Habit|interface Habit' --type=ts | rg -A 20 'metricDefinitions'♻️ Optional: Simplify the mapping
- const localMetrics: LocalMetricDefinition[] = habit.metricDefinitions.map(
- (m) => {
- return {
- config: m.config as LocalMetricDefinition['config'],
- id: m.id,
- isRequired: m.isRequired,
- name: m.name,
- sortOrder: m.sortOrder,
- type: m.type,
- };
- }
- );
+ const localMetrics: LocalMetricDefinition[] = habit.metricDefinitions.map(
+ (m) => ({
+ config: m.config as LocalMetricDefinition['config'],
+ id: m.id,
+ isRequired: m.isRequired,
+ name: m.name,
+ sortOrder: m.sortOrder,
+ type: m.type,
+ })
+ );🤖 Prompt for AI Agents
In `@src/components/habit/EditHabitDialog.tsx` around lines 65 - 76, The mapping
from habit.metricDefinitions to localMetrics uses an unchecked cast (m.config as
LocalMetricDefinition['config']) which can hide incompatibilities; replace the
cast by either validating/transforming m.config with a type guard or converter
function (e.g., isValidLocalMetricConfig or normalizeMetricConfig) and use a
concise spread mapping (map(m => ({ ...m, config:
normalizeMetricConfig(m.config) }))) so LocalMetricDefinition fields are
constructed safely; update the mapping in EditHabitDialog.tsx (localMetrics and
the map callback) to call the validator/converter and remove the raw type
assertion.
Remove useHabitMetrics and useOccurrenceMetricValues from the metrics store and drop the zustand shallow import. Update EditHabitDialog to stop fetching habit metrics and instead read metricDefinitions from the habit prop, simplify related effects, and wire metrics actions (add/remove/update) accordingly. This reduces redundant fetching and relies on the habit's embedded metric definitions for editing.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.