Ahmed: Refactor/champion chart to Master [DO NOT MERGE] #318
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rt-champion Ahmed/feat: add document to implement smart charts champion and adaptor layer
…ahmed/feat-adaptor-bug-fixes
Ahmed/Feat: Upgrade React 17x to 18.2.0 and Resolve Bugs
Dependency ReviewThe following issues were found:
License Issuespackage-lock.json
package.json
packages/trader/package.json
OpenSSF ScorecardScorecard details
Scanned Manifest Filespackage-lock.json
package.json
packages/api-v2/package.json
packages/api/package.json
packages/components/package.json
packages/core/package.json
packages/reports/package.json
packages/shared/package.json
packages/stores/package.json
packages/trader/package.json
|
Deploying derivatives-trader with
|
| Latest commit: |
c7ee359
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://be6a9ece.derivatives-trader.pages.dev |
| Branch Preview URL: | https://refactor-champion-chart.derivatives-trader.pages.dev |
ahmed-deriv
changed the title
…rt-package fix: update smart chart package to get latest changes
…med/feat--add-markers-smart-chart
…arkers-smart-chart
fix: multipliers markers not showing after contract is ended
…act-marker feat: add contract markers for accumulator
|
Claude finished @ahmed-deriv's task —— View job Comprehensive PR Review CompleteI've conducted a thorough review of this React 18 upgrade and SmartCharts Champion migration. Unfortunately, the critical issues identified in previous reviews remain unaddressed, and this PR is still not ready for merge. 🔴 Critical Issues (Must Fix Before Merge)1. React 18 Migration Incomplete: Deprecated API Still UsedLocation: Status: ❌ STILL NOT FIXED (flagged in both Oct 24 and Oct 27 reviews) // eslint-disable-next-line react/no-deprecated
ReactDOM.render(<App root_store={root_store} />, wrapper);Problem: The entire purpose of upgrading to React 18 is defeated by continuing to use the deprecated Impact:
Required Fix: import { createRoot } from 'react-dom/client';
const root = createRoot(wrapper);
root.render(<App root_store={root_store} />);Why This Matters: This is a blocking issue. Without this change, the React 18 upgrade provides zero benefit. 2. Memory Leak + Broken Functionality: Transformation Code Commented OutLocation: Status: ❌ STILL NOT FIXED (actually worse - transformation is now disabled) const subscriptionId = transport.subscribe(apiRequest, (response: any) => {
try {
// transformations.toTQuoteFromStream(response, request.granularity);
// console.log('Stream message received:', response);
const quote = response; // ← Raw API response passed directly to chart!
callback(quote);
} catch (error) {
logger.error('Error transforming stream message:', error);
}
});Problems:
Required Fixes:
3. Silent Failures: No Loading/Error States in Chart ComponentsLocations:
Status: ❌ STILL NOT FIXED All three chart components have identical problematic patterns: React.useEffect(() => {
const fetchChartData = async () => {
try {
const data = await smartChartsAdapter.getChartData();
setChartData({ activeSymbols: data.activeSymbols, tradingTimes: data.tradingTimes });
} catch (error) {
console.error('Error fetching chart data:', error);
// No error state set, no retry mechanism, no user feedback
}
};
fetchChartData();
}, [smartChartsAdapter]);
// Later in render:
if (!chartData || !chartData.tradingTimes) return null; // ← Silent failureProblems:
User Impact: Users experience unexplained blank screens when network issues occur or API calls fail. Required Fix: const [isLoading, setIsLoading] = React.useState(true);
const [error, setError] = React.useState<Error | null>(null);
const fetchChartData = async () => {
setIsLoading(true);
setError(null);
try {
const data = await smartChartsAdapter.getChartData();
setChartData({ activeSymbols: data.activeSymbols, tradingTimes: data.tradingTimes });
} catch (error) {
setError(error);
} finally {
setIsLoading(false);
}
};
// In render:
if (isLoading) return <Loader />;
if (error) return <ErrorMessage onRetry={fetchChartData} />;4. Type Safety Violations: Unsafe Type AssertionsLocations:
Status: ❌ STILL NOT FIXED const result = await smartChartsAdapter.getQuotes({
symbol: params.symbol,
granularity: params.granularity as any, // ← Type safety defeated
count: params.count,
start: params.start,
end: params.end,
});Problem: The adapter defines Required Fix: // Add type guard
const isValidGranularity = (g: number): g is TGranularity => {
return [0, 60, 120, 180, 300, 600, 900, 1800, 3600, 7200, 14400, 28800, 86400].includes(g);
};
// Use with validation
const granularity = isValidGranularity(params.granularity)
? params.granularity
: 0; // safe fallback🟡 High Priority Issues (Should Fix Before Merge)5. Performance: Inefficient Observable ConversionLocation: const plainActiveSymbols = Array.isArray(activeSymbolsData)
? JSON.parse(JSON.stringify(activeSymbolsData)) // ← Very inefficient
: [];
const plainTradingTimes = tradingTimesData
? JSON.parse(JSON.stringify(tradingTimesData.tradingTimes)) // ← Very inefficient
: {};Problems:
Better Approach: import { toJS } from 'mobx';
const plainActiveSymbols = Array.isArray(activeSymbolsData)
? toJS(activeSymbolsData)
: [];Or use native 6. Missing Memoization in Chart ComponentsAll three chart implementations Problem: Callback functions passed to SmartChart are recreated on every render: const getQuotes: TGetQuotes = async params => { /* ... */ };
// ↑ New function reference every render
<SmartChart getQuotes={getQuotes} />
// ↑ Causes unnecessary re-renders in SmartChartRequired Fix: const getQuotes = React.useCallback<TGetQuotes>(
async params => { /* implementation */ },
[smartChartsAdapter]
);
const subscribeQuotes = React.useCallback(
(params, callback) => smartChartsAdapter.subscribeQuotes(params, callback),
[smartChartsAdapter]
);
const unsubscribeQuotes = React.useCallback(
params => smartChartsAdapter.unsubscribeQuotes(params),
[smartChartsAdapter]
);7. Silent Error Swallowing in TransformersLocation: } catch (markets_error) {
return active_symbols; // Silent failure, no logging
}Problem: Errors are swallowed without any logging, making debugging impossible. Fix: Add logging: } catch (markets_error) {
logger.error('Failed to process markets in enrichActiveSymbols', markets_error);
return active_symbols;
}8. Zero Test Coverage for New Adapter CodeStatus: ❌ STILL NOT FIXED Issue: ~1,200 lines of new adapter code across 5 files with no test coverage:
Required Tests:
Example test structure: describe('SmartCharts Adapter', () => {
describe('transformations', () => {
it('should transform ticks_history to TGetQuotesResult');
it('should handle empty/null responses gracefully');
it('should transform streaming tick to TQuote');
});
describe('subscriptions', () => {
it('should create and cleanup subscriptions');
it('should handle subscription errors');
it('should prevent duplicate subscriptions');
});
});🟠 Medium Priority Issues (Recommended Improvements)9. High Cyclomatic Complexity in Chart MarkersLocation: Issue: File grew from 304 lines to 741 lines (+437 lines, +144% growth). The Recommendation: Refactor into contract-type-specific functions: const markerCalculators = {
accumulator: calculateAccumulatorMarkers,
tick: calculateTickContractMarkers,
default: calculateNonTickContractMarkers,
};
const calculateMarker = (contract) => {
const calculator = markerCalculators[getContractCategory(contract)];
return calculator(contract);
};10. Commented Code Should Be RemovedLocations:
Action: Remove commented code. Use git history if needed to recover old code. 11. Console Logging in Production CodeLocations: Multiple adapter files Direct
Recommendation: Use a configurable logging service: const logger = createLogger({
enabled: process.env.NODE_ENV === 'development'
});12. Code Duplication: Three Identical Chart ComponentsIssue: Three nearly identical chart implementations:
All duplicate adapter initialization, data fetching, and quote wrapping logic. Recommendation: Extract shared logic into a custom hook: const useSmartChartsAdapter = (config) => {
// Shared initialization, data fetching, quote functions
return { getQuotes, subscribeQuotes, unsubscribeQuotes, chartData, isLoading, error };
};✅ Security ReviewNo critical security vulnerabilities found:
Note: 📊 React 18 Compatibility Assessment✅ Positive Changes
❌ Incomplete Migration
Recommendation: Add StrictMode in development: const isDevelopment = process.env.NODE_ENV === 'development';
const AppWrapper = isDevelopment ? React.StrictMode : React.Fragment;
const root = createRoot(wrapper);
root.render(
<AppWrapper>
<App root_store={root_store} />
</AppWrapper>
);📈 Test Coverage Assessment
|
fix price line issue
|
Claude finished @ahmed-deriv's task —— View job Comprehensive PR Review: React 18 Upgrade & SmartCharts Champion MigrationI've completed a thorough review of this PR. Unfortunately, this PR is NOT ready for merge due to multiple critical blocking issues that remain unresolved from previous reviews. 🔴 CRITICAL BLOCKING ISSUES (Must Fix)1. ❌ React 18 Migration Incomplete - Deprecated API Still UsedLocation: // eslint-disable-next-line react/no-deprecated
ReactDOM.render(<App root_store={root_store} />, wrapper);Problem: Using deprecated Impact:
Required Fix: import { createRoot } from 'react-dom/client';
const root = createRoot(wrapper);
root.render(<App root_store={root_store} />);Why This Matters: Without this change, React 18 provides ZERO benefits. This is the #1 blocking issue. 2.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request upgrades the project to React 18, updates related dependencies, and replaces the
@deriv-com/derivatives-chartspackage with@deriv-com/smartcharts-championacross all packages. It also updates Babel configurations to use the automatic JSX runtime and modernizes plugin usage. These changes aim to ensure compatibility with React 18, improve build performance, and maintain up-to-date charting functionality.React 18 upgrade and dependency updates:
reactandreact-domdependencies to version 18.2.0 in all relevantpackage.jsonfiles, ensuring the project uses React 18 throughout.Charting library migration:
Replaced all references to
@deriv-com/derivatives-chartswith@deriv-com/smartcharts-championin dependencies, build configs, and code imports, ensuring the new charting library is used project-wide.Babel configuration modernization:
Switched Babel preset configuration to use the automatic JSX runtime for React (
runtime: "automatic") in all Babel config files, improving build performance and compatibility with React 18.Updated Babel plugins to use modern
transform-*plugins instead of legacyproposal-*plugins for class properties, object rest/spread, export namespace, optional chaining, and nullish coalescing operator.Webpack and alias changes:
Code cleanup and minor fixes: