🤖 AI Review

Summary

The pull request introduces a new feature to the Angular language service, specifically adding functionality for smart selection range expansion in templates. The implementation appears well-structured and follows modern coding practices. The code is generally clean, with clear separation of concerns and appropriate use of TypeScript features. However, there are some areas for improvement, particularly in documentation, error handling, and testing coverage.

Findings

FILE 1: packages/language-service/api.ts

• Finding: No issues found.

FILE 2: packages/language-service/src/language_service.ts

• Finding: No issues found.

FILE 3: packages/language-service/src/selection_range.ts

• Finding: Lack of error handling for getTypeCheckInfoAtPosition.

  • Impact: If getTypeCheckInfoAtPosition fails or returns unexpected results, it could lead to runtime errors or undefined behavior.
  • Suggested Fix: Add error handling to manage unexpected results gracefully.

    const typeCheckInfo = getTypeCheckInfoAtPosition(fileName, position, compiler);
    if (!typeCheckInfo) {
        console.error(`Failed to get type check info for ${fileName} at position ${position}`);
        return undefined; // or throw an error based on your error handling strategy
    }

• Finding: The buildSelectionRangeChain function could benefit from a more explicit return type.

  • Impact: While TypeScript infers the return type, being explicit improves code readability and maintainability.
  • Suggested Fix: Specify the return type explicitly.

    function buildSelectionRangeChain(path: Array<TmplAstNode | AST>): ts.SelectionRange | undefined {
        // existing code...
    }

FILE 4: packages/language-service/src/ts_plugin.ts

• Finding: No issues found.

FILE 5: vscode-ng-language-service/server/src/handlers/initialization.ts

• Finding: No issues found.

FILE 6: vscode-ng-language-service/server/src/handlers/selection_range.ts

• Finding: Lack of input validation for params.positions.

  • Impact: If params.positions is malformed or contains invalid data, it could lead to runtime errors.
  • Suggested Fix: Validate params.positions before processing.

    if (!Array.isArray(params.positions) || params.positions.length === 0) {
        console.warn('Invalid positions array');
        return null;
    }

• Finding: No error handling for the conversion process in convertSelectionRange.

  • Impact: If the conversion fails, it could lead to silent failures where no selection range is returned.
  • Suggested Fix: Add error handling to log or manage conversion failures.

    const range = tsTextSpanToLspRange(scriptInfo, tsRange.textSpan);
    if (!range) {
        console.error('Failed to convert selection range', tsRange);
        return undefined;
    }

FILE 7: vscode-ng-language-service/server/src/session.ts

• Finding: No issues found.

Conclusion

Overall, the implementation of the selection range feature is solid, with a clear structure and adherence to TypeScript best practices. The suggestions provided focus on enhancing error handling, improving documentation, and ensuring input validation, which will contribute to the robustness and maintainability of the code.

— Posted via PR Review Agent

In the example, <span>{{user.name}}</span>, the second expand goes to {{user.name}} because that's the entirety of the element. If it was instead <span>{{user.name}} - {{user.phone}}</span>, the expand would go straight to {{user.name}} - {{user.phone}} rather than having one at {{user.name}}. Should the expansions include the binding expression boundaries?

In the example, <span>{{user.name}}</span>, the second expand goes to {{user.name}} because that's the entirety of the element. If it was instead <span>{{user.name}} - {{user.phone}}</span>, the expand would go straight to {{user.name}} - {{user.phone}} rather than having one at {{user.name}}. Should the expansions include the binding expression boundaries?

I checked how it works for html tags and You are right it should expand in this order:

user.name -> {{user.name}} -> {{user.name}} - {{user.phone}} -> <span>...</span> 

Added this behaviour and tests for it.

@atscott I will add more tests for complicated template patterns , should I convert to draft?

There's a significant amount of unrelated changes in here. (adev, forms). Can you please clean this up?

cleaned up PR, sorry for that

@kbrilla is this ready for review again?

@kbrilla is this ready for review again?

@atscott yes, it is now ready for review

The "feat(language-service): delegate css selection ranges in style bindings" commit took things too far. Please drop that from this PR. Please also clean up the commit history.

You are right, it should have been moved to #67266