Add dynamix refresh service, debounce store sync, and use ConfigService for notifications path#1888
Add dynamix refresh service, debounce store sync, and use ConfigService for notifications path#1888
Conversation
WalkthroughA new periodic dynamix config refresh service is introduced that automatically loads configuration from disk at 5-second intervals. The store getter is simplified to return persisted state directly, while the service handles actual config loading and synchronization logic on module initialization. Changes
Sequence DiagramsequenceDiagram
participant Module as NestJS Module
participant Service as DynamixConfigRefreshService
participant FileSystem as File System
participant Store as Store
participant Logger as Logger
Module->>Service: onModuleInit()
Service->>Service: Start 5s interval timer
loop Every 5 seconds
Service->>Store: Get dynamix-config paths
Store-->>Service: Config paths
Service->>FileSystem: loadDynamixConfigFromDiskSync()
alt Success
FileSystem-->>Service: Config data
Service->>Service: Serialize config
alt Config changed
Service->>Store: updateDynamixConfig(config, LOADED)
Store->>Store: Update state
else Config unchanged
Service->>Service: Skip dispatch
end
else Loading fails
FileSystem-->>Service: Error
Logger->>Logger: Log error
Service->>Store: updateDynamixConfig(_, FAILED_LOADING)
Store->>Store: Update state
end
end
Module->>Service: onModuleDestroy()
Service->>Service: Clear interval timer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 621f9064df
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (serializedConfig === this.lastSerializedConfig) { | ||
| return; |
There was a problem hiding this comment.
The unchanged-config guard returns before any status update, which means a transient read error can permanently leave dynamix.status as FAILED_LOADING: the catch block dispatches failure but keeps lastSerializedConfig, and when the same config content becomes readable again, this early return skips the LOADED dispatch. In that scenario, consumers that gate behavior on FileLoadStatus.LOADED will stay in an error state until the file contents actually change.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| PluginService.logger.error(`Plugin from ${pkgName} is invalid: %o`, error as object); | ||
| const notificationService = new NotificationsService(); | ||
| const notificationService = new NotificationsService(new ConfigService()); |
There was a problem hiding this comment.
Constructing NotificationsService with new ConfigService() creates an empty config source, so NotificationsService.getConfiguredPath() falls back to /tmp/notifications instead of the runtime store.dynamix.notify.path. If users configure a non-default notification path, plugin import errors will be written to the wrong directory and won't show up where the rest of notifications are read.
Useful? React with 👍 / 👎.
![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 (1)
api/src/unraid-api/config/dynamix-config-refresh.service.spec.ts (1)
53-88: Add regression tests forpathsmissing and fail→recover status transition.
Current cases don’t cover the Line 33 crash path (pathsundefined) or the scenario where a failure is followed by a successful reload with unchanged config.If useful, I can draft the two Vitest cases directly in this spec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/unraid-api/config/dynamix-config-refresh.service.spec.ts` around lines 53 - 88, Add two Vitest cases to cover the regression: (1) a test where loadDynamixConfigFromDiskSync returns an object without a paths property (e.g. { notify: {...} } or {} ) and ensure service.onModuleInit() does not throw and still calls updateDynamixConfig/dispatch appropriately; (2) a test simulating a failure first (mock loadDynamixConfigFromDiskSync to throw) then a subsequent successful load with the same config (mock implementations and advance timers) and assert updateDynamixConfig/dispatch are called for the FAILED_LOADING then called again for the recovery but not duplicated when the recovered config is unchanged; use the existing helpers/mocks (loadDynamixConfigFromDiskSync, service.onModuleInit, updateDynamixConfig, dispatch, FileLoadStatus) to locate where to add these specs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/unraid-api/config/dynamix-config-refresh.service.ts`:
- Around line 38-40: The dedupe check using lastSerializedConfig prevents
dispatching LOADED after a prior FAILED_LOADING when the succeeding config is
identical; fix by not updating lastSerializedConfig on failures and by clearing
or nulling lastSerializedConfig in error paths so identical successful payloads
re-trigger processing. Specifically, in the block comparing serializedConfig to
lastSerializedConfig and in the error/catch handling (references:
serializedConfig, lastSerializedConfig, and the FAILED_LOADING / LOADED
dispatches), ensure lastSerializedConfig is only assigned after successful
handling (after dispatching LOADED) and that any catch/failure path resets
lastSerializedConfig (or leaves it unset) so a later successful match will not
be deduped.
- Line 33: Guard access to store.getState().paths before indexing to avoid
crashes when paths is undefined: replace the direct indexing in the assignment
to configPaths with a safe access (e.g. use optional chaining or a default
object). Concretely, update the line that assigns configPaths (currently using
store.getState().paths['dynamix-config'] ?? []) to something like
store.getState().paths?.['dynamix-config'] ?? [] or first capture const paths =
store.getState().paths ?? {} and then read paths['dynamix-config'] ?? []; this
change in dynamix-config-refresh.service.ts (the assignment creating
configPaths) will prevent startup errors when paths is undefined.
---
Nitpick comments:
In `@api/src/unraid-api/config/dynamix-config-refresh.service.spec.ts`:
- Around line 53-88: Add two Vitest cases to cover the regression: (1) a test
where loadDynamixConfigFromDiskSync returns an object without a paths property
(e.g. { notify: {...} } or {} ) and ensure service.onModuleInit() does not throw
and still calls updateDynamixConfig/dispatch appropriately; (2) a test
simulating a failure first (mock loadDynamixConfigFromDiskSync to throw) then a
subsequent successful load with the same config (mock implementations and
advance timers) and assert updateDynamixConfig/dispatch are called for the
FAILED_LOADING then called again for the recovery but not duplicated when the
recovered config is unchanged; use the existing helpers/mocks
(loadDynamixConfigFromDiskSync, service.onModuleInit, updateDynamixConfig,
dispatch, FileLoadStatus) to locate where to add these specs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a54b3a77-ee68-4b0e-978a-3b680783be3d
📒 Files selected for processing (4)
api/src/store/index.tsapi/src/unraid-api/config/dynamix-config-refresh.service.spec.tsapi/src/unraid-api/config/dynamix-config-refresh.service.tsapi/src/unraid-api/config/legacy-config.module.ts
| } | ||
|
|
||
| private refresh() { | ||
| const configPaths = store.getState().paths['dynamix-config'] ?? []; |
There was a problem hiding this comment.
Guard paths before indexing to prevent startup crash.
Line 33 can throw when store.getState().paths is undefined (matches CI failure), which breaks module init.
💡 Proposed fix
- private refresh() {
- const configPaths = store.getState().paths['dynamix-config'] ?? [];
-
- try {
+ private refresh() {
+ try {
+ const configPaths = store.getState().paths?.['dynamix-config'] ?? [];
const config = loadDynamixConfigFromDiskSync(configPaths);
const serializedConfig = JSON.stringify(config);🧰 Tools
🪛 GitHub Actions: CI - Main (API)
[error] 33-33: Cannot read properties of undefined (reading 'dynamix-config') during DynamixConfigRefreshService.refresh invoked on AppModule initialization (AppModule Integration Tests).
🪛 GitHub Check: Test API
[failure] 33-33: src/unraid-api/app/test/app.module.integration.spec.ts > AppModule Integration Tests
TypeError: Cannot read properties of undefined (reading 'dynamix-config')
❯ DynamixConfigRefreshService.refresh src/unraid-api/config/dynamix-config-refresh.service.ts:33:51
❯ DynamixConfigRefreshService.onModuleInit src/unraid-api/config/dynamix-config-refresh.service.ts:17:14
❯ MapIterator.iteratee ../node_modules/.pnpm/@nestjs+core@11.1.6_@nestjs+common@11.1.6_class-transformer@0.5.1_class-validator@0.14.bc2bc0d32b3785c29eaf84bdeadd843f/node_modules/@nestjs/core/hooks/on-module-init.hook.js:22:43
❯ MapIterator.next ../node_modules/.pnpm/iterare@1.2.1/node_modules/iterare/src/map.ts:9:39
❯ IteratorWithOperators.next ../node_modules/.pnpm/iterare@1.2.1/node_modules/iterare/src/iterate.ts:19:28
❯ IteratorWithOperators.toArray ../node_modules/.pnpm/iterare@1.2.1/node_modules/iterare/src/iterate.ts:227:22
❯ callOperator ../node_modules/.pnpm/@nestjs+core@11.1.6@nestjs+common@11.1.6_class-transformer@0.5.1_class-validator@0.14.bc2bc0d32b3785c29eaf84bdeadd843f/node_modules/@nestjs/core/hooks/on-module-init.hook.js:23:10
❯ callModuleInitHook ../node_modules/.pnpm/@nestjs+core@11.1.6@nestjs+common@11.1.6_class-transformer@0.5.1_class-validator@0.14.bc2bc0d32b3785c29eaf84bdeadd843f/node_modules/@nestjs/core/hooks/on-module-init.hook.js:43:23
❯ Proxy.callInitHook ../node_modules/.pnpm/@nestjs+core@11.1.6@nestjs+common@11.1.6_class-transformer@0.5.1_class-validator@0.14._bc2bc0d32b3785c29eaf84bdeadd843f/node_modules/@nestjs/core/nest-application-context.js:242:50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/unraid-api/config/dynamix-config-refresh.service.ts` at line 33,
Guard access to store.getState().paths before indexing to avoid crashes when
paths is undefined: replace the direct indexing in the assignment to configPaths
with a safe access (e.g. use optional chaining or a default object). Concretely,
update the line that assigns configPaths (currently using
store.getState().paths['dynamix-config'] ?? []) to something like
store.getState().paths?.['dynamix-config'] ?? [] or first capture const paths =
store.getState().paths ?? {} and then read paths['dynamix-config'] ?? []; this
change in dynamix-config-refresh.service.ts (the assignment creating
configPaths) will prevent startup errors when paths is undefined.
| if (serializedConfig === this.lastSerializedConfig) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Reset dedupe state after errors so successful recovery dispatches LOADED.
With current logic, a failure can leave state stuck as FAILED_LOADING if the next successful config matches the previous serialized value.
💡 Proposed fix
} catch (error) {
this.logger.error(error, 'Failed to refresh dynamix config from disk');
store.dispatch(
updateDynamixConfig({
status: FileLoadStatus.FAILED_LOADING,
})
);
+ this.lastSerializedConfig = null;
}Also applies to: 49-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/unraid-api/config/dynamix-config-refresh.service.ts` around lines 38
- 40, The dedupe check using lastSerializedConfig prevents dispatching LOADED
after a prior FAILED_LOADING when the succeeding config is identical; fix by not
updating lastSerializedConfig on failures and by clearing or nulling
lastSerializedConfig in error paths so identical successful payloads re-trigger
processing. Specifically, in the block comparing serializedConfig to
lastSerializedConfig and in the error/catch handling (references:
serializedConfig, lastSerializedConfig, and the FAILED_LOADING / LOADED
dispatches), ensure lastSerializedConfig is only assigned after successful
handling (after dispatching LOADED) and that any catch/failure path resets
lastSerializedConfig (or leaves it unset) so a later successful match will not
be deduped.
1 participant
Motivation
ConfigServiceby debouncing and avoiding identical writes.ConfigServicerather than reading directly from store getters.Description
getters.dynamixto returnstore.getState().dynamixdirectly instead of callingloadDynamixConfigfrom the getter.DynamixConfigRefreshServiceto periodically reload dynamix config from disk (DYNAMIX_REFRESH_INTERVAL_MS = 5000), dispatch updates only when the serialized config changes, and dispatchFAILED_LOADINGon errors, and registered the service inLegacyConfigModule.StoreSyncServicewithSTORE_SYNC_DEBOUNCE_MS = 1000, trackinglastSyncedStateto skip redundant writes and clearing timers on destroy.NotificationsServiceto acceptConfigServicevia DI, read the notification path fromconfigService.get('store.dynamix.notify.path', '/tmp/notifications'), and updated constructor/paths()accordingly to recreate the watcher when the configured path changes.PluginServiceto instantiateNotificationsServicewith aConfigServicewhen reporting plugin load errors so notifications can be created without relying on store getters.dynamix-config-refresh.service.spec.ts,store-sync.service.spec.ts, and adjusted notifications tests (loadNotificationsFile.test.ts,notifications.service.spec.ts) to provideConfigServicein test modules or helpers.Testing
dynamix-config-refresh.service.spec.tsandstore-sync.service.spec.tsand the updated notifications specs, and all tests completed successfully.NotificationsServiceintegration-style specs that interact with the filesystem, and they passed locally under the updatedConfigService-based configuration.ConfigServiceor to use the new helpercreateNotificationsService.Codex Task
Summary by CodeRabbit
Release Notes
New Features
Tests