fix: keep log viewer pinned during drawer resize and minimize/reexpand - #48
Merged
Conversation
…e position after minimize/reexpand Add a ResizeObserver on the log scroll container that re-pins to the last entry when follow mode is active during any container size change (drag resize, minimize/expand, fullscreen toggle). When follow mode is off and the drawer is restored from minimized, the first visible line index is saved and restored so the user's scroll position is preserved.
📝 WalkthroughWalkthroughReplaces single scroll ref with a stateful Changes
Sequence Diagram(s)sequenceDiagram
participant RO as ResizeObserver
participant LV as LogViewer
participant V as Virtualizer
participant DOM as ScrollContainer
RO->>LV: notify resize (height change)
LV->>LV: read followRef, filteredLineCountRef, filteredEntriesRef, lastContainerHeight
alt follow === true and DOM.height > 0 and filteredEntries > 0
LV->>V: scrollToIndex(lastFilteredIndex)
V->>DOM: update scroll position
else follow === false and prevHeight === 0 and DOM.height > 0
LV->>LV: resolve savedAnchorTimestamp -> index via findEntryIndexByTime
LV->>V: scrollToIndex(restoredIndex)
V->>DOM: update scroll position
else follow === false and DOM.height === 0 and prevHeight > 0
V->>LV: provide current startIndex
LV->>LV: savedAnchorTimestamp = timestampOf(startIndex)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/providers/BottomDrawer/containers/LogViewer/index.tsx`:
- Around line 189-194: The follow-mode branch resets the auto-scroll guard too
early by using a single requestAnimationFrame; update the block that sets
isAutoScrolling.current = true, calls
rowVirtualizer.scrollToIndex(savedFirstVisibleRef.current, { align: 'start' }),
and clears savedFirstVisibleRef.current to use a double requestAnimationFrame
before setting isAutoScrolling.current = false so the scroll handler has settled
(i.e., wrap the current requestAnimationFrame callback in another
requestAnimationFrame), keeping references to isAutoScrolling.current,
rowVirtualizer.scrollToIndex, and savedFirstVisibleRef.current.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17052e90-94f7-4f16-be86-d31e8f70c952
📒 Files selected for processing (1)
ui/providers/BottomDrawer/containers/LogViewer/index.tsx
…mode guard timing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/providers/BottomDrawer/containers/LogViewer/index.tsx`:
- Around line 167-169: The effect that creates/attaches the ResizeObserver bails
out when parentRef.current is null, so the observer never attaches if the scroll
viewport mounts later; change the attachment logic to track the actual scroll
element via a state-backed or callback ref (e.g., create scrollElement state and
set it from a ref callback) and move the ResizeObserver setup/cleanup into a
useEffect that depends on scrollElement (or parentRef.current) so the observer
is (re-)attached when the viewport mounts; update any code that uses parentRef
(follow-mode repinning / restore-from-minimize) to reference the new
scrollElement ref/state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2ce28cab-0654-4e63-aaf3-d46b6b878310
📒 Files selected for processing (1)
ui/providers/BottomDrawer/containers/LogViewer/index.tsx
…r mounts late The scroll container only renders once log entries arrive. Using a callback ref + state ensures the ResizeObserver effect re-runs when the element actually appears in the DOM, rather than bailing out on initial mount when parentRef.current is still null.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/providers/BottomDrawer/containers/LogViewer/index.tsx`:
- Around line 169-170: savedFirstVisibleRef currently stores a mutable array
index (range.startIndex) which becomes invalid when filteredEntries reindexes
due to buffer eviction or filter changes; change the ref to store a stable
anchor (e.g., the LogEntry.timestamp or lineNumber of the first visible entry)
instead of an index, update the save site (where savedFirstVisibleRef is set
around lines 205-208) to capture entry.timestamp (or entry.lineNumber), and
update the restore site (where the ref is read around lines 191-195) to resolve
that anchor back to the current index by using the existing findEntryIndexByTime
(or analogous lookup) or jumpToTime logic against filteredEntries; adjust the
ref declaration (around lines 169-170) to use useRef<number | null> for the
timestamp/lineNumber type and ensure fallbacks if the anchor is not found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 29b91be7-d2ca-4de1-b16f-6fbc12b7831b
📒 Files selected for processing (1)
ui/providers/BottomDrawer/containers/LogViewer/index.tsx
Store the first visible entry's timestamp (stable across buffer eviction) rather than its array index. On restore, resolve the timestamp back to the current index via findEntryIndexByTime so the scroll position survives head eviction while minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/providers/BottomDrawer/containers/LogViewer/index.tsx`:
- Line 172: The savedVisibleTimestampRef currently stores only entry.timestamp
which makes restore ambiguous for duplicate timestamps; change the saved anchor
to a composite key (e.g., timestamp + sourceId + lineNumber) wherever
savedVisibleTimestampRef is set and read, then on restore attempt to locate the
exact entry by that composite (match timestamp, sourceId, lineNumber) before
falling back to findEntryIndexByTime(targetTimestamp). Update the code paths
that set/read savedVisibleTimestampRef (the savedVisibleTimestampRef declaration
and its setters/consumers around the restore logic) and ensure the restore logic
resolves the exact row first and only uses the timestamp-only lookup if that
composite entry is missing due to eviction or filtering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1adcd218-cff9-4ec2-a7c0-ba03e33c27ba
📒 Files selected for processing (1)
ui/providers/BottomDrawer/containers/LogViewer/index.tsx
| // Re-pin to bottom (or restore position) when the scroll container resizes. | ||
| // This covers drawer drag-resize, minimize/re-expand, and fullscreen toggle. | ||
| // We store a timestamp (not an index) so the anchor survives buffer eviction. | ||
| const savedVisibleTimestampRef = useRef<string | null>(null); |
There was a problem hiding this comment.
Anchor restore to a unique log entry.
Saving only entry.timestamp makes this restore ambiguous: findEntryIndexByTime() returns the first row with timestamp >= target, so duplicate timestamps snap back to the earliest sibling instead of the row that was actually visible. Save a composite anchor for the visible entry (for example timestamp + sourceId + lineNumber) and resolve that exact row first, falling back to the timestamp lookup only if the entry has been evicted or filtered out.
🔧 Suggested change
- // We store a timestamp (not an index) so the anchor survives buffer eviction.
- const savedVisibleTimestampRef = useRef<string | null>(null);
+ // Store a stable row identity so restore stays exact across eviction/filter changes.
+ const savedVisibleAnchorRef =
+ useRef<Pick<LogEntry, 'timestamp' | 'sourceId' | 'lineNumber'> | null>(null);
...
- } else if (prevHeight === 0 && savedVisibleTimestampRef.current !== null) {
+ } else if (prevHeight === 0 && savedVisibleAnchorRef.current !== null) {
+ const anchor = savedVisibleAnchorRef.current;
const idx = findEntryIndexByTime(
filteredEntriesRef.current,
- new Date(savedVisibleTimestampRef.current),
+ new Date(anchor.timestamp),
);
- savedVisibleTimestampRef.current = null;
- if (idx >= 0) {
+ let restoreIdx = idx;
+ for (let i = idx; i >= 0 && i < filteredEntriesRef.current.length; i += 1) {
+ const candidate = filteredEntriesRef.current[i];
+ if (candidate.timestamp !== anchor.timestamp) break;
+ if (
+ candidate.sourceId === anchor.sourceId
+ && candidate.lineNumber === anchor.lineNumber
+ ) {
+ restoreIdx = i;
+ break;
+ }
+ }
+ savedVisibleAnchorRef.current = null;
+ if (restoreIdx >= 0) {
isAutoScrolling.current = true;
- rowVirtualizer.scrollToIndex(idx, { align: 'start' });
+ rowVirtualizer.scrollToIndex(restoreIdx, { align: 'start' });
requestAnimationFrame(() => {
requestAnimationFrame(() => {
isAutoScrolling.current = false;
});
});
}
}
...
- if (entry?.timestamp) {
- savedVisibleTimestampRef.current = entry.timestamp;
+ if (entry?.timestamp) {
+ savedVisibleAnchorRef.current = {
+ timestamp: entry.timestamp,
+ sourceId: entry.sourceId,
+ lineNumber: entry.lineNumber,
+ };
}Also applies to: 194-203, 217-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/providers/BottomDrawer/containers/LogViewer/index.tsx` at line 172, The
savedVisibleTimestampRef currently stores only entry.timestamp which makes
restore ambiguous for duplicate timestamps; change the saved anchor to a
composite key (e.g., timestamp + sourceId + lineNumber) wherever
savedVisibleTimestampRef is set and read, then on restore attempt to locate the
exact entry by that composite (match timestamp, sourceId, lineNumber) before
falling back to findEntryIndexByTime(targetTimestamp). Update the code paths
that set/read savedVisibleTimestampRef (the savedVisibleTimestampRef declaration
and its setters/consumers around the restore logic) and ensure the restore logic
resolves the exact row first and only uses the timestamp-only lookup if that
composite entry is missing due to eviction or filtering.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
ResizeObserveron the log viewer scroll container to detect drawer size changesTickets
Verification
pnpm buildsucceedspnpm lintpasses (no new errors — all errors are pre-existing)Summary by CodeRabbit