Feat/adds ai orchestration - #57
Conversation
… tokens and initial architecture setup
…ills for routine optimization
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (60)
WalkthroughThis PR adds agent-event streaming, retry handling, and an ChangesAI orchestration refactor
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 38.50% 39.57% +1.07%
==========================================
Files 85 121 +36
Lines 14084 15023 +939
==========================================
+ Hits 5423 5946 +523
- Misses 8661 9077 +416 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workout-logger/lib/services/ai/gemini_ai_service.dart (1)
176-197: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate
thinkingLevelby model family.workout-logger/lib/services/ai/gemini_ai_service.dartstill exposesgemini-2.5-flashandgemini-2.5-flash-lite, but_makeBody()always sendsgenerationConfig.thinkingConfig.thinkingLevel. Gemini 2.5 rejects that field, so those requests fail unless you switch 2.5 tothinkingBudgetor omitthinkingConfigfor that family.Proposed fix
- 'thinkingConfig': {'thinkingLevel': thinkingLevel}, + if (_model.startsWith('gemini-3')) + 'thinkingConfig': {'thinkingLevel': thinkingLevel},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@workout-logger/lib/services/ai/gemini_ai_service.dart` around lines 176 - 197, The request body builder in `GeminiAIService._makeBody()` always emits `generationConfig.thinkingConfig.thinkingLevel`, but that field is only valid for the Gemini 3.x family. Update the body construction to branch by model family so `gemini-3` uses `thinkingLevel` while `gemini-2.5-flash` and `gemini-2.5-flash-lite` either omit `thinkingConfig` or send the older `thinkingBudget` shape. Use the existing model selection logic in `GeminiAIService` to decide which config to include.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@workout-logger/lib/services/ai/agent_orchestrator.dart`:
- Around line 150-183: The round handling in agent_orchestrator.dart is still
entering the “no tools used” retry path after a stream/tool exception is
converted into AgentError, which can trigger unwanted extra model calls. Update
the orchestration flow around the streamFuture try/catch/finally and the
subsequent queryNeedsTools check so a round failure is tracked and exits or
rethrows before the missing-tool retry logic runs. Use the existing AgentError,
streamFuture, and queryNeedsTools flow in the round loop to keep failure
handling separate from tool-missing re-prompting.
- Around line 140-146: The orchestrator currently streams coach replies via
streamCoachReply without surfacing retry-wait status updates, so 429/transient
waits never reach the UI. Thread the AI service’s retry-status callback through
AgentOrchestrator’s stream path, and in the callback map retry-wait statuses to
AgentRetryWait so both view models can react consistently. Update the call site
around streamCoachReply and the surrounding orchestrator logic to forward these
statuses instead of dropping them.
In `@workout-logger/lib/services/ai/gemini_ai_service.dart`:
- Around line 225-235: The retry setup in GeminiAiService’s request flow leaks
sockets because each retry creates a new http.Client inside makeRequest without
closing it. Rework the streaming request path around retryPolicy.execute so the
client is created once outside the callback, reused for the request, and always
closed in a finally block after the stream completes or fails. Make sure the
change is applied in the code path that builds the http.Request and calls
client.send, keeping the retry behavior intact while ensuring cleanup.
- Around line 25-29: The Gemini model picker in kGeminiModels is using an
invalid model ID for the Gemini 3 entry. Update the model code in this list from
the current Gemini 3 value to the documented Gemini 3 model ID, and keep the
display label unchanged so the picker still shows “Gemini 3.0 Flash” while the
underlying value uses the valid API identifier.
In `@workout-logger/lib/services/ai/retry_policy.dart`:
- Around line 194-199: The retry handling in retry_policy.dart is using the
wrong response body when throwing a non-retryable error: after draining
retryResponse.stream, the code still passes the earlier body into
parseErrorMessage. Update the error path in the retry logic to parse the body
from retryResponse itself (the response that actually failed), and keep the
existing drain call only for consumption before reading/parsing the failed
response content.
- Around line 165-174: The retry wait calculation in RetryPolicy currently caps
each Retry-After only by maxWait, so repeated 429s can exceed the intended total
retry budget. Update the logic in the retry loop to track elapsed retry time
across attempts, and clamp each computed wait in relation to the remaining
allowed total time before calling onStatus and Future.delayed. Keep the behavior
centered around parseRetryAfter, backoff, maxWait, and maxRetries so the total
time stays within the documented limit.
- Around line 107-126: The parseRetryAfter helper in retry_policy.dart should
handle RFC Retry-After dates with HttpDate.parse instead of the current date
parsing path, since date-form headers are otherwise treated as invalid. Also
update the retry-after seconds handling in parseRetryAfter to clamp negative
delay-seconds to zero before creating the Duration, while still capping values
at maxWait.
In `@workout-logger/lib/viewmodels/ai_coach_view_model.dart`:
- Around line 139-141: The AgentError branch in AICoachViewModel is updating
buffer and calling notifyListeners(), but _streamingText is left unchanged so
the UI can rebuild with stale streamed content. In the ai_coach_view_model.dart
logic around the AgentError case in the stream handling, update _streamingText
at the same time you append the error to buffer, then notify listeners so the
refreshed error text is reflected immediately.
In `@workout-logger/lib/viewmodels/routine_optimizer_view_model.dart`:
- Around line 169-171: The AgentError branch in
routine_optimizer_view_model.dart buffers the error message but never updates
the live streaming state, so the UI keeps showing stale text. In the switch case
for AgentError inside the routine optimizer view model, update _streamingText
with the new buffer contents before calling _notify(), and keep the existing
persistence path intact so the loading bubble refreshes immediately with the
error text.
In `@workout-logger/test/retry_policy_test.dart`:
- Around line 9-32: The RetryPolicy.parseRetryAfter tests only cover
delay-seconds and miss the HTTP-date and negative-value cases called out in the
review. Update the retry policy test in retry_policy_test.dart by adding
expectations around parseRetryAfter for an RFC-style HTTP-date value and for a
negative Retry-After value, using the RetryPolicy symbol and its parseRetryAfter
method so the parser’s contract is verified for both supported formats and
invalid input.
---
Outside diff comments:
In `@workout-logger/lib/services/ai/gemini_ai_service.dart`:
- Around line 176-197: The request body builder in `GeminiAIService._makeBody()`
always emits `generationConfig.thinkingConfig.thinkingLevel`, but that field is
only valid for the Gemini 3.x family. Update the body construction to branch by
model family so `gemini-3` uses `thinkingLevel` while `gemini-2.5-flash` and
`gemini-2.5-flash-lite` either omit `thinkingConfig` or send the older
`thinkingBudget` shape. Use the existing model selection logic in
`GeminiAIService` to decide which config to include.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 112d5ac9-da73-4a3f-a51f-8aa2f7c34bbe
📒 Files selected for processing (14)
workout-logger/lib/main.dartworkout-logger/lib/screens/ai_coach_screen.dartworkout-logger/lib/screens/routine_optimizer_screen.dartworkout-logger/lib/services/ai/agent_event.dartworkout-logger/lib/services/ai/agent_orchestrator.dartworkout-logger/lib/services/ai/gemini_ai_service.dartworkout-logger/lib/services/ai/retry_policy.dartworkout-logger/lib/viewmodels/ai_coach_view_model.dartworkout-logger/lib/viewmodels/routine_optimizer_view_model.dartworkout-logger/test/agent_orchestrator_test.dartworkout-logger/test/ai_coach_view_model_test.dartworkout-logger/test/retry_policy_test.dartworkout-logger/test/routine_optimizer_screen_test.dartworkout-logger/test/routine_optimizer_view_model_test.dart
| await for (final chunk in _ai.streamCoachReply( | ||
| userMessage: currentUserMessage, | ||
| systemPrompt: systemPrompt, | ||
| history: currentHistory, | ||
| tools: tools, | ||
| onToolCall: instrumentedToolCall, | ||
| )) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Emit AgentRetryWait from retry status callbacks.
Both view models handle AgentRetryWait, but this orchestrator never emits it, so 429/transient retry waits won’t surface in the new UI. Thread the retry-status callback from the AI service into this call and map retry-wait statuses to AgentRetryWait.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/services/ai/agent_orchestrator.dart` around lines 140 -
146, The orchestrator currently streams coach replies via streamCoachReply
without surfacing retry-wait status updates, so 429/transient waits never reach
the UI. Thread the AI service’s retry-status callback through
AgentOrchestrator’s stream path, and in the callback map retry-wait statuses to
AgentRetryWait so both view models can react consistently. Update the call site
around streamCoachReply and the surrounding orchestrator logic to forward these
statuses instead of dropping them.
| } catch (e) { | ||
| safeAdd(AgentError('$e')); | ||
| } finally { | ||
| if (!isClosed) { | ||
| isClosed = true; | ||
| await controller.close(); | ||
| } | ||
| } | ||
| }(); | ||
|
|
||
| // Yield events from the controller as they arrive. | ||
| yield* controller.stream; | ||
|
|
||
| // Ensure the stream future completes. | ||
| await streamFuture; | ||
|
|
||
| final roundReply = currentRoundTextBuffer.toString().trim(); | ||
|
|
||
| // Check if we need another round: did the user ask a query that needs | ||
| // tools, but the model didn't call any tools? | ||
| final queryNeedsTools = _queryRequiresTools(userMessage); | ||
| if (queryNeedsTools && toolsUsed.isEmpty && round < maxRounds - 1) { | ||
| // Model failed to use tools. Update history and feedback prompt. | ||
| currentHistory.add(Content.text(currentUserMessage)); | ||
| currentHistory.add(Content.model([TextPart(roundReply)])); | ||
|
|
||
| currentUserMessage = 'You are answering a query about the user\'s progress or history, ' | ||
| 'but you did not query their actual logged workouts. Please use the relevant tools ' | ||
| '(e.g. get_exercise_performance, get_workouts_in_range, get_personal_records) ' | ||
| 'to retrieve the user\'s real data before answering.'; | ||
|
|
||
| yield const AgentStatusUpdate('Analyzing further with database tools…'); | ||
| yield const AgentTextChunk('\n\n'); // Spacer between attempts | ||
| continue; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Stop re-prompting after stream or tool failures.
The catch converts failures into AgentError, then the loop still evaluates the “no tools used” path. For progress queries, a real stream/tool failure can trigger additional model calls; it also masks caller control flow such as the optimizer abort path. Track the round failure and return/rethrow before the missing-tool retry logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/services/ai/agent_orchestrator.dart` around lines 150 -
183, The round handling in agent_orchestrator.dart is still entering the “no
tools used” retry path after a stream/tool exception is converted into
AgentError, which can trigger unwanted extra model calls. Update the
orchestration flow around the streamFuture try/catch/finally and the subsequent
queryNeedsTools check so a round failure is tracked and exits or rethrows before
the missing-tool retry logic runs. Use the existing AgentError, streamFuture,
and queryNeedsTools flow in the round loop to keep failure handling separate
from tool-missing re-prompting.
| // Ordered list of available Gemini models shown in the picker. | ||
| const kGeminiModels = [ | ||
| ('gemini-2.5-flash', 'Gemini 2.5 Flash'), | ||
| ('gemini-2.5-flash-lite', 'Gemini 2.5 Flash Lite'), | ||
| ('gemini-3.0-flash', 'Gemini 3.0 Flash'), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby references.
git ls-files workout-logger/lib/services/ai
printf '\n--- gemini_ai_service.dart (1-220) ---\n'
cat -n workout-logger/lib/services/ai/gemini_ai_service.dart | sed -n '1,220p'
printf '\n--- search for gemini model strings ---\n'
rg -n "gemini-[0-9]" workout-logger -SRepository: Devasy/Workout-logger
Length of output: 10753
🌐 Web query:
Gemini 3 Flash Preview model code documentation ai.google.dev gemini-3-flash-preview gemini-3.0-flash
💡 Result:
The Gemini 3 Flash model, identified by the model code gemini-3-flash-preview, is a preview-stage model in the Gemini 3 series released on December 17, 2025 [1][2]. It is designed to provide frontier-class intelligence optimized for speed, cost-efficiency, and agentic workflows [1][2]. Key specifications and documentation details for gemini-3-flash-preview include: Model ID: gemini-3-flash-preview [3][2]. Context Window: 1,048,576 tokens (Input) and 65,536 tokens (Output) [3][2]. Knowledge Cutoff: January 2025 [3][2]. Capabilities: Supports multimodal inputs including text, image, video, audio, and PDF [3][2]. It features advanced visual and spatial reasoning, code execution, function calling, search grounding, and Google Maps grounding [3][4][1]. Thinking Levels: The model supports customizable thinking levels to manage reasoning depth, including MINIMAL, MEDIUM, and HIGH (the default) [5][6]. Consumption: Available via the Gemini API, Google AI Studio, and Vertex AI, with support for Batch API, Flex inference, and Priority inference [3][1][2]. Developers are directed to the Gemini 3 Developer Guide for comprehensive information on features, migration, and best practices [5][4]. Note that as of mid-2026, newer models like Gemini 3.5 Flash and various 3.1 series variants have been released, and users are encouraged to check the latest developer documentation for migration guidance and updated feature sets [5][4][7].
Citations:
- 1: https://blog.google/innovation-and-ai/technology/developers-tools/build-with-gemini-3-flash/
- 2: https://docs.cloud.google.com/gemini-enterprise-agent-platform/models/gemini/3-flash
- 3: https://ai.google.dev/gemini-api/docs/models/gemini-3-flash-preview
- 4: https://ai.google.dev/gemini-api/docs/changelog
- 5: https://ai.google.dev/gemini-api/docs/gemini-3
- 6: https://docs.cloud.google.com/gemini-enterprise-agent-platform/models/start/get-started-with-gemini-3
- 7: https://docs.cloud.google.com/gemini-enterprise-agent-platform/models/model-versions
Use the documented Gemini 3 model code. gemini-3.0-flash isn’t a valid Gemini API model ID; use gemini-3-flash-preview instead.
Proposed fix
- ('gemini-3.0-flash', 'Gemini 3.0 Flash'),
+ ('gemini-3-flash-preview', 'Gemini 3 Flash Preview'),📝 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.
| // Ordered list of available Gemini models shown in the picker. | |
| const kGeminiModels = [ | |
| ('gemini-2.5-flash', 'Gemini 2.5 Flash'), | |
| ('gemini-2.5-flash-lite', 'Gemini 2.5 Flash Lite'), | |
| ('gemini-3.0-flash', 'Gemini 3.0 Flash'), | |
| // Ordered list of available Gemini models shown in the picker. | |
| const kGeminiModels = [ | |
| ('gemini-2.5-flash', 'Gemini 2.5 Flash'), | |
| ('gemini-2.5-flash-lite', 'Gemini 2.5 Flash Lite'), | |
| ('gemini-3-flash-preview', 'Gemini 3 Flash Preview'), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/services/ai/gemini_ai_service.dart` around lines 25 - 29,
The Gemini model picker in kGeminiModels is using an invalid model ID for the
Gemini 3 entry. Update the model code in this list from the current Gemini 3
value to the documented Gemini 3 model ID, and keep the display label unchanged
so the picker still shows “Gemini 3.0 Flash” while the underlying value uses the
valid API identifier.
| // Use RetryPolicy to establish the initial connection. | ||
| final streamed = await retryPolicy.execute( | ||
| makeRequest: () { | ||
| final client = http.Client(); | ||
| final request = http.Request('POST', uri) | ||
| ..headers['Content-Type'] = 'application/json' | ||
| ..body = encodedBody; | ||
| return client.send(request); | ||
| }, | ||
| onStatus: onRetryStatus, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines
sed -n '180,290p' workout-logger/lib/services/ai/gemini_ai_service.dart
# Look for any existing client lifecycle handling in this service
rg -n "http\.Client|client\.close\(|RetryPolicy|execute\(" workout-logger/lib/services/ai/gemini_ai_service.dartRepository: Devasy/Workout-logger
Length of output: 4785
Close the manually created http.Client after the stream finishes.
makeRequest allocates a new client on every retry attempt and never closes it, so retries and long-lived streams can leak sockets. Move the client outside the callback and close it in finally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/services/ai/gemini_ai_service.dart` around lines 225 -
235, The retry setup in GeminiAiService’s request flow leaks sockets because
each retry creates a new http.Client inside makeRequest without closing it.
Rework the streaming request path around retryPolicy.execute so the client is
created once outside the callback, reused for the request, and always closed in
a finally block after the stream completes or fails. Make sure the change is
applied in the code path that builds the http.Request and calls client.send,
keeping the retry behavior intact while ensuring cleanup.
| Duration? parseRetryAfter(Map<String, String> headers) { | ||
| final value = headers['retry-after'] ?? headers['Retry-After']; | ||
| if (value == null || value.isEmpty) return null; | ||
|
|
||
| // Try as seconds first (most common for Gemini 429s). | ||
| final seconds = int.tryParse(value); | ||
| if (seconds != null) { | ||
| return Duration(seconds: math.min(seconds, maxWait.inSeconds)); | ||
| } | ||
|
|
||
| // Try as HTTP-date. | ||
| try { | ||
| final date = _parseHttpDate(value); | ||
| final diff = date.difference(DateTime.now()); | ||
| if (diff.isNegative) return Duration.zero; | ||
| return diff > maxWait ? maxWait : diff; | ||
| } catch (_) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="workout-logger/lib/services/ai/retry_policy.dart"
echo "== outline =="
ast-grep outline "$file" --view expanded || true
echo "== lines 1-260 =="
nl -ba "$file" | sed -n '1,260p'Repository: Devasy/Workout-logger
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== matching files =="
git ls-files | rg '^workout-logger/lib/services/ai/.*retry_policy\.dart$|^.*retry_policy\.dart$' || true
echo "== retry policy files =="
fd -a 'retry_policy.dart' . || true
echo "== parseRetryAfter references =="
rg -n "parseRetryAfter|retry-after|Retry-After|HttpDate|DateTime\.parse|_parseHttpDate" workout-logger 2>/dev/null || true
echo "== file content =="
file="workout-logger/lib/services/ai/retry_policy.dart"
if [ -f "$file" ]; then
awk 'NR>=1 && NR<=260 { printf "%4d %s\n", NR, $0 }' "$file"
fiRepository: Devasy/Workout-logger
Length of output: 13569
Use HttpDate.parse here and clamp negative seconds to zero.
DateTime.parse rejects RFC Retry-After dates, so date-form headers are treated as unparseable. Negative delay-seconds also produce negative Durations and can surface as “retrying in -5s”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/services/ai/retry_policy.dart` around lines 107 - 126, The
parseRetryAfter helper in retry_policy.dart should handle RFC Retry-After dates
with HttpDate.parse instead of the current date parsing path, since date-form
headers are otherwise treated as invalid. Also update the retry-after seconds
handling in parseRetryAfter to clamp negative delay-seconds to zero before
creating the Duration, while still capping values at maxWait.
Source: Learnings
| if (attempt < maxRetries) { | ||
| // Determine wait duration: Retry-After header > backoff. | ||
| final retryAfter = parseRetryAfter(response.headers); | ||
| final wait = retryAfter ?? backoff(attempt); | ||
| final reason = response.statusCode == 429 | ||
| ? 'Rate limit reached' | ||
| : 'Server busy (${response.statusCode})'; | ||
|
|
||
| onStatus?.call(RetryWaiting(wait, reason, attempt + 1, maxRetries)); | ||
| await Future<void>.delayed(wait); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep fast retry waits within maxWait.
Line 168 caps each Retry-After to maxWait, not the remaining total budget. With multiple 429 responses carrying long Retry-After values, total retry time can exceed the documented “maximum total time” by maxRetries × maxWait.
Proposed direction
- final wait = retryAfter ?? backoff(attempt);
+ final requestedWait = retryAfter ?? backoff(attempt);
+ final remaining = maxWait - stopwatch.elapsed;
+ if (remaining <= Duration.zero) {
+ final error = parseErrorMessage(response.statusCode, body);
+ onStatus?.call(RetryExhausted(error, response.statusCode));
+ throw Exception(
+ 'API unavailable after ${stopwatch.elapsed.inSeconds}s of retrying: $error',
+ );
+ }
+ final wait = requestedWait > remaining ? remaining : requestedWait;📝 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.
| if (attempt < maxRetries) { | |
| // Determine wait duration: Retry-After header > backoff. | |
| final retryAfter = parseRetryAfter(response.headers); | |
| final wait = retryAfter ?? backoff(attempt); | |
| final reason = response.statusCode == 429 | |
| ? 'Rate limit reached' | |
| : 'Server busy (${response.statusCode})'; | |
| onStatus?.call(RetryWaiting(wait, reason, attempt + 1, maxRetries)); | |
| await Future<void>.delayed(wait); | |
| if (attempt < maxRetries) { | |
| // Determine wait duration: Retry-After header > backoff. | |
| final requestedWait = retryAfter ?? backoff(attempt); | |
| final remaining = maxWait - stopwatch.elapsed; | |
| if (remaining <= Duration.zero) { | |
| final error = parseErrorMessage(response.statusCode, body); | |
| onStatus?.call(RetryExhausted(error, response.statusCode)); | |
| throw Exception( | |
| 'API unavailable after ${stopwatch.elapsed.inSeconds}s of retrying: $error', | |
| ); | |
| } | |
| final wait = requestedWait > remaining ? remaining : requestedWait; | |
| final reason = response.statusCode == 429 | |
| ? 'Rate limit reached' | |
| : 'Server busy (${response.statusCode})'; | |
| onStatus?.call(RetryWaiting(wait, reason, attempt + 1, maxRetries)); | |
| await Future<void>.delayed(wait); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/services/ai/retry_policy.dart` around lines 165 - 174, The
retry wait calculation in RetryPolicy currently caps each Retry-After only by
maxWait, so repeated 429s can exceed the intended total retry budget. Update the
logic in the retry loop to track elapsed retry time across attempts, and clamp
each computed wait in relation to the remaining allowed total time before
calling onStatus and Future.delayed. Keep the behavior centered around
parseRetryAfter, backoff, maxWait, and maxRetries so the total time stays within
the documented limit.
| // Drain the failed response body. | ||
| await retryResponse.stream.bytesToString(); | ||
| if (!isRetryableStatus(retryResponse.statusCode)) { | ||
| final errBody = body; // already drained above | ||
| throw Exception( | ||
| parseErrorMessage(retryResponse.statusCode, errBody)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse the body from the response that actually failed.
Line 195 drains retryResponse, but Line 197 reuses the earlier body, so a later non-retryable error reports the wrong Gemini error message.
Proposed fix
- // Drain the failed response body.
- await retryResponse.stream.bytesToString();
+ final errBody = await retryResponse.stream.bytesToString();
if (!isRetryableStatus(retryResponse.statusCode)) {
- final errBody = body; // already drained above
throw Exception(
parseErrorMessage(retryResponse.statusCode, errBody));
}📝 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.
| // Drain the failed response body. | |
| await retryResponse.stream.bytesToString(); | |
| if (!isRetryableStatus(retryResponse.statusCode)) { | |
| final errBody = body; // already drained above | |
| throw Exception( | |
| parseErrorMessage(retryResponse.statusCode, errBody)); | |
| final errBody = await retryResponse.stream.bytesToString(); | |
| if (!isRetryableStatus(retryResponse.statusCode)) { | |
| throw Exception( | |
| parseErrorMessage(retryResponse.statusCode, errBody)); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/services/ai/retry_policy.dart` around lines 194 - 199, The
retry handling in retry_policy.dart is using the wrong response body when
throwing a non-retryable error: after draining retryResponse.stream, the code
still passes the earlier body into parseErrorMessage. Update the error path in
the retry logic to parse the body from retryResponse itself (the response that
actually failed), and keep the existing drain call only for consumption before
reading/parsing the failed response content.
| case AgentError(:final message): | ||
| buffer.write('\n\n_Error: ${message}_'); | ||
| notifyListeners(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Refresh streaming text for agent errors.
This writes the error into buffer but not _streamingText, so the rebuild triggered by notifyListeners() still shows stale content.
Proposed fix
case AgentError(:final message):
buffer.write('\n\n_Error: ${message}_');
+ _streamingText = buffer.toString();
+ _statusText = '';
notifyListeners();📝 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.
| case AgentError(:final message): | |
| buffer.write('\n\n_Error: ${message}_'); | |
| notifyListeners(); | |
| case AgentError(:final message): | |
| buffer.write('\n\n_Error: ${message}_'); | |
| _streamingText = buffer.toString(); | |
| _statusText = ''; | |
| notifyListeners(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/viewmodels/ai_coach_view_model.dart` around lines 139 -
141, The AgentError branch in AICoachViewModel is updating buffer and calling
notifyListeners(), but _streamingText is left unchanged so the UI can rebuild
with stale streamed content. In the ai_coach_view_model.dart logic around the
AgentError case in the stream handling, update _streamingText at the same time
you append the error to buffer, then notify listeners so the refreshed error
text is reflected immediately.
| case AgentError(:final message): | ||
| buffer.write('\n\n_Error: ${message}_'); | ||
| _notify(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Refresh streaming text for agent errors.
This buffers the error but never updates _streamingText, so the loading bubble rebuilds with stale content until persistence finishes.
Proposed fix
case AgentError(:final message):
buffer.write('\n\n_Error: ${message}_');
+ _streamingText = buffer.toString();
+ _statusText = '';
_notify();📝 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.
| case AgentError(:final message): | |
| buffer.write('\n\n_Error: ${message}_'); | |
| _notify(); | |
| case AgentError(:final message): | |
| buffer.write('\n\n_Error: ${message}_'); | |
| _streamingText = buffer.toString(); | |
| _statusText = ''; | |
| _notify(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/lib/viewmodels/routine_optimizer_view_model.dart` around lines
169 - 171, The AgentError branch in routine_optimizer_view_model.dart buffers
the error message but never updates the live streaming state, so the UI keeps
showing stale text. In the switch case for AgentError inside the routine
optimizer view model, update _streamingText with the new buffer contents before
calling _notify(), and keep the existing persistence path intact so the loading
bubble refreshes immediately with the error text.
| test('parseRetryAfter returns correct durations', () { | ||
| const policy = RetryPolicy(maxWait: Duration(seconds: 60)); | ||
|
|
||
| // Seconds parsing | ||
| expect( | ||
| policy.parseRetryAfter({'retry-after': '12'}), | ||
| const Duration(seconds: 12), | ||
| ); | ||
| expect( | ||
| policy.parseRetryAfter({'Retry-After': '5'}), | ||
| const Duration(seconds: 5), | ||
| ); | ||
|
|
||
| // Capped at maxWait | ||
| expect( | ||
| policy.parseRetryAfter({'retry-after': '120'}), | ||
| const Duration(seconds: 60), | ||
| ); | ||
|
|
||
| // Empty or missing | ||
| expect(policy.parseRetryAfter({}), null); | ||
| expect(policy.parseRetryAfter({'retry-after': ''}), null); | ||
| expect(policy.parseRetryAfter({'retry-after': 'abc'}), null); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Cover HTTP-date and invalid negative Retry-After values.
The parser claims HTTP-date support, but these tests only cover delay-seconds. Add coverage for RFC-style dates and negative seconds so the retry countdown contract stays correct. Dart’s HTTP-date parser supports the expected HTTP formats. (api.dart.dev)
Proposed test additions
+import 'dart:io' show HttpDate;
+
@@
expect(policy.parseRetryAfter({'retry-after': 'abc'}), null);
+
+ final futureHttpDate = HttpDate.format(
+ DateTime.now().toUtc().add(const Duration(seconds: 10)),
+ );
+ final parsed = policy.parseRetryAfter({'retry-after': futureHttpDate});
+ expect(parsed?.inSeconds, inInclusiveRange(8, 10));
+
+ expect(
+ policy.parseRetryAfter({'retry-after': '-5'}),
+ Duration.zero,
+ );
});Based on learnings, Web is not a supported target for this app unless the change explicitly enables it.
📝 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.
| test('parseRetryAfter returns correct durations', () { | |
| const policy = RetryPolicy(maxWait: Duration(seconds: 60)); | |
| // Seconds parsing | |
| expect( | |
| policy.parseRetryAfter({'retry-after': '12'}), | |
| const Duration(seconds: 12), | |
| ); | |
| expect( | |
| policy.parseRetryAfter({'Retry-After': '5'}), | |
| const Duration(seconds: 5), | |
| ); | |
| // Capped at maxWait | |
| expect( | |
| policy.parseRetryAfter({'retry-after': '120'}), | |
| const Duration(seconds: 60), | |
| ); | |
| // Empty or missing | |
| expect(policy.parseRetryAfter({}), null); | |
| expect(policy.parseRetryAfter({'retry-after': ''}), null); | |
| expect(policy.parseRetryAfter({'retry-after': 'abc'}), null); | |
| }); | |
| import 'dart:io' show HttpDate; | |
| test('parseRetryAfter returns correct durations', () { | |
| const policy = RetryPolicy(maxWait: Duration(seconds: 60)); | |
| // Seconds parsing | |
| expect( | |
| policy.parseRetryAfter({'retry-after': '12'}), | |
| const Duration(seconds: 12), | |
| ); | |
| expect( | |
| policy.parseRetryAfter({'Retry-After': '5'}), | |
| const Duration(seconds: 5), | |
| ); | |
| // Capped at maxWait | |
| expect( | |
| policy.parseRetryAfter({'retry-after': '120'}), | |
| const Duration(seconds: 60), | |
| ); | |
| // Empty or missing | |
| expect(policy.parseRetryAfter({}), null); | |
| expect(policy.parseRetryAfter({'retry-after': ''}), null); | |
| expect(policy.parseRetryAfter({'retry-after': 'abc'}), null); | |
| final futureHttpDate = HttpDate.format( | |
| DateTime.now().toUtc().add(const Duration(seconds: 10)), | |
| ); | |
| final parsed = policy.parseRetryAfter({'retry-after': futureHttpDate}); | |
| expect(parsed?.inSeconds, inInclusiveRange(8, 10)); | |
| expect( | |
| policy.parseRetryAfter({'retry-after': '-5'}), | |
| Duration.zero, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@workout-logger/test/retry_policy_test.dart` around lines 9 - 32, The
RetryPolicy.parseRetryAfter tests only cover delay-seconds and miss the
HTTP-date and negative-value cases called out in the review. Update the retry
policy test in retry_policy_test.dart by adding expectations around
parseRetryAfter for an RFC-style HTTP-date value and for a negative Retry-After
value, using the RetryPolicy symbol and its parseRetryAfter method so the
parser’s contract is verified for both supported formats and invalid input.
Source: Learnings
|
@coderabbitai I need you to review the orchestration layer against the likes of langchain/langraph. As this will be the base for this apps, agentic AI services, and not just that, It will be reused in other apps as well. There is a future plan off adding tools like: showGraph tool where ai can pull multiple data from sources and then process it and then show a plot on the screen to user. in the chat itself. just like genui. So the implementation needs to be scalable and modular to allow this things |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Locate relevant files =="
fd -i 'agent_orchestrator.dart|agent_event.dart|retry_policy.dart|gemini_ai_service.dart|ai_coach_view_model.dart|routine_optimizer_view_model.dart|agent_orchestrator_test.dart' .
echo
echo "== AST outlines =="
ast-grep outline workout-logger/lib/services/ai/agent_orchestrator.dart --view expanded || true
echo
ast-grep outline workout-logger/lib/services/ai/agent_event.dart --view expanded || true
echo
ast-grep outline workout-logger/lib/services/ai/retry_policy.dart --view expanded || true
echo
ast-grep outline workout-logger/lib/services/ai/gemini_ai_service.dart --view expanded || true
echo
ast-grep outline workout-logger/lib/viewmodels/ai_coach_view_model.dart --view expanded || true
echo
ast-grep outline workout-logger/lib/viewmodels/routine_optimizer_view_model.dart --view expanded || true
echo
ast-grep outline workout-logger/test/agent_orchestrator_test.dart --view expanded || true
echo
echo "== agent_orchestrator.dart =="
sed -n '1,260p' workout-logger/lib/services/ai/agent_orchestrator.dart
echo
echo "== agent_event.dart =="
sed -n '1,220p' workout-logger/lib/services/ai/agent_event.dart
echo
echo "== retry_policy.dart =="
sed -n '1,280p' workout-logger/lib/services/ai/retry_policy.dart
echo
echo "== gemini_ai_service.dart (targeted excerpts) =="
sed -n '1,320p' workout-logger/lib/services/ai/gemini_ai_service.dart
echo
echo "== ai_coach_view_model.dart =="
sed -n '1,260p' workout-logger/lib/viewmodels/ai_coach_view_model.dart
echo
echo "== routine_optimizer_view_model.dart =="
sed -n '1,260p' workout-logger/lib/viewmodels/routine_optimizer_view_model.dart
echo
echo "== agent_orchestrator_test.dart =="
sed -n '1,260p' workout-logger/test/agent_orchestrator_test.dartLength of output: 51743
Short version: good first orchestration pass for a single app, but not yet the right long-term foundation if this is meant to become a reusable agent runtime across apps. What is good already
Where it falls short for a reusable agent framework1. The orchestrator is still policy-hardcoded, not graph/state-drivenIn
That works for one coach flow, but it does not scale like LangGraph where the runtime is driven by explicit state + node transitions. Right now, if you later add:
you’ll keep growing conditionals inside one orchestrator. My review take: this should evolve into a small runtime with:
So the orchestration policy is configurable, not embedded in one class. 2. Tool semantics are mixed with UI presentation
That means the orchestration layer currently knows:
That is a coupling problem. For a reusable base, tools should declare their own metadata, e.g.:
The orchestrator should emit structured events, and the UI should decide how to render them. 3. The orchestrator does not fully own the agent loopThis is the biggest architectural gap compared with LangGraph-style systems. Your comment in That means the orchestrator is not truly orchestrating the core cycle:
Instead, it is mostly observing a provider-owned loop. That makes it much harder to support future flows like:
Recommendation: move toward a lower-level AI provider contract, something like:
Then let the orchestrator own the loop. 4.
|
|
@coderabbitai Can you turn this into a concrete target architecture for this repo like a LangGraph-inspired Dart design |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Locate likely relevant files =="
fd -i 'iai_service.dart|ai_service.dart|coach_tool_service.dart|conversation_manager.dart|conversation.dart|message.dart|ai_coach_view_model.dart|routine_optimizer_view_model.dart|agent_orchestrator.dart|agent_event.dart|gemini_ai_service.dart' workout-logger/lib
echo
echo "== AST outlines for core AI files =="
for f in \
workout-logger/lib/services/ai/agent_orchestrator.dart \
workout-logger/lib/services/ai/agent_event.dart \
workout-logger/lib/services/ai/gemini_ai_service.dart \
workout-logger/lib/viewmodels/ai_coach_view_model.dart \
workout-logger/lib/viewmodels/routine_optimizer_view_model.dart
do
if [ -f "$f" ]; then
echo "--- OUTLINE: $f ---"
ast-grep outline "$f" --view expanded || true
echo
fi
done
echo "== Find interface/service definitions =="
rg -n -C3 --glob 'workout-logger/lib/**/*.dart' \
'abstract class IAiService|class CoachToolService|class ConversationManager|typedef|enum MessageRole|class Conversation|class ChatMessage|class Message' \
workout-logger/lib
echo
echo "== Read targeted files if present =="
for f in \
workout-logger/lib/services/ai/ai_service.dart \
workout-logger/lib/services/ai/i_ai_service.dart \
workout-logger/lib/services/coach_tool_service.dart \
workout-logger/lib/services/conversation_manager.dart \
workout-logger/lib/models/conversation.dart \
workout-logger/lib/models/chat_message.dart \
workout-logger/lib/models/message.dart \
workout-logger/lib/viewmodels/ai_coach_view_model.dart \
workout-logger/lib/viewmodels/routine_optimizer_view_model.dart \
workout-logger/lib/services/ai/agent_orchestrator.dart \
workout-logger/lib/services/ai/agent_event.dart
do
if [ -f "$f" ]; then
echo "--- FILE: $f (first 260 lines) ---"
sed -n '1,260p' "$f"
echo
fi
doneLength of output: 32201 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Read AI interface, tool service, and chat models =="
for f in \
workout-logger/lib/services/interfaces/ai_service_interface.dart \
workout-logger/lib/services/ai/coach_tool_service.dart \
workout-logger/lib/services/managers/conversation_manager.dart \
workout-logger/lib/models/models.dart
do
echo "--- FILE: $f ---"
sed -n '1,280p' "$f"
echo
done
echo "== Focused searches for tool declarations and message persistence APIs =="
rg -n -C3 \
'buildTools\s*\(|handleCall\s*\(|askUserQuestionsDeclaration|appendMessage\s*\(|startNewConversation\s*\(|selectConversation\s*\(|class ChatMessage|class Conversation' \
workout-logger/lib/services/ai/coach_tool_service.dart \
workout-logger/lib/services/managers/conversation_manager.dart \
workout-logger/lib/models/models.dart \
workout-logger/lib/services/interfaces/ai_service_interface.dartLength of output: 33964
Target outcomeUse this mental model:
So the system becomes: flowchart LR
UI["Screens / ViewModels"] --> RT["AgentRuntime"]
RT --> G["AgentGraph / Nodes"]
G --> MR["ModelRuntime"]
G --> TR["ToolRegistry / ToolExecutor"]
G --> HR["HumanInterruptHandler"]
MR --> GP["GeminiProviderAdapter"]
TR --> TS["CoachToolService-backed tools"]
RT --> EV["AgentEvent stream"]
RT --> AF["Typed Artifacts"]
UI --> CM["ConversationManager"]
1. What to keep vs what to changeKeepThese are already good foundations:
ChangeThese should evolve:
2. Proposed module layoutI’d aim for this structure: You do not need to create all of this in one PR. This is the target shape. 3. Core design: graph + state + node executionThe main shift is: Current
Target
That’s the LangGraph-style part. Core runtime typessealed class AgentNodeResult {
const AgentNodeResult();
}
class NextNode extends AgentNodeResult {
final String nodeId;
const NextNode(this.nodeId);
}
class CompleteRun extends AgentNodeResult {
const CompleteRun();
}
class InterruptRun extends AgentNodeResult {
final AgentInterrupt interrupt;
const InterruptRun(this.interrupt);
}class AgentRunState {
final String runId;
final String graphId;
final String userMessage;
final List<ModelMessage> transcript;
final List<ToolInvocation> toolCalls;
final List<AgentArtifact> artifacts;
final String? statusText;
final Set<String> activeToolIds;
final Map<String, Object?> workingMemory;
final AgentPhase phase;
final int round;
final bool isComplete;
const AgentRunState({
required this.runId,
required this.graphId,
required this.userMessage,
required this.transcript,
required this.toolCalls,
required this.artifacts,
required this.workingMemory,
required this.activeToolIds,
this.statusText,
this.phase = AgentPhase.idle,
this.round = 0,
this.isComplete = false,
});
AgentRunState copyWith({...});
}abstract class AgentNode {
String get id;
Future<AgentNodeResult> execute(
AgentContext ctx,
AgentRunState state,
);
}4. Split the provider contract from orchestrationThis is the most important refactor. Right now
That makes the provider too smart. Replace it with a lower-level model contractTarget provider contractabstract class ModelRuntime {
bool get isConfigured;
String get currentModel;
Stream<ModelStep> streamStep({
required String systemPrompt,
required List<ModelMessage> messages,
required List<ToolSpec> tools,
});
}
|
|
@coderabbitai you can give repo-specific implementation plan with: exact new files to add |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Relevant AI/runtime file tree =="
fd . workout-logger/lib/services/ai workout-logger/lib/services/interfaces workout-logger/lib/services/managers workout-logger/lib/models workout-logger/lib/viewmodels | sed 's#^\./##' | sort
echo
echo "== AST outlines for exact files we may rename/split =="
for f in \
workout-logger/lib/services/interfaces/ai_service_interface.dart \
workout-logger/lib/services/ai/gemini_ai_service.dart \
workout-logger/lib/services/ai/agent_orchestrator.dart \
workout-logger/lib/services/ai/agent_event.dart \
workout-logger/lib/services/ai/coach_tool_service.dart \
workout-logger/lib/services/managers/conversation_manager.dart \
workout-logger/lib/models/models.dart \
workout-logger/lib/viewmodels/ai_coach_view_model.dart \
workout-logger/lib/viewmodels/routine_optimizer_view_model.dart
do
if [ -f "$f" ]; then
echo "--- OUTLINE: $f ---"
ast-grep outline "$f" --view expanded || true
echo
fi
done
echo "== Targeted excerpts for signatures and responsibilities =="
for f in \
workout-logger/lib/services/interfaces/ai_service_interface.dart \
workout-logger/lib/services/ai/gemini_ai_service.dart \
workout-logger/lib/services/ai/agent_orchestrator.dart \
workout-logger/lib/services/ai/coach_tool_service.dart \
workout-logger/lib/services/managers/conversation_manager.dart \
workout-logger/lib/models/models.dart \
workout-logger/lib/viewmodels/ai_coach_view_model.dart \
workout-logger/lib/viewmodels/routine_optimizer_view_model.dart
do
if [ -f "$f" ]; then
echo "--- FILE: $f (lines 1-260) ---"
sed -n '1,260p' "$f"
echo
fi
done
echo "== Focused searches for core symbols =="
rg -n -C2 \
'abstract class IAiService|streamCoachReply\s*\(|buildTools\s*\(|handleCall\s*\(|askUserQuestionsDeclaration|appendMessage\s*\(|startNewConversation\s*\(|selectConversation\s*\(|class ChatMessage|class Conversation|enum MessageRole' \
workout-logger/libLength of output: 78113
1. What I would do in the next PRI would keep the next PR focused on this:
That gets you the scalable core without rewriting the whole app. 2. Exact new files to addA. Provider layerAdd these files: B. Runtime layerAdd these files: C. Tools layerAdd these files: D. Graph definitionsAdd these files: E. Optional UI adapter layerUseful if you want to keep UI-facing events decoupled: F. New testsAdd these tests: 3. Which current files to rename/splitHere’s the repo-specific split plan.
|
Summary by CodeRabbit
New Features
Bug Fixes