refactor(core): move UI `ButtonRequest`-related code into `ButtonRequestHandler` by romanz · Pull Request #6741 · trezor/trezor-firmware

romanz · 2026-04-09T19:49:49Z

Also, minimize context-related access in ui.Layout and move debug-specific functionality under __debug__.

Will be used to resolve #6348 (Case 1) by #6651 (stopping button-request handling in backup flow after an error/timeout).

Existing functionality doesn't change, so AFAIU there is no specific QA needed.

coderabbitai · 2026-04-09T19:49:59Z

Walkthrough

Button-request handling was reworked: ButtonRequestHandler now owns an internal mailbox and lifecycle methods put, br_task(ack_callback), and join. Layout no longer stores Context/LayoutState for ACK sequencing; it holds a button_request_handler from context, starts the handler via br_task(self._button_request_acked), enqueues requests with put, awaits handler completion with join(...) before returning results, clears handler/task refs on stop, and uses a debug-only _is_attached plus debug errors to signal pending ACKs. The handler now requires a non-optional ack callback invoked unconditionally and clears pending state internally.

Sequence Diagram(s)

sequenceDiagram
    participant Layout
    participant BRHandler as ButtonRequestHandler
    participant Firmware as Firmware/Transport
    participant AckCB as AckCallback

    Layout->>BRHandler: br_task(ack_callback)
    Note right of BRHandler: starts internal _handle loop (task)
    Layout->>BRHandler: put(ButtonRequest)
    BRHandler->>BRHandler: enqueue request in internal box
    BRHandler->>Firmware: send ButtonRequest / await ButtonAck
    Firmware-->>BRHandler: ButtonAck received
    BRHandler->>AckCB: invoke ack_callback()
    AckCB-->>Layout: notify _button_request_acked
    Layout->>BRHandler: join(_waiting_screen_task)
    BRHandler->>BRHandler: enqueue sentinel (None) and finish loop
    BRHandler-->>Layout: join returns (handler completed)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 47.83% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	❓ Inconclusive	The PR description is minimal but adequate, aligning with the template's non-intrusive style for internal developers; however, it lacks project management metadata per template guidance.	Consider adding project setup details (project assignment, priority, sprint) and QA status as suggested in the template for core developers' final PRs.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: refactoring UI ButtonRequest-related code into ButtonRequestHandler, which aligns with the substantial modifications in both files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch romanz/2604/br-rework

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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-04-09T19:51:15Z

`en` main(all)

model	device_test	click_test	persistence_test
T2T1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)	test(all) main(all)

Translations

`cs` main(all)

model	device_test	click_test
T2T1	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)

`de` main(all)

model	device_test	click_test
T2T1	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)

`es` main(all)

model	device_test	click_test
T2T1	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)

`fr` main(all)

model	device_test	click_test
T2T1	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)

`pt` main(all)

model	device_test	click_test
T2T1	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)

Latest CI run: 25055517876

coderabbitai

🧹 Nitpick comments (1)

core/src/trezor/ui/__init__.py (1)

342-350: Minor: Redundant msg and check.

On line 346, the msg and ButtonRequest(...) pattern is redundant since msg is already checked for None on line 343. However, this is a minor style issue and doesn't affect correctness.

♻️ Optional cleanup

     def put_button_request(self, msg: ButtonRequestMsg | None) -> bool:
         if self.button_request_handler is None or msg is None:
             return False
 
-        br = msg and ButtonRequest(
+        br = ButtonRequest(
             code=msg[0], name=msg[1], pages=self.layout.page_count()
         )
         self.button_request_handler.put(br)
         return True

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@core/src/trezor/ui/__init__.py` around lines 342 - 350, In
put_button_request, remove the redundant "msg and" when building the
ButtonRequest since msg is already checked for None; directly construct br =
ButtonRequest(code=msg[0], name=msg[1], pages=self.layout.page_count()) and then
call self.button_request_handler.put(br) (symbols: put_button_request,
ButtonRequest, button_request_handler, ButtonRequestMsg).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@core/src/trezor/ui/__init__.py`:
- Around line 342-350: In put_button_request, remove the redundant "msg and"
when building the ButtonRequest since msg is already checked for None; directly
construct br = ButtonRequest(code=msg[0], name=msg[1],
pages=self.layout.page_count()) and then call
self.button_request_handler.put(br) (symbols: put_button_request, ButtonRequest,
button_request_handler, ButtonRequestMsg).

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7c20ca49-5e1e-4548-a848-035bd21f995f

📥 Commits

Reviewing files that changed from the base of the PR and between f45b057 and 204a167.

📒 Files selected for processing (2)

core/src/trezor/ui/__init__.py
core/src/trezor/wire/protocol_common.py

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@core/src/trezor/wire/protocol_common.py`:
- Around line 166-171: The join() method currently uses self.box.put(None,
replace=True) which can overwrite a queued ButtonRequest and cause the handler
(_handle) to exit without sending/acking the last prompt; change this to enqueue
a shutdown sentinel without replacing existing items — e.g. call
self.box.put(None) (omit replace=True) or, better, enqueue a unique sentinel
object (SENTINEL = object()) via self.box.put(SENTINEL) so the mailbox will not
drop any queued ButtonRequest and the _handle loop can process all work before
seeing the sentinel.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 95dcfc04-739c-46e8-9ddf-7302f3a13a36

📥 Commits

Reviewing files that changed from the base of the PR and between d6a847e and 6dbb78e.

📒 Files selected for processing (2)

core/src/trezor/ui/__init__.py
core/src/trezor/wire/protocol_common.py

🚧 Files skipped from review as they are similar to previous changes (1)

core/src/trezor/ui/init.py

mmilata

Thanks for making this limited in scope, it's still scary:) Have you tried running the tests on hardware? Might help discover potential timing issues, unless we want to merge this before freeze.

mmilata · 2026-04-27T17:25:31Z

+            has_br = self.put_button_request(self.layout.button_request())
+            if __debug__:
+                self._is_attached = not has_br
                notify_layout_change(self)


Why is it ok to call this if has_br is True? IIUC previously this was not the case.

Good catch - fixed in 92a5adc.

mmilata · 2026-04-27T17:27:00Z

        self.ctx = ctx
+        self.box: loop.mailbox[ButtonRequest | None] = loop.mailbox()
+        self.is_done: loop.mailbox[None] = loop.mailbox()
+        self.pending = False


Nit: consider adding short explanations of what each member variable means?

c4e1d20 (also moved self.pending under __debug__)

romanz · 2026-04-28T06:59:23Z

Ran device tests on T3T1 @ 374d32f:

$ TREZOR_MODEL=T3T1 PYOPT=0 QUIET_MODE=1 STORAGE_INSECURE_TESTING_MODE=1 BITCOIN_ONLY=0 \
  make -C core clean build_firmware
...
Apr 28 01:13:14 == 1658 passed, 233 skipped, 6 deselected, 25 xfailed in 13660.61s (3:47:40) ===

Also ran them on T3B1 @ c4e1d20

Apr 28 12:46:07 == 1627 passed, 264 skipped, 6 deselected, 25 xfailed in 10346.58s (2:52:26) ===

romanz · 2026-04-28T07:04:14Z

Let's merge it after the freeze - it's not needed for the upcoming release.

…Handler` Also, minimize context-related access in `ui.Layout`. [no changelog]

romanz self-assigned this Apr 9, 2026

romanz added code Code improvements translations Put this label on a PR to run tests in all languages labels Apr 9, 2026

trezor-bot Bot added this to Firmware Apr 9, 2026

github-project-automation Bot moved this to 🔎 Needs review in Firmware Apr 9, 2026

romanz marked this pull request as ready for review April 9, 2026 19:50

romanz requested a review from obrusvit as a code owner April 9, 2026 19:50

coderabbitai Bot reviewed Apr 9, 2026

View reviewed changes

romanz force-pushed the romanz/2604/br-rework branch from 204a167 to d6a847e Compare April 9, 2026 20:07

romanz moved this from 🔎 Needs review to 🏃‍♀️ In progress in Firmware Apr 9, 2026

romanz marked this pull request as draft April 10, 2026 05:13

romanz force-pushed the romanz/2604/br-rework branch 3 times, most recently from 7df7642 to 6dbb78e Compare April 10, 2026 14:12

romanz marked this pull request as ready for review April 10, 2026 14:13

coderabbitai Bot reviewed Apr 10, 2026

View reviewed changes

Comment thread core/src/trezor/wire/protocol_common.py

romanz changed the title ~~refactor(core): move ButtonRequest-related code into ButtonRequestHandler~~ refactor(core): move UI ButtonRequest-related code into ButtonRequestHandler Apr 10, 2026

romanz force-pushed the romanz/2604/br-rework branch from 6dbb78e to d0a935d Compare April 10, 2026 19:34

This comment was marked as duplicate.

Sign in to view

romanz marked this pull request as draft April 10, 2026 20:02

romanz force-pushed the romanz/2604/br-rework branch from d0a935d to 830ee66 Compare April 21, 2026 05:49

romanz marked this pull request as ready for review April 21, 2026 06:42

romanz force-pushed the romanz/2604/br-rework branch 2 times, most recently from c754fc8 to 374d32f Compare April 24, 2026 17:35

romanz mentioned this pull request Apr 24, 2026

fix(core): continue backup workflow even after I/O errors & timeouts #6651

Draft

1 task

romanz requested a review from mmilata April 27, 2026 08:02

romanz moved this from 🏃‍♀️ In progress to 🔎 Needs review in Firmware Apr 27, 2026

mmilata reviewed Apr 27, 2026

View reviewed changes

romanz requested a review from mmilata April 28, 2026 11:47

mmilata approved these changes Apr 28, 2026

View reviewed changes

romanz force-pushed the romanz/2604/br-rework branch from c4e1d20 to 80f484b Compare April 28, 2026 13:03

romanz added the no-QA On PR-merge, automatically transition status in the "Firmware" project to "Done (no QA)" state. label Apr 28, 2026

refactor(core): move ButtonRequest-related code into `ButtonRequest…

1a197d9

…Handler` Also, minimize context-related access in `ui.Layout`. [no changelog]

romanz force-pushed the romanz/2604/br-rework branch from 80f484b to 1a197d9 Compare April 28, 2026 13:25

romanz moved this from 🔎 Needs review to ✅ Done (no QA) in Firmware Apr 28, 2026

romanz merged commit 9d49d90 into main Apr 28, 2026
299 of 300 checks passed

romanz deleted the romanz/2604/br-rework branch April 28, 2026 15:16

Uh oh!

Conversation

romanz commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Sequence Diagram(s)

Estimated code review effort

❌ Failed checks (1 warning, 1 inconclusive)

Uh oh!

github-actions Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

en main(all)

cs main(all)

de main(all)

es main(all)

fr main(all)

pt main(all)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

mmilata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mmilata Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

romanz Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

mmilata Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

romanz Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

romanz commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romanz commented Apr 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

romanz commented Apr 9, 2026 •

edited

Loading

coderabbitai Bot commented Apr 9, 2026 •

edited

Loading

github-actions Bot commented Apr 9, 2026 •

edited

Loading

`en` main(all)

`cs` main(all)

`de` main(all)

`es` main(all)

`fr` main(all)

`pt` main(all)

romanz commented Apr 28, 2026 •

edited

Loading