diff --git a/apps/studio/components/interfaces/Account/AccessTokens/Scoped/Form/NewScopedTokenForm.tsx b/apps/studio/components/interfaces/Account/AccessTokens/Scoped/Form/NewScopedTokenForm.tsx index eed6f0cae90e8..0d4cb0b02e4e4 100644 --- a/apps/studio/components/interfaces/Account/AccessTokens/Scoped/Form/NewScopedTokenForm.tsx +++ b/apps/studio/components/interfaces/Account/AccessTokens/Scoped/Form/NewScopedTokenForm.tsx @@ -1,4 +1,5 @@ import { zodResolver } from '@hookform/resolvers/zod' +import { useReducedMotion } from 'common' import { ChevronRight, X } from 'lucide-react' import { useEffect, useRef, useState } from 'react' import { useForm, useWatch } from 'react-hook-form' @@ -58,10 +59,10 @@ export const NewScopedTokenForm = ({ }) const [step, setStep] = useState<'form' | 'review'>('form') const [formValues, setFormValues] = useState(DEFAULT_VALUES) - // Dismissal sticks for the sheet's lifetime, so bouncing between steps doesn't resurface it. const [isCreateHintDismissed, setIsCreateHintDismissed] = useState(false) - const [showMissingPermissionsWarning, setShowMissingPermissionsWarning] = useState(false) + const [missingPermissionsAttempts, setMissingPermissionsAttempts] = useState(0) const resourceSectionRef = useRef(null) + const missingPermissionsRef = useRef(null) const resourceAccess = useWatch({ control: form.control, name: 'resourceAccess' }) const selection = useWatch({ control: form.control, name: 'permissions' }) const organizationSlugs = useWatch({ @@ -85,6 +86,9 @@ export const NewScopedTokenForm = ({ }) const { data: permissionScopeMap, isError } = useGetEnabledEndpointsForCapability() + const isReducedMotionPreferred = useReducedMotion() + const isReducedMotionPreferredRef = useRef(isReducedMotionPreferred) + isReducedMotionPreferredRef.current = isReducedMotionPreferred useEffect(() => { if (isError) { @@ -93,15 +97,20 @@ export const NewScopedTokenForm = ({ } }, [onCancel, isError]) - // 'account' switches to the classic token flow: name + expiry only, no permissions or review. + useEffect(() => { + if (missingPermissionsAttempts === 0) return + missingPermissionsRef.current?.scrollIntoView({ + behavior: isReducedMotionPreferredRef.current ? 'auto' : 'smooth', + block: 'nearest', + }) + }, [missingPermissionsAttempts]) + const isClassicMode = resourceAccess === 'account' - // Single owner of the mode switch, so every entry point resets the same dependent fields. const handleSelectLegacyMode = () => { form.setValue('resourceAccess', 'account', { shouldValidate: true }) form.setValue('organizationSlugs', []) form.setValue('projectRefs', []) - // The fields unmount in legacy mode, so drop any validation errors they were holding. form.clearErrors(['organizationSlugs', 'projectRefs']) } @@ -111,7 +120,7 @@ export const NewScopedTokenForm = ({ return } if (configuredCount === 0) { - setShowMissingPermissionsWarning(true) + setMissingPermissionsAttempts((attempts) => attempts + 1) return } setFormValues(values) @@ -120,20 +129,17 @@ export const NewScopedTokenForm = ({ const handlePermissionChange = (key: string, mode: PermissionMode) => { form.setValue('permissions', { ...selection, [key]: mode }) - if (mode !== 'none') setShowMissingPermissionsWarning(false) + if (mode !== 'none') setMissingPermissionsAttempts(0) } const handleApplyPreset = (preset: PermissionPreset) => { const next = applyPreset(preset, selection) form.setValue('permissions', next) - if (countConfigured(next) > 0) setShowMissingPermissionsWarning(false) + if (countConfigured(next) > 0) setMissingPermissionsAttempts(0) } return ( <> - {/* Radix wraps viewport children in an inline-styled display:table div that grows to fit - the widest child, which would let one long endpoint path expand the sheet instead of - clipping — force it back to block so widths are bounded and rows can truncate. */} {step === 'form' ? (
@@ -180,12 +186,9 @@ export const NewScopedTokenForm = ({ onApplyPreset={handleApplyPreset} access={access} /> - {showMissingPermissionsWarning && ( -
+ {missingPermissionsAttempts > 0 && ( +
{ - node?.scrollIntoView() - }} type="warning" title="No permissions selected" description="This token won't be able to do anything until you grant at least one permission." diff --git a/apps/studio/components/interfaces/Account/AccessTokens/Scoped/NewScopedTokenSheet.test.tsx b/apps/studio/components/interfaces/Account/AccessTokens/Scoped/NewScopedTokenSheet.test.tsx index 9811078e177ba..811cc1f20e47e 100644 --- a/apps/studio/components/interfaces/Account/AccessTokens/Scoped/NewScopedTokenSheet.test.tsx +++ b/apps/studio/components/interfaces/Account/AccessTokens/Scoped/NewScopedTokenSheet.test.tsx @@ -2,7 +2,7 @@ import { fireEvent, screen, waitFor } from '@testing-library/react' import userEvent from '@testing-library/user-event' import { platformComponents as components } from 'api-types' import { HttpResponse } from 'msw' -import { beforeEach, describe, expect, test } from 'vitest' +import { beforeEach, describe, expect, test, vi } from 'vitest' import { NewScopedTokenSheet } from './NewScopedTokenSheet' import type { ProfileContextType } from '@/lib/profile' @@ -15,6 +15,12 @@ type ProjectsResponse = components['schemas']['ListProjectsPaginatedResponse'] type CreateTokenResponse = components['schemas']['CreateScopedAccessTokenResponse'] type CreateClassicTokenResponse = components['schemas']['CreateAccessTokenResponse'] +const mockUseReducedMotion = vi.fn(() => false) +vi.mock('common', async (importOriginal) => { + const actual = (await importOriginal()) as typeof import('common') + return { ...actual, useReducedMotion: () => mockUseReducedMotion() } +}) + const user = userEvent.setup({ writeToClipboard: true, }) @@ -227,6 +233,59 @@ describe('NewScopedTokenSheet', () => { fireEvent.click(await screen.findByRole('button', { name: 'Review access' })) expect(await screen.findByText('No permissions selected', { selector: '[role="alert"] *' })) }) + test('scrolls the missing permissions warning into view once per attempt', async () => { + const scrollIntoView = vi.spyOn(window.HTMLElement.prototype, 'scrollIntoView') + renderSheet() + fireEvent.click(await screen.findByRole('button', { name: 'Generate new token' })) + await screen.findByRole('dialog') + await user.type(await screen.findByLabelText('Name'), 'test') + await user.click(await screen.findByRole('radio', { name: /Organization/ })) + fireEvent.click(await screen.findByRole('combobox', { name: 'Organizations' })) + fireEvent.click(await screen.findByRole('option', { name: 'Acme Production' })) + scrollIntoView.mockClear() + + fireEvent.click(await screen.findByRole('button', { name: 'Review access' })) + await screen.findByText('No permissions selected', { selector: '[role="alert"] *' }) + await waitFor(() => expect(scrollIntoView).toHaveBeenCalledTimes(1)) + expect(scrollIntoView).toHaveBeenLastCalledWith({ behavior: 'smooth', block: 'nearest' }) + + // Re-rendering the form while the warning is up must not scroll again + await expandPermissionCategory('Database') + expect(scrollIntoView).toHaveBeenCalledTimes(1) + + fireEvent.click(await screen.findByRole('button', { name: 'Review access' })) + await waitFor(() => expect(scrollIntoView).toHaveBeenCalledTimes(2)) + scrollIntoView.mockRestore() + }) + test('does not re-scroll when the motion preference changes while the warning is visible', async () => { + const scrollIntoView = vi.spyOn(window.HTMLElement.prototype, 'scrollIntoView') + try { + renderSheet() + fireEvent.click(await screen.findByRole('button', { name: 'Generate new token' })) + await screen.findByRole('dialog') + await user.type(await screen.findByLabelText('Name'), 'test') + await user.click(await screen.findByRole('radio', { name: /Organization/ })) + fireEvent.click(await screen.findByRole('combobox', { name: 'Organizations' })) + fireEvent.click(await screen.findByRole('option', { name: 'Acme Production' })) + + fireEvent.click(await screen.findByRole('button', { name: 'Review access' })) + await screen.findByText('No permissions selected', { selector: '[role="alert"] *' }) + await waitFor(() => expect(scrollIntoView).toHaveBeenCalledTimes(1)) + + mockUseReducedMotion.mockReturnValue(true) + // Round-tripping the resource-access scope re-renders the form (resourceAccess is + // watched at the top level) without touching missingPermissionsAttempts, so the + // warning stays up — this is what would surface a stale effect dependency on the + // motion preference. + await user.click(await screen.findByRole('radio', { name: /Project/ })) + await user.click(await screen.findByRole('radio', { name: /Organization/ })) + await screen.findByText('No permissions selected', { selector: '[role="alert"] *' }) + expect(scrollIntoView).toHaveBeenCalledTimes(1) + } finally { + mockUseReducedMotion.mockReturnValue(false) + scrollIntoView.mockRestore() + } + }) test('creates the token when scope is Organization', async () => { renderSheet() fireEvent.click(await screen.findByRole('button', { name: 'Generate new token' })) diff --git a/apps/studio/components/interfaces/Organization/BillingSettings/BillingCustomerData/BillingCustomerData.tsx b/apps/studio/components/interfaces/Organization/BillingSettings/BillingCustomerData/BillingCustomerData.tsx index 42f0369936f41..8484dccb449e8 100644 --- a/apps/studio/components/interfaces/Organization/BillingSettings/BillingCustomerData/BillingCustomerData.tsx +++ b/apps/studio/components/interfaces/Organization/BillingSettings/BillingCustomerData/BillingCustomerData.tsx @@ -57,7 +57,10 @@ export const BillingCustomerData = () => { isSuccess, } = useOrganizationCustomerProfileQuery( { slug }, - { enabled: canReadBillingCustomerData && inView } + { + enabled: canReadBillingCustomerData && inView, + select: (data) => (data ? { address: data.address, billing_name: data.billing_name } : data), + } ) const { diff --git a/apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.test.tsx b/apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.test.tsx new file mode 100644 index 0000000000000..83f8d59289d2d --- /dev/null +++ b/apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.test.tsx @@ -0,0 +1,252 @@ +import { PermissionAction } from '@supabase/shared-types/out/constants' +import { QueryClient } from '@tanstack/react-query' +import { fireEvent, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { platformComponents as components } from 'api-types' +import { mockAnimationsApi } from 'jsdom-testing-mocks' +import { HttpResponse } from 'msw' +import { beforeEach, describe, expect, test, vi } from 'vitest' + +import BillingEmail from './BillingEmail' +import { organizationKeys } from '@/data/organizations/keys' +import { customRender } from '@/tests/lib/custom-render' +import { addAPIMock } from '@/tests/lib/msw' + +type CustomerResponse = components['schemas']['CustomerResponse'] + +// The additional-emails control renders a Radix Popover when adding a recipient. +mockAnimationsApi() + +const SLUG = 'acme-org' + +const mockCheckPermissions = vi.hoisted(() => vi.fn()) + +vi.mock('common', async (importOriginal) => { + const original = (await importOriginal()) as typeof import('common') + return { + ...original, + // The customer-profile query is platform-only; IS_PLATFORM is false by default in tests. + IS_PLATFORM: true, + useParams: () => ({ slug: SLUG }), + } +}) + +vi.mock('@/hooks/misc/useCheckPermissions', () => ({ + useAsyncCheckPermissions: (action: string) => mockCheckPermissions(action), +})) + +vi.mock('react-intersection-observer', () => ({ + useInView: () => ({ ref: vi.fn(), inView: true }), +})) + +const createCustomerProfileResponse = ( + overrides: Partial = {} +): CustomerResponse => ({ + additional_emails: [], + balance: 0, + billing_via_partner: false, + email: 'billing@example.com', + tax_id: null, + ...overrides, +}) + +const mockCustomerProfile = (overrides: Partial = {}) => { + addAPIMock({ + method: 'get', + path: '/platform/organizations/:slug/customer', + response: () => HttpResponse.json(createCustomerProfileResponse(overrides)), + }) +} + +const mockUpdateCustomerProfile = () => { + const requests: Array<{ slug: string | undefined; body: unknown }> = [] + addAPIMock({ + method: 'put', + path: '/platform/organizations/:slug/customer', + response: async ({ request, params }) => { + requests.push({ slug: params.slug as string | undefined, body: await request.json() }) + return HttpResponse.json({}, { status: 204 }) + }, + }) + return requests +} + +const addRecipient = async (email: string) => { + const input = screen.getByPlaceholderText('Add additional recipients') + await userEvent.click(input) + await waitFor(() => expect(input).toHaveAttribute('aria-expanded', 'true')) + // fireEvent.change (rather than userEvent.type) avoids racing the popover's open-state + // transition character-by-character, which otherwise drops the first keystroke(s). + fireEvent.change(input, { target: { value: email } }) + fireEvent.click(await screen.findByRole('option', { name: new RegExp(`Create "${email}"`) })) +} + +const removeRecipient = (email: string) => { + const badge = screen.getByText(email) + fireEvent.click(badge.querySelector('svg')!.parentElement!) +} + +describe('BillingEmail', () => { + beforeEach(() => { + vi.clearAllMocks() + mockCheckPermissions.mockImplementation(() => ({ can: true, isSuccess: true })) + }) + + test('renders the current billing email and additional recipients', async () => { + mockCustomerProfile({ + email: 'billing@example.com', + additional_emails: ['cc@example.com'], + }) + + customRender() + + expect(await screen.findByPlaceholderText('Email')).toHaveValue('billing@example.com') + expect(screen.getByText('cc@example.com')).toBeInTheDocument() + }) + + test('shows a permission notice when the user cannot read billing data', async () => { + mockCheckPermissions.mockImplementation((action: string) => ({ + can: action !== PermissionAction.BILLING_READ, + isSuccess: true, + })) + + customRender() + + expect(await screen.findByText(/view this organization's email recipients/)).toBeInTheDocument() + expect(screen.queryByPlaceholderText('Email')).not.toBeInTheDocument() + }) + + test('disables the email controls when the user cannot update billing data', async () => { + mockCheckPermissions.mockImplementation((action: string) => ({ + can: action !== PermissionAction.BILLING_WRITE, + isSuccess: true, + })) + mockCustomerProfile({ email: 'billing@example.com', additional_emails: [] }) + + customRender() + + expect(await screen.findByPlaceholderText('Email')).toBeDisabled() + expect(await screen.findByPlaceholderText('Add additional recipients')).toBeDisabled() + expect( + screen.getByText('You need additional permissions to update billing emails') + ).toBeInTheDocument() + }) + + test('saves the updated email while keeping the existing additional recipients', async () => { + mockCustomerProfile({ + email: 'billing@example.com', + additional_emails: ['cc@example.com'], + }) + const requests = mockUpdateCustomerProfile() + + customRender() + + const emailInput = await screen.findByPlaceholderText('Email') + await userEvent.clear(emailInput) + await userEvent.type(emailInput, 'new-billing@example.com') + + fireEvent.click(await screen.findByRole('button', { name: 'Save' })) + + await waitFor(() => expect(requests).toHaveLength(1)) + expect(requests[0]).toEqual({ + slug: SLUG, + body: { email: 'new-billing@example.com', additional_emails: ['cc@example.com'] }, + }) + }) + + test('adds a recipient when there are none', async () => { + mockCustomerProfile({ email: 'billing@example.com', additional_emails: [] }) + const requests = mockUpdateCustomerProfile() + + customRender() + await screen.findByPlaceholderText('Email') + + await addRecipient('new@example.com') + fireEvent.click(await screen.findByRole('button', { name: 'Save' })) + + await waitFor(() => expect(requests).toHaveLength(1)) + expect(requests[0]).toEqual({ + slug: SLUG, + body: { email: 'billing@example.com', additional_emails: ['new@example.com'] }, + }) + }) + + test('removes a recipient when there is one', async () => { + mockCustomerProfile({ email: 'billing@example.com', additional_emails: ['cc@example.com'] }) + const requests = mockUpdateCustomerProfile() + + customRender() + await screen.findByText('cc@example.com') + + removeRecipient('cc@example.com') + fireEvent.click(await screen.findByRole('button', { name: 'Save' })) + + await waitFor(() => expect(requests).toHaveLength(1)) + expect(requests[0]).toEqual({ + slug: SLUG, + body: { email: 'billing@example.com', additional_emails: [] }, + }) + }) + + test('appends a recipient when there is already one', async () => { + mockCustomerProfile({ + email: 'billing@example.com', + additional_emails: ['existing@example.com'], + }) + const requests = mockUpdateCustomerProfile() + + customRender() + await screen.findByText('existing@example.com') + + await addRecipient('new@example.com') + fireEvent.click(await screen.findByRole('button', { name: 'Save' })) + + await waitFor(() => expect(requests).toHaveLength(1)) + expect(requests[0]).toEqual({ + slug: SLUG, + body: { + email: 'billing@example.com', + additional_emails: ['existing@example.com', 'new@example.com'], + }, + }) + }) + + test('keeps an in-progress edit when the profile data refetches', async () => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }) + let getCallCount = 0 + let customerProfile = createCustomerProfileResponse({ + email: 'initial@example.com', + additional_emails: ['cc-initial@example.com'], + }) + addAPIMock({ + method: 'get', + path: '/platform/organizations/:slug/customer', + response: () => { + getCallCount++ + return HttpResponse.json(customerProfile) + }, + }) + + customRender(, { queryClient }) + + const emailInput = await screen.findByPlaceholderText('Email') + await waitFor(() => expect(emailInput).toHaveValue('initial@example.com')) + + await userEvent.clear(emailInput) + await userEvent.type(emailInput, 'edited@example.com') + + // Simulate an unrelated refetch of the same shared customer-profile query (e.g. triggered + // by another section, or a background refetch) returning newer server data. + customerProfile = createCustomerProfileResponse({ + email: 'server-updated@example.com', + additional_emails: ['cc-updated@example.com'], + }) + await queryClient.invalidateQueries({ queryKey: organizationKeys.customerProfile(SLUG) }) + await waitFor(() => expect(getCallCount).toBeGreaterThanOrEqual(2)) + + // The dirty form is left untouched - not overwritten with the newly fetched data. + expect(emailInput).toHaveValue('edited@example.com') + expect(screen.getByText('cc-initial@example.com')).toBeInTheDocument() + expect(screen.queryByText('cc-updated@example.com')).not.toBeInTheDocument() + }) +}) diff --git a/apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.tsx b/apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.tsx index f2b7e943f7611..a035f9a4650a1 100644 --- a/apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.tsx +++ b/apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.tsx @@ -27,9 +27,8 @@ import { FormPanel } from '@/components/ui/Forms/FormPanel' import { FormSection, FormSectionContent } from '@/components/ui/Forms/FormSection' import { NoPermission } from '@/components/ui/NoPermission' import { useOrganizationCustomerProfileQuery } from '@/data/organizations/organization-customer-profile-query' -import { useOrganizationUpdateMutation } from '@/data/organizations/organization-update-mutation' +import { useOrganizationCustomerProfileUpdateMutation } from '@/data/organizations/organization-customer-profile-update-mutation' import { useAsyncCheckPermissions } from '@/hooks/misc/useCheckPermissions' -import { useSelectedOrganizationQuery } from '@/hooks/misc/useSelectedOrganization' const FORM_ID = 'org-billing-email' const formSchema = z.object({ @@ -39,53 +38,56 @@ const formSchema = z.object({ const BillingEmail = () => { const { slug } = useParams() - const { data: selectedOrganization } = useSelectedOrganizationQuery() - - const { name, billing_email } = selectedOrganization ?? {} const { can: canReadBillingEmail, isSuccess: isPermissionsLoaded } = useAsyncCheckPermissions( PermissionAction.BILLING_READ, - 'stripe.subscriptions' + 'stripe.customer' ) - const { can: canUpdateOrganization } = useAsyncCheckPermissions( - PermissionAction.UPDATE, - 'organizations' + const { can: canUpdateBillingData } = useAsyncCheckPermissions( + PermissionAction.BILLING_WRITE, + 'stripe.customer' ) const { ref, inView } = useInView({ triggerOnce: true }) - const { data: billingCustomer, isPending: loadingBillingCustomer } = - useOrganizationCustomerProfileQuery({ slug }, { enabled: canReadBillingEmail && inView }) + const { data: customerProfile, isPending: loadingBillingCustomer } = + useOrganizationCustomerProfileQuery( + { slug }, + { + enabled: canReadBillingEmail && inView, + select: (data) => + data ? { email: data.email, additional_emails: data.additional_emails } : data, + } + ) const form = useForm>({ resolver: zodResolver(formSchema), defaultValues: { - billingEmail: billing_email ?? '', - additionalBillingEmails: billingCustomer?.additional_emails ?? [], + billingEmail: customerProfile?.email ?? '', + additionalBillingEmails: customerProfile?.additional_emails ?? [], }, }) const additionalBillingEmails = useWatch({ control: form.control, name: 'additionalBillingEmails', }) - const { errors } = form.formState + const { errors, isDirty } = form.formState const additionalEmailsError = errors.additionalBillingEmails ?? [] - const { mutate: updateOrganization, isPending: isUpdating } = useOrganizationUpdateMutation() + const { mutate: updateCustomerProfile, isPending: isUpdating } = + useOrganizationCustomerProfileUpdateMutation() const onUpdateOrganizationEmail = async (values: z.infer) => { - if (!canUpdateOrganization) { + if (!canUpdateBillingData) { return toast.error('You do not have the required permissions to update this organization') } if (!slug) return console.error('Slug is required') - if (!name) return console.error('Organization name is required') - updateOrganization( + updateCustomerProfile( { slug, - name, - billing_email: values.billingEmail, - additional_billing_emails: values.additionalBillingEmails, + email: values.billingEmail, + additional_emails: values.additionalBillingEmails, }, { onSuccess: () => { @@ -97,13 +99,13 @@ const BillingEmail = () => { } useEffect(() => { - if (billingCustomer) { + if (customerProfile && !isDirty) { form.reset({ - billingEmail: billing_email ?? '', - additionalBillingEmails: billingCustomer.additional_emails ?? [], + billingEmail: customerProfile.email ?? '', + additionalBillingEmails: customerProfile.additional_emails ?? [], }) } - }, [billingCustomer]) + }, [form, customerProfile, isDirty]) return ( @@ -127,11 +129,11 @@ const BillingEmail = () => { { type="email" {...field} placeholder="Email" - disabled={!canUpdateOrganization} + disabled={!canUpdateBillingData} /> @@ -177,7 +179,11 @@ const BillingEmail = () => { } > - + { expect(screen.queryByText("This update can't be applied as written")).not.toBeInTheDocument() }) + it('renders the snapshot-derived diff for a completed delete_cell update, without fetching the notebook', () => { + render( + + ) + + expect(screen.getByText('−1')).toBeInTheDocument() + expect(screen.getByRole('link', { name: 'Open notebook' })).toHaveAttribute( + 'href', + `/project/default/explorer/notebook/${NOTEBOOK_ID}` + ) + }) + + it('renders a single added cell for a completed insert_cell update, not a phantom duplicate', () => { + render( + + ) + + expect(screen.getByText('+1')).toBeInTheDocument() + expect(screen.getAllByRole('button', { name: 'Added Markdown cell' })).toHaveLength(1) + }) + + it('falls back to the compact completed body when previous_content is absent', () => { + render( + + ) + + expect(screen.getByText('Notebook updated: Signup funnel')).toBeInTheDocument() + expect(screen.queryByText("This update can't be applied as written")).not.toBeInTheDocument() + expect(screen.getByRole('link', { name: 'Open notebook' })).toBeInTheDocument() + }) + + it('falls back to the compact completed body when the snapshot no longer matches the operations', () => { + render( + + ) + + expect(screen.getByText('Notebook updated: Signup funnel')).toBeInTheDocument() + expect(screen.queryByText("This update can't be applied as written")).not.toBeInTheDocument() + expect(screen.queryByText('Preview unavailable')).not.toBeInTheDocument() + }) + + it('falls back to the compact completed body when the output id does not match the requested notebook', () => { + render( + + ) + + expect(screen.getByText('Notebook updated: Signup funnel')).toBeInTheDocument() + expect(screen.queryByText('−1')).not.toBeInTheDocument() + }) + it('derives the diff against live content for a denied update', async () => { mockContentItem(mockNotebookRow()) diff --git a/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.tsx b/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.tsx index d2634c6a94830..af710fbf15b77 100644 --- a/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.tsx +++ b/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.tsx @@ -13,6 +13,7 @@ import { createNotebookInputSchema, notebookToolOutputSchema, updateNotebookInputSchema, + updateNotebookToolOutputSchema, } from './Message.utils' import { AlertError } from '@/components/ui/AlertError' import { @@ -340,8 +341,31 @@ function UpdateNotebookProposal({ } if (isCompleted) { - const parsedOutput = notebookToolOutputSchema.safeParse(output) + const parsedOutput = updateNotebookToolOutputSchema.safeParse(output) const notebookName = parsedOutput.success ? parsedOutput.data.name : undefined + const isOutputForRequestedNotebook = + parsedOutput.success && parsedOutput.data.id === parsedInput.data.id + const previousContent = isOutputForRequestedNotebook + ? parsedOutput.data.previous_content + : undefined + const diff = previousContent + ? deriveNotebookDiff(previousContent, parsedInput.data.operations) + : undefined + + if (diff?.success) { + return ( + + + + ) + } return ( ({ mutationFn: (vars) => updateOrganizationCustomerProfile(vars), async onSuccess(data, variables, context) { - const { address, slug, billing_name, tax_id, dry_run } = variables + const { address, slug, billing_name, tax_id, email, additional_emails, dry_run } = variables if (dry_run) { await onSuccess?.(data, variables, context) return } - // Optimistically update the cache for immediate UI consistency + // Optimistically update the cache for immediate UI consistency. Only patch the fields + // that were actually part of this mutation's variables - each caller (e.g. BillingEmail, + // BillingCustomerData) only sends the subset it owns, so an unconditional overwrite here + // would wipe out the other fields in the shared cache entry. queryClient.setQueriesData( { queryKey: organizationKeys.customerProfile(slug) }, (prev: any) => { if (!prev) return prev return { ...prev, - billing_name, + ...(billing_name !== undefined ? { billing_name } : {}), ...(address !== undefined ? { address } : {}), + ...(email !== undefined ? { email } : {}), + ...(additional_emails !== undefined ? { additional_emails: additional_emails } : {}), } } ) diff --git a/apps/studio/lib/ai/generate-assistant-response.utils.test.ts b/apps/studio/lib/ai/generate-assistant-response.utils.test.ts index 7e5d23429df5b..7bce339e1a419 100644 --- a/apps/studio/lib/ai/generate-assistant-response.utils.test.ts +++ b/apps/studio/lib/ai/generate-assistant-response.utils.test.ts @@ -115,6 +115,30 @@ describe('prepareMessagesForModel', () => { expect(result[0].parts).toEqual([]) }) + it('strips update_notebook previous_content before it reaches the model on history replay', () => { + const messages = [ + assistantMessage([ + toolPart({ + state: 'output-available', + output: { + id: 'notebook-1', + name: 'Signup funnel', + previous_content: { schema_version: 1, cells: [] }, + }, + }), + ]), + ] + + const result = prepareMessagesForModel(messages, 'schema') + + expect(result[0].parts).toEqual([ + toolPart({ + state: 'output-available', + output: { id: 'notebook-1', name: 'Signup funnel' }, + }), + ]) + }) + it('keeps non-tool parts untouched', () => { const messages = [assistantMessage([{ type: 'text', text: 'hello' }])] diff --git a/apps/studio/lib/ai/message-utils.test.ts b/apps/studio/lib/ai/message-utils.test.ts index d5acac0b32862..a0be44767f244 100644 --- a/apps/studio/lib/ai/message-utils.test.ts +++ b/apps/studio/lib/ai/message-utils.test.ts @@ -1,4 +1,4 @@ -import type { DynamicToolUIPart, UIMessage } from 'ai' +import type { DynamicToolUIPart, ToolUIPart, UIMessage } from 'ai' import { describe, expect, it } from 'vitest' import { @@ -6,6 +6,7 @@ import { isManualApprovalRequested, prepareMessagesForAPI, } from './message-utils' +import { createAssistantMessageWithUpdateNotebookTool } from './test-fixtures' const makeApprovalPart = (id: string, isAutomatic = false): DynamicToolUIPart => ({ @@ -262,4 +263,38 @@ describe('prepareMessagesForAPI', () => { expect(result[2]).toEqual(messages[2]) expect(result[3]).not.toHaveProperty('results') }) + + it('strips update_notebook previous_content before re-uploading to the API', () => { + const messages = [createAssistantMessageWithUpdateNotebookTool()] + + const result = prepareMessagesForAPI(messages) + + expect((result[0].parts[0] as ToolUIPart).output).toEqual({ + id: 'notebook-1', + name: 'Signup funnel', + }) + }) + + it('does not mutate the original message parts when stripping previous_content', () => { + const messages = [createAssistantMessageWithUpdateNotebookTool()] + const originalParts = messages[0].parts + + prepareMessagesForAPI(messages) + + expect(messages[0].parts).toBe(originalParts) + expect((originalParts[0] as ToolUIPart).output).toHaveProperty('previous_content') + }) + + it('leaves an update_notebook output without previous_content unchanged', () => { + const messages = [ + createAssistantMessageWithUpdateNotebookTool({ id: 'notebook-1', name: 'Signup funnel' }), + ] + + const result = prepareMessagesForAPI(messages) + + expect((result[0].parts[0] as ToolUIPart).output).toEqual({ + id: 'notebook-1', + name: 'Signup funnel', + }) + }) }) diff --git a/apps/studio/lib/ai/message-utils.ts b/apps/studio/lib/ai/message-utils.ts index bb00d524a2c8a..4d86fad7a878e 100644 --- a/apps/studio/lib/ai/message-utils.ts +++ b/apps/studio/lib/ai/message-utils.ts @@ -8,6 +8,15 @@ import { type UIPart = UIMessagePart +/** Strips `update_notebook`'s `previous_content` snapshot — display-only, never re-uploaded. */ +function stripNotebookSnapshot(part: UIPart): UIPart { + if (!isToolUIPart(part) || part.type !== 'tool-update_notebook') return part + if (!part.output || typeof part.output !== 'object') return part + + const { previous_content, ...sanitizedOutput } = part.output as Record + return { ...part, output: sanitizedOutput } as UIPart +} + /** * Prepares messages for API transmission by cleaning and limiting history */ @@ -26,6 +35,11 @@ export function prepareMessagesForAPI(messages: UIMessage[]): UIMessage[] { if (message.role === 'assistant' && message.results) { delete cleanedMessage.results } + // Map into a new array rather than mutating in place — `parts` is shared by reference + // with the locally persisted message the assistant panel reads from. + if (cleanedMessage.parts) { + cleanedMessage.parts = cleanedMessage.parts.map(stripNotebookSnapshot) + } return cleanedMessage as UIMessage }) diff --git a/apps/studio/lib/ai/test-fixtures.ts b/apps/studio/lib/ai/test-fixtures.ts index 1270e795763a8..83bcfbc304326 100644 --- a/apps/studio/lib/ai/test-fixtures.ts +++ b/apps/studio/lib/ai/test-fixtures.ts @@ -50,6 +50,29 @@ export function createAssistantMessageWithExecuteSqlTool( } } +export function createAssistantMessageWithUpdateNotebookTool( + output: Record = { + id: 'notebook-1', + name: 'Signup funnel', + previous_content: { schema_version: 1, cells: [] }, + }, + id = 'assistant-notebook-msg-1' +): UIMessage { + return { + id, + role: 'assistant', + parts: [ + { + type: 'tool-update_notebook', + state: 'output-available', + toolCallId: 'call-notebook-1', + input: { id: 'notebook-1', expected_updated_at: '2026-01-01T00:00:00.000Z' }, + output, + } satisfies ToolUIPart, + ], + } +} + export function createAssistantMessageWithMultipleTools( id = 'assistant-multi-tool-msg-1' ): UIMessage { diff --git a/apps/studio/lib/ai/tools/notebook-tools.test.ts b/apps/studio/lib/ai/tools/notebook-tools.test.ts index 7df9b8b72a62d..2ab12beb1a479 100644 --- a/apps/studio/lib/ai/tools/notebook-tools.test.ts +++ b/apps/studio/lib/ai/tools/notebook-tools.test.ts @@ -632,7 +632,37 @@ describe('ai/tools/notebook-tools', () => { 'cell-2', ]) expect(content.cells[2].sql).toBe('select * from auth.users limit 100') - expect(result).toEqual({ id: 'notebook-1', name: 'Signup funnel' }) + // previous_content is the pre-update notebook (unaffected by this update's + // operations), not the post-update `sentBody` asserted above — it lets the client + // re-derive the diff at the time of approval + expect(result).toEqual({ + id: 'notebook-1', + name: 'Signup funnel', + previous_content: { + schema_version: 1, + cells: [ + { _tag: 'markdown_cell', _id: 'cell-1', text: '# Signup funnel' }, + { + _tag: 'database_cell', + _id: 'cell-2', + sql: 'select * from auth.users limit 100', + row_limit: 100, + view: 'table', + }, + { + _tag: 'log_cell', + _id: 'cell-3', + sql: 'select timestamp, event_message from edge_logs limit 10', + time_range: { _tag: 'relative_time_range', unit: 'hour', amount: 1 }, + view: 'table', + }, + ], + }, + }) + expect((tools.update_notebook as any).toModelOutput({ output: result })).toEqual({ + type: 'json', + value: { id: 'notebook-1', name: 'Signup funnel' }, + }) }) it('should throw a descriptive, assistant-exposable error instead of PUTting when an operation targets an unknown cell id', async () => { diff --git a/apps/studio/lib/ai/tools/notebook-tools.ts b/apps/studio/lib/ai/tools/notebook-tools.ts index 0b7f18ba2df4a..558bc562a56ba 100644 --- a/apps/studio/lib/ai/tools/notebook-tools.ts +++ b/apps/studio/lib/ai/tools/notebook-tools.ts @@ -314,8 +314,15 @@ export const getNotebookTools = (ctx: NotebookToolsContext = {}) => { authHeaders ) - return { id, name: notebook.name } + // `previous_content` lets the client re-derive the diff it already showed for + // approval, without re-fetching a notebook that's now post-update. Never reaches + // the model — see toModelOutput below. + return { id, name: notebook.name, previous_content: wireNotebook } }, + toModelOutput: ({ output }) => ({ + type: 'json', + value: { id: output.id, name: output.name }, + }), }), } } diff --git a/apps/studio/lib/ai/tools/tool-sanitizer.test.ts b/apps/studio/lib/ai/tools/tool-sanitizer.test.ts index 43ecfcc78f52a..14705228f1679 100644 --- a/apps/studio/lib/ai/tools/tool-sanitizer.test.ts +++ b/apps/studio/lib/ai/tools/tool-sanitizer.test.ts @@ -7,6 +7,7 @@ import { prepareMessagesForAPI } from '../message-utils' import { createAssistantMessageWithExecuteSqlTool, createAssistantMessageWithMultipleTools, + createAssistantMessageWithUpdateNotebookTool, createLongConversation, } from '../test-fixtures' import { NO_DATA_PERMISSIONS, sanitizeMessagePart } from './tool-sanitizer' @@ -173,4 +174,23 @@ describe('messages are sanitized based on opt-in level', () => { } }) }) + + test('update_notebook previous_content is stripped from the tool output regardless of opt-in level', () => { + const message = createAssistantMessageWithUpdateNotebookTool() + + const sanitized = sanitizeMessagePart(message.parts[0], 'schema') as ToolUIPart + + expect(sanitized.output).toEqual({ id: 'notebook-1', name: 'Signup funnel' }) + }) + + test('an update_notebook output that never had previous_content passes through unchanged', () => { + const message = createAssistantMessageWithUpdateNotebookTool({ + id: 'notebook-1', + name: 'Signup funnel', + }) + + const sanitized = sanitizeMessagePart(message.parts[0], 'schema') as ToolUIPart + + expect(sanitized.output).toEqual({ id: 'notebook-1', name: 'Signup funnel' }) + }) }) diff --git a/apps/studio/lib/ai/tools/tool-sanitizer.ts b/apps/studio/lib/ai/tools/tool-sanitizer.ts index 1f277dc49a3fe..766bdd0dbe1af 100644 --- a/apps/studio/lib/ai/tools/tool-sanitizer.ts +++ b/apps/studio/lib/ai/tools/tool-sanitizer.ts @@ -32,8 +32,24 @@ const executeSqlSanitizer: ToolSanitizer = { }, } +// `previous_content` is UI-only and must never reach the model — `toModelOutput` on the +// tool covers the same turn, but history replay skips it, so it's stripped here too. +const updateNotebookSanitizer: ToolSanitizer = { + toolName: 'update_notebook', + sanitize: (tool) => { + if (!tool.output || typeof tool.output !== 'object') return tool + + const { previous_content, ...sanitizedOutput } = tool.output as Record + return { + ...tool, + output: sanitizedOutput, + } + }, +} + export const ALL_TOOL_SANITIZERS = { [executeSqlSanitizer.toolName]: executeSqlSanitizer, + [updateNotebookSanitizer.toolName]: updateNotebookSanitizer, } export function sanitizeMessagePart(