diff --git a/PR_REVIEW.md b/PR_REVIEW.md new file mode 100644 index 0000000..c7b32b0 --- /dev/null +++ b/PR_REVIEW.md @@ -0,0 +1,59 @@ +# PR #283 Review: Use property descriptions for completion items + +## Summary + +This PR fixes a bug where completion items for action.yml were missing descriptions. The root cause was that `mappingValues()` only looked at the type definition's description, ignoring property-level descriptions in the schema. + +## Changes Analysis + +### Core Fix ([definition.ts](languageservice/src/value-providers/definition.ts)) + +**Before:** +```typescript +let description: string | undefined; +if (value.type) { + const typeDef = definitions[value.type]; + description = typeDef?.description; +} +``` + +**After:** +```typescript +let description: string | undefined = value.description; +if (value.type) { + const typeDef = definitions[value.type]; + if (!description) { + description = typeDef?.description; + } +} +``` + +✅ **Correct approach** - prioritizes property description, falls back to type description. + +### Test Coverage + +1. **complete-action.test.ts**: Two new tests verify `author` and `branding` completions include documentation +2. **hover-action.test.ts**: New test for `author` hover + updated `branding` test to verify "Documentation" link + +## Potential Issues + +### 1. One-of expansion doesn't use property description + +Looking at line 140-142: +```typescript +const expanded = expandOneOfToCompletions(oneOfDef, definitions, key, description, indentation, mode); +``` + +This passes `description` to `expandOneOfToCompletions`, but at this point `description` may have been populated from the property. **This is correct** - the property description is passed through. + +### 2. Consistency check + +The PR description mentions this is consistent with hover. Verified: [template-reader.ts#L225](workflow-parser/src/templates/template-reader.ts#L225) shows hover uses `nextPropertyDef.description` when available. + +## Verdict + +✅ **LGTM** - Clean, minimal fix that aligns completion behavior with hover. Good test coverage for the specific cases mentioned. + +## Minor Suggestions (non-blocking) + +1. Could add a test for a property that has NO description but whose type DOES have one, to verify fallback works (e.g., `inputs` which references `inputs-strict` type that has a description) diff --git a/languageservice/src/complete-action.test.ts b/languageservice/src/complete-action.test.ts index dd6e069..8f7cca9 100644 --- a/languageservice/src/complete-action.test.ts +++ b/languageservice/src/complete-action.test.ts @@ -251,6 +251,38 @@ runs: expect(labels).not.toContain("jobs"); }); + it("includes descriptions from schema for completion items", async () => { + const [doc, position] = createActionDocument(`|`, "file:///my-repo/action.yml"); + const completions = await complete(doc, position); + + const authorCompletion = completions.find(c => c.label === "author"); + expect(authorCompletion).toBeDefined(); + expect(authorCompletion?.documentation).toBeDefined(); + expect((authorCompletion?.documentation as {value: string})?.value).toContain("author"); + }); + + it("includes descriptions for branding completion", async () => { + const [doc, position] = createActionDocument(`|`, "file:///my-repo/action.yml"); + const completions = await complete(doc, position); + + const brandingCompletion = completions.find(c => c.label === "branding"); + expect(brandingCompletion).toBeDefined(); + expect(brandingCompletion?.documentation).toBeDefined(); + expect((brandingCompletion?.documentation as {value: string})?.value).toContain("branding"); + }); + + it("falls back to type description when property has no description", async () => { + // `inputs` uses shorthand form in schema: "inputs": "inputs-strict" + // So the property has no description, but the type `inputs-strict` does + const [doc, position] = createActionDocument(`|`, "file:///my-repo/action.yml"); + const completions = await complete(doc, position); + + const inputsCompletion = completions.find(c => c.label === "inputs"); + expect(inputsCompletion).toBeDefined(); + expect(inputsCompletion?.documentation).toBeDefined(); + expect((inputsCompletion?.documentation as {value: string})?.value).toContain("Input parameters"); + }); + it("does not route workflow files to action completion", async () => { const doc = TextDocument.create("file:///repo/.github/workflows/ci.yml", "yaml", 1, `o`); const completions = await complete(doc, {line: 0, character: 1}); diff --git a/languageservice/src/hover-action.test.ts b/languageservice/src/hover-action.test.ts index 387f560..13293f3 100644 --- a/languageservice/src/hover-action.test.ts +++ b/languageservice/src/hover-action.test.ts @@ -53,6 +53,20 @@ ru|ns: expect(result).not.toBeNull(); expect(result?.contents).toContain("runs"); }); + + it("shows description for author key", async () => { + const [doc, position] = createActionDocument(`name: My Action +description: Test +au|thor: Me +runs: + using: node20 + main: index.js`); + const result = await hover(doc, position); + + expect(result).not.toBeNull(); + expect(result?.contents).toContain("author"); + expect(result?.contents).toContain("Documentation"); + }); }); describe("runs properties", () => { @@ -145,6 +159,7 @@ brand|ing: expect(result).not.toBeNull(); expect(result?.contents).toContain("brand"); + expect(result?.contents).toContain("Documentation"); }); it("shows description for icon key", async () => { diff --git a/languageservice/src/value-providers/definition.ts b/languageservice/src/value-providers/definition.ts index 3939dd1..be1d81d 100644 --- a/languageservice/src/value-providers/definition.ts +++ b/languageservice/src/value-providers/definition.ts @@ -107,10 +107,14 @@ function mappingValues( for (const [key, value] of Object.entries(mappingDefinition.properties)) { let insertText: string | undefined; - let description: string | undefined; + // Prefer the property's own description (from the schema's property definition), + // fall back to the type definition's description if the property doesn't have one + let description: string | undefined = value.description; if (value.type) { const typeDef = definitions[value.type]; - description = typeDef?.description; + if (!description) { + description = typeDef?.description; + } if (typeDef) { switch (typeDef.definitionType) {