From cd0bec17d743f61e7195b93e9d8fe6695b56a560 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Fri, 6 Feb 2026 08:19:15 -0500 Subject: [PATCH 1/3] Handle LiquidDocParamNode in rename provider for cursor boundary fix When the cursor is at the first character of a LiquidDoc @param name, rename was not triggering because the node boundary check used the LiquidDocParamNode position (which includes the @ prefix) instead of the paramName position. Now we detect LiquidDocParamNode and use its paramName sub-node for boundary calculations. Closes #936 Co-Authored-By: Claude Opus 4.6 --- .changeset/fix-rename-boundary.md | 5 ++ .../LiquidVariableRenameProvider.spec.ts | 57 +++++++++++++++++++ .../providers/LiquidVariableRenameProvider.ts | 30 ++++++++-- 3 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 .changeset/fix-rename-boundary.md diff --git a/.changeset/fix-rename-boundary.md b/.changeset/fix-rename-boundary.md new file mode 100644 index 000000000..0608664b3 --- /dev/null +++ b/.changeset/fix-rename-boundary.md @@ -0,0 +1,5 @@ +--- +"@shopify/theme-language-server-common": patch +--- + +Fix rename not triggering when cursor is at the first character of a LiquidDoc param name diff --git a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts index 1ec05ff9c..750de9e39 100644 --- a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts +++ b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts @@ -156,6 +156,31 @@ describe('LiquidVariableRenameProvider', () => { ); }); + it('returns new name when cursor is at first character of liquid doc param name', async () => { + const params = { + textDocument, + position: Position.create(0, 25), + newName: 'meal', + }; + + const result = await provider.rename(params); + assert(result); + assert(result.documentChanges); + expect((result.documentChanges[0] as TextDocumentEdit).edits).to.applyEdits( + textDocument, + `{% doc %}@param {string} meal - favorite food{% enddoc %} +{% assign animal = 'dog' %} +{% assign plant = 'cactus' %} +{% liquid + echo plant + echo meal +%} +{% assign painting = 'mona lisa' %} +{% assign paintings = 'starry night, sunday afternoon, the scream' %} +

I have a cool animal, a great plant, and my favorite food

`, + ); + }); + it('returns new name after liquid doc param is renamed on variable usage', async () => { const params = { textDocument, @@ -326,6 +351,38 @@ describe('LiquidVariableRenameProvider', () => { ); }); + it('returns new name when cursor is at first character of untyped param name', async () => { + const params = { + textDocument, + position: Position.create(0, 16), + newName: 'ppp', + }; + const result = await provider.rename(params); + assert(result); + assert(result.documentChanges); + expect((result.documentChanges[0] as TextDocumentEdit).edits).to.applyEdits( + textDocument, + `{% doc %}@param ppp - product{% enddoc %} +{% liquid + for prod in products + echo prod.title + endfor + echo ppp +%}`, + ); + }); + + it('does not trigger rename when cursor is at @ symbol position', async () => { + const params = { + textDocument, + position: Position.create(0, 9), + newName: 'ppp', + }; + + const result = await provider.rename(params); + expect(result).to.be.null; + }); + it('returns new name after variable is renamed inside loop, but is scoped outside it', async () => { const params = { textDocument, diff --git a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts index e7182eeb2..f6cfbc92d 100644 --- a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts +++ b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts @@ -1,6 +1,7 @@ import { AssignMarkup, ForMarkup, + LiquidDocParamNode, LiquidHtmlNode, LiquidTagFor, LiquidTagTablerow, @@ -48,15 +49,18 @@ export class LiquidVariableRenameProvider implements BaseRenameProvider { if (!textDocument || !node || !ancestors) return null; if (!supportedTags(node, ancestors)) return null; + const targetNode = node.type === NodeTypes.LiquidDocParamNode ? node.paramName : node; const oldName = variableName(node); - const offsetOfVariableNameEnd = node.position.start + oldName.length; + const cursorOffset = textDocument.offsetAt(params.position); + const offsetOfVariableNameEnd = targetNode.position.start + oldName.length; - // The cursor could be past the end of the variable name - if (textDocument.offsetAt(params.position) > offsetOfVariableNameEnd) return null; + // The cursor must be within the variable name range + if (cursorOffset < targetNode.position.start || cursorOffset > offsetOfVariableNameEnd) + return null; return { range: Range.create( - textDocument.positionAt(node.position.start), + textDocument.positionAt(targetNode.position.start), textDocument.positionAt(offsetOfVariableNameEnd), ), placeholder: oldName, @@ -76,6 +80,13 @@ export class LiquidVariableRenameProvider implements BaseRenameProvider { if (document.ast instanceof Error) return null; if (!supportedTags(node, ancestors)) return null; + // When node is a LiquidDocParamNode, ensure cursor is on the param name, not the @ prefix + if (node.type === NodeTypes.LiquidDocParamNode) { + const cursorOffset = textDocument.offsetAt(params.position); + const nameEnd = node.paramName.position.start + node.paramName.value.length; + if (cursorOffset < node.paramName.position.start || cursorOffset > nameEnd) return null; + } + const oldName = variableName(node); const scope = variableNameBlockScope(oldName, ancestors); const replaceRange = textReplaceRange(oldName, textDocument, scope); @@ -117,12 +128,13 @@ export class LiquidVariableRenameProvider implements BaseRenameProvider { function supportedTags( node: LiquidHtmlNode, ancestors: LiquidHtmlNode[], -): node is AssignMarkup | LiquidVariableLookup | ForMarkup | TextNode { +): node is AssignMarkup | LiquidVariableLookup | ForMarkup | TextNode | LiquidDocParamNode { return ( node.type === NodeTypes.AssignMarkup || node.type === NodeTypes.VariableLookup || node.type === NodeTypes.ForMarkup || - isLiquidDocParamNameNode(node, ancestors) + isLiquidDocParamNameNode(node, ancestors) || + isLiquidDocParamNode(node) ); } @@ -140,6 +152,10 @@ function isLiquidDocParamNameNode( ); } +function isLiquidDocParamNode(node: LiquidHtmlNode): node is LiquidDocParamNode { + return node.type === NodeTypes.LiquidDocParamNode && node.paramName?.type === NodeTypes.TextNode; +} + function variableName(node: LiquidHtmlNode): string { switch (node.type) { case NodeTypes.VariableLookup: @@ -149,6 +165,8 @@ function variableName(node: LiquidHtmlNode): string { return node.variableName ?? ''; case NodeTypes.TextNode: return node.value; + case NodeTypes.LiquidDocParamNode: + return node.paramName.value; default: return ''; } From fc0625dca55b5679f438c375f4cadbb50aabbe4a Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Fri, 6 Feb 2026 08:36:20 -0500 Subject: [PATCH 2/3] Address PR review feedback for rename boundary fix Use indexOf for test positions instead of hardcoded numbers, clarify comment about @param keyword/type annotation exclusion, and expand negative tests to cover @param keyword and space before param name. Co-Authored-By: Claude Opus 4.6 --- .../LiquidVariableRenameProvider.spec.ts | 43 ++++++++++++++----- .../providers/LiquidVariableRenameProvider.ts | 2 +- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts index 750de9e39..e3fd7fb31 100644 --- a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts +++ b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts @@ -159,7 +159,7 @@ describe('LiquidVariableRenameProvider', () => { it('returns new name when cursor is at first character of liquid doc param name', async () => { const params = { textDocument, - position: Position.create(0, 25), + position: Position.create(0, documentSource.indexOf('food')), newName: 'meal', }; @@ -181,6 +181,17 @@ describe('LiquidVariableRenameProvider', () => { ); }); + it('does not trigger rename when cursor is on type annotation', async () => { + const params = { + textDocument, + position: Position.create(0, documentSource.indexOf('{string}') + 1), + newName: 'meal', + }; + + const result = await provider.rename(params); + expect(result).to.be.null; + }); + it('returns new name after liquid doc param is renamed on variable usage', async () => { const params = { textDocument, @@ -354,7 +365,7 @@ describe('LiquidVariableRenameProvider', () => { it('returns new name when cursor is at first character of untyped param name', async () => { const params = { textDocument, - position: Position.create(0, 16), + position: Position.create(0, documentSource.indexOf('prod')), newName: 'ppp', }; const result = await provider.rename(params); @@ -372,15 +383,27 @@ describe('LiquidVariableRenameProvider', () => { ); }); - it('does not trigger rename when cursor is at @ symbol position', async () => { - const params = { - textDocument, - position: Position.create(0, 9), - newName: 'ppp', - }; + [ + { desc: 'at @ symbol', position: Position.create(0, documentSource.indexOf('@param')) }, + { + desc: 'at @param keyword', + position: Position.create(0, documentSource.indexOf('@param') + 1), + }, + { + desc: 'at space before param name', + position: Position.create(0, documentSource.indexOf('@param') + '@param'.length), + }, + ].forEach(({ desc, position }) => { + it(`does not trigger rename when cursor is ${desc}`, async () => { + const params = { + textDocument, + position, + newName: 'ppp', + }; - const result = await provider.rename(params); - expect(result).to.be.null; + const result = await provider.rename(params); + expect(result).to.be.null; + }); }); it('returns new name after variable is renamed inside loop, but is scoped outside it', async () => { diff --git a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts index f6cfbc92d..dd91f9c23 100644 --- a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts +++ b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts @@ -80,7 +80,7 @@ export class LiquidVariableRenameProvider implements BaseRenameProvider { if (document.ast instanceof Error) return null; if (!supportedTags(node, ancestors)) return null; - // When node is a LiquidDocParamNode, ensure cursor is on the param name, not the @ prefix + // When node is a LiquidDocParamNode, ensure cursor is on the param name, not the @param keyword or type annotation if (node.type === NodeTypes.LiquidDocParamNode) { const cursorOffset = textDocument.offsetAt(params.position); const nameEnd = node.paramName.position.start + node.paramName.value.length; From b49a4d02e968aec3bf369e474550a8131f53ce93 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Fri, 6 Feb 2026 12:06:30 -0500 Subject: [PATCH 3/3] fix: scope rename to param name, not type annotation when they share the same text Co-Authored-By: Claude Opus 4.6 --- .../LiquidVariableRenameProvider.spec.ts | 26 +++++++++++++++++++ .../providers/LiquidVariableRenameProvider.ts | 4 ++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts index e3fd7fb31..3bc4aa60d 100644 --- a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts +++ b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.spec.ts @@ -156,6 +156,32 @@ describe('LiquidVariableRenameProvider', () => { ); }); + it('does not rename type annotation when type and param name share the same text', async () => { + const collisionSource = `{% doc %}@param {food} food - favorite food{% enddoc %} +{% liquid + echo food +%}`; + const collisionDoc = TextDocument.create(textDocumentUri, 'liquid', 1, collisionSource); + documentManager.open(collisionDoc.uri, collisionSource, 1); + + const params = { + textDocument: collisionDoc, + position: Position.create(0, collisionSource.indexOf('} food') + 2), + newName: 'meal', + }; + + const result = await provider.rename(params); + assert(result); + assert(result.documentChanges); + expect((result.documentChanges[0] as TextDocumentEdit).edits).to.applyEdits( + collisionDoc, + `{% doc %}@param {food} meal - favorite food{% enddoc %} +{% liquid + echo meal +%}`, + ); + }); + it('returns new name when cursor is at first character of liquid doc param name', async () => { const params = { textDocument, diff --git a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts index dd91f9c23..b901630ca 100644 --- a/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts +++ b/packages/theme-language-server-common/src/rename/providers/LiquidVariableRenameProvider.ts @@ -98,7 +98,9 @@ export class LiquidVariableRenameProvider implements BaseRenameProvider { AssignMarkup: replaceRange, ForMarkup: replaceRange, TextNode: (node: LiquidHtmlNode, ancestors: (LiquidHtmlNode | JSONNode)[]) => { - if (ancestors.at(-1)?.type !== NodeTypes.LiquidDocParamNode) return; + const parentNode = ancestors.at(-1); + if (parentNode?.type !== NodeTypes.LiquidDocParamNode || parentNode?.paramName !== node) + return; liquidDocParamUpdated = true;