Refactor KclPlugin to not keep any state #9272

franknoirot · 2025-12-10T02:28:58Z

Towards #8885, broken out from #9190. Refactors the KCL LSP extension to not store any EditorView or other state. In addition, splits out concerns of highlighting engine-side entities and executing code. Users should experience no degradations from this refactor, although it is possible that this fixes some niche case bugs.

vercel · 2025-12-10T02:29:03Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
modeling-app	Ready	Preview	Comment	Dec 10, 2025 7:10pm

graphite-app · 2025-12-10T02:32:26Z

src/editor/plugins/lsp/kcl/index.ts

+  const executeKclDeferred = deferExecution(
+    async ({ newCode }: { newCode: string }) => {
+      void systemDeps.kclManager.writeToFile(newCode)
+      await systemDeps.kclManager.executeCode(newCode)
+    },
+    changesDelay
+  )


Race condition: writeToFile is not awaited before executeCode runs. The old code explicitly waited for the file write to complete via .then() before scheduling execution. Now execution can start while the file is still being written.

const executeKclDeferred = deferExecution( async ({ newCode }: { newCode: string }) => { await systemDeps.kclManager.writeToFile(newCode) await systemDeps.kclManager.executeCode(newCode) }, changesDelay )

Suggested change

const executeKclDeferred = deferExecution(

async ({ newCode }: { newCode: string }) => {

void systemDeps.kclManager.writeToFile(newCode)

await systemDeps.kclManager.executeCode(newCode)

},

changesDelay

)

const executeKclDeferred = deferExecution(

async ({ newCode }: { newCode: string }) => {

await systemDeps.kclManager.writeToFile(newCode)

await systemDeps.kclManager.executeCode(newCode)

},

changesDelay

)

Spotted by Graphite Agent

Is this helpful? React 👍 or 👎 to let us know.

I'm not sure this is a problem, writing to file at executing the code at the same time? Even if either fails we probably want to do the other?

Yeah that's what I was thinking. I shouldn't change that when so many other critical things are being nudged though, so I reverted it anyway.

andrewvarga · 2025-12-10T13:12:16Z

src/editor/plugins/lsp/kcl/index.ts

+
+    if (shouldExecute) {
+      const newCode = update.state.doc.toString()
+      executeKclDeferred({ newCode })


In this case newCode may not be the same as kclManager.code ?

andrewvarga · 2025-12-10T13:13:23Z

src/lang/KclManager.ts

  }
-  async executeCode(): Promise<void> {
-    const ast = await this.safeParse(this.code)
+  async executeCode(newCode = this.code): Promise<void> {


This pattern of optional params that use a member property if not given can be error prone but here it's needed because of executeKclDeferred right?

Nah I agree bad. It was my way to try to get new execution flow behavior (not relying on the duplicate kclManager.code state) to work without breaking all the call sites of kclManager.executeCode() scattered about, but it turns out this was the thing breaking that test! I've reverted it, thanks for highlighting it.

andrewvarga

It's so much nicer without the state in the plugin, less code as well, makes me wonder why we added that in the first place.
Great cleanup.

franknoirot · 2025-12-10T15:03:33Z

src/editor/plugins/lsp/kcl/index.ts

-    // Gotcha: Code can be written into the CodeMirror editor but not propagated to kclManager.code
-    // because the update function has not run. We need to initialize the kclManager.code when lsp initializes
-    // because new code could have been written into the editor before the update callback is initialized.
-    // There appears to be limited ways to safely get the current doc content. This appears to be sync and safe.
-    const kclLspPlugin = this.client.plugins.find((plugin) => {
-      return plugin.client.name === 'kcl'
-    })
-    if (kclLspPlugin) {
-      systemDeps.kclManager.code = view.state.doc.toString()
-    }


I revived this block of code as hackSetInitialCodeFromLsp, a "run once" extension that sets the code on kclManager initially. I believe we still need it until we get rid of React / tearing down the EditorView, although we have no automated tests that capture that.

franknoirot · 2025-12-10T15:05:54Z

src/editor/plugins/lsp/kcl/index.ts

-    // If we're in sketchSolveMode, update Rust state with the latest AST
-    // This handles the case where the user directly edits in the CodeMirror editor
-    // these are short term hacks while in rapid development for sketch revamp
-    // should be clean up.
-    try {
-      const modelingState = kclManager.modelingState
+  const executeKclDeferred = deferExecution(
+    async ({ newCode }: { newCode: string }) => {
+      const modelingState = systemDeps.kclManager.modelingState
      if (modelingState?.matches('sketchSolveMode')) {
-        await kclManager.executeCode()
-        const { sceneGraph, execOutcome } = await rustContext.hackSetProgram(
-          kclManager.ast,
-          await jsAppSettings()
-        )
-
-        // Convert SceneGraph to SceneGraphDelta and send to sketch solve machine
-        const sceneGraphDelta: SceneGraphDelta = {
-          new_graph: sceneGraph,
-          new_objects: [],
-          invalidates_ids: false,
-          exec_outcome: execOutcome,
-        }
-
-        const kclSource: SourceDelta = {
-          text: kclManager.code,
-        }
-
-        // Send event to sketch solve machine via modeling machine
-        kclManager.sendModelingEvent({
-          type: 'update sketch outcome',
-          data: {
-            kclSource,
-            sceneGraphDelta,
-          },
-        })


I had completely overlooked this new hack code for sketch solve mode while I was initially writing this PR last night! I've revived it as hackExecutionForSketchSolve.

franknoirot · 2025-12-10T15:06:59Z

src/editor/plugins/lsp/kcl/index.ts

+  const executeKclDeferred = deferExecution(
+    async ({ newCode }: { newCode: string }) => {
+      void systemDeps.kclManager.writeToFile(newCode)
+      await systemDeps.kclManager.executeCode(newCode)
+    },
+    changesDelay
+  )


Yeah that's what I was thinking. I shouldn't change that when so many other critical things are being nudged though, so I reverted it anyway.

graphite-app · 2025-12-10T18:34:45Z

src/editor/plugins/lsp/kcl/index.ts

-            sceneGraphDelta,
-          },
-        })
+        await hackExecutionForSketchSolve(newCode, systemDeps.kclManager)


Missing code assignment in sketch solve mode. When in sketchSolveMode, the kclManager.code is never updated with newCode, causing stale code to persist in the manager.

In the old implementation, kclManager.code was set before execution (line 149 in old code). In the refactored version, code is only set in the else branch (line 73), but not in the sketch solve path.

Fix:

await hackExecutionForSketchSolve(newCode, systemDeps.kclManager) systemDeps.kclManager.code = newCode

Or alternatively, set it inside hackExecutionForSketchSolve before the execution.

Suggested change

await hackExecutionForSketchSolve(newCode, systemDeps.kclManager)

await hackExecutionForSketchSolve(newCode, systemDeps.kclManager)

systemDeps.kclManager.code = newCode

Spotted by Graphite Agent

Is this helpful? React 👍 or 👎 to let us know.

Towards #8885, broken out from #9190. Refactors the KCL LSP extension to not store any EditorView or other state. In addition, splits out concerns of highlighting engine-side entities and executing code. Users should experience no degradations from this refactor, although it is possible that this fixes some niche case bugs.

franknoirot · 2025-12-10T20:00:37Z

This is tricky as shit!

If I write kclManager.code = newCode within my deferredExecution function, one test fails
If I don't do that and instead adjust kclManager.executeCode(newCode) to take a code parameter and not read from state, 18 tests fail

So annoying

Irev-Dev · 2025-12-13T18:02:54Z

If I write kclManager.code = newCode within my deferredExecution function, one test fails

Is the failure showing a real problem? just wanna make sure it's not a case where the test just needs to be tweaked.

franknoirot marked this pull request as ready for review December 10, 2025 02:29

franknoirot requested a review from a team as a code owner December 10, 2025 02:29

graphite-app bot reviewed Dec 10, 2025

View reviewed changes

vercel bot deployed to Preview December 10, 2025 02:43 View deployment

Irev-Dev approved these changes Dec 10, 2025

View reviewed changes

andrewvarga reviewed Dec 10, 2025

View reviewed changes

andrewvarga approved these changes Dec 10, 2025

View reviewed changes

franknoirot commented Dec 10, 2025

View reviewed changes

vercel bot deployed to Preview December 10, 2025 15:28 View deployment

vercel bot deployed to Preview December 10, 2025 18:03 View deployment

graphite-app bot reviewed Dec 10, 2025

View reviewed changes

vercel bot deployed to Preview December 10, 2025 18:45 View deployment

franknoirot force-pushed the franknoirot/8885/refactor-kcl-execution-highlight-extensions branch from bfdf1a2 to 662c41f Compare December 10, 2025 18:46

franknoirot added 6 commits December 10, 2025 13:55

wip fixes

798fd25

Got test passing with organization I dislike

69d8e44

Okay returned to the previous organization and confirmed the fix

944beaf

tsc and lint

a8d761a

Forgot to add back the line where we set the code before executing

7bdc1de

Move code set statement after executeCode

a84f53b

franknoirot force-pushed the franknoirot/8885/refactor-kcl-execution-highlight-extensions branch from 662c41f to a84f53b Compare December 10, 2025 18:56

vercel bot deployed to Preview December 10, 2025 19:10 View deployment

franknoirot marked this pull request as draft December 10, 2025 20:00

	await hackExecutionForSketchSolve(newCode, systemDeps.kclManager)
	await hackExecutionForSketchSolve(newCode, systemDeps.kclManager)
	systemDeps.kclManager.code = newCode

Refactor KclPlugin to not keep any state #9272

Are you sure you want to change the base?

Refactor KclPlugin to not keep any state #9272

Uh oh!

Conversation

franknoirot commented Dec 10, 2025

Uh oh!

vercel bot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graphite-app bot Dec 10, 2025

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!

andrewvarga 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!

graphite-app bot Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

franknoirot commented Dec 10, 2025

Uh oh!

Irev-Dev commented Dec 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vercel bot commented Dec 10, 2025 •

edited

Loading