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 2 times, most recently from 805d167 to 90ed6d3 Compare June 24, 2026 09:48
@github-actions

This comment has been minimized.

@r0b1n r0b1n force-pushed the feat/pusher-module branch from 90ed6d3 to 47b556b Compare June 29, 2026 13:30
@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 to multi-handler event binding via eventHandlers list
packages/pluggableWidgets/pusher-web/src/Pusher.xml Replaced notifyActionName/notifyEventAction with eventHandlers list object
packages/pluggableWidgets/pusher-web/src/Pusher.editorConfig.ts Added check() for duplicate action names; updated getCaption()
packages/pluggableWidgets/pusher-web/src/utils/PusherListener.ts Reworked to bind_global + eventHandlersMap; channel-level rebind optimization
packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts Removed unsafe cast
packages/pluggableWidgets/pusher-web/typings/PusherProps.d.ts Updated typings: objectSourceDynamicValue<ObjectItem>, added EventHandlersType
packages/pluggableWidgets/pusher-web/CHANGELOG.md No entry for this breaking change (widget behavior changed)
packages/modules/pusher/ New module scaffold (package.json, scripts, CHANGELOG, LICENSE)
packages/pluggableWidgets/pusher-web/package.json Version bump 2.0.0 → 4.0.0

Skipped (out of scope): pnpm-lock.yaml, binary PNG assets


Findings

🔶 Medium — No CHANGELOG entry for breaking API change

File: packages/pluggableWidgets/pusher-web/CHANGELOG.md
Problem: The XML schema changed (removed notifyActionName + notifyEventAction, added eventHandlers list) and PusherContainerProps changed (objectSource type changed from ListValue to DynamicValue<ObjectItem>). This is a breaking change for existing widget users and requires a changelog entry per repo conventions.
Fix: Add an entry under [Unreleased] describing the breaking change, e.g.:

### Changed

- Replaced single `notifyActionName`/`notifyEventAction` properties with a configurable `eventHandlers` list, enabling multiple event bindings per widget instance. **Breaking:** existing widget configurations must be migrated.
- Changed `objectSource` from a list datasource to a single-object datasource.

Run pnpm -w changelog to open the guided prompt if available.


🔶 Medium — Placeholder test file — no real coverage for changed logic

File: packages/pluggableWidgets/pusher-web/src/__tests__/Pusher.spec.tsx
Problem: The test file contains only a passing placeholder (expect(true).toBe(true)) with a TODO comment. The PR introduces substantial new logic: multi-handler binding, usePusherSubscribe hook lifecycle, PusherListener channel-change optimisation, and check() in editorConfig. None of this is covered.
Fix: At minimum, add unit tests for:

  • PusherListener.subscribe() — verifies bind_global is called once per channel and eventHandlersMap is updated on re-subscription without a channel rebind.
  • check() in editorConfig — verifies duplicate actionName produces an error, unique names produce none.
  • Pusher component — renders the div, calls executeAction when the relevant event fires.

Use @mendix/widget-plugin-test-utils builders for Mendix data mocking, e.g.:

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

🔶 Medium — objectSource status not checked before dereferencing .value

File: packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts line 4
Problem: objectSource.value is accessed directly without checking objectSource.status. When status is "loading" or "unavailable", .value is undefined, which the if (!object) guard handles — but this can mask a genuine unavailability and cause a silent no-subscribe silently indefinitely. Mendix conventions require checking .status before reading .value from DynamicValue.
Fix:

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

⚠️ Low — useMemo over mutable eventHandlers array causes spurious re-subscriptions

File: packages/pluggableWidgets/pusher-web/src/Pusher.tsx line 45
Note: eventHandlers is a new array reference on every render (Mendix re-creates list prop arrays), so useMemo([channelName, eventHandlers, handleError]) will recompute on every render even if handler contents haven't changed. PusherListener.subscribe() handles this gracefully (skips re-subscription if channelName is the same), but the subscription object reference changes every render, causing the useEffect in usePusherSubscribe to call unsubscribe/subscribe on each render. Consider stabilizing the eventHandlers reference, or memoizing per-handler identity, to avoid churn.


⚠️ Low — extractEntityName uses unsafe symbol-based internal API access

File: packages/pluggableWidgets/pusher-web/src/utils/getChannelName.ts line 22
Note: (object as any)[Object.getOwnPropertySymbols(object)[0]] accesses an internal property via the first Symbol key. This is fragile — symbol order is not guaranteed across Mendix runtime versions, and the approach bypasses TypeScript safety. Consider using a supported Mendix API to get the entity name (e.g. via mx.meta.getMetaObject), or document the risk with a comment referencing the specific Mendix version where this is known to work.


⚠️ Low — action not guarded with canExecute before calling executeAction

File: packages/pluggableWidgets/pusher-web/src/Pusher.tsx line 31
Note: executeAction(handler.action) calls the helper without checking handler.action?.canExecute. executeAction from @mendix/widget-plugin-platform likely guards this internally, but per repo conventions (AGENTS.md: "Check ActionValue.canExecute before execute()") the explicit check should be visible at the call site to avoid ambiguity.


Positives

  • The bind_global + eventHandlersMap pattern is a clean optimization: channel subscriptions are only torn down when the channel name changes, not on every handler update — this avoids Pusher re-auth overhead.
  • The check() function in editorConfig correctly detects duplicate actionName entries at design-time in Studio Pro before runtime, which is a great DX improvement.
  • The AbortController pattern in usePusherSubscribe properly guards against state updates after unmount.
  • Removing the // TODO: fix typings cast in getChannelName.ts and the ListValueDynamicValue<ObjectItem> correction are clean type-safety improvements.
  • The module scaffold in packages/modules/pusher/ follows the existing module pattern (compare web-actions) faithfully.

@iobuhov iobuhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

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