restart conversation#1489
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 474cad3746
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !KommunicateUI.pendingClosedConversationBanner || | ||
| KommunicateUI.closedConversationBannerTabId !== CURRENT_GROUP_DATA.tabId | ||
| ) { |
There was a problem hiding this comment.
The timeout callback drops the banner whenever CURRENT_GROUP_DATA.tabId no longer matches the scheduled tab, but that global is reassigned for any incoming KM_STATUS message (see mck-sidebox-1.0.js around the status handler where CURRENT_GROUP_DATA.tabId = resp.message.groupId). In practice, if conversation A is resolved and scheduled, then a status update for conversation B arrives before the delay expires, this check returns early and the resolved banner/input lock never appears for A even if A is still the open chat. This introduces a missed-resolved-state regression under normal multi-conversation socket traffic.
Useful? React with 👍 / 👎.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds tab-aware, debounced closed-conversation banner state and APIs to Changes
Sequence Diagram(s)sequenceDiagram
participant Sidebox as Sidebox
participant Kommunicate as KommunicateUI
participant Timer as Timeout
Sidebox->>Kommunicate: scheduleClosedConversationBanner(delayMs, tabId)
Kommunicate->>Kommunicate: clear existing timeout (if any)
Kommunicate->>Timer: setTimeout -> after delay
Note right of Timer: delay elapses
Timer->>Kommunicate: timeout callback
Kommunicate->>Kommunicate: if pending && tabId matches -> showClosedConversationBanner(true)
Kommunicate->>Sidebox: (banner shown) update UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
webplugin/js/app/kommunicate-ui.js (1)
112-120: Reset pending banner state after timeout fires.When the delayed path succeeds, internal pending/tab state remains set until another explicit clear. Resetting it in-callback avoids stale state across later UI transitions.
Proposed tweak
KommunicateUI.closedConversationBannerTimeoutId = setTimeout(function () { if ( !KommunicateUI.pendingClosedConversationBanner || KommunicateUI.closedConversationBannerTabId !== CURRENT_GROUP_DATA.tabId ) { return; } + KommunicateUI.clearClosedConversationBannerTimeout(); KommunicateUI.showClosedConversationBanner(true); }, delayMs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webplugin/js/app/kommunicate-ui.js` around lines 112 - 120, The timeout callback should clear the pending banner state when it runs successfully to avoid leaving stale flags; inside the existing setTimeout callback (where it checks KommunicateUI.pendingClosedConversationBanner and tabId and then calls KommunicateUI.showClosedConversationBanner(true)), reset KommunicateUI.pendingClosedConversationBanner to false and clear KommunicateUI.closedConversationBannerTabId (and clear KommunicateUI.closedConversationBannerTimeoutId if desired) so the internal state is cleared after the delayed action completes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webplugin/js/app/mck-sidebox-1.0.js`:
- Line 4224: The call to KommunicateUI.clearClosedConversationBannerTimeout()
unconditionally clears global timeout state and should only clear if the current
tab owns it; update the restartConversation flow to either add a new
KommunicateUI.clearClosedConversationBannerTimeoutForTab(tabId) or guard the
existing call by checking KommunicateUI.closedConversationBannerTabId ===
CURRENT_GROUP_DATA.tabId before invoking clearClosedConversationBannerTimeout();
reference the symbols restartConversation, clearClosedConversationBannerTimeout,
closedConversationBannerTabId and CURRENT_GROUP_DATA.tabId when applying the
change.
---
Nitpick comments:
In `@webplugin/js/app/kommunicate-ui.js`:
- Around line 112-120: The timeout callback should clear the pending banner
state when it runs successfully to avoid leaving stale flags; inside the
existing setTimeout callback (where it checks
KommunicateUI.pendingClosedConversationBanner and tabId and then calls
KommunicateUI.showClosedConversationBanner(true)), reset
KommunicateUI.pendingClosedConversationBanner to false and clear
KommunicateUI.closedConversationBannerTabId (and clear
KommunicateUI.closedConversationBannerTimeoutId if desired) so the internal
state is cleared after the delayed action completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da49c46e-9272-4ed1-9182-0533bdf0744e
📒 Files selected for processing (2)
webplugin/js/app/kommunicate-ui.jswebplugin/js/app/mck-sidebox-1.0.js
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (1)
webplugin/js/app/mck-sidebox-1.0.js (1)
15521-15524:⚠️ Potential issue | 🟡 MinorScope delayed banner timers per tab.
KommunicateUI.scheduleClosedConversationBanner()inwebplugin/js/app/kommunicate-ui.js:1-50clears a singletonclosedConversationBannerTimeoutIdbefore every schedule. In a multi-tab session, scheduling here for tab B cancels tab A's still-pending delayed banner, so only the most recent tab keeps its restart CTA timing. Please key the timeout/pending state bytabIdinstead of sharing one global slot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webplugin/js/app/mck-sidebox-1.0.js` around lines 15521 - 15524, The scheduling currently uses a singleton closedConversationBannerTimeoutId that is cleared on every call to KommunicateUI.scheduleClosedConversationBanner(), which causes timers from other tabs to be cancelled; change the implementation to track timeouts per tabId (use a map/object keyed by tabId) so scheduleClosedConversationBanner(tabId, ...) stores the timeout under that tabId and clear only that specific entry when rescheduling or cancelling; update any references that clear or read closedConversationBannerTimeoutId to use the per-tab map (e.g., use closedConversationBannerTimeouts[CURRENT_GROUP_DATA.tabId]) and ensure KommunicateUI.scheduleClosedConversationBanner is invoked with the CURRENT_GROUP_DATA.tabId when scheduling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@webplugin/js/app/mck-sidebox-1.0.js`:
- Around line 15521-15524: The scheduling currently uses a singleton
closedConversationBannerTimeoutId that is cleared on every call to
KommunicateUI.scheduleClosedConversationBanner(), which causes timers from other
tabs to be cancelled; change the implementation to track timeouts per tabId (use
a map/object keyed by tabId) so scheduleClosedConversationBanner(tabId, ...)
stores the timeout under that tabId and clear only that specific entry when
rescheduling or cancelling; update any references that clear or read
closedConversationBannerTimeoutId to use the per-tab map (e.g., use
closedConversationBannerTimeouts[CURRENT_GROUP_DATA.tabId]) and ensure
KommunicateUI.scheduleClosedConversationBanner is invoked with the
CURRENT_GROUP_DATA.tabId when scheduling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 813af65a-94f4-4841-8040-becc81209191
📒 Files selected for processing (1)
webplugin/js/app/mck-sidebox-1.0.js
| setTimeout(function () { | ||
| KommunicateUI.showClosedConversationBanner(true); | ||
| }, MCK_BOT_MESSAGE_DELAY); | ||
| KommunicateUI.isConvJustResolved = !!!KommunicateUI.isConvJustResolved; |
There was a problem hiding this comment.
"KommunicateUI.isConvJustResolved = !!!KommunicateUI.isConvJustResolved;
"
this line is present in both if and else condition . if its not dependent on any condition then keep it outside and use it once
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webplugin/js/app/mck-sidebox-1.0.js`:
- Around line 15516-15525: The current code toggles
KommunicateUI.isConvJustResolved using the triple-negation pattern; change this
to set the flag deterministically to true (KommunicateUI.isConvJustResolved =
true) when a bot-sent message meets the conditions (MCK_BOT_MESSAGE_DELAY !== 0,
mckMessageLayout.isMessageSentByBot(resp.message, contact), and
!CURRENT_GROUP_DATA.TOKENIZE_RESPONSE) so duplicate resolved events do not flip
it back; keep the existing restart/reset path that clears
KommunicateUI.isConvJustResolved to false elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7bc9be05-4977-400c-a067-ce284002ecfe
📒 Files selected for processing (1)
webplugin/js/app/mck-sidebox-1.0.js
| '#km-widget-options', | ||
| '.mck-box-form-container' | ||
| ); | ||
| kommunicateCommons.show('.mck-csat-text-1'); |
There was a problem hiding this comment.
Shows the restart banner text (“If you have any other queries, just Restart this conversation”).
| }); | ||
| _this.restartConversation = function (event) { | ||
| kmWidgetEvents.eventTracking(eventMapping.onRestartConversationClick); | ||
| kommunicateCommons.hide('.mck-csat-text-1'); |
There was a problem hiding this comment.
So this block is what brings the chat input back when the user clicks “Restart conversation.”
2 participants
What do you want to achieve?
PR Checklist
How was the code tested?
-Locally
What new thing you came across while writing this code?
-The closed‑conversation banner is delayed via MCK_BOT_MESSAGE_DELAY, and showClosedConversationBanner behavior is influenced by KommunicateUI.isConvJustResolved.
In case you fixed a bug then please describe the root cause of it?
-Root cause: the delayed setTimeout that shows the “Restart this conversation” banner wasn’t cancelled on restart, so it could fire later and re‑show the banner, clearing the input.
Screenshot
NOTE: Make sure you're comparing your branch with the correct base branch
Summary by CodeRabbit
New Features
Bug Fixes