-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(cli): improve formatting for unknown message types and JSON content #4740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add robust JSON parsing in CI mode with proper error handling - Distinguish between JSON objects/arrays (metadata) and plain text (content) - Enhance unknown message type handling in interactive mode - Add comprehensive test coverage (80+ test cases) - Handle edge cases: malformed JSON, empty values, nested structures - Ensure backward compatibility with existing message types
🦋 Changeset detectedLatest commit: 091b736 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // Try to parse JSON content for better display | ||
| let displayContent = message.text || `Unknown say type: ${message.say}` | ||
|
|
||
| // If text looks like JSON, try to format it nicely |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition is inconsistent with AskMessageRouter.tsx line 34 which correctly uses parentheses. Due to operator precedence (&& binds tighter than ||), this evaluates as (message.text && message.text.trim().startsWith("{")) || message.text?.trim().startsWith("[") which happens to work but is harder to read and maintain.
| // If text looks like JSON, try to format it nicely | |
| if (message.text && (message.text.trim().startsWith("{") || message.text.trim().startsWith("["))) { |
✅ Previous Issues AddressedThe operator precedence issue from my previous review has been resolved. The code has been refactored to use the 16 files reviewed | Confidence: 95% | Recommendation: Merge Review DetailsFiles Reviewed:
Changes Summary:
Checked: Security, bugs, performance, error handling - no issues found |
- Added formatUnknownMessageContent() to utils.ts for formatting unknown message content - Updated AskMessageRouter.tsx and SayMessageRouter.tsx to use the shared utility - Added 20 comprehensive unit tests for the new utility function - Reduces code duplication and improves testability
# Conflicts: # cli/src/ui/components/__tests__/ThinkingAnimation.test.tsx
Problem
This PR fixes CLI formatting issues identified in the analysis where unknown message types and JSON content were not being properly handled in both CI mode and interactive terminal mode. Specifically code search (see screenshots)
Before:
After: