Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a new AIFeedbackButtons React component and wires it into AIMessage to show for assistant responses. Extracts several tool-use UI components from aimessage.tsx into a new aitooluse.tsx module and exports AIToolUseGroup. Extends EmojiButton props to accept an optional icon and suppressFlyUp. Adds a new telemetry event name and property "waveai:feedback" (frontend types and backend telemetry serialization). Updates AI panel welcome text. Adds a normalization step for OSC 7 URL paths in termwrap. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Rationale: Heterogeneous changes across frontend and backend (new components, component extraction, prop signature change, telemetry type/serialization addition, and a terminal URL-normalization tweak) requiring verification of UI behavior, event logging, type alignment, and integration points. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/telemetry/telemetrydata/telemetrydata.go (1)
135-135: Validate feedback value server‑side.Add a small check so only "good" | "bad" are accepted for this event.
Apply near Validate():
@@ func (te *TEvent) Validate(current bool) error { if !ValidEventNames[te.Event] { return fmt.Errorf("TEvent.Event not valid: %q", te.Event) } + if te.Event == "waveai:feedback" { + if te.Props.WaveAIFeedback != "good" && te.Props.WaveAIFeedback != "bad" { + return fmt.Errorf("invalid waveai:feedback value: %q", te.Props.WaveAIFeedback) + } + }frontend/app/element/emojibutton.tsx (2)
7-21: Tighten props (emoji XOR icon) and add aria semantics.Prevent ambiguous renders and improve a11y.
Suggested refactor:
+type EmojiButtonProps = + | { emoji: string; icon?: never; isClicked: boolean; onClick: () => void; className?: string; suppressFlyUp?: boolean; ariaLabel?: string } + | { icon: string; emoji?: never; isClicked: boolean; onClick: () => void; className?: string; suppressFlyUp?: boolean; ariaLabel?: string }; - -export const EmojiButton = ({ - emoji, - icon, - isClicked, - onClick, - className, - suppressFlyUp, -}: { - emoji?: string; - icon?: string; - isClicked: boolean; - onClick: () => void; - className?: string; - suppressFlyUp?: boolean; -}) => { +export const EmojiButton = ({ emoji, icon, isClicked, onClick, className, suppressFlyUp, ariaLabel }: EmojiButtonProps) => {Also mark the icon as decorative:
-const content = icon ? <i className={makeIconClass(icon, false)} /> : emoji; +const content = icon ? <i className={makeIconClass(icon, false)} aria-hidden="true" /> : emoji;Optionally pass aria-label to the .
Also applies to: 33-33
25-31: Clear the timeout to avoid updates after unmount.Minor safety improvement for the fly‑up timer.
useLayoutEffect(() => { - if (isClicked && !prevClickedRef.current && !suppressFlyUp) { - setShowFloating(true); - setTimeout(() => setShowFloating(false), 600); - } - prevClickedRef.current = isClicked; -}, [isClicked, suppressFlyUp]); + let timer: number | undefined; + if (isClicked && !prevClickedRef.current && !suppressFlyUp) { + setShowFloating(true); + timer = window.setTimeout(() => setShowFloating(false), 600); + } + prevClickedRef.current = isClicked; + return () => { + if (timer) window.clearTimeout(timer); + }; +}, [isClicked, suppressFlyUp]);frontend/app/aipanel/aimessage.tsx (1)
242-249: Gate feedback on actual assistant text; precompute once.Avoid showing copy/feedback when no text is present (e.g., tool‑only messages), and reuse a single computed string.
Apply:
- {message.role === "assistant" && !isStreaming && displayParts.length > 0 && ( - <AIFeedbackButtons - messageText={parts - .filter((p) => p.type === "text") - .map((p) => p.text || "") - .join("\n")} - /> - )} + {(() => { + const messageText = parts + .filter((p) => p.type === "text" && (p.text?.trim().length ?? 0) > 0) + .map((p) => p.text!.trim()) + .join("\n"); + return message.role === "assistant" && !isStreaming && messageText.length > 0 ? ( + <AIFeedbackButtons messageText={messageText} /> + ) : null; + })()}Optional follow‑up: include message id/model in telemetry for attribution (would require extending AIFeedbackButtons props and its event payload).
frontend/app/aipanel/aitooluse.tsx (1)
82-95: Consider extracting keepalive interval constant.The 4000ms keepalive interval is duplicated in both
AIToolUseBatchandAIToolUse. While the duplication is acceptable given different component contexts, extracting the magic number to a module-level constant would improve maintainability.Add at the top of the file:
+const KEEPALIVE_INTERVAL_MS = 4000;Then use it in both locations:
- }, 4000); + }, KEEPALIVE_INTERVAL_MS);Also applies to: 154-165
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
frontend/app/aipanel/aifeedbackbuttons.tsx(1 hunks)frontend/app/aipanel/aimessage.tsx(3 hunks)frontend/app/aipanel/aipanel.tsx(1 hunks)frontend/app/aipanel/aitooluse.tsx(1 hunks)frontend/app/element/emojibutton.tsx(3 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/telemetry/telemetrydata/telemetrydata.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-14T06:30:54.751Z
Learnt from: sawka
PR: wavetermdev/waveterm#2430
File: frontend/app/aipanel/aimessage.tsx:137-144
Timestamp: 2025-10-14T06:30:54.751Z
Learning: In `frontend/app/aipanel/aimessage.tsx`, the `AIToolUseGroup` component splits file operation tool calls into separate batches (`fileOpsNeedApproval` and `fileOpsNoApproval`) based on their approval state before passing them to `AIToolUseBatch`. This ensures each batch has homogeneous approval states, making group-level approval handling valid.
Applied to files:
frontend/app/aipanel/aitooluse.tsxfrontend/app/aipanel/aimessage.tsx
🧬 Code graph analysis (3)
frontend/app/aipanel/aitooluse.tsx (4)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(25-25)frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/app/block/block-model.ts (1)
BlockModel(12-51)
frontend/app/aipanel/aifeedbackbuttons.tsx (3)
frontend/app/store/wshclientapi.ts (1)
RpcApi(542-542)frontend/app/store/wshrpcutil.ts (1)
TabRpcClient(37-37)frontend/util/util.ts (1)
makeIconClass(426-426)
frontend/app/aipanel/aimessage.tsx (2)
frontend/app/aipanel/aitypes.ts (1)
WaveUIMessagePart(25-25)frontend/app/aipanel/aifeedbackbuttons.tsx (1)
AIFeedbackButtons(13-94)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (13)
pkg/telemetry/telemetrydata/telemetrydata.go (1)
34-34: New event name acknowledged.Adding "waveai:feedback" to ValidEventNames looks correct.
frontend/app/element/emojibutton.tsx (1)
4-4: Import change OK.makeIconClass usage aligns with icon rendering.
frontend/app/aipanel/aipanel.tsx (1)
159-159: Confirm UX copy matches policy.“BETA: Free to use. Daily limits keep our costs in check.” — verify this reflects current limits to avoid support churn.
frontend/types/gotypes.d.ts (1)
997-997: Typed telemetry prop added — looks good.Union "good" | "bad" aligns with backend tags.
frontend/app/aipanel/aimessage.tsx (3)
8-10: Modularization imports LGTM.Using AIFeedbackButtons and AIToolUseGroup here is consistent with splitting batches by approval state. Based on learnings.
13-51: AIThinking UX is solid.Auto-scrolls reasoning and shows only the latest chunk; clean and lightweight.
171-201: Thinking state logic is clear.Spinner-only fallback via empty message is intentional and works with AIThinking.
frontend/app/aipanel/aifeedbackbuttons.tsx (2)
18-46: LGTM! Feedback logic is well-structured.The mutually exclusive feedback states and telemetry recording only on activation are correctly implemented.
54-93: LGTM! UI implementation is clean.The conditional styling, icon switching, and accessibility titles are properly implemented.
frontend/app/aipanel/aitooluse.tsx (4)
17-39: LGTM! Approval buttons implementation is clear.The dynamic labeling based on item count is well-implemented.
46-67: LGTM! Batch item rendering is correct.The status icon logic and error message handling are properly implemented.
191-224: LGTM! Block highlighting logic is well-implemented.The mouse enter/leave handlers properly manage refs and timeouts to avoid race conditions, and the cleanup effect ensures no memory leaks.
254-295: LGTM! Tool grouping logic correctly handles approval states.The categorization logic properly splits file operations by approval state into separate batches, ensuring homogeneous approval states per batch as intended. Based on learnings.
| const handleCopy = () => { | ||
| navigator.clipboard.writeText(messageText); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 2000); | ||
| }; |
There was a problem hiding this comment.
Add error handling and timeout cleanup.
The clipboard API can fail (e.g., permission denied, insecure context), and the timeout is not cleaned up if the component unmounts during the 2-second window.
Apply this diff to add error handling and cleanup:
+ const copyTimeoutRef = useRef<NodeJS.Timeout | null>(null);
+
+ useEffect(() => {
+ return () => {
+ if (copyTimeoutRef.current) {
+ clearTimeout(copyTimeoutRef.current);
+ }
+ };
+ }, []);
+
const handleCopy = () => {
- navigator.clipboard.writeText(messageText);
- setCopied(true);
- setTimeout(() => setCopied(false), 2000);
+ navigator.clipboard.writeText(messageText).then(
+ () => {
+ setCopied(true);
+ if (copyTimeoutRef.current) {
+ clearTimeout(copyTimeoutRef.current);
+ }
+ copyTimeoutRef.current = setTimeout(() => setCopied(false), 2000);
+ },
+ (err) => {
+ console.error("Failed to copy text:", err);
+ }
+ );
};Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <div className="flex items-start gap-2 p-2 rounded bg-gray-800 border border-gray-700"> | ||
| <div className="flex-1"> | ||
| <div className="font-semibold">Reading Files</div> |
There was a problem hiding this comment.
Hardcoded label may not match batch content.
The "Reading Files" label is hardcoded but the batch could contain read_dir operations or a mix of file operations. Consider making this dynamic or more generic (e.g., "File Operations").
Apply this diff to use a generic label:
- <div className="font-semibold">Reading Files</div>
+ <div className="font-semibold">File Operations</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="font-semibold">Reading Files</div> | |
| <div className="font-semibold">File Operations</div> |
🤖 Prompt for AI Agents
In frontend/app/aipanel/aitooluse.tsx around line 120, the UI uses a hardcoded
"Reading Files" label which may be inaccurate for batches containing read_dir or
mixed file operations; change it to a generic label like "File Operations" or
compute the label dynamically from the batch (e.g., if all ops are "read" use
"Reading Files", if all ops are "read_dir" use "Reading Directories", otherwise
"File Operations"). Implement by inspecting the batch ops where the label is
rendered, derive the text with a small helper or inline logic, and provide a
sensible fallback of "File Operations" to replace the hardcoded string.
Merges 59 commits from wavetermdev/waveterm v0.12.0 release into a5af fork. Resolves 49 merge conflicts across frontend, backend, and configuration files. ## Major Upstream Features Integrated ### AI Enhancements (v0.12.0) - AI Response Feedback + Copy Buttons (wavetermdev#2457) - Reasoning Deltas Display (wavetermdev#2443) - Google AI File Summarization (wavetermdev#2455) - `wsh ai` Command Reimplementation (wavetermdev#2435) - Terminal Context Improvements (wavetermdev#2444) - Batch Tool Approval System - Enhanced AI Panel with welcome message - Context menu support for AI messages ### Infrastructure Updates - Mobile User Agent Emulation for web widgets (wavetermdev#2454) - OSC 7 Support for Fish & PowerShell shells (wavetermdev#2456) - Log Rotation System (wavetermdev#2432) - Onboarding improvements - React 19 compatibility updates - Tailwind v4 migration progress - Dependency updates (50+ commits) ## Fork Features Preserved ✅ **Horizontal Widget Bar** (tabbar.tsx) - Widgets remain in horizontal tab bar (not reverted to sidebar) - Fork-specific layout maintained ✅ **Optional Pane Title Labels** (blockframe.tsx) - Auto-generated pane titles preserved - Custom block rendering logic intact ✅ **Layout Model Modifications** (layoutModel.ts) - Fork's widget positioning logic maintained - Horizontal layout integration preserved ## Conflict Resolution Summary **Configuration (8 files):** - Accepted upstream: .golangci.yml, Taskfile.yml, package.json, go.mod, etc. - All dependencies updated to v0.12.0 levels **Backend AI (13 files):** - Accepted upstream: All pkg/aiusechat/ files - New AI tools: read_dir, screenshot, terminal context - Enhanced OpenAI backend with reasoning support **Frontend AI Panel (12 files):** - Accepted upstream: All frontend/app/aipanel/ files - New features: reasoning display, feedback buttons, welcome message - Enhanced message handling and UX **Backend Infrastructure (7 files):** - Accepted upstream: emain, pkg/telemetry, pkg/wcore, pkg/wshrpc - Updated RPC types and telemetry data structures **Frontend Fork Features (8 files):** - Preserved fork: blockframe.tsx, tabbar.tsx, layoutModel.ts - Accepted upstream: keymodel.ts, wshclientapi.ts, termwrap.ts, etc. **Deleted Files (1 file):** - Removed: frontend/app/modals/tos.tsx (deleted upstream) ## Files Changed - Configuration: 8 files - Backend: 20+ files - Frontend: 25+ files - Total staged: 135 files ## Testing Required 1. Verify AI panel functionality (reasoning, feedback, tools) 2. Test horizontal widget bar (fork feature) 3. Test pane title labels (fork feature) 4. Build verification: `npm install && npm run build:dev` 5. Backend build: `go build ./...` 6. Full test suite: `npm test` ## Known Issues⚠️ Widget bar integration may need review - upstream removed widget sidebar⚠️ Layout changes may conflict with horizontal widget positioning⚠️ React 19 compatibility should be tested thoroughly ## Rollback If issues arise, rollback available at: - Tag: fork-v0.11.6-pre-v0.12-merge - Branch: backup-pre-v0.12-merge ## Next Steps 1. Test all functionality thoroughly 2. Fix any runtime errors from merge 3. Review fork feature integration points 4. Update documentation if needed 5. Create PR to main after validation --- 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
No description provided.