Add analytics for the features new in 6.5 (BL-16716) by JohnThomson · Pull Request #8215 · BloomBooks/BloomDesktop

JohnThomson · 2026-08-18T21:34:02Z

Problem

Bloom's 6.5 features shipped essentially blind. All 41 existing analytics events came from C#, so
anything implemented in React could only be measured by inventing a bespoke endpoint for it —
which in practice meant it wasn't measured. Nothing under Publish/Rab reported anything at all,
the AI image editor and the new image chooser reported nothing, and Change Picture — the one
event covering how a picture gets into a book — was bypassed by both new 6.5 image routes, so
it had been quietly under-counting.

A second, subtler problem made all of this hard to fix safely: DesktopAnalytics is constructed
with allowTracking: false in DEBUG and acts on that inside its own Track method, so a new
event was easy to write, easy to believe in, and impossible to observe without shipping to alpha.

Fix

The test applied to every candidate was whether the answer would change a decision. Counting
use of a capability we already know is essential does not; nor does measuring a step whose
direction is settled. What does is data that splits a known behaviour into actionable parts.

Plumbing

POST analytics/track plus a trackEvent() helper, so front-end code can report an event at
all. C# fills in BookId, since React in the edit view does not know it. Not the branding: every
event has always carried that as BrandingProjectName, an application property set per
collection, and it holds the richer subscription descriptor rather than the branding-folder key.
BloomAnalytics wraps DesktopAnalytics and logs every event before handing it on — to the
Bloom log always, and to stderr when tracking is off, so a developer running ./go.sh watches
events scroll past. All 53 call sites go through it, and build/check-csharp-analytics.sh keeps
it that way: a partial log would be worse than none, since a missing line would mean "not
instrumented" as readily as "did not happen".

The events, by what they answer

Where pictures come from, and whether searches succeed — Image Search, Image Chooser Closed, Image Source Unavailable, Pixabay Key Saved, Image Preview Slow, and Change Picture with source/provider from all four routes (repairing the under-count on the way
past). Result counts can't answer the real question: Pixabay and Openverse nearly always return
some pictures, so the failure is a full page of wrong ones, and only what the user did next
reveals it.
Whether the AI editor's funnel converts, and what it costs — open, generate, commit,
cancel, key-saved, unavailable. Generation is reported by the editor itself over a new bridge
message; a session that generates and then throws everything away is our clearest quality
signal.
Two things we cannot otherwise evaluate — every explicit image-transparency override, each
of which is a user telling us which way our line-art detection failed; and custom cover layout,
a Pro-gated feature that has shipped two releases with no usage data, including the demand we
refuse and why.
Whether the RAB publish path works on real machines — nothing else watches it, since CI
never runs a real build.
Metadata-apply duration on large books, and whether users correct the reading direction
ethnolib chose from their script. That last pair is a ratio -- how often does the direction
derived from a script get overridden -- so both halves are reported at the same moment: when
the user accepts Collection Settings, not when a sub-dialog closes on a value Cancel can still
throw away.

No free-form user text is collected. Program errors go to Sentry, not Segment. AI prompt text is
excluded by name, since it carries sentences lifted out of the book. And image-search terms are
dropped at Bloom's boundary: an earlier round of this branch did send them and we were asked not to,
so a search is now recorded as "someone searched Pixabay in English and got 20 results" with no
record of the subject. The gallery still hands the term to Bloom, so re-enabling would be one
property here and no change there. Two things that costs us, stated so nobody expects them: we
cannot see which subjects people search for and never find, and searchCount counts queries with no
way to tell one idea tried in three languages from three different ideas.

Also registers bloom-image-gallery in the dev-libraries registry, so
./go.sh --with bloom-image-gallery works for developing the two repos together.

Two properties worth stating, because reviewers found places where neither was true.

Recording an event cannot affect what it is observing. The reporting is isolated at every
boundary it crosses. The worst of the four cases found would have turned a successful,
already-paid-for AI generation into a reported failure and lost the image.

An event describes what happened, not what was about to happen. Four events were being sent at the
moment a decision was made rather than the moment it took effect, so a user who backed out was counted
as though they had not -- inflating exactly the numbers those events exist to provide. All four are
fixed. The language and reading-direction pair now report when Collection Settings is accepted rather
than when a sub-dialog closes on a value Cancel can still discard; closing the AI editor while a commit
is in flight no longer counts the session as thrown away as well as committed; and the AI commit's
applied count -- with the per-picture Change Picture events it drives -- moved out of C# into the
overlay, because C# stages a swap on the page being edited and hands it to the browser to make, so only
the browser ever learns whether it landed. For the ordinary journey, editing the picture you
right-clicked, that meant the count was always "1 applied, 0 failed" whatever became of it.

A third property had to be learned as well: an event must not be able to describe one thing as two.
Five separate ways a single AI editing session could appear in both the "committed" and the "thrown away"
figures -- or twice in one of them -- were found and closed, all in the AI editor's session bookkeeping.

Chasing these turned up two user-facing bugs, neither of which this work introduced. Applying image
credits to a whole book re-read the embedded metadata of every picture once per picture -- a lazy
sequence asked for its count inside the loop -- so a 400-picture book did roughly 160,000 metadata reads
instead of 400 and appeared frozen for minutes; and worse, the loop writes metadata into those files as it
goes while the re-enumeration re-decides which to include, so the total it divided by could shift
underneath it. And:
answering the AI editor after the user has closed it throws, because the iframe is detached, and that
ack is the first statement of a finally block -- so the throw skipped the save of the page the new
pictures had just landed on. A user who closed the window while their picture was being applied could
lose a picture they had already paid to generate. A stale commit reply could also tear down a
relaunched editor. Both are fixed, with tests.

Both dependencies are merged and in

Nothing is pinned to a branch or to a stale build any more:

bloom-image-gallery — Save page class changes (such as addition of bloom-trilingual) #20 is merged; this tracks that repository's default branch again,
resolving to the merge commit (0.0.4).
bloom-ai-image-tools — Replace MessageEvent with ajax call in reader setup #2 is merged and dist-v0.1.4 published from it; the pin has moved
off dist-v0.1.3, which predated the editor's own reporting. That is what lets AI Editor Generate
reach Bloom at all: the bridge, the allow-list and the session counting were already here, with
nothing sending to them.

pnpm install --frozen-lockfile accepts the lockfile, and the installed editor build is 0.1.4 with
AI Editor Generate present in its bundle.

Still unverified end to end: nobody has driven the AI editor by hand and watched that event appear
in Bloom's event log. It is first on the tester's list on the card.

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

Devin review

This change is

Bloom's 6.5 features were shipping blind. Every one of our 41 analytics events came from C#, so anything implemented in React could only be measured by inventing a bespoke endpoint for it -- which in practice meant it wasn't measured. Nothing under Publish/Rab reported anything, the AI image editor and the new image chooser reported nothing at all, and the one event that did cover choosing a picture ("Change Picture") had been bypassed by both new image routes, so it was quietly under-counting. The guiding test for what to instrument was whether the answer would change a decision. Counting use of a capability we know is essential does not; nor does measuring a step whose direction is already settled. What does is data that splits a known behaviour into actionable parts: which source a picture came from, whether a search ended in an accepted image, which way a heuristic guessed wrong, which gate turned a paying customer away. The plumbing: - POST analytics/track plus a trackEvent() helper, so front-end code can report an event at all. C# fills in BookId and the collection's branding, since React in the edit view knows neither and almost every question is worth asking per project. - BloomAnalytics wraps DesktopAnalytics and logs every event before handing it on. DesktopAnalytics decides not to send in a DEBUG build, and decides it inside its own Track method, so a new event used to be impossible to observe without shipping to alpha. All 53 call sites now go through the wrapper, and build/check-csharp-analytics.sh keeps it that way -- a partial log would be worse than none, since a missing line would mean "not instrumented" as readily as "did not happen". The events, by what they answer: - Where pictures come from and whether searches succeed: Image Search, Image Chooser Closed, Image Source Unavailable, Pixabay Key Saved, Image Preview Slow, and Change Picture with source/provider from all four routes (fixing the under-count on the way past). - Whether the AI editor's funnel converts, and what it costs: open, generate, commit, cancel, key-saved, unavailable. - Two things we cannot otherwise evaluate: every explicit image-transparency override, which tells us which way our line-art detection failed; and custom cover layout, a Pro-gated feature that has shipped two releases with no usage data, including the demand we refuse and why. - Whether the RAB publish path works on real machines, which nothing else watches -- CI never runs a real build. - Metadata-apply duration on big books, and whether users correct the reading direction ethnolib chose from their script. Deliberately excluded: program errors (they belong in Sentry), and prompt text from the AI editor, which can carry arbitrary content lifted from the book. Image-search terms are in, deliberately: short queries typed at a public art library, and the term is what makes the rest of the data mean anything. Also registers bloom-image-gallery in the dev-libraries registry, so `./go.sh --with bloom-image-gallery` works for developing the two repos together. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Greptile found this class of bug in both companion PRs, which prompted looking for it here -- and Bloom had it twice over. trackEvent used a plain postJson, which meant: - Anything it threw would be caught by the caller's own try/catch, which generally means something else entirely. In the image chooser that catch shows "Sorry, there was a problem adding the image" -- so a hiccup in analytics would have told the user their picture had failed to import, for a picture that had in fact been imported. - A failed request went through wrapAxios's default error reporting, which raises a problem report. An analytics endpoint returning 404 (no project open, say) is never worth interrupting anyone for. Recording an event now cannot break, or appear to break, whatever the user was doing. Both failure paths still log to the browser console, so a genuinely broken endpoint is still findable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Four bugs and three of the four Investigate flags from Devin's review. All of them were in code this PR added, and three would have corrupted the data the events exist to provide: - App Builder: reporting the outcome ran inside the try/catch that decides whether the action succeeded, and it calls GetStatus(), which reads and parses files. An I/O hiccup there would have written "the build failed" to the log of a build that finished fine. BloomAnalytics.Track now swallows its own exceptions too, so this cannot happen at any of the 53 call sites. - "Add this info to all images" scanned every picture twice: GetImagePaths reads embedded metadata from every file, and counting them up front ran before the progress dialog appeared. On the 400-image books this measurement was added for, it made the wait longer in order to measure it. The count now comes back from the work itself. - The transparency-choice history was keyed on the image's src, which the change being recorded rewrites (setImgTransparentParam adds ?transparent=yes), so the path restarted on the first change -- losing exactly the users who cycle through the options, the ones it exists to find. Keyed on the file now, and "from" is read from the classes at click time rather than from state captured when the menu was built. - The overlay forwarded any event name the editor iframe sent. Bloom is what actually posts to Segment, so it now enforces its own privacy line with an allowlist instead of trusting a sibling repo not to regress. - The image chooser's new mount effect uses the project's useMountEffect helper, and guards its durable counter against StrictMode's double invocation, which would otherwise have double-counted every developer's visit. - onPickLocalFile stamps providerId itself, so the "local disk" split no longer depends on the gallery package continuing to stamp it. Also makes check-csharp-analytics.sh ignore commented-out calls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two bugs and two flags, all in code this PR added: - "Cover Layout Changed" counted a switch nobody made. setupPageLayoutMenu posts the toggle endpoint by itself when a legacy theme cannot support a custom layout, forcing the page back to standard as it opens. That reached the same handler as a real choice, so the figures over-counted "standard" -- and did so precisely on the books where custom had been wanted. The request now says whether a user initiated it, and only those are reported. - A nested ternary built the rtlFromEthnolib property, which AGENTS.md explicitly forbids. Spelled out as an if, with a note that "unknown" is a real answer distinct from left-to-right. - BloomAnalytics.ReportException is now guarded like Track. It is called from Program's global unhandled-exception handler, where a throw would have been a failure while reporting a failure. - The event-name allowlist added last round did not constrain the properties riding with each event, so a new property holding prompt text could still have flowed through. Each allowed event now declares exactly which property names may accompany it, and anything else is dropped. The editor's promise not to send prompt text is made in another repository; this is what makes it true on the side that actually posts to Segment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…L-16716) Devin, on the previous fix. The transparency-choice history was keyed on the image file, but a page image's src is just its bare name relative to the book folder -- "placeholder.png", "aor_AOR_ABC.png". The map is module-level and lives for the whole run of Bloom, so two books, or two pages, using the same file appended to one another's history. The reported path could then describe a sequence no single picture ever went through, overstating how much users were guessing -- the opposite of the error the previous fix removed. Keyed on page id plus file now, which is unique across books since page ids are GUIDs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-08-18T22:28:15Z

[Claude Opus 5 (1M context) via John Thomson] Consulted Devin on 2026-08-18 22:28 UTC, up to commit 1c83144.

Four rounds over this branch. It found 6 real defects in the new analytics code and we fixed all of them — the App Builder reporting that could describe a finished build as failed, the image-credit command that scanned every picture twice before showing progress, two bugs in the transparency-choice history key, a cover-layout event that counted a switch Bloom made for itself, and a nested ternary against AGENTS.md. It also prompted two hardening changes: an allowlist for what the AI editor iframe may report, and the same exception guard on ReportException that Track had.

Each finding has its own thread above with the outcome recorded. Two threads are deliberately left open for the developer: the dependency pin that must be reverted before merge, and the AI-tools tag that has to be published before generate events appear.

CodeRabbit is configured off in this repo (.coderabbit.yml: auto_review.enabled: false) and Greptile is not enabled on it, so Devin was the only bot reviewing here. Note that BloomDesktop's build/test CI runs in TeamCity and does not report to GitHub checks — the only build and test evidence for this PR is the local suites, recorded in the preflight report.

…BL-16716) Devin, on the allowlist added two commits ago. It was an object literal, and `event in obj` is true for inherited members -- so an event named "toString" or "constructor" was treated as permitted, then threw while its properties were filtered. That aborts the handler for that message, and it is the same handler that processes commit and cancel, so a stray name could have taken out the editor's ability to hand images back. A Map now, whose has() only sees real entries. Two tests pin it: an unknown event name is ignored without breaking the session, and a permitted event drops any property not on its list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

confirm.trx was output from a diagnostic test run of mine and has no business in the repository. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The comment beside the new Cover Layout Changed event explains that the endpoint is a toggle and the menu does not guard against re-picking the ticked option, so a "standard" event can come from someone who clicked Custom. Per John's decision that menu bug is not fixed here -- folding a user-facing behaviour change into an analytics PR is how a reviewer loses track of what they are approving -- so it is filed as BL-16725, and the comment now says so. Reading the code, that bug is worse than the analytics caveat it causes: switching off Custom deletes the saved custom layout, with no confirmation and no undo. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…716) Devin's finding on the previous head. "Collection Language Rtl Overridden" was sent the moment the Script Settings sub-dialog closed, but that dialog only edits PendingLanguages: nothing is saved until the user accepts Collection Settings. So a correction the user then abandoned with Cancel was counted as if they had made it, and one they made and undid was counted twice -- inflating precisely the number the event exists to provide (which scripts ethnolib gets the direction wrong for). The reading direction the script produced is now captured before the sub-dialog opens, once per language, and the event is sent from the OK handler by comparing the value the user ended up with against that baseline. The baseline carries its language tag, because the user can replace the language in the same dialog session, and comparing a new language's direction against the old one's would invent an override nobody made. GetRtlOverridesToReport holds the decision and is internal and static, like UpdateLanguageSettings beside it, so the five cases that matter are unit tested: corrected, corrected and undone, never opened, language replaced afterwards, and two languages at once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Devin, and the last of a run of findings in this one area -- each fix exposing the next interaction, which is itself worth noticing. cleanup deferred the cancel decision while any commit was outstanding, but reportCancel did not check that for itself. So with two commits in the air and the user closing: commit A comes back a failure and reports the cancel, then commit B succeeds and reports a commit with a picture applied. One session in both figures. The outstanding-commit test now lives inside reportCancel rather than at its call sites, because every caller needs it and the deferred ones are the easy ones to get wrong. Each reply decrements the count before calling, so whichever settles last is the one that reports -- and cleanup can simply call it. Removing that one condition now fails four tests rather than one, which is the point: the rule has one home instead of three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-08-19T17:47:56Z

[Claude Opus 5 (1M context) from John Thomson's machine during preflight]

Consulted Devin up to 3d8d09de. Ten jobs this run, and it earned every one.

Moving the AI commit's reporting into the browser (John's decision) set off the longest review chain of this branch. Devin found eight bugs, each visible only once the previous was fixed, and each has a resolved thread:

a partly-successful commit counted as thrown away as well as committed;
a commit reported twice when the reply path threw;
an in-flight flag a second overlapping commit could clear too early;
a double decrement of the count that replaced it — a defect I introduced — which could leave a session permanently unreportable;
the first of two overlapping replies deciding the cancel before the second landed its picture;
a refused cover layout blamed on the wrong cause while the subscription lookup was in flight;
a property value that parses as a date-time recorded in the local culture's format instead of as sent;
and, at "severe" and nothing to do with analytics, applying image credits to a whole book re-reading every picture's metadata once per picture — roughly 160,000 reads instead of 400 on a 400-picture book.

Two of its findings were partly wrong and worth recording as such: the GIF route it called unreported is in fact reported (just mislabelled, now fixed), and its date example ("2024-01-01") does not reproduce — Newtonsoft leaves date-only strings alone, so the reachable case is a full timestamp, which is what the tests use.

Its final pass raised one more, of the same narrowing kind. I stopped at a clean state rather than an empty one; the report explains why and asks whether that was right. Everything still listed as "current" on Devin's page is either stale or one of the two open decisions.

Read that page with the usual care: it re-lists every bug it has ever raised here, including the twenty-one already fixed.

…6716) bloom-ai-image-tools#2 is merged and dist-v0.1.4 published from it, so the pin moves off dist-v0.1.3 -- which predated the editor's own reporting. This is what makes "AI Editor Generate" actually reach Bloom: the bridge, the allow-list and the session counting have all been in place on Bloom's side, with nothing sending to them. Verified beyond the pin resolving: the installed dist-app really is 0.1.4 and its bundle contains the "AI Editor Generate" string, and the published tag records source-commit b250158 on master. So the build is from the merged code, not a stale one. The lockfile also shows some (supports-color) peer-context keys flipping. That is not this change: successive pnpm installs on this tree keep producing either keying, and an earlier commit on this branch flipped a different set the other way. Net against master, the only real differences are these two dependencies. Gates against the new pin: typecheck, lint, C# (3143), vitest (731) and the production bundle all pass, and pnpm install --frozen-lockfile accepts the lockfile. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Comment only. John's sign-off on the one question this branch left open, recorded where the next person to wonder about it will be standing rather than in a review thread they will never see. Sending the term is deliberate: users are told Bloom collects analytics, we take care that what we collect is not personally identifiable, and these are the same one- or two-word queries the user is simultaneously sending to a public image service. AI prompt text stays excluded by name, for the opposite reason. Two things the comment preserves for whoever revisits it. Searching a local collection needs no network at all, so those terms could in principle stay on the machine while online ones are reported -- John raised that, and the split is available if we ever want it. And the decision was "stick with this unless someone complains", which names the trigger to reopen it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…BL-16716) John's real-world run showed "branding=Test" on the events that came through the analytics endpoint and NOT on the Change Picture from the same user action, which comes from C#. Chasing that inconsistency turned up something better: the property should not exist at all. CollectionSettings.SetAnalyticsProperties already hands the branding to DesktopAnalytics as an application property -- "which will go out with every subsequent event", as its own comment says -- so every event, C# and front-end alike, has carried it all along as "BrandingProjectName". And that is the better value: the subscription DESCRIPTOR, which also encodes the tier, any flavor and the individual subscriber, where the BrandingKey this endpoint was adding normalizes all of that down to a branding folder name ("Acme-LC" becomes "Local-Community", "Steve-Trainer" and an empty descriptor both become "Default"). So this was a second, coarser name for a dimension we already had, present on some events and not others. Removed from the endpoint's auto-fill and from Cover Layout Changed, with a comment where it was explaining why not to re-add it -- including the trap that made it look missing: SetAnalyticsProperties returns early when tracking is off, and BloomAnalytics logs only per-event properties, so a developer build shows no branding on any line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-08-19T23:03:45Z

[Claude Opus 5 (1M context) from John Thomson's machine during preflight]

A property removed, on the strength of a real run. 57a03c24.

John exercised the image chooser for real and the log showed branding=Test on the two Image Search events and on Image Chooser Closed, but not on the Change Picture from the very same action — because that one is emitted from C# (ImageGalleryApi), while the others came through analytics/track, which was auto-filling it. One user action, one property, inconsistently present.

Chasing that turned up something better than the inconsistency: the property should never have existed. CollectionSettings.SetAnalyticsProperties already hands the branding to DesktopAnalytics as an application property — "which will go out with every subsequent event", as its own comment says — so every event, C# and front-end alike, has carried it all along as BrandingProjectName. And that value is the better one: the subscription descriptor, which also encodes the tier, any flavor, and the individual subscriber, where the BrandingKey I was adding normalizes all of that down to a branding folder name (Acme-LC → Local-Community; Steve-Trainer and an empty descriptor both → Default).

So it was a second, coarser name for a dimension we already had. Removed from the endpoint's auto-fill and from Cover Layout Changed.

Worth recording why this was easy to get wrong, since the comment now guards it: SetAnalyticsProperties returns early when tracking is off, and BloomAnalytics logs only the per-event properties. So on a developer build no line shows any branding — which looks exactly like it being absent in production. I read that as a gap, proposed adding the property to ~30 more events, and only found the truth when I went looking for how to reach the collection from a static class. The comment now says all of this, and says not to re-add it.

Gates after the removal: C# 3143 passed, vitest 731 passed, typecheck and lint clean, BloomExe builds.

We have been told not to include search terms in analytics, reversing the decision recorded here a day ago. The term was the only free-form user text this instrumentation ever carried, so with it gone there is none. Dropped at Bloom's boundary rather than upstream: the image gallery still hands us report.term, because if this is revisited the change is adding one property back and nothing in the gallery has to move. That is what John asked for. Also gone is acceptedTerm on Image Chooser Closed, and the ref that existed only to feed it. Which SOURCE satisfied the user is what makes that event a success rate, and a provider id is not user text. The comment where the term used to be now says what the remaining properties can and cannot answer, because two things really are lost and a later reader should not have to work that out: the commissioning signal (which subjects people search for and never find, the original argument for sending it), and the ability to tell one idea tried in three languages from three different ideas -- so searchIndex and searchCount now count queries and nothing finer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

greptile-apps

Your trial has ended. Reactivate Greptile to resume code reviews.

Comments and test data only; no behaviour change. The previous commit stopped sending the term, and a claim left behind in the code is worse than no claim: the next reader believes it. - BloomAnalytics's remarks used a search term as its example of a value that might contain a brace. A search provider's error message is the true example now, and still makes the point: we pass that along as we received it. - AnalyticsApi's date-parsing remarks said several property values are free user text, "an image search term above all". None of them are user text any more; what remains is text whose SHAPE we do not control -- a provider's error, a model name, an id from someone else's API -- which is the same reason not to rewrite it. - BloomAnalyticsTests used term=dog as its sample property and asserted that search terms "reach us verbatim". Now resultCount and a provider error message, so the tests no longer document a policy we have reversed. Also corrected outside the code: the tester's notes on BL-16716 now say what you type is NOT reported and that seeing it would be a bug, the PR description says the same, and so does the decision report. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

greptile-apps

Your trial has ended. Reactivate Greptile to resume code reviews.

Two things John raised reading the overlay. "Editor" means the Edit tab to anyone who works on Bloom, so a bare "the editor" in this file reads as the wrong thing however dedicated the file is to the other one. Comments now say "ai-editor" wherever the AI image editor is meant. The capitalised "AI Image Editor" survives only where it names the feature to a reader; "the page being edited" and the page frame are untouched, since those really are about Bloom's editing view. And the property allow-list is gone. Its stated purpose was keeping prompt text out, but we control both ends of this channel, so it was guarding us against ourselves -- and AGENTS.md is explicit that this is not where to be defensive. If something specific must not be forwarded, the place to stop it is in the ai-editor or by removing that one property by name. Today it sends only ids, enums, numbers and booleans; I checked the merged ai-editor source rather than assuming. The event-NAME check stays, as a Set rather than a Map now that the values are gone. That is a different concern: an unrecognized name would create a new event type in our analytics rather than land in an existing one, so the vocabulary is worth pinning even though the properties are not. Say the word if you would rather that went too. The test that pinned the filtering now pins the opposite -- properties arrive as the ai-editor sent them -- and the "toString" test still stands for the name check, which is what it was really about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

greptile-apps

Your trial has ended. Reactivate Greptile to resume code reviews.

JohnThomson

[Claude Opus 5 (1M context)] For anyone reading this review: the three notices above mean Greptile's trial has expired, and that changes how to read the reviewer coverage here.

The notices are from today (13:54, 14:06 and 14:58 UTC), one per push. So Greptile is installed on this repository and has been attempting to review each commit, but it has produced no review on this PR at all — its trial ran out first. Its silence here is not approval; it is a bot that has stopped running. Worth saying because those two states look identical from the outside, which is exactly the trap this project's review process warns about for Devin.

Where it did work, earlier and on the package repositories, it earned its place: Greptile found the single most valuable defect in this whole branch — analytics callbacks sitting inside the control flow they observe, which in the AI editor would have lost a generation the user had already paid for. That prompted a look at Bloom, which had the same fault twice over. Those passes were before the expiry and stand.

So the effective reviewer panel for the commits since yesterday is Devin plus the local suites, not Devin plus Greptile. If Greptile is worth reactivating, this branch is decent evidence for it.

Every line-level discussion on this review is a Devin finding I mirrored here, and each one already carries its outcome — the fix and its commit, or the reasoning for leaving it. I have set my own disposition on those rather than posting a reply that would repeat what the thread already says; resolving them is yours, not mine.

@JohnThomson+AGNT made 1 comment and resolved 34 discussions.
Reviewable status: 0 of 56 files reviewed, all discussions resolved.

greptile-apps

Your trial has ended. Reactivate Greptile to resume code reviews.

andrew-polk

Not done yet, but publishing a few things I've written already. It may spawn a discussion.

@andrew-polk reviewed 29 files and all commit messages, and made 6 comments.
Reviewable status: 29 of 56 files reviewed, 5 unresolved discussions (waiting on JohnThomson).

src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts line 25 at r15 (raw file):

//      ai-editor posts `ready`; we post `init` (the launch reply + the right-clicked
//      image as selectedBookImageId). Image bytes never ride postMessage — they go
//      over HTTP via aiImageEditor/file; the ai-editor references results by id.

I can't say I agree that these comment changes are an improvement.
Seems to reduce clarity.
(same below)

src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts line 61 at r15 (raw file):

// real entries.
const kAnalyticsEventsTheAiEditorMaySend = new Set<string>([
    "AI Editor Generate",

I think all these wants to be "AI Image Editor..." rather than "AI Editor...".

Did you consider a different model such that the event is something like "AI Image Editor" and the properties carry the specific action? Then the editor could add new things to track without any more plumbing. There would be both the pro and con that the analytics database would have one table with multiple event/actions in it. Over all, I would actually see that as a pro, for multiple reasons.

But as I continue through this file I see there are probably lots of complications with this...

Happy to discuss.

src/BloomExe/Collection/CollectionSettingsDialog.cs line 80 at r15 (raw file):

        /// baseline for that slot meaningless.
        /// </summary>
        internal class RtlBaseline

There is a LOT of fanfare around getting this one analytic point of data. (At least, I think this is all just we can say if we are overriding the current rtl setting?)

I don't think it is worth it for a few reasons:

I can't think what we would do with the data
This whole part of the system is going to be reworked in 6.6.
It adds a lot of complexity.

src/BloomExe/Edit/EditingModel.cs line 1867 at r15 (raw file):

            PageEditingModel.ImageInfoForJavascript args,
            string source,
            string provider

The comments are definitely helpful, but seems like we could probably come up with better, more descriptive names than source and provider which are too similar.

src/BloomExe/Edit/EditingModel.cs line 2012 at r15 (raw file):

                    { "BookId", CurrentBook.ID },
                }
            );

This is not a new feature, so it seems like it requires a higher bar to get into this PR.
I don't see it meeting that.

andrew-polk · 2026-08-20T23:05:54Z

src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx line 160 at r15 (raw file):

            providers: [...providersTriedRef.current].join(","),
            pixabayKeyPresent: pixabayKeyPresentRef.current,
            priorChooserSessions: priorChooserSessionsRef.current,

I'll be honest, I'm overwhelmed by the amount of complexity we're adding for all these events.

It is hard to know how much thinking you've put into this, so I'm not sure how much to push back, but the whole thing feels way overdone.

As one example, priorChooserSessions requires a new user variable to store how many times we have ever been in the chooser, right? Lots of plumbing but I don't see any value in it.

andrew-polk

@andrew-polk reviewed 9 files and made 9 comments.
Reviewable status: 38 of 56 files reviewed, 15 unresolved discussions (waiting on JohnThomson).

src/BloomBrowserUI/utils/bloomApi.ts line 778 at r15 (raw file):

new events have to be verified on alpha

With BloomAnalytics, that's no longer true, right?

src/BloomExe/BloomAnalytics.cs line 28 at r15 (raw file):
This paragraph can also be dropped.
The first line is wrong (this is the class, not the method).
Maybe it is worth stating something like

All analytics traffic should route through this class. build/check-csharp-analytics.sh attempts to enforce that at commit time.

src/BloomExe/web/controllers/AiImageEditorApi.cs line 449 at r15 (raw file):

demoOnly

I would name this as isPlaygroundBook. I don't think I would guess what demoOnly means.

src/BloomExe/BloomAnalytics.cs line 17 at r15 (raw file):

    /// Analytics.Track. So a new event is easy to write, easy to believe in, and impossible to
    /// see: nothing is sent, and nothing says that nothing was sent. The only way to confirm one
    /// was to ship it to alpha and wait.

I don't think this paragraph is helpful, nor actually accurate. The way to test is to change the code temporarily to send analytics to the test space.
I would just drop it.

src/BloomExe/BloomAnalytics.cs line 49 at r15 (raw file):

                    Analytics.Track(eventName);
                else
                    Analytics.Track(eventName, properties);

In all cases, shouldn't the Segment call precede the Log call? Else a problem with logging would prevent all analytics.

src/BloomExe/web/controllers/AiImageEditorApi.cs line 415 at r15 (raw file):

                // Should never happen in the field. If it does, our packaging is broken and we
                // would otherwise only hear about it from a confused user.
                BloomAnalytics.Track("AI Editor Unavailable");

Doesn't seem worth adding an event for.
Each event is a new database table.

src/BloomExe/web/controllers/AiImageEditorApi.cs line 537 at r15 (raw file):

                    { "cleared", string.IsNullOrEmpty(payload.apiKey) ? "true" : "false" },
                }
            );

Could/should this event be folded in to the commit event?

Doesn't feel worth a full event.

src/BloomExe/web/controllers/AiImageEditorApi.cs line 1233 at r15 (raw file):

            // it gets our per-slot results below, the page frame tells it how many current-page
            // swaps landed, and the analytics/track endpoint fills in BookId. (Branding needs no
            // property: every event already carries "BrandingProjectName" -- see AnalyticsApi.)

Most of this is bloat. Do we care about prior bugs we fixed?

src/BloomExe/Publish/Rab/RabPublishApi.cs line 62 at r15 (raw file):

        /// support traffic. BL-16469 improved the error messages; this is how we find out which
        /// errors people actually hit.
        /// </summary>

Most of this summary is bloat. I would drop all but the first line.

JohnThomson · 2026-08-24T18:50:58Z

We decided to implement a reduced set of analytics in a new pR

JohnThomson · 2026-08-24T19:01:02Z

[Claude Opus 5 (1M context) from John Thomson's machine]

Andrew — this push-back was right, and both halves of it have been acted on. The work now lives in
#8228; this PR is closed, and its branch is parked so nothing is lost.

On priorChooserSessions specifically: gone. So is the user variable behind it, the read and
write on every chooser open, and a React StrictMode guard that only existed because that write
happened on mount. You were right twice over, as it turns out. Its real purpose was the Pixabay-key
funnel — "a key fetched on the first visit is a speed bump, one that takes eight visits is a
barrier" — and that event is also gone, which left the field answering a much weaker question. And
that weaker question is one we can already answer without it: Bloom identifies users to Segment, so
"how many times has this person opened the chooser before" is just a count of their earlier close
events. With it removed, the PR persists nothing at all on the user's machine.

On the general point: 22 events became 7. Thirteen event names dropped outright, four merged
into two, and the field count went from 73 to 38. What went includes the per-search event, the
Pixabay key, the blocked-cover-layout pair, both collection-language events, the "apply info to all
images" timing, and the App Builder's prepare stage.

What survives is the set where we could name a decision the answer would change:

Image Chooser Closed — whether searching actually ends in a picture. A result count can't answer
it: Pixabay and Openverse nearly always return something, so the failure is a full page of wrong
pictures. The first real run showed Openverse returning 240 results with none accepted.
Change Picture — one new field naming the route, which also repairs a count that both new 6.5
image paths were bypassing.
AI Image Editor Generate / Closed — what generations cost, and whether they reach the book.
Cover Layout Changed — two releases of charging for custom covers with no usage data at all.
Image Transparency Set — the only read we have on whether the line-art heuristic is wrong, and
which way.
Publish App — CI never runs a real Reading App Builder build, so this is that path's only
continuous coverage.

If any of those still looks unjustified to you, say so — they're independent, and dropping one more
is a small change rather than a rework. There's a written record of what each dropped event would
have told us, so reversing any of these is a decision rather than an archaeology exercise.

Three things from his review of #8215 that survived the trim, and he was right about all three. **Send before logging.** BloomAnalytics.Track logged the event and then handed it to DesktopAnalytics, inside one try/catch. So a failure in our own logging would have stopped the event ever reaching Segment -- the wrong way round, as he put it. Both are now sent first and logged second, each guarded separately, because a single guard around both would have announced a logging failure as a failure to send. The log-side guard is the one deliberately swallowed catch in the file: the channel we would report a logging failure on is the one that just failed. ReportException had the same ordering and gets the same fix. **The class summary claimed something untrue.** It said the only way to confirm a new event was to ship it to alpha and wait. Not so -- you can point analytics at the test space. The paragraph built on that is gone, along with a line that told the reader to "call this rather than Analytics.Track" while sitting on the class rather than the method. His suggested framing replaces both: all analytics traffic should route through this class, and the commit-time check attempts to enforce it. **The same wrong claim in bloomApi.ts.** "New events have to be verified on alpha" was the front-end copy of it. It now says what actually confirms an event fired: the line BloomAnalytics writes to Bloom's log and to standard error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson

@JohnThomson+AGNT made 11 comments.
Reviewable status: 38 of 56 files reviewed, 15 unresolved discussions.

src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx line 160 at r15 (raw file):

Previously, andrew-polk wrote…

I'll be honest, I'm overwhelmed by the amount of complexity we're adding for all these events.

It is hard to know how much thinking you've put into this, so I'm not sure how much to push back, but the whole thing feels way overdone.

As one example, priorChooserSessions requires a new user variable to store how many times we have ever been in the chooser, right? Lots of plumbing but I don't see any value in it.

[Claude Opus 5 (1M context) from John Thomson's machine] This push-back was right and has been acted on; there is a fuller reply as a comment on the PR itself, but in short: priorChooserSessions is gone, and so is the user variable behind it, the read and write on every chooser open, and a StrictMode guard that existed only because that write happened on mount. The PR now persists nothing at all on the user's machine.

On the general point: 22 events became 7, and 73 fields became 38.

src/BloomBrowserUI/utils/bloomApi.ts line 778 at r15 (raw file):

Previously, andrew-polk wrote…

new events have to be verified on alpha

With BloomAnalytics, that's no longer true, right?

[Claude Opus 5 (1M context) from John Thomson's machine] Correct, and that claim was wrong in the same way as the one in BloomAnalytics.cs. The comment now says what actually confirms a new event fired: the line BloomAnalytics writes to Bloom's log (Help > Show Event Log) and to standard error. Fixed in 6ffe7b7 on #8228.

src/BloomExe/BloomAnalytics.cs line 17 at r15 (raw file):

Previously, andrew-polk wrote…

I don't think this paragraph is helpful, nor actually accurate. The way to test is to change the code temporarily to send analytics to the test space.
I would just drop it.

[Claude Opus 5 (1M context) from John Thomson's machine] You were right that it was inaccurate, not just unhelpful — the paragraph claimed the only way to confirm a new event was to ship it to alpha and wait, and pointing analytics at the test space is a perfectly good way. That paragraph is gone. Fixed in 6ffe7b7 on #8228.

src/BloomExe/BloomAnalytics.cs line 28 at r15 (raw file):

Previously, andrew-polk wrote…

This paragraph can also be dropped.
The first line is wrong (this is the class, not the method).
Maybe it is worth stating something like

All analytics traffic should route through this class. build/check-csharp-analytics.sh attempts to enforce that at commit time.

[Claude Opus 5 (1M context) from John Thomson's machine] Dropped, and your wording taken almost verbatim: the summary now says that all analytics traffic should route through this class and that build/check-csharp-analytics.sh attempts to enforce it at commit time. You were also right that the old first line was wrong — it told the reader to "call this rather than Analytics.Track" while sitting on the class, not the method. Fixed in 6ffe7b7 on #8228.

src/BloomExe/BloomAnalytics.cs line 49 at r15 (raw file):

Previously, andrew-polk wrote…

In all cases, shouldn't the Segment call precede the Log call? Else a problem with logging would prevent all analytics.

[Claude Opus 5 (1M context) from John Thomson's machine] Good catch, and yes — in all cases. Logging first meant a failure in our own logging could stop the event ever reaching Segment. Both Track and ReportException now send first and log second.

One wrinkle worth knowing: they are guarded separately rather than sharing one try/catch, because a single guard would have reported a logging failure as a failure to send. The log-side catch is now the one deliberately swallowed exception in the file — the channel we would report a logging failure on is the one that just failed — so it goes to Debug.WriteLine and no further. Fixed in 6ffe7b7 on #8228.

src/BloomExe/Collection/CollectionSettingsDialog.cs line 80 at r15 (raw file):

Previously, andrew-polk wrote…

There is a LOT of fanfare around getting this one analytic point of data. (At least, I think this is all just we can say if we are overriding the current rtl setting?)

I don't think it is worth it for a few reasons:

I can't think what we would do with the data

This whole part of the system is going to be reworked in 6.6.

It adds a lot of complexity.

[Claude Opus 5 (1M context) from John Thomson's machine] Agreed on all three counts, and both collection-language events are gone. The work moved to #8228, which keeps 7 of the 22 events. CollectionSettingsDialog.cs is now untouched by the PR — the RtlBaseline/LanguageChoice plumbing you were reacting to went with them.

src/BloomExe/Edit/EditingModel.cs line 1867 at r15 (raw file):

Previously, andrew-polk wrote…

The comments are definitely helpful, but seems like we could probably come up with better, more descriptive names than source and provider which are too similar.

[Claude Opus 5 (1M context) from John Thomson's machine] Fixed, and by removing the problem rather than renaming it: source and provider are now a single source field. Every provider worth telling apart belonged to one route (the image chooser), so each is a first-class source in its own right — pixabay, openverse, a collection slug, local-disk, clipboard, ai-editor. No confusable pair left.

src/BloomExe/Edit/EditingModel.cs line 2012 at r15 (raw file):

Previously, andrew-polk wrote…

This is not a new feature, so it seems like it requires a higher bar to get into this PR.
I don't see it meeting that.

[Claude Opus 5 (1M context) from John Thomson's machine] Agreed — the Image Metadata Applied To Book event is dropped.

One thing I did keep, and want to flag rather than slip past you: instrumenting that operation is how we found it was re-reading every picture's metadata once per picture — roughly 160,000 reads on a 400-picture book, which is why it appeared to freeze. That fix (a .ToList() in ImageUpdater) stayed; the event that found it did not. If you would rather that fix travelled on its own card, say so and I will move it.

src/BloomExe/web/controllers/AiImageEditorApi.cs line 415 at r15 (raw file):

Previously, andrew-polk wrote…

Doesn't seem worth adding an event for.
Each event is a new database table.

[Claude Opus 5 (1M context) from John Thomson's machine] Agreed, and it is gone. "Each event is a new database table" was the point that did the most work in the trim — it is the reason thirteen event names went rather than three or four.

src/BloomExe/web/controllers/AiImageEditorApi.cs line 449 at r15 (raw file):

Previously, andrew-polk wrote…

demoOnly

I would name this as isPlaygroundBook. I don't think I would guess what demoOnly means.

[Claude Opus 5 (1M context) from John Thomson's machine] The analytics use of demoOnly is gone with AI Editor Open, so there is nothing to rename in this PR.

For the record I think you are right about the name. It still exists as a field the host passes to the ai-editor app (pre-existing, not introduced here), so isPlaygroundBook would be a better name there too — but renaming it reaches into the package's own contract, so I have left it rather than widen this PR.

src/BloomExe/web/controllers/AiImageEditorApi.cs line 537 at r15 (raw file):

Previously, andrew-polk wrote…

Could/should this event be folded in to the commit event?

Doesn't feel worth a full event.

[Claude Opus 5 (1M context) from John Thomson's machine] Better than folded in: AI Editor Key Saved is dropped outright. The question it answered — how many people get as far as supplying a key — was already answerable from hasApiKey over time, and that whole open/key funnel is gone too.

Your instinct about folding did get applied elsewhere, though: AI Editor Commit and AI Editor Cancel are now one AI Image Editor Closed event, where an appliedCount of zero is the cancel.

Andrew's review of #8215 objected that renaming the prose from "AI image editor" to "ai-editor" reduced clarity, and he was right: "ai-editor" reads as a code identifier, not as the product. The original complaint it was answering was also right, though -- a bare "the editor" in these files reads as Bloom's Edit tab to anyone who works on Bloom. Writing "AI Image Editor" satisfies both, and matches what the events are now called. Changed in prose only, across the overlay, its tests, AiImageEditorApi and the two go.mjs console messages. Four things deliberately left alone: - "AI Editor Generate", the event name the bloom-ai-image-tools package sends us and which we translate rather than rename, plus the comment that quotes it while explaining why; - "an AI editor for text, or video, or games", which means a hypothetical different one and is the reason our names say "Image"; - the "ai-editor" Change Picture source value and the "ai-editor-overlay" DOM id, which are data and markup rather than prose; - "the editor" in EditingModel.cs, which means the Edit tab and is already right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson

@JohnThomson made 4 comments.
Reviewable status: 38 of 56 files reviewed, 15 unresolved discussions.

src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts line 25 at r15 (raw file):

Previously, andrew-polk wrote…

I can't say I agree that these comment changes are an improvement.
Seems to reduce clarity.
(same below)

Fixed in new PR

src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts line 61 at r15 (raw file):

Previously, andrew-polk wrote…

I think all these wants to be "AI Image Editor..." rather than "AI Editor...".

Did you consider a different model such that the event is something like "AI Image Editor" and the properties carry the specific action? Then the editor could add new things to track without any more plumbing. There would be both the pro and con that the analytics database would have one table with multiple event/actions in it. Over all, I would actually see that as a pro, for multiple reasons.

But as I continue through this file I see there are probably lots of complications with this...

Happy to discuss.

We did discuss this and got it down to two events which seem to have different enough fields and timing that merging them will just add complexity. We also merged a few other things. Let me know if you want to go further. https://bloombooks.github.io/dev-process-artifacts/reports/BL-16716-event-catalogue.html has the current summary

src/BloomExe/Publish/Rab/RabPublishApi.cs line 62 at r15 (raw file):

Previously, andrew-polk wrote…

Most of this summary is bloat. I would drop all but the first line.

will do

src/BloomExe/web/controllers/AiImageEditorApi.cs line 1233 at r15 (raw file):

Previously, andrew-polk wrote…

Most of this is bloat. Do we care about prior bugs we fixed?

I'm working on simplifying

JohnThomson · 2026-08-24T21:05:49Z

[Claude Opus 5 (1M context) from John Thomson's machine]

Correction to my reply above, Andrew — one of the things I offered as evidence wasn't evidence.

I wrote that the first real run "showed what this is for: Openverse returned 240 results and none
were accepted; Pixabay returned 20 and one was taken." John has since pointed out what that run
actually was: he was deliberately trying more than one provider to see what the analytics looked
like with several in play. The Openverse results went unaccepted because he wasn't shopping for a
picture, not because 240 pictures were useless.

So that number says nothing about whether searching works. It was me dressing up a smoke test as a
finding, in the middle of arguing that these events earn their keep — which is exactly where I
should have been most careful. I've removed it from the event catalogue too.

The argument for Image Chooser Closed doesn't rest on it and I don't think it needs to: a result
count can't distinguish "found nothing" from "found a page of wrong pictures", so the only signal
is what the user did next. But that's reasoning, not data. We have no field data on any of these
seven events yet — nothing has been confirmed arriving in Segment, and only the AI Image Editor
pair has been watched firing by hand. If it turns out that searching works fine and this event
tells us nothing we didn't assume, dropping it later is one small change.

* Route every analytics event through one wrapper (BL-16716) DesktopAnalytics decides whether to send an event inside Analytics.Track, and in a DEBUG build it decides not to (Program.InitializeAnalytics passes allowTracking:false). An event that calls it directly is therefore impossible to observe on a developer machine: it neither sends nor says that it didn't. BloomAnalytics.Track logs every event before handing it on, so a new event can be seen without shipping to alpha. That log is only worth reading if it is complete, so build/check-csharp-analytics.sh (run from the existing pre-commit hook) refuses a commit that reaches DesktopAnalytics directly, and every existing call site is converted here. Also adds the analytics/track endpoint the front end posts to, so a TypeScript caller can report an event without a bespoke API of its own. It fills in BookId from the selected book, and parses the body with DateParseHandling.None so that a property value which happens to look like a timestamp reaches Segment as the caller wrote it rather than as the local culture renders a DateTime. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record which route each picture came into the book by (BL-16716) "Change Picture" has been reported for years with no properties, so it could only ever say how many pictures were chosen -- a number we already knew was large and would never act on. It now says where each one came from, which is the part that could change a decision: whether a source is earning its place. Both routes new in 6.5 -- the image chooser and the AI image editor -- were bypassing the event altogether, so even the old count was quietly low. One "source" field rather than a route and a provider separately. Every provider worth telling apart belonged to a single route, the image chooser, so each is now a first-class source in its own right: pixabay, openverse, a local collection's slug, local-disk, clipboard. The two routes that never had a provider of their own name themselves the same way. The vocabulary lives in exactly two places -- AnalyticsApi.TrackChangePicture for what reports from C#, and trackChangePicture in bloomApi.ts for what reports from the browser -- because the event is worthless if its call sites disagree about the words. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record whether a visit to the image chooser ended in a picture (BL-16716) The new image chooser searches several sources, and we have no way of knowing whether it works. A result count cannot tell us: Pixabay and Openverse nearly always return *something*, so the failure that matters is not an empty page but a full page of pictures that are all wrong -- and the only reliable sign of that is what the user did next. So one event per visit, when the chooser closes, saying how it ended and which source earned the picture, alongside how many searches it took and which sources were tried. The very first real run showed what this is for: Openverse returned 240 results and none were accepted; Pixabay returned 20 and one was taken. priorChooserSessions comes from a new durable user setting, because "a Pixabay key supplied on the first visit" and "one supplied on the eighth" mean opposite things about how much of an obstacle it is. WE DO NOT SEND THE SEARCH TERM. It is the only free-form user text this instrumentation ever had. The image gallery still hands it to us, so revisiting that decision is a one-property change, but it is dropped at Bloom's boundary deliberately rather than by omission. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record what the AI Image Editor costs and whether it lands (BL-16716) Every generation spends real OpenRouter credit, and we had no idea how much or on what. Two events now cover it. "AI Image Editor Generate" comes from the ai-editor itself, over the existing bridge: what each attempt cost, which model and tool it used, how often people retry, and how much of it fails. The bridge translates the ai-editor's own name for the event into ours, so Bloom's vocabulary stays Bloom's business and a package release is not needed to change it -- and so "AI Editor" can mean the image one today without being ambiguous when there is one for text or video. "AI Image Editor Closed" is one event per session, saying what the session achieved: how many replacements were attempted, how many reached the book, and how many of them were newly generated rather than reused. An appliedCount of zero IS the abandoned case, which is why there is no separate cancel event to keep in step with this one; generatedThisSession against a zero appliedCount is how much AI work was thrown away, which is the clearest read we have on whether the output is good enough. Three things about that event are not obvious and are the reason for the tests: - It reports when the session SETTLES -- the overlay gone and no commit still outstanding -- so the counts accumulate rather than being sent per reply. Sending per reply would let a session with two commits, one failing and one succeeding, be recorded by whichever answered first, filing a session whose pictures did land as one that threw everything away. - It is sent from the browser, not from C#. For a slot on the page being edited C# only stages the replacement and hands it back, so counting a staged slot as applied overstated success in exactly the case the event exists to catch -- and in the ordinary case at that, since the picture the user right-clicked is by definition on the page they have open. - reportClosed is called after cleanup() as well as by it, because cleanup short-circuits on a session that has already ended, which is precisely when this reply is the last thing anyone was waiting for. Also bumps bloom-ai-image-tools to dist-v0.1.4, which is the build that reports its generations, and lets a linked dev library omit watchCommands (the image gallery is consumed as source and has nothing to build). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record when a custom cover layout is chosen (BL-16716) Custom cover layouts have been subscription-gated across two releases -- BL-15902 added the capability, BL-15976 put it behind Pro -- with no usage data at all. That is a weak position in any pricing or renewal conversation, and this is the baseline that fixes it: how much custom-cover use there is, and on which page. The "page" property separates the long-standing front-cover case from BL-16648's inside-back-cover extension. Whether that extension generalised past the one project it was built for is a question about branding, and needs no property here -- every event already carries BrandingProjectName. Two placement details that are the whole correctness of the event: - userInitiated. Bloom itself reverts a page to standard when a legacy theme cannot show a custom layout. Counting that would count switches nobody made, and would over-count "standard" precisely on the books where custom was wanted, so the front end says which kind of call it is making. - It reports between the endpoint's two early returns. The first is the no-op case, which must not be counted -- now that BL-16725 has made this a "set" rather than a toggle, a "standard" event really does mean someone left a custom layout. The second replies "false" for a switch to custom with no saved state, after which the front end builds the first custom layout itself; that is every bit a switch to custom and has to be counted as one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record which way line-art detection got it wrong (BL-16716) Bloom guesses whether an image is line art and, if it thinks so, makes its background transparent. When that guess is wrong the user fixes it by hand from the Transparency menu -- and each of those choices tells us which way we were wrong, in a way nothing else can. "Opaque" means Auto fired when it should not have and Bloom was eating parts of their picture; forcing transparency "can also erase very light-coloured parts of an image". "Transparent" means the detector failed to recognise line art that plainly is line art. There is no other way to evaluate that heuristic in the field. pageBackgroundIsColored is what gates Auto in the first place, so a failure can be read against it, and imageFormat says whether the failures cluster on photos or on drawings. "path" is the whole sequence of choices made on one picture -- "auto > transparent > opaque" -- because someone cycling the options is someone who did not like what they saw and was guessing, which is a UI problem (the three labels do not predict the result) rather than an algorithm one. Its history is keyed on page id AND file name, not the file alone: a page image's src is just its bare name relative to the book folder, so two books or two pages using "placeholder.png" would otherwise append to each other's history and produce a sequence no single picture ever went through. Not keyed on the raw src either, since setting transparency adds and removes a "?transparent=yes" parameter, which would restart the path on the very first change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Record how Reading App Builder builds turn out (BL-16716) Nothing under Publish/Rab has ever reported anything. The feature shipped as "initial, unpolished, experimental", and CI never runs a real RAB build at all -- only a developer who deliberately sets BLOOM_RUN_RAB_MANUAL_TESTS=1 exercises it end to end. So whether the toolchain works on real machines, and where it breaks, is otherwise something we can only learn from support traffic. BL-16469 improved the error messages; this is how we find out which errors people actually hit. One event, "Publish App", with the stage as a property rather than a name per stage -- and one event per stage that finishes rather than one per publish run, so a build the user never goes on to install still reports its own result and its own duration. Prepare is not reported: it is fast and entirely local, so it says nothing about the toolchain. errorKind is the exception type only. Never the Gradle log, which is enormous and full of file paths. The whole of the gathering is inside the try, not just the send. Reading the status to get bookCount and apkSizeMB touches files, and this is called from inside the try/catch that decides whether the action succeeded -- so an I/O hiccup here would have written "the build failed" to the log of a build that finished fine. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Enumerate a book's images once when applying credits to all of them (BL-16716) "Apply this information to all images" appeared to freeze Bloom for minutes on a book with many pictures. GetImagePaths is an iterator that reads the embedded metadata of every candidate file, to skip images that came from official collections -- and the progress percentage asked it for its Count on every pass round the loop, re-running the whole enumeration each time. A 400-picture book therefore did on the order of 160,000 metadata reads instead of 400. There is a second fault in the same line. Because the enumeration was lazy, it re-decided which files to include as it went, while this very loop is writing metadata INTO those files -- so the count it was dividing by could change underneath it. Enumerating once, up front, fixes both. Found while instrumenting this operation for BL-16716 (the card worried about books with hundreds of images: "I tested one with 400"). The event that found it is not being kept; the fix stands on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Drop a flag the session-outcome rewrite left behind (BL-16716) commitSucceeded existed only so the old cancel event could decline to fire after a successful commit. reportClosed keys on the accumulated counts and on whether any commit is still outstanding, so nothing reads it any more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say why the generated and applied counts do not tally (BL-16716) Devin flagged, as an informational note, that generatedCount and reusedCount are counted over every replacement the ai-editor sent while appliedCount counts only the ones that landed -- so a session whose generated picture failed to swap in reports a non-zero generatedCount against a lower appliedCount. It read the intent correctly and asked for confirmation, which belongs next to the code rather than in a review thread: the two pairs answer different questions. What the user chose is what they paid OpenRouter for, and that is true whether or not the picture then reached the book; appliedCount and failedCount are the pair that says what landed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say plainly what the commit relay's type is for (BL-16716) The comment on the `commit` message type was inherited and misleading in three ways, all of which cost a reader real time. It read as though it described the code below it, when the block is a type assertion that builds nothing -- so "we relay this array as-is" looked flatly contradicted by a literal that names every field including `credits`. It justified the exhaustive declaration with the one hazard that declaration has already removed: rebuilding field by field would no longer drop `credits`, because `credits` is now declared. The live hazard is the next field the ai-editor adds that this type has not caught up with, so that is what it now says. And "credits" here means the picture's attribution -- copyright, creator, license -- while this same message type also carries costUSD and spentCredits, which mean OpenRouter money. Nothing said which was meant, and the reference to "the bug this whole feature exists to prevent" named neither the bug nor the feature. It is BL-16603, "editing an image loses its credits", and it is now named. Comment only; no behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Stop keeping a durable count of image-chooser visits (BL-16716) priorChooserSessions was added for "Pixabay Key Saved": a key fetched on the first visit is a speed bump, one that takes eight visits is a real barrier. That event is not being kept, and the question the field still answers on "Image Chooser Closed" -- does success improve with practice -- is one the event stream can already answer, because Bloom identifies users to Segment (RegistrationManager.GetAnalyticsUserInfo, with retainPii) and so a user's earlier chooser visits are simply their earlier events. That leaves it costing more than it is worth: a durable per-machine user setting, a read AND a write on every chooser open, and a StrictMode guard needed only because that write happens on mount. It was also the only thing this branch persisted on the user's machine. With it gone, Settings.settings and Settings.Designer.cs are untouched, and a PR that exists to record what people do now writes nothing to their disk. What we give up, for the record: the setting counted visits on that machine for all time, where counting events can only start from the day this ships and cannot see a visit whose close event went missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Take Andrew's review of the analytics wrapper (BL-16716) Three things from his review of #8215 that survived the trim, and he was right about all three. **Send before logging.** BloomAnalytics.Track logged the event and then handed it to DesktopAnalytics, inside one try/catch. So a failure in our own logging would have stopped the event ever reaching Segment -- the wrong way round, as he put it. Both are now sent first and logged second, each guarded separately, because a single guard around both would have announced a logging failure as a failure to send. The log-side guard is the one deliberately swallowed catch in the file: the channel we would report a logging failure on is the one that just failed. ReportException had the same ordering and gets the same fix. **The class summary claimed something untrue.** It said the only way to confirm a new event was to ship it to alpha and wait. Not so -- you can point analytics at the test space. The paragraph built on that is gone, along with a line that told the reader to "call this rather than Analytics.Track" while sitting on the class rather than the method. His suggested framing replaces both: all analytics traffic should route through this class, and the commit-time check attempts to enforce it. **The same wrong claim in bloomApi.ts.** "New events have to be verified on alpha" was the front-end copy of it. It now says what actually confirms an event fired: the line BloomAnalytics writes to Bloom's log and to standard error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Say "AI Image Editor" wherever that is what is meant (BL-16716) Andrew's review of #8215 objected that renaming the prose from "AI image editor" to "ai-editor" reduced clarity, and he was right: "ai-editor" reads as a code identifier, not as the product. The original complaint it was answering was also right, though -- a bare "the editor" in these files reads as Bloom's Edit tab to anyone who works on Bloom. Writing "AI Image Editor" satisfies both, and matches what the events are now called. Changed in prose only, across the overlay, its tests, AiImageEditorApi and the two go.mjs console messages. Four things deliberately left alone: - "AI Editor Generate", the event name the bloom-ai-image-tools package sends us and which we translate rather than rename, plus the comment that quotes it while explaining why; - "an AI editor for text, or video, or games", which means a hypothetical different one and is the reason our names say "Image"; - the "ai-editor" Change Picture source value and the "ai-editor-overlay" DOM id, which are data and markup rather than prose; - "the editor" in EditingModel.cs, which means the Edit tab and is already right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Shorten two comments Andrew called bloat (BL-16716) He was right about both, and John agreed. TrackRabAction's summary is down to its first line plus the one fact that would otherwise get "fixed" by someone: prepare is deliberately not reported. The case for the event at all, the choice of one name with a stage property, and the BL-16469 provenance were all argument rather than information; they belong in the PR and the card, not on the method. The commit-reply comment keeps the reason and drops the history. What a reader needs is that C# only STAGES a replacement for the page the user has open, so only the browser learns whether it landed and reporting from here would claim success we do not have. The retelling of BL-16702, the "ORDINARY case, not a rare one" elaboration, and the inventory of what the overlay already knows were all history or restatement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Leave a trace when analytics logging itself fails (BL-16716) The guard added a moment ago reported a logging failure via Debug.WriteLine, which is [Conditional("DEBUG")]: in a release build the call is removed, so the one failure we cannot log anywhere else would leave no trace at all, and the caught exception would become an unused variable. Console.Error carries in every build, costs nothing when nothing fails, and is where Log already writes when tracking is off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Take Andrew's second review of the analytics endpoint (BL-16716) Four of his five comments, all on comments rather than behaviour except the last. **"did" implied printAnalytics is gone.** It isn't; publish/pdf/printAnalytics still invents its own endpoint. Now says "still does". **"this endpoint used to do it" made no sense on a new endpoint.** He was right -- what "used to" carry a branding property was an earlier draft of this PR, which is the PR's history and not the code's. Gone, along with the sentence about what it cost me to learn. The note now opens by saying what not to add, which is what he could not work out from it, and keeps the one fact that stops the mistake recurring: on a developer build no branding appears anywhere, and that is not evidence it is missing in production. **The transparency note described a value it never showed.** It called `path` "the whole sequence of choices" without saying what that looks like, so there was nothing to connect it to. It now gives the actual shape -- "auto > transparent > opaque" -- and names the function that builds it. It also asserted what a long path means; per his point that is now "may mean", with the likelier innocent reading ("just exploring") stated alongside. **A heavier guard when the event name is missing** -- his suggestion, though not his mechanism. The endpoint now writes the complaint to the Bloom log and to standard error as well as failing the request, so it reaches the event log and the terminal a developer is already watching, not only the browser console. NOT Debug.Fail: with no debugger attached that is Environment.FailFast, and killing Bloom over a mis-shaped analytics call is precisely what this feature is built never to do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Add BloomDebug.Fail, and fail hard on a mis-shaped analytics call (BL-16716) Andrew asked for a heavier development guard when analytics/track arrives with no event name, and suggested Debug.Fail. I pushed back on the mechanism -- with no debugger attached Debug.Fail is Environment.FailFast, which takes Bloom down and, mid-test-run, kills the test host while VSTest reports a cheerful "Passed!" for however many tests had run. He then made the point that settles it: none of the softer channels "would be observed by a developer who just broke it". Both are true, so the answer is a replacement for Debug.Fail rather than either horn. BloomDebug.Fail(message) picks the loudest thing that suits where Bloom is running: break into the debugger if one is attached; throw if we are under test, so the offending test fails and says why; otherwise show the developer a dialog with the stack and offer to attach a debugger from it (Debugger.Launch, which is the old Debug.Fail "Retry" for free). It is [Conditional("DEBUG")] rather than #if DEBUG around the body, which is what John hoped for: that is the same mechanism Debug.Fail uses, and it removes the CALL, so a release build does not even evaluate the arguments. It throws rather than calling NUnit's Assert.Fail because BloomExe deliberately does not reference a test framework -- and an exception is how NUnit reports a failure anyway. What matters is that the test-run branch never reaches the dialog: a modal MessageBox in an automated run blocks until something kills the suite. Two tests pin that branch, since it is the only one a test run can exercise. The log line and the failed request stay, because in a release build BloomDebug compiles away and they are all that is left to say a caller is broken. Eventually this should replace Debug.Fail everywhere, with a sibling for Debug.Assert, which has the same problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Rename the transparency event's "path" field to "choices" (BL-16716) Andrew's point on review: in a file that a few lines earlier is genuinely handling image file paths, "path" reads as a file path, and almost anyone would assume that first. "choices" is also what the rest of the code already called it -- recordTransparencyChoice builds it and transparencyChoicesByImage stores it -- so only the event field was out of step. The two comments that used "path" for the sequence now say "sequence" and "the whole of it". Nothing has shipped, so there is no recorded data to migrate. The event catalogue is updated to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Compile BloomDebugTests only in DEBUG (BL-16716) Found by the preflight review, not by a run: build/agent-dotnet.sh builds Debug, so the fixture passed locally and hid the problem. BloomDebug.Fail is [Conditional("DEBUG")], which removes the CALL from the CALLING assembly -- and the caller here is BloomTests itself. Built Release, the BloomDebug.Fail(...) inside each test simply is not emitted, nothing throws, and Assert.Throws fails. build/Bloom.proj defaults to Configuration=Release and the nightly runs BloomTests.dll out of output/Tests/Release/x64, so the nightly is where this would have broken -- days after the merge, and in a workflow nobody watches per-PR. A Release failure there would also have meant nothing: doing nothing is exactly what Fail is supposed to do in a release build, so there is nothing to assert and the fixture now stands down. Verified both ways: Debug still passes 2 tests; Release compiles clean and reports "No test matches the given testcase filter". The same pattern and reasoning already appear in BloomServerTests.cs:1190. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson and others added 3 commits August 18, 2026 16:31

Merge remote-tracking branch 'origin/master' into BL-16716-analytics

10b906f