Conversation
Summary of ChangesHello @rjison, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new 'TV Mode' feature, enabling users to view a live, grid-based display of multiple applications running on a specific device. This mode is accessible via a dedicated URL, making it suitable for display on large screens like smart TVs. The changes include new backend logic to serve the view, a new frontend template for the display, and UI integration to easily access this feature from device management. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'TV Mode' feature, which provides a URL to display multiple apps simultaneously, suitable for a TV browser. The changes include a new HTTP handler and route, a corresponding HTML template with styling and JavaScript for this mode, and a link to access it from the device card.
My review identifies a couple of areas for improvement:
- In
internal/server/handlers_device.go, the newhandleDeviceTVhandler contains redundant logic. It performs an unnecessarynilcheck and a database query that is already handled by the middleware. Removing this will simplify the code and improve performance. - In
web/templates/manager/device_tv.html, there is a duplicated CSS rule for.app-gridwhich should be removed to improve maintainability.
Overall, the changes are a good addition. Addressing these points will make the implementation cleaner and more efficient.
|
So you'd need to login on your TV browser ? |
There was a problem hiding this comment.
Code Review
The pull request introduces a new "TV Mode" feature, allowing users to view multiple apps running on a device simultaneously through a dedicated URL. The changes include adding a new handler in handlers_device.go, registering a new route and template in server.go, creating a new HTML template device_tv.html, and adding a "TV Mode" button to the device card in device_card.html. The new mode includes fullscreen functionality and automatic image refreshing. Overall, the implementation is straightforward, but there are a few areas for improvement regarding database efficiency and CSS maintainability.
| func (s *Server) handleUpdateDeviceGet(w http.ResponseWriter, r *http.Request) { | ||
| user := GetUser(r) | ||
| device := GetDevice(r) |
There was a problem hiding this comment.
The device object passed to handleDeviceTV via the RequireDevice middleware should already have its Apps preloaded. The s.DB.Preload("Apps").First(device, "id = ?", device.ID) call here is redundant and performs an unnecessary database query, impacting efficiency. You can remove this block as the device.Apps field should already be populated.
| </script> | ||
| <style> | ||
| body, html { | ||
| background-color: black !important; |
There was a problem hiding this comment.
Using !important in CSS is generally discouraged as it can lead to specificity wars and make stylesheets harder to maintain and debug. Consider refactoring your CSS to achieve the desired styling without relying on !important by adjusting selector specificity or order.
| background-color: black !important; | |
| background-color: black; |
| .app-grid { | ||
| display: grid; | ||
| /* Changed to auto-fit and increased min-width to 350px | ||
| so apps look better on large screens */ |
There was a problem hiding this comment.
This grid-template-columns declaration is duplicated on line 89. Please remove one of the redundant declarations to keep the CSS clean and avoid confusion.
I didn't actually test on my TV.. that's a good point. Early in development I got stuck because it wouldn't let me add that /tv because it wasn't requiring you to be logged in. Any suggestions? |
|
I don't think having to log in is a big deal actually. It's a pain once, but not that bad. |
Yeah it's fine require a login i think. Better for security too. Although we could allow it on local ip's like we do for auto login feature. |
2 participants
Add a TV Mode to the manager. So you have a URL that you can call on your TVs browser and see multiple APPs running at once. You could configure a new "TV" device, or simply monitor an existing app.