feat: adds feature flags support to console-api's notifications proxy#1472
feat: adds feature flags support to console-api's notifications proxy#1472
Conversation
WalkthroughThis update introduces feature flag support using Unleash, adds environment configuration for feature flags, and implements a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server
participant FeatureFlagsService
participant UnleashClient
Client->>API_Server: Request to /v1/alerts (POST/PATCH)
API_Server->>FeatureFlagsService: isEnabled('NOTIFICATIONS_ALERT_MUTATION')
FeatureFlagsService->>UnleashClient: check feature flag with context
UnleashClient-->>FeatureFlagsService: flag enabled/disabled
FeatureFlagsService-->>API_Server: true/false
alt Feature enabled
API_Server->>Client: Process request (proxy)
else Feature disabled
API_Server->>Client: 405 Method Not Allowed
end
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! ✨ 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 #1472 +/- ##
==========================================
+ Coverage 39.44% 39.62% +0.17%
==========================================
Files 866 869 +3
Lines 20888 20958 +70
Branches 3825 3839 +14
==========================================
+ Hits 8239 8304 +65
- Misses 11915 12470 +555
+ Partials 734 184 -550
🚀 New features to boost your workflow:
|
stalniy
force-pushed
the
feat/feature-flags-notifications
branch
from
390683c to
2a7b09b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
apps/api/src/core/services/execution-context/execution-context.service.ts (1)
36-43: 🛠️ Refactor suggestionSimplify
runWithContextimplementation
AsyncLocalStorage.runalready propagates the promise result; the extranew Promisewrapper and redundantgetStorecall add overhead and hide stack traces.-async runWithContext<R>(cb: (...args: any[]) => Promise<R>): Promise<R> { - return await new Promise((resolve, reject) => { - this.storage.run(new Map(), () => { - this.storage.getStore(); - cb().then(resolve).catch(reject); - }); - }); -} +runWithContext<R>(cb: () => Promise<R>): Promise<R> { + return this.storage.run(new Map(), cb); +}apps/api/src/notifications/routes/proxy/proxy.route.spec.ts (1)
94-105: 🛠️ Refactor suggestionAvoid
unknown as AppContext; provide a typed factory for safer mocksCasting the entire object to
unknown as AppContextdefeats the stronger typings you just introduced and can let real-world regressions slip through tests.
Create a tiny helper that builds a minimal, properly-typedAppContextstub instead of the blanket cast.-const context = { +const context: AppContext = { req: { … }, get: jest.fn().mockReturnValue(undefined) -} as unknown as AppContext; +} as AppContext; // ✅ keeps compile-time checks(If other props are mandatory, stub them explicitly so the compiler tells you when the contract changes.)
🧹 Nitpick comments (4)
apps/api/package.json (1)
102-104: Pinunleash-client& audittsyringebump
•unleash-client@^6.6.0introduces a new major peer-dependency on Node ≥18; make sure your runtime matrix in CI/CD reflects that.
• Movingtsyringeto^4.10.0pulls in ESM‐ready internals that broke some transpile pipelines in <4.9 (see issues #299/#301). Verify bundle builds still tree-shake correctly.If you want deterministic builds (recommended for infra services) pin exact versions and rely on Renovate/Dependabot for upgrades:
- "tsyringe": "^4.10.0", - "unleash-client": "^6.6.0", + "tsyringe": "4.10.0", + "unleash-client": "6.6.0",apps/api/src/core/services/feature-flags/feature-flags.ts (1)
1-5: Expose a “key” union for cleaner type-guarding
Down-stream code often needs"NOTIFICATIONS_ALERT_MUTATION"itself rather than the mapped value. Consider addingexport const FeatureFlags = { NOTIFICATIONS_ALERT_MUTATION: "notifications_general_alerts_create_update" } as const; +export type FeatureFlagKey = keyof typeof FeatureFlags; export type FeatureFlagValue = (typeof FeatureFlags)[keyof typeof FeatureFlags];This lets consumers accept either the key or the value with proper inference and keeps enum-like ergonomics.
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
61-64: Indefinite wait for Unleash synchronization
await new Promise(... once("synchronized") ...)has no timeout. If the Unleash server is unreachable the request thread can hang forever, leading to request pile-ups.Add a reasonable timeout (e.g., 5-10 s) and fail gracefully/log a warning so the application keeps serving requests.
apps/api/src/notifications/routes/proxy/proxy.route.ts (1)
57-64: Resolve the service once and reuse
container.resolve(FeatureFlagsService)is invoked on every request, incurring DI overhead. Consider resolving once outside the handler (or injecting via Hono context middleware) to reuse the singleton instance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
apps/api/package.json(1 hunks)apps/api/src/app.ts(2 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/core/config/env.config.ts(1 hunks)apps/api/src/core/services/core-config/core-config.service.ts(1 hunks)apps/api/src/core/services/execution-context/execution-context.service.ts(2 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.spec.ts(2 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(3 hunks)apps/api/src/user/controllers/user/user.controller.ts(1 hunks)packages/docker/docker-compose.dev.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: validate-notifications
- GitHub Check: validate-api
- GitHub Check: test-api-build
- GitHub Check: test-provider-console-build
- GitHub Check: test-deploy-web-build
- GitHub Check: test-stats-web-build
- GitHub Check: Validate local packages
🔇 Additional comments (3)
apps/api/src/core/services/core-config/core-config.service.ts (1)
3-4: Confirm relative import paths compile for consumers
Switching to relative paths fixes intra-package references but can break TypeScript path-aliasing for external consumers if@src/*is relied upon elsewhere. Runtsc -p tsconfig.strict.jsonand ensure no duplicate modules are emitted.packages/docker/docker-compose.dev.yml (1)
65-104:Details
✅ Verification successful
YAML indentation & host-port clash check
- The
mock-oauth2-serverblock is aligned withprovider-console, not underservices:. Make sure it has the same two-space indent as the other top-level services to avoid a parsing error.- Port
8080is a common choice; verify no other service in this compose file (or on your dev host) already binds it to prevent startup failures.Example fix:
mock-oauth2-server: image: ghcr.io/navikt/mock-oauth2-server:2.1.10 ports: - "8080:8080" …
🏁 Script executed:
#!/bin/bash echo "=== Services sections ===" rg -n "^services:" packages/docker/docker-compose.dev.yml || true echo -e "\n=== Top of file (1-50) ===" sed -n '1,50p' packages/docker/docker-compose.dev.yml || true echo -e "\n=== Lines 60-110 ===" sed -n '60,110p' packages/docker/docker-compose.dev.yml || true echo -e "\n=== Occurrences of port 8080 ===" rg -n "8080" packages/docker/docker-compose.dev.yml || trueLength of output: 3012
No changes required: indentation and port binding verified
Both checks pass as-is:
- The
mock-oauth2-serverservice is already indented two spaces underservices:.- Port 8080 only appears once in this compose file, so there’s no intra-file conflict.
apps/api/src/notifications/routes/proxy/proxy.route.ts (1)
39-41: Nice defensive checkConditionally setting
"x-owner-address"avoids sending empty headers – good catch.
apps/api/src/core/services/execution-context/execution-context.service.ts
Show resolved
Hide resolved
apps/api/src/core/services/execution-context/execution-context.service.ts
Show resolved
Hide resolved
apps/api/src/core/services/feature-flags/feature-flags.service.ts
Outdated
Show resolved
Hide resolved
stalniy
force-pushed
the
feat/feature-flags-notifications
branch
from
2a7b09b to
cd2ffd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
apps/api/src/core/services/config/config.service.ts (2)
3-10: Avoid the banned{}type – tighten the default forCThe Biome linter warning is right: using
{}as a type effectively means “any non-nullish value”, defeating type-safety. A minimal fix is to default to an explicit empty-object shape instead of{}.-export class ConfigService<E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, C extends Record<string, any> = {}> { +export class ConfigService< + E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, + C extends Record<string, unknown> = Record<string, never> +> {Besides satisfying the linter, this keeps external config strongly typed and prevents accidental misuse.
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
12-17:options.configcan beundefined– defensive spread would be saferIf a caller omits
configthe spread...options.configis fine at runtime (it becomesundefined→ ignored) but TypeScript still treatsoptions.configas possiblyundefined.
A tiny improvement:- ...options.config, + ...(options.config ?? {}),This removes the implicit
anyon the spread operand and clarifies intent.apps/api/src/app.ts (1)
178-187: Graceful shutdown handler looks good – one micro-nitNice job wiring
SIGINT/SIGTERMintoserver.close(); this resolves the earlier feedback 👏.
Consider removing theisShuttingDown = falsereset inside the callback. Once the server is closed successfully we should stay in the “shutdown” state; flipping it back tofalsere-opens the door for a second shutdown attempt (harmless but confusing).- } finally { - isShuttingDown = false; - } + }Purely cosmetic – feel free to ignore if you prefer the current semantics.
apps/api/src/core/config/env.config.ts (3)
19-21: ValidateUNLEASH_SERVER_API_URLas a URLRight now the variable is only
z.string(). A malformed value will surface later when the Unleash client attempts to connect. Adding.url()gives immediate feedback at boot-time:- UNLEASH_SERVER_API_URL: z.string().optional(), + UNLEASH_SERVER_API_URL: z.string().url().optional(),Same applies to any other env vars that are expected to be URLs.
22-26: Boolean env var conversion – small readability tweakUsing
.transform(value => value === "true")works, butz.coerce.boolean()makes the intent clearer and accepts"1"/"0"/"true"/"false"out of the box:- FEATURE_FLAGS_ENABLE_ALL: z - .string() - .default("false") - .transform(value => value === "true") + FEATURE_FLAGS_ENABLE_ALL: z + .coerce.boolean() + .default(false)Not critical, just a readability win.
27-34: Super-refine guard is solid – consider including the var names in the error pathCurrently the error message is attached to the root; attaching it to the offending fields improves UX in config dashboards:
- ctx.addIssue({ + ctx.addIssue({ code: z.ZodIssueCode.custom, message: "UNLEASH_SERVER_API_URL and UNLEASH_SERVER_API_TOKEN are required when FEATURE_FLAGS_ENABLE_ALL is false", + path: ["UNLEASH_SERVER_API_URL"] });(duplicate for the token).
Optional, but surfaces nicely in many config UIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
apps/api/package.json(1 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/core/config/env.config.ts(1 hunks)apps/api/src/core/services/config/config.service.ts(1 hunks)apps/api/src/core/services/core-config/core-config.service.ts(1 hunks)apps/api/src/core/services/execution-context/execution-context.service.ts(2 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.spec.ts(2 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(3 hunks)apps/api/src/user/controllers/user/user.controller.ts(1 hunks)packages/docker/docker-compose.dev.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/api/package.json
- apps/api/src/notifications/routes/proxy/proxy.route.spec.ts
- apps/api/src/core/services/core-config/core-config.service.ts
- apps/api/src/auth/services/auth.service.ts
- apps/api/src/core/services/feature-flags/feature-flags.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- apps/api/src/user/controllers/user/user.controller.ts
- packages/docker/docker-compose.dev.yml
- apps/api/src/notifications/routes/proxy/proxy.route.ts
- apps/api/src/core/services/execution-context/execution-context.service.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/api/src/core/config/env.config.ts (1)
apps/api/src/chain/config/env.config.ts (1)
envSchema(3-5)
🪛 Biome (1.9.4)
apps/api/src/core/services/config/config.service.ts
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: validate-api
- GitHub Check: test-api-build
- GitHub Check: test-provider-console-build
- GitHub Check: test-deploy-web-build
- GitHub Check: validate-deploy-web
- GitHub Check: test-provider-proxy-build
- GitHub Check: validate-provider-proxy
- GitHub Check: test-stats-web-build
- GitHub Check: validate-stats-web
- GitHub Check: validate-notifications
- GitHub Check: test-indexer-build
- GitHub Check: Validate local packages
stalniy
force-pushed
the
feat/feature-flags-notifications
branch
2 times, most recently
from
4b6f41b to
8cc2687
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
47-53: Hard assertion will crash the app on mis-configurationThis issue was already identified in a previous review. The
assertstatement will crash the entire application if configuration is missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
apps/api/package.json(1 hunks)apps/api/src/app.ts(2 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/core/config/env.config.ts(1 hunks)apps/api/src/core/services/config/config.service.ts(1 hunks)apps/api/src/core/services/core-config/core-config.service.ts(1 hunks)apps/api/src/core/services/execution-context/execution-context.service.ts(2 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.spec.ts(2 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(3 hunks)apps/api/src/user/controllers/user/user.controller.ts(1 hunks)packages/docker/docker-compose.dev.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/api/package.json
- apps/api/src/core/services/core-config/core-config.service.ts
- apps/api/src/app.ts
- apps/api/src/notifications/routes/proxy/proxy.route.spec.ts
- apps/api/src/auth/services/auth.service.ts
- apps/api/src/user/controllers/user/user.controller.ts
- apps/api/src/core/services/feature-flags/feature-flags.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- apps/api/src/notifications/routes/proxy/proxy.route.ts
- packages/docker/docker-compose.dev.yml
- apps/api/src/core/config/env.config.ts
- apps/api/src/core/services/execution-context/execution-context.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (4)
apps/api/src/core/services/core-config/core-config.service.ts (1)
singleton(6-11)apps/api/src/core/services/execution-context/execution-context.service.ts (1)
singleton(14-44)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlagValue(5-5)apps/api/src/auth/services/auth.service.ts (2)
currentUser(12-14)currentUser(16-19)
🪛 Biome (1.9.4)
apps/api/src/core/services/config/config.service.ts
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: validate-api
- GitHub Check: validate-notifications
- GitHub Check: test-deploy-web-build
🔇 Additional comments (6)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (4)
9-24: Well-structured service with good testability!The class is properly designed as a singleton with dependency injection and implements the Disposable interface for proper cleanup. The optional
createClientparameter injection is a nice touch for testability.
26-45: Comprehensive feature flag evaluation with proper context!The method correctly handles the global override and builds a rich context for feature flag evaluation, including user information, client details, and environment properties.
69-71: Proper cleanup implementation!The dispose method correctly implements the Disposable interface and safely cleans up the Unleash client with
destroyWithFlush.
74-76: Clean factory function for testability!The standalone factory function provides good separation of concerns and makes the service easily testable.
apps/api/src/core/services/config/config.service.ts (2)
1-1: Correctly imported ZodEffects
IncludingZodEffectsin the import is required to support effect-wrapped schemas. This aligns with the new generic constraints.
3-6: Expanded generic constraint is valid
AllowingEto be either a plainZodObjector aZodEffects<ZodObject>correctly accommodates both simple and effect-wrapped schemas. The interface change looks good.
apps/api/src/core/services/feature-flags/feature-flags.service.ts
Outdated
Show resolved
Hide resolved
stalniy
force-pushed
the
feat/feature-flags-notifications
branch
from
8cc2687 to
11eedcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/api/src/core/services/config/config.service.ts (1)
8-10:{}default type is still bannedLint keeps flagging this. Replace the default
{}withRecord<string, unknown>(or remove the default entirely) to satisfy the rule and improve clarity.-export class ConfigService<E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, C extends Record<string, any> = {}> { +export class ConfigService< + E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, + C extends Record<string, unknown> = Record<string, unknown> +> {🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (2)
26-30: Throwing on un-initialized service replicates earlier concern
assert(this.client, ...)will terminate the request path with a 500 ifinitialize()is forgotten. Past feedback proposed a safer fallback (e.g., returnfalseand log). Consider adopting it.
52-63: Hard assertion & connection failure handling unchangedThe strict
assert(url && token, …)and the un-guardedsynchronized/errorpromise still risk crashing the whole process on mis-configuration or network glitches. Previous review supplied a controlled-startup and try/catch pattern—please revisit.
🧹 Nitpick comments (1)
apps/api/src/core/services/config/config.service.ts (1)
3-6: Reduceanyusage in generics
C extends Record<string, any>silently loses type-safety because every property becomesany.
Preferunknownto force callers to narrow or define a proper interface.-interface ConfigServiceOptions<E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, C extends Record<string, any>> { +interface ConfigServiceOptions< + E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, + C extends Record<string, unknown> +> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/api/env/.env.functional.test(1 hunks)apps/api/env/.env.local.sample(1 hunks)apps/api/env/.env.production(1 hunks)apps/api/env/.env.sample(1 hunks)apps/api/env/.env.unit.test(1 hunks)apps/api/package.json(1 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/core/config/env.config.ts(1 hunks)apps/api/src/core/services/config/config.service.ts(1 hunks)apps/api/src/core/services/core-config/core-config.service.ts(1 hunks)apps/api/src/core/services/execution-context/execution-context.service.ts(2 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.spec.ts(2 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(3 hunks)apps/api/src/user/controllers/user/user.controller.ts(1 hunks)packages/docker/docker-compose.dev.yml(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- apps/api/env/.env.unit.test
- apps/api/env/.env.functional.test
- apps/api/env/.env.production
- apps/api/env/.env.local.sample
- apps/api/env/.env.sample
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/api/src/core/services/core-config/core-config.service.ts
- apps/api/src/user/controllers/user/user.controller.ts
- apps/api/package.json
- apps/api/src/notifications/routes/proxy/proxy.route.spec.ts
- apps/api/src/auth/services/auth.service.ts
- apps/api/src/app.ts
- packages/docker/docker-compose.dev.yml
- apps/api/src/core/services/feature-flags/feature-flags.ts
- apps/api/src/core/services/execution-context/execution-context.service.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- apps/api/src/notifications/routes/proxy/proxy.route.ts
- apps/api/src/core/config/env.config.ts
🧰 Additional context used
🧠 Learnings (1)
apps/api/src/core/services/config/config.service.ts (1)
Learnt from: stalniy
PR: akash-network/console#1362
File: apps/api/src/core/services/config/config.service.ts:8-9
Timestamp: 2025-05-24T04:25:48.222Z
Learning: In the ConfigService class in apps/api/src/core/services/config/config.service.ts, the `{}` type is used as the default for generic parameter `C` because it's intersected with `z.infer<E>` to create `C & z.infer<E>`. Using `Record<string, never>` instead would break type checking for the merged types since it represents an object that cannot have any properties.
🪛 Biome (1.9.4)
apps/api/src/core/services/config/config.service.ts
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test-stats-web-build
- GitHub Check: validate-provider-proxy
- GitHub Check: validate-notifications
- GitHub Check: test-provider-console-build
- GitHub Check: test-api-build
- GitHub Check: validate-api
- GitHub Check: test-indexer-build
- GitHub Check: validate-deploy-web
- GitHub Check: test-deploy-web-build
- GitHub Check: Validate local packages
🔇 Additional comments (1)
apps/api/src/core/services/feature-flags/feature-flags.service.ts (1)
77-79:Details
❌ Incorrect review comment
Potential misuse of Unleash client constructor
unleash-client’s public API isinitialize(config), notnew Unleash(config).
Using the non-documented class may break on library updates.-import { Unleash, UnleashConfig } from "unleash-client"; +import { initialize as unleashInitialize, UnleashConfig, Unleash } from "unleash-client"; … -export function createUnleashClient(config: UnleashConfig): Unleash { - return new Unleash(config); +export function createUnleashClient(config: UnleashConfig): Unleash { + return unleashInitialize(config); }
🌐 Web query:
What is the recommended way to create a client instance with the npm package "unleash-client"?💡 Result:
The recommended way to create a client instance with the npm package "unleash-client" involves requiring or importing the package and then instantiating a new client with the desired configuration. Typically, you configure the client with the URL of your Unleash server and an application name.
Here is an example in Node.js:
const { UnleashClient } = require('unleash-client'); const unleash = new UnleashClient({ url: 'https://unleash-server-url/api/', appName: 'my-nodejs-app', instanceId: 'my-unique-instance-id', }); unleash.start();In this example:
urlis the URL to your Unleash server API.appNameis a string used to identify your application.instanceIdis a unique identifier for the client instance (optional but recommended for identifying the client).After creating the client, you start it with
unleash.start().You can then use the client instance methods to check feature toggles, such as:
const isEnabled = unleash.isEnabled('someFeatureToggle');If you need a more specific or updated example, I can retrieve the latest usage details from the official documentation or npm page. Would you like me to do that?
Incorrect recommendation: use
new Unleash(config)instead of a non-existentinitialize()
Theunleash-clientpackage exports a client class (UnleashorUnleashClient) that you instantiate withnew Unleash(config)and then call.start(). It does not export aninitialize()helper. The existing code is already using the correct public API.• File: apps/api/src/core/services/feature-flags/feature-flags.service.ts (lines 77–79):
export function createUnleashClient(config: UnleashConfig): Unleash { return new Unleash(config); }Likely an incorrect or invalid review comment.
stalniy
force-pushed
the
feat/feature-flags-notifications
branch
from
c7d3393 to
1c27ae9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/api/src/core/services/config/config.service.ts (2)
3-6: Avoid repeating the verbose union – extract a helper alias
ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>now appears twice (here and on the class below). Creating a single alias clarifies intent and prevents the two occurrences from silently diverging in the future.-import type { z, ZodEffects, ZodObject, ZodRawShape } from "zod"; +import type { z, ZodEffects, ZodObject, ZodRawShape } from "zod"; +type EnvSchema = + | ZodObject<ZodRawShape> + | ZodEffects<ZodObject<ZodRawShape>>; -interface ConfigServiceOptions<E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, C extends Record<string, any>> { +interface ConfigServiceOptions< + E extends EnvSchema, + C extends Record<string, any> +> {
9-10: Tighten the generic & satisfy the “no banned types” rule
Record<string, any>widens the config shape and defeats type-safety, while the default{}triggers the linter.
Switching toRecord<string, unknown>keeps the flexibility without opting out of type checking, and we can still default to an empty object via the type parameter itself.-export class ConfigService<E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, C extends Record<string, any> = {}> { +export class ConfigService< + E extends EnvSchema, + C extends Record<string, unknown> = Record<string, unknown> +> {🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/api/env/.env.functional.test(1 hunks)apps/api/env/.env.local.sample(1 hunks)apps/api/env/.env.production(1 hunks)apps/api/env/.env.sample(1 hunks)apps/api/env/.env.unit.test(1 hunks)apps/api/package.json(1 hunks)apps/api/src/app.ts(3 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/core/config/env.config.ts(1 hunks)apps/api/src/core/services/config/config.service.ts(1 hunks)apps/api/src/core/services/core-config/core-config.service.ts(1 hunks)apps/api/src/core/services/execution-context/execution-context.service.ts(2 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.spec.ts(2 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(3 hunks)apps/api/src/user/controllers/user/user.controller.ts(1 hunks)packages/docker/docker-compose.dev.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/api/env/.env.production
🚧 Files skipped from review as they are similar to previous changes (17)
- apps/api/env/.env.functional.test
- apps/api/src/core/services/core-config/core-config.service.ts
- apps/api/env/.env.local.sample
- apps/api/package.json
- apps/api/env/.env.unit.test
- apps/api/src/auth/services/auth.service.ts
- apps/api/src/user/controllers/user/user.controller.ts
- apps/api/src/notifications/routes/proxy/proxy.route.spec.ts
- apps/api/env/.env.sample
- apps/api/src/app.ts
- apps/api/src/core/services/feature-flags/feature-flags.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- packages/docker/docker-compose.dev.yml
- apps/api/src/notifications/routes/proxy/proxy.route.ts
- apps/api/src/core/services/execution-context/execution-context.service.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.ts
- apps/api/src/core/config/env.config.ts
🧰 Additional context used
🧠 Learnings (1)
apps/api/src/core/services/config/config.service.ts (1)
Learnt from: stalniy
PR: akash-network/console#1362
File: apps/api/src/core/services/config/config.service.ts:8-9
Timestamp: 2025-05-24T04:25:48.222Z
Learning: In the ConfigService class in apps/api/src/core/services/config/config.service.ts, the `{}` type is used as the default for generic parameter `C` because it's intersected with `z.infer<E>` to create `C & z.infer<E>`. Using `Record<string, never>` instead would break type checking for the merged types since it represents an object that cannot have any properties.
🪛 Biome (1.9.4)
apps/api/src/core/services/config/config.service.ts
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: validate-api
- GitHub Check: test-api-build
- GitHub Check: validate-notifications
- GitHub Check: test-provider-console-build
- GitHub Check: test-deploy-web-build
🔇 Additional comments (1)
apps/api/src/core/services/config/config.service.ts (1)
1-1: Import extension looks goodAdding
ZodEffectsis the right move to support effect-wrapped schemas.
stalniy
force-pushed
the
feat/feature-flags-notifications
branch
from
1c27ae9 to
ef21de4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/api/src/core/services/config/config.service.ts (1)
13-16:⚠️ Potential issueSpreading
undefinedat runtime causes a crashWhen
envSchemais absent,options.envSchema?.parse(process.env)returnsundefined, and
{ ...undefined }throwsTypeError: Cannot convert undefined or null to object.- ...options.config, - ...options.envSchema?.parse(process.env) + ...options.config, + ...(options.envSchema ? options.envSchema.parse(process.env) : {})
♻️ Duplicate comments (1)
apps/api/src/core/services/config/config.service.ts (1)
8-10: 🛠️ Refactor suggestionReplace banned type
{}withRecord<string, unknown>Biome still flags
{}even though ESLint is suppressed.
Switching toRecord<string, unknown>keeps full property flexibility while satisfying lint rules and avoids thenever-property pitfall ofRecord<string, never>mentioned in previous discussions.-export class ConfigService<E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, C extends Record<string, any> = {}> { +export class ConfigService< + E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, + C extends Record<string, unknown> = Record<string, unknown> +> {🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🧹 Nitpick comments (6)
apps/api/env/.env.sample (1)
39-41: Add a dedicated Feature Flags section with inline docsGroup these new vars under a
# Feature Flagsheader and include brief descriptions to improve clarity:@@ NOTIFICATIONS_API_BASE_URL= +# Feature Flags +# Globally enable all flags when `true`; set to `false` to use Unleash evaluations. FEATURE_FLAGS_ENABLE_ALL=true +# Unleash API token (required when FEATURE_FLAGS_ENABLE_ALL=false). UNLEASH_SERVER_API_TOKEN= +# Base URL of the Unleash server API (e.g., https://unleash.example.com/api). UNLEASH_SERVER_API_URL=🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 39-39: [UnorderedKey] The FEATURE_FLAGS_ENABLE_ALL key should go before the FEE_ALLOWANCE_REFILL_AMOUNT key
[warning] 40-40: [UnorderedKey] The UNLEASH_SERVER_API_TOKEN key should go before the WEBSITE_URL key
[warning] 41-41: [UnorderedKey] The UNLEASH_SERVER_API_URL key should go before the WEBSITE_URL key
apps/api/env/.env.unit.test (1)
28-28: Validate newNOTIFICATIONS_API_BASE_URLintegration and address key ordering warning.
EnsureNOTIFICATIONS_API_BASE_URLis registered inenv.config.tsand consumed by the notifications proxy logic. Also reorder this key to satisfy the dotenv-linterUnorderedKeywarning for consistency.🧰 Tools
🪛 dotenv-linter (3.3.0)
[warning] 28-28: [UnorderedKey] The NOTIFICATIONS_API_BASE_URL key should go before the POSTGRES_DB_URI key
apps/api/src/core/services/shutdown-server/shutdown-server.ts (1)
24-26: KeepingisShuttingDownfalseafter completion re-opens the gateOnce the shutdown sequence has completed, further invocations of
shutdownServer()will run again because the flag is reset.
When the process is already on its way out this is harmless, but during tests or when multiple subsystems reuse the helper it can lead to duplicate disposal attempts.Consider leaving the flag
true(or introducing a separatehasShutdownflag) so subsequent calls are always short-circuited:- isShuttingDown = false; + // keep the flag set to prevent repeat shutdown attempts + // isShuttingDown = false;apps/api/src/core/services/shutdown-server/shutdown-server.spec.ts (2)
15-20: Test still passes even if the early-return bug sneaks inBecause
return;yields an implicit resolved promise, the concurrency test would keep passing even ifshutdownServer()stopped returning a promise altogether for the second and subsequent invocations.Add an explicit assertion that every
shutdownServer()call yields aPromiseto strengthen the contract:const results = Array.from({ length: 5 }, () => shutdownServer(server, appLogger, onShutdown)); results.forEach(r => expect(r).toBeInstanceOf(Promise)); await Promise.all(results);
40-47: Missing verification that the outer promise fulfillsThe third test checks that the error is logged, but it doesn’t assert that
shutdownServer()eventually resolves.
Add an assertion such as:await expect(shutdownServer(server, appLogger, onShutdown)).resolves.toBeUndefined();to catch potential hangs introduced by future refactors.
apps/api/src/core/services/config/config.service.ts (1)
3-6: Add a default forCinConfigServiceOptionsto match the class generic
ConfigServicegivesCa default, butConfigServiceOptionsdoes not.
Without it, callers have to specify both generics when they only care aboutE, which is inconsistent and noisy.-interface ConfigServiceOptions<E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, C extends Record<string, any>> { +interface ConfigServiceOptions< + E extends ZodObject<ZodRawShape> | ZodEffects<ZodObject<ZodRawShape>>, + C extends Record<string, unknown> = Record<string, unknown> +> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
apps/api/env/.env.functional.test(1 hunks)apps/api/env/.env.local.sample(1 hunks)apps/api/env/.env.production(1 hunks)apps/api/env/.env.sample(1 hunks)apps/api/env/.env.unit.test(1 hunks)apps/api/package.json(1 hunks)apps/api/src/app.ts(2 hunks)apps/api/src/auth/services/auth.service.ts(1 hunks)apps/api/src/core/config/env.config.ts(1 hunks)apps/api/src/core/services/config/config.service.ts(1 hunks)apps/api/src/core/services/core-config/core-config.service.ts(1 hunks)apps/api/src/core/services/execution-context/execution-context.service.ts(2 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.ts(1 hunks)apps/api/src/core/services/feature-flags/feature-flags.ts(1 hunks)apps/api/src/core/services/shutdown-server/shutdown-server.spec.ts(1 hunks)apps/api/src/core/services/shutdown-server/shutdown-server.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.spec.ts(2 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(3 hunks)apps/api/src/user/controllers/user/user.controller.ts(1 hunks)packages/docker/docker-compose.dev.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/api/env/.env.functional.test
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/api/env/.env.local.sample
- apps/api/src/core/services/core-config/core-config.service.ts
- apps/api/package.json
- apps/api/env/.env.production
- apps/api/src/user/controllers/user/user.controller.ts
- apps/api/src/notifications/routes/proxy/proxy.route.spec.ts
- apps/api/src/app.ts
- apps/api/src/auth/services/auth.service.ts
- apps/api/src/core/services/feature-flags/feature-flags.ts
- apps/api/src/core/services/execution-context/execution-context.service.ts
- packages/docker/docker-compose.dev.yml
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- apps/api/src/notifications/routes/proxy/proxy.route.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.ts
- apps/api/src/core/config/env.config.ts
🧰 Additional context used
🧠 Learnings (1)
apps/api/src/core/services/config/config.service.ts (1)
Learnt from: stalniy
PR: akash-network/console#1362
File: apps/api/src/core/services/config/config.service.ts:8-9
Timestamp: 2025-05-24T04:25:48.222Z
Learning: In the ConfigService class in apps/api/src/core/services/config/config.service.ts, the `{}` type is used as the default for generic parameter `C` because it's intersected with `z.infer<E>` to create `C & z.infer<E>`. Using `Record<string, never>` instead would break type checking for the merged types since it represents an object that cannot have any properties.
🧬 Code Graph Analysis (1)
apps/api/src/core/services/shutdown-server/shutdown-server.ts (2)
apps/notifications/src/common/services/shutdown/shutdown.service.ts (1)
onShutdown(8-10)packages/logging/src/servicies/logger/logger.service.ts (1)
error(107-109)
🪛 dotenv-linter (3.3.0)
apps/api/env/.env.sample
[warning] 39-39: [UnorderedKey] The FEATURE_FLAGS_ENABLE_ALL key should go before the FEE_ALLOWANCE_REFILL_AMOUNT key
[warning] 40-40: [UnorderedKey] The UNLEASH_SERVER_API_TOKEN key should go before the WEBSITE_URL key
[warning] 41-41: [UnorderedKey] The UNLEASH_SERVER_API_URL key should go before the WEBSITE_URL key
apps/api/env/.env.unit.test
[warning] 28-28: [UnorderedKey] The NOTIFICATIONS_API_BASE_URL key should go before the POSTGRES_DB_URI key
🪛 Biome (1.9.4)
apps/api/src/core/services/config/config.service.ts
[error] 9-9: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: test-provider-console-build
- GitHub Check: test-provider-proxy-build
- GitHub Check: validate-provider-proxy
- GitHub Check: test-indexer-build
- GitHub Check: validate-deploy-web
- GitHub Check: test-deploy-web-build
- GitHub Check: validate-stats-web
- GitHub Check: test-stats-web-build
- GitHub Check: validate-notifications
- GitHub Check: test-api-build
- GitHub Check: validate-api
- GitHub Check: Validate local packages
🔇 Additional comments (2)
apps/api/env/.env.unit.test (1)
30-30: Enable all feature flags for unit tests
SettingFEATURE_FLAGS_ENABLE_ALL=trueensures feature flags are globally enabled during unit tests, aligning with the introducedFeatureFlagsServiceoverride.apps/api/src/core/services/shutdown-server/shutdown-server.ts (1)
8-11: 🛠️ Refactor suggestionReturn a resolved promise when shutdown is already in-progress
return;inside anasyncfunction produces an implicitPromise<undefined>, but the consumers ofshutdownServer()expect aPromise<void>that is always present.
Returningundefinedcan breakawait/Promise.allcall-sites that rely on a guaranteed promise object (your tests work only because Jest tolerates it).- if (isShuttingDown) return; + if (isShuttingDown) { + // ensure the caller still receives a settled promise + return Promise.resolve(); + }Likely an incorrect or invalid review comment.
stalniy
force-pushed
the
feat/feature-flags-notifications
branch
from
ef21de4 to
a4bc5ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/api/src/core/services/shutdown-server/shutdown-server.spec.ts (3)
8-20: Mockserver.closeasynchronously to better emulate real-world timing
jest.fn().mockImplementation(cb => cb())executes the callback synchronously.
http.Server.close()in Node calls its callback on the next tick, so the unit test is not exercising any race conditions that might exist around asynchronous completion. Switching tosetImmediate(orprocess.nextTick) will make the “many concurrent calls” scenario closer to production reality.-const server = mock<ServerType>({ - close: jest.fn().mockImplementation(cb => cb()) -}); +const server = mock<ServerType>({ + close: jest.fn().mockImplementation(cb => { + // defer to next tick to mimic http.Server behaviour + setImmediate(cb); + }), +});This change keeps the test fast while increasing its fidelity.
24-27: Add an assertion thatserver.closewas invokedThe error-path test checks that the error is logged, but it doesn’t verify that
shutdownServeractually attempted to close the server. A quick extra expectation will guard against regressions where the close call is skipped entirely.await shutdownServer(server, appLogger, onShutdown); +expect(server.close).toHaveBeenCalledTimes(1); expect(appLogger.error).toHaveBeenCalledWith({ event: "SERVER_CLOSE_ERROR", error });
42-45: Avoidas jest.Mockby typing the mock function directlyThe cast hides type-safety issues and can mask accidental signature changes. Provide the generic signature to
jest.fn()instead:-close: jest.fn().mockImplementation(() => { - throw error; -}) as jest.Mock +close: jest.fn<Parameters<ServerType["close"]>[0], void>(() => { + throw error; +}),This keeps strict typing intact without relying on a broad cast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/app.ts(2 hunks)apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts(1 hunks)apps/api/src/core/services/shutdown-server/shutdown-server.spec.ts(1 hunks)apps/api/src/core/services/shutdown-server/shutdown-server.ts(1 hunks)apps/api/src/notifications/routes/proxy/proxy.route.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/api/src/app.ts
- apps/api/src/core/services/shutdown-server/shutdown-server.ts
- apps/api/src/core/services/feature-flags/feature-flags.service.spec.ts
- apps/api/src/notifications/routes/proxy/proxy.route.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test-stats-web-build
- GitHub Check: validate-notifications
- GitHub Check: test-provider-console-build
- GitHub Check: test-deploy-web-build
- GitHub Check: validate-deploy-web
- GitHub Check: validate-api
- GitHub Check: test-api-build
- GitHub Check: test-indexer-build
- GitHub Check: Validate local packages
…#1472) * feat: adds feature flags support to console-api's notifications proxy * chore: improve coverage
3 participants
Why
refs: #1454
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores