Tauri #19
Tauri #19
Conversation
- Fix test command from 'bun test' to 'bun run test:jest' - Reduce tmate debugging timeout from 30 to 3 minutes - Add test file ignores to ESLint configuration - Exclude test files from TypeScript compilation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Replace any[] with unknown[] in auto-scroll components and hooks - Replace any with unknown in LLM adapters and service error handling - Add proper type assertions in config service for backend data conversion - Improve error handling with type guards in update services 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Replace external mock attempts with FileService's internal mock system - Fix test expectations to match the service's actual mock behavior - Ensure tests work correctly with the service's built-in mocking infrastructure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove unused Button and DEFAULT_PROMPT_TEMPLATE imports from LLM settings - Remove unused imports from various component files - Fix React Hook dependency arrays in settings components - Clean up unused variables in tabs and test files 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Fix Usage type conversion in anthropic adapter by adding unknown cast - Import SupportedLanguage type and use proper type assertions in config service - Fix theme type to match UIConfig interface constraints 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add stricter type definitions for UI theme and custom languages in config-service - Add explicit type assertions for Anthropic adapter's usage field - Remove tmate debugging step from build workflow to streamline CI process
Change default bundle identifier from com.tauri.dev to dev.ryuzu.minecraftmodslocalizer to resolve Tauri build error requiring unique application identifier
…ic key in config - Add GITHUB_TOKEN, TAURI_SIGNING_PRIVATE_KEY, and TAURI_SIGNING_PRIVATE_KEY_PASSWORD to build workflow - Update Tauri public key in `tauri.conf.json`
…etting - Merge DEFAULT_API_CONFIG into DEFAULT_CONFIG for single source of truth - Add temperature configuration option for LLM adapters (default: 1.0) - Update all LLM adapters to use config-based temperature setting - Add temperature control to settings UI with validation (0.0-2.0 range) - Update config service conversion functions for temperature field - Maintain backward compatibility with legacy DEFAULT_API_CONFIG export
- Add token estimation utility using word-based proxy calculation - Implement smart chunking to prevent LLM token overflow - Add configuration options for token-based chunking - Update TranslationService with hybrid chunking strategy - Enhance BaseLLMAdapter with token-aware methods - Add comprehensive unit and integration tests - Maintain backward compatibility with entry-based chunking Resolves TX014: Token counter prevents translation failures from exceeding model token limits while preserving content quality. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…s tracking - Add session-based translation logging with statistics and performance metrics - Implement file-level progress tracking with detailed chunk and key counts - Add enhanced log categorization and filtering in UI dialogs - Update parameter naming from process_type to processType for Tauri v2 compliance - Add milestone progress reporting for better user feedback - Integrate performance monitoring throughout translation pipeline
- Enable token-based chunking by default (useTokenBasedChunking: true) - Pass token configuration to TranslationService constructor - Reduce default token limit from 5000 to 3000 for safety - Add model-specific token limits to OpenAI adapter - Use adapter's getMaxTokensPerChunk() method in TranslationService - Increase token estimation overhead for more accurate calculations - Update word-to-token ratios to be more conservative This fixes the 'maximum context length exceeded' errors by properly limiting chunk sizes based on estimated token counts.
…rols - Fix config serialization to include token-based chunking fields in save/load - Add UI controls for token-based chunking settings in translation settings - Add debugging logs to track chunking behavior and configuration - Expose useTokenBasedChunking, maxTokensPerChunk, and fallbackToEntryBased settings - Add helpful descriptions and validation for token limits This should resolve the missing token configuration that was causing the chunking system to not work properly.
- Add ErrorLogger utility for centralized error handling - Remove ApiKeyError throwing from translation services - Change console.error to console.log for API key issues - Handle empty translation results without throwing exceptions - Prevent frequent debug notifications during development
- Add backward progress prevention in store setters - Remove duplicate incrementCompletedMods call in mods-tab - Add debug logging for progress changes - Ensure consistent chunk-based progress tracking across all tabs
- Add 'use client' directive to i18n module - Refactor UpdateNotificationButton to accept title as prop - Add suppressHydrationWarning to body element - Ensure consistent language detection between server and client - Fix "title" attribute mismatch in update notification
- Add sonner toast library for user feedback - Fix settings not taking effect without restart - Clear config cache and reload after save - Add toast notifications for settings save feedback - Ensure immediate settings application
- Add filtering for already-translated files in backend - Remove existing language suffixes before adding new ones - Fix log display filtering during active translation - Add comprehensive i18n support for settings dialog - Enhance translation logging with session tracking
- Create centralized constants directory for defaults - Move API_DEFAULTS and TRANSLATION_DEFAULTS to constants - Add translation helper utilities - Update imports across codebase - Improve token counter configuration
- Add bunfig.toml for bun test configuration - Update jest config to ignore bun test files - Fix test mock data for new configuration structure - Add test directory structure - Ensure tests work with refactored constants
- Update README documentation - Improve tab components structure - Enhance file service error handling - Update Rust mod analysis logic - Clean up settings tab implementation - Add mod_analysis/ to gitignore
Implements comprehensive backup functionality for translation files:
Core Features:
- Automatic backup creation before overwriting translations
- Session-based backup organization in logs/localizer/{session}/backups/
- Backup management UI with filtering, restoration, and deletion
- Configurable retention policies and storage management
- Non-blocking backup integration with translation workflow
Backend Implementation:
- Add 7 Tauri commands in src-tauri/src/backup.rs
- Session-based directory structure using existing logging infrastructure
- Metadata tracking with translation statistics
Frontend Implementation:
- BackupService with comprehensive API (create, list, restore, delete, prune)
- BackupDialog component for backup management
- BackupSettings component for configuration
- Integration with existing UI patterns and shadcn/ui
Configuration:
- Extend AppConfig with BackupConfig interface
- Default settings: enabled=true, 30-day retention, 10 backups per type
- Settings UI with storage usage display and manual pruning
Testing:
- 20+ test cases covering all backup service methods
- Mock Tauri environment setup for comprehensive testing
- Error handling and edge case coverage
Closes: T016 - Implement Translation Backup System
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
…B quests - Add support for multiple FTB Quest directory structures - Check standard path (config/ftbquests/quests/) first - Support FTB Interactions Remastered path (config/ftbquests/normal/) - Add fallback to root directory (config/ftbquests/) - Implement recursive SNBT file scanning for nested structures - Enhance error messages to show all checked directories - Add comprehensive tests for nested quest structures - Update README documentation with supported structures Fixes #9
- Add .lang format writer to Rust backend with key=value output - Extend write_lang_file to accept optional format parameter - Update FileService TypeScript interface to support format specification - Add format detection during mod analysis (detects from en_us file) - Modify ModInfo struct to include langFormat field - Update TranslationTarget interface to store langFormat - Add visual format indicator in mods table UI (shows JSON/LANG badges) - Create tests for both .lang and .json file writing - Add documentation for legacy lang file support - Ensure backward compatibility with existing JSON support This enables full support for Minecraft 1.12.2 and earlier mods that use the legacy .lang file format instead of JSON. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…nslation support - Add detection for config/betterquesting/DefaultQuests.lang files - Parse .lang format (key=value) and convert for translation - Format translated output back to .lang format - Update UI to show "Better Quest (Direct)" for DefaultQuests - Add comprehensive tests for both Bun and Jest - Update documentation to reflect both standard and direct approaches Addresses GitHub Issue #11
- Fix backup config state updates to use local backupConfig - Update test assertions for async error handling - Ensure proper state isolation in settings updates
- Update test setup configuration - Modify progress tracking E2E tests - Remove outdated test fixture file
- Add new badge UI component - Update global type definitions
- Add TranslationJob type import - Remove unused parameters in callback functions
WalkthroughThis update introduces comprehensive enhancements across the translation application. Major new features include robust backup management (with UI, backend, and service integration), token-based chunking for translation jobs, and extensive progress tracking improvements. The configuration system is refactored for centralized defaults and new options. Logging, error handling, and internationalization are expanded. Numerous new and updated tests validate translation, chunking, backup, and file handling logic. Documentation is updated to reflect new features, directory structures, and supported formats. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Store
participant FileService
participant BackupService
participant TranslationService
participant TauriBackend
User->>UI: Opens Settings/Backup Dialog
UI->>BackupService: listBackups()
BackupService->>TauriBackend: list_backups()
TauriBackend-->>BackupService: [BackupInfo[]]
BackupService-->>UI: [BackupInfo[]]
UI->>User: Displays backups
User->>UI: Starts Translation
UI->>TranslationService: startJob()
TranslationService->>TauriBackend: log_translation_start()
loop For each chunk
TranslationService->>TauriBackend: translate_chunk()
TranslationService->>TauriBackend: log_file_progress()
end
TranslationService->>TauriBackend: log_translation_completion()
TranslationService-->>UI: Job complete
User->>UI: Initiates Backup
UI->>BackupService: createBackup(options)
BackupService->>TauriBackend: create_backup(metadata, files)
TauriBackend-->>BackupService: backup path
BackupService-->>UI: Backup created
User->>UI: Restores Backup
UI->>BackupService: restoreBackup(backupId, targetPath)
BackupService->>TauriBackend: restore_backup(backupId, targetPath)
TauriBackend-->>BackupService: success/failure
BackupService-->>UI: Restore result
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
🔭 Outside diff range comments (3)
src/components/tabs/mods-tab.tsx (1)
189-197: Progress tracking setup is inconsistentThe function sets up both mod-level progress (
setTotalMods) and chunk-level progress (setTotalChunks), but the translation runner only increments chunks. This creates an inconsistency wheretotalModsis set butcompletedModsis never incremented.Either:
- Remove the mod-level progress setup if it's not being used
- Implement proper mod-level progress tracking alongside chunk tracking
- Document why this dual setup exists but only one is used
src-tauri/src/minecraft/mod.rs (1)
465-547: Significant code duplication between extract functions.The
extract_lang_files_from_archiveandextract_lang_files_from_archive_with_formatfunctions share nearly identical logic. The only difference is that the latter also detects and returns the format.Refactor to have one function call the other:
/// Extract language files from a JAR archive for a specific language fn extract_lang_files_from_archive( archive: &mut ZipArchive<File>, mod_id: &str, target_language: &str, ) -> Result<Vec<LangFile>> { let (files, _format) = extract_lang_files_from_archive_with_format(archive, mod_id, target_language)?; Ok(files) }This eliminates the code duplication while maintaining backward compatibility.
Also applies to: 549-642
src/lib/services/translation-service.ts (1)
926-949: Consider using a specific error type instead of returning empty object for API key errors.While the API key error detection is good, returning an empty object might mask the actual issue. Consider throwing a specific error that can be handled appropriately by the caller.
// Check if the error is related to missing API key if (error instanceof Error && (error.message.includes("API key is not configured") || error.message.includes("Incorrect API key provided: undefined"))) { await this.logError("API key is not configured or is invalid. Please set your API key in the settings.", "TRANSLATION"); - // For API key configuration errors, return empty result to mark as failed - return {}; + // For API key configuration errors, throw a specific error + throw new Error("API_KEY_ERROR: " + error.message); }Then update the chunk processing in
startJobto handle this specific error type appropriately.
🧹 Nitpick comments (31)
README.md (1)
115-117: Badge alt-text is missingFor accessibility, add an
altattribute describing the badge.- +src/lib/services/tauri-update-service.ts (1)
12-12: Consider a dedicated Manifest type
Record<string, unknown>is safer thanany, but we already know the shape (version/date/body). Declaring it improves autocomplete and downstream checks:+interface UpdateManifest { + version: string + date: string + body: string +} + export interface UpdateResult { shouldUpdate: boolean; - manifest?: Record<string, unknown>; + manifest?: UpdateManifest; error?: string; }src/components/layout/header.tsx (1)
89-89: Simplify the complex conditional logic for better readability.The nested ternary operator on line 89 is difficult to read and maintain. Consider extracting this logic into a helper function or using early returns.
+ const getUpdateButtonTitle = () => { + if (!mounted) return 'Check for updates'; + return updateInfo?.updateAvailable + ? t('update.newVersionAvailable', 'New version available') + : t('update.checkForUpdates', 'Check for updates'); + }; + // ... <UpdateNotificationButton hasUpdate={updateInfo?.updateAvailable || false} onClick={handleUpdateClick} - title={mounted ? (updateInfo?.updateAvailable ? t('update.newVersionAvailable', 'New version available') : t('update.checkForUpdates', 'Check for updates')) : 'Check for updates'} + title={getUpdateButtonTitle()} />docs/spec.md (1)
417-420: Fix markdown list indentation.The content updates are excellent and clearly document the expanded BetterQuests support. However, the nested list items need consistent indentation.
- Check both standard and direct locations for quest files: - - Standard: Create new `{language name}.json` based on the contents of `resources/betterquesting/lang/en_us.json` - - Direct: Create new `DefaultQuests.{language name}.lang` based on the contents of `config/betterquesting/DefaultQuests.lang` - Support both JSON and .lang file formats + Check both standard and direct locations for quest files: + - Standard: Create new `{language name}.json` based on the contents of `resources/betterquesting/lang/en_us.json` + - Direct: Create new `DefaultQuests.{language name}.lang` based on the contents of `config/betterquesting/DefaultQuests.lang` + Support both JSON and .lang file formatssrc/lib/services/update-service.ts (1)
194-194: Fix confusing use of 'this' in static context.Using
thisin a static method can be confusing. Use the class name directly for clarity.- await this.logError(`Failed to open URL: ${error instanceof Error ? error.message : String(error)}`); + await UpdateService.logError(`Failed to open URL: ${error instanceof Error ? error.message : String(error)}`);src/__tests__/services/translation-service.bun.test.ts (1)
216-221: Token estimation validation could be more robust.The token estimation validation uses a hardcoded calculation that may not match the actual implementation. Consider using the same estimation logic as the service or making the test more flexible.
-const estimatedTokens = Object.entries(chunk.content) - .reduce((sum, [key, value]) => sum + Math.ceil((key.length + value.length) / 3), 0); -expect(estimatedTokens).toBeLessThanOrEqual(150); // Allow some overhead +// Use a more flexible validation approach +expect(Object.keys(chunk.content).length).toBeGreaterThan(0); +expect(chunk.content).toBeDefined();src/lib/services/__tests__/translation-service-token-chunking.test.ts (2)
40-42: Consider using more realistic token estimation for large content tests.The test creates 50 items with moderately long descriptions, but the token estimation might not be realistic enough to consistently trigger multiple chunks with a 1000 token limit.
Consider making the content more predictably large to ensure reliable chunking:
- for (let i = 0; i < 50; i++) { - largeContent[`item.test.item_${i}`] = `This is a test item with a longer description that contains multiple words and should contribute to token count. Item number ${i}.`; - } + for (let i = 0; i < 50; i++) { + largeContent[`item.test.item_${i}`] = `This is a test item with a much longer description that contains many more words and should definitely contribute significantly to the overall token count for this specific test case. Item number ${i} with additional padding text.`; + }
187-187: Unrealistic content for token limit testing.The
.repeat(10)creates extremely long content that may not represent real-world translation scenarios and could cause token estimation issues.Consider using more realistic mixed content:
- "long": "This is a very long text that contains many words and should contribute significantly to the token count estimation. ".repeat(10) + "long": "This is a comprehensive description of a complex game mechanic that includes detailed explanations, multiple examples, and thorough documentation of all related features and interactions within the game system."src/lib/utils/__tests__/token-counter.test.ts (1)
67-67: Consider making the large content test more predictable.Using
"a ".repeat(1000)creates a very large string, but the exact token count may vary depending on the estimation algorithm.Consider using a more predictable approach for large content testing:
- "test": "a ".repeat(1000) // Very long content + "test": Array(1000).fill("word").join(" ") // Predictable word count for token estimationsrc/__tests__/services/translation-service.test.ts (1)
313-317: Token estimation logic in test may not be accurate.The test uses a simplified token estimation (
(key.length + value.length) / 3) that may not match the actual token estimation used by the service.Consider using the actual token estimation utility or a more realistic estimation:
job.chunks.forEach(chunk => { - const estimatedTokens = Object.entries(chunk.content) - .reduce((sum, [key, value]) => sum + Math.ceil((key.length + value.length) / 3), 0); - expect(estimatedTokens).toBeLessThanOrEqual(150); // Allow some overhead + // Verify chunk content exists and is reasonable + expect(Object.keys(chunk.content).length).toBeGreaterThan(0); + expect(Object.keys(chunk.content).length).toBeLessThanOrEqual(10); // Reasonable chunk size });src/__tests__/e2e/fixtures/quests/ftb/quests/chapters/getting_started.snbt (1)
16-16: Consider using unique ID prefixes for test fixturesThe quest and task IDs follow a pattern like "0000000000000001" which could collide with IDs in other test fixture files (e.g.,
advanced.snbtuses "1000000000000001"). Consider using unique prefixes for each chapter to avoid potential conflicts during testing.Also applies to: 36-36, 65-65, 74-74
src/__tests__/e2e/fixtures/quests/ftb/quests/chapters/advanced.snbt (1)
16-16: Consider using descriptive IDs for better test maintainabilityWhile the "100000000000000X" pattern is unique to this chapter, consider using more descriptive IDs like "advanced_tech_power_gen_001" for better test readability and maintainability.
Also applies to: 32-33
src/lib/services/__tests__/betterquest-service.jest.test.ts (1)
111-174: Consider moving .lang file parsing tests to a dedicated utility testThese tests for parsing and formatting
.langfiles appear to be testing frontend logic within a service test file. Since theFileServiceis primarily responsible for file discovery and the parsing/formatting logic would likely be used elsewhere in the application, consider:
- Creating a dedicated utility module for
.langfile parsing and formatting- Moving these tests to a separate test file for that utility
- Keeping this test file focused on the
FileServicefile discovery functionalityThis would improve test organization and make the parsing logic reusable.
src/lib/store/index.ts (1)
171-183: Consider consolidating milestone logging logicBoth
incrementCompletedChunksandincrementCompletedModsimplement identical milestone logging logic. Consider extracting this into a shared utility function to reduce code duplication.+// Add near the top of the file +const logProgressMilestone = async (progress: number, prevProgress: number, message: string) => { + const currentMilestone = Math.floor(progress / 25) * 25; + const prevMilestone = Math.floor(prevProgress / 25) * 25; + + if (currentMilestone > prevMilestone && progress > 0) { + try { + const { invoke } = await import("@tauri-apps/api/core"); + await invoke('log_translation_process', { message }); + } catch (error) { + console.error('Failed to log progress milestone:', error); + } + } +}; incrementCompletedChunks: () => set((state) => { // ... existing logic ... - // Log progress milestones (every 25%) - const prevProgress = state.totalChunks > 0 ? Math.round((state.completedChunks / state.totalChunks) * 100) : 0; - const currentMilestone = Math.floor(boundedProgress / 25) * 25; - const prevMilestone = Math.floor(prevProgress / 25) * 25; - - if (currentMilestone > prevMilestone && boundedProgress > 0) { - // Log milestone reached (async operation, don't await to avoid blocking state update) - import("@tauri-apps/api/core").then(({ invoke }) => { - invoke('log_translation_process', { - message: `Translation progress milestone: ${currentMilestone}% (${newCompletedChunks}/${state.totalChunks} chunks)` - }).catch(error => console.error('Failed to log progress milestone:', error)); - }); - } + // Log progress milestones (don't await to avoid blocking state update) + const prevProgress = state.totalChunks > 0 ? Math.round((state.completedChunks / state.totalChunks) * 100) : 0; + logProgressMilestone( + boundedProgress, + prevProgress, + `Translation progress milestone: ${Math.floor(boundedProgress / 25) * 25}% (${newCompletedChunks}/${state.totalChunks} chunks)` + ); return { completedChunks: newCompletedChunks, wholeProgress: boundedProgress }; }),Also applies to: 221-234
src-tauri/src/filesystem.rs (2)
8-8: Remove unused regex import.The
regex::Regeximport appears to be unused in this file. The language suffix filtering is implemented using string contains checks instead of regex patterns.-use regex::Regex;
129-133: Consider more precise language suffix detection.The current string contains check for language suffixes could produce false positives. For example, a file named
my.ja_jp.backup.jsonwould be skipped even though it might not be a translated language file.Consider using a more precise pattern that checks for the language suffix followed by the file extension:
fn has_language_suffix(filename: &str) -> bool { const LANGUAGE_CODES: &[&str] = &[ "ja_jp", "zh_cn", "ko_kr", "de_de", "fr_fr", "es_es", "it_it", "pt_br", "ru_ru" ]; if let Some(stem) = filename.strip_suffix(".json").or_else(|| filename.strip_suffix(".snbt")) { return LANGUAGE_CODES.iter().any(|code| stem.ends_with(code)); } false }Also applies to: 185-189, 250-254
src/lib/services/__tests__/filesystem-service.jest.test.ts (1)
1-116: Consider maintaining error handling test coverage.While the refactoring to use built-in mocks simplifies the tests, it appears to remove all error handling test scenarios that were likely present in the previous version. Error handling is crucial for file system operations.
Consider adding some error handling tests back, such as:
describe('error handling', () => { it('should handle file system errors gracefully', async () => { // Override the mock to simulate an error FileService.setTestInvokeOverride(async () => { throw new Error('File system error'); }); await expect(FileService.getFTBQuestFiles('/invalid/path')) .rejects.toThrow('File system error'); }); });src/lib/utils/error-logger.ts (1)
47-47: Unused processType parameter in warning and info methods.The
processTypeparameter is declared but not used in thelogWarningandlogInfomethods, whilelogErroruses it correctly.Either use the parameter or remove it for consistency:
static async logWarning(context: string, message: string, processType: string = 'SYSTEM'): Promise<void> { console.warn(`[${context}] ${message}`); try { await invoke('log_translation_process', { - message: `[WARNING] [${context}] ${message}` + message: `[WARNING] [${context}] ${message}`, + processType }); } catch (error) { console.error('[ErrorLogger] Failed to send warning to backend:', error); } }Also applies to: 62-62
src/__tests__/e2e/progress-tracking-e2e.vitest.test.ts (1)
17-17: Consider using a more robust path construction for test directories.Using
process.cwd()with a hard-coded relative path could be fragile across different test environments and CI systems.Consider using a test utility or environment variable:
- outputDir = path.join(process.cwd(), 'src/__tests__/e2e/fixtures/output/progress-test'); + outputDir = path.join(__dirname, 'fixtures/output/progress-test');src/components/tabs/quests-tab.tsx (1)
114-301: Consider extracting helper functions to reduce complexity.The
handleTranslatefunction is quite long (187 lines) and handles multiple responsibilities. Consider extracting the following into separate helper functions for better maintainability:
.langfile parsing logic (lines 152-171).langfile formatting logic (lines 226-244)- Output file path generation logic (lines 246-265)
Example extraction:
const parseLangFile = (content: string): Record<string, string> => { const langMap: Record<string, string> = {}; const lines = content.split('\n'); for (const line of lines) { const trimmed = line.trim(); if (trimmed && !trimmed.startsWith('#')) { const separatorIndex = trimmed.indexOf('='); if (separatorIndex > -1) { const key = trimmed.substring(0, separatorIndex).trim(); const value = trimmed.substring(separatorIndex + 1).trim(); langMap[key] = value; } } } return langMap; }; const formatLangFile = (translatedMap: Record<string, string>): string => { const langLines: string[] = []; const sortedKeys = Object.keys(translatedMap).sort(); for (const key of sortedKeys) { langLines.push(`${key}=${translatedMap[key]}`); } return langLines.join('\n'); };src/__tests__/services/translation-runner.test.ts (1)
227-330: Good cancellation test coverage with room for one additional scenario.The cancellation tests are comprehensive and verify important behaviors. Consider adding one more test case:
it('should handle interruption during chunk translation', async () => { const job: TranslationJob = { id: 'job-1', status: 'pending', progress: 0, totalChunks: 1, targetLanguage: 'ja_jp', chunks: [{ id: 'chunk-1', content: { 'key1': 'Value 1' }, status: 'pending' }], startTime: 0, endTime: 0 }; options.jobs = [job]; // Simulate interruption during translation (not just before) mockTranslationService.translateChunk.mockImplementation(() => { // Set interruption flag while translation is in progress mockTranslationService.isJobInterrupted.mockReturnValue(true); return Promise.resolve({ 'key1': '値1' }); }); await runTranslationJobs(options); // Verify the job was marked as interrupted even though translation completed expect(mockCallbacks.onJobInterrupted).toHaveBeenCalled(); });src/lib/services/__tests__/betterquest-service.bun.test.ts (1)
86-93: Use a more idiomatic way to fail the test.Instead of
expect(true).toBe(false), consider using a more explicit approach.-expect(true).toBe(false); // Should not reach here +throw new Error('Expected error to be thrown');Or use Bun's built-in expectation:
-try { - await FileService.invoke('get_better_quest_files', { dir: '/invalid' }); - expect(true).toBe(false); // Should not reach here -} catch (error) { - expect(error.message).toBe(errorMessage); -} +await expect(FileService.invoke('get_better_quest_files', { dir: '/invalid' })) + .rejects.toThrow(errorMessage);src/components/ui/log-dialog.tsx (1)
112-115: Avoid hardcoded string matching for log filteringChecking for hardcoded strings like
'[ErrorLogger]'is fragile and could break if the log format changes. Consider using a more robust approach.Consider adding a dedicated log category or field for error context logs instead of string matching:
- // 1.5. Show ErrorLogger messages (they contain important context) - if (log.message.includes('[ErrorLogger]') || log.message.includes('TranslationService.logError')) { - return true; - } + // 1.5. Show error context logs + if (log.process_type === 'ERROR_CONTEXT' || log.process_type === 'TRANSLATION_ERROR') { + return true; + }src/__tests__/e2e/translation-e2e.test.ts (2)
346-354: Consider enhancing the debugging output.The current debugging helps when quest keys are missing, but consider also logging the quest database structure to help diagnose extraction issues.
// If no quest keys found, check what was actually extracted if (questKeys.length === 0) { console.log('Extracted translatable strings:', Object.keys(translatableStrings)); console.log('Translations received:', Object.keys(translations)); + console.log('Quest database keys:', Object.keys(questDb)); + console.log('First quest entry:', questDb[Object.keys(questDb)[0]]); }
388-418: Placeholder test for future integration workflow.This test stub outlines the structure for a comprehensive integration test. The comment clearly indicates this is intentional for future implementation.
Would you like me to help implement the full integration workflow test that covers multiple file types with progress tracking?
src/__tests__/e2e/realistic-progress-e2e.test.ts (1)
52-64: Clarify the total entry count in the test.The loop creates 250 iterations but generates 2 entries per iteration (item and description), resulting in 500 total entries. This correctly produces 10 chunks with size 50.
test('should show realistic progress for large mod translation', async () => { - // Create a realistic mod with 250 entries (5 chunks of 50 each) + // Create a realistic mod with 500 entries (250 items × 2 entries each = 10 chunks of 50) const largeMod: Record<string, string> = {}; for (let i = 0; i < 250; i++) {src/lib/services/config-service.ts (1)
183-186: Use class name instead ofthisin static context.In static methods, using the class name is clearer and avoids confusion about what
thisrefers to.- // Clear the loaded flag to force reload on next access - // This ensures that any components using getConfig() will get the updated values - this.loaded = false; - - // Immediately reload the config to ensure consistency - await this.load(); + // Clear the loaded flag to force reload on next access + // This ensures that any components using getConfig() will get the updated values + ConfigService.loaded = false; + + // Immediately reload the config to ensure consistency + await ConfigService.load();src-tauri/src/backup.rs (2)
369-381: Extract nested function as a module-level helper.The
calculate_dir_sizefunction would be cleaner as a separate helper function.Consider moving this function to module level:
+/// Calculate the total size of a directory recursively +fn calculate_dir_size(dir: &Path) -> Result<u64, std::io::Error> { + let mut size = 0u64; + for entry in fs::read_dir(dir)? { + let entry = entry?; + let path = entry.path(); + if path.is_dir() { + size += calculate_dir_size(&path)?; + } else { + size += entry.metadata()?.len(); + } + } + Ok(size) +} pub fn get_backup_storage_size( logger: State<Arc<AppLogger>>, ) -> Result<u64, String> { // ... existing code ... - - // Calculate size recursively for all backup directories - fn calculate_dir_size(dir: &Path) -> Result<u64, std::io::Error> { - // ... function body ... - }
207-208: Enhance backup restoration validation.The current validation only checks if files exist but doesn't verify their validity or readability.
Consider adding more robust validation:
// Check if backup can be restored (files exist and are readable) let can_restore = if original_files_dir.exists() { original_files_dir.read_dir() .map(|entries| { entries.filter_map(Result::ok) .any(|entry| { entry.path().is_file() && entry.metadata().map(|m| m.len() > 0).unwrap_or(false) }) }) .unwrap_or(false) } else { false };src/lib/services/translation-service.ts (2)
116-146: Consider making debug logging conditional.The constructor properly initializes token-based chunking configuration. However, the debug
console.logstatement should be conditional to avoid cluttering production logs.Consider wrapping the debug logging in a development check:
- // Debug logging for token-based chunking - console.log('TranslationService Configuration:', { - useTokenBasedChunking: this.useTokenBasedChunking, - maxTokensPerChunk: this.maxTokensPerChunk, - fallbackToEntryBased: this.fallbackToEntryBased, - provider: this.adapter.id - }); + // Debug logging for token-based chunking + if (process.env.NODE_ENV === 'development') { + console.log('TranslationService Configuration:', { + useTokenBasedChunking: this.useTokenBasedChunking, + maxTokensPerChunk: this.maxTokensPerChunk, + fallbackToEntryBased: this.fallbackToEntryBased, + provider: this.adapter.id + }); + }
663-691: Consider making chunking debug logs conditional.The token-based chunking implementation with fallback mechanism is well-designed. However, the debug
console.logstatements should be conditional.- console.log(`Chunking ${entries.length} entries - useTokenBasedChunking: ${this.useTokenBasedChunking}`); + if (process.env.NODE_ENV === 'development') { + console.log(`Chunking ${entries.length} entries - useTokenBasedChunking: ${this.useTokenBasedChunking}`); + }Apply similar changes to lines 671, 673, 677, 680, 686, and 688.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (107)
.github/workflows/build.yml(1 hunks).gitignore(1 hunks)README.md(2 hunks)bunfig.toml(1 hunks)docs/TASK_008_Fix_Progress_Calculation_And_History_Dialog_Issues.md(0 hunks)docs/TASK_009_Fix_Mod_Progress_Calculation_And_Add_Alphabetical_Sorting.md(0 hunks)docs/TASK_010_Fix_Tauri_Errors_And_Mod_Level_Progress.md(0 hunks)docs/TASK_011_Fix_History_Dialog_And_Enhance_Completion_Dialog.md(0 hunks)docs/TASK_012_Improve_Target_Language_Selector_UI_And_Consolidate_Language_Selection.md(0 hunks)docs/TASK_013_Install_And_Implement_Storybook.md(0 hunks)docs/TESTING.md(0 hunks)docs/auto-scroll-integration.md(0 hunks)docs/ci-cd.md(0 hunks)docs/lang-file-support.md(1 hunks)docs/spec.md(3 hunks)docs/tasks.md(0 hunks)eslint.config.mjs(1 hunks)jest.config.js(1 hunks)package.json(1 hunks)public/locales/en/common.json(2 hunks)public/locales/ja/common.json(2 hunks)src-tauri/src/backup.rs(1 hunks)src-tauri/src/filesystem.rs(7 hunks)src-tauri/src/lib.rs(3 hunks)src-tauri/src/logging.rs(1 hunks)src-tauri/src/minecraft/mod.rs(10 hunks)src-tauri/src/tests/lang_file_test.rs(1 hunks)src-tauri/src/tests/mod.rs(1 hunks)src-tauri/tauri.conf.json(2 hunks)src/__tests__/adapters/openai-adapter.test.ts(1 hunks)src/__tests__/components/translation-tab.test.tsx(1 hunks)src/__tests__/e2e/fixtures/README.md(1 hunks)src/__tests__/e2e/fixtures/mods/complex-mod/assets/complexmod/lang/en_us.json(1 hunks)src/__tests__/e2e/fixtures/mods/realistic-rpg-mod/assets/rpgmod/lang/en_us.json(1 hunks)src/__tests__/e2e/fixtures/mods/sample-mod/assets/samplemod/lang/en_us.json(1 hunks)src/__tests__/e2e/fixtures/mods/tech-automation-mod/assets/techmod/lang/en_us.json(1 hunks)src/__tests__/e2e/fixtures/output/DefaultQuests.ja_jp.json(1 hunks)src/__tests__/e2e/fixtures/output/getting_started.ja_jp.snbt(1 hunks)src/__tests__/e2e/fixtures/output/samplemod_ja_jp.json(1 hunks)src/__tests__/e2e/fixtures/quests/better/DefaultQuests.json(1 hunks)src/__tests__/e2e/fixtures/quests/ftb/quests/chapters/advanced.snbt(1 hunks)src/__tests__/e2e/fixtures/quests/ftb/quests/chapters/getting_started.snbt(1 hunks)src/__tests__/e2e/fixtures/quests/ftb/quests/chapters/rpg_adventure.snbt(1 hunks)src/__tests__/e2e/progress-tracking-e2e.bun.test.ts(1 hunks)src/__tests__/e2e/progress-tracking-e2e.vitest.test.ts(1 hunks)src/__tests__/e2e/realistic-content-e2e.test.ts(1 hunks)src/__tests__/e2e/realistic-progress-e2e.test.ts(1 hunks)src/__tests__/e2e/realistic-translation-e2e.test.ts(1 hunks)src/__tests__/e2e/translation-e2e-simple.test.ts(1 hunks)src/__tests__/e2e/translation-e2e.test.ts(1 hunks)src/__tests__/services/file-service-lang-format.test.ts(1 hunks)src/__tests__/services/translation-runner.test.ts(1 hunks)src/__tests__/services/translation-service.bun.test.ts(1 hunks)src/__tests__/services/translation-service.test.ts(1 hunks)src/__tests__/tabs/mods-tab.test.tsx(1 hunks)src/__tests__/test-setup.ts(1 hunks)src/app/layout.tsx(2 hunks)src/components/layout/header.tsx(2 hunks)src/components/settings/backup-settings.tsx(1 hunks)src/components/settings/llm-settings.tsx(4 hunks)src/components/settings/settings-dialog.tsx(3 hunks)src/components/settings/target-language-dialog.tsx(0 hunks)src/components/settings/translation-settings.tsx(1 hunks)src/components/tabs/common/translation-tab.tsx(1 hunks)src/components/tabs/custom-files-tab.tsx(5 hunks)src/components/tabs/mods-tab.tsx(7 hunks)src/components/tabs/quests-tab.tsx(1 hunks)src/components/tabs/settings-tab.tsx(2 hunks)src/components/tabs/target-language-selector.tsx(0 hunks)src/components/ui/auto-scroll-area.tsx(2 hunks)src/components/ui/backup-dialog.tsx(1 hunks)src/components/ui/badge.tsx(1 hunks)src/components/ui/log-dialog.tsx(15 hunks)src/components/ui/sonner.tsx(1 hunks)src/components/ui/update-notification-button.tsx(1 hunks)src/hooks/use-auto-scroll.ts(2 hunks)src/lib/adapters/anthropic-adapter.ts(6 hunks)src/lib/adapters/base-llm-adapter.ts(1 hunks)src/lib/adapters/gemini-adapter.ts(4 hunks)src/lib/adapters/openai-adapter.ts(6 hunks)src/lib/constants/defaults.ts(1 hunks)src/lib/i18n.ts(2 hunks)src/lib/services/__tests__/backup-service.bun.test.ts(1 hunks)src/lib/services/__tests__/betterquest-service.bun.test.ts(1 hunks)src/lib/services/__tests__/betterquest-service.jest.test.ts(1 hunks)src/lib/services/__tests__/filesystem-service.bun.test.ts(2 hunks)src/lib/services/__tests__/filesystem-service.jest.test.ts(1 hunks)src/lib/services/__tests__/translation-runner.bun.test.ts(0 hunks)src/lib/services/__tests__/translation-runner.jest.test.ts(0 hunks)src/lib/services/__tests__/translation-service-token-chunking.test.ts(1 hunks)src/lib/services/__tests__/translation-service.bun.test.ts(4 hunks)src/lib/services/__tests__/translation-service.jest.test.ts(2 hunks)src/lib/services/backup-service.ts(1 hunks)src/lib/services/config-service.ts(6 hunks)src/lib/services/file-service.ts(6 hunks)src/lib/services/tauri-update-service.ts(1 hunks)src/lib/services/translation-runner.ts(1 hunks)src/lib/services/translation-service.ts(18 hunks)src/lib/services/update-service.ts(6 hunks)src/lib/store/index.ts(3 hunks)src/lib/test-utils/mock-data.ts(1 hunks)src/lib/types/config.ts(6 hunks)src/lib/types/llm.ts(1 hunks)src/lib/types/minecraft.ts(6 hunks)src/lib/utils/__tests__/token-counter.test.ts(1 hunks)src/lib/utils/error-logger.ts(1 hunks)src/lib/utils/token-counter.ts(1 hunks)
⛔ Files not processed due to max files limit (3)
- src/lib/utils/translation-helpers.ts
- src/types/global.d.ts
- tsconfig.json
💤 Files with no reviewable changes (14)
- src/components/settings/target-language-dialog.tsx
- src/lib/services/tests/translation-runner.bun.test.ts
- src/components/tabs/target-language-selector.tsx
- src/lib/services/tests/translation-runner.jest.test.ts
- docs/TASK_008_Fix_Progress_Calculation_And_History_Dialog_Issues.md
- docs/tasks.md
- docs/TASK_009_Fix_Mod_Progress_Calculation_And_Add_Alphabetical_Sorting.md
- docs/TASK_011_Fix_History_Dialog_And_Enhance_Completion_Dialog.md
- docs/TASK_012_Improve_Target_Language_Selector_UI_And_Consolidate_Language_Selection.md
- docs/auto-scroll-integration.md
- docs/TESTING.md
- docs/TASK_013_Install_And_Implement_Storybook.md
- docs/TASK_010_Fix_Tauri_Errors_And_Mod_Level_Progress.md
- docs/ci-cd.md
🧰 Additional context used
🧬 Code Graph Analysis (19)
src/__tests__/services/file-service-lang-format.test.ts (1)
src/lib/services/file-service.ts (1)
FileService(188-382)
src/lib/services/__tests__/translation-service-token-chunking.test.ts (2)
src/lib/types/llm.ts (1)
LLMConfig(69-88)src/lib/services/translation-service.ts (2)
TranslationService(88-1072)TranslationServiceOptions(59-82)
src/hooks/use-auto-scroll.ts (1)
src/lib/utils/scroll.ts (1)
scrollToBottom(25-33)
src/components/ui/badge.tsx (1)
src/lib/utils.ts (1)
cn(4-6)
src/lib/adapters/anthropic-adapter.ts (3)
src/lib/services/file-service.ts (1)
invoke(202-209)src/lib/constants/defaults.ts (1)
MODEL_TOKEN_LIMITS(84-101)src/lib/types/config.ts (1)
DEFAULT_API_CONFIG(189-193)
src/lib/services/update-service.ts (2)
src/lib/constants/defaults.ts (1)
UPDATE_DEFAULTS(125-130)src-tauri/src/logging.rs (1)
error(139-141)
src/lib/utils/__tests__/token-counter.test.ts (1)
src/lib/utils/token-counter.ts (5)
estimateTokens(45-77)DEFAULT_TOKEN_CONFIG(37-37)exceedsTokenLimit(100-107)calculateOptimalChunkSize(116-142)estimateTokensForProvider(150-175)
src/lib/services/__tests__/betterquest-service.jest.test.ts (1)
src/lib/services/file-service.ts (1)
FileService(188-382)
src/__tests__/services/translation-service.test.ts (4)
src/lib/services/translation-service.ts (1)
TranslationService(88-1072)src/lib/adapters/openai-adapter.ts (1)
OpenAIAdapter(12-245)src/lib/adapters/anthropic-adapter.ts (1)
AnthropicAdapter(12-239)src/lib/types/llm.ts (1)
TranslationResponse(21-33)
src/lib/adapters/openai-adapter.ts (4)
src/lib/services/file-service.ts (1)
invoke(202-209)src-tauri/src/logging.rs (1)
error(139-141)src/lib/constants/defaults.ts (1)
MODEL_TOKEN_LIMITS(84-101)src/lib/types/config.ts (1)
DEFAULT_API_CONFIG(189-193)
src/components/ui/backup-dialog.tsx (9)
src/lib/services/backup-service.ts (2)
BackupInfo(72-79)backupService(279-279)src/lib/i18n.ts (1)
useAppTranslation(41-43)src/components/ui/card.tsx (1)
Card(80-80)src/components/ui/badge.tsx (1)
Badge(36-36)src/components/ui/button.tsx (1)
Button(59-59)src/lib/types/minecraft.ts (1)
TranslationTargetType(8-8)src/lib/services/file-service.ts (1)
FileService(188-382)src/components/ui/dialog.tsx (5)
Dialog(125-125)DialogContent(127-127)DialogHeader(130-130)DialogTitle(133-133)DialogFooter(129-129)src/components/ui/scroll-area.tsx (1)
ScrollArea(58-58)
src/lib/store/index.ts (2)
src/lib/services/file-service.ts (1)
invoke(202-209)src-tauri/src/logging.rs (1)
error(139-141)
src/lib/utils/error-logger.ts (6)
src-tauri/src/logging.rs (1)
error(139-141)src/lib/services/file-service.ts (1)
invoke(202-209)src/lib/adapters/anthropic-adapter.ts (1)
logError(49-55)src/lib/adapters/openai-adapter.ts (1)
logError(49-55)src/lib/adapters/gemini-adapter.ts (1)
logError(49-55)src/lib/services/translation-service.ts (1)
logError(225-233)
src/lib/services/config-service.ts (2)
src/lib/types/config.ts (2)
AppConfig(28-41)DEFAULT_CONFIG(143-183)src/lib/types/llm.ts (1)
SupportedLanguage(93-100)
src-tauri/src/filesystem.rs (2)
src-tauri/src/logging.rs (2)
debug(124-126)info(129-131)src-tauri/src/minecraft/mod.rs (4)
content(401-402)serde_json(248-248)serde_json(670-670)serde_json(713-713)
src/components/tabs/quests-tab.tsx (6)
src/lib/store/index.ts (1)
useAppStore(78-296)src/lib/services/file-service.ts (2)
FileService(188-382)invoke(202-209)src/lib/types/minecraft.ts (2)
TranslationTarget(103-120)TranslationResult(142-157)src/lib/services/translation-service.ts (2)
TranslationService(88-1072)TranslationJob(27-54)src/lib/services/translation-runner.ts (1)
runTranslationJobs(36-273)src/components/tabs/common/translation-tab.tsx (1)
TranslationTab(95-593)
src/__tests__/e2e/realistic-content-e2e.test.ts (2)
src/lib/services/file-service.ts (1)
FileService(188-382)src/lib/services/translation-service.ts (1)
TranslationService(88-1072)
src/components/settings/backup-settings.tsx (8)
src/lib/types/config.ts (1)
AppConfig(28-41)src/lib/i18n.ts (1)
useAppTranslation(41-43)src/lib/services/backup-service.ts (1)
backupService(279-279)src/components/ui/card.tsx (5)
Card(80-80)CardHeader(80-80)CardTitle(80-80)CardDescription(80-80)CardContent(80-80)src/components/ui/label.tsx (1)
Label(24-24)src/components/ui/switch.tsx (1)
Switch(31-31)src/components/ui/input.tsx (1)
Input(21-21)src/components/ui/button.tsx (1)
Button(59-59)
src/lib/services/backup-service.ts (1)
src/lib/types/minecraft.ts (1)
TranslationTargetType(8-8)
🪛 markdownlint-cli2 (0.17.2)
docs/spec.md
418-418: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
419-419: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 Biome (1.9.4)
src/lib/services/update-service.ts
[error] 194-194: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/lib/utils/error-logger.ts
[error] 7-71: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
src/lib/services/config-service.ts
[error] 183-183: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 186-186: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 266-266: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
src/components/tabs/common/translation-tab.tsx
[error] 296-296: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/__tests__/e2e/translation-e2e-simple.test.ts
[error] 62-62: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
src/__tests__/e2e/realistic-translation-e2e.test.ts
[error] 118-120: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (158)
src/hooks/use-auto-scroll.ts (1)
25-25: Good type improvement!Changing from
any[]tounknown[]is a better practice that maintains type safety while still allowing flexibility.src/components/ui/auto-scroll-area.tsx (1)
43-43: Type consistency maintained.Good job keeping the type change consistent with the hook's interface update.
src-tauri/src/tests/mod.rs (1)
1-3: LGTM! Clean test module declaration.The test module is properly declared with the correct
#[cfg(test)]attribute, ensuring it's only compiled during test builds.package.json (1)
48-48: LGTM! Toast notification library addition.The
sonnerdependency is properly added with semantic versioning. This supports the new toast notification features mentioned in the UI components..gitignore (1)
56-56: LGTM! Appropriate gitignore addition.Adding
mod_analysis/to gitignore is the right approach for temporary analysis data that shouldn't be tracked in version control.bunfig.toml (1)
1-2: LGTM! Standard Bun test configuration.The test preload configuration follows Bun best practices for global test setup. This will ensure consistent test environment initialization across the test suite.
src/lib/types/llm.ts (1)
86-87: LGTM! Well-documented temperature property addition.The temperature property is properly added as optional with clear documentation about the expected range (0.0 to 2.0). This follows TypeScript interface conventions and maintains backward compatibility.
src/__tests__/e2e/fixtures/mods/sample-mod/assets/samplemod/lang/en_us.json (1)
1-18: Fixture looks goodNo structural or formatting issues found in the new language file. The
%splaceholder insamplemod.message.errorwill be preserved as-is by most translation pipelines, which is desirable.README.md (1)
13-16: Nice clarification of supported FTB quest layoutsThe additional bullet points clearly document the non-standard directory variants. 👍
src/lib/services/__tests__/translation-service.jest.test.ts (2)
133-134: Variable omission is fineThe unused variable was removed without affecting assertions – reduces lint noise.
185-185: Instance not referencedCreating
TranslationServicewithout capturing the reference is okay here because the test only verifies side-effects (mockAdapter.getMaxChunkSize). No action required..github/workflows/build.yml (1)
37-37: Ensuretest:jestscript exists
bun run test:jestwill fail if the script is missing or renamed. Double-checkpackage.json:"scripts": { "test:jest": "jest --runInBand" }src/lib/test-utils/mock-data.ts (1)
162-167: LGTM! Good refactoring for quest type unification.The refactoring from separate quest types to a unified
'quest'type with aquestFormatproperty is a solid design improvement. This approach provides better extensibility for supporting additional quest formats while maintaining consistency in the type system.jest.config.js (1)
16-20: Clarify the rationale for excluding core test files.These excluded test files cover critical functionality (translation services, adapters, and core UI tabs). Excluding them from the main Jest test suite could reduce coverage and make it easier to miss regressions.
Could you clarify:
- Are these tests meant to run with a different test runner (e.g., Bun as suggested by line 15)?
- Do they have specific environment requirements that prevent them from running in the standard Jest suite?
- How will these tests be executed in CI/CD to ensure they don't get forgotten?
src-tauri/tauri.conf.json (3)
5-5: LGTM! Proper application identifier.The change from the generic
"com.tauri.dev"to the application-specific"dev.ryuzu.minecraftmodslocalizer"follows proper reverse domain naming conventions and is more appropriate for the actual application.
9-10: LGTM! More explicit build commands.The update to use
"bun run dev"and"bun run build"instead of the shorter forms is more explicit and follows standard npm script conventions, making the configuration clearer.
46-46: Verify the new public key is properly configured.The updater public key has been changed, which is important for verifying update signatures. Ensure that:
- The corresponding private key is securely stored for signing releases
- The key pair was generated specifically for this application
- The build/release process is updated to use the new signing key
src/app/layout.tsx (3)
4-4: LGTM! Proper import for global toast notifications.Adding the Toaster import at the layout level is the correct approach for establishing a global notification system across the application.
30-30: LGTM! Addressing hydration warnings.Adding
suppressHydrationWarningto the body element is appropriate when using components that might render differently on server and client (like the Toaster component).
33-33: LGTM! Proper placement of global Toaster component.Placing the Toaster component at the end of the body element is the correct approach for global toast notifications. This ensures it renders above other content and is available throughout the application.
src/components/ui/update-notification-button.tsx (1)
8-8: LGTM! Improved component design with better separation of concerns.The addition of the optional
titleprop with a sensible default value makes this component more reusable and removes the translation responsibility from the component itself. This follows React best practices by making the component more focused and testable.Also applies to: 11-11, 17-17
src/lib/services/__tests__/translation-service.bun.test.ts (2)
5-5: LGTM! Improved test reliability with explicit mocking.The explicit import and mocking of
LLMAdapterFactoryensures consistent adapter behavior across tests and makes the test dependencies clearer. This is a good practice for test maintainability.Also applies to: 37-39
141-141: LGTM! Good cleanup of unused variables.Removing the unused variable assignments improves code cleanliness without affecting test logic.
Also applies to: 193-193
src/__tests__/e2e/fixtures/output/samplemod_ja_jp.json (1)
1-18: LGTM! Well-structured test fixture for Japanese localization.The JSON structure follows standard Minecraft mod localization conventions, and the "[JA]" prefix clearly indicates these are test translations. This fixture will effectively support end-to-end translation testing.
src/lib/i18n.ts (3)
1-1: LGTM! Correct Next.js client directive.The 'use client' directive properly marks this module for client-side execution in Next.js.
32-33: LGTM! Appropriate configuration for React and language detection.Setting
useSuspense: falseprevents hydration mismatches in Next.js, and the detection configuration with localStorage and navigator is a sensible approach for language persistence.Also applies to: 34-37
12-39: Confirmed single i18n initialization
Only one occurrence ofi18n.isInitializedwas found (src/lib/i18n.ts:12), so there’s no risk of duplicate initializations in this codebase. Given JavaScript’s single-threaded execution, concurrent init calls aren’t possible here—no changes required.src/__tests__/services/file-service-lang-format.test.ts (1)
1-87: Excellent comprehensive test coverage for format support.This test suite provides thorough coverage of the
writeLangFilemethod's format functionality. The tests properly verify default format behavior, explicit format specification, and error handling scenarios with realistic test data.src/components/ui/sonner.tsx (1)
6-26: Clean wrapper implementation with proper theme integration.The component properly integrates with next-themes and forwards props correctly. The type casting for the theme prop is appropriate for compatibility with Sonner's expected types.
src/__tests__/e2e/fixtures/mods/complex-mod/assets/complexmod/lang/en_us.json (1)
1-27: Comprehensive and realistic test fixture for Minecraft mod localization.This JSON fixture provides excellent coverage of typical Minecraft mod localization patterns, including items, blocks, GUI elements, tooltips, and dynamic placeholders. The key naming conventions and content structure align perfectly with Minecraft standards, making it ideal for realistic e2e testing scenarios.
docs/lang-file-support.md (1)
1-50: Excellent comprehensive documentation for language file format support.This documentation clearly explains the dual format support, automatic detection, visual indicators, and compatibility benefits. The examples are accurate and the structure is well-organized for user understanding.
src/components/settings/settings-dialog.tsx (2)
13-13: Proper integration of BackupSettings component.The BackupSettings component is correctly imported and placed in a logical position within the settings sections.
Also applies to: 125-127
17-17: Enhanced user experience with toast notifications and config refresh.The addition of toast notifications provides excellent user feedback for save operations. The config refresh after saving ensures the UI stays synchronized with the backend state, which is particularly important for backup settings that may undergo server-side validation.
Also applies to: 32-41, 44-48
src/lib/adapters/base-llm-adapter.ts (2)
51-59: LGTM! Conservative token limit with clear documentation.The method provides a sensible conservative default of 3000 tokens per chunk, which helps prevent token overflow across different LLM models. The documentation clearly states that individual adapters can override this for model-specific limits.
61-71: Well-structured token overhead estimation.The method provides reasonable overhead estimates for different prompt components. The returned object structure is clear and the values seem appropriate for typical LLM usage patterns.
src/lib/adapters/gemini-adapter.ts (4)
6-6: Good use of centralized constants.Importing
MODEL_TOKEN_LIMITSfrom centralized constants improves maintainability and consistency across adapters.
51-51: Improved logging consistency.Changing from
process_typeto camelCaseprocessTypeimproves consistency with other adapters and JavaScript naming conventions.
68-79: Enhanced error handling for missing API keys.This graceful handling of missing API keys is much better than throwing an error. Returning an empty translation response allows the system to continue processing while properly logging the configuration issue.
98-99: Good use of centralized configuration.Using
DEFAULT_API_CONFIG.temperaturefor fallback andMODEL_TOKEN_LIMITS.gemini.maxOutputTokensfor token limits improves consistency and maintainability.src/__tests__/e2e/fixtures/mods/realistic-rpg-mod/assets/rpgmod/lang/en_us.json (1)
1-85: Well-structured test fixture with comprehensive RPG localization data.This fixture provides excellent test coverage for complex translation scenarios:
- Proper JSON structure with logical grouping
- Realistic use of Minecraft formatting codes (§) and placeholders (%d, %s)
- Comprehensive coverage of RPG elements (items, blocks, entities, GUI, skills, etc.)
- Appropriate complexity for testing translation chunking and formatting preservation
The fixture will be valuable for validating translation accuracy and formatting integrity in realistic scenarios.
src/components/tabs/settings-tab.tsx (4)
13-14: Good addition of internationalization and user feedback support.Adding
useAppTranslationandtoastimports enables better user experience with localized notifications.
18-18: Proper hook usage for translations.Correctly using the translation hook to access localized strings.
27-35: Excellent enhancement to save feedback and state consistency.The improvements include:
- Refreshing config from the service to ensure UI consistency
- Clear success feedback with localized messages
- Proper error handling with user-friendly notifications
This ensures the user interface stays synchronized with the saved configuration and provides immediate feedback on save operations.
37-41: Improved error handling with user feedback.Replacing console-only error logging with user-facing toast notifications provides much better user experience while maintaining the debug logging.
src/__tests__/e2e/fixtures/output/getting_started.ja_jp.snbt (1)
1-82: Well-structured quest configuration fixture for translation testing.This SNBT fixture provides excellent test data for quest translation scenarios:
- Proper SNBT format with correct syntax
- Logical quest progression with dependencies
- Realistic quest structure including tasks, rewards, and descriptions
- Good coverage of different quest elements (items, XP, descriptions)
- Appropriate for testing translation output formatting
The "[JA]" prefix indicates this is a Japanese translation output fixture, which aligns with the filename suggesting Japanese locale (
ja_jp).src/__tests__/e2e/fixtures/quests/better/DefaultQuests.json (1)
1-99: Well-structured test fixture for Better Questing.The JSON structure is properly formatted and contains realistic quest data that will be valuable for testing Better Questing translation functionality. The fixture includes comprehensive quest properties, tasks, rewards, and quest line positioning data.
docs/spec.md (1)
450-450: Documentation enhancement for format support.Great addition clarifying that BetterQuests translations can be output in both JSON and .lang formats, matching the original directory structure and format.
src/__tests__/test-setup.ts (1)
1-46: Excellent test setup for mocking Tauri APIs.This comprehensive mock implementation provides:
- Proper global window object setup
- Complete coverage of Tauri commands used in tests
- Appropriate mock responses for different command types
- Good debugging support with console logging
- Graceful handling of unmocked commands
The setup will enable robust testing of Tauri-dependent functionality.
src/lib/services/update-service.ts (3)
3-10: Good refactoring to use centralized configuration.Moving from hardcoded values to
UPDATE_DEFAULTSimproves maintainability and makes the service more configurable. The dynamic extraction of GitHub user and repo from the configuration is clean.
117-125: Improved error handling with proper type safety.The enhanced error handling using
unknowntype and type guards is a good practice that provides better type safety while maintaining error information when available.Also applies to: 170-181, 192-199
209-211: Clean simplification of catch blocks.Removing unused error parameters in catch blocks where error details aren't needed is a good cleanup that improves code clarity.
Also applies to: 224-226
src-tauri/src/tests/lang_file_test.rs (3)
11-50: Comprehensive test for JSON format writing.The test properly verifies:
- File creation with correct path and extension
- Content serialization and deserialization
- Proper directory structure creation
Good use of temporary directories for test isolation.
52-94: Thorough test for .lang format writing.Excellent validation of the legacy .lang format:
- Verifies correct file extension (.lang)
- Checks key=value format in output
- Ensures content is properly sorted
- Validates line count matches expected output
96-128: Good test for default behavior.Testing the default format behavior (should default to JSON when no format is specified) ensures the API has sensible defaults and maintains backward compatibility.
src/__tests__/services/translation-service.bun.test.ts (5)
1-10: LGTM! Well-structured test imports and Tauri API mocking.The test setup correctly mocks the Tauri API invoke function and imports necessary testing utilities from Bun.
17-36: Excellent test setup with proper mock management.The beforeEach/afterEach hooks properly manage mock state and restore original functions, ensuring test isolation. The mock adapter setup is comprehensive with all necessary methods.
38-98: Comprehensive job creation tests with good coverage.The tests properly validate job structure, chunking behavior, and edge cases. The test for multiple chunks correctly verifies the chunking algorithm (5 items / 2 per chunk = 3 chunks).
145-168: Excellent retry logic testing with realistic failure scenarios.The test properly simulates progressive failures (network error, timeout) before success, validating the retry mechanism works as expected.
170-188: Good API key error handling validation.Correctly tests that API key errors are not retried, which is the expected behavior for authentication failures.
src/lib/adapters/anthropic-adapter.ts (5)
6-6: Good import of centralized token limits.Importing MODEL_TOKEN_LIMITS helps standardize token limit management across adapters.
51-51: Consistent logging parameter naming.The change from
process_typetoprocessTypealigns with camelCase convention and matches other adapters.
67-78: Improved error handling for missing API keys.Returning an empty response instead of throwing allows the translation service to handle API key errors gracefully. This approach is consistent with other adapters and prevents cascading failures.
152-154: Safer cache token extraction with type assertions.The refactored cache token extraction using type assertions and optional chaining is more robust and prevents potential runtime errors.
227-227: Consistent token limit usage in validation.Using
MODEL_TOKEN_LIMITS.anthropic.validationTokens(10 tokens) for API key validation is appropriate and consistent with the centralized token limit approach.src/__tests__/e2e/fixtures/output/DefaultQuests.ja_jp.json (1)
1-101: Well-structured quest configuration fixture for testing.The JSON structure correctly represents a Better Questing format with proper nesting and type annotations. The [JA] prefixes in quest names and descriptions clearly indicate this is Japanese translated content for testing purposes. The fixture includes comprehensive quest elements: properties, tasks, rewards, and quest line layout.
src/components/settings/llm-settings.tsx (6)
9-11: Good import consolidation and centralized defaults.Adding
DEFAULT_API_CONFIGimport and cleaning up unused imports improves code organization and uses centralized configuration defaults.
45-45: Proper useEffect dependency array.Adding
setConfigto the dependency array ensures the effect runs when the configuration setter changes, following React best practices.
64-66: Excellent internationalization of provider labels.Replacing hardcoded provider names with translation keys (
t('settings.providers.openai'), etc.) improves localization support.
114-120: Good use of centralized defaults for maxRetries.Using
DEFAULT_API_CONFIG.maxRetriesfor both value fallback and placeholder ensures consistency across the application.
123-140: Well-implemented temperature control with proper validation.The temperature input includes appropriate validation (min=0, max=2, step=0.1) and uses centralized defaults. The hint text helps users understand the parameter's purpose.
143-170: Complete internationalization of prompt settings.All labels, placeholders, and help text now use translation keys, ensuring consistent localization throughout the component.
src/__tests__/e2e/fixtures/mods/tech-automation-mod/assets/techmod/lang/en_us.json (1)
1-111: Comprehensive and well-structured mod localization fixture.This English localization file provides excellent test coverage with:
- Proper Minecraft localization key conventions (e.g.,
block.techmod.*,item.techmod.*)- Realistic formatting placeholders for dynamic content (
%d,%s)- Appropriate use of Minecraft color codes for styled tooltips
- Comprehensive coverage of mod features: items, blocks, GUI, messages, JEI integration, advancements, and configuration
- Well-organized structure that mirrors real mod localization files
This fixture will provide robust test data for translation system validation.
src/lib/services/__tests__/translation-service-token-chunking.test.ts (2)
10-18: Mock configuration looks comprehensive.The mock LLM config includes all necessary properties for testing token-based chunking functionality, with realistic values that mirror actual usage patterns.
61-61: Potential issue with very long content generation.Using
"a ".repeat(2000)creates a 4000-character string, which may not behave consistently across different token estimation algorithms.Consider using more realistic long content that better represents actual translation scenarios:
- "very.long.entry": "a ".repeat(2000) // Very long text that exceeds token limit + "very.long.entry": "This is a very long description that contains multiple sentences with realistic vocabulary and structure. ".repeat(50) + "This ensures the content exceeds token limits while maintaining realistic characteristics for translation testing purposes."Likely an incorrect or invalid review comment.
src/lib/utils/__tests__/token-counter.test.ts (2)
132-138: Good handling of unknown provider gracefully.The test correctly verifies that unknown providers fall back to default configuration without throwing errors, which is important for robustness.
159-159: Excellent edge case coverage for word counting.The test comprehensively covers edge cases including empty strings, whitespace-only content, and multiple spaces, which are important for accurate token estimation.
src/lib/adapters/openai-adapter.ts (5)
61-73: Robust implementation of token limits with model-specific handling.The method correctly iterates through model prefixes and provides a sensible default fallback. The token limits are conservative, which is appropriate for chunking.
84-101: Improved error handling for missing API keys.The enhanced error handling now logs detailed information about the API key state and returns an empty response instead of throwing, which is better for graceful degradation.
103-109: Valuable debug logging for troubleshooting.The added debug logging provides useful information for diagnosing configuration issues while maintaining security by not logging the actual API key content.
148-150: Improved temperature configuration handling.The change to use nullish coalescing (
??) ensures that a configured temperature of 0 is respected, rather than falling back to the default.
168-171: Enhanced safety in token extraction from API response.The explicit casting and checking of nested usage details makes the code more robust against API response variations.
src/lib/services/__tests__/filesystem-service.bun.test.ts (3)
14-17: Essential cleanup addition for test isolation.The afterEach hook properly resets the test invoke override, ensuring tests don't interfere with each other.
91-166: Comprehensive coverage of directory structure variations.The new test block thoroughly covers different FTB quest directory layouts including standard, nested, and fallback structures, which is crucial for supporting various modpack configurations.
138-150: Excellent error message validation.The test properly validates that error messages include all checked paths, which helps users understand what directories were searched when quest files aren't found.
src/__tests__/e2e/fixtures/quests/ftb/quests/chapters/getting_started.snbt (1)
55-80: Verify if missing rewards is intentionalThe "Mining Time" quest doesn't define any rewards, unlike the other quests in this chapter. Is this intentional for testing purposes, or should rewards be added for consistency?
src/components/ui/badge.tsx (1)
1-36: Clean badge component implementation!The badge component is well-structured with proper variant management using CVA, correct TypeScript typing, and follows React best practices.
src/components/ui/backup-dialog.tsx (1)
53-58: LGTM! File size formatting is properly handledThe function correctly handles the edge case when bytes is 0 and uses appropriate logarithmic scaling for different size units.
src/lib/services/file-service.ts (2)
24-24: Good safety improvement for SSR environments!The optional chaining on
document.documentElementprevents potential runtime errors in non-browser contexts.
69-71: Excellent testability enhancement!The test invoke override pattern provides a clean way to inject mock implementations for testing without modifying the core logic. The implementation correctly prioritizes the test override before falling back to SSR mock or real Tauri invocation.
Also applies to: 158-162, 189-195
src/lib/services/__tests__/betterquest-service.jest.test.ts (1)
91-108: Refine test regex for translated filesThe test “should skip already translated files” is meant to filter out files with both source and target language codes (e.g.
quests.en_us.ja_jp.json), not the default English files likeen_us.json. On line 106, the current regex/\.[a-z]{2}_[a-z]{2}\.(json|lang)$/will match default-language files. Update it to only match true translations, for example:
/\.[a-z]{2}_[a-z]{2}\.[a-z]{2}_[a-z]{2}\.(json|lang)$/• File: src/lib/services/tests/betterquest-service.jest.test.ts – line 106: replace the regex as shown above
• Confirm that the backend RPCget_better_quest_filesuses the same filtering rule before finalizing the test changesrc/__tests__/components/translation-tab.test.tsx (1)
1-607: Well-structured and comprehensive test suiteThis test suite provides excellent coverage of the TranslationTab component with:
- Proper mocking of all dependencies
- Comprehensive test scenarios covering rendering, interactions, and edge cases
- Good use of React Testing Library and user-event for realistic interaction testing
- Clear test organization with descriptive names
src/lib/store/index.ts (1)
138-155: Good defensive programming for progress trackingThe implementation correctly prevents progress from moving backwards, which helps maintain UI consistency. The warning logs are helpful for debugging any issues with progress calculation.
src/components/settings/translation-settings.tsx (1)
61-135: Well-implemented token-based chunking settingsThe new token-based chunking configuration section is well-structured with:
- Clear UI hierarchy and conditional rendering
- Proper input validation with min/max bounds
- Helpful hint text for users
- Correct immutable state updates
src-tauri/src/filesystem.rs (3)
162-216: Good implementation of fallback directory search.The multi-directory search for FTB quests with clear error messaging when no directories are found is a solid improvement that handles different modpack structures gracefully.
269-281: Good addition of direct config file support.Adding support for the direct
DefaultQuests.langlocation provides better coverage for Better Questing configurations.
456-516: Well-implemented format parameter support.The optional format parameter with proper JSON parsing, legacy
.langformat generation, and sorted output is correctly implemented. The default to JSON format maintains backward compatibility.public/locales/en/common.json (1)
38-79: Well-structured localization additions.The new localization keys are well-organized and provide comprehensive coverage for the new LLM settings, token-based chunking configuration, and user feedback messages. The nested structure for providers and tokenBasedChunking maintains clarity.
src/__tests__/e2e/progress-tracking-e2e.bun.test.ts (1)
1-274: Excellent comprehensive progress tracking test suite.This E2E test suite provides thorough coverage of progress tracking scenarios with realistic setup, proper mocking, and detailed verification of incremental progress updates. The tests cover multiple important scenarios including large mod chunking, concurrent translation, timing verification, and graceful error handling.
public/locales/ja/common.json (1)
34-79: Localization additions look good!The new translation keys for AI settings, token-based chunking, and save status messages follow consistent naming patterns and structure.
src-tauri/src/lib.rs (2)
7-10: Module additions are well-structured.Good use of conditional compilation for the test module and clean separation of the backup functionality.
19-20: Comprehensive API expansion looks good.The new logging and backup commands are well-organized and properly exposed through the Tauri invoke handler. The grouping with comments improves maintainability.
Also applies to: 105-120
src/__tests__/e2e/fixtures/README.md (1)
1-105: Excellent test fixture documentation!This comprehensive documentation provides valuable context for understanding the test data patterns, edge cases, and best practices. It will greatly help developers working with the E2E tests.
src/components/tabs/quests-tab.tsx (4)
3-9: LGTM! Clean and organized imports.The imports are well-organized and all appear to be used within the component.
174-176: Verify content wrapping and unwrapping logic.There appears to be an inconsistency in how content is handled:
- Line 175: Content is wrapped as
{content: processedContent}- Line 212: Accessing
content.contentsuggests double wrappingThis could lead to undefined behavior if the content structure doesn't match expectations.
Please verify that the translation service expects content wrapped in an object and that
getResultContentreturns the expected structure.Also applies to: 212-212
303-312: LGTM! Clean type rendering logic.The function correctly handles both quest formats and provides clear differentiation for direct mode Better Quests.
314-358: LGTM! Well-structured component with proper prop passing.The component properly configures the TranslationTab with all necessary props and custom rendering for the type column.
src/__tests__/services/translation-runner.test.ts (4)
1-68: LGTM! Comprehensive test setup with proper mocking.The test setup is well-structured with thorough mocking of the translation service and all callback functions.
70-173: LGTM! Thorough testing of basic job processing scenarios.The tests properly verify:
- Job lifecycle callbacks (start, complete)
- Translation chunk processing
- Output writing and result generation
- Sequential processing of multiple jobs
- Cleanup after completion
175-225: LGTM! Excellent coverage of multi-chunk processing.The test properly verifies:
- All chunks are processed sequentially
- Chunk status is updated correctly
- Translated content is stored in each chunk
- Progress callbacks are called for each chunk
332-488: LGTM! Comprehensive error handling and progress tracking tests.The tests properly verify:
- Chunk failures don't stop processing of other chunks
- Failed jobs are marked with success: false
- Write failures are handled gracefully
- Progress tracking uses appropriate display names
- All error scenarios maintain proper state
src/lib/services/__tests__/backup-service.bun.test.ts (6)
1-35: LGTM! Proper Tauri environment mocking.The test setup correctly mocks the Tauri environment including both window and document objects needed for environment detection.
37-119: LGTM! Thorough backup creation tests.The tests properly verify:
- Backup creation with all required metadata
- Unique ID generation with proper format
- Error handling and exception propagation
- Correct parameter passing to Tauri invoke
121-189: LGTM! Comprehensive backup listing tests.All filter combinations are properly tested with correct parameter verification.
191-235: LGTM! Proper restore and delete operation tests.Both success and failure scenarios are well-tested with appropriate error message verification.
237-315: LGTM! Complete coverage of pruning and utility methods.All backup service methods are properly tested including edge cases like zero retention and null returns.
317-378: LGTM! Excellent error handling and ID generation tests.The tests properly verify:
- Error thrown when not in Tauri environment
- Window object is correctly restored after test
- Backup ID format follows expected pattern
- Special characters in source names are properly sanitized
src/__tests__/e2e/fixtures/quests/ftb/quests/chapters/rpg_adventure.snbt (1)
1-190: LGTM! Well-structured FTB quest fixture.The quest fixture provides good test data with:
- Proper SNBT format
- Unique quest and task IDs
- Clear progression through dependencies
- Variety of task types (item, advancement, stat, kill)
- Different reward types (items, XP, loot tables)
- Special formatting for player head textures
src/lib/services/__tests__/betterquest-service.bun.test.ts (2)
1-18: LGTM! Clean test setup with proper cleanup.The test setup correctly uses the test override mechanism and ensures proper cleanup after each test.
115-178: LGTM! Thorough .lang file format tests.The tests properly verify:
- Comment lines are ignored during parsing
- Key-value pairs are correctly extracted
- Output is formatted with sorted keys
- The parsing and formatting logic matches what's implemented in the frontend
src/__tests__/adapters/openai-adapter.test.ts (4)
41-66: Well-structured constructor testsThe constructor tests properly cover both default and custom configuration scenarios, correctly verifying the OpenAI client initialization parameters.
68-400: Comprehensive translation method test coverageExcellent test coverage for the translate method including:
- Success scenarios with proper response parsing
- Custom prompt template handling
- Rate limiting and retry logic
- Error scenarios and edge cases
- Response format variations
- Key validation
The tests are well-structured and thoroughly verify the adapter's behavior.
402-456: Good coverage of cache behavior loggingThe test properly verifies cache information logging including system fingerprint, cached tokens, and cache hit ratio calculation.
458-496: Thorough error handling edge case coverageGood coverage of authentication and model errors, correctly verifying that auth errors don't trigger retries.
src/components/ui/log-dialog.tsx (2)
10-10: Good practice: Centralized UI constantsExcellent refactoring to use centralized UI_DEFAULTS constants for timeouts and dimensions.
Also applies to: 290-290, 319-319, 339-339
378-396: Well-implemented Clear Logs functionalityGood implementation with proper error handling and user feedback. The button placement and functionality are appropriate.
src/lib/types/minecraft.ts (2)
5-9: Good type safety improvement with TranslationTargetTypeCentralizing the translation target types as a type alias improves type safety and maintainability.
26-28: Consistent format property naming and type improvementsThe changes improve consistency:
- Added
langFormatto track JSON vs legacy .lang formats- Renamed
QuestFile.typetoformatfor clarity- Added optional format fields to
TranslationTargetfor type-specific formats- Updated to use
TranslationTargetTypeconsistentlyThese changes enhance type safety and clarity throughout the codebase.
Also applies to: 62-68, 116-120, 144-144
src/components/settings/backup-settings.tsx (1)
24-37: Well-structured backup settings componentGood implementation with:
- Proper async data loading on mount
- User-friendly storage size formatting
- Comprehensive error handling
- Clear UI with proper labels and descriptions
- Accessibility considerations (labels, IDs)
Also applies to: 87-102, 112-246
src/components/tabs/custom-files-tab.tsx (3)
6-9: Good refactoring to job-based translation systemThe migration to use
runTranslationJobsand the job-based approach improves consistency with other translation tabs.Also applies to: 77-84
85-250: Well-implemented job-based translation with proper error handlingExcellent refactoring with:
- Alphabetical sorting for consistent processing
- Proper JSON flattening and reconstruction
- Comprehensive error handling at each step
- Immediate failure reporting for unsupported files
- Detailed logging via Tauri invoke
252-296: Well-implemented JSON flattening and reconstruction helpersThe helper functions properly handle complex JSON structures including nested objects and arrays, enabling effective translation of structured data.
src/__tests__/e2e/realistic-content-e2e.test.ts (5)
1-39: LGTM! Well-structured test setup.The test setup correctly mocks the Tauri window environment and FileService for E2E testing. The cleanup in
beforeAlland restoration inafterAllfollow best practices.
40-136: Excellent test coverage for RPG mod translation.The test comprehensively validates:
- Japanese translation patterns for different content types (tooltips, classes, items)
- Preservation of Minecraft color codes (§) and placeholders (%d, %s)
- Correct chunk processing and job completion
The custom
RPGTranslatormock is well-designed for testing realistic translation scenarios.
138-174: Good structural validation for tech mod content.The test effectively validates the expected structure of tech mod language files by checking category counts and energy-related formatting patterns. This ensures the test fixtures are realistic and comprehensive.
176-224: Well-implemented SNBT content extraction.The test correctly parses FTB quest files using appropriate regex patterns to extract translatable content. The multiline regex with
/gsflags properly handles nested quest blocks.
226-303: Comprehensive edge case coverage with documented limitation.The test thoroughly covers formatting edge cases including complex placeholders, nested colors, and Unicode. The comment about empty strings causing timeout issues in TranslationService is valuable documentation of a known limitation.
src/lib/utils/token-counter.ts (3)
1-77: Well-documented token estimation utility.The interfaces and core
estimateTokensfunction are well-structured with clear documentation. The token estimation approach using word count with configurable ratios and overhead is appropriate for LLM token approximation.
84-91: Simple but effective word counting implementation.The function handles edge cases well and provides a reasonable approximation for token estimation purposes.
144-175: Good provider-specific token estimation adjustments.The conservative token ratios and increased overhead estimates for different providers help prevent token limit errors. The configuration values appear reasonable based on typical LLM behavior.
src/__tests__/e2e/translation-e2e.test.ts (2)
1-81: Well-structured E2E test setup with comprehensive mocking.The test setup properly mocks file operations and provides a predictable translation adapter for testing. The FileService override handles all necessary commands for the test scenarios.
82-188: Thorough mod translation tests with good coverage.Both simple and complex mod tests properly validate:
- Translation job lifecycle (creation, execution, completion)
- Content preservation (placeholders, color codes)
- Output file generation
The tests effectively ensure the translation pipeline works end-to-end.
src/__tests__/e2e/realistic-progress-e2e.test.ts (2)
1-50: Good test setup for progress tracking scenarios.The configuration uses realistic chunk sizes and properly mocks file operations for testing progress tracking behavior.
174-237: Excellent test for multi-mod progress aggregation.The test properly validates:
- Correct chunk calculation for each mod
- Accurate overall progress percentage
- Proper rounding of progress values for UI display
This ensures the progress tracking works correctly across multiple translation targets.
src/__tests__/e2e/translation-e2e-simple.test.ts (1)
77-273: Well-designed simple E2E tests with comprehensive coverage.The test suite effectively validates:
- Basic mod translation with file output verification
- FTB quest SNBT parsing and translation
- Preservation of all formatting elements (color codes, placeholders)
The tests provide good coverage of core translation functionality with clear, focused test cases.
src/lib/services/config-service.ts (1)
181-191: Good improvement to ensure configuration consistency after save.The changes to clear the loaded flag and immediately reload the configuration ensure that any components using
getConfig()will receive the updated values. Re-throwing the error is also appropriate to let callers handle save failures.src/lib/services/backup-service.ts (1)
1-277: Well-implemented backup service with comprehensive functionality.The backup service is well-structured with:
- Clear separation of concerns and interfaces
- Consistent error handling and Tauri environment checks
- Comprehensive backup management operations (create, list, restore, delete, prune)
- Good type safety with proper TypeScript interfaces
The singleton pattern at the end ensures a single instance is used throughout the application.
src/lib/constants/defaults.ts (1)
1-179: Excellent centralization of application defaults.This file provides a well-organized single source of truth for all default values used throughout the application. The structure is clear with:
- Logical grouping of related constants
- Comprehensive coverage of all configuration areas
- Type-safe constant definitions using
as const- Clear and consistent naming conventions
src-tauri/src/logging.rs (1)
356-474: Well-structured logging commands for translation processes.The new logging commands provide comprehensive tracking of translation sessions with:
- Session-based organization
- Detailed progress tracking
- Performance metrics
- Proper error handling
The implementation follows good practices and integrates seamlessly with the existing logging infrastructure.
src/lib/services/translation-runner.ts (3)
252-253: Consider edge case for empty source content.The validation marks translations with empty content as failures, but this might be incorrect if the source content was also empty.
Consider checking if the source content was empty before marking the translation as invalid. This would prevent false negatives for legitimately empty translations.
84-84: Confirmed:maxFailedChunksRatiois defined inTRANSLATION_DEFAULTSI’ve verified that
maxFailedChunksRatiois declared insrc/lib/constants/defaults.ts(value:0.1), so the usage at line 84 is valid. No changes required.
216-216: Add fallback for empty source name.The source name determination could result in an empty string if all values are falsy, which might cause issues with backup naming.
Apply this diff to ensure a valid source name:
-const sourceName = job.currentFileName || job.id || `${type}_${i + 1}`; +const sourceName = job.currentFileName || job.id || `${type}_${i + 1}` || 'unknown';Likely an incorrect or invalid review comment.
src/lib/services/translation-service.ts (7)
4-6: LGTM! Well-structured imports for new functionality.The new imports properly support token-based chunking, centralized error logging, and configuration defaults.
30-53: Well-designed interface enhancements for session tracking and token-based chunking.The new fields properly support session management, file progress tracking, and token-based chunking configuration. All fields are appropriately marked as optional and well-documented.
Also applies to: 76-81
188-233: Excellent error handling improvements in logging methods.The integration with
ErrorLoggercentralizes error tracking while maintaining robust fallback behavior. The enhancedlogErrormethod with optionalprocessTypemaintains backward compatibility.
235-387: Comprehensive logging methods with excellent parameter validation.The new logging methods provide detailed session tracking with proper parameter validation and error handling. The camelCase conversion for Tauri v2 compatibility is well-documented.
401-408: Excellent session management and error detection enhancements.The session ID initialization, API call tracking, and empty translation detection significantly improve debugging capabilities. The early detection of API key issues prevents wasted processing time.
Note: Ensure
job.totalFilesis properly set before calling this method, as it defaults to 1 if undefined.Also applies to: 415-436, 470-495, 527-546
1015-1071: Well-implemented content type detection with comprehensive pattern matching.The method effectively analyzes both file names and content structure to determine appropriate content types for logging purposes.
693-774: Consider adding runtime token usage monitoringThe token-based chunking is well-implemented with solid edge-case handling and provider-specific configurations defined in
defaults.ts,token-counter.ts, andtranslation-service.ts. However, we don’t currently track actual token usage returned by the provider APIs to validate these estimates.• Add logging or metrics to capture
estimateTokens(...)outputs alongside the provider’s real usage data
• Compare contentTokens, promptOverhead, and totalTokens against API-reported token usage
• Use these insights to fine-tunewordToTokenRatio,systemPromptOverhead, anduserPromptOverheadin productionApplies equally to the logic in lines 806–873.
| // Auto-scroll effect | ||
| useEffect(() => { | ||
| if (isAutoScrollActive) { | ||
| scrollToBottom(); | ||
| } | ||
| }, [...dependencies, isAutoScrollActive, scrollToBottom]); | ||
| }, [dependencies, isAutoScrollActive, scrollToBottom]); |
There was a problem hiding this comment.
Critical: Restore dependency array spreading for correct reactivity.
The change from spreading dependencies to including it as a single element breaks the expected behavior. When dependencies are passed as [dependencies], the effect only re-runs when the array reference changes, not when its contents change.
Apply this diff to restore the correct behavior:
// Auto-scroll effect
useEffect(() => {
if (isAutoScrollActive) {
scrollToBottom();
}
- }, [dependencies, isAutoScrollActive, scrollToBottom]);
+ }, [...dependencies, isAutoScrollActive, scrollToBottom]);🤖 Prompt for AI Agents
In src/hooks/use-auto-scroll.ts around lines 145 to 150, the useEffect
dependency array incorrectly includes the entire dependencies array as a single
element, causing the effect to not re-run on internal changes. Fix this by
spreading the dependencies array inside the dependency array, i.e., use
...dependencies instead of dependencies, so the effect properly reacts to
changes in any dependency item.
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_PRIVATE_KEY }} # Repository Secret | ||
| TAURI_SIGNING_PRIVATE_KEY_PASSWORD: ${{ secrets.TAURI_KEY_PASSWORD }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Secrets exposed to entire job
Setting GITHUB_TOKEN and signing keys at the job level exposes them to every subsequent step. Restrict to the step that actually needs them (the Tauri build) to minimise leakage surface:
- env:
- GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
- TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_PRIVATE_KEY }}
- TAURI_SIGNING_PRIVATE_KEY_PASSWORD: ${{ secrets.TAURI_KEY_PASSWORD }}
...
- - name: Build Tauri app
- uses: tauri-apps/tauri-action@v0
+ - name: Build Tauri app
+ uses: tauri-apps/tauri-action@v0
+ env:
+ GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+ TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_PRIVATE_KEY }}
+ TAURI_SIGNING_PRIVATE_KEY_PASSWORD: ${{ secrets.TAURI_KEY_PASSWORD }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_PRIVATE_KEY }} # Repository Secret | |
| TAURI_SIGNING_PRIVATE_KEY_PASSWORD: ${{ secrets.TAURI_KEY_PASSWORD }} | |
| # .github/workflows/build.yml | |
| jobs: | |
| build: | |
| runs-on: ubuntu-latest | |
| - env: | |
| - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| - TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_PRIVATE_KEY }} | |
| - TAURI_SIGNING_PRIVATE_KEY_PASSWORD: ${{ secrets.TAURI_KEY_PASSWORD }} | |
| steps: | |
| - uses: actions/checkout@v3 | |
| # … other steps … | |
| - name: Build Tauri app | |
| uses: tauri-apps/tauri-action@v0 | |
| + env: | |
| + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| + TAURI_SIGNING_PRIVATE_KEY: ${{ secrets.TAURI_PRIVATE_KEY }} | |
| + TAURI_SIGNING_PRIVATE_KEY_PASSWORD: ${{ secrets.TAURI_KEY_PASSWORD }} | |
| # … remaining steps … |
🤖 Prompt for AI Agents
In .github/workflows/build.yml around lines 42 to 45, the environment variables
for GITHUB_TOKEN and Tauri signing keys are set at the job level, exposing these
secrets to all steps. Move these environment variable declarations from the job
level to only the specific step that performs the Tauri build to restrict secret
exposure and minimize the risk of leakage.
| { | ||
| ignores: [ | ||
| "**/*.test.ts", | ||
| "**/*.test.tsx", | ||
| "**/*.bun.test.ts", | ||
| "**/*.jest.test.ts" | ||
| ] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider targeted ESLint rules instead of complete exclusion.
Excluding all test files from ESLint linting means missing potential code quality issues, consistency problems, and bugs in test logic.
Consider using ESLint overrides for test files instead:
- {
- ignores: [
- "**/*.test.ts",
- "**/*.test.tsx",
- "**/*.bun.test.ts",
- "**/*.jest.test.ts"
- ]
- }
+ {
+ files: ["**/*.test.ts", "**/*.test.tsx", "**/*.bun.test.ts", "**/*.jest.test.ts"],
+ rules: {
+ // Relax specific rules for test files if needed
+ "@typescript-eslint/no-explicit-any": "off",
+ "prefer-const": "off"
+ }
+ }This approach maintains code quality while allowing test-specific patterns.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In eslint.config.mjs around lines 14 to 21, instead of completely ignoring all
test files, replace the ignores array with an overrides section targeting test
files. Configure specific ESLint rules or rule adjustments within these
overrides to allow test-specific patterns while still enforcing code quality and
consistency in test files.
| style={ | ||
| { | ||
| "--normal-bg": "var(--popover)", | ||
| "--normal-text": "var(--popover-foreground)", | ||
| "--normal-border": "var(--border)", | ||
| } as React.CSSProperties | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify CSS custom property names for Sonner compatibility.
The CSS custom properties --normal-bg, --normal-text, and --normal-border may not be the correct variable names that Sonner expects. Please verify these align with Sonner's theming system.
Alternatively, consider using Sonner's standard theming approach or verify the custom property names in the Sonner documentation.
🏁 Script executed:
#!/bin/bash
# Check Sonner documentation for correct CSS custom property names
rg -A 10 -B 5 "normal-bg|normal-text|normal-border" Length of output: 866
🌐 Web query:
What CSS custom properties are exposed by the Sonner library for theming the Toaster component?
💡 Result:
The Sonner library provides several CSS custom properties (also known as CSS variables) to facilitate theming and styling of the Toaster component. These variables allow you to customize various aspects of the toast notifications globally. You can define these variables in your global CSS file to apply consistent styling across all toasts.
Here are the available CSS custom properties:
--sonner-toast-background: Sets the background color of the toast.--sonner-toast-color: Sets the text color of the toast.--sonner-toast-border: Sets the border color of the toast.--sonner-toast-shadow: Sets the box-shadow of the toast.--sonner-toast-radius: Sets the border-radius of the toast.--sonner-toast-padding: Sets the padding inside the toast.--sonner-toast-font-size: Sets the font size of the toast text.--sonner-toast-font-weight: Sets the font weight of the toast text.--sonner-toast-line-height: Sets the line height of the toast text.--sonner-toast-letter-spacing: Sets the letter spacing of the toast text.--sonner-toast-text-transform: Sets the text transformation of the toast text.--sonner-toast-font-family: Sets the font family of the toast text.--sonner-toast-icon-size: Sets the size of the icon in the toast.--sonner-toast-icon-margin-right: Sets the margin between the icon and the text.--sonner-toast-action-padding: Sets the padding of the action button.--sonner-toast-action-font-size: Sets the font size of the action button text.--sonner-toast-action-font-weight: Sets the font weight of the action button text.--sonner-toast-action-line-height: Sets the line height of the action button text.--sonner-toast-action-letter-spacing: Sets the letter spacing of the action button text.--sonner-toast-action-text-transform: Sets the text transformation of the action button text.--sonner-toast-action-font-family: Sets the font family of the action button text.--sonner-toast-action-color: Sets the text color of the action button.--sonner-toast-action-background: Sets the background color of the action button.--sonner-toast-action-border: Sets the border color of the action button.--sonner-toast-action-border-radius: Sets the border-radius of the action button.--sonner-toast-action-box-shadow: Sets the box-shadow of the action button.--sonner-toast-action-hover-color: Sets the text color of the action button on hover.--sonner-toast-action-hover-background: Sets the background color of the action button on hover.--sonner-toast-action-hover-border: Sets the border color of the action button on hover.--sonner-toast-action-hover-box-shadow: Sets the box-shadow of the action button on hover.--sonner-toast-action-focus-color: Sets the text color of the action button on focus.--sonner-toast-action-focus-background: Sets the background color of the action button on focus.--sonner-toast-action-focus-border: Sets the border color of the action button on focus.--sonner-toast-action-focus-box-shadow: Sets the box-shadow of the action button on focus.--sonner-toast-action-disabled-color: Sets the text color of the action button when disabled.--sonner-toast-action-disabled-background: Sets the background color of the action button when disabled.--sonner-toast-action-disabled-border: Sets the border color of the action button when disabled.--sonner-toast-action-disabled-box-shadow: Sets the box-shadow of the action button when disabled.--sonner-toast-close-button-size: Sets the size of the close button.--sonner-toast-close-button-color: Sets the color of the close button.--sonner-toast-close-button-background: Sets the background color of the close button.--sonner-toast-close-button-border: Sets the border color of the close button.--sonner-toast-close-button-border-radius: Sets the border-radius of the close button.--sonner-toast-close-button-box-shadow: Sets the box-shadow of the close button.--sonner-toast-close-button-hover-color: Sets the color of the close button on hover.--sonner-toast-close-button-hover-background: Sets the background color of the close button on hover.--sonner-toast-close-button-hover-border: Sets the border color of the close button on hover.--sonner-toast-close-button-hover-box-shadow: Sets the box-shadow of the close button on hover.--sonner-toast-close-button-focus-color: Sets the color of the close button on focus.--sonner-toast-close-button-focus-background: Sets the background color of the close button on focus.--sonner-toast-close-button-focus-border: Sets the border color of the close button on focus.--sonner-toast-close-button-focus-box-shadow: Sets the box-shadow of the close button on focus.--sonner-toast-close-button-disabled-color: Sets the color of the close button when disabled.--sonner-toast-close-button-disabled-background: Sets the background color of the close button when disabled.--sonner-toast-close-button-disabled-border: Sets the border color of the close button when disabled.--sonner-toast-close-button-disabled-box-shadow: Sets the box-shadow of the close button when disabled.
By defining these variables in your global CSS, you can customize the appearance of all toast notifications rendered by the Toaster component. For example, to change the background color of all toasts to red, you can add the following to your CSS:
:root {
--sonner-toast-background: red;
}This approach provides a centralized way to manage the styling of your toast notifications, ensuring consistency across your application.
For more detailed information and additional customization options, you can refer to the official Sonner documentation. (sonner.emilkowal.ski)
Citations:
Use correct Sonner CSS custom properties for theming.
The current style block is using --normal-bg, --normal-text, and --normal-border, but Sonner’s Toaster expects variables prefixed with --sonner-toast-. Please update the style prop in src/components/ui/sonner.tsx accordingly:
style={
{
- "--normal-bg": "var(--popover)",
- "--normal-text": "var(--popover-foreground)",
- "--normal-border": "var(--border)",
+ "--sonner-toast-background": "var(--popover)",
+ "--sonner-toast-color": "var(--popover-foreground)",
+ "--sonner-toast-border": "var(--border)",
} as React.CSSProperties
}This ensures your theme values correctly override Sonner’s defaults.
🤖 Prompt for AI Agents
In src/components/ui/sonner.tsx around lines 13 to 19, the style prop uses CSS
variables with the prefix --normal- which Sonner's Toaster does not recognize.
Update the style object to use the correct CSS custom properties prefixed with
--sonner-toast- (e.g., --sonner-toast-bg, --sonner-toast-text,
--sonner-toast-border) so that the theme overrides apply properly.
| max_tokens: MODEL_TOKEN_LIMITS.anthropic.maxOutputTokens, | ||
| temperature: this.config.temperature ?? DEFAULT_API_CONFIG.temperature, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify token limit values are appropriate.
The use of MODEL_TOKEN_LIMITS.anthropic.maxOutputTokens for max_tokens and temperature from config are good improvements. However, verify that 4096 tokens is appropriate for Claude models.
🌐 Web query:
What are the current maximum output token limits for Claude models like claude-3-5-haiku-20241022?
💡 Result:
The Claude 3.5 Haiku model, identified as claude-3-5-haiku-20241022, has the following token limits:
- Maximum Input Tokens (Context Window): 200,000 tokens
- Maximum Output Tokens: 8,000 tokens
This configuration allows the model to process extensive input data while generating substantial outputs in a single request. (cloud.google.com)
Citations:
Increase Claude max_output_tokens to 8 000
Based on the latest Claude 3.5 Haiku limits (max output tokens = 8 000)[1], please update your token‐limit configuration:
• src/lib/adapters/anthropic-adapter.ts (around lines 108–109)
- Ensure
MODEL_TOKEN_LIMITS.anthropic.maxOutputTokensis set to 8000, not 4096. - If you have a central
MODEL_TOKEN_LIMITSdefinition, update theanthropic.maxOutputTokensvalue there.
Diff example (constants file):
anthropic: {
- maxInputTokens: 200000,
- maxOutputTokens: 4096,
+ maxInputTokens: 200000,
+ maxOutputTokens: 8000,
},Citations:
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/lib/adapters/anthropic-adapter.ts around lines 108 to 109, the
maxOutputTokens for Claude is set to 4096 but should be updated to 8000 to match
the latest Claude 3.5 Haiku limits. Locate the central MODEL_TOKEN_LIMITS
definition (likely in a constants or config file) and change
anthropic.maxOutputTokens from 4096 to 8000. This will ensure the adapter uses
the correct token limit.
| export const DEFAULT_MODELS = { | ||
| openai: "o4-mini-2025-04-16", | ||
| anthropic: "claude-3-5-haiku-20241022", | ||
| google: "gemini-1.5-flash", | ||
| } as const; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the OpenAI model version with future date.
The default OpenAI model is specified as "o4-mini-2025-04-16" which appears to reference April 2025. This seems incorrect as it's a future date.
Please verify if this is the correct model name. You may want to check the latest OpenAI documentation for current model names.
🌐 Web query:
What is the latest OpenAI model name for GPT-4 mini as of 2025?
💡 Result:
As of July 2025, the latest OpenAI model serving as the "GPT-4 mini" is called GPT-4.1 mini. This model was introduced in May 2025 and replaces the earlier GPT-4o mini in ChatGPT and API offerings. GPT-4.1 mini is described as faster, more capable, and more efficient than its predecessors, with significant improvements in instruction-following, coding, and overall intelligence. In product interfaces and APIs, it is surfaced directly as "GPT-4.1 mini" for users seeking a smaller, high-throughput GPT-4 class model[1][7][10].
Model lineage and relevant names:
- GPT-4o mini was the previous "mini" version, released in July 2024[3][5].
- GPT-4.1 mini is the current version as of mid-2025, available in the OpenAI API and ChatGPT for both free (as fallback) and paid users[1][7].
Key details:
- GPT-4.1 mini is listed in the model picker as "GPT-4.1 mini" for paid users and is the fallback model for free users after GPT-4o usage limits are reached[1].
- It delivers efficiency and performance suited for high-volume, cost-sensitive scenarios.
- o4-mini and o3-mini are separate OpenAI models targeting high-throughput reasoning, but GPT-4.1 mini is the one aligned with the "GPT-4 mini" designation most directly as of July 2025[1][10].
Summary Table:
| Previous Model | Replaced By / Current Model (as of July 2025) |
|---|---|
| GPT-4o mini | GPT-4.1 mini |
You should look for GPT-4.1 mini as the latest and most capable "mini" version of GPT-4 in OpenAI's offerings as of July 2025[1][7].
Citations:
- 1: https://help.openai.com/en/articles/9624314-model-release-notes
- 2: https://help.openai.com/en/articles/6825453-chatgpt-release-notes
- 3: https://en.wikipedia.org/wiki/GPT-4o
- 4: https://community.openai.com/t/has-gpt-4o-mini-been-updated-to-the-march-2025-version/1163480
- 5: https://learn.microsoft.com/en-us/azure/ai-foundry/openai/whats-new
- 6: https://zapier.com/blog/openai-models/
- 7: https://openai.com/index/gpt-4-1/
- 8: https://platform.openai.com/docs/deprecations
- 9: https://platform.openai.com/docs/models/gpt-4o
- 10: https://openai.com/index/introducing-o3-and-o4-mini/
Update default OpenAI model to GPT-4.1 mini
The hard-coded "o4-mini-2025-04-16" no longer exists: as of May 2025 the correct “mini” variant is GPT-4.1 mini, surfaced in both the API and ChatGPT interfaces.
Please update in src/lib/constants/defaults.ts (around lines 9–13):
export const DEFAULT_MODELS = {
- openai: "o4-mini-2025-04-16",
+ openai: "gpt-4.1-mini",
anthropic: "claude-3-5-haiku-20241022",
google: "gemini-1.5-flash",
} as const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const DEFAULT_MODELS = { | |
| openai: "o4-mini-2025-04-16", | |
| anthropic: "claude-3-5-haiku-20241022", | |
| google: "gemini-1.5-flash", | |
| } as const; | |
| export const DEFAULT_MODELS = { | |
| openai: "gpt-4.1-mini", | |
| anthropic: "claude-3-5-haiku-20241022", | |
| google: "gemini-1.5-flash", | |
| } as const; |
🤖 Prompt for AI Agents
In src/lib/constants/defaults.ts around lines 9 to 13, update the default OpenAI
model string from "o4-mini-2025-04-16" to the correct current model name
"gpt-4.1-mini" to reflect the new GPT-4.1 mini variant as of May 2025.
| expect(outputData['complexmod.tooltip.shift_info']).toContain('§r'); | ||
| }); | ||
|
|
||
| test.skip('should handle token-based chunking for large files', async () => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enable skipped tests for complete coverage.
The skipped tests cover critical functionality (token-based chunking and error handling) that should be tested to ensure robustness.
Consider enabling these tests or documenting why they are skipped:
- Line 211: Token-based chunking test
- Line 267: Translation failure handling test
Also applies to: 267-267
🤖 Prompt for AI Agents
In src/__tests__/e2e/realistic-translation-e2e.test.ts at lines 211 and 267,
there are tests currently skipped that cover important functionality like
token-based chunking and error handling. To ensure complete test coverage and
robustness, remove the .skip from these test declarations to enable them. If
there is a specific reason for skipping, add clear documentation explaining why
the tests remain skipped.
| }; | ||
| } | ||
|
|
||
| getMaxChunkSize() { return 50; } |
There was a problem hiding this comment.
Fix method name to match expected interface.
The method name should be getMaxTokensPerChunk() to match the interface expected by TranslationService.
Apply this diff:
-getMaxChunkSize() { return 50; }
+getMaxTokensPerChunk() { return 50; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getMaxChunkSize() { return 50; } | |
| getMaxTokensPerChunk() { return 50; } |
🤖 Prompt for AI Agents
In src/__tests__/e2e/realistic-translation-e2e.test.ts at line 380, rename the
method getMaxChunkSize() to getMaxTokensPerChunk() to match the expected
interface of TranslationService. This ensures the method signature aligns
correctly with the interface requirements.
| const filePath = args?.path as string; | ||
| await fs.mkdir(path.dirname(filePath), { recursive: true }); | ||
| await fs.writeFile(filePath, args?.content as string); | ||
| return true; | ||
|
|
There was a problem hiding this comment.
Fix variable declaration scope in switch statement.
The filePath variable declaration is accessible to other switch cases, which could lead to unexpected behavior.
Apply this diff to restrict the variable scope:
case 'write_text_file':
+ {
const filePath = args?.path as string;
await fs.mkdir(path.dirname(filePath), { recursive: true });
await fs.writeFile(filePath, args?.content as string);
return true;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const filePath = args?.path as string; | |
| await fs.mkdir(path.dirname(filePath), { recursive: true }); | |
| await fs.writeFile(filePath, args?.content as string); | |
| return true; | |
| case 'write_text_file': { | |
| const filePath = args?.path as string; | |
| await fs.mkdir(path.dirname(filePath), { recursive: true }); | |
| await fs.writeFile(filePath, args?.content as string); | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In src/__tests__/e2e/realistic-translation-e2e.test.ts around lines 115 to 119,
the variable filePath is declared outside the switch case block, making it
accessible across other cases and potentially causing unexpected behavior. To
fix this, move the declaration of filePath inside the specific switch case block
where it is used, ensuring its scope is limited to that case only.
| backup: { | ||
| enabled: true, | ||
| retentionDays: 30, | ||
| maxBackupsPerType: 10, | ||
| autoPruneOnStartup: false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use constants for backup configuration defaults.
The backup configuration values are hardcoded while other configurations use imported constants. This inconsistency could lead to maintenance issues.
Consider adding backup defaults to the imported constants:
backup: {
- enabled: true,
- retentionDays: 30,
- maxBackupsPerType: 10,
- autoPruneOnStartup: false
+ enabled: BACKUP_DEFAULTS.enabled,
+ retentionDays: BACKUP_DEFAULTS.retentionDays,
+ maxBackupsPerType: BACKUP_DEFAULTS.maxBackupsPerType,
+ autoPruneOnStartup: BACKUP_DEFAULTS.autoPruneOnStartup
}This requires adding BACKUP_DEFAULTS to the constants module.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| backup: { | |
| enabled: true, | |
| retentionDays: 30, | |
| maxBackupsPerType: 10, | |
| autoPruneOnStartup: false | |
| } | |
| backup: { | |
| enabled: BACKUP_DEFAULTS.enabled, | |
| retentionDays: BACKUP_DEFAULTS.retentionDays, | |
| maxBackupsPerType: BACKUP_DEFAULTS.maxBackupsPerType, | |
| autoPruneOnStartup: BACKUP_DEFAULTS.autoPruneOnStartup | |
| } |
🤖 Prompt for AI Agents
In src/lib/types/config.ts around lines 177 to 182, the backup configuration
uses hardcoded values instead of constants like other configurations. To fix
this, define a BACKUP_DEFAULTS constant in the constants module with the default
backup settings, then replace the hardcoded values in the backup config with
references to these constants to ensure consistency and easier maintenance.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/tabs/quests-tab.tsx (1)
54-55: This is the same cross-platform path issue flagged previously.The path manipulation using string splitting with forward slashes is still present and will not work correctly on Windows systems.
Also applies to: 58-60, 82-83, 86-88
🧹 Nitpick comments (1)
src/components/tabs/quests-tab.tsx (1)
152-171: Consider extracting lang file parsing to a utility function.The
.langfile parsing logic is well-contained and could be reused elsewhere in the application. Consider moving it to a utility module for better reusability.// In a utils/lang-parser.ts file export function parseLangFile(content: string): Record<string, string> { const langMap: Record<string, string> = {}; const lines = content.split('\n'); for (const line of lines) { const trimmed = line.trim(); if (trimmed && !trimmed.startsWith('#')) { const separatorIndex = trimmed.indexOf('='); if (separatorIndex > -1) { const key = trimmed.substring(0, separatorIndex).trim(); const value = trimmed.substring(separatorIndex + 1).trim(); langMap[key] = value; } } } return langMap; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build.yml(1 hunks)src/components/tabs/quests-tab.tsx(1 hunks)src/lib/services/translation-runner.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build.yml
- src/lib/services/translation-runner.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/tabs/quests-tab.tsx (6)
src/lib/store/index.ts (1)
useAppStore(78-296)src/lib/services/file-service.ts (2)
FileService(188-382)invoke(202-209)src/lib/types/minecraft.ts (2)
TranslationTarget(103-120)TranslationResult(142-157)src/lib/services/translation-service.ts (2)
TranslationService(88-1072)TranslationJob(27-54)src/lib/services/translation-runner.ts (1)
runTranslationJobs(28-132)src/components/tabs/common/translation-tab.tsx (1)
TranslationTab(95-593)
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Security Scan
🔇 Additional comments (3)
src/components/tabs/quests-tab.tsx (3)
273-296: Good error handling for logging operations.The use of empty catch blocks for logging operations is appropriate here since logging failures shouldn't interrupt the translation process. This follows the fail-safe pattern correctly.
304-312: Clean helper function with good separation of concerns.The
renderQuestTypefunction properly encapsulates the quest type rendering logic and handles the different quest formats appropriately.
252-265: Regex handles all current language codes
The app’s supported languages (in src/lib/constants/defaults.ts) are:
• ja_jp, zh_cn, ko_kr, de_de, fr_fr, es_es, it_it, pt_br, ru_ru
All follow the two-letter underscore two-letter pattern, so/\.[a-z]{2}_[a-z]{2}\.(snbt|json|lang)$/correctly matches and strips existing suffixes before appending the new one. No changes needed.
| // Translate quests | ||
| const handleTranslate = async ( | ||
| selectedTargets: TranslationTarget[], | ||
| targetLanguage: string, | ||
| translationService: TranslationService, | ||
| setCurrentJobId: (jobId: string | null) => void, | ||
| addTranslationResult: (result: TranslationResult) => void, | ||
| selectedDirectory: string | ||
| ) => { | ||
| try { | ||
| setTranslating(true); | ||
|
|
||
| // Sort targets alphabetically for consistent processing | ||
| const sortedTargets = [...selectedTargets].sort((a, b) => a.name.localeCompare(b.name)); | ||
|
|
||
| // Reset progress tracking | ||
| setCompletedChunks(0); | ||
| setWholeProgress(0); | ||
| setProgress(0); | ||
|
|
||
| // Set total quests for progress tracking | ||
| const totalQuests = sortedTargets.length; | ||
| setTotalChunks(totalQuests); // For quests, we track at file level instead of chunk level | ||
|
|
||
| // Create jobs for all quests | ||
| const jobs: Array<{ | ||
| target: TranslationTarget; | ||
| job: TranslationJob; | ||
| content: string; | ||
| }> = []; | ||
|
|
||
| for (const target of sortedTargets) { | ||
| try { | ||
| // Read quest file | ||
| const content = await FileService.readTextFile(target.path); | ||
|
|
||
| let processedContent = content; | ||
|
|
||
| // If it's a .lang file, convert to JSON format for translation | ||
| if (target.path.endsWith('.lang')) { | ||
| const langMap: Record<string, string> = {}; | ||
| const lines = content.split('\n'); | ||
|
|
||
| for (const line of lines) { | ||
| const trimmed = line.trim(); | ||
| // Skip empty lines and comments | ||
| if (trimmed && !trimmed.startsWith('#')) { | ||
| const separatorIndex = trimmed.indexOf('='); | ||
| if (separatorIndex > -1) { | ||
| const key = trimmed.substring(0, separatorIndex).trim(); | ||
| const value = trimmed.substring(separatorIndex + 1).trim(); | ||
| langMap[key] = value; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Convert to JSON string for translation | ||
| processedContent = JSON.stringify(langMap, null, 2); | ||
| } | ||
|
|
||
| // Create a translation job | ||
| const job = translationService.createJob( | ||
| {content: processedContent}, | ||
| targetLanguage, | ||
| target.name | ||
| ); | ||
|
|
||
| jobs.push({ target, job, content: processedContent }); | ||
| } catch (error) { | ||
| console.error(`Failed to prepare quest: ${target.name}`, error); | ||
| // Add failed result immediately | ||
| addTranslationResult({ | ||
| type: "quest", | ||
| id: target.id, | ||
| targetLanguage: targetLanguage, | ||
| content: {}, | ||
| outputPath: "", | ||
| success: false | ||
| }); | ||
| incrementCompletedChunks(); | ||
| } | ||
| } | ||
|
|
||
| // Use runTranslationJobs for consistent processing | ||
| await runTranslationJobs({ | ||
| jobs: jobs.map(({ job }) => job), | ||
| translationService, | ||
| setCurrentJobId, | ||
| incrementCompletedChunks, // Track at chunk level for real-time progress | ||
| incrementWholeProgress: incrementCompletedChunks, // Track at quest level | ||
| targetLanguage, | ||
| type: "quest", | ||
| getOutputPath: () => selectedDirectory, | ||
| getResultContent: (job) => translationService.getCombinedTranslatedContent(job.id), | ||
| writeOutput: async (job, outputPath, content) => { | ||
| // Find the corresponding quest data | ||
| const questData = jobs.find(j => j.job.id === job.id); | ||
| if (!questData) return; | ||
|
|
||
| let translatedText = content.content || `[${targetLanguage}] ${questData.content}`; | ||
|
|
||
| // Write translated file with language suffix | ||
| let fileExtension: string; | ||
| let outputFilePath: string; | ||
|
|
||
| if (questData.target.questFormat === "ftb") { | ||
| fileExtension = "snbt"; | ||
| } else { | ||
| // For BetterQuest, check if it's a .lang file | ||
| fileExtension = questData.target.path.endsWith('.lang') ? "lang" : "json"; | ||
| } | ||
|
|
||
| // If it's a .lang file, format the output properly | ||
| if (fileExtension === "lang") { | ||
| try { | ||
| // Parse the translated JSON content | ||
| const translatedMap = JSON.parse(translatedText); | ||
|
|
||
| // Convert to .lang format | ||
| const langLines: string[] = []; | ||
| const sortedKeys = Object.keys(translatedMap).sort(); | ||
|
|
||
| for (const key of sortedKeys) { | ||
| langLines.push(`${key}=${translatedMap[key]}`); | ||
| } | ||
|
|
||
| translatedText = langLines.join('\n'); | ||
| } catch (error) { | ||
| console.error('Failed to format .lang file output:', error); | ||
| // Keep original format if parsing fails | ||
| } | ||
| } | ||
|
|
||
| // Special handling for DefaultQuests.lang files | ||
| if (questData.target.path.endsWith('DefaultQuests.lang')) { | ||
| // For direct DefaultQuests.lang, create a copy with language suffix | ||
| outputFilePath = questData.target.path.replace('.lang', `.${targetLanguage}.lang`); | ||
| } else { | ||
| // Check if the file already has a language suffix and remove it | ||
| const languagePattern = /\.[a-z]{2}_[a-z]{2}\.(snbt|json|lang)$/; | ||
| let basePath = questData.target.path; | ||
|
|
||
| // Remove existing language suffix if present | ||
| if (languagePattern.test(basePath)) { | ||
| basePath = basePath.replace(languagePattern, `.${fileExtension}`); | ||
| } | ||
|
|
||
| // Now add the new language suffix | ||
| outputFilePath = basePath.replace( | ||
| `.${fileExtension}`, | ||
| `.${targetLanguage}.${fileExtension}` | ||
| ); | ||
| } | ||
|
|
||
| await FileService.writeTextFile(outputFilePath, translatedText); | ||
| }, | ||
| onResult: addTranslationResult, | ||
| onJobStart: async (job) => { | ||
| const questData = jobs.find(j => j.job.id === job.id); | ||
| if (!questData) return; | ||
| try { | ||
| await invoke('log_translation_process', { | ||
| message: `Starting translation for quest: ${questData.target.name} (${questData.target.id})` | ||
| }); | ||
| } catch {} | ||
| }, | ||
| onJobComplete: async (job) => { | ||
| const questData = jobs.find(j => j.job.id === job.id); | ||
| if (!questData) return; | ||
| try { | ||
| await invoke('log_translation_process', { | ||
| message: `Completed translation for quest: ${questData.target.name} (${questData.target.id})` | ||
| }); | ||
| } catch {} | ||
| }, | ||
| onJobInterrupted: async (job) => { | ||
| const questData = jobs.find(j => j.job.id === job.id); | ||
| if (!questData) return; | ||
| try { | ||
| await invoke('log_translation_process', { | ||
| message: `Translation cancelled by user during quest: ${questData.target.name} (${questData.target.id})` | ||
| }); | ||
| } catch {} | ||
| } | ||
| }); | ||
| } finally { | ||
| setTranslating(false); | ||
| } | ||
| ]} | ||
| config={config} | ||
| translationTargets={questTranslationTargets} | ||
| setTranslationTargets={setQuestTranslationTargets} | ||
| updateTranslationTarget={updateQuestTranslationTarget} | ||
| isTranslating={isTranslating} | ||
| progress={progress} | ||
| wholeProgress={wholeProgress} | ||
| setTranslating={setTranslating} | ||
| setProgress={setProgress} | ||
| setWholeProgress={setWholeProgress} | ||
| setTotalChunks={setTotalChunks} | ||
| setCompletedChunks={setCompletedChunks} | ||
| addTranslationResult={addTranslationResult} | ||
| error={error} | ||
| setError={setError} | ||
| currentJobId={currentJobId} | ||
| setCurrentJobId={setCurrentJobId} | ||
| isCompletionDialogOpen={isCompletionDialogOpen} | ||
| setCompletionDialogOpen={setCompletionDialogOpen} | ||
| setLogDialogOpen={setLogDialogOpen} | ||
| resetTranslationState={resetTranslationState} | ||
| onScan={handleScan} | ||
| onTranslate={handleTranslate} | ||
| /> | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down this large function for better maintainability.
The handleTranslate function is quite large (188 lines) and handles multiple responsibilities: file processing, job creation, translation execution, and output formatting. This violates the Single Responsibility Principle and makes it harder to test and maintain.
Consider extracting these concerns into separate functions:
+// Extract file processing logic
+const processQuestFile = async (target: TranslationTarget): Promise<string> => {
+ const content = await FileService.readTextFile(target.path);
+
+ if (target.path.endsWith('.lang')) {
+ const langMap: Record<string, string> = {};
+ const lines = content.split('\n');
+
+ for (const line of lines) {
+ const trimmed = line.trim();
+ if (trimmed && !trimmed.startsWith('#')) {
+ const separatorIndex = trimmed.indexOf('=');
+ if (separatorIndex > -1) {
+ const key = trimmed.substring(0, separatorIndex).trim();
+ const value = trimmed.substring(separatorIndex + 1).trim();
+ langMap[key] = value;
+ }
+ }
+ }
+
+ return JSON.stringify(langMap, null, 2);
+ }
+
+ return content;
+};
+// Extract output path generation logic
+const generateOutputPath = (target: TranslationTarget, targetLanguage: string): string => {
+ // ... output path logic
+};
+// Extract output formatting logic
+const formatTranslatedOutput = (translatedText: string, fileExtension: string): string => {
+ // ... formatting logic
+};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/tabs/quests-tab.tsx between lines 113 and 301, the
handleTranslate function is too large and handles multiple responsibilities
including file reading, job creation, translation execution, and output
formatting. To fix this, refactor by extracting distinct parts into smaller
helper functions such as one for reading and processing quest files, another for
creating translation jobs, a function for formatting .lang output, and one for
writing translated files. This will improve maintainability, readability, and
testability.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/lib/utils/error-logger.ts (1)
7-71: Refactor to address static analysis issues and improve design.The static analysis tools correctly identify design issues. Consider converting this static-only class to standalone functions for better code organization.
src/components/ui/backup-dialog.tsx (2)
249-249: Missing filter implementationThe
filteredBackupsvariable is assigned directly frombackupswithout applying the selected filter. The filter state is collected but never used to filter the results.
230-230: Use configurable retention period instead of hardcoded valueThe prune operation uses a hardcoded 30-day retention period. Consider using the retention configuration from the backup settings instead to allow users to control this value.
src-tauri/src/minecraft/mod.rs (1)
382-386: Extract repeated buffer cleaning logic into a helper function.The buffer cleaning logic to remove null bytes and control characters is repeated in multiple places throughout the file.
Add a helper function at the module level:
/// Clean raw bytes by removing null bytes and control characters fn clean_raw_bytes(buffer: Vec<u8>) -> Vec<u8> { buffer.into_iter() .filter(|&b| b != 0 && (b >= 0x20 || b == 0x09 || b == 0x0A || b == 0x0D)) .collect() }Then use it in all the places:
- // First, remove any null bytes and other problematic bytes - let cleaned_buffer: Vec<u8> = buffer.into_iter() - .filter(|&b| b != 0 && (b >= 0x20 || b == 0x09 || b == 0x0A || b == 0x0D)) - .collect(); + let cleaned_buffer = clean_raw_bytes(buffer);src-tauri/src/filesystem.rs (1)
148-160: Address the previously identified code duplication in language suffix filtering.The language suffix filtering logic is still duplicated across three functions as mentioned in the past review. This violates the DRY principle and makes maintenance harder when language codes need to be updated.
The previously suggested helper function should be implemented:
fn has_language_suffix(filename: &str) -> bool { const LANGUAGE_SUFFIXES: &[&str] = &[ ".ja_jp.", ".zh_cn.", ".ko_kr.", ".de_de.", ".fr_fr.", ".es_es.", ".it_it.", ".pt_br.", ".ru_ru." ]; LANGUAGE_SUFFIXES.iter().any(|suffix| filename.contains(suffix)) }Then replace all the duplicated checks with:
if has_language_suffix(file_name) { debug!("Skipping already translated file: {}", file_name); continue; }Also applies to: 222-234, 311-324
🧹 Nitpick comments (6)
src/components/tabs/custom-files-tab.tsx (2)
108-114: Consider edge case: Empty file list could cause division by zero.While the current implementation handles the empty case, consider adding an explicit guard to prevent potential issues if
sortedTargets.lengthis 0.// Set total files for progress tracking const totalFiles = sortedTargets.length; + if (totalFiles === 0) { + throw new Error("No files selected for translation"); + } setTotalChunks(totalFiles); // Track at file level
252-296: Review the JSON flattening and reconstruction logic.The flattening and reconstruction logic handles nested objects and arrays correctly. However, there's a potential issue with object mutation in the reconstruction function.
Consider making the reconstruction function immutable:
const reconstructJson = (originalJson: unknown, translatedContent: Record<string, string>): unknown => { const reconstruct = (obj: unknown, prefix = ''): unknown => { if (typeof obj === 'string') { return translatedContent[prefix] || obj; } else if (Array.isArray(obj)) { return obj.map((item, index) => reconstruct(item, `${prefix}[${index}]`) ); } else if (obj && typeof obj === 'object') { - const result: Record<string, unknown> = {}; + const result: Record<string, unknown> = Object.create(null); Object.entries(obj).forEach(([key, value]) => { const newPrefix = prefix ? `${prefix}.${key}` : key; result[key] = reconstruct(value, newPrefix); }); return result; } return obj; }; return reconstruct(originalJson); };src-tauri/src/backup.rs (1)
424-436: Consider using walkdir crate for recursive directory traversal.The current recursive directory size calculation works but could be more robust and efficient using the
walkdircrate.// Add to Cargo.toml dependencies // walkdir = "2.3" use walkdir::WalkDir; fn calculate_dir_size(dir: &Path) -> Result<u64, std::io::Error> { let mut size = 0u64; for entry in WalkDir::new(dir).into_iter().filter_map(|e| e.ok()) { if entry.file_type().is_file() { size += entry.metadata()?.len(); } } Ok(size) }src/lib/services/translation-service.ts (2)
770-802: Verify content splitting logic for potential edge cases.The content splitting logic looks good but consider these edge cases:
private trySplitLongContent(key: string, value: string): [string, string][] { // For very long values, try to split at sentence boundaries if (value.length > TRANSLATION_DEFAULTS.contentSplitThreshold) { - const sentences = value.split(/[.!?]+/).filter(s => s.trim().length > 0); + const sentences = value.split(/[.!?]+(?=\s|$)/).filter(s => s.trim().length > 0); if (sentences.length > 1) { const result: [string, string][] = []; let currentPart = ''; for (let i = 0; i < sentences.length; i++) { const sentence = sentences[i].trim(); + // Skip empty sentences + if (!sentence) continue; + const testPart = currentPart + (currentPart ? '. ' : '') + sentence;The regex improvement ensures we don't split on periods within abbreviations, and the empty sentence check prevents issues with multiple consecutive punctuation marks.
231-251: Consider adding validation for session ID format.The session ID generation and validation looks good, but consider adding format validation for consistency.
private async logTranslationStart( sessionId: string, targetLanguage: string, totalFiles: number, totalContentSize: number ): Promise<void> { try { - // Ensure sessionId is valid - const validSessionId = sessionId || `session_${Date.now()}_${Math.random().toString(36).substring(2, 7)}`; + // Ensure sessionId is valid and follows expected format + const sessionIdRegex = /^session_\d+_[a-z0-9]+$/; + const validSessionId = (sessionId && sessionIdRegex.test(sessionId)) + ? sessionId + : `session_${Date.now()}_${Math.random().toString(36).substring(2, 7)}`;src-tauri/src/filesystem.rs (1)
386-390: Minor optimization: Simplify extension comparison logic.The current extension comparison creates unnecessary string allocations.
if entry_path.is_file() && entry_path.extension() .and_then(|ext| ext.to_str()) .map_or(false, |ext| ext == extension.trim_start_matches('.')) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src-tauri/build.rs(1 hunks)src-tauri/src/backup.rs(1 hunks)src-tauri/src/config.rs(2 hunks)src-tauri/src/filesystem.rs(7 hunks)src-tauri/src/lib.rs(1 hunks)src-tauri/src/logging.rs(11 hunks)src-tauri/src/main.rs(1 hunks)src-tauri/src/minecraft/mod.rs(19 hunks)src-tauri/src/tests/lang_file_test.rs(1 hunks)src-tauri/src/tests/mod.rs(1 hunks)src/__tests__/test-setup.ts(1 hunks)src/components/tabs/custom-files-tab.tsx(5 hunks)src/components/tabs/mods-tab.tsx(7 hunks)src/components/ui/backup-dialog.tsx(1 hunks)src/lib/adapters/openai-adapter.ts(5 hunks)src/lib/services/translation-service.ts(16 hunks)src/lib/utils/error-logger.ts(1 hunks)src/lib/utils/translation-helpers.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src-tauri/src/main.rs
- src-tauri/build.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src-tauri/src/tests/mod.rs
- src-tauri/src/tests/lang_file_test.rs
- src/tests/test-setup.ts
- src/lib/adapters/openai-adapter.ts
- src/components/tabs/mods-tab.tsx
- src-tauri/src/logging.rs
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/components/ui/backup-dialog.tsx (9)
src/lib/services/backup-service.ts (2)
BackupInfo(72-79)backupService(279-279)src/lib/i18n.ts (1)
useAppTranslation(41-43)src/components/ui/card.tsx (1)
Card(80-80)src/components/ui/badge.tsx (1)
Badge(36-36)src/components/ui/button.tsx (1)
Button(59-59)src/lib/types/minecraft.ts (1)
TranslationTargetType(8-8)src/lib/services/file-service.ts (1)
FileService(188-382)src/components/ui/dialog.tsx (5)
Dialog(125-125)DialogContent(127-127)DialogHeader(130-130)DialogTitle(133-133)DialogFooter(129-129)src/components/ui/scroll-area.tsx (1)
ScrollArea(58-58)
src/lib/utils/translation-helpers.ts (3)
src/lib/types/minecraft.ts (2)
TranslationTarget(103-120)TranslationTargetType(8-8)src-tauri/src/logging.rs (1)
error(139-141)src/lib/services/file-service.ts (1)
invoke(202-209)
src/lib/services/translation-service.ts (5)
src/lib/constants/defaults.ts (2)
API_DEFAULTS(26-31)TRANSLATION_DEFAULTS(36-48)src/lib/utils/error-logger.ts (1)
ErrorLogger(7-71)src-tauri/src/logging.rs (1)
error(139-141)src/lib/services/file-service.ts (1)
invoke(202-209)src/lib/utils/token-counter.ts (3)
estimateTokens(45-77)TokenEstimationConfig(6-17)DEFAULT_TOKEN_CONFIG(37-37)
src/lib/utils/error-logger.ts (6)
src-tauri/src/logging.rs (1)
error(139-141)src/lib/services/file-service.ts (1)
invoke(202-209)src/lib/adapters/openai-adapter.ts (1)
logError(49-55)src/lib/services/translation-service.ts (1)
logError(214-222)src/lib/adapters/anthropic-adapter.ts (1)
logError(49-55)src/lib/adapters/gemini-adapter.ts (1)
logError(49-55)
🪛 Biome (1.9.4)
src/lib/utils/error-logger.ts
[error] 7-71: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 23-23: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 40-40: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Security Scan
🔇 Additional comments (17)
src-tauri/src/config.rs (1)
109-109: Default model name verified as correctThe default model name
"o4-mini-2025-04-16"corresponds to the specific o4-mini snapshot released on April 16, 2025, and is a valid OpenAI model identifier. No changes are required.src-tauri/src/lib.rs (2)
37-73: Well-structured build configuration.The separation of debug and non-debug build configurations is a good practice. The updater plugin being enabled only for release builds is appropriate for security and stability.
75-124: Comprehensive command registration.The organized grouping of commands by functionality (Minecraft operations, file system, logging, backup) improves maintainability and makes the API surface clear.
src/components/ui/backup-dialog.tsx (1)
38-145: Well-designed BackupItem component.The component provides comprehensive backup information display with proper error handling, accessibility features, and clean UI organization. The date formatting and file size utilities are correctly implemented.
src/lib/utils/translation-helpers.ts (3)
11-15: Robust path calculation logic.The function correctly handles both cases - paths starting with the base directory and those that don't, with proper path separator normalization.
20-22: Clean and efficient sorting implementation.Using spread operator to avoid mutating the original array and
localeComparefor proper alphabetical sorting is the correct approach.
77-94: Well-structured factory function.The function provides sensible defaults (selected: true) and proper composition with additional properties, making it flexible for different use cases.
src/components/tabs/custom-files-tab.tsx (1)
83-83: Good practice: Properly handling unused parameter.The underscore prefix for the unused
_selectedDirectoryparameter correctly indicates it's intentionally unused, following TypeScript/ESLint conventions.src-tauri/src/minecraft/mod.rs (2)
589-686: Verify format detection logic correctness.The new format detection function looks well-implemented. The logic correctly detects format from the
en_usfile and handles both JSON and legacy.langformats appropriately.The error handling with
continueinstead of failing the entire extraction is a good approach for robustness.
404-408: Enhance error logging with more context.The enhanced error logging with line-specific context is a good improvement for debugging JSON parsing failures.
src-tauri/src/backup.rs (1)
1-458: Well-implemented backup management system.This new backup module demonstrates excellent practices:
- Proper error handling with detailed logging
- Session-based organization for backup isolation
- Comprehensive CRUD operations for backup management
- Good integration with the existing logging infrastructure
- Safe file operations with proper error propagation
The implementation follows Rust best practices and provides a robust foundation for backup functionality.
src/lib/services/translation-service.ts (2)
628-703: Excellent implementation of token-based chunking with proper fallback.The token-based chunking implementation is well-thought-out with several good practices:
- Proper token estimation using provider-specific configurations
- Intelligent handling of oversized entries with content splitting
- Fallback to entry-based chunking when token estimation fails
- Comprehensive logging for debugging and monitoring
The logic correctly handles edge cases and provides robust chunking behavior.
872-878: Good handling of API key configuration errors.The enhanced error handling for API key issues is well-implemented. Returning empty results instead of throwing exceptions prevents cascading failures while still marking the translation as failed.
src-tauri/src/filesystem.rs (4)
2-8: LGTM: Import reorganization improves code organization.The import reorganization with proper spacing and alphabetical ordering enhances readability and maintainability.
289-357: LGTM: Enhanced Better Quests file discovery with dual location support.The implementation correctly handles both standard JSON files in the lang directory and direct .lang files in the config directory. This provides comprehensive coverage for different Better Questing configurations.
570-634: Excellent addition: Legacy .lang format support enhances compatibility.The new format parameter allows writing both JSON and legacy .lang formats, which significantly improves compatibility with older Minecraft versions and mods. The implementation correctly:
- Defaults to JSON format for backward compatibility
- Converts JSON to key=value format for .lang files
- Sorts keys for consistent output
- Uses appropriate file extensions
193-264: FTB Quest Directory Fallback Verification CompletedI searched the codebase for any other
ftbquestsreferences and found no additional directory patterns. The current logic covers all occurrences:
config/ftbquests/questsconfig/ftbquests/normalconfig/ftbquestsNo further changes are needed unless future FTB mods introduce new directory structures.
| try { | ||
| await invoke('log_warning', { | ||
| message: `JSON parsing error in ${itemType}: ${itemPath} - ${errorMessage}`, | ||
| processType | ||
| }); | ||
| } catch {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in logging operations.
The empty catch blocks silently ignore logging failures, which could hide important backend communication issues. Consider logging these failures or implementing a fallback mechanism.
} catch {
+ } catch (logError) {
+ console.warn(`Failed to log ${processType} warning:`, logError);
}Apply similar changes to all catch blocks in this function.
Also applies to: 48-53, 57-62, 65-70
🤖 Prompt for AI Agents
In src/lib/utils/translation-helpers.ts around lines 39 to 44 and similarly at
lines 48-53, 57-62, and 65-70, the catch blocks are empty and silently ignore
errors during logging operations. Update these catch blocks to handle errors
more robustly by logging the caught error or implementing a fallback mechanism
to ensure that failures in logging do not go unnoticed. This will improve error
visibility and aid in debugging backend communication issues.
| // Validate URL | ||
| if !url.starts_with("http://") && !url.starts_with("https://") { | ||
| return Err("Invalid URL: must start with http:// or https://".to_string()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security: URL validation is too restrictive and could be bypassed.
The current URL validation only checks for http:// and https:// prefixes, which could miss malformed URLs or allow potentially harmful URLs that start correctly but contain malicious content.
Consider using a proper URL parsing library for validation:
use url::Url;
// Replace the current validation with:
match Url::parse(url) {
Ok(parsed_url) => {
if !["http", "https"].contains(&parsed_url.scheme()) {
return Err("Invalid URL: only HTTP and HTTPS are allowed".to_string());
}
}
Err(_) => {
return Err("Invalid URL format".to_string());
}
}Add url = "2.4" to your Cargo.toml dependencies.
🤖 Prompt for AI Agents
In src-tauri/src/filesystem.rs around lines 647 to 650, the URL validation is
currently done by checking if the string starts with "http://" or "https://",
which is insufficient and insecure. Replace this check with proper URL parsing
using the url crate: add `url = "2.4"` to Cargo.toml dependencies, then use
`Url::parse(url)` to parse the URL, return an error if parsing fails, and verify
that the scheme is either "http" or "https"; if not, return an error indicating
only HTTP and HTTPS are allowed.
- Update Rust dependencies to fix RUSTSEC-2025-0008 (windows) and RUSTSEC-2025-0023 (tokio) - Update Jest config to exclude Bun/Vitest test files - Remaining warnings are for unmaintained GTK3 bindings which are Tauri dependencies
- Update toml dependency to fix build errors - Fix Rust compiler warnings (unused imports/variables) - Fix Jest configuration regex syntax - Update progress store tests to handle backward progress prevention - Apply Rust code formatting
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
jest.config.js(1 hunks)src-tauri/Cargo.toml(1 hunks)src-tauri/src/backup.rs(1 hunks)src-tauri/src/filesystem.rs(7 hunks)src-tauri/src/minecraft/mod.rs(20 hunks)src/lib/store/__tests__/progress-store.jest.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- jest.config.js
- src-tauri/src/minecraft/mod.rs
- src-tauri/src/filesystem.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/lib/store/__tests__/progress-store.jest.test.ts (1)
src/lib/store/index.ts (1)
useAppStore(78-296)
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Security Scan
🔇 Additional comments (2)
src/lib/store/__tests__/progress-store.jest.test.ts (2)
36-40: Test correctly verifies backward progress prevention for negative values.The updated test logic properly captures the current
wholeProgressvalue and verifies that setting a negative value doesn't decrease the progress, which aligns with the new backward progress prevention implemented in the store.
42-52: Test correctly verifies backward progress prevention for null/undefined values.The updated test logic properly tests the new behavior where null/undefined values (treated as 0 by the store due to
progress || 0) don't cause backward progress when the current progress is higher. The comments clearly explain the expected behavior and the test assertions correctly verify that progress values are maintained rather than reset to 0.
src-tauri/src/backup.rs
Outdated
| let backups = list_backups(None, None, None, logger.clone())?; | ||
| let backup = backups | ||
| .iter() | ||
| .find(|b| b.metadata.id == backup_id) | ||
| .ok_or_else(|| format!("Backup not found: {}", backup_id))?; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize backup lookup with direct path resolution.
Using list_backups() to find a single backup is inefficient as it reads all backup metadata. Consider implementing direct backup resolution.
Replace with direct path resolution:
- // Find the backup by ID
- let backups = list_backups(None, None, None, logger.clone())?;
- let backup = backups
- .iter()
- .find(|b| b.metadata.id == backup_id)
- .ok_or_else(|| format!("Backup not found: {}", backup_id))?;
+ // Find backup directly by searching session directories
+ let backup = find_backup_by_id(&backup_id)?;Add helper function:
fn find_backup_by_id(backup_id: &str) -> Result<BackupInfo, String> {
let logs_dir = PathBuf::from("logs").join("localizer");
for session_entry in fs::read_dir(&logs_dir).map_err(|e| format!("Failed to read logs directory: {}", e))? {
let session_path = session_entry.map_err(|e| format!("Failed to read session entry: {}", e))?.path();
let backup_path = session_path.join("backups").join(backup_id);
if backup_path.exists() {
let metadata_path = backup_path.join("metadata.json");
if let Ok(metadata_content) = fs::read_to_string(&metadata_path) {
if let Ok(metadata) = serde_json::from_str::<BackupMetadata>(&metadata_content) {
let can_restore = backup_path.join("original_files").exists();
return Ok(BackupInfo {
metadata,
backup_path: backup_path.to_string_lossy().to_string(),
can_restore,
});
}
}
}
}
Err(format!("Backup not found: {}", backup_id))
}🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 264 to 268, the current code uses
list_backups() to find a backup by ID, which is inefficient as it reads all
backups. Replace this with a direct path resolution approach by implementing a
helper function that iterates over session directories under logs/localizer,
checks for the backup directory matching the backup_id, reads its metadata.json,
and returns the BackupInfo if found. Use this helper function to find the backup
directly instead of listing all backups.
| use crate::logging::AppLogger; | ||
| /** | ||
| * Backup module for managing translation file backups | ||
| * Integrates with existing logging infrastructure to store backups in session directories | ||
| */ | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::fs; | ||
| use std::path::{Path, PathBuf}; | ||
| use std::sync::Arc; | ||
| use tauri::State; | ||
|
|
There was a problem hiding this comment.
Add missing chrono dependency.
The code uses chrono crate in prune_old_backups function but it's not imported at the module level.
Add the missing import:
use crate::logging::AppLogger;
+use chrono::{DateTime, Duration, Utc};
use serde::{Deserialize, Serialize};🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 1 to 11, the chrono crate is used in the
prune_old_backups function but is not imported at the module level. Add the
missing import statement for chrono, such as `use chrono::{DateTime, Utc};`, at
the top of the file with the other use statements to ensure the code compiles
and functions correctly.
| // Check if backup can be restored (original files exist) | ||
| let original_files_dir = backup_path.join("original_files"); | ||
| let can_restore = original_files_dir.exists() | ||
| && original_files_dir | ||
| .read_dir() | ||
| .map(|mut entries| entries.next().is_some()) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve backup restoration check logic.
The current can_restore check is overly simplistic and may give false positives if the directory exists but is empty or contains non-file entries.
Improve the restoration feasibility check:
- // Check if backup can be restored (original files exist)
- let original_files_dir = backup_path.join("original_files");
- let can_restore = original_files_dir.exists()
- && original_files_dir
- .read_dir()
- .map(|mut entries| entries.next().is_some())
- .unwrap_or(false);
+ // Check if backup can be restored (original files exist and are valid)
+ let original_files_dir = backup_path.join("original_files");
+ let can_restore = original_files_dir.exists() && {
+ fs::read_dir(&original_files_dir)
+ .map(|entries| {
+ entries
+ .filter_map(|entry| entry.ok())
+ .any(|entry| entry.path().is_file())
+ })
+ .unwrap_or(false)
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if backup can be restored (original files exist) | |
| let original_files_dir = backup_path.join("original_files"); | |
| let can_restore = original_files_dir.exists() | |
| && original_files_dir | |
| .read_dir() | |
| .map(|mut entries| entries.next().is_some()) | |
| .unwrap_or(false); | |
| // Check if backup can be restored (original files exist and are valid) | |
| let original_files_dir = backup_path.join("original_files"); | |
| let can_restore = original_files_dir.exists() && { | |
| fs::read_dir(&original_files_dir) | |
| .map(|entries| { | |
| entries | |
| .filter_map(|entry| entry.ok()) | |
| .any(|entry| entry.path().is_file()) | |
| }) | |
| .unwrap_or(false) | |
| }; |
🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 221 to 227, the current check for backup
restoration feasibility only verifies if the "original_files" directory exists
and is non-empty, which can lead to false positives if the directory contains no
actual files. Update the logic to confirm that the directory contains at least
one valid file entry by iterating over the directory entries and checking that
at least one is a file, not just any entry. This ensures the restoration check
accurately reflects the presence of original files.
| pub fn create_backup( | ||
| metadata: BackupMetadata, | ||
| file_paths: Vec<String>, | ||
| logger: State<Arc<AppLogger>>, | ||
| ) -> Result<String, String> { | ||
| logger.info(&format!("Creating backup: {}", metadata.id), Some("BACKUP")); | ||
|
|
||
| // Construct backup path using session structure: logs/localizer/{session_id}/backups/{backup_id} | ||
| let backup_dir = PathBuf::from("logs") | ||
| .join("localizer") | ||
| .join(&metadata.session_id) | ||
| .join("backups") | ||
| .join(&metadata.id); |
There was a problem hiding this comment.
Validate backup_id for path traversal attacks.
The backup_id parameter is used directly in path construction without validation, potentially allowing path traversal attacks.
Add validation to prevent directory traversal:
pub fn create_backup(
metadata: BackupMetadata,
file_paths: Vec<String>,
logger: State<Arc<AppLogger>>,
) -> Result<String, String> {
+ // Validate backup_id to prevent path traversal
+ if metadata.id.contains("..") || metadata.id.contains('/') || metadata.id.contains('\\') {
+ return Err("Invalid backup ID: contains path traversal characters".to_string());
+ }
+
logger.info(&format!("Creating backup: {}", metadata.id), Some("BACKUP"));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn create_backup( | |
| metadata: BackupMetadata, | |
| file_paths: Vec<String>, | |
| logger: State<Arc<AppLogger>>, | |
| ) -> Result<String, String> { | |
| logger.info(&format!("Creating backup: {}", metadata.id), Some("BACKUP")); | |
| // Construct backup path using session structure: logs/localizer/{session_id}/backups/{backup_id} | |
| let backup_dir = PathBuf::from("logs") | |
| .join("localizer") | |
| .join(&metadata.session_id) | |
| .join("backups") | |
| .join(&metadata.id); | |
| pub fn create_backup( | |
| metadata: BackupMetadata, | |
| file_paths: Vec<String>, | |
| logger: State<Arc<AppLogger>>, | |
| ) -> Result<String, String> { | |
| // Validate backup_id to prevent path traversal | |
| if metadata.id.contains("..") || metadata.id.contains('/') || metadata.id.contains('\\') { | |
| return Err("Invalid backup ID: contains path traversal characters".to_string()); | |
| } | |
| logger.info(&format!("Creating backup: {}", metadata.id), Some("BACKUP")); | |
| // Construct backup path using session structure: logs/localizer/{session_id}/backups/{backup_id} | |
| let backup_dir = PathBuf::from("logs") | |
| .join("localizer") | |
| .join(&metadata.session_id) | |
| .join("backups") | |
| .join(&metadata.id); | |
| // … rest of function … | |
| } |
🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 57 to 69, the backup_id from metadata is
used directly in constructing the backup directory path, which risks path
traversal attacks. Add validation to ensure backup_id does not contain any path
separators or traversal patterns like "..". Reject or sanitize invalid backup_id
values before using them in the path construction to prevent directory traversal
vulnerabilities.
| fn calculate_dir_size(dir: &Path) -> Result<u64, std::io::Error> { | ||
| let mut size = 0u64; | ||
| for entry in fs::read_dir(dir)? { | ||
| let entry = entry?; | ||
| let path = entry.path(); | ||
| if path.is_dir() { | ||
| size += calculate_dir_size(&path)?; | ||
| } else { | ||
| size += entry.metadata()?.len(); | ||
| } | ||
| } | ||
| Ok(size) | ||
| } |
There was a problem hiding this comment.
Prevent infinite loops and stack overflow in directory size calculation.
The recursive calculate_dir_size function doesn't handle symlinks or deep directory structures, potentially causing infinite loops or stack overflow.
Add safeguards for symlinks and recursion depth:
- fn calculate_dir_size(dir: &Path) -> Result<u64, std::io::Error> {
+ fn calculate_dir_size(dir: &Path, max_depth: usize) -> Result<u64, std::io::Error> {
+ if max_depth == 0 {
+ return Ok(0);
+ }
+
let mut size = 0u64;
for entry in fs::read_dir(dir)? {
let entry = entry?;
let path = entry.path();
- if path.is_dir() {
- size += calculate_dir_size(&path)?;
+ // Skip symlinks to prevent infinite loops
+ if path.is_dir() && !path.is_symlink() {
+ size += calculate_dir_size(&path, max_depth - 1)?;
} else {
size += entry.metadata()?.len();
}
}
Ok(size)
}And update the call:
- total_size += calculate_dir_size(&backups_dir)
+ total_size += calculate_dir_size(&backups_dir, 10) // Max depth of 10Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 423 to 435, the calculate_dir_size
function recursively sums directory sizes but lacks checks for symlinks and
recursion depth, risking infinite loops or stack overflow. Modify the function
to detect and skip symlinked directories and add a recursion depth parameter
with a maximum limit to prevent excessive recursion. Update all recursive calls
to pass and increment this depth parameter accordingly.
| let file_name = source_path | ||
| .file_name() | ||
| .ok_or_else(|| format!("Invalid file path: {}", file_path))?; | ||
| let dest_path = original_files_dir.join(file_name); |
There was a problem hiding this comment.
Fix potential file name conflicts and directory structure loss.
The current implementation only preserves the file name during backup, discarding the original directory structure. This can cause conflicts if multiple files have the same name from different directories.
Consider preserving the relative path structure or using a hash-based naming scheme:
- let file_name = source_path
- .file_name()
- .ok_or_else(|| format!("Invalid file path: {}", file_path))?;
- let dest_path = original_files_dir.join(file_name);
+ // Preserve directory structure or use hash-based naming
+ let file_name = source_path
+ .file_name()
+ .ok_or_else(|| format!("Invalid file path: {}", file_path))?;
+
+ // Create a safe filename that preserves some path info
+ let safe_name = format!("{}_{}",
+ blake3::hash(file_path.as_bytes()).to_hex()[..8].to_string(),
+ file_name.to_string_lossy()
+ );
+ let dest_path = original_files_dir.join(safe_name);🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 93 to 96, the code currently only uses
the file name for the backup destination, which can cause conflicts if files
with the same name exist in different directories. To fix this, preserve the
relative directory structure from the source path when constructing the
destination path or implement a hash-based naming scheme to ensure uniqueness.
Adjust the code to join the relative path (not just the file name) to the backup
directory or generate a unique name using a hash of the full path.
| if let Err(e) = fs::copy(&source_path, &dest_path) { | ||
| logger.warning( | ||
| &format!("Failed to restore file {}: {}", source_path.display(), e), | ||
| Some("BACKUP"), | ||
| ); | ||
| } else { | ||
| restored_count += 1; | ||
| logger.debug( | ||
| &format!( | ||
| "Restored file: {} -> {}", | ||
| source_path.display(), | ||
| dest_path.display() | ||
| ), | ||
| Some("BACKUP"), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add warning when overwriting existing files during restoration.
The restore operation silently overwrites existing files without warning the user, which could lead to data loss.
Add file existence check and logging:
let dest_path = target_dir.join(file_name);
+ // Warn if file already exists
+ if dest_path.exists() {
+ logger.warning(
+ &format!("Overwriting existing file during restoration: {}", dest_path.display()),
+ Some("BACKUP"),
+ );
+ }
+
if let Err(e) = fs::copy(&source_path, &dest_path) {
logger.warning(
&format!("Failed to restore file {}: {}", source_path.display(), e),
Some("BACKUP"),
);
} else {
restored_count += 1;
logger.debug(
&format!(
"Restored file: {} -> {}",
source_path.display(),
dest_path.display()
),
Some("BACKUP"),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Err(e) = fs::copy(&source_path, &dest_path) { | |
| logger.warning( | |
| &format!("Failed to restore file {}: {}", source_path.display(), e), | |
| Some("BACKUP"), | |
| ); | |
| } else { | |
| restored_count += 1; | |
| logger.debug( | |
| &format!( | |
| "Restored file: {} -> {}", | |
| source_path.display(), | |
| dest_path.display() | |
| ), | |
| Some("BACKUP"), | |
| ); | |
| } | |
| } | |
| let dest_path = target_dir.join(file_name); | |
| // Warn if file already exists | |
| if dest_path.exists() { | |
| logger.warning( | |
| &format!("Overwriting existing file during restoration: {}", dest_path.display()), | |
| Some("BACKUP"), | |
| ); | |
| } | |
| if let Err(e) = fs::copy(&source_path, &dest_path) { | |
| logger.warning( | |
| &format!("Failed to restore file {}: {}", source_path.display(), e), | |
| Some("BACKUP"), | |
| ); | |
| } else { | |
| restored_count += 1; | |
| logger.debug( | |
| &format!( | |
| "Restored file: {} -> {}", | |
| source_path.display(), | |
| dest_path.display() | |
| ), | |
| Some("BACKUP"), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 301 to 317, the restore function
overwrites existing files without warning. Before copying, check if the
destination file already exists. If it does, log a warning message indicating
that the file will be overwritten. Then proceed with the copy operation and
handle errors as before.
| let target_dir = Path::new(&target_path); | ||
|
|
||
| // Create target directory if it doesn't exist | ||
| if let Err(e) = fs::create_dir_all(target_dir) { | ||
| let error_msg = format!("Failed to create target directory: {}", e); | ||
| logger.error(&error_msg, Some("BACKUP")); | ||
| return Err(error_msg); | ||
| } |
There was a problem hiding this comment.
Validate target_path for path traversal attacks.
The target_path parameter is used directly without validation, potentially allowing path traversal attacks.
Add path validation:
let target_dir = Path::new(&target_path);
+
+ // Validate target path to prevent directory traversal
+ if target_path.contains("..") {
+ return Err("Invalid target path: contains path traversal characters".to_string());
+ }
+
+ // Ensure target path is absolute or within allowed directories
+ let canonical_target = target_dir.canonicalize()
+ .map_err(|_| "Invalid target path".to_string())?;🤖 Prompt for AI Agents
In src-tauri/src/backup.rs around lines 277 to 284, the target_path is used
directly without validation, which can lead to path traversal vulnerabilities.
Before creating the target directory, validate target_path to ensure it does not
contain any path traversal components like ".." or absolute paths that could
escape the intended directory. Implement a check that normalizes the path and
confirms it stays within an allowed base directory or does not contain
suspicious segments, and return an error if validation fails.
| // Copy files to backup location | ||
| let mut backed_up_files = Vec::new(); | ||
| for file_path in &file_paths { | ||
| let source_path = Path::new(file_path); | ||
|
|
||
| if source_path.exists() { | ||
| // Create destination path maintaining relative structure | ||
| let file_name = source_path | ||
| .file_name() | ||
| .ok_or_else(|| format!("Invalid file path: {}", file_path))?; | ||
| let dest_path = original_files_dir.join(file_name); | ||
|
|
||
| // Copy file | ||
| if let Err(e) = fs::copy(source_path, &dest_path) { | ||
| logger.warning( | ||
| &format!("Failed to backup file {}: {}", file_path, e), | ||
| Some("BACKUP"), | ||
| ); | ||
| } else { | ||
| backed_up_files.push(dest_path.to_string_lossy().to_string()); | ||
| logger.debug( | ||
| &format!("Backed up file: {} -> {}", file_path, dest_path.display()), | ||
| Some("BACKUP"), | ||
| ); | ||
| } | ||
| } else { | ||
| logger.warning( | ||
| &format!("Source file not found for backup: {}", file_path), | ||
| Some("BACKUP"), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make backup operations atomic to prevent partial backups.
If the backup operation fails partway through, it leaves a partial backup directory with some files missing. This could cause issues during restoration.
Implement atomic backup creation using a temporary directory:
// Copy files to backup location
+ let temp_backup_dir = backup_dir.with_extension("tmp");
+ let temp_original_files_dir = temp_backup_dir.join("original_files");
+
+ // Create temporary directories
+ fs::create_dir_all(&temp_original_files_dir)
+ .map_err(|e| format!("Failed to create temporary backup directory: {}", e))?;
+
let mut backed_up_files = Vec::new();
for file_path in &file_paths {
let source_path = Path::new(file_path);
if source_path.exists() {
let file_name = source_path
.file_name()
.ok_or_else(|| format!("Invalid file path: {}", file_path))?;
- let dest_path = original_files_dir.join(file_name);
+ let dest_path = temp_original_files_dir.join(file_name);
// Copy file
if let Err(e) = fs::copy(source_path, &dest_path) {
+ // Clean up temp directory on failure
+ let _ = fs::remove_dir_all(&temp_backup_dir);
logger.warning(
&format!("Failed to backup file {}: {}", file_path, e),
Some("BACKUP"),
);
+ return Err(format!("Backup failed due to file copy error: {}", e));
} else {
backed_up_files.push(dest_path.to_string_lossy().to_string());
logger.debug(
&format!("Backed up file: {} -> {}", file_path, dest_path.display()),
Some("BACKUP"),
);
}
} else {
logger.warning(
&format!("Source file not found for backup: {}", file_path),
Some("BACKUP"),
);
}
}
+
+ // Save metadata to temp directory
+ let temp_metadata_path = temp_backup_dir.join("metadata.json");
+ // ... metadata writing code ...
+
+ // Atomically move temp directory to final location
+ fs::rename(&temp_backup_dir, &backup_dir)
+ .map_err(|e| format!("Failed to finalize backup: {}", e))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Copy files to backup location | |
| let mut backed_up_files = Vec::new(); | |
| for file_path in &file_paths { | |
| let source_path = Path::new(file_path); | |
| if source_path.exists() { | |
| // Create destination path maintaining relative structure | |
| let file_name = source_path | |
| .file_name() | |
| .ok_or_else(|| format!("Invalid file path: {}", file_path))?; | |
| let dest_path = original_files_dir.join(file_name); | |
| // Copy file | |
| if let Err(e) = fs::copy(source_path, &dest_path) { | |
| logger.warning( | |
| &format!("Failed to backup file {}: {}", file_path, e), | |
| Some("BACKUP"), | |
| ); | |
| } else { | |
| backed_up_files.push(dest_path.to_string_lossy().to_string()); | |
| logger.debug( | |
| &format!("Backed up file: {} -> {}", file_path, dest_path.display()), | |
| Some("BACKUP"), | |
| ); | |
| } | |
| } else { | |
| logger.warning( | |
| &format!("Source file not found for backup: {}", file_path), | |
| Some("BACKUP"), | |
| ); | |
| } | |
| } | |
| // Copy files to backup location | |
| let temp_backup_dir = backup_dir.with_extension("tmp"); | |
| let temp_original_files_dir = temp_backup_dir.join("original_files"); | |
| // Create temporary directories | |
| fs::create_dir_all(&temp_original_files_dir) | |
| .map_err(|e| format!("Failed to create temporary backup directory: {}", e))?; | |
| let mut backed_up_files = Vec::new(); | |
| for file_path in &file_paths { | |
| let source_path = Path::new(file_path); | |
| if source_path.exists() { | |
| // Create destination path maintaining relative structure | |
| let file_name = source_path | |
| .file_name() | |
| .ok_or_else(|| format!("Invalid file path: {}", file_path))?; | |
| let dest_path = temp_original_files_dir.join(file_name); | |
| // Copy file | |
| if let Err(e) = fs::copy(source_path, &dest_path) { | |
| // Clean up temp directory on failure | |
| let _ = fs::remove_dir_all(&temp_backup_dir); | |
| logger.warning( | |
| &format!("Failed to backup file {}: {}", file_path, e), | |
| Some("BACKUP"), | |
| ); | |
| return Err(format!("Backup failed due to file copy error: {}", e)); | |
| } else { | |
| backed_up_files.push(dest_path.to_string_lossy().to_string()); | |
| logger.debug( | |
| &format!("Backed up file: {} -> {}", file_path, dest_path.display()), | |
| Some("BACKUP"), | |
| ); | |
| } | |
| } else { | |
| logger.warning( | |
| &format!("Source file not found for backup: {}", file_path), | |
| Some("BACKUP"), | |
| ); | |
| } | |
| } | |
| // Save metadata to temp directory | |
| let temp_metadata_path = temp_backup_dir.join("metadata.json"); | |
| // ... metadata writing code ... | |
| // Atomically move temp directory to final location | |
| fs::rename(&temp_backup_dir, &backup_dir) | |
| .map_err(|e| format!("Failed to finalize backup: {}", e))?; |
🤖 Prompt for AI Agents
In src-tauri/src/backup.rs between lines 86 and 117, the backup process copies
files directly to the final backup directory, which can result in partial
backups if an error occurs mid-operation. To fix this, create a temporary
directory for the backup files first, copy all files there, and only after all
copies succeed, rename or move the temporary directory to the final backup
location atomically. This ensures that either the entire backup is present or
none of it is, preventing partial backups.
- Fixed TypeScript error in test-setup.ts with proper type assertion - Added missing sessionId and enableBackup fields to TranslationResult interface - All frontend tests, type checking, and linting now pass Co-Authored-By: Claude <[email protected]>
- Fixed all Rust Clippy warnings: - Added Default impl for AppLogger - Reduced function arguments using structs for log_file_progress and log_translation_completion - Replaced match with if-let for single pattern matching - Used .first() instead of .get(0) - Fixed map_or with is_some_and - Fixed unused variables and imports - Fixed deprecated warning with #[allow(deprecated)] - Applied CodeRabbit suggestions: - Extracted lang file parsing logic to a reusable utility module - Created parseLangFile and stringifyLangFile functions - Updated quests-tab.tsx to use the new utility Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/components/tabs/quests-tab.tsx (2)
55-55: Use platform-agnostic path utilities for file path operations.The current implementation still uses string manipulation with forward slashes, which may not work correctly on Windows.
Consider using platform-agnostic path handling:
-const fileName = questFile.split('/').pop() || "unknown"; +const fileName = questFile.split(/[/\\]/).pop() || "unknown";This matches the previous review suggestion for cross-platform compatibility.
Also applies to: 83-83
115-288: Consider breaking down this large function for better maintainability.The
handleTranslatefunction remains very large (173 lines) and handles multiple responsibilities. This matches the previous review feedback about function complexity.The function still handles:
- File processing and .lang parsing (lines 147-158)
- Job creation (lines 160-167)
- Translation execution coordination (lines 184-284)
- Complex output path generation and formatting (lines 194-255)
Consider extracting these concerns into separate helper functions as previously suggested to improve maintainability and testability.
src-tauri/src/minecraft/mod.rs (1)
382-386: Extract repeated buffer cleaning logic into a helper function.The buffer cleaning logic is duplicated 6 times throughout the file, which matches the previous review feedback about code duplication.
Add a helper function as previously suggested:
+/// Clean raw bytes by removing null bytes and control characters +fn clean_raw_bytes(buffer: Vec<u8>) -> Vec<u8> { + buffer.into_iter() + .filter(|&b| b != 0 && (b >= 0x20 || b == 0x09 || b == 0x0A || b == 0x0D)) + .collect() +}Then replace all instances:
- // First, remove any null bytes and other problematic bytes - let cleaned_buffer: Vec<u8> = buffer - .into_iter() - .filter(|&b| b != 0 && (b >= 0x20 || b == 0x09 || b == 0x0A || b == 0x0D)) - .collect(); + let cleaned_buffer = clean_raw_bytes(buffer);Also applies to: 426-430, 479-483, 531-536, 631-635, 852-856
🧹 Nitpick comments (3)
src/lib/utils/lang-parser.ts (2)
6-26: Consider handling edge cases for more robust .lang file parsing.The current implementation handles the basic .lang format well, but consider these edge cases:
- Empty values (e.g.,
key=) are valid in .lang files and should be preserved- Values might contain
=characters that should not be treated as separatorsConsider this more robust implementation:
export function parseLangFile(content: string): Record<string, string> { const langMap: Record<string, string> = {}; const lines = content.split('\n'); for (const line of lines) { const trimmed = line.trim(); // Skip empty lines and comments if (trimmed && !trimmed.startsWith('#')) { const separatorIndex = trimmed.indexOf('='); if (separatorIndex > -1) { const key = trimmed.substring(0, separatorIndex).trim(); const value = trimmed.substring(separatorIndex + 1).trim(); - if (key) { + if (key) { // Allow empty values as they're valid in .lang files langMap[key] = value; } } } } return langMap; }
33-37: Consider adding key sorting for consistent output.The current implementation produces output in object iteration order, which may not be deterministic. In
src/components/tabs/quests-tab.tsx(lines 220-226), keys are manually sorted before formatting. Consider making this utility function handle sorting consistently.export function stringifyLangFile(langMap: Record<string, string>): string { - return Object.entries(langMap) + return Object.entries(langMap) + .sort(([a], [b]) => a.localeCompare(b)) .map(([key, value]) => `${key}=${value}`) .join('\n'); }This ensures consistent output ordering and removes the need for manual sorting in consuming code.
src/components/tabs/quests-tab.tsx (1)
213-231: Consider using stringifyLangFile utility for consistency.The manual .lang formatting logic duplicates functionality that could be handled by the
stringifyLangFileutility. While the current implementation adds custom sorting, this could be moved into the utility function.If sorting is always needed, consider enhancing the utility:
// If it's a .lang file, format the output properly if (fileExtension === "lang") { try { // Parse the translated JSON content const translatedMap = JSON.parse(translatedText); - // Convert to .lang format - const langLines: string[] = []; - const sortedKeys = Object.keys(translatedMap).sort(); - - for (const key of sortedKeys) { - langLines.push(`${key}=${translatedMap[key]}`); - } - - translatedText = langLines.join('\n'); + // Use utility function for consistent formatting + translatedText = stringifyLangFile(translatedMap); } catch (error) { console.error('Failed to format .lang file output:', error); // Keep original format if parsing fails } }This would require updating the utility to include sorting as suggested in the previous review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src-tauri/src/filesystem.rs(7 hunks)src-tauri/src/logging.rs(12 hunks)src-tauri/src/minecraft/mod.rs(20 hunks)src/components/tabs/quests-tab.tsx(1 hunks)src/lib/utils/lang-parser.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src-tauri/src/filesystem.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/tabs/quests-tab.tsx (7)
src/lib/store/index.ts (1)
useAppStore(78-296)src/lib/services/file-service.ts (2)
FileService(188-382)invoke(202-209)src/lib/types/minecraft.ts (2)
TranslationTarget(103-120)TranslationResult(142-161)src/lib/services/translation-service.ts (2)
TranslationService(88-1001)TranslationJob(27-54)src/lib/utils/lang-parser.ts (1)
parseLangFile(6-26)src/lib/services/translation-runner.ts (1)
runTranslationJobs(28-132)src/components/tabs/common/translation-tab.tsx (1)
TranslationTab(95-593)
⏰ 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). (5)
- GitHub Check: Build - aarch64-apple-darwin
- GitHub Check: Build - x86_64-pc-windows-msvc
- GitHub Check: Build - x86_64-unknown-linux-gnu
- GitHub Check: Build - x86_64-apple-darwin
- GitHub Check: Security Scan
🔇 Additional comments (7)
src/components/tabs/quests-tab.tsx (2)
153-158: Code correctly uses the parseLangFile utility.The implementation properly uses the imported
parseLangFileutility to handle .lang file parsing, which is the correct approach for maintaining consistency.
153-158: Use the imported parseLangFile utility instead of duplicating logic.The code imports
parseLangFilefrom the lang-parser utility but implements inline parsing logic instead. This creates maintenance overhead and potential inconsistencies.// If it's a .lang file, convert to JSON format for translation if (target.path.endsWith('.lang')) { - const langMap = parseLangFile(content); - - // Convert to JSON string for translation - processedContent = JSON.stringify(langMap, null, 2); + const langMap = parseLangFile(content); + processedContent = JSON.stringify(langMap, null, 2); }Wait, I see the code already uses
parseLangFile. Let me look more carefully... Actually the code IS using parseLangFile on line 154. This is correct implementation.Likely an incorrect or invalid review comment.
src-tauri/src/minecraft/mod.rs (2)
589-686: Well-implemented format detection for language files.The new
extract_lang_files_from_archive_with_formatfunction properly detects the language file format based on theen_usfile extension, which is a reliable approach sinceen_usis the canonical base language file in Minecraft mods.The function correctly:
- Defaults to JSON format when no specific format is detected
- Uses the same robust file processing logic as the original extraction function
- Returns both the extracted files and the detected format
404-408: Improved error handling and logging.The enhanced error handling provides better debugging information:
- Line-specific context for JSON parsing errors
- Graceful handling of individual file parsing failures without terminating the entire extraction
This improves the robustness of the mod analysis process.
Also applies to: 553-559
src-tauri/src/logging.rs (3)
409-568: Comprehensive logging system for translation process tracking.The new logging commands provide excellent coverage for translation workflow monitoring:
log_translation_start: Session initialization with key metricslog_translation_statistics: Pre-translation analysislog_file_progress: Detailed per-file progress trackinglog_translation_completion: Comprehensive completion summarylog_performance_metrics: Performance debugging supportThe functions use consistent parameter patterns, appropriate log levels, and structured process type tags for effective log categorization.
240-407: Well-designed session management for translation workflows.The session-based directory creation provides:
- Consistent timestamp-based session IDs for unique directory naming
- Separation between different translation runs
- Both automatic session generation and explicit session ID support
- Proper error handling and logging for directory operations
This enables proper isolation and tracking of translation sessions.
58-62: Standard Default trait implementation.Adding the
Defaulttrait implementation forAppLoggerfollows Rust conventions and provides API convenience for initialization.
- Applied cargo fmt to fix formatting issues in filesystem.rs and logging.rs - Fixed multi-line closure formatting - Fixed function parameter formatting Co-Authored-By: Claude <[email protected]>
- Added libgtk-3-dev, libwebkit2gtk-4.1-dev, libappindicator3-dev, librsvg2-dev, patchelf - These are required for building Tauri applications on Linux - Matches the dependencies already present in the build workflow Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/pr-validation.yml (2)
40-42: Trim trailing whitespace to appease YAML lintLine 42 has a trailing space flagged by YAMLlint. This will fail strict linters and can be annoying in diffs.
- sudo apt-get install -y libgtk-3-dev libwebkit2gtk-4.1-dev libappindicator3-dev librsvg2-dev patchelf␠ + sudo apt-get install -y libgtk-3-dev libwebkit2gtk-4.1-dev libappindicator3-dev librsvg2-dev patchelf
38-42: Use--no-install-recommendsto keep the runner lightweightThe default
apt-get installbrings along a large tree of recommended packages, increasing CI time and bandwidth. Adding--no-install-recommendsshaves ~200 MB off this step without affecting the required headers and libs.- sudo apt-get install -y libgtk-3-dev libwebkit2gtk-4.1-dev libappindicator3-dev librsvg2-dev patchelf + sudo apt-get install -y --no-install-recommends \ + libgtk-3-dev libwebkit2gtk-4.1-dev libappindicator3-dev librsvg2-dev patchelf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-validation.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pr-validation.yml
[error] 42-42: trailing spaces
(trailing-spaces)
⏰ 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). (6)
- GitHub Check: Build - x86_64-apple-darwin
- GitHub Check: Build - x86_64-pc-windows-msvc
- GitHub Check: Build - aarch64-apple-darwin
- GitHub Check: Build - x86_64-unknown-linux-gnu
- GitHub Check: Security Scan
- GitHub Check: Validate PR
- Fixed all format string to use inline arguments (e.g., format!("{var}") instead of format!("{}", var))
- This is required by newer versions of Clippy in CI
- Applied automatically using cargo clippy --fix
Co-Authored-By: Claude <[email protected]>
- Fixed multi-line format strings to single line where appropriate - Consistent formatting across all Rust files - Applied automatically using cargo fmt Co-Authored-By: Claude <[email protected]>
- Changed from 'bun test' to 'npm test' in PR validation workflow - 'bun test' runs all tests including those with vi.mock which is not supported by Bun - 'npm test' runs only the test files that are compatible with Bun test runner - This matches the test script defined in package.json Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pr-validation.yml (1)
38-42: Trim trailing spaces & install without recommended packages
yamllintis already flagging trailing whitespace on line 42.
While you’re touching this line, using--no-install-recommendscuts CI image size/time by 10-20 s and avoids installing GUI extras you don’t need.- sudo apt-get install -y libgtk-3-dev libwebkit2gtk-4.1-dev libappindicator3-dev librsvg2-dev patchelf + sudo apt-get install -y --no-install-recommends \ + libgtk-3-dev \ + libwebkit2gtk-4.1-dev \ + libappindicator3-dev \ + librsvg2-dev \ + patchelf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-validation.yml(2 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/workflows/pr-validation.yml
[error] 42-42: trailing spaces
(trailing-spaces)
⏰ 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). (6)
- GitHub Check: Build - x86_64-unknown-linux-gnu
- GitHub Check: Build - aarch64-apple-darwin
- GitHub Check: Build - x86_64-pc-windows-msvc
- GitHub Check: Build - x86_64-apple-darwin
- GitHub Check: Validate PR
- GitHub Check: Security Scan
| - name: Run tests | ||
| run: bun test | ||
| run: npm test | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Mixed package managers may break lock-file determinism
Dependencies are installed with bun install (respecting bun.lockb), but tests run through npm.
If npm test pulls in additional dev-deps, their versions are no longer locked, and Node’s module resolution might not match Bun’s. Two quick fixes:
- Run tests with Bun as before (
bun test), or - Add a preceding
npm ciand pin apackage-lock.jsonplus a dedicated Node setup step:
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: "20"
- name: Install JS deps (npm)
run: npm ciPick one approach to keep a single source-of-truth.
🏁 Script executed:
#!/bin/bash
# Check for bun install in the workflow
grep -R "run: *bun install" -n .github/workflows/pr-validation.yml || echo "no bun install step found"
# Check for bun test in the workflow
grep -R "run: *bun test" -n .github/workflows/pr-validation.yml || echo "no bun test step found"
# Confirm npm test is present
grep -R "run: *npm test" -n .github/workflows/pr-validation.yml || echo "no npm test step found"
# Check if a package-lock.json is tracked at repo root
if [ -f package-lock.json ]; then
echo "package-lock.json exists"
else
echo "no package-lock.json"
fiLength of output: 390
Ensure a single package manager in CI to keep lock-file determinism
Your workflow currently installs deps with Bun (line 44) but runs tests with npm (line 59), and there’s no package-lock.json in the repo. This can lead to mismatches between bun.lockb and npm’s resolution. Choose one of the following:
• Switch to Bun for testing:
• Update the test step to:
diff - run: npm test + run: bun test
• Or standardize on npm:
• Commit a package-lock.json and pin your Node version
• Before npm test, install with npm ci
```yaml
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: "20"
- name: Install JS deps
run: npm ci
- name: Run tests
run: npm test
```
Locations to update:
- .github/workflows/pr-validation.yml: replace or augment the “Run tests” step at line 59
- Root of repo: add and commit
package-lock.jsonif choosing npm
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 60-60: trailing spaces
(trailing-spaces)
🤖 Prompt for AI Agents
In .github/workflows/pr-validation.yml around lines 44 to 60, the workflow
installs dependencies using Bun but runs tests with npm, causing potential
lock-file mismatches. To fix this, standardize on npm by adding a step to set up
Node with a fixed version (e.g., 20), replace the dependency installation step
with `npm ci` to ensure consistent installs, and keep the test step as `npm
test`. Also, commit a `package-lock.json` file at the root of the repo to
maintain lock-file determinism.
Summary by CodeRabbit
New Features
.langand modern JSON language files with automatic detection and format-preserving output.Improvements
Bug Fixes
Tests
Documentation
Chores