Skip to content

Commit 2b7b96c

Browse files
chore: pr review instructions, address comments.
1 parent d953982 commit 2b7b96c

4 files changed

Lines changed: 63 additions & 9 deletions

File tree

.github/instructions/pr-review.md

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
applyTo: '**'
3+
---
4+
5+
# Pull Request Review Guidance
6+
7+
You are reviewing changes for @knighted/develop. Be concise, technical, and specific. Prioritize actionable feedback tied to concrete files and lines.
8+
9+
## Browser support policy
10+
11+
- Target evergreen browsers only (current stable Chrome, Edge, Safari, and Firefox).
12+
- Do not require old-browser compatibility unless the PR explicitly expands support scope.
13+
- Do not request legacy polyfills or fallback-only workarounds by default.
14+
15+
## Focus areas
16+
17+
- CDN-first runtime integrity: imports and fallback behavior should remain compatible with src/modules/cdn.js patterns.
18+
- UI state correctness: drawers, toggles, dialogs, and compact/mobile states should remain synchronized and predictable.
19+
- BYOT safety: token handling must stay browser-local, avoid leakage, and preserve clear user-facing privacy/removal cues.
20+
- Accessibility and semantics: form controls, labels, button types, ARIA relationships, and keyboard interactions should be valid.
21+
- Build and workflow stability: scripts and output behavior should remain consistent unless change is explicitly documented.
22+
- Tests and docs alignment: behavior changes should update tests and relevant docs.
23+
24+
## What to verify
25+
26+
- No generated artifacts are edited (dist/, coverage/, test-results/).
27+
- CDN import/fallback behavior is not bypassed with ad hoc URLs in feature modules.
28+
- Sensitive values (PAT/token) are not logged or exposed in UI/status output.
29+
- New UI behavior is covered in Playwright where appropriate.
30+
- Lint/build expectations still pass for changed areas.
31+
32+
## Validation expectations
33+
34+
- Run npm run lint for code and HTML/a11y checks.
35+
- Run npm run build when touching scripts/, bootstrap/runtime wiring, or import map behavior.
36+
- For interactive UI changes, confirm behavior in compact/mobile layout and at least one non-default mode.
37+
38+
## Review output format
39+
40+
- Present findings first, ordered by severity.
41+
- Label each finding as blocking, important, or nit.
42+
- Include file reference and the minimal fix direction.
43+
- Keep summary brief and secondary to findings.
44+
45+
## Ask for changes when
46+
47+
- Behavior changes ship without corresponding test updates.
48+
- New dependencies are added without clear approval.
49+
- Build/CI/import-map contracts change without docs updates.
50+
- Accessibility regressions or semantic HTML issues are introduced.
51+
- Feedback requests are based on unsupported legacy-browser constraints.

AGENTS.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,13 @@ Repository structure:
5555
- Maintain graceful fallback behavior when CDN modules fail to load.
5656
- Keep the app usable in local dev without requiring a local bundle step.
5757

58+
## Browser support policy
59+
60+
- Target evergreen browsers only (current stable Chrome, Edge, Safari, and Firefox).
61+
- Prefer platform features available in evergreen browsers without adding legacy polyfills.
62+
- Do not add legacy-browser workarounds unless a task explicitly requires expanded support.
63+
- In code review, treat requests for old-browser compatibility as out-of-scope unless documented.
64+
5865
## Testing and validation expectations
5966

6067
- Run npm run lint after JavaScript edits.

playwright/app.spec.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,9 @@ test('deleting saved GitHub token requires confirmation modal', async ({ page })
256256
await expect(page.locator('#clear-confirm-copy')).toHaveText(
257257
'This action removes the token from browser storage. You can add another token at any time.',
258258
)
259-
await expect(dialog.getByRole('button', { name: 'Remove' })).toBeVisible()
259+
const removeButton = dialog.getByRole('button', { name: 'Remove' })
260+
await expect(removeButton).toBeVisible()
261+
await expect(removeButton).not.toHaveAttribute('aria-label')
260262

261263
await dialog.getByRole('button', { name: 'Cancel' }).click()
262264
await expect(dialog).not.toHaveAttribute('open', '')
@@ -265,7 +267,7 @@ test('deleting saved GitHub token requires confirmation modal', async ({ page })
265267

266268
await tokenDelete.click()
267269
await expect(dialog).toHaveAttribute('open', '')
268-
await dialog.getByRole('button', { name: 'Remove' }).click()
270+
await removeButton.click()
269271
await expect(dialog).not.toHaveAttribute('open', '')
270272

271273
await expect(page.locator('#status')).toHaveText('GitHub token removed')

src/app.js

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,6 @@ const byotControls = createGitHubByotControls({
538538
title: 'Remove saved GitHub token?',
539539
copy: 'This action removes the token from browser storage. You can add another token at any time.',
540540
confirmButtonText: 'Remove',
541-
confirmButtonAriaLabel: 'Confirm remove saved GitHub token',
542541
fallbackConfirmText:
543542
'Remove saved GitHub token? This action removes the token from browser storage.',
544543
onConfirm,
@@ -983,7 +982,6 @@ const confirmAction = ({
983982
title,
984983
copy,
985984
confirmButtonText = 'Clear',
986-
confirmButtonAriaLabel,
987985
fallbackConfirmText,
988986
onConfirm,
989987
}) => {
@@ -1012,11 +1010,7 @@ const confirmAction = ({
10121010

10131011
if (clearConfirmButton instanceof HTMLButtonElement) {
10141012
clearConfirmButton.textContent = confirmButtonText
1015-
if (typeof confirmButtonAriaLabel === 'string' && confirmButtonAriaLabel) {
1016-
clearConfirmButton.setAttribute('aria-label', confirmButtonAriaLabel)
1017-
} else {
1018-
clearConfirmButton.removeAttribute('aria-label')
1019-
}
1013+
clearConfirmButton.removeAttribute('aria-label')
10201014
}
10211015

10221016
pendingClearAction = onConfirm

0 commit comments

Comments
 (0)