SDK audit: fix 85 bug/footprint/DX findings across react, next, and browser - #161
Open
Rieranthony wants to merge 2 commits into
Open
SDK audit: fix 85 bug/footprint/DX findings across react, next, and browser#161Rieranthony wants to merge 2 commits into
Rieranthony wants to merge 2 commits into
Conversation
Full multi-agent audit of @cossistant/react, @cossistant/next, and @cossistant/browser targeting bugs, bundle footprint, and DX. The audit record (114 findings with evidence, verification verdicts, and the fix map) lives in audit/. Bugs: - Visitor identification never ran: IdentifySupportVisitor latched before the visitor loaded; now waits, retries, and re-identifies on payload changes - Failed sends lost the visitor's message; text/attachments/draft are restored and the error is surfaced - Provider re-pushed defaultOpen/size defaults on every effect re-run, force-closing the widget and defeating persisted open state; the controller also rebuilt on every render for inline support objects - Data hooks double-fetched on mount, leaked pagination cursors across conversation switches, and let fetch failures escape as unhandled rejections; missed events now resync after WebSocket reconnect - RealtimeProvider died permanently under StrictMode; reconnect backoff leaked duplicate sockets - Feedback failures were silent; onClick on triggers replaced internal toggles; Escape wiped typed feedback - asChild slot clobbered child handlers/styles, Enter submitted mid-IME composition, focus trap hijacked Tab document-wide, plus dialog/ radiogroup a11y and scroll anchoring fixes - SSR: localStorage reads moved out of render, hydration-stable timestamps and default messages Footprint: - zod v4 + @hono/zod-openapi excised from all consumer bundles by moving runtime helpers to zod-free @cossistant/types/support-onboarding (widget.js: 191KB -> 128.6KB gzip) - @floating-ui/react replaced with @floating-ui/react-dom; ulid removed - Library dist ships unminified with sourcemaps for consumer treeshaking DX: - pub:* scripts published a broken tarball (npm ignores publishConfig.directory); all packages now publish ./dist - CDN embed: documented snippet threw TypeError; new one-tag install via data-public-key, throw-resilient queue replay, React moved to peers - @cossistant/next now mirrors all ~41 react subpaths (was 12) and its d.ts no longer breaks skipLibCheck:false - "use client" on every entry, prop types exported, Button collision resolved via SupportButton, docs snippets verified to compile, new CDN embed docs page 301 react / 137 core / 32 types / 18 browser / 3 next tests pass (+65 new regression tests), tsc and biome clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Too many files changed for review. ( Bypass the limit by tagging |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 176f9fa6fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+87
to
+91
| merged[key] = (...args: unknown[]) => { | ||
| childValue(...args); | ||
| const event = args[0] as { defaultPrevented?: boolean } | undefined; | ||
| if (!event?.defaultPrevented) { | ||
| slotValue(...args); |
There was a problem hiding this comment.
When asChild is used with a child that already wires the render-prop toggle to onClick, this new composition calls the child's handler and then the slot handler. The default support launcher does exactly that (DefaultTrigger renders <Primitive.Trigger asChild> and its child button has onClick={toggle}), so one click toggles open and immediately toggles closed, making the default widget trigger appear broken. The slot merge needs to avoid double-invoking the same internal action, or the default/custom render-prop triggers need to stop attaching toggle when the primitive already injects it.
Useful? React with 👍 / 👎.
Comment on lines
+125
to
+135
| return realtime.onStateChange((state) => { | ||
| if (lastStatus !== "connected" && state.status === "connected") { | ||
| onConnectRef.current?.(); | ||
| } | ||
|
|
||
| if (lastStatus === "connected" && state.status !== "connected") { | ||
| onDisconnectRef.current?.(); | ||
| } | ||
|
|
||
| if (state.error && state.error !== lastError) { | ||
| onErrorRef.current?.(state.error); |
There was a problem hiding this comment.
In the normal SupportProvider path, these props are already passed into createSupportController, whose realtime state listener calls runtimeOptions.onWsConnect/onWsDisconnect/onWsError; the provider also passes the same callbacks into this WebSocketProvider. With this added listener, every connect/disconnect/error transition now fires user callbacks twice, which can duplicate analytics, toasts, or app-side side effects whenever the support socket changes state.
Useful? React with 👍 / 👎.
Follow-ups found while auditing #161: - packages/{core,types,tiny-markdown}: pub:* used bare `npm publish`, which ignores publishConfig.directory. Combined with `files: ["dist"]` the tarball root package.json still pointed main/exports at ./src/*.ts, which is excluded from the tarball — every entry point resolved to a missing file. These three are runtime dependencies of the published @cossistant/react, so the tarball fix applied to react/next/browser was incomplete without them. (`changeset publish`, the CI release path, was unaffected: it honours publishConfig.directory.) - The 65 new regression tests never ran under `turbo run test`: the five SDK packages had no `test` script, so the task was skipped entirely. Add `bun test src` to react, core, types, next and browser. - apps/web changelog v0.2.0 still documented the async-loader + immediate `window.Cossistant.init()` snippet, which throws "Cannot read properties of undefined (reading 'init')" — the exact bug #161 fixed in the README and docs page. Switch it to the one-tag data-public-key install. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rieranthony
added a commit
that referenced
this pull request
Jul 30, 2026
fake-support-context rebuilt its state object on every getState/getSnapshot call. useSyncExternalStore requires getSnapshot to return a stable reference between store changes — a fresh object each call makes React see a new snapshot on every render and bail out with "Maximum update depth exceeded". Its subscribe() was also a no-op, so store changes never reached React. Harmless today because nothing subscribes to the controller through useSyncExternalStore. #161 adds useStoreSelector(controller, ...) inside useSupport(), at which point the timeline UI test page infinite-loops — in the browser as well as in tests. The real controller in @cossistant/core already does this correctly: it holds `snapshot` in a closure and rebuilds it only in syncSnapshot(). The fake now mirrors that and forwards store changes to its subscribers, so it satisfies the contract in both cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rieranthony
added a commit
that referenced
this pull request
Aug 3, 2026
* fix(tests): make apps/web and apps/api suites green `bun run test` failed with 149 failures in apps/web and 240 in apps/api. Two independent causes, no production code involved. Cause 1 — cross-file mock leakage (the large majority). bun registers mock.module() factories globally for the whole test process and never unwinds them. Almost every component/router test mocks a module partially (say @tanstack/react-query with only useMutation), so in a combined run the first partial factory wins and every later file importing a missing export dies with "SyntaxError: Export named 'useX' not found in module ...". Run alone, 157/170 web files and 123/129 api files already passed. Each app now runs one `bun test` process per file (scripts/test-isolated.ts), which is the same remedy CI already applies to apps/api's OpenAPI contract test, generalized. Parallel, so the web suite takes ~10s and api ~21s. Cause 2 — 19 genuinely broken files, fixed individually: - Tests that never mocked tRPC/react-query/website context and only passed when a sibling file's mock happened to leak in (precision-flow-section, promo-precision-flow-scene, fake-conversation, page-tree-node, timeline-ui-test-page, use-faq-mutations, use-file-mutations). - Assertions left behind by deliberate source changes: background fps/pointer trail defaults (changed in "feat: bolder ascii"), the tools catalog order and section counts, and the visitor-source-badge favicon, which asserted 12 while the source has always rendered 16 intrinsic pixels at a 12px display size — both landed in the same commit, so it never passed. Where the source exports a constant the test now reads it instead of hardcoding, so tuning a value cannot silently break the test again. - docs-widget-release hardcoded /Users/anthonyriera/code/cossistant-monorepo, so it only passed on one machine and would fail in CI. It now derives the repo root from import.meta.dir. - seo-routes and sitemap assumed a localhost origin; getSiteUrl() only falls back to localhost when NODE_ENV is "development", which bun test does not set. They now pin NEXT_PUBLIC_APP_URL. - Tests whose fake db predated a query extraction (plan.self-hosted, website, contact-control, load-context) now serve the same fixtures through the extracted queries. Result: 15/15 turbo test tasks pass — web 692, api 703, protocol 18, memory 33, facehash 24, release 8. check-types 20/20 and Biome clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(tests): run the orphaned workers, jobs and tiny-markdown suites apps/workers (7 files), packages/jobs (8) and packages/tiny-markdown (1) each had test files but no `test` script, so `turbo run test` skipped them entirely and 16 files had never run in CI. Wiring them up surfaced two real failures: - apps/workers/src/queues/index.test.ts mocked five worker factories but not ./lifecycle-email/worker, which was added later. Unmocked it builds a real BullMQ worker and opens a Redis connection, so the test could only pass against a live Redis — it failed with ECONNREFUSED everywhere else. It also mocked @cossistant/redis partially, dropping createRedisConnection. - packages/jobs ai-agent-background asserted a 60s default delay; AI_AGENT_BACKGROUND_DELAY_MS is 30s. The test now reads the exported constant instead of hardcoding it. Full suite: 18/18 turbo test tasks pass — web 692, api 703, jobs 52, workers 33, memory 33, facehash 24, protocol 18, release 8, tiny-markdown 7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(test-ui): cache the fake support controller snapshot fake-support-context rebuilt its state object on every getState/getSnapshot call. useSyncExternalStore requires getSnapshot to return a stable reference between store changes — a fresh object each call makes React see a new snapshot on every render and bail out with "Maximum update depth exceeded". Its subscribe() was also a no-op, so store changes never reached React. Harmless today because nothing subscribes to the controller through useSyncExternalStore. #161 adds useStoreSelector(controller, ...) inside useSupport(), at which point the timeline UI test page infinite-loops — in the browser as well as in tests. The real controller in @cossistant/core already does this correctly: it holds `snapshot` in a closure and rebuilds it only in syncSnapshot(). The fake now mirrors that and forwards store changes to its subscribers, so it satisfies the contract in both cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Rieranthony
added a commit
that referenced
this pull request
Aug 3, 2026
…163) Two release-hygiene gaps found while auditing #161. CI never ran the SDK regression suites. @cossistant/{react,core,types,next, browser} had no `test` script, so `turbo run test` skipped them and the suites only ever ran locally. #161 adds the `test` scripts; this adds the CI step that consumes them, so the provider-lifecycle, data-hook and embed regressions gate merges instead of relying on someone running `bun test` by hand. The step is a no-op until #161 lands (no `test` task to run, so turbo executes nothing and passes), and becomes meaningful the moment it does — 473 tests across the five packages. @cossistant/protocol's pub:* scripts used bare `npm publish`, which ignores publishConfig.directory. With `files: ["dist"]` the tarball root package.json still pointed main/exports at ./src/*.ts, which is excluded from the tarball, so all 12 entry points resolved to missing files. #161 fixed this for react, next and browser; #161's follow-up commit fixed core, types and tiny-markdown. protocol is the last one. `changeset publish` (the CI release path) was never affected: it honours publishConfig.directory. Co-authored-by: Claude Opus 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.
What
Full audit of
@cossistant/react,@cossistant/next, and@cossistant/browser(12 audit dimensions, every finding adversarially verified against the code), followed by fixes for 85 of the 114 findings. The complete audit record — findings with file:line evidence, verification verdicts, fix ownership map, and remaining follow-ups — is committed underaudit/.Why
Goal: zero bugs, tiny footprint, perfect DX for the SDK surface (headless
<Support />, feedback module, Next.js port,<script>CDN embed).Highlights
Bugs (all with new regression tests — 65 added, 301 total in react)
IdentifySupportVisitorlatched before the visitor loaded)defaultOpenre-pushed throughupdateOptions); persisted open state now works; controller no longer rebuilds per render; StrictMode/Activity remounts reviveRealtimeProviderpermanently dead under StrictMode; reconnect backoff leaked duplicate socketsonClickreplacing internal toggles, Escape wiping typed feedbackasChildclobbering child handlers/styles, Enter submitting mid-IME composition (CJK), focus trap hijacking Tab document-wide, dialog/radiogroup a11y, prepend scroll anchoringFootprint
widget.js: 191KB → 128.6KB gzip (−33%) — zod v4 +@hono/zod-openapiwere bundled because two runtime helpers lived in a zod schema module; they moved to zod-free@cossistant/types/support-onboarding@floating-ui/react(169KB ESM) →@floating-ui/react-dom(positioning only); deaduliddep removedDX / publish
pub:*scripts would have shipped a broken tarball (npm ignorespublishConfig.directory) — all three packages now publish./dist, react gated bycheck:pack<script async src=".../loader.js" data-public-key="pk_...">), throw-resilient queue replay, React moved to peer deps (no dual-React crash on React 18 hosts)@cossistant/nextmirrors all ~41 react subpaths (was 12); shipped.d.tsno longer breaksskipLibCheck:false"use client"on every entry point, prop types exported from barrels,Buttonnaming collision resolved (SupportButton+ deprecated alias), every README/docs snippet verified to compile, new CDN-embed docs pageReviewer notes
packages/react/src/provider.tsx— it's the one coherent refactor (lifecycle, persistence, StrictMode revive); behavior is pinned byprovider.controller-regression.test.tsrefetch()args are per-call only; manual refetches bypass dedup; SSR HTML no longer contains default welcome messages (they appear post-hydration — this is what fixes the hydration mismatch)audit/task_plan.md): clock-skew conversation creation (needs API-side verification), sounds as CDN assets instead of 24KB inline base64, core-siderehydrate()API, 29 low-severity polish findingsVerification
bun test: react 301/301, core 137/137, types 32/32, next 3/3, browser 18/18.tsc --noEmitand Biome clean on all five packages. Browser embed rebuilt and size re-measured.🤖 Generated with Claude Code