Refactor: Migrate monorepo to Dart pub workspaces #4530
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis PR migrates the monorepo to a Dart pub workspace: the root pubspec.yaml declares a workspace and central dependency_overrides, and the root Dart SDK constraint is raised to >=3.9.0. Sub-packages adopt resolution: workspace, remove inline git blocks, and update dev-tool constraints. CI workflows now hash the root pubspec.lock for Flutter cache keys. scripts/prebuild.sh is refactored to run pub get and build_runner once at the workspace root and centralizes intl generation. Hive adapter registration is consolidated via generated bulk registration with guards to avoid duplicates. Redundant non-null assertions are removed in several source files. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0089-migrate-to-dart-pub-workspaces.md`:
- Line 15: Add language identifiers to the two fenced code blocks that show the
tmail-flutter directory trees (the triple-backtick blocks around "tmail-flutter/
├── pubspec.yaml ..."), changing ``` to ```text for both occurrences so the
blocks are tagged and MD040 is resolved; ensure both the first example block and
the second workspace declaration block use ```text.
In `@scripts/prebuild.sh`:
- Around line 20-21: The script uses chained `&&` between the intl_generator
invocations ("dart run intl_generator:extract_to_arb --output-dir=./lib/l10n
lib/main/localizations/app_localizations.dart > /dev/null 2>&1" and "dart run
intl_generator:generate_from_arb --output-dir=lib/l10n --no-use-deferred-loading
lib/main/localizations/app_localizations.dart lib/l10n/intl*.arb > /dev/null
2>&1"), which suppresses set -e errexit behavior on failure; change each
intl_generator invocation so it is on its own line (do not chain with &&) so a
failing `extract_to_arb` or `generate_from_arb` immediately stops execution and
preserves error handling; apply the same change for the later duplicate commands
(the other extract/generate pair) in the script.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: effa3610-d600-4594-8ac0-b63f137b2ecd
⛔ Files ignored due to path filters (12)
contact/pubspec.lockis excluded by!**/*.lockcore/pubspec.lockis excluded by!**/*.lockcozy/pubspec.lockis excluded by!**/*.lockemail_recovery/pubspec.lockis excluded by!**/*.lockfcm/pubspec.lockis excluded by!**/*.lockforward/pubspec.lockis excluded by!**/*.locklabels/pubspec.lockis excluded by!**/*.lockmodel/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lockrule_filter/pubspec.lockis excluded by!**/*.lockscribe/pubspec.lockis excluded by!**/*.lockserver_settings/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/workflows/analyze-test.yaml.github/workflows/build.yaml.github/workflows/gh-pages.yaml.github/workflows/release.yamlcontact/pubspec.yamlcore/pubspec.yamlcozy/pubspec.yamldocs/adr/0089-migrate-to-dart-pub-workspaces.mdemail_recovery/pubspec.yamlfcm/pubspec.yamlforward/pubspec.yamllabels/pubspec.yamllib/features/caching/caching_manager.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/mailbox_dashboard/presentation/sentry_ecosystem.dartmodel/pubspec.yamlpubspec.yamlrule_filter/pubspec.yamlscribe/pubspec.yamlscripts/prebuild.shserver_settings/pubspec.yaml
dab246
force-pushed
the
migrate-dart-pub-workspaces
branch
from
876b8ae to
76db6d7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/prebuild.sh (1)
15-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSplit
intl_generatorcommands;&&weakens fail-fast behavior withset -e.At Line 15 and Line 23, a failing
extract_to_arbcan bypass immediate exit semantics and the script may continue to later "Done" logs with stale l10n outputs. Run each command on its own line.Suggested patch
-dart run intl_generator:extract_to_arb --output-dir=./lib/l10n lib/main/localizations/app_localizations.dart > /dev/null 2>&1 && - dart run intl_generator:generate_from_arb --output-dir=lib/l10n --no-use-deferred-loading lib/main/localizations/app_localizations.dart lib/l10n/intl*.arb > /dev/null 2>&1 +dart run intl_generator:extract_to_arb --output-dir=./lib/l10n lib/main/localizations/app_localizations.dart > /dev/null 2>&1 +dart run intl_generator:generate_from_arb --output-dir=lib/l10n --no-use-deferred-loading lib/main/localizations/app_localizations.dart lib/l10n/intl*.arb > /dev/null 2>&1 @@ - dart run intl_generator:extract_to_arb --output-dir=./lib/scribe/ai/l10n lib/scribe/ai/localizations/scribe_localizations.dart > /dev/null 2>&1 && - dart run intl_generator:generate_from_arb --output-dir=lib/scribe/ai/l10n --no-use-deferred-loading lib/scribe/ai/localizations/scribe_localizations.dart lib/scribe/ai/l10n/intl*.arb > /dev/null 2>&1 + dart run intl_generator:extract_to_arb --output-dir=./lib/scribe/ai/l10n lib/scribe/ai/localizations/scribe_localizations.dart > /dev/null 2>&1 + dart run intl_generator:generate_from_arb --output-dir=lib/scribe/ai/l10n --no-use-deferred-loading lib/scribe/ai/localizations/scribe_localizations.dart lib/scribe/ai/l10n/intl*.arb > /dev/null 2>&1#!/usr/bin/env bash set -euo pipefail echo "1) Demonstrate bash errexit behavior for left side of &&" bash -c 'set -e; false && echo SHOULD_NOT_PRINT; echo STILL_RUNNING' echo "2) Inspect the relevant section in prebuild script" nl -ba scripts/prebuild.sh | sed -n '12,26p'Also applies to: 23-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/prebuild.sh` around lines 15 - 16, The combined pipeline using "dart run intl_generator:extract_to_arb ... > /dev/null 2>&1 && dart run intl_generator:generate_from_arb ..." in scripts/prebuild.sh weakens set -e fail-fast behavior; split these into two separate commands (one per line) so a failing extract_to_arb immediately causes the script to exit, and do the same for the generate_from_arb invocation elsewhere (the occurrences of "extract_to_arb" and "generate_from_arb" should be run on their own lines rather than chained with &&).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/caching/config/hive_cache_config.dart`:
- Around line 127-145: The closeHive method is resetting the
adapter-registration guards which causes duplicate adapter registration later;
update closeHive({bool isolated = true}) to stop modifying
_isolatedAdaptersRegistered and _regularAdaptersRegistered (i.e., remove the
lines that set them to false) and only await IsolatedHive.close() or
Hive.close() respectively so that _registerAdapter() remains the authoritative
guard for adapter registration.
---
Duplicate comments:
In `@scripts/prebuild.sh`:
- Around line 15-16: The combined pipeline using "dart run
intl_generator:extract_to_arb ... > /dev/null 2>&1 && dart run
intl_generator:generate_from_arb ..." in scripts/prebuild.sh weakens set -e
fail-fast behavior; split these into two separate commands (one per line) so a
failing extract_to_arb immediately causes the script to exit, and do the same
for the generate_from_arb invocation elsewhere (the occurrences of
"extract_to_arb" and "generate_from_arb" should be run on their own lines rather
than chained with &&).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c175ac56-9e39-4b61-8046-967421db5832
⛔ Files ignored due to path filters (12)
contact/pubspec.lockis excluded by!**/*.lockcore/pubspec.lockis excluded by!**/*.lockcozy/pubspec.lockis excluded by!**/*.lockemail_recovery/pubspec.lockis excluded by!**/*.lockfcm/pubspec.lockis excluded by!**/*.lockforward/pubspec.lockis excluded by!**/*.locklabels/pubspec.lockis excluded by!**/*.lockmodel/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lockrule_filter/pubspec.lockis excluded by!**/*.lockscribe/pubspec.lockis excluded by!**/*.lockserver_settings/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/analyze-test.yaml.github/workflows/build.yaml.github/workflows/gh-pages.yaml.github/workflows/release.yamlcontact/pubspec.yamlcore/pubspec.yamlcozy/pubspec.yamldocs/adr/0090-migrate-to-dart-pub-workspaces.mdemail_recovery/pubspec.yamlfcm/pubspec.yamlforward/pubspec.yamllabels/pubspec.yamllib/features/caching/caching_manager.dartlib/features/caching/config/hive_cache_config.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/mailbox_dashboard/presentation/sentry_ecosystem.dartmodel/pubspec.yamlpubspec.yamlrule_filter/pubspec.yamlscribe/pubspec.yamlscripts/prebuild.shserver_settings/pubspec.yaml
✅ Files skipped from review due to trivial changes (1)
- docs/adr/0090-migrate-to-dart-pub-workspaces.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4530. |
|
I love -10K lines of code diffs :-) |
|
linagora/intl_generator#5 was merged, please update |
There was a problem hiding this comment.
Gates Passed
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.
updated |
3 participants
Why
Before this PR, each of the 11 sub-packages maintained its own
pubspec.lockand duplicateddependency_overridesfor git-sourced packages. That caused several problems:jmap_dart_client,linagora_design_flutter,intl_generator) required editing up to 12 files.prebuild.shranflutter pub get+build_runner11 times sequentially.What changed
Workspace migration
workspace:key to rootpubspec.yamllisting all 11 sub-packages.resolution: workspaceto each sub-packagepubspec.yaml; remove theirpubspec.lockfiles anddependency_overridesblocks.dependency_overrides(jmap_dart_client,linagora_design_flutter,intl_generator) into rootpubspec.yamlonly.pubspec.lockat the root now represents the entire monorepo.CI & build
analyze-test,build,gh-pages,release) fromhashFiles('**/pubspec.lock')tohashFiles('pubspec.lock').prebuild.sh: oneflutter pub get+ onedart run build_runner build --workspacereplaces 12 sequential invocations.Dependency updates
hive_ce2.11.32.16.0hive_ce_generator 1.11.2hive_generator(dev)2.0.0hive_ce_generator 1.11.2hive_ce; auto-generatesHiveRegistrarextensionhiveoverrideIO-Design-Team)hive_ce_generator 1.11.2useshive_cedirectlyworker_manager7.2.9(pub.dev)Code improvements
HiveCacheConfig._registerAdapter: replaced 22 manualregisterCacheAdaptercalls withIsolatedHive.registerAdapters()/Hive.registerAdapters()from the generatedhive_registrar.g.dart. New adapters are now picked up automatically.!operators insingle_email_controller.dart,sentry_ecosystem.dart, andcaching_manager.dartwhere flow analysis already proves non-null.Documentation
ADR-0090documenting the previous architecture, migration rationale, dependency changes, and consequences.Blocker
Summary by CodeRabbit
Chores
Bug Fixes
Refactor