Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
57 changes: 53 additions & 4 deletions src/components/view/view-event-list.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
RTCConnection,
TlsTunnel
} from '../../types';
import { UiStore } from '../../model/ui/ui-store';

import {
getSummaryColor,
Expand Down Expand Up @@ -54,6 +55,7 @@ interface ViewEventListProps {
isPaused: boolean;

contextMenuBuilder: ViewEventContextMenuBuilder;
uiStore: UiStore;

moveSelection: (distance: number) => void;
onSelected: (event: CollectedEvent | undefined) => void;
Expand Down Expand Up @@ -879,19 +881,37 @@ export class ViewEventList extends React.Component<ViewEventListProps> {
if (!listWindow) return true; // This means no rows, so we are effectively at the bottom
else return (listWindow.scrollTop + SCROLL_BOTTOM_MARGIN) >= (listWindow.scrollHeight - listWindow.offsetHeight);
}

private wasListAtBottom = true;
private updateScrolledState = () => {
requestAnimationFrame(() => { // Measure async, once the scroll has actually happened
this.wasListAtBottom = this.isListAtBottom();

// Only save scroll position after we've restored the initial state
if (this.hasRestoredInitialState) {
const listWindow = this.listBodyRef.current?.parentElement;
if (listWindow) {
this.props.uiStore.setViewScrollPosition(listWindow.scrollTop);
}
}
});
}

private hasRestoredInitialState = false;
componentDidMount() {
this.updateScrolledState();
// Don't save scroll state immediately - wait until we've restored first

// Use a more aggressive delay to ensure DOM is fully ready
setTimeout(() => {
this.restoreScrollPosition();

// Only start tracking scroll changes after we've restored
setTimeout(() => {
this.hasRestoredInitialState = true;
}, 100);
}, 100);
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to avoid the fixed timeouts here. I think we can do so though - we just need to wait until listBodyRef is ready, right? I think we can convert this into a callback ref, which means we can trigger the DOM interaction (updating the scroll position) immediately after that's rendered.

That will be a bit more responsive (no visible jumping around I hope) and should avoid most of the risk of the race condition you're protecting against with hasRestoredInitialState here as well. Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, good idea. I wasn't sure what to look for. I'll get rid of this and set it to be handled by a new callback ref (setListBodyRef)

}

componentDidUpdate() {
componentDidUpdate(prevProps: ViewEventListProps) {
if (this.listBodyRef.current?.parentElement?.contains(document.activeElement)) {
// If we previously had something here focused, and we've updated, update
// the focus too, to make sure it's in the right place.
Expand All @@ -901,7 +921,29 @@ export class ViewEventList extends React.Component<ViewEventListProps> {
// If we previously were scrolled to the bottom of the list, but now we're not,
// scroll there again ourselves now.
if (this.wasListAtBottom && !this.isListAtBottom()) {
this.listRef.current?.scrollToItem(this.props.events.length - 1);
this.listRef.current?.scrollToItem(this.props.events.length - 1);
} else if (prevProps.selectedEvent !== this.props.selectedEvent && this.props.selectedEvent) {
// If the selected event changed and we have a selected event, scroll to it
// This handles restoring the selected event when returning to the tab
this.scrollToEvent(this.props.selectedEvent);
} else if (prevProps.filteredEvents.length !== this.props.filteredEvents.length) {
// If the filtered events changed (e.g., new events loaded), try to restore scroll position
setTimeout(() => {
this.restoreScrollPosition();
}, 50);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I totally understand the goal of this bit. Can you explain when this applies in practice, and how it helps? I haven't noticed issues with the scroll position while filtering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put it there as more of a safeguard since I've had issues with wonky scroll position behavior depending on the browser/rendering engine in use. We can take it out if you don't expect that to change, I wasn't sure how consistent the web view experience was across platforms.

}
}

private restoreScrollPosition = () => {
// Only restore if we have a saved position
const savedPosition = this.props.uiStore.viewScrollPosition;
if (savedPosition > 0) {
const listWindow = this.listBodyRef.current?.parentElement;
if (listWindow) { // Only restore if we're not close to the current position (avoid unnecessary scrolling)
if (Math.abs(listWindow.scrollTop - savedPosition) > 10) {
listWindow.scrollTop = savedPosition;
}
}
}
}

Expand Down Expand Up @@ -1005,5 +1047,12 @@ export class ViewEventList extends React.Component<ViewEventListProps> {
}

event.preventDefault();
} // Public method to force scroll and selection restoration
public restoreViewState = () => {
if (this.props.selectedEvent) {
this.scrollToEvent(this.props.selectedEvent);
} else {
this.restoreScrollPosition();
}
}
}
30 changes: 25 additions & 5 deletions src/components/view/view-page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,13 @@ class ViewPage extends React.Component<ViewPageProps> {
filteredEventCount: [filteredEvents.length, events.length]
};
}

@computed
get selectedEvent() {
// First try to use the URL-based eventId, then fallback to the persisted selection
const targetEventId = this.props.eventId || this.props.uiStore.selectedEventId;

return _.find(this.props.eventsStore.events, {
id: this.props.eventId
id: targetEventId
});
}

Expand Down Expand Up @@ -240,12 +242,16 @@ class ViewPage extends React.Component<ViewPageProps> {
this.onBuildRuleFromExchange,
this.onPrepareToResendRequest
);

componentDidMount() {
// After first render, scroll to the selected event (or the end of the list) by default:
requestAnimationFrame(() => {
if (this.props.eventId && this.selectedEvent) {
this.onScrollToCenterEvent(this.selectedEvent);
} else if (!this.props.eventId && this.props.uiStore.selectedEventId) {
// If no URL eventId but we have a persisted selection, restore it
setTimeout(() => {
this.listRef.current?.restoreViewState();
}, 100);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto here - we should restructure this to trigger when listRef is set, directly, instead of guessing how long it'll take.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, added new callback (setListRef)

} else {
this.onScrollToEnd();
}
Expand Down Expand Up @@ -327,6 +333,18 @@ class ViewPage extends React.Component<ViewPageProps> {
})
);
}
componentWillUnmount() {
// Component is unmounting
}

componentDidUpdate(prevProps: ViewPageProps) {
// Only clear persisted selection if we're explicitly navigating to a different event via URL
// Don't clear it when going from eventId to no eventId (which happens when clearing selection)
if (this.props.eventId && prevProps.eventId && this.props.eventId !== prevProps.eventId) {
// Clear persisted selection only when explicitly navigating between different events via URL
this.props.uiStore.setSelectedEventId(undefined);
}
}

isSendAvailable() {
return versionSatisfies(serverVersion.value as string, SERVER_SEND_API_SUPPORTED);
Expand Down Expand Up @@ -447,8 +465,8 @@ class ViewPage extends React.Component<ViewPageProps> {

moveSelection={this.moveSelection}
onSelected={this.onSelected}

contextMenuBuilder={this.contextMenuBuilder}
uiStore={this.props.uiStore}

ref={this.listRef}
/>
Expand Down Expand Up @@ -488,9 +506,11 @@ class ViewPage extends React.Component<ViewPageProps> {
onSearchFiltersConsidered(filters: FilterSet | undefined) {
this.searchFiltersUnderConsideration = filters;
}

@action.bound
onSelected(event: CollectedEvent | undefined) {
// Persist the selected event to UiStore for tab switching
this.props.uiStore.setSelectedEventId(event?.id);

this.props.navigate(event
? `/view/${event.id}`
: '/view'
Expand Down
18 changes: 18 additions & 0 deletions src/model/ui/ui-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,13 @@ export class UiStore {
@observable
expandedViewCard: ExpandableViewCardKey | undefined;

// Store view list scroll position and selected entry to persist when switching tabs
@observable
viewScrollPosition: number = 0;

@observable
selectedEventId: string | undefined;

@computed
get viewCardProps() {
return _.mapValues(this.viewCardStates, (state, key) => ({
Expand Down Expand Up @@ -436,6 +443,17 @@ export class UiStore {
@persist @observable
exportSnippetFormat: string | undefined;

// Actions for persisting view state when switching tabs
@action.bound
setViewScrollPosition(position: number) {
this.viewScrollPosition = position;
}

@action.bound
setSelectedEventId(eventId: string | undefined) {
this.selectedEventId = eventId;
}

/**
* This tracks the context menu state *only if it's not handled natively*. This state
* is rendered by React as a fallback when DesktopApi.openContextMenu is not available.
Expand Down