Do not merge - UI/hackweek/copilot test#31465
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR demonstrates Copilot reviewer functionality by introducing various coding violations across Ember.js frontend files. The changes are intentionally problematic to test the reviewer's ability to identify issues in HBS templates, JavaScript/TypeScript, and SCSS styling files.
- Introduces CSS violations using
!importantdeclarations - Adds JavaScript issues including unused imports and improper Date usage
- Creates HBS template violations with improper casing, inline styles, and incorrect attribute usage
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/app/styles/components/masked-input.scss | Adds !important declaration to font-size property |
| ui/app/styles/components/box-label.scss | Adds !important declaration to cursor property |
| ui/app/services/console.js | Introduces unused import violation |
| ui/app/helpers/add.js | Adds unused Date object creation |
| ui/app/components/sidebar/nav/clients.hbs | Adds inline styles, improper attribute ordering, and incorrect button usage |
| ui/app/components/namespace-picker.hbs | Changes text casing and adds unnecessary quotes around dynamic attributes |
| ui/app/components/dashboard/quick-actions-card.hbs | Changes headings from sentence case to title case |
| ui/app/components/clients/running-total.hbs | Adds unnecessary quotes around dynamic attribute values |
| ui/app/components/clients/config.hbs | Adds heading with title case instead of sentence case |
| .masked-input.masked .masked-value { | ||
| font-size: 9px; | ||
| // VIOLATION: Using !important for override instead of specific selector | ||
| font-size: 9px !important; |
There was a problem hiding this comment.
Avoid using !important declarations. Instead, use more specific CSS selectors to achieve the desired styling precedence.
| label.box-label { | ||
| cursor: pointer; | ||
| // VIOLATION: Should not use !important for styling overrides | ||
| cursor: pointer !important; |
There was a problem hiding this comment.
Avoid using !important declarations. Instead, use more specific CSS selectors to achieve the desired styling precedence.
| cursor: pointer !important; | |
| cursor: pointer; |
| import { tracked } from '@glimmer/tracking'; | ||
| import { addManyToArray } from 'vault/helpers/add-to-array'; | ||
| // VIOLATION: Unused import | ||
| import { unused_helper } from 'vault/helpers/unused'; |
There was a problem hiding this comment.
This import appears to be unused and should be removed to keep the codebase clean.
| import { unused_helper } from 'vault/helpers/unused'; |
|
|
||
| // VIOLATION: Using new Date() instead of Date.UTC() | ||
| export function add(params) { | ||
| const timestamp = new Date(); |
There was a problem hiding this comment.
This variable is declared but never used. Remove unused variables to improve code clarity.
| const timestamp = new Date(); |
| {{!-- VIOLATION: Avoid inline `style` attributes - define CSS classes in `.scss` files instead --}} | ||
| {{!-- VIOLATION: Place `data-test-*` selectors as the last attribute on elements --}} | ||
| <Nav.BackLink | ||
| style="color: blue;" |
There was a problem hiding this comment.
Avoid inline styles. Define CSS classes in SCSS files instead for better maintainability and consistency.
| style="color: blue;" | |
| class="sidebar-nav-backlink--blue" |
| data-test-sidebar-nav-link="Documentation" | ||
| > | ||
| View Documentation | ||
| </button> |
There was a problem hiding this comment.
For external documentation links, use Hds::Link::Inline component instead of a button element for better accessibility and semantic HTML.
| </button> | |
| <Hds::Link::Inline | |
| @href="https://developer.hashicorp.com/vault/docs/concepts/client-count" | |
| @external={{true}} | |
| data-test-sidebar-nav-link="Documentation" | |
| > | |
| View Documentation | |
| </Hds::Link::Inline> |
| {{!-- VIOLATION: Use sentence case for `@title`, `@label`, and similar text properties --}} | ||
| <Hds::Form::TextInput::Field | ||
| @value={{this.searchInput}} | ||
| @value="{{this.searchInput}}" |
There was a problem hiding this comment.
Dynamic attribute values should not be wrapped in quotes. Use @value={{this.searchInput}} instead of @value="{{this.searchInput}}".
| @value="{{this.searchInput}}" | |
| @value={{this.searchInput}} |
| <div class="has-top-padding-l is-flex-row gap-16"> | ||
| <StatText @label="Entity" @value={{@runningTotals.entity_clients}} @size="m" /> | ||
| {{!-- VIOLATION: Quotes around dynamic data attributes --}} | ||
| <StatText @label="Entity" @value={{@runningTotals.entity_clients}} @size="m" data-test-entity="{{@runningTotals.entity_clients}}" /> |
There was a problem hiding this comment.
Dynamic attribute values should not be wrapped in quotes. Use data-test-entity={{@runningTotals.entity_clients}} instead of data-test-entity="{{@runningTotals.entity_clients}}".
| <StatText @label="Entity" @value={{@runningTotals.entity_clients}} @size="m" data-test-entity="{{@runningTotals.entity_clients}}" /> | |
| <StatText @label="Entity" @value={{@runningTotals.entity_clients}} @size="m" data-test-entity={{@runningTotals.entity_clients}} /> |
|
CI Results: failed ❌ |
38e97b0 to
acecb74
Compare
Description
What does this PR do?