Skip to content

Bug 2047013 - Allows rating of summarizations#303

Open
boek wants to merge 1 commit into
mozilla-firefox:mainfrom
boek:bug_2047013
Open

Bug 2047013 - Allows rating of summarizations#303
boek wants to merge 1 commit into
mozilla-firefox:mainfrom
boek:bug_2047013

Conversation

@boek

@boek boek commented Jun 23, 2026

Copy link
Copy Markdown
Contributor
image

@github-actions

Copy link
Copy Markdown
Contributor

Warning

The base branch is currently set to main. Please Edit this PR and set the base to autoland.

@github-actions

Copy link
Copy Markdown
Contributor

View this pull request in Lando to land it once approved.

@boek

boek commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

When you rate:
image

@xianghongai

Copy link
Copy Markdown

https://addons.mozilla.org/en-US/firefox/ Log in:

{
  "detail": "You do not have permission to perform this action."
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param onFeedbackClicked Invoked when the user rates the summary. When `null`, the feedback

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we add some spacing between each of these composables to make it easier to read

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm can you explain why we have this if branch? I'm probably missing something but I don't really get why we need it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could make sense for this to be here. It could also make sense to add an entirely new middleware for this, since we seem to just be leveraging our telemetry system to measure this non telemetry event. Let me know what your thoughts are. If you think it should be here, that works for me too

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user presses one of the two buttons and then changes their mind, it seems that we will record their response multiple times. Should we do something to account for this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It seems like we use the term good review / bad review and positive / negative. Can we centralize on 1 naming so its less confusing

Comment on lines 162 to 163

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we should just do feedback.name.lowercase() here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using talkback with the buttons. Thinking aloud, would it be more clear to make the content description have some sort of verb so its more like a call to action. I think something like Rate good summary would be good? Right now, users swipe from AI can make mistakes to Good summary, button - maybe this also depends on the placement, FWIW, which you were also skeptical about.

After commenting this, I also tested the buttons google search gemini and they say Good response, button, Bad response, button - so maybe it's more about the positioning then and not the content description

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird because if the user gives a rating, the content description becomes Bad summary, Button, disabled and Good summary, Button, disabled. I would also expect for the fact that a response was already selected to be announced somehow

@nicholaspoon03 nicholaspoon03 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Jeff,
Good work! Just a few things that I wanted to ask about and I don't know how data collection processes work on github - so I asked in slack to see how to do that

@flodolo

flodolo commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Stumbled upon this patch by accident.

Patches with strings should not go through this workflow for now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants