Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
tddang-linagora
left a comment
tddang-linagora
left a comment
There was a problem hiding this comment.
Both options are solid. Personally, I would want to replace GetX with Riverpod.
| |---|---|---| | ||
| | `ArchiveEmailAction extends EmailAction` | — | new file | | ||
| | `ArchiveBusHandler extends GetxService` | — | new file | | ||
| | Register in `BusHandlerRegistry` | 1-line entry | — | |
There was a problem hiding this comment.
This can be avoided with dependency injection as well I presume
| | Task | Edit | Create | | ||
| |---|---|---| | ||
| | `ArchiveEmailAction extends EmailAction` | — | new file | | ||
| | `ArchiveBusHandler extends GetxService` | — | new file | |
There was a problem hiding this comment.
Have a more specialized interface that GetXService
That interface actually can extend it.
More specialized interfaces préserve intent, helps di, etc...
|
|
||
| ### Positive | ||
|
|
||
| - **OCP-compliant** for adding new email actions — zero edits to existing files beyond a 1-line registry entry. |
There was a problem hiding this comment.
With Dependency Injection we should even be able to sneak this 1 line centralized registry entry and can turn it into a 1 line decentralized registry......
| ## Alternatives Considered | ||
|
|
||
| - **Keep `consumeState` + mixin** — fails OCP, controllers keep growing. | ||
| - **Migrate to Riverpod + service architecture** (see ADR 0077) — cleaner long-term boundaries but high effort and framework risk. |
There was a problem hiding this comment.
Stupid question: does this move makes moving in the future to riverpod easier? Or harder?
|
|
||
| - God-controllers are hard to test — setup requires spinning up DI graphs, bindings, and mocking `Get.context`. | ||
| - Cross-controller `Get.find` creates implicit, untyped dependencies. | ||
| - Integration tests are tied to GetX lifecycle (robots use `Get.put`). |
There was a problem hiding this comment.
That's a real problem.
If test depends on internal framework then it is not it tests.
Can we imagine writing such tests without getX?
Imo it is the most priority topic...
| - **High effort** — ~77 engineer-days across 16 phases. | ||
| - **High risk** — two DI systems coexist during months of migration; partial states possible on main. | ||
| - **Routing migration is invasive** — deferred loading, 15+ routes, web/mobile parity all at once. | ||
| - **Test suite rewrite** — all widget and integration tests must move off `Get.put` / robots. |
There was a problem hiding this comment.
:-/
MUST happen in priority to really protect the refactoring stage.
| - **Routing migration is invasive** — deferred loading, 15+ routes, web/mobile parity all at once. | ||
| - **Test suite rewrite** — all widget and integration tests must move off `Get.put` / robots. | ||
| - **Team learning curve** — Riverpod generator, code-gen workflow, `ref` lifecycle rules. | ||
| - **Feature-delivery slowdown** during routing and auth phases (2-3 week partial freeze likely). |
|
will address in mob session with team in next monday |
3 participants
2 Options: