Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The current “Save & Reconnect” flow may not actually reconnect when a socket is OPEN/CONNECTING, and teardown() doesn’t fully clean up the active WebSocket connection/handlers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
spicetify-connect-api.js:613
- For accessibility, associate this label with its corresponding input so screen readers and click-to-focus work (set
htmlForto the input’s id).
keyLabel.textContent = "API Key / Secret Token (Optional)";
keyLabel.style.display = "block";
keyLabel.style.marginBottom = "4px";
keyLabel.style.fontWeight = "bold";
spicetify-connect-api.js:641
- For accessibility, associate this label with its corresponding input so screen readers and click-to-focus work (set
htmlForto the input’s id).
intervalLabel.textContent = "Reconnect Interval (ms)";
intervalLabel.style.display = "block";
intervalLabel.style.marginBottom = "4px";
intervalLabel.style.fontWeight = "bold";
- Files reviewed: 1/1 changed files
- Comments generated: 5
- Review effort level: Lite
Comment on lines
+550
to
+565
| function teardown() { | ||
| if (statePollInterval) { | ||
| clearInterval(statePollInterval); | ||
| statePollInterval = null; | ||
| } | ||
| if (reconnectTimer) { | ||
| clearTimeout(reconnectTimer); | ||
| reconnectTimer = null; | ||
| } | ||
| if (volumeDebounceTimer) { | ||
| clearTimeout(volumeDebounceTimer); | ||
| volumeDebounceTimer = null; | ||
| } | ||
| stopHeartbeat(); | ||
| isInitialized = false; | ||
| } |
Comment on lines
+670
to
+684
| saveBtn.onclick = () => { | ||
| const newUrl = urlInput.value.trim(); | ||
| const newApiKey = keyInput.value.trim(); | ||
| const newInterval = intervalInput.value.trim(); | ||
|
|
||
| if (newUrl) Spicetify.LocalStorage.set(STORAGE_KEYS.SERVER_URL, newUrl); | ||
| Spicetify.LocalStorage.set(STORAGE_KEYS.API_KEY, newApiKey); | ||
| if (newInterval) Spicetify.LocalStorage.set(STORAGE_KEYS.RECONNECT_INTERVAL, newInterval); | ||
|
|
||
| Spicetify.PopupModal.hide(); | ||
| Spicetify.showNotification("Settings saved! Reconnecting..."); | ||
|
|
||
| if (reconnectTimer) clearTimeout(reconnectTimer); | ||
| connect(); | ||
| }; |
Comment on lines
+583
to
+587
| urlLabel.textContent = "WebSocket Server URL"; | ||
| urlLabel.style.display = "block"; | ||
| urlLabel.style.marginBottom = "4px"; | ||
| urlLabel.style.fontWeight = "bold"; | ||
|
|
|
|
||
| const urlHelp = document.createElement("small"); | ||
| urlHelp.style.color = "#aaa"; | ||
| urlHelp.innerHTML = "Use <code>ws://127.0.0.1:9090</code> for local, <code>wss://IP:PORT:9090</code> for encrypted connections or <code>wss://DOMAIN:PORT</code> for remote connections."; |
Comment on lines
+732
to
739
| closeBtn.onmouseenter = () => { | ||
| closeBtn.style.color = "#fff"; | ||
| closeBtn.style.background = "rgba(255, 255, 255, 0.1)"; | ||
| }; | ||
| closeBtn.onmouseleave = () => { | ||
| closeBtn.style.color = "#b3b3b3"; | ||
| closeBtn.style.background = "transparent"; | ||
| }; |
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
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.
This pull request updates the
spicetify-connect-api.jsextension to version 0.3.2, focusing on improved connection handling, volume/mute logic, parameter validation, and a more robust, accessible settings UI. The changes enhance reliability, security awareness, and user experience.Key changes include:
Connection and Lifecycle Management
teardownfunction to clean up timers and listeners, and registered it onwindow.beforeunloadfor graceful shutdowns. Improved initialization guard with anisInitializedflag to prevent duplicate event listener setup. [1] [2] [3] [4]wss://for remote connections. [1] [2]Volume and Mute Handling
preMuteVolume), and ensured volume changes are tracked more accurately. Also updated the volume event polling to initialize state and debounce updates. [1] [2] [3] [4] [5]Command and Parameter Validation
SetVolume,Seek) to prevent invalid or malformed commands from causing errors. [1] [2] [3]Settings UI Improvements
innerHTML, improving accessibility, maintainability, and style consistency. Enhanced the close button styling and made the UI more robust. [1] [2]Miscellaneous Updates
These improvements make the extension more reliable, secure, and user-friendly.