fix(pc-list): manual scan button refreshes state instead of killing mDNS - #51
Merged
Conversation
The manual scan button called MdnsDiscovery.discover(), but the background mDNS scan already set isDiscovering=true, so startDiscovery() early-returned and no new query fired. discover() then calls stopDiscovery(), which tears down the ongoing background scan (disposed=true, listeners off, discoveryService nulled). onPageShow only restarts polling, not the background scan, so mDNS auto-discovery of new hosts stayed dead for the rest of the page lifetime. Repurpose the button into a pure state refresh: call refreshAllComputers() to re-poll known hosts with no side effects on the background scan. Rename scanNetwork -> refreshKnownHosts and update label/toast/description to match.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesPcListPageV2 刷新入口调整
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Pull request overview
本 PR 修复 PcList 首页「扫描网络」按钮会误杀后台 mDNS 发现的问题：将手动操作降级为“刷新已知主机状态”，避免触发 MdnsDiscovery.discover() 导致后台扫描被 stopDiscovery() 停止，从而保证新主机仍可由后台持续 mDNS 自动发现。
Changes:
- 将按钮行为从
scanNetwork()改为refreshKnownHosts()，内部改调用ComputerManager.refreshAllComputers()刷新已知主机在线状态。 - 更新空状态与按钮文案（“扫描网络”→“刷新”，“扫描完成/失败”→“刷新完成/失败”）。
- 增加注释说明为何不再调用 mDNS discover（避免后续回归同类问题）。
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
591
to
+595
| .height(48) | ||
| .padding({ left: AppSpacing.XLarge, right: AppSpacing.XLarge }) | ||
| .borderRadius(AppSizes.RadiusLarge) | ||
| .backgroundColor(AppColors.Primary) | ||
| .onClick(() => this.scanNetwork()) | ||
| .onClick(() => this.refreshKnownHosts()) |
| .fillColor(AppColors.TextOnPrimary) | ||
| .margin({ right: 8 }) | ||
| Text('扫描网络') | ||
| Text('刷新') |
Comment on lines
581
to
+585
| .width(20) | ||
| .height(20) | ||
| .fillColor(AppColors.TextOnPrimary) | ||
| .margin({ right: 8 }) | ||
| Text('扫描网络') | ||
| Text('刷新') |
Comment on lines
718
to
+722
| .width(44) | ||
| .height(44) | ||
| .backgroundColor(0x1A000000) // 与 SaveButton 保持一致的半透明背景 | ||
| .enabled(!this.isScanning) | ||
| .onClick(() => this.scanNetwork()) | ||
| .onClick(() => this.refreshKnownHosts()) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
问题
首页「扫描网络」按钮不仅没用，还会把后台 mDNS 发现杀掉。
调用链：按钮 `scanNetwork()` → `ComputerManager.scanNetwork()` → `MdnsDiscovery.discover(5000)`：
```ts
async discover(timeout = 5000) {
await this.startDiscovery(); // (a)
await new Promise(r => setTimeout(r, timeout)); // (b) 干等 5s
this.stopDiscovery(); // (c)
return Array.from(this.discoveredComputers.values());
}
```
而 `startDiscovery()` 开头：
```ts
if (this.isDiscovering) {
console.info('MdnsDiscovery: 已经在发现中');
return; // ← 后台扫描已在跑，直接返回
}
```
由于 `aboutToAppear → initializeAsync()` 已经调了 `startBackgroundScan()`，`isDiscovering` 恒为 `true`，所以：
更糟的是它恢复不了：`onPageShow` 只重启 `startPolling()`，不重启 `startBackgroundScan()`（后者只在 `aboutToAppear` 调过一次）。所以点过一次按钮后，当前页面生命周期内 mDNS 自动发现新主机的能力就永久失效了，toast 却显示「扫描完成」。
修复
既然后台 mDNS 已经是持续发现，手动按钮对「发现新主机」本就是冗余的。把按钮改成纯状态刷新：
新主机发现仍由 `startBackgroundScan()` 持续负责，行为不变。
与安卓对比（背景）
安卓 `ComputerManagerService` 同样不做子网扫描，而是「持续 mDNS 发现 + 持续轮询已知主机」，架构与本仓库一致（离线阈值 2/3 等常量也是照抄）。所以这里把按钮降级为刷新，不影响与安卓的架构对齐。
遗留（可选后续）
`ComputerManager.scanNetwork()`（`ComputerManager.ets:1091`）现在是死代码——它就是触发上述 bug 的入口，且已无任何引用（grep 已确认）。建议单独清理（连同判断 `MdnsDiscovery.discover()` 是否还需要保留）。
测试
Summary by CodeRabbit
新功能
改进