Skip to content

fix(pos): close the payment queue drawer on Escape - #1507

Open
khaira777 wants to merge 2 commits into
mainfrom
fix/pos-escape-payment-queue
Open

khaira777 wants to merge 2 commits into
mainfrom
fix/pos-escape-payment-queue

Conversation

@khaira777

Copy link
Copy Markdown
Contributor

Related work

Issue / Discussion: None (audit finding F-19a)

Summary

onDismissModalOrBlur on the POS page handled the scheduled orders drawer but not the payment queue drawer. The drawer has no Escape handler of its own, so Escape left it open. It is now closed after the paymentBill and refundModalTarget cases, so a payment or refund modal opened above the drawer closes first.

Scope

Only the payment queue drawer's Escape behavior.

Verification

Commands run:

  • npm run lint, npm run build, npm run build:frontend
  • Playwright on isolated ports: npx playwright test e2e/pos-keyboard-shortcuts.spec.ts --project=chromium
  • oxfmt --check, git diff --check

Results: all passed. The new test "Escape closes the payment queue drawer" fails without the fix (checked against a frontend build with the fix removed). The test mocks multi_counter_enabled so the queue button shows, focuses the drawer search, and checks that the first Escape blurs it and the second closes the drawer.

Compatibility & data safety

  • Database/data impact: None
  • Migration: None
  • User-visible behavior: Escape closes the payment queue drawer
  • Offline/network impact: None

Cross-cutting surfaces

Surface Status
UI affected
API not affected
Persistence not affected
Permissions not affected

Developer documentation

  • No developer documentation change needed
  • Existing developer documentation updated
  • New page justified
  • Not applicable (docs-only or code-only change)

Contributor checklist

  • This is a small isolated fix/doc update OR follows an approved issue/direction.
  • Unrelated cleanups, refactors, and dependency changes were kept out of this PR.
  • Tests were added or updated for any behavior changes.
  • Verification commands were run and documented above.
  • Customer data and upgrade safety were preserved (if touching database/storage). (N/A)
  • Documentation or translations were updated where relevant.
  • I understand and can explain all submitted changes.

onDismissModalOrBlur handled the scheduled orders drawer but not the payment queue drawer, so Escape left it open. Close it after the payment and refund modals that can open above it.
@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Close the POS payment queue drawer on Escape

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Close the payment queue drawer on Escape after higher-priority modals have been dismissed.
• Add a browser test confirming Escape first blurs drawer search, then closes the drawer.
Diagram

graph TD
  Escape["Escape key"] --> Handler["POS shortcut handler"] --> Focused{"Input focused?"}
  Focused -->|Yes| Blur["Blur input"]
  Focused -->|No| Modal{"Higher modal open?"}
  Modal -->|Yes| Dismiss["Dismiss modal"]
  Modal -->|No| Queue{"Queue open?"}
  Queue -->|Yes| Close["Close drawer"]
  subgraph Legend
    direction LR
    _event["Event"] ~~~ _decision{"Decision"} ~~~ _action["Action"]
  end
Loading
High-Level Assessment

Keep dismissal in the existing POS Escape handler. A drawer-specific keyboard listener would duplicate shortcut handling and make dismissal priority harder to maintain.

Files changed (2) +25 / -0

Bug fix (1) +4 / -0
page.tsxDismiss the payment queue through the POS Escape handler +4/-0

Dismiss the payment queue through the POS Escape handler

• Adds a payment queue check after the existing modal and scheduled-order dismissal checks, closing the drawer when Escape reaches it.

frontend/src/app/(dashboard)/pos/page.tsx

Tests (1) +21 / -0
pos-keyboard-shortcuts.spec.tsCover payment queue Escape behavior +21/-0

Cover payment queue Escape behavior

• Adds a browser test that enables the queue button through mocked settings, opens the drawer, and verifies that Escape first blurs its search input and then closes the drawer.

frontend/e2e/pos-keyboard-shortcuts.spec.ts

@qodo-code-review

qodo-code-review Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Payment queue focus lost after Escape ✓ Resolved

Resolving commit: 54c0df0

Resolution reason
After setShowPaymentQueue(false), a setTimeout defers focusing the pos-payment-queue-btn element via
querySelector, so focus returns to the opener once the drawer unmounts. The e2e test now asserts
that button is focused after Escape.
Description
The new showPaymentQueue branch in onDismissModalOrBlur calls setShowPaymentQueue(false) and
returns without moving focus back to the pos-payment-queue-btn opener, so focus falls to <body>
once the search input unmounts. This happens on every Escape close of the drawer, and the PR's
Playwright test asserts only drawer visibility, so it never catches the lost focus; the
scheduled-orders branch above it shares the same gap, so the copied pattern repeats it.

Grey Divider

Context sources
✅ Compliance rules (platform): 5 rules
✅ Cross-repo context — repo relationships
✅ REVIEW.md
Review mode: Auto: 🚀 Fast: Four-line localized Escape handler addition plus an e2e test; single concern.

Grey Divider

Tip of the day
💡 Did you know, you can hide the parts of a finding you never read, like the evidence or the agent prompt

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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.

1 participant