From 37de42d463a1c81b06f852b036e7c9d1e40a2d72 Mon Sep 17 00:00:00 2001
From: Tam Nhu Tran <kaitran.ntt@gmail.com>
Date: Sat, 25 Apr 2026 12:36:58 -0400
Subject: [PATCH] fix(ui): address PR-Agent review feedback on JsonPane and
 SectionRail

JsonPane:
- Reset active tab when the tabs prop changes. Previously activeTab was
  initialized once and never re-synced; a parent swapping the tab set
  (e.g. selecting a different entity) could leave us pointing at a stale
  id, leaving the pane empty and Copy returning '{}'.
- Force textarea remount on data change in editable mode via key={text}.
  Uncontrolled defaultValue retained the prior value when the underlying
  config changed, so onBlur could save stale text.

SectionRail:
- Default scroll-spy root to the nearest scrollable ancestor instead of
  the page viewport. FormPane wraps its body in a shadcn ScrollArea, so
  IntersectionObserver with root:null was watching the wrong scroller and
  the active section never updated as the form scrolled.
- observeRoot prop still wins when supplied for explicit overrides.
---
 ui/src/components/config-layout/json-pane.tsx | 15 ++++++++--
 .../components/config-layout/section-rail.tsx | 28 ++++++++++++++++++-
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/ui/src/components/config-layout/json-pane.tsx b/ui/src/components/config-layout/json-pane.tsx
index 35fa0414..1c48fac7 100644
--- a/ui/src/components/config-layout/json-pane.tsx
+++ b/ui/src/components/config-layout/json-pane.tsx
@@ -39,7 +39,14 @@ export function JsonPane({
   className,
 }: JsonPaneProps) {
   const hasTabs = tabs && tabs.length > 0;
-  const [activeTab, setActiveTab] = useState<string>(tabs?.[0]?.id ?? 'data');
+  const [selectedTabId, setSelectedTabId] = useState<string>(tabs?.[0]?.id ?? 'data');
+
+  // Derive the *effective* active tab during render rather than mutating state
+  // in an effect. If the parent swaps the `tabs` array (e.g. selects a different
+  // entity), the previous selectedTabId may no longer exist — fall back to the
+  // first available tab so the pane never shows empty content.
+  const activeTab =
+    hasTabs && tabs.some((t) => t.id === selectedTabId) ? selectedTabId : (tabs?.[0]?.id ?? 'data');
 
   return (
     <div className={cn('flex h-full flex-col', className)}>
@@ -61,7 +68,7 @@ export function JsonPane({
       {hasTabs ? (
         <Tabs
           value={activeTab}
-          onValueChange={setActiveTab}
+          onValueChange={setSelectedTabId}
           className="flex min-h-0 flex-1 flex-col"
         >
           <TabsList className="mx-3 mt-2 w-fit">
@@ -94,8 +101,12 @@ function JsonView({ data, editable, onChange }: JsonViewProps) {
   const text = useMemo(() => JSON.stringify(data ?? {}, null, 2), [data]);
 
   if (editable) {
+    // `key={text}` forces React to remount the textarea when the underlying data
+    // changes (e.g. parent swaps the selected entity). Without this, an
+    // uncontrolled textarea retains the prior value and onBlur saves stale text.
     return (
       <textarea
+        key={text}
         defaultValue={text}
         onBlur={(e) => onChange?.(e.target.value)}
         spellCheck={false}
diff --git a/ui/src/components/config-layout/section-rail.tsx b/ui/src/components/config-layout/section-rail.tsx
index 2b80f016..796ef52a 100644
--- a/ui/src/components/config-layout/section-rail.tsx
+++ b/ui/src/components/config-layout/section-rail.tsx
@@ -43,6 +43,13 @@ export function SectionRail({
       .filter((el): el is HTMLElement => el !== null);
     if (els.length === 0) return;
 
+    // FormPane wraps its body in shadcn ScrollArea (radix), which means the
+    // actual scrolling element is an ancestor with overflow-y:auto/scroll —
+    // NOT the page viewport. Walk up from the first FormSection to find it.
+    // Without this, IntersectionObserver watches the wrong scroller and the
+    // active section never updates as the form scrolls.
+    const root = observeRoot ?? findScrollableAncestor(els[0]);
+
     const observer = new IntersectionObserver(
       (entries) => {
         // Pick the section closest to top that's intersecting
@@ -52,7 +59,7 @@ export function SectionRail({
         if (visible[0]) setActiveId(visible[0].target.id);
       },
       {
-        root: observeRoot ?? null,
+        root,
         rootMargin: '-20% 0px -60% 0px',
         threshold: 0,
       }
@@ -67,6 +74,8 @@ export function SectionRail({
       onJump(id);
       return;
     }
+    // scrollIntoView walks ancestors — works whether the scroller is the
+    // viewport or the FormPane's internal ScrollArea.
     document.getElementById(id)?.scrollIntoView({ behavior: 'smooth', block: 'start' });
   };
 
@@ -108,3 +117,20 @@ export function SectionRail({
     </div>
   );
 }
+
+/**
+ * Walk up the DOM until we find an element whose computed style scrolls vertically.
+ * Returns null if none found (IntersectionObserver treats null as the viewport).
+ */
+function findScrollableAncestor(el: HTMLElement | null): HTMLElement | null {
+  let node: HTMLElement | null = el?.parentElement ?? null;
+  while (node && node !== document.body) {
+    const style = window.getComputedStyle(node);
+    const overflowY = style.overflowY;
+    if (overflowY === 'auto' || overflowY === 'scroll' || overflowY === 'overlay') {
+      return node;
+    }
+    node = node.parentElement;
+  }
+  return null;
+}