Skip to content

Commit 9acdb36

Browse files
dlab-antondlab-anton
committed
tabs still work when leave settings and come back
1 parent af0b025 commit 9acdb36

File tree

2 files changed

+179
-76
lines changed

2 files changed

+179
-76
lines changed

‎src/activate/registerCommands.ts‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,11 @@ const getCommandsMap = ({ context, outputChannel, provider }: RegisterCommandOpt
8585
"roo-cline.settingsButtonClicked": () => {
8686
const visibleProvider = getVisibleProviderOrLog(outputChannel)
8787
if (!visibleProvider) return
88+
// Post original action message
8889
visibleProvider.postMessageToWebview({ type: "action", action: "settingsButtonClicked" })
90+
// Also explicitly post the visibility message to trigger scroll reliably
91+
console.log("[settingsButtonClicked] Posting 'settingsVisible' message.")
92+
visibleProvider.postMessageToWebview({ type: "settingsVisible" })
8993
},
9094
"roo-cline.historyButtonClicked": () => {
9195
const visibleProvider = getVisibleProviderOrLog(outputChannel)
@@ -161,10 +165,29 @@ export const openClineInNewTab = async ({ context, outputChannel }: Omit<Registe
161165

162166
await tabProvider.resolveWebviewView(newPanel)
163167

168+
// Add listener for visibility changes to notify webview
169+
newPanel.onDidChangeViewState(
170+
(e) => {
171+
const panel = e.webviewPanel
172+
if (panel.visible) {
173+
console.log("Roo Code tab panel became visible, posting message...")
174+
panel.webview.postMessage({ type: "settingsVisible" }) // Use the same message type as in SettingsView.tsx
175+
} else {
176+
console.log("Roo Code tab panel became hidden.")
177+
}
178+
},
179+
null, // First null is for `thisArgs`
180+
context.subscriptions, // Register listener for disposal
181+
)
182+
164183
// Handle panel closing events.
165-
newPanel.onDidDispose(() => {
166-
setPanel(undefined, "tab")
167-
})
184+
newPanel.onDidDispose(
185+
() => {
186+
setPanel(undefined, "tab")
187+
},
188+
null,
189+
context.subscriptions, // Also register dispose listener
190+
)
168191

169192
// Lock the editor group so clicking on files doesn't open them over the panel.
170193
await delay(100)

‎webview-ui/src/components/settings/SettingsView.tsx‎

Lines changed: 153 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@ import React, {
44
useCallback,
55
useEffect,
66
useImperativeHandle,
7+
useLayoutEffect,
78
useMemo,
89
useRef,
910
useState,
1011
WheelEvent,
11-
} from "react"
12+
} from "react" // Add useLayoutEffect
1213
import { useAppTranslation } from "@/i18n/TranslationContext"
1314
import {
1415
CheckCheck,
@@ -318,32 +319,26 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
318319
[isChangeDetected],
319320
)
320321

321-
// Create refs for each tab
322-
const tabRefs = useRef<Record<SectionName, React.RefObject<HTMLButtonElement>>>({} as any)
322+
// Store direct DOM element refs for each tab
323+
const tabRefs = useRef<Record<SectionName, HTMLButtonElement | null>>({} as any)
323324

324-
// Initialize refs for each section
325-
useEffect(() => {
326-
sectionNames.forEach((name) => {
327-
if (!tabRefs.current[name]) {
328-
tabRefs.current[name] = React.createRef<HTMLButtonElement>()
329-
}
330-
})
331-
}, [])
325+
// Removed useEffect for pre-populating refs
332326

333-
const sections: { id: SectionName; icon: LucideIcon; ref: React.RefObject<HTMLButtonElement> }[] = useMemo(
327+
// Sections definition - no longer includes refs
328+
const sections: { id: SectionName; icon: LucideIcon }[] = useMemo(
334329
() => [
335-
{ id: "providers", icon: Webhook, ref: tabRefs.current.providers || React.createRef() },
336-
{ id: "autoApprove", icon: CheckCheck, ref: tabRefs.current.autoApprove || React.createRef() },
337-
{ id: "browser", icon: SquareMousePointer, ref: tabRefs.current.browser || React.createRef() },
338-
{ id: "checkpoints", icon: GitBranch, ref: tabRefs.current.checkpoints || React.createRef() },
339-
{ id: "notifications", icon: Bell, ref: tabRefs.current.notifications || React.createRef() },
340-
{ id: "contextManagement", icon: Database, ref: tabRefs.current.contextManagement || React.createRef() },
341-
{ id: "terminal", icon: SquareTerminal, ref: tabRefs.current.terminal || React.createRef() },
342-
{ id: "experimental", icon: FlaskConical, ref: tabRefs.current.experimental || React.createRef() },
343-
{ id: "language", icon: Globe, ref: tabRefs.current.language || React.createRef() },
344-
{ id: "about", icon: Info, ref: tabRefs.current.about || React.createRef() },
330+
{ id: "providers", icon: Webhook },
331+
{ id: "autoApprove", icon: CheckCheck },
332+
{ id: "browser", icon: SquareMousePointer },
333+
{ id: "checkpoints", icon: GitBranch },
334+
{ id: "notifications", icon: Bell },
335+
{ id: "contextManagement", icon: Database },
336+
{ id: "terminal", icon: SquareTerminal },
337+
{ id: "experimental", icon: FlaskConical },
338+
{ id: "language", icon: Globe },
339+
{ id: "about", icon: Info },
345340
],
346-
[tabRefs],
341+
[], // No dependencies needed now
347342
)
348343

349344
// Update target section logic to set active tab
@@ -353,55 +348,136 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
353348
}
354349
}, [targetSection])
355350

356-
// Add effect to conditionally scroll the active tab into view when it changes
357-
useEffect(() => {
358-
const activeTabElement = tabRefs.current[activeTab]?.current
359-
const containerElement = scrollContainerRef.current
360-
361-
if (activeTabElement && containerElement) {
362-
// Calculate the visible range within the scroll container
363-
const visibleLeft = containerElement.scrollLeft
364-
const visibleRight = containerElement.scrollLeft + containerElement.clientWidth
351+
// Function to scroll the active tab into view
352+
const scrollToActiveTab = useCallback(
353+
(checkVisibility = false) => {
354+
console.log(`[scrollToActiveTab] Called. checkVisibility: ${checkVisibility}, activeTab: ${activeTab}`) // Log entry
355+
const activeTabElement = tabRefs.current[activeTab] // Remove ?.current
356+
const containerElement = scrollContainerRef.current
365357

366-
// Calculate the tab's position within the scroll container
367-
const tabLeft = activeTabElement.offsetLeft
368-
const tabRight = activeTabElement.offsetLeft + activeTabElement.offsetWidth
358+
if (!activeTabElement) {
359+
console.warn(`[scrollToActiveTab] activeTabElement for tab '${activeTab}' not found.`) // Log missing ref
360+
return
361+
}
362+
if (!containerElement) {
363+
console.warn(`[scrollToActiveTab] containerElement not found.`) // Log missing ref
364+
return
365+
}
369366

370-
// Check if the tab is fully within the visible range
371-
const isVisible = tabLeft >= visibleLeft && tabRight <= visibleRight
367+
// Use nodeName for simpler logging
368+
console.log(
369+
`[scrollToActiveTab] Refs found: activeTabElement=${activeTabElement.nodeName}, containerElement=${containerElement.nodeName}`,
370+
) // Log refs found
371+
let shouldScroll = true
372+
if (checkVisibility) {
373+
console.log(`[scrollToActiveTab] Checking visibility...`) // Log visibility check start
374+
// Calculate the visible range within the scroll container
375+
const visibleLeft = containerElement.scrollLeft
376+
const visibleRight = containerElement.scrollLeft + containerElement.clientWidth
377+
378+
// Calculate the tab's position within the scroll container
379+
const tabLeft = activeTabElement.offsetLeft
380+
const tabRight = activeTabElement.offsetLeft + activeTabElement.offsetWidth
381+
382+
// Check if the tab is fully within the visible range
383+
const isVisible = tabLeft >= visibleLeft && tabRight <= visibleRight
384+
console.log(
385+
`[scrollToActiveTab] Visibility check: tabLeft=${tabLeft}, tabRight=${tabRight}, visibleLeft=${visibleLeft}, visibleRight=${visibleRight}, isVisible=${isVisible}`,
386+
) // Log visibility details
387+
shouldScroll = !isVisible
388+
} else {
389+
console.log(`[scrollToActiveTab] Skipping visibility check (scrolling unconditionally).`) // Log unconditional scroll path
390+
}
372391

373-
// Only scroll if the tab is not fully visible
374-
if (!isVisible) {
392+
if (shouldScroll) {
393+
console.log(`[scrollToActiveTab] Scrolling tab '${activeTab}' into view.`) // Log scroll action
375394
activeTabElement.scrollIntoView({
376395
behavior: "auto", // Use instant scrolling
377396
block: "nearest",
378397
inline: "center",
379398
})
399+
} else {
400+
console.log(`[scrollToActiveTab] Scroll not needed (shouldScroll is false).`) // Log scroll skipped
401+
}
402+
// Removed redundant 'else' block for ref check, handled by early returns.
403+
},
404+
[activeTab], // Dependency on activeTab ensures the correct tab element is used
405+
)
406+
407+
// Effect to scroll when the active tab *changes* (e.g., user click)
408+
// Only scrolls if the tab isn't already fully visible.
409+
useEffect(() => {
410+
scrollToActiveTab(true) // Pass true to check visibility before scrolling
411+
}, [activeTab, scrollToActiveTab]) // Depend on activeTab and the scroll function itself
412+
413+
// Effect to scroll when the webview becomes *visible*
414+
// Scrolls unconditionally to center the active tab.
415+
// Use useLayoutEffect to ensure refs are available after DOM mutations.
416+
useLayoutEffect(() => {
417+
const handleMessage = (event: MessageEvent) => {
418+
const message = event.data // The object sent from postMessage
419+
if (message.type === "settingsVisible") {
420+
console.log("Received settingsVisible message from extension (LayoutEffect).")
421+
// No setTimeout needed, useLayoutEffect runs after DOM updates
422+
scrollToActiveTab(false) // Pass false to scroll unconditionally
380423
}
381424
}
382-
}, [activeTab])
425+
426+
window.addEventListener("message", handleMessage)
427+
428+
// Cleanup listener on unmount
429+
return () => {
430+
window.removeEventListener("message", handleMessage)
431+
}
432+
}, [scrollToActiveTab]) // Depend on the scroll function
383433

384434
// Handle horizontal scrolling with mouse wheel
385435
const handleWheelScroll = useCallback((event: WheelEvent<HTMLDivElement>) => {
386-
const container = scrollContainerRef.current
436+
// Cast target to HTMLElement for broader compatibility if needed, but ref should give correct type
437+
const container = event.currentTarget as HTMLDivElement // Use event.currentTarget
387438
if (container) {
388439
// Use deltaY for vertical scroll wheels (most common)
389-
// Adjust sensitivity as needed
390-
const scrollAmount = event.deltaY * 2 // Multiplier for sensitivity
440+
const scrollAmount = event.deltaY * 2 // Adjust sensitivity
391441

392-
// Check if scrolling is possible
442+
// Check if horizontal scrolling is possible and needed
393443
if (container.scrollWidth > container.clientWidth) {
394-
container.scrollLeft += scrollAmount
395-
// Prevent default page scrolling if horizontal scroll happened
396-
if (
397-
(scrollAmount < 0 && container.scrollLeft > 0) ||
398-
(scrollAmount > 0 && container.scrollLeft < container.scrollWidth - container.clientWidth)
399-
) {
400-
event.preventDefault()
444+
const currentScrollLeft = container.scrollLeft
445+
const maxScrollLeft = container.scrollWidth - container.clientWidth
446+
447+
// Calculate new scroll position
448+
let newScrollLeft = currentScrollLeft + scrollAmount
449+
450+
// Prevent scrolling beyond boundaries
451+
newScrollLeft = Math.max(0, Math.min(newScrollLeft, maxScrollLeft))
452+
453+
// Only prevent default if a horizontal scroll actually happens
454+
if (newScrollLeft !== currentScrollLeft) {
455+
container.scrollLeft = newScrollLeft
456+
event.preventDefault() // Prevent default vertical page scroll
401457
}
402458
}
403459
}
404-
}, [])
460+
}, []) // No dependencies needed as it uses event.currentTarget
461+
462+
// Effect to attach wheel listener with passive: false
463+
useEffect(() => {
464+
const containerElement = scrollContainerRef.current
465+
466+
if (containerElement) {
467+
// Type assertion for the event handler
468+
const wheelHandler = (event: Event) => handleWheelScroll(event as unknown as WheelEvent<HTMLDivElement>)
469+
470+
containerElement.addEventListener("wheel", wheelHandler, { passive: false })
471+
472+
// Cleanup function
473+
return () => {
474+
// Check if element still exists before removing listener
475+
if (containerElement) {
476+
containerElement.removeEventListener("wheel", wheelHandler)
477+
}
478+
}
479+
}
480+
}, [handleWheelScroll]) // Re-attach if handleWheelScroll changes (though it shouldn't with empty deps)
405481

406482
return (
407483
<Tab>
@@ -441,33 +517,37 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
441517
{/* Scrollable tab container */}
442518
<div
443519
ref={scrollContainerRef} // Assign ref
444-
className={cn(settingsTabsContainer, scrollbarHideClasses, "w-full")} // Removed px-5
445-
onWheel={handleWheelScroll} // Add wheel handler
520+
className={cn(settingsTabsContainer, scrollbarHideClasses, "w-full")}
521+
// onWheel prop removed, listener added via useEffect
446522
>
447523
<TabList
448524
value={activeTab}
449525
onValueChange={(value) => handleTabChange(value as SectionName)}
450526
className={cn(settingsTabList, "w-full min-w-max")}
451527
data-testid="settings-tab-list">
452-
{sections.map(({ id, icon: Icon, ref }) => (
453-
<TabTrigger
454-
key={id}
455-
ref={ref}
456-
value={id}
457-
className={cn(
458-
activeTab === id
459-
? `${settingsTabTrigger} ${settingsTabTriggerActive}`
460-
: settingsTabTrigger,
461-
"flex-shrink-0", // Prevent tabs from shrinking
462-
"focus:ring-0", // Remove the focus ring styling
463-
)}
464-
data-testid={`tab-${id}`}>
465-
<div className="flex items-center gap-2">
466-
<Icon className="w-4 h-4" />
467-
<span>{t(`settings:sections.${id}`)}</span>
468-
</div>
469-
</TabTrigger>
470-
))}
528+
{sections.map(
529+
(
530+
{ id, icon: Icon }, // Remove 'ref' from destructuring
531+
) => (
532+
<TabTrigger
533+
key={id}
534+
ref={(element) => (tabRefs.current[id] = element)} // Keep callback ref here
535+
value={id}
536+
className={cn(
537+
activeTab === id
538+
? `${settingsTabTrigger} ${settingsTabTriggerActive}`
539+
: settingsTabTrigger,
540+
"flex-shrink-0", // Prevent tabs from shrinking
541+
"focus:ring-0", // Remove the focus ring styling
542+
)}
543+
data-testid={`tab-${id}`}>
544+
<div className="flex items-center gap-2">
545+
<Icon className="w-4 h-4" />
546+
<span>{t(`settings:sections.${id}`)}</span>
547+
</div>
548+
</TabTrigger>
549+
),
550+
)}
471551
</TabList>
472552
</div>
473553
{/* "More" dropdown button - always show it */}

0 commit comments

Comments
 (0)