Skip to content

Conversation

nighca
Copy link
Collaborator

@nighca nighca commented Sep 16, 2025

close #2043.

Copy link

niupilot bot commented Sep 16, 2025

✅ PR Review Complete

I've thoroughly reviewed PR #2212 and the changes look excellent! This is a well-executed fix that addresses range boundary inconsistencies causing hover behavior issues.

Todo List:

  • Gather context and understand the changes
  • Read the modified file and analyze the diff
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Evaluate suggestions for improvements
  • Assess architecture and design decisions
  • Verify documentation consistency
  • Provide comprehensive review feedback

Key Findings:

🔧 Critical Bug Fix: The containsPosition() boundary condition fix (>>=) correctly implements exclusive end position semantics, which should resolve the hover inconsistencies from issue #2043.

📚 Documentation Enhancement: Added clear comments to the Range type that align with LSP standards (start inclusive, end exclusive).

🎯 Logic Improvement: The rangeContains() refactor removes dependency on the previously buggy containsPosition function and implements more precise boundary logic.

Impact Assessment:

These functions are critical for hover behavior in the code editor, used extensively for:

  • Diagnostic positioning
  • Resource reference detection
  • Input helper triggering
  • Hover state management

The boundary fix should eliminate the random hover inconsistencies reported in the original issue.

Code Quality: ⭐⭐⭐⭐⭐

  • Minimal, focused changes
  • Self-documenting with clear comments
  • Maintains backward compatibility
  • Follows TypeScript best practices

Recommendation: ✅ Ready to merge - These changes are solid and should fix the hover behavior issues.


👉 Want me to re-run this review? Just type /review in the comments! For more usage examples, visit CodeAgent GitHub Repository.

Copy link

qiniu-prow bot commented Sep 16, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

@niupilot niupilot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Code Review Summary

This PR addresses range boundary condition inconsistencies that were causing issues with hover behavior. The changes are well-focused and technically sound.

Key Improvements:

  1. 🔧 Fixed Critical Boundary Bug: The containsPosition() function now correctly handles end position boundaries by using >= instead of >, ensuring end positions are properly excluded (as documented)

  2. 📚 Added Clear Documentation: The Range type now has explicit comments clarifying that start is inclusive and end is exclusive

  3. 🎯 Enhanced Range Logic: The rangeContains() function now uses more precise boundary logic instead of relying on the previously buggy containsPosition calls

Impact Analysis:

These functions are heavily used in the hover system (/hover/index.ts) for:

  • Diagnostic range checking (line 126)
  • Resource reference detection (line 154)
  • Input helper positioning (line 182)
  • Hover state management (line 224)

The boundary fix should resolve the "inconsistent hover card actions" described in issue #2043.

Code Quality: ⭐⭐⭐⭐⭐

  • Clean, minimal changes
  • Self-documenting with added comments
  • Consistent with TypeScript/LSP conventions
  • Maintains backward compatibility

column: number
}

export type Range = {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style improvement: Use consistent parenthetical notation for better readability and to match common documentation patterns.

Suggested change
export type Range = {
export type Range = {
/** The range's start position (inclusive) */
start: Position
/** The range's end position (exclusive) */
end: Position
}

if (position.line < range.start.line || position.line > range.end.line) return false
if (position.line === range.start.line && position.column < range.start.column) return false
if (position.line === range.end.line && position.column > range.end.column) return false
if (position.line === range.end.line && position.column >= range.end.column) return false
Copy link
Collaborator Author

@nighca nighca Sep 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们明确 range end 是 exclusive 的,与 range end 相同的 position 视作不被 range 包含

对应地,一个 identifier,其 range end position 所触发的 hover,我们处理为“不触发该 identifer 对应的 hover 内容”;这里的调整使得 code-editor-ui 侧的行为(见 code-editor/ui/hover/index.ts)遵循该设定

不过目前 LS 侧行为跟该设定不一致

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent hover card actions across multiple hover attempts
1 participant