Conversation
Add comprehensive Bilibili platform support alongside existing YouTube functionality: - Implement platform detection to identify YouTube and Bilibili URLs - Add Bilibili video ID extraction supporting BV format - Update UI to display platform-specific labels and placeholders dynamically - Fix thumbnail URLs by converting HTTP to HTTPS for security The DetectPlatform method identifies video platform from URL patterns, enabling context-aware UI text and proper URL parsing for both platforms. Co-Authored-By: Claude <noreply@anthropic.com>
Add comprehensive error logging for deferred file close operations and write errors across the codebase. This ensures that errors occurring in cleanup paths are properly logged rather than silently ignored, improving observability and debugging capabilities. Changes include: - Add error logging for deferred Close() calls on files and HTTP responses - Log errors for file write operations (VTT conversion, subtitle serving) - Add error logging for environment variable updates - Ensure all deferred cleanup operations report failures Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling across the codebase by checking and logging errors that were previously ignored. The changes add proper error checking for file operations, I/O operations, and external calls, ensuring that failures are logged appropriately.
Key changes:
- Added error checking for deferred
Close()operations on files and HTTP response bodies - Added error handling for write operations (
fmt.Fprintln,io.Copy) - Added error checking for
os.Setenvandfmt.Sscanfoperations - Introduced platform detection functionality to support multiple video platforms (YouTube, Bilibili)
- Fixed thumbnail URLs by converting HTTP to HTTPS
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/utils/pathfinder.go | Added error handling for os.Setenv when updating PATH |
| internal/services/yap.go | Added error handling for SaveLog calls |
| internal/services/summarizer.go | Added error handling for deferred resp.Body.Close() |
| internal/services/subtitle_converter.go | Added error handling for fmt.Fprintln write operations |
| internal/services/storage.go | Added error handling for deferred file close |
| internal/services/settings_store.go | Added error handling for file close operations |
| internal/services/mediaserver.go | Added error handling for file operations and I/O copy |
| internal/services/downloader.go | Added error handling for SaveLog calls and implemented platform detection |
| app.go | Added error handling for various operations, removed unused error handling for WindowSetSize, added platform detection, and fixed thumbnail URLs |
| frontend/wailsjs/go/main/App.js | Added DetectPlatform function export |
| frontend/wailsjs/go/main/App.d.ts | Added TypeScript type definition for DetectPlatform |
| frontend/src/pages/NewTranscriptionPage.tsx | Implemented dynamic platform detection with adaptive UI text |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of fb5c26c...5c50dc7
Analysis
-
Missing Platform Persistence: The Task and VideoMetadata types don't include a platform field, causing platform information to be lost after task creation, preventing filtering by platform, and creating potential ambiguity with overlapping video IDs across platforms.
-
Performance Issues in Frontend: Platform detection happens on every keystroke without debouncing, causing excessive API calls, potential race conditions, and UI flickering. This implementation lacks request cancellation and stale result handling.
-
Limited Extensibility: Platform-specific logic uses switch statements with inline regex patterns rather than a strategy/plugin pattern, making it difficult to maintain as more platforms are added. There's no proper abstraction for platform-specific behaviors.
-
Inconsistent Logging Approach: The codebase mixes
log.Printffor new error logs andslog(the primary logger) elsewhere, creating inconsistency in the logging system. -
Code Duplication: HTTP→HTTPS conversion logic for thumbnails is duplicated in three places in app.go, violating DRY principles and making maintenance more difficult.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
12 files reviewed | 4 comments | Edit Agent Settings • Read Docs
internal/services/downloader.go
Outdated
| for _, pattern := range patterns { | ||
| re := regexp.MustCompile(pattern) | ||
| case "bilibili": | ||
| re := regexp.MustCompile(`(BV[a-zA-Z0-9]+)`) |
There was a problem hiding this comment.
The Bilibili BV ID regex is too permissive. Bilibili BV IDs are exactly 12 characters ("BV" + 10 alphanumeric characters), but this pattern matches any length. Use (BV[a-zA-Z0-9]{10}) instead to ensure only valid BV IDs are extracted and prevent false matches.
Agent: 🧠 Logic
Replace standard log package with log/slog across all services to enable structured logging with consistent error fields. This improves log parsing and monitoring capabilities. Changes: - Update all log.Printf calls to slog.Error with structured fields - Replace "log" imports with "log/slog" in all service files - Maintain same error handling logic with improved logging format Co-Authored-By: Claude <noreply@anthropic.com>
Mesa DescriptionTL;DRIntroduced Bilibili platform support for video transcription and significantly enhanced error handling and robustness across various backend services. What changed?
Description generated by Mesa. Update settings |
Replace hardcoded platform detection with extensible platform registry system. This enables cleaner separation of concerns and easier addition of new video platforms in the future. Changes: - Create platform.Platform interface with Registry pattern - Implement YouTubePlatform and BilibiliPlatform detectors - Extract URL pattern matching from downloader to platform implementations - Add platform field to Task and VideoMetadata types - Introduce utils.EnsureHTTPS helper for URL normalization - Update duplicate detection to consider platform in addition to video ID Co-Authored-By: Claude <noreply@anthropic.com>
Implement useDebounce hook to optimize platform detection by preventing excessive API calls while user is typing. The hook delays platform detection by 300ms after the last keystroke, improving performance and reducing unnecessary backend requests. Changes: - Add useDebounce custom hook with configurable delay - Create hooks barrel export file for better organization - Integrate debouncing in NewTranscriptionPage for URL input - Add proper cleanup with isCurrent flag to prevent race conditions Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func EnsureHTTPS(url string) string { | ||
| if strings.HasPrefix(url, "http://") { | ||
| return strings.Replace(url, "http://", "https://", 1) | ||
| } |
There was a problem hiding this comment.
The function doesn't handle URLs that are already HTTPS or URLs without a scheme. If a URL is 'https://...' or lacks 'http://', it returns unchanged, which is correct. However, if a URL lacks any scheme entirely (e.g., 'example.com/image.jpg'), it won't be modified. Consider documenting this behavior or prepending 'https://' for scheme-less URLs.
| } | |
| } | |
| if !strings.HasPrefix(url, "https://") && !strings.HasPrefix(url, "http://") { | |
| return "https://" + url | |
| } |
| if _, seekErr := file.Seek(0, 0); seekErr != nil { | ||
| slog.Error("seek file", "error", seekErr) | ||
| } | ||
| if _, copyErr := io.Copy(w, file); copyErr != nil { | ||
| slog.Error("copy file content", "error", copyErr) | ||
| } |
There was a problem hiding this comment.
After logging the seek error, the code continues to attempt io.Copy which will likely fail if seek failed. Consider returning early after seek error: if _, seekErr := file.Seek(0, 0); seekErr != nil { slog.Error(\"seek file\", \"error\", seekErr); return }
|
|
||
| if (!url.trim()) { | ||
| setError('Please enter a YouTube URL') | ||
| setError(`Please enter a ${getPlatformText().label.toLowerCase()}`) |
There was a problem hiding this comment.
The error message 'Please enter a youtube url' (when platform is YouTube) is grammatically awkward. Consider either keeping the label capitalized ('Please enter a YouTube URL') or restructuring to 'Please enter a video URL for [platform]'.
|
|
||
| if (!videoMetadata || !videoMetadata.isValid) { | ||
| setError('Please enter a valid YouTube URL') | ||
| setError(`Please enter a valid ${getPlatformText().label.toLowerCase()}`) |
There was a problem hiding this comment.
Same issue as above - 'Please enter a valid youtube url' should maintain proper capitalization. The label 'YouTube URL' should not be lowercased.
No description provided.