Skip to content

Stop a mid-turn rename offering to switch to the conversation on screen - #4857

Open
springfall2008 wants to merge 5 commits into
mainfrom
fix/chat-title-banner-switch-prompt
Open

Stop a mid-turn rename offering to switch to the conversation on screen#4857
springfall2008 wants to merge 5 commits into
mainfrom
fix/chat-title-banner-switch-prompt

Conversation

@springfall2008

Copy link
Copy Markdown
Owner

Summary

The "Replying in 'X' — switch to it" banner exists to say a reply is running in a conversation the user is not looking at. setBusy() got that right. handleTitle() restated the same rule inverted:

if (state.busy && state.busy.conversation_id === state.conversation) {
    showBanner(state.busy.conversation_id, data.title);
}

title events are scoped server-side to the conversation being viewed, so that branch is true exactly when the busy conversation is the one already open — the case setBusy() deliberately hides the banner for. So the moment the model named a new conversation with set_chat_title, a banner appeared offering to switch the user to the transcript already in front of them.

The same handler also never updated state.titles, which updateChatTitle() reads, so the header went on saying "New chat" while the row in the list already showed the real name — both halves of the reported screenshot, one handler.

Fix

The decision gets a single owner, refreshBanner(), which setBusy() and handleTitle() both route through. Restating the rule at each call site is precisely what let the two drift apart. handleTitle() now refreshes the header and keeps state.busy.title current, so switching away afterwards shows the banner with the name the conversation actually has rather than the one it was created with.

Testing

Written test-first; the assertions were watched failing before the fix existed.

Beyond the string assertions this repo uses for its JS, I ran the real generated script in a DOM shim, before and after:

Step Before After
setBusy on the conversation being viewed hidden hidden
title event arrives mid-turn visible hidden
header title after the rename undefined "Octopus saving sessions today"
reply is in a different conversation visible visible

That reproduces the reported screenshot and shows both defects fixed with no regression on the case the banner is actually for.

./run_pre_commit — exit 0, all hooks passed, full --quick suite green.

Note on existing coverage

test_busy_banner_only_points_at_another_conversation asserted the rule inside setBusy(). Rather than dropping it, it is retargeted at refreshBanner() and additionally checks that setBusy() still delegates — so the coverage moves with the rule, and the rule cannot quietly migrate back into one caller and drift again.

The "Replying in 'X' - switch to it" banner exists to say a reply is running
in a conversation the user is NOT looking at. setBusy() got that right, but
handleTitle() restated the same rule inverted:

    if (state.busy && state.busy.conversation_id === state.conversation) {
        showBanner(state.busy.conversation_id, data.title);
    }

'title' events are scoped server-side to the conversation being viewed, so
that branch is true exactly when the busy conversation is the one already
open - the case setBusy() deliberately hides the banner for. The moment the
model named a new conversation with set_chat_title, the banner appeared
offering to switch the user to the transcript in front of them.

The same handler also never updated state.titles, which updateChatTitle()
reads, so the header went on saying "New chat" while the row in the list
already showed the real name - both halves of the reported screenshot.

Fixed by giving the decision a single owner, refreshBanner(), which setBusy()
and handleTitle() both route through; restating it per call site is what let
the two drift apart. handleTitle() now refreshes the header and keeps
state.busy.title current, so switching away afterwards shows the banner with
the name the conversation actually has, rather than the one it was created
with.

Verified before and after by running the real generated script in a DOM
shim: on the same conversation a title event took the banner from visible to
hidden, the header title from undefined to the new name, and a reply in a
different conversation still raises the banner.

The existing banner test asserted the rule inside setBusy(), so it is
retargeted at refreshBanner() and additionally checks setBusy() still
delegates - the coverage moves with the rule rather than being dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@springfall2008 springfall2008 self-assigned this Aug 30, 2026
@springfall2008
springfall2008 marked this pull request as ready for review August 30, 2026 09:33
Copilot AI lite review requested due to automatic review settings August 30, 2026 09:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The behavioral fix is localized, aligns setBusy() and handleTitle() behind a shared decision point, and is covered by targeted regression tests (only a minor comment wording nit noted).

Pull request overview

This PR fixes a UI logic mismatch in the Chat tab where a mid-turn conversation rename could incorrectly display the “Replying in ‘X’ — switch to it” banner for the currently open conversation, and could also leave the header title stale. The change centralizes the banner visibility decision and ensures title events update both the conversation list row and the header state.

Changes:

  • Introduce refreshBanner() as the single owner of the banner show/hide decision and route setBusy() / handleTitle() through it.
  • Update handleTitle() to keep state.titles (and thus the header via updateChatTitle()) in sync with server title events, and to keep state.busy.title current when applicable.
  • Add/retarget tests to assert the banner rule is centralized and that title events don’t trigger a “switch to it” banner for the open conversation.
File summaries
File Description
apps/predbat/web_chat.py Centralizes busy-banner visibility logic and updates title-event handling to keep header/list/banner state consistent.
apps/predbat/tests/test_web_chat.py Updates existing banner rule test to target refreshBanner() and adds a regression test for mid-turn title events.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread apps/predbat/web_chat.py Outdated
springfall2008 and others added 3 commits August 30, 2026 13:23
…true

Review follow-up. The comment claimed every caller goes through
refreshBanner() rather than reaching for showBanner/hideBanner itself, while
setIdle() still called hideBanner() directly.

Rewording the comment would have documented the exception. Removing it is
better: setIdle() clears state.busy immediately before, so refreshBanner()
takes the hide branch and the outcome is identical, with one fewer place that
decides for itself. A direct call is precisely the shape the inverted rule
grew in - whoever calls one is deciding, and that decision drifted.

refreshBanner() is now the only caller of either, and the comment says what
it actually owns: the decision, not every path that ends in a hidden banner.

The test asserts the same for setBusy() and setIdle(), and strips line
comments before checking - naming the old call while explaining why it is no
longer made must not read as making it, which the first version of the
assertion got wrong.

Mutation-checked: reverting setIdle() to hideBanner() fails both assertions.
Behaviour confirmed unchanged in a DOM shim - busy elsewhere raises the
banner, setIdle clears it, busy on the open conversation leaves it down.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@springfall2008 springfall2008 added the BOT_CLEANUP Trigger: bot should address PR review feedback and CI failures, then commit and push label Aug 30, 2026
The test added in 11f54ba returned None on success, so the runner's
'failed |= test(...)' raised TypeError and the whole web_chat module
aborted after the first test - the cause of the red pre-commit CI check.

Co-Authored-By: Claude Code <noreply@anthropic.com>
@springfall2008

Copy link
Copy Markdown
Owner Author

The red pre-commit check was a test-harness bug, not a code regression: test_title_event_does_not_offer_to_switch_to_the_open_conversation (added in 11f54ba) ended without return failed, so it returned None and the runner's failed |= test(...) raised TypeError right after the test printed its own 'passed' line — aborting the whole web_chat module. Fixed in 921811f by adding the missing return; web_chat now passes end to end locally (exit 0) and the full run_pre_commit gate is green. No code change — the earlier merge-into-main and the Copilot thread's setIdle() resolution were already in.

@springfall2008 springfall2008 removed the BOT_CLEANUP Trigger: bot should address PR review feedback and CI failures, then commit and push label Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants