Fix state problems with main volume dial #389
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces modifications to several files related to track audio management and volume control. The changes primarily focus on adjusting connection state handling, refreshing volume controls, and updating feedback mechanisms. The modifications affect the main volume and station volume controllers, as well as the track audio connection and voice connected state event handlers. The changes refine the logic for managing audio connections and updating user interface elements accordingly. Changes
Possibly related issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (2)
src/events/trackAudio/mainVolumeChange.ts (1)
23-23: Enhance logging with controller context.While adding logging is good, including the controller identifier would make debugging easier, especially when multiple controllers are involved.
- logger.info(`Setting mainVolume to ${data.value.volume.toString()}`); + logger.info(`Setting mainVolume to ${data.value.volume.toString()} for controller ${entry.action.context}`);src/controllers/mainVolume.ts (1)
177-185: Consider extracting color values to constants.The color values are used in multiple places. Extracting them to constants would improve maintainability and consistency.
+const FEEDBACK_COLORS = { + CONNECTED: "white", + DISCONNECTED: "grey", +} as const; // Then use them in the feedback: title: { - color: "white", + color: FEEDBACK_COLORS.CONNECTED, }, indicator: { value: this.volume, - bar_fill_c: "white", + bar_fill_c: FEEDBACK_COLORS.CONNECTED, }, value: { value: `${this.volume.toString()}%`, - color: "white", + color: FEEDBACK_COLORS.CONNECTED, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
com.neil-enns.trackaudio.sdPlugin/images/actions/mainVolume/template.svgis excluded by!**/*.svg
📒 Files selected for processing (6)
.vscode/settings.json(2 hunks)src/controllers/mainVolume.ts(2 hunks)src/controllers/stationVolume.ts(1 hunks)src/events/trackAudio/connected.ts(0 hunks)src/events/trackAudio/mainVolumeChange.ts(1 hunks)src/events/trackAudio/voiceConnectedState.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/events/trackAudio/connected.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, lint and pack
🔇 Additional comments (4)
src/events/trackAudio/voiceConnectedState.ts (1)
11-11: LGTM! Good improvement to state management.Moving the main volume refresh to the voice connection handler ensures the volume state is synchronized at the right moment - when a voice connection is established.
src/controllers/mainVolume.ts (1)
140-143: LGTM! Good improvement to connection state check.Using
isVoiceConnectedinstead ofisConnectedis more precise and better reflects the actual state needed for volume control.src/controllers/stationVolume.ts (1)
259-259: LGTM! Consistent with mainVolume changes.The simplified condition using
isVoiceConnectedaligns with the changes in mainVolume.ts and correctly represents the state needed for volume control..vscode/settings.json (1)
25-25: LGTM! Domain-specific terms added to spell checker.The additions of "ATIS" and "vatsim" to the spell checker dictionary are appropriate for this aviation-focused audio management plugin.
Also applies to: 35-36
src/controllers/mainVolume.ts
Outdated
| logger.info( | ||
| `mainVolume refresh title, isVoiceConnected is ${ | ||
| trackAudioManager.isConnected ? "true" : "false" | ||
| }` | ||
| ); |
There was a problem hiding this comment.
Fix inconsistent connection check in logging.
The logging message uses isConnected while the code logic uses isVoiceConnected. This inconsistency could lead to confusing logs.
- `mainVolume refresh title, isVoiceConnected is ${
- trackAudioManager.isConnected ? "true" : "false"
- }`
+ `mainVolume refresh title, isVoiceConnected is ${
+ trackAudioManager.isVoiceConnected ? "true" : "false"
+ }`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.info( | |
| `mainVolume refresh title, isVoiceConnected is ${ | |
| trackAudioManager.isConnected ? "true" : "false" | |
| }` | |
| ); | |
| logger.info( | |
| `mainVolume refresh title, isVoiceConnected is ${ | |
| trackAudioManager.isVoiceConnected ? "true" : "false" | |
| }` | |
| ); |
|
Build for this pull request: |
Summary by CodeRabbit
Chores
Bug Fixes
Refactor