Skip to content
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

fix: fix scroll to latest entry [DET-3673] #936

Merged
merged 5 commits into from
Jul 28, 2020

Conversation

hkang1
Copy link
Contributor

@hkang1 hkang1 commented Jul 23, 2020

Description

There is a race condition between when new entries are fetched and when the scroll-to-latest button is clicked. What is considered the end or the bottom may differ due to timing. To address this, add a threshold of what is considered to be the "bottom" or the "end" so there is some flexibility so the tailing feature will continue to function even though the scroll-to-latest does not necessary hit the absolute end (threshold provided as the wiggle room).

Test Plan

Commentary (optional)

@@ -323,7 +327,7 @@ const LogViewer: React.FC<Props> = forwardRef((
*/
setTimeout(() => {
if (!container.current) return;
container.current.scrollTo({ top: container.current.scrollHeight });
container.current.scrollTo({ behavior: 'smooth', top: container.current.scrollHeight });
Copy link
Member

Choose a reason for hiding this comment

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

I personally prefer a janky instant jump to smooth animation (it's faster too)

Copy link
Member

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

I couldn't reproduce the issue on master reliably to be able to test the same behavior against this branch but this looks reasonable a slight downside being that if you slightly scroll away from the bottom you could still be in tailing mode.

An approach I was thinking of was using an explicit flag to show either we are tailing or not. this flag could be switched by

  1. user explicitly moves the scroll all the way to the bottom
  2. user explicitly moves the scroll away from the bottom
  3. maybe an actual UI toggle

then on every new entry instead of computing scroll position and pixel differences we would just look at the flag. I haven't spent much time on it so it might not be better/feasible/or trivial.

should we retest this after #876 has landed?

@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Jul 24, 2020
@hkang1 hkang1 force-pushed the 3673-fix-scroll-to-latest-entry branch from 94fc055 to 0e84ab2 Compare July 25, 2020 03:53
@hkang1
Copy link
Contributor Author

hkang1 commented Jul 27, 2020

I couldn't reproduce the issue on master reliably to be able to test the same behavior against this branch but this looks reasonable a slight downside being that if you slightly scroll away from the bottom you could still be in tailing mode.

An approach I was thinking of was using an explicit flag to show either we are tailing or not. this flag could be switched by

  1. user explicitly moves the scroll all the way to the bottom
  2. user explicitly moves the scroll away from the bottom
  3. maybe an actual UI toggle

then on every new entry instead of computing scroll position and pixel differences we would just look at the flag. I haven't spent much time on it so it might not be better/feasible/or trivial.

should we retest this after #876 has landed?

I like these ideas. I haven't looked into this yet, but it could be a bit tricky to distinguish between user triggered scrolling and scrolling triggered by tailing or other events. The detection of when to start or end the tailing behavior could end up being some similar logic. Let's leave this for a separate ticket as it seems a bit non-trivial.

On thought is that the scroll to latest entry button can just be the 'enable tailUI. So it can show atailing` icon + highlight/glow when tailing, and otherwise it would just be the default look.

@hkang1 hkang1 force-pushed the 3673-fix-scroll-to-latest-entry branch from 0280080 to 3e59bd1 Compare July 28, 2020 04:46
@hkang1 hkang1 merged commit 0cc7821 into determined-ai:master Jul 28, 2020
@hkang1 hkang1 deleted the 3673-fix-scroll-to-latest-entry branch July 28, 2020 06:12
stoksc pushed a commit that referenced this pull request Jul 20, 2023
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
eecsliu pushed a commit that referenced this pull request Jul 24, 2023
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
stoksc pushed a commit that referenced this pull request Oct 17, 2023
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
azhou-determined pushed a commit that referenced this pull request Dec 7, 2023
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
wes-turner pushed a commit that referenced this pull request Feb 2, 2024
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
@dannysauer dannysauer added this to the 0.12.13 milestone Feb 6, 2024
rb-determined-ai pushed a commit that referenced this pull request Feb 29, 2024
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
amandavialva01 pushed a commit that referenced this pull request Mar 18, 2024
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
eecsliu pushed a commit that referenced this pull request Apr 18, 2024
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
eecsliu pushed a commit to determined-ai/determined-release-testing that referenced this pull request Apr 22, 2024
Makes the configuration for o184i054 (Grenoble)
a little more like the current setup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants