docs(code-review): extract simplicity audit from #3530#4505
Conversation
Per the #4339 ratification thread, extracting the simplicity-audit artifact into a small standalone PR so it can land against `main` rather than waiting on the full #3530 docs draft. The other audit docs in #3530 (architecture, performance, error-handling, testing, etc.) belong under their respective epics (#4335 / #4336 / #4337) and will be extracted by those owners. Content copied verbatim from #3530 head `docs/repo-recommendations` to preserve attribution and keep this PR a pure extraction (no content edits). Numbers reflect the audit baseline at PR-open time; follow-up PRs can update them as decomposition lands. Co-authored-by: Oscar Martin <martinm.oscar@gmail.com>
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Requesting changes because this is being positioned as a source-of-truth debt artifact on main, but it currently lands with known inaccuracies. The main blockers are stale issue status references and contradictory baseline numbers inside the document itself. Please refresh those facts in this PR, then this should be fine to merge.
|
|
||
| **Effort**: High. Each oversized file requires a domain-specific decomposition strategy. Priority targets: `video_event_service` (self-documented 9-concern split), `share_video_menu` (move business logic into `ShareSheetBloc`), `auth_service` (extract key management, session lifecycle, profile ops). | ||
|
|
||
| **GitHub ticket**: [#3594](https://github.com/divinevideo/divine-mobile/issues/3594) |
There was a problem hiding this comment.
#3594 is already closed, and the same problem appears again later with #3595, #3596, and #3598. If this doc is meant to be the current simplicity debt artifact on main, it should not present already-completed work as still-outstanding remediation. Please refresh these ticket statuses or reframe the sections so they are clearly historical baseline items rather than active debt tickets.
There was a problem hiding this comment.
If this doc is meant to be the current simplicity debt artifact on
main, it should not present already-completed work as still-outstanding remediation. Please refresh these ticket statuses or reframe the sections so they are clearly historical baseline items rather than active debt tickets.
Done in 565d804 — went with both framings together:
- Added an "April 2026 snapshot" preamble at the top of the doc that points readers at epic epic(maintainability): reduce oversized files, complexity, and general code drag #4339 for live status and explicitly notes the Waves in that epic supersede the per-ticket links below where applicable.
- Annotated the four closed tickets inline with closure date and successor pointers so a reader doesn't have to click through to see status:
#3594— closed 2026-05-13; superseded by epic epic(maintainability): reduce oversized files, complexity, and general code drag #4339 Wave 2/4 children#3595— closed 2026-05-13; superseded by refactor(app): split main.dart and AppShell into focused modules #3337#3596— closed 2026-05-12; work landed#3598— closed 2026-05-13; work landed
- Left
#3597,#3599,#3600untouched — they're live.
| ### `main.dart` is a 1,801-line entry point with 7+ responsibilities | ||
| **Problem**: `main.dart` bundles startup orchestration, service initialization, deep link handling, provider wiring, logging configuration, and UI widgets into a single file. Each concern is tightly coupled to the rest, making the startup sequence hard to understand, test, or modify independently. | ||
|
|
||
| **Evidence**: `mobile/lib/main.dart` (1,801 lines, 84 imports) contains: |
There was a problem hiding this comment.
This section says main.dart is 1,801 lines, but the oversized-files evidence above lists main.dart as 1,784 lines. Please reconcile these counts so the audit does not contradict itself.
There was a problem hiding this comment.
This section says
main.dartis1,801lines, but the oversized-files evidence above listsmain.dartas1,784lines. Please reconcile these counts so the audit does not contradict itself.
Done in 565d804 — reconciled to 1,784 in the intro paragraph and the section heading, since that's the value in the granular Evidence list. No 1,801 references remain in the doc.
|
|
||
| **Done well**: The new `lib/notifications/` feature demonstrates the correct BLoC-based architecture. The replacement is built; it just needs to fully replace the old implementation. | ||
|
|
||
| **Impact**: Medium. Two notification systems running simultaneously; confusion about which is canonical; ~1,500 LOC ready for removal once the new system is fully verified. |
There was a problem hiding this comment.
This says ~1,500 LOC is ready for removal, while the effort line below says ~1,000 LOC removed. Please pick one baseline or explain why the two numbers differ.
There was a problem hiding this comment.
This says
~1,500 LOCis ready for removal, while the effort line below says~1,000 LOC removed. Please pick one baseline or explain why the two numbers differ.
Done in 565d804 — both numbers were measuring different things and I disambiguated them rather than dropping either. Impact now reads "~1,500 LOC of dual-system code in total (old screen + provider + wiring)" and Effort reads "~1,000 LOC net deletion after wiring updates" — so the relationship between total scope and net delete is explicit on the page.
Addresses blocking review on #4505: - Add April 2026 snapshot preamble pointing readers at epic #4339 for live status, since the audit baseline is intentionally pinned. - Annotate the four closed tickets (#3594, #3595, #3596, #3598) with their closure dates and successor references where applicable, so the doc does not present completed work as outstanding remediation. - Reconcile main.dart count to 1,784 (matches the granular Evidence value) in both the intro paragraph and the section heading. - Disambiguate the notification LOC numbers: ~1,500 is total dual-system scope, ~1,000 is net deletion after wiring updates.
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Re-reviewed the updated doc and prior feedback. The blocking factual issues are addressed: closed-ticket status is now framed correctly as a snapshot with live-status pointers, the main.dart line count is reconciled internally, and the notification LOC numbers are disambiguated. Approving.
2 participants
Description
Extracts
docs/code-review/issues-simplicity.mdfrom @omartinma's #3530 docs draft into a small standalone PR againstmain. Per the ratification thread on #4339, this is the source-of-truth artifact the maintainability epic references for debt repayment and decomposition expectations; landing it now unblocks the rest of the #4339 child-issue work without forcing the full #3530 docs draft to merge.Content initially copied verbatim from #3530 head
docs/repo-recommendations. Per review feedback on the initial commit, the following corrections were applied so the doc can ship as a source-of-truth artifact onmain:main.dartcount reconciled to1,784everywhere (the granular Evidence value); the stray1,801mentions are gone.~1,500= total dual-system scope,~1,000= net deletion after wiring updates.Numbers remain the April 2026 audit baseline; follow-up PRs can update them as decomposition lands.
Out of scope for this PR (intentional):
issues-architecture.md,issues-performance.md,issues-error-handling.md,issues-testing.md,issues-ci-cd.md, etc.) — they belong under their respective epics (epic(data-foundation): unify caching, persistence, and platform dependency strategy #4335 / epic(reliability): standardize error handling, diagnostics, and failure contracts #4336 / epic(testing): reduce skipped coverage debt and stabilize test infrastructure #4337) and will be extracted by those epic owners.CODE_REVIEW.mdindex — would reference files that aren't shipped here; it can land once the rest of the audit docs are extracted by their owners.export_progress_widget.dartchange in docs: code review #3530 — separate concern, not in this PR's scope.Related Issue: Relates to #4339, supersedes one file from #3530.
Out of Scope
The other 13 audit docs in #3530 (see above). They belong under #4335 / #4336 / #4337.
Verification
diffagainst the source file in docs: code review #3530 head ref: differs only by the four review-driven corrections listed above.Type of Change
Checklist
main.Co-authored-bytrailer.