feat(data-types): wire the .dt code view into the ST language server (DOPE-537) - #998
Conversation
Go-to-definition on a user type now lands in that type's own code view
with the cursor on the declaration — a struct field target lands on its
field line — instead of merely opening the form tab (DOPE-537).
The buffer gets a real identity (`inmemory://dtview/<name>.dt`) so the
LSP can recognise it, and `resolveStLspContext` remaps it onto the
aggregate datatypes document. Both frames open with a `TYPE` line, so a
single shift derived from the type's span covers completion, hover,
signature help, definition, references and formatting at once. The span
map is computed from the serializer rather than tracked in the offset
registry: there is no registration lifecycle to keep in sync and no
window where a datatype edit and the stored offset disagree.
Semantic tokens and diagnostics need more than a shift, because the
model's text and the document the answers come from are two different
strings that only agree while the buffer is committed:
- The token window holds the entry's own lines and is rebased onto the
view's frame via `outputStartLine`. Widening the window instead
would drag in the previous entry's last line, whose columns overrun
the 4-character `TYPE` line and make Monaco reject the whole batch.
- While the buffer diverges from the store the window is empty. No
colours beats colours describing the previous text.
- A store change re-drives both, since the model's text is untouched
by it and Monaco would otherwise never re-query.
- The diagnostics mirror caches each publish together with the spans
it was computed against, and replays it when a model mounts later.
Replaying through freshly computed spans puts markers on the wrong
line, or drops them, once the store has moved.
The go-to-definition cursor is deliberately keyed on its own identity
and not on the current display: including the display would re-fire the
forced switch when the user toggles back to the table and pin the tab in
code mode.
Refs DOPE-537
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GvGrhB1LyJz9MBwYhjoBDY
…37-datatype-lsp-goto-def
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds per-data-type code-view URIs, definition navigation, cursor positioning, Monaco model selection, semantic-token rebasing, diagnostic synchronization, and regression coverage. ChangesData-type code-view integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LSPClient
participant GotoDefinitionRedirect
participant DataTypeEditor
participant VariablesCodeEditor
participant STLSPAdapter
LSPClient->>GotoDefinitionRedirect: request data-type definition
GotoDefinitionRedirect->>DataTypeEditor: open code view and set cursor
DataTypeEditor->>VariablesCodeEditor: pass model URI and cursor position
VariablesCodeEditor->>STLSPAdapter: request mapped view data
STLSPAdapter-->>VariablesCodeEditor: return rebased tokens and diagnostics
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/frontend/services/st-lsp/index.ts`:
- Around line 149-162: Update applyDataTypeDiagnostics to clear markers for
deleted or locally modified .dt models before applying diagnostics: compare each
model against its corresponding store data serialized with
serializeDataTypeToText, clear markers when the type is absent or
model.getValue() differs, and only map current diagnostics for matching,
unchanged models. Add regression coverage for deleted types and uncommitted
local edits.
In `@src/frontend/services/st-lsp/types.ts`:
- Around line 157-162: Update parseDtViewUri to safely handle decodeURIComponent
failures for malformed percent-encoded paths, such as invalid sequences in the
captured URI segment. Return null when decoding fails, while preserving the
existing decoded data type name for valid dtview URIs.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 77d6b198-89b1-42e2-8532-be08d6221cb1
📒 Files selected for processing (15)
src/frontend/components/_features/[workspace]/data-type/__tests__/code-view-toggle.test.tsxsrc/frontend/components/_features/[workspace]/data-type/index.tsxsrc/frontend/components/_organisms/variables-code-editor/index.tsxsrc/frontend/services/lsp-shared/__tests__/semantic-tokens-shift.test.tssrc/frontend/services/lsp-shared/internal/semantic-tokens-shift.tssrc/frontend/services/lsp-shared/semantic-tokens.tssrc/frontend/services/lsp-shared/start-language-service.tssrc/frontend/services/st-lsp/__tests__/goto-definition-redirect.test.tssrc/frontend/services/st-lsp/__tests__/types.test.tssrc/frontend/services/st-lsp/goto-definition-redirect.tssrc/frontend/services/st-lsp/index.tssrc/frontend/services/st-lsp/types.tssrc/frontend/store/slices/editor/types.tssrc/frontend/utils/PLC/__tests__/data-type-serializer.test.tssrc/frontend/utils/PLC/data-type-serializer.ts
…parsers `parsePouUri`, `parsePouVarsUri` and `parseDtViewUri` decoded their name segment with a bare `decodeURIComponent`, which raises `URIError` on input like `%ZZ`. All three run inside `resolveStLspContext`, on every model URI the providers see, so one malformed URI would take hover, completion and definition down for that model rather than simply not matching. Not reachable today — these URIs are only minted by the matching builders, which encode the name — but the guard belongs in all three rather than in whichever one was touched last. Refs DOPE-537 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvGrhB1LyJz9MBwYhjoBDY
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/frontend/services/st-lsp/__tests__/types.test.ts (1)
85-87: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the encoded
dtViewUristring directly.The current round-trip assertion checks only the parser result. It would still pass if
dtViewUriemitted an unencoded space or slash. Add an exact serialization assertion for a name containing both characters.Proposed test assertion
it('round-trips a data type name, encoding included', () => { expect(dtViewUri('Motor')).toBe('inmemory://dtview/Motor.dt') + expect(dtViewUri('My Type/Variant')).toBe( + 'inmemory://dtview/My%20Type%2FVariant.dt', + ) expect(parseDtViewUri(dtViewUri('My Type'))).toBe('My Type') })🤖 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 `@src/frontend/services/st-lsp/__tests__/types.test.ts` around lines 85 - 87, Update the round-trip test around dtViewUri and parseDtViewUri to assert the exact encoded URI produced for a data type name containing both a space and a slash, while retaining the parser round-trip assertion.
🤖 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 `@src/frontend/services/st-lsp/__tests__/types.test.ts`:
- Around line 85-87: Update the round-trip test around dtViewUri and
parseDtViewUri to assert the exact encoded URI produced for a data type name
containing both a space and a slash, while retaining the parser round-trip
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1840ba2f-e4dd-48c4-b37a-d5213ddbd188
📒 Files selected for processing (2)
src/frontend/services/st-lsp/__tests__/types.test.tssrc/frontend/services/st-lsp/types.ts
marconetsf
left a comment
marconetsf
left a comment
There was a problem hiding this comment.
Review of the .dt LSP wiring. The design reads well and the manual-validation notes in the description are genuinely useful — the four semantic-token defects you describe finding and fixing are exactly the ones this shape invites. Six comments below, all on the seam between the two coordinate frames.
The one I'd treat as blocking is the fallback in resolveStLspContext; the rest are a mix of correctness, a feature-flag escape, and the test gap that would have caught them.
A `.dt` view whose name has no entry in the aggregate document fell back to `span?.start ?? 1`, which is the first entry's mapping — so hover, completion and go-to-definition answered for an unrelated type. An unparseable `.dt` file reaches this path with a live, visible buffer. Pass the view's own un-indexed URI through instead: strucpp guards every handler on the document being known, so each provider answers nothing. Move the frame arithmetic into `dtview-context.ts` so it can be tested without a worker, and drop the duplicated shift in the diagnostics fan-out. Gate the store subscription on `isDataTypeFilesEnabled()` and on a `.dt` model actually being mounted. `refreshSemanticTokens()` re-tokenises every model in the ST language, so with the flag off a datatype table edit was triggering a worker round trip per open ST editor. Convert the goto-definition cursor at the call site, next to the POU conversions, rather than half inside the routing helper. Reported by review on #657 / #998. The frame-line aliasing (DOPE-554) and the formatting end-clip (DOPE-555) are tracked separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvGrhB1LyJz9MBwYhjoBDY
Review round —
|
2 participants
Wires the per-type
.dtcode view (DOPE-535) into the ST language server, and upgrades go-to-definition to land inside it. Last implementation card of DOPE-385 — only the flag flip (DOPE-542) remains.Mirror of https://github.com/Autonomy-Logic/openplc-web/pull/657
What changed
A URI for the buffer.
dtViewUri(name)→inmemory://dtview/<name>.dt, alongside the existingpouvars://pair.VariablesCodeEditorgained amodelUriprop so a non-POU surface can route to the LSP;pouNamestill works unchanged for POUs.One offset, many providers. New
dataTypeLineSpans()in the serializer returns each type's{start, length}in the aggregate document.resolveStLspContextremapsdtview://ontoDATA_TYPES_URIwithspan.start - 1— both frames open with aTYPEline, so that single shift covers completion, hover, signature help, definition, references, document symbols and formatting.findDataTypeAtLinenow reuses the same helper instead of recomputing the cursor walk by hand.Spans are derived from the serializer on demand rather than registered in
body-offsets: no registration lifecycle to keep in sync, and no window where a datatype edit and a stored offset disagree.Go-to-definition.
routeToDataTypeCodeViewopens the tab, switches to code mode and places the cursor at the in-entry line, tagged with a new'data-type'cursor target so a POU jump can't bleed into a datatype tab. With the flag off it degrades to today's behaviour — form tab, no cursor.Semantic tokens and diagnostics needed more than a shift, because the model's text and the document the answers come from are two different strings that agree only while the buffer is committed. Four pieces, each earned during manual validation:
outputStartLineargument. Widening the window instead — which is what I did first — drags in the previous entry's last line, whose columns overrun the 4-characterTYPEline, and Monaco rejects the batch withend character > model.getLineLength.LanguageService.refreshSemanticTokens()(the registration already hadrefresh(); it just wasn't reachable). Without it, committing leaves the model's text untouched and Monaco never re-queries, so wrong colours persist until the view is remounted..dtmodel mounts later. Replaying through freshly recomputed spans lands markers on the wrong line — or drops them entirely — once the store has moved.Verification
Validated interactively against a running project, six passes: line alignment, diagnostics (right line, no bleed between types), hover/completion, rename while the view is open, delete while it's open, and POU-side regressions. Several defects were found and fixed in that loop, including the ones described above and a go-to-definition cursor that pinned the tab in code mode — the forced switch is now keyed on the cursor's identity, not the current display, matching the variables editor.
Automated:
tscclean in both repos,validate:archpasses, eslint 0 errors, prettier clean, mirror gatediffs: 0. Editor jest 6083 pass / 0 fail. Web vitest: 6087 pass with 78 failures across 9 files that are identical on a cleandevelopment(DOPE-549). 21 tests added — span arithmetic against the rendered document,dtViewUriround-trip and non-collision, datatype go-to-definition (declaration line, struct field, out-of-range, flag-off fallback), the token-window rebase including the exact regression it fixes, and a component test pinning the toggle-after-jump behaviour.Flag
Everything user-visible stays behind
isDataTypeFilesEnabled(), stillfalse. DOPE-542 owns the flip.Filed while validating
provideDefinition. Pre-existing, affects every redirect branch..dt. Pre-existing, confirmed by A/B.Summary by CodeRabbit