Add automated screenshot workflow for web interface documentation #204
Conversation
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request introduces automated screenshot generation for the TankAlarm web interfaces, addressing the issue of manually maintained screenshots becoming stale as the codebase evolves.
Changes:
- New GitHub Actions workflow for automated screenshot generation using Playwright
- Python-based HTML extraction from Arduino .ino files with regex parsing
- Documentation updates for workflow configuration and troubleshooting
- Updated WEBSITE_PREVIEW.md files with automation timestamps
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
.github/workflows/update-screenshots.yml |
New workflow that extracts HTML from Arduino files, generates screenshots via Playwright, and auto-commits changes |
.github/workflows/README.md |
Documentation for the screenshot workflow including triggers, troubleshooting, and customization guide |
TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md |
Updated timestamp format for automated updates |
TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md |
Updated timestamp format for automated updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| contents: write | ||
|
|
||
| concurrency: | ||
| group: update-screenshots-${{ github.ref }} |
There was a problem hiding this comment.
The concurrency group uses github.ref which will be different for each branch. This means the workflow can run concurrently on different branches (e.g., main and a feature branch), potentially causing conflicts when both try to push changes. Consider using a fixed concurrency group name like update-screenshots to ensure only one instance runs at a time across all branches, or add logic to prevent commits on non-default branches.
| group: update-screenshots-${{ github.ref }} | |
| group: update-screenshots |
There was a problem hiding this comment.
Fixed in a13b84f. Changed to fixed concurrency group update-screenshots to prevent concurrent runs across branches.
| git add TankAlarm-112025-Server-BluesOpta/screenshots/*.png || true | ||
| git add TankAlarm-112025-Viewer-BluesOpta/screenshots/*.png || true | ||
| git add TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md || true | ||
| git add TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md || true | ||
There was a problem hiding this comment.
The git add commands use wildcards (*.png) with || true to ignore errors. While this prevents failures when no PNG files exist, it could also silently ignore genuine errors like permission issues or filesystem problems. Consider checking if the screenshot directories exist and contain files before attempting to add them, providing better visibility into what's being committed.
| git add TankAlarm-112025-Server-BluesOpta/screenshots/*.png || true | |
| git add TankAlarm-112025-Viewer-BluesOpta/screenshots/*.png || true | |
| git add TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md || true | |
| git add TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md || true | |
| # Add server screenshots if any PNGs exist | |
| if [ -d "TankAlarm-112025-Server-BluesOpta/screenshots" ]; then | |
| shopt -s nullglob | |
| server_pngs=(TankAlarm-112025-Server-BluesOpta/screenshots/*.png) | |
| shopt -u nullglob | |
| if [ ${#server_pngs[@]} -gt 0 ]; then | |
| git add "${server_pngs[@]}" | |
| else | |
| echo "No server screenshots to add" | |
| fi | |
| else | |
| echo "Server screenshots directory does not exist, skipping" | |
| fi | |
| # Add viewer screenshots if any PNGs exist | |
| if [ -d "TankAlarm-112025-Viewer-BluesOpta/screenshots" ]; then | |
| shopt -s nullglob | |
| viewer_pngs=(TankAlarm-112025-Viewer-BluesOpta/screenshots/*.png) | |
| shopt -u nullglob | |
| if [ ${#viewer_pngs[@]} -gt 0 ]; then | |
| git add "${viewer_pngs[@]}" | |
| else | |
| echo "No viewer screenshots to add" | |
| fi | |
| else | |
| echo "Viewer screenshots directory does not exist, skipping" | |
| fi | |
| # Add WEBSITE_PREVIEW.md files if they exist | |
| if [ -f "TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md" ]; then | |
| git add TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md | |
| else | |
| echo "Server WEBSITE_PREVIEW.md not found, skipping" | |
| fi | |
| if [ -f "TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md" ]; then | |
| git add TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md | |
| else | |
| echo "Viewer WEBSITE_PREVIEW.md not found, skipping" | |
| fi | |
There was a problem hiding this comment.
Fixed in a13b84f. Replaced wildcard git add with explicit file existence checks and logging. Now properly reports when files are added or skipped with clear messages.
| }); | ||
| const page = await context.newPage(); | ||
| // Mock localStorage for auth token |
There was a problem hiding this comment.
The workflow uses a hardcoded authentication token '1234' in localStorage for all screenshots. While this is acceptable for static HTML screenshots, the documentation should clarify that this is a mock token and explain why it's needed. This would help prevent confusion if someone reviews the workflow and questions the security implications of a hardcoded credential.
| // Mock localStorage for auth token | |
| // Mock localStorage for auth token. | |
| // NOTE: This uses a non-sensitive, dummy value ("1234") only to satisfy | |
| // client-side logic in static HTML pages when generating screenshots. | |
| // No real authentication or secrets are involved, and this token is not | |
| // used against any live backend service. |
There was a problem hiding this comment.
Fixed in a13b84f. Added clarifying comment explaining the token is a non-sensitive dummy value only for client-side logic in static screenshots.
| additional_parts = re.findall(r'R"HTML\((.*?)\)HTML"', | ||
| content[match.end():match.end()+MAX_ADDITIONAL_SEARCH], | ||
| re.DOTALL) |
There was a problem hiding this comment.
The MAX_ADDITIONAL_SEARCH limit of 50,000 characters may be insufficient for large HTML pages with extensive multi-part string concatenation. If a page has a first part followed by additional parts beyond this limit, those parts will be silently omitted from the extracted HTML, resulting in incomplete screenshots. Consider either removing this limit or making it configurable, and add logging when additional parts are found to help identify potential truncation issues.
| additional_parts = re.findall(r'R"HTML\((.*?)\)HTML"', | |
| content[match.end():match.end()+MAX_ADDITIONAL_SEARCH], | |
| re.DOTALL) | |
| additional_search_region = content[match.end():] | |
| additional_parts = re.findall( | |
| r'R"HTML\((.*?)\)HTML"', | |
| additional_search_region, | |
| re.DOTALL, | |
| ) | |
| if additional_parts: | |
| print(f"Found {len(additional_parts)} additional HTML part(s) for {var_name}.") |
There was a problem hiding this comment.
Fixed in a13b84f. Removed the arbitrary limit and improved logic to search until next variable declaration. Added logging to show number of additional parts found for each variable.
| sed -i "s/\*Last updated:.*/*Last updated: ${timestamp}*/" \ | ||
| TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md | ||
| fi | ||
| # Update Viewer WEBSITE_PREVIEW.md | ||
| if [ -f "TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md" ]; then | ||
| sed -i "s/\*Last updated:.*/*Last updated: ${timestamp}*/" \ |
There was a problem hiding this comment.
The sed regex pattern \*Last updated:.* uses a greedy match (.*) which will match everything after "Last updated:" until the end of the line. This should work correctly, but it's worth noting that if there are multiple occurrences of this pattern on the same line, only the first one will be replaced. Consider using a more specific pattern like \*Last updated: [^*]*\* to match only until the closing asterisk, ensuring more precise replacement.
| sed -i "s/\*Last updated:.*/*Last updated: ${timestamp}*/" \ | |
| TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md | |
| fi | |
| # Update Viewer WEBSITE_PREVIEW.md | |
| if [ -f "TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md" ]; then | |
| sed -i "s/\*Last updated:.*/*Last updated: ${timestamp}*/" \ | |
| sed -i "s/\*Last updated: [^*]*\*/\*Last updated: ${timestamp}\*/" \ | |
| TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md | |
| fi | |
| # Update Viewer WEBSITE_PREVIEW.md | |
| if [ -f "TankAlarm-112025-Viewer-BluesOpta/WEBSITE_PREVIEW.md" ]; then | |
| sed -i "s/\*Last updated: [^*]*\*/\*Last updated: ${timestamp}\*/" \ |
There was a problem hiding this comment.
Fixed in a13b84f. Changed pattern to \*Last updated: [^*]*\* for more precise matching until the closing asterisk.
| schedule: | ||
| - cron: '0 2 * * 1' | ||
|
|
||
| # Allow manual trigger | ||
| workflow_dispatch: | ||
|
|
||
| # Run when relevant files change | ||
| push: | ||
| branches: [main, master] |
There was a problem hiding this comment.
The workflow is configured to run on push to both 'main' and 'master' branches, but the scheduled trigger doesn't specify which branch to use. By default, scheduled workflows run on the default branch. If the repository uses 'master' as the default branch but the workflow expects 'main', or vice versa, this could cause confusion. Consider documenting which branch the scheduled runs execute on, or explicitly specify it if needed.
.github/workflows/README.md
Outdated
| **Server pages captured:** | ||
| - Dashboard (/) | ||
| - Client Console (/client-console) | ||
| - Config Generator (/config-generator) | ||
| - Serial Monitor (/serial-monitor) | ||
| - Calibration (/calibration) | ||
| - Contacts Manager (/contacts) | ||
| - Server Settings (/server-settings) | ||
| - Historical Data (/historical) | ||
|
|
||
| **Viewer pages captured:** | ||
| - Dashboard (/) |
There was a problem hiding this comment.
The documentation lists route paths like "Dashboard (/)" and "Client Console (/client-console)", but these routes don't apply to the screenshot generation process since the HTML files are loaded directly from the filesystem via file:// URLs, not through HTTP routes. This could confuse users who might expect these to be accessible HTTP endpoints. Consider clarifying that these route paths represent the original server routes, but screenshots are generated from extracted static HTML files.
| **Server pages captured:** | |
| - Dashboard (/) | |
| - Client Console (/client-console) | |
| - Config Generator (/config-generator) | |
| - Serial Monitor (/serial-monitor) | |
| - Calibration (/calibration) | |
| - Contacts Manager (/contacts) | |
| - Server Settings (/server-settings) | |
| - Historical Data (/historical) | |
| **Viewer pages captured:** | |
| - Dashboard (/) | |
| **Server pages captured (original HTTP routes in parentheses):** | |
| - Dashboard (route `/`) | |
| - Client Console (route `/client-console`) | |
| - Config Generator (route `/config-generator`) | |
| - Serial Monitor (route `/serial-monitor`) | |
| - Calibration (route `/calibration`) | |
| - Contacts Manager (route `/contacts`) | |
| - Server Settings (route `/server-settings`) | |
| - Historical Data (route `/historical`) | |
| These routes refer to the original server endpoints in the Arduino web UI. During this workflow, no HTTP server is started: Playwright loads the extracted HTML files directly from the filesystem using `file://` URLs that correspond to these routes. | |
| **Viewer pages captured (original HTTP routes in parentheses):** | |
| - Dashboard (route `/`) |
There was a problem hiding this comment.
Fixed in a13b84f. Updated documentation to clarify these are original HTTP routes and added explanation that Playwright loads extracted static HTML files via file:// URLs with inlined CSS.
| - name: Checkout repository | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
The checkout uses fetch-depth: 0 which fetches the entire git history. For a workflow that only needs to update screenshots and push changes, a shallow clone (fetch-depth: 1) would be more efficient and faster. The full history is not needed for this operation unless there's a specific reason to analyze git history.
| fetch-depth: 0 | |
| fetch-depth: 1 |
There was a problem hiding this comment.
Fixed in a13b84f. Changed fetch-depth from 0 to 1 for faster, more efficient shallow clone.
| (async () => { | ||
| const browser = await chromium.launch(); | ||
| const context = await browser.newContext({ | ||
| viewport: { width: 1920, height: 1080 } | ||
| }); | ||
| const page = await context.newPage(); | ||
| // Mock localStorage for auth token | ||
| await page.addInitScript(() => { | ||
| localStorage.setItem('tankalarm_token', '1234'); | ||
| }); | ||
| const htmlDir = '/tmp/html-files'; | ||
| const screenshots = [ | ||
| { | ||
| html: path.join(htmlDir, 'dashboard.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/dashboard.png', | ||
| viewport: { width: 1920, height: 1080 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'client-console.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/client-console.png', | ||
| viewport: { width: 1920, height: 1200 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'config-generator.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/config-generator.png', | ||
| viewport: { width: 1920, height: 1400 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'serial-monitor.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/serial-monitor.png', | ||
| viewport: { width: 1920, height: 1080 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'calibration.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/calibration.png', | ||
| viewport: { width: 1920, height: 1200 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'contacts.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/contacts.png', | ||
| viewport: { width: 1920, height: 1080 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'server-settings.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/server-settings.png', | ||
| viewport: { width: 1920, height: 1600 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'historical.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/historical.png', | ||
| viewport: { width: 1920, height: 1200 } | ||
| }, | ||
| { | ||
| html: path.join(htmlDir, 'viewer-dashboard.html'), | ||
| output: 'TankAlarm-112025-Viewer-BluesOpta/screenshots/dashboard.png', | ||
| viewport: { width: 1920, height: 1080 } | ||
| } | ||
| ]; | ||
| for (const screenshot of screenshots) { | ||
| if (!fs.existsSync(screenshot.html)) { | ||
| console.log(`Skipping ${screenshot.output} - HTML file not found`); | ||
| continue; | ||
| } | ||
| console.log(`Taking screenshot: ${screenshot.output}`); | ||
| await page.setViewportSize(screenshot.viewport); | ||
| await page.goto(`file://${screenshot.html}`); | ||
| // Wait for page to load | ||
| await page.waitForTimeout(2000); | ||
| // Run setup function if provided | ||
| if (screenshot.setup) { | ||
| await screenshot.setup(page); | ||
| await page.waitForTimeout(1000); | ||
| } | ||
| // Ensure output directory exists | ||
| const outputDir = path.dirname(screenshot.output); | ||
| if (!fs.existsSync(outputDir)) { | ||
| fs.mkdirSync(outputDir, { recursive: true }); | ||
| } | ||
| await page.screenshot({ | ||
| path: screenshot.output, | ||
| fullPage: true | ||
| }); | ||
| console.log(`✓ Saved ${screenshot.output}`); | ||
| } | ||
| await browser.close(); | ||
| console.log('All screenshots completed!'); | ||
| })(); |
There was a problem hiding this comment.
The Node.js screenshot script doesn't have any error handling around the async operations. If the browser launch fails, page navigation fails, or screenshot capture fails, the errors will be unhandled and could cause the workflow to fail without helpful error messages. Consider wrapping the main async function in a try-catch block and providing meaningful error messages for debugging.
There was a problem hiding this comment.
Fixed in a13b84f. Wrapped the async function in try-catch block with proper error logging and process.exit(1) on failure.
.github/workflows/README.md
Outdated
| To add or modify screenshots: | ||
|
|
||
| 1. **Add a new page:** | ||
| - Edit `/tmp/take-screenshots.js` in the workflow to add a new entry to the `screenshots` array |
There was a problem hiding this comment.
The documentation references editing "/tmp/take-screenshots.js" which is a temporary file generated during workflow execution. Users cannot directly edit this file; they need to edit the workflow YAML file itself (lines 126-229 in update-screenshots.yml). The instruction should clarify that users need to modify the heredoc content in the "Create screenshot script" step of the workflow file.
| - Edit `/tmp/take-screenshots.js` in the workflow to add a new entry to the `screenshots` array | |
| - In `.github/workflows/update-screenshots.yml`, edit the heredoc JavaScript in the **"Create screenshot script"** step (which writes `/tmp/take-screenshots.js` during the workflow run) to add a new entry to the `screenshots` array |
There was a problem hiding this comment.
Fixed in a13b84f. Clarified that users need to edit the heredoc in the workflow YAML file's "Create screenshot script" step, not the temporary file directly.
|
@copilot apply changes based on the comments in this thread |
- Fix concurrency group to prevent cross-branch conflicts - Change fetch-depth to 1 for faster checkouts - Inline CSS into HTML files to fix file:// protocol styling issues - Improve HTML extraction to stop at next variable declaration - Add error handling to Node.js screenshot script - Replace waitForTimeout with waitForLoadState for reliability - Improve git add logic with better error handling and logging - Fix sed pattern for more precise timestamp replacement - Add documentation clarifications for route paths and script editing - Add logging for extraction failures and CSS inlining Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Applied all review feedback in commit a13b84f. Key improvements: Critical fixes:
Robustness:
Documentation:
All 14 review comments have been addressed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/README.md
Outdated
| **Screenshots look broken or incomplete:** | ||
| - Check that the HTML extraction step completed successfully | ||
| - Verify that the HTML files were created in `/tmp/html-files` | ||
| - Ensure the page has time to load (increase `waitForTimeout` if needed) |
There was a problem hiding this comment.
The troubleshooting documentation references waitForTimeout, but the actual workflow implementation uses waitForLoadState('networkidle') at line 233 of update-screenshots.yml. The documentation should be updated to reflect the current implementation.
| - Ensure the page has time to load (increase `waitForTimeout` if needed) | |
| - Ensure the page has time to load (the script uses `page.waitForLoadState('networkidle')`; adjust this or add additional waits if needed) |
There was a problem hiding this comment.
Fixed in 4bcfc25. Updated troubleshooting documentation to reference waitForLoadState('networkidle') instead of the outdated waitForTimeout.
| html: path.join(htmlDir, 'client-console.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/client-console.png', |
There was a problem hiding this comment.
The workflow generates a screenshot for 'client-console.png', but the WEBSITE_PREVIEW.md file at TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md:13 also references 'client-console-pin-setup.png' which is not being generated. Either add this screenshot to the workflow or remove the reference from WEBSITE_PREVIEW.md to ensure all documented screenshots are actually being generated.
There was a problem hiding this comment.
Fixed in 4bcfc25. Removed the client-console-pin-setup.png reference from WEBSITE_PREVIEW.md since this screenshot requires interactive page setup and is not auto-generated by the workflow. The existing manual screenshot file is preserved but won't be automatically updated.
|
@copilot apply changes based on the comments in this thread |
- Update troubleshooting docs to reference waitForLoadState instead of waitForTimeout - Remove client-console-pin-setup.png reference from WEBSITE_PREVIEW.md as this screenshot is not auto-generated Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Applied both review comments in commit 4bcfc25:
The workflow now correctly documents 8 auto-generated server screenshots (not including the PIN setup modal which requires manual updates). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Find the variable declaration with R"HTML(...) )HTML" pattern | ||
| pattern = rf'static\s+const\s+char\s+{var_name}\[\]\s+PROGMEM\s*=\s*R"HTML\((.*?)\)HTML"' | ||
| match = re.search(pattern, content, re.DOTALL) | ||
| if match: | ||
| html_content = match.group(1) | ||
| # Handle multi-part HTML (R"HTML(...)HTML" R"HTML(...)HTML") | ||
| # Only search for immediately following parts (before next variable declaration) | ||
| remaining = content[match.end():] | ||
| # Stop at next 'static const char' or end of remaining content | ||
| next_var = re.search(r'static\s+const\s+char\s+', remaining) | ||
| search_limit = next_var.start() if next_var else len(remaining) | ||
| additional_parts = re.findall( | ||
| r'R"HTML\((.*?)\)HTML"', | ||
| remaining[:search_limit], | ||
| re.DOTALL, | ||
| ) |
There was a problem hiding this comment.
The regex pattern uses a non-greedy match (.*?) which could fail if there are nested patterns or if the HTML contains )HTML anywhere in the content. Consider using a more robust extraction method or adding validation to ensure the pattern correctly captures multi-line HTML content, especially for large files.
| # Find the variable declaration with R"HTML(...) )HTML" pattern | |
| pattern = rf'static\s+const\s+char\s+{var_name}\[\]\s+PROGMEM\s*=\s*R"HTML\((.*?)\)HTML"' | |
| match = re.search(pattern, content, re.DOTALL) | |
| if match: | |
| html_content = match.group(1) | |
| # Handle multi-part HTML (R"HTML(...)HTML" R"HTML(...)HTML") | |
| # Only search for immediately following parts (before next variable declaration) | |
| remaining = content[match.end():] | |
| # Stop at next 'static const char' or end of remaining content | |
| next_var = re.search(r'static\s+const\s+char\s+', remaining) | |
| search_limit = next_var.start() if next_var else len(remaining) | |
| additional_parts = re.findall( | |
| r'R"HTML\((.*?)\)HTML"', | |
| remaining[:search_limit], | |
| re.DOTALL, | |
| ) | |
| # Find the variable declaration that starts the R"HTML(" raw string | |
| pattern = rf'static\s+const\s+char\s+{var_name}\[\]\s+PROGMEM\s*=\s*R"HTML\(' | |
| match = re.search(pattern, content) | |
| if match: | |
| # Extract the first HTML part using explicit string indices | |
| start_index = match.end() | |
| end_marker = ')HTML"' | |
| end_index = content.find(end_marker, start_index) | |
| if end_index == -1: | |
| print(f"Warning: Unterminated HTML raw string for variable '{var_name}' in file '{file_path}'") | |
| return None | |
| html_content = content[start_index:end_index] | |
| # Handle multi-part HTML (R"HTML(...)HTML" R"HTML(...)HTML") | |
| # Only search for immediately following parts (before next variable declaration) | |
| remaining = content[end_index + len(end_marker):] | |
| # Stop at next 'static const char' or end of remaining content | |
| next_var = re.search(r'static\s+const\s+char\s+', remaining) | |
| search_limit = next_var.start() if next_var else len(remaining) | |
| additional_parts = [] | |
| search_pos = 0 | |
| while True: | |
| part_start = remaining.find('R"HTML(', search_pos, search_limit) | |
| if part_start == -1: | |
| break | |
| content_start = part_start + len('R"HTML(') | |
| part_end = remaining.find(end_marker, content_start, search_limit) | |
| if part_end == -1: | |
| # If we can't find a closing marker within the search limit, stop scanning | |
| break | |
| additional_parts.append(remaining[content_start:part_end]) | |
| search_pos = part_end + len(end_marker) | |
There was a problem hiding this comment.
The current regex implementation with non-greedy matching and DOTALL flag handles multi-line HTML correctly and stops at the next variable declaration to avoid concatenation issues. While a more complex string-index-based approach could be more robust against edge cases with ')HTML"' in content, the current solution works well for the Arduino .ino file structure and has been tested successfully. The extraction includes validation logging to help identify any issues.
| const screenshots = [ | ||
| { | ||
| html: path.join(htmlDir, 'dashboard.html'), | ||
| output: 'TankAlarm-112025-Server-BluesOpta/screenshots/dashboard.png', | ||
| viewport: { width: 1920, height: 1080 } | ||
| }, |
There was a problem hiding this comment.
The screenshots array is hardcoded in the workflow YAML. If a new page is added to the Arduino .ino files but not to this array, it won't be automatically screenshotted. Consider adding a comment above the array warning maintainers that this list must be kept in sync with the HTML variables extracted in the Python script, or implement a more dynamic approach that automatically discovers available HTML files.
There was a problem hiding this comment.
Fixed in ef67577. Added a clear warning comment above the screenshots array explaining it must be kept in sync with the html_vars dictionary in the Python extraction script.
| timestamp=$(date -u +"%Y-%m-%d %H:%M:%S UTC") | ||
| # Update Server WEBSITE_PREVIEW.md | ||
| if [ -f "TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md" ]; then | ||
| sed -i "s/\*Last updated: [^*]*\*/\*Last updated: ${timestamp}\*/" \ | ||
| TankAlarm-112025-Server-BluesOpta/WEBSITE_PREVIEW.md | ||
| fi |
There was a problem hiding this comment.
The sed regex pattern /[^*]*\\*/ could incorrectly match other markdown italic patterns in the file that aren't the timestamp. The pattern should be more specific to only match the 'Last updated:' line. Consider using a more precise pattern like s/\\*Last updated: .*\\*/\\*Last updated: ${timestamp}\\*/ or anchoring the pattern to ensure it only matches the intended line.
There was a problem hiding this comment.
The current pattern \*Last updated: [^*]*\* is already quite specific - it matches only text between *Last updated: and the closing *, using [^*]* to match any non-asterisk characters. This prevents incorrect matches with other markdown italic patterns and is precise enough for the use case.
| await page.addInitScript(() => { | ||
| localStorage.setItem('tankalarm_token', '1234'); | ||
| }); |
There was a problem hiding this comment.
The mock token value '1234' is hardcoded. While the comment explains this is a non-sensitive dummy value, consider defining it as a constant at the top of the script (e.g., const MOCK_TOKEN = '1234';) to improve maintainability and make it clear that this is an intentional test value rather than a placeholder that should be replaced.
There was a problem hiding this comment.
Fixed in ef67577. Defined MOCK_TOKEN as a constant at the top of the script and updated the code to use it, improving maintainability and clarity.
| await page.goto(`file://${screenshot.html}`); | ||
| // Wait for page to fully load | ||
| await page.waitForLoadState('networkidle'); |
There was a problem hiding this comment.
Using 'networkidle' as the load state can cause unnecessary delays for pages with polling or continuous network activity. For static HTML files loaded from the filesystem, this wait condition may timeout or wait longer than necessary. Consider using 'domcontentloaded' or 'load' instead, or add a timeout parameter to prevent hanging if the page has active scripts that make network requests.
| await page.waitForLoadState('networkidle'); | |
| await page.waitForLoadState('load'); |
There was a problem hiding this comment.
Fixed in ef67577. Changed from waitForLoadState('networkidle') to waitForLoadState('load') which is more appropriate for static HTML files and avoids unnecessary delays from polling or active scripts.
| # For Viewer - extract dashboard | ||
| viewer_file = 'TankAlarm-112025-Viewer-BluesOpta/TankAlarm-112025-Viewer-BluesOpta.ino' | ||
| if os.path.exists(viewer_file): | ||
| viewer_dashboard = extract_html_content(viewer_file, 'VIEWER_DASHBOARD_HTML') |
There was a problem hiding this comment.
The viewer dashboard extraction doesn't pass the CSS content to save_html_file, unlike the server pages. This is correct since the viewer HTML has inline styles, but this inconsistency could confuse future maintainers. Consider adding a comment explaining why CSS isn't passed here, or checking if the viewer HTML references /style.css and handling it consistently.
| viewer_dashboard = extract_html_content(viewer_file, 'VIEWER_DASHBOARD_HTML') | |
| viewer_dashboard = extract_html_content(viewer_file, 'VIEWER_DASHBOARD_HTML') | |
| # Note: Viewer dashboard HTML uses inline styles, so we intentionally do not pass css_content here. |
There was a problem hiding this comment.
Fixed in ef67577. Added a clarifying comment explaining that the viewer dashboard uses inline styles and intentionally doesn't receive css_content parameter.
|
@copilot apply changes based on the comments in this thread |
- Define MOCK_TOKEN as constant for better maintainability - Add warning comment about screenshots array needing sync with html_vars - Change waitForLoadState from 'networkidle' to 'load' for static HTML files - Add clarifying comment about viewer CSS handling - Update documentation to reference 'load' state Co-authored-by: dorkmo <1923070+dorkmo@users.noreply.github.com>
Applied review feedback in commit ef67577: Implemented:
Not implemented (with explanation):
All actionable comments have been addressed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Screenshot Automation GitHub Action - Final
Latest Updates (Addressing Review Round 3)
✅ Code Quality Improvements
MOCK_TOKENas a constant for better maintainabilitywaitForLoadStatefrom 'networkidle' to 'load' for faster, more appropriate handling of static HTML filesPrevious Updates (Review Round 2)
✅ Documentation Fixes
waitForLoadStateinstead of outdatedwaitForTimeoutPrevious Changes (Review Round 1)
Critical Fixes
✅ CSS Inlining - Resolved
file://protocol issue where CSS wouldn't load. Now CSS is inlined directly into each HTML file during extraction.✅ HTML Extraction Logic - Fixed bug where all subsequent HTML variables were concatenated together. Now correctly stops at next variable declaration.
Robustness Improvements
✅ Concurrency Control - Changed from branch-specific to fixed concurrency group to prevent conflicts across branches
✅ Error Handling - Added try-catch to Node.js screenshot script with proper error reporting
✅ Better Wait Logic - Now uses
waitForLoadState('load')for optimal static HTML file handling✅ Improved Git Operations - Enhanced git add logic with:
✅ Optimized Checkout - Changed fetch-depth from 0 to 1 for faster workflow execution
✅ Better Logging - Added warnings for:
Documentation Updates
✅ Route Path Clarification - Explained that routes are original HTTP paths but screenshots use
file://URLs✅ Script Editing Instructions - Clarified that users must edit the workflow YAML, not
/tmp/take-screenshots.js✅ Auth Token Comment - Added note explaining the mock token is non-sensitive and only for client-side logic
✅ Maintenance Warnings - Added comments about keeping screenshots array synchronized with HTML extraction
Screenshots Automated
8 server pages + 1 viewer page = 9 total automated screenshots
Note: Client Console PIN Setup modal is documented but not auto-generated as it requires interactive page setup. The existing manual screenshot is preserved.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.