Updated UI with different protocols-USB, WIFI, BLE added #40
Conversation
…ond) then close the window.
|
""" WalkthroughThis update introduces a major refactor and modularization of the bio-signal acquisition and streaming system. The legacy subprocess-based and stateful connection management for USB, WiFi, and BLE devices is replaced by a unified, asynchronous, and object-oriented approach. New modules ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI
participant FlaskAPI
participant ConnectionManager
participant Device (USB/WiFi/BLE)
participant LSL
participant AppScript
User->>WebUI: Click protocol button (USB/WiFi/BLE)
WebUI->>FlaskAPI: /connect (with protocol, device address, CSV flag)
FlaskAPI->>ConnectionManager: connect_usb/connect_wifi/connect_ble
ConnectionManager->>Device: Establish connection, start data stream
Device-->>ConnectionManager: Stream samples
ConnectionManager->>LSL: Push samples to LSL
ConnectionManager->>FlaskAPI: Connection status updates
FlaskAPI->>WebUI: JSON response (success/failure)
WebUI->>WebUI: Enable app buttons, update status
User->>WebUI: Click app button
WebUI->>FlaskAPI: /launch_app (app name)
FlaskAPI->>AppScript: Start app subprocess
AppScript-->>FlaskAPI: Running status
FlaskAPI->>WebUI: JSON response (app running)
User->>WebUI: Click disconnect
WebUI->>FlaskAPI: /disconnect
FlaskAPI->>ConnectionManager: Cleanup connections
ConnectionManager->>Device: Stop stream, cleanup
ConnectionManager->>LSL: Close outlet
FlaskAPI->>WebUI: JSON response (disconnected)
Possibly related PRs
Poem
""" ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
emgenvelope.py (1)
132-138: Timer stops but inlet is left open – consider explicit cleanupAfter detecting a disconnection you stop the
QTimerand close the window, but the LSLStreamInletobject remains open.
Although closing the window ends the process in__main__, if this widget were embedded inside a larger Qt application the inlet would keep running in the background.Add an explicit
self.inlet.close_stream()(or set it toNone) before shutting down to release the socket promptly.gui.py (1)
36-43: UI shuts down gracefully, but a tiny race remains
timer.stop()andwin.close()are executed from the timer thread-context.
Ifplot_lsl_data()has already returned itsappand the main thread is about to callapp.exec_(), a rapid disconnect could close the window before the event loop starts, leading to:RuntimeError: wrapped C/C++ object of type QWidget has been deletedSafest pattern is to emit a Qt signal or use
QtCore.QTimer.singleShot(0, win.close)instead of closing synchronously from the timer callback.This is optional but removes a rare crash on flaky networks.
chords_ble.py (1)
95-99:print_rate()resets the counter but never prints — dead codeThe coroutine continuously sleeps and sets
self.samples_received = 0, but the value is neither read nor logged, so sample-rate statistics are lost.
Either add aprint()(or better, a proper logger) or drop the task to avoid unnecessary context-switches.- await asyncio.sleep(1) - self.samples_received = 0 + await asyncio.sleep(1) + print(f"Samples/s: {self.samples_received}") + self.samples_received = 0chords_wifi.py (4)
5-5: Remove unused SciPy imports
butterandfiltfiltare never referenced in this module, so the extra dependency only increases install size and import time.-from scipy.signal import butter, filtfilt🧰 Tools
🪛 Ruff (0.8.2)
5-5:
scipy.signal.butterimported but unusedRemove unused import
(F401)
5-5:
scipy.signal.filtfiltimported but unusedRemove unused import
(F401)
66-74: Computed throughput metrics are unused
sps,fps, andbpsare calculated every second but never consumed. Either log them (handy for debugging) or remove the dead code to save a few µs per loop.- sps = self.calculate_rate(self.sample_size, elapsed_time) - fps = self.calculate_rate(self.packet_size, elapsed_time) - bps = self.calculate_rate(self.data_size, elapsed_time) + # Uncomment for live-rate diagnostics + # print(f"SPS={self.calculate_rate(self.sample_size, elapsed_time):.0f}, " + # f"FPS={self.calculate_rate(self.packet_size, elapsed_time):.0f}, " + # f"BPS={self.calculate_rate(self.data_size, elapsed_time):.0f}")🧰 Tools
🪛 Ruff (0.8.2)
67-67: Local variable
spsis assigned to but never usedRemove assignment to unused variable
sps(F841)
68-68: Local variable
fpsis assigned to but never usedRemove assignment to unused variable
fps(F841)
69-69: Local variable
bpsis assigned to but never usedRemove assignment to unused variable
bps(F841)
96-104: Don’t terminate the whole process from a library class
sys.exit()will kill whichever program imports this module. Prefer raising a domain-specific exception and let callers decide how to recover.- print("\nError: Sample Lost") - self.cleanup() - sys.exit(1) + raise RuntimeError("Sample lost – connection appears unreliable")(Apply the same change for the duplicate-sample branch.)
116-124: Exception is swallowed during cleanupThe bare
exceptmasks real bugs and the unused variableetriggers Ruff F841. At least log the message.- except Exception as e: - pass + except Exception as e: + print(f"Error while closing WebSocket: {e}")🧰 Tools
🪛 Ruff (0.8.2)
120-120: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
chords_serial.py (2)
61-62: Remove unusedsampling_rateassignment
sampling_rateis read fromsupported_boardsbut never referenced afterwards.- sampling_rate = self.supported_boards[self.board]["sampling_rate"]🧰 Tools
🪛 Ruff (0.8.2)
61-61: Local variable
sampling_rateis assigned to but never usedRemove assignment to unused variable
sampling_rate(F841)
148-149:cleanup()should not callsys.exit()For a reusable library, forcing the interpreter to exit prevents callers (e.g., Flask backend) from recovering or cleaning up additional resources. Return gracefully or raise an exception instead.
- print("Cleanup Completed.") - sys.exit(0) + print("Cleanup Completed.") + # Let caller decide whether to exitconnection.py (2)
7-8: Remove unusedsysimport
sysis not referenced in this file.-import sys🧰 Tools
🪛 Ruff (0.8.2)
7-7:
sysimported but unusedRemove unused import:
sys(F401)
60-68: Open the CSV file with a context managerUsing
with open(...)ensures the file handle is flushed and closed even if an exception occurs before__del__executes.- self.csv_file = open(filename, 'w', newline='') - headers = ['Counter'] + [f'Channel{i+1}' for i in range(self.num_channels)] - self.csv_writer = csv.writer(self.csv_file) - self.csv_writer.writerow(headers) + with open(filename, 'w', newline='') as f: + self.csv_writer = csv.writer(f) + headers = ['Counter'] + [f'Channel{i+1}' for i in range(self.num_channels)] + self.csv_writer.writerow(headers) + # Re-open in append mode for the streaming loop + self.csv_file = open(filename, 'a', newline='') + self.csv_writer = csv.writer(self.csv_file)🧰 Tools
🪛 Ruff (0.8.2)
63-63: Use a context manager for opening files
(SIM115)
static/style.css (3)
333-344:.header h1rules are duplicated – keep a single source of truthThese lines repeat the exact style block already defined at ~35-46. Duplicate selectors bloat the sheet and increase the risk of the two blocks getting out-of-sync in future edits.
-/* REMOVE duplicated .header h1 block (already defined above) */ -.header h1 { - font-size: 48px; - font-weight: bold; - color: #333; - margin: 0 0 15px 0; - background: linear-gradient(90deg, #ec4899, #a855f7, #3b82f6, #ec4899); - background-size: 300%; - -webkit-background-clip: text; - background-clip: text; - -webkit-text-fill-color: transparent; - animation: scroll-gradient 5s linear infinite; -}
452-475:protocol-button-group&protocol-buttons-combined buttonblocks appear twiceThe definitions at 55-62 and here are identical. Consider extracting common button styling into one block and using utility classes or CSS variables to override only what differs (e.g. size in media queries).
This will slim the stylesheet and make future tweaks less error-prone.
287-308: Back-to-back identical@media (max-width: 600px)queriesTwo consecutive media queries target the same breakpoint with overlapping rules for the same selectors. Merge them to reduce CSS size and ensure later edits remain consistent.
Example:
@media (max-width: 600px) { - /* first block … */ -} - -@media (max-width: 600px) { - /* second block … */ + /* merged declarations … */ }app.py (1)
172-176: Terminated subprocesses may linger as zombies
running_apps[app_name].terminate()sends SIGTERM but you neverwait()on the process. On POSIX systems this leaves a zombie until the parent collects the exit status.running_apps[app_name].terminate() -running_apps.pop(app_name, None) +running_apps[app_name].wait(timeout=5) +running_apps.pop(app_name, None)Optionally use
kill()after a grace period.templates/index.html (1)
136-141: CSV toggle is permanently disabled after initiating a connection
setCSVToggleState(false)is called at the start of every connect function but never reset totrueupon successful connection, so users cannot toggle CSV logging for subsequent sessions without refreshing.Add a call after a successful connection:
// inside success branch enableAllAppButtons(); setCSVToggleState(true);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app.py(1 hunks)beetle.py(7 hunks)chords.py(0 hunks)chords_ble.py(1 hunks)chords_serial.py(1 hunks)chords_wifi.py(1 hunks)connection.py(1 hunks)emgenvelope.py(4 hunks)eog.py(3 hunks)ffteeg.py(4 hunks)gui.py(5 hunks)heartbeat_ecg.py(4 hunks)keystroke.py(2 hunks)npg-ble.py(0 hunks)static/style.css(3 hunks)templates/index.html(2 hunks)
💤 Files with no reviewable changes (2)
- chords.py
- npg-ble.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
keystroke.py (1)
eog.py (1)
detect_blinks(146-165)
chords_wifi.py (2)
chords_ble.py (2)
connect(161-172)cleanup(151-159)chords_serial.py (1)
cleanup(138-149)
🪛 Ruff (0.8.2)
chords_ble.py
75-75: Local variable channels is assigned to but never used
Remove assignment to unused variable channels
(F841)
chords_serial.py
61-61: Local variable sampling_rate is assigned to but never used
Remove assignment to unused variable sampling_rate
(F841)
connection.py
7-7: sys imported but unused
Remove unused import: sys
(F401)
63-63: Use a context manager for opening files
(SIM115)
chords_wifi.py
5-5: scipy.signal.butter imported but unused
Remove unused import
(F401)
5-5: scipy.signal.filtfilt imported but unused
Remove unused import
(F401)
67-67: Local variable sps is assigned to but never used
Remove assignment to unused variable sps
(F841)
68-68: Local variable fps is assigned to but never used
Remove assignment to unused variable fps
(F841)
69-69: Local variable bps is assigned to but never used
Remove assignment to unused variable bps
(F841)
120-120: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
135-135: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
app.py
5-5: webbrowser imported but unused
Remove unused import: webbrowser
(F401)
🔇 Additional comments (24)
ffteeg.py (4)
12-12: Added time module import for disconnection detection.Adds the time module to enable tracking timestamps for disconnection detection.
21-22: Added stream state tracking for disconnection detection.Good addition of variables to track stream state and detect disconnections:
stream_activeflag to maintain stream statuslast_data_timeto track when data was last receivedThis enables graceful shutdown on disconnection.
115-115: Updated timestamp tracking for disconnection detection.Records the current time whenever data is received from the LSL stream, supporting the disconnection detection mechanism.
137-143: Added disconnection detection and graceful shutdown.Good implementation of disconnection detection logic:
- Checks if more than 2 seconds have passed since last data reception
- Sets the stream status flag to inactive
- Provides console feedback
- Stops the UI update timer
- Gracefully closes the application window
This prevents the application from freezing or displaying stale data when the stream is lost.
beetle.py (11)
5-5: Added sys module import for error handling.The sys module is necessary for the sys.exit() calls added for proper error handling.
16-18: Added stream state tracking variables.Good addition of variables to track stream state:
last_data_timeto record timestamp of last received datastream_activeflag to maintain connection statusinletexplicit initialization to None for better error handling
22-22: Added proper error handling for missing streams.Improved error handling by properly exiting when no LSL streams are found rather than continuing execution.
37-37: Added proper error handling for connection failure.Ensured the application properly exits when unable to connect to any stream.
81-84: Added event handling for graceful exit.Good addition of event handling during message display to allow users to close the window at any time, ensuring the application exits cleanly rather than hanging.
113-113: Added global declaration for last_data_time.Ensures the global variable is properly accessed within the calibration function.
135-135: Added timestamp tracking during calibration.Records the time when data is received during calibration to support disconnection detection.
138-142: Added disconnection detection during calibration.Good implementation of disconnection detection during the critical calibration phase:
- Checks if 2+ seconds passed since last data reception
- Shows a user-friendly message
- Properly cleans up pygame resources
- Exits with an error code
144-147: Added event handling during calibration.Important addition of event handling to allow users to exit during the calibration process.
197-197: Added timestamp tracking in main game loop.Records the time when data is received in main game loop to support disconnection detection.
200-204: Added disconnection detection in main game loop.Good implementation of disconnection detection during the main game loop:
- Checks if 2+ seconds passed since last data reception
- Shows a user-friendly message
- Sets the running flag to false to exit the loop gracefully
- Uses break to immediately exit the loop
heartbeat_ecg.py (4)
10-10: Added time module import for disconnection detection.Adds the time module to enable tracking timestamps for disconnection detection.
19-20: Added stream state tracking for disconnection detection.Good addition of variables to track stream state and detect disconnections:
stream_activeflag to maintain stream statuslast_data_timeto track when data was last receivedThis enables graceful shutdown on disconnection.
114-114: Updated timestamp tracking for disconnection detection.Records the current time whenever data is received from the LSL stream, supporting the disconnection detection mechanism.
128-133: Added disconnection detection and graceful shutdown.Good implementation of disconnection detection logic:
- Checks if more than 2 seconds have passed since last data reception
- Sets the stream status flag to inactive
- Provides console feedback
- Stops the UI update timer
- Gracefully closes the application window
This prevents the application from freezing or displaying stale data when the stream is lost.
eog.py (3)
17-18: Added stream state tracking for disconnection detection.Good addition of variables to track stream state and detect disconnections:
stream_activeflag to maintain stream statuslast_data_timeto track when data was last receivedThis enables graceful shutdown on disconnection.
105-105: Updated timestamp tracking for disconnection detection.Records the current time whenever data is received from the LSL stream, supporting the disconnection detection mechanism.
139-144: Added disconnection detection and graceful shutdown.Good implementation of disconnection detection logic:
- Checks if more than 2 seconds have passed since last data reception
- Sets the stream status flag to inactive
- Provides console feedback
- Stops the UI update timer
- Gracefully closes the application window
This prevents the application from freezing or displaying stale data when the stream is lost.
static/style.css (1)
7-12: Avoidoverflow: hiddenon<body>– it prevents scrolling on small screensLocking the viewport with
height: 100vh; overflow: hidden;can make the entire UI unreachable / un-scrollable on devices where the content vertically exceeds 100 % of the viewport (mobile browsers add a dynamic URL bar, virtual keyboard, etc.).
Consider either:-body { - height: 100vh; - overflow: hidden; +body { + min-height: 100vh; /* allow taller content */ + overflow-y: auto; /* enable scrolling when needed */ }or moving the height/overflow constraints to a subordinate wrapper that truly needs to be fixed-height (e.g. a splash screen).
templates/index.html (1)
513-578: Client expects/disconnect– update JS once the backend route is addedOnce you add the Flask
/disconnectroute (see backend comment), remember to check the response here and surface errors to the user instead of refreshing regardless of failure.const resp = await fetch('/disconnect', { method: 'POST' }); const data = await resp.json(); if (data.status !== 'success') { console.error(data.message); statusDiv.textContent = data.message || 'Disconnection failed'; statusDiv.className = 'error-status'; return; // skip reload }
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
templates/index.html (1)
6-8: Title update and icon library version
The<title>refactor looks good. For the Font Awesome import, consider upgrading to a more recent major version (5.x or 6.x) to benefit from additional icons and security patches.static/style.css (4)
7-8: Revisitoverflow: hiddenon<body>
Hiding overflow on the<body>can prevent scrolling when content (e.g., the BLE popup) exceeds viewport height on smaller screens. Consider removing or toggling it only when needed (e.g., when the popup is shown).
35-46: Duplicate.header h1styles
The gradient-animated header text rules are defined twice (lines 35–46 and 343–354). Consolidate into a single block to avoid maintenance overhead.Also applies to: 343-354
55-61: Consolidate duplicated.protocol-button-grouprules
The.protocol-button-groupselector appears twice with identical properties. Merge them into one definition to DRY up the CSS.Also applies to: 462-468
275-300: Duplicate media queries formax-width: 600px
Two nearly identical blocks target the same breakpoint. Combine these into a single@media (max-width: 600px)section to avoid inconsistencies.Also applies to: 302-323
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
templates/index.html (3)
32-34: UndefinedconnectDefault()function
The generic "Connect" button callsconnectDefault(), but this function is not defined in your JavaScript. This will throw a runtime error. Either implementconnectDefault()to delegate based on the selected protocol or remove/wire this button directly to the protocol connect functions.
133-137: MissingcsvToggleelement
The script referencesdocument.getElementById('csvToggle'), but there is no element with this ID in the markup. This will throw at runtime. Consider adding a corresponding<input type="checkbox" id="csvToggle">or update the JS to use the existing control (e.g., the CSV record button).
145-150: Commented-outsetCSVToggleStatefunction
ThesetCSVToggleStatehelper is fully commented out, yet it is invoked in the connection flows. Uncomment or reimplement it to avoid undefined-function errors when toggling CSV recording state.
🧹 Nitpick comments (5)
templates/index.html (1)
37-37: Avoid inline visibility styles
The.disconnect-buttoncontainer usesstyle="display: none;"inline. Consider moving this to a CSS rule (e.g., a.hiddenclass) and toggling visibility via JS to improve maintainability.static/style.css (4)
7-11: Avoid usingoverflow: hiddenand fixed height onbody
Settingheight: 100vhandoverflow: hiddenon the<body>prevents scrolling and may cause content to be clipped on smaller viewports. Consider usingmin-heightand allowingoverflow: autoor handling overflow per container to improve responsiveness.
15-26: Review fixed height on.container
Usingheight: 95vhon the main container may lead to layout issues on shorter screens or when content grows. Consider switching tomax-heightwithoverflow-y: autoto handle dynamic content and improve mobile support.
180-197: Style container for.disconnect-button
You have a<div class="disconnect-button">wrapper in the HTML, but no corresponding CSS rule targeting.disconnect-button. If you need specific positioning or animations for this container, add styles to ensure consistent layout.
100-115: Accessibility: Add focus styles to interactive buttons
The.protocol-btnselector does not define any:focusstyles, which may hinder keyboard navigation. Consider adding visible outlines or custom focus indicators for better accessibility (WCAG).
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
app.py (1)
131-132:⚠️ Potential issueDebug mode enabled in production.
Running Flask with
debug=Truein production is a security risk as it exposes sensitive information and enables the interactive debugger.Modify to use environment variables for configuration:
if __name__ == "__main__": - app.run(debug=True) + import os + debug_mode = os.environ.get('FLASK_DEBUG', 'False').lower() == 'true' + app.run(debug=debug_mode)🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 131-131: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/(debug-enabled-python)
🧹 Nitpick comments (3)
app.py (1)
20-29: Manual asyncio wrapper function can be replaced with Flask-specific solution.The custom
run_asyncdecorator is implementing functionality that can be better handled by existing solutions likequartorflask_asyncextensions, which provide better integration with Flask's lifecycle and error handling.Consider using a Flask-specific async extension like Quart or flask_async instead of implementing a custom decorator. This would also help with proper cleanup of asyncio resources.
templates/index.html (2)
531-539: Duplicate DOM element queries inupdateConnectionStatus.The function re-queries DOM elements that are already available in the global scope, which is inefficient.
Remove redundant DOM queries by using the global variables:
// Function to update the connection status on the frontend function updateConnectionStatus(message, icon, colorClass) { - const statusText = document.getElementById('status-text'); - const statusIcon = document.getElementById('status-icon'); - const statusDiv = document.getElementById('connection-status'); statusText.textContent = message; statusIcon.innerHTML = `<i class="fas ${icon} ${colorClass}"></i>`; statusDiv.classList.remove('hidden'); }
129-134: Typo in one of the app titles.There's a typo in the app title "EOG Keystroke Emualtor" - should be "Emulator".
Fix the typo in the app title:
const apps = [ { title: 'ECG with Heart Rate', icon: 'fa-heartbeat', color: 'cyan' }, { title: 'EMG with Envelope', icon: 'fa-wave-square', color: 'red' }, { title: 'EOG with Blinks', icon: 'fa-chart-line', color: 'purple' }, { title: 'EEG with FFT', icon: 'fa-hand-holding-water', color: 'green' }, { title: 'EEG Tug of War game', icon: 'fa-brain', color: 'yellow' }, { title: 'EEG Beetle Game', icon: 'fa-eye', color: 'pink' }, - { title: 'EOG Keystroke Emualtor', icon: 'fa-lungs', color: 'blue' }, + { title: 'EOG Keystroke Emulator', icon: 'fa-lungs', color: 'blue' }, { title: 'GUI Visualization', icon: 'fa-heart-pulse', color: 'orange' }, { title: 'CSV Plotter', icon: 'fa-thermometer-half', color: 'teal' }, ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.py(1 hunks)static/style.css(0 hunks)templates/index.html(1 hunks)
💤 Files with no reviewable changes (1)
- static/style.css
🧰 Additional context used
🪛 Ruff (0.8.2)
app.py
9-9: Redefinition of unused threading from line 3
Remove definition: threading
(F811)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
templates/index.html (5)
2-2: Review static dark mode class
Havingclass="dark"on the<html>tag forces the dark theme before your script runs and may cause a flash of dark styling even if the user prefers light. Consider removing the staticdarkclass and relying solely on your JS initialization (localStorage orprefers-color-scheme) to set the theme.
7-7: Pin Font Awesome to a maintained version
You’re using Font Awesome v6.0.0 from the CDN. That version is over a year old—consider updating to the latest stable release or a security-patched version to avoid known vulnerabilities.
17-25: Add ARIA labels for icon buttons
Buttons like the theme-toggle, GitHub, and info buttons only contain<i>icons, which are not announced by screen readers. Addaria-label(oraria-labelledby) attributes to each button to improve accessibility.
Example:-<button id="theme-toggle" class="text-white hover:text-cyan-400 transition-colors"> +<button id="theme-toggle" aria-label="Toggle theme" class="text-white hover:text-cyan-400 transition-colors">
597-599: Display console messages in the UI
Currently,eventSource.onmessagesimply logs updates to the browser console. To give users live feedback, append incoming messages to a scrollable container (e.g.,<div id="console-container">) in the page instead of—or in addition to—console.log().
727-731: Replace polling with SSE/WebSocket
You’re polling/check_streamevery second, which can add unnecessary HTTP overhead. Consider pushing status updates over your existing SSE/console_updatesstream or upgrading to a WebSocket solution to drive UI state changes in real time.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
templates/index.html (2)
599-601: Render console updates in the UI
Currentlyonmessageonly logs to console. To surface real-time feedback, append messages to a scrollable container in the DOM (as recommended earlier).
145-154: 🛠️ Refactor suggestionDynamic Tailwind classes may be purged
Using template literals likebg-${app.color}-100andtext-${app.color}-600can be stripped out by PurgeCSS. Either safelist these patterns intailwind.config.jsor mapapp.colorto explicit class strings so PurgeCSS recognizes them.
🧹 Nitpick comments (5)
templates/index.html (5)
2-2: Remove default dark theme class
Settingclass="dark"on the<html>tag forces dark mode before your script runs, causing a flash of dark UI for users who prefer light theme. Consider removing this default and relying solely on your JS initialization to applydarkbased onlocalStorageorprefers-color-scheme.
6-7: Add SRI and crossorigin to CDN links
Including integrity (integrity="") andcrossorigin="anonymous"attributes on your Font Awesome (and later Tailwind) CDN imports will harden against CDN tampering.
81-94: Enhance modal accessibility
The BLE device modal is great, but consider addingrole="dialog",aria-modal="true", and proper focus trapping when it’s shown to improve screen-reader and keyboard navigation support.Also applies to: 96-99
181-189: Prevent flash of wrong theme on load
You initialize theme after DOM load, but sincedarkmay be on<html>, users can see a brief incorrect theme. Consider inlining a small script in<head>to set thedarkclass before rendering, or remove the staticdarkclass and rely exclusively on the inline script.
603-608: Implement SSE reconnection strategy
Add retry logic with exponential backoff ineventSource.onerrorto handle transient network disruptions and improve resilience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/index.html(1 hunks)
🔇 Additional comments (24)
templates/index.html (24)
9-18: Approve body and header markup
The<body>layout classes and sticky header implementation using Tailwind utilities look solid and responsive.
30-38: Approve connectivity and recording UI markup
The protocol selection buttons, Connect/Disconnect controls, filename input group, recording toggle, and status display are well-structured with clear IDs/classes matching your JS.Also applies to: 47-51, 53-55, 59-66, 68-72, 74-77
103-116: Approve applications filter and grid layout
The Select dropdown for category filtering and the responsive grid container (#app-grid) correctly set up the applications section.Also applies to: 119-120
131-142: Approve apps metadata initialization
Defining theappsarray and fetching the#app-gridelement is clear and concise.Also applies to: 143-143
157-164: Approve application launch click handler
CheckingisConnectedbefore launching and invokinglaunchApplication(app.script)provides a clear flow and user feedback.
168-176: Approve theme toggle logic
ThetoggleThemefunction and event binding correctly switch icons and persist the user’s preference inlocalStorage.Also applies to: 179-179
192-205: Approve connection state variables
DefiningselectedProtocol,selectedBleDevice,isConnected,isRecording, andeventSourceat the top clearly captures the global state required for connection management.Also applies to: 207-213
215-223: Approve filename utilities
YourgetTimestamp(),initializeFilename(),sanitizeFilename(), and input listener correctly generate and sanitize filenames, preserving cursor position.Also applies to: 252-257, 259-266, 268-276
279-292: Approve protocol selection handler
The click listener on.connection-btnelements cleanly highlights the selected protocol, manages modal display for BLE, and enables the Connect button appropriately.
295-303: Approve BLE scanning flow
YourscanBleDevices()function guards against duplicate scans, shows loading state, fetches/scan_ble, and handles both success and error cases with user feedback.Also applies to: 305-313, 319-324, 331-339
343-353: Approve BLE device list rendering and selection
TherenderBleDevices()logic creates clickable device entries, highlights the selected one, and ensures the Connect button is enabled only after a selection.Also applies to: 355-365, 368-375
379-385: Approve BLE modal controls
The modal close and rescan button handlers properly reset state and re-trigger scanning.Also applies to: 387-389
391-401: Approve Connect button click handler
The validation (protocol & BLE device), UI transitions to “Connecting…”, payload construction,fetch('/connect'), and polling viapollConnectionStatus()are well-structured and error-handled.Also applies to: 404-442
445-488: Approve polling connection status logic
Your recursivecheckStatus()with timeout and max attempts provides a robust approach to detect connection success or failure.
490-515: Approve connection success handling
On successful connect, you update UI controls, disable other protocols, and trigger console updates via SSE.
517-524: Approve application launch API integration
launchApplication()cleanly posts to/launch_appand handles both success and failure JSON responses.Also applies to: 528-533
536-586: Approve Disconnect button handler and cleanup
The disconnect flow—including UI state transitions,/disconnectcall, resetting buttons, stopping recording, and closing SSE—is comprehensive and properly error-handled.
592-610: Approve SSE setup for console updates
Usingnew EventSource('/console_updates')and cleaning up existing connections avoids duplicate streams.
613-620: Approve recording toggle implementation
ThetoggleRecording()function correctly handles start/stop flows, updates button labels/styles, and interacts with/start_recordingand/stop_recordingendpoints.Also applies to: 623-628, 630-641, 645-661, 663-667
671-674: Approve initial UI setup
Disabling Connect until protocol selection and initializing filename on load ensures a consistent starting state.
674-678: Approve status and alert helpers
showStatus()andshowAlert()provide centralized feedback mechanisms for various interactions.Also applies to: 680-682
684-726: Approve stream status polling
Periodic/check_streampolling with UI toggles for connect/disconnect and cleanup of recording/SSE maintains sync between client and server.
729-733: Approve polling interval and bootstrap call
CallingcheckStreamStatus()immediately and then every second ensures the UI stays up-to-date without user action.
734-740: Approve external link buttons
The GitHub and Info buttons open the correct URL and display version info viaalert().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
connection.py (4)
60-82: 🛠️ Refactor suggestionUse a context manager for CSV file handling.
Opening files without a context manager could lead to resource leaks if an exception occurs. The file might not be properly closed in all scenarios.
def start_csv_recording(self, filename=None): if self.recording_active: return False try: if not filename: timestamp = datetime.now().strftime("%Y%m%d_%H%M%S") filename = f"ChordsPy_{timestamp}.csv" elif not filename.endswith('.csv'): filename += '.csv' - self.csv_file = open(filename, 'w', newline='') - headers = ['Counter'] + [f'Channel{i+1}' for i in range(self.num_channels)] - self.csv_writer = csv.writer(self.csv_file) - self.csv_writer.writerow(headers) + self.csv_file = open(filename, 'w', newline='') + try: + headers = ['Counter'] + [f'Channel{i+1}' for i in range(self.num_channels)] + self.csv_writer = csv.writer(self.csv_file) + self.csv_writer.writerow(headers) + self.recording_active = True + self.sample_counter = 0 + print(f"CSV recording started: {filename}") + return True + except Exception as e: + self.csv_file.close() + self.csv_file = None + raise e - self.recording_active = True - self.sample_counter = 0 - print(f"CSV recording started: {filename}") - return True except Exception as e: print(f"Error starting CSV recording: {str(e)}") return False🧰 Tools
🪛 Ruff (0.8.2)
71-71: Use a context manager for opening files
(SIM115)
183-220:⚠️ Potential issueNo error handling for broken WebSocket inside WiFi loop.
If the connection drops,
ws.recv()raisesWebSocketConnectionClosedException, crashing the loop. Surround the body withtry/exceptsimilar tochords_wifi.process_data()or delegate to that method instead of duplicating code.def connect_wifi(self): self.wifi_connection = Chords_WIFI() self.wifi_connection.connect() self.num_channels = self.wifi_connection.channels sampling_rate = self.wifi_connection.sampling_rate if not self.lsl_connection: self.setup_lsl(self.num_channels, sampling_rate) try: print("\nConnected! (Press Ctrl+C to stop)") while True: - data = self.wifi_connection.ws.recv() - - if isinstance(data, (bytes, list)): - for i in range(0, len(data), self.wifi_connection.block_size): - block = data[i:i + self.wifi_connection.block_size] - if len(block) < self.wifi_connection.block_size: - continue - - channel_data = [] - for ch in range(self.wifi_connection.channels): - offset = 1 + ch * 2 - sample = int.from_bytes(block[offset:offset + 2], byteorder='big', signed=True) - channel_data.append(sample) - - if self.lsl_connection: # Push to LSL - self.lsl_connection.push_sample(channel_data) - if self.recording_active: - self.log_to_csv(channel_data) + try: + data = self.wifi_connection.ws.recv() + + if isinstance(data, (bytes, list)): + for i in range(0, len(data), self.wifi_connection.block_size): + block = data[i:i + self.wifi_connection.block_size] + if len(block) < self.wifi_connection.block_size: + continue + + channel_data = [] + for ch in range(self.wifi_connection.channels): + offset = 1 + ch * 2 + sample = int.from_bytes(block[offset:offset + 2], byteorder='big', signed=True) + channel_data.append(sample) + + if self.lsl_connection: # Push to LSL + self.lsl_connection.push_sample(channel_data) + if self.recording_active: + self.log_to_csv(channel_data) + except (websocket.WebSocketConnectionClosedException, ConnectionResetError) as e: + print(f"\nConnection closed: {str(e)}") + break + except Exception as e: + print(f"\n[ERROR] Connection error: {e}") + break except KeyboardInterrupt: - self.wifi_connection.disconnect() + self.wifi_connection.cleanup() print("\nDisconnected") finally: self.stop_csv_recording()
216-217:⚠️ Potential issue
disconnect()is undefined forChords_WIFI.
Chords_WIFIonly exposescleanup(). Calling a non-existent method will raiseAttributeError.- self.wifi_connection.disconnect() + self.wifi_connection.cleanup()
248-254:⚠️ Potential issue
disconnect()is undefined forChords_WIFI.Similar to the previous issue,
disconnect()is called in the cleanup method, butChords_WIFIonly exposescleanup().if self.wifi_connection: try: - self.wifi_connection.disconnect() + self.wifi_connection.cleanup() print("WiFi connection closed") except Exception as e: print(f"Error closing WiFi connection: {str(e)}") finally: self.wifi_connection = Nonetemplates/index.html (4)
145-154: Tailwind dynamic classes may be purged.The app-card template uses dynamic class names (
bg-${app.color}-100,dark:bg-${app.color}-900,text-${app.color}-600, etc.). Tailwind's PurgeCSS won't detect these at build time and may strip them out.You have two options:
- Safelist all variants of these classes in your
tailwind.config.js:// tailwind.config.js module.exports = { // ... safelist: [ 'bg-cyan-100', 'bg-cyan-900', 'text-cyan-600', 'text-cyan-300', 'bg-red-100', 'bg-red-900', 'text-red-600', 'text-red-300', // Add other color combinations used ] }
- Use a lookup object instead of dynamic interpolation:
const colorClasses = { cyan: { bg: 'bg-cyan-100', bgDark: 'dark:bg-cyan-900', text: 'text-cyan-600', textDark: 'dark:text-cyan-300' }, red: { // ...other colors } } // Then in your template: <div class="${colorClasses[app.color].bg} ${colorClasses[app.color].bgDark} ...">
612-631: Enhance SSE reconnection logic.The
startConsoleUpdatesfunction tears down theEventSourceon error but never attempts to reconnect, which may result in loss of console updates if there's a temporary network issue.function startConsoleUpdates() { if (eventSource) { eventSource.close(); } eventSource = new EventSource('/console_updates'); eventSource.onmessage = function(e) { console.log('Console update:', e.data); }; + let reconnectAttempts = 0; + const maxReconnectAttempts = 5; + eventSource.onerror = function() { console.error('EventSource failed'); + + // Handle reconnection + if (reconnectAttempts < maxReconnectAttempts) { + reconnectAttempts++; + const timeout = Math.min(1000 * reconnectAttempts, 5000); // Exponential backoff with cap + console.log(`Attempting to reconnect in ${timeout/1000} seconds...`); + setTimeout(startConsoleUpdates, timeout); + } else { + console.error('Maximum reconnection attempts reached'); + showStatus('Connection lost', 'fa-exclamation-circle', 'text-orange-500'); + } + if (eventSource) { eventSource.close(); eventSource = null; } }; }
619-622: Add UI console message display.The EventSource for console updates correctly receives data, but the messages aren't displayed to the user in the UI.
eventSource.onmessage = function(e) { console.log('Console update:', e.data); + // Display message in UI if it's not a heartbeat + if (e.data !== "heartbeat") { + const consoleDiv = document.createElement('div'); + consoleDiv.className = 'text-sm text-gray-600 dark:text-gray-400 my-1'; + consoleDiv.textContent = e.data; + + // Find a container to append the message, or create one if it doesn't exist + let consoleContainer = document.getElementById('console-container'); + if (!consoleContainer) { + consoleContainer = document.createElement('div'); + consoleContainer.id = 'console-container'; + consoleContainer.className = 'mt-2 py-2 px-4 border rounded-lg bg-gray-50 dark:bg-gray-800 max-h-40 overflow-y-auto'; + statusDiv.parentNode.insertBefore(consoleContainer, statusDiv.nextSibling); + } + + consoleContainer.appendChild(consoleDiv); + consoleContainer.scrollTop = consoleContainer.scrollHeight; + } };
767-779: 🛠️ Refactor suggestionMove Tailwind config before CDN import.
Your
tailwind.configscript appears after loadinghttps://cdn.tailwindcss.com, so customizations won't apply on first render.- <script src="https://cdn.tailwindcss.com"></script> - <script> - tailwind.config = { - darkMode: 'class', - theme: { - extend: { - cursor: { - 'not-allowed': 'not-allowed', - } - } - } - } - </script> + <script> + tailwind.config = { + darkMode: 'class', + theme: { + extend: { + cursor: { + 'not-allowed': 'not-allowed', + } + } + } + } + </script> + <script src="https://cdn.tailwindcss.com"></script>
🧹 Nitpick comments (5)
chords_serial.py (3)
5-5: Remove unused import.The
sysmodule is imported but never used in the codebase.-import sys🧰 Tools
🪛 Ruff (0.8.2)
5-5:
sysimported but unusedRemove unused import:
sys(F401)
61-61: Unused variable assignment.The
sampling_ratevariable is assigned but never used within this function.- sampling_rate = self.supported_boards[self.board]["sampling_rate"]If this was for future reference or debugging, consider adding a comment explaining its purpose.
🧰 Tools
🪛 Ruff (0.8.2)
61-61: Local variable
sampling_rateis assigned to but never usedRemove assignment to unused variable
sampling_rate(F841)
153-156: Missing error handling in main block.The main block runs the hardware detection and streaming without any error handling, which could lead to unhandled exceptions if hardware detection fails.
if __name__ == "__main__": client = Chords_USB() - client.detect_hardware() - client.start_streaming() + try: + if client.detect_hardware(): + client.start_streaming() + else: + print("No compatible hardware detected. Exiting.") + except KeyboardInterrupt: + print("\nProgram terminated by user.") + except Exception as e: + print(f"Error: {str(e)}") + finally: + client.cleanup()connection.py (1)
7-7: Remove unused import.The
sysmodule is imported but never used in the codebase.-import sys🧰 Tools
🪛 Ruff (0.8.2)
7-7:
sysimported but unusedRemove unused import:
sys(F401)
templates/index.html (1)
705-751: Make connection status checking more explicit.The check for connection status uses
if (data.connected)which could be more explicit to avoid type coercion issues.function checkStreamStatus() { fetch('/check_stream') .then(response => response.json()) .then(data => { - if (data.connected) { + if (data.connected === true) { // If connected, update the frontend if (!isConnected) { // ... } } else { // ... } }) // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
chords_serial.py(1 hunks)connection.py(1 hunks)templates/index.html(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
chords_serial.py (3)
chords_wifi.py (1)
cleanup(115-124)connection.py (1)
cleanup(221-258)chords_ble.py (1)
cleanup(151-159)
connection.py (3)
chords_serial.py (5)
Chords_USB(9-151)detect_hardware(73-84)read_data(96-126)start_streaming(128-136)cleanup(142-148)chords_wifi.py (3)
Chords_WIFI(7-127)connect(25-34)cleanup(115-124)chords_ble.py (5)
Chords_BLE(8-178)scan_devices(36-45)notification_handler(82-93)connect(161-172)cleanup(151-159)
🪛 Ruff (0.8.2)
chords_serial.py
5-5: sys imported but unused
Remove unused import: sys
(F401)
61-61: Local variable sampling_rate is assigned to but never used
Remove assignment to unused variable sampling_rate
(F841)
connection.py
7-7: sys imported but unused
Remove unused import: sys
(F401)
71-71: Use a context manager for opening files
(SIM115)
🔇 Additional comments (18)
chords_serial.py (7)
31-42: Good initialization practice with signal handler thread safety.The initialization is well-designed with appropriate member variables and thread-safe signal handler registration limited to the main thread to avoid duplicate signal handlers in threaded environments.
44-72: Well-implemented hardware connection with robust retry mechanism.The connection implementation includes proper error handling, retries, and validation of device responses. It correctly configures the packet length and data arrays based on the detected board.
🧰 Tools
🪛 Ruff (0.8.2)
61-61: Local variable
sampling_rateis assigned to but never usedRemove assignment to unused variable
sampling_rate(F841)
73-85: Efficient hardware detection implementation.The method appropriately tries different baudrates and ports combinations to detect compatible hardware, with clean handling of success and failure cases.
86-95: Clean serial command implementation.The send_command method properly flushes I/O buffers, adds error tolerance with utf-8 decoding, and handles the possibility of a closed serial connection.
96-127: Robust packet parsing with proper error handling.The data reading implementation includes:
- Proper detection of serial disconnection
- Efficient packet boundary detection
- Validation of packet structure with sync and end bytes
- Clean extraction of multi-channel sensor data
- Appropriate handling of serial exceptions with cleanup
This is a well-implemented data acquisition routine for binary packet protocols.
128-137: Streaming implementation with proper exception handling.The streaming implementation starts the data flow and handles keyboard interrupts appropriately by triggering cleanup.
142-149: Clean resource management in cleanup method.The cleanup method properly stops streaming and closes serial connections with exception handling, which is essential for proper resource management.
connection.py (6)
13-32: Well-structured Connection class initialization.The Connection class has a clean initialization with appropriate attributes for different connection types, LSL stream configuration, and state tracking.
33-52: Good implementation of BLE device discovery and selection.The async method for BLE device discovery presents devices to the user and handles selection appropriately, with proper error handling for user input and cancellation.
111-155: Robust BLE connection handling with notification interception.The connection method correctly wraps the original notification handler to intercept data for LSL streaming and CSV recording. It properly handles device address selection and user interaction.
157-181: Clean USB connection implementation with thread safety.The USB connection method properly detects hardware and sets up a background thread for streaming with daemon=True for automatic cleanup. The method wraps the original read_data method to intercept data for LSL and CSV recording without modifying the original class.
221-258: Well-implemented cleanup with proper resource management.The cleanup method properly handles all connection types, stops CSV recording, and resets stream flags. It includes proper exception handling and resource cleanup in finally blocks.
263-282: Well-designed CLI entry point.The main function provides a well-structured command-line interface for connecting to devices with proper error handling and protocol selection.
templates/index.html (5)
1-9: Clean HTML structure with dark mode support.The HTML is well-structured with proper meta tags, title, font awesome integration, and dark mode enabled by default.
10-28: Responsive header with theme toggling and navigation.The header is well-designed with a gradient title, theme toggle, GitHub link, and info button using Tailwind's responsive utility classes.
30-78: Well-organized connectivity section with clear protocol options.The connectivity section provides a clear interface for protocol selection (WiFi, Bluetooth, Serial), connection controls, filename input with CSV extension, and recording functionality. The layout is responsive with appropriate spacing and visual hierarchy.
80-101: Well-designed BLE device selection modal.The BLE device selection modal provides a clean interface for scanning and selecting devices with appropriate loading states and rescan functionality.
103-122: Responsive application grid with filtering.The application section provides a filterable grid of applications with appropriate spacing and responsive layouts using Tailwind's grid system.
PayalLakra
changed the title
|
@lorforlinux
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
app.py (7)
60-66: Inconsistent stream status managementThis function updates a global
stream_activevariable, which duplicates the state already tracked inconnection_manager.stream_active. This can lead to inconsistent state management.def post_console_message(message): - global stream_active + global connection_manager if "LSL stream started" in message: - stream_active = True + if connection_manager: + connection_manager.stream_active = True elif "disconnected" in message: - stream_active = False + if connection_manager: + connection_manager.stream_active = False console_queue.put(message)
68-75: Potential resource leak in event streamThe
console_updatesfunction creates an infinite loop that will run until the client disconnects. There's no mechanism to detect client disconnection or to stop the stream gracefully, which could lead to resource leaks.@app.route('/console_updates') def console_updates(): def event_stream(): while True: - message = console_queue.get() + try: + message = console_queue.get(timeout=30) # Add timeout to prevent indefinite blocking + except queue.Empty: + # Send a heartbeat to detect disconnected clients + yield "data: heartbeat\n\n" + continue yield f"data: {message}\n\n" return Response(event_stream(), mimetype="text/event-stream")
117-124: Redundant stream status settingThe code references a global variable
stream_activethat duplicates state inconnection_manager.stream_active. This should be simplified.def connect_device(): - global connection_manager, connection_thread, stream_active + global connection_manager, connection_thread data = request.get_json() protocol = data.get('protocol') device_address = data.get('device_address') # Reset stream status - stream_active = False + if connection_manager: + connection_manager.stream_active = False
127-131: Race condition risk in connection cleanupThere's a potential race condition in the cleanup process. If
connection_threadis running, the code joins it but doesn't set a timeout, which could block indefinitely if the thread is stuck.Apply this diff to add a timeout to the join operation:
# Clean up any existing connection if connection_manager: connection_manager.cleanup() if connection_thread and connection_thread.is_alive(): - connection_thread.join() + connection_thread.join(timeout=5) # Add timeout to prevent indefinite blocking + if connection_thread.is_alive(): + logging.warning("Connection thread did not terminate within timeout")
141-150: BLE connection creates but doesn't close asyncio event loopFor BLE connections, an event loop is created but never explicitly closed, which may lead to resource leaks and warnings. The
finallyblock should close the loop.Apply this diff to fix the issue:
elif protocol == 'ble': if not device_address: logging.error("No BLE device address provided") return # For BLE, we need to run in an event loop loop = asyncio.new_event_loop() asyncio.set_event_loop(loop) - success = connection_manager.connect_ble(device_address) + try: + success = connection_manager.connect_ble(device_address) + finally: + loop.close()
165-173: Inconsistent stream status management in disconnect handlerThe disconnect handler updates a global
stream_activevariable, but it should be usingconnection_manager.stream_activeto maintain consistency with the rest of the code.@app.route('/disconnect', methods=['POST']) def disconnect_device(): - global connection_manager, stream_active + global connection_manager if connection_manager: connection_manager.cleanup() - stream_active = False post_console_message("disconnected") return jsonify({'status': 'disconnected'}) return jsonify({'status': 'no active connection'})
211-212: Debug mode enabled in production codeRunning Flask with
debug=Trueis not recommended for production environments as it exposes sensitive information and can be a security risk.if __name__ == "__main__": - app.run(debug=True) + app.run(debug=False)For development, consider using environment variables to control debug mode:
import os if __name__ == "__main__": debug_mode = os.environ.get('FLASK_DEBUG', 'False').lower() == 'true' app.run(debug=debug_mode)🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 211-211: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/(debug-enabled-python)
templates/index.html (4)
130-142: Move Tailwind config before CDN importYour
tailwind.configscript appears after loadinghttps://cdn.tailwindcss.com, so customizations won't apply on first render. Per the CDN docs, inline the config script before the CDN<script>tag.<footer class="bg-gray-900 dark:bg-gray-800 text-white sticky bottom-0 z-50"> <div class="max-w-full px-8 py-3 text-center"> Made with ❤️ at Upside Down Labs </div> </footer> + <script> + tailwind.config = { + darkMode: 'class', + theme: { + extend: { + cursor: { + 'not-allowed': 'not-allowed', + } + } + } + } + </script> <script src="https://cdn.tailwindcss.com"></script> - <script> - tailwind.config = { - darkMode: 'class', - theme: { - extend: { - cursor: { - 'not-allowed': 'not-allowed', - } - } - } - } - </script>
158-167: Tailwind dynamic classes may be purgedThe app-card template uses dynamic class names (
bg-${app.color}-100,dark:bg-${app.color}-900,text-${app.color}-600, etc.). Tailwind's PurgeCSS won't detect these at build time and may strip them out.You have two options:
- Safelist all variants of these classes in your
tailwind.config.jssafelistarray.- Build the class list explicitly (e.g., use a lookup object) so that PurgeCSS sees the full set of class names.
For option 2, you could use a lookup object:
const colorClasses = { cyan: { bg: 'bg-cyan-100', bgDark: 'dark:bg-cyan-900', text: 'text-cyan-600', textDark: 'dark:text-cyan-300' }, red: { bg: 'bg-red-100', bgDark: 'dark:bg-red-900', text: 'text-red-600', textDark: 'dark:text-red-300' }, // Add entries for all your colors }; // Then in your template: `<div class="w-full h-32 ${colorClasses[app.color].bg} ${colorClasses[app.color].bgDark} rounded-lg flex items-center justify-center mb-4"> <i class="fas ${app.icon} text-4xl ${colorClasses[app.color].text} ${colorClasses[app.color].textDark}"></i> </div>`
625-644: Enhance SSE reconnection logicThe
startConsoleUpdatesfunction tears down theEventSourceon error but never attempts to reconnect. For more robust console streaming, implement an exponential-backoff retry strategy.function startConsoleUpdates() { if (eventSource) { eventSource.close(); } + let reconnectAttempts = 0; + const maxReconnectAttempts = 5; eventSource = new EventSource('/console_updates'); eventSource.onmessage = function(e) { console.log('Console update:', e.data); }; eventSource.onerror = function() { console.error('EventSource failed'); + + // Handle reconnection + if (reconnectAttempts < maxReconnectAttempts) { + reconnectAttempts++; + const timeout = Math.min(1000 * reconnectAttempts, 5000); // Exponential backoff with cap + console.log(`Attempting to reconnect in ${timeout/1000} seconds...`); + setTimeout(startConsoleUpdates, timeout); + } else { + console.error('Maximum reconnection attempts reached'); + showStatus('Connection lost', 'fa-exclamation-circle', 'text-orange-500'); + } + if (eventSource) { eventSource.close(); eventSource = null; } }; }
633-635: 🛠️ Refactor suggestionConsole messages not displayed in UI
The EventSource for console updates correctly receives data, but the messages aren't displayed to the user in the UI. The
onmessagehandler only logs to the console, but doesn't update the UI with the messages.eventSource.onmessage = function(e) { console.log('Console update:', e.data); + // Display message in UI if not a heartbeat + if (e.data !== "heartbeat") { + const consoleDiv = document.createElement('div'); + consoleDiv.className = 'text-sm text-gray-600 dark:text-gray-400 my-1'; + consoleDiv.textContent = e.data; + + // Find a container to append the message, or create one if it doesn't exist + let consoleContainer = document.getElementById('console-container'); + if (!consoleContainer) { + consoleContainer = document.createElement('div'); + consoleContainer.id = 'console-container'; + consoleContainer.className = 'mt-2 py-2 px-4 border rounded-lg bg-gray-50 dark:bg-gray-800 max-h-40 overflow-y-auto'; + statusDiv.parentNode.insertBefore(consoleContainer, statusDiv.nextSibling); + } + + consoleContainer.appendChild(consoleDiv); + consoleContainer.scrollTop = consoleContainer.scrollHeight; + } };
🧹 Nitpick comments (2)
app.py (2)
3-9: Fix imports to follow best practicesThe imports should be organized more clearly: group standard library imports, third-party imports, and local imports together. The Flask imports are also split across multiple lines.
from flask import Flask, render_template, request, jsonify, Response from connection import Connection import threading import asyncio import logging from bleak import BleakScanner -from flask import Response import queue
92-96: Imports inside function should be moved to top of fileThe
subprocessandsysimports should be at the top of the file with other imports, not inside a function. This improves readability and prevents repeated imports.from flask import Flask, render_template, request, jsonify, Response from connection import Connection import threading import asyncio import logging from bleak import BleakScanner import queue +import subprocess +import sys # Rest of the code... @app.route('/launch_app', methods=['POST']) def launch_application(): # ... try: - import subprocess - import sys python_exec = sys.executable # ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app.py(1 hunks)requirements.txt(1 hunks)templates/index.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- requirements.txt
🧰 Additional context used
🧬 Code Graph Analysis (1)
app.py (1)
connection.py (7)
Connection(13-261)cleanup(221-258)connect_usb(157-181)connect_wifi(183-219)connect_ble(111-155)start_csv_recording(60-81)stop_csv_recording(83-97)
🔇 Additional comments (2)
app.py (1)
48-52:❓ Verification inconclusive
Inconsistent stream status checking endpoint
The
/check_streamendpoint returns{'connected': True/False}, but the frontend API clients might expect astatusfield for consistency with other endpoints like/check_connectionwhich returns astatusfield.
🏁 Script executed:
#!/bin/bash # Check if the frontend is expecting a 'status' field in the response rg -A 5 $'/check_stream'.*\.then.*json templatesLength of output: 48
I couldn’t find any references to how
/check_connectionis implemented or consumed in the frontend. Let’s first verify the implementations of both endpoints in app.py so we can confirm what each returns:#!/bin/bash # Show the /check_connection endpoint implementation rg "app\.route.*check_connection" -n app.py -A 5 # Show the /check_stream endpoint implementation rg "app\.route.*check_stream" -n app.py -A 5
Let’s retrieve both endpoint implementations to compare their response keys:
🏁 Script executed:
#!/bin/bash # Show /check_connection implementation rg -n "app\.route\('/check_connection'\)" -A 5 app.py # Show /check_stream implementation rg -n "app\.route\('/check_stream'\)" -A 5 app.pyLength of output: 567
Standardize
/check_streamresponse to usestatusCurrently in app.py:
/check_connectionreturns{ status: 'connected' | 'connecting' }/check_streamreturns{ connected: true | false }For consistency (and to avoid frontend confusion), consider having
/check_streamreturn astatusfield as well. For example:@app.route('/check_stream') -def check_stream(): - if connection_manager and connection_manager.stream_active: - return jsonify({'connected': True}) - return jsonify({'connected': False}) +def check_stream(): + status = ( + 'connected' + if connection_manager and connection_manager.stream_active + else 'connecting' + ) + return jsonify({'status': status})• Please verify that your frontend clients expect and handle the
statusfield for both endpoints.templates/index.html (1)
718-722:✅ Verification successful
Consistent stream status checking in
checkStreamStatusEnsure the
checkStreamStatusfunction's check is consistent with the expected response format from the/check_streamendpoint for more robust error handling.
🏁 Script executed:
#!/bin/bash # Compare implementation of stream status checking with backend response format echo "Backend response format from /check_stream:" rg -A2 "check_stream" app.py | grep "return jsonify" echo "\nFrontend stream status checking implementation:" rg -A5 "data.connected" templates/index.htmlLength of output: 673
No changes needed for
checkStreamStatus
The frontend already checks the sameconnectedboolean that the/check_streamendpoint returns (jsonify({'connected': True})), so the implementation is consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (13)
app.py (10)
68-73: Inconsistent stream status response formatThe
/check_streamendpoint returns{'connected': true/false}, but previous review comments noted that the frontend expects astatusfield to be included.@app.route('/check_stream') def check_stream(): if connection_manager and connection_manager.stream_active: - return jsonify({'connected': True}) - return jsonify({'connected': False}) + return jsonify({'connected': True, 'status': 'connected'}) + return jsonify({'connected': False, 'status': 'disconnected'})
19-20: 🛠️ Refactor suggestionRemove redundant global
stream_activevariableThe global
stream_activevariable duplicates functionality already provided byconnection_manager.stream_active, which leads to inconsistent state management across the codebase.This was previously identified in a review comment and should be removed. All references to this global variable should use
connection_manager.stream_activeinstead.
80-87: 🛠️ Refactor suggestionInconsistent stream status management
This function updates a global
stream_activevariable instead of usingconnection_manager.stream_active, leading to state inconsistency.def post_console_message(message): - global stream_active + global connection_manager if "LSL stream started" in message: - stream_active = True + if connection_manager: + connection_manager.stream_active = True elif "disconnected" in message: - stream_active = False + if connection_manager: + connection_manager.stream_active = False console_queue.put(message)
88-96: 🛠️ Refactor suggestionAdd timeout and heartbeat to EventSource stream
The console updates function creates an infinite loop with no mechanism to detect client disconnection or to stop the stream gracefully, which could lead to resource leaks.
@app.route('/console_updates') def console_updates(): def event_stream(): while True: - message = console_queue.get() + try: + message = console_queue.get(timeout=30) # Add timeout to prevent indefinite blocking + except queue.Empty: + # Send a heartbeat to detect disconnected clients + yield "data: heartbeat\n\n" + continue yield f"data: {message}\n\n" return Response(event_stream(), mimetype="text/event-stream")
135-138: 🛠️ Refactor suggestionReference to undefined global variable
Line 137 references the global variable
stream_active, but this should be usingconnection_manager.stream_activeinstead.@app.route('/connect', methods=['POST']) def connect_device(): - global connection_manager, connection_thread, stream_active + global connection_manager, connection_thread
143-144: 🛠️ Refactor suggestionReset stream status using connection manager
The code resets a global
stream_activevariable instead of usingconnection_manager.stream_active.# Reset stream status - stream_active = False + if connection_manager: + connection_manager.stream_active = False
147-151: 🛠️ Refactor suggestionRace condition risk in connection cleanup
The code joins the connection thread without setting a timeout, which could block indefinitely if the thread is stuck.
# Clean up any existing connection if connection_manager: connection_manager.cleanup() if connection_thread and connection_thread.is_alive(): - connection_thread.join() + connection_thread.join(timeout=5) # Add timeout to prevent indefinite blocking + if connection_thread.is_alive(): + logging.warning("Connection thread did not terminate within timeout")
161-170: 🛠️ Refactor suggestionBLE connection creates but doesn't close asyncio event loop
For BLE connections, an event loop is created but not explicitly closed in case of exceptions, which may lead to resource leaks.
elif protocol == 'ble': if not device_address: logging.error("No BLE device address provided") return # For BLE, we need to run in an event loop loop = asyncio.new_event_loop() asyncio.set_event_loop(loop) - success = connection_manager.connect_ble(device_address) + try: + success = connection_manager.connect_ble(device_address) + finally: + loop.close()
185-193: 🛠️ Refactor suggestionInconsistent stream status management in disconnect handler
The disconnect handler directly updates the global
stream_activevariable instead of usingconnection_manager.stream_active.@app.route('/disconnect', methods=['POST']) def disconnect_device(): - global connection_manager, stream_active + global connection_manager if connection_manager: connection_manager.cleanup() - stream_active = False post_console_message("disconnected") return jsonify({'status': 'disconnected'}) return jsonify({'status': 'no active connection'})
231-232: 🛠️ Refactor suggestionDebug mode enabled in production code
Running Flask with
debug=Trueis not recommended for production environments as it exposes sensitive information and can be a security risk.if __name__ == "__main__": - app.run(debug=True) + app.run(debug=False)For development, consider using environment variables to control debug mode:
import os if __name__ == "__main__": debug_mode = os.environ.get('FLASK_DEBUG', 'False').lower() == 'true' app.run(debug=debug_mode)🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 231-231: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/(debug-enabled-python)
templates/index.html (3)
200-272: Tailwind dynamic classes may be purgedThe app card template uses dynamic class names like
bg-${app.color}-100, which Tailwind's PurgeCSS won't detect at build time.Consider either:
- Safelisting these color variants in your
tailwind.config.js- Building the class list explicitly using a lookup object
// Example of a color class lookup approach const colorClasses = { red: { bg: 'bg-red-100', darkBg: 'dark:bg-red-900', text: 'text-red-600', darkText: 'dark:text-red-300' }, blue: { bg: 'bg-blue-100', darkBg: 'dark:bg-blue-900', text: 'text-blue-600', darkText: 'dark:text-blue-300' }, // Add other colors here }; // Then use it like: // <div class="${colorClasses[app.color].bg} ${colorClasses[app.color].darkBg}">
130-142: Move Tailwind config before CDN importYour Tailwind config script appears after loading the CDN, so customizations might not apply on first render.
- <script src="https://cdn.tailwindcss.com"></script> - <script> - tailwind.config = { - darkMode: 'class', - theme: { - extend: { - cursor: { - 'not-allowed': 'not-allowed', - } - } - } - } - </script> + <script> + tailwind.config = { + darkMode: 'class', + theme: { + extend: { + cursor: { + 'not-allowed': 'not-allowed', + } + } + } + } + </script> + <script src="https://cdn.tailwindcss.com"></script>
757-776: 🛠️ Refactor suggestionEnhance SSE reconnection logic
The
startConsoleUpdatesfunction lacks proper reconnection logic when the EventSource encounters an error.// Start console updates via SSE function startConsoleUpdates() { if (eventSource) { eventSource.close(); } eventSource = new EventSource('/console_updates'); eventSource.onmessage = function(e) { console.log('Console update:', e.data); + // Display console message in UI + if (e.data !== "heartbeat") { + const consoleDiv = document.getElementById('console-container'); + if (consoleDiv) { + const message = document.createElement('div'); + message.className = 'text-sm text-gray-600 dark:text-gray-400 my-1'; + message.textContent = e.data; + consoleDiv.appendChild(message); + consoleDiv.scrollTop = consoleDiv.scrollHeight; + } + } }; + let reconnectAttempts = 0; + const maxReconnectAttempts = 5; + eventSource.onerror = function() { console.error('EventSource failed'); if (eventSource) { eventSource.close(); eventSource = null; + + // Handle reconnection + if (reconnectAttempts < maxReconnectAttempts) { + reconnectAttempts++; + const timeout = Math.min(1000 * reconnectAttempts, 5000); // Exponential backoff + console.log(`Reconnecting in ${timeout/1000} seconds...`); + setTimeout(startConsoleUpdates, timeout); + } else { + console.error('Maximum reconnection attempts reached'); + showStatus('Connection lost', 'fa-exclamation-circle', 'text-orange-500'); + } } }; }
🧹 Nitpick comments (2)
app.py (1)
1-1: Remove unused importThe
send_from_directoryfunction is imported but never used in this file.-from flask import Flask, render_template, request, jsonify, send_from_directory +from flask import Flask, render_template, request, jsonify🧰 Tools
🪛 Ruff (0.8.2)
1-1:
flask.send_from_directoryimported but unusedRemove unused import:
flask.send_from_directory(F401)
templates/index.html (1)
912-914: Add version number source from serverThe version number is hardcoded in the frontend, which will require frontend changes whenever the version changes.
Consider fetching the version number from the server or a configuration file:
document.getElementById('info-btn').addEventListener('click', () => { - alert('Chords Python - Biopotential Data Acquisition System\nVersion 2.1.0'); + fetch('/api/version') + .then(response => response.json()) + .then(data => { + alert(`Chords Python - Biopotential Data Acquisition System\nVersion ${data.version}`); + }) + .catch(err => { + alert('Chords Python - Biopotential Data Acquisition System\nVersion information unavailable'); + }); });You would need to add a corresponding endpoint in the backend:
@app.route('/api/version') def get_version(): return jsonify({'version': '2.1.0'})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.py(1 hunks)config/apps.yaml(1 hunks)requirements.txt(1 hunks)templates/index.html(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/apps.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements.txt
🧰 Additional context used
🧬 Code Graph Analysis (1)
app.py (1)
connection.py (7)
Connection(13-261)cleanup(221-258)connect_usb(157-181)connect_wifi(183-219)connect_ble(111-155)start_csv_recording(60-81)stop_csv_recording(83-97)
🪛 Ruff (0.8.2)
app.py
1-1: flask.send_from_directory imported but unused
Remove unused import: flask.send_from_directory
(F401)
🔇 Additional comments (8)
app.py (3)
55-67: Good implementation of BLE device scanningThe BLE scanning endpoint is well-implemented with proper async handling using the
run_asyncdecorator, error handling, and meaningful responses.
195-215: Well-implemented recording start endpointThe
/start_recordingendpoint is properly implemented with good error handling and sends appropriate status responses.
217-229: Well-implemented recording stop endpointThe
/stop_recordingendpoint is properly implemented with good error handling and consistent response formats.templates/index.html (5)
33-45: Well-structured protocol selection UIThe protocol buttons (WiFi, Bluetooth, Serial) are well-designed and structured with clear visual elements, proper icons, and intuitive labels.
59-66: Well-designed recording filename inputThe recording input field with the fixed CSV extension indicator provides a good user experience and maintains file format consistency.
80-101: Well-implemented BLE device selection modalThe BLE device selection modal is well-structured with proper loading state, device listing, and selection functionality.
143-196: Well-implemented application loading and error handlingThe application loading functionality includes proper loading states, error handling, and user feedback.
850-896: Consistent stream status checking incheckStreamStatusThis function checks
data.connecteddirectly, which is correct based on the current backend response format, but be aware that there was a suggestion to modify the backend to include astatusfield.However, make sure the backend and frontend expectations align. If you implement the suggested change to the
/check_streamendpoint to include astatusfield, you might need to update this code as well.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
app.py (8)
20-20: Redundant global state variable.This global variable duplicates state already tracked in
connection_manager.stream_active, which can lead to inconsistent state management.
64-68: Inconsistent stream status checking endpoint.The
/check_streamendpoint returns{'connected': True/False}, but the frontend (in index.html) expects astatusfield. This mismatch will cause the connection status to not update properly.- if connection_manager and connection_manager.stream_active: - return jsonify({'connected': True}) - return jsonify({'connected': False}) + if connection_manager and connection_manager.stream_active: + return jsonify({'connected': True, 'status': 'connected'}) + return jsonify({'connected': False, 'status': 'disconnected'})
76-82: Inconsistent stream status managementThis function updates a global
stream_activevariable, which duplicates the state already tracked inconnection_manager.stream_active. This can lead to inconsistent state management.def post_console_message(message): - global stream_active + global connection_manager if "LSL stream started" in message: - stream_active = True + if connection_manager: + connection_manager.stream_active = True elif "disconnected" in message: - stream_active = False + if connection_manager: + connection_manager.stream_active = False console_queue.put(message)
84-91: Potential resource leak in event stream.The
console_updatesfunction creates an infinite loop that will run until the client disconnects. There's no mechanism to detect client disconnection or to stop the stream gracefully, which could lead to resource leaks.@app.route('/console_updates') def console_updates(): def event_stream(): while True: - message = console_queue.get() + try: + message = console_queue.get(timeout=30) # Add timeout to prevent indefinite blocking + except queue.Empty: + # Send a heartbeat to detect disconnected clients + yield "data: heartbeat\n\n" + continue yield f"data: {message}\n\n" return Response(event_stream(), mimetype="text/event-stream")
143-147: Race condition risk in connection cleanup.There's a potential race condition in the cleanup process. If
connection_threadis running, the code joins it but doesn't set a timeout, which could block indefinitely if the thread is stuck.# Clean up any existing connection if connection_manager: connection_manager.cleanup() if connection_thread and connection_thread.is_alive(): - connection_thread.join() + connection_thread.join(timeout=5) # Add timeout to prevent indefinite blocking + if connection_thread.is_alive(): + logging.warning("Connection thread did not terminate within timeout")
157-166: BLE connection creates but doesn't close asyncio event loop.For BLE connections, an event loop is created but never explicitly closed, which may lead to resource leaks and warnings. The
finallyblock should close the loop.elif protocol == 'ble': if not device_address: logging.error("No BLE device address provided") return # For BLE, we need to run in an event loop loop = asyncio.new_event_loop() asyncio.set_event_loop(loop) - success = connection_manager.connect_ble(device_address) + try: + success = connection_manager.connect_ble(device_address) + finally: + loop.close()
181-189: Inconsistent stream status management in disconnect handler.The disconnect handler updates a global
stream_activevariable, but it should be usingconnection_manager.stream_activeto maintain consistency with the rest of the code.@app.route('/disconnect', methods=['POST']) def disconnect_device(): - global connection_manager, stream_active + global connection_manager if connection_manager: connection_manager.cleanup() - stream_active = False post_console_message("disconnected") return jsonify({'status': 'disconnected'}) return jsonify({'status': 'no active connection'})
227-228: Debug mode enabled in production code.Running Flask with
debug=Trueis not recommended for production environments as it exposes sensitive information and can be a security risk.if __name__ == "__main__": - app.run(debug=True) + app.run(debug=False)For development, consider using environment variables to control debug mode:
import os if __name__ == "__main__": debug_mode = os.environ.get('FLASK_DEBUG', 'False').lower() == 'true' app.run(debug=debug_mode)🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 227-227: Detected Flask app with debug=True. Do not deploy to production with this flag enabled as it will leak sensitive information. Instead, consider using Flask configuration variables or setting 'debug' using system environment variables.
Context: app.run(debug=True)
Note: [CWE-489] Active Debug Code. [REFERENCES]
- https://labs.detectify.com/2015/10/02/how-patreon-got-hacked-publicly-exposed-werkzeug-debugger/(debug-enabled-python)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
app.py (1)
connection.py (7)
Connection(13-261)cleanup(221-258)connect_usb(157-181)connect_wifi(183-219)connect_ble(111-155)start_csv_recording(60-81)stop_csv_recording(83-97)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
templates/index.html (9)
6-7: Include SRI andcrossoriginon external CDNs.
Linking to Font Awesome over the public CDN is fine, but you should addintegrityandcrossorigin="anonymous"attributes to guard against tampering and improve security.
17-25: Icon-only buttons need accessible labels.
Buttons like the theme toggle, GitHub, and info icons lackaria-label(or visually hidden text), which will be confusing for screen readers.
Example:- <button id="theme-toggle" class="..."> + <button id="theme-toggle" class="..." aria-label="Toggle dark/light theme">
31-37: Add ARIA landmarks or labels for the connectivity section.
Your sticky section groups protocol buttons, connect controls, and recording controls, but it has noroleoraria-label.
Wrap it in<section role="region" aria-label="Device connectivity and recording">to improve screen-reader navigation.
34-45: Protocol buttons lack keyboard focus styles.
When navigating via keyboard, it’s hard to see which protocol is selected.
Consider addingfocus:outline-none focus:ring-2 focus:ring-cyan-400to the.connection-btnclasses for better accessibility.
47-55: Explicitly settype="button"on non-form buttons.
By default,<button>in some contexts behaves as a submit button. Although you aren’t inside a<form>, it’s a good practice to declaretype="button"on#connect-btn,#disconnect-btn, etc.
63-69: Missing<label>for the filename input.
Your text input for recording name has no<label>element. Without it, users relying on assistive tech won’t know what the field is for.
Consider:<label for="filename" class="sr-only">Recording filename</label> <input id="filename" …>
83-89: BLE modal needs proper dialog semantics.
You should addrole="dialog",aria-modal="true", andaria-labelledby="ble-modal-title"to the modal container for accessibility. Also ensure the close button hasaria-label="Close Bluetooth device selector".
112-119: Filter<select>should have anidand<label>.
You rely ondocument.querySelector('select')in JS, which may break if more selects are added later.
- Give the
<select>anid="category-filter".- Add a
<label for="category-filter">Filter by category</label>for accessibility.
146-922: Extract inline script into modular JS files.
The<script>block now spans hundreds of lines, mixing concerns (app loading, theming, connectivity, recording, SSE, UI updates).
- Split into separate modules:
theme.js,connectivity.js,apps.js,console.js.- Serve them as external scripts for better caching, maintainability, and testability.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
templates/index.html (2)
2-2: Remove hard-codeddarkclass to avoid FOUCLeaving
class="dark"on the root element forces dark mode on first paint and then toggles to the user’s preferred theme, causing a visual flicker. Instead, add the class only after you’ve readlocalStorage/prefers-color-schemein JS (the logic is already inscript.js).
133-146: Place Tailwind config before the CDN importPer Tailwind CDN docs, the
<script>tailwind.config…</script>block must precede
<script src="https://cdn.tailwindcss.com">or the custom settings (dark mode, cursor) won’t be applied on first render.static/script.js (3)
78-83: Dynamic colour classes may be purged / unavailableClasses like
bg-${app.color}-100andtext-${app.color}-600are generated at runtime.
Tailwind’s CDN compiler may not catch them, resulting in missing styles.
Prefer a lookup table:const colourMap = { red: ['bg-red-100','dark:bg-red-900','text-red-600','dark:text-red-300'], cyan: ['bg-cyan-100', ...], // … }; const [bg, bgDark, fg, fgDark] = colourMap[app.color] || colourMap.cyan; card.innerHTML = ` <div class="${bg} ${bgDark} …"> <i class="… ${fg} ${fgDark}"></i> </div> `;
617-633: Console SSE only logs to dev console — no UI feedbackUsers cannot see live device logs unless they open DevTools.
Append messages to a scrollable<div id="console-container">(as previously suggested) and implement reconnection/back-off logic so the stream resumes after network hiccups.
426-452: 🛠️ Refactor suggestionNo CSRF protection on POST requests
All
fetchPOSTs (/connect,/start_recording, etc.) omit CSRF tokens.
If your Flask app has (or will have) CSRF protection, these requests will fail and remain vulnerable to cross-site attacks.
🧹 Nitpick comments (1)
templates/index.html (1)
60-68: Validate & constrain filename on the client
<input id="filename">currently allows any characters, but the sanitiser in JS only replaces spaces and dots.
Add an explicit pattern (or usetype="text"+pattern) to block path separators and non-ASCII characters to prevent invalid filenames reaching the backend.-<input type="text" id="filename" - class="flex-1 h-10 px-4 rounded-l-lg focus:outline-none focus:ring-1 border border-gray-600 dark:text-white dark:bg-gray-800" - placeholder="Enter recording name"> +<input type="text" id="filename" + pattern="^[A-Za-z0-9_-]+$" + title="Use letters, numbers, dashes or underscores only" + class="flex-1 h-10 px-4 rounded-l-lg focus:outline-none focus:ring-1 border border-gray-600 dark:text-white dark:bg-gray-800" + placeholder="Enter recording name">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
static/script.js(1 hunks)templates/index.html(1 hunks)
🔇 Additional comments (1)
static/script.js (1)
320-358:isScanningflag never reset when modal closesIf the user closes the BLE modal mid-scan,
isScanningstaystrue, blocking future rescans.
Reset the flag in the modal’s close handler.closeBleModal.addEventListener('click', () => { bleModal.classList.add('hidden'); + isScanning = false; … });
| app_processes[app_name] = process # Track running process in global dictionary | ||
| current_message = f"{app_name} started successfully" | ||
| if connection_manager.start_csv_recording(filename): | ||
| post_console_message(f"Recording started: {filename or 'default filename'}") |
There was a problem hiding this comment.
@PayalLakra We should add a way to push the toast under our web UI.
| time.sleep(0.5) | ||
|
|
||
| return Response(event_stream(), mimetype="text/event-stream") | ||
| logging.error(f"Recording error: {str(e)}") |
There was a problem hiding this comment.
There should be log.txt created with timestamps for error logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
static/script.js (3)
78-80: Dynamic Tailwind classes will be purged in production builds
bg-${app.color}-100,dark:bg-${app.color}-900,text-${app.color}-600etc. are generated at runtime.
Unless you safelist every possible permutation intailwind.config.js, PurgeCSS/JIT will strip them, leaving blank styles in production.Safelist with a regex pattern or switch to a small predefined map:
const COLOR_CLASSES = { red : ['bg-red-100','dark:bg-red-900','text-red-600','dark:text-red-300'], cyan : ['bg-cyan-100','dark:bg-cyan-900','text-cyan-600','dark:text-cyan-300'], … }; const [bgLight,bgDark,txtLight,txtDark] = COLOR_CLASSES[app.color] ?? COLOR_CLASSES.cyan;
761-763: Continuous 1 s polling may overload the backend & client
setInterval(checkStreamStatus, 1000)fires forever, even when the page is idle and an SSE channel already exists. Consider:
- Switching to a single SSE/WebSocket event that emits connection changes, or
- Increasing the interval / using exponential back-off once a stable state is detected, stopping entirely while the SSE stream is alive.
This will cut network chatter and CPU usage.
708-710: Replace blockingalert()with non-blocking toast for better UXNative
alert()is synchronous, blocks the main thread, and cannot be styled.
Adopt a lightweight toast/snackbar component (e.g. Tailwind + Alpine, or a small JS lib) to keep the UI responsive and visually consistent.
| // Get the filename (already sanitized in the display) | ||
| let filename = filenameInput.value.trim(); | ||
|
|
||
| // If empty, use default (pass null to let server generate default) | ||
| if (filename === '') { | ||
| filename = null; | ||
| } | ||
|
|
||
| if (isRecording) { | ||
| // Stop recording | ||
| fetch('/stop_recording', { method: 'POST' }) |
There was a problem hiding this comment.
Re-sanitise filename just before the network request
The input event filter is good for UX, but a malicious user can still alter the DOM value via dev-tools or crafted script injection.
Run sanitizeFilename again right before you send it to the server to guarantee server-side safety.
-// Get the filename (already sanitized in the display)
-let filename = filenameInput.value.trim();
+// Re-sanitise to defend against DOM tampering
+let filename = sanitizeFilename(filenameInput.value);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get the filename (already sanitized in the display) | |
| let filename = filenameInput.value.trim(); | |
| // If empty, use default (pass null to let server generate default) | |
| if (filename === '') { | |
| filename = null; | |
| } | |
| if (isRecording) { | |
| // Stop recording | |
| fetch('/stop_recording', { method: 'POST' }) | |
| // Re-sanitise to defend against DOM tampering | |
| let filename = sanitizeFilename(filenameInput.value); | |
| // If empty, use default (pass null to let server generate default) | |
| if (filename === '') { | |
| filename = null; | |
| } | |
| if (isRecording) { | |
| // Stop recording | |
| fetch('/stop_recording', { method: 'POST' }) |
| card.className = `group bg-gradient-to-b from-white to-gray-50 dark:from-gray-700 dark:to-gray-800 rounded-xl shadow border hover:shadow-lg transition-all duration-300 dark:border-gray-700 cursor-pointer overflow-hidden`; | ||
|
|
||
| card.innerHTML = ` | ||
| <div class="relative h-full flex flex-col"> | ||
| <div class="w-full h-32 bg-${app.color}-100 dark:bg-${app.color}-900 rounded-t-lg flex items-center justify-center transition-all duration-300 group-hover:opacity-90"> | ||
| <i class="fas ${app.icon} text-4xl text-${app.color}-600 dark:text-${app.color}-300"></i> | ||
| </div> | ||
| <div class="p-4 flex-1 flex flex-col"> | ||
| <h3 class="text-lg font-semibold text-gray-800 dark:text-gray-200 mb-1">${app.title}</h3> | ||
| <p class="text-sm text-gray-600 dark:text-gray-400 mb-3 flex-1">${app.description}</p> | ||
| </div> | ||
| </div> | ||
| `; | ||
|
|
There was a problem hiding this comment.
Avoid innerHTML with unsanitised data – potential XSS vector
app.title, app.description, app.icon, app.color, and app.script originate from a YAML file that is editable outside the UI. Injecting them directly with innerHTML lets any embedded markup (or worse, <script> tags) execute in the browser.
Refactor to build the DOM with textContent / classList or sanitise the strings with a vetted library such as DOMPurify before insertion.
-card.innerHTML = `
- <div class="relative h-full flex flex-col">
- <div class="w-full h-32 bg-${app.color}-100 dark:bg-${app.color}-900 …">
- <i class="fas ${app.icon} text-4xl …"></i>
- </div>
- <div class="p-4 flex-1 flex flex-col">
- <h3 class="text-lg … mb-1">${app.title}</h3>
- <p class="text-sm … mb-3 flex-1">${app.description}</p>
- </div>
- </div>
-`;
+const wrapper = document.createElement('div');
+wrapper.className = 'relative h-full flex flex-col';
+
+const banner = document.createElement('div');
+banner.className = `w-full h-32 bg-${app.color}-100 dark:bg-${app.color}-900 …`;
+
+const icon = document.createElement('i');
+icon.className = `fas ${app.icon} text-4xl …`;
+banner.appendChild(icon);
+
+const body = document.createElement('div');
+body.className = 'p-4 flex-1 flex flex-col';
+
+const title = document.createElement('h3');
+title.className = 'text-lg … mb-1';
+title.textContent = app.title;
+
+const desc = document.createElement('p');
+desc.className = 'text-sm … mb-3 flex-1';
+desc.textContent = app.description;
+
+body.append(title, desc);
+wrapper.append(banner, body);
+card.appendChild(wrapper);Committable suggestion skipped: line range outside the PR's diff.
|
@PayalLakra Thank you so much for your contribution, I am merging this now. |
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Chores