Skip to content

fix(legacy): fix the display issue and respond to the client side can…#145

Merged
somebodyLi merged 1 commit intomasterfrom
3.12.0
Jun 4, 2025
Merged

fix(legacy): fix the display issue and respond to the client side can…#145
somebodyLi merged 1 commit intomasterfrom
3.12.0

Conversation

@somebodyLi
Copy link
Contributor

@somebodyLi somebodyLi commented Jun 4, 2025

…cellation

Summary by CodeRabbit

  • Bug Fixes
    • Improved key handling to ensure safer and more consistent behavior during user input, including correct handling of abort scenarios and fixing missing semicolons.
  • New Features
    • Introduced a unified macro for handling key input with automatic abort detection, streamlining user interactions across various dialogs and confirmation screens.
  • Chores
    • Removed an unused variable and made minor code cleanups for improved maintainability.

@coderabbitai
Copy link

coderabbitai bot commented Jun 4, 2025

Walkthrough

The updates introduce safer macro usage for key handling. They add early exit checks for abort conditions in macros. Key input handling now uses a unified macro across the codebase. Some minor code cleanups and missing semicolons are fixed. No changes affect public interfaces.

Changes

File(s) Change Summary
legacy/buttons.h Updated HANDLE_KEY macro: added do { ... } while (0), early abort check, explicit return.
legacy/firmware/protect.h Added new macro WAIT_KEY_OR_ABORT(timeout, mode, key) for unified key input and abort handling.
legacy/firmware/layout2.c Replaced direct protectWaitKey calls with WAIT_KEY_OR_ABORT. Removed unused variable. Fixed string draw and added missing semicolons.
legacy/firmware/tron.c Added missing semicolons after HANDLE_KEY macro calls in two functions.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant UI_Function
    participant WAIT_KEY_OR_ABORT
    participant protectWaitKey
    participant System

    User->>UI_Function: Triggers action needing input
    UI_Function->>WAIT_KEY_OR_ABORT: Wait for key input
    WAIT_KEY_OR_ABORT->>protectWaitKey: Get key input
    protectWaitKey-->>WAIT_KEY_OR_ABORT: Return key
    WAIT_KEY_OR_ABORT->>UI_Function: If abort flag set, return false
    WAIT_KEY_OR_ABORT->>UI_Function: Else, continue with key
    UI_Function->>System: Continue processing or abort
Loading

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 9fdd976 and 58e979d.

📒 Files selected for processing (4)
  • legacy/buttons.h (1 hunks)
  • legacy/firmware/layout2.c (15 hunks)
  • legacy/firmware/protect.h (1 hunks)
  • legacy/firmware/tron.c (2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
legacy/firmware/layout2.c (2)
legacy/firmware/oled_text.c (1)
  • oledDrawStringAdapter (290-294)
legacy/firmware/oled_text.h (1)
  • oledDrawStringAdapter (22-22)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Style check
  • GitHub Check: Defs check
  • GitHub Check: Gen check
🔇 Additional comments (17)
legacy/firmware/protect.h (1)

74-80: Clean macro implementation with proper abort handling.

The do-while(0) pattern ensures safe usage in conditional statements. The abort condition checks are logical and follow the established pattern.

legacy/firmware/tron.c (3)

1006-1006: Good syntax fix for macro compatibility.

The semicolon correctly terminates the macro statement.


1049-1049: Good syntax fix for macro compatibility.

The semicolon correctly terminates the macro statement.


933-1007:

Details

❌ Incorrect review comment

Verify complete function implementations.

Both functions call HANDLE_KEY but appear to lack the switch statement logic to process the key input and return appropriate values. The functions should handle the key response and return a boolean result.

Also applies to: 1009-1050


🏁 Script executed:

#!/bin/bash
# Description: Check for complete implementations of these functions and similar patterns
# Expected: Find switch statements or key handling logic after HANDLE_KEY calls

echo "Searching for complete function implementations with HANDLE_KEY patterns:"
rg -A 10 -B 5 "HANDLE_KEY.*;" --type c

echo -e "\nSearching for switch statements after HANDLE_KEY in similar functions:"
rg -A 15 "HANDLE_KEY.*switch" --type c

Length of output: 7032


I’ll check how HANDLE_KEY is defined to see if it covers the switch logic.


🏁 Script executed:

#!/bin/bash
# Locate HANDLE_KEY macro definition and surrounding lines
grep -R -n -C3 "#define HANDLE_KEY" .

Length of output: 654


I need to see the full HANDLE_KEY macro to confirm where the switch logic lives.


🏁 Script executed:

#!/bin/bash
# Show the full HANDLE_KEY definition from buttons.h
sed -n '60,120p' legacy/buttons.h

Length of output: 2484


No extra key‐handling code needed: HANDLE_KEY covers the switch and return logic.

The HANDLE_KEY macro in legacy/buttons.h already implements the switch(key) cases, index changes, and final return result;. All layout functions calling HANDLE_KEY(bubble_key) are complete. Please disregard the request to add a separate switch statement.

Likely an incorrect or invalid review comment.

legacy/firmware/layout2.c (10)

1023-1023: Good use of unified key handling macro.

The replacement of protectWaitKey(0, 0) with WAIT_KEY_OR_ABORT(0, 0, key) improves safety by automatically checking abort conditions.


1216-1216: Direct string drawing optimizes fee label display.

This direct call to oledDrawStringAdapter replaces string concatenation, making the fee display more efficient.


1225-1225: Consistent use of abort-aware key handling.

Another proper replacement with the WAIT_KEY_OR_ABORT macro for safer key input handling.


1567-1567: Macro usage maintains abort safety in address layout.

Good replacement that ensures abort conditions are checked during key waiting in address display functions.


1754-1754: XPUB layout now uses safe key handling.

The macro replacement ensures consistent abort behavior across all key input scenarios.


4105-4105: Missing semicolon added to HANDLE_KEY macro.

The semicolon makes the macro invocation syntactically correct and consistent with similar usage throughout the codebase.


4246-4246: Transaction signing uses unified key handling.

Both EVM transaction signing functions now properly use the abort-aware key handling macro.

Also applies to: 4316-4316


4354-4354: Semicolon fixes HANDLE_KEY syntax.

This completes the syntax correction for the macro invocation.


4566-4566: Message and hash signing functions updated.

All message signing, hash signing, and encryption functions now use the safer WAIT_KEY_OR_ABORT macro consistently.

Also applies to: 4924-4924, 5044-5044, 5117-5117


4669-4670: Local key variables for abort-aware handling.

The local key variable declarations enable proper use of the WAIT_KEY_OR_ABORT macro in message signing functions.

Also applies to: 4693-4694

legacy/buttons.h (3)

72-73: Good practice: do-while wrapper enhances macro safety.

The do-while wrapper prevents macro expansion issues in conditional statements and other contexts. This is a standard best practice for multi-statement macros.

Also applies to: 105-105


76-78:

Details

✅ Verification successful

Abort handling aligns with PR objectives.

The early exit check for protectAbortedByInitialize and protectAbortedByCancel properly addresses client-side cancellation as stated in the PR objectives.

Verify that the abort variables are properly declared and accessible:


🏁 Script executed:

#!/bin/bash
# Check if abort variables are declared in the codebase
rg -A 2 "protectAbortedByInitialize|protectAbortedByCancel" --type c --type h

Length of output: 8106


Abort flags are declared and defined

  • Found extern bool protectAbortedByInitialize and protectAbortedByCancel in legacy/firmware/protect.h.
  • Confirmed their definitions in legacy/firmware/protect.c.

Everything compiles and aligns with the PR objectives. Approving changes.


74-75:

Details

✅ Verification successful

Verify macro dependencies are available.

The macro uses several variables (max_index, index, result) and function (protectWaitKey) that must be in scope. Also uses goto refresh_menu requiring a label.

Check that macro dependencies exist where used:

Also applies to: 79-104


🏁 Script executed:

#!/bin/bash
# Find usage of HANDLE_KEY macro and verify context
rg -A 10 -B 5 "HANDLE_KEY" --type c --type h

# Check for required variables and functions
rg "max_index|refresh_menu|protectWaitKey" --type c --type h

Length of output: 27514


🏁 Script executed:

#!/bin/bash
# Check that global abort flags are defined
rg "protectAbortedByInitialize" --type c --type h
rg "protectAbortedByCancel" --type c --type h

# Confirm the refresh_menu label exists in neo.c
rg -n "refresh_menu:" legacy/firmware/neo.c

Length of output: 5298


HANDLE_KEY macro dependencies are satisfied. protectWaitKey and the abort flags are declared. Every caller defines max_index, index, result (and bubble_key) and provides a refresh_menu label. No action required.

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary or Summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@somebodyLi somebodyLi merged commit 6a500f4 into master Jun 4, 2025
6 of 8 checks passed
@somebodyLi somebodyLi deleted the 3.12.0 branch June 4, 2025 11:26
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.

3 participants