-
Notifications
You must be signed in to change notification settings - Fork 63
Restore intellisense to the Collaborative Editor #4387
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
base: main
Are you sure you want to change the base?
Conversation
assets/js/collaborative-editor/components/CollaborativeMonaco.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| return ( | ||
| <div className={cn('relative', className || 'h-full w-full')}> | ||
| {/* Loading indicator */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO remove this stupid comment
doc-han
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIGAAA!
It works but only after I've switched the adaptor.
eg.
- I select a salesforce adaptor from the canvas, then I visit the job code
- The intellisense doesn't seem to work
- Now I switch adaptor from salesforce to http. I see intellisense working.
could it be that it's downloading?
|
@doc-han I think the problem is I'll take a little look but I suspect that's an older bug - a smear in the old greatness |
|
@doc-han I've raised out OpenFn/kit#1245 - this is a long standing bug in kit. I may raise a fix separately, I think it's cheap |
|
@doc-han the severity of this shouldn't be too high: for new steps, when you pick an adaptor, it'll select the actual latest version, not |
elias-ba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @josephjclark this is looking good to me, nice job 👏🏽. Noted the discussion with @doc-han about latest being tracked separately.
bb7738a to
b6c0684
Compare
CollaborativeMonaco now calls monaco.languages.typescript.javascriptDefaults.setCompilerOptions() for intellisense. Both the global Monaco mock and the local mock in the diff test need to provide this API to prevent "Cannot read properties of undefined (reading 'typescript')" errors.
The loadDTS function makes network calls that can fail in test environments, causing unhandled rejection errors that make vitest exit with code 1 even when all tests pass.
|
Fixed the failing JS tests with two changes:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4387 +/- ##
==========================================
- Coverage 89.39% 89.37% -0.02%
==========================================
Files 425 425
Lines 20051 20051
==========================================
- Hits 17924 17921 -3
- Misses 2127 2130 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thank you @elias-ba !!! |
|
@doc-han @theroinaochieng would love to get this into the demo release tomorrow |
| // Create overflow widgets container for suggestions/tooltips | ||
| if (!overflowNodeRef.current) { | ||
| const overflowNode = document.createElement('div'); | ||
| overflowNode.className = 'monaco-editor widgets-overflow-container'; | ||
| document.body.appendChild(overflowNode); | ||
| overflowNodeRef.current = overflowNode; | ||
|
|
||
| // Update editor options with overflow container | ||
| // @ts-ignore - overflowWidgetsDomNode exists but isn't in updateOptions type | ||
| editor.updateOptions({ | ||
| overflowWidgetsDomNode: overflowNode, | ||
| fixedOverflowWidgets: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't monaco have a default fixedOverflowWidgets which is good enough? that's what monaco uses to keep all our tooltip high in z-index. I"m wondering why we had to create a new node to be appended to body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch!
This is old legacy editor code, introduced when we had problems with the z index. Feels fine without it in the collab editor. I'll cut it out
|
@josephjclark change log not added. |
|
@theroinaochieng I'd like to merge this to main and release it alongside single-session AI stuff. It's got some important fixes for demos IMO. It does increase the risk and QA effort of the release though |
Description
This PR fixes intellisense and code complete in the collaborative editor (it fixes the legacy editor too)
I've basically asked Claude to copy across the legacy solution and do a little refactoring.
Closes #4386
Note that magic metadata appears to not work. I'll look at that separately.
Validation steps
fn()!Also note that the only things in the pick list should be adaptor functions - no basic JS clutter
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy