refactor(api): simplify registration and var watcher flow#1934
refactor(api): simplify registration and var watcher flow#1934
Conversation
WalkthroughRemoved fingerprint/retry logic for registration reloads, replaced with a direct two-step reload (load var state, then load registration). Added a serialized watchQueue for chokidar events and made the watcher ignore non- Changes
Sequence DiagramsequenceDiagram
participant FS as File System
participant Chokidar as Chokidar Watcher
participant Queue as watchQueue
participant WatchHandler as Registration Watch Handler
participant Store as Vuex Store
participant Registration as Registration Module
FS->>Chokidar: file event (var.ini or .key)
Chokidar->>Queue: enqueue event (serialize)
Queue->>WatchHandler: dequeue -> invoke reloadVarIniWithRetry()
WatchHandler->>Store: dispatch loadSingleStateFile(StateFileKey.var)
Store->>Store: update var state
WatchHandler->>Store: dispatch loadRegistrationKey()
Store->>Registration: request registration reload
Registration-->>Store: registration data loaded
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: cd54fb4002
ℹ️ 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 (stateFile === StateFileKey.var) { | ||
| await store.dispatch(loadRegistrationKey()); |
There was a problem hiding this comment.
var.ini is the general emhttp state file, not a registration-only file: api/src/store/state-parsers/var.ts includes unrelated fields like mdResyncPos, sbEvents, and shareMoverActive, so this branch now dispatches loadRegistrationKey() for routine array/mover updates as well as real license changes. loadRegistrationKey() ultimately reads the key from paths['keyfile-base'] (/boot/config in api/src/store/modules/paths.ts:56), which means steady-state var.ini churn will keep re-reading and re-encoding the flash-resident .key file even when regFile never changed. Before this refactor we only did that work on startup or actual key-file events, so this introduces unnecessary flash I/O in production.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/src/store/watch/registration-watch.ts (2)
26-27: Silent error swallowing may hide failures.The
.catch(() => undefined)discards any errors from the previous queue entry, including errors fromreloadVarIniWithRetry(). This could make debugging difficult if dispatches fail silently. Consider logging errors before discarding:♻️ Suggested improvement
watchQueue = watchQueue - .catch(() => undefined) + .catch((error) => { + keyServerLogger.error('Previous key watch handler failed: %o', error); + }) .then(async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/store/watch/registration-watch.ts` around lines 26 - 27, The current chain assigns watchQueue = watchQueue.catch(() => undefined), which swallows errors (including those from reloadVarIniWithRetry()) and hides failures; change this to log the error before returning undefined so failures are visible—e.g., replace the empty catch with a handler that logs the caught error (using your existing logger) and then returns undefined, referencing the watchQueue promise and reloadVarIniWithRetry() as the implicated operations to locate the code to update.
34-34: The return statement has no effect on chokidar behavior.Chokidar's event handlers don't use return values. This
return watchQueue;line only serves to make the handler awaitable in tests. Consider adding a brief comment to clarify the intent, so future maintainers don't remove it thinking it's dead code.📝 Suggested clarification
+ // Return the queue promise to allow testing to await completion return watchQueue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/store/watch/registration-watch.ts` at line 34, The return statement "return watchQueue;" inside the chokidar event handler has no effect on chokidar but is kept so tests can await the handler; add a short inline comment above or beside that return explaining this intent (mentioning that chokidar ignores handler return values and that watchQueue is returned solely to make the handler awaitable in tests) so future maintainers won't remove it; locate the return in the registration-watch.ts chokidar callback where watchQueue is referenced and update the comment there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/store/watch/registration-watch.ts`:
- Around line 26-27: The current chain assigns watchQueue = watchQueue.catch(()
=> undefined), which swallows errors (including those from
reloadVarIniWithRetry()) and hides failures; change this to log the error before
returning undefined so failures are visible—e.g., replace the empty catch with a
handler that logs the caught error (using your existing logger) and then returns
undefined, referencing the watchQueue promise and reloadVarIniWithRetry() as the
implicated operations to locate the code to update.
- Line 34: The return statement "return watchQueue;" inside the chokidar event
handler has no effect on chokidar but is kept so tests can await the handler;
add a short inline comment above or beside that return explaining this intent
(mentioning that chokidar ignores handler return values and that watchQueue is
returned solely to make the handler awaitable in tests) so future maintainers
won't remove it; locate the return in the registration-watch.ts chokidar
callback where watchQueue is referenced and update the comment there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 584b3506-5db0-4111-8eaf-156652df0f2d
📒 Files selected for processing (4)
api/src/__test__/store/watch/registration-watch.test.tsapi/src/__test__/store/watch/state-watch.test.tsapi/src/store/watch/registration-watch.tsapi/src/store/watch/state-watch.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1934 +/- ##
==========================================
- Coverage 51.61% 51.52% -0.10%
==========================================
Files 1021 1021
Lines 70477 70446 -31
Branches 7763 7804 +41
==========================================
- Hits 36379 36299 -80
- Misses 33976 34025 +49
Partials 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/src/__test__/store/watch/registration-watch.test.ts (1)
72-79: Assert exact dispatch count to lock the two-step refresh contract.You already assert order; adding count assertions will also catch accidental extra dispatches in future changes.
Proposed tightening
await reloadVarIniWithRetry(); + expect(store.dispatch).toHaveBeenCalledTimes(2); expect(store.dispatch).toHaveBeenNthCalledWith(1, loadSingleStateFile(StateFileKey.var)); expect(store.dispatch).toHaveBeenNthCalledWith(2, loadRegistrationKey()); @@ await registrationKeyWatchHandler?.('add', '/boot/config/Pro.key'); + expect(store.dispatch).toHaveBeenCalledTimes(2); expect(store.dispatch).toHaveBeenNthCalledWith(1, loadSingleStateFile(StateFileKey.var)); expect(store.dispatch).toHaveBeenNthCalledWith(2, loadRegistrationKey());Also applies to: 96-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/__test__/store/watch/registration-watch.test.ts` around lines 72 - 79, The tests for reloadVarIniWithRetry assert call order but not total dispatch count, so add an exact dispatch count assertion to prevent extra dispatches: after invoking reloadVarIniWithRetry() assert store.dispatch was called exactly twice (e.g., expect(store.dispatch).toHaveBeenCalledTimes(2)) in the test that currently checks order using loadSingleStateFile(StateFileKey.var) and loadRegistrationKey(); apply the same tightening to the other similar test that checks the two-step refresh around loadRegistrationKey to ensure both order and exact call count.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/src/__test__/store/watch/registration-watch.test.ts`:
- Around line 72-79: The tests for reloadVarIniWithRetry assert call order but
not total dispatch count, so add an exact dispatch count assertion to prevent
extra dispatches: after invoking reloadVarIniWithRetry() assert store.dispatch
was called exactly twice (e.g., expect(store.dispatch).toHaveBeenCalledTimes(2))
in the test that currently checks order using
loadSingleStateFile(StateFileKey.var) and loadRegistrationKey(); apply the same
tightening to the other similar test that checks the two-step refresh around
loadRegistrationKey to ensure both order and exact call count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fdf48828-efba-4607-89e2-09d46a8d0c2a
📒 Files selected for processing (1)
api/src/__test__/store/watch/registration-watch.test.ts
1 participant
Summary
Notes
Summary by CodeRabbit
Tests
Refactor