Implement the new design from #649

[sstidl]

User description

I will close #649 to follow up here in this PR. Thanks to @Timendus there for implementing it.

The when the merge is ready we will have a new design as proposed by @Chris-ZoGo in #585

---

PR Type

Enhancement

---

Description

• Implement new fromScratch design for LibreSpeed frontend

• Add complete HTML/CSS/JS frontend with modern UI components

• Update Docker configurations to include frontend directory

• Add server selection, gauges, and telemetry support

---

Diagram Walkthrough

flowchart LR
  A["New Frontend Files"] -->|HTML/CSS/JS| B["Modern UI Design"]
  C["Docker Config"] -->|Copy Frontend| D["Frontend Deployment"]
  B -->|Server Selection| E["Test Interface"]
  B -->|Gauges & Metrics| E
  E -->|Telemetry Support| F["Results Sharing"]

File Walkthrough

	Relevant files
Enhancement	10 files index.js Main application logic and UI rendering                                    +397/-0  index.html New HTML structure with modern layout                                        +151/-0  index.css Main stylesheet with design imports                                            +85/-0    colors.css Color theme variables and palette                                                +36/-0    main.css Main content area styling                                                                +58/-0    button.css Button styles with animations                                                        +83/-0    server-selector.css Server dropdown selector styling                                                  +171/-0  results.css Gauge and results display styling                                                +260/-0  dialog.css Modal dialog popup styling                                                              +132/-0  fonts.css Inter font family declarations                                                      +22/-0
Configuration changes	5 files server-list.json Global server list configuration                                                  +409/-0  settings.json Frontend settings and telemetry config                                      +9/-0      entrypoint.sh Update Docker setup for frontend deployment                            +6/-3      Dockerfile Add frontend directory to Docker image                                      +1/-0      Dockerfile.alpine Add frontend directory to Alpine image                                      +1/-0
Documentation	1 files README.md Frontend documentation and credits                                              +76/-0

---

[qodo-free-for-open-source-projects[bot]]

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/librespeed/speedtest/commit/dda78410139b4af549de22c7af8dba7f1d845ec0)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 585 - Fully compliant Compliant requirements: Implemented redesigned UI based on fromScratch Studio's designs Implemented desktop view design Implemented mobile view design Created responsive layout that works on different screen sizes Maintained core functionality of the speed test Improved usability and user experience

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The code lacks robust error handling for network failures or API errors. For example, in the applySettingsJSON and applyServerListJSON functions, errors are caught but only logged to console, with no user-facing error messages. async function applySettingsJSON() { try { const response = await fetch("settings.json"); const settings = await response.json(); if (!settings || typeof settings !== "object") { return console.error("Settings are empty or malformed"); } for (let setting in settings) { testState.speedtest.setParameter(setting, settings[setting]); if ( setting == "telemetry_level" && settings[setting] && settings[setting] != "off" && settings[setting] != "disabled" && settings[setting] != "false" ) { testState.telemetryEnabled = true; document.querySelector("#privacy-warning").classList.remove("hidden"); } } } catch (error) { console.error("Failed to fetch settings:", error); } } /** * Load server list from server-list.json on the server and populate the * dropdown */ async function applyServerListJSON() { try { const response = await fetch("server-list.json"); const servers = await response.json(); if (!servers || !Array.isArray(servers) || servers.length === 0) { return console.error("Server list is empty or malformed"); } testState.servers = servers; populateDropdown(testState.servers); if (servers.length > 1) { testState.speedtest.addTestPoints(servers); testState.speedtest.selectServer((server) => { if (server) { selectServer(server); } else { alert( "Can't reach any of the speedtest servers! But you're on this page. Something weird is going on with your network." ); } }); } } catch (error) { console.error("Failed to fetch server list:", error); } } Security Concern The alert message on line 157-160 could expose information about the network configuration that might be useful for attackers. Consider using a more generic error message. alert( "Can't reach any of the speedtest servers! But you're on this page. Something weird is going on with your network." ); } Logic Issue The conditional structure has a mismatched 'fi' statement. The 'fi' on line 49 closes the if statement started on line 44, but there's no matching 'fi' for the if statement started on line 42. # Set up index.php for frontend-only or standalone modes if [[ "$MODE" == "frontend" || "$MODE" == "dual" ]]; then cp -av /speedtest/frontend/* /var/www/html/ elif [ "$MODE" == "standalone" ]; then cp -av /speedtest/frontend/* /var/www/html/ echo '[{"name":"local","server":"/backend", "dlURL": "garbage.php", "ulURL": "empty.php", "pingURL": "empty.php", "getIpURL": "getIP.php", "sponsorName": "", "sponsorURL": "", "id":1 }]' > /var/www/html/server-list.json fi

[qodo-free-for-open-source-projects[bot]]

PR Code Suggestions ✨

Latest suggestions up to 4fc0932

Category	Suggestion	Impact
Possible issue	Fix mode logic inconsistency There's a logical error in the conditional blocks. In the first block, if mode is "standalone" or "backend", SPEEDTEST_SERVERS is set to an empty array. But then in the dual mode block, it tries to add a server to SPEEDTEST_SERVERS, which would be empty if mode is "standalone". This contradicts the entrypoint.sh logic where standalone mode should have a local server. docker/ui.php [14-30] -if($mode=="standalone" || $mode=="backend"){ ?> +if($mode=="backend"){ ?> var SPEEDTEST_SERVERS=[]; <?php } else { ?> var SPEEDTEST_SERVERS= <?= file_get_contents('/servers.json') ?: '[]' ?>; <?php } -if ($mode=="dual"){ ?> -// add own server in dual mode +if ($mode=="dual" || $mode=="standalone"){ ?> +// add own server in dual or standalone mode SPEEDTEST_SERVERS.unshift({ "name":"This Server", "server":document.location.href+"backend/", "id":1, "dlURL":"garbage.php", "ulURL":"empty.php", "pingURL":"empty.php", "getIpURL":"getIP.php", "pingT":-1 }) [ ] Apply this suggestion <!-- /improve --apply_suggestion=0 --> Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical logical error in the mode handling. The current code sets SPEEDTEST_SERVERS to empty for standalone mode but then tries to add a server in dual mode only, which would cause standalone mode to have no servers. The fix properly aligns with the entrypoint.sh logic.	High
	Fix mode configuration consistency The server-list.json file is only created for standalone mode, but the ui.php logic expects it for dual mode as well. This creates an inconsistency where dual mode might not have the proper server configuration. docker/entrypoint.sh [43-49] # Set up index.php for frontend-only or standalone modes -if [[ "$MODE" == "frontend" || "$MODE" == "dual" ]]; then +if [[ "$MODE" == "frontend" || "$MODE" == "dual" || "$MODE" == "standalone" ]]; then cp -av /speedtest/frontend/* /var/www/html/ -elif [ "$MODE" == "standalone" ]; then - cp -av /speedtest/frontend/* /var/www/html/ - echo '[{"name":"local","server":"/backend", "dlURL": "garbage.php", "ulURL": "empty.php", "pingURL": "empty.php", "getIpURL": "getIP.php", "sponsorName": "", "sponsorURL": "", "id":1 }]' > /var/www/html/server-list.json + if [[ "$MODE" == "standalone" ]]; then + echo '[{"name":"local","server":"/backend", "dlURL": "garbage.php", "ulURL": "empty.php", "pingURL": "empty.php", "getIpURL": "getIP.php", "sponsorName": "", "sponsorURL": "", "id":1 }]' > /var/www/html/server-list.json + fi fi [ ] Apply this suggestion <!-- /improve --apply_suggestion=1 --> Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential inconsistency between the entrypoint.sh and ui.php handling of dual mode. The improved code consolidates the logic for copying frontend files and creates a more maintainable structure that ensures proper configuration across different modes.	Medium
General	Fix duplicate server names Multiple servers have the same name "Amsterdam, Netherlands" with different IDs (51 and 94). This can cause confusion for users when selecting a server and may lead to unexpected behavior if the application relies on unique server names. frontend/server-list.json [2-12] { - "name": "Amsterdam, Netherlands", + "name": "Amsterdam, Netherlands (Clouvider)", "server": "//ams.speedtest.clouvider.net/backend", "id": 51, "dlURL": "garbage.php", "ulURL": "empty.php", "pingURL": "empty.php", "getIpURL": "getIP.php", "sponsorName": "Clouvider", "sponsorURL": "https://www.clouvider.co.uk/" }, [ ] Apply this suggestion <!-- /improve --apply_suggestion=2 --> Suggestion importance[1-10]: 7 __ Why: Having multiple servers with identical names ("Amsterdam, Netherlands") but different IDs (51 and 94) could confuse users when selecting a server. Adding the sponsor name to differentiate them improves usability and prevents potential selection errors.	Medium
Possible issue	Complete test loop implementation The test script has a loop structure but doesn't contain any actual commands inside the loops, making them ineffective. Add commands to test each container combination. docker/test/test-script.sh [15-16] for db in sqlite pg mysql; do for distro in alpine debian; do + echo "Testing $distro with $db database..." + w3m -dump http://speedtest-${distro}-${db}:${PORT}/ || echo "Failed to connect to $distro with $db" + done +done [ ] Apply this suggestion <!-- /improve --apply_suggestion=13 --> Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the test script has empty loops without any actual testing commands. Adding the w3m commands to test each container combination is critical for the test script to function properly.	High
Security	Fix privacy policy placeholder The privacy policy contains a placeholder email address that needs to be replaced with a real contact email. This could cause compliance issues with privacy regulations if deployed as-is. frontend/index.html [145] -<a href="mailto:PUT@YOUR_EMAIL.HERE">TO BE FILLED BY DEVELOPER</a> +<a href="mailto:${EMAIL}">Contact Us</a> [ ] Apply this suggestion <!-- /improve --apply_suggestion=14 --> Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a critical issue where a placeholder email address in the privacy policy could cause compliance problems with privacy regulations. Using an environment variable would properly configure the contact email.	Medium
	Use explicit HTTPS protocol Protocol-relative URLs (starting with "//") can cause issues in certain contexts, especially when accessed via file:// protocol or in certain embedded contexts. Using explicit HTTPS is more secure and reliable. frontend/server-list.json [4-11] { "name": "Amsterdam, Netherlands", - "server": "//ams.speedtest.clouvider.net/backend", + "server": "https://ams.speedtest.clouvider.net/backend", "id": 51, "dlURL": "garbage.php", "ulURL": "empty.php", "pingURL": "empty.php", "getIpURL": "getIP.php", "sponsorName": "Clouvider", "sponsorURL": "https://www.clouvider.co.uk/" }, [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: Protocol-relative URLs can cause issues in certain contexts like file:// access or embedded environments. Using explicit HTTPS improves security and reliability, making this a valuable suggestion that affects multiple server entries in the file.	Medium
General	Handle invalid gauge values The rotation calculation for the gauge could produce NaN if speed is negative, which would cause visual issues. Add a check to ensure speed is a positive number before performing calculations. frontend/javascript/index.js [382-383] -// Make sure we stay within bounds at all times +// Make sure we stay within bounds at all times and handle invalid values +if (isNaN(rotation)) return 0; return Math.max(Math.min(rotation, maxRotation), minRotation); [ ] Apply this suggestion <!-- /improve --apply_suggestion=16 --> Suggestion importance[1-10]: 7 __ Why: This suggestion addresses a potential bug where NaN values could cause visual issues in the gauge display. Adding a check for NaN values improves the robustness of the UI rendering code.	Medium
	Fix dialog positioning The large fixed margin-top of 23rem pushes dialogs far down the page, potentially making them inaccessible on smaller screens. While there is a mobile media query, it only applies at max-width 800px, leaving medium-sized screens with potential issues. frontend/styling/dialog.css [10-14] -margin-top: 23rem; +margin: auto; background: var(--popup-background-color); border: none; border-radius: 0.8rem; [ ] Apply this suggestion <!-- /improve --apply_suggestion=17 --> Suggestion importance[1-10]: 6 __ Why: The fixed margin-top of 23rem could push dialogs too far down on medium-sized screens, potentially causing accessibility issues. The suggestion to use auto margin is reasonable and would improve the dialog positioning across different screen sizes.	Low
[ ] More <!-- /improve --more_suggestions=true -->

---

Previous suggestions

✅ Suggestions up to commit dda7841

Category	Suggestion	Impact
Possible issue	Fix conditional logic error There's a logical error in the conditional checks. The "dual" mode is first checked with "standalone" (line 1), then separately (line 7). This means in "dual" mode, SPEEDTEST_SERVERS will be set to empty array first, then the code tries to add a server to it with unshift(), which won't work as expected. docker/ui.php [14-19] -if($mode=="standalone" || $mode=="backend"){ ?> +if($mode=="standalone"){ ?> +var SPEEDTEST_SERVERS=[]; +<?php } else if($mode=="backend") { ?> var SPEEDTEST_SERVERS=[]; <?php } else { ?> var SPEEDTEST_SERVERS= <?= file_get_contents('/servers.json') ?: '[]' ?>; <?php } if ($mode=="dual"){ ?> Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical logical error in the conditional checks. In dual mode, SPEEDTEST_SERVERS would be set to an empty array first, then the code would try to add a server to it with unshift(), which would result in incorrect behavior. The fix properly separates the conditions.	High
	✅ Add missing CSS units The width and height properties are missing units (px, rem, etc.), which can cause rendering issues. Add appropriate units to these values. frontend/styling/server-selector.css [31-39] & > div.chevron { content: ""; position: absolute; display: block; - width: 32; - height: 32; + width: 32px; + height: 32px; right: 1.8rem; top: 1rem; } [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: The width and height properties are missing units (px, rem, etc.), which will cause rendering inconsistencies across browsers. This is a clear CSS error that needs to be fixed.	Medium
	Add missing sanity check The script is missing the actual sanitycheck call mentioned in the comment. The script installs w3m but doesn't use it to perform any tests on the containers. docker/test/test-script.sh [1-10] #!/bin/sh ## simple script to call sanitycheck on all containers PORT=8080 if ! apk add w3m; then echo "Failed to install w3m package" exit 1 fi +# Function to perform sanity check +perform_sanity_check() { + echo "Performing sanity check on $1..." + w3m -dump http://localhost:$PORT/ +} + Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the script installs w3m but doesn't actually use it to perform any sanity checks as mentioned in the comment. Adding a function to perform these checks would improve the script's functionality and align with its stated purpose.	Medium
	Add missing display property The dialog element is missing the display: flex property which is required for flex-direction, align-items, and justify-content to work properly when the dialog is not open. frontend/styling/dialog.css [3-8] dialog { + display: flex; flex-direction: column; align-items: center; justify-content: center; width: 70vw; height: 70vh; margin: auto; margin-top: 23rem; Suggestion importance[1-10]: 6 __ Why: The dialog element needs display: flex for the flex properties to work properly when the dialog is not open. The animation works when open because of the explicit display:flex in the [open] selector.	Low
Security	Use explicit HTTPS protocol The server URLs starting with "//" (protocol-relative URLs) may cause issues in some contexts. Consider using explicit HTTPS protocol for better security and reliability. frontend/server-list.json [2-12] { "name": "Amsterdam, Netherlands", - "server": "//ams.speedtest.clouvider.net/backend", + "server": "https://ams.speedtest.clouvider.net/backend", "id": 51, "dlURL": "garbage.php", "ulURL": "empty.php", "pingURL": "empty.php", "getIpURL": "getIP.php", "sponsorName": "Clouvider", "sponsorURL": "https://www.clouvider.co.uk/" }, Suggestion importance[1-10]: 7 __ Why: Using protocol-relative URLs (starting with "//") can cause issues in certain contexts, especially when accessed via file:// protocol. Explicitly using HTTPS improves security and reliability.	Medium
Possible issue	Fix logarithmic calculation The Math.log10(minSpeed + 1) calculation is problematic because minSpeed is 0, so this always evaluates to Math.log10(1) which equals 0. This makes the denominator in the power calculation potentially very small, which could lead to unexpected results. Remove the unnecessary logarithm of minSpeed. frontend/javascript/index.js [364-384] function mbpsToRotation(speed, oscillate) { speed = Number(speed); if (speed <= 0) return 0; const minSpeed = 0; const maxSpeed = 10000; // 10 Gbps maxes out the gauge const minRotation = 0; const maxRotation = 180; - // Can't do log10 of values less than one, +1 all to keep it fair - const logMinSpeed = Math.log10(minSpeed + 1); + // Use logarithmic scale for better visualization of wide range of speeds const logMaxSpeed = Math.log10(maxSpeed + 1); const logSpeed = Math.log10(speed + 1); - const power = (logSpeed - logMinSpeed) / (logMaxSpeed - logMinSpeed); + const power = logSpeed / logMaxSpeed; const oscillation = oscillate ? 1 + 0.01 * Math.sin(Date.now() / 100) : 1; const rotation = power * oscillation * maxRotation; // Make sure we stay within bounds at all times return Math.max(Math.min(rotation, maxRotation), minRotation); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a mathematical issue where calculating log10(minSpeed + 1) is redundant since minSpeed is 0. The improved code simplifies the calculation while maintaining the logarithmic scale, making the gauge calculation more efficient and clearer.	Medium
	Add null check The selectServer function doesn't handle potential errors if the server parameter is null or undefined, which could happen if server selection fails. Add a check to prevent potential errors when trying to access properties of an invalid server object. frontend/javascript/index.js [212-216] function selectServer(server) { + if (!server) { + console.error("Cannot select server: Invalid server object"); + return; + } testState.speedtest.setSelectedServer(server); testState.selectedServerDirty = true; testState.state = READY; } Suggestion importance[1-10]: 6 __ Why: The suggestion adds a defensive null check to prevent potential errors if the server parameter is undefined or null. This is a good defensive programming practice that improves code robustness, especially since server selection could potentially fail.	Low
General	Add error handling The function doesn't handle errors that might occur when trying to access the clipboard API, which may fail due to permissions or browser compatibility issues. Add try-catch to handle potential errors gracefully. frontend/javascript/index.js [98-108] async function copyLinkButtonClickHandler() { - const link = document.querySelector("img#results").src; - await navigator.clipboard.writeText(link); - const button = document.querySelector("#copy-link"); - button.classList.add("active"); - button.textContent = "Copied!"; - setTimeout(() => { - button.classList.remove("active"); - button.textContent = "Copy link"; - }, 3000); + try { + const link = document.querySelector("img#results").src; + await navigator.clipboard.writeText(link); + const button = document.querySelector("#copy-link"); + button.classList.add("active"); + button.textContent = "Copied!"; + setTimeout(() => { + button.classList.remove("active"); + button.textContent = "Copy link"; + }, 3000); + } catch (error) { + console.error("Failed to copy to clipboard:", error); + alert("Could not copy link to clipboard. Your browser may not support this feature."); + } } Suggestion importance[1-10]: 7 __ Why: The suggestion adds proper error handling for clipboard operations which might fail due to permissions or browser compatibility issues. This improves user experience by providing feedback when clipboard operations fail instead of silently failing.	Medium
	Improve dialog positioning The fixed margin-top: 23rem for dialogs can cause them to appear partially off-screen on smaller viewports. This should be removed or adjusted in the mobile media query to ensure dialogs are properly centered on all devices. frontend/styling/dialog.css [10] -margin-top: 23rem; +margin: auto; Suggestion importance[1-10]: 7 __ Why: The fixed margin-top of 23rem could cause dialogs to appear off-screen on smaller viewports. Changing to just margin: auto would improve responsive behavior and ensure dialogs are properly centered on all devices.	Medium
	Fix gauge pointer positioning Using position: fixed for gauge pointers can cause positioning issues when scrolling or when the page has multiple gauges. This is a hack that may lead to unexpected behavior. Consider using a different approach like position: absolute with proper z-index management. frontend/styling/results.css [118-120] -position: fixed; +position: absolute; top: calc(var(--gauge-width) / 2 - var(--speed-width) / 3); left: var(--progress-width); +z-index: 2; Suggestion importance[1-10]: 5 __ Why: Using position: fixed for gauge pointers can cause positioning issues when scrolling or with multiple gauges. While the code includes a comment acknowledging this as a hack, switching to position: absolute with proper z-indexing would be more reliable.	Low