Repository navigation
refactor(upload): move the upload core into its own package - #9120
NotThatKindOfDrLiz merged 28 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
The extraction appears behavior-preserving, and all current checks are green. I found two package-boundary issues that should be resolved before merge:
-
mobile/scripts/check_service_god_file_ceiling.sh:32only scansmobile/lib/services, while this PR removes the oldupload_manager.dartbaseline entry and moves the still-oversized implementation tomobile/packages/upload_repository/lib/src/upload_repository.dart. That leaves the 1,868-line core with no mechanical growth ceiling. Deferring the split to #4339 is reasonable, but please preserve an equivalent package baseline/provenance constraint or deliberately broaden the guard so the file cannot grow while that work remains outstanding. -
mobile/test/services/upload/pending_upload_store_test.dart:13directly tests the newly packagedPendingUploadStore, but its roughly 987 lines remain outside the package test tree and therefore do not contribute to the package's 40% coverage floor. The store already accepts an injected box opener. Please move the core store coverage into the package and retain only app/plugin adapter integration coverage in the app, or document the concrete dependency that prevents establishing that boundary.
Please also clean up the small extraction artifacts while touching this area:
- Point the package reporter tests at the static APIs and remove the test-only instance delegators at
mobile/packages/upload_repository/lib/src/upload_progress_reporter.dart:154andmobile/packages/upload_repository/lib/src/upload_progress_reporter.dart:313. - Restore the intentional no-op rationale removed from
mobile/lib/services/upload_manager.dart:182andmobile/lib/services/upload_manager.dart:185. - Remove the carried-forward commented-out
// rethrow;atmobile/packages/upload_repository/lib/src/upload_repository.dart:244. - Replace "pure-Dart" at
mobile/lib/services/upload_manager.dart:1with the accurate "no direct Flutter dependency" wording;nostr_sdkandunified_loggerremain Flutter-dependent packages.
Afterward, please rerun package tests with coverage, the app upload suites, analysis, generated-file/ratchet checks, and required CI.
|
@NotThatKindOfDrLiz, all six items are addressed at
Dispositions:
Validation at this head: Required CI for the head has not finished yet. @realmeylisdev already has a pending review request and the head moved, so please both take a look at the updated state. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
realmeylisdev
left a comment
realmeylisdev
left a comment
There was a problem hiding this comment.
@dcadenas I reviewed 91bfa7b1a and pushed nine commits to this branch (head is now a7515646b0), one per finding so you can revert any of them on its own. The extraction itself is clean — everything below is boundary hygiene and dead code in the seams, not the move.
Pushed
Package boundary — the cluster worth looking at first
0cb168fb19The facade traded three narrowshowre-exports forexport 'package:upload_repository/upload_repository.dart', so all 35 files importingupload_manager.dartgotPendingUploadStore,UploadRetryPolicy,UploadProgressReporter,VideoCircuitBreakerand the Hive box opener in scope. That reopens, at the facade, the boundary this PR exists to draw, and nothing catches it:check_layer_direction.shonly rejectsproviders/services/repositories/stateimportingscreens/widgets/router, not UI reaching storage internals. It now re-exports an eight-name allowlist, and the seven app files that had swapped a narrow model import for the whole barrel carry ashowclause for the one or two names each uses.flutter analyze lib test integration_testis clean, which is the check that the allowlist is complete.4f449d0bcdThumbnailExtractorwas declared in both libraries, so the facade imported the package twice (oncehide, once aliasedshow), re-exported it with ahide, and bridged the two. Two test files already import both libraries; the first reference to that name from either would have been an ambiguous import naming two declarations that read identically in source. Nothing outside the facade ever named the app typedef, so it is nowVideoThumbnailExtractorand the hide/alias plumbing is gone. The adapter stays — your port carries just the path andVideoThumbnailServicereturns a timestamp with it.
Dead code and test-only surface
0536762e5eUploadConnectivity.otherhad no producer anywhere._checkConnectivityreturnsnonefor everything it does not recognise, which is whatUploadProgressReporter.checkNetworkConnectivitydid before the extraction, so the'Unknown'network-type string and the'ConnectivityResult.other'crash-report value were both unreachable. Removing the member keeps both switches exhaustive and the reported values identical.edfcbd53a9_checkConnectivityended withif (results.contains(none)) return none;immediately abovereturn none;.4f44c9ddf0UploadManager.getUserFriendlyErrorMessageand its private_mapConnectivityhad no production callers — only 15 call sites across two app test files. That is the shapecc6b20c81deleted fromUploadProgressReporter, re-created one layer up, and it made a secondConnectivityResult -> UploadConnectivitymapping something to keep in step with_checkConnectivity. Both suites now callUploadProgressReporter.userFriendlyErrorMessagedirectly, the way the package's own tests already do; the drift guard still guards the same copy. The classification suite no longer needs anUploadManager, so its fixture went too.bb29c6572dunawaited()wrapped threeMap<String, Future<void>>.remove()calls. The value those return is the future being removed — the one whosewhenCompleteis running — not a newly started operation, so the wrapper silenceddiscarded_futureswhile asserting the opposite. A wildcard discard says what is true and satisfies the lint without suppressing it, so a genuinely dropped future there would still be caught. Kept to one line soupload_repository.dartstays at exactly its 1,867-line ceiling.
Duplication and CI
fe1e208261UploadPhasesrestated six telemetry stringsPublishPhasesalready owned, with a new test asserting the six pairs equal. That is a test compensating for duplication: it covered only the copied six, so a seventh phase on either side would have been unguarded. The package now owns the names andPublishPhasesrefers to them, so divergence is a compile error; the parity test goes with the duplication it policed.e83af3d0f7This one touches your most recent commit, so to be explicit: it keeps the whole-tree provenance match you added in91bfa7b1aand only drops--find-copies-harder, which asks git to consider every unmodified file in the repo as a copy source. Git answerswarning: you may want to set your diff.renameLimit variable to at least 7243and degrades to "only found copies from modified paths" anyway. A claimed source is a modified path by definition — the move either deletes it or leaves a facade — so plain--find-copiesfinds the same pair. Measured here: identicalC087 lib/services/upload_manager.dart -> packages/upload_repository/lib/src/upload_repository.dart, 0.02s instead of 0.55s, without the two spurious copy pairs the harder scan invented between unrelated new files. The guard still honours the claim.a7515646b0Two things I found re-reading my own diff: theclassifyPublishErrorMessagedartdoc namedvideo_publish_service_test.dartas its drift guard, which contains no such assertion (it has always beenpublish_error_classification_test.dart); and my export comment overstated the allowlist, since the inheritedcircuitBreakerparameter takes aVideoCircuitBreakerthat is deliberately not re-exported because nothing passes one.
Not pushed
The god-file guard covers one package by name. EXTRA_SCAN_DIRS defaults to packages/upload_repository/lib, and [[ -d "$dir" ]] || continue means it sees exactly that one. Nineteen files under mobile/packages/*/lib are already over 1,500 lines and stay invisible — dm_repository.dart at 9,283, relay_pool.dart at 3,739, blossom_upload_service.dart at 3,466 and so on.
I am deliberately not broadening it here, and I want to be clear it is not a defect in this PR. At the merge base the guard scanned only mobile/lib/services, so packages were never covered; this PR narrows that gap by one rather than opening it. @NotThatKindOfDrLiz's review offered "preserve an equivalent package baseline/provenance constraint or deliberately broaden the guard", and you took the first option, which is a legitimate reading. Broadening it properly means globbing packages/*/lib and seeding nineteen ceilings — including a 10,118-line generated app_database.steps.dart the current exclusions do not filter — which is a separate change with its own review. Worth a follow-up issue; happy to file one if you want it tracked.
Package coverage floor at 40 against 56.4 measured. I measured 56.44% at this head. You have already explained the 40 to Liz — floors are not raised automatically and #4339 will move covered code back out — so this is a note rather than a request. The cost is that coverage can fall sixteen points with CI green and no signal. Either way I could not have changed it: .github/workflows/upload_repository.yaml is a platform-sensitive path and I am on @divinevideo/reviewers, not @divinevideo/platform, so that file is outside my branch-modification authority.
lib/hive_registrar.g.dart — I was wrong about this one. I had it down as a dead file in the package's public lib/ root with no callers. It is dead in the sense that nothing calls registerAdapters() (the real registration is explicit in upload_initialization_helper.dart:148,151), but deleting it is not the fix: hive_ce_generator:hive_registrar_generator re-emits it for any package containing a @HiveType. I deleted it, ran dart run build_runner build in the package, and it came straight back byte-identical with git status clean. Its location is the generator's choice, and AGENTS.md names hive_registrar.g.dart among the artifacts to commit. Leaving it as is.
One finding folded into another: the PendingUpload import deleted from video_event_publisher.dart and video_publish_service.dart is resolved by the allowlist rather than separately. With the whole package re-exported, those two files depending on the re-export was accidental; with a deliberate eight-name allowlist it is the declared contract, and adding a second import for names the facade already re-exports trips unnecessary_import.
Validation at a7515646b0
dart formatclean on all 22 changed Dart files;flutter analyze lib test integration_testand the package'slib testboth clean.- Package: 190 tests pass,
flutter test --coverage841/1490 lines = 56.44% against the 40 floor. - App: the publish, upload-progress, provider, model and upload suites, plus every touched test file — 1,767 and 677 in the two runs, all pass. The pre-push hook ran the affected suites again on the way out.
- 25 ratchets run directly, all OK, including
check_service_god_file_ceiling.sh(rename claim still honoured, 1,867 unchanged),check_layer_direction,check_package_*, and the test-quality set. - CI was green on
91bfa7b1abefore this push, so anything red now is mine to fix.
@NotThatKindOfDrLiz your review sits on 8cb18f66f and the head has moved twice since — you already have a pending request, so I have not created a duplicate one, but the six items you raised are worth a look at the current state. @dcadenas please re-read the nine commits above and revert anything you disagree with; the merge decision stays yours.
|
@realmeylisdev, re-read all nine commits and kept every one; none reverted. The allowlist export, the A second independent review of
Validation at the final head:
No merge or approval from me; the state is open for review. @NotThatKindOfDrLiz your review sits on |
This comment has been minimized.
This comment has been minimized.
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Re-reviewed at 6e33b55. The six issues from my prior review are resolved, the subsequent boundary-cleanup commits are sound, and all required and relevant checks passed on this exact head. Ready to merge.
|
Overlap heads-up: this package extraction intersects #9108 in upload_manager.dart and recovery tests, #9102 across ten Hive-sensitive upload tests, and #9147 in async-safety baselines and agent guidance. This is the common collision point. Please coordinate merge order and rebase after the smaller fixes land, preserving disposed-manager behavior, Hive isolation, behavioral coverage, and the reduced guardrail baselines. |
|
Synchronized the branch with The merge takes
Validation at
@realmeylisdev @NotThatKindOfDrLiz this push makes the existing approvals stale, so please take another look. On the overlap heads-up: #9108 is still open and touches the same file, so whichever of the two lands second will need a small reconciliation; #9102, #9147, #9150 and #9175 are already in this branch. |
This comment has been minimized.
This comment has been minimized.
|
@dcadenas, please rebase this branch onto fresh |
|
Updated overlap heads-up: this package extraction also intersects #9057 in upload initialization/provider cleanup and draft #8467 in video publishing and its tests. Please coordinate merge order; after either lands, rebase onto fresh origin/main and preserve Hive-open isolation, package boundaries, audio-reuse policy, and publishing coverage. |
`UploadConnectivity.other` had no producer. `UploadManager._checkConnectivity` returns `none` for every result it does not recognise, which matches what `UploadProgressReporter.checkNetworkConnectivity` did before the extraction, so the `'Unknown'` network-type string and the `'ConnectivityResult.other'` crash-report value were both unreachable. Removing the member keeps both switches exhaustive, preserves the reported values exactly, and stops every future switch on this enum from paying for a case that cannot occur.
`_checkConnectivity` ended with `if (results.contains(none)) return none;` immediately followed by `return none;`, so the branch could never alter the result. It also made the priority ladder read as if an explicit "no network" answer were distinguished from an unrecognised one, which it is not.
`UploadManager.getUserFriendlyErrorMessage` and its private `_mapConnectivity` had no production callers: the app reaches the copy through `UploadRepository`, and these two existed purely so two app test files could keep their old call shape. That is the shape cc6b20c deleted from `UploadProgressReporter`, re-created one layer up, and it made a second `ConnectivityResult -> UploadConnectivity` mapping something we had to keep in step with `_checkConnectivity`. Both test files now call `UploadProgressReporter.userFriendlyErrorMessage` directly, the way the package's own tests already do, so the drift guard still guards the same copy without an app-side shim to carry it. The classification suite no longer needs an `UploadManager` at all, so its fixture and the imports that fed it go too.
The facade declared `ThumbnailExtractor` while the package declares one too, so the library imported the package twice -- once `hide`, once aliased `show` -- re-exported it with a `hide`, and bridged the two with an adapter. Two files already import both libraries; the first reference to that name from either of them would have been an ambiguous import naming two declarations that look identical in source. Nothing outside this library ever named the app typedef, so renaming it to `VideoThumbnailExtractor` removes the collision and the double import, alias and hide that existed only to work around it. The adapter stays: the package's port carries just the path, and `VideoThumbnailService` returns a timestamp with it.
The facade replaced three narrow `show` re-exports with `export 'package:upload_repository/upload_repository.dart'`, so all 35 files importing `upload_manager.dart` got `PendingUploadStore`, `UploadRetryPolicy`, `UploadProgressReporter`, `VideoCircuitBreaker` and the Hive box opener in scope. That reopens, at the facade, the boundary this PR exists to draw, and nothing guards it: check_layer_direction.sh only rejects providers/services/repositories/state importing screens/widgets/router, not UI reaching storage internals. Re-export only the upload vocabulary that appears in `UploadManager`'s own signatures, and give the seven app files that had swapped a narrow model import for the whole barrel a `show` clause for the one or two names each actually uses. The Hive adapters are an initialization concern, so the helper and the five suites that register them import them directly rather than reaching them through the facade. `flutter analyze lib test integration_test` is clean, which is the check that the allowlist is complete.
`UploadPhases` restated six telemetry strings that `PublishPhases` already owned, and a new test asserted the six pairs were equal. That is a test compensating for duplication rather than a single source of truth: it covered only the six copied constants, so a seventh phase added to either side would have been unguarded, and a key drift would surface as a failing test rather than as a compile error. The upload leg is timed inside the package, so the package owns the names and `PublishPhases` refers to them. Divergence is now impossible to write, and the parity test goes with the duplication it existed to police.
`unawaited()` wrapped three `Map<String, Future<void>>.remove()` calls. The value those return is the future being removed -- the same one whose `whenComplete` callback is running -- not a newly started operation. The wrapper silenced `discarded_futures` while asserting the opposite of what is happening, so anyone auditing fire-and-forget work had three false positives to rule out. A wildcard discard says what is actually true: the returned value is thrown away. It also satisfies the lint without suppressing it, so a genuinely dropped future here would still be caught.
The provenance check ran `git diff` with `--find-copies-harder` and no pathspec, which asks git to consider every *unmodified* file in the repo as a candidate copy source. On this branch git answers with `warning: you may want to set your diff.renameLimit variable to at least 7243` and degrades to "only found copies from modified paths" anyway. A claimed rename source is a modified path by definition -- the move either deletes it or leaves a facade behind -- so plain --find-copies finds the same pair. Measured on this branch: identical `C087 lib/services/upload_manager.dart -> packages/upload_repository/lib/src/upload_repository.dart`, 0.02s instead of 0.55s, and without two spurious copy pairs the harder scan invented between unrelated new files. The guard still honours the claim.
Two follow-ups from re-reading this branch's own diff: The `classifyPublishErrorMessage` dartdoc named `video_publish_service_test.dart` as the drift guard, which contains no such assertion -- the guard has always been the category table in `publish_error_classification_test.dart`. Wrong enough to send the next reader to the wrong file. The export comment claimed the allowlist covers everything in `UploadManager`'s signatures, which overstates it: the inherited `circuitBreaker` parameter takes a `VideoCircuitBreaker` that is deliberately not re-exported, because nothing passes one. Say what the list is instead -- what callers use -- so reaching past the facade stays a visible act rather than looking like an oversight.
935c851 to
26f83bd
Compare
|
@dcadenas I completed the authorized takeover and rebased this branch onto the current Current head: All review items are fixed; none were declined or escalated. All 23 exact-head checks have completed successfully, aside from the expected skipped daily service-result publication job. The branch is clean, linear, and GitHub reports it mergeable. Please review the takeover changes and merge when you are ready; the merge decision remains yours. @hm21 and @realmeylisdev are still requested reviewers. |
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Approved at 26f83bd4882f4bbabd099c43d19b1ba7e45db9f5 after the takeover rebase onto current origin/main. The reconciliation preserves #9057 Hive-opening observability and #9108 disposal/race protections, and the remaining Hive test mutation is isolated. All exact-head checks passed (with only the expected daily service-result publication job skipped). Ready to merge.
Description
The upload core had already been decomposed inside the app, but it could not be reused or tested as a package of its own, and its tests could not run without the full app. This change moves the upload core (orchestration, persistence, retry, recovery, and the pending-upload model) into
mobile/packages/upload_repository, leaving the Flutter, plugin, and lifecycle wiring in the app as a thin facade.Related Issue: Closes #8301
What Changed
upload_repositorypackage with its own analysis config, tests, Hive adapters, and CI workflow. Its pubspec declares no Flutter runtime dependency; the direct dependencies aredio,hive_ce,meta, and the in-workspaceblossom_upload_service,models,nostr_sdk, andunified_logger.UploadManagerstays in the app and keeps theBackgroundAwareServiceregistration and recovery wiring.PublishPhasesrefers to them, so a rename on either side is a compile error rather than something a test has to catch.SERVICE_GOD_FILE_EXTRA_SCAN_DIRSand accepts a verified Git rename or copy from the modified-path diff for the provenance claim. The baseline row ispackages/upload_repository/lib/src/upload_repository.dartat 1,867 lines with# renamed-from: lib/services/upload_manager.dart, so the file cannot grow past the ceiling retired byupload_manager.dartwhile the epic(maintainability): reduce oversized files, complexity, and general code drag #4339 split is open.showclauses. The app-side thumbnail typedef is nowVideoThumbnailExtractor, distinct from the package'sThumbnailExtractor, which removes the hide/alias import plumbing.UploadConnectivity.other(no producer), an unreachable connectivity branch, andUploadManager.getUserFriendlyErrorMessagewith its private mapping. The suites call the package reporter's static API directly, and removed map entries use a wildcard discard instead ofunawaited(...), so the lint stays enabled and a genuinely dropped future would still be caught.PendingUploadStoreon a package-local Hive home and an injected opener port, and the app keeps 3 adapter integration tests.// rethrow;is deleted, and the "pure-Dart" wording is corrected to "no direct Flutter dependency" in the facade, the package dartdoc, and the stale reference incrash_reporter.dart.Out of Scope
PendingUploadtype in the sharedmodelspackage is unchanged except that its dedup TODO now points at the moved copy.Verification
dart format --set-exit-if-changedandflutter analyze lib test integration_test tools: clean. At the merged head (8537ef1d), the full plainflutter testrun reports 20,520 pass, 52 skip, and 3 fail, all three in the Docker-dependenttest/manual/acceptance suite that CI tags out.DIVINE_STRICT_CHANNELS=true: 147 pass, 9 skipped.packages/upload_repository:flutter analyzeclean;flutter test --coverage: 190 tests, 840/1490 lines (56.4%) against the 40% floor.packages/models:flutter analyzeclean;flutter test: 972 tests pass.6e33b55): all checks green, one skipped, none failing, including Analyze, Format, Generated Files, Goldens, all four test shards, Integration Tests (services), and both build jobs. The head has since mergedmain, and required CI runs again for the merged head..codex/scripts/test-config.shand the.github/scripts/testssuite (110 tests): pass.Validation gaps: mergeability and semantic-title are reusable CI workflows and not locally runnable. The unfiltered
flutter testalso runs Docker-dependent manual acceptance tests undertest/manual/that CI excludes with--exclude-tags integration; those fail locally without the local stack and are unrelated to this change.Type of Change