refactor(lsp): effectify LSP service with InstanceState#19150
refactor(lsp): effectify LSP service with InstanceState#19150kitlangton merged 15 commits intodevfrom
Conversation
Migrate LSP from Instance.state() to Effect Service/Layer/InstanceState pattern with makeRunPromise and async facades. - InstanceState for per-directory lifecycle with ScopedCache - Effect.fn tracing on all 14 service methods - Effect.fnUntraced for internal helpers (getClients, runForFile, runForAll) - addFinalizer for client shutdown on instance disposal - No changes to server.ts, client.ts, launch.ts, or language.ts - No consumer changes — all 15+ callers use unchanged async facades
packages/opencode/src/lsp/index.ts
Outdated
|
|
||
| const getClients = Effect.fnUntraced(function* (file: string) { | ||
| if (!Instance.containsPath(file)) return [] as LSPClient.Info[] | ||
| const s = yield* InstanceState.get(cache) |
There was a problem hiding this comment.
We usually call this state (not cache)
Same pattern as the message/part projectors — catch and log FK constraint errors instead of throwing. Fixes flaky CI where test ordering causes Session.create to fire before the project row exists.
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| log.error(`Failed to initialize LSP client ${server.id}`, { error: err }) | ||
| return undefined | ||
| }) | ||
|
|
There was a problem hiding this comment.
Style suggestion: The old code used run and runAll as helper names. Consider keeping these shorter names instead of runForFile and runForAll. Per style guide: "Use single word names by default for new locals, params, and helper functions."
| expect(Array.isArray(result)).toBe(true) | ||
| expect(result.length).toBe(0) | ||
| spawnSpy.mockRestore() | ||
| }), |
There was a problem hiding this comment.
Minor: Consider moving spawnSpy.mockRestore() to an afterEach hook instead of repeating it in each test. The beforeEach already handles setup, and cleanup in afterEach would reduce repetition and ensure cleanup even if tests fail.
|
Code review complete. Overall the Effect migration looks solid and follows the documented patterns well. I left two minor style suggestions on the code:
These are just suggestions - the code is otherwise clean and the migration approach is correct. The try/catch in projectors.ts is appropriate for handling the foreign key constraint error. |
| try { | ||
| db.insert(SessionTable).values(Session.toRow(data.info)).run() | ||
| } catch (err) { | ||
| if (!foreign(err)) throw err | ||
| log.warn("ignored session create — project missing", { sessionID: data.info.id, projectID: data.info.projectID }) | ||
| } |
There was a problem hiding this comment.
Hmm. This probably should not be part of this PR.
- Rename `cache` → `state` for InstanceState (convention) - Rename `runForFile`/`runForAll` → `run`/`runAll` (shorter helpers) - Move `spawnSpy.mockRestore()` to `afterEach` in lifecycle tests - Revert unrelated projectors.ts try/catch change
1 participant
Summary
Migrate LSP from
Instance.state()to EffectService/Layer/InstanceStatepattern withmakeRunPromiseand async facades.InstanceStatefor per-directory lifecycle withScopedCacheEffect.fntracing on all 14 service methodsEffect.fnUntracedfor internal helpers (getClients,runForFile,runForAll,callHierarchyRequest)addFinalizerfor client shutdown on instance disposalcallHierarchyRequesthelper (incoming/outgoing calls were near-identical)server.ts,client.ts,launch.ts, orlanguage.tsTest plan