feat(build): add build-time secret injection for launchdarkly #157
Conversation
- Add reusable workflow for AWS Secrets Manager integration - Update Dockerfile with BuildKit secret mounts for secure injection - Configure Angular define feature for build-time constant replacement - Implement OpenFeature with LaunchDarkly provider - Add feature flag service and provider with SSR support - Update all Docker build workflows to use reusable secret workflow - Add comprehensive documentation for build-time secrets pattern - Support local development with fallback values for ng serve LFXV2-750 Signed-off-by: Asitha de Silva <[email protected]>
github-actions
bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
|
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. WalkthroughIntegrates LaunchDarkly feature flags via OpenFeature into the Angular frontend. Adds build-time secret injection from AWS Secrets Manager through GitHub Actions OIDC and Docker to the application bundle. Implements FeatureFlagService with signal-based reactive flag accessors. Feature-gates sidebar Projects item and organization selector via flags. Includes architecture and secrets management documentation. Changes
Sequence DiagramssequenceDiagram
participant GHA as GitHub Actions
participant AWS as AWS Secrets Manager
participant Docker as Docker Build
participant Bundle as App Bundle
participant Runtime as Runtime
GHA->>GHA: OIDC Auth (configure credentials)
GHA->>AWS: Retrieve LaunchDarkly Client ID
AWS-->>GHA: Return secret value
GHA->>Docker: Pass LAUNCHDARKLY_CLIENT_ID via secret-env
Docker->>Docker: Mount secret at /run/secrets/LAUNCHDARKLY_CLIENT_ID
Docker->>Bundle: Inject via --define LAUNCHDARKLY_CLIENT_ID
Bundle->>Bundle: Build-time constant embedded in JS
Runtime->>Runtime: Loaded in browser at startup
sequenceDiagram
participant App as App Startup
participant FeatureFlag as FeatureFlagService
participant Provider as OpenFeature Provider
participant LD as LaunchDarkly
App->>FeatureFlag: initialize(user)
FeatureFlag->>Provider: Create LaunchDarklyClientProvider
Provider->>LD: Connect with streaming
LD-->>Provider: Flag values + subscribe to updates
Provider-->>FeatureFlag: Provider ready
FeatureFlag->>FeatureFlag: Store context in signal
FeatureFlag-->>App: Promise resolved
App->>FeatureFlag: getBooleanFlag('showProjects')
FeatureFlag->>Provider: Evaluate flag with context
Provider-->>FeatureFlag: Return value
FeatureFlag-->>App: Return Signal<boolean>
Note over LD,FeatureFlag: On flag change, provider emits event
LD-->>Provider: FlagChanged event
Provider-->>FeatureFlag: Trigger event handler
FeatureFlag->>FeatureFlag: refreshFlags() → update context signal
FeatureFlag->>FeatureFlag: All flag Signals recompute
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
🔇 Additional comments (10)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive build-time secret injection system for LaunchDarkly client IDs, enabling secure feature flag management in the LFX One application. The implementation follows a defense-in-depth approach using AWS Secrets Manager, GitHub OIDC authentication, Docker BuildKit secret mounts, and Angular's build-time constant replacement.
Key Changes:
- Implemented OpenFeature-based feature flag system with LaunchDarkly provider and signal-based reactivity
- Created reusable GitHub Actions workflow for secure AWS secret retrieval with validation and masking
- Modified Docker build process to inject secrets at build time without persisting them in image layers
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Added OpenFeature and LaunchDarkly SDK dependencies for feature flag support |
packages/shared/src/interfaces/feature-flags.interface.ts |
Defined TypeScript interfaces for evaluation context and LaunchDarkly configuration |
packages/shared/src/interfaces/index.ts |
Exported feature flag interfaces for use across packages |
docs/build-time-secrets.md |
Comprehensive documentation for build-time secret injection architecture and workflows |
docs/architecture/frontend/feature-flags.md |
Detailed guide on feature flag implementation, usage patterns, and best practices |
apps/lfx-one/src/environments/environment*.ts |
Added LaunchDarkly client ID configuration with build-time constant injection |
apps/lfx-one/src/app/shared/services/feature-flag.service.ts |
Signal-based feature flag service with OpenFeature integration and real-time updates |
apps/lfx-one/src/app/shared/providers/feature-flag.provider.ts |
APP_INITIALIZER provider for OpenFeature/LaunchDarkly setup with SSR guards |
apps/lfx-one/src/app/modules/dashboards/board-member/* |
Example usage: conditional organization selector rendering based on feature flag |
apps/lfx-one/src/app/layouts/main-layout/* |
Example usage: dynamic sidebar navigation filtering using computed signals |
apps/lfx-one/src/app/app.config.ts |
Registered feature flag provider in application configuration |
apps/lfx-one/src/app/app.component.ts |
Initialized feature flags with authenticated user context |
apps/lfx-one/package.json |
Added OpenFeature and LaunchDarkly client SDK dependencies |
apps/lfx-one/angular.json |
Configured define option for build-time constant replacement across all build configurations |
apps/lfx-one/.env.example |
Added LaunchDarkly configuration example for local development |
Dockerfile |
Implemented BuildKit secret mount for secure build-time secret injection |
.github/workflows/get-launchdarkly-secrets.yml |
Reusable workflow for AWS Secrets Manager retrieval with OIDC auth and validation |
.github/workflows/docker-build-*.yml |
Updated all Docker build workflows to use secret retrieval workflow and pass secrets to builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.github/workflows/get-launchdarkly-secrets.yml (1)
24-30: Consider making the IAM role ARN configurable.The OIDC authentication is implemented securely. However, the hardcoded IAM role ARN at Line 29 may require updates if the AWS account or role changes. Consider using a GitHub environment variable or repository secret for the role ARN to improve maintainability.
Example refactor:
- role-to-assume: arn:aws:iam::788942260905:role/github-actions-deploy + role-to-assume: ${{ secrets.AWS_GITHUB_ACTIONS_ROLE_ARN }}apps/lfx-one/src/app/shared/providers/feature-flag.provider.ts (1)
28-33: Consider timeout implications.The LaunchDarkly provider is configured correctly. The 5-second initialization timeout (Line 30) is reasonable but may cause feature flags to be unavailable on slow connections. Consider whether this timeout aligns with your UX requirements, especially for users with slower network conditions.
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (1)
52-62: Consider more robust item identification.The computed signal correctly implements reactive filtering based on the feature flag. However, filtering by
item.label !== 'Projects'couples the logic to the display string. If the label changes (e.g., for i18n), the filter will break.Consider adding an
idproperty toSidebarMenuItemfor more robust identification:- // Filter out Projects if feature flag is disabled - if (!this.showProjectsInSidebar()) { - return items.filter((item) => item.label !== 'Projects'); - } + // Filter out Projects if feature flag is disabled + if (!this.showProjectsInSidebar()) { + return items.filter((item) => item.routerLink !== '/projects'); + }Alternatively, use
routerLinkas shown above, which is less likely to change than display text.apps/lfx-one/angular.json (1)
82-84: Consider removing redundant configuration-level defines.Each build configuration duplicates the root-level
definewith identical values. Since Angular merges configuration with base options, the root-level define (lines 57-59) should suffice.You can simplify by removing the
defineblocks from individual configurations and relying on the root-level definition:"fileReplacements": [ { "replace": "src/environments/environment.ts", "with": "src/environments/environment.prod.ts" } - ], - "define": { - "LAUNCHDARKLY_CLIENT_ID": "''" - } + ] },Apply this pattern to all configurations (staging, development, local) to reduce duplication. The Dockerfile can still override via
--defineflag.Also applies to: 106-108, 120-122, 128-130
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (24)
.github/workflows/docker-build-main.yml(2 hunks).github/workflows/docker-build-pr.yml(3 hunks).github/workflows/docker-build-tag.yml(3 hunks).github/workflows/get-launchdarkly-secrets.yml(1 hunks)Dockerfile(1 hunks)apps/lfx-one/.env.example(1 hunks)apps/lfx-one/angular.json(4 hunks)apps/lfx-one/package.json(1 hunks)apps/lfx-one/src/app/app.component.ts(3 hunks)apps/lfx-one/src/app/app.config.ts(2 hunks)apps/lfx-one/src/app/layouts/main-layout/main-layout.component.html(2 hunks)apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts(3 hunks)apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html(1 hunks)apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.ts(2 hunks)apps/lfx-one/src/app/shared/providers/feature-flag.provider.ts(1 hunks)apps/lfx-one/src/app/shared/services/feature-flag.service.ts(1 hunks)apps/lfx-one/src/environments/environment.dev.ts(2 hunks)apps/lfx-one/src/environments/environment.prod.ts(2 hunks)apps/lfx-one/src/environments/environment.staging.ts(2 hunks)apps/lfx-one/src/environments/environment.ts(2 hunks)docs/architecture/frontend/feature-flags.md(1 hunks)docs/build-time-secrets.md(1 hunks)packages/shared/src/interfaces/feature-flags.interface.ts(1 hunks)packages/shared/src/interfaces/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 125
File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350
Timestamp: 2025-10-21T21:19:13.599Z
Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.
Applied to files:
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html
🧬 Code graph analysis (4)
apps/lfx-one/src/app/shared/providers/feature-flag.provider.ts (4)
apps/lfx-one/src/environments/environment.dev.ts (1)
environment(7-20)apps/lfx-one/src/environments/environment.prod.ts (1)
environment(7-20)apps/lfx-one/src/environments/environment.staging.ts (1)
environment(7-20)apps/lfx-one/src/environments/environment.ts (1)
environment(7-21)
apps/lfx-one/src/app/app.config.ts (1)
apps/lfx-one/src/app/shared/providers/feature-flag.provider.ts (1)
provideFeatureFlags(46-46)
apps/lfx-one/src/app/shared/services/feature-flag.service.ts (1)
packages/shared/src/interfaces/feature-flags.interface.ts (1)
EvaluationContext(8-17)
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (1)
packages/shared/src/interfaces/components.interface.ts (1)
SidebarMenuItem(267-288)
🪛 GitHub Actions: Markdown Lint
docs/build-time-secrets.md
[error] 22-22: MD040/fenced-code-language Fenced code blocks should have a language specified [Context: "```"]
🪛 GitHub Check: markdown-lint
docs/build-time-secrets.md
[failure] 111-111: Fenced code blocks should have a language specified [Context: ""] docs/build-time-secrets.md:111 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
[failure] 22-22: Fenced code blocks should have a language specified [Context: ""] docs/build-time-secrets.md:22 MD040/fenced-code-language Fenced code blocks should have a language specified [Context: ""]
🪛 LanguageTool
docs/build-time-secrets.md
[uncategorized] ~36-~36: The official name of this software platform is spelled with a capital “H”.
Context: ...use a reusable GitHub Actions workflow (.github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
[uncategorized] ~211-~211: The official name of this software platform is spelled with a capital “H”.
Context: ... Step 2: Update Reusable Workflow Edit .github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
docs/architecture/frontend/feature-flags.md
[grammar] ~561-~561: Ensure spelling is correct
Context: ...y**: Updates typically arrive within 50-200ms - Automatic Reconnection: Provider handl...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1337-~1337: Use a hyphen to join words.
Context: ...g Solution: - Verify browser check in feature-flag.provider.ts - Initiali...
(QB_NEW_EN_HYPHEN)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (50)
docs/build-time-secrets.md (1)
1-345: Excellent documentation for build-time secrets management.This document provides comprehensive coverage of the build-time secret injection architecture, including security considerations, troubleshooting, and practical examples. The step-by-step guide for adding new secrets will be valuable for future maintenance.
docs/architecture/frontend/feature-flags.md (1)
1-1460: Outstanding feature flags documentation.This is exceptionally comprehensive documentation that covers architecture, implementation details, usage patterns, best practices, error handling, and troubleshooting. The real-world examples and ASCII diagrams make it easy to understand the system. This will be an excellent resource for developers working with feature flags.
apps/lfx-one/src/app/app.component.ts (1)
10-10: LGTM: Feature flag initialization is correctly placed.The initialization happens after authentication is confirmed and passes the user context for targeting. Error handling is graceful, allowing the app to continue with default flag values if initialization fails. This matches the recommended pattern from the feature flags documentation.
Also applies to: 23-23, 57-61
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html (1)
13-30: LGTM: Conditional rendering with feature flag is correctly implemented.The organization selector is now controlled by a feature flag with proper signal invocation. This enables progressive rollout and quick rollback capabilities for this UI element.
apps/lfx-one/.env.example (1)
69-73: LGTM: Clear documentation for LaunchDarkly configuration.The comments clearly explain that this value is injected at build time via Docker and provide guidance for local development. This aligns with the build-time secrets architecture documented in the PR.
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.html (1)
7-7: LGTM: Signal invocation is correctly updated.The sidebar items binding now correctly invokes the computed signal to get the filtered navigation items. This enables dynamic filtering based on feature flags as described in the documentation.
Also applies to: 26-26
apps/lfx-one/src/app/app.config.ts (1)
18-18: LGTM: Feature flags provider is correctly registered.The
provideFeatureFlags()provider is properly imported and added to the application configuration. This initializes the OpenFeature provider during application bootstrap viaAPP_INITIALIZER.Also applies to: 49-49
packages/shared/src/interfaces/index.ts (1)
52-53: LGTM: Feature flags interface export follows established patterns.The export for feature flag interfaces is correctly structured and maintains consistency with other interface exports in this index file.
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.ts (2)
11-11: LGTM: Clean service integration.The FeatureFlagService import and injection follow Angular best practices with proper visibility and readonly modifier.
Also applies to: 27-27
36-37: The implementation is correct and follows sound architectural patterns.The code uses a graceful fallback design where if the
'organization-selector'flag is unavailable in LaunchDarkly or the flag service is uninitialized, the default value oftrueensures the organization selector remains visible. The feature flag service is built with error handling that logs issues but continues execution, making the system resilient to flag configuration issues.Flag key validation occurs at runtime through LaunchDarkly; the codebase intentionally doesn't maintain a central registry of keys to allow flexibility. This is appropriate for the OpenFeature abstraction pattern being used.
.github/workflows/docker-build-main.yml (3)
12-15: LGTM: Correct workflow-level permissions.The workflow-level permissions correctly grant
id-token: writefor AWS OIDC authentication andpackages: writefor container registry operations.
22-29: LGTM: Proper secret retrieval and propagation.The workflow correctly orchestrates secret retrieval through the reusable workflow and propagates the LaunchDarkly client ID to the build job via outputs and environment variables.
75-76: Verification complete—the code is correct.docker/build-push-action@v5 supports the secret-envs input, and the syntax correctly maps the environment variable to a Docker secret for BuildKit secret mounts.
apps/lfx-one/src/environments/environment.staging.ts (2)
4-5: LGTM: Correct ambient declaration.The ambient declaration properly types the build-time constant that will be injected via Angular's
definefeature.
17-19: LGTM: Safe fallback for missing client ID.The implementation correctly falls back to an empty string when the build-time constant is not provided, which aligns with the provider's falsy check to disable feature flags gracefully.
.github/workflows/get-launchdarkly-secrets.yml (4)
1-15: LGTM: Proper reusable workflow structure.The workflow is correctly configured as a reusable workflow with appropriate permissions and output definition.
32-37: LGTM: Correct AWS Secrets Manager integration.The secret retrieval step properly uses the official AWS action and matches the documented secret path from the PR prerequisites.
39-61: LGTM: Robust secret validation.The validation step properly checks for the presence of required secrets, parses JSON correctly, and provides clear error messages with fail-fast behavior.
63-74: LGTM: Defensive masking approach.The output step correctly parses and masks the client ID. Note that LaunchDarkly client-side SDK IDs are typically public values (visible in browser requests), but the explicit masking is a safe defensive practice that aligns with the PR's security-focused approach.
Dockerfile (1)
27-30: LGTM: Secure BuildKit secret injection.The implementation correctly uses BuildKit secret mounts to avoid persisting secrets in image layers. The define flag format is correct for Angular's build-time constant replacement.
Note: If the secret file is empty, the build will proceed with an empty string, which is handled gracefully by the environment fallback logic. The validation step in
get-launchdarkly-secrets.ymlensures the secret exists before reaching this point.apps/lfx-one/src/app/shared/providers/feature-flag.provider.ts (4)
17-20: LGTM: Proper SSR handling.The server-side guard correctly prevents OpenFeature initialization during server-side rendering using the standard
typeof window === 'undefined'check.
22-26: LGTM: Graceful fallback for missing client ID.The implementation correctly handles missing client IDs with a warning message and allows the app to continue without feature flags, which is appropriate for this scenario.
35-39: LGTM: Proper error handling.The provider initialization correctly uses async/await and handles errors gracefully by logging and allowing the app to continue without feature flags.
46-46: LGTM: Correct Angular 19 provider pattern.The provider export properly uses Angular 19's
provideAppInitializerAPI with the correct return type.apps/lfx-one/src/environments/environment.dev.ts (1)
4-5: LGTM: Consistent environment configuration.The implementation matches the pattern in
environment.staging.tswith proper ambient declaration and safe fallback behavior.Also applies to: 17-19
apps/lfx-one/package.json (1)
43-45: All dependency versions are current and free from known vulnerabilities.Verification confirms that all four dependencies are pinned to their latest available versions on npm, with no security advisories detected. The use of caret ranges (^) is appropriate for stable versions. No action required.
apps/lfx-one/src/environments/environment.prod.ts (2)
4-5: LGTM: Ambient declaration for build-time constant.The ambient declaration appropriately types the build-time constant as
string | undefined, which aligns with the Angular define mechanism where the value may not be provided during local development.
17-19: LGTM: Build-time constant with appropriate fallback.The property correctly uses the build-time constant with an empty string fallback for production builds. This ensures the application won't crash if the secret injection fails, though feature flags will be disabled.
apps/lfx-one/src/environments/environment.ts (2)
4-5: LGTM: Consistent ambient declaration.The ambient declaration matches the production environment, maintaining consistency across the codebase.
17-20: LGTM: Development environment with local fallback.The hardcoded dev client ID fallback enables local development without requiring secret injection. Since LaunchDarkly client IDs are public client-side values, this approach is secure and appropriate for the development workflow.
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (3)
5-5: LGTM: Feature flag service integration.The imports and service injection follow Angular best practices for integrating the feature flag system into the layout component.
Also applies to: 9-9, 25-25
30-31: LGTM: Feature flag for conditional sidebar rendering.The boolean flag with a default value of
trueensures the Projects item remains visible when the feature flag service is uninitialized or the flag is not configured, maintaining backward compatibility.
33-50: LGTM: Base sidebar items refactoring.Renaming to
baseSidebarItemsclearly distinguishes the static configuration from the computed, feature-flag-awaresidebarItems, improving code clarity.packages/shared/src/interfaces/feature-flags.interface.ts (2)
8-17: LGTM: Well-documented evaluation context interface.The interface appropriately models OpenFeature's evaluation context with optional properties, enabling flexible user targeting for LaunchDarkly flag evaluation.
22-34: LGTM: Comprehensive LaunchDarkly configuration interface.The interface correctly models LaunchDarkly provider configuration with
clientIdas the only required property, providing sensible optional configuration for timeouts, streaming, and logging..github/workflows/docker-build-pr.yml (4)
15-15: LGTM: OIDC authentication permission.The
id-token: writepermission enables GitHub Actions to authenticate with AWS using OIDC, eliminating the need for long-lived credentials.
22-30: LGTM: Secure secret retrieval with fork protection.The conditional execution ensures secrets are only retrieved for legitimate preview deployments from trusted sources, preventing secret exposure to forked repositories.
33-33: LGTM: Proper job dependency and secret propagation.The dependency chain ensures secrets are retrieved before the build starts, and the environment variable correctly propagates the LaunchDarkly client ID from the get-secrets job output.
Also applies to: 42-43
89-90: LGTM: BuildKit secret mount configuration.The
secret-envsmapping correctly passes the LaunchDarkly client ID to the Docker build as a secret mount, ensuring it doesn't persist in the image layers..github/workflows/docker-build-tag.yml (3)
13-14: LGTM: Top-level permissions configuration.Moving permissions to the workflow level is cleaner and ensures both OIDC authentication and package publishing are properly authorized.
23-24: LGTM: Consistent secret retrieval pattern.The workflow correctly implements the same secret retrieval pattern as other workflows, ensuring consistent handling of build-time secrets across all build types.
Also applies to: 27-27, 29-30
93-94: LGTM: BuildKit secret mount for release builds.The secret injection mechanism is consistently implemented across all Docker build workflows.
apps/lfx-one/angular.json (1)
57-59: LGTM: Root-level define configuration.The base-level define configuration ensures
LAUNCHDARKLY_CLIENT_IDis defined for all build configurations with a safe empty string default.apps/lfx-one/src/app/shared/services/feature-flag.service.ts (7)
11-16: LGTM: Well-structured service fields.The service correctly uses private signals for internal state management and exposes a readonly signal for external consumption, following Angular best practices.
22-45: LGTM: Robust initialization with proper error handling.The initialization method correctly handles the OpenFeature client setup, user context configuration, and error cases. The fallback chain for
targetingKey(preferred_username || username || sub) provides good resilience for different authentication providers.
51-67: LGTM: Reactive boolean flag accessor.The computed signal correctly establishes a reactive dependency on the context signal, ensuring automatic re-evaluation when flags change. The error handling and default value fallback are appropriate.
73-89: LGTM: Consistent flag accessor pattern.The string and number flag accessors follow the same robust pattern as the boolean accessor, with appropriate type-specific default values.
Also applies to: 95-111
117-133: LGTM: Type-safe object flag accessor.The generic object flag accessor correctly constrains the type to
JsonValuefrom OpenFeature, ensuring type safety for JSON-serializable flag values.
138-157: Consider implementing event handler cleanup.The event handlers are properly set up for real-time flag updates. However, since the service is provided in root, the handlers will persist for the application lifetime, which is acceptable. If the service scope changes in the future, you'll need to implement cleanup (e.g., in
ngOnDestroyor viaDestroyRef).Current implementation is fine for a root-scoped service, but document this assumption or add cleanup if the service scope might change.
162-167: LGTM: Signal refresh mechanism.Creating a new object reference with the spread operator is an effective way to trigger re-evaluation of all computed flag signals. This approach leverages Angular's signal change detection correctly.
- Remove duplicate EvaluationContext and LaunchDarklyConfig interfaces from shared package (available from @openfeature/web-sdk) - Update feature flags documentation to show provideAppInitializer instead of deprecated APP_INITIALIZER pattern - Replace hardcoded LaunchDarkly client IDs with placeholders in documentation - Add preferred_username field to User interface for feature flag targeting - Improve type safety in feature-flag.service.ts by using User type LFXV2-750 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/build-time-secrets.md (1)
199-207: Minor path placeholder inconsistency in AWS Secrets Manager exampleIn Step 1 you describe the pattern as
/cloudops/managed-secrets/[service]/[app]/[secret_name], but the CLI example hardcodesservice:--name /cloudops/managed-secrets/service/lfx_one/new_secretConsider keeping it clearly templated to avoid copy‑paste confusion, e.g.:
- --name /cloudops/managed-secrets/service/lfx_one/new_secret \ + --name /cloudops/managed-secrets/<service>/lfx_one/new_secret \or mirror the actual service name you expect.
docs/architecture/frontend/feature-flags.md (1)
568-588: Environment configuration section should reflect build-timeLAUNCHDARKLY_CLIENT_IDflowThis section still suggests hardcoding
launchDarklyClientIdvalues inenvironment.ts/environment.prod.ts:export const environment = { production: false, launchDarklyClientId: 'your-dev-client-id-here', };Given the new build-time injection via
--define LAUNCHDARKLY_CLIENT_ID(documented indocs/build-time-secrets.md), it would be clearer to either:
- Show the current pattern with the injected constant and dev fallback, e.g.:
+declare const LAUNCHDARKLY_CLIENT_ID: string | undefined; + export const environment = { production: false, - launchDarklyClientId: 'your-dev-client-id-here', + launchDarklyClientId: + typeof LAUNCHDARKLY_CLIENT_ID !== 'undefined' + ? LAUNCHDARKLY_CLIENT_ID + : 'dev-client-id-fallback', };(and the analogous prod variant), or
- Explicitly reference
docs/build-time-secrets.mdhere and note that the preferred configuration in CI is via build-time secrets rather than checked-in client IDs.That keeps the architecture doc aligned with the actual deployment story.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Dockerfile(1 hunks)apps/lfx-one/src/app/shared/services/feature-flag.service.ts(1 hunks)docs/architecture/frontend/feature-flags.md(1 hunks)docs/build-time-secrets.md(1 hunks)packages/shared/src/interfaces/auth.interface.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🧰 Additional context used
🧬 Code graph analysis (1)
apps/lfx-one/src/app/shared/services/feature-flag.service.ts (1)
packages/shared/src/interfaces/auth.interface.ts (1)
User(8-43)
🪛 LanguageTool
docs/architecture/frontend/feature-flags.md
[grammar] ~560-~560: Ensure spelling is correct
Context: ...y**: Updates typically arrive within 50-200ms - Automatic Reconnection: Provider handl...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~1336-~1336: Use a hyphen to join words.
Context: ...g Solution: - Verify browser check in feature-flag.provider.ts - Initiali...
(QB_NEW_EN_HYPHEN)
docs/build-time-secrets.md
[uncategorized] ~36-~36: The official name of this software platform is spelled with a capital “H”.
Context: ...use a reusable GitHub Actions workflow (.github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
[uncategorized] ~211-~211: The official name of this software platform is spelled with a capital “H”.
Context: ... Step 2: Update Reusable Workflow Edit .github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
🔇 Additional comments (3)
packages/shared/src/interfaces/auth.interface.ts (1)
37-38:preferred_usernamefield addition looks correct and aligns with usageThe optional
preferred_usernameclaim matches how the feature-flag user context is built and keeps the sharedUserinterface in sync with Auth0/OIDC-style profiles.apps/lfx-one/src/app/shared/services/feature-flag.service.ts (2)
4-18: Typed user context + signal-based API look solidUsing the shared
Usertype here (includingpreferred_username) and exposing read‑only signals for initialization status and flag values is a clean, type‑safe integration of OpenFeature with Angular signals.
23-45: Initialization is one-shot; flags won’t adapt if the active user changes
initializeshort‑circuits whenthis.isInitialized()is true, so if your app ever supports changing the authenticated user without a full reload (logout/login, account switch, etc.), the evaluation context and targetingKey will remain tied to the first user.If user switching is a realistic scenario, consider either:
- Allowing
initializeto update the context when the user identity changes, or- Adding a dedicated
setUserContext(user: User)/reset()method that updates context (and, if needed, re-registers handlers) without duplicating setup.If the app is guaranteed single-user per browser session, the current behavior is fine but worth documenting in the service JSDoc.
- Fix Dockerfile to use --define=KEY=VALUE without -- separator - Update documentation with verified working syntax - Remove incorrect -- separator from all examples - Tested build completes successfully and value is injected The Angular CLI requires --define=KEY=VALUE format directly on yarn build commands without the -- separator. String values must be quoted within quotes: --define=KEY="'value'" Verified with test build that value is properly injected into bundle. LFXV2-750 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/build-time-secrets.md (1)
1-349: ✅ Comprehensive documentation with all past issues resolved.This is well-structured documentation that clearly explains the build-time secrets architecture. Past review concerns have been addressed:
- ✅ All fenced code blocks now include language specifiers (lines 22, 46, 111, etc.)
- ✅ Placeholder values used throughout instead of hardcoded credentials (e.g.,
'dev-client-id-fallback','your-client-id-here')- ✅ Strong security guidance emphasizing client-side exposure and appropriate use cases (lines 191–195)
The content provides a clear end-to-end flow (Overview → Architecture → Data Flow → Local Dev → Workflow Integration → Troubleshooting) with practical examples and step-by-step guidance for extending to new secrets. References to related docs are provided.
For readers seeking the complete workflow implementation details (jq extraction, masking steps, etc.), consider adding a note in the "Reusable Workflow" section (lines 34–52) to reference
.github/workflows/get-launchdarkly-secrets.ymlin the PR for the full implementation details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Dockerfile(1 hunks)docs/build-time-secrets.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
🧰 Additional context used
🪛 LanguageTool
docs/build-time-secrets.md
[uncategorized] ~36-~36: The official name of this software platform is spelled with a capital “H”.
Context: ...use a reusable GitHub Actions workflow (.github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
[uncategorized] ~211-~211: The official name of this software platform is spelled with a capital “H”.
Context: ... Step 2: Update Reusable Workflow Edit .github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
- Build shared package separately before UI to avoid TypeScript compiler errors - Use yarn workspace commands for targeted builds - Update Dockerfile to build @lfx-one/shared then lfx-one-ui - Update documentation with workspace build approach - Change --define syntax to use space (matches Angular CLI docs) This resolves the issue where running build from root would fail because the shared package's TypeScript compiler doesn't understand --define flags. Building them separately allows each package to use appropriate build flags. LFXV2-750 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
30-33: Consider adding explicit secret validation for clearer error messages.While the current implementation will fail if the secret is missing (via Docker's BuildKit), adding an explicit validation step would provide clearer error messages and fail faster during builds.
The past review suggested defensive validation. Consider this defensive approach:
RUN --mount=type=secret,id=LAUNCHDARKLY_CLIENT_ID \ + if [ ! -f /run/secrets/LAUNCHDARKLY_CLIENT_ID ]; then \ + echo "Error: LAUNCHDARKLY_CLIENT_ID secret not found" && exit 1; \ + fi && \ LAUNCHDARKLY_CLIENT_ID=$(cat /run/secrets/LAUNCHDARKLY_CLIENT_ID) && \ yarn workspace lfx-one-ui build:${BUILD_ENV} --define LAUNCHDARKLY_CLIENT_ID="'${LAUNCHDARKLY_CLIENT_ID}'"This provides fail-fast behavior with a clear error message if the secret mount fails, improving the developer experience during troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Dockerfile(1 hunks)docs/build-time-secrets.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/build-time-secrets.md
[uncategorized] ~36-~36: The official name of this software platform is spelled with a capital “H”.
Context: ...use a reusable GitHub Actions workflow (.github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
[uncategorized] ~221-~221: The official name of this software platform is spelled with a capital “H”.
Context: ... Step 2: Update Reusable Workflow Edit .github/workflows/get-launchdarkly-secrets.yml...
(GITHUB)
⏰ 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). (1)
- GitHub Check: build-and-push
🔇 Additional comments (2)
Dockerfile (1)
27-28: Shared package build correctly separated.Building the shared package separately before the Angular UI ensures compatibility with TypeScript compiler, which doesn't support the
--defineflag. This is the right approach.docs/build-time-secrets.md (1)
1-363: Comprehensive and well-organized documentation.The documentation effectively guides users through build-time secret injection, from architecture through troubleshooting. Structure is logical, examples are accurate and use proper placeholders, security considerations are clearly highlighted, and the step-by-step guide for adding new secrets is thorough. The past concerns (language specifiers, placeholder values) have been addressed.
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-157.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
Remove get-launchdarkly-secrets.yml reusable workflow and implement direct AWS Secrets Manager retrieval in each job using GITHUB_ENV. This resolves the GitHub Actions issue where masked values cannot be used in workflow outputs. Changes: - Update docker-build-main.yml to retrieve secrets directly in job - Update docker-build-tag.yml to retrieve secrets directly in job - Update docker-build-pr.yml to retrieve secrets directly in job - Delete get-launchdarkly-secrets.yml reusable workflow file - Update build-time-secrets.md documentation to reflect new approach - LaunchDarkly client-side IDs are NOT masked (they're public values) LFXV2-750 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
🧹 Deployment RemovedThe deployment for PR #157 has been removed. |
Summary
Implements secure build-time secret injection for LaunchDarkly client IDs following the pattern established in PR #156. Secrets are retrieved from AWS Secrets Manager during GitHub Actions builds and injected at build time using Docker BuildKit secret mounts and Angular's
definefeature.Key Changes
Infrastructure
.github/workflows/get-launchdarkly-secrets.ymlfor AWS secret retrievalApplication
definefeature for build-time constant replacementng serveDocumentation
docs/build-time-secrets.mddocs/architecture/frontend/feature-flags.mdSecurity Features
Local Development
yarn startwith hardcoded dev client ID fallbackyarn build --definefor build-time injectionPrerequisites
Before merging, ensure AWS Secrets Manager secret exists:
/cloudops/managed-secrets/launchdarkly/lfx_one/ld_client_id{"ld_client_id": "your-client-id"}Test Plan
LFXV2-750