Skip to content

[WC-3452] Add pusher module to the repo#2258

Open
r0b1n wants to merge 6 commits into
mainfrom
feat/pusher-module
Open

[WC-3452] Add pusher module to the repo#2258
r0b1n wants to merge 6 commits into
mainfrom
feat/pusher-module

Conversation

@r0b1n

@r0b1n r0b1n commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

This adds pusher module to the repo, has old widget as an asses to copy into the module.

@r0b1n r0b1n requested a review from a team as a code owner June 9, 2026 14:19
@github-actions

This comment has been minimized.

Comment thread packages/modules/pusher/assets/Pusher_widget_legacy_1.2.0.mpk Outdated
@r0b1n r0b1n force-pushed the feat/pusher-module branch from 9e445e5 to 0455139 Compare June 9, 2026 15:36
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

gjulivan
gjulivan previously approved these changes Jun 11, 2026
@r0b1n r0b1n force-pushed the feat/pusher-module branch from 4cd3a12 to 56a23fa Compare June 11, 2026 12:25
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@r0b1n r0b1n force-pushed the feat/pusher-module branch from fd463f9 to acfb4d9 Compare June 15, 2026 07:36
Comment thread packages/pluggableWidgets/pusher-web/src/Pusher.tsx Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts
Comment thread packages/pluggableWidgets/pusher-web/src/Pusher.tsx Outdated
Comment thread packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts
@r0b1n r0b1n force-pushed the feat/pusher-module branch from acfb4d9 to fab3036 Compare June 15, 2026 07:48
@github-actions

This comment has been minimized.

@r0b1n r0b1n force-pushed the feat/pusher-module branch from fab3036 to f95239d Compare June 15, 2026 08:19
@github-actions

This comment has been minimized.

@r0b1n r0b1n force-pushed the feat/pusher-module branch from 4d1b69c to 9429277 Compare June 16, 2026 07:48
@github-actions

This comment has been minimized.

@r0b1n r0b1n force-pushed the feat/pusher-module branch from 9429277 to 805d167 Compare June 22, 2026 13:40
@r0b1n r0b1n force-pushed the feat/pusher-module branch from 805d167 to 90ed6d3 Compare June 24, 2026 09:48
@github-actions

Copy link
Copy Markdown
Contributor

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 Refactored from single event handler to eventHandlers[] list
packages/pluggableWidgets/pusher-web/src/Pusher.xml Replaced notifyActionName/notifyEventAction with eventHandlers object list
packages/pluggableWidgets/pusher-web/src/Pusher.editorConfig.ts Added check() for duplicate names; updated getCaption
packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts Refactored to bind_global + eventHandlersMap; channel-only resubscription
packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts Removed as any cast — type fix
packages/pluggableWidgets/pusher-web/src/__tests__/Pusher.spec.tsx Placeholder only
packages/pluggableWidgets/pusher-web/typings/PusherProps.d.ts Updated types to match new XML
packages/pluggableWidgets/pusher-web/CHANGELOG.md Not updated for this change
packages/modules/pusher/ New module package (scripts, package.json, CHANGELOG, LICENSE)

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

⚠️ CI checks could not be retrieved (command required approval). Please verify CI is green before merging.


Findings

🔶 Medium — pusher-web CHANGELOG not updated for breaking API change

File: packages/pluggableWidgets/pusher-web/CHANGELOG.md
Problem: The existing Unreleased entry only says "Initial widget scaffolding". This PR replaces the entire public API (notifyActionName + notifyEventActioneventHandlers[]), which is a breaking change for any existing users of this widget. The CHANGELOG must be updated so the module CHANGELOG at packages/modules/pusher/CHANGELOG.md correctly documents what changed in the bundled widget.
Fix:

## [Unreleased]

### Changed

- Replaced single `notifyActionName`/`notifyEventAction` properties with a configurable
  `eventHandlers` list, allowing multiple Pusher events to be bound to separate client actions.

### Added

- Design-time validation: Studio Pro now reports an error when duplicate action names are configured.

Run pnpm -w changelog to add the entry interactively.


🔶 Medium — Unit tests are a stub; changed behaviour has no coverage

File: packages/pluggableWidgets/pusher-web/src/__tests__/Pusher.spec.tsx
Problem: The spec file contains only expect(true).toBe(true) with a TODO comment. The PR introduces non-trivial logic: multi-handler subscription, deduplicate-action-name validation, the bind_global dispatch path, and the channel-stable / handler-update split in PusherListener. None of this is exercised by any test.
Fix: Replace the placeholder with at minimum:

  • PusherListener: subscribe builds handler map; calling with same channel updates map without resubscribing; unsubscribe clears state.
  • Pusher component: no subscription rendered when eventHandlers is empty; executeAction is called on the matching handler when an event fires.
  • Use @mendix/widget-plugin-test-utils builders (new EditableValueBuilder, actionValue()) instead of manual mock objects.

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

File: packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts line 4
Note: objectSource.value is read without first checking objectSource.status. Mendix convention is to check .status === "available" before reading .value to avoid acting on stale data. The function handles undefined gracefully (returns undefined → no subscription), so there is no crash risk, but a loading datasource is silently treated identically to an unavailable one.

export function getChannelName(objectSource: DynamicValue<ObjectItem>): string | undefined {
    if (objectSource.status !== "available") {
        return undefined;
    }
    const object = objectSource.value;
    // ...
}

⚠️ Low — eventHandlers array reference in useMemo deps may cause subscription churn

File: packages/pluggableWidgets/pusher-web/src/Pusher.tsx line 45
Note: eventHandlers (a Mendix object isList prop) is listed as a useMemo dependency. If the Mendix runtime passes a new array reference on each render (which is possible for list-type props), subscription will be a new object every render, causing the subscription useEffect to unsubscribe and resubscribe on every render. Because PusherListener.subscribe guards against channel re-subscription, the Pusher connection won't bounce, but the React cleanup/effect cycle will still fire unnecessarily.

If this becomes a problem in practice, stabilise subscription by computing only when the serialised handler list changes:

const handlersKey = eventHandlers.map(h => `${h.actionName}`).join(",");
const subscription = useMemo(() => {
    // ...
}, [channelName, handlersKey, handleError]);

The action callbacks themselves can be re-captured from eventHandlers inside the subscription via a ref if needed.


Positives

  • PusherListener.subscribe only resubscribes when channelName changes; handler-only changes update the map without touching the Pusher connection — this is the right optimisation.
  • bind_global + eventHandlersMap is a clean pattern that avoids re-binding Pusher channel events on every React render.
  • check() in editorConfig.ts gives developers immediate duplicate-name feedback in Studio Pro at design time — a good UX addition.
  • Proper AbortController pattern in usePusherSubscribe correctly handles the async-init-then-unmount race.
  • Removing the as any cast in getChannelName is the right fix now that the typings correctly reflect DynamicValue<ObjectItem>.
  • Module package scaffolding (build.ts, push-update.ts, release.ts) follows the patterns established by other modules in the repo.

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