Fix Team Collection status dialog blanking the whole window (BL-16757)

[StephenMcConnel]

Clicking the Team Collection button blanked the entire Bloom window — nothing in the outer
window, no recourse but to quit — for collections whose history contains an event with a null
userid. Reported against ~/Documents/Bloom/Bunong Bible Materials; 6.5 was already immune.

Why it blanked the whole window rather than just the dialog

The Team Collection dialog is not its own window: TeamCollectionDialogLauncher is mounted inside
the main app's single React root (CollectionsTabPane.tsx:694), and ErrorBoundary.tsx is
imported nowhere, so any throw during the dialog's render unmounts the whole tree — top bar, book
list and all.

The throw

The dialog opens on the History tab, whose CollectionHistoryTable renders
<BloomAvatar email={e.UserId}> per row. HistoryEvent.UserId is a nullable SQLite column
([Column("userid")]), and older Bloom versions did leave it null. HandleGetHistory uses plain
JsonConvert.SerializeObject with no NullValueHandling.Ignore, so those rows arrive as
"UserId":null, and BloomAvatar passed that straight into getMd5, whose first statement is
message.length — TypeError, blank window.

The collection that prompted this has 88 history events across 12 history.db files, three of
which have a NULL userid (two written by 6.0.2, one by 5.6.7). Any one of them is enough.

The fix

Guard the email before hashing it. Guarding inside BloomAvatar rather than at the one bad call
site covers every present and future caller, and matches where master's equivalent guard lives:
BL-16514 rewrote this component to fetch avatars from Bloom's own server behind
props.email ? ... : undefined, which is why 6.5 does not have the bug. That commit is a
~1500-line login/avatar feature depending on server endpoints 6.4 does not have (AccountApi,
AvatarApi, AvatarCache), so this writes the guard by hand instead of backporting it.

An undefined md5Email is what react-avatar already expects for "no gravatar to look up":
GravatarSource.isCompatible is !!email || !!md5Email, so the gravatar source is skipped and
the fallback chain proceeds — generated initials from name, or the placeholder glyph when the
name is empty too (which is the case for these rows). For call sites that pass "", the only
change is that we no longer fire a request for md5("") that always 404s.

Testing

• Manually verified with the reporting collection: the dialog now opens on the History tab and
  lists all 88 events, including the three legacy rows, with the rest of the window intact.
• yarn lint clean (0 errors). Full front-end suite green: 532 passed, 5 skipped.
• No C# suite run — the diff is one .tsx file that no C# test reads or builds.

Not fixed here (deliberately)

ErrorBoundary.tsx exists and is used nowhere, on this branch and on master. That is what turned
one null into "quit the program". Master shares the weakness — 0b405085a (BL-16209) was the same
failure mode from a different trigger. Worth its own card rather than widening a stable-branch fix.

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16757

Devin review

---

This change is 

[StephenMcConnel]

[Claude Opus 5 (1M context)] Consulted Devin on 2026-08-26 18:18 UTC up to commit a05c97d65.

Devin's review finished clean: no bugs, no Investigate flags, and no informational items. Nothing to mirror. CI (pr-automation, track / set-waiting) passed.

[greptile-apps]

Greptile Summary

This PR prevents missing legacy Team Collection user IDs from crashing avatar rendering and blanking the main React window.

• Allows BloomAvatar to accept nullable email values and skips MD5 generation when no email exists.
• Updates the Team Collection history response type to accurately represent nullable UserId values.

Important Files Changed

Filename	Overview
src/BloomBrowserUI/react_components/bloomAvatar.tsx	Safely bypasses Gravatar hashing for null, undefined, or empty email values while preserving avatar fallback rendering.
src/BloomBrowserUI/teamCollection/CollectionHistoryTable.tsx	Aligns the history event TypeScript contract with the nullable user ID returned for legacy records.

_{Reviews (2): Last reviewed commit: "Say why UserId uses null where we normal..." | Re-trigger Greptile}

[StephenMcConnel]

[Claude Opus 5 (1M context)] Re Greptile's point — "the exported email prop remains typed as an always-present string":

Agreed, and it's the more interesting half of this bug. The type lie starts one level further up: IBookHistoryEvent in CollectionHistoryTable.tsx declares UserId: string for a C# column that is genuinely nullable ([Column("userid")] public string UserId in History/HistoryEvent.cs). That declaration is precisely why the null sailed past typechecking into a render. Widening BloomAvatar's email alone would describe the component honestly but still leave the caller handing it a value TypeScript believes is a string.

Not changed here on purpose: this targets the 6.4 stable branch and the fix was deliberately scoped to the runtime guard. The type question is going to the developer as a follow-up decision rather than being folded in silently — noting it here so it is on the record either way, and so a human reviewer doesn't have to re-derive it.

No runtime consequence in the meantime: the guard makes the component correct for null, undefined and empty-string emails regardless of what the annotation claims.

[StephenMcConnel]

[Claude Opus 5 (1M context)] Consulted Devin on 2026-08-26 18:49 UTC up to commit 130e733ca.

Re-reviewed after the two follow-up commits (the type widening and its explanatory comment). 0 bugs, 0 Investigate flags, 1 informational — so nothing mirrored as a review thread, but recording the informational item rather than reporting it as absent:

UserId typed as string | null vs undefined convention — src/BloomBrowserUI/AGENTS.md prefers undefined over null. UserId: string | null deviates, but accurately models the JSON, which arrives as literal null from the nullable SQLite column, so it is defensible rather than a violation.

Assessed and kept as string | null, for the reason Devin itself lands on: this interface describes JSON we don't control, the field really does arrive as null, and typing it undefined would invite an === undefined test that never fires — the same shape of mistake as the plain string that let this crash through originally. 130e733ca adds a comment at the declaration so the deviation reads as deliberate.

Earlier commits this run: a05c97d65 (also 0 findings).

[StephenMcConnel]

[Claude Opus 5 (1M context)] Note on Greptile coverage, so its earlier pass isn't mistaken for a review of the current code.

Greptile reviewed a05c97d65 and passed. It has not re-reviewed the two commits since (124b7cad7, 130e733ca): its summary above still reads "Reviews (1) — last reviewed commit a05c97d", and no Greptile check context was reported for the new HEAD. Waited ~40 minutes. Its own "Re-trigger Greptile" link needs a browser session, so I couldn't fire it from here — anyone reviewing can click it from the summary comment above.

What it hasn't seen is small and low-risk: two type declarations widened to admit null, plus comments. No runtime change, and tsc reports the same 1107 pre-existing errors as before with none in either file. Devin did re-review the final HEAD (130e733ca) and found 0 bugs and 0 Investigate flags.

Worth noting that the follow-up commits exist because of Greptile's one finding on the first commit — the email prop being typed as always-present. That is now addressed.

Nothing is blocked: Version6.4 has no branch protection, so the missing context is not a required check (the combined status reads pending only because Greptile never reported one).