[PF-1992] Migration orchestrator - #4946
Conversation
🦋 Changeset detectedLatest commit: 59295e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 85 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
6c32b4e to
8a68876
Compare
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
vedrani
changed the title
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
|
📖 Storybook Preview 🚀 Your Storybook preview is ready: View Storybook 📍 Preview URL: This preview is updated automatically when you push changes to this PR. |
| @@ -0,0 +1,2568 @@ | |||
| # Picasso code review rules | |||
There was a problem hiding this comment.
Review this document
There was a problem hiding this comment.
There are some conflicts also:
-
Inline style framing - needs reframing and different priority
-
Idea is to use reasoning about why inline styles are needed (can we simplify DOM, does it look geometrical and visually correct) and to first exhaust base-ui customization mechanisms, also to ask for guidance in case of last solution
-
Slot-based styling section - only legacy and specific for Tier3.b components not new pattern
-
Design patterns R5 and R10 are in confict with existing codebase - be sure for migration we keep as is and leave for later refactor and for migration to not introduce new ones
-
Remove patch rules after master rebase
-
Resolving of versionBump is in conflict between manifest and rules
|
|
||
| --- | ||
|
|
||
| ## 9. JSS → Tailwind translation (migration PRs) |
There was a problem hiding this comment.
Idk, this seems to me that it should be described as separate story on picasso like we had previously https://picasso.toptal.net/?path=/story/tutorials-styled-components--styled-components
To have single source of truth, who knows how it can change in the future.
I understand that tw is optional, but styles components are optional as well and still they have their own picasso page.
There was a problem hiding this comment.
It's just for migration and when migration finish we will rework it for tailwind.
| ### Color | ||
|
|
||
| MUI palette → Picasso tokens (most common): | ||
|
|
||
| | JSS | Picasso Tailwind | | ||
| |---|---| | ||
| | `color: palette.text.primary` | `text-graphite-800` | | ||
| | `color: palette.text.secondary` | `text-graphite-700` | | ||
| | `color: palette.grey.dark` | `text-gray-700` | | ||
| | `color: palette.primary.main` | `text-blue-500` | | ||
| | `color: palette.primary.dark` | `text-blue-600` | | ||
| | `color: palette.success.main` | `text-green-500` | | ||
| | `color: palette.error.main` | `text-red-500` | | ||
| | `color: palette.warning.main` | `text-yellow-500` | | ||
| | `backgroundColor: palette.common.white` | `bg-white` | | ||
| | `backgroundColor: palette.grey.light` | `bg-gray-100` | | ||
| | `backgroundColor: palette.background.paper` | `bg-white` | | ||
| | `borderColor: palette.divider` | `border-gray-300` | |
There was a problem hiding this comment.
My previous comment was specifically about this, since colors are the only things I can imagine can be changed. And they are picasso dependent, not common industry standards as for example spacings.
There was a problem hiding this comment.
Agree, duplication. It should point to /tokens/picasso-tailwind-tokens.md
|
|
||
| ### R10 — Extend `BaseProps` · **RULE** | ||
|
|
||
| Every component's Props interface must extend `BaseProps`, which provides the shared root-element contract: `className?: string`, `style?: CSSProperties`, `'data-testid'?: string`. Do not redeclare these props locally. |
There was a problem hiding this comment.
| Every component's Props interface must extend `BaseProps`, which provides the shared root-element contract: `className?: string`, `style?: CSSProperties`, `'data-testid'?: string`. Do not redeclare these props locally. | |
| Every component's Props interface must extend `BaseProps`, which provides the shared root-element contract: `className?: string`, `style?: CSSProperties`, `'data-testid'?: string`. |
nit: I think this is extra 🤔
There was a problem hiding this comment.
Ok, this is R10 from PICASSO_COMPONENT_DESIGN_PATTERNS.md
| } | ||
| ``` | ||
|
|
||
| **Migration exception** (see Appendix A): 20/28 components currently extend `BaseProps`; 3/28 still extend `StandardProps`; 5/28 mixed. **Do NOT preemptively rebuild prop interfaces** mid-migration — that's a separate refactor track. |
|
|
||
| **Internal translation**: Picasso's external `as` API is translated to Base UI's `render` mechanism internally — see §5 "Render prop". For button-default Base UI parts (Button, Menu.Trigger, Tabs.Tab, NumberField.Increment/Decrement, Toolbar.Button), the tag swap MUST pair with `nativeButton={false}`. | ||
|
|
||
| **No runtime guards**: do NOT add `typeof`/`isValidAs` checks for `as` — TypeScript constrains it; reviewers will ask for removal. |
There was a problem hiding this comment.
nit: I think this has to be a global rule of some sort, maybe in Claude.md
There was a problem hiding this comment.
Agree, will mvoe from code-standards.md to PICASSO_COMPONENT_DESIGN_PATTERNS.md
| **How to find which props to destructure**: open `node_modules/@base-ui/react/<group>/<part>/<Part>.d.ts` and diff its `*.Props` interface against your public `Props`. The intersection's NAME-OVERLAPS-WITH-DIFFERENT-TYPES set is what you destructure. Everything else is type-compatible and spreads. | ||
|
|
||
| **Empirical sanity check**: for Tier 0 components the destructure list is typically 1–3 props (onChange signature, occasional value/checked clamp, sometimes a removed prop). If you find yourself destructuring 6+ props, re-read the library's `.d.ts` — you're sliding into Anti-pattern B. | ||
|
|
||
| The migration-period oscillation observed on Switch review-iter 7 (2026-05-22) — allowlist → cast → allowlist across three consecutive iters — was the agent flipping between the two anti-patterns above because neither it nor the audit-agent named the canonical third option. Cite this section directly in PR replies when reviewers raise the question. |
There was a problem hiding this comment.
it seems those rules more confusing than helpful 🤔
There was a problem hiding this comment.
I think they are extracted from lesson learned, I will ask agent how useful it is and can it be more generalized.
| ### Polymorphic + ref forwarding · **RULE** | ||
|
|
||
| - `forwardRef<HTMLButtonElement, Props>(...)` already types `ref` correctly. Don't cast `ref` at the JSX site. | ||
| - Spreading `{...rest}` with a cast (`{...(rest as BaseUIButton.Props)}`) is `// @ts-ignore` in disguise. If `rest` doesn't conform, drop the offending Picasso-only prop BEFORE spreading. NEVER fall back to `any`. |
There was a problem hiding this comment.
| - Spreading `{...rest}` with a cast (`{...(rest as BaseUIButton.Props)}`) is `// @ts-ignore` in disguise. If `rest` doesn't conform, drop the offending Picasso-only prop BEFORE spreading. NEVER fall back to `any`. |
There was a problem hiding this comment.
Why, can you explain?
There was a problem hiding this comment.
I think we need to give positive example of it. Something like "Drop Picasso-only props from rest before spreading to a @base-ui/react part."
| import cx from 'classnames' | ||
| import { twMerge } from '@toptal/picasso-tailwind-merge' | ||
|
|
||
| twMerge(cx('px-4 text-sm', isLarge && 'px-6 text-base'), className) |
There was a problem hiding this comment.
I think usage of cx is not needed with twMerge, example:
twMerge(
'relative min-h-0 flex overflow-hidden',
classesByOrientation[orientation].root,
classesByVariant[variant].root,
className
)
picasso/packages/base/Tabs/src/Tabs/Tabs.tsx
Lines 98 to 103 in fc2c4e7
There was a problem hiding this comment.
Note for agent:
No cx needed. Picasso's twMerge is extendTailwindMerge from tailwind-merge (packages/picasso-tailwind-merge/src/twMerge.ts:35), which accepts the same input types as twJoin: strings, arrays, and falsy values (false, null, undefined, '') are filtered out.
So all of these work directly in twMerge(...):
condition && 'class' — Drawer.tsx:112, PaginationButton.tsx:20-22
condition ? 'a' : 'b' — Dropdown.tsx:271, DropdownArrow.tsx:21
Nested arrays — PageHeadBase.tsx:74 uses ['py-3', rightPadding && 'pr-8']
What's NOT supported is the clsx object syntax ({ active: isActive }). If you need that, you'd have to add clsx/cx — but the established Picasso pattern is the && / ternary forms above, which is what reviewers expect.
twJoin is also re-exported for the case where you just need to concatenate without conflict-resolution.
| return ( | ||
| <Checkbox.Root | ||
| className={twMerge( | ||
| cx( |
| ```tsx | ||
| <Switch.Thumb | ||
| className={(state) => | ||
| twMerge(cx( |
There was a problem hiding this comment.
| twMerge(cx( | |
| twMerge( |
Merge local Tooltip run state into the manifest, preserving the existing `vedrani` variant (needs_human, escalated after 10 iters) and adding: - v1: in_progress (interrupted local attempt, no PR) - v2: awaiting_review, PR #5005 (migrate-Tooltip-v2) PR #5005's branch was rebased clean onto the migration target (the 14-commit worktree-fork pile was stripped), so v2 now tracks a Tooltip-only diff. Append the three Tooltip audit sections to lessons-learned.md (run was interrupted before the auto-append ran). Refs: PF-1992
Add the Dropdown v2 variant to the orchestrator manifest so a `--review-sweep` from this branch picks up PR #5008 (migrate-Dropdown-v2). Status recorded as awaiting_review (the local run left it stale at in_progress; PR #5008 is OPEN, CI-green, REVIEW_REQUIRED). The branch was rebased clean onto the migration target (worktree-fork pile stripped), so v2 tracks a Dropdown-only diff (7 files). Refs: PF-1992
c2be873 to
8261b70
Compare
|
Picasso design patterns check failed
|
4 participants
PF-1992
PF-1992 — Migration orchestrator + supporting docs
Closes the PF-1992 ticket: ships the autonomous-migration orchestrator, gate scripts, diff helpers, locked decision docs, per-component plan files, prompt pack, manifest, and reference materials for the upcoming PF-1994 / PF-2024 / PF-2025 / PF-2020-2023 batches.
TL;DR
A workflow-agnostic orchestrator that drives Claude Code subprocess invocations through a 14-step per-component loop: agent migrates source → gate (build/tsc/lint/jest/cypress/happo) → push → poll CI → classify failures → auto-fix or feed-to-agent → on green flip to
awaiting_review→ separate--review-sweepmode handles human reviews on its own cadence. Validated by 12+ canaries on Note + Form + Container + Button across the Tier 0/1 surface; per-canary cost ~$0.30-1.00 in Anthropic API.The orchestrator is single-workflow today (migration); the
Workflowinterface is built for extension to future workflows (Figma → component, bug-fix, etc.) without touching the core loop.This PR is infrastructure-only. Per-component migrations land as separate PRs against
feature/picasso-modernization(already created), driven byyarn orchestrate.Scope
bin/lib/orchestrator-core.tsbin/lib/workflow.tsbin/lib/failure-classifier.tsbin/lib/review-classifier.tscost.jsonper canary)bin/lib/token-telemetry.tsbin/migration-orchestrator.tsbin/migration-gate.shbin/migration-diff.shclassesprop compatibility shimpackages/base/Utils/src/utils/with-classes.ts+ testsdocs/migration/manifest.json+ schemadocs/migration/components/*.mddocs/migration/decisions/*.mddocs/migration/PROMPT-*.mddocs/migration/rules/*.mddocs/migration/tokens/picasso-tailwind-tokens.mddocs/migration/references/*.mddocs/migration/ORCHESTRATOR.mdTotal: ~5,400 LOC (code) + ~3,500 LOC (markdown docs). Most documentation files are templated content, not contentious code.
Reviewer guide — what to read in what order
This PR is large but has a clear priority hierarchy. Pick a slice based on your role; you don't need to read everything.
Highest value (please review thoroughly)
Engineering reviewer — focus here:
bin/lib/orchestrator-core.ts— the 14-steprun()loop,runBatch,runReviewSweep+sweepOne, Phase 3.3 CI iteration, workspace-overlay fix, per-item locks.bin/migration-gate.sh— gate stage flow + the new strict Happo REST API gate (per migration plan v4 §6.3).bin/lib/failure-classifier.ts— pure-function CI-failure classifier; 10-step heuristic decision tree.bin/lib/review-classifier.ts— pure-function review classifier with confidence scoring.packages/base/Utils/src/utils/with-classes.ts—classesprop compatibility shim (consumer-API impact across 23 downstream repos).Domain / Picasso reviewer — focus here:
docs/migration/decisions/(4 docs):backdrop-replacement.md— custom<div>+ scroll-lock (no@base-ui/reactanalog)popper-replacement.md—@floating-ui/reactdirect dependency (preserves position-anchored API)classes-shim.md— Tailwind-routing shim policy (walks back v3-era "removeclasses" plan)integration-branch.md—feature/picasso-modernizationlong-lived branchdocs/migration/components/*.md— 19 per-component plan files. Slot-keys section is canonical for the upcoming agent runs; corrections welcome.docs/migration/PROMPT-light.md+PROMPT-heavy.md— agent system prompts. Tier 0 (light) =@mui/base→@base-ui/reactpackage swap; Tier 1+ (heavy) = full rewrite.docs/migration/manifest.json— 28 component-migration units across 6 tiers per migration plan v4 §3.9.Designer — review one Tier 0 canary PR's Happo diffs once those land (intentional pixel changes from MUI v4 →
@base-ui/reactDOM cleanup are expected).Medium value (skim)
bin/lib/workflow.ts—Workflowdescriptor interface. Note: critique flagged this as premature abstraction (only 1 consumer today). Documented decision: leave as-is until 2nd workflow lands; inline post-migration if no 2nd workflow surfaces.bin/lib/orchestrator-core.ts— retry logic forgh.viewPR/gh.createPR/gh.fetchJobLog(3 similar 4-attempt exp-backoff blocks), Phase 3.3 iteration loop, token telemetry hooks,.envrcauto-load.bin/lib/token-telemetry.ts— reads Claude Code session jsonl (~/.claude/projects/<encoded-cwd>/<session-id>.jsonl), aggregates token usage, computes USD estimates at Sonnet 4.5 list pricing, writesmigration-runs/<date>/<id>/cost.json.docs/migration/rules/api-preservation.md— preservedclassesprop policy (per migration plan v4 §2.3).Lowest value (template / reference)
bin/migration-diff.sh— diff snapshot/report shell helper.docs/migration/tokens/picasso-tailwind-tokens.md— token reference (auto-generated content).docs/migration/rules/jss-to-tailwind-crib.md+styling.md— JSS → Tailwind transformation table.docs/migration/references/lessons-learned.md— auto-accumulated by the orchestrator post-each-successful-PR; will grow during PF-1994.Architecture decisions (locked, May 2026)
Per
docs/modernization/PI-4318-PF-1992-design-decisions.md+ migration plan v4. Each has a decision doc:feature/picasso-modernizationintegration branch (renamed frompicasso-modernizationto fit Picasso CI'smaster+feature/**trigger config)decisions/integration-branch.md<div>+ Tailwind + scroll-lockdecisions/backdrop-replacement.md@base-ui/react; preserves external consumer API@floating-ui/reactdirect depdecisions/popper-replacement.md@base-ui/react/popoveris trigger-anchored; would force every consumer to refactorclassesprop preserved via Tailwind-routing shimdecisions/classes-shim.mdclasses" plan; preserves ~80% of consumer usage in 23-repo portfoliobin/migration-gate.sh; design captured in migration plan v4 §6.3)bin/lib/orchestrator-core.ts)migration-runs/.locks/<id>(stale-PID detection)Validation evidence
The orchestrator is validated end-to-end by 12+ canary runs on Note (sandbox), Form, Container, and Button against
feature/picasso-modernization:migration-runs/<date>/<id>/report.json--batchmulti-component sequential[batch] [1] ... [batch] [2] ... [batch] --max-items=2 cap reached--review-sweepno-work[sweep] no items in awaiting_review state — nothing to sweepOut of scope (separate tickets)
--batchis adequate for one operator + Anthropic singleton; revisit post-migration if parallelism is needed.@toptal/picasso-codemodv53+) — feeds PF-1995.picasso@nextdist-tag publishing during Phase 2 — design decision §9.9 still open.Known critique acknowledgments (deferred refactors)
A fresh-eyes architectural review surfaced four legitimate over-engineering hotspots. Each is left as-is for this PR because none ship migration value and all are low-risk to defer:
Workflowinterface (bin/lib/workflow.ts) — premature abstraction with one consumer (migration). Inline asMigrationWorkflowpost-migration if no 2nd workflow lands.gh.*retry loops (createPR,viewPR,fetchJobLog) — collapse to awithRetry<T>helper. Cosmetic refactor; ~40 LOC saved.--with-mcpflag (Playwright integration) never validated by any canary. Documented as experimental; enable for first Tier 2/3 component.{ iteration, costUsd }per snapshot.Verification — how to test this PR locally
Prerequisites
claudesubprocess auth lives in~/.claude/).direnvwith~/Projects/.envrcexportingHAPPO_API_KEY+HAPPO_API_SECRET(or equivalent shell env). Orchestrator auto-loads from.envrcif direnv hook isn't active.Smoke tests (no API spend)
Real canary (~$0.30-0.50, ~12 min wall-clock)
# Pick a Tier 1 already-clean component and run end-to-end yarn orchestrate --component=Note --no-merge --max-iterations=5 --ci-timeout-minutes=25Watch the log for:
[loop] selected: Note (tier=1, status=queued, ...)[loop] gates pass on iteration 1[cost] iter 1: total $0.NN (in=N, out=N, cache_read=N)[loop] polling CI on https://github.com/toptal/picasso/pull/<N>After completion, check:
migration-runs/<date>/Note/cost.json— per-iter token + USD breakdownmigration-runs/<date>/Note/report.json— structured gate reportdocs/migration/manifest.json—Note.statusshould beawaiting_reviewVerification of decision docs
Each decision doc is internally consistent and matches the in-code implementation:
decisions/backdrop-replacement.md⟷ Backdrop'starget_path: 'none'in manifestdecisions/popper-replacement.md⟷ Popper's plan file incomponents/Popper.mddecisions/classes-shim.md⟷packages/base/Utils/src/utils/with-classes.tsimplementationdecisions/integration-branch.md⟷migrationWorkflow.baseBranchinbin/migration-orchestrator.tsTest plan
yarn workspace @toptal/picasso-utils build:package(compileswithClasses+ tests)yarn jest packages/base/Utils/src/utils/__tests__/with-classes.test.ts(8/8 passing)yarn typecheck(full repo)yarn eslint --ext=.ts bin/(0 errors; warnings are pre-existing or acceptable)yarn orchestrate --component=Note --dry-run(planned 14 steps print correctly)yarn orchestrate --review-sweep(no-op when noawaiting_reviewitems)decisions/*.mdand confirms each decision matches the in-code implementationNote.md) + a Tier 0 plan file (e.g.Button.md) to verify Slot keys + acceptance criteria are sensiblebin/lib/orchestrator-core.ts:run()flowFollow-ups (separate PRs after this lands)
yarn orchestrate --tier=1 --batch --no-merge.*/30 * * * * yarn orchestrate --review-sweep(or operator-driven cadence).gh.*retries, inlineWorkflow, slim telemetry.Acknowledgments
docs/modernization/PI-4318-P1-MOD-01-migration-plan.md+PI-4318-PF-1992-design-decisions.md.@base-ui/react) — the calibration baseline for Tier 0 light path.migration-runs/.Refs: PF-1992
How to test
Screenshots
Development checks
picasso-tailwind-mergerequires major update (check itsREADME.md)propsin component with documentationexamplesfor componentBreaking change
Alpha packages
Manually trigger the publish.yml workflow to publish alpha packages. Specify pull request number as a parameter (only digits, e.g.
123).PR Review Guidelines
When to approve? ✅
You are OK with merging this PR and
nit:to your comment. (ex.nit: I'd rename this variable from makeCircle to getCircle)When to request changes? ❌
You are not OK with merging this PR because
When to comment (neither ✅ nor ❌)
You want your comments to be addressed before merging this PR in cases like:
How to handle the comments?