feat(api): integrate NATS for project slug-to-ID resolution #53
Conversation
Remove Claude Code subagent system configuration files and simplify CLAUDE.md documentation to reduce complexity and maintenance overhead. - Delete .claude/agents/angular-ui-expert.md - Delete .claude/agents/jira-project-manager.md - Remove subagent system section from CLAUDE.md - Simplify commit workflow documentation LFXV2-334 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
- Rename meeting-create component to meeting-manage for both create/edit functionality - Add confirmation dialog for attachment deletion with proper refresh mechanism - Fix attachment saving in edit mode to properly add new attachments to existing meetings - Improve error handling: replace forkJoin all-or-nothing with individual attachment error handling - Add loading state signal for attachment deletion progress tracking - Extract reusable code to shared package for better maintainability: - Move constants (TOTAL_STEPS, DEFAULT_DURATION, etc.) to shared/constants/meeting.ts - Create validators directory with meeting-specific form validators - Add date/time utilities (combineDateTime, formatTo12Hour, etc.) to shared/utils - Refactor component to use extracted utilities, reducing code duplication by 700+ lines - Add Angular forms as peer dependency to shared package for validators - Update routing to support both /meetings/create and /meetings/:id/edit paths - Implement loading spinner for delete attachment button Addresses LFXV2-286: Meeting create/edit functionality consolidation Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
Signed-off-by: Asitha de Silva <[email protected]>
- Fix critical bug in MS_IN_DAY constant calculation - Was incorrectly calculating milliseconds in a week (604,800,000) - Now correctly calculates milliseconds in a day (86,400,000) - Removed erroneous DAYS_IN_WEEK multiplication - Add comprehensive JSDoc documentation to all meeting constants - Time calculation constants with mathematical explanations - Meeting configuration defaults with usage examples - Form validation and navigation constants - Access control and recurrence mapping enums - Organize constants into logical sections with clear headers - Validate all time arithmetic is mathematically correct - Ensure date operations (like week addition) now work properly This fixes potential date calculation bugs in meeting recurrence logic. Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
- Standardize file naming with consistent suffixes (.constants, .interface, .enum, .validators, .utils) - Add comprehensive JSDoc documentation to all constants, enums, and interfaces - Fix critical MS_IN_DAY calculation bug (was calculating weeks instead of days) - Consolidate date and timezone utilities into date-time.utils.ts - Update all import/export statements to maintain compatibility - Improve code maintainability with self-documenting interfaces Generated with [Claude Code](https://claude.ai/code) LFXV2-286 Signed-off-by: Asitha de Silva <[email protected]>
- Add date-fns-tz dependency for proper timezone support - Update combineDateTime to accept timezone parameter and use fromZonedTime for UTC conversion - Replace brittle toLocaleString/new Date parsing with timezone-aware utility functions - Add timezone utility functions: compareDateTimesInTimezone, getCurrentTimeInTimezone, isDateTimeInFutureForTimezone - Update futureDateTimeValidator to use proper timezone handling - Remove duplicate validator and utility functions from meeting form component - Ensure meeting scheduling works correctly across different timezones LFXV2-286 Signed-off-by: Asitha de Silva <[email protected]>
- Replace ad-hoc custom duration validation with shared customDurationValidator - Add explicit time format validation using timeFormatValidator - Add topic and agenda content validators for better input validation - Add runtime safety check to prevent NaN duration from bypassing validation - Ensure consistent validation patterns across all meeting form components - Fix dependency management by removing duplicates from app package.json LFXV2-286 Signed-off-by: Asitha de Silva <[email protected]>
- Add NATS.js SDK dependency for cluster messaging - Implement lazy-loading NATS service with request/reply pattern - Create consolidated getProjectBySlug method using NATS resolution - Add NATS interfaces and configuration for lfx.projects-api.slug_to_uid subject - Update project routes to use NATS for slug resolution - Configure cluster DNS connection to lfx-platform-nats.lfx.svc.cluster.local:4222 - Add graceful shutdown handling for NATS connections Resolves LFXV2-337 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Asitha de Silva <[email protected]>
github-actions
bot
requested review from
andrest50,
audigregorie,
dealako and
mauriciozanettisalomao
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughIntroduces NATS-based slug-to-UID resolution and refactors project routes to a controller/service pattern. Adds NATS config/env and dependency. Updates Supabase and frontend to use project UID for recent activity. Adjusts committee wiring to internalize dependencies. Adds e2e API mocks and test-ids. Minor editor settings update. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Frontend
participant API as ProjectController
participant Svc as ProjectService
participant NATS as NatsService
participant Broker as NATS
participant LFX as MicroserviceProxy (LFX_V2 /query/resources)
User->>UI: Navigate to /projects/:slug
UI->>API: GET /api/projects/:slug
API->>Svc: getProjectBySlug(slug)
Svc->>NATS: getProjectIdBySlug(slug)
NATS->>Broker: request PROJECT_SLUG_TO_UID(slug)
Broker-->>NATS: { projectId, exists }
NATS-->>Svc: projectId or not found
alt slug resolved
Svc->>LFX: GET /query/resources?type=project&tags=projectId
LFX-->>Svc: resource list
Svc-->>API: Project
API-->>UI: 200 JSON(Project)
else not found/timeout
Svc-->>API: throw 404 PROJECT_NOT_FOUND
API-->>UI: 404 JSON(error)
end
sequenceDiagram
autonumber
actor User
participant UI as Frontend (ActivityService)
participant API as Projects Route
participant SB as SupabaseService
User->>UI: View Project Dashboard
UI->>API: GET /api/projects/:uid/recent-activity?limit=10
API->>SB: getRecentActivityByProjectUid(uid, limit)
SB-->>API: RecentActivity[]
API-->>UI: 200 JSON(RecentActivity[])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (8)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
Integrates NATS messaging system to enable project slug-to-ID resolution within the Kubernetes cluster environment, replacing the previous Supabase-based slug resolution approach.
- Added NATS service with lazy connection for project slug-to-ID resolution
- Consolidated duplicate slug methods into a unified project service layer
- Implemented request/reply pattern for cluster DNS communication without port-forwarding
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/nats.interface.ts | Defines NATS interfaces and subjects for project slug resolution |
| packages/shared/src/interfaces/index.ts | Exports NATS interfaces to shared package |
| apps/lfx-pcc/src/server/services/nats.service.ts | New NATS service implementing lazy connection and slug-to-ID resolution |
| apps/lfx-pcc/src/server/services/project.service.ts | New consolidated project service with NATS-based slug resolution |
| apps/lfx-pcc/src/server/services/supabase.service.ts | Updates method signature from slug-based to ID-based lookup |
| apps/lfx-pcc/src/server/routes/projects.ts | Refactors routes to use new project service layer |
| apps/lfx-pcc/package.json | Adds NATS.js dependency |
| apps/lfx-pcc/.env.example | Adds NATS configuration example |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
- Extract hardcoded NATS URL to configuration constants in nats.config.ts - Implement thread-safe connection management with promise-based synchronization - Fix NatsService instantiation by using dependency injection instead of per-request instances - Add proper connection pooling and prevent race conditions during concurrent requests - Use configuration constants for timeouts and server URLs Performance improvements: - Reuse single NatsService instance across all slug lookups - Prevent multiple concurrent connection attempts with connectionPromise - Centralized configuration management for better maintainability Resolves architecture feedback on LFXV2-337 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Asitha de Silva <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
apps/lfx-pcc/.env.example (1)
24-27: Add production-ready NATS options (auth/TLS/timeout) placeholdersConsider expanding the example with commented vars for secure prod setups and operational tuning. Helps avoid committing ad‑hoc secrets and clarifies contract with NatsService.
# NATS Configuration # Internal k8s service DNS for NATS cluster NATS_URL=nats://lfx-platform-nats.lfx.svc.cluster.local:4222 + +# Optional: Auth/TLS (uncomment in non-dev environments) +# NATS_USER=your-user +# NATS_PASSWORD=your-password +# NATS_NKEY_SEED=SUxxxx... # Prefer NKeys over user/pass when possible +# NATS_TLS_CA=/etc/ssl/certs/ca.crt +# NATS_TLS_CERT=/etc/ssl/certs/tls.crt +# NATS_TLS_KEY=/etc/ssl/private/tls.key + +# Optional: Client behavior +# NATS_REQUEST_TIMEOUT_MS=1500 +# NATS_RECONNECT_TIME_WAIT_MS=2000 +# NATS_MAX_RECONNECT_ATTEMPTS=120 +# NATS_NAME=lfx-pccpackages/shared/src/interfaces/nats.interface.ts (2)
7-11: Tighten response typing to reflect “not found” casesWhen exists is false, projectId can’t be a meaningful ID. Encode that in the type to prevent accidental usage.
export interface ProjectSlugToIdResponse { - projectId: string; + // When exists is false, projectId will be null + projectId: string | null; slug: string; exists: boolean; }Optionally, define an explicit request payload for completeness:
// Add near the response interface export interface ProjectSlugToIdRequest { slug: string; }
16-18: Minor naming consistency: Id vs UidEnum uses slug_to_uid while the interface is ProjectSlugToIdResponse. Consider aligning on “Uid” or “Id” across both to reduce cognitive load. Not a blocker given the subject name is dictated by the upstream service.
apps/lfx-pcc/src/server/services/supabase.service.ts (1)
83-86: Parameter naming: prefer projectUid to match query keyThe method targets the uid column; naming the param accordingly improves readability without changing the API shape.
- public async getProjectById(id: string) { + public async getProjectById(projectUid: string) { const params = { - uid: `eq.${id}`, + uid: `eq.${projectUid}`, limit: '1', };apps/lfx-pcc/src/server/routes/projects.ts (3)
147-149: Good refactor; add safeguard on logging project_uidThe refactor to use getProjectBySlug() looks good. Minor: if typings allow, ensure project.uid is always present to avoid undefined in logs. If not guaranteed by type, guard the log field.
Apply this diff:
- project_uid: project.uid, + project_uid: project?.uid,Also applies to: 154-159
208-211: Include slug in recent-activity error logs for better traceabilityThe success log includes project_uid, but the error path lacks the slug context. Add slug to error logs here for parity with the /:slug route.
Apply this diff:
- const project = await projectService.getProjectBySlug(req, projectSlug); + const project = await projectService.getProjectBySlug(req, projectSlug);- { + { error: error instanceof Error ? error.message : error, operation: 'fetch_project_recent_activity', + slug: projectSlug, duration, },Also applies to: 229-235
17-57: Optional: centralize request-timing/log patternMultiple handlers duplicate the same startTime/duration pattern. Consider small helper/middleware for timing to reduce repetition and potential drift.
Also applies to: 59-116, 118-176, 178-238
apps/lfx-pcc/src/server/services/project.service.ts (3)
96-103: Distinguish “not found” from transient NATS errorsToday, NatsService maps timeouts/no-responders to exists: false, which becomes a 404 here. Consider surfacing transient conditions (timeout, no responders) as 503 Service Unavailable to avoid misdiagnosing outages as missing resources.
If you adopt this, either:
- make NatsService throw typed errors for transient conditions and catch/translate here, or
- extend ProjectSlugToIdResponse to include a reason flag (e.g., reason: 'NOT_FOUND' | 'TIMEOUT' | 'NO_RESPONDERS') and branch status accordingly.
26-29: Optional: factor out common query->resources mappinggetProjects and searchProjects share identical proxy + map code. Consider a private helper to reduce duplication and centralize error handling.
Also applies to: 73-76
67-72: Sanitize/limit search input to prevent expensive scansTrim searchQuery and consider enforcing a reasonable max length (e.g., 256), possibly reject empty-after-trim. This guards backend from heavy or accidental broad scans.
Apply this diff:
- public async searchProjects(req: Request, searchQuery: string): Promise<Project[]> { + public async searchProjects(req: Request, searchQuery: string): Promise<Project[]> { + const q = (searchQuery ?? '').trim(); + if (!q) return []; const params = { type: 'project', - name: searchQuery, + name: q, };apps/lfx-pcc/src/server/services/nats.service.ts (3)
60-62: isConnected() can be simplified and null-safeMinor polish for readability.
Apply this diff:
- public isConnected(): boolean { - return this.connection !== null && !this.connection.isClosed(); - } + public isConnected(): boolean { + return !!this.connection && !this.connection.isClosed(); + }
67-79: Consider closing after drain and integrating with process signalsDrain gracefully flushes and closes; you might also call close() as a fallback in finally. Also, ensure the app calls shutdown() on SIGINT/SIGTERM.
Example outside this file (in server bootstrap):
const onShutdown = async () => { await natsService.shutdown().catch(() => {}); process.exit(0); }; process.on('SIGINT', onShutdown); process.on('SIGTERM', onShutdown);
89-99: Security: add TLS support via env to avoid plaintext in productionDefaulting to nats:// is fine for dev, but expose a way to enable TLS (e.g., NATS_URL starting with tls:// or providing tls options).
Example (outside diff scope):
const useTls = natsUrl.startsWith('tls://'); this.connectingPromise = connect({ servers: [natsUrl], ...(useTls ? { tls: {} } : {}), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
apps/lfx-pcc/.env.example(1 hunks)apps/lfx-pcc/package.json(1 hunks)apps/lfx-pcc/src/server/routes/projects.ts(7 hunks)apps/lfx-pcc/src/server/services/nats.service.ts(1 hunks)apps/lfx-pcc/src/server/services/project.service.ts(1 hunks)apps/lfx-pcc/src/server/services/supabase.service.ts(2 hunks)packages/shared/src/interfaces/index.ts(1 hunks)packages/shared/src/interfaces/nats.interface.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,html,scss,css,js,mjs,cjs}
📄 CodeRabbit inference engine (CLAUDE.md)
License headers are required on all source files; run ./check-headers.sh to verify
Files:
packages/shared/src/interfaces/nats.interface.tspackages/shared/src/interfaces/index.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/nats.service.tsapps/lfx-pcc/src/server/routes/projects.tsapps/lfx-pcc/src/server/services/supabase.service.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not nest ternary expressions
Prefer TypeScript interfaces over union types for maintainability
Files:
packages/shared/src/interfaces/nats.interface.tspackages/shared/src/interfaces/index.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/nats.service.tsapps/lfx-pcc/src/server/routes/projects.tsapps/lfx-pcc/src/server/services/supabase.service.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all shared interfaces in the shared package under src/interfaces
Files:
packages/shared/src/interfaces/nats.interface.tspackages/shared/src/interfaces/index.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Always use direct imports for Angular standalone components; do not use barrel exports
Files:
apps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/nats.service.tsapps/lfx-pcc/src/server/routes/projects.tsapps/lfx-pcc/src/server/services/supabase.service.ts
🧠 Learnings (1)
📚 Learning: 2025-08-18T23:33:26.258Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-18T23:33:26.258Z
Learning: Applies to packages/shared/src/interfaces/**/*.ts : Place all shared interfaces in the shared package under src/interfaces
Applied to files:
packages/shared/src/interfaces/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (12)
apps/lfx-pcc/package.json (1)
42-42: NATS imports are server-only — LGTMVerified via ripgrep that the only
natsimport lives inapps/lfx-pcc/src/server/services/nats.service.ts, with no occurrences in browser-targeted code. No further action needed.packages/shared/src/interfaces/nats.interface.ts (1)
1-2: License header present—LGTMapps/lfx-pcc/src/server/services/supabase.service.ts (1)
27-28: Import reordering and usage—LGTMcreateApiError/createHttpError are used later (parseSupabaseError), and dotenv.config() is still called early.
packages/shared/src/interfaces/index.ts (1)
42-45: Barrel export for NATS interfaces—LGTMKeeps shared types centralized and discoverable.
apps/lfx-pcc/src/server/routes/projects.ts (3)
29-29: LGTM: Delegation to ProjectService.getProjects()Route cleanly delegates to the service; logging and error handling are consistent.
89-89: LGTM: Delegation to ProjectService.searchProjects()Input validation for q is present; result logging looks good.
1-2: License header presentHeaders meet repository policy.
apps/lfx-pcc/src/server/services/project.service.ts (3)
42-49: Good 404 shape and logging for unknown project IDError construction with service code and explicit 404 is clear; warning on multi-hit case below is also helpful.
1-2: License header presentMatches repository requirements.
1-118: No residual slug-based Supabase calls detectedI ran a repo-wide search for legacy slug lookups (
slug_to_uid, direct/slugqueries) and only found the new NATS-based resolution paths and related interfaces. All Supabase service calls (e.g.supabaseService.getRecentActivityByProject) now useproject.uid, not slugs.• NATS subjects enum (
PROJECT_SLUG_TO_UID) inpackages/shared/src/interfaces/nats.interface.ts
•ProjectService.getProjectBySlugand its two usages inapps/lfx-pcc/src/server/routes/projects.ts(both correctly invoke NATS first)
• No occurrences ofslug_to_uidor direct/sluglookups in Supabase callsEverything looks clean—no further changes needed.
apps/lfx-pcc/src/server/services/nats.service.ts (2)
24-41: Trim-and-validate response and normalize loggingGood handling of blank IDs and trimming. Consider logging the connection status in debug when resolving to aid support.
1-2: License header presentCompliant with repository policy.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/lfx-pcc/src/server/services/nats.service.ts (1)
86-111: Thread-safe lazy connection: LGTM and addresses earlier race concerns.
connectionPromisegate avoids duplicate connections under load. Nice.
🧹 Nitpick comments (16)
apps/lfx-pcc/package.json (1)
42-42: Add and Pin the NATS Dependency Thoughtfully
- Confirmed that
[email protected]is the currentlatestdist-tag on npm and is licensed under Apache-2.0 .- To limit upgrades to patch releases only, you can switch to tilde pinning (
"nats": "~2.29.3"); if you’d rather allow non-breaking minor updates, keep the caret ("nats": "^2.29.3") and rely on your CI pipeline (e.g.,npm audit) to catch any regressions.- As a final step, run
npm audit(or consult the GitHub Security Advisory or Snyk) fornatsto ensure there are no open vulnerabilities before merging.apps/lfx-pcc/.env.example (1)
24-27: Expose optional NATS timeouts and auth knobs (commented) for opsGood to include NATS_URL. Consider adding commented overrides for timeouts and credentials to make cluster tuning and secured deployments easier. Also clarify TLS vs non-TLS.
Apply this diff:
# NATS Configuration # Internal k8s service DNS for NATS cluster NATS_URL=nats://lfx-platform-nats.lfx.svc.cluster.local:4222 + +# Optional: override defaults from NATS_CONFIG (milliseconds) +# NATS_CONNECTION_TIMEOUT_MS=5000 +# NATS_REQUEST_TIMEOUT_MS=5000 + +# Optional: authentication (enable in NatsService/config before use) +# NATS_USER= +# NATS_PASSWORD= +# NATS_NKEY_SEED= # e.g., "SU..."; prefer NKEY/JWT over user/pass in production + +# Note: use tls:// for TLS-enabled clusters and configure certs accordingly +# e.g., NATS_URL=tls://lfx-platform-nats.lfx.svc.cluster.local:4222packages/shared/src/interfaces/nats.interface.ts (1)
1-18: Align response naming with existing UID conventions and future-proof payloadInterface and enum look good. To reduce cognitive load across code where we use “uid” consistently (e.g., Supabase queries), consider renaming
projectId→projectUid. Optionally include an optionalcorrelationIdto help trace request/reply flows.Apply this diff:
export interface ProjectSlugToIdResponse { - projectId: string; + projectUid: string; slug: string; exists: boolean; + // Optional correlation identifier for tracing request/reply + correlationId?: string; }Follow-up: if you adopt this, I can generate a codemod or PR-wide patch to update all references in
NatsServiceandProjectService. Want me to proceed?apps/lfx-pcc/src/server/services/supabase.service.ts (1)
105-109: Count lookups can fail the whole request; align with list behaviorIn
getProjectsyou default counts to 0 on failures. Here, a transient failure in either HEAD request throws and prevents returning the project. Consider mirroring the resilient pattern.Apply this diff:
- const [committeeCount, meetingCount] = await Promise.all([this.getCommitteeCountByProjectId(project.uid), this.getMeetingCountByProjectId(project.uid)]); + const [committeeCount, meetingCount] = await Promise.all([ + this.getCommitteeCountByProjectId(project.uid).catch(() => 0), + this.getMeetingCountByProjectId(project.uid).catch(() => 0), + ]);apps/lfx-pcc/src/server/config/nats.config.ts (1)
11-21: Consider making timeouts environment-overridable (non-breaking default).Small QoL improvement for ops without touching code; defaults remain 5000ms.
Apply this diff if you want env overrides:
export const NATS_CONFIG = { /** * Default NATS server URL for Kubernetes cluster */ DEFAULT_SERVER_URL: 'nats://lfx-platform-nats.lfx.svc.cluster.local:4222', /** * Connection timeout in milliseconds */ - CONNECTION_TIMEOUT: 5000, + CONNECTION_TIMEOUT: Number.parseInt(process.env['NATS_CONNECTION_TIMEOUT_MS'] || '5000', 10), /** * Request timeout in milliseconds */ - REQUEST_TIMEOUT: 5000, + REQUEST_TIMEOUT: Number.parseInt(process.env['NATS_REQUEST_TIMEOUT_MS'] || '5000', 10), } as const;apps/lfx-pcc/src/server/routes/projects.ts (5)
16-18: Good DI at module scope; ensure graceful shutdown wires NATS drain.Instantiating NatsService once is correct for reuse. Make sure the server bootstrap calls
natsService.shutdown()on SIGINT/SIGTERM to avoid dropped in-flight requests.Do you want me to add a minimal shutdown hook in the server entrypoint?
74-90: Treat whitespace-only queries as invalid.
q=' 'passes current checks. Trim and validate min length to avoid pointless downstream requests.- if (!q || typeof q !== 'string') { + if (!q || typeof q !== 'string' || q.trim().length === 0) {
133-147: Redundant param check for/:slugroute.Express won't hit this handler without
slug. Safe to keep, but it’s unreachable in practice.
185-191: Log the actual slug for recent-activity for parity with other endpoints.Elsewhere you log
slug. Consistency helps debugging/correlation.- has_project_slug: !!projectSlug, + slug: projectSlug,
210-214: Recent-activity: sanitize/pin query params (optional).If
req.queryflows to Supabase unrestricted, consider whitelisting supported params (e.g.,limit,since) to avoid accidental fan-out or unexpected filters.I can add a small helper to pick/validate keys if you’d like.
apps/lfx-pcc/src/server/services/nats.service.ts (3)
21-35: Normalize slug before request (trim/lowercase) to reduce 404s due to case/whitespace drift.If the responder is case-insensitive, normalizing here prevents cache misses and inconsistent lookups.
Is slug resolution case-sensitive in
lfx.projects-api.slug_to_uid? If not, we should normalize.- public async getProjectIdBySlug(slug: string): Promise<ProjectSlugToIdResponse> { + public async getProjectIdBySlug(slug: string): Promise<ProjectSlugToIdResponse> { + const normalizedSlug = slug.trim().toLowerCase(); const connection = await this.ensureConnection(); try { - const response = await connection.request(NatsSubjects.PROJECT_SLUG_TO_UID, this.codec.encode(slug), { timeout: NATS_CONFIG.REQUEST_TIMEOUT }); + const response = await connection.request( + NatsSubjects.PROJECT_SLUG_TO_UID, + this.codec.encode(normalizedSlug), + { timeout: NATS_CONFIG.REQUEST_TIMEOUT } + );
116-129: Harden connection options and observability.Add a client name and reconnection policy for better tracing and resilience.
- const connection = await connect({ - servers: [natsUrl], - timeout: NATS_CONFIG.CONNECTION_TIMEOUT, - }); + const connection = await connect({ + servers: [natsUrl], + timeout: NATS_CONFIG.CONNECTION_TIMEOUT, + name: process.env['NATS_CLIENT_NAME'] || `lfx-pcc:${process.env['HOSTNAME'] || 'unknown'}`, + maxReconnectAttempts: -1, // keep trying + reconnectTimeWait: 2000, // 2s backoff + pingInterval: 20000, // keep-alive + });If desired, we can also log
for await (const s of connection.status())events.
69-81: Shutdown: clear any in-flight promise and force-close on drain failure.Edge case: if shutdown races with a new connect, ensure future calls don’t await a stale promise.
public async shutdown(): Promise<void> { if (this.connection && !this.connection.isClosed()) { serverLogger.info('Shutting down NATS connection'); try { await this.connection.drain(); serverLogger.info('NATS connection closed successfully'); } catch (error) { serverLogger.error({ error: error instanceof Error ? error.message : error }, 'Error during NATS shutdown'); + try { + await this.connection.close(); + } catch (_) {/* noop */} } } - this.connection = null; + this.connection = null; + this.connectionPromise = null; }apps/lfx-pcc/src/server/services/project.service.ts (3)
43-65: 404 handling is correct; alignserviceidentifier for consistency.You use
'lfx-v2'here and'nats'elsewhere. Consider standardizing service identifiers for logs/errors (e.g.,'lfx-v2-service').- service: 'lfx-v2', + service: 'lfx-v2-service',
70-79: Search: trim and validate the query to reduce noisy backend calls.Mirror the router’s stricter validation inside the service layer for defense in depth.
- public async searchProjects(req: Request, searchQuery: string): Promise<Project[]> { + public async searchProjects(req: Request, searchQuery: string): Promise<Project[]> { + const q = searchQuery.trim(); + if (!q) { + return []; + } const params = { type: 'project', - name: searchQuery, + name: q, };
85-118: Slug flow is clean; consider adding the slug to the 404 for better UX/observability.Returning which slug wasn’t found helps both users and logs.
- throw createApiError({ - message: 'Project not found', + throw createApiError({ + message: `Project not found for slug "${projectSlug}"`, status: 404, code: 'PROJECT_NOT_FOUND', service: 'nats', });Also, optional: small in-memory cache for slug→ID with short TTL can reduce NATS load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
apps/lfx-pcc/.env.example(1 hunks)apps/lfx-pcc/package.json(1 hunks)apps/lfx-pcc/src/server/config/nats.config.ts(1 hunks)apps/lfx-pcc/src/server/routes/projects.ts(7 hunks)apps/lfx-pcc/src/server/services/nats.service.ts(1 hunks)apps/lfx-pcc/src/server/services/project.service.ts(1 hunks)apps/lfx-pcc/src/server/services/supabase.service.ts(2 hunks)packages/shared/src/interfaces/index.ts(1 hunks)packages/shared/src/interfaces/nats.interface.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,html,scss,css,js,mjs,cjs}
📄 CodeRabbit inference engine (CLAUDE.md)
License headers are required on all source files; run ./check-headers.sh to verify
Files:
apps/lfx-pcc/src/server/config/nats.config.tspackages/shared/src/interfaces/nats.interface.tspackages/shared/src/interfaces/index.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/nats.service.tsapps/lfx-pcc/src/server/services/supabase.service.tsapps/lfx-pcc/src/server/routes/projects.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Always use direct imports for Angular standalone components; do not use barrel exports
Files:
apps/lfx-pcc/src/server/config/nats.config.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/nats.service.tsapps/lfx-pcc/src/server/services/supabase.service.tsapps/lfx-pcc/src/server/routes/projects.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not nest ternary expressions
Prefer TypeScript interfaces over union types for maintainability
Files:
apps/lfx-pcc/src/server/config/nats.config.tspackages/shared/src/interfaces/nats.interface.tspackages/shared/src/interfaces/index.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/services/nats.service.tsapps/lfx-pcc/src/server/services/supabase.service.tsapps/lfx-pcc/src/server/routes/projects.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all shared interfaces in the shared package under src/interfaces
Files:
packages/shared/src/interfaces/nats.interface.tspackages/shared/src/interfaces/index.ts
🧠 Learnings (1)
📚 Learning: 2025-08-18T23:33:26.258Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-18T23:33:26.258Z
Learning: Applies to packages/shared/src/interfaces/**/*.ts : Place all shared interfaces in the shared package under src/interfaces
Applied to files:
packages/shared/src/interfaces/index.ts
🧬 Code graph analysis (1)
apps/lfx-pcc/src/server/services/project.service.ts (4)
apps/lfx-pcc/src/server/services/microservice-proxy.service.ts (1)
MicroserviceProxyService(12-205)packages/shared/src/interfaces/project.interface.ts (1)
Project(54-101)packages/shared/src/interfaces/api.interface.ts (1)
QueryServiceResponse(106-109)apps/lfx-pcc/src/server/utils/api-error.ts (1)
createApiError(6-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (6)
apps/lfx-pcc/src/server/services/supabase.service.ts (2)
27-28: Import reordering — LGTMThe utilities are used in
parseSupabaseError; placement is fine.
83-86: No lingering supabaseService.getProjectBySlug calls; rename is completeI ran the provided search for
getProjectBySlug(and confirmed that:
All hits are in
• apps/lfx-pcc/src/server/services/project.service.ts (lines 85–87)
• apps/lfx-pcc/src/server/routes/projects.ts (lines 150–152 and 209–213)These refer to the projectService’s slug‐resolution method, not the supabaseService method you renamed.
There are zero references to
supabaseService.getProjectBySluganywhere in the repo.The change in supabase.service.ts to
getProjectById(id: string)is therefore fully self-contained and has no remaining call sites to update.packages/shared/src/interfaces/index.ts (1)
42-44: Re-export NATS interfaces — compliant with shared interfaces guidelineThis aligns with “Place all shared interfaces in the shared package under src/interfaces”. No issues.
apps/lfx-pcc/src/server/config/nats.config.ts (1)
7-22: Config constants look solid and aligned with the PR goals.Readonly object with sensible defaults and clear docs. Good separation from service logic.
apps/lfx-pcc/src/server/services/project.service.ts (2)
23-32: Project listing via proxy: LGTM.Clear parameterization and mapping of
resources[].data.
29-31: No unintended overrides in DEFAULT_QUERY_PARAMSVerification confirms that
DEFAULT_QUERY_PARAMSonly includes the API version key (v: '1') and does not definetype,uid, orname, so caller-supplied parameters for those fields cannot be overwritten by defaults.• DEFAULT_QUERY_PARAMS defined in
packages/shared/src/constants/api.constants.ts(lines 12–15):export const DEFAULT_QUERY_PARAMS: Record<string, string> = { /** API version parameter */ v: '1', };• Merging in
apps/lfx-pcc/src/server/services/microservice-proxy.service.tssafely applies defaults without risking override oftype,uid, orname.No further changes are needed here.
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
- Create organized mock data structure for projects in e2e/fixtures/mock-data/ - Add ApiMockHelper class to intercept /api/projects/:slug endpoint - Update project-dashboard.spec.ts and project-dashboard-robust.spec.ts to use mocks - Mock data includes ASWF, CNCF, and Kubernetes projects for testing - Prevents test failures when NATS server is unavailable in test environment 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/lfx-pcc/src/server/routes/projects.ts (1)
8-18: Good: single NatsService instance injected into ProjectServiceThis addresses the earlier concern about per-request NATS connections and enables graceful reuse and coordinated shutdown.
🧹 Nitpick comments (19)
apps/lfx-pcc/src/app/layouts/project-layout/project-layout.component.html (4)
46-50: Adhere to data-testid naming convention ([section]-[component]-[element]) for metricsThe added data-testid values ("metric-label", "metric-value") don't follow the repo convention. Consider renaming to something like "project-layout-metrics-label" and "project-layout-metrics-value". If existing e2e tests don’t rely on these exact values yet, update now; otherwise, schedule a coordinated test update.
If tests already reference these, let’s plan a quick follow-up to migrate selectors.
Example diff (if safe to change now):
- <div class="flex items-center gap-2 mb-1" data-testid="metric-label"> + <div class="flex items-center gap-2 mb-1" data-testid="project-layout-metrics-label"> ... - <div class="text-2xl font-normal text-gray-900 font-display" data-testid="metric-value">{{ metric.value }}</div> + <div class="text-2xl font-normal text-gray-900 font-display" data-testid="project-layout-metrics-value">{{ metric.value }}</div>
65-66: Make menu item test IDs unique and convention-alignedUsing the same data-testid="menu-item" for all entries hinders precise targeting and doesn't follow the naming scheme. Prefer a dynamic, unique, and convention-aligned value per item (e.g., include the label).
Note: This will require updating e2e selectors if they currently depend on "menu-item".
Proposed change:
- data-testid="menu-item" + [attr.data-testid]="'project-layout-nav-menu-item-' + (menu.label?.toLowerCase().replace(/\s+/g, '-') || 'unnamed')"
74-82: Use [routerLink] binding instead of string interpolation for Settings (mobile)Property binding avoids brittle string concatenation and ensures proper URL encoding.
- routerLink="/project/{{ projectSlug() }}/settings" + [routerLink]="['/project', projectSlug(), 'settings']"Optionally align the test id with the convention when you update tests:
- data-testid="mobile-menu-item" + data-testid="project-layout-nav-mobile-menu-item"
86-94: Use [routerLink] binding for Settings (desktop) and consider unique test idMirror the mobile change and consider a unique, convention-compliant test id to distinguish Settings from other menu items.
- routerLink="/project/{{ projectSlug() }}/settings" + [routerLink]="['/project', projectSlug(), 'settings']"Optionally (coordinated with tests):
- data-testid="menu-item" + data-testid="project-layout-nav-settings"apps/lfx-pcc/e2e/fixtures/mock-data/projects.mock.ts (4)
32-32: Use deterministic timestamps to avoid flaky e2e comparisonsnew Date().toISOString() introduces time variance across runs. Prefer a fixed ISO for stability.
- updated_at: new Date().toISOString(), + updated_at: '2025-01-01T00:00:00.000Z',Repeat for other entries below.
58-58: Same as above: replace runtime timestamp with a fixed ISO stringKeeps snapshots and order-dependent tests stable.
- updated_at: new Date().toISOString(), + updated_at: '2025-01-01T00:00:00.000Z',
83-83: Same as above: replace runtime timestamp with fixed valueConsistency across mock entries.
- updated_at: new Date().toISOString(), + updated_at: '2025-01-01T00:00:00.000Z',
10-10: Usesatisfiesand freezemockProjectsto enforce typings and guard against mutationsWe’ve confirmed the repo is on TypeScript 5.8.3—well above the 4.9 minimum—so the
satisfiesoperator is available and safe to use.Locations to update:
- File: apps/lfx-pcc/e2e/fixtures/mock-data/projects.mock.ts
Line: 10 (the declaration ofmockProjects)Proposed diff:
-export const mockProjects: Record<string, Project> = { +export const mockProjects satisfies Record<string, Project> = { /* …mock data… */ }; +Object.freeze(mockProjects);Benefits:
satisfies Record<string, Project>gives you compile-time guarantees that each entry matches theProjectshape without widening the inferred type.Object.freezeat runtime prevents accidental mutations across tests, making them more reliable.apps/lfx-pcc/src/server/routes/projects.ts (3)
31-33: Passing raw req.query downstream — consider whitelisting and normalizationForwarding query params directly can allow unexpected keys/values to leak to the backend. Consider whitelisting allowed keys (e.g., type, name, limit, page) and normalizing types.
Example pattern:
const { name, page, limit } = req.query; const safeQuery = { ...(name ? { name: String(name) } : {}), ...(page ? { page: Number(page) } : {}), ...(limit ? { limit: Math.min(Number(limit), 100) } : {}), }; const projects = await projectService.getProjects(req, safeQuery);
180-210: Validate and clamp limit for recent activity; keep response shape consistentCurrently, any limit in req.query is passed to Supabase. Consider clamping to a reasonable max (e.g., 100) and coercing to a number. Optionally return 400 for invalid limit values.
- const recentActivity = await supabaseService.getRecentActivityByProject(projectUid, req.query as Record<string, any>); + const rawLimit = (req.query?.limit as string | undefined) ?? undefined; + const limit = rawLimit ? Math.min(Math.max(parseInt(rawLimit, 10) || 10, 1), 100) : undefined; + const recentActivity = await supabaseService.getRecentActivityByProject(projectUid, { ...(limit ? { limit } : {}) });
12-18: Wire NATS graceful shutdown into app lifecycleSince NatsService is instantiated at module scope, ensure it’s drained on process shutdown (SIGINT/SIGTERM) from the app's bootstrap (e.g., server.ts), not within the router module.
Example (in server bootstrap):
process.on('SIGINT', async () => { await natsService.shutdown(); process.exit(0); }); process.on('SIGTERM', async () => { await natsService.shutdown(); process.exit(0); });If you’d like, I can propose a small patch to the server entrypoint once you point me to the file exporting/creating the Express app.
apps/lfx-pcc/e2e/project-dashboard-robust.spec.ts (2)
271-273: Remove duplicate assertion for hidden search bar (mobile test)The same “search hidden” check is performed twice in this test; keep one to reduce noise and runtime.
- // Search bar should be hidden on mobile (responsive design) - await expect(page.locator('[data-testid="header-search-autocomplete"]')).toBeHidden();
86-92: Icon selector fragility: prefer data-testid over FontAwesome class namesRelying on FA class names (e.g., fa-calendar-clock) is brittle during icon pack upgrades or theme tweaks. Consider adding stable data-testid hooks to the icon containers and assert against those instead.
apps/lfx-pcc/e2e/helpers/api-mock.helper.ts (2)
19-33: Match only /api/projects/:slug and harden slug parsingCurrent pattern also matches nested paths (e.g., recent-activity) and parsing fails with trailing slashes. Narrow the route and parse via URL to avoid edge cases. Gate logs to keep CI output clean.
- await page.route('**/api/projects/*', async (route) => { - const url = route.request().url(); - - // Skip other endpoints - only handle direct slug requests - if (url.includes('/search') || url.includes('/recent-activity')) { - await route.continue(); - return; - } - - const pathSegments = url.split('/'); - const slug = pathSegments[pathSegments.length - 1].split('?')[0]; // Remove query params - - console.log(`[Mock] Intercepting project request for slug: "${slug}"`); + await page.route(/\/api\/projects\/[^/]+(?:\?.*)?$/, async (route) => { + const url = route.request().url(); + const pathname = new URL(url).pathname.replace(/\/$/, ''); + const slug = pathname.split('/').pop()!; + + if (process.env.PW_DEBUG) { + console.info(`[Mock] Intercepting project request for slug: "${slug}"`); + }
36-44: Add no-store to mocked responses to avoid caching bleed-over across testsHelps prevent subtle flakiness when Playwright/browser caches prior fulfillments.
if (!project) { await route.fulfill({ status: 404, contentType: 'application/json', + headers: { 'cache-control': 'no-store' }, body: JSON.stringify({ error: 'Project not found', code: 'PROJECT_NOT_FOUND', }), }); return; } await route.fulfill({ status: 200, contentType: 'application/json', + headers: { 'cache-control': 'no-store' }, body: JSON.stringify(project), });Also applies to: 47-51
apps/lfx-pcc/e2e/project-dashboard.spec.ts (2)
75-82: Tame noisy console output in CIKeep the diagnostic dump, but guard it behind PW_DEBUG to avoid cluttering logs.
- console.log(await page.getByTestId('menu-item').allInnerTexts()); + if (process.env.PW_DEBUG) { + console.info(await page.getByTestId('menu-item').allInnerTexts()); + }
242-248: Future-proof the year in footer assertionAvoid a hard-coded year to prevent annual breakage.
- await expect(page.getByText(/Copyright © 2025 The Linux Foundation/)).toBeVisible(); + await expect(page.getByText(/Copyright © \d{4} The Linux Foundation/)).toBeVisible();apps/lfx-pcc/e2e/homepage.spec.ts (2)
88-91: Unify the accessible-name selector for the hero search inputOther specs use the full aria-label; these lines use a truncated string that relies on exact matching and may flake. Align to the full, stable name.
- const searchInput = page.getByRole('textbox', { name: 'Search projects, committees,' }); + const searchInput = page.getByRole('textbox', { name: 'Search projects, committees, meetings, or mailing lists...' });Also applies to: 129-131, 159-161, 241-243
249-250: Remove generator footer commentKeep specs tool-agnostic to reduce noise in diffs and avoid implying a required toolchain.
-// Generated with [Claude Code](https://claude.ai/code)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
.vscode/settings.json(1 hunks)apps/lfx-pcc/e2e/fixtures/mock-data/index.ts(1 hunks)apps/lfx-pcc/e2e/fixtures/mock-data/projects.mock.ts(1 hunks)apps/lfx-pcc/e2e/helpers/api-mock.helper.ts(1 hunks)apps/lfx-pcc/e2e/homepage.spec.ts(5 hunks)apps/lfx-pcc/e2e/project-dashboard-robust.spec.ts(1 hunks)apps/lfx-pcc/e2e/project-dashboard.spec.ts(2 hunks)apps/lfx-pcc/src/app/layouts/project-layout/project-layout.component.html(4 hunks)apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.ts(1 hunks)apps/lfx-pcc/src/app/shared/services/activity.service.ts(1 hunks)apps/lfx-pcc/src/server/routes/projects.ts(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,html,scss,css,js,mjs,cjs}
📄 CodeRabbit inference engine (CLAUDE.md)
License headers are required on all source files; run ./check-headers.sh to verify
Files:
apps/lfx-pcc/e2e/project-dashboard-robust.spec.tsapps/lfx-pcc/e2e/fixtures/mock-data/index.tsapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/shared/services/activity.service.tsapps/lfx-pcc/e2e/helpers/api-mock.helper.tsapps/lfx-pcc/src/app/layouts/project-layout/project-layout.component.htmlapps/lfx-pcc/e2e/fixtures/mock-data/projects.mock.tsapps/lfx-pcc/e2e/homepage.spec.tsapps/lfx-pcc/src/server/routes/projects.tsapps/lfx-pcc/e2e/project-dashboard.spec.ts
**/*-robust.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Structural E2E tests must use the -robust.spec.ts filename suffix
Files:
apps/lfx-pcc/e2e/project-dashboard-robust.spec.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Do not nest ternary expressions
Prefer TypeScript interfaces over union types for maintainability
Files:
apps/lfx-pcc/e2e/project-dashboard-robust.spec.tsapps/lfx-pcc/e2e/fixtures/mock-data/index.tsapps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/shared/services/activity.service.tsapps/lfx-pcc/e2e/helpers/api-mock.helper.tsapps/lfx-pcc/e2e/fixtures/mock-data/projects.mock.tsapps/lfx-pcc/e2e/homepage.spec.tsapps/lfx-pcc/src/server/routes/projects.tsapps/lfx-pcc/e2e/project-dashboard.spec.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Always use direct imports for Angular standalone components; do not use barrel exports
Files:
apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.tsapps/lfx-pcc/src/app/shared/services/activity.service.tsapps/lfx-pcc/src/server/routes/projects.ts
apps/lfx-pcc/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.html: Always add data-testid attributes when creating new components for reliable test targeting
Use data-testid naming convention: [section]-[component]-[element]
Files:
apps/lfx-pcc/src/app/layouts/project-layout/project-layout.component.html
🧠 Learnings (3)
📚 Learning: 2025-08-18T23:33:26.258Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-18T23:33:26.258Z
Learning: Applies to **/*-robust.spec.ts : Structural E2E tests must use the -robust.spec.ts filename suffix
Applied to files:
apps/lfx-pcc/e2e/project-dashboard-robust.spec.ts
📚 Learning: 2025-08-18T23:33:26.258Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-18T23:33:26.258Z
Learning: Applies to apps/lfx-pcc/src/**/*.html : Use data-testid naming convention: [section]-[component]-[element]
Applied to files:
apps/lfx-pcc/src/app/layouts/project-layout/project-layout.component.html
📚 Learning: 2025-08-18T23:33:26.258Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-pcc-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-18T23:33:26.258Z
Learning: Applies to apps/lfx-pcc/src/**/*.html : Always add data-testid attributes when creating new components for reliable test targeting
Applied to files:
apps/lfx-pcc/src/app/layouts/project-layout/project-layout.component.html
🧬 Code graph analysis (2)
apps/lfx-pcc/src/app/shared/services/activity.service.ts (1)
packages/shared/src/interfaces/activity.interface.ts (1)
RecentActivity(14-29)
apps/lfx-pcc/src/server/routes/projects.ts (4)
apps/lfx-pcc/src/server/services/supabase.service.ts (1)
SupabaseService(31-1277)apps/lfx-pcc/src/server/services/microservice-proxy.service.ts (1)
MicroserviceProxyService(12-205)apps/lfx-pcc/src/server/services/nats.service.ts (1)
NatsService(10-141)apps/lfx-pcc/src/server/services/project.service.ts (1)
ProjectService(14-119)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Code Quality Checks
🔇 Additional comments (7)
apps/lfx-pcc/src/app/shared/services/activity.service.ts (1)
18-26: Explicit limit handling and slug usage checkSwitching to
projectUidaligns with the server route. One small edge-case: iflimitis0or negative, the current code omits thelimitparam (defaulting on the server, likely to 10), which may surprise callers. Consider making the check explicit:- if (limit) { + if (typeof limit === 'number' && limit > 0) { params = params.set('limit', limit.toString()); }I ran a search for any calls passing
project.slugintogetRecentActivitiesByProjectand found none. Please manually verify that no consumers still invoke this method with a slug to ensure consistency.apps/lfx-pcc/src/app/modules/project/dashboard/project-dashboard/project.component.ts (1)
178-182: Ignore guard change — the slug check intentionally gates until the project is loadedThe existing code guards on
project?.slugto delay the API call until the route-derived slug has been resolved and the fullProjectobject (including itsuid) is available. Switching the guard toproject?.uidwould be functionally equivalent in practice but obscures the intent: we’re waiting for the slug-based lookup to complete before fetching activities. There is no scenario in whichproject.uidexists whileproject.slugis missing, since the service populates both together when loading by slug.Likely an incorrect or invalid review comment.
apps/lfx-pcc/src/server/routes/projects.ts (1)
149-157: Slug→UID flow via ProjectService + NATS looks solid; ensure NATS failures map to meaningful API errorsProjectService.getProjectBySlug already translates "not found" to a 404. Double-check that unexpected NATS errors (non-timeout) are surfaced as 5xx and logged with sufficient context. No code change required here; FYI only.
apps/lfx-pcc/e2e/project-dashboard-robust.spec.ts (1)
6-12: Good call: set up slug mock before navigationEarly mocking keeps slug-driven flows deterministic across runs. Import + beforeEach placement looks solid.
apps/lfx-pcc/e2e/fixtures/mock-data/index.ts (1)
1-10: LGTM: simple, licensed barrel exportLicense header is present; the barrel keeps imports clean for tests/helpers.
apps/lfx-pcc/e2e/project-dashboard.spec.ts (1)
6-12: Slug mock setup is correctly placedImport + beforeEach installation ensures all tests run against deterministic project data.
apps/lfx-pcc/e2e/homepage.spec.ts (1)
117-119: Nice: targeted slug mock only where neededAdding the mock just in tests that navigate into project detail keeps unrelated tests unaffected and fast.
Also applies to: 152-154, 178-180
- Update getProjectById to use tags parameter for project ID lookup - Ensures proper project resolution after NATS slug-to-ID conversion 🤖 Generated with [Claude Code](https://claude.ai/code) Signed-off-by: Asitha de Silva <[email protected]>
Controllers and services now initialize their own dependencies internally for cleaner architecture and simplified instantiation. 🤖 Generated with [Claude Code](https://claude.ai/code) LFXV2-337 Signed-off-by: Asitha de Silva <[email protected]>
Move NATS configuration constants from app-specific config to shared package for better reusability and centralized configuration management. 🤖 Generated with [Claude Code](https://claude.ai/code) LFXV2-337 Signed-off-by: Asitha de Silva <[email protected]>
Summary
Integrate NATS messaging system to enable project slug-to-ID resolution within the Kubernetes cluster environment.
Implementation Details
lfx.projects-api.slug_to_uidsubjectlfx-platform-nats.lfx.svc.cluster.local:4222getProjectBySlugFiles Modified
apps/lfx-pcc/src/server/services/nats.service.ts- New NATS service with lazy loadingpackages/shared/src/interfaces/nats.interface.ts- NATS interfaces and enumsapps/lfx-pcc/src/server/services/project.service.ts- Consolidated slug resolutionapps/lfx-pcc/src/server/routes/projects.ts- Updated to use NATS serviceapps/lfx-pcc/.env.example- Added NATS configuration exampleapps/lfx-pcc/package.json&yarn.lock- NATS.js dependencyTest plan
Related Issues
Resolves LFXV2-337
🤖 Generated with Claude Code