Skip to content

[WC-3420] Pusher initial implementation#2224

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

[WC-3420] Pusher initial implementation#2224
r0b1n wants to merge 11 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

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@r0b1n r0b1n force-pushed the new-pusher-widget branch from abe155f to 82fedd8 Compare May 26, 2026 11:29
gjulivan
gjulivan previously approved these changes May 26, 2026
Copy link
Copy Markdown
Collaborator

@iobuhov iobuhov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is module code?

Comment thread packages/pluggableWidgets/pusher-web/src/utils/fetchPusherConfig.ts Outdated
@github-actions

This comment was marked as outdated.

@r0b1n r0b1n changed the title chore(pusher-web): initial implementation [WC-3420] Pusher initial implementation Jun 4, 2026
@r0b1n r0b1n force-pushed the new-pusher-widget branch from a8f9c5f to e1a0a82 Compare June 4, 2026 15:17
@github-actions

This comment was marked as outdated.

Comment thread packages/pluggableWidgets/pusher-web/src/hooks/useFetchPusherConfig.ts Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/hooks/usePusherListener.ts Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/Pusher.tsx
@github-actions

This comment was marked as outdated.

@r0b1n r0b1n force-pushed the new-pusher-widget branch from 71b571b to b4d24d1 Compare June 5, 2026 15:04
@github-actions

This comment was marked as outdated.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 5, 2026

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 New main widget component
packages/pluggableWidgets/pusher-web/src/Pusher.xml Widget manifest
packages/pluggableWidgets/pusher-web/typings/PusherProps.d.ts Generated type definitions
packages/pluggableWidgets/pusher-web/src/hooks/usePusherSubscribe.ts Pusher lifecycle hook
packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts Pusher wrapper class
packages/pluggableWidgets/pusher-web/src/utils/createPusherListener.ts Factory for PusherListener
packages/pluggableWidgets/pusher-web/src/utils/fetchPusherConfig.ts Config fetch utility
packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts Channel name builder
packages/pluggableWidgets/pusher-web/src/Pusher.editorConfig.ts Studio Pro editor config
packages/pluggableWidgets/pusher-web/src/Pusher.editorPreview.tsx Studio Pro preview component
packages/pluggableWidgets/pusher-web/src/__tests__/Pusher.spec.tsx Unit test file
packages/pluggableWidgets/pusher-web/src/ui/Pusher.scss Widget styles
packages/pluggableWidgets/pusher-web/src/ui/PusherPreview.css Editor preview styles
packages/pluggableWidgets/pusher-web/package.json Package manifest
packages/pluggableWidgets/pusher-web/CHANGELOG.md Changelog

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


Findings

🔶 Medium — XML declares objectSource as datasource (isList="false") but typings generate ListValue

File: packages/pluggableWidgets/pusher-web/typings/PusherProps.d.ts line 34 / packages/pluggableWidgets/pusher-web/src/Pusher.xml line 4

Problem: The XML property objectSource is type="datasource" isList="false", which maps to a single-object datasource (ObjectItem / DynamicValue<ObjectItem>). However the generated typings declare objectSource: ListValue. This is inconsistent — ListValue is for isList="true" datasources. The widget code then casts to DynamicValue<ObjectItem> with as any, masking the mismatch at runtime.

Fix: Either change the XML to isList="true" if a list is intended, or regenerate the typings. For a single-object datasource the correct type is ObjectItem delivered via a data view context rather than a datasource property. Confirm the intended UX and fix the XML ↔ typings alignment, then remove the as any cast in Pusher.tsx line 200.


🔶 Medium — objectSource read without checking status; widget crashes when datasource is loading

File: packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts line 2

Problem: getChannelName accesses objectSource.value directly. Mendix's DynamicValue/ListValue is asynchronous — .value is undefined while status === "loading" or status === "unavailable". The function checks !object for undefined but does not guard against unavailable/error states, and callers in Pusher.tsx rely on undefined return to skip subscription — which works today only by accident. More importantly, extractEntityName throws an Error if the internal symbol is not found; this will surface as an unhandled exception if the Mendix runtime layout changes.

Fix:

export function getChannelName(objectSource: ListValue): string | undefined {
    if (objectSource.status !== "available") {
        return undefined;
    }
    // …rest of logic
}

🔶 Medium — Placeholder test suite — no real tests

File: packages/pluggableWidgets/pusher-web/src/__tests__/Pusher.spec.tsx

Problem: The entire test file is a single expect(true).toBe(true) placeholder. For an initial implementation that involves async lifecycle, Pusher subscriptions, action execution, and Mendix data integration this provides zero safety net. The TODO comment enumerates what needs testing but none of it is implemented.

Fix: Add at minimum:

  • Render test that usePusherSubscribe is called (mock createPusherListener)
  • Test that notifyEventAction.execute is invoked when the event callback fires
  • Test that no subscription is created when channelName is undefined (loading state)

Use @mendix/widget-plugin-test-utils builders per the repo conventions:

import { actionValue } from "@mendix/widget-plugin-test-utils";

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

🔶 Medium — extractEntityName accesses private Mendix internals via Object.getOwnPropertySymbols

File: packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts lines 18-23

Problem: This function reads the first own symbol of an ObjectItem and calls .getEntity() on it. This relies on undocumented Mendix Client internals. It will silently break with any Mendix Client refactor and the throw new Error will surface as an uncaught runtime crash rather than a graceful degradation.

Fix: Check whether mx.meta.getEntity or the public ObjectItem API provides the entity type. If entity name is not part of the public API, raise with the Mendix platform team before shipping. At minimum, wrap the entire function in a try/catch and return undefined on failure instead of throwing.


⚠️ Low — console.debug / console.error left in production code

File: packages/pluggableWidgets/pusher-web/src/Pusher.tsx line 186; src/utils/PusherListener.ts lines 449, 454; src/utils/fetchPusherConfig.ts lines 510, 515, 524, 529

Note: Multiple console.debug and console.error statements are present throughout. Production widgets in this repo typically do not log to the console unless via a dedicated logging utility. These should be removed or gated behind a debug flag before release.


⚠️ Low — PR description template not filled in; PR type not uncommented

File: PR body

Note: All <!-- --> type sections remain commented out and the description section is empty. This makes it hard to understand what should be tested. Please fill in the PR type and the testing steps.


⚠️ Low — classNames imported but used with only a single static class

File: packages/pluggableWidgets/pusher-web/src/Pusher.editorPreview.tsx line 3

Note: classNames("widget-pusher-preview") is equivalent to just "widget-pusher-preview". The classnames import is unnecessary here. In Pusher.tsx the usage is classnames("widget-pusher", className) which is valid.


Positives

  • Clean separation of concerns: PusherListener encapsulates the Pusher SDK, usePusherSubscribe manages React lifecycle, fetchPusherConfig handles the HTTP layer — each is independently testable.
  • AbortController pattern in usePusherSubscribe correctly cancels in-flight config fetches on unmount and guards against setting state after abort.
  • CSRF token is forwarded on both the key-fetch request and the Pusher auth endpoint, which is the correct security posture for same-origin Mendix REST endpoints.
  • subscription object is stabilised with useMemo rather than creating a new reference on every render, preventing spurious re-subscriptions.
  • CHANGELOG entry present for the new widget as required.

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.

3 participants