From db3adf390588724f3a15ec56abc76dbb31a345af Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Fri, 28 Aug 2026 08:57:39 -0600 Subject: [PATCH 1/2] fix(marketplace): stop the review flow reporting failures twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Approve refused on a private agent showed the backend's message twice: inline on the page, next to the button that was just pressed, and again in the global toast in the corner. The toast is the worse copy of the two — further from the control, and it disappears on its own. Opt the review flow out via the existing SUPPRESS_ERROR_TOAST context token: the submission read, the diff, the review decision, and the withdrawal decision. Each of those already renders the backend's own message inline, and the diff's is load-bearing — it distinguishes "this submission predates snapshots" from a transport failure, which a toast would flatten into "something went wrong". Deliberately not applied service-wide, and there is a test pinning that: takedown has no inline error region on the Listings page, so the toast is its only surface and silencing it would turn a visible failure into a silent one. A call earns the opt-out by having inline UI, not by being in this service. Co-Authored-By: Claude Opus 5 --- .../admin-marketplace.service.spec.ts | 97 +++++++++++++++++++ .../services/admin-marketplace.service.ts | 32 +++++- 2 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.spec.ts diff --git a/frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.spec.ts b/frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.spec.ts new file mode 100644 index 00000000..84b99ce4 --- /dev/null +++ b/frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.spec.ts @@ -0,0 +1,97 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { TestBed } from '@angular/core/testing'; +import { provideHttpClient } from '@angular/common/http'; +import { + HttpTestingController, + provideHttpClientTesting, + TestRequest, +} from '@angular/common/http/testing'; + +import { AdminMarketplaceService } from './admin-marketplace.service'; +import { ConfigService } from '../../../services/config.service'; +import { SUPPRESS_ERROR_TOAST } from '../../../auth/error.interceptor'; + +/** + * The review flow opts out of the global error toast. + * + * Every failure on these calls already renders inline, next to the control that caused it. + * Without the opt-out an admin whose Approve is refused reads the backend's message twice — + * once where they are looking, once in a corner toast that is further away and disappears + * on its own. + * + * Asserted per call rather than as "the service suppresses toasts", because the rule is + * *not* service-wide: silencing a call whose caller renders nothing would turn a visible + * failure into a silent one. A new call has to earn the opt-out by having inline UI, and + * the list below is the record of which ones have. + */ +describe('AdminMarketplaceService — error surfacing', () => { + let service: AdminMarketplaceService; + let http: HttpTestingController; + + beforeEach(() => { + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ + providers: [ + provideHttpClient(), + provideHttpClientTesting(), + AdminMarketplaceService, + { provide: ConfigService, useValue: { appApiUrl: () => 'https://api.test' } }, + ], + }); + service = TestBed.inject(AdminMarketplaceService); + http = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + http.verify(); + TestBed.resetTestingModule(); + }); + + function suppressed(req: TestRequest): boolean { + return req.request.context.get(SUPPRESS_ERROR_TOAST); + } + + it('suppresses the toast when reading a submission', () => { + void service.loadSubmission('ast-001').catch(() => undefined); + const req = http.expectOne('https://api.test/admin/agents/ast-001/submission'); + expect(suppressed(req)).toBe(true); + req.flush({}); + }); + + it('suppresses the toast when reading the diff', () => { + // The backend distinguishes "predates snapshots" from a transport failure, and the + // diff component renders that distinction. A toast would flatten it. + void service.loadDiff('ast-001').catch(() => undefined); + const req = http.expectOne('https://api.test/admin/agents/ast-001/diff'); + expect(suppressed(req)).toBe(true); + req.flush({}); + }); + + it('suppresses the toast when recording a decision', () => { + // The case that prompted this: Approve refused on a private agent showed the same + // message inline and in a toast. + void service.review('ast-001', { decision: 'approve' }).catch(() => undefined); + const req = http.expectOne('https://api.test/admin/agents/ast-001/review'); + expect(suppressed(req)).toBe(true); + req.flush({}); + }); + + it('suppresses the toast when deciding a withdrawal', () => { + void service + .decideWithdrawal('ast-001', { decision: 'grant' }) + .catch(() => undefined); + const req = http.expectOne('https://api.test/admin/agents/ast-001/withdrawal'); + expect(suppressed(req)).toBe(true); + req.flush({}); + }); + + it('leaves the toast on for calls with no inline error surface', () => { + // The guard against a well-meaning sweep that suppresses everything. A takedown + // failure has no inline region on the Listings page, so the toast is its only + // surface — silencing it would turn a visible failure into a silent one. + void service.takedown('ast-001', 'Because.').catch(() => undefined); + const req = http.expectOne('https://api.test/admin/agents/ast-001/takedown'); + expect(suppressed(req)).toBe(false); + req.flush({}); + }); +}); diff --git a/frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.ts b/frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.ts index 3aa44486..bcba5c86 100644 --- a/frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.ts +++ b/frontend/ai.client/src/app/admin/marketplace/services/admin-marketplace.service.ts @@ -1,7 +1,8 @@ import { Injectable, inject, signal, computed } from '@angular/core'; -import { HttpClient } from '@angular/common/http'; +import { HttpClient, HttpContext } from '@angular/common/http'; import { firstValueFrom } from 'rxjs'; import { ConfigService } from '../../../services/config.service'; +import { SUPPRESS_ERROR_TOAST } from '../../../auth/error.interceptor'; import { AgentVersionDiff, AgentVersionsResponse, @@ -48,6 +49,24 @@ export class AdminMarketplaceService { private readonly baseUrl = computed(() => `${this.config.appApiUrl()}/admin/agents`); + /** + * Opts a request out of the global error toast. + * + * Applied to the **review flow** only, where every failure already renders inline next + * to the control that caused it. Without this an admin whose Approve is refused gets the + * backend's message twice — once in the page's own error region, where they are looking, + * and once in a toast in the corner — and the toast is the worse copy of the two: it is + * further from the button they pressed and it disappears on its own. + * + * ⚠️ Deliberately **not** applied service-wide. The toast is the only error surface some + * of these calls have, and silencing one whose caller renders nothing would turn a + * visible failure into a silent one. Add it per call, only after checking that the caller + * actually shows the error. + */ + private inlineErrors(): { context: HttpContext } { + return { context: new HttpContext().set(SUPPRESS_ERROR_TOAST, true) }; + } + private _loading = signal(false); private _error = signal(null); private _pendingCount = signal(0); @@ -76,7 +95,7 @@ export class AdminMarketplaceService { */ async loadDiff(agentId: string): Promise { return firstValueFrom( - this.http.get(`${this.baseUrl()}/${agentId}/diff`), + this.http.get(`${this.baseUrl()}/${agentId}/diff`, this.inlineErrors()), ); } @@ -91,6 +110,7 @@ export class AdminMarketplaceService { return firstValueFrom( this.http.get( `${this.baseUrl()}/${encodeURIComponent(agentId)}/submission`, + this.inlineErrors(), ), ); } @@ -118,7 +138,9 @@ export class AdminMarketplaceService { /** Approve a submission, return it with a reason, or decline it for the store. */ async review(agentId: string, request: ReviewListingRequest): Promise { - await firstValueFrom(this.http.post(`${this.baseUrl()}/${agentId}/review`, request)); + await firstValueFrom( + this.http.post(`${this.baseUrl()}/${agentId}/review`, request, this.inlineErrors()), + ); } /** @@ -130,7 +152,9 @@ export class AdminMarketplaceService { * author while actually re-publishing over their request. */ async decideWithdrawal(agentId: string, request: WithdrawalDecisionRequest): Promise { - await firstValueFrom(this.http.post(`${this.baseUrl()}/${agentId}/withdrawal`, request)); + await firstValueFrom( + this.http.post(`${this.baseUrl()}/${agentId}/withdrawal`, request, this.inlineErrors()), + ); } /** Every snapshot this agent has, newest first — the rollback picker's source (§8). */ From 554c0e7130f1f956c5df809782b305f1e15edc01 Mon Sep 17 00:00:00 2001 From: Phil Merrell Date: Fri, 28 Aug 2026 09:09:54 -0600 Subject: [PATCH 2/2] fix(marketplace): report a refused decision beside the decision buttons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verifying the toast removal on a running stack showed the inline message it was supposed to leave behind renders at the top of the page, 565px from the Approve button that produced it. The decision bar is sticky and the read above it is not, so on a submission with real instructions — this test agent had none, which is why it looked fine — the reviewer presses Approve at the bottom of a scrolled page and the explanation is off-screen above. That is the gap the global toast was covering, and removing the toast without moving the message would have turned a duplicated failure into a silent one. Split the two error regions, because they are read at different moments and from different scroll positions: a load failure is the first thing on an otherwise empty page and stays at the top; a refused decision now renders inside the sticky bar, directly above the buttons. Co-Authored-By: Claude Opus 5 --- .../pages/submission-review.page.spec.ts | 20 +++++++++++ .../pages/submission-review.page.ts | 36 +++++++++++++++++-- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.spec.ts b/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.spec.ts index 95dd6496..a51fb325 100644 --- a/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.spec.ts +++ b/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.spec.ts @@ -237,6 +237,26 @@ describe('SubmissionReviewPage', () => { expect(text(fixture)).toContain('no marketplace listing to review'); }); + it('reports a refused decision beside the buttons, not at the top of the page', async () => { + // The decision bar is sticky and the read above it is not, so on a submission with + // real instructions a message rendered at the top is off-screen at the moment the + // reviewer presses the button. That gap is what the global toast used to cover. + mockService.review.mockRejectedValue({ error: { detail: 'Visibility is now Private.' } }); + const fixture = await render(); + button(fixture, 'Approve').click(); + await fixture.whenStable(); + fixture.detectChanges(); + + const el = fixture.nativeElement as HTMLElement; + const alert = [...el.querySelectorAll('[role="alert"]')].find((a) => + a.textContent?.includes('Visibility is now Private.'), + ); + expect(alert).toBeTruthy(); + // In the same sticky container as the decision buttons. + const bar = button(fixture, 'Approve').closest('.sticky'); + expect(bar?.contains(alert!)).toBe(true); + }); + it('keeps the reviewer on the page when a decision fails', async () => { mockService.review.mockRejectedValue({ error: { detail: 'Visibility is now Private.' } }); const fixture = await render(); diff --git a/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.ts b/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.ts index d0ab9e14..ab5df0b4 100644 --- a/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.ts +++ b/frontend/ai.client/src/app/admin/marketplace/pages/submission-review.page.ts @@ -88,6 +88,8 @@ import { Review queue + @if (error(); as message) {
+ + @if (decisionError(); as message) { + + } + +

@if (isWithdrawal()) { The author asked to pull this listing. Decide it from the queue. @@ -319,6 +338,7 @@ import {

} +
} @@ -334,6 +354,15 @@ export class SubmissionReviewPage implements OnInit { readonly submission = signal(null); readonly loading = signal(true); readonly error = signal(null); + /** + * A refused decision, kept apart from ``error`` so it can render beside the buttons. + * + * Two error regions rather than one because they are read at different moments and from + * different scroll positions: a load failure is the first thing on an otherwise empty + * page, while a decision failure answers a button in a sticky bar the reviewer may have + * scrolled a long way to reach. + */ + readonly decisionError = signal(null); readonly busy = signal(false); private readonly agentId = signal(''); @@ -399,6 +428,7 @@ export class SubmissionReviewPage implements OnInit { } this.loading.set(true); this.error.set(null); + this.decisionError.set(null); try { this.submission.set(await this.service.loadSubmission(id)); } catch (err) { @@ -459,12 +489,12 @@ export class SubmissionReviewPage implements OnInit { note?: string; }): Promise { this.busy.set(true); - this.error.set(null); + this.decisionError.set(null); try { await this.service.review(this.agentId(), request); await this.router.navigate(['/admin/marketplace/review']); } catch (err) { - this.error.set(this.detail(err) ?? 'Failed to record the decision.'); + this.decisionError.set(this.detail(err) ?? 'Failed to record the decision.'); this.busy.set(false); } }