FEAT: Load Scores Per Message in GUI - #2352
Conversation
Roman Lutz (romanlutz)
left a comment
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
|
I think would be preferable if the score was at the bottom of a piece rather than vertically centered. I also think the multiple score experience is a bit confusing. 
I had to look back and forth a few times to realize the 0.91 was the same as the one presented to the left and not a separate score. Wild idea: Would it be hard to show them visually stacked (can be static with 3 maybe, doesn't need to adjust the number of levels to the number of scores) |
Roman Lutz (romanlutz)
left a comment
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
| appearance="subtle" | ||
| size="small" | ||
| className={styles.scoreChip} | ||
| aria-label="View score details" |
There was a problem hiding this comment.
Every multi-score group has the same accessible name, View score details, so text and attachment score groups in one message produce indistinguishable buttons. Please include the piece/source label in this trigger's name, and in the score chip name too, since identical scorer/value pairs can otherwise collide.
|
|
||
| return ( | ||
| <div className={styles.scoreList}> | ||
| <MessageScore score={selectedScore} groupId={groupId} scoreIndex={selectedIndex} /> |
There was a problem hiding this comment.
This still makes only the selected/default score visible; every other score is hidden behind +, and selecting one replaces the current chip. We should keep every score visible inline, optionally highlighting the objective score, rather than making users discover and swap them one at a time.
There was a problem hiding this comment.
Yes designed that way on purpose so we wouldn't overflow with scores!
| clients don't have to dig into ``scorer_class_identifier``. | ||
| """ | ||
|
|
||
| is_objective_score: bool = Field( |
There was a problem hiding this comment.
Message responses can mark the canonical objective score, but attack_result_to_summary_async builds ar.last_score without setting this flag, so the same score is serialized as false in every AttackSummary. Please pass is_objective_score=True for the summary's last_score and cover that mapper path.
| # Historical unnamed GUI attacks persisted this sentinel. Normalize it | ||
| # at the API boundary so clients can use the objective value directly, | ||
| # without a separate metadata or has_explicit_objective flag. | ||
| objective="" if ar.objective == _LEGACY_MANUAL_ATTACK_PLACEHOLDER else ar.objective, |
There was a problem hiding this comment.
This string-only compatibility check erases a legitimate explicit objective named exactly Manual attack via GUI, even for non-manual attacks; the current test uses a Crescendo attack and codifies that collision. Please scope the fallback to legacy unnamed ManualAttack identity/metadata instead of normalizing every matching objective.
| <MenuPopover> | ||
| <MenuList> | ||
| {scores.map((score, scoreIndex) => ( | ||
| <MenuItem |
There was a problem hiding this comment.
These menu items are the controls users tap after opening +, but Fluent's default MenuItem rows are only 32px high on the coarse-pointer viewport. Please apply the mobile 44px minimum here and extend the E2E test to open the menu and measure its items; it currently measures only the chip and trigger.
| {/* Text content (converted / primary), with any scores for the | ||
| text piece(s) shown alongside it at the same height rather | ||
| than stacked above or below. */} | ||
| {message.content && ( |
There was a problem hiding this comment.
message.scores is inside the message.content truthiness guard, so a scored text piece whose converted response is "" renders no score at all. Please render this row when either content or scores are present and add an empty-content scored-response test.
|
|
||
| return { | ||
| ...score, | ||
| sourcePieceId: piece.id, |
There was a problem hiding this comment.
ScoreView already serializes the canonical message_piece_id, and each score also arrives nested under its originating BackendMessagePiece. Can we add message_piece_id to BackendScore instead of duplicating it as sourcePieceId, keep the backend DTO free of frontend-only optional fields, and use a separate display type only for derived fields such as pieceIndex, pieceType, and sourceLabel?
|
|
||
| is_objective_score: bool = Field( | ||
| default=False, | ||
| description="Whether this is the attack's canonical objective score.", |
There was a problem hiding this comment.
This description is ambiguous because "objective score" could mean any score produced by the objective scorer. Since the flag is set only when the score ID matches AttackResult.last_score.id, please describe it explicitly as "Whether this is the score referenced by AttackResult.last_score."
2 participants
Description
Loads scores in the GUI for each message. Loads the last message if there are multiple and adds objective when present above the conversation.
Screenshots:
multiple scores:
multiple pieces:
Tests and Documentation
Ran frontend tests & added new tests for changes