Branch review, 2026-08-30

What is this?

A maximum-rigor read of origin/dev..feat/admin-and-merchant-portals before merge, concentrated on the ~97 commits landed today.

• 305 commits, 923 files, +70584 / -3631. npm run validate is green (37 passed, 1 skipped, 0 failed) and the admin suite is 684, so this looks only for what a gate cannot see.
• Scope of the pass: the header allowlist in packages/shared/services/index.js, the new apps/admin/src/admin/dashboard/ and its runtime ALPHA.md parser, the offerInheritance / offerPlacements move, the --muted-dark-2 token split, the ad-network and analytics section moves, the new analytics endpoints and the Firestore query console, the sliding unlock window, and the hand GCP changes recorded today.
• Already-tracked items were checked against the open issue list and are not restated here: #1000, #1004, #1009, #1011.

Count by severity: 2 high, 2 medium, 4 low. No blocker to merging, and nothing found that is a security or privacy regression.

High

1. The Dashboard's launch timeline and changelog serve indefinitely stale cached markdown, and Refresh cannot clear it.

• Where. apps/admin/src/admin/dashboard/releaseFeed.js:170, :175, :188, :428 call listDocuments() / getDocument() with no options. apps/admin/src/shared/lib/docsApi.js:322-353 (getDocument) and :277-306 (listDocuments) are stale-while-revalidate over IndexedDB, with no TTL, no eviction, and a background refresh whose only effect without an onBackgroundUpdate callback is to rewrite the cache row.
• Failure path.
  1. The operator opens the Dashboard on Monday. loadAlphaPhases() fetches docs/business/launches/ALPHA.md and caches it under that path.
  2. During the week ALPHA.md gains five ✅ Built rows and a new phase.
  3. The operator reopens the Dashboard. getDocument(ALPHA_PLAN_PATH) finds the cache row and returns Monday's content. The background revalidate rewrites the cache but passes nothing back, so React state never updates and the rendered phase row keeps Monday's counts for the whole session.
  4. Pressing Refresh runs useAdminDashboard.js:200-207 to loadProgress() to loadAlphaPhases() to getDocument(path) with no forceRefresh, which hits the same cache. The counts cannot change until the next page load, which then shows the previous session's content. The row is permanently one page load behind and Refresh is a no-op for the entire reading column.
• Why this outranks a normal staleness bug. It falsifies the change's own stated purpose. releaseFeed.js:290-293 says "Deriving it from the plan makes the row true by construction and keeps the plan the only place phase state is written down", and LaunchTimeline.jsx:22-26 says "the plan is now the only place the state is written, so the row cannot drift from it". It can drift. The drift moved from hardcoded copy into the client cache, which is harder to see.
• Same path, same effect on the Release changelog card: a new docs/changelogs/dev/ file does not appear until a second page load, because listDocuments({ dir }) returns the cached listing.
• Contrast that shows it is an oversight, not a decision. SelfHostedDocsEditor.jsx:405, :421, :487 pass forceRefresh: true and onBackgroundUpdate at exactly these call sites. The Dashboard passes neither.
• Fix shape. forceRefresh: true on the refresh path, onBackgroundUpdate on mount. No API change needed.
• Confirmed, by reading both call sites and the cache implementation end to end.

2. analyticsApi.js still turns a non-JSON 200 into data, which is the exact shape merchantsApi.js fixed today.

• Where. apps/admin/src/shared/lib/analyticsApi.js:33-63. A body that will not parse becomes data = { detail, raw: true }, and the throw is gated on !response.ok, so a 200 with an HTML body is returned to the caller as data.
• The sibling got the guard this session. apps/admin/src/shared/lib/merchantsApi.js:14-33 now throws NON_JSON_RESPONSE on a 2xx that is not JSON, with a comment naming #981 and the four days it hid. The same hole is one file over, in the client every headline surface added today reads through.
• How a 200 HTML body actually arrives.
  • analyticsApi.js:10-11 builds every URL from VITE_ANALYTICS_API_URL, which is /api in the repo-root .env.local. Vite's envDir is the repo root (apps/admin/vite.config.js:22), so that is the file that counts.
  • The /api/analytics proxy is registered only when ANALYTICS_API_ORIGIN is set (apps/admin/vite.config.js:28-34). That variable is not committed, so a fresh checkout has the base path without the proxy: the #981 configuration exactly.
  • Vite's SPA fallback then answers. Verified in the installed Vite 8.2.1 source at node_modules/vite/dist/node/chunks/node.js:19266: the middleware runs for GET/HEAD when accept includes */*, which is what fetch sends by default (apps/admin/src/shared/lib/apiClient.js:33-40 sets no Accept), and rewrites the request to /index.html. The response is 200 text/html.
• Wrong output.
  • useDashboardRail.js:110-175: getRealtimeActivity() and getSystemHealthApi() resolve to an HTML blob, count() returns null for every read, and all five analytics-sourced tiles render "Not available" with no error shown anywhere.
  • AdPlacements.jsx:116-124: result.placements is undefined, error stays null, and every metric on all three placement cards renders the unavailable glyph. The card copy at :199-201 explicitly promises that absence means "not measured yet" rather than a failure, so the page states the opposite of what happened.
  • Delivery and Economics take the same path.
• Not affected. POST routes (/analytics/admin/firestore-query, bq-query) are outside the GET/HEAD fallback and 404 correctly.
• Not confirmed. What VITE_ANALYTICS_API_URL is set to on the deployed Cloudflare Pages build. If it is the absolute Cloud Run URL there, this is a local-dev-only trap, which is still the trap that cost four days in August.

Medium

3. fetchAdEconomics sums a row list capped at 5000 and drops the truncation flag, so per-campaign figures silently undercount.

• Where. services/api/analytics/src/services/adDelivery.service.js, fetchAdEconomics builds byOffer from delivery.rows. fetchAdDeliveryRows returns truncated: rows.length >= MAX_ROWS (line 609) and its row query carries LIMIT ${MAX_ROWS} (line 257) with MAX_ROWS = 5000 (line 78). fetchAdEconomics reads delivery.rows and never reads delivery.truncated, and the /ads/economics response in routes/admin.js carries no truncation field, so AdEconomicsReport has nothing to render.
• Failure path. ad_delivery_daily has grain (day, environment, offer_id, merchant_id, placement, target_audience) and an offer carries one placement, so rows per day is roughly the number of offers that delivered. At about 166 delivering offers per day the default 30-day window saturates the cap; the route also accepts up to MAX_RANGE_DAYS = 366, where about 14 delivering offers per day is enough. Past that point every campaign's fills, impressions and clicks are lower than the truth, with nothing on the page saying so.
• The sibling paths avoid it deliberately. /ads/delivery computes totals in SQL for exactly this reason (buildAdDeliveryTotalsSql), and fetchAdPlacementSummary uses a GROUP BY. Economics is the one read that walks rows.
• Latent today at 9 offers, so this is a correctness bug on a timer rather than a live one.
• Confirmed by reading the grain in the aggregation SQL and the cap in the read path.

4. The /api/docs Vite proxy added on this branch is inert, and its comment claims the opposite.

• The claim. apps/admin/vite.config.js:56-66: "Docs was the one service with no proxy entry, so docsApi called its Cloud Run URL straight from the browser and died on CORS from any origin not in the server allowlist (e.g. a tailnet-IP preview). Proxying keeps the call same-origin like every other service."
• What the code does. apps/admin/src/shared/lib/docsApi.js:16 still builds every URL from VITE_DOCS_API_URL || 'https://docs-api-531553779372.us-central1.run.app'. Neither VITE_DOCS_API_URL nor DOCS_API_ORIGIN is set in the repo-root .env.local, so the proxy branch never registers. Even with DOCS_API_ORIGIN set, the client would still not use it: the rewrite strips ^/api/docs, so the client would have to be pointed at VITE_DOCS_API_URL=/api/docs to land on /api/documents, and nothing does that.
• Consequence. The new Dashboard's whole reading column calls the deployed docs-api cross-origin from the browser. http://localhost:5173 is in ADMIN_ORIGINS (packages/shared/services/index.js:84-104), so it works from the standard port and fails from any other origin, which is precisely the tailnet-preview case the comment says was fixed.
• Confirmed by reading the config, the client, the env file and the origin list.

Low

5. Post Announcement discards a typed draft with no feedback.

• apps/admin/src/admin/dashboard/PostAnnouncementModal.jsx:138 calls onSubmit({ title, message, pinned }); AdminDashboard.jsx:335-338 calls onPostAnnouncement?.(draft) and closes the modal; AdminDashboardPage.jsx:34-45 passes no onPostAnnouncement. So a real announcement typed into the modal vanishes on click, and the modal closes as if it had posted.
• Documented as intentional pending #992 (AdminDashboard.jsx:257-261, PostAnnouncementModal.jsx:26-29), and the card behind it explains that nothing is stored. Flagged only because the submit control itself says nothing, and a person who opens the modal first never reads the card.
• Likely already inside #992's scope. Not filed as a defect.

6. The rail rows carry a retired destination.

• apps/admin/src/admin/dashboard/useDashboardRail.js:128 and :135 set to: '/admin/analytics/dashboards' on the Lanterns and Waves rows. That path was retired on this branch and now redirects to Event Taxonomy (apps/admin/src/admin/AdminShell.jsx:831-834), which is not where those numbers live. The file's own header (line 12) names Venue Activity as the source screen.
• Not a live failure path. CountsRail.jsx renders no links and nothing outside useDashboardRail.js reads .to. It becomes a wrong cross-link the moment anyone wires the field its name invites.

7. A comment points at a file this branch deleted.

• packages/ui/offers/FeedOfferCard.stories.jsx:40 references apps/admin/src/merchant/offers/constants/placements.js, moved to apps/admin/src/shared/lib/offerPlacements.js in e4695e24. All 19 real importers were repointed; this one comment sits in packages/ui and was outside the sweep.

8. Half the launch-plan parser is exercised only by its tests.

• loadLaunchStages, parseLaunchStages and parseStageMeanings in releaseFeed.js:186-277 are imported by __tests__/releaseFeed.test.js and by nothing else. The Dashboard hardcodes the stage row in dashboardCopy.js, which says so and explains why.
• Not a defect. Noted because 531 lines of test cover a parser no screen calls, while the half that IS called reaches the screen through the cache in finding 1.

What was checked and found clean?

The header allowlist does what it says, verified live rather than by reading.

• Ran a real Express app with pinoHttp({ redact: PINO_REDACT_CONFIG }) and app.set('trust proxy', 1), sent one request carrying x-forwarded-for, user-agent, authorization, cookie, x-firebase-appcheck, sec-ch-ua-platform and origin, and read both the log line and what the route handler saw.
• The log line kept only host and origin on the request and content-type on the response. set-cookie and an arbitrary x-custom-leak response header were both dropped. None of the six planted secrets appeared in the line.
• x-forwarded-for still reaches the middleware. The handler read req.headers['x-forwarded-for'] intact and req.ip resolved through the proxy chain correctly. The separation the change depends on holds, and the redaction restore is clean because pino-std-serializers stores res.headers as a plain value rather than a getter (node_modules/pino-std-serializers/lib/res.js:35).
• No service registers a res.on('finish') handler, so nothing reads headers during the window when fast-redact has them mutated.
• The req.url query-string leak the config calls a KNOWN GAP is real (the raw ?phone=... appears in the line while req.query.phone is redacted). It is stated in the code and tracked on #1004, so it is not counted here.

The ALPHA.md parser is correct against the real file, and its failure modes are the declared ones.

• Ran parseAlphaPhases and parseLaunchStages against the live docs/business/launches/ALPHA.md and README.md. Six phases parse, counts total 59 rows with zero unknown, and the stage table and meanings table both resolve without the second table poisoning the first.
• Column reorder is safe (Status is found by name). A new status marker degrades to unknown, which counts against its phase, as documented. A renamed Status header or a phase with no table throws and the row says "Launch plan unreadable", which is the declared behaviour rather than a silent wrong answer.
• Behaviour on a heading rename of ## What each stage means is a graceful empty, guarded by the end-of-input note at releaseFeed.js:220-229.

Everything else that was opened and traced.

Surface	Result
--muted-dark-2 split into four tokens	All four defined once in apps/admin/src/shared/styles/styles.css, every var() consumer inside a cascade that sees them, no dangling reference anywhere in source, commit-message counts match the diff
offerInheritance / offerPlacements move	100 percent-similarity renames, all importers resolve, no duplicate copies, no export mismatch, tests are real
Ad Network and Analytics section moves	Every nav item, tab and cross-link resolves against the route table; every tab strip's active detection matches the new paths; no deleted component is still imported
Firestore query console allowlist	Field-level allowlist enforced before the read and again on projection, document key gated per collection, maps summarised by key count, filter VALUES kept out of the audit log
/ads/placements k-anonymity	Ungated deliberately and consistently with /ads/delivery's admin path; validateDateRange is applied on both new read paths
countEligibleOffersByPlacement	offer.placement is the real singular field (services/api/merchants/src/routes/offers.js:49), and "expired" is a derived status so stored-active is the right filter
usersProfilesComplete fix	encryptedInterests / encryptedMood are the real written field names, and the plaintext fields really are deleted on save (apps/web/src/lib/profileService.js:322-329)
addressConfirmed on manual venues	All four patch shapes (pin only, address only, both, explicit flag) resolve to the documented value
Sliding unlock window	getSessionEntropy has exactly two callers, both user-driven, so the claim that idle time still expires the key holds
seed-ad-network.mjs	The prod guard matches the real project id lantern-app-prod, and selectOffers really does drop an offer whose venue doc is missing (packages/shared/ads/index.js:186-197)
Failed offer decision inside the drawer	Fix is complete: OfferReviewDrawer has exactly one caller, and the assertion is scoped to within(getByRole('dialog'))
Cloud Run request-log exclusion	Both sinks, reversal recorded, and the loss it names (rows for requests that never reached our code) is the loss that actually occurs; the System Health Errors tab reads severity>=ERROR across all logs, so our own pino lines survive it

What was too uncertain to file?

Reporting windows are UTC days while the picker is local.

• bqMetrics.service.js resolveWindow builds from at 00:00:00Z and an exclusive end at midnight UTC after to. A Pacific operator picking "Aug 30" gets 17:00 Aug 29 to 17:00 Aug 30 local. The ad rollup buckets on DATE(timestamp), presumably also UTC, so the two are probably consistent with each other and only inconsistent with the reader. Not filed: it may be the intended reporting convention and I did not confirm what the picker means by a day.

Four services set no trust proxy.

• lanterns, docs, venues and analytics do not call app.set('trust proxy', 1) (assistant, merchants and auth do). Any IP-keyed limiter on those would collapse to one bucket behind Google's front end. I checked the docs-api limiter, which keys on req.user.uid and only guards writes, so it is unaffected. I did not audit the other three, and in any case this predates the branch and is not something the header change altered.

ratioPct(stats.impressions, stats.fills) can exceed 100 percent.

• AdPlacements.jsx:213 labels impressions over fills as "Fill to view rate". If the rollup ever records more impressions than fills for a placement the card reads above 100 percent. Whether that combination can occur depends on the client event contract, which I did not trace.