Skip to content

feat: [ENG-2621] serve analytics disclosure markdown via daemon + canonicalise privacy URL#752

Merged
ncnthien merged 7 commits into
proj/analytics-system-tool-modefrom
feat/ENG-2621
Jun 2, 2026
Merged

feat: [ENG-2621] serve analytics disclosure markdown via daemon + canonicalise privacy URL#752
ncnthien merged 7 commits into
proj/analytics-system-tool-modefrom
feat/ENG-2621

Conversation

@ncnthien
Copy link
Copy Markdown
Collaborator

@ncnthien ncnthien commented Jun 1, 2026

Summary

Two related changes that finish the wiring between `src/shared/assets/analytics-disclosure.md` and the local web UI's Privacy tab.

1. Privacy URL canonicalised (`e8de448a`)

`PRIVACY_POLICY_URL` in `src/shared/constants/privacy.ts` now points at the published `https://www.byterover.dev/services/privacy\` page. Drops the stale "M1.5 placeholder" comment.

2. Daemon-served disclosure markdown (`94b1cae7`)

The webui Privacy panel previously rendered a hardcoded 2-column icon grid of Lorem-ipsum bodies, which drifted out of sync once the real PM/legal copy landed in the markdown file (commit `125b5c44`).

  • New transport event `analytics:getDisclosure` + Zod response schema (`AnalyticsDisclosureResponseSchema`).
  • New `AnalyticsDisclosureHandler` wraps `loadAnalyticsDisclosureText()` (already used by the CLI consent prompt) and exposes it over the transport. Wired into `feature-handlers.ts` next to the other Analytics handlers.
  • Webui `useGetAnalyticsDisclosure` TanStack Query hook + `DisclosureDetails` rewrite that renders the markdown via the existing `MarkdownView` from the contexts feature (handles lists, code blocks, links).
  • Deletes the obsolete `ANALYTICS_DISCLOSURE_SECTIONS` array, the icon grid layout, and the duplicate `ANALYTICS_PRIVACY_URL` constant. `analytics-panel.tsx` now consumes `PRIVACY_POLICY_URL` directly from `src/shared/constants/privacy.ts`.
  • Test coverage: 3 handler tests (`analytics-disclosure-handler.test.ts`) + 3 webui API hook tests (`get-analytics-disclosure.test.ts`); the obsolete constants test file is removed.

Single source of truth is now the `.md` file — PM/legal edits propagate to both the CLI prompt and the webui Privacy panel with no code changes.

Test plan

  • `npm run dev:ui` → `brv webui` → Configuration → Privacy → "What data will be collected?" section renders the real markdown (5 H2 sections, bullet list, code block for the CLI command, privacy link)
  • Refocus the tab → disclosure refreshes (TanStack Query default + override on the globalConfig query)
  • Click the footer privacy link → opens `https://www.byterover.dev/services/privacy\` in a new tab
  • Toggle on/off still routes to the enable/disable confirm dialogs
  • `./bin/dev.js settings set analytics.share true` shows the same markdown text in the CLI prompt
  • `npm run lint`, `npm run typecheck`, `npm test` green

ncnthien added 2 commits June 1, 2026 18:01
Replace the docs.byterover.dev placeholder with the canonical
https://www.byterover.dev/services/privacy URL now that the public page
has been published. Drops the stale "placeholder until M1.5" comment —
the URL is no longer a placeholder.
…ender it in the webui

Single source of truth for the disclosure copy lives in
src/shared/assets/analytics-disclosure.md and was already consumed by the
CLI consent prompt via loadAnalyticsDisclosureText(). The webui's Privacy
panel had a separate hardcoded copy that drifted out of sync once the real
PM/legal copy landed.

- Add `analytics:getDisclosure` transport event + Zod response schema.
- New AnalyticsDisclosureHandler wraps loadAnalyticsDisclosureText() and
  exposes it over the transport; wired into feature-handlers.ts alongside
  the other AnalyticsHandler family.
- Webui `useGetAnalyticsDisclosure` query hook + render via the existing
  MarkdownView from the contexts feature so lists, code blocks, and links
  in the markdown render properly.
- Delete the now-obsolete ANALYTICS_DISCLOSURE_SECTIONS / icon grid and
  the ANALYTICS_PRIVACY_URL duplicate; analytics-panel.tsx now consumes
  PRIVACY_POLICY_URL directly from src/shared/constants/privacy.ts.
- Drop the constants test file along with the hardcoded data it covered.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 1, 2026

Claude finished @ncnthien's task in 2m 52s —— View job


Review summary

Overall score: 4 / 5 — Good Outside-In design (consumer-driven transport event, shared loader, sensible DI, solid unit tests). One blocking inconsistency between this PR's two commits, plus a handful of small follow-ups. Inline comments posted on the relevant lines.


Findings

🔴 blocking — privacy URLs disagree inside the same panel
After both commits land, the Configuration → Privacy panel will simultaneously show:

  • Footer link: https://www.byterover.dev/services/privacy (new PRIVACY_POLICY_URL)
  • Inside the rendered markdown body: https://byterover.dev/privacy (the placeholder still in src/shared/assets/analytics-disclosure.md:56)

The CLI brv settings set analytics.share true prompt prints the same stale URL. This directly contradicts the PR's stated goal of canonicalising the URL with a single source of truth. I couldn't post an inline comment on the markdown because it isn't in this PR's diff — fix by updating src/shared/assets/analytics-disclosure.md line 56 to match PRIVACY_POLICY_URL, or by interpolating the constant into the markdown server-side in AnalyticsDisclosureHandler. Fix this →

🟡 suggestion — analytics-panel.tsx:111 hardcodes the display label byterover.dev/services/privacy next to href={PRIVACY_POLICY_URL}. Centralising the URL doesn't help much if the label can still drift. Posted inline.

🟡 nitpick — PR description claim doesn't match code. Test plan says "Refocus the tab → disclosure refreshes (TanStack Query default + override on the globalConfig query)", but useGetAnalyticsDisclosure inherits refetchOnWindowFocus: false from the default config and doesn't override it. The disclosure won't refetch on focus. Bundled-asset markdown probably shouldn't refetch, so consider staleTime: Infinity + fixing the description. Posted inline.

🟡 nitpick — AnalyticsDisclosureResponseSchema declared but never safeParse'd. The schema enforces markdown: z.string().min(1), but nothing on the wire actually checks it. If loadDisclosure() ever returns "", the webui silently renders an empty <MarkdownView/> instead of hitting the error-state UI. Posted inline.


What's good

  • Outside-In: the panel drove the new transport event, shared loader, single source of truth (modulo the URL drift above).
  • DI in AnalyticsDisclosureHandler (optional loadDisclosure) keeps the unit test fast and in-memory.
  • Three handler tests + three webui hook tests cover success, error, and "not connected" paths.
  • Wired correctly into feature-handlers.ts:306 next to the other analytics handlers; barrel export updated; obsolete ANALYTICS_DISCLOSURE_SECTIONS / ANALYTICS_PRIVACY_URL fully removed (verified — no stragglers).
  • The webui no longer maintains a parallel copy of legal copy; PM/legal edits to the .md propagate.

  • Read changed files
  • Verify transport event wiring
  • Check handler implementation + tests
  • Review webui consumption (API hook + DisclosureDetails)
  • Validate test coverage
  • Post inline findings + summary
    · branch feat/ENG-2621

Comment thread src/webui/features/analytics/components/analytics-panel.tsx
Comment thread src/webui/features/analytics/api/get-analytics-disclosure.ts
Comment thread src/server/infra/transport/handlers/analytics-disclosure-handler.ts
- Derive the visible privacy link label from PRIVACY_POLICY_URL so the URL
  is the single source of truth — the label can no longer drift on a
  future URL change.
- Mark the disclosure query as staleTime: Infinity. The markdown is a
  bundled asset that never changes at runtime, so refetching on focus is
  pointless; making the cache-forever semantic explicit avoids confusion.
- Guard against an empty disclosure body in AnalyticsDisclosureHandler:
  throw a visible error instead of returning an empty string the webui
  would silently render as a blank panel. Test added for the empty case.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Claude finished @ncnthien's task in 1m 43s —— View job


Claude finished review of 1275ff0

  • Pull latest state and confirm changes in 1275ff0
  • Verify each of the 3 prior non-blocking threads is genuinely addressed
  • Re-check the prior blocking finding (stale URL in analytics-disclosure.md line 56)
  • Skim handler / webui consumption / tests one more time
  • Run typecheck / lint / unit testsnpm install is not permitted in this sandbox; please rely on CI for the green check

Review summary

Overall score: 3 / 5 — The three non-blocking nits from my prior pass are cleanly fixed and well-tested. However, the 🔴 blocking finding from that same review was not addressed, so the PR's stated goal ("single source of truth for the privacy URL") is still not met on main and the two URLs continue to disagree inside the same panel.


Findings

🔴 blocking — same as last round: URL drift inside the rendered panel is still live

src/shared/assets/analytics-disclosure.md:56 still contains the legacy https://byterover.dev/privacy, while src/shared/constants/privacy.ts:1 defines PRIVACY_POLICY_URL = 'https://www.byterover.dev/services/privacy'. After this PR lands, the Configuration → Privacy panel will simultaneously render:

  • Footer link/label (derived from the constant): byterover.dev/services/privacy
  • Inside the markdown body of "What data will be collected?": https://byterover.dev/privacy

The same drift is visible from the CLI consent prompt (brv settings set analytics.share true), because the markdown is shared. Recommended fix (smallest possible): update the URL on line 56 of the markdown asset to match the constant. The stricter "single source of truth" fix is to interpolate PRIVACY_POLICY_URL server-side in AnalyticsDisclosureHandler (e.g. replace a {{PRIVACY_POLICY_URL}} placeholder before returning markdown) so the asset can't drift in the future — but a direct edit unblocks this PR. Fix this →

🟢 resolved — visible label now derives from the constant

src/webui/features/analytics/components/analytics-panel.tsx:111 is now {PRIVACY_POLICY_URL.replace(/^https?:\/\/(www\.)?/, '')} — label and href can no longer drift independently. ✓

🟢 resolved — disclosure query semantics made explicit

src/webui/features/analytics/api/get-analytics-disclosure.ts:21 adds staleTime: Number.POSITIVE_INFINITY. Correct call: the markdown is bundled into the CLI binary, so cache-forever is the right semantic and the prior PR-description claim about refocus-refetch was wrong. ✓

🟢 resolved — empty-body guard added with test

AnalyticsDisclosureHandler now throws 'Analytics disclosure markdown is missing or empty.' when the loader returns ''. New test throws when the loader returns an empty string so the webui surfaces an error state (analytics-disclosure-handler.test.ts:27-36) pins the contract; the existing throw-from-loader test still covers the propagation path. The schema's markdown: z.string().min(1) is now redundant at runtime (the schema is type-inference-only on this path, never safeParse'd), but the explicit guard is the cheaper defence and is fine. ✓

🟡 nit — (www\.)? is www-specific

The new label transform PRIVACY_POLICY_URL.replace(/^https?:\/\/(www\.)?/, '') strips https://, http://, https://www., http://www.. If PRIVACY_POLICY_URL ever moved to e.g. https://docs.byterover.dev/privacy, the rendered label would include docs. — probably what you want, but worth a thought if a future subdomain change is anticipated. Non-blocking; current URL renders correctly as byterover.dev/services/privacy.


What's good

  • Outside-In design from the prior round still holds: consumer-driven transport event, shared loader, DI in the handler, three handler tests + three webui hook tests covering happy / error / not-connected / empty-body.
  • Footer URL and label are now in lock-step via a single constant — exactly the right call.
  • staleTime: Infinity matches the asset's actual lifecycle (bundled, immutable at runtime).

· branch feat/ENG-2621

…on grid layout

Reviewer rejected dumping the full markdown body into the Privacy panel —
the original M1.6 icon-grid layout should be preserved. Move the markdown
-> sections conversion daemon-side so the webui still gets structured data
without owning a copy of the section labels.

- New parseAnalyticsDisclosure() helper splits the markdown on ## H2
  headings into {label, body}[]. Intro paragraphs above the first H2 and
  any H3+ nested headings stay inside their parent section verbatim.
- Updated AnalyticsDisclosureResponseSchema to return sections instead of
  the raw markdown string. Handler throws if the markdown has zero H2s
  (single-source-of-truth invariant still surfaced as an error toast).
- DisclosureDetails reverts to the 2-column icon grid (Database / Eye /
  Server / Link2 / PowerOff mapped by section index), filters out the
  "Privacy policy" section (already shown as the footer link), and uses
  the parsed label + body. Labels render uppercased to match the design.
- Hook + handler tests updated for the new shape; new parser test covers
  multi-paragraph bodies, code blocks, no-H2 input, and nested headings.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Claude finished @ncnthien's task in 4m 6s —— View job


Review summary — synchronize on e4f8b10

Overall score: 3 / 5 — The icon-grid restoration is a reasonable design call and the new daemon-side parser is well-tested in isolation. The previously-resolved nits stay resolved. However, the original 🔴 blocking finding (URL drift between analytics-disclosure.md:56 and PRIVACY_POLICY_URL) is still unaddressed at the source — this commit only hides the symptom in the webui (by filtering the section out by label) while leaving the CLI consent prompt to print the stale URL verbatim. And the new icon-grid rendering introduces a couple of fresh design coupling / UX issues. Four inline comments posted.


Findings

🔴 blocking (still) — URL drift at the source is not fixed; webui filter only masks one of two surfaces

src/shared/assets/analytics-disclosure.md:56 still contains https://byterover.dev/privacy. The webui now sidesteps this by filtering the ## Privacy policy section out of the grid (disclosure-details.tsx:14-16), but the same markdown is loaded verbatim by src/oclif/lib/analytics-disclosure.ts:69-71 and printed unchanged by the CLI consent flow (brv settings set analytics.share true). So a user who runs the CLI prompt still sees https://byterover.dev/privacy, while the webui footer they hit five minutes later shows byterover.dev/services/privacy. The PR's "single source of truth" framing in the description is therefore still not delivered.

Cheapest fix: update line 56 to https://www.byterover.dev/services/privacy. Stronger fix: have AnalyticsDisclosureHandler (and collectConsent's log path) interpolate {{PRIVACY_POLICY_URL}} placeholder in the markdown server-side so the asset can never drift. Fix this →

🟡 suggestion — icon-to-section mapping is positional and brittle

SECTION_ICONS[index] (disclosure-details.tsx:10, 52) couples PM/legal-owned section ordering in the .md file to engineering-owned visual mapping. Reordering or inserting a section silently mis-icons every following row. A label-keyed lookup with a fallback icon is the same code size and survives drift. Posted inline.

🟡 suggestion — privacy-section filter is a fragile string match with no regression test

isVisibleSection compares section.label.trim().toLowerCase() to the literal 'privacy policy' (disclosure-details.tsx:12-16). Any renaming of ## Privacy policy in the .md silently lets the legacy-URL section back into the grid alongside the new footer link. No component test pins the filter. Two safer alternatives in the inline comment. Posted inline.

🟡 suggestion — body rendered as plain text loses bullets + code blocks (UX regression vs 94b1cae)

The .md has a markdown bullet list ("What is collected") and a fenced code block ("How to disable"). Rendering with whitespace-pre-line shows literal - hyphens, backticks, and the three-backtick code fence to users. Previous commit used <MarkdownView> which handled this correctly. Posted inline.

🟡 nit — AnalyticsDisclosureResponseSchema still never safeParse'd

Schema declares body.min(1) and label.min(1) but nothing on the wire validates. Concrete edge case: ## Foo\n\n## Bar produces {label: 'Foo', body: ''} from the parser → renders a card with a label and no body. Posted inline.


What's good

  • New parseAnalyticsDisclosure is small, pure, well-tested (5 cases including multi-paragraph, code blocks, no-H2, trim, H3 nesting).
  • DI in AnalyticsDisclosureHandler is preserved; handler tests now cover the empty-sections throw path explicitly.
  • Webui hook + footer label / staleTime polish from the previous round still in place.
  • Daemon-side parsing keeps the webui from owning a copy of section labels — correct call.

Checklist

  • Diff origin/proj/analytics-system-tool-mode...HEAD
  • Re-check the blocking URL-drift finding (still live at the source — only the webui symptom is masked)
  • Review parse-analytics-disclosure.ts + tests
  • Review rewritten disclosure-details.tsx (icon grid restored, plain-text body)
  • Post 4 inline comments (icon mapping, label-string filter, body rendering, schema unused)
  • Run typecheck / lint / unit tests — sandbox blocks npm install; rely on CI
    · branch feat/ENG-2621

Comment thread src/webui/features/analytics/components/disclosure-details.tsx Outdated
Comment thread src/webui/features/analytics/components/disclosure-details.tsx Outdated
Comment thread src/webui/features/analytics/components/disclosure-details.tsx Outdated
Comment thread src/shared/transport/events/analytics-events.ts
…icon grid

The previous commit rendered each section's body as a plain <p>, so real
markdown tokens (**bold**, inline `code`, fenced code blocks, bullet lists)
leaked into the UI as raw text — see the screenshot in the PR review.

Swap the <p> for the existing MarkdownView (already used by the contexts
feature) with a small wrapper class that keeps the cell text styling
(muted, 0.8125rem, leading-relaxed). Bold / inline code / lists / fenced
code now render correctly; cell heights are still variable but the
content reads cleanly.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Claude finished @ncnthien's task in 2m 43s —— View job


Review summary — synchronize on 8cb7599

Overall score: 3 / 5 — The new commit cleanly resolves the previous round's body-rendering UX regression by swapping the plain <p> for MarkdownView. Bullet lists, inline code, and bold now render correctly, and the daemon-side parser / handler tests still pin the contract. However, the original 🔴 blocking finding (URL drift between analytics-disclosure.md:56 and PRIVACY_POLICY_URL) is still unfixed at the source — the webui only masks it by filtering the section out of the grid, and the CLI consent prompt continues to print the stale URL verbatim. Three of the four prior non-blocking findings from the e4f8b10 round (icon mapping is positional, privacy-section filter is a fragile string match with no component test, schema not safeParse'd on the wire) are also still open. One fresh non-blocking observation about the heavyweight CodeBlock inside the icon-grid cell, posted inline.


Findings

🔴 blocking (round 3) — URL drift at the source is still live; only the webui symptom is masked

src/shared/assets/analytics-disclosure.md:56 still contains https://byterover.dev/privacy, while src/shared/constants/privacy.ts:1 is https://www.byterover.dev/services/privacy. The webui filters the ## Privacy policy section out of the icon grid (disclosure-details.tsx:13-17) so a webui-only user sees only the canonical footer URL — but collectConsent in src/oclif/lib/analytics-disclosure.ts:69-71 does onLog(disclosure) on the full markdown, so brv settings set analytics.share true still prints the stale URL to the user before asking for consent. The PR's stated "single source of truth" goal is therefore still not delivered on the CLI surface. Inline comment posted on the markdown line with both a one-line fix and the stricter placeholder-interpolation follow-up. Fix this →

🟢 resolved — body now renders markdown correctly

disclosure-details.tsx:61-64 swaps the previous plain <p> (with whitespace-pre-line) for <MarkdownView content={section.body} />. Bullet lists in ## What is collected, inline code in device_id etc., and the fenced code block in ## How to disable now render as intended instead of leaking raw markdown tokens. ✓

🟡 issue (new, non-blocking — UX) — CodeBlock inside the icon-grid cell is visually disproportionate

MarkdownView resolves fenced blocks to the full contexts-feature CodeBlock (src/webui/features/context/components/markdown-view.tsx:19-56): bordered card, bg-secondary header bar, "Copy code" button, syntax-highlighter, plus a <span> for the language name which renders empty because the fence in analytics-disclosure.md:46 has no language tag. Two cheap mitigations posted inline (tag the fence with bash and/or pass a constrained components override that drops the heavy chrome). Acceptable to ship as-is but worth a follow-up.

🟡 still open (carried over from e4f8b10 round) — icon-to-section mapping is positional

SECTION_ICONS[index] (disclosure-details.tsx:11, 53) still keys off section ordering in the .md, so PM/legal reordering or inserting a section silently mis-icons every following row. The label-keyed lookup map suggestion from the prior review still applies.

🟡 still open (carried over from e4f8b10 round) — privacy-section filter is a fragile string match, no regression test

isVisibleSection still does section.label.trim().toLowerCase() !== 'privacy policy' (disclosure-details.tsx:13-17). A PM-driven rename to ## Privacy notice / ## Privacy Policy & Terms / ## Privacy silently lets the legacy-URL section back into the grid alongside the canonical footer link — the exact failure mode the URL-canonicalisation work was supposed to prevent. No component test pins this contract. The prior-round suggestions (filter by content equalling PRIVACY_POLICY_URL, or return {label, body, kind} from the daemon-side parser so the partition is explicit on the wire) still apply.

🟡 still open (carried over from e4f8b10 round) — AnalyticsDisclosureResponseSchema never safeParse'd

analytics-events.ts:24-26 declares sections.min(1) + per-section label.min(1) / body.min(1), but neither the handler nor the webui hook validates the wire payload. The handler explicitly checks sections.length === 0, but the per-section body.min(1) invariant is unenforced — a section like ## Foo\n\n## Bar produces {label: 'Foo', body: ''} from the parser → renders an icon-grid card with a label and no body. Cheap fix: drop empty-body sections in parseAnalyticsDisclosure (or assert in the handler).


What's good

  • 8cb7599 is a tight, focused fix for the previous UX regression — exactly the right scope.
  • MarkdownView was the existing primitive; reuse over hand-rolled markdown handling is the right call.
  • Daemon-side parsing + DI in the handler + handler/parser unit tests (handler.test.ts × 4, parser.test.ts × 5) are all preserved.
  • PRIVACY_POLICY_URL label/href synced from 1275ff0 and staleTime: Infinity semantics still in place.

Checklist

  • Diff origin/proj/analytics-system-tool-mode...HEAD, focus on 8cb7599
  • Re-check the original 🔴 blocking finding (URL drift in analytics-disclosure.md:56) — still live at the source
  • Re-check the four prior non-blocking findings from e4f8b10 round (body-rendering: ✅ fixed; icon mapping / filter / schema: ✗ still open)
  • Review the new markdown rendering inside the icon-grid cards
  • Check for fresh issues — flagged the heavyweight CodeBlock UX concern
  • Post summary + inline findings
  • Run typecheck / lint / unit tests — sandbox blocks npm install; rely on CI
    · branch feat/ENG-2621

Comment thread src/webui/features/analytics/components/disclosure-details.tsx
…tring

- The disclosure markdown is a bundled asset that never changes at runtime,
  but the handler re-loaded and re-parsed it on every analytics:getDisclosure
  request. Lazy-cache the parsed sections on first success so subsequent
  requests are a single field read.
- Drop the multi-line WHAT docstring on the handler class per the
  zero-comments codebase convention. Single-source-of-truth context is
  already covered by the load + parse pipeline reading the markdown asset.
- New test pins the caching contract: three back-to-back requests trigger
  exactly one load. Existing error-propagation tests still cover the
  load-failure path (errors don't poison the cache).
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Claude finished @ncnthien's task in 2m 11s —— View job


Review summary — synchronize on ed90d74

Overall score: 3 / 5ed90d74 is a tight, well-scoped perf/cleanup commit: lazy-cache the parsed sections, drop the docstring per the no-comments convention, add a 3-call → 1-load test that pins the contract. The cache placement is correct (set only after a successful parse, so loader errors don't poison it) and matches the bundled-asset lifecycle. However, the original 🔴 blocking finding from the first three review rounds (URL drift between analytics-disclosure.md:56 and PRIVACY_POLICY_URL) is still unaddressed at the source. Three of the four prior non-blocking findings (positional icon mapping, fragile privacy-section filter with no component test, schema-never-safeParse'd) are also still open. One small new observation about cache return-by-reference, posted inline.


Findings

🔴 blocking (round 4) — URL drift at the source is still live; webui filter only masks one of two surfaces

src/shared/assets/analytics-disclosure.md:56 still contains https://byterover.dev/privacy, while src/shared/constants/privacy.ts:1 is https://www.byterover.dev/services/privacy. The webui sidesteps the conflict by filtering out the ## Privacy policy section (disclosure-details.tsx:15-17), so a webui-only user sees only the canonical footer URL. But collectConsent in src/oclif/lib/analytics-disclosure.ts:69-71 does deps.onLog(disclosure) on the full markdown verbatim, so brv settings set analytics.share true still prints the stale URL to the user immediately before the consent prompt. The PR description's "single source of truth" framing therefore still isn't delivered on the CLI surface — and ed90d74's new caching layer means the daemon now serves that stale-URL section to any future consumer too, just from RAM instead of disk.

Cheapest fix: update line 56 to https://www.byterover.dev/services/privacy. Stronger fix: have the disclosure loader (or the handler before caching, since the cache is now persistent for the daemon's lifetime) substitute a {{PRIVACY_POLICY_URL}} placeholder in the markdown so the asset can't drift again. Fix this →

🟢 resolved (this commit) — handler no longer re-reads + re-parses on every request

analytics-disclosure-handler.ts:32-43 lazy-caches cachedSections on first successful parse; subsequent requests are a single field read. Error path is correctly excluded from the cache write (the throw happens before this.cachedSections = sections), so a transient load failure isn't sticky. New test loads and parses the markdown once, then serves the cached sections (analytics-disclosure-handler.test.ts:36-52) pins the 3-calls → 1-load contract. ✓

🟡 nit (new, non-blocking) — cache returns the array by reference

getSections() returns this.cachedSections directly, and the request handler spreads it as {sections: await this.getSections()}. If any future consumer mutates the array in place (push, sort, etc.) the cache becomes corrupted for every subsequent request without observability. The transport then JSON-serialises the response, so over-the-wire consumers are insulated, but an in-process caller (e.g. a future TUI handler that shares the daemon) would not be. Cheap defence: return [...this.cachedSections] (shallow clone — sections are plain {label, body} so deep-clone is overkill). Strictly defensive; ignore if you'd rather pay the cost only on a real regression.

🟡 still open (carried over) — icon-to-section mapping is positional

SECTION_ICONS[index] (disclosure-details.tsx:11, 53) still keys off section ordering in the .md, so PM/legal reordering or inserting a section silently mis-icons every following row. Label-keyed lookup map suggestion from the prior round still applies.

🟡 still open (carried over) — privacy-section filter is a fragile string match, no regression test

isVisibleSection still does section.label.trim().toLowerCase() !== 'privacy policy' (disclosure-details.tsx:13-17). A rename to ## Privacy notice / ## Privacy Policy & Terms / ## Privacy silently lets the legacy-URL section back into the grid alongside the canonical footer link. No component test pins this contract. Prior-round suggestions (filter by content equalling PRIVACY_POLICY_URL, or return {label, body, kind} from the daemon-side parser so the partition is explicit on the wire) still apply.

🟡 still open (carried over) — AnalyticsDisclosureResponseSchema never safeParse'd

analytics-events.ts:19-26 declares sections.min(1) + per-section label.min(1) / body.min(1), but neither the handler nor the webui hook validates the wire payload. The handler explicitly checks sections.length === 0, but the per-section body.min(1) invariant is unenforced — ## Foo\n\n## Bar would produce {label: 'Foo', body: ''} from the parser → renders an icon-grid card with a label and no body. Cheap fix: drop empty-body sections in parseAnalyticsDisclosure (or assert in the handler).


What's good

  • ed90d74 correctly identifies the bundled-asset lifecycle: load+parse-once is the right cost profile.
  • Cache placement after the validation throw means a transient load error doesn't poison the cache.
  • New caching test is minimal but conclusive (expect(calls).to.equal(1) after 3 invocations).
  • Trimmed docstring matches the codebase's "no comments unless WHY is non-obvious" convention.
  • Daemon-side parsing + DI in the handler + handler/parser unit tests (handler: 5, parser: 5) all preserved.
  • PRIVACY_POLICY_URL label/href synced from 1275ff0, staleTime: Infinity, and MarkdownView-rendered bodies from 8cb7599 are all still in place.

Checklist

  • Diff origin/proj/analytics-system-tool-mode...HEAD, focus on ed90d74
  • Re-check the round-1 🔴 blocking finding (URL drift in analytics-disclosure.md:56) — still live at the source
  • Verify the new caching logic in the handler
  • Re-check the carried-over nits (icon mapping ✗ / filter ✗ / schema ✗ / CodeBlock UX still pending follow-up)
  • Post review summary
  • Run typecheck / lint / unit tests — sandbox blocks npm install; rely on CI
    · branch feat/ENG-2621

- Switch icon-grid mapping from positional (SECTION_ICONS[index]) to a
  label-keyed Record with an Info fallback. PM/legal can now reorder or
  insert sections in the markdown without silently shifting every
  following icon to the wrong section.
- Use a substring "privacy" match for the icon-grid filter instead of an
  exact-string compare. Survives renames to "Privacy notice", "Privacy
  Policy & Terms", etc. so the section never accidentally leaks into the
  grid next to the footer link.
- Parser now drops sections with an empty body so the schema's body.min(1)
  matches the runtime contract — a stray "## Foo\n\n## Bar" can no longer
  render a blank labelled card.
- Tag the analytics-disclosure.md fenced code block with `bash` so the
  CodeBlock header is no longer empty in the webui (and the CLI prompt
  gets the language label too).
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 2, 2026

Claude finished @ncnthien's task in 2m 33s —— View job


Review summary — synchronize on 1aa060b

Overall score: 4 / 5 — Solid round of fixes. The previously-blocking URL is now https://byterover.dev/services/privacy in src/shared/assets/analytics-disclosure.md:56, the icon mapping is label-keyed with an Info fallback, the privacy filter uses substring matching, the parser drops empty-body sections (with test coverage), and the fenced code block is tagged bash so the webui CodeBlock header is no longer empty. One small remnant of the round-1 blocking finding remains (the www. discrepancy between the asset and the constant — both URLs resolve, but it's not literally one canonical string), and the previous-round nit about AnalyticsDisclosureResponseSchema never being safeParse'd is now effectively redundant rather than risky. No fresh regressions spotted.


Findings

🟢 resolved — icon mapping is now label-keyed

disclosure-details.tsx:11-21SECTION_ICONS: Record<string, LucideIcon> with iconForLabel(label) returning Info for unknown labels. PM/legal can reorder or insert sections without silently shifting icons. ✓

🟢 resolved — privacy-section filter survives label drift

disclosure-details.tsx:23-25!section.label.trim().toLowerCase().includes('privacy'). Renames to Privacy notice, Privacy policy & terms, Privacy all stay filtered. Edge case: a future PM-intended visible section literally containing the word "privacy" (e.g. Privacy controls) would also be hidden — acceptable trade-off; the inverse failure (legacy URL silently leaking back next to the canonical footer) is far worse. ✓

🟢 resolved — empty-body sections no longer render blank cards

parse-analytics-disclosure.ts:13-17 factors out pushCurrent() which early-returns when body.length === 0. The new test drops sections whose body is empty so the webui never renders a blank card (parse-analytics-disclosure.test.ts:60-65) pins the contract. The schema's per-section body.min(1) invariant now matches runtime without needing a safeParse on the wire. ✓

🟢 resolved — fenced code block has a language tag

analytics-disclosure.md:46 is now ```bash instead of bare ```. The CodeBlock header in the webui icon-grid cell renders bash instead of an empty span, and the CLI prompt also gets the language label. ✓

🟡 suggestion — URL drift between asset and constant is now small, but not zero

src/shared/constants/privacy.ts:1 is https://www.byterover.dev/services/privacy (with www.), while src/shared/assets/analytics-disclosure.md:56 is https://byterover.dev/services/privacy (no www.). The previous review-round drift (/privacy vs /services/privacy) is fixed — both URLs now point at the same path on the same site, and the webui's label transform .replace(/^https?:\/\/(www\.)?/, '') collapses both to the same display string. But the actual click-through URLs the user gets from the CLI consent prompt vs. the webui footer link still differ by www., so "single source of truth" isn't literally true on the wire. Cheap fix: edit line 56 to https://www.byterover.dev/services/privacy. Stronger fix: interpolate a {{PRIVACY_POLICY_URL}} placeholder in the loader / handler so the asset can't drift again. Fix this →

🟡 nit — AnalyticsDisclosureResponseSchema still never safeParse'd, but now harmless

analytics-events.ts:19-26 declares sections.min(1) + per-section label.min(1) / body.min(1). The schema is still only used for type inference, but each invariant is now also enforced by other code: the handler throws when sections.length === 0 (analytics-disclosure-handler.ts:37-39), and the parser skips empty-body sections (parse-analytics-disclosure.ts:16). So the wire payload can no longer violate the schema in practice. Either keep the schema as documentation, drop it in favour of a hand-written type, or add a one-line safeParse in the handler before returning — all defensible. No action required.

🟡 nit (carried over from ed90d74 round, not addressed) — cached sections returned by reference

analytics-disclosure-handler.ts:32-43getSections() returns this.cachedSections directly; an in-process consumer that mutates the array (push/sort/splice) would corrupt the cache silently. The transport JSON-serialises responses, so wire consumers are insulated; only an in-daemon caller could trigger this. return [...this.cachedSections] would defuse it. Strictly defensive; ignore if you'd rather pay the cost only on a real regression.

🟡 nit — no component test for DisclosureDetails

The filter + icon-lookup logic is now in three places (parser, filter, icon map) and only the parser has unit tests. A small render-test feeding a fixture with ## Privacy notice + ## Cross-device alias + an unknown-label section would pin: (1) "privacy"-substring sections are filtered, (2) known labels get their mapped icon, (3) unknown labels get Info. Non-blocking — the implementation is small enough to verify by inspection.


What's good

  • 1aa060b is a tight, well-scoped follow-up that addresses 4 of 5 prior carryover findings in one commit.
  • Label-keyed icon map with Info fallback is the right design — engineering owns the mapping, PM owns the labels, and unmapped labels degrade gracefully.
  • Substring filter inverts the failure-mode asymmetry correctly (over-hiding is safer than under-hiding when the section contains a legacy URL).
  • Empty-body parser guard + test means the runtime invariant matches the schema without paying for safeParse on the wire.
  • Code-fence language tag fixes the webui chrome AND benefits the CLI prompt — nice two-for-one.

Checklist

  • Diff origin/proj/analytics-system-tool-mode..HEAD, focus on 1aa060b
  • Re-check the standing 🔴 blocking finding (URL drift in analytics-disclosure.md:56) — mostly fixed, small www. remnant remains as 🟡
  • Verify each prior carryover nit is addressed (icon mapping ✅ / filter ✅ / empty body ✅ / fence ✅ / schema-unused now harmless)
  • Check for fresh regressions — none spotted
  • Post review summary
  • Run typecheck / lint / unit tests — sandbox blocks npm install; rely on CI
    · branch feat/ENG-2621

@ncnthien ncnthien merged commit a146b79 into proj/analytics-system-tool-mode Jun 2, 2026
7 checks passed
@github-actions github-actions Bot mentioned this pull request Jun 2, 2026
26 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants