-
Notifications
You must be signed in to change notification settings - Fork 246
chore: add custom copilot instructions COMPASS-10013 #7628
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds custom Copilot instructions for code reviews and includes test changes to validate the reviewer's detection capabilities.
- Adds a new
.github/copilot-instructions.mdfile with guidelines for code reviews - Introduces intentional anti-pattern code in
diagram-editor.tsxto test Copilot's detection
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
.github/copilot-instructions.md |
New file defining custom instructions for Copilot code reviews, covering performance, code quality, best practices, and MongoDB-specific guidelines |
packages/compass-data-modeling/src/components/diagram-editor.tsx |
Adds unused import and intentional React anti-pattern (direct state mutation in useEffect) for testing purposes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/compass-data-modeling/src/components/diagram-editor.tsx
Outdated
Show resolved
Hide resolved
packages/compass-data-modeling/src/components/diagram-editor.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| obj.a = 'abc'; | ||
| }, [obj]); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code directly mutates the state object instead of using the setObj setter function. Direct state mutation bypasses React's state management and will not trigger re-renders. Replace obj.a = 'abc' with setObj({ a: 'abc' }). Additionally, the dependency array [obj] will cause an infinite loop since obj is recreated on every render. See React documentation on state updates.
| obj.a = 'abc'; | |
| }, [obj]); | |
| setObj({ a: 'abc' }); | |
| }, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const stats = await runCommand( | ||
| db, | ||
| { dbStats: 1 }, | ||
| { dbStats: 1, freeStorage: 1 }, |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The freeStorage: 1 option can have significant performance implications on large deployments as it requires scanning all storage metadata. Consider documenting this performance impact and evaluating if this metric is necessary for all use cases. See https://www.mongodb.com/docs/manual/reference/command/dbStats/#performance-considerations
nbbeeken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this only configure the review bot or will it also change the local copilot experience. I think my comment may only be relevant to the local exp, generally I think we all do use the modern practices but I've seen copilot output a healthy amount of legacy stuff, it would be nice to steer it away
|
|
||
| ## Best practices | ||
|
|
||
| - Refer to official documentation and best practices for React.js, Redux and Node.js. If you see anti-patterns, highlight them and provide links to the relevant official documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't want to give it too much of an opinion but either here or above I would add a mention of using modern JS practices since its been trained on a lot of legacy stuff, like I see copilot not using for-of when it should etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point!
Description
Note: I've tested a basic react antipattern and the freeStorage tag, but mostly I think we'll just need to see how it performs on real PRs and keep adjusting for a good signal-to-noise ratio.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes