Expose cloud event logging endpoint to GUI and render GUI editor as react component. #9951
Conversation
| await window.enso?.runApp(config, accessToken) | ||
| }, | ||
| }, | ||
| appRunner: null, |
There was a problem hiding this comment.
probably (?) fine for now?
but we will probably want to refactor the Vite setup so that the cloud build also bundles GUI2, very soon.
i guess it's out of scope for this PR? but i imagine we eventually want the cloud dashboard to import the module for GUI2, and remove appRunner as a configuration option
There was a problem hiding this comment.
Technically the GUI Vite setup does bundle dashboard and editor. We could just use that instead.
I agree that direct import will be preferred over the current appRunner indirection. We can refactor that once the apps are unified.
There was a problem hiding this comment.
yeah. about the GUI Vite setup - i guess the idea then, is to make sure we have the option to disable the Local backend at build time (for the cloud build). iirc, the Vite configs are already pretty similar so ideally there shouldn't be too many issues with that
app/ide-desktop/lib/dashboard/src/providers/RemoteBackendProvider.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/providers/RemoteBackendProvider.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/providers/RemoteBackendProvider.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/providers/BackendProvider.tsx
Outdated
Show resolved
Hide resolved
|
also i remember the dashboard's custom |
app/ide-desktop/lib/dashboard/src/providers/BackendProvider.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/providers/BackendProvider.tsx
Outdated
Show resolved
Hide resolved
|
@Frizi what is the status here? What is needed to undraft this PR? |
Frizi
added
the
CI: No changelog needed
Frizi
requested review from
farmaazon,
vitvakatu,
kazcw and
AdRiley
as code owners
app/gui2/src/App.vue
Outdated
| registerAutoBlurHandler() | ||
| const logger = provideEventLogger(toRef(props, 'logEvent'), toRef(props, 'projectId')) | ||
| watch( | ||
| [toRef(props, 'projectId')], |
There was a problem hiding this comment.
Cannot we just watch props.projectId? Why we wrap it in an array?
There was a problem hiding this comment.
i assume props.projectId comes from React so you wouldn't get reactivity. although, i also assume toRef(props, 'projectId') without the wrapping array would be valid, since it is a ref (i.e. it is a valid watch source)?
There was a problem hiding this comment.
The array is indeed unnecessary, but toRef is needed to pass a ref the watcher, not the current value.
| const projectStore = useProjectStore() | ||
| const suggestionDb = useSuggestionDbStore() | ||
|
|
||
| // Initialize suggestion db immediately, so it will be ready when user needs it. |
There was a problem hiding this comment.
AFAIR providing the suggestionDbStore should now initialize it, not the code below, so this comment is to be removed (before, we called useSuggestionDbStore inside onMounted)
|
|
||
| const syncModule = computed(() => proj.module && markRaw(new MutableModule(proj.module.doc.ydoc))) | ||
| export type GraphStore = ReturnType<typeof useGraphStore> | ||
| export const { injectFn: useGraphStore, provideFn: provideGraphStore } = createContextStore( |
There was a problem hiding this comment.
Everywhere else, we use inject prefix, not use.
There was a problem hiding this comment.
Actually the inject ones are a bit non-standard, and I'm considering renaming those. For now, I kept the names as they used to be for stores previously implemented through pinia. We might consider renaming things, but I'd do it outside of this PR.
| this.#setupUpdateHandler(lsRpc) | ||
| this.#loadGroups(lsRpc, projectStore.firstExecution) |
There was a problem hiding this comment.
Why do we change it? Isn't TS compile-time checking sufficient?
There was a problem hiding this comment.
In this case the use of private caused weird issues with typechecking after the types went through proxyRefs. I just decided to use native private fields to get around that, as those have no such issues.
app/gui2/src/components/GraphEditor/widgets/WidgetArgumentName.vue
Outdated
Show resolved
Hide resolved
Frizi
added
the
CI: Clean build required
| alias: { | ||
| ...resolve?.alias, | ||
| // Use `ffiPolyglot` module as `ffi` interface during the build. | ||
| 'shared/ast/ffi': fileURLToPath(new URL('./shared/ast/ffiPolyglot.ts', import.meta.url)), |
There was a problem hiding this comment.
@Frizi it does not work. If you run
$ npm --workspace=enso-gui2 run build-ydoc-server-polyglot
it will use the original ffi.ts module
Pull Request Description
fixes #9730
Added a
logEventmethod to remote backend implementation in dashboard. Added an always-present remote backend instance that can be used for logging even when running a local project (intentional behavior).Important Notes
Because the backend implementation requires access to always fresh session token, the logger needs to be periodically updated on GUI side, so it can continue to function. To accomplish that, I simplified the app loading logic to treat GUI as an ordinary react component, so its props can be updated with normal react rendering flow. That refactor also removed the dynamic GUI asset loading code that was only needed for Rust GUI.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.