Code improvments #547
Code improvments #547
Conversation
WalkthroughWidened env-related ignores, added a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant EPSN as EditProfileScreenNotifier
participant API as Profile API
U->>EPSN: updateProfileData(newData)
EPSN->>EPSN: prev = state.asData?.value ?? default
EPSN->>EPSN: state = Data(prev with isSaving=true, clear error/stack)
EPSN->>API: saveProfile(newData)
alt success
API-->>EPSN: saved profile
EPSN->>EPSN: state = Data(updated, isSaving=false)
else error
API-->>EPSN: throw e, st
EPSN->>EPSN: message = e is ApiError ? await e.messageText() : e.toString()
EPSN->>EPSN: state = Data(prev with isSaving=false, error=message, stack=st)
end
sequenceDiagram
autonumber
participant A as ActiveAccountWatcher
participant FP as FollowsProvider
A-->>FP: activePubkey changed
note right of FP #eef2ff: Schedule with microtask (was post-frame)
FP->>FP: clearFollows()
FP->>FP: loadFollows()
alt state.isLoading == true
FP-->>FP: return early
else
FP->>FP: state.isLoading = true
FP->>API: fetch follows
API-->>FP: result / error
FP->>FP: update state, isLoading=false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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)
✨ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/config/providers/create_profile_screen_provider.dart (1)
41-43: Localize user-facing strings.Replace hardcoded toasts with AppLocalizations-backed messages (or inject a translation service via getIt), per project guidelines.
I can wire a small I18n helper for providers that lack BuildContext.
Also applies to: 55-56, 95-96, 109-110
lib/config/providers/metadata_cache_provider.dart (2)
288-292: Make isContactCached async and update all call sites
- Apply this diff in lib/config/providers/metadata_cache_provider.dart to use _getStandardizedNpub and return Future:
- bool isContactCached(String publicKey) { - final normalizedKey = _normalizePublicKey(publicKey); - final cached = state.cache[normalizedKey]; - return cached != null && !cached.isExpired; - } + Future<bool> isContactCached(String publicKey) async { + final standardNpub = await _getStandardizedNpub(publicKey); + final cached = state.cache[standardNpub]; + return cached != null && !cached.isExpired; + }- In lib/examples/metadata_cache_examples.dart:
• Change the example wrapper toFuture<bool> isContactCached(...) async { … }
• Addawaitwhen callingcache.isContactCached(publicKey)(e.g. beforeawait expensiveNetworkCall())- In docs/METADATA_CACHING_SOLUTION.md: update all snippets to use
await metadataCache.isContactCached(publicKey)within async functions
317-328: Use standardized npub in updateCachedMetadata
ChangeupdateCachedMetadatatoFuture<void>, callawait _getStandardizedNpub(publicKey)instead of_normalizePublicKey, and update all call sites inlib/utils/metadata_cache_utils.dartandlib/examples/metadata_cache_examples.darttoawait cache.updateCachedMetadata(...).
🧹 Nitpick comments (8)
lib/config/providers/create_profile_screen_provider.dart (2)
60-93: Handle first-time profile creation (initialMetadata == null).Current flow silently does nothing if there's no existing metadata. Either construct new FlutterMetadata from inputs or route to a dedicated “create” path.
Would you like me to draft a minimal create-path that sets displayName/about/picture when initialMetadata is null?
46-53: Declare explicit local variable types.Guidelines require explicit types for locals (e.g., FlutterMetadata?, ActiveAccountState, String?). Replace inferred types in these spots.
Also applies to: 103-107
.gitignore (1)
15-19: Broader env ignore rules look good; add direnv cache too.Include .direnv/ so developer-local env caches aren’t committed.
Apply this diff:
.env .env.* .envrc +/.direnv/ *.env !.env.example !.env.*.examplelib/config/providers/metadata_cache_provider.dart (1)
103-150: Consider surfacing “no active account” distinctly.The throw is caught and converted to an “Unknown User” fallback, masking auth state issues. Either (a) check active account before try/catch and return a typed failure to caller, or (b) attach a sentinel to the fallback (e.g., error field in state) so callers can react.
justfile (1)
217-238: Release preflight checks are a good safety net.Clean working tree and build script checks reduce failed releases. Optionally call just doctor upfront to fail fast on missing tools.
Apply this small addition:
release: @echo "🔨 Building versioned release..." + @echo "🏥 Running environment doctor..." + just doctor @echo "🔍 Verifying working tree is clean..."lib/config/providers/edit_profile_screen_provider.dart (3)
56-61: PreferasDataand explicit typing; avoid mutating non-data states infinally.Use
state.asData?.value(notstate.value) and add an explicit type to satisfy the code style and make intent clear.- final stateValue = state.value; - if (stateValue != null) { - state = AsyncValue.data( - stateValue.copyWith(isSaving: false, stackTrace: null, error: null), - ); - } + final ProfileState? current = state.asData?.value; + if (current != null) { + state = AsyncValue.data( + current.copyWith(isSaving: false, stackTrace: null, error: null), + ); + }
117-119: Good state preservation before save; add explicit type per guidelines.Declare the variable type explicitly.
- final previousState = state.asData?.value ?? const ProfileState(); + final ProfileState previousState = state.asData?.value ?? const ProfileState();
53-53: Fix log context label.Method name is
fetchProfileData, notloadProfileData.- _logger.severe('loadProfileData', e, st); + _logger.severe('fetchProfileData', e, st);
📜 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 ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.gitignore(1 hunks)justfile(2 hunks)lib/config/providers/create_profile_screen_provider.dart(1 hunks)lib/config/providers/edit_profile_screen_provider.dart(3 hunks)lib/config/providers/follows_provider.dart(2 hunks)lib/config/providers/metadata_cache_provider.dart(1 hunks)
🧰 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/follows_provider.dartlib/config/providers/metadata_cache_provider.dartlib/config/providers/create_profile_screen_provider.dartlib/config/providers/edit_profile_screen_provider.dart
🧠 Learnings (1)
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use Riverpod to manage state. Use StreamProviders to watch for state changes that come from the rust api. See keepAlive if you need to keep the state alive.
Applied to files:
lib/config/providers/follows_provider.dart
🪛 GitHub Actions: CI
lib/config/providers/follows_provider.dart
[error] 1-1: Dart formatter updated this file. Run 'dart format -w' to apply formatting. Command: 'dart format --set-exit-if-changed lib/ integration_test/'.
lib/config/providers/create_profile_screen_provider.dart
[error] 1-1: Dart formatter updated this file. Run 'dart format -w' to apply formatting. Command: 'dart format --set-exit-if-changed lib/ integration_test/'.
🔇 Additional comments (6)
lib/config/providers/create_profile_screen_provider.dart (2)
56-57: Good catch: clear loading state on early return.Prevents the spinner from sticking when no active account exists.
1-1: Format lib/config/providers/create_profile_screen_provider.dart
Rundart format -w lib/config/providers/create_profile_screen_provider.dartlocally to satisfy CI formatting checks.lib/config/providers/metadata_cache_provider.dart (1)
116-117: Prefer typed exceptions—nice improvement.Switching to StateError improves signal in logs/handlers.
lib/config/providers/follows_provider.dart (2)
44-56: Switch to microtask: verify no “set state during build” hazards.Microtasks can run before the next frame; ensure no rebuild exceptions on listener setup. If issues arise, use Future(() { ... }) instead to defer to next event loop.
I can provide a quick test harness to simulate pubkey flips and assert no exceptions.
1-1: Run Dart formatter on follows_provider.dart
Please rundart format -w lib/config/providers/follows_provider.dartlocally to apply the required formatting for CI.justfile (1)
18-28: precommit-check: solid CI-parity set.Nice separation from auto-fixing precommit.
| final prev = state.asData?.value ?? const ProfileState(); | ||
| final message = e is ApiError ? (await e.messageText()) : e.toString(); | ||
| state = AsyncValue.data(prev.copyWith(isSaving: false, error: message, stackTrace: st)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error handling: keep errors inside ProfileState everywhere and add explicit types.
This block correctly retains form data by storing the error in ProfileState instead of flipping to AsyncValue.error. Apply this pattern consistently (e.g., authentication/active-account failures and image-pick failure currently use AsyncValue.error) to avoid UI state loss and inconsistency. Also add explicit types.
- final prev = state.asData?.value ?? const ProfileState();
- final message = e is ApiError ? (await e.messageText()) : e.toString();
+ final ProfileState prev = state.asData?.value ?? const ProfileState();
+ final String message = e is ApiError ? (await e.messageText()) : e.toString();
state = AsyncValue.data(prev.copyWith(isSaving: false, error: message, stackTrace: st));Example helper you can add to this notifier to replace all current AsyncValue.error(...) sites:
void _setLocalError(String message, StackTrace st) {
final ProfileState prev = state.asData?.value ?? const ProfileState();
state = AsyncValue.data(prev.copyWith(isSaving: false, error: message, stackTrace: st));
}Then replace, for example:
- Lines 25–29 (“Not authenticated”)
- Lines 37–38 (“No active account found”)
- Line 54 (catch in
fetchProfileData) - Lines 112–113 (image pick failure)
- Lines 124–125, 133–135 (auth/active account in
updateProfileData)
with _setLocalError(...) to keep the UI data intact.
🤖 Prompt for AI Agents
In lib/config/providers/edit_profile_screen_provider.dart around lines 175–178,
error handling flips to AsyncValue.error in other places which loses UI form
data and lacks explicit types; add a typed helper like _setLocalError(String
message, StackTrace st) that reads the previous ProfileState (final ProfileState
prev = state.asData?.value ?? const ProfileState()) and sets state =
AsyncValue.data(prev.copyWith(isSaving: false, error: message, stackTrace: st));
then replace all current AsyncValue.error(...) and untyped error assignments at
the listed locations (lines 25–29 “Not authenticated”, 37–38 “No active account
found”, the catch at line 54 in fetchProfileData, lines 112–113 image pick
failure, and lines 124–125 and 133–135 in updateProfileData) with calls to
_setLocalError(message, st) using explicit String and StackTrace types so errors
remain inside ProfileState and form data is preserved.
| if (state.isLoading) return; | ||
|
|
||
| state = state.copyWith(isLoading: true, error: null); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Re-entrancy guard can drop needed refreshes.
If loadFollows is in-flight, subsequent calls (e.g., after add/remove) return early, leaving stale state. Track and share a single in-flight Future instead of a boolean guard.
Apply this diff within loadFollows:
- Future<void> loadFollows() async {
- if (state.isLoading) return;
-
- state = state.copyWith(isLoading: true, error: null);
+ Future<void> loadFollows() async {
+ // De-duplicate concurrent calls by sharing a single in-flight Future.
+ if (_inflightLoad != null) return _inflightLoad!;
+ state = state.copyWith(isLoading: true, error: null);And add the following to the class:
// At class level:
Future<void>? _inflightLoad;Refactor the body to set/clear the shared future:
_inflightLoad = _doLoadFollows();
return _inflightLoad!;Move existing logic into _doLoadFollows() and in finally set:
finally {
state = state.copyWith(isLoading: false);
_inflightLoad = null;
}🤖 Prompt for AI Agents
In lib/config/providers/follows_provider.dart around lines 72 to 75, the current
boolean re-entrancy guard (isLoading) causes subsequent callers to return early
and miss needed refreshes; instead add a class-level Future<void>?
_inflightLoad; move the existing load logic into a new private method
_doLoadFollows(), refactor loadFollows to check for and return the shared
_inflightLoad if present, otherwise set _inflightLoad = _doLoadFollows() and
return it; ensure _doLoadFollows() sets state = state.copyWith(isLoading: true,
error: null) at start and in a finally block clears the loading flag and sets
_inflightLoad = null.
There was a problem hiding this comment.
I prefer the guard you added, this coderabbit suggestion seems less readable to me ..
There was a problem hiding this comment.
@josefinalliende Thank you for the feedback! You're absolutely right that the simple boolean guard is much more readable and maintainable. The straightforward if (state.isLoading) return; approach is clear, easy to understand, and effectively prevents concurrent calls without adding unnecessary complexity.
Your preference for readability over the more complex Future-based approach is a solid engineering decision. The simple guard accomplishes the primary goal of preventing re-entrant calls while keeping the code clean and maintainable.
✏️ Learnings added
Learnt from: josefinalliende
PR: parres-hq/whitenoise_flutter#547
File: lib/config/providers/follows_provider.dart:0-0
Timestamp: 2025-08-29T19:58:49.679Z
Learning: In the whitenoise_flutter project, the user josefinalliende prefers simple boolean guards over complex Future-based re-entrancy protection for readability and maintainability reasons.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
josefinalliende
left a comment
josefinalliende
left a comment
There was a problem hiding this comment.
✅ LGTM. Tried it locally and it seems to me that master and this branch have the same behaviour
| if (state.isLoading) return; | ||
|
|
||
| state = state.copyWith(isLoading: true, error: null); | ||
|
|
There was a problem hiding this comment.
I prefer the guard you added, this coderabbit suggestion seems less readable to me ..
Description
Improved precommit checks for releases
Added all types of envs to gitignore
Improved logic in some providers.
Type 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 changes (if they affect the user experience)Summary by CodeRabbit
Bug Fixes
Refactor
Chores