The min-max control context menu at the chart config dialog#918
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a header portal and context-menu wrappers for per-field Min/Max inputs, hardens setMin/setMax numeric parsing, and introduces currentState plus bulk and per-field Min/Max helper actions exposed via a hierarchical context menu. ChangesGraph Min/Max Context Menu
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/components/GraphConfigDialog.vue (2)
711-714: 💤 Low valueDuplicate separator in menu items.
Two consecutive separators serve no visual purpose.
♻️ Proposed fix
{ label: 'Default', onSelect() { setMinMaxToDefault(); } }, { type: 'separator', }, - { - type: 'separator', - }, { label: '\u25BCClose', },🤖 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/GraphConfigDialog.vue` around lines 711 - 714, The menu items array in GraphConfigDialog.vue contains two consecutive entries with type: 'separator', which is redundant; remove one of the duplicate separator objects (or add a small dedupe step when building the menuItems array) so there are no adjacent items whose only property is type: 'separator'—look for the menu array or computed property that constructs the menu (the objects with type: 'separator') and delete the extra separator or filter out consecutive separators before rendering.
717-718: 💤 Low valueConsider clarifying the "Close" menu item purpose.
The "Close" item has a Unicode down-arrow character (
\u25BC) but noonSelecthandler. If this is intended to close the menu automatically (default behavior), consider removing the down-arrow character for clarity. If it needs a specific action, add anonSelecthandler.🤖 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/GraphConfigDialog.vue` around lines 717 - 718, The menu item currently defined as label: '\u25BCClose' is ambiguous because it shows a down-arrow but has no onSelect handler; either remove the Unicode arrow from the label to avoid implying extra behavior (change label to "Close") or add an explicit onSelect property on the same menu item object (e.g., onSelect: () => { /* call the dialog/menu close function */ }) so it performs the intended close action; update the menu item definition where label: '\u25BCClose' is declared and ensure you reference the dialog/menu close function you use elsewhere (or call the component's close handler) in the added onSelect.
🤖 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/GraphConfigDialog.vue`:
- Around line 618-625: The function setMinMaxToDefault updates fields via
resetMin(field) and resetMax(field) but never notifies the parent; after the
loop (or after modifying each field) call emitUpdate() so the updated
currentState.graph changes propagate; locate setMinMaxToDefault and add a single
emitUpdate() invocation (or ensure existing emitUpdate helper is used) to
trigger the parent update after resetting fields.
- Around line 638-649: setMinMaxCentered currently reads field.curve.MinMax
without guarding and doesn't call emitUpdate(); update it to first check that
currentState.graph exists and that each field.curve and field.curve.MinMax exist
(e.g., guard with if (!field.curve || !field.curve.MinMax) continue) before
reading MinMax.min/Max, then compute min/max and call setMin(field, min) and
setMax(field, max) as before, and finally call emitUpdate() once after the loop
to notify changes; reference setMinMaxCentered, currentState.graph,
field.curve.MinMax, setMin, setMax, and emitUpdate in your changes.
- Around line 651-665: setMinMaxOneScale currently reads field.curve.MinMax
without guarding and doesn’t notify changes; update it to first check that
currentState.graph exists and that each field has a curve and a MinMax object
(e.g., guard with field.curve && field.curve.MinMax or use optional chaining)
before accessing min/max, skip fields that lack the structure, then call
setMin(field, min) and setMax(field, max) as before and finally call
emitUpdate() after the loop to propagate the changes (referenced symbols:
setMinMaxOneScale, currentState.graph, field.curve.MinMax, setMin, setMax,
emitUpdate).
- Around line 627-636: In setMinMaxLikeThis, guard against missing nested data
and call emitUpdate after mutating fields: check that currentState.field and
currentState.field.curve and currentState.field.curve.MinMax exist (or use
optional chaining) before reading MinMax.min and MinMax.max, bail out early if
absent, then loop over currentState.graph.fields calling setMin(field, min) and
setMax(field, max) as before, and finally invoke emitUpdate() so the changes
propagate; reference symbols: setMinMaxLikeThis,
currentState.field.curve.MinMax, setMin, setMax, emitUpdate.
---
Nitpick comments:
In `@src/components/GraphConfigDialog.vue`:
- Around line 711-714: The menu items array in GraphConfigDialog.vue contains
two consecutive entries with type: 'separator', which is redundant; remove one
of the duplicate separator objects (or add a small dedupe step when building the
menuItems array) so there are no adjacent items whose only property is type:
'separator'—look for the menu array or computed property that constructs the
menu (the objects with type: 'separator') and delete the extra separator or
filter out consecutive separators before rendering.
- Around line 717-718: The menu item currently defined as label: '\u25BCClose'
is ambiguous because it shows a down-arrow but has no onSelect handler; either
remove the Unicode arrow from the label to avoid implying extra behavior (change
label to "Close") or add an explicit onSelect property on the same menu item
object (e.g., onSelect: () => { /* call the dialog/menu close function */ }) so
it performs the intended close action; update the menu item definition where
label: '\u25BCClose' is declared and ensure you reference the dialog/menu close
function you use elsewhere (or call the component's close handler) in the added
onSelect.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0dc53e3c-a62d-47f3-910c-4710c6b14f9e
📒 Files selected for processing (1)
src/components/GraphConfigDialog.vue
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/components/GraphConfigDialog.vue (1)
628-631:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nested
curve.MinMaxreads before bulk actions.At Line 630, Line 643, and Line 659, the code reads
field.curve.MinMaxwithout guards. If any field is missing that structure, these menu actions will throw and abort.🔧 Suggested fix
function setMinMaxLikeThis() { if (currentState.graph && currentState.field) { - const min = currentState.field.curve.MinMax.min; - const max = currentState.field.curve.MinMax.max; + ensureCurveMinMax(currentState.field); + const min = currentState.field.curve.MinMax.min ?? -500; + const max = currentState.field.curve.MinMax.max ?? 500; for (const field of currentState.graph.fields) { setMin(field, min); setMax(field, max); } emitUpdate(); } } function setMinMaxCentered() { if (currentState.graph) { for (const field of currentState.graph.fields) { - let min = field.curve.MinMax.min; - let max = field.curve.MinMax.max; + ensureCurveMinMax(field); + let min = field.curve.MinMax.min ?? -500; + let max = field.curve.MinMax.max ?? 500; max = Math.max(Math.abs(min), Math.abs(max)); min = -max; setMin(field, min); setMax(field, max); } emitUpdate(); } } function setMinMaxOneScale() { let max = -Number.MAX_VALUE, min = Number.MAX_VALUE; if (currentState.graph) { for (const field of currentState.graph.fields) { - max = Math.max(max, Math.max(Math.abs(field.curve.MinMax.min), Math.abs(field.curve.MinMax.max))); + ensureCurveMinMax(field); + const fMin = field.curve.MinMax.min ?? -500; + const fMax = field.curve.MinMax.max ?? 500; + max = Math.max(max, Math.max(Math.abs(fMin), Math.abs(fMax))); } min = -max;Also applies to: 640-645, 654-660
🤖 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/GraphConfigDialog.vue` around lines 628 - 631, The menu actions read currentState.field.curve.MinMax directly (e.g., in setMinMaxLikeThis) and can throw if field/curve/MinMax is missing; guard these accesses by checking currentState.field && currentState.field.curve && currentState.field.curve.MinMax (or use optional chaining currentState.field?.curve?.MinMax) before reading .min/.max, return early or disable the action when absent, and apply the same change to the other functions that read field.curve.MinMax at the locations referenced (the other menu action handlers around lines ~643 and ~659).
🤖 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.
Duplicate comments:
In `@src/components/GraphConfigDialog.vue`:
- Around line 628-631: The menu actions read currentState.field.curve.MinMax
directly (e.g., in setMinMaxLikeThis) and can throw if field/curve/MinMax is
missing; guard these accesses by checking currentState.field &&
currentState.field.curve && currentState.field.curve.MinMax (or use optional
chaining currentState.field?.curve?.MinMax) before reading .min/.max, return
early or disable the action when absent, and apply the same change to the other
functions that read field.curve.MinMax at the locations referenced (the other
menu action handlers around lines ~643 and ~659).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d7e5256-c1ac-45e4-befe-564f5f1fe67c
📒 Files selected for processing (1)
src/components/GraphConfigDialog.vue
demvlad
dismissed stale reviews from haslinghuis and coderabbitai[bot]
via
565ed54
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/GraphConfigDialog.vue (2)
640-652:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUndefined MinMax values produce NaN.
Lines 643-644 read
field.curve?.MinMax?.min/maxwithout fallbacks. If any field's MinMax is undefined,Math.abs(undefined)returnsNaN, which propagates through the calculation:max = Math.max(NaN, NaN) = NaN,min = -NaN = NaN. All fields end up withNaNmin/max values.🛡️ Proposed fix
function setMinMaxCentered() { if (currentState.graph) { for (const field of currentState.graph.fields) { + ensureCurveMinMax(field); let min = field.curve.MinMax.min; let max = field.curve.MinMax.max; max = Math.max(Math.abs(min), Math.abs(max));🤖 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/GraphConfigDialog.vue` around lines 640 - 652, The function setMinMaxCentered currently reads field.curve?.MinMax?.min/max without fallbacks which yields NaN when undefined; update setMinMaxCentered to guard those accesses (e.g. read const rawMin = field.curve?.MinMax?.min ?? 0 and const rawMax = field.curve?.MinMax?.max ?? 0 or skip the field if both are undefined), then compute max = Math.max(Math.abs(rawMin), Math.abs(rawMax)), min = -max, and call setMin(field, min) and setMax(field, max) before emitUpdate to avoid propagating NaN; target the setMinMaxCentered function and the field.curve?.MinMax?.min/max reads, leaving setMin, setMax, and emitUpdate intact.
654-669:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUndefined MinMax values produce NaN.
Line 659 computes
Math.abs(field.curve?.MinMax?.min)andMath.abs(field.curve?.MinMax?.max)without fallbacks. If any field's MinMax is undefined,Math.abs(undefined)returnsNaN, andMath.max(..., NaN)returnsNaN. The finalminandmaxwill beNaN, corrupting all fields.🛡️ Proposed fix
function setMinMaxOneScale() { let max = -Number.MAX_VALUE; let min; if (currentState.graph) { for (const field of currentState.graph.fields) { + ensureCurveMinMax(field); max = Math.max(max, Math.max(Math.abs(field.curve.MinMax.min), Math.abs(field.curve.MinMax.max))); }🤖 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/GraphConfigDialog.vue` around lines 654 - 669, In setMinMaxOneScale, accessing Math.abs(field.curve?.MinMax?.min/max) can produce NaN when MinMax is undefined; update the loop to only consider numeric finite values (e.g., check that field.curve?.MinMax?.min/max are != null and isFinite) or coerce with a numeric fallback before Math.abs, initialize max to -Infinity instead of -Number.MAX_VALUE, compute max as the maximum of only valid values, then derive min = -max and call setMin(field, min)/setMax(field, max) as before; reference symbols: setMinMaxOneScale, currentState.graph.fields, field.curve?.MinMax, setMin, setMax, emitUpdate.
🧹 Nitpick comments (2)
src/components/GraphConfigDialog.vue (2)
31-31: 💤 Low valueUnused portal container.
The
menu-portal-containerdiv is not used because bothUContextMenuinstances set:portal="false"(lines 163, 182), so menus render in-place rather than being portaled. Either remove this div or document why it's reserved for future use.🤖 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/GraphConfigDialog.vue` at line 31, The <div id="menu-portal-container"></div> is unused because both UContextMenu instances set :portal="false"; either remove this unused DOM node from the template or make the portal active: set :portal="true" on the UContextMenu components (references: UContextMenu instances around the menu definitions) and ensure the container id "menu-portal-container" remains in the template; if you keep the div for future use, add a short comment in the template explaining it's reserved for portaled menus so it's not mistaken as dead code.
717-719: 💤 Low valueDuplicate separators.
Lines 717–718 define two consecutive separator items with no menu items between them. This creates redundant visual spacing.
🧹 Proposed fix
{ type: 'separator', }, - { - type: 'separator', - }, { label: '\u25BCClose', },🤖 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/GraphConfigDialog.vue` around lines 717 - 719, There are two consecutive menu separator objects (type: 'separator') in the menu/items array inside GraphConfigDialog.vue, creating redundant spacing; remove the duplicate separator or add a small dedupe check so you never push two adjacent { type: 'separator' } entries (e.g., when building the menu array, skip adding a separator if the last item is already a separator) to ensure only single separators remain between groups.
🤖 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/GraphConfigDialog.vue`:
- Around line 628-638: The code in setMinMaxLikeThis reads
currentState.field.curve?.MinMax?.min/max without fallbacks, so undefined flows
into setMin/setMax and becomes NaN; change setMinMaxLikeThis to first read the
raw values (e.g., const minRaw = currentState.field.curve?.MinMax?.min; const
maxRaw = currentState.field.curve?.MinMax?.max), convert/validate them (e.g.,
const min = minRaw !== undefined && !Number.isNaN(Number(minRaw)) ?
Number(minRaw) : undefined), and only call setMin(field, min) or setMax(field,
max) when the validated value is a finite number (or provide a safe default)
before emitting via emitUpdate; reference setMinMaxLikeThis, currentState,
MinMax, setMin, setMax, and emitUpdate when locating the code.
---
Duplicate comments:
In `@src/components/GraphConfigDialog.vue`:
- Around line 640-652: The function setMinMaxCentered currently reads
field.curve?.MinMax?.min/max without fallbacks which yields NaN when undefined;
update setMinMaxCentered to guard those accesses (e.g. read const rawMin =
field.curve?.MinMax?.min ?? 0 and const rawMax = field.curve?.MinMax?.max ?? 0
or skip the field if both are undefined), then compute max =
Math.max(Math.abs(rawMin), Math.abs(rawMax)), min = -max, and call setMin(field,
min) and setMax(field, max) before emitUpdate to avoid propagating NaN; target
the setMinMaxCentered function and the field.curve?.MinMax?.min/max reads,
leaving setMin, setMax, and emitUpdate intact.
- Around line 654-669: In setMinMaxOneScale, accessing
Math.abs(field.curve?.MinMax?.min/max) can produce NaN when MinMax is undefined;
update the loop to only consider numeric finite values (e.g., check that
field.curve?.MinMax?.min/max are != null and isFinite) or coerce with a numeric
fallback before Math.abs, initialize max to -Infinity instead of
-Number.MAX_VALUE, compute max as the maximum of only valid values, then derive
min = -max and call setMin(field, min)/setMax(field, max) as before; reference
symbols: setMinMaxOneScale, currentState.graph.fields, field.curve?.MinMax,
setMin, setMax, emitUpdate.
---
Nitpick comments:
In `@src/components/GraphConfigDialog.vue`:
- Line 31: The <div id="menu-portal-container"></div> is unused because both
UContextMenu instances set :portal="false"; either remove this unused DOM node
from the template or make the portal active: set :portal="true" on the
UContextMenu components (references: UContextMenu instances around the menu
definitions) and ensure the container id "menu-portal-container" remains in the
template; if you keep the div for future use, add a short comment in the
template explaining it's reserved for portaled menus so it's not mistaken as
dead code.
- Around line 717-719: There are two consecutive menu separator objects (type:
'separator') in the menu/items array inside GraphConfigDialog.vue, creating
redundant spacing; remove the duplicate separator or add a small dedupe check so
you never push two adjacent { type: 'separator' } entries (e.g., when building
the menu array, skip adding a separator if the last item is already a separator)
to ensure only single separators remain between groups.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db552e92-1a65-4abd-a64a-d551437e35e9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
src/components/GraphConfigDialog.vue
|
Please
|
|
@demvlad please check the bot to satisfy |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Will try to use drop menu instead of context. |
|
The simple PreventDefault solved everything:) |
demvlad
dismissed stale reviews from haslinghuis and coderabbitai[bot]
via
d498935
PR #918 ReviewSummary: Adds a right-click context menu on Min/Max inputs in the graph config dialog, providing bulk actions (reset to defaults, copy range, center, one scale, zoom, full range) for all fields or a single selected field. Issues Found1. Mutable shared state via
|
|
@demvlad review is AI based - it got wrong about i18n 😃 |
|
|
Preview URL: https://pr918.betaflight-blackbox.pages.dev |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/GraphConfigDialog.vue (1)
635-745:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat non-finite
MinMaxvalues as invalid in the bulk helpers.Lines 637, 655, 675, 703, 716, 729, and 741 only exclude
undefined. Existing configs are still cloned verbatim intofield.curve.MinMax, so persistedNaN/Infinityvalues will pass these guards, poison theMath.max/Math.minmath, and make the selected actions silently no-op. Please switch these checks to finite-number guards or normalize once on load.💡 Minimal hardening
+function hasFiniteMinMax(mm) { + return Number.isFinite(mm?.min) && Number.isFinite(mm?.max); +} + function setMinMaxLikeThis() { const mm = currentState.value.field?.curve?.MinMax; - if (currentState.value.graph?.fields && mm?.min !== undefined && mm?.max !== undefined) { + if (currentState.value.graph?.fields && hasFiniteMinMax(mm)) { const min = mm.min; const max = mm.max; for (const field of currentState.value.graph.fields) { setMin(field, min); setMax(field, max); @@ for (const field of currentState.value.graph.fields) { const mm = field?.curve?.MinMax; - if (mm?.min !== undefined && mm?.max !== undefined) { + if (hasFiniteMinMax(mm)) { max = Math.max(max, mm.max); min = Math.min(min, mm.min); } }🤖 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/GraphConfigDialog.vue` around lines 635 - 745, The bulk MinMax helpers (setMinMaxLikeThis, setMinMaxOneScale, setMinMaxCentered, setMinMaxSelectedCentered, setMinMaxZoom, setMinMaxSelectedZoom, setMinMaxToFullRangeDuringAllTime, setMinMaxSelectedToFullRangeDuringAllTime) only guard against undefined and therefore allow NaN/Infinity to poison Math.min/Math.max; update each helper to treat only finite numeric min/max as valid (use Number.isFinite or an equivalent finite-number check) before using mm.min/mm.max, or alternatively normalize field.curve.MinMax to finite numbers when loading configs so subsequent checks can remain simple. Ensure all occurrences listed replace the mm?.min !== undefined && mm?.max !== undefined checks with finite-number guards and/or normalization to prevent silent no-ops.
🤖 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.
Outside diff comments:
In `@src/components/GraphConfigDialog.vue`:
- Around line 635-745: The bulk MinMax helpers (setMinMaxLikeThis,
setMinMaxOneScale, setMinMaxCentered, setMinMaxSelectedCentered, setMinMaxZoom,
setMinMaxSelectedZoom, setMinMaxToFullRangeDuringAllTime,
setMinMaxSelectedToFullRangeDuringAllTime) only guard against undefined and
therefore allow NaN/Infinity to poison Math.min/Math.max; update each helper to
treat only finite numeric min/max as valid (use Number.isFinite or an equivalent
finite-number check) before using mm.min/mm.max, or alternatively normalize
field.curve.MinMax to finite numbers when loading configs so subsequent checks
can remain simple. Ensure all occurrences listed replace the mm?.min !==
undefined && mm?.max !== undefined checks with finite-number guards and/or
normalization to prevent silent no-ops.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6fe08d9d-9d8c-4fb5-843e-1afd93cd307f
📒 Files selected for processing (1)
src/components/GraphConfigDialog.vue
2 participants
Added min-max control context menu
Summary by CodeRabbit