feat(lfx): scaffold angular 20 ssr app with express server#338
Open
feat(lfx): scaffold angular 20 ssr app with express server#338
Conversation
- Angular 20 SSR with stable zoneless change detection - Tailwind CSS v4 with @theme design tokens (no PrimeNG) - Simplified Express backend: LfxService (flat fetch client) - Single ApiError class with factory methods - Centralized error logging via error-handler middleware - Auth0 selective auth middleware (express-openid-connect) - Pino structured logging with logger service - ESLint flat config + Prettier with organize-imports and tailwindcss plugins - 7 Claude skills for guided development workflow - PLAN.md with 7-phase migration roadmap LFXV2-1316 Signed-off-by: Asitha de Silva <asithade@gmail.com>
github-actions
bot
requested review from
andrest50,
dealako,
jordane and
mauriciozanettisalomao
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Angular 20 SSR application at Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser as Client
participant Express as Server
participant OIDC as OIDC Provider
participant LFXAPI as Upstream API
participant SSR as Angular SSR Engine
User->>Browser: Navigate / request page
Browser->>Express: HTTP GET /
Express->>Express: classifyRoute(path)
alt route requires auth
Express->>OIDC: check session / fetch user
OIDC-->>Express: session / token
Express->>Express: extract/refresh bearer token
end
Express->>Express: makeAuthDecision(...)
alt decision = allow (SSR)
Express->>SSR: render AppComponent with auth context
SSR-->>Express: HTML
Express-->>Browser: 200 HTML
else decision = api proxy
Express->>LFXAPI: lfxService request (uses req.bearerToken)
LFXAPI-->>Express: JSON or error
Express-->>Browser: JSON or ApiError response
else decision = redirect/deny
Express-->>Browser: 302 /login or 401
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-license-compliance[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
- Remove old skills (develop, preflight, setup) - Update CLAUDE.md to reference both apps/lfx and apps/lfx-one - Update skill-guidance.md to remove lfx-one skill references - Add ecosystem.config.js for PM2 production config - Mark Phase 1 and Phase 2 as complete in PLAN.md - Update architecture decisions to reflect simplified backend LFXV2-1316 Signed-off-by: Asitha de Silva <asithade@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Scaffolds a new apps/lfx Angular SSR application and its supporting Express server/middleware, plus repo-wide formatting/tooling updates and Claude skill docs intended to guide ongoing development.
Changes:
- Adds the
apps/lfxAngular 20 SSR app scaffold (Angular config, TS configs, Tailwind v4 tokens, basic root component). - Introduces an Express SSR server with Auth0/OIDC middleware, centralized error handling, and Pino-based logging utilities/services.
- Adds/updates formatting and workflow support (Prettier plugins, ESLint flat config, PLAN/skills docs, lockfile updates).
Reviewed changes
Copilot reviewed 38 out of 41 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Updates lockfile for new apps/lfx dependencies (Angular SSR/build tooling, Tailwind, Prettier plugins, etc.). |
| package.json | Adds Prettier plugins at repo root. |
| apps/lfx/tsconfig.json | Base TS config for the new Angular app, including @lfx-one/shared path aliases. |
| apps/lfx/tsconfig.app.json | App TS build config including server entrypoints. |
| apps/lfx/src/types/express.d.ts | Augments Express.Request with bearerToken. |
| apps/lfx/src/styles.css | Tailwind v4 import and initial @theme design tokens. |
| apps/lfx/src/server/services/logger.service.ts | Adds request/infrastructure logging service with operation tracking/deduping. |
| apps/lfx/src/server/services/lfx.service.ts | Adds upstream client service using native fetch and ApiError factories. |
| apps/lfx/src/server/server.ts | Express SSR server wiring: static, compression, Pino, OIDC, auth/error middleware, SSR handler. |
| apps/lfx/src/server/middleware/error-handler.middleware.ts | Centralized API error logging/response middleware. |
| apps/lfx/src/server/middleware/auth.middleware.ts | Selective auth middleware with token extraction/refresh behavior. |
| apps/lfx/src/server/helpers/url-validation.ts | URL and cookie validation helpers for redirect/cookie allowlisting. |
| apps/lfx/src/server/helpers/server-logger.ts | Base Pino logger configuration + serializers/redaction. |
| apps/lfx/src/server/helpers/error-serializer.ts | Custom error serializer for Pino. |
| apps/lfx/src/server/helpers/api-error.ts | Consolidated ApiError with factory helpers and response shaping. |
| apps/lfx/src/main.ts | Browser bootstrap entrypoint. |
| apps/lfx/src/main.server.ts | Server bootstrap entrypoint for SSR. |
| apps/lfx/src/index.html | Base HTML shell and font includes. |
| apps/lfx/src/app/app.routes.ts | Empty route config placeholder. |
| apps/lfx/src/app/app.routes.server.ts | Server-side wildcard route config. |
| apps/lfx/src/app/app.config.ts | App providers: zoneless, hydration, router, HttpClient (fetch). |
| apps/lfx/src/app/app.config.server.ts | Server rendering providers and route wiring. |
| apps/lfx/src/app/app.component.ts | Minimal root component with router outlet. |
| apps/lfx/src/app/app.component.html | Root template with <router-outlet />. |
| apps/lfx/src/app/app.component.css | Root component stylesheet scaffold. |
| apps/lfx/public/favicon.ico | Adds favicon asset. |
| apps/lfx/package.json | App-level dependencies/scripts for build/lint/format. |
| apps/lfx/eslint.config.js | ESLint flat config for Angular + TS. |
| apps/lfx/angular.json | Angular CLI project config for SSR build/serve/lint. |
| apps/lfx/.prettierrc.js | App-level Prettier config including plugins and overrides. |
| apps/lfx/.prettierignore | Prettier ignore list for app artifacts and lockfile. |
| apps/lfx/.env.example | Example env var template for OIDC, upstream, logging. |
| PLAN.md | Migration plan/roadmap and conventions for apps/lfx. |
| .claude/skills/lfx-ui-builder/SKILL.md | Skill documentation for generating Angular feature code in apps/lfx. |
| .claude/skills/lfx-setup/SKILL.md | Skill documentation for setting up the apps/lfx dev environment. |
| .claude/skills/lfx-research/SKILL.md | Skill documentation for read-only research/exploration. |
| .claude/skills/lfx-preflight/SKILL.md | Skill documentation for pre-PR validation and protected-file checks. |
| .claude/skills/lfx-design/SKILL.md | Skill documentation for Tailwind v4 design-system components. |
| .claude/skills/lfx-coordinator/SKILL.md | Skill documentation for orchestrating multi-step work. |
| .claude/skills/lfx-backend-builder/SKILL.md | Skill documentation for generating Express proxy/backend code. |
| .claude/rules/skill-guidance.md | Updates skill guidance to distinguish lfx vs lfx-one workflows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (20)
apps/lfx/src/styles.css-6-6 (1)
6-6:⚠️ Potential issue | 🟠 MajorConfigure Stylelint to allow Tailwind v4 at-rules (
@theme,@utility,@variant).The
@themedirective on line 6 uses Tailwind v4 syntax, which the defaultat-rule-no-unknownrule fromstylelint-config-standard-scsswill flag as an unknown at-rule. Update.stylelintrc.jsonto ignore Tailwind at-rules:Suggested configuration
{ "extends": ["stylelint-config-standard-scss"], "rules": { "at-rule-no-unknown": [true, { "ignoreAtRules": ["theme", "utility", "variant", "custom-variant"] }], ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/styles.css` at line 6, Stylelint is flagging Tailwind v4 at-rules like `@theme` in styles.css because the at-rule-no-unknown rule from stylelint-config-standard-scss doesn't recognize them; update your Stylelint config (e.g., .stylelintrc.json) to adjust the "at-rule-no-unknown" rule to ignore the Tailwind at-rules by adding "theme", "utility", "variant" (and "custom-variant" if used) to its ignoreAtRules array so those directives are allowed.apps/lfx/src/server/helpers/error-serializer.ts-29-34 (1)
29-34:⚠️ Potential issue | 🟠 MajorDon’t mirror arbitrary error properties into logs.
This copies every enumerable field on the error object into log output.
ApiError.upstream()already attachesdetails.errorBody, so upstream payloads can leak into logs here and bypass the current top-level redaction rules. Prefer a small allowlist, or explicitly sanitizedetailsbefore attaching it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/helpers/error-serializer.ts` around lines 29 - 34, The current logic in error-serializer.ts shallow-copies all enumerable properties from err into serialized (Object.keys(err).forEach(...) block), which risks leaking upstream payloads like ApiError.upstream()'s details.errorBody into logs; change this to use a small allowlist of safe properties (e.g., only copy known safe keys such as 'code', 'status', 'details' after explicit sanitization) or explicitly sanitize details before attaching it (sanitize or omit details.errorBody), updating the loop around Object.keys(err) to only include those allowlisted keys and/or run a sanitizer on err.details before assigning to serialized.apps/lfx/src/server/helpers/url-validation.ts-70-73 (1)
70-73:⚠️ Potential issue | 🟠 MajorThe development Auth0 cookie cannot pass this validator as written.
DOMAIN_ALLOWLIST.developmentallowsauth0.InaRygxwVLWCKf6k6rmOc25mTPvvBrDy.is.authenticated, butextractDomainFromCookie()only accepts client IDjStGXyf3nwTswv8goh6FcbU4EaWUZBNP. That mismatch makes the listed development cookie unreachable. Drive both checks from the same config source so they cannot drift.Also applies to: 115-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/helpers/url-validation.ts` around lines 70 - 73, DOMAIN_ALLOWLIST.development contains a specific Auth0 cookie name that won't match extractDomainFromCookie's current client-id check, causing valid dev cookies to be rejected; refactor so both DOMAIN_ALLOWLIST (used for domain checks) and extractDomainFromCookie (the function that validates cookie client IDs) pull their valid client IDs/domains from the same single source of truth (e.g., a shared DEV_AUTH0_CONFIG or ENV-driven array) and replace any hardcoded literals in DOMAIN_ALLOWLIST and extractDomainFromCookie with references to that shared config; update both places (including the similar check around extractDomainFromCookie usage at lines ~115-123) to use the unified list so they cannot drift.apps/lfx/src/server/helpers/api-error.ts-39-59 (1)
39-59:⚠️ Potential issue | 🟠 MajorKeep transport diagnostics out of
toResponse().
timeout(),upstream(), andnetworkError()all stash internal metadata indetails, andtoResponse()echoesdetailsback to the client. That leaks backend paths and, forupstream(), potentially the raw upstream error body. Split internal log metadata from public response details, or only exposedetailsfor safe 4xx validation errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/helpers/api-error.ts` around lines 39 - 59, The toResponse() method currently echoes internal transport diagnostics stored by timeout(), upstream(), and networkError() via the details field, leaking backend paths and upstream bodies; update the ApiError construction and toResponse() so internal metadata (e.g., path, errorBody) is kept separate from public-facing details — either add an internalDetails/private field or ensure toResponse() only includes a sanitized details object (for example allow only safe validation info and only for 4xx errors) and explicitly exclude path and errorBody fields; adjust timeout(), upstream(), and networkError() to populate the internal metadata container or to pass only safe public details so callers like timeout, upstream, networkError and toResponse are consistent.apps/lfx/src/server/helpers/url-validation.ts-50-58 (1)
50-58:⚠️ Potential issue | 🟠 MajorFail closed when no allowlist is supplied.
When
allowedDomainsis omitted, this helper accepts any absolutehttp/httpsURL. For a utility that claims open-redirect protection, that means one missed call site reopens the vulnerability. Default to relative URLs or same-originPCC_BASE_URLwhen no allowlist is provided.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/helpers/url-validation.ts` around lines 50 - 58, The current logic treats a missing or empty allowedDomains as allowing any absolute http/https URL; change it to "fail closed" by permitting only relative URLs or absolute URLs that match PCC_BASE_URL when allowedDomains is not supplied. In the url-validation helper (references: allowedDomains, parsedUrl, PCC_BASE_URL), add a branch for when !allowedDomains || allowedDomains.length === 0 that: 1) allows relative URLs (i.e., parsedUrl.origin is empty/null or indicates a relative parse) and 2) for absolute URLs require parsedUrl.origin.toLowerCase() === new URL(PCC_BASE_URL).origin.toLowerCase(), otherwise return null; keep the existing allowlist behavior when allowedDomains is provided.apps/lfx/src/server/helpers/url-validation.ts-93-97 (1)
93-97:⚠️ Potential issue | 🟠 MajorParse the
Domainattribute case-insensitively.The gate only looks for lowercase
domain=, but real cookies are commonly emitted asDomain=.... Those cookies never reach the latertoLowerCase()check, so valid domains are rejected.🛠️ Proposed fix
- if (cookie.includes(';') && cookie.includes('=') && cookie.includes('domain=')) { + if (cookie.includes(';') && cookie.includes('=') && /(?:^|;)\s*domain=/i.test(cookie)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/helpers/url-validation.ts` around lines 93 - 97, The cookie Domain parsing is case-sensitive and only matches 'domain='; update the check that finds domainPart to be case-insensitive by normalizing each cookieParts element (e.g., trim and toLowerCase()) before testing startsWith('domain=') or use a case-insensitive regex to locate the Domain attribute; ensure you still extract the original value after matching so later toLowerCase() checks work correctly (refer to the variables cookie, cookieParts, domainPart).apps/lfx/src/server/services/lfx.service.ts-50-70 (1)
50-70: 🛠️ Refactor suggestion | 🟠 MajorExpose an explicit M2M auth path for public-endpoint calls.
This client only knows how to forward
req.bearerToken. That leaves/public/apicallers with no way to make the required server-to-server call when there is no user token, and it also lets optional/public flows piggyback end-user credentials when one happens to exist. Add an auth mode or token provider so public endpoints can request an M2M token explicitly instead of hard-wiring user-token forwarding. As per coding guidelines, "Server-side API calls from public endpoints must use M2M tokens for authentication".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/services/lfx.service.ts` around lines 50 - 70, The request<T> method currently always forwards req.bearerToken; update it to accept an explicit auth mode or token provider so callers can request an M2M token for public endpoints (e.g., add an optional parameter authMode: 'user' | 'm2m' or tokenProvider: () => Promise<string> to request<T>), and when authMode === 'm2m' call a new or existing M2M helper (getM2MToken or injected tokenProvider) to set Authorization instead of using req.bearerToken; ensure callers to public routes pass authMode:'m2m' and private/user routes continue to use the existing behavior, keep header construction in request<T> (referencing request<T>, DEFAULT_QUERY_PARAMS, buildUrl, and this.userAgent) and do not leak end-user tokens into public/M2M flows.apps/lfx/src/server/middleware/auth.middleware.ts-23-25 (1)
23-25:⚠️ Potential issue | 🟠 MajorUse the public meeting path here, not
/meetings/.With
/meetings/in the route table, real/meeting...URLs fall through to the final'/'rule and become protected instead of optional/public. Keep this pattern aligned with the matching silent-login mount inapps/lfx/src/server/server.ts. As per coding guidelines, "Public routes at /meeting and /public/api endpoints bypass authentication, while protected routes require authentication".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/middleware/auth.middleware.ts` around lines 23 - 25, The route entry currently uses the plural pattern '/meetings/' which causes real '/meeting...' requests to not match and fall through to the protected '/' rule; update the route table entry (the object where pattern: '/meetings/' is set) to use the public meeting path '/meeting' (matching the silent-login mount in server.ts) and keep type: 'ssr' and auth: 'optional' so /meeting and its subpaths bypass authentication as intended.apps/lfx/src/server/services/logger.service.ts-410-419 (1)
410-419:⚠️ Potential issue | 🟠 Major
sanitize()doesn't protect the current log output.None of the logging methods call this helper before attaching metadata under
data, so tokens, emails, and similar fields can still leak. The top-level redaction inapps/lfx/src/server/helpers/server-logger.tsonly covers a small fixed set of fields; nested metadata needs to be sanitized before it is logged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/services/logger.service.ts` around lines 410 - 419, The sanitize(metadata: Record<string, unknown>) method is never applied before metadata is attached to logs, so sensitive nested fields can leak; update all logging entry points in the LoggerService (the public logging methods in logger.service.ts that attach metadata under the data field) to call this.sanitize(...) on any metadata/object before adding it to the log payload, and ensure nested objects passed into server-logger.ts are sanitized before being handed off (replace direct metadata usage with the sanitized result); keep using the existing SENSITIVE_FIELDS and return value shape to avoid changing downstream expectations.apps/lfx/src/server/middleware/auth.middleware.ts-45-52 (1)
45-52:⚠️ Potential issue | 🟠 MajorMatch auth routes on path boundaries, not raw prefixes.
startsWith('/health')also matches/healthcheck, andstartsWith('/api')matches/api-docs. That can accidentally bypass auth or enforce it on the wrong route. Match exact paths or segment boundaries instead.💡 Suggested fix
function classifyRoute(path: string, config: AuthConfig): RouteAuthConfig { for (const routeConfig of config.routes) { if (typeof routeConfig.pattern === 'string') { - if (path.startsWith(routeConfig.pattern)) { + const pattern = routeConfig.pattern.replace(/\/$/, '') || '/'; + const normalizedPath = path.replace(/\/$/, '') || '/'; + if (pattern === '/' || normalizedPath === pattern || normalizedPath.startsWith(`${pattern}/`)) { return routeConfig; } } else if (routeConfig.pattern.test(path)) { return routeConfig; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/middleware/auth.middleware.ts` around lines 45 - 52, The string-pattern branch in classifyRoute incorrectly uses startsWith and can match prefixes across segment boundaries; update classifyRoute so when routeConfig.pattern is a string you return the route only if path === routeConfig.pattern or the path continues with a segment boundary (e.g., path.startsWith(routeConfig.pattern + '/') or routeConfig.pattern endsWith('/') handling), ensuring exact-path matches and proper segment-boundary matches instead of raw prefix matches; modify the logic inside classifyRoute (the branch that checks typeof routeConfig.pattern === 'string') to implement this exact-or-boundary check so routes like '/health' don't match '/healthcheck' and '/api' doesn't match '/api-docs'.apps/lfx/src/server/server.ts-166-177 (1)
166-177:⚠️ Potential issue | 🟠 MajorKeep the token-claims fallback instead of forcing logout here.
Line 164 already seeds
authContext.userfromreq.oidc.user. Logging the user out whenfetchUserInfo()fails turns a transient IdP/userinfo outage into a forced sign-out for every SSR page, even though you already have basic claims available.💡 Suggested fix
} catch (error) { // If userinfo fetch fails, fall back to basic user info from token logger.warning(req, 'ssr_user_info', 'Failed to fetch user info, using basic user data', { err: error, path: req.path, }); - - res.oidc.logout(); - return; + // Keep the claims from req.oidc.user and continue rendering. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/server.ts` around lines 166 - 177, The catch block for req.oidc.fetchUserInfo() currently logs a warning then calls res.oidc.logout() and returns, forcing a sign-out even though authContext.user was already seeded from req.oidc.user; instead, remove the forced logout/return and allow the request to continue using the existing token claims. Update the catch handler (surrounding req.oidc.fetchUserInfo(), authContext.user, logger.warning, res.oidc.logout()) to only log the error and continue, ensuring authContext.user remains populated from req.oidc.user as the fallback.apps/lfx/src/server/middleware/auth.middleware.ts-292-296 (1)
292-296:⚠️ Potential issue | 🟠 MajorUse the computed redirect target instead of bypassing it here.
makeAuthDecision()already builds a/login?returnTo=...redirect, but this branch ignoresdecision.redirectUrland jumps straight intores.oidc.login({ returnTo: req.originalUrl }). That means thereturnTovalidation inapps/lfx/src/server/server.tsLines 121-139 never runs.💡 Suggested fix
case 'redirect': if (decision.redirectUrl) { - // Use OIDC login method which handles the redirect properly - res.oidc.login({ returnTo: req.originalUrl }); + res.redirect(decision.redirectUrl); } else { res.redirect('/'); } break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/middleware/auth.middleware.ts` around lines 292 - 296, The redirect branch currently bypasses the computed redirect target by calling res.oidc.login({ returnTo: req.originalUrl }); instead, use the precomputed decision.redirectUrl from makeAuthDecision() so the /login?returnTo=... flow (and its returnTo validation in server.ts) runs; replace the res.oidc.login call in the 'redirect' case with a redirect to decision.redirectUrl (or invoke res.oidc.login only with that computed target) so server-side validation in server.ts Lines 121-139 is exercised..claude/skills/lfx-preflight/SKILL.md-85-97 (1)
85-97:⚠️ Potential issue | 🟠 MajorUpdate the protected-file patterns for the new app.
This list still guards
microservice-proxy.service.ts, even though the PR moves the app toLfxService, and*/package.jsondoes not cover nested manifests likeapps/lfx/package.json. Preflight will miss some of the exact files this check is meant to surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/lfx-preflight/SKILL.md around lines 85 - 97, The protected-file patterns list is outdated: it still references microservice-proxy.service.ts and a generic */package.json which misses nested manifests; update the patterns in SKILL.md so they reference the new LfxService file names (replace microservice-proxy.service.ts with the moved/renamed LfxService file under apps/lfx/src/server/services, and include apps/lfx/package.json and any other nested package.json paths), and ensure entries point to apps/lfx/src/server/server.ts, server-logger.ts, middleware/*, services/logger.service.ts, app.routes.ts, turbo.json, CLAUDE.md, PLAN.md, check-headers.sh, and package.json variants so the preflight check will catch the moved/renamed files.PLAN.md-78-85 (1)
78-85:⚠️ Potential issue | 🟠 MajorAlign the plan with the backend that was actually scaffolded.
This plan still calls for
MicroserviceProxyServiceand a multi-filesrc/server/errors/hierarchy, but the PR objective describes a singleLfxServiceand oneApiError. Because this document is labeled the source of truth, those legacy references will misdirect follow-on phases.Also applies to: 149-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PLAN.md` around lines 78 - 85, The plan still references a legacy MicroserviceProxyService and a multi-file src/server/errors/ hierarchy; update the document to match the actual scaffold by replacing any mention of MicroserviceProxyService with the single LfxService and replacing the multi-file errors description with a single ApiError (single file) implementation, and ensure any architecture bullets, examples, and references (e.g., under "Server Architecture" and the lines around 149-153) reflect LfxService and ApiError so future work is not misdirected.PLAN.md-90-93 (1)
90-93:⚠️ Potential issue | 🟠 MajorRemove the workstation-specific paths from the setup examples.
Both
ng newsnippets assume/Users/asithadesilva/Sites/lfx-v2-ui, so contributors and CI cannot follow the plan verbatim. Use repo-relative instructions or derive the workspace root dynamically.Also applies to: 134-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PLAN.md` around lines 90 - 93, Replace the workstation-specific absolute path used before the ng new command with a repo-relative approach: remove "/Users/asithadesilva/Sites/lfx-v2-ui" and instruct contributors to run the ng new command from the repository workspace root (or show how to compute the repo root dynamically), and update both ng new snippets (the bash snippet that runs ng new lfx --ssr --style css --zoneless --prefix lfx --skip-tests and the other occurrence) so examples are portable for contributors and CI..claude/rules/skill-guidance.md-59-68 (1)
59-68:⚠️ Potential issue | 🟠 MajorResolve the
/lfx-researchworkflow split.This diagram says
/lfx-coordinatordelegates to/lfx-research, but.claude/skills/lfx-coordinator/SKILL.mdsays research must happen inline and not via Skill delegation. Keeping both instructions will make the agent choose between incompatible flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/rules/skill-guidance.md around lines 59 - 68, The diagram currently shows /lfx-coordinator delegating to /lfx-research, which conflicts with the guidance in SKILL.md that research must be done inline; update the skill relationships to remove /lfx-research from the delegation list (leave /lfx-coordinator delegating only to /lfx-backend-builder, /lfx-ui-builder, /lfx-design) and add a short clarifying note next to /lfx-coordinator stating “research must be performed inline, do not delegate to /lfx-research” so the diagram and the /lfx-coordinator SKILL.md are consistent..claude/skills/lfx-coordinator/SKILL.md-44-50 (1)
44-50:⚠️ Potential issue | 🟠 MajorAbort the workflow when
apps/lfxis missing.
|| echolets the coordinator keep running against the wrong checkout after the repository check fails. Exit here so the later GitHub and filesystem commands do not act on an arbitrary repo.🔧 Proposed fix
-# Confirm we're in the right repo -[ -f apps/lfx/angular.json ] || echo "ERROR: apps/lfx not found" +# Confirm we're in the right repo +[ -f apps/lfx/angular.json ] || { echo "ERROR: apps/lfx not found"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/lfx-coordinator/SKILL.md around lines 44 - 50, The repo-check currently uses "[ -f apps/lfx/angular.json ] || echo "ERROR: apps/lfx not found"" which only prints an error but lets the script continue; update that check so it prints the error and then exits with a non-zero status (e.g., replace the trailing "|| echo ..." with a grouped command that echoes the message and exits 1) so the workflow aborts when apps/lfx is missing; ensure the check immediately precedes the branch/checkout logic (the line currently followed by "git branch --show-current") so no further commands run on the wrong repo..claude/skills/lfx-coordinator/SKILL.md-138-150 (1)
138-150:⚠️ Potential issue | 🟠 MajorDon’t bake one developer’s repo path into the skill.
The required-args example and validation step both hardcode
/Users/asithadesilva/Sites/lfx-v2-ui, which makes the workflow unusable on every other workstation and in CI. Derive the repo root at runtime and pass that through instead.🔧 Proposed fix
-| **Absolute repo path** (`/Users/asithadesilva/Sites/lfx-v2-ui`) | Skills don't inherit your working directory | +| **Absolute repo path** (derive with `git rev-parse --show-toplevel`) | Skills don't inherit your working directory | … -cd /Users/asithadesilva/Sites/lfx-v2-ui +cd "$(git rev-parse --show-toplevel)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/lfx-coordinator/SKILL.md around lines 138 - 150, The SKILL.md currently hardcodes the absolute path `/Users/asithadesilva/Sites/lfx-v2-ui` in the "Required" example and under "Step 6: Validate"; replace those occurrences with a runtime-derived repo root and use that variable in the validation commands instead of baking one developer’s path. Update the "Required" example and the "Step 6: Validate" instructions to (a) show how to derive the repo root at runtime (e.g., using a git or environment-based approach) and set it to a REPO_ROOT variable, and (b) run the validation steps by changing directory to that REPO_ROOT and passing it through the commands (so references to the string `/Users/asithadesilva/Sites/lfx-v2-ui` are removed and replaced with the runtime variable).apps/lfx/eslint.config.js-38-38 (1)
38-38:⚠️ Potential issue | 🟠 MajorEnable TypeScript strict type guardrails and suppress legitimate exceptions locally.
The rules
@typescript-eslint/no-explicit-anyand@typescript-eslint/consistent-type-definitionsare disabled globally, removing type enforcement across the entire app. While server-sidecatch (error: any)blocks are legitimate and necessary, they represent narrow exceptions that should be suppressed witheslint-disable-next-linecomments at those specific sites. The codebase already follows this pattern for other rules (@typescript-eslint/no-require-imports,@typescript-eslint/no-empty-function, etc.), establishing a precedent for local suppression of exceptions.🔧 Proposed fix
- '@typescript-eslint/no-explicit-any': 'off', + '@typescript-eslint/no-explicit-any': 'error', … - '@typescript-eslint/consistent-type-definitions': 'off', + '@typescript-eslint/consistent-type-definitions': ['error', 'interface'],Then add
// eslint-disable-next-line@typescript-eslint/no-explicit-any`` above each legitimatecatch (error: any)site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/eslint.config.js` at line 38, Re-enable the TypeScript lint rules by changing '@typescript-eslint/no-explicit-any' and '@typescript-eslint/consistent-type-definitions' from 'off' to a stricter setting (e.g., 'error' or 'warn') in the ESLint config, and then locally suppress legitimate exceptions by adding "// eslint-disable-next-line `@typescript-eslint/no-explicit-any`" immediately above each valid catch clause that uses catch (error: any); keep the existing local-disable pattern used for other rules (e.g., `@typescript-eslint/no-require-imports`) so only those narrow sites allow `any` while the rest of the codebase benefits from the stricter guardrails.apps/lfx/eslint.config.js-29-32 (1)
29-32:⚠️ Potential issue | 🟠 MajorSet
tsconfigRootDir: __dirnameto resolve the TypeScript project correctly.This config is nested under
apps/lfx/, so the relativeproject: './tsconfig.app.json'resolves to the process working directory (typically repo root in monorepo setups) rather than to the config file's directory. WithouttsconfigRootDir, ESLint cannot locate the project file when invoked from the repository root, breaking typed linting.🔧 Proposed fix
languageOptions: { parserOptions: { + tsconfigRootDir: __dirname, project: './tsconfig.app.json', }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/eslint.config.js` around lines 29 - 32, The ESLint TypeScript project path is resolved relative to the process CWD, causing typed linting to fail in monorepos; update the languageOptions.parserOptions in the ESLint config by adding tsconfigRootDir set to __dirname alongside the existing project: './tsconfig.app.json' so the parser resolves the tsconfig relative to apps/lfx; modify the parserOptions object (the same block containing project) to include tsconfigRootDir: __dirname.
🟡 Minor comments (2)
apps/lfx/.prettierrc.js-17-18 (1)
17-18:⚠️ Potential issue | 🟡 MinorUse a recursive glob for the Angular parser override.
Line 18 uses
files: '*.html', which only matches HTML files directly inapps/lfx/. The actual HTML templates (src/app/app.component.html,src/index.html) are nested undersrc/, so they won't be formatted with the Angular parser. Change to**/*.html:Proposed fix
- { files: '*.html', options: { parser: 'angular' } }, + { files: '**/*.html', options: { parser: 'angular' } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/.prettierrc.js` around lines 17 - 18, The HTML override in the Prettier config uses a non-recursive glob ("files: '*.html'") so nested templates like src/app/app.component.html are not matched; update the override entry that currently reads files: '*.html' to use a recursive glob files: '**/*.html' so the Angular parser override (parser: 'angular') applies to HTML files in all subdirectories.apps/lfx/src/server/services/lfx.service.ts-78-80 (1)
78-80:⚠️ Potential issue | 🟡 MinorDon't drop falsy JSON payloads.
0,false,'', andnullare valid request bodies, but this truthiness check omits them entirely. Gate ondata !== undefinedinstead.💡 Suggested fix
- if (data && ['POST', 'PUT', 'PATCH'].includes(method)) { + if (data !== undefined && ['POST', 'PUT', 'PATCH'].includes(method)) { init.body = JSON.stringify(data); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/services/lfx.service.ts` around lines 78 - 80, The code currently only sets init.body when data is truthy, which drops valid falsy payloads like 0, false, '', and null; change the condition in the block that assigns init.body (the variables/methods involved are init, data, and method and the POST/PUT/PATCH check) to gate on data !== undefined instead of truthiness so any provided value except undefined is JSON.stringified and assigned to init.body for POST/PUT/PATCH requests.
🧹 Nitpick comments (4)
apps/lfx/src/app/app.routes.ts (1)
6-6: Add a brief JSDoc for the exported route config.This keeps the public route module self-documenting as the file evolves.
As per coding guidelines "Add JSDoc comments for public functions and exported modules."📝 Suggested documentation
+/** Client-side route configuration for the LFX app. */ export const routes: Routes = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/app.routes.ts` at line 6, Add a concise JSDoc comment above the exported symbol routes (type Routes) that describes what the exported route configuration represents, its expected contents (array of route definitions), and any usage notes (e.g., consumed by the router setup). Update the comment for the export const routes: Routes declaration to be a one- or two-line JSDoc block that documents its purpose and shape so the public module is self-documenting.apps/lfx/src/app/app.routes.server.ts (1)
6-11: Add JSDoc for exported SSR route config.A short docstring here improves discoverability of SSR routing intent.
As per coding guidelines "Add JSDoc comments for public functions and exported modules."📝 Suggested documentation
+/** Server-side route configuration for Angular SSR rendering. */ export const serverRoutes: ServerRoute[] = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/app.routes.server.ts` around lines 6 - 11, Add a JSDoc block above the exported constant `serverRoutes` in app.routes.server.ts that documents this public SSR route configuration: state that `serverRoutes` is a catch-all ServerRoute array used for server-side rendering (uses `RenderMode.Server`), describe its purpose (fallback route rendering on the server), and note the wildcard `path: '**'` behavior so consumers know this is the default SSR route for the app.apps/lfx/.env.example (1)
2-6: Reorder Auth0 keys to clear dotenv-linter warning.Line 5 currently triggers
UnorderedKey; sorting this block avoids persistent lint noise.♻️ Suggested key reorder
- PCC_AUTH0_CLIENT_ID= - PCC_AUTH0_CLIENT_SECRET= - PCC_AUTH0_ISSUER_BASE_URL= PCC_AUTH0_AUDIENCE= + PCC_AUTH0_CLIENT_ID= + PCC_AUTH0_CLIENT_SECRET= + PCC_AUTH0_ISSUER_BASE_URL= PCC_AUTH0_SECRET=🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/.env.example` around lines 2 - 6, Reorder the Auth0 env keys in apps/lfx/.env.example to satisfy dotenv-linter's UnorderedKey rule: replace the current block (PCC_AUTH0_CLIENT_ID, PCC_AUTH0_CLIENT_SECRET, PCC_AUTH0_ISSUER_BASE_URL, PCC_AUTH0_AUDIENCE, PCC_AUTH0_SECRET) with the keys sorted alphabetically (PCC_AUTH0_AUDIENCE, PCC_AUTH0_CLIENT_ID, PCC_AUTH0_CLIENT_SECRET, PCC_AUTH0_ISSUER_BASE_URL, PCC_AUTH0_SECRET) so lint warnings for UnorderedKey are resolved.apps/lfx/src/main.server.ts (1)
9-11: Document the exported SSR bootstrap.A short JSDoc on this default export would make the server entry contract clearer for future scaffolding and maintenance.
📝 Proposed fix
+/** + * Bootstraps the server-rendered Angular application for a request-scoped rendering context. + */ const bootstrap = (context: BootstrapContext) => bootstrapApplication(AppComponent, config, context);As per coding guidelines,
Add JSDoc comments for public functions and exported modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/main.server.ts` around lines 9 - 11, Add a concise JSDoc comment above the default export to document the server-side rendering bootstrap function: describe the exported bootstrap function (bootstrap), its purpose (initializes the SSR app using bootstrapApplication with AppComponent), the expected parameter type (context: BootstrapContext) and its return/behavior. Place the comment immediately above the line exporting bootstrap in main.server.ts so future maintainers understand the SSR entry contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/lfx-preflight/SKILL.md:
- Around line 39-43: The auto-fix for `.css` files is using TypeScript `//`
comments which are invalid in CSS; update the `.css` auto-fix header to use CSS
block comment syntax (/* ... */) and include the same copyright and SPDX lines
inside the block so the header is valid CSS; locate the `.css` entry in the
auto-fix section and replace the `.ts`-style header with a `/* Copyright The
Linux Foundation and each contributor to LFX.\n SPDX-License-Identifier: MIT
*/` block comment.
In `@apps/lfx/package.json`:
- Around line 27-31: Package.json is missing the runtime dependency "pino" (it's
imported in server-logger.ts) and "pino-pretty" is incorrectly listed as a
devDependency; update package.json to add "pino" to dependencies with a version
compatible with pino-http 11.x (e.g., a caret major that matches pino-http's
required pino), move "pino-pretty" from devDependencies into dependencies, and
normalize version pinning (use caret ^ for "pino-http" and "dotenv" to match the
project's dependency style) so runtime logging libraries are installed in
production.
In `@apps/lfx/src/server/server.ts`:
- Around line 41-43: The file uses ESM (import.meta.url) but incorrectly calls
require('compression'), which crashes at runtime; replace the bare require usage
for the compression variable with a proper ES module import (e.g., use an import
statement for compression) and remove the eslint-disable comment tied to
require; ensure the symbol compression is imported at the top of server.ts so
existing references to compression continue to work and TypeScript types are
resolved (install or reference `@types/compression` if needed).
---
Major comments:
In @.claude/rules/skill-guidance.md:
- Around line 59-68: The diagram currently shows /lfx-coordinator delegating to
/lfx-research, which conflicts with the guidance in SKILL.md that research must
be done inline; update the skill relationships to remove /lfx-research from the
delegation list (leave /lfx-coordinator delegating only to /lfx-backend-builder,
/lfx-ui-builder, /lfx-design) and add a short clarifying note next to
/lfx-coordinator stating “research must be performed inline, do not delegate to
/lfx-research” so the diagram and the /lfx-coordinator SKILL.md are consistent.
In @.claude/skills/lfx-coordinator/SKILL.md:
- Around line 44-50: The repo-check currently uses "[ -f apps/lfx/angular.json ]
|| echo "ERROR: apps/lfx not found"" which only prints an error but lets the
script continue; update that check so it prints the error and then exits with a
non-zero status (e.g., replace the trailing "|| echo ..." with a grouped command
that echoes the message and exits 1) so the workflow aborts when apps/lfx is
missing; ensure the check immediately precedes the branch/checkout logic (the
line currently followed by "git branch --show-current") so no further commands
run on the wrong repo.
- Around line 138-150: The SKILL.md currently hardcodes the absolute path
`/Users/asithadesilva/Sites/lfx-v2-ui` in the "Required" example and under "Step
6: Validate"; replace those occurrences with a runtime-derived repo root and use
that variable in the validation commands instead of baking one developer’s path.
Update the "Required" example and the "Step 6: Validate" instructions to (a)
show how to derive the repo root at runtime (e.g., using a git or
environment-based approach) and set it to a REPO_ROOT variable, and (b) run the
validation steps by changing directory to that REPO_ROOT and passing it through
the commands (so references to the string `/Users/asithadesilva/Sites/lfx-v2-ui`
are removed and replaced with the runtime variable).
In @.claude/skills/lfx-preflight/SKILL.md:
- Around line 85-97: The protected-file patterns list is outdated: it still
references microservice-proxy.service.ts and a generic */package.json which
misses nested manifests; update the patterns in SKILL.md so they reference the
new LfxService file names (replace microservice-proxy.service.ts with the
moved/renamed LfxService file under apps/lfx/src/server/services, and include
apps/lfx/package.json and any other nested package.json paths), and ensure
entries point to apps/lfx/src/server/server.ts, server-logger.ts, middleware/*,
services/logger.service.ts, app.routes.ts, turbo.json, CLAUDE.md, PLAN.md,
check-headers.sh, and package.json variants so the preflight check will catch
the moved/renamed files.
In `@apps/lfx/eslint.config.js`:
- Line 38: Re-enable the TypeScript lint rules by changing
'@typescript-eslint/no-explicit-any' and
'@typescript-eslint/consistent-type-definitions' from 'off' to a stricter
setting (e.g., 'error' or 'warn') in the ESLint config, and then locally
suppress legitimate exceptions by adding "// eslint-disable-next-line
`@typescript-eslint/no-explicit-any`" immediately above each valid catch clause
that uses catch (error: any); keep the existing local-disable pattern used for
other rules (e.g., `@typescript-eslint/no-require-imports`) so only those narrow
sites allow `any` while the rest of the codebase benefits from the stricter
guardrails.
- Around line 29-32: The ESLint TypeScript project path is resolved relative to
the process CWD, causing typed linting to fail in monorepos; update the
languageOptions.parserOptions in the ESLint config by adding tsconfigRootDir set
to __dirname alongside the existing project: './tsconfig.app.json' so the parser
resolves the tsconfig relative to apps/lfx; modify the parserOptions object (the
same block containing project) to include tsconfigRootDir: __dirname.
In `@apps/lfx/src/server/helpers/api-error.ts`:
- Around line 39-59: The toResponse() method currently echoes internal transport
diagnostics stored by timeout(), upstream(), and networkError() via the details
field, leaking backend paths and upstream bodies; update the ApiError
construction and toResponse() so internal metadata (e.g., path, errorBody) is
kept separate from public-facing details — either add an internalDetails/private
field or ensure toResponse() only includes a sanitized details object (for
example allow only safe validation info and only for 4xx errors) and explicitly
exclude path and errorBody fields; adjust timeout(), upstream(), and
networkError() to populate the internal metadata container or to pass only safe
public details so callers like timeout, upstream, networkError and toResponse
are consistent.
In `@apps/lfx/src/server/helpers/error-serializer.ts`:
- Around line 29-34: The current logic in error-serializer.ts shallow-copies all
enumerable properties from err into serialized (Object.keys(err).forEach(...)
block), which risks leaking upstream payloads like ApiError.upstream()'s
details.errorBody into logs; change this to use a small allowlist of safe
properties (e.g., only copy known safe keys such as 'code', 'status', 'details'
after explicit sanitization) or explicitly sanitize details before attaching it
(sanitize or omit details.errorBody), updating the loop around Object.keys(err)
to only include those allowlisted keys and/or run a sanitizer on err.details
before assigning to serialized.
In `@apps/lfx/src/server/helpers/url-validation.ts`:
- Around line 70-73: DOMAIN_ALLOWLIST.development contains a specific Auth0
cookie name that won't match extractDomainFromCookie's current client-id check,
causing valid dev cookies to be rejected; refactor so both DOMAIN_ALLOWLIST
(used for domain checks) and extractDomainFromCookie (the function that
validates cookie client IDs) pull their valid client IDs/domains from the same
single source of truth (e.g., a shared DEV_AUTH0_CONFIG or ENV-driven array) and
replace any hardcoded literals in DOMAIN_ALLOWLIST and extractDomainFromCookie
with references to that shared config; update both places (including the similar
check around extractDomainFromCookie usage at lines ~115-123) to use the unified
list so they cannot drift.
- Around line 50-58: The current logic treats a missing or empty allowedDomains
as allowing any absolute http/https URL; change it to "fail closed" by
permitting only relative URLs or absolute URLs that match PCC_BASE_URL when
allowedDomains is not supplied. In the url-validation helper (references:
allowedDomains, parsedUrl, PCC_BASE_URL), add a branch for when !allowedDomains
|| allowedDomains.length === 0 that: 1) allows relative URLs (i.e.,
parsedUrl.origin is empty/null or indicates a relative parse) and 2) for
absolute URLs require parsedUrl.origin.toLowerCase() === new
URL(PCC_BASE_URL).origin.toLowerCase(), otherwise return null; keep the existing
allowlist behavior when allowedDomains is provided.
- Around line 93-97: The cookie Domain parsing is case-sensitive and only
matches 'domain='; update the check that finds domainPart to be case-insensitive
by normalizing each cookieParts element (e.g., trim and toLowerCase()) before
testing startsWith('domain=') or use a case-insensitive regex to locate the
Domain attribute; ensure you still extract the original value after matching so
later toLowerCase() checks work correctly (refer to the variables cookie,
cookieParts, domainPart).
In `@apps/lfx/src/server/middleware/auth.middleware.ts`:
- Around line 23-25: The route entry currently uses the plural pattern
'/meetings/' which causes real '/meeting...' requests to not match and fall
through to the protected '/' rule; update the route table entry (the object
where pattern: '/meetings/' is set) to use the public meeting path '/meeting'
(matching the silent-login mount in server.ts) and keep type: 'ssr' and auth:
'optional' so /meeting and its subpaths bypass authentication as intended.
- Around line 45-52: The string-pattern branch in classifyRoute incorrectly uses
startsWith and can match prefixes across segment boundaries; update
classifyRoute so when routeConfig.pattern is a string you return the route only
if path === routeConfig.pattern or the path continues with a segment boundary
(e.g., path.startsWith(routeConfig.pattern + '/') or routeConfig.pattern
endsWith('/') handling), ensuring exact-path matches and proper segment-boundary
matches instead of raw prefix matches; modify the logic inside classifyRoute
(the branch that checks typeof routeConfig.pattern === 'string') to implement
this exact-or-boundary check so routes like '/health' don't match '/healthcheck'
and '/api' doesn't match '/api-docs'.
- Around line 292-296: The redirect branch currently bypasses the computed
redirect target by calling res.oidc.login({ returnTo: req.originalUrl });
instead, use the precomputed decision.redirectUrl from makeAuthDecision() so the
/login?returnTo=... flow (and its returnTo validation in server.ts) runs;
replace the res.oidc.login call in the 'redirect' case with a redirect to
decision.redirectUrl (or invoke res.oidc.login only with that computed target)
so server-side validation in server.ts Lines 121-139 is exercised.
In `@apps/lfx/src/server/server.ts`:
- Around line 166-177: The catch block for req.oidc.fetchUserInfo() currently
logs a warning then calls res.oidc.logout() and returns, forcing a sign-out even
though authContext.user was already seeded from req.oidc.user; instead, remove
the forced logout/return and allow the request to continue using the existing
token claims. Update the catch handler (surrounding req.oidc.fetchUserInfo(),
authContext.user, logger.warning, res.oidc.logout()) to only log the error and
continue, ensuring authContext.user remains populated from req.oidc.user as the
fallback.
In `@apps/lfx/src/server/services/lfx.service.ts`:
- Around line 50-70: The request<T> method currently always forwards
req.bearerToken; update it to accept an explicit auth mode or token provider so
callers can request an M2M token for public endpoints (e.g., add an optional
parameter authMode: 'user' | 'm2m' or tokenProvider: () => Promise<string> to
request<T>), and when authMode === 'm2m' call a new or existing M2M helper
(getM2MToken or injected tokenProvider) to set Authorization instead of using
req.bearerToken; ensure callers to public routes pass authMode:'m2m' and
private/user routes continue to use the existing behavior, keep header
construction in request<T> (referencing request<T>, DEFAULT_QUERY_PARAMS,
buildUrl, and this.userAgent) and do not leak end-user tokens into public/M2M
flows.
In `@apps/lfx/src/server/services/logger.service.ts`:
- Around line 410-419: The sanitize(metadata: Record<string, unknown>) method is
never applied before metadata is attached to logs, so sensitive nested fields
can leak; update all logging entry points in the LoggerService (the public
logging methods in logger.service.ts that attach metadata under the data field)
to call this.sanitize(...) on any metadata/object before adding it to the log
payload, and ensure nested objects passed into server-logger.ts are sanitized
before being handed off (replace direct metadata usage with the sanitized
result); keep using the existing SENSITIVE_FIELDS and return value shape to
avoid changing downstream expectations.
In `@apps/lfx/src/styles.css`:
- Line 6: Stylelint is flagging Tailwind v4 at-rules like `@theme` in styles.css
because the at-rule-no-unknown rule from stylelint-config-standard-scss doesn't
recognize them; update your Stylelint config (e.g., .stylelintrc.json) to adjust
the "at-rule-no-unknown" rule to ignore the Tailwind at-rules by adding "theme",
"utility", "variant" (and "custom-variant" if used) to its ignoreAtRules array
so those directives are allowed.
In `@PLAN.md`:
- Around line 78-85: The plan still references a legacy MicroserviceProxyService
and a multi-file src/server/errors/ hierarchy; update the document to match the
actual scaffold by replacing any mention of MicroserviceProxyService with the
single LfxService and replacing the multi-file errors description with a single
ApiError (single file) implementation, and ensure any architecture bullets,
examples, and references (e.g., under "Server Architecture" and the lines around
149-153) reflect LfxService and ApiError so future work is not misdirected.
- Around line 90-93: Replace the workstation-specific absolute path used before
the ng new command with a repo-relative approach: remove
"/Users/asithadesilva/Sites/lfx-v2-ui" and instruct contributors to run the ng
new command from the repository workspace root (or show how to compute the repo
root dynamically), and update both ng new snippets (the bash snippet that runs
ng new lfx --ssr --style css --zoneless --prefix lfx --skip-tests and the other
occurrence) so examples are portable for contributors and CI.
---
Minor comments:
In `@apps/lfx/.prettierrc.js`:
- Around line 17-18: The HTML override in the Prettier config uses a
non-recursive glob ("files: '*.html'") so nested templates like
src/app/app.component.html are not matched; update the override entry that
currently reads files: '*.html' to use a recursive glob files: '**/*.html' so
the Angular parser override (parser: 'angular') applies to HTML files in all
subdirectories.
In `@apps/lfx/src/server/services/lfx.service.ts`:
- Around line 78-80: The code currently only sets init.body when data is truthy,
which drops valid falsy payloads like 0, false, '', and null; change the
condition in the block that assigns init.body (the variables/methods involved
are init, data, and method and the POST/PUT/PATCH check) to gate on data !==
undefined instead of truthiness so any provided value except undefined is
JSON.stringified and assigned to init.body for POST/PUT/PATCH requests.
---
Nitpick comments:
In `@apps/lfx/.env.example`:
- Around line 2-6: Reorder the Auth0 env keys in apps/lfx/.env.example to
satisfy dotenv-linter's UnorderedKey rule: replace the current block
(PCC_AUTH0_CLIENT_ID, PCC_AUTH0_CLIENT_SECRET, PCC_AUTH0_ISSUER_BASE_URL,
PCC_AUTH0_AUDIENCE, PCC_AUTH0_SECRET) with the keys sorted alphabetically
(PCC_AUTH0_AUDIENCE, PCC_AUTH0_CLIENT_ID, PCC_AUTH0_CLIENT_SECRET,
PCC_AUTH0_ISSUER_BASE_URL, PCC_AUTH0_SECRET) so lint warnings for UnorderedKey
are resolved.
In `@apps/lfx/src/app/app.routes.server.ts`:
- Around line 6-11: Add a JSDoc block above the exported constant `serverRoutes`
in app.routes.server.ts that documents this public SSR route configuration:
state that `serverRoutes` is a catch-all ServerRoute array used for server-side
rendering (uses `RenderMode.Server`), describe its purpose (fallback route
rendering on the server), and note the wildcard `path: '**'` behavior so
consumers know this is the default SSR route for the app.
In `@apps/lfx/src/app/app.routes.ts`:
- Line 6: Add a concise JSDoc comment above the exported symbol routes (type
Routes) that describes what the exported route configuration represents, its
expected contents (array of route definitions), and any usage notes (e.g.,
consumed by the router setup). Update the comment for the export const routes:
Routes declaration to be a one- or two-line JSDoc block that documents its
purpose and shape so the public module is self-documenting.
In `@apps/lfx/src/main.server.ts`:
- Around line 9-11: Add a concise JSDoc comment above the default export to
document the server-side rendering bootstrap function: describe the exported
bootstrap function (bootstrap), its purpose (initializes the SSR app using
bootstrapApplication with AppComponent), the expected parameter type (context:
BootstrapContext) and its return/behavior. Place the comment immediately above
the line exporting bootstrap in main.server.ts so future maintainers understand
the SSR entry contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58e95ee3-1362-4730-a61c-5376ac976e08
⛔ Files ignored due to path filters (2)
apps/lfx/public/favicon.icois excluded by!**/*.icoyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
.claude/rules/skill-guidance.md.claude/skills/lfx-backend-builder/SKILL.md.claude/skills/lfx-coordinator/SKILL.md.claude/skills/lfx-design/SKILL.md.claude/skills/lfx-preflight/SKILL.md.claude/skills/lfx-research/SKILL.md.claude/skills/lfx-setup/SKILL.md.claude/skills/lfx-ui-builder/SKILL.mdPLAN.mdapps/lfx/.env.exampleapps/lfx/.prettierignoreapps/lfx/.prettierrc.jsapps/lfx/angular.jsonapps/lfx/eslint.config.jsapps/lfx/package.jsonapps/lfx/src/app/app.component.cssapps/lfx/src/app/app.component.htmlapps/lfx/src/app/app.component.tsapps/lfx/src/app/app.config.server.tsapps/lfx/src/app/app.config.tsapps/lfx/src/app/app.routes.server.tsapps/lfx/src/app/app.routes.tsapps/lfx/src/index.htmlapps/lfx/src/main.server.tsapps/lfx/src/main.tsapps/lfx/src/server/helpers/api-error.tsapps/lfx/src/server/helpers/error-serializer.tsapps/lfx/src/server/helpers/server-logger.tsapps/lfx/src/server/helpers/url-validation.tsapps/lfx/src/server/middleware/auth.middleware.tsapps/lfx/src/server/middleware/error-handler.middleware.tsapps/lfx/src/server/server.tsapps/lfx/src/server/services/lfx.service.tsapps/lfx/src/server/services/logger.service.tsapps/lfx/src/styles.cssapps/lfx/src/types/express.d.tsapps/lfx/tsconfig.app.jsonapps/lfx/tsconfig.jsonpackage.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/lfx/ecosystem.config.js (1)
18-19: Cluster mode with single instance may be intentional, but worth clarifying.Using
exec_mode: 'cluster'withinstances: 1works but negates the primary benefit of cluster mode (multi-core utilization). If single-process is intended for simplicity,exec_mode: 'fork'is more conventional. If horizontal scaling is planned, considerinstances: 'max'or a specific count > 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/ecosystem.config.js` around lines 18 - 19, The config currently sets exec_mode: 'cluster' with instances: 1 which disables multi-core benefits; update the process config so intent is explicit: either switch exec_mode to 'fork' if you want a single-process setup, or set instances to 'max' (or a specific >1 count) while keeping exec_mode: 'cluster' to enable multi-core scaling; adjust the values for exec_mode and instances in the exported object (look for the exec_mode and instances keys) to reflect the chosen behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/rules/skill-guidance.md:
- Around line 49-56: The diagram lists /lfx-backend-builder and /lfx-ui-builder
but the "Skills for apps/lfx" table doesn't include them; update the table to
either add entries for /lfx-backend-builder and /lfx-ui-builder (with a short
description and invocation details) or add a clear note in the table header that
these are internal/sub-skills (not directly invocable) and link them to
/lfx-coordinator or /lfx-research as appropriate; modify the Skills for apps/lfx
table or its header accordingly and ensure the diagram and table stay consistent
(refer to the /lfx-coordinator, /lfx-research, /lfx-backend-builder,
/lfx-ui-builder symbols to locate spots to edit).
In `@PLAN.md`:
- Around line 135-138: The PLAN.md contains a hardcoded, user-specific path in
the shell example (the `cd /Users/asithadesilva/Sites/lfx-v2-ui/apps` and
following `ng new` command); change it to a relative or repo-root-based path
(e.g., `cd ./apps` or `cd ${REPO_ROOT}/apps`) so the steps work for other
developers, and update any other similar examples in PLAN.md to use relative
paths or an environment variable placeholder instead of
`/Users/asithadesilva/...`.
- Around line 91-94: Remove the hardcoded user-specific path in the bash example
(the `cd /Users/asithadesilva/Sites/lfx-v2-ui/apps` line) and replace it with a
generic instruction: tell users to change into the repository's apps directory
(e.g., "cd apps" from the repo root) or use a placeholder like
"<repo-root>/apps" before running the `ng new lfx --ssr --style css --zoneless
--prefix lfx --skip-tests` command so the doc works for all contributors.
---
Nitpick comments:
In `@apps/lfx/ecosystem.config.js`:
- Around line 18-19: The config currently sets exec_mode: 'cluster' with
instances: 1 which disables multi-core benefits; update the process config so
intent is explicit: either switch exec_mode to 'fork' if you want a
single-process setup, or set instances to 'max' (or a specific >1 count) while
keeping exec_mode: 'cluster' to enable multi-core scaling; adjust the values for
exec_mode and instances in the exported object (look for the exec_mode and
instances keys) to reflect the chosen behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8419976-53e8-4ac5-9b45-348861f8bc30
📒 Files selected for processing (14)
.claude/rules/skill-guidance.md.claude/skills/develop/SKILL.md.claude/skills/develop/evals/evals.json.claude/skills/develop/references/backend-endpoint.md.claude/skills/develop/references/frontend-component.md.claude/skills/develop/references/frontend-service.md.claude/skills/develop/references/shared-types.md.claude/skills/preflight/SKILL.md.claude/skills/preflight/evals/evals.json.claude/skills/setup/SKILL.md.claude/skills/setup/evals/evals.jsonCLAUDE.mdPLAN.mdapps/lfx/ecosystem.config.js
💤 Files with no reviewable changes (10)
- .claude/skills/develop/references/backend-endpoint.md
- .claude/skills/develop/SKILL.md
- .claude/skills/develop/references/frontend-service.md
- .claude/skills/setup/evals/evals.json
- .claude/skills/develop/references/frontend-component.md
- .claude/skills/develop/references/shared-types.md
- .claude/skills/preflight/evals/evals.json
- .claude/skills/develop/evals/evals.json
- .claude/skills/preflight/SKILL.md
- .claude/skills/setup/SKILL.md
Add 12 base UI components (avatar, badge, button, card, checkbox, chip, link-button, radio-button, spinner, tabs, toggle-switch, tooltip) with Tailwind v4 tokens, Storybook stories, and accessibility attributes. Configure Storybook for Angular 20 with Tailwind v4 PostCSS processing. Add /lfx entry-point skill and update skill routing hierarchy. Signed-off-by: Asitha de Silva <asithade@gmail.com>
- Replace require('compression') with ESM import (CodeRabbit)
- Use descriptive local-dev placeholders for OIDC config fallbacks
- Fix SSR render error handler: proper startTime capture and type guard
- Use request start time in apiErrorHandler for accurate duration
- Derive Auth0 client ID from env var instead of hardcoding
- Use decision.redirectUrl consistently in auth middleware
- Move pino-pretty to dependencies, use caret for pinned versions
- Fix JSDoc typo in auth middleware
Signed-off-by: Asitha de Silva <asithade@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (1)
apps/lfx/src/server/server.ts (1)
41-43:⚠️ Potential issue | 🔴 CriticalReplace
require('compression')in this ESM server entrypoint.This file already uses
import.meta.url, sorequire()is unavailable at runtime here. The server will fail before compression middleware is registered unlesscompressionis imported as an ES module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/server.ts` around lines 41 - 43, The server uses require('compression') in an ESM context (it already uses import.meta.url), which will fail at runtime; replace the CommonJS require with an ES module import (e.g., add "import compression from 'compression';" and remove the require line) and keep using the same compression identifier where middleware is registered (e.g., app.use(compression())). If dynamic import is preferred, use a top-level await import and grab the default export ((await import('compression')).default) and assign it to the existing compression variable.
🧹 Nitpick comments (14)
.gitignore (1)
66-66: Optional cleanup: remove duplicate coverage ignore rule.
/coveragehere overlaps withcoveragealready defined on Line 19. Keeping a single rule improves maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 66, The .gitignore contains a duplicated coverage ignore entry: remove the redundant "/coverage" line so only the existing "coverage" rule remains; ensure the file keeps a single coverage ignore entry and run a quick git status to confirm no unintended changes.apps/lfx/src/app/shared/components/badge/badge.ts (1)
6-7: Consider exporting type aliases for external use.If consumers of the
Badgecomponent need to programmatically setvariantorsize, exporting these types would provide type safety at the call site.♻️ Optional: Export type aliases
-type BadgeVariant = 'neutral' | 'info' | 'success' | 'warning' | 'danger' | 'discovery'; -type BadgeSize = 'sm' | 'lg'; +export type BadgeVariant = 'neutral' | 'info' | 'success' | 'warning' | 'danger' | 'discovery'; +export type BadgeSize = 'sm' | 'lg';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/badge/badge.ts` around lines 6 - 7, Export the type aliases BadgeVariant and BadgeSize so external consumers can import them for type-safe props; locate the type declarations BadgeVariant and BadgeSize in the badge.ts file and add exports (e.g., export type BadgeVariant = ... and export type BadgeSize = ...) ensuring they are available alongside the Badge component's props for external use.apps/lfx/src/app/shared/components/checkbox/checkbox.ts (1)
27-43: Minor simplification: extract commonrounded-smclass.The
rounded-smclass is applied regardless of size. Consider extracting it to avoid repetition:♻️ Optional simplification
- const sizeClass = isSmall ? 'size-3 rounded-sm' : 'size-4 rounded-sm'; + const sizeClass = isSmall ? 'size-3' : 'size-4'; - return `${sizeClass} ${stateClass}`; + return `${sizeClass} rounded-sm ${stateClass}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/checkbox/checkbox.ts` around lines 27 - 43, The computed boxClasses function duplicates the 'rounded-sm' token in the sizeClass; simplify by extracting 'rounded-sm' out of the conditional: in the computed property boxClasses (in checkbox.ts) keep the size-specific part as just 'size-3' or 'size-4' (e.g., variable sizeOnly) and then compose the final class string as `${sizeOnly} rounded-sm ${stateClass}` so rounded-sm is not repeated and behavior remains identical.apps/lfx/src/app/shared/components/checkbox/checkbox.html (1)
4-11: Add accessibility attributes for screen readers when no label is provided.The checkbox works well with a visible label, but when
label()is falsy, screen readers won't have context for the checkbox. Consider adding anaria-labelattribute to the input for labelless usage.Additionally, the
indeterminateHTML attribute doesn't exist on inputs—it's a JavaScript property. While the visual state is handled via the icon, the native input won't reflect the indeterminate state to assistive technology.♻️ Suggested accessibility improvements
- <input type="checkbox" class="peer sr-only" [checked]="checked()" [disabled]="disabled()" (change)="onToggle()" data-testid="checkbox-input" /> + <input + type="checkbox" + class="peer sr-only" + [checked]="checked()" + [disabled]="disabled()" + [attr.aria-label]="label() ? null : 'checkbox'" + [attr.aria-checked]="indeterminate() ? 'mixed' : checked()" + (change)="onToggle()" + data-testid="checkbox-input" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/checkbox/checkbox.html` around lines 4 - 11, When the checkbox has no visible label the input lacks an accessible name and the indeterminate state is only visual; add an aria-label to the <input> when label() is falsy (use the existing label() value or a passed prop/default string) and ensure the native indeterminate property is set in the component code (not the template) by querying the input element with data-testid="checkbox-input" and assigning input.indeterminate = indeterminate() on init and whenever indeterminate() changes; update the component methods (those around checked(), indeterminate(), onToggle(), boxClasses(), iconClasses(), disabled()) to keep the DOM input.indeterminate and aria-label in sync.apps/lfx/src/app/shared/components/spinner/spinner.ts (1)
6-14: Adddata-testidhost attribute for consistency.Other components in this PR (Card, Badge, Tooltip, Button) include a host
data-testidattribute for testing. Consider adding it here for consistency.💡 Suggested change
`@Component`({ selector: 'lfx-spinner', imports: [], templateUrl: './spinner.html', styleUrl: './spinner.css', + host: { + '[attr.data-testid]': '"spinner"', + }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/spinner/spinner.ts` around lines 6 - 14, The Spinner component is missing the host data-testid attribute used elsewhere; update the `@Component` decorator for the Spinner class to include a host entry that sets data-testid to "lfx-spinner" so tests can consistently target it (i.e., add a host property in the `@Component` metadata for the Spinner component to set the attribute).apps/lfx/src/app/shared/components/button/button.ts (1)
31-40: Consider using a design token instead of hardcoded hex color.Line 33 uses
hover:bg-[#0082d9]which bypasses the design token system. For consistency with the rest of the design system, consider defining this as a theme token (e.g.,hover:bg-info-600).💡 Suggested change
const variantClasses: Record<ButtonVariant, string> = { - primary: 'bg-info-500 text-white hover:bg-[`#0082d9`]', + primary: 'bg-info-500 text-white hover:bg-info-600',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/button/button.ts` around lines 31 - 40, The buttonClasses computed mapping uses a hardcoded hex color for the primary hover state; replace the inline hex in the primary variant (currently 'hover:bg-[`#0082d9`]') with the design token class (e.g., 'hover:bg-info-600') and, if that token does not exist yet, add it to your theme/token config so Tailwind/your CSS system recognizes it; update the primary entry in the buttonClasses mapping (refer to buttonClasses and ButtonVariant) accordingly to use the token-based class.apps/lfx/src/app/shared/components/tooltip/tooltip.html (1)
4-5: Consider combining static and dynamic classes in the computed property.Using both
class="..."and[class]="containerClasses()"works in Angular but can be clearer if all classes are returned from the computed property. This ensures predictable class application and makes the template more readable.♻️ Suggested refactor
In
tooltip.ts, update the computed properties to include the base classes:public containerClasses = computed(() => { - return this.description() ? 'flex-col gap-1 items-start' : ''; + const base = 'flex rounded-md bg-neutral-900 px-2 py-1.5 shadow-lg'; + return this.description() ? `${base} flex-col gap-1 items-start` : base; }); public labelClasses = computed(() => { - return this.description() ? '' : 'text-center w-full'; + const base = 'text-xs leading-4 font-semibold text-white'; + return this.description() ? base : `${base} text-center w-full`; });Then simplify the template:
-<div class="flex rounded-md bg-neutral-900 px-2 py-1.5 shadow-lg" [class]="containerClasses()" data-testid="tooltip-container" role="tooltip"> - <span class="text-xs leading-4 font-semibold text-white" [class]="labelClasses()" data-testid="tooltip-label">{{ label() }}</span> +<div [class]="containerClasses()" data-testid="tooltip-container" role="tooltip"> + <span [class]="labelClasses()" data-testid="tooltip-label">{{ label() }}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/tooltip/tooltip.html` around lines 4 - 5, Merge static and dynamic class strings into the computed class getters so the template only binds the computed value: update the containerClasses() and labelClasses() methods in tooltip.ts to return the full base class strings (e.g., include "flex rounded-md bg-neutral-900 px-2 py-1.5 shadow-lg" for the container and "text-xs leading-4 font-semibold text-white" for the label) plus any conditional classes, then simplify tooltip.html by removing the static class attributes and using only [class]="containerClasses()" and [class]="labelClasses()" respectively.apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts (1)
108-114: Hardcoded arrow color may drift from Tailwind design tokens.The arrow uses
#0f172awhile the tooltip background usesbg-neutral-900. In Tailwind v4 with OKLCH color space, these may not match exactly. Consider using a CSS custom property or ensuring the hex value aligns with your@themetokens.♻️ Option: Use CSS for the arrow instead of inline styles
Define the arrow in
tooltip.cssusing Tailwind's color tokens:.tooltip-arrow { position: absolute; left: 50%; transform: translateX(-50%); width: 0; height: 0; border-left: 5px solid transparent; border-right: 5px solid transparent; } .tooltip-arrow--top { bottom: -5px; border-top: 5px solid theme(colors.neutral.900); } .tooltip-arrow--bottom { top: -5px; border-bottom: 5px solid theme(colors.neutral.900); }Then in
createArrow():- this.renderer.setStyle(arrow, 'border-left', `${ARROW_SIZE}px solid transparent`); - this.renderer.setStyle(arrow, 'border-right', `${ARROW_SIZE}px solid transparent`); - - if (position === 'top') { - this.renderer.setStyle(arrow, 'bottom', `-${ARROW_SIZE}px`); - this.renderer.setStyle(arrow, 'border-top', `${ARROW_SIZE}px solid `#0f172a``); - } else { - this.renderer.setStyle(arrow, 'top', `-${ARROW_SIZE}px`); - this.renderer.setStyle(arrow, 'border-bottom', `${ARROW_SIZE}px solid `#0f172a``); - } + this.renderer.addClass(arrow, 'tooltip-arrow'); + this.renderer.addClass(arrow, position === 'top' ? 'tooltip-arrow--top' : 'tooltip-arrow--bottom');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts` around lines 108 - 114, The arrow's border color is hardcoded to `#0f172a` and can drift from Tailwind tokens; update createArrow (or the method setting arrow styles) to use a CSS custom property or the tooltip's computed background token instead of the hex literal — e.g., apply a class (tooltip-arrow/tooltip-arrow--top|bottom) or set the border-color via a CSS variable like --tooltip-bg that maps to theme(colors.neutral.900), or read the tooltip host's computed background and assign that value to the arrow's border (use renderer to set border-top-color/border-bottom-color or add the class). Ensure both top and bottom branches reference that token/variable rather than `#0f172a`.apps/lfx/src/app/shared/components/link-button/link-button.html (1)
12-16: Hide decorative icons from screen readers.On Line 12 and Line 16, mark icon-only
<i>elements as decorative to avoid unnecessary announcements.♿ Suggested tweak
- <i [class]="'fa-light fa-' + leftIcon()" data-testid="link-button-left-icon"></i> + <i [class]="'fa-light fa-' + leftIcon()" aria-hidden="true" data-testid="link-button-left-icon"></i> ... - <i [class]="'fa-light fa-' + rightIcon()" data-testid="link-button-right-icon"></i> + <i [class]="'fa-light fa-' + rightIcon()" aria-hidden="true" data-testid="link-button-right-icon"></i>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/link-button/link-button.html` around lines 12 - 16, The leftIcon() and rightIcon() <i> elements are decorative and should be hidden from screen readers; update the link-button template to add accessibility attributes to those <i> elements (e.g., aria-hidden="true" and focusable="false" or role="presentation") so the icon-only elements rendered by the leftIcon() and rightIcon() checks are not announced by assistive tech.apps/lfx/src/app/shared/components/card/card.ts (1)
15-15: Add a brief JSDoc block for the exportedCardcomponent.Please document the component purpose and its public inputs (
padding,hoverable) to align with repository standards.✍️ Proposed update
- export class Card { + /** + * Reusable container component with configurable internal padding + * and optional hover elevation styling. + */ + export class Card {As per coding guidelines "Add JSDoc comments for public functions and exported modules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/card/card.ts` at line 15, Add a brief JSDoc block above the exported Card class describing its purpose and documenting its public inputs: `padding` (type/expected values and default behavior) and `hoverable` (boolean behavior and effect). Place the comment immediately above `export class Card {` and include one-line summary, `@param` or `@property` entries for `padding` and `hoverable`, and any default values or side effects so the exported component follows the repository's documentation standard.apps/lfx/eslint.config.js (1)
14-14: Avoid takingsrc/main.tsout of lint coverage.Line 14 excludes the bootstrap file entirely. In this app that's where the zoneless/bootstrap wiring lives, so the entry point loses all lint coverage instead of just the one rule that's noisy. Prefer a targeted override for
src/main.tsor a local disable for the specific rule you need to relax.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/eslint.config.js` at line 14, The project-wide ignore removed the entire entry file from linting by excluding the main.ts bootstrap, so revert removing that file from the exclude list in eslint.config.js and instead apply a targeted relaxation: either add an ESLint override in eslint.config.js that targets the entry file (main.ts) and turns off only the noisy rule(s) used for the zoneless/bootstrap wiring, or add a local inline eslint-disable comment in main.ts for the specific rule(s); update the configuration to re-enable full lint coverage for main.ts except for the single rule you need to relax.apps/lfx/src/app/shared/components/avatar/avatar.ts (1)
6-8: Consider extracting types to a shared location.
AvatarSizeandAvatarTypeare defined locally. Per coding guidelines, interfaces for object shapes should be used, and shared types could live in@lfx-one/sharedfor reuse (e.g., by the Chip component that imports Avatar).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/avatar/avatar.ts` around lines 6 - 8, AvatarSize and AvatarType are local type aliases and should be extracted to the shared types package and converted to exported interfaces/types for reuse; create exported types (e.g., export type AvatarSize = 'xs'|'sm'|'md'|'lg'|'xl' and export type AvatarType = 'photo'|'placeholder'|'initials'|'org-logo'|'project-placeholder'|'org-placeholder') in the shared package (e.g., `@lfx-one/shared`), update the Avatar component to import these exported types instead of redefining them, and update any other consumers (like the Chip component) to import the shared types to avoid duplication while preserving existing names and runtime behavior.apps/lfx/src/app/shared/components/chip/chip.ts (1)
20-47: Add JSDoc for exported/public API surface.The exported
Chipclass and publiconDismiss()method should include short JSDoc blocks to align with repo standards.✅ Suggested fix
-export class Chip { +/** + * Renders a design-system chip with label, icon, or avatar variants. + */ +export class Chip { @@ - public onDismiss(): void { + /** Emits when the dismiss button is clicked. */ + public onDismiss(): void { this.dismissed.emit(); } }As per coding guidelines, "Add JSDoc comments for public functions and exported modules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/chip/chip.ts` around lines 20 - 47, Add concise JSDoc for the exported Chip class and its public onDismiss() method: add a short one-line description above the exported class Chip explaining its purpose, and add a JSDoc block above onDismiss() describing that it emits the dismissed output event (include `@returns` {void} or `@return` void if your linter requires it). Ensure the comments reference the public API symbols Chip and onDismiss and follow the repo’s JSDoc style (brief description and tags as needed).apps/lfx/src/app/shared/components/tabs/tabs.stories.ts (1)
4-23: Type the story fixtures againstTabDefinition.These arrays currently rely on inference, so the Storybook data can drift from the component contract without a compiler error. Annotating them as
TabDefinition[]keeps the fixtures aligned with the component API.♻️ Suggested cleanup
-import { Tabs } from './tabs'; +import { type TabDefinition, Tabs } from './tabs'; -const sampleTabs = [ +const sampleTabs: TabDefinition[] = [ @@ -const sampleTabsNoIcons = [ +const sampleTabsNoIcons: TabDefinition[] = [As per coding guidelines "Use type annotations in TypeScript files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/tabs/tabs.stories.ts` around lines 4 - 23, The story fixtures sampleTabs and sampleTabsNoIcons should be explicitly typed to match the component contract: annotate both arrays as TabDefinition[] so they will be checked by the compiler against the Tabs component's expected shape; update the declarations for sampleTabs and sampleTabsNoIcons to use the TabDefinition type (importing TabDefinition if necessary) to prevent fixture drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/lfx-design/SKILL.md:
- Around line 26-33: The fenced code block containing the call to
mcp__plugin_figma_figma__get_design_context is missing a language identifier
which triggers MD040; update that fence to include a language tag (for example
"text" or "bash") immediately after the opening ``` so the block becomes ```text
and CI will accept it. Locate the block with the function-like snippet
(mcp__plugin_figma_figma__get_design_context(...)) in SKILL.md and add the
language identifier to the opening fence.
In @.claude/skills/lfx/SKILL.md:
- Around line 22-33: Update the unlabeled code fences in
.claude/skills/lfx/SKILL.md to include a fence language (use "text") so
markdown-lint MD040 is satisfied; specifically label the fence that wraps the
"Welcome to LFX development! What would you like to do? ..." snippet and the
fence that wraps the "I can see you're working in the lfx-v2-ui repository..."
snippet with ```text to replace the current unlabeled ``` fences; ensure both
occurrences (the snippet starting with "Welcome to LFX development!" and the
later snippet starting with "I can see you're working...") are updated
consistently.
In `@apps/lfx/eslint.config.js`:
- Around line 42-49: The current `@typescript-eslint/naming-convention` block is
too permissive (it allows PascalCase for variables/parameters and UPPER_CASE for
any variable); update the rule set so that the { selector: 'variable', format:
... } and { selector: 'parameter', format: ... } entries only allow camelCase
(keep leadingUnderscore: 'allow' for both) and add a new { selector: 'variable',
modifiers: ['const'], format: ['UPPER_CASE'] } entry to restrict UPPER_CASE to
constants; adjust/remove the existing PascalCase entries for
variables/parameters so only typeLike keeps PascalCase.
In `@apps/lfx/src/app/shared/components/avatar/avatar.ts`:
- Around line 29-38: The initials computed getter can produce "UNDEFINED" when
trailing/multiple spaces yield empty words; in the initials computed (variables
nameValue, words, first, last) ensure you filter out empty strings (e.g., use
.filter(Boolean) or find the last non-empty word) before taking characters, and
defensively default both first and last to '' so the result is always
(first+last).toUpperCase(); update the logic in the initials computed to use the
cleaned words array or a safe last-word lookup.
In `@apps/lfx/src/app/shared/components/button/button.html`:
- Around line 4-10: The button lacks relative positioning so the loading spinner
(which is absolutely positioned) cannot be centered; update the button element
(the one using type(), buttonClasses(), isDisabled(), onClick()) to include
relative positioning by adding the "relative" class to its class list (or ensure
buttonClasses() returns a class string that includes "relative") so the spinner
overlay is positioned correctly inside the button.
In `@apps/lfx/src/app/shared/components/card/card.stories.ts`:
- Around line 58-61: In the AllVariants story for the lfx-card component the
"None" example is misleading because the inner element uses a p-4 wrapper which
adds padding; update the story so the padding="none" card has no added internal
padding (remove the <div class="p-4"> wrapper or replace it with plain text or a
non-padding wrapper) so the lfx-card padding="none" variant is shown correctly;
locate the AllVariants story and the lfx-card instance with padding="none" and
remove or change the p-4 class on its inner content.
In `@apps/lfx/src/app/shared/components/chip/chip.html`:
- Around line 16-17: The dismiss button in chip.html is icon-only and lacks an
accessible name; update the <button> element that calls onDismiss()
(data-testid="chip-dismiss") to provide an accessible label—e.g., add an
aria-label or include visually-hidden text describing the action (like "Dismiss
chip" or "Remove tag") so screen readers can announce it, ensuring the label is
concise and matches the component intent.
In `@apps/lfx/src/app/shared/components/link-button/link-button.ts`:
- Around line 18-50: Add JSDoc comments for the exported class LinkButton and
its public method onClick: add a brief description for the LinkButton component
(its purpose, props/inputs like variant, size, disabled, underline, leftIcon,
rightIcon, and the clicked output) and a JSDoc block for onClick describing the
event parameter (MouseEvent) and that it emits clicked when not disabled; place
the comments immediately above the exported class declaration (LinkButton) and
above the onClick method to satisfy the public API documentation guideline.
In `@apps/lfx/src/app/shared/components/tabs/tab-item.ts`:
- Around line 14-21: The host currently exposes role="tab", tabindex and
keyboard/click handlers while the template also renders a native <button>,
resulting in two interactive targets; remove the interactive ARIA/keyboard
attributes from the host and move them onto the inner button so there is a
single focusable element. Specifically, remove '[attr.role]',
'[attr.aria-selected]', '[attr.tabindex]', '(click)', '(keydown.enter)',
'(keydown.space)' from the host definition and add equivalent bindings to the
template's button (reusing the existing selected() and onSelect() methods),
keeping the host-only attributes like '[attr.data-testid]' if needed; ensure the
button has role="tab", [attr.aria-selected]="selected()",
[attr.tabindex]="selected()?0:-1" and the click/keydown handlers so
arrow/Home/End and focus behavior follow the tab pattern.
In `@apps/lfx/src/app/shared/components/tabs/tabs.html`:
- Around line 6-14: The template sets data-testid="tabs-tab" on the lfx-tab-item
element which conflicts with the host-bound data-testid="tab-item" in the
TabItem component (see lfx-tab-item / tab-item.ts); update one side to avoid
duplicate attributes at runtime — either make the child host test id
configurable (add an `@Input` like testId or host binding read from an input in
TabItem) so both selectors can coexist, or change this template to use a
different attribute (e.g., data-tab-test or aria-label) and update tests
accordingly; locate lfx-tab-item usage in tabs.html and the TabItem component
(tab-item.ts) to apply the fix.
In `@apps/lfx/src/app/shared/components/tabs/tabs.ts`:
- Around line 25-48: The component input named style conflicts with the DOM
element's native style binding; change the input declaration public style =
input<TabStyle>('switch') in the Tabs component (and the equivalent in TabItem)
to use an alias like tabStyle (e.g., declare the input with an alias/renamed
identifier so Angular exposes it as tabStyle), and then update all call sites to
bind [tabStyle]="..." instead of [style]="..."; ensure you update both
apps/lfx/src/app/shared/components/tabs/tabs.ts (the public style input and
containerClasses usage) and apps/lfx/src/app/shared/components/tabs/tab-item.ts
to use the new input name/alias.
In `@apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts`:
- Line 138: The tooltip directive currently accesses window directly (e.g., in
the clampedLeft calculation in positionTooltip) which breaks SSR; inject
PLATFORM_ID in the class constructor and derive an isBrowser boolean (using
isPlatformBrowser(PLATFORM_ID)), then guard DOM/window logic: make show() no-op
when !isBrowser and have positionTooltip() return early when !isBrowser so no
window/innerWidth/tooltipRect calls run on the server; update uses of
clampedLeft and any other window/document accesses inside positionTooltip() to
be executed only when isBrowser is true.
In `@apps/lfx/src/server/helpers/url-validation.ts`:
- Around line 70-74: DOMAIN_ALLOWLIST contains a hardcoded Auth0 client cookie
name that drifts from the client id used in extractDomainFromCookie (the literal
jStGXyf3nwTswv8goh6FcbU4EaWUZBNP), causing valid dev cookies to be rejected;
update the logic so the cookie client id is derived from config/allowlist rather
than a second literal: either add cookie-name or cookie-pattern entries to
DOMAIN_ALLOWLIST and have extractDomainFromCookie read the allowed cookie
pattern(s) from DOMAIN_ALLOWLIST (or central config) when validating
auth0.<client>.is.authenticated, or change extractDomainFromCookie to validate
against any auth0.<allowedClient>.is.authenticated where allowedClient comes
from DOMAIN_ALLOWLIST entries (e.g., hostnames or explicit client ids) so the
check remains in sync with the allowlist used elsewhere.
In `@apps/lfx/src/server/middleware/auth.middleware.ts`:
- Around line 16-31: The route classifier is using naive startsWith checks which
misclassifies prefixes (e.g., '/healthz', '/public/apiary', '/api-docs') and
misses '/meetings' because the config uses '/meetings/'; update classifyRoute to
perform boundary-aware matching against DEFAULT_ROUTE_CONFIG: treat a pattern as
matching when the request path equals the pattern OR when the path has the
pattern as a full segment prefix (i.e., pattern === '/' or path === pattern or
path.startsWith(pattern + '/') ), and normalize trailing slashes so '/meetings'
and '/meetings/' both match; apply the same boundary-aware logic wherever
classifyRoute or its matching logic is duplicated (the other matching block
referenced at lines 45-62).
In `@apps/lfx/src/server/middleware/error-handler.middleware.ts`:
- Around line 35-36: The duration is wrong because error-handler.middleware.ts
computes now = Date.now() at error time; instead add an early middleware that
sets a request-level start timestamp (e.g., attach startTime to req.startTime or
res.locals.startTime) on every incoming request, ensure this middleware runs
before routing/handlers, then modify the error handler (where
getOperationFromPath and logger.error are used) to read that startTime and
compute duration = Date.now() - startTime (fall back to 0 if missing); reference
the request startTime property in the error handler rather than using a
freshly-captured now.
In `@apps/lfx/src/server/server.ts`:
- Around line 96-112: The authConfig currently uses permissive fallback values
for critical OIDC settings (authConfig keys: clientID, clientSecret,
issuerBaseURL, audience, secret, baseURL) which allows the server to start in a
broken auth state; modify server startup to validate these environment variables
(PCC_AUTH0_CLIENT_ID, PCC_AUTH0_CLIENT_SECRET, PCC_AUTH0_ISSUER_BASE_URL,
PCC_AUTH0_AUDIENCE, PCC_AUTH0_SECRET, PCC_BASE_URL) before building authConfig
and throw an error (exit process) if any are missing or equal to the current
dummy defaults, so authConfig is only created with real values and the process
fails fast on misconfiguration.
- Around line 161-176: The catch block that handles failures from
req.oidc.fetchUserInfo() should not call res.oidc.logout() or return; instead
preserve req.oidc?.user as the fallback and allow SSR to continue. Modify the
catch in server.ts so it logs the warning (including error and path) but does
not call res.oidc.logout() or abort; ensure authContext.user remains whatever
was on req.oidc?.user and proceed with rendering (references: authContext.user,
req.oidc?.user, req.oidc.fetchUserInfo(), res.oidc.logout()).
In `@apps/lfx/src/styles.css`:
- Around line 121-147: Rename the CSS custom property namespaces to Tailwind v4
conventions: change --font-family-sans to --font-sans, rename all --font-size-*
variables (e.g., --font-size-sm, --font-size-lg, --font-size-2xl) to --text-*
(e.g., --text-sm, --text-lg, --text-2xl), rename
--line-height-tight/normal/relaxed to --leading-tight/normal/relaxed, and either
relocate or rename the --font-weight-* variables (e.g.,
--font-weight-normal/medium/bold) if you want Tailwind utilities to pick them up
(or keep them outside `@theme` if not used by utilities); leave --spacing-* as-is
and update any references in components to use the new variable names (look for
usages of --font-family-sans, --font-size-*, --line-height-*, and
--font-weight-* to update accordingly).
---
Duplicate comments:
In `@apps/lfx/src/server/server.ts`:
- Around line 41-43: The server uses require('compression') in an ESM context
(it already uses import.meta.url), which will fail at runtime; replace the
CommonJS require with an ES module import (e.g., add "import compression from
'compression';" and remove the require line) and keep using the same compression
identifier where middleware is registered (e.g., app.use(compression())). If
dynamic import is preferred, use a top-level await import and grab the default
export ((await import('compression')).default) and assign it to the existing
compression variable.
---
Nitpick comments:
In @.gitignore:
- Line 66: The .gitignore contains a duplicated coverage ignore entry: remove
the redundant "/coverage" line so only the existing "coverage" rule remains;
ensure the file keeps a single coverage ignore entry and run a quick git status
to confirm no unintended changes.
In `@apps/lfx/eslint.config.js`:
- Line 14: The project-wide ignore removed the entire entry file from linting by
excluding the main.ts bootstrap, so revert removing that file from the exclude
list in eslint.config.js and instead apply a targeted relaxation: either add an
ESLint override in eslint.config.js that targets the entry file (main.ts) and
turns off only the noisy rule(s) used for the zoneless/bootstrap wiring, or add
a local inline eslint-disable comment in main.ts for the specific rule(s);
update the configuration to re-enable full lint coverage for main.ts except for
the single rule you need to relax.
In `@apps/lfx/src/app/shared/components/avatar/avatar.ts`:
- Around line 6-8: AvatarSize and AvatarType are local type aliases and should
be extracted to the shared types package and converted to exported
interfaces/types for reuse; create exported types (e.g., export type AvatarSize
= 'xs'|'sm'|'md'|'lg'|'xl' and export type AvatarType =
'photo'|'placeholder'|'initials'|'org-logo'|'project-placeholder'|'org-placeholder')
in the shared package (e.g., `@lfx-one/shared`), update the Avatar component to
import these exported types instead of redefining them, and update any other
consumers (like the Chip component) to import the shared types to avoid
duplication while preserving existing names and runtime behavior.
In `@apps/lfx/src/app/shared/components/badge/badge.ts`:
- Around line 6-7: Export the type aliases BadgeVariant and BadgeSize so
external consumers can import them for type-safe props; locate the type
declarations BadgeVariant and BadgeSize in the badge.ts file and add exports
(e.g., export type BadgeVariant = ... and export type BadgeSize = ...) ensuring
they are available alongside the Badge component's props for external use.
In `@apps/lfx/src/app/shared/components/button/button.ts`:
- Around line 31-40: The buttonClasses computed mapping uses a hardcoded hex
color for the primary hover state; replace the inline hex in the primary variant
(currently 'hover:bg-[`#0082d9`]') with the design token class (e.g.,
'hover:bg-info-600') and, if that token does not exist yet, add it to your
theme/token config so Tailwind/your CSS system recognizes it; update the primary
entry in the buttonClasses mapping (refer to buttonClasses and ButtonVariant)
accordingly to use the token-based class.
In `@apps/lfx/src/app/shared/components/card/card.ts`:
- Line 15: Add a brief JSDoc block above the exported Card class describing its
purpose and documenting its public inputs: `padding` (type/expected values and
default behavior) and `hoverable` (boolean behavior and effect). Place the
comment immediately above `export class Card {` and include one-line summary,
`@param` or `@property` entries for `padding` and `hoverable`, and any default
values or side effects so the exported component follows the repository's
documentation standard.
In `@apps/lfx/src/app/shared/components/checkbox/checkbox.html`:
- Around line 4-11: When the checkbox has no visible label the input lacks an
accessible name and the indeterminate state is only visual; add an aria-label to
the <input> when label() is falsy (use the existing label() value or a passed
prop/default string) and ensure the native indeterminate property is set in the
component code (not the template) by querying the input element with
data-testid="checkbox-input" and assigning input.indeterminate = indeterminate()
on init and whenever indeterminate() changes; update the component methods
(those around checked(), indeterminate(), onToggle(), boxClasses(),
iconClasses(), disabled()) to keep the DOM input.indeterminate and aria-label in
sync.
In `@apps/lfx/src/app/shared/components/checkbox/checkbox.ts`:
- Around line 27-43: The computed boxClasses function duplicates the
'rounded-sm' token in the sizeClass; simplify by extracting 'rounded-sm' out of
the conditional: in the computed property boxClasses (in checkbox.ts) keep the
size-specific part as just 'size-3' or 'size-4' (e.g., variable sizeOnly) and
then compose the final class string as `${sizeOnly} rounded-sm ${stateClass}` so
rounded-sm is not repeated and behavior remains identical.
In `@apps/lfx/src/app/shared/components/chip/chip.ts`:
- Around line 20-47: Add concise JSDoc for the exported Chip class and its
public onDismiss() method: add a short one-line description above the exported
class Chip explaining its purpose, and add a JSDoc block above onDismiss()
describing that it emits the dismissed output event (include `@returns` {void} or
`@return` void if your linter requires it). Ensure the comments reference the
public API symbols Chip and onDismiss and follow the repo’s JSDoc style (brief
description and tags as needed).
In `@apps/lfx/src/app/shared/components/link-button/link-button.html`:
- Around line 12-16: The leftIcon() and rightIcon() <i> elements are decorative
and should be hidden from screen readers; update the link-button template to add
accessibility attributes to those <i> elements (e.g., aria-hidden="true" and
focusable="false" or role="presentation") so the icon-only elements rendered by
the leftIcon() and rightIcon() checks are not announced by assistive tech.
In `@apps/lfx/src/app/shared/components/spinner/spinner.ts`:
- Around line 6-14: The Spinner component is missing the host data-testid
attribute used elsewhere; update the `@Component` decorator for the Spinner class
to include a host entry that sets data-testid to "lfx-spinner" so tests can
consistently target it (i.e., add a host property in the `@Component` metadata for
the Spinner component to set the attribute).
In `@apps/lfx/src/app/shared/components/tabs/tabs.stories.ts`:
- Around line 4-23: The story fixtures sampleTabs and sampleTabsNoIcons should
be explicitly typed to match the component contract: annotate both arrays as
TabDefinition[] so they will be checked by the compiler against the Tabs
component's expected shape; update the declarations for sampleTabs and
sampleTabsNoIcons to use the TabDefinition type (importing TabDefinition if
necessary) to prevent fixture drift.
In `@apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts`:
- Around line 108-114: The arrow's border color is hardcoded to `#0f172a` and can
drift from Tailwind tokens; update createArrow (or the method setting arrow
styles) to use a CSS custom property or the tooltip's computed background token
instead of the hex literal — e.g., apply a class
(tooltip-arrow/tooltip-arrow--top|bottom) or set the border-color via a CSS
variable like --tooltip-bg that maps to theme(colors.neutral.900), or read the
tooltip host's computed background and assign that value to the arrow's border
(use renderer to set border-top-color/border-bottom-color or add the class).
Ensure both top and bottom branches reference that token/variable rather than
`#0f172a`.
In `@apps/lfx/src/app/shared/components/tooltip/tooltip.html`:
- Around line 4-5: Merge static and dynamic class strings into the computed
class getters so the template only binds the computed value: update the
containerClasses() and labelClasses() methods in tooltip.ts to return the full
base class strings (e.g., include "flex rounded-md bg-neutral-900 px-2 py-1.5
shadow-lg" for the container and "text-xs leading-4 font-semibold text-white"
for the label) plus any conditional classes, then simplify tooltip.html by
removing the static class attributes and using only [class]="containerClasses()"
and [class]="labelClasses()" respectively.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1becf62f-70f9-4d74-bf70-d72671ec206e
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (80)
.claude/rules/skill-guidance.md.claude/settings.json.claude/skills/lfx-backend-builder/SKILL.md.claude/skills/lfx-coordinator/SKILL.md.claude/skills/lfx-design/SKILL.md.claude/skills/lfx-preflight/SKILL.md.claude/skills/lfx-setup/SKILL.md.claude/skills/lfx/SKILL.md.gitignorePLAN.mdapps/lfx/.postcssrc.jsonapps/lfx/.storybook/main.tsapps/lfx/.storybook/preview.tsapps/lfx/.storybook/tsconfig.doc.jsonapps/lfx/.storybook/tsconfig.jsonapps/lfx/.storybook/typings.d.tsapps/lfx/angular.jsonapps/lfx/eslint.config.jsapps/lfx/package.jsonapps/lfx/src/app/shared/components/avatar/avatar.cssapps/lfx/src/app/shared/components/avatar/avatar.htmlapps/lfx/src/app/shared/components/avatar/avatar.stories.tsapps/lfx/src/app/shared/components/avatar/avatar.tsapps/lfx/src/app/shared/components/badge/badge.cssapps/lfx/src/app/shared/components/badge/badge.htmlapps/lfx/src/app/shared/components/badge/badge.stories.tsapps/lfx/src/app/shared/components/badge/badge.tsapps/lfx/src/app/shared/components/button/button.cssapps/lfx/src/app/shared/components/button/button.htmlapps/lfx/src/app/shared/components/button/button.stories.tsapps/lfx/src/app/shared/components/button/button.tsapps/lfx/src/app/shared/components/card/card.cssapps/lfx/src/app/shared/components/card/card.htmlapps/lfx/src/app/shared/components/card/card.stories.tsapps/lfx/src/app/shared/components/card/card.tsapps/lfx/src/app/shared/components/checkbox/checkbox.cssapps/lfx/src/app/shared/components/checkbox/checkbox.htmlapps/lfx/src/app/shared/components/checkbox/checkbox.stories.tsapps/lfx/src/app/shared/components/checkbox/checkbox.tsapps/lfx/src/app/shared/components/chip/chip.cssapps/lfx/src/app/shared/components/chip/chip.htmlapps/lfx/src/app/shared/components/chip/chip.stories.tsapps/lfx/src/app/shared/components/chip/chip.tsapps/lfx/src/app/shared/components/link-button/link-button.cssapps/lfx/src/app/shared/components/link-button/link-button.htmlapps/lfx/src/app/shared/components/link-button/link-button.stories.tsapps/lfx/src/app/shared/components/link-button/link-button.tsapps/lfx/src/app/shared/components/radio-button/radio-button.cssapps/lfx/src/app/shared/components/radio-button/radio-button.htmlapps/lfx/src/app/shared/components/radio-button/radio-button.stories.tsapps/lfx/src/app/shared/components/radio-button/radio-button.tsapps/lfx/src/app/shared/components/spinner/spinner.cssapps/lfx/src/app/shared/components/spinner/spinner.htmlapps/lfx/src/app/shared/components/spinner/spinner.stories.tsapps/lfx/src/app/shared/components/spinner/spinner.tsapps/lfx/src/app/shared/components/tabs/tab-item.cssapps/lfx/src/app/shared/components/tabs/tab-item.htmlapps/lfx/src/app/shared/components/tabs/tab-item.tsapps/lfx/src/app/shared/components/tabs/tabs.cssapps/lfx/src/app/shared/components/tabs/tabs.htmlapps/lfx/src/app/shared/components/tabs/tabs.stories.tsapps/lfx/src/app/shared/components/tabs/tabs.tsapps/lfx/src/app/shared/components/toggle-switch/toggle-switch.cssapps/lfx/src/app/shared/components/toggle-switch/toggle-switch.htmlapps/lfx/src/app/shared/components/toggle-switch/toggle-switch.stories.tsapps/lfx/src/app/shared/components/toggle-switch/toggle-switch.tsapps/lfx/src/app/shared/components/tooltip/tooltip-trigger.tsapps/lfx/src/app/shared/components/tooltip/tooltip.cssapps/lfx/src/app/shared/components/tooltip/tooltip.htmlapps/lfx/src/app/shared/components/tooltip/tooltip.stories.tsapps/lfx/src/app/shared/components/tooltip/tooltip.tsapps/lfx/src/index.htmlapps/lfx/src/server/helpers/url-validation.tsapps/lfx/src/server/middleware/auth.middleware.tsapps/lfx/src/server/middleware/error-handler.middleware.tsapps/lfx/src/server/server.tsapps/lfx/src/styles.cssapps/lfx/tsconfig.app.jsonpackage.jsonturbo.json
✅ Files skipped from review due to trivial changes (11)
- apps/lfx/src/app/shared/components/chip/chip.css
- apps/lfx/src/app/shared/components/tabs/tab-item.css
- apps/lfx/src/app/shared/components/badge/badge.css
- apps/lfx/src/app/shared/components/button/button.css
- apps/lfx/src/app/shared/components/card/card.css
- apps/lfx/src/app/shared/components/avatar/avatar.css
- apps/lfx/src/app/shared/components/tabs/tabs.css
- apps/lfx/src/app/shared/components/tooltip/tooltip.css
- apps/lfx/.storybook/tsconfig.doc.json
- apps/lfx/src/app/shared/components/checkbox/checkbox.css
- apps/lfx/src/app/shared/components/toggle-switch/toggle-switch.css
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/lfx/package.json
- apps/lfx/angular.json
- apps/lfx/tsconfig.app.json
- package.json
- apps/lfx/src/index.html
- .claude/skills/lfx-preflight/SKILL.md
- .claude/skills/lfx-backend-builder/SKILL.md
Signed-off-by: Asitha de Silva <asithade@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
apps/lfx/src/server/server.ts (2)
41-42:⚠️ Potential issue | 🔴 CriticalReplace bare
require()in this ESM entrypoint.Line 26 already uses
import.meta.url, so this file is running as ESM.require('compression')will fail before middleware setup.Suggested fix
+import compression from 'compression'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import pinoHttp from 'pino-http'; @@ -// eslint-disable-next-line `@typescript-eslint/no-require-imports` -const compression = require('compression'); app.use( compression({#!/bin/bash set -euo pipefail echo "server.ts mixes ESM-only and CommonJS-only syntax:" rg -n 'import\.meta\.url|require\(' apps/lfx/src/server/server.ts echo echo "Relevant module settings under apps/lfx:" fd '^tsconfig.*\.json$' apps/lfx -x sh -c ' printf "\n== %s ==\n" "$1" rg -n "\"module\"\\s*:" "$1" || true ' sh {} echo echo "Package-level module markers:" fd '^package\.json$' . -x sh -c ' printf "\n== %s ==\n" "$1" rg -n "\"type\"\\s*:\\s*\"module\"" "$1" || true ' sh {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/server.ts` around lines 41 - 42, The file mixes ESM and CommonJS by using require('compression') while the module is ESM (uses import.meta.url); replace the bare require with an ESM import by importing the compression package with ESM syntax (update the declaration of the compression symbol used in middleware setup) or, if import must be conditional, use a top-level dynamic import of 'compression' and await it before using it; update references to the compression variable (where used to register middleware) accordingly and remove the eslint disable comment around the require.
168-176:⚠️ Potential issue | 🟠 MajorKeep the token-claims fallback instead of forcing logout.
Line 163 already seeds
authContext.userfrom the session. Callingres.oidc.logout()here turns a transientfetchUserInfo()failure into a forced sign-out and aborts SSR, which defeats the stated fallback.Suggested fix
} catch (error) { // If userinfo fetch fails, fall back to basic user info from token logger.warning(req, 'ssr_user_info', 'Failed to fetch user info, using basic user data', { err: error, path: req.path, }); - - res.oidc.logout(); - return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/server.ts` around lines 168 - 176, The catch block currently forces a logout by calling res.oidc.logout() which aborts SSR and prevents the intended fallback to token-derived claims; remove the res.oidc.logout() call and the early return so the handler continues execution and uses the pre-seeded authContext.user when fetchUserInfo() fails, leaving the existing logger.warning as-is to record the error (referencing fetchUserInfo failure handling, logger.warning, and authContext.user).apps/lfx/src/server/middleware/auth.middleware.ts (1)
16-31:⚠️ Potential issue | 🟠 MajorUse segment-aware matching in
classifyRoute().Raw
startsWith()still makes/healthz,/public/apiary, and/api-docsinherit the wrong auth policy, while/meetingsmisses the optional rule because the config only matches/meetings/. Normalize trailing slashes and match exact paths or full path segments instead.As per coding guidelines, "Public routes at /meeting and /public/api endpoints bypass authentication, while protected routes require authentication".Suggested fix
function classifyRoute(path: string, config: AuthConfig): RouteAuthConfig { + const normalizedPath = path.replace(/\/+$/, '') || '/'; + for (const routeConfig of config.routes) { if (typeof routeConfig.pattern === 'string') { - if (path.startsWith(routeConfig.pattern)) { + const normalizedPattern = routeConfig.pattern.replace(/\/+$/, '') || '/'; + + if ( + normalizedPattern === '/' || + normalizedPath === normalizedPattern || + normalizedPath.startsWith(`${normalizedPattern}/`) + ) { return routeConfig; } } else if (routeConfig.pattern.test(path)) {Also applies to: 45-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/middleware/auth.middleware.ts` around lines 16 - 31, classifyRoute() currently uses naive startsWith() against DEFAULT_ROUTE_CONFIG patterns which causes '/healthz', '/public/apiary', and '/api-docs' to inherit incorrect policies and misses '/meetings' (pattern is '/meetings/'); change classifyRoute() to be segment-aware: first normalize the incoming path (remove duplicate/trailing slashes and ensure leading '/'), then for each pattern in DEFAULT_ROUTE_CONFIG treat a match as either exact equality (requestPath === patternTrimmed) or a segment prefix match (requestPath.startsWith(patternTrimmed + '/') ) so patterns like '/api', '/public/api' and '/meetings' only match full segments or exact paths; also normalize patterns (trim trailing slashes) before comparison to keep behavior consistent.
🧹 Nitpick comments (1)
apps/lfx/src/server/server.ts (1)
180-210: Flatten the SSR render path withawait.This handler is already
async; mixingawaitearlier in the function with a trailing.then().catch()makes the control flow harder to follow and splits error handling across two styles.As per coding guidelines, "Use async/await instead of .then() chains for asynchronous operations".Suggested refactor
- angularApp - .handle(req, { - auth: authContext, - providers: [ - { provide: APP_BASE_HREF, useValue: process.env['PCC_BASE_URL'] }, - { provide: REQUEST, useValue: req }, - ], - }) - .then((response) => { - if (response) { - return writeResponseToNodeResponse(response, res); - } - - return next(); - }) - .catch((error: unknown) => { - const err = error instanceof Error ? error : new Error(String(error)); - const code = error instanceof Error ? (error as Error & { code?: string }).code : undefined; - - if (code === 'NOT_FOUND') { - res.status(404).send('Not Found'); - } else if (code === 'UNAUTHORIZED') { - res.status(401).send('Unauthorized'); - } else { - logger.error(req, 'ssr_render', ssrStartTime, err, { - url: req.url, - method: req.method, - }); - res.status(500).send('Internal Server Error'); - } - }); + try { + const response = await angularApp.handle(req, { + auth: authContext, + providers: [ + { provide: APP_BASE_HREF, useValue: process.env['PCC_BASE_URL'] }, + { provide: REQUEST, useValue: req }, + ], + }); + + if (response) { + await writeResponseToNodeResponse(response, res); + return; + } + + next(); + } catch (error: unknown) { + const err = error instanceof Error ? error : new Error(String(error)); + const code = error instanceof Error ? (error as Error & { code?: string }).code : undefined; + + if (code === 'NOT_FOUND') { + res.status(404).send('Not Found'); + } else if (code === 'UNAUTHORIZED') { + res.status(401).send('Unauthorized'); + } else { + logger.error(req, 'ssr_render', ssrStartTime, err, { + url: req.url, + method: req.method, + }); + res.status(500).send('Internal Server Error'); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/server.ts` around lines 180 - 210, Convert the trailing promise chain on angularApp.handle to use await inside the existing async handler: call await angularApp.handle(req, { auth: authContext, providers: [...] }) inside a try block, check the returned response and call writeResponseToNodeResponse(response, res) if truthy or call next() otherwise, and move the current .catch() logic into the corresponding catch block preserving the error normalization, code checks for 'NOT_FOUND' and 'UNAUTHORIZED', and the logger.error call that references ssrStartTime, req.url and req.method before sending the 500 response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/lfx/src/server/middleware/error-handler.middleware.ts`:
- Around line 57-60: The middleware currently returns error.toResponse()
directly which can leak raw upstream payloads stored in ApiError.details (see
ApiError.upstream() and toResponse()); update the error handling to sanitize or
omit details before sending: call error.toResponse(), create a shallow copy,
remove or replace the details property (or filter sensitive fields) and include
only safe fields plus path, then send that sanitized object via
res.status(...).json; alternatively, update ApiError.toResponse() to return a
sanitized details value by default so callers like the middleware
(error-handler.middleware) never emit raw upstream payloads.
In `@apps/lfx/src/server/server.ts`:
- Around line 145-146: Insert an explicit middleware before the apiErrorHandler
registration that catches any unmatched API routes under /api/* and
/public/api/* and forwards a 404 error to the existing apiErrorHandler;
specifically, add a middleware (registered before app.use('/api/*',
apiErrorHandler)) that constructs a 404 Error (or uses createError(404)) and
calls next(err) so the apiErrorHandler sees the 404 instead of letting the
request fall through to the Angular SSR catch-all.
---
Duplicate comments:
In `@apps/lfx/src/server/middleware/auth.middleware.ts`:
- Around line 16-31: classifyRoute() currently uses naive startsWith() against
DEFAULT_ROUTE_CONFIG patterns which causes '/healthz', '/public/apiary', and
'/api-docs' to inherit incorrect policies and misses '/meetings' (pattern is
'/meetings/'); change classifyRoute() to be segment-aware: first normalize the
incoming path (remove duplicate/trailing slashes and ensure leading '/'), then
for each pattern in DEFAULT_ROUTE_CONFIG treat a match as either exact equality
(requestPath === patternTrimmed) or a segment prefix match
(requestPath.startsWith(patternTrimmed + '/') ) so patterns like '/api',
'/public/api' and '/meetings' only match full segments or exact paths; also
normalize patterns (trim trailing slashes) before comparison to keep behavior
consistent.
In `@apps/lfx/src/server/server.ts`:
- Around line 41-42: The file mixes ESM and CommonJS by using
require('compression') while the module is ESM (uses import.meta.url); replace
the bare require with an ESM import by importing the compression package with
ESM syntax (update the declaration of the compression symbol used in middleware
setup) or, if import must be conditional, use a top-level dynamic import of
'compression' and await it before using it; update references to the compression
variable (where used to register middleware) accordingly and remove the eslint
disable comment around the require.
- Around line 168-176: The catch block currently forces a logout by calling
res.oidc.logout() which aborts SSR and prevents the intended fallback to
token-derived claims; remove the res.oidc.logout() call and the early return so
the handler continues execution and uses the pre-seeded authContext.user when
fetchUserInfo() fails, leaving the existing logger.warning as-is to record the
error (referencing fetchUserInfo failure handling, logger.warning, and
authContext.user).
---
Nitpick comments:
In `@apps/lfx/src/server/server.ts`:
- Around line 180-210: Convert the trailing promise chain on angularApp.handle
to use await inside the existing async handler: call await
angularApp.handle(req, { auth: authContext, providers: [...] }) inside a try
block, check the returned response and call
writeResponseToNodeResponse(response, res) if truthy or call next() otherwise,
and move the current .catch() logic into the corresponding catch block
preserving the error normalization, code checks for 'NOT_FOUND' and
'UNAUTHORIZED', and the logger.error call that references ssrStartTime, req.url
and req.method before sending the 500 response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bdb0553-5efd-4e15-90e5-38e764b3569b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
apps/lfx/package.jsonapps/lfx/src/server/helpers/url-validation.tsapps/lfx/src/server/middleware/auth.middleware.tsapps/lfx/src/server/middleware/error-handler.middleware.tsapps/lfx/src/server/server.ts
✅ Files skipped from review due to trivial changes (1)
- apps/lfx/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx/src/server/helpers/url-validation.ts
- Add aria-label and aria-hidden for chip dismiss button a11y - Add .filter(Boolean) defensive guard in avatar initials - Add relative class for button loading spinner overlay - Fix card stories padding="none" demo accuracy - Rename tabs style input to tabStyle to avoid DOM collision - Fix tab-item dual focusable by moving ARIA to inner button - Remove conflicting data-testid on tabs template - Add isPlatformBrowser SSR guard for tooltip trigger - Remove logout on transient fetchUserInfo failure - Add API 404 catch-all middleware for unmatched routes - Sanitize ApiError details from 5xx responses - Use boundary-aware path matching in auth middleware - Fix MD040 fenced code blocks in skill files - Update preflight protected file path reference Signed-off-by: Asitha de Silva <asithade@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
apps/lfx/src/app/shared/components/chip/chip.html (1)
5-7: Addaria-hidden="true"to decorative icon for consistency.The chip icon is decorative since the label (line 14) conveys the semantic meaning. For accessibility consistency with the dismiss button icon (line 22), hide this icon from assistive technology.
♿ Suggested fix
`@if` (type() === 'icon' && icon()) { - <i [class]="'fa-light fa-' + icon()" data-testid="chip-icon"></i> + <i [class]="'fa-light fa-' + icon()" aria-hidden="true" data-testid="chip-icon"></i> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/chip/chip.html` around lines 5 - 7, The decorative chip icon (<i> element rendered when type() === 'icon' and icon() is truthy, identified by data-testid="chip-icon") should be hidden from assistive tech; update the template rendering in chip.html so that the <i> element includes aria-hidden="true" (matching the dismiss button's accessibility treatment) while leaving type(), icon() and the data-testid unchanged..claude/skills/lfx/SKILL.md (1)
147-147: Consider adding "please" for a more courteous tone.The clarifying question could sound slightly more polite with the addition of "please": "Could you please tell me specifically what you'd like to add or change?"
✨ Optional refinement
-- "It sounds like you want to change how committee members are displayed. Could you tell me specifically what you'd like to add or change?" +- "It sounds like you want to change how committee members are displayed. Could you please tell me specifically what you'd like to add or change?"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/lfx/SKILL.md at line 147, The sentence string "It sounds like you want to change how committee members are displayed. Could you tell me specifically what you'd like to add or change?" should be made more courteous by inserting "please" into the second sentence; update that exact string in .claude/skills/lfx/SKILL.md so it reads "Could you please tell me specifically what you'd like to add or change?" while preserving the rest of the text and punctuation.apps/lfx/src/app/shared/components/button/button.html (1)
4-10: Expose loading state to assistive tech.Consider adding
aria-busyon the<button>so screen readers can detect the busy state whenloading()is true.Suggested update
<button [type]="type()" class="focus-visible:ring-info-500 relative inline-flex items-center justify-center rounded-full font-semibold transition-colors focus-visible:ring-2 focus-visible:ring-offset-1 focus-visible:outline-none" [class]="buttonClasses()" + [attr.aria-busy]="loading()" [disabled]="isDisabled()" (click)="onClick($event)" data-testid="button-element">Also applies to: 22-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/button/button.html` around lines 4 - 10, The button lacks an accessibility indicator for its loading state—update the <button> element logic to add aria-busy when loading() is true (e.g., set attribute aria-busy="{{loading()}}" or via property binding [attr.aria-busy]="loading()") so assistive tech can detect the busy state; locate the button element that uses type(), buttonClasses(), isDisabled(), loading(), and onClick() and add the aria-busy attribute bound to loading(), ensuring it updates alongside existing disabled handling.apps/lfx/src/server/server.ts (1)
184-215: Consider refactoring.then()chain toasync/awaitfor consistency.The handler is already declared
asyncbut uses a.then()chain forangularApp.handle(). Per coding guidelines, preferasync/awaitover.then()chains for clearer control flow and error handling.♻️ Suggested refactor
- angularApp - .handle(req, { + try { + const response = await angularApp.handle(req, { auth: authContext, providers: [ { provide: APP_BASE_HREF, useValue: process.env['PCC_BASE_URL'] }, { provide: REQUEST, useValue: req }, ], - }) - .then((response) => { - if (response) { - return writeResponseToNodeResponse(response, res); - } - - return next(); - }) - .catch((error: unknown) => { - const err = error instanceof Error ? error : new Error(String(error)); - const code = error instanceof Error ? (error as Error & { code?: string }).code : undefined; - - if (code === 'NOT_FOUND') { - res.status(404).send('Not Found'); - } else if (code === 'UNAUTHORIZED') { - res.status(401).send('Unauthorized'); - } else { - logger.error(req, 'ssr_render', ssrStartTime, err, { - url: req.url, - method: req.method, - }); - res.status(500).send('Internal Server Error'); - } }); + + if (response) { + await writeResponseToNodeResponse(response, res); + } else { + next(); + } + } catch (error: unknown) { + const err = error instanceof Error ? error : new Error(String(error)); + const code = error instanceof Error ? (error as Error & { code?: string }).code : undefined; + + if (code === 'NOT_FOUND') { + res.status(404).send('Not Found'); + } else if (code === 'UNAUTHORIZED') { + res.status(401).send('Unauthorized'); + } else { + logger.error(req, 'ssr_render', ssrStartTime, err, { + url: req.url, + method: req.method, + }); + res.status(500).send('Internal Server Error'); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/server/server.ts` around lines 184 - 215, The current async handler uses a .then()/.catch() chain on angularApp.handle; change this to use await inside the existing async function: await angularApp.handle(...) and capture its return in a variable, then call writeResponseToNodeResponse(response, res) if truthy or call next() otherwise; wrap the await in a try/catch and in the catch reproduce the existing error normalization and status branching (check error.code for 'NOT_FOUND' and 'UNAUTHORIZED', call logger.error(req, 'ssr_render', ssrStartTime, err, { url: req.url, method: req.method }) for other errors and respond 500), keeping references to angularApp.handle, writeResponseToNodeResponse, logger.error, ssrStartTime, req, res, and next.apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts (2)
42-79: Consider adding ARIA attributes for screen reader accessibility.The tooltip is not programmatically associated with its trigger element. Screen reader users won't have the tooltip content announced when focusing the trigger.
For accessible tooltips, the trigger should have
aria-describedbypointing to the tooltip's ID.♿ Suggested accessibility enhancement
public show(): void { if (!isPlatformBrowser(this.platformId) || this.tooltipRef) { return; } this.tooltipRef = this.vcr.createComponent(Tooltip); this.tooltipRef.setInput('label', this.lfxTooltip()); const description = this.lfxTooltipDescription(); if (description) { this.tooltipRef.setInput('description', description); } this.tooltipRef.changeDetectorRef.detectChanges(); const tooltipEl = this.tooltipRef.location.nativeElement as HTMLElement; + const tooltipId = `tooltip-${Math.random().toString(36).slice(2, 9)}`; + this.renderer.setAttribute(tooltipEl, 'id', tooltipId); + this.renderer.setAttribute(tooltipEl, 'role', 'tooltip'); + this.renderer.setAttribute(this.el.nativeElement, 'aria-describedby', tooltipId); this.tooltipWrapper = this.renderer.createElement('div') as HTMLElement;And in
hide():public hide(): void { + if (isPlatformBrowser(this.platformId)) { + this.renderer.removeAttribute(this.el.nativeElement, 'aria-describedby'); + } + if (this.tooltipWrapper) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts` around lines 42 - 79, The tooltip created in show() (via this.tooltipRef and this.tooltipWrapper) is not associated with its trigger for screen readers; generate a stable unique id for the tooltip when creating it (e.g., tooltip-<unique>), set that id on the tooltip DOM node (tooltipRef.location.nativeElement or tooltipWrapper), and set aria-describedby on the trigger element (the host/trigger element used by this component) to that id; also ensure hide() clears the aria-describedby attribute and removes the id when the tooltip is destroyed (update show(), hide(), and the code paths around tooltipRef/tooltipWrapper, createArrow(), and positionTooltip() accordingly).
109-115: Hardcoded color value could use a design token.The arrow color
#0f172ais hardcoded. Since this PR integrates Tailwind CSS v4 with@themedesign tokens, consider using a CSS variable for consistency and easier theming.🎨 Example using CSS variable
if (position === 'top') { this.renderer.setStyle(arrow, 'bottom', `-${ARROW_SIZE}px`); - this.renderer.setStyle(arrow, 'border-top', `${ARROW_SIZE}px solid `#0f172a``); + this.renderer.setStyle(arrow, 'border-top', `${ARROW_SIZE}px solid var(--color-tooltip-bg, `#0f172a`)`); } else { this.renderer.setStyle(arrow, 'top', `-${ARROW_SIZE}px`); - this.renderer.setStyle(arrow, 'border-bottom', `${ARROW_SIZE}px solid `#0f172a``); + this.renderer.setStyle(arrow, 'border-bottom', `${ARROW_SIZE}px solid var(--color-tooltip-bg, `#0f172a`)`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts` around lines 109 - 115, Replace the hardcoded arrow color (`#0f172a`) with a design-token CSS variable so theming works consistently: in tooltip-trigger.ts where the arrow element is styled (the block that uses ARROW_SIZE and the local variable `arrow`), set the border color portion to use a CSS variable (e.g. var(--tooltip-bg, `#0f172a`)) instead of the literal hex for both the `border-top` and `border-bottom` styles; ensure you include a sensible fallback value in the var() so existing appearance is preserved if the token is not defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/lfx/SKILL.md:
- Line 96: Replace the hardcoded developer path in the Agent invocation examples
with a clear placeholder: find the prompt examples containing the string
starting "Invoke /lfx-coordinator to add a bio text field to committee members.
Domain: committees. Scope: field addition. Repo:
/Users/asithadesilva/Sites/lfx-v2-ui..." and substitute the path value with a
placeholder like "<repo-path>" or "$REPO_PATH"; apply the same replacement for
the other similar examples referenced (the lines with identical Agent invocation
prompts) so all occurrences use the placeholder instead of the absolute path.
In `@apps/lfx/src/app/shared/components/button/button.html`:
- Around line 22-25: The spinner wrapper currently only has "absolute" and can
float off-center; update the span surrounding <lfx-spinner> (the element
rendered when loading() is true) to occupy the button overlay and center its
content by giving it full inset positioning and centering utilities (e.g., make
the span use inset-0 and flex with items-center and justify-center or equivalent
classes) so the <lfx-spinner> is horizontally and vertically centered over the
button.
In `@apps/lfx/src/server/middleware/auth.middleware.ts`:
- Line 24: The route pattern '/meetings/' currently won't match '/meetings' due
to the boundary-aware matcher referenced near line 51; update the route entry
that currently uses the literal '/meetings/' pattern to either use '/meetings'
(no trailing slash) or include both variants so that requests to '/meetings' and
'/meetings/' match the intended auth rule; locate the array/object that defines
the pattern (the entry with pattern: '/meetings/') in auth.middleware.ts and
replace or augment that pattern to ensure both trailing- and non-trailing-slash
requests are covered so they don't fall through to the '/' catch-all with auth:
'required'.
In `@apps/lfx/src/server/server.ts`:
- Around line 229-234: The JSDoc for startServer incorrectly states the default
port is 4200 while the code sets const port = process.env['PORT'] || 4201; —
make them consistent by updating the JSDoc comment to state the default is 4201
(or alternatively change the fallback value to 4200 if 4200 is the intended
default); locate the startServer function and either adjust the comment text or
change the fallback number so the JSDoc and the code agree.
- Around line 121-139: The validateAndSanitizeUrl call in the /login handler
uses process.env['PCC_BASE_URL'] cast to string which can be undefined at
runtime; update both calls that pass allowed domains to validateAndSanitizeUrl
(lines around the /login middleware) to provide a safe fallback value when
PCC_BASE_URL is missing (e.g., use a default local URL or a hardcoded allowed
domain) so the array passed to validateAndSanitizeUrl is never [undefined];
ensure both branches (the res.redirect path and the res.oidc.login path) use the
same fallback logic referencing process.env['PCC_BASE_URL'] and
validateAndSanitizeUrl to avoid silent validation failure.
---
Nitpick comments:
In @.claude/skills/lfx/SKILL.md:
- Line 147: The sentence string "It sounds like you want to change how committee
members are displayed. Could you tell me specifically what you'd like to add or
change?" should be made more courteous by inserting "please" into the second
sentence; update that exact string in .claude/skills/lfx/SKILL.md so it reads
"Could you please tell me specifically what you'd like to add or change?" while
preserving the rest of the text and punctuation.
In `@apps/lfx/src/app/shared/components/button/button.html`:
- Around line 4-10: The button lacks an accessibility indicator for its loading
state—update the <button> element logic to add aria-busy when loading() is true
(e.g., set attribute aria-busy="{{loading()}}" or via property binding
[attr.aria-busy]="loading()") so assistive tech can detect the busy state;
locate the button element that uses type(), buttonClasses(), isDisabled(),
loading(), and onClick() and add the aria-busy attribute bound to loading(),
ensuring it updates alongside existing disabled handling.
In `@apps/lfx/src/app/shared/components/chip/chip.html`:
- Around line 5-7: The decorative chip icon (<i> element rendered when type()
=== 'icon' and icon() is truthy, identified by data-testid="chip-icon") should
be hidden from assistive tech; update the template rendering in chip.html so
that the <i> element includes aria-hidden="true" (matching the dismiss button's
accessibility treatment) while leaving type(), icon() and the data-testid
unchanged.
In `@apps/lfx/src/app/shared/components/tooltip/tooltip-trigger.ts`:
- Around line 42-79: The tooltip created in show() (via this.tooltipRef and
this.tooltipWrapper) is not associated with its trigger for screen readers;
generate a stable unique id for the tooltip when creating it (e.g.,
tooltip-<unique>), set that id on the tooltip DOM node
(tooltipRef.location.nativeElement or tooltipWrapper), and set aria-describedby
on the trigger element (the host/trigger element used by this component) to that
id; also ensure hide() clears the aria-describedby attribute and removes the id
when the tooltip is destroyed (update show(), hide(), and the code paths around
tooltipRef/tooltipWrapper, createArrow(), and positionTooltip() accordingly).
- Around line 109-115: Replace the hardcoded arrow color (`#0f172a`) with a
design-token CSS variable so theming works consistently: in tooltip-trigger.ts
where the arrow element is styled (the block that uses ARROW_SIZE and the local
variable `arrow`), set the border color portion to use a CSS variable (e.g.
var(--tooltip-bg, `#0f172a`)) instead of the literal hex for both the `border-top`
and `border-bottom` styles; ensure you include a sensible fallback value in the
var() so existing appearance is preserved if the token is not defined.
In `@apps/lfx/src/server/server.ts`:
- Around line 184-215: The current async handler uses a .then()/.catch() chain
on angularApp.handle; change this to use await inside the existing async
function: await angularApp.handle(...) and capture its return in a variable,
then call writeResponseToNodeResponse(response, res) if truthy or call next()
otherwise; wrap the await in a try/catch and in the catch reproduce the existing
error normalization and status branching (check error.code for 'NOT_FOUND' and
'UNAUTHORIZED', call logger.error(req, 'ssr_render', ssrStartTime, err, { url:
req.url, method: req.method }) for other errors and respond 500), keeping
references to angularApp.handle, writeResponseToNodeResponse, logger.error,
ssrStartTime, req, res, and next.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b5c0133-2b2d-409c-86bc-5e4bad4a56bf
📒 Files selected for processing (16)
.claude/skills/lfx-design/SKILL.md.claude/skills/lfx-preflight/SKILL.md.claude/skills/lfx/SKILL.mdapps/lfx/src/app/shared/components/avatar/avatar.tsapps/lfx/src/app/shared/components/button/button.htmlapps/lfx/src/app/shared/components/card/card.stories.tsapps/lfx/src/app/shared/components/chip/chip.htmlapps/lfx/src/app/shared/components/tabs/tab-item.htmlapps/lfx/src/app/shared/components/tabs/tab-item.tsapps/lfx/src/app/shared/components/tabs/tabs.htmlapps/lfx/src/app/shared/components/tabs/tabs.stories.tsapps/lfx/src/app/shared/components/tabs/tabs.tsapps/lfx/src/app/shared/components/tooltip/tooltip-trigger.tsapps/lfx/src/server/middleware/auth.middleware.tsapps/lfx/src/server/middleware/error-handler.middleware.tsapps/lfx/src/server/server.ts
✅ Files skipped from review due to trivial changes (2)
- .claude/skills/lfx-preflight/SKILL.md
- .claude/skills/lfx-design/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/lfx/src/server/middleware/error-handler.middleware.ts
- apps/lfx/src/app/shared/components/tabs/tabs.ts
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
apps/lfxAngular 20 SSR application with stable zoneless change detectionLfxService(flat fetch client) replacing MicroserviceProxyService + ApiClientServiceApiErrorclass with factory methods replacing 5-class error hierarchyexpress-openid-connect@themedesign tokens (no PrimeNG)apps/lfx-oneto prevent duplicate dependency hoistingGenerated with Claude Code