[chore] Split PlaygroundConfigSection - #5572
Conversation
Follows the parked Phase-1 plan: the file keeps its path (barrel import
unchanged) and its parts move into the co-located PlaygroundConfigSection/
dir, in a one-directional import DAG to avoid the circular-dep trap:
types <- constants <- {metadata, llmConfig, schemaData} <- atoms <- adapters
The component body's two contiguous blocks become hooks called at their
original positions, so hook order, count and dep arrays are unchanged:
useModelConfigurePopover the configure popover (26-entry useMemo dep
array kept as a memo, not made a component)
useFieldSlots the per-field slot renderers
The four public types moved to types.ts and are re-exported from
PlaygroundConfigSection.tsx, so no importer changed and no shim was left.
PlaygroundConfigSection.tsx: 2003 -> 478 lines.
dosubot
Bot
added
the
size:XXL
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
refactoring
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesPlayground configuration flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigSection as PlaygroundConfigSection
participant Popover as useModelConfigurePopover
participant Slots as useFieldSlots
participant Router as configUpdateRouterAtom
participant Molecule as workflowMolecule
ConfigSection->>Popover: initialize model configuration state
ConfigSection->>Slots: provide popover state and field dependencies
Slots->>Popover: render configuration popover content
Popover->>Router: dispatch configuration changes
Router->>Molecule: update configuration or sibling data
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/adapters.ts (1)
161-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the dead placeholder
schemaAtPath.
buildWorkflowMoleculeAdapterbuilds aschemaAtPaththat always returnsatom(() => null)(with a double cast and a comment referring to a wrapper), only forbuildDefaultAdapterto overwrite it. AssignmoleculeSchemaAtPathdirectly and drop the extra layer.♻️ Proposed simplification
selectors: { - schemaAtPath: memoAtom((key: string) => { - // key is serialized "{id}:{path}" — parse it - // But the interface takes {id, path}, so we use a wrapper below - return atom(() => null) - }) as unknown as ConfigSectionMoleculeAdapter["selectors"]["schemaAtPath"], + schemaAtPath: moleculeSchemaAtPath, }, } } -function buildDefaultAdapter(): ConfigSectionMoleculeAdapter { - const base = buildWorkflowMoleculeAdapter() - return { - ...base, - selectors: { - schemaAtPath: moleculeSchemaAtPath, - }, - } -} - -export const defaultAdapter = buildDefaultAdapter() +export const defaultAdapter = buildWorkflowMoleculeAdapter()web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/atoms.ts (2)
71-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
console.debugdebug artifact from the atom read.This runs on every recompute of the root-path schema atom in production and logs schema internals.
♻️ Proposed cleanup
- const resolved = getSchemaAtPathUtil(schema, params.path) ?? null - if (params.path.length === 0) { - console.debug("[PlaygroundConfigSection] root schema", params.id.slice(0, 8), { - schemaType: schema?.type, - schemaHasProperties: !!schema?.properties, - schemaPropertyKeys: schema?.properties - ? Object.keys(schema.properties as Record<string, unknown>) - : null, - resolvedType: resolved?.type, - resolvedPropertyKeys: resolved?.properties - ? Object.keys(resolved.properties as Record<string, unknown>) - : null, - }) - } - return resolved + return getSchemaAtPathUtil(schema, params.path) ?? null
54-60: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffUnbounded module-level cache keyed by
(id, path).Referential stability is the stated goal, but this map is never pruned: every revision id combined with every drilled-in path adds a permanent entry for the lifetime of the tab (the
memoAtomcaches inadapters.tshave the same characteristic, though those are keyed by id only). Consider an LRU bound or eviction when a revision is no longer mounted if long playground sessions traverse many paths.web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/useModelConfigurePopover.tsx (1)
533-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--ag-c-*literals carried over during the move.Since these lines are being relocated anyway, consider switching the popover chrome to Ant Design semantic tokens / Tailwind color utilities (or
var(--ag-color*)), and verify light + dark. Fine to defer if the split is meant to be byte-identical.As per coding guidelines: "Consume theme colors through Ant Design semantic tokens, Tailwind color utilities, or supported
var(--ag-color*)variables; do not use raw hex colors or--ag-c-*literals."Source: Coding guidelines
web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/useFieldSlots.tsx (1)
269-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
revisionIdandupdatePromptRootFieldare unused insidefieldHeaderSlot.They are only present to keep the dependency array identical to the monolith, and they force the header slot (and thus the drill-in slots object) to churn whenever
parameterschange viaupdatePromptRootField. Once you're confident the extraction is behavior-equivalent, dropping both from the deps and fromUseFieldSlotsParams(and from the call site inPlaygroundConfigSection.tsxlines 359/365) would remove two dead props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fc7eda6-8047-4145-a5d8-2fb41560db6b
📒 Files selected for processing (10)
web/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection.tsxweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/adapters.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/atoms.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/constants.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/llmConfig.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/metadata.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/schemaData.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/types.tsweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/useFieldSlots.tsxweb/packages/agenta-entity-ui/src/DrillInView/components/PlaygroundConfigSection/useModelConfigurePopover.tsx
Railway Preview Environment
Updated at 2026-07-31T19:19:53.085Z |
Three leftovers the split moved verbatim from the 2003-line monolith: - adapters.ts: buildWorkflowMoleculeAdapter built a schemaAtPath that always returned atom(() => null), only for buildDefaultAdapter to overwrite it. Assign moleculeSchemaAtPath directly; this also removes the `as unknown as` cast that was the only thing letting the wrong-typed placeholder compile. - atoms.ts: a console.debug fired on every recompute of the root-path schema atom, in production, logging schema internals. - useFieldSlots: revisionId and updatePromptRootField were never read in fieldHeaderSlot's body, only listed in its dep array, churning the drill-in slots object whenever parameters changed. Dropped from the params, the dep array and the call site; updatePromptRootField also leaves useModelConfigurePopover's return (still used inside that hook). Also fixes a stale closure the split inherited: stickyHeaderTop is read in fieldHeaderSlot but was missing from its deps. activeData stays out on purpose — its only use is hasParameters(activeData), which the parameters dep tracks. No behavior change otherwise. tsc clean for @agenta/entity-ui and @agenta/oss; pnpm lint-fix makes no changes.
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Context
PlaygroundConfigSection.tsxhad reached 2003 lines: 8 inline components and 26 module-level helpers in one file.Changes
The file keeps its path so the barrel import is unchanged. Its parts move into the co-located
PlaygroundConfigSection/directory, which already held four components.Helpers and types split along a one-directional import chain, which matters here because an earlier attempt at this hit a circular dependency:
The component body's two contiguous blocks become hooks, called at their original positions so hook order, hook count and dependency arrays are unchanged:
useModelConfigurePopoverfor the configure popover. This stayed a hook rather than becoming a component, because the popover content is auseMemowith a 26-entry dependency array. Turning it into a component would change the memoization semantics.useFieldSlotsfor the per-field slot renderers.PlaygroundConfigSection.tsxis 478 lines.The four public types moved to
types.tsand are re-exported fromPlaygroundConfigSection.tsx, so none of the six call sites in oss, ee and playground-ui changed, and no compatibility shim was left behind.Tests
pnpm --filter @agenta/entity-ui exec tsc --noEmitpasses, as do@agenta/ossand@agenta/ee.pnpm lint-fixacross the repo makes no changes. Prettier clean.Not runtime verified. The gates above are static.
One piece of the original plan is not done here:
useRawEditor(the JSON and YAML serialize-sync effect, roughly 110 lines) stays in the main file. Pulling it out needs about eight parameters, which is not worth it at a 478-line root.What to QA