Skip to content

fix: gemma thinking - #841

Merged
ndizazzo merged 1 commit into
mainfrom
fix/gemma-channel-thinking-ui
Jun 13, 2026
Merged

fix: gemma thinking#841
ndizazzo merged 1 commit into
mainfrom
fix/gemma-channel-thinking-ui

Conversation

@ndizazzo

@ndizazzo ndizazzo commented Jun 13, 2026

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • New Features

    • Extended thinking trace capabilities to support and properly display Gemma-style channel thinking markers in addition to existing thought formats.
  • Tests

    • Added comprehensive test coverage validating correct parsing and rendering of Gemma-style thinking format in message components.

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR adds support for Gemma-style channel thinking markers (<|channel|>...<channel|>) in the chat message parser, alongside the existing <think> tag format. The core parser detects and splits Gemma-formatted thinking segments, unit tests validate the segmentation logic, and an integration test confirms correct rendering in MessageRow.

Changes

Gemma Channel Thinking Support

Layer / File(s) Summary
Core parser implementation for Gemma channel format
crates/mesh-llm-ui/src/features/chat/components/thinking-segments.ts
Adds TagMatch type and Gemma tag constants for case-insensitive matching. findFirstTag helper locates the earliest tag occurrence across multiple candidates, and splitGemmaChannelThinking parses message bodies into thinking and response segments when Gemma markers are detected. splitAssistantThinking now attempts Gemma parsing first, returning early when applicable, before falling back to existing <think> tag logic.
Parser unit tests for Gemma format
crates/mesh-llm-ui/src/features/chat/components/thinking-segments.test.ts
Extends test suite with two test cases for splitAssistantThinking: one validates splitting with complete <|channel|> format, another validates the case with missing opening pipe. Both confirm correct segmentation into closed thinking and response segments.
MessageRow integration test for Gemma thinking rendering
crates/mesh-llm-ui/src/features/chat/components/MessageRow.test.tsx
New test case verifies that Gemma channel-formatted thinking is parsed into the completed "Thinking trace" section, the full thinking content appears under the trace as complete, the post-tag response text is selectable, and raw channel tag patterns are absent from the rendered output.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fix: gemma thinking' is vague and does not clearly convey the specific nature of the changes, which relate to supporting Gemma-style channel thinking markers in the UI. Consider using a more descriptive title such as 'Support Gemma-style channel thinking markers' or 'Add Gemma channel thinking format support' to better communicate the main changes.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/gemma-channel-thinking-ui

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
crates/mesh-llm-ui/src/features/chat/components/thinking-segments.test.ts (1)

25-39: ⚡ Quick win

Add test case for unclosed Gemma thinking segment.

The existing test suite covers unclosed <think> tags (line 41-45), but there's no equivalent test for the Gemma format. The parser correctly handles unclosed Gemma thinking at lines 71-74 of thinking-segments.ts by setting open: true, but this edge case lacks test coverage.

✅ Proposed test case
  it('splits Gemma channel thinking when the thought marker is missing the leading pipe', () => {
    expect(splitAssistantThinking('<channel|>thoughtCheck facts.<channel|>Final answer.')).toEqual([
      { kind: 'thinking', text: 'Check facts.', open: false },
      { kind: 'response', text: 'Final answer.' }
    ])
  })
+
+ it('keeps an unclosed Gemma thinking segment open for streaming', () => {
+   expect(splitAssistantThinking('<|channel|>thoughtStill analyzing the input')).toEqual([
+     { kind: 'thinking', text: 'Still analyzing the input', open: true }
+   ])
+ })

As per coding guidelines: "Cover edge cases in tests: missing status, empty peers, client nodes, warm/cold models, malformed attachment data, and localStorage failures."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/mesh-llm-ui/src/features/chat/components/thinking-segments.test.ts`
around lines 25 - 39, Add a new unit test in thinking-segments.test.ts that
exercises splitAssistantThinking for an unclosed Gemma-format thinking segment:
call splitAssistantThinking with a string containing an opening Gemma marker and
thought text but no closing channel marker (e.g., starting with
"<|channel|>thought...") and assert the result is a single thinking segment with
text normalized (capitalized/punctuation as parser does) and open: true (i.e.,
[{ kind: 'thinking', text: '<expected normalized text>', open: true }]). Use the
same test style and expect/assert helpers as the other tests so it parallels the
existing unclosed <think> case and references splitAssistantThinking and the
thinking-segments parsing behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/mesh-llm-ui/src/features/chat/components/thinking-segments.test.ts`:
- Around line 25-39: Add a new unit test in thinking-segments.test.ts that
exercises splitAssistantThinking for an unclosed Gemma-format thinking segment:
call splitAssistantThinking with a string containing an opening Gemma marker and
thought text but no closing channel marker (e.g., starting with
"<|channel|>thought...") and assert the result is a single thinking segment with
text normalized (capitalized/punctuation as parser does) and open: true (i.e.,
[{ kind: 'thinking', text: '<expected normalized text>', open: true }]). Use the
same test style and expect/assert helpers as the other tests so it parallels the
existing unclosed <think> case and references splitAssistantThinking and the
thinking-segments parsing behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 990cebea-a395-4a15-aa8f-c4aa4543068d

📥 Commits

Reviewing files that changed from the base of the PR and between 226d1c6 and af205bc.

📒 Files selected for processing (3)
  • crates/mesh-llm-ui/src/features/chat/components/MessageRow.test.tsx
  • crates/mesh-llm-ui/src/features/chat/components/thinking-segments.test.ts
  • crates/mesh-llm-ui/src/features/chat/components/thinking-segments.ts

@ndizazzo
ndizazzo merged commit 6bd7d77 into main Jun 13, 2026
25 checks passed
@ndizazzo
ndizazzo deleted the fix/gemma-channel-thinking-ui branch June 13, 2026 04:23
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.

1 participant