The latest Buf updates on your PR. Results from workflow Buf CI / validate (pull_request).

@jvorcak is this ready for review? looks like a lot of changes

@claude review

Claude finished @jvorcak's task in 9m 11s —— View job

PR Review: UX-870 Remove MobX from Core State

• Gather context and read CLAUDE.md
• Review changed files
• Analyze patterns and identify issues
• Post final review

Summary

This is a substantial refactoring that migrates core state from MobX to Zustand, converting UIState, UISettings, SupportedFeatures, and parts of the Connect cluster state. The approach is sound and backward-compatible proxies are a smart migration strategy. However, there are several functional regressions and bugs introduced that need to be addressed.

🔴 Critical Issues

1. create-auto-modal.tsx: Error-clearing behavior is broken

The original "click OK to clear error, click OK again to retry" UX flow is broken. The check if (!currentUserState) on line 114 will never be true because currentUserState is always assigned from prev.userState (which is non-null while the modal is visible). As a result, after an error, clicking OK both clears the error AND immediately calls onOk again, skipping the intended "click to acknowledge error" step.

// PROBLEM: currentUserState is captured unconditionally before the error check
updateState?.((prev) => {
  currentUserState = prev.userState; // <-- always set (non-null while modal is visible)
  if (prev.result?.error) {
    return { ...prev, result: null };
  }
  return prev;
});
if (!currentUserState) { // <-- this is NEVER true
  return;
}
// Always falls through to call onOk, even when we just cleared an error

The fix would need to track separately whether an error was cleared, e.g. using a local boolean flag. Fix this →

🟠 Major Issues

2. state/ui.ts: Double-write to localStorage on every state change

The useUISettingsStore is configured with Zustand's persist middleware (which calls setItem on every state change), AND there's also a manual useUISettingsStore.subscribe() call on line 710 that triggers scheduleSave() which also writes to localStorage. This causes every state update to trigger two localStorage writes — one immediately via persist and one delayed by 2 seconds via the subscription. The manual subscription is redundant and should be removed (or the persist middleware should be removed in favor of just the manual subscription). Fix this →

3. config.ts: Sidebar items are no longer reactive

The old autorun re-ran whenever api.endpointCompatibility changed. The new code calls updateSidebarItems() exactly once inside the setTimeout:

// Call once on initialization
updateSidebarItems();
// If routes can change dynamically, you can subscribe to relevant state changes
// For now, we just call it once since routes are static

This means if api.endpointCompatibility is not yet available when setTimeout fires (e.g., the feature-check API call is in-flight), sidebar items will be emitted without feature-gating applied, and they will never be updated when the endpoint info arrives. This is a regression in embedded mode where sidebar items need to reflect feature support. Fix this →

4. config.ts: Unbounded polling loop for api.userData

const checkUserData = () => {
  if (api.userData) {
    api.refreshSupportedEndpoints();
    api.listLicenses();
  } else {
    setTimeout(checkUserData, 100); // <-- infinite loop until userData arrives
  }
};

If authentication fails or the user data never arrives, this loop runs forever. The original when() from MobX was reactive and cleaned up. Consider adding a max retry count or a cancellation mechanism. Fix this →

🟡 Moderate Issues

5. state/connect/state.ts: updateConnnector uses stale secret.id instead of response secretId

// Before (used response secretId):
const createSecretResponse = yield api.updateSecret(...);
property.value = secret.getSecretString(key, createSecretResponse?.secretId);

// After (uses pre-update id):
await api.updateSecret(this.clusterName, secret.id, secret.serialized);
property.value = secret.getSecretString(key, secret.id); // secret.id not refreshed from response

If the updateSecret API returns a new/changed secret ID, the property value won't be set correctly. This is a behavioral change worth verifying against the API contract.

6. state/ui-state.ts: Silent error swallowing in topicSettings getter

get topicSettings() {
  try {
    // ...
    throw new Error('reaction for "currentTopicName" was supposed to create topicDetail settings container');
  } catch (error) {
    // In test environments, stores might not be properly initialized
    return createTopicDetailsSettings(''); // silently returns default
  }
},

The broad catch block will hide real production bugs. Consider only catching specific errors, or at minimum console.warn-ing in non-test environments. Fix this →

7. components/misc/search-bar.tsx / cluster-details.tsx: isFilterMatch not memoized

The new SearchBar uses isFilterMatch as a useMemo dependency:

const filteredData = useMemo(() => {
  return source.filter((item) => isFilterMatch(filterText, item));
}, [source, filterText, isFilterMatch]);

But in ConnectorsList (cluster-details.tsx:115), isFilterMatch is defined as an inline arrow function without useCallback, creating a new reference on every render. This causes filteredData to recompute on every render, defeating useMemo. The isFilterMatch prop should be wrapped in useCallback at the call site. Fix this →

🔵 Minor / Design Notes

8. create-auto-modal.tsx: updateState closure variable is fragile for concurrent instances

The module-level closure variable let updateState means only one active Component mount per createAutoModal call is properly supported. If two <modal.Component /> elements from the same createAutoModal were ever mounted simultaneously, they'd overwrite each other's setter. This matches the current singleton-per-page usage, but a useRef pattern would be more robust.

9. require-auth.tsx: observer wrapper on FeatureErrorCheck is no longer needed for Zustand

The FeatureErrorCheck component is still wrapped with observer() from mobx-react, but featureErrors now comes from a Zustand store hook. The observer wrapper is harmless but technically unnecessary for this component (it only remains relevant if there are other MobX observables being accessed elsewhere in the component tree it inherits from).

10. state/ui.ts: persist version bumped to 3 without migration function

Setting version: 3 in the persist config will cause Zustand to discard any stored data from previous versions by default (unless a migrate function is provided). If users have existing settings stored under the old key format (which uses the same settingsName), those will be dropped on first load. The custom getItem already handles the upgrade logic, but since the version is hardcoded to 3 in the response, Zustand's built-in migration is bypassed. This works but is worth a comment clarifying the intentional bypass.

✅ Positive Notes

• The backward-compatible Proxy exports (uiSettings, uiState, Features) are an excellent incremental migration strategy — allows consumers to be migrated independently without a big-bang change.
• SearchBar refactor to a functional component is clean and significantly simpler.
• AdminDebugBundle splitting the class render logic into AdminDebugBundleContent FC is good practice.
• Deletion of filterable-data-source.ts is correct cleanup.
• The createFilterEntry and createTopicDetailsSettings factory functions are clean replacements for constructor initialization.
• Converting flow() to async/await in ConnectClusterStore improves readability significantly.