Test/coverage check #464
Test/coverage check #464
Conversation
WalkthroughAdd CI coverage steps and a coverage-check script; document and ignore coverage artifacts; inject SharedPreferences into two notifiers; remove two toast-related public APIs and an extension method; and expand tests for persistence error handling and toast auto-dismiss behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as GitHub Actions
participant Flutter as Flutter Test
participant Script as check_diff_coverage.sh
participant Lcov as lcov
Dev->>CI: Push/PR
CI->>CI: checkout (fetch-depth: 0)
CI->>Flutter: run flutter test --coverage
CI->>Lcov: install/check lcov
CI->>Script: run ./scripts/check_diff_coverage.sh
Script->>CI: compute changed files vs base, extract LCOV for those files
Script-->>CI: return pass/fail based on per-diff coverage threshold
sequenceDiagram
participant UI as UI
participant Notifier as Theme/Profile Notifier
participant Prefs as SharedPreferences
UI->>Notifier: load / set / toggle / dismiss / reset
alt injected prefs provided
Notifier->>Prefs: use injected get/set/remove
else no injected prefs
Notifier->>Prefs: SharedPreferences.getInstance()
Notifier->>Prefs: get/set/remove
end
Notifier-->>UI: updated state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
lib/config/providers/theme_provider.dart (2)
22-31: Avoid exceptions and late-updates after dispose: validate index and guard with ref.mounted.
- If a stale/invalid int is stored, accessing ThemeMode.values[index] can throw; handle gracefully without exceptions.
- Because _loadThemeMode is async (scheduled via microtask), the provider may be disposed before it completes; guard updates with ref.mounted to avoid setting state after dispose.
- final themeModeIndex = prefs.getInt(_themePreferenceKey); - - if (themeModeIndex != null) { - final themeMode = ThemeMode.values[themeModeIndex]; - state = state.copyWith(themeMode: themeMode); - _logger.info('Loaded theme mode: ${themeMode.name}'); - } else { - _logger.info('No saved theme mode, using system default'); - } + final index = prefs.getInt(_themePreferenceKey); + if (index == null) { + _logger.info('No saved theme mode, using system default'); + return; + } + if (index < 0 || index >= ThemeMode.values.length) { + _logger.warning('Invalid saved theme mode index: $index; ignoring'); + return; + } + if (!ref.mounted) return; + final themeMode = ThemeMode.values[index]; + state = state.copyWith(themeMode: themeMode); + _logger.info('Loaded theme mode: ${themeMode.name}');
39-45: Don’t update state if persistence fails or if mode is unchanged; guard with ref.mounted.
- SharedPreferences.setInt returns Future. If it returns false (no exception), current code still updates state — diverging UI from persisted storage.
- Skip redundant writes if the mode is already selected.
- Guard against setting state after provider disposal.
- final prefs = _prefs ?? await SharedPreferences.getInstance(); - await prefs.setInt(_themePreferenceKey, themeMode.index); - state = state.copyWith(themeMode: themeMode); - _logger.info('Theme mode set to: ${themeMode.name}'); + final prefs = _prefs ?? await SharedPreferences.getInstance(); + if (state.themeMode == themeMode) { + _logger.fine('Theme mode unchanged; skipping save'); + return; + } + final saved = await prefs.setInt(_themePreferenceKey, themeMode.index); + if (!saved) { + _logger.warning('Failed to persist theme mode: ${themeMode.name}'); + return; + } + if (!ref.mounted) return; + state = state.copyWith(themeMode: themeMode); + _logger.info('Theme mode set to: ${themeMode.name}');
🧹 Nitpick comments (4)
scripts/check_diff_coverage.sh (2)
31-31: Fix shellcheck warning: declare and assign separately.Shellcheck warns about masking return values when declaring and assigning in one line.
- local tools_list=$(printf ", %s" "${missing_tools[@]}") + local tools_list + tools_list=$(printf ", %s" "${missing_tools[@]}")
76-77: Fix shellcheck warnings in temp file handling.Two shellcheck issues: declare/assign separately and trap quoting.
- local temp_lcov=$(mktemp) - trap "rm -f $temp_lcov" EXIT + local temp_lcov + temp_lcov=$(mktemp) + trap 'rm -f "$temp_lcov"' EXIT.github/workflows/ci.yml (1)
78-78: Remove trailing spaces.YAMLlint detected trailing spaces that should be removed.
- +lib/config/providers/theme_provider.dart (1)
10-13: Nit: Make logger static; align DI visibility across providers.
- Prefer a static logger to avoid per-instance creation; functional behavior is identical.
- Ensure DI shape (private vs public prefs field) is consistent across providers to reduce cognitive load.
- final _logger = Logger('ThemeNotifier'); + static final _logger = Logger('ThemeNotifier');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci.yml(1 hunks).gitignore(1 hunks)README.md(1 hunks)justfile(1 hunks)lib/config/extensions/toast_extension.dart(0 hunks)lib/config/providers/profile_ready_card_visibility_provider.dart(4 hunks)lib/config/providers/theme_provider.dart(2 hunks)lib/config/providers/toast_message_provider.dart(0 hunks)scripts/check_diff_coverage.sh(1 hunks)test/config/providers/profile_ready_card_visibility_provider_test.dart(3 hunks)test/config/providers/theme_provider_test.dart(2 hunks)
💤 Files with no reviewable changes (2)
- lib/config/extensions/toast_extension.dart
- lib/config/providers/toast_message_provider.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:39:00.467Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.467Z
Learning: Applies to **/*_test.dart : Use the standard widget testing for Flutter.
Applied to files:
test/config/providers/theme_provider_test.dart
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 78-78: trailing spaces
(trailing-spaces)
🪛 Shellcheck (0.10.0)
scripts/check_diff_coverage.sh
[warning] 31-31: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 76-76: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 77-77: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Flutter CI
🔇 Additional comments (15)
.gitignore (1)
57-59: LGTM!Adding coverage output files to .gitignore is the correct approach to prevent generated coverage data from being committed to version control.
README.md (1)
145-166: LGTM!The coverage documentation is well-structured and provides clear instructions for both prerequisites and usage. The commands and installation steps are accurate.
lib/config/providers/profile_ready_card_visibility_provider.dart (3)
8-11: LGTM!The dependency injection implementation for SharedPreferences is well-designed with proper optional constructor parameter and lazy initialization fallback.
16-16: LGTM!The lazy initialization properly uses the injected prefs or falls back to SharedPreferences.getInstance(), maintaining the existing behavior while enabling testability.
28-28: LGTM!Consistent usage of the injected prefs instance throughout all SharedPreferences operations maintains the dependency injection pattern correctly.
Also applies to: 42-42, 56-56
test/config/providers/profile_ready_card_visibility_provider_test.dart (3)
16-29: LGTM!The mock failing SharedPreferences implementation correctly simulates storage errors by throwing exceptions on write operations while providing appropriate fallbacks for other methods.
131-161: LGTM!The error handling test for dismissCard properly verifies that storage failures are handled gracefully, with the UI state still updating appropriately despite persistence errors.
242-272: LGTM!The error handling test for resetVisibility correctly ensures that even when SharedPreferences operations fail, the visibility state remains consistent and the app doesn't crash.
scripts/check_diff_coverage.sh (1)
1-174: LGTM with minor shellcheck fixes!The coverage checking script is well-structured with proper error handling, prerequisites checking, and comprehensive coverage validation. The logic for extracting changed files, calculating coverage, and enforcing thresholds is sound.
justfile (1)
105-111: LGTM!The coverage recipe properly integrates test execution with coverage validation, following established justfile patterns and providing clear user feedback.
.github/workflows/ci.yml (2)
70-70: LGTM!Adding coverage flag to the existing Flutter test step is the correct approach to generate coverage data in CI.
72-77: LGTM!Proper installation of lcov prerequisite and execution of the coverage validation script integrates well with the CI workflow.
test/config/providers/theme_provider_test.dart (2)
7-18: LGTM!The mock failing SharedPreferences correctly simulates storage errors while providing appropriate defaults for other operations, following the same pattern as the profile provider tests.
83-106: LGTM!The error handling test properly verifies that SharedPreferences failures don't crash the application and that the theme state remains consistent when persistence errors occur.
lib/config/providers/theme_provider.dart (1)
8-8: LGTM: Constructor-based SharedPreferences injection is clean and testable.This enables easy provider overrides in tests (e.g., overrideWith(() => ThemeNotifier(prefs: mock))). Good move.
josefinalliende
force-pushed
the
test/coverage-check
branch
3 times, most recently
from
fa28591 to
f4be10c
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
lib/config/providers/profile_ready_card_visibility_provider.dart (1)
16-16: Consider simplifying the lazy initialization logicThe current implementation uses both a public field
prefsand a private field_prefswith lazy initialization. This could be simplified to reduce complexity.Consider using only the private field with a getter:
- SharedPreferences? prefs; - final SharedPreferences? _prefs; + SharedPreferences? _prefs; + + Future<SharedPreferences> get prefs async { + _prefs ??= await SharedPreferences.getInstance(); + return _prefs!; + } @override Future<bool> build() async { - prefs ??= _prefs ?? await SharedPreferences.getInstance(); + await prefs; // Initialize if needed final activeAccountPubkey = ref.watch(activeAccountProvider);Then update the usage sites:
- prefs?.getBool('${_profileReadyCardDismissedKey}_$_currentPubKey') ?? false; + (await prefs).getBool('${_profileReadyCardDismissedKey}_$_currentPubKey') ?? false;scripts/check_diff_coverage.sh (3)
108-108: Improve coverage percentage extraction reliabilityThe current regex pattern might be fragile. Consider making it more robust.
- coverage_percentage=$(lcov --quiet --list "$temp_lcov" 2>/dev/null | grep -v "Message summary" | sed -n "s/.*Total:|\([^%]*\)%.*/\1/p") + coverage_percentage=$(lcov --quiet --list "$temp_lcov" 2>/dev/null | grep "Total:" | sed -n 's/.*Total:|\s*\([0-9.]*\)%.*/\1/p' | head -1)
148-149: Improve floating point comparison consistencyUse the same comparison pattern as suggested above for consistency.
- if [ -n "$coverage_percentage" ] && (($(echo "$coverage_percentage >= $MIN_COVERAGE" | bc -l))); then + if [ -n "$coverage_percentage" ] && [ "$(echo "$coverage_percentage >= $MIN_COVERAGE" | bc -l)" = "1" ]; then
174-174: Consider making the script more flexibleThe script immediately executes when sourced, which prevents it from being used as a library. Consider adding a main guard.
-check_diff_coverage "$BASE_BRANCH" "$FILE_PATTERN" "$MIN_COVERAGE" +# Only run if executed directly, not sourced +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + check_diff_coverage "$BASE_BRANCH" "$FILE_PATTERN" "$MIN_COVERAGE" +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/ci.yml(1 hunks).gitignore(1 hunks)README.md(1 hunks)justfile(1 hunks)lib/config/extensions/toast_extension.dart(0 hunks)lib/config/providers/profile_ready_card_visibility_provider.dart(4 hunks)lib/config/providers/theme_provider.dart(2 hunks)lib/config/providers/toast_message_provider.dart(0 hunks)scripts/check_diff_coverage.sh(1 hunks)test/config/providers/profile_ready_card_visibility_provider_test.dart(3 hunks)test/config/providers/theme_provider_test.dart(2 hunks)test/config/providers/toast_message_provider_test.dart(5 hunks)
💤 Files with no reviewable changes (2)
- lib/config/providers/toast_message_provider.dart
- lib/config/extensions/toast_extension.dart
✅ Files skipped from review due to trivial changes (1)
- .github/PULL_REQUEST_TEMPLATE.md
🚧 Files skipped from review as they are similar to previous changes (6)
- justfile
- README.md
- .gitignore
- test/config/providers/profile_ready_card_visibility_provider_test.dart
- test/config/providers/theme_provider_test.dart
- lib/config/providers/theme_provider.dart
🧰 Additional context used
📓 Path-based instructions (2)
**/*.dart
📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
test/config/providers/toast_message_provider_test.dartlib/config/providers/profile_ready_card_visibility_provider.dart
**/*_test.dart
📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)
**/*_test.dart: Follow the Arrange-Act-Assert convention for tests.
Name test variables clearly. Follow the convention: inputX, mockX, actualX, expectedX, etc.
Write unit tests for each public function. Use test doubles to simulate dependencies, except for third-party dependencies that are not expensive to execute.
Write acceptance tests for each module. Follow the Given-When-Then convention.
Use the standard widget testing for Flutter.
Files:
test/config/providers/toast_message_provider_test.dart
🪛 Shellcheck (0.10.0)
scripts/check_diff_coverage.sh
[warning] 31-31: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 76-76: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 77-77: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 72-72: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Flutter CI
🔇 Additional comments (6)
test/config/providers/toast_message_provider_test.dart (4)
486-521: Good test coverage for auto-dismiss functionality!The tests properly verify that auto-dismissable messages are removed after the specified duration while other messages remain intact. The setup and teardown are correctly handled.
543-552: Well-structured auto-dismiss tests across all toast typesThe auto-dismiss test coverage is comprehensive, testing each toast type (success, warning, info, error) with consistent patterns. Good use of async/await for timing verification.
648-705: Comprehensive showRawToast test coverageExcellent addition of tests for the showRawToast method, covering both raw message display without sanitization and auto-dismiss functionality. The tests properly verify that raw messages bypass the standard message processing.
707-753: Good coverage for dismissToast and dismissAll methodsThe tests properly verify the removal of specific toasts and clearing all toasts. Test structure follows AAA pattern consistently.
lib/config/providers/profile_ready_card_visibility_provider.dart (1)
8-12: Good implementation of dependency injection for SharedPreferencesThe constructor-based dependency injection pattern allows for better testability. The dual-field approach (public
prefsand private_prefs) provides flexibility for lazy initialization.scripts/check_diff_coverage.sh (1)
9-11: Good use of color formatting for user feedbackThe color-coded output functions enhance readability of the script's output. The implementation is clean and follows shell scripting best practices.
Also applies to: 13-16
lib/config/providers/profile_ready_card_visibility_provider.dart
Outdated
Show resolved
Hide resolved
josefinalliende
force-pushed
the
test/coverage-check
branch
13 times, most recently
from
712a812 to
6b5da49
Compare
josefinalliende
force-pushed
the
test/coverage-check
branch
from
6b5da49 to
5c8c834
Compare
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/config/providers/profile_ready_card_visibility_provider.dart (1)
8-13: LGTM: DI enables testability and fixes previous null-safety concernConstructor injection + assigning in build() removes the prior nullable prefs access and aligns with the testing pattern used elsewhere. This addresses the earlier “prefs?.getBool” comment.
Also applies to: 17-17
🧹 Nitpick comments (11)
.github/PULL_REQUEST_TEMPLATE.md (1)
31-32: Nit: tighten wording for coverage checklist itemMinor grammar/style tweak for clarity and consistency with “Flutter” as a proper noun.
- - [ ] Run `just check-flutter-coverage` to ensure that flutter coverage rules are passing + - [ ] Run `just check-flutter-coverage` to ensure Flutter coverage checks are passinglib/config/providers/profile_ready_card_visibility_provider.dart (2)
8-13: Make the injected SharedPreferences field privateThe injected field doesn’t need to be part of the public API. Making it private communicates intent and reduces surface area.
-class ProfileReadyCardVisibilityNotifier extends AsyncNotifier<bool> { - ProfileReadyCardVisibilityNotifier({SharedPreferences? sharedPreferences}) - : injectedSharedPreferences = sharedPreferences; - - late SharedPreferences sharedPreferences; - final SharedPreferences? injectedSharedPreferences; +class ProfileReadyCardVisibilityNotifier extends AsyncNotifier<bool> { + ProfileReadyCardVisibilityNotifier({SharedPreferences? sharedPreferences}) + : _injectedSharedPreferences = sharedPreferences; + + late SharedPreferences sharedPreferences; + final SharedPreferences? _injectedSharedPreferences; @@ - sharedPreferences = injectedSharedPreferences ?? await SharedPreferences.getInstance(); + sharedPreferences = _injectedSharedPreferences ?? await SharedPreferences.getInstance();Also applies to: 17-17
42-44: Remove stray blank lines inside functionsHouse style discourages blank lines within functions; these two can be dropped without hurting readability.
if (_currentPubKey == null || _currentPubKey!.isEmpty) { state = const AsyncValue.data(false); return; } - - await sharedPreferences.setBool('${_profileReadyCardDismissedKey}_$_currentPubKey', true); + await sharedPreferences.setBool('${_profileReadyCardDismissedKey}_$_currentPubKey', true); state = const AsyncValue.data(false); @@ if (_currentPubKey == null || _currentPubKey!.isEmpty) { state = const AsyncValue.data(true); return; } - - await sharedPreferences.remove('${_profileReadyCardDismissedKey}_$_currentPubKey'); + await sharedPreferences.remove('${_profileReadyCardDismissedKey}_$_currentPubKey'); state = const AsyncValue.data(true);Also applies to: 56-58
scripts/check_diff_coverage.sh (4)
191-191: Trap should use single quotes to avoid premature expansion (SC2064)Using double quotes expands the variable immediately. Prefer single quotes so the command is registered literally and expands at signal time.
- trap "rm -f $temp_file" EXIT + trap 'rm -f '"$temp_file" EXIT
4-7: Honor CI-provided base ref and allow overrides via envMake defaults configurable from the environment (e.g., GitHub’s GITHUB_BASE_REF) to avoid hardcoding “master”.
-MIN_COVERAGE=100 -FILE_PATTERN="provider" -BASE_BRANCH="master" +MIN_COVERAGE="${MIN_COVERAGE:-100}" +FILE_PATTERN="${FILE_PATTERN:-provider}" +# GITHUB_BASE_REF is set on pull_request events; fall back to “master” +BASE_BRANCH="${BASE_BRANCH:-${GITHUB_BASE_REF:-master}}"
45-50: Handle shallow clones in the script to avoid fetch-depth: 0 in CITeach the script to unshallow when needed. This lets the workflow use a shallow checkout while still computing a correct merge-base.
check_coverage_file_presence() { if [ ! -f "coverage/lcov.info" ]; then raise_error "Coverage file not found. Run 'flutter test --coverage' first." fi } +ensure_full_history() { + if git rev-parse --is-shallow-repository >/dev/null 2>&1; then + printf "Repository is shallow. Unshallowing...\n" + git fetch --no-tags --prune --unshallow || true + fi +} @@ check_diff_coverage() { printf "Checking coverage for changed files...\n" local base_branch base_branch=$1 @@ check_prerequisites check_coverage_file_presence + ensure_full_history fetch_base_branch "$base_branch"Also applies to: 176-186
163-171: Unify bc-based comparisons for robustnessElsewhere you already compare bc output against "1". Do the same here to avoid arithmetic context pitfalls.
- if [ -n "$coverage_percentage" ] && (($(echo "$coverage_percentage >= $min_coverage" | bc -l))); then + if [ -n "$coverage_percentage" ] && [ "$(echo "$coverage_percentage >= $min_coverage" | bc -l)" = "1" ]; then return 0 else return 1 fi.github/workflows/ci.yml (3)
71-75: Remove trailing whitespace and make lcov install more resilientDrop the trailing spaces and add an update + noninteractive install to reduce flakiness on ubuntu-latest images.
- - name: Run Flutter tests - run: flutter test --coverage - - - name: Install lcov - run: sudo apt-get install lcov + - name: Run Flutter tests + run: flutter test --coverage + + - name: Install lcov + run: sudo apt-get update && sudo apt-get install -y lcov
16-19: Optional: avoid full-history checkout; let the script unshallow as neededWith the script handling shallow clones, you can reduce checkout time by using the default depth here.
- with: - fetch-depth: 0 + # With scripts/check_diff_coverage.sh unshallowing when needed, + # default depth is sufficient here. + # with: + # fetch-depth: 0If you prefer to keep full history for other steps, feel free to ignore.
77-78: Optional: pass the PR base branch to the scriptThis ensures the diff is computed against the actual PR target when it’s not “master”.
- - name: Check coverage - run: ./scripts/check_diff_coverage.sh + - name: Check coverage + run: ./scripts/check_diff_coverage.sh "${{ github.base_ref }}" "provider" "100"Your script now uses env/arg fallbacks, so this will work for PRs and still fall back for pushes.
README.md (1)
145-155: Tweak coverage section wording and Linux install snippetMinor grammar fix and suggest including apt-get update + sudo for consistency.
-### Coverage - -You need to install lcov to generate report +### Coverage + +You need to install lcov to generate coverage reports. @@ -# Linux -apt-get install lcov +# Linux +sudo apt-get update && sudo apt-get install -y lcov
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks).github/workflows/ci.yml(2 hunks).gitignore(1 hunks)README.md(1 hunks)justfile(1 hunks)lib/config/extensions/toast_extension.dart(0 hunks)lib/config/providers/profile_ready_card_visibility_provider.dart(4 hunks)lib/config/providers/theme_provider.dart(2 hunks)lib/config/providers/toast_message_provider.dart(0 hunks)scripts/check_diff_coverage.sh(1 hunks)test/config/providers/profile_ready_card_visibility_provider_test.dart(3 hunks)test/config/providers/theme_provider_test.dart(2 hunks)test/config/providers/toast_message_provider_test.dart(5 hunks)
💤 Files with no reviewable changes (2)
- lib/config/extensions/toast_extension.dart
- lib/config/providers/toast_message_provider.dart
🚧 Files skipped from review as they are similar to previous changes (6)
- test/config/providers/theme_provider_test.dart
- test/config/providers/toast_message_provider_test.dart
- justfile
- .gitignore
- lib/config/providers/theme_provider.dart
- test/config/providers/profile_ready_card_visibility_provider_test.dart
🧰 Additional context used
📓 Path-based instructions (2)
**/*.dart
📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/config/providers/profile_ready_card_visibility_provider.dart
**/*.md
📄 CodeRabbit Inference Engine (.cursor/rules/whitenoise.mdc)
**/*.md: NIPs (Nostr Implementation Possibilities) are numbered likeNIP-XXwhereXXare two capitalized hexadecimal digits, e.g.,NIP-01andNIP-C7.
To read a specific NIP, construct the NIP URL following this template:https://raw.githubusercontent.com/nostr-protocol/nips/refs/heads/master/{nip}.md(replace{nip}in the URL template with the relevant NIP name, e.g.,07for NIP-07, orC7for NIP-C7).
To read the definition of a specific kind, construct a URL following this template:https://nostrbook.dev/kinds/{kind}.md(replace{kind}in the template with the kind number, e.g.,https://nostrbook.dev/kinds/0.mdfor kind 0).
Files:
README.md
🪛 YAMLlint (1.37.1)
.github/workflows/ci.yml
[error] 73-73: trailing spaces
(trailing-spaces)
🪛 LanguageTool
.github/PULL_REQUEST_TEMPLATE.md
[grammar] ~24-~24: There might be a mistake here.
Context: ...] 📝 Documentation - [ ] 🗑️ Chore - [ ] 🧪 Tests ## Checklist <!-- Please make...
(QB_NEW_EN)
[grammar] ~26-~26: There might be a mistake here.
Context: ...n - [ ] 🗑️ Chore - [ ] 🧪 Tests ## Checklist <!-- Please make sure you've done ...
(QB_NEW_EN)
README.md
[grammar] ~147-~147: There might be a mistake here.
Context: ...# Coverage You need to install lcov to generate report ```bash # Mac OS brew install lc...
(QB_NEW_EN)
🪛 Shellcheck (0.10.0)
scripts/check_diff_coverage.sh
[warning] 191-191: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
🔇 Additional comments (2)
.github/PULL_REQUEST_TEMPLATE.md (1)
24-24: Nice addition: explicit “Tests” change typeAdding a dedicated “🧪 Tests” type makes intent clearer in PRs touching only test assets.
README.md (1)
188-199: Docs/CI mismatch: branches differ between README and workflowREADME mentions triggers on “main” and “develop”, while the workflow triggers on “master” only. Please align docs or workflow to avoid confusion.
Would you like me to open a follow-up PR to update either the README or the workflow triggers?
| @override | ||
| Future<bool> build() async { | ||
| _prefs = await SharedPreferences.getInstance(); | ||
| sharedPreferences = injectedSharedPreferences ?? await SharedPreferences.getInstance(); |
There was a problem hiding this comment.
Awesome stuff, is this instance of the shared pref used outside of this ProfileReadyCardVisibilityNotifier class ? if not i think it makes more sense if its private annotated with a leading underscore
There was a problem hiding this comment.
Thanks for the feedback! It makes sense to add the underscore in this case, I'll add it
| # Linux | ||
| apt-get install lcov | ||
| ``` | ||
|
|
There was a problem hiding this comment.
show some love to windows 😅 they should get the lcov treatment too
There was a problem hiding this comment.
OMG, I don't know how to install anything on windows 😓 ...
Also me right now:
...
Chatgpt says choco install lcov ... is this correct? Do you have a windows to check it out?
There was a problem hiding this comment.
I don't own a windows myself which i probably should when we begin desktop rollout.
but after searching the internet i also found out choco works to install lcov.
There was a problem hiding this comment.
Ok so I'll add the choco 🍫 instructions haha
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/config/providers/profile_ready_card_visibility_provider.dart (2)
8-12: Make the injected SharedPreferences private and pre-initialize _sharedPreferences when provided (avoids late-init hazards).
- Keep the DI — great for testability.
- Per prior feedback, make the injected instance private (underscore prefix).
- If an instance is injected, assign it to
_sharedPreferencesin the constructor so methods called beforebuild()in tests/direct instantiation won’t hit a late-init error.Apply this diff:
- ProfileReadyCardVisibilityNotifier({SharedPreferences? injectedSharedPreferences}) - : injectedSharedPreferences = injectedSharedPreferences; - - late SharedPreferences _sharedPreferences; - final SharedPreferences? injectedSharedPreferences; + ProfileReadyCardVisibilityNotifier({SharedPreferences? injectedSharedPreferences}) + : _injectedSharedPreferences = injectedSharedPreferences { + // Pre-initialize when injected to avoid late-init errors in tests or direct calls. + if (_injectedSharedPreferences != null) { + _sharedPreferences = _injectedSharedPreferences!; + } + } + + late SharedPreferences _sharedPreferences; + final SharedPreferences? _injectedSharedPreferences;
29-29: Null-safety fix looks good.Using
_sharedPreferences.getBool(...)instead of the previous nullable access resolves the earlier “prefs?.…” risk noted in prior reviews.
🧹 Nitpick comments (4)
lib/config/providers/profile_ready_card_visibility_provider.dart (4)
17-17: Use the private field in build() for consistency with the refactor.Small follow-up to the above: switch to
_injectedSharedPreferences.Also, per repo guidelines “Always declare the type of each variable,” consider explicitly typing the variable on Line 18:
- Example:
final String? activeAccountPubkey = ref.watch(activeAccountProvider);- _sharedPreferences = injectedSharedPreferences ?? await SharedPreferences.getInstance(); + _sharedPreferences = _injectedSharedPreferences ?? await SharedPreferences.getInstance();
29-30: Avoid key duplication by extracting a helper.The key format is repeated in three places (Lines 29, 43, 57). Extracting a helper reduces duplication and typo risk.
Apply these diffs to call a helper:
- _sharedPreferences.getBool('${_profileReadyCardDismissedKey}_$_currentPubKey') ?? false; + _sharedPreferences.getBool(_prefsKeyFor(_currentPubKey!)) ?? false;- await _sharedPreferences.setBool('${_profileReadyCardDismissedKey}_$_currentPubKey', true); + await _sharedPreferences.setBool(_prefsKeyFor(_currentPubKey!), true);- await _sharedPreferences.remove('${_profileReadyCardDismissedKey}_$_currentPubKey'); + await _sharedPreferences.remove(_prefsKeyFor(_currentPubKey!));Add this helper (outside the selected ranges):
String _prefsKeyFor(String pubKey) => '${_profileReadyCardDismissedKey}_$pubKey';
43-47: Consider handling the boolean result of setBool and log failures.setBool returns a bool that indicates persistence success. You currently set state to false regardless (which is reasonable UX), but logging when persistence fails will aid diagnostics.
Example (no diff applied):
final bool persisted = await _sharedPreferences.setBool(_prefsKeyFor(_currentPubKey!), true); if (!persisted) { // Consider debugPrint or a logger; keep UI state as false. // debugPrint('Failed to persist dismissal for $_currentPubKey'); } state = const AsyncValue.data(false);Note: per guidelines, avoid blank lines within functions; the empty line at Line 42 can be removed.
57-61: Similarly, consider handling the boolean result of remove and log failures.remove returns a bool indicating whether a value was removed. Logging failures will help troubleshoot storage issues while keeping UI responsive.
Example (no diff applied):
final bool removed = await _sharedPreferences.remove(_prefsKeyFor(_currentPubKey!)); if (!removed) { // debugPrint('Failed to reset visibility for $_currentPubKey'); } state = const AsyncValue.data(true);Also, per guidelines, avoid blank lines within functions; the empty line at Line 56 can be removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
lib/config/providers/profile_ready_card_visibility_provider.dart(4 hunks)test/config/providers/profile_ready_card_visibility_provider_test.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/config/providers/profile_ready_card_visibility_provider_test.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)
**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...
Files:
lib/config/providers/profile_ready_card_visibility_provider.dart
josefinalliende
force-pushed
the
test/coverage-check
branch
from
120af22 to
856cb8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
147-147: Grammar and clarity: tweak sentence about lcovSuggest clarifying and fixing the article.
-You need to install lcov to generate report +You need to install lcov to generate the coverage report and enable diff coverage checks.
149-157: Polish OS-specific install instructions (macOS naming, Linux sudo, Windows admin note)Minor but helpful nits to reduce setup friction across OSes.
-# Mac OS +# macOS brew install lcov -# Linux -apt-get install lcov +# Linux (Debian/Ubuntu) +sudo apt-get update && sudo apt-get install -y lcov + +# Windows +# Run PowerShell as Administrator choco install lcov
160-168: Make the coverage workflow more actionable (CI note + OS-specific open commands)
- Add a CI note about fetch-depth: 0 so diff coverage works (matches PR context).
- Provide explicit open commands for the HTML report per OS.
# Run tests with coverage and check diff coverage for changed files just check-flutter-coverage +# Note: In CI, ensure your checkout step uses `fetch-depth: 0` so the diff coverage script +# can compare your changes against the base branch. # Or run tests with coverage output manually flutter test --coverage # Generate coverage html report genhtml coverage/lcov.info -o coverage/html -# Open coverage/html/index.html in your browser +# Open the HTML report +# macOS +open coverage/html/index.html +# Linux +xdg-open coverage/html/index.html +# Windows (PowerShell) +start coverage/html/index.html
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.md
📄 CodeRabbit Inference Engine (.cursor/rules/whitenoise.mdc)
**/*.md: NIPs (Nostr Implementation Possibilities) are numbered likeNIP-XXwhereXXare two capitalized hexadecimal digits, e.g.,NIP-01andNIP-C7.
To read a specific NIP, construct the NIP URL following this template:https://raw.githubusercontent.com/nostr-protocol/nips/refs/heads/master/{nip}.md(replace{nip}in the URL template with the relevant NIP name, e.g.,07for NIP-07, orC7for NIP-C7).
To read the definition of a specific kind, construct a URL following this template:https://nostrbook.dev/kinds/{kind}.md(replace{kind}in the template with the kind number, e.g.,https://nostrbook.dev/kinds/0.mdfor kind 0).
Files:
README.md
🪛 LanguageTool
README.md
[grammar] ~147-~147: There might be a mistake here.
Context: ...# Coverage You need to install lcov to generate report ```bash # Mac OS brew install lc...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Flutter CI
Description
🎯 We’re adding provider tests, and I want to add a coverage check (to start just for providers files) to make sure we’re not uploading new providers without tests—otherwise, I’ll never finish testing providers 😅.
✅ This PR improves some providers coverage and adds a bash script to check that changed provider files are tested. It adds a CI step to check that new providers have min coverage.
🔍 Commit by commit
📸 Providers coverage
TLDR: now we have 100% in 3 providers 😅
❤️ Reviewer tips
just check-flutter-coveragegenhtml coverage/lcov.info -o coverage/htmlType of Change
Checklist
just precommitto ensure that formatting and linting are correctjust check-flutter-coverageto ensure that flutter coverage rules are passingCHANGELOG.mdfile with your changesSummary by CodeRabbit
New Features
Documentation
Tests
Refactor
Chores / CI