feat: add a 'save' button in peripheral device settings, instead of autosave#1791
Open
jstarpl wants to merge 3 commits into
Open
feat: add a 'save' button in peripheral device settings, instead of autosave#1791jstarpl wants to merge 3 commits into
jstarpl wants to merge 3 commits into
Conversation
… before the config assignment is saved
WalkthroughThis PR adds a staged "pending save" editing workflow to Studio device settings tables (Ingest, Input, Playout sub-devices, and the shared GenericSubDevicesTable) using local unsaved-overrides state, batched vs. instant override helpers, and updatedIds tracking with explicit Save/Discard controls. ParentDevices gains an analogous staged assignment flow. Unrelated Upgrades button restyling is included. ChangesStaged Save/Discard editing workflow
Upgrades Fix Up buttons restyle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SubDeviceEditRow
participant GenericSubDevicesTable
participant BatchedOverrideHelper
participant StudioDocument
User->>SubDeviceEditRow: edit device ID
SubDeviceEditRow->>GenericSubDevicesTable: updateObjectId(oldId, newId)
GenericSubDevicesTable->>BatchedOverrideHelper: changeItemId(...).commit()
User->>GenericSubDevicesTable: click Save
GenericSubDevicesTable->>StudioDocument: saveChanges() persists overrides
StudioDocument-->>GenericSubDevicesTable: updatedIds cleared
sequenceDiagram
participant User
participant ParentDeviceEditRow
participant StudioParentDevices
participant Studios
participant MeteorCall
User->>ParentDeviceEditRow: select peripheral device
ParentDeviceEditRow->>StudioParentDevices: changeAssignment(configId, deviceId)
User->>ParentDeviceEditRow: click Save
StudioParentDevices->>Studios: update(overrides)
StudioParentDevices->>MeteorCall: assignConfigToPeripheralDevice(...)
MeteorCall-->>StudioParentDevices: clear unsaved state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
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 (2)
packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx (1)
74-97: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStage sub-device deletes instead of instant-saving them
instantSaveOverrideHelper()here writes the currentsettingsWithOverridesarray back to the DB, so a delete while other edits are staged persists those pending ops too and leaves the unsaved state able to restore the item on Save. Route this throughoverrideHelper()and drop the instant helper from this path.🤖 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 `@packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx` around lines 74 - 97, The sub-device removal flow in confirmRemove is using instantSaveOverrideHelper(), which immediately writes the full settings state and can accidentally persist other staged edits; switch this delete path to use overrideHelper() so the removal is only staged, and remove the instant-save helper from the useCallback dependencies and onAccept path.packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx (1)
82-106: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStage the new ingest device instead of
$pushing it directly. WhenunsavedOverridesis set, this bypasses the staged edit flow and the new item can stay hidden behind the pending overrides until save/discard. Also addstudioIdto the callback deps and dropsettingsWithOverrides.overrides, which isn’t used here.🤖 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 `@packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx` around lines 82 - 106, The addNewItem callback in IngestSubDevices is bypassing the staged edit flow by calling Studios.update with a direct $push, which can leave the new ingest device hidden when unsavedOverrides exists. Update the logic to stage the new device through the same pending-override mechanism used elsewhere in this component instead of writing directly, and ensure the callback includes studioId in its dependency list while removing the unused settingsWithOverrides.overrides dependency. Use addNewItem, Studios.update, and wrappedSubDevices as the key locations to adjust.
🧹 Nitpick comments (2)
packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx (1)
300-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
handleUpdateIdomitseditItemWithIdfrom its dependency array.It's called inside the callback but not listed in deps
[item.id, updateObjectId]. IfeditItemWithIdisn't stably memoized this captures a stale closure, and it will otherwise tripreact-hooks/exhaustive-deps.♻️ Proposed fix
- [item.id, updateObjectId] + [item.id, updateObjectId, editItemWithId]🤖 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 `@packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx` around lines 300 - 309, The `handleUpdateId` callback in `GenericSubDevices` is missing `editItemWithId` from its dependency array, which can leave a stale closure and violate the hooks lint rule. Update the `useCallback` dependencies for `handleUpdateId` to include every referenced value used inside the callback, especially `editItemWithId`, alongside `item.id` and `updateObjectId`.packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx (1)
126-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
saveChangesreadsupdatedIdsbut omits it from deps.The dependency array is
[studio?._id, unsavedOverrides];updatedIdsis used to decide the reset and can be stale. Add it (also satisfiesexhaustive-deps).♻️ Proposed fix
- }, [studio?._id, unsavedOverrides]) + }, [studio?._id, unsavedOverrides, updatedIds])🤖 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 `@packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx` around lines 126 - 139, The saveChanges callback in IngestSubDevices reads updatedIds but does not list it in the useCallback dependency array, which can leave the reset logic stale. Update the dependency list for saveChanges to include updatedIds alongside studio?._id and unsavedOverrides so the callback stays in sync and satisfies exhaustive-deps.
🤖 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 `@packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx`:
- Around line 335-341: The shared edit state in ParentDevices is being wired
into each row’s Save/Discard controls, so any expanded row can act on all
pending changes. Update the row rendering in ParentDevices/ParentDeviceRow usage
so Save/Discard are not repeated per row; either lift those actions out into a
single global toolbar for the whole table, or scope hasUnsavedChanges,
saveChanges, discardChanges, and currentAssignment to the specific row/item
being edited instead of using the shared unsavedOverrides/unsavedAssignments
state across all rows.
---
Outside diff comments:
In `@packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx`:
- Around line 74-97: The sub-device removal flow in confirmRemove is using
instantSaveOverrideHelper(), which immediately writes the full settings state
and can accidentally persist other staged edits; switch this delete path to use
overrideHelper() so the removal is only staged, and remove the instant-save
helper from the useCallback dependencies and onAccept path.
In `@packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx`:
- Around line 82-106: The addNewItem callback in IngestSubDevices is bypassing
the staged edit flow by calling Studios.update with a direct $push, which can
leave the new ingest device hidden when unsavedOverrides exists. Update the
logic to stage the new device through the same pending-override mechanism used
elsewhere in this component instead of writing directly, and ensure the callback
includes studioId in its dependency list while removing the unused
settingsWithOverrides.overrides dependency. Use addNewItem, Studios.update, and
wrappedSubDevices as the key locations to adjust.
---
Nitpick comments:
In `@packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx`:
- Around line 300-309: The `handleUpdateId` callback in `GenericSubDevices` is
missing `editItemWithId` from its dependency array, which can leave a stale
closure and violate the hooks lint rule. Update the `useCallback` dependencies
for `handleUpdateId` to include every referenced value used inside the callback,
especially `editItemWithId`, alongside `item.id` and `updateObjectId`.
In `@packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx`:
- Around line 126-139: The saveChanges callback in IngestSubDevices reads
updatedIds but does not list it in the useCallback dependency array, which can
leave the reset logic stale. Update the dependency list for saveChanges to
include updatedIds alongside studio?._id and unsavedOverrides so the callback
stays in sync and satisfies exhaustive-deps.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c94d86ca-fa6d-4518-912f-7c8aecb1a8cd
📒 Files selected for processing (6)
packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsxpackages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsxpackages/webui/src/client/ui/Settings/Studio/Devices/InputSubDevices.tsxpackages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsxpackages/webui/src/client/ui/Settings/Studio/Devices/PlayoutSubDevices.tsxpackages/webui/src/client/ui/Settings/Upgrades/Components.tsx
Comment on lines
+335
to
+341
| hasUnsavedChanges={hasUnsavedChanges} | ||
| saveChanges={saveChanges} | ||
| discardChanges={discardChanges} | ||
| currentAssignment={ | ||
| item.id in unsavedAssignments ? unsavedAssignments[item.id] : peripheralDevice?._id | ||
| } | ||
| changeAssignment={changeAssignment} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and surrounding symbols.
ast-grep outline packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx --view expanded || true
# Read the relevant sections around the reported lines and the helper functions.
sed -n '120,220p' packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx | cat -n
printf '\n----\n'
sed -n '300,380p' packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx | cat -n
printf '\n----\n'
sed -n '500,560p' packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx | cat -n
# Find all uses of hasUnsavedChanges, discardChanges, saveChanges, and ParentDeviceEditRow.
rg -n "hasUnsavedChanges|discardChanges|saveChanges|ParentDeviceEditRow" packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsxRepository: Sofie-Automation/sofie-core
Length of output: 13511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the save/discard implementation and nearby state.
sed -n '84,116p' packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx | cat -n
printf '\n----\n'
sed -n '116,160p' packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx | cat -n
# Locate the expansion helper definition.
rg -n "function useToggleExpandHelper|const useToggleExpandHelper|useToggleExpandHelper" packages/webui/src/client/ui -g '!**/dist/**' -g '!**/build/**'Repository: Sofie-Automation/sofie-core
Length of output: 6246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/webui/src/client/ui/util/useToggleExpandHelper.tsx | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 1627
Avoid repeating shared Save/Discard controls per row
hasUnsavedChanges is shared across the whole table, so any expanded row shows Save/Discard as soon as one edit exists. Since discardChanges clears both unsavedOverrides and unsavedAssignments, clicking Discard in any row drops edits from every row, not just the one being edited. If this is meant to be one global edit session, move the actions out of each row; otherwise scope dirty state and discard to the current row.
🤖 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 `@packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx`
around lines 335 - 341, The shared edit state in ParentDevices is being wired
into each row’s Save/Discard controls, so any expanded row can act on all
pending changes. Update the row rendering in ParentDevices/ParentDeviceRow usage
so Save/Discard are not repeated per row; either lift those actions out into a
single global toolbar for the whole table, or scope hasUnsavedChanges,
saveChanges, discardChanges, and currentAssignment to the specific row/item
being edited instead of using the shared unsavedOverrides/unsavedAssignments
state across all rows.
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.
About the Contributor
This pull request is posted on behalf of the NRK.
Type of Contribution
This is a:
Feature
Current Behavior
Peripheral device settings are autosaved on every input edit, potentially causing problems on air
New Behavior
Peripheral device settings are only saved upon clicking a "save" button
Testing
Affected areas
This PR affects the Peripheral Device configuration UI
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
This PR subsumes #1572 and adds some bugfixes/improvements. We like this feature and would like to see it merged, but the original PR has now become stale.
Status