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

Monaco bugs #2922

Merged
merged 1 commit into from
Dec 9, 2023
Merged

Monaco bugs #2922

merged 1 commit into from
Dec 9, 2023

Conversation

jameskerr
Copy link
Member

@jameskerr jameskerr commented Dec 6, 2023

This PR fixes two things.

  1. The monaco files are now served from the public folder of the app.
  2. The font size is remeasured whenever a new font loads on the page.

Notes, I tried to use the npm package as the source for the editor to load, but that did not work with Nextjs trying to server render things. Some dom apis were not available and it broke.

Fixes #2921

@jameskerr jameskerr requested a review from philrz December 6, 2023 19:17
Copy link
Contributor

@philrz philrz left a comment

Choose a reason for hiding this comment

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

I ran some tests with a Dev build based on commit a7b609e from this branch (Actions run) and it seems to fix both #2921 and #2915 so I'm a functional 👍 on this merging. A couple things I'll point out though:

  1. When I ran the test of the fix for Monaco editor doesn't load when airgapped #2915, the editor now loads fine, but I did still see the attached failed GET in DevTools. If it's benign I'm ok letting it happen, but figured I'd point it out.

image

  1. It's not clear to me why we went from local port 3000 to 4567 as part of these changes. Perhaps that's explained by your opening PR notes and I just don't see how it relates. This actually seems like a win for me regardless because Grafana happens to run locally by default on 3000 so we'll no longer clash with that port. That said, I've often seen it advocated that rather than putting magic numbers like 4567 directly into the code they should maybe be defined as constants somewhere and referenced by name so that way if we ever make a change like this again we'll only have to make the change in one place. If there's some reason why that's not easy in this case, no problem, but figured I'd point it out. 😄

@philrz
Copy link
Contributor

philrz commented Dec 8, 2023

@jameskerr: I just tested again now using the Dev build artifact from https://github.com/brimdata/zui/actions/runs/7146094385 that's at commit 5aeb461 of this branch. RE: #2915, what I observe looks much the same as what I reported in my prior test above: There's no errors in DevTools when I first launch the app, but that same GET error as before appears once I drag a file into the app and Preview & Load comes up, then it appears again when I click Load and am able to start querying. And as before, the editor otherwise gives the feeling of being healthy, so if we think these message are benign I'm fine living with them for now since the bugs being fixed here seem to be having more impact than noise in the DevTools console. But I don't know what other implications there might be so I'll let you make the call on if you wanna go ahead and merge with what's here. Just based on the tests, my Approve stands. 😉

image

@philrz
Copy link
Contributor

philrz commented Dec 8, 2023

Due to the #2915 fix being more involved than anticipated, @jameskerr and I discussed offline backing out that part of the changes and using this PR to deliver just what's needed to fix #2921. To that end, I just tested with a Dev build from https://github.com/brimdata/zui/actions/runs/7146603945 that's based on commit e98ff5d from this branch. It does indeed still address #2921. However, I used the opportunity to confirm that the symptom of #2915 is the same as it was before, and it looks like it's actually gotten worse: Whereas before the editor simply became unusable (hung at "Loading...") when running airgapped, now if launched when airgapped the entire app window is a blank white. Granted, the app was not particularly usable airgapped before either so this is arguably not much different (and, if anything, the fact that the breakage is less subtle is almost a blessing since it doesn't give the user any false impression that the app is going to be usable airgapped). But if this was unexpected it might be worthy of additional study before we merge.

image

@philrz
Copy link
Contributor

philrz commented Dec 8, 2023

I just did one more test, now with the Dev build from https://github.com/brimdata/zui/actions/runs/7147092706 which is based on commit 591e415. It showed that #2921 is still fixed, and the #2915 symptom is back to where it was with GA Zui v1.4.1, i.e., the app seems to load ok, but the editor hangs with "Loading..." which will be fixed separately.

Looks to me like it's good to merge. 👍

@jameskerr jameskerr merged commit 7da28bd into main Dec 9, 2023
6 checks passed
@jameskerr jameskerr deleted the monaco-bugs branch December 9, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cursor position becomes inaccurate while typing in editor
2 participants