Skip to content

ai-assistant: appbar: Register events after render - #1247

Open
illume wants to merge 1 commit into
headlamp-k8s:mainfrom
illume:ai-assistant-fix-event-registration
Open

ai-assistant: appbar: Register events after render#1247
illume wants to merge 1 commit into
headlamp-k8s:mainfrom
illume:ai-assistant-fix-event-registration

Conversation

@illume

@illume illume commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • register the Headlamp event callback after React commits instead of during render
  • keep callbacks connected to the latest shared plugin state without registering on rerender
  • render the handler in tests and cover single callback registration

Was seeing this

Warning: Cannot update a component (`Plugins`) while rendering a different component (`qG`). To locate the bad setState() call inside `qG`, follow the stack trace as described in https://reactjs.org/link/setstate-in-render Error Component Stack
    at qG (../src/components/appbar/HeadlampEventHandler.tsx:38:24)
    at ErrorBoundary (ErrorBoundary.tsx:55:1)
    at AppBarActions (TopBar.tsx:248:3)
    at div (<anonymous>)
    at chunk-F3CYMQTH.js:2916:38
    at Toolbar2 (chunk-XAIC7LRB.js:126:17)
    at nav (<anonymous>)
    at chunk-F3CYMQTH.js:2916:38
    at Paper2 (chunk-FWZRMJK5.js:148:17)
    at chunk-F3CYMQTH.js:2916:38
    at AppBar2 (chunk-76NB2BJB.js:186:17)
    at TopBar.tsx:277:5
    at TopBar (TopBar.tsx:102:32)
    at div (<anonymous>)
    at chunk-F3CYMQTH.js:2916:38
    at Box3 (chunk-F2ZA2UMM.js:87:19)
    at div (<anonymous>)
    at chunk-F3CYMQTH.js:2916:38
    at Box3 (chunk-F2ZA2UMM.js:87:19)
    at Layout (Layout.tsx:201:32)
    at MonacoEditorLoaderInitializer (MonacoEditorLoaderInitializer.tsx:29:49)
    at PreviousRouteProvider (RouteSwitcher.tsx:244:41)
    at Router2 (chunk-4MSBMKCL.js:1598:30)
    at BrowserRouter2 (react-router-dom.js:71:35)
    at Router (AppContainer.tsx:160:19)
    at SnackbarProvider2 (notistack.js:1251:24)
    at AppContainer (AppContainer.tsx:168:59)
    at DefaultPropsProvider (chunk-FO5STQHE.js:31:3)
    at RtlProvider (chunk-CCQBJLQB.js:194:5)
    at ThemeProvider (chunk-CCQBJLQB.js:141:5)
    at ThemeProvider2 (chunk-CCQBJLQB.js:356:5)
    at ThemeProvider (chunk-QUXTRDVZ.js:343:12)
    at StyledEngineProvider (chunk-F3CYMQTH.js:3602:5)
    at ThemeProviderNexti18n (ThemeProviderNexti18n.tsx:126:40)
    at I18nextProvider (I18nextProvider.js:4:3)
    at AppWithRedux (App.tsx:39:3)
    at QueryClientProvider (chunk-D4U4PBCO.js:3312:3)
    at Provider (react-redux.js:1013:11)
    at ErrorBoundary (ErrorBoundary.tsx:55:1)
    at App (<anonymous>)

Testing

  • CI=true npm run check
  • look at dev console, and don't see above error.

@illume
illume force-pushed the ai-assistant-fix-event-registration branch from 9e9e8e9 to 399fe74 Compare August 25, 2026 16:10
@illume
illume requested a balanced review from Copilot August 25, 2026 16:29

Copilot AI left a comment

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.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR moves Headlamp event callback registration out of render and into a post-commit effect to avoid React’s “setState during render” warning, while keeping the callback reading the latest plugin state via a ref.

Changes:

  • Register registerHeadlampEventCallback inside useEffect instead of during render.
  • Use a useRef to keep the event handler wired to the latest useGlobalState() value without re-registering.
  • Update tests to render the component and assert single registration across rerenders.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
ai-assistant/src/components/appbar/HeadlampEventHandler.tsx Moves callback registration into useEffect and uses a ref to avoid stale state closures.
ai-assistant/src/components/appbar/HeadlampEventHandler.test.tsx Renders the component in tests and adds coverage for single registration on rerender.
Suppressed comments (1)

ai-assistant/src/components/appbar/HeadlampEventHandler.test.tsx:40

  • After moving registration into useEffect, the callback is registered post-commit rather than during render. This helper reads registerHeadlampEventCallback.mock.calls immediately after render, which can become timing-sensitive depending on React/test environment. To avoid flakes, wait until the mock has been called (e.g., waitFor an expected call count) before accessing .mock.calls.at(-1).
async function getEventCallback() {
  const { default: HeadlampEventHandler } = await import('./HeadlampEventHandler');
  render(<HeadlampEventHandler />);
  return registerHeadlampEventCallback.mock.calls.at(-1)![0] as (event: {
    type: string;
    data?: unknown;
  }) => null;
}

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ai-assistant/src/components/appbar/HeadlampEventHandler.tsx
Comment thread ai-assistant/src/components/appbar/HeadlampEventHandler.test.tsx Outdated
@illume
illume force-pushed the ai-assistant-fix-event-registration branch from 399fe74 to 0ebf5b2 Compare August 25, 2026 16:50
@illume
illume requested a balanced review from Copilot August 25, 2026 16:56

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread ai-assistant/src/components/appbar/HeadlampEventHandler.tsx Outdated
Register the Headlamp callback after commit to avoid dispatching to the store
during React render.

Signed-off-by: René Dudfield <renedudfield@microsoft.com>
@illume
illume force-pushed the ai-assistant-fix-event-registration branch from 0ebf5b2 to 3f846b2 Compare August 25, 2026 18:23
@illume
illume requested a balanced review from Copilot August 25, 2026 19:34

Copilot AI left a comment

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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

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