fix: add translation on active symbol #420
Conversation
|
Claude finished @ahmed-deriv's task —— View job PR Review Complete ✅I've completed a thorough review of this PR. Here's my analysis organized by priority: 🔴 High Priority Issues1. Memory Leak: Timeout Not Cleaned Up on Component Unmount
Issue: The Current code: React.useEffect(() => {
// First time: fetch data
if (!hasFetchedRef.current) {
hasFetchedRef.current = true;
fetchChartData();
return;
}
// Language changed: re-enrich existing data after 2 second delay
if (previousLanguageRef.current !== current_language) {
// ... sets timeout ...
}
// Cleanup timeout on unmount
return () => {
if (languageChangeTimeoutRef.current) {
clearTimeout(languageChangeTimeoutRef.current);
}
};
}, [fetchChartData, current_language]);Problem: The cleanup function is only returned when Fix: React.useEffect(() => {
// First time: fetch data
if (!hasFetchedRef.current) {
hasFetchedRef.current = true;
fetchChartData();
// Still need to return cleanup for unmount case
return () => {
if (languageChangeTimeoutRef.current) {
clearTimeout(languageChangeTimeoutRef.current);
}
};
}
// Language changed: re-enrich existing data after 2 second delay
if (previousLanguageRef.current !== current_language) {
previousLanguageRef.current = current_language;
if (languageChangeTimeoutRef.current) {
clearTimeout(languageChangeTimeoutRef.current);
}
languageChangeTimeoutRef.current = setTimeout(() => {
const { rawActiveSymbols, rawTradingTimes } = rawDataRef.current;
const enrichedSymbols = enrichActiveSymbols(rawActiveSymbols, rawTradingTimes);
setChartData(prev => ({
...prev,
activeSymbols: enrichedSymbols,
}));
}, 2000);
}
// Cleanup timeout on unmount or when dependencies change
return () => {
if (languageChangeTimeoutRef.current) {
clearTimeout(languageChangeTimeoutRef.current);
}
};
}, [fetchChartData, current_language]);🟡 Medium Priority Issues2. Hard-coded 2-Second Delay Without Explanation
Issue: The 2-second delay before re-enriching symbols on language change is hard-coded with no explanation. This feels arbitrary and could impact UX. Questions:
Recommendation: // At the top of the file
const LANGUAGE_CHANGE_DEBOUNCE_MS = 2000; // Wait for translation library to fully update
// In the effect
languageChangeTimeoutRef.current = setTimeout(() => {
// ...
}, LANGUAGE_CHANGE_DEBOUNCE_MS);Add a comment explaining the rationale for the delay. 3. No Error Handling for Translation Function Failures
Issue: The Current pattern: const market_display_names: Record<string, string> = {
forex: localize('Forex'),
synthetic_index: localize('Derived'),
// ...
};Potential issue: If Recommendation: Add a safety wrapper or at least handle the fallback case explicitly: const safeLocalize = (key: string, fallback: string = key) => {
try {
const translated = localize(key);
return translated || fallback;
} catch {
return fallback;
}
};4. Exporting Previously Private Functions Changes API Surface
Issue: Three previously private functions are now exported:
Impact: This changes the module's API surface area. While this is necessary for the refactoring, ensure these are considered stable public APIs now. Recommendation: Add JSDoc comments to document these as public APIs: /**
* Returns the localized display name for a market code.
* @param market - Market code (e.g., 'forex', 'synthetic_index')
* @returns Localized market display name
* @public
*/
export const getMarketDisplayName = (market: string) => {
// ...
};5. Inefficient: Re-enriching All Symbols on Language Change
Issue: When language changes, the entire active symbols array is re-enriched. For large symbol lists (100+ symbols), this could cause performance issues. Current approach: const enrichedSymbols = enrichActiveSymbols(rawActiveSymbols, rawTradingTimes);
setChartData(prev => ({
...prev,
activeSymbols: enrichedSymbols,
}));Performance concern:
Recommendation: Consider memoizing the enrichment function or only updating the display name properties: const enrichedSymbols = React.useMemo(
() => enrichActiveSymbols(rawActiveSymbols, rawTradingTimes),
[rawActiveSymbols, rawTradingTimes, current_language]
);Or optimize 6. Type Safety: ChartDataResponse rawData Could Be More Specific
Issue: The Current: export interface ChartDataResponse {
rawData: {
activeSymbols: ActiveSymbols; // ❌ These are NOT enriched ActiveSymbols
tradingTimes: TradingTimesMap;
};
activeSymbols: ActiveSymbols; // ✅ These ARE enriched
tradingTimes: TradingTimesMap;
}Recommendation: Use a different type or export interface ChartDataResponse {
rawData: {
activeSymbols: any[]; // Raw API response, not enriched
tradingTimes: any; // Raw API response
};
activeSymbols: ActiveSymbols; // Enriched symbols
tradingTimes: TradingTimesMap;
}🟢 Low Priority / Nice to Have7. Missing Tests for New Language Change LogicIssue: The new language change detection and re-enrichment logic in Recommendation: Add tests for:
Note: I couldn't run tests as they require permissions not in my 8. Deprecated Code Should Have Removal Timeline
Issue: Recommendation: // @deprecated Use getMarketDisplayName and getSubmarketDisplayName from symbol-categories-utils.ts instead
// TODO: Remove in Q2 2025 after confirming no external usage (Ticket: JIRA-123)
export const MARKET_MAPPINGS = {9. Inconsistent Null Checking Pattern
Issue: The function checks // Line 62
if (!trading_times?.markets) {
return active_symbols;
}
// Line 70 (redundant check)
if (!trading_times.markets || !Array.isArray(trading_times.markets)) {
return active_symbols;
}Recommendation: Combine into a single check at the top: if (!trading_times?.markets || !Array.isArray(trading_times.markets)) {
return active_symbols;
}10. Magic Number: 2000ms TimeoutSee Medium Priority Issue #2 - extract to named constant. 📊 SummaryCorrectness & Edge Cases
Code Quality & Best Practices
Performance
Security
Test Coverage
🎯 Recommended Action ItemsMust Fix Before Merge:
Should Fix Before Merge: Nice to Have: 📝 Questions for Author
|
3 participants
Description
(Brief description of what this PR does)
Related Issue: Fixes #(issue)
Type of Change
Testing
Screenshots
(Add screenshots if UI changes)
Before:
After: