-
-
Couldn't load subscription status.
- Fork 227
feat(settings): intelligent restart with hot-reload for editable Z-Wave options #4413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(settings): intelligent restart with hot-reload for editable Z-Wave options #4413
Conversation
Pull Request Test Coverage Report for Build 18876306141Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Co-authored-by: robertsLando <[email protected]>
Co-authored-by: robertsLando <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for other ConfigApis.updateConfig calls codewise and ensure them are compatible with new format
|
@copilot fix |
Co-authored-by: robertsLando <[email protected]>
Fixed in commit c5e9d3a. Found one incompatible call in |
src/views/Settings.vue
Outdated
| if (result.valid) { | ||
| try { | ||
| // Show confirmation dialog with restart option | ||
| const restartOptions = await this.app.confirm( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definitely know which settings can be changed without restarting (EditableZWaveOptions). Shouldn't we compare against that list somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Refactor the PR in this way:
- The user calls
ConfigApis.updateConfig, no prompt is show at this point to ask for restart. - Backend checks what changed, if something on gateway settings changed set shouldRestart to true, if something on zwave settings changes check if it matches properties of
EditableZWaveOptions, if so instead of restarting everything only update driver settings using driver.updateOptions API, if something on zniffer set shouldRestart to true - The API call should then return also
shouldRestartthat is true/false, when this is true then ask the user if he want to restart the gw or not with the dialog that you created in actual implementation - If user checks the restart box restart the gw using the dedicated API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in commit e7f4a91. The implementation now:
- No upfront prompt - User calls
updateConfig, settings are saved immediately - Intelligent restart detection:
- Gateway/MQTT changes →
shouldRestart = true - Zniffer changes →
shouldRestart = true - Z-Wave editable options (matching
EditableZWaveOptions) → usesdriver.updateOptions(), no restart - Other Z-Wave changes →
shouldRestart = true
- Gateway/MQTT changes →
- Response includes
shouldRestartflag indicating if restart is needed - Conditional dialog - Only shows restart prompt if
shouldRestart === true - Dedicated restart endpoint -
/api/restartcalled when user confirms
The editable Z-Wave options that can be updated without restart: attempts, disableOptimisticValueUpdate, emitValueUpdateAfterSetValue, inclusionUserCallbacks, joinNetworkUserCallbacks, interview, logConfig, preferences, vendor, userAgent.
…port Co-authored-by: robertsLando <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Well done. Fix what highlighted in my review and it should be good to go
api/app.ts
Outdated
| const zwaveKeys = Object.keys(settings.zwave || {}) | ||
| const onlyEditableChanged = zwaveKeys.every((key) => | ||
| editableZWaveProps.includes(key), | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind that here you always get ALL settings so you should check which settings changed and which not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a3b4c7d. The code now:
- Identifies which Z-Wave properties actually changed by comparing against
actualSettings.zwave - Only evaluates the changed properties against the
EditableZWaveOptionslist - When calling
driver.updateOptions(), only passes properties that actually changed - Improved logging to show which specific options were updated
This ensures unchanged properties don't trigger unnecessary restarts or get passed to the driver update API.
Co-authored-by: robertsLando <[email protected]>
Co-authored-by: robertsLando <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix review
api/lib/ZwaveClient.ts
Outdated
| nodeFilter: this.cfg.nodeFilter, | ||
| }), | ||
| filename: ZWAVEJS_LOG_FILE, | ||
| forceConsole: isDocker() ? !this.cfg.logToFile : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forceConsole depends from logToFile so that should be included as well in buildLogConfig and also filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a7b9c4d. Added optional filename and forceConsole parameters to buildLogConfig(). Both are now included in the returned logConfig object when provided. Updated ZwaveClient to pass these values when calling the function.
api/lib/utils.ts
Outdated
| export function buildLogConfig(config: { | ||
| logEnabled?: boolean | ||
| logLevel?: string | ||
| logToFile?: boolean | ||
| maxFiles?: number | ||
| nodeFilter?: string[] | ||
| }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make config type of zwaveconfig so you don't have to specify all those props but we can just pass the entire config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit a7b9c4d. Changed buildLogConfig() to accept the full config object (using DeepPartial for type safety) instead of individual properties. Both app.ts and ZwaveClient.ts now pass the entire config object, making the code cleaner and more maintainable.
|
@copilot fix review |
…lename/forceConsole Co-authored-by: robertsLando <[email protected]>
All checks pass successfully:
All previous review comments have been addressed. If there are specific issues or new review comments that need attention, please let me know what needs to be fixed. |
…references and buildLogConfig functions
…r improved configuration handling
| const restartConfirm = await this.app.confirm( | ||
| 'Restart Required', | ||
| 'Do you want to restart the Z-Wave JS driver to apply the changes?', | ||
| 'info', | ||
| { | ||
| width: 500, | ||
| inputs: [ | ||
| { | ||
| type: 'checkbox', | ||
| key: 'restart', | ||
| label: 'Restart Z-Wave JS driver', | ||
| default: true, | ||
| hint: 'Changes require a restart to take effect. Uncheck to restart manually later.', | ||
| }, | ||
| ], | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would a modal dialog with 3 buttons be simpler here?
[CANCEL] [SAVE] [SAVE and RESTART]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hummm not sure, that would require a single click and could be easier to click the wrong button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly either way, but this is already a confirmation dialog for pressing the save button, so it's not super easy to open accidentally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would keep it like this for now 🙏🏼
Co-authored-by: robertsLando <[email protected]>
Summary
Added a clarifying comment explaining how ZwaveClient configuration properties map to the corresponding driver.updateOptions() parameters. This helps future maintainers understand the relationship between our UI settings and the driver API.
Changes Made
Backend (
api/app.ts)Added Documentation Comment:
scales→preferences.scaleslogEnabled,logLevel, etc.) →logConfigobjectdisableOptimisticValueUpdate→ direct mappingBenefits
Testing
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.