From 0218de559b755fd0f3e51de776592dc056bbe21d Mon Sep 17 00:00:00 2001 From: "kemal.earth" <606977+kemaldotearth@users.noreply.github.com> Date: Fri, 21 Aug 2026 17:32:59 +0100 Subject: [PATCH 1/4] fix(studio): validation scroll area bug in scoped pat (#49395) ## I have read the [CONTRIBUTING.md](https://github.com/supabase/supabase/blob/master/CONTRIBUTING.md) file. YES ## What kind of change does this PR introduce? When trying to submit the scoped pat creation form a second time, after expanding accordion in the `` the `scrollTo` was breaking the height of the container. This PR fixes that. ## Summary by CodeRabbit - **Bug Fixes** - Improved the missing-permissions warning when creating scoped access tokens. - The warning now scrolls into view after each invalid submission attempt, using smooth scrolling when supported. - Prevented repeated scrolling during unrelated form updates or motion-preference changes. - Selecting a permission or applying a non-empty preset clears the warning state. - Improved accessibility by respecting reduced-motion preferences. --- .../Scoped/Form/NewScopedTokenForm.tsx | 35 ++++++----- .../Scoped/NewScopedTokenSheet.test.tsx | 61 ++++++++++++++++++- 2 files changed, 79 insertions(+), 17 deletions(-) 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' })) From b2a216b617f1fb700961cfb11e4d4c852a9db819 Mon Sep 17 00:00:00 2001 From: Daniel Guerra <15204776+danielmx-dev@users.noreply.github.com> Date: Fri, 21 Aug 2026 10:56:20 -0600 Subject: [PATCH 2/4] feat(billing): Use the customer data endpoint to update billing emails (#49160) ## What kind of change does this PR introduce? Change the update billing email component so it uses the update customer endpoint instead of the update org endpoint. This allows users that have the BILLING_WRITE permission to use the endpoint to update relevant organization data, while keeping the restrictions of the update organization endpoint that allow updating other values (e.g. the org name). This change requires an update in the Update Customer endpoint to support billing email updates. Do not merge until that is deployed. ## What is the current behavior? - Admins are not allowed to update the billing emails of an organization. - The update organization endpoint (`PATCH /platform/organizations/{slug}/`) is used to update the billing email details. ## What is the new behavior? - Both admin and owners are allowed to update the billing email details. - The update customer endpoint (`PUT /platform/organizations/{slug}/customer`) is used to update the billing email details. ### Additional Context [Platform PR](https://github.com/supabase/platform/pull/37145), needs to be deployed first. ## Summary by CodeRabbit * **New Features** * Billing email settings now use customer profile information. * Added support for updating primary and additional billing email addresses. * Billing customer details now display address and billing name information. * **Bug Fixes** * Prevented unrelated billing profile fields from being overwritten during updates. * Billing forms now synchronize correctly when customer profile data changes. * Removed unnecessary organization name requirements from billing profile updates. --- .../BillingCustomerData.tsx | 5 +- .../BillingSettings/BillingEmail.test.tsx | 252 ++++++++++++++++++ .../BillingSettings/BillingEmail.tsx | 66 ++--- ...zation-customer-profile-update-mutation.ts | 19 +- 4 files changed, 307 insertions(+), 35 deletions(-) create mode 100644 apps/studio/components/interfaces/Organization/BillingSettings/BillingEmail.test.tsx 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 = () => { } > - + ({ 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 } : {}), } } ) From 8920439569c39eb03ff8eccdc11e6aca97f7fee4 Mon Sep 17 00:00:00 2001 From: Charis <26616127+charislam@users.noreply.github.com> Date: Fri, 21 Aug 2026 14:58:32 -0400 Subject: [PATCH 3/4] Expose previous notebook content in update_notebook (#49401) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Plumb pre-update notebook snapshot through `update_notebook` tool response as `previous_content` - Add sanitizers in `tool-sanitizer.ts` to strip snapshot before model sees it - Add client-side stripping in `prepareMessagesForAPI` to avoid re-uploading snapshot on subsequent turns - This is PR 2 of 3 fixing Linear issue FE-4243 (notebook update proposal shows 'unapplyable' error for already-completed updates) - Ships no visible behavior change on its own; enables PR 3 to restore diff preview for completed updates ## Test plan - [x] Unit tests: 80/80 passing across notebook-tools.test.ts, tool-sanitizer.test.ts, generate-assistant-response.utils.test.ts, message-utils.test.ts, and mock-tools.test.ts - [x] Typecheck: clean for all changed files - [x] ESLint: zero errors, lint:ratchet passes (exit 0) - [x] Integration: previous_content is correctly populated with pre-update notebook, stripped before model context, and stripped on client-side re-upload ## Summary by CodeRabbit - **Bug Fixes** - Notebook updates now retain previous content for recovery and history. - AI responses expose only the notebook’s ID and name, keeping previous content out of model-visible data. - **Tests** - Added coverage for notebook update results, content sanitization, and message preparation, including cases where previous content is absent or preserved. --- .../generate-assistant-response.utils.test.ts | 24 ++++++++++++ apps/studio/lib/ai/message-utils.test.ts | 37 ++++++++++++++++++- apps/studio/lib/ai/message-utils.ts | 14 +++++++ apps/studio/lib/ai/test-fixtures.ts | 23 ++++++++++++ .../lib/ai/tools/notebook-tools.test.ts | 32 +++++++++++++++- apps/studio/lib/ai/tools/notebook-tools.ts | 9 ++++- .../lib/ai/tools/tool-sanitizer.test.ts | 20 ++++++++++ apps/studio/lib/ai/tools/tool-sanitizer.ts | 16 ++++++++ 8 files changed, 172 insertions(+), 3 deletions(-) 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( From 233cbdc8e5f29cd44aa6e5d4a45930ca0e0c7bc2 Mon Sep 17 00:00:00 2001 From: Charis <26616127+charislam@users.noreply.github.com> Date: Fri, 21 Aug 2026 15:09:44 -0400 Subject: [PATCH 4/4] Restore notebook diff preview for completed updates (#49402) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary This is **PR 3 of 3** in the FE-4243 stack fixing "Notebook update proposal shows 'unapplyable' error for already-completed updates." - Consumes the `previous_content` field added by PR 2 (#49401) to reconstruct diffs for already-applied notebook updates - Restores the diff preview that PR 1 initially dropped — completed updates now show the full before/after instead of a generic "Notebook updated" message - Uses the same diff derivation function called pre-approval, guaranteeing the rendered diff matches what was shown during confirmation - Includes defensive fallback handling for older persisted chats (before `previous_content` existed) and edge cases **Depends on**: PR 2 (#49401) merging first — this PR consumes the `previous_content` field from that server change. Resolves FE-4243 ## Test plan - ✅ 19/19 tests pass in NotebookProposalRenderer.test.tsx (2 confirmed as real regressions) - ✅ 118/118 tests pass in full AIAssistantPanel suite - ✅ Typecheck: clean on modified files - ✅ ESLint: zero errors/warnings on changed files - ✅ Lint ratchet: passes (some rules improved) - ✅ New regression tests cover: delete_cell, insert_cell, missing previous_content, and operations that no longer reconcile - ✅ No notebook fetch in completed update tests (proves no redundant re-fetching) ## Summary by CodeRabbit * **New Features** * Added visual previews showing notebook changes, including inserted and deleted cells, when prior content is available. * Prevented duplicate cells from appearing in update previews. * Retained a compact completion message when change details are unavailable or inconsistent. * Ensured previews are shown only for the relevant notebook. * **Tests** * Added coverage for notebook update previews, deletion and insertion diffs, duplicate prevention, notebook matching, and fallback behavior. --- .../ui/AIAssistantPanel/Message.utils.ts | 6 +- .../NotebookProposalRenderer.test.tsx | 135 ++++++++++++++++++ .../NotebookProposalRenderer.tsx | 26 +++- 3 files changed, 165 insertions(+), 2 deletions(-) diff --git a/apps/studio/components/ui/AIAssistantPanel/Message.utils.ts b/apps/studio/components/ui/AIAssistantPanel/Message.utils.ts index 9867cc27511cf..1f901c2ccfe7d 100644 --- a/apps/studio/components/ui/AIAssistantPanel/Message.utils.ts +++ b/apps/studio/components/ui/AIAssistantPanel/Message.utils.ts @@ -2,7 +2,7 @@ import { untrustedSql } from '@supabase/pg-meta' import { z, type SafeParseReturnType } from 'zod' import { notebookOperationsSchema } from '@/data/content/notebooks/notebook-operations' -import { agentNotebookSchema } from '@/data/content/notebooks/notebook-schema' +import { agentNotebookSchema, notebookSchema } from '@/data/content/notebooks/notebook-schema' // Splits markdown into alternating [plain, code, plain, code, ...] segments. // Odd-indexed segments are already inside code spans/fences and should be left alone. @@ -143,6 +143,10 @@ export const updateNotebookInputSchema = z.object({ export const notebookToolOutputSchema = z.object({ id: z.string(), name: z.string() }) +export const updateNotebookToolOutputSchema = notebookToolOutputSchema.extend({ + previous_content: notebookSchema.optional(), +}) + export const rateMessageResponseSchema = z.object({ category: z.enum([ 'sql_generation', diff --git a/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.test.tsx b/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.test.tsx index f48bef2b8c749..f445dec37d290 100644 --- a/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.test.tsx +++ b/apps/studio/components/ui/AIAssistantPanel/NotebookProposalRenderer.test.tsx @@ -339,6 +339,141 @@ describe('NotebookProposalRenderer', () => { 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 (