Skip to content

chore(pusher-web): initial implementation#2224

Open
r0b1n wants to merge 1 commit into
mainfrom
new-pusher-widget
Open

chore(pusher-web): initial implementation#2224
r0b1n wants to merge 1 commit into
mainfrom
new-pusher-widget

Conversation

@r0b1n
Copy link
Copy Markdown
Collaborator

@r0b1n r0b1n commented May 21, 2026

Pull request type


Description

@r0b1n r0b1n requested a review from a team as a code owner May 21, 2026 15:25
@github-actions
Copy link
Copy Markdown

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed


What was reviewed

File Change
src/Pusher.tsx New root widget component
src/hooks/usePusherConfig.ts Hook to fetch Pusher key/cluster from backend
src/hooks/usePusherListener.ts Hook managing Pusher listener lifecycle
src/utils/PusherListener.ts Class wrapping pusher-js for subscribe/unsubscribe
src/utils/useMxObjectInfo.ts Hook extracting GUID + entity name from Mendix object
src/__tests__/Pusher.spec.tsx Test file
src/Pusher.xml Widget XML manifest
typings/PusherProps.d.ts Generated typings
src/Pusher.editorConfig.ts Studio Pro design-time config
src/Pusher.editorPreview.tsx Studio Pro preview component
src/ui/Pusher.scss Widget styles
src/ui/PusherPreview.css Preview styles
src/package.xml Package manifest
package.json Package metadata
CHANGELOG.md Changelog

Skipped (out of scope): pnpm-lock.yaml


Findings

🔶 Medium — Subscription never fires after config loads

File: src/hooks/usePusherListener.ts lines 38–50
Problem: The two useEffect hooks are split: one initialises the listener (depends on pusherConfig/enabled), the other subscribes (depends only on subscription). On first render subscription is already set, so effect 2 runs — but listenerRef.current is still null and returns early. When usePusherConfig resolves and effect 1 runs (creating the listener), effect 2 does not re-run because subscription hasn't changed. The widget will never subscribe unless the user's object or event name changes after mount.

Fix: Add pusherConfig (or the enabled flag) to the subscription effect's dependency array so it re-runs once the listener is ready:

useEffect(() => {
    const listener = listenerRef.current;
    if (!listener || !subscription) {
        return;
    }

    listener.subscribe(subscription);

    return () => {
        listener.unsubscribe();
    };
}, [subscription, pusherConfig]); // pusherConfig triggers re-run when listener is created

🔶 Medium — Async fetch without unmount guard

File: src/hooks/usePusherConfig.ts lines 16–52
Problem: The fetch chain calls setConfig(...) after an async response, but there is no cleanup guard. If the component unmounts before the request resolves, React logs a "state update on unmounted component" warning and may cause subtle bugs. No AbortController is used either, so the in-flight request is never cancelled.

Fix:

useEffect(() => {
    let active = true;
    const controller = new AbortController();

    fetch(endpoint, { signal: controller.signal, credentials: "same-origin", headers: { "X-Csrf-Token": csrfToken } })
        .then(res => { ... return res.text(); })
        .then(data => {
            if (active) setConfig({ ... });
        })
        .catch(err => {
            if (active) console.error("[usePusherConfig]", err);
        });

    return () => {
        active = false;
        controller.abort();
    };
}, []);

🔶 Medium — No unit tests for a new widget

File: src/__tests__/Pusher.spec.tsx
Problem: The test file contains only a describe block with TODO comments. A new widget with real-time subscription logic (init, subscribe, unsubscribe, error handling, action execution) ships with zero tests. This is especially risky given the subscription race identified above — a test would have caught it.

Fix: Add tests using @mendix/widget-plugin-test-utils builders. Minimum coverage needed:

  • PusherListener: subscribe, unsubscribe, destroy, error callback
  • usePusherConfig: successful fetch, failed fetch, unmount cleanup
  • usePusherListener: subscription fires after config loads, cleans up on unmount
  • Pusher component: renders without crashing, executes action on event

⚠️ Low — Fragile undocumented symbol access for entity name

File: src/utils/useMxObjectInfo.ts lines 26–32
Problem: extractEntityName reads the first Symbol key on the ObjectItem to find the internal mxObject:

const mxObj = (object as any)[Object.getOwnPropertySymbols(object)[0]];

This relies on undocumented Mendix internals. The symbol position, presence, and shape are not guaranteed by any public API and may silently break on a Mendix runtime upgrade.

Note: If no public API exists yet for this, add an explicit code comment explaining why this is necessary, file a PWT issue to expose getEntity(), and pin the minimum Mendix version in package.json to one you have verified this works with.


⚠️ Low — objectSource type mismatch between XML, typings, and usage

File: src/Pusher.tsx line 13, src/utils/useMxObjectInfo.ts line 9
Problem: The generated typings declare objectSource: ListValue, but useMxObjectInfo treats it as DynamicValue<ObjectItem> (accessing .value as an ObjectItem). The as any cast on line 13 of Pusher.tsx suppresses the TypeScript error rather than resolving the mismatch. A ListValue exposes .items[], not .value.

Fix: Decide on the correct type. If a single-context object is intended, the XML type="datasource" isList="false" with the typings updated to reflect the actual runtime shape. Resolve without as any.


⚠️ Low — objectSource.status not checked before reading value

File: src/utils/useMxObjectInfo.ts line 10
Problem: (objectSource as any)?.value is accessed unconditionally. If the data source is in a Loading or Unavailable state, this returns undefined, and useMemo returns undefined too — which causes the subscription to be skipped silently. The convention is to check objectSource.status === "available" first.


⚠️ Low — Misleading XML property name notifyChannelName

File: src/Pusher.xml line 13, src/utils/PusherListener.ts line 59
Problem: The property is named notifyChannelName and its caption is "Notify event name", but it maps to the event name (the argument to channel.bind()), not the channel name. The actual channel is built as private-${entityName}.${guid}. This will confuse Studio Pro users who read the property key in microflows and expect to pass a channel name.

Fix: Rename to notifyEventName (XML key + TS prop) to match the Studio Pro caption and actual usage.


Positives

  • PusherListener class cleanly separates Pusher lifecycle (init/subscribe/unsubscribe/destroy) from React, making it independently testable.
  • unsubscribe() correctly unbinds the event handler before calling pusher.unsubscribe(), avoiding stale event listener accumulation.
  • CSRF token is forwarded to both the key endpoint and the Pusher auth endpoint — correct for a Mendix REST integration.
  • The buildChannelName convention (private-${entityName}.${guid}) matches Pusher's private channel naming scheme.
  • CHANGELOG.md entry is present for the initial release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant