Dansday

The Month 863 Lines Lost to 137

Published on Nov 30, 2024

Twenty-one pull requests in November, and the number that actually describes the month is this: eleven of them were fixes to work that had merged days or hours earlier. Staging fixes, feedback addressed, a fix to a fix. Half of what I shipped was a correction to the other half.

That is not a complaint. It is what shipping into a live shop with three reviewers and a QA pass looks like when the features stop being new.

The same picture, forty times

Two tickets arrived that month with the same root cause, filed separately, and it took me a while to see they were one problem.

[HIGH] Optimise GIF Delivery
1) These are fairly large GIFs - is there a way to optimise the pics?
2) The same pics are being duplicated across different products

[HIGH] Images should be correctly reused across site
From PageSpeed Insights, identical images used across our site are
treated as duplicated images.

The promotion system I had built in May and restructured in June let marketing attach a badge, an icon and a decorative frame to each product. Every one of those was uploaded per product. So the same promotional GIF, applied to forty iPhones, was forty separate files at forty separate URLs, and a browser has no way to know they are the same image. It downloads all forty. PageSpeed noticed before we did.

This is a cost of the design I chose in May and did not think about once. Per-product attributes are exactly right for a price or a title. For a shared decorative asset they are a duplication machine, and the machine had been running for six months.

863 lines lost to 137

I tried to fix it twice, and the two attempts are the most instructive pair of pull requests in my whole time on this codebase.

The first ran from 6 to 21 November. 863 additions, fifteen files, built around a 725-line admin template. The idea was to move promotion imagery out of per-product attributes entirely and into the theme customisation system, where one uploaded asset is shared by everything that references it. It deleted the promotion config, deleted a service provider, and deleted one of the test files I had been so pleased about in June.

It was the right diagnosis. It was also, I realised while building it, a partial undo of a decision from June — the commit that month called Use bagisto default configuration panel, where I threw away my own settings screen because the framework already had one. Five months later I was building a custom screen again, for reasons that felt good at the time.

The second attempt merged on 20 November. 137 additions, one deletion, two files. It overrides the framework's attribute-value repository and changes where an uploaded file is stored:

const ATTRIBUTES_PATH = "product/attribute/";

if (in_array($attribute->type, ['image', 'file'])) {
    $data[$attribute->code] = gettype($data[$attribute->code]) === 'object'
        ? request()->file($attribute->code)->storeAs(
              self::ATTRIBUTES_PATH . $attribute->code,
              request()->file($attribute->code)->getClientOriginalName()
          )
        : $data[$attribute->code];
}

Bagisto stored uploads under a per-product directory with a generated name. This stores them under the attribute's own directory using the original filename. Upload promo-star.gif as the promotion icon for forty products and it lands at one path, resolves to one URL, and the browser downloads it once and caches it for the other thirty-nine.

The day after that merged, I closed the 863-line one.

There is a real trade in the small version and I want to name it, because a fix that looks free usually is not. Keying storage on the original filename means two genuinely different images that happen to share a name will collide, and the second upload silently replaces the first. The big approach would not have had that problem. It would have had a 725-line admin screen to maintain instead, and a second configuration system competing with the framework's.

Nine hundred lines of correct thinking, closed in favour of nine lines in the right place. That is the single best lesson in this series, and I only learned it by writing the wrong one first.

Full payment reservations

The month's largest feature continued a thread that started as a one-day experiment in March. Customers reserving a device for in-store pickup could now pay for it in full up front, rather than a deposit.

The ticket explained why, and the reasons are all human rather than technical:

Reasons customers use full payment reservations:
- Urgent order (can't wait for courier)
- Need help with data transfer
- Still want to inspect device
- Want to use online payment method (already at store)

Somebody standing in a 3cat store, phone in hand, wanting to pay online. That is not a case you would invent at a desk. It is the sort of requirement that only exists because somebody watched customers.

Four pull requests: the interface, two rounds of review feedback, and a staging fix touching fourteen files. March's throwaway STATUS_RESERVED, April's delivery-versus-pickup panels, September's confirmation slide-up, November's full payment. None of those months knew about the next one.

A sea of red error messages

My favourite ticket of the month, and the phrase is the reporter's, not mine:

Today, when customer sets focus in mandatory field, but does not fill in
info, we throw an error message. Customers typically want to 'explore'
more before they fill in their details. From Hotjar, this is a common
behaviour, but ends up in a confusing sea of red error messages.

Note where the evidence came from. Hotjar — the session-recording script I had ripped out as a hotfix four weeks earlier, with an empty issue body and no stated reason. The recordings had already done their work before I deleted them, and this ticket is what they produced.

The checkout had been validating on every keystroke. Tab into the phone field, tab out to read the price, and the form scolds you for a field you were always going to fill in. Three pull requests on 22 November — the fix and two fixes to the fix — landed on the right behaviour:

target.addEventListener('input', (event) => this.clearErrorOnInput(event.target));
target.addEventListener('blur',  (event) => this.validateFieldOnBlur(event.target));
validateFieldOnBlur(target) {
    if (target.value.trim().length > 0) {
        if (this.isPhoneNumber(target)) { ... }
        else if (this.isEmail(target)) { ... }
        else if (this.isPostcode(target)) { ... }
    }
}

Typing clears an error; leaving a field checks it; and the length guard means an empty field is never wrong, only unfinished. That distinction — wrong versus not yet answered — is the entire ticket, and it is four lines of JavaScript.

What to do when there is none left

Out-of-stock products had simply had no checkout button. Nothing explained the absence, so the page just looked broken to anyone who did not know the stock system existed.

The replacement turns a dead end into a conversation, with a WhatsApp message pre-written in the customer's voice:

'out_of_stock' => [
    'low_stock' => 'Low Stock',
    'is_very_popular_and_almost_gone' => 'is very popular and almost gone!',
    'please_chat_to_confirm_stock_and_buy' => 'Please chat to confirm stock and buy:'
],
'pdp_out_of_stock'   => 'Hi, I\'m interested in the :product',
'pdp_out_of_stock_2' => 'Do you still have stock available?',

Better than a missing button, and a stranger thing than it looks. The badge says Low Stock and the sentence says almost gone, on a product where the actual quantity is zero. Both are a softer version of the truth, and I wrote them.

I spent December making a product page stop claiming discounts it did not have, and eleven months later I shipped a label that rounds none up to almost none. I do not think it is dishonest — the device may genuinely be findable in a store, which is the entire point of the chat button. But it is the same category of decision, and it went in without anybody pausing over it, including me.

Two words, two pull requests

On 21 November I changed one word:

- 'Walk into any store to experience our top-quality devices, ...'
+ 'Walk into any store to experience our great quality devices, ...'

One addition, one deletion, one file, merged. Then a staging fix the same day, because the same phrase was also sitting in an FAQ answer:

- '... your closest store to experience our top-quality devices, ...'
+ '... your closest store to experience our great quality devices, ...'

Two pull requests to change one phrase in two places. The reason it needed a second pass is that the phrase was duplicated, which is the same disease as the images — one idea stored in more than one location, so changing it means finding every copy.

That FAQ answer is worth reading for another reason. It now reads we currently have :total_stores stores across Malaysia in Penang, Kedah, Perak, KL, Selangor, Johor, Kelantan and Sabah — eight states, with the count interpolated from the store list rather than typed. In September it was five states. In December it was four stores in Kuala Lumpur and the number was written by hand.

And somebody, at some point between March and November, fixed the typo I shipped in that exact question. It reads Do you have physical stores? now. It went out as strores.

Half of what I shipped fixed what I had just shipped

Eleven of twenty-one pull requests were follow-ups. Three merged on a single day for one ticket. A one-word copy change needed two. The full payment feature needed three rounds after the main one.

The pattern I read in that is not sloppiness so much as a shortening feedback loop. Almost every one of those fixes came from staging or from a reviewer, which means things were being caught before customers saw them — and the cost of catching them is a pull request with a slightly embarrassing title. Six months earlier this codebase produced monthly feature branches with thirty commits. Now it produced a dozen small ones a week, most of them merged the day they were opened.

A fallback that everybody downloads

219 additions, 111 deletions, fourteen files, filed as Bugs on iPad, and I did not mention it. It is the direct sequel to something I did write about.

In October I described giving WebP images a way out — a script that detects whether the browser supports the format and, if not, rewrites every image, every link href and every background-image on the page to a PNG. In November ninety-six lines of that came out of common.js, the bundle every page loads, and became its own file:

fallback: {
    src: [ "resources/js/old_browser_fallback.js" ],
    name: 'old_browser_fallback.js',
    dest: 'js'
},

Which is the right instinct and not quite the right outcome, because of how it gets loaded:

<script src="{!! Option::getAssetCdn() . mix('js/common.js') !!}" defer></script>
+<script src="{!! Option::getAssetCdn() . mix('js/old_browser_fallback.js') !!}" defer></script>

Unconditionally. A file named old_browser_fallback is fetched by every browser, old or not. What moved out of common.js did not stop being downloaded; it became a second request instead of part of the first. Only the conversion itself is gated, by the same support check as before.

I am writing this in a month where my headline was 863 lines lost to 137, so the shape of the thing is familiar: a change that reads as a reduction, and is really a relocation. And it has held: that script tag is still in the master template today, still unconditional, fifteen months later.

The other half of that pull request is the iPad bugs themselves — a misaligned location icon, a banner and a call-to-action stuck together on the trade-in page, and Find the nearest store not working at checkout. The fix is ten templates each receiving one line, almost all of them a <span> wrapped around a translation string so that a flex container has an element to lay out instead of a bare text node. Ten files, ten identical one-line edits, no shared component. Nobody could find that pattern again by searching for it.

The banner left the framework

229 additions, 286 deletions, eleven files, titled Google-style Banner Nav — the month's largest change after the two I wrote about, and a net deletion of 57 lines.

What it does is take the homepage promotional banner off Stimulus. A 204-line controller, slide_controller.js, is deleted. A 178-line plain script, banner.js, replaces it as its own bundle. The markup loses its framework wiring and gains CSS:

-<div data-controller="slide"
-     data-slide-absolute-class="absolute"
-     data-slide-hidden-class="hidden">
-    <div data-slide-target="slide" class="slide block w-full cursor-pointer bg-white">
+<div class="relative w-full">
+    <div class="banner block w-full bg-white absolute transform
+                translate-x-full first:translate-x-0">

The same pull request deletes cwv-lcp/home-banner.blade.php — a component that existed solely to help the Core Web Vitals largest-contentful-paint measurement on the homepage banner.

This is the most consequential architectural decision of the month and it arrived under a title describing a visual style. The homepage banner is the first thing a customer sees and the element every performance score is measured against, and in November it stopped being a framework component and became a script and some transforms. I would defend the change — a carousel that has to paint immediately is a bad fit for a controller that initialises after hydration. What I would not defend is that the month's summary, as I wrote it, does not contain it.

Two smaller things also missing: the order notes field lost its subtext and gained a new placeholder, and two pull requests adjusted the local development environment, one of them fixing the other.

What November was

Twenty-one pull requests. Reservations that take full payment, a checkout that waits until you have finished typing before objecting, an out-of-stock page that offers a conversation, and forty copies of a GIF that became one.

The one I would tell is closing my own 863-line pull request. It was not wrong — it correctly identified that shared assets do not belong in per-product attributes, and a version of that argument is still true. It was just far more machinery than the problem required, and I could only see that once the nine-line version was sitting next to it. Being willing to throw away two weeks of your own correct reasoning is a skill I did not have in December.

Revised: this article described twenty pull requests and named about a dozen. It omitted the WebP fallback extraction — the sequel to October's WebP work — and the banner rewrite that took the homepage's most performance-critical element off the framework. The sections above were added from the original diffs.