fix(weave): Prevent Double URL State Update in ButtonOverlay Navigation

[chance-wnb]

Description

Fix: Prevent Double URL State Update in ButtonOverlay Navigation

Problem

When clicking a ButtonOverlay component to navigate to a new URL, the navigation process was creating an unnecessary history entry. This happened because:

1. ButtonOverlay would navigate to the new URL
2. The already-mounted CallsTable component would then update the URL again by adding its column visibility state (cols parameter)
3. This resulted in two history entries: one for the navigation and one for the column visibility update

Solution

I refactored the column visibility state management to separate concerns:

1. Renamed props to be more explicit about their purpose:

// Before
columnVisibilityModel?: GridColumnVisibilityModel;
setColumnVisibilityModel?: (newModel: GridColumnVisibilityModel) => void;

// After
columnVisibilityModelUrlParam?: GridColumnVisibilityModel;
setColumnVisibilityModelUrlParam?: (newModel: GridColumnVisibilityModel) => void;

2. Changed the column visibility logic from an effect to a memo:

// Before
useEffect(() => {
  if (!setColumnVisibilityModel || !columnVisibilityModel) {
    return;
  }
  // ... calculate hidden columns
  setColumnVisibilityModel({
    ...columnVisibilityModel,
    ...hiddenColumnVisibilityFalse,
  });
}, [columns.cols, columnVisibilityModel, setColumnVisibilityModel]);

// After
const columnVisibilityModel = useMemo(() => {
  if (!columnVisibilityModelUrlParam) {
    return undefined;
  }
  // ... calculate hidden columns
  return {
    ...columnVisibilityModelUrlParam,
    ...hiddenColumnVisibilityFalse,
  };
}, [columns.cols, columnVisibilityModelUrlParam]);

This solution works because:

1. It separates the URL state management from the component's internal state
2. Instead of updating the URL when hidden columns are detected, it just computes the effective column visibility model
3. The URL is only updated when the user explicitly changes column visibility through the UI
4. During navigation, the column visibility state is preserved through the URL parameters, but no new URL updates are triggered

• Fixes WB-22275

Alternative Solutions Analysis

I considered several alternative approaches to solve this issue, but each had limitations:

1. Direct history.replace in setColumnVisibilityModel

const setColumnVisibilityModel = (newModel: GridColumnVisibilityModel) => {
  const currentUrl = new URL(window.location.href);
  currentUrl.searchParams.set('cols', JSON.stringify(newModel));
  history.replace(currentUrl.toString());
};

Why it doesn't work:

• The setColumnVisibilityModel is used in many other occasions where we expect a new history record to appear when users manually change column visibility settings
• If I modify it to use history.replace or skip history updates, it would break the expected behavior in those cases
• The root issue is that we need different behavior for user-initiated column visibility changes versus programmatic updates during navigation

2. Add a Navigation Flag to CallsTable

// In ButtonOverlay.tsx
const onClick = () => {
  const targetUrl = new URL(url, window.location.origin);
  targetUrl.searchParams.set('_navigating', 'true');
  history.push(targetUrl.toString());
};

// In CallsTable.tsx
useEffect(() => {
  if (!setColumnVisibilityModel || !columnVisibilityModel) {
    return;
  }
  const isNavigating = new URL(window.location.href).searchParams.has('_navigating');
  if (isNavigating) {
    // Use replace instead of push to avoid intermediate state
    history.replace(newUrl.toString());
  } else {
    history.push(newUrl.toString());
  }
}, [columns.cols, columnVisibilityModel, setColumnVisibilityModel]);

Why it doesn't work that well:

• While this approach technically works by using history.replace during navigation, it's an ugly solution
• It requires adding a special flag to the URL just to control component behavior
• The flag itself becomes part of the URL state, which is not semantically meaningful
• It makes the code harder to understand and maintain
• It's a hacky way to handle what should be a clean state management solution

3. Check if CallsTable is mounted to decide replace/push

const onClick = () => {
  const isCallsTableMounted = document.querySelector('.MuiDataGrid-root') !== null;
  if (isCallsTableMounted) {
    history.replace(url);
  } else {
    history.push(url);
  }
};

Why it doesn't work:

• CallsTable has already been mounted and never gets unmounted during the navigation
• The component persists throughout the entire navigation process
• Even if I use a more reliable way to check if CallsTable is mounted (like a ref or context), the component is still mounted
• The root issue isn't about component lifecycle, but about state management during navigation

4. Dismount CallsTable with keys based on page type

const CallsPageBinding = () => {
  const {tab} = useParamsDecoded<Browse3TabParams>();
  return (
    <CallsTable 
      key={tab}
      // ... other props
    />
  );
};

Why it doesn't work:

• While this approach is technically feasible by forcing CallsTable to remount and detecting the mount state to use history.replace, it has significant drawbacks
• It would require complex code to extract the page type from the path and use it as a key
• The impact is high as it affects the component lifecycle and state management
• It's an overly complex solution for what should be a simple state management issue
• The root issue is about preserving state during navigation, not about component lifecycle

5. Pre-inject the cols= parameter.

Modified the ButtonOverlay component to preserve the current column visibility state during navigation:

-. Before navigation, we capture the current cols parameter from the URL
-. We construct the target URL with the existing column visibility state
-. The downside of this approach is that we manage the URL update in multiple places.
-. This approach is not bullet-proof, it is susceptible to external changes.

This ensures that when we navigate, we're already including the column visibility state, preventing the CallsTable from needing to update the URL again.

The key insight was that instead of trying to prevent the column visibility update from happening, I should ensure it's already included in the navigation URL. This way, there's no need for a second URL update.

Testing

Manually tested FE code against production data locally. I also tested against the production behavior today, there are no additional visibility entries added or removed.

[circle-job-mirror]

Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=908d45bb01a8c59a5d31a97a84b148e05f907097

[tssweeney]

Looks good to me! Maybe one more thing to consider: should all updates to the columnVisiblityModel in the url binding component be treated as a replace? seems a bit odd that changing the column settings results in a pushed history at all.