Use notification provider instead or deprecated behaviour#100
Use notification provider instead or deprecated behaviour#100klesaulnier wants to merge 10 commits into
Conversation
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe hook useAppParametersInvalidationListener is relocated to src/app/notifications and rewired to use a NotificationsProvider/url generator; messages parsed by the listener trigger invalidateConfigQueries when headers.parameterName is present. Old WebSocket-based hook and its tests are removed; new tests verify listener and URL generation. ChangesNotifications-Based Parameter Invalidation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/notifications/use-app-parameters-invalidation-listener.ts (1)
21-30: ⚡ Quick winWrap
invalidateAppParameterinuseCallbackto prevent listener re-registration on every render.
invalidateAppParameteris recreated on every render and passed aslistenerCallbackMessagetouseNotificationsListener. If the hook internally tracks the callback in auseEffectdependency array (a standard pattern for subscription hooks), any render ofApp— e.g. from a Redux state change — will tear down and re-register the WebSocket listener unnecessarily. Sincedispatchis a stable reference fromuseAppDispatch, wrapping inuseCallbackmakes the function reference effectively stable for the lifetime of the component.♻️ Proposed fix
-import { NotificationsUrlKeys, useNotificationsListener } from '@gridsuite/commons-ui'; +import { useCallback } from 'react'; +import { NotificationsUrlKeys, useNotificationsListener } from '@gridsuite/commons-ui'; import { useAppDispatch } from '../store/store'; import { invalidateConfigQueries } from '../../shared/api/config-api/config-api'; // ... export const useAppParametersInvalidationListener = () => { const dispatch = useAppDispatch(); - const invalidateAppParameter = (event: MessageEvent) => { + const invalidateAppParameter = useCallback((event: MessageEvent) => { const eventData = JSON.parse(event.data) as ConfigNotificationData; if (eventData.headers?.parameterName) { invalidateConfigQueries(dispatch, eventData.headers.parameterName); } - }; + }, [dispatch]); useNotificationsListener(NotificationsUrlKeys.CONFIG, { listenerCallbackMessage: invalidateAppParameter, }); };Please verify the
useNotificationsListenerimplementation in@gridsuite/commons-uito confirm whether thelistenerCallbackMessagereference is used as auseEffectdependency, which would confirm the impact of this change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/notifications/use-app-parameters-invalidation-listener.ts` around lines 21 - 30, The listener callback invalidateAppParameter is recreated on every render and should be wrapped in React's useCallback to avoid re-registering the notification listener; update the component to memoize invalidateAppParameter using useCallback with dispatch as its dependency so the function reference passed to useNotificationsListener (listenerCallbackMessage) remains stable, keeping the internal invalidateConfigQueries(dispatch, ...) call intact and preventing unnecessary teardown/re-registration of the WebSocket listener.src/shared/api/ws/use-notifications-url-generator.ts (1)
21-36: ⚡ Quick winMemoize the returned URL map to prevent spurious WebSocket reconnections.
The hook returns a new object literal on every call.
AppProvidersWithStorere-renders whenever language or theme changes (viauseGetConfigParameterWithFallback), so a fresh object reference is passed toNotificationsProvideron every such render. IfNotificationsProvideruses reference equality to decide when to reconnect (as many subscription providers do), this will cause unnecessary WebSocket teardown/reconnect cycles even though the URL string itself hasn't changed.Since
webSocketBaseUrl,PREFIX_CONFIG_NOTIFICATION_WS, andAPP_NAMEare all effectively constant, onlytokenIdshould gate a new URL.♻️ Proposed fix
-import { NotificationsUrlKeys, PREFIX_CONFIG_NOTIFICATION_WS } from '@gridsuite/commons-ui'; +import { NotificationsUrlKeys, PREFIX_CONFIG_NOTIFICATION_WS } from '@gridsuite/commons-ui'; +import { useMemo } from 'react'; import { APP_NAME } from 'app/config/app-config'; import { selectUser } from '../../../features/authentication/store/authentication.selectors'; import { useAppSelector } from '../../../app/store/store'; // ... export const useNotificationsUrlGenerator = (): Partial<Record<NotificationsUrlKeys, string | undefined>> => { const webSocketBaseUrl = document.baseURI.replace(/^http:\/\//, 'ws://').replace(/^https:\/\//, 'wss://'); const tokenId = useAppSelector(selectUser)?.id_token; - return { - [NotificationsUrlKeys.CONFIG]: getUrlWithToken( - `${webSocketBaseUrl}${PREFIX_CONFIG_NOTIFICATION_WS}/notify?${new URLSearchParams({ - appName: APP_NAME, - })}`, - tokenId - ), - }; + return useMemo( + () => ({ + [NotificationsUrlKeys.CONFIG]: getUrlWithToken( + `${webSocketBaseUrl}${PREFIX_CONFIG_NOTIFICATION_WS}/notify?${new URLSearchParams({ + appName: APP_NAME, + })}`, + tokenId + ), + }), + [webSocketBaseUrl, tokenId] + ); };Please verify how
NotificationsProviderconsumes theurlsprop — specifically whether it uses auseEffectwithurlsin its dependency array (reference equality) or performs a deep/value comparison before reconnecting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/api/ws/use-notifications-url-generator.ts` around lines 21 - 36, The hook useNotificationsUrlGenerator currently returns a new object each render causing spurious reconnects; wrap the returned map in React.useMemo and memoize it by tokenId (and any other non-constant inputs) so it only changes when tokenId changes; specifically compute the URL with getUrlWithToken as before but return the object inside useMemo(() => ({ [NotificationsUrlKeys.CONFIG]: ... }), [tokenId]) so NotificationsProvider receives a stable reference unless the token actually changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/app/__tests__/notifications/use-app-parameters-invalidation-listener.test.ts`:
- Around line 40-48: The test is asserting against a non-existent enum key
NotificationsUrlKeys.MONITOR; update the test to use the correct key
NotificationsUrlKeys.CONFIG where useNotificationsListener is expected to be
called, and change the test description string to reflect the monitor→config
correction; ensure the assertion still checks useNotificationsListener was
called with NotificationsUrlKeys.CONFIG and the listenerCallbackMessage function
(the test references useAppParametersInvalidationListener,
useNotificationsListener and listenerCallbackMessage).
---
Nitpick comments:
In `@src/app/notifications/use-app-parameters-invalidation-listener.ts`:
- Around line 21-30: The listener callback invalidateAppParameter is recreated
on every render and should be wrapped in React's useCallback to avoid
re-registering the notification listener; update the component to memoize
invalidateAppParameter using useCallback with dispatch as its dependency so the
function reference passed to useNotificationsListener (listenerCallbackMessage)
remains stable, keeping the internal invalidateConfigQueries(dispatch, ...) call
intact and preventing unnecessary teardown/re-registration of the WebSocket
listener.
In `@src/shared/api/ws/use-notifications-url-generator.ts`:
- Around line 21-36: The hook useNotificationsUrlGenerator currently returns a
new object each render causing spurious reconnects; wrap the returned map in
React.useMemo and memoize it by tokenId (and any other non-constant inputs) so
it only changes when tokenId changes; specifically compute the URL with
getUrlWithToken as before but return the object inside useMemo(() => ({
[NotificationsUrlKeys.CONFIG]: ... }), [tokenId]) so NotificationsProvider
receives a stable reference unless the token actually changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b313d35-77d8-42af-87ed-284d084d8be6
📒 Files selected for processing (7)
src/app/App.tsxsrc/app/__tests__/notifications/use-app-parameters-invalidation-listener.test.tssrc/app/notifications/use-app-parameters-invalidation-listener.tssrc/app/providers/AppProviders.tsxsrc/features/app-parameters/__tests__/hooks/use-app-parameters-invalidation-listener.test.tssrc/features/app-parameters/hooks/use-app-parameters-invalidation-listener.tssrc/shared/api/ws/use-notifications-url-generator.ts
💤 Files with no reviewable changes (2)
- src/features/app-parameters/tests/hooks/use-app-parameters-invalidation-listener.test.ts
- src/features/app-parameters/hooks/use-app-parameters-invalidation-listener.ts
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/shared/api/ws/__tests__/use-notifications-url-generator.test.ts`:
- Line 45: The test contains a misspelled variable name `expecteConfigUrl`;
rename it to `expectedConfigUrl` everywhere in the test (e.g., where it's
declared in use-notifications-url-generator.test.ts and where it is referenced
later) so the identifier is spelled correctly and consistent; ensure any
assertions or comparisons use `expectedConfigUrl` after the rename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2258ff0e-f8f9-4dc4-8897-720d3e4f4112
📒 Files selected for processing (4)
src/__mocks__/svgrMock.tsxsrc/app/__tests__/notifications/use-app-parameters-invalidation-listener.test.tssrc/shared/api/ws/__tests__/use-notifications-url-generator.test.tssrc/test-utils/mocks/gridsuite-commons-ui.ts
💤 Files with no reviewable changes (1)
- src/mocks/svgrMock.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/tests/notifications/use-app-parameters-invalidation-listener.test.ts
Signed-off-by: LE SAULNIER Kevin <kevin.lesaulnier.pro@gmail.com>
|
1 participant
PR Summary
Replace ws.onMessage which was use before by NotificationProvider now used in our other frontends