feat/PRO-3332/trading-view-chart #488
Conversation
📝 WalkthroughWalkthroughAdds TradingView integration backed by a new MobulaDatafeed (HTTP + WebSocket), new OHLCV RTK Query endpoints and OhlcvHistory type, TradingViewChart component, TokenGraphColumn refactor to use real-time prices, package/script and gitignore updates, and supporting type/declaration files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TokenGraphColumn
participant TradingViewChart
participant MobulaDatafeed
participant RTKQuery as RTK Query
participant REST as Mobula REST
participant WS as Mobula WS
User->>TokenGraphColumn: Mount
TokenGraphColumn->>TradingViewChart: render (onPriceUpdate callback)
TradingViewChart->>MobulaDatafeed: instantiate + getBars()
MobulaDatafeed->>RTKQuery: initiate tokenOhlcvHistory (from,to,period)
RTKQuery->>REST: POST /token/ohlcv-history
REST-->>RTKQuery: bars[]
RTKQuery-->>MobulaDatafeed: bars[]
MobulaDatafeed-->>TradingViewChart: onResult(bars)
TradingViewChart-->>TokenGraphColumn: historical bars rendered
rect rgb(200,220,255)
TradingViewChart->>MobulaDatafeed: subscribeBars()
MobulaDatafeed->>WS: connect & subscribe
WS-->>MobulaDatafeed: OPEN / messages
end
rect rgb(220,255,220)
loop realtime messages
WS->>MobulaDatafeed: {t,o,h,l,c,v}
MobulaDatafeed->>TradingViewChart: onTick / onPriceUpdate
TradingViewChart->>TokenGraphColumn: price update
TokenGraphColumn-->>User: update displayed price
end
end
User->>TradingViewChart: Unmount
TradingViewChart->>MobulaDatafeed: destroy()
MobulaDatafeed->>WS: close
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying x with
|
| Latest commit: |
431bd91
|
| Status: | 🚫 Build failed. |
Deploying pillarx-debug with
|
| Latest commit: |
431bd91
|
| Status: | 🚫 Build failed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
package.json (1)
16-16: Cross-platform compatibility issue withcp -Rcommand.The
cp -Rcommand is Unix-specific and will fail on Windows. Consider using a cross-platform approach.🔎 Proposed fix using cross-platform copy
- "copy-files": "cp -R node_modules/charting_library/charting_library public/ && cp -R node_modules/charting_library/charting_library src/", + "copy-files": "npx shx cp -R node_modules/charting_library/charting_library public/ && npx shx cp -R node_modules/charting_library/charting_library src/",Alternatively, add
shxas a devDependency, or use a Node.js script for copying.vite.config.js (1)
33-42: Commented-out code should be removed or documented.The commented-out proxy configuration for
/api/coinbaseand other server options should either be removed if no longer needed, or accompanied by a comment explaining why they're preserved.🔎 Proposed cleanup
server: { port: 3000, - // https: true, - // host: '0.0.0.0', - // proxy: { - // '/api/coinbase': { - // target: 'https://api.cdp.coinbase.com', - // changeOrigin: true, - // rewrite: (path) => path.replace(/^\/api\/coinbase/, ''), - // secure: true, - // }, - // }, },src/apps/token-atlas/components/TokenGraphColumn/test/TokenGraphColumn.test.tsx (1)
156-165: Consider usinggetByTestIdovergetElementByIdfor consistency.The test uses
document.getElementById()for price elements while usingscreen.getByTestId()elsewhere. For consistency with React Testing Library best practices, preferscreen.getByTestId()if these elements havedata-testidattributes, or add them if possible.🔎 Proposed refactor
- const priceElement = document.getElementById( - 'token-atlas-graph-column-price-today' - ); - expect(priceElement).toBeInTheDocument(); - - const percentageElement = document.getElementById( - 'token-atlas-graph-column-price-change-percentage' - ); - expect(percentageElement).toBeInTheDocument(); + const priceElement = screen.getByTestId('token-atlas-graph-column-price-today'); + expect(priceElement).toBeInTheDocument(); + + const percentageElement = screen.getByTestId('token-atlas-graph-column-price-change-percentage'); + expect(percentageElement).toBeInTheDocument(); expect(percentageElement?.textContent).toMatch(/%/);src/apps/token-atlas/api/token.ts (1)
145-217: Consider consolidating duplicate API slices.The
tokenOhlcvHistoryandmarketOhlcvHistoryslices are nearly identical, differing only inreducerPathand the APIpath. Consider consolidating them into a single API slice with two endpoints to reduce duplication.🔎 Proposed consolidation
-export const tokenOhlcvHistory = createApi({ - reducerPath: 'tokenOhlcvHistory', +export const ohlcvHistory = createApi({ + reducerPath: 'ohlcvHistory', baseQuery: fetchBaseMobula, endpoints: (builder) => ({ getTokenOhlcvHistory: builder.query< OhlcvHistory, { address: string; chainId: string; period: string; from: number; to?: number; amount?: number; } >({ query: ({ address, chainId, period, from, to, amount }) => { return { url: `?${chainIdsQuery}&testnets=${String(isTestnet)}`, method: 'POST', body: { version: '2', path: 'token/ohlcv-history', params: { address, chainId, period, from: from * 1000, to: to ? to * 1000 : undefined, amount: amount || undefined, }, }, }; }, }), - }), -}); - -export const marketOhlcvHistory = createApi({ - reducerPath: 'marketOhlcvHistory', - baseQuery: fetchBaseMobula, - endpoints: (builder) => ({ getMarketOhlcvHistory: builder.query< OhlcvHistory, { address: string; chainId: string; period: string; from: number; to?: number; amount?: number; } >({ - query: ({ address, chainId, period, from, to, amount }) => { - return { - url: `?${chainIdsQuery}&testnets=${String(isTestnet)}`, - method: 'POST', - body: { - version: '2', - path: 'market/ohlcv-history', - params: { - address, - chainId, - period, - from: from * 1000, - to: to ? to * 1000 : undefined, - amount: amount || undefined, - }, - }, - }; - }, + query: ({ address, chainId, period, from, to, amount }) => ({ + url: `?${chainIdsQuery}&testnets=${String(isTestnet)}`, + method: 'POST', + body: { + version: '2', + path: 'market/ohlcv-history', + params: { + address, + chainId, + period, + from: from * 1000, + to: to ? to * 1000 : undefined, + amount: amount || undefined, + }, + }, + }), }), }), });Then update the middleware registration and exports accordingly.
src/apps/token-atlas/components/TradingViewChart/TradingViewChart.tsx (2)
47-52:containerReadystate is unnecessary.The
useEffectwith an empty dependency array runs after the first render, at which pointchartContainerRef.currentis already assigned. This pattern doesn't properly detect when the ref becomes available; it's always available after mount. ThecontainerReadystate can be removed.🔎 Proposed simplification
- const [containerReady, setContainerReady] = useState(false); - - // ... - - // Set container ready when ref is available - useEffect(() => { - if (chartContainerRef.current) { - setContainerReady(true); - } - }, []); // In the main effect, just check the ref directly: useEffect(() => { const container = chartContainerRef.current; - if (!containerReady || !container) return () => {}; + if (!container) return () => {}; // ... - }, [chartSymbol, chartName, address, mobulaChainId, containerReady, userTimezone]); + }, [chartSymbol, chartName, address, mobulaChainId, userTimezone]);The ref is always populated after the component mounts, so
containerReadyprovides no additional gating.
104-115: Simplify native token address handling.The address logic is convoluted with multiple fallbacks. Since
addressis already computed on line 62, the fallbacks here are redundant.🔎 Proposed simplification
const datafeed = new MobulaDatafeed(chartSymbol, chartName, { - address: isNativeToken(address || '') + address: isNativeToken(address ?? '') ? '0xeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee' - : address || undefined, - chainId: mobulaChainId || undefined, + : address, + chainId: mobulaChainId, apiKey: import.meta.env.VITE_MOBULA_API_KEY, timezone: userTimezone, onPriceUpdate: (price: number) => { onPriceUpdateRef.current?.(price); }, });Since line 94 guards against
!address || !mobulaChainId, both are guaranteed to be defined at this point.src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx (1)
126-128: Consider debouncing the resize handler.Resize events fire rapidly during window resizing. While this is unlikely to cause noticeable performance issues given the simple state update, adding a debounce would reduce unnecessary re-renders.
🔎 Optional: Add debounce to resize handler
+import { debounce } from 'lodash'; // or implement a simple debounce - const handleResize = () => { - setViewportWidth(window.innerWidth); - }; // Listen to window resize events useEffect(() => { + const handleResize = debounce(() => { + setViewportWidth(window.innerWidth); + }, 150); + window.addEventListener('resize', handleResize); return () => { window.removeEventListener('resize', handleResize); + handleResize.cancel?.(); }; }, []);Also applies to: 155-161
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/apps/token-atlas/components/TokenGraphColumn/test/__snapshots__/TokenGraphColumn.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
.gitignorepackage.jsonpublic/charting_library.d.tssrc/apps/index.tssrc/apps/token-atlas/api/token.tssrc/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsxsrc/apps/token-atlas/components/TokenGraphColumn/test/TokenGraphColumn.test.tsxsrc/apps/token-atlas/components/TradingViewChart/MobulaDatafeed.tssrc/apps/token-atlas/components/TradingViewChart/TradingViewChart.tsxsrc/types/api.tsvite.config.js
💤 Files with no reviewable changes (1)
- src/apps/index.ts
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-12-03T10:01:15.801Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 471
File: src/apps/pillarx-app/components/AlgoInsightsTile/AlgoInsightsTile.tsx:36-48
Timestamp: 2025-12-03T10:01:15.801Z
Learning: In the AlgoInsightsTile component (src/apps/pillarx-app/components/AlgoInsightsTile/AlgoInsightsTile.tsx), the data is always hardcoded and guaranteed to be present with complete history arrays for all timeframes, so edge case handling for empty or sparse history data is not required.
Applied to files:
src/apps/token-atlas/components/TokenGraphColumn/test/TokenGraphColumn.test.tsxsrc/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
📚 Learning: 2025-08-20T09:14:16.888Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 374
File: src/apps/pillarx-app/index.tsx:12-12
Timestamp: 2025-08-20T09:14:16.888Z
Learning: In this codebase, Transaction Kit providers are set up at the container level (src/containers/Authorized.tsx), not at individual app component levels. App components like src/apps/pillarx-app/index.tsx are children that consume the context through the provider tree.
Applied to files:
src/apps/token-atlas/components/TokenGraphColumn/test/TokenGraphColumn.test.tsxsrc/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
📚 Learning: 2025-03-28T09:22:22.712Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 275
File: src/apps/the-exchange/components/DropdownTokensList/DropdownTokenList.tsx:180-195
Timestamp: 2025-03-28T09:22:22.712Z
Learning: In the Exchange app, `swapTokenList` and `receiveTokenList` are derived from `searchTokenResult` when search is active, so including `searchToken` in the useEffect dependency array that uses these lists would be redundant as the lists will update when search results change.
Applied to files:
src/apps/token-atlas/components/TokenGraphColumn/test/TokenGraphColumn.test.tsxsrc/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
📚 Learning: 2025-11-21T13:10:33.422Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 461
File: src/apps/pulse/components/Search/MarketList.tsx:10-15
Timestamp: 2025-11-21T13:10:33.422Z
Learning: In the Pulse app's MarketList component (src/apps/pulse/components/Search/MarketList.tsx), markets should display liquidity (not price) in the right-hand column. This is per the design specification.
Applied to files:
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
📚 Learning: 2025-04-23T15:04:20.826Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 290
File: src/apps/pillarx-app/components/TileTitle/TitleTitle.tsx:6-10
Timestamp: 2025-04-23T15:04:20.826Z
Learning: In this repository, TileTitleProps and TileTitle are different types that serve different purposes. TileTitleProps is used for the TileTitle component and has optional fields (title?, leftDecorator?, rightDecorator?), while TileTitle in api.ts has a required text field. The TileTitleProps interface aligns with the TokensMarketData.title type in api.ts which also has optional fields.
Applied to files:
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
📚 Learning: 2025-11-04T14:34:00.373Z
Learnt from: RanaBug
Repo: pillarwallet/x PR: 445
File: src/containers/Authorized.tsx:63-77
Timestamp: 2025-11-04T14:34:00.373Z
Learning: In src/containers/Authorized.tsx, the tempKit useEffect intentionally excludes chainId from its dependency array because the kit is used by useWalletModeVerification to check wallet mode across all supported chains, regardless of the currently selected chainId. The chainId parameter is only used for initial kit configuration and does not affect the multi-chain verification results.
Applied to files:
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
📚 Learning: 2025-12-11T12:40:09.964Z
Learnt from: aldin4u
Repo: pillarwallet/x PR: 478
File: src/apps/pulse/components/App/AppWrapper.tsx:22-23
Timestamp: 2025-12-11T12:40:09.964Z
Learning: In the Pulse app's AppWrapper component (src/apps/pulse/components/App/AppWrapper.tsx), when users enter via `/pulse?searching=true` from PillarX home and select a token or market, they should continue into Pulse to complete their transaction, not navigate back to home. Only when closing the search without making a selection should they return to PillarX home.
Applied to files:
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx
🧬 Code graph analysis (5)
src/apps/token-atlas/components/TradingViewChart/TradingViewChart.tsx (3)
src/types/api.ts (1)
TokenAtlasInfoData(271-307)src/apps/token-atlas/types/types.tsx (1)
SelectedTokenType(14-23)src/apps/token-atlas/components/TradingViewChart/MobulaDatafeed.ts (1)
MobulaDatafeed(99-1185)
src/apps/token-atlas/components/TokenGraphColumn/test/TokenGraphColumn.test.tsx (1)
src/apps/token-atlas/types/types.tsx (1)
SelectedTokenType(14-23)
src/apps/token-atlas/api/token.ts (3)
src/types/api.ts (1)
OhlcvHistory(550-563)src/utils/blockchain.ts (1)
isTestnet(33-39)src/store.ts (1)
addMiddleware(52-62)
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx (1)
src/apps/token-atlas/utils/converters.ts (1)
limitDigits(8-23)
src/apps/token-atlas/components/TradingViewChart/MobulaDatafeed.ts (2)
src/store.ts (1)
store(67-79)src/apps/token-atlas/api/token.ts (2)
tokenOhlcvHistory(145-180)tokenOhlcvHistory(234-234)
⏰ 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). (4)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: build
- GitHub Check: Cloudflare Pages: pillarx-debug
🔇 Additional comments (12)
package.json (1)
133-133: SSH-based git dependency may cause installation issues.The SSH URL (
git@github.com:...) requires SSH key access to the TradingView repository. This could fail in CI/CD environments or for contributors without SSH keys configured. If HTTPS with token authentication is an option, it may be more portable.Ensure that:
- CI/CD environments have SSH keys configured for accessing this private repository
- All contributors have access to the TradingView charting_library repository
- The semver tag
30.1.0exists and is stablesrc/apps/token-atlas/components/TradingViewChart/TradingViewChart.tsx (1)
180-190: LGTM on cleanup logic.The cleanup properly destroys the datafeed (which handles WebSocket cleanup) and removes the widget. The null-safety checks are appropriate.
src/apps/token-atlas/components/TokenGraphColumn/TokenGraphColumn.tsx (2)
130-152: LGTM with minor observation.The initialization logic correctly prevents overwriting
initialPriceusing the!initialPriceguard. The effects properly handle initialization from bothtokenDataInfoandtokenDataGraphsources.One note:
realTimePriceis set unconditionally in both effects (lines 133, 149), which means the second effect to run will overwrite the first. This appears intentional to keep values synchronized, but verify this is the desired behavior.
301-301: LGTM! Clean integration with TradingViewChart.The
TradingViewChartcomponent is properly wired with the memoizedhandlePriceUpdatecallback, which will update the real-time price display without causing unnecessary re-renders.src/apps/token-atlas/components/TradingViewChart/MobulaDatafeed.ts (8)
16-97: LGTM! Well-structured types and constants.The type definitions are comprehensive and handle multiple data formats from the Mobula API. Extracting magic numbers into named constants improves readability and maintainability.
245-379: LGTM! Robust timestamp validation and caching.The
getBarsimplementation handles edge cases well:
- Validates timestamps and falls back to sensible defaults
- Allows 1 hour clock skew for near-future timestamps
- Caches results to avoid redundant API calls
- Cleanly separates static bars path from network fetch
381-490: LGTM! Good integration with RTK Query.Using
store.dispatch(tokenOhlcvHistory.endpoints.getTokenOhlcvHistory.initiate(...))ensures consistency with the app's data fetching patterns and leverages RTK Query's built-in features. Error handling covers multiple response formats.
551-656: LGTM! Solid WebSocket connection management.The implementation handles connection lifecycle well:
- Resets reconnect attempts on successful connection
- Only reconnects on abnormal closes when subscriptions exist
- Delays subscription after connection to ensure stability
867-993: Bar update logic correctly implements TradingView streaming pattern.The implementation properly handles:
- Creating new bars when the time interval advances
- Updating existing bars with merged high/low values
- Using previous bar's close as the new bar's open when data is missing
- Notifying the price update callback consistently
The main concern (0 fallbacks) was noted in the previous comment.
1079-1150: LGTM! Proper subscription lifecycle management.The subscribe/unsubscribe methods correctly:
- Track subscriptions in the map
- Initialize from static bars when available
- Only connect WebSocket when needed
- Clean up WebSocket when all subscriptions are removed
492-522: LGTM! Resolution mapping is comprehensive.The mapping covers all TradingView resolution formats and includes a sensible default fallback. The method correctly handles both numeric minute resolutions and named resolutions (D, W, M variants).
1152-1184: LGTM! Comprehensive resource cleanup with proper integration.The
destroymethod properly releases all resources:
- Sends unsubscribe message before closing WebSocket
- Clears ping and reconnect timers
- Clears subscriptions and cache maps
The method is correctly called in the TradingViewChart component's useEffect cleanup function (TradingViewChart.tsx:183), which fires on component unmount, preventing memory leaks.
| # TradingView Charting Library (large files, should not be committed) | ||
| /public/charting_library | ||
| /public/datafeeds | ||
| /src/charting_library |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if charting_library package includes a datafeeds directory
fd -t d 'datafeeds' node_modules/charting_library 2>/dev/null || echo "No datafeeds directory found in charting_library package"
# Also check the package.json for the copy-files script
echo "=== Checking copy-files script in package.json ==="
rg -A 20 'copy-files' package.json || echo "No copy-files script found"Repository: pillarwallet/x
Length of output: 1147
Remove unnecessary /public/datafeeds ignore pattern.
The .gitignore includes /public/datafeeds, but there is no datafeeds directory in the charting_library package and the copy-files script does not create one. This entry should be removed unless a future version of the library includes it.
🤖 Prompt for AI Agents
In .gitignore around lines 33 to 36, the entry "/public/datafeeds" is
unnecessary because no datafeeds directory is produced by the charting_library
package or the copy-files script; remove the "/public/datafeeds" line from
.gitignore and keep the other TradingView ignore entries intact so the file only
ignores existing/expected build artifacts.
| minmov: 1, | ||
| pricescale: 100, |
There was a problem hiding this comment.
pricescale: 100 may truncate prices for low-value tokens.
A pricescale of 100 means the chart can only display 2 decimal places (price × pricescale must be an integer). Many cryptocurrencies trade at fractions of a cent (e.g., 0.00001234), which would display incorrectly as 0.00.
Consider using a higher pricescale or dynamically calculating it based on the token's price magnitude.
🔎 Suggested fix for dynamic pricescale
- pricescale: 100,
+ // Dynamic pricescale based on typical crypto precision
+ // 10^8 allows 8 decimal places for small-value tokens
+ pricescale: 100000000,Alternatively, calculate dynamically in the constructor if you have access to the token's current price:
// Calculate pricescale based on price magnitude
const calculatePricescale = (price?: number): number => {
if (!price || price >= 1) return 100; // 2 decimals for >= $1
if (price >= 0.01) return 10000; // 4 decimals
if (price >= 0.0001) return 1000000; // 6 decimals
return 100000000; // 8 decimals for very small prices
};🤖 Prompt for AI Agents
In src/apps/token-atlas/components/TradingViewChart/MobulaDatafeed.ts around
lines 147-148, the static setting pricescale: 100 forces only two decimal places
and will truncate very small token prices; change this to compute or set a
larger/dynamic pricescale based on the token's current price magnitude (or
precision metadata) so that price × pricescale is an integer without losing
significant digits — implement a helper that inspects the latest price (or a
configured precision) and returns an appropriate pricescale (e.g., larger powers
of 10 for sub-cent tokens), then replace the hardcoded 100 with the computed
value when building the chart configuration.
| // Extract OHLCV values | ||
| let open = 0; | ||
| if (item.o !== undefined) { | ||
| open = item.o; | ||
| } else if (item.open !== undefined) { | ||
| open = item.open; | ||
| } | ||
|
|
||
| let high = 0; | ||
| if (item.h !== undefined) { | ||
| high = item.h; | ||
| } else if (item.high !== undefined) { | ||
| high = item.high; | ||
| } | ||
|
|
||
| let low = 0; | ||
| if (item.l !== undefined) { | ||
| low = item.l; | ||
| } else if (item.low !== undefined) { | ||
| low = item.low; | ||
| } | ||
|
|
||
| let close = 0; | ||
| if (item.c !== undefined) { | ||
| close = item.c; | ||
| } else if (item.close !== undefined) { | ||
| close = item.close; | ||
| } | ||
|
|
||
| let volume = 0; | ||
| if (item.v !== undefined) { | ||
| volume = item.v; | ||
| } else if (item.volume !== undefined) { | ||
| volume = item.volume; | ||
| } |
There was a problem hiding this comment.
Fallback to 0 for missing OHLCV values may corrupt bar data.
When a field is missing from the WebSocket message, using 0 as a fallback can cause incorrect chart data:
- Line 961:
Math.min(lastBar.low, low)→ iflowis0, the bar's low becomes0 - Line 962:
Math.max(lastBar.high, high)→ ifhighis0, the high won't update correctly
Consider using undefined or the last bar's values as fallbacks, and skip updates when critical data is missing.
🔎 Suggested fix to preserve data integrity
// Extract OHLCV values
- let open = 0;
- if (item.o !== undefined) {
- open = item.o;
- } else if (item.open !== undefined) {
- open = item.open;
- }
+ const open = item.o ?? item.open;
+ const high = item.h ?? item.high;
+ const low = item.l ?? item.low;
+ const close = item.c ?? item.close;
+ const volume = item.v ?? item.volume;
- let high = 0;
- if (item.h !== undefined) {
- high = item.h;
- } else if (item.high !== undefined) {
- high = item.high;
- }
-
- let low = 0;
- if (item.l !== undefined) {
- low = item.l;
- } else if (item.low !== undefined) {
- low = item.low;
- }
-
- let close = 0;
- if (item.c !== undefined) {
- close = item.c;
- } else if (item.close !== undefined) {
- close = item.close;
- }
-
- let volume = 0;
- if (item.v !== undefined) {
- volume = item.v;
- } else if (item.volume !== undefined) {
- volume = item.volume;
- }
+ // Skip update if critical price data is missing
+ if (close === undefined) {
+ return;
+ }Then update the bar merge logic to handle undefined values:
const updatedBar: Bar = {
time: lastBar.time,
open: lastBar.open,
- high: Math.max(lastBar.high, high),
- low: Math.min(lastBar.low, low),
+ high: high !== undefined ? Math.max(lastBar.high, high) : lastBar.high,
+ low: low !== undefined ? Math.min(lastBar.low, low) : lastBar.low,
close,
- volume,
+ volume: volume ?? lastBar.volume,
};Committable suggestion skipped: line range outside the PR's diff.
| const numericChainId: number | undefined = useMemo(() => { | ||
| if (selectedToken?.chainId) return selectedToken.chainId; | ||
| const blockchainId = | ||
| String(selectedToken?.chainId) ?? contracts[0]?.blockchainId; | ||
| if (!blockchainId) return undefined; | ||
| const parsed = parseInt(blockchainId, 10); | ||
| return Number.isFinite(parsed) ? parsed : undefined; | ||
| }, [contracts, selectedToken?.chainId]); |
There was a problem hiding this comment.
Redundant chainId parsing logic.
Line 65 returns early if selectedToken?.chainId exists, but lines 66-67 then try to use String(selectedToken?.chainId) again as part of the fallback. This branch is dead code.
🔎 Proposed fix
const numericChainId: number | undefined = useMemo(() => {
if (selectedToken?.chainId) return selectedToken.chainId;
- const blockchainId =
- String(selectedToken?.chainId) ?? contracts[0]?.blockchainId;
+ const blockchainId = contracts[0]?.blockchainId;
if (!blockchainId) return undefined;
const parsed = parseInt(blockchainId, 10);
return Number.isFinite(parsed) ? parsed : undefined;
}, [contracts, selectedToken?.chainId]);📝 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 numericChainId: number | undefined = useMemo(() => { | |
| if (selectedToken?.chainId) return selectedToken.chainId; | |
| const blockchainId = | |
| String(selectedToken?.chainId) ?? contracts[0]?.blockchainId; | |
| if (!blockchainId) return undefined; | |
| const parsed = parseInt(blockchainId, 10); | |
| return Number.isFinite(parsed) ? parsed : undefined; | |
| }, [contracts, selectedToken?.chainId]); | |
| const numericChainId: number | undefined = useMemo(() => { | |
| if (selectedToken?.chainId) return selectedToken.chainId; | |
| const blockchainId = contracts[0]?.blockchainId; | |
| if (!blockchainId) return undefined; | |
| const parsed = parseInt(blockchainId, 10); | |
| return Number.isFinite(parsed) ? parsed : undefined; | |
| }, [contracts, selectedToken?.chainId]); |
🤖 Prompt for AI Agents
In src/apps/token-atlas/components/TradingViewChart/TradingViewChart.tsx around
lines 64 to 71, the current numericChainId useMemo contains dead code that
attempts to read String(selectedToken?.chainId) after already returning when
selectedToken?.chainId exists; replace the whole block with a clear fallback: if
selectedToken?.chainId is present return it (coerced to number if needed),
otherwise read contracts[0]?.blockchainId (as string), parse it with
parseInt(base 10) and return the parsed number only if finite, otherwise
undefined; ensure the useMemo dependency array includes contracts and
selectedToken?.chainId and tighten types so numericChainId is number |
undefined.
| export type OhlcvHistory = { | ||
| result: { | ||
| data: [ | ||
| { | ||
| t: number; | ||
| o: number; | ||
| h: number; | ||
| l: number; | ||
| c: number; | ||
| v: number; | ||
| }, | ||
| ]; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Incorrect tuple type for OHLCV data array.
The data field is typed as a single-element tuple [{ t: number; ... }] instead of an array. This will cause TypeScript to expect exactly one element, while OHLCV history typically returns multiple candles.
🔎 Proposed fix
export type OhlcvHistory = {
result: {
- data: [
- {
- t: number;
- o: number;
- h: number;
- l: number;
- c: number;
- v: number;
- },
- ];
+ data: {
+ t: number;
+ o: number;
+ h: number;
+ l: number;
+ c: number;
+ v: number;
+ }[];
};
};🤖 Prompt for AI Agents
In src/types/api.ts around lines 550 to 563, the OhlcvHistory type wrongly
declares `data` as a single-element tuple which forces exactly one candle;
change the type to an array of candle objects instead. Update the `data` field
from the tuple form to a regular array type with the same object shape (t, o, h,
l, c, v) so it accepts zero or more OHLCV entries.
github-actions
bot
requested a deployment
to
Preview (feat/PRO-3332/trading-view-chart)
github-actions
bot
requested a deployment
to
Preview (feat/PRO-3332/trading-view-chart)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @package.json:
- Line 16: The "copy-files" npm script uses Unix-only `cp -R`; replace it with a
cross-platform copier: add a dev dependency like "cpx" (or "cpy-cli") and update
the "copy-files" script to use that tool to recursively copy
node_modules/charting_library/charting_library and datafeeds into public/ and
src/ as before; ensure package.json's devDependencies include the chosen package
(e.g., "cpx": "^1.5.0" or "cpy-cli": "^5.0.0") and update the "copy-files"
script to invoke that CLI so it works on Windows and CI.
- Line 133: The package.json entry referencing "charting_library":
"https://github.com/tradingview/charting_library.git#semver:30.1.0" is invalid
because the TradingView charting_library is private/proprietary; remove this
public GitHub git dependency and replace it with an approved distribution
mechanism: either point to a licensed internal/private npm registry package or
include the library as a checked-in vendor artifact (or document steps to obtain
and install the licensed tarball). Update README/CONTRIBUTING to require a
TradingView license and installation steps, and add a startup/runtime check or
CI gate that fails if the charting library artifact is missing to avoid silent
failures; also note you should verify TradingView security advisories before
enabling the library in production.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json
⏰ 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). (4)
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: build
- GitHub Check: Cloudflare Pages: x
| "sentry:sourcemaps": "sentry-cli sourcemaps inject --org pillar-project --project x ./build && sentry-cli sourcemaps upload --org pillar-project --project x ./build", | ||
| "postinstall": "patch-package", | ||
| "postinstall": "patch-package && npm run copy-files", | ||
| "copy-files": "cp -R node_modules/charting_library/charting_library public/ && cp -R node_modules/charting_library/charting_library src/ && cp -R node_modules/charting_library/datafeeds public/", |
There was a problem hiding this comment.
Use a cross-platform file copy solution.
The cp -R command is Unix/Linux-specific and will fail on Windows, breaking the installation process for Windows developers and CI/CD environments.
🔧 Proposed fix using cpx (cross-platform)
First, add cpx as a dev dependency:
"devDependencies": {
"cpx": "^1.5.0",
...
}Then update the script:
- "copy-files": "cp -R node_modules/charting_library/charting_library public/ && cp -R node_modules/charting_library/charting_library src/ && cp -R node_modules/charting_library/datafeeds public/",
+ "copy-files": "cpx \"node_modules/charting_library/charting_library/**/*\" public/charting_library && cpx \"node_modules/charting_library/charting_library/**/*\" src/charting_library && cpx \"node_modules/charting_library/datafeeds/**/*\" public/datafeeds",Alternatively, use cpy-cli which is more modern:
"devDependencies": {
"cpy-cli": "^5.0.0",
...
}- "copy-files": "cp -R node_modules/charting_library/charting_library public/ && cp -R node_modules/charting_library/charting_library src/ && cp -R node_modules/charting_library/datafeeds public/",
+ "copy-files": "cpy node_modules/charting_library/charting_library public --parents && cpy node_modules/charting_library/charting_library src --parents && cpy node_modules/charting_library/datafeeds public --parents",Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @package.json at line 16, The "copy-files" npm script uses Unix-only `cp -R`;
replace it with a cross-platform copier: add a dev dependency like "cpx" (or
"cpy-cli") and update the "copy-files" script to use that tool to recursively
copy node_modules/charting_library/charting_library and datafeeds into public/
and src/ as before; ensure package.json's devDependencies include the chosen
package (e.g., "cpx": "^1.5.0" or "cpy-cli": "^5.0.0") and update the
"copy-files" script to invoke that CLI so it works on Windows and CI.
package.json
Outdated
| "wagmi": "2.14.16", | ||
| "webfontloader": "1.6.28" | ||
| "webfontloader": "1.6.28", | ||
| "charting_library": "https://github.com/tradingview/charting_library.git#semver:30.1.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the tag 30.1.0 exists in the TradingView charting_library repository
git ls-remote --tags https://github.com/tradingview/charting_library.git | grep "30.1.0"Repository: pillarwallet/x
Length of output: 170
🌐 Web query:
TradingView charting_library version 30.1.0 security vulnerabilities
💡 Result:
Short answer: There are no official TradingView / CVE disclosures stating that charting_library v30.1.0 itself contains a published security vulnerability. TradingView’s release notes list feature/bug fixes for v30.1.0 but do not mention a security advisory. (tradingview.com)
Related signals:
- Independent posts and blog write-ups (late‑2025) allege DOM‑XSS/Universal XSS issues in TradingView’s chart library; these are third‑party reports and not an official TradingView advisory or CVE record. Treat them as unverified until TradingView or a CVE entry confirms specifics. (blog.krybot.com)
If you want, I can:
- look for any CVE entries or vendor security advisories (I’ll search NVD, MITRE, and TradingView support/announcements), or
- check a specific deployment of charting_library (scan files you provide) for known patterns of DOM XSS. Which would you prefer?
Citations:
- 1: https://www.tradingview.com/charting-library-docs/blog/version-3010/?utm_source=openai
- 2: https://blog.krybot.com/t/tradingview-dom-xss-vulnerability-analysis/35628?utm_source=openai
🌐 Web query:
TradingView charting_library 30.1.0 release notes production
💡 Result:
TradingView Charting Library v30.1.0 — summary (released Nov 10, 2025)
Key highlights:
- New featureset always_show_study_symbol_input_values_in_legend to keep symbol-type indicator inputs visible in legends. [1]
- More flexible, event-driven drag-to-export implementation. [2]
- New getVisibleBarsRange method for precise visible-data range (handles incomplete bars and non-time-based styles). [2]
- Documentation updates and troubleshooting guides (Time & sessions, drag-to-export guide, FAQs). [1][2]
- Various bug fixes (VWAP fix, UI resize fixes, indicator/legend improvements). [1]
Check full release notes and blog walkthrough for details; you can also verify your installed library with TradingView.version(). [1][2]
Sources:
[1] TradingView Charting Library — Release notes (v30.1.0).
[2] TradingView Blog — Version 30.1.0 announcement.
🌐 Web query:
TradingView charting_library license terms
💡 Result:
Short summary — TradingView charting libraries and their licensing (with sources)
-
Lightweight Charts: open-source, Apache‑2.0 license (can be used freely). It requires showing TradingView attribution (built‑in attribution option available). [1][2]
-
Advanced Charts (often called “Charting Library” / “Advanced Charts”): proprietary; source is not publicly provided and access is granted by TradingView to companies/individuals for use in public web projects only (not for personal/hobby/test projects). You must request access/obtain a license from TradingView; examples/tutorial repos are published separately (MIT for example repos), but the main Advanced Charts library is private/proprietary. [1][3]
-
Trading Platform library: a separate proprietary product (includes trading features) with its own licensing/requirements; also provided by TradingView under license. [1]
-
Use restrictions / data rules: TradingView’s policies state market data and platform content are licensed for display‑only use and forbid non‑display uses (e.g., automated trading, re‑serving/processing data to create products that violate data provider restrictions). If you plan commercial/ non‑display uses, confirm with TradingView and data providers. [4]
Sources
- TradingView — Charting libraries overview / licensing FAQ. [TradingView Charting Library page].
- TradingView — Lightweight Charts (Apache‑2.0, attribution requirement).
- TradingView GitHub — charting‑library examples & tutorial repos (examples MIT; core library access is private/controlled).
- TradingView Policies / Terms (display‑only data restrictions).
The charting_library GitHub URL is inaccessible and requires proper TradingView licensing.
The repository https://github.com/tradingview/charting_library.git is private and cannot be accessed as a standard git dependency. Additionally, TradingView's Charting Library is a proprietary product that requires explicit authorization and licensing from TradingView—it cannot be freely used by including a GitHub reference. Ensure your project has the proper TradingView license agreement in place before using this library.
Additionally, third-party security reports (unverified) mention potential DOM-XSS vulnerabilities in charting_library. While no official CVE exists for v30.1.0, confirm with TradingView's latest security advisories and documentation before deployment.
🤖 Prompt for AI Agents
In @package.json at line 133, The package.json entry referencing
"charting_library":
"https://github.com/tradingview/charting_library.git#semver:30.1.0" is invalid
because the TradingView charting_library is private/proprietary; remove this
public GitHub git dependency and replace it with an approved distribution
mechanism: either point to a licensed internal/private npm registry package or
include the library as a checked-in vendor artifact (or document steps to obtain
and install the licensed tarball). Update README/CONTRIBUTING to require a
TradingView license and installation steps, and add a startup/runtime check or
CI gate that fails if the charting library artifact is missing to avoid silent
failures; also note you should verify TradingView security advisories before
enabling the library in production.
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Types
✏️ Tip: You can customize this high-level summary in your review settings.