-
Notifications
You must be signed in to change notification settings - Fork 110
H-5676, H-5929: Stop Simulation when complete + UX Tweaks + Sentry side-effect import fix #8249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
H-5676, H-5929: Stop Simulation when complete + UX Tweaks + Sentry side-effect import fix #8249
Conversation
We need to move website outside of petrinaut lib package.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryImplements terminal-state detection and UX around simulation completion, plus small fixes.
Written by Cursor Bugbot for commit 1de1582. This will update automatically on new commits. Configure here. |
libs/@hashintel/petrinaut/src/core/simulation/check-transition-enablement.test.ts
Outdated
Show resolved
Hide resolved
🤖 Augment PR SummarySummary: This PR adds “simulation complete” handling to Petrinaut by detecting terminal (deadlock) markings and updating the UI accordingly. Changes:
Technical Notes: Terminal detection is based on token-count structural enablement (not lambda evaluation), and completion notification is triggered via a Zustand subscription. 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return children; | ||
| } | ||
|
|
||
| return <Tooltip content={message}>{children}</Tooltip>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tooltip uses ArkTooltip.Trigger asChild, and disabled form controls (e.g. <input disabled>) typically don’t emit pointer events—so this wrapper may not actually show the tooltip when disabled is true. Worth verifying the tooltip appears in the readonly/simulation mode UX.
🤖 Was this useful? React with 👍 or 👎
| notif.id === id ? { ...notif, exiting: true } : notif, | ||
| ), | ||
| ); | ||
| }, duration - 200); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…better user experience in read-only mode
| const notify = useCallback((options: NotifyOptions) => { | ||
| const id = nextIdRef.current++; | ||
| const duration = options.duration ?? DEFAULT_DURATION; | ||
|
|
||
| setNotifications((prev) => [ | ||
| ...prev, | ||
| { id, message: options.message, exiting: false }, | ||
| ]); | ||
|
|
||
| // Start exit animation before removing | ||
| setTimeout(() => { | ||
| setNotifications((prev) => | ||
| prev.map((notif) => | ||
| notif.id === id ? { ...notif, exiting: true } : notif, | ||
| ), | ||
| ); | ||
| }, duration - 200); | ||
|
|
||
| // Remove after exit animation completes | ||
| setTimeout(() => { | ||
| setNotifications((prev) => prev.filter((notif) => notif.id !== id)); | ||
| }, duration); | ||
| }, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leak potential: setTimeout callbacks are not cleaned up if the component unmounts or if multiple notifications with the same ID are created rapidly. The timeouts continue to execute and call setNotifications on an unmounted component, which will cause React warnings and potential memory leaks.
Fix: Store timeout IDs in a ref and clear them in a cleanup function:
const timeoutsRef = useRef<Map<number, NodeJS.Timeout[]>>(new Map());
const notify = useCallback((options: NotifyOptions) => {
const id = nextIdRef.current++;
const duration = options.duration ?? DEFAULT_DURATION;
setNotifications((prev) => [
...prev,
{ id, message: options.message, exiting: false },
]);
const exitTimeout = setTimeout(() => {
setNotifications((prev) =>
prev.map((notif) =>
notif.id === id ? { ...notif, exiting: true } : notif,
),
);
}, duration - 200);
const removeTimeout = setTimeout(() => {
setNotifications((prev) => prev.filter((notif) => notif.id !== id));
timeoutsRef.current.delete(id);
}, duration);
timeoutsRef.current.set(id, [exitTimeout, removeTimeout]);
}, []);
useEffect(() => {
return () => {
timeoutsRef.current.forEach((timeouts) => {
timeouts.forEach(clearTimeout);
});
};
}, []);Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.

Kapture 2026-01-10 at 03.19.33.mp4 (uploaded via Graphite)
🌟 What is the purpose of this PR?
sideEffectsimport for Sentry instrument import (which relies on side-effects)Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
❓ How to test this?
Use latest Petrinaut deployment