fix(helfer): Schichten-Liste im Helfer-Detail zuverlässig laden

[sansan88]

Fixes #257

Problem

Die Helferliste «hängt immer wieder»: Im Helfer-Detail-Modal erscheinen nur die Event-Details, die Schichten-Liste lädt nie, und es kommt wiederholt eine Fehlermeldung (gemeldet für Helfer-Event dYT4m48vvJeHL0w2HrKL, mehrheitlich im Browser genutzt).

Ursachen

Die Pipeline getHelferEventSchichtenWithAttendees() konnte auf mehreren Wegen hängen bleiben oder das Template crashen:

1. forkJoin([]) completed ohne Emission, wenn die Clubmitglieder-Liste leer ist → schichten$ emittiert nie, die Sektion bleibt für immer leer.
2. Ein Firestore-Read pro Clubmitglied, und forkJoin wartet ohne Timeout auf alle → bei grossen Clubs/instabiler Verbindung bleibt die Liste beliebig lange leer.
3. changedAt.toDate() wirft bei Attendee-Dokumenten ohne changedAt.
4. Die catchError-Fallbacks lieferten status: null ohne children — das Template liest aber schicht.status.length, wodurch jede Change Detection erneut wirft (die wiederkehrende Meldung) und die Ansicht eingefroren bleibt.
5. schichten$ kann emittieren, bevor event$ this.user gesetzt hat (this.user.uid → TypeError → Fallback → Punkt 4).

Änderungen

• Sofort rendern: Die Schichten erscheinen unmittelbar mit leeren Attendee-Listen (startWith-Platzhalter, nur beim initialen Laden — Live-Updates flackern nicht auf den Platzhalter zurück); die Profil-Joins füllen sie nach — gleiches non-blocking Muster wie auf den Listen-Seiten (29278ef). confirmSchichten() wartet weiterhin auf die echten Daten.
• forkJoin-Guard: Leere Mitgliederliste → of([]) statt nie emittierendem forkJoin([]).
• timeout(10000) pro Profil-Read; bei Timeout/Fehler greift der «Unknown»-Platzhalter statt dass die ganze Liste blockiert.
• Nullsicherer Sort: changedAt?.toDate?.()?.getTime() ?? 0.
• Template-sichere Fallbacks: zentral in toPlaceholderSchicht() (status: [], children: [] statt status: null), damit schicht.status.length nie mehr wirft.
• pending-Guard: Platzhalter- und Fehler-Fallback-Zeilen sind nicht interaktiv — Taps können die Kapazitätsprüfung nicht mehr mit leeren Attendee-Listen umgehen (toggleSchicht, toggleSchichtItem, addMembersToSchicht).
• Stream-Lebensdauer: schichten$ wird per shareReplay geteilt und über eine komponenten-eigene Subscription für die Seiten-Lebensdauer gepinnt (Freigabe in ngOnDestroy) — der Edit-Toggle zwischen den beiden @if-Zweigen startet die Lesekaskade nicht mehr neu. loadData() baut die Streams nur noch einmal (ngOnInit + ionViewWillEnter starteten sie doppelt).
• this.user?.uid mit filter(Boolean) gegen die Race mit event$.

Tests

• Spec-Grundlage repariert: Der AuthService-Spy kannte getAuthenticatedUser$ nicht — alle 24 Tests der Suite waren rot (gleiches Problem in 13 weiteren Specs → test: AuthService-Spies in 13 Spec-Dateien veraltet – Suiten schlagen seit der getAuthenticatedUser$-Umstellung fehl #260). Spy ergänzt.
• 9 neue Regressionstests: leere Mitgliederliste, fehlendes changedAt, Fehler-Fallbacks (template-sicher und nicht interaktiv), Eager-Platzhalter (sofort, kein Replay bei Live-Updates, langsame Mitgliederliste), 10s-Timeout-Pfad (fakeAsync), Taps auf pending-Zeilen, keine Kaskaden-Neustarts bei Re-Subscription/View-Re-Entry.
• ng test (helfer-detail-Suite): 33/33 grün; ng build: erfolgreich (AOT/strictTemplates).

Follow-ups: #259 (Profil-Lookups bündeln/cachen statt N Reads pro Clubmitglied), #260 (veraltete AuthService-Spies in 13 weiteren Specs).

🤖 Generated with Claude Code

https://claude.ai/code/session_01FkgikK428UwuRY3RJ6FMUM

[claude]

Review

Solid, well-targeted fix — each of the five root causes named in the PR description is addressed at the right layer, and the reasoning is easy to follow from the code + inline comments. A few notes, none blocking.

Correctness

• forkJoin([]) guard (clubMemberProfiles$.length > 0 ? forkJoin(...) : of([])) correctly avoids the classic RxJS trap of forkJoin never emitting on an empty array — this alone plausibly explains a chunk of the reported "hängt immer wieder" reports for smaller/empty clubs.
• changedAt?.toDate?.()?.getTime() ?? 0 and this.user?.uid + .filter(Boolean) are the right minimal, defensive fixes for legacy docs and the event$/schichten$ race, respectively.
• Fallback shape consistency (status: [], children: [] instead of status: null) is the most important fix here — the template reads schicht.status.length, so the old fallback shape was actively crashing change detection and reproducing the same freeze it was trying to recover from. Good catch.
• timeout(10000) on the per-member profile read is a reasonable bound; worth double-checking that getUserProfileById doesn't itself internally retry/backoff in a way that fights the timeout, but nothing in this diff suggests that.

Minor suggestions (non-blocking)

• The "empty schicht" placeholder object literal (attendees: [], attendeeListTrue: [], attendeeListFalse: [], unrespondedMembers: ..., status: [], children: []) is now duplicated three times: the per-schicht catchError, the club-members catchError, and the eagerPlaceholder startWith. Extracting a small toPlaceholderSchicht(schicht, unrespondedMembers = []) helper would remove the risk of the three copies drifting out of sync next time this shape needs a field.
• confirmSchichten() intentionally calls the non-eager path, so a club with several stalled member profiles could still make the user wait up to ~10s (bounded now by timeout, vs. unbounded before) with no loading indicator. Given the bug report was specifically about perceived hangs, a lightweight loading state on that action might be a nice follow-up, though it's out of scope for this fix.
• The N+1-style "one profile read per club member" pattern itself is unchanged (per the PR description, this is a known, only partially mitigated issue) — worth a follow-up ticket for batching/caching profile lookups for larger clubs, since the timeout reduces the worst case but doesn't reduce read volume.
• Test coverage is good for the emission-shape bugs (empty members, missing changedAt, member-list error, eager placeholder). One gap: there's no test that actually exercises the timeout(10000) firing (e.g. via fakeAsync/tick) to lock in that a genuinely slow (not just NEVER-hanging) profile read degrades to the "Unknown" fallback rather than blocking indefinitely — the current NEVER test only covers the eager-placeholder path, not the timeout path itself.
• Nice catch fixing the AuthService spy — good that it surfaced the entire spec suite was silently red before this PR.

Security / performance

No security concerns (no new user input paths, standard Firestore reads scoped by club/event id). Performance impact is neutral-to-positive: this trades "wait forever" for "wait up to 10s, or render immediately with a placeholder," which is strictly better for perceived responsiveness.

Overall: good root-cause analysis, matches an existing non-blocking pattern already used elsewhere in the Helfer pages, and the test additions meaningfully cover the regressions being fixed.

[claude]

Review

Solid, well-scoped fix for the "Helferliste hängt" bug. The five root causes listed in the PR description all check out against the diff, and the new spec cases (forkJoin([]) guard, missing changedAt, member-load failure, slow-profile timeout, eager placeholder) exercise exactly those paths. confirmSchichten() correctly keeps using the non-eagerPlaceholder path so it still waits for real data before building the confirm-dialog inputs.

Potential bug / regression to double-check

getClubHelferEventSchichtenRef and getClubMemberRefs are live Firestore listeners (collectionData(...), no first()/take(1)), so the outer switchMap in getHelferEventSchichtenWithAttendees re-runs the entire nested pipeline whenever the schichten collection emits again (e.g. an admin renames a Schicht or changes countNeeded while another user has the page open).

Before this change, that resubscription would keep the previously-rendered (fully resolved) array on screen via the async pipe until the new one was ready — a stale-but-correct display. With eagerPlaceholder: true now prepending a fresh startWith([...placeholders]) on every resubscription, every live viewer's whole Schichten list (not just the changed Schicht) will flash back to the "unanswered" (status: [], warning icon) state for the few hundred ms it takes attendees/profiles to reload — even though nothing about their own status actually changed. It's not a hang/crash, but it's a new, visible regression for concurrent viewers that's worth confirming is an acceptable trade-off (or scoping the placeholder to first-subscription only, e.g. via a shareReplay/one-shot flag instead of an unconditional startWith inside the re-run pipe).

Minor / nits

• toPlaceholderSchicht is a nice dedup of the three previously-inconsistent fallback shapes (this was the actual source of the template crash, since status: null vs status: [] mismatched what the template unconditionally reads via schicht.status.length) — good fix.
• The timeout(10000) is placed correctly relative to take(1)/catchError, and the fakeAsync test (tick(10000) vs. a 20s delay) genuinely proves the timeout path fires rather than just asserting on mocked-instant data.
• this.user?.uid + .filter(Boolean) correctly closes the race with event$, and is covered by the "no changedAt"/robustness tests using component.user set directly rather than relying on event$ timing.

Test coverage

Good targeted coverage of the new branches. One gap: no test asserts that a change to the attendees subcollection (as opposed to the schichten collection or member list) does not re-trigger the eager placeholder — that's the scenario underlying the regression concern above, and a test would make the intended behavior explicit either way.

Security / performance

No security concerns (no new user input handling, no rule changes implied). Performance-wise, the timeout(10000) per member profile is a good bound on the previous unbounded forkJoin wait, and forkJoin([]) → of([]) avoids the permanent-hang case cleanly.

🤖 Generated with Claude Code

[claude]

Review

Solid, well-targeted bug fix. The root-cause analysis in the description matches what's in the diff, and the new regression tests specifically cover each failure mode described (empty forkJoin, missing changedAt, member-list error, race with this.user, and the eager-placeholder behavior). A few notes:

Code quality / correctness

• toPlaceholderSchicht shape consistency (nice fix): Centralizing the template-safe empty shape (status: [], children: [], etc.) instead of duplicating status: null across three call sites removes the exact class of bug that caused the original crash loop (schicht.status.length on null). Good catch.
• schichtenEmissionIndex gating (helfer-detail.page.ts:197,361): Using switchMap's emission index to only prepend the placeholder on the first emission is a good way to avoid flashing "unanswered" on every live update — and it's actually exercised by the "should not replay the eager placeholder on live schichten/attendees updates" test. Nice.
• confirmSchichten() (line 388-395) intentionally omits eagerPlaceholder, so firstValueFrom still resolves only once the real join has settled — correct, since confirming attendance based on placeholder (empty) data would silently confirm nothing.
• Minor: toPlaceholderSchicht is typed (schicht: any, ...). Since Schicht is already imported in this file, it'd be nice to type this more strongly at some point, though this is consistent with the rest of the method's existing any-heavy style, so not a blocker.

Potential remaining issues (pre-existing, not introduced by this PR, but worth flagging)

• N+1 Firestore reads per club member (clubMembers.map(member => userProfileService.getUserProfileById(...))) is still present — for large clubs this means one doc read per member on every load, per subscriber. The new timeout(10000) bounds the hang, but not the cost. Might be worth a follow-up to batch these or cache clubMembersWithDetails across the two schichten$ | async usages in the template (see below).
• schichten$ is consumed via | async twice in the template (helfer-detail.page.html:93 and :388 — once for the member view, once for the admin-edit view). Since schichten$ is a plain (non-shareReplay'd) observable, each async pipe subscribes independently, so the entire member-list + per-member profile read cascade runs twice concurrently whenever both blocks are in the DOM. Not introduced by this PR, but the new eager-placeholder logic doubles the "flash" surface area slightly. Consider shareReplay(1) on schichten$ in loadData() in a follow-up.
• Outer catchError still terminates the live listener (helfer-detail.page.ts:381-384): a single error from getClubHelferEventSchichtenRef (e.g. a transient permission blip) replaces the stream with of([]) and completes it — the live Firestore listener is not automatically resubscribed, so the Schichten section would go empty and stay empty until the modal is reopened (ionViewWillEnter re-triggers loadData()). This is pre-existing behavior, not a regression, but worth knowing given the bug being fixed here was exactly this route "gets stuck empty."

Security

No concerns — no new user input handling, and Firestore access remains inside the service layer per docs/guidelines.md.

Test coverage

Good coverage of the specific bugs fixed: empty member list, missing changedAt, member-list load failure, and the eager-placeholder/no-replay behavior, including a fakeAsync test for the 10s timeout path. One gap: there's no test asserting that a slow but eventually-successful getClubMemberRefs (i.e., the club-members fetch itself, not a per-profile fetch) still surfaces the eager placeholder first and then the resolved list — only the schichten-source emission is exercised for that path via the Subject-based test. Not a blocker given the existing coverage is already thorough for the reported bug.

Style / conventions

Matches docs/guidelines.md: no Firestore access in the page beyond existing patterns, no hardcoded UI strings added, files/identifiers stay consistent with existing (German-domain, English-code) conventions in this module.

Overall: a targeted, well-tested fix for a real production bug. Nothing here blocks merging.

[claude]

Review

Solid fix — this correctly diagnoses and addresses all five root causes described in the issue (forkJoin([]) never emitting on an empty member list, unbounded per-member Firestore reads, the changedAt.toDate() crash on legacy docs, the template-unsafe status: null fallback, and the this.user.uid race with event$). The commit history also shows the PR responding to its own review feedback (extracting toPlaceholderSchicht, fixing placeholder replay on live updates, sharing schichten$), which is a good sign of iteration.

Code quality

• toPlaceholderSchicht() is a good extraction — it removes the risk of the three duplicated placeholder shapes drifting apart, and the doc comment correctly explains why the shape matters (unconditional status.length/children reads in the template).
• The schichtenEmissionIndex > 0 guard to avoid replaying the placeholder on later live-listener emissions (e.g. an admin renaming a Schicht) is a subtle and correct fix — confirmed by the "should not replay the eager placeholder on live schichten/attendees updates" test.
• Nit: the comment justifying shareReplay({bufferSize: 1, refCount: true}) says the template "consumes schichten$ through two async pipes... concurrently" (helfer-detail.page.ts:114-116). Looking at the template, the two schichten$ | async usages (helfer-detail.page.html:93 and :388) are inside mutually exclusive @if (!allowEdit) / @if (allowEdit) branches, so they are never actually subscribed at the same time today — toggling allowEdit unsubscribes one before the other subscribes. shareReplay is still harmless/defensive (and cheap insurance against future template changes), but the comment overstates the current concurrency; might be worth tightening the comment or dropping the "concurrently" claim.
• Minor: the spec's AuthService spy still registers getUser$ (helfer-detail.page.spec.ts:81-86) alongside the newly-added getAuthenticatedUser$, but the component only calls getAuthenticatedUser$() now. Harmless, but it's dead test setup that could be trimmed.

Potential bugs

• Behavioral change worth flagging (not necessarily a blocker): with eagerPlaceholder: true, shift rows are now interactive (status icons have (click) handlers) immediately, using placeholder data where attendeeListTrue = []. The toggleSchicht/toggleSchichtItem "shift full" check (schicht.attendeeListTrue.length >= schicht.countNeeded) will see 0 >= countNeeded as false during that window, so a tap during the placeholder phase could bypass the client-side capacity check and register an attendee before the real counts are known. Previously the whole schichten$ block simply did not render until data resolved, so nothing was clickable during loading — this is a new (if narrow, typically sub-second, up to ~10s in the timeout-fallback case) window. Since this is only a client-side UX guard anyway (nothing here suggests server-side enforcement of countNeeded), this is likely acceptable, but worth a conscious decision rather than an incidental side effect — e.g. disabling the click handler while the row is still a placeholder.
• Everything else (null-safe changedAt sort, template-safe status: []/children: [] fallbacks, forkJoin([]) guard, this.user?.uid optional chaining) looks correct and directly addresses the reported crash/hang.

Performance

• timeout(10000) per member-profile read is a reasonable bound and runs in parallel across members (not sequential), so worst case is ~10s regardless of club size, not 10s-per-member.
• shareReplay on schichten$ is a reasonable defensive measure even if not strictly required by the current template structure (see nit above).
• Pre-existing (not introduced by this PR): the full member-profile cascade re-runs on every emission of getClubMemberRefs/getClubHelferEventSchichtenRef, which are live listeners — for large clubs this still means re-fetching every member profile on unrelated live updates. Out of scope for this fix, but could be worth a follow-up (e.g. caching resolved profiles across emissions) if getHelferEventSchichtenWithAttendees shows up in profiling later.

Security

• No concerns — no new user input handling, no new endpoints, and the changes are read-path robustness/error-handling only.

Test coverage

• Good regression coverage: empty member list, missing changedAt, member-list load failure fallback, no-placeholder-replay-on-live-update, slow member list (fakeAsync), and the 10s timeout degrade path are all covered — including the specific hang/crash scenarios called out in the issue.
• Given the finding above, consider adding a test that clicks/toggles a shift while it is still in its placeholder state (attendeeListTrue: []) to document/decide the intended behavior for that window.

Overall: a well-targeted, well-tested fix for the reported hang/crash. The one thing I'd want a second look at before merge is the interactive-placeholder capacity-check window — worth a quick gut check on whether it's acceptable as-is.

[claude]

Code Review

Reviewed the fix for the Helfer-Schichten loading hang (helfer-detail.page.ts / .spec.ts). Overall this is a solid, well-targeted fix — each of the five root causes described in the PR body is addressed, and the regression tests map 1:1 to each cause.

Strengths

• forkJoin([]) guard (clubMemberProfiles$.length > 0 ? forkJoin(...) : of([])) directly fixes the "never emits" bug — good catch, and easy to miss.
• timeout(10000) + catchError per profile read bounds the worst case instead of leaving the whole list hostage to one slow/stuck read.
• Null-safe changedAt?.toDate?.()?.getTime() ?? 0 and this.user?.uid with filter(Boolean) correctly close the two crash paths that were turning into the recurring error toast.
• toPlaceholderSchicht() centralizes the template-safe shape (status: [], children: [] instead of null) so every fallback/placeholder path stays consistent — nice refactor, reduces the chance of a future fallback reintroducing the status: null crash.
• The eagerPlaceholder + startWith-on-first-emission-only design (guarded via the switchMap index) is a thoughtful way to avoid flashing existing viewers back to "unanswered" on live Firestore updates, and is directly covered by the "should not replay the eager placeholder on live schichten/attendees updates" test.
• Test coverage is strong: the forkJoin-empty case, missing-changedAt, member-list error fallback, eager-placeholder-vs-live-update behavior, and the timeout degrade path are all exercised with fakeAsync/Subject/BehaviorSubject where needed rather than only happy-path of(...).

Potential gap: addMembersToSchicht isn't guarded against the placeholder state

toggleSchichtItem and toggleSchicht both bail out early when schicht?.pending is true (lines ~572, ~659), which correctly prevents a tap during the loading window from bypassing the capacity check. addMembersToSchicht (line 1011) doesn't have the same guard, though it's reachable from the same admin sliding-item action while schichten$ is still emitting placeholders (schicht.attendeeListTrue is [] in that state):

const existingMemberIds = schicht.attendeeListTrue.map((m) => m.id);
const availableMembers = clubMembers.filter(
  (member) => member && !existingMemberIds.includes(member.id),
);

During the pending window this will list every club member as "available to add", including ones already signed up (whose real status just hasn't resolved yet). Functionally it's likely harmless (setClubHelferEventSchichtAttendeeStatusAdmin(true, …) just re-sets status for an existing doc rather than duplicating), but it's a UX inconsistency worth either guarding (if (schicht?.pending) return;, same pattern as the other two handlers) or at least covering with a test, for consistency with the rest of the fix.

Minor / non-blocking

• During the placeholder window the badge shows 0 / countNeeded and child rows briefly disappear (since children: [] in the placeholder) before snapping to the resolved values. This is presumably an accepted trade-off for "render immediately," but on a slow connection (near the 10s timeout budget) it could read as "nobody signed up yet" for a noticeable moment. Worth confirming this UX is intentional/acceptable — a subtle loading indicator on the badge while pending is true might avoid the momentary "0 signed up" impression.
• getClubMemberRefs(clubId) and the full per-member profile fan-out are re-run from scratch on every schichten-collection emission (this predates this PR, not a regression), which combined with the new per-profile timeout(10000) means a large club with a flaky read could repeatedly pay the fetch cost on each live update. Not introduced here, but since this code path just got touched, it might be worth a follow-up to cache/memoize club member profiles independently of the schichten listener.
• Nit: the "24 tests were red" pre-existing issue (stale getUser$ spy vs. getAuthenticatedUser$) is a good catch to fix in this PR, but it does mean the whole suite was silently not running as intended before — might be worth double-checking no other spec files have a similar stale-spy drift from the AuthService API change.

Security

No concerns — no new user input surfaces, Angular's template binding still handles escaping, and error logging (console.error) doesn't introduce anything beyond what was already logged.

Test coverage

Good — the new tests target the actual failure modes from the bug report rather than just re-asserting happy path. The one gap noted above (addMembersToSchicht during pending) would be a nice addition but isn't blocking.

🤖 Generated with Claude Code

[claude]

Review

Solid fix for the hang — the forkJoin([]) guard, timeout(10000) per profile read, null-safe changedAt sort, and template-safe fallback shapes all directly address the causes listed in the PR description, and the new spec coverage (empty member list, missing changedAt, member-load error, no-replay-on-live-update, slow member list, profile timeout, eagerPlaceholder) is thorough for the happy/degraded paths.

One correctness gap I'd like to see addressed before merge:

pending isn't set on the two error-fallback placeholders, so a real fetch error can bypass the capacity check

toPlaceholderSchicht() (helfer-detail.page.ts:182-199) defaults pending to false, and both error-path callers rely on that default:

• helfer-detail.page.ts:350-356 — attendee-fetch catchError → toPlaceholderSchicht(schicht, clubMembersWithDetails)
• helfer-detail.page.ts:365-369 — club-member-load catchError → toPlaceholderSchicht(schicht)

Both produce a Schicht with attendeeListTrue: [] but pending: false. toggleSchichtItem, toggleSchicht, and addMembersToSchicht only early-return if (schicht?.pending) — so on a genuine Firestore error (permission-denied, transient failure, the same class of error this PR is explicitly hardening against) the row looks tappable and:

• toggleSchicht/toggleSchichtItem's capacity check (schicht.attendeeListTrue.length >= schicht.countNeeded) always passes since the list is empty, silently bypassing the "shift full" guard for non-admins.
• addMembersToSchicht computes existingMemberIds from the same empty attendeeListTrue (helfer-detail.page.ts:1039), so it offers every club member as "available," including ones already signed up.

Since this fallback path is precisely what fires when Firestore has the kind of transient error this PR is designed to survive, this isn't a hypothetical — it's the realistic failure mode. Given the loading-placeholder case already got a pending flag and guard, it'd be a small, consistent fix to mark these two error fallbacks pending: true as well (or otherwise disable interaction while status/children are empty due to error rather than "no data"). Worth a regression test analogous to the existing "should ignore taps while the shift row is still a placeholder" one, but for the error-fallback path instead of the loading path.

Minor / non-blocking

• No visual affordance for pending rows — the template (helfer-detail.page.html) doesn't reference pending at all, so during the brief placeholder window the row renders as fully interactive and a tap silently no-ops. Not a regression (previously it just hung/crashed), but a disabled/loading state on the row would avoid a confusing silent-no-op for users who tap during that window.
• The schichtenEmissionIndex-based "only show eager placeholder on the first emission" logic is clever and well-commented, and the accompanying test (should not replay the eager placeholder on live schichten/attendees updates) does a good job proving it doesn't flash existing viewers back to "unanswered" on live updates.

Nice catch fixing the schichten vs sortedSchichten mismatch in the member-load-error fallback while you were in there, and good call preserving confirmSchichten()'s wait-for-real-data behavior by not passing eagerPlaceholder there.

🤖 Generated with Claude Code

[claude]

Review

I read through the diff for helfer-detail.page.ts / .spec.ts in detail. Overall this is a well-targeted fix for the described hang, with solid regression tests. A few notes below.

Strengths

• Each root cause from the PR description maps to a concrete, minimal fix: the forkJoin([]) guard, the per-profile timeout(10000), the null-safe changedAt sort, and the pending-flag guards on toggleSchichtItem/toggleSchicht/addMembersToSchicht all look correct and are exercised by new tests.
• Good catch turning the old status: null fallback into a pending: true placeholder — the old fallback (attendeeListTrue: []) already prevented a reported capacity overbooking, but a stalled fetch previously still let a tap through with unresolved data; blocking taps while pending is a real safety improvement.
• Test coverage for this change is thorough: empty member list, missing changedAt, member-list failure, no-replay-on-live-update, slow member list, and profile-read timeout are all covered independently.

Potential issue: shareReplay({ refCount: true }) + mutually-exclusive @if blocks

schichten$ is consumed via | async in two places in the template — the member view (@if (!allowEdit), line ~93) and the admin-edit view (@if (allowEdit), line ~388). These are exclusive today, which is exactly why the comment in loadData() justifies shareReplay. But because they're exclusive, toggling edit() (allowEdit = true/false) tears down one @if block's async-pipe subscription and creates the other's in the same change-detection pass. With refCount: true, the subscriber count for schichten$ briefly drops to 0 during that swap, which causes shareReplay to fully unsubscribe from the source and discard its buffer — the next subscription starts the whole pipeline over from scratch (a fresh getClubHelferEventSchichtenRef subscription, schichtenEmissionIndex back to 0, a full club-member + per-member-profile refetch, and the eager placeholder flashing again).

So every time an admin clicks "Bearbeiten" → "Fertig", the shift list likely flickers back to the empty placeholder and re-issues all the member/profile reads it just did — which undercuts the "reuse one read cascade" rationale in the comment and adds avoidable Firestore reads. Worth manually verifying (toggle edit mode a few times and watch network/console for repeated "Failed to fetch profile" style logs or duplicated getClubMemberRefs calls). If confirmed, shareReplay({ bufferSize: 1, refCount: false }) (or keeping the subscription alive for the page's lifetime and tearing it down in ngOnDestroy instead) would avoid the reset.

Minor

• The pipeline still does one Firestore document read per club member on every load (unchanged from before, now bounded by a 10s timeout per read instead of hanging indefinitely) — fine for this bug-fix scope, but for large clubs this is still O(n) reads per view; might be worth a follow-up to batch these (e.g. getUserProfilesByIds) if this page is opened often.
• When the member-list fetch fails (catchError around getClubMemberRefs) or a single Schicht's attendee fetch fails, the resulting pending: true state is permanent for that emission — there's no retry/backoff, so a transient error leaves the shift row interaction-disabled until the underlying live listener re-emits on its own. That's a safe failure mode (no silent overbooking) but there's no user-facing indication that a shift is "stuck" vs. genuinely empty; consider a toast or subtle indicator if this turns out to happen often in the field.
• toPlaceholderSchicht's doc comment and the inline comments throughout the diff are genuinely helpful here (explaining why, e.g. the pending flag, the eager-placeholder-only-on-first-emission logic) — good use of comments for non-obvious invariants.

Security

No concerns — this is client-side UI/data-shaping logic; the capacity/overbooking check being bypassable during a pending state was already effectively mitigated by empty arrays before, and is now more robust by short-circuiting entirely. Firestore security rules (not touched here) remain the actual authorization boundary.

Test coverage

Good — the new spec cases target the exact failure modes described in the PR body. One gap: no test directly exercises the shareReplay/allowEdit-toggle interaction above, since specs call getHelferEventSchichtenWithAttendees(...) directly rather than through the shared schichten$ + template toggle path. If the shareReplay concern is real, a test around loadData() + toggling allowEdit (or just asserting getClubMemberRefs call counts across two subscribe/unsubscribe cycles of schichten$) would catch it.

Nice fix overall — the core hang and the template-crash paths are addressed cleanly and the reasoning in the PR description matches what's in the diff.

[claude]

Review

Solid fix — the root-cause analysis in the PR description matches what's in the diff, and each of the five failure modes has a corresponding code change plus a regression test. A few notes below, nothing blocking.

Code quality

• toPlaceholderSchicht() is a good extraction — having a single template-safe shape used by both the eager placeholder and every catchError fallback removes the class of bug where one fallback path forgot a field (which is exactly what caused the original status: null crash).
• The comments explaining why (forkJoin([]) never emitting, changedAt missing on legacy docs, the shareReplay/refCount interaction with the two mutually-exclusive @if template branches) are exactly the kind of non-obvious context worth keeping in-line — good restraint on not over-commenting the rest.
• getHelferEventSchichtenWithAttendees was already deeply nested before this change; it's now a few levels deeper (switchMap → switchMap → switchMap → .map). Not something to fix in this PR, but if this function gets touched again, pulling the per-schicht attendee-join into a named method would help readability.

Potential issues

• Silent no-op on pending taps: toggleSchichtItem, toggleSchicht, and addMembersToSchicht all return early with no user feedback when schicht.pending is true. Since the placeholder window is normally sub-second this is a minor UX nit rather than a bug, but a quick presentToast() (there's already a helper for that) would avoid a "why didn't my tap do anything" moment if a user manages to tap during a slow load.
• unrespondedMembers passed into the attendee-fetch error fallback: toPlaceholderSchicht(schicht, clubMembersWithDetails) populates unrespondedMembers even though the same object also sets pending: true, which makes every handler that would consume unrespondedMembers (e.g. addMembersToSchicht) bail out before it's used. Harmless today, but worth a one-line comment (or just passing [] like the other call site) so a future reader doesn't wonder if it's load-bearing.
• Per-emission full cascade on live updates: eagerPlaceholder's startWith only fires on schichtenEmissionIndex === 0; every later live emission of the schichten collection re-runs getClubMemberRefs → per-member profile reads → combineLatest of attendee refs from scratch (this predates the PR, just noting it's not fixed here). For large clubs this means any Schicht metadata edit re-fetches every member profile again. Worth a follow-up if clubs with many members start noticing lag on live updates, but out of scope for this bug fix.

Performance

• The 10s per-profile timeout is a reasonable bound, but since it's inside forkJoin, a club with many members each hitting the timeout independently (e.g. a flaky connection) still means the whole shift list is blocked for up to 10s before falling back — bounded now (good, that was the point), but still a visible stall for larger clubs. Not a blocker given it replaces an unbounded hang.

Security

Nothing concerning — no new user input paths, no template innerHTML/bypassSecurityTrust* usage, capacity/overbooking checks are preserved (and actually hardened by the pending guard closing the race where a tap during the loading window could have bypassed the capacity check against a stale/empty attendee list).

Test coverage

Good depth here — fakeAsync/tick are used correctly to assert the exact 10s timeout boundary, the eagerPlaceholder replay behavior is tested for first-load vs. live-update cases separately (verifying no placeholder "flash" on subsequent emissions), and the schichtenSub/shareReplay pinning behavior has a dedicated test for the two-view-toggle scenario. Also worth calling out: catching that the AuthService spy was missing getAuthenticatedUser$ (silently failing all 24 existing tests) is a good catch independent of the main fix — that's exactly the kind of thing that could mask future regressions in CI if left unnoticed.

One gap: there's no test asserting the admin-overbooking path (isAdmin bypassing the capacity check in toggleSchichtItem) still works correctly now that placeholder rows short-circuit before that check — probably fine since it's unchanged logic, but since capacity/overbooking is the security-sensitive bit here, an explicit test would make the invariant more future-proof.

Nice work tracking down a genuinely gnarly combination of RxJS completion/timing bugs.