fix: disable all config watchers #1306
Conversation
WalkthroughThe pull request removes configuration file watching from the store sync process by deleting the import and invocation of the Changes
Sequence Diagram(s)sequenceDiagram
participant DevCmd as DeveloperCommand
participant Stop as StopCommand
participant Start as StartCommand
DevCmd->>Stop: run([])
DevCmd->>Start: run([], {})
sequenceDiagram
participant SsoCmd as SSOCommand (Add/Remove)
participant Stop as StopCommand
participant Start as StartCommand
SsoCmd->>Stop: run([])
SsoCmd->>SsoCmd: process user update
SsoCmd->>Start: run([], {})
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (8)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (2)
api/src/unraid-api/cli/sso/add-sso-user.command.ts (1)
10-13: Remove unused RestartCommand importThe code has been refactored to use separate StartCommand and StopCommand instead of RestartCommand, but the import for RestartCommand on line 10 is still present and unused.
-import { RestartCommand } from '@app/unraid-api/cli/restart.command.js';api/src/unraid-api/cli/sso/remove-sso-user.command.ts (1)
35-36: Consider removing extra newlineThere appears to be an extra newline at line 35 that could be removed for consistency.
options = await this.inquirerService.prompt(RemoveSSOUserQuestionSet.name, options); - await this.stopCommand.run([]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/src/store/store-sync.ts(0 hunks)api/src/store/watch/config-watch.ts(0 hunks)api/src/unraid-api/cli/developer/developer.command.ts(3 hunks)api/src/unraid-api/cli/sso/add-sso-user.command.ts(3 hunks)api/src/unraid-api/cli/sso/remove-sso-user.command.ts(2 hunks)
💤 Files with no reviewable changes (2)
- api/src/store/watch/config-watch.ts
- api/src/store/store-sync.ts
🧰 Additional context used
🧬 Code Definitions (1)
api/src/unraid-api/cli/sso/remove-sso-user.command.ts (1)
api/src/store/modules/config.ts (2)
loadConfigFile(121-166)removeSsoUser(222-232)
🪛 GitHub Check: Build API
api/src/unraid-api/cli/developer/developer.command.ts
[failure] 10-10:
Replace opCommand·}·from·'@app/unraid-api/cli/stop with artCommand·}·from·'@app/unraid-api/cli/start
[failure] 11-11:
Replace artCommand·}·from·'@app/unraid-api/cli/start with opCommand·}·from·'@app/unraid-api/cli/stop
api/src/unraid-api/cli/sso/remove-sso-user.command.ts
[failure] 10-10:
Replace opCommand·}·from·'@app/unraid-api/cli/stop with artCommand·}·from·'@app/unraid-api/cli/start
[failure] 11-11:
Replace artCommand·}·from·'@app/unraid-api/cli/start with opCommand·}·from·'@app/unraid-api/cli/stop
🪛 GitHub Check: Test API
api/src/unraid-api/cli/developer/developer.command.ts
[failure] 10-10:
Replace opCommand·}·from·'@app/unraid-api/cli/stop with artCommand·}·from·'@app/unraid-api/cli/start
[failure] 11-11:
Replace artCommand·}·from·'@app/unraid-api/cli/start with opCommand·}·from·'@app/unraid-api/cli/stop
api/src/unraid-api/cli/sso/remove-sso-user.command.ts
[failure] 10-10:
Replace opCommand·}·from·'@app/unraid-api/cli/stop with artCommand·}·from·'@app/unraid-api/cli/start
[failure] 11-11:
Replace artCommand·}·from·'@app/unraid-api/cli/start with opCommand·}·from·'@app/unraid-api/cli/stop
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
api/src/unraid-api/cli/sso/add-sso-user.command.ts (2)
30-32: LGTM: Constructor now properly uses start and stop commandsThe constructor has been updated to use separate StartCommand and StopCommand dependencies instead of the RestartCommand, which aligns with the PR objective to disable config watchers.
40-46: LGTM: Implementation now properly stops and starts the serviceThe run method now correctly stops the service before modifying the configuration and starts it afterward. This approach gives better control over the service lifecycle compared to the previous restart mechanism.
api/src/unraid-api/cli/developer/developer.command.ts (2)
26-28: LGTM: Constructor now properly uses start and stop commandsThe constructor has been updated to use separate StartCommand and StopCommand dependencies instead of the RestartCommand, which aligns with the PR objective to disable config watchers.
39-47: LGTM: Implementation now properly stops and starts the serviceThe run method now correctly stops the service before modifying the configuration, waits for 5 seconds, and then starts it afterward. This approach gives better control over the service lifecycle compared to the previous restart mechanism.
api/src/unraid-api/cli/sso/remove-sso-user.command.ts (2)
26-29: LGTM: Constructor now properly includes start and stop commandsThe constructor has been updated to include StartCommand and StopCommand dependencies, which aligns with the PR objective to disable config watchers.
36-44: LGTM: Implementation now properly stops and starts the serviceThe run method now correctly stops the service before removing the SSO user and starts it afterward. This approach gives better control over the service lifecycle and helps avoid potential synchronization issues.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (1)
41-57: 🛠️ Refactor suggestionFurther elimination of jGrowl calls in notification loop.
Good follow-through on removing jGrowl expansions. However, lines 643–653 (unchanged in this diff) still set
$.jGrowl.defaults. If jGrowl is no longer used, consider removing or refactoring those references to avoid confusion.
🧹 Nitpick comments (5)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)
108-114: Newmonitormethod and sharedsubscribersarray.Registering each
NchanSubscriberin a shared array is a straightforward approach. Ensure each subscriber is removed fromsubscribersif it is destroyed or no longer needed, to avoid potential memory leaks or stale references.api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (4)
108-108: Monitor every new subscriber
This addition is straightforward: it tracks each subscriber instance by pushing it into the globalsubscribersarray. Ensure there's no unbounded growth or repeated registrations, especially if subscribers are repeatedly instantiated.
1275-1278: Use consistent data types forblurTimer
blurTimeris initiallyfalsebut then stores a timer ID. Consider initializing withnullto make the code more explicit and consistent with typical JS patterns.- var blurTimer = false; + var blurTimer = null;
1284-1289: Confirm the 30-second delay before pausing
A 30-second wait after blur might not be obvious to users. Verify that this aligns with your requirement to reduce resource usage while inactive.
1292-1305: Reconcileblurevent andvisibilitychangelogic
There’s overlapping functionality between windowblurandvisibilitychange. Consider centralizing the approach to avoid confusion or missed edge cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/.login.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php(28 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/Notifications.page.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/auth-request.php.last-download-time(1 hunks)api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php(27 hunks)api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/DefaultPageLayout.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/auth-request.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/.login.php.last-download-time
- api/src/unraid-api/unraid-file-modifier/modifications/test/fixtures/downloaded/Notifications.page.last-download-time
🧰 Additional context used
🧠 Learnings (2)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (6)
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:30-54
Timestamp: 2025-04-02T14:32:20.884Z
Learning: The removal of jGrowl notifications from DefaultPageLayout.php is intentional as notifications are now handled on a separate page as part of the architectural design to override existing Unraid pages.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:6-20
Timestamp: 2025-04-02T14:32:20.884Z
Learning: The default-page-layout.patch removes the old jGrowl notification system and is complemented by the unraid-toaster component implementation. The new system is added through the DefaultPageLayout modification which inserts the toaster component with proper position configuration based on user preferences.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:6-20
Timestamp: 2025-04-02T14:32:20.884Z
Learning: The default-page-layout.patch is used to remove the old jGrowl notification system from Unraid pages, as notifications are handled by a new system implemented on a different page.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch:24-27
Timestamp: 2025-04-02T14:32:20.884Z
Learning: The Unraid UI uses a modern notification system with a custom `unraid-toaster` component replacing the legacy jGrowl notifications. The system is backed by a comprehensive GraphQL API with real-time subscription support for notification updates.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-04-02T14:32:20.884Z
Learning: The project uses patches to override existing Unraid pages rather than modifying them directly.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-04-02T14:32:20.884Z
Learning: Notifications are implemented on a separate page rather than in the DefaultPageLayout.php file.
api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (2)
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-04-02T14:32:20.884Z
Learning: Notifications are implemented on a separate page rather than in the DefaultPageLayout.php file.
Learnt from: elibosley
PR: unraid/api#1101
File: api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php:203-207
Timestamp: 2025-04-02T14:32:20.884Z
Learning: The project uses patches to override existing Unraid pages rather than modifying them directly.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
api/src/unraid-api/unraid-file-modifier/modifications/patches/default-page-layout.patch (2)
5-24: Removal of jGrowl notifications inopenNotifierfunction.These lines remove the entire jGrowl-based approach and leave an empty loop body. This aligns with the move to the new toaster mechanism. Be sure you have no upstream calls relying on jGrowl usage here.
73-80: Addition of<uui-toaster>for the new notification system.Introducing
<uui-toaster>is a clean approach to modern notifications. Verify it’s fully integrated (e.g., calls to show/hide or update toaster content) so that all legacy jGrowl usage is replaced and tested.api/src/unraid-api/unraid-file-modifier/modifications/__test__/snapshots/DefaultPageLayout.php.modified.snapshot.php (1)
1258-1327: Pause/resume logic for live updates on window blur/focus.This logic correctly suspends Nchan updates after a brief delay on blur and resumes them on focus/visibility. Consider edge cases where switching tabs back and forth rapidly or manually stopping subscribers might conflict with these timers. Overall, it’s a solid improvement for controlling background traffic.
api/src/unraid-api/unraid-file-modifier/modifications/__test__/__fixtures__/downloaded/DefaultPageLayout.php (4)
761-762: Verify page auto-reload logic
Reloading the page periodically could disrupt ongoing tasks (e.g., unsaved changes). Confirm whether there's user acceptance for automatically reloading—especially ifnchanPausedis toggled.Would you like to incorporate a prompt preventing reload if forms are unsaved?
1279-1281: Focus event logic looks good
This ensures live updates resume when the window regains focus. Implementation is clear and concise.
1306-1324: Resume live updates gracefully
The function checks for a paused state, removes banner warnings, and restarts subscribers. The logic seems correct.
1326-1343: Pause live updates on window inactivity
Stopping each subscriber and setting a banner is effective. The catch block ensures graceful error handling if a specific subscriber stop call fails. No changes needed.
|
|
||
| function closeNotifier() { | ||
| $.post('/webGui/include/Notify.php',{cmd:'get',csrf_token:csrf_token},function(msg) { | ||
| @@ -698,12 +689,12 @@ | ||
| @@ -737,12 +728,12 @@ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Removal of jGrowl references in closeNotifier function.
Likewise, this cleans up the old jGrowl code. Confirm that no references to $('div.jGrowl') remain if you intend to remove jGrowl completely.
| // list of nchan subscribers to start/stop at focus change | ||
| var subscribers = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider better scoping for subscribers
Using a global subscribers array can lead to coupling and potential memory leaks if the page or script is included in different contexts. A closure-based or module-scoped approach might improve maintainability.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.5.0](v4.4.1...v4.5.0) (2025-04-02) ### Features * add webgui theme switcher component ([#1304](#1304)) ([e2d00dc](e2d00dc)) * api plugin system & offline versioned dependency vendoring ([#1252](#1252)) ([9f492bf](9f492bf)) * **api:** add `unraid-api --delete` command ([#1289](#1289)) ([2f09445](2f09445)) * basic array controls ([#1291](#1291)) ([61fe696](61fe696)) * basic docker controls ([#1292](#1292)) ([12eddf8](12eddf8)) * copy to webgui repo script docs + wc build options ([#1285](#1285)) ([e54f189](e54f189)) ### Bug Fixes * additional url fixes ([4b2763c](4b2763c)) * **api:** redirect benign pnpm postinstall warning to log file ([#1290](#1290)) ([7fb7849](7fb7849)) * **deps:** update dependency chalk to v5 ([#1296](#1296)) ([6bed638](6bed638)) * **deps:** update dependency diff to v7 ([#1297](#1297)) ([3c6683c](3c6683c)) * disable all config watchers ([#1306](#1306)) ([5c1b435](5c1b435)) * extract callbacks to library ([#1280](#1280)) ([2266139](2266139)) * OEM plugin issues ([#1288](#1288)) ([d5a3d0d](d5a3d0d)) * replace files lost during pruning ([d0d2ff6](d0d2ff6)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
<uui-toaster>component for notifications, replacing the previous jGrowl notification system and enhancing user experience.Refactor