Skip to content

Conversation

@adrianlyjak
Copy link
Collaborator

@adrianlyjak adrianlyjak commented Oct 26, 2025

Fixes/tweaks to address the following issues:

  • Sub classes of stop events were not triggering stream completion
  • Failures and cancellations were considered successes
  • mutations to handlers on store state were not triggering re-renders
  • no way to react to a bare handler

@adrianlyjak adrianlyjak marked this pull request as ready for review October 26, 2025 18:00
* Useful when a component stores a Handler instance directly in local state
* (outside of the Zustand store selector lifecycle).
*/
export function useObservedHandler(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is maybe silly, bad hook, but I was passing around handler references from a callback., and then not getting notifications from it in a state var (obviously, but also tempting to do).

Alternately, maybe we should not pass around the full handler, and just an ID like before to enforce that you get the handler details from the store?

@zhaotai
Copy link
Collaborator

zhaotai commented Oct 27, 2025

It looks like you're trying to make handler observable, which should already be covered by store. If no then there might be some bugs. We need to fix those bugs instead of introducing a Pub/Sub inside store. (store itself already has Pub/Sub mechanism).

I will take care of this feature after events persistent merged. Just give me little more time.

@adrianlyjak
Copy link
Collaborator Author

@zhaotai

It looks like you're trying to make handler observable, which should already be covered by store. If no then there might be some bugs. We need to fix those bugs instead of introducing a Pub/Sub inside store. (store itself already has Pub/Sub mechanism).

I will take care of this feature after events persistent merged. Just give me little more time.

Ok, there's going to have to be a big overhaul to handlers then. Handlers are currently just being mutable from what I can see. From what I understand of Zustand, its some get/set sugar on top of standard flux immutable structure. If going towards zustand integration, I think perhaps the functions you call directly on handlers may want to be rethought. I think perhaps we want to move to functional/service APIs existing directly on the store interface, and you qualify with a handler id.

@adrianlyjak
Copy link
Collaborator Author

I will take care of this feature after events persistent merged. Just give me little more time.

IMO this seems much higher priority than persistent events. Right now this is not usable. Workflow handlers never complete, and their attributes never change. You can basically start a workflow and that's it

@zhaotai
Copy link
Collaborator

zhaotai commented Oct 27, 2025

I will take care of this feature after events persistent merged. Just give me little more time.

IMO this seems much higher priority than persistent events. Right now this is not usable. Workflow handlers never complete, and their attributes never change. You can basically start a workflow and that's it

ok, let me prioritize this then.
Actually, I was using workflow-debugger as test instance. Now it all relies on handler-store.
You can take a look at run-details-panel. Is that also not working?

@zhaotai
Copy link
Collaborator

zhaotai commented Oct 28, 2025

According to the requirement from this PR. Having a more comprehensive PR: #154

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