Skip to content

fix: content scroll when switching database views #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
appflowy merged 1 commit into from
Nov 23, 2025
Merged

fix: content scroll when switching database views #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 67 additions & 49 deletions src/components/database/DatabaseViews.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ function DatabaseViews({
const viewContainerRef = useRef<HTMLDivElement | null>(null);
const [lockedHeight, setLockedHeight] = useState<number | null>(fixedHeight ?? null);
const lastScrollRef = useRef<number | null>(null);
const heightLockTimeoutRef = useRef<NodeJS.Timeout | null>(null);
Copy link

@sourcery-ai sourcery-ai bot Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Using NodeJS.Timeout here may be incompatible with browser timer typings.

In a DOM-centric React app, setTimeout returns a number, not NodeJS.Timeout. This typing can conflict with TS configs that only include dom or with non-Node environments. Consider a more portable type like number | null or ReturnType<typeof setTimeout> | null so it works cleanly across environments.

Suggested implementation:

  const lastScrollRef = useRef<number | null>(null);
  const heightLockTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
  const value = useMemo(() => {

Anywhere in this file where heightLockTimeoutRef.current is assigned or cleared (e.g., via setTimeout / clearTimeout), the existing code should already remain type-correct with this change:

  • Assigning: heightLockTimeoutRef.current = setTimeout(() => { ... }, delay);
  • Clearing:
    • if (heightLockTimeoutRef.current) { clearTimeout(heightLockTimeoutRef.current); }
    • or clearTimeout(heightLockTimeoutRef.current!); if you use non-null assertion.

No further changes should be necessary unless there are explicit type annotations elsewhere that still reference NodeJS.Timeout; if so, update those to use ReturnType<typeof setTimeout> as well for consistency.

const value = useMemo(() => {
return Math.max(
0,
Expand Down Expand Up @@ -79,7 +80,6 @@ function DatabaseViews({
};

observerEvent();

activeView.observe(observerEvent);

return () => {
Expand All @@ -96,6 +96,12 @@ function DatabaseViews({
scrollTop: lastScrollRef.current,
});

// Clear any pending height lock timeout from previous transition
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
heightLockTimeoutRef.current = null;
}

const currentHeight = viewContainerRef.current?.offsetHeight;
const heightToLock = fixedHeight ?? currentHeight ?? null;

Expand All @@ -114,16 +120,26 @@ function DatabaseViews({


const view = useMemo(() => {
if (isLoading) return null;
// IMPORTANT: Don't return null during loading to prevent unmounting
//
// Previously: `if (isLoading) return null;` caused the Grid to unmount
// Problem: This reset the virtualizer's parentOffsetRef from 955px → 0px
// Result: Scroll position jumped from 617px → 467px (150px jump)
//
// Solution: Keep components mounted but hide with opacity/visibility
// This preserves virtualizer state and prevents scroll jumps
switch (layout) {
case DatabaseViewLayout.Grid:
return <Grid />;
case DatabaseViewLayout.Board:
return <Board />;
case DatabaseViewLayout.Calendar:
return <Calendar />;
default:
// Return null only when layout is truly not set
return null;
}
}, [layout, isLoading]);
}, [layout]);

useEffect(() => {
if (!isLoading && viewContainerRef.current) {
Expand All @@ -140,8 +156,7 @@ function DatabaseViews({
}
}, [isLoading, viewVisible, layout, viewId, iidIndex]);

// Scroll restoration with RAF enforcement
// Board's autoScrollForElements interferes with scroll, so we enforce for multiple frames
// Enhanced scroll restoration with better timing coordination
useEffect(() => {
if (isLoading) return;
if (lastScrollRef.current === null) return;
Expand All @@ -155,61 +170,61 @@ function DatabaseViews({

const targetScroll = lastScrollRef.current;
let rafCount = 0;
let rafId: number;

// Temporarily prevent scroll events during restoration
const preventScroll = (e: Event) => {
if (scrollElement.scrollTop !== targetScroll) {
e.preventDefault();
scrollElement.scrollTop = targetScroll;
}
};
const maxRAFs = 3; // Wait 3 animation frames (≈50ms at 60fps) for layout to settle
const rafIds: number[] = [];

scrollElement.addEventListener('scroll', preventScroll, { passive: false });
const restoreScroll = () => {
rafCount++;

// Use RAF loop to enforce scroll position
const enforceScroll = () => {
const currentScroll = scrollElement.scrollTop;
const delta = Math.abs(currentScroll - targetScroll);
if (rafCount < maxRAFs) {
// Continue waiting for layout to stabilize
// Each RAF waits for the next browser paint (~16ms at 60fps)
const id = requestAnimationFrame(restoreScroll);
rafIds.push(id);
return;
}

if (delta > 0.5) {
// After layout has settled (3 RAFs), restore scroll position
// By this time:
// - New view component has rendered
// - Virtualizer has initialized and measured parentOffset
// - ResizeObserver has fired and stabilized
// - CSS transitions have started
if (Math.abs(scrollElement.scrollTop - targetScroll) > 1) {
scrollElement.scrollTop = targetScroll;
logDebug('[DatabaseViews] RAF restore scroll', {
frame: rafCount,
logDebug('[DatabaseViews] restored scroll position after layout settled', {
target: targetScroll,
current: currentScroll,
delta,
current: scrollElement.scrollTop,
rafCount,
});
}

rafCount++;
// Run for 5 frames (~80ms) to catch delayed scroll changes from Board mount
if (rafCount < 5) {
rafId = requestAnimationFrame(enforceScroll);
} else {
logDebug('[DatabaseViews] scroll restoration completed', {
final: scrollElement.scrollTop,
target: targetScroll,
});
// Remove scroll listener and clean up
scrollElement.removeEventListener('scroll', preventScroll);
lastScrollRef.current = null;
setViewVisible(true);
// Release height lock to allow view to resize to its natural height
if (!fixedHeight) {
// Clean up and show view
lastScrollRef.current = null;
setViewVisible(true);

// Release height lock after a small delay to allow content to settle
if (!fixedHeight) {
heightLockTimeoutRef.current = setTimeout(() => {
setLockedHeight(null);
}
heightLockTimeoutRef.current = null;
}, 100);
}
};

rafId = requestAnimationFrame(enforceScroll);
// Start the RAF chain
const firstId = requestAnimationFrame(restoreScroll);
rafIds.push(firstId);

return () => {
if (rafId) {
cancelAnimationFrame(rafId);
}
// Cancel all pending RAFs on cleanup
rafIds.forEach(id => cancelAnimationFrame(id));

scrollElement.removeEventListener('scroll', preventScroll);
// Cancel pending height lock timeout to prevent stale releases
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
heightLockTimeoutRef.current = null;
}
};
}, [isLoading, viewId, fixedHeight]);

Expand Down Expand Up @@ -267,11 +282,14 @@ function DatabaseViews({
>
<div
className='h-full w-full'
style={
effectiveHeight !== null
style={{
...(effectiveHeight !== null
? { height: `${effectiveHeight}px`, maxHeight: `${effectiveHeight}px` }
: {}
}
: {}),
opacity: viewVisible && !isLoading ? 1 : 0,
visibility: viewVisible && !isLoading ? 'visible' : 'hidden',
transition: 'opacity 150ms ease-in-out',
}}
>
<Suspense fallback={null}>
<ErrorBoundary fallbackRender={ElementFallbackRender}>{view}</ErrorBoundary>
Expand Down
101 changes: 69 additions & 32 deletions src/components/database/components/grid/grid-table/useGridVirtualizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ const logDebug = (...args: Parameters<typeof console.debug>) => {
export function useGridVirtualizer({ data, columns }: { columns: RenderColumn[]; data: RenderRow[] }) {
const { isDocumentBlock, paddingStart, paddingEnd } = useDatabaseContext();
const parentRef = useRef<HTMLDivElement | null>(null);

const parentOffsetRef = useRef(0);
const parentOffsetRef = useRef<number | null>(null);
const [parentOffset, setParentOffset] = useState(0);
const rafIdRef = useRef<number>();
Copy link

@sourcery-ai sourcery-ai bot Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (complexity): Consider simplifying the new parent offset logic by using a null check, a single threshold constant, and a small RAF helper to reduce branching and make updateParentOffset easier to follow.

You can keep all the new behavior (null-initialized offset, no reset on view switches, multi-RAF on first measurement) with less branching by:

  • Removing isInitialMountRef and using parentOffsetRef.current === null as the sole “first measurement” indicator.
  • Using a single threshold value instead of dual thresholds.
  • Extracting the RAF chaining into a tiny helper, so updateParentOffset is easier to reason about.

Concretely, you can refactor updateParentOffset like this:

  1. Use parentOffsetRef.current === null instead of isInitialMountRef + null check:
// outside the hook or at top-level in the file
const PARENT_OFFSET_STABLE_THRESHOLD = 5;

// inside the hook
const parentOffsetRef = useRef<number | null>(null);
// remove isInitialMountRef entirely
  1. Extract the RAF chain into a small helper and simplify the logic:
// inside the hook
const runAfterRafs = useCallback((count: number, fn: () => void) => {
  let rafs = 0;

  const tick = () => {
    rafs += 1;
    if (rafs < count) {
      rafIdRef.current = requestAnimationFrame(tick);
      return;
    }
    fn();
  };

  rafIdRef.current = requestAnimationFrame(tick);
}, []);

Then updateParentOffset becomes:

const updateParentOffset = useCallback(() => {
  if (rafIdRef.current !== undefined) {
    cancelAnimationFrame(rafIdRef.current);
  }

  const first = measureParentOffset();
  if (first === null) {
    logDebug('[GridVirtualizer] skip parent offset update; missing refs', {
      hasParent: !!parentRef.current,
      hasScrollElement: !!getScrollElement(),
    });
    return;
  }

  const isFirstMeasurement = parentOffsetRef.current === null;
  const rafCount = isFirstMeasurement ? 3 : 1;

  runAfterRafs(rafCount, () => {
    const measured = measureParentOffset();
    const nextOffset = measured ?? first;

    // First ever measurement: accept unconditionally
    if (isFirstMeasurement) {
      parentOffsetRef.current = nextOffset;
      setParentOffset(nextOffset);
      logDebug('[GridVirtualizer] initial parent offset set', { nextOffset });
      return;
    }

    const delta = Math.abs(nextOffset - parentOffsetRef.current!);

    if (delta < PARENT_OFFSET_STABLE_THRESHOLD) {
      logDebug('[GridVirtualizer] parent offset stable', {
        current: parentOffsetRef.current,
        measured: nextOffset,
        delta,
        threshold: PARENT_OFFSET_STABLE_THRESHOLD,
      });
      return;
    }

    parentOffsetRef.current = nextOffset;
    setParentOffset(nextOffset);
    logDebug('[GridVirtualizer] parent offset updated', {
      nextOffset,
      previous: parentOffset,
      delta,
    });
  });
}, [measureParentOffset, getScrollElement, parentOffset, runAfterRafs]);
  1. Keep the data.length dependency but simplify the explanation (no isInitialMountRef semantics needed):
useLayoutEffect(() => {
  // Re-measure when data length changes (proxy for view changes),
  // but keep the last known parentOffsetRef to avoid scroll jumps.
  updateParentOffset();
}, [updateParentOffset, data.length]);

This preserves:

  • Null-based “first measurement” that’s always accepted.
  • Multi-RAF behavior on first measurement only.
  • Stable threshold-based updates to avoid micro-adjustments.
  • No reset of parentOffsetRef between view switches.

But it removes:

  • The extra isInitialMountRef state and related comments.
  • Dual thresholds (10 vs 5px) and their branching.
  • The inline multi-RAF control variables (currentRaf, rafCount) cluttering the main logic.

const isInitialMountRef = useRef(true);

const getScrollElement = useCallback(() => {
if (!parentRef.current) return null;
Expand All @@ -48,10 +48,7 @@ export function useGridVirtualizer({ data, columns }: { columns: RenderColumn[];
cancelAnimationFrame(rafIdRef.current);
}

// Triple RAF to avoid transient measurements during layout thrash when views switch.
// First frame: Initial measurement (may be unstable)
// Second frame: Browser has processed initial layout
// Third frame: Layout is fully settled
// For embedded databases, measure offset more carefully
const first = measureParentOffset();

if (first === null) {
Comment on lines 48 to 54
Copy link

@sourcery-ai sourcery-ai bot Nov 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): isInitialMountRef is set to false before this branch, so the higher initial threshold is never used.

Since parentOffsetRef.current === null is handled in the earlier branch where you set isInitialMountRef.current = false, any execution that reaches this block will always see isInitialMountRef.current as false. As a result, threshold is effectively always 5 and the ? 10 : 5 never yields 10. If you truly need a looser threshold for the first update after mount, consider either deferring the change to isInitialMountRef.current until after the first delta-based update, or deriving the threshold from something like rafCount/a hasDeltaCheckRun flag. If that behavior isn’t needed, simplifying to a single constant threshold would avoid confusion.

Expand All @@ -62,38 +59,78 @@ export function useGridVirtualizer({ data, columns }: { columns: RenderColumn[];
return;
}

rafIdRef.current = requestAnimationFrame(() => {
rafIdRef.current = requestAnimationFrame(() => {
rafIdRef.current = requestAnimationFrame(() => {
const second = measureParentOffset();
const nextOffset = second ?? first;
const delta = Math.abs(nextOffset - parentOffsetRef.current);

// Only update if change is significant (>1px) to avoid micro-adjustments
if (delta < 1) {
logDebug('[GridVirtualizer] parent offset stable (delta < 1px)', {
current: parentOffsetRef.current,
measured: nextOffset,
delta,
});
return;
}

parentOffsetRef.current = nextOffset;
setParentOffset(nextOffset);
logDebug('[GridVirtualizer] parent offset updated', {
nextOffset,
previous: parentOffset,
delta,
});
// Use multiple RAFs during initial mount to ensure layout is stable
// This helps prevent scroll jumps during view transitions
const rafCount = isInitialMountRef.current ? 3 : 1;
let currentRaf = 0;

const performUpdate = () => {
currentRaf++;

if (currentRaf < rafCount) {
rafIdRef.current = requestAnimationFrame(performUpdate);
return;
}

const measured = measureParentOffset();
const nextOffset = measured ?? first;

// If this is the first measurement, always accept it without threshold check
// This prevents rejecting valid initial offsets (e.g., 955px) that would fail
// the delta check if we started from 0.
if (parentOffsetRef.current === null) {
parentOffsetRef.current = nextOffset;
setParentOffset(nextOffset);
logDebug('[GridVirtualizer] initial parent offset set', {
nextOffset,
isInitialMount: isInitialMountRef.current,
});
isInitialMountRef.current = false;
return;
}

const delta = Math.abs(nextOffset - parentOffsetRef.current);

// Only update if change is significant (>10px for initial, >5px after)
// Increased threshold for embedded databases to prevent flashing
const threshold = isInitialMountRef.current ? 10 : 5;

if (delta < threshold) {
logDebug('[GridVirtualizer] parent offset stable', {
current: parentOffsetRef.current,
measured: nextOffset,
delta,
threshold,
isInitialMount: isInitialMountRef.current,
});
isInitialMountRef.current = false;
return;
}

parentOffsetRef.current = nextOffset;
setParentOffset(nextOffset);
logDebug('[GridVirtualizer] parent offset updated', {
nextOffset,
previous: parentOffset,
delta,
isInitialMount: isInitialMountRef.current,
});
});
isInitialMountRef.current = false;
};

rafIdRef.current = requestAnimationFrame(performUpdate);
}, [measureParentOffset, getScrollElement, parentOffset]);

useLayoutEffect(() => {
// IMPORTANT: We don't reset isInitialMountRef here
//
// The Grid component now stays mounted during view switches (it's just hidden),
// so isInitialMountRef stays false after the first mount. This prevents the
// parentOffsetRef from being reset to null, which would cause scroll jumps.
//
// We watch data.length to detect when the view has changed and needs remeasurement.
updateParentOffset();
}, [updateParentOffset]);
}, [updateParentOffset, data.length]); // Watch data.length for view changes

const virtualizer = useVirtualizer({
count: data.length,
Expand Down
Loading