Fix the multiple socket connection issue when opening the widget immediately #1388
Conversation
WalkthroughAdds explicit socket lifecycle state and race guards in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant W as Widget (mck-sidebox)
participant S as ALSocket
Note over W: IS_SOCKET_INITIALIZED=false\nIS_SOCKET_CONNECTED=false\nMCK_BOT_MESSAGE_QUEUE=[]
U->>W: launcher click / initializeSocketConnection(reInit?)
alt Already initialized and not reInit
W-->>U: no-op
else Start init
W->>W: IS_SOCKET_INITIALIZED = true
rect rgba(200,230,255,0.18)
W->>S: connect()
alt connect success
S-->>W: onConnect
W->>W: IS_SOCKET_CONNECTED = true\nIS_SOCKET_INITIALIZED = false
else connect failure
S-->>W: onConnectFailed
W->>W: IS_SOCKET_CONNECTED = false\nIS_SOCKET_INITIALIZED = false
end
end
end
sequenceDiagram
autonumber
participant UI as User UI
participant W as Widget
participant S as ALSocket
Note over W: CONNECT_SOCKET_ON_WIDGET_CLICK may trigger\nSOCKET_DISCONNECT_PROCEDURE when closing
UI->>W: user closes widget
W->>W: start SOCKET_DISCONNECT_PROCEDURE
W->>S: disconnect()
S-->>W: onDisconnect
W->>W: IS_SOCKET_CONNECTED = false\nIS_SOCKET_INITIALIZED = false
Note right of W: Later operations check\nif !S.isSocketActive() -> connectToSocket(true)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
webplugin/js/app/mck-sidebox-1.0.js (3)
621-626: Keep state consistent even if disconnect throwsWrap ALSocket.disconnect() in try/finally so flags are always reset.
- window.Applozic.ALSocket.disconnect(); - window.Applozic.SOCKET_DISCONNECT_PROCEDURE.DISCONNECTED = true; - IS_SOCKET_INITIALIZING = false; - IS_SOCKET_CONNECTED = false; - console.debug('disconnected'); + try { + window.Applozic.ALSocket.disconnect(); + window.Applozic.SOCKET_DISCONNECT_PROCEDURE.DISCONNECTED = true; + } catch (e) { + console.error('[KM] ALSocket.disconnect() threw:', e); + } finally { + IS_SOCKET_INITIALIZING = false; + IS_SOCKET_CONNECTED = false; + console.debug('disconnected'); + }
720-723: Reset retry counter on successful connectPrevents stale SOCKET_RECONNECT_FAIL_COUNT from showing disconnect UI unnecessarily.
IS_SOCKET_INITIALIZING = false; IS_SOCKET_CONNECTED = true; - console.debug('connected..'); + console.debug('[KM] connected'); + SOCKET_RECONNECT_FAIL_COUNT = 0;
1218-1220: Logout path: also clear connected flag and guard disconnectEnsure consistent state even if disconnect throws.
- window.Applozic.ALSocket.disconnect(); - IS_SOCKET_INITIALIZING = false; + try { + window.Applozic.ALSocket.disconnect(); + } catch (e) { + console.error('[KM] ALSocket.disconnect() threw during logout:', e); + } finally { + IS_SOCKET_INITIALIZING = false; + IS_SOCKET_CONNECTED = false; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webplugin/js/app/mck-sidebox-1.0.js(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T05:44:56.580Z
Learnt from: panwaranuj01
PR: Kommunicate-io/Kommunicate-Web-SDK#1320
File: webplugin/js/app/voice/mck-voice.js:758-758
Timestamp: 2025-06-25T05:44:56.580Z
Learning: The code in webplugin/js/app/voice/mck-voice.js runs in an isolated scope and does not pollute the global namespace of other windows or the host page. Global variables within this isolated scope are acceptable as they don't affect the embedding environment.
Applied to files:
webplugin/js/app/mck-sidebox-1.0.js
🔇 Additional comments (1)
webplugin/js/app/mck-sidebox-1.0.js (1)
14-14: Good: single-flight guard for socket initThe IS_SOCKET_INITIALIZING flag is the right approach to serialize init/reconnect.
|
A friendly reminder that this PR had no activity for 15 days. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
webplugin/js/app/mck-sidebox-1.0.js (2)
700-712: Unblock recovery: reset flag before returning and stop rethrowing here.Line 701 still exits before clearing
IS_SOCKET_INITIALIZING, so any caller that hits this branch will leave the flag stucktrue, blocking all future init/reconnect attempts. In the same block, rethrowing the sync reconnect error (Line 711) aborts the fail-count/backoff path, undoing the main objective of the PR. This was raised earlier and remains unresolved. Please ensure the flags are cleared beforereturnand keep the catch non-throwing so the retry flow continues.- kommunicate.reloadWidget(); - return; - } - IS_SOCKET_INITIALIZING = false; - IS_SOCKET_CONNECTED = false; + IS_SOCKET_INITIALIZING = false; + IS_SOCKET_CONNECTED = false; + kommunicate.reloadWidget(); + return; + } + IS_SOCKET_INITIALIZING = false; + IS_SOCKET_CONNECTED = false; if (navigator.onLine) { try { IS_SOCKET_INITIALIZING = true; window.Applozic.ALSocket.reconnect(); } catch (error) { IS_SOCKET_INITIALIZING = false; - throw error; + console.error('[KM] Socket reconnect threw synchronously:', error); } }
13606-13615: GuardstompClientaccess and avoid rethrowing reconnect errors.This block still dereferences
window.Applozic.ALSocket.stompClient.connectedwithout verifying the client exists, so a null client crashes with a TypeError and we never reach the retry logic. Additionally, rethrowing the sync reconnect error (Line 13615) unwinds the handler and prevents the recovery/backoff just added. Please add the null guard and keep the catch non-throwing.- if (!window.Applozic.ALSocket.stompClient.connected) { + if ( + !window.Applozic.ALSocket.stompClient || + !window.Applozic.ALSocket.stompClient.connected + ) { if (IS_SOCKET_INITIALIZING) { return; } try { IS_SOCKET_INITIALIZING = true; window.Applozic.ALSocket.reconnect(); } catch (error) { IS_SOCKET_INITIALIZING = false; - throw error; + console.error('[KM] Socket reconnect threw synchronously:', error); } }
🧹 Nitpick comments (2)
playwright_test/tests/socketConnection.spec.js (2)
1-6: LGTM! Consider adding JSDoc for the URL filter.The imports and
isWidgetSocketUrlfilter are well-structured and appropriate for the test suite.Optionally, add JSDoc to document the URL patterns:
+/** + * Checks if a WebSocket URL is related to the Kommunicate widget. + * Matches URLs containing '/stomp' or 'kommunicate.io/ws' or 'kommunicate.net/ws'. + * @param {string} url - The WebSocket URL to check + * @returns {boolean} True if the URL matches widget socket patterns + */ const isWidgetSocketUrl = (url) => /\/stomp/i.test(url) || /kommunicate\.(?:io|net)\/ws/i.test(url);
8-46: LGTM! Minor strictness improvement.The
createSocketTrackerutility is well-designed and provides clear methods for tracking socket lifecycle. The event handling and filtering logic are correct.Consider using strict equality for consistency:
const waitForRecordClose = async (record) => { - if (record.closedAt != null) { + if (record.closedAt !== null) { return; } await new Promise((resolve) => record.ws.on('close', resolve)); };
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playwright_test/tests/socketConnection.spec.js(1 hunks)webplugin/js/app/mck-sidebox-1.0.js(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T05:44:56.580Z
Learnt from: panwaranuj01
PR: Kommunicate-io/Kommunicate-Web-SDK#1320
File: webplugin/js/app/voice/mck-voice.js:758-758
Timestamp: 2025-06-25T05:44:56.580Z
Learning: The code in webplugin/js/app/voice/mck-voice.js runs in an isolated scope and does not pollute the global namespace of other windows or the host page. Global variables within this isolated scope are acceptable as they don't affect the embedding environment.
Applied to files:
webplugin/js/app/mck-sidebox-1.0.js
🔇 Additional comments (1)
playwright_test/tests/socketConnection.spec.js (1)
70-74: Verify global variable existence and extract hardcoded iframe name.The function assumes
window.IS_SOCKET_CONNECTEDexists in the iframe context. If this variable is undefined or the widget doesn't set it, the test will hang until timeout.Extract the hardcoded iframe name to a constant:
+const WIDGET_IFRAME_NAME = 'Kommunicate widget iframe'; + const waitForSocketStatus = async (page, expectedStatus) => { - const iframe = await page.frame({ name: 'Kommunicate widget iframe' }); + const iframe = await page.frame({ name: WIDGET_IFRAME_NAME }); await iframe.waitForFunction((status) => window.IS_SOCKET_CONNECTED === status, expectedStatus); return iframe; };Additionally, verify that
IS_SOCKET_CONNECTEDis reliably set by the widget:
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
webplugin/js/app/mck-sidebox-1.0.js (1)
1841-1847: Don’t rethrow from init/reconnect catch; guarantee flag reset before any early returnRethrowing aborts backoff/retry and can leave flags stuck. Ensure IS_SOCKET_INITIALIZING resets on all paths and avoid rethrow, as previously noted.
Run to verify there’s no
throwin ALSocket init/reconnect catch blocks and flags reset before returns:#!/bin/bash # Catch blocks around ALSocket init/reconnect that rethrow or don't reset flags rg -nP -C3 '(try\s*\{(?s).*?ALSocket\.(init|reconnect)\(.*\).*?\}\s*catch\s*\(\w+\)\s*\{(?s:.*?)throw\s+)' --type=js webplugin/js/app/mck-sidebox-1.0.js # Ensure IS_SOCKET_INITIALIZING is reset near early returns after init/reconnect attempts rg -nP -C2 '(IS_SOCKET_INITIALIZING\s*=\s*false).*?\n.*return;' --type=js webplugin/js/app/mck-sidebox-1.0.jsplaywright_test/tests/socketConnection.spec.js (4)
54-59: Use cross‑platform, deterministic input (avoid Meta+A).Use locator.fill() to replace value directly; this removes OS‑specific shortcuts and reduces flakiness. Based on learnings.
- await page.click(LOCATORS.appIdField); - await page.keyboard.press('Meta+A'); - await page.type(LOCATORS.appIdField, APP_ID.kmAppId); + await page.fill(LOCATORS.appIdField, APP_ID.kmAppId); await page.click(LOCATORS.scriptFiled); - await page.keyboard.press('Meta+A'); - await page.keyboard.press('Delete'); - await page.type(LOCATORS.scriptFiled, SCRIPT.kmAllBooleanIsTrue); + await page.fill(LOCATORS.scriptFiled, SCRIPT.kmAllBooleanIsTrue);
91-91: Replace arbitrary timeouts with condition‑based waits.These sleeps are flaky and slow. Rely on element state or a specific readiness signal you already wait for (close button visible/hidden, socket status). Based on learnings.
- await page.waitForTimeout(2000); - await page.waitForTimeout(500); - await page.waitForTimeout(500); - await page.waitForTimeout(1000); - await page.waitForTimeout(500); - await page.waitForTimeout(500); - await page.waitForTimeout(500); - await page.waitForTimeout(300);Example replacements already present in your code:
- Use await closeButton.waitFor({ state: 'visible' }) and state: 'hidden' instead of 500ms sleeps.
- For stabilization after socket connect, prefer a deterministic selector inside iframe (e.g., a “ready” attribute) or reuse waitForSocketStatus.
Also applies to: 95-95, 102-102, 128-128, 136-136, 159-159, 168-168, 176-176
108-112: Guard ALSocket.disconnect with a function check.Avoid throwing if ALSocket exists but disconnect is missing/not a function.
- await iframe.evaluate(() => { - if (window.Applozic?.ALSocket) { - window.Applozic.ALSocket.disconnect(); - } - }); + await iframe.evaluate(() => { + const disconnect = window.Applozic?.ALSocket?.disconnect; + if (typeof disconnect === 'function') { + disconnect.call(window.Applozic.ALSocket); + } + });Apply the same change to the other two cleanup blocks:
- await iframe.evaluate(() => { - if (window.Applozic?.ALSocket) { - window.Applozic.ALSocket.disconnect(); - } - }); + await iframe.evaluate(() => { + const disconnect = window.Applozic?.ALSocket?.disconnect; + if (typeof disconnect === 'function') { + disconnect.call(window.Applozic.ALSocket); + } + });- await iframe.evaluate(() => { - if (window.Applozic?.ALSocket) { - window.Applozic.ALSocket.disconnect(); - } - }); + await iframe.evaluate(() => { + const disconnect = window.Applozic?.ALSocket?.disconnect; + if (typeof disconnect === 'function') { + disconnect.call(window.Applozic.ALSocket); + } + });Also applies to: 144-146, 201-203
153-177: Extract openWidget/closeWidget helpers to top‑level for reuse and clarity.These inline helpers duplicate logic and make tests longer. Move them to a shared creator (returning openWidget/closeWidget) and reuse across tests.
Example:
// Top-level (near configureWidgetPreview) const createWidgetHelpers = (launcher, getCloseButtonLocator, tracker) => ({ openWidget: async ({ expectNewSocket }) => { const before = tracker.socketRecords.length; const socketPromise = expectNewSocket ? tracker.waitForSocketRecord() : null; await launcher.click(); await getCloseButtonLocator().waitFor({ state: 'visible' }); if (expectNewSocket) { const record = await socketPromise; expect(tracker.socketRecords.length).toBe(before + 1); expect(record.closedAt).toBeNull(); return record; } expect(tracker.socketRecords.length).toBe(before); return tracker.socketRecords[tracker.socketRecords.length - 1]; }, closeWidget: async () => { await getCloseButtonLocator().click(); await getCloseButtonLocator().waitFor({ state: 'hidden' }); }, });Then replace the inline definitions with:
const { openWidget, closeWidget } = createWidgetHelpers(launcher, getCloseButtonLocator, tracker);
🧹 Nitpick comments (3)
webplugin/js/app/mck-sidebox-1.0.js (1)
14-16: Clarify socket lifecycle flags; bound and drain the new bot queue
- Ensure IS_SOCKET_INITIALIZED is set true only after onConnect and cleared on all failure/early-return paths.
- Bound/clear MCK_BOT_MESSAGE_QUEUE on disconnect/destroy to avoid leaks.
Based on learnings
playwright_test/tests/socketConnection.spec.js (2)
25-28: Avoid race: return the exact WebSocket record instead of “last element.”Concurrent sockets could make “last item” brittle. Capture the ws from waitForEvent and map back to its record.
- const waitForSocketRecord = async () => { - await page.waitForEvent('websocket', (ws) => isWidgetSocketUrl(ws.url())); - return socketRecords[socketRecords.length - 1]; - }; + const waitForSocketRecord = async () => { + const ws = await page.waitForEvent('websocket', (w) => isWidgetSocketUrl(w.url())); + return socketRecords.find((r) => r.ws === ws); + };
62-63: Drop unused iframeLocator and inline frameLocator usage.iframeLocator is only used to build launcher and not returned/used elsewhere.
- const iframeLocator = await page.frameLocator(WIDGET_LOCATORS.kmIframe); - const launcher = iframeLocator.locator(WIDGET_LOCATORS.kmLaunchWidget); + const launcher = page.frameLocator(WIDGET_LOCATORS.kmIframe).locator(WIDGET_LOCATORS.kmLaunchWidget); @@ - return { iframeLocator, launcher, getCloseButtonLocator }; + return { launcher, getCloseButtonLocator };Also applies to: 67-67
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
webplugin/js/app/applozic.chat-6.2.9.min.jsis excluded by!**/*.min.jswebplugin/js/app/applozic.chat-6.3.0.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (5)
playwright_test/tests/socketConnection.spec.js(1 hunks)webplugin/development.js(1 hunks)webplugin/gulpfile.js(1 hunks)webplugin/js/app/mck-app.js(1 hunks)webplugin/js/app/mck-sidebox-1.0.js(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-25T05:44:56.580Z
Learnt from: panwaranuj01
PR: Kommunicate-io/Kommunicate-Web-SDK#1320
File: webplugin/js/app/voice/mck-voice.js:758-758
Timestamp: 2025-06-25T05:44:56.580Z
Learning: The code in webplugin/js/app/voice/mck-voice.js runs in an isolated scope and does not pollute the global namespace of other windows or the host page. Global variables within this isolated scope are acceptable as they don't affect the embedding environment.
Applied to files:
webplugin/js/app/mck-sidebox-1.0.js
🧬 Code graph analysis (3)
webplugin/js/app/mck-app.js (1)
webplugin/controller.js (1)
MCK_CONTEXTPATH(23-23)
webplugin/gulpfile.js (2)
webplugin/development.js (2)
copyFileToBuild(122-129)buildDir(16-16)webplugin/bundleFiles.js (1)
buildDir(2-2)
webplugin/development.js (2)
webplugin/gulpfile.js (2)
copyFileToBuild(360-368)buildDir(28-28)webplugin/bundleFiles.js (1)
buildDir(2-2)
🔇 Additional comments (2)
webplugin/js/app/mck-app.js (1)
31-33: Sync all loaders with the 6.3.0 URLPlease double‑check that every loader/optimizer (e.g.,
pluginOptimizer.js, deployment manifests) now points toapplozic.chat-6.3.0.min.js; the inline comment still applies, and any forgotten reference will serve the wrong asset.webplugin/gulpfile.js (1)
255-257: Ensure the 6.3.0 asset is actually presentThe copy step will now fail if
webplugin/js/app/applozic.chat-6.3.0.min.jshasn’t been added to the repo/build pipeline. Please confirm the new artifact exists (and is committed) so the task doesn’t break.
|
A friendly reminder that this PR had no activity for 15 days. |
What do you want to achieve?
PR Checklist
How was the code tested?
What new thing you came across while writing this code?
In case you fixed a bug then please describe the root cause of it?
Screenshot
NOTE: Make sure you're comparing your branch with the correct base branch
Summary by CodeRabbit
Bug Fixes
Tests
Chores