修复离线/在线历史记录与离线路线同步问题#2257
Hidden character warning
Conversation
…mestamps - Introduced `normalize` method in `HistoryEntryKind` to standardize entry types. - Updated `History` and `Progress` classes to support entry kind and timestamp management. - Modified repository methods to handle entry kind in history retrieval and progress updates. - Improved synchronization logic to maintain separate online and offline histories. - Added tests to ensure correct handling of entry kinds and progress timestamps.
…lution - Updated `PlayerController` to directly use `danmakuEpisodeNumber` for loading danmaku. - Modified `PlaybackInitParams` to include `danmakuEpisodeNumber`. - Enhanced `_VideoPageController` to resolve episodes more effectively, improving history tracking and logging. - Refactored methods to utilize `ResolvedEpisode` for better clarity and maintainability.
- Added methods to `DownloadController` for finding episodes by URL, number, and name. - Implemented logic to check if an episode's download status is claimed. - Updated `DownloadEpisodeMatcher` to improve episode lookup capabilities. - Refactored `DownloadEpisodeSheet` and `VideoPage` to utilize new matching methods for better download status handling. - Improved repository interface to support enhanced episode retrieval.
- Enhanced `DownloadController` with new methods for episode retrieval by various identifiers. - Implemented checks for claimed download statuses of episodes. - Updated `DownloadEpisodeMatcher` for better episode lookup efficiency. - Refactored `DownloadEpisodeSheet` and `VideoPage` to leverage new matching capabilities for improved download status management. - Enhanced repository interface for more effective episode retrieval.
…agement - Introduced a new method in `DownloadManager` for reconciling episode data with local files, improving download state accuracy. - Updated `_DownloadController` to reset incomplete episode statuses and reconcile episodes before resuming downloads. - Enhanced error handling and logging for reconciliation failures. - Improved progress tracking and state updates for episodes during download operations.
…e management - Added SyncPlayEpisodeIdentity class to encapsulate episode identification details. - Updated PlayerController and related classes to utilize SyncPlayEpisodeIdentity for episode handling. - Refactored methods to improve episode resolution and synchronization in sync play scenarios. - Enhanced error handling and logging for episode identity resolution. - Updated tests to validate new SyncPlay functionality and ensure correct episode identity handling.
- Introduced OfflineRoadListSnapshot for improved organization of downloaded episodes by original road. - Updated video controller to utilize the new snapshot for building offline road lists and resolving episodes. - Enhanced methods for finding offline episodes, including preferred road handling. - Improved UI to reflect current road names and episode identifiers dynamically. - Added tests for OfflineRoadListSnapshot to ensure correct grouping and episode retrieval logic.
- Updated history synchronization logic to incorporate legacy entity keys for improved version management. - Introduced a new method to determine the appropriate deleted version for upserts based on entry kind. - Enhanced tests to validate the behavior of legacy delete tombstones and their impact on online and offline upserts. - Ensured that legacy delete tombstones do not block offline upserts while correctly managing online upserts.
|
审查前需要优先解决合并冲突 |
|
我看到 #1984 了，但是这个pr的合并冲突确实需要优先处理 |
1. 合并 upstream/main 并保留 services 目录迁移后的历史、下载和播放修复 2. 合并 HistorySync watch-state 拆分与 online/offline scoped 历史语义 3. 更新本地 todo 记录冲突解决状态和后续手动边界验证
- Updated package versions for several dependencies, including _fe_analyzer_shared (99.0.0) and analyzer (12.1.0). - Changed package source URLs from "https://pub.dev" to "https://pub.flutter-io.cn". - Removed unused fields in bangumi_history_card.dart to streamline the code.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| final keyMap = { | ||
| for (final history in histories) history.key: history.key, | ||
| for (final history in histories) | ||
| History.legacyKey(history.adapterName, history.bangumiItem): |
There was a problem hiding this comment.
WARNING: The legacy key is mapped to a history's scoped key for every history regardless of entryKind. If a snapshot contains an offline history for a (plugin, id) pair together with a legacy bare-key online delete tombstone in deletedVersions, _canonicalizeVersionMap rewrites that online tombstone onto the offline entry's scoped key (...::offline). A subsequent offline upsert then reads that (online) deletion via _deletedVersionForUpsert and can be wrongly suppressed, silently dropping offline history.
Consider only registering the legacy→scoped mapping for online entries (e.g. guard with HistoryEntryKind.normalize(history.entryKind) == HistoryEntryKind.online). The same pattern exists in HistorySyncState.fromSnapshot (line 361) and in the _canonicalExistingEntityKey loop (line 585), so a legacy-form delete event can also resolve onto an offline entry when only an offline entry exists locally.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Incremental Review (since
|
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 0 |
| SUGGESTION | 0 |
Incremental Review (since 6c19e51)
The latest commits resolve the previously carried-over WARNING. Legacy→scoped key mapping in history_sync.dart is now guarded by entryKind == HistoryEntryKind.online at all three sites (L280 fromJson, L362 fromSnapshot, L588 _canonicalExistingEntityKey), so legacy online delete tombstones can no longer be canonicalized onto offline entries. New tests in test/history_sync_test.dart cover both the offline-snapshot canonicalization and legacy delete-event cases.
history_repository.dart was refactored to inject historiesBox and privateModeReader for testability (defaults preserve prior behavior), and the now-unused setSettingsBoxForTest / clearSettingsForTest helpers were removed from storage.dart (verified no remaining references).
Previously flagged WARNING is now resolved. No new issues found.
Files Reviewed in this increment (5 files)
lib/modules/history/history_sync.dart- resolved prior WARNINGlib/repositories/history_repository.dart- dependency injection refactorlib/services/storage/storage.dart- removed unused test helperstest/history_repository_test.dart- updated teststest/history_sync_test.dart- new test coverage
Previous review (commit 6c19e51)
Status: 1 Issue Found (unchanged from prior review) | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 1 |
| SUGGESTION | 0 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
lib/modules/history/history_sync.dart |
280 | Legacy→scoped key map is built for every history regardless of entryKind; a legacy online delete tombstone can be canonicalized onto an offline entry and suppress offline upserts (also affects fromSnapshot L361 and _canonicalExistingEntityKey L585). (Carried over — file not modified in latest commits; still unresolved.) |
Incremental Review (since 0593c34)
The only change in the latest commit (6c19e51) is pubspec.lock, synced to upstream main. This is a generated lock file and is excluded from review.
No new issues found in the incremental diff.
Files Reviewed in this increment (1 file)
pubspec.lock(generated lock file — skipped)
Fix these issues in Kilo Cloud
Previous review (commit 0593c34)
Status: 1 Issue Found (unchanged from prior review) | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 1 |
| SUGGESTION | 0 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
lib/modules/history/history_sync.dart |
280 | Legacy→scoped key map is built for every history regardless of entryKind; a legacy online delete tombstone can be canonicalized onto an offline entry and suppress offline upserts (also affects fromSnapshot L361 and _canonicalExistingEntityKey L585). (Carried over — file not modified in latest commits; still unresolved.) |
Incremental Review (since 406c8b0)
Latest commits changed only test infrastructure:
lib/services/storage/storage.dart— addedsetSettingsBoxForTest/clearSettingsForTesthelpers and removed a blank import line._settingislate final; the setter is invoked once fromsetUpAlland the clear helper only clears the box, so both are safe in the test context.test/history_repository_test.dart— switched to the new helpers instead of the previously publicsettingfield.
No new issues found in the incremental diff.
Files Reviewed in this increment (2 files)
lib/services/storage/storage.darttest/history_repository_test.dart
Fix these issues in Kilo Cloud
Previous review (commit 406c8b0)
Status: 1 Issue Found | Recommendation: Address before merge (note: maintainer already flagged unresolved merge conflicts that must be resolved first)
Overview
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 1 |
| SUGGESTION | 0 |
Issue Details (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
lib/modules/history/history_sync.dart |
280 | Legacy→scoped key map is built for every history regardless of entryKind; a legacy online delete tombstone can be canonicalized onto an offline entry and suppress offline upserts (also affects fromSnapshot L361 and _canonicalExistingEntityKey L585). |
Files Reviewed (26 files)
History / sync / storage
lib/modules/history/history_module.dartlib/modules/history/history_module.g.dartlib/modules/history/history_sync.dart- 1 issuelib/pages/history/history_controller.dartlib/repositories/history_repository.dartlib/services/sync/history_sync_service.dartlib/services/sync/webdav.dartlib/services/storage/storage.dartlib/bean/card/bangumi_history_card.dart
Download
lib/pages/download/download_controller.dartlib/pages/download/download_episode_sheet.dartlib/repositories/download_repository.dartlib/services/download/download_manager.dart
Player / video / syncplay
lib/pages/player/controller/player_models.dartlib/pages/player/controller/player_syncplay_controller.dartlib/pages/player/player_controller.dartlib/pages/player/player_item.dartlib/pages/video/video_controller.dartlib/pages/video/video_page.dart
Other / tests / generated
.gitignoreandroid/app/src/main/AndroidManifest.xmlpubspec.lock(generated, skipped)test/download_cancellation_test.darttest/download_episode_matcher_test.darttest/download_progress_reconcile_test.darttest/history_repository_test.darttest/history_sync_test.darttest/resolved_episode_test.darttest/syncplay_episode_codec_test.dart
Notes
- The episode-identity rework, SyncPlay
kazumi:v2codec (encode/decode + legacy compatibility), offline roadList grouping, download progress reconciliation, and pause/cancel handling were reviewed and appear consistent with the included tests; bounds checks now guard previously crash-proneroadList/identifier/dataindexing. - The flagged WARNING is a narrow but real cross-version edge case (offline entry + legacy bare-key online tombstone in the same snapshot).
Reviewed by claude-4.8-opus-20260528 · Input: 4.6K · Output: 9.7K · Cached: 588.9K
|
CI 没有通过，除了自动代码审查机器人指出的问题，语法分析没有通过 更改 pubspec.lock 需要恰当的理由，再本地使用 flutter 大陆地区镜像是可以的，但是不要把它们提交到云端，云端的 CI 服务器运行在大陆之外的地区，使用大陆地区镜像反而容易出问题 |
- 添加 GStorage 测试专用 settings box 初始化与清理方法 - 更新 history_repository_test 避免访问已移除的 GStorage.setting 接口 - 通过 flutter analyze 和完整 flutter test 验证
- 恢复 pubspec.lock 为 upstream/main 当前版本 - 移除当前 PR 分支中的锁文件差异
|
直接把 测试后门 塞到存储持久化实现文件中，是很糟糕的实践，可以知道你在使用哪个模型进行辅助编程吗 |
|
Codex 5.5 xHigh，我有 Cpp 5年左右的经验，对 Flutter 这类不是很熟悉，正在学习 |
|
生产部分的测试后门我稍后会移除 |
- 更新历史记录同步逻辑，根据条目类型有条件地处理旧版键，确保在线和离线历史记录得到正确管理。 - 重构 HistoryRepository，使其支持自定义 histories box 和隐私模式读取器，提升可测试性与灵活性。 - 移除与 GStorage 设置相关的过时测试方法，简化测试初始化流程。 - 新增测试，用于验证隐私模式下历史记录更新行为，以及旧版删除事件的处理逻辑。
|
SyncPlay v2 改造的目的是什么，实际上切换 road 并不会触发一次硬同步，这样行为和之前基本一致，还新增了大量代码，在这里进行 road 级别的同步似乎也没有太大的意义 |
|
此PR的代码清洁性也有问题，看上去多次迭代后死代码和过时的接口没有正确清理 |
|
我PR的最后一句话 “推进全链路统一集数身份模型，进一步收敛 episodeNumber、currentEpisode、roadList.data、identifier 对列表位置、真实集号、源站 URL 和展示标题的混用” ，在修改在线，离线历史记录同步时我发现很多对 “集” 的描述有问题，最经典的就是下载后，list 12345[6] ，这里的数字毫无意义，只是描述当前下载的 index。 这次设计 v2 的主要原因是：旧的 SyncPlay payload 只有
这些概念在观看历史、在线播放、离线播放、弹幕加载和 SyncPlay 之间反复转换，旧协议无法表达完整身份，只能靠列表位置猜测。这会导致离线播放、标题集号不连续、不同线路列表结构不一致时，同步目标和历史记录目标不稳定。 所以 SyncPlay v2 的意义是引入一个更明确的 episode identity：
这样 SyncPlay 不再只同步一个模糊的列表下标，而是可以携带足够信息，让接收端根据自己的在线/离线 roadList 去解析同一个“规范集数目标”。这也是后续收敛观看历史、离线播放、弹幕和同步逻辑到统一集数身份模型的基础。 因此 v2 当前的设计重点不是 road 级硬同步，而是为全链路统一 episode identity 做协议层铺垫，并保持旧版 |
|
听上去超棒的想法，但是我们要么应该在这里直接完成完全体的 syncplay v2 ，要么就在这个PR中移除这部分，在之后单独的PR中实现 当前PR中的实现是半成品，只是增加了冗余代码，没有看到好处 |
|
你说的确实有道理，它实际上与这个PR的解决目标不一致。 |
- 从观看历史分支移除 SyncPlay v2 identity、codec 和接收端解析逻辑 - 保留 ResolvedEpisode、danmakuEpisodeNumber、在线离线历史修复 - 删除 SyncPlay v2 专属测试断言
|
还是有一些代码清洁性问题，例如
|
|
在更仔细的查看这个实现后，我理解了 syncplay 变更的重要性，以及此PR之前的离线视频使用一起看时的严重bug 也就是离线视频自己的分集列表长度与完整分集列表存在巨大出入，可能出现 syncplay 同步时的分集漂移甚至越界 想知道你怎么看这个问题 |
|
这个问题的原因很明显就是底层 “在观看历史、在线播放、离线播放、弹幕加载和 SyncPlay 之间反复转换”，原有的 数据模型 不能完整表达身份。这个问题同时还影响历史记录跳转问题 它应该另起一个 RP，对数据模型，协议进行一个整体的规划和重新设计，包括之前提交的 syncplay v2 并不能完全修复根因。 同时我也觉得这个PR过于臃肿，合并风险较高，我会关闭它，后续再陆续提供几个较小的PR逐步推进目标 |
2 participants
目标
修复离线/在线播放历史记录的同步与恢复问题，并保留离线播放时的原始线路语义，避免在线历史、离线历史、弹幕、下载状态和 SyncPlay 在同一番同一集上互相覆盖或错位。
修改的部分
历史记录同步
History.key按entryKindscoped，在线与离线历史分开保存和同步。Progress增加updatedAtMs，WebDAV merge 可按单集更新时间合并。播放与集数身份
ResolvedEpisode/ episode identity 边界，统一表达 listIndex、展示标题、源站 URL、历史集数、弹幕集数和原始 road。离线线路与 SyncPlay
DownloadEpisode.road分组，不再把所有下载集伪造成单个播放列表1。kazumi:v2SyncPlay 集数协议，携带bangumiId、roadIndex、listIndex、episodeNumber，同时兼容旧协议。下载兼容与恢复
episodePageUrl的情况。测试项目
flutter test test/resolved_episode_test.dart test/syncplay_episode_codec_test.dartflutter test test/history_repository_test.dart test/history_sync_test.dartgit diff --check origin/main...HEADflutter analyze后续可能继续修改的功能
episodeNumber、currentEpisode、roadList.data、identifier对列表位置、真实集号、源站 URL 和展示标题的混用。