Repository navigation
Replace qmicli IMS probing with native QMI detection - #17
Conversation
📝 WalkthroughWalkthroughThe IMS implementation now uses native QMI through ChangesNative IMS diagnostics
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ModemService
participant NativeImsProbe
participant QmiProxy
participant ModemPage
ModemService->>NativeImsProbe: probe modem IMS data
NativeImsProbe->>QmiProxy: send native QMI requests
QmiProxy-->>NativeImsProbe: return capability and runtime TLVs
NativeImsProbe-->>ModemService: return classified voice and SMS status
ModemPage->>ModemService: request authenticated modem status
ModemService-->>ModemPage: return translated IMS diagnostics data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/modem/ims.rs (4)
673-694: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the probe error paths.
The new tests cover successful responses only. Three error paths carry user-visible diagnostics and have no test:
NativeQmiError::Qmi(_)from a response whose result TLV reports a failure. That path drivesnative_qmi_probe_failed.NativeQmiError::PermissionDeniedandNativeQmiError::ProxyUnavailablefrommap_proxy_io_error. Those driveims_probe_permission_deniedandqmi_proxy_unavailable.- The early return at lines 357-360 when the version query fails, which must leave
probe.availablefalse.
FakeNativeQmiClientalready makes the first case cheap: return a frame with a non-zerofailedfield in the result TLV.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modem/ims.rs` around lines 673 - 694, The NativeImsProbe tests cover only successful responses and omit user-visible error paths. Extend the test coverage around NativeImsProbe and FakeNativeQmiClient for a non-zero result-TLV failure producing NativeQmiError::Qmi(_) and native_qmi_probe_failed, map_proxy_io_error cases producing PermissionDenied and ProxyUnavailable with their corresponding diagnostics, and a failed version query that returns early while leaving probe.available false.
886-890: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the test socket cleanup panic-safe.
Line 890 creates the socket file. Line 952 removes it. If any assertion between those lines fails, or if
server.await.unwrap()panics because a server-side assertion failed,remove_filenever runs and the socket file stays in/tmp.Bind the socket inside a temporary directory that removes itself on drop, or wrap the path in a guard type with a
Dropimpl.Also applies to: 951-952
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modem/ims.rs` around lines 886 - 890, Make the test socket lifecycle around the UnixListener binding and cleanup panic-safe by placing the socket path in a temporary directory with automatic removal, or by introducing a Drop-based cleanup guard. Ensure cleanup still occurs when assertions or server.await.unwrap() panic, and remove the reliance on the explicit remove_file call near the listener teardown.
374-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRecord a diagnostic when the NAS query is absent or fails.
Line 378 discards a NAS error with
if let Ok(response) = nas. Line 374 also skips the query silently whenversions.nasis false. In both caseslte_voice_supportandims_voice_supportstayNone, and the panel renders them as "Unknown" with no diagnostic line.Every other query in this function records a blocker or a warning: IMSA at lines 415-418 and 443-444, IMS at lines 483-490. Add the same treatment for NAS so the two voice-capability fields have an explanation when they are unknown.
New codes also need entries in
imsDiagnosticMessageinfrontend/src/components/modem/modem-status-panel.tsxand in all six locale files.♻️ Proposed change
if versions.nas { let nas = self .request_response(&device, QMI_SERVICE_NAS, QMI_NAS_GET_SYSTEM_INFO, deadline) .await; - if let Ok(response) = nas { - result.lte_voice_support = response.bool(0x21); - result.ims_voice_support = response.bool(0x29); - if result.ims_voice_support == Some(true) { - result - .evidence - .push("qmi_nas_ims_voice_support".to_string()); + match nas { + Ok(response) => { + result.lte_voice_support = response.bool(0x21); + result.ims_voice_support = response.bool(0x29); + if result.ims_voice_support == Some(true) { + result + .evidence + .push("qmi_nas_ims_voice_support".to_string()); + } } + Err(error) => result.warnings.push(native_query_error_code( + error, + "ims_voice_support_query_failed", + )), } + } else { + result + .warnings + .push("ims_voice_support_query_unavailable".to_string()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modem/ims.rs` around lines 374 - 387, Update the NAS handling in the function containing the versions.nas query to record a diagnostic when the NAS service is unavailable and when request_response fails, while preserving the existing capability parsing on success. Add distinct diagnostic codes/messages using the established blocker or warning conventions, map both codes in imsDiagnosticMessage, and provide translations for each in all six locale files.
290-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
Defaultimpl forNativeImsProbe.
NativeImsProbe::new()is public and takes no arguments, which triggersclippy::new_without_default. Addimpl Default for NativeImsProbe { fn default() -> Self { Self::new() } }so the public constructor remains usable without a warning in CI configurations that treat clippy warnings as errors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modem/ims.rs` around lines 290 - 293, Add an implementation of Default for NativeImsProbe that returns Self::new(), while preserving the existing public new constructor and its behavior.frontend/src/components/modem/modem-status-panel.tsx (2)
524-526: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude the unmapped code in the fallback text.
The fallback returns a generic message and discards
code. If the backend adds a diagnostic code before the frontend map is updated, the user and support see no identifier for the condition.Append the raw code so an unmapped diagnostic stays traceable.
♻️ Proposed change
return code in keys ? t(keys[code as keyof typeof keys]) - : t("modem.imsDiagnostics.fallback"); + : `${t("modem.imsDiagnostics.fallback")} (${code})`;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/modem/modem-status-panel.tsx` around lines 524 - 526, Update the fallback branch in the diagnostic translation logic to append the raw code to the existing fallback message, while leaving mapped-code translation unchanged. Use the `code` value in the `t("modem.imsDiagnostics.fallback")` path so unmapped diagnostics remain identifiable.
464-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
formatImsEnumparameter type so missing translations fail at compile time.The parameter is
value: stringand the fallback at line 477 returns the raw value. If a future backend enum variant reaches this function without a key, the panel renders a raw snake_case code such asnot_registeredto the user, and TypeScript reports nothing.Type the parameter as the union of the enums that are actually passed. Then a new variant without a key becomes a compile error.
Also note that
not_availableat line 472 is unreachable. No backend enum serializes to that value;ImsServiceStatusinsrc/modem/ims.rsusesunavailable. The matchingenum.notAvailablekey is dead in all six locale files.♻️ Proposed change
-function formatImsEnum(value: string, t: TFunction) { +type ImsEnumValue = + | SmsOverIms["status"] + | SmsOverIms["voice_over_ims"] + | SmsOverIms["registration"] + | SmsOverIms["voice_service"] + | SmsOverIms["configured"]; + +function formatImsEnum(value: ImsEnumValue, t: TFunction) { const keys = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/modem/modem-status-panel.tsx` around lines 464 - 478, Update formatImsEnum to accept the concrete union type of backend IMS enum values passed by its callers instead of string, so adding an unmapped variant produces a compile-time error. Remove the unreachable not_available key and its corresponding unused enum.notAvailable translations, while retaining unavailable and the existing mapped translation behavior.src/modem/ims/qmi.rs (1)
137-142: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not let the CID release response mask a valid service response.
Line 137 uses
?on the release response. If the release read fails or the proxy returns an error forQMI_CTL_RELEASE_CID, the function returnsProtocoland discards the service response that was already read at line 124. The release is best-effort cleanup: the stream is per-request and is closed when it drops.Resolve the service response first, then treat the release result as advisory.
♻️ Proposed change to prioritize the service response
- let release_response = read_qmi_frame(&mut stream).await?; - QmiResponse::parse(&release_response, QMI_SERVICE_CTL, QMI_CTL_RELEASE_CID)?; - - let response = service_response?; + let release_outcome = read_qmi_frame(&mut stream) + .await + .and_then(|raw| { + QmiResponse::parse(&raw, QMI_SERVICE_CTL, QMI_CTL_RELEASE_CID).map(|_| ()) + }); + + let response = service_response?; + release_outcome?; QmiResponse::parse(&response, service, message)?; Ok(response)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modem/ims/qmi.rs` around lines 137 - 142, Update the cleanup flow around the service_response resolution and QMI_CTL_RELEASE_CID parsing so the service response is resolved and returned first; perform CID release only as best-effort advisory cleanup without allowing release read or parse errors to override a valid service response. Preserve the existing service-response error propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/components/modem/modem-status-panel.tsx`:
- Around line 362-368: De-duplicate the combined diagnostics before rendering in
the diagnostics panel, since identical codes from reasons and warnings can
produce duplicate React keys. Update the diagnostics rendering around the
diagnostics.map call to iterate over unique codes while preserving the existing
imsDiagnosticMessage translation and display behavior.
In `@frontend/src/locales/es.ts`:
- Around line 72-73: The IMS card descriptions still use the outdated generic
route wording. Update the description entries in frontend/src/locales/es.ts
lines 72-73, frontend/src/locales/fr.ts lines 72-73, frontend/src/locales/ja.ts
lines 72-73, and frontend/src/locales/ko.ts lines 72-73 with localized text
explaining that VoLTE or VoWiFi is active only when the modem reports IMS
registration, voice service, and access technology, matching the updated English
and Chinese meaning.
---
Nitpick comments:
In `@frontend/src/components/modem/modem-status-panel.tsx`:
- Around line 524-526: Update the fallback branch in the diagnostic translation
logic to append the raw code to the existing fallback message, while leaving
mapped-code translation unchanged. Use the `code` value in the
`t("modem.imsDiagnostics.fallback")` path so unmapped diagnostics remain
identifiable.
- Around line 464-478: Update formatImsEnum to accept the concrete union type of
backend IMS enum values passed by its callers instead of string, so adding an
unmapped variant produces a compile-time error. Remove the unreachable
not_available key and its corresponding unused enum.notAvailable translations,
while retaining unavailable and the existing mapped translation behavior.
In `@src/modem/ims.rs`:
- Around line 673-694: The NativeImsProbe tests cover only successful responses
and omit user-visible error paths. Extend the test coverage around
NativeImsProbe and FakeNativeQmiClient for a non-zero result-TLV failure
producing NativeQmiError::Qmi(_) and native_qmi_probe_failed, map_proxy_io_error
cases producing PermissionDenied and ProxyUnavailable with their corresponding
diagnostics, and a failed version query that returns early while leaving
probe.available false.
- Around line 886-890: Make the test socket lifecycle around the UnixListener
binding and cleanup panic-safe by placing the socket path in a temporary
directory with automatic removal, or by introducing a Drop-based cleanup guard.
Ensure cleanup still occurs when assertions or server.await.unwrap() panic, and
remove the reliance on the explicit remove_file call near the listener teardown.
- Around line 374-387: Update the NAS handling in the function containing the
versions.nas query to record a diagnostic when the NAS service is unavailable
and when request_response fails, while preserving the existing capability
parsing on success. Add distinct diagnostic codes/messages using the established
blocker or warning conventions, map both codes in imsDiagnosticMessage, and
provide translations for each in all six locale files.
- Around line 290-293: Add an implementation of Default for NativeImsProbe that
returns Self::new(), while preserving the existing public new constructor and
its behavior.
In `@src/modem/ims/qmi.rs`:
- Around line 137-142: Update the cleanup flow around the service_response
resolution and QMI_CTL_RELEASE_CID parsing so the service response is resolved
and returned first; perform CID release only as best-effort advisory cleanup
without allowing release read or parse errors to override a valid service
response. Preserve the existing service-response error propagation.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c811d376-5608-4549-b74a-5564a2d5904e
📒 Files selected for processing (17)
README.mddocs/operations/sms-over-ims.mdfrontend/src/components/modem/modem-status-panel.tsxfrontend/src/lib/modem-api.tsfrontend/src/locales/en.tsfrontend/src/locales/es.tsfrontend/src/locales/fr.tsfrontend/src/locales/ja.tsfrontend/src/locales/ko.tsfrontend/src/locales/zh-CN.tsfrontend/src/modem-status-panel.test.tsxinstall.shscripts/install-private-qmicli-debian.shsrc/modem.rssrc/modem/ims.rssrc/modem/ims/qmi.rstests/install-private-qmicli.sh
💤 Files with no reviewable changes (3)
- tests/install-private-qmicli.sh
- install.sh
- scripts/install-private-qmicli-debian.sh
frankwei98
changed the title
1 participant
Summary
Review follow-up
Testing
cargo fmt --checkcargo test --locked— 304 passedcargo clippy --all-targets --locked -- -D warnings -A clippy::large-enum-variant -A clippy::assertions-on-constantspnpm checkpnpm test— 131 passedpnpm build