-
Notifications
You must be signed in to change notification settings - Fork 27
Added AI-generated label indicator and tooltip message #4094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…logic to check SQL tables. Added a tooltip message that pops up on hover across gallery, admin, and validate pages.
misaugstad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks incredibly solid, great work!! Some comments:
- Looks like when switching out the AI icon, something was missed on the /admin/label/ search page:

- There are references to
admin-ai-icon-header... Are those remnants of when you had an AI icon next to the label type name in the popup headers? If so, there's some stuff left to clean out there! If not, what does it do? - I didn't have a chance to carefully look through the three
AiLabelIndicator.jsfiles, but I assume that they have quite a bit of overlap. Do you think that it makes sense to try to consolidate them? We can talk in our meeting about how they're differentiated. - There are some more comments in the code, including some about writing more efficient queries!
app/models/label/LabelTable.scala
Outdated
| at.stale, | ||
| comment.comments | ||
| comment.comments, | ||
| EXISTS ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that, instead of adding a subquery here, it would be more efficient to just do the joins in the main query. So within the query it would look like
...
INNER JOIN sidewalk_user AS u ON at.user_id = u.user_id
INNER JOIN user_role AS ur ON u.user_id = ur.user_id
INNER JOIN role AS r ON ur.role_id = r.role_id
INNER JOIN label_point AS lp ON lb1.label_id = lp.label_id
...
And then you could just refer to it as r.role = 'AI' rather than doing the subquery in the SELECT statement.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@misaugstad I removed the per-row EXISTS subquery in getRecentLabelsMetadata and pulled the role check into the main query: we now join user_role/role once and select (ai_user.user_id IS NOT NULL) AS ai_generated, using r.role = 'AI' from the join instead of a subquery.
app/models/label/LabelTable.scala
Outdated
| if (includeAiTags) la.flatMap(_.tags).getOrElse(List.empty[String].bind).asColumnOf[Option[List[String]]] | ||
| else None.asInstanceOf[Option[List[String]]].asColumnOf[Option[List[String]]] | ||
| else None.asInstanceOf[Option[List[String]]].asColumnOf[Option[List[String]]], | ||
| isAiUser(l.userId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same comment above applies to these Slick queries as well. Rather than having this function that essentially creates a subquery for each row, we can do the joins above:
val _labelInfo = for {
(_lb, _at, _us) <- labelsWithAuditTasksAndUserStats
_ur <- userRoles if _us.userId === _ur.userId
_r <- roleTable if _ur.roleId === _r.roleId
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would apply this to all the queries that you have, even if I didn't explicitly comment on them.
| aiIcon.setAttribute('data-toggle', 'tooltip'); | ||
| aiIcon.setAttribute('data-placement', 'top'); | ||
| aiIcon.setAttribute('title', i18next.t('common:ai-disclaimer', { aiVal: aiValidation })); | ||
| aiIcon.setAttribute('title', i18next.t('common:ai-generated-label-tooltip')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this change, but I'm assuming it's a mistake?
… fixed /admin/label/search icon issue, cleaned up code.
misaugstad
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app/models/label/LabelTable.scala
Outdated
| at.stale, | ||
| comment.comments | ||
| comment.comments, | ||
| EXISTS ( |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| function setLabel(labelMetadata) { | ||
| const isAiGenerated = Boolean(labelMetadata['ai_generated']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we should be using Boolean() here... I think that it's meant to coerce anything to a boolean primitive. But if our API is returning something other than a boolean primitive, then we have bigger problems and we shouldn't be hiding it like this.
But that's just based on my cursory googling, since I've never used the Boolean() function before. Did you have a reason for using it..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@misaugstad I used Boolean() just to normalize potential null/undefined to a boolean when the field was missing, not because I expected non-boolean values. I agree we shouldn’t mask bad data - happy to drop the wrapper and rely on the API returning a real boolean. Let me switch it to use the value directly.
…ltip z-index, added comments, reordered AiLabelIndicator.js
| self.panorama.setPano(labelMetadata['gsv_panorama_id'], labelMetadata['heading'], | ||
| labelMetadata['pitch'], labelMetadata['zoom'], panoCallback); | ||
|
|
||
| const isAiGenerated = Boolean(labelMetadata['ai_generated']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed that you used Boolean() here. You should just be able to pass in labelMetadata['ai_generated'] directly to AdminPanoramaLabel() like we do for all the other parameters (you shouldn't need to check if it's equal to true either).
| column-gap: 10px; | ||
| } | ||
|
|
||
| .gallery-expanded-view-header-label { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This CSS class isn't used anywhere, is it? I think that since you removed the icon from the header, all the edits from this file can likely be removed?
| .ai-icon-marker-expanded { | ||
| top: -6px; | ||
| left: -4px; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaand just missing the newline here. Most minor thing in the world lol. But I would strongly recommend looking through the "Files changed" section of any PR before asking for reviews, since that's what the reviewers will be looking at! Will help you catch all sorts of mistakes
| setProperty("newTags", [...params.tags]); // Copy tags to newTags. | ||
| } | ||
| if ("ai_tags" in params) setAuditProperty("aiTags", params.ai_tags); | ||
| if ("ai_generated" in params) setAuditProperty("aiGenerated", Boolean(params.ai_generated)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still using Boolean() here...
|
@ishajagadish I've fixed the issue with the inner joins.
I've fixed this in all four queries (Gallery, Validate, LabelMap, and the label popup). |


Resolves #4035
Added an “AI-generated” icon and tooltip across gallery, admin/label map, and validate views. The frontend uses new AiLabelIndicator helpers and CSS tweaks so the icon sits on the marker corner, and a tooltip explains the label was AI-generated. On the backend, LabelTable.getRecentLabelsMetadata flags ai_generated via a SQL EXISTS check on labels placed by users with the AI role, and that flag is exposed through LabelFormats/AdminController to drive the UI.
Before/After screenshots
Before:

After:

Testing instructions
Things to check before submitting the PR