docs: code review#3530
Conversation
🔗 Link this PR to an issueThis PR doesn't reference a tracking issue yet. These open issues look related:
To link: edit the PR description and add This comment and the |
|
|
||
| **Reference commit:** [`4f2834ddb`](https://github.com/divinevideo/divine-mobile/commit/4f2834ddb529487020333feea8e269c6fa19bfbc): `feat(feed): move captions control into more info (#3105)` (2026-04-16) | ||
|
|
||
| > **Note:** All file paths, line numbers, and code snippets in the issue files below were captured at the reference commit. Since `main` continues to evolve, some references may be outdated — files may have moved, lines shifted, or code changed. When acting on an issue, verify against the current state of the codebase. |
There was a problem hiding this comment.
Worth calling out a couple of same-day merges here so readers don’t over-index on the April 16 snapshot. As of April 28, 2026, #3522 (feed-item BlocProvider repo-identity fix), #3533, and #3536 (new Riverpod→Bloc capture rules) have already landed, so some examples in the audit are now better read as "fixed incident / remaining class of risk" rather than "currently broken on main." A short note here would make the document age better.
|
|
||
| --- | ||
|
|
||
| ### Inconsistent serialization approaches across models |
There was a problem hiding this comment.
This one looks stale after #3514, which merged earlier today. VideoEvent no longer uses json_serializable; the generated video_event.g.dart was removed and toJson is now a hand-written allow-list. I’d either drop this issue or rewrite it as a smaller point about standardizing the remaining manual serialization patterns, because the specific "one model still uses codegen" claim is no longer true on main.
|
|
||
| Issues related to layer violations, dependency direction, state management patterns, and project organization. | ||
|
|
||
| Note: The layered architecture (`UI → BLoC → Repository → Client`) is established in 42 BLoC directories and packages like `videos_repository` and `comments_repository`. These issues cover the structural gaps — primarily the 140-file `services/` directory that bypasses the BLoC layer, three concurrent state management patterns, and missing documentation for data source and caching strategy. |
There was a problem hiding this comment.
This "missing documentation" part is a bit narrower after today’s merges. #3533 and #3536 just added explicit docs for the Riverpod-captured-dependency → Bloc trap that caused #3503, so for that slice the gap is now more about adoption/enforcement than absence of guidance. I’d update this sentence or add a note so the architecture section doesn’t overstate the current-docs gap.
NotThatKindOfDrLiz
added
do-not-merge
Draft review notesThis is docs-only and useful as an audit package, but the current draft framing is right: it should not merge as-is. Findings
ReadinessBefore marking ready: fill out the PR body, resolve the issue-link language, replace |
rabble
force-pushed
the
docs/repo-recommendations
branch
from
5fe79bf to
2a1b6ed
Compare
Mobile PR PreviewPreview refreshed for Last refresh:
|
* docs(code-review): extract simplicity audit from #3530 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> * docs(code-review): reconcile simplicity audit per review 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. --------- Co-authored-by: Oscar Martin <martinm.oscar@gmail.com>
|
Closing (not because all of this work has been addressed) but just to keep the PR queue clean. The corresponding issues continue to be worked through as we build. Thank you, again, @omartinma for all of the effort, expertise, and care that went into this code review. |
3 participants
DO NOT MERGE YET
Description
Related Issue: Closes # or Relates to #
Type of Change