Feat/eng 3007#734
Merged
Merged
Conversation
…cs.enabled Surfaces the analytics opt-in via a new `analytics.enabled` boolean descriptor. SettingsHandler acts as a facade: GET/SET/RESET/LIST for the key route through GlobalConfigHandler instead of FileSettingsStore, so the canonical storage in config.json, the device-id seeding race fix, the analytics cache, and the abort-on-disable side-effect all stay unchanged. The descriptor carries a new `storage: 'global-config'` flag; the file store refuses to persist such keys and the validator pushes any hand-edited override into the invalid bucket. The disclosure prompt moves into `brv settings set analytics.enabled true` with a `--yes` flag for CI / non-interactive runs (CLIError + non-zero exit when no TTY and no --yes). The TUI settings page gains a confirm-disclosure mode that renders the markdown inline on the false-to-true toggle and waits for Enter / Esc. Disclosure markdown moved from `src/server/templates/sections/` to `src/shared/assets/` so the TUI can read it without crossing the `tui` -> `server` import boundary. Both surfaces consume it via the new canonical loader at `src/shared/utils/load-analytics-disclosure.ts`. The legacy `brv analytics enable` command delegates to the extracted disclosure helper at `src/oclif/lib/analytics-disclosure.ts` so the command file can be safely deleted in a follow-up.
The disclosure overlay rendered the full markdown as a single Text block, which let wrapped long lines push the sticky `Enter / Esc` footer off the visible area in short terminal windows. Now the body lives in a sized viewport: each markdown line is its own `wrap="truncate-end"` Text so source-to-visual row mapping is 1:1, the body height is clamped to `terminalRows - 12` (defensive for the REPL's own bottom bar), and overflow shows as yellow `↑ more above` / `↓ more below` indicators. ↑/↓ scroll by line; PgUp/PgDn (and `b` / `Space`) scroll by page.
The `brv analytics enable | disable | status` commands are folded into
the unified `brv settings` surface (`analytics.enabled` boolean facade
and `analytics.status` readonly-info snapshot, shipped in earlier
milestone tickets). The legacy command files and their test directory
are removed; `brv analytics <anything>` now exits with oclif's
"Command not found" message.
Lib re-exports survive the deletion so the M16.2 + M16.3 work keeps
working:
- `src/oclif/lib/analytics-disclosure.ts` (already extracted earlier)
- `src/oclif/lib/analytics-status-formatter.ts` (NEW thin re-export
from the canonical home at `src/shared/utils/format-analytics-status.ts`,
so the AC's "importable from src/oclif/lib/" surface is preserved
while the TUI keeps consuming the same renderer via shared/).
Two replacement smoke tests confirm the oclif command path resolves
both analytics keys to the `settings:get` transport event:
- test/commands/settings/analytics-enabled.test.ts
- test/commands/settings/analytics-status.test.ts
Heavy lifting stays in the unit test suites that already cover the
facade routing, disclosure flow, and formatter parity.
The disclosure markdown content contract (5 required sections + privacy
URL) is rescued from the deleted enable.test.ts into
test/unit/shared/assets/analytics-disclosure-content.test.ts so a
future copy edit cannot silently break the section headers.
Contributor
|
Claude finished @bao-byterover's task in 3m 55s —— View job PR Review — Feat/ENG-3007: delete
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Disclosure markdown: brv analytics disable -> brv settings set analytics.enabled false - Tighten disclosure-content regex to require the new command form - settings set: warn when --yes is passed for non-analytics keys - settings set: JSON mode refuses interactive consent, returns requires_consent envelope - analytics-disclosure: translate inquirer ExitPromptError to declined consent - settings-page TUI: import ANALYTICS_ENABLED_KEY from shared/constants - settings-page TUI: surface transport rejection on disclosure-confirm toggle - settings-handler: facade-missing branches use code: 'misconfigured' - settings-handler: explicit guard for non-boolean global-config RESET - shared/transport: widen SettingsErrorDTO.code union with 'misconfigured' - file-settings-store: drop trailing blank line - rewrite historical doc-comment refs from brv analytics to brv settings
Contributor
|
Claude finished @bao-byterover's task in 2m 10s —— View job PR Review — Feat/ENG-3007 (re-review after
|
| # | Prior comment | Status |
|---|---|---|
| 1 | Blocking — disclosure.md still pointing at deleted brv analytics disable |
✅ src/shared/assets/analytics-disclosure.md:31 now says brv settings set analytics.enabled false; content-contract test (test/unit/shared/assets/analytics-disclosure-content.test.ts:29-33) now requires the new form AND explicitly forbids the old one |
| 2 | --yes flag is a no-op for non-analytics keys |
✅ set.ts:69-75 now emits this.warn(...) when -y is passed for any other key; test at test/commands/settings/set.test.ts:417-433 |
| 3 | --format json + interactive consent collides |
✅ set.ts:139-167 refuses to prompt in JSON mode, emits a structured requires_consent error envelope (or fast-paths through silently when --yes is also set); both branches tested |
| 4 | Hardcoded ANALYTICS_ENABLED_KEY in TUI |
✅ Lifted into src/shared/constants/settings-keys.ts and re-imported in the TUI page — single source of truth without crossing the tui/ -> server/ boundary |
| 5 | performToggle(...).catch(() => {}) swallows mutation rejections |
✅ settings-page.tsx:296-299 now surfaces the error via setRowError and restores browse mode |
| 6 | Boolean-only RESET branch needs explicit guard | ✅ settings-handler.ts:319-328 now explicitly refuses non-boolean global-config descriptors with code: 'misconfigured'. (See one inline observation below — the symmetric SET branch was not updated.) |
| 7 | Missing-facade error using 'invalid_value' collides with type errors |
✅ Both SET (line 211) and RESET (line 307) now use 'misconfigured'; the new code is added to the SettingsErrorDTO.code union with an inline doc comment explaining the difference |
| 8 | No try/catch around inquirer prompt | ✅ analytics-disclosure.ts:84-96 catches ExitPromptError by name (not instanceof, to avoid nested-node_modules instance mismatch) and treats Ctrl-C as declined consent; non-ExitPromptError failures still propagate; both paths tested |
| 9 | Disclosure regex accepts old form | ✅ Tightened to require ONLY the new form, plus an explicit not.match(/brv analytics disable/i) negative assertion |
Plus: doc-comment refs in src/server/infra/analytics/*, feature-handlers.ts, analytics-events.ts, format-analytics-status.ts, and analytics-disabled.ts are now scrubbed of brv analytics enable|disable|status — the user-facing strings now point at the surviving brv settings ... surface, as the previous review's misc-nits requested.
Inline (one observation, non-blocking)
src/server/infra/transport/handlers/settings-handler.ts:208-220— the boolean-only guard added to the RESET branch (lines 319-328) was not mirrored into the SET branch. Today both are exercised only byanalytics.enabledso behaviour is identical, but a future PR adding an integerstorage: 'global-config'descriptor would passcheckValueType, hitdata.value as boolean, and silently hand an integer to the boolean-only facade. Symmetric fix is one extraif (descriptor.type !== 'boolean')block — see inline comment for the suggested location.
Praise
- The
'misconfigured'code shipped as a wire-schema change with an inline doc-comment explaining the difference vsinvalid_valueandinvalid_value_type. That's the right level of intent capture — the next consumer (WebUI alert routing, log dashboard) doesn't have to reverse-engineer it. - The
ExitPromptErrordetection bynameinstead ofinstanceofis the correct call. The accompanying test comment explains the nested-@inquirer/coreresolution gotcha, which is exactly the kind of why that earns its keep. - The TUI
.catchfix preserves the existingsetRowErrorUX for non-okresponses but adds the previously-missing path for thrown rejections — minimal change, no UI rewrites. - The JSON-mode consent envelope (
code: 'requires_consent') is a real structured error, not a stringly-typed message. Scripts can branch on it.
· branchfeat/ENG-3007
Contributor
|
Claude finished @bao-byterover's task in 1m 53s —— View job PR Review — Feat/ENG-3007 (re-review after
|
bao-byterover
merged commit c6f7ccd
into
proj/analytics-system-tool-mode
7 checks passed
26 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.