Integrate AppBootstrap and split response interceptors based on flags #1678
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughIntroduces AppBootstrap to perform async bootstrap (fetch flags, initializeInternal) and show a loading spinner; refactors Axios interceptors into a modular setup with request/response factories and idempotent registration; InitialDataGuard now reads flags and calls setupResponseInterceptor; main render tree now wraps App with AppBootstrap and an error boundary. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser
participant Main as main.tsx
participant ErrorBoundary as OpsErrorBoundary
participant AB as AppBootstrap
participant FlagsAPI as flagsApi.getAll
participant Cache as queryClient
participant Init as initializeInternal
participant App as App
Browser->>Main: load bundle
Main->>ErrorBoundary: render
ErrorBoundary->>AB: render <AppBootstrap>
AB->>AB: show LoadingSpinner (mount)
par bootstrap tasks
AB->>FlagsAPI: flagsApi.getAll()
FlagsAPI-->>AB: flags
AB->>Cache: cache flags at [QueryKeys.flags]
AB->>Init: initializeInternal()
Init-->>AB: resolved
end
AB->>AB: set isReady = true
AB->>App: render children
App->>Browser: application UI
sequenceDiagram
autonumber
participant App as Application
participant IDG as InitialDataGuard
participant Hooks as flagsHooks.useFlags
participant Setup as setupResponseInterceptor
participant InterceptorMgr as interceptors/index
participant HTTP as axios request
participant RespInt as response interceptor
App->>IDG: mount
IDG->>Hooks: read flags
Hooks-->>IDG: flags (FEDERATED_LOGIN_ENABLED)
IDG->>IDG: useEffect fires
alt flags present
IDG->>Setup: setupResponseInterceptor({ isFederatedAuth })
Setup->>InterceptorMgr: register response interceptor once
else flags missing
IDG->>IDG: console.error("flags missing")
end
App->>HTTP: make request
HTTP->>RespInt: response arrives
alt 401 Unauthorized
RespInt->>RespInt: check request URL, sign-in exception?
alt requires logout
RespInt->>Auth: authenticationSession.logOut(...)
RespInt->>Browser: location.reload()
end
else other
RespInt-->>App: pass response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR integrates an AppBootstrap component to orchestrate early initialization of the application, and refactors HTTP interceptors to be dynamically configured based on feature flags (specifically federated authentication).
Key changes:
- Introduces
AppBootstrapto pre-fetch flags and initialize internal services before rendering the main app - Splits request and response interceptors into separate modules with factory functions
- Configures response interceptors conditionally based on the
FEDERATED_LOGIN_ENABLEDflag
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-ui/src/main.tsx | Wraps the App component with the new AppBootstrap component |
| packages/react-ui/src/app/lib/initialize-internal.ts | Adds a placeholder initialization function for internal setup |
| packages/react-ui/src/app/interceptors/response-interceptor.ts | Extracts response interceptor logic into factory functions, adds federated auth variant |
| packages/react-ui/src/app/interceptors/request-interceptor.ts | Extracts request interceptor logic into a factory function |
| packages/react-ui/src/app/interceptors/index.ts | Provides setup functions for request and response interceptors with conditional logic |
| packages/react-ui/src/app/interceptors.ts | Removes the old monolithic interceptor file |
| packages/react-ui/src/app/common/guards/intial-data-guard.tsx | Updates to call setupResponseInterceptor with federated auth flag |
| packages/react-ui/src/app/app-bootstrap.tsx | Implements the new bootstrap component that pre-fetches flags and initializes services |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile OverviewGreptile SummaryRefactored app initialization to load feature flags early at startup using a new
Confidence Score: 4/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Main as main.tsx
participant Bootstrap as AppBootstrap
participant FlagsAPI as flagsApi
participant QueryClient as queryClient
participant InitInternal as initializeInternal
participant App as App
participant Guard as InitialDataGuard
participant Interceptors as setupResponseInterceptor
participant Axios as axios
Main->>Bootstrap: Render with children
activate Bootstrap
Bootstrap->>FlagsAPI: getAll()
FlagsAPI-->>Bootstrap: flags (FlagsMap)
Bootstrap->>QueryClient: setQueryData([QueryKeys.flags], flags)
Bootstrap->>InitInternal: await initializeInternal()
InitInternal-->>Bootstrap: (empty for OSS)
Bootstrap->>Bootstrap: setState({ status: 'ready' })
Bootstrap->>App: Render children
deactivate Bootstrap
activate App
App->>Guard: Render with children
activate Guard
Guard->>QueryClient: useFlags() via useSuspenseQuery
QueryClient-->>Guard: flags from cache
Guard->>Interceptors: setupResponseInterceptor({ isFederatedAuth })
activate Interceptors
Interceptors->>Axios: register response interceptor
Note over Interceptors,Axios: Chooses createResponseInterceptor()<br/>or createFederatedResponseInterceptor()<br/>based on FEDERATED_LOGIN_ENABLED flag
deactivate Interceptors
Guard->>App: Render children (routes)
deactivate Guard
deactivate App
Note over Axios: Request Interceptor (setup at module load)
Note over Axios: Adds Authorization header to local API calls
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (!flags) { | ||
| console.error('Missing flags for response interceptor configuration'); | ||
| return; | ||
| } | ||
| setupResponseInterceptor({ | ||
| isFederatedAuth: Boolean(flags?.FEDERATED_LOGIN_ENABLED), | ||
| }); | ||
| }, [flags]); |
There was a problem hiding this comment.
logic: response interceptor setup happens inside a guard that's rendered after the app starts - any API calls made before this guard renders (like during initial routing) won't have 401 handling, which could cause auth state issues
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-ui/src/app/common/guards/intial-data-guard.tsx
Line: 14:22
Comment:
**logic:** response interceptor setup happens inside a guard that's rendered after the app starts - any API calls made before this guard renders (like during initial routing) won't have 401 handling, which could cause auth state issues
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
API requests should start after flags are loded
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/react-ui/src/app/interceptors/response-interceptor.ts (1)
5-42: LGTM! Response interceptor logic is sound.The 401 handling correctly excludes sign-in routes and cloud user info endpoints before logging out. The URL parsing is appropriately defensive with error handling.
As per static analysis hints, consider using
globalThisinstead ofwindowfor better compatibility across different JavaScript environments (lines 21, 36). However, this is purely a code-style improvement and not critical for browser-only code.packages/react-ui/src/app/common/guards/intial-data-guard.tsx (1)
11-22: Verify dependency array completeness.The
useEffectonly depends onflags, butsetupResponseInterceptoris called inside. IfsetupResponseInterceptorcan change between renders (unlikely but possible with module updates), this could cause stale references.Consider adding
setupResponseInterceptorto the dependency array or disabling the ESLint rule with a comment explaining why it's safe to omit if it's guaranteed stable:- }, [flags]); + }, [flags]); // setupResponseInterceptor is stable (module-level function)packages/react-ui/src/app/app-bootstrap.tsx (1)
8-10: Consider marking props as readonly.As per static analysis hints, marking the props interface as readonly improves immutability guarantees.
Apply this diff:
type AppBootstrapProps = { - children: React.ReactNode; + readonly children: React.ReactNode; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/react-ui/src/app/app-bootstrap.tsx(1 hunks)packages/react-ui/src/app/common/guards/intial-data-guard.tsx(1 hunks)packages/react-ui/src/app/interceptors.ts(0 hunks)packages/react-ui/src/app/interceptors/index.ts(1 hunks)packages/react-ui/src/app/interceptors/request-interceptor.ts(1 hunks)packages/react-ui/src/app/interceptors/response-interceptor.ts(1 hunks)packages/react-ui/src/app/lib/initialize-internal.ts(1 hunks)packages/react-ui/src/main.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react-ui/src/app/interceptors.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Indentation: 2 spaces for TypeScript/JavaScript
Braces required for all control blocks, even single-line
One space between keywords and parentheses:if (condition) {
Use camelCase for variables and functions
Use PascalCase for classes and types
Use UPPER_SNAKE_CASE for constants
Use lowercase with hyphens for file names (e.g.,user-profile.ts)
Preferconstoverletorvarin TypeScript/JavaScript
Prefer arrow functions for callbacks and functional components in TypeScript/JavaScript
Files:
packages/react-ui/src/app/app-bootstrap.tsxpackages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/interceptors/response-interceptor.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Use types and interfaces where appropriate in TypeScript
Use explicit return types for exported functions in TypeScript
Files:
packages/react-ui/src/app/app-bootstrap.tsxpackages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/interceptors/response-interceptor.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{tsx,ts}: Frontend tech stack must strictly use: React 18, Zustand, react-query v5, shadcn, and Axios withqspackage for query strings
Follow best practices for React hooks
Prefer small, composable components and extract helper functions where possible
Usecnutility to group tailwind classnames in React components
Files:
packages/react-ui/src/app/app-bootstrap.tsxpackages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/interceptors/response-interceptor.ts
🧠 Learnings (1)
📚 Learning: 2025-11-25T10:22:44.853Z
Learnt from: CR
Repo: openops-cloud/openops PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-25T10:22:44.853Z
Learning: Use the `react-ui` package for main application logic in the frontend
Applied to files:
packages/react-ui/src/app/app-bootstrap.tsxpackages/react-ui/src/main.tsx
🧬 Code graph analysis (5)
packages/react-ui/src/app/app-bootstrap.tsx (4)
packages/react-ui/src/app/lib/flags-api.ts (2)
flagsApi(4-8)FlagsMap(3-3)packages/react-ui/src/app/lib/query-client.ts (1)
queryClient(3-3)packages/react-ui/src/app/constants/query-keys.ts (1)
QueryKeys(1-70)packages/react-ui/src/app/lib/initialize-internal.ts (1)
initializeInternal(1-1)
packages/react-ui/src/app/interceptors/request-interceptor.ts (2)
packages/react-ui/src/app/lib/api.ts (2)
isUrlRelative(12-14)API_URL(10-10)packages/react-ui/src/app/lib/authentication-session.ts (1)
authenticationSession(14-127)
packages/react-ui/src/app/interceptors/index.ts (2)
packages/react-ui/src/app/interceptors/request-interceptor.ts (1)
createRequestInterceptor(24-34)packages/react-ui/src/app/interceptors/response-interceptor.ts (2)
createFederatedResponseInterceptor(44-50)createResponseInterceptor(5-42)
packages/react-ui/src/app/common/guards/intial-data-guard.tsx (2)
packages/react-ui/src/app/common/hooks/flags-hooks.ts (1)
flagsHooks(31-86)packages/react-ui/src/app/interceptors/index.ts (1)
setupResponseInterceptor(25-37)
packages/react-ui/src/main.tsx (2)
packages/react-ui/src/app/app-bootstrap.tsx (1)
AppBootstrap(12-51)packages/react-ui/src/app/app.tsx (1)
App(28-47)
🪛 GitHub Check: SonarCloud Code Analysis
packages/react-ui/src/app/app-bootstrap.tsx
[warning] 12-12: Mark the props of the component as read-only.
packages/react-ui/src/app/interceptors/request-interceptor.ts
[warning] 15-15: Unexpected negated condition.
packages/react-ui/src/app/interceptors/response-interceptor.ts
[warning] 21-21: Prefer globalThis over window.
[warning] 36-36: Prefer globalThis over window.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Engine Image for amd64
- GitHub Check: Build App Image for arm64
- GitHub Check: Build App Image for amd64
- GitHub Check: Build Engine Image for arm64
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/react-ui/src/app/interceptors/response-interceptor.ts (1)
44-50: Clear placeholder for federated authentication.The explicit error message makes it clear this is OSS-only code and federated authentication is not supported.
packages/react-ui/src/main.tsx (1)
8-8: LGTM! Clean integration of AppBootstrap.The wrapper correctly ensures feature flags are loaded before the App component renders.
Also applies to: 15-17
packages/react-ui/src/app/lib/initialize-internal.ts (1)
1-1: LGTM! Clear stub for future internal initialization.The async stub provides a clean extension point for future bootstrap logic.
packages/react-ui/src/app/app-bootstrap.tsx (2)
18-33: Verify fail-open behavior on bootstrap failure.When bootstrap fails (line 28), the component still sets
isReady = true(line 30), which renders the children. This means the app will load even if flag fetching fails.Is this fail-open behavior intentional? If flags are critical for app functionality, consider:
- Showing an error UI instead of rendering children
- Retrying the bootstrap
- Using default/fallback flags
If the behavior is intentional (e.g., the app should work without flags), consider adding a comment explaining this design decision.
15-40: LGTM! Proper cleanup with mounted guard.The mounted guard correctly prevents state updates after unmount, avoiding memory leaks and React warnings.
packages/react-ui/src/app/interceptors/request-interceptor.ts (1)
24-34: LGTM! Token attachment logic is correct.The interceptor properly checks for token existence, URL presence, and auth requirements before attaching the Bearer token.
packages/react-ui/src/app/interceptors/index.ts (2)
12-17: LGTM! Proper guard against duplicate request interceptor installation.The null check ensures the request interceptor is only registered once.
19-19: Module-level side effect is appropriate for request interceptor.Running
setupRequestInterceptor()at module load ensures the request interceptor is installed before any API calls are made.packages/react-ui/src/app/common/guards/intial-data-guard.tsx (1)
14-22: Based on my verification, I now have sufficient information to rewrite the review comment. The investigation confirms the concerns raised:
useSuspenseQuery behavior: In react-query v5,
useSuspenseQueryeither returns data (not null) or throws a promise for suspense. It never returns null data.Architecture issue: The component calls
useSuspenseQuerywithout a parent Suspense boundary. The Suspense boundary is inside the component (only wraps children), not outside. However, there's an Error Boundary (OpsErrorBoundary) which won't catch suspense promises.Null check redundancy: The
if (!flags)check cannot protect against suspense being thrown. If data is unavailable, the component will error before reaching this check. If data is available,flagswill be truthy.
The null check on
flagsis redundant foruseSuspenseQuerybehavior.In react-query v5,
useSuspenseQueryeither suspends (throws a promise) when data is unavailable, or returns the data object (not null) when available. The null check at line 17 cannot execute if data is unavailable (suspense throws first) and won't evaluate true if data is available. Remove the null check, or if kept for defensive programming, add a comment explaining the intent.Note: The component lacks a parent Suspense boundary to properly handle the suspense from
useFlags(), which is an architectural concern separate from this check.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/react-ui/src/app/common/guards/intial-data-guard.tsx (2)
13-15:prefetchFlagscall is redundant withuseFlags+ AppBootstrap
flagsHooks.useFlags()already drives fetching/caching of the same flags, and AppBootstrap now seeds[QueryKeys.flags]into the query cache. The extraflagsHooks.prefetchFlags();here doesn’t add value anymore and just adds another hook invocation.You can safely drop it:
- const { data: flags } = flagsHooks.useFlags(); - flagsHooks.prefetchFlags(); + const { data: flags } = flagsHooks.useFlags();
16-23: Ensure the response interceptor is installed before any calls that rely on 401 handlingBecause
setupResponseInterceptoris invoked inside this guard’suseEffect, any Axios calls that can fire beforeInitialDataGuardmounts (e.g., during early routing or in providers above it) won’t get the centralized 401 handling.If such calls exist, consider moving
setupResponseInterceptorearlier in the lifecycle (e.g., intoAppBootstraponce flags are known) or ensuring this guard wraps all routes that issue authenticated requests.packages/react-ui/src/app/app-bootstrap.tsx (1)
18-33: Bootstrap errors are logged but app still proceeds as “ready”If
flagsApi.getAll()orinitializeInternal()fail, the code logs the error but still flipsisReadytotrue, so the app renders in a potentially inconsistent state (e.g., missing flags).Consider surfacing a dedicated error UI and/or a retry path instead of unconditionally proceeding, especially if downstream flows assume flags are present.
packages/react-ui/src/app/interceptors/request-interceptor.ts (1)
14-21: Fix local-API detection inneedsAuthHeaderto avoid leaking tokensUsing
resolvedUrl.includes(API_URL)means any URL that merely containsAPI_URLas a substring can be treated as “local” and get theAuthorizationheader, which is a security risk. You can tighten this and also remove the negated condition for readability:-const needsAuthHeader = (url: string): boolean => { - const resolvedUrl = !isUrlRelative(url) ? url : `${API_URL}${url}`; - const isLocalUrl = resolvedUrl.includes(API_URL); +const needsAuthHeader = (url: string): boolean => { + const resolvedUrl = isUrlRelative(url) ? `${API_URL}${url}` : url; + const isLocalUrl = resolvedUrl.startsWith(API_URL); const isUnauthenticatedRoute = unauthenticatedRoutes.some((route) => url.startsWith(route), ); return !isUnauthenticatedRoute && isLocalUrl; };This keeps the behavior for relative URLs while ensuring only true API URLs get the auth header.
🧹 Nitpick comments (2)
packages/react-ui/src/app/lib/initialize-internal.ts (1)
1-1: Clarify intent ofinitializeInternalplaceholderThe no-op
initializeInternalis fine as a staging hook, but consider adding a brief comment/TODO about what’s expected to live here long term (e.g., internal SDK wiring, telemetry init) so future contributors don’t assume it’s dead code.packages/react-ui/src/app/interceptors/response-interceptor.ts (1)
18-24: Avoid duplicating sign-in route literals across interceptorsThis interceptor hard-codes
'/api/v1/authentication/sign-in', whilerequest-interceptor.tsuses'/v1/authentication/sign-in'inunauthenticatedRoutes. Even if this currently matches your base-URL setup, having two separate string literals for the same logical route is brittle.Consider introducing a shared constant for the sign-in API path (and, if needed, its
/api-prefixed variant) so both request and response interceptors stay in sync when routes change.Also applies to: 30-37, 44-50
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/react-ui/src/app/app-bootstrap.tsx(1 hunks)packages/react-ui/src/app/common/guards/intial-data-guard.tsx(1 hunks)packages/react-ui/src/app/interceptors.ts(0 hunks)packages/react-ui/src/app/interceptors/index.ts(1 hunks)packages/react-ui/src/app/interceptors/request-interceptor.ts(1 hunks)packages/react-ui/src/app/interceptors/response-interceptor.ts(1 hunks)packages/react-ui/src/app/lib/initialize-internal.ts(1 hunks)packages/react-ui/src/main.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react-ui/src/app/interceptors.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Indentation: 2 spaces for TypeScript/JavaScript
Braces required for all control blocks, even single-line
One space between keywords and parentheses:if (condition) {
Use camelCase for variables and functions
Use PascalCase for classes and types
Use UPPER_SNAKE_CASE for constants
Use lowercase with hyphens for file names (e.g.,user-profile.ts)
Preferconstoverletorvarin TypeScript/JavaScript
Prefer arrow functions for callbacks and functional components in TypeScript/JavaScript
Files:
packages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/interceptors/response-interceptor.tspackages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Use types and interfaces where appropriate in TypeScript
Use explicit return types for exported functions in TypeScript
Files:
packages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/interceptors/response-interceptor.tspackages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{tsx,ts}: Frontend tech stack must strictly use: React 18, Zustand, react-query v5, shadcn, and Axios withqspackage for query strings
Follow best practices for React hooks
Prefer small, composable components and extract helper functions where possible
Usecnutility to group tailwind classnames in React components
Files:
packages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/interceptors/response-interceptor.tspackages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-25T10:22:44.853Z
Learnt from: CR
Repo: openops-cloud/openops PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-25T10:22:44.853Z
Learning: Applies to **/*.{tsx,ts} : Follow best practices for React hooks
Applied to files:
packages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsx
📚 Learning: 2025-11-25T10:22:44.853Z
Learnt from: CR
Repo: openops-cloud/openops PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-25T10:22:44.853Z
Learning: Use the `react-ui` package for main application logic in the frontend
Applied to files:
packages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
🧬 Code graph analysis (2)
packages/react-ui/src/app/interceptors/response-interceptor.ts (2)
packages/react-ui/src/app/constants/cloud.ts (1)
OPENOPS_CLOUD_USER_INFO_API_URL(6-6)packages/react-ui/src/app/lib/authentication-session.ts (1)
authenticationSession(14-127)
packages/react-ui/src/app/common/guards/intial-data-guard.tsx (2)
packages/react-ui/src/app/common/hooks/flags-hooks.ts (1)
flagsHooks(31-86)packages/react-ui/src/app/interceptors/index.ts (1)
setupResponseInterceptor(25-37)
🪛 GitHub Check: SonarCloud Code Analysis
packages/react-ui/src/app/interceptors/response-interceptor.ts
[warning] 21-21: Prefer globalThis over window.
[warning] 36-36: Prefer globalThis over window.
packages/react-ui/src/app/interceptors/request-interceptor.ts
[warning] 15-15: Unexpected negated condition.
🔇 Additional comments (2)
packages/react-ui/src/main.tsx (1)
8-8: AppBootstrap wrapper integration looks goodWrapping
<App />in<AppBootstrap>is a clean way to centralize bootstrap/flags initialization without changing the public entrypoint shape. This composition underStrictModelooks correct.Also applies to: 15-17
packages/react-ui/src/app/interceptors/index.ts (1)
9-17: Idempotent interceptor setup looks solidTracking interceptor IDs and guarding both setup functions with
=== nullgives you clean, one-time registration for request/response interceptors and avoids accidental double registration when modules are re-imported or flags hook re-runs.Also applies to: 21-37
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
alexandrudanpop
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/react-ui/src/app/interceptors/request-interceptor.ts (2)
17-19: Improve route matching to handle trailing slashes and query parameters.The current
url.startsWith(route)check can produce false positives. For example,/v1/flagswould incorrectly match/v1/flags-admin, and/v1/forms/might not match/v1/forms/123consistently.Consider pathname-based matching:
- const isUnauthenticatedRoute = unauthenticatedRoutes.some((route) => - url.startsWith(route), - ); + const pathname = url.startsWith('/') ? url.split('?')[0] : new URL(resolvedUrl).pathname; + const isUnauthenticatedRoute = unauthenticatedRoutes.some((route) => + pathname === route || pathname.startsWith(route + '/'), + );
16-16: Security: UsestartsWith()instead ofincludes()for URL matching.Using
includes()on line 16 creates a security vulnerability. A malicious URL likehttps://evil.com/path?redirect=https://api.openops.comwould incorrectly be classified as a local URL and receive the Authorization header.Apply this diff:
- const isLocalUrl = resolvedUrl.includes(API_URL); + const isLocalUrl = resolvedUrl.startsWith(API_URL);packages/react-ui/src/app/interceptors/index.ts (1)
25-37: Handle the error when federated auth is enabled in OSS.When
isFederatedAuthis true, line 30 callscreateFederatedResponseInterceptor()which throwsError('Not implemented in OSS'). This unhandled error will propagate toInitialDataGuardand crash the app.Add error handling:
export function setupResponseInterceptor({ isFederatedAuth, }: ResponseInterceptorOptions): void { if (responseInterceptorId === null) { + if (isFederatedAuth) { + console.warn('Federated authentication is not supported in this build, using standard auth interceptor'); + } const responseInterceptor = isFederatedAuth - ? createFederatedResponseInterceptor() + ? createResponseInterceptor() : createResponseInterceptor(); responseInterceptorId = axios.interceptors.response.use( responseInterceptor.onFulfilled, responseInterceptor.onRejected, ); } }This ensures the app remains functional even if the federated login flag is incorrectly set in OSS.
🧹 Nitpick comments (1)
packages/react-ui/src/app/lib/initialize-internal.ts (1)
1-1: Add JSDoc to clarify placeholder purpose.The no-op
initializeInternalfunction is a placeholder for future initialization logic. Adding documentation would clarify its intended use and prevent confusion.+/** + * Internal initialization hook called during app bootstrap. + * Currently a no-op placeholder for future initialization logic. + * TODO: Implement internal initialization for federated authentication setup. + */ export const initializeInternal = async () => {};
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/react-ui/src/app/app-bootstrap.tsx(1 hunks)packages/react-ui/src/app/common/guards/intial-data-guard.tsx(1 hunks)packages/react-ui/src/app/common/hooks/flags-hooks.ts(1 hunks)packages/react-ui/src/app/interceptors.ts(0 hunks)packages/react-ui/src/app/interceptors/index.ts(1 hunks)packages/react-ui/src/app/interceptors/request-interceptor.ts(1 hunks)packages/react-ui/src/app/interceptors/response-interceptor.ts(1 hunks)packages/react-ui/src/app/lib/initialize-internal.ts(1 hunks)packages/react-ui/src/main.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react-ui/src/app/interceptors.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx,js,jsx}: Indentation: 2 spaces for TypeScript/JavaScript
Braces required for all control blocks, even single-line
One space between keywords and parentheses:if (condition) {
Use camelCase for variables and functions
Use PascalCase for classes and types
Use UPPER_SNAKE_CASE for constants
Use lowercase with hyphens for file names (e.g.,user-profile.ts)
Preferconstoverletorvarin TypeScript/JavaScript
Prefer arrow functions for callbacks and functional components in TypeScript/JavaScript
Files:
packages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/common/hooks/flags-hooks.tspackages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/interceptors/response-interceptor.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{ts,tsx}: Use types and interfaces where appropriate in TypeScript
Use explicit return types for exported functions in TypeScript
Files:
packages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/common/hooks/flags-hooks.tspackages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/interceptors/response-interceptor.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (.cursor/rules/coding-guidelines.mdc)
**/*.{tsx,ts}: Frontend tech stack must strictly use: React 18, Zustand, react-query v5, shadcn, and Axios withqspackage for query strings
Follow best practices for React hooks
Prefer small, composable components and extract helper functions where possible
Usecnutility to group tailwind classnames in React components
Files:
packages/react-ui/src/app/interceptors/request-interceptor.tspackages/react-ui/src/app/common/hooks/flags-hooks.tspackages/react-ui/src/app/lib/initialize-internal.tspackages/react-ui/src/app/interceptors/index.tspackages/react-ui/src/app/interceptors/response-interceptor.tspackages/react-ui/src/app/common/guards/intial-data-guard.tsxpackages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
🧠 Learnings (4)
📚 Learning: 2025-11-25T10:22:44.853Z
Learnt from: CR
Repo: openops-cloud/openops PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-25T10:22:44.853Z
Learning: Applies to **/*.{tsx,ts} : Frontend tech stack must strictly use: React 18, Zustand, react-query v5, shadcn, and Axios with `qs` package for query strings
Applied to files:
packages/react-ui/src/app/common/hooks/flags-hooks.ts
📚 Learning: 2025-11-25T10:22:44.853Z
Learnt from: CR
Repo: openops-cloud/openops PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-25T10:22:44.853Z
Learning: Applies to **/*.{tsx,ts} : Follow best practices for React hooks
Applied to files:
packages/react-ui/src/app/common/hooks/flags-hooks.tspackages/react-ui/src/main.tsx
📚 Learning: 2025-11-25T10:22:44.853Z
Learnt from: CR
Repo: openops-cloud/openops PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-25T10:22:44.853Z
Learning: Use the `react-ui` package for main application logic in the frontend
Applied to files:
packages/react-ui/src/main.tsxpackages/react-ui/src/app/app-bootstrap.tsx
📚 Learning: 2025-11-25T10:22:44.853Z
Learnt from: CR
Repo: openops-cloud/openops PR: 0
File: .cursor/rules/coding-guidelines.mdc:0-0
Timestamp: 2025-11-25T10:22:44.853Z
Learning: Applies to **/*.{tsx,ts} : Prefer small, composable components and extract helper functions where possible
Applied to files:
packages/react-ui/src/main.tsx
🧬 Code graph analysis (5)
packages/react-ui/src/app/interceptors/request-interceptor.ts (2)
packages/react-ui/src/app/lib/api.ts (2)
isUrlRelative(12-14)API_URL(10-10)packages/react-ui/src/app/lib/authentication-session.ts (1)
authenticationSession(14-127)
packages/react-ui/src/app/interceptors/index.ts (2)
packages/react-ui/src/app/interceptors/request-interceptor.ts (1)
createRequestInterceptor(24-34)packages/react-ui/src/app/interceptors/response-interceptor.ts (2)
createFederatedResponseInterceptor(44-50)createResponseInterceptor(5-42)
packages/react-ui/src/app/interceptors/response-interceptor.ts (2)
packages/react-ui/src/app/constants/cloud.ts (1)
OPENOPS_CLOUD_USER_INFO_API_URL(6-6)packages/react-ui/src/app/lib/authentication-session.ts (1)
authenticationSession(14-127)
packages/react-ui/src/app/common/guards/intial-data-guard.tsx (2)
packages/react-ui/src/app/common/hooks/flags-hooks.ts (1)
flagsHooks(31-78)packages/react-ui/src/app/interceptors/index.ts (1)
setupResponseInterceptor(25-37)
packages/react-ui/src/main.tsx (3)
packages/react-ui/src/app/common/error-boundaries/ops-error-boundary.tsx (1)
OpsErrorBoundary(45-57)packages/react-ui/src/app/app-bootstrap.tsx (1)
AppBootstrap(17-66)packages/react-ui/src/app/app.tsx (1)
App(28-47)
🪛 GitHub Check: SonarCloud Code Analysis
packages/react-ui/src/app/interceptors/request-interceptor.ts
[warning] 15-15: Unexpected negated condition.
packages/react-ui/src/app/interceptors/response-interceptor.ts
[warning] 21-21: Prefer globalThis over window.
[warning] 36-36: Prefer globalThis over window.
🔇 Additional comments (5)
packages/react-ui/src/app/common/hooks/flags-hooks.ts (1)
1-78: LGTM! Clean refactor to reactive flag loading.The removal of
prefetchFlagsin favor of the reactiveuseFlagshook aligns well with the new AppBootstrap initialization flow. All flag hooks appropriately useuseSuspenseQuerywithstaleTime: Infinity, which is correct for configuration flags.packages/react-ui/src/app/interceptors/response-interceptor.ts (1)
5-42: LGTM! Robust 401 handling with proper URL parsing.The response interceptor correctly:
- Excludes sign-in routes and cloud user info endpoints from automatic logout
- Includes proper URL parsing with try/catch
- Checks pathname for precise route matching
- Prevents logout on authentication endpoints
Note: The static analysis warnings about using
windowinstead ofglobalThisare false positives—windowis appropriate and idiomatic for browser-specific code.packages/react-ui/src/main.tsx (1)
14-22: LGTM! Proper component composition.The render tree correctly nests components with the error boundary outermost, followed by bootstrap initialization, then the App. This ensures:
- Errors during bootstrap are caught by the error boundary
- App only renders after successful bootstrap
- Proper initialization flow
Based on learnings: Follows React best practices for component composition and error handling.
packages/react-ui/src/app/app-bootstrap.tsx (1)
17-66: LGTM! Well-structured bootstrap with proper error propagation.The AppBootstrap component correctly:
- Uses a state machine for loading/ready/error states
- Prevents state updates after unmount with a cleanup flag
- Pre-populates flags in the query cache for downstream consumers
- Throws errors to the parent
OpsErrorBoundary(in main.tsx) which will handle retry UIThe design properly separates concerns—bootstrap handles initialization and delegates error UI to the error boundary wrapper.
Based on learnings: Follows React best practices for async initialization and error handling.
packages/react-ui/src/app/common/guards/intial-data-guard.tsx (1)
13-22: LGTM! Flags-driven interceptor setup with proper defensive checks.The guard correctly:
- Uses
useFlags()with Suspense to ensure flags are available- Logs a warning if flags are unexpectedly missing (defensive programming)
- Safely accesses
flags?.FEDERATED_LOGIN_ENABLEDwith optional chaining- Relies on idempotent
setupResponseInterceptorto register onceThe initialization sequence ensures flags are loaded in
AppBootstrapbefore this guard renders, so the interceptor is configured with the correct authentication mode before any protected API calls.
rSnapkoOpenOps
left a comment
rSnapkoOpenOps
left a comment
There was a problem hiding this comment.
code looks good, but I think there is a bug
https://www.loom.com/share/8a4383d97e094571ba80760340e59ecb
|
|
||
| setupRequestInterceptor(); | ||
|
|
||
| type ResponseInterceptorOptions = { |
There was a problem hiding this comment.
nitpick: consier moving type declaration to the top
|
|
fixed |
Part of OPS-3125
Additional Notes
Testing Checklist
Check all that apply:
I tested the feature thoroughly, including edge cases
I verified all affected areas still work as expected
Changes are backwards compatible with any existing data, otherwise a migration script is provided
Visual Changes (if applicable)
None
Summary by CodeRabbit
New Features
Bug Fixes / Reliability
Chores
✏️ Tip: You can customize this high-level summary in your review settings.