FEAT: Load Scores Per Message in GUI - #2352
Conversation
Roman Lutz (romanlutz)
left a comment
There was a problem hiding this comment.
Can you attach screenshots or videos?
Other comments AI generated but they apply nontheless :-)
|
What does this look like with multiple scores? Also the spacing is not right. This doesn't look very good with "Score" and no space between. |
…ores # Conflicts: # frontend/src/App.test.tsx # pyrit/backend/services/attack_service.py
Roman Lutz (romanlutz)
left a comment
There was a problem hiding this comment.
These are GHCP generated and at least worth a look. Making another manual pass as well
|
I exercised the score UI across single/multiple pieces, single/multiple scores, and wide/narrow viewports. Two visual issues remain:
|
|
Roman Lutz (@romanlutz) implemented your suggestions but I don't love this tab spacing/look: |
okay here's a better version I'm happier with but lmk if that is ok with you
|
|
Image doesn't load for me somehow. The tablist has the advantage that overflow is handled. I'll try options and will get back to you. |
| size, | ||
| pieceId: piece.id, | ||
| metadata: piece.prompt_metadata || undefined, | ||
| scores: !isOriginal && sortedScores.length > 0 ? sortedScores : undefined, |
There was a problem hiding this comment.
A scored media piece loses all of its scores when converted_value is empty: pieceToAttachment returns null before reaching this assignment, getTextScores excludes media, and original attachments intentionally omit scores. Please retain a score-only piece row when converted media cannot render, and add an empty-media regression test.
There was a problem hiding this comment.
The score itself is preserved now, but the fix introduces a second problem: a non-renderable media piece is represented as a real MessageAttachment with url: "", and that object is also added to message.attachments. Downstream code cannot tell that it exists only to carry scores.
I reproduced this through the API/render path by blanking both media values and URLs while retaining the score: the piece rendered 0 images and 1 score control, but the message still exposed a Download image… action. handleDownload then calls fetch(""), which resolves to the current page. The copy handlers also add this object to the composer; because it retains scores, the optimistic new message can display the previous message's scores and submit an empty media value. Exports likewise list a phantom attachment.
Can we keep score presentation separate from actionable attachments—for example, let the media display piece own scores and allow it to have no attachment—or otherwise ensure score-only placeholders never enter copy/download/export paths? Please add a component/action regression test, not only the mapper-shape test.
| justifyContent: 'center', | ||
| minWidth: '30px', | ||
| minHeight: '20px', | ||
| padding: `0 ${tokens.spacingHorizontalXS}`, |
There was a problem hiding this comment.
GHCP comment: score_type="unknown" allows arbitrary strings, but both the stacked value and single-score chip are unbounded. A long unbroken value produced a 435px button in a 390px viewport and was clipped. Please constrain and ellipsize the visible value while preserving the full value in the tooltip, accessible name, and details panel.
There was a problem hiding this comment.
The collapsed chip is constrained now, but this is only partially fixed: the details panel still leaves score metadata cells unbounded. Using the real API/render path with a valid 200-character unknown score at a 320px viewport, I measured 701px document scroll width, 684px panel scroll width, and a 604px value inside a 206px cell.
scoreRow and the value Badge/Text elements have no min-width: 0 or wrapping/overflow rule, so long score values—and similarly long scorer names, source labels, or categories—escape the popover. The new tests verify that the full value remains available, but not that the panel stays within the viewport. Please constrain/wrap the detail value cells and add a geometry assertion such as scrollWidth <= clientWidth at a narrow viewport.
| ':hover': { | ||
| backgroundColor: tokens.colorNeutralBackground2Hover, | ||
| }, | ||
| '&[aria-selected="true"]': { |
There was a problem hiding this comment.
I don't think we should override Fluent's tab appearance here. In light mode the selected dark-blue tab renders with black text, while the objective tab's separate border/background makes it look selected too:
Can we remove the custom pill, selected, and objective-state styling and let TabList/Tab render their native selection state? Objective status can remain in the tooltip and details panel instead of introducing a second active-looking state.
There was a problem hiding this comment.
good catch! k changed & checked it works in both light & dark adding fluent borders bc it looks odd to me w no borders. but will totally remove the borders if there's a real ui issue w them
|
One backend coverage gap remains around the core objective-score provenance path. The service test verifies argument forwarding to a mocked mapper, while the response-contract tests call That means a regression which attaches scores to the wrong piece, marks every score objective, or fails to mark the matching score could still leave the current tests green. Please add a mapper-level test with text and media pieces plus several scores, and assert by score ID (not list position) that:
This is the backend contract the frontend's piece-local rendering now depends on. |



















Description
Adds score visibility to the conversation UI and visibility into the objective for loaded attacks.
UI behavior
Screenshots:
Single Score & Single Piece Response:

Sample Score Details Card (when you press on score value)

multiple scores on single piece:



(stacked scores indicate multiple scores)
tabs revealing details on multiple scores:
Overflow (more scores than what fits in window)
multiple pieces:

Tests and Documentation
All existing tests pass and new tests added/updated: