Use app monitoring instead of polling to establish websocket connection #367
Conversation
WalkthroughThe pull request introduces significant changes to the TrackAudio Stream Deck plugin, focusing on application monitoring and event handling. The modifications include adding an "ApplicationsToMonitor" section to the manifest file, which specifies monitored applications for macOS and Windows. Additionally, it removes generic "actionAdded" event emissions across various action-related files and introduces new event handlers for application launch, termination, and system wake-up. These changes enhance the plugin's interaction with the Stream Deck system and the tracked applications. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Finishing Touches
🪧 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 (
|
|
Build for this pull request: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/events/streamDeck/applicationDidTerminate.ts (1)
6-6: Fix logger service nameThe logger service name should match the function name for consistency.
-const logger = mainLogger.child({ service: "handleOnApplicationDidTerminate" }); +const logger = mainLogger.child({ service: "applicationDidTerminate" });src/plugin.ts (1)
67-69: Add comment explaining application monitoring strategyAdd a comment block to explain the application monitoring approach for better maintainability.
-// Register the event handlers +// Monitor TrackAudio application lifecycle +// These handlers connect/disconnect when the TrackAudio application launches/terminates, +// replacing the previous polling mechanism for establishing websocket connections. streamDeck.system.onApplicationDidLaunch(handleOnApplicationDidLaunch); streamDeck.system.onApplicationDidTerminate(handleOnApplicationDidTerminate);com.neil-enns.trackaudio.sdPlugin/manifest.json (1)
133-136: Document application monitoring in READMEConsider adding documentation about the application monitoring feature in the project's README to help users understand how the plugin detects and connects to TrackAudio.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
com.neil-enns.trackaudio.sdPlugin/manifest.json(1 hunks)src/events/action/actionAdded.ts(0 hunks)src/events/action/removed.ts(0 hunks)src/events/streamDeck/applicationDidLaunch.ts(1 hunks)src/events/streamDeck/applicationDidTerminate.ts(1 hunks)src/events/streamDeck/atisLetter/addAtisLetter.ts(0 hunks)src/events/streamDeck/hotline/addHotline.ts(0 hunks)src/events/streamDeck/mainVolume/addMainVolume.ts(0 hunks)src/events/streamDeck/pushToTalk/addPushToTalk.ts(0 hunks)src/events/streamDeck/stationStatus/addStation.ts(0 hunks)src/events/streamDeck/stationVolume/addStationVolume.ts(0 hunks)src/events/streamDeck/trackAudioStatus/addTrackAudioStatus.ts(0 hunks)src/managers/action.ts(0 hunks)src/plugin.ts(3 hunks)
💤 Files with no reviewable changes (10)
- src/events/streamDeck/mainVolume/addMainVolume.ts
- src/events/streamDeck/hotline/addHotline.ts
- src/events/streamDeck/pushToTalk/addPushToTalk.ts
- src/events/streamDeck/stationStatus/addStation.ts
- src/events/streamDeck/atisLetter/addAtisLetter.ts
- src/events/action/actionAdded.ts
- src/events/streamDeck/trackAudioStatus/addTrackAudioStatus.ts
- src/events/action/removed.ts
- src/managers/action.ts
- src/events/streamDeck/stationVolume/addStationVolume.ts
🔇 Additional comments (1)
src/events/streamDeck/applicationDidLaunch.ts (1)
7-10: Verify application name before connectingThe handler should verify that the launched application matches one of the monitored applications before attempting to connect.
export const handleOnApplicationDidLaunch = (ev: ApplicationDidLaunchEvent) => { logger.info("Received applicationDidLaunch event", ev.application); + const monitoredApps = ["com.vatsim.trackaudio", "trackaudio.exe"]; + if (!monitoredApps.includes(ev.application)) { + logger.debug("Ignoring non-monitored application launch", ev.application); + return; + } trackAudioManager.connect(); };✅ Verification successful
Application validation check is required
The review comment is accurate. The manifest.json confirms that only "com.vatsim.trackaudio" (Mac) and "trackaudio.exe" (Windows) should trigger a connection. The suggested implementation correctly validates against these applications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Extract monitored applications from manifest.json # to verify application name validation jq -r '.ApplicationsToMonitor | (.mac[], .windows[])' com.neil-enns.trackaudio.sdPlugin/manifest.jsonLength of output: 140
| export const handleOnApplicationDidLaunch = (ev: ApplicationDidLaunchEvent) => { | ||
| logger.info("Received applicationDidLaunch event", ev.application); | ||
| trackAudioManager.connect(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for connect() call
The connection attempt should be wrapped in a try-catch block to handle potential connection failures gracefully.
export const handleOnApplicationDidLaunch = (ev: ApplicationDidLaunchEvent) => {
logger.info("Received applicationDidLaunch event", ev.application);
- trackAudioManager.connect();
+ try {
+ trackAudioManager.connect();
+ } catch (error) {
+ logger.error("Failed to connect to TrackAudio", error);
+ }
};📝 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.
| export const handleOnApplicationDidLaunch = (ev: ApplicationDidLaunchEvent) => { | |
| logger.info("Received applicationDidLaunch event", ev.application); | |
| trackAudioManager.connect(); | |
| }; | |
| export const handleOnApplicationDidLaunch = (ev: ApplicationDidLaunchEvent) => { | |
| logger.info("Received applicationDidLaunch event", ev.application); | |
| try { | |
| trackAudioManager.connect(); | |
| } catch (error) { | |
| logger.error("Failed to connect to TrackAudio", error); | |
| } | |
| }; |
| export const handleOnApplicationDidTerminate = ( | ||
| ev: ApplicationDidTerminateEvent | ||
| ) => { | ||
| logger.info("Received applicationDidTerminate event", ev.application); | ||
| trackAudioManager.disconnect(); | ||
| vatsimManager.stop(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and application validation
The handler should validate the application and handle potential errors from disconnect() and stop() calls.
export const handleOnApplicationDidTerminate = (
ev: ApplicationDidTerminateEvent
) => {
logger.info("Received applicationDidTerminate event", ev.application);
+ const monitoredApps = ["com.vatsim.trackaudio", "trackaudio.exe"];
+ if (!monitoredApps.includes(ev.application)) {
+ logger.debug("Ignoring non-monitored application termination", ev.application);
+ return;
+ }
+ try {
trackAudioManager.disconnect();
vatsimManager.stop();
+ } catch (error) {
+ logger.error("Error during cleanup", error);
+ }
};📝 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.
| export const handleOnApplicationDidTerminate = ( | |
| ev: ApplicationDidTerminateEvent | |
| ) => { | |
| logger.info("Received applicationDidTerminate event", ev.application); | |
| trackAudioManager.disconnect(); | |
| vatsimManager.stop(); | |
| }; | |
| export const handleOnApplicationDidTerminate = ( | |
| ev: ApplicationDidTerminateEvent | |
| ) => { | |
| logger.info("Received applicationDidTerminate event", ev.application); | |
| const monitoredApps = ["com.vatsim.trackaudio", "trackaudio.exe"]; | |
| if (!monitoredApps.includes(ev.application)) { | |
| logger.debug("Ignoring non-monitored application termination", ev.application); | |
| return; | |
| } | |
| try { | |
| trackAudioManager.disconnect(); | |
| vatsimManager.stop(); | |
| } catch (error) { | |
| logger.error("Error during cleanup", error); | |
| } | |
| }; |
|
Build for this pull request: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/events/streamDeck/systemDidWakeUp.ts (1)
4-8: Add error handling for better reliability.While the logging is good, consider adding try-catch blocks to handle potential errors during event processing.
export const handleOnSystemDidWakeUp = () => { const logger = mainLogger.child({ service: "systemDidWakeUp" }); logger.info("Received systemDidWakeUp event"); + + try { + // Connection logic here... + } catch (error) { + logger.error({ error }, "Failed to handle system wake up event"); + } };src/managers/trackAudio.ts (1)
51-66: Add validation in the isAppRunning setter.While the getter and setter are well documented, the setter lacks validation which could lead to inconsistent state.
Consider adding validation:
public set isAppRunning(newValue: boolean) { + if (typeof newValue !== 'boolean') { + throw new TypeError('isAppRunning must be a boolean value'); + } this._isAppRunning = newValue; }src/events/streamDeck/applicationDidTerminate.ts (1)
9-11: Use consistent logger service name.The service name in the child logger differs from the applicationDidLaunch handler.
const logger = mainLogger.child({ - service: "handleOnApplicationDidTerminate", + service: "applicationDidTerminate", });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/events/streamDeck/applicationDidLaunch.ts(1 hunks)src/events/streamDeck/applicationDidTerminate.ts(1 hunks)src/events/streamDeck/systemDidWakeUp.ts(1 hunks)src/managers/trackAudio.ts(2 hunks)src/plugin.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugin.ts
🔇 Additional comments (5)
src/events/streamDeck/systemDidWakeUp.ts (2)
1-2: LGTM! Clean imports using proper path aliasing.
9-14: Enhance connection reliability with logging and verification.While the basic reconnection logic is present, consider these improvements for better reliability and debugging:
// This ensures reconnection to the TrackAudio manager if somehow the websocket // connection doesn't automatically start back up after a system wake. if (trackAudioManager.isAppRunning) { + logger.debug("Attempting to reconnect to TrackAudio manager"); trackAudioManager.connect(); + + // Consider adding connection verification + // Consider implementing a retry mechanism with backoff }Let's verify if there's a retry mechanism or connection verification elsewhere in the codebase:
src/managers/trackAudio.ts (1)
32-32: LGTM! Property follows naming conventions.The
_isAppRunningprivate property aligns with the class's naming conventions and supports the transition to app monitoring.src/events/streamDeck/applicationDidLaunch.ts (1)
5-11:⚠️ Potential issueAdd application validation and error handling.
The handler needs to validate the application and handle potential connection errors.
Apply these improvements:
export const handleOnApplicationDidLaunch = (ev: ApplicationDidLaunchEvent) => { const logger = mainLogger.child({ service: "applicationDidLaunch" }); logger.info("Received applicationDidLaunch event", ev.application); + const monitoredApps = ["com.vatsim.trackaudio", "trackaudio.exe"]; + if (!monitoredApps.includes(ev.application)) { + logger.debug("Ignoring non-monitored application launch", ev.application); + return; + } + trackAudioManager.isAppRunning = true; - trackAudioManager.connect(); + try { + trackAudioManager.connect(); + } catch (error) { + logger.error("Failed to connect to TrackAudio", error); + trackAudioManager.isAppRunning = false; // Reset state on connection failure + } };Likely invalid or redundant comment.
src/events/streamDeck/applicationDidTerminate.ts (1)
6-17:⚠️ Potential issueAdd application validation and error handling for cleanup.
The handler should validate the application and handle potential errors during cleanup.
Apply these improvements:
export const handleOnApplicationDidTerminate = ( ev: ApplicationDidTerminateEvent ) => { const logger = mainLogger.child({ - service: "handleOnApplicationDidTerminate", + service: "applicationDidTerminate", }); logger.info("Received applicationDidTerminate event", ev.application); + const monitoredApps = ["com.vatsim.trackaudio", "trackaudio.exe"]; + if (!monitoredApps.includes(ev.application)) { + logger.debug("Ignoring non-monitored application termination", ev.application); + return; + } + trackAudioManager.isAppRunning = false; - trackAudioManager.disconnect(); - vatsimManager.stop(); + try { + trackAudioManager.disconnect(); + vatsimManager.stop(); + } catch (error) { + logger.error("Error during cleanup", error); + } };Likely invalid or redundant comment.
Fixes #366
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes