feat(tooling): add antd-button-to-core codemod - #30754
Conversation
Second Wave 1 transform (guide: docs/antd-migration/button.md). Maps type->color incl. the no-type default inversion (antd default 'default' -> explicit color=secondary since core defaults to primary), folds danger into -destructive colors, remaps ghost->tertiary with a per-site review warning, applies the approved size map (small/middle/large -> xs/sm/md), renames disabled/loading/icon/htmlType with type-collision ordering handled, moves block to tw:w-full, passes refs through (forwardRef landed in #30664), and skips-with-warnings the hand-finish punch list (shapes, Button.Group, dynamic type/size, loading objects). 44 tests; suite 81/81. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| root.find(j.JSXElement).forEach((elPath) => { | ||
| const classification = classifyElement(elPath.node); | ||
| if (!classification) { | ||
| return; | ||
| } | ||
| if (classification.kind === 'ButtonGroup') { | ||
| buttonSkips.push({ kind: 'Button.Group', reason: 'button-group' }); | ||
| return; | ||
| } | ||
| if (classification.kind === 'ButtonGroupSubpath') { | ||
| subpathSkips.push({ kind: 'ButtonGroup', reason: 'button-group-subpath' }); | ||
| return; | ||
| } | ||
| const result = convertElement(elPath.node); | ||
| if (result.skip) { |
There was a problem hiding this comment.
⚠️ Edge Case: Non-JSX references to Button break when antd import is removed
Full-conversion detection (fullyConvertedFile = buttonSkips.length === 0) and element matching only inspect j.JSXElement nodes. Any non-JSX reference to the imported Button identifier — e.g. styled(Button), Button.defaultProps, or passing Button as a prop/argument — is invisible to the transform, so the antd Button import is removed while the reference remains, producing code that no longer resolves Button. During a bulk sweep this silently emits broken files (caught only later at build time). Consider scanning for remaining Identifier references to buttonLocalName outside the converted JSX (mirroring the typography transform's bareUsageFound guard) and forcing partial-conversion/keeping the import when any exist.
Was this helpful? React with 👍 / 👎
| const ghostAttr = findAttr(attrs, 'ghost'); | ||
| let ghostApplies = false; | ||
| if (ghostAttr) { | ||
| const lit = readAttrLiteral(ghostAttr); | ||
| if (!lit.isLiteral || typeof lit.value !== 'boolean') { | ||
| return { skip: true, reason: 'dynamic-ghost' }; | ||
| } | ||
| ghostApplies = lit.value === true; | ||
| } | ||
|
|
||
| const typeAttr = findAttr(attrs, 'type'); | ||
| let typeValue = null; | ||
| if (typeAttr) { | ||
| const lit = readAttrLiteral(typeAttr); | ||
| if (!lit.isLiteral || typeof lit.value !== 'string') { |
There was a problem hiding this comment.
💡 Bug: ghost + danger silently drops destructive styling
When both ghost (or type="ghost") and danger are present, ghostApplies short-circuits color to 'tertiary' and the danger attribute is dropped (it's in EXCLUDE), losing the destructive intent that antd renders for a danger ghost button. The element still converts and emits only a generic ghost-remap warning that doesn't mention the discarded danger. Consider either skipping (skip: true) or emitting a distinct warning when ghostApplies && dangerApplies so reviewers know the destructive styling was dropped.
Was this helpful? React with 👍 / 👎
| const newAttrs = []; | ||
| attrs.forEach((attr) => { | ||
| if (attr.type !== 'JSXAttribute') { | ||
| newAttrs.push(attr); // JSXSpreadAttribute — pass through | ||
| return; | ||
| } | ||
| if (EXCLUDE.has(attr.name.name)) { | ||
| return; // handled below | ||
| } | ||
| newAttrs.push(attr); | ||
| }); | ||
|
|
||
| newAttrs.push(j.jsxAttribute(j.jsxIdentifier('color'), j.stringLiteral(color))); | ||
| if (sizeValue) { | ||
| newAttrs.push(j.jsxAttribute(j.jsxIdentifier('size'), j.stringLiteral(sizeValue))); |
There was a problem hiding this comment.
💡 Edge Case: Appended color/renamed props can override a trailing spread
Converted props (color, renamed size/isDisabled/etc., and merged className) are always appended at the end of the attribute list. For <Button type="primary" {...props}>, antd lets a later spread override the explicit prop, but after transform color is appended after the spread and now wins — a behavior change if props intended to override. This is a narrow edge case (spread-after-explicit); if worth handling, insert generated attributes at the original attribute's position rather than at the end.
Was this helpful? React with 👍 / 👎
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| j.jsxAttribute(j.jsxIdentifier('className'), j.stringLiteral('tw:w-full')) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| opening.attributes = newAttrs; | ||
| return { skip: false, warnings }; | ||
| } | ||
|
|
||
| const buttonSkips = []; // ties up the antd Button import; forces partial conversion | ||
| const subpathSkips = []; // unrelated import; warn-only |
There was a problem hiding this comment.
When an antd button uses block={false} or block={isFullWidth}, the transform treats the attribute's presence as true and always appends tw:w-full, causing the converted button to remain full width even when block evaluates to false.
✅ Playwright Results — workflow succeededValidated commit ✅ 549 passed · ❌ 0 failed · 🟡 1 flaky · ⏭️ 3 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 48m 43s ⏱️ Max setup 2m 56s · max shard execution 15m 12s · max shard-job elapsed before upload 18m 10s · reporting 6s 🌐 201.74 requests/attempt · 2.83 app boots/UI scenario · 12.39% common-shard skew Optimization targets still in progress:
🟡 1 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (7)
tooling/antd-codemods/transforms/antd-button-to-core.js:269
- After introducing
blockApplies,classNameshould only be excluded (and later re-added) whenblockactually applies. Otherwiseblock={false}will still drop the originalclassName.
if (blockAttr) {
EXCLUDE.add('className');
}
tooling/antd-codemods/transforms/antd-button-to-core.js:301
- The full-width
classNamemerge should be gated onblockApplies(literaltrue) rather than the mere presence of ablockattribute, to avoid turningblock={false}into full width.
if (blockAttr) {
tooling/antd-codemods/transforms/antd-button-to-core.js:237
blockis treated as truthy based on attribute presence only. This will incorrectly addtw:w-fullforblock={false}and cannot safely representblock={expr}(should be skipped + warned). Parseblockas a literal boolean and skip when it's dynamic; only apply full-width behavior when it resolves totrue.
This issue also appears in the following locations of the same file:
- line 267
- line 301
const blockAttr = findAttr(attrs, 'block');
const classNameAttr = findAttr(attrs, 'className');
if (blockAttr && classNameAttr) {
const isPlainString =
classNameAttr.value &&
(classNameAttr.value.type === 'StringLiteral' ||
classNameAttr.value.type === 'Literal');
if (!isPlainString) {
return { skip: true, reason: 'dynamic-classname' };
}
}
tooling/antd-codemods/tests/antd-button-to-core.test.js:229
- The
blockmapping tests don’t coverblock={false}(should not become full-width) orblock={expr}(should be skipped + warned). Adding these cases will prevent regressions in the full-width conversion logic.
// -- `block` into existing/absent className + dynamic-className skip --
defineInlineTest(
transform,
OPTS,
tooling/antd-codemods/tests/antd-button-to-core.test.js:364
- There’s no assertion that the skip warning includes a distinct reason for dynamic
block={expr}. Adding a focused warning test helps ensure reviewers see the correct hand-finish category.
it('warns with the file path and skip reason for a dynamic size', () => {
transform(
{
path: 'src/components/Foo.tsx',
source: `import { Button } from 'antd';\nconst App = ({ sz }) => <Button size={sz}>Click</Button>;`,
tooling/antd-codemods/README.md:44
- README references
.context/wave1-prep/button-gap-check.md, but that path isn’t present in the repo. This makes the guidance hard to follow for contributors.
Mapping rules approved 2026-07-30 — see
`.context/wave1-prep/button-gap-check.md` and the mapping guide
(`docs/antd-migration/button.md`) for the full survey data behind them.
tooling/antd-codemods/README.md:99
- Once the transform skips
block={expr}(dynamic) with a dedicated reason, the README should document that skip category alongsidedynamic-classnameso sweep reviewers know what to hand-finish.
- **`block` combined with a non-literal `className`** (e.g.
`className={someVar}`) — `dynamic-classname`. The transform can only
safely merge `tw:w-full` into a string literal.
…ock the merge queue (#30784) * ci(playwright): raise the chromium shard budget to 21 minutes The chromium lane outgrew a 19-minute shard. At the COMMON_MAX_SHARDS ceiling of 24 the heaviest shard is predicted at 19.2m, so assign_lane_within_budget() raises SystemExit and full-mode planning aborts before a single test runs. Every merge_group run today failed this way (PRs #30705, #30768, #30458, #30725, #30754), while pull_request_target runs pass because targeted selection is far smaller. Raise COMMON_SHARD_BUDGET_MS from 19m to 21m. At 24 shards the heaviest is 19.2m, so the loop is guaranteed to converge at or before the ceiling. 21m stays inside the 25m `timeout` wrapper around `npx playwright test` and the 35m playwright-ci-postgresql job clock, leaving ~4m of headroom. Note the common lane now sits 1m above the dedicated lanes rather than 1m below. The strict 20-minute TARGET_MS ceiling is unaffected: it bounds a single atomic unit, not a shard, so a 21m shard built from units each under 20m does not trip it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(playwright): cover full-mode allocator convergence at the shard ceiling Addresses review on #30784. assign_lane_within_budget() was only exercised in "targeted" mode, so neither the full-mode convergence path nor the SystemExit at COMMON_MAX_SHARDS had coverage -- the exact code path that took the merge queue down. Add both: - test_full_mode_chromium_converges_at_the_shard_ceiling builds a lane that needs the window above 19m and asserts the allocator converges at or before the ceiling. Verified as a real guard: with the budget reverted to 19m it fails with "needs more than 24 shards ... heaviest shard is predicted at 20.4m". - test_full_mode_chromium_reports_a_lane_the_ceiling_cannot_hold pins the SystemExit path, which had no coverage at all. Also reword the budget comment: ~4m of headroom is relative to the 25m playwright timeout wrapper specifically, not to the 35m job clock, which is looser and additionally absorbs setup/teardown. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar | Powered by Gitar — free for open source
Summary
The Button sweep's mechanical transform (Wave 1, #30565 / epic #30570), implementing the approved mapping in
docs/antd-migration/button.md(landed in #30721):type→colorfor all literals, including the subtle default inversion: antd's no-typedefault is default, core's is primary, so a bare<Button>gets explicitcolor="secondary"to preserve appearancedangerfolds into-destructivecolors branching on the base type;ghost/type="ghost"converts tocolor="tertiary"with a per-siteghost-remapwarning for reviewer eyeballssmall/middle/large → xs/sm/md;disabled→isDisabled,loading→isLoading,icon→iconLeading,htmlType→type(visual-type rewrite ordered first so the two never collide);blockmergestw:w-fullinto className;refpasses through (forwardRef landed in feat(ui-core): forward refs on Button (+ vitest bootstrap) #30664)Button.Group(incl. subpath imports), dynamic type/size,loadingobjects, dynamic classNames onblockCoreButtonalias, imports managed with license-header preservationTests
44 new (37 inline transforms + 7 warning-behavior); full codemod suite 81/81 (move-named-imports 10 + typography 27 + button 44).
Fixes #30753
🤖 Generated with Claude Code
Greptile Summary
Adds an antd Button-to-core codemod and its documentation.
Confidence Score: 4/5
The PR is not yet safe to merge because conditional and false-valued
blockprops are still converted into unconditional full-width styling.The transform checks only whether a
blockattribute exists and always appendstw:w-full, soblock={false}and runtime conditions lose their original width behavior.Files Needing Attention: tooling/antd-codemods/transforms/antd-button-to-core.js; tooling/antd-codemods/tests/antd-button-to-core.test.js
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Find antd Button imports] --> B[Classify each Button usage] B -->|Unsupported shape or dynamic prop| C[Keep antd Button and warn] B -->|Supported props| D[Rewrite props for core Button] D --> E{Any skipped antd Buttons?} E -->|Yes| F[Keep antd import and add CoreButton alias] E -->|No| G[Replace antd import with core Button] C --> EReviews (2): Last reviewed commit: "Merge branch 'main' into antd-migration/..." | Re-trigger Greptile