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

Modify title and favicon when item in progress (issue #2392) #6204

Conversation

jungleBadger
Copy link
Contributor

@jungleBadger jungleBadger commented Apr 12, 2024

Summary

Description

Two methods (updatePageTitle and updatePageFavicon) were created and executed within the WebSocket handler.

When a Queue item's status changes, the methods append a (1) prefix and modify the favicon, respectively. I included an id attribute to the link element to ensure the query wouldn't fail or collide.

After Hipsterusername's suggestion, I modified the updatePageTitle method to display the active queue length instead of (1).

Files changed:

  1. invokeai/frontend/web/src/app/store/middleware/listenerMiddleware/listeners/socketio/socketQueueItemStatusChanged.ts
  2. invokeai/frontend/web/index.html

Files added

  1. invokeai/frontend/web/public/assets/images/invoke-alert-favicon.svg

Demo

demo.mp4

(I will update the final demo video with all suggestions implemented once it's done)

Open questions

  1. Do you have any recommendations on how to create automated tests?
  2. Should I split the methods into a dedicated file and import them on the Socket handler? I thought about doing so, but it would make more sense to go with the simplest approach and adjust if needed.

Related Issues / Discussions

Address issue #2392

QA Instructions

  1. Ensure there is an active model
  2. Input a prompt
  3. Click on the invoke button
  4. Observe the title and favicon

Merge Plan

Checklist

  • The PR has a short but descriptive title, suitable for a changelog
  • Tests added / updated (if applicable)
  • Documentation added / updated (if applicable)

@github-actions github-actions bot added the frontend PRs that change frontend files label Apr 12, 2024
@hipsterusername
Copy link
Member

Neat! And even some banana sushi!?! 👏

Is there a reason for (1) instead of the current active queue?

@jungleBadger
Copy link
Contributor Author

jungleBadger commented Apr 12, 2024

Neat! And even some banana sushi!?! 👏

Is there a reason for (1) instead of the current active queue?

I couldn't think of a better combination; as a good Brazilian, I love bananas and sushis!

Keeping track of running items and clearing them upon completion is simpler. Still, the current implementation can be flawed if parallel operations are to be supported.

I reviewed it, and it seems I could achieve that by doing something similar to the queueSize that considers pending and in_progress items.

I'm open if you have any suggestions to improve this feature.

@jungleBadger
Copy link
Contributor Author

@hipsterusername Thanks for your suggestion; I reviewed the code and implemented a solution similar to what I found in the QueueActionsMenuButton.tsx script to determine the activeQueueLength

Demo

demo.mp4

Questions

  1. I implemented a small regular expression to clean up the title. I did so to avoid hardcoding the page title in the code or implementing some separator that could cause problems later on. However, I could split the string in the ) pattern and go from there. Please let me know if RegEx is not desirable, and I can try to adapt.
  2. Do you have any tips on modularizing and creating unit testing considering the project stack?

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Cool!

The implementation isn't quite right. Currently, we are imperatively updating the title and favicon in the socket handler, but the queue status can be changed by other code paths. For example, if you queue 5 items, then clear the queue, the count and icon do not reset as expected, because that query handler isn't updating the title and favicon.

The cache for useGetQueueStatusQuery can be considered the source of truth for the queue status. The socket events and endpoints all sync to the cache. For example, in the listener you updated, you can see we update the query cache.

I'd suggest a hook that uses useEffect to sync the title and favicon, using the data from that query cache.

  • The hook would be a singleton. It's fine to just add a docstring to the hook saying to consider it a singleton, instead of programmatically enforcing it.
  • Define constants as module-scoped variables outside the hook (document.title can be referenced for the initial value outside the hook)
  • Use a type guard that checks if faviconEl is an instance of HTMLLinkElement instead of casting it
  • Call the hook once in App.tsx

@psychedelicious
Copy link
Collaborator

We don't have an e2e test setup for the frontend. Don't worry about tests.

@psychedelicious psychedelicious self-requested a review April 14, 2024 21:49
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@psychedelicious psychedelicious merged commit 14e41a1 into invoke-ai:main Apr 14, 2024
14 checks passed
@jungleBadger jungleBadger deleted the modify-title-and-favicon-when-in-progress branch April 14, 2024 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend PRs that change frontend files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants