-
Notifications
You must be signed in to change notification settings - Fork 18
Add dynamix refresh service, debounce store sync, and use ConfigService for notifications path #1888
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
base: main
Are you sure you want to change the base?
Add dynamix refresh service, debounce store sync, and use ConfigService for notifications path #1888
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { FileLoadStatus } from '@app/store/types.js'; | ||
| import { DynamixConfigRefreshService } from '@app/unraid-api/config/dynamix-config-refresh.service.js'; | ||
|
|
||
| const { dispatch, getState, loadDynamixConfigFromDiskSync, updateDynamixConfig } = vi.hoisted(() => ({ | ||
| dispatch: vi.fn(), | ||
| getState: vi.fn(), | ||
| loadDynamixConfigFromDiskSync: vi.fn(), | ||
| updateDynamixConfig: vi.fn((payload: unknown) => ({ type: 'dynamix/update', payload })), | ||
| })); | ||
|
|
||
| vi.mock('@app/store/index.js', () => ({ | ||
| store: { | ||
| dispatch, | ||
| getState, | ||
| }, | ||
| })); | ||
|
|
||
| vi.mock('@app/store/actions/load-dynamix-config-file.js', () => ({ | ||
| loadDynamixConfigFromDiskSync, | ||
| })); | ||
|
|
||
| vi.mock('@app/store/modules/dynamix.js', () => ({ | ||
| updateDynamixConfig, | ||
| })); | ||
|
|
||
| describe('DynamixConfigRefreshService', () => { | ||
| let service: DynamixConfigRefreshService; | ||
|
|
||
| beforeEach(() => { | ||
| vi.useFakeTimers(); | ||
| dispatch.mockReset(); | ||
| getState.mockReset(); | ||
| loadDynamixConfigFromDiskSync.mockReset(); | ||
| updateDynamixConfig.mockClear(); | ||
|
|
||
| getState.mockReturnValue({ | ||
| paths: { | ||
| 'dynamix-config': ['/etc/default.cfg', '/boot/config/plugins/dynamix/dynamix.cfg'], | ||
| }, | ||
| }); | ||
|
|
||
| service = new DynamixConfigRefreshService(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| service.onModuleDestroy(); | ||
| vi.runOnlyPendingTimers(); | ||
| vi.useRealTimers(); | ||
| }); | ||
|
|
||
| it('loads on init and dispatches loaded status', () => { | ||
| loadDynamixConfigFromDiskSync.mockReturnValue({ notify: { path: '/tmp/notifications' } }); | ||
|
|
||
| service.onModuleInit(); | ||
|
|
||
| expect(loadDynamixConfigFromDiskSync).toHaveBeenCalledWith([ | ||
| '/etc/default.cfg', | ||
| '/boot/config/plugins/dynamix/dynamix.cfg', | ||
| ]); | ||
| expect(updateDynamixConfig).toHaveBeenCalledWith({ | ||
| notify: { path: '/tmp/notifications' }, | ||
| status: FileLoadStatus.LOADED, | ||
| }); | ||
| expect(dispatch).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('skips dispatch when loaded config is unchanged', () => { | ||
| loadDynamixConfigFromDiskSync.mockReturnValue({ notify: { path: '/tmp/notifications' } }); | ||
|
|
||
| service.onModuleInit(); | ||
| vi.advanceTimersByTime(5000); | ||
|
|
||
| expect(dispatch).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('dispatches failed status when loading throws', () => { | ||
| loadDynamixConfigFromDiskSync.mockImplementation(() => { | ||
| throw new Error('boom'); | ||
| }); | ||
|
|
||
| service.onModuleInit(); | ||
|
|
||
| expect(updateDynamixConfig).toHaveBeenCalledWith({ status: FileLoadStatus.FAILED_LOADING }); | ||
| expect(dispatch).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import { Injectable, Logger, OnModuleDestroy, OnModuleInit } from '@nestjs/common'; | ||
|
|
||
| import { loadDynamixConfigFromDiskSync } from '@app/store/actions/load-dynamix-config-file.js'; | ||
| import { store } from '@app/store/index.js'; | ||
| import { updateDynamixConfig } from '@app/store/modules/dynamix.js'; | ||
| import { FileLoadStatus } from '@app/store/types.js'; | ||
|
|
||
| const DYNAMIX_REFRESH_INTERVAL_MS = 5000; | ||
|
|
||
| @Injectable() | ||
| export class DynamixConfigRefreshService implements OnModuleInit, OnModuleDestroy { | ||
| private readonly logger = new Logger(DynamixConfigRefreshService.name); | ||
| private refreshTimer: NodeJS.Timeout | null = null; | ||
| private lastSerializedConfig: string | null = null; | ||
|
|
||
| onModuleInit() { | ||
| this.refresh(); | ||
| this.refreshTimer = setInterval(() => { | ||
| this.refresh(); | ||
| }, DYNAMIX_REFRESH_INTERVAL_MS); | ||
| } | ||
|
|
||
| onModuleDestroy() { | ||
| if (!this.refreshTimer) { | ||
| return; | ||
| } | ||
|
|
||
| clearInterval(this.refreshTimer); | ||
| this.refreshTimer = null; | ||
| } | ||
|
|
||
| private refresh() { | ||
| const configPaths = store.getState().paths['dynamix-config'] ?? []; | ||
|
Check failure on line 33 in api/src/unraid-api/config/dynamix-config-refresh.service.ts
|
||
|
|
||
| try { | ||
| const config = loadDynamixConfigFromDiskSync(configPaths); | ||
| const serializedConfig = JSON.stringify(config); | ||
| if (serializedConfig === this.lastSerializedConfig) { | ||
| return; | ||
|
Comment on lines
+38
to
+39
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The unchanged-config guard returns before any status update, which means a transient read error can permanently leave Useful? React with 👍 / 👎. |
||
| } | ||
|
Comment on lines
+38
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reset dedupe state after errors so successful recovery dispatches 💡 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 |
||
|
|
||
| store.dispatch( | ||
| updateDynamixConfig({ | ||
| ...config, | ||
| status: FileLoadStatus.LOADED, | ||
| }) | ||
| ); | ||
| this.lastSerializedConfig = serializedConfig; | ||
| } catch (error) { | ||
| this.logger.error(error, 'Failed to refresh dynamix config from disk'); | ||
| store.dispatch( | ||
| updateDynamixConfig({ | ||
| status: FileLoadStatus.FAILED_LOADING, | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
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.
Guard
pathsbefore indexing to prevent startup crash.Line 33 can throw when
store.getState().pathsis undefined (matches CI failure), which breaks module init.💡 Proposed fix
🧰 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