Fro 7 chart printing#10
Conversation
📝 WalkthroughWalkthroughChangesThe PR establishes a Vite React frontend with CI, typed configuration, authentication and session management, globe-based landing experience, portfolio and live market dashboards, reusable UI components, styling, and Login component tests. Frontend foundation
Authentication
Product features
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (18)
src/components/PortfolioView.tsx-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCI Prettier
--checkfails on three files in this PR. The formatter wasn't run before committing; a singlenpx prettier --writepass over the changed files clears the whole job.
src/components/PortfolioView.tsx#L1-L1: reformat — trailing whitespace around lines 199-204.src/components/TradingViewChart.tsx#L93-L141: reformat — over-width inline style objects and trailing whitespace.src/pages/LiveMarketDashboard.tsx#L1-L1: reformat — over-width JSX lines (200, 281, 285, 345, 349).Consider adding a pre-commit hook so this doesn't block CI again.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PortfolioView.tsx` at line 1, Run Prettier on the changed files to resolve CI formatting failures: reformat src/components/PortfolioView.tsx lines 199-204, src/components/TradingViewChart.tsx lines 93-141, and src/pages/LiveMarketDashboard.tsx lines 200, 281, 285, 345, and 349; no functional changes are needed. Consider adding a pre-commit formatting hook to prevent future failures.Source: Pipeline failures
src/pages/LiveMarketDashboard.tsx-21-21 (1)
21-21: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winInsecure
ws://fallback ships to production builds.
import.meta.env.VITE_MARKET_WS_BASE_URLis inlined at build time; if it's unset in the deploy environment the bundle silently targetsws://localhost:8001, which browsers also block as mixed content on an HTTPS origin. Fail fast (or derivewss://fromwindow.location) instead of defaulting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` at line 21, Update the MARKET_WS_URL initialization to remove the insecure ws://localhost:8001 production fallback. Require VITE_MARKET_WS_BASE_URL at build/runtime and fail fast when absent, or derive a secure wss:// URL from window.location while preserving valid configured URLs.src/components/PortfolioView.tsx-57-62 (1)
57-62: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winInconsistent assumptions about
portfolio.holdingsbeing defined.Line 92 defensively uses
portfolio?.holdings?.length, but line 60 spreadsportfolio.holdingsand line 198 maps it directly. If the API ever returns a portfolio withoutholdings(which line 92 implies is possible), those two sites throw. Pick one contract — either the type guarantees an array (drop the?.on line 92) or normalize on fetch.🛡️ Normalize once at fetch time
const data = await portfolioService.getPortfolio(); - setPortfolio(data); + setPortfolio({ ...data, holdings: data.holdings ?? [] });Also applies to: 92-92, 198-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PortfolioView.tsx` around lines 57 - 62, Normalize the portfolio data when it is fetched so `portfolio.holdings` is always an array, defaulting missing holdings to an empty array. Preserve the existing direct spread in the holding-addition update and direct map usage, and remove the inconsistent optional chaining in the holdings-length check to rely on this normalized contract.src/pages/LiveMarketDashboard.tsx-84-95 (1)
84-95: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGet-then-create is a TOCTOU on portfolio creation.
Two concurrent "Add Stock" clicks both 404 on line 85 and both call
createPortfolio, so the outcome depends on backend uniqueness enforcement. It also costs an extra round-trip on every add. Prefer an idempotent server-side upsert, or at least deduplicate the create with an in-flight promise ref.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` around lines 84 - 95, Replace the get-then-create flow in the Add Stock handler around portfolioService.getPortfolio and createPortfolio with an idempotent server-side portfolio upsert, preserving concurrent-safe creation and avoiding the extra lookup round trip. If upsert is unavailable, deduplicate creation through an in-flight promise ref so concurrent clicks share one create operation before calling addHolding.src/components/StockSearchBar.tsx-34-49 (1)
34-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDebounce doesn't guard against out-of-order responses.
The cleanup clears the pending timer but not an in-flight request. If the user keeps typing after a request has already fired, a slower earlier response can land last and repopulate the dropdown with stale results (and flip
isLoadingoff prematurely). Add a cancellation flag orAbortSignal.🐛 Proposed fix
setIsLoading(true); + let cancelled = false; const timer = setTimeout(async () => { try { const searchResults = await marketService.searchStocks(query); + if (cancelled) return; setResults(searchResults); setIsOpen(true); } catch (err) { + if (cancelled) return; console.error('Failed to search stocks:', err); setResults([]); } finally { - setIsLoading(false); + if (!cancelled) setIsLoading(false); } }, 300); - return () => clearTimeout(timer); + return () => { + cancelled = true; + clearTimeout(timer); + }; }, [query]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/StockSearchBar.tsx` around lines 34 - 49, Update the search effect around the setTimeout callback and marketService.searchStocks call to guard against stale in-flight responses after cleanup. Use a per-effect cancellation flag or AbortSignal, check it before applying search results and loading state updates, and preserve the existing debounce behavior while preventing earlier requests from overwriting newer query results.src/components/TradingViewChart.tsx-26-27 (1)
26-27: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against a null/empty analyze response.
Per
src/services/market.service.ts(lines 21-25)analyzeStockreturns whatever the endpoint sends and is typedany. If the body isnull, line 27 throws aTypeErrorthat gets caught and rendered verbatim to the user as "Cannot read properties of null…". Use optional access and a friendly fallback.🛡️ Proposed fix
const response = await marketService.analyzeStock(symbol); - setAnalysisData(response.analysis); + if (!response) { + setError('No analysis available for this symbol.'); + return; + } + setAnalysisData(response.analysis ?? null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/TradingViewChart.tsx` around lines 26 - 27, Update the analyzeStock response handling in TradingViewChart to safely access analysis when the response is null or empty, and provide a friendly fallback value instead of allowing a property-access TypeError to reach the user. Preserve the existing setAnalysisData flow for valid responses.src/pages/LiveMarketDashboard.tsx-72-79 (1)
72-79: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winEmpty catch hides real failures (and trips lint).
The comment says 404 is expected, but this also silently swallows 500s, auth failures, and network errors — the user sees an empty holdings list with no explanation. Handle 404 explicitly and surface the rest. This also resolves the
no-unused-varswarning on theerrbinding.🛡️ Proposed fix
const fetchPortfolioHoldings = async () => { try { const data = await portfolioService.getPortfolio(); - setPortfolioHoldings(data.holdings.map((h) => h.symbol)); - } catch (err) { - // 404 is normal if portfolio is not yet created + setPortfolioHoldings((data.holdings ?? []).map((h) => h.symbol)); + } catch (err: any) { + // 404 is normal if portfolio is not yet created + if (err?.response?.status === 404) return; + setFeedback({ message: 'Failed to load portfolio holdings.', type: 'error' }); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` around lines 72 - 79, Update fetchPortfolioHoldings to handle errors by status: treat the expected 404 as an empty or unchanged portfolio, but surface non-404 failures through the page’s existing error-reporting mechanism or user-facing state. Remove the unused catch binding if using status-independent handling, or use the caught error to distinguish 404 from other failures.Source: Linters/SAST tools
package.json-22-22 (1)
22-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeclare the Vitest coverage provider.
test:coverageenables coverage, but the manifest only listsvitestand no Vitest coverage provider package. Coverage providers are optional, so this script can prompt or fail in non-interactive CI instead of producing coverage.Add a provider matching the Vitest major version, or remove this script.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 22, Add a Vitest coverage provider dependency to the package manifest, matching the existing Vitest major version, so the test:coverage script can run non-interactively in CI; alternatively remove the test:coverage script if coverage is not supported.src/index.css-1-1 (2)
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStylelint
value-keyword-case: lowercasecurrentColorin both files. Same rule violation, same one-line fix, in two files.
src/index.css#L184-184: changebox-shadow: 0 0 6px currentColor;tocurrentcolor.src/styles/components/country-list.css#L89-89: changebox-shadow: 0 0 6px currentColor;tocurrentcolor.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` at line 1, Update both box-shadow declarations using currentColor in src/index.css and src/styles/components/country-list.css to use the lowercase currentcolor keyword, preserving the existing shadow values.Source: Linters/SAST tools
1-1: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCI Prettier check failed on this file.
Run
prettier --writelocally to resolve the formatting warnings surfaced in the pipeline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` at line 1, Run Prettier with write mode on the stylesheet represented by the top-level CSS content, then commit the formatter’s changes so the CI formatting check passes.Source: Pipeline failures
src/index.css-6-6 (1)
6-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStylelint: unquote the font-family name.
CI's Stylelint run flags this line for
font-family-name-quotes.🎨 Proposed fix
- font-family: 'Inter', -apple-system, BlinkMacSystemFont, sans-serif; + font-family: Inter, -apple-system, BlinkMacSystemFont, sans-serif;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` at line 6, Update the font-family declaration to use the unquoted Inter font name, while preserving the existing fallback fonts and ordering.Source: Linters/SAST tools
src/components/Sidebar.tsx-30-30 (1)
30-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHardcoded placeholder name as username fallback.
'Anjal Dev VK'looks like a leftover dev/test value rather than an intentional generic fallback. Ifuseris ever null/loading when this renders, real users would see this specific name instead of something like'Guest'.Suggested fix
- const username = user?.username || 'Anjal Dev VK'; + const username = user?.username || 'Guest';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Sidebar.tsx` at line 30, Replace the hardcoded developer name fallback in the Sidebar username assignment with a generic guest label such as the established “Guest” value, while preserving the authenticated user.username path.src/components/ui/AuthLayout/AuthLayout.tsx-18-26 (1)
18-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCustom
role="button"only responds to Enter, not Space.Native buttons activate on both Enter and Space; matching that expectation here is a small, high-value a11y fix.
⌨️ Proposed fix
- onKeyDown={(e) => e.key === 'Enter' && navigate(ROUTES.HOME)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + navigate(ROUTES.HOME); + } + }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/AuthLayout/AuthLayout.tsx` around lines 18 - 26, Update the brand-header keyboard handler in AuthLayout to activate navigation for both Enter and Space, matching native button behavior; preserve the existing click navigation and accessibility attributes.src/types/env.d.ts-8-12 (1)
8-12: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing
VITE_MARKET_API_BASE_URLdeclaration.
src/lib/api.client.tsreadsimport.meta.env.VITE_MARKET_API_BASE_URL(with a localhost fallback), but it's not declared inImportMetaEnvhere, contradicting this file's own comment that allVITE_variables must be declared for type safety.🔧 Proposed fix
interface ImportMetaEnv { readonly VITE_APP_NAME: string; readonly VITE_APP_ENV: 'development' | 'staging' | 'production'; readonly VITE_API_BASE_URL: string; + readonly VITE_MARKET_API_BASE_URL: string; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types/env.d.ts` around lines 8 - 12, Add the missing readonly VITE_MARKET_API_BASE_URL string declaration to the ImportMetaEnv interface, alongside the existing VITE_API_BASE_URL entry, so the environment variable used by api.client.ts is covered by type checking.src/components/VerifyEmail.tsx-58-64 (1)
58-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winError
detailmay be a validation-error array, not a string.Per
HTTPValidationErrorinsrc/features/auth/auth.types.ts,detailcan beValidationError[]. Rendering that directly at Line 161 aserrorMessagerisks a React "objects are not valid as a child" crash instead of a friendly message.🛠️ Proposed normalization
- setErrorMessage( - err.response?.data?.detail || 'Invalid or expired email verification token.', - ); + const detail = err.response?.data?.detail; + setErrorMessage( + typeof detail === 'string' + ? detail + : 'Invalid or expired email verification token.', + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/VerifyEmail.tsx` around lines 58 - 64, Normalize the `err.response?.data?.detail` value in `VerifyEmail` before passing it to `setErrorMessage`: preserve string details, but convert `ValidationError[]` entries into a user-readable string (or use the existing fallback) so `errorMessage` never contains objects that React would render directly.src/components/Login.tsx-197-208 (1)
197-208: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win"Remember Me" checkbox has no effect.
rememberMe/setRememberMe(line 21) is only read by this checkbox and never referenced inhandleSubmitor passed toauthService.login. Users toggling it get no actual behavior change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Login.tsx` around lines 197 - 208, Connect the rememberMe state from the checkbox in Login to the authentication submission flow: update handleSubmit to use the selected value and pass it through to authService.login using that method’s existing remember-session option. Preserve the current checkbox state updates and loading behavior.src/components/Login.tsx-241-243 (1)
241-243: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSilent
localhostfallback for the OAuth redirect URL.If
VITE_API_BASE_URLisn't set in a production build, users clicking "Continue with Google" get redirected tohttp://localhost:8000/..., a confusing dead end instead of a clear error.🔧 Proposed fix
- const API_URL = import.meta.env.VITE_API_BASE_URL || 'http://localhost:8000'; + const API_URL = import.meta.env.VITE_API_BASE_URL; + if (!API_URL) { + console.error('VITE_API_BASE_URL is not configured'); + return; + } window.location.href = `${API_URL}/auth/oauth2/google/login`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Login.tsx` around lines 241 - 243, Remove the silent localhost fallback in the Google OAuth redirect handler within Login. Validate that VITE_API_BASE_URL is configured before constructing the redirect URL, and fail clearly instead of navigating to localhost when it is missing; preserve the existing redirect behavior when the variable is present.src/App.tsx-97-106 (1)
97-106: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPost-login redirect-back (
location.state.from) is never consumed.
AuthGuard.tsxpreserves the original destination in route state specifically so users land back where they intended after logging in, butonLoginSuccesshere always navigates toROUTES.DASHBOARDunconditionally, so that state is dead.🔧 Proposed fix
+import { useNavigate, useLocation, ... } from 'react-router-dom'; function AppContent() { const navigate = useNavigate(); + const location = useLocation(); const { logout, initAuth, user } = useUserStore(); @@ <Login - onLoginSuccess={() => navigate(ROUTES.DASHBOARD)} + onLoginSuccess={() => + navigate((location.state as { from?: Location })?.from?.pathname ?? ROUTES.DASHBOARD) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/App.tsx` around lines 97 - 106, Update the login route’s onLoginSuccess handler in App.tsx to read the preserved location.state.from value and navigate to that destination after authentication, falling back to ROUTES.DASHBOARD when no return location exists. Keep the existing signup and forgot-password navigation handlers unchanged.
🧹 Nitpick comments (17)
src/components/StockSearchBar.tsx (2)
93-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
type="button"to these buttons. They default totype="submit"; if this component is ever dropped inside a<form>(a plausible placement for a search bar), clicking clear/add would submit it.Also applies to: 130-134
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/StockSearchBar.tsx` around lines 93 - 103, Add type="button" to the clear button and the additional button identified around the second occurrence, so neither defaults to form submission when StockSearchBar is rendered inside a form. Preserve their existing click handlers and behavior.
62-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
handleAddswallows nothing but surfaces nothing either.
onAddStockis typedPromise<void>; if a consumer's implementation rejects, this becomes an unhandled rejection and the dropdown stays open with no feedback. Acatchthat keeps the dropdown open and logs (or anonErrorprop) would make the contract safe regardless of caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/StockSearchBar.tsx` around lines 62 - 71, Update handleAdd in StockSearchBar to catch rejected onAddStock promises, keep the dropdown open and query intact on failure, and provide error visibility through the component’s established logging or error-reporting mechanism; retain the existing success cleanup and always reset adding state in finally.src/components/PortfolioView.tsx (1)
199-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClickable
divis not keyboard accessible.The holding row carries an
onClickbut no role,tabIndex, or key handler, so keyboard users can't select a stock. Consider a<button>wrapper for the info area (nesting the Remove button inside a clickable row is also a nesting hazard) or add the ARIA/keyboard affordances.♿ Minimal fix
<div key={holding.id} className="holding-item" + role={onSelectStock ? 'button' : undefined} + tabIndex={onSelectStock ? 0 : undefined} onClick={() => onSelectStock && onSelectStock(holding.symbol)} + onKeyDown={(e) => { + if (onSelectStock && (e.key === 'Enter' || e.key === ' ')) { + e.preventDefault(); + onSelectStock(holding.symbol); + } + }} style={{ cursor: onSelectStock ? 'pointer' : 'default' }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/PortfolioView.tsx` around lines 199 - 204, Make the holding row around the `holding-item` div keyboard accessible while preserving stock selection: add an appropriate interactive role, focusability, and keyboard handler that triggers `onSelectStock` for Enter and Space. Ensure the existing nested Remove button remains independently operable and does not trigger row selection when activated.src/pages/LiveMarketDashboard.tsx (3)
98-98: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFeedback timeouts aren't cleared on unmount.
Both
setTimeoutcalls firesetFeedbackup to 5s later with no cleanup. Harmless today, but store the handle in a ref and clear it on unmount / before scheduling the next one so overlapping toasts don't cancel each other early.Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` at line 98, Update the feedback timeout handling in LiveMarketDashboard so both setTimeout calls use a ref to store their handles, clear the existing timeout before scheduling a replacement, and clear the handle during component unmount cleanup. Preserve independent timing for successive feedback messages without allowing stale callbacks after unmount.
244-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGainer and loser cards are near-identical duplicates.
~50 lines are copy-pasted with only the pill class, sign prefix, and icon border color differing. Extract a
<MarketStockCard stock variant="gainer" | "loser" isAdded onAdd />so the two grids stay in sync.Also applies to: 308-356
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` around lines 244 - 292, Extract the duplicated gainer and loser card markup into a reusable MarketStockCard component accepting stock, variant ("gainer" or "loser"), isAdded, and onAdd props. Move the shared layout and behavior into that component, deriving the change-pill styling, sign prefix, and variant-specific icon border color from variant, then replace both grid mappings with the component while preserving existing portfolio and click behavior.
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
searchQueryis permanently''—filterStocksis dead code.No setter is destructured, so lines 119-125 always return the input unchanged and the whole filter path is unreachable. Either wire the header
StockSearchBarto drive this state or removesearchQuery/filterStocksand usemarketData.gainers/losersdirectly.Also applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` at line 37, Update the LiveMarketDashboard search flow by either connecting StockSearchBar input to the searchQuery state via its setter and preserving filterStocks usage, or removing searchQuery and filterStocks and rendering marketData.gainers/losers directly; do not leave a permanently empty search state with dead filtering logic.src/features/globe/useGlobe.ts (1)
51-53: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCore visual assets depend on unpkg.com at runtime.
globeImageUrl/bumpImageUrl/backgroundImageUrl(Line 51-53) and the country-boundary GeoJSON fetch (Line 140) all hitunpkg.comdirectly. The existing TODO only calls out self-hosting the GeoJSON — the three texture URLs aren't covered and have no error handling if the CDN is slow/unreachable, unlike the GeoJSON fetch which at least has a.catch.Want me to open a follow-up issue to self-host all four assets (textures + GeoJSON) under
/public, consistent with the existing TODO's intent?Also applies to: 139-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/globe/useGlobe.ts` around lines 51 - 53, Update the Globe configuration in useGlobe to load the globe, bump, and background textures from locally self-hosted assets under /public, and apply the same local-asset approach to the country-boundary GeoJSON fetch near the existing TODO; remove the direct unpkg.com dependencies while preserving the current asset usage and error handling behavior.src/index.css (1)
21-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSubstantial orphaned/duplicate CSS: no matching markup exists in
GlobeView.tsx.
#ui-overlay,.home-overlay-title,.home-overlay-status,.status-dot(+ itspulsekeyframe), and.resume-rotate-btnhave no corresponding elements rendered inGlobeView.tsx. Additionally, the.country-list/.country-item/.country-item.activerules here (Line 110-169) duplicate the more complete, accessible version already added insrc/styles/components/country-list.css(which adds list semantics,:focus-visible, and design-token fallbacks).Recommend removing this legacy block from
index.cssin favor of the dedicatedcountry-list.css, once/if the country-list UI referenced inGlobeView.tsxis confirmed unused or superseded.Also applies to: 110-169, 187-208
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.css` around lines 21 - 64, Remove the orphaned legacy selectors from index.css: `#ui-overlay`, .home-overlay-title, .home-overlay-status, .status-dot, .resume-rotate-btn, and the duplicate .country-list/.country-item/.country-item.active rules. Retain the dedicated country-list.css implementation as the single source of truth, and remove its associated pulse keyframe only if no remaining markup or styles use it.src/components/Sidebar.tsx (1)
82-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInert nav buttons (Analytics, AI Discipline, Global Feeds, Settings) have no
onClick.Unlike "Explore Markets"/"Portfolio", these buttons render as clickable but do nothing. If intentional placeholders for upcoming routes, consider a
disabledstate ortitle="Coming soon"so the affordance matches behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Sidebar.tsx` around lines 82 - 119, Update the Analytics, AI Discipline, Global Feeds, and Settings buttons in the Sidebar component to reflect their unavailable navigation state by adding the appropriate disabled behavior or a clear “Coming soon” title. Ensure these placeholder buttons no longer appear to be actionable while preserving the existing behavior of Explore Markets and Portfolio.src/styles/components/sidebar.css (1)
3-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared sidebar width; z-index outlier bypasses the token scale.
width: 240pxhere is duplicated as a magic number inlive-market.css's.vercel-dashboard-main { margin-left: 240px; }. A shared CSS variable (e.g.--sidebar-width: 240pxintokens.css) would keep both in sync and make the offset fordashboard.css(see related comment) easier to add correctly.Separately,
z-index: 1000(line 19) is hardcoded well above the scaletokens.cssestablishes (--z-modal: 100,--z-toast: 200), unlikeoverlay.csswhich correctly usesvar(--z-overlay, 10). Any future modal/toast using the token scale would render behind this sidebar.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/styles/components/sidebar.css` around lines 3 - 20, Define a shared --sidebar-width token in tokens.css and replace the 240px width in .vercel-sidebar and the matching .vercel-dashboard-main margin-left with that variable. Replace the hardcoded z-index: 1000 in .vercel-sidebar with the appropriate existing z-index token, preserving the sidebar’s intended stacking order within the established scale.src/styles/components/auth-layout.css (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded colors bypass the design-token system.
This file (and
live-market.css,sidebar.css,stock-search-bar.css) hardcodes literal hex values (#000000,#ededed,#0070f3, …) throughout, whiledashboard.cssandportfolio.cssconsistently referencetokens.cssvariables viavar(--color-*, fallback). This shows AuthLayout importing auth-layout.css so its full-screen layout and form-card styles apply. Sincetokens.cssalready defines equivalent values (--color-bg-primary:#000000, `--color-text-primary: `#ededed,--color-accent-blue:#0070f3``, etc.), consider aligning this file with the established token pattern to avoid future drift between the two theming approaches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/styles/components/auth-layout.css` around lines 5 - 16, Replace the hardcoded color literals throughout auth-layout.css with the equivalent tokens from tokens.css, using var(--color-*, fallback) consistently for layout, text, backgrounds, borders, and accents. Preserve the existing visual values and styles while aligning AuthLayout with the established token-based pattern used by dashboard.css and portfolio.css.src/services/market.service.ts (1)
21-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnescaped path-segment interpolation for ticker symbols.
Both services splice a caller-provided
symboldirectly into a URL path without encoding, so any unexpected character (slash, space, unicode) would malform the request path instead of failing cleanly.
src/services/market.service.ts#L21-L25: wrapsymbolwithencodeURIComponentbefore building/dashboard/analyze-stock/${symbol}.src/services/portfolio.service.ts#L40-L42: wrapsymbolwithencodeURIComponentbefore building/portfolio/holdings/${symbol}.🔧 Proposed fixes
async analyzeStock(symbol: string): Promise<any> { if (!symbol) return null; - const { data } = await marketApiClient.get(`/dashboard/analyze-stock/${symbol}`); + const { data } = await marketApiClient.get(`/dashboard/analyze-stock/${encodeURIComponent(symbol)}`); return data; },async removeHolding(symbol: string): Promise<void> { - await marketApiClient.delete(`/portfolio/holdings/${symbol}`); + await marketApiClient.delete(`/portfolio/holdings/${encodeURIComponent(symbol)}`); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/market.service.ts` around lines 21 - 25, Encode the caller-provided symbol before interpolating it into the request path in analyzeStock within src/services/market.service.ts (lines 21-25), using encodeURIComponent while preserving the existing API call behavior. Apply the same encoding change to the symbol path segment in src/services/portfolio.service.ts (lines 40-42) for the portfolio holdings request.src/lib/api.client.ts (1)
19-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate request-interceptor logic across both clients.
Lines 20-26 and 29-35 are identical apart from the client reference. Extract a shared
attachAuthHeaderhelper to avoid drift.♻️ Proposed dedup
+const attachAuthHeader = (config: any) => { + const token = useUserStore.getState().accessToken; + if (token && config.headers) { + config.headers.Authorization = `Bearer ${token}`; + } + return config; +}; + -marketApiClient.interceptors.request.use((config) => { - const token = useUserStore.getState().accessToken; - if (token && config.headers) { - config.headers.Authorization = `Bearer ${token}`; - } - return config; -}); +marketApiClient.interceptors.request.use(attachAuthHeader); -apiClient.interceptors.request.use((config) => { - const token = useUserStore.getState().accessToken; - if (token && config.headers) { - config.headers.Authorization = `Bearer ${token}`; - } - return config; -}); +apiClient.interceptors.request.use(attachAuthHeader);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/api.client.ts` around lines 19 - 35, Extract the duplicated token-and-Authorization-header logic from both request interceptors into a shared attachAuthHeader helper, then register that helper with marketApiClient and apiClient. Preserve the existing behavior of reading useUserStore.getState().accessToken, conditionally setting the Bearer header, and returning the config.src/components/ui/ErrorBoundary/ErrorBoundary.tsx (1)
36-36: 📐 Maintainability & Code Quality | 🔵 TrivialTODO: wire real error tracking.
Flagging the explicit TODO for a production error-tracking integration (e.g. Sentry). Happy to help scaffold
componentDidCatchto forward to a tracking SDK if useful — want me to draft that, or should this be tracked as a separate issue?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ui/ErrorBoundary/ErrorBoundary.tsx` at line 36, Replace the placeholder TODO in ErrorBoundary with a real production error-tracking integration, wiring componentDidCatch to forward the captured error and relevant component information to the configured tracking service such as Sentry. Remove the TODO after the integration is connected.src/features/auth/auth.service.ts (1)
68-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
anyfor the register payload.Every other method here is strongly typed;
register(data: any)loses compile-time safety for a shape ({ username, email, password }) consumed directly fromRegister.tsx. Define and export aRegisterRequesttype alongsideToken/UserResponseinauth.types.ts.♻️ Proposed fix
-import type { Token, UserResponse, UserSessionResponse } from './auth.types'; +import type { Token, UserResponse, UserSessionResponse, RegisterRequest } from './auth.types'; @@ - async register(data: any): Promise<UserResponse> { + async register(data: RegisterRequest): Promise<UserResponse> { const response = await apiClient.post<UserResponse>('/auth/register', data); return response.data; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/auth/auth.service.ts` around lines 68 - 74, Replace the any payload in AuthService.register with an exported RegisterRequest type defined alongside Token and UserResponse in auth.types.ts, containing username, email, and password; import and use that type so Register.tsx input is compile-time validated.src/components/Register.tsx (1)
21-22: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSignup password policy is weaker than the reset-password policy.
Registration only requires 8 characters with no complexity check, while
ResetPassword.tsxrequires 12+ characters plus a number/symbol for the same account. Consider aligning client-side validation here with the stronger policy (the backend's 8-char minimum is still satisfied by any password meeting the 12+/complexity rule).♻️ Proposed fix
- const hasMinLength = password.length >= 8; // OpenAPI schema says minLength: 8 for UserCreate - const isValid = username.length >= 3 && email.includes('@') && hasMinLength; + const hasMinLength = password.length >= 12; + const hasNumberOrSymbol = /[0-9!@#$%^&*(),.?":{}|<>]/.test(password); + const isValid = username.length >= 3 && email.includes('@') && hasMinLength && hasNumberOrSymbol;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Register.tsx` around lines 21 - 22, Update the registration validation around hasMinLength and isValid to match ResetPassword.tsx’s stronger password policy: require at least 12 characters plus the same number and symbol checks, while preserving the existing username and email validation.src/components/Login.tsx (1)
103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile substring match to gate the resend-verification UI.
Deciding whether to show "Resend verification email" by checking
errorMsg.toLowerCase().includes('verify')couples UI logic to exact wording set in the catch block. Prefer a dedicated boolean set alongside the status check.♻️ Proposed fix
const [errorMsg, setErrorMsg] = useState<string | null>(null); + const [needsVerification, setNeedsVerification] = useState(false); @@ if (err.response?.status === 403) { setErrorMsg(err.response.data?.detail || 'Please verify your email before logging in.'); + setNeedsVerification(true); } else if (err.response?.status === 401 || err.response?.status === 400) { setErrorMsg('Invalid email or password.'); + setNeedsVerification(false); } else { setErrorMsg('An unexpected error occurred. Please try again.'); + setNeedsVerification(false); } @@ - {errorMsg.toLowerCase().includes('verify') && ( + {needsVerification && (🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Login.tsx` at line 103, Replace the wording-dependent check in the Login component’s error rendering with a dedicated boolean tracking whether email verification is required, set alongside the relevant status check in the catch flow. Use that boolean to gate the “Resend verification email” UI while preserving the existing behavior for other errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 380a6f7c-c9ea-4bbc-93dc-989fbf6d4810
⛔ Files ignored due to path filters (4)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/favicon.svgis excluded by!**/*.svgpublic/icons.svgis excluded by!**/*.svgpublic/trading_banner.pngis excluded by!**/*.png
📒 Files selected for processing (64)
.env.example.github/workflows/ci.yml.gitignore.node-version.nvmrc.oxlintrc.json.prettierignore.prettierrc.json.vscode/extensions.json.vscode/settings.jsonREADME.mdindex.htmlpackage.jsonsrc/App.tsxsrc/components/AuthCallback.tsxsrc/components/ForgotPassword.tsxsrc/components/Login.tsxsrc/components/PortfolioView.tsxsrc/components/Register.tsxsrc/components/ResetPassword.tsxsrc/components/Sidebar.tsxsrc/components/StockSearchBar.tsxsrc/components/SuccessPage.tsxsrc/components/TradingViewChart.tsxsrc/components/VerifyEmail.tsxsrc/components/ui/AuthLayout/AuthLayout.tsxsrc/components/ui/ErrorBoundary/ErrorBoundary.tsxsrc/components/ui/ErrorBoundary/index.tssrc/constants/routes.constants.tssrc/features/auth/AuthGuard.tsxsrc/features/auth/auth.service.tssrc/features/auth/auth.types.tssrc/features/dashboard/DashboardPage.tsxsrc/features/globe/GlobeView.tsxsrc/features/globe/globe.constants.tssrc/features/globe/nations.constants.tssrc/features/globe/useGlobe.tssrc/index.csssrc/lib/api.client.tssrc/main.tsxsrc/pages/LiveMarketDashboard.tsxsrc/services/market.service.tssrc/services/portfolio.service.tssrc/stores/userStore.tssrc/styles/components/auth-layout.csssrc/styles/components/country-list.csssrc/styles/components/dashboard.csssrc/styles/components/live-market.csssrc/styles/components/overlay.csssrc/styles/components/portfolio.csssrc/styles/components/sidebar.csssrc/styles/components/stock-search-bar.csssrc/styles/reset.csssrc/styles/tokens.csssrc/test/Login.test.tsxsrc/test/setup.tssrc/types.d.tssrc/types/env.d.tssrc/types/globe.types.tstsconfig.app.jsontsconfig.jsontsconfig.node.jsonvite.config.tsvitest.config.ts
| @@ -0,0 +1,321 @@ | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Auth forms log raw Axios errors containing credentials/PII to the console. Each of these catch blocks calls console.error('...', err) with the full Axios error object; err.config.data embeds the exact request body sent to the backend (passwords, reset tokens, emails). If any error-tracking or session-replay tool mirrors console.error, these secrets get shipped off-device.
src/components/Login.tsx#L60-68: replaceconsole.error('Login failed', err)with a sanitized log (e.g.err.message/err.response?.status), sinceerr.config.datahere contains the plaintext password.src/components/Register.tsx#L34-34: replaceconsole.error('Registration failed', err)the same way —err.config.datacontains{ username, email, password }.src/components/ResetPassword.tsx#L40-40: replaceconsole.error('Reset failed', err)the same way —err.config.datacontains{ token, new_password }, the most sensitive combination here.src/components/ForgotPassword.tsx#L24-24: replaceconsole.error('Forgot password failed', err)the same way —err.config.datacontains{ email }.
🔧 Example fix pattern (apply per site)
- console.error('Login failed', err);
+ console.error('Login failed', err?.response?.status, err?.message);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Login.tsx` at line 1, Replace the raw Axios error objects
passed to console.error in the Login, Register, ResetPassword, and
ForgotPassword catch blocks with sanitized details such as err.message and
err.response?.status. Preserve each existing failure label while ensuring
request configuration, bodies, credentials, tokens, and other PII are never
logged.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/pages/LiveMarketDashboard.tsx (2)
240-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConnection status text isn't announced to assistive tech.
The
isConnectedmessage changes dynamically but has noaria-liveregion, so screen reader users won't be notified when the stream disconnects/reconnects.♻️ Suggested tweak
- <span style={{ fontSize: '13px', color: isConnected ? '`#666666`' : '`#ff4444`' }}> + <span role="status" aria-live="polite" style={{ fontSize: '13px', color: isConnected ? '`#666666`' : '`#ff4444`' }}> {isConnected🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` around lines 240 - 244, Update the connection status span in LiveMarketDashboard to expose its dynamic isConnected message through an appropriate aria-live region, preserving the existing text and styling so screen readers announce disconnect and reconnect changes.
79-86: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider adding jitter to the reconnect backoff.
Pure exponential backoff (
1000 * 2^attempts) without jitter means all clients reconnecting after a shared server restart retry in lockstep, which can create a reconnect spike on the backend. Adding randomized jitter is a low-cost improvement.♻️ Suggested tweak
- const delay = Math.min(1000 * Math.pow(2, reconnectAttempts), MAX_RECONNECT_DELAY); + const baseDelay = Math.min(1000 * Math.pow(2, reconnectAttempts), MAX_RECONNECT_DELAY); + const delay = baseDelay / 2 + Math.random() * (baseDelay / 2); reconnectAttempts++; reconnectTimeout = setTimeout(connect, delay);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/LiveMarketDashboard.tsx` around lines 79 - 86, Update the reconnect scheduling in the WebSocket onclose handler to add randomized jitter to the exponential delay before applying MAX_RECONNECT_DELAY. Preserve the existing reconnectAttempts increment, reconnect timeout assignment, and exponential backoff behavior while ensuring clients do not retry in lockstep.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/pages/LiveMarketDashboard.tsx`:
- Around line 240-244: Update the connection status span in LiveMarketDashboard
to expose its dynamic isConnected message through an appropriate aria-live
region, preserving the existing text and styling so screen readers announce
disconnect and reconnect changes.
- Around line 79-86: Update the reconnect scheduling in the WebSocket onclose
handler to add randomized jitter to the exponential delay before applying
MAX_RECONNECT_DELAY. Preserve the existing reconnectAttempts increment,
reconnect timeout assignment, and exponential backoff behavior while ensuring
clients do not retry in lockstep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c9d35f2-8cb6-46ed-b86e-6e4b16c544a1
📒 Files selected for processing (11)
.github/workflows/ci.yml.gitignoresrc/App.tsxsrc/components/ForgotPassword.tsxsrc/components/Login.tsxsrc/components/Register.tsxsrc/components/ResetPassword.tsxsrc/lib/api.client.tssrc/pages/LiveMarketDashboard.tsxsrc/stores/userStore.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- src/App.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- .github/workflows/ci.yml
- .gitignore
- src/stores/userStore.ts
- src/components/ForgotPassword.tsx
- src/components/ResetPassword.tsx
- src/components/Login.tsx
- src/lib/api.client.ts
- src/components/Register.tsx
2 participants
Summary by CodeRabbit