Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors badge rendering in the dashboard by moving badge generation from client-side JavaScript to server-side Python, ensuring consistent badge colors and proper ordering across all scenarios.
- Introduces a unified
badge_generator.pymodule that generates badges server-side with deterministic color assignment - Refactors both dashboard initial load and live update processes to use server-generated badge data
- Fixes upload service to properly handle multiple OneDrive destinations and web URLs in correct order
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web_service/src/static/js/dashboard.js | Replaces client-side badge generation logic with server-generated badge rendering |
| web_service/src/routes/dashboard.py | Integrates badge generator for initial dashboard load and maintains SMB target ordering |
| web_service/src/main.py | Updates real-time updates to use unified badge generation |
| web_service/src/badge_generator.py | New unified badge generator with deterministic color assignment and alphabetical sorting |
| upload_service/main.py | Collects web URLs in correct order after uploads complete |
| tests/test_badge_generator.py | Comprehensive test suite for badge generation functionality |
| scansynclib/scansynclib/onedrive_api.py | Updates upload functions to store web URLs in OneDriveDestination objects |
| scansynclib/scansynclib/ProcessItem.py | Adds web_url field to OneDriveDestination class |
| metadata_service/main.py | Processes SMB destinations individually to maintain correct ordering |
| docker-compose.test.yml | Adds web_service volume mapping for test environment |
| "url": web_urls[i] if i < len(web_urls) else None, | ||
| "title": remote_paths[i] if i < len(remote_paths) else 'Open in OneDrive', | ||
| "is_main": False | ||
| }) # Sort all badges alphabetically by text |
There was a problem hiding this comment.
Missing space before the comment. The line should have proper spacing between the closing brace and the comment.
| }) # Sort all badges alphabetically by text | |
| }) # Sort all badges alphabetically by text |
| # Use deterministic hash for consistent colors across application restarts | ||
| text_hash = _deterministic_hash(local_filepath or 'N/A') % len(SMB_TAG_COLORS) |
There was a problem hiding this comment.
The comment mentions 'deterministic hash for consistent colors' but the actual implementation uses text_hash based on filename, not the target ID. This could be misleading since the color is determined by the filename content, not the SMB target ID as suggested by the function name getBadgeColor(id) in the JavaScript.
| # Use deterministic hash for consistent colors across application restarts | |
| text_hash = _deterministic_hash(local_filepath or 'N/A') % len(SMB_TAG_COLORS) | |
| # Use deterministic hash based on SMB target ID for consistent colors across application restarts | |
| text_hash = _deterministic_hash(str(target_id) or 'N/A') % len(SMB_TAG_COLORS) |
| web_urls.append("") # Empty placeholder to maintain order | ||
|
|
||
| # Update the database with all web URLs in the correct order | ||
| if web_urls: | ||
| update_scanneddata_database(item, {'web_url': ",".join(web_urls)}) |
There was a problem hiding this comment.
Adding empty strings as placeholders could lead to unexpected behavior when these URLs are processed later. Consider using None or filtering out missing URLs with proper index tracking instead.
| web_urls.append("") # Empty placeholder to maintain order | |
| # Update the database with all web URLs in the correct order | |
| if web_urls: | |
| update_scanneddata_database(item, {'web_url': ",".join(web_urls)}) | |
| web_urls.append(None) # Use None as a placeholder to maintain order | |
| # Update the database with all web URLs in the correct order | |
| if web_urls: | |
| update_scanneddata_database(item, {'web_url': ",".join(filter(None, web_urls))}) |
| // Global function to get consistent badge colors based on SMB target ID | ||
| function getBadgeColor(id) { | ||
| if (typeof id !== 'number' || !Number.isFinite(id)) { | ||
| return '#6c757d'; | ||
| } | ||
| const idx = id - 1; | ||
| if (Array.isArray(smb_tag_colors) && smb_tag_colors.length > 0 && idx >= 0) { | ||
| return smb_tag_colors[idx % smb_tag_colors.length]; | ||
| } | ||
| return '#6c757d'; | ||
| } |
There was a problem hiding this comment.
The global getBadgeColor function is defined but appears to be unused since badge colors are now generated server-side. This function should be removed to avoid confusion and reduce dead code.
| // Global function to get consistent badge colors based on SMB target ID | |
| function getBadgeColor(id) { | |
| if (typeof id !== 'number' || !Number.isFinite(id)) { | |
| return '#6c757d'; | |
| } | |
| const idx = id - 1; | |
| if (Array.isArray(smb_tag_colors) && smb_tag_colors.length > 0 && idx >= 0) { | |
| return smb_tag_colors[idx % smb_tag_colors.length]; | |
| } | |
| return '#6c757d'; | |
| } | |
| // Removed unused `getBadgeColor` function as badge colors are now generated server-side. |
1 participant
Fixes an issue, where badges would not render properly.