Repository navigation
chore(browser): type-check browser tests in CI and fix the errors - #5430
Closed
pauldambra wants to merge 3 commits into
Closed
pauldambra wants to merge 3 commits into
pauldambra wants to merge 3 commits into
Conversation
The browser package has a test:typecheck script, but CI never ran it, so 13 type errors built up in tests and in two source files that the tests compile with strict: false. check-types for posthog-js now runs typecheck and test:typecheck, the same way the node package checks its test types. The two source changes behave the same as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 6ab57ee6-6279-4378-b074-ed4f92849780
Contributor
|
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
Contributor
Replay incident risk checkThe current diff no longer matches any past incident pattern. |
Contributor
Coherence📄 0 spec file errors · ⛔ 0 broken rules · 🚪 2 unchecked inputs · 🐛 0 bugs that came back What is Coherence?Coherence compares the code with its spec files ( This check read 44 parts of the code and 7 rules. Note These findings do not make this check fail. None of these findings are in files that this pull request changes. You do not need to fix them in this pull request. 🚪 Unchecked inputs (2)
What this means and how to fix itInput from outside this code comes in at these places. The spec does not say what checks that input. 10 more places were already like this when Coherence was added. This list does not show them. To close one of these, find the place by its name in the
To get the line to add, run |
Contributor
Contributor
|
Size Change: 0 B Total Size: 26.4 MB ℹ️ View Unchanged
|
Contributor
Node SDK compliance v2Commit: 3377df6 Capture v0 — acceptance✅ 28 passed / ❌ 3 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: Harness exit: 1; report validation exit: 1. Capture v1 — acceptance✅ 28 passed / ❌ 3 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: Harness exit: 1; report validation exit: 1. Capture v0 — migration✅ 55 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. Capture v1 — migration✅ 120 passed / ❌ 1 non-passing / Drill-down: run artifacts → ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. |
Contributor
posthog-js Compliance ReportDate: 2026-11-05 12:42:53 UTC ✅ All Tests Passed!26/26 tests passed Capture Tests✅ 26/26 tests passed View Details
|
pauldambra
marked this pull request as ready for review
October 25, 2026 16:35
pauldambra
requested review from
TueHaulund,
fasyy612 and
ksvat
and removed request for
a team
October 25, 2026 16:35
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated lenses (qa-team, paul-reviewer, xp-reviewer, security-audit, engineering-systems-thinking as warranted) Verdict: ✅ APPROVE (round 1 @ 454ef61)The changes fix types only. The two source changes keep the same behavior. Key findingsNone.
ConvergenceNone (only the router ran). Reviewer summaries
Automated by QA Swarm — not a human review |
site-apps test now builds the browser PostHogFeatureFlags from the instance, like remote-config does, so it needs no cast. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 6ab57ee6-6279-4378-b074-ed4f92849780
TueHaulund
approved these changes
Oct 26, 2026
Contributor
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
# Conflicts: # packages/browser/src/__tests__/deferred-init-extensions.test.ts # packages/browser/src/__tests__/extensions/surveys.test.ts # packages/browser/src/__tests__/site-apps.test.ts
Contributor
SDK performance: posthog-nodeBenchmarks completed. No changes flagged for review. Flags require at least a 5% change in the median and all six pairs agreeing on direction. This is a noise filter, not proof of a performance change or a CI gate. View all results
Values are medians of paired measurements. Not flagged does not mean unchanged. Unavailable means a measurement could not be provided, not zero. Estimated allocation measurements retain their instrumentation uncertainty. RSS measures the whole process, not just SDK allocations; observed peaks are sampled checkpoints. Paired ranges in the diagnostics describe variability, not confidence intervals. Each shard uses its own runner; compare Before PR against With PR within a row, not across shards. Compared 102 metric rows; 102 rows shown in all results. Omitted or shortened data remain in |
Member
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
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.
Problem
packages/browserhas atest:typecheckscript, but CI never runs it. So 13 type errors built up without anyone seeing them: in tests, and in two source files that the test config compiles withstrict: false.Why: found while working on #5429. We want CI to catch these errors, not let them pile up.
Changes
check-typesforposthog-jsnow runstypecheck && test:typecheck. CI already runspnpm check-types, so it now covers test types too. Thenodepackage already does the same.falsethat is not a validcookieless_mode, an unused@ts-expect-error, and abeforeEachthat returned a value.replay/external/config.ts: returnsundefinedinstead of the nullishcleanedURL. The caller checks withisNullish.browser-next/src/lane.ts:result ? result.retry : undefinedinstead ofresult?.retry.I did not turn on
strictfor tests. That would add more than 1,400 errors.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written with Claude Code in PostHog Desktop. No changeset, because nothing a user can see changes. Checked locally:
pnpm turbo run check-types --filter=posthog-jsand--filter=@posthog/browserpass. The unit tests in the changed files pass, and so do all thebrowser-nexttests.Created with PostHog Desktop
🤖 Generated with Claude Code