feat(auth): uses user aware feature flagging in console web#1494
feat(auth): uses user aware feature flagging in console web#1494ygrishajev merged 1 commit intomainfrom
Conversation
WalkthroughThe changes refactor feature flag handling to support richer user context, especially user ID, when evaluating flags. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant UserAwareFlagProvider
participant useUser
participant FlagProvider
User->>App: Visit page
App->>UserAwareFlagProvider: Render
UserAwareFlagProvider->>useUser: Get current user
useUser-->>UserAwareFlagProvider: Return user (with id)
UserAwareFlagProvider->>FlagProvider: Provide config.context.userId = user.id
FlagProvider-->>App: Render children with user-aware flags
sequenceDiagram
participant SSR
participant SessionService
participant FeatureFlagService
participant Unleash
SSR->>SessionService: Get user session
SessionService-->>SSR: Return session (with user.id)
SSR->>FeatureFlagService: isEnabledForCtx(flag, ctx, { userId })
FeatureFlagService->>Unleash: getFlag(flag, { userId })
Unleash-->>FeatureFlagService: Return flag status
FeatureFlagService-->>SSR: Return enabled/disabled
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1494 +/- ##
==========================================
+ Coverage 39.65% 39.69% +0.03%
==========================================
Files 869 869
Lines 20961 20968 +7
Branches 3838 3841 +3
==========================================
+ Hits 8312 8323 +11
+ Misses 12049 12045 -4
Partials 600 600
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/deploy-web/src/pages/alerts/notification-channels/new.tsx (1)
3-8: SameregisteredOnly/ user-id concerns apply herePass
{ registeredOnly: true }explicitly and ensureFeatureFlagServiceenforces it (see earlier comments).apps/deploy-web/src/pages/alerts/index.tsx (1)
3-8: Replicate the fix from the[id]pageKeep the call signature consistent with:
export const getServerSideProps = featureFlagService.showIfEnabled("alerts", { registeredOnly: true });but make sure the service actually checks the requirement.
🧹 Nitpick comments (2)
apps/deploy-web/src/pages/_app.tsx (1)
96-128: Nit: gigantic JSX chain hurts readabilityConsider extracting the deep provider stack inside
AppRootto a<RootProviders>component to keep_app.tsxleaner.apps/deploy-web/src/context/FlagProvider/FlagProvider.tsx (1)
9-14: Potentially overriding essential Unleash config
UserAwareFlagProviderpasses a newconfigobject with onlycontext. If the original provider relied on other keys (url,clientKey,appName, etc.) coming from environment variables via props spread in the old implementation, those values are now lost.-return <FlagProviderOriginal config={{ context: { userId: user?.userId } }}>{children}</FlagProviderOriginal>; +return ( + <FlagProviderOriginal + config={{ + ...browserEnvConfig.UNLEASH_PROVIDER_CONFIG, // ensure existing config stays intact + context: { userId: user?.userId } + }} + > + {children} + </FlagProviderOriginal> +);Double-check that the provider still receives the full configuration at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/deploy-web/src/context/FlagProvider/FlagProvider.tsx(1 hunks)apps/deploy-web/src/pages/_app.tsx(2 hunks)apps/deploy-web/src/pages/alerts/index.tsx(1 hunks)apps/deploy-web/src/pages/alerts/notification-channels/[id]/index.tsx(1 hunks)apps/deploy-web/src/pages/alerts/notification-channels/index.tsx(1 hunks)apps/deploy-web/src/pages/alerts/notification-channels/new.tsx(1 hunks)apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts(3 hunks)apps/deploy-web/src/services/feature-flag/feature-flag.service.ts(3 hunks)apps/deploy-web/src/services/feature-flag/index.ts(1 hunks)apps/deploy-web/src/services/route-protector/index.ts(0 hunks)apps/deploy-web/src/services/route-protector/route-protector.service.spec.ts(0 hunks)apps/deploy-web/src/services/route-protector/route-protector.service.ts(0 hunks)
💤 Files with no reviewable changes (3)
- apps/deploy-web/src/services/route-protector/route-protector.service.spec.ts
- apps/deploy-web/src/services/route-protector/index.ts
- apps/deploy-web/src/services/route-protector/route-protector.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/deploy-web/src/pages/alerts/notification-channels/[id]/index.tsx (2)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
isEnabled(26-46)apps/deploy-web/src/services/feature-flag/index.ts (1)
featureFlagService(7-7)
apps/deploy-web/src/services/feature-flag/index.ts (1)
apps/deploy-web/src/services/feature-flag/feature-flag.service.ts (1)
FeatureFlagService(8-56)
apps/deploy-web/src/pages/alerts/index.tsx (3)
apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.tsx (1)
RegisteredUsersOnly(6-22)apps/deploy-web/src/pages/alerts/notification-channels/[id]/index.tsx (1)
getServerSideProps(29-65)apps/deploy-web/src/services/feature-flag/index.ts (1)
featureFlagService(7-7)
apps/deploy-web/src/pages/alerts/notification-channels/index.tsx (4)
apps/deploy-web/src/hoc/registered-users-only/registered-users-only.hoc.tsx (1)
RegisteredUsersOnly(6-22)apps/deploy-web/src/components/alerts/NotificationChannelsPage.tsx (1)
NotificationChannelsPage(13-33)apps/deploy-web/src/pages/alerts/notification-channels/[id]/index.tsx (1)
getServerSideProps(29-65)apps/deploy-web/src/services/feature-flag/index.ts (1)
featureFlagService(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (4)
apps/deploy-web/src/services/feature-flag/index.ts (1)
1-8: Instantiation looks correct – nothing to flag.
getSessionis injected in the right argument position and the import side-effect is negligible on the server bundle.apps/deploy-web/src/pages/_app.tsx (1)
76-86: Provider order may starve upstream hooks of feature-flag context
FlagProvidernow lives insideUserProviders, but providers such asServicesProvider,SettingsProvider, etc. (declared inAppRoot) sit outside ofUserProviders.
If any of those providers calluseFlagthey will crash (useFlag must be used within FlagProvider).Two options:
- <UserProviders> - <FlagProvider> + <FlagProvider> + <UserProviders> ... - </FlagProvider> - </UserProviders> + </UserProviders> + </FlagProvider>or expose a light wrapper that forwards the userId down so
FlagProvidercan stay at the top of the tree.Please audit the upstream providers before merging.
apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts (1)
100-110: Tests now encode incorrect business semanticsThe case “returns true when registeredOnly is true and user is NOT logged in” should in fact expect
falseonce the server-side guard is properly enforced (see service comment). Otherwise unit tests will mask the regression instead of catching it.apps/deploy-web/src/pages/alerts/notification-channels/index.tsx (1)
7-7: Server-side guard currently ineffective for anonymous usersBecause
registeredOnlyis not honoured insideFeatureFlagService, the SSR entry point will happily serve the page (and potentially make downstream API calls) for visitors without a session when the “alerts” flag is enabled.
Until the service is fixed, theRegisteredUsersOnlyHOC hides the page after the markup is sent, but the data layer remains exposed.
Re-evaluate once the service logic is corrected.
apps/deploy-web/src/pages/alerts/notification-channels/[id]/index.tsx
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/services/feature-flag/feature-flag.service.ts
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/services/feature-flag/feature-flag.service.ts
Outdated
Show resolved
Hide resolved
ygrishajev
force-pushed
the
feature/ff
branch
3 times, most recently
from
c8ef1ce to
b3a310d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/deploy-web/src/context/FlagProvider/FlagProvider.spec.tsx (2)
10-16: Prefer explicit typings overanyfor stronger type-safetyUsing
anyin test helpers bypasses TypeScript’s guarantees and can silently mask refactor breakage.
A minimal explicit interface keeps the test self-documenting while remaining lightweight.-const customFlagProvider = ({ config, children }: any) => ( +interface TestFlagProviderProps { + config: { context: { userId: string } }; + children: React.ReactNode; +} + +const customFlagProvider: React.FC<TestFlagProviderProps> = ({ config, children }) => ( <div data-testid="flag-provider"> {config.context.userId} {children} </div> ); -const customUseUser = () => testUser; +const customUseUser = (): typeof testUser => testUser;
24-25: Assert exact content to avoid false positives
toContain("my-user-id")passes even if additional unexpected characters are present.
Checking for an exact match provides a tighter guarantee.-expect(getByTestId("flag-provider").textContent).toContain("my-user-id"); +expect(getByTestId("flag-provider").textContent).toBe("my-user-id");apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts (3)
9-9: Prefer the dynamic class name over a literal stringUsing the literal
"FeatureFlagService.name"defeats the purpose of keeping the suite name in sync with future refactors (e.g. if the class is renamed).-describe("FeatureFlagService.name", () => { +describe(FeatureFlagService.name, () => {
100-110: Test description contradicts asserted behaviourThe test name claims it “returns true when registeredOnly is true and user is NOT logged in”, yet the expectation is
false. Consider renaming for clarity.-it("returns true when registeredOnly is true and user is NOT logged in", async () => { +it("returns false when registeredOnly is true and user is NOT logged in", async () => {
147-154:isEnabledoption should be typed as boolean
isEnabledis treated as a boolean but typed asjest.Mock, which is misleading.-function setup(options?: { enableAll?: boolean; isEnabled?: jest.Mock; hasUserSession?: boolean }) { +function setup(options?: { enableAll?: boolean; isEnabled?: boolean; hasUserSession?: boolean }) { ... - flagsClient.isEnabled.mockReturnValue(!!options?.isEnabled); + flagsClient.isEnabled.mockReturnValue(options?.isEnabled ?? false);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/deploy-web/src/context/FlagProvider/FlagProvider.spec.tsx(1 hunks)apps/deploy-web/src/context/FlagProvider/FlagProvider.tsx(1 hunks)apps/deploy-web/src/pages/_app.tsx(2 hunks)apps/deploy-web/src/pages/alerts/index.tsx(1 hunks)apps/deploy-web/src/pages/alerts/notification-channels/[id]/index.tsx(1 hunks)apps/deploy-web/src/pages/alerts/notification-channels/index.tsx(1 hunks)apps/deploy-web/src/pages/alerts/notification-channels/new.tsx(1 hunks)apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts(3 hunks)apps/deploy-web/src/services/feature-flag/feature-flag.service.ts(3 hunks)apps/deploy-web/src/services/feature-flag/index.ts(1 hunks)apps/deploy-web/src/services/route-protector/index.ts(0 hunks)apps/deploy-web/src/services/route-protector/route-protector.service.spec.ts(0 hunks)apps/deploy-web/src/services/route-protector/route-protector.service.ts(0 hunks)
💤 Files with no reviewable changes (3)
- apps/deploy-web/src/services/route-protector/index.ts
- apps/deploy-web/src/services/route-protector/route-protector.service.spec.ts
- apps/deploy-web/src/services/route-protector/route-protector.service.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/deploy-web/src/pages/alerts/index.tsx
- apps/deploy-web/src/pages/_app.tsx
- apps/deploy-web/src/pages/alerts/notification-channels/new.tsx
- apps/deploy-web/src/services/feature-flag/index.ts
- apps/deploy-web/src/pages/alerts/notification-channels/index.tsx
- apps/deploy-web/src/pages/alerts/notification-channels/[id]/index.tsx
- apps/deploy-web/src/context/FlagProvider/FlagProvider.tsx
- apps/deploy-web/src/services/feature-flag/feature-flag.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: validate-deploy-web
- GitHub Check: test-deploy-web-build
🔇 Additional comments (8)
apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts (8)
13-15: Unit-test paths look soundThe happy-path assertions for
getFlagcorrectly verify that the short-circuit and evaluation flows returntrue.Also applies to: 21-28
60-62: Session ID propagation verified correctlyThe expectation on
getFlagguarantees that the session ID extracted from cookies is forwarded, covering an easy-to-miss detail.
64-98: Comprehensive coverage ofregisteredOnlyanduserIdoptionsNice spread of scenarios ensuring the correct precedence between explicit
userId, session-derived IDs, and the early-exit when no user is present.
112-122: Edge-case (no userId) handled wellThe negative path is asserted correctly and guards against accidental access for anonymous users when
registeredOnlyis requested.
128-133:showIfEnabledspy arguments verifiedEnsuring
optionsare forwarded (even whenundefined) protects against silent API drift in the wrapper helper.
138-143: Negative branch ofshowIfEnabledcoveredGood to see both branches exercised; keeps the helper honest.
159-161: MockinggetSessioninline is concise and flexibleThe helper cleanly toggles between “logged-in” and anonymous scenarios without leaking complexity into the individual tests.
164-164: Return object fromsetupexposes all useful test doublesMakes downstream assertions straightforward—good practice.
apps/deploy-web/src/services/feature-flag/feature-flag.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/deploy-web/src/services/feature-flag/feature-flag.service.ts
Outdated
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit