Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a full agent-templates feature: routing and settings entry, service and exception, template models and evolution workflow, UI (list, detail, model selector, 1‑on‑1 evolve), metrics, extensive i18n, tests, mocks, and a version/changelog bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Agent 1-on-1 Page
participant Service as AgentTemplateService
participant Workflow as TemplateEvolutionWorkflow
participant LLM as LLM API
participant DB as Database
User->>UI: Enter feedback & tap "Evolve Template"
UI->>Service: Request template, active version, and performance metrics
Service->>DB: Query template, versions, and metrics
DB-->>Service: Return template + metrics
Service-->>UI: Return data
UI->>Workflow: Request evolution proposal (directives + metrics + feedback)
Workflow->>LLM: Send payload for rewrite
LLM-->>Workflow: Return proposed directives
Workflow-->>UI: Return EvolutionProposal
UI->>User: Show proposal preview
alt Approve
UI->>Service: Create new template version with proposed directives
Service->>DB: Insert version record
DB-->>Service: Confirm
Service-->>UI: Success + invalidate providers
UI->>User: Show success snackbar
else Reject
UI->>User: Clear proposal (no DB change)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Summary of ChangesHello @matthiasn, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a significant new feature by introducing a dedicated user interface for managing AI agent templates. Users can now create, edit, and version agent templates, providing a structured way to define agent personalities and directives. A key innovation is the integration of LLM-assisted evolution, enabling templates to be refined based on performance metrics and user feedback. This enhancement streamlines the process of developing and optimizing AI agents within the application. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
matthiasn
force-pushed
the
feat/agent_templates_rebased
branch
from
964b152 to
1399774
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-implemented feature: the UI layer for agent templates. It adds several new pages, widgets, and corresponding routes for creating, listing, editing, and managing agent templates, including a sophisticated "1-on-1" page for LLM-assisted template evolution. The changes are comprehensive, covering UI, state management, routing, and extensive testing. My feedback focuses on a couple of areas to enhance the robustness and maintainability of the new routing logic and error handling.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2698 +/- ##
==========================================
+ Coverage 89.25% 89.40% +0.15%
==========================================
Files 829 833 +4
Lines 52132 52920 +788
==========================================
+ Hits 46528 47311 +783
- Misses 5604 5609 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (6)
lib/l10n/app_fr.arb (1)
162-162: Optional: "Retour" as the feedback section title could be mistaken for a navigation action.
agentTemplateFeedbackTitle: "Retour"is valid French for feedback, but "Retour" is predominantly associated with back-navigation in French-language UIs (e.g., standard← Retourback buttons). In a form/section heading context,"Commentaires"or"Retour d'expérience"would be unambiguous.💡 Suggested alternative
- "agentTemplateFeedbackTitle": "Retour", + "agentTemplateFeedbackTitle": "Commentaires",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_fr.arb` at line 162, The localization key agentTemplateFeedbackTitle currently maps to "Retour", which is ambiguous with navigation; update the value to an unambiguous French heading such as "Commentaires" or "Retour d'expérience" by replacing the string for agentTemplateFeedbackTitle in lib/l10n/app_fr.arb so the feedback section title reads clearly as user feedback rather than a back action.lib/l10n/app_localizations.dart (1)
699-745: Add descriptions for the new localization getters.The generated docs show “No description provided” for the new agentTemplate keys, which leaves translators without context. Consider adding descriptions in the
.arbentries and regenerating.As per coding guidelines: “Whenever touching any function, consider its docstring and if it needs updating”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_localizations.dart` around lines 699 - 745, The new localization getters (agentTemplateFeedbackChangesHint, agentTemplateFeedbackChangesLabel, agentTemplateFeedbackDidntWorkHint, agentTemplateFeedbackDidntWorkLabel, agentTemplateFeedbackEnjoyedHint, agentTemplateFeedbackEnjoyedLabel, agentTemplateFeedbackTitle, agentTemplateInstanceCount) lack translator descriptions; open the corresponding .arb file, add meaningful "description" fields for each key (explaining context, usage, placeholders like {count}), save and then regenerate the Dart localization files so the docstrings in lib/l10n/app_localizations.dart are updated.test/features/agents/ui/agent_template_list_page_test.dart (1)
39-138: Avoid blanketpumpAndSettle()in these widget tests.
Most tests callpumpAndSettle()right afterpumpWidget(). If there are no animations or ongoing frames, considerawait tester.pump()(or a boundedpump(Duration(...))) to keep tests faster and more deterministic.As per coding guidelines, "Prefer
tester.pump(duration)overtester.pumpAndSettle(); usepumpAndSettleonly when genuinely needed and never with duration > 1 second".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/agents/ui/agent_template_list_page_test.dart` around lines 39 - 138, The tests in AgentTemplateListPage (e.g., tests "renders template cards...", "shows empty state...", "shows page title...", "has FAB for creating new template") use blanket await tester.pumpAndSettle() after pumpWidget/buildSubject; replace those with await tester.pump() or a bounded await tester.pump(Duration(milliseconds: 50)) where there are no animations, keeping pumpAndSettle only in the "shows version number from active version" case if awaiting async provider overrides via makeTestableWidgetNoScroll/agentTemplatesProvider/activeTemplateVersionProvider requires settling; update calls in those tests (referencing buildSubject, makeTestableWidgetNoScroll, and the specific test names) to use the lighter pump to make tests faster and deterministic.test/features/agents/ui/agent_one_on_one_page_test.dart (1)
49-180: ReducepumpAndSettle()usage for faster, deterministic tests.
If these screens don’t rely on animations or ongoing frames, replace the initialpumpAndSettle()calls withpump()(or a boundedpump(Duration(...))) to keep tests leaner.As per coding guidelines, "Prefer
tester.pump(duration)overtester.pumpAndSettle(); usepumpAndSettleonly when genuinely needed and never with duration > 1 second".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/agents/ui/agent_one_on_one_page_test.dart` around lines 49 - 180, Tests overuse tester.pumpAndSettle() causing slow/less-deterministic runs; replace the initial tester.pumpAndSettle() calls right after await tester.pumpWidget(buildSubject()) in the tests for AgentOneOnOnePage with a simple await tester.pump() or a bounded await tester.pump(Duration(milliseconds: 100)) (keep the explicit pumpAndSettle only where you need to wait for the scroll/drag to finish or real animations, e.g., after the tester.drag calls in the evolve button tests); update occurrences inside tests: the "shows page title with template name", "shows metrics dashboard with data", "shows feedback form fields", "evolve button is disabled when all feedback empty", and "evolve button enables after entering feedback" to use pump/pump(Duration) immediately after buildSubject(), leaving pumpAndSettle after drag/settle operations intact.lib/features/agents/ui/agent_template_detail_page.dart (1)
459-486:FutureBuilderwith inlinefuture:recreates on every build.
templateService.getAgentsForTemplate(templateId)is called inbuild(), meaning every widget rebuild creates a newFuture— causing unnecessary re-fetches and potential flickering. The idiomatic fix is to either cache the future in state or create a dedicated Riverpod provider for this lookup.Suggested approach: cache in a Riverpod provider
// In agent_providers.dart final agentsForTemplateProvider = FutureProvider.autoDispose.family<List<AgentIdentityEntity>, String>( (ref, templateId) { final service = ref.watch(agentTemplateServiceProvider); return service.getAgentsForTemplate(templateId); }, );Then in the widget:
- return FutureBuilder<List<AgentIdentityEntity>>( - future: templateService.getAgentsForTemplate(templateId), - builder: (context, snapshot) { - final agents = snapshot.data ?? []; + final agentsAsync = ref.watch(agentsForTemplateProvider(templateId)); + return agentsAsync.when( + data: (agents) { if (agents.isEmpty) return const SizedBox.shrink(); // ... rest unchanged + }, + loading: () => const SizedBox.shrink(), + error: (e, _) => const SizedBox.shrink(), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/agents/ui/agent_template_detail_page.dart` around lines 459 - 486, The FutureBuilder is recreating the Future each build because you call templateService.getAgentsForTemplate(templateId) inline in build(), causing re-fetches and flicker; fix it by moving that async lookup into a cached provider or state: create a FutureProvider/family (e.g., agentsForTemplateProvider) that calls agentTemplateServiceProvider.getAgentsForTemplate(templateId) and consume that provider in the widget (or alternatively store the Future once in State/Hook and reuse it), then replace the inline FutureBuilder future with the provider's AsyncValue to avoid recreating the Future on every build.test/features/agents/ui/agent_template_detail_page_test.dart (1)
431-457:thenThrowon an async method throws synchronously — considerthenAnswerfor fidelity.Line 436 uses
thenThrowfordeleteTemplate, which returnsFuture<void>. This throws synchronously rather than producing a failed future. It works here because theawaitre-throws into the surroundingtry/catch, butthenAnswerwith an async throw would more accurately simulate the real failure mode.Proposed fix
- when(() => mockTemplateService.deleteTemplate(any())) - .thenThrow(Exception('has instances')); + when(() => mockTemplateService.deleteTemplate(any())) + .thenAnswer((_) async => throw Exception('has instances'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/agents/ui/agent_template_detail_page_test.dart` around lines 431 - 457, The test uses thenThrow on mockTemplateService.deleteTemplate which is an async method; replace the thenThrow call with thenAnswer((_) async => throw Exception('has instances')) to return a failed Future and more accurately simulate the async failure; update the expectation code around deleteTemplate (the mockTemplateService.deleteTemplate setup) so it uses thenAnswer and keeps the rest of the test (the testWidgets block and assertions on context.messages.agentTemplateDeleteHasInstances) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flatpak/com.matthiasn.lotti.metainfo.xml`:
- Line 41: The release note for version 0.9.868 was added to
flatpak/com.matthiasn.lotti.metainfo.xml but the project CHANGELOG was not
updated; add an entry for 0.9.868 to the CHANGELOG matching the text and details
you added in the metainfo.xml (including summary of agent cross-device sync,
zone-based transaction isolation, nested rollback support, and LLM-assisted
template evolution) so both files remain consistent; ensure the version header,
date, and bullet points mirror the metainfo.xml wording and follow the existing
CHANGELOG formatting conventions.
In `@lib/features/agents/ui/agent_detail_page.dart`:
- Around line 298-342: The UI currently treats templateAsync.value == null as
"none assigned" and thus shows the agentTemplateNoneAssigned text even while the
provider is loading or errored; update the build method to inspect the
AsyncValue from templateForAgentProvider (templateAsync) using its
when/whenData/hasError/isLoading helpers and render three states: loading (e.g.,
a small CircularProgressIndicator or SizedBox), error (a short error Text using
the error message), and data (show the ActionChip using template.displayName and
navigate to AgentTemplateDetailPage with template.id); ensure you only show the
"none assigned" message when data is loaded and the template is actually null,
referencing templateForAgentProvider, templateAsync, template, and
AgentTemplateDetailPage to locate the code to change.
In `@lib/features/agents/ui/agent_model_selector.dart`:
- Around line 215-242: The "All providers" tile in _ProviderPickerContent
incorrectly uses context.messages.agentTemplateModelRequirements; replace that
string with the dedicated localization key for "All Providers" (e.g.,
context.messages.allProviders) in the _PickerTile title so the tile shows the
correct label; update the _ProviderPickerContent class where the _PickerTile is
constructed to pass the all-providers localization key instead of
agentTemplateModelRequirements.
In `@lib/features/agents/ui/agent_one_on_one_page.dart`:
- Around line 304-344: The error SnackBar in _handleApprove currently shows raw
e.toString() to users; change it to show a localized, generic message (like
context.messages.agentTemplateEvolveError) instead while keeping the
developer.log(e, s) for diagnostics, i.e., replace the SnackBar content in the
catch block of _handleApprove with
Text(context.messages.agentTemplateEvolveError) and leave the existing
developer.log call unchanged so internal details are not exposed to the user.
In `@lib/features/agents/ui/agent_template_detail_page.dart`:
- Line 84: Replace the three hardcoded user-visible strings with localized
lookups: add keys (e.g., "templateNotFound", "noVersions", "statusActive",
"statusArchived") to the ARB files in lib/l10n/ and provide translations,
regenerate localizations, then update the widgets that show 'Template not found'
(currently a const Center(child: Text(...))), 'No versions' and
'Active'/'Archived' to use the generated localization getters (for example
Text(AppLocalizations.of(context)!.templateNotFound),
Text(AppLocalizations.of(context)!.noVersions),
Text(AppLocalizations.of(context)!.statusActive) /
Text(AppLocalizations.of(context)!.statusArchived)) and remove const where
needed so context is available. Ensure keys are named clearly and run the
project’s gen-l10n/intl generation command so the new getters exist.
- Around line 428-442: The onPressed handler calls
templateService.rollbackToVersion(templateId: templateId, versionId: version.id)
without error handling, so any exception will be unhandled; wrap the await call
in a try/catch inside the onPressed (or in a helper like _handleRollback) to
catch errors from rollbackToVersion, log/report the error (e.g., process via
your logger or show a user-facing error via Navigator/ScaffoldMessenger), and
only invalidate providers (activeTemplateVersionProvider(templateId) and
templateVersionHistoryProvider(templateId)) after a successful rollback; ensure
you reference templateService, templateId, version.id, and the two providers
when implementing the try/catch.
In `@lib/features/agents/ui/agent_template_list_page.dart`:
- Around line 148-165: The subtitle Row can overflow when template.modelId is
long; update the subtitle build in AgentTemplateListPage to allow wrapping by
replacing the Row (containing _KindBadge, SizedBox, Text(template.modelId),
optional version Text) with a Wrap or make the modelId Text flexible (e.g., wrap
it in Flexible or Expanded and set softWrap/overflow properties) so
template.modelId and the version label can break onto multiple lines without
overflowing; ensure the _KindBadge and spacing remain intact and the
versionNumber branch still uses the same styled Text.
- Around line 38-49: Replace the raw error string shown in the
SliverFillRemaining error builder with a localized, user-friendly message from
the app's l10n (e.g., use AppLocalizations.of(context).<appropriateKey>) instead
of error.toString(), and move the actual exception details into a non-UI log
(use the existing logger or debugPrint) so internal details aren't displayed;
update or add the localization key in lib/l10n/*.arb and reference that key in
the Text widget inside the SliverFillRemaining error builder.
In `@lib/features/tasks/ui/header/task_header_meta_card.dart`:
- Around line 231-265: The bottom-sheet builder in _showTemplateSelectionSheet
currently lays out the Column with an unbounded list of ListTile widgets, which
can overflow; fix it by wrapping the template items in a scrollable with a
constrained max height (e.g., replace the spread ...templates.map(...) with a
ConstrainedBox or SizedBox that limits height and contains a ListView or
SingleChildScrollView of the ListTiles) so the sheet becomes scrollable when
templates are long and avoids overflow on small screens.
In `@lib/l10n/app_de.arb`:
- Line 157: The German translation for agentTemplateFeedbackChangesLabel is a
semantic drift: replace "Gewünschte Änderungen" with a more accurate German
equivalent of "Specific changes" such as "Spezifische Änderungen" or "Konkrete
Änderungen" in lib/l10n/app_de.arb by updating the value for the key
agentTemplateFeedbackChangesLabel to one of those options so the prompt asks for
targeted/specific feedback rather than desired changes.
In `@lib/l10n/app_fr.arb`:
- Around line 134-217: The French label for agentTemplateAssignedLabel is
ambiguous with AI model labels; update the value for the
"agentTemplateAssignedLabel" key in app_fr.arb to a disambiguated string such as
"Modèle d'agent" (or similar clear phrase) so users can tell it refers to the
agent template (leave "agentTemplateModelLabel" as "ID du modèle"). Locate and
edit the "agentTemplateAssignedLabel" entry in the diff shown and ensure the new
text matches French grammar/apostrophe usage.
In `@lib/l10n/app_localizations_en.dart`:
- Line 418: The label agentTemplateRollbackAction uses the noun form "Rollback"
while agentTemplateRollbackConfirm uses the correct verb phrase; update the
English ARB source (app_en.arb) to change the value for
agentTemplateRollbackAction to "Roll back to This Version" so it matches the
verb form used by agentTemplateRollbackConfirm, then regenerate localization
files so lib/l10n/app_localizations_en.dart reflects the change.
In `@lib/l10n/app_localizations_es.dart`:
- Around line 261-457: The review notes that lib/l10n/app_localizations_es.dart
is a generated file and should not be edited directly; instead, update the
Spanish .arb source(s) (the app_en.arb / app_es.arb or equivalent) for the keys
shown (e.g., agentTemplateCreatedSuccess, agentTemplateDeleteConfirm,
agentTemplateEvolveError, agentTemplateInstanceCount,
agentTemplateOneOnOneTitle, agentTemplateRollbackConfirm, etc.), then run the
localization generation tool to regenerate app_localizations_es.dart so changes
persist and won’t be overwritten; do not make manual edits in
app_localizations_es.dart.
In `@lib/l10n/app_localizations_ro.dart`:
- Around line 357-366: The Romanian pluralization in agentTemplateInstanceCount
is incomplete: update the source .arb entry so the generated
agentTemplateInstanceCount includes a few form for counts 2–19 and changes the
other form to include the required "de" for ≥20; specifically ensure the
generated intl.Intl.pluralLogic call for agentTemplateInstanceCount sets few to
' $count instanțe' (without "de") and other to '$count de instanțe' and keeps
one='1 instanță' and zero='Nicio instanță' so the plural categories follow
Romanian CLDR rules.
In `@lib/l10n/app_localizations.dart`:
- Around line 567-578: Several generated localization getters (e.g.,
agentTemplateActiveInstancesTitle and agentTemplateAllProviders and the other
`@agentTemplate`* keys) lack description metadata in the .arb sources; open the
.arb entries for each agentTemplate key in the app_en.arb, app_de.arb,
app_es.arb, app_fr.arb and app_ro.arb files and add a clear "description" field
for each `@agentTemplate`* message giving translator context (what the string
refers to and where it is used), then regenerate the Dart localization by
running make l10n so the updated descriptions are included in the generated
app_localizations (the symbols to verify are the agentTemplate* entries listed
in the generated lib/l10n/app_localizations.dart).
In `@lib/l10n/app_ro.arb`:
- Around line 138-139: Replace the formal plural imperatives with informal
singular (tu-form) in the new agentTemplate* localization entries: update
agentTemplateCreateTitle, agentTemplateDeleteConfirm,
agentTemplateCreateDescription (or the key containing "Definiți
personalitatea..."), agentTemplateEditTitle (or the key for "Editați șablonul"),
agentTemplateEmptyState (or the key for "Apăsați +..."),
agentTemplateChangePrompt (keys for "Descrieți..." variants),
agentTemplateMissingOne (key for "Creați unul în Setări..."),
agentTemplateRevertLabel and agentTemplateRevertConfirm (keys for
"Reveniți..."), agentTemplateSaveAsNew (key for "Salvați ca versiune nouă"),
agentTemplateSelectPlaceholder (key for "Selectați un șablon"),
agentTemplateManageDescription (key for "Gestionați personalitățile..."), and
the key containing "...distrugeți acest agent..." to use the informal Romanian
forms shown in the review (e.g., "Creează un șablon", "Ștergi acest șablon?",
"Definește...", "Editează...", "Apasă +...", "Descrie...", "Creează unul în
Setări mai întâi.", "Revino...", "Salvează...", "Selectează...",
"Gestionează...", "...și creează unul nou."). Ensure each updated string
preserves interpolation placeholders like {version} unchanged.
---
Nitpick comments:
In `@lib/features/agents/ui/agent_template_detail_page.dart`:
- Around line 459-486: The FutureBuilder is recreating the Future each build
because you call templateService.getAgentsForTemplate(templateId) inline in
build(), causing re-fetches and flicker; fix it by moving that async lookup into
a cached provider or state: create a FutureProvider/family (e.g.,
agentsForTemplateProvider) that calls
agentTemplateServiceProvider.getAgentsForTemplate(templateId) and consume that
provider in the widget (or alternatively store the Future once in State/Hook and
reuse it), then replace the inline FutureBuilder future with the provider's
AsyncValue to avoid recreating the Future on every build.
In `@lib/l10n/app_fr.arb`:
- Line 162: The localization key agentTemplateFeedbackTitle currently maps to
"Retour", which is ambiguous with navigation; update the value to an unambiguous
French heading such as "Commentaires" or "Retour d'expérience" by replacing the
string for agentTemplateFeedbackTitle in lib/l10n/app_fr.arb so the feedback
section title reads clearly as user feedback rather than a back action.
In `@lib/l10n/app_localizations.dart`:
- Around line 699-745: The new localization getters
(agentTemplateFeedbackChangesHint, agentTemplateFeedbackChangesLabel,
agentTemplateFeedbackDidntWorkHint, agentTemplateFeedbackDidntWorkLabel,
agentTemplateFeedbackEnjoyedHint, agentTemplateFeedbackEnjoyedLabel,
agentTemplateFeedbackTitle, agentTemplateInstanceCount) lack translator
descriptions; open the corresponding .arb file, add meaningful "description"
fields for each key (explaining context, usage, placeholders like {count}), save
and then regenerate the Dart localization files so the docstrings in
lib/l10n/app_localizations.dart are updated.
In `@test/features/agents/ui/agent_one_on_one_page_test.dart`:
- Around line 49-180: Tests overuse tester.pumpAndSettle() causing
slow/less-deterministic runs; replace the initial tester.pumpAndSettle() calls
right after await tester.pumpWidget(buildSubject()) in the tests for
AgentOneOnOnePage with a simple await tester.pump() or a bounded await
tester.pump(Duration(milliseconds: 100)) (keep the explicit pumpAndSettle only
where you need to wait for the scroll/drag to finish or real animations, e.g.,
after the tester.drag calls in the evolve button tests); update occurrences
inside tests: the "shows page title with template name", "shows metrics
dashboard with data", "shows feedback form fields", "evolve button is disabled
when all feedback empty", and "evolve button enables after entering feedback" to
use pump/pump(Duration) immediately after buildSubject(), leaving pumpAndSettle
after drag/settle operations intact.
In `@test/features/agents/ui/agent_template_detail_page_test.dart`:
- Around line 431-457: The test uses thenThrow on
mockTemplateService.deleteTemplate which is an async method; replace the
thenThrow call with thenAnswer((_) async => throw Exception('has instances')) to
return a failed Future and more accurately simulate the async failure; update
the expectation code around deleteTemplate (the
mockTemplateService.deleteTemplate setup) so it uses thenAnswer and keeps the
rest of the test (the testWidgets block and assertions on
context.messages.agentTemplateDeleteHasInstances) unchanged.
In `@test/features/agents/ui/agent_template_list_page_test.dart`:
- Around line 39-138: The tests in AgentTemplateListPage (e.g., tests "renders
template cards...", "shows empty state...", "shows page title...", "has FAB for
creating new template") use blanket await tester.pumpAndSettle() after
pumpWidget/buildSubject; replace those with await tester.pump() or a bounded
await tester.pump(Duration(milliseconds: 50)) where there are no animations,
keeping pumpAndSettle only in the "shows version number from active version"
case if awaiting async provider overrides via
makeTestableWidgetNoScroll/agentTemplatesProvider/activeTemplateVersionProvider
requires settling; update calls in those tests (referencing buildSubject,
makeTestableWidgetNoScroll, and the specific test names) to use the lighter pump
to make tests faster and deterministic.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
flatpak/com.matthiasn.lotti.metainfo.xmllib/beamer/locations/settings_location.dartlib/features/agents/README.mdlib/features/agents/ui/agent_detail_page.dartlib/features/agents/ui/agent_model_selector.dartlib/features/agents/ui/agent_one_on_one_page.dartlib/features/agents/ui/agent_template_detail_page.dartlib/features/agents/ui/agent_template_list_page.dartlib/features/settings/ui/pages/settings_page.dartlib/features/tasks/ui/header/task_header_meta_card.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_cs.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_ro.dartlib/l10n/app_ro.arbtest/beamer/locations/settings_location_test.darttest/features/agents/ui/agent_detail_page_test.darttest/features/agents/ui/agent_model_selector_test.darttest/features/agents/ui/agent_one_on_one_page_test.darttest/features/agents/ui/agent_template_detail_page_test.darttest/features/agents/ui/agent_template_list_page_test.darttest/features/tasks/ui/header/task_header_meta_card_test.dart
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/tasks/ui/header/task_header_meta_card.dart (1)
212-224:⚠️ Potential issue | 🟡 MinorAvoid exposing raw exception details in the SnackBar.
Use a generic localized error for users and keep the full error in logs.🛠️ Suggested fix
- content: Text( - context.messages.taskAgentCreateError(e.toString()), - ), + content: Text(context.messages.commonError),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/tasks/ui/header/task_header_meta_card.dart` around lines 212 - 224, The SnackBar is exposing raw exception text (e.toString()); keep the detailed error only in logs and show users a generic localized message instead. In the catch block of _TaskAgentChip (the developer.log call should stay as-is), replace the SnackBar content argument that currently uses context.messages.taskAgentCreateError(e.toString()) with a generic localized message (for example, a context.messages.taskAgentCreateErrorGeneric() or other existing generic/localized error string) so users see a non-sensitive, user-friendly error while the full exception remains in developer.log.
♻️ Duplicate comments (3)
flatpak/com.matthiasn.lotti.metainfo.xml (1)
46-46: CHANGELOG still needs updating for the amended 0.9.868 description.The 0.9.868 release description has been further extended with the LLM-assisted template evolution paragraph, widening the drift from the CHANGELOG.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flatpak/com.matthiasn.lotti.metainfo.xml` at line 46, The 0.9.868 release description in flatpak/com.matthiasn.lotti.metainfo.xml was extended (see the <p> paragraph mentioning "LLM-assisted template evolution") but the CHANGELOG was not updated; update the CHANGELOG entry for version 0.9.868 to include the same amended details (agent cross-device sync, wake subscription restoration, sync maintenance UI, zone-based transaction isolation with nested rollback, and the LLM-assisted template evolution 1-on-1 review/rewrite workflow) so the changelog matches the metainfo description.lib/l10n/app_de.arb (1)
157-157: Previously flagged translation drift resolved —"Konkrete Änderungen"is accurate.The past review flagged
"Gewünschte Änderungen"(desired changes) as a semantic drift from the English"Specific changes". The value is now"Konkrete Änderungen", which is a precise German equivalent. ✓🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_de.arb` at line 157, The translation for key agentTemplateFeedbackChangesLabel is now correct ("Konkrete Änderungen") and requires no change; keep the current value unchanged in lib/l10n/app_de.arb and mark the PR as approved for this key.lib/l10n/app_localizations.dart (1)
567-925: Add translator descriptions for new agentTemplate keys in ARB and regenerate.The new getters still show “No description provided,” which means the ARB
@metadataentries lack descriptions. Add description fields in the.arbsources (then regenerate), rather than editing this file directly.Based on learnings: Applies to lib/l10n/app_localizations_*.dart : Never edit the generated
lib/l10n/app_localizations_*.dartfiles directly; always edit the.arbsource files and regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_localizations.dart` around lines 567 - 925, The new localization getters (e.g., agentTemplateActiveInstancesTitle, agentTemplateAllProviders, agentTemplateAssignedLabel, agentTemplateCreatedSuccess, agentTemplateCreateTitle, agentTemplateDeleteConfirm, agentTemplateDeleteHasInstances, agentTemplateDirectivesHint, agentTemplateDirectivesLabel, agentTemplateDisplayNameLabel, agentTemplateEditTitle, agentTemplateEmptyList, agentTemplateEvolveAction, agentTemplateEvolveApprove, agentTemplateEvolveButton, agentTemplateEvolveCurrentLabel, agentTemplateEvolveError, agentTemplateEvolvePreviewTitle, agentTemplateEvolveProposedLabel, agentTemplateEvolveReject, agentTemplateEvolveSuccess, agentTemplateEvolvingProgress, agentTemplateFeedbackChangesHint, agentTemplateFeedbackChangesLabel, agentTemplateFeedbackDidntWorkHint, agentTemplateFeedbackDidntWorkLabel, agentTemplateFeedbackEnjoyedHint, agentTemplateFeedbackEnjoyedLabel, agentTemplateFeedbackTitle, agentTemplateInstanceCount, agentTemplateKindTaskAgent, agentTemplateMetricsActiveInstances, agentTemplateMetricsAvgDuration, agentTemplateMetricsFailureCount, agentTemplateMetricsFirstWake, agentTemplateMetricsLastWake, agentTemplateMetricsSuccessRate, agentTemplateMetricsTitle, agentTemplateMetricsTotalWakes, agentTemplateModelLabel, agentTemplateModelRequirements, agentTemplateNoMetrics, agentTemplateNoneAssigned, agentTemplateNoSuitableModels, agentTemplateNoTemplates, agentTemplateNotFound, agentTemplateNoVersions, agentTemplateOneOnOneTitle, agentTemplateRollbackAction, agentTemplateRollbackConfirm, agentTemplateSaveNewVersion, agentTemplateSelectTitle, agentTemplateSettingsSubtitle, agentTemplateStatusActive, agentTemplateStatusArchived, agentTemplatesTitle, agentTemplateSwitchHint, agentTemplateVersionHistoryTitle, agentTemplateVersionLabel, agentTemplateVersionSaved) are missing translator descriptions in the ARB metadata; do not edit the generated lib/l10n/app_localizations*.dart files—add appropriate "@<key>@description" entries to the .arb source(s) for each key (matching each getter/function name), then re-run the localization generator to regenerate the Dart files so the "No description provided" comments are replaced with the ARB descriptions.
🧹 Nitpick comments (1)
test/features/agents/ui/agent_template_detail_page_test.dart (1)
346-349: Prefer localized strings for status labels in tests.Hardcoding
"Active"/"Archived"makes the test brittle if localization changes. Usecontext.messages.agentTemplateStatusActive/Archivedinstead.♻️ Suggested update
- expect(find.text('Active'), findsOneWidget); - expect(find.text('Archived'), findsOneWidget); + expect( + find.text(context.messages.agentTemplateStatusActive), + findsOneWidget, + ); + expect( + find.text(context.messages.agentTemplateStatusArchived), + findsOneWidget, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/agents/ui/agent_template_detail_page_test.dart` around lines 346 - 349, Replace hardcoded "Active"/"Archived" assertions with the localized message values: use context.messages.agentTemplateStatusActive and context.messages.agentTemplateStatusArchived in the expect calls (i.e. expect(find.text(context.messages.agentTemplateStatusActive), findsOneWidget) and similarly for Archived). Ensure the test has access to a BuildContext (e.g. obtain context from the pumped widget tree or wrap the tested widget in a Builder/MaterialApp so you can call context.messages) and update the two expect lines in agent_template_detail_page_test.dart that currently call expect(find.text('Active'), ...) and expect(find.text('Archived'), ...).
🤖 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/features/agents/ui/agent_one_on_one_page.dart`:
- Around line 408-412: Replace the hardcoded "s" suffix shown when rendering
averageDuration in the _MetricCard with a localized string key and use the l10n
API: fetch the localized seconds label (e.g.,
context.messages.agentTemplateMetricsAvgDurationSeconds) and concatenate/format
it with metrics.averageDuration!.inSeconds; update the ARB files to add the
agentTemplateMetricsAvgDurationSeconds entry and regenerate localization so the
_MetricCard rendering (where metrics.averageDuration is used) shows a localized
unit instead of the hardcoded "s".
In `@lib/features/agents/ui/agent_template_detail_page.dart`:
- Around line 68-88: The code treats a provider error as "not found"; update the
UI to handle provider errors first by checking templateAsync.hasError (or
templateAsync.when/error) and activeVersionAsync.hasError and returning an error
Scaffold instead of the not-found state; locate the async values from
agentTemplateProvider and activeTemplateVersionProvider (templateAsync,
activeVersionAsync) and add an error branch that displays a useful message
(e.g., using the error.toString() or context.messages) before the existing
template == null not-found branch so real failures aren't masked.
In `@lib/l10n/app_localizations_cs.dart`:
- Around line 258-464: The changes are in a generated localization file (symbols
like agentTemplateActiveInstancesTitle, agentTemplateInstanceCount,
agentTemplateOneOnOneTitle, agentTemplateRollbackConfirm, etc.); revert any
direct edits to lib/l10n/app_localizations_cs.dart, update the corresponding
.arb source entries (e.g., add/update the keys for the affected strings and
plural forms), then re-run the localization generation tool to regenerate
app_localizations_cs.dart so the updated/generated getters and methods match the
.arb translations.
In `@test/features/agents/ui/agent_template_detail_page_test.dart`:
- Around line 249-263: The test 'shows edit title' currently only asserts the
title; extend it to assert a meaningful UI element to validate functionality by
locating and asserting presence of a save/new-version action or an editable form
field on AgentTemplateDetailPage. After building the widget with
buildEditSubject(templateId: templateId) and pumping, add an assertion that
verifies either a button with the localized label (e.g.,
context.messages.saveButtonText or context.messages.newVersionButtonText) is
found (find.text(...), findsOneWidget) or that an editable TextFormField for the
template name exists (find.byType(TextFormField) or
find.widgetWithText(TextFormField, expectedPlaceholder), findsOneWidget). Ensure
you reference the mockTemplateService usage
(mockTemplateService.getAgentsForTemplate) remains stubbed so the added
assertion focuses on the actionable UI element.
---
Outside diff comments:
In `@lib/features/tasks/ui/header/task_header_meta_card.dart`:
- Around line 212-224: The SnackBar is exposing raw exception text
(e.toString()); keep the detailed error only in logs and show users a generic
localized message instead. In the catch block of _TaskAgentChip (the
developer.log call should stay as-is), replace the SnackBar content argument
that currently uses context.messages.taskAgentCreateError(e.toString()) with a
generic localized message (for example, a
context.messages.taskAgentCreateErrorGeneric() or other existing
generic/localized error string) so users see a non-sensitive, user-friendly
error while the full exception remains in developer.log.
---
Duplicate comments:
In `@flatpak/com.matthiasn.lotti.metainfo.xml`:
- Line 46: The 0.9.868 release description in
flatpak/com.matthiasn.lotti.metainfo.xml was extended (see the <p> paragraph
mentioning "LLM-assisted template evolution") but the CHANGELOG was not updated;
update the CHANGELOG entry for version 0.9.868 to include the same amended
details (agent cross-device sync, wake subscription restoration, sync
maintenance UI, zone-based transaction isolation with nested rollback, and the
LLM-assisted template evolution 1-on-1 review/rewrite workflow) so the changelog
matches the metainfo description.
In `@lib/l10n/app_de.arb`:
- Line 157: The translation for key agentTemplateFeedbackChangesLabel is now
correct ("Konkrete Änderungen") and requires no change; keep the current value
unchanged in lib/l10n/app_de.arb and mark the PR as approved for this key.
In `@lib/l10n/app_localizations.dart`:
- Around line 567-925: The new localization getters (e.g.,
agentTemplateActiveInstancesTitle, agentTemplateAllProviders,
agentTemplateAssignedLabel, agentTemplateCreatedSuccess,
agentTemplateCreateTitle, agentTemplateDeleteConfirm,
agentTemplateDeleteHasInstances, agentTemplateDirectivesHint,
agentTemplateDirectivesLabel, agentTemplateDisplayNameLabel,
agentTemplateEditTitle, agentTemplateEmptyList, agentTemplateEvolveAction,
agentTemplateEvolveApprove, agentTemplateEvolveButton,
agentTemplateEvolveCurrentLabel, agentTemplateEvolveError,
agentTemplateEvolvePreviewTitle, agentTemplateEvolveProposedLabel,
agentTemplateEvolveReject, agentTemplateEvolveSuccess,
agentTemplateEvolvingProgress, agentTemplateFeedbackChangesHint,
agentTemplateFeedbackChangesLabel, agentTemplateFeedbackDidntWorkHint,
agentTemplateFeedbackDidntWorkLabel, agentTemplateFeedbackEnjoyedHint,
agentTemplateFeedbackEnjoyedLabel, agentTemplateFeedbackTitle,
agentTemplateInstanceCount, agentTemplateKindTaskAgent,
agentTemplateMetricsActiveInstances, agentTemplateMetricsAvgDuration,
agentTemplateMetricsFailureCount, agentTemplateMetricsFirstWake,
agentTemplateMetricsLastWake, agentTemplateMetricsSuccessRate,
agentTemplateMetricsTitle, agentTemplateMetricsTotalWakes,
agentTemplateModelLabel, agentTemplateModelRequirements, agentTemplateNoMetrics,
agentTemplateNoneAssigned, agentTemplateNoSuitableModels,
agentTemplateNoTemplates, agentTemplateNotFound, agentTemplateNoVersions,
agentTemplateOneOnOneTitle, agentTemplateRollbackAction,
agentTemplateRollbackConfirm, agentTemplateSaveNewVersion,
agentTemplateSelectTitle, agentTemplateSettingsSubtitle,
agentTemplateStatusActive, agentTemplateStatusArchived, agentTemplatesTitle,
agentTemplateSwitchHint, agentTemplateVersionHistoryTitle,
agentTemplateVersionLabel, agentTemplateVersionSaved) are missing translator
descriptions in the ARB metadata; do not edit the generated
lib/l10n/app_localizations*.dart files—add appropriate "@<key>@description"
entries to the .arb source(s) for each key (matching each getter/function name),
then re-run the localization generator to regenerate the Dart files so the "No
description provided" comments are replaced with the ARB descriptions.
---
Nitpick comments:
In `@test/features/agents/ui/agent_template_detail_page_test.dart`:
- Around line 346-349: Replace hardcoded "Active"/"Archived" assertions with the
localized message values: use context.messages.agentTemplateStatusActive and
context.messages.agentTemplateStatusArchived in the expect calls (i.e.
expect(find.text(context.messages.agentTemplateStatusActive), findsOneWidget)
and similarly for Archived). Ensure the test has access to a BuildContext (e.g.
obtain context from the pumped widget tree or wrap the tested widget in a
Builder/MaterialApp so you can call context.messages) and update the two expect
lines in agent_template_detail_page_test.dart that currently call
expect(find.text('Active'), ...) and expect(find.text('Archived'), ...).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
CHANGELOG.mdflatpak/com.matthiasn.lotti.metainfo.xmllib/features/agents/service/agent_template_service.dartlib/features/agents/ui/agent_detail_page.dartlib/features/agents/ui/agent_model_selector.dartlib/features/agents/ui/agent_one_on_one_page.dartlib/features/agents/ui/agent_template_detail_page.dartlib/features/agents/ui/agent_template_list_page.dartlib/features/tasks/ui/header/task_header_meta_card.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_cs.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_ro.dartlib/l10n/app_ro.arbpubspec.yamltest/features/agents/service/agent_template_service_test.darttest/features/agents/state/agent_providers_test.darttest/features/agents/ui/agent_detail_page_test.darttest/features/agents/ui/agent_template_detail_page_test.dart
💤 Files with no reviewable changes (1)
- test/features/agents/state/agent_providers_test.dart
✅ Files skipped from review due to trivial changes (1)
- pubspec.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- test/features/agents/ui/agent_detail_page_test.dart
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/features/agents/ui/agent_one_on_one_page.dart (1)
407-411:⚠️ Potential issue | 🟡 MinorLocalize the seconds suffix in average duration.
Thevaluestring hardcodes"s", which is user-visible and should go through l10n.♻️ Suggested fix
if (metrics.averageDuration != null) _MetricCard( label: context.messages.agentTemplateMetricsAvgDuration, - value: '${metrics.averageDuration!.inSeconds}s', + value: context.messages.agentTemplateMetricsAvgDurationSeconds( + metrics.averageDuration!.inSeconds, + ), ),(Please add
agentTemplateMetricsAvgDurationSecondsto the ARB files and regenerate l10n.)As per coding guidelines, "All user-visible label texts MUST be localized using arb files in
lib/l10n/; never hardcode user-visible strings".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/agents/ui/agent_one_on_one_page.dart` around lines 407 - 411, Replace the hardcoded "s" suffix in the average duration value with a localized string: use the l10n message (via context.messages) for the seconds suffix and format the value by combining metrics.averageDuration!.inSeconds with that localized suffix when building the _MetricCard value; add the new key agentTemplateMetricsAvgDurationSeconds to the ARB files and regenerate l10n so context.messages.agentTemplateMetricsAvgDurationSeconds is available, and update the _MetricCard usage that references metrics.averageDuration to use the localized suffix instead of the literal "s".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@lib/features/agents/ui/agent_one_on_one_page.dart`:
- Around line 407-411: Replace the hardcoded "s" suffix in the average duration
value with a localized string: use the l10n message (via context.messages) for
the seconds suffix and format the value by combining
metrics.averageDuration!.inSeconds with that localized suffix when building the
_MetricCard value; add the new key agentTemplateMetricsAvgDurationSeconds to the
ARB files and regenerate l10n so
context.messages.agentTemplateMetricsAvgDurationSeconds is available, and update
the _MetricCard usage that references metrics.averageDuration to use the
localized suffix instead of the literal "s".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/features/agents/ui/agent_one_on_one_page.darttest/features/agents/ui/agent_detail_page_test.darttest/features/agents/ui/agent_model_selector_test.darttest/features/agents/ui/agent_one_on_one_page_test.darttest/features/agents/ui/agent_template_detail_page_test.darttest/features/agents/ui/agent_template_list_page_test.darttest/helpers/fallbacks.darttest/mocks/mocks.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/features/agents/ui/agent_template_detail_page_test.dart
- test/features/agents/ui/agent_one_on_one_page_test.dart
matthiasn
force-pushed
the
feat/agent_templates_rebased
branch
from
6ae389b to
31401bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
lib/l10n/app_localizations_es.dart (1)
261-474: Generated localization file — confirm changes come from.arb+ regen.Please ensure these updates were made in
lib/l10n/app_es.arb(not here) and that localization files were regenerated (e.g.,make l10n), so changes won’t be overwritten.
As per coding guidelines: "lib/l10n/app_localizations_*.dart: Never edit the generatedlib/l10n/app_localizations_*.dartfiles directly; always edit the.arbsource files and regenerate."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_localizations_es.dart` around lines 261 - 474, These strings in app_localizations_es.dart (e.g., agentTemplateActiveInstancesTitle, agentTemplateEvolveSuccess, agentTemplateInstanceCount, agentTemplateOneOnOneTitle, agentTemplateRollbackConfirm, etc.) are generated and must not be edited directly; instead update the corresponding keys/translations in lib/l10n/app_es.arb (matching the key names used in this file) and then regenerate the localization Dart files (e.g., run make l10n or your project's flutter gen-l10n command) so the changes are applied and not overwritten.lib/l10n/app_localizations_cs.dart (1)
258-468: Generated localization file edited directly.
Please update the.arbsources and regenerate instead of editing this Dart file.
As per coding guidelines: "lib/l10n/app_localizations_*.dart: Never edit the generatedlib/l10n/app_localizations_*.dartfiles directly; always edit the.arbsource files and regenerate".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_localizations_cs.dart` around lines 258 - 468, This Dart localization file was edited directly; undo the manual edits to restore the generated file and instead update the source .arb entries for the affected keys (examples: agentTemplateActiveInstancesTitle, agentTemplateInstanceCount, agentTemplateEvolveError, agentTemplateOneOnOneTitle, agentTemplateVersionLabel, etc.), then regenerate the localization artifacts (run your project's localization generation) so the changes propagate into the generated getters and plural methods correctly; do not commit direct edits to the generated lib/l10n/* Dart outputs.lib/l10n/app_localizations.dart (1)
741-925:⚠️ Potential issue | 🟡 MinorAdd description metadata for the remaining agentTemplate strings.
The following new entries still show “No description provided” (Line 741
agentTemplateInstanceCount, Line 855agentTemplateOneOnOneTitle, Line 867agentTemplateRollbackConfirm, Line 921agentTemplateVersionLabel). Please adddescriptionfields for their@agentTemplate*metadata in the ARB sources (app_en.arb and the other locale ARBs) and regenerate the localization output.Based on learnings: Never edit the generated
lib/l10n/app_localizations_*.dartfiles directly; always edit the.arbsource files and regenerate; Runmake l10nafter adding labels to generate the Dart files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/l10n/app_localizations.dart` around lines 741 - 925, The generated localization file is missing description metadata for keys agentTemplateInstanceCount, agentTemplateOneOnOneTitle, agentTemplateRollbackConfirm, and agentTemplateVersionLabel; open the ARB source(s) (e.g., app_en.arb and other locale ARBs), add a "description" field for each corresponding `@agentTemplate`... entry with a concise human-readable explanation, keep the existing "message" values, save the ARBs, then run the localization generator (make l10n) to regenerate lib/l10n/app_localizations_*.dart; do not edit the generated Dart files directly.
🧹 Nitpick comments (4)
test/features/agents/ui/agent_detail_page_test.dart (1)
545-556: Preferpump()overpumpAndSettle()here.
pumpAndSettle()looks unnecessary for this error state and conflicts with the test guideline preference.♻️ Suggested tweak
- await tester.pumpAndSettle(); + await tester.pump();As per coding guidelines: “Prefer tester.pump(duration) over tester.pumpAndSettle().”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/agents/ui/agent_detail_page_test.dart` around lines 545 - 556, The test uses tester.pumpAndSettle() after building the widget which is unnecessary and conflicts with the guideline; replace the await tester.pumpAndSettle() call in the testWidgets block (the test named 'shows error text when template loading fails' that calls buildSubject with templateOverride) with a simple await tester.pump() (or await tester.pump(const Duration(...)) if a short animation tick is needed) so the test follows the "prefer pump over pumpAndSettle" guideline.test/features/agents/ui/agent_template_detail_page_test.dart (1)
358-361: Minor: prefercontext.messages.*over hardcoded strings for consistency.Lines 359–360 hardcode
'Active'and'Archived', while every other test in this file uses localized lookups likecontext.messages.agentTemplateStatusActive. This works because tests run with English locale, but it's inconsistent and fragile if the English label ever changes.♻️ Suggested fix
- expect(find.text('Active'), findsOneWidget); - expect(find.text('Archived'), findsOneWidget); + expect( + find.text(context.messages.agentTemplateStatusActive), + findsOneWidget, + ); + expect( + find.text(context.messages.agentTemplateStatusArchived), + findsOneWidget, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/features/agents/ui/agent_template_detail_page_test.dart` around lines 358 - 361, Replace the two hardcoded expectations for 'Active' and 'Archived' with the localized message lookups to match the rest of the file: use context.messages.agentTemplateStatusActive and context.messages.agentTemplateStatusArchived in the expect calls that currently reference find.text('Active') and find.text('Archived') so the test uses localized labels instead of hardcoded strings.lib/features/agents/ui/agent_one_on_one_page.dart (1)
59-101: Consider handling template loading/error states at the page level.When
agentTemplateProvidererrors or is still loading, the page renders an empty title and still shows the feedback form and evolve button. Tapping "Evolve" with a failed template would just show a snackbar error — functional but a subpar UX. Consider showing a loading/error indicator similar to whatAgentTemplateDetailPagedoes (lines 76–103 of that file).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/agents/ui/agent_one_on_one_page.dart` around lines 59 - 101, The page currently renders UI even when agentTemplateProvider is loading or errored (templateAsync/template) so users can interact with feedback/evolve controls prematurely; update the build method to check templateAsync's state and, similar to AgentTemplateDetailPage, render a loading indicator while templateAsync is loading and an error UI when it has an error, only showing _MetricsDashboard, _buildFeedbackSection, _buildEvolutionControls and the proposal preview when template != null; use the existing symbols agentTemplateProvider, templateAsync, template, _buildFeedbackSection, _buildEvolutionControls and _proposal to gate rendering and surface the provider error message in the error UI.lib/features/agents/ui/agent_template_detail_page.dart (1)
493-529:FutureBuildercreates a new future on every rebuild — consider a Riverpod provider.
templateService.getAgentsForTemplate(templateId)is called inline inbuild, so each rebuild triggers a new network request. While the empty-list fallback (snapshot.data ?? []) masks the loading flicker, the data is still re-fetched unnecessarily whenever upstream providers are invalidated.A dedicated Riverpod
FutureProvider.familywould avoid this and align with how the rest of the page manages async state.♻️ Example provider approach
// In agent_providers.dart: final agentsForTemplateProvider = FutureProvider.family<List<AgentIdentityEntity>, String>( (ref, templateId) { final service = ref.watch(agentTemplateServiceProvider); return service.getAgentsForTemplate(templateId); }, );Then in the widget:
-class _ActiveInstancesSection extends ConsumerWidget { +class _ActiveInstancesSection extends ConsumerWidget { const _ActiveInstancesSection({required this.templateId}); final String templateId; `@override` Widget build(BuildContext context, WidgetRef ref) { - final templateService = ref.watch(agentTemplateServiceProvider); - - return FutureBuilder<List<AgentIdentityEntity>>( - future: templateService.getAgentsForTemplate(templateId), - builder: (context, snapshot) { - final agents = snapshot.data ?? []; - if (agents.isEmpty) return const SizedBox.shrink(); + final agentsAsync = ref.watch(agentsForTemplateProvider(templateId)); + + return agentsAsync.when( + data: (agents) { + if (agents.isEmpty) return const SizedBox.shrink(); + return Column(/* ... */); + }, + loading: () => const SizedBox.shrink(), + error: (_, __) => const SizedBox.shrink(), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/agents/ui/agent_template_detail_page.dart` around lines 493 - 529, Replace the inline FutureBuilder call to templateService.getAgentsForTemplate(templateId) in _ActiveInstancesSection with a Riverpod FutureProvider.family to avoid recreating the future on every rebuild: add a provider (e.g., agentsForTemplateProvider as FutureProvider.family<List<AgentIdentityEntity>, String>) that watches agentTemplateServiceProvider and calls getAgentsForTemplate, then in _ActiveInstancesSection use ref.watch(agentsForTemplateProvider(templateId)) and handle the returned AsyncValue (loading/data/error) instead of building a new Future each build; update the widget to read the provider and render the same UI for the data case (agents list) and appropriate loading/error states.
🤖 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/l10n/app_localizations_de.dart`:
- Around line 261-475: The PR edits generated localization output in
lib/l10n/app_localizations_de.dart (e.g., getters like
agentTemplateActiveInstancesTitle, agentTemplateCreateTitle and methods like
agentTemplateInstanceCount, agentTemplateOneOnOneTitle), which must not be
edited directly; revert these changes in the generated Dart file, add the
new/updated German strings to the corresponding .arb source(s) (e.g., the
app_en.arb/app_de.arb entries for the same keys), and then regenerate the Dart
localization files (run the localization generator, e.g., make l10n) so
app_localizations_de.dart is produced from the .arb sources.
In `@lib/l10n/app_localizations_fr.dart`:
- Line 357: The translation for agentTemplateFeedbackTitle is ambiguous
("Retour"); update the value for agentTemplateFeedbackTitle in
app_localizations_fr.dart to a clearer feedback label such as "Commentaires" or
"Retour d'expérience" so the UI clearly indicates a feedback section rather than
navigation.
In `@test/features/agents/ui/agent_template_list_page_test.dart`:
- Around line 132-139: The test 'has FAB for creating new template' only asserts
that a FloatingActionButton exists; add a second meaningful assertion by either
(A) retrieving the widget via
tester.widget<FloatingActionButton>(find.byType(FloatingActionButton)) and
asserting its onPressed is not null, or (B) simulate user interaction with
tester.tap(find.byType(FloatingActionButton)); await tester.pumpAndSettle(); and
then assert navigation or the expected outcome (e.g., presence of the template
creation screen or a route-specific widget). Update the test that uses
buildSubject and testWidgets to include one of these checks so the FAB is
verified actionable in addition to being present.
In `@test/features/settings/ui/pages/settings_page_test.dart`:
- Around line 160-210: The tests only assert presence/absence of the 'Agent
Templates' text; add a second meaningful assertion to each test that verifies
the tile widget type or interactivity—for example, assert that the text has an
ancestor ListTile when enabled and that same ancestor is absent when disabled by
using find.ancestor(of: find.text('Agent Templates'), matching:
find.byType(ListTile)) (or another concrete widget type used for the tile) in
the testWidgets blocks that construct SettingsPage and
mockJournalDb.watchConfigFlags, so each test asserts both visibility and the
expected widget structure/interactivity.
---
Duplicate comments:
In `@lib/l10n/app_localizations_cs.dart`:
- Around line 258-468: This Dart localization file was edited directly; undo the
manual edits to restore the generated file and instead update the source .arb
entries for the affected keys (examples: agentTemplateActiveInstancesTitle,
agentTemplateInstanceCount, agentTemplateEvolveError,
agentTemplateOneOnOneTitle, agentTemplateVersionLabel, etc.), then regenerate
the localization artifacts (run your project's localization generation) so the
changes propagate into the generated getters and plural methods correctly; do
not commit direct edits to the generated lib/l10n/* Dart outputs.
In `@lib/l10n/app_localizations_es.dart`:
- Around line 261-474: These strings in app_localizations_es.dart (e.g.,
agentTemplateActiveInstancesTitle, agentTemplateEvolveSuccess,
agentTemplateInstanceCount, agentTemplateOneOnOneTitle,
agentTemplateRollbackConfirm, etc.) are generated and must not be edited
directly; instead update the corresponding keys/translations in
lib/l10n/app_es.arb (matching the key names used in this file) and then
regenerate the localization Dart files (e.g., run make l10n or your project's
flutter gen-l10n command) so the changes are applied and not overwritten.
In `@lib/l10n/app_localizations.dart`:
- Around line 741-925: The generated localization file is missing description
metadata for keys agentTemplateInstanceCount, agentTemplateOneOnOneTitle,
agentTemplateRollbackConfirm, and agentTemplateVersionLabel; open the ARB
source(s) (e.g., app_en.arb and other locale ARBs), add a "description" field
for each corresponding `@agentTemplate`... entry with a concise human-readable
explanation, keep the existing "message" values, save the ARBs, then run the
localization generator (make l10n) to regenerate
lib/l10n/app_localizations_*.dart; do not edit the generated Dart files
directly.
---
Nitpick comments:
In `@lib/features/agents/ui/agent_one_on_one_page.dart`:
- Around line 59-101: The page currently renders UI even when
agentTemplateProvider is loading or errored (templateAsync/template) so users
can interact with feedback/evolve controls prematurely; update the build method
to check templateAsync's state and, similar to AgentTemplateDetailPage, render a
loading indicator while templateAsync is loading and an error UI when it has an
error, only showing _MetricsDashboard, _buildFeedbackSection,
_buildEvolutionControls and the proposal preview when template != null; use the
existing symbols agentTemplateProvider, templateAsync, template,
_buildFeedbackSection, _buildEvolutionControls and _proposal to gate rendering
and surface the provider error message in the error UI.
In `@lib/features/agents/ui/agent_template_detail_page.dart`:
- Around line 493-529: Replace the inline FutureBuilder call to
templateService.getAgentsForTemplate(templateId) in _ActiveInstancesSection with
a Riverpod FutureProvider.family to avoid recreating the future on every
rebuild: add a provider (e.g., agentsForTemplateProvider as
FutureProvider.family<List<AgentIdentityEntity>, String>) that watches
agentTemplateServiceProvider and calls getAgentsForTemplate, then in
_ActiveInstancesSection use ref.watch(agentsForTemplateProvider(templateId)) and
handle the returned AsyncValue (loading/data/error) instead of building a new
Future each build; update the widget to read the provider and render the same UI
for the data case (agents list) and appropriate loading/error states.
In `@test/features/agents/ui/agent_detail_page_test.dart`:
- Around line 545-556: The test uses tester.pumpAndSettle() after building the
widget which is unnecessary and conflicts with the guideline; replace the await
tester.pumpAndSettle() call in the testWidgets block (the test named 'shows
error text when template loading fails' that calls buildSubject with
templateOverride) with a simple await tester.pump() (or await tester.pump(const
Duration(...)) if a short animation tick is needed) so the test follows the
"prefer pump over pumpAndSettle" guideline.
In `@test/features/agents/ui/agent_template_detail_page_test.dart`:
- Around line 358-361: Replace the two hardcoded expectations for 'Active' and
'Archived' with the localized message lookups to match the rest of the file: use
context.messages.agentTemplateStatusActive and
context.messages.agentTemplateStatusArchived in the expect calls that currently
reference find.text('Active') and find.text('Archived') so the test uses
localized labels instead of hardcoded strings.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
lib/beamer/locations/settings_location.dartlib/features/agents/ui/agent_one_on_one_page.dartlib/features/agents/ui/agent_template_detail_page.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_cs.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_ro.dartlib/l10n/app_ro.arbtest/features/agents/ui/agent_detail_page_test.darttest/features/agents/ui/agent_model_selector_test.darttest/features/agents/ui/agent_one_on_one_page_test.darttest/features/agents/ui/agent_template_detail_page_test.darttest/features/agents/ui/agent_template_list_page_test.darttest/features/settings/ui/pages/settings_page_test.darttest/helpers/fallbacks.darttest/mocks/mocks.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/features/agents/ui/agent_model_selector_test.dart
- test/mocks/mocks.dart
Owner
Author
|
/gemini review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/features/agents/state/agent_providers_test.dart`:
- Around line 1425-1431: The test that only constructs WakeRunner via
container.read(wakeRunnerProvider) is a meaningless constructor-only smoke test;
either remove it or fold its instantiation into the existing dispose test and
replace the trivial assertion with a meaningful behavior assertion (for example,
after obtaining the WakeRunner from wakeRunnerProvider, assert that it can
acquire and release its lock or that its lifecycle methods change observable
state), ensuring you reference WakeRunner and wakeRunnerProvider and add
assertions that verify lock acquisition/release or disposal side-effects rather
than mere instantiation.
- Around line 1414-1422: Replace the constructor-only assertion with a
behavioral test that exercises wakeQueueProvider/WakeQueue: create a
ProviderContainer, read wakeQueueProvider to get a WakeQueue instance, then call
its enqueue method with two distinct items and assert dequeue returns them in
FIFO order (first enqueued first dequeued), and finally assert the queue reports
empty (isEmpty or length == 0) after both dequeues; if method names differ, use
the WakeQueue methods that perform enqueue, dequeue and check emptiness/size.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive UI layer for agent templates, including creation, editing, versioning, and an LLM-assisted evolution workflow. The implementation is well-structured, with thorough test coverage for new services, providers, and UI components. My review focuses on a couple of minor documentation inconsistencies and a suggestion for navigation consistency.
matthiasn
force-pushed
the
feat/agent_templates_rebased
branch
from
0715572 to
14456ac
Compare
Closed
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 by CodeRabbit
New Features
Bug Fixes
Tests
Documentation