mirror of
https://github.com/tiennm99/ccs.git
synced 2026-09-02 18:18:43 +00:00
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:
@@ -39,7 +39,14 @@ export function JsonPane({
|
|||||||
className,
|
className,
|
||||||
}: JsonPaneProps) {
|
}: JsonPaneProps) {
|
||||||
const hasTabs = tabs && tabs.length > 0;
|
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 (
|
return (
|
||||||
<div className={cn('flex h-full flex-col', className)}>
|
<div className={cn('flex h-full flex-col', className)}>
|
||||||
@@ -61,7 +68,7 @@ export function JsonPane({
|
|||||||
{hasTabs ? (
|
{hasTabs ? (
|
||||||
<Tabs
|
<Tabs
|
||||||
value={activeTab}
|
value={activeTab}
|
||||||
onValueChange={setActiveTab}
|
onValueChange={setSelectedTabId}
|
||||||
className="flex min-h-0 flex-1 flex-col"
|
className="flex min-h-0 flex-1 flex-col"
|
||||||
>
|
>
|
||||||
<TabsList className="mx-3 mt-2 w-fit">
|
<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]);
|
const text = useMemo(() => JSON.stringify(data ?? {}, null, 2), [data]);
|
||||||
|
|
||||||
if (editable) {
|
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 (
|
return (
|
||||||
<textarea
|
<textarea
|
||||||
|
key={text}
|
||||||
defaultValue={text}
|
defaultValue={text}
|
||||||
onBlur={(e) => onChange?.(e.target.value)}
|
onBlur={(e) => onChange?.(e.target.value)}
|
||||||
spellCheck={false}
|
spellCheck={false}
|
||||||
|
|||||||
@@ -43,6 +43,13 @@ export function SectionRail({
|
|||||||
.filter((el): el is HTMLElement => el !== null);
|
.filter((el): el is HTMLElement => el !== null);
|
||||||
if (els.length === 0) return;
|
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(
|
const observer = new IntersectionObserver(
|
||||||
(entries) => {
|
(entries) => {
|
||||||
// Pick the section closest to top that's intersecting
|
// Pick the section closest to top that's intersecting
|
||||||
@@ -52,7 +59,7 @@ export function SectionRail({
|
|||||||
if (visible[0]) setActiveId(visible[0].target.id);
|
if (visible[0]) setActiveId(visible[0].target.id);
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
root: observeRoot ?? null,
|
root,
|
||||||
rootMargin: '-20% 0px -60% 0px',
|
rootMargin: '-20% 0px -60% 0px',
|
||||||
threshold: 0,
|
threshold: 0,
|
||||||
}
|
}
|
||||||
@@ -67,6 +74,8 @@ export function SectionRail({
|
|||||||
onJump(id);
|
onJump(id);
|
||||||
return;
|
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' });
|
document.getElementById(id)?.scrollIntoView({ behavior: 'smooth', block: 'start' });
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -108,3 +117,20 @@ export function SectionRail({
|
|||||||
</div>
|
</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;
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user