Repository navigation
chore: Add agent coding-convention instructions #5047
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
Merged
Merged
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
8248051
chore: Add agent coding-convention instructions
gethinwebster 4aa8c8b
chore: Refine component-structure instructions
gethinwebster 4f190aa
chore: Drop niche CSS-custom-properties SCSS rule
gethinwebster 46c70b5
chore: Document useBaseComponent config-arg usage
gethinwebster ede05bd
chore: Tune instruction rules to reduce false positives
gethinwebster e45f0bb
chore: Exempt AppLayout and permutations pages from SimplePage rule
gethinwebster 66cb44f
chore: Refine test-utils rule, drop generic duplicate-tests rule
gethinwebster af52dd4
Potential fix for pull request finding 'Include implementation.tsx an…
gethinwebster c626e10
Potential fix for pull request finding 'Allow documented exceptions f…
gethinwebster 6aff0f9
Potential fix for pull request finding 'Correct inaccurate widgetized…
gethinwebster File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| # Cloudscape components — repo conventions | ||
|
|
||
| You are reviewing/writing code as a Cloudscape maintainer: terse, self-explanatory code, | ||
| no parallel implementations, strong types, and the public API kept small and consistent. | ||
| When you flag something, cite the rule and say why. These are the repo's house conventions. | ||
|
|
||
| ## Comments & documentation | ||
| - Comment only the non-obvious WHY (why a warning is suppressed, why a workaround exists, | ||
| an a11y constraint). Delete comments that restate what the adjacent code plainly does | ||
| ("Build tree", "Compute layout"). WHY: restatements are noise; unexplained hacks bite later. | ||
| - Never silently drop an existing explanatory comment during a refactor — the rationale | ||
| is the part a reader can't reconstruct. | ||
|
|
||
| ## Reuse before adding | ||
| - Before writing a helper, search `src/internal/` and `@cloudscape-design/component-toolkit` | ||
| for an existing one (e.g. `useUniqueId`, `isHTMLElement`, `findUpUntil`). Reuse existing | ||
| constants instead of hardcoding magic numbers. WHY: divergent reimplementations drift and | ||
| multiply maintenance; the toolkit versions are more complete. | ||
| - Prefer ONE shared implementation over parallel branches (sticky vs virtual, VR vs one-theme, | ||
| collapsible vs not): factor common logic into one hook/mixin. WHY: parallel copies drift apart | ||
| and double the bug surface. | ||
|
|
||
| ## Types over runtime checks | ||
| - Constrain values with unions/enums, not a runtime keyword array + `isX` check. Remove | ||
| redundant `as X` casts. Never use `as any`. WHY: type-level constraints catch invalid values | ||
| at compile time; runtime checks fail silently and are hard to debug. | ||
|
|
||
| ## Naming | ||
| - Never name by physical position — `isRightmost`/`left`/`right` flip under RTL. Use logical | ||
| names (`isLastColumn`). WHY: the codebase runs RTL; position names become wrong. | ||
| - Name by purpose, not data-structure shape or generic terms (`ColumnGroupsLayout`, not | ||
| `HierarchicalThing`; `column`, not `leaf`). WHY: shape/generic names force readers to | ||
| reverse-engineer intent. | ||
|
|
||
| ## i18n | ||
| - Never hardcode an English fallback. Resolve translatable strings via `useInternalI18n('component')` | ||
| and let the provider supply the default; `?? ''` is only for type-safety on required string | ||
| props, never an English string. WHY: a hardcoded fallback bypasses the provider and ships | ||
| untranslated text. | ||
|
|
||
| ## Scope & process | ||
| - Keep PRs scoped: don't fold an unrelated change (flipping a shared constant, a drive-by | ||
| refactor) into a feature PR — land it as its own PR first. WHY: unrelated changes hide | ||
| regressions and enlarge the blast radius. | ||
| - Treat the public API of an already-public component as a stable contract. Flag changes that break | ||
| consumers — removing/renaming a public prop or export, narrowing a prop's type, tightening | ||
| optional→required, changing an observable default, or altering published test-utils behavior — and | ||
| prefer deprecation (keep it, mark `@deprecated`, add the replacement) over removal. Additive changes | ||
| and required→optional relaxations are fine. WHY: this is a published component library; a breaking | ||
| change ripples across every consumer. (See `.github/instructions/component-structure.instructions.md` | ||
| for the file-level detail.) | ||
|
|
||
| --- | ||
| Path-specific conventions (component structure & API, tests, SCSS, dev pages, test-utils) are in | ||
| `.github/instructions/*.instructions.md` and apply automatically to matching files. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| --- | ||
| applyTo: "src/*/{index,internal,interfaces}.{ts,tsx}" | ||
| --- | ||
| # Component structure & doc voice | ||
|
|
||
| Apply the contract rules below — they hold regardless of a component's shape. But **structure has several | ||
| legitimate shapes** in this repo, so before flagging anything shape-related, check the accepted-variants | ||
| list further down and don't flag a component for matching one of them. Cite the rule when flagging; never | ||
| invent a divergence. | ||
|
|
||
| ## Contract rules — hard, shape-independent (a violation is almost certainly a bug) | ||
| - **Wrapper checklist (new, non-single-impl components):** `index.tsx` starts with `'use client';` as its | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: After the copyright/liscense header? |
||
| first statement, calls `applyDisplayName(X, '${Name}')`, and has `export { ${Name}Props }` + one default | ||
| export. These are near-universal in the repo — flag any that are absent. | ||
| - **forwardRef ⇔ `Props.Ref`.** If (and only if) the component exposes a `${Name}Props.Ref`, it must use | ||
| `React.forwardRef` and forward the ref. A component with **no** `Ref` correctly uses a plain function — | ||
| absence of `forwardRef` is only a problem when a `Ref` exists (or a widgetized seam blocks it). | ||
| - **`Ref` exposes methods, not the DOM node** (`focus()`, `select()`), never `HTMLElement`/`element:`. | ||
| - **This component's own sub-types live under `export namespace ${Name}Props`** — its `Ref`, its | ||
| `*Detail`/`*ChangeDetail` event payloads, and its variant/size/type aliases must NOT be loose top-level | ||
| exports. (Private sub-component prop types are exempt — see the allowlist.) | ||
| - **`getBaseProps(...)` reaches the root:** the impl (wherever it lives) calls `getBaseProps` and spreads it | ||
| on the root — on EVERY return branch (incl. loading/early returns) — so consumer `className`/`id`/`data-*` | ||
| are not dropped. Flag only if genuinely dropped. | ||
| - **`__internalRootRef` is attached to the root element** (directly or via `useMergeRefs`). | ||
| - **Internal-only props are `__`-prefixed.** Concrete test: a prop on the internal props type but NOT on the | ||
| public `${Name}Props` is internal-only → it must start with `__`. Flag any that doesn't. | ||
| - **Internal props derive from the public API** — don't re-declare a prop the public `${Name}Props` already | ||
| has on the internal props type (promote with `SomeRequired<…>`). | ||
|
|
||
| ## Interface typing | ||
| - A union representing a **visual variant / size / type** must be a named alias in the namespace | ||
| (`${Name}Props.Variant`). **Do NOT flag** behavioral-mode unions (`keyboardActivationMode`, | ||
| `contentRenderStrategy`) or unions referencing another component's type. | ||
| - Event handler props (`on*`) use `CancelableEventHandler<Detail>` / `NonCancelableEventHandler<Detail>`, | ||
| never a bare function. WHY: the build fails for non-standard event-handler types. | ||
| - Doc comments must be Documenter-extractable and describe the prop, not the component. Only flag a MISSING | ||
| doc when sibling props in the same interface are documented and this one conspicuously is not — do not | ||
| demand JSDoc on every namespace/`i18nStrings` sub-field. | ||
|
|
||
| ## API doc voice & tone (SOFT — surface as non-blocking nits, not blocking flags) | ||
| Existing docblocks vary, so treat these as gentle nudges toward the house voice, not hard failures. | ||
| - **Open with a third-person present-tense verb**, declaratively: `Specifies …`, `Determines …`, `Adds …`, | ||
| `Called when …`. Avoid imperative openers ("Set this to…"), questions, and addressing "you" in the FIRST | ||
| sentence (a following `Use this to…` guidance sentence may). | ||
| - **Booleans:** a third-person verb + "whether" (`Determines whether …` / `Specifies whether …`), optionally | ||
| `If \`true\`, …`. **Events:** `Called when …` (+ `The event \`detail\` contains …`). **aria props:** | ||
| `Adds \`aria-x\` to <element>`. | ||
| - **Enums (3+ values):** a bulleted list of backticked values marking `(default)`. Inline `(default)` / | ||
| "Defaults to…" prose is fine; only flag a redundant hand-written scalar `@default` **tag** (Documenter | ||
| extracts defaults). **Mechanics:** capitalize + terminating period; backtick code tokens and sibling-prop | ||
| references; terse and factual (no marketing, no inline rationale beyond one guidance clause); a | ||
| `@deprecated` note names its replacement. | ||
|
|
||
| ## Data flow (in the impl, not the wrapper — subject to the wrapper-side-hooks variant below) | ||
| - State/controllability (`useControllable`), context (`useFormFieldContext`), events | ||
| (`fireNonCancelableEvent`), i18n (`useInternalI18n`, no hardcoded English fallback) live in the impl. | ||
| - Call `warnOnce` directly — it already gates on the dev environment; don't wrap it in a separate | ||
| `isDevelopment` check. | ||
| - `useBaseComponent`'s config arg (`{ props, metadata }`) is a fire-once-on-mount usage-telemetry | ||
| snapshot, so put only stable, low-cardinality config in it. `props` carries actual public config prop | ||
| values passed through directly — enumerated `variant`/`size`/`type`/density unions, booleans, and | ||
| low-cardinality numbers (a numeric prop value like `limit` in `props` is fine). `metadata` carries | ||
| derived facts — booleans (`hasX: Boolean(...)`) or numbers (counts) — especially reductions of | ||
| high-cardinality inputs. Never pass `children`, i18n strings, handlers, ids/identifiers, labels, or | ||
| user values; reduce any high-cardinality signal to a boolean/number in `metadata`. Omit the arg when | ||
| nothing is trackable. | ||
|
|
||
| ## Accepted architectural variants — DO NOT flag these | ||
| - **Single-impl:** a component with no `internal.tsx` folds its implementation into `index.tsx` (plus | ||
| helper files). `getBaseProps`, state, events, context, `forwardRef` and the root node legitimately live | ||
| in `index.tsx` here (e.g. tabs, tag-editor, badge, textarea). | ||
| - **Widgetized / loadable:** `internal.ts(x)` re-exports `createWidgetizedX(createLoadableComponent(…))`; | ||
| the real impl is in `implementation.tsx`, internal props in `internal-interfaces.ts` (e.g. flashbar, | ||
| app-layout, table, breadcrumb-group, drawer). Don't expect `InternalX`/`getBaseProps`/root in | ||
| `internal.tsx`; `forwardRef` may be impossible through the widgetized seam. | ||
|
Copilot marked this conversation as resolved.
Resolved
|
||
| - **Wrapper-side hooks:** `index.tsx` may run analytics/funnel hooks, `useSetGlobalX`, | ||
| `getAnalyticsMetadataAttribute`, or a loading/skeleton fallback (alert, modal, breadcrumb-group, button, | ||
| input). "Thin wrapper" is a tendency, not a rule — only flag wrapper logic with no such purpose. | ||
| - **`internal.tsx` holding private sub-components** (`TagControl`, `UndoButton`) rather than an `InternalX` is fine. | ||
| - **Sanctioned imports:** importing a sibling's `internal.tsx`/`implementation.tsx`, or another component's | ||
| **publicly exported** interface, is normal. Only flag importing another component's **styles** or clearly | ||
| private files across `src/<other>/`. | ||
| - **Top-level prop types of private SUB-COMPONENTS** (`BreadcrumbItemProps`, `TagControlProps`) are not | ||
| `${Name}Props` sub-types — do not force them into the namespace. (This exemption is ONLY for sub-component | ||
| prop types — see the namespace contract rule for the component's own sub-types.) | ||
|
|
||
| ## Generic sweep (best effort, lowest priority) | ||
| - After the above, compare against 2–3 sibling components and flag any OTHER real divergence not covered | ||
| here (an undocumented prop where siblings document theirs, unusual file naming). Respect the accepted | ||
| variants — do not flag a shape that matches a known variant. Name the sibling you compared against. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| --- | ||
| applyTo: "pages/**" | ||
| --- | ||
| # Dev / demo page conventions | ||
| - Build pages with the `SimplePage` helper (h1, content paddings, i18n, screenshot area), not | ||
| hand-rolled bespoke scaffolding. WHY: consistent, low-boilerplate, avoids a11y violations. | ||
| EXCEPTIONS — don't flag these page types for not using `SimplePage`: (a) **AppLayout demo pages**, | ||
| which can't nest inside `SimplePage` (itself a content wrapper); (b) **permutations pages** — the | ||
| `*.permutations.page.tsx` shape built from `createPermutations` + `PermutationsView` + `ScreenshotArea` | ||
| with a bare `<h1>` heading is the established structure for screenshot-matrix pages. | ||
| - Keep pages minimal: render the components under test directly (a single `SpaceBetween` is enough); | ||
| no decorative containers/subtitles, no hand-maintained prose list of components. WHY: test pages | ||
| should isolate behavior; decorative structure and lists drift. | ||
| - Drive page configuration (variant, selection, toggles) from URL query params via `AppContext`, | ||
| not local `useState` or click-only buttons. WHY: URL state is directly targetable by integ tests | ||
| and shareable as a link. | ||
| - Reuse shared fixtures (`generateItems`, shared column configs, `common-props`) instead of | ||
| duplicating; add `screenshotArea={{}}` for coverage and keep the page header outside it. WHY: | ||
| deduplicates fixtures, keeps screenshots isolated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| applyTo: "src/**/*.scss" | ||
| --- | ||
| # SCSS conventions | ||
| - Use existing design tokens instead of hardcoded values (`$box-shadow-focused-width`, radius | ||
| tokens). WHY: keeps theming consistent. | ||
| - Put classes that exist only for test selection in `src/<component>/test-classes/styles.scss`. | ||
| WHY: segregated test classes keep integ selectors stable. | ||
| - Don't disable the repo's own lint rules or write selectors that depend on raw DOM structure / | ||
| `nth-child` — add classnames to the inner elements and target those. WHY: structure-dependent | ||
| selectors and rule overrides are brittle and re-break as markup changes. | ||
|
Copilot marked this conversation as resolved.
Resolved
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| applyTo: "src/test-utils/**" | ||
| --- | ||
| # Test-utils (public contract) conventions | ||
| - Selector methods must generate a STATIC CSS selector with no access to query results — they | ||
| can't branch on `nth-child` conditionally. Design the DOM (e.g. always-present `data-column-index`) | ||
| so one static selector works, or take an explicit mode param. WHY: test-utils are a cross-version | ||
| consumer contract; a selector that needs results to decide its shape can't be generated. | ||
| - Test-utils methods declare explicit return types; wrapper classes expose a static `rootSelector`; | ||
| always-present results use a non-null assertion. WHY: documented public-contract conventions. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| --- | ||
| applyTo: "**/{__tests__,__integ__,__a11y__,__motion__}/**" | ||
| --- | ||
| # Testing conventions | ||
| - Assert the observable EFFECT (focus lands on the specific element, value/count/position is | ||
| correct), never `toBeDefined()`/"renders"/"does not crash". A test with no real assertion isn't | ||
| a test. WHY: `toBeDefined()` passes even when focus lands on `document.body`. | ||
| - Never write filler tests just to keep patch/codecov green — restructure or test real behavior. | ||
| WHY: coverage-gaming tests assert accidental behavior and are frequently AI-generated noise. | ||
| - Test through the component/public surface; find elements with official test-utils | ||
| (`createWrapper().findX()`), not raw class-name/DOM selectors. WHY: proves the helper works AND | ||
| is wired correctly; test-utils are the stable contract. EXCEPTION: when no test-utils finder exists | ||
| for the element/state (e.g. an animation/settle state, or a styling primitive like Box whose public | ||
| contract IS its class names), a scoped class assertion is acceptable. | ||
| - Split unit vs integ by what each can verify: explicit handlers/logic in unit, real focus/Tab in | ||
| integ. Don't fire an event on an element the user can't focus. WHY: emulating an impossible | ||
| interaction verifies nothing; integ repetition is expensive. | ||
| - Don't `istanbul ignore` testable code (e.g. keyboard nav — testable on a plain `<table>`). | ||
| Restructure so the util is reachable. WHY: ignored branches hide real, testable behavior. | ||
| - Assert dev warnings by mocking `warnOnce` from `@cloudscape-design/component-toolkit/internal` | ||
| and asserting `(componentName, message)`. WHY: the established deterministic pattern. | ||
| - Name tests by user-observable behavior, not internal util names. WHY: internal names in titles | ||
| rot and don't communicate intent. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.