Make propertiesPanel.showEntry reliable across collapsed groups and async renders#486
Draft
Copilot wants to merge 7 commits into
Draft
Make propertiesPanel.showEntry reliable across collapsed groups and async renders#486Copilot wants to merge 7 commits into
propertiesPanel.showEntry reliable across collapsed groups and async renders#486Copilot wants to merge 7 commits into
Conversation
Agent-Logs-Url: https://github.com/bpmn-io/properties-panel/sessions/52312ae6-208d-478c-9055-ed4c559c8af3 Co-authored-by: philippfromme <7633572+philippfromme@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bpmn-io/properties-panel/sessions/52312ae6-208d-478c-9055-ed4c559c8af3 Co-authored-by: philippfromme <7633572+philippfromme@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
philippfromme
April 17, 2026 08:22
View session
|
|
Contributor
|
@copilot What about tests? |
Author
Added in |
Contributor
|
@copilot I've tried it in e2e in camunda/linting by removing the setTimout hack and it doesn't work. It only focuses the input if the group was already open. |
Agent-Logs-Url: https://github.com/bpmn-io/properties-panel/sessions/464cc948-64a1-4615-9bfc-c7896cc45bab Co-authored-by: philippfromme <7633572+philippfromme@users.noreply.github.com>
Author
Good catch — reproduced and fixed in The bug: in the linting flow Fix: track the most recently committed request token. On element change, only cancel a pending request if it was already alive in the previous render (stale) — requests issued in the same batch as the element change are preserved. Added a regression test ( Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
|
@copilot Slightly better, it works when I have the element already selected. But if I don't I still need to click twice. |
Agent-Logs-Url: https://github.com/bpmn-io/properties-panel/sessions/1b632a6a-16bf-4aac-bd41-ebbc757b56a7 Co-authored-by: philippfromme <7633572+philippfromme@users.noreply.github.com>
Author
Found the underlying bug — fixed in Root cause: preact runs effects child-first. On the same render where Fix in Could you give it another try in camunda/linting without the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
|
@copilot It still doesn't work. It only focuses if either the element is already selected or the group was already open. |
Agent-Logs-Url: https://github.com/bpmn-io/properties-panel/sessions/da031e39-f468-43d5-b5a1-183805189a11 Co-authored-by: philippfromme <7633572+philippfromme@users.noreply.github.com>
Author
Made the focus mechanism retry-based in Previously the focus effect only re-attempted when something triggered another render of the entry — which works in my isolated tests but is sensitive to the exact ordering of effects and state flushes in real life (host wrapper subscribing to Now: if the entry is not visible at effect time, schedule Also added a regression test ( Could you give it another try in camunda/linting without the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
|
@copilot But isn't this just moving the |
Agent-Logs-Url: https://github.com/bpmn-io/properties-panel/sessions/d2944dac-5c86-4f89-b3eb-16050f666e90 Co-authored-by: philippfromme <7633572+philippfromme@users.noreply.github.com>
Author
Fair point — replaced the rAF retry with a synchronous Unlike
No timers, no animation frames, no waiting on the event loop. The parent always opens before the child paints, and the child always focuses on the same frame. Could you give it another try in camunda/linting without the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
|
@copilot The Let's flip it: instead of pushing focus down, let the entry pull. Keep // useShowEntryEvent.js
useLayoutEffect(() => {
if (!ref.current) return;
if (pendingRequest?.id !== id) return;
if (ref.current.offsetParent === null) return; // not visible yet, wait for next commit
ref.current.focus();
if (ref.current.select) ref.current.select();
resolve(); // clears pendingRequest
}); // no deps — runs on every commitThe parent This is essentially what the Can you try this and update the tests? The element-change cancellation logic in |
…commit Agent-Logs-Url: https://github.com/bpmn-io/properties-panel/sessions/ba69bff7-4ddd-4d33-9f35-4025ebb47e31 Co-authored-by: philippfromme <7633572+philippfromme@users.noreply.github.com>
Author
Good call — flipped to pull-based in
One subtlety I had to keep: the entry latches "wants focus" the first time it observes itself as the target. Without this latch, a panel-level cancellation (e.g. when All 656 tests pass. Could you give it another try in camunda/linting without the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
propertiesPanel.showEntry(id)currently depends on the target entry being mounted at event-fire time: the listener lives inside each entry viauseShowEntryEvent. If the entry's group is collapsed (hidden via CSS —.focus()no-ops) or the entry has not rendered yet (async selection), the request is silently dropped; firing twice "works" but is a visible hack.Changes
src/context/ShowEntryContext.js(new) — coordinator context exposing{ pendingRequest: { id, token } | null, requestShow, resolve }.src/PropertiesPanel.js— subscribes topropertiesPanel.showEntryat the panel root, stores the pending request (monotonic token internally for element-change race handling), cancels on element change (viauseUpdateLayoutEffectso initial requests aren't clobbered), and provides the context. Element-change cancellation tracks the most recently committed request token so requests issued in the same React batch as the element change (the commonselection.select(el); fire('propertiesPanel.showEntry', { id })pattern) are preserved instead of clobbered.resolve()simply clears the current pending request (no token argument).src/components/Group.js,ListGroup.js,entries/Collapsible.js— open themselves viasetOpen(true)when the pending id matches one of their (possibly nested) entries. The open-on-pendingRequesteffect usesuseLayoutEffectsosetOpenruns synchronously after DOM mutations.src/hooks/useShowEntryEvent.js— pull-based: a single deps-lessuseLayoutEffectruns on every commit and asks "am I the coordinator's pending target and am I currently visible (offsetParent !== null)?" If yes, focus + clear the request viaresolve(). If no, do nothing — the next commit will check again. Whatever sequence of renders / event fires it takes for the entry to become visible (group opening, host re-renders, un-batched fires), the entry will focus itself on the first commit where it is both the target and visible. No tokens on the focus path, nosetTimeout/requestAnimationFrame, no assumptions about effect ordering across the tree. The entry latches its "wants focus" state when it first observes itself as the target so a panel-level cancellation (e.g. element-change race) cannot silently drop the request before the entry reacts; the latch is released only when a competing request arrives or after focus succeeds. The legacyuseEvent('propertiesPanel.showEntry', …)subscription is kept for consumers using the hook outside a<PropertiesPanel>and sets the same latch.test/spec/showEntryCoordinator.spec.js(new) — covers: already-visible, collapsed group, late-mounted entry, no focusable control, unknown id, element-change cancellation, batched element-change + showEntry (regression for theselection.select+fireflow), visibility-aware focus (asserts.focus()is never called on a hidden input — Karma's iframe doesn't reliably own browser focus, so the tracker records(id, visible)tuples instead of relying ondocument.activeElement), host-driven separate-fires flow (mounts a wrapper subscribed toselection.changedand fires the two diagram-js events without React-batching them — the closest faithful reproduction of the camunda/linting flow), rapid succession (last wins),data-entry-idregression, legacy hook path.Flow
Backward compatibility
propertiesPanel.showEntryevent name and{ id }payload unchanged.useShowEntryEvent(id)export and signature unchanged; legacy event-bus path preserved.data-entry-idon entry wrappers andPropertiesPanelContext.onShowunchanged.