Optimize Translation#53
Conversation
Pull Request Test Coverage Report for Build 19223695342Details
💛 - Coveralls |
The 3-second timeout was too aggressive for the translation API, especially for longer posts. Increasing to 10 seconds provides more headroom while still preventing indefinite hangs
| const [isEnglish, translatedContent] = await translate.translate(data); | ||
|
|
||
| // Set defaults immediately - translation will happen in background | ||
| const isEnglish = true; |
There was a problem hiding this comment.
Good: Setting immediate defaults ensures posts are created quickly without waiting for the translation API. This significantly improves perceived performance.
| const pid = data.pid || await db.incrObjectField('global', 'nextPid'); | ||
| let postData = { pid, uid, tid, content, sourceContent, timestamp, isEnglish, translatedContent }; | ||
|
|
||
| // Start translation in background (non-blocking) |
There was a problem hiding this comment.
Excellent pattern: Background translation with promise chaining. Consider: What happens if a user edits the post before the translation completes? Might want to add a check to avoid overwriting user edits.
| // Update post asynchronously when translation completes | ||
| Posts.setPostFields(pid, { | ||
| isEnglish: detected, | ||
| translatedContent: translated, |
There was a problem hiding this comment.
Nice: Silent fail with error logging is appropriate here since the post is already created successfully. Consider emitting a socket event when translation completes so the UI can update without page refresh.
|
|
||
| // Set up timeout to prevent hanging | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), 10000); // 10 second timeout |
There was a problem hiding this comment.
Good improvement: 10-second timeout provides better balance than the original 3 seconds, especially for longer posts. This prevents hanging while giving the API sufficient time to respond.
| const timeoutId = setTimeout(() => controller.abort(), 10000); // 10 second timeout | ||
|
|
||
| const response = await fetch( | ||
| `${TRANSLATOR_API}/?content=${encodeURIComponent(postData.content)}`, |
There was a problem hiding this comment.
Proper URL encoding with encodeURIComponent prevents injection issues and handles special characters correctly.
| // Fallback to simple version if fetch fails | ||
| return ['is_english', postData]; | ||
| // Fallback: assume English if translation fails (timeout, network error, etc.) | ||
| if (error.name === 'AbortError') { |
There was a problem hiding this comment.
Good separation: Distinguishing timeout errors from other failures helps with debugging. Consider: You could add a retry mechanism for transient network failures (non-timeout errors).
hangyiw
left a comment
hangyiw
left a comment
There was a problem hiding this comment.
LGTM! ✅
This is a well-implemented performance optimization that:
- Makes post creation non-blocking by moving translation to background
- Includes proper timeout handling (10s) to prevent hanging
- Has graceful error handling with appropriate fallbacks
- Uses URL encoding to prevent injection issues
The commit increasing timeout to 10 seconds is a good balance. Approved!
3 participants
Improvements:
This PR impacts performance, reliability and fixes bugs.