mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-16 08:17:11 +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,
|
||||
}: 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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user