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) <[email protected]>

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) <[email protected]>

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) <[email protected]>

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) <[email protected]>

…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) <[email protected]>

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) <[email protected]>

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) <[email protected]>

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) <[email protected]>

…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) <[email protected]>

…l (BL-16716) Devin. "Cover Layout Blocked" exists to split the demand we refuse by cause, because the two causes call for opposite responses: a legacy-theme wall is a migration path we can fix, a subscription wall is the demand signal for the Pro tier. So a wrong cause is worse than no event. useGetFeatureStatus returns undefined until its request completes, and blockedByLegacyTheme is defined as "the theme blocks it AND the subscription does not". During that window a legacy theme therefore looks like the only wall, so a menu opened in the first moment blamed the theme for a refusal the subscription also caused -- and the same book, a second later, reported the other cause. Nothing is reported now until the lookup has answered. One lost event is much cheaper than a wrong one in the only property this event has. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

…BL-16716) Devin. Newtonsoft's default DateParseHandling materializes a JSON string that parses as a date-time as a DateTime rather than a string, and GetProperties then falls through to JValue.ToString() -- which formats it with the local culture. So a property arriving as "2024-01-01T10:00:00Z" was recorded as "1/1/2024 10:00:00 AM" here, and as something else on the next machine. That matters because several of these properties are free user text, an image search term above all: the record has to say what was sent, and it has to mean the same thing in every locale. The request body is now parsed with DateParseHandling.None, so every JSON string stays a string. Fixed at the parse rather than by adding a Date case to GetProperties, because the coercion is what is wrong, not the formatting of it. Devin's example was a date-only "2024-01-01", which Newtonsoft in fact leaves alone; I probed the real behaviour before writing the tests, and they use a full timestamp, which is the reachable case. Both fail if the parse setting goes back. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Devin flagged the GIF route as unreported. It is reported -- doChangeGifImage posts to imageGallery/imageGalleryResult, which reports Change Picture -- but it was reported as the wrong thing: it sent no provider, so the endpoint filed it under "image chooser" with provider "unknown". Changing a GIF never opens the chooser at all; it goes straight to the native file picker, so it belongs with the other pictures a user takes off their own disk. It now sends provider "local-disk", the same id the chooser uses for that route, which also stops "unknown" appearing in a breakdown whose whole purpose is to say where pictures come from. Also documents EditingView.SaveChangedImage, which Devin noted has no callers anywhere while this PR gave it a hard-coded "paste"/"clipboard" pair. Left as it is, since it is unreachable, but the comment says the pair is the route the method used to serve rather than anything the caller supplies -- so anyone reusing it knows to pass the real route through. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Devin's last finding, and the cheapest way to close a class rather than argue about whether it is reachable. commitInFlight was a boolean, but the AI editor is free to send a second commit before the first is answered -- and the first reply then cleared the flag while the second was still in the air, so closing the overlay at that moment reported the session as thrown away with a commit still running. A count instead. The single-commit case behaves exactly as before; the test sends two overlapping commits, answers the first, closes, and asserts no cancel until the second is answered too. It fails if the count goes back to behaving like a flag. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Devin, and this one is a defect I introduced two commits ago when the in-flight flag became a count. postJson chains .then(success).catch(error), so a throw escaping the success callback runs the error callback for the same request -- which this code already relies on being reachable, since that is why reportCommit is guarded. But both paths decremented the count, so one commit could take it to -1, after which "no commit outstanding" is never true again and a session the user threw away would never be reported as abandoned. With the old boolean, setting false twice was harmless; with a count it is not. Both paths now go through noteCommitSettled, which decrements once, in the same shape as the reportCommit guard beside it. The test makes the commit report throw, runs the error callback for the same request as postJson would, closes the overlay, and asserts the cancel is still reported. It fails without the guard. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

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) <[email protected]>

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) <[email protected]>

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) <[email protected]>

…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) <[email protected]>

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) <[email protected]>

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) <[email protected]>

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) <[email protected]>

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 and others added 3 commits August 18, 2026 16:31

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

10b906f