Skip to content

chore(pusher-web): initial implementation#2224

Open
r0b1n wants to merge 6 commits into
mainfrom
new-pusher-widget
Open

chore(pusher-web): initial implementation#2224
r0b1n wants to merge 6 commits 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

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@r0b1n r0b1n force-pushed the new-pusher-widget branch from 25f4e17 to c821834 Compare May 22, 2026 11:52
@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 Main widget component
src/Pusher.xml Widget XML manifest
typings/PusherProps.d.ts Generated prop typings
src/hooks/useFetchPusherConfig.ts Config fetch hook
src/hooks/usePusherListener.ts Pusher instance lifecycle hook
src/hooks/usePusherSubscribe.ts Subscription management hook
src/utils/PusherListener.ts Core Pusher wrapper class
src/utils/fetchPusherConfig.ts REST config fetcher
src/utils/useMxObjectInfo.ts Mendix object GUID/entity extractor
src/Pusher.editorConfig.ts Studio Pro design-time config
src/Pusher.editorPreview.tsx Studio Pro preview component
src/__tests__/Pusher.spec.tsx Unit tests
src/ui/Pusher.scss, src/ui/PusherPreview.css Styles
package.json, tsconfig.json, CHANGELOG.md Package scaffolding

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

CI checks could not be retrieved (command requires approval in this environment).


Findings

🔶 Medium — No unit tests

File: src/__tests__/Pusher.spec.tsx lines 1–7
Problem: The test file contains only a TODO comment inside an empty describe block. A new production widget with network I/O, Pusher subscriptions, and Mendix action execution ships with zero test coverage.
Fix: At minimum add tests for:

import { render } from "@testing-library/react";
import { actionValue, obj } from "@mendix/widget-plugin-test-utils";
import Pusher from "../Pusher";

const defaultProps = {
    name: "pusher1",
    class: "",
    objectSource: /* ListValueBuilder or DynamicValue mock */ undefined as any,
    notifyActionName: "change",
    notifyEventAction: actionValue()
};

describe("Pusher", () => {
    it("renders without crashing when objectSource is unavailable", () => {
        const { container } = render(<Pusher {...defaultProps} />);
        expect(container.firstChild).toBeInTheDocument();
    });
    // test that notifyEventAction.execute() is called when onEvent fires
});

Also cover PusherListener subscribe/unsubscribe lifecycle and fetchPusherConfig parsing.


🔶 Medium — Stale onEvent handler when callback identity changes

File: src/utils/PusherListener.ts lines 465–467
Problem: subscribe() short-circuits when channelName and eventName match the current subscription:

if (channelName === this.currentChannelName && config.eventName === this.currentEventName) {
    return; // ← new onEvent callback is silently ignored
}

In Pusher.tsx, handleEvent is wrapped in useCallback with notifyEventAction as a dependency. When notifyEventAction changes (Mendix can provide a new reference each render), handleEvent gets a new identity, subscription useMemo fires, usePusherSubscribe calls listener.subscribe(subscription) again — but subscribe() returns early without rebinding. The widget then calls the stale action indefinitely.
Fix: Store the latest callback in a ref rather than relying on channel-level re-subscription, or explicitly check for onEvent identity change:

if (channelName === this.currentChannelName && config.eventName === this.currentEventName) {
    // Update the handler in place instead of early-returning
    this.currentChannel?.unbind(this.currentEventName);
    this.currentChannel?.bind(config.eventName, config.onEvent);
    return;
}

🔶 Medium — objectSource typed as ListValue but XML declares isList="false"

File: typings/PusherProps.d.ts line 12 / src/Pusher.xml line 5
Problem: The generated typings declare objectSource: ListValue, but the XML has type="datasource" isList="false", which in the Mendix PWT corresponds to a single-object source (DynamicValue<ObjectItem>). The mismatch forced objectSource as any in Pusher.tsx (line 14) and (objectSource as any)?.value in useMxObjectInfo.ts (line 8). If the typings are auto-generated they need to be re-generated after correcting the XML; if they are hand-authored they need to be updated.
Fix: Update PusherProps.d.ts:

import { ActionValue, DynamicValue } from "mendix";
import { ObjectItem } from "mendix";

export interface PusherContainerProps {
    // ...
    objectSource: DynamicValue<ObjectItem>;
    // ...
}

Then remove the as any casts in Pusher.tsx and useMxObjectInfo.ts.


⚠️ Low — extractEntityName relies on private Mendix symbol internals

File: src/utils/useMxObjectInfo.ts lines 22–28
Note: Accessing Object.getOwnPropertySymbols(object)[0] to reach mxObject is undocumented and implementation-specific. It will silently break when Mendix changes its internal ObjectItem representation. If a public API exists (e.g., via window.mx.meta.getEntityByObject), prefer that. If not, document the specific Mendix version this was tested against.


⚠️ Low — fetchPusherConfig rejects only HTTP 200, not the full 2xx range

File: src/utils/fetchPusherConfig.ts line 31
Note: if (response.status !== 200) will silently fail on 201 or 204 responses. Prefer if (!response.ok) to accept the full 2xx range.


⚠️ Low — PR description is blank; type checkboxes not selected

File: PR template
Note: The PR body has every section commented out. Please uncomment the correct PR type (new feature) and fill in the "What should be covered while testing?" section so reviewers and QA can reproduce the expected behaviour.


Positives

  • useFetchPusherConfig correctly uses an active flag and AbortController to prevent state updates after unmount — the async-with-cleanup guard pattern is implemented correctly.
  • PusherListener.destroy() properly calls unsubscribe() before disconnect(), ensuring clean teardown in widget unmount.
  • CSRF token forwarded to both the config endpoint and Pusher auth endpoint — correct Mendix security pattern.
  • CHANGELOG entry present for a new widget, following Keep a Changelog format.
  • subscription object is stabilised with useMemo in Pusher.tsx, avoiding unnecessary re-subscription churn.

@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
packages/pluggableWidgets/pusher-web/src/Pusher.tsx Root widget component
packages/pluggableWidgets/pusher-web/src/Pusher.xml Widget XML manifest
packages/pluggableWidgets/pusher-web/typings/PusherProps.d.ts Generated prop types
packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts Pusher connection/subscription class
packages/pluggableWidgets/pusher-web/src/utils/fetchPusherConfig.ts Backend config fetch
packages/pluggableWidgets/pusher-web/src/utils/useMxObjectInfo.ts GUID/entity extraction hook
packages/pluggableWidgets/pusher-web/src/hooks/useFetchPusherConfig.ts Config fetch hook
packages/pluggableWidgets/pusher-web/src/hooks/usePusherListener.ts Listener lifecycle hook
packages/pluggableWidgets/pusher-web/src/hooks/usePusherSubscribe.ts Subscription management hook
packages/pluggableWidgets/pusher-web/src/Pusher.editorConfig.ts Studio Pro design-time config
packages/pluggableWidgets/pusher-web/src/Pusher.editorPreview.tsx Editor preview component
packages/pluggableWidgets/pusher-web/src/__tests__/Pusher.spec.tsx Unit tests
packages/pluggableWidgets/pusher-web/src/ui/Pusher.scss Widget styles
packages/pluggableWidgets/pusher-web/src/ui/PusherPreview.css Preview styles
packages/pluggableWidgets/pusher-web/package.json Package manifest
packages/pluggableWidgets/pusher-web/CHANGELOG.md Changelog

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

CI checks could not be retrieved (permissions); manual verification recommended.


Findings

🔶 Medium — Empty test file with only TODO comments

File: packages/pluggableWidgets/pusher-web/src/__tests__/Pusher.spec.tsx lines 1–7
Problem: The test file contains zero actual test cases. A new widget with async side effects (fetch, Pusher WebSocket connection, execute call), hooks with cleanup, and a utility class should have coverage before merging. An empty describe block will always pass CI but provides no safety net.
Fix: At minimum, add tests for the happy path and the key branches. Using the project's builder pattern:

import { render } from "@testing-library/react";
import { actionValue } from "@mendix/widget-plugin-test-utils";
import Pusher from "../Pusher";

const defaultProps = {
    name: "pusher1",
    class: "",
    objectSource: /* ListValueBuilder */ ...,
    notifyActionName: "change",
    notifyEventAction: actionValue()
};

describe("Pusher", () => {
    it("renders without crashing", () => {
        const { container } = render(<Pusher {...defaultProps} />);
        expect(container.querySelector(".widget-pusher")).toBeTruthy();
    });
    // Test action execution, loading state, null objectSource, etc.
});

🔶 Medium — objectSource type mismatch between XML and runtime usage

File: packages/pluggableWidgets/pusher-web/src/Pusher.tsx line 184 and packages/pluggableWidgets/pusher-web/src/utils/useMxObjectInfo.ts line 7
Problem: The generated PusherProps.d.ts types objectSource as ListValue (correct for the XML isList="false" datasource), but useMxObjectInfo is parameterised as DynamicValue<ObjectItem> and accesses .value — a field that does not exist on ListValue. The as any cast in Pusher.tsx hides this at compile time. At runtime, objectSource.value will be undefined, so guid and entityName will never be extracted and the widget will never subscribe to any Pusher channel.
Fix: Align the access pattern with ListValue: use objectSource.items?.[0] (or the appropriate API) to get the single object, or change the XML to type="object" if a single-object reference is what's needed. Remove the as any cast once the types agree.


🔶 Medium — extractEntityName relies on undocumented internal Mendix symbol property

File: packages/pluggableWidgets/pusher-web/src/utils/useMxObjectInfo.ts lines 23–29
Problem: The function reads the entity name via object[Object.getOwnPropertySymbols(object)[0]], then calls .getEntity() on the result. This hard-codes an assumption about Mendix's internal object representation (a private symbol at index 0). The Mendix client does not guarantee this API shape; it can change silently in any runtime update, causing the widget to throw during render with no graceful recovery.
Fix: Use the stable public Mendix API. ObjectItem exposes its entity name through mx.meta.getEntity(object.id) or through the type property depending on the Mendix version. If the public API does not yet expose this, leave a clear // intentional internal API comment and document the Mendix version it was verified against, or open a PWT feature request and avoid shipping this until a stable API exists.


🔶 Medium — pusher:subscription_error listener is never unbound (memory leak)

File: packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts lines 83–90 and 96–107
Problem: subscribe() binds an inline arrow function to "pusher:subscription_error". unsubscribe() only calls unbind(this.currentEventName) — it never unbinds the error handler. Because the arrow function is anonymous, it can never be targeted for removal. Each subscribe() call accumulates another error handler on the same channel object, and they all fire on the next error even after the channel should be "clean".
Fix: Store the error handler as a class field and explicitly unbind it:

private currentErrorHandler: ((error: unknown) => void) | null = null;

// in subscribe():
this.currentErrorHandler = (error: unknown) => { ... };
this.currentChannel.bind("pusher:subscription_error", this.currentErrorHandler);

// in unsubscribe():
if (this.currentErrorHandler) {
    this.currentChannel.unbind("pusher:subscription_error", this.currentErrorHandler);
    this.currentErrorHandler = null;
}

🔶 Medium — PR description is blank; PR type not selected

File: PR metadata
Problem: The entire PR body is the unfilled template. The PR type block is not uncommented, and the Description section is empty. There is no explanation of what changed, why, or how to test it. This blocks proper review and traceability.
Fix: Fill in at minimum: PR type (New feature), a 2–3 sentence description of the widget's purpose and integration approach, and a "What to test" checklist.


⚠️ Low — console.debug calls left in production code paths

File: packages/pluggableWidgets/pusher-web/src/Pusher.tsx line 19; packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts line 74
Note: console.debug calls on the event-received and state-change hot paths will appear in every production browser console that has verbose logging enabled. Strip them or gate behind a process.env.NODE_ENV === "development" guard.


⚠️ Low — classNames called with a single static string in editor preview

File: packages/pluggableWidgets/pusher-web/src/Pusher.editorPreview.tsx line 8
Note: classNames("widget-pusher-preview") is equivalent to the plain string literal. The classnames import is only needed when conditionally combining class names. Use className="widget-pusher-preview" directly and remove the import.


⚠️ Low — useFetchPusherConfig silently swallows fetch failures with no retry

File: packages/pluggableWidgets/pusher-web/src/hooks/useFetchPusherConfig.ts lines 16–19
Note: If fetchPusherConfig resolves to null (network error, non-200 status), the hook stays at config = null forever. From the outside this is indistinguishable from "still loading". The widget will silently never subscribe. Consider exposing an error state or retry mechanism, or at least logging a widget-level warning so developers can diagnose misconfigured environments.


Positives

  • Proper active guard and AbortController in useFetchPusherConfig — avoids the classic set-state-on-unmounted-component bug.
  • PusherListener cleanly separates initialize, subscribe, unsubscribe, and destroy — easy to test in isolation once the unit tests are added.
  • useMemo on the subscription config in Pusher.tsx prevents spurious re-subscriptions from referential inequality.
  • CHANGELOG entry present for the initial release.
  • CSRF token threaded through both the config fetch and the Pusher auth endpoint — correct security posture for a same-origin Mendix REST call.

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