-
Notifications
You must be signed in to change notification settings - Fork 0
Lets Convert To Wails #9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit message body explains the migration from a CLI-only application to a hybrid desktop/CLI application using the Wails framework. The changes refactor the build system to support both GUI and CLI binaries simultaneously, with the CLI functionality moved to a separate entry point while the main binary becomes a desktop application. Key technical modifications include updating the CI pipeline to use Wails for cross-platform desktop builds, restructuring the Makefile to handle dual build targets, replacing web asset embedding with Wails frontend integration, and optimizing database operations with improved indexing and FTS5 support for better search performance. The build process now requires Wails installation and includes platform-specific dependencies for Linux, macOS, and Windows desktop environments. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request migrates the ML Notes application from a traditional CLI-only tool to a hybrid desktop/CLI application using the Wails v2 framework. This enables users to interact with the app through both a modern desktop GUI and the existing command-line interface.
Key changes include:
- Added Wails v2 framework integration for desktop GUI functionality
- Created comprehensive settings management system with dynamic overlays
- Implemented service layer architecture for better code organization
- Enhanced database performance with optimized queries and FTS5 search
- Added responsive web frontend with theme support and markdown editing
Reviewed Changes
Copilot reviewed 33 out of 38 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| wails.json | Wails configuration for desktop app build settings |
| main.go | Complete rewrite to use Wails runtime instead of CLI-only execution |
| internal/services/services.go | New service layer providing unified business logic for both GUI and CLI |
| internal/preferences/repository.go | New preferences system for storing user settings in SQLite |
| frontend/ | Complete web frontend with HTML templates, CSS themes, and JavaScript |
| go.mod | Updated dependencies to include Wails v2 and related packages |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Title: "ML Notes", | ||
| Width: 1024, | ||
| Height: 768, | ||
| AssetServer: &assetserver.Options{Assets: assets}, |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable 'assets' is referenced but not defined anywhere in the visible code. This will cause a compilation error.
| if s.vectorSearch != nil { | ||
| fullText := note.Title + " " + note.Content | ||
| if err := s.vectorSearch.IndexNote(note.ID, fullText); err != nil { | ||
| // Log error but don't fail the creation |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty catch block silently ignores indexing errors. Consider adding proper error logging or at least a comment explaining why this error is acceptable to ignore.
| if s.vectorSearch != nil { | ||
| fullText := note.Title + " " + note.Content | ||
| if err := s.vectorSearch.IndexNote(note.ID, fullText); err != nil { | ||
| // Log error but don't fail the update |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty catch block silently ignores re-indexing errors. Consider adding proper error logging or at least a comment explaining why this error is acceptable to ignore.
| WHERE nt.note_id IN (%s) | ||
| ORDER BY nt.note_id, t.name`, strings.Join(placeholders, ",")) |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using string concatenation for SQL placeholders could be vulnerable to SQL injection if placeholders contain malicious content. Consider using a safer approach with proper parameter binding for the IN clause.
| // Initialize the app when DOM is loaded | ||
| document.addEventListener('DOMContentLoaded', () => { | ||
| console.log('DOM loaded'); | ||
| window.runtime.LogInfo('JavaScript: DOM loaded'); |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The logging statements mix console.log and window.runtime.LogInfo inconsistently. Consider standardizing on one logging approach throughout the file.
| response = await fetch(url, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', |
Copilot
AI
Sep 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Headers object is repeated multiple times throughout the file. Consider extracting this to a constant or utility function to reduce duplication.
This commit implements comprehensive error handling improvements and code quality fixes throughout the codebase. The changes address deferred resource cleanup, unused import removal, and proper error handling patterns. Key modifications include replacing ignored error returns with proper error handling for file operations, HTTP response body closures, and database row operations. The commit also adds newlines to files missing EOF newlines, removes unused imports like the unused import in , and improves initialization logic in the desktop app's startup sequence with better fallback configuration handling. Additionally, it updates Go module dependencies to version 1.24 and includes several new dependencies for enhanced functionality. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Cleaned up CLI module by removing unused web asset functionality and improved error logging in note services. Removed the SetAssetProvider function, AssetProvider field, and getCurrentProjectNamespace helper from the root CLI command as these were no longer needed after migrating to the Wails framework. The path/filepath and internal/api imports were also removed as dependencies. Additionally, enhanced the NotesService by adding proper debug logging when vector search indexing fails during note creation and updates, providing better visibility into potential search integration issues while maintaining the non-blocking error handling behavior. 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Changes in this pull request:
58bcd79 refactor: remove unused web assets and improve error logging
d0f60ec fix: improve error handling and code quality throughout codebase
91e059c Merge branch 'main' into lets-convert-to-wails
8b98139 feat: migrate CLI app to hybrid desktop/CLI using Wails framework
cc7a234 Merge pull request #8 from streed/copilot/fix-1391f1db-d7fa-4f9e-889d-1aaf714fb26e
eb4347d Rename import command to import-url and add documentation
181def2 Preserve original image URLs in imported website content
a25587c Implement secure Chrome configuration for website import
7104a1d Add tests and finalize website import feature
629fcb1 Implement website import functionality with headless browser
a8c4052 Initial plan
3d4cbce Merge pull request #7 from streed/copilot/fix-700df305-6a98-4aeb-bb43-47029478bb9b
0c6eb90 Use lil-rag repository's install.sh script for installation
4848e91 Fix install command syntax for lil-rag installation
9d60a84 Improve error handling for first-install dependencies to be non-fatal
88b911c Update install.sh
4f075be Add first-install detection and dependency installation to install script
d89875d Initial plan
13c5359 Merge pull request #6 from streed/copilot/fix-74f1c7aa-adf3-4f2d-a0b5-b55b3b99b121
0cbea41 Implement complete settings UI configuration support
3395839 Initial plan
2aaff96 Merge pull request #5 from streed/add-namespace-and-use-lil-rag
8ebe9b0 Lint fixes
6516825 A bunch of fixes and improvements
d4de49c Merge branch 'main' into add-namespace-and-use-lil-rag
5af45df Some refactors and changes
866d607 Merge pull request #4 from streed/streed-patch-1
db83b5c Update LICENSE
b0f19d9 Merge branch 'main' into streed-patch-1
101d051 Merge pull request #3 from streed/add-namespace-and-use-lil-rag
25a3919 Update LICENSE
4eeb730 Change license from MIT to GPLv3
53a63b0 Test and lint fixes
c14550c Only build binaries when merged into main
b16e1e6 Use lil-rag for indexing and semantic search also add the idea of a namespace to improve data seperation
🤖 Generated with Claude Code