fix: search panel issues #8093
Open
fix: search panel issues #8093
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors the view‐fetching logic in CommandPaletteBloc to robustly handle failures by using a fold-based result, logging errors, and retrying the fetch until it succeeds, and ensures this behavior is enabled on initialization. Sequence diagram for robust view fetching with retry in CommandPaletteBlocsequenceDiagram
participant CommandPaletteBloc
participant ViewBackendService
participant Log
actor Timer as Retry Timer
CommandPaletteBloc->>ViewBackendService: getAllViews()
alt Success
ViewBackendService-->>CommandPaletteBloc: Result (views)
CommandPaletteBloc->>CommandPaletteBloc: add(updateCachedViews)
else Failure
ViewBackendService-->>CommandPaletteBloc: Error
CommandPaletteBloc->>Log: error(message)
alt refreshUntilSuccess is true
CommandPaletteBloc->>Timer: Future.delayed(3s)
Timer-->>CommandPaletteBloc: (after 3s)
CommandPaletteBloc->>ViewBackendService: getAllViews() (retry)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @asjqkkkk - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `frontend/appflowy_flutter/lib/workspace/application/command_palette/command_palette_bloc.dart:100` </location>
<code_context>
+ 'command palette bloc gets all views failed: $e${refreshUntilSuccess ? ', retrying...' : ''}',
+ );
+ if (refreshUntilSuccess) {
+ unawaited(
+ Future.delayed(
+ const Duration(seconds: 3),
+ () => _refreshCachedViews(refreshUntilSuccess: true),
+ ),
+ );
</code_context>
<issue_to_address>
Potential for unbounded retries if getAllViews keeps failing.
Add a maximum retry limit or implement exponential backoff to prevent resource exhaustion and excessive logging.
Suggested implementation:
```
if (isClosed) return;
final result = await ViewBackendService.getAllViews();
result.fold((v) {
if (isClosed) return;
add(CommandPaletteEvent.updateCachedViews(views: v.items));
}, (e) {
Log.error(
'command palette bloc gets all views failed: $e${refreshUntilSuccess ? ', retrying...' : ''}',
);
if (refreshUntilSuccess) {
// Retry logic with max retries and exponential backoff
const int maxRetries = 5;
final int currentRetry = retryCount ?? 0;
if (currentRetry < maxRetries) {
final int delaySeconds = 3 * (1 << currentRetry); // Exponential backoff: 3, 6, 12, 24, 48
unawaited(
Future.delayed(
Duration(seconds: delaySeconds),
() => _refreshCachedViews(
refreshUntilSuccess: true,
retryCount: currentRetry + 1,
),
),
);
} else {
Log.error(
'command palette bloc gets all views failed after $maxRetries retries. Giving up.',
);
}
}
}
}
Future<void> _refreshCachedViews({bool refreshUntilSuccess = false, int? retryCount}) async {
/// Sometimes non-existent views appear in the search results
/// and the icon data for the search results is empty
/// Fetching all views can temporarily resolve these issues
```
- You must update all calls to `_refreshCachedViews` to include the new `retryCount` parameter where appropriate, or ensure it defaults to `null`/`0` for initial calls.
- If you want to make the retry/backoff logic configurable, consider moving `maxRetries` and the base delay to class-level constants.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+100
to
+103
| unawaited( | ||
| Future.delayed( | ||
| const Duration(seconds: 3), | ||
| () => _refreshCachedViews(refreshUntilSuccess: true), |
There was a problem hiding this comment.
suggestion (bug_risk): Potential for unbounded retries if getAllViews keeps failing.
Add a maximum retry limit or implement exponential backoff to prevent resource exhaustion and excessive logging.
Suggested implementation:
if (isClosed) return;
final result = await ViewBackendService.getAllViews();
result.fold((v) {
if (isClosed) return;
add(CommandPaletteEvent.updateCachedViews(views: v.items));
}, (e) {
Log.error(
'command palette bloc gets all views failed: $e${refreshUntilSuccess ? ', retrying...' : ''}',
);
if (refreshUntilSuccess) {
// Retry logic with max retries and exponential backoff
const int maxRetries = 5;
final int currentRetry = retryCount ?? 0;
if (currentRetry < maxRetries) {
final int delaySeconds = 3 * (1 << currentRetry); // Exponential backoff: 3, 6, 12, 24, 48
unawaited(
Future.delayed(
Duration(seconds: delaySeconds),
() => _refreshCachedViews(
refreshUntilSuccess: true,
retryCount: currentRetry + 1,
),
),
);
} else {
Log.error(
'command palette bloc gets all views failed after $maxRetries retries. Giving up.',
);
}
}
}
}
Future<void> _refreshCachedViews({bool refreshUntilSuccess = false, int? retryCount}) async {
/// Sometimes non-existent views appear in the search results
/// and the icon data for the search results is empty
/// Fetching all views can temporarily resolve these issues
- You must update all calls to
_refreshCachedViewsto include the newretryCountparameter where appropriate, or ensure it defaults tonull/0for initial calls. - If you want to make the retry/backoff logic configurable, consider moving
maxRetriesand the base delay to class-level constants.
asjqkkkk
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Feature Preview
PR Checklist