-
Notifications
You must be signed in to change notification settings - Fork 25
perf(social): cap feed length to stop long browsing sessions going ch… #44
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -279,6 +279,11 @@ private void DrawFeedList(Rect listRect, SocialFeedScope scope) | |
| var snapshot = store.Feed(scope); | ||
| using (AppSurface.Begin(listRect)) | ||
| { | ||
| if (ImGui.GetScrollY() < 1f) | ||
| { | ||
| store.TrimFeed(scope, SocialFeedStore.FeedCap); | ||
|
|
||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: stray blank line inside the block. |
||
| } | ||
| if (snapshot.Length == 0) | ||
| { | ||
| var message = store.IsLoading(scope) ? Loc.T(L.Common.Loading) : | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,26 @@ public void ApplyMore(TPost[] incoming, string? nextCursor) | |
| } | ||
| } | ||
|
|
||
| public void Trim(int max) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking, answering the open question: option 1 is the right call and option 3 does not work. Per-page cursors cannot rewind a trim, because refreshes prepend to the head, so a recorded page boundary drifts deeper over the day. In the issue's own scenario (head growth from repeated app opens) the page-1 boundary ends up past the cap, and a boundary-aligned trim either refuses to fire or rewinds to a cursor from hours ago. A real rewind needs a cursor the client can derive from a post id or timestamp, which is a server change. |
||
| { | ||
| if (max <= 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| lock (gate) | ||
| { | ||
| if (items.Length <= max) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: please add coverage to |
||
| { | ||
| return; | ||
| } | ||
|
|
||
| var trimmed = new TPost[max]; | ||
| Array.Copy(items, trimmed, max); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: |
||
| items = trimmed; | ||
| } | ||
| } | ||
|
|
||
| public void Clear() | ||
| { | ||
| lock (gate) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,10 @@ private void OnSessionChanged() | |
|
|
||
| public bool LoadingMore(SocialFeedScope scope) => Lane(scope).LoadingMore; | ||
|
|
||
| public const int FeedCap = 300; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: the cap constant lives here, but the policy that decides when to apply it (the scroll check) lives in each app's draw method, so a new social feed opts out just by forgetting a line. |
||
|
|
||
| public void TrimFeed(SocialFeedScope scope, int max) => Lane(scope).Trim(max); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: Velvet is a third social feed with the same problem and it is not covered here. It has its own |
||
|
|
||
| private FeedLane<PostDto> Lane(SocialFeedScope scope) => | ||
| scope == SocialFeedScope.ForYou ? forYouLane : followingLane; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking:
OnOpeneddoes not reset feed scroll (onlyRefreshActiveFeedsetsfeedScrollTopPending) and ImGui keeps the child's scroll offset, so a user who leaves the feed scrolled down can go a long time without ever satisfying this guard. That is exactly the browse-all-day case in the issue, so the mitigation may fire less often than expected.