Fix dynamic playground form state reconciliation#3972
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughArgument handling now reconciles form values against active schemas, including defaults, typed markers, recursive classes, and schema changes. ChangesArgument reconciliation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArgsForm
participant ExecutionPanel
participant reconcileArgs
participant FunctionEncoder
ArgsForm->>ExecutionPanel: submit form arguments
ExecutionPanel->>reconcileArgs: reconcile against active schema
reconcileArgs-->>ExecutionPanel: return reconciled arguments
ExecutionPanel->>FunctionEncoder: encode arguments for execution
FunctionEncoder-->>ExecutionPanel: return execution bytes
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
typescript2/pkg-playground/src/args-form-model.ts (1)
145-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent parameter ordering between
defaultValueandreconcileValue.
defaultValue(…, classPath, aliasPath)orders the two path sets(classPath, aliasPath), whilereconcileValue(…, aliasPath, classPath)(Lines 403-411) uses the opposite(aliasPath, classPath). Every current call site threads them correctly, but two sibling recursive helpers taking the same twoSet<string>args in opposite positions is easy to silently transpose in a future edit (both areSet<string>, so TypeScript won't catch a swap). Consider aligning the order.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/pkg-playground/src/args-form-model.ts` around lines 145 - 151, Align the parameter ordering of defaultValue and reconcileValue so both accept classPath before aliasPath; update their recursive implementations and every call site accordingly, preserving the correct set passed to each parameter.typescript2/pkg-playground/src/ExecutionPanel.tsx (2)
2012-2015: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
argsSchemaKeyto avoid recomputingJSON.stringifyon every render.
argsSchemaKeyis recomputed on every render ofExecutionPanel(it's a plainconst, notuseMemo), even for renders triggered by unrelated state (cursorOffset,highlightedNodeId,workflowCacheVersion, etc.). It stringifiesparamSchemasand the entireprojectTypesmap every time, which is wasted work whenever those references haven't actually changed.♻️ Proposed fix
- const argsSchemaKey = JSON.stringify([ - paramSchemas ?? null, - projectTypes ?? null, - ]); + const argsSchemaKey = useMemo( + () => JSON.stringify([paramSchemas ?? null, projectTypes ?? null]), + [paramSchemas, projectTypes], + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/pkg-playground/src/ExecutionPanel.tsx` around lines 2012 - 2015, Memoize the argsSchemaKey computation in ExecutionPanel using useMemo, with paramSchemas and projectTypes as dependencies, so JSON.stringify only runs when either input reference changes.
2030-2135: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnguarded
reconcileArgscalls could crash rendering/effects if it ever throws.
reconcileArgsis invoked directly with notry/catchin three places: thereconciledFormArgsmemo (Lines 2030-2033), the schema-scoped reconcile effect (Line 2067), andonRunFunction(Line 2105). A throw from any of these (e.g. an unexpected/malformed live-edited schema) would crash the render (memo) or surface as an unhandled effect error, with no fallback to the raw-JSON view. Given schemas here come from a live-editing playground (higher chance of transiently malformed/edge-case shapes), some defensive handling — e.g. falling back to the unreconciled value and surfacing a notice — would improve resilience.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@typescript2/pkg-playground/src/ExecutionPanel.tsx` around lines 2030 - 2135, Guard every reconcileArgs call in reconciledFormArgs, the schema-scoped reconciliation useEffect, and onRunFunction with try/catch. Preserve the original parsed/raw arguments when reconciliation fails, and surface a clear validation or form notice instead of allowing render or effect errors to escape; ensure the run path still submits the unreconciled arguments only when reconciliation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@typescript2/pkg-playground/src/args-form-model.ts`:
- Around line 145-151: Align the parameter ordering of defaultValue and
reconcileValue so both accept classPath before aliasPath; update their recursive
implementations and every call site accordingly, preserving the correct set
passed to each parameter.
In `@typescript2/pkg-playground/src/ExecutionPanel.tsx`:
- Around line 2012-2015: Memoize the argsSchemaKey computation in ExecutionPanel
using useMemo, with paramSchemas and projectTypes as dependencies, so
JSON.stringify only runs when either input reference changes.
- Around line 2030-2135: Guard every reconcileArgs call in reconciledFormArgs,
the schema-scoped reconciliation useEffect, and onRunFunction with try/catch.
Preserve the original parsed/raw arguments when reconciliation fails, and
surface a clear validation or form notice instead of allowing render or effect
errors to escape; ensure the run path still submits the unreconciled arguments
only when reconciliation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa16d0f3-d0a7-4d24-9dd5-5632da6a17f3
📒 Files selected for processing (4)
typescript2/app-vscode-webview/src/ExecutionPanel.strict-mode.test.tsxtypescript2/pkg-playground/src/ExecutionPanel.tsxtypescript2/pkg-playground/src/args-form-model.test.tstypescript2/pkg-playground/src/args-form-model.ts
1 participant
Summary
Root cause
Form seeding and marker normalization were guarded only by the selected function name. When a same-name function changed shape during hot reload, those guards did not run again and widget-local state could survive against the old schema.
Separately, typed controls could display HTML fallback defaults for properties that were not yet present in serialized form state. Running before the hydration effect completed therefore submitted an incomplete object even though the form visibly showed a value.
Validation
pnpm --filter @b/pkg-playground test— 108 tests passedpnpm --filter app-vscode-webview test:unit:run— 15 tests passedpnpm --filter @b/pkg-playground typecheckpnpm --filter app-vscode-webview typecheckpnpm --filter app-vscode-webview buildPersongaining a required fieldfalseSummary by CodeRabbit