Skip to content

frontend: fix feature flag reactivity after MobX observer removal#2273

Merged
c-julin merged 3 commits into
masterfrom
frontend-test-updates
Mar 9, 2026
Merged

frontend: fix feature flag reactivity after MobX observer removal#2273
c-julin merged 3 commits into
masterfrom
frontend-test-updates

Conversation

@c-julin
Copy link
Copy Markdown
Contributor

@c-julin c-julin commented Mar 9, 2026

Summary

  • Fix feature flags not updating in React components after MobX observer() removal. isSupported() was reading from MobX api.endpointCompatibility, but components no longer re-rendered when that observable changed.
  • Fix login error modal not closing after MobX removal.

Changes

  • Moved endpointCompatibility into the Zustand useSupportedFeaturesStore as actual state, replacing JS getters with computed boolean properties that update when setEndpointCompatibility is called
  • backend-api.ts now syncs endpoint data to Zustand after fetch
  • ACL create page and shadow link overview card use Zustand hook selectors for reactive feature flag reads
  • Added SecretService and MCPServerService to the list of services that gracefully return false when absent from backend response

Test plan

  • bun run type:check passes
  • bun run lint passes (no new warnings)
  • bun run test — 478/478 tests pass
  • Enterprise E2E tests — 35 passed, 12 skipped, 0 failures
    • ACL creation with schema registry operations: passes
    • ACL allow-all operations (all 6 resource types): passes
    • Shadow link overview card renders: passes
    • Shadow link create with filter: passes

LoginPage reads loginError through the legacy uiState proxy but is
wrapped in observer() which only tracks MobX observables. After PR #2220
migrated loginError to Zustand, the component stopped re-rendering when
loginError changed to null, leaving the modal permanently stuck open.

Subscribe to loginError via useUIStateStore so the component properly
re-renders when the error is dismissed.
Move endpointCompatibility into the Zustand supported-features store
so feature flags recompute when backend data arrives. Components using
useSupportedFeaturesStore selectors now re-render reactively, fixing
ACL schema registry buttons staying disabled and shadow link card
never appearing. Also silence console errors for SecretService and
MCPServerService endpoints not yet returned by the backend.
@c-julin c-julin force-pushed the frontend-test-updates branch from 0c7595f to 6151a6e Compare March 9, 2026 12:25
Comment on lines +223 to +224
const loginError = useUIStateStore((s) => s.loginError);
const setLoginError = useUIStateStore((s) => s.setLoginError);
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.

I think we should check if there is a way to just do 1 useUIStateStore call and fetch both the getter and setter from it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

had a dig into it and claude said the way its done is idiomatic zustand which pushed on it gave

const { loginError, setLoginError } = useUIStateStore(
    useShallow((s) => ({
      loginError: s.loginError,
      setLoginError: s.setLoginError,
    }))

dont know which is better

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.

ok I am fine either way, yes useShallow is to ensure there is no circular dependency.

@c-julin
Copy link
Copy Markdown
Contributor Author

c-julin commented Mar 9, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Mar 9, 2026

Claude finished @c-julin's task in 2m 14s —— View job


Code Review

  • Gather context and read changed files
  • Analyze code for bugs, issues, and improvements
  • Post review feedback

Overall this is a well-structured fix. The core architecture — flattening feature flags into Zustand state and recomputing them atomically in setEndpointCompatibility — is the right approach for replacing MobX reactivity. The computeSupported extraction into a pure function is a nice testability improvement too. A few things worth discussing:


supported-features.ts

isSupported() is still non-reactive — and still being used in components

The PR converts create-acl.tsx and shadow-link-overview-card.tsx to use Zustand selectors (good), but isSupported() is still called inside React components/utilities in:

  • frontend/src/components/pages/topics/Tab.Messages/helpers.tsx (lines 27, 43)
  • frontend/src/utils/route-utils.tsx (lines 116, 259) — called from route config that renders in React

The JSDoc on isSupported() warns about this, but since these call-sites weren't updated, those features remain non-reactive (same bug as before). These may be low-risk if the routes aren't expected to update after initial load, but it's worth confirming — or migrating them too.

featureErrors gets silently replaced on each setEndpointCompatibility call

computeAllFeatures returns a featureErrors array which gets spread into the store state. This means every time setEndpointCompatibility fires, any errors previously added via addFeatureError (e.g. from isSupported() calls) are wiped out and replaced. This is a subtle behavior that might make error tracking unreliable. Consider accumulating errors instead of replacing them, or at least documenting the intention.

Dual source of truth

api.endpointCompatibility (MobX) and useSupportedFeaturesStore.endpointCompatibility (Zustand) both hold the same data. This is fine as a migration step, but it means two places to keep in sync. A comment in backend-api.ts near the sync call (supported-features.ts line ~1018) noting this is intentional during migration would help future readers.


login.tsx

Two separate useUIStateStore calls as already noted by @malinskibeniamin:

// Current
const loginError = useUIStateStore((s) => s.loginError);
const setLoginError = useUIStateStore((s) => s.setLoginError);

Since setLoginError is a Zustand action (stable reference), you can get it from getState() to avoid the second subscription entirely:

const loginError = useUIStateStore((s) => s.loginError);
const setLoginError = useUIStateStore.getState().setLoginError;

Or combine with useShallow if you prefer one hook call:

const { loginError, setLoginError } = useUIStateStore(
  useShallow((s) => ({ loginError: s.loginError, setLoginError: s.setLoginError }))
);

Either way, the current code is functionally correct — just creates an unnecessary subscription for the setter. Fix this →


supported-features.tsTransformService missing from store

Feature.TransformsService is defined in the Feature class but not included in computeAllFeatures or the SupportedFeaturesStore type. This appears to be pre-existing, but with the new approach it's more conspicuous since all other services are listed. Is this intentional?


Test infrastructure (global-setup.mjs, Dockerfile.backend)

The shadowlink test infrastructure additions look well thought-out — separate source/destination clusters, workspace build support, and robust error logging. The CONTAINER_ID_REGEX extraction for failed container diagnostics is a nice touch.

One minor nit: startBackendServerWithConfig duplicates a lot of logic from startBackendServer (license mounting, container start/check, port wait). A future cleanup could extract the shared pattern.


Summary

Core fix ✅ Correct approach, well-structured
login.tsx ⚠️ Two useUIStateStore calls (minor, flagged by reviewer)
Remaining isSupported() call-sites in components ⚠️ Still non-reactive, worth confirming these are acceptable
featureErrors replacement on recompute ℹ️ Subtle behavior, worth documenting
Test infra ✅ Solid additions

return null;
}
this.endpointCompatibility = r.endpointCompatibility;
useSupportedFeaturesStore.getState().setEndpointCompatibility(r.endpointCompatibility);
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.

not sure if we can call the hook in here since it might break the rules of React hooks - ideally the hook is called at all times and we just reference the state at the top.

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.

@claude check if this is ok according to react guidelines

Copy link
Copy Markdown

@claude claude Bot Mar 9, 2026

Choose a reason for hiding this comment

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

Claude finished @malinskibeniamin's task in 47s —— View job


  • Read backend-api.ts around line 1018
  • Analyze React hooks compliance
  • Post findings

React hooks compliance at backend-api.ts:1018

This is fine. The concern is understandable, but line 1018 is not calling the React hook — it's calling Zustand's static .getState() method:

useSupportedFeaturesStore.getState().setEndpointCompatibility(r.endpointCompatibility);

There's an important distinction here:

Call What it is Rules of Hooks apply?
useSupportedFeaturesStore() React hook ✅ Yes — must be called at top level of a component/hook
useSupportedFeaturesStore.getState() Static method on the store object ❌ No — plain JS function call

Zustand attaches getState() and setState() directly to the hook function object. These are ordinary JavaScript functions — they don't interact with React's fiber/hook system at all. The Zustand docs explicitly document this as the recommended pattern for reading/writing store state outside React components (utilities, API clients, async functions, etc.).

So backend-api.ts using .getState() here is the correct Zustand idiom and does not violate any React guidelines. No changes needed.

@c-julin c-julin force-pushed the frontend-test-updates branch 3 times, most recently from 77165e1 to bd1bd4f Compare March 9, 2026 15:50
Wait for connector deletion to propagate before recreating, and use
Playwright's built-in retry helpers instead of manual retry loops.
@c-julin c-julin force-pushed the frontend-test-updates branch from bd1bd4f to 56f32ef Compare March 9, 2026 16:05
@c-julin c-julin merged commit e95f37a into master Mar 9, 2026
13 checks passed
@c-julin c-julin deleted the frontend-test-updates branch March 9, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants