Curves MinMax control extended menu migration to Vue#919
Merged
haslinghuis merged 8 commits intoMay 27, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughGraphConfigDialog.vue now accepts a ChangesContext Menu State-Driven Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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: 2
🤖 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 851-863: getFieldsCheckboxedSubmenu currently dereferences
currentState.value.graph.fields and can throw if state isn't seeded; update the
function to early-return an empty array (or safe fallback) when
currentState.value, currentState.value.graph, or currentState.value.graph.fields
is falsy, and ensure you read isFieldChecked safely (fallback to an array) and
guard index access when setting currentState.value.isFieldChecked[index]; this
keeps getFieldsCheckboxedSubmenu from throwing before onContextMenu initializes
state while preserving the existing mapping behavior.
- Around line 865-1084: The extendedMenuItems handlers are no-ops (they only
call e.preventDefault()) so batch Min/Max and Zoom actions never run; update
each actionable onSelect in the extendedMenuItems computed (the "SET", "At the
all/window/markers time", "ZOOM IN/OUT", "SET CURVES TO DEFAULT", etc.) to call
the existing batch helper applyToCheckedFields(...) and pass the appropriate
operation (e.g., invoke the same logic used by setMinMaxSelectedCentered,
setMinMaxSelectedZoom, setMinMaxSelectedDefault but applied per-field) and
ensure applyToCheckedFields uses isFieldChecked to scope updates to checked
fields; keep using friendlyName(currentState.value.field?.name...), zoom, and
getFieldsCheckboxedSubmenu() as needed and remove the empty e.preventDefault()
placeholders or call preventDefault then invoke applyToCheckedFields for the
action.
🪄 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: c58fb2ec-c148-45a5-b17d-4e2dde57e141
📒 Files selected for processing (1)
src/components/GraphConfigDialog.vue
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1006-1022:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWrong function called for batch zoom operations.
setMinMaxSelectedZoomat line 728 only accepts one argument (zoom) and operates on the single selected field. The second argumenttrueis silently ignored, so these batch operations effectively zoom only the right-clicked field instead of all checked fields.Should call
setMinMaxZoom(zoom, setCheckedOnly)which is the batch-aware version.🐛 Proposed fix
[ { label: "ZOOM IN", onSelect(e) { - setMinMaxSelectedZoom(1 / zoom, true); + setMinMaxZoom(1 / zoom, true); e.preventDefault(); }, }, { label: "ZOOM OUT", onSelect(e) { - setMinMaxSelectedZoom(zoom, true); + setMinMaxZoom(zoom, true); e.preventDefault(); }, }, ],🤖 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 1006 - 1022, The context menu handlers for "ZOOM IN"/"ZOOM OUT" call setMinMaxSelectedZoom(…, true) but that function only accepts one argument and affects a single selected field; replace those calls with the batch-aware setMinMaxZoom(zoom, true) so the second argument (setCheckedOnly) is honored and the operation applies to all checked fields; update the onSelect functions in the menu item objects (the ones labeled "ZOOM IN" and "ZOOM OUT") to call setMinMaxZoom with the same zoom value and preserve the e.preventDefault() behavior.
🧹 Nitpick comments (1)
src/components/GraphConfigDialog.vue (1)
654-679: ⚡ Quick winExtract repeated field-check condition to reduce complexity.
SonarCloud flags cognitive complexity of 20 (limit 15). The same condition appears multiple times across this and other setter functions.
♻️ Suggested helper extraction
+function shouldApplyToField(index, setCheckedOnly) { + return (!setCheckedOnly || !currentState.value.isFieldChecked) || currentState.value.isFieldChecked[index]; +} + function setMinMaxOneScale(setCheckedOnly) { let max = -Number.MAX_VALUE; let min = Number.MAX_VALUE; if (currentState.value.graph?.fields) { for (const [index, field] of currentState.value.graph.fields.entries()) { - if ((!setCheckedOnly || !currentState.value.isFieldChecked) || currentState.value.isFieldChecked[index]) { + if (shouldApplyToField(index, setCheckedOnly)) { const mm = field?.curve?.MinMax; // ... } }Then apply the same pattern to
setMinMaxCentered,setMinMaxZoom,setMinMaxToFullRangeDuringAllTime, etc.🤖 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 - 679, Refactor the repeated field-check logic in setMinMaxOneScale by extracting a helper (e.g., shouldProcessField(index, setCheckedOnly)) that encapsulates the condition using currentState.value.isFieldChecked and the setCheckedOnly flag, then replace the duplicated checks in both loops inside setMinMaxOneScale with calls to that helper; apply the same extraction and replacement in the similar functions setMinMaxCentered, setMinMaxZoom, setMinMaxToFullRangeDuringAllTime, etc., to reduce cognitive complexity and centralize the field-checked decision.
🤖 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 865-882: In getFieldsCheckboxedSubmenu, guard access to
currentState.value.isFieldChecked and use the local fields variable: use the
local const fields for mapping (fields.map(...)) and for each index check that
currentState.value.isFieldChecked is an array (and optionally has an entry at
index) before reading or assigning; if it's null/undefined, treat checked as
false and initialize or set the entry on update (e.g., ensure onUpdateChecked
writes into an array after creating it if needed). This prevents null
dereference when the computed runs before onContextMenu seeds state and keeps
the code consistent by using the local fields variable instead of
currentState.value.graph.fields.
---
Outside diff comments:
In `@src/components/GraphConfigDialog.vue`:
- Around line 1006-1022: The context menu handlers for "ZOOM IN"/"ZOOM OUT" call
setMinMaxSelectedZoom(…, true) but that function only accepts one argument and
affects a single selected field; replace those calls with the batch-aware
setMinMaxZoom(zoom, true) so the second argument (setCheckedOnly) is honored and
the operation applies to all checked fields; update the onSelect functions in
the menu item objects (the ones labeled "ZOOM IN" and "ZOOM OUT") to call
setMinMaxZoom with the same zoom value and preserve the e.preventDefault()
behavior.
---
Nitpick comments:
In `@src/components/GraphConfigDialog.vue`:
- Around line 654-679: Refactor the repeated field-check logic in
setMinMaxOneScale by extracting a helper (e.g., shouldProcessField(index,
setCheckedOnly)) that encapsulates the condition using
currentState.value.isFieldChecked and the setCheckedOnly flag, then replace the
duplicated checks in both loops inside setMinMaxOneScale with calls to that
helper; apply the same extraction and replacement in the similar functions
setMinMaxCentered, setMinMaxZoom, setMinMaxToFullRangeDuringAllTime, etc., to
reduce cognitive complexity and centralize the field-checked decision.
🪄 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: 1f252311-7301-4572-af2f-4fe40dad79d8
📒 Files selected for processing (1)
src/components/GraphConfigDialog.vue
Contributor
Author
|
These "Full range" submenu items do not work til:
|
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)
1007-1018:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the batch zoom helper here.
Line 728’s
setMinMaxSelectedZoomonly updatescurrentState.value.field, so these Shift-mode “SELECT CURVES” actions still zoom a single curve and ignore the checkbox selection. CallsetMinMaxZoom(..., true)instead.🔧 Proposed fix
{ label: "ZOOM IN", onSelect(e) { - setMinMaxSelectedZoom(1 / zoom, true); + setMinMaxZoom(1 / zoom, true); e.preventDefault(); }, }, { label: "ZOOM OUT", onSelect(e) { - setMinMaxSelectedZoom(zoom, true); + setMinMaxZoom(zoom, true); e.preventDefault(); }, },🤖 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 1007 - 1018, The Shift-mode zoom actions labeled "ZOOM IN" and "ZOOM OUT" are calling setMinMaxSelectedZoom which only updates currentState.value.field and thus ignores the checkbox-selected curves; replace those calls with the batch helper setMinMaxZoom(..., true) (preserving the same zoom value and the e.preventDefault() behavior) so the actions operate on all selected curves instead of a single curve.
🤖 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 1007-1018: The Shift-mode zoom actions labeled "ZOOM IN" and "ZOOM
OUT" are calling setMinMaxSelectedZoom which only updates
currentState.value.field and thus ignores the checkbox-selected curves; replace
those calls with the batch helper setMinMaxZoom(..., true) (preserving the same
zoom value and the e.preventDefault() behavior) so the actions operate on all
selected curves instead of a single curve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf40bf30-1e74-4418-b42f-b5d5c6cb8422
📒 Files selected for processing (1)
src/components/GraphConfigDialog.vue
demvlad
changed the title
demvlad
changed the title
Contributor
Author
|
@haslinghuis |
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 (2)
src/components/GraphConfigDialog.vue (1)
1038-1049:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the batch Zoom helper in the Shift menu.
These handlers sit under the checked-curves submenu, but they still call the single-field helper. Shift → Zoom therefore updates only the clicked curve instead of the checked set.
🛠️ Proposed fix
{ label: "ZOOM IN", onSelect(e) { - setMinMaxSelectedZoom(1 / zoom, true); + setMinMaxZoom(1 / zoom, true); e.preventDefault(); }, }, { label: "ZOOM OUT", onSelect(e) { - setMinMaxSelectedZoom(zoom, true); + setMinMaxZoom(zoom, true); e.preventDefault(); }, },🤖 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 1038 - 1049, The ZOOM IN/OUT handlers under the checked-curves submenu are calling the single-curve helper setMinMaxSelectedZoom(…, true) with zoom, so Shift→Zoom only affects the clicked curve; replace those calls with the batch Zoom helper that applies to the checked set (the project’s batch helper for checked curves—e.g., setMinMaxSelectedZoomForChecked or setMinMaxSelectedZoomBatch) and pass the same arguments (1/zoom, true) for ZOOM IN and (zoom, true) for ZOOM OUT, keeping the existing e.preventDefault() behavior so the action updates all checked curves instead of just the clicked one.src/App.vue (1)
99-104:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the trailing comma from the
:grapherprop binding insrc/App.vue
<GraphConfigDialog ... :grapher="graphStore.graph",@save="...">contains a trailing comma after the attribute value, which makes the Vue SFC template invalid and will fail to compile.🛠️ Proposed fix
- :grapher="graphStore.graph", + :grapher="graphStore.graph"🤖 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/App.vue` around lines 99 - 104, Summary: Remove the stray trailing comma after the :grapher prop in the GraphConfigDialog component usage to fix the invalid Vue template. Fix: edit the GraphConfigDialog JSX/Template invocation (the line with :grapher="graphStore.graph",) in src/App.vue and remove the comma so it reads :grapher="graphStore.graph" and ensure the next attribute (`@save`="onGraphConfigSave") remains on the same element; verify the template compiles and run linter/formatter to keep spacing consistent.
🤖 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/App.vue`:
- Around line 99-104: Summary: Remove the stray trailing comma after the
:grapher prop in the GraphConfigDialog component usage to fix the invalid Vue
template. Fix: edit the GraphConfigDialog JSX/Template invocation (the line with
:grapher="graphStore.graph",) in src/App.vue and remove the comma so it reads
:grapher="graphStore.graph" and ensure the next attribute
(`@save`="onGraphConfigSave") remains on the same element; verify the template
compiles and run linter/formatter to keep spacing consistent.
In `@src/components/GraphConfigDialog.vue`:
- Around line 1038-1049: The ZOOM IN/OUT handlers under the checked-curves
submenu are calling the single-curve helper setMinMaxSelectedZoom(…, true) with
zoom, so Shift→Zoom only affects the clicked curve; replace those calls with the
batch Zoom helper that applies to the checked set (the project’s batch helper
for checked curves—e.g., setMinMaxSelectedZoomForChecked or
setMinMaxSelectedZoomBatch) and pass the same arguments (1/zoom, true) for ZOOM
IN and (zoom, true) for ZOOM OUT, keeping the existing e.preventDefault()
behavior so the action updates all checked curves instead of just the clicked
one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf231762-5120-4087-92f6-9717d1c3c4be
📒 Files selected for processing (2)
src/App.vuesrc/components/GraphConfigDialog.vue
Member
|
Can do the sonar issues after - or you could include them here |
Contributor
Author
Will look now. |
|
|
Preview URL: https://pr919.betaflight-blackbox.pages.dev |
Contributor
Author
Done. |
haslinghuis
approved these changes
May 26, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The extended MinMax control menu at the charts config dialog box.
Press Shift+Right mouse click at the min-max input fields to show it.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes