Preparation for N4W1

[romanz]

The commits below should not change current FW behavior.

N4W1 support will be introduced in #6799.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: da6953b3-a0b5-4c97-ac25-102219d6666a

📥 Commits

Reviewing files that changed from the base of the PR and between ac4c601 and 1df8713.

📒 Files selected for processing (1)

• core/src/apps/debug/n4w1_mock.py

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

• core/src/apps/debug/n4w1_mock.py

---

Walkthrough

This pull request refactors recovery device handler logic and enhances debug and layout systems. The recovery handler implementation is moved from homescreen.py to layout.py, with homescreen.py now delegating handler selection via layout.choose_handler(). In n4w1_mock.py, two new methods are added to N4W1Context: an async connect() coroutine awaiting connection notification and a confirm_connect() method that builds a UI layout running connect() as an async task. The reset device layout extracts the per-share confirmation loop into a dedicated _backup_share() method. Menu interaction functions gain an optional layout_type parameter for custom layout type specification.

Sequence Diagram

sequenceDiagram
    participant N4W1Ctx as N4W1Context
    participant Rx as rx Channel
    participant Layout as UI Layout
    participant Menu as confirm_with_menu
    
    N4W1Ctx->>N4W1Ctx: confirm_connect() called
    N4W1Ctx->>Layout: Create Layout with async task
    Layout->>N4W1Ctx: Run connect() task
    N4W1Ctx->>Rx: await first value
    Rx->>N4W1Ctx: DebugLinkN4W1Response(value=None)
    N4W1Ctx->>N4W1Ctx: Assert connection notification
    N4W1Ctx->>Layout: Emit CONFIRMED message
    Layout->>Menu: Run confirm_with_menu with Layout
    Menu->>Layout: Return with external menu
    Layout->>N4W1Ctx: Complete

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is minimal and lacks required template sections. It does not follow the provided description template with sections for PR setup, development status, or post-merge status guidance.	Add missing template sections including assignment, project configuration, development status, and QA/testing notes as specified in the repository's description template.
Docstring Coverage	⚠️ Warning	Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Preparation for N4W1' is vague and does not clearly describe the specific changes made. While it references the overall theme, it provides no meaningful information about what was actually changed in the codebase.	Use a more specific title that indicates key changes, such as 'Refactor recovery layouts and add N4W1 connection support' or describe the primary technical changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
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/n4w1-prep

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

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: 24764937635

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In @.github/workflows/core.yml:
- Line 130: Add the N4W1 environment variable to the core_firmware and
core_emu_arm job definitions using the same conditional expression used in
core_emu: set N4W1: ${{ matrix.type == 'debuglink' && '1' || '0' }} so the build
system (SConscript.firmware and SConscript.unix) receives the flag when
PYOPT=='0' and debuglink variants exercise N4W1 plumbing; mirror the exact
expression already present in the core_emu job.

In `@core/src/apps/management/recovery_device/layout.py`:
- Around line 281-289: The debug log in choose_handler currently warns for any
method not equal to BackupMethod.Display, which incorrectly flags method None
(used by recovery_homescreen -> recovery_process) as unsupported; update the
conditional in choose_handler to only warn when __debug__ is true and method is
not None and method is not BackupMethod.Display (put __debug__ first to
short-circuit in non-debug builds) so None is treated as the valid sentinel and
no warning is emitted; keep returning _DisplayHandler unchanged.

🪄 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: 0b690685-3495-4fb5-a919-e62ace8d98e8

📥 Commits

Reviewing files that changed from the base of the PR and between 9687d46 and ac4c601.

📒 Files selected for processing (6)

• .github/workflows/core.yml
• core/src/apps/debug/n4w1_mock.py
• core/src/apps/management/recovery_device/homescreen.py
• core/src/apps/management/recovery_device/layout.py
• core/src/apps/management/reset_device/layout.py
• core/src/trezor/ui/layouts/menu.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Suppress the unsupported-method warning for method is None as well.

recovery_homescreen() calls recovery_process(None), so the common recovery path reaches choose_handler(None). With the current condition, debug builds will log "Unsupported backup method: None" on every ordinary recovery, even though None is the sentinel reserved for the future interactive-selection flow (analogous to choose_backup_method() in reset_device/layout.py) and is not an "unsupported" value.

Based on learnings: "method=None passed to choose_backup_method() is intentionally distinct from BackupMethod.Display. … It should NOT be treated as equivalent to BackupMethod.Display, and debug-mode unsupported-method warnings should be suppressed for None (as well as for BackupMethod.Display)."

🛠 Proposed fix

 async def choose_handler(method: BackupMethod | None) -> type[RecoveryHandler]:
     from trezor.enums import BackupMethod
 
-    if method is not BackupMethod.Display and __debug__:
+    if __debug__ and method is not None and method is not BackupMethod.Display:
         from trezor import log
 
         log.warning(__name__, "Unsupported backup method: %s", method)
 
     return _DisplayHandler

(Also slightly cheaper: __debug__ is a compile-time constant, so placing it first lets the rest short-circuit out entirely in non-debug builds.)

🤖 Prompt for AI Agents

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

In `@core/src/apps/management/recovery_device/layout.py` around lines 281 - 289,
The debug log in choose_handler currently warns for any method not equal to
BackupMethod.Display, which incorrectly flags method None (used by
recovery_homescreen -> recovery_process) as unsupported; update the conditional
in choose_handler to only warn when __debug__ is true and method is not None and
method is not BackupMethod.Display (put __debug__ first to short-circuit in
non-debug builds) so None is treated as the valid sentinel and no warning is
emitted; keep returning _DisplayHandler unchanged.