-
Notifications
You must be signed in to change notification settings - Fork 905
fix(desktop): refresh v2 terminal link tooltip editor + nudge plain clicks #3552
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
Changes from all commits
d930ff5
48254eb
1196764
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,75 +1,68 @@ | ||||||
| import type { ExternalApp } from "@superset/local-db"; | ||||||
| import { useEffect, useState } from "react"; | ||||||
| import { AnimatePresence, motion } from "framer-motion"; | ||||||
| import { createPortal } from "react-dom"; | ||||||
| import { getAppOption } from "renderer/components/OpenInExternalDropdown/constants"; | ||||||
| import type { LinkHoverInfo } from "renderer/lib/terminal/terminal-runtime-registry"; | ||||||
| import { electronTrpcClient } from "renderer/lib/trpc-client"; | ||||||
| import type { LinkClickHint } from "../../hooks/useLinkClickHint"; | ||||||
| import type { HoveredLink } from "../../hooks/useLinkHoverState"; | ||||||
|
|
||||||
| const TOOLTIP_OFFSET_PX = 14; | ||||||
| const TOOLTIP_CLASSES = | ||||||
| "pointer-events-none fixed z-50 w-fit rounded-md bg-foreground px-3 py-1.5 text-xs text-background"; | ||||||
|
|
||||||
| const isMac = | ||||||
| typeof navigator !== "undefined" && | ||||||
| navigator.platform.toLowerCase().includes("mac"); | ||||||
| const MOD_LABEL = isMac ? "⌘" : "Ctrl"; | ||||||
| const MOD_SHIFT_LABEL = isMac ? "⌘⇧" : "Ctrl+Shift"; | ||||||
| const HINT_LABEL = `Hold ${MOD_LABEL} to open · ${MOD_SHIFT_LABEL} for external`; | ||||||
|
|
||||||
| interface LinkHoverTooltipProps { | ||||||
| hoveredLink: HoveredLink | null; | ||||||
| hint: LinkClickHint | null; | ||||||
| } | ||||||
|
|
||||||
| function getAppLabel(app: ExternalApp): string { | ||||||
| const option = getAppOption(app); | ||||||
| return option?.displayLabel ?? option?.label ?? "external editor"; | ||||||
| } | ||||||
|
|
||||||
| function getLabel( | ||||||
| info: LinkHoverInfo, | ||||||
| shift: boolean, | ||||||
| defaultEditor: ExternalApp | null, | ||||||
| ): string { | ||||||
| function getLabel(info: LinkHoverInfo, shift: boolean): string { | ||||||
| if (info.kind === "url") { | ||||||
| return shift ? "Open in external browser" : "Open in browser"; | ||||||
| } | ||||||
| if (shift) { | ||||||
| return defaultEditor | ||||||
| ? `Open in ${getAppLabel(defaultEditor)}` | ||||||
| : "Open externally"; | ||||||
| return shift ? "Open in external browser" : "Open in pane"; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Restore the non-shift URL tooltip label to match the browser action. Prompt for AI agents
Suggested change
|
||||||
| } | ||||||
| return info.isDirectory ? "Reveal in sidebar" : "Open in editor"; | ||||||
| if (shift) return "Open in external editor"; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Keep the shift-hover label tied to the current default editor instead of hard-coding a generic string. Prompt for AI agents |
||||||
| return info.isDirectory ? "Reveal in sidebar" : "Open in pane"; | ||||||
| } | ||||||
|
|
||||||
| export function LinkHoverTooltip({ hoveredLink }: LinkHoverTooltipProps) { | ||||||
| const [defaultEditor, setDefaultEditor] = useState<ExternalApp | null>(null); | ||||||
|
|
||||||
| useEffect(() => { | ||||||
| let cancelled = false; | ||||||
| electronTrpcClient.settings.getDefaultEditor | ||||||
| .query() | ||||||
| .then((editor) => { | ||||||
| if (!cancelled) setDefaultEditor(editor); | ||||||
| }) | ||||||
| .catch((error) => { | ||||||
| if (cancelled) return; | ||||||
| console.warn( | ||||||
| "[LinkHoverTooltip] Failed to fetch default editor:", | ||||||
| error, | ||||||
| ); | ||||||
| setDefaultEditor(null); | ||||||
| }); | ||||||
| return () => { | ||||||
| cancelled = true; | ||||||
| }; | ||||||
| }, []); | ||||||
|
|
||||||
| if (!hoveredLink || !hoveredLink.modifier) return null; | ||||||
|
|
||||||
| const label = getLabel(hoveredLink.info, hoveredLink.shift, defaultEditor); | ||||||
| export function LinkHoverTooltip({ hoveredLink, hint }: LinkHoverTooltipProps) { | ||||||
| const showingHover = Boolean(hoveredLink?.modifier); | ||||||
|
|
||||||
| return createPortal( | ||||||
| <div | ||||||
| className="pointer-events-none fixed z-50 w-fit rounded-md bg-foreground px-3 py-1.5 text-xs text-background" | ||||||
| style={{ | ||||||
| left: hoveredLink.clientX + TOOLTIP_OFFSET_PX, | ||||||
| top: hoveredLink.clientY + TOOLTIP_OFFSET_PX, | ||||||
| }} | ||||||
| > | ||||||
| {label} | ||||||
| </div>, | ||||||
| <> | ||||||
| {hoveredLink?.modifier && ( | ||||||
| <div | ||||||
| className={TOOLTIP_CLASSES} | ||||||
| style={{ | ||||||
| left: hoveredLink.clientX + TOOLTIP_OFFSET_PX, | ||||||
| top: hoveredLink.clientY + TOOLTIP_OFFSET_PX, | ||||||
| }} | ||||||
| > | ||||||
| {getLabel(hoveredLink.info, hoveredLink.shift)} | ||||||
| </div> | ||||||
| )} | ||||||
| <AnimatePresence> | ||||||
| {hint && !showingHover && ( | ||||||
| <motion.div | ||||||
| key="hint" | ||||||
| initial={{ opacity: 0 }} | ||||||
| animate={{ opacity: 1 }} | ||||||
| exit={{ opacity: 0 }} | ||||||
| transition={{ duration: 0.15 }} | ||||||
| className={TOOLTIP_CLASSES} | ||||||
| style={{ | ||||||
| left: hint.clientX + TOOLTIP_OFFSET_PX, | ||||||
| top: hint.clientY + TOOLTIP_OFFSET_PX, | ||||||
| }} | ||||||
| > | ||||||
| {HINT_LABEL} | ||||||
| </motion.div> | ||||||
| )} | ||||||
| </AnimatePresence> | ||||||
| </>, | ||||||
| document.body, | ||||||
| ); | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { type LinkClickHint, useLinkClickHint } from "./useLinkClickHint"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| import { useCallback, useEffect, useRef, useState } from "react"; | ||
|
|
||
| export interface LinkClickHint { | ||
| clientX: number; | ||
| clientY: number; | ||
| } | ||
|
|
||
| const HINT_DURATION_MS = 2000; | ||
| const MAX_HINTS_PER_SESSION = 2; | ||
|
|
||
| let hintsRemaining = MAX_HINTS_PER_SESSION; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is non-critical for production users since HMR isn't active there. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useLinkClickHint/useLinkClickHint.ts
Line: 11
Comment:
**Module-level counter survives HMR reloads in dev**
`hintsRemaining` is initialized at module evaluation time and never resets as long as the module stays in memory. In development with hot module replacement, a file save that reloads only the component (not the module) won't reset the counter, so developers testing the nudge will see it fire fewer times than expected after the first two clicks. A comment noting this is intentional (or using `sessionStorage` to survive actual page reloads while still being resettable on dev reload) would help future contributors understand the choice.
This is non-critical for production users since HMR isn't active there.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| export function useLinkClickHint() { | ||
| const [hint, setHint] = useState<LinkClickHint | null>(null); | ||
| const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
|
|
||
| const showHint = useCallback((clientX: number, clientY: number) => { | ||
| if (hintsRemaining <= 0) return; | ||
| hintsRemaining -= 1; | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| setHint({ clientX, clientY }); | ||
| timeoutRef.current = setTimeout(() => { | ||
| setHint(null); | ||
| timeoutRef.current = null; | ||
| }, HINT_DURATION_MS); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| return () => { | ||
| if (timeoutRef.current) clearTimeout(timeoutRef.current); | ||
| }; | ||
| }, []); | ||
|
|
||
| return { hint, showHint }; | ||
| } | ||
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.
navigator.platformis deprecatednavigator.platformhas been deprecated in modern browser/Electron versions. The recommended alternative isnavigator.userAgentData?.platform(with a fallback for non-supporting environments) or parsingnavigator.userAgent. While this still works fine in current Electron, it may produce warnings in future versions.Prompt To Fix With AI