Add per-area mowing overrides (outline count/overlap/offset, angle) - #5
Conversation
|
|
8fc70d0 to
8351bb7
Compare
|
@coderabbitai full review Edit: sorry |
|
I’ll run a full review of the PR now. ✅ Action performedFull review finished. |
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds optional mowing override fields to ChangesPer-area mowing overrides
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
The PR description images show an Area Settings modal in a dark-themed UI with green accents and white form cards. The first image shows Name set to
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/map/edit/AreaSettingsDialog.tsx`:
- Around line 93-103: The override parsing in AreaSettingsDialog’s applyOverride
helper is too permissive for outline_count and outline_overlap_count, which are
defined as non-negative integers in the schema. Update the parsing/validation
path used by applyOverride so these count fields only accept whole, non-negative
integers and reject fractional or negative input instead of silently coercing it
via parseInt. Keep outline_offset parsing separate as a float, and make the
change in the AreaSettingsDialog logic that sets properties[key] so persisted
area properties match the schema contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 33bd30b0-4017-4b47-950b-ce83f19bccc8
📒 Files selected for processing (2)
src/components/map/edit/AreaSettingsDialog.tsxsrc/stores/schemas.ts
8351bb7 to
39572a7
Compare
🧪 Test this PRA multi-arch image (amd64 + arm64) for this PR has been pushed to: OpenMowerOS v2
Plain Dockerdocker run --rm -p 3000:3000 ghcr.io/xtech/openmower-app:pr-5
|
Add optional per-area overrides for outline_count, outline_overlap_count, outline_offset and angle, shown in a collapsible "Advanced" section in the area settings dialog (auto-expanded when the area already has overrides set, with a badge showing how many are set). Empty fields fall back to the global config defaults by omitting the key from map.json, matching ROS's overrideOrGlobal sentinels. Angle is entered in degrees and stored as radians to match ROS (mower_map MapArea.msg).
39572a7 to
5546f03
Compare
|
Tested this today, functionality LGTM! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/ui/TooltipTextField.tsx (2)
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIcon button lacks an accessible name and is unreachable via keyboard.
tabIndex={-1}removes the info button from tab order, and there is noaria-label, so keyboard/screen-reader users cannot discover the tooltip content conveyed only through this icon.🛠️ Proposed fix
- <IconButton size="small" tabIndex={-1} edge="end"> + <IconButton size="small" tabIndex={-1} edge="end" aria-label={tooltip}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/TooltipTextField.tsx` around lines 32 - 34, The IconButton in TooltipTextField is missing an accessible name and is removed from the keyboard tab order. Update the IconButton used with InfoOutlinedIcon to be reachable by keyboard (remove the negative tabIndex) and add an appropriate aria-label so screen readers can identify the tooltip trigger. Keep the fix localized to TooltipTextField and preserve the existing icon/tooltip behavior.
50-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnsafe
Record<string, unknown>cast forslotProps.input.Casting away the typed
slotPropsshape loses type safety on the merge target. Consider narrowing viaTextFieldProps['slotProps']instead of an opaque cast.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/TooltipTextField.tsx` around lines 50 - 56, The merge for TooltipTextField’s slotProps.input is using an unsafe Record<string, unknown> cast that bypasses the intended type shape. Update the merge in TooltipTextField to use the typed TextFieldProps['slotProps'] shape (or a properly narrowed equivalent) when spreading slotProps.input so the merge stays type-safe and doesn’t rely on an opaque cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ui/TooltipTextField.tsx`:
- Around line 46-58: The TooltipTextField wrapper is overriding any
caller-provided onKeyDown handler by always setting blockArrowKeys after
spreading props, so update the TextField composition to chain both handlers
instead of replacing one. Locate the TextField render in TooltipTextField and
merge props.onKeyDown with blockArrowKeys so both run when present, preserving
the existing arrow-key blocking behavior while honoring consumer callbacks.
---
Nitpick comments:
In `@src/components/ui/TooltipTextField.tsx`:
- Around line 32-34: The IconButton in TooltipTextField is missing an accessible
name and is removed from the keyboard tab order. Update the IconButton used with
InfoOutlinedIcon to be reachable by keyboard (remove the negative tabIndex) and
add an appropriate aria-label so screen readers can identify the tooltip
trigger. Keep the fix localized to TooltipTextField and preserve the existing
icon/tooltip behavior.
- Around line 50-56: The merge for TooltipTextField’s slotProps.input is using
an unsafe Record<string, unknown> cast that bypasses the intended type shape.
Update the merge in TooltipTextField to use the typed
TextFieldProps['slotProps'] shape (or a properly narrowed equivalent) when
spreading slotProps.input so the merge stays type-safe and doesn’t rely on an
opaque cast.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b1f2ae2d-86e2-4784-8e5b-f4725adf7b4e
📒 Files selected for processing (3)
src/components/map/edit/AreaSettingsDialog.tsxsrc/components/ui/TooltipTextField.tsxsrc/stores/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/stores/schemas.ts
- src/components/map/edit/AreaSettingsDialog.tsx
| return ( | ||
| <TextField | ||
| {...props} | ||
| onKeyDown={blockArrowKeys} | ||
| slotProps={{ | ||
| ...slotProps, | ||
| input: { | ||
| endAdornment: infoAdornment, | ||
| ...(slotProps as Record<string, unknown>)?.input, | ||
| }, | ||
| }} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
onKeyDown silently overrides any caller-supplied handler.
onKeyDown={blockArrowKeys} is set after {...props}, so if a consumer ever passes its own onKeyDown, it is discarded instead of composed. No current caller does this, but it's a latent trap for this reusable wrapper.
♻️ Proposed fix to chain handlers
- <TextField
- {...props}
- onKeyDown={blockArrowKeys}
+ <TextField
+ {...props}
+ onKeyDown={(e) => {
+ blockArrowKeys?.(e);
+ props.onKeyDown?.(e);
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <TextField | |
| {...props} | |
| onKeyDown={blockArrowKeys} | |
| slotProps={{ | |
| ...slotProps, | |
| input: { | |
| endAdornment: infoAdornment, | |
| ...(slotProps as Record<string, unknown>)?.input, | |
| }, | |
| }} | |
| /> | |
| ); | |
| return ( | |
| <TextField | |
| {...props} | |
| onKeyDown={(e) => { | |
| blockArrowKeys?.(e); | |
| props.onKeyDown?.(e); | |
| }} | |
| slotProps={{ | |
| ...slotProps, | |
| input: { | |
| endAdornment: infoAdornment, | |
| ...(slotProps as Record<string, unknown>)?.input, | |
| }, | |
| }} | |
| /> | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ui/TooltipTextField.tsx` around lines 46 - 58, The
TooltipTextField wrapper is overriding any caller-provided onKeyDown handler by
always setting blockArrowKeys after spreading props, so update the TextField
composition to chain both handlers instead of replacing one. Locate the
TextField render in TooltipTextField and merge props.onKeyDown with
blockArrowKeys so both run when present, preserving the existing arrow-key
blocking behavior while honoring consumer callbacks.
4 participants
Summary
Adds optional per-area overrides for the global mowing settings, editable in the area settings dialog:
outline_countoutline_overlap_countoutline_offset(m)angle(fixed mow direction)They live in a collapsible Advanced section (matching the settings fieldset style). It's collapsed by default and auto-expands when the area already has any override set, with a badge showing how many are set. Empty fields fall back to the global config default — the key is simply omitted from
map.json.Context
The ROS v1.2.0 release notes introduced these per-area overrides and noted the app side was still to come:
This PR implements that planned support for all four attributes.
Relationship to #3
This extends #3 (thanks @jrv), which surfaces
outline_count. This PR covers all four per-area overrides and implements the advanced collapsible section and "use global default" placeholders suggested in the review of #3.Details
properties(src/stores/schemas.ts).featuresToMap→map.replace; the fields are shown only for mowing areas.map.jsonto match ROS (open_mower_rosmower_map/msg/MapArea.msg:angleis radians). The "use global default" sentinels match ROS'soverrideOrGlobal(omit the key; ROS uses-1for the counts andNaNfor offset/angle).Screenshots
Testing
tsc --noEmit,prettier --check, andnext buildall pass.map.replacepersists and reads back correctly, with other areas untouched.Summary by CodeRabbit