Skip to content

ref(ui): Drop leftover unused optional args in drawer, table, and filters #123259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JonasBa wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ref(ui): Drop leftover unused optional args in drawer, table, and filters #123259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion static/app/components/core/drawer/components.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function useDrawerContentContext() {
/**
* Rendering props for the inner DrawerPanel component. Inherits the shared
* panel-configuration props directly from DrawerOptions so the two interfaces
* can't drift. GlobalDrawer-only options (onOpen, shouldClose*, onClose
* can't drift. GlobalDrawer-only options (shouldClose*, onClose
* callback) are consumed before reaching this component.
*/
interface DrawerPanelProps extends Pick<
Expand Down
5 changes: 0 additions & 5 deletions static/app/components/core/drawer/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@ export interface DrawerOptions {
* Callback for when the drawer closes
*/
onClose?: () => void;
/**
* Callback for when the drawer opens
*/
onOpen?: () => void;
/**
* If true (default), allows the drawer to be resized - requires `drawerKey`
* to be defined
Expand Down Expand Up @@ -137,7 +133,6 @@ export function GlobalDrawer({children}: any) {
scrollLock.acquire();
}
overwriteDrawerConfig({renderer, options, callerId});
options.onOpen?.();
},
[scrollLock]
);
Expand Down
3 changes: 1 addition & 2 deletions static/app/components/core/table/table.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ Omit it to size the column with `minmax(minimumColumnWidth, auto)`, and set `res
</Table>
```

Widths are uncontrolled by default, and the component tracks them per column key.
Pass `onColumnResize` to control them instead, which is how `GridEditable` persists widths to the URL.
Widths are uncontrolled, and the component tracks them per column key.

## Status Rows

Expand Down
28 changes: 8 additions & 20 deletions static/app/components/core/table/table.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
within,
} from 'sentry-test/reactTestingLibrary';

import {COL_WIDTH_UNDEFINED, Table, type TableColumnConfig} from '@sentry/scraps/table';
import {Table, type TableColumnConfig} from '@sentry/scraps/table';

const COLUMNS: TableColumnConfig[] = [
{key: 'name', width: 200},
Expand Down Expand Up @@ -135,13 +135,12 @@ describe('Table', () => {
await waitFor(() => expect(gridTemplate()).toBe('90px 150px minmax(90px, auto)'));
});

it('reports the final width to onColumnResize when a drag ends', () => {
const onColumnResize = jest.fn();
render(<TestTable onColumnResize={onColumnResize} />);
it('keeps the resized width after a drag ends', async () => {
render(<TestTable />);

dragHandle(resizers()[1]!, {from: 100, to: 350});

expect(onColumnResize).toHaveBeenCalledWith(1, 250);
await waitFor(() => expect(gridTemplate()).toBe('200px 250px minmax(90px, auto)'));
});

it('retains the resized width when no onColumnResize is provided', async () => {
Expand All @@ -164,15 +163,6 @@ describe('Table', () => {
);
});

it('reports an undefined width to onColumnResize when a handle is double-clicked', async () => {
const onColumnResize = jest.fn();
render(<TestTable onColumnResize={onColumnResize} />);

await userEvent.dblClick(resizers()[0]!);

expect(onColumnResize).toHaveBeenCalledWith(0, COL_WIDTH_UNDEFINED);
});

it('places a resize handle in the tab order', async () => {
render(<TestTable />);

Expand Down Expand Up @@ -201,22 +191,20 @@ describe('Table', () => {
});

it('commits a resize when a focused handle is arrowed', async () => {
const onColumnResize = jest.fn();
render(<TestTable onColumnResize={onColumnResize} />);
render(<TestTable />);

await userEvent.tab();
await userEvent.keyboard('{ArrowRight}');

expect(onColumnResize).toHaveBeenCalledWith(0, 90);
await waitFor(() => expect(gridTemplate()).toBe('90px 150px minmax(90px, auto)'));
});

it('does not commit a resize when a handle is right-clicked', () => {
const onColumnResize = jest.fn();
render(<TestTable onColumnResize={onColumnResize} />);
render(<TestTable />);

dragHandle(resizers()[0]!, {button: 2, from: 100, to: 400});

expect(onColumnResize).not.toHaveBeenCalled();
expect(gridTemplate()).toBe('200px 150px minmax(90px, auto)');
});

it('keeps the in-progress width when an unrelated re-render lands mid-drag', async () => {
Expand Down
13 changes: 4 additions & 9 deletions static/app/components/core/table/table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ export interface TableProps extends Omit<
columns?: TableColumnConfig[];
flexibleLastColumn?: boolean;
minimumColumnWidth?: number;
onColumnResize?: (index: number, width: number) => void;
prependColumnWidths?: string[];
ref?: RefObject<HTMLTableElement | null>;
}
Expand All @@ -117,7 +116,6 @@ export function Table({
columns = EMPTY_COLUMNS,
flexibleLastColumn = true,
minimumColumnWidth = COL_WIDTH_MINIMUM,
onColumnResize,
prependColumnWidths,
ref,
...props
Expand All @@ -126,12 +124,11 @@ export function Table({
const gridRef = ref ?? internalRef;

const [internalWidths, setInternalWidths] = useState<Record<string, number>>({});
const isControlled = !!onColumnResize;

const resolveWidth = useCallback(
(column: TableColumnConfig): ResolvedWidth =>
isControlled ? column.width : (internalWidths[column.key] ?? column.width),
[internalWidths, isControlled]
internalWidths[column.key] ?? column.width,
[internalWidths]
);

const buildTemplate = useCallback(
Expand Down Expand Up @@ -159,13 +156,11 @@ export function Table({
(index: number, width: number) => {
const key = columns[index]?.key;

if (onColumnResize) {
onColumnResize(index, width);
} else if (key) {
if (key) {
setInternalWidths(current => ({...current, [key]: width}));
}
},
[columns, onColumnResize]
[columns]

@cursor cursor Bot Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Table resize callback still used

High Severity

Table no longer accepts or invokes onColumnResize, and it always stores widths internally. GridEditable still passes that callback through DataTable.Grid so Discover, Performance, Insights, and Explore can persist column widths, so those resizes never reach the parent.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c65a096. Configure here.

);

const getResizeTemplate = useCallback(
Expand Down
3 changes: 0 additions & 3 deletions static/app/components/modals/createTeamModal.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import CreateTeamModal from 'sentry/components/modals/createTeamModal';
describe('CreateTeamModal', () => {
const org = OrganizationFixture();
const closeModal = jest.fn();
const onClose = jest.fn();

beforeEach(() => {
jest.clearAllMocks();
Expand All @@ -34,7 +33,6 @@ describe('CreateTeamModal', () => {
Header={p => <span>{p.children}</span>}
organization={org}
closeModal={closeModal}
onClose={onClose}
CloseButton={makeCloseButton(() => {})}
/>
);
Expand All @@ -47,7 +45,6 @@ describe('CreateTeamModal', () => {
`/organizations/${org.slug}/teams/`,
expect.objectContaining({data: {slug: 'new-team'}})
);
expect(onClose).toHaveBeenCalledWith(team);
expect(closeModal).toHaveBeenCalled();
});
});
3 changes: 0 additions & 3 deletions static/app/components/modals/createTeamModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {slugify} from 'sentry/utils/slugify';

interface Props extends ModalRenderProps {
organization: Organization;
onClose?: (team: Team) => void;
}

const schema = z.object({
Expand All @@ -28,7 +27,6 @@ function CreateTeamModal({
Footer,
Header,
organization,
onClose,
closeModal,
}: Props) {
const {mutateAsync: submitCreateTeam} = useMutation({
Comment on lines 27 to 32

@sentry sentry Bot Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The CreateTeamModal no longer accepts an onClose prop, so the callback passed by callers to receive the newly created team is never executed.
Severity: HIGH

Suggested Fix

Update the openCreateTeamModal function to follow the pattern used by other modal openers. Destructure the onClose callback from the options object and pass it as part of the second argument to the openModal function. This ensures the modal framework handles the onClose event correctly when the modal is closed.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: static/app/components/modals/createTeamModal.tsx#L27-L32

Potential issue: The `CreateTeamModal` component was refactored to no longer accept an
`onClose` prop. However, the `openCreateTeamModal` function still accepts an `onClose`
callback from its callers (e.g., in `TeamSelector`) and spreads it as a prop to the
modal component. Since the modal component no longer uses the `onClose` prop, the
callback is never invoked when a team is successfully created and the modal is closed.
This breaks the expected behavior for callers, which rely on this callback to receive
the newly created team object and resolve promises or trigger subsequent UI updates.

Also affects:

  • static/app/components/onboarding/createSampleEventButton.tsx:22~27
  • static/app/components/timeSince.tsx:41~46
  • static/app/components/core/table/table.tsx:107~112
  • static/app/components/pageFilters/project/projectPageFilter.tsx:49~54

Did we get this right? 👍 / 👎 to inform future reviews.

Expand All @@ -49,7 +47,6 @@ function CreateTeamModal({
})
);
closeModal();
onClose?.(team);

@cursor cursor Bot Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Team create callback still required

High Severity

CreateTeamModal no longer calls onClose after a successful create. TeamSelector and the settings team-select dropdown still pass that callback to add the new team to a project and resolve the create-team promise, so those flows hang and skip the follow-up work.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit c65a096. Configure here.

},
onError: (_err, variables) => {
addErrorMessage(
Expand Down
4 changes: 0 additions & 4 deletions static/app/components/onboarding/createSampleEventButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import {useOrganization} from 'sentry/utils/useOrganization';
type CreateSampleEventButtonProps = ButtonProps & {
source: string;
hasScmOnboarding?: boolean;
onClick?: () => void;
project?: Project;
};

Expand All @@ -32,7 +31,6 @@ const EVENT_POLL_INTERVAL = 1000;
export function CreateSampleEventButton({
source,
hasScmOnboarding,
onClick,
project,
...buttonProps
}: CreateSampleEventButtonProps) {
Expand Down Expand Up @@ -129,8 +127,6 @@ export function CreateSampleEventButton({
source,
});

onClick?.();

navigate(
normalizeUrl(
`/organizations/${organization.slug}/issues/${groupID}/?project=${project!.id}&referrer=sample-error`
Expand Down
5 changes: 1 addition & 4 deletions static/app/components/pageFilters/project/projectPageFilter.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,7 @@ describe('ProjectPageFilter', () => {
});

it('handles reset', async () => {
const onReset = jest.fn();
const {router} = render(<ProjectPageFilter onReset={onReset} />, {
const {router} = render(<ProjectPageFilter />, {
organization,
initialRouterConfig: {
location: {pathname: '/organizations/org-slug/issues/', query: {}},
Expand All @@ -178,9 +177,7 @@ describe('ProjectPageFilter', () => {
await userEvent.click(screen.getByRole('button', {name: 'project-1'}));
await userEvent.click(screen.getByRole('button', {name: 'Reset'}));

// Trigger button was updated, onReset was called
expect(screen.getByRole('button', {name: 'My Projects'})).toBeInTheDocument();
expect(onReset).toHaveBeenCalled();
});

it('responds to page filter changes, async e.g. from back button nav', async () => {
Expand Down
6 changes: 0 additions & 6 deletions static/app/components/pageFilters/project/projectPageFilter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,6 @@ export interface ProjectPageFilterProps extends Partial<
* Called when the selection changes
*/
onChange?: (selected: number[]) => void;
/**
* Called when the reset button is clicked
*/
onReset?: () => void;
/**
* Reset these URL params when we fire actions (custom routing only)
*/
Expand All @@ -66,7 +62,6 @@ export interface ProjectPageFilterProps extends Partial<

export function ProjectPageFilter({
onChange,
onReset,
disabled,
menuTitle,
menuWidth,
Expand Down Expand Up @@ -497,7 +492,6 @@ export function ProjectPageFilter({
const handleReset = () => {
clearDraftSelectionState();
commitSelection(memberProjectIds(projects));
onReset?.();

trackAnalytics('projectselector.clear', {
path: routePath,
Expand Down
10 changes: 0 additions & 10 deletions static/app/components/timeSince.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,21 +39,11 @@ describe('TimeSince', () => {
expect(screen.getByText('10 minutes')).toBeInTheDocument();
});

it('renders a relative date without prefix', () => {
render(<TimeSince date={futureTenMin} prefix="" />);
expect(screen.getByText('10 minutes')).toBeInTheDocument();
});

it('renders a custom suffix', () => {
render(<TimeSince date={pastTenMin} suffix="until lunch" />);
expect(screen.getByText('10 minutes until lunch')).toBeInTheDocument();
});

it('renders a custom prefix', () => {
render(<TimeSince date={futureTenMin} prefix="lunch is in" />);
expect(screen.getByText('lunch is in 10 minutes')).toBeInTheDocument();
});

it('renders a custom suffix with shortened', () => {
render(<TimeSince unitStyle="extraShort" date={pastTenMin} suffix="atrás" />);
expect(screen.getByText('10m atrás')).toBeInTheDocument();
Expand Down
13 changes: 3 additions & 10 deletions static/app/components/timeSince.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,6 @@ interface Props extends Omit<
* Max width of the tooltip
*/
maxWidth?: InfoTextProps<'time'>['maxWidth'];
/**
* Prefix before upcoming time (when the date is in the future)
*
* @default "in"
*/
prefix?: string;
/**
* Suffix after elapsed time e.g. "ago" in "5 minutes ago"
*
Expand Down Expand Up @@ -91,7 +85,6 @@ export function TimeSince({
variant = 'inherit',
maxWidth,
unitStyle,
prefix = t('in'),
suffix = t('ago'),
liveUpdateInterval = 'minute',
...props
Expand All @@ -101,8 +94,8 @@ export function TimeSince({

const relative = useMemo(() => {
void tick; // Ensure recomputation when tick changes
return getRelativeDate(date, suffix, prefix, unitStyle);
}, [date, suffix, prefix, unitStyle, tick]);
return getRelativeDate(date, suffix, t('in'), unitStyle);
}, [date, suffix, unitStyle, tick]);

useEffect(() => {
const interval =
Expand Down Expand Up @@ -137,7 +130,7 @@ export function TimeSince({
<RelativeTime
date={date}
label={tooltipPrefix}
prefix={prefix}
prefix={t('in')}
suffix={suffix}
unitStyle={unitStyle}
showSeconds={tooltipShowSeconds}
Expand Down
Loading