Fro 7 chart printing#10
Conversation
feat:started authentication
…rs, and hash params
chore:configured frontend to backend connection
created home page and navigations
…-rate setup-the-dashbaord-perfectly
…re-integration feat(api): update API client for shared infrastructure microservices
…ervice feat(frontend): connect market features to market service
…landing page design tokens
…debar on explore page for authenticated users
… to guest explore page
…avoid scientific notation
feat:FRO-6/stock search and ui change
📝 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 | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable checkout credential persistence for PR code.
The workflow runs npm ci and repository-controlled commands after checkout, while the default checkout token remains in Git config. A dependency lifecycle script or test could read and misuse it. Set persist-credentials: false and explicitly grant only read access.
🔐 Proposed hardening
+permissions:
+ contents: read
+
jobs:
quality:
...
- name: Checkout code
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 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.
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 15-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/ci.yml around lines 15 - 16, Harden the actions/checkout
step by setting persist-credentials to false and configuring the workflow
permissions to grant only read access, while preserving the existing checkout
behavior.
Source: Linters/SAST tools
| # Environment variables — NEVER commit real .env files | ||
| .env | ||
| .envrc | ||
| .venv | ||
| env/ | ||
| venv/ | ||
| ENV/ | ||
| env.bak/ | ||
| venv.bak/ | ||
|
|
||
| # Spyder project settings | ||
| .spyderproject | ||
| .spyproject | ||
|
|
||
| # Rope project settings | ||
| .ropeproject | ||
|
|
||
| # mkdocs documentation | ||
| /site | ||
|
|
||
| # mypy | ||
| .mypy_cache/ | ||
| .dmypy.json | ||
| dmypy.json | ||
|
|
||
| # Pyre type checker | ||
| .pyre/ | ||
|
|
||
| # pytype static type analyzer | ||
| .pytype/ | ||
|
|
||
| # Cython debug symbols | ||
| cython_debug/ | ||
|
|
||
| # PyCharm | ||
| # JetBrains specific template is maintained in a separate JetBrains.gitignore that can | ||
| # be found at https://github.com/github/gitignore/blob/main/Global/JetBrains.gitignore | ||
| # and can be added to the global gitignore or merged into this file. For a more nuclear | ||
| # option (not recommended) you can uncomment the following to ignore the entire idea folder. | ||
| # .idea/ | ||
|
|
||
| # Abstra | ||
| # Abstra is an AI-powered process automation framework. | ||
| # Ignore directories containing user credentials, local state, and settings. | ||
| # Learn more at https://abstra.io/docs | ||
| .abstra/ | ||
|
|
||
| # Visual Studio Code | ||
| # Visual Studio Code specific template is maintained in a separate VisualStudioCode.gitignore | ||
| # that can be found at https://github.com/github/gitignore/blob/main/Global/VisualStudioCode.gitignore | ||
| # and can be added to the global gitignore or merged into this file. However, if you prefer, | ||
| # you could uncomment the following to ignore the entire vscode folder | ||
| # .vscode/ | ||
| # Temporary file for partial code execution | ||
| tempCodeRunnerFile.py | ||
|
|
||
| # Ruff stuff: | ||
| .ruff_cache/ | ||
|
|
||
| # PyPI configuration file | ||
| .pypirc | ||
|
|
||
| # Marimo | ||
| marimo/_static/ | ||
| marimo/_lsp/ | ||
| __marimo__/ | ||
| .env.local | ||
| .env.*.local |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Ignore all real environment-file variants.
.env.development, .env.production, and similar files are not ignored by the current patterns, so secrets in those files can be committed despite the file’s warning. Ignore .env.* and explicitly re-include .env.example.
🔒 Proposed fix
# Environment variables — NEVER commit real .env files
.env
-.env.local
-.env.*.local
+.env.*
+!.env.example📝 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.
| # Environment variables — NEVER commit real .env files | |
| .env | |
| .envrc | |
| .venv | |
| env/ | |
| venv/ | |
| ENV/ | |
| env.bak/ | |
| venv.bak/ | |
| # Spyder project settings | |
| .spyderproject | |
| .spyproject | |
| # Rope project settings | |
| .ropeproject | |
| # mkdocs documentation | |
| /site | |
| # mypy | |
| .mypy_cache/ | |
| .dmypy.json | |
| dmypy.json | |
| # Pyre type checker | |
| .pyre/ | |
| # pytype static type analyzer | |
| .pytype/ | |
| # Cython debug symbols | |
| cython_debug/ | |
| # PyCharm | |
| # JetBrains specific template is maintained in a separate JetBrains.gitignore that can | |
| # be found at https://github.com/github/gitignore/blob/main/Global/JetBrains.gitignore | |
| # and can be added to the global gitignore or merged into this file. For a more nuclear | |
| # option (not recommended) you can uncomment the following to ignore the entire idea folder. | |
| # .idea/ | |
| # Abstra | |
| # Abstra is an AI-powered process automation framework. | |
| # Ignore directories containing user credentials, local state, and settings. | |
| # Learn more at https://abstra.io/docs | |
| .abstra/ | |
| # Visual Studio Code | |
| # Visual Studio Code specific template is maintained in a separate VisualStudioCode.gitignore | |
| # that can be found at https://github.com/github/gitignore/blob/main/Global/VisualStudioCode.gitignore | |
| # and can be added to the global gitignore or merged into this file. However, if you prefer, | |
| # you could uncomment the following to ignore the entire vscode folder | |
| # .vscode/ | |
| # Temporary file for partial code execution | |
| tempCodeRunnerFile.py | |
| # Ruff stuff: | |
| .ruff_cache/ | |
| # PyPI configuration file | |
| .pypirc | |
| # Marimo | |
| marimo/_static/ | |
| marimo/_lsp/ | |
| __marimo__/ | |
| .env.local | |
| .env.*.local | |
| # Environment variables — NEVER commit real .env files | |
| .env | |
| .env.* | |
| !.env.example |
🤖 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 @.gitignore around lines 15 - 18, Update the environment-file ignore patterns
in .gitignore to ignore all .env variants, including .env.development and
.env.production, while explicitly re-including .env.example so the template
remains trackable.
| const initializeSession = async () => { | ||
| // Extract Google OAuth token from URL hash if present on the callback route | ||
| const hash = window.location.hash; | ||
| if (hash.includes('/auth/callback') && hash.includes('token=')) { | ||
| const tokenMatch = hash.match(/token=([^&]+)/); | ||
| if (tokenMatch && tokenMatch[1]) { | ||
| useUserStore.getState().setAccessToken(tokenMatch[1]); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Google OAuth access token travels through the URL hash.
The access token is parsed straight out of window.location.hash and stored in memory. Unlike the rest of the app's auth model (HttpOnly refresh cookie, per userStore.ts), this path briefly exposes a live access token in the browser URL/history before AuthCallback.tsx strips it via history.replaceState. Consider having the backend exchange a short-lived one-time code for the token server-side (or set the session via the same HttpOnly cookie mechanism used elsewhere) instead of embedding the access token directly in the redirect URL.
🤖 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 52 - 60, Update the Google OAuth callback flow
around initializeSession and AuthCallback.tsx to stop placing or parsing a live
access token from window.location.hash. Have the backend exchange a short-lived,
one-time authorization code and establish the existing HttpOnly refresh-cookie
session, then update the client auth state from that session while preserving
the callback redirect flow.
| await authService.forgotPassword(email); | ||
| setIsSuccess(true); | ||
| } catch (err) { | ||
| console.error('Forgot password failed', err); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Logging the full error exposes the submitted email.
err.config.data on this Axios error contains { email }. See consolidated comment for the shared fix across auth forms.
🤖 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/ForgotPassword.tsx` at line 24, Update the error logging in
the ForgotPassword component’s failure handler to avoid logging the full Axios
error, which can expose err.config.data and the submitted email. Log only a
safe, non-sensitive error detail or sanitized message, consistent with the
shared auth-form logging approach.
| @@ -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.
| const handleResponseError = (client: any) => async (error: any) => { | ||
| const originalRequest = error.config; | ||
|
|
||
| if (error.response?.status === 401 && !originalRequest._retry) { | ||
| if (originalRequest.url?.includes('/auth/refresh')) { | ||
| useUserStore.getState().logout(); | ||
| return Promise.reject(error); | ||
| } | ||
|
|
||
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| const { data } = await axios.post(`${API_URL}/auth/refresh`, {}, { withCredentials: true }); | ||
| const newAccessToken = data.access_token; | ||
| useUserStore.getState().setAccessToken(newAccessToken); | ||
|
|
||
| originalRequest.headers.Authorization = `Bearer ${newAccessToken}`; | ||
| return client(originalRequest); | ||
| } catch (refreshError) { | ||
| useUserStore.getState().logout(); | ||
| return Promise.reject(refreshError); | ||
| } | ||
| } | ||
|
|
||
| return Promise.reject(error); | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Concurrent 401s trigger parallel, un-deduplicated refresh calls; refresh call also has no timeout.
Every failing request independently calls /auth/refresh (Line 50). If several requests 401 at once (common when multiple portfolio/market calls fire together), each starts its own refresh; if the backend rotates the refresh cookie, only the first succeeds and the rest fail, forcing an unnecessary logout. The raw axios.post here also omits the timeout used by the configured clients (Lines 10, 16), so a stalled auth server can hang this retry path indefinitely.
🔒 Proposed fix: single in-flight refresh + timeout
+let refreshPromise: Promise<string> | null = null;
+
const handleResponseError = (client: any) => async (error: any) => {
const originalRequest = error.config;
if (error.response?.status === 401 && !originalRequest._retry) {
if (originalRequest.url?.includes('/auth/refresh')) {
useUserStore.getState().logout();
return Promise.reject(error);
}
originalRequest._retry = true;
try {
- const { data } = await axios.post(`${API_URL}/auth/refresh`, {}, { withCredentials: true });
- const newAccessToken = data.access_token;
+ if (!refreshPromise) {
+ refreshPromise = axios
+ .post(`${API_URL}/auth/refresh`, {}, { withCredentials: true, timeout: 5000 })
+ .then(({ data }) => data.access_token as string)
+ .finally(() => {
+ refreshPromise = null;
+ });
+ }
+ const newAccessToken = await refreshPromise;
useUserStore.getState().setAccessToken(newAccessToken);
originalRequest.headers.Authorization = `Bearer ${newAccessToken}`;
return client(originalRequest);
} catch (refreshError) {
useUserStore.getState().logout();
return Promise.reject(refreshError);
}
}
return Promise.reject(error);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleResponseError = (client: any) => async (error: any) => { | |
| const originalRequest = error.config; | |
| if (error.response?.status === 401 && !originalRequest._retry) { | |
| if (originalRequest.url?.includes('/auth/refresh')) { | |
| useUserStore.getState().logout(); | |
| return Promise.reject(error); | |
| } | |
| originalRequest._retry = true; | |
| try { | |
| const { data } = await axios.post(`${API_URL}/auth/refresh`, {}, { withCredentials: true }); | |
| const newAccessToken = data.access_token; | |
| useUserStore.getState().setAccessToken(newAccessToken); | |
| originalRequest.headers.Authorization = `Bearer ${newAccessToken}`; | |
| return client(originalRequest); | |
| } catch (refreshError) { | |
| useUserStore.getState().logout(); | |
| return Promise.reject(refreshError); | |
| } | |
| } | |
| return Promise.reject(error); | |
| }; | |
| let refreshPromise: Promise<string> | null = null; | |
| const handleResponseError = (client: any) => async (error: any) => { | |
| const originalRequest = error.config; | |
| if (error.response?.status === 401 && !originalRequest._retry) { | |
| if (originalRequest.url?.includes('/auth/refresh')) { | |
| useUserStore.getState().logout(); | |
| return Promise.reject(error); | |
| } | |
| originalRequest._retry = true; | |
| try { | |
| if (!refreshPromise) { | |
| refreshPromise = axios | |
| .post(`${API_URL}/auth/refresh`, {}, { withCredentials: true, timeout: 5000 }) | |
| .then(({ data }) => data.access_token as string) | |
| .finally(() => { | |
| refreshPromise = null; | |
| }); | |
| } | |
| const newAccessToken = await refreshPromise; | |
| useUserStore.getState().setAccessToken(newAccessToken); | |
| originalRequest.headers.Authorization = `Bearer ${newAccessToken}`; | |
| return client(originalRequest); | |
| } catch (refreshError) { | |
| useUserStore.getState().logout(); | |
| return Promise.reject(refreshError); | |
| } | |
| } | |
| return Promise.reject(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/lib/api.client.ts` around lines 38 - 63, Update handleResponseError to
reuse a single in-flight refresh promise so concurrent 401 responses share one
/auth/refresh request instead of starting parallel refreshes. Add the configured
client timeout to that refresh request, clear the shared promise when it
settles, and preserve the existing token update, request retry, and logout
behavior for refresh failures.
| useEffect(() => { | ||
| const ws = new WebSocket(`${MARKET_WS_URL}/dashboard/ws/market`); | ||
|
|
||
| ws.onmessage = (event) => { | ||
| try { | ||
| const liveData = JSON.parse(event.data); | ||
| setMarketData(liveData); | ||
| } catch (err) { | ||
| console.error('Error parsing websocket message', err); | ||
| } | ||
| }; | ||
|
|
||
| return () => ws.close(); | ||
| }, []); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
No onerror/onclose handling or reconnect for the market stream.
If the socket drops (server restart, network blip, proxy idle timeout), the UI keeps showing the last snapshot and the "Live WebSocket Market Stream Active" banner on line 202 while nothing is actually streaming. At minimum surface a disconnected state; ideally reconnect with backoff.
🤖 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 50 - 63, Update the market
WebSocket lifecycle in the useEffect within LiveMarketDashboard to track
connection status and handle both onerror and onclose, so the UI no longer
presents the stream as active after disconnection. Add reconnection with bounded
backoff when the socket closes, clear pending timers during cleanup, and ensure
the existing message parsing and state updates remain intact.
| ws.onmessage = (event) => { | ||
| try { | ||
| const liveData = JSON.parse(event.data); | ||
| setMarketData(liveData); | ||
| } catch (err) { | ||
| console.error('Error parsing websocket message', err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unvalidated WebSocket payload can crash the page.
setMarketData(liveData) accepts whatever parses. Any message that isn't { gainers, losers } (heartbeat, error frame, schema drift) leaves marketData.gainers undefined, and filterStocks on line 121 then calls .filter on undefined — an uncaught render-time TypeError that blanks the dashboard. Validate before setting.
🛡️ Proposed fix
try {
const liveData = JSON.parse(event.data);
- setMarketData(liveData);
+ if (!Array.isArray(liveData?.gainers) || !Array.isArray(liveData?.losers)) return;
+ setMarketData({ gainers: liveData.gainers, losers: liveData.losers });
} catch (err) {📝 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.
| ws.onmessage = (event) => { | |
| try { | |
| const liveData = JSON.parse(event.data); | |
| setMarketData(liveData); | |
| } catch (err) { | |
| console.error('Error parsing websocket message', err); | |
| } | |
| }; | |
| ws.onmessage = (event) => { | |
| try { | |
| const liveData = JSON.parse(event.data); | |
| if (!Array.isArray(liveData?.gainers) || !Array.isArray(liveData?.losers)) return; | |
| setMarketData({ gainers: liveData.gainers, losers: liveData.losers }); | |
| } catch (err) { | |
| console.error('Error parsing websocket message', err); | |
| } | |
| }; |
🤖 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 53 - 60, Validate the parsed
payload in the WebSocket onmessage handler before calling setMarketData: require
the expected object shape with valid gainers and losers collections, and ignore
or log invalid frames such as heartbeats or error messages. Keep setMarketData
limited to validated market data so filterStocks always receives defined
collections.
| initAuth: async () => { | ||
| try { | ||
| // If we have a user in localStorage, we can optimistically say they might be authenticated. | ||
| // But to be sure, and to get the access token in memory, we fetch their profile. | ||
| // Since there is no access token in memory yet, the Axios interceptor will hit a 401 | ||
| // when it tries to get /auth/me, which will trigger the /auth/refresh flow automatically! | ||
| const user = await authService.getMe(); | ||
| set({ user, isAuthenticated: true }); | ||
| } catch (error) { | ||
| // If refresh fails, clear the state | ||
| set({ user: null, isAuthenticated: false, accessToken: null }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
initAuth logs the user out on any error, not just auth failures.
If authService.getMe() fails due to a transient network error or a 5xx from the backend, this catch block clears user, isAuthenticated, and accessToken just as it would for a genuine 401/expired-session — silently ending a valid session on unrelated failures. This also explains the flagged unused error param: the fix needs it.
🔧 Proposed fix
initAuth: async () => {
try {
const user = await authService.getMe();
set({ user, isAuthenticated: true });
- } catch (error) {
- // If refresh fails, clear the state
- set({ user: null, isAuthenticated: false, accessToken: null });
+ } catch (error: any) {
+ // Only clear the session on an explicit auth rejection.
+ // Transient network/server errors should not silently sign the user out.
+ if (error?.response?.status === 401) {
+ set({ user: null, isAuthenticated: false, accessToken: null });
+ } else {
+ console.error('initAuth: non-auth error while restoring session', error);
+ }
}
},📝 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.
| initAuth: async () => { | |
| try { | |
| // If we have a user in localStorage, we can optimistically say they might be authenticated. | |
| // But to be sure, and to get the access token in memory, we fetch their profile. | |
| // Since there is no access token in memory yet, the Axios interceptor will hit a 401 | |
| // when it tries to get /auth/me, which will trigger the /auth/refresh flow automatically! | |
| const user = await authService.getMe(); | |
| set({ user, isAuthenticated: true }); | |
| } catch (error) { | |
| // If refresh fails, clear the state | |
| set({ user: null, isAuthenticated: false, accessToken: null }); | |
| } | |
| }, | |
| initAuth: async () => { | |
| try { | |
| // If we have a user in localStorage, we can optimistically say they might be authenticated. | |
| // But to be sure, and to get the access token in memory, we fetch their profile. | |
| // Since there is no access token in memory yet, the Axios interceptor will hit a 401 | |
| // when it tries to get /auth/me, which will trigger the /auth/refresh flow automatically! | |
| const user = await authService.getMe(); | |
| set({ user, isAuthenticated: true }); | |
| } catch (error: any) { | |
| // Only clear the session on an explicit auth rejection. | |
| // Transient network/server errors should not silently sign the user out. | |
| if (error?.response?.status === 401) { | |
| set({ user: null, isAuthenticated: false, accessToken: null }); | |
| } else { | |
| console.error('initAuth: non-auth error while restoring session', error); | |
| } | |
| } | |
| }, |
🧰 Tools
🪛 GitHub Check: Type-check · Lint · Format · Test · Build
[warning] 57-57: eslint(no-unused-vars)
Catch parameter 'error' is caught but never used.
🤖 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/stores/userStore.ts` around lines 49 - 61, Update initAuth so only
authentication failures from authService.getMe() clear user, isAuthenticated,
and accessToken; preserve the existing session state for transient network
errors and backend 5xx responses. Use the caught error parameter to classify the
failure according to the project’s existing HTTP/auth error handling
conventions, and handle genuine 401 or expired-session responses with the
current logout state update.
Source: Linters/SAST tools
| // Only collect coverage from source files | ||
| include: ['src/**/*.{ts,tsx}'], | ||
| exclude: [ | ||
| 'src/test/**', | ||
| 'src/**/*.d.ts', | ||
| 'src/**/*.types.ts', | ||
| 'src/**/*.constants.ts', | ||
| 'src/main.tsx', | ||
| ], | ||
| // Minimum coverage thresholds — CI will fail if these are not met | ||
| thresholds: { | ||
| statements: 60, | ||
| branches: 60, | ||
| functions: 60, | ||
| lines: 60, | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Align coverage thresholds with the actual test scope.
The coverage glob includes almost every src/**/*.ts and src/**/*.tsx file, while the provided suite only covers Login. With the dashboard, globe, portfolio, routing, and service modules also included, the 60% thresholds will fail CI unless substantial additional tests exist. Either add coverage for the included modules or narrow the coverage scope intentionally.
🤖 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 `@vitest.config.ts` around lines 18 - 33, Align the Vitest coverage
configuration with the currently tested scope: either add tests covering the
included dashboard, globe, portfolio, routing, and service modules, or
intentionally narrow coverage.include and related exclusions to the
Login-covered sources. Ensure the configured 60% thresholds can be met without
unintentionally omitting files that should remain in scope.
3 participants
Summary by CodeRabbit
New Features
Documentation
Tests