Skip to content

Update guard and interceptors to prevent conflicts#1716

Merged
alexandrudanpop merged 2 commits intomainfrom
chore/sync-internal
Dec 3, 2025
Merged

Update guard and interceptors to prevent conflicts#1716
alexandrudanpop merged 2 commits intomainfrom
chore/sync-internal

Conversation

@alexandrudanpop
Copy link
Copy Markdown
Contributor

Fixes OPS-3144

@linear
Copy link
Copy Markdown

linear Bot commented Dec 3, 2025

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Dec 3, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Note

Other AI code review bot(s) detected

CodeRabbit 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.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/sync-internal

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the authentication interceptor setup to prevent conflicts by ensuring interceptors are only initialized once with the correct configuration. The key change is removing the automatic initialization of the request interceptor and instead deferring both request and response interceptor setup until flags are available.

  • Moved request interceptor initialization to be controlled by the guard component
  • Added state management to ensure interceptors are ready before rendering children
  • Updated type signatures to align federated and standard response interceptor interfaces

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
packages/react-ui/src/app/interceptors/response-interceptor.ts Updated federated response interceptor return type to match standard interceptor
packages/react-ui/src/app/interceptors/index.ts Exported setupRequestInterceptor and consolidated type definitions
packages/react-ui/src/app/common/guards/intial-data-guard.tsx Added controlled interceptor initialization with loading state

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +34 to +40
if (!interceptorsReady) {
return (
<div className="bg-background flex h-screen w-screen items-center justify-center ">
<LoadingSpinner size={50}></LoadingSpinner>
</div>
);
}
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interceptorsReady check will block rendering even when flags is undefined. This creates a situation where if flags never load, the application will show a loading spinner indefinitely instead of handling the error state. Consider checking both flags and interceptorsReady, or handling the flags error case differently.

Copilot uses AI. Check for mistakes.
if (!interceptorsReady) {
return (
<div className="bg-background flex h-screen w-screen items-center justify-center ">
<LoadingSpinner size={50}></LoadingSpinner>
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-closing tag should be used for components without children. Replace </LoadingSpinner> with /> to follow React conventions.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1min fix, looks legit.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Dec 3, 2025

Greptile Overview

Greptile Summary

Prevented race condition where API calls could execute before Axios interceptors were configured by moving interceptor setup from module-level initialization to controlled setup within InitialDataGuard.

Key changes:

  • Request interceptor now requires explicit setup with isFederatedAuth flag instead of auto-initializing on module import
  • Added interceptorsReady state to block children rendering until both request and response interceptors are configured
  • Unified type naming from ResponseInterceptorOptions to InterceptorOptions for consistency
  • Corrected TypeScript return type for createFederatedResponseInterceptor to match actual signature

Confidence Score: 4/5

  • This PR is safe to merge with one edge case to monitor
  • The changes effectively solve the race condition by ensuring interceptors are configured before children render. The implementation is sound, but there's an edge case where if flags fail to load (null/undefined), users will see an infinite loading spinner rather than a proper error state.
  • Pay attention to intial-data-guard.tsx - monitor error handling for flags loading failures in production

Important Files Changed

File Analysis

Filename Score Overview
packages/react-ui/src/app/common/guards/intial-data-guard.tsx 4/5 Added state management to ensure interceptors are setup before rendering children, preventing race conditions where API calls occur before interceptors are configured
packages/react-ui/src/app/interceptors/index.ts 5/5 Removed auto-initialization of request interceptor and made it explicitly configurable, unified type names for consistency
packages/react-ui/src/app/interceptors/response-interceptor.ts 5/5 Updated return type signature for federated response interceptor to match actual implementation

Sequence Diagram

sequenceDiagram
    participant App
    participant InitialDataGuard
    participant FlagsAPI
    participant Interceptors
    participant AxiosInstance
    participant Children

    App->>InitialDataGuard: Render with children
    InitialDataGuard->>FlagsAPI: useFlags() (useSuspenseQuery)
    Note over InitialDataGuard,FlagsAPI: Suspends until flags loaded
    FlagsAPI-->>InitialDataGuard: Returns flags data
    
    alt flags is null/undefined
        InitialDataGuard->>InitialDataGuard: console.error + return early
        InitialDataGuard->>InitialDataGuard: Show LoadingSpinner (interceptorsReady=false)
    else flags exists
        InitialDataGuard->>Interceptors: setupRequestInterceptor({isFederatedAuth})
        Interceptors->>AxiosInstance: Register request interceptor
        AxiosInstance-->>Interceptors: interceptorId
        InitialDataGuard->>Interceptors: setupResponseInterceptor({isFederatedAuth})
        Interceptors->>AxiosInstance: Register response interceptor
        AxiosInstance-->>Interceptors: interceptorId
        InitialDataGuard->>InitialDataGuard: setInterceptorsReady(true)
        InitialDataGuard->>Children: Render children (with Suspense)
        Children->>AxiosInstance: Make API calls
        Note over AxiosInstance,Children: Interceptors now active
    end
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment thread packages/react-ui/src/app/common/guards/intial-data-guard.tsx
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Dec 3, 2025

@alexandrudanpop alexandrudanpop merged commit 26434d1 into main Dec 3, 2025
24 checks passed
@alexandrudanpop alexandrudanpop deleted the chore/sync-internal branch December 3, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants