SDK audit: fix 85 bug/footprint/DX findings across react, next, and browser#161
SDK audit: fix 85 bug/footprint/DX findings across react, next, and browser#161Rieranthony wants to merge 1 commit 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>
|
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".
| 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 👍 / 👎.
| 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 👍 / 👎.
1 participant
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