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

JohnThomson · 2026-08-24T13:47:10Z

Problem

Bloom's 6.5 features shipped essentially blind. Every existing analytics event 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 at all. Nothing under Publish/Rab reported anything,
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
even the old count was quietly low.

Cause

A second, subtler problem made this hard to fix safely. DesktopAnalytics decides whether to send
an event inside its own Track method, and Bloom constructs it with allowTracking: false in
DEBUG. So a developer could not tell "fired but not sent" from "never fired" — a new event was
easy to write, easy to believe in, and impossible to observe without shipping to alpha.

Fix

Seven events, chosen by one test: would the answer change a decision? Counting use of a
capability we already know is essential does not. Splitting a known behaviour into actionable
parts does.

Plumbing

POST analytics/track plus a trackEvent() helper, so front-end code can report an event at
all. C# fills in BookId, which React in the edit view does not know. Not the branding: every
event already carries BrandingProjectName, which holds the richer subscription descriptor.
BloomAnalytics wraps DesktopAnalytics and logs every event as well as sending it, so a
developer running ./go.sh watches events scroll past. Every call site goes through it, and
build/check-csharp-analytics.sh (run from the existing pre-commit hook) keeps it that way: a
partial log would be worse than none, since a missing line would read as "not instrumented" as
readily as "did not happen".

The events, by what they answer

Where pictures come from — Change Picture gains a source field naming the route
(pixabay, openverse, a collection slug, local-disk, clipboard, ai-editor), repairing
the under-count on the way past.
Whether searching works — Image Chooser Closed, one event per visit to the chooser. A
result count could never answer this: Pixabay and Openverse nearly always return something,
so the failure is a full page of wrong pictures, and only what the user did next reveals it.
What AI costs and whether it lands — AI Image Editor Generate (from the ai-editor over
the existing bridge: cost, model, retries, failures) and AI Image Editor Closed, one event
per session, where an appliedCount of zero is the abandoned case.
A Pro feature with no usage data — Cover Layout Changed, the baseline nobody has after
two releases of charging for custom cover layouts.
A heuristic we cannot otherwise evaluate — Image Transparency Set. Every explicit choice
is a user telling us which way our line-art detection was wrong.
A path CI cannot run — Publish App, with the stage as a field. CI never runs a real
Reading App Builder build, so field telemetry is this path's only continuous coverage.

One fix that is not analytics. "Apply this information to all images" re-read every picture's
metadata once per picture — about 160,000 reads on a 400-picture book, appearing to freeze Bloom
for minutes. Found while instrumenting that operation. The event that found it is not being kept;
the fix is.

No search terms, and nothing persisted. The image gallery still hands Bloom the search term, so
revisiting that is a one-property change, but it is dropped at Bloom's boundary deliberately rather
than by omission. And this PR writes nothing to the user's disk: the durable count of image-chooser
visits it used to keep was dropped once we established that, since Bloom identifies users to Segment,
a user's earlier visits are simply their earlier events.

One dependency bump comes with it. Image Chooser Closed reads a search-report callback that
exists in bloom-image-gallery 0.0.4 and not in 0.0.3, so this advances the lockfile to the
gallery's current tip (e376463b, "Let the host see searches and where an image came from"). Six
lines. Worth a reviewer's eye only because the gallery is declared with no tag, so the lockfile is
the only thing pinning it, and pnpm holds a git dependency at its locked commit until someone runs
an explicit update — which is why master sat on 0.0.3 for five days after that gallery PR merged.

Scope

This supersedes #8215, which instrumented 22 events. That set was reviewed and judged too
large; thirteen event names were dropped and four merged into two. The parked
BL-16716-analytics branch keeps the dropped code should any of it be wanted back, and the
event catalogue records
what each dropped event would have told us.

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

This change is

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>

"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>

…716) 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>

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>

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>

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>

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>

…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>

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>

greptile-apps

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

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>

greptile-apps

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

JohnThomson · 2026-08-24T14:07:34Z

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

Consulted Devin on this PR up to 376a5579cb04be0741b571bf558b467d2745df35.

No bugs and no Investigate flags, across two rounds. So there is nothing mirrored as a review
thread here — that absence is the result, not an oversight.

It did leave seven informational notes, which by convention are not posted as threads. Two were
verifications of the things most likely to be wrong, and it is worth recording that it looked:
the AI-editor session-settlement logic, and this branch's new pre-commit analytics guard (its exit
code and its regex). Three more were reasoned confirmations rather than doubts — the picture-source
reporting being split between C# and the browser, the transparency-choice history being
process-lifetime but bounded, and the image-chooser session counter genuinely incrementing rather
than string-concatenating.

Two were questions, and both have been answered:

Generated and reused counts include replacements that failed. Correct, and intentional; those
figures say what the user chose and therefore paid OpenRouter for, while appliedCount and
failedCount say what reached the book. Devin read the intent right and asked for confirmation,
so the confirmation now lives next to the code rather than in a thread — 376a557.
The analytics guard script skips only leading comments, so a line of real code followed by an
inline comment that happens to name Analytics.Track( would block a commit. A genuine, narrow
false positive. Left as it is deliberately: the script prints exactly why it refused, so the fix
is to rephrase the comment, and a cleverer regex risks the far worse direction — letting a real
unwrapped call through.

Greptile also ran and posted only "Your trial has ended", so there is no Greptile review of this PR.
CI (pr-automation) is green, and both test suites pass locally: 740 front-end tests and 3184 C#.

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>

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>

greptile-apps · 2026-08-24T16:48:41Z

Greptile Summary

This PR adds frontend-to-host analytics plumbing and instruments the image chooser, AI Image Editor, cover layout, image transparency, picture-source, and Reading App Builder flows. It also centralizes C# analytics logging, adds a pre-commit enforcement check, updates the image-gallery dependency, and avoids repeatedly enumerating image metadata during apply-to-all operations.

Adds the analytics/track endpoint and TypeScript tracking helpers.
Records session outcomes and generated/applied image counts for the AI Image Editor.
Adds analytics fields and events across image editing and publishing workflows.
Routes existing C# analytics calls through BloomAnalytics.
Adds analytics tests and pre-commit enforcement.
Materializes the image list once when applying metadata to all images.

Important Files Changed

Filename	Overview
src/BloomExe/web/controllers/AnalyticsApi.cs	Adds the project-scoped frontend analytics endpoint, validates its event payload, enriches it with the current book ID, and replies through the normal API contract.
src/BloomBrowserUI/utils/bloomApi.ts	Adds non-disruptive frontend helpers for general analytics and Change Picture source tracking.
src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts	Adds allowlisted AI editor event forwarding and session-level outcome accounting while guarding overlapping commits, detached frames, and repeated cleanup.
src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx	Records chooser outcomes and picture sources using the updated image-gallery callback contract.
src/BloomExe/BloomAnalytics.cs	Introduces a logging wrapper around DesktopAnalytics and preserves failure isolation for analytics calls.
build/check-csharp-analytics.sh	Adds a staged-file pre-commit check requiring C# analytics calls to use the logging wrapper.
src/BloomExe/Book/ImageUpdater.cs	Materializes the eligible image sequence once so apply-to-all metadata work does not repeatedly reread every file.
src/BloomExe/Publish/Rab/RabPublishApi.cs	Adds stage-specific telemetry to Reading App Builder publishing operations.
src/BloomBrowserUI/pnpm-lock.yaml	Advances the pinned bloom-image-gallery revision needed for search-result and selected-image reporting callbacks.
src/BloomTests/web/controllers/AnalyticsApiTests.cs	Verifies event forwarding, property handling, BookId enrichment, malformed requests, and endpoint failure isolation.

_{Reviews (5): Last reviewed commit: "Leave a trace when analytics logging its..." | Re-trigger Greptile}

JohnThomson · 2026-08-24T16:59:09Z

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

Consulted Devin again, up to 510e4ceb2860612f6a0ef8fdeba57d2900049867 (the earlier log above
covered 376a557).

Still no bugs and no Investigate flags, so again nothing is mirrored as a review thread. Eight
informational notes this time. The two new ones since the last log:

analytics/track is handled off the UI thread, and HandleTrack reads the current book from the
worker. Devin's own conclusion is that this is safe — the read is of a reference and the send is
async — with the nuance that an event fired during a book switch attaches whichever book is
current when it sends. Left alone: the window is a few milliseconds and the alternative is putting
analytics on the UI thread, which is worse.
The analytics guard script skips only leading comments, so a line of real code followed by an
inline comment that happens to name Analytics.Track( would block a commit. A genuine but narrow
false positive, left deliberately: the script prints exactly why it refused, so the fix is to
rephrase, and a cleverer regex risks the far worse direction of letting a real unwrapped call
through.

One note is stale and should be ignored: it describes the image-chooser visit counter and its
mount effect, which this same commit removed. Devin appears to have carried it forward from the
previous round.

Also worth recording for anyone reading this PR's history: pr-automation stopped firing on
pushes part way through this run. It ran for the first two commits and not for the two after, so
Devin only reviewed those because I re-triggered the workflow by hand. On this repo a commit whose
pr-automation never fires gets no Devin review at all, and the missing check looks exactly like a
passing one.

…trimmed Two dependency resolutions, both of them master's business rather than this branch's: bloom-ai-image-tools: master has moved to dist-v0.1.5, past the dist-v0.1.4 this branch pinned for the generation-reporting build. Took master's. It still sends "AI Editor Generate", which is the only name the bridge's map needs, so the translation to "AI Image Editor Generate" is unaffected. package.json therefore drops out of this PR's diff entirely. bloom-image-gallery: this branch needs 0.0.4, whose search-report callback "Image Chooser Closed" reads. The package is declared with no tag, so the lockfile is the only thing pinning it -- and pnpm keeps a git dependency at its locked commit until told otherwise, which is why master was still on 0.0.3 five days after the gallery's PR #20 merged. `pnpm update bloom-image-gallery` moves it to the current tip, which IS 0.0.4 (e376463b, "Let the host see searches and where an image came from"). Six lines of lockfile, after re-running the repo's prettier over what pnpm rewrote in its own YAML style. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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>

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>

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>

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>

JohnThomson · 2026-08-24T20:09:39Z

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

Consulted Devin up to bbddb3d72b0ba8bbb7bb51bfc09b6ba7c81cddd6.

No bugs. Twelve informational notes and one Investigate flag — and that flag is
the gallery-version one already answered and resolved above,
re-raised unchanged on this commit. Nothing to add: the lockfile bump to gallery 0.0.4 landed with
the base merge and the typecheck passes on it. Not mirrored a second time, because a duplicate
thread saying the same thing would only bury the first.

Two of the informational notes are worth a reader's attention, both left deliberately and both
consistent across runs: the pre-commit analytics guard scans the working tree rather than the
staged blob (real, but it matches its sibling check-csharp-*.sh scripts, so consistency argues
against diverging here), and a failed commit still contributes to generatedCount/reusedCount
— which Devin traced to the comment explaining that those count what the user chose and paid for,
not what landed.

One note is stale and can be ignored: it describes the image-chooser visit counter and its mount
effect, which were removed several commits ago. Devin has carried it forward for three runs now.

andrew-polk

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

src/BloomExe/web/controllers/AnalyticsApi.cs line 14 at r4 (raw file):

publish/pdf/printAnalytics did

"did" is misleading since it still does.

I see there are complications with replacing that call with our new one. I'm not necessarily advocating for trying to work through those complications and making the replacement, but it does make me wonder if there anything about the design of the API which we should consider changing.
I don't have anything specific in mind, it just smells that our only existing analytics endpoint can't be fully modeled by the new endpoint.

src/BloomExe/web/controllers/AnalyticsApi.cs line 61 at r4 (raw file):

this endpoint used to do it

This is a new endpoint, so this type of comment seems less than unhelpful.

src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts line 843 at r4 (raw file):

                        // they saw and was guessing. A lot of those means the three labels do not
                        // predict the result well enough, which is a UI problem rather than an
                        // algorithm one.

What is the "whole sequence of choices"? It seems to be referring to something which isn't here.

Also, it is overstating its theory on what switching means. That might be what it means. Or they are just exploring, etc. Anyway, if this comment even fits somewhere, I would just add a "wiggle word" like might/maybe.

src/BloomExe/web/controllers/AnalyticsApi.cs line 53 at r4 (raw file):

                // quietly recording nothing, which is a very hard thing to notice.
                request.Failed("analytics/track requires an 'event' name");
                return;

Is it worth adding a heavier development guard here?
Maybe Debug.Fail?

src/BloomExe/web/controllers/AnalyticsApi.cs line 73 at r4 (raw file):

            // tracking is off, and BloomAnalytics logs only the per-event properties, so on a
            // developer build the log line shows no branding at all. That is not evidence it is
            // missing in production. It cost a round trip to learn; please do not re-add it.

Sorry; I'm probably being dense, but this paragraph is not clicking for me. I'm not sure what I'm supposed to not re-add.

JohnThomson and others added 9 commits August 23, 2026 15:27

greptile-apps Bot reviewed Aug 24, 2026

View reviewed changes

JohnThomson and others added 2 commits August 24, 2026 11:35

JohnThomson commented Aug 24, 2026

View reviewed changes

Comment thread src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx

JohnThomson marked this pull request as ready for review August 24, 2026 18:59

JohnThomson mentioned this pull request Aug 24, 2026

Add analytics for the features new in 6.5 (BL-16716) #8215

Closed

JohnThomson and others added 3 commits August 24, 2026 14:10

JohnThomson marked this pull request as draft August 24, 2026 19:56

JohnThomson marked this pull request as ready for review August 24, 2026 20:48

andrew-polk requested changes Aug 25, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

Analytics for the features new in 6.5 (BL-16716) - #8228

Analytics for the features new in 6.5 (BL-16716)#8228
JohnThomson wants to merge 17 commits into
masterfrom
BL-16716-analytics-trimmed

JohnThomson commented Aug 24, 2026 •

edited by hatton

Loading

Uh oh!

greptile-apps Bot left a comment

Uh oh!

greptile-apps Bot left a comment

Uh oh!

JohnThomson commented Aug 24, 2026

Uh oh!

greptile-apps Bot commented Aug 24, 2026 •

edited

Loading

Greptile Summary

Important Files Changed

Uh oh!

JohnThomson commented Aug 24, 2026

Uh oh!

Uh oh!

JohnThomson commented Aug 24, 2026

Uh oh!

andrew-polk left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

JohnThomson commented Aug 24, 2026 • edited by hatton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Cause

Fix

Scope

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

JohnThomson commented Aug 24, 2026

Uh oh!

greptile-apps Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Important Files Changed

Uh oh!

JohnThomson commented Aug 24, 2026

Uh oh!

Uh oh!

JohnThomson commented Aug 24, 2026

Uh oh!

andrew-polk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JohnThomson commented Aug 24, 2026 •

edited by hatton

Loading

greptile-apps Bot commented Aug 24, 2026 •

edited

Loading