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.
This commit is contained in:
Tam Nhu Tran
2026-04-25 12:36:58 -04:00
parent e0d2ab0444
commit 37de42d463
2 changed files with 40 additions and 3 deletions
+13 -2
View File
@@ -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}
@@ -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;
}