Enhance measurement tracking UI and improve preferences handling #1690
+156
−82
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Extract the measurements configuration UI (approximately 160 lines) into a standalone MeasurementsSection component to improve code organization and maintainability. This reduces complexity in the main Page component while keeping all functionality intact, including drag-and-drop reordering, add/remove operations, and form state management. The component follows project conventions by using a single props parameter and maintaining proper TypeScript typing.
Contributor
WalkthroughExtracted measurements UI into a new MeasurementsSection with drag-and-drop reordering; measurement form labels now include units and onChange handlers update the form's statistics array directly. EditDashboardElement signature now accepts an isEditDisabled prop; notificationContent renamed to disabledNotificationContent. Changes
Sequence DiagramsequenceDiagram
participant User
participant MeasurementsSection
participant DragDropContext as Drag-Drop Context
participant FormState as Form State
User->>MeasurementsSection: Drag item
MeasurementsSection->>Drag-Drop Context: onDragEnd(event)
Drag-Drop Context-->>MeasurementsSection: {sourceIndex, destinationIndex}
MeasurementsSection->>MeasurementsSection: Reorder measurements array
MeasurementsSection->>Form State: form.setFieldValue("information.statistics", newArray)
Form State-->>MeasurementsSection: Updated form values
MeasurementsSection->>User: Render reordered list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/frontend/app/components/routes/dashboard/forms/create-or-update-measurement-form.tsx`:
- Around line 117-126: The onChange handler for updating statistics mutates
objects inside form.values.information.statistics via newStatistics[idx].value =
..., causing direct state mutation; fix by creating a new array with a new
object for the replaced item (e.g., map or slice + object spread) instead of
mutating the existing object, then call
form.setFieldValue("information.statistics", newArray); ensure you update only
the matching item by name and leave others as new references so form libraries
detect the change (refer to the onChange closure,
form.values.information.statistics, and form.setFieldValue).
In `@apps/frontend/app/routes/_dashboard.settings.preferences.tsx`:
- Around line 708-722: The onDragEnd handler inside DragDropContext currently
uses destination?.index || 0 which moves items to index 0 when destination is
undefined; update the onDragEnd callback to early-return if destination is falsy
(i.e., if (!destination) return) before calling reorder and
props.form.setFieldValue, preserving order when dropped outside; keep the
existing props.isEditDisabled guard and
notifications.show(disabledNotificationContent) behavior for disabled edits and
apply the same early-return pattern to the other DragDropContext onDragEnd that
also uses reorder.
- Around line 731-734: The key currently uses the array index
(key={`measurement-${index}`}) which breaks reconciliation for
reorderable/drag-and-drop lists; change the component to use a stable identifier
(e.g., measurement.id) instead of index and, if measurements lack an id, assign
a UUID when a measurement is created/added (update the
addMeasurement/createMeasurement logic to set id). Replace
key={`measurement-${index}`} with something like
key={`measurement-${measurement.id}`} and ensure all places that construct new
measurement objects generate and persist the stable id.
apps/frontend/app/components/routes/dashboard/forms/create-or-update-measurement-form.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/frontend/app/routes/_dashboard.settings.preferences.tsx`:
- Around line 705-707: The UI currently only warns about removing measurements
(the Text size="xs" c="dimmed" message) but allows renaming in the measurement
rename flow (the rename input/handler around lines 766-781) without any warning;
update the small help text to mention that renaming a measurement will also
affect history visibility and then add a confirmation/warning step in the rename
handler (the function that processes the rename action in the measurement rename
flow) that prompts the user to confirm the rename or choose to preserve history,
ensuring the confirmation includes the same wording as the updated Text and
blocks the rename unless the user explicitly accepts.
- Around line 681-685: Extract the MeasurementsSection React component into its
own module: create a new file exporting the MeasurementsSection component (named
export or default) that accepts props { isEditDisabled: boolean; form:
ReturnType<typeof useForm<UserPreferences>> } and preserve its internal JSX and
imports (bring over any used hooks, types like UserPreferences and useForm, and
UI components). In the original file replace the component definition with an
import of MeasurementsSection and update any relative imports/types as needed so
TypeScript compiles (ensure you export the component from the new module and
update the original file’s import statement). Run a quick build/TS check and
update any tests or story references that referenced MeasurementsSection
directly. Ensure the original file size drops below the 500-line guideline.
Comment on lines
+681
to
+685
| const MeasurementsSection = (props: { | ||
| isEditDisabled: boolean; | ||
| form: ReturnType<typeof useForm<UserPreferences>>; | ||
| }) => ( | ||
| <Stack gap="xs"> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting MeasurementsSection to keep the file under 500 lines.
This file is ~950 lines; moving MeasurementsSection to its own module would align with the 500-line guideline and improve maintainability.
As per coding guidelines: Keep code files below 500 lines. If a file exceeds this limit, split it into smaller files using functions, components, or modules to improve readability and maintainability.
🤖 Prompt for AI Agents
In `@apps/frontend/app/routes/_dashboard.settings.preferences.tsx` around lines
681 - 685, Extract the MeasurementsSection React component into its own module:
create a new file exporting the MeasurementsSection component (named export or
default) that accepts props { isEditDisabled: boolean; form: ReturnType<typeof
useForm<UserPreferences>> } and preserve its internal JSX and imports (bring
over any used hooks, types like UserPreferences and useForm, and UI components).
In the original file replace the component definition with an import of
MeasurementsSection and update any relative imports/types as needed so
TypeScript compiles (ensure you export the component from the new module and
update the original file’s import statement). Run a quick build/TS check and
update any tests or story references that referenced MeasurementsSection
directly. Ensure the original file size drops below the 500-line guideline.
Comment on lines
+705
to
+707
| <Text size="xs" c="dimmed"> | ||
| Removing a measurement will hide its past values from history. If you add | ||
| it again, those values will show back up. |
Contributor
There was a problem hiding this comment.
Warn on renaming measurements to avoid silent history loss.
Line 705-707 only mentions removal, but Line 766-781 allows renaming without warning; per the issue objective, users should be warned or history preserved.
✏️ Suggested warning update
- <Text size="xs" c="dimmed">
- Removing a measurement will hide its past values from history. If you add
- it again, those values will show back up.
- </Text>
+ <Text size="xs" c="dimmed">
+ Renaming or removing a measurement will hide its past values from history.
+ If you add it again using the original name, those values will show back up.
+ </Text>Also applies to: 766-781
🤖 Prompt for AI Agents
In `@apps/frontend/app/routes/_dashboard.settings.preferences.tsx` around lines
705 - 707, The UI currently only warns about removing measurements (the Text
size="xs" c="dimmed" message) but allows renaming in the measurement rename flow
(the rename input/handler around lines 766-781) without any warning; update the
small help text to mention that renaming a measurement will also affect history
visibility and then add a confirmation/warning step in the rename handler (the
function that processes the rename action in the measurement rename flow) that
prompts the user to confirm the rename or choose to preserve history, ensuring
the confirmation includes the same wording as the updated Text and blocks the
rename unless the user explicitly accepts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce an add button for measurement tracking, streamline the statistics update logic, and enhance the user interface for managing measurements. Refactor the measurements section into a dedicated component for better organization and maintainability. Update notification handling for demo users in preferences and add unit display to measurement statistics inputs. Additionally, provide informational text regarding measurement removal in preferences.
Fixes #1689.
Summary by CodeRabbit
New Features
Improvements
Refactor