# Admin UI review

Browser review of the plugin's admin screens. Round 1 findings and what was done.

---

## Round 1 — 2026-08-06

Reviewed: Settings (all tabs), Visitor Journeys list, single journey report.

### Blocking bugs — both regressions from moving to a top-level menu

**B1. The journey report was completely inaccessible.** Following a link from the list
returned *"Sorry, you are not allowed to access this page."*

`admin.php?page=…` works out which capability to check by looking the page up in
`$submenu`. `remove_submenu_page()` deletes exactly that entry, so a hidden page under
a top-level menu cannot be authorised at all. It worked while the plugin lived under
Settings only because `options-general.php` has its own capability to fall back on.

**Missed by the test suite because every server-side test called `render_viewer()`
directly**, bypassing WordPress's admin routing. A method that renders correctly when
called in isolation tells you nothing about whether the page is reachable. Only loading
the real URL in a browser caught it.

*Fixed:* the report is now a mode of the Visitor Journeys page — a `token`/`record`
parameter renders one journey, its absence renders the list. One registered page, no
hidden page, nothing to authorise around.

**B2. All admin CSS and JavaScript silently stopped loading on the settings page.**
`enqueue()` compared the hook against a hardcoded `'settings_page_' . $slug`, which
became `toplevel_page_…` when the menu moved. The page still rendered, so nothing
looked broken in code — it just lost every stylesheet, the Test tab scripts and the
UTM builder. Collapsible sections showed as bare `▶` characters.

*Fixed:* compare against the hook suffix `add_menu_page()` actually returns, so it
cannot drift again if the menu moves.

### Usability and formatting

| # | Finding |
|---|---|
| U1 | **Wordy by default.** Every field carries an always-visible description; tab intros run to two paragraphs. Detail should be revealable, not permanent. |
| U2 | **Line length far too long.** Body text spans ~1060px. Hard to scan; nothing caps measure. |
| U3 | **Inconsistent widths.** Collapsed sections span the full width while the content above them is capped at 900px, so edges are ragged down the page. |
| U4 | **Interactions tab is nine near-identical rows** using ~700px of vertical space for nine checkboxes. |
| U5 | **"Track this" repeated nine times** adds nothing — the row label already says what it is. |
| U6 | **Heading noise in the report** — an `<h2>` "More detail" immediately followed by four more `<h2>`s inside the collapsed summaries. |
| U7 | **List page**: three-line intro; the Enquired column shows a full timestamp where the date alone would do. |
| U8 | Page title plus seven tabs nearly wraps at common widths. |
| U9 | On Settings, the first section is open while the rest are closed — arbitrary. |
| U10 | "Journey link / recommended" — the sub-label under the field name reads awkwardly. |

### Fixes applied

1. **Revealable help.** A `?`/"Why?" toggle replaces always-on descriptions, using
   `<details>` — no JavaScript. Short label visible, detail one click away.
2. **Readable measure.** Body copy capped at ~820px; controls and tables keep their own
   widths.
3. **Consistent width** between sections, tables and copy.
4. **Interactions compacted** into a single table: name, toggle, one-line hint. "Track
   this" removed.
5. **Intros cut to one sentence** on every tab.
6. **Report heading hierarchy** flattened — "More detail" is now a quiet divider, and
   the collapsed sections use `<h3>`.
7. **List shows the date**, with the time available on hover.

---

## Round 2 — 2026-08-06

Re-reviewed every screen after the round-1 revisions. The compaction worked — the
Handoff tab went from a wall of prose to a seven-row index, Interactions from ~700px
of vertical space to ~500px for the same nine controls. What round 2 found was
mostly the *consequences* of that compaction, plus formatting faults round 1 had
recorded but not actually fixed.

### The review kept lying to me

Twice, a fix looked like it had failed when it had in fact worked: the stylesheet is
enqueued with `BW_LEAD_AI_VERSION`, which does not move between releases, so every
CSS edit sat behind a browser cache. The first time this cost a wrong diagnosis —
the `.bw-help` rules appeared not to apply at all.

*Fixed at the root:* admin assets are versioned by `filemtime()`, so any edit busts
the cache. `asset_ver()` is shared with the journeys page, which registers the same
handle and would otherwise pin the other page's stylesheet to a stale URL.

### Findings

| # | Finding | Fix |
|---|---|---|
| V1 | **U3 was never actually fixed.** The report had five different right edges (1131 / 1114 / 1084 / 1082 / 1002). `box-sizing` is `content-box` in WP admin, so padding and borders are added *outside* `max-width` — a `max-width: 900px` card with 22px padding renders 949px wide. | One border-box rule and two named measures (`--bw-measure`, `--bw-block`). Every block now lands at the same edge. |
| V2 | **Collapsing the sections cost the reader the state.** Finding out whether data handoff was on meant opening five panels in turn. | Each closed row carries what it is set to — "Journey link + Data handoff", "9 stored · 3 sent", "kept 730 days", "4 tracked". Green when on, italic grey when off. |
| V3 | **Timeline times were UTC** while the header used the site's timezone, so one record showed two clocks. The visit dates were also the raw client-formatted string. | `journey_dt()` formats from the stored epoch-ms in the site's timezone and date format, falling back to the client string for older records. The note claiming times were "the visitor's own local time" was updated — it had become false. |
| V4 | **`<details>` nested inside `<p>`** in four places. A browser closes an open paragraph the moment flow content starts, orphaning the help block. | Those intros are `<div class="description">` with matching metrics. |
| V5 | **Opening a help toggle moved it.** `inline-block` sizes to its widest child, so the box grew to the panel's width and wrapped to its own line — the sentence above looked truncated. | `display: inline`, which lets the browser split the box: toggle stays put, panel renders beneath. |
| V6 | **The "Why?" target was 27×17px.** | Padded to 39×23 with a negative margin so nothing shifts, plus hover and focus states. |
| V7 | **Two reference panels sat below Save Changes**, outside the form, reading as fields the button had covered. | A quiet "Reference" divider separates them. |
| V8 | **Interactions: the checkbox is far right of a wide row**, so the click target was a 16px box a long way from its label. | The row name is a `<label for>` and toggles it; the row highlights on hover. |
| V9 | Status counts and the search box collided — WP floats `.subsubsub` with no gap once the labels carry counts. | Spacing on the filter row. |
| V10 | The tab bar sat flush against the content. Core's `.wrap h2.nav-tab-wrapper` outranked the first attempt at a margin. | Matched the specificity. |

### Verified in the browser, not just in code

- Section status hints render and are accurate against live settings.
- The help toggle opens, closes, and stays inline; the panel renders below it.
- Clicking an interaction's name toggles its checkbox (`true → false`).
- Every report block shares one right edge at 1082px.
- Timeline dates read "June 2, 2026 9:12 am", matching the header.

Round 1's lesson was that a method rendering correctly in isolation says nothing
about whether a page is reachable. Round 2's is narrower and about this document:
**a fix recorded as applied is not a fix observed to work.** Four of the ten items
above were things round 1 believed it had already done.

---

## Round 3 — 2026-08-06 (rian's review)

Rian's verdict: the journey report is "much much better", the configuration page is
still lacking — which he noted was funny, since configuration was the focus. Two
things, and the second is the substantive one.

### W1. The margins, properly this time

Rounds 1 and 2 both recorded margins as fixed. They were not, and the reason is the
same shape as V1: **the inset was applied with `padding`, and two of the three
element types on the page do not honour padding as an inset.**

| Element | What `padding: 0 16px` does |
|---|---|
| `<p>`, `<div>` | Insets the content — correct |
| `<table>` under `border-collapse: collapse` | **Nothing.** The collapsed model discards table-box padding |
| `<textarea>`, `<input>` | Insets the *text inside the control*; the control stays flush |

So the prose indented 16px and every settings row and textarea sat hard against the
section border. Each element type now gets the inset in the form it honours —
edge-cell padding for tables, margin for form controls and bordered cards.

### W2. The configuration was a pile of settings, not a path

> "I see stuff like claim the data and journey link to store, but that isn't even
> clear to me what that is even though I've been involved in the building. It's
> missing the code we need to add to the form."

That is the real finding, and it is not a formatting problem. The Handoff tab had
every control needed to configure the feature and no way to be *told what to do*.
Worse, the section named "Setting up the destination" listed four bare API URLs and
stopped — the destination-side script, the actual deliverable, was left as an
exercise for the reader.

*Built:* a four-step wizard (`&wizard=1..4` — a mode of the Handoff tab, not a menu
entry, for the same `$submenu` capability reason as B1). It asks where the form
lives, what should happen, and what the form's fields are called; writes the
settings; and generates the snippet. The field mapping is persisted, so the settings
screen shows the same code regenerated from current settings rather than a stale
copy, and re-running the wizard starts from what is already configured.

### The bug that says the harness is still wrong

Step 4 fataled on first run: `BW_Lead_AI_Settings::handoff_param()` does not exist —
that value is read straight from the settings array. `test-plugin.sh` passed, because
it lints syntax and headers and cannot see an undefined method.

**This is the third render-time failure in this plugin that a green test run did not
catch** (B1 unreachable page, B2 dropped assets, now this). The pattern is consistent:
the checks verify files, not behaviour.

*Added `tests/check-internal-calls.php`* — tokenises every class, collects the methods
each defines, and flags any `Foo::bar(` or `$this->bar(` with no definition. 454 calls
verified in under a second, no WordPress needed. It was regression-tested by
reintroducing the exact bug, which it caught at both call sites.

Two false-positive classes had to be fixed in the checker first, and both are worth
recording: string interpolation (`{$var}`) opens with `T_CURLY_OPEN` but closes with
a plain `}`, so naive brace counting drifts negative and ends classes early; and a
class extending Gravity Forms' `GF_Field` inherits methods we cannot see, so the
checker declines to judge once the chain leaves our code.

### The generated code was verified, not assumed

A wizard that emits broken code is worse than no wizard, so the snippet was tested
rather than eyeballed:

- Parses under `node --check`.
- Behaviour-tested in a `vm` sandbox: token picked up from the URL and persisted,
  claim fetched once and cached, confirm fired on submit — with `sendBeacon`
  present *and* on the `fetch` fallback path.
- Driven against the live REST API end to end: `claim` returned the shared
  datapoints, `confirm` returned 200, and the record moved to `saved` with the
  identity attached.

One harness bug worth remembering: Node 24 has a built-in read-only `navigator`, so
`global.navigator = {...}` silently fails. The first run showed "confirm never
fired" and the snippet was innocent — it had correctly taken the fallback path.

### Known warning, deliberately not silenced

`test-plugin.sh` warns that `tests/check-internal-calls.php` has no `ABSPATH` guard.
It is a CLI script; the guard would exit it immediately. The honest fix is a
`tests/` exclusion in the shared harness — rian's call, since that file is umbrella
tooling used by every plugin.

---

## Round 4 — 2026-08-06 (rian's second review)

Four questions, three of which were findings.

### Q. "There is just one code snippet now. Previously I had 2."

Correct, and one is enough. The old instructions implied two tag-manager tags — one
on page view to claim the data and fill the hidden fields, one on the form-submit
trigger to confirm. The generated script does both, because it registers its own
capture-phase `submit` listener on the document rather than relying on a tag
manager's trigger.

**The honest limit, now stated in the wizard:** that listener sees the browser's
native submit event, which covers ordinary forms including ones injected after page
load. A form that submits purely in JavaScript and never fires the event — some
single-page apps — will fill correctly but never report back. The symptom is
journeys that stay Held after a real enquiry, and the fix is a `sendBeacon` call to
the confirm URL from the form's own success callback.

### Q. "How does it know which fields to insert into?"

Step 3 maps each journey detail to the `name` attribute of a field on the
destination form; that becomes the `FIELDS` object in the snippet, which does
`document.querySelector('[name="…"]')` and fills anything still empty. If you never
saw that step, it is because you chose "just let me see the journey" — which has no
fields to fill. Which leads to the actual bug:

### F1. The wizard marked a step DONE that was never shown

Choosing the recommended "just let me see the journey" skipped step 3, but the step
list was a fixed four items, so **"The fields" rendered with a green completed
tick** while the reader sat on the last step looking for a way forward that did not
exist. The list is now built per run — the skipped step is not shown at all, and the
numbering has no gap.

### F2. The wizard set up its own half and left the rest

Retention, "keep growing" and identity capture were still back on the settings
screen, so finishing the wizard left a half-configured feature. There is now a
plain-English settings step: *how long should a journey be kept · when someone comes
back, what should happen · should journeys have a name on them*. It seeds from
current settings, so a re-run shows the truth rather than first-run defaults.

### F3. "Handoff" was the wrong name for half of what was on the tab

> "there are settings like the journey retention that are applicable whether there's
> cross domain setup or not"

Right, and it was worse than a naming problem — it put general settings behind a
feature most sites never turn on. Split along exactly that line:

| Journeys | Cross-domain |
|---|---|
| Who the visitor is | Modes |
| Datapoints (what a journey stores, and the subset sent) | Destinations |
| How long journeys live | The token |
| Stored journeys | The generated code |

Datapoints stayed one table across both columns because Store and Send are designed
to be read together — Send is visibly a subset of Store — and because the sanitizer
reads both off a single presence marker.

### The refactor immediately broke three things, and a new test caught all of them

Splitting one form into two is exactly the shape that produced the earlier
"saving one tab reset another" bug, so it got a test first:
`tests/tab-save-roundtrip.php` renders each tab for real, scrapes the form it
produced, feeds that through the actual sanitizer, and asserts nothing else moved.

It found, in order: the cross-domain skip list still claiming four settings whose
inputs had moved (each would reset to default on save); the Datapoints block
referencing `$available`/`$sensitive`, locals that did not exist in the new method,
so the table rendered empty while still posting its presence marker — which would
have wiped both datapoint lists on the first save; and the datapoint presence marker
left behind on the wrong form.

It also surfaced a **pre-existing** bug: the Stored journeys panel read
`$stats['pending']` and `$stats['confirmed']`, keys renamed to held/saved earlier in
1.7.0. It had been reporting `0` and `0` regardless of what was stored.

Two of its first findings were bugs in the test itself, both worth recording: field
names like `event_types_checked[…]` contain the substring "checked", so a substring
test marked every box on the Interactions tab as ticked; and `<textarea>`/`<select>`
carry their value as content rather than as a `value` attribute.

`tests/check-internal-calls.php` earned its keep a third time, catching
`$this->help()` in the wizard — `help()` belongs to the admin class — before the page
was ever loaded.

---

## Round 5 — 2026-08-06 (rian: "don't we still need a hidden field for the link?")

Yes. This was a hole in the feature, not just the wizard.

**Journey-link mode is defined as "let the destination store a link back to this
site."** Nothing asked where that link should go, and the generated snippet never
built one — in link mode it claimed nothing, filled nothing, and only fired the
confirm call. So the mode reported "this became a real enquiry" to *us* and left the
destination with no way to reach the journey at all. Matching a CRM lead back to a
journey fell to name-and-email guesswork, which is exactly what the link exists to
avoid.

The old settings screen did list it — "Journey link to store:
`…&token=<token>`" — as one of the four bare endpoints the user was expected to wire
up themselves. Replacing those four URLs with a generated snippet dropped it.

*Fixed:*

- A `handoff_link_field` setting, and the wizard asks for it **first**, in both modes,
  before anything else — it is the field that matters most and it was missing
  entirely.
- The snippet writes `VIEWER + token` into it. No API call: the report URL is this
  site's own address plus the token the browser already holds, so journey-link mode
  now needs no network round-trip at all — one assignment and the confirm on submit.
- Round 4's "adaptive step list" is reverted. Removing the fields step in link mode
  was the wrong fix for the right complaint: the step was never redundant, it was
  *incomplete*. It now always appears and scales — one field in link mode, the full
  mapping when datapoints are being sent.

**Two carry-through bugs found while testing it**, both the same shape as each other
and invisible in code review: stepping Back from the fields step dropped the link
field (step 2 passed a literal `''` to `state_inputs()` instead of the real value),
and the settings step builds its hidden inputs by hand and had no `link_field` among
them, so the answer never reached `apply()`. Caught by walking the flow and reading
the value back at each step rather than only checking the end.

*Verified:* the generated link opens a real report (`token=…` routes to the viewer);
the link-mode snippet fills the field and fires confirm with no fetch at all; the
data-mode snippet fills the link, the mapped datapoints, and reports the email.

---

## Round 6 — 2026-08-07 (Reports dashboard, review loop 1)

Built from `docs/planning/reports-design.md` against the real 434-journey dataset.
All six blocks render; every range renders with zero PHP notices.

### What the review found

| # | Finding | Fix |
|---|---|---|
| D1 | **"+106%" on the 90-day view.** The comparison window reached back before the site had any journeys at all, so the delta measured the day tracking was switched on, not performance. The most confidently wrong number on the page. | Deltas are withheld when the comparison window predates the earliest journey, and the card says why. Verified: 30-day and 7-day keep their real deltas (−18%, −26%); 90-day and year withhold. |
| D2 | **Eighteen channel rows, eleven of them one lead at 0% share.** The design said minimize noise and the table did the opposite. | Top 8 keep their rows; the tail collapses to one "N other channels" row that still carries the leads and assists, so the arithmetic stays whole. |
| D3 | A form with 2 leads shows 70.5 average visits beside forms at 1.3. Real data, but it reads as a bug. | Left for loop 2 — it is entangled with the visit-inflation question below. |

### Verified, not assumed

Every range rendered through the real code path with `error_reporting(E_ALL)` and a
capturing error handler: 7 / 30 / 90 / year / all — no fatals, no notices. The
aggregate's numbers were cross-checked against independent SQL before the UI was
reviewed at all: Direct 88 leads at 5.9 pages and 48% multi-visit against Google
Ads 56 at 1.7 and 18%; Google Organic 18 assists; Contact Form 273 at 3.1 pages
against RISE 151 at 2.1. All agreed.

### A bug the dashboard review surfaced in the data underneath it

While reviewing, rian noticed that multi-visit journeys in the Journeys list were
mostly single visits with repeated identical pages. Checking it found **100% of
multi-visit journeys collapsed to one visit** at a 60-second grouping, with three
"visits" sharing an identical millisecond timestamp.

That turned out to be two separate faults:

1. **A parser bug of mine** — appending a PHP reference to the visit under
   construction, so every appended element aliased the same storage and each new
   visit overwrote the last. Fixed and the history re-imported.
2. **A genuine capture behaviour**, still open: after the repair, multi-visit
   journeys average 2.73 stored visits but 1.80 real sessions at a 30-minute
   boundary, and 39% of page views repeat a URL already seen. The capture appears
   to start a new visit whenever a navigation carries tracking parameters, rather
   than bounding a visit by a session timeout.

The second one matters to this dashboard because "pages read" and "came back" are
two of its three quality columns. They are inflated by a known amount. Loop 2
should either surface that in Data Quality or bound a visit properly at capture —
the latter is the real fix, and it is a separate piece of work.

---

## Round 7 — 2026-08-07 (Reports, review loop 2)

Loop 2 was prompted by a question worth recording, because the answer changed the
product: *"these findings are helpful, but do they rely on Claude being in the
loop? If Claude were not here, would they be caught?"*

### The honest answer, and what it changed

The Data Quality block is plain PHP that runs on every page load for every site —
no AI involved. But **every detector in it existed because something was found by
hand first and then written down as a rule.** The detectors encode findings
already made; they do not discover new ones. The misclassified search hostnames,
the Gmail-as-organic bug, the visit inflation, the parser aliasing bug — none of
those would have been caught by the plugin.

That is how linters and health checks work, and it is fine, but it means the way
this keeps working unattended is to keep converting one-off findings into general
rules. Two from this week generalised, so they were added:

| Detector | The specific finding it generalises |
|---|---|
| `unmatched_referrer` — a channel that is still a bare hostname means no rule claimed it | A broken hostname matcher stored every organic search visit under its raw hostname for months, and the numbers looked plausible throughout. **This detector would have caught it automatically.** |
| `malformed_medium` — a one or two letter medium cannot be real | `google / d` and `google / g`: an ad platform firing its tracking template before substituting the value. |

On the live dataset the first immediately flagged `email.hbci.com` — a webmail
host that belongs in the `email` rule and was not there. It found a real gap on
its first run.

### The finding that mattered most in this round

| # | Finding | Fix |
|---|---|---|
| D4 | **The engagement columns used a mean, and the mean was lying.** Direct showed 5.9 pages read and looked like the site's highest-intent channel — a conclusion that was stated twice in reports to the owner. Its median is **2.0**, identical to organic search; a single visitor who read 164 pages was carrying the whole story. | Median throughout. Long tails are the norm in web analytics, so the mean was wrong on principle, not just on this dataset. |
| D3 | A form with 2 leads shows 70.5 visits. | Inspected: genuine heavy users, not corruption. With n=2 no statistic helps. Left as-is; the real fix is capture-side visit bounding. |

After the switch, Google Organic / Direct / self-referral all read 2.0 pages and
Google Ads reads 1.0. What survives the correction is the genuinely different
number: Direct's *came back* at 48% against 8–19% everywhere else. A percentage
was never vulnerable to the outlier that the mean was.

**The lesson worth keeping:** a statistic that flatters one channel deserves the
same suspicion as a number that looks broken. This one was believed and repeated
because it told a tidy story.

---

## Round 8 — 2026-08-07 (an audit prompted by disbelief)

The owner looked at the finished dashboard and said the result did not match his
experience: Google Organic showed as the biggest channel, but anecdotally Google
Ads had seemed to drive almost everything. He asked to double-check that ads had
not been miscategorised as organic — specifically doubting work I had already done
twice-corrected.

**That was the right instinct and it found a third bug.**

### The reclassification was sound

| Channel | Leads | Reclassified | Carrying any paid marker |
|---|---:|---:|---:|
| Google Organic | 119 | 111 | **2** |
| Google Ads | 56 | 0 | 56 |

117 of 119 organic leads have no paid signal anywhere in their raw summary. The
two that do are genuine multi-touch — an ad found them, organic search closed
them — which is the pattern the dashboard's own "what last-click is hiding" block
is built to show. Ads were never reclassified at all, because reclassification
only ever touched journeys sitting in the `referral` catch-all, and an ad click
arrives carrying a click-ID that never lands there.

### But the doubt was still justified

Auditing it turned up **22 summaries written with an en dash** — "Converted via –
Google Ads" — where the format uses a hyphen. Every prefix test in the parser
matches a literal hyphen, so those entries parsed to nothing at all: no channel,
no landing page, no visits, no journey. They were not visibly broken, they were
absent, and **9 of them were paid ad clicks that consequently looked like leads
with no source**.

After fixing the separator and re-importing:

| | before | after |
|---|---:|---:|
| journeys imported | 434 | **435** |
| unparseable | 1 | **0** |
| with page history | 413 | **434** |
| leads with no channel at all | 17 | **0** |
| credited to Google Ads (incl. malformed mediums) | 56 | **71** |

Google Organic is still the larger channel at 119 against 71. But ads are touching
**80 of 435 leads (18%)** once assists and ad-signalled journeys credited elsewhere
are counted, against the 13% the report showed before the audit.

### The lesson

Three of the bugs in this feature were found because a number looked *wrong to
somebody who knew the business* — not because a test failed. The suites were green
for all three. A report that contradicts the person who lives with the data is not
automatically right; it is a prompt to audit, and twice now the audit has found
that both the report and the intuition were partly correct.

---

## Round 9 — 2026-08-07 (autonomous session review: the three new surfaces)

Reviewed fresh-eyes in the browser: the Reports page with its new AI Analysis
panel, the Reprocess panel, and the Cross-domain tab's new "A property you own"
section.

| # | Finding | Outcome |
|---|---|---|
| R9-1 | A 1559px right edge — the page scrolls horizontally. Chased it down the DOM before touching any CSS. | **Not ours** — WordPress's own #wpadminbar overflows at this viewport with this site's toolbar load. Left alone; "fixing" plugin CSS for it would have been noise. |
| R9-2 | The AI Analysis panel sat at a 1182px edge while all ten other report elements aligned at 1082. It carries `.bw-lead-ai-section.bw-report-block`, and the generic section 1000px cap out-specifies the bare block cap. | Fixed with a two-class selector; re-measured: **every element at 1082**. |
| R9-3 | On a site with handoff off, the Cross-domain tab shows "A property you own" as its only reference section, without "Setting up the destination". | **Correct behaviour, verified deliberately** — the shared-property snippet needs no handoff config; the destination section is rightly gated on it. |

Also verified working on real pages this round: Reprocess panel state ("4 pending"
before, "up to date" after), the corrected came-back column (Direct 30→16%), the
own-property-Direct finding rendered, and the shared-property snippet section with
its distinct copy target.

Process note: the adminbar chase is the round's lesson — measure to the *source*
of an overflow before writing CSS at the symptom.