fix: content scroll when switching database views #161
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideRefines scroll restoration and layout handling when switching database views so that virtualized grid/board content retains its scroll position without jumps, by keeping views mounted during loading, coordinating height locking and delayed scroll restoration, and making the grid virtualizer’s parent offset measurement more stable. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In DatabaseViews, using
NodeJS.TimeoutforheightLockTimeoutRefcan cause type issues in the browser/DOM environment; consider switching tonumber | nullorReturnType<typeof setTimeout> | nullfor better compatibility. - The inline style object for opacity/visibility/transition in DatabaseViews could be extracted into a CSS class or styled component to keep JSX cleaner and make the transition behavior easier to reuse and tweak.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In DatabaseViews, using `NodeJS.Timeout` for `heightLockTimeoutRef` can cause type issues in the browser/DOM environment; consider switching to `number | null` or `ReturnType<typeof setTimeout> | null` for better compatibility.
- The inline style object for opacity/visibility/transition in DatabaseViews could be extracted into a CSS class or styled component to keep JSX cleaner and make the transition behavior easier to reuse and tweak.
## Individual Comments
### Comment 1
<location> `src/components/database/DatabaseViews.tsx:48` </location>
<code_context>
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);
const value = useMemo(() => {
return Math.max(
</code_context>
<issue_to_address>
**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:
```typescript
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.
</issue_to_address>
### Comment 2
<location> `src/components/database/components/grid/grid-table/useGridVirtualizer.ts:74-80` </location>
<code_context>
+ 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) {
</code_context>
<issue_to_address>
**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.
</issue_to_address>
### Comment 3
<location> `src/components/database/DatabaseViews.tsx:173` </location>
<code_context>
- scrollElement.scrollTop = targetScroll;
- }
- };
+ const maxRAFs = 3; // Wait 3 animation frames (≈50ms at 60fps) for layout to settle
+ const rafIds: number[] = [];
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the scroll restoration logic by using a single RAF ref and shared helpers for the height-lock timeout to make the lifecycle easier to follow.
You can reduce complexity in the new scroll restoration / height-lock code without changing behavior by:
1) Using a single `rafIdRef` instead of an array
2) Centralizing the height lock timeout management
Both changes keep the timing and behavior, but make the lifecycle easier to reason about.
1) Replace `rafIds` with a single ref
Since the RAF chain is sequential, only one frame is pending at any moment. You can store just the latest id in a ref and cancel it in cleanup:
```ts
const scrollRestoreRafIdRef = useRef<number | null>(null);
```
Then simplify the effect:
```ts
useEffect(() => {
if (isLoading) return;
if (lastScrollRef.current === null) return;
const scrollElement = getScrollElement();
if (!scrollElement) {
lastScrollRef.current = null;
return;
}
const targetScroll = lastScrollRef.current;
let rafCount = 0;
const maxRAFs = 3;
const restoreScroll = () => {
rafCount++;
if (rafCount < maxRAFs) {
scrollRestoreRafIdRef.current = requestAnimationFrame(restoreScroll);
return;
}
if (Math.abs(scrollElement.scrollTop - targetScroll) > 1) {
scrollElement.scrollTop = targetScroll;
logDebug('[DatabaseViews] restored scroll position after layout settled', {
target: targetScroll,
current: scrollElement.scrollTop,
rafCount,
});
}
lastScrollRef.current = null;
setViewVisible(true);
if (!fixedHeight) {
heightLockTimeoutRef.current = setTimeout(() => {
setLockedHeight(null);
heightLockTimeoutRef.current = null;
}, 100);
}
};
scrollRestoreRafIdRef.current = requestAnimationFrame(restoreScroll);
return () => {
if (scrollRestoreRafIdRef.current != null) {
cancelAnimationFrame(scrollRestoreRafIdRef.current);
scrollRestoreRafIdRef.current = null;
}
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
heightLockTimeoutRef.current = null;
}
};
}, [isLoading, viewId, fixedHeight]);
```
This removes the `rafIds` array and the loop in cleanup while preserving the same “wait N frames then restore scroll” behavior.
2) Centralize height lock timeout handling
You’re currently touching `heightLockTimeoutRef` in both `handleViewChange` and the scroll restoration effect. Wrapping that in two small helpers keeps the logic consistent and easier to maintain:
```ts
const clearHeightLockTimeout = useCallback(() => {
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
heightLockTimeoutRef.current = null;
}
}, []);
const scheduleHeightUnlock = useCallback(() => {
if (fixedHeight) return;
clearHeightLockTimeout();
heightLockTimeoutRef.current = setTimeout(() => {
setLockedHeight(null);
heightLockTimeoutRef.current = null;
}, 100);
}, [clearHeightLockTimeout, fixedHeight]);
```
Use them in both places:
```ts
const handleViewChange = useCallback(
(newViewId: string) => {
const scrollElement = getScrollElement();
lastScrollRef.current = scrollElement?.scrollTop ?? null;
logDebug('[DatabaseViews] captured scroll before view change', {
scrollTop: lastScrollRef.current,
});
clearHeightLockTimeout();
const currentHeight = viewContainerRef.current?.offsetHeight;
const heightToLock = fixedHeight ?? currentHeight ?? null;
setLockedHeight(heightToLock ?? null);
logDebug('[DatabaseViews] handleViewChange height lock', {
currentHeight,
fixedHeight,
heightToLock,
});
setIsLoading(true);
setViewVisible(false);
onChangeView(newViewId);
},
[fixedHeight, onChangeView, clearHeightLockTimeout]
);
```
And in the scroll restoration effect:
```ts
// inside restoreScroll, after setViewVisible(true)
scheduleHeightUnlock();
```
This keeps:
- The “clear pending timeout on new transition” behavior
- The “release height lock after a small delay post-scroll-restore” behavior
but moves the timeout concerns into small, focused helpers, reducing the cognitive load in the main effect.
</issue_to_address>
### Comment 4
<location> `src/components/database/components/grid/grid-table/useGridVirtualizer.ts:51` </location>
<code_context>
+ // preventing any threshold-based rejection.
+ const parentOffsetRef = useRef<number | null>(null);
const [parentOffset, setParentOffset] = useState(0);
const rafIdRef = useRef<number>();
+ const isInitialMountRef = useRef(true);
</code_context>
<issue_to_address>
**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:
```ts
// 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
```
2) Extract the RAF chain into a small helper and simplify the logic:
```ts
// 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:
```ts
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]);
```
3) Keep the `data.length` dependency but simplify the explanation (no `isInitialMountRef` semantics needed):
```ts
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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); |
There was a problem hiding this comment.
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.
Comment on lines
74
to
54
| 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) { |
There was a problem hiding this comment.
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.
| // preventing any threshold-based rejection. | ||
| const parentOffsetRef = useRef<number | null>(null); | ||
| const [parentOffset, setParentOffset] = useState(0); | ||
| const rafIdRef = useRef<number>(); |
There was a problem hiding this comment.
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
isInitialMountRefand usingparentOffsetRef.current === nullas the sole “first measurement” indicator. - Using a single threshold value instead of dual thresholds.
- Extracting the RAF chaining into a tiny helper, so
updateParentOffsetis easier to reason about.
Concretely, you can refactor updateParentOffset like this:
- Use
parentOffsetRef.current === nullinstead ofisInitialMountRef+nullcheck:
// 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- 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]);- Keep the
data.lengthdependency but simplify the explanation (noisInitialMountRefsemantics 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
parentOffsetRefbetween view switches.
But it removes:
- The extra
isInitialMountRefstate and related comments. - Dual thresholds (10 vs 5px) and their branching.
- The inline multi-RAF control variables (
currentRaf,rafCount) cluttering the main logic.
josue693
pushed a commit
to josue693/AppFlowy-Web
that referenced
this pull request
Dec 21, 2025
fix: content scroll when switching database views
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Improve scroll stability and state preservation when switching database views to prevent content jumps.
Bug Fixes:
Enhancements: