feat: LSP hover with resolved CFN properties and template locations - #1669
Conversation
…te links Hovering a construct's creation line surfaces its synthesized CloudFormation property values, with each value linking to its location in the template. Adds the cfnProperties attribute to the construct tree, resolveResourceRanges for per-property template ranges, a pure hover content builder, and the onHover server wiring.
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
First-letter-upper only capitalized the first character, so properties whose CloudFormation name has an uppercase acronym run (SSESpecification, EnableECSManagedTags, TemplateURL) did not link and rendered as plain text. The L1 camelCase name and the template PascalCase name differ only in letter case, so key both sides lower-cased. Removes pascalCase.
6f74962 to
853b11b
Compare
ShadowCat567
left a comment
There was a problem hiding this comment.
Looks good overall, have some comments about variable names and UI details
| * in which case values render without links. | ||
| */ | ||
| export interface HoverLinks { | ||
| readonly blocks: Record<string, LinkTarget>; |
There was a problem hiding this comment.
What does blocks refer to in this context? Can it get a more descriptive name?
There was a problem hiding this comment.
blocks is the name that was used for the chunk of the template associated with a resource from the template range resolver. Renamed to resourceLocations which better describes what these are, a map from construct path to the location of that resource's definition in the synthesized template. Pushed.
| } | ||
|
|
||
| /** Group auxiliary resources by short CFN type, most common first, e.g. "8× Subnet". */ | ||
| function histogram(nodes: readonly ResourceConstruct[]): string { |
There was a problem hiding this comment.
What does this look like in the hover UI?
There was a problem hiding this comment.
Do you have a test for that? (default same depth as the other constructs)
| if (nodes.length === 0) { | ||
| return undefined; | ||
| } | ||
| const value = selection === undefined |
There was a problem hiding this comment.
Can selection instead be called defaultChild? Since it seems like the logic here revolves around whether the construct we are hovering over has a default child or not
There was a problem hiding this comment.
It's not TOTALLY the default child, selectPrimary picks the uniquely shallowest resource on the line, which is usually the default child but could be any uniquely-shallow resource. Renamed to primaryResource, which feels more descriptive than selection without implying the default child relationship specifically. Looks ok?
| const properties = primary.cfnProperties ?? {}; | ||
| const keys = Object.keys(properties); | ||
| const lines = keys.slice(0, MAX_PROPERTIES).map((key) => { | ||
| const value = `\`${renderValue(properties[key])}\``; |
There was a problem hiding this comment.
For nested properties like S3Bucket and S3Key in Code for Lambda functions, any particular reason why you render it like:
code: { s3bucket, s3key }
instead of:
code: {
s3bucket: <bucket name>
s3key: <key zip file>
}
There was a problem hiding this comment.
mostly just to save hover space, nested CFN values are usually intrinsics or deeply nested objects that make the tooltip very long. Showing key names felt like a good middle ground, and the property value is clickable, so it links to the exact line in the template where the full expansion is visible. happy to expand shallow objects if feels too terse, lmk.
There was a problem hiding this comment.
If we have the link to get the information, then this should be fine
…imary selection for VPC - Rename `blocks` → `resourceLocations` for clarity - Rename `selection` → `primaryResource` to avoid confusion with defaultChild - Fix selectPrimary to recognize the default child as primary when peers exist at the same depth (e.g. VPC + IGW + VPCGW all at depth 4)
|
|
||
| /** | ||
| * Builds the hover for the resource(s) created on a line, given the primary | ||
| * primaryResource. When one resource is the construct's primary (default) child, its |
There was a problem hiding this comment.
nit: this comment is confusing if the primary resource is not actually the default child (default child of construct), can you change the wording of this comment and comments like this to describe what you mean by primary child and probably remove the reference to default child if primary child != default child
| const properties = primary.cfnProperties ?? {}; | ||
| const keys = Object.keys(properties); | ||
| const lines = keys.slice(0, MAX_PROPERTIES).map((key) => { | ||
| const value = `\`${renderValue(properties[key])}\``; |
There was a problem hiding this comment.
If we have the link to get the information, then this should be fine
…ry vs default-child comments
…#1711) startServer now requires toolkitBindingsFactory (added in #1634/#1669), but the cdk lsp command still called it with only { readable, writable }, breaking the build. Expose startLspServer() from cdk-explorer (wires the Toolkit bindings and starts the server) and call it from the CLI. main.ts becomes that exported function, making cdk lsp the single LSP entrypoint. Fixes # ### Checklist - [ ] This change contains a major version upgrade for a dependency and I confirm all breaking changes are addressed - Release notes for the new version: --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

template-ranges.tswithresolveResourceRanges/indexTemplateRangesto resolve per-property character ranges in a single parsecfnPropertiesfrom the construct tree builder so the hover has access to resolved property valuesexample hover:

Fixes #
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license