feat: add core UI components for the prompt repository#1968
Merged
Conversation
This was referenced Mar 6, 2026
Contributor
Author
This was referenced Mar 6, 2026
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (50)
📝 WalkthroughWalkthroughAdds a large set of new UI components (combobox, dropdowns, input primitives, fields, editors, controls, layout utilities) plus backend prompt-repository DB tables, store methods, migrations, and HTTP handlers; also updates package deps and several CodeEditor import paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Combobox as ComboboxSelect
participant State as Query/Selection State
participant Filter as ClientFilter
participant Render as List/Chips Renderer
User->>Combobox: focus / type
Combobox->>State: setQuery(q)
State->>Filter: filter(options, q)
Filter-->>State: filteredOptions
State->>Render: render(filteredOptions, selected)
Render-->>User: show list / chips
User->>Render: click/select item
Render->>Combobox: select(item)
Combobox->>State: updateSelection
Combobox-->>User: emit onChange
sequenceDiagram
participant User
participant SearchInput
participant Searchable as SearchableDropdown
participant Dropdown as CustomDropdown
participant State
User->>SearchInput: type term
SearchInput->>Searchable: setSearchTerm(t)
Searchable->>State: compute groupedFilteredOptions
State-->>Dropdown: render(groups/items)
Dropdown-->>User: show results
User->>Dropdown: navigate/select
Dropdown->>State: update selected / emit onChange
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
impoiler
changed the title
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
e04af20 to
fa0fd1e
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
from
9b02bdb to
34cc0dc
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
2 times, most recently
from
53e58d3 to
60593c9
Compare
|
|
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
60593c9 to
b9a4672
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
2 times, most recently
from
89f39fa to
a5b9469
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
b9a4672 to
5bdfea7
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
from
a5b9469 to
b5581b9
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
5bdfea7 to
6eefddf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (15)
ui/components/ui/custom/modelParameters/textFieldView.tsx-37-43 (1)
37-43:⚠️ Potential issue | 🟡 MinorAvoid mixing
valueanddefaultValueon the same input.Using both
valueanddefaultValueprops simultaneously is problematic. Whenvalueis provided (even asundefined), React treats the input as controlled and ignoresdefaultValue. Ifconfig[field.id]is initiallyundefined, this can cause issues or unexpected behavior.Consider using only controlled mode since you have
onChange:🔧 Proposed fix — use controlled mode with fallback
<Input className="mr-2 ml-auto h-[35px] w-full" - value={config[field.id] as string} + value={(config[field.id] as string) ?? field.default ?? ""} disabled={props.disabled && props.disabled === true} onChange={(e) => props.onChange(e.target.value)} - defaultValue={field.default} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textFieldView.tsx` around lines 37 - 43, The Input is using both value and defaultValue which conflicts; update the component to be purely controlled: remove the defaultValue prop and set value to a safe fallback such as value={(config[field.id] as string) ?? field.default ?? ''}, keep onChange={(e) => props.onChange(e.target.value)} and simplify disabled to disabled={!!props.disabled} so the Input (component named Input in this file) never mixes controlled and uncontrolled modes.ui/components/ui/slider.tsx-47-70 (1)
47-70:⚠️ Potential issue | 🟡 MinorMissing
keyprop on the iterable root element.The static analyzer correctly flags that the
keyshould be on the outermost element in theArray.frommap callback. Currently, thekeyis placed onSliderPrimitive.Thumb(lines 54, 66) but whenthumbTooltipTextis provided, the root element isTooltipProvider, which lacks akey.🔧 Proposed fix
{Array.from({ length: _values.length }, (_, index) => thumbTooltipText ? ( - <TooltipProvider delayDuration={100}> + <TooltipProvider key={index} delayDuration={100}> <Tooltip> <TooltipTrigger asChild> <SliderPrimitive.Thumb data-slot="slider-thumb" - key={index} className="border-primary ring-ring/50 block size-4 shrink-0 rounded-full border bg-white shadow-sm transition-[color,box-shadow] hover:ring-4 focus-visible:ring-4 focus-visible:outline-hidden disabled:pointer-events-none disabled:opacity-50" /> </TooltipTrigger> <TooltipContent className="text-md w-[300px] font-normal"> {thumbTooltipText} </TooltipContent> </Tooltip> </TooltipProvider> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/slider.tsx` around lines 47 - 70, The issue is that the `key` is on the inner `SliderPrimitive.Thumb` but when `thumbTooltipText` is present the outer element is `TooltipProvider` (so the iterator root lacks the `key`); update the map callback so the outermost element in the tooltip branch has `key={index}` (add the key to `TooltipProvider`) and remove or avoid duplicating the key on the inner `SliderPrimitive.Thumb`; keep the existing `key={index}` on `SliderPrimitive.Thumb` in the non-tooltip branch. This ensures the iterable root for `_values.map/Array.from` is keyed correctly (referencing `thumbTooltipText`, `TooltipProvider`, and `SliderPrimitive.Thumb`).ui/components/ui/custom/modelParameters/textArrayFieldView.tsx-143-146 (1)
143-146:⚠️ Potential issue | 🟡 MinorInconsistent boundary check compared to
numberFieldView.This uses
value >= range.max(exclusive upper bound), whilenumberFieldViewusesvalue > range.max(inclusive upper bound). Ensure consistency across validators.Proposed fix (if max should be inclusive)
const isInvalid = (value: number, range: { min: number; max: number }): boolean => { if (!value) return false; - return isNaN(value) || value < range.min || value >= range.max; + return isNaN(value) || value < range.min || value > range.max; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx` around lines 143 - 146, The isInvalid validator in textArrayFieldView.tsx is using an exclusive upper bound (value >= range.max) which is inconsistent with numberFieldView's check (value > range.max); update the isInvalid function to use the same upper-bound comparison as numberFieldView (replace >= with >) so both validators behave consistently, and keep the function name isInvalid as the target for the change.ui/components/ui/custom/modelParameters/paramFieldView.tsx-58-68 (1)
58-68:⚠️ Potential issue | 🟡 Minor
SelectFieldViewis missingforceHideFieldsprop.
SelectFieldViewrenders nestedParameterFieldViewcomponents for subFields (see lines 86-94 in selectFieldView.tsx), butforceHideFieldsis not passed here. This means hide rules won't propagate to nested select fields.Proposed fix
case ParameterType.SELECT: return ( <SelectFieldView field={field} config={config} onChange={props.onChange} multiselect={field.multiple} disabled={props.disabled} className={props.className} + forceHideFields={props.forceHideFields} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/paramFieldView.tsx` around lines 58 - 68, ParameterType.SELECT branch returns SelectFieldView without passing the forceHideFields prop, so nested ParameterFieldView instances inside SelectFieldView won't receive hide rules; update the JSX returned in the ParameterType.SELECT case to pass forceHideFields (e.g., forceHideFields={props.forceHideFields}) into <SelectFieldView> so SelectFieldView can forward it to its nested <ParameterFieldView> components and preserve hide behavior.ui/components/ui/custom/modelParameters/booleanFieldView.tsx-40-57 (1)
40-57:⚠️ Potential issue | 🟡 Minor
field.falseValueis not used when turning off the switch.The
Parametertype definesfalseValuefor custom false values, butonFieldChangealways usesfalseorundefinedwhen turning off. IffalseValueis configured (e.g.,"off"or0), it won't be respected.Proposed fix
+ const falseVal = field.falseValue !== undefined ? field.falseValue : false; + const onFieldChange = (fieldValue: boolean) => { // When turning on => set to trueVal if (fieldValue) { const valToSet = trueVal; const res = field.accesorKey ? { [field.accesorKey]: valToSet } : valToSet; props.onChange(res); return; } // Turning off => either remove the field or set to false depending on config if (field.accesorKey) { // nested field inside parent object if (field.removeFieldOnFalse) { // remove the entire parent object when configured to remove on false props.onChange(undefined); } else { - props.onChange({ [field.accesorKey]: false }); + props.onChange({ [field.accesorKey]: falseVal }); } } else { // top-level if (field.removeFieldOnFalse) { props.onChange(undefined); } else { - props.onChange(false); + props.onChange(falseVal); } } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/booleanFieldView.tsx` around lines 40 - 57, The off-branch currently ignores Parameter.falseValue and always passes false or undefined to props.onChange; update the logic in booleanFieldView (the branch that handles turning off the switch) to use field.falseValue when present (falling back to false) for values and still respect field.removeFieldOnFalse behavior for removing the parent by sending undefined; ensure both nested-case (when field.accesorKey exists) and top-level case use the falseValue fallback so props.onChange receives the configured falseValue instead of a hardcoded false.ui/components/ui/custom/modelParameters/numberFieldView.tsx-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorNon-null assertion on optional
field.rangemay cause runtime issues.
field.rangeis optional per theParametertype, butisInvalidis called withfield.range!unconditionally. Whenfield.rangeis undefined,isInvalidreceives undefined and the validation check at line 94 (range?.min === undefined) returns false, but the intent seems unclear.Consider guarding the call or removing the non-null assertion:
Proposed fix
- const invalid = isInvalid(config[field.id] as number, field.range!, field.id); + const invalid = field.range ? isInvalid(config[field.id] as number, field.range, field.id) : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/numberFieldView.tsx` at line 24, The code currently passes field.range with a non-null assertion to isInvalid (see the invalid assignment and function isInvalid), which can cause runtime issues when field.range is actually undefined; update the call to either pass field.range as-is (remove the !) and let isInvalid handle undefined, or guard the call with a conditional (e.g., only call isInvalid when field.range is present) and adjust handling of the invalid variable accordingly so absent ranges are treated correctly; reference the field.range property on the Parameter, the isInvalid(...) function, and the invalid variable in numberFieldView.tsx to locate and fix the call.ui/components/ui/custom/modelParameters/textArrayFieldView.tsx-57-76 (1)
57-76:⚠️ Potential issue | 🟡 MinorUsing array index as
keycauses issues when deleting items.When an item is deleted, React may incorrectly associate DOM elements with the wrong data because indices shift. This can cause focus/input state to appear on the wrong item.
Consider using a stable identifier or generating unique keys:
Proposed approach using stable IDs
+import { useRef, useCallback } from "react"; + export default function TextArrayFieldView(props: Props) { + const keyCounterRef = useRef(0); + const keysRef = useRef<number[]>([]); + + // Ensure we have keys for all items + while (keysRef.current.length < fieldValue.length) { + keysRef.current.push(keyCounterRef.current++); + } + // ... in map: - <StringInput key={i} ... + <StringInput key={keysRef.current[i]} ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx` around lines 57 - 76, The items are keyed by index which breaks identity on deletes—update the component to use stable IDs for each entry instead of array indices: change the data shape used by modelParameters/textArrayFieldView (and wherever fieldValue is constructed) to an array of objects like {id, value} or attach a generated stable id when adding entries, then render StringInput with key={item.id} (reference StringInput, fieldValue, props.onChange, setShouldFocus, invalid, shouldFocus) and update onChange/onDelete/onEnterPress to map/modify by id rather than by numeric index so keys remain stable when items are removed.ui/components/ui/custom/modelParameters/jsonFieldView.tsx-21-30 (1)
21-30:⚠️ Potential issue | 🟡 MinorPotential render loop due to
valuebeing recomputed each render.
JSON.stringify(rawValue, null, 2)creates a new string reference on every render. TheuseEffectat line 28-30 depends onvalue, so it will callsetCurrentValuerepeatedly ifrawValueis an object that changes reference.Consider memoizing or comparing the serialized value:
Proposed fix
+import { useMemo } from "react"; + export default function JSONFieldView(props: Props) { const { field, parentField, config } = props; const rawValue = parentField ? (config[parentField.id] as any)?.[field.id] ?? field.default : config[field.id] ?? field.default; - const value = JSON.stringify(rawValue, null, 2); + const value = useMemo(() => JSON.stringify(rawValue, null, 2), [rawValue]); const [currentValue, setCurrentValue] = useState<string>(value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/jsonFieldView.tsx` around lines 21 - 30, The effect's dependency on value (which is recomputed via JSON.stringify(rawValue, null, 2) each render) can trigger a render loop; change the component to stabilize the serialized form before using it in useEffect: compute the serialized string with useMemo (or compute a stable key using a deep-equality check) based on rawValue, then use that memoizedSerialized (instead of value) for initializing and as the dependency in useEffect and for setCurrentValue; update references to rawValue, value, JSON.stringify, useMemo, useEffect, and setCurrentValue so the effect only runs when the actual content of rawValue changes.ui/components/ui/custom/dropdown/searchableDropdown.tsx-106-112 (1)
106-112:⚠️ Potential issue | 🟡 MinorSelector
[role="listbox"]won't match any element.The
handleSearchKeyDownattempts to focus an element withrole="listbox", but theCustomDropdowncontainer doesn't have this role (it renders as a plaindiv). This means the focus delegation will silently fail.🔧 Consider using a data attribute or ref instead
If focus delegation is needed, consider:
- Adding
role="listbox"to the CustomDropdown container, or- Using a ref forwarded from CustomDropdown, or
- Using a data attribute like
[data-slot="dropdown-container"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/searchableDropdown.tsx` around lines 106 - 112, handleSearchKeyDown currently queries for '[role="listbox"]' which doesn't exist on the CustomDropdown container, so change focus delegation to a reliable target: either add a data attribute to the container (e.g., data-slot="dropdown-container") and update handleSearchKeyDown to querySelector('[data-slot="dropdown-container"]'), or forward a ref from CustomDropdown (e.g., dropdownContainerRef) and use e.currentTarget.parentElement?.querySelector or the forwarded ref to call focus(); update the CustomDropdown render to include the chosen data attribute or accept/attach the forwarded ref accordingly and use that same identifier in handleSearchKeyDown.ui/components/ui/custom/dropdown/searchableDropdown.tsx-52-100 (1)
52-100:⚠️ Potential issue | 🟡 MinorMissing
removeEmptyGroupsin useMemo dependency array.
removeEmptyGroupsis used inside the memoization callback (Line 69) but is not included in the dependency array (Line 100). This could cause stale filtering behavior when the prop changes.🔧 Proposed fix
- }, [options, searchTerm, hideSelectedValues, value, selectedValuesSafe]); + }, [options, searchTerm, hideSelectedValues, value, selectedValuesSafe, removeEmptyGroups]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/searchableDropdown.tsx` around lines 52 - 100, The memoized filteredOptions calculation in useMemo references removeEmptyGroups inside filterOption but it isn’t included in the dependency array, which can produce stale results; update the dependency array for the useMemo that defines filteredOptions to include removeEmptyGroups (along with the existing dependencies options, searchTerm, hideSelectedValues, value, selectedValuesSafe) so that changes to removeEmptyGroups will recompute the memoized value used by filteredOptions/filterOption/isOptionSelected.ui/components/ui/custom/modelParameters/types.ts-15-21 (1)
15-21:⚠️ Potential issue | 🟡 MinorTypo:
accesorKeyshould beaccessorKey.The property name appears to be misspelled (missing 's'). This will require consumers to use the incorrect spelling, making the API confusing and error-prone.
✏️ Proposed fix
/** * use when value is nested in object * e.g. { config: { response_format: { type: "json_schema", json_schema: { type: "object" } } } } * parameter.id = response_format - * parameter.accesorKey = response_format.json_schema + * parameter.accessorKey = response_format.json_schema */ - accesorKey?: string; + accessorKey?: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/types.ts` around lines 15 - 21, The property name in the model parameter type is misspelled as "accesorKey"; rename it to "accessorKey" in the type declaration (replace "accesorKey?: string;" with "accessorKey?: string;") and then update all usages and references (e.g., any code accessing parameter.accesorKey, prop names, serializers/deserializers, tests, and React props) to use parameter.accessorKey so consumers use the correct spelling consistently.ui/components/ui/splitButton.tsx-141-157 (1)
141-157:⚠️ Potential issue | 🟡 MinorPotential negative width when
contentWidthis small.If the button renders at a width less than 24px,
contentWidth - 24produces a negative value which results in invalid styling. Consider usingMath.max(0, contentWidth - 24)to ensure the width is non-negative.🛡️ Proposed fix
{clicked ? ( <div - style={{ width: contentWidth - 24 }} + style={{ width: Math.max(0, contentWidth - 24) }} className="flex items-center justify-center" > <Check className="h-4 w-4" /> </div> ) : isLoading ? ( <div - style={{ width: contentWidth - 24 }} + style={{ width: Math.max(0, contentWidth - 24) }} className="flex items-center justify-center" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/splitButton.tsx` around lines 141 - 157, The inline style uses contentWidth - 24 which can become negative for small widths; update both places where style={{ width: contentWidth - 24 }} appears in the splitButton component (the clicked and isLoading branches) to clamp the value (e.g., Math.max(0, contentWidth - 24)) so the computed width is never negative while leaving the rest of the rendering logic (clicked, isLoading, children) unchanged.ui/components/ui/custom/dropdown/dropdown.tsx-99-105 (1)
99-105:⚠️ Potential issue | 🟡 MinorUse strict equality and review dependency array.
Line 101 uses loose equality (
!=) which can lead to unexpected type coercion. The eslint-disable comment suggests the dependency array may be incomplete -internalSelectedIndexis used but not listed as a dependency.🔧 Proposed fix
useEffect(() => { // Resetting the selected index if the options change - if (!isControlled && internalSelectedIndex != (selectFirstOptionByDefault ? 0 : -1)) { + if (!isControlled && internalSelectedIndex !== (selectFirstOptionByDefault ? 0 : -1)) { setInternalSelectedIndex(selectFirstOptionByDefault ? 0 : -1); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [flattenedOptions, isControlled, selectFirstOptionByDefault]); + }, [flattenedOptions, isControlled, selectFirstOptionByDefault, internalSelectedIndex]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 99 - 105, In the useEffect that resets selected index, replace the loose inequality check (internalSelectedIndex != ...) with strict !== and add internalSelectedIndex to the effect dependency array so the effect runs when its value changes; update the dependency list [flattenedOptions, isControlled, selectFirstOptionByDefault, internalSelectedIndex] and remove the eslint-disable-next-line comment; ensure you still call setInternalSelectedIndex(selectFirstOptionByDefault ? 0 : -1) when !isControlled && internalSelectedIndex !== (selectFirstOptionByDefault ? 0 : -1).ui/components/ui/custom/richTextarea.tsx-659-681 (1)
659-681:⚠️ Potential issue | 🟡 Minor
noSuggestionTextcan never render.
SuggestionDropdownreturnsnullas soon asfilteredSuggestions.length === 0, soCustomDropdownnever gets a chance to show itsemptyViewText. The dropdown just disappears instead of rendering the configured empty state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/richTextarea.tsx` around lines 659 - 681, The dropdown never renders its empty state because the early-return checks both showDropdown and filteredSuggestions length; remove the length check so only showDropdown gates rendering (i.e., change the early return in the SuggestionDropdown/render block to only return null when !showDropdown or filteredSuggestions is undefined), allowing createPortal to mount CustomDropdown with emptyViewText when filteredSuggestions is an empty array; update the condition around the createPortal call that references showDropdown and filteredSuggestions so CustomDropdown (in createPortal) receives the empty array and can display emptyViewText.ui/components/ui/custom/richTextarea.tsx-105-144 (1)
105-144:⚠️ Potential issue | 🟡 MinorFilter
__descriptionout of the generated options too.
keysexcludesdescription, but not__description. If a node has real children plus a__descriptionhelper field, this code will render a bogus__descriptionsuggestion item. The special-case branch only handles the leaf-node case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/richTextarea.tsx` around lines 105 - 144, The generated options include "__description" because the initial keys list only filters out "description"; update the keys computation to also filter out "__description" (i.e., change Object.keys(current).filter((key) => key !== "description") to exclude "__description" as well) so the map over keys will never produce a "__description" option; keep the existing leaf special-case (keys.length === 1 && keys[0] === "__description") and existing hasChildren/description logic (hasOnlySpecialDescription, hasChildren, description) unchanged.
🧹 Nitpick comments (18)
ui/components/ui/custom/number.tsx (1)
7-7: Use@/lib/utilsalias instead of relative import.Per codebase conventions, UI components should use the
@/lib/*path alias for imports from the lib directory rather than relative paths.♻️ Proposed fix
-import { cn } from "../utils"; +import { cn } from "@/lib/utils";Based on learnings: "In the UI portion of the repo (ui/), import from the lib directory using the path alias '@/lib/*' as configured in tsconfig.json."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/number.tsx` at line 7, Replace the relative import of the utility helper with the path-alias import: change the import of cn (currently from "../utils") to use the project alias (e.g., "@/lib/utils") so the UI component follows the codebase convention; update the import statement that references cn in number.tsx accordingly (search for the import line that imports cn to locate the change).ui/components/ui/custom/modelParameters/textFieldView.tsx (1)
40-40: Simplify the disabled condition.The expression
props.disabled && props.disabled === trueis redundant and can be simplified.♻️ Proposed fix
- disabled={props.disabled && props.disabled === true} + disabled={props.disabled}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textFieldView.tsx` at line 40, The disabled prop check in the TextFieldView component is redundant: replace the expression props.disabled && props.disabled === true with a simple truthy check like !!props.disabled or props.disabled === true; update the disabled assignment in the TextFieldView (the JSX prop using props.disabled) to use one of these simplified forms to make the intent clear and remove unnecessary duplication.ui/components/ui/custom/modelParameters/selectFieldView.tsx (1)
92-92: Redundant boolean check.Same pattern as other files:
props.disabled && props.disabled === truesimplifies toprops.disabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/selectFieldView.tsx` at line 92, The disabled prop uses a redundant boolean check (props.disabled && props.disabled === true); simplify it to just use props.disabled. Locate the JSX in SelectFieldView (the component rendering the select input) and replace the expression for the disabled attribute with props.disabled so the prop is passed directly without the unnecessary && check.ui/components/ui/custom/modelParameters/booleanFieldView.tsx (1)
93-93: Redundant disabled checks.
props.disabled && props.disabled === truesimplifies toprops.disabled.Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/booleanFieldView.tsx` at line 93, In the Boolean field component (booleanFieldView.tsx) simplify the redundant disabled checks by replacing occurrences of "props.disabled && props.disabled === true" with just "props.disabled" (apply the same change wherever that pattern appears, e.g., the second occurrence near the other JSX usage) so the JSX uses the boolean prop directly.ui/components/ui/custom/modelParameters/paramFieldView.tsx (1)
24-74: Consider adding a default case for unsupported field types.If a new
ParameterTypeis added but not handled here, the component silently returnsundefined. A default case could log a warning or return a fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/paramFieldView.tsx` around lines 24 - 74, The getField function currently switches on ParameterType but lacks a default branch, so unhandled/new ParameterType values return undefined; add a default case in getField to handle unsupported types (e.g., log a warning via console.warn or a logger and return a safe fallback such as null or a simple placeholder component) and reference ParameterType, getField, and the component props (e.g., props.onChange) when implementing the fallback so the behavior is consistent with other branches.ui/components/ui/custom/modelParameters/numberFieldView.tsx (2)
56-56: Redundant boolean checks can be simplified.
props.disabled && props.disabled === trueis equivalent to justprops.disabled. This pattern appears multiple times.Proposed fix
- disabled={props.disabled && props.disabled === true} + disabled={props.disabled}Also applies to: 68-68, 80-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/numberFieldView.tsx` at line 56, The disabled prop expressions in numberFieldView.tsx use redundant checks like "props.disabled && props.disabled === true"; replace them with the simpler "props.disabled" (or Boolean(props.disabled) if explicit casting is desired) in the components where it's used (e.g., the JSX elements inside the NumberFieldView component referenced by props.disabled and other occurrences around the current checks at the three locations). Update each instance (the occurrences currently at the other two similar spots) to remove the duplicate boolean comparison so the prop becomes a single truthy check.
26-33: Missing dependencies inuseEffectmay cause stale closures.The effect depends on
props.onInvalidandfield.id, but these are not in the dependency array. This can lead to stale callback references.Proposed fix
useEffect(() => { if (!props.onInvalid) return; if (invalid) { props.onInvalid(true, field.id); } else { props.onInvalid(false, field.id); } - }, [invalid]); + }, [invalid, props.onInvalid, field.id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/numberFieldView.tsx` around lines 26 - 33, The useEffect watching invalid is missing dependencies and can hold stale closures; update the dependency array to include props.onInvalid and field.id (or destructure const { onInvalid } = props and const { id } = field and use those) so the effect reads current values when calling props.onInvalid/onInvalid with field.id; ensure the effect signature still checks for onInvalid before calling.ui/components/ui/custom/modelParameters/textArrayFieldView.tsx (2)
29-36:useEffectdependency array includes entirepropsobject.Including
propsin the dependency array causes the effect to run on any prop change, not justonInvalidchanges. Use specific dependencies.Proposed fix
useEffect(() => { if (!props.onInvalid) return; if (invalid) { props.onInvalid(true, field.id); } else { props.onInvalid(false); } - }, [props, invalid, field.id]); + }, [props.onInvalid, invalid, field.id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx` around lines 29 - 36, The useEffect currently lists the whole props object in its dependency array causing unnecessary runs; update the dependency array to reference only the specific values used: props.onInvalid (or destructured onInvalid), invalid, and field.id. Locate the useEffect in textArrayFieldView.tsx (the one that calls props.onInvalid when invalid changes) and replace props with onInvalid (or destructured onInvalid) so the effect runs only when onInvalid, invalid, or field.id change.
100-100: TODO comment indicates planned refactoring.The comment indicates
StringInputshould be moved to a UI library. Consider tracking this as a follow-up task.Would you like me to open an issue to track moving
StringInputto the shared UI library?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx` at line 100, The inline TODO comment "TODO - `@SURESH` - move this to a UI Library - DO IT" should be replaced with a tracked task rather than a casual note: create a follow-up issue in the project tracker referencing the StringInput component and the ui/components/ui/custom/modelParameters/textArrayFieldView.tsx usage, then update the file to remove or replace the TODO with a concise comment that includes the issue/PR number (e.g., "Follow-up: move StringInput to shared UI lib — issue `#1234`") so the refactor is discoverable; ensure the comment references StringInput and the desire to centralize it in the shared UI library.ui/components/ui/custom/dropdown/types.ts (1)
10-15:Record<string, any>intersection undermines type safety.The generic
Tparameter already allows extendingDropdownItemOptionwith additional properties. The& Record<string, any>intersection defeats TypeScript's excess property checking and allows any property without type constraints.Proposed fix
export type DropdownItemOption<T = {}> = DropdownOptionBase<T> & { type?: "item"; value: string; // Required for items description?: ReactNode; hidden?: boolean; -} & Record<string, any>; +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/types.ts` around lines 10 - 15, The type DropdownItemOption currently intersects with Record<string, any>, which defeats TypeScript excess property checking; remove the unsafe intersection and make the generic T the extension point instead (e.g., constrain T to an object type if needed) so additional properties must be provided via the generic; update the declaration for DropdownItemOption to rely on DropdownOptionBase<T> and T for extensions and remove "& Record<string, any>" so TypeScript enforces proper typing on extra fields.ui/components/ui/custom/modelParameters/index.tsx (1)
49-63: Effect may fire unintentionally ifonChangeis not stable.Including
onChangein the dependency array means this effect will re-run wheneveronChangechanges. If the parent doesn't memoizeonChangewithuseCallback, this could cause unexpected config resets on every parent render.Consider documenting this requirement or using a ref to store onChange:
Alternative approach using ref
+ const onChangeRef = useRef(onChange) + onChangeRef.current = onChange + useEffect(() => { if (parameters.length === 0) return if (prevModelRef.current === model) return prevModelRef.current = model const defaults: Record<string, any> = {} for (const p of parameters) { if (p.default !== undefined) { defaults[p.id] = p.default } } - onChange(defaults) - }, [model, parameters, onChange]) + onChangeRef.current(defaults) + }, [model, parameters])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/index.tsx` around lines 49 - 63, The effect currently depends on onChange which can be unstable and trigger unintended resets; change it to store the latest onChange in a ref (e.g., const onChangeRef = useRef(onChange); useEffect(() => { onChangeRef.current = onChange }, [onChange])) and then remove onChange from the dependency array of the reset effect, calling onChangeRef.current(defaults) instead of onChange; keep prevModelRef, parameters and model in the reset effect's deps so the reset still runs only when the model or parameters actually change.ui/components/ui/custom/modelParameters/jsonFieldView.tsx (1)
53-57: Silent error handling provides no user feedback for invalid JSON.When
JSON.parsefails, the error is silently ignored. Users won't know their JSON is malformed. Consider surfacing validation state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/jsonFieldView.tsx` around lines 53 - 57, The onChange handler currently swallows JSON.parse errors leaving users unaware of invalid JSON; update the handler in jsonFieldView.tsx (the onChange closure that calls setCurrentValue and props.onChange) to only call props.onChange when JSON.parse succeeds, and in the catch block set a validation state (e.g., setIsValidJson or setJsonErrorMessage) so the component can render an inline error/invalid state; do not leave the catch empty and ensure the UI displays the validation error (or exposes it via a props callback) so users get feedback about malformed JSON.ui/components/ui/custom/dropdown/searchableDropdown.tsx (1)
6-6: Use alias import instead of relative path for Icons.Per coding guidelines, prefer
@/...alias imports over relative paths in the UI codebase. Based on learnings: "In the UI codebase, prefer alias imports using@/... over relative imports."♻️ Proposed fix
-import { Icons } from "../../icons"; +import { Icons } from "@/components/ui/icons";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/searchableDropdown.tsx` at line 6, The import for Icons in searchableDropdown.tsx uses a relative path; replace it with the project alias import (use the '@/...' alias) so Icons is imported via the alias rather than "../../icons" (update the import statement that references Icons in searchableDropdown.tsx).ui/components/ui/custom/modelParameters/types.ts (1)
22-22: Consider using more specific types instead ofany.Using
anyfordefaultandvalueproperties reduces type safety. Consider usingunknownfor values that truly can be anything, or a union type if the possible types are known.Also applies to: 47-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/types.ts` at line 22, The type declarations in ui/components/ui/custom/modelParameters/types.ts use `any` for the `default` and `value` properties which weakens type safety; update those properties to use `unknown` (or a specific union of allowed types if you know them) in the interface/type definitions so callers must explicitly narrow or validate values—locate the type(s) that declare `default?: any;` and the corresponding `value` property and replace `any` with `unknown` or an appropriate union (e.g., string | number | boolean | object) and adjust any sites that rely on implicit `any` by adding explicit type guards or casts.ui/components/ui/combobox.tsx (1)
431-501: Consider memoizingComboboxSelectMulti.
ComboboxSelectMultireceives many props and creates a new ref on every render. Since it's only used internally byComboboxSelect, wrapping it withReact.memocould prevent unnecessary re-renders when parent state changes but the relevant props remain the same.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/combobox.tsx` around lines 431 - 501, ComboboxSelectMulti is re-created on every parent render and also recreates anchorRef each time; wrap it with React.memo to avoid unnecessary re-renders when its props haven't changed. Replace the current function declaration with a memoized version (e.g., const ComboboxSelectMulti = React.memo(function ComboboxSelectMulti(...) { ... }) or export/assign React.memo(ComboboxSelectMulti) after the function), keeping the existing anchorRef inside the component and ensure any callbacks passed in (onValueChange, setQuery, handleOpenChange, getLabel) are stable or acceptable to compare shallowly by memo.ui/components/ui/input-group.tsx (1)
11-37: Nestedrole="group"may confuse assistive technologies.
InputGroup(Line 15) hasrole="group"andInputGroupAddon(Line 67) also hasrole="group". Nested groups without clear labeling can make the component hierarchy confusing for screen reader users. Consider removing the role from the addon or usingrole="presentation"if it's purely decorative.Also applies to: 60-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/input-group.tsx` around lines 11 - 37, InputGroup currently sets role="group" on the container (InputGroup) while its child component InputGroupAddon also uses role="group", causing nested unlabeled groups for assistive tech; remove the redundant role from InputGroupAddon (or change it to role="presentation" if it is purely decorative) and keep role="group" only on the main InputGroup (or ensure any group has an accessible name via aria-label/aria-labelledby) so screen readers see a single, properly labeled grouping.ui/components/ui/splitButton.tsx (2)
106-112: Destructuring required prop with fallback to empty object.
dropdownContentis defined as required in the interface (Line 47), but Line 112 uses?? {}which would make all destructured properties undefined if somehow passed as undefined. Since it's required, the fallback is misleading. Consider either making it optional in the interface or removing the fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/splitButton.tsx` around lines 106 - 112, The destructuring of dropdownContent uses a fallback (dropdownContent ?? {}) even though dropdownContent is declared required in the component's props, which is misleading; either remove the fallback so the destructuring directly uses dropdownContent, or update the props/interface to make dropdownContent optional and keep the fallback—modify the interface/prop type where dropdownContent is declared and adjust the destructuring in splitButton.tsx (the const that extracts className: dropdownContentClassName, open: dropdownContentOpen, onOpenChange: dropdownContentOnOpenChange, align: dropdownContentAlign, ...dropdownContentProps) to match the chosen approach.
130-138: Unhandled promise rejection in async onClick.If the user-provided
onClickthrows, the error will become an unhandled rejection. Consider wrapping the call in try/catch to prevent the component from breaking silently when the action fails.♻️ Proposed fix with error handling
- onClick={async (e) => { - if (onClick) { - await onClick(e) - if (!disabledCheck) { - setClicked(true) - setTimeout(() => setClicked(false), 1000) - } - } - }} + onClick={async (e) => { + if (onClick) { + try { + await onClick(e) + if (!disabledCheck) { + setClicked(true) + setTimeout(() => setClicked(false), 1000) + } + } catch (error) { + console.error("SplitButton onClick failed:", error) + } + } + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/splitButton.tsx` around lines 130 - 138, The inline async onClick handler in splitButton.tsx can produce an unhandled promise rejection if the user-provided onClick throws; wrap the await onClick(e) call in a try/catch inside the onClick prop, handle or log the error (e.g., console.error or a provided onError callback) and ensure setClicked(true)/setTimeout(...) only run after a successful onClick (or still run on failure if desired but guarded), referencing the onClick handler, disabledCheck, setClicked and the anonymous async function to locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e51cb9ca-6ca0-4690-a774-48fe573a8cc1
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
ui/components/ui/combobox.tsxui/components/ui/custom/dropdown/dropdown.tsxui/components/ui/custom/dropdown/dropdownGroup.tsxui/components/ui/custom/dropdown/dropdownItem.tsxui/components/ui/custom/dropdown/index.tsxui/components/ui/custom/dropdown/searchableDropdown.tsxui/components/ui/custom/dropdown/types.tsui/components/ui/custom/input.cssui/components/ui/custom/modelParameters/booleanFieldView.tsxui/components/ui/custom/modelParameters/index.tsxui/components/ui/custom/modelParameters/jsonFieldView.tsxui/components/ui/custom/modelParameters/numberFieldView.tsxui/components/ui/custom/modelParameters/paramFieldView.tsxui/components/ui/custom/modelParameters/selectFieldView.tsxui/components/ui/custom/modelParameters/textArrayFieldView.tsxui/components/ui/custom/modelParameters/textFieldView.tsxui/components/ui/custom/modelParameters/types.tsui/components/ui/custom/number.tsxui/components/ui/custom/richTextarea.tsxui/components/ui/input-group.tsxui/components/ui/markdown.tsxui/components/ui/resizable.tsxui/components/ui/scrollArea.tsxui/components/ui/slider.tsxui/components/ui/splitButton.tsxui/package.json
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
6eefddf to
3836523
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
2 times, most recently
from
d377bc1 to
6c32621
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
3 times, most recently
from
5c1b508 to
a2e1b48
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
4d8dbc9 to
41c2700
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
41c2700 to
764e19e
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
from
a64df08 to
41f1894
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
ui/components/ui/input-group.tsx (1)
71-76:⚠️ Potential issue | 🟡 MinorFocus the shared control slot instead of only
<input>.This still skips
InputGroupTextarea, so clicking the addon does nothing for textarea-based groups. Query the shareddata-slot="input-group-control"hook here instead of hard-coding"input".Proposed fix
onClick={(e) => { if ((e.target as HTMLElement).closest("button")) { return } - e.currentTarget.parentElement?.querySelector("input")?.focus() + ;( + e.currentTarget.parentElement?.querySelector( + '[data-slot="input-group-control"]' + ) as HTMLElement | null + )?.focus() }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/input-group.tsx` around lines 71 - 76, The onClick handler in InputGroup currently focuses a hard-coded "input" element which ignores textarea-based groups (e.g., InputGroupTextarea); update the focus selector to query the shared slot attribute instead: look up parentElement?.querySelector('[data-slot="input-group-control"]') and call focus() on that element, preserving the early return when the click originates inside a button (the existing (e.target as HTMLElement).closest("button") check) so clicking addons will correctly focus any control placed in the shared input-group-control slot.ui/components/ui/combobox.tsx (1)
471-483:⚠️ Potential issue | 🟠 MajorDisabled multiselect can still be mutated via chip removal.
disabledonly reachesComboboxChipsInputhere. The selectedComboboxChips still render active remove buttons, so a disabled field can change after selection. WiredisabledintoshowRemove(or an equivalent removable flag) when rendering the chips.Suggested fix
<ComboboxValue> {(selectedValue: string) => ( - <ComboboxChip key={selectedValue}> + <ComboboxChip key={selectedValue} showRemove={!disabled}> {getLabel(selectedValue)} </ComboboxChip> )} </ComboboxValue>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/combobox.tsx` around lines 471 - 483, The chips are still removable when the combobox is disabled because `disabled` is only passed to `ComboboxChipsInput`; update the chip render so the remove control is suppressed when disabled by threading `disabled` into the chip render path (e.g., pass a prop like `showRemove={!disabled && showRemove}` or `removable={!disabled}` into `ComboboxChip` from the `ComboboxValue` render callback) so that `ComboboxChip` can hide/disable its remove button when `disabled` is true; ensure any existing `showRemove` logic is combined with `disabled` to prevent mutation of selections when the control is disabled.ui/components/ui/custom/dropdown/dropdown.tsx (2)
119-120:⚠️ Potential issue | 🟠 MajorExpose listbox semantics on the dropdown container.
DropdownItemalready rendersrole="option", so this wrapper should provide the matchingrole="listbox"and be focusable for list-level interaction. Right now the ARIA structure is incomplete.Suggested fix
- <div className={cn("flex flex-col rounded-md border p-2", className)} ref={containerRef} style={style}> + <div + ref={containerRef} + role="listbox" + tabIndex={-1} + className={cn("flex flex-col rounded-md border p-2", className)} + style={style} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 119 - 120, The dropdown container currently lacks listbox semantics that match DropdownItem's role="option"; update the JSX return div to include role="listbox" and make it keyboard-focusable (e.g., tabIndex={0}) while keeping the existing containerRef, className and style props so the component supports list-level ARIA interactions and focus handling.
9-13:⚠️ Potential issue | 🟠 Major
valueanddefaultValueare still no-ops.The component surface advertises option-based selection, but highlight state is derived only from
selectedIndex. Reopening with an existingvaluestill won’t mark that option as selected, anddefaultValuenever seeds the initial state. Either derive the index from these props or remove them from the public API before more stack layers depend on them.Also applies to: 23-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 9 - 13, The component advertises value/defaultValue but selectedIndex is the source of truth; initialize selectedIndex from props and keep it in sync: when mounting set selectedIndex = indexOf(defaultValue) if value is undefined, and treat value as controlled by updating selectedIndex to indexOf(value) whenever value prop changes; also recompute index when options change (use DropdownOption identity or a stable key) and ensure setSelectedIndex and onChange use the same index-to-option mapping (refer to CustomDropdownProps, value, defaultValue, selectedIndex, options, and any setSelectedIndex/useEffect hooks) so the advertised props actually affect highlighted/selected state.ui/components/ui/custom/modelParameters/textArrayFieldView.tsx (1)
24-27:⚠️ Potential issue | 🟠 MajorThe min/max validation flow still makes some valid states unreachable.
invalidis being used both for validation UI and for add-blocking. That means any field withminElements > 1becomes invalid after the first item and then refuses to add the next one. The helper also treatsmaxElementsas exclusive (>=) instead of inclusive. Split “too few items” validation from “can add another item” gating.Suggested direction
- const invalid = isInvalid(fieldValue.length as number, { - max: (field.array?.maxElements as number) || Infinity, - min: field.array?.minElements || 1, - }); + const minElements = field.array?.minElements ?? 0; + const maxElements = field.array?.maxElements ?? Infinity; + const invalid = fieldValue.length < minElements || fieldValue.length > maxElements; ... - onEnterPress={() => { - if ((fieldValue && !fieldValue[fieldValue.length - 1]) || invalid) return; + onEnterPress={() => { + if (!fieldValue[fieldValue.length - 1]?.trim() || fieldValue.length >= maxElements) return; setShouldFocus(true); props.onChange([...(fieldValue || []), ""]); }} ... - disabled={invalid || props.disabled} + disabled={fieldValue.length >= maxElements || props.disabled} ... const isInvalid = (value: number, range: { min: number; max: number }): boolean => { - if (!value) return false; - return isNaN(value) || value < range.min || value >= range.max; + return Number.isNaN(value) || value < range.min || value > range.max; };Also applies to: 53-57, 70-77, 129-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx` around lines 24 - 27, The current code uses the computed `invalid` (via `isInvalid`) both to render validation UI and to decide whether to allow adding elements, which makes fields with `minElements > 1` block further adds and treats `maxElements` as exclusive; update the logic in `textArrayFieldView.tsx` to separate concerns: keep `isInvalid(fieldValue.length, { min, max })` only for validation/error display, and create a new boolean like `canAddMore` that checks inclusion-aware max: `fieldValue.length < (field.array?.maxElements ?? Infinity)`, and use a separate check for whether adding is allowed (not the validation result). Also ensure min check uses inclusive comparison (`length < min` for invalid) and max uses inclusive comparison (`length >= max` to block adds), and replace uses of `invalid` in add-blocking code paths (the places around the add button and the logic referenced by the earlier diff and the other occurrences you noted) with the new `canAddMore`/explicit comparisons.ui/components/ui/custom/modelParameters/index.tsx (1)
51-57:⚠️ Potential issue | 🟠 MajorSeed the new model config from datasheet defaults instead of always clearing to
{}.This reset drops every
Parameter.default, so default-selected branches and nested config shapes never materialize unless the user touches those fields manually. In the stacked prompt flows that will consume this component next, that means the UI can start from a payload that disagrees with the datasheet contract; initialize fromparametersdefaults here instead of always callingonChange({}), and honoraccesorKeyfor nested defaults when present.As per coding guidelines, "always check the stack if there is one for the current PR ... always see all changes in the light of the whole stack of PRs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/index.tsx` around lines 51 - 57, When the model changes in the useEffect (prevModelRef, useEffect), don't clear state with onChange({}); instead build an initial config by walking the current parameters array and the datasheetModel defaults: for each Parameter in parameters, use Parameter.default when present and, if the parameter has an accessorKey, place the default value into that nested path (honor nested keys) to reconstruct the expected shape; then call onChange(initialConfig). Ensure prevModelRef.current is still updated to model and keep deps (model, datasheetModel, parameters, onChange) unchanged.
🧹 Nitpick comments (2)
ui/components/ui/markdown.tsx (1)
32-44: Consider hoisting constant default values for reference stability.The inline
shikiThemedefault array (line 32) andplugins={{ code }}object (line 44) create new references on each render. IfStreamdownuses shallow prop comparison internally, this could trigger unnecessary re-renders—especially noticeable during streaming when content updates frequently.♻️ Proposed refactor to hoist constants
import { cn } from "@/components/ui/utils"; +const DEFAULT_SHIKI_THEME: StreamdownProps["shikiTheme"] = [ + "github-light", + "github-dark", +]; +const PLUGINS = { code }; + interface MarkdownProps {Then update the component:
function Markdown({ content, className, isStreaming = false, animated = false, caret, controls = true, - shikiTheme = ["github-light", "github-dark"], + shikiTheme = DEFAULT_SHIKI_THEME, components, }: MarkdownProps) { return ( <div className={cn("text-sm text-foreground", className)}> <Streamdown mode={isStreaming ? "streaming" : "static"} isAnimating={isStreaming} animated={animated} caret={caret} controls={controls} shikiTheme={shikiTheme} - plugins={{ code }} + plugins={PLUGINS} components={components} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/markdown.tsx` around lines 32 - 44, Hoist the inline defaults to module-level constants to avoid new references each render: create a SHIKI_THEME_DEFAULT = ["github-light","github-dark"] and a DEFAULT_STREAMDOWN_PLUGINS = { code } (using the existing code symbol) at top of the file, change the Markdown component signature to use shikiTheme = SHIKI_THEME_DEFAULT, and pass plugins={plugins ?? DEFAULT_STREAMDOWN_PLUGINS} into the Streamdown component (keep referring to Streamdown, shikiTheme, plugins and code symbols).ui/components/ui/resizable.tsx (1)
10-23: Consider acceptingorientationas an alias too.Line 10 narrows the wrapper API to
directiononly, but the current shadcn v4 docs/examples useorientationforResizablePanelGroup. Since this file is a base primitive for the rest of the stack, supporting both names now would avoid copy/paste breakage and follow-up churn in the downstream PRs. (ui.shadcn.com)♻️ Suggested compatibility tweak
-type ResizablePanelGroupProps = Omit<GroupProps, 'orientation'> & { - direction?: 'horizontal' | 'vertical' -} +type ResizablePanelGroupProps = Omit<GroupProps, "orientation"> & { + direction?: GroupProps["orientation"] + orientation?: GroupProps["orientation"] +} function ResizablePanelGroup({ className, - direction = 'horizontal', + direction, + orientation, ...props }: ResizablePanelGroupProps) { + const resolvedOrientation = orientation ?? direction ?? "horizontal" + return ( <Group data-slot="resizable-panel-group" - data-panel-group-direction={direction} - orientation={direction} + data-panel-group-direction={resolvedOrientation} + orientation={resolvedOrientation} className={cn( "flex h-full w-full data-[panel-group-direction=vertical]:flex-col", className )} {...props}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/resizable.tsx` around lines 10 - 23, The component API currently only accepts a `direction` prop (ResizablePanelGroupProps and ResizablePanelGroup) but the upstream shadcn examples use `orientation`; update the props and component to accept `orientation` as an alias: extend ResizablePanelGroupProps to allow both `direction` and `orientation`, prefer the explicit prop if both provided, normalize to a single internal variable used for the Group props (orientation attribute and data-panel-group-direction), and pass that normalized value into the Group element (and its data attributes) so either `direction` or `orientation` from callers works interchangeably.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/components/ui/codeEditor.tsx`:
- Around line 24-75: The shared CodeEditorProps surface declares onFocus,
onSave, customLanguage and customCompletions but the editor implementation never
wires them into the Monaco instance; update the CodeEditor component to register
the missing hooks: subscribe the Monaco editor's onDidFocusEditorWidget (or
onDidFocusEditorText) to call props.onFocus, wire a keybinding or editor command
(e.g., Ctrl/Cmd+S) and the editor's onDidSave-like mechanism to call
props.onSave, load and register props.customLanguage with monaco.languages when
provided, and inject props.customCompletions into
monaco.languages.registerCompletionItemProvider for the active language; ensure
these behaviors are tied to the lifecycle methods that create/dispose the editor
so they are removed on unmount.
- Around line 110-120: The current auto-resize logic computes contentHeight from
editor.getContentHeight(), clamps it to props.minHeight/props.maxHeight, then
adds 15px padding which can push the final height past props.maxHeight; change
the order so you compute contentHeight = editor.getContentHeight(), add the
padding (e.g. +15), then clamp the resulting finalHeight between props.minHeight
and props.maxHeight before calling setEditorHeight; update the block around
props.shouldAdjustInitialHeight/props.autoResize that uses
editor.getContentHeight and setEditorHeight to perform padding-first then
clamp-final.
- Around line 212-245: The current beforeMount handler in codeEditor.tsx sets a
no-op global MonacoEnvironment (mutating MonacoEnvironment and disabling
language features); replace this by configuring MonacoEnvironment to return real
worker instances for each label (e.g., "editorWorkerService", "typescript",
"json", "css", "html", etc.) using new Worker(new
URL('monaco-editor/esm/vs/<path>', import.meta.url)) and ensure this setup runs
in a client context (add 'use client' at the top of the module or move
initialization into a client-only module) before the editor mounts (retain
monaco.editor.defineTheme usage), and if using Next.js wrap the editor component
with next/dynamic({ ssr: false }) so workers run in the browser; if you must
keep static export, explicitly document and gate the current no-op worker
fallback so language services are intentionally degraded rather than silently
broken.
---
Duplicate comments:
In `@ui/components/ui/combobox.tsx`:
- Around line 471-483: The chips are still removable when the combobox is
disabled because `disabled` is only passed to `ComboboxChipsInput`; update the
chip render so the remove control is suppressed when disabled by threading
`disabled` into the chip render path (e.g., pass a prop like
`showRemove={!disabled && showRemove}` or `removable={!disabled}` into
`ComboboxChip` from the `ComboboxValue` render callback) so that `ComboboxChip`
can hide/disable its remove button when `disabled` is true; ensure any existing
`showRemove` logic is combined with `disabled` to prevent mutation of selections
when the control is disabled.
In `@ui/components/ui/custom/dropdown/dropdown.tsx`:
- Around line 119-120: The dropdown container currently lacks listbox semantics
that match DropdownItem's role="option"; update the JSX return div to include
role="listbox" and make it keyboard-focusable (e.g., tabIndex={0}) while keeping
the existing containerRef, className and style props so the component supports
list-level ARIA interactions and focus handling.
- Around line 9-13: The component advertises value/defaultValue but
selectedIndex is the source of truth; initialize selectedIndex from props and
keep it in sync: when mounting set selectedIndex = indexOf(defaultValue) if
value is undefined, and treat value as controlled by updating selectedIndex to
indexOf(value) whenever value prop changes; also recompute index when options
change (use DropdownOption identity or a stable key) and ensure setSelectedIndex
and onChange use the same index-to-option mapping (refer to CustomDropdownProps,
value, defaultValue, selectedIndex, options, and any setSelectedIndex/useEffect
hooks) so the advertised props actually affect highlighted/selected state.
In `@ui/components/ui/custom/modelParameters/index.tsx`:
- Around line 51-57: When the model changes in the useEffect (prevModelRef,
useEffect), don't clear state with onChange({}); instead build an initial config
by walking the current parameters array and the datasheetModel defaults: for
each Parameter in parameters, use Parameter.default when present and, if the
parameter has an accessorKey, place the default value into that nested path
(honor nested keys) to reconstruct the expected shape; then call
onChange(initialConfig). Ensure prevModelRef.current is still updated to model
and keep deps (model, datasheetModel, parameters, onChange) unchanged.
In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx`:
- Around line 24-27: The current code uses the computed `invalid` (via
`isInvalid`) both to render validation UI and to decide whether to allow adding
elements, which makes fields with `minElements > 1` block further adds and
treats `maxElements` as exclusive; update the logic in `textArrayFieldView.tsx`
to separate concerns: keep `isInvalid(fieldValue.length, { min, max })` only for
validation/error display, and create a new boolean like `canAddMore` that checks
inclusion-aware max: `fieldValue.length < (field.array?.maxElements ??
Infinity)`, and use a separate check for whether adding is allowed (not the
validation result). Also ensure min check uses inclusive comparison (`length <
min` for invalid) and max uses inclusive comparison (`length >= max` to block
adds), and replace uses of `invalid` in add-blocking code paths (the places
around the add button and the logic referenced by the earlier diff and the other
occurrences you noted) with the new `canAddMore`/explicit comparisons.
In `@ui/components/ui/input-group.tsx`:
- Around line 71-76: The onClick handler in InputGroup currently focuses a
hard-coded "input" element which ignores textarea-based groups (e.g.,
InputGroupTextarea); update the focus selector to query the shared slot
attribute instead: look up
parentElement?.querySelector('[data-slot="input-group-control"]') and call
focus() on that element, preserving the early return when the click originates
inside a button (the existing (e.target as HTMLElement).closest("button") check)
so clicking addons will correctly focus any control placed in the shared
input-group-control slot.
---
Nitpick comments:
In `@ui/components/ui/markdown.tsx`:
- Around line 32-44: Hoist the inline defaults to module-level constants to
avoid new references each render: create a SHIKI_THEME_DEFAULT =
["github-light","github-dark"] and a DEFAULT_STREAMDOWN_PLUGINS = { code }
(using the existing code symbol) at top of the file, change the Markdown
component signature to use shikiTheme = SHIKI_THEME_DEFAULT, and pass
plugins={plugins ?? DEFAULT_STREAMDOWN_PLUGINS} into the Streamdown component
(keep referring to Streamdown, shikiTheme, plugins and code symbols).
In `@ui/components/ui/resizable.tsx`:
- Around line 10-23: The component API currently only accepts a `direction` prop
(ResizablePanelGroupProps and ResizablePanelGroup) but the upstream shadcn
examples use `orientation`; update the props and component to accept
`orientation` as an alias: extend ResizablePanelGroupProps to allow both
`direction` and `orientation`, prefer the explicit prop if both provided,
normalize to a single internal variable used for the Group props (orientation
attribute and data-panel-group-direction), and pass that normalized value into
the Group element (and its data attributes) so either `direction` or
`orientation` from callers works interchangeably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fd5457d3-f469-4e33-b9f9-968b82bc02c3
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
ui/app/workspace/logs/views/codeEditor.tsxui/components/ui/codeEditor.tsxui/components/ui/combobox.tsxui/components/ui/custom/dropdown/dropdown.tsxui/components/ui/custom/dropdown/dropdownGroup.tsxui/components/ui/custom/dropdown/dropdownItem.tsxui/components/ui/custom/dropdown/index.tsxui/components/ui/custom/dropdown/searchableDropdown.tsxui/components/ui/custom/dropdown/types.tsui/components/ui/custom/input.cssui/components/ui/custom/modelParameters/booleanFieldView.tsxui/components/ui/custom/modelParameters/fieldLabel.tsxui/components/ui/custom/modelParameters/index.tsxui/components/ui/custom/modelParameters/jsonFieldView.tsxui/components/ui/custom/modelParameters/numberFieldView.tsxui/components/ui/custom/modelParameters/paramFieldView.tsxui/components/ui/custom/modelParameters/selectFieldView.tsxui/components/ui/custom/modelParameters/textArrayFieldView.tsxui/components/ui/custom/modelParameters/textFieldView.tsxui/components/ui/custom/modelParameters/types.tsui/components/ui/custom/number.tsxui/components/ui/custom/richTextarea.tsxui/components/ui/input-group.tsxui/components/ui/markdown.tsxui/components/ui/mcpToolSelector.tsxui/components/ui/resizable.tsxui/components/ui/scrollArea.tsxui/components/ui/slider.tsxui/components/ui/splitButton.tsxui/package.json
🚧 Files skipped from review as they are similar to previous changes (15)
- ui/components/ui/custom/input.css
- ui/components/ui/custom/dropdown/index.tsx
- ui/package.json
- ui/components/ui/custom/dropdown/dropdownItem.tsx
- ui/components/ui/custom/dropdown/types.ts
- ui/components/ui/custom/modelParameters/fieldLabel.tsx
- ui/components/ui/custom/number.tsx
- ui/components/ui/splitButton.tsx
- ui/components/ui/custom/modelParameters/textFieldView.tsx
- ui/components/ui/custom/modelParameters/paramFieldView.tsx
- ui/components/ui/custom/modelParameters/booleanFieldView.tsx
- ui/components/ui/slider.tsx
- ui/components/ui/custom/modelParameters/jsonFieldView.tsx
- ui/components/ui/custom/richTextarea.tsx
- ui/components/ui/custom/dropdown/searchableDropdown.tsx
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
from
41f1894 to
663ecb4
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
764e19e to
149a57d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
ui/package.json (1)
95-95:⚠️ Potential issue | 🟠 MajorMove
react-textarea-autosizeintodependencies.This PR adds a runtime rich textarea component, so keeping its backing package in
devDependencieswill break installs/builds that omit dev packages.Suggested fix
"dependencies": { + "react-textarea-autosize": "8.5.9", ... }, "devDependencies": { - "react-textarea-autosize": "8.5.9", ... }Read-only verification
#!/bin/bash rg -n \ -g 'ui/**/*.{ts,tsx,js,jsx}' \ -g '!ui/**/*.test.*' \ -g '!ui/**/*.spec.*' \ -g '!ui/**/__tests__/**' \ "from ['\"]react-textarea-autosize['\"]|require\\(['\"]react-textarea-autosize['\"]\\)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/package.json` at line 95, The package "react-textarea-autosize" is currently listed under devDependencies but is required at runtime by the rich textarea component; move the "react-textarea-autosize": "8.5.9" entry from devDependencies into dependencies in package.json, then reinstall/update the lockfile (npm install or yarn install) so the runtime dependency is recorded in package-lock.json/yarn.lock; ensure imports/require of "react-textarea-autosize" in source files remain unchanged.ui/components/ui/codeEditor.tsx (3)
212-245:⚠️ Potential issue | 🟠 MajorDon't replace Monaco workers with no-op stubs.
Returning a fake object from
MonacoEnvironment.getWorkerdisables Monaco's worker-backed language services for every shared editor instance. The documented ESM setup is to return realWorkerinstances fromgetWorker, and@monaco-editor/reactalready treats Monaco access as browser-only in Next.js. (github.com)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/codeEditor.tsx` around lines 212 - 245, The beforeMount block currently replaces MonacoEnvironment.getWorker with a no-op stub which disables Monaco's worker-backed language services; remove the fake getWorker and instead either remove the MonacoEnvironment override entirely or implement the documented ESM approach to return real Worker instances (so language features work), keeping the custom theme via monaco.editor.defineTheme("custom-dark"). Ensure the change is made in the beforeMount callback surrounding MonacoEnvironment.getWorker and monaco.editor.defineTheme so `@monaco-editor/react` can rely on its browser-only handling.
24-75:⚠️ Potential issue | 🟠 MajorThe shared editor API still exposes silent no-op props.
onFocus,onSave,customLanguage, andcustomCompletionsare part of the public contract, but this implementation never registers them with Monaco. Now that the shared editor is being imported across this stack, callers can pass these props and get no behavior back.Also applies to: 104-149, 197-210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/codeEditor.tsx` around lines 24 - 75, The component's public props (onFocus, onSave, customLanguage, customCompletions) are declared in CodeEditorProps but never wired into Monaco; update the component that renders the Monaco editor to: attach onFocus via editor.onDidFocusEditorText and onBlur via editor.onDidBlurEditorText, implement onSave by adding an editor.addAction or addCommand bound to Ctrl/Cmd+S that calls props.onSave, wire customLanguage by registering the language and applying it to the editor model when customLanguage is provided, and register customCompletions by calling monaco.languages.registerCompletionItemProvider for the given language using the customCompletions array to produce CompletionItems; reference the CodeEditorProps interface and the component function that creates the Monaco editor to locate where to add these registrations and dispose handlers on unmount.
109-120:⚠️ Potential issue | 🟡 MinorClamp the final height, not the pre-padding content height.
contentHeightis capped and then15pxis added, somaxHeight={300}still renders at315px.ui/components/ui/mcpToolSelector.tsxalready passesmaxHeight={300}to this shared editor, so this is a live downstream layout bug in the current stack.Suggested fix
const updateHeight = () => { try { - let contentHeight = editor.getContentHeight(); + const chromeOffset = 15; + let contentHeight = editor.getContentHeight() + chromeOffset; if (props.minHeight && contentHeight < props.minHeight) { contentHeight = props.minHeight; } if (props.maxHeight && contentHeight > props.maxHeight) { contentHeight = props.maxHeight; } - setEditorHeight(contentHeight + 15); + setEditorHeight(contentHeight); editor.layout(); } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/codeEditor.tsx` around lines 109 - 120, The auto-resize logic in updateHeight currently clamps contentHeight against props.minHeight/props.maxHeight before adding the 15px padding, causing final height to exceed props.maxHeight (e.g., 300 -> 315). Modify updateHeight (used when props.shouldAdjustInitialHeight || props.autoResize) to add the padding first (let finalHeight = contentHeight + 15) and then clamp finalHeight to props.minHeight and props.maxHeight (or compute allowedContent = clamp(contentHeight, minHeight - 15, maxHeight - 15) and then add padding) and pass the clamped value to setEditorHeight so the visible height respects props.maxHeight.ui/components/ui/input-group.tsx (1)
71-76:⚠️ Potential issue | 🟡 MinorAddon click-to-focus still misses textarea groups.
The handler only looks for
input, so clicking an addon next toInputGroupTextareadoes nothing. Query the shareddata-slot="input-group-control"instead.Suggested fix
onClick={(e) => { if ((e.target as HTMLElement).closest("button")) { return } - e.currentTarget.parentElement?.querySelector("input")?.focus() + ;( + e.currentTarget.parentElement?.querySelector( + '[data-slot="input-group-control"]' + ) as HTMLElement | null + )?.focus() }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/input-group.tsx` around lines 71 - 76, The onClick handler in the InputGroup component currently focuses only an "input" element, which misses textarea controls; update the handler used in the onClick callback (the arrow function that checks (e.target as HTMLElement).closest("button")) to query and focus the shared slot selector '[data-slot="input-group-control"]' instead of "input" so both input and textarea controls inside InputGroup (e.g., InputGroupTextarea) receive focus when clicking an addon.ui/components/ui/custom/dropdown/dropdown.tsx (2)
119-120:⚠️ Potential issue | 🟠 MajorAdd listbox semantics to the container.
The root is still a plain
<div>, so it can't receive focus and there's no semantic container for option-style children. Give it the listbox role and atabIndexso keyboard handoff and ARIA structure are complete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 119 - 120, The root container in dropdown.tsx is a plain div; update the JSX returned by the component (the element using cn(...), ref={containerRef}, style={style}) to include listbox semantics by adding role="listbox" and a keyboard-focusable tabIndex (e.g. tabIndex={0}) so the container can receive focus and serve as the ARIA container for option-style children; keep ref={containerRef}, className and style intact when adding these attributes.
9-37:⚠️ Potential issue | 🟠 Major
valueanddefaultValueare still ignored.Selection is derived only from
selectedIndex, so reopening with a presetvaluewon't highlight anything anddefaultValuenever seeds the initial state. Either map those props to a flattened index or drop them from the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 9 - 37, CustomDropdown currently only uses selectedIndex/internalSelectedIndex so value and defaultValue are ignored; fix by mapping value/defaultValue to the flattened option index: when initializing internalSelectedIndex (in useState) derive the starting index from defaultValue (falling back to selectFirstOptionByDefault) by searching the flattened options for the matching DropdownOption (use whatever identity key the option exposes), and add an effect that, when the value prop changes and selectedIndex is not provided (i.e., isControlled is false), finds the matching option index and calls setInternalSelectedIndex to keep the highlighted item in sync; reference CustomDropdown, CustomDropdownProps, value, defaultValue, selectedIndex, internalSelectedIndex, setInternalSelectedIndex and the flattened options lookup in your changes.ui/components/ui/custom/number.tsx (1)
73-147:⚠️ Potential issue | 🟠 MajorThis primitive can still emit
NaN/ disallowed values.
handleChangeonly applies the generic numeric regex, so non-keyboard paths can still bypassallowDecimal/allowNegative. On blur, partial strings like"-"or"."still flow throughNumber(formattedValue), which producesNaN. Centralize validation so this component only ever callsonChangewithnumber | undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/number.tsx` around lines 73 - 147, The component is still calling onChange with NaN or disallowed values because numeric parsing/validation is split across handleChange and handleBlur; centralize and harden validation in validateAndFormatNumber so it returns either a normalized numeric string or undefined (never a string that produces NaN) and enforces allowDecimal/allowNegative, min/max and decimalPlaces; then update handleChange to call validateAndFormatNumber(newValue) and only call onChange with a number when the validator returns a non-empty numeric string (otherwise call onChange(undefined) or do nothing), and update handleBlur to use validateAndFormatNumber(internalValue) (or fallback logic) so partial inputs like "-" or "." are rejected and never turned into Number(...) resulting in NaN; ensure prevValueRef, setInternalValue and setErrorMessage flows use the validator output so onChange is always invoked with number | undefined.ui/components/ui/custom/modelParameters/paramFieldView.tsx (2)
24-25:⚠️ Potential issue | 🟠 Major
hasValueandonCleardon't account for nested subfield context.When this component is rendered as a subfield (with
parentFieldset), values are stored atconfig[parentField.id][field.id], notconfig[field.id]. The current logic computeshasValueagainst the wrong path, causing the clear button to appear incorrectly andonClearto target the wrong key.🛠️ Proposed fix sketch
-const hasValue = config[field.id] !== undefined; -const onClear = hasValue && !props.disabled ? () => props.onChange(undefined) : undefined; +const rawValue = parentField + ? (config[parentField.id] as Record<string, unknown>)?.[field.id] + : config[field.id]; +const hasValue = rawValue !== undefined; +const onClear = hasValue && !props.disabled ? () => props.onChange(undefined) : undefined;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/paramFieldView.tsx` around lines 24 - 25, The hasValue and onClear logic must use the nested key when parentField is present: compute the currentValue by reading config[field.id] when parentField is falsy, otherwise config[parentField.id]?.[field.id]; set hasValue = currentValue !== undefined; set onClear to undefined when props.disabled or !hasValue, otherwise to a function that calls props.onChange(undefined) for top-level fields or calls props.onChange({...config, [parentField.id]: {...config[parentField.id], [field.id]: undefined}}) (or removes the key) for subfields so the clear action targets the correct nested path; update the code around hasValue and onClear in paramFieldView.tsx to use currentValue, parentField.id, and props.onChange accordingly.
63-74:⚠️ Potential issue | 🟠 Major
SelectFieldViewstill missingforceHideFieldsprop.The previous review flagged that
forceHideFieldsmust be propagated to subviews.BooleanFieldView(line 61) was fixed, butSelectFieldViewstill doesn't receive it. Per the relevant snippet fromselectFieldView.tsx(lines 74-83), nestedParameterFieldViewcalls depend on this prop to hide fields correctly.🐛 Proposed fix
case ParameterType.SELECT: return ( <SelectFieldView field={field} config={config} onChange={props.onChange} multiselect={field.multiple} disabled={props.disabled} onClear={onClear} className={props.className} + forceHideFields={props.forceHideFields} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/paramFieldView.tsx` around lines 63 - 74, The SelectFieldView usage in paramFieldView.tsx is missing the forceHideFields prop, so nested ParameterFieldView instances won't receive it; update the SelectFieldView JSX to pass forceHideFields={props.forceHideFields} (or the relevant variable) along with the other props so SelectFieldView can forward it to its internal ParameterFieldView calls (see SelectFieldView and ParameterFieldView references).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/components/ui/custom/modelParameters/jsonFieldView.tsx`:
- Around line 20-47: The JSON field currently always reads/writes
config[field.id] (or config[parentField.id][field.id]) which breaks consistency
with SelectFieldView; update the read/write path to respect field.accesorKey:
when computing rawValue use field.accesorKey (falling back to
parentField/field.id) to resolve the nested value from config, and in the
onChange/onBlur handlers emit the updated value at that same accessor path (not
the raw parsed object) by merging into the parent object so sibling keys are
preserved; update the handlers around currentValue/setCurrentValue, the
try/catch JSON.parse in onChange, and the onBlur JSON.stringify formatting to
use the accessorKey-aware get/set logic so reads and writes target the exact
same nested key (preserve existing siblings when setting).
In `@ui/components/ui/custom/modelParameters/textFieldView.tsx`:
- Around line 20-28: FieldLabel's htmlFor isn't wired to the Input and the Input
lacks an id, so clicks on the label won't focus the field; update the
TextFieldView to pass a matching id to both components (use a stable identifier
like field.id or a generated id) by supplying htmlFor={field.id} (or the
generated id) to FieldLabel and id={field.id} (or same generated id) to Input,
ensuring onClear/onChange behavior remains unchanged and preserving existing
props.disabled and value handling.
---
Duplicate comments:
In `@ui/components/ui/codeEditor.tsx`:
- Around line 212-245: The beforeMount block currently replaces
MonacoEnvironment.getWorker with a no-op stub which disables Monaco's
worker-backed language services; remove the fake getWorker and instead either
remove the MonacoEnvironment override entirely or implement the documented ESM
approach to return real Worker instances (so language features work), keeping
the custom theme via monaco.editor.defineTheme("custom-dark"). Ensure the change
is made in the beforeMount callback surrounding MonacoEnvironment.getWorker and
monaco.editor.defineTheme so `@monaco-editor/react` can rely on its browser-only
handling.
- Around line 24-75: The component's public props (onFocus, onSave,
customLanguage, customCompletions) are declared in CodeEditorProps but never
wired into Monaco; update the component that renders the Monaco editor to:
attach onFocus via editor.onDidFocusEditorText and onBlur via
editor.onDidBlurEditorText, implement onSave by adding an editor.addAction or
addCommand bound to Ctrl/Cmd+S that calls props.onSave, wire customLanguage by
registering the language and applying it to the editor model when customLanguage
is provided, and register customCompletions by calling
monaco.languages.registerCompletionItemProvider for the given language using the
customCompletions array to produce CompletionItems; reference the
CodeEditorProps interface and the component function that creates the Monaco
editor to locate where to add these registrations and dispose handlers on
unmount.
- Around line 109-120: The auto-resize logic in updateHeight currently clamps
contentHeight against props.minHeight/props.maxHeight before adding the 15px
padding, causing final height to exceed props.maxHeight (e.g., 300 -> 315).
Modify updateHeight (used when props.shouldAdjustInitialHeight ||
props.autoResize) to add the padding first (let finalHeight = contentHeight +
15) and then clamp finalHeight to props.minHeight and props.maxHeight (or
compute allowedContent = clamp(contentHeight, minHeight - 15, maxHeight - 15)
and then add padding) and pass the clamped value to setEditorHeight so the
visible height respects props.maxHeight.
In `@ui/components/ui/custom/dropdown/dropdown.tsx`:
- Around line 119-120: The root container in dropdown.tsx is a plain div; update
the JSX returned by the component (the element using cn(...),
ref={containerRef}, style={style}) to include listbox semantics by adding
role="listbox" and a keyboard-focusable tabIndex (e.g. tabIndex={0}) so the
container can receive focus and serve as the ARIA container for option-style
children; keep ref={containerRef}, className and style intact when adding these
attributes.
- Around line 9-37: CustomDropdown currently only uses
selectedIndex/internalSelectedIndex so value and defaultValue are ignored; fix
by mapping value/defaultValue to the flattened option index: when initializing
internalSelectedIndex (in useState) derive the starting index from defaultValue
(falling back to selectFirstOptionByDefault) by searching the flattened options
for the matching DropdownOption (use whatever identity key the option exposes),
and add an effect that, when the value prop changes and selectedIndex is not
provided (i.e., isControlled is false), finds the matching option index and
calls setInternalSelectedIndex to keep the highlighted item in sync; reference
CustomDropdown, CustomDropdownProps, value, defaultValue, selectedIndex,
internalSelectedIndex, setInternalSelectedIndex and the flattened options lookup
in your changes.
In `@ui/components/ui/custom/modelParameters/paramFieldView.tsx`:
- Around line 24-25: The hasValue and onClear logic must use the nested key when
parentField is present: compute the currentValue by reading config[field.id]
when parentField is falsy, otherwise config[parentField.id]?.[field.id]; set
hasValue = currentValue !== undefined; set onClear to undefined when
props.disabled or !hasValue, otherwise to a function that calls
props.onChange(undefined) for top-level fields or calls
props.onChange({...config, [parentField.id]: {...config[parentField.id],
[field.id]: undefined}}) (or removes the key) for subfields so the clear action
targets the correct nested path; update the code around hasValue and onClear in
paramFieldView.tsx to use currentValue, parentField.id, and props.onChange
accordingly.
- Around line 63-74: The SelectFieldView usage in paramFieldView.tsx is missing
the forceHideFields prop, so nested ParameterFieldView instances won't receive
it; update the SelectFieldView JSX to pass
forceHideFields={props.forceHideFields} (or the relevant variable) along with
the other props so SelectFieldView can forward it to its internal
ParameterFieldView calls (see SelectFieldView and ParameterFieldView
references).
In `@ui/components/ui/custom/number.tsx`:
- Around line 73-147: The component is still calling onChange with NaN or
disallowed values because numeric parsing/validation is split across
handleChange and handleBlur; centralize and harden validation in
validateAndFormatNumber so it returns either a normalized numeric string or
undefined (never a string that produces NaN) and enforces
allowDecimal/allowNegative, min/max and decimalPlaces; then update handleChange
to call validateAndFormatNumber(newValue) and only call onChange with a number
when the validator returns a non-empty numeric string (otherwise call
onChange(undefined) or do nothing), and update handleBlur to use
validateAndFormatNumber(internalValue) (or fallback logic) so partial inputs
like "-" or "." are rejected and never turned into Number(...) resulting in NaN;
ensure prevValueRef, setInternalValue and setErrorMessage flows use the
validator output so onChange is always invoked with number | undefined.
In `@ui/components/ui/input-group.tsx`:
- Around line 71-76: The onClick handler in the InputGroup component currently
focuses only an "input" element, which misses textarea controls; update the
handler used in the onClick callback (the arrow function that checks (e.target
as HTMLElement).closest("button")) to query and focus the shared slot selector
'[data-slot="input-group-control"]' instead of "input" so both input and
textarea controls inside InputGroup (e.g., InputGroupTextarea) receive focus
when clicking an addon.
In `@ui/package.json`:
- Line 95: The package "react-textarea-autosize" is currently listed under
devDependencies but is required at runtime by the rich textarea component; move
the "react-textarea-autosize": "8.5.9" entry from devDependencies into
dependencies in package.json, then reinstall/update the lockfile (npm install or
yarn install) so the runtime dependency is recorded in
package-lock.json/yarn.lock; ensure imports/require of "react-textarea-autosize"
in source files remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62334e8d-f36b-4a59-8547-2597cbe347d8
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
ui/app/workspace/logs/views/codeEditor.tsxui/components/ui/codeEditor.tsxui/components/ui/combobox.tsxui/components/ui/custom/dropdown/dropdown.tsxui/components/ui/custom/dropdown/dropdownGroup.tsxui/components/ui/custom/dropdown/dropdownItem.tsxui/components/ui/custom/dropdown/index.tsxui/components/ui/custom/dropdown/searchableDropdown.tsxui/components/ui/custom/dropdown/types.tsui/components/ui/custom/input.cssui/components/ui/custom/modelParameters/booleanFieldView.tsxui/components/ui/custom/modelParameters/fieldLabel.tsxui/components/ui/custom/modelParameters/index.tsxui/components/ui/custom/modelParameters/jsonFieldView.tsxui/components/ui/custom/modelParameters/numberFieldView.tsxui/components/ui/custom/modelParameters/paramFieldView.tsxui/components/ui/custom/modelParameters/selectFieldView.tsxui/components/ui/custom/modelParameters/textArrayFieldView.tsxui/components/ui/custom/modelParameters/textFieldView.tsxui/components/ui/custom/modelParameters/types.tsui/components/ui/custom/number.tsxui/components/ui/custom/richTextarea.tsxui/components/ui/input-group.tsxui/components/ui/markdown.tsxui/components/ui/mcpToolSelector.tsxui/components/ui/resizable.tsxui/components/ui/scrollArea.tsxui/components/ui/slider.tsxui/components/ui/splitButton.tsxui/package.json
🚧 Files skipped from review as they are similar to previous changes (12)
- ui/components/ui/custom/input.css
- ui/components/ui/custom/richTextarea.tsx
- ui/components/ui/custom/modelParameters/textArrayFieldView.tsx
- ui/components/ui/combobox.tsx
- ui/components/ui/custom/modelParameters/booleanFieldView.tsx
- ui/components/ui/markdown.tsx
- ui/components/ui/custom/dropdown/index.tsx
- ui/components/ui/custom/dropdown/dropdownItem.tsx
- ui/components/ui/custom/modelParameters/fieldLabel.tsx
- ui/components/ui/scrollArea.tsx
- ui/components/ui/custom/modelParameters/index.tsx
- ui/components/ui/custom/dropdown/searchableDropdown.tsx
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
2 times, most recently
from
286f740 to
b0d356f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (8)
ui/package.json (1)
95-95:⚠️ Potential issue | 🟠 MajorMove
react-textarea-autosizeback todependencies.This stack adds a runtime
RichTextareacomponent, so keeping the package indevDependencieswill break production installs that omit dev packages.As per coding guidelines, "always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/package.json` at line 95, The dependency "react-textarea-autosize" is currently listed in devDependencies but is used at runtime by the RichTextarea component; move "react-textarea-autosize" from devDependencies into dependencies in package.json so production installs include it, then run your package manager (npm/yarn/pnpm) to update lockfiles (package-lock.json / yarn.lock) so the change is persisted; ensure the package name "react-textarea-autosize" is only present under "dependencies" and removed from "devDependencies".ui/components/ui/custom/modelParameters/booleanFieldView.tsx (1)
25-32:⚠️ Potential issue | 🟠 MajorHonor
field.defaultwhen the config is still unset.
valueonly reflectsconfig, so a fresh form withdefault: trueordefault: trueValstill renders off and hides any default-on subfields. Initialize the rendered state fromfield.defaultuntil the field has an explicit value.Proposed fix
- let value = false; + let value = field.default === true || field.default === trueVal; if (field.accesorKey) { const parent = config[field.id] as Record<string, unknown> | undefined; - const v = parent ? parent[field.accesorKey] : undefined; - if (v !== undefined) value = v === trueVal; + if (parent && field.accesorKey in parent) { + value = parent[field.accesorKey] === trueVal; + } } else { - if (config[field.id] !== undefined) value = config[field.id] === trueVal; + if (field.id in config) { + value = config[field.id] === trueVal; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/booleanFieldView.tsx` around lines 25 - 32, The rendered boolean value currently only reads from config and ignores field.default; update the initialization in booleanFieldView.tsx so that value is set from field.default when there is no explicit value in config (for both the root key config[field.id] and when using field.accesorKey on the parent object). Concretely, detect whether the config (or parent) actually contains the key (use hasOwnProperty or the in operator) and if not, initialize value = field.default === true || field.default === trueVal; otherwise keep the existing comparison to trueVal; ensure references include field.accesorKey, config[field.id], parent (const parent = config[field.id]), trueVal and field.default.ui/components/ui/custom/modelParameters/textArrayFieldView.tsx (1)
24-27:⚠️ Potential issue | 🟠 MajorDon't use overall invalidity to block adding items.
The current
invalidflag mixes under-min and over-max states, then both the Enter path and the Add button stop working whenever it istrue. WithminElements > 1, the list gets stuck after the first item, andisInvalidalso treatsmaxElementsas exclusive instead of inclusive.Proposed fix
- const invalid = isInvalid(fieldValue.length as number, { - max: (field.array?.maxElements as number) || Infinity, - min: field.array?.minElements || 1, - }); + const minElements = field.array?.minElements ?? 0; + const maxElements = field.array?.maxElements ?? Infinity; + const tooFewItems = fieldValue.length < minElements; + const tooManyItems = fieldValue.length > maxElements; + const invalid = tooFewItems || tooManyItems; @@ onEnterPress={() => { - if ((fieldValue && !fieldValue[fieldValue.length - 1]) || invalid) return; + if (!fieldValue[fieldValue.length - 1]?.trim() || fieldValue.length >= maxElements) return; setShouldFocus(true); props.onChange([...(fieldValue || []), ""]); }} @@ - disabled={invalid || props.disabled} + disabled={fieldValue.length >= maxElements || props.disabled} @@ const isInvalid = (value: number, range: { min: number; max: number }): boolean => { - if (!value) return false; - return isNaN(value) || value < range.min || value >= range.max; + return Number.isNaN(value) || value < range.min || value > range.max; };Also applies to: 53-77, 129-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx` around lines 24 - 27, The current use of isInvalid to gate both adding-via-Enter and the Add button conflates "under min" and "over max" and treats maxElements as exclusive; change the logic to compute two separate flags: one for overMax (use field.array?.maxElements as an inclusive limit: overMax = fieldValue.length >= maxElements) and one for underMin (underMin = fieldValue.length < (field.array?.minElements || 1)) and only disable adding when overMax is true, not when underMin is true; update the places using invalid (the isInvalid call and the handlers that check invalid—e.g., the Enter key handler and the Add button enable/disable logic) to use overMax for blocking adds and keep underMin only for validation messages or submission blocking.ui/components/ui/custom/number.tsx (1)
73-103:⚠️ Potential issue | 🟠 MajorRoute every edit path through the same numeric validator.
validateAndFormatNumberstill ignoresallowDecimal/allowNegative, and the blur/step paths callNumber(...)on partial strings. Inputs like"-"or"."can therefore survive until blur and emitNaN, and arrow stepping can still produce disallowed negatives/decimals. BecauseNumberFieldViewreuses this primitive, invalid numeric state leaks into the model-parameter stack.Proposed fix
const validateAndFormatNumber = useCallback( - (value: string): string => { + (value: string): string | undefined => { if (!value) return ""; + if (!/^-?\d*\.?\d*$/.test(value)) return undefined; + if (!allowNegative && value.includes("-")) return undefined; + if (!allowDecimal && value.includes(".")) return undefined; let formattedValue = value; // Handle decimal places if (decimalPlaces !== undefined && value.includes(".")) { const [whole, decimal] = value.split("."); formattedValue = `${whole}.${decimal.slice(0, decimalPlaces)}`; } const numValue = Number(formattedValue); + if (!Number.isFinite(numValue)) return undefined; // Validate min/max if (min !== undefined && numValue < min) { onValueError?.(`Value cannot be less than ${min}`); setErrorMessage(`Value cannot be less than ${min}`); return min.toString(); @@ - [min, max, decimalPlaces, onValueError], + [min, max, decimalPlaces, allowDecimal, allowNegative, onValueError], ); @@ - if (!/^-?\d*\.?\d*$/.test(newValue)) return; + const normalized = validateAndFormatNumber(newValue); + if (normalized === undefined) return; - setInternalValue(newValue); + setInternalValue(newValue); - // Only call onChange with valid numbers - const parsed = Number(newValue); - if (!isNaN(parsed)) { + const parsed = Number(normalized); + if (Number.isFinite(parsed)) { onChange?.(parsed); } @@ - const formattedValue = validateAndFormatNumber(internalValue); + const formattedValue = validateAndFormatNumber(internalValue); + if (formattedValue === undefined) { + setInternalValue(""); + onChange?.(undefined); + return; + } setInternalValue(formattedValue); if (!errorMessage && !formattedValue) setErrorMessage(`Value cannot be empty, replaced with ${fallbackValue}`); onChange?.(formattedValue ? Number(formattedValue) : fallbackValue); @@ - const newValue = validateAndFormatNumber((currentValue + step).toString()); + const newValue = validateAndFormatNumber((currentValue + step).toString()); + if (newValue === undefined) return; setInternalValue(newValue); onChange?.(Number(newValue)); } else if (e.key === "ArrowDown") { e.preventDefault(); const currentValue = Number(target.value) || 0; - const newValue = validateAndFormatNumber((currentValue - step).toString()); + const newValue = validateAndFormatNumber((currentValue - step).toString()); + if (newValue === undefined) return; setInternalValue(newValue); onChange?.(Number(newValue)); }Also applies to: 105-125, 127-148, 203-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/number.tsx` around lines 73 - 103, validateAndFormatNumber currently converts partial inputs with Number(...) and ignores allowDecimal/allowNegative, letting strings like "-" or "." or disallowed negatives/decimals leak into the model; update validateAndFormatNumber to (1) validate the raw string first against allowDecimal and allowNegative (e.g., reject or normalize lone "-" or "." without calling Number), (2) enforce decimalPlaces by trimming the fractional part only when allowDecimal is true, (3) avoid calling Number on partial invalid strings and instead set error via setErrorMessage and onValueError and return a safe normalized value (e.g., "" or min/max as appropriate), and (4) ensure all edit entry points (blur handler, step/up-down handlers, and NumberFieldView usage) call this single validator so stepping never produces disallowed negatives/decimals; reference validateAndFormatNumber, allowDecimal, allowNegative, decimalPlaces, min, max, onValueError, setErrorMessage, and NumberFieldView when making the changes.ui/components/ui/custom/dropdown/searchableDropdown.tsx (1)
52-100:⚠️ Potential issue | 🟡 Minor
removeEmptyGroupsis missing from the useMemo dependencies.The
filterOptionfunction readsremoveEmptyGroups(line 69), but it's not in the dependency array (line 100). If a parent toggles this prop, the filtered results will be stale until another dependency changes.Suggested fix
- }, [options, searchTerm, hideSelectedValues, value, selectedValuesSafe]); + }, [options, searchTerm, hideSelectedValues, value, selectedValuesSafe, removeEmptyGroups]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/searchableDropdown.tsx` around lines 52 - 100, The useMemo that computes filteredOptions reads the removeEmptyGroups variable inside filterOption but does not include it in the dependency array, which can cause stale filtering when that prop changes; update the dependency array of the useMemo that defines filteredOptions (referencing filteredOptions, filterOption, isOptionSelected) to include removeEmptyGroups alongside options, searchTerm, hideSelectedValues, value, and selectedValuesSafe so the memo recalculates when removeEmptyGroups changes.ui/components/ui/custom/dropdown/dropdown.tsx (2)
119-120:⚠️ Potential issue | 🟠 MajorContainer lacks
role="listbox"for ARIA compliance.
DropdownItemrendersrole="option", but the parent container has no correspondingrole="listbox". This breaks the ARIA ownership model. Additionally,SearchableDropdownqueries for[role="listbox"]which will fail.Suggested fix
- <div className={cn("flex flex-col rounded-md border p-2", className)} ref={containerRef} style={style}> + <div + ref={containerRef} + role="listbox" + tabIndex={-1} + className={cn("flex flex-col rounded-md border p-2", className)} + style={style} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 119 - 120, The container div returned by the Dropdown component is missing role="listbox", breaking ARIA ownership with DropdownItem (which uses role="option") and breaking SearchableDropdown's query for [role="listbox"]; update the JSX in dropdown.tsx (the returned <div ... ref={containerRef} style={style} className={cn(..., className)}>) to include role="listbox" and, if applicable, add any needed aria attributes (e.g., aria-activedescendant or aria-labelledby) to match how SearchableDropdown and DropdownItem manage focus/selection.
9-21:⚠️ Potential issue | 🟠 Major
valueanddefaultValueprops are defined but unused.These props are accepted but never read in the component logic. The selection state is driven only by
selectedIndex/internalSelectedIndex. This creates a misleading API contract.Either:
- Wire
defaultValueto initializeinternalSelectedIndexandvalueto control selection- Remove these props from the interface if not supported
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 9 - 21, The component currently accepts value and defaultValue in CustomDropdownProps but never uses them; update the CustomDropdown component to support both controlled and uncontrolled modes: when value is provided treat the component as controlled and derive the selected index by finding the matching option in options (e.g., options.findIndex(o => o.value === value.value or o === value) and call onChange but do not update internalSelectedIndex directly), when value is undefined use internalSelectedIndex initialized from defaultValue (set internalSelectedIndex = options.findIndex(...) in the component initialization) and keep mutating internalSelectedIndex on user selection; also add an effect to update internalSelectedIndex when defaultValue or options change for uncontrolled mode and ensure selectedIndex prop still overrides internal state when provided.ui/components/ui/input-group.tsx (1)
71-76:⚠️ Potential issue | 🟡 MinorAddon click-to-focus still skips
InputGroupTextarea.This was flagged in a previous review and appears unaddressed. The
querySelector("input")only targets<input>elements, butInputGroupTextarea(line 153) also usesdata-slot="input-group-control". Clicking the addon in a textarea group won't focus the textarea.🐛 Proposed fix using the shared data-slot selector
onClick={(e) => { if ((e.target as HTMLElement).closest("button")) { return } - e.currentTarget.parentElement?.querySelector("input")?.focus() + ;( + e.currentTarget.parentElement?.querySelector( + '[data-slot="input-group-control"]' + ) as HTMLElement | null + )?.focus() }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/input-group.tsx` around lines 71 - 76, The onClick handler in the InputGroup component currently uses querySelector("input") so clicking an addon won't focus InputGroupTextarea; update the handler (the onClick arrow function) to find the control by the shared data-slot attribute instead (e.g., use parentElement?.querySelector('[data-slot="input-group-control"]') and call .focus() on that element) so both <input> and textarea-based controls (InputGroupTextarea) receive focus when the addon is clicked.
🧹 Nitpick comments (3)
ui/components/ui/custom/dropdown/searchableDropdown.tsx (1)
127-141: Consider addingidfor properaria-labelledbyassociation.The search input has
aria-labelbut lacks anid. For more robust accessibility, consider adding anidto the input and usingaria-labelledbyon the dropdown container (once it hasrole="listbox").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/searchableDropdown.tsx` around lines 127 - 141, Add an id to the search input and wire up aria-labelledby on the dropdown listbox: give the input element a stable id (e.g., searchInputId) and set that id as the label reference on the container that has role="listbox" (or add a separate label element and reference it). Update any props/state that generate the placeholder (searchPlaceholder) so the same id is used consistently, and ensure existing handlers (handleSearchChange, handleSearchKeyDown) and className (searchClassName) remain unchanged. This will provide a proper aria association between the listbox and the search input for improved accessibility.ui/components/ui/splitButton.tsx (1)
145-161: Potential negative width whencontentWidthis small.If the button content is narrower than 24px (e.g., a very short label or during initial render before measurement),
contentWidth - 24yields a negative value, which could cause layout issues.♻️ Proposed fix to clamp width
{clicked ? ( <div - style={{ width: contentWidth - 24 }} + style={{ width: Math.max(0, contentWidth - 24) }} className="flex items-center justify-center" > <Check className="h-4 w-4" /> </div> ) : isLoading ? ( <div - style={{ width: contentWidth - 24 }} + style={{ width: Math.max(0, contentWidth - 24) }} className="flex items-center justify-center" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/splitButton.tsx` around lines 145 - 161, The conditional render uses style={{ width: contentWidth - 24 }} which can produce negative widths when contentWidth is small; change the computed width to clamp to a non-negative value (e.g., use Math.max(0, contentWidth - 24)) for the branches that render the icons (the blocks using clicked, isLoading, Check and Loader2) so the inline style never receives a negative width and layout is stable.ui/components/ui/custom/dropdown/dropdown.tsx (1)
99-105: Effect resets selection on everyflattenedOptionschange.The comparison
internalSelectedIndex != (selectFirstOptionByDefault ? 0 : -1)uses loose inequality (!=). More importantly, this effect resets selection wheneverflattenedOptionsidentity changes (which happens on every render due to theuseMemoreturning a new array). This could cause unexpected selection resets.♻️ Consider comparing options length or serialized values
+ const prevOptionsLengthRef = useRef(flattenedOptions.length); useEffect(() => { - // Resetting the selected index if the options change - if (!isControlled && internalSelectedIndex != (selectFirstOptionByDefault ? 0 : -1)) { + // Only reset if options actually changed in count + if (!isControlled && prevOptionsLengthRef.current !== flattenedOptions.length) { + prevOptionsLengthRef.current = flattenedOptions.length; setInternalSelectedIndex(selectFirstOptionByDefault ? 0 : -1); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [flattenedOptions, isControlled, selectFirstOptionByDefault]); + }, [flattenedOptions.length, isControlled, selectFirstOptionByDefault]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/dropdown/dropdown.tsx` around lines 99 - 105, The effect currently resets selection whenever flattenedOptions identity changes and uses loose !=; update the useEffect in dropdown.tsx (the effect that references internalSelectedIndex, flattenedOptions, isControlled, selectFirstOptionByDefault and calls setInternalSelectedIndex) to: 1) use strict comparison !== for internalSelectedIndex versus (selectFirstOptionByDefault ? 0 : -1), and 2) stop depending on flattenedOptions identity by deriving a stable dependency such as flattenedOptions.length or a small serializable key (e.g., a joined string of option IDs/values computed where flattenedOptions is created) so the effect only runs when the actual option set changes rather than on every render. Ensure isControlled remains in deps and update the dependency array accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/components/ui/custom/modelParameters/fieldLabel.tsx`:
- Around line 33-42: The clear Button (rendered when onClear exists) is only
revealed via group-hover/label:opacity-100 which hides it on touch and keyboard
focus; update its visibility classes to also show on keyboard focus and
focus-within by adding something like group-focus-within/label:opacity-100 and
group-focus-visible/label:opacity-100 (and consider a mobile-visible default
such as always-visible on small screens) to the className on the Button
component (the element using props onClear, label, Button and X) so the clear
action is discoverable on touch and when tabbing.
In `@ui/components/ui/custom/modelParameters/numberFieldView.tsx`:
- Around line 38-56: Compute a single resolved numeric value (e.g., const
resolved = (config[field.id] as number) ?? field.default ?? 0) and use that for
both controls so they show the same current value; update NumberInput's value
prop to use the resolved value instead of config[field.id] and update Slider's
value array to [resolved], keeping existing min/max/step/disabled logic on
Slider and placeholder behavior on NumberInput as needed.
In `@ui/components/ui/custom/richTextarea.tsx`:
- Around line 598-613: The render code is causing a side effect by calling
setSelectionIfStillFocused(updatedPosition) during JSX mapping; instead, record
the desired selection position in state (e.g., add a state like pendingSelection
or selectionRequest) when the condition (inlineSuggestionText && part === "{{}}"
&& cursorInsidePart) is met, and remove the direct call from the render path;
then use a useEffect that watches that state plus textareaRef.current and, when
focused, calls setSelectionIfStillFocused(pendingSelection) and clears the
pendingSelection; update references to currentPartPosition,
inlineSuggestionText, textareaRef, and setSelectionIfStillFocused accordingly so
selection is applied after render rather than during render.
- Around line 489-494: The handler handleKeyDownCapture currently checks e.key
=== "Space" which never matches; update the condition to detect the spacebar
properly (e.g., check e.key === " " or use e.code === "Space" or combine both)
so that onKeyDownCapture triggers setSearchText("") and setCurrentPath([]) when
the user presses space; locate handleKeyDownCapture and replace the incorrect
key check with a correct space detection.
- Around line 306-314: The setTimeout callbacks in setSelectionIfStillFocused
(and the other callers handleSuggestionSelect and updateDropdownForChildren) can
run after unmount and access stale refs; add a mounted-ref guard and/or clear
pending timers on unmount: create a mountedRef (e.g., useRef<boolean>) set to
true in useEffect and false in cleanup, store any timeout IDs returned by
setTimeout and clear them in the same cleanup, and inside each timeout callback
check mountedRef.current and the textareaRef before updating
textarea.selectionStart/selectionEnd; apply the same pattern to the other
setTimeout uses so no callback mutates DOM after unmount.
---
Duplicate comments:
In `@ui/components/ui/custom/dropdown/dropdown.tsx`:
- Around line 119-120: The container div returned by the Dropdown component is
missing role="listbox", breaking ARIA ownership with DropdownItem (which uses
role="option") and breaking SearchableDropdown's query for [role="listbox"];
update the JSX in dropdown.tsx (the returned <div ... ref={containerRef}
style={style} className={cn(..., className)}>) to include role="listbox" and, if
applicable, add any needed aria attributes (e.g., aria-activedescendant or
aria-labelledby) to match how SearchableDropdown and DropdownItem manage
focus/selection.
- Around line 9-21: The component currently accepts value and defaultValue in
CustomDropdownProps but never uses them; update the CustomDropdown component to
support both controlled and uncontrolled modes: when value is provided treat the
component as controlled and derive the selected index by finding the matching
option in options (e.g., options.findIndex(o => o.value === value.value or o ===
value) and call onChange but do not update internalSelectedIndex directly), when
value is undefined use internalSelectedIndex initialized from defaultValue (set
internalSelectedIndex = options.findIndex(...) in the component initialization)
and keep mutating internalSelectedIndex on user selection; also add an effect to
update internalSelectedIndex when defaultValue or options change for
uncontrolled mode and ensure selectedIndex prop still overrides internal state
when provided.
In `@ui/components/ui/custom/dropdown/searchableDropdown.tsx`:
- Around line 52-100: The useMemo that computes filteredOptions reads the
removeEmptyGroups variable inside filterOption but does not include it in the
dependency array, which can cause stale filtering when that prop changes; update
the dependency array of the useMemo that defines filteredOptions (referencing
filteredOptions, filterOption, isOptionSelected) to include removeEmptyGroups
alongside options, searchTerm, hideSelectedValues, value, and selectedValuesSafe
so the memo recalculates when removeEmptyGroups changes.
In `@ui/components/ui/custom/modelParameters/booleanFieldView.tsx`:
- Around line 25-32: The rendered boolean value currently only reads from config
and ignores field.default; update the initialization in booleanFieldView.tsx so
that value is set from field.default when there is no explicit value in config
(for both the root key config[field.id] and when using field.accesorKey on the
parent object). Concretely, detect whether the config (or parent) actually
contains the key (use hasOwnProperty or the in operator) and if not, initialize
value = field.default === true || field.default === trueVal; otherwise keep the
existing comparison to trueVal; ensure references include field.accesorKey,
config[field.id], parent (const parent = config[field.id]), trueVal and
field.default.
In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx`:
- Around line 24-27: The current use of isInvalid to gate both adding-via-Enter
and the Add button conflates "under min" and "over max" and treats maxElements
as exclusive; change the logic to compute two separate flags: one for overMax
(use field.array?.maxElements as an inclusive limit: overMax = fieldValue.length
>= maxElements) and one for underMin (underMin = fieldValue.length <
(field.array?.minElements || 1)) and only disable adding when overMax is true,
not when underMin is true; update the places using invalid (the isInvalid call
and the handlers that check invalid—e.g., the Enter key handler and the Add
button enable/disable logic) to use overMax for blocking adds and keep underMin
only for validation messages or submission blocking.
In `@ui/components/ui/custom/number.tsx`:
- Around line 73-103: validateAndFormatNumber currently converts partial inputs
with Number(...) and ignores allowDecimal/allowNegative, letting strings like
"-" or "." or disallowed negatives/decimals leak into the model; update
validateAndFormatNumber to (1) validate the raw string first against
allowDecimal and allowNegative (e.g., reject or normalize lone "-" or "."
without calling Number), (2) enforce decimalPlaces by trimming the fractional
part only when allowDecimal is true, (3) avoid calling Number on partial invalid
strings and instead set error via setErrorMessage and onValueError and return a
safe normalized value (e.g., "" or min/max as appropriate), and (4) ensure all
edit entry points (blur handler, step/up-down handlers, and NumberFieldView
usage) call this single validator so stepping never produces disallowed
negatives/decimals; reference validateAndFormatNumber, allowDecimal,
allowNegative, decimalPlaces, min, max, onValueError, setErrorMessage, and
NumberFieldView when making the changes.
In `@ui/components/ui/input-group.tsx`:
- Around line 71-76: The onClick handler in the InputGroup component currently
uses querySelector("input") so clicking an addon won't focus InputGroupTextarea;
update the handler (the onClick arrow function) to find the control by the
shared data-slot attribute instead (e.g., use
parentElement?.querySelector('[data-slot="input-group-control"]') and call
.focus() on that element) so both <input> and textarea-based controls
(InputGroupTextarea) receive focus when the addon is clicked.
In `@ui/package.json`:
- Line 95: The dependency "react-textarea-autosize" is currently listed in
devDependencies but is used at runtime by the RichTextarea component; move
"react-textarea-autosize" from devDependencies into dependencies in package.json
so production installs include it, then run your package manager (npm/yarn/pnpm)
to update lockfiles (package-lock.json / yarn.lock) so the change is persisted;
ensure the package name "react-textarea-autosize" is only present under
"dependencies" and removed from "devDependencies".
---
Nitpick comments:
In `@ui/components/ui/custom/dropdown/dropdown.tsx`:
- Around line 99-105: The effect currently resets selection whenever
flattenedOptions identity changes and uses loose !=; update the useEffect in
dropdown.tsx (the effect that references internalSelectedIndex,
flattenedOptions, isControlled, selectFirstOptionByDefault and calls
setInternalSelectedIndex) to: 1) use strict comparison !== for
internalSelectedIndex versus (selectFirstOptionByDefault ? 0 : -1), and 2) stop
depending on flattenedOptions identity by deriving a stable dependency such as
flattenedOptions.length or a small serializable key (e.g., a joined string of
option IDs/values computed where flattenedOptions is created) so the effect only
runs when the actual option set changes rather than on every render. Ensure
isControlled remains in deps and update the dependency array accordingly.
In `@ui/components/ui/custom/dropdown/searchableDropdown.tsx`:
- Around line 127-141: Add an id to the search input and wire up aria-labelledby
on the dropdown listbox: give the input element a stable id (e.g.,
searchInputId) and set that id as the label reference on the container that has
role="listbox" (or add a separate label element and reference it). Update any
props/state that generate the placeholder (searchPlaceholder) so the same id is
used consistently, and ensure existing handlers (handleSearchChange,
handleSearchKeyDown) and className (searchClassName) remain unchanged. This will
provide a proper aria association between the listbox and the search input for
improved accessibility.
In `@ui/components/ui/splitButton.tsx`:
- Around line 145-161: The conditional render uses style={{ width: contentWidth
- 24 }} which can produce negative widths when contentWidth is small; change the
computed width to clamp to a non-negative value (e.g., use Math.max(0,
contentWidth - 24)) for the branches that render the icons (the blocks using
clicked, isLoading, Check and Loader2) so the inline style never receives a
negative width and layout is stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3ad9ecf-d922-40d2-951f-be9717086133
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
ui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/logs/views/emptyState.tsxui/app/workspace/logs/views/logChatMessageView.tsxui/app/workspace/logs/views/logResponsesMessageView.tsxui/app/workspace/logs/views/transcriptionView.tsxui/app/workspace/logs/views/videoView.tsxui/app/workspace/mcp-logs/views/emptyState.tsxui/app/workspace/mcp-logs/views/mcpLogDetailsSheet.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/plugins/fragments/pluginFormFragments.tsxui/app/workspace/plugins/views/pluginsView.tsxui/components/ui/codeEditor.tsxui/components/ui/combobox.tsxui/components/ui/custom/dropdown/dropdown.tsxui/components/ui/custom/dropdown/dropdownGroup.tsxui/components/ui/custom/dropdown/dropdownItem.tsxui/components/ui/custom/dropdown/index.tsxui/components/ui/custom/dropdown/searchableDropdown.tsxui/components/ui/custom/dropdown/types.tsui/components/ui/custom/input.cssui/components/ui/custom/modelParameters/booleanFieldView.tsxui/components/ui/custom/modelParameters/fieldLabel.tsxui/components/ui/custom/modelParameters/index.tsxui/components/ui/custom/modelParameters/jsonFieldView.tsxui/components/ui/custom/modelParameters/numberFieldView.tsxui/components/ui/custom/modelParameters/paramFieldView.tsxui/components/ui/custom/modelParameters/selectFieldView.tsxui/components/ui/custom/modelParameters/textArrayFieldView.tsxui/components/ui/custom/modelParameters/textFieldView.tsxui/components/ui/custom/modelParameters/types.tsui/components/ui/custom/number.tsxui/components/ui/custom/richTextarea.tsxui/components/ui/input-group.tsxui/components/ui/markdown.tsxui/components/ui/mcpToolSelector.tsxui/components/ui/resizable.tsxui/components/ui/scrollArea.tsxui/components/ui/slider.tsxui/components/ui/splitButton.tsxui/package.json
✅ Files skipped from review due to trivial changes (2)
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- ui/app/workspace/logs/views/emptyState.tsx
🚧 Files skipped from review as they are similar to previous changes (14)
- ui/components/ui/slider.tsx
- ui/components/ui/custom/modelParameters/paramFieldView.tsx
- ui/components/ui/mcpToolSelector.tsx
- ui/components/ui/custom/dropdown/dropdownItem.tsx
- ui/components/ui/custom/dropdown/dropdownGroup.tsx
- ui/components/ui/custom/dropdown/index.tsx
- ui/components/ui/custom/modelParameters/selectFieldView.tsx
- ui/components/ui/scrollArea.tsx
- ui/components/ui/resizable.tsx
- ui/components/ui/custom/modelParameters/textFieldView.tsx
- ui/components/ui/custom/dropdown/types.ts
- ui/components/ui/custom/modelParameters/types.ts
- ui/components/ui/combobox.tsx
- ui/components/ui/custom/modelParameters/jsonFieldView.tsx
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
from
663ecb4 to
30f4c4e
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
b0d356f to
0c30125
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
from
30f4c4e to
23525c8
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
2 times, most recently
from
5256479 to
01a6bfc
Compare
Contributor
Merge activity
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (7)
ui/components/ui/custom/richTextarea.tsx (2)
659-681:⚠️ Potential issue | 🟡 MinorLet the empty-state dropdown render.
This early return hides the portal when
filteredSuggestionsis an empty array, soemptyViewText={noSuggestionText}below never renders. Returnnullonly when suggestions areundefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/richTextarea.tsx` around lines 659 - 681, The current guard `if (!showDropdown || !filteredSuggestions || !filteredSuggestions.length) return null;` prevents the portal and CustomDropdown from rendering when filteredSuggestions is an empty array so emptyViewText (noSuggestionText) never appears; change the check to only return null when filteredSuggestions is null/undefined (e.g., `if (!showDropdown || filteredSuggestions == null) return null;`) so createPortal and CustomDropdown (with emptyViewText) still render for an empty array.
547-559:⚠️ Potential issue | 🟠 MajorDon't drop the standard
onPasteprop.Filtering
onPasteout ofpropsmeans consumers can never receive paste events fromRichTextarea, even whenonFilePastedis unset. Since this component exposes the textarea prop surface, it should compose the file handler withprops.onPasteinstead of discarding one side.Suggested fix
- <TextareaAutosize - onPaste={props.onFilePasted ? filePasteHandler : undefined} + <TextareaAutosize + onPaste={(e) => { + if (props.onFilePasted) { + filePasteHandler(e); + } + props.onPaste?.(e); + }} ref={textareaRef} value={value} @@ - {...Object.fromEntries(Object.entries(props).filter(([key]) => !["onFilePasted", "style", "onPaste"].includes(key)))} + {...Object.fromEntries(Object.entries(props).filter(([key]) => !["onFilePasted", "style", "onPaste"].includes(key)))} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/richTextarea.tsx` around lines 547 - 559, The component currently removes the standard onPaste prop when spreading props into TextareaAutosize; update TextareaAutosize usage (the JSX element with ref textareaRef and handlers) to compose the paste handlers instead of dropping props.onPaste: have the element call a combined handler that first invokes filePasteHandler when props.onFilePasted is set (or always handles file paste logic) and then calls props.onPaste (if present), and stop filtering "onPaste" out in the Object.fromEntries filter so the original props.onPaste is preserved and invoked after/alongside filePasteHandler.ui/components/ui/input-group.tsx (1)
71-76:⚠️ Potential issue | 🟡 MinorFocus the shared input-group control, not just
<input>.
InputGroupAddonalready supportsInputGroupTextarea, but this click-to-focus path only queries"input", so addons stop working for textarea groups. Target[data-slot="input-group-control"]instead.Suggested fix
onClick={(e) => { if ((e.target as HTMLElement).closest("button")) { return } - e.currentTarget.parentElement?.querySelector("input")?.focus() + ;( + e.currentTarget.parentElement?.querySelector( + '[data-slot="input-group-control"]' + ) as HTMLElement | null + )?.focus() }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/input-group.tsx` around lines 71 - 76, The click handler on the InputGroup root only focuses the first "input" element, which breaks Addon controls that render a textarea; update the handler (the onClick defined in ui/components/ui/input-group.tsx) to query and focus the element with data-slot="input-group-control" instead of "input" so both InputGroupAddon and InputGroupTextarea receive focus; ensure you still guard clicks that originate from buttons (the existing closest("button") check) and call .focus() on the found element if present.ui/components/ui/custom/modelParameters/numberFieldView.tsx (1)
37-56:⚠️ Potential issue | 🟠 MajorUse one resolved value for both controls.
When
config[field.id]is unset butfield.defaultexists, the slider renders the default while the number input stays empty. These two controls show different current values for the same field until the user edits them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/numberFieldView.tsx` around lines 37 - 56, Compute a single resolved numeric value (e.g., const resolved = (config[field.id] as number) ?? field.default ?? 0) and use that for both controls so they show the same current value: replace the NumberInput value and the Slider value array with this resolved value and keep existing onChange handlers (props.onChange) to update config[field.id]; ensure min/max/step remain unchanged and disabled logic stays the same.ui/components/ui/custom/modelParameters/textArrayFieldView.tsx (1)
24-27:⚠️ Potential issue | 🟠 MajorMin/max validation still blocks valid adds.
invalidis doing double duty as both the validation state and the add guard. WithminElements > 1, the first item immediately makes the field invalid and disables the Enter/add-button paths, so the user can never reach the minimum.isInvalidalso treatslength === maxElementsas invalid because it uses>= max.Also applies to: 53-57, 70-77, 129-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx` around lines 24 - 27, The current code uses the single `invalid` result from `isInvalid(fieldValue.length, { max: field.array?.maxElements || Infinity, min: field.array?.minElements || 1 })` both to show validation errors and to guard the Add/Enter actions, which blocks adding when length < min and also treats length === max as an add-block; instead, split concerns: keep `displayInvalid` (or keep `invalid`) for rendering validation using `isInvalid(...)`, but introduce an explicit `canAdd` check used by add/Enter handlers and the Add button that only prevents adding when `fieldValue.length >= (field.array?.maxElements ?? Infinity)`; update all places that currently use `invalid` to disable/add behavior (including the handlers and buttons referenced near lines 24-27, 53-57, 70-77, 129-132) to use `canAdd` while leaving `displayInvalid` for UI validation messages.ui/components/ui/custom/modelParameters/booleanFieldView.tsx (1)
25-32:⚠️ Potential issue | 🟡 Minor
field.defaultis not used when config value is undefined.When a field has
default: truebut no value exists inconfig, the switch will render unchecked becausevaluestaysfalse. The past review suggested falling back tofield.defaultwhen the config value is undefined.Proposed fix
- let value = false; + let value = field.default === true || field.default === trueVal; if (field.accesorKey) { const parent = config[field.id] as Record<string, unknown> | undefined; const v = parent ? parent[field.accesorKey] : undefined; - if (v !== undefined) value = v === trueVal; + if (v !== undefined) { + value = v === trueVal; + } } else { - if (config[field.id] !== undefined) value = config[field.id] === trueVal; + if (config[field.id] !== undefined) { + value = config[field.id] === trueVal; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/booleanFieldView.tsx` around lines 25 - 32, The value fallback logic in booleanFieldView.tsx currently initializes value=false and only sets it if a config entry exists, ignoring field.default; update the branches that compute value (the block using field.accesorKey and the else branch checking config[field.id]) to treat undefined config as "use field.default" — i.e., if parent or config[field.id] is undefined, set value = field.default === trueVal (or simply field.default if trueVal is boolean), otherwise derive value from the config as currently implemented; ensure you still respect field.accesorKey when reading from parent.ui/components/ui/custom/modelParameters/paramFieldView.tsx (1)
63-74:⚠️ Potential issue | 🟡 Minor
forceHideFieldsnot passed toSelectFieldView.The
BooleanFieldViewat line 61 correctly receivesforceHideFields, butSelectFieldViewdoes not. SinceSelectFieldViewalso renders nestedParameterFieldViewcomponents for subFields (as seen in the selectFieldView.tsx code at lines 73-87), those nested fields won't honor the hide list.Proposed fix
case ParameterType.SELECT: return ( <SelectFieldView field={field} config={config} onChange={props.onChange} multiselect={field.multiple} disabled={props.disabled} onClear={onClear} className={props.className} + forceHideFields={props.forceHideFields} /> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/paramFieldView.tsx` around lines 63 - 74, The SelectFieldView is missing the forceHideFields prop so nested ParameterFieldView instances won't respect the hide list; update the call site in the switch case for ParameterType.SELECT to pass props.forceHideFields into <SelectFieldView ... />, and ensure SelectFieldView (in selectFieldView.tsx) accepts and forwards that prop down to the nested ParameterFieldView renders so hidden fields are honored (mirror how BooleanFieldView receives forceHideFields).
🧹 Nitpick comments (2)
ui/components/ui/markdown.tsx (1)
6-6: Use the shared@/lib/utilshelper here.This is the only new UI primitive in the stack pulling
cnfrom a different module, which makes the utility source inconsistent with the rest of the PR. Aligning this to@/lib/utilswill keep the foundational components uniform before downstream PRs consume them.Based on learnings, "In the UI codebase, prefer alias imports using
@/... (e.g.,@/lib/store,@/lib/types/config,@/components/ui/button) over relative imports."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/markdown.tsx` at line 6, Replace the import of the classnames helper so this component uses the shared utility: change the import source for the cn symbol in ui/components/ui/markdown.tsx from "@/components/ui/utils" to "@/lib/utils" (i.e., update the import that brings in cn to reference the shared helper) so the UI primitives all use the same alias-based utils module.ui/components/ui/custom/modelParameters/selectFieldView.tsx (1)
82-82: Simplify redundant disabled check.The expression
props.disabled && props.disabled === trueis equivalent to justprops.disabledsince it's already a boolean. This pattern also appears in other field views.Proposed fix
- disabled={props.disabled && props.disabled === true} + disabled={props.disabled}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/components/ui/custom/modelParameters/selectFieldView.tsx` at line 82, The disabled prop check in selectFieldView.tsx is redundant: replace occurrences of the expression props.disabled && props.disabled === true with the simpler props.disabled; update the SelectFieldView (and other field view components that use the same pattern) to use direct truthy boolean checks on props.disabled (e.g., disabled={props.disabled}) to remove unnecessary duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/components/ui/custom/modelParameters/booleanFieldView.tsx`:
- Around line 25-32: The value fallback logic in booleanFieldView.tsx currently
initializes value=false and only sets it if a config entry exists, ignoring
field.default; update the branches that compute value (the block using
field.accesorKey and the else branch checking config[field.id]) to treat
undefined config as "use field.default" — i.e., if parent or config[field.id] is
undefined, set value = field.default === trueVal (or simply field.default if
trueVal is boolean), otherwise derive value from the config as currently
implemented; ensure you still respect field.accesorKey when reading from parent.
In `@ui/components/ui/custom/modelParameters/numberFieldView.tsx`:
- Around line 37-56: Compute a single resolved numeric value (e.g., const
resolved = (config[field.id] as number) ?? field.default ?? 0) and use that for
both controls so they show the same current value: replace the NumberInput value
and the Slider value array with this resolved value and keep existing onChange
handlers (props.onChange) to update config[field.id]; ensure min/max/step remain
unchanged and disabled logic stays the same.
In `@ui/components/ui/custom/modelParameters/paramFieldView.tsx`:
- Around line 63-74: The SelectFieldView is missing the forceHideFields prop so
nested ParameterFieldView instances won't respect the hide list; update the call
site in the switch case for ParameterType.SELECT to pass props.forceHideFields
into <SelectFieldView ... />, and ensure SelectFieldView (in
selectFieldView.tsx) accepts and forwards that prop down to the nested
ParameterFieldView renders so hidden fields are honored (mirror how
BooleanFieldView receives forceHideFields).
In `@ui/components/ui/custom/modelParameters/textArrayFieldView.tsx`:
- Around line 24-27: The current code uses the single `invalid` result from
`isInvalid(fieldValue.length, { max: field.array?.maxElements || Infinity, min:
field.array?.minElements || 1 })` both to show validation errors and to guard
the Add/Enter actions, which blocks adding when length < min and also treats
length === max as an add-block; instead, split concerns: keep `displayInvalid`
(or keep `invalid`) for rendering validation using `isInvalid(...)`, but
introduce an explicit `canAdd` check used by add/Enter handlers and the Add
button that only prevents adding when `fieldValue.length >=
(field.array?.maxElements ?? Infinity)`; update all places that currently use
`invalid` to disable/add behavior (including the handlers and buttons referenced
near lines 24-27, 53-57, 70-77, 129-132) to use `canAdd` while leaving
`displayInvalid` for UI validation messages.
In `@ui/components/ui/custom/richTextarea.tsx`:
- Around line 659-681: The current guard `if (!showDropdown ||
!filteredSuggestions || !filteredSuggestions.length) return null;` prevents the
portal and CustomDropdown from rendering when filteredSuggestions is an empty
array so emptyViewText (noSuggestionText) never appears; change the check to
only return null when filteredSuggestions is null/undefined (e.g., `if
(!showDropdown || filteredSuggestions == null) return null;`) so createPortal
and CustomDropdown (with emptyViewText) still render for an empty array.
- Around line 547-559: The component currently removes the standard onPaste prop
when spreading props into TextareaAutosize; update TextareaAutosize usage (the
JSX element with ref textareaRef and handlers) to compose the paste handlers
instead of dropping props.onPaste: have the element call a combined handler that
first invokes filePasteHandler when props.onFilePasted is set (or always handles
file paste logic) and then calls props.onPaste (if present), and stop filtering
"onPaste" out in the Object.fromEntries filter so the original props.onPaste is
preserved and invoked after/alongside filePasteHandler.
In `@ui/components/ui/input-group.tsx`:
- Around line 71-76: The click handler on the InputGroup root only focuses the
first "input" element, which breaks Addon controls that render a textarea;
update the handler (the onClick defined in ui/components/ui/input-group.tsx) to
query and focus the element with data-slot="input-group-control" instead of
"input" so both InputGroupAddon and InputGroupTextarea receive focus; ensure you
still guard clicks that originate from buttons (the existing closest("button")
check) and call .focus() on the found element if present.
---
Nitpick comments:
In `@ui/components/ui/custom/modelParameters/selectFieldView.tsx`:
- Line 82: The disabled prop check in selectFieldView.tsx is redundant: replace
occurrences of the expression props.disabled && props.disabled === true with the
simpler props.disabled; update the SelectFieldView (and other field view
components that use the same pattern) to use direct truthy boolean checks on
props.disabled (e.g., disabled={props.disabled}) to remove unnecessary
duplication.
In `@ui/components/ui/markdown.tsx`:
- Line 6: Replace the import of the classnames helper so this component uses the
shared utility: change the import source for the cn symbol in
ui/components/ui/markdown.tsx from "@/components/ui/utils" to "@/lib/utils"
(i.e., update the import that brings in cn to reference the shared helper) so
the UI primitives all use the same alias-based utils module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 067c613c-1dcd-443d-a080-dea57079e3f3
⛔ Files ignored due to path filters (1)
ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
ui/app/workspace/logs/sheets/logDetailsSheet.tsxui/app/workspace/logs/views/emptyState.tsxui/app/workspace/logs/views/logChatMessageView.tsxui/app/workspace/logs/views/logResponsesMessageView.tsxui/app/workspace/logs/views/transcriptionView.tsxui/app/workspace/logs/views/videoView.tsxui/app/workspace/mcp-logs/views/emptyState.tsxui/app/workspace/mcp-logs/views/mcpLogDetailsSheet.tsxui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/plugins/fragments/pluginFormFragments.tsxui/app/workspace/plugins/views/pluginsView.tsxui/components/ui/codeEditor.tsxui/components/ui/combobox.tsxui/components/ui/custom/dropdown/dropdown.tsxui/components/ui/custom/dropdown/dropdownGroup.tsxui/components/ui/custom/dropdown/dropdownItem.tsxui/components/ui/custom/dropdown/index.tsxui/components/ui/custom/dropdown/searchableDropdown.tsxui/components/ui/custom/dropdown/types.tsui/components/ui/custom/input.cssui/components/ui/custom/modelParameters/booleanFieldView.tsxui/components/ui/custom/modelParameters/fieldLabel.tsxui/components/ui/custom/modelParameters/index.tsxui/components/ui/custom/modelParameters/jsonFieldView.tsxui/components/ui/custom/modelParameters/numberFieldView.tsxui/components/ui/custom/modelParameters/paramFieldView.tsxui/components/ui/custom/modelParameters/selectFieldView.tsxui/components/ui/custom/modelParameters/textArrayFieldView.tsxui/components/ui/custom/modelParameters/textFieldView.tsxui/components/ui/custom/modelParameters/types.tsui/components/ui/custom/number.tsxui/components/ui/custom/richTextarea.tsxui/components/ui/input-group.tsxui/components/ui/markdown.tsxui/components/ui/mcpToolSelector.tsxui/components/ui/resizable.tsxui/components/ui/scrollArea.tsxui/components/ui/slider.tsxui/components/ui/splitButton.tsxui/package.json
✅ Files skipped from review due to trivial changes (1)
- ui/components/ui/custom/modelParameters/fieldLabel.tsx
🚧 Files skipped from review as they are similar to previous changes (22)
- ui/components/ui/custom/modelParameters/textFieldView.tsx
- ui/components/ui/slider.tsx
- ui/app/workspace/logs/views/transcriptionView.tsx
- ui/app/workspace/logs/sheets/logDetailsSheet.tsx
- ui/components/ui/custom/dropdown/index.tsx
- ui/app/workspace/plugins/views/pluginsView.tsx
- ui/app/workspace/logs/views/logChatMessageView.tsx
- ui/components/ui/codeEditor.tsx
- ui/components/ui/custom/number.tsx
- ui/app/workspace/mcp-logs/views/emptyState.tsx
- ui/components/ui/custom/modelParameters/jsonFieldView.tsx
- ui/app/workspace/logs/views/videoView.tsx
- ui/components/ui/custom/dropdown/types.ts
- ui/components/ui/resizable.tsx
- ui/app/workspace/logs/views/logResponsesMessageView.tsx
- ui/components/ui/scrollArea.tsx
- ui/components/ui/custom/dropdown/dropdownItem.tsx
- ui/package.json
- ui/app/workspace/logs/views/emptyState.tsx
- ui/components/ui/custom/dropdown/dropdownGroup.tsx
- ui/components/ui/custom/dropdown/searchableDropdown.tsx
- ui/components/ui/custom/dropdown/dropdown.tsx
feat/prompt-playground-ui-components WIP: prompt playground draft
feat/prompt-playground-ui-components WIP: prompt playground draft
feat/prompt-playground-ui-components WIP: prompt playground draft
impoiler
force-pushed
the
03-06-feat_prompt_management_core_components
branch
from
01a6bfc to
c0ddd3d
Compare
impoiler
force-pushed
the
03-06-feat_prompt_management_transport
branch
from
23525c8 to
b58a2cf
Compare
akshaydeo
changed the base branch from
03-06-feat_prompt_management_transport
to
graphite-base/1968
This was referenced Mar 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
All the core UI components that will help build a prompt playground and repository UI in upcoming PRs
Changes
Type of change
Affected areas
How to test
N/A - These are core components added to the UI but are not yet being used. They are for future use.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines