fix: guard against deleted trigger race condition during async popove…

[ShugKnight24]

Summary

Fixes #720

When a user types a trigger character (e.g. @) and quickly deletes it before the async #showPopover() method completes, the popover code continues executing against a trigger that no longer exists in the editor. This can cause unexpected behavior or errors since #buildPopover() and #filterOptions() are both async operations that yield control back to the user.

Approach

Added a #isTriggerPresent() guard check after each await point in #showPopover():

async #showPopover() {
  this.popoverElement ??= await this.#buildPopover()
  if (!this.#isTriggerPresent()) return  // guard after popover build

  this.#resetPopoverPosition()
  await this.#filterOptions()
  if (!this.#isTriggerPresent()) return  // guard after filter

  this.popoverElement.classList.toggle("lexxy-prompt-menu--visible", true)
  // ...
}

[Copilot]

Pull request overview

This PR fixes a race condition in LexicalPromptElement where typing a trigger character and quickly deleting it before async #showPopover() completes would cause JavaScript errors and lock up the editor. It adds #isTriggerPresent() guard checks at both async await points in #showPopover(), plus a system test and test infrastructure (slow people_controller delay) to reproduce the race.

Changes:

• src/elements/prompt.js + app/assets/javascript/lexxy.js: Add #isTriggerPresent() private method and guard checks in #showPopover() after each await point
• test/system/prompts_test.rb: Add system test for the race condition
• test/dummy/app/controllers/people_controller.rb + views: Add delay param support to simulate slow remote loading

Reviewed changes

Copilot reviewed 6 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/elements/prompt.js	Core fix: adds #isTriggerPresent() guards after each await in #showPopover()
app/assets/javascript/lexxy.js	Manually patched built asset (mirrors source changes; should be regenerated from source)
test/system/prompts_test.rb	New system test reproducing the race condition
test/dummy/app/controllers/people_controller.rb	Adds delay parameter support to simulate slow remote response
test/dummy/app/views/posts/_form.html.erb	Adds conditional slow-prompt lexxy-prompt element for tests
test/dummy/app/views/sandbox/show.html.erb	Passes delay param through to the sandbox people prompt

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When #isTriggerPresent() returns false and #showPopover() returns early, #addTriggerListener() is never called. Since #addTriggerListener() already unregistered itself (via unregister() at line 100) before calling #showPopover(), the trigger listener is permanently lost after an early return. The prompt for this trigger will never fire again until the page is reloaded.

To fix this, each early-return guard in #showPopover() should re-add the trigger listener before returning — similar to how #hidePopover() calls this.#addTriggerListener() before it finishes.

Suggested change

	if (!this.#isTriggerPresent()) return
	this.#resetPopoverPosition()
	await this.#filterOptions()
	if (!this.#isTriggerPresent()) return
	if (!this.#isTriggerPresent()) {
	this.#addTriggerListener()
	return
	}
	this.#resetPopoverPosition()
	await this.#filterOptions()
	if (!this.#isTriggerPresent()) {
	this.#addTriggerListener()
	return
	}

[Copilot]

Pull request overview

Copilot reviewed 5 out of 11 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

When #filterOptions() detects the trigger is absent, it calls this.#hidePopover() (line 298) without await. #hidePopover is async and calls #addTriggerListener() after await nextFrame(). However, because #hidePopover() is not awaited in #filterOptions, the async #filterOptions method resolves immediately, and then the second guard in #showPopover (line 164) also detects the trigger is absent and calls #addTriggerListener() directly (line 165).

This leads to two #addTriggerListener() calls: once from the guard at line 165, and once (slightly later) from #hidePopover after its nextFrame await. Each call to #addTriggerListener() registers a new Lexical update listener without cleaning up the previous one. The result is that two trigger listeners are active simultaneously, so the next time the user types the trigger character #showPopover() will be called twice concurrently, potentially causing duplicate popovers or other unexpected behavior.

To fix this, either:

1. Remove the #addTriggerListener() call from the second guard path (lines 164-167), since #hidePopover() will handle re-registering the trigger listener anyway (note: this requires ensuring #hidePopover is called to clean up even for the case where the popover was never visible), or
2. await the #hidePopover() call inside #filterOptions so it completes before the second guard in #showPopover evaluates — but this would require changes to #filterOptions.

[Copilot]

The sleep 0.2 hardcoded delay at line 140 is fragile for testing purposes. The test relies on the 0.2-second sleep falling within the 2-second artificial fetch delay, but this assumption can fail in two ways: on slow CI machines where Capybara/browser startup adds overhead, or on fast CI environments where the remote fetch could unexpectedly complete before 0.2 seconds. If the delay completes before the backspace is sent, the race condition the test is meant to reproduce won't actually occur and the test would pass vacuously.

A more reliable approach would be to wait for an observable browser state that confirms the fetch is in progress before sending the backspace — for example, waiting until the prompt trigger listener is active (i.e., the editor has received the trigger character) before pressing backspace, rather than relying on a fixed sleep duration.

[Copilot]

The delay parameter is passed directly from user-controlled query params and used as an argument to sleep with no upper bound. While this is in a test dummy app, an unbounded sleep with a user-supplied value could allow someone to deliberately tie up a server thread for an arbitrarily long time (e.g., ?delay=99999), which is a resource exhaustion risk even in test/development environments. Consider capping the delay to a reasonable maximum (e.g., sleep [params[:delay].to_f, 10].min).