Skip to content

Conversation

@Robot7769
Copy link
Member

  • Support for running on an HTTPS server
  • Fix: QR host configuration (--host-qr)
  • White border around QR codes

@Robot7769 Robot7769 requested a review from Copilot August 14, 2025 07:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for running the application over HTTPS and improves QR code functionality. The changes implement proxy middleware configuration, enhance Socket.IO connection handling for different protocols, and add visual improvements to QR code display.

  • Adds HTTPS support with proxy middleware configuration and secure cookie settings
  • Updates Socket.IO connection logic to work with both HTTP and HTTPS without hardcoded protocols
  • Implements white border styling around QR codes for better visual presentation

Reviewed Changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ForrestHub-app/run.py Adds ProxyFix middleware and HTTPS configuration with secure cookie settings
ForrestHub-app/assets/js/forrestHubLib.js Updates Socket.IO connection to support dynamic protocol detection and path handling
ForrestHub-app/assets/css/global_style.css Adds QR code wrapper styling with white borders
ForrestHub-app/templates/header.html Wraps QR code in styled container
ForrestHub-app/pages/admin/index.html Wraps QR code in styled container

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

.qrcode-wrapper canvas,
.qrcode-wrapper img {
display: block;
margin-top: -15px;
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

Using negative margin to offset positioning is fragile and could break if the QR code dimensions change. Consider using flexbox or grid layout for more robust positioning.

Suggested change
margin-top: -15px;

Copilot uses AI. Check for mistakes.
background: #c2c2c2 !important;
padding: 0px;
height: 286px; /* 256px height + 15px border top + 15px border bottom */
border: 15px solid #ffffff !important;
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

The hardcoded height calculation (256px + 30px border) creates a magic number that's tightly coupled to QR code dimensions. Consider using CSS calc() or making this more flexible to accommodate different QR code sizes.

Suggested change
border: 15px solid #ffffff !important;
--qrcode-size: 256px;
--qrcode-border: 15px;
.qrcode-wrapper {
display: inline-block;
background: #c2c2c2 !important;
padding: 0px;
height: calc(var(--qrcode-size) + 2 * var(--qrcode-border));
border: var(--qrcode-border) solid #ffffff !important;

Copilot uses AI. Check for mistakes.
PREFERRED_URL_SCHEME="https",
SESSION_COOKIE_SECURE=True,
REMEMBER_COOKIE_SECURE=True,
)
Copy link

Copilot AI Aug 14, 2025

Choose a reason for hiding this comment

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

Forcing HTTPS scheme globally without checking if the application is actually running over HTTPS could cause issues in development or non-HTTPS environments. Consider making this conditional based on the actual deployment context.

Suggested change
)
# Preferuj https pro generované URL a zabezpeč cookies pouze pokud je povolen HTTPS
if getattr(config, "PREFERRED_URL_SCHEME", "http") == "https":
app.config.update(
PREFERRED_URL_SCHEME="https",
SESSION_COOKIE_SECURE=True,
REMEMBER_COOKIE_SECURE=True,
)
else:
app.config.update(
PREFERRED_URL_SCHEME="http",
SESSION_COOKIE_SECURE=False,
REMEMBER_COOKIE_SECURE=False,
)

Copilot uses AI. Check for mistakes.
@Robot7769 Robot7769 merged commit 42aaf5c into Helceletka:main Aug 14, 2025
2 checks passed
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.

1 participant