added dynamic json form generator for funcion paramters (+ fixed filter)#3326
added dynamic json form generator for funcion paramters (+ fixed filter)#3326rossirpaulo wants to merge 1 commit intocanaryfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
baml_language/crates/baml_compiler2_hir/src/item_tree.rs (2)
463-468:⚠️ Potential issue | 🟡 MinorDrop empty function refs after trimming.
Line 467 can normalize
[],"", or a trailing comma to an empty string and still wrap it inName::new(...). That later leaks a blankfunction_nameinto project metadata instead of treating the entry as absent.Suggested fix
item.value .split(',') - .map(|s| Name::new(s.trim().trim_matches(|c| c == '"' || c == '[' || c == ']').trim())) + .filter_map(|s| { + let name = s + .trim() + .trim_matches(|c| c == '"' || c == '[' || c == ']') + .trim(); + (!name.is_empty()).then(|| Name::new(name)) + }) .collect::<Vec<_>>()
175-183:⚠️ Potential issue | 🟠 Major
Test.argsandraw_args_jsoncan't diverge.
Teststill exposes a structuredargsfield, butalloc_test()now hardcodes it toVec::new()and only preservesraw_args_json. That makes the HIR internally inconsistent and can silently strip test inputs from any downstream path that still readsTest.argsfor validation or execution. Keep populating both representations until all consumers migrate, or removeargsin the same change so the contract break is explicit.Based on learnings: Parser modifications should be coordinated with updates to IR/validation in baml-core, compiler bytecode generation, and VM execution to maintain consistency across the BAML stack.
Also applies to: 471-479
typescript2/pkg-playground/src/ExecutionPanel.tsx (1)
662-668:⚠️ Potential issue | 🟠 MajorKeep
formDatain sync whenRun Testis used.This callback only updates
argsJson. In the default Form mode the editor keeps showing the previous inputs, and the first field edit rebuildsargsJsonfrom stale state instead of from the test case that was just executed. Reuse the same JSON→form sync here thathandleSelectTestalready performs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3302af82-f8ba-41e7-ab8c-6c933f399776
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
baml_language/crates/baml_compiler2_ast/src/ast.rsbaml_language/crates/baml_compiler2_ast/src/lower_cst.rsbaml_language/crates/baml_compiler2_hir/src/item_tree.rsbaml_language/crates/baml_project/src/symbols.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/bex_lsp/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_project/src/lib.rsbaml_language/crates/bridge_wasm/src/wasm_playground.rstypescript2/pkg-playground/src/ExecutionPanel.tsxtypescript2/pkg-playground/src/components/FieldRenderer.tsxtypescript2/pkg-playground/src/components/ParameterForm.tsxtypescript2/pkg-playground/src/worker-protocol.ts
| fn resolve_function_params( | ||
| function_name: &str, | ||
| iface: &baml_compiler2_tir::package_interface::PackageInterface, | ||
| ) -> Vec<crate::bex_lsp::ParamInfo> { | ||
| for ns_funcs in iface.functions.values() { | ||
| if let Some(exported_fn) = ns_funcs.get(&Name::new(function_name)) { | ||
| return exported_fn | ||
| .params | ||
| .iter() | ||
| .map(|(name, ty)| crate::bex_lsp::ParamInfo { | ||
| name: name.to_string(), | ||
| field_type: ty_to_field_type(ty, iface, &mut HashSet::new()), | ||
| }) | ||
| .collect(); | ||
| } | ||
| } | ||
| Vec::new() |
There was a problem hiding this comment.
Resolve params with the fully qualified function symbol.
PackageInterface.functions is namespace-scoped, but this helper scans every namespace and matches only Name::new(function_name). That reintroduces cross-namespace ambiguity: duplicate function names can pick the wrong signature, and qualified names can miss entirely. Carry namespace/qualified-name information through from list_functions_with_metadata and look up that exact entry instead of doing a global bare-name scan.
Based on learnings: bare name lookups intentionally resolve only within the declaring file's local namespace, and reintroducing implicit cross-namespace bare-name fallback is explicitly avoided.
| Ty::Enum(qtn, _) => { | ||
| let enum_name = qtn.name().to_string(); | ||
| let values = find_enum_values(iface, &enum_name); | ||
| crate::bex_lsp::FieldType::Enum { | ||
| name: enum_name, | ||
| values, | ||
| } | ||
| } | ||
|
|
||
| Ty::Class(qtn, _) => { | ||
| let class_name = qtn.name().to_string(); | ||
| if !ancestors.insert(class_name.clone()) { | ||
| // Cycle detected | ||
| return crate::bex_lsp::FieldType::RecursiveRef { name: class_name }; | ||
| } | ||
| let fields = find_class_fields(iface, &class_name) | ||
| .into_iter() | ||
| .map(|(name, field_ty)| crate::bex_lsp::ParamInfo { | ||
| name: name.to_string(), | ||
| field_type: ty_to_field_type(&field_ty, iface, ancestors), | ||
| }) | ||
| .collect(); | ||
| ancestors.remove(&class_name); | ||
| crate::bex_lsp::FieldType::Class { | ||
| name: class_name, | ||
| fields, |
There was a problem hiding this comment.
Preserve the qualified type name when expanding enums and classes.
qtn.name() drops the namespace before both lookup and cycle detection. Two namespaces that each define User or Status can therefore resolve to whichever type iface.types.values() yields first, and unrelated types with the same leaf name can be reported as recursive. Use the qualified type identity for find_enum_values/find_class_fields and for the ancestors key.
Based on learnings: bare name lookups intentionally resolve only within the declaring file's local namespace, and reintroducing implicit cross-namespace bare-name fallback is explicitly avoided.
Also applies to: 830-854
| Ty::Literal(lit, _, _) => { | ||
| let value = match lit { | ||
| baml_base::Literal::String(s) => crate::bex_lsp::LiteralValue::String(s.clone()), | ||
| baml_base::Literal::Int(i) => crate::bex_lsp::LiteralValue::Int(*i), | ||
| baml_base::Literal::Bool(b) => crate::bex_lsp::LiteralValue::Bool(*b), | ||
| baml_base::Literal::Float(_) => { | ||
| // Float literals are rare; treat as Any for now | ||
| return crate::bex_lsp::FieldType::Any; | ||
| } |
There was a problem hiding this comment.
Don't widen float literals to Any.
Ty::Literal(Float) is already a resolved type, but this branch erases it before the playground sees it. Literal-float parameters and unions then lose their fixed-value constraint in the generated form. Please add a float case to LiteralValue/transport instead of falling back to Any here.
| const isEnabled = value !== undefined && value !== null; | ||
| return ( | ||
| <div className="space-y-1"> | ||
| <label className="flex items-center gap-1.5 cursor-pointer"> | ||
| <input | ||
| type="checkbox" | ||
| checked={isEnabled} | ||
| onChange={(e) => { | ||
| if (!e.target.checked) { | ||
| onChange(undefined); | ||
| } else { | ||
| onChange(getDefaultValue(fieldType.inner)); | ||
| } |
There was a problem hiding this comment.
Logic bug with optional null types.
When the inner type is null, enabling the optional sets value to null (via getDefaultValue), but then isEnabled evaluates to false because null !== null fails. This creates a state where the user enables the optional but it immediately appears disabled.
Consider checking only for undefined:
- const isEnabled = value !== undefined && value !== null;
+ const isEnabled = value !== undefined;Alternatively, if you need to distinguish "not set" from "explicitly null", use a sentinel or wrapper object for the optional state.
| const [text, setText] = useState(() => | ||
| value !== undefined ? JSON.stringify(value, null, 2) : '', | ||
| ); | ||
| return ( | ||
| <Textarea | ||
| value={text} | ||
| onChange={(e) => { | ||
| setText(e.target.value); | ||
| try { | ||
| onChange(JSON.parse(e.target.value)); | ||
| } catch { | ||
| // Don't update until valid JSON | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Local state won't sync with external value prop changes.
The text state is initialized from value only on mount. If the parent changes value (e.g., form reset, undo, or external data load), the textarea will show stale content.
Consider using a key prop on the parent to force remount, or sync state when value changes:
♻️ Proposed fix using useEffect to sync
const JsonFallback: FC<{ value: unknown; onChange: (value: unknown) => void }> = ({
value,
onChange,
}) => {
const [text, setText] = useState(() =>
value !== undefined ? JSON.stringify(value, null, 2) : '',
);
+
+ // Sync local state when external value changes
+ const serialized = value !== undefined ? JSON.stringify(value, null, 2) : '';
+ if (text !== serialized && serialized !== '') {
+ // Only sync if external value changed (not from local edits)
+ try {
+ const localParsed = JSON.parse(text);
+ if (JSON.stringify(localParsed) !== JSON.stringify(value)) {
+ setText(serialized);
+ }
+ } catch {
+ // Local text is invalid JSON, don't overwrite user's typing
+ }
+ }Alternatively, a simpler approach is to derive state from props using a key on the component.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [text, setText] = useState(() => | |
| value !== undefined ? JSON.stringify(value, null, 2) : '', | |
| ); | |
| return ( | |
| <Textarea | |
| value={text} | |
| onChange={(e) => { | |
| setText(e.target.value); | |
| try { | |
| onChange(JSON.parse(e.target.value)); | |
| } catch { | |
| // Don't update until valid JSON | |
| } | |
| }} | |
| const [text, setText] = useState(() => | |
| value !== undefined ? JSON.stringify(value, null, 2) : '', | |
| ); | |
| useEffect(() => { | |
| const serialized = value !== undefined ? JSON.stringify(value, null, 2) : ''; | |
| setText(serialized); | |
| }, [value]); | |
| return ( | |
| <Textarea | |
| value={text} | |
| onChange={(e) => { | |
| setText(e.target.value); | |
| try { | |
| onChange(JSON.parse(e.target.value)); | |
| } catch { | |
| // Don't update until valid JSON | |
| } | |
| }} |
| {items.map((item, i) => ( | ||
| <div key={i} className="flex items-start gap-1"> |
There was a problem hiding this comment.
Using array index as key causes state bugs with dynamic lists.
When items are removed, React may incorrectly preserve state from the wrong item because keys shift. This is especially problematic since list items can contain stateful components like JsonFallback.
Consider using a stable identifier. One approach is to wrap items with generated IDs:
♻️ Proposed fix using stable IDs
+import { useRef, useCallback } from 'react';
+
+// Inside ListField:
+const idCounter = useRef(0);
+const [itemsWithIds, setItemsWithIds] = useState<Array<{ id: number; value: unknown }>>(() =>
+ items.map((v) => ({ id: idCounter.current++, value: v }))
+);
+
+// Sync when external items change (simplified - may need refinement)
+// ...
+
{items.map((item, i) => (
- <div key={i} className="flex items-start gap-1">
+ <div key={itemsWithIds[i]?.id ?? i} className="flex items-start gap-1">A simpler alternative: if items are unlikely to have internal state, this may be acceptable with a code comment acknowledging the trade-off.
| onChange={(e) => { | ||
| const newKey = e.target.value; | ||
| const next: Record<string, unknown> = {}; | ||
| for (const [k, v] of entries) { | ||
| next[k === key ? newKey : k] = v; | ||
| } | ||
| onChange(next); | ||
| }} |
There was a problem hiding this comment.
Key rename can silently overwrite existing entries.
If the user renames a key to one that already exists, the existing entry's value is silently lost. For example, renaming "a" to "b" when "b" already exists will drop the original "b" entry.
Consider preventing the rename or warning the user:
🛡️ Proposed fix to prevent collision
onChange={(e) => {
const newKey = e.target.value;
+ // Prevent overwriting existing keys (except self)
+ if (newKey !== key && newKey in obj) {
+ return; // Or show validation error
+ }
const next: Record<string, unknown> = {};
for (const [k, v] of entries) {
next[k === key ? newKey : k] = v;
}
onChange(next);
}}| const onArgsJsonChange = useCallback((e: ChangeEvent<HTMLInputElement>) => { | ||
| setArgsJson(e.target.value); | ||
| try { | ||
| setFormData(JSON.parse(e.target.value)); | ||
| } catch { | ||
| // Don't update formData until valid JSON | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Require object-shaped JSON before syncing it into formData.
These paths accept any valid JSON and assign it to formData. ParameterForm later does value[param.name] and { ...value, [paramName]: ... } in typescript2/pkg-playground/src/components/ParameterForm.tsx Lines 12-14 and Lines 31-37, so null will throw on render and arrays/primitives will corrupt the next edit. Reuse the same object check you already apply before encodeCallArgs.
Possible fix
+const parseArgsObject = (json: string): Record<string, unknown> | null => {
+ try {
+ const parsed: unknown = JSON.parse(json);
+ return typeof parsed === 'object' && parsed !== null && !Array.isArray(parsed)
+ ? (parsed as Record<string, unknown>)
+ : null;
+ } catch {
+ return null;
+ }
+};
+
const onArgsJsonChange = useCallback((e: ChangeEvent<HTMLInputElement>) => {
- setArgsJson(e.target.value);
- try {
- setFormData(JSON.parse(e.target.value));
- } catch {
- // Don't update formData until valid JSON
- }
+ const nextJson = e.target.value;
+ setArgsJson(nextJson);
+ const parsed = parseArgsObject(nextJson);
+ if (parsed !== null) setFormData(parsed);
}, []);
…
const handleSelectTest = useCallback((test: TestInfo) => {
setSelectedFn(test.functionName);
setArgsJson(test.argsJson);
- try {
- setFormData(JSON.parse(test.argsJson));
- } catch {
- setFormData({});
- }
+ setFormData(parseArgsObject(test.argsJson) ?? {});
setActiveTab('run');
}, []);
…
onValueChange={(mode) => {
if (mode === 'form' && inputMode === 'json') {
- try { setFormData(JSON.parse(argsJson)); } catch { /* keep current formData */ }
+ const parsed = parseArgsObject(argsJson);
+ if (parsed !== null) setFormData(parsed);
}
setInputMode(mode);
}}Also applies to: 598-602, 1077-1080
| // Reset form data when function changes — build defaults from schema | ||
| useEffect(() => { | ||
| if (!fnParams.length) { | ||
| setFormData({}); | ||
| setArgsJson('{}'); | ||
| return; | ||
| } | ||
| const defaults: Record<string, unknown> = {}; | ||
| for (const param of fnParams) { | ||
| defaults[param.name] = getDefaultValue(param.fieldType); | ||
| } | ||
| setFormData(defaults); | ||
| setArgsJson(JSON.stringify(defaults)); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [selectedFn]); // Only react to function changes, not param reference changes |
There was a problem hiding this comment.
Don't reset stored test args to schema defaults on function switch.
This effect unconditionally rewrites both formData and argsJson whenever selectedFn changes. If the user selects a test for a different function, the explicit test.argsJson loaded just beforehand is immediately replaced with defaults, so the editor no longer reflects the saved test case.
Merging this PR will not alter performance
|
Binary size checks passed✅ 7 passed
Generated by |
1 participant
Added dynamic json form generator for playground functions and also fixed function list filter.
Summary by CodeRabbit