Skip to content

Conversation

@BrunoQuaresma
Copy link
Contributor

Fix #9687

Copy link
Contributor

@buenos-nachos buenos-nachos left a comment

Choose a reason for hiding this comment

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

The overall approach makes sense to me – my worry is that I don't think the code is set up 100% correctly, and it won't address all problems that could pop up

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

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
Contributor

@buenos-nachos buenos-nachos Sep 30, 2024

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%" }}

Comment on lines 88 to 90
if (!build) {
return <Loader />;
}
Copy link
Contributor

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

Copy link
Contributor

@buenos-nachos buenos-nachos left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for bearing with me

// 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);
const [height, setHeight] = useState<CSSProperties["width"]>("100%");
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny thing, but could we set the type to be based on CSSProperties["height"]>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol yes, idk why I used width 🤦

@BrunoQuaresma BrunoQuaresma merged commit 52f03db into main Oct 3, 2024
@BrunoQuaresma BrunoQuaresma deleted the bq/fix-build-logs-scrolling branch October 3, 2024 16:27
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workspace build log scrolling doesn't work in Safari

3 participants