feat: add a 'save' button in peripheral device settings, instead of autosave#1572
feat: add a 'save' button in peripheral device settings, instead of autosave#1572
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
anteeek
force-pushed
the
feat/SOFIE-40
branch
from
94a1a8f to
2f87e4f
Compare
|
Saftret
added
the
Contribution from SuperFly.tv
anteeek
changed the title
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThe pull request introduces a batched override management system across device settings components. Changes enable users to make multiple edits to device configurations before committing to the backend, with state tracking for unsaved overrides and ID changes, plus save/discard handlers throughout the component hierarchy. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Edit UI Component
participant State as Local State Manager
participant Backend as Database/Backend
User->>UI: Make changes (e.g., edit ID, settings)
activate UI
UI->>State: Update unsavedOverrides via batchedOverrideHelper
State->>State: Store staged changes in memory
State->>UI: Reflect updates (show Save/Discard buttons)
deactivate UI
User->>UI: Click Save or Discard
activate UI
alt Save Changes
UI->>State: Invoke saveChanges()
State->>Backend: Persist unsavedOverrides via Studios.update/MeteorCall
Backend->>Backend: Commit changes to database
State->>State: Clear unsavedOverrides & updatedIds
State->>UI: Update UI (hide Save/Discard, show edit controls)
else Discard Changes
UI->>State: Invoke discardChanges()
State->>State: Reset unsavedOverrides & updatedIds to empty
State->>UI: Revert UI to last-saved state
end
deactivate UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
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/InputSubDevices.tsx (1)
76-100: MissingstudioIdin dependency array.The
addNewItemcallback usesstudioIdin theStudios.updatecall but it's not included in the dependency array. Additionally,settingsWithOverrides.overridesis in the dependency array but isn't used in the callback.Proposed fix
const addNewItem = useCallback(() => { const existingDevices = new Set(wrappedSubDevices.map((d) => d.id)) let idx = 0 while (existingDevices.has(`device${idx}`)) { idx++ } const newId = `device${idx}` const newDevice = literal<StudioInputDevice>({ peripheralDeviceId: undefined, options: {}, }) const addOp = literal<ObjectOverrideSetOp>({ op: 'set', path: newId, value: newDevice, }) Studios.update(studioId, { $push: { 'peripheralDeviceSettings.inputDevices.overrides': addOp, }, }) - }, [wrappedSubDevices, settingsWithOverrides.overrides]) + }, [studioId, wrappedSubDevices])packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx (1)
79-103: MissingstudioIdin dependency array.Same issue as in
InputSubDevices.tsx— the callback usesstudioIdbut it's not in the dependency array.Proposed fix
- }, [wrappedSubDevices, settingsWithOverrides.overrides]) + }, [studioId, wrappedSubDevices])
🤖 Fix all issues with AI agents
In @packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx:
- Around line 300-309: handleUpdateId captures editItemWithId but the hook
dependency array only lists [item.id, updateObjectId], risking stale closures;
update the useCallback dependencies to include editItemWithId (i.e., change the
dependency array for handleUpdateId to [item.id, updateObjectId,
editItemWithId]) so the callback updates when editItemWithId changes.
In @packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx:
- Around line 122-135: The saveChanges callback is missing updatedIds in its
dependency array, which can cause stale closures; update the useCallback
declaration for saveChanges to include updatedIds (and setUpdatedIds if not
already stable) in its dependency array so the hook rebinds when updatedIds
changes; locate the saveChanges function and modify its dependency list to
include updatedIds (and ensure setUpdatedIds is referenced correctly) so the
reset logic using setUpdatedIds(new Map<string, string>()) runs with the latest
state.
- Line 153: Compute and pass a memoized hasUnsavedChanges that includes both
unsavedOverrides and updatedIds like the other components: create a useMemo that
returns (!!unsavedOverrides || updatedIds.size > 0) and use that value for the
hasUnsavedChanges prop instead of the current !!unsavedOverrides; reference the
existing updatedIds and unsavedOverrides variables and the hasUnsavedChanges
prop in the component so Save/Discard appear when an ID was renamed.
In @packages/webui/src/client/ui/Settings/Studio/Devices/InputSubDevices.tsx:
- Around line 119-132: The saveChanges callback references updatedIds.size but
updatedIds is not included in its dependency array, which can cause stale reads;
update the dependency array for saveChanges to include updatedIds (and keep
setUpdatedIds and other existing deps like studio?._id and unsavedOverrides) so
the hook correctly re-registers when updatedIds changes; locate the saveChanges
function and add updatedIds to its useCallback dependency list.
In @packages/webui/src/client/ui/Settings/Studio/Devices/PlayoutSubDevices.tsx:
- Around line 125-138: The saveChanges callback uses updatedIds.size but
updatedIds is not included in the dependency array, which can produce a stale
closure; update the dependency array for saveChanges to include updatedIds (and
keep studio?._id and unsavedOverrides) so the callback sees the current Map,
ensuring the branch that checks updatedIds.size runs correctly and that
setUpdatedIds(new Map<string,string>()) resets state as intended; locate the
saveChanges function and modify its dependency array to include updatedIds.
🧹 Nitpick comments (1)
packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx (1)
114-123: Consider surfacing assignment save failures to the user.The
Promise.all(...).catch()only logs toconsole.error. If any assignment fails, the user won't receive feedback, andunsavedAssignmentsis cleared regardless of success or failure. Consider adding user-facing error notification.Proposed improvement sketch
if (Object.keys(unsavedAssignments).length > 0) { Promise.all( Object.entries<PeripheralDeviceId | undefined>(unsavedAssignments).map(async ([configId, deviceId]) => { return MeteorCall.studio.assignConfigToPeripheralDevice(studioId, configId, deviceId ?? null) }) - ).catch((e) => { + ).then(() => { + setUnsavedAssignments({}) + }).catch((e) => { console.error('Failed to save assignments', e) + // TODO: Show user-facing error notification }) - setUnsavedAssignments({}) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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.tsx
🧰 Additional context used
🧬 Code graph analysis (5)
packages/webui/src/client/ui/Settings/Studio/Devices/InputSubDevices.tsx (3)
packages/corelib/src/settings/objectWithOverrides.ts (2)
SomeObjectOverrideOp(19-19)wrapDefaultObject(48-53)packages/webui/src/client/ui/Settings/util/OverrideOpHelper.tsx (1)
useOverrideOpHelper(14-29)packages/corelib/src/dataModel/Studio.ts (1)
StudioInputDevice(113-121)
packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx (3)
packages/corelib/src/settings/objectWithOverrides.ts (2)
SomeObjectOverrideOp(19-19)wrapDefaultObject(48-53)packages/webui/src/client/ui/Settings/util/OverrideOpHelper.tsx (1)
useOverrideOpHelper(14-29)packages/corelib/src/dataModel/Studio.ts (1)
StudioIngestDevice(103-111)
packages/webui/src/client/ui/Settings/Studio/Devices/PlayoutSubDevices.tsx (3)
packages/corelib/src/settings/objectWithOverrides.ts (1)
SomeObjectOverrideOp(19-19)packages/webui/src/client/ui/Settings/util/OverrideOpHelper.tsx (1)
useOverrideOpHelper(14-29)packages/corelib/src/dataModel/Studio.ts (1)
StudioPlayoutDevice(123-131)
packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx (2)
packages/corelib/src/overrideOpHelper.ts (1)
OverrideOpHelper(174-174)packages/webui/src/client/lib/Components/TextInput.tsx (1)
TextInputControl(27-121)
packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx (4)
packages/corelib/src/settings/objectWithOverrides.ts (2)
SomeObjectOverrideOp(19-19)wrapDefaultObject(48-53)packages/corelib/src/dataModel/Studio.ts (1)
StudioDeviceSettings(133-140)packages/webui/src/client/ui/Settings/util/OverrideOpHelper.tsx (1)
useOverrideOpHelper(14-29)packages/webui/src/client/lib/Components/DropdownInput.tsx (1)
DropdownInputOption(5-9)
🔇 Additional comments (4)
packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx (1)
344-360: LGTM!The conditional rendering for Save/Discard buttons vs. the Confirm button is well structured. The UI correctly surfaces unsaved changes state and provides clear action buttons.
packages/webui/src/client/ui/Settings/Studio/Devices/PlayoutSubDevices.tsx (1)
53-73: LGTM!The batched override pattern is correctly implemented. The
deviceSettingsmemo properly layersunsavedOverrideson top ofbaseSettings, andwrappedSubDevicescorrectly depends on the merged settings.packages/webui/src/client/ui/Settings/Studio/Devices/ParentDevices.tsx (2)
47-62: LGTM!The state management for
unsavedOverridesandunsavedAssignmentsis well structured. ThedeviceSettingsmemo correctly merges unsaved changes with the base settings.
491-507: LGTM!The conditional Save/Discard UI in
ParentDeviceEditRowfollows the same pattern as the other device edit rows and provides a consistent user experience.
packages/webui/src/client/ui/Settings/Studio/Devices/GenericSubDevices.tsx
Show resolved
Hide resolved
packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx
Show resolved
Hide resolved
packages/webui/src/client/ui/Settings/Studio/Devices/IngestSubDevices.tsx
Show resolved
Hide resolved
packages/webui/src/client/ui/Settings/Studio/Devices/InputSubDevices.tsx
Show resolved
Hide resolved
packages/webui/src/client/ui/Settings/Studio/Devices/PlayoutSubDevices.tsx
Show resolved
Hide resolved
tsorbo
left a comment
tsorbo
left a comment
There was a problem hiding this comment.
I have not review the code, but Henrik and I at NRK like the idea of adding a save button when editing peripheral device settings.
jstarpl
left a comment
jstarpl
left a comment
There was a problem hiding this comment.
Other than the things coderabbit pointed out, I have these suggestions:
| }, [studioId, wrappedSubDevices]) | ||
| }, [wrappedSubDevices, settingsWithOverrides.overrides]) | ||
|
|
||
| const [updatedIds, setUpdatedIds] = useState(new Map<string, string>()) |
There was a problem hiding this comment.
I would really appreciate a comment here describing what the strings in key and value of this Map actually mean, that the key is the oldId for a sub-device, and the value is the newId.
| const [updatedIds, setUpdatedIds] = useState(new Map<string, string>()) | ||
|
|
||
| const updateObjectId = useCallback( | ||
| (oldId: string, newId: string) => { | ||
| if (oldId === newId) return | ||
|
|
||
| batchedOverrideHelper().changeItemId(oldId, newId).commit() | ||
| setUpdatedIds((prev) => new Map(prev).set(oldId, newId)) | ||
| }, | ||
| [batchedOverrideHelper, setUpdatedIds] | ||
| ) | ||
|
|
||
| const discardChanges = useCallback(() => { | ||
| setUnsavedOverrides(undefined) | ||
| setUpdatedIds(new Map<string, string>()) | ||
| }, []) | ||
|
|
||
| const saveChanges = useCallback(() => { | ||
| if (studio?._id && unsavedOverrides) { | ||
| Studios.update(studio._id, { | ||
| $set: { | ||
| 'peripheralDeviceSettings.inputDevices.overrides': unsavedOverrides, | ||
| }, | ||
| }) | ||
| setUnsavedOverrides(undefined) | ||
| } | ||
|
|
||
| if (updatedIds.size > 0) { | ||
| setUpdatedIds(new Map<string, string>()) | ||
| } | ||
| }, [studio?._id, unsavedOverrides]) | ||
|
|
||
| const hasUnsavedChanges = useMemo(() => !!unsavedOverrides || updatedIds.size > 0, [unsavedOverrides, updatedIds]) |
There was a problem hiding this comment.
This whole section seems to be repeated for all of these *Devices.tsx files. Do you think it would make sense to maybe wrap them in a React hook that takes the one variable part (I think the saveChanges callback, perhaps even just the key for the $set object?), and just warp it up in a reusable code block?
There was a problem hiding this comment.
Very good point - tried to do that and IMO it turned out even worse - same amount of lines of code but with more abstraction and didnt turn out well
anteeek
force-pushed
the
feat/SOFIE-40
branch
2 times, most recently
from
9f98714 to
04edca7
Compare
|
anteeek
force-pushed
the
feat/SOFIE-40
branch
from
04edca7 to
daeef4f
Compare
|
@jstarpl applied changes & responded to remarks, please let me know if it's ok now |
anteeek
force-pushed
the
feat/SOFIE-40
branch
from
daeef4f to
18bcb42
Compare
5 participants
About the Contributor
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
Peripheral device settings UI
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status
Summary
This PR replaces autosave behavior in peripheral device settings with an explicit save workflow. Instead of persisting changes immediately on each input edit, users must now click a "save" button to commit configuration changes, preventing accidental on-air modifications.
Changes Overview
The implementation introduces a consistent staged-changes pattern across five peripheral device setting components. Each component now maintains local state for unsaved changes before persisting to the database.
Core Architecture Changes
Staged Override Management:
unsavedOverridesstate in each device component (Ingest, Input, Playout, Parent) to batch configuration changessettingsWithOverridesby merging base settings with unsaved changes, ensuring UI reflects pending modificationsChange Tracking:
updatedIdsmap to track device ID renames during editsupdateObjectIdhandler to apply ID changes through batched helpershasUnsavedChangesflag from unsaved overrides and updated IDsPersistence Control:
saveChangeshandler to persist all staged changes to the database in a single operationdiscardChangeshandler to revert all unsaved modificationsComponent-Level Changes
GenericSubDevices.tsx: Extended public interfaces and component props:
SubDevicesTableProps,SummaryRowProps,SubDeviceEditRowPropsnow include change-tracking propertiesinstantSaveOverrideHelper,hasUnsavedChanges,saveChanges,discardChanges,updateObjectId,updatedIdsSummaryRowdisplays pending-save indicator when device ID has changedSubDeviceEditRowconditionally renders Discard/Save buttons when unsaved changes exist; otherwise shows original Confirm flowIngestSubDevices.tsx (+63/-10):
unsavedOverridesstate andbaseSettingsmemosettingsWithOverridesfor staged changesGenericSubDevicesTableInputSubDevices.tsx (+65/-10):
unsavedOverridesandsettingsWithOverrideswrappedSubDevicesbased on staged settingsaddNewItemlogic to work with staged changesParentDevices.tsx (+107/-40):
AssignPeripheralDeviceConfigIdrefactored: removedstudioIdparameter, addedonChangecallback for staged updatesGenericParentDevicesTablePropsandParentDeviceEditRowPropsfor assignment state managementPlayoutSubDevices.tsx (+60/-5):
unsavedOverridesandupdatedIdstrackingdeviceSettingsconditionally from unsaved or base settingsGenericSubDevicesTableBenefits