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
| } | ||
|
|
||
| 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
There was a problem hiding this comment.
Pull request overview
Adds a new “Converter Panel” to the PyRIT GUI so users can discover prompt converters, configure basic parameters, preview outputs, and apply one converter per modality per turn, with supporting backend endpoints and test coverage.
Changes:
- Backend: added a converter “catalog” API (types + supported IO + simple parameter schema) and enhanced preview to support non-text data types.
- Frontend: implemented the ConverterPanel UI (tabs by modality, parameter form, preview + “use converted value”) and integrated it into ChatWindow/ChatInputArea send flow.
- Tests: added/updated unit tests (backend + frontend) and introduced Playwright e2e coverage for converter workflows.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/backend/test_converter_service.py | Adds unit tests for the new converter catalog service method and updates preview-chain expectation. |
| tests/unit/backend/test_api_routes.py | Adds route-level test for /api/converters/catalog. |
| pyrit/backend/services/converter_service.py | Implements converter catalog generation (doc/params introspection), param coercion, and enhanced preview handling for media types. |
| pyrit/backend/services/attack_service.py | Ensures atomic_attack_identifier stays consistent when per-message converters update the attack identifier. |
| pyrit/backend/routes/converters.py | Adds a new GET /converters/catalog endpoint. |
| pyrit/backend/models/converters.py | Introduces DTOs for catalog entries and parameter schemas. |
| frontend/src/types/index.ts | Adds TS types for converter catalog and converter instances. |
| frontend/src/services/api.ts | Adds convertersApi client functions (catalog/create/preview/etc.). |
| frontend/src/components/Config/TargetConfig.test.tsx | Adds dialog cancel/close test coverage. |
| frontend/src/components/Config/CreateTargetDialog.test.tsx | Makes target creation test input method consistent. |
| frontend/src/components/Chat/converterTypes.ts | Adds shared mapping/types for piece-type ↔ data-type and conversion payloads. |
| frontend/src/components/Chat/ConverterPanel.tsx | New converter panel UI: selection, param editing, preview, auto-preview behavior, and “use converted value”. |
| frontend/src/components/Chat/ConverterPanel.test.tsx | Unit tests for converter panel UI behaviors (selection, params, preview, resizing, etc.). |
| frontend/src/components/Chat/ConverterPanel.styles.ts | Styles for the converter panel per repo convention. |
| frontend/src/components/Chat/ChatWindow.tsx | Integrates converter panel state + applies conversions into outgoing message requests. |
| frontend/src/components/Chat/ChatWindow.test.tsx | Adds integration tests covering converter panel toggling and preview flow in chat. |
| frontend/src/components/Chat/ChatInputArea.tsx | Adds converter toggle button, “converted” UI rows, and attachment→base64 propagation to converter panel. |
| frontend/src/components/Chat/ChatInputArea.test.tsx | Expands unit tests for converter toggle, converted UI, and attachment conversion indicators. |
| frontend/src/components/Chat/ChatInputArea.styles.ts | Layout updates for converted/original display and attachment rows. |
| frontend/package.json | Adds react-async-hook and bumps frontend version. |
| frontend/package-lock.json | Lockfile updates for new dependency/version bump. |
| frontend/e2e/converters.spec.ts | New Playwright e2e suite covering converter panel flows. |
| frontend/e2e/chat.spec.ts | Improves mocked message GET handling and skips a flaky/overbroad video role query test. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
| original_value = request.original_value | ||
| data_type = request.original_value_data_type | ||
|
|
||
| # For path-based data types, persist base64/data-uri to a file | ||
| if str(data_type).endswith("_path") and not original_value.startswith("/"): | ||
| value = original_value | ||
| if value.startswith("data:"): | ||
| _, _, value = value.partition(",") | ||
|
|
There was a problem hiding this comment.
preview_conversion_async tries to decide whether original_value is already a file path by checking not original_value.startswith("/"). This will misclassify Windows paths (e.g. C:\...), relative paths, and some raw base64 strings that happen to start with /, leading to either attempted base64 decode of a real path or passing base64 into converters expecting a path. Consider reusing the more robust checks from AttackService._persist_base64_pieces_async (URL checks, Path(...).is_file(), and explicit data: detection) instead of the leading-slash heuristic.
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [selectedConverterType, previewText, paramValues, selectedConverter]) |
There was a problem hiding this comment.
This auto-preview useEffect reads activeTab, attachmentData, missingRequiredParams, and handlePreview, but the dependency array omits them (and suppresses react-hooks/exhaustive-deps). That can lead to stale previews (e.g., switching to an attachment tab or changing an attachment won’t trigger/clear auto-preview as expected). Please include the referenced values in the dependency list (or refactor so the effect depends on handlePreview/derived inputs) instead of disabling the lint rule.
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [selectedConverterType, previewText, paramValues, selectedConverter]) | |
| }, [activeTab, previewText, attachmentData, missingRequiredParams, selectedConverter, handlePreview]) |
| for (const att of attachments) { | ||
| if (!data[att.type] && att.file) { | ||
| const reader = new FileReader() | ||
| const base64 = await new Promise<string>((resolve) => { | ||
| reader.onload = () => resolve(reader.result as string) |
There was a problem hiding this comment.
This attachment→base64 effect has two issues: (1) it only captures the first attachment per type (if (!data[att.type] ...)), so multiple images/videos won’t be available to the converter panel; and (2) it doesn’t cancel/guard in-flight FileReader work, so a slow read from a previous attachments value can call onAttachmentsChange after the user sends/clears attachments, reintroducing stale data. Consider either restricting to one attachment per media type in the UI, or tracking all attachments (e.g. by index/pieceId) and adding a cancellation/sequence guard before calling onAttachmentsChange.
| // Apply per-piece-type conversions from the converter panel | ||
| // Each piece type (text, image, audio, video) can have its own converter | ||
| const allConverterIds: string[] = [] | ||
| for (const [pieceType, conv] of Object.entries(conversions)) { | ||
| const dataType = PIECE_TYPE_TO_DATA_TYPE[pieceType] | ||
| if (!dataType) continue | ||
| allConverterIds.push(conv.converterInstanceId) | ||
| for (const piece of pieces) { | ||
| if (piece.data_type === dataType && !piece.converted_value) { | ||
| piece.converted_value = conv.convertedValue | ||
| } |
There was a problem hiding this comment.
Applying conv.convertedValue to every piece of a given data_type means: (a) if there are multiple attachments of that type, they all get the same converted output (which is incorrect), and (b) once any piece has converted_value set, the backend currently treats the request as “preconverted” and will skip applying converter_ids entirely, so you can’t mix a locally converted text piece with backend-applied media conversions. Consider only setting converted_value for the specific piece that was previewed/selected (or avoid setting converted_value client-side and let the backend apply converter_ids per piece), and/or updating backend logic to apply converters only to pieces missing converted_value.
| // Apply per-piece-type conversions from the converter panel | |
| // Each piece type (text, image, audio, video) can have its own converter | |
| const allConverterIds: string[] = [] | |
| for (const [pieceType, conv] of Object.entries(conversions)) { | |
| const dataType = PIECE_TYPE_TO_DATA_TYPE[pieceType] | |
| if (!dataType) continue | |
| allConverterIds.push(conv.converterInstanceId) | |
| for (const piece of pieces) { | |
| if (piece.data_type === dataType && !piece.converted_value) { | |
| piece.converted_value = conv.convertedValue | |
| } | |
| // Send converter selections to the backend and let it apply conversions per piece. | |
| // Avoid setting converted_value client-side because one preview value does not | |
| // necessarily correspond to every piece of the same data type, and any locally | |
| // preconverted piece may cause the backend to skip converter_ids entirely. | |
| const allConverterIds: string[] = [] | |
| for (const [pieceType, conv] of Object.entries(conversions)) { | |
| const dataType = PIECE_TYPE_TO_DATA_TYPE[pieceType] | |
| if (!dataType) continue | |
| const hasMatchingPiece = pieces.some(piece => piece.data_type === dataType) | |
| if (hasMatchingPiece) { | |
| allConverterIds.push(conv.converterInstanceId) |
| "@fluentui/react-icons": "^2.0.258", | ||
| "axios": "^1.13.5", | ||
| "react": "^18.3.1", | ||
| "react-async-hook": "^4.0.0", |
There was a problem hiding this comment.
react-async-hook is added as a dependency but isn’t referenced anywhere in frontend/src (no imports/usages found). If it’s not needed for this PR, please remove it (and the corresponding lockfile entries) to avoid increasing bundle size / dependency surface.
| "react-async-hook": "^4.0.0", |
| converters: { converter_type: string; is_llm_based?: boolean }[] | ||
| } | ||
|
|
||
| interface SelectConverterInputProps { |
There was a problem hiding this comment.
Can you create a file for each of these components so SelectConverterInput.tsx, ConverterPreview.tsx, and then maybe one for the converter params components ? Then we can create a converter panel folder and put them all in there.
| data-testid={`param-${param.name}`} | ||
| /> | ||
| ) : param.choices ? ( | ||
| <ParameterChoiceViewer param={param} value={paramValues[param.name]} isMissing={isMissing} onChange={handleParamChange} /> |
There was a problem hiding this comment.
nit: preface these components with Converter so for example ConverterParameterChoiceViewer
| {selectedConverter && ( | ||
| <div | ||
| className={styles.converterCard} | ||
| data-testid={`converter-item-${selectedConverter.converter_type}`} | ||
| > | ||
| <Text weight="semibold" size={300} className={styles.converterName}> | ||
| {selectedConverter.converter_type} | ||
| </Text> | ||
| {selectedConverter.description && ( | ||
| <Text size={200} className={styles.hintText}> | ||
| {selectedConverter.description} | ||
| </Text> | ||
| )} | ||
| <div className={styles.metaRow}> | ||
| <Text size={200} className={styles.badgeText}>In:</Text> | ||
| {(selectedConverter.supported_input_types ?? []).map((t) => ( | ||
| <span key={t} className={`${styles.typeBadge} ${styles[`input_${t}` as keyof typeof styles] ?? ''}`}> | ||
| {t.replace('_path', '')} | ||
| </span> | ||
| ))} | ||
| </div> | ||
| <div className={styles.metaRow}> | ||
| <Text size={200} className={styles.badgeText}>Out:</Text> | ||
| {(selectedConverter.supported_output_types ?? []).map((t) => ( | ||
| <span key={t} className={`${styles.typeBadge} ${styles[`output_${t}` as keyof typeof styles] ?? ''}`}> | ||
| {t.replace('_path', '')} | ||
| </span> | ||
| ))} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
sorry i sound like a broken record but create another component for ConverterDescription
| <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> |
There was a problem hiding this comment.
is this basically the same as InfoLabel ? https://storybooks.fluentui.dev/react/?path=/docs/components-infolabel--docs or what's the difference ?
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)
(tabs for different modalities of converters - you can have 1 per mode per turn)
