languageservices/PR_REVIEW.md

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)

Before:

let description: string | undefined;
if (value.type) {
  const typeDef = definitions[value.type];
  description = typeDef?.description;
}

After:

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:

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 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)