diff --git a/.ai/rules/pages.md b/.ai/rules/pages.md index be1918f..d73c608 100644 --- a/.ai/rules/pages.md +++ b/.ai/rules/pages.md @@ -16,3 +16,9 @@ For custom-data (`->records()`-backed, non-Eloquent) tables: use `->callTableAct On a `Table::records()`-backed (non-Eloquent) page, `BulkAction::make(...)->fetchSelectedRecords(false)` does NOT skip hydration the way it does for an Eloquent table — the `Collection $records` passed to `->action()` still contains full row arrays (keyed by the record key), not bare ids. Use `$records->keys()->all()` to get just the selected ids; `$records->all()`/`$records->values()` gives you full row data instead. See `ManageSuggestions::deleteSelectedBulkAction()` / `ManageSuggestionMisses::promoteBulkAction()`. Also: `BnfexpressAdminClient`'s non-2xx handling (`errorMessage()`) must handle `detail` being a list of `{msg, ...}` objects, not just a string — FastAPI's own request-validation failures (422s) return `detail` in that shape, and casting it straight to `(string)` silently produces the literal "Array". + +## "Promote" on ManageSuggestionMisses now reuses the Create Many bulk-add modal +`ManageSuggestionMisses::promoteBulkAction()` no longer calls `promoteSuggestionMisses()` (miss-id based). It opens the same modal shape as `ManageSuggestions::createManyAction()` — info text + editable `items_raw` textarea + lang/intent — prefilled via `fillForm(fn (Collection $records) => ...)` (auto-injected selected records, works in `fillForm` the same way it does in a bulk `->action()`) from the selected misses' own `text_norm`/`lang`. Submits through `batchCreateSuggestions()`, so edited/added lines in the textarea are honored — the created rows no longer map 1:1 to the original miss ids, and the original miss rows are NOT auto-dismissed by this path (unlike the old promote endpoint). + +## `assertNotified()` pulls (and clears) ALL queued notifications on its first call +Filament's `Notification::assertNotified()` reads via `session()->pull('filament.notifications')`, which empties the session key. Chaining `->assertNotified('A')->assertNotified('B')` after one action fails on the second call even if both were actually sent — the first call already drained the queue. To check two notifications from one action, assert only one (whichever isn't covered by another test) rather than chaining. See `ManageSuggestionMisses`'s promote-cleanup-failure test. diff --git a/app-modules/ai-agent/src/Filament/Pages/ManageSuggestionMisses.php b/app-modules/ai-agent/src/Filament/Pages/ManageSuggestionMisses.php index 4ffec1c..1b04f95 100644 --- a/app-modules/ai-agent/src/Filament/Pages/ManageSuggestionMisses.php +++ b/app-modules/ai-agent/src/Filament/Pages/ManageSuggestionMisses.php @@ -5,9 +5,11 @@ namespace Modules\AiAgent\Filament\Pages; use BackedEnum; use Filament\Actions\Action; use Filament\Actions\BulkAction; +use Filament\Forms\Components\Textarea; use Filament\Forms\Components\TextInput; use Filament\Notifications\Notification; use Filament\Pages\Page; +use Filament\Schemas\Components\Text; use Filament\Support\Icons\Heroicon; use Filament\Tables\Columns\IconColumn; use Filament\Tables\Columns\TextColumn; @@ -20,6 +22,7 @@ use Illuminate\Support\Collection; use Modules\AiAgent\Filament\Concerns\HandlesBnfexpressErrors; use Modules\AiAgent\Filament\Concerns\PaginatesBnfexpressLists; use Modules\Shared\Bnfexpress\BnfexpressAdminClient; +use Modules\Shared\Bnfexpress\Exceptions\BnfexpressApiException; use UnitEnum; /** @@ -112,35 +115,74 @@ class ManageSuggestionMisses extends Page implements HasTable }); } + /** + * Opens the same "bulk add" modal shape as ManageSuggestions' + * createManyAction() (info text + editable items textarea + lang/intent), + * prefilled with the selected misses' own text — one per line — so an + * admin can review/tweak the batch before it's created. Submits via + * batchCreateSuggestions() (same endpoint createManyAction uses, and per + * its own comment, bnfexpress's thin wrapper around the misses-promotion + * promote()), not promoteSuggestionMisses() — editing the textarea means + * the created rows no longer map 1:1 to the original miss ids. Once that + * succeeds, the selected misses are deleted via batchDeleteSuggestionMisses() + * so they don't linger in this list now that they're suggestions — best + * effort, since editing the textarea already broke the id mapping and the + * promotion itself is done by that point either way. + */ protected function promoteBulkAction(): BulkAction { return BulkAction::make('promote') ->label('Promote Selected') ->icon(Heroicon::OutlinedArrowUp) - ->fetchSelectedRecords(false) + ->fillForm(fn (Collection $records): array => [ + 'items_raw' => $records->pluck('text_norm')->implode("\n"), + 'lang' => $records->pluck('lang')->filter()->unique()->count() === 1 + ? $records->pluck('lang')->filter()->first() + : null, + ]) ->schema([ + Text::make('Created rows are tagged source: mined by bnfexpress.') + ->color('gray'), + Textarea::make('items_raw') + ->label('Phrases (one per line)') + ->required() + ->rows(8), TextInput::make('lang') - ->label('Language override') - ->helperText("Applied to every selected miss; leave blank to keep each one's own language.") + ->required() ->maxLength(10), TextInput::make('intent'), ]) ->deselectRecordsAfterCompletion() ->action(function (array $data, Collection $records): void { - // Same caveat as ManageSuggestions' deleteSelected — the collection - // holds full row arrays, not just keys, for a custom-data table. + $items = collect(preg_split('/\r\n|\r|\n/', (string) $data['items_raw'])) + ->map(fn (string $line): string => trim($line)) + ->filter() + ->map(fn (string $text): array => array_filter([ + 'text' => $text, + 'lang' => $data['lang'], + 'intent' => $data['intent'] ?: null, + ], fn (mixed $value): bool => $value !== null)) + ->values() + ->all(); + $this->callBnfexpressForResult( - fn () => app(BnfexpressAdminClient::class)->promoteSuggestionMisses( - $records->keys()->all(), - $data['lang'] ?: null, - $data['intent'] ?: null, - ), - function (array $result): void { + fn () => app(BnfexpressAdminClient::class)->batchCreateSuggestions($items), + function (array $result) use ($records): void { Notification::make() ->title("{$result['created']} promoted, {$result['skipped']} skipped") ->success() ->send(); + try { + app(BnfexpressAdminClient::class)->batchDeleteSuggestionMisses($records->keys()->all()); + } catch (BnfexpressApiException $exception) { + Notification::make() + ->title('Promoted, but failed to remove the original misses') + ->body($exception->getMessage()) + ->warning() + ->send(); + } + $this->resetTable(); }, failureTitle: 'Failed to promote suggestion misses', diff --git a/app-modules/ai-agent/tests/Feature/ManageSuggestionMissesTest.php b/app-modules/ai-agent/tests/Feature/ManageSuggestionMissesTest.php index 5643181..9dcb8fe 100644 --- a/app-modules/ai-agent/tests/Feature/ManageSuggestionMissesTest.php +++ b/app-modules/ai-agent/tests/Feature/ManageSuggestionMissesTest.php @@ -69,9 +69,25 @@ test('a failed dismiss surfaces the gateway detail message', function () { ->assertNotified('Failed to dismiss miss'); }); -test('promote bulk action calls promoteSuggestionMisses with the selected ids and shows the result', function () { +test('promote bulk action prefills the bulk-add modal with the selected misses\' text and language', function () { + Http::fake(['bnfexpress.test/*' => Http::response([ + ['id' => 1, 'text_norm' => 'ev charging cost', 'lang' => 'my', 'syllables' => 3, 'was_used' => false, 'created_at' => now()->toIso8601String()], + ['id' => 2, 'text_norm' => 'nearest charger', 'lang' => 'my', 'syllables' => 2, 'was_used' => false, 'created_at' => now()->toIso8601String()], + ])]); + + Livewire::test(ManageSuggestionMisses::class) + ->loadTable() + ->mountTableBulkAction('promote', [1, 2]) + ->assertTableBulkActionDataSet([ + 'items_raw' => "ev charging cost\nnearest charger", + 'lang' => 'my', + ]); +}); + +test('promote bulk action calls batchCreateSuggestions with the (possibly edited) phrases and shows the result', function () { Http::fake(['bnfexpress.test/*' => fn (Request $request) => match (true) { - str_contains($request->url(), '/admin/suggestion-misses/promote') => Http::response(['created' => 2, 'skipped' => 0, 'trie_rebuilt' => true]), + str_contains($request->url(), '/admin/suggestions/batch') => Http::response(['created' => 2, 'skipped' => 0, 'trie_rebuilt' => true]), + str_contains($request->url(), '/admin/suggestion-misses/batch') => Http::response(['deleted' => 2, 'skipped' => 0]), default => Http::response([ ['id' => 1, 'text_norm' => 'a', 'lang' => 'en', 'syllables' => 1, 'was_used' => false, 'created_at' => now()->toIso8601String()], ['id' => 2, 'text_norm' => 'b', 'lang' => 'en', 'syllables' => 1, 'was_used' => false, 'created_at' => now()->toIso8601String()], @@ -80,10 +96,54 @@ test('promote bulk action calls promoteSuggestionMisses with the selected ids an Livewire::test(ManageSuggestionMisses::class) ->loadTable() - ->callTableBulkAction('promote', [1, 2], data: ['lang' => 'my', 'intent' => null]) + ->callTableBulkAction('promote', [1, 2], data: ['items_raw' => "a\nb\nc", 'lang' => 'my', 'intent' => null]) ->assertNotified('2 promoted, 0 skipped'); - Http::assertSent(fn (Request $request) => str_contains($request->url(), '/admin/suggestion-misses/promote') - && $request['miss_ids'] === [1, 2] - && $request['lang'] === 'my'); + Http::assertSent(fn (Request $request) => str_contains($request->url(), '/admin/suggestions/batch') + && $request['items'] === [ + ['text' => 'a', 'lang' => 'my'], + ['text' => 'b', 'lang' => 'my'], + ['text' => 'c', 'lang' => 'my'], + ]); + + Http::assertSent(fn (Request $request) => $request->method() === 'DELETE' + && str_contains($request->url(), '/admin/suggestion-misses/batch') + && $request['miss_ids'] === [1, 2]); +}); + +test('promoting deletes the original misses after a successful promote', function () { + Http::fake(['bnfexpress.test/*' => fn (Request $request) => match (true) { + str_contains($request->url(), '/admin/suggestions/batch') => Http::response(['created' => 1, 'skipped' => 0, 'trie_rebuilt' => true]), + str_contains($request->url(), '/admin/suggestion-misses/batch') => Http::response(['deleted' => 1, 'skipped' => 0]), + default => Http::response([ + ['id' => 1, 'text_norm' => 'ev charging cost', 'lang' => 'en', 'syllables' => 3, 'was_used' => false, 'created_at' => now()->toIso8601String()], + ]), + }]); + + Livewire::test(ManageSuggestionMisses::class) + ->loadTable() + ->callTableBulkAction('promote', [1], data: ['items_raw' => 'ev charging cost', 'lang' => 'en', 'intent' => null]) + ->assertNotified('1 promoted, 0 skipped'); + + Http::assertSent(fn (Request $request) => $request->method() === 'DELETE' + && str_contains($request->url(), '/admin/suggestion-misses/batch') + && $request['miss_ids'] === [1]); +}); + +test('a failed cleanup delete after a successful promote surfaces a separate warning notification', function () { + Http::fake(['bnfexpress.test/*' => fn (Request $request) => match (true) { + str_contains($request->url(), '/admin/suggestions/batch') => Http::response(['created' => 1, 'skipped' => 0, 'trie_rebuilt' => true]), + str_contains($request->url(), '/admin/suggestion-misses/batch') => Http::response(['detail' => 'boom'], 500), + default => Http::response([ + ['id' => 1, 'text_norm' => 'ev charging cost', 'lang' => 'en', 'syllables' => 3, 'was_used' => false, 'created_at' => now()->toIso8601String()], + ]), + }]); + + // assertNotified() pulls (and clears) every queued notification on its + // first call, so only one chained call can see anything — check the + // warning here; the success title is covered by the other promote tests. + Livewire::test(ManageSuggestionMisses::class) + ->loadTable() + ->callTableBulkAction('promote', [1], data: ['items_raw' => 'ev charging cost', 'lang' => 'en', 'intent' => null]) + ->assertNotified('Promoted, but failed to remove the original misses'); }); diff --git a/app-modules/shared/src/Bnfexpress/BnfexpressAdminClient.php b/app-modules/shared/src/Bnfexpress/BnfexpressAdminClient.php index dd14f33..84a64f9 100644 --- a/app-modules/shared/src/Bnfexpress/BnfexpressAdminClient.php +++ b/app-modules/shared/src/Bnfexpress/BnfexpressAdminClient.php @@ -275,6 +275,15 @@ class BnfexpressAdminClient ], fn (mixed $value): bool => $value !== null)); } + /** + * @param list $missIds + * @return array{deleted: int, skipped: int} + */ + public function batchDeleteSuggestionMisses(array $missIds): array + { + return $this->request('DELETE', '/admin/suggestion-misses/batch', body: ['miss_ids' => $missIds]); + } + // --- Suggestion sync/embeddings ------------------------------------------ /**