Repository navigation
feat(telemetry): report safe app setting values - #866
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to No actionable issue remains from the reviewed changes. Complete the documented database migration before deploying the Worker. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 18 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely identifies the main change: reporting safe application setting values in telemetry. Description check The description includes the required Change and Validation sections. It explains the behavior, exclusions, migration order, compatibility conditions, test coverage, passed checks, and known unrun tes… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 18 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
…unknown event fields Review follow-ups on #866 (Codex astra medium, 2026-10-12), with the two settings #811 added meanwhile: - The browser posted its settings snapshot to the local server as soon as the app was unlocked, before the version-3 notice and whatever the switch or the environment said. Both producers (the notice line and Settings → About) now read the server's status first and tell the snapshot only while telemetry is on, not blocked and the notice seen; the first snapshot rides with the acknowledgement, and turning the switch on brings the snapshot the browser kept to itself. Off or blocked, About still shows the switch and the preview from the status alone. - The receiver refuses an event body with a field no sender has ever had, so a misspelled or foreign field is a 400 instead of silently dropped; the `changed_settings` list of senders before 0.4.9 is still accepted and not stored. - `pageOverlapRows` (0-100) and `chatPageKeys` (#811) join the schema and the projection: 45 values and 7 flags/counts. The strict receiver means the Worker must be deployed before the app release that adds a key, or events are refused and retried unreported. - `voiceLanguage` stays in the allowlist: an explicit preference from a fixed list, noted in the review as the one value naming a region beyond the UI language. - telemetry regression: the first snapshot is the acknowledgement's, and an opted-out page sends none on reload; receiver tests for the refused field and the legacy sender. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Owner
Author
|
Merge-session review (Codex astra medium, 2026-10-12); follow-ups pushed as 358bd7a on top of a merge of current main (6b108cd, no conflicts):
Verified locally: typecheck, build, unit tests, the telemetry contract tests against a live herdr, the telemetry browser regression with a fresh build. The earlier CI failures on 9945230 were the Docker rate limit in the ssh job and the prompt-dock worker-load flake; both passed on rerun. |
devswha
added a commit
to demrom/herdr-web-ui
that referenced
this pull request
Oct 12, 2026
…pshot devswha#866's schema asks for a collection decision on every new Settings key: attachLimit is a bounded integer (1-50), so it is reported like the other numbers. 46 values and 7 flags/counts now. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Oct 12, 2026
devswha
added a commit
that referenced
this pull request
Oct 13, 2026
…try key (#948) ## Change Two release-time mistakes that keep coming back now fail CI instead of relying on memory: forgetting to raise the remote bundle version, and releasing before the telemetry Worker has been deployed. Both checks live in `scripts/release-guards.ts`, which runs in CI's **Fast checks** (after `check fast`, once a shallow fetch has pulled in the tags) and in the **Release** preflight (`--release X.Y.Z`). A tree counts as a release when its `package.json` version has no `v<version>` tag yet: a release PR, `main` between that PR's merge and the Release run, and the Release run itself. Every other tree passes at once, so feature PRs are not affected. ### Remote bundle version - The inputs are what `build-remote-bundle.ts` packs: `server/` and `shared/` (without `*.test.ts`, `*.md` and `server/fixtures/`), `bun.lock`, `patches/`, the builder itself, and the dependency fields of `package.json`. `dist/` is left out: the connection server serves the page, and the bridge never does. - Baseline: the `remote-vN` tag for the `REMOTE_BUNDLE_VERSION` in the tree. If that tag exists and the inputs differ from it, the check fails and says: raise N to N+1 in `shared/machines.ts`, then push `remote-v(N+1)` after the merge and before Release. If the tag does not exist yet (N was raised in this PR), the PR passes. The Release run fails until `remote-vN` is published from a tree with these same inputs. It also fails when N is lower than an existing `remote-vM`. - Tested on past releases: v0.4.5 to v0.4.8 pass. **v0.4.4 would have failed**: it shipped `server/index.ts`, `server/prompt.ts`, `server/conversation.ts`, `bun.lock` and others with no new bundle (remote-v23 was cut at v0.4.3). ### Telemetry Worker - Parity between client and Worker already holds at compile time: the Worker validates with `shared/telemetry.ts`, and `telemetrySettingsSnapshot` must cover every Settings field. A new unit test in `src/lib/settings.test.ts` adds a clear failure message on top: it lists each key the app sends that the Worker refuses, and the reverse. The message says to add the key to `TelemetrySettings` and `TELEMETRY_SETTINGS_VALIDATORS` and to the worker.test.ts fixture, then deploy the Worker before releasing. `TELEMETRY_SETTINGS_KEYS` is exported for this test. - What the code alone cannot show is whether the Worker was deployed. `telemetry/deployed.json` stores the sha256 of `telemetry/worker.ts` + `shared/telemetry.ts` as last deployed. A release whose sources differ fails and says to deploy (telemetry/README.md), run `bun scripts/release-guards.ts --worker-deployed` and commit the file. **Starting value:** the v0.4.8 sources. #866 and #863 changed the schema after that, so the next release PR will fail this check until the Worker is deployed and the file is updated. That deploy is already planned before the next release, so the failure is intended. ### Release metadata - `server/release-notes.test.ts` already requires patch notes in every language, but only for versions that have a CHANGELOG heading. `release.yml` checked the version itself only after merge. The guard now runs the same `releaseNotes()` check on the release PR, so a version bump without a CHANGELOG section, without a full `release-summaries.json` entry, or with a mismatched `herdr-plugin.toml` fails before merge. Docs: one paragraph in docs/development.md "Releasing" (the language list there now includes `tr` as well), one line in telemetry/README.md Deploy, one pointer in scripts/AGENTS.md. No CHANGELOG entry, since users see no change. ## Validation - `bun run check fast`: workflow syntax, generated types, typecheck, build, unit (3031 pass, 0 fail). - `scripts/release-guards.test.ts` builds throwaway git repos and checks each case: a tagged tree is left alone; a release PR that changed only the client passes; server, `bun.lock` and dependency changes under an already-published bundle fail; a raised bundle passes the PR but fails Release until `remote-vN` exists; a changed Worker without a recorded deploy fails; missing patch notes fail. - On this branch: `bun scripts/release-guards.ts` reports "Not a release" and exits 0. `--release 0.4.8` fails with both expected messages: 22 bundle inputs changed since remote-v27, and the Worker was changed since its deploy. - Simulated the CI step on a `--depth 1` clone: the tag fetch takes about 3 s and fetches 92 tags. With `package.json` bumped to 0.4.9, all three problems are reported. - Mutation check: adding a key to the snapshot that the Worker does not accept fails the new settings test with the "add it ... deploy the Worker before releasing" message. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
Change
Expand install/update telemetry from changed appearance-setting names to actual, allowlisted app setting values. Branched from current
main(55cfcfbf).Plan implemented
Raw custom fonts, shortcut/key-bar contents, quick-reply text and account identifiers/order are excluded. Custom settings contribute only flags/counts. Future Settings fields require an explicit collection decision; unknown or malformed telemetry fields are rejected.
NOTICE_VERSIONis now 3. Acknowledgement requires that exact version, so an old open page cannot acknowledge the expanded notice. Existing opt-outs, environment blocks, the ten-minute grace period and install/update-only delivery remain unchanged. Settings changes do not create additional events once the version has been counted. The snapshot is the last reporting browser's settings, not per-device/person tracking.The demo intentionally retains its existing telemetry 404 and sends nothing.
Receiver deployment order
Before releasing the app: add the
settingscolumn to the existing D1 table using the command intelemetry/README.md, then deploy the Worker. Historical rows are preserved. This PR does not deploy the Worker or access production telemetry data.Validation
bun run check fast— passed: workflow syntax, generated types, TypeScript, production build; 2,919 unit tests passed, 8 skipped, 0 failures across 218 files.bun run check run bun test ./server/api.contract.test.ts -t 'telemetry API'— 3 passed, 28 assertions. Covers full snapshots, malformed/private-field rejection, version-bound notice acknowledgement, origin/header/auth protection and opt-out.bun run check run --build bun scripts/telemetry-regression.ts— passed against an isolated herdr/server with a fake external transport. Verifies the updated notice, safe server-confirmed preview, non-default-to-non-default setting changes and Turn off preventing pending delivery. Registered in the existing UI regression suite.git diff --check— passed.Receiver tests exercise SQLite persistence/deduplication, historical table upgrade, README distribution queries, bounded payloads and rejection of unsafe snapshots. No test sends to the production telemetry endpoint.
Full integration/browser lanes and native Windows CI were not run locally. Vite retains its existing large-chunk warning; no warnings were suppressed.