feat: add manifest refresh with 2h TTL caching#4476
Open
Conversation
- Add 2-hour TTL caching for chat tools manifest fetching
- Add manual refresh button in manage app screen
- Add /v1/apps/{app_id}/refresh-manifest API endpoint
- Manually refreshed GitHub app manifest (7 tools)
Backend changes:
- Modified fetch_app_chat_tools_from_manifest() to use Redis cache with 2h TTL
- Added force_refresh parameter to bypass cache
- Created new POST endpoint for manual manifest refresh
Frontend changes:
- Added refresh icon button in UpdateAppPage AppBar
- Added refreshManifest() method to AddAppProvider
- Added isRefreshingManifest state with loading indicator
- Button only shows when app has external integration with manifest URL
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces manifest refresh with caching, which is a great improvement for efficiency. However, it also includes significant, unrelated changes to the Action Items page for a new "Goals" feature and refactors the bottom navigation bar. Bundling multiple features into a single pull request makes it harder to review and increases the risk of introducing bugs. In the future, please try to keep pull requests focused on a single, cohesive change. This aligns with the repository's general rule about separating large-scale changes into dedicated pull requests.
Comment on lines
+106
to
+142
| Future<void> _loadGoals() async { | ||
| try { | ||
| final goals = await getAllGoals(); | ||
| if (!mounted) return; | ||
| if (goals.isNotEmpty) { | ||
| setState(() { | ||
| _goals = goals; | ||
| _isLoadingGoals = false; | ||
| }); | ||
| _pruneTaskGoalLinks(); | ||
| return; | ||
| } | ||
| } catch (_) {} | ||
|
|
||
| // Fallback to locally cached goals from the goals widget | ||
| try { | ||
| final prefs = await SharedPreferences.getInstance(); | ||
| final goalsJson = prefs.getString(_goalsStorageKey); | ||
| if (goalsJson != null) { | ||
| final List<dynamic> decoded = jsonDecode(goalsJson); | ||
| final localGoals = decoded.map((e) => Goal.fromJson(e)).toList(); | ||
| if (mounted) { | ||
| setState(() { | ||
| _goals = localGoals; | ||
| _isLoadingGoals = false; | ||
| }); | ||
| _pruneTaskGoalLinks(); | ||
| return; | ||
| } | ||
| } | ||
| } catch (_) {} | ||
|
|
||
| if (!mounted) return; | ||
| setState(() { | ||
| _isLoadingGoals = false; | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
The catch blocks in this function are empty, which silently swallows any exceptions that occur when fetching or parsing goals. This can make debugging difficult and lead to unexpected behavior if an operation fails. At a minimum, errors should be logged.
Future<void> _loadGoals() async {
try {
final goals = await getAllGoals();
if (!mounted) return;
if (goals.isNotEmpty) {
setState(() {
_goals = goals;
_isLoadingGoals = false;
});
_pruneTaskGoalLinks();
return;
}
} catch (e, stackTrace) {
Logger.debug('Failed to load goals from API: $e, $stackTrace');
}
// Fallback to locally cached goals from the goals widget
try {
final prefs = await SharedPreferences.getInstance();
final goalsJson = prefs.getString(_goalsStorageKey);
if (goalsJson != null) {
final List<dynamic> decoded = jsonDecode(goalsJson);
final localGoals = decoded.map((e) => Goal.fromJson(e)).toList();
if (mounted) {
setState(() {
_goals = localGoals;
_isLoadingGoals = false;
});
_pruneTaskGoalLinks();
return;
}
}
} catch (e, stackTrace) {
Logger.debug('Failed to load goals from SharedPreferences: $e, $stackTrace');
}
if (!mounted) return;
setState(() {
_isLoadingGoals = false;
});
}
Comment on lines
+585
to
+605
| Future<bool> refreshManifest() async { | ||
| if (updateAppId == null) { | ||
| AppSnackbar.showSnackbarError('App ID not found'); | ||
| return false; | ||
| } | ||
|
|
||
| setIsRefreshingManifest(true); | ||
| var success = await refreshAppManifestServer(updateAppId!); | ||
| if (success) { | ||
| // Reload app details to get updated chat tools | ||
| var app = await getAppDetailsServer(updateAppId!); | ||
| if (app != null) { | ||
| appProvider!.updateLocalApp(App.fromJson(app)); | ||
| AppSnackbar.showSnackbarSuccess('Manifest refreshed successfully'); | ||
| } | ||
| } else { | ||
| AppSnackbar.showSnackbarError('Failed to refresh manifest'); | ||
| } | ||
| setIsRefreshingManifest(false); | ||
| return success; | ||
| } |
Contributor
There was a problem hiding this comment.
The loading state isRefreshingManifest is not guaranteed to be reset to false if an error occurs after it's set to true (e.g., if App.fromJson or appProvider!.updateLocalApp throws an exception). This could leave the UI in a permanent loading state. Using a try...finally block will ensure the loading state is always reset.
Future<bool> refreshManifest() async {
if (updateAppId == null) {
AppSnackbar.showSnackbarError('App ID not found');
return false;
}
setIsRefreshingManifest(true);
var success = false;
try {
success = await refreshAppManifestServer(updateAppId!);
if (success) {
// Reload app details to get updated chat tools
var app = await getAppDetailsServer(updateAppId!);
if (app != null) {
appProvider?.updateLocalApp(App.fromJson(app));
AppSnackbar.showSnackbarSuccess('Manifest refreshed successfully');
}
} else {
AppSnackbar.showSnackbarError('Failed to refresh manifest');
}
} catch (e, stackTrace) {
Logger.debug('Failed to refresh manifest: $e, $stackTrace');
AppSnackbar.showSnackbarError('Failed to refresh manifest');
success = false;
} finally {
setIsRefreshingManifest(false);
}
return success;
}
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Implements chat tools manifest refresh functionality with 2-hour TTL caching and a manual refresh button in the app management screen.
Changes
Backend
fetch_app_chat_tools_from_manifest()to cache manifests in Redis with 2-hour TTLforce_refreshparameter to bypass cache when neededPOST /v1/apps/{app_id}/refresh-manifestfor manual refreshFrontend (Flutter)
UpdateAppPageAppBarrefreshManifest()method toAddAppProviderisRefreshingManifeststate with loading indicatorManual Refresh
Benefits
Test Plan
🤖 Generated with Claude Code