8000 fix(site): fix build logs scrolling on safari by BrunoQuaresma · Pull Request #14884 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

fix(site): fix build logs scrolling on safari #14884

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
Oct 3, 2024
Merged

fix(site): fix build logs scrolling on safari #14884

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1adeb36
fix(site): fix build logs scrolling on safari
BrunoQuaresma Sep 30, 2024
c81a42b
Extract scroll area to encapsulate effect and state changes
BrunoQuaresma Oct 1, 2024
170bc1c
Use state to set height
BrunoQuaresma Oct 2, 2024
bbb93bb
Use height instead of width
BrunoQuaresma Oct 3, 2024
7a5f53e
Try to fix test
BrunoQuaresma Oct 3, 2024
8912122
Merge branch 'main' of https://github.com/coder/coder into bq/fix-bui…
BrunoQuaresma Oct 3, 2024
01e66ed
Fix tests
BrunoQuaresma Oct 3, 2024
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
35 changes: 33 additions & 2 deletions site/src/pages/WorkspaceBuildPage/WorkspaceBuildPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import {
WorkspaceBuildDataSkeleton,
} from "modules/workspaces/WorkspaceBuildData/WorkspaceBuildData";
import { WorkspaceBuildLogs } from "modules/workspaces/WorkspaceBuildLogs/WorkspaceBuildLogs";
import type { FC } from "react";
import { useLayoutEffect, useRef, type FC } from "react";
import { Link } from "react-router-dom";
import { displayWorkspaceBuildDuration } from "utils/workspace";
import { Sidebar, SidebarCaption, SidebarItem } from "./Sidebar";
Expand Down Expand Up @@ -57,6 +57,34 @@ export const WorkspaceBuildPageView: FC<WorkspaceBuildPageViewProps> = ({
defaultValue: "build",
});

// TODO: Use only CSS to set the height of the content.
// Note: On Safari, when content is rendered inside a flex container and needs
// to scroll, the parent container must have a height set. Achieving this may
// require significant refactoring of the layout components where we currently
// use height and min-height set to 100%.
// Issue: https://github.com/coder/coder/issues/9687
// Reference: https://stackoverflow.com/questions/43381836/height100-works-in-chrome-but-not-in-safari
const contentRef = useRef<HTMLDivElement>(null);
useLayoutEffect(() => {
const contentEl = contentRef.current;
if (!contentEl) {
return;
}

const resizeObserver = new ResizeObserver(() => {
const parentEl = contentEl.parentElement;
if (!parentEl) {
return;
}
contentEl.style.setProperty("height", `${parentEl.clientHeight}px`);
});
resizeObserver.observe(document.body);

return () => {
resizeObserver.disconnect();
};
}, []);

if (!build) {
return <Loader />;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we have an edge case here that could break the effect logic:

  1. Component mounts with no build
  2. We set up the ref and effect to run after the first render
  3. The first render completes, but because there's no build, we return out a loader and don't attach the ref to anything
  4. The layout effect runs and whiffs
  5. We re-render at some point with a build, but because the layout effect dependency array is empty, nothing in it re-runs
  6. The only way to get the size mutations to apply at this point is if the browser resizes, but if the user never resizes anything, we're stuck with the same issue as before

Wondering if might be best to make a new component boundary and move this state there. That way, the state only mounts when we're guaranteed to have a build

Expand Down Expand Up @@ -144,7 +172,10 @@ export const WorkspaceBuildPageView: FC<WorkspaceBuildPageViewProps> = ({
))}
</Sidebar>

<div css={{ height: "100%", overflowY: "auto", width: "100%" }}>
<div
ref={contentRef}
css={{ height: "100%", overflowY: "auto", width: "100%" }}
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to update the height, I think we have no choice put to put the value in render state

I just tried making a test component to see what would happen if we don't put height changes in state, and it feels like it's too easy to accidentally wipe the adjustments away

Copy link
Member
@Parkreiner Parkreiner Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think I'd do something like this:

const [contentHeight, setContentHeight] = useState<string>()
const contentRef = useRef<HTMLDivElement>(null);
useLayoutEffect(() => {
  // It's been a while since I've had to do this, but dispatching a state update from the
  // resize callback might actually be too slow, and you might get a UI flicker still. You 
  // might need to set up the resize callback to dispatch state updates, but then also
  // do a one-time direct mutation when the effect first mounts
}, [])

css={{ height: contentHeight || "100%" }}

>
<Tabs active={tabState.value}>
<TabsList>
<TabLink to={`?${LOGS_TAB_KEY}=build`} value="build">
Expand Down
Loading
0