Skip to content

Commit 701691d

Browse files
sjsyrekclaude
andcommitted
fix: resolve critical cross-platform and reliability issues
Fixed two CRITICAL issues identified in code quality review: 1. Windows Path Detection Bug (Issue #4) - Added cross-platform path separator detection (/, \) - Added URL exclusion to prevent treating URLs as file paths - Fixed CLI being completely broken for Windows users - Added 7 comprehensive tests for path detection 2. Watch Service Race Condition (Issue #1) - Added isWatching flag to prevent race conditions - Prevents timer callbacks from executing after stop() called - Handles rapid start/stop cycles correctly - Added 5 comprehensive tests for race condition scenarios Test suite: 1165 tests passing (12 new tests added for these fixes) All fixes validated through TDD methodology (RED → GREEN → REFACTOR) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
1 parent 339ebf6 commit 701691d

4 files changed

Lines changed: 334 additions & 4 deletions

File tree

src/cli/commands/translate.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,29 @@ export class TranslateCommand {
115115

116116
/**
117117
* Check if input is a file path
118+
* Handles cross-platform paths (Windows backslashes and Unix forward slashes)
119+
* Excludes URLs from being treated as file paths
118120
*/
119121
private isFilePath(input: string): boolean {
120122
// Check if file exists
121123
if (fs.existsSync(input)) {
122124
return true;
123125
}
124126

125-
// If input contains file extensions, might be a file
126-
return this.fileTranslationService.isSupportedFile(input) && input.includes('/');
127+
// Don't treat URLs as file paths
128+
// Check for common URL protocols: http://, https://, ftp://, file://
129+
if (/^[a-zA-Z][a-zA-Z0-9+.-]*:\/\//.test(input)) {
130+
return false;
131+
}
132+
133+
// Check for path separators (cross-platform)
134+
// Windows: backslash (\), Unix: forward slash (/)
135+
const hasPathSep = input.includes(path.sep) ||
136+
input.includes('/') ||
137+
input.includes('\\');
138+
139+
// Must have path separator AND supported extension to be considered a file path
140+
return hasPathSep && this.fileTranslationService.isSupportedFile(input);
127141
}
128142

129143
/**

src/services/watch.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -131,12 +131,21 @@ export class WatchService {
131131

132132
/**
133133
* Handle file change event
134+
* Uses isWatching flag to prevent race conditions with stop()
134135
*/
135136
handleFileChange(filePath: string): void {
137+
// Check if watch has been started at least once
136138
if (!this.watchOptions) {
137139
throw new Error('Watch not started');
138140
}
139141

142+
// Early check: Don't schedule new timers if watch is being stopped or has stopped
143+
// This prevents race conditions where stop() is called between the check above
144+
// and scheduling the timer below
145+
if (!this.stats.isWatching) {
146+
return;
147+
}
148+
140149
// Check if file is supported
141150
if (!this.fileTranslationService.isSupportedFile(filePath)) {
142151
return;
@@ -157,8 +166,9 @@ export class WatchService {
157166
// Wrap async code to handle Promise properly (void operator tells TypeScript we intentionally ignore the Promise)
158167
void (async () => {
159168
try {
160-
// Check if watch is still active (prevent race condition with stop())
161-
if (!this.watchOptions) {
169+
// Double-check watch is still active (prevent race condition with stop())
170+
// Use isWatching flag for reliable state checking
171+
if (!this.stats.isWatching) {
162172
this.debounceTimers.delete(filePath);
163173
return;
164174
}

tests/unit/services/watch.test.ts

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -663,4 +663,157 @@ describe('WatchService', () => {
663663
expect(() => watchService.handleFileChange(testFile)).toThrow('Watch not started');
664664
});
665665
});
666+
667+
describe('race condition handling (Issue #1)', () => {
668+
it('should not execute timer callback if watch is stopped after timer is scheduled', async () => {
669+
jest.useFakeTimers();
670+
671+
const service = new WatchService(mockFileTranslationService, {
672+
debounceMs: 500,
673+
});
674+
675+
const testFile = path.join(testDir, 'test.txt');
676+
fs.writeFileSync(testFile, 'Hello');
677+
678+
const options = {
679+
targetLangs: ['es' as const],
680+
outputDir: path.join(testDir, 'output'),
681+
};
682+
683+
await service.watch(testDir, options);
684+
685+
// Schedule timer (file change event)
686+
service.handleFileChange(testFile);
687+
688+
// Stop immediately (before timer fires)
689+
await service.stop();
690+
691+
// Now advance time (timer callback should be cancelled or should check watch state)
692+
jest.advanceTimersByTime(500);
693+
await Promise.resolve();
694+
695+
// Translation should NOT occur because watch was stopped
696+
expect(mockFileTranslationService.translateFile).not.toHaveBeenCalled();
697+
698+
jest.useRealTimers();
699+
});
700+
701+
it('should not schedule new timers if watch is stopped', async () => {
702+
const testFile = path.join(testDir, 'test.txt');
703+
fs.writeFileSync(testFile, 'Hello');
704+
705+
const options = {
706+
targetLangs: ['es' as const],
707+
outputDir: path.join(testDir, 'output'),
708+
};
709+
710+
await watchService.watch(testDir, options);
711+
await watchService.stop();
712+
713+
// Try to trigger file change after stop
714+
// Should not throw, but also should not schedule translation
715+
expect(() => watchService.handleFileChange(testFile)).toThrow('Watch not started');
716+
});
717+
718+
it('should handle rapid start/stop cycles without orphaned timers', async () => {
719+
jest.useFakeTimers();
720+
721+
const service = new WatchService(mockFileTranslationService, {
722+
debounceMs: 300,
723+
});
724+
725+
const testFile = path.join(testDir, 'test.txt');
726+
fs.writeFileSync(testFile, 'Hello');
727+
728+
const options = {
729+
targetLangs: ['es' as const],
730+
outputDir: path.join(testDir, 'output'),
731+
};
732+
733+
// Rapid start/stop cycles
734+
await service.watch(testDir, options);
735+
service.handleFileChange(testFile);
736+
await service.stop();
737+
738+
await service.watch(testDir, options);
739+
service.handleFileChange(testFile);
740+
await service.stop();
741+
742+
await service.watch(testDir, options);
743+
service.handleFileChange(testFile);
744+
await service.stop();
745+
746+
// Fast-forward all timers
747+
jest.advanceTimersByTime(1000);
748+
await Promise.resolve();
749+
750+
// No translations should occur (all timers cancelled on stop)
751+
expect(mockFileTranslationService.translateFile).not.toHaveBeenCalled();
752+
753+
jest.useRealTimers();
754+
});
755+
756+
it('should check watch state inside timer callback to prevent race condition', async () => {
757+
jest.useFakeTimers();
758+
759+
const service = new WatchService(mockFileTranslationService, {
760+
debounceMs: 500,
761+
});
762+
763+
const testFile = path.join(testDir, 'test.txt');
764+
fs.writeFileSync(testFile, 'Hello');
765+
766+
mockFileTranslationService.translateFile.mockResolvedValue(undefined);
767+
768+
const options = {
769+
targetLangs: ['es' as const],
770+
outputDir: path.join(testDir, 'output'),
771+
};
772+
773+
await service.watch(testDir, options);
774+
service.handleFileChange(testFile);
775+
776+
// Fast-forward to just before timer fires
777+
jest.advanceTimersByTime(499);
778+
779+
// Stop now (timer is about to fire in 1ms)
780+
await service.stop();
781+
782+
// Fire the timer (it's already in the event loop queue)
783+
jest.advanceTimersByTime(1);
784+
await Promise.resolve();
785+
786+
// Translation should NOT occur - the timer callback should check watch state
787+
expect(mockFileTranslationService.translateFile).not.toHaveBeenCalled();
788+
789+
jest.useRealTimers();
790+
});
791+
792+
it('should not throw errors if timer callback executes after stop', async () => {
793+
jest.useFakeTimers();
794+
795+
const service = new WatchService(mockFileTranslationService, {
796+
debounceMs: 300,
797+
});
798+
799+
const testFile = path.join(testDir, 'test.txt');
800+
fs.writeFileSync(testFile, 'Hello');
801+
802+
const options = {
803+
targetLangs: ['es' as const],
804+
outputDir: path.join(testDir, 'output'),
805+
};
806+
807+
await service.watch(testDir, options);
808+
service.handleFileChange(testFile);
809+
await service.stop();
810+
811+
// Should not throw when timer fires after stop
812+
expect(() => {
813+
jest.advanceTimersByTime(300);
814+
}).not.toThrow();
815+
816+
jest.useRealTimers();
817+
});
818+
});
666819
});

tests/unit/translate-command.test.ts

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1355,4 +1355,157 @@ describe('TranslateCommand', () => {
13551355
expect(mockFs.readFileSync).toHaveBeenCalledWith('/path/to/unicode.txt', 'utf-8');
13561356
});
13571357
});
1358+
1359+
describe('isFilePath() - cross-platform path detection (Issue #4)', () => {
1360+
it('should detect Windows paths with backslashes (C:\\Users\\file.txt)', async () => {
1361+
const fs = jest.requireActual('fs');
1362+
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
1363+
1364+
// Access the private isFilePath method via translate()
1365+
// Windows path should be detected as file path
1366+
const spy = jest.spyOn(translateCommand as any, 'translateFile')
1367+
.mockResolvedValue('File translation result');
1368+
1369+
// Mock fileTranslationService.isSupportedFile to return true for .txt
1370+
const mockFileService = (translateCommand as any).fileTranslationService;
1371+
jest.spyOn(mockFileService, 'isSupportedFile').mockReturnValue(true);
1372+
1373+
await translateCommand.translate('C:\\Users\\Documents\\file.txt', {
1374+
to: 'es',
1375+
output: '/out.txt',
1376+
});
1377+
1378+
expect(spy).toHaveBeenCalledWith('C:\\Users\\Documents\\file.txt', { to: 'es', output: '/out.txt' });
1379+
spy.mockRestore();
1380+
});
1381+
1382+
it('should detect Unix paths with forward slashes (/home/user/file.txt)', async () => {
1383+
const fs = jest.requireActual('fs');
1384+
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
1385+
1386+
const spy = jest.spyOn(translateCommand as any, 'translateFile')
1387+
.mockResolvedValue('File translation result');
1388+
1389+
const mockFileService = (translateCommand as any).fileTranslationService;
1390+
jest.spyOn(mockFileService, 'isSupportedFile').mockReturnValue(true);
1391+
1392+
await translateCommand.translate('/home/user/documents/file.txt', {
1393+
to: 'es',
1394+
output: '/out.txt',
1395+
});
1396+
1397+
expect(spy).toHaveBeenCalledWith('/home/user/documents/file.txt', { to: 'es', output: '/out.txt' });
1398+
spy.mockRestore();
1399+
});
1400+
1401+
it('should NOT treat URLs as file paths (http://example.com/file.txt)', async () => {
1402+
const fs = jest.requireActual('fs');
1403+
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
1404+
1405+
const mockFileService = (translateCommand as any).fileTranslationService;
1406+
jest.spyOn(mockFileService, 'isSupportedFile').mockReturnValue(true);
1407+
1408+
(mockTranslationService.translate as jest.Mock).mockResolvedValueOnce({
1409+
text: 'Texto traducido',
1410+
});
1411+
1412+
// URL should be treated as text, not file path
1413+
await translateCommand.translate('http://example.com/file.txt', {
1414+
to: 'es',
1415+
});
1416+
1417+
// Should call translateText, NOT translateFile
1418+
expect(mockTranslationService.translate).toHaveBeenCalledWith(
1419+
'http://example.com/file.txt',
1420+
expect.any(Object),
1421+
expect.any(Object)
1422+
);
1423+
});
1424+
1425+
it('should NOT treat text containing "/" as file path', async () => {
1426+
const fs = jest.requireActual('fs');
1427+
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
1428+
1429+
const mockFileService = (translateCommand as any).fileTranslationService;
1430+
jest.spyOn(mockFileService, 'isSupportedFile').mockReturnValue(false);
1431+
1432+
(mockTranslationService.translate as jest.Mock).mockResolvedValueOnce({
1433+
text: 'Texto traducido',
1434+
});
1435+
1436+
// Text with / should be treated as text, not file path
1437+
await translateCommand.translate('Check https://example.com for details', {
1438+
to: 'es',
1439+
});
1440+
1441+
// Should call translateText, NOT translateFile
1442+
expect(mockTranslationService.translate).toHaveBeenCalledWith(
1443+
'Check https://example.com for details',
1444+
expect.any(Object),
1445+
expect.any(Object)
1446+
);
1447+
});
1448+
1449+
it('should detect relative Windows paths (folder\\file.txt)', async () => {
1450+
const fs = jest.requireActual('fs');
1451+
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
1452+
1453+
const spy = jest.spyOn(translateCommand as any, 'translateFile')
1454+
.mockResolvedValue('File translation result');
1455+
1456+
const mockFileService = (translateCommand as any).fileTranslationService;
1457+
jest.spyOn(mockFileService, 'isSupportedFile').mockReturnValue(true);
1458+
1459+
await translateCommand.translate('folder\\subfolder\\file.txt', {
1460+
to: 'es',
1461+
output: '/out.txt',
1462+
});
1463+
1464+
expect(spy).toHaveBeenCalledWith('folder\\subfolder\\file.txt', { to: 'es', output: '/out.txt' });
1465+
spy.mockRestore();
1466+
});
1467+
1468+
it('should detect relative Unix paths (folder/file.txt)', async () => {
1469+
const fs = jest.requireActual('fs');
1470+
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
1471+
1472+
const spy = jest.spyOn(translateCommand as any, 'translateFile')
1473+
.mockResolvedValue('File translation result');
1474+
1475+
const mockFileService = (translateCommand as any).fileTranslationService;
1476+
jest.spyOn(mockFileService, 'isSupportedFile').mockReturnValue(true);
1477+
1478+
await translateCommand.translate('folder/subfolder/file.txt', {
1479+
to: 'es',
1480+
output: '/out.txt',
1481+
});
1482+
1483+
expect(spy).toHaveBeenCalledWith('folder/subfolder/file.txt', { to: 'es', output: '/out.txt' });
1484+
spy.mockRestore();
1485+
});
1486+
1487+
it('should handle files with no path separator (file.txt) as text when file doesn\'t exist', async () => {
1488+
const fs = jest.requireActual('fs');
1489+
jest.spyOn(fs, 'existsSync').mockReturnValue(false);
1490+
1491+
const mockFileService = (translateCommand as any).fileTranslationService;
1492+
jest.spyOn(mockFileService, 'isSupportedFile').mockReturnValue(true);
1493+
1494+
(mockTranslationService.translate as jest.Mock).mockResolvedValueOnce({
1495+
text: 'archivo.txt',
1496+
});
1497+
1498+
// No path separator and file doesn't exist -> treat as text
1499+
await translateCommand.translate('file.txt', {
1500+
to: 'es',
1501+
});
1502+
1503+
// Should call translateText, NOT translateFile
1504+
expect(mockTranslationService.translate).toHaveBeenCalledWith(
1505+
'file.txt',
1506+
expect.any(Object),
1507+
expect.any(Object)
1508+
);
1509+
});
1510+
});
13581511
});

0 commit comments

Comments
 (0)