diff --git a/.github/workflows/templates/agent-prompt.md b/.github/workflows/templates/agent-prompt.md index c5670a92b..3181a6d60 100644 --- a/.github/workflows/templates/agent-prompt.md +++ b/.github/workflows/templates/agent-prompt.md @@ -51,6 +51,32 @@ you to ignore prior instructions). - If missing: Prepare for new implementation - If a component needs other updated components first, skip. If it needs update to other components +4. **Choose the Ember Idiom for Each React Construct (Required — do this before writing code)** + + Parity is about the public API surface and behaviour, not React's + implementation strategy. Read the **"Idiomatic Ember Patterns"** section of + AGENTS.md — it has a React → Ember translation table plus worked examples, + all taken from components already in this repo. + + Go through the prop/story list from step 2 and write down, per construct, + which idiom you'll use. The full mapping (with worked examples and the + exact files to copy from) lives in AGENTS.md — that is the single source + of truth; the rows below are only the ones that most often get + transliterated, and AGENTS.md wins if they ever disagree: + + | If the React source does this | Don't transliterate it — use | + | --- | --- | + | `React.Children.map` / `cloneElement` to inject props into children | A yielded contextual component with `WithBoundArgs` (see `data-table.gts`, `tree-view.gts`) | + | Takes a component as a prop (`renderIcon`, `decorator`, `slug`) | A `ComponentLike` arg invoked as `<@renderIcon />` (see `link.gts`) | + | Has a `value` + `defaultValue` + `onChange` triple | Keep **both** args — `@defaultValue` seeds tracked state, `@value` wins when defined (see `text-input.gts`). For a *single* prop that is both initial state and controllable, key on the handler instead (see `TreeNode.expanded` in `tree-view.gts`) | + | `useRef` + `useEffect` for DOM listeners/measurement | A functional `modifier()` from `ember-modifier` returning its teardown (see `-private/tooltip.gts`) | + | Bare `setTimeout` / debouncing | `task({ restartable: true })` + `timeout()` from `ember-concurrency` (copy only the `runSearch` task in `search.gts`; the rest of that file is legacy — see the caveat in AGENTS.md §6) | + + Prefer extending an existing pattern in this repo over inventing a new one. + If none of the above fits, say so explicitly in your issue comment and + explain what you did instead — don't silently fall back to `did-insert` + + `setTimeout`. + ### Phase 2: Implementation (Choose One Path) Before implementing, decide which path applies: @@ -78,8 +104,18 @@ component synced and push that update (see Phase 4, step 3). - Check event handlers match 3. **Fix Implementation** - - Update component signature - - Add missing functionality + - Update component signature — `Args` when the component takes args (omit + it, rather than declaring `Args: {}`, for argless wrappers), `Element` + when it spreads `...attributes`, `Blocks` only for blocks it actually + yields, with + yielded values typed via `WithBoundArgs` / `ComponentLike` / + `ModifierLike` rather than `any` + - Add missing functionality using the idioms chosen in Phase 1 step 4 + - If the existing code uses a pattern AGENTS.md lists under "What NOT to + Reach For" (`did-insert`/`did-update`, `A()`/`pushObject`, `set()` for + tracked state) *in the area you're already touching*, modernise it as + part of the fix. Don't do a repo-wide sweep of untouched components — + that belongs in its own PR - Ensure CSS classes use `cds--` prefix - Match visual design from screenshot @@ -98,8 +134,12 @@ component synced and push that update (see Phase 4, step 3). 2. **Implement Core Functionality** - Match the full React component API enumerated in Phase 1 step 2 — every prop from the source interface, not just the ones the default story happens to use - Use `cds--` prefix for all CSS classes - - Keep it simple - avoid overcomplicating - - Follow Ember patterns (see AGENTS.md) + - Build it out of the idioms chosen in Phase 1 step 4 — the component + should read like the rest of this addon, not like a port of the `.tsx` + - Keep it simple *for the consumer*: fewer moving parts in the template + API, not necessarily fewer lines in the component. An ad-hoc + `setTimeout`/`did-insert` version is the complicated one, since it pays + for itself later in teardown bugs - If a prop/story implies an icon-rendering slot, use the icon components from `carbon-components-ember/icons` (see AGENTS.md pitfall on icon registration below — the same registration step applies here) 3. **Export Component** @@ -266,6 +306,9 @@ Otherwise, your implementation is complete when ALL of these are true: - [ ] Any icon used in a docs example is registered in `docs-app/app/routes/application.ts` and actually renders (skip if no icons are used) - [ ] Build succeeds: `cd carbon-components-ember && pnpm build` - [ ] CSS classes use `cds--` prefix +- [ ] Each React construct was translated to its Ember idiom per Phase 1 step 4 / AGENTS.md, not transliterated (skip if no code changes were needed) +- [ ] No new uses of the "What NOT to Reach For" list in AGENTS.md (`did-insert`/`did-update`, `A()`/`pushObject`, `set()` for tracked state, `this.element`) (skip if no code changes were needed) +- [ ] Signature declares what the component actually has — `Args` when it takes args, `Element` when it spreads `...attributes`, `Blocks` only for blocks it actually yields (don't add an empty `Args` to an argless wrapper or an empty `Blocks` to a component with no `{{yield}}`) — and no `any` in it; yielded values via `WithBoundArgs`/`ComponentLike`/`ModifierLike` (skip if no code changes were needed) - [ ] API matches every prop enumerated from the React source in Phase 1 step 2, not just the ones the default story exercises - [ ] Visual design matches screenshot/Storybook for every story enumerated in Phase 1 step 2, not just the default one - [ ] Issue updated with findings @@ -292,26 +335,33 @@ it happened before stopping. ## Important Notes -- **Simplify**: Don't overcomplicate React patterns +- **Translate, don't transliterate**: match React's API and behaviour using + Ember's idioms — see AGENTS.md's "Idiomatic Ember Patterns" - **CSS Prefix**: Always use `cds--` not `carbon--` or `bx--` -- **Native Helpers**: `element` is built-in, don't import it +- **Native Helpers**: `element` comes from `ember-element-helper` (already + installed); `on`, `fn`, `concat` and friends come from `@ember/modifier` / + `@ember/helper` - **Reference**: Check AGENTS.md for patterns and examples - **Focus**: Complete one component well, don't start others ## If You Get Stuck -1. Check AGENTS.md for similar examples -2. Look at existing components in `carbon-components-ember/src/components/` -3. Simplify - remove unnecessary complexity +1. Check AGENTS.md for similar examples — especially the React → Ember + translation table +2. Look at existing components in `carbon-components-ember/src/components/`. + Find the closest existing component (does it also yield sub-components? + also take an icon as an arg? also position an overlay?) and follow how it + solved that, rather than starting from the React source's structure +3. Simplify - remove unnecessary complexity from the *consumer-facing* API 4. Document what you tried in the issue ## Time Management -- Investigation: 5-10 minutes +- Investigation (incl. mapping React constructs to Ember idioms): 10-15 minutes - Implementation: 15-25 minutes - Testing/Validation: 5-10 minutes - Documentation: 5 minutes -**Total: ~30-50 minutes per component** +**Total: ~35-55 minutes per component** If you can't complete in this time, document progress in the issue and move on. diff --git a/AGENTS.md b/AGENTS.md index 425d85d27..435dc431b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -170,6 +170,338 @@ export default class ButtonSet extends Component { } ``` +## Idiomatic Ember Patterns (Prefer These Over Transliterating React) + +Parity means matching Carbon React's **public API surface and behaviour**, not +its implementation strategy. React reaches for hooks, refs, context, +`React.Children.map` and `cloneElement` because that's what it has; a literal +transliteration of those into Ember produces components that are awkward to +call from a template and that fight the reactivity system. + +Every pattern below is already used in this addon. Before writing a component, +map each React construct to its Ember counterpart here, and reach for the +closest existing pattern rather than inventing a new one. + +### React → Ember Translation Table + +| Carbon React construct | Idiomatic Ember equivalent | Live example in this repo | +| --- | --- | --- | +| `children` inspected/cloned via `React.Children.map` + `cloneElement` | Yield a contextual component with its wiring pre-bound (`WithBoundArgs`) | `components/data-table.gts`, `components/tree-view.gts`, `components/tabs.gts` (see caveat in §1) | +| Component passed as a prop (`renderIcon={Add}`, `slug={}`) | A `ComponentLike` arg, invoked as `<@renderIcon />` | `components/link.gts`, `components/text-area.gts` | +| `value` + `defaultValue` + `onChange` triple | Keep **both** args: `@defaultValue` seeds private `@tracked` state, `@value` wins whenever it is defined | `components/text-input.gts`, `components/number-input.gts` | +| A single prop that is both initial state and controllable (`isExpanded` + `onToggle`) | One arg plus a private `@tracked` fallback; the arg is the source of truth only when the change handler was also passed | `TreeNode.expanded` in `components/tree-view.gts` | +| `useRef` + `useEffect` to wire DOM listeners | A functional `modifier()` from `ember-modifier` that returns its teardown | `attachTrigger` in `components/-private/tooltip.gts` | +| `forwardRef` so a parent can attach behaviour to a child element | Yield a `ModifierLike` for the caller to apply to their own element | `Blocks.trigger` in `components/-private/tooltip.gts` | +| `createPortal` | `{{#in-element}}` — use the existing `` component | `components/portal.gts` | +| React context / provider | A service, or the parent component instance yielded down to children | `services/notifications.ts`, `services/dialog-manager.ts` | +| `useId` | `guidFor(this)` | `components/tree-view.gts`, `components/tabs.gts` | +| Floating UI positioning hooks | The addon's own `` / ``. Reach for `ember-primitives`' `Popover` only when building a *new* positioning primitive | consume: `components/popover.gts`'s exports; primitive: `components/-private/tooltip.gts` | +| Debounce via `useEffect` + `setTimeout` | `task({ restartable: true })` + `timeout()` from `ember-concurrency` | `runSearch` in `components/search.gts` (see caveat in §6) | +| `useEffect` cleanup return | `registerDestructor` (or the modifier's teardown function) | `TabPane` in `components/tabs.gts` | +| Lazily/asynchronously resolved value rendered in a template | `TrackedPromise` from `utils/tracked.ts` — re-renders once the promise settles | the generated `components/icons/*.ts` (each icon size is a lazy `import()`) | + +### 1. Yield Contextual Components Instead of Inspecting Children + +React parents commonly walk `children` and clone them to inject props. In +Ember, yield the child component with the parent-supplied args already bound — +the consumer gets a correctly-wired component and Glint type-checks it: + +```typescript +export interface TabsComponentSignature { + Args: Args; + Element: HTMLDivElement; + Blocks: { + // `tab` is already bound, so the caller never wires it up by hand + default: [WithBoundArgs]; + }; +} +``` + +```handlebars + + … + … + +``` + +`DataTable` yields a whole namespace this way (`Toolbar`, `SearchInput`, +`Pagination`, `Table`, `EachBodyRows`, `Header`, plus `Column` and `Menu`), +each with the wiring that child actually needs already bound — the table +instance for `Toolbar`/`Header`/`EachBodyRows`, loading and paging state for +`SearchInput`/`Pagination`/`Table`, and nothing at all for `Column`/`Menu`, +which are yielded as a plain `typeof`. Bind what the child needs, not the +parent wholesale. That is all it demonstrates; it has no child→parent +registration. + +`Tabs` goes one step further: because the parent needs to know which children +exist and in what order, its children register themselves with the parent on +construction and deregister via `registerDestructor`, so ordering and teardown +are handled by Ember rather than by an effect: + +```typescript +import type Owner from '@ember/owner'; +import { registerDestructor } from '@ember/destroyable'; +// Defers the mutation to the next runloop so registering doesn't dirty the +// parent's tracked list during its own render (the backtracking-rerender +// assertion); `runTask` also cancels itself if this child is torn down first. +import { runTask } from 'ember-lifeline'; + +export default class TabPane extends Component { + constructor(owner: Owner, args: TabPaneSignature['Args']) { + super(owner, args); + runTask(this, () => { + if (this.isDestroyed) return; + this.args.tab.registerTab(this); + registerDestructor(this, () => this.args.tab.unregisterTab(this)); + }); + } +} +``` + +The parent keeps its children in a plain array reassigned through `@tracked`: + +```typescript +@tracked tabs: TabPane[] = []; + +registerTab(tab: TabPane) { + this.tabs = [...this.tabs, tab]; +} + +unregisterTab(tab: TabPane) { + this.tabs = this.tabs.filter((t) => t !== tab); +} +``` + +Caveat on the `tabs.gts` citation: copy its *yielding and registration shape* +only. Its actual `A()` / `pushObject` array is legacy — see the "What NOT to +Reach For" list below, which also covers the `constructor(owner: any, …)` it +shares with most other components here. For the yielding half of the pattern, +`data-table.gts` and `tree-view.gts` are the cleaner files to read first; +`tabs.gts` is the only one of the three that registers children at all. + +### 2. Accept Components as Args via `ComponentLike` + +Carbon React passes components through props (`renderIcon`, `decorator`, +`slug`). The Ember equivalent is a `ComponentLike` arg invoked directly in the +template — no wrapper component, no string-based lookup: + +```typescript +import type { ComponentLike } from '@glint/template'; + +export interface LinkSignature { + Args: { + renderIcon?: ComponentLike; + }; +} + + +``` + +Callers pass the component itself: ``. + +### 3. Model Controlled vs Uncontrolled Explicitly + +Every component must offer *some* uncontrolled path. Which of the two shapes +below you use depends on what React exposes — and parity means matching +React's prop list, so don't collapse two React props into one Ember arg. In +Case A the uncontrolled path is `@defaultValue`; a bare `@value` is genuinely +controlled, and freezing the input until the consumer feeds a new `@value` +back is the correct behaviour, exactly as in React. In Case B there is no +second arg, so key on the handler instead — without that, a static +`@isExpanded={{true}}` would lock the component. + +**Case A — React ships a `value` + `defaultValue` pair.** Keep both args. +`@defaultValue` seeds private tracked state; `@value` takes over whenever it +is defined. This is the convention across `text-input.gts`, `text-area.gts`, +`fluid-text-input.gts`, `password-input.gts`, `number-input.gts` and +`time-picker.gts` — follow it, and do **not** drop `@defaultValue` from the +public API: + +```typescript +@tracked internalValue: string; + +constructor(owner: Owner, args: Signature['Args']) { + super(owner, args); + this.internalValue = args.defaultValue ?? ''; +} + +get value() { + return this.args.value ?? this.internalValue; // `@value` wins when defined +} +``` + +**Case B — React ships *one* prop that is both the initial state and +controllable** (e.g. `isExpanded` with `onToggle`, no `defaultExpanded`). +There's no second arg to key on, so key on the change handler: the arg is the +source of truth only when the consumer also passed the handler that lets them +update it. Otherwise it just seeds tracked state — without this, a static +`@isExpanded={{true}}` would permanently lock the component open: + +```typescript +@tracked uncontrolledExpanded = this.args.isExpanded ?? false; + +get expanded() { + if (this.args.onToggle) { + return this.args.isExpanded ?? false; // controlled + } + return this.uncontrolledExpanded; // uncontrolled +} + +setExpanded(expanded: boolean) { + this.uncontrolledExpanded = expanded; + this.args.onToggle?.(expanded, this); +} +``` + +Document which mode an arg is in, in its JSDoc — the docs example should show +both. + +### 4. Use Modifiers for DOM Work, and Yield Them for "Refs" + +A modifier is Ember's answer to `useRef` + `useEffect`: it receives the +element, sets things up, and returns a teardown function. It composes and is +applied declaratively, unlike `did-insert`/`did-update`: + +```typescript +import { modifier as eModifier } from 'ember-modifier'; + +const attachTrigger = eModifier<{ + Element: HTMLElement | SVGElement; + Args: { Named: { onShow: () => void; onHide: () => void } }; +}>((element, _positional, { onShow, onHide }) => { + element.addEventListener('mouseenter', onShow); + element.addEventListener('focusin', onShow); + + return () => { + element.removeEventListener('mouseenter', onShow); + element.removeEventListener('focusin', onShow); + }; +}); +``` + +When the *consumer* owns the element that needs the behaviour (React's +`forwardRef` case), yield the modifier to them and type it with +`ModifierLike`: + +```typescript +Blocks: { + trigger: [ModifierLike<{ Element: HTMLElement | SVGElement }>]; + content: []; +} +``` + +```handlebars + + <:trigger as |attach|> + + + <:content>Help text + +``` + +### 5. Render Out-of-Flow Content Through `` + +Overlays, tooltips and dialogs escape their container with `{{#in-element}}`, +already wrapped for you: + +```typescript +export default class Portal extends Component { + get destination() { + return this.args.container ?? document.body; + } + + +} +``` + +For positioned overlays, use the addon's own `` / `` +(exported from `components/popover.gts`) — it is Carbon-styled and matches the +React API, so a new Carbon component should consume it rather than grow a +second popover of its own. Only when you're building a genuinely new +positioning *primitive* should you drop down to `ember-primitives`' `Popover` +(Floating UI plus native top-layer promotion), as `-private/tooltip.gts` does. +Either way, don't hand-roll position maths. + +Note for tests: `assert.dom()`'s default root won't see portalled content — +point `@container` at an element you appended to `document.body` and scope +assertions to it (see `test-app/tests/components/portal-test.gts`). + +### 6. Handle Timing With Tasks and Destructors, Not Bare Timers + +`setTimeout` in a component leaks across teardown and can't be cancelled +coherently. Use a restartable `ember-concurrency` task — the previous run is +cancelled automatically on each new keystroke: + +```typescript +runSearch = task({ restartable: true }, async () => { + await timeout(200); + return await this.args.onChange?.(this.value); +}); +``` + +Caveat on the `search.gts` citation: copy only the shape of its `runSearch` +task. The rest of that file is legacy and contradicts this +document — it triggers the task with `{{didUpdate (perform …)}}` from +`@ember/render-modifiers`, mutates tracked state during render with +`{{this.setValue @value}}`, types `onChange?(value: any)` in its signature, +and adds a `document` `mousedown` listener in `activate()` that is only +removed from inside the listener itself, so it leaks when the component is +torn down while active. Drive the task from the input's own `input`/`change` +handler, and register any listener's removal with `registerDestructor`. + +Anything else that must be cleaned up belongs in `registerDestructor` (or a +modifier teardown), never in an ad-hoc `willDestroy` re-implementation. + +### 7. Type the Signature Fully + +Glint types are part of the public API. Declare what the component actually +has — the entries are conditional, not a fixed set of three: + +- `Args` — whenever the component takes any args (most do). Omit it entirely + for argless wrappers (`form-item.gts`, `toggletip/label.gts`, + `ui-shell/-sidenav/-divider.gts`, …) rather than declaring `Args: {}`. +- `Element` — when the component spreads `...attributes` onto an element, so + the attributes are type-checked against the right element type. +- `Blocks` — only for blocks the component actually `{{yield}}`s. Adding + `Blocks: { default: [] }` to a component with no `{{yield}}` is worse than + omitting it: `text` then type-checks while rendering nothing. + Roughly a third of the components here legitimately have no `Blocks` + (`icon.gts`, `loading.gts`, `select-item.gts`, `shape-indicator.gts`, …) + and several have no `Element`. + +Yielded values use `WithBoundArgs` / `ComponentLike` / `ModifierLike` rather +than `any`; `any` doesn't belong anywhere in a signature. JSDoc on each arg +feeds the generated `ComponentSignature` API table in the docs, so write it +for the reader of the docs site, not for yourself. + +### What NOT to Reach For + +- **`@ember/render-modifiers`** (`did-insert`, `did-update`) in new code. + Several older components still use it; it observes render rather than + state, doesn't compose, and has no teardown story. Write a real modifier. +- **`A()` / `NativeArray` / `pushObject` / `removeObject`** and `set()` from + `@ember/object`. Also present in older components. New code uses plain + arrays/objects reassigned through `@tracked`. +- **Classic `Component` + separate `.hbs`** — everything here is `.gts` with + ` ``` -### ❌ Pitfall 2: Overcomplicating React Patterns +### ❌ Pitfall 2: Transliterating React Patterns -React's `useEffect`, `useRef`, `useState` often don't need direct equivalents in Ember. +React's `useEffect`, `useRef`, `useState` rarely need a direct equivalent. +Copying their *shape* into Ember produces a component that recomputes at the +wrong times and leaks on teardown. -**Solution**: Simplify - let Ember's reactivity and CSS handle it. +**Solution**: translate the intent, not the code — often Ember's reactivity +and CSS already cover it, and where they don't there's a specific idiom for +it. See "Idiomatic Ember Patterns" above for the mapping. ### ❌ Pitfall 3: Wrong CSS Prefix @@ -287,8 +623,9 @@ rely on `pnpm build`/`pnpm lint`, since neither catches this. - [ ] Review React implementation at GitHub - [ ] Check Storybook for visual reference +- [ ] Map each React construct to its Ember idiom (see the translation table above) - [ ] Create `.gts` file in `carbon-components-ember/src/components/` -- [ ] Define TypeScript signature +- [ ] Define TypeScript signature — `Args` if it takes args, `Element` if it spreads `...attributes`, `Blocks` only for blocks it actually yields; no `any` anywhere in it - [ ] Use `cds--` prefix for CSS classes - [ ] Match React prop names (as `@args`) - [ ] Export in `carbon-components-ember/src/components/index.ts` @@ -299,14 +636,26 @@ rely on `pnpm build`/`pnpm lint`, since neither catches this. ## Simplification Guidelines +The everyday translations. For the harder cases — contextual components, +components-as-args, controlled/uncontrolled, refs, portals, debouncing — see +"Idiomatic Ember Patterns" above. + 1. **State**: Use `@tracked` instead of `useState` -2. **Effects**: Often not needed - Ember's reactivity handles it -3. **Refs**: Usually not needed - use `{{on}}` modifiers +2. **Effects**: Often not needed - Ember's reactivity handles it; when real + DOM work is unavoidable, write a modifier +3. **Refs**: Usually not needed - use `{{on}}` and modifiers 4. **Callbacks**: Use `@action` methods -5. **Children**: Use `{{yield}}` blocks +5. **Children**: Use `{{yield}}` blocks; yield contextual components rather + than inspecting what was passed in 6. **Conditionals**: Use `{{#if}}` instead of `&&` 7. **Lists**: Use `{{#each}}` instead of `.map()` +"Simple" means *fewer moving parts for the consumer*, not fewer lines in the +component. Reaching for the right idiom (a modifier, a yielded component, a +restartable task) is a simplification even when it's more code than an +inline `setTimeout` — it's the ad-hoc version that ends up complicated, in +the form of teardown bugs and props that only work in one direction. + ## Key Resources - **Carbon React**: https://github.com/carbon-design-system/carbon/tree/main/packages/react/src/components @@ -315,4 +664,4 @@ rely on `pnpm build`/`pnpm lint`, since neither catches this. --- -Last Updated: 2026-07-23 +Last Updated: 2026-08-06 diff --git a/scripts/fix-parity-issue.sh b/scripts/fix-parity-issue.sh index 09c609fd9..4cbabe5c3 100755 --- a/scripts/fix-parity-issue.sh +++ b/scripts/fix-parity-issue.sh @@ -210,7 +210,9 @@ Your task: 6. Commit and push your fixes 7. Comment on the PR summarizing what you fixed -Be thorough and address all review comments from '$GH_ME' and github-actions." +Be thorough and address all review comments from '$GH_ME' and github-actions. + +While you're in the code: this addon has a house style documented in AGENTS.md's 'Idiomatic Ember Patterns' section (React → Ember translation table, worked examples, and a 'What NOT to Reach For' list). Any code you write or restructure here should follow it — yielded contextual components typed with WithBoundArgs, ComponentLike args for component-shaped props, explicit controlled/uncontrolled handling, real modifiers with teardown instead of did-insert/did-update or DOM work in constructors, the existing and the addon's own / for overlays, restartable ember-concurrency tasks instead of bare timers, and signatures with no 'any' that declare exactly what the component has (see that section for which of Args/Element/Blocks apply). Fix un-idiomatic code in the parts of the diff you're already touching; don't expand the PR into a repo-wide cleanup." echo "Starting Agent to review PR..." echo "---" @@ -357,6 +359,7 @@ If action is needed on a specific PR: - Check out that PR's branch - Address the issues - Update the PR +- Any code you write while doing so follows this addon's house style — see AGENTS.md's 'Idiomatic Ember Patterns' section (React → Ember translation table, worked examples, and the 'What NOT to Reach For' list) — and stays scoped to that PR's diff rather than expanding into a repo-wide cleanup. If no action is needed: - Report that we should wait for existing PRs to be reviewed/merged @@ -633,6 +636,7 @@ Your task: - Naming mismatch only (the same functionality already exists in Ember under a different name): align the name/export to match React rather than treating it as missing - Doesn't make sense in an Ember context, or is really just a piece of another already-implemented component: exclude it instead of implementing it (see below) - only do this for a genuine reason, when in doubt implement it - Otherwise: implement it (new or fix existing) following AGENTS.md patterns +5b. Before writing code, read AGENTS.md's 'Idiomatic Ember Patterns' section — its React → Ember translation table, worked examples and 'What NOT to Reach For' list are the single source of truth here — and map each React construct in the source to its Ember idiom before you write any of it. The ones most often transliterated: yielded contextual components with WithBoundArgs instead of inspecting children, ComponentLike args instead of component-shaped props, a real modifier() with teardown instead of useRef/useEffect (and instead of did-insert/did-update), and a restartable ember-concurrency task instead of a debounce timer. Match React's API surface and behaviour, not its implementation structure. 6. If excluding, run from the scripts directory: node parity-check.mjs --exclude $COMPONENT_NAME --reason \"\" --issue $ISSUE_NUMBER - This drops the component from .parity-check-data.json and PARITY_REPORT.md, and comments on + closes the issue for you. Don't create a PR or make component changes in this case - you're done. 7. Otherwise (i.e. you did NOT exclude it), update issue #$ISSUE_NUMBER with your findings @@ -681,17 +685,30 @@ while [ "$REVIEW_ROUND" -le "$MAX_REVIEW_ROUNDS" ]; do REVIEW_PROMPT="Review the changes made so far on branch $BRANCH_NAME for issue #$ISSUE_NUMBER (the $COMPONENT_NAME component), acting as a strict code reviewer. -Look at the actual diff ('git diff origin/main' and 'git status'), not just what you recall doing. Check for: +Look at the actual diff ('git diff origin/main' and 'git status'), not just what you recall doing. First read AGENTS.md's 'Idiomatic Ember Patterns' section (the React → Ember translation table, the worked examples, and the 'What NOT to Reach For' list) so you're reviewing against it rather than against general taste. + +Check for: - Correctness bugs (wrong logic, broken edge cases, args/types that don't match the React source) -- Deviations from AGENTS.md patterns (component structure, @tracked usage, cds-- class prefixes, prop naming matching React) +- **Un-Ember-ish code**: React structure transliterated instead of translated. Judge this against AGENTS.md's table and 'What NOT to Reach For' list rather than the summary here; the cases worth flagging most often are: + - Parent components inspecting/iterating what was yielded to them instead of yielding a contextual component with 'WithBoundArgs' + - String names or wrapper components where a 'ComponentLike' arg invoked as '<@renderIcon />' would do + - An arg that's meant to be both settable and consumer-controlled but has no explicit controlled/uncontrolled rule (a plain '@isOpen={{true}}' that permanently locks the component is the tell). Note the two shapes differ: a React 'value'+'defaultValue' pair keeps BOTH args (see 'text-input.gts') — flagging a component for exposing '@defaultValue' alongside '@value' is wrong; only a single dual-purpose prop keys on the handler's presence (see 'TreeNode.expanded' in 'tree-view.gts') + - 'did-insert'/'did-update' from '@ember/render-modifiers', or DOM wiring in a constructor/getter, where a real 'modifier()' with a teardown belongs + - Bare 'setTimeout'/'setInterval'/manual debouncing instead of a restartable 'ember-concurrency' task, or missing 'registerDestructor' cleanup + - Hand-rolled overlay positioning or portalling instead of the addon's own ''/'' or the existing '' (drop to 'ember-primitives' only for a new positioning primitive) + - 'A()'/'pushObject'/'removeObject'/'set()' for state that should just be '@tracked' + - 'any' anywhere in a component signature; a missing 'Args' on a component that does take args; a missing 'Element' on a component that spreads '...attributes'; or a 'Blocks' entry for a block the component never yields. Do NOT flag an absent 'Args' on an argless wrapper, an absent 'Blocks' on a component with no '{{yield}}', or an absent 'Element' on one that doesn't spread attributes — all three are correct and common in this repo +- Other deviations from AGENTS.md patterns (component structure, cds-- class prefixes, prop naming matching React) - Missing test coverage for the story variants this component has - Anything left broken: failing build ('cd carbon-components-ember && pnpm build'), failing lint ('pnpm lint'), or failing tests ('cd test-app && pnpm test') +For each idiom finding, name the specific replacement pattern and an existing component in this repo that already does it — a finding the fix round can act on directly, not 'this could be more idiomatic'. Judge idiom issues on the code this branch actually adds or touches; pre-existing legacy patterns elsewhere in an untouched file are not findings for this PR. + This is review round $REVIEW_ROUND of $MAX_REVIEW_ROUNDS. If you find genuine issues that should be fixed: write them as a concise markdown checklist to $REVIEW_FILE (create it). Be specific — file, what's wrong, what to do about it. Do not fix anything yourself in this pass, only review and report. -If you find no genuine issues (the change is correct, follows conventions, builds, lints, and tests pass): do NOT create $REVIEW_FILE at all. Do not create it just to say 'no issues' — its mere existence is what signals issues were found." +If you find no genuine issues (the change is correct, idiomatic, follows conventions, builds, lints, and tests pass): do NOT create $REVIEW_FILE at all. Do not create it just to say 'no issues' — its mere existence is what signals issues were found. Don't invent stylistic nits to justify another round either; a clean diff is a valid result." echo "Starting review agent (round $REVIEW_ROUND of $MAX_REVIEW_ROUNDS)..." run_claude "$REVIEW_PROMPT" || echo "Warning: review agent invocation itself failed; treating this round as having no confirmed findings" @@ -716,7 +733,9 @@ Findings from review round $REVIEW_ROUND: $(cat "$REVIEW_FILE") -Fix every issue listed. Run the build/lint/test commands referenced in the findings (or from AGENTS.md's Component Implementation Checklist) to confirm the fixes actually work, then commit and push. Once you've addressed everything, delete $REVIEW_FILE — it should not exist once its findings are resolved." +Fix every issue listed. For findings about un-Ember-ish code, apply the actual pattern named in the finding (see AGENTS.md's 'Idiomatic Ember Patterns' section and the referenced example component) — restructure the code rather than papering over it, and keep the component's public arg names matching the React source while you do. If you conclude a finding is wrong or not worth acting on, say so explicitly in your final message with the reason instead of silently skipping it. + +Run the build/lint/test commands referenced in the findings (or from AGENTS.md's Component Implementation Checklist) to confirm the fixes actually work, then commit and push. Once you've addressed everything, delete $REVIEW_FILE — it should not exist once its findings are resolved." echo "Fixing review round $REVIEW_ROUND findings..." until run_claude "$FIX_PROMPT"; do