fix(meetings): evict detail cache on cancelOccurrence + map 400 error

[andrest50]

Summary

Fixes issue 1 of 2 from linuxfoundation/lfx-self-serve-ops#13: the "occurrence already cancelled" UX problem caused by a concurrent-session race condition amplified by stale caching.

Root cause

Two independent caching gaps meant a successful cancelOccurrence left the occurrence still visible in the UI:

1. Angular in-memory cache — MeetingService.cancelOccurrence was not evicting the meetingDetailCache entry on success, unlike updateMeeting and deleteMeeting which both call this.meetingDetailCache.delete(id). The occurrence remained visible in the same session until the TTL expired.
2. Misleading error message — When two concurrent sessions raced, the second got a correct 400 "occurrence for meeting is already cancelled" from itx-service-zoom. The component mapped this to the generic "Failed to cancel occurrence. Please try again." — which encouraged further retries.

Changes

apps/lfx-one/src/app/shared/services/meeting.service.ts

• Add tap(() => this.meetingDetailCache.delete(meetingId)) to the cancelOccurrence pipe on success, consistent with updateMeeting (line 315) and deleteMeeting (line 330).

apps/lfx-one/src/app/modules/meetings/components/cancel-occurrence-confirmation/cancel-occurrence-confirmation.component.ts

• Add a 400 branch that maps to: "This occurrence has already been cancelled — please refresh the page." instead of the generic retry prompt.

Testing

• Cancel an occurrence: the occurrence disappears from the list immediately without a manual refresh.
• Simulate a concurrent cancel (e.g. two tabs): the second attempt shows the specific "already cancelled" message rather than the generic error.

Made with Cursor

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Essentials

Run ID: 26d510d2-e9bd-4dc0-8235-fea70975024f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

Fixes stale meeting details after occurrence cancellation and improves concurrent-cancellation feedback.

Changes:

• Evicts cached meeting details after successful cancellation.
• Adds specialized handling for already-cancelled responses.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
meeting.service.ts	Invalidates the meeting detail cache after cancellation.
cancel-occurrence-confirmation.component.ts	Maps HTTP 400 responses to cancellation feedback.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[dealako]

Hey @andrest50, thanks for the fast turnaround on this one.

Overall impression: Small, targeted fix that closes a real gap — cancelOccurrence was the only cache-evicting meeting mutation that didn't evict, and the 400 mapping gives users a much clearer signal on the concurrent-cancel race than the generic failure toast. The follow-up commit narrowing the 400 match to the specific upstream message (instead of any 400) is exactly right and already addresses the one issue Copilot flagged on the first commit. Three independent reviewers (security/privacy, correctness/perf/tests, style/consistency) went over the diff; no blocking or privacy issues came back.

• 🔴 Blocking: 0 issues
• 🟡 Minor: 2 issues: no test coverage for either new branch (cache eviction, 400 mapping); the upstream-message substring match is inherently brittle if the wording ever changes
• ⚪ Nit: 3 issues: cancelOccurrence skips the catchError logging block its siblings (updateMeeting/deleteMeeting) have; 'already cancelled' is a hardcoded string rather than a shared constant; no inline comment explaining why a 400 needs special-case matching

Bot reconciliation: Copilot's only comment (400 too broad, would mislabel other validation failures as "already cancelled") was fixed in the very next commit (b33c67633) — confirmed by re-reading the current code, agree it's resolved.

Final decision: ✅ Approved with minor comments

[dealako]

Automated review — see summary comment for full context. Inline notes below.

[dealako]

[minor] Missing test coverage for cache eviction

Issue: The new tap(() => this.meetingDetailCache.delete(meetingId)) has no test verifying it fires on success and uses the correct cache key (meetingId, not occurrenceId).
Proof: No spec file exists for this frontend meeting.service.ts (the only meeting.service.spec.ts in the repo is the backend one). updateMeeting/deleteMeeting have the same gap, so this isn't a regression, but it's new logic worth covering given it's fixing a real stale-cache bug.
Why it matters: A future refactor of this pipe chain could silently drop the eviction with nothing to catch it.
Fix: Add a unit test asserting meetingDetailCache.delete is called with meetingId on success and not called on error.

[dealako]

[nit] Inconsistent with sibling methods: no catchError logging

Issue: updateMeeting and deleteMeeting both log via catchError before rethrowing; cancelOccurrence omits it.
Proof: Compare lines ~308-336 (updateMeeting/deleteMeeting) to this method — both siblings have catchError((error) => { console.error(...); return throwError(() => error); }), this one doesn't.
Why it matters: Purely an observability gap (errors still propagate correctly to the component without it), but it breaks the established logging pattern for this class of mutation.
Fix: Add the same catchError block for consistency, e.g. logging the meetingId/occurrenceId before rethrowing.

[dealako]

[minor] Brittle upstream-message substring match, no test

Issue: The friendly "already cancelled" message only fires if the BFF-serialized upstream error text contains the exact case-sensitive substring 'already cancelled'.
Proof: Line 43 — (error.error?.error ?? '').includes('already cancelled'). No spec file exists for this component, so there's nothing to catch a wording drift from the upstream service. This is a fail-safe brittleness (a mismatch just falls through to the generic message, not a broken error path), but it already needed one narrowing pass in this same PR (see the follow-up commit fixing the original overly-broad 400 check), which suggests the upstream contract here is worth locking down harder.
Why it matters: If itx-service-zoom ever rewords this message, the UX regresses silently back to the generic "Failed to cancel" toast with no test failure to flag it.
Fix: Add a component spec covering both the match and non-match 400 cases, and/or ask upstream for a stable error code instead of matching on free text.

[dealako]

[nit] Hardcoded error substring vs. shared constant

Issue: 'already cancelled' is inlined here rather than defined as a shared constant.
Proof: CLAUDE.md: "All shared constants and interfaces live in @lfx-one/shared — no module-level consts." This string represents a stable upstream API contract value, similar in spirit to the constants that convention is meant to centralize.
Why it matters: low impact today, but if this string needs to be referenced from another spot later it'll drift.
Fix: Optional — could extract to packages/shared/src/constants if this pattern is reused elsewhere; not required for a single call site.

[dealako]

[nit] Missing WHY comment for the special-case match

Issue: No comment explains why a 400 needs this specific string match rather than being treated as a generic client error.
Proof: Line 43 — the reasoning (itx-service-zoom returns a plain 400 rather than a distinct status/code for a concurrent-cancel race) is only in the commit message, not the code.
Why it matters: The repo convention is to comment non-obvious WHY logic; a future reader has no clue why this particular substring match exists without digging into git blame.
Fix: One-line comment above the branch, e.g. noting itx-service-zoom returns a plain 400 (not a distinct code) when the occurrence is already cancelled.