TML-2934: surface PSL symbol-table diagnostics in the language server#862
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtends the language server with optional control-stack resolution, a unified parse-plus-symbol-table diagnostics pipeline, per-document AST caching with a per-project symbol table, and server accessors for cached artifacts. All artifacts are initialized, updated, and cleaned up across document lifecycle events. ChangesPSL Diagnostics Pipeline and Artifact Caching
Sequence Diagram(s)sequenceDiagram
participant Client as LSP Client
participant Server as server.ts
participant ConfigRes as resolveConfigInputs
participant Artifacts as ProjectArtifacts
participant Pipeline as runPipeline
participant Parse as parse / buildSymbolTable
Client->>Server: didOpen/didChange uri, text
Server->>ConfigRes: loadProject configPath
ConfigRes-->>Server: inputs, controlStack
Server->>Artifacts: update uri, text, inputs, controlStack
Artifacts->>Pipeline: runPipeline text, controlStack
Pipeline->>Parse: parse text
Parse-->>Pipeline: document, sourceFile, parseDiagnostics
Pipeline->>Parse: buildSymbolTable document, sourceFile, ...
Parse-->>Pipeline: symbolTable, symbolTableDiagnostics
Pipeline-->>Artifacts: merged diagnostics, artifacts
Artifacts-->>Server: diagnostics
Server-->>Client: publishDiagnostics
Client->>Server: didClose uri
Server->>Artifacts: remove uri
Artifacts-->>Server: artifact cache cleared
Server-->>Client: clear diagnostics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/1-framework/3-tooling/language-server/src/config-resolution.ts (1)
21-26: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider using
ifDefinedfor conditional object spread.The conditional return could use
ifDefinedfrom@prisma-next/utils/definedfor consistency with codebase patterns:export async function resolveConfigInputs(configPath: string): Promise<ConfigResolution> { const config = await loadConfig(configPath); const inputs = resolveSchemaInputs(config); const controlStack = resolveControlStackInputs(config); - return controlStack === undefined ? { inputs } : { inputs, controlStack }; + return { inputs, ...ifDefined('controlStack', controlStack) }; }Based on learnings: prefer
ifDefinedfrom@prisma-next/utils/definedfor conditional object spreads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/language-server/src/config-resolution.ts` around lines 21 - 26, The resolveConfigInputs function uses a ternary operator for conditional return statements instead of the ifDefined utility. Import ifDefined from `@prisma-next/utils/defined` and refactor the return statement to use object spread syntax with ifDefined helper for the controlStack property, replacing the current ternary conditional pattern to maintain consistency with codebase conventions.Source: Learnings
packages/1-framework/3-tooling/language-server/src/server.ts (1)
132-135: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider using
ifDefinedfor conditional object spread.The conditional
ProjectStateconstruction could useifDefinedfrom@prisma-next/utils/definedfor consistency:async function loadProject(configPath: string): Promise<ProjectState> { const resolution = await resolveConfigInputs(configPath); - const project: ProjectState = - resolution.controlStack === undefined - ? { configPath, inputs: resolution.inputs } - : { configPath, inputs: resolution.inputs, controlStack: resolution.controlStack }; + const project: ProjectState = { + configPath, + inputs: resolution.inputs, + ...ifDefined('controlStack', resolution.controlStack), + }; projects.set(configPath, project); return project; }Based on learnings: prefer
ifDefinedfrom@prisma-next/utils/definedfor conditional object spreads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/language-server/src/server.ts` around lines 132 - 135, The ProjectState object construction uses a ternary operator to conditionally include the controlStack property. Replace this conditional construction with an object spread pattern using ifDefined from `@prisma-next/utils/defined`. First ensure ifDefined is imported from `@prisma-next/utils/defined`, then refactor the ProjectState assignment to use object spread with ifDefined applied to resolution.controlStack for consistency with the codebase conventions. This maintains the same functionality while using the preferred utility function for conditional property inclusion.Source: Learnings
packages/1-framework/3-tooling/language-server/test/document-diagnostics.test.ts (1)
20-28: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider extracting shared test constants.
The
duplicateModelSourceconstant is duplicated inserver.test.ts(lines 384-392). Similarly,scalarTypesappears in bothpipeline.test.tsandserver.test.ts. While duplication in test files is acceptable, extracting these to a shared test helper (e.g.,test/fixtures.ts) would improve maintainability if additional tests need the same data.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/language-server/test/document-diagnostics.test.ts` around lines 20 - 28, Extract the duplicateModelSource constant and scalarTypes constant from the test files (document-diagnostics.test.ts, server.test.ts, and pipeline.test.ts) into a shared test helper file such as test/fixtures.ts. Create the fixtures file if it doesn't exist, move these shared constants into it, and then update all test files that use these constants to import them from the new fixtures file instead of defining them locally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/1-framework/3-tooling/language-server/src/config-resolution.ts`:
- Around line 21-26: The resolveConfigInputs function uses a ternary operator
for conditional return statements instead of the ifDefined utility. Import
ifDefined from `@prisma-next/utils/defined` and refactor the return statement to
use object spread syntax with ifDefined helper for the controlStack property,
replacing the current ternary conditional pattern to maintain consistency with
codebase conventions.
In `@packages/1-framework/3-tooling/language-server/src/server.ts`:
- Around line 132-135: The ProjectState object construction uses a ternary
operator to conditionally include the controlStack property. Replace this
conditional construction with an object spread pattern using ifDefined from
`@prisma-next/utils/defined`. First ensure ifDefined is imported from
`@prisma-next/utils/defined`, then refactor the ProjectState assignment to use
object spread with ifDefined applied to resolution.controlStack for consistency
with the codebase conventions. This maintains the same functionality while using
the preferred utility function for conditional property inclusion.
In
`@packages/1-framework/3-tooling/language-server/test/document-diagnostics.test.ts`:
- Around line 20-28: Extract the duplicateModelSource constant and scalarTypes
constant from the test files (document-diagnostics.test.ts, server.test.ts, and
pipeline.test.ts) into a shared test helper file such as test/fixtures.ts.
Create the fixtures file if it doesn't exist, move these shared constants into
it, and then update all test files that use these constants to import them from
the new fixtures file instead of defining them locally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8922d94b-ff62-4d35-9962-0be871987485
⛔ Files ignored due to path filters (7)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/lsp-diagnostic-extend/plan.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/manual-qa.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/plan.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/qa-run-report.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/spec.mdis excluded by!projects/**projects/lsp-diagnostic-extend/spec.mdis excluded by!projects/**
📒 Files selected for processing (10)
packages/1-framework/3-tooling/language-server/package.jsonpackages/1-framework/3-tooling/language-server/src/config-resolution.tspackages/1-framework/3-tooling/language-server/src/document-diagnostics.tspackages/1-framework/3-tooling/language-server/src/pipeline.tspackages/1-framework/3-tooling/language-server/src/schema-inputs.tspackages/1-framework/3-tooling/language-server/src/server.tspackages/1-framework/3-tooling/language-server/test/config-resolution.test.tspackages/1-framework/3-tooling/language-server/test/document-diagnostics.test.tspackages/1-framework/3-tooling/language-server/test/pipeline.test.tspackages/1-framework/3-tooling/language-server/test/server.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/language-server/src/server.ts`:
- Around line 71-82: The publish function is vulnerable to race conditions
because it performs an asynchronous operation with await
resolveProjectForDocument(uri) but callers invoke it without waiting, allowing
stale publishes to overwrite cached artifacts or recreate them after documents
are closed. Add a guard condition after resolving the project that verifies the
document state hasn't changed (such as checking if the document is still open or
hasn't been replaced by a newer version) before proceeding with the
artifactsForConfig mutation. This prevents the update call and subsequent
mutations from executing on stale data.
- Around line 262-268: In the documents.onDidClose handler, the code removes
cached artifacts using projectArtifacts.get(configPath)?.remove(uri) but fails
to remove the URI entry from the documentConfigPaths map. This causes Line 98 to
trust stale config path mappings when documents are reopened after config
changes. Add a call to documentConfigPaths.delete(uri) after removing the
artifacts to ensure the URI-to-config mapping is cleared, forcing rediscovery of
the correct config path when the document is reopened.
In `@packages/1-framework/3-tooling/language-server/test/server.test.ts`:
- Around line 1072-1074: Replace the setTimeout-based synchronization approach
in the test with an event-based barrier by waiting for the expected second
diagnostics publish event instead of using await new Promise((resolve) =>
setTimeout(resolve, 0)). This ensures the test synchronization is deterministic
and tied to actual application behavior rather than arbitrary timing,
eliminating the flakiness caused by variable event loop scheduling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d740e34e-bcd3-49ec-9ec1-943f1e674829
⛔ Files ignored due to path filters (1)
projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/spec.mdis excluded by!projects/**
📒 Files selected for processing (9)
packages/1-framework/3-tooling/language-server/README.mdpackages/1-framework/3-tooling/language-server/src/document-diagnostics.tspackages/1-framework/3-tooling/language-server/src/pipeline.tspackages/1-framework/3-tooling/language-server/src/project-artifacts.tspackages/1-framework/3-tooling/language-server/src/server.tspackages/1-framework/3-tooling/language-server/test/document-diagnostics.test.tspackages/1-framework/3-tooling/language-server/test/pipeline.test.tspackages/1-framework/3-tooling/language-server/test/project-artifacts.test.tspackages/1-framework/3-tooling/language-server/test/server.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/1-framework/3-tooling/language-server/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/1-framework/3-tooling/language-server/test/pipeline.test.ts
- packages/1-framework/3-tooling/language-server/src/pipeline.ts
- packages/1-framework/3-tooling/language-server/test/document-diagnostics.test.ts
SevInf
commented
Jun 23, 2026
|
|
||
| export const CONFIG_FILENAME = 'prisma-next.config.ts'; | ||
|
|
||
| type LoadedConfig = Awaited<ReturnType<typeof loadConfig>>; |
Contributor
Author
There was a problem hiding this comment.
Don't we have specific type for that?
Rebase the language-server diagnostic work onto current main, preserving main formatter support while requiring the project control stack for PSL diagnostics. The server now publishes parse plus symbol-table diagnostics, preserves per-URI ASTs and one project symbol table, and keeps the project artifacts on project state. Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
SevInf
force-pushed
the
lsp-diagnostic-extend
branch
from
5681b65 to
fbbb2df
Compare
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/extension-supabase
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/config-loader
@prisma-next/emitter
@prisma-next/language-server
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
size-limit report 📦
|
Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/1-framework/3-tooling/language-server/test/document-diagnostics.test.ts (1)
49-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the repeated non-configured-input null test.
Line 49-53 and Line 95-100 assert the same contract (
computeDocumentDiagnostics(...)returnsnullfor an unconfigured URI). Keeping one case (or changing one to a distinct edge case) will reduce noise and improve failure clarity.♻️ Suggested simplification
- it('returns null for a document that is not a configured input', () => { - const otherUri = pathToFileURL('/abs/not-a-schema.psl').toString(); - expect( - computeDocumentDiagnostics(otherUri, duplicateModelSource, inputs, controlStack), - ).toBeNull(); - });As per coding guidelines,
**/*.{test,spec}.{ts,tsx,js,jsx}: Keep tests concise; omit 'should':.agents/rules/omit-should-in-tests.mdc.Also applies to: 95-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/language-server/test/document-diagnostics.test.ts` around lines 49 - 53, Remove the duplicate test case for the non-configured-input scenario. The test at lines 49-53 and the test at lines 95-100 both verify that computeDocumentDiagnostics returns null for an unconfigured URI, which is redundant. Keep one of these test cases and either delete the other or replace it with a distinct edge case test to improve test clarity and reduce noise in the test suite.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/1-framework/3-tooling/language-server/test/document-diagnostics.test.ts`:
- Around line 49-53: Remove the duplicate test case for the non-configured-input
scenario. The test at lines 49-53 and the test at lines 95-100 both verify that
computeDocumentDiagnostics returns null for an unconfigured URI, which is
redundant. Keep one of these test cases and either delete the other or replace
it with a distinct edge case test to improve test clarity and reduce noise in
the test suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6830e109-f834-4780-8885-7eda4b08dbad
⛔ Files ignored due to path filters (7)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/lsp-diagnostic-extend/plan.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/manual-qa.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/plan.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/qa-run-report.mdis excluded by!projects/**projects/lsp-diagnostic-extend/slices/symbol-table-diagnostics/spec.mdis excluded by!projects/**projects/lsp-diagnostic-extend/spec.mdis excluded by!projects/**
📒 Files selected for processing (14)
packages/1-framework/3-tooling/config-loader/src/exports/index.tspackages/1-framework/3-tooling/language-server/README.mdpackages/1-framework/3-tooling/language-server/package.jsonpackages/1-framework/3-tooling/language-server/src/config-resolution.tspackages/1-framework/3-tooling/language-server/src/document-diagnostics.tspackages/1-framework/3-tooling/language-server/src/pipeline.tspackages/1-framework/3-tooling/language-server/src/project-artifacts.tspackages/1-framework/3-tooling/language-server/src/schema-inputs.tspackages/1-framework/3-tooling/language-server/src/server.tspackages/1-framework/3-tooling/language-server/test/config-resolution.test.tspackages/1-framework/3-tooling/language-server/test/document-diagnostics.test.tspackages/1-framework/3-tooling/language-server/test/pipeline.test.tspackages/1-framework/3-tooling/language-server/test/project-artifacts.test.tspackages/1-framework/3-tooling/language-server/test/server.test.ts
✅ Files skipped from review due to trivial changes (2)
- packages/1-framework/3-tooling/config-loader/src/exports/index.ts
- packages/1-framework/3-tooling/language-server/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/1-framework/3-tooling/language-server/src/pipeline.ts
- packages/1-framework/3-tooling/language-server/src/schema-inputs.ts
- packages/1-framework/3-tooling/language-server/test/pipeline.test.ts
- packages/1-framework/3-tooling/language-server/README.md
- packages/1-framework/3-tooling/language-server/src/project-artifacts.ts
- packages/1-framework/3-tooling/language-server/src/server.ts
Signed-off-by: Serhii Tatarintsev <tatarintsev@prisma.io>
wmadden-electric
added a commit
that referenced
this pull request
Jun 24, 2026
Main moved 2 commits ahead (language-server PSL symbol-table diagnostics #862, a zsh :agent-script fix #842). Neither touches the slice's postgres target/adapter source. Clean auto-merge, no conflicts, no src/ conflicts. Lockfile matches main; pnpm install reconciles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
This was referenced Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends
@prisma-next/language-serverso configured PSL documents publish both parse diagnostics and symbol-table diagnostics from the in-memory editor buffer, while preserving parser outputs for future LSP features.Changes
runPipelinepath that parses the current document text, builds a symbol table with config-derived PSL inputs, and publishes parse diagnostics before symbol-table diagnostics.prisma-next.config.ts; configured PSL projects fail fast on invalid stack creation instead of silently falling back to parse-only diagnostics.PrismaNextConfigfrom@prisma-next/config-loaderinstead of inferring the loaded config type fromloadConfig.Why
The language server needs intermediate parser and symbol-table stages for diagnostics now and for richer editor features later. Running those stages directly against the open document keeps diagnostics accurate for unsaved edits, avoids interpretation work that is out of scope for this PR, and lets the server preserve the AST/symbol table shape it will need for future LSP capabilities.
Validation
pnpm --filter @prisma-next/language-server typecheckpnpm --filter @prisma-next/language-server test— 74 testspnpm --filter @prisma-next/language-server lintpnpm lint:depsCloses TML-2934.