Skip to content
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

feat(web): use websocket to update the feature photo #10683

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

martabal
Copy link
Member

@martabal martabal commented Jun 28, 2024

following #9443

With this PR, if the server sends a thumbnail generation event of a person, the client updates the thumbnail of that person. This PR changes the value of updatedAt to force the browser to fetch the new thumbnail. IMO, it's not a bad thing to change this value even if it's not the same as the one stored on the server: updatedAt is only used to force the browser to request a new thumbnail.

How was this PR tested ?

  • Stop the machine-learning process
  • Change a feature photo
  • Go back to /people
  • Re-start the machine-learning process
  • See the thumbnail changing without reloading

Copy link
Member

@bo0tzz bo0tzz left a comment

Choose a reason for hiding this comment

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

Nice!

@alextran1502 alextran1502 merged commit e0bb9ad into main Jun 28, 2024
24 checks passed
@alextran1502 alextran1502 deleted the feat/update-feature-photo-ws branch June 28, 2024 19:40
return websocketEvents.on('on_person_thumbnail', (personId: string) => {
people.map((person) => {
if (person.id === personId) {
person.updatedAt = Date.now().toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be new Date().toISOString()?

Copy link
Member Author

Choose a reason for hiding this comment

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

This works too, the main objective is to have a different value

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

Successfully merging this pull request may close these issues.

4 participants