review: Fix side panel filtering

Author: pulpdrew

packages/app/src/DBDashboardPage.tsx

@@ -672,12 +672,15 @@ const Tile = ({
     () =>
       onFilter && source && (isLogSource(source) || isTraceSource(source))
         ? {
+            // Only the cell popover passes an action; the side panel's
+            // "Add to filters" passes none and shouldn't toggle a condition off.
             onPropertyAddClick: (key, value, action) =>
               onFilter(
                 [{ key, value: String(value) }],
-                action === 'exclude' ? 'exclude' : 'include',
+                action === 'include' || action === 'exclude' ? action : 'add',
               ),
             source,
+            isAdhocFilterTarget: true,
           }
         : undefined,
     [onFilter, source],
@@ -1392,7 +1395,6 @@ const Tile = ({
                       variant="default"
                       errorVariant="collapsible"
                       context={searchRowContext}
-                      isAdhocFilterTarget
                     />
                   </ChartContainer>
                 )}

packages/app/src/components/DBRowSidePanel.tsx

@@ -113,6 +113,11 @@ export type RowSidePanelContextProps = {
   setChildModalOpen?: (open: boolean) => void;
   source?: TLogSource | TTraceSource;
   onOpenLinkedTrace?: (link: SpanLinkData) => void;
+  /**
+   * onPropertyAddClick updates ad hoc dashboard filters. Tables shape
+   * each cell's key and value for that with getAdhocCellCondition.
+   */
+  isAdhocFilterTarget?: boolean;
 };
 
 export const RowSidePanelContext = createContext<RowSidePanelContextProps>({});

packages/app/src/components/DBRowTable.tsx

@@ -378,7 +378,6 @@ export const RawLogTable = memo(
     showExpandButton = true,
     enableRowSelection = false,
     selectionResetKey,
-    isAdhocFilterTarget,
     onSelectedRowsChange,
     getRowWhere,
     variant = 'default',
@@ -414,12 +413,6 @@ export const RawLogTable = memo(
     showExpandButton?: boolean;
     enableRowSelection?: boolean;
     selectionResetKey?: string;
-    /**
-     * Cell include / exclude actions update ad hoc dashboard filters, which
-     * apply outside this query and compare `toString(key)` with the value. See
-     * getAdhocCellCondition for how that shapes each cell's key and value.
-     */
-    isAdhocFilterTarget?: boolean;
     onSelectedRowsChange?: (hasSelectedRows: boolean) => void;
     renderRowDetails?: (row: {
       id: string;
@@ -521,6 +514,8 @@ export const RawLogTable = memo(
     // Get the alias map from the config so we resolve correct column ids
     const { data: aliasMap } = useAliasMapFromChartConfig(config);
 
+    const { isAdhocFilterTarget } = use(RowSidePanelContext);
+
     const getCellFilter = useCallback(
       (
         column: string | undefined,
@@ -1653,7 +1648,6 @@ function DBSqlRowTableComponent({
   showExpandButton = true,
   enableRowSelection = false,
   selectionResetKey,
-  isAdhocFilterTarget,
   onSelectedRowsChange,
   renderRowDetails,
   onSortingChange,
@@ -1688,7 +1682,6 @@ function DBSqlRowTableComponent({
   showExpandButton?: boolean;
   enableRowSelection?: boolean;
   selectionResetKey?: string;
-  isAdhocFilterTarget?: boolean;
   onSelectedRowsChange?: (hasSelectedRows: boolean) => void;
   initialSortBy?: SortingState;
   onSortingChange?: (v: SortingState | null) => void;
@@ -2012,7 +2005,6 @@ function DBSqlRowTableComponent({
         showExpandButton={showExpandButton}
         enableRowSelection={enableRowSelection}
         selectionResetKey={selectionResetKey}
-        isAdhocFilterTarget={isAdhocFilterTarget}
         onSelectedRowsChange={onSelectedRowsChange}
         enableSorting={true}
         onSortingChange={_onSortingChange}

packages/app/src/components/DBSqlRowTableWithSidebar.tsx

@@ -54,7 +54,6 @@ interface Props {
   // it. Enabled by default; pass `false` to opt out.
   closeOnClickOutside?: boolean;
   keepOpenSelector?: string;
-  isAdhocFilterTarget?: boolean;
 }
 
 // Clicking the results table (selecting/switching rows, scrolling) keeps the
@@ -85,7 +84,6 @@ export default function DBSqlRowTableWithSideBar({
   onResolvedColumnsChange,
   closeOnClickOutside = true,
   keepOpenSelector = DEFAULT_KEEP_OPEN_SELECTOR,
-  isAdhocFilterTarget,
 }: Props) {
   const { data: sourceData } = useSource({ id: sourceId });
   const [rowId, setRowId] = useQueryState('rowWhere', parseAsStringEncoded);
@@ -165,7 +163,6 @@ export default function DBSqlRowTableWithSideBar({
         selectionResetKey={selectionResetKey}
         onSelectedRowsChange={onSelectedRowsChange}
         onResolvedColumnsChange={onResolvedColumnsChange}
-        isAdhocFilterTarget={isAdhocFilterTarget}
       />
     </RowSidePanelContext>
   );

packages/app/tests/e2e/features/dashboard-adhoc-click-filters.spec.ts

@@ -13,6 +13,7 @@ import {
   INTERESTING_FILTER_KEYS_SOURCE_NAME,
 } from '../utils/constants';
 import {
+  adhocConditionsParam,
   expectFiltersParam,
   type FilterEntry,
   filtersParam,
@@ -256,22 +257,60 @@ test.describe(
         { name: 'Conditions', sources: [DEFAULT_LOGS_SOURCE_NAME] },
       ]);
 
-      await new TableComponent(page).clickFirstRow();
-      await new SidePanelComponent(
-        page,
-        'row-side-panel',
-      ).addParsedFieldToFilter('ServiceName');
-      await expect
-        .poll(() => filtersParam(page))
-        .toEqual([
-          {
-            type: 'adhoc',
-            name: 'Conditions',
-            conditions: [
-              { key: 'ServiceName', operator: '=', value: expect.any(String) },
-            ],
-          },
-        ]);
+      const sidePanel = new SidePanelComponent(page, 'row-side-panel');
+      const serviceCondition = {
+        key: 'ServiceName',
+        operator: '=',
+        value: expect.any(String),
+      };
+      const expectServiceCondition = () =>
+        expect
+          .poll(() => filtersParam(page))
+          .toEqual([
+            {
+              type: 'adhoc',
+              name: 'Conditions',
+              conditions: [serviceCondition],
+            },
+          ]);
+
+      await test.step('Add to filters adds a condition', async () => {
+        await new TableComponent(page).clickFirstRow();
+        await sidePanel.addParsedFieldToFilter('ServiceName');
+        await expectServiceCondition();
+      });
+
+      await test.step('Adding it again keeps the condition', async () => {
+        await sidePanel.addParsedFieldToFilter('ServiceName');
+        await expectServiceCondition();
+      });
+
+      await test.step('Surrounding context cells are shaped like tile cells', async () => {
+        const [{ value: serviceName }] = adhocConditionsParam(
+          page,
+          'Conditions',
+        );
+        await sidePanel.clickTab('context');
+        const contextTable = sidePanel.contextTab.getByTestId(
+          'search-results-table',
+        );
+        const includeAction = contextTable.getByRole('button', {
+          name: 'Toggle filter for this value',
+        });
+        await contextTable
+          .getByText(serviceName, { exact: true })
+          .first()
+          .hover();
+        await expect(includeAction.last()).toBeVisible();
+        await contextTable
+          .getByText(/\d:\d{2}:\d{2}\.\d{3}/)
+          .first()
+          .hover();
+        await expect(
+          contextTable.getByRole('button', { name: 'Copy field value' }).last(),
+        ).toBeVisible();
+        await expect(includeAction).toHaveCount(0);
+      });
     });
 
     test('offers filter actions only where an ad hoc filter applies', async () => {

packages/app/tests/e2e/utils/filters-param.ts

@@ -26,6 +26,13 @@ export const filtersParam = (page: Page): FilterEntry[] | null => {
   return JSON.parse(decodeURIComponent(raw));
 };
 
+/** The conditions of ad hoc filter `name` in the `filters=` param. */
+export const adhocConditionsParam = (page: Page, name: string) =>
+  filtersParam(page)?.find(
+    (entry): entry is Extract<FilterEntry, { type: 'adhoc' }> =>
+      entry.type === 'adhoc' && entry.name === name,
+  )?.conditions ?? [];
+
 /** Wait for the param to settle on `expected` — nuqs writes it asynchronously. */
 export const expectFiltersParam = async (
   page: Page,

packages/common-utils/src/__tests__/filters.test.ts

@@ -1884,6 +1884,23 @@ describe('filters', () => {
       ).toEqual([svc('=')]);
     });
 
+    it('adds a value without toggling an existing condition off', () => {
+      expect(
+        mergeAdhocConditions(
+          [svc('='), level],
+          [{ key: 'ServiceName', value: 'api' }],
+          'add',
+        ),
+      ).toEqual([svc('='), level]);
+      expect(
+        mergeAdhocConditions(
+          [svc('!=')],
+          [{ key: 'ServiceName', value: 'api' }],
+          'add',
+        ),
+      ).toEqual([svc('=')]);
+    });
+
     it('toggles an excluded value off', () => {
       expect(
         mergeAdhocConditions(

packages/common-utils/src/filters.ts

@@ -1004,7 +1004,7 @@ export function adhocConditionsToFilters(
 }
 
 /** How a value clicked in a tile updates an ad hoc filter. */
-export type AdhocConditionAction = 'include' | 'exclude';
+export type AdhocConditionAction = 'include' | 'exclude' | 'add';
 
 /** A clicked `key = value` pair, before an action gives it an operator. */
 export type AdhocConditionEntry = Pick<AdhocFilterCondition, 'key' | 'value'>;
@@ -1016,13 +1016,14 @@ export type AdhocConditionEntry = Pick<AdhocFilterCondition, 'key' | 'value'>;
  * `include` / `exclude` add a condition per entry, replacing the opposite
  * condition on the same key and value. When every entry's condition already
  * exists, they are removed instead, so a second click undoes the first.
+ * `add` is `include` without the undo, for actions labeled as adding.
  */
 export function mergeAdhocConditions(
   existingConditions: AdhocFilterCondition[],
   newConditions: AdhocConditionEntry[],
   action: AdhocConditionAction,
 ): AdhocFilterCondition[] {
-  const operator: AdhocFilterOperator = action === 'include' ? '=' : '!=';
+  const operator: AdhocFilterOperator = action === 'exclude' ? '!=' : '=';
   const matches = (
     condition: AdhocFilterCondition,
     entry: AdhocConditionEntry,
@@ -1035,14 +1036,14 @@ export function mergeAdhocConditions(
   const isApplied = newConditions.every(entry =>
     existingConditions.some(condition => matches(condition, entry, operator)),
   );
-  if (isApplied) {
+  if (isApplied && action !== 'add') {
     return existingConditions.filter(
       condition =>
         !newConditions.some(entry => matches(condition, entry, operator)),
     );
   }
 
-  const opposite: AdhocFilterOperator = action === 'include' ? '!=' : '=';
+  const opposite: AdhocFilterOperator = action === 'exclude' ? '=' : '!=';
   const kept = existingConditions.filter(
     condition =>
       !newConditions.some(entry => matches(condition, entry, opposite)),