add copyable code snippets#8292
Conversation
There was a problem hiding this comment.
PR Review: add copyable code snippets (#8292)
Summary
Despite the title focusing on copyable code snippets, this PR bundles four distinct features into one:
- Click-to-copy inline code in assistant messages (new
CopyableInlineCodecomponent) - Model name + provider logo labels above each assistant response
- SSE custom event filter to fix #8280 (OpenAI-compatible servers sending non-standard event types)
- Emoji-prefixed chat titles + emoji font fallback in
--font-sans
Each feature is individually well-implemented, but bundling them makes the PR harder to review and bisect. Consider splitting into separate PRs for independent features (especially SSE filter vs UI changes).
Code Quality Assessment
CopyableInlineCode (CopyableInlineCode.tsx) -- Well designed.
- Event delegation via
data-streamdown="inline-code"attribute is the right call -- avoids disrupting Streamdown's fenced-code-block renderer. display: contentswrapper avoids layout side effects.- Text-selection guard (
window.getSelection().isCollapsed) is a nice touch. - Timer cleanup in
useEffectis correct. - Graceful clipboard API degradation (
.catch(() => {})) is appropriate. - Portal-based badge avoids re-rendering the markdown body.
SSE Event Filter (model-factory.ts) -- Solid and well-tested.
- The
TransformStreamapproach withremainderbuffering handles split chunks correctly. - CRLF normalization is handled.
- The
flush()method processes any leftover data. - Test coverage (202 lines in
sse-event-filter.test.ts) is thorough -- covers split chunks, CRLF, empty event types, integration withcreateCustomFetch.
Model labels (MessageItem.tsx) -- Functional but has a concern.
- The
useMemodependency array is correct and complete. - The fallback logic (per-message metadata -> current selection for last message) is reasonable.
- Non-null assertion
modelProviderForDisplay!on line ~598 is safe (guarded bymodelProviderLogotruthiness which requiresmodelProviderForDisplayto be truthy), but a more defensive approach would be cleaner.
Thread title emoji (thread-title-summarizer.ts) -- Clean.
- The regex
[^\p{L}\p{N}\p{Extended_Pictographic}\uFE0F\u200D\s]correctly preserves emoji sequences including ZWJ. - Good test coverage for emoji edge cases.
Issues & Observations
-
Scope creep / PR title mismatch: The title says "add copyable code snippets" but ~60% of the diff is unrelated (model labels, SSE filter, emoji titles). The PR description is excellent and thorough, but the title should reflect the actual scope or, better yet, this should be multiple PRs.
-
Commit history is messy: Commit 1 (
ab6c2d2) contains model labels, SSE filter, AND emoji titles. Commit 2 is review feedback for model labels. Commit 3 is the inline code feature. Consider squashing or reorganizing before merge. -
$threadId.tsxmutation pattern: The code mutatesmessageMetadatadirectly:if (!messageMetadata.modelId && currentModelState.selectedModel) { messageMetadata.modelId = currentModelState.selectedModel.id messageMetadata.modelProvider = currentModelState.selectedProvider }
This mutates the object cast from
message.metadata. Ifmessage.metadatais shared or frozen upstream, this could be fragile. Consider creating a new object ({ ...messageMetadata, modelId: ... }) instead. -
Missing
useMemoformodelProviderForDisplayandmodelProviderLogo: These are computed inline on every render ofMessageItem. While cheap, for consistency with the memoizedmodelDisplayName, wrapping them inuseMemowould be more idiomatic (especially sinceMessageItemis wrapped inmemo). -
No unit tests for
CopyableInlineCode: The new component has no dedicated test file. The PR description mentions existingRenderMarkdowntests pass, but there are no tests for the click-to-copy behavior, badge positioning, or text-selection guard. Consider adding tests (possibly using@testing-library/user-eventwith a mockednavigator.clipboard). -
getModelDisplayName,getProviderTitle,getProviderLogoimports: The diff changes the import from@/lib/utilsto include these helpers. The currentmainbranch exports them fromutils.ts, confirmed. This is fine. -
CSS scoping looks correct:
.copyable-inline-code [data-streamdown='inline-code']ensures the hover/pointer styles only apply within the copyable wrapper.
Minor Nits
- The
FEEDBACK_MS = 1200constant is reasonable but could be documented as to why 1.2s was chosen (it's a UX taste call, not a bug). - The badge
z-50is high -- if there are other z-indexed elements (modals, dropdowns), this could conflict. Verify it doesn't overlap with other UI elements.
Recommendation: improve needed
The code quality per-feature is good, but the PR would benefit from: (a) splitting into separate PRs for independent features (SSE filter fix alone is worth its own PR), (b) adding unit tests for CopyableInlineCode, and (c) avoiding direct mutation of messageMetadata in $threadId.tsx. None of these are blocking bugs, but they improve maintainability and reviewability.
fa972dd to
b55bb79
Compare
|
I have made a mistake in the original PR containing edits from other branches as well. I have now removed those and kept only the click-to-copy inline code feature:
I also addressed the feature-specific feedback:
|
Follow-up Review — All Prior Feedback AddressedThe author has fully addressed the concerns raised in the initial review:
The remaining nits from the first review (mutation of `messageMetadata`, `useMemo` for provider fields) were in the removed commits and no longer apply. The current diff is minimal and well-scoped. Event delegation, `display: contents`, portal badge, timer cleanup, and graceful degradation are all correctly implemented. Recommendation: can merge |
Description
Inline code spans (
`like this`) in assistant chat messages are now click-to-copy. Clicking an inline code snippet copies its contents to the clipboard and shows a brief "Copied!" badge at the cursor.Previously, only fenced code blocks (triple-backtick) were copyable via Streamdown's built-in block copy button. Short inline snippets — like a setting name, flag, or command referenced mid-sentence — had no way to be copied other than manual text selection.
Changes
CopyableInlineCodewrapper (web-app/src/components/CopyableInlineCode.tsx) that makes inline code click-to-copy and renders a transient "Copied!" badge at the click position.RenderMarkdowngains an opt-incopyableInlineCodeprop; when set, it wraps the rendered markdown inCopyableInlineCode. Off by default.MessageItemenables the prop only on the assistant-message render path, scoping the feature to assistant messages.markdown.cssadds acursor: pointerand subtle hover tint on inline code, scoped to the copyable wrapper so the affordance only appears where copying is enabled.Implementation notes
data-streamdown="inline-code". The wrapper listens for clicks and matches that attribute, rather than overriding React-Markdown'scodecomponent. This is deliberate: overridingcodewould replace Streamdown's block renderer and lose syntax highlighting and the existing copy button on fenced code blocks. Fenced blocks are completely untouched.display: contents, so it adds no box to the DOM and doesn't affect markdown spacing/layout.Testing
RenderMarkdowntest suite passes (23/23).tsc -b) and lint (eslint) clean on changed files.