feat: Add theme module and enhance GUI styling#11
Conversation
- Add new theme.py module with Nord-inspired color palette - Update archive dialog with improved styling - Enhance tag editor widget appearance - Improve main window theme integration - Update CLAUDE.md with latest GUI development status 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
|
WalkthroughThis pull request introduces archive and hard-delete features with ZIP archive creation and permanent database removal, implements a new dark theme system using TOKENS-based styling, adds new service layers (git_service, archive_service, docs_discovery_service), and significantly redesigns the desktop GUI with enhanced toolbars, typography, table delegation, and themed components throughout. Changes
Sequence DiagramsequenceDiagram
participant User
participant ArchiveDialog
participant FileSystem
participant Database
participant UI
User->>ArchiveDialog: Initiate Archive
ArchiveDialog->>FileSystem: Delete original directory<br/>(elevated PowerShell)
activate FileSystem
FileSystem-->>ArchiveDialog: Deletion complete
deactivate FileSystem
ArchiveDialog->>UI: Update progress: "Creating ZIP archive"
ArchiveDialog->>FileSystem: Create ZIP from library
activate FileSystem
FileSystem-->>ArchiveDialog: ZIP created
deactivate FileSystem
ArchiveDialog->>UI: Update progress: "Updating database"
ArchiveDialog->>Database: archive_project() with ZIP metadata
activate Database
Database-->>ArchiveDialog: Archive record stored
deactivate Database
ArchiveDialog->>UI: Show success state
ArchiveDialog->>User: Archive complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Pull Request Review: Theme Module and GUI Styling EnhancementsOverviewThis PR successfully introduces a comprehensive theme system for the PySide6 desktop GUI, centralizing styling into a reusable module with Nord-inspired design tokens. The changes demonstrate good architectural thinking and improve code maintainability. ✅ Strengths1. Architecture and Design
2. Code Quality
3. UI Improvements
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/project_manager_desktop/window.py (2)
1377-1388: Path not escaped; potential command injection or failure with special characters.Unlike
archive_dialog.pywhich escapes single quotes, this method directly interpolatesproject_pathinto the PowerShell command. Paths containing quotes, backticks, or$could cause failures or security issues.🔎 Apply path escaping
+ # Escape for PowerShell double-quoted string + safe_path = project_path.replace('`', '``').replace('"', '`"').replace('$', '`$') # PowerShell command to kill processes holding handles and delete directory - ps_command = f'''$path="{project_path}"; if(Test-Path -LiteralPath $path){{ ... + ps_command = f'''$path="{safe_path}"; if(Test-Path -LiteralPath $path){{ ...Alternatively, consider refactoring to share the escaping logic with
archive_dialog.pyand use single-quoted strings consistently.
1378-1378: Inconsistent Sysinternals tool reference:handle.exevshandle64.exe.
archive_dialog.pyuseshandle64.exewhile this method useshandle.exe. For consistency and to ensure the correct version runs on 64-bit Windows, consider standardizing onhandle64.exeor using a helper that detects the available version.CLAUDE.md (1)
270-270: Update status date to reflect current year (2025).Line 270 states "Current Status (December 2024)" but the PR was created December 18, 2025. Update this to December 2025 to match the actual current date.
Also applies to: 274-274
🧹 Nitpick comments (7)
src/project_manager_desktop/widgets/tag_editor.py (2)
112-133: Consider extracting the bold font to a local variable with clearer scope.The font
fis defined forcurrent_labeland reused foravailable_label20 lines later. This works but relies on implicit scope. Consider renaming tobold_fontfor clarity.🔎 Suggested improvement
current_label = QtWidgets.QLabel("Current Tags:") - f = current_label.font() - f.setBold(True) - current_label.setFont(f) + bold_font = current_label.font() + bold_font.setBold(True) + current_label.setFont(bold_font) layout.addWidget(current_label) ... available_label = QtWidgets.QLabel("Available Tags (click to add):") - available_label.setFont(f) + available_label.setFont(bold_font)
241-250: Hardcoded accent rgba values duplicate TOKENS.accent.The
rgba(91, 140, 255, ...)values correspond toTOKENS.accent(#5b8cff). While Qt stylesheets don't support hex-to-rgba conversion, consider documenting this relationship or extracting these derived colors as additional token fields if they're used frequently.src/project_manager_desktop/theme.py (2)
255-258: Cross-platform font consideration."Segoe UI" is Windows-specific. On macOS/Linux, Qt will fall back to a default font, but the experience may be inconsistent. Consider specifying a font-family stack.
🔎 Suggested improvement
- font = QtGui.QFont("Segoe UI") + # Prefer platform-native fonts: Segoe UI (Windows), SF Pro (macOS), system default (Linux) + font = QtGui.QFont() + font.setFamilies(["Segoe UI", "-apple-system", "BlinkMacSystemFont", "Ubuntu", "Cantarell", "sans-serif"]) font.setPointSize(10)Note:
setFamilies()is available in Qt 5.13+. For older versions, the first font in the list that's available will be used.
230-249: Horizontal scrollbar styling may be needed for consistency.Only
QScrollBar:verticalis styled. If horizontal scrollbars appear (e.g., in tables or code previews), they'll use default styling. Consider adding matching horizontal scrollbar rules.🔎 Add horizontal scrollbar styling
# After line 249, add: QScrollBar:horizontal {{ background: transparent; height: 12px; margin: 0px; }} QScrollBar::handle:horizontal {{ background: rgba(154, 164, 178, 0.35); border-radius: 6px; min-width: 30px; }} QScrollBar::handle:horizontal:hover {{ background: rgba(154, 164, 178, 0.5); }} QScrollBar::add-line:horizontal, QScrollBar::sub-line:horizontal {{ width: 0px; }} QScrollBar::add-page:horizontal, QScrollBar::sub-page:horizontal {{ background: transparent; }}src/project_manager_desktop/window.py (2)
197-204: Silent exception handling for optional icon decoration.The
try-except-passpattern here is acceptable since icon loading is non-critical, but consider logging at debug level to aid troubleshooting if icons unexpectedly fail to load.🔎 Optional: Add debug logging
try: icon = QtGui.QIcon.fromTheme("edit-find") if icon.isNull(): icon = self.style().standardIcon(QtWidgets.QStyle.StandardPixmap.SP_FileDialogContentsView) act = self.search_input.addAction(icon, QtWidgets.QLineEdit.ActionPosition.LeadingPosition) act.setEnabled(False) - except Exception: - pass + except Exception as e: + # Icon decoration is non-critical; log and continue + import logging + logging.debug("Failed to add search icon: %s", e)
497-500: Consider usinghasattrinstead of try-except for attribute existence check.If the concern is that
projects_countmight not be initialized, a guard check is cleaner than exception swallowing.🔎 Suggested improvement
- try: - self.projects_count.setText(f"({len(projects)})") - except Exception: - pass + if hasattr(self, 'projects_count'): + self.projects_count.setText(f"({len(projects)})")CLAUDE.md (1)
281-339: Consider adding more detail about theme system implementation.The PR title mentions "Add theme module and enhance GUI styling," and the AI summary references a "TOKENS-based styling system" with
build_qssandapply_themefunctions. However, the CLAUDE.md updates don't include details about the newsrc/project_manager_desktop/theme.pymodule or how the Nord-inspired theme system is integrated. Consider adding a brief section explaining the theming architecture for future Claude Code sessions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.md(3 hunks)src/project_manager_desktop/dialogs/archive_dialog.py(7 hunks)src/project_manager_desktop/main.py(1 hunks)src/project_manager_desktop/theme.py(1 hunks)src/project_manager_desktop/widgets/tag_editor.py(6 hunks)src/project_manager_desktop/window.py(16 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/project_manager_desktop/dialogs/archive_dialog.py (2)
src/project_manager_cli/services/archive_service.py (1)
ArchiveService(13-166)src/project_manager_cli/services/git_service.py (1)
GitService(8-65)
src/project_manager_desktop/main.py (1)
src/project_manager_desktop/theme.py (1)
apply_theme(253-273)
src/project_manager_desktop/window.py (2)
src/project_manager_desktop/models.py (1)
ProjectsTableModel(22-127)src/core/models.py (1)
DocFile(83-96)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
284-284: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
284-284: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
🪛 Ruff (0.14.8)
src/project_manager_desktop/dialogs/archive_dialog.py
205-205: subprocess call: check for execution of untrusted input
(S603)
206-206: Starting a process with a partial executable path
(S607)
218-218: Consider moving this statement to an else block
(TRY300)
303-303: Do not catch blind exception: Exception
(BLE001)
src/project_manager_desktop/window.py
26-26: Unused noqa directive (non-enabled: N802)
Remove unused noqa directive
(RUF100)
203-204: try-except-pass detected, consider logging the exception
(S110)
203-203: Do not catch blind exception: Exception
(BLE001)
352-353: try-except-pass detected, consider logging the exception
(S110)
352-352: Do not catch blind exception: Exception
(BLE001)
499-500: try-except-pass detected, consider logging the exception
(S110)
499-499: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (8)
src/project_manager_desktop/main.py (1)
18-22: LGTM!The theme integration is correctly placed after
QApplicationcreation and beforeMainWindowinstantiation, ensuring all widgets inherit the styled palette and stylesheet.src/project_manager_desktop/dialogs/archive_dialog.py (2)
61-65: LGTM!The warning styling appropriately uses
TOKENS.warningfor the text color while using rgba variants for background and border, maintaining consistency with the theme system's semantic color usage.
146-180: Path escaping may be insufficient for PowerShell metacharacters.The escaping
path_ps = self.project_path.replace("'", "''")handles single quotes but PowerShell paths can contain other metacharacters like backticks (`) or$which could cause issues or injection vectors in the inner script.🔎 Consider additional escaping
- path_ps = self.project_path.replace("'", "''") + # Escape both single quotes and backticks for PowerShell single-quoted strings + path_ps = self.project_path.replace("'", "''").replace("`", "``")Alternatively, consider validating that
project_pathcontains only safe filesystem characters before constructing the command.Likely an incorrect or invalid review comment.
src/project_manager_desktop/theme.py (1)
8-40: Well-structured theme token system.The frozen dataclass provides immutability and clear semantic naming. The token organization (surfaces, text, borders, brand/states, radii) follows established design system conventions.
src/project_manager_desktop/window.py (3)
19-42: LGTM! Clean table delegate implementation.
ProjectsTableDelegateeffectively applies visual hierarchy by bolding the Name column and muting meta columns (Path, Tags, Updated). The palette manipulation for selected rows preserves readability.
942-1062: LGTM!The markdown preview HTML generation effectively integrates TOKENS for consistent theming. The separation of body (margin: 0) and
.doccontainer (padding: 16px) provides clean layout control.
1213-1230: LGTM!The warning label styling consistently uses
TOKENS.dangerwith appropriate rgba variants for backgrounds and borders, matching the theme system patterns established intheme.py.CLAUDE.md (1)
450-455: No changes needed for model name validity.o4-mini is OpenAI's latest small o-series model, released on April 16, 2025, to all ChatGPT users and via the Chat Completions API and Responses API. This is a distinct, valid model from gpt-4o-mini (which is in the gpt-4o series). The defensive comment "This is NOT a typo" is accurate—o4-mini is not a typo or unusual naming but an official OpenAI model identifier.
However, there is a documentation inaccuracy: the referenced file path
src/project_manager_cli/config.py:81does not exist in the repository. The actual model configuration is insrc/core/config.py(line ~44) andsrc/core/config_manager.py.Likely an incorrect or invalid review comment.
| **✅ ARCHIVE & HARD DELETE FEATURES COMPLETED** - Latest | ||
| - ✅ **Archive Project**: Backup projects to ZIP archives with library folder cleanup | ||
| - Git repository detection and uncommitted changes warning | ||
| - Automatic deletion of library folders (node_modules, venv, dist, build, __pycache__, etc.) | ||
| - ZIP archive creation with compression (excludes .git) | ||
| - Archives stored in `%APPDATA%\project-manager-cli\archives\` | ||
| - Archive naming: `{uuid}_{project-name}_YYYY-MM-DD.zip` | ||
| - Progress dialog with real-time feedback | ||
| - ✅ **Hard Delete**: Permanently remove project records from database | ||
| - Checkbox in delete dialog: "Permanently delete from database (cannot be undone)" | ||
| - Dynamic warning messages based on checkbox state | ||
| - Soft delete remains the default (enabled=0) | ||
| - ✅ **Archive Filtering**: Hide/show archived projects | ||
| - "📦 Show Archived" toggle button in toolbar | ||
| - Default view filters `enabled=1 AND archived=0` | ||
| - Database schema: Added `archived`, `archive_path`, `archive_date`, `archive_size_mb` columns | ||
| - ✅ **Directory Cleanup**: PowerShell-based physical directory deletion | ||
| - "Also delete project files from disk" checkbox in delete dialog | ||
| - "Delete original directory after archiving" checkbox in archive dialog | ||
| - Uses handle.exe to kill processes holding file locks | ||
| - Force-deletion with ownership takeover and permission grants | ||
| - CRITICAL warnings for destructive operations |
There was a problem hiding this comment.
Fix markdown strong-style formatting to use asterisks.
The static analysis tool (markdownlint-cli2) flagged MD050 violations around lines 283-284. Markdown strong emphasis should use asterisks (**text**) instead of underscores (__text__). Review the formatting in this section and ensure consistency throughout.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
284-284: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
284-284: Strong style
Expected: asterisk; Actual: underscore
(MD050, strong-style)
🤖 Prompt for AI Agents
In CLAUDE.md around lines 281 to 302, markdownlint flagged MD050 because strong
emphasis was applied using underscores; replace any strong-style underscores
(e.g., __text__) with asterisks (**text**) and ensure literal names that contain
underscores (like __pycache__) are not treated as emphasis—wrap those in
backticks (`__pycache__`) or escape the underscores. Update all occurrences in
this section to use **bold** with asterisks and use inline code for folder names
with underscores so the file passes markdownlint checks.
1 participant
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.