feat:started authentication#1
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
🧹 Nitpick comments (18)
src/features/globe/useGlobe.ts (4)
82-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
.fill([0, 0, 0])shares one array reference across all initial slots.
Array.from({ length: N }).fill([0, 0, 0])fills every index with the same array object reference, not independent tuples. This happens to be harmless today only becauseanimate()runs synchronously before the firstpathsData()call and immediately overwrites every slot (Line 105) — but it's a well-known JS footgun that will silently break if the initial-render logic changes.🛡️ Safer pre-allocation
- const pathCoordinateBuffers: Coordinate[][] = NATIONS.map(() => - Array.from<Coordinate>({ length: GLOBE_CONFIG.PATH_POINTS }).fill([0, 0, 0]), - ); + const pathCoordinateBuffers: Coordinate[][] = NATIONS.map(() => + Array.from<Coordinate>({ length: GLOBE_CONFIG.PATH_POINTS }, () => [0, 0, 0]), + );🤖 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 82 - 84, The pre-allocation in useGlobe currently uses Array.from(...).fill([0, 0, 0]), which reuses the same coordinate array reference for every slot. Update the pathCoordinateBuffers initialization so each position gets its own independent Coordinate tuple/array, keeping the logic in useGlobe and animate() unchanged but avoiding shared mutable state.
81-111: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAnimation loop still allocates a new array per point per frame, defeating the stated pre-allocation goal.
The comment at Line 81 says buffers are pre-allocated "to avoid 60fps garbage collection pressure," but Line 105 does
coords[i] = [nation.lat + ..., nation.lng + ..., alt]— a new array literal replacing the slot every frame, for every nation ×PATH_POINTS(14 × 15 = 210 allocations/frame at 60fps). Mutate the existing tuple in place instead.⚡ Proposed fix
const alt = Math.max( 0.01, GLOBE_CONFIG.PATH_BASE_ALTITUDE + Math.sin(i * 0.5 + t * 4) * GLOBE_CONFIG.PATH_WAVE_AMPLITUDE + Math.cos(i * 1.2 - t * 2) * 0.012, ); - coords[i] = [nation.lat + Math.sin(i) * 0.5, nation.lng + lngOffset, alt]; + const point = coords[i]; + point[0] = nation.lat + Math.sin(i) * 0.5; + point[1] = nation.lng + lngOffset; + point[2] = alt;🤖 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 81 - 111, The animation loop in useGlobe still creates a new Coordinate array for every point on each frame, which defeats the pre-allocation in pathCoordinateBuffers. Update animate() so it reuses the existing per-point buffer entries instead of assigning a fresh tuple to coords[i], mutating the stored Coordinate values in place while keeping world.pathsData(chartBuffer) fed from the same preallocated arrays.
50-59: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGlobe textures hotlinked from unpkg without fallback.
globeImageUrl/bumpImageUrl/backgroundImageUrlpoint atunpkg.com/three-globe/example/...assets. unpkg is a CDN meant for package distribution, not guaranteed for high-traffic production hotlinking, and there's noonload/error handling if these fail — the globe would silently render untextured. Consider self-hosting these assets (same as the GeoJSON TODO at Line 132) or adding a fallback.🤖 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 50 - 59, The Globe setup in useGlobe currently hotlinks textures via globeImageUrl, bumpImageUrl, and backgroundImageUrl from unpkg without any fallback or error handling. Update the Globe initialization to use self-hosted/local asset URLs (consistent with the existing GeoJSON self-hosting approach) or add a fallback path if the remote textures fail to load, so the texture setup remains reliable in production.
133-135: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo
res.okcheck before parsing the GeoJSON response.If the unpkg fetch returns a non-2xx status (e.g. 404/503),
res.json()will still be attempted and either throw an opaque parse error or resolve unexpectedly, rather than surfacing a clear "failed to fetch boundaries" error via the existing.catch.🤖 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 133 - 135, The GeoJSON load path in useGlobe should validate the fetch response before calling res.json(), since fetch can resolve on non-2xx statuses and produce unclear parse failures. Update the promise chain around the unpkg request in useGlobe to check the response status first, then either parse the body or throw a clear "failed to fetch boundaries" error so the existing catch handles it consistently.src/types/globe.types.ts (1)
11-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnused, name-colliding
GlobeInstance/GlobeControlsduplicate the real library types.
useGlobe.tsimportsGlobeInstancedirectly from'globe.gl'(via the real bundled declarations re-exported insrc/types.d.ts), not from here — this hand-rolledGlobeInstance/GlobeControlspair (lines 11-54) is unused dead code. Having two differently-shaped types both namedGlobeInstanceacross the codebase is confusing for future maintainers. Recommend dropping these two interfaces and keeping only the app-specific types (Coordinate,PathData,LabelData,GeoFeature,PointOfView,GeoJSON).♻️ Proposed cleanup
-export interface GlobeControls { - autoRotate: boolean; - autoRotateSpeed: number; - enableZoom: boolean; -} - -export interface GlobeInstance { - // Image layers - globeImageUrl(url: string): GlobeInstance; - bumpImageUrl(url: string): GlobeInstance; - backgroundImageUrl(url: string): GlobeInstance; - - // Paths (stock chart lines) - pathsData(data: PathData[]): GlobeInstance; - pathPoints(key: string): GlobeInstance; - pathPointLat(fn: (p: Coordinate) => number): GlobeInstance; - pathPointLng(fn: (p: Coordinate) => number): GlobeInstance; - pathPointAlt(fn: (p: Coordinate) => number): GlobeInstance; - pathColor(fn: (d: PathData) => string): GlobeInstance; - pathStroke(value: number): GlobeInstance; - - // Labels (country name overlays) - labelsData(data: LabelData[]): GlobeInstance; - labelLat(fn: (d: LabelData) => number): GlobeInstance; - labelLng(fn: (d: LabelData) => number): GlobeInstance; - labelText(fn: (d: LabelData) => string): GlobeInstance; - labelSize(size: number): GlobeInstance; - labelColor(fn: (d: LabelData) => string): GlobeInstance; - labelDotRadius(r: number): GlobeInstance; - - // Polygons (country boundary fills) - polygonsData(data: GeoFeature[]): GlobeInstance; - polygonCapColor(fn: (d: GeoFeature) => string): GlobeInstance; - polygonSideColor(fn: (d: GeoFeature) => string): GlobeInstance; - polygonStrokeColor(fn: (d: GeoFeature) => string): GlobeInstance; - polygonLabel(fn: (d: GeoFeature) => string): GlobeInstance; - onPolygonHover(fn: (hoverD: GeoFeature | null) => void): GlobeInstance; - - // Camera - pointOfView(pov: Partial<PointOfView>, durationMs?: number): GlobeInstance; - width(w: number): GlobeInstance; - height(h: number): GlobeInstance; - controls(): GlobeControls; -} -🤖 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/globe.types.ts` around lines 11 - 54, Remove the unused hand-rolled GlobeInstance and GlobeControls interfaces from globe.types.ts, since useGlobe.ts relies on the real GlobeInstance type re-exported from globe.gl. Keep only the app-specific type definitions (Coordinate, PathData, LabelData, GeoFeature, PointOfView, GeoJSON) and update any local references so there is no duplicate name-colliding GlobeInstance in the codebase.src/styles/tokens.css (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStylelint casing hints on font names — safe to ignore.
Font family proper nouns (
BlinkMacSystemFont,Roboto,Helvetica,Arial,Consolas) are conventionally kept in their canonical case; lowercasing per thevalue-keyword-caserule would hurt readability with no functional benefit.🤖 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/tokens.css` around lines 33 - 35, No code change is needed here: the font-family tokens in tokens.css use canonical proper-noun casing (for example BlinkMacSystemFont, Roboto, Helvetica, Arial, and Consolas), so keep the existing values in the --font-family-base and --font-family-mono declarations as-is and do not apply value-keyword-case lowercasing.Source: Linters/SAST tools
src/components/ui/ErrorBoundary/ErrorBoundary.tsx (1)
35-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTODO: wire up real error tracking.
componentDidCatchonly logs to console per the TODO. Want me to help integrate Sentry (or another tracker) here, or open a tracking issue for this?🤖 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` around lines 35 - 39, componentDidCatch in ErrorBoundary is still only writing to console, so replace the placeholder logging with real error tracking integration. Update ErrorBoundary.componentDidCatch to send the caught error and info.componentStack to the chosen service (for example Sentry) with the existing context preserved, and keep console logging only as a fallback if needed.src/index.css (1)
2-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded colors/fonts bypass the design-token system.
tokens.cssdocuments that "All design values are defined here as CSS custom properties. Use these variables throughout all component CSS files. DO NOT hard-code raw color values in component styles." Lines 5, 6, and 8 hardcode#000, a font stack, and#fffinstead of--color-bg-primary,--font-family-base, and--color-text-primary.♻️ Proposed fix — use design tokens
body { margin: 0; padding: 0; - background-color: `#000`; - font-family: 'Segoe UI', Tahoma, Geneva, Verdana, sans-serif; + background-color: var(--color-bg-primary); + font-family: var(--font-family-base); overflow: hidden; - color: `#fff`; + color: var(--color-text-primary); }🤖 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 2 - 9, The global `body` styles in `src/index.css` are hardcoding theme values instead of using the design-token system. Update the `body` rule to use the existing CSS custom properties from `tokens.css` for background, font family, and text color, specifically replacing the raw color and font stack with the corresponding `--color-bg-primary`, `--font-family-base`, and `--color-text-primary` tokens.src/styles/reset.css (1)
17-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStylelint casing hint is a style-only nit; skip.
optimizeLegibilityat line 22 is the canonical CSS value spelling; the stylelintvalue-keyword-caserule flags it purely for casing convention with no functional effect.🤖 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/reset.css` around lines 17 - 23, No code change is needed here: the `body` reset in `reset.css` already uses the canonical `text-rendering: optimizeLegibility` value, and the `value-keyword-case` warning is only about casing style. Leave the `body` rule as-is and do not alter `optimizeLegibility`.Source: Linters/SAST tools
src/features/auth/auth.service.ts (1)
71-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the registration payload instead of
any.Every other method here has a typed request/response;
register(data: any)loses type safety on a critical auth entry point. Define aRegisterRequestinterface (mirroring the backend schema) and use it here.♻️ Proposed fix
+export interface RegisterRequest { + username: string; + email: string; + password: string; +} + - 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 71 - 74, The register method in auth.service loses type safety by accepting any, unlike the other typed auth methods. Define a RegisterRequest interface that matches the backend registration payload and update AuthService.register to accept that type instead of any, keeping the existing UserResponse return type and using the register symbol to locate the change.src/features/dashboard/DashboardPage.tsx (1)
22-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove profile-card inline styles into
dashboard.css.The file already imports
dashboard.cssfor page-level classes; the profile-details block should follow the same pattern instead of large inlinestyleobjects, for consistency and easier theming.🤖 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/dashboard/DashboardPage.tsx` around lines 22 - 55, The profile details block in DashboardPage should stop using large inline style objects and be moved into dashboard.css to match the existing page-level styling pattern. Update the JSX in DashboardPage to use class names for the profile card, heading, and timestamp, and add the corresponding CSS rules in dashboard.css so the visual appearance stays the same while making the styles easier to maintain and theme.src/App.tsx (2)
16-16: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse selectors instead of subscribing to the whole store.
Destructuring the full store causes
AppContentto re-render on unrelated state changes (e.g.,accessTokenupdates during token refresh), even though onlyuser,logout, andinitAuthare used here.♻️ Suggested selector-based subscription
- const { logout, initAuth, user } = useUserStore(); + const logout = useUserStore((s) => s.logout); + const initAuth = useUserStore((s) => s.initAuth); + const user = useUserStore((s) => s.user);🤖 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` at line 16, The AppContent subscription is pulling the whole useUserStore state via destructuring, which causes unnecessary re-renders on unrelated updates. Update the store access in App.tsx to use selector-based subscriptions in AppContent so it only subscribes to logout, initAuth, and user, instead of the entire store. Keep the change localized to the useUserStore call and preserve the existing behavior of AppContent.
76-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading prop wiring:
onNavigateToDashboardnavigates to login, not dashboard.See related comment in
src/components/SuccessPage.tsx(Lines 5-8, 49-56) where this prop is defined and bound to a "Go to Sign In" button — the naming here should match that it routes toROUTES.LOGIN, not the 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 76 - 84, The `SuccessPage` prop wiring is misleading because `onNavigateToDashboard` in `App` actually navigates to `ROUTES.LOGIN`, not the dashboard. Rename the prop in `SuccessPage` and its usage sites to match its real behavior, or change the handler to point to the dashboard if that was intended; use the `SuccessPage` component and its `onNavigateToDashboard` prop definition to locate the related wiring.src/components/SuccessPage.tsx (2)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
onNavigateToDashboardto reflect actual behavior.The prop is always wired to navigate to the login screen (confirmed in
App.tsxLine 80, with the comment "Needs to login first"), and the button itself reads "Go to Sign In." The name implies dashboard navigation, which is misleading for future maintainers wiring this component.♻️ Suggested rename
interface SuccessPageProps { - onNavigateToDashboard: () => void; + onNavigateToLogin: () => void; onBackToLogin: () => void; } export const SuccessPage: React.FC<SuccessPageProps> = ({ - onNavigateToDashboard, + onNavigateToLogin, onBackToLogin, }) => {Also applies to: 49-56
🤖 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/SuccessPage.tsx` around lines 5 - 8, The SuccessPage props and usage are misleading because onNavigateToDashboard does not navigate to a dashboard and is actually wired to the login/sign-in flow. Rename the prop in SuccessPageProps and all call sites, including the handler passed from App.tsx and any button wiring in SuccessPage, to a name that reflects the real action (such as sign-in/login navigation). Keep the naming consistent across the component interface, destructuring, and any references so future maintainers can understand the behavior immediately.
16-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting inline styles into CSS classes.
This component leans heavily on inline
styleobjects (icon circle, button stack, tip card) instead of the class-based system already established inauth-layout.css. Moving these into named classes would keep styling consistent and easier to theme alongside the rest of the auth flow.🤖 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/SuccessPage.tsx` around lines 16 - 105, SuccessPage.tsx relies on several inline style objects, which should be moved to the existing CSS class-based styling approach. Extract the styles used in the main card, icon circle, button stack, and onboarding tip card into named classes in auth-layout.css, then update SuccessPage to reference those classes instead of inline style props. Keep the component structure and behavior the same while aligning the styling with the rest of the auth flow.src/styles/components/auth-layout.css (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope auth-specific tokens instead of polluting
:root.These variables are redefined at the global
:rootlevel in a component-scoped stylesheet. Since CSS imports are global side-effects, this can silently override or be overridden by tokens with the same names defined elsewhere (e.g., the app-widetokens.css), depending on import order.♻️ Suggested scoping
-:root { +.auth-screen-wrapper { --color-bg-left: `#0b1120`; --color-bg-right: `#f8fafc`; --color-text-dark: `#09090b`; --color-text-muted: `#64748b`; --color-text-grey: `#475569`; --color-brand-teal: `#0d9488`; --color-brand-teal-hover: `#0f766e`; --color-border: `#e2e8f0`; --color-border-hover: `#cbd5e1`; --color-input-focus: `#0d9488`; }🤖 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, The auth layout color tokens are being declared on the global :root inside a component stylesheet, which can leak into or collide with app-wide tokens. Move these custom properties into the auth-specific scope used by this stylesheet (for example the auth layout wrapper/class defined in auth-layout styles) and keep the existing variable names so only the auth layout consumes them. Use the :root token block in auth-layout.css and the auth layout container selector as the main places to update.src/components/ui/AuthLayout/AuthLayout.tsx (1)
18-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeyboard activation only handles Enter, not Space.
Elements with
role="button"are expected to activate on both Enter and Space per ARIA authoring practices. Consider using a native<button>element instead, which gets this for free and removes the need for manualrole/tabIndex/onKeyDownwiring.♻️ Suggested fix
- <div + <button + type="button" className="brand-header" onClick={() => navigate(ROUTES.HOME)} - role="button" - tabIndex={0} - onKeyDown={(e) => e.key === 'Enter' && navigate(ROUTES.HOME)} - style={{ cursor: 'pointer' }} aria-label="Go back to home" > <div className="brand-logo-icon"> <TrendingUp size={22} strokeWidth={2.5} /> </div> <span className="brand-logo-text">Copilot AI</span> - </div> + </button>🤖 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, The clickable brand header in AuthLayout only supports Enter via its onKeyDown handler, so update the `brand-header` interaction to also activate on Space, or बेहतर replace the div with a native button in `AuthLayout` so `role`, `tabIndex`, and keyboard handling are no longer manual. If keeping the current element, adjust the `onKeyDown` logic alongside `navigate(ROUTES.HOME)` so both Enter and Space trigger the same navigation behavior.src/test/Login.test.tsx (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
useUserStoreisn't mocked, sosetAccessToken/setUserhit the real Zustand store withpersist(localStorage) during tests.Since only
authServiceis mocked,Login's successful-login test path exercises the real store, including itspersistmiddleware writing tolocalStorage. This risks state leaking across test files/cases since the store singleton and localStorage aren't reset between tests, and isn't representative of a pure unit test forLogin. Consider mockinguseUserStoreas well.🤖 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/test/Login.test.tsx` around lines 8 - 13, The Login test setup only mocks authService, so the successful-login flow still uses the real useUserStore singleton and its persist/localStorage side effects. Update the test mocks to include useUserStore as well, and make its setAccessToken and setUser actions jest/vi fns so Login can be tested without touching Zustand persistence. Keep the mock aligned with the existing authService mock in Login.test.tsx so the unit test stays isolated and does not leak state across cases.
🤖 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.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 15-22: Disable persisted credentials in the Checkout code step by
updating the actions/checkout@v4 configuration to set persist-credentials to
false. This change belongs in the same workflow job that also uses Setup
Node.js, and the key symbol to locate is the Checkout code step under
actions/checkout@v4.
In `@src/components/Login.tsx`:
- Around line 37-54: The login flow in Login’s submit handler leaves a partial
authenticated session if authService.getMe() fails after setAccessToken()
succeeds. Update the catch path in the same handler to clear the auth state
(reset the access token and authenticated flag, and avoid leaving user state
set) before showing the error, so AuthGuard does not treat the failed login as
valid.
In `@src/features/globe/useGlobe.ts`:
- Around line 174-182: Cleanup in useGlobe currently clears the DOM but does not
fully release the globe.gl resources. Update the unmount cleanup in useGlobe to
dispose the globe instance first by calling world._destructor() before removing
event listeners and clearing the container, then null out
globeInstanceRef.current so renderer, scene, controls, and WebGL resources are
released properly.
- Around line 122-128: The wheel handler in useGlobe should preserve the current
camera state instead of resetting it on every scroll tick. Update handleWheel so
it reads the existing world point of view and only adjusts lng, or guard the
handler when a country is active, so flyTo and the current lat/altitude are not
immediately overwritten.
In `@src/index.css`:
- Around line 1-14: The global body styles in src/index.css are applying a
permanent scroll lock via body overflow hidden, which affects all routes loaded
from src/main.tsx. Remove the overflow hidden rule from the global stylesheet
and move the scroll-lock behavior into the specific fullscreen layout/component
that needs it, keeping Login/Register/Forgot Password/Reset Password/Dashboard
able to scroll when their content overflows. Use the body and `#root` styles in
src/index.css as the place to locate and adjust this change.
In `@src/lib/api.client.ts`:
- Around line 20-58: The response interceptor in apiClient is allowing multiple
401s to trigger separate /auth/refresh calls, which can race and break token
rotation handling. Update the refresh flow in the response interceptor to
serialize refresh attempts with a single shared in-flight promise/queue so
concurrent requests wait for the same refresh result, then retry using the new
token. Also add a timeout to the axios.post refresh call so a stalled backend
does not leave requests hanging indefinitely.
In `@src/stores/userStore.ts`:
- Around line 49-61: The initAuth catch block in userStore has an unused error
binding; use the caught error instead of ignoring it. Update the initAuth method
to log the failure from authService.getMe before clearing state, so silent
auth-init issues are debuggable, and keep the existing reset logic for user,
isAuthenticated, and accessToken in the catch path.
In `@src/styles/components/country-list.css`:
- Around line 85-91: The .country-indicator style uses the nonstandard casing
currentColor, which triggers the Stylelint warning. Update the box-shadow
declaration in the country list styles to use the lowercase currentcolor token
so the rule passes while preserving the same visual effect.
In `@src/test/Login.test.tsx`:
- Around line 18-23: The test helper in Login.test.tsx is passing an invalid
Login prop: replace onBackToHome with one of the supported LoginProps callbacks.
Update renderLogin to use the actual Login component API from Login.tsx (for
example, pass onNavigateToSignup or onNavigateToForgot if that is what the test
needs, or remove the extra prop entirely) so the JSX matches the component’s
defined props and TypeScript checks pass.
---
Nitpick comments:
In `@src/App.tsx`:
- Line 16: The AppContent subscription is pulling the whole useUserStore state
via destructuring, which causes unnecessary re-renders on unrelated updates.
Update the store access in App.tsx to use selector-based subscriptions in
AppContent so it only subscribes to logout, initAuth, and user, instead of the
entire store. Keep the change localized to the useUserStore call and preserve
the existing behavior of AppContent.
- Around line 76-84: The `SuccessPage` prop wiring is misleading because
`onNavigateToDashboard` in `App` actually navigates to `ROUTES.LOGIN`, not the
dashboard. Rename the prop in `SuccessPage` and its usage sites to match its
real behavior, or change the handler to point to the dashboard if that was
intended; use the `SuccessPage` component and its `onNavigateToDashboard` prop
definition to locate the related wiring.
In `@src/components/SuccessPage.tsx`:
- Around line 5-8: The SuccessPage props and usage are misleading because
onNavigateToDashboard does not navigate to a dashboard and is actually wired to
the login/sign-in flow. Rename the prop in SuccessPageProps and all call sites,
including the handler passed from App.tsx and any button wiring in SuccessPage,
to a name that reflects the real action (such as sign-in/login navigation). Keep
the naming consistent across the component interface, destructuring, and any
references so future maintainers can understand the behavior immediately.
- Around line 16-105: SuccessPage.tsx relies on several inline style objects,
which should be moved to the existing CSS class-based styling approach. Extract
the styles used in the main card, icon circle, button stack, and onboarding tip
card into named classes in auth-layout.css, then update SuccessPage to reference
those classes instead of inline style props. Keep the component structure and
behavior the same while aligning the styling with the rest of the auth flow.
In `@src/components/ui/AuthLayout/AuthLayout.tsx`:
- Around line 18-26: The clickable brand header in AuthLayout only supports
Enter via its onKeyDown handler, so update the `brand-header` interaction to
also activate on Space, or बेहतर replace the div with a native button in
`AuthLayout` so `role`, `tabIndex`, and keyboard handling are no longer manual.
If keeping the current element, adjust the `onKeyDown` logic alongside
`navigate(ROUTES.HOME)` so both Enter and Space trigger the same navigation
behavior.
In `@src/components/ui/ErrorBoundary/ErrorBoundary.tsx`:
- Around line 35-39: componentDidCatch in ErrorBoundary is still only writing to
console, so replace the placeholder logging with real error tracking
integration. Update ErrorBoundary.componentDidCatch to send the caught error and
info.componentStack to the chosen service (for example Sentry) with the existing
context preserved, and keep console logging only as a fallback if needed.
In `@src/features/auth/auth.service.ts`:
- Around line 71-74: The register method in auth.service loses type safety by
accepting any, unlike the other typed auth methods. Define a RegisterRequest
interface that matches the backend registration payload and update
AuthService.register to accept that type instead of any, keeping the existing
UserResponse return type and using the register symbol to locate the change.
In `@src/features/dashboard/DashboardPage.tsx`:
- Around line 22-55: The profile details block in DashboardPage should stop
using large inline style objects and be moved into dashboard.css to match the
existing page-level styling pattern. Update the JSX in DashboardPage to use
class names for the profile card, heading, and timestamp, and add the
corresponding CSS rules in dashboard.css so the visual appearance stays the same
while making the styles easier to maintain and theme.
In `@src/features/globe/useGlobe.ts`:
- Around line 82-84: The pre-allocation in useGlobe currently uses
Array.from(...).fill([0, 0, 0]), which reuses the same coordinate array
reference for every slot. Update the pathCoordinateBuffers initialization so
each position gets its own independent Coordinate tuple/array, keeping the logic
in useGlobe and animate() unchanged but avoiding shared mutable state.
- Around line 81-111: The animation loop in useGlobe still creates a new
Coordinate array for every point on each frame, which defeats the pre-allocation
in pathCoordinateBuffers. Update animate() so it reuses the existing per-point
buffer entries instead of assigning a fresh tuple to coords[i], mutating the
stored Coordinate values in place while keeping world.pathsData(chartBuffer) fed
from the same preallocated arrays.
- Around line 50-59: The Globe setup in useGlobe currently hotlinks textures via
globeImageUrl, bumpImageUrl, and backgroundImageUrl from unpkg without any
fallback or error handling. Update the Globe initialization to use
self-hosted/local asset URLs (consistent with the existing GeoJSON self-hosting
approach) or add a fallback path if the remote textures fail to load, so the
texture setup remains reliable in production.
- Around line 133-135: The GeoJSON load path in useGlobe should validate the
fetch response before calling res.json(), since fetch can resolve on non-2xx
statuses and produce unclear parse failures. Update the promise chain around the
unpkg request in useGlobe to check the response status first, then either parse
the body or throw a clear "failed to fetch boundaries" error so the existing
catch handles it consistently.
In `@src/index.css`:
- Around line 2-9: The global `body` styles in `src/index.css` are hardcoding
theme values instead of using the design-token system. Update the `body` rule to
use the existing CSS custom properties from `tokens.css` for background, font
family, and text color, specifically replacing the raw color and font stack with
the corresponding `--color-bg-primary`, `--font-family-base`, and
`--color-text-primary` tokens.
In `@src/styles/components/auth-layout.css`:
- Around line 5-16: The auth layout color tokens are being declared on the
global :root inside a component stylesheet, which can leak into or collide with
app-wide tokens. Move these custom properties into the auth-specific scope used
by this stylesheet (for example the auth layout wrapper/class defined in
auth-layout styles) and keep the existing variable names so only the auth layout
consumes them. Use the :root token block in auth-layout.css and the auth layout
container selector as the main places to update.
In `@src/styles/reset.css`:
- Around line 17-23: No code change is needed here: the `body` reset in
`reset.css` already uses the canonical `text-rendering: optimizeLegibility`
value, and the `value-keyword-case` warning is only about casing style. Leave
the `body` rule as-is and do not alter `optimizeLegibility`.
In `@src/styles/tokens.css`:
- Around line 33-35: No code change is needed here: the font-family tokens in
tokens.css use canonical proper-noun casing (for example BlinkMacSystemFont,
Roboto, Helvetica, Arial, and Consolas), so keep the existing values in the
--font-family-base and --font-family-mono declarations as-is and do not apply
value-keyword-case lowercasing.
In `@src/test/Login.test.tsx`:
- Around line 8-13: The Login test setup only mocks authService, so the
successful-login flow still uses the real useUserStore singleton and its
persist/localStorage side effects. Update the test mocks to include useUserStore
as well, and make its setAccessToken and setUser actions jest/vi fns so Login
can be tested without touching Zustand persistence. Keep the mock aligned with
the existing authService mock in Login.test.tsx so the unit test stays isolated
and does not leak state across cases.
In `@src/types/globe.types.ts`:
- Around line 11-54: Remove the unused hand-rolled GlobeInstance and
GlobeControls interfaces from globe.types.ts, since useGlobe.ts relies on the
real GlobeInstance type re-exported from globe.gl. Keep only the app-specific
type definitions (Coordinate, PathData, LabelData, GeoFeature, PointOfView,
GeoJSON) and update any local references so there is no duplicate name-colliding
GlobeInstance in the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6f6d6bb-2efa-4874-8776-53004f216f0c
⛔ 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 (51)
.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/ForgotPassword.tsxsrc/components/Login.tsxsrc/components/Register.tsxsrc/components/ResetPassword.tsxsrc/components/SuccessPage.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/stores/userStore.tssrc/styles/components/auth-layout.csssrc/styles/components/country-list.csssrc/styles/components/dashboard.csssrc/styles/components/overlay.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 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: '.nvmrc' | ||
| cache: 'npm' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' .github/workflows/ci.ymlRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 1353
🏁 Script executed:
sed -n '1,160p' .github/workflows/ci.ymlRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 1353
🏁 Script executed:
printf 'Workflow file:\n'
sed -n '1,220p' .github/workflows/ci.yml
printf '\nGit usage in workflow:\n'
rg -n '\bgit\b' .github/workflows/ci.ymlRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 1392
Disable persisted credentials on checkout.
actions/checkout writes the token into git config by default. This job only builds and tests, so persist-credentials: false removes an unnecessary auth path.
🔒 Proposed fix
- 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: Setup Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version-file: '.nvmrc' | |
| cache: 'npm' | |
| - name: Checkout code | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false | |
| - name: Setup Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version-file: '.nvmrc' | |
| cache: 'npm' |
🧰 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 - 22, Disable persisted credentials
in the Checkout code step by updating the actions/checkout@v4 configuration to
set persist-credentials to false. This change belongs in the same workflow job
that also uses Setup Node.js, and the key symbol to locate is the Checkout code
step under actions/checkout@v4.
Source: Linters/SAST tools
| try { | ||
| setIsLoading(true); | ||
| const tokenResponse = await authService.login(email, password); | ||
| setAccessToken(tokenResponse.access_token); | ||
| const userProfile = await authService.getMe(); | ||
| setUser(userProfile); | ||
| onLoginSuccess(); | ||
| } catch (err: any) { | ||
| console.error('Login failed', err); | ||
| if (err.response?.status === 401 || err.response?.status === 400) { | ||
| setErrorMsg('Invalid email or password.'); | ||
| } else { | ||
| setErrorMsg('An unexpected error occurred. Please try again.'); | ||
| } | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files | rg '^(src/components/Login\.tsx|src/store/userStore\.ts|src/.+AuthGuard.+|src/.+guard.+|src/.+auth.+)'Repository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 355
🏁 Script executed:
sed -n '1,220p' src/components/Login.tsx && printf '\n---\n' && sed -n '1,260p' src/store/userStore.ts && printf '\n---\n' && rg -n "AuthGuard|isAuthenticated|accessToken|setAccessToken|getMe\\(" srcRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 7067
🏁 Script executed:
#!/bin/sh
set -eu
for f in src/components/Login.tsx src/store/userStore.ts; do
echo "===== $f ====="
nl -ba "$f" | sed -n '1,240p'
echo
done
echo "===== matches ====="
rg -n "AuthGuard|isAuthenticated|accessToken|setAccessToken|getMe\\(" srcRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 2594
🏁 Script executed:
sed -n '1,220p' src/stores/userStore.ts && printf '\n---\n' && sed -n '1,220p' src/features/auth/AuthGuard.tsx && printf '\n---\n' && sed -n '1,220p' src/lib/api.client.ts && printf '\n---\n' && sed -n '1,220p' src/features/auth/auth.service.ts && printf '\n---\n' && sed -n '1,180p' src/App.tsxRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 12256
🏁 Script executed:
cat -n src/stores/userStore.ts && printf '\n---\n' && cat -n src/features/auth/AuthGuard.tsx && printf '\n---\n' && cat -n src/lib/api.client.ts && printf '\n---\n' && cat -n src/features/auth/auth.service.tsRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 10953
🏁 Script executed:
cat -n src/App.tsx | sed -n '1,180p'Repository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 4201
Reset auth state if getMe() fails
setAccessToken() marks the store authenticated before the profile load finishes. If getMe() throws, the login form stays on screen but the store still has accessToken/isAuthenticated = true, and AuthGuard will treat that session as valid. Clear the token/auth flag in the catch path instead of leaving a partial session behind.
🤖 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 37 - 54, The login flow in Login’s
submit handler leaves a partial authenticated session if authService.getMe()
fails after setAccessToken() succeeds. Update the catch path in the same handler
to clear the auth state (reset the access token and authenticated flag, and
avoid leaving user state set) before showing the error, so AuthGuard does not
treat the failed login as valid.
| // ── 4. Scroll-to-rotate ─────────────────────────────────────────────── | ||
| let scrollAngle = 0; | ||
| const handleWheel = (e: WheelEvent) => { | ||
| scrollAngle += e.deltaY * GLOBE_CONFIG.SCROLL_SENSITIVITY; | ||
| world.pointOfView({ lat: 0, lng: scrollAngle, altitude: 2 }); | ||
| }; | ||
| window.addEventListener('wheel', handleWheel, { passive: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== useGlobe file outline ==\n'
ast-grep outline src/features/globe/useGlobe.ts --view expanded || true
printf '\n== relevant occurrences ==\n'
rg -n "pointOfView|flyTo|controls\\(|SCROLL_SENSITIVITY|handleWheel|wheel" src/features/globe/useGlobe.ts src -g '!**/node_modules/**' || trueRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 3439
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== useGlobe relevant section ==\n'
sed -n '1,220p' src/features/globe/useGlobe.ts | cat -n
printf '\n== globe types ==\n'
sed -n '1,220p' src/types/globe.types.ts | cat -nRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 12172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== globe package references ==\n'
rg -n "3d-globe|globe\\.gl? |pointOfView\\(|controls\\(" package.json pnpm-lock.yaml package-lock.json yarn.lock src -g '!**/node_modules/**' || trueRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 1929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package dependencies mentioning globe ==\n'
sed -n '1,220p' package.json | cat -nRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 1955
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/features/globe/useGlobe.ts | cat -nRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 8801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package manifests and lockfile refs for globe ==\n'
rg -n '"globe|three-globe|worldwind|pointOfView|controls"' package.json pnpm-lock.yaml package-lock.json yarn.lock src/types/globe.types.ts -g '!**/node_modules/**' || trueRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== package.json ==\n'
sed -n '1,220p' package.json | cat -n
printf '\n== globe-related lockfile matches ==\n'
for f in pnpm-lock.yaml package-lock.json yarn.lock; do
[ -f "$f" ] && { echo "--- $f ---"; rg -n "globe|three-globe|controls|pointOfView" "$f" || true; }
doneRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 2519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== useGlobe.ts with line numbers ==\n'
sed -n '1,220p' src/features/globe/useGlobe.ts | cat -n
printf '\n== globe.types.ts with line numbers ==\n'
sed -n '1,220p' src/types/globe.types.ts | cat -nRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 12197
🌐 Web query:
globe.gl pointOfView getter current camera position api
💡 Result:
In globe.gl, the pointOfView method serves as both a getter and a setter for the camera's position [1][2]. To retrieve the current camera position, call the method without any arguments [3][4]: const currentPov = globe.pointOfView; // Returns { lat: number, lng: number, altitude: number } To set a new camera position, pass an object containing the target coordinates and an optional transition duration (in milliseconds) [1][2]: globe.pointOfView({ lat, lng, altitude }, [ms]); Additionally, you can use the onZoom callback to listen for changes to the camera position triggered by user interaction [2][5]: globe.onZoom(pov => { console.log('New camera position:', pov); });
Citations:
- 1: https://github.com/vasturiano/globe.gl
- 2: https://alxprd.github.io/globe.gl/
- 3: How to capture the coordinates of the center place on earth based on camera view? vasturiano/react-globe.gl#73
- 4: https://github.com/vasturiano/globe.gl/blob/master/src/index.d.ts
- 5: https://npmx.dev/package/globe.gl
Preserve the current camera state in the wheel handler
world.pointOfView({ lat: 0, lng: scrollAngle, altitude: 2 }) resets the focused view on every wheel tick, so flyTo() loses its lat/altitude immediately and the wheel zoom gets overwritten. Read the current POV and update only lng, or skip this handler while a country is active.
🤖 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 122 - 128, The wheel handler in
useGlobe should preserve the current camera state instead of resetting it on
every scroll tick. Update handleWheel so it reads the existing world point of
view and only adjusts lng, or guard the handler when a country is active, so
flyTo and the current lat/altitude are not immediately overwritten.
| // ── Cleanup ─────────────────────────────────────────────────────────── | ||
| return () => { | ||
| isMounted = false; | ||
| cancelAnimationFrame(animationFrameId); | ||
| window.removeEventListener('resize', handleResize); | ||
| window.removeEventListener('wheel', handleWheel); | ||
| container.innerHTML = ''; | ||
| globeInstanceRef.current = null; | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and locate the cleanup/instance creation code.
ast-grep outline src/features/globe/useGlobe.ts --view expanded || true
printf '\n--- useGlobe.ts (relevant lines) ---\n'
sed -n '1,260p' src/features/globe/useGlobe.ts
printf '\n--- search for _destructor / resetProps / globe instance usage ---\n'
rg -n "_destructor|resetProps|new Globe|globeInstanceRef|container\.innerHTML" src || true
printf '\n--- search for globe.gl typings in repo ---\n'
rg -n "interface GlobeGenericInstance|_destructor\(\): void|GlobeGenericInstance" . || trueRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 8568
🌐 Web query:
globe.gl _destructor GlobeGenericInstance dispose renderer scene controls documentation
💡 Result:
The globe.gl library provides an internal _destructor method, which is defined in the GlobeGenericInstance interface [1][2]. This method is designed to perform cleanup of the component's internal state [3][4]. When you need to clean up a globe.gl instance, particularly in environments where components are frequently created and destroyed (like React), you should invoke this method: globeInstance._destructor; In addition to calling _destructor, manual DOM cleanup is often recommended to ensure the browser garbage collector (GC) can fully release the memory associated with the Three.js renderer and scene [5]. A common approach is to clear the container element's innerHTML after calling the destructor: // Example cleanup flow globeInstance._destructor; containerElement.innerHTML = ''; // Removes the canvas and associated DOM elements Regarding the specific components mentioned: - Renderer, Scene, and Controls: globe.gl provides accessors to these Three.js objects (e.g.,.renderer,.scene,.controls) [6], but their lifecycle is primarily managed by the library. Calling _destructor signals the internal processes to stop animation loops (requestAnimationFrame) and clean up event listeners [3][4]. - Memory Leaks: While the library has built-in mechanisms to dispose of resources [7][8], some users have historically encountered issues with incremental data updates (such as textures or geometries) not being disposed of properly [9]. If you experience memory growth during runtime, ensure your data is memoized (especially in React) [7] or consider verifying the state of renderer.info.memory [9]. For complex applications, particularly those using react-globe.gl, be aware that specialized methods like globeTileEngineClearCache may also be available depending on your version if you are dealing with persistent tile caching issues [10].
Citations:
- 1: https://app.unpkg.com/globe.gl@2.45.0/files/dist/globe.gl.d.ts
- 2: https://cdn.jsdelivr.net/npm/globe.gl@2.26.1/dist/globe.gl.d.ts
- 3: Bug: component destructor leaves animationFrame running vasturiano/three-globe#72
- 4: Bug: destructor cleanup vasturiano/three-globe#75
- 5: d3mocide/Sovereign_Watch@060cc71
- 6: https://github.com/vasturiano/globe.gl/blob/master/src/index.d.ts
- 7: Memory leak with dynamic arcs and rings vasturiano/react-globe.gl#102
- 8: Memory leak when updating Globe.tilesData() vasturiano/three-globe#49
- 9: Textures and geometries leak on globeImageUrl / pointsData updates vasturiano/three-globe#116
- 10: vasturiano/react-globe.gl@583e9a5
Dispose the globe.gl instance in cleanup
container.innerHTML = '' only removes the canvas; call world._destructor() first so the renderer, scene, controls, and WebGL resources are released before unmount/remount cycles.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 179-179: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
🤖 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 174 - 182, Cleanup in useGlobe
currently clears the DOM but does not fully release the globe.gl resources.
Update the unmount cleanup in useGlobe to dispose the globe instance first by
calling world._destructor() before removing event listeners and clearing the
container, then null out globeInstanceRef.current so renderer, scene, controls,
and WebGL resources are released properly.
| /* Reset margins and set core black backgrounds */ | ||
| body { | ||
| margin: 0; | ||
| padding: 0; | ||
| background-color: #000; | ||
| font-family: 'Segoe UI', Tahoma, Geneva, Verdana, sans-serif; | ||
| overflow: hidden; | ||
| color: #fff; | ||
| } | ||
|
|
||
| #root { | ||
| width: 100vw; | ||
| height: 100vh; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Files of interest ==\n'
git ls-files 'src/*' 'src/**/*' | sed -n '1,200p'
printf '\n== main entry and routing references ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' "index\.css|BrowserRouter|Routes|Route|createBrowserRouter|RouterProvider|react-router" src
printf '\n== index.css and related layout styles ==\n'
for f in src/index.css src/main.tsx; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
fi
done
printf '\n== Search for body/overflow/page root classes ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' "overflow\s*:\s*hidden|body\s*\\{|globe-page-root|scroll|overflow" srcRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 10611
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== App routes and page shells ==\n'
cat -n src/App.tsx
printf '\n== Reset and token globals ==\n'
cat -n src/styles/reset.css
printf '\n--- tokens.css ---\n'
cat -n src/styles/tokens.css
printf '\n== Auth and dashboard layout styles/components ==\n'
for f in \
src/components/Login.tsx \
src/components/Register.tsx \
src/components/ForgotPassword.tsx \
src/components/ResetPassword.tsx \
src/features/dashboard/DashboardPage.tsx \
src/components/ui/AuthLayout/AuthLayout.tsx \
src/styles/components/auth-layout.css \
src/styles/components/dashboard.css; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f"
echo
fi
done
printf '\n== Body/root/full-height usage ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' "height:\s*100vh|min-height:\s*100vh|overflow:\s*hidden|overflow-y:\s*auto|position:\s*fixed|document\.body|body\s*\\{" srcRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 50412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== auth-layout.css: layout and overflow rules ==\n'
sed -n '1,260p' src/styles/components/auth-layout.css | cat -n
printf '\n== dashboard.css ==\n'
sed -n '1,220p' src/styles/components/dashboard.css | cat -n
printf '\n== GlobeView inline styles that intentionally lock overflow ==\n'
sed -n '1,180p' src/features/globe/GlobeView.tsx | cat -nRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 12375
Move the body scroll lock out of the global stylesheet
src/index.css:2-8 is loaded app-wide from src/main.tsx, so body { overflow: hidden; } affects Login/Register/Forgot Password/Reset Password/Dashboard too. Those routes don’t create their own scroll container, so any overflow on smaller viewports will be clipped with no way to scroll.
🤖 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 1 - 14, The global body styles in src/index.css
are applying a permanent scroll lock via body overflow hidden, which affects all
routes loaded from src/main.tsx. Remove the overflow hidden rule from the global
stylesheet and move the scroll-lock behavior into the specific fullscreen
layout/component that needs it, keeping Login/Register/Forgot Password/Reset
Password/Dashboard able to scroll when their content overflows. Use the body and
`#root` styles in src/index.css as the place to locate and adjust this change.
| // Response Interceptor: Handle 401s and automatic token refresh | ||
| apiClient.interceptors.response.use( | ||
| (response) => response, | ||
| async (error) => { | ||
| const originalRequest = error.config; | ||
|
|
||
| // If it's a 401 and we haven't already retried this request | ||
| if (error.response?.status === 401 && !originalRequest._retry) { | ||
| // Prevent infinite loops on the refresh endpoint itself | ||
| if (originalRequest.url === '/auth/refresh') { | ||
| // Refresh failed (cookie expired/invalid) -> force logout | ||
| useUserStore.getState().logout(); | ||
| return Promise.reject(error); | ||
| } | ||
|
|
||
| originalRequest._retry = true; | ||
|
|
||
| try { | ||
| // Attempt to get a new access token using the HttpOnly cookie | ||
| const { data } = await axios.post(`${API_URL}/auth/refresh`, {}, { withCredentials: true }); | ||
|
|
||
| const newAccessToken = data.access_token; | ||
|
|
||
| // Update the store with the new token | ||
| useUserStore.getState().setAccessToken(newAccessToken); | ||
|
|
||
| // Update the original request's header and retry | ||
| originalRequest.headers.Authorization = `Bearer ${newAccessToken}`; | ||
| return apiClient(originalRequest); | ||
| } catch (refreshError) { | ||
| // Refresh failed (e.g., cookie expired) -> logout | ||
| useUserStore.getState().logout(); | ||
| return Promise.reject(refreshError); | ||
| } | ||
| } | ||
|
|
||
| return Promise.reject(error); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files in scope:\n'
git ls-files src/lib/api.client.ts src | sed -n '1,120p'
printf '\nOutline of src/lib/api.client.ts:\n'
ast-grep outline src/lib/api.client.ts --view expanded || true
printf '\nRelevant auth-related symbols across src:\n'
rg -n --hidden -S "refreshPromise|_retry|interceptors.response.use|auth/refresh|setAccessToken|logout\\(" srcRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 2664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Read src/lib/api.client.ts with line numbers:\n'
cat -n src/lib/api.client.ts | sed -n '1,220p'
printf '\nSearch for any refresh deduplication or queue logic:\n'
rg -n --hidden -S "queue|single-flight|in-flight|refreshPromise|isRefreshing|failedRequests|subscribers" srcRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 2686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant file sections if present.
if [ -f src/lib/api.client.ts ]; then
sed -n '1,220p' src/lib/api.client.ts
fi
# Probe whether the refresh path is already deduplicated elsewhere.
rg -n --hidden -S "refresh.*Promise|Promise.*refresh|isRefreshing|queue.*401|401.*queue|single[- ]flight" src || trueRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 2250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== src/lib/api.client.ts ==="
if [ -f src/lib/api.client.ts ]; then
cat -n src/lib/api.client.ts | sed -n '1,220p'
fi
echo
echo "=== related auth flow search ==="
rg -n --hidden -S "withCredentials: true|/auth/refresh|setAccessToken\\(|logout\\(|_retry|interceptors.response.use" src || trueRepository: TradeDisciplineAI/ai-trading-discipline-copilot-frontned
Length of output: 3790
Serialize the refresh request path src/lib/api.client.ts:27-49 — multiple 401s will each fire their own /auth/refresh call and retry independently. If refresh tokens are rotated server-side, those parallel calls can invalidate one another; the refresh call also has no timeout, so a hung backend leaves retries stuck. Add a single in-flight refresh promise/queue and a timeout on the refresh request.
🤖 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 20 - 58, The response interceptor in
apiClient is allowing multiple 401s to trigger separate /auth/refresh calls,
which can race and break token rotation handling. Update the refresh flow in the
response interceptor to serialize refresh attempts with a single shared
in-flight promise/queue so concurrent requests wait for the same refresh result,
then retry using the new token. Also add a timeout to the axios.post refresh
call so a stalled backend does not leave requests hanging indefinitely.
| 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.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unused catch parameter flagged by lint.
Static analysis flags the unused error binding; at minimum log it for debuggability of silent auth-init failures.
Proposed fix
- } catch (error) {
+ } catch (error) {
+ console.error('initAuth failed', error);
// If refresh fails, clear the state
set({ user: null, isAuthenticated: false, accessToken: null });
}📝 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) { | |
| console.error('initAuth failed', error); | |
| // If refresh fails, clear the state | |
| set({ user: null, isAuthenticated: false, accessToken: null }); | |
| } | |
| }, |
🧰 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, The initAuth catch block in
userStore has an unused error binding; use the caught error instead of ignoring
it. Update the initAuth method to log the failure from authService.getMe before
clearing state, so silent auth-init issues are debuggable, and keep the existing
reset logic for user, isAuthenticated, and accessToken in the catch path.
Source: Linters/SAST tools
| .country-indicator { | ||
| width: 8px; | ||
| height: 8px; | ||
| border-radius: 50%; | ||
| box-shadow: 0 0 6px currentColor; | ||
| flex-shrink: 0; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stylelint: lowercase currentcolor.
🎨 Fix
.country-indicator {
width: 8px;
height: 8px;
border-radius: 50%;
- box-shadow: 0 0 6px currentColor;
+ box-shadow: 0 0 6px currentcolor;
flex-shrink: 0;
}📝 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.
| .country-indicator { | |
| width: 8px; | |
| height: 8px; | |
| border-radius: 50%; | |
| box-shadow: 0 0 6px currentColor; | |
| flex-shrink: 0; | |
| } | |
| .country-indicator { | |
| width: 8px; | |
| height: 8px; | |
| border-radius: 50%; | |
| box-shadow: 0 0 6px currentcolor; | |
| flex-shrink: 0; | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 89-89: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 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/country-list.css` around lines 85 - 91, The
.country-indicator style uses the nonstandard casing currentColor, which
triggers the Stylelint warning. Update the box-shadow declaration in the country
list styles to use the lowercase currentcolor token so the rule passes while
preserving the same visual effect.
Source: Linters/SAST tools
| const renderLogin = (onSuccess = vi.fn(), onBack = vi.fn()) => | ||
| render( | ||
| <MemoryRouter> | ||
| <Login onLoginSuccess={onSuccess} onBackToHome={onBack} /> | ||
| </MemoryRouter>, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
onBackToHome is not a valid Login prop.
Login's props interface only defines onLoginSuccess, onNavigateToSignup, and onNavigateToForgot (see src/components/Login.tsx lines 7-11). This test passes onBackToHome={onBack}, which doesn't exist on LoginProps. TypeScript performs excess-property checking on JSX attributes, so this will fail type-checking / CI (tsc), and the prop is silently dropped at runtime regardless.
🐛 Suggested fix
-const renderLogin = (onSuccess = vi.fn(), onBack = vi.fn()) =>
+const renderLogin = (onSuccess = vi.fn()) =>
render(
<MemoryRouter>
- <Login onLoginSuccess={onSuccess} onBackToHome={onBack} />
+ <Login onLoginSuccess={onSuccess} />
</MemoryRouter>,
);📝 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 renderLogin = (onSuccess = vi.fn(), onBack = vi.fn()) => | |
| render( | |
| <MemoryRouter> | |
| <Login onLoginSuccess={onSuccess} onBackToHome={onBack} /> | |
| </MemoryRouter>, | |
| ); | |
| const renderLogin = (onSuccess = vi.fn()) => | |
| render( | |
| <MemoryRouter> | |
| <Login onLoginSuccess={onSuccess} /> | |
| </MemoryRouter>, | |
| ); |
🤖 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/test/Login.test.tsx` around lines 18 - 23, The test helper in
Login.test.tsx is passing an invalid Login prop: replace onBackToHome with one
of the supported LoginProps callbacks. Update renderLogin to use the actual
Login component API from Login.tsx (for example, pass onNavigateToSignup or
onNavigateToForgot if that is what the test needs, or remove the extra prop
entirely) so the JSX matches the component’s defined props and TypeScript checks
pass.
2 participants
authentication
Summary by CodeRabbit
New Features
Documentation
Chores