Skip to content

Conversation

@guowei0105
Copy link
Contributor

@guowei0105 guowei0105 commented Sep 25, 2025

Summary by CodeRabbit

  • New Features

    • Added new prompts for confirming typed data, authorization warnings, review struct, and tips across EN, DE, ES, JA, KO, PT-BR, ZH-CN, and ZH-TW.
    • Updated Security menu label to “Security Keys.”
    • Device now auto-locks after certain PIN changes for enhanced security.
  • Bug Fixes

    • Clarified caution message and improved PIN instruction formatting with line breaks for better readability in multiple languages.

@coderabbitai
Copy link

coderabbitai bot commented Sep 25, 2025

Walkthrough

Updates increase I18N item count, add four i18n keys, and append new/modified translations across eight locales. Menu handling centralizes confirm-only key waits and renames a menu label. PIN change flow adds a post-change lock sequence when a passphrase PIN is removed.

Changes

Cohort / File(s) Summary
I18N constants & keys
legacy/firmware/i18n/i18n.h, legacy/firmware/i18n/keys.h
Incremented I18N_ITEMS_COUNT 423→427. Added four keys: T_CONFIRM_TYPED_DATA, I_TYPED_DATA_AUTHORIZATION_WARNING, I_REVIEW_STRUCT, C__TIPS_PRESS_BOTH_AT_ONCE_TO_SWITCH_CASE_NUMBERS_SYMBOLS.
Locale strings (EN/DE/ES/JA/KO/PT-BR/ZH-CN/ZH-TW)
legacy/firmware/i18n/locales/de.inc, .../en.inc, .../es.inc, .../ja.inc, .../ko_kr.inc, .../pt_br.inc, .../zh_cn.inc, .../zh_tw.inc
Modified one caution/PIN message in some locales. Appended four new strings per locale for typed data confirmation, authorization warning, struct review, and tips.
Menu interaction flow
legacy/firmware/menu_list.c
Added static helper wait_for_confirm_only(mode). Replaced direct protectWaitKey calls with helper. Changed menu label “FIDO Keys”→“Security Keys”. Adjusted key handling to return KEY_CONFIRM and propagate KEY_CANCEL/KEY_NULL.
PIN change lock flow
legacy/firmware/protect.c
In protectChangePinOnDevice, added lock_required path: after main PIN change that deletes a passphrase PIN, trigger session clear, return-to-home, cache clears, and ensure re-lock sequence.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant UI as UI/Menu
  participant Keys as KeyHandler
  participant Sec as SecurityFlow

  rect rgba(230,240,255,0.6)
    note over UI,Keys: Centralized confirm-only wait (new)
    User->>UI: Navigate menu
    UI->>Keys: wait_for_confirm_only(mode)
    alt Confirm
      Keys-->>UI: KEY_CONFIRM
      UI->>Sec: Proceed with selected action
    else Cancel
      Keys-->>UI: KEY_CANCEL
      UI-->>User: Exit/Back
    else No key
      Keys-->>UI: KEY_NULL
      UI-->>User: Keep waiting
    end
  end
Loading
sequenceDiagram
  autonumber
  actor User
  participant Dev as Device
  participant Prot as protectChangePinOnDevice
  participant Sess as SessionManager
  participant UI as Home

  User->>Prot: Change main PIN
  Prot->>Prot: Perform PIN change
  alt Passphrase PIN deleted
    Prot->>Prot: lock_required = true
    note right of Prot: New post-change lock sequence
    Prot->>Sess: clearSession()
    Prot->>UI: returnToHome()
    Prot->>Sess: clearPinStateCache()
    loop Ensure re-lock
      Prot->>Sess: verifyUnlockedOrPrompt()
      Prot->>Sess: clearInternalState()
    end
  else No passphrase PIN deletion
    Prot-->>User: Complete without extra locking
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title “fix: update fide i18n and fix issue” is overly generic, contains a typo, and fails to convey the full scope of this pull request; it only hints at I18N updates and makes no mention of the new helper function or lock flow enhancements. As written it does not clearly summarize key changes like the I18N_ITEMS_COUNT bump, the addition of new translation entries, or control-flow adjustments in menu_list.c and protect.c. This lack of precision prevents a quick understanding of the primary modifications. Please revise the title to correct “fide” to the intended term and succinctly summarize the main updates in this PR. For example, mention the I18N_ITEMS_COUNT bump, new localization keys, and control-flow changes such as the wait_for_confirm_only helper and lock logic. A title like “feat: bump I18N count, add new translations, and centralize confirm key handling” would clearly communicate the scope of these changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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

@revan-zhang
Copy link
Contributor

revan-zhang commented Sep 25, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
legacy/firmware/menu_list.c (1)

1194-1230: Introduce standard wait helper only if protectAbortedByCancel is visible

wait_for_confirm_only() reads protectAbortedByCancel, but this file never included that symbol before. Please confirm that protectAbortedByCancel is declared in a header visible here; otherwise this will not compile. If it isn’t, add the proper extern declaration (ideally in protect.h) or rework the helper to avoid the dependency.

📜 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

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between d117f32 and 9139e60.

📒 Files selected for processing (12)
  • legacy/firmware/i18n/i18n.h (1 hunks)
  • legacy/firmware/i18n/keys.h (1 hunks)
  • legacy/firmware/i18n/locales/de.inc (2 hunks)
  • legacy/firmware/i18n/locales/en.inc (2 hunks)
  • legacy/firmware/i18n/locales/es.inc (1 hunks)
  • legacy/firmware/i18n/locales/ja.inc (1 hunks)
  • legacy/firmware/i18n/locales/ko_kr.inc (2 hunks)
  • legacy/firmware/i18n/locales/pt_br.inc (2 hunks)
  • legacy/firmware/i18n/locales/zh_cn.inc (1 hunks)
  • legacy/firmware/i18n/locales/zh_tw.inc (1 hunks)
  • legacy/firmware/menu_list.c (5 hunks)
  • legacy/firmware/protect.c (4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Style check
  • GitHub Check: Gen check
  • GitHub Check: Defs check
🔇 Additional comments (12)
legacy/firmware/i18n/locales/zh_cn.inc (1)

425-428: New strings align with new keys.

Appends 4 items for the new keys at the end. Good placement to preserve indices.

legacy/firmware/i18n/locales/ja.inc (1)

425-428: JP locale updated with 4 new entries.

Appended, preserving index alignment. Looks good.

legacy/firmware/i18n/locales/ko_kr.inc (2)

260-260: PIN hint formatting changed to multiline.

Text-only change; index preserved. Confirm it renders well on the target display.


425-428: KR locale: 4 appended entries.

Placement and content align with the new keys.

legacy/firmware/i18n/locales/de.inc (2)

261-261: German PIN hint formatting adjusted.

Multiline phrasing is fine; index kept. Please sanity-check on device for line wrapping.


425-428: DE locale: 4 appended entries.

End-append preserves stable indices. Good.

legacy/firmware/i18n/locales/pt_br.inc (2)

136-136: PT-BR “0 attempts” message reflowed.

Text-only tweak. Ensure the line breaks fit the screen width.


425-428: PT-BR: 4 new strings appended.

Matches new keys; ordering is correct.

legacy/firmware/protect.c (1)

1225-1496: Post-change lock flow added; behavior looks correct.

lock_required is set when a main PIN change deletes the current passphrase PIN, then forces a re-lock and cache/state clears after the user acknowledges success. This reduces exposure. Good addition.

Please verify these cases on-device:

  • Starting in USER PIN session, set new main PIN equal to an existing passphrase PIN. Confirm: passphrase PIN deleted, success dialog shown, device locks after confirm.
  • Starting in PASSPHRASE PIN session, set a new passphrase PIN equal to USER PIN. Confirm the overwrite warning path and that no unintended lock occurs.
  • If se_delete_pin_passphrase succeeds but the subsequent config_changePin fails, confirm the device remains in a safe state (no hidden session lingering). If needed, consider locking immediately upon deleting a current passphrase PIN even when the change fails.
legacy/firmware/i18n/i18n.h (1)

8-8: I18N_ITEMS_COUNT and locales sizes validated
I18N_ITEMS_COUNT = 427 (max key 426 + 1). All locale arrays contain 427 entries.

legacy/firmware/menu_list.c (1)

755-756: Rename menu item consistently with i18n keys

The menu label changed from “FIDO Keys” to “Security Keys”. Make sure the corresponding localization entries (and any key lookups that expect “FIDO Keys”) are updated across locales so the UI stays consistent. If everything already lines up, feel free to disregard.

legacy/firmware/i18n/locales/zh_tw.inc (1)

425-428: Polish zh-TW copy for clarity and consistency

  • “確認結構數據” → “確認結構化數據”
  • “{} 授權。請先檢查 dApp 信任度。” → “為 {} 授權。請先檢查 dApp 的信任度。”
  • “來切換” → “以切換”

All locale files still have 427 entries (I18N_ITEMS_COUNT).

@guowei0105 guowei0105 merged commit 0f2d652 into OneKeyHQ:master Sep 25, 2025
6 of 8 checks passed
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.

4 participants