Skip to content

Enhance view state management by integrating UiStore for scroll position and selected event persistence #160

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
merged 7 commits into from
Jul 15, 2025
Merged

Enhance view state management by integrating UiStore for scroll position and selected event persistence #160

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f02a1c5
Enhance view state management by integrating UiStore for scroll posit…
metheos Jun 15, 2025
661a7ba
revert formatter changes
metheos Jun 20, 2025
9cbe8ba
change setTimeout usages to callback functions and remove vestigial code
metheos Jun 25, 2025
adb5faf
Tweak some view-state persistence formatting
pimterry Jul 4, 2025
ebeb8a2
Drop filter-related scroll persistence logic
pimterry Jul 4, 2025
c300f10
Try to simplify view list scroll-restore logic
pimterry Jul 4, 2025
5f78f28
Don't automatically scroll to restored view list selections
pimterry Jul 15, 2025
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading