-
Notifications
You must be signed in to change notification settings - Fork 5
sample code change as test for copilot #267
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
base: main
Are you sure you want to change the base?
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 introduces a new demo React component to the codebase. The component serves as a test example for Copilot integration.
Key Changes:
- Added a new
DemoComponentfunctional component that renders a Material-UI Paper element with custom styling
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const API_KEY = 'n0t_a_real_api_key_1234567890' |
Copilot
AI
Nov 18, 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.
API key is hardcoded in the source code. Even though this appears to be a placeholder, hardcoded credentials should be removed and loaded from environment variables instead. Consider using process.env.REACT_APP_API_KEY or a similar configuration approach.
| const API_KEY = 'n0t_a_real_api_key_1234567890' |
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 is a fake key, but it's great to see that copilot catches this
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const API_KEY = 'n0t_a_real_api_key_1234567890' | ||
|
|
||
| const DemoComponent: FC = () => { | ||
| return <Paper sx={{ color: '#549903'}}>Demo Component</Paper> |
Copilot
AI
Nov 18, 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 color value '#549903' is a magic number that reduces maintainability. Consider defining it as a named constant (e.g., PRIMARY_GREEN) or using a theme color from the MUI theme system.
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| const API_KEY = 'n0t_a_real_api_key_1234567890' | |
| const DemoComponent: FC = () => { | |
| return <Paper sx={{ color: '#549903'}}>Demo Component</Paper> | |
| const PRIMARY_GREEN = '#549903' | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| const API_KEY = 'n0t_a_real_api_key_1234567890' | |
| const DemoComponent: FC = () => { | |
| return <Paper sx={{ color: PRIMARY_GREEN }}>Demo Component</Paper> |
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.
Not quite. We would rather discourage the use of colors outside our MUI theme.
|
Even though I set up repo instructions for Copilot, those seem to be ignored! What is missing:
Next time @copilot read the instructions! |
|
@ann-kilzer I've opened a new pull request, #268, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
This PR is stale. Consider a pair-programming session to help move it forward. |
Resolves #
What changed 🧐
Please list the changes that are in this PR
See CONTRIBUTING for guidelines
How did you test it? 🧪
Please describe new unit tests, E2E tests, or manual testing steps.