Skip to content

feat(desktop): clarify variant-only timeline notices - #54880

Merged
Hona merged 6 commits into
devfrom
variant-switch-notice
Oct 13, 2026
Merged

Hona merged 6 commits into
devfrom
variant-switch-notice

Conversation

@usrnk1

@usrnk1 usrnk1 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Issue for this PR

Reported in Slack; no issue number.

Type of change

  • Bug fix
  • New feature
  • Refactor / code improvement
  • Documentation

What does this PR do?

A variant-only switch back to Default looked like a model change in the web timeline. The notice now compares the previous and selected model and names the variant change instead. Live and optimistic notices carry the previous model too, so the text stays correct before the server echo arrives.

How did you verify your code works?

  • bun run check
  • App timeline and client data unit tests
  • Session timeline component tests (4 passed)
  • Reviewed the same Storybook fixture before and after

Screenshots / recordings

Before After
Variant notice before Variant notice after

Checklist

  • I have tested my changes locally
  • I have not included unrelated changes in this PR

@opencode-agent opencode-agent Bot 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.

The change looks sound. The server already sends previous on session.model.selected and on model-switched messages, and switchModel skips switches where nothing changed. Carrying previous into the optimistic and live notices keeps the label right before the echo, and older messages without previous still show "Switched to X". The projection and client data unit tests and the app/client/session-ui/ui type checks pass. I couldn't run the Playwright component tests (no browser in the sandbox) or render the web timeline here, so I relied on your Storybook before/after screenshots.

Public API: unchanged. previous already exists on the event and message schemas; this only adds two English UI strings.

One small point on the new client test (inline).

}
})

test("keeps the previous model in a live variant notice until the projected message arrives", async () => {

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.

This test is large for what it checks: a mocked fetch held back by a deferred promise plus a wait for the refetch. And because the event already carries previous, it only checks that the field passes through. It never reaches the new fallback (event.data.previous ?? store.session.info[...]?.model). Could it be cut down to the fallback case: an event without previous, checking that the notice gets the session's stored model? Or drop it if the projection test is enough.

@Hona
Hona enabled auto-merge (squash) October 13, 2026 08:42
@Hona
Hona merged commit 5b22e91 into dev Oct 13, 2026
10 checks passed
@Hona
Hona deleted the variant-switch-notice branch October 13, 2026 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants