feat: add demo bar with role switcher and clear cache #205
Conversation
jordancclive
commented
Dec 10, 2025
jordancclive
commented
- Add demo-bar component with role toggle buttons (Core Developer, Maintainer, Board Member)
- Add Clear Cache button to demo bar
- Demo bar appears at top of app, above sidebar and content
- Remove persona selector and clear cache from sidebar (relocated to demo bar)
- Use CSS custom properties to offset fixed elements when demo bar is present
- Add demo-bar component with role toggle buttons (Core Developer, Maintainer, Board Member) - Add Clear Cache button to demo bar - Demo bar appears at top of app, above sidebar and content - Remove persona selector and clear cache from sidebar (relocated to demo bar) - Use CSS custom properties to offset fixed elements when demo bar is present
WalkthroughA new demo bar component has been added to the top of the application to manage role selection and cache clearing functionality. The component integrates with PersonaService and CookieRegistryService, and the corresponding UI elements have been removed from the sidebar. App component structure has been updated to include and layout the demo bar wrapper. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DemoBar as DemoBarComponent
participant PersonaService
participant CookieRegistry as CookieRegistryService
rect rgb(200, 220, 240)
Note over User,PersonaService: Role Selection Flow
User->>DemoBar: Select Role
DemoBar->>DemoBar: FormControl valueChanges triggered
DemoBar->>PersonaService: setPersona(selectedRole)
PersonaService->>PersonaService: Update currentPersona
end
rect rgb(240, 200, 200)
Note over User,CookieRegistry: Cache Clear Flow
User->>DemoBar: Click Clear Cache
DemoBar->>CookieRegistry: clearAllCookies()
CookieRegistry->>CookieRegistry: Remove all cookies
DemoBar->>DemoBar: location.reload()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
requested review from
andrest50,
asithade,
dealako and
mauriciozanettisalomao
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-205.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.scss (1)
4-8: Consider reducing the z-index value.A z-index of 9999 is unnecessarily high and may cause conflicts with modals, tooltips, or other overlay elements. Consider using a more reasonable value (e.g., 1000-1050) that fits within your application's z-index hierarchy.
Apply this diff to use a more reasonable z-index:
.demo-bar { - @apply sticky top-0 left-0 right-0 z-[9999] bg-gray-900; + @apply sticky top-0 left-0 right-0 z-[1000] bg-gray-900; height: 48px; flex-shrink: 0; }apps/lfx-one/src/app/app.component.html (1)
5-10: Consider adding data-testid attributes to wrapper elements.The coding guidelines specify that data-testid attributes should be added to new components. Consider adding them to the wrapper divs to enable reliable test targeting:
Apply this diff:
-<div class="demo-bar-wrapper"> +<div class="demo-bar-wrapper" data-testid="app-demo-bar-wrapper"> <lfx-demo-bar /> - <div class="demo-bar-content-wrapper"> + <div class="demo-bar-content-wrapper" data-testid="app-content-wrapper"> <router-outlet /> </div> </div>apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.html (1)
7-18: Add data-testid attributes to role selection elements.Following the coding guidelines, add data-testid attributes to enable reliable test targeting for the role selection functionality:
Apply this diff:
- <div class="role-toggle"> + <div class="role-toggle" data-testid="demo-bar-role-toggle"> <span class="demo-label">Select Role:</span> <ng-container [formGroup]="roleForm"> <p-selectbutton formControlName="role" [options]="roleOptions" optionLabel="label" optionValue="value" size="small" - styleClass="demo-role-toggle"></p-selectbutton> + styleClass="demo-role-toggle" + data-testid="demo-bar-role-selector"></p-selectbutton> </ng-container> </div>apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts (1)
33-33: Remove unused computed property.The
currentRolecomputed property is not referenced in the template or elsewhere in the component. Since the role value is already available viaroleForm.get('role')?.value, this property can be removed.Apply this diff:
public readonly roleForm = new FormGroup({ role: new FormControl<PersonaType>(this.personaService.currentPersona()), }); - public readonly currentRole = computed(() => this.personaService.currentPersona()); - public constructor() {
📜 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 (7)
apps/lfx-one/src/app/app.component.html(1 hunks)apps/lfx-one/src/app/app.component.scss(1 hunks)apps/lfx-one/src/app/app.component.ts(2 hunks)apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.html(1 hunks)apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.scss(1 hunks)apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts(1 hunks)apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.html(0 hunks)
💤 Files with no reviewable changes (1)
- apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.html
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Always use direct imports for standalone components - no barrel exports
Use TypeScript interfaces instead of union types for better maintainability
Do not nest ternary expressions
Prefer interface for defining object shapes in TypeScript
Use path mappings and import aliases as configured in tsconfig.json for imports
Files:
apps/lfx-one/src/app/app.component.tsapps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts
**/*.{html,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always add data-testid attributes when creating new components for reliable test targeting
Files:
apps/lfx-one/src/app/app.component.tsapps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.tsapps/lfx-one/src/app/app.component.htmlapps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.html
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: License headers are required on all source files
Prepend 'Generated with Claude Code (https://claude.ai/code)' if assisted with the code
Files:
apps/lfx-one/src/app/app.component.tsapps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts
**/*.component.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Angular 19 zoneless change detection with signals for component state management
Files:
apps/lfx-one/src/app/app.component.tsapps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts
**/*.{css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Tailwind CSS with PrimeUI plugin and LFX custom colors for styling
Files:
apps/lfx-one/src/app/app.component.scssapps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.scss
**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
Use data-testid naming convention - [section]-[component]-[element] for hierarchical structure
Files:
apps/lfx-one/src/app/app.component.htmlapps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.html
🧬 Code graph analysis (1)
apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts (1)
packages/shared/src/interfaces/persona.interface.ts (1)
PersonaType(8-8)
⏰ 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). (2)
- GitHub Check: build-and-push
- GitHub Check: Agent
🔇 Additional comments (3)
apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.scss (1)
32-61: ::ng-deep usage acknowledged for PrimeNG styling.The use of
::ng-deepis deprecated in Angular but remains necessary for styling third-party components like PrimeNG. The implementation here is appropriate for achieving the desired visual design, though be aware that future Angular versions may remove this feature.apps/lfx-one/src/app/app.component.ts (1)
10-10: LGTM! Clean component integration.The DemoBarComponent is correctly imported using direct import paths and properly added to the component's imports array, following Angular standalone component best practices.
Also applies to: 19-19
apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts (1)
23-27: Reconcile the 'projects' persona type.The
PersonaTypeinterface defines four values ('core-developer' | 'maintainer' | 'projects' | 'board-member'), butroleOptionsonly includes three. The'projects'value exists in the type definition but is not exposed inPERSONA_OPTIONS,PERSONA_PRIORITY, or any UI persona selection. Confirm whether'projects'should be added to the demo bar's role options or removed from thePersonaTypedefinition to avoid type-implementation mismatch.
| // Global styles to offset fixed elements when demo bar is present | ||
| ::ng-deep .demo-bar-wrapper { | ||
| // Offset the fixed sidebar | ||
| .fixed.top-0 { | ||
| top: var(--demo-bar-height, 0) !important; | ||
| } | ||
|
|
||
| // Adjust min-h-screen containers to account for demo bar | ||
| .min-h-screen { | ||
| min-height: calc(100vh - var(--demo-bar-height, 0)); | ||
| } | ||
|
|
||
| // Adjust sticky elements | ||
| .sticky.top-0 { | ||
| top: var(--demo-bar-height, 0) !important; | ||
| } | ||
|
|
||
| // Adjust mobile sticky header | ||
| .sticky.top-0.z-30 { | ||
| top: var(--demo-bar-height, 0) !important; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find elements with fixed/sticky positioning that will be affected by demo bar offsets
echo "=== Finding elements with 'fixed top-0' classes ==="
rg -n "class=.*['\"].*fixed.*top-0" --type=html -C 2
echo -e "\n=== Finding elements with 'sticky top-0' classes ==="
rg -n "class=.*['\"].*sticky.*top-0" --type=html -C 2
echo -e "\n=== Finding min-h-screen usage ==="
rg -n "class=.*['\"].*min-h-screen" --type=html -C 2Repository: linuxfoundation/lfx-v2-ui
Length of output: 4531
Consider using CSS variables or Tailwind utilities instead of ::ng-deep for demo bar offsets.
The global selectors with ::ng-deep will affect all descendant elements matching .fixed.top-0, .sticky.top-0, and .min-h-screen classes throughout the app (found in main-layout sidebar, header, and multiple dashboard components). While these adjustments appear intentional, this approach violates the coding guideline to use Tailwind CSS with PrimeUI plugin for styling. Consider applying the --demo-bar-height CSS variable and offset logic through Tailwind's configuration, component-level classes, or a more scoped utility pattern to avoid relying on ::ng-deep and global style cascading.
🤖 Prompt for AI Agents
In apps/lfx-one/src/app/app.component.scss around lines 20-41, replace the
global ::ng-deep rules that indiscriminately change .fixed.top-0, .sticky.top-0
and .min-h-screen across the app with a scoped, Tailwind-friendly approach:
remove the ::ng-deep block, ensure the --demo-bar-height CSS variable is set on
a scoped container (e.g., the main layout wrapper or :root if truly global), and
create/apply Tailwind utilities or component-level classes (via tailwind.config
or a small plugin/utility class) that compute top and min-height using
calc(100vh - var(--demo-bar-height)) and top: var(--demo-bar-height) so you can
add those utility classes to the sidebar/header/components instead of using
::ng-deep global selectors.
| public constructor() { | ||
| // Sync form with persona service | ||
| this.roleForm.get('role')?.valueChanges.subscribe((value) => { | ||
| if (value) { | ||
| this.personaService.setPersona(value); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Memory leak: unsubscribed form value changes subscription.
The subscription to roleForm.get('role')?.valueChanges in the constructor is never unsubscribed, which will cause a memory leak if the component is destroyed and recreated. With Angular 19's zoneless change detection and signals, consider using toSignal() or the effect() API instead.
Apply this diff to use the effect API:
+import { Component, computed, effect, inject } from '@angular/core';
-import { Component, computed, inject } from '@angular/core';
import { FormControl, FormGroup, ReactiveFormsModule } from '@angular/forms';
import { PersonaType } from '@lfx-one/shared/interfaces';
import { SelectButtonModule } from 'primeng/selectbutton';
import { CookieRegistryService } from '../../services/cookie-registry.service';
import { PersonaService } from '../../services/persona.service';
@Component({
selector: 'lfx-demo-bar',
standalone: true,
imports: [ReactiveFormsModule, SelectButtonModule],
templateUrl: './demo-bar.component.html',
styleUrl: './demo-bar.component.scss',
})
export class DemoBarComponent {
private readonly personaService = inject(PersonaService);
private readonly cookieRegistry = inject(CookieRegistryService);
public readonly roleOptions = [
{ label: 'Core Developer', value: 'core-developer' as PersonaType },
{ label: 'Maintainer', value: 'maintainer' as PersonaType },
{ label: 'Board Member', value: 'board-member' as PersonaType },
];
public readonly roleForm = new FormGroup({
role: new FormControl<PersonaType>(this.personaService.currentPersona()),
});
public readonly currentRole = computed(() => this.personaService.currentPersona());
public constructor() {
- // Sync form with persona service
- this.roleForm.get('role')?.valueChanges.subscribe((value) => {
- if (value) {
- this.personaService.setPersona(value);
- }
- });
+ // Sync form with persona service using effect
+ effect(() => {
+ const role = this.roleForm.get('role')?.value;
+ if (role) {
+ this.personaService.setPersona(role);
+ }
+ });
}
protected clearCache(): void {
this.cookieRegistry.clearAllCookies();
if (typeof window !== 'undefined') {
window.location.reload();
}
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts around
lines 35-42, the subscription to roleForm.get('role')?.valueChanges is never
unsubscribed; replace the raw subscription with Angular Signals: import toSignal
and effect from '@angular/core', convert the control observable to a signal
(using toSignal with an appropriate initialValue), create an effect that reads
the signal and calls personaService.setPersona(value) when value is truthy,
store the returned stop function as a private property, implement OnDestroy and
call that stop function in ngOnDestroy to ensure cleanup.
There was a problem hiding this comment.
Pull request overview
This PR introduces a demo bar component that provides quick access to role switching and cache clearing functionality, primarily intended for demonstration and development purposes. The demo bar appears at the top of the application and relocates features previously found in the sidebar.
Key Changes:
- Creates a new
DemoBarComponentwith role toggle buttons (Core Developer, Maintainer, Board Member) and a Clear Cache button - Relocates persona selection and cache clearing functionality from sidebar to the demo bar
- Implements CSS custom properties (
--demo-bar-height) to dynamically offset fixed/sticky positioned elements throughout the app
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.ts | New component implementing role selection form and cache clearing logic |
| apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.html | Template with role toggle buttons and clear cache button |
| apps/lfx-one/src/app/shared/components/demo-bar/demo-bar.component.scss | Styling for demo bar with PrimeNG SelectButton customization |
| apps/lfx-one/src/app/app.component.ts | Imports DemoBarComponent |
| apps/lfx-one/src/app/app.component.html | Wraps router-outlet with demo bar structure |
| apps/lfx-one/src/app/app.component.scss | Global CSS adjustments to offset fixed/sticky elements when demo bar is present |
| apps/lfx-one/src/app/shared/components/sidebar/sidebar.component.html | Removes persona selector and clear cache menu item from sidebar |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public readonly currentRole = computed(() => this.personaService.currentPersona()); | ||
|
|
There was a problem hiding this comment.
The computed signal currentRole is defined but never used in the component. The role value is already tracked by the form control, making this signal redundant. Consider removing it to reduce unnecessary complexity.
| public readonly currentRole = computed(() => this.personaService.currentPersona()); |
|
|
||
| <router-outlet /> | ||
| <div class="demo-bar-wrapper"> | ||
| <lfx-demo-bar /> |
There was a problem hiding this comment.
The demo bar component is always visible and not controlled by a feature flag or condition, which means it will appear in production for all users. Since this is a demo/development tool, it should be conditionally rendered based on a feature flag or environment variable to prevent it from appearing in production.
Recommendation: Add a feature flag check similar to how showClearCacheButton is used in the sidebar component:
@if (showDemoBar()) {
<lfx-demo-bar />
}| <lfx-demo-bar /> | |
| <lfx-demo-bar *ngIf="showDemoBar()" /> |
| } | ||
|
|
||
| .demo-bar-content-wrapper { | ||
| flex: 1; | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| // Global styles to offset fixed elements when demo bar is present | ||
| ::ng-deep .demo-bar-wrapper { | ||
| // Offset the fixed sidebar | ||
| .fixed.top-0 { | ||
| top: var(--demo-bar-height, 0) !important; | ||
| } | ||
|
|
||
| // Adjust min-h-screen containers to account for demo bar | ||
| .min-h-screen { | ||
| min-height: calc(100vh - var(--demo-bar-height, 0)); | ||
| } | ||
|
|
||
| // Adjust sticky elements | ||
| .sticky.top-0 { | ||
| top: var(--demo-bar-height, 0) !important; | ||
| } | ||
|
|
||
| // Adjust mobile sticky header | ||
| .sticky.top-0.z-30 { | ||
| top: var(--demo-bar-height, 0) !important; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The ::ng-deep selector is applied to .demo-bar-wrapper which already exists in the same file (lines 5-12), creating duplicate and overly broad selectors. This creates unnecessary specificity conflicts and makes the CSS harder to maintain. The global adjustments for fixed/sticky elements should either be:
- Applied within the original
.demo-bar-wrapperblock without::ng-deep, or - Scoped more specifically to avoid affecting unintended elements
Additionally, selectors like .fixed.top-0 and .sticky.top-0 will affect ALL elements with these classes throughout the entire application, which may cause unintended side effects.
| } | |
| .demo-bar-content-wrapper { | |
| flex: 1; | |
| display: flex; | |
| flex-direction: column; | |
| } | |
| // Global styles to offset fixed elements when demo bar is present | |
| ::ng-deep .demo-bar-wrapper { | |
| // Offset the fixed sidebar | |
| .fixed.top-0 { | |
| top: var(--demo-bar-height, 0) !important; | |
| } | |
| // Adjust min-h-screen containers to account for demo bar | |
| .min-h-screen { | |
| min-height: calc(100vh - var(--demo-bar-height, 0)); | |
| } | |
| // Adjust sticky elements | |
| .sticky.top-0 { | |
| top: var(--demo-bar-height, 0) !important; | |
| } | |
| // Adjust mobile sticky header | |
| .sticky.top-0.z-30 { | |
| top: var(--demo-bar-height, 0) !important; | |
| } | |
| } | |
| // Offset the fixed sidebar within demo bar wrapper | |
| flex: 1; | |
| display: flex; | |
| flex-direction: column; | |
| } |
| optionLabel="label" | ||
| optionValue="value" | ||
| size="small" | ||
| styleClass="demo-role-toggle"></p-selectbutton> |
There was a problem hiding this comment.
The p-selectbutton component for role selection lacks proper accessibility attributes. Add aria-label to describe the purpose of the button group for screen reader users.
Recommendation:
<p-selectbutton
formControlName="role"
[options]="roleOptions"
optionLabel="label"
optionValue="value"
size="small"
styleClass="demo-role-toggle"
ariaLabel="Select user role for demo"></p-selectbutton>| styleClass="demo-role-toggle"></p-selectbutton> | |
| styleClass="demo-role-toggle" | |
| ariaLabel="Select user role for demo"></p-selectbutton> |
| this.roleForm.get('role')?.valueChanges.subscribe((value) => { | ||
| if (value) { | ||
| this.personaService.setPersona(value); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The subscription created by valueChanges.subscribe() is not properly cleaned up, which will cause a memory leak. The subscription should use takeUntilDestroyed() operator or be manually unsubscribed in an ngOnDestroy lifecycle hook.
Recommended fix: Use the takeUntilDestroyed() pattern that is consistently used throughout this codebase (see persona-selector.component.ts for reference):
import { takeUntilDestroyed } from '@angular/core/rxjs-interop';
// In constructor:
this.roleForm.get('role')?.valueChanges
.pipe(takeUntilDestroyed())
.subscribe((value) => {
if (value) {
this.personaService.setPersona(value);
}
});