Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…in attack history
…ype hints where its redundant
jbolor21
force-pushed
the
bjagdagdorj/frontend_converters
branch
from
446c643 to
69d038f
Compare
| } | ||
| } | ||
|
|
||
| void loadConverters() |
There was a problem hiding this comment.
This is a way to use async/await - think callbacks must be synchronous in React, aka it requires them to either return nothing or return a cleanup function. If you make it async, it returns a Promise instead of a cleanup function, which would break cleanup mechanism.
Then this inner function is just a workaround. Open to other ways to do this for sure!
| }, [selectedConverterType, previewText, paramValues, selectedConverter]) | ||
|
|
||
| const [panelWidth, setPanelWidth] = useState(320) | ||
| const isDragging = useRef(false) |
There was a problem hiding this comment.
hmm why are we using useRef here ?
There was a problem hiding this comment.
useRef.current will give latest value wo triggering re-renders on every mouse drag! this is for the resize handle to resize the panel
| } | ||
|
|
||
| const ChatInputArea = forwardRef<ChatInputAreaHandle, ChatInputAreaProps>(function ChatInputArea({ onSend, disabled = false, activeTarget, singleTurnLimitReached = false, onNewConversation, operatorLocked = false, crossTargetLocked = false, onUseAsTemplate, attackOperator, noTargetSelected = false, onConfigureTarget }, ref) { | ||
| const ChatInputArea = forwardRef<ChatInputAreaHandle, ChatInputAreaProps>(function ChatInputArea({ onSend, disabled = false, activeTarget, singleTurnLimitReached = false, onNewConversation, operatorLocked = false, crossTargetLocked = false, onUseAsTemplate, attackOperator, noTargetSelected = false, onConfigureTarget, onToggleConverterPanel, isConverterPanelOpen = false, onInputChange, onAttachmentsChange, convertedValue, originalValue: _originalValue, onClearConversion, onConvertedValueChange, mediaConversions = [], onClearMediaConversion }, ref) { |
There was a problem hiding this comment.
ik you didn't write this but do you know what this forward ref is doing ? I'm not super familiar with forwardref
There was a problem hiding this comment.
lets parent component get direct reference to things inside ChatInputArea - here the parent passes ref to CHatInputArea and then the useImperativeHandle exposes the addAttachment and setText methods
There was a problem hiding this comment.
couldn't we use a callback so the parentcomponent passes a callback and then the child component calls that callback in addAttachment & setText methods. I'm guessing the parent component needs the attachment & text information so that could be sent back in the callback. but maybe i'm missing something
| // Invalid mock payload triggers MediaWithFallback error state | ||
| await expect(page.getByTestId("video-error")).toBeVisible({ timeout: 10000 }); |
There was a problem hiding this comment.
why did this change ? since it's skipped I think we should just leave it for now. this conflicts with the test description as well which doesn't mention an error
| {...defaultProps} | ||
| onSend={onSend} | ||
| activeTarget={{ target_registry_name: "t", target_type: "T", endpoint: "e", model_name: "m" }} | ||
| convertedValue="aGVsbG8=" |
There was a problem hiding this comment.
nit: make the convertedValue just like convertedHello just bc it's kinda unclear what this is if the test fails and you're trying to debug
| describe('ConverterPanel loading', () => { | ||
| it('shows loading spinner then renders converter list on success', async () => { | ||
| renderPanel() | ||
| expect(screen.getByTestId('converter-panel-loading')).toBeInTheDocument() |
There was a problem hiding this comment.
I think we shouldn't use toBeInTheDocument and should prefer ToBeVisible bc toBeInTheDocument just means its there but could be hidden
| try: | ||
| sig = inspect.signature(converter_class.__init__) # type: ignore[misc] | ||
| except (ValueError, TypeError): | ||
| return params |
There was a problem hiding this comment.
do we need to output a more informative error message ?
| if converter_type in ("PromptConverter", "ConverterResult") or "Strategy" in converter_type: | ||
| continue | ||
| if converter_type in ("HumanInTheLoopConverter", "SelectiveTextConverter"): | ||
| continue |
There was a problem hiding this comment.
can't we combine these ?
| } | ||
| return supported.includes(activeDataType) | ||
| }) | ||
| if (query !== selectedConverterType) { |
There was a problem hiding this comment.
wait why do we have a query and a selected converter type if the query could equal the type?
| onOptionSelect={(type, text) => { | ||
| setSelectedConverterType(type) | ||
| setQuery(text) | ||
| const newConverter = converters.find((c) => c.converter_type === type) | ||
| const defaults: Record<string, string> = {} | ||
| for (const p of newConverter?.parameters ?? []) { | ||
| if (p.default_value != null) { | ||
| defaults[p.name] = p.default_value | ||
| } | ||
| } | ||
| setParamValues(defaults) | ||
| setPreviewOutput('') | ||
| setPreviewConverterInstanceId(null) | ||
| setPreviewError(null) | ||
| setShowValidation(false) | ||
| }} |
There was a problem hiding this comment.
define this as a function
| onTabSelect={(_, data) => { | ||
| const newTab = data.value as string | ||
| setActiveTab(newTab) | ||
| setSelectedConverterType('') | ||
| setQuery('') | ||
| setParamValues({}) | ||
| setPreviewOutput('') | ||
| setPreviewConverterInstanceId(null) | ||
| setPreviewError(null) | ||
| setShowValidation(false) | ||
| cachedInstanceRef.current = null | ||
| }} |
There was a problem hiding this comment.
nit: make this a function
| onClick={() => { | ||
| const input = document.createElement('input') | ||
| input.type = 'file' | ||
| input.onchange = () => { | ||
| const file = input.files?.[0] | ||
| if (file) { | ||
| // Use webkitRelativePath or name — for local backend the full path isn't available | ||
| // The user can also manually type/paste the path | ||
| setParamValues((prev) => ({ ...prev, [param.name]: file.name })) | ||
| } | ||
| } | ||
| input.click() |
| <Button | ||
| appearance="transparent" | ||
| size="small" | ||
| icon={paramsExpanded ? <ChevronDownRegular /> : <ChevronRightRegular />} | ||
| onClick={() => setParamsExpanded((prev) => !prev)} | ||
| className={styles.paramsSectionHeader} | ||
| data-testid="toggle-params-btn" | ||
| > | ||
| Parameters | ||
| </Button> | ||
| {paramsExpanded && (selectedConverter.parameters ?? []).map((param) => { | ||
| const isMissing = showValidation && param.required && !paramValues[param.name]?.trim() | ||
| return ( | ||
| <div key={param.name} className={styles.paramBlock}> | ||
| <span className={styles.paramLabel}> | ||
| <Text size={200} weight="semibold">{param.name}{param.required ? ' *' : ''}</Text> | ||
| {param.description && ( | ||
| <Tooltip content={param.description} relationship="description"> | ||
| <span className={styles.paramInfo}><InfoRegular fontSize={12} /></span> | ||
| </Tooltip> | ||
| )} | ||
| </span> | ||
| {param.type_name === 'bool' || param.type_name === 'Optional[bool]' ? ( | ||
| <Switch | ||
| checked={(paramValues[param.name] ?? param.default_value ?? 'false').toLowerCase() === 'true'} | ||
| onChange={(_, data) => | ||
| setParamValues((prev) => ({ ...prev, [param.name]: data.checked ? 'true' : 'false' })) | ||
| } | ||
| label={(paramValues[param.name] ?? param.default_value ?? 'false').toLowerCase() === 'true' ? 'True' : 'False'} | ||
| data-testid={`param-${param.name}`} | ||
| /> | ||
| ) : param.choices ? ( | ||
| <Select | ||
| value={paramValues[param.name] ?? param.default_value ?? ''} | ||
| onChange={(_, data) => | ||
| setParamValues((prev) => ({ ...prev, [param.name]: data.value })) | ||
| } | ||
| data-testid={`param-${param.name}`} | ||
| > | ||
| {param.choices.map((choice) => ( | ||
| <option key={choice} value={choice}> | ||
| {choice} | ||
| </option> | ||
| ))} | ||
| </Select> | ||
| ) : /path|file/i.test(param.name) ? ( | ||
| <div className={styles.filePickerRow}> | ||
| <Input | ||
| value={paramValues[param.name] ?? ''} | ||
| placeholder={param.default_value ?? 'Select a file...'} | ||
| onChange={(_, data) => | ||
| setParamValues((prev) => ({ ...prev, [param.name]: data.value })) | ||
| } | ||
| className={isMissing ? styles.paramInputError : undefined} | ||
| data-testid={`param-${param.name}`} | ||
| /> | ||
| <Button | ||
| appearance="subtle" | ||
| size="small" | ||
| onClick={() => { | ||
| const input = document.createElement('input') | ||
| input.type = 'file' | ||
| input.onchange = () => { | ||
| const file = input.files?.[0] | ||
| if (file) { | ||
| // Use webkitRelativePath or name — for local backend the full path isn't available | ||
| // The user can also manually type/paste the path | ||
| setParamValues((prev) => ({ ...prev, [param.name]: file.name })) | ||
| } | ||
| } | ||
| input.click() | ||
| }} | ||
| data-testid={`param-${param.name}-browse`} | ||
| > | ||
| Browse | ||
| </Button> |
There was a problem hiding this comment.
maybe create a few components so one for parameterchoiceviewer, parameterviewer, parameterfileviewer
2 participants
Description
Adding converter panels to the GUI interface! This PR only lets you run ONE converter per turn per type of media (ie you can do an image + text conversion in same turn but not two text conversions in one turn)
Tests and Documentation
Images:
(new button is shown):
(new panel)
(panel dropdown)
(once you've selected converter)
