Revert "Added power spectral density curves at the spectrum chart" #824
Conversation
…taflight#820)" This reverts commit a263152.
WalkthroughThis change removes all code, UI elements, and logic related to the "Power Spectral Density" (PSD) spectrum type from the application. This includes eliminating PSD from dropdown menus, spectrum type enums, calculation methods, plotting routines, and event handlers across several source files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Dropdown)
participant SpectrumModule
User->>UI (Dropdown): Selects spectrum type
UI (Dropdown)->>SpectrumModule: Requests data for selected type
alt FFT or Normalized
SpectrumModule->>SpectrumModule: Load and plot data
else (PSD option removed)
Note right of UI (Dropdown): PSD option no longer available
end
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
src/graph_spectrum_plot.js (1)
15-20: 💡 Verification agent🧩 Analysis chain
Beware of enum‐index drift after removing the PSD entry
SPECTRUM_TYPEis now a dense 0…3 enum after the PSD member was removed.
If the selected spectrum type is ever persisted externally as the numeric value (e.g. in localStorage, URL params, or saved project files) the mapping will silently shift and load the wrong view for existing users.Consider one of the following so future refactors do not create hidden breaking changes:
-export const SPECTRUM_TYPE = { - FREQUENCY: 0, - FREQ_VS_THROTTLE: 1, - PIDERROR_VS_SETPOINT: 2, - FREQ_VS_RPM: 3, -}; +export const SPECTRUM_TYPE = Object.freeze({ + FREQUENCY: 0, + FREQ_VS_THROTTLE: 1, + PIDERROR_VS_SETPOINT: 2, + FREQ_VS_RPM: 3, + // Intentionally leave 4 reserved to prevent index drift after PSD removal +});or store / compare the string key instead of the numeric value.
I’d like to verify how
SPECTRUM_TYPEis used—especially any persistence or URL-param logic that could be affected. Running searches for its references, plus common persistence patterns:
🏁 Script executed:
#!/bin/bash set -e # 1. Find where SPECTRUM_TYPE is referenced rg -n "SPECTRUM_TYPE" # 2. Look for localStorage/sessionStorage usage rg -n "localStorage" rg -n "sessionStorage" # 3. Look for URLSearchParams or direct query-string handling rg -n "URLSearchParams" rg -n "location\\.search" # 4. Scan for load/save or persist logic mentioning spectrum rg -n "spectrum" -n srcLength of output: 32451
I want to confirm how
userSettings.spectrumTypeis loaded and saved (i.e. persisted via localStorage). Checking imports and where it’s read/written ingraph_spectrum.js:
🏁 Script executed:
#!/bin/bash set -e # Show module imports at top of graph_spectrum.js head -n 50 src/graph_spectrum.js # Find all references to userSettings (load, assign, save) rg -n -C3 "userSettings" src/graph_spectrum.jsLength of output: 4522
Preserve numeric enum indices to avoid shifting persisted spectrumType
The
userSettings.spectrumTypevalue is saved viaPrefStorage(backed bylocalStorage) as a numeric index. Removing the PSD entry has compacted the enum (FREQ_VS_THROTTLE is now 1 instead of 2, PIDERROR_VS_SETPOINT is 2 instead of 3, etc.), so existing users’ saved settings will silently load the wrong view.Recommended fixes:
- Add a reserved placeholder for the removed PSD entry to maintain original indices:
// src/graph_spectrum_plot.js export const SPECTRUM_TYPE = Object.freeze({ FREQUENCY: 0, + // reserved placeholder for removed PSD spectrum + PSD: 1, FREQ_VS_THROTTLE: 2, PIDERROR_VS_SETPOINT: 3, FREQ_VS_RPM: 4, });- Or switch to persisting/comparing string keys instead of numeric values.
Key locations:
- src/graph_spectrum_plot.js (definition at lines 15–20)
- src/graph_spectrum.js (usage and persistence around lines 199–209)
src/graph_spectrum_calc.js (1)
286-305:⚠️ Potential issue
_getFlightSamplesFreqnow always uses raw values – confirm that scaling removal is intentionalThe helper previously accepted a
scaled/rawtoggle; after the PSD rollback it unconditionally calls
this._dataBuffer.curve.lookupRaw(...).
- If any remaining caller expected scaled (post-curve) values this will silently change frequency-domain amplitudes.
_dataBuffer.curvemust be a valid object before looping; the default stub{ curve: 0 }set ininitializewill now throw.Consider:
- samples[samplesCount] = (this._dataBuffer.curve.lookupRaw(frame[this._dataBuffer.fieldIndex])); + const { curve } = this._dataBuffer; + if (curve && typeof curve.lookupRaw === 'function') { + samples[samplesCount] = curve.lookupRaw(frame[this._dataBuffer.fieldIndex]); + } else { + // Fallback to the raw numeric value if no curve is defined + samples[samplesCount] = frame[this._dataBuffer.fieldIndex]; + }…and audit all call sites to ensure the “always raw” behaviour is expected.
🧹 Nitpick comments (2)
src/graph_spectrum_plot.js (2)
988-1006: Hard-coding tick count reduces flexibility
_drawVerticalGridLines()now fixesTICKS = 5. Previously the caller could supply an arbitrary tick count (or the value was at least configurable). In zoomed-in/fullscreen situations this can make the grid feel cramped or sparse.If configurability is no longer required that’s fine, but if the constant was introduced only for convenience you could retain flexibility with a default parameter:
-GraphSpectrumPlot._drawVerticalGridLines = function (canvasCtx, LEFT, TOP, WIDTH, HEIGHT, minValue, maxValue, label) { - const TICKS = 5; +GraphSpectrumPlot._drawVerticalGridLines = function ( + canvasCtx, LEFT, TOP, WIDTH, HEIGHT, + minValue, maxValue, label, + ticks = 5 /* default */ +) { + const TICKS = ticks;Minor change, but keeps the API future-proof.
Also applies to: 1009-1034
1131-1134: Contrast tweak – double-check in light UI themesThe fullscreen gradient now transitions
rgba(0,0,0,0.7) → rgba(0,0,0,0.9), making the background ~20 % darker. No functional problem, but ensure text/readability is still adequate on low-brightness monitors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
index.html(0 hunks)src/graph_spectrum.js(0 hunks)src/graph_spectrum_calc.js(2 hunks)src/graph_spectrum_plot.js(6 hunks)
💤 Files with no reviewable changes (2)
- src/graph_spectrum.js
- index.html
🔇 Additional comments (1)
src/graph_spectrum_plot.js (1)
860-866: Label wording clarified – looks goodRenaming “Max noise” → “Max motor noise” improves specificity for the end-user without impacting behaviour. 👍
|
Thank's. |
|
Yes after you update your fork |
Reverts #820
Summary by CodeRabbit