feat(tempgroups): add user-organizable temperature tabs#2434
feat(tempgroups): add user-organizable temperature tabs#2434Ragilo wants to merge 3 commits intomainsail-crew:developfrom
Conversation
Adds ability to organize temperature sensors into custom groups (tabs). Each tab shows its own sensor list and filtered temperature graph. Features: - Create, edit, and delete temperature groups - Drag & drop to reorder groups and sensors within groups - Optional "All" tab showing all sensors (like before) - Custom display names for sensors (solves Cartographer naming issue) - Per-group chart visibility toggle - Graph Y-axis scales appropriately for filtered sensors - Full backwards compatibility (no tabs shown without groups) Closes mainsail-crew#2376 Signed-off-by: Christopher Hesse <christopher.hesse@alphaflow.gmbh>
📝 WalkthroughWalkthroughAdds a new "tempgroups" feature: settings UI for creating/reordering sensor groups, a namespaced Vuex tempgroups module to persist groups and active selection, and UI changes that render tabbed temperature panels filtering lists and charts by the active group. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Tabs as TemperaturePanelTabs
participant Store as Vuex Store
participant Panel as TemperaturePanel
participant List as TemperaturePanelList
participant Chart as TempChart
User->>Tabs: select group tab
Tabs->>Store: dispatch setActiveGroup(groupId)
Store->>Store: update activeGroupId
Panel->>Store: read getActiveSensorFilter()
Panel->>List: pass sensorFilter
Panel->>Chart: pass sensorFilter
List->>List: compute filtered lists
List->>User: render filtered sensor list
Chart->>Chart: compute filteredSeries from sensorFilter
Chart->>User: render filtered chart
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
🤖 Fix all issues with AI agents
In `@src/components/settings/SettingsTempgroupsTabEdit.vue`:
- Around line 200-212: The debounced updater dispatches invalid names because
updateGroupNameDebounced(newVal: string) is called unconditionally from the
groupName setter; change updateGroupNameDebounced to perform the same validation
checks as the component's rules (e.g., non-empty, uniqueness against existing
temp groups, and any length/format checks) before calling this.$store.dispatch,
and also ensure it returns early if validation fails or if newVal ===
this.localGroupName; reference the groupName setter, updateGroupNameDebounced,
this.localGroupName and the component's validation rules when implementing the
guard.
In `@src/store/gui/tempgroups/actions.ts`:
- Around line 73-82: addSensorToGroup currently creates a GuiTempgroupSensor
with pos: 0 which causes duplicate positions; instead, read the group's existing
sensors from the action context (use the destructured state or state.groups /
groupStore equivalent) to compute the next position as (max existing sensor.pos
+ 1) or 0 if none, set sensor.pos to that computed nextPos before calling
commit('addSensorToGroup', { id, sensor }) and then dispatch('groupUpload', id);
reference the addSensorToGroup action, GuiTempgroupSensor shape, the
'addSensorToGroup' mutation and dispatch to 'groupUpload' when making the
change.
🧹 Nitpick comments (10)
src/store/gui/tempgroups/actions.ts (2)
54-55:awaiton a synchronous Vuexcommitis misleading.
commitis synchronous in Vuex and does not return a Promise. Theawaiton line 54 is a no-op and suggests asynchrony that doesn't exist. Only thedispatchon line 55 is actually async.Suggested fix
- await commit('groupStore', { id, values: group }) - await dispatch('groupUpload', id) + commit('groupStore', { id, values: group }) + await dispatch('groupUpload', id)
107-112: Group reorder triggers N individual socket calls.
reorderGroupsdispatchesgroupUploadfor each group, causing one socket emit per group. This works but is chatty. Not a problem for a handful of groups, but worth noting if the number could grow.src/locales/en.json (1)
1385-1408: Overlapping locale keys betweenSettings.TempgroupsTabandTempgroups.Several keys are duplicated across these two sections with inconsistent casing (e.g.,
"Available Sensors"vs"Available sensors","add group"vs"Add Group"). While having separate sections for settings vs. panel context is a valid pattern in this codebase, consider aligning the casing for shared concepts to maintain consistency, or consolidating keys that are truly identical.Also applies to: 1609-1623
src/store/gui/tempgroups/types.ts (1)
9-22: Inconsistent naming:positioninGuiTempgroupvsposinGuiTempgroupSensor.Both fields serve the same purpose (ordering), but use different names. Consider aligning them (e.g., both
positionor bothpos) for consistency.src/store/gui/tempgroups/mutations.ts (2)
62-73: Minor: shallow-copied array still mutates original sensor objects in-place.
sensorsis a shallow copy of the array, butsensor[payload.option] = payload.value(Line 69) mutates the original sensor object reference beforeVue.setreplaces the array. While this works becauseVue.settriggers reactivity on thesensorskey, it's not purely immutable. Consider spreading the sensor object too for consistency withgroupUpdate:Suggested improvement
if (sensorIndex !== -1) { - const sensor = sensors[sensorIndex] - // `@ts-ignore` - sensor[payload.option] = payload.value + sensors[sensorIndex] = { + ...sensors[sensorIndex], + [payload.option]: payload.value, + } Vue.set(state.groups[payload.id], 'sensors', sensors) }This also eliminates the
@ts-ignore.
41-51: Minor:payload.sensor.posis mutated on the caller's object.Line 46 mutates
payload.sensor.posdirectly, which modifies the object the caller passed in. Consider creating a new sensor object instead:Suggested improvement
if (group) { const sensors = [...(group.sensors ?? [])] const maxPos = sensors.reduce((max, s) => Math.max(max, s.pos), -1) - payload.sensor.pos = maxPos + 1 - sensors.push(payload.sensor) + sensors.push({ ...payload.sensor, pos: maxPos + 1 }) Vue.set(state.groups[payload.id], 'sensors', sensors) }src/components/settings/SettingsTempgroupsTab.vue (1)
134-136: Consider adding a confirmation dialog before deleting a group.
deleteGroupimmediately dispatches the delete action with no user confirmation. If a group contains many carefully configured sensors, accidental deletion could be frustrating. A simple confirmation prompt would be a low-effort UX safeguard.src/components/settings/SettingsTempgroupsTabEdit.vue (3)
173-173:searchSensorscan benullat runtime due to Vuetifyclearable.When the user clicks the clear icon on a Vuetify
v-text-fieldwithclearable, the model is set tonull. The type annotation saysstring, but the actual runtime value can benull. The guard on line 260 (!this.searchSensors) happens to cover this case, so there's no runtime crash, but the type is inaccurate.Proposed fix
-private searchSensors: string = '' +private searchSensors: string | null = ''
146-160: Class component structure and mixin usage look correct.The component correctly uses
@Componentdecorator,Mixins(BaseMixin, ThemeMixin), and@Propwith the right configuration for Vue 2 class components. One minor note: line 148 uses a relative import path ('../mixins/base') while line 149 uses an alias path ('@/components/mixins/theme'). Both work, but using the alias consistently would be slightly cleaner.
235-249: Consider using aSetfor deduplication of sensors.The
allSensorscomputed builds a list by pushing heaters first, then conditionally adding sensors not already present. Theincludescheck is O(n) per sensor, making this O(n²). For typical printer setups this is fine, but aSetwould be both cleaner and O(n) overall.Proposed refactor
get allSensors(): string[] { - const sensors: string[] = [] - - const heaters = this.$store.state.printer?.heaters?.available_heaters || [] - sensors.push(...heaters) - - const tempSensors = this.$store.state.printer?.heaters?.available_sensors || [] - tempSensors.forEach((sensor: string) => { - if (!sensors.includes(sensor)) { - sensors.push(sensor) - } - }) - - return sensors + const heaters: string[] = this.$store.state.printer?.heaters?.available_heaters || [] + const tempSensors: string[] = this.$store.state.printer?.heaters?.available_sensors || [] + return [...new Set([...heaters, ...tempSensors])] }
| set groupName(newVal: string) { | ||
| this.localGroupName = newVal | ||
| this.updateGroupNameDebounced(newVal) | ||
| } | ||
|
|
||
| @Debounce(250) | ||
| updateGroupNameDebounced(newVal: string) { | ||
| if (!this.groupId) return | ||
| this.$store.dispatch('gui/tempgroups/groupUpdate', { | ||
| id: this.groupId, | ||
| values: { name: newVal }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Debounced store update bypasses validation — invalid names get persisted.
The groupName setter dispatches updateGroupNameDebounced on every keystroke (after debounce), regardless of whether the rules pass. If the user clears the field or types a duplicate name, the Vuetify text field correctly shows the error, but the store is still updated with the invalid value after 250ms.
Guard the dispatch with the same checks used in the rules:
Proposed fix
`@Debounce`(250)
updateGroupNameDebounced(newVal: string) {
if (!this.groupId) return
+ if (newVal === '' || this.existsGroupName(newVal)) return
this.$store.dispatch('gui/tempgroups/groupUpdate', {
id: this.groupId,
values: { name: newVal },
})
}📝 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.
| set groupName(newVal: string) { | |
| this.localGroupName = newVal | |
| this.updateGroupNameDebounced(newVal) | |
| } | |
| @Debounce(250) | |
| updateGroupNameDebounced(newVal: string) { | |
| if (!this.groupId) return | |
| this.$store.dispatch('gui/tempgroups/groupUpdate', { | |
| id: this.groupId, | |
| values: { name: newVal }, | |
| }) | |
| } | |
| set groupName(newVal: string) { | |
| this.localGroupName = newVal | |
| this.updateGroupNameDebounced(newVal) | |
| } | |
| `@Debounce`(250) | |
| updateGroupNameDebounced(newVal: string) { | |
| if (!this.groupId) return | |
| if (newVal === '' || this.existsGroupName(newVal)) return | |
| this.$store.dispatch('gui/tempgroups/groupUpdate', { | |
| id: this.groupId, | |
| values: { name: newVal }, | |
| }) | |
| } |
🤖 Prompt for AI Agents
In `@src/components/settings/SettingsTempgroupsTabEdit.vue` around lines 200 -
212, The debounced updater dispatches invalid names because
updateGroupNameDebounced(newVal: string) is called unconditionally from the
groupName setter; change updateGroupNameDebounced to perform the same validation
checks as the component's rules (e.g., non-empty, uniqueness against existing
temp groups, and any length/format checks) before calling this.$store.dispatch,
and also ensure it returns early if validation fails or if newVal ===
this.localGroupName; reference the groupName setter, updateGroupNameDebounced,
this.localGroupName and the component's validation rules when implementing the
guard.
| addSensorToGroup({ commit, dispatch }, payload: { id: string; sensorName: string; displayName?: string }) { | ||
| const sensor: GuiTempgroupSensor = { | ||
| pos: 0, | ||
| name: payload.sensorName, | ||
| displayName: payload.displayName, | ||
| } | ||
|
|
||
| commit('addSensorToGroup', { id: payload.id, sensor }) | ||
| dispatch('groupUpload', payload.id) | ||
| }, |
There was a problem hiding this comment.
New sensors always get pos: 0, which may cause ambiguous ordering.
Every sensor added via addSensorToGroup gets pos: 0 regardless of how many sensors already exist in the group. If pos is used for sorting, multiple sensors will share the same position until explicitly reordered.
Consider computing the next position similarly to how groupStore computes maxPosition:
Suggested fix
addSensorToGroup({ commit, dispatch }, payload: { id: string; sensorName: string; displayName?: string }) {
+ const group = (this as any).state.gui.tempgroups.groups[payload.id]
+ const maxPos = group?.sensors?.reduce((max: number, s: GuiTempgroupSensor) => Math.max(max, s.pos), -1) ?? -1
const sensor: GuiTempgroupSensor = {
- pos: 0,
+ pos: maxPos + 1,
name: payload.sensorName,
displayName: payload.displayName,
}Alternatively, you could use state from the action context:
Using destructured state
-addSensorToGroup({ commit, dispatch }, payload: { id: string; sensorName: string; displayName?: string }) {
+addSensorToGroup({ commit, dispatch, state }, payload: { id: string; sensorName: string; displayName?: string }) {
+ const existingSensors = state.groups[payload.id]?.sensors ?? []
+ const maxPos = existingSensors.reduce((max, s) => Math.max(max, s.pos), -1)
const sensor: GuiTempgroupSensor = {
- pos: 0,
+ pos: maxPos + 1,
name: payload.sensorName,
displayName: payload.displayName,
}📝 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.
| addSensorToGroup({ commit, dispatch }, payload: { id: string; sensorName: string; displayName?: string }) { | |
| const sensor: GuiTempgroupSensor = { | |
| pos: 0, | |
| name: payload.sensorName, | |
| displayName: payload.displayName, | |
| } | |
| commit('addSensorToGroup', { id: payload.id, sensor }) | |
| dispatch('groupUpload', payload.id) | |
| }, | |
| addSensorToGroup({ commit, dispatch, state }, payload: { id: string; sensorName: string; displayName?: string }) { | |
| const existingSensors = state.groups[payload.id]?.sensors ?? [] | |
| const maxPos = existingSensors.reduce((max, s) => Math.max(max, s.pos), -1) | |
| const sensor: GuiTempgroupSensor = { | |
| pos: maxPos + 1, | |
| name: payload.sensorName, | |
| displayName: payload.displayName, | |
| } | |
| commit('addSensorToGroup', { id: payload.id, sensor }) | |
| dispatch('groupUpload', payload.id) | |
| }, |
🤖 Prompt for AI Agents
In `@src/store/gui/tempgroups/actions.ts` around lines 73 - 82, addSensorToGroup
currently creates a GuiTempgroupSensor with pos: 0 which causes duplicate
positions; instead, read the group's existing sensors from the action context
(use the destructured state or state.groups / groupStore equivalent) to compute
the next position as (max existing sensor.pos + 1) or 0 if none, set sensor.pos
to that computed nextPos before calling commit('addSensorToGroup', { id, sensor
}) and then dispatch('groupUpload', id); reference the addSensorToGroup action,
GuiTempgroupSensor shape, the 'addSensorToGroup' mutation and dispatch to
'groupUpload' when making the change.
Adds validation to the debounced name update method to prevent empty or duplicate names from being saved to the store. Signed-off-by: Stefan Dej <meteyou@gmail.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unnecessary await on synchronous commit call - Use immutable pattern in mutations (spread instead of mutating payload) - Fix TypeScript type for searchSensors (can be null from Vuetify clearable) Signed-off-by: Stefan Dej <meteyou@gmail.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/settings/SettingsTempgroupsTabEdit.vue`:
- Around line 184-187: The onGroupChanged watcher is overwriting the local
editing buffer (localGroupName) whenever the store's group object reference
changes; update it to only sync when the external name actually differs from the
local buffer (e.g., compare newVal?.name with this.localGroupName and return
without assigning if equal) or replace the watcher by initializing
localGroupName once from groupId (or in created/mounted) so subsequent store
mutations don't clobber in-flight edits; locate the watcher method
onGroupChanged and either add the name-equals check before setting
this.localGroupName or remove the watcher and seed localGroupName from the
groupId computed/prop on component mount.
🧹 Nitpick comments (4)
src/store/gui/tempgroups/mutations.ts (2)
75-93:removeSensorFromGroupdirectly mutates sensor objects in state, inconsistent with immutable pattern elsewhere.Line 87 mutates
s.poson the original sensor references (the spread on line 78 is a shallow array copy, not a deep copy). Compare withupdateSensorInGroup(line 66-69) andreorderSensorsInGroup(line 98), which both create new sensor objects via spread. Not a bug since we're inside a mutation, but for consistency with the stated intent ("Use immutable pattern in mutations"):Suggested fix
- sensors - .filter((s) => s.pos > oldPos) - .forEach((s) => { - s.pos = s.pos - 1 - }) + const updatedSensors = sensors.map((s) => + s.pos > oldPos ? { ...s, pos: s.pos - 1 } : s + ) + sensors.splice(0, sensors.length, ...updatedSensors)Or more simply, replace the entire block (lines 78–92) with a single immutable pipeline:
- const sensors = [...(group.sensors ?? [])] - const deletedIndex = sensors.findIndex((s) => s.name === payload.sensorName) - if (deletedIndex !== -1) { - const oldPos = sensors[deletedIndex].pos - sensors.splice(deletedIndex, 1) - - sensors - .filter((s) => s.pos > oldPos) - .forEach((s) => { - s.pos = s.pos - 1 - }) - } - - Vue.set(state.groups[payload.id], 'sensors', sensors) + const target = (group.sensors ?? []).find((s) => s.name === payload.sensorName) + if (!target) return + + const sensors = (group.sensors ?? []) + .filter((s) => s.name !== payload.sensorName) + .map((s) => (s.pos > target.pos ? { ...s, pos: s.pos - 1 } : { ...s })) + + Vue.set(state.groups[payload.id], 'sensors', sensors)
103-109: Silent skip for unknown IDs could mask bugs.If
orderedIdscontains an ID not present instate.groups, it's silently ignored. This is safe for now, but if groups are deleted concurrently (e.g., via another browser tab syncing to moonraker), the remaining groups could end up with non-contiguous or duplicatepositionvalues. Consider logging or filtering beforehand if this becomes an issue.src/components/settings/SettingsTempgroupsTabEdit.vue (2)
270-286:existsGroupNameis case-sensitive — intentional?
group.name === namemeans "Chamber" and "chamber" would both be allowed. If Klipper or the UI treats these as visually distinct that's fine, but it could confuse users. Worth a brief confirmation.
236-250: Use printer store getters instead of directly accessing state.While there's no single getter that returns the combined list of heaters and sensors, the printer store exposes
getAvailableHeatersandgetAvailableSensorsgetters (atsrc/store/printer/getters.ts). Using these would decouple the component from the state shape and centralize the dedup logic. Alternatively, consider whether the tempgroupsgetUngroupedSensorsgetter (which already combines and deduplicates heaters + sensors) could be reused or adapted for this use case.
| @Watch('group', { immediate: true }) | ||
| onGroupChanged(newVal: GuiTempgroup | null) { | ||
| this.localGroupName = newVal?.name ?? '' | ||
| } |
There was a problem hiding this comment.
onGroupChanged watcher resets localGroupName on any group mutation, discarding in-flight edits.
When the user types a new group name, localGroupName is updated and a 250ms debounce starts. If showChart (or anything else) is toggled before the debounce fires, the resulting groupUpdate dispatch replaces the store's group object → group computed returns a new reference → this watcher fires → localGroupName is overwritten with the old store name, losing the user's unsaved keystrokes.
Suggested fix — only sync when the name actually changed externally
`@Watch`('group', { immediate: true })
onGroupChanged(newVal: GuiTempgroup | null) {
- this.localGroupName = newVal?.name ?? ''
+ const storeName = newVal?.name ?? ''
+ // Only overwrite localGroupName when the store name genuinely differs
+ // from what we last dispatched, to avoid clobbering in-flight edits
+ if (storeName !== this.localGroupName && storeName !== '') {
+ this.localGroupName = storeName
+ }
}A cleaner alternative is to drop this watcher entirely and seed localGroupName once in a @Watch('groupId', { immediate: true }) handler (or in created/mounted), so subsequent store-side mutations never interfere with the local editing buffer.
🤖 Prompt for AI Agents
In `@src/components/settings/SettingsTempgroupsTabEdit.vue` around lines 184 -
187, The onGroupChanged watcher is overwriting the local editing buffer
(localGroupName) whenever the store's group object reference changes; update it
to only sync when the external name actually differs from the local buffer
(e.g., compare newVal?.name with this.localGroupName and return without
assigning if equal) or replace the watcher by initializing localGroupName once
from groupId (or in created/mounted) so subsequent store mutations don't clobber
in-flight edits; locate the watcher method onGroupChanged and either add the
name-equals check before setting this.localGroupName or remove the watcher and
seed localGroupName from the groupId computed/prop on component mount.
1 participant
Summary
Adds ability to organize temperature sensors into custom groups (tabs), addressing the issue of cluttered temperature panels with many sensors.
Screenshots
Test plan
Closes #2376