Fix printer Wi-Fi config flow to match working v1.0 behavior#3
Fix printer Wi-Fi config flow to match working v1.0 behavior#3Karimrekiki wants to merge 11 commits into
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRefactors printer Wi‑Fi configuration to use a SunmiPrinterClient SDK (BluetoothBinding.aar). Adds client-driven connection and timeout flows in MainActivity and WifiConfigActivity, removes direct RFCOMM-based PrinterConfigHelper, adds device classification and receiver wrapper, UI for manual SSID entry, proguard/build/manifest updates, and many new strings. Changes
Sequence DiagramsequenceDiagram
actor User
participant MA as MainActivity
participant SPC as SunmiPrinterClient
participant BT as BluetoothDevice
participant WCA as WifiConfigActivity
participant Router as Router/Network
User->>MA: Select device from discovery list
MA->>MA: store address/name, show connecting UI, start 10s timeout
MA->>SPC: getPrinterSn(device)
SPC->>BT: perform BLE connection
SPC->>MA: onSnReceived(sn)
MA->>MA: cancel timeout, open WifiConfigActivity with device details
WCA->>SPC: request printer WiFi list
SPC->>Router: scan networks
SPC->>WCA: routerFound()... onGetWifiListFinish()
WCA->>User: populate spinner (or manual entry)
User->>WCA: select network & submit password
WCA->>SPC: setWifi(router, password)
SPC->>BT: send WiFi config to printer
SPC->>WCA: onSetWifiSuccess() / onWifiConfigFail()
WCA->>User: show success or error
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e55103533c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| progressBar.setVisibility(View.GONE); | ||
| scanButton.setEnabled(true); | ||
| scanButton.setText(R.string.scan_bluetooth); | ||
| statusText.setText(""); | ||
| openWifiConfigScreen(); |
There was a problem hiding this comment.
This callback always transitions to the Wi-Fi screen, even if the request has already timed out or a newer connection attempt is underway. connectionTimeoutRunnable only flips state; it does not cancel the in-flight SDK request, so a late onSnReceived can still arrive and trigger navigation with whatever pendingPrinterAddress currently contains, which can reopen the flow unexpectedly or target the wrong printer after retries. Guard this path with the active request state (and ideally a request token/address check) before opening the next activity.
Useful? React with 👍 / 👎.
| if (!availableRouters.isEmpty()) { | ||
| wifiSpinner.setSelection(0); | ||
| manualSsidContainer.setVisibility(View.GONE); |
There was a problem hiding this comment.
routerFound() refreshes the spinner on every discovered network, and this branch always resets selection to index 0 when any routers exist. While scanning is still streaming results, a user's manual choice (including manual-entry option) can be overwritten by a later callback, so tapping Configure may send credentials for a different SSID than the one they picked. Keep/restoring the prior selection during updates avoids this misconfiguration risk.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
app/proguard-rules.pro (1)
20-20: Overly broad package patternlibrary.**may cause unintended side effects.The pattern
library.**is extremely generic and could match any package starting with "library" across all dependencies, not just the intended one. This risks:
- Keeping unrelated classes, bloating the final APK
- Masking issues where actual required classes aren't properly kept
If this is meant to complement the inuker Bluetooth library on line 21, it may be redundant. Otherwise, specify the full package path (e.g.,
com.somevendor.library.**).♻️ Suggested fix: Remove or qualify the rule
# Keep callback implementers and SDK internals that may be invoked reflectively. -keep class * implements com.sunmi.cloudprinter.presenter.SunmiPrinterClient$IPrinterClient { *; } --keep class library.** { *; } -keep class com.inuker.bluetooth.library.** { *; }If a specific library package is needed, use its fully qualified name instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/proguard-rules.pro` at line 20, The ProGuard rule "keep class library.** { *; }" is too broad and may retain unintended classes; update the rule in proguard-rules.pro by either removing this line if it's redundant with the inuker Bluetooth rule on the next line, or replace "library.**" with the library's full package (e.g., com.vendor.library.**) so only the intended package is retained; ensure the rule references the exact package used by the inuker Bluetooth dependency rather than the generic "library.**".app/src/main/java/com/sunmi/printerconfig/BluetoothDeviceAdapter.java (1)
50-57: Prefer a formatted string resource over runtime concatenation.Line 56 builds user-facing text with
" • "in code. Using a formatted resource keeps localization flexible.🌐 Suggested refactor
- if (PrinterDeviceClassifier.isLikelySunmi(name)) { - baseName = baseName + " \u2022 " + holder.itemView.getContext().getString(R.string.likely_sunmi); - } + if (PrinterDeviceClassifier.isLikelySunmi(name)) { + baseName = holder.itemView.getContext().getString( + R.string.device_name_with_hint, + baseName, + holder.itemView.getContext().getString(R.string.likely_sunmi) + ); + }Also add in
app/src/main/res/values/strings.xml:<string name="device_name_with_hint">%1$s • %2$s</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/sunmi/printerconfig/BluetoothDeviceAdapter.java` around lines 50 - 57, Replace the runtime concatenation of the hint bullet in BluetoothDeviceAdapter with a formatted string resource: when PrinterDeviceClassifier.isLikelySunmi(name) is true, call holder.itemView.getContext().getString(R.string.device_name_with_hint, baseName, holder.itemView.getContext().getString(R.string.likely_sunmi)) instead of baseName + " • " + ...; add the suggested <string name="device_name_with_hint">%1$s • %2$s</string> to strings.xml so localization uses the formatted resource and remove the hard-coded " • " from the adapter.app/src/main/res/layout/activity_wifi_config.xml (1)
58-67: Link the manual SSID label to its input for accessibility.Line 59 should set
android:labelFor="@id/manualSsidInput"so screen readers associate the label with Line 66 input.♿ Suggested tweak
<TextView android:id="@+id/manualSsidLabel" android:layout_width="match_parent" android:layout_height="wrap_content" + android:labelFor="@id/manualSsidInput" android:text="@string/wifi_ssid" android:textSize="16sp" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/layout/activity_wifi_config.xml` around lines 58 - 67, The TextView with id manualSsidLabel should be linked to the EditText manualSsidInput for accessibility: add the attribute android:labelFor="@id/manualSsidInput" to the manualSsidLabel element so screen readers associate the label (manualSsidLabel) with the input field (manualSsidInput).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/build.gradle`:
- Line 59: The dependency declaration implementation
files('libs/BluetoothBinding.aar') should be protected by pinning its SHA-256
and documenting its origin; update CI to compute and verify the artifact's
SHA-256 before build (reject if mismatch) and add the expected hex hash to the
CI job or a repository file, and also add vendor/source and download provenance
for BluetoothBinding.aar to CONTRIBUTING.md or the build README; alternatively,
generate/commit a gradle verification-metadata.xml entry for
BluetoothBinding.aar and have CI validate it prior to invoking the Gradle build.
In `@app/src/main/AndroidManifest.xml`:
- Line 16: Scope the ACCESS_FINE_LOCATION permission to Android 11 and earlier
by adding maxSdkVersion="30" to the uses-permission element named
android.permission.ACCESS_FINE_LOCATION; locate the uses-permission with
android:name="android.permission.ACCESS_FINE_LOCATION" and update it to mirror
the ACCESS_COARSE_LOCATION entry (which uses maxSdkVersion="30") so the precise
location permission is not requested on Android 12+ where NEARBY_WIFI_DEVICES
with neverForLocation is used.
In `@app/src/main/java/com/sunmi/printerconfig/MainActivity.java`:
- Around line 188-220: onDeviceClick currently allows multiple taps to start
overlapping connections; add an early guard that checks the
waitingForPrinterConnection flag (or set it atomically) at the start of
onDeviceClick and if true show a brief Toast (or ignore) and return to prevent
overwriting pendingPrinterAddress/pendingPrinterName and racing
sunmiPrinterClient.getPrinterSn; ensure the flag is set to true only once before
posting the connection timeout and cleared by all failure/success paths that
call handlePrinterConnectionFailure or the success handler so subsequent taps
are allowed.
- Around line 295-317: Add an early guard in the asynchronous callback handlers
so they no-op if the connection attempt is already finished: at the start of
sendDataFail(int code, String msg) and onSnReceived(String sn) check the
waitingForPrinterConnection boolean and return immediately if it is false; this
prevents delayed callbacks from executing UI/state changes (like
handlePrinterConnectionFailure(...) or openWifiConfigScreen()) after a timeout
or competing result, while leaving existing cleanup calls (e.g.,
connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable)) intact
where appropriate.
In `@app/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.java`:
- Around line 341-358: Add an early guard that checks the boolean
waitingForWifiConfigResult at the start of each callback (onSetWifiSuccess,
wifiConfigSuccess, onWifiConfigFail, sendDataFail) and return immediately if
it's false so late SDK callbacks cannot overwrite timeout/failure UI state; keep
the rest of each method unchanged (e.g., runOnUiThread in onSetWifiSuccess,
handleConfigurationSuccess(), handleConfigurationFailure(...)) so
waitingForWifiConfigResult is still set to false by those handlers when
appropriate.
---
Nitpick comments:
In `@app/proguard-rules.pro`:
- Line 20: The ProGuard rule "keep class library.** { *; }" is too broad and may
retain unintended classes; update the rule in proguard-rules.pro by either
removing this line if it's redundant with the inuker Bluetooth rule on the next
line, or replace "library.**" with the library's full package (e.g.,
com.vendor.library.**) so only the intended package is retained; ensure the rule
references the exact package used by the inuker Bluetooth dependency rather than
the generic "library.**".
In `@app/src/main/java/com/sunmi/printerconfig/BluetoothDeviceAdapter.java`:
- Around line 50-57: Replace the runtime concatenation of the hint bullet in
BluetoothDeviceAdapter with a formatted string resource: when
PrinterDeviceClassifier.isLikelySunmi(name) is true, call
holder.itemView.getContext().getString(R.string.device_name_with_hint, baseName,
holder.itemView.getContext().getString(R.string.likely_sunmi)) instead of
baseName + " • " + ...; add the suggested <string
name="device_name_with_hint">%1$s • %2$s</string> to strings.xml so localization
uses the formatted resource and remove the hard-coded " • " from the adapter.
In `@app/src/main/res/layout/activity_wifi_config.xml`:
- Around line 58-67: The TextView with id manualSsidLabel should be linked to
the EditText manualSsidInput for accessibility: add the attribute
android:labelFor="@id/manualSsidInput" to the manualSsidLabel element so screen
readers associate the label (manualSsidLabel) with the input field
(manualSsidInput).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignoreapp/build.gradleapp/libs/BluetoothBinding.aarapp/proguard-rules.proapp/src/main/AndroidManifest.xmlapp/src/main/java/com/sunmi/printerconfig/BluetoothDeviceAdapter.javaapp/src/main/java/com/sunmi/printerconfig/MainActivity.javaapp/src/main/java/com/sunmi/printerconfig/PrinterConfigHelper.javaapp/src/main/java/com/sunmi/printerconfig/PrinterDeviceClassifier.javaapp/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.javaapp/src/main/res/layout/activity_wifi_config.xmlapp/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- app/src/main/java/com/sunmi/printerconfig/PrinterConfigHelper.java
| } | ||
|
|
||
| dependencies { | ||
| implementation files('libs/BluetoothBinding.aar') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Verify artifact exists
if [ -f app/libs/BluetoothBinding.aar ]; then
echo "✓ File exists: app/libs/BluetoothBinding.aar"
# 2) Produce deterministic digest to pin in CI/docs
sha256sum app/libs/BluetoothBinding.aar
else
echo "✗ File NOT found: app/libs/BluetoothBinding.aar"
fi
# 3) Confirm local AAR usage points
echo ""
echo "Searching for local AAR references in build files:"
rg -n "implementation\s+files\('libs/.*\.aar" --type gradleRepository: Arieboomsma/connectprinter
Length of output: 181
🏁 Script executed:
# Check for CI configuration that might validate dependencies
echo "Checking for CI/CD configuration:"
find . -type f \( -name ".github" -o -name ".gitlab-ci.yml" -o -name "*.yml" -o -name "*.yaml" \) -o -path "*/.github/workflows/*" | head -20
# Look for any checksum or integrity validation docs
echo ""
echo "Searching for provenance/checksum documentation:"
rg -i "checksum|sha256|integrity|provenance|BluetoothBinding" --type md --type txtRepository: Arieboomsma/connectprinter
Length of output: 153
🏁 Script executed:
# Check if there are any gradle dependency verification mechanisms
echo "Checking for gradle dependency verification:"
cat -n gradle/verification-metadata.xml 2>/dev/null || echo "No verification-metadata.xml found"
# Check build.gradle structure for other dependency management
echo ""
echo "Full app/build.gradle to understand context:"
cat -n app/build.gradle | head -80Repository: Arieboomsma/connectprinter
Length of output: 2415
🏁 Script executed:
# Search for any documentation or context about BluetoothBinding
echo "Searching for BluetoothBinding references:"
rg -i "bluetooth" --type md --type txt --type java --type kt
# Check for README or documentation
echo ""
echo "Checking for README or documentation:"
find . -maxdepth 2 -type f \( -name "README*" -o -name "*.md" \)Repository: Arieboomsma/connectprinter
Length of output: 268
🏁 Script executed:
# Check git history to understand when and why this was added
echo "Checking file existence and context:"
ls -lh app/libs/BluetoothBinding.aar 2>/dev/null || echo "File details unavailable"
# Look for any build documentation
echo ""
echo "Build/dependency documentation:"
find . -maxdepth 2 -type f -name "*build*" -o -name "*DEPEND*" -o -name "*.md"Repository: Arieboomsma/connectprinter
Length of output: 336
🏁 Script executed:
# Check README for dependency context
echo "=== README.md ==="
cat README.md
echo ""
echo "=== QUICK_START.md ==="
cat QUICK_START.mdRepository: Arieboomsma/connectprinter
Length of output: 10807
🏁 Script executed:
# Check build.sh for any validation or checksum logic
echo "=== build.sh ==="
cat build.sh
# Check root build.gradle for dependency management
echo ""
echo "=== root build.gradle ==="
cat build.gradleRepository: Arieboomsma/connectprinter
Length of output: 2008
Pin the BluetoothBinding.aar artifact hash in CI to verify supply-chain integrity.
The local BluetoothBinding.aar dependency bypasses Maven metadata and weakens supply-chain visibility. Add a SHA-256 verification step in your CI/CD pipeline to ensure the artifact hasn't been tampered with. If this is a vendor-supplied library, document its origin and source in the repository (CONTRIBUTING.md or build README).
No existing verification mechanisms (gradle verification-metadata.xml, CI checks) were found.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/build.gradle` at line 59, The dependency declaration implementation
files('libs/BluetoothBinding.aar') should be protected by pinning its SHA-256
and documenting its origin; update CI to compute and verify the artifact's
SHA-256 before build (reject if mismatch) and add the expected hex hash to the
CI job or a repository file, and also add vendor/source and download provenance
for BluetoothBinding.aar to CONTRIBUTING.md or the build README; alternatively,
generate/commit a gradle verification-metadata.xml entry for
BluetoothBinding.aar and have CI validate it prior to invoking the Gradle build.
| <!-- Needed for Bluetooth discovery on Android 6-11 --> | ||
| <uses-permission android:name="android.permission.ACCESS_COARSE_LOCATION" android:maxSdkVersion="30" /> | ||
| <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" android:maxSdkVersion="30" /> | ||
| <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the AndroidManifest.xml file
find . -name "AndroidManifest.xml" -type fRepository: Arieboomsma/connectprinter
Length of output: 104
🏁 Script executed:
# Check the content of the file around lines 16-23
if [ -f "app/src/main/AndroidManifest.xml" ]; then
echo "=== Current AndroidManifest.xml (lines 1-35) ==="
head -35 "app/src/main/AndroidManifest.xml" | cat -n
fiRepository: Arieboomsma/connectprinter
Length of output: 2023
🏁 Script executed:
# Check git history for this file to see what changed
cd . && git log --oneline -10 -- "app/src/main/AndroidManifest.xml" 2>/dev/null || echo "Git history not available"Repository: Arieboomsma/connectprinter
Length of output: 142
🏁 Script executed:
# Try to see the diff for the most recent change to this file
git diff HEAD~1 HEAD -- "app/src/main/AndroidManifest.xml" 2>/dev/null || echo "Cannot get git diff"Repository: Arieboomsma/connectprinter
Length of output: 89
🏁 Script executed:
# Try to get the full git log with patch to see if maxSdkVersion was removed
git log -p --all -- "app/src/main/AndroidManifest.xml" 2>/dev/null | grep -A5 -B5 "ACCESS_FINE_LOCATION" | head -50 || echo "Cannot retrieve full history"Repository: Arieboomsma/connectprinter
Length of output: 52
🏁 Script executed:
# Check if there are any recent commits that mention this change
git log --all --oneline -20 2>/dev/null || echo "Git not available"Repository: Arieboomsma/connectprinter
Length of output: 599
🏁 Script executed:
# Search for any issues or comments in the code about location permissions
rg "ACCESS_FINE_LOCATION|maxSdkVersion.*30" --type xml -B2 -A2Repository: Arieboomsma/connectprinter
Length of output: 1077
Scope ACCESS_FINE_LOCATION to Android 11 and earlier.
Line 16 declares precise location permission without API level constraints, but the comment on line 14 explicitly states this is needed only for "Bluetooth discovery on Android 6-11." The parallel permission on line 15 (ACCESS_COARSE_LOCATION) is correctly scoped to maxSdkVersion="30" for the same feature. On Android 12+, the app uses NEARBY_WIFI_DEVICES with neverForLocation flag, making ACCESS_FINE_LOCATION unnecessary. Leaving it unscoped unnecessarily broadens the privacy/compliance scope on modern Android versions.
🔧 Suggested fix
- <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />
+ <uses-permission
+ android:name="android.permission.ACCESS_FINE_LOCATION"
+ android:maxSdkVersion="30" />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" /> | |
| <uses-permission | |
| android:name="android.permission.ACCESS_FINE_LOCATION" | |
| android:maxSdkVersion="30" /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/AndroidManifest.xml` at line 16, Scope the ACCESS_FINE_LOCATION
permission to Android 11 and earlier by adding maxSdkVersion="30" to the
uses-permission element named android.permission.ACCESS_FINE_LOCATION; locate
the uses-permission with android:name="android.permission.ACCESS_FINE_LOCATION"
and update it to mirror the ACCESS_COARSE_LOCATION entry (which uses
maxSdkVersion="30") so the precise location permission is not requested on
Android 12+ where NEARBY_WIFI_DEVICES with neverForLocation is used.
| private void onDeviceClick(BluetoothDevice device) { | ||
| if (checkBluetoothPermission() && bluetoothAdapter.isDiscovering()) { | ||
| if (!checkBluetoothPermission()) { | ||
| Toast.makeText(this, R.string.permissions_required, Toast.LENGTH_SHORT).show(); | ||
| return; | ||
| } | ||
|
|
||
| if (bluetoothAdapter.isDiscovering()) { | ||
| bluetoothAdapter.cancelDiscovery(); | ||
| } | ||
|
|
||
| pendingPrinterAddress = safeGetDeviceAddress(device); | ||
| pendingPrinterName = safeGetDeviceName(device); | ||
|
|
||
| if (pendingPrinterAddress.isEmpty()) { | ||
| Toast.makeText(this, R.string.printer_address_unavailable, Toast.LENGTH_LONG).show(); | ||
| return; | ||
| } | ||
|
|
||
| waitingForPrinterConnection = true; | ||
| progressBar.setVisibility(View.VISIBLE); | ||
| scanButton.setEnabled(false); | ||
| statusText.setText(R.string.connecting_to_printer); | ||
|
|
||
| connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable); | ||
| connectionTimeoutHandler.postDelayed(connectionTimeoutRunnable, PRINTER_CONNECTION_TIMEOUT_MS); | ||
|
|
||
| try { | ||
| // Match the known stable flow: establish BLE session before Wi-Fi config screen. | ||
| sunmiPrinterClient.getPrinterSn(pendingPrinterAddress); | ||
| } catch (Throwable t) { | ||
| handlePrinterConnectionFailure(getString(R.string.printer_connection_failed)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent overlapping connection attempts from repeated device taps.
Line 188 currently allows another click while a connection attempt is active, which can overwrite pendingPrinterAddress/pendingPrinterName and race requests.
🔒 Suggested guard
private void onDeviceClick(BluetoothDevice device) {
+ if (waitingForPrinterConnection) {
+ return;
+ }
+
if (!checkBluetoothPermission()) {
Toast.makeText(this, R.string.permissions_required, Toast.LENGTH_SHORT).show();
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void onDeviceClick(BluetoothDevice device) { | |
| if (checkBluetoothPermission() && bluetoothAdapter.isDiscovering()) { | |
| if (!checkBluetoothPermission()) { | |
| Toast.makeText(this, R.string.permissions_required, Toast.LENGTH_SHORT).show(); | |
| return; | |
| } | |
| if (bluetoothAdapter.isDiscovering()) { | |
| bluetoothAdapter.cancelDiscovery(); | |
| } | |
| pendingPrinterAddress = safeGetDeviceAddress(device); | |
| pendingPrinterName = safeGetDeviceName(device); | |
| if (pendingPrinterAddress.isEmpty()) { | |
| Toast.makeText(this, R.string.printer_address_unavailable, Toast.LENGTH_LONG).show(); | |
| return; | |
| } | |
| waitingForPrinterConnection = true; | |
| progressBar.setVisibility(View.VISIBLE); | |
| scanButton.setEnabled(false); | |
| statusText.setText(R.string.connecting_to_printer); | |
| connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable); | |
| connectionTimeoutHandler.postDelayed(connectionTimeoutRunnable, PRINTER_CONNECTION_TIMEOUT_MS); | |
| try { | |
| // Match the known stable flow: establish BLE session before Wi-Fi config screen. | |
| sunmiPrinterClient.getPrinterSn(pendingPrinterAddress); | |
| } catch (Throwable t) { | |
| handlePrinterConnectionFailure(getString(R.string.printer_connection_failed)); | |
| } | |
| } | |
| private void onDeviceClick(BluetoothDevice device) { | |
| if (waitingForPrinterConnection) { | |
| return; | |
| } | |
| if (!checkBluetoothPermission()) { | |
| Toast.makeText(this, R.string.permissions_required, Toast.LENGTH_SHORT).show(); | |
| return; | |
| } | |
| if (bluetoothAdapter.isDiscovering()) { | |
| bluetoothAdapter.cancelDiscovery(); | |
| } | |
| pendingPrinterAddress = safeGetDeviceAddress(device); | |
| pendingPrinterName = safeGetDeviceName(device); | |
| if (pendingPrinterAddress.isEmpty()) { | |
| Toast.makeText(this, R.string.printer_address_unavailable, Toast.LENGTH_LONG).show(); | |
| return; | |
| } | |
| waitingForPrinterConnection = true; | |
| progressBar.setVisibility(View.VISIBLE); | |
| scanButton.setEnabled(false); | |
| statusText.setText(R.string.connecting_to_printer); | |
| connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable); | |
| connectionTimeoutHandler.postDelayed(connectionTimeoutRunnable, PRINTER_CONNECTION_TIMEOUT_MS); | |
| try { | |
| // Match the known stable flow: establish BLE session before Wi-Fi config screen. | |
| sunmiPrinterClient.getPrinterSn(pendingPrinterAddress); | |
| } catch (Throwable t) { | |
| handlePrinterConnectionFailure(getString(R.string.printer_connection_failed)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/sunmi/printerconfig/MainActivity.java` around lines 188
- 220, onDeviceClick currently allows multiple taps to start overlapping
connections; add an early guard that checks the waitingForPrinterConnection flag
(or set it atomically) at the start of onDeviceClick and if true show a brief
Toast (or ignore) and return to prevent overwriting
pendingPrinterAddress/pendingPrinterName and racing
sunmiPrinterClient.getPrinterSn; ensure the flag is set to true only once before
posting the connection timeout and cleared by all failure/success paths that
call handlePrinterConnectionFailure or the success handler so subsequent taps
are allowed.
| public void sendDataFail(int code, String msg) { | ||
| String failure = getString(R.string.wifi_push_error_with_code, code, msg == null ? "Unknown" : msg); | ||
| handlePrinterConnectionFailure(failure); | ||
| } | ||
|
|
||
| @Override | ||
| public void getSnRequestSuccess() { | ||
| // Wait for onSnReceived callback. | ||
| } | ||
|
|
||
| @Override | ||
| public void onSnReceived(String sn) { | ||
| waitingForPrinterConnection = false; | ||
| connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable); | ||
|
|
||
| runOnUiThread(() -> { | ||
| progressBar.setVisibility(View.GONE); | ||
| scanButton.setEnabled(true); | ||
| scanButton.setText(R.string.scan_bluetooth); | ||
| statusText.setText(""); | ||
| openWifiConfigScreen(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MainActivity.java" -path "*/sunmi/printerconfig/*" | head -5Repository: Arieboomsma/connectprinter
Length of output: 131
🏁 Script executed:
wc -l ./app/src/main/java/com/sunmi/printerconfig/MainActivity.javaRepository: Arieboomsma/connectprinter
Length of output: 135
🏁 Script executed:
cat -n ./app/src/main/java/com/sunmi/printerconfig/MainActivity.javaRepository: Arieboomsma/connectprinter
Length of output: 15562
Guard callback handlers against stale invocations after connection attempt completes.
The methods sendDataFail() and onSnReceived() (lines 295-317) implement asynchronous callbacks from the SunmiPrinterClient layer. These callbacks can be queued in the BLE/network layer and may arrive after the connection attempt has already concluded (via timeout or a competing callback), causing them to overwrite previous state and unexpectedly reopen the Wi-Fi configuration screen or show duplicate error messages.
While connectionTimeoutRunnable itself guards against stale execution (line 55), the callback methods do not. Add early-return checks for waitingForPrinterConnection to prevent delayed callbacks from executing side effects after their associated connection attempt is no longer pending.
Suggested fix
`@Override`
public void sendDataFail(int code, String msg) {
+ if (!waitingForPrinterConnection) {
+ return;
+ }
String failure = getString(R.string.wifi_push_error_with_code, code, msg == null ? "Unknown" : msg);
handlePrinterConnectionFailure(failure);
}
`@Override`
public void onSnReceived(String sn) {
+ if (!waitingForPrinterConnection) {
+ return;
+ }
waitingForPrinterConnection = false;
connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable);
runOnUiThread(() -> {
progressBar.setVisibility(View.GONE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/sunmi/printerconfig/MainActivity.java` around lines 295
- 317, Add an early guard in the asynchronous callback handlers so they no-op if
the connection attempt is already finished: at the start of sendDataFail(int
code, String msg) and onSnReceived(String sn) check the
waitingForPrinterConnection boolean and return immediately if it is false; this
prevents delayed callbacks from executing UI/state changes (like
handlePrinterConnectionFailure(...) or openWifiConfigScreen()) after a timeout
or competing result, while leaving existing cleanup calls (e.g.,
connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable)) intact
where appropriate.
| public void onSetWifiSuccess() { | ||
| runOnUiThread(() -> statusText.setText(R.string.configuring)); | ||
| } | ||
|
|
||
| @Override | ||
| public void wifiConfigSuccess() { | ||
| handleConfigurationSuccess(); | ||
| } | ||
|
|
||
| @Override | ||
| public void onWifiConfigFail() { | ||
| handleConfigurationFailure(getString(R.string.wifi_push_failed_try_24g)); | ||
| } | ||
|
|
||
| @Override | ||
| public void sendDataFail(int code, String msg) { | ||
| String failureMessage = getString(R.string.wifi_push_error_with_code, code, msg == null ? "Unknown" : msg); | ||
| handleConfigurationFailure(failureMessage); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f WifiConfigActivity.javaRepository: Arieboomsma/connectprinter
Length of output: 135
🏁 Script executed:
wc -l app/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.javaRepository: Arieboomsma/connectprinter
Length of output: 139
🏁 Script executed:
cat -n app/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.javaRepository: Arieboomsma/connectprinter
Length of output: 15333
Add state guards to config callbacks to prevent late SDK callbacks from overwriting timeout/failure state.
The callback handlers at lines 341-358 can still execute after timeout (line 50-54) or explicit failure, overwriting the UI state with contradictory outcomes. Although handleConfigurationSuccess() and handleConfigurationFailure() both set waitingForWifiConfigResult = false, this happens after being called, so late callbacks proceed unchecked. Add waitingForWifiConfigResult guards to all four callback entry points as suggested.
Suggested fix
`@Override`
public void onSetWifiSuccess() {
+ if (!waitingForWifiConfigResult) {
+ return;
+ }
runOnUiThread(() -> statusText.setText(R.string.configuring));
}
`@Override`
public void wifiConfigSuccess() {
+ if (!waitingForWifiConfigResult) {
+ return;
+ }
handleConfigurationSuccess();
}
`@Override`
public void onWifiConfigFail() {
+ if (!waitingForWifiConfigResult) {
+ return;
+ }
handleConfigurationFailure(getString(R.string.wifi_push_failed_try_24g));
}
`@Override`
public void sendDataFail(int code, String msg) {
+ if (!waitingForWifiConfigResult) {
+ return;
+ }
String failureMessage = getString(R.string.wifi_push_error_with_code, code, msg == null ? "Unknown" : msg);
handleConfigurationFailure(failureMessage);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.java` around
lines 341 - 358, Add an early guard that checks the boolean
waitingForWifiConfigResult at the start of each callback (onSetWifiSuccess,
wifiConfigSuccess, onWifiConfigFail, sendDataFail) and return immediately if
it's false so late SDK callbacks cannot overwrite timeout/failure UI state; keep
the rest of each method unchanged (e.g., runOnUiThread in onSetWifiSuccess,
handleConfigurationSuccess(), handleConfigurationFailure(...)) so
waitingForWifiConfigResult is still set to false by those handlers when
appropriate.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
app/src/main/java/com/sunmi/printerconfig/MainActivity.java (2)
191-223:⚠️ Potential issue | 🟠 MajorPrevent overlapping connection attempts from repeated taps.
onDeviceClickshould return immediately when a connection attempt is already pending to avoid racingpendingPrinterAddress/pendingPrinterName.Suggested fix
private void onDeviceClick(BluetoothDevice device) { + if (waitingForPrinterConnection) { + return; + } + if (!checkBluetoothPermission()) { Toast.makeText(this, R.string.permissions_required, Toast.LENGTH_SHORT).show(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/sunmi/printerconfig/MainActivity.java` around lines 191 - 223, The onDeviceClick method can start overlapping connections; add an early guard that returns immediately when waitingForPrinterConnection is true to prevent races updating pendingPrinterAddress/pendingPrinterName and duplicate connection attempts. Locate the onDeviceClick(BluetoothDevice) method and at its start check the waitingForPrinterConnection flag and return (optionally show a Toast or ignore) if a connection is already pending, ensuring existing logic that sets progressBar, scanButton, statusText, and posts connectionTimeoutRunnable only runs when not already waiting.
297-320:⚠️ Potential issue | 🟠 MajorIgnore stale printer callbacks after attempt completion.
sendDataFailandonSnReceivedshould guard onwaitingForPrinterConnectionso delayed callbacks can’t reopen flow or overwrite terminal state.Suggested fix
`@Override` public void sendDataFail(int code, String msg) { + if (!waitingForPrinterConnection) { + return; + } String failure = getString(R.string.wifi_push_error_with_code, code, msg == null ? "Unknown" : msg); handlePrinterConnectionFailure(failure); } `@Override` public void onSnReceived(String sn) { + if (!waitingForPrinterConnection) { + return; + } waitingForPrinterConnection = false; connectionTimeoutHandler.removeCallbacks(connectionTimeoutRunnable); runOnUiThread(() -> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/sunmi/printerconfig/MainActivity.java` around lines 297 - 320, Both sendDataFail and onSnReceived should ignore stale callbacks by checking waitingForPrinterConnection at the start and bailing out if it's false; update sendDataFail to return immediately when waitingForPrinterConnection is false before building failure string/handling, and update onSnReceived to return immediately if waitingForPrinterConnection is false (otherwise keep the existing logic that clears waitingForPrinterConnection, removes callbacks, updates UI and calls openWifiConfigScreen). Reference the methods sendDataFail(...) and onSnReceived(String) when making these guards.app/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.java (1)
343-362:⚠️ Potential issue | 🟠 MajorGuard Wi‑Fi config callbacks against stale invocations.
These async callbacks should no-op once the request is no longer active; otherwise late SDK events can overwrite timeout/failure UI state.
Suggested fix
`@Override` public void onSetWifiSuccess() { + if (!waitingForWifiConfigResult) { + return; + } runOnUiThread(() -> statusText.setText(R.string.configuring)); } `@Override` public void wifiConfigSuccess() { + if (!waitingForWifiConfigResult) { + return; + } handleConfigurationSuccess(); } `@Override` public void onWifiConfigFail() { + if (!waitingForWifiConfigResult) { + return; + } handleConfigurationFailure(getString(R.string.wifi_push_failed_try_24g)); } `@Override` public void sendDataFail(int code, String msg) { + if (!waitingForWifiConfigResult) { + return; + } String failureMessage = getString(R.string.wifi_push_error_with_code, code, msg == null ? "Unknown" : msg); handleConfigurationFailure(failureMessage); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.java` around lines 343 - 362, The Wi‑Fi callback handlers (onSetWifiSuccess, wifiConfigSuccess, onWifiConfigFail, sendDataFail) must be guarded so they no‑op if the original request is no longer active; add a check at the top of each method that verifies a config-in-progress marker (e.g., a boolean like isConfigActive or a currentRequestId match) and return early if the marker indicates the request was cancelled/expired, and ensure the marker is set true when starting configuration and cleared on timeout/success/failure so late SDK events cannot overwrite UI state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/src/main/java/com/sunmi/printerconfig/MainActivity.java`:
- Around line 191-223: The onDeviceClick method can start overlapping
connections; add an early guard that returns immediately when
waitingForPrinterConnection is true to prevent races updating
pendingPrinterAddress/pendingPrinterName and duplicate connection attempts.
Locate the onDeviceClick(BluetoothDevice) method and at its start check the
waitingForPrinterConnection flag and return (optionally show a Toast or ignore)
if a connection is already pending, ensuring existing logic that sets
progressBar, scanButton, statusText, and posts connectionTimeoutRunnable only
runs when not already waiting.
- Around line 297-320: Both sendDataFail and onSnReceived should ignore stale
callbacks by checking waitingForPrinterConnection at the start and bailing out
if it's false; update sendDataFail to return immediately when
waitingForPrinterConnection is false before building failure string/handling,
and update onSnReceived to return immediately if waitingForPrinterConnection is
false (otherwise keep the existing logic that clears
waitingForPrinterConnection, removes callbacks, updates UI and calls
openWifiConfigScreen). Reference the methods sendDataFail(...) and
onSnReceived(String) when making these guards.
In `@app/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.java`:
- Around line 343-362: The Wi‑Fi callback handlers (onSetWifiSuccess,
wifiConfigSuccess, onWifiConfigFail, sendDataFail) must be guarded so they no‑op
if the original request is no longer active; add a check at the top of each
method that verifies a config-in-progress marker (e.g., a boolean like
isConfigActive or a currentRequestId match) and return early if the marker
indicates the request was cancelled/expired, and ensure the marker is set true
when starting configuration and cleared on timeout/success/failure so late SDK
events cannot overwrite UI state.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/build.gradleapp/src/main/java/com/sunmi/printerconfig/MainActivity.javaapp/src/main/java/com/sunmi/printerconfig/ReceiverSafeContext.javaapp/src/main/java/com/sunmi/printerconfig/WifiConfigActivity.java
🚧 Files skipped from review as they are similar to previous changes (1)
- app/build.gradle
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b492a564f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private Router buildManualRouter(String ssid) { | ||
| Router router = new Router(); | ||
| router.setName(ssid); | ||
| router.setEssid(ssid.getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Manual SSID entry stores router.setEssid(ssid.getBytes(...)) as a variable-length byte array, but the Sunmi SDK Wi‑Fi command format expects the ESSID field in the fixed-size form used by scanned routers (64-byte payload). When users rely on manual entry (for hidden SSIDs or failed scans), this mismatch can produce malformed config payloads and cause provisioning to fail; encode/pad manual SSIDs to the SDK’s expected ESSID width before calling setPrinterWifi.
Useful? React with 👍 / 👎.
| public void wifiConfigSuccess() { | ||
| handleConfigurationSuccess(); |
There was a problem hiding this comment.
This success callback always finalizes as success, even after wifiConfigTimeoutRunnable has already marked the request failed or a newer attempt has started. Since timeout handling does not cancel the in-flight SDK request, late wifiConfigSuccess (or fail) callbacks from an older attempt can still close the activity and overwrite current state; guard callbacks with active-request state/token checks before updating UI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2f1b23d77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| progressBar.setVisibility(View.GONE); | ||
| configureButton.setEnabled(true); | ||
|
|
There was a problem hiding this comment.
onGetWifiListFinish always hides the progress indicator and re-enables the Configure button even if setPrinterWifi is already in flight (waitingForWifiConfigResult == true). Because manual submission is possible before list fetch completes, a late list-finish callback can overwrite the “sending” state and let users submit a second configuration request while the first is still pending, causing duplicated/competing writes and misleading status.
Useful? React with 👍 / 👎.
1 participant
Summary\n- switch Wi-Fi push to printer-reported network flow (Sunmi SDK + with router ESSID)\n- fix long configuring state with explicit timeout handling\n- stabilize BLE handoff sequence before Wi-Fi config\n- fix SSID selection/manual-entry behavior\n- release version bumped through internal test to (code 8)\n\n## Validation\n- \n- \n- internal testing release published and confirmed working
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores