fix(taskserver): resolve bugs and improve CCSync v3 integration#618
Closed
inderjeet20 wants to merge 1 commit intoCCExtractor:mainfrom
Closed
fix(taskserver): resolve bugs and improve CCSync v3 integration#618inderjeet20 wants to merge 1 commit intoCCExtractor:mainfrom
inderjeet20 wants to merge 1 commit intoCCExtractor:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PR addresses five key issues in TaskServer 3 integration: replaces a hardcoded origin URL with dynamic retrieval from credentials storage, removes unsafe ScaffoldMessenger calls from network error handling, reduces HTTP timeout from 10000 to 10 seconds, adds annotation parsing to task deserialization, displays profile mode labels in the UI, and registers GTK and AppLinks plugins on multiple platforms. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/app/modules/home/views/show_tasks.dart (1)
246-251:⚠️ Potential issue | 🟡 MinorInconsistent error handling:
deleteTasklacks await and try-catch.Similar to
_markTaskAsCompleted, the_markTaskAsDeletedmethod should awaitdeleteTaskand handle potential errors to provide consistent user feedback and prevent silent failures.🛡️ Proposed fix for consistency
void _markTaskAsDeleted(String uuid) async { TaskDatabase taskDatabase = TaskDatabase(); await taskDatabase.open(); taskDatabase.markTaskAsDeleted(uuid); - deleteTask('email', uuid); + try { + await deleteTask('email', uuid); + } catch (e) { + debugPrint('Error deleting task on server: $e'); + Get.snackbar( + 'Sync Error', + 'Failed to delete task on server', + snackPosition: SnackPosition.BOTTOM, + duration: const Duration(seconds: 3), + ); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/modules/home/views/show_tasks.dart` around lines 246 - 251, The _markTaskAsDeleted method currently calls deleteTask without awaiting it and lacks error handling; update _markTaskAsDeleted to await deleteTask('email', uuid) and wrap the async work in a try/catch (similar to _markTaskAsCompleted) so you open TaskDatabase, await taskDatabase.markTaskAsDeleted(uuid) if it returns a Future, then await deleteTask(...), and on error catch and surface user feedback/logging via the same mechanism used elsewhere (e.g., show a Snackbar or log the error) to avoid silent failures.
🧹 Nitpick comments (4)
lib/app/v3/net/fetch.dart (1)
26-30: Consider propagating errors instead of silently returning an empty list.The current error handling returns an empty list on failure, which makes it difficult for callers to distinguish between "no tasks exist" and "server error occurred". This could lead to data loss scenarios where users think they have no tasks when the server is actually unreachable.
Consider throwing the exception and letting callers handle it, similar to how
completeTaskwas refactored in this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/v3/net/fetch.dart` around lines 26 - 30, The current catch block in fetchTasks (the one logging "Error fetching tasks: $e") swallows errors and returns an empty list; change it to rethrow the caught exception (or throw a wrapped exception) instead of returning [], so callers can distinguish server errors from legitimately empty results—follow the same pattern used by completeTask by logging the error then rethrowing the exception (preserve stack trace using the caught stack variable).lib/app/v3/models/task.dart (1)
47-47: Consider removing debug print in production code.This debug statement logs on every task deserialization. If many tasks are fetched, this could create log noise. Consider removing it or using a more selective logging approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/app/v3/models/task.dart` at line 47, Remove the unconditional debug print in the Task.fromJson deserialization (the debugPrint("Annotation fromJson: ...") call) to avoid noisy logs in production; either delete the line or guard it behind a debug-only check (e.g., use Flutter's kDebugMode) or switch to a configurable logger so only debug builds emit this message while production builds remain silent.test/api_service_test.dart (2)
299-299: VariablemockClientshadows outer scope declaration.Line 299 declares a new
mockClientthat shadows theMockClient mockClientat line 27. While this works due to Dart's scoping rules, it can cause confusion. Consider using a distinct name likelocalMockClientor renaming the outer variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/api_service_test.dart` at line 299, The local declaration "final mockClient = MockClient();" shadows the outer "MockClient mockClient" variable; rename the inner variable (for example to "localMockClient" or "testMockClient") or rename the outer variable so they are distinct across scope, and update all uses in the test block to reference the new name (search for "mockClient" inside the test around line 299 and replace with the chosen new identifier).
313-319: Timeout regression test doesn't verify actual code behavior.This test only verifies that
Duration(seconds: 10)equals 10 seconds, which is always true. It doesn't actually verify that the timeout value inmanage_task_champion_creds_controller.dartis configured correctly.Consider testing the actual behavior by mocking a slow server response, or at minimum, importing and referencing the actual timeout constant from the controller if one exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/api_service_test.dart` around lines 313 - 319, The test currently asserts Duration(seconds: 10) instead of the app constant—update the test in test/api_service_test.dart to reference the real timeout constant or behavior from manage_task_champion_creds_controller.dart: import the exported timeout constant (e.g., CREDENTIAL_CHECK_TIMEOUT or the named constant used in that controller) and assert its value, or better mock a slow server (using your HTTP client test utility or a fake server) and assert that the credential-check call times out as expected (throws a timeout or returns an error) when the controller's timeout is applied; locate references to the controller's credential-check function or HTTP call (e.g., the method that performs the credential check) and write the test against that symbol rather than a locally created Duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/app/v3/db/update.dart`:
- Around line 85-87: The call to deleteTask when localTask.status == 'deleted'
is not awaited or error-handled like completeTask, so wrap the
deleteTask('email', localTask.uuid!) call in an await and surround it with
try-catch (similar to the completeTask handling) to catch and log errors;
reference the deleteTask function call and localTask.status check and ensure you
use await deleteTask(...) and a try { await deleteTask(...) } catch (err) { /*
log error via existing logger/processLogger */ } pattern to keep behavior
consistent.
In `@lib/app/v3/net/complete.dart`:
- Around line 6-8: The function completeTask creates an http.Client when no
client is injected but never closes it, leaking sockets; change completeTask to
track whether the client was created (e.g., createdClient = client == null) and
ensure httpClient.close() is called in a finally block (or use a try/finally) so
only internally created clients are closed while injected clients are left open;
reference the httpClient local and completeTask function to locate where to add
the finally/close logic.
In `@lib/app/v3/net/origin.dart`:
- Around line 1-3: Remove the dead helper getOrigin() and its containing file:
delete the getOrigin function and the file defining it (the function name
getOrigin and its single use of CredentialsStorage.getApiUrl() are unused), and
ensure no other code imports or references getOrigin; replace any remaining
calls (if any) with direct calls to CredentialsStorage.getApiUrl() or remove the
import entirely so the file can be safely deleted.
---
Outside diff comments:
In `@lib/app/modules/home/views/show_tasks.dart`:
- Around line 246-251: The _markTaskAsDeleted method currently calls deleteTask
without awaiting it and lacks error handling; update _markTaskAsDeleted to await
deleteTask('email', uuid) and wrap the async work in a try/catch (similar to
_markTaskAsCompleted) so you open TaskDatabase, await
taskDatabase.markTaskAsDeleted(uuid) if it returns a Future, then await
deleteTask(...), and on error catch and surface user feedback/logging via the
same mechanism used elsewhere (e.g., show a Snackbar or log the error) to avoid
silent failures.
---
Nitpick comments:
In `@lib/app/v3/models/task.dart`:
- Line 47: Remove the unconditional debug print in the Task.fromJson
deserialization (the debugPrint("Annotation fromJson: ...") call) to avoid noisy
logs in production; either delete the line or guard it behind a debug-only check
(e.g., use Flutter's kDebugMode) or switch to a configurable logger so only
debug builds emit this message while production builds remain silent.
In `@lib/app/v3/net/fetch.dart`:
- Around line 26-30: The current catch block in fetchTasks (the one logging
"Error fetching tasks: $e") swallows errors and returns an empty list; change it
to rethrow the caught exception (or throw a wrapped exception) instead of
returning [], so callers can distinguish server errors from legitimately empty
results—follow the same pattern used by completeTask by logging the error then
rethrowing the exception (preserve stack trace using the caught stack variable).
In `@test/api_service_test.dart`:
- Line 299: The local declaration "final mockClient = MockClient();" shadows the
outer "MockClient mockClient" variable; rename the inner variable (for example
to "localMockClient" or "testMockClient") or rename the outer variable so they
are distinct across scope, and update all uses in the test block to reference
the new name (search for "mockClient" inside the test around line 299 and
replace with the chosen new identifier).
- Around line 313-319: The test currently asserts Duration(seconds: 10) instead
of the app constant—update the test in test/api_service_test.dart to reference
the real timeout constant or behavior from
manage_task_champion_creds_controller.dart: import the exported timeout constant
(e.g., CREDENTIAL_CHECK_TIMEOUT or the named constant used in that controller)
and assert its value, or better mock a slow server (using your HTTP client test
utility or a fake server) and assert that the credential-check call times out as
expected (throws a timeout or returns an error) when the controller's timeout is
applied; locate references to the controller's credential-check function or HTTP
call (e.g., the method that performs the credential check) and write the test
against that symbol rather than a locally created Duration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 814a2e35-5889-45ea-80c3-088af564807f
📒 Files selected for processing (15)
lib/app/modules/home/views/show_tasks.dartlib/app/modules/manage_task_champion_creds/controllers/manage_task_champion_creds_controller.dartlib/app/modules/profile/views/profile_view.dartlib/app/modules/profile/views/profiles_list.dartlib/app/v3/db/update.dartlib/app/v3/models/task.dartlib/app/v3/net/complete.dartlib/app/v3/net/fetch.dartlib/app/v3/net/origin.dartlinux/flutter/generated_plugin_registrant.cclinux/flutter/generated_plugins.cmakemacos/Flutter/GeneratedPluginRegistrant.swifttest/api_service_test.dartwindows/flutter/generated_plugin_registrant.ccwindows/flutter/generated_plugins.cmake
Comment on lines
85
to
87
| } else if (localTask.status == 'deleted') { | ||
| deleteTask('email', localTask.uuid!); | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling: deleteTask is not awaited or wrapped in try-catch.
completeTask is now properly awaited with error handling, but deleteTask on line 86 is called without await and has no try-catch. This inconsistency could lead to unhandled errors if deleteTask throws, potentially causing silent failures or unhandled promise rejections.
🛡️ Proposed fix for consistency
if (localTask.status == 'completed') {
try {
await completeTask('email', localTask.uuid!);
} catch (e) {
debugPrint('Failed to complete task on server: $e');
}
} else if (localTask.status == 'deleted') {
- deleteTask('email', localTask.uuid!);
+ try {
+ await deleteTask('email', localTask.uuid!);
+ } catch (e) {
+ debugPrint('Failed to delete task on server: $e');
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if (localTask.status == 'deleted') { | |
| deleteTask('email', localTask.uuid!); | |
| } | |
| } else if (localTask.status == 'deleted') { | |
| try { | |
| await deleteTask('email', localTask.uuid!); | |
| } catch (e) { | |
| debugPrint('Failed to delete task on server: $e'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/v3/db/update.dart` around lines 85 - 87, The call to deleteTask when
localTask.status == 'deleted' is not awaited or error-handled like completeTask,
so wrap the deleteTask('email', localTask.uuid!) call in an await and surround
it with try-catch (similar to the completeTask handling) to catch and log
errors; reference the deleteTask function call and localTask.status check and
ensure you use await deleteTask(...) and a try { await deleteTask(...) } catch
(err) { /* log error via existing logger/processLogger */ } pattern to keep
behavior consistent.
Comment on lines
+6
to
+8
| Future<void> completeTask(String email, String taskUuid, | ||
| {http.Client? client}) async { | ||
| final httpClient = client ?? http.Client(); |
There was a problem hiding this comment.
Resource leak: internally created http.Client is never closed.
When no client is injected, a new http.Client() is created but never closed. The http.Client should be closed after use to release underlying resources (socket connections).
🛡️ Proposed fix
Future<void> completeTask(String email, String taskUuid,
{http.Client? client}) async {
- final httpClient = client ?? http.Client();
+ final httpClient = client;
+ final shouldCloseClient = httpClient == null;
+ final effectiveClient = httpClient ?? http.Client();
var c = await CredentialsStorage.getClientId();
var e = await CredentialsStorage.getEncryptionSecret();
var baseUrl = await CredentialsStorage.getApiUrl();
// ... rest of the code ...
try {
- final response = await httpClient.post(
+ final response = await effectiveClient.post(
// ...
);
// ...
} catch (e, s) {
debugPrint('Error completing task: $e\n$s');
rethrow;
+ } finally {
+ if (shouldCloseClient) {
+ effectiveClient.close();
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Future<void> completeTask(String email, String taskUuid, | |
| {http.Client? client}) async { | |
| final httpClient = client ?? http.Client(); | |
| Future<void> completeTask(String email, String taskUuid, | |
| {http.Client? client}) async { | |
| final httpClient = client; | |
| final shouldCloseClient = httpClient == null; | |
| final effectiveClient = httpClient ?? http.Client(); | |
| var c = await CredentialsStorage.getClientId(); | |
| var e = await CredentialsStorage.getEncryptionSecret(); | |
| var baseUrl = await CredentialsStorage.getApiUrl(); | |
| // ... rest of the code ... | |
| try { | |
| final response = await effectiveClient.post( | |
| // ... | |
| ); | |
| // ... | |
| } catch (e, s) { | |
| debugPrint('Error completing task: $e\n$s'); | |
| rethrow; | |
| } finally { | |
| if (shouldCloseClient) { | |
| effectiveClient.close(); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/v3/net/complete.dart` around lines 6 - 8, The function completeTask
creates an http.Client when no client is injected but never closes it, leaking
sockets; change completeTask to track whether the client was created (e.g.,
createdClient = client == null) and ensure httpClient.close() is called in a
finally block (or use a try/finally) so only internally created clients are
closed while injected clients are left open; reference the httpClient local and
completeTask function to locate where to add the finally/close logic.
Comment on lines
+1
to
+3
| import 'package:taskwarrior/app/utils/taskchampion/credentials_storage.dart'; | ||
|
|
||
| Future<String> getOrigin() async => await CredentialsStorage.getApiUrl() ?? ''; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of getOrigin() across the codebase
# Search for imports of origin.dart
echo "=== Imports of origin.dart ==="
rg -l "import.*origin\.dart" --type dart
# Search for getOrigin function calls
echo -e "\n=== Usages of getOrigin() ==="
rg -n "getOrigin\(" --type dartRepository: CCExtractor/taskwarrior-flutter
Length of output: 245
Remove unused getOrigin() function and file.
The getOrigin() function is not imported or called anywhere in the codebase. It is dead code left over from refactoring where direct calls to CredentialsStorage.getApiUrl() replaced its usage. Delete lib/app/v3/net/origin.dart entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/app/v3/net/origin.dart` around lines 1 - 3, Remove the dead helper
getOrigin() and its containing file: delete the getOrigin function and the file
defining it (the function name getOrigin and its single use of
CredentialsStorage.getApiUrl() are unused), and ensure no other code imports or
references getOrigin; replace any remaining calls (if any) with direct calls to
CredentialsStorage.getApiUrl() or remove the import entirely so the file can be
safely deleted.
Collaborator
|
@inderjeet20 , We're no longer using CCSync APIs to sync. Closing this |
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
2 participants
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
This PR resolves critical bugs, adds missing features, and improves the user experience for TaskServer 3 (CCSync v3) in the Flutter app.
fixes #617
Changes
🐛 Bug Fixes
ScaffoldMessengercrash incomplete.dart.manage_task_champion_creds_controller.dart.✨ Feature Enhancements
TaskForC.fromJson.🧪 Tests
completeTaskexception handlingFiles Changed
🐛 Bug Fixes
origin.dartfetch.dartcomplete.dartmanage_task_champion_creds_controller.dart✨ Feature Enhancements
task.dartprofiles_list.dartprofile_view.dart🧪 Tests
api_service_test.dartVerification
How to Test
1️⃣ Mode Selector
2️⃣ Annotations Parsing
3️⃣ Error Handling
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Chores