Conversation
WalkthroughAdds a new Insights app: UI primitives, many React components (cards, charts, consent flow, modals), hooks, utils, types, styles and Tailwind config, an API client for Firebase Functions, a Supabase-compatible adapter, and a top-level App coordinating consent, subscription gating, polling, sparklines and PnL aggregation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Cache as LocalStorage
participant API as FirebaseFunctions
participant Adapter as SupabaseAdapter
User->>App: open Insights app
App->>Cache: read stored consent, logo cache, cached sparklines
App->>App: evaluate consent & subscription state
alt consent missing
App->>User: show ConsentModal
User->>App: accept consent
App->>Cache: persist consent
end
App->>API: GET /insights/trading-signals
API-->>App: returns signals
App->>Adapter: optional normalize/query via supabase-style adapter
App->>App: compute timelines, feed events, PnL, identify sparklines needed
loop per ticker needing sparkline
App->>API: POST /insights/sparkline-data (ticker,start,end)
API-->>App: sparkline data
App->>Cache: save sparkline
end
loop every 30s when active
App->>API: POST /insights/update-signal-prices
API-->>App: updated prices
App->>App: recompute PnL & refresh UI
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 pillarx-debug with
|
| Latest commit: |
cd08069
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://39cd51d7.pillarx-debug.pages.dev |
| Branch Preview URL: | https://app-insights.pillarx-debug.pages.dev |
github-actions
bot
requested a deployment
to
Preview (app/insights)
Deploying x with
|
| Latest commit: |
cd08069
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a1aedacb.x-e62.pages.dev |
| Branch Preview URL: | https://app-insights.x-e62.pages.dev |
github-actions
bot
requested a deployment
to
Preview (app/insights)
github-actions
bot
requested a deployment
to
Preview (app/insights)
github-actions
bot
requested a deployment
to
Preview (app/insights)
github-actions
bot
requested a deployment
to
Preview (app/insights)
github-actions
bot
requested a deployment
to
Preview (app/insights)
github-actions
bot
requested a deployment
to
Preview (app/insights)
github-actions
bot
requested a deployment
to
Preview (app/insights)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
src/apps/insights/components/ui/carousel.tsx-97-102 (1)
97-102: Missing cleanup for "reInit" event listener.The
reInitevent listener is registered but not removed in the cleanup function, which can cause memory leaks if the carousel unmounts while the API still holds a reference.Apply this diff to remove both listeners:
return () => { + api?.off("reInit", onSelect); api?.off("select", onSelect); };src/apps/insights/utils/formatUtils.ts-5-21 (1)
5-21: Align PEPE comment with actual behavior informatPrice
normalizeSymboland the general pricing rules look fine. One small mismatch:
- The comment says: “except PEPEUSDT uses 2 decimals”.
- The code applies
toFixed(2)to any ticker whose symbol includes"PEPE"(e.g.PEPE,PEPEUSDT,PEPEUSD).Either broaden the comment (e.g. “PEPE tickers use 2 decimals”) or narrow the condition to the exact symbol(s) you intend (for example by checking a normalized symbol). Using
toFixedis consistent with the existing trade-off around float precision in this codebase. Based on learnings, this matches prior decisions.src/apps/insights/components/ui/input-otp.tsx-27-29 (1)
27-29: Add defensive check for slot access.Accessing
inputOTPContext.slots[index]without validation could cause a runtime error ifindexis out of bounds or the context is not properly initialized.>(({ index, className, ...props }, ref) => { const inputOTPContext = React.useContext(OTPInputContext); - const { char, hasFakeCaret, isActive } = inputOTPContext.slots[index]; + const slot = inputOTPContext?.slots?.[index]; + if (!slot) { + return null; + } + const { char, hasFakeCaret, isActive } = slot;src/apps/insights/components/Header/Header.tsx-84-91 (1)
84-91: Redundant calculation:closedTotalPnLis already a prop.
calculateTotalPnL(closedSignals)is called on line 86, but the computed valueclosedTotalPnLis already passed as a prop. This creates inconsistency with how Open Positions P&L usesopenTotalPnLdirectly.<KPICard title="Closed Trades P&L" - value={applyLeverage(calculateTotalPnL(closedSignals))} + value={applyLeverage(closedTotalPnL)} badge={closedSignals.length} badgeColor="slate" sparklineData={closedPnLSparklineData} onClick={() => onPnLViewClick('closed')} />If
calculateTotalPnLis intentionally needed here for a different calculation, consider removingclosedTotalPnLfrom props to avoid confusion.src/apps/insights/components/consent/useConsentValidation.ts-3-3 (1)
3-3: Unused import:CORRECT_Q2_ANSWERS.This constant is imported but never used in the hook.
-import { CORRECT_Q2_ANSWERS } from "./constants";src/apps/insights/hooks/useSparklineData.ts-37-40 (1)
37-40: Add type definition for the API response structure.The
fetchSparklineDatafunction returns an untypedanyresponse, and line 39's cast toSparklineDataPoint[]lacks validation. Define a proper response interface (e.g.,SparklineApiResponse) with thecandlesproperty typed asSparklineDataPoint[]to ensure type safety and catch potential mismatches at compile time rather than runtime.src/apps/insights/components/PnLDetailModal.tsx-481-481 (1)
481-481: Typo: Double colon::will render as text.There's an extraneous
::at the end of the className string that will be rendered as visible text in the DOM.- <DialogContent className="w-full sm:w-full max-w-[calc(100vw-2rem)] sm:max-w-4xl mx-4 sm:mx-0 h-[92vh] sm:h-auto max-h-[90vh] overflow-y-auto rounded-3xl sm:rounded-3xl p-5 sm:p-7 glass-card border-primary/20">:: + <DialogContent className="w-full sm:w-full max-w-[calc(100vw-2rem)] sm:max-w-4xl mx-4 sm:mx-0 h-[92vh] sm:h-auto max-h-[90vh] overflow-y-auto rounded-3xl sm:rounded-3xl p-5 sm:p-7 glass-card border-primary/20">src/apps/insights/components/PnLDetailModal.tsx-302-307 (1)
302-307:colSpanmismatch: Table has 7 columns but empty state spans 6.The table header defines 7 columns (Ticker, Entry, Current/Exit, P&L, TPs Hit, Status, Date), but the empty state
colSpanis set to 6, causing misalignment.- <TableCell colSpan={6} className="text-center text-muted-foreground py-8"> + <TableCell colSpan={7} className="text-center text-muted-foreground py-8">src/apps/insights/components/FeedEventCard/FeedEventCard.tsx-36-48 (1)
36-48: Missing default case ingetEventColor- returnsundefinedfor unknown event types.If an unexpected
event.typeis passed, this function returnsundefined, which could cause styling issues. Consider adding a default case.const getEventColor = () => { switch (event.type) { case 'signal_opened': return event.order_side === 'buy' ? 'border-green-500/30' : 'border-red-500/30'; case 'tp_hit': case 'completed': return 'border-green-500/30'; case 'stop_loss_hit': return 'border-red-500/30'; case 'opposite_closed': return 'border-yellow-500/30'; + default: + return 'border-slate-500/30'; } };src/apps/insights/components/consent/ConsentTabContent.tsx-86-105 (1)
86-105: Gate cooling‑off waiver error onneedsCoolingOffWaiverThe error message:
{showErrors && !validation.isCoolingOffWaiverValid && (/* ... */)}is not conditioned on
needsCoolingOffWaiver. IfneedsCoolingOffWaiverbecomesfalsewhileisCoolingOffWaiverValidis stillfalse, users could see an error for a control that is no longer rendered.Consider:
{needsCoolingOffWaiver && showErrors && !validation.isCoolingOffWaiverValid && ( /* error */ )}to keep UI and validation messaging aligned.
src/apps/insights/components/ui/breadcrumb.tsx-80-80 (1)
80-80: Typo in displayName: "BreadcrumbElipssis" should be "BreadcrumbEllipsis".-BreadcrumbEllipsis.displayName = "BreadcrumbElipssis"; +BreadcrumbEllipsis.displayName = "BreadcrumbEllipsis";src/apps/insights/components/ui/menubar.tsx-185-188 (1)
185-188: Typo:displaynameshould bedisplayName(capital N).This will cause
MenubarShortcutto not show its name correctly in React DevTools.const MenubarShortcut = ({ className, ...props }: React.HTMLAttributes<HTMLSpanElement>) => { return <span className={cn("ml-auto text-xs tracking-widest text-muted-foreground", className)} {...props} />; }; -MenubarShortcut.displayname = "MenubarShortcut"; +MenubarShortcut.displayName = "MenubarShortcut";src/apps/insights/components/ui/card.tsx-17-22 (1)
17-22: Type mismatch inCardTitleref typing.The
forwardRefgeneric specifiesHTMLParagraphElementfor the ref, but the component renders an<h3>element (which isHTMLHeadingElement). This should be consistent.-const CardTitle = React.forwardRef<HTMLParagraphElement, React.HTMLAttributes<HTMLHeadingElement>>( +const CardTitle = React.forwardRef<HTMLHeadingElement, React.HTMLAttributes<HTMLHeadingElement>>( ({ className, ...props }, ref) => ( <h3 ref={ref} className={cn("text-2xl font-semibold leading-none tracking-tight", className)} {...props} /> ), );src/apps/insights/components/ui/use-toast.ts-166-177 (1)
166-177: Incorrect dependency inuseEffectcauses unnecessary re-subscriptions.The effect depends on
[state], butstateis not used inside the effect body. This causes the listener to unsubscribe and re-subscribe on every state change, which is unnecessary and could lead to subtle bugs if the array order changes during rapid updates.React.useEffect(() => { listeners.push(setState); return () => { const index = listeners.indexOf(setState); if (index > -1) { listeners.splice(index, 1); } }; - }, [state]); + }, []);src/apps/insights/utils/signalUtils.ts-48-73 (1)
48-73: Wrap switch case declarations in blocks to prevent scope leakage.The
constdeclarations inside switch cases can be erroneously accessed by other cases. This is flagged by static analysis (Biome: noSwitchDeclarations).Apply this diff to fix the scoping issue:
case 'tp_hit': + { const tpPercent = ((event.tp_price || 0) - signal.entry_price) / signal.entry_price * 100; const lockedIn = tpPercent * 0.3333; timelineItems.push({ icon: 'Target', iconColor: 'text-emerald-400', label: `${event.tp_level?.toUpperCase()} Hit`, price: event.tp_price || 0, timestamp: event.timestamp, pnl: lockedIn, }); // Add stop loss move event if it moved if (event.moved && event.old_stop && event.new_stop) { timelineItems.push({ icon: 'RefreshCw', iconColor: 'text-green-400', label: 'Stop Loss Moved', price: event.new_stop, timestamp: event.timestamp, pnl: null, detail: `$${formatPrice(event.old_stop, signal.ticker)} → $${formatPrice(event.new_stop, signal.ticker)}` }); } break; + }src/apps/insights/utils/signalUtils.ts-75-89 (1)
75-89: Wrapstop_loss_hitcase declarations in a block.Same issue as above - the
constdeclarations need block scoping to prevent accidental access from other cases.Apply this diff:
case 'stop_loss_hit': + { // Calculate stop loss P&L based on position type const isShortSL = signal.order_side?.toLowerCase() === 'sell'; const slPercent = isShortSL ? ((signal.entry_price - (event.stop_price || latestStop)) / signal.entry_price) * 100 // SHORT : (((event.stop_price || latestStop) - signal.entry_price) / signal.entry_price) * 100; // LONG timelineItems.push({ icon: 'XCircle', iconColor: 'text-red-400', label: 'Stop Loss Hit', price: event.stop_price || latestStop, timestamp: event.timestamp, pnl: slPercent, }); break; + }src/apps/insights/components/ui/chart.tsx-210-214 (1)
210-214: Bug: Truthy check onitem.valuewill skip rendering when value is0.The condition
item.value && (...)evaluates tofalsewhenitem.valueis0, which is a valid numeric value that should be displayed in charts.Apply this diff to fix:
- {item.value && ( + {item.value !== undefined && item.value !== null && ( <span className="font-mono font-medium tabular-nums text-foreground"> {item.value.toLocaleString()} </span> )}
🧹 Nitpick comments (44)
.gitignore (1)
32-32: Reconsider the section placement for this entry.The
/src/apps/insights/_lovableentry is placed under the "Sentry Config File" section comment, but based on the directory name, this appears to be generated or third-party tooling output (likely from Lovable AI), not Sentry-specific configuration. Consider either:
- Creating a new section comment for generated/tooling artifacts (e.g.,
# Generated/tooling artifacts), or- Moving this entry under a more appropriate existing section (e.g.,
# misc).The ignore pattern itself is correct and appropriate to keep generated content out of version control.
src/apps/insights/components/ui/carousel.tsx (1)
70-81: Consider supporting ArrowUp/ArrowDown for vertical orientation.Keyboard navigation only handles ArrowLeft/ArrowRight. For vertical carousels, users would intuitively expect ArrowUp/ArrowDown to navigate slides.
const handleKeyDown = React.useCallback( (event: React.KeyboardEvent<HTMLDivElement>) => { - if (event.key === "ArrowLeft") { + const prevKey = orientation === "horizontal" ? "ArrowLeft" : "ArrowUp"; + const nextKey = orientation === "horizontal" ? "ArrowRight" : "ArrowDown"; + + if (event.key === prevKey) { event.preventDefault(); scrollPrev(); - } else if (event.key === "ArrowRight") { + } else if (event.key === nextKey) { event.preventDefault(); scrollNext(); } }, - [scrollPrev, scrollNext], + [orientation, scrollPrev, scrollNext], );src/apps/insights/tailwind.insights.config.js (1)
4-6: Simplify redundant glob pattern.The content path
**/**/*.{...}is redundant. A single**/*.{...}will recursively match all nested directories.content: [ - './src/apps/insights/**/**/*.{js,ts,jsx,tsx,html,mdx}', + './src/apps/insights/**/*.{js,ts,jsx,tsx,html,mdx}', ],src/apps/insights/utils/supabaseAdapter.ts (1)
99-101: Reminder: Address the TODO comment.The
recalculate-historical-pnlfunction is not yet implemented.Would you like me to help implement this function or open a tracking issue for it?
src/apps/insights/hooks/use-mobile.tsx (1)
5-16: Consider initializing state to avoid hydration mismatch.The initial
undefinedstate meansuseIsMobile()returnsfalsebefore the effect runs. On SSR or first client render, this can cause a layout shift when the effect updates the state. Consider initializing with a computed value or handling the undefined case explicitly.Apply this diff to initialize state immediately:
export function useIsMobile() { - const [isMobile, setIsMobile] = React.useState<boolean | undefined>(undefined); + const [isMobile, setIsMobile] = React.useState<boolean | undefined>( + () => typeof window !== 'undefined' ? window.innerWidth < MOBILE_BREAKPOINT : undefined + ); React.useEffect(() => {Alternatively, return
boolean | undefinedand let consumers handle the undefined case during initial render.src/apps/insights/components/ui/sonner.tsx (1)
6-23: Clarify toast system usage and consider theme configuration.Two observations:
The theme is hardcoded to
"dark"(Line 9). Consider making this configurable or deriving it from a theme context to support potential light mode or user preferences.This Sonner-based toaster coexists with a custom toast system in
toaster.tsx. Both export aToastercomponent, which may cause confusion about which system to use where. Consider documenting the intended usage pattern or consolidating to a single toast system.src/apps/insights/components/Filters/Filters.tsx (1)
55-59: Consider graduated risk indicators for different leverage levels.The current implementation shows a "High Risk" badge for any leverage > 1, treating 3x and 10x equally. Since 10x leverage is significantly riskier than 3x, consider showing graduated risk levels (e.g., "Moderate Risk" for 3x, "High Risk" for 5x, "Extreme Risk" for 10x) to better inform users.
src/apps/insights/components/consent/RiskTabContent.tsx (1)
3-102: React.memo is unnecessary for components without props.The component is wrapped in
React.memobut accepts no props. Memoization only prevents re-renders when props change, so it provides no benefit here. The component will still re-render whenever its parent re-renders.Apply this diff to simplify:
-export const RiskTabContent = React.memo(() => { +export const RiskTabContent = () => { return ( <div className="space-y-6"> {/* ... content ... */} </div> ); -}); - -RiskTabContent.displayName = "RiskTabContent"; +};src/apps/insights/hooks/useLogoMap.ts (2)
54-81: Batch cache saves to avoid excessive localStorage writes.
saveLogoCache(cache)is called after every individual logo fetch, causing N localStorage writes for N missing symbols. This is inefficient and could cause performance issues on devices with slower storage.Consider saving the cache once after all fetches complete:
for (const chunk of chunks) { await Promise.all( chunk.map(async (symbol) => { try { const logoUrl = await fetchLogoFromMobula(symbol); if (logoUrl) { // Update logoMap progressively setLogoMap(prev => ({ ...prev, [symbol]: logoUrl })); // Update cache cache[symbol] = { url: logoUrl, updatedAt: Date.now() }; - saveLogoCache(cache); console.log(`✓ ${symbol} logo cached:`, logoUrl); } else { console.log(`❌ No logo found for ${symbol}`); // Mark as miss in cache cache[symbol] = { miss: true, updatedAt: Date.now() }; - saveLogoCache(cache); } } catch (err) { console.error(`❌ Failed to fetch logo for ${symbol}:`, err); cache[symbol] = { miss: true, updatedAt: Date.now() }; - saveLogoCache(cache); } }) ); + // Save cache after each chunk completes + saveLogoCache(cache); }
92-93: Effect may re-run excessively ifsignalsreference changes.Using
signalsarray directly in the dependency array triggers re-runs whenever the array reference changes, even if the content is identical. Additionally, there's no cleanup to abort in-flight fetches if the component unmounts or signals change mid-fetch.Consider stabilizing the dependency and adding cleanup:
+import { useRef } from 'react'; + export const useLogoMap = (signals: TradingSignal[]) => { const [logoMap, setLogoMap] = useState<Record<string, string>>({}); + const abortControllerRef = useRef<AbortController | null>(null); useEffect(() => { if (signals.length === 0) return; + + // Abort previous fetch if still running + abortControllerRef.current?.abort(); + abortControllerRef.current = new AbortController(); const fetchLogos = async () => { // ... existing logic }; fetchLogos(); + + return () => { + abortControllerRef.current?.abort(); + }; - }, [signals]); + }, [signals.map(s => s.id).join(',')]); // Or use a memoized signals array from parentsrc/apps/insights/hooks/useTradingSignals.ts (1)
31-53: Consider gating verbose debug logs.These detailed console logs are helpful during development but will be noisy in production. Consider wrapping them in a development check or removing before release.
- console.log('🔍 [useTradingSignals] API response:', result); + if (process.env.NODE_ENV === 'development') { + console.log('🔍 [useTradingSignals] API response:', result); + }src/apps/insights/components/consent/ConsentModal.tsx (1)
18-18:isMobileis not reactive to window resize.The mobile detection is computed once at render and won't update if the user resizes their browser window. Consider using a custom hook with
useEffectand resize listener, or a library likeusehooks-ts'suseMediaQueryfor reactive behavior.+import { useState, useEffect } from 'react'; + +function useIsMobile(breakpoint = 768) { + const [isMobile, setIsMobile] = useState(() => + typeof window !== 'undefined' ? window.innerWidth < breakpoint : false + ); + + useEffect(() => { + const handler = () => setIsMobile(window.innerWidth < breakpoint); + window.addEventListener('resize', handler); + return () => window.removeEventListener('resize', handler); + }, [breakpoint]); + + return isMobile; +}Then use
const isMobile = useIsMobile();instead of the directwindow.innerWidthcheck. This also guards against SSR hydration issues.src/apps/insights/components/consent/ConsentModalLayout.tsx (1)
4-4: Unused import:TabsContent.
TabsContentis imported but not used in this file. It's used by the children passed to this layout component.-import { Tabs, TabsList, TabsTrigger, TabsContent } from "../ui/tabs"; +import { Tabs, TabsList, TabsTrigger } from "../ui/tabs";src/apps/insights/components/KPICard/KPICard.tsx (2)
57-59: Hardcoded decimal separator may not match user's locale.The
.replace('.', ',')converts the decimal point to a comma, which is correct for some European locales but incorrect for US/UK users. Consider usingIntl.NumberFormatfor locale-aware formatting:- {value >= 0 ? '+' : ''}{value.toFixed(2).replace('.', ',')}% + {value >= 0 ? '+' : ''}{new Intl.NumberFormat(undefined, { minimumFractionDigits: 2, maximumFractionDigits: 2 }).format(value)}%Or if the comma format is intentional for a specific target audience, add a comment documenting this decision.
27-31: Consider aligning badge colors with Badge component variants.The custom
badgeColorsmap duplicates styling that could potentially be added as variants to the Badge component. This would centralize badge styling and improve consistency across the app.If these colors are specific to KPICard, the current approach is acceptable. Otherwise, consider extending
badgeVariantsinbadge.tsxwithblue,violet, andslatevariants.src/apps/insights/components/Header/Header.tsx (1)
10-26: Consider grouping related props for maintainability.The component has 16 props which works but could become unwieldy. For better maintainability, consider grouping related props:
interface PnLData { floating: number; open: number; closed: number; sparklines: { overall: Array<{ value: number }>; open: Array<{ value: number }>; closed: Array<{ value: number }>; }; } interface HeaderProps { signals: { open: TradingSignal[]; closed: TradingSignal[] }; pnlData: PnLData; controls: { activeTab: TabType; onTabChange: (tab: TabType) => void; leverage: LeverageType; onLeverageChange: (leverage: LeverageType) => void; }; // ... }This is optional and can be deferred if the current structure is working well.
src/apps/insights/components/consent/useConsentValidation.ts (1)
5-8: Consider importingUseConsentValidationPropsfromtypes.ts.The AI summary indicates this interface is already defined in
types.ts. Importing it avoids duplication and keeps types centralized.-import { ConsentState, ValidationState } from "./types"; -import { CORRECT_Q2_ANSWERS } from "./constants"; - -interface UseConsentValidationProps { - userRegion?: string; - immediateAccess?: boolean; -} +import { ConsentState, ValidationState, UseConsentValidationProps } from "./types";src/apps/insights/utils/logoUtils.ts (3)
65-68: Add timeout to prevent indefinite hangs on network issues.The fetch call has no timeout, which could cause the request to hang indefinitely if the Mobula API is unresponsive.
- const response = await fetch( - `https://explorer-api.mobula.io/api/1/search?mode=og&sortBy=volume_24h&input=${symbol}` - ); + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); + + try { + const response = await fetch( + `https://explorer-api.mobula.io/api/1/search?mode=og&sortBy=volume_24h&input=${symbol}`, + { signal: controller.signal } + ); + clearTimeout(timeoutId);
77-98: Define a type for the Mobula API response token.Using
anyreduces type safety and IDE support. Consider defining an interface for the token structure.interface MobulaToken { symbol?: string; name?: string; logo?: string; market_cap?: number; liquidity?: number; volume?: number; }
16-37: Pruned cache entries are not persisted.
loadLogoCacheprunes expired entries but doesn't save the pruned cache back to localStorage. This means expired entries persist until the nextsaveLogoCachecall.// Prune expired entries const pruned: Record<string, LogoCacheEntry> = {}; Object.entries(cache).forEach(([symbol, entry]) => { const ttl = entry.miss ? MISS_TTL_MS : HIT_TTL_MS; if (now - entry.updatedAt < ttl) { pruned[symbol] = entry; } }); + // Persist pruned cache if entries were removed + if (Object.keys(pruned).length < Object.keys(cache).length) { + saveLogoCache(pruned); + } + return pruned;src/apps/insights/hooks/useSparklineData.ts (1)
50-57: Fire-and-forget async calls inforEachlose error context.Using
forEachwith an async callback means errors thrown by individualfetchSparklinecalls won't propagate to the caller. While errors are logged internally, the caller has no way to know if all fetches succeeded.If the caller needs to track completion or handle errors:
- const fetchSparklines = useCallback((signals: TradingSignal[]) => { + const fetchSparklines = useCallback(async (signals: TradingSignal[]) => { const activeSignals = signals.filter(s => s.status === 'active'); console.log(`🔄 [Sparkline] Fetching sparklines for ${activeSignals.length} active signals`); - activeSignals.forEach(signal => { - fetchSparkline(signal); - }); + await Promise.allSettled(activeSignals.map(signal => fetchSparkline(signal))); }, [fetchSparkline]);src/apps/insights/components/PnLDetailModal.tsx (1)
57-64: Consider typing theisNowproperty instead of usingas any.The type assertion on line 63 bypasses TypeScript's type checking. Consider defining a proper interface for chart data points that includes the optional
isNowproperty.interface ChartDataPoint { ts: number; value: number; label: string; isNow?: boolean; }src/apps/insights/components/SignalCard/SignalCard.tsx (2)
140-156: Trailing stop heuristic may produce false positives.The secondary check (lines 142-145) infers trailing stop based on whether
stop_losshas moved pastentry_price. While this is a reasonable heuristic whentrailing_stop_historyis unavailable, it could incorrectly flag signals as "trailing" if the initial stop loss was set beyond entry (e.g., a very tight stop on a short).Consider adding a comment documenting this heuristic behavior for maintainability.
330-337: Exit price conditions can be consolidated.The
closedandcompletedcases produce identical output. Consider combining them:{signal.status === 'stopped' || signal.stop_loss_hit ? `$${formatPrice(getEffectiveStopLoss(signal), signal.ticker)}` - : signal.status === 'closed' - ? `$${formatPrice(signal.current_price || signal.entry_price, signal.ticker)}` - : signal.status === 'completed' - ? `$${formatPrice(signal.current_price || signal.entry_price, signal.ticker)}` - : `$${formatPrice(signal.entry_price, signal.ticker)}` + : ['closed', 'completed'].includes(signal.status || '') + ? `$${formatPrice(signal.current_price || signal.entry_price, signal.ticker)}` + : `$${formatPrice(signal.entry_price, signal.ticker)}` }src/apps/insights/components/FeedEventCard/FeedEventCard.tsx (1)
84-89: Inconsistent price formatting.Other components (SignalCard, PnLDetailModal) use the shared
formatPriceutility for consistent decimal handling, but heretoFixed(2)is used directly. This could cause inconsistent formatting for low-value assets (e.g., PEPE).+import { formatPrice } from '../../utils/formatUtils'; // ... {event.details.entry_price && ( - <p>Entry: ${event.details.entry_price.toFixed(2)}</p> + <p>Entry: ${formatPrice(event.details.entry_price, event.ticker)}</p> )} {event.details.exit_price && ( - <p>Exit: ${event.details.exit_price.toFixed(2)}</p> + <p>Exit: ${formatPrice(event.details.exit_price, event.ticker)}</p> )}src/apps/insights/hooks/useSubscriptionStatus.ts (1)
31-55: Guard against overlapping fetches and stale subscription updates
fetchStatuscan be invoked both from the mount effect and the polling effect, and isasyncbut scheduled viasetInterval. With a slow network or a shorterpollIntervalMs, multiple in‑flight calls can overlap, and the last to resolve (possibly for an oldereoaAddress) wins.A lightweight pattern is to track an in‑flight request ID or an
isMounted/latestRequestIdref and ignore results that don’t match, or to skip starting a new fetch while one is active.This isn’t a blocker with the current 10s default but would make the hook more robust to future changes.
Also applies to: 68-86
src/apps/insights/components/SparklineChart.tsx (1)
59-71: Remove debugconsole.logfrom render pathThe
"Sparkline positions"console.loginsideuseMemowill fire on every data change and can be noisy in production.Recommend removing it or guarding it behind an explicit debug flag.
src/apps/insights/components/ui/alert.tsx (1)
21-41: Align forwardRef generics with actual DOM elements
AlertTitleandAlertDescriptionuseforwardRef<HTMLParagraphElement>(...)but render<h5>and<div>respectively. This makes the exposed ref type inconsistent with the underlying element:const AlertTitle = React.forwardRef< HTMLParagraphElement, // but renders <h5> React.HTMLAttributes<HTMLHeadingElement> >(...)and
const AlertDescription = React.forwardRef< HTMLParagraphElement, // but renders <div> React.HTMLAttributes<HTMLParagraphElement> >(...)For stricter typing and clearer refs, consider changing these to match the actual elements, e.g.
HTMLHeadingElementforAlertTitleandHTMLDivElementforAlertDescription.src/apps/insights/types/index.ts (1)
72-119: Document timestamp units on subscription fields
SubscriptionRecordcarries several time-like numeric fields (currentPeriodEnd,trialEnd,canceledAt,upcomingInvoiceDueDate, etc.) typed asnumber | null, but the unit (seconds vs milliseconds) isn’t specified.Since other logic (e.g.
useSubscriptionStatus) compares these directly toDate.now(), it would help future maintainers to document the intended unit in a comment or via a nominal type alias to avoid subtle expiry bugs.src/apps/insights/styles/insights.css (1)
174-181: Hardcoded hex color breaks design system consistency.The focus color
#8A77FFshould use the CSS variable for maintainability and theme consistency./* Override default focus colors */ [type='text']:focus, [type='number']:focus, [type='search']:focus, textarea:focus { - --tw-ring-color: #8A77FF; - border-color: #8A77FF; + --tw-ring-color: hsl(var(--ring)); + border-color: hsl(var(--ring)); }src/apps/insights/index.tsx (3)
199-199: Remove debug console.log statements before production.Multiple
console.logstatements (lines 199, 243-250, 258, 264) should be removed or replaced with a proper logging utility that can be disabled in production.
370-374: Replaceanywith proper type for consent payload.The
ConsentPayloadtype is imported but not used here.+import type { TradingSignal, TabType, LeverageType, PnLViewType } from './types'; +import type { ConsentPayload } from './components/consent'; - const handleConsentAccepted = (payload: any) => { + const handleConsentAccepted = (payload: ConsentPayload) => {
299-301: Empty catch blocks silently swallow errors.The empty
.catch(() => {})hides potential issues. Consider logging or handling the error.const handleRefreshSubscription = useCallback(() => { - refetchSubscription().catch(() => {}); + refetchSubscription().catch((err) => { + console.error('[insights] Failed to refresh subscription:', err); + }); }, [refetchSubscription]);src/apps/insights/lib/stopLossUtils.ts (2)
3-18: DuplicateTradingSignalinterface with looser typing.This interface duplicates the
TradingSignaltype already defined insrc/apps/insights/types/index.ts. The local version usesArray<any>fortrailing_stop_history(line 11), which loses type safety compared to the canonical type.Consider importing and extending the existing type, or using a
Pick/Partialutility type to avoid duplication and maintain type safety.+import type { TradingSignal as BaseTradingSignal } from '../types'; + +// Use a subset of TradingSignal fields needed for stop-loss utilities +type TradingSignal = Pick<BaseTradingSignal, + 'stop_loss' | 'order_side' | 'created_at' | 'closed_at' | 'status' | + 'stop_loss_hit' | 'current_price' | 'trailing_stop_history' | + 'tp1' | 'tp1_hit' | 'tp2' | 'tp2_hit' | 'tp3' | 'tp3_hit' +>; -interface TradingSignal { - stop_loss: number; - order_side?: string | null; - // ... rest of local interface -}
164-166: Synthetic timestamps in inferred timeline may confuse users.When history is missing, the code generates evenly-spaced synthetic timestamps between
created_atandclosed_at. This could be misleading in the UI if users interpret these as actual event times.Consider adding a flag (e.g.,
inferred: true) to these events so the UI can indicate they are approximate.src/apps/insights/api/insightsApi.ts (3)
17-41: Inconsistent error handling patterns across API functions.The API functions use two different patterns:
fetchSparklineData,getTradingSignals,webhookReceiver: throw errorsupdateSignalPrices,recalculateHistoricalPnL: return{ data, error }tuplesThis inconsistency makes it harder for consumers to handle errors uniformly. Consider standardizing on one approach across all functions.
Also applies to: 46-65, 70-89
121-121: Avoidanytype forsignalDataparameter.Using
anyloses type safety. Consider defining a proper type for the webhook payload.-export const webhookReceiver = async (signalData: any) => { +export const webhookReceiver = async (signalData: Record<string, unknown>) => {
17-41: Consider adding request timeouts to prevent hanging requests.The fetch calls have no timeout configuration. If the server is slow or unresponsive, requests will hang indefinitely. Consider using
AbortControllerwith a timeout.Example pattern:
const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 30000); try { const response = await fetch(url, { signal: controller.signal, ... }); // ... } finally { clearTimeout(timeoutId); }src/apps/insights/utils/signalUtils.ts (2)
131-137: RedundantisShortdeclaration shadows outer scope.
isShortis already declared at line 115 with the same value. This inner declaration shadows it and can cause confusion.Consider removing the redundant declaration:
if (tpHit === true && tpPrice && Number(tpPrice) > 0) { // Calculate TP profit based on position type - const isShort = signal.order_side?.toLowerCase() === 'sell'; const tpPercent = isShort ? ((signal.entry_price - Number(tpPrice)) / signal.entry_price) * 100 // SHORT : ((Number(tpPrice) - signal.entry_price) / signal.entry_price) * 100; // LONG
224-236: Null check may not catchundefinedvalues.The filter
s.realized_pnl_percent !== nullwon't catchundefinedvalues. Based on theTradingSignalinterface,realized_pnl_percentis optional (realized_pnl_percent?: number), so it could beundefined.Consider using a truthy check or explicit undefined handling:
const closedWithPnL = closedSignals - .filter(s => s.realized_pnl_percent !== null && s.closed_at) + .filter(s => s.realized_pnl_percent != null && s.closed_at) .sort((a, b) => new Date(a.closed_at!).getTime() - new Date(b.closed_at!).getTime());The same applies to
generateOpenPnLSparkline(line 243) andgenerateClosedPnLSparkline(line 264).src/apps/insights/components/ui/sidebar.tsx (1)
67-69: Consider SSR safety for cookie access.Direct
document.cookieaccess will throw in SSR environments. If this app may be server-rendered, wrap the cookie access in a check.// This sets the cookie to keep the sidebar state. - document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; + if (typeof document !== 'undefined') { + document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; + }src/apps/insights/components/ui/pagination.tsx (1)
24-27: Empty className string is unnecessary.The
cn("", className)can be simplified since an empty string adds no value.const PaginationItem = React.forwardRef<HTMLLIElement, React.ComponentProps<"li">>(({ className, ...props }, ref) => ( - <li ref={ref} className={cn("", className)} {...props} /> + <li ref={ref} className={className} {...props} /> ));src/apps/insights/components/ui/command.tsx (1)
24-24: Empty interface could be a type alias.The empty interface extending
DialogPropscould be simplified to a type alias, though keeping it as an interface allows for future extension.-interface CommandDialogProps extends DialogProps {} +type CommandDialogProps = DialogProps;However, keeping it as an interface is also fine if you anticipate adding command-dialog-specific props later.
src/apps/insights/components/ui/chart.tsx (1)
61-88: Acknowledged:dangerouslySetInnerHTMLusage for CSS injection.The static analysis correctly flags this. However, in this case the CSS is generated purely from developer-defined
ChartConfigkeys and color strings—not user input—so XSS risk is minimal. Consider adding a comment to clarify this is intentional and safe because the config is controlled by developers.+// ChartStyle injects CSS custom properties for chart theming. +// Safe: config keys and colors are developer-defined, not user input. const ChartStyle = ({ id, config }: { id: string; config: ChartConfig }) => {
| const calculateDistance = (from: number, to: number): string => { | ||
| const percent = ((to - from) / from) * 100; | ||
| return `${percent >= 0 ? '+' : ''}${percent.toFixed(2)}%`; | ||
| }; | ||
|
|
||
| const slDistance = calculateDistance(currentPrice, stopLoss); | ||
| const tpDistance = calculateDistance(currentPrice, nextTP); | ||
|
|
There was a problem hiding this comment.
Use orderSide to make risk/reward distances intuitive for sell orders
calculateDistance currently does:
const percent = ((to - from) / from) * 100;For buys this yields negative SL distance and positive TP distance, which is intuitive. For sells (SL above, TP below), SL shows as +X% and TP as -Y%, effectively inverting risk/reward signs.
Since you already accept orderSide, consider folding it in, e.g.:
const calculateDistance = (from: number, to: number, side: "buy" | "sell"): string => {
const raw = ((to - from) / from) * 100;
const percent = side === "sell" ? -raw : raw;
return `${percent >= 0 ? "+" : ""}${percent.toFixed(2)}%`;
};
const slDistance = calculateDistance(currentPrice, stopLoss, orderSide);
const tpDistance = calculateDistance(currentPrice, nextTP, orderSide);so that “risk” remains negative and “reward” positive for both directions.
🤖 Prompt for AI Agents
In src/apps/insights/components/SparklineChart.tsx around lines 76 to 83,
calculateDistance currently computes percent = ((to - from)/from)*100 which
causes SL/TP signs to invert for sell orders; update the function to accept
orderSide ("buy" | "sell") and negate the raw percent when side === "sell" so
that risk is always negative and reward always positive, then call it with
currentPrice, stopLoss, orderSide and currentPrice, nextTP, orderSide
respectively and format the signed percent as before.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/apps/insights/components/PnLDetailModal.tsx (1)
21-43: Fix Rules of Hooks violation caused by early return before hooks.The
if (!view) return null;at Line 22 preventsuseIsMobileand theuseMemohooks from running consistently on every render, which violates React’s Rules of Hooks and matches the Biome errors you’re seeing.Move the early return after all hook calls (you already have a guard at Line 89), and rely on the
if (!view) return [];checks inside theuseMemos for safe handling.A minimal fix is simply to remove the early return at Line 22:
-export const PnLDetailModal = ({ open, onOpenChange, view, signals }: PnLDetailModalProps) => { - if (!view) return null; - - const isMobile = useIsMobile(); +export const PnLDetailModal = ({ open, onOpenChange, view, signals }: PnLDetailModalProps) => { + const isMobile = useIsMobile();The later
if (!view) return null;(Line 89) can stay.
🧹 Nitpick comments (4)
src/apps/insights/hooks/useTradingSignals.ts (2)
18-61: Clarify or removeisInitialLoadusage insidefetchSignals.
isInitialLoadis only used insidefetchSignals, but it is not part of theuseCallbackdependency array. As a result, the callback always seesisInitialLoadastrueand callssetIsInitialLoad(false)on every successful fetch (React will short‑circuit the state update once it’s alreadyfalse, but it’s misleading and unnecessary work).If you don’t need this flag outside the hook, consider removing the
isInitialLoadstate entirely. If you do want a one‑time side effect, either:
- Track it with a
useRefthat is local tofetchSignals, or- Move the “mark initial load complete” logic into the effect that triggers the first fetch, where you can safely derive it from
signals.length/loading.
31-52: Consider gating verbose logging behind an env flag.The detailed
console.logcalls for every fetch and first signal structure are useful during development but can be noisy in production and in tests.You might want to wrap these logs in an environment check (e.g. only in development) or centralize them behind a debug flag to reduce console noise.
src/apps/insights/components/PnLDetailModal.tsx (1)
26-87: Verify 'floating' view filtering and chart semantics.For
view === 'floating':
filteredSignalsreturns onlystatus === 'active'signals (same as'open'), butchartDatafalls into the non‑openbranch, usingrealized_pnl_percentand timestamps based onclosed_at || last_price_update || created_at.This means the “Floating P&L Performance” chart is effectively plotting realized P&L over time for active positions only, excluding closed trades, which may not match how
floatingPnLis computed in the main App or user expectations for “floating”.If the intent is to reflect combined open + closed performance, you may want to:
- Include closed signals in
filteredSignalsfor'floating', or- Document that
'floating'is “realized portion of active positions only” and adjust naming accordingly.src/apps/insights/index.tsx (1)
256-268: Verify PnL formulas to avoid double-counting realized PnL for open positions.Current logic:
openTotalPnL= sum ofprofit_loss_percentfor open signals.closedTotalPnL= sum ofrealized_pnl_percentfor closed signals +realized_pnl_percentfor open signals.floatingPnL=openTotalPnL + closedTotalPnL.If
profit_loss_percentfor an open signal already represents the total PnL for that position (realized + unrealized), thenfloatingPnLis effectively:floating = (openRealized + openUnrealized) + (closedRealized + openRealized) = closedRealized + openUnrealized + 2 * openRealizedi.e., realized PnL from open signals is counted twice.
Please confirm the intended semantics:
- If
profit_loss_percentis total PnL, consider settingfloatingPnL = openTotalPnL + closedPnL(without addingopenRealizedPnLagain), or- If
profit_loss_percentis strictly unrealized, adjust names/comments to make that clear.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
src/assets/landing-fonts/Formular-Bold.woffis excluded by!**/*.woffsrc/assets/landing-fonts/Formular-Bold.woff2is excluded by!**/*.woff2src/assets/landing-fonts/Formular-Medium.woffis excluded by!**/*.woffsrc/assets/landing-fonts/Formular-Medium.woff2is excluded by!**/*.woff2src/assets/landing-fonts/Formular-Regular.woffis excluded by!**/*.woffsrc/assets/landing-fonts/Formular-Regular.woff2is excluded by!**/*.woff2
📒 Files selected for processing (11)
src/apps/insights/components/Filters/Filters.tsx(1 hunks)src/apps/insights/components/PnLDetailModal.tsx(1 hunks)src/apps/insights/components/SparklineChart.tsx(1 hunks)src/apps/insights/components/ui/form.tsx(1 hunks)src/apps/insights/components/ui/progress.tsx(1 hunks)src/apps/insights/hooks/useSubscriptionStatus.ts(1 hunks)src/apps/insights/hooks/useTradingSignals.ts(1 hunks)src/apps/insights/index.tsx(1 hunks)src/apps/insights/styles/insights.css(1 hunks)src/apps/insights/tailwind.insights.config.js(1 hunks)src/apps/insights/utils/supabaseAdapter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/apps/insights/components/ui/progress.tsx
- src/apps/insights/components/SparklineChart.tsx
- src/apps/insights/tailwind.insights.config.js
- src/apps/insights/hooks/useSubscriptionStatus.ts
- src/apps/insights/components/ui/form.tsx
- src/apps/insights/components/Filters/Filters.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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/insights/hooks/useTradingSignals.ts
📚 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/insights/index.tsx
📚 Learning: 2025-12-03T10:01:15.789Z
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.789Z
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/insights/index.tsx
🧬 Code graph analysis (3)
src/apps/insights/hooks/useTradingSignals.ts (2)
src/apps/insights/types/index.ts (1)
TradingSignal(5-35)src/apps/insights/api/insightsApi.ts (1)
getTradingSignals(46-65)
src/apps/insights/components/PnLDetailModal.tsx (4)
src/apps/insights/types/index.ts (1)
TradingSignal(5-35)src/apps/insights/hooks/use-mobile.tsx (1)
useIsMobile(5-19)src/apps/insights/lib/stopLossUtils.ts (2)
normalizeTrailingHistory(84-229)getEffectiveStopLoss(25-33)src/apps/insights/utils/formatUtils.ts (1)
formatPrice(18-21)
src/apps/insights/utils/supabaseAdapter.ts (1)
src/apps/insights/api/insightsApi.ts (3)
getTradingSignals(46-65)fetchSparklineData(17-41)updateSignalPrices(70-89)
🪛 Biome (2.1.2)
src/apps/insights/components/PnLDetailModal.tsx
[error] 24-24: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 26-26: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 42-42: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/apps/insights/utils/supabaseAdapter.ts
[error] 62-76: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
⏰ 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 (5)
src/apps/insights/styles/insights.css (3)
141-194: LGTM!Utility layer is well-structured with clear separation of concerns: glass effects, gradients, glows, and backgrounds. Naming is descriptive and follows BEM-like conventions.
1-1: Paths are correctly configured and resolve properly.The @config directive and @font-face declarations reference existing assets. All paths resolve correctly:
- Line 1:
../tailwind.insights.config.jscorrectly points tosrc/apps/insights/tailwind.insights.config.js✓- Lines 6-7, 16-17, 26-27: Font files correctly reference
src/assets/landing-fonts/with all Formular font variants (Regular, Medium, Bold) in both .woff and .woff2 formats ✓
41-104: Ensure sidebar tokens are consistently integrated and consolidate profit/loss color definitions.The sidebar-specific tokens (lines 96-103) are properly defined and actively consumed by the
sidebar.tsxcomponent and Tailwind config, confirming their usage. However, the--green-profitand--red-losstokens defined in the CSS are not referenced anywhere in the codebase; components currently hardcode equivalent HSL values inline (e.g.,text-[hsl(142,76%,58%)]for profit). To maintain design consistency and enable future theme updates, migrate KPICard and similar components to use the corresponding CSS variables instead of hardcoded values. The--destructivetoken is defined and used in UI components for alert/error states, which adequately covers error handling; no additional warning or success state tokens are required unless the product roadmap specifies distinct warning visuals.src/apps/insights/components/PnLDetailModal.tsx (1)
486-486: Remove stray::fromDialogContentto fix JSX/TSX syntax.The
DialogContentopening tag currently ends with">::", which will not compile:<DialogContent ... glass-card border-primary/20">::It should be a normal JSX tag without the trailing
:::- <DialogContent className="w-full sm:w-full max-w-[calc(100vw-2rem)] sm:max-w-4xl mx-4 sm:mx-0 h-[92vh] sm:h-auto max-h-[90vh] overflow-y-auto rounded-3xl sm:rounded-3xl p-5 sm:p-7 glass-card border-primary/20">:: + <DialogContent className="w-full sm:w-full max-w-[calc(100vw-2rem)] sm:max-w-4xl mx-4 sm:mx-0 h-[92vh] sm:h-auto max-h-[90vh] overflow-y-auto rounded-3xl sm:rounded-3xl p-5 sm:p-7 glass-card border-primary/20">src/apps/insights/index.tsx (1)
353-369: Fix stalesignalsclosure in 30s polling effect.The polling effect at lines 353-369 depends only on
[consentGiven, hasActiveSubscription]but usessignalsandfetchSparklinesinside the interval callback. Becausesignalsis not in the dependency array, the closure captures whatever valuesignalsheld when the effect first ran (typically[]before initial data load completes). The interval then continues using that stale reference every 30 seconds, sofetchSparklinesnever receives updated signals after the initial load.Add
signalsandfetchSparklinesto the dependency array and remove the eslint override:}, [consentGiven, hasActiveSubscription, signals, fetchSparklines]);This ensures the interval always operates on current signals data. Alternatively, store
signalsin auseRefto update it without recreating the effect.
github-actions
bot
requested a deployment
to
Preview (app/insights)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/apps/insights/utils/supabaseAdapter.ts (2)
101-104: TODO: Implement recalculate-historical-pnl handler.The recalculate-historical-pnl function is not yet implemented.
Would you like me to generate a placeholder implementation or open an issue to track this task?
112-120: Consider returning a proper mock subscription object.The
subscribe()method returns an empty object. If callers attempt to use subscription methods (e.g.,unsubscribe()), this could cause runtime errors.Consider returning a minimal subscription interface:
subscribe: () => ({ - // Return a mock subscription - realtime will be handled by polling + unsubscribe: () => { + // No-op: realtime handled by polling + }, }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/apps/insights/components/ui/form.tsx(1 hunks)src/apps/insights/utils/supabaseAdapter.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/apps/insights/components/ui/form.tsx (2)
src/apps/insights/lib/utils.ts (1)
cn(4-6)src/apps/insights/components/ui/label.tsx (1)
Label(17-17)
src/apps/insights/utils/supabaseAdapter.ts (1)
src/apps/insights/api/insightsApi.ts (3)
getTradingSignals(46-65)fetchSparklineData(17-41)updateSignalPrices(70-89)
⏰ 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 (3)
src/apps/insights/utils/supabaseAdapter.ts (2)
59-75: LGTM! Async Promise executor anti-pattern resolved.The refactored
runQueryis now a plain async function, eliminating the previous async Promise executor issue.
77-81: LGTM! Clean query execution flow.The
eqandexecmethods correctly return Promises fromrunQuery, avoiding the thenable anti-pattern flagged in previous reviews.src/apps/insights/components/ui/form.tsx (1)
33-57: Previous issue resolved: guards now properly placed before usage.The null checks for
fieldContext?.nameanditemContext?.idnow occur before these values are accessed on lines 46-47, fixing the critical bug from the previous review.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
src/apps/insights/index.tsx (3)
45-86: Consider extracting utility functions to a dedicated utils file.Functions like
getStoredValue,getInitialDevicePlatform,shortenAddress, andformatDateare generic utilities that would benefit from extraction tosrc/apps/insights/utils/for improved reusability, testability, and separation of concerns.
98-107: Consider extracting wallet resolution logic into a custom hook.The EOA address resolution logic (Privy user wallet, Privy wallets array, localStorage) could be extracted into a reusable hook like
useEoaAddress()for better testability and reusability across the app.Example:
// src/apps/insights/hooks/useEoaAddress.ts export const useEoaAddress = () => { const storedEoaAddress = useMemo(() => getStoredValue('EOA_ADDRESS'), []); const { user } = usePrivy(); const { wallets } = useWallets(); const privyWalletAddress = user?.wallet?.address; const fallbackWalletAddress = wallets.length > 0 ? wallets[0]?.address : undefined; return useMemo(() => { return storedEoaAddress || privyWalletAddress || fallbackWalletAddress || null; }, [storedEoaAddress, privyWalletAddress, fallbackWalletAddress]); };
545-632: Consider wrapping main content in an Error Boundary.The main content rendering doesn't have an error boundary. If any child component (Header, SignalCard, FeedEventCard, etc.) throws an error, the entire app will crash with no fallback UI.
Add an error boundary:
// src/apps/insights/components/ErrorBoundary.tsx import { Component, ReactNode } from 'react'; interface Props { children: ReactNode; } interface State { hasError: boolean; error?: Error; } export class ErrorBoundary extends Component<Props, State> { constructor(props: Props) { super(props); this.state = { hasError: false }; } static getDerivedStateFromError(error: Error) { return { hasError: true, error }; } componentDidCatch(error: Error, errorInfo: any) { console.error('Insights app error:', error, errorInfo); } render() { if (this.state.hasError) { return ( <div className="min-h-screen bg-parallax-glow flex items-center justify-center px-4"> <div className="max-w-md rounded-3xl border border-white/10 bg-white/5 p-8 text-center backdrop-blur"> <h1 className="text-2xl font-semibold text-white mb-4"> Something went wrong </h1> <p className="text-muted-foreground mb-4"> We encountered an unexpected error. Please refresh the page to try again. </p> <button onClick={() => window.location.reload()} className="rounded-2xl bg-white py-2 px-6 font-semibold text-black hover:bg-white/90" > Refresh page </button> </div> </div> ); } return this.props.children; } }Then wrap the main content:
return ( <> {consentModalElement} + <ErrorBoundary> <div className="min-h-screen bg-parallax-glow"> {/* ... rest of content ... */} </div> + </ErrorBoundary> </> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/insights/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/insights/index.tsx
📚 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/insights/index.tsx
🧬 Code graph analysis (1)
src/apps/insights/index.tsx (12)
src/utils/blockchain.ts (1)
isTestnet(33-39)src/apps/insights/types/index.ts (4)
TabType(57-57)PnLViewType(58-58)LeverageType(59-59)TradingSignal(5-35)src/apps/insights/hooks/useSubscriptionStatus.ts (1)
useSubscriptionStatus(12-146)src/apps/insights/hooks/useTradingSignals.ts (1)
useTradingSignals(13-96)src/apps/insights/hooks/useSparklineData.ts (1)
useSparklineData(9-65)src/apps/insights/utils/signalUtils.ts (1)
generateFeedEvents(110-219)src/apps/insights/api/insightsApi.ts (1)
updateSignalPrices(70-89)src/apps/insights/components/consent/ConsentModal.tsx (1)
ConsentModal(9-81)src/apps/insights/components/consent/index.ts (1)
ConsentModal(1-1)src/apps/insights/components/Header/Header.tsx (1)
Header(28-103)src/apps/insights/components/FeedEventCard/FeedEventCard.tsx (1)
FeedEventCard(16-100)src/apps/insights/components/SignalCard/SignalCard.tsx (1)
SignalCard(34-365)
⏰ 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: pillarx-debug
🔇 Additional comments (1)
src/apps/insights/index.tsx (1)
177-207: Disabled exhaustive-deps rule may cause stale closures.The effect uses
sparklineDataMap,sparklineLoading, andfetchSparklinesbut these are excluded from the dependency array via the disabled ESLint rule. While this may be intentional to avoid infinite loops, it creates a risk of stale closures if the hook implementations change.Consider one of these safer alternatives:
Option 1: Use a callback ref pattern for the effect logic:
const fetchSparklineLogicRef = useRef<(signals: TradingSignal[]) => void>(); useEffect(() => { fetchSparklineLogicRef.current = (signals) => { if (!hasActiveSubscription || signals.length === 0 || loading) return; const openSignals = signals.filter(s => s.status === 'active'); const signalsNeedingSparklines = openSignals.filter(signal => { // ... existing filter logic }); if (signalsNeedingSparklines.length > 0) { fetchSparklines(signalsNeedingSparklines); } }; }); useEffect(() => { fetchSparklineLogicRef.current?.(signals); }, [signals, loading, hasActiveSubscription]);Option 2: Ensure
fetchSparklinesfromuseSparklineDatais wrapped inuseCallbackwith stable deps (verify in that hook file).⛔ Skipped due to learnings
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.
| const STRIPE_CHECKOUT_URL_MAINNET = | ||
| import.meta.env.VITE_STRIPE_CHECKOUT_URL_MAINNET || | ||
| STRIPE_CHECKOUT_URL_TESTNET; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -50 src/apps/insights/index.tsxRepository: pillarwallet/x
Length of output: 1883
🏁 Script executed:
# Search for vite config files
fd -e "vite.config" -e ".env*"Repository: pillarwallet/x
Length of output: 40
🏁 Script executed:
# Search for environment variable validation or checks
rg "VITE_STRIPE_CHECKOUT_URL" --type ts --type tsx -A 3 -B 3Repository: pillarwallet/x
Length of output: 85
🏁 Script executed:
# Search for similar fallback patterns in the codebase
rg "import.meta.env" --type ts --type tsx | head -20Repository: pillarwallet/x
Length of output: 85
🏁 Script executed:
# Find vite config and env files
find . -maxdepth 2 -name "vite.config*" -o -name ".env*" | grep -v node_modulesRepository: pillarwallet/x
Length of output: 89
🏁 Script executed:
# Search for STRIPE env vars more broadly
rg "VITE_STRIPE_CHECKOUT_URL" -A 3 -B 3Repository: pillarwallet/x
Length of output: 758
🏁 Script executed:
# Look for build scripts and CI/CD configurations
find . -maxdepth 2 -type f \( -name "*.yml" -o -name "*.yaml" -o -name "package.json" \) | grep -v node_modules | head -10Repository: pillarwallet/x
Length of output: 95
🏁 Script executed:
cat .env.exampleRepository: pillarwallet/x
Length of output: 820
🏁 Script executed:
cat vite.config.jsRepository: pillarwallet/x
Length of output: 1121
🏁 Script executed:
cat .circleci/config.ymlRepository: pillarwallet/x
Length of output: 4971
🏁 Script executed:
cat package.json | head -50Repository: pillarwallet/x
Length of output: 2136
🏁 Script executed:
# Search for GitHub workflows
find . -path "./.github/workflows" -type f 2>/dev/null | head -5Repository: pillarwallet/x
Length of output: 40
🏁 Script executed:
# Search for any env validation in the codebase
rg "VITE_STRIPE_CHECKOUT_URL_MAINNET" -B 5 -A 5Repository: pillarwallet/x
Length of output: 779
Mainnet Stripe checkout URL lacks required environment variable validation.
The VITE_STRIPE_CHECKOUT_URL_MAINNET environment variable is not documented in .env.example and has no build-time or CI/CD validation. If this variable is missing during a production build, mainnet users will be silently directed to the testnet Stripe checkout URL without any error notification.
This is a configuration gap that could cause production payment failures. Add the environment variable to .env.example and implement build-time validation to ensure it's set before deployment:
-const STRIPE_CHECKOUT_URL_MAINNET =
- import.meta.env.VITE_STRIPE_CHECKOUT_URL_MAINNET ||
- STRIPE_CHECKOUT_URL_TESTNET;
+const STRIPE_CHECKOUT_URL_MAINNET = (() => {
+ const url = import.meta.env.VITE_STRIPE_CHECKOUT_URL_MAINNET;
+ if (!url && !isTestnet) {
+ throw new Error('VITE_STRIPE_CHECKOUT_URL_MAINNET is required for mainnet builds');
+ }
+ return url || STRIPE_CHECKOUT_URL_TESTNET;
+})();Additionally, document VITE_STRIPE_CHECKOUT_URL_MAINNET as a required environment variable in .env.example.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/apps/insights/index.tsx around lines 37-39 the code falls back to the
testnet Stripe checkout URL when VITE_STRIPE_CHECKOUT_URL_MAINNET is missing,
causing silent misrouting in production; update .env.example to include
VITE_STRIPE_CHECKOUT_URL_MAINNET with a clear "required for production" note and
add build-time validation (e.g., during app startup or CI build script) that
checks process.env.VITE_STRIPE_CHECKOUT_URL_MAINNET when NODE_ENV ===
"production" and fails the build/exit with a clear error message if it's
undefined or empty so mainnet traffic cannot be silently redirected to testnet.
| useEffect(() => { | ||
| const storedConsent = localStorage.getItem('pillarx_consent_accepted'); | ||
| if (storedConsent) { | ||
| try { | ||
| const consent = JSON.parse(storedConsent); | ||
| const consentDate = new Date(consent.timestamp); | ||
| const oneYearAgo = new Date(); | ||
| oneYearAgo.setFullYear(oneYearAgo.getFullYear() - 1); | ||
|
|
||
| if (consentDate > oneYearAgo) { | ||
| setConsentGiven(true); | ||
| } else { | ||
| setShowConsentModal(true); | ||
| } | ||
| } catch { | ||
| setShowConsentModal(true); | ||
| } | ||
| } else { | ||
| setShowConsentModal(true); | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
Add structure validation after parsing consent from localStorage.
The code assumes the parsed consent object has a timestamp property without validating its structure. If the localStorage data is corrupted or manually edited, this could fail unexpectedly.
Apply this diff to add validation:
const storedConsent = localStorage.getItem('pillarx_consent_accepted');
if (storedConsent) {
try {
const consent = JSON.parse(storedConsent);
- const consentDate = new Date(consent.timestamp);
+ if (!consent?.timestamp) {
+ setShowConsentModal(true);
+ return;
+ }
+ const consentDate = new Date(consent.timestamp);
const oneYearAgo = new Date();
oneYearAgo.setFullYear(oneYearAgo.getFullYear() - 1);
if (consentDate > oneYearAgo) {
setConsentGiven(true);
} else {
setShowConsentModal(true);
}
} catch {
setShowConsentModal(true);
}
} else {
setShowConsentModal(true);
}📝 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.
| useEffect(() => { | |
| const storedConsent = localStorage.getItem('pillarx_consent_accepted'); | |
| if (storedConsent) { | |
| try { | |
| const consent = JSON.parse(storedConsent); | |
| const consentDate = new Date(consent.timestamp); | |
| const oneYearAgo = new Date(); | |
| oneYearAgo.setFullYear(oneYearAgo.getFullYear() - 1); | |
| if (consentDate > oneYearAgo) { | |
| setConsentGiven(true); | |
| } else { | |
| setShowConsentModal(true); | |
| } | |
| } catch { | |
| setShowConsentModal(true); | |
| } | |
| } else { | |
| setShowConsentModal(true); | |
| } | |
| }, []); | |
| useEffect(() => { | |
| const storedConsent = localStorage.getItem('pillarx_consent_accepted'); | |
| if (storedConsent) { | |
| try { | |
| const consent = JSON.parse(storedConsent); | |
| if (!consent?.timestamp) { | |
| setShowConsentModal(true); | |
| return; | |
| } | |
| const consentDate = new Date(consent.timestamp); | |
| const oneYearAgo = new Date(); | |
| oneYearAgo.setFullYear(oneYearAgo.getFullYear() - 1); | |
| if (consentDate > oneYearAgo) { | |
| setConsentGiven(true); | |
| } else { | |
| setShowConsentModal(true); | |
| } | |
| } catch { | |
| setShowConsentModal(true); | |
| } | |
| } else { | |
| setShowConsentModal(true); | |
| } | |
| }, []); |
🤖 Prompt for AI Agents
In src/apps/insights/index.tsx around lines 150 to 170, after parsing
storedConsent you must validate the parsed object has a timestamp property and
that it represents a valid date before using it; update the try block to check
that consent is an object, consent.timestamp exists and is a string/number,
create a Date and verify !isNaN(date.getTime()) (or similar) before comparing to
oneYearAgo, and if any check fails fall back to setShowConsentModal(true) so
corrupted or malformed localStorage entries don’t cause incorrect behavior.
| const openTotalPnL = useMemo(() => { | ||
| const value = calculateOpenPnL(); | ||
| console.log(`💰 [PnL] openTotalPnL: ${value}% (${openSignals.length} signals)`); | ||
| if (openSignals.length > 0) { | ||
| console.log('📊 [PnL] Open signals breakdown:', openSignals.map(s => ({ | ||
| ticker: s.ticker, | ||
| profit_loss_percent: s.profit_loss_percent, | ||
| realized_pnl_percent: s.realized_pnl_percent, | ||
| }))); | ||
| } | ||
| return value; | ||
| }, [openSignals]); | ||
|
|
||
| const closedTotalPnL = useMemo(() => { | ||
| const closedPnL = calculateTotalPnL(closedSignals); | ||
| const openRealizedPnL = calculateOpenRealizedPnL(); | ||
| const value = closedPnL + openRealizedPnL; | ||
| console.log(`💰 [PnL] closedTotalPnL: ${value}% (closed: ${closedPnL}%, open realized: ${openRealizedPnL}%)`); | ||
| return value; | ||
| }, [closedSignals, openSignals]); | ||
|
|
||
| const floatingPnL = useMemo(() => { | ||
| const value = openTotalPnL + closedTotalPnL; | ||
| console.log(`💰 [PnL] floatingPnL: ${value}% (open: ${openTotalPnL}%, closed: ${closedTotalPnL}%)`); | ||
| return value; | ||
| }, [openTotalPnL, closedTotalPnL]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove or guard console.log statements for production.
The PnL calculation logging (lines 245-252, 260, 266) should be removed or wrapped in a debug flag to avoid cluttering the console in production and exposing calculation logic to end users.
Apply this diff to remove the logs:
const openTotalPnL = useMemo(() => {
const value = calculateOpenPnL();
- console.log(`💰 [PnL] openTotalPnL: ${value}% (${openSignals.length} signals)`);
- if (openSignals.length > 0) {
- console.log('📊 [PnL] Open signals breakdown:', openSignals.map(s => ({
- ticker: s.ticker,
- profit_loss_percent: s.profit_loss_percent,
- realized_pnl_percent: s.realized_pnl_percent,
- })));
- }
return value;
}, [openSignals]);
const closedTotalPnL = useMemo(() => {
const closedPnL = calculateTotalPnL(closedSignals);
const openRealizedPnL = calculateOpenRealizedPnL();
const value = closedPnL + openRealizedPnL;
- console.log(`💰 [PnL] closedTotalPnL: ${value}% (closed: ${closedPnL}%, open realized: ${openRealizedPnL}%)`);
return value;
}, [closedSignals, openSignals]);
const floatingPnL = useMemo(() => {
const value = openTotalPnL + closedTotalPnL;
- console.log(`💰 [PnL] floatingPnL: ${value}% (open: ${openTotalPnL}%, closed: ${closedTotalPnL}%)`);
return value;
}, [openTotalPnL, closedTotalPnL]);Alternatively, wrap them in a debug flag:
const DEBUG = import.meta.env.DEV;
if (DEBUG) console.log(...);🤖 Prompt for AI Agents
In src/apps/insights/index.tsx around lines 243 to 268, there are several
console.log statements used to debug PnL calculations; remove these logs for
production or guard them behind a debug flag (e.g. const DEBUG =
Boolean(import.meta.env.DEV) at module scope) and wrap each console.log with if
(DEBUG) { ... } so logging only occurs in dev; ensure the DEBUG variable is
defined once in the module and used consistently for all PnL logs.
|
|
||
| const handleSubscribeClick = useCallback(() => { | ||
| if (!eoaAddress) { | ||
| alert('No wallet detected. Please re-open PillarX from the mobile app.'); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace alert() calls with toast notifications or modals.
Multiple alert() calls (lines 307, 323, 348, 381) provide a poor user experience and look unprofessional. Consider using a toast notification library (e.g., sonner, react-hot-toast) or modal dialogs for a more modern UX.
Example with a toast library:
import { toast } from 'sonner';
// Instead of:
alert('No wallet detected...');
// Use:
toast.error('No wallet detected. Please re-open PillarX from the mobile app.');Also applies to: 323-323, 348-350, 381-381
🤖 Prompt for AI Agents
In src/apps/insights/index.tsx around lines 307, 323, 348-350 and 381, replace
the use of synchronous browser alert() calls with non-blocking toast
notifications or modal dialogs; remove alert(...) calls and instead import and
use a toast library (e.g., sonner or react-hot-toast) to show error/info toasts
(e.g., toast.error(...) for errors) or open a reusable modal component where
appropriate, ensuring the messages remain the same and that the toast/modal UI
is styled consistently with the app and triggered in the same control flow
contexts as the original alerts.
| const checkoutUrl = `${STRIPE_CHECKOUT_URL}?client_reference_id=${encodeURIComponent( | ||
| eoaAddress | ||
| )}`; |
There was a problem hiding this comment.
Privacy concern: wallet address exposed in URL query parameter.
Passing the wallet address as a URL query parameter means it will be logged by Stripe's servers, proxies, browser history, and potentially leaked via Referer headers. While wallet addresses are public on-chain, this creates an unnecessary privacy vector linking users to their subscription actions.
Consider alternatives:
- Pass an opaque session ID in the URL and resolve to wallet address server-side
- Use Stripe's Customer Portal with pre-authenticated sessions
- If client_reference_id must be in URL, document the privacy implications in your privacy policy
// Better approach: use session-based checkout
const session = await createCheckoutSession({ walletAddress: eoaAddress });
const checkoutUrl = `${STRIPE_CHECKOUT_URL}?session_id=${session.id}`;🤖 Prompt for AI Agents
In src/apps/insights/index.tsx around lines 315-317, the code places the wallet
address into the checkout URL query (client_reference_id) which leaks the
address to Stripe, proxies, browser history and Referer headers; instead create
a server-side opaque checkout/session object that stores the wallet address and
return only the session id to the client (or use Stripe Customer Portal
pre-authenticated sessions), then build the checkout URL with that session_id;
if you must keep client_reference_id, document the privacy implications in the
privacy policy and avoid exposing the raw address in URLs or referer-prone
places.
| // Poll for price updates every 30 seconds | ||
| useEffect(() => { | ||
| if (!consentGiven || !hasActiveSubscription) return; | ||
|
|
||
| handleUpdatePrices(); | ||
| const interval = setInterval(() => { | ||
| handleUpdatePrices(); | ||
| // Refresh sparklines for open signals | ||
| const currentOpenSignals = signals.filter(s => s.status === 'active'); | ||
| if (currentOpenSignals.length > 0) { | ||
| fetchSparklines(currentOpenSignals); | ||
| } | ||
| }, 30000); | ||
|
|
||
| return () => clearInterval(interval); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [consentGiven, hasActiveSubscription]); |
There was a problem hiding this comment.
Disabled exhaustive-deps may cause stale closures in polling effect.
The effect uses handleUpdatePrices, signals, and fetchSparklines but excludes them from dependencies. Since handleUpdatePrices is defined inline (lines 287-299), it's recreated on every render, and the effect may reference stale versions.
Apply this diff to fix:
+const handleUpdatePrices = useCallback(async () => {
+ setUpdating(true);
+ try {
+ const result = await updateSignalPrices();
+ if (result.error) {
+ console.error('Error updating prices:', result.error);
+ }
+ } catch (error) {
+ console.error('Error calling update function:', error);
+ } finally {
+ setUpdating(false);
+ }
+}, []);
// Poll for price updates every 30 seconds
useEffect(() => {
if (!consentGiven || !hasActiveSubscription) return;
handleUpdatePrices();
const interval = setInterval(() => {
handleUpdatePrices();
// Refresh sparklines for open signals
const currentOpenSignals = signals.filter(s => s.status === 'active');
if (currentOpenSignals.length > 0) {
fetchSparklines(currentOpenSignals);
}
}, 30000);
return () => clearInterval(interval);
- // eslint-disable-next-line react-hooks/exhaustive-deps
-}, [consentGiven, hasActiveSubscription]);
+}, [consentGiven, hasActiveSubscription, handleUpdatePrices, signals, fetchSparklines]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/apps/insights/index.tsx around lines 355-371, the polling effect closes
over inline handleUpdatePrices, fetchSparklines and signals which can become
stale; to fix, memoize handleUpdatePrices and fetchSparklines with useCallback
(listing their real dependencies) so they are stable, replace direct use of
signals inside the interval with a signalsRef (useRef) that you update whenever
signals change and read signalsRef.current inside the setInterval, and include
the stable callbacks and consentGiven/hasActiveSubscription in the effect
dependency array so the interval uses current logic while avoiding unnecessary
resets.
| }, [consentGiven, hasActiveSubscription]); | ||
|
|
||
| // Consent handlers | ||
| const handleConsentAccepted = (payload: any) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use proper TypeScript type instead of any for consent payload.
The payload parameter should be typed as ConsentPayload (imported from consent types) instead of any for better type safety.
-const handleConsentAccepted = (payload: any) => {
+const handleConsentAccepted = (payload: ConsentPayload) => {
localStorage.setItem('pillarx_consent_accepted', JSON.stringify(payload));
setConsentGiven(true);
setShowConsentModal(false);
};Make sure to import the type:
// Types
-import type { TradingSignal, TabType, LeverageType, PnLViewType } from './types';
+import type { TradingSignal, TabType, LeverageType, PnLViewType, ConsentPayload } from './types';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/apps/insights/index.tsx around line 374, the handler signature uses
payload: any; change the parameter type to ConsentPayload and import that type
from the consent types module where ConsentPayload is exported (e.g., add an
import for ConsentPayload at the top of the file). Update the function signature
to use the ConsentPayload type and adjust any internal usages if needed to
satisfy the type (narrow properties or add guards) so the handler is fully
typed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/apps/insights/index.tsx (6)
34-43: Environment variable validation concern already raised.The fallback pattern for
VITE_STRIPE_CHECKOUT_URL_MAINNETsilently falling back to testnet was flagged in a previous review. Please address that feedback to prevent production payment routing issues.
149-170: Consent parsing validation concern already raised.The missing validation of
consent.timestampafter JSON parsing was flagged in a previous review. Please add a guard to check thatconsent?.timestampexists before using it.
243-268: Console.log removal concern already raised.The debug logging in PnL calculations was flagged in a previous review. Please remove these or wrap them in a
DEBUGflag.
305-360: Previous concerns still apply.The use of
alert()/window.confirm()(lines 307, 323, 355-357) and the wallet address in URL query parameter (lines 315-317) were flagged in previous reviews. Please address those concerns.The popup security handling at line 346 (
popup.opener = null) is a good practice to prevent reverse tabnapping.
362-378: Stale closure concern already raised.The disabled exhaustive-deps and potential stale closures were flagged in a previous review. Please memoize
handleUpdatePriceswithuseCallbackand properly include dependencies to avoid referencing stale function versions.
380-389: Previous typing and alert concerns still apply.The
anytype forpayloadparameter (line 381) and thealert()usage (line 388) were flagged in previous reviews. Please type the payload properly and replace alerts with toast notifications.
🧹 Nitpick comments (2)
src/apps/insights/index.tsx (2)
200-202: Remove debug console.log for production.This console.log statement should be removed or wrapped in a debug flag, consistent with the approach needed for the PnL calculation logs.
if (signalsNeedingSparklines.length > 0) { - console.log(`📊 [Sparkline] Initial fetch for ${signalsNeedingSparklines.length} new signals`); fetchSparklines(signalsNeedingSparklines); }
128-128: Consider removing unusedsetSignalsdestructuring.
setSignalsis destructured fromuseTradingSignalsbut not used in this component. If it's not needed, remove it to avoid confusion.- const { signals, loading, setSignals } = useTradingSignals({ + const { signals, loading } = useTradingSignals({ enabled: consentGiven && hasActiveSubscription, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/insights/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/insights/index.tsx
📚 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/insights/index.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/insights/index.tsx
⏰ 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: pillarx-debug
🔇 Additional comments (3)
src/apps/insights/index.tsx (3)
45-86: Utility functions are well-implemented.SSR-safe checks, proper null handling, and the timestamp normalization logic in
formatDate(detecting seconds vs milliseconds) are correctly implemented.
88-132: State and hook initialization looks good.The wallet address resolution priority (stored → Privy → fallback) aligns with existing patterns. Conditional enabling of
useTradingSignalsbased on consent and subscription prevents unnecessary API calls.
552-639: Main render structure is well-organized.The component properly handles different states with early returns, uses
AnimatePresencecorrectly for animated list transitions, and provides appropriate keys for list items. The modal open state is cleanly derived fromselectedPnLView.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/apps/insights/index.tsx (3)
423-431: Consider detecting actual user region instead of hardcoding "Other".The
userRegionprop is hardcoded to"Other", which may not accurately reflect the user's location. If consent requirements differ by region (e.g., GDPR for EU users), this could lead to compliance issues.Consider implementing region detection (via IP geolocation or browser locale) if consent requirements vary by jurisdiction:
const [userRegion, setUserRegion] = useState<string>('Other'); useEffect(() => { // Detect user region via API or browser locale // e.g., fetch from a geolocation service // setUserRegion(detectedRegion); }, []); const consentModalElement = ( <ConsentModal open={showConsentModal} onConsentAccepted={handleConsentAccepted} onConsentDeclined={handleConsentDeclined} userRegion={userRegion} immediateAccess={true} /> );
306-308: Empty catch block silently swallows subscription refresh errors.The
refetchSubscription().catch(() => {})pattern suppresses all errors without logging or notifying the user. If the refresh fails, users won't know why their subscription status isn't updating.Apply this diff to log errors:
const handleRefreshSubscription = useCallback(() => { - refetchSubscription().catch(() => {}); + refetchSubscription().catch((error) => { + console.error('Failed to refresh subscription:', error); + }); }, [refetchSubscription]);
324-326: Empty catch block silently swallows subscription refresh errors.Similar to the
handleRefreshSubscription, thisrefetchSubscription().catch(() => {})call suppresses errors without logging, making it difficult to debug subscription issues.Apply this diff to log errors:
setIsAwaitingSubscription(true); startPolling(); -refetchSubscription().catch(() => {}); +refetchSubscription().catch((error) => { + console.error('Failed to refetch subscription after subscribe click:', error); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/insights/index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/insights/index.tsx
📚 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/insights/index.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/insights/index.tsx
⏰ 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: pillarx-debug
🔇 Additional comments (1)
src/apps/insights/index.tsx (1)
182-212: ThefetchSparklinesfunction is properly memoized—no issue here.The
useSparklineDatahook wraps bothfetchSparkline(line 16) andfetchSparklines(line 50) inuseCallback, making them stable across renders. The disabledexhaustive-depslinting rule is justified and safe because the effect intentionally depends only onsignals.lengthandloadingto avoid unnecessary re-fetches when signal data changes.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/apps/insights/hooks/useSparklineData.ts (2)
19-19: Consider removing or conditionalizing console logs for production.Multiple console.log statements are present throughout the hook. While useful for debugging, these should be removed or wrapped in a conditional flag for production builds to avoid console pollution.
Consider creating a debug utility:
const DEBUG = process.env.NODE_ENV === 'development'; const debugLog = (...args: any[]) => DEBUG && console.log(...args);Then replace console.log calls with
debugLog.Also applies to: 30-30, 47-47, 49-49
56-63: Consider rate limiting or batching concurrent requests.Fetching sparklines for all active signals simultaneously could trigger many concurrent API calls. Consider adding rate limiting or request batching to avoid overwhelming the backend or hitting rate limits.
Example approach using promise batching:
const fetchSparklines = useCallback(async (signals: TradingSignal[]) => { const activeSignals = signals.filter(s => s.status === 'active'); console.log(`🔄 [Sparkline] Fetching sparklines for ${activeSignals.length} active signals`); // Batch requests in groups of 5 const batchSize = 5; for (let i = 0; i < activeSignals.length; i += batchSize) { const batch = activeSignals.slice(i, i + batchSize); await Promise.all(batch.map(signal => fetchSparkline(signal))); } }, [fetchSparkline]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/apps/insights/hooks/useSparklineData.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/apps/insights/hooks/useSparklineData.ts (2)
src/apps/insights/types/index.ts (2)
SparklineDataPoint(52-55)TradingSignal(5-35)src/apps/insights/api/insightsApi.ts (1)
fetchSparklineData(17-41)
⏰ 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 (3)
src/apps/insights/hooks/useSparklineData.ts (3)
1-14: LGTM!The hook initialization is well-structured. The use of
loadingRefalongside state is a good pattern to prevent dependency loops while keeping the UI reactive.
16-21: LGTM!The early-return pattern using the ref to check loading state is a good approach to prevent duplicate fetches without causing dependency issues.
65-71: LGTM!The return interface is clean and provides the expected API for consumers of this hook.
| try { | ||
| loadingRef.current[signal.id] = true; | ||
| setLoading(prev => ({ ...prev, [signal.id]: true })); | ||
|
|
||
| const startTime = new Date(signal.created_at).getTime(); | ||
| const endTime = Date.now(); | ||
|
|
||
| console.log(`📊 [Sparkline] Fetching ${signal.ticker} (${signal.id})`); | ||
| const result = await fetchSparklineData(signal.ticker, startTime, endTime); |
There was a problem hiding this comment.
Validate signal.created_at before converting to timestamp.
If signal.created_at is malformed or missing, new Date(signal.created_at).getTime() will return NaN, leading to incorrect API calls or unexpected behavior.
Apply this diff to add validation:
try {
loadingRef.current[signal.id] = true;
setLoading(prev => ({ ...prev, [signal.id]: true }));
- const startTime = new Date(signal.created_at).getTime();
+ const startTime = new Date(signal.created_at).getTime();
+ if (isNaN(startTime)) {
+ throw new Error(`Invalid created_at date: ${signal.created_at}`);
+ }
const endTime = Date.now();
console.log(`📊 [Sparkline] Fetching ${signal.ticker} (${signal.id})`);🤖 Prompt for AI Agents
In src/apps/insights/hooks/useSparklineData.ts around lines 23 to 31, the code
converts signal.created_at to a timestamp without validating it, so new
Date(signal.created_at).getTime() can produce NaN if created_at is missing or
malformed; validate that signal.created_at exists and is a valid date before
using it (e.g., attempt to parse new Date(signal.created_at), check
isFinite(date.getTime())/!isNaN, and if invalid fall back to a safe default or
abort the fetch with an error/log and clear the loading flags), and ensure any
early returns also reset loadingRef.current[signal.id] and setLoading to avoid
leaving the UI stuck in loading state.
| if (result.error) { | ||
| throw result.error; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for fetchSparklineData implementation
rg -A 20 "export const fetchSparklineData" --type tsRepository: pillarwallet/x
Length of output: 1383
🏁 Script executed:
#!/bin/bash
# Get the complete fetchSparklineData function
rg -A 30 "export const fetchSparklineData" src/apps/insights/api/insightsApi.tsRepository: pillarwallet/x
Length of output: 781
🏁 Script executed:
#!/bin/bash
# Get complete context around lines 33-35 in useSparklineData.ts
cat -n src/apps/insights/hooks/useSparklineData.ts | head -50Repository: pillarwallet/x
Length of output: 2271
Remove the incorrect error check on lines 33-35.
The fetchSparklineData function throws errors directly (via throw error in its catch block) rather than returning an object with an error property. The check if (result.error) is dead code that will never be reached—if fetchSparklineData encounters an error, it throws immediately and the execution jumps to the catch block on line 48. This check should be removed as it's misleading and serves no purpose.
🤖 Prompt for AI Agents
In src/apps/insights/hooks/useSparklineData.ts around lines 33-35, remove the
dead error check "if (result.error) { throw result.error; }" because
fetchSparklineData throws errors directly and never returns an object with an
error property; update the function by deleting those lines so errors propagate
to the existing catch block instead and ensure no other code depends on
result.error.
| const candles = Array.isArray(result?.candles) | ||
| ? result.candles | ||
| : Array.isArray(result?.data?.candles) | ||
| ? result.data.candles | ||
| : []; | ||
|
|
||
| setSparklineDataMap(prev => ({ | ||
| ...prev, | ||
| [signal.id]: candles as SparklineDataPoint[], | ||
| })); |
There was a problem hiding this comment.
Add type validation before casting to SparklineDataPoint[].
The extracted candles array is cast to SparklineDataPoint[] without validating that each element has the required time and price properties. This could lead to runtime errors if the API returns unexpected data.
Apply this diff to add validation:
const candles = Array.isArray(result?.candles)
? result.candles
: Array.isArray(result?.data?.candles)
? result.data.candles
: [];
+
+ // Validate candles structure
+ const validCandles = candles.filter((c: any) =>
+ typeof c?.time === 'number' && typeof c?.price === 'number'
+ );
setSparklineDataMap(prev => ({
...prev,
- [signal.id]: candles as SparklineDataPoint[],
+ [signal.id]: validCandles as SparklineDataPoint[],
}));
- console.log(`✅ [Sparkline] Fetched ${signal.ticker} - ${candles.length} candles`);
+ console.log(`✅ [Sparkline] Fetched ${signal.ticker} - ${validCandles.length} candles`);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/apps/insights/hooks/useSparklineData.ts around lines 37 to 46, the code
assigns result-derived candles to SparklineDataPoint[] by casting without
validation; add a runtime type guard that checks each item is an object with
numeric (or parseable) time and price properties, use Array.isArray on the
source then filter using that guard to produce a validated array (or [] if
none), and pass the filtered array to setSparklineDataMap instead of the direct
cast so only properly shaped SparklineDataPoint objects are stored.
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.