fix: flaky watch on boot drive's dynamix config #1753
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the async/thunk file-watch loader with a synchronous, TTL-memoized disk loader; adds createTtlMemoizedLoader utility; exports a new store API Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller / getter
participant Store as Store (api/src/store/index.ts)
participant LoaderAPI as loadDynamixConfig()
participant DiskLoader as loadDynamixConfigFromDiskSync
participant TTL as TTL Memoized Loader
participant Reducer as updateDynamixConfig
Caller->>Store: access dynamix getter
Store->>LoaderAPI: invoke loadDynamixConfig()
LoaderAPI->>Store: read paths['dynamix-config']
LoaderAPI->>TTL: request cached load for paths
alt Cache hit (within TTL)
TTL-->LoaderAPI: return cached merged config
else Cache miss / expired
TTL->>DiskLoader: execute sync load & merge files
DiskLoader-->TTL: return merged config (cached)
end
LoaderAPI->>Reducer: dispatch updateDynamixConfig(..., LOADED) or FAILED_LOADING
LoaderAPI->>Store: return store.getState().dynamix
Store->>Caller: return dynamix config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-06-11T14:14:30.348ZApplied to files:
🔇 Additional comments (1)
Comment |
pujitm
force-pushed
the
fix/config-watch
branch
from
9e2ec99 to
ec7aa06
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1753 +/- ##
==========================================
- Coverage 53.35% 52.69% -0.67%
==========================================
Files 860 865 +5
Lines 48275 49343 +1068
Branches 4929 4952 +23
==========================================
+ Hits 25756 26000 +244
- Misses 22450 23270 +820
- Partials 69 73 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/src/store/index.ts (1)
50-55: Make getters pure by removing side effects fromdynamixgetterThe review comment correctly identifies that
getters.dynamix()has side effects: it callsloadDynamixConfig(), which reads from disk and dispatches store updates (lines 29-47 of api/src/store/index.ts). While TTL caching mitigates repeated disk I/O, multiple sequential accesses in a single method (e.g., customization.service.ts lines 452-457 calls the getter 6 times) demonstrate that callers treat this as a pure selector, not a triggering function.The suggested refactoring is valid and beneficial: change the getter to return
store.getState().dynamix(pure), keepingloadDynamixConfig()as an explicit function. This pattern is already supported by the codebase—loadDynamixConfig()is already called during bootstrap (api/src/index.ts line 77), ensuring data is pre-loaded before services access it. All current usages will continue to work without modification.
🧹 Nitpick comments (10)
packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts (2)
21-37: Safer fake-timers restoreWrap
useFakeTimers()withtry/finallyto always restore real timers, or movevi.useRealTimers()to anafterEachfor this file to avoid leakage across tests.it("evicts cache entries once ttl expires", () => { - vi.useFakeTimers(); + vi.useFakeTimers(); let loadCount = 0; // ... - vi.useRealTimers(); + vi.useRealTimers(); }); +afterEach(() => { + vi.useRealTimers(); +});
56-71: Add coverage for edge-cases: clear() and zero TTLRecommend adding tests to assert:
clear()invalidates the cache immediately.ttlMs: 0results in no reuse even within the same tick (now < expiresAtis false on equality).api/src/unraid-api/app/__test__/app.module.integration.spec.ts (1)
116-119: Remove noisy console log in integration testsDrop or gate the debug
console.logto keep CI output clean.-const { getters } = await import('@app/store/index.js'); -console.log('CSRF Token from store:', getters.emhttp().var.csrfToken); +// Optionally enable when debugging: +// if (process.env.DEBUG_TESTS) { +// const { getters } = await import('@app/store/index.js'); +// console.log('CSRF Token from store:', getters.emhttp().var.csrfToken); +// }api/src/store/index.ts (1)
39-47: Failure path may leave stale config in stateMerging only
{ status: FAILED_LOADING }can retain previous config keys. Prefer clearing or marking data stale to avoid consumers reading old values with a failure status.} catch (error) { logger.error(error, 'Failed to load dynamix config from disk'); - store.dispatch( - updateDynamixConfig({ - status: FileLoadStatus.FAILED_LOADING, - }) - ); + store.dispatch( + updateDynamixConfig({ + // Option A: minimal + status: FileLoadStatus.FAILED_LOADING, + // Option B: explicitly clear known sections to avoid stale reads: + // display: {} as any, notify: {} as any, ssmtp: {} as any, parity: {} as any, remote: {} as any, + }) + );Would you like me to scan the slice definition and propose an exact “clear” payload for all top-level keys?
api/src/__test__/core/utils/images/image-file-helpers.test.ts (1)
13-21: Align test init order with app startupIf these assertions depend on
paths['dynamix-config'], initialize paths before loading config to avoid future flakiness.-import { loadDynamixConfig } from '@app/store/index.js'; +import { loadDynamixConfig, store } from '@app/store/index.js'; +import { loadStateFiles } from '@app/store/modules/emhttp.js'; ... test('get banner path returns the banner (state loaded)', async () => { - loadDynamixConfig(); + await store.dispatch(loadStateFiles()); + loadDynamixConfig(); await expect(getBannerPathIfPresent()).resolves.toContain('/dev/dynamix/banner.png'); }); test('get banner path returns null when no banner (state loaded)', async () => { - loadDynamixConfig(); + await store.dispatch(loadStateFiles()); + loadDynamixConfig(); await expect(getBannerPathIfPresent('notabanner.png')).resolves.toMatchInlineSnapshot('null'); });As per coding guidelines.
api/src/store/actions/load-dynamix-config-file.ts (3)
33-37: Stabilize cache key to avoid churn from undefined/null entries
JSON.stringify(configPaths)treatsundefinedasnullin arrays and includes them, generating different keys for semantically equivalent inputs. Filter first to only valid strings when deriving the key.- getCacheKey: (configPaths: ConfigPaths): string => JSON.stringify(configPaths), + getCacheKey: (configPaths: ConfigPaths): string => + JSON.stringify(configPaths.filter((p): p is string => Boolean(p))),
41-47: Shallow merge may drop nested config keys
{ ...accumulator, ...configFile }overwrites whole top-level sections. If multiple files provide different nested keys under the same section, earlier keys are lost. Consider a deep merge.- return configFiles.reduce<RecursivePartial<DynamixConfig>>( - (accumulator, configFile) => ({ - ...accumulator, - ...configFile, - }), - {} - ); + const deepMerge = <T extends Record<string, any>>(a: T, b: T): T => { + const out: any = { ...a }; + for (const [k, v] of Object.entries(b)) { + if (v && typeof v === 'object' && !Array.isArray(v) && typeof out[k] === 'object') { + out[k] = deepMerge(out[k], v); + } else { + out[k] = v; + } + } + return out; + }; + return configFiles.reduce<RecursivePartial<DynamixConfig>>( + (acc, cfg) => deepMerge(acc, cfg), + {} + );If
lodash.mergeis already a dependency, we can switch to it instead to reduce code.
28-49: Expose a cache clear for tests and manual refreshProviding a small export to call
memoizedConfigLoader.clear()helps tests and admin endpoints force a reload after on-disk changes.const memoizedConfigLoader = createTtlMemoizedLoader< RecursivePartial<DynamixConfig>, ConfigPaths, string >({ // ... }); +export const clearDynamixConfigCache = () => memoizedConfigLoader.clear();api/src/index.ts (1)
79-83: Without a watcher, consider a refresh strategySince the dynamix watch is removed, runtime changes won’t apply unless
loadDynamixConfig()is called again. Options:
- Periodic refresh with backoff (e.g., every few minutes).
- Admin endpoint/command to call
loadDynamixConfig()(and clear memoizer).- Triggered reload when relevant GraphQL resolvers need fresh values.
packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
58-66: Single-entry cache can surprise callers using multiple keysCurrent design holds only one cached entry; switching keys evicts the previous key’s value. If “independent entries” are desired, use a per-key Map while keeping the no-key case fast.
-export const createTtlMemoizedLoader = <T, Args = void, Key = Args>( +export const createTtlMemoizedLoader = <T, Args = void, Key = Args>( options: CreateTtlMemoizedLoaderOptions<T, Args, Key> ): TtlMemoizedLoader<T, Args> => { - let cache: CacheEntry<T, Key> | undefined; + const NO_KEY: unique symbol = Symbol('NO_KEY'); + const cache = new Map<unknown, CacheEntry<T, Key>>(); const clear = () => { - cache = undefined; + cache.clear(); }; const get = (args: Args): T => { const now = Date.now(); - const key = options.getCacheKey?.(args); + const key = options.getCacheKey ? options.getCacheKey(args) : NO_KEY; - if (cache) { - const keyMatches = - options.getCacheKey === undefined ? true : Object.is(cache.key, key); - - if (keyMatches && now < cache.expiresAt) { - return cache.value; - } - } + const entry = cache.get(key as any); + if (entry && now < entry.expiresAt) { + return entry.value; + } const value = options.load(args); if (!options.shouldCache || options.shouldCache(value)) { - cache = { + const newEntry: CacheEntry<T, Key> = { value, expiresAt: now + Math.max(0, options.ttlMs), key, }; + cache.set(key as any, newEntry); } else { - cache = undefined; + cache.delete(key as any); } return value; }; return { get, clear }; };Note: This trades a tiny memory increase (per live key within TTL) for clearer semantics. If capacity limits are needed, we can add an optional max size and LRU eviction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/src/__test__/core/utils/images/image-file-helpers.test.ts(1 hunks)api/src/index.ts(2 hunks)api/src/store/actions/load-dynamix-config-file.ts(2 hunks)api/src/store/index.ts(2 hunks)api/src/store/modules/dynamix.ts(0 hunks)api/src/store/watch/dynamix-config-watch.ts(0 hunks)api/src/unraid-api/app/__test__/app.module.integration.spec.ts(2 hunks)packages/unraid-shared/src/index.ts(1 hunks)packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts(1 hunks)packages/unraid-shared/src/util/create-ttl-memoized-loader.ts(1 hunks)
💤 Files with no reviewable changes (2)
- api/src/store/watch/dynamix-config-watch.ts
- api/src/store/modules/dynamix.ts
🧰 Additional context used
📓 Path-based instructions (8)
**/{store,stores}/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
In store files, explicitly import Vue reactivity utilities (e.g., computed, ref, watchEffect); do not rely on Nuxt auto-imports
Files:
api/src/store/index.tsapi/src/store/actions/load-dynamix-config-file.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
api/src/store/index.tsapi/src/store/actions/load-dynamix-config-file.tsapi/src/__test__/core/utils/images/image-file-helpers.test.tspackages/unraid-shared/src/util/create-ttl-memoized-loader.tspackages/unraid-shared/src/index.tspackages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/index.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/store/index.tsapi/src/store/actions/load-dynamix-config-file.tsapi/src/__test__/core/utils/images/image-file-helpers.test.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/index.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/__test__/core/utils/images/image-file-helpers.test.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.ts
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
api/src/__test__/core/utils/images/image-file-helpers.test.tspackages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/__test__/core/utils/images/image-file-helpers.test.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/__test__/core/utils/images/image-file-helpers.test.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/app/__test__/app.module.integration.spec.ts
🧠 Learnings (3)
📚 Learning: 2025-01-22T18:34:06.925Z
Learnt from: elibosley
PR: unraid/api#1068
File: api/src/unraid-api/auth/api-key.service.ts:122-137
Timestamp: 2025-01-22T18:34:06.925Z
Learning: The store in app/store is implemented using Redux's configureStore, where dispatch operations for config updates are synchronous in-memory state updates that cannot fail, making transaction-like patterns unnecessary.
Applied to files:
api/src/store/index.ts
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.spec.ts
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: Applies to api/**/*.{test,spec}.{ts,tsx} : API test suite is Vitest; do not use Jest
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.spec.ts
🧬 Code graph analysis (7)
api/src/store/index.ts (3)
api/src/store/actions/load-dynamix-config-file.ts (1)
loadDynamixConfigFromDiskSync(57-61)api/src/store/modules/dynamix.ts (1)
updateDynamixConfig(20-22)api/src/core/log.ts (1)
logger(50-80)
api/src/store/actions/load-dynamix-config-file.ts (4)
packages/unraid-shared/src/util/file.ts (1)
fileExistsSync(22-29)api/src/core/utils/misc/parse-config.ts (1)
parseConfig(146-203)packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
createTtlMemoizedLoader(45-82)api/src/core/types/ini.ts (1)
DynamixConfig(126-132)
api/src/__test__/core/utils/images/image-file-helpers.test.ts (1)
api/src/store/index.ts (1)
loadDynamixConfig(29-48)
packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
packages/unraid-shared/src/index.ts (3)
CreateTtlMemoizedLoaderOptions(9-9)TtlMemoizedLoader(10-10)createTtlMemoizedLoader(7-7)
packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts (2)
packages/unraid-shared/src/index.ts (1)
createTtlMemoizedLoader(7-7)packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
createTtlMemoizedLoader(45-82)
api/src/unraid-api/app/__test__/app.module.integration.spec.ts (1)
api/src/store/index.ts (1)
loadDynamixConfig(29-48)
api/src/index.ts (1)
api/src/store/index.ts (1)
loadDynamixConfig(29-48)
🔇 Additional comments (2)
packages/unraid-shared/src/index.ts (1)
7-11: LGTM — public surface re-export looks correctESM
.jsspecifiers used; types re-exported withexport typeto avoid runtime impact.api/src/index.ts (1)
76-78: LGTM — startup now uses direct, synchronous config loadOrder matches: state files → registration → dynamix config. This aligns with the new loader.
| it("treats different cache keys as independent entries", () => { | ||
| let loadCount = 0; | ||
| const loader = createTtlMemoizedLoader<number, { key: string }>({ | ||
| ttlMs: 500, | ||
| getCacheKey: ({ key }) => key, | ||
| load: ({ key }) => { | ||
| loadCount += 1; | ||
| return Number(`${loadCount}${key.length}`); | ||
| }, | ||
| }); | ||
|
|
||
| expect(loader.get({ key: "a" })).toBe(11); | ||
| expect(loader.get({ key: "a" })).toBe(11); | ||
| expect(loader.get({ key: "ab" })).toBe(22); | ||
| expect(loadCount).toBe(2); | ||
| }); |
There was a problem hiding this comment.
Test name suggests per-key caching that the implementation doesn’t provide
The memoizer currently caches only the last key, not independent entries per key. Calling get({ key: 'a' }) again after get({ key: 'ab' }) would recompute. Either:
- Adjust the test name/expectations, or
- Update the implementation to cache per key (Map) and add an assertion for revisiting the first key.
🤖 Prompt for AI Agents
In packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts
around lines 39–54, the test is written to assert per-key caching but the
implementation currently only remembers the last key; update either the test or
the implementation — to preserve the test intent, change the memoizer to use a
Map keyed by getCacheKey that stores {value, expiry} per key, have get check the
map for an unexpired entry and call load only when missing/expired, and then add
an assertion that revisiting the first key after loading a second key returns
the cached value and does not increment loadCount; alternatively, if you intend
single-entry caching, rename the test and adjust expectations to reflect
last-key-only behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
27-36: Clarify single-slot semantics; minor API ergonomics.
- Doc says “entry” but not that it’s single-slot; note that a new key evicts the previous one to avoid misuse expectations.
- Consider making
getfriendlier forArgs = voidvia tuple conditional args:
[ suggest_optional_refactor ]-export type TtlMemoizedLoader<T, Args = void> = { - get: (args: Args) => T; +export type TtlMemoizedLoader<T, Args = void> = { + get: (...args: Args extends void ? [] : [Args]) => T;
- Recommend documenting that
getCacheKeyshould return a stable primitive (e.g., string) sinceObject.iscompares by reference for objects.Also applies to: 45-49, 58-66
api/src/__test__/core/utils/images/image-file-helpers.test.ts (1)
7-7: Direct loader use is fine; optionally centralize init.Calling
loadDynamixConfig()synchronously per test is OK. Consider moving it to abeforeAllto reduce duplication.Also applies to: 14-14, 19-19
api/src/store/actions/load-dynamix-config-file.ts (1)
16-23: Sync loader is clean; consider deep merge and per-file resilience.
- Current merge is shallow; nested sections (e.g.,
display,notify) from later files overwrite entire objects. If prior behavior expected key-level overlay, switch to deep merge.- return configFiles.reduce<RecursivePartial<DynamixConfig>>( - (accumulator, configFile) => ({ - ...accumulator, - ...configFile, - }), - {} - ); + return configFiles.reduce<RecursivePartial<DynamixConfig>>((acc, cfg) => deepMerge(acc, cfg), {});Add a small
deepMergeutility (non-mutating) locally or reuse an existing helper if present.
Optional: continue loading remaining files when one parse fails (catch inside
loadConfigFileSyncor within the reducer) to avoid one corrupt file breaking the load; still let the caller set FAILED status if all fail.Nit: reuse the
ConfigPathsalias in the exported function signature for consistency.Would you like a PR-ready
deepMergehelper added undercore/utils?Also applies to: 25-49, 57-61
api/src/store/index.ts (1)
22-48: Consider adding explicit return type annotation.The function implementation correctly handles both success and error cases, and the TTL-cached loading approach appropriately addresses the FAT32 file system issue described in the PR objectives. However, adding an explicit return type would improve code clarity and maintainability.
Apply this diff to add the return type:
/** * Loads the dynamix config into the store. * Can be called multiple times - uses TTL caching internally. * @returns The loaded dynamix config. */ -export const loadDynamixConfig = () => { +export const loadDynamixConfig = (): RootState['dynamix'] => { const configPaths = store.getState().paths['dynamix-config'] ?? [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/src/__test__/core/utils/images/image-file-helpers.test.ts(1 hunks)api/src/index.ts(2 hunks)api/src/store/actions/load-dynamix-config-file.ts(2 hunks)api/src/store/index.ts(2 hunks)api/src/store/modules/dynamix.ts(0 hunks)api/src/store/watch/dynamix-config-watch.ts(0 hunks)api/src/unraid-api/app/__test__/app.module.integration.spec.ts(2 hunks)packages/unraid-shared/src/index.ts(1 hunks)packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts(1 hunks)packages/unraid-shared/src/util/create-ttl-memoized-loader.ts(1 hunks)
💤 Files with no reviewable changes (2)
- api/src/store/modules/dynamix.ts
- api/src/store/watch/dynamix-config-watch.ts
🧰 Additional context used
📓 Path-based instructions (8)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
{**/*.test.ts,**/__test__/{components,store}/**/*.ts}: Use .rejects.toThrow() without arguments when asserting that async functions throw; avoid checking exact error message strings unless the message format is explicitly under test
Focus tests on observable behavior and outcomes, not implementation details such as exact error messages
Use await nextTick() for DOM update assertions and flushPromises() for complex async chains; always await async operations before asserting
Place module mock declarations (vi.mock) at the top level of the test file to avoid hoisting issues
Use factory functions in vi.mock calls to define mocks and avoid hoisting pitfalls
Use vi.spyOn() to specify return values or behavior of methods under test
Reset/clear mocks between tests using vi.clearAllMocks() (and vi.resetAllMocks() when appropriate) to ensure isolation
Do not rely on Nuxt auto-imports in tests; import required Vue utilities explicitly in test files
Remember that vi.mock calls are hoisted; avoid mixing mock declarations and module mocks incorrectly
Files:
packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.tsapi/src/__test__/core/utils/images/image-file-helpers.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript import specifiers with .js extensions for ESM compatibility
Never use the any type; prefer precise typing
Avoid type casting; model proper types from the start
Files:
packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.tspackages/unraid-shared/src/index.tspackages/unraid-shared/src/util/create-ttl-memoized-loader.tsapi/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/store/index.tsapi/src/store/actions/load-dynamix-config-file.tsapi/src/__test__/core/utils/images/image-file-helpers.test.tsapi/src/index.ts
api/src/unraid-api/**
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
Prefer adding new files to the Nest repo at api/src/unraid-api/ instead of legacy code
Files:
api/src/unraid-api/app/__test__/app.module.integration.spec.ts
api/**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/api-rules.mdc)
api/**/*.{test,spec}.{js,jsx,ts,tsx}: Use Vitest for tests in the api; do not use Jest
Prefer not to mock simple dependencies in tests
For error testing, use.rejects.toThrow()without arguments; avoid asserting exact error messages unless the message format is the subject under test
Files:
api/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/__test__/core/utils/images/image-file-helpers.test.ts
api/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
api/**/*.{test,spec}.{ts,tsx}: API test suite is Vitest; do not use Jest
Prefer not to mock simple dependencies in API tests
Files:
api/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/__test__/core/utils/images/image-file-helpers.test.ts
{api,web}/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
{api,web}/**/*.{test,spec}.{ts,tsx}: For error tests, use .rejects.toThrow() without arguments; avoid asserting exact error messages unless that format is the subject
Focus tests on behavior, not implementation details
Avoid brittle tests tied to exact error or log wording
Use mocks as nouns, not verbs
Always await async operations before making assertions
Place all mock declarations at the top level; use factory functions for module mocks; clear mocks between tests
Files:
api/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/__test__/core/utils/images/image-file-helpers.test.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 TTL values must be in milliseconds (e.g., 600000 for 10 minutes)
Files:
api/src/unraid-api/app/__test__/app.module.integration.spec.tsapi/src/store/index.tsapi/src/store/actions/load-dynamix-config-file.tsapi/src/__test__/core/utils/images/image-file-helpers.test.tsapi/src/index.ts
**/{store,stores}/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/web-testing-rules.mdc)
In store files, explicitly import Vue reactivity utilities (e.g., computed, ref, watchEffect); do not rely on Nuxt auto-imports
Files:
api/src/store/index.tsapi/src/store/actions/load-dynamix-config-file.ts
🧠 Learnings (3)
📚 Learning: 2025-08-11T15:10:28.150Z
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-08-11T15:10:28.150Z
Learning: This Nuxt app is tested with vitest outside the Nuxt runtime; structure tests accordingly
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.spec.ts
📚 Learning: 2025-09-12T01:36:59.838Z
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-12T01:36:59.838Z
Learning: Applies to api/**/*.{test,spec}.{ts,tsx} : API test suite is Vitest; do not use Jest
Applied to files:
api/src/unraid-api/app/__test__/app.module.integration.spec.ts
📚 Learning: 2025-01-22T18:34:06.925Z
Learnt from: elibosley
PR: unraid/api#1068
File: api/src/unraid-api/auth/api-key.service.ts:122-137
Timestamp: 2025-01-22T18:34:06.925Z
Learning: The store in app/store is implemented using Redux's configureStore, where dispatch operations for config updates are synchronous in-memory state updates that cannot fail, making transaction-like patterns unnecessary.
Applied to files:
api/src/store/index.ts
🧬 Code graph analysis (7)
packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts (2)
packages/unraid-shared/src/index.ts (1)
createTtlMemoizedLoader(7-7)packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
createTtlMemoizedLoader(45-82)
packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
packages/unraid-shared/src/index.ts (3)
CreateTtlMemoizedLoaderOptions(9-9)TtlMemoizedLoader(10-10)createTtlMemoizedLoader(7-7)
api/src/unraid-api/app/__test__/app.module.integration.spec.ts (1)
api/src/store/index.ts (1)
loadDynamixConfig(29-48)
api/src/store/index.ts (3)
api/src/store/actions/load-dynamix-config-file.ts (1)
loadDynamixConfigFromDiskSync(57-61)api/src/store/modules/dynamix.ts (1)
updateDynamixConfig(20-22)api/src/core/log.ts (1)
logger(50-80)
api/src/store/actions/load-dynamix-config-file.ts (4)
packages/unraid-shared/src/util/file.ts (1)
fileExistsSync(22-29)api/src/core/utils/misc/parse-config.ts (1)
parseConfig(146-203)packages/unraid-shared/src/util/create-ttl-memoized-loader.ts (1)
createTtlMemoizedLoader(45-82)api/src/core/types/ini.ts (1)
DynamixConfig(126-132)
api/src/__test__/core/utils/images/image-file-helpers.test.ts (1)
api/src/store/index.ts (1)
loadDynamixConfig(29-48)
api/src/index.ts (1)
api/src/store/index.ts (1)
loadDynamixConfig(29-48)
🔇 Additional comments (7)
api/src/index.ts (2)
77-77: Direct, synchronous config load on startup LGTM.Keeps reads scoped and avoids watch-induced atime bumps. No await needed since loader is sync.
22-22: Public API import verified—no lingering references to old thunk.Verification confirms
loadDynamixConfigFilehas no remaining usages in the codebase. The import at line 22 correctly usesloadDynamixConfigwith ESM.jsextension.packages/unraid-shared/src/util/__tests__/create-ttl-memoized-loader.test.ts (1)
1-72: Solid coverage and ESM-correct imports.Tests cover TTL reuse, expiry, keyed invalidation, and conditional caching. Nice use of
useFakeTimers()/useRealTimers().api/src/unraid-api/app/__test__/app.module.integration.spec.ts (1)
9-9: Import and init order LGTM.Importing
{ loadDynamixConfig, store }with.jsis correct, and loading config beforeloadStateFiles()is reasonable for these tests.Also applies to: 113-113
packages/unraid-shared/src/index.ts (1)
7-11: Public re-exports look good.ESM
.jsspecifiers andexport typeusage are correct; no side effects introduced.api/src/store/index.ts (2)
3-8: LGTM! Imports follow ESM guidelines.All imports correctly use
.jsextensions for ESM compatibility.
51-51: LGTM! Getter appropriately leverages TTL-cached loading.The shift from passive state access to active loading via
loadDynamixConfig()is well-suited to the FAT32 use case described in the PR objectives. The TTL memoization (250ms) ensures that consecutive accesses within short intervals don't trigger redundant disk reads, making this pattern both correct and performant.
![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.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| type ConfigPaths = readonly (string | undefined | null)[]; | ||
| const CACHE_WINDOW_MS = 250; | ||
|
|
||
| const memoizedConfigLoader = createTtlMemoizedLoader< | ||
| RecursivePartial<DynamixConfig>, | ||
| ConfigPaths, | ||
| string | ||
| >({ | ||
| ttlMs: CACHE_WINDOW_MS, | ||
| getCacheKey: (configPaths: ConfigPaths): string => JSON.stringify(configPaths), | ||
| load: (configPaths: ConfigPaths) => { | ||
| const validPaths = configPaths.filter((path): path is string => Boolean(path)); | ||
| if (validPaths.length === 0) { | ||
| return {}; | ||
| } | ||
| const configFiles = validPaths.map((path) => loadConfigFileSync<DynamixConfig>(path)); | ||
| return configFiles.reduce<RecursivePartial<DynamixConfig>>( | ||
| (accumulator, configFile) => ({ | ||
| ...accumulator, | ||
| ...configFile, | ||
| }), | ||
| {} | ||
| ); | ||
| }, | ||
| }); | ||
|
|
||
| /** | ||
| * Load the dynamix.cfg into the store. | ||
| * Loads dynamix config from disk with TTL caching. | ||
| * | ||
| * Note: If the file doesn't exist this will fallback to default values. | ||
| * @param configPaths - Array of config file paths to load and merge | ||
| * @returns Merged config object from all valid paths | ||
| */ | ||
| export const loadDynamixConfigFile = createAsyncThunk< | ||
| RecursiveNullable<RecursivePartial<DynamixConfig>>, | ||
| string | undefined | ||
| >('config/load-dynamix-config-file', async (filePath) => { | ||
| if (filePath) { | ||
| return loadConfigFile<DynamixConfig>(filePath); | ||
| } | ||
| const store = await import('@app/store/index.js'); | ||
| const paths = store.getters.paths()['dynamix-config']; | ||
| const { data: configs } = await batchProcess(paths, (path) => loadConfigFile<DynamixConfig>(path)); | ||
| const [defaultConfig = {}, customConfig = {}] = configs; | ||
| return { ...defaultConfig, ...customConfig }; | ||
| }); | ||
| export const loadDynamixConfigFromDiskSync = ( | ||
| configPaths: readonly (string | undefined | null)[] | ||
| ): RecursivePartial<DynamixConfig> => { | ||
| return memoizedConfigLoader.get(configPaths); |
There was a problem hiding this comment.
The new memoized loader keeps a 250 ms snapshot of the config (memoizedConfigLoader), but there is no way to invalidate that cache after the config file is modified. Callers such as CustomizationService.applyDisplaySettings read the current config through getters.dynamix() before writing updates and then immediately call getters.dynamix() again to return the updated theme. Because the cache is still valid, the second read reuses the pre‑update snapshot and the mutation responds with stale values until the TTL expires. Consider exposing a clear() or bypassing the cache when a write occurs so that reads performed in the same request reflect the new file contents.
Useful? React with 👍 / 👎.
🤖 I have created a release *beep* *boop* --- ## [4.25.3](v4.25.2...v4.25.3) (2025-10-22) ### Bug Fixes * flaky watch on boot drive's dynamix config ([ec7aa06](ec7aa06)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.release-please-manifest.json(1 hunks)api/CHANGELOG.md(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .release-please-manifest.json
🔇 Additional comments (1)
package.json (1)
4-4: Version bump is correct.The patch version increment (4.25.2 → 4.25.3) is appropriate for a bug fix.
| ## [4.25.3](https://github.com/unraid/unraid-api/compare/v4.25.2...v4.25.3) (2025-10-22) | ||
|
|
||
|
|
||
| ### Bug Fixes | ||
|
|
||
| * flaky watch on boot drive's dynamix config ([ec7aa06](https://github.com/unraid/unraid-api/commit/ec7aa06d4a5fb1f0e84420266b0b0d7ee09a3663)) |
There was a problem hiding this comment.
Fix the compare URL repository path
The new 4.25.3 entry points to https://github.com/unraid/unraid-api/compare/..., but the rest of this changelog—and the actual repo—use https://github.com/unraid/api/compare/.... That typo breaks the compare link for this release. Please change the URL to https://github.com/unraid/api/compare/v4.25.2...v4.25.3 to keep the changelog consistent and the link working.
🤖 Prompt for AI Agents
In api/CHANGELOG.md around lines 3 to 8, the compare URL for version 4.25.3 uses
the wrong repository path "unraid/unraid-api"; update the URL to use
"unraid/api/compare/v4.25.2...v4.25.3" so the compare link matches the
repository and the rest of the changelog; ensure only the repo segment is
changed and keep the rest of the line intact.
275e1de to
0a902b0
Compare
|
🚀 Storybook has been deployed to staging: https://unraid-ui-storybook-staging.unraid-workers.workers.dev |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
1 participant
On FAT32,
fs.stat()updates accesstime, which means file reads are also writes, which means we can't useusePollwithout degrading users' flash drives.To keep file reads lazy without a larger refactor, I override
getters.dynamix()as the entrypoint to re-read the boot drive's dynamix config.Consecutive calls to
getters.dynamix()are a common access pattern, which means we have to memoize to avoid many redundant file reads, so I used a TTL cache with a 250ms lifetime, hoping to scope config files to each request.getters.dynamix()is also used synchonously, so bit the bullet and switched away from async reads for simplicity, considering that most reads will be occurring from memory, even during cache misses.Summary by CodeRabbit
New Features
Refactor
Tests
Chores