Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Review now includes code selections #2128

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

dustinbyrne
Copy link
Contributor

Problem

JetBrains IDEs (IntelliJ, etc.) were sending pinned items as 'code selections' without location information, causing them to be ignored during the @review command. This resulted in incomplete context being considered during code reviews in JetBrains environments.

Solution

Modified the getPinnedItems function to include code selections that don't have location information. This maintains backward compatibility while ensuring that pinned items in JetBrains IDEs are properly included in the review context.

Changes

  • Enhanced pinned item handling to include code selections without locations
  • Added compatibility layer for JetBrains IDE pinned items
  • Added test coverage to verify the handling of non-located code selections
  • Maintained existing functionality for code selections with locations

Testing

  • Added new test case verifying code selections without locations are properly included
  • Verified existing functionality remains unchanged
  • Confirmed compatibility with both JetBrains IDEs and other supported editors

Review Summary

The changes have been reviewed and approved across multiple domains:

  • Compatibility: Successfully addresses JetBrains IDE integration while maintaining backward compatibility
  • Code Quality: Well-structured implementation with clear intent
  • Documentation: Clear commit message and code comments explaining the purpose
  • Testing: Comprehensive test coverage for the new functionality

These changes improve the integration with JetBrains IDEs without disrupting existing functionality in other environments.

@dustinbyrne dustinbyrne self-assigned this Nov 27, 2024
Comment on lines 214 to 218
pinnedItems.filter(UserContext.isCodeSelectionItem).forEach((cs) =>
pinnedItemLookup.push({
type: ContextV2.ContextItemType.CodeSelection,
content: cs.content,
})
);
Copy link
Collaborator

@dividedmind dividedmind Dec 2, 2024

Choose a reason for hiding this comment

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

Small nitpick — forEach is great for when you already have a named function to iterate with, but with a lambda like that for ... of is IMO much more readable (not to mention about 20× faster). Or do something like this:

Suggested change
pinnedItems.filter(UserContext.isCodeSelectionItem).forEach((cs) =>
pinnedItemLookup.push({
type: ContextV2.ContextItemType.CodeSelection,
content: cs.content,
})
);
pinnedItemLookup.push(
...pinnedItems
.filter(UserContext.isCodeSelectionItem)
.map((cs) => ({ ...cs, type: ContextV2.ContextItemType.CodeSelection }))
);

Copy link
Collaborator

Choose a reason for hiding this comment

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

(BTW, it's weird that we have to retype type like that even though the value is the same. ContextV2.ContextItemType should really have been a union type, not an enum.)

The JetBrains is still sending pinned items as 'code selections',
instead of located files. This means that in IntelliJ, pinned items
would not be considered during `@review`.
@dustinbyrne dustinbyrne force-pushed the fix/review-code-selections branch from 1a0c820 to 9dc6ee7 Compare December 6, 2024 20:10
@dustinbyrne dustinbyrne merged commit 8ea7137 into main Dec 17, 2024
23 checks passed
@dustinbyrne dustinbyrne deleted the fix/review-code-selections branch December 17, 2024 19:55
@appland-release
Copy link
Contributor

🎉 This PR is included in version @appland/navie-v1.38.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants