frontend: show snackbar notifications on parameter changes#3868
Open
Williangalvani wants to merge 1 commit intobluerobotics:masterfrom
Open
frontend: show snackbar notifications on parameter changes#3868Williangalvani wants to merge 1 commit intobluerobotics:masterfrom
Williangalvani wants to merge 1 commit intobluerobotics:masterfrom
Conversation
Reviewer's GuideRefactors the global Alerter component from a single snackbar to a stack of dismissible alerts, introduces queueing and eviction logic for up to five concurrent messages, and wires parameter updates to emit dev‑mode notifications once all parameters are loaded, including minimal support in the parameter table and message manager. Sequence diagram for dev-mode parameter change notificationssequenceDiagram
actor Developer
participant Autopilot
participant ParameterFetcher
participant ParametersTable
participant MessageManager
participant Alerter
Developer->>Autopilot: Change parameter value
Autopilot-->>ParameterFetcher: PARAM_VALUE message
ParameterFetcher->>ParametersTable: updateParam(param_name, trimmed_value)
ParametersTable-->>ParameterFetcher: ChangeInfo or null
ParameterFetcher->>ParameterFetcher: allParametersLoaded()
alt ChangeInfo not null AND allParametersLoaded AND dev mode
ParameterFetcher->>MessageManager: emitMessage(Info, formatted message)
MessageManager->>Alerter: callback(level, msg)
Alerter->>Alerter: showAlert(entry)
Alerter->>Developer: Display alert in stack
else No notification conditions met
note over ParameterFetcher: No message emitted
end
opt Auto timeout
Alerter->>Alerter: dismiss(id)
end
opt Manual dismiss
Developer->>Alerter: Click dismiss
Alerter->>Alerter: dismiss(id)
end
Class diagram for updated parameter notifications and Alerter stackclassDiagram
class MessageLevel {
<<enumeration>>
Success
Error
Warning
Info
Critical
}
class AlertEntry {
+number id
+MessageLevel level
+string message
}
class Alerter {
+AlertEntry[] alerts
+AlertEntry[] queue
+showAlert(entry AlertEntry) void
+dismiss(id number) void
+promoteFromQueue() void
+startDrain() void
+stopDrain() void
+alertType(level MessageLevel) string
+getTimeout(level MessageLevel) number
}
class MessageManager {
-Array~function~ callbacks
+addCallback(callback function) void
+removeCallback(callback function) void
+emitMessage(level MessageLevel, msg string) void
}
class ParameterFetcher {
-number loaded_params_count
-number total_params_count
-ParametersTable parameter_table
+allParametersLoaded() boolean
+reset() void
+requestParamsWatchdog() void
+handleParamUpdate(param_name string, param_value number, param_index number) void
}
class ParametersTable {
-Record~number, Parameter~ parametersDict
+updateParam(param_name string, param_value number) ChangeInfo
+parameters() Parameter[]
}
class ChangeInfo {
+number oldValue
}
class Parameter {
+number id
+string name
+number value
}
Alerter ..> AlertEntry
Alerter ..> MessageLevel
MessageManager ..> MessageLevel
ParameterFetcher ..> ParametersTable
ParameterFetcher ..> MessageManager
ParameterFetcher ..> MessageLevel
ParametersTable ..> Parameter
ParametersTable ..> ChangeInfo
MessageManager --> Alerter : invokes callbacks
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The Alerter component relies on module-scoped
nextId,boundCallback, anddrainTimer, which can cause interference or incorrect cleanup if multiple instances are ever mounted; consider moving these into component instance state (data/created) so each instance manages its own subscription and timers. - In
Alerter.showAlert, thesetTimeoutcallbacks are never cleared when an alert is dismissed or the component is destroyed, so timeouts may fire after the component is gone; it would be safer to track these timers per-alert and clear them ondismiss/beforeDestroy.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Alerter component relies on module-scoped `nextId`, `boundCallback`, and `drainTimer`, which can cause interference or incorrect cleanup if multiple instances are ever mounted; consider moving these into component instance state (`data`/`created`) so each instance manages its own subscription and timers.
- In `Alerter.showAlert`, the `setTimeout` callbacks are never cleared when an alert is dismissed or the component is destroyed, so timeouts may fire after the component is gone; it would be safer to track these timers per-alert and clear them on `dismiss`/`beforeDestroy`.
## Individual Comments
### Comment 1
<location path="core/frontend/src/components/app/Alerter.vue" line_range="38-47" />
<code_context>
+ message: string
+}
+
+let nextId = 0
+let boundCallback: ((level: MessageLevel, msg: string) => void) | null = null
+let drainTimer: ReturnType<typeof setInterval> | null = null
+
export default Vue.extend({
name: 'ErrorMessage',
data() {
return {
- level: undefined as MessageLevel|undefined,
- message: '',
- show: false,
+ alerts: [] as AlertEntry[],
+ queue: [] as AlertEntry[],
+ }
+ },
+ mounted() {
+ boundCallback = (level: MessageLevel, message: string) => {
+ nextId += 1
+ const entry = { id: nextId, level, message }
+ if (this.alerts.length < MAX_VISIBLE) {
+ this.showAlert(entry)
+ } else {
+ this.queue.push(entry)
+ this.startDrain()
+ }
+ }
+ message_manager.addCallback(boundCallback)
+ },
+ beforeDestroy() {
+ if (boundCallback) {
+ message_manager.removeCallback(boundCallback)
</code_context>
<issue_to_address>
**issue (bug_risk):** Using module-level state for the callback and timer will behave incorrectly if multiple Alerter instances are mounted.
Because `boundCallback` and `drainTimer` are module-scoped, all `Alerter` instances share them. The last-mounted instance overwrites `boundCallback`, so when an earlier instance is destroyed its `beforeDestroy` calls `removeCallback` with the wrong reference. That instance’s callback remains registered, causing leaks and messages targeting a destroyed component.
Store these on the instance instead (e.g. `this.boundCallback`, `this.drainTimer`) so each component registers and cleans up its own callback and timer correctly.
</issue_to_address>
### Comment 2
<location path="core/frontend/src/components/app/Alerter.vue" line_range="93-101" />
<code_context>
+ this.stopDrain()
+ }
+ },
+ startDrain() {
+ if (drainTimer) return
+ drainTimer = setInterval(() => {
+ const evictIdx = this.alerts.findIndex((a) => this.getTimeout(a.level) > 0)
+ if (evictIdx !== -1) {
+ this.dismiss(this.alerts[evictIdx].id)
+ }
+ }, DRAIN_INTERVAL_MS)
+ },
+ stopDrain() {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Queue drain logic can stall permanently when all visible alerts have no timeout.
In `startDrain`, eviction only targets alerts with `getTimeout(level) > 0`. If the visible `alerts` are all non-expiring (e.g. `Error`/`Critical`) and new alerts keep arriving, `evictIdx` stays `-1`, the timer runs indefinitely, the queue grows, and queued alerts never surface.
You may want a fallback (e.g. evict the oldest when the queue is non-empty, even if timeouts are 0), or clearly document that starving queued alerts in this scenario is intentional behavior.
```suggestion
startDrain() {
if (drainTimer) return
drainTimer = setInterval(() => {
const evictIdx = this.alerts.findIndex((a) => this.getTimeout(a.level) > 0)
if (evictIdx !== -1) {
// Prefer to evict alerts that have a timeout configured
this.dismiss(this.alerts[evictIdx].id)
} else if (this.queue.length > 0 && this.alerts.length > 0) {
// Fallback: ensure the queue continues to drain even if all visible alerts are non-expiring
// Evict the oldest visible alert to make room for queued ones
this.dismiss(this.alerts[0].id)
}
}, DRAIN_INTERVAL_MS)
},
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Emit an info-level snackbar when a MAVLink parameter value changes after initial loading is complete, only in developer mode. Refactor Alerter to render a stack of v-alert components so multiple notifications are visible at once, with auto-dismiss timers and a cap of 5 visible alerts. Persistent alerts (error/critical) are preserved when evicting to stay within the cap. Made-with: Cursor
Williangalvani
force-pushed
the
parameter_snackbar
branch
from
b4f5860 to
fd587d0
Compare
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
1 participant
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.
changes snackbar to a stacking-alerters layout, showing up to 5 simultaneously.
Shows ALL parameter changes, but only when in developer mode
Summary by Sourcery
Introduce stacked alert notifications in the frontend and emit developer-mode messages for autopilot parameter changes once all parameters are loaded.
New Features:
Enhancements: