In-flight feedback for hero image actions

I picked up a low-priority “for later” ticket about the hero-image assignment card giving zero feedback when you click anything. It was filed after I noticed it during manual testing of an earlier issue - click “Commit to live article” and nothing happens on screen until, eventually, if you wait, the card updates. No spinner, no disabled button, nothing telling you the click even registered.

I went in expecting a small CSS/JS tweak. It turned into a more interesting design problem than the ticket title suggested.

Two different problems wearing one ticket

Once I actually read the code, this split into two unrelated fixes. Search-load, Select, and Remove are synchronous - one HTTP round trip, done. Those just needed Turbo’s built-in data-turbo-submits-with attribute, which disables the button and swaps its label until the response comes back. No JS to write, just an attribute.

Commit was the actual problem. The controller enqueues a background job and returns almost instantly - the job is what does the real work (fetching the file from GitHub, maybe downloading and re-encoding an image, writing it back). A “disable until the response returns” trick doesn’t cover that window at all, because the response returns before any of the real work happens.

The design I almost shipped, and why I didn’t

My first plan was a persisted hero_commit_in_progress boolean: set true when the job is enqueued, cleared by the job when it finishes (success or failure). Straightforward, and it would’ve worked for the happy path.

I ran the plan past a second opinion before writing any code, and it caught something I’d missed: if that flag ever got stuck true - say, a deploy restarts the background worker mid-job - the card would show a permanent spinner with no retry button and no remove button. Worse than the bug I was fixing, since at least today you always have an actionable button.

This app deploys multiple times a day. “The worker gets killed mid-job” isn’t a corner case, it’s a Tuesday.

So I dropped the DB column entirely. Instead, the controller passes a transient committing: true value into the same render call it already makes right after enqueueing the job - shown for exactly one response, never persisted anywhere. A page reload mid-job just shows the normal button again; the job’s own broadcast still corrects the card once it’s actually done. Nothing to get stuck, because there’s no state to get stuck in.

The tradeoff: a second browser tab, or a different person, watching the same post won’t see the in-progress state - only the tab that made the click does. For a single-operator internal tool that’s a fine trade. For a multi-user product it wouldn’t be.

The bug I shipped anyway (and code review caught)

This is a Rails app with two independent views that both show hero-image state - a batch “missing hero” list and the post detail page - and they’d drifted into duplicating the same commit/retry/remove logic rather than sharing a partial. I found this while researching the ticket; the issue only named one of the two views.

I wrote the same three-branch conditional in both files: “is it already live” / “is it committing” / “did it error” / “show the normal button.” I put the live-check first, matching the pre-existing code. That was wrong. If a post already has a live hero and you commit a new image over it, the live flag doesn’t clear until the job finishes - so my new “committing” branch never won the if/elsif race. The re-commit case, which is arguably the most common real usage (swap the hero, not just set it once), silently showed stale “Applied to the live article” text with no spinner for the entire job run.

The automated multi-angle review I run before every merge - eight independent passes over the diff, each from a different angle - flagged this exact precedence bug from three different directions. That’s the kind of thing that’s obvious once you see it and very easy to not see while you’re writing it, because the happy-path test (first-ever commit, no live hero yet) passes cleanly and looks done.

Fix was a two-line reorder: check committing first, then live, then error. I added a regression test for the re-commit-over-live case specifically, since it’s exactly the kind of thing that regresses silently if someone reorders branches again later.

A second, sneakier bug from my own test code

While writing a test for the Select button’s new attribute, I needed to stub out an external image-search call without hitting real APIs. There’s an existing pattern in this test file for stubbing a class method with no mocking gem installed: define a fake singleton method, then remove_method in an ensure block to clean up.

I copied it. It looked identical. It broke a completely different test file - and only in CI, not locally.

The existing pattern stubs Github::ContentClient.new, which is inherited from Class#new - removing the stub just uncovers the inherited method again, harmless. My case, ImageSearch.search, is a method the module defines itself. remove_method doesn’t uncover anything underneath - it deletes the only implementation that ever existed, for the rest of that test process.

Locally, with 10 parallel test workers, my stubbing test and the test that calls the real method almost never land in the same worker, so it looked fine through several full local runs. CI runs 2 workers. Much higher odds of collision, and it did collide - a completely unrelated test file failed with NoMethodError: undefined method 'search' for module ImageSearch, several files away from anything I’d touched. Took a minute to place why a passing local suite would fail in CI on a test I hadn’t even modified.

Fix: capture the actual Method object before stubbing, restore that object instead of deleting anything. Wrote it up as a knowledge-base note since it’s a general Ruby gotcha, not specific to this app - the exact bug you get by copy-pasting a stubbing helper without checking whether the original method is inherited or explicitly defined.

What’s next

Nothing else planned for this one - it was scoped as a small polish ticket and it stayed that size once the design settled. If a future issue wants the in-progress state visible across tabs/viewers, that’d mean revisiting the persisted-flag approach with a proper recovery path (e.g. the remove button always staying live even mid-commit) rather than the transient-local shortcut this used.