fix: event emitter setup for writing status #1496
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThis update shifts the registration of Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant MothershipModule
participant ConnectStatusWriterService
App->>MothershipModule: Bootstrap application
MothershipModule->>ConnectStatusWriterService: Initialize on application bootstrap
App->>MothershipModule: Emit MOTHERSHIP_CONNECTION_STATUS_CHANGED event
MothershipModule->>ConnectStatusWriterService: @OnEvent triggers writeStatus()
ConnectStatusWriterService->>FileSystem: Write status file
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (4)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts (3)
34-37: Consider a more robust approach for test file path override.While the property override works, it's somewhat fragile. Consider injecting the file path through the constructor or using a more explicit testing approach.
- // Override the status file path to use our test location - Object.defineProperty(service, 'statusFilePath', { - get: () => testFilePath, - }); + // Consider passing testFilePath as a constructor parameter + // or using dependency injection for better testability
23-23: Remove debug console.log statements.These console.log statements appear to be debugging artifacts and should be removed before merging to production.
- console.log(`ConfigService.get called with key: ${key}`); - console.log('=== Starting onApplicationBootstrap ==='); - console.log('Initial status written:', initialData); - console.log('=== Calling writeStatus directly ==='); - console.log('Updated status after writeStatus:', updatedData); - console.log(`Returning status ${changeIndex}: ${change.status}`); - console.log(`=== Calling writeStatus for final status: ${statusChanges[changeIndex].status} ===`); - console.log('Final status after status change:', finalData); - console.log(`writeStatus called at ${timestamp}`); - console.log(`Initial writes: ${initialWrites}`); - console.log(`Calling writeStatus ${i}`); - console.log(`Total writes: ${writes.length}`); - console.log('Write timestamps:', writes);Also applies to: 48-48, 57-57, 70-70, 76-76, 94-94, 103-103, 109-109, 123-123, 131-131, 135-135, 139-140
116-144: Creative approach to testing multiple writes.The method override to track write calls is clever, though consider using a spy instead for cleaner test code.
- const writes: number[] = []; - const originalWriteStatus = service['writeStatus'].bind(service); - - service['writeStatus'] = async function() { - const timestamp = Date.now(); - writes.push(timestamp); - console.log(`writeStatus called at ${timestamp}`); - return originalWriteStatus(); - }; + const writeStatusSpy = vi.spyOn(service as any, 'writeStatus');packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts (1)
30-30: Remove debug console.log statements.These console.log statements should be removed before merging to production.
- console.log(`ConfigService.get('${key}') called, returning:`, configStore[key]); - console.log(`ConfigService.set('${key}', ${JSON.stringify(value)}) called`); - console.log('Initial status:', data); - console.log('\n=== Updating config to CONNECTED ==='); - console.log('Status after config update:', data); - console.log(`\n=== Updating to ${update.status} ===`); - console.log(`Status file shows: ${data.connectionStatus}`); - console.log('\n=== Calling writeStatus with no config ==='); - console.log('Status with no config:', data); - console.log('\n=== Setting config and calling writeStatus ==='); - console.log('Status after setting config:', data2);Also applies to: 34-34, 59-59, 63-63, 75-75, 92-92, 102-102, 115-115, 120-120, 124-124, 134-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.claude/settings.local.json(1 hunks)packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts(1 hunks)packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts(1 hunks)packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.ts(1 hunks)packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .claude/settings.local.json
- packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: TypeScript imports use .js extensions for ESM compatibility
**/*.{ts,tsx}: TypeScript imports use .js extensions for ESM compatibility
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.tspackages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.tspackages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts
🧠 Learnings (3)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.ts (17)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Mock external services and API calls in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes by updating the store in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes after actions in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Mock external dependencies and services in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Mock external dependencies appropriately in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test action side effects and state changes in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test async operations completely in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test getter dependencies are properly mocked in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify proper error handling in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test store interactions with other stores in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to web/test/mocks/**/*.ts : Frequently used mocks are stored under `web/test/mocks`
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Use `vi.mock()` for module-level mocks in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Remember that `vi.mock()` calls are hoisted
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T14:50:43.946Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-06-30T14:50:52.281Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Test suite is VITEST, do not use jest
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Clear mocks between tests to ensure isolation
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Use factory functions for module mocks to avoid hoisting issues in test files
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts (14)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes by updating the store in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes after actions in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Mock external services and API calls in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Mock external dependencies and services in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Mock external dependencies appropriately in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test getter dependencies are properly mocked in store tests
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T14:50:43.946Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test action side effects and state changes in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Test component interactions (clicks, inputs, etc.) in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Don't mix mock declarations and module mocks incorrectly in test files
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Clear mocks between tests to ensure isolation
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to web/test/mocks/**/*.ts : Frequently used mocks are stored under `web/test/mocks`
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Check for expected prop handling and event emissions in component tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify proper error handling in store tests
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts (17)
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes by updating the store in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Mock external services and API calls in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify state changes after actions in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Mock external dependencies appropriately in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Mock external dependencies and services in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test getter dependencies are properly mocked in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test async operations completely in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test store interactions with other stores in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Test action side effects and state changes in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/__test__/store/**/*.ts : Verify proper error handling in store tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to web/test/mocks/**/*.ts : Frequently used mocks are stored under `web/test/mocks`
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Use `vi.mock()` for module-level mocks in tests
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Remember that `vi.mock()` calls are hoisted
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Reset mocks between tests with `vi.clearAllMocks()`
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-06-30T14:52:37.535Z
Learning: Applies to **/*.test.ts : Clear mocks between tests to ensure isolation
Learnt from: CR
PR: unraid/api#0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-06-30T14:50:52.281Z
Learning: Applies to api/**/*.{test,spec}.{js,ts,tsx} : Test suite is VITEST, do not use jest
Learnt from: CR
PR: unraid/api#0
File: CLAUDE.md:0-0
Timestamp: 2025-06-30T14:50:43.946Z
Learning: Applies to api/**/*.{test,spec}.{ts,js} : Test suite is VITEST, do not use jest in the API package
🪛 Biome (1.9.4)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts
[error] 112-112: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build API
- GitHub Check: Build Unraid UI Library (Webcomponent Version)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.service.spec.ts (5)
1-11: Good module setup and ESM compliance.The imports correctly use
.jsextensions for ESM compatibility, and the filesystem mocking is properly configured using Vitest'svi.mock().
19-39: Well-structured test setup with proper mock isolation.The
beforeEachandafterEachhooks correctly clear mocks and manage fake timers. The ConfigService mock provides realistic connection metadata for testing.
52-61: Event-driven test correctly accesses private method.The test appropriately uses bracket notation to access the private
writeStatusmethod, which is necessary for testing the event-driven behavior since the@OnEventdecorator can't be directly triggered in unit tests.
105-114: Excellent error handling verification.The test properly verifies that write failures are handled gracefully without throwing exceptions, which is crucial for service reliability.
132-139: Comprehensive cleanup testing.This test ensures the service handles cleanup correctly even when no file was written, which is important for preventing errors during module shutdown.
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.integration.spec.ts (1)
82-114: Good test coverage for rapid status changes.This test effectively simulates real-world scenarios where connection status can change rapidly. The test logic is sound and verifies the service handles state transitions correctly.
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts (3)
15-37: Well-designed config store mock.The configStore approach effectively simulates real ConfigService behavior with proper get/set methods. The implementation is clean and testable.
138-157: Excellent cleanup testing with proper file verification.The cleanup tests properly verify file existence before and after cleanup, ensuring the service handles both normal and edge cases correctly.
79-105: Comprehensive status update flow testing.This test effectively validates the service's ability to handle multiple sequential status updates, which is crucial for real-world usage patterns.
packages/unraid-api-plugin-connect/src/connection-status/connect-status-writer.config.spec.ts
Show resolved
Hide resolved
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
pujitm
approved these changes
Jul 9, 2025
mdatelle
approved these changes
Jul 9, 2025
elibosley
pushed a commit
that referenced
this pull request
Jul 9, 2025
🤖 I have created a release *beep* *boop* --- ## [4.9.4](v4.9.3...v4.9.4) (2025-07-09) ### Bug Fixes * backport `<unraid-modals>` upon plg install when necessary ([#1499](#1499)) ([33e0b1a](33e0b1a)) * DefaultPageLayout patch rollback omits legacy header logo ([#1497](#1497)) ([ea20d1e](ea20d1e)) * event emitter setup for writing status ([#1496](#1496)) ([ca4e2db](ca4e2db)) --- 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>
Draft
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit