Module Store #14092

farmaazon · 2025-10-01T11:38:07Z

Pull Request Description

Part of prerequisites for testing #13741 properly

Split graph store to graph and module stores. The module store contains all operations related to module, including AST edits. Graph store focuses on nodes and edges.
Project storeas are moved in repo to $/providers/openedProjects because they don't need to be bound to project-view only.
Replaced deprecated useXStore with useCurrentProject
When doing above I felt that handling "no project" case in every place is wearisome, so changed WithCurrentProject component, so it renders the default slot only when there is a currently opened project. If it's not, the fallback component is displayed. On one hand it makes many components remount when closing project with DocumentationPanel opened, on the other it saves a lot of developer's time.
Changed the edit API to make use of returned results. Because onUpdate may return promise, I made use of that information. As this change was considered controversial, it is a separate commit to check: 6afc0f1

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
~~[ ] Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.~~
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
~~[ ] Unit tests have been written where possible.~~
[ ] If meaningful changes were made to logic or tests affecting Enso Cloud integration in the libraries,
or the Snowflake database integration, a run of the Extra Tests has been scheduled.
- If applicable, it is suggested to paste a link to a successful run of the Extra Tests.

kazcw

As I mentioned during the daily, I think it would be nice to have some high-level docs clarifying the responsibilities of the module store vs. the graph store.

Besides that, I have some thoughts that may be considered non-blocking / ideas for related work (as this PR seems at risk of merge conflicts as it is):

We have two interfaces to useCurrentProject, ref and storesRefs--it seems we usually use storesRefs, and in the few cases we use the ref it seems we could just as well use storesRefs. Maybe we could simplify this?
In general, we often accept ToValue arguments to make APIs flexible to use; however, we are now accepting stores as arguments in many places. Typically, the stores are already Refs at the call site. If we type them as Refs in the argument lists, we could access them with the .value syntax, which IMO is more readable.
We are using module for a few different things--there is module: DistributedModule in the ProjectStore, module as the name of the ModuleStore, and sometimes we use it as the name of a MutableModule; and of course it has a meaning in JS. I don't have a better alternative to propose right now; maybe we should just rename some of these things if we come up with anything later.
You mentioned that we sometimes commit the same MutableModule more than once--I think we should detect this and print a warning; not necessarily in this PR though.

kazcw · 2025-10-07T08:45:27Z

...gui/src/project-view/components/GraphEditor/widgets/WidgetFunction/widgetFunctionCallInfo.ts

  })

-  const visualizationData = project.useVisualizationData(visualizationConfig)
+  // TODO[ao]:


kazcw · 2025-10-07T08:50:49Z

app/gui/src/project-view/components/GraphEditor/widgets/WidgetCheckbox.vue

-        directInteraction: true,
-      })
-    }
+    module.value.edit((edit) => {


This may be a good time to move the side effect here from the setter to a watcher

I thought it's nothing wrong with side effects in setters?

kazcw · 2025-10-07T08:53:13Z

app/gui/src/components/WithCurrentProject.vue

-        widgetRegistry: computed(() => ref.value?.widgetRegistry),
-      } satisfies ToRefs<{ [K in keyof OpenedProject]: OpenedProject[K] | undefined }>,
+        store: computed(() => ref.value.store),
+        names: computed(() => ref.value.names),


While we're changing this, I've always thought names an odd name for the store that holds project names. A lot of things have names.

That's because I assumed usages project.names, but as we use more storeRefs than refs, this should be projectNames

project.names is sort of a pun, as project does double duty as the "current project" and the "project name" store associated with the current project.

kazcw · 2025-10-07T08:56:44Z

app/gui/src/project-view/components/GraphEditor/widgets/WidgetFunction.vue

-      (app) => 'portId' in app.argument && app.argument.portId === origin,
-    )
-    const argApp = applications[argAppIndex]
+    const f = (edit: Ast.MutableModule) => {


As the usage of this function occurs many lines from its definition, I think a descriptive name would be helpful

kazcw · 2025-10-07T09:48:27Z

app/gui/src/providers/openedProjects/module/module.ts

+export type ModuleStore = ReturnType<typeof createModuleStore>
+
+/**
+ *


app/gui/src/providers/openedProjects/module/module.ts

kazcw · 2025-10-07T09:52:43Z

app/gui/src/providers/openedProjects/module/module.ts

+  function addMissingImportsDisregardConflicts(
+    edit: MutableModule,
+    imports: RequiredImport[],
+    existingImports?: AbstractImport[] | undefined,


I think we could remove existingImports and just always call analyzeImports. It looks like the argument is only used to avoid having to do the analysis twice, but the analysis shouldn't be an expensive operation so there's no need for this API complication.

Instead, I removed addMissingImportsDisregardConflicts from public API.

kazcw · 2025-10-07T09:53:55Z

app/gui/src/providers/openedProjects/module/module.ts

+
+    const importsToAdd = filterOutRedundantImports(existingImports_, imports)
+    if (!importsToAdd.length) return
+    addImports(edit.getVersion(topLevel), importsToAdd, projectNames)


Suggested change

addImports(edit.getVersion(topLevel), importsToAdd, projectNames)

addImports(topLevel, importsToAdd, projectNames)

kazcw · 2025-10-07T09:55:42Z

app/gui/src/providers/openedProjects/widgetRegistry/widgetRegistry.ts

-            edit.replaceValue(origin, ast)
-          } else if (typeof value === 'string') {
-            edit.tryGet(origin)?.syncToCode(value)
+    const f = (edit: Ast.MutableModule) => {


Another long f function

kazcw · 2025-10-07T09:59:52Z

app/ydoc-shared/src/ast/mutableModule.ts

+        const update = this.observeEvents(events, tryAsOrigin(transaction.origin))
+        for (const observer of this.updateObservers ?? []) observer(update)
+      } catch (err) {
+        console.error('AST observer caught an exception', err)


If we catch an error here, some type of misbehaviour is likely. If we try to continue, I think we should at least have a toast, something like "An internal error has occurred; reopening the project is recommended."

Can we “reopen” the project ourselves in this case? It does not make sense to transfer this decision to the user, who doesn’t understand what is happening.

Can we “reopen” the project ourselves in this case? It does not make sense to transfer this decision to the user, who doesn’t understand what is happening.

Probably, but not in this PR.

I just removed the try-catch for now. Actually, the AST state should not be broken because of throwing oberver, in this case it was broken because of something else.

I will add a task for tracking crashes in ProjectView.

Co-authored-by: Keziah Wesley <[email protected]>

farmaazon added 11 commits September 29, 2025 14:12

Module store and adjustments

d44d938

New edit API for opened module

6afc0f1

Fix for WithCurrentProject

d273631

Fix unit tests

8593f5e

WIP

a8e6c2a

Fix type imports

abb5264

Fix current resource context

6abeffb

Fixes and a lot of debug assertions

7c167f7

WIP more debugs

4ee374a

Remove sync watchers tracking AST

fc0d850

Remove unused code and add a disclaimer

83eb824

farmaazon self-assigned this Oct 1, 2025

farmaazon requested review from Frizi, vitvakatu and kazcw as code owners October 1, 2025 11:38

farmaazon added the CI: No changelog needed Do not require a changelog entry for this PR. label Oct 1, 2025

farmaazon requested review from AdRiley, PabloBuchu, indiv0 and somebody1234 as code owners October 1, 2025 11:38

Add necessary comment

75206d2

farmaazon changed the title ~~Wip/farmaazon/project stores refactor~~ Module Store Oct 1, 2025

farmaazon added 2 commits October 1, 2025 15:18

Lint

3fa6127

Prettier

42cccd9

kazcw reviewed Oct 7, 2025

View reviewed changes

Update app/gui/src/providers/openedProjects/module/module.ts

626ddba

Co-authored-by: Keziah Wesley <[email protected]>

	addImports(edit.getVersion(topLevel), importsToAdd, projectNames)
	addImports(topLevel, importsToAdd, projectNames)

Module Store #14092

Are you sure you want to change the base?

Module Store #14092

Uh oh!

Conversation

farmaazon commented Oct 1, 2025

Pull Request Description

Important Notes

Checklist

Uh oh!

kazcw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!