Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4186 +/- ##
==========================================
+ Coverage 97.35% 97.37% +0.01%
==========================================
Files 889 891 +2
Lines 26040 26210 +170
Branches 9417 9485 +68
==========================================
+ Hits 25352 25521 +169
- Misses 641 642 +1
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4fa101b to
efb51d9
Compare
8343dd7 to
aea04d1
Compare
2e7cb8e to
1f4c85f
Compare
221927b to
4379ee3
Compare
493d050 to
fbd48d1
Compare
9dc81fc to
6c9b625
Compare
| * clicking outside the prompt, shifting focus out of the prompt or pressing ESC. | ||
| */ | ||
| onDismiss?: NonCancelableEventHandler<null>; | ||
| onDismiss?: NonCancelableEventHandler<{ method?: string }>; |
There was a problem hiding this comment.
The method is a way the feature prompt was closed - blur, escape, close-button, click-outside.
6286dcf to
1618aa1
Compare
| 'use client'; | ||
| export * from '../internal/plugins/widget/interfaces'; | ||
| export { | ||
| registerFeatureNotifications, |
There was a problem hiding this comment.
Should we group the APIs, same as we do for internal ones?
Like:
const api = {
featureNotifications: {
registerFeatureNotifications,
showFeaturePromptIfPossible,
clearFeatureNotifications,
}
}
export default api;
| id: 'local-feature-notifications', | ||
| suppressFeaturePrompt: false, | ||
| featuresPageLink: '/#/feature-notifications/feature-prompt?appLayoutToolbar=true', | ||
| filterFeatures: () => true, |
There was a problem hiding this comment.
nit: The filterFeatures is unnecessary here
| persistenceConfig: { | ||
| uniqueKey: 'feature-notifications', | ||
| }, | ||
| // DON'T USE |
| }); | ||
|
|
||
| function delay() { | ||
| return act(() => { |
There was a problem hiding this comment.
Is act() really needed? I think the delay() function can potentially be replaced with jest.runAllTimersAsync() or, better, jest.advanceTimersByTimeAsync(time).
Also, the name "delay()" it not semantically sound as we do not use it to delay anything - we use it to wait for the component delays.
| async function renderComponent(jsx: React.ReactElement) { | ||
| const { container, rerender, ...rest } = render(jsx); | ||
| const wrapper = createWrapper(container).findAppLayout()!; | ||
| await delay(); |
There was a problem hiding this comment.
Even if that is needed for all tests to wait until the initial appearance of feature notifications - I would call it explicitly in each test suite to represent the intent better.
| test('shows feature prompt for a latest features', async () => { | ||
| awsuiWidgetPlugins.registerFeatureNotifications(featureNotificationsDefaults); | ||
| const { container } = await renderComponent(<AppLayout />); | ||
| await delay(); |
There was a problem hiding this comment.
We call delay() inside renderComponent, so this call should not be necessary - there are tests where it is called and some where it is not called immediately after render. I recommend to remove it from the render helper and always call it explicitly.
| expect(persistedFeaturesMap).toHaveProperty('feature-1'); | ||
| expect(persistedFeaturesMap).toHaveProperty('feature-2'); | ||
| expect(persistedFeaturesMap).toHaveProperty('recent-seen-feature'); | ||
| expect(persistedFeaturesMap).not.toHaveProperty('old-seen-feature'); |
There was a problem hiding this comment.
nit: would it makes sense to move persistence-related tests to a separate file?
| if (!mountContent) { | ||
| return; | ||
| } | ||
| const container = ref.current!; |
There was a problem hiding this comment.
instead of expecting the content to be there to satisfy TS - should we include it to the if check above?
if (!mountContent || !ref.content) {
return;
}
| Demo page | ||
| </Header> | ||
| </div> | ||
| <Button |
There was a problem hiding this comment.
nit: these buttons do not have margins between them
| import * as toolsContent from '../app-layout/utils/tools-content'; | ||
| import ScreenshotArea from '../utils/screenshot-area'; | ||
|
|
||
| registerFeatureNotifications({ |
There was a problem hiding this comment.
These only work when using appLayoutToolbar=true. I suggest to test if the non-toolbar layout does not crash when using this feature in unit tests, and always render the toolbar variant on this test page - it is confusing otherwise.
There was a problem hiding this comment.
The feature does not seem to be working correctly. The prompt shows when I open dev tools, but not upon page reload. I am testing it in Chrome.
Screen.Recording.2026-02-18.at.15.18.31.mov
There was a problem hiding this comment.
When feature notifications are registered (I added a delay for this to be better noticeable) - the tools disappear.
Screen.Recording.2026-02-18.at.15.24.57.mov
| const i18n = useInternalI18n('features-notification-drawer'); | ||
|
|
||
| return ( | ||
| <InternalDrawer header={i18n('i18nStrings.title', undefined)} disableContentPaddings={true}> |
There was a problem hiding this comment.
This means that the feature cannot be used w/o built-in i18n, and that changing the default messages is only possible with a workaround (by overriding our messages objects).
Should we include (optional) i18nStrings to the runtime API instead?
| ), | ||
| secondaryContent: ( | ||
| <> | ||
| {!!item.releaseDate && ( |
There was a problem hiding this comment.
The releaseDate is required as per our types.
| secondaryContent: ( | ||
| <> | ||
| {!!item.releaseDate && ( | ||
| <Box margin={{ top: 'xs' }} fontSize="body-s" color="text-body-secondary"> |
There was a problem hiding this comment.
There is already a default padding between content and secondary content - why do we add some more?
There was a problem hiding this comment.
Can we use space-between instead of boxes with margins?
| destructor?.(); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
I would add a unit tests where registerFeatureNotifications is called multiple times - sync or async, to assert that the latest content and the latest mountItem are used.
| <InternalDrawer header={i18n('i18nStrings.title', undefined)} disableContentPaddings={true}> | ||
| <Box | ||
| padding={{ top: 'm', left: 'xl', right: 'xl', bottom: 'm' }} | ||
| className={styles['runtime-feature-notifications-drawer-content']} |
There was a problem hiding this comment.
Overriding styles on our components is not recommended. Can we use a custom div here instead?
There was a problem hiding this comment.
The same applies to the .runtime-feature-notifications-footer
| } | ||
| } | ||
|
|
||
| .runtime-feature-notifications-drawer-content { |
There was a problem hiding this comment.
Do we have plans to support test-utils for this feature? I believe we should, as the consumers should have a reliable way to assert the feature prompt and drawer content.
| }; | ||
|
|
||
| const { featureNotificationsProps, onOpenFeatureNotificationsDrawer, featureNotificationsMessageHandler } = | ||
| useFeatureNotifications({ getDrawersIds: () => drawers?.map(drawer => drawer.id) ?? [] }); |
There was a problem hiding this comment.
Why is it necessary to pass the getDrawersIds to the hook? The feature notifications should not care about other drawers.
If I understand it correctly, the registerFeatureNotifications API is responsible for rendering the drawer and also the feature prompt. Can this two actions be joined? For instance, the featureNotificationsProps.drawer can include an optional featurePrompt object - describing the prompt. When it is present - the prompt is shown, but only during the initial render (app layout would handle that).
There was a problem hiding this comment.
Otherwise, there is a bi-directional connection between the app layout state and feature notifications state, which can cause race conditions and is overall difficult to understand.
|
|
||
| setFeatureNotificationsData({ ...payload, features }); | ||
|
|
||
| const persistenceConfig = payload.persistenceConfig ?? DEFAULT_PERSISTENCE_CONFIG; |
There was a problem hiding this comment.
nit: I would add some code comments here explaining the relation between the event payload and the data we extract from the persistence mechanism.
| const persistenceConfig = payload.persistenceConfig ?? DEFAULT_PERSISTENCE_CONFIG; | ||
| const __retrieveFeatureNotifications: | ||
| | ((persistenceConfig: FeatureNotificationsPersistenceConfig) => Promise<Record<string, string>>) | ||
| | undefined = (payload as any)?.__retrieveFeatureNotifications; |
There was a problem hiding this comment.
Can this __retrieveFeatureNotifications be used from the public API? If so - do we have use cases besides testing? If no - I recommend doing some internal version of the API that allows it and make sure that everything starting from __ is omitted from the public plugin (using getExternalProps util).
| } | ||
|
|
||
| if (event.type === 'showFeaturePromptIfPossible') { | ||
| if (markAllAsRead) { |
There was a problem hiding this comment.
Why do we need this extra state? Can we check the featureNotificationsData instead?
| triggerRef?.current!.focus(); | ||
| } | ||
| setFeaturePromptDismissed(true); | ||
| Promise.resolve().then(() => { |
There was a problem hiding this comment.
If the feature prompt was rendered by the app layout (with an extra internal prop on the drawer) - then we could suppress the tooltip whenever the feature prompt is present, w/o introducing the data-attributes.
Is there a reason why the feature cannot be fully controlled with the React state alone?
| const featuresMap = featureNotificationsData.features.reduce((acc, feature) => { | ||
| return { | ||
| ...acc, | ||
| [feature.id]: feature.releaseDate.toString(), |
There was a problem hiding this comment.
the Date.toString() is browser and locale dependent. If the persistence is cross-browser - the keys won't match. If the user would change their locale - the keys won't match.
Description
Introduced a feature notifications API that allows dynamically injecting a feature notifications drawer on pages using the AppLayout component.
The new API includes 3 methods:
registerFeatureNotifications- registers a new drawershowFeaturePromptIfPossible- manually shows a feature prompt next to the drawer's trigger buttonclearFeatureNotifications- clears the feature notifications drawerAlso enhanced the existing feature prompt internal component by adding dismissal context to the onDismiss method (blur, close-button, outside-click, etc.).
Related links, issue #, if available: n/a
How has this been tested?
U tests / manually
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.