feat: make Unleash session id visible for backend#1935
feat: make Unleash session id visible for backend#1935stalniy merged 3 commits intoakash-network:mainfrom
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/deploy-web/src/pages/api/proxy/[...path].ts (1)
15-22: Only set Cookie header when present; avoid sending an empty headerIf neither cookie exists, this writes an empty Cookie header. Prefer setting it only when you have values (or delete it). Also consider parsing via the cookie library for robustness.
-const cookiesToForward = [cfClearance, unleashSessionId].filter(Boolean); -req.headers.cookie = cookiesToForward.join("; "); +const cookiesToForward = [cfClearance, unleashSessionId].filter(Boolean) as string[]; +if (cookiesToForward.length > 0) { + req.headers.cookie = cookiesToForward.join("; "); +} else { + delete req.headers.cookie; +}Optional: switch to cookie.parse for correctness with spaces/encoding:
+import { parse as parseCookie } from "cookie"; @@ -const cookies = req.headers.cookie?.split(";").map(c => c.trim()); -const cfClearance = cookies?.find(c => c.startsWith("cf_clearance=")); -const unleashSessionId = cookies?.find(c => c.startsWith("unleash-session-id=")); +const parsed = req.headers.cookie ? parseCookie(req.headers.cookie) : undefined; +const cfClearance = parsed?.cf_clearance ? `cf_clearance=${parsed.cf_clearance}` : undefined; +const unleashSessionId = parsed?.["unleash-session-id"] ? `unleash-session-id=${parsed["unleash-session-id"]}` : undefined;apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
54-64: More robust cookie parsing (avoid substring/startsWith pitfalls)Use a cookie parser and a cookie-name constant (without “=”) to avoid whitespace/order issues and accidental matches.
-import type { AppContext } from "@src/core/types/app-context"; +import type { AppContext } from "@src/core/types/app-context"; +import { parse as parseCookie } from "cookie"; @@ - private readonly UNLEASH_COOKIE_KEY = "unleash-session-id="; + private readonly UNLEASH_COOKIE_NAME = "unleash-session-id"; @@ - private extractSessionId(): string | undefined { - const httpContext = this.executionContext.get("HTTP_CONTEXT") as AppContext | undefined; - if (!httpContext) return undefined; - - const cookieHeader = httpContext.req.header("cookie"); - if (!cookieHeader) return undefined; - - const cookies = cookieHeader.split(";").map(c => c.trim()); - const unleashCookie = cookies.find(c => c.startsWith(this.UNLEASH_COOKIE_KEY)); - return unleashCookie?.replace(this.UNLEASH_COOKIE_KEY, ""); - } + private extractSessionId(): string | undefined { + const httpContext = this.executionContext.get("HTTP_CONTEXT") as AppContext | undefined; + if (!httpContext) return undefined; + const cookieHeader = httpContext.req.header("cookie"); + if (!cookieHeader) return undefined; + const parsed = parseCookie(cookieHeader); + const value = parsed[this.UNLEASH_COOKIE_NAME]; + return typeof value === "string" && value.length > 0 ? value : undefined; + }
18-18: Deduplicate cookie name across appsThis cookie name also appears in the web proxy. Consider exporting a shared constant (e.g., @src/shared/constants/cookies.ts) to prevent drift.
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts (2)
186-193: Place setup() at the very bottom of the root describe blockGuideline requires setup at the bottom. Move createUnleashMockClient above, so setup remains last.
- async function setup(input: { + // (Move createUnleashMockClient above this function) + async function setup(input: { config?: Partial<typeof envConfig>; createClient?: (config: UnleashConfig) => Unleash; currentUser?: { id: string }; httpClientInfo?: ClientInfoContextVariables["clientInfo"]; unleashSessionId?: string; skipInitialization?: boolean; }) { @@ - function createUnleashMockClient(input?: { isEnabledFeatureFlag?: (featureFlag: FeatureFlagValue) => boolean }) { - return mock<Unleash>({ - once(event, callback) { - if (event === "synchronized") { - process.nextTick(callback); - } - return this as Unleash; - }, - isEnabled: input?.isEnabledFeatureFlag ? input.isEnabledFeatureFlag : () => false - }); - } + // place this helper above `setup` so `setup` stays the last item in the describe block
230-231: Avoidanyin testsReplace the
as anycast per TS guideline. A simpleRecord<string, unknown>keeps type safety without resorting toany.- } - }) as any + } + }) as Record<string, unknown>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(3 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(4 hunks)apps/deploy-web/src/pages/api/proxy/[...path].ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/deploy-web/src/pages/api/proxy/[...path].tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/deploy-web/src/pages/api/proxy/[...path].tsapps/api/src/core/services/feature-flags/feature-flags.service.tsapps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
🧬 Code graph analysis (2)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
apps/api/src/auth/services/auth.service.ts (2)
currentUser(12-14)currentUser(16-19)apps/api/src/user/controllers/user/user.controller.ts (1)
httpContext(31-33)
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts (3)
apps/api/src/auth/services/auth.service.ts (2)
currentUser(12-14)currentUser(16-19)apps/api/src/middlewares/clientInfoMiddleware.ts (1)
ClientInfoContextVariables(7-13)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlags(1-5)
⏰ 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). (4)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (2)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
35-44: LGTM: sessionId now included in Unleash contextPassing sessionId into the Unleash context aligns with per-session rollout semantics and keeps backwards compatibility when absent.
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts (1)
115-154: LGTM: covers cookie-based sessionId propagationGood assertion that sessionId from the Unleash cookie reaches the Unleash client context.
apps/api/src/core/services/feature-flags/feature-flags.service.ts
Outdated
Show resolved
Hide resolved
jzsfkzm
force-pushed
the
feature/1867-full-stack-unleash-session
branch
from
bede4a5 to
4c4eebb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
35-45: Session stickiness: OK; confirm FE/BE context parity.Propagating sessionId into Unleash context solves the rollout stickiness objective. Confirm whether we must keep FE and BE contexts identical; if yes, consider dropping remoteAddress/userAgent/fingerprint/nodeEnv to prevent divergence-driven toggling.
🧹 Nitpick comments (2)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
18-18: Don’t bake “=” into the cookie constant; minor parsing hardening.Storing the cookie name without “=” simplifies matching and avoids subtle bugs. Also handle empty values.
- private readonly UNLEASH_COOKIE_KEY = "unleash-session-id="; + private readonly UNLEASH_COOKIE_NAME = "unleash-session-id";- const cookies = cookieHeader.split(";").map(c => c.trim()); - const unleashCookie = cookies.find(c => c.startsWith(this.UNLEASH_COOKIE_KEY)); - return unleashCookie?.replace(this.UNLEASH_COOKIE_KEY, ""); + const pairs = cookieHeader.split(";").map(c => c.trim()); + const match = pairs.find(c => c.startsWith(`${this.UNLEASH_COOKIE_NAME}=`)); + if (!match) return undefined; + const value = match.slice(this.UNLEASH_COOKIE_NAME.length + 1); + try { + return decodeURIComponent(value) || undefined; + } catch { + return value || undefined; + }
54-65: Extractor is simple and safe; add tiny edge-case guard.Current code can return an empty string when cookie is present without a value. Treat empty as undefined (covered in the diff above). If you prefer maximum robustness, consider using the tiny “cookie” parser library already common in Node stacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(3 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(4 hunks)apps/deploy-web/src/pages/api/proxy/[...path].ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- apps/deploy-web/src/pages/api/proxy/[...path].ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/core/services/feature-flags/feature-flags.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/core/services/feature-flags/feature-flags.service.ts
🧬 Code graph analysis (1)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
apps/api/src/auth/services/auth.service.ts (2)
currentUser(12-14)currentUser(16-19)apps/api/src/user/controllers/user/user.controller.ts (1)
httpContext(31-33)
⏰ 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). (4)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (1)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
6-6: Good: type-only import.Using a type-only import for AppContext avoids runtime bloat and keeps types explicit.
jzsfkzm
force-pushed
the
feature/1867-full-stack-unleash-session
branch
from
4c4eebb to
1957b8f
Compare
2 participants
closes #1867
Summary by CodeRabbit
New Features
Tests