Monaco: stop being greedy, add resize observers, refactor

[josephjclark]

Implementation notes:

• [x] Set overflow-hidden on the collapsible columns in the inspector. This sets up the flexbox properly so that the editor resizes to fit its parent, rather than the other way around. This might be forcing a wider minimum width - but even in production a small or zoomed window very quickly
• [x] Use resize observers to control sizing with debouncing - a more modern an elegant solution. Suddenly I'm getting less flicker too - I didn't notice that while experimenting yesterday
• [x] Refactor Monaco boiler-plate into a new component which sets up theming and resize observers

Validation steps

• Open the inspector on a run (either run something or open it from history)
• Ensure you have three monaco editors visible
• Resize the browser window - horizontal, vertical, diagonal, go crazy with that stuff
• The three columns should all retain their size, perhaps after a small delay, especially when shrinking
• Change browser zoom and the columns should doggedly remain the same width, little heroes they are

Monaco may well flicker while resizing. On my machine at the moment it's mostly scrollbars flicking in and out - the general flicker seems to be reduced. Although tbh I don't trust that - if it keeps up I'll raise a bug in monaco.

Related issue

Fixes #2120

Review checklist

• [ ] I have performed a self-review of my code
• [ ] I have verified that all appropriate authorization policies (:owner, :admin, :editor, :viewer) have been implemented and tested
• [ ] If needed, I have updated the changelog
• [ ] Product has QA'd this feature

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 89.60%. Comparing base (144cf4a) to head (2a38a2a).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2126   +/-   ##
=======================================
  Coverage   89.60%   89.60%           
=======================================
  Files         269      269           
  Lines        8947     8947           
=======================================
  Hits         8017     8017           
  Misses        930      930           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[josephjclark]

Yeah @midigofrank I can see that actually it's totally not working ¯_(ツ)_/¯

I'm trying to dig into the root of the problem. This should just work really.

I have a hunch it's actualy an issue in react-monaco. It forces in this <section> element with a bunch of default styles and I think that's what makes it greedy.

I'm gonna continue digging in and hopefully this will evolve to a holistic layout fix for the editors :crossed_fingers:

[josephjclark]

So, two issues I'm staring down. No progress on either

1. The main code editor flickers weirdly on resize. That's because for some reason some of the resize calls cause the editor's contents to collapse to a tiny size ~100px. I can see it in the debugger. The editors root and parent nodes do not resize, just the editor area. It's this reduction of the editor's height and width that causes the flicker. Calling layout with a fixed size fixes this. I have no idea why layout is runninh with these weird values. I can't see anything in the parent DOM nodes that might be having an impact

2. The input editor is the only data clip view which eats up the real-estate. The output viewer is fine, perfectly well behaved. The input editor seems to ignore layout requests. I've even tried resize observers sending static values, but they don't work either. I think there's something in the parent DOM on that panel which stops the element shrinking. But I can't tell what, the parent elements all look fine and straightforward to me.

I'll dig around tomorrow with fresh eyes.

[josephjclark]

Ok, maybe it's a Friday afternoon, but this seems to be working. Suspiciously well actually. I'm gonna update the PR description.

[josephjclark]

I've plugged this in with rc-resize-observer but I wonder if https://www.npmjs.com/package/@react-hook/resize-observer is a better bet (it uses one global observer so its more efficient).

I'm gonna leave it for now but if we see performance issues on resize maybe it's worth a refactor. At least we only have to touch one file now!

[josephjclark]

@midigofrank we should be able to forward the refs through easily enough.

I'll make an example for you. Not sure if I'll do it here or create a merge branch... I'll get back to you

[josephjclark]

Actually Frank you can still get the editor and monaco refs from onMount, as the job editor does here. Just use onMount instead of beforeMount.

I don't even know why we're using beforeMount. I think it's an ill-advised pattern I introduced without thinking about it, but onMount is more conventional and does the same thing so far as I can tell.

[josephjclark]

@midigofrank you'll have to rebase the log viewer against this and it might be a little painful, sorry! It should just be a case of removing redundant code the new wrapper here should be fully compatible with monaco react. So maybe it won't be too bad.

I'm not familiar with the processes over here - does this want more QA or should we merge it?

[midigofrank]

Let's get @stuartc 's review as well. He was doing some work related to this

[midigofrank]

Actually Frank you can still get the editor and monaco refs from onMount, as the job editor does here. Just use onMount instead of beforeMount.

I don't even know why we're using beforeMount. I think it's an ill-advised pattern I introduced without thinking about it, but onMount is more conventional and does the same thing so far as I can tell.

@josephjclark I'm actually using the onMount callback, but won't that override the logic in here?

  const handleOnMount = useCallback((editor: any, monaco: Monaco) => {
    monacoRef.current = monaco;
    setTheme(monaco);
    onMount(editor, monaco);
  }, []);

  return (
    <ResizeObserver
      onResize={({ width, height }) => {
        // TODO maybe either debounce or track sizes
        monacoRef.current?.editor.layout({ width, height: height });
      }}
    >
      <Editor onMount={handleOnMount} {...props} />
    </ResizeObserver>
  );
};

[midigofrank]

Aah, wait. I think I see it. You're calling the onMount that has been added

[stuartc]

@josephjclark the resizing is definitely smoother than my implementation 👏 !

However if you open the inspector and then minimise the run viewer pane, the input pane just gobbles up the entire screen. And if you minimise either the 1st or 2nd panes the collapsed bar is about 10px wide and you can't expand them again.

[josephjclark]

@stuartc I don't really know why it would be smoother because it's the same basic components :thinking: Maybe passing in the width and height helps, I don't know

Yeah there's still something off when one of the panels is collapsed - we're back to the same old bullshit. I'm investigating

[josephjclark]

@stuartc Added some CSS hackery, what do you reckon?

[stuartc]

@stuartc Added some CSS hackery, what do you reckon?

Lol, that works in an uncomfortable way... but yeah this is good by me - will merge.