feat(platform): refine chat streaming UX and simplify marker parser#511
Conversation
Rewrite auto-scroll to use wheel/touch events instead of scrollTop comparison, preventing false positives from nested scrollable containers and browser scroll clamping. Fix double-cursor rendering in incremental markdown by detecting cursor-eligible child nodes. Simplify thinking animation to hide when text is actively streaming. Reduce marker parser to only split on [[NEXT_STEPS]] — all other markers are stripped so content renders as plain markdown. Remove unused structured section components (Conclusion, KeyPoints, Details, Questions) and their translations.
Greptile SummaryThis PR improves chat streaming UX across several areas: auto-scroll detection is rewritten to use wheel/touch events instead of
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| services/platform/app/hooks/use-auto-scroll.ts | Major rewrite: replaces scrollTop-based detection with wheel/touch events and nested container checks. Well-documented with clear comments. Logic is sound — only concern is that keyboard scrolling (Page Up, arrow keys) won't disable auto-scroll, but this is likely acceptable. |
| services/platform/app/features/chat/components/incremental-markdown.tsx | Fixes double-cursor rendering by adding CURSOR_ELIGIBLE_TAGS set and hasCursorEligibleChild detection. Also adds pre to cursor-eligible tags and removes consolidation on stream end to prevent scroll jumps. Well-tested changes. |
| services/platform/app/features/chat/components/tests/incremental-markdown.test.tsx | New comprehensive test file covering cursor injection (single cursor guarantee for paragraphs, loose lists, tight lists, headings, tables, code blocks) and split stability (scroll jump prevention). Good coverage of the double-cursor bug fix. |
| services/platform/lib/utils/marker-parser.ts | Simplified to only split on [[NEXT_STEPS]] while stripping all other markers. Logic is clean and well-documented. Tests updated to match. |
| services/platform/lib/utils/marker-parser.test.ts | Tests rewritten to reflect simplified parser behavior. Good coverage of stripped markers, NEXT_STEPS splitting, streaming partial-marker buffering, and progressive streaming simulation. |
| services/platform/app/features/chat/components/thinking-animation.tsx | Simple addition: returns null when text is streaming and no tools are running, hiding the thinking indicator when the user can see text progress directly. |
| services/platform/app/features/chat/hooks/use-message-processing.ts | Refactored hasIncompleteAssistantMessage to use newest message by _creationTime instead of last assistant message. Behavior change breaks existing test ("returns false when there are no assistant messages"). Also leaves dead code: pendingToolResponse, hasActiveTools, isProcessingToolResult are still computed but no longer consumed. |
| services/platform/app/features/chat/components/chat-interface.tsx | Removes destructuring of pendingToolResponse, hasActiveTools, isProcessingToolResult, and isPending prop passing. Clean removal aligned with the simplified thinking animation logic. |
| services/platform/app/features/chat/components/chat-messages.tsx | Simplifies thinking animation condition from a complex multi-state check to just hasIncompleteAssistantMessage. Removes unused props from interface and function signature. |
| services/platform/app/features/chat/components/structured-message/structured-message.tsx | Removes CONCLUSION, KEY_POINTS, DETAILS, QUESTIONS section rendering from the switch statement, keeping only NEXT_STEPS. Clean removal consistent with marker parser simplification. |
| services/platform/app/features/chat/components/structured-message/section-renderers.tsx | Large deletion of unused section components (ConclusionSection, KeyPointsSection, DetailsSection, QuestionsSection) and their shared props/imports. Only NextStepsSection remains. |
| services/platform/messages/en.json | Removes 5 unused translation keys corresponding to deleted section renderers (conclusion, keyPoints, showDetails, hideDetails, questions). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User sends message] --> B{hasIncompleteAssistantMessage?}
B -->|true| C[Show ThinkingAnimation]
C --> D{streamingMessage.text && no tools?}
D -->|yes| E[ThinkingAnimation returns null]
D -->|no| F[Show thinking indicator with tool details]
E --> G[User sees streaming text with cursor]
G --> H{Content growing?}
H -->|yes| I{autoScrollEnabled?}
I -->|yes| J[ResizeObserver triggers scrollTo bottom]
I -->|no| K[No scroll - user scrolled away]
H -->|no| L[No scroll action]
M[User scrolls up via wheel/touch] --> N{Nested container can absorb?}
N -->|yes| O[Ignore - nested scroll]
N -->|no| P[Disable auto-scroll]
Q[User reaches bottom] --> R[Re-enable auto-scroll]
S[Streaming ends] --> T{wasEnabled && autoScrollEnabled?}
T -->|yes| U[One deferred scroll via rAF]
T -->|no| V[No action]
B -->|false - assistant completed| W[No ThinkingAnimation]
Last reviewed commit: 4b6792a
Additional Comments (1)
These values are still computed (lines 155-203) and returned (lines 229-231) from the hook, but are no longer consumed by any component after this PR removes them from |
📝 WalkthroughWalkthroughThe PR refactors chat message handling by removing tool-response UI components and simplifying structured message rendering to support only NEXT_STEPS sections. It improves streaming cursor stability by introducing nesting checks to prevent duplicate cursors in block-level elements, replaces scroll-top heuristics with input-driven auto-scroll detection, and updates loading-state logic to check the newest message by creation time. Tool-related props are removed from ChatMessages, TypewriterText-based rendering is eliminated for non-NEXT_STEPS sections, and related translation keys are removed. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
When only user messages exist with no assistant reply, the thread is waiting for a response so hasIncompleteAssistantMessage should be true.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@services/platform/app/features/chat/components/__tests__/incremental-markdown.test.tsx`:
- Around line 10-13: The test's countCursors function uses a too-broad selector
('[aria-hidden="true"]'); narrow it to target the blinking cursor element
specifically (e.g., select the span with the cursor class or a test id) so other
hidden markdown nodes aren't counted — update countCursors to
querySelectorAll('span.cursor[aria-hidden="true"]') or
'span[data-testid="cursor"][aria-hidden="true"]' (or whatever class/testid your
cursor element uses) to reliably count only the cursor element.
In `@services/platform/app/features/chat/hooks/use-message-processing.ts`:
- Around line 212-214: The reduce call that computes newestMessage from
uiMessages is brittle; replace it with a clearer, safer approach such as sorting
a shallow copy (e.g. use [...uiMessages].sort((a,b)=>b._creationTime -
a._creationTime)[0]) or call reduce with an explicit initial value to avoid any
ambiguity; update the logic around newestMessage and keep references to
uiMessages and the _creationTime comparison so the code remains correct and
still respects the existing early-return guard.
In `@services/platform/app/hooks/use-auto-scroll.ts`:
- Around line 121-156: The auto-scroll disabling currently only handles
wheel/touch events (handleWheel, handleTouchMove) so scrollbar drags and
keyboard scrolling don't flip autoScrollEnabledRef; update the hook to also
detect pointer/keyboard intent by adding listeners for pointerdown and keydown
that set autoScrollEnabledRef.current = false when user interaction is detected
(use canNestedContainerScrollUp(container) check like in handleWheel), and add a
programmaticScrollRef guard that you set true/false around any internal
programmatic scroll calls so handleScroll can ignore those and only re-enable
auto-scroll via isAtBottom() when programmaticScrollRef is false; ensure you
clean up the new listeners and use the existing container, handleScroll, and
canNestedContainerScrollUp helpers.
Summary
scrollTopcomparison, preventing false positives from nested scrollable containers and browser scroll clamping[[NEXT_STEPS]]— all other markers are stripped so content renders as plain markdownTest plan
incremental-markdown.test.tsxcovering single cursor guarantee for paragraphs, loose lists, code blocks, and cursor-hidden statemarker-parser.test.tsreflecting simplified split-on-NEXT_STEPS-only behavior🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Changes
Tests