SY-4145: Fix Schematic Keyboard Triggering

[sy-nico]

Issue Pull Request

Linear Issue

SY-4145

Description

Suppress keyboard triggers when target is outside the trigger region.
Fixed at the trigger level rather than checking modal state, so it applies universally to any overlay (modals, dialogs, command palette, etc.).

Basic Readiness

• I have performed a self-review of my code.
• I have added relevant, automated tests to cover the changes.

Greptile Summary

This PR fixes keyboard trigger leakage in the Schematic by adding a DOM-containment check to filterInRegion: in addition to the existing cursor bounding-box test, the new code requires the keyboard event's target to be either inside the region or an ancestor of it (e.g. document.body), blocking triggers whose target lives in a sibling subtree such as a modal input or command palette.

Confidence Score: 4/5

Safe to merge with the minor caveat that spurious "end" events can fire without a prior "start" in the blocked-target scenario.

Only P2 findings — the core logic is correct and the new test adequately covers the fixed behaviour. The "end-without-start" issue is a pre-existing pattern that this PR makes newly reachable for the sibling case, but existing callers handle it gracefully.

pluto/src/triggers/hooks.ts — the unfiltered removed path at lines 69-77

Important Files Changed

Filename	Overview
pluto/src/triggers/hooks.ts	Adds a DOM-containment check alongside the existing bounding-box check so keyboard triggers are suppressed when the event target lives in a sibling subtree. Logic is correct for the "start" path, but the "end" path remains unfiltered, which can produce a spurious "end" callback when the blocked keys are released.
pluto/src/triggers/Triggers.spec.tsx	Adds a focused regression test for the sibling-subtree suppression — verifies both that a keydown on a sibling input is blocked and that the same shortcut on document.body still fires.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Keyboard event fires] --> B{region prop set?}
    B -- No --> Z[Pass trigger through]
    B -- Yes --> C{region.current null?}
    C -- Yes --> Y[Block all triggers]
    C -- No --> D{regionMustBeElement\nor Mouse trigger?}
    D -- Yes --> E{cursor in bbox\nAND target === region.current?}
    E -- Yes --> Z
    E -- No --> Y
    D -- No --> F{cursor in bbox?}
    F -- No --> Y
    F -- Yes --> G{"region.current.contains(target)\nOR target.contains(region.current)?"}
    G -- Yes --> Z
    G -- No --> Y[Block trigger — sibling subtree]

    style G fill:#d4edda,stroke:#28a745
    style Y fill:#f8d7da,stroke:#dc3545
    style Z fill:#d1ecf1,stroke:#17a2b8

Loading

Comments Outside Diff (1)

1. pluto/src/triggers/hooks.ts, line 69-77 (link)

   Spurious "end" events without a preceding "start"

   With this change, a keyboard trigger's "start" is now suppressed when the event target is in a sibling subtree (correct), but the removed diff is never passed through filterInRegion. That means when the keys are released after a blocked start, the callback is still called with stage: "end" even though stage: "start" was never fired.

   For callers like useHeldRef, a spurious "end" is benign (purge on an empty list is a no-op). But any caller that treats "start" and "end" as a matched pair (e.g. setting a loading state to true on start and false on end) could enter an inconsistent state here.

   Consider applying the same target-containment guard to removed:

   const filteredRemoved = filterInRegion(e.target, e.cursor, removed, region, regionMustBeElement);
   if (added.length > 0)
     f?.({ ...base, stage: "start", triggers: added, prevTriggers: e.prev });
   if (filteredRemoved.length > 0)
     f?.({ ...base, stage: "end", triggers: filteredRemoved, prevTriggers: e.prev });

_{Reviews (1): Last reviewed commit: "SY-4145: Suppress keyboard triggers when..." | Re-trigger Greptile}

[emilbon99]

We already have a regionMustBeElement option? Is that option still necessary? I remember getting the triggers to work correctly in all scenarios was pretty tricky and it took me a while to get right, so I'm a bit concerned that this will unexpectedly break triggers in places where we don't expect.

Just double check and make sure you're confident that this fix is the right one and is sustainable, and if regionMustBeElement is necessary.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.23%. Comparing base (f7acaa9) to head (456423a).
⚠️ Report is 1 commits behind head on rc.

Additional details and impacted files

@@            Coverage Diff             @@
##               rc    #2287      +/-   ##
==========================================
- Coverage   64.25%   64.23%   -0.02%     
==========================================
  Files        2178     2178              
  Lines      110433   110435       +2     
  Branches     8298     8291       -7     
==========================================
- Hits        70956    70939      -17     
- Misses      33407    33424      +17     
- Partials     6070     6072       +2     

Flag	Coverage Δ
console	20.42% <ø> (ø)
pluto	55.64% <100.00%> (-0.11%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.