fix(stats): avg-agents chart + Sankey labels + idle install presence + 0.9.72#78
Conversation
…ith line chart - stats-page.component.ts: remove fsb-avg-agents-per-user early-return in redrawChart(); replace null-returning switch arm in buildChartConfig() with a fsb-tokens-shaped line config computed from latestFsbSeries.d30 as agents_active / unique_installs per day (divide-by-zero guarded). Drop unused DecimalPipe import to silence NG8113. - stats-page.component.html: remove the [hidden] guard and the @if big-number-tile branch under .chart-mount; canvas now renders for this view like every other tab. - stats-page.component.scss: drop the now-unused .big-number-tile rule block. - messages.xlf (+ 5 locale xlf): register new SHOWCASE_STATS_FSB_CHART_AVG_AGENTS_LEGEND trans-unit so ng build passes for de/es/ja/zh-CN/zh-TW locales (translations sourced from existing translations.stats-274.*.json files). - public/sitemap.xml + llms-full.txt: prebuild script regenerated lastmod dates (2026-05-18 -> 2026-05-24).
…nerds"
The browser tab title ("FSB · Stats") is left unchanged - brand context
is still useful for tab/share surfaces. Only the visible page header is
trimmed. Removes the now-unused .dot SCSS rule as well.
…ommits" -> "Commits" The two commit-related views collapse into one. The cumulative line is the more informative of the pair, so it keeps the slot and inherits the shorter "Commits" tab label. The punchcard arm in buildChartConfig and the dual-case in tabMetrics are removed; the StatsViewId union drops 'commits-over-time'. The chart's internal dataset legend stays as "Cumulative commits" since it accurately labels what the line plots. The commitPunchcard() service helper is intentionally left in place - it has dedicated test coverage in tests/stats-chart-overhaul.test.js and removing it would expand the diff. Dead code in the service can be cleaned up in a separate housekeeping pass.
…ts Issues tab - renderSankeySvg() leftX bumped 60 -> 110 (Opened (N) label x=leftX-8 now inside viewBox) - renderSankeySvg() rightX bumped 60 -> 110 (Closed (N) / Still open (N) labels x=rightX+nodeW+8 now inside viewBox) - All Sankey geometry (mkRect, mkPath) reads leftX/rightX/nodeW, so nodes/flows shift inward consistently - Two-line edit only; no SCSS / template / other-chart changes - Angular production build: npm --prefix showcase/angular run build -> exit 0
…talls
- Insert single conditional block between per-group enqueue loop and _applyFifoCap
- When agg.groups.length === 0 (no MCP activity since last beat), push one zero-valued 'periodic' event via _buildEvent
- Reuses already-resolved installUuid and activeAgentCount; no extra storage round-trips
- Inline literals only ('periodic', 'unknown', null, 0); shape dictated by server allowlist
- Sits AFTER BEAT-07 opt-out short-circuit and install_uuid resolution check; privacy + crash invariants unchanged
- Server-side recordSeen() in routes/telemetry.js will now flip idle installs into the /stats active_users_now window
…ESENCE-01 heartbeat - New Section 11 (a/b/c): locks the 3 invariants of PRESENCE-01 - 11.a: idle install + empty queue -> exactly ONE heartbeat POSTed, 9-key shape locked - 11.b: aggregated MCP groups present -> heartbeat does NOT double-emit - 11.c: opt-out short-circuit beats the heartbeat code path (no POST at all) - Section 2: flush #2 now POSTs the heartbeat (was: no POST); flush #3 indices shifted by 1; added heartbeat-shape asserts on the 2nd POST - Section 4: POST body now contains 3 events (2 fresh queue + 1 heartbeat); stale-drop semantics unchanged; heartbeat identified by mcp_client=unknown - Section 7: seed fresh MCP rows between flushes to suppress the heartbeat so the test stays focused on install_announce retry-cap lifecycle; assertion targets install_announce specifically rather than queue length - Section 10: two concurrent flushes now produce 2 serialized POSTs (1 aggregated + 1 heartbeat from the second flush seeing no rows); flushLock invariant unchanged - node tests/telemetry-collector.test.js exits 0 with 105 passed, 0 failed Deviation rationale: the plan's truth that "all 10 sections continue to pass unchanged" was structurally incompatible with the plan's mandated unconditional heartbeat behavior. Tests asserting "no POST when no MCP rows" were testing behavior that is now legitimately changed by design. Sections 1/3/5/6/8/9 untouched. Section 11 added verbatim from plan.
…ESENCE-01 heartbeat Same alignment pattern as the executor applied to telemetry-collector.test.js Sections 2/4/7/10 -- this sibling test was missed in scope. Both scenarios seed empty fsbUsageData so the new presence heartbeat now legitimately fires alongside the seeded queue events; the wire-strip and re-enqueue contracts still apply uniformly to all events (including the heartbeat). - Scenario A: POST body event count 2 -> 3 (2 seeded + 1 heartbeat) - Scenario B: wire body 1 -> 2 events; residue queue 1 -> 2 events; residueEvent lookup switched from queue[0] to find(event_id === seedId) so the seeded event's attempts-retention assertions still target the right event regardless of in-queue ordering. No change to the implementation; the heartbeat behavior itself was already covered by Section 11 of telemetry-collector.test.js.
Six canonical version surfaces bumped (covers the APP_VERSION gap that PR #56 missed at 0.9.66 -- live homepage stayed on the old version post-deploy): - extension/manifest.json (name + version) - package.json (version + package script zip filename) - package-lock.json (top-level + packages[""] version) - showcase/angular/package.json - showcase/angular/package-lock.json - showcase/angular/src/app/core/seo/version.ts (APP_VERSION) mcp/package.json (0.9.2) is independently versioned per the extension/MCP-server release cadence -- not part of this bump. Verification: - node tests/mcp-version-parity.test.js -> 10 passed, 0 failed - node tests/verify-store-listing.test.js -> 3 passed, 0 failed - node scripts/verify-store-listing.mjs -> PASS (5 checks green)
…op orphan tile label CI-02 (Verify ng extract-i18n produces no diff) failed on PR #78 because the earlier stats-page edits in this branch shifted line numbers above all FSB-prefixed trans-units AND deleted the big-number tile (whose SHOWCASE_STATS_FSB_TILE_AVG_AGENTS_LABEL trans-unit had been deferred for cleanup per the avg-agents fix SUMMARY). Re-running `npx ng extract-i18n` produces exactly the expected delta: - Drops the orphan trans-unit (no template references it anymore) - Shifts 13 linenumber annotations to match the current source Locale variants (de/es/ja/zh-CN/zh-TW) still carry the orphan target translations -- harmless because Angular build only errors on MISSING translations, not orphan ones. Locale-side cleanup is a separate i18n housekeeping pass.
… TDD cases - New clientLabelFor(ownerAgentId, labelsMap) helper -- looks up the canonical MCP client label for an owner agent from a plain map; returns null in every "no canonical label available" path so the caller can fall back to the legacy ownerLabelFor formatter. - Exported on both globalThis.FSBOwnerChip and module.exports. - Pre-existing helpers (shouldShowOwnerChip / buildChipText / ownerLabelFor / findOwnerInEnvelope) are byte-unchanged. - 6 new ok() cases in tests/owner-chip.test.js cover: happy path, missing agentId, null map, undefined map, array/string map, empty-string value.
…e.session
- New FSB_AGENT_CLIENT_LABELS_KEY constant ('fsbAgentClientLabels') sits
near MCP_NAVIGATION_RECOVERY_TOOLS and is exported on
_mcp_dispatcher_exports so consumers (popup, sidepanel) share the same
storage key as the dispatcher writer.
- New async _persistAgentClientLabel(agentId, label) helper: best-effort
fire-and-forget write to chrome.storage.session, no-throw on missing
chrome API or storage failure, no-op when the label already matches
(avoids storms on hot dispatch paths).
- resolveMcpClientLabel now fire-and-forgets the persistence helper
immediately after the existing _agentClientLabelCache.set call when a
real (non-'unknown') label is observed with an agentId. Function
remains synchronous; return values are byte-identical; both
recordDispatch call sites untouched.
- clearLastKnownMcpClientLabel now also fires a best-effort
chrome.storage.session.remove so a fresh bridge reconnect does not
inherit the prior bridge's persisted labels.
- Test pair: tests/mcp-dispatcher-client-label.test.js still 45/45 PASS
(byte-identical semantics + regression guards intact);
tests/owner-chip.test.js 45/45 PASS.
…hat input
popup.js + sidepanel.js (identical shape across both surfaces):
- New module-scope _chatLockedByOwnerChip flag (declared near isRunning /
stopRequested), set/cleared exclusively by refreshOwnerChip.
- updateSendButtonState composes the new gate via OR:
sendBtn.disabled = !hasContent || isRunning || _chatLockedByOwnerChip
Preserves existing hasContent / isRunning behaviour; no normal idle /
typing / running transition leaves the input enabled while the chip is
showing, and no transition leaves it disabled after ownership is released.
- refreshOwnerChip now reads both fsbAgentRegistry AND fsbAgentClientLabels
in a single chrome.storage.session.get round-trip, prefers
FSBOwnerChip.clientLabelFor() over ownerLabelFor() (falls back when no
canonical label has been observed yet for this owner), and toggles the
contenteditable attribute on the chat input + the _chatLockedByOwnerChip
flag based on chip visibility. setAttribute('contenteditable', 'false'/
'true') is used because chatInput is a contenteditable DIV; .disabled
does not block typing on contenteditable elements.
- Defensive null guards: both the FSBOwnerChip-undefined branch and the
missing-tab branch now release the lock so a helper-unload race or empty
tab query cannot strand the input permanently disabled.
- setRunningState / setIdleState / setErrorState gained inline comments
noting that their bare sendBtn.disabled assignments are safe because
updateSendButtonState re-derives the disabled state honouring the new
flag immediately after.
sidepanel-only:
- chrome.storage.onChanged listener now refreshes the chip on EITHER
fsbAgentRegistry OR fsbAgentClientLabels changes so a freshly-arriving
label re-renders immediately without waiting for a tab switch.
- popup has no equivalent listener by design (short-lived surface;
close-and-reopen is the natural refresh path).
Verify:
- node tests/owner-chip.test.js -> 45/45 PASS (source-level audits 8a-10c
still PASS; chip stays a <span>, no HTML touched).
- node tests/mcp-dispatcher-client-label.test.js -> 45/45 PASS
(byte-identical resolveMcpClientLabel semantics + regression guards).
|
@codex review This PR is a multi-area batch on the FSB extension + showcase site. Areas of interest for review:
Tests: Please flag any P1/P2 issues you would normally surface. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71f4efc92c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const prior = await chrome.storage.session.get([FSB_AGENT_CLIENT_LABELS_KEY]); | ||
| const existing = prior && prior[FSB_AGENT_CLIENT_LABELS_KEY]; | ||
| const map = (existing && typeof existing === 'object' && !Array.isArray(existing)) ? existing : {}; | ||
| if (map[agentId] === label) return; // no-op write -- avoid storm of identical writes on hot dispatch paths | ||
| map[agentId] = label; |
There was a problem hiding this comment.
Concurrent dispatches can clobber each other here: two _persistAgentClientLabel calls can both read the same prior map, each mutate its own copy, and the later set overwrites the earlier update. In hub mode with multiple active agents, this loses agent→label entries intermittently, so popup/sidepanel chips fall back to agent_<hex> even after labels were observed. This was introduced with the storage mirror path and needs a serialized/merge-safe write strategy.
Useful? React with 👍 / 👎.
| if (agg.groups.length === 0) { | ||
| var beat = _buildEvent({ | ||
| install_uuid: installUuid, | ||
| mcp_client: 'unknown', | ||
| model: null, |
There was a problem hiding this comment.
This unconditional idle heartbeat enqueue runs even when queue already has unsent retry events. At queue cap, each idle flush adds one heartbeat and then _applyFifoCap drops the oldest real event, which causes avoidable telemetry loss before attempts/stale policies would have removed those events. Since any existing queued event already marks the install as seen when posted, the heartbeat should be suppressed when there is pending backlog.
Useful? React with 👍 / 👎.
| _chatLockedByOwnerChip = true; | ||
| chatInput.setAttribute('contenteditable', 'false'); | ||
| chatInput.title = 'Disabled while tab is owned by ' + displayLabel; | ||
| updateSendButtonState(); |
There was a problem hiding this comment.
When this path sets _chatLockedByOwnerChip = true, the popup can remain locked after ownership is released because popup.js only calls refreshOwnerChip() on initial load and does not subscribe to chrome.storage.onChanged/tab-change updates (unlike sidepanel.js). If ownership flips while the popup stays open, the input and send state can be stale until the user closes/reopens the popup, which is a functional regression from the new lock behavior.
Useful? React with 👍 / 👎.
| if (typeof chrome !== 'undefined' && chrome.storage && chrome.storage.session | ||
| && typeof chrome.storage.session.remove === 'function') { | ||
| // Fire-and-forget; bridge reconnect must not stall on storage. | ||
| chrome.storage.session.remove(FSB_AGENT_CLIENT_LABELS_KEY).catch(function() {}); |
There was a problem hiding this comment.
The reconnect reset is intended to clear persisted labels, but _persistAgentClientLabel runs asynchronously and fire-and-forget, so an in-flight persist from the previous connection can complete after this remove and repopulate fsbAgentClientLabels with stale data. That violates the stated “fresh reconnect” invariant and can temporarily show incorrect client ownership labels until newer dispatches overwrite them.
Useful? React with 👍 / 👎.
…gs 1+4) Wrap _persistAgentClientLabel's chrome.storage.session get/set pair and clearLastKnownMcpClientLabel's chrome.storage.session.remove in a shared module-scope promise-chain mutex (_withLabelStorageLock), mirroring the agent-registry.js _registryChain pattern verbatim. - Finding 1: two concurrent resolveMcpClientLabel persists in hub mode no longer race -- the second persist's read sees the first persist's committed write (no lost updates). - Finding 4: a chrome.storage.session.remove submitted AFTER an in-flight persist is now serialized after that persist, so the final state is empty (no stale-label resurrection after bridge reconnect). Tests 10 + 11 in tests/mcp-dispatcher-client-label.test.js install a microtask-resolving chrome.storage.session shim on globalThis, freshly re-require the dispatcher, then exercise the real RMW paths. Both tests fail against the prior code and pass after the mutex lands.
The PRESENCE-01 heartbeat enqueue now requires BOTH `agg.groups.length === 0` AND `queue.length === 0`. Pre-fix the heartbeat fired whenever aggregation produced no new groups -- but a non-empty queue (unsent retry events from a prior failed POST) would receive the heartbeat on top. At queue cap (200) _applyFifoCap would then drop the OLDEST real event to make room for a redundant heartbeat -- telemetry loss for nothing, since the retried event's own POST already triggers server-side recordSeen() for this install_uuid. Section 11.d in tests/telemetry-collector.test.js seeds the queue with one attempts:1 retry event and zero new MCP rows, then asserts the POST contains exactly one event (the retry) with no heartbeat. The new section also cross- checks that the heartbeat invariant still fires when the queue is genuinely empty. Pre-existing Section 4 (telemetry-collector.test.js) and Scenarios A + B (telemetry-collector-strips-internal-fields.test.js) asserted the prior "heartbeat lands on top of seeded queue events" behaviour. Updated those counts to match the new contract (heartbeat suppressed when queue is non- empty). All wire-strip allowlist assertions remain unchanged.
…refresh (Finding 3) Mirrors sidepanel.js:224..241 verbatim: popup.js now installs a module-scope chrome.storage.onChanged listener that calls refreshOwnerChip() whenever fsbAgentRegistry or fsbAgentClientLabels mutates in the session namespace. Pre-fix the popup chip went stale until close/reopen if an MCP agent claimed the active tab while the popup was held open (e.g. during a long automation). Post-fix the chip re-renders within microseconds of the storage write. Defensive guards (outer try + chrome-availability checks) ensure popup boot is never poisoned in test environments without the chrome.storage.onChanged API. The arrow body does NOT need its own try -- refreshOwnerChip already wraps its body in try/catch. Test 8f (5 assertions) in tests/owner-chip.test.js asserts the listener is present, references both keys in a single branch, and gates on area === 'session'. Sidepanel.js is unmodified.
|
@codex review Re-review request: the 4 P2 findings from your prior review at 2026-05-24T11:02:48Z have been addressed across 3 atomic commits on top of 71f4efc. Please confirm each fix is sufficient or surface any gaps. Finding 1 + Finding 4 (mcp-tool-dispatcher.js label-storage RMW race + reconnect-stale-resurrection) Finding 2 (telemetry-collector.js heartbeat enqueue with pending backlog) Finding 3 (popup.js missing chrome.storage.onChanged listener) CI: all 8 jobs green on the latest commit cab3913. Please confirm or flag any residual issues. |
|
@chatgpt-codex-connector review Friendly re-ping. All 4 P2 findings from your prior review (2026-05-24T11:02:48Z, commit 71f4efc) are now fixed across commits 522e5f7, 16339c3, cab3913. Please confirm the fixes hold up. If the @codex handle is the canonical trigger and the @chatgpt-codex-connector handle is not -- @codex review please. |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Eight commits batched on a single branch. Three concerns, all driven from the live
/statspage observations + a follow-on extension-side fix for the active-users metric./stats UI fixes
b72e74f3Average agents per user tab: replace the overflowing big-number tile with a line chart in.chart-mount. Root cause: SCSSdisplay: block !importantoverrode[hidden]on the canvas, so the empty canvas kept its 360px slot and pushed the big-number tile into the footer. Now renders a per-day avg =agents_active / unique_installsseries (mirrors thefsb-tokensarm). Adds localized legend across all 6 XLIFF files.42e8906aH1 trim:FSB · Stats for nerds->Stats for nerds. Drops the now-unused.dotSCSS rule. Browser tab<title>(FSB · Stats) left as-is for share/tab brand context.23b62afaDrop the standaloneCommitspunchcard tab; renameCumulative commits->Commits. Two near-duplicate tabs collapse into one; the cumulative line is the more informative of the pair.commits-over-timeremoved from theStatsViewIdunion and the chart-config switch.commitPunchcard()helper retained ingithub-stats.service.tsbecause dedicated tests still cover it (separate housekeeping pass can prune later).966ff696Sankey labels on the Issues tab were clipped at the viewBox edges (Opened (N)off the left atx=52anchor=end;Closed (N)off the right atx=548anchor=start). Bumped the symmetric inset constant60 -> 110inrenderSankeySvg()so labels fit insideviewBox=0 0 600 240. Two-line edit; geometry (flow paths, node rects) all derive fromleftX/rightX/nodeWso they self-adjust.Extension telemetry -- idle installs now count toward active-users
71b800d4Presence heartbeat intelemetry-collector._runFlush(). Previously, when_aggregateMcpRowsSinceWatermark()returned zero groups (idle install with no MCP activity), nothing was enqueued, nothing POSTed, and the server'sactive-tracker.recordSeen()never fired -- so installed-and-alive extensions were invisible tocountActiveUsers(5min). Fix enqueues exactly one zero-valuedevent_type:'periodic'event withmcp_client:'unknown'when there are no MCP groups. Server contract unchanged (heartbeat shape already in the allowlist).d92f2f03Test coverage:Section 11(3 sub-scenarios) for the new heartbeat path plus alignment of Sections 2/4/7/10 oftelemetry-collector.test.jsthat had assumed silent-on-idle behavior.c992680bSame alignment pattern applied to the siblingtelemetry-collector-strips-internal-fields.test.js(scope leak from the previous quick task -- caught by re-running fullnpm run ci).Release
0dc0b02aVersion bump0.9.71 -> 0.9.72across six canonical surfaces, includingshowcase/angular/src/app/core/seo/version.ts(theAPP_VERSIONconstant PR chore(release): bump version 0.9.65 -> 0.9.66 #56 missed at 0.9.66).mcp/package.json(0.9.2) is independently versioned and not part of this bump.Test plan
npm run ciexits 0 (validate:extension + full test suite + mcp-smoke + showcase:build + showcase:smoke=48 passed)node tests/telemetry-collector.test.js-> 105 passed (Sections 1..11)node tests/telemetry-collector-strips-internal-fields.test.js-> 17 passednode tests/mcp-version-parity.test.js-> 10 passednode tests/verify-store-listing.test.js-> 3 passednode scripts/verify-store-listing.mjs-> PASS (5 checks green)npm --prefix showcase/angular run build-> exit 0/stats:Stats for nerds(no FSB prefix)Commitstab (the cumulative line chart)Average agents per userrenders a line chart inside the chart container/statslocalized routes: legend reads in DE / ES / JA / zh-CN / zh-TWactive_users_nowwithin ~5 min of the next telemetry beat