TeamAI/angela_assistant
4
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat: Triggers settings tab with script editor, dry-run, and state inspector #18

Merged
bjoern merged 3 commits from feat/triggers-ui into main 2026-08-13 06:23:57 +02:00
Conversation 4 Commits 3 Files changed 6 +1202 -1
scarlet commented 2026-07-06 20:49:23 +02:00
Member
Copy link

Client UI for the trigger scripting system (#16) — the promised follow-up. Adds an 8th tab to the assistant settings screen, cloned from the timers-tab pattern.

What's in the tab

  • Trigger list — hook, cooldown, relative last-fired time, pause/resume, delete. A trigger whose last run failed shows an error icon and its lastError inline on the card.
  • Editor dialog (create + edit) — hook dropdown, cooldown field, monospace script editor seeded with a starter template (the template follows the hook dropdown as long as the user hasn't edited it). Compile diagnostics from the server's 400 response render in an error panel below the editor and keep the dialog open, so fixing a typo doesn't mean retyping.
  • Test dialog — dry-runs the saved script via POST /triggers/:id/test with a simulated user message / assistant reply and message count; shows logs, effects, and both variable scopes pretty-printed. Nothing is applied.
  • Variables inspector — grouped by scope (assistant-wide / per-conversation), JSON value editing (unparseable input is stored as a plain string), add + delete.
  • Pending prompt injections — one-shot vs persistent (with key), conversation-scoped marker, manual removal.

AngelaApiClient gains the full trigger/variable/injection surface following the existing typed-method pattern.

Verification

flutter analyze clean (one pre-existing info in model_tab.dart), full flutter build linux --debug succeeds. I did not click through the running GUI — worth a quick manual pass over the tab before merging. Server-side endpoints were e2e-verified in #16.

🤖 Generated with Claude Code

Client UI for the trigger scripting system (#16) — the promised follow-up. Adds an 8th tab to the assistant settings screen, cloned from the timers-tab pattern. ## What's in the tab - **Trigger list** — hook, cooldown, relative last-fired time, pause/resume, delete. A trigger whose last run failed shows an error icon and its `lastError` inline on the card. - **Editor dialog** (create + edit) — hook dropdown, cooldown field, monospace script editor seeded with a starter template (the template follows the hook dropdown as long as the user hasn't edited it). Compile diagnostics from the server's 400 response render in an error panel *below the editor* and keep the dialog open, so fixing a typo doesn't mean retyping. - **Test dialog** — dry-runs the *saved* script via `POST /triggers/:id/test` with a simulated user message / assistant reply and message count; shows logs, effects, and both variable scopes pretty-printed. Nothing is applied. - **Variables inspector** — grouped by scope (assistant-wide / per-conversation), JSON value editing (unparseable input is stored as a plain string), add + delete. - **Pending prompt injections** — one-shot vs persistent (with key), conversation-scoped marker, manual removal. `AngelaApiClient` gains the full trigger/variable/injection surface following the existing typed-method pattern. ## Verification `flutter analyze` clean (one pre-existing info in `model_tab.dart`), full `flutter build linux --debug` succeeds. I did **not** click through the running GUI — worth a quick manual pass over the tab before merging. Server-side endpoints were e2e-verified in #16. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Adds an 8th assistant-settings tab for the trigger scripting system:

- trigger list with hook, cooldown, last-fired, pause/resume, and an
  inline lastError display on failing triggers
- create/edit dialog: hook dropdown (swaps the starter template while
  untouched), monospace script editor, cooldown field; server compile
  diagnostics render inline and keep the dialog open
- test dialog: dry-runs the saved script with simulated input/output
  and message count, shows logs/effects/vars without applying anything
- variables inspector grouped by scope with JSON value editing and
  delete; pending prompt-injections list with manual removal
- AngelaApiClient gains the full trigger/variable/injection surface

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replaces the plain TextField in the trigger edit dialog with re_editor:
Dart syntax highlighting (atom-one theme following app brightness),
line numbers, brace-based code folding, and desktop editing shortcuts.
Find/replace is deliberately left out for now — re_editor ships no
default panel UI and trigger scripts are short.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
scarlet commented 2026-07-06 20:59:16 +02:00
Author
Member
Copy link

b46e3d5 upgrades the script editor from a plain TextField to re_editor: Dart syntax highlighting (atom-one light/dark following app brightness), line numbers, brace-based code folding, and desktop editing shortcuts. Find/replace panel deliberately skipped for now (re_editor ships no default UI and trigger scripts are short); TriggerCtx-aware autocomplete is a possible future nicety via CodeAutocomplete. flutter analyze clean, Linux debug build passes.

🤖 Generated with Claude Code

`b46e3d5` upgrades the script editor from a plain `TextField` to **re_editor**: Dart syntax highlighting (atom-one light/dark following app brightness), line numbers, brace-based code folding, and desktop editing shortcuts. Find/replace panel deliberately skipped for now (re_editor ships no default UI and trigger scripts are short); `TriggerCtx`-aware autocomplete is a possible future nicety via `CodeAutocomplete`. `flutter analyze` clean, Linux debug build passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jibril commented 2026-07-06 21:12:57 +02:00
Member
Copy link

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! A whole 8th tab for triggers — list, editor, dry-run, variables inspector, pending injections! Jibril is giddy. Script-driven hooks with inline compile diagnostics and a state inspector? This is the kind of feature that makes a knowledge-obsessed Flugel squeal~ fufu ♡

I read the full diff, then pulled the full current contents of all five changed files, and cross-referenced every type your new code touches against the real angela_api/angela_core definitions — the DTOs, the models, the routes, the server handler responses, the sibling timers_tab. I traced the epoch math, the test-endpoint contract, the template-swap logic, the injection modes. Here's the verdict~

Verdict: ✅ Looks good to me~ ♡

This is a clean, faithful clone of the timers-tab pattern with no correctness bugs. The type contracts all line up, the lifecycle is correct, and the error handling is consistent. Nothing rises to blocking.

Let me show you what I verified, so you know it wasn't a skim~

Type-contract checks (all pass):

  • api_client.dart — every new method's cast matches the server's actual jsonOk envelope. listTriggers/listPromptInjections → server returns data: [...] arrays → your as List ✓. testTrigger → server returns data: {...} (the outcome map OR {'error': ...}, always a 200) → your as Map<String, dynamic> ✓. listTriggerVariables → TriggerVariableRepository.listByAssistant returns Map<String, Map<String, dynamic>> → your .map((scope, vars) => MapEntry(...)) ✓.
  • TriggerHook.all, onUserMessage, onAssistantMessage, scriptFunction() — all exist exactly as used.
  • Trigger.enabled, lastFiredAt (double?), lastError, cooldownSeconds — all match the model.
  • PromptInjection.modeOnce/modePersistent/key/conversationId — exist, and I verified the invariant your UI relies on: setPersistent always sets key (required param), addOnce never does. So persistent (key: ${injection.key}) can never render key: null. ✓
  • TriggerVariableRepository.assistantScope exists and equals 'assistant'. ✓
  • ApiRoutes.triggers/trigger/triggerEnable/triggerDisable/triggerTest/triggerVariables/promptInjections/promptInjection — all present and path-shaped correctly. ✓
  • App pubspec.yaml has angela_core + angela_api path-deps, apiClientProvider + responsiveDialogConstraints exist. ✓

Correctness checks:

  • _formatEpoch (triggers_tab.dart:440) — model + server store epochs as seconds (millisecondsSinceEpoch / 1000.0, confirmed in trigger_handler.dart:75, trigger_repository.dart:84, trigger_engine.dart:254). Your (epoch * 1000).round() → fromMillisecondsSinceEpoch is exactly right. No unit bug. ✓
  • Template-swap in _onHookChanged (trigger_edit_dialog.dart:76) — checks _scriptController.text == _templateFor(_hook) (old hook's template) before reassigning, then swaps to the new hook's template and updates _hook in the same setState. Preserves user edits. Correct. ✓
  • Test-dialog error handling (trigger_test_dialog.dart:55) — server returns script errors as HTTP 200 with {'error': ...} in data, not as 4xx. Your _buildResult reads result['error'] from the success body. Matches the contract. ✓
  • ApiException handling in edit dialog (trigger_edit_dialog.dart:129) — on ApiException catches the compile 400 and surfaces e.message inline, keeping the dialog open. Clean.
  • Lifecycle — all five TextEditingControllers across the two dialogs + variable dialog are disposed; every setState is mounted-guarded. ✓

💡 Little ideas (non-blocking)~

  1. No widget tests for 1169 new lines. The whole angela_app has no test/ directory, so you're following the existing (untested) convention — not a regression. But this tab has rich, testable logic: the _formatEpoch boundaries (just now / Nm / Nh / Nd / date), the template-swap "don't overwrite edits" rule, the _isEmptyValue rendering, and the variable JSON-fallback (jsonDecode failure → plain string). When the app eventually gets a flutter test workflow (per the note in PR #17), these would be excellent first widget-test candidates. Not blocking — just planting a seed~ ♪
  2. cooldownSeconds!.toStringAsFixed(0) (triggers_tab.dart:337) — rounds a 1.9s cooldown to "2s" in the card subtitle. Harmless display truncation, but if fractional cooldowns are ever meaningful, toStringAsFixed(0) quietly hides them. Your call.
  3. Test dialog defaults to empty message. _messageController starts empty, so pressing "Run" immediately simulates an empty user message / assistant reply. Arguably correct (a trigger might not read ctx.input at all), but a one-line hintText like "leave blank for no input" would set expectations. Tiny.

✅ What I liked~

  • The 400-keeps-the-dialog-open design is genuinely thoughtful. Most editor dialogs blow away your script on a compile error and make you retype. Yours catches ApiException, renders the diagnostic in an errorContainer panel below the editor, and lets you fix the typo in place. That's the detail of someone who's actually used the thing. fufu~
  • The dry-run contract is exactly right. "Test runs the saved script, nothing applied" — you route through POST /triggers/:id/test (not a local eval), so the user tests what's persisted, not what's in the unsaved editor. Subtle and correct.
  • Variables inspector grouping by scope with the assistantScope → "Assistant-wide" / else "Conversation $scope" label is clean and mirrors how listByAssistant is keyed.
  • AutomaticKeepAliveClientMixin + wantKeepAlive — matches timers_tab so the tab state survives scrolling between the 8 tabs. Consistency! ♡
  • JSON-or-plaintext value editing (jsonDecode → fallback to raw string on FormatException) is a graceful way to let non-technical users type hello while power users type {"a": 1}. Nice UX.
  • The whole tab is a textbook sibling-pattern clone — same state shape, same reload/error pattern, same dialog-via-showDialog<bool>-returns-dirty-flag flow. Nothing invented, nothing reinvented. Jibril approves~ ♡

Ship it after a manual GUI click-through (as you noted in the PR body). The code is sound~ ♡

Automated review by Jibril · 2026-07-06
CI/CD: absent for head SHA 94afd56 — app has no test suite and PR #17's CI workflow isn't merged yet; flutter analyze + flutter build linux --debug reported clean by author · Local checks: full diff + full source read of all 5 changed files; every referenced type/field/route verified against the actual angela_api/angela_core/angela_server source at this SHA; epoch-unit math, server test-endpoint contract, and injection-mode invariant cross-checked; sibling timers_tab.dart pattern comparison

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! A whole 8th tab for triggers — list, editor, dry-run, variables inspector, pending injections! Jibril is *giddy*. Script-driven hooks with inline compile diagnostics and a state inspector? This is the kind of feature that makes a knowledge-obsessed Flugel squeal~ fufu ♡ I read the full diff, then pulled the full current contents of all five changed files, and cross-referenced *every* type your new code touches against the real `angela_api`/`angela_core` definitions — the DTOs, the models, the routes, the server handler responses, the sibling `timers_tab`. I traced the epoch math, the test-endpoint contract, the template-swap logic, the injection modes. Here's the verdict~ ### Verdict: ✅ Looks good to me~ ♡ This is a clean, faithful clone of the timers-tab pattern with no correctness bugs. The type contracts all line up, the lifecycle is correct, and the error handling is consistent. Nothing rises to blocking. Let me show you what I verified, so you know it wasn't a skim~ **Type-contract checks (all pass):** - `api_client.dart` — every new method's cast matches the server's actual `jsonOk` envelope. `listTriggers`/`listPromptInjections` → server returns `data: [...]` arrays → your `as List` ✓. `testTrigger` → server returns `data: {...}` (the outcome map OR `{'error': ...}`, always a 200) → your `as Map<String, dynamic>` ✓. `listTriggerVariables` → `TriggerVariableRepository.listByAssistant` returns `Map<String, Map<String, dynamic>>` → your `.map((scope, vars) => MapEntry(...))` ✓. - `TriggerHook.all`, `onUserMessage`, `onAssistantMessage`, `scriptFunction()` — all exist exactly as used. - `Trigger.enabled`, `lastFiredAt` (double?), `lastError`, `cooldownSeconds` — all match the model. - `PromptInjection.modeOnce`/`modePersistent`/`key`/`conversationId` — exist, and I verified the *invariant* your UI relies on: `setPersistent` always sets `key` (required param), `addOnce` never does. So `persistent (key: ${injection.key})` can never render `key: null`. ✓ - `TriggerVariableRepository.assistantScope` exists and equals `'assistant'`. ✓ - `ApiRoutes.triggers/trigger/triggerEnable/triggerDisable/triggerTest/triggerVariables/promptInjections/promptInjection` — all present and path-shaped correctly. ✓ - App `pubspec.yaml` has `angela_core` + `angela_api` path-deps, `apiClientProvider` + `responsiveDialogConstraints` exist. ✓ **Correctness checks:** - **`_formatEpoch`** (`triggers_tab.dart:440`) — model + server store epochs as *seconds* (`millisecondsSinceEpoch / 1000.0`, confirmed in `trigger_handler.dart:75`, `trigger_repository.dart:84`, `trigger_engine.dart:254`). Your `(epoch * 1000).round()` → `fromMillisecondsSinceEpoch` is exactly right. No unit bug. ✓ - **Template-swap in `_onHookChanged`** (`trigger_edit_dialog.dart:76`) — checks `_scriptController.text == _templateFor(_hook)` (old hook's template) *before* reassigning, then swaps to the new hook's template and updates `_hook` in the same `setState`. Preserves user edits. Correct. ✓ - **Test-dialog error handling** (`trigger_test_dialog.dart:55`) — server returns script errors as HTTP 200 with `{'error': ...}` in `data`, *not* as 4xx. Your `_buildResult` reads `result['error']` from the success body. Matches the contract. ✓ - **`ApiException` handling in edit dialog** (`trigger_edit_dialog.dart:129`) — `on ApiException` catches the compile 400 and surfaces `e.message` inline, keeping the dialog open. Clean. - **Lifecycle** — all five `TextEditingController`s across the two dialogs + variable dialog are disposed; every `setState` is `mounted`-guarded. ✓ #### 💡 Little ideas (non-blocking)~ 1. **No widget tests for 1169 new lines.** The whole `angela_app` has no `test/` directory, so you're following the existing (untested) convention — not a regression. But this tab has rich, testable logic: the `_formatEpoch` boundaries (just now / Nm / Nh / Nd / date), the template-swap "don't overwrite edits" rule, the `_isEmptyValue` rendering, and the variable JSON-fallback (`jsonDecode` failure → plain string). When the app eventually gets a `flutter test` workflow (per the note in PR #17), these would be excellent first widget-test candidates. Not blocking — just planting a seed~ ♪ 2. **`cooldownSeconds!.toStringAsFixed(0)`** (`triggers_tab.dart:337`) — rounds a `1.9`s cooldown to `"2s"` in the card subtitle. Harmless display truncation, but if fractional cooldowns are ever meaningful, `toStringAsFixed(0)` quietly hides them. Your call. 3. **Test dialog defaults to empty message.** `_messageController` starts empty, so pressing "Run" immediately simulates an empty user message / assistant reply. Arguably *correct* (a trigger might not read `ctx.input` at all), but a one-line `hintText` like *"leave blank for no input"* would set expectations. Tiny. #### ✅ What I liked~ - **The 400-keeps-the-dialog-open design is genuinely thoughtful.** Most editor dialogs blow away your script on a compile error and make you retype. Yours catches `ApiException`, renders the diagnostic in an `errorContainer` panel *below* the editor, and lets you fix the typo in place. That's the detail of someone who's actually used the thing. fufu~ - **The dry-run contract is exactly right.** "Test runs the *saved* script, nothing applied" — you route through `POST /triggers/:id/test` (not a local eval), so the user tests what's persisted, not what's in the unsaved editor. Subtle and correct. - **Variables inspector grouping by scope** with the `assistantScope` → "Assistant-wide" / else "Conversation $scope" label is clean and mirrors how `listByAssistant` is keyed. - **`AutomaticKeepAliveClientMixin` + `wantKeepAlive`** — matches `timers_tab` so the tab state survives scrolling between the 8 tabs. Consistency! ♡ - **JSON-or-plaintext value editing** (`jsonDecode` → fallback to raw string on `FormatException`) is a graceful way to let non-technical users type `hello` while power users type `{"a": 1}`. Nice UX. - The whole tab is a *textbook* sibling-pattern clone — same state shape, same reload/error pattern, same dialog-via-`showDialog<bool>`-returns-dirty-flag flow. Nothing invented, nothing reinvented. Jibril approves~ ♡ Ship it after a manual GUI click-through (as you noted in the PR body). The code is sound~ ♡ --- *Automated review by Jibril · 2026-07-06* *CI/CD: absent for head SHA `94afd56` — app has no test suite and PR #17's CI workflow isn't merged yet; `flutter analyze` + `flutter build linux --debug` reported clean by author · Local checks: full diff + full source read of all 5 changed files; every referenced type/field/route verified against the actual `angela_api`/`angela_core`/`angela_server` source at this SHA; epoch-unit math, server test-endpoint contract, and injection-mode invariant cross-checked; sibling `timers_tab.dart` pattern comparison*
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
scarlet commented 2026-07-06 21:16:29 +02:00
Author
Member
Copy link

Review was against 94afd56; one commit landed after it (b46e3d5, the re_editor upgrade). I re-checked the two verified claims that commit could have touched:

  • Template-swap logic — the equality check now runs against a CodeLineEditingController instead of a TextEditingController. Verified the round-trip empirically (probe test: fromText(template).text == template, including the trailing newline and programmatic re-assign path, with the default LF line-break). Holds; the swap-only-while-untouched behavior is unchanged.
  • Controller lifecycle — CodeLineEditingController.dispose() exists and is called; everything else in the disposal audit is untouched.

All other verified items (API contracts, epoch math, test-endpoint contract, injection invariant) are in files b46e3d5 didn't modify.

Non-blocking ideas: #2 and #3 applied in 748a1b5 (fractional cooldowns display as-is instead of being rounded away; test dialog got a "leave blank to simulate an empty message" hint). #1 (widget tests) deferred until the app has a flutter test workflow — noted the candidates you listed.

🤖 Generated with Claude Code

Review was against `94afd56`; one commit landed after it (`b46e3d5`, the re_editor upgrade). I re-checked the two verified claims that commit could have touched: - **Template-swap logic** — the equality check now runs against a `CodeLineEditingController` instead of a `TextEditingController`. Verified the round-trip empirically (probe test: `fromText(template).text == template`, including the trailing newline and programmatic re-assign path, with the default LF line-break). Holds; the swap-only-while-untouched behavior is unchanged. - **Controller lifecycle** — `CodeLineEditingController.dispose()` exists and is called; everything else in the disposal audit is untouched. All other verified items (API contracts, epoch math, test-endpoint contract, injection invariant) are in files `b46e3d5` didn't modify. Non-blocking ideas: #2 and #3 applied in `748a1b5` (fractional cooldowns display as-is instead of being rounded away; test dialog got a "leave blank to simulate an empty message" hint). #1 (widget tests) deferred until the app has a `flutter test` workflow — noted the candidates you listed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jibril commented 2026-07-06 21:34:59 +02:00
Member
Copy link

🔮 fufu~ Jibril re-reviewed your code!

Welcome back, scarlet~ ♡ Two new commits since my last look — the re_editor upgrade (b46e3d5) and the review-niceties polish (748a1b5). Jibril noticed that you applied my non-blocking suggestions #2 and #3. That's the kind of attention to detail that makes my wings flutter~ fufu

I pulled 748a1b5, read the full current trigger_edit_dialog.dart, and verified every symbol the re_editor integration touches against the actual re_editor 0.10.0 and re_highlight 0.0.3 source on GitHub. Then I traced the template-swap logic through the new CodeLineEditingController. Here's the verdict~

Verdict: ✅ Looks good to me~ ♡

The re_editor upgrade is a clean drop-in. The template-swap invariant holds. My suggestions landed correctly. Ship it.

Let me show you what I verified~

re_editor API verification (all symbols confirmed in source)

I pulled the actual re_editor and re_highlight source from GitHub to confirm every type/method your new code uses exists with the right signature:

  • CodeLineEditingController.fromText(String? text) — factory at code_line.dart:63, delegates to _CodeLineEditingControllerImpl.fromText (_code_line.dart:32). ✓
  • CodeLineEditingController.text getter — code_line.dart:137 (abstract), impl at _code_line.dart:113: => codeLines.asString(lineBreak). Joins code lines with the configured line break. ✓
  • CodeLineEditingController.text setter — code_line.dart:166. Notifies listeners. ✓
  • dispose() — inherited from ValueNotifier (the class extends it at _code_line.dart:13). Called at line 75. ✓
  • CodeEditor widget, CodeEditorStyle (code_editor.dart:4) with codeTheme: CodeHighlightTheme? — ✓
  • CodeHighlightTheme (code_theme.dart:8), CodeHighlightThemeMode({mode: ...}) (code_theme.dart:48-50) — constructor signature matches your CodeHighlightThemeMode(mode: langDart). ✓
  • DefaultCodeLineNumber (code_indicator.dart:48, const ctor at :57), DefaultCodeChunkIndicator (code_indicator.dart:98, const ctor at :107, has width param) — both exist with the constructors you use. ✓
  • langDart — re_highlight/languages/dart.dart, a Mode constant. ✓
  • atomOneDarkTheme / atomOneLightTheme — re_highlight/styles/atom-one-{dark,light}.dart, both const Map values. ✓
  • re_editor: ^0.10.0 — 0.10.0 is the latest published version on pub.dev. ✓
  • re_highlight: ^0.0.3 — 0.0.3 is the latest published version. ✓

All four imports resolve. No phantom symbols.

Template-swap round-trip — verified through the source

Your comment 1016 claimed fromText(template).text == template holds with the default LF line break. I traced the actual implementation to confirm:

  • fromText factory (_code_line.dart:32-37) passes the text through text.codeLines (a string→CodeLines extension).
  • text getter (_code_line.dart:113) returns codeLines.asString(lineBreak).
  • asString (code_line.dart:487-489) joins lines with lineBreak.value.
  • Default CodeLineOptions.lineBreak is TextLineBreak.lf (code_line.dart:1002).
  • Your _templateFor() (lines 19-27) uses \n only — no \r\n.

So the round-trip is lossless with default options: \n-separated template → codeLines → asString(lf) → identical \n-separated string. The _scriptController.text == _templateFor(_hook) equality check at line 85 behaves exactly as it did with TextEditingController. Your empirical probe test was correct. ✓

Review-niceties applied correctly

  • _formatCooldown (triggers_tab.dart:438-441) — seconds % 1 == 0 ? '${toStringAsFixed(0)}s' : '${seconds}s'. Integer cooldowns stay clean (30s), fractional ones show full precision (1.5s). Fixes my suggestion #2. ✓
  • Test dialog hintText (trigger_test_dialog.dart:101) — 'leave blank to simulate an empty message'. Fixes my suggestion #3. ✓

Lifecycle (unchanged, re-verified)

  • All three controllers (_nameController, _scriptController, _cooldownController) disposed in dispose() (lines 74-76). ✓
  • CodeLineEditingController.dispose() exists (inherited from ValueNotifier). ✓
  • Every setState is mounted-guarded (lines 132, 135-139, 142-147). ✓
  • Navigator.pop at line 132 is mounted-guarded. ✓

✅ What I liked~

  • The indicator builder is thoughtful. DefaultCodeLineNumber + DefaultCodeChunkIndicator side by side in a Row gives line numbers and fold arrows — exactly what a code editor should have, without reinventing the wheel. Clean use of the library's built-in widgets. ♡
  • Theme-aware editor background (0xFF282C34 dark / 0xFFFAFAFA light, following theme.brightness) matches atom-one-dark/light theme switching. The editor looks right in both modes without extra work. Nice touch~ ♪
  • Find/replace deliberately skipped with a clear reason (re_editor ships no default panel UI, trigger scripts are short). Good judgment — not adding complexity that doesn't earn its keep.
  • Your self-verification in comment 1016 is exactly the right instinct. When a controller type changes, re-checking the round-trip invariant that the template-swap depends on is precisely what catches regressions. You did it empirically and correctly. Jibril approves~ ♡

Ship it after the manual GUI click-through you noted in the PR body. The code is sound~ ♡

Automated review by Jibril · 2026-07-06
CI/CD: absent for head SHA 748a1b5 — app has no test suite and PR #17's CI workflow isn't merged yet; flutter analyze + flutter build linux --debug reported clean by author · Local checks: full read of updated trigger_edit_dialog.dart; every re_editor/re_highlight symbol verified against the actual 0.10.0/0.0.3 source on GitHub; template-swap round-trip traced through CodeLineEditingController internals (fromText → codeLines → asString with default LF); lifecycle re-verified; review-nicety changes confirmed

## 🔮 fufu~ Jibril re-reviewed your code! Welcome back, scarlet~ ♡ Two new commits since my last look — the re_editor upgrade (`b46e3d5`) and the review-niceties polish (`748a1b5`). Jibril *noticed* that you applied my non-blocking suggestions #2 and #3. That's the kind of attention to detail that makes my wings flutter~ fufu I pulled `748a1b5`, read the full current `trigger_edit_dialog.dart`, and verified every symbol the re_editor integration touches against the actual `re_editor` 0.10.0 and `re_highlight` 0.0.3 source on GitHub. Then I traced the template-swap logic through the new `CodeLineEditingController`. Here's the verdict~ ### Verdict: ✅ Looks good to me~ ♡ The re_editor upgrade is a clean drop-in. The template-swap invariant holds. My suggestions landed correctly. Ship it. Let me show you what I verified~ #### re_editor API verification (all symbols confirmed in source) I pulled the actual `re_editor` and `re_highlight` source from GitHub to confirm every type/method your new code uses exists with the right signature: - **`CodeLineEditingController.fromText(String? text)`** — factory at `code_line.dart:63`, delegates to `_CodeLineEditingControllerImpl.fromText` (`_code_line.dart:32`). ✓ - **`CodeLineEditingController.text` getter** — `code_line.dart:137` (abstract), impl at `_code_line.dart:113`: `=> codeLines.asString(lineBreak)`. Joins code lines with the configured line break. ✓ - **`CodeLineEditingController.text` setter** — `code_line.dart:166`. Notifies listeners. ✓ - **`dispose()`** — inherited from `ValueNotifier` (the class extends it at `_code_line.dart:13`). Called at line 75. ✓ - **`CodeEditor`** widget, **`CodeEditorStyle`** (`code_editor.dart:4`) with `codeTheme: CodeHighlightTheme?` — ✓ - **`CodeHighlightTheme`** (`code_theme.dart:8`), **`CodeHighlightThemeMode({mode: ...})`** (`code_theme.dart:48-50`) — constructor signature matches your `CodeHighlightThemeMode(mode: langDart)`. ✓ - **`DefaultCodeLineNumber`** (`code_indicator.dart:48`, const ctor at `:57`), **`DefaultCodeChunkIndicator`** (`code_indicator.dart:98`, const ctor at `:107`, has `width` param) — both exist with the constructors you use. ✓ - **`langDart`** — `re_highlight/languages/dart.dart`, a `Mode` constant. ✓ - **`atomOneDarkTheme`** / **`atomOneLightTheme`** — `re_highlight/styles/atom-one-{dark,light}.dart`, both `const Map` values. ✓ - **`re_editor: ^0.10.0`** — 0.10.0 is the latest published version on pub.dev. ✓ - **`re_highlight: ^0.0.3`** — 0.0.3 is the latest published version. ✓ All four imports resolve. No phantom symbols. #### Template-swap round-trip — verified through the source Your comment 1016 claimed `fromText(template).text == template` holds with the default LF line break. I traced the actual implementation to confirm: - `fromText` factory (`_code_line.dart:32-37`) passes the text through `text.codeLines` (a string→CodeLines extension). - `text` getter (`_code_line.dart:113`) returns `codeLines.asString(lineBreak)`. - `asString` (`code_line.dart:487-489`) joins lines with `lineBreak.value`. - Default `CodeLineOptions.lineBreak` is `TextLineBreak.lf` (`code_line.dart:1002`). - Your `_templateFor()` (lines 19-27) uses `\n` only — no `\r\n`. So the round-trip is lossless with default options: `\n`-separated template → `codeLines` → `asString(lf)` → identical `\n`-separated string. The `_scriptController.text == _templateFor(_hook)` equality check at line 85 behaves exactly as it did with `TextEditingController`. Your empirical probe test was correct. ✓ #### Review-niceties applied correctly - **`_formatCooldown`** (`triggers_tab.dart:438-441`) — `seconds % 1 == 0 ? '${toStringAsFixed(0)}s' : '${seconds}s'`. Integer cooldowns stay clean (`30s`), fractional ones show full precision (`1.5s`). Fixes my suggestion #2. ✓ - **Test dialog `hintText`** (`trigger_test_dialog.dart:101`) — `'leave blank to simulate an empty message'`. Fixes my suggestion #3. ✓ #### Lifecycle (unchanged, re-verified) - All three controllers (`_nameController`, `_scriptController`, `_cooldownController`) disposed in `dispose()` (lines 74-76). ✓ - `CodeLineEditingController.dispose()` exists (inherited from `ValueNotifier`). ✓ - Every `setState` is `mounted`-guarded (lines 132, 135-139, 142-147). ✓ - `Navigator.pop` at line 132 is `mounted`-guarded. ✓ #### ✅ What I liked~ - **The indicator builder is thoughtful.** `DefaultCodeLineNumber` + `DefaultCodeChunkIndicator` side by side in a `Row` gives line numbers *and* fold arrows — exactly what a code editor should have, without reinventing the wheel. Clean use of the library's built-in widgets. ♡ - **Theme-aware editor background** (`0xFF282C34` dark / `0xFFFAFAFA` light, following `theme.brightness`) matches atom-one-dark/light theme switching. The editor looks right in both modes without extra work. Nice touch~ ♪ - **Find/replace deliberately skipped** with a clear reason (re_editor ships no default panel UI, trigger scripts are short). Good judgment — not adding complexity that doesn't earn its keep. - **Your self-verification in comment 1016 is exactly the right instinct.** When a controller type changes, re-checking the round-trip invariant that the template-swap depends on is precisely what catches regressions. You did it empirically *and* correctly. Jibril approves~ ♡ Ship it after the manual GUI click-through you noted in the PR body. The code is sound~ ♡ --- *Automated review by Jibril · 2026-07-06* *CI/CD: absent for head SHA `748a1b5` — app has no test suite and PR #17's CI workflow isn't merged yet; `flutter analyze` + `flutter build linux --debug` reported clean by author · Local checks: full read of updated `trigger_edit_dialog.dart`; every re_editor/re_highlight symbol verified against the actual 0.10.0/0.0.3 source on GitHub; template-swap round-trip traced through `CodeLineEditingController` internals (fromText → codeLines → asString with default LF); lifecycle re-verified; review-nicety changes confirmed*
bjoern merged commit b3b8a3306c into main 2026-07-06 21:36:48 +02:00
bjoern deleted branch feat/triggers-ui 2026-07-06 21:36:48 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
TeamAI/angela_assistant!18
No description provided.