Skip to content

Conversation

@ByteZhang1024
Copy link
Contributor

@ByteZhang1024 ByteZhang1024 commented Nov 14, 2025

Summary by CodeRabbit

  • Refactor

    • Improved firmware update flow to tolerate missing device identifiers in update scenarios, reducing interruptions during firmware checks.
    • Adjusted compatibility lookup to use a dedicated firmware-update context for more accurate compatibility handling.
  • New Features

    • Added a specific firmware-update context to better support update-specific device interactions.

@revan-zhang
Copy link
Contributor

revan-zhang commented Nov 14, 2025

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

Status Scanner Critical High Medium Low Total (0)
Licenses 0 0 0 0 0 issues
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 14, 2025

Walkthrough

Adds an UPDATE_FIRMWARE hardware context, relaxes connectId validation under that context, and updates firmware update code to accept an optional allowEmptyConnectId flag. The firmware changelog page now requests compatibility using UPDATE_FIRMWARE.

Changes

Cohort / File(s) Change Summary
Type definitions
packages/shared/types/device.ts
Added UPDATE_FIRMWARE = 'update_firmware' member to EHardwareCallContext enum.
Firmware update service
packages/kit-bg/src/services/ServiceFirmwareUpdate/ServiceFirmwareUpdate.ts
Updated checkDeviceIsBootloaderMode signature to accept object destructuring and optional allowEmptyConnectId; forwards allowEmptyConnectId into getFeaturesWithoutCache.
Hardware service
packages/kit-bg/src/services/ServiceHardware/ServiceHardware.ts
Relaxed connectId validation in getCompatibleConnectId to return empty string (instead of throwing) when hardwareCallContext is UPDATE_FIRMWARE and no connectId/device/features are present.
Firmware update UI
packages/kit/src/views/FirmwareUpdate/pages/PageFirmwareUpdateChangeLog.tsx
Changed hardwareCallContext passed to getCompatibleConnectId from USER_INTERACTION to UPDATE_FIRMWARE.

Sequence Diagram(s)

sequenceDiagram
    participant UI as PageFirmwareUpdateChangeLog
    participant HW as ServiceHardware
    participant FW as ServiceFirmwareUpdate

    rect rgb(230,240,255)
    note right of UI: Start firmware compatibility check
    UI->>HW: getCompatibleConnectId(hardwareCallContext=UPDATE_FIRMWARE)
    alt no connectId/device/features & UPDATE_FIRMWARE
        HW-->>UI: return "" (allow empty)
    else has connectId or other contexts
        HW-->>UI: return connectId or throw
    end
    end

    rect rgb(240,255,230)
    note right of FW: Check bootloader with optional empty id
    UI->>FW: checkDeviceIsBootloaderMode({connectId?, allowEmptyConnectId: true})
    FW->>FW: getFeaturesWithoutCache({connectId?, allowEmptyConnectId: true})
    FW-->>UI: bootloader status
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review ServiceHardware.getCompatibleConnectId guard logic to ensure non-UPDATE_FIRMWARE contexts still enforce validation.
  • Verify all call sites for checkDeviceIsBootloaderMode match the new signature and correctly pass allowEmptyConnectId when intended.
  • Confirm the new enum member is handled where EHardwareCallContext is switch/conditional based.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
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.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly references the specific issue (OK-45857) and indicates the fix targets boot model update errors, directly relating to the changeset's purpose.
Linked Issues check ✅ Passed The changes enable empty connectId handling in firmware update contexts by adding UPDATE_FIRMWARE enum, relaxing validation in getCompatibleConnectId, and propagating allowEmptyConnectId parameter, addressing the boot mode connection error.
Out of Scope Changes check ✅ Passed All changes directly support the boot mode firmware update fix: enum addition, validation relaxation, context parameter passing, and method signature updates stay focused on resolving the connectId error.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/bootModleUpdate

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

@ByteZhang1024 ByteZhang1024 enabled auto-merge (squash) November 14, 2025 07:53
Copy link
Contributor

@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: 1

📜 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 8dcc14a and 45f5d8e.

📒 Files selected for processing (4)
  • packages/kit-bg/src/services/ServiceFirmwareUpdate/ServiceFirmwareUpdate.ts (3 hunks)
  • packages/kit-bg/src/services/ServiceHardware/ServiceHardware.ts (1 hunks)
  • packages/kit/src/views/FirmwareUpdate/pages/PageFirmwareUpdateChangeLog.tsx (1 hunks)
  • packages/shared/types/device.ts (1 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). (2)
  • GitHub Check: unittest (20.x)
  • GitHub Check: lint (20.x)
🔇 Additional comments (4)
packages/shared/types/device.ts (1)

404-404: LGTM! Enum addition aligns with PR objective.

The new UPDATE_FIRMWARE context is clear and follows the existing naming pattern.

packages/kit-bg/src/services/ServiceFirmwareUpdate/ServiceFirmwareUpdate.ts (2)

160-165: LGTM! Parameter addition supports boot mode firmware checks.

The optional allowEmptyConnectId parameter enables firmware checks when connectId is unavailable during boot mode.


387-390: Code is correct—boot mode scenario properly uses allowEmptyConnectId: true.

The pattern is intentional. The method checkDeviceIsBootloaderMode accepts allowEmptyConnectId as an optional parameter that relaxes validation rules. Line 388 correctly applies this flag in a bootloader/recovery scenario where connectId may be empty, while line 307 omits it in normal firmware checks where connectId is guaranteed valid. The parameter flows through to downstream hardware service calls as designed.

Note: The original review references "checkAllFirmwareRelease," but the actual method called is checkDeviceIsBootloaderMode.

packages/kit/src/views/FirmwareUpdate/pages/PageFirmwareUpdateChangeLog.tsx (1)

66-66: Verified: Context change correctly enables boot mode firmware updates.

The change from USER_INTERACTION to UPDATE_FIRMWARE is correct. The getCompatibleConnectId method in ServiceHardware.ts (lines 1622–1632) explicitly allows null connectId only for UPDATE_FIRMWARE context, which is exactly what the firmware update flow needs for boot mode.

Comment on lines +1624 to +1626
[EHardwareCallContext.UPDATE_FIRMWARE].includes(
hardwareCallContext || EHardwareCallContext.USER_INTERACTION,
) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

这段代码写的很啰嗦?应该是 === 就行 ?

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.

5 participants