Repository navigation
chore(browser): type-check browser tests in CI and fix the errors - #5430
Open
pauldambra wants to merge 2 commits into
Open
pauldambra wants to merge 2 commits into
pauldambra wants to merge 2 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 checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
Contributor
Coherence4 spec problems · 0 failing chokepoints · 2 spec gaps · 0 guard failures 44 components, 7 bullets (5 invariants, 2 requirements), 4 problems Spec problems (spec --check)
Chokepoints: none declared. Spec gaps: 2 entrances with outside or unknown trust and no traced control, and 10 more held by the adoption baseline
Close one with |
Contributor
Contributor
|
Size Change: +6.89 kB (+0.03%) Total Size: 25.2 MB 📦 View Changed
ℹ️ 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-10-25 16:50:20 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
This branch has not been deployed
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