Skip to content

feat: model name labels, SSE custom event filter, emoji chat titles#8286

Open
FedeCuci wants to merge 2 commits into
janhq:mainfrom
FedeCuci:feat/model-label-sse-emoji-titles
Open

feat: model name labels, SSE custom event filter, emoji chat titles#8286
FedeCuci wants to merge 2 commits into
janhq:mainfrom
FedeCuci:feat/model-label-sse-emoji-titles

Conversation

@FedeCuci

@FedeCuci FedeCuci commented Jun 6, 2026

Copy link
Copy Markdown

Describe Your Changes

  1. Show model name + provider logo above each assistant response. This is useful in case a user frequently uses different models in the same chat and wants to see which model was the one outputting a specific response. Other interfaces such as Open Web UI also do this.
  2. Filter custom SSE event types to fix bug: SSE parser fails on custom event types from OpenAI-compatible API servers #8280 (OpenAI-compatible servers).
  3. Auto-prefix generated chat titles with a relevant emoji. This is also similar to how Open Web UI does it. I believe adding an emoji to the chat titles helps the user remember chats with a visual cue rather than a title they never read.

Fixes Issues

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features) - n.a, no API or config changes
  • Created issues for follow-up changes or refactoring needed - none needed

1. Show model name + provider logo above each assistant response
2. Filter custom SSE event types to fix janhq#8280 (OpenAI-compatible servers)
3. Auto-prefix generated chat titles with a relevant emoji
4. Add emoji font fallback for cross-platform rendering
@tokamak-pm

tokamak-pm Bot commented Jun 7, 2026

Copy link
Copy Markdown

PR Review: feat: model name labels, SSE custom event filter, emoji chat titles

Summary

This PR bundles three features in a single commit:

  1. Model name labels -- displays the model name and provider logo above each assistant message in the chat UI.
  2. SSE custom event filter -- adds a TransformStream that strips non-standard SSE event types (e.g. hermes.tool.progress) before they reach the AI SDK parser, fixing bug: SSE parser fails on custom event types from OpenAI-compatible API servers #8280.
  3. Emoji chat titles -- modifies the title summarization prompt to request an emoji prefix and updates cleanTitle() to preserve emoji characters.

Files changed: 7 (356 additions, 6 deletions)


Code Quality & Correctness

SSE Event Filter (model-factory.ts) -- This is the strongest part of the PR. The createSSEEventFilter() TransformStream is well-documented, handles chunked delivery, CRLF normalization, and incomplete blocks in the flush() handler. The integration into createCustomFetch is clean -- it only activates for text/event-stream responses. The test suite (sse-event-filter.test.ts) is thorough with 202 lines covering split chunks, CRLF, edge cases, and integration with createCustomFetch.

Emoji in titles (thread-title-summarizer.ts) -- The regex change from [^\p{L}\p{N}\s] to [^\p{L}\p{N}\p{Extended_Pictographic}\uFE0F\u200D\s] is correct and handles ZWJ sequences. Tests cover the key cases.

Emoji font fallback (index.css) -- Adding "Apple Color Emoji", "Segoe UI Emoji", "Noto Color Emoji" to --font-sans is a sensible cross-platform fix.


Issues Found

Bug: useModelProvider.getState() called inside useMemo (MessageItem.tsx)

const modelDisplayName = useMemo(() => {
  if (messageModelId) {
    const provider = useModelProvider.getState().providers.find(
      (p) => p.provider === messageModelProvider
    )
    ...
  }
}, [messageModelId, messageModelProvider, isLastMessage, selectedModel])

Calling useModelProvider.getState() (a Zustand store) inside useMemo is a React anti-pattern. The providers list is read imperatively and is not reactive -- if providers load or change after this component renders, the memoized value won't update. The dependency array only tracks messageModelId etc., not providers. This should either:

  • Subscribe to providers via useModelProvider((s) => s.providers) at the component level and pass it into the useMemo, or
  • Accept that for messages with metadata already set, this is effectively a one-time lookup (which may be acceptable if providers are always loaded before messages render, but it's fragile).

Dead code: modelProviderForDisplay always resolves to undefined for non-metadata messages

const modelProviderForDisplay = messageModelProvider ?? (isLastMessage ? undefined : undefined)

The ternary (isLastMessage ? undefined : undefined) always evaluates to undefined. This appears to be leftover from a draft where the fallback branch did something else. As a result, the provider logo/icon never renders for the backwards-compatibility case (last message without metadata), even though modelDisplayName is computed for it. This is a logic gap -- the label will show the model name but no icon for the current message during streaming until metadata is persisted.

Concern: PR bundles 3 independent features

These are three conceptually independent features (UI labels, SSE transport fix, title generation tweak) in a single commit and PR. This makes it harder to revert one feature without the others and complicates bisection. Ideally these would be separate PRs or at minimum separate commits.

Minor: getProviderLogo called twice in render path

{modelProviderForDisplay && getProviderLogo(modelProviderForDisplay) ? (
  <img src={getProviderLogo(modelProviderForDisplay)} ... />

getProviderLogo() is called twice with the same argument -- once for the conditional check and once for the src attribute. This is a minor inefficiency; extract it to a variable.

Minor: No test for the model label UI component

The SSE filter has excellent tests, but the MessageItem model label rendering has no unit/integration test. Given the useMemo complexity, a test that verifies the label renders correctly (with metadata, without metadata, for last message, etc.) would be valuable.


Risks & Regressions

  • SSE filter is low risk -- it only activates on text/event-stream responses and correctly passes through standard blocks. Unlikely to regress existing behavior.
  • Emoji title change is low risk -- the prompt change is additive and cleanTitle now accepts a superset of characters. Worst case: the model ignores the emoji instruction and titles work as before.
  • Model label has moderate risk -- the useModelProvider.getState() inside useMemo could cause stale data if providers load asynchronously. The dead modelProviderForDisplay ternary means the feature is partially broken for the fallback case.

Recommendation: improve needed

The SSE event filter is production-ready and well-tested. The emoji title feature is clean. However, the model name label feature has two code issues that should be addressed before merging:

  1. Fix the modelProviderForDisplay dead ternary to actually provide the current provider for the last-message fallback case.
  2. Either make the providers lookup reactive or add a comment explaining why the imperative getState() is safe in this context.
  3. Extract the double getProviderLogo() call.
  4. Consider splitting this into separate PRs (or at least separate commits) for easier revert/bisect.

Items 1-3 are small fixes; item 4 is a process suggestion.

@FedeCuci

FedeCuci commented Jun 7, 2026

Copy link
Copy Markdown
Author

Addressed the code-level review feedback:

  • Reactive providers lookup: replaced useModelProvider.getState() inside useMemo with a proper Zustand subscription (useModelProvider((s) => s.providers)) at the component level, adding providers to the dependency array so the label updates if providers change after first render.
  • Fixed dead modelProviderForDisplay ternary: the fallback branch always resolved to undefined, so the streaming last message showed a name but no provider logo. It now correctly falls back to selectedProvider from the store.
  • Extracted duplicate getProviderLogo() call into a modelProviderLogo variable used in both the conditional check and the src attribute.
  • Added 4 tests to the existing MessageItem.test.tsx covering: metadata-driven name + logo, unknown model ID falling back to raw id, last-message fallback to current selection, and no label for non-last messages without metadata.

@tokamak-pm tokamak-pm Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Follow-up review (new commits detected since last review)

Commit 3c4c674 ("fix: address model label PR review feedback") addresses the issues raised in the previous review. Here is what changed:

Previously raised issues -- status

  1. useModelProvider.getState() inside useMemo (anti-pattern) -- Fixed. The follow-up commit subscribes to providers reactively via useModelProvider((state) => state.providers) at the component level and passes it into the useMemo dependency array. The imperative getState() call is gone. This is the correct approach.

  2. Dead ternary in modelProviderForDisplay -- Fixed. The expression messageModelProvider ?? (isLastMessage ? undefined : undefined) has been replaced with messageModelProvider ?? (isLastMessage ? selectedProvider : undefined). The fallback now correctly uses the current selectedProvider for the last message during streaming, which means the provider logo renders in the backwards-compatibility case as intended.

  3. Double getProviderLogo() call in render path -- Fixed. The logo is now computed once into modelProviderLogo and reused in both the conditional check and the src attribute.

  4. No tests for model label rendering -- Fixed. Four new test cases were added to MessageItem.test.tsx:

    • Renders model name and provider logo from message metadata
    • Falls back to raw model ID when the metadata model is unknown
    • Falls back to current selection for the last message without metadata
    • Does not render a label for a non-last message without metadata

    The mock for useModelProvider was also updated to include selectedProvider and providers state, making the test infrastructure match the new component subscriptions.

Remaining observation

  • PR bundles 3 independent features -- This was a process suggestion from the previous review. The features (model labels, SSE event filter, emoji titles) remain bundled in one PR. This is not a blocker, just a note for future PRs.

Final state of the code

The MessageItem.tsx model label feature is now clean: reactive state subscriptions, correct fallback logic, no redundant calls, and solid test coverage. The SSE event filter and emoji title features were already in good shape and are unchanged by this commit.

Recommendation: can merge

@tokamak-pm

tokamak-pm Bot commented Jun 10, 2026

Copy link
Copy Markdown

Follow-up review — new commit(s) since last review.

Reviewing commit 3c4c674 ("fix: address model label PR review feedback") which addresses feedback from the initial review.


Issues from prior review — resolution status

# Issue Status
1 useModelProvider.getState() inside useMemo (non-reactive) Fixed — replaced with useModelProvider((s) => s.providers) subscription at component level; providers added to the useMemo dependency array.
2 Dead ternary in modelProviderForDisplay (always undefined) Fixed — fallback branch now correctly resolves to selectedProvider for the last message, so the provider logo renders during streaming.
3 Duplicate getProviderLogo() call in render Fixed — extracted to modelProviderLogo variable, used for both the condition check and src.
4 No tests for model label rendering Fixed — 4 new tests added covering metadata-driven display, unknown model fallback, last-message fallback to current selection, and suppression for non-last messages without metadata. Test mock updated to include selectedProvider and providers.

All four code-level issues have been properly addressed.

Review of the fix commit

MessageItem.tsx — The component now has three clean Zustand subscriptions (selectedModel, selectedProvider, providers). The useMemo for modelDisplayName correctly depends on all five relevant values. The modelProviderForDisplay fallback logic is sound: per-message metadata takes priority, then the current selectedProvider for the streaming last message, then undefined for older messages without metadata (which correctly suppresses the logo). The non-null assertion modelProviderForDisplay! in the alt text is safe because it is inside a branch where modelProviderLogo is truthy, which requires modelProviderForDisplay to be defined.

MessageItem.test.tsx — The four new tests are well-structured and cover the key scenarios. The mock infrastructure correctly simulates the three store selectors.

.tsx — The remaining useModelProvider.getState() call (line ~274) is inside the onFinish callback (an event handler, not a render path), so imperative Zustand access is correct here. No issue.

Other PR components (unchanged, already reviewed)

  • SSE event filter — Well-implemented TransformStream with thorough tests. No changes in this commit.
  • Emoji chat titles — Prompt change + regex update with test coverage. No changes in this commit.
  • Emoji font stack — Cross-platform fallback in CSS. No changes in this commit.

Note

The PR still bundles three independent features. This remains a process observation, not a blocker.


Recommendation: can merge

@qnixsynapse qnixsynapse self-requested a review June 10, 2026 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

bug: SSE parser fails on custom event types from OpenAI-compatible API servers

2 participants