Better integration for terminal links (better context-menu and add a tooltip)#2934
Better integration for terminal links (better context-menu and add a tooltip)#2934
Conversation
Deploying waveterm with
|
| Latest commit: |
cfda907
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82b22339.waveterm.pages.dev |
| Branch Preview URL: | https://sawka-url-hover.waveterm.pages.dev |
WalkthroughThis pull request introduces tooltip functionality for hovered links in a terminal view component. It adds a new NullErrorBoundary error handling component, creates two interrelated tooltip primitives (TermTooltip and TermLinkTooltip) using Floating UI for mouse-position-anchored display, extends the TermWrap class with link hover tracking via hoveredLinkUri and onLinkHover callback, updates term context menu logic to use hovered link URIs instead of selected text URLs, and integrates the tooltip display in the main TerminalView component with error boundary protection. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
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 |
| }, | ||
| }; | ||
| refs.setPositionReference(virtualEl); | ||
| }, [isOpen, mousePos?.x, mousePos?.y]); |
There was a problem hiding this comment.
WARNING: Missing refs in dependency array
The useLayoutEffect calls refs.setPositionReference() but refs is not included in the dependency array. While refs from useFloating is typically stable, it's better to include it for correctness and to avoid potential stale closure issues.
| }, [isOpen, mousePos?.x, mousePos?.y]); | |
| }, [isOpen, mousePos?.x, mousePos?.y, refs]); |
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/app/view/term/term-tooltip.tsx (1)
90-95:clearMaxTimeoutis recreated on every render; consider hoisting into auseRef-backed stable reference.Since the function only closes over
maxTimeoutRef(a stable ref), it doesn't need to be recreated each render. A common approach is to define it as auseCallbackwith an empty dependency array or inline it directly inside the effect.♻️ Proposed refactor
- const clearMaxTimeout = () => { - if (maxTimeoutRef.current != null) { - window.clearTimeout(maxTimeoutRef.current); - maxTimeoutRef.current = null; - } - }; - React.useEffect(() => { if (termWrap == null) { return; } + + const clearMaxTimeout = () => { + if (maxTimeoutRef.current != null) { + window.clearTimeout(maxTimeoutRef.current); + maxTimeoutRef.current = null; + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/view/term/term-tooltip.tsx` around lines 90 - 95, The helper clearMaxTimeout is being recreated every render even though it only uses the stable ref maxTimeoutRef; make it stable by memoizing it (e.g. wrap clearMaxTimeout in useCallback with an empty dependency array) or move its logic inline into the effect that needs it, ensuring references remain to maxTimeoutRef.current and you still call window.clearTimeout and set maxTimeoutRef.current = null; update any callers to use the new stable clearMaxTimeout reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/app/element/errorboundary.tsx`:
- Around line 48-50: Replace the console.log in the ErrorBoundary's
componentDidCatch with console.error so caught exceptions are recorded as
errors; locate the componentDidCatch method in
frontend/app/element/errorboundary.tsx (the method receiving parameters error:
Error, info: React.ErrorInfo) and change the logging call to
console.error(`${this.props.debugName ?? "NullErrorBoundary"} error boundary
caught error`, error, info) so devtools and monitoring treat it as an error.
In `@frontend/app/view/term/term-model.ts`:
- Around line 796-799: The menu label building uses hoveredURL.hostname which is
empty for non-HTTP(S) schemes; update the label in the menu.push call (the block
that constructs "Open URL (" + hoveredURL.hostname + ")") to fall back to a
meaningful value such as hoveredURL.href (or hoveredURL.protocol +
hoveredURL.pathname) when hoveredURL.hostname is falsy so the label becomes
"Open URL (…)" with the actual URI for file://, ssh://, and custom schemes.
In `@frontend/app/view/term/term-tooltip.tsx`:
- Line 58: In the term-tooltip component's className string (the JSX attribute
on the element rendering the tooltip in term-tooltip.tsx), remove the accidental
double space between "bg-zinc-800/70" and "rounded-md" so the className tokens
are separated by a single space; update the className value on that JSX element
accordingly to avoid the extra whitespace.
---
Nitpick comments:
In `@frontend/app/view/term/term-tooltip.tsx`:
- Around line 90-95: The helper clearMaxTimeout is being recreated every render
even though it only uses the stable ref maxTimeoutRef; make it stable by
memoizing it (e.g. wrap clearMaxTimeout in useCallback with an empty dependency
array) or move its logic inline into the effect that needs it, ensuring
references remain to maxTimeoutRef.current and you still call
window.clearTimeout and set maxTimeoutRef.current = null; update any callers to
use the new stable clearMaxTimeout reference.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/element/errorboundary.tsxfrontend/app/view/term/term-model.tsfrontend/app/view/term/term-tooltip.tsxfrontend/app/view/term/term.tsxfrontend/app/view/term/termwrap.ts
| componentDidCatch(error: Error, info: React.ErrorInfo) { | ||
| console.log(`${this.props.debugName ?? "NullErrorBoundary"} error boundary caught error`, error, info); | ||
| } |
There was a problem hiding this comment.
Use console.error instead of console.log for caught errors.
Error boundary catches are exceptional conditions, not informational logs. Using console.log means they won't be surfaced in monitoring tools or browser devtools error filters.
🛠 Proposed fix
componentDidCatch(error: Error, info: React.ErrorInfo) {
- console.log(`${this.props.debugName ?? "NullErrorBoundary"} error boundary caught error`, error, info);
+ console.error(`${this.props.debugName ?? "NullErrorBoundary"} error boundary caught error`, error, info);
}📝 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.
| componentDidCatch(error: Error, info: React.ErrorInfo) { | |
| console.log(`${this.props.debugName ?? "NullErrorBoundary"} error boundary caught error`, error, info); | |
| } | |
| componentDidCatch(error: Error, info: React.ErrorInfo) { | |
| console.error(`${this.props.debugName ?? "NullErrorBoundary"} error boundary caught error`, error, info); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/element/errorboundary.tsx` around lines 48 - 50, Replace the
console.log in the ErrorBoundary's componentDidCatch with console.error so
caught exceptions are recorded as errors; locate the componentDidCatch method in
frontend/app/element/errorboundary.tsx (the method receiving parameters error:
Error, info: React.ErrorInfo) and change the logging call to
console.error(`${this.props.debugName ?? "NullErrorBoundary"} error boundary
caught error`, error, info) so devtools and monitoring treat it as an error.
| if (hoveredURL) { | ||
| menu.push({ | ||
| label: "Open URL (" + selectionURL.hostname + ")", | ||
| label: "Open URL (" + hoveredURL.hostname + ")", | ||
| click: () => { |
There was a problem hiding this comment.
hoveredURL.hostname is empty for non-HTTP(S) URLs, producing "Open URL ()".
file://, ssh://, or other custom-scheme URIs have an empty hostname, resulting in a misleading label. Guard with a fallback.
🛠 Proposed fix
- label: "Open URL (" + hoveredURL.hostname + ")",
+ label: "Open URL (" + (hoveredURL.hostname || hoveredURL.href) + ")",📝 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.
| if (hoveredURL) { | |
| menu.push({ | |
| label: "Open URL (" + selectionURL.hostname + ")", | |
| label: "Open URL (" + hoveredURL.hostname + ")", | |
| click: () => { | |
| if (hoveredURL) { | |
| menu.push({ | |
| label: "Open URL (" + (hoveredURL.hostname || hoveredURL.href) + ")", | |
| click: () => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/term-model.ts` around lines 796 - 799, The menu label
building uses hoveredURL.hostname which is empty for non-HTTP(S) schemes; update
the label in the menu.push call (the block that constructs "Open URL (" +
hoveredURL.hostname + ")") to fall back to a meaningful value such as
hoveredURL.href (or hoveredURL.protocol + hoveredURL.pathname) when
hoveredURL.hostname is falsy so the label becomes "Open URL (…)" with the actual
URI for file://, ssh://, and custom schemes.
| <div | ||
| ref={refs.setFloating} | ||
| style={floatingStyles} | ||
| className="bg-zinc-800/70 rounded-md px-2 py-1 text-xs text-secondary shadow-xl z-50 pointer-events-none select-none" |
There was a problem hiding this comment.
Double space in className string.
- className="bg-zinc-800/70 rounded-md px-2 py-1 text-xs text-secondary shadow-xl z-50 pointer-events-none select-none"
+ className="bg-zinc-800/70 rounded-md px-2 py-1 text-xs text-secondary shadow-xl z-50 pointer-events-none select-none"📝 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.
| className="bg-zinc-800/70 rounded-md px-2 py-1 text-xs text-secondary shadow-xl z-50 pointer-events-none select-none" | |
| className="bg-zinc-800/70 rounded-md px-2 py-1 text-xs text-secondary shadow-xl z-50 pointer-events-none select-none" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/view/term/term-tooltip.tsx` at line 58, In the term-tooltip
component's className string (the JSX attribute on the element rendering the
tooltip in term-tooltip.tsx), remove the accidental double space between
"bg-zinc-800/70" and "rounded-md" so the className tokens are separated by a
single space; update the className value on that JSX element accordingly to
avoid the extra whitespace.
addresses issue #2901