You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Replace drawer context with global store using version-bridge
✨ Enhancement
Walkthroughs
Description
• Replaced React Context with global store for drawer state management
• Removed AppDrawerProvider requirement, enabling drawer usage anywhere
• Reorganized drawer module structure into components, hooks, and utilities
• Added @backstage/version-bridge dependency for singleton store pattern
• Updated tests to work without provider wrapper
• Added demo drawer items to sidebar for testing
Diagram
flowchart LR
A["AppDrawerContext<br/>React Context"] -->|"Replaced with"| B["drawerStore<br/>Global Singleton"]
B -->|"Accessed via"| C["useAppDrawer Hook<br/>useSyncExternalStore"]
D["AppDrawerProvider<br/>Removed"] -->|"No longer needed"| C
E["Reorganized Files<br/>components/hooks/utils"] -->|"Cleaner structure"| F["Scalable architecture"]
1. Drawer state leaks between tests 🐞 Bug☼ Reliability
Description
After removing the provider wrapper in ApplicationDrawer.test.tsx, drawer state now lives in a
global singleton and is not reset between tests. Tests that open a drawer can leave activeDrawerId
set, making subsequent tests order-dependent/flaky.
The drawer store is a global singleton, so unmounting components between tests does not reset it.
ApplicationDrawer.test.tsx opens a drawer in some tests without a corresponding global store
reset, while other tests expect no active drawer.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
Drawer tests became susceptible to cross-test state leakage because the drawer state is now stored in a global singleton and `ApplicationDrawer.test.tsx` does not reset it.
### Issue Context
`useAppDrawer.test.tsx` already implements a local `resetStore()` helper, but `ApplicationDrawer.test.tsx` does not.
### Fix Focus Areas
- workspaces/app-defaults/plugins/app-react/src/drawer/components/ApplicationDrawer.test.tsx[33-109]
- workspaces/app-defaults/plugins/app-react/src/drawer/hooks/useAppDrawer.test.tsx[22-35]
- workspaces/app-defaults/plugins/app-react/src/drawer/utils/drawerStore.ts[27-79]
### Suggested fix
Add a `beforeEach` (or `afterEach`) in `ApplicationDrawer.test.tsx` that resets the global drawer store state.
Preferred:
- Add an internal `reset()` method on `drawerStore` (clears `activeDrawerId` and `widths`) and use it in both test files.
Minimal alternative:
- Import `drawerStore` into `ApplicationDrawer.test.tsx` and close any active drawer in `beforeEach` similar to `useAppDrawer.test.tsx`.
Ensure the reset also clears widths if any tests set widths, to prevent future flakiness.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2. Snapshot bypass in hook 🐞 Bug≡ Correctness
Description
useAppDrawer() subscribes with useSyncExternalStore, but returns isOpen/getWidth as direct
store methods that read the mutable store state instead of the captured snapshot. This can produce
inconsistent results within a render (e.g., activeDrawerId from the snapshot but
isOpen()/getWidth() computed from a newer store state).
The hook captures a snapshot via useSyncExternalStore(...), but exposes
drawerStore.isOpen/drawerStore.getWidth which directly read the internal mutable state
variable inside the store, bypassing the subscribed snapshot.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`useAppDrawer()` mixes snapshot-derived state (`activeDrawerId`) with store methods (`isOpen`, `getWidth`) that read the current mutable store state, bypassing the snapshot returned by `useSyncExternalStore`.
### Issue Context
This can lead to briefly inconsistent results during rendering because `isOpen()` / `getWidth()` are not guaranteed to align with the snapshot used for `activeDrawerId`.
### Fix Focus Areas
- workspaces/app-defaults/plugins/app-react/src/drawer/hooks/useAppDrawer.tsx[28-42]
- workspaces/app-defaults/plugins/app-react/src/drawer/utils/drawerStore.ts[19-78]
### Suggested fix
In `useAppDrawer`, implement `isOpen` and `getWidth` as closures over the `state` snapshot (optionally memoized with `useCallback`). Keep mutators (`openDrawer`, `closeDrawer`, `toggleDrawer`, `setWidth`) as store methods.
Example approach:
- `isOpen: (id) => state.activeDrawerId === id`
- `getWidth: (id) => state.widths.get(id) ?? DEFAULT_WIDTH`
If you want to avoid duplicating `DEFAULT_WIDTH`, export a shared constant from the store/util module and import it into the hook.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
3. No server snapshot provided 🐞 Bug☼ Reliability
Description
useSyncExternalStore is called without the getServerSnapshot argument. If this hook is ever used
during server rendering, React will not have a deterministic server snapshot, risking hydration
mismatches or runtime warnings/errors.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`useSyncExternalStore` is invoked without the third `getServerSnapshot` parameter, which is the SSR-safe snapshot provider.
### Issue Context
Even if the app is currently client-only, this is a latent correctness/reliability issue if SSR or pre-rendering is introduced.
### Fix Focus Areas
- workspaces/app-defaults/plugins/app-react/src/drawer/hooks/useAppDrawer.tsx[28-33]
- workspaces/app-defaults/plugins/app-react/src/drawer/utils/drawerStore.ts[21-79]
### Suggested fix
Pass a third argument to `useSyncExternalStore`, e.g. a function that returns the initial drawer state for server rendering.
Example:
- `useSyncExternalStore(subscribe, getSnapshot, () => ({ activeDrawerId: null, widths: new Map() }))`
(Optionally centralize the initial state creation in the store util to avoid duplication.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
appDrawerModule.tsx documentation still claims it “provides the AppDrawerContext” and relies on a
context provider, but the provider was removed in this PR. This will mislead maintainers about why
the wrapper exists and how drawer state is propagated.
The comment explicitly references AppDrawerContext and a provider, but the implementation now only
renders ApplicationDrawer directly, relying on the new global store rather than React context.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`appDrawerModule.tsx` has outdated documentation referencing `AppDrawerContext`/provider semantics that no longer exist after the global-store migration.
### Issue Context
The wrapper extension may still be needed to render `ApplicationDrawer` and wire NFS inputs, but it no longer "provides context".
### Fix Focus Areas
- workspaces/app-defaults/plugins/app-react/src/drawer/extensions/appDrawerModule.tsx[26-37]
### Suggested fix
Rewrite the comment to describe the current behavior:
- The module registers an app-root wrapper that renders `ApplicationDrawer`.
- Drawer state is provided by a global store (`useAppDrawer`) rather than a React context provider.
- Keep any NFS-related explanation that remains accurate.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ⓘ The new review experience is currently in Beta. Learn more
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![rhdh-qodo-merge[bot]](https://avatars.githubusercontent.com/in/1961108?s=60&v=4){kind=small}
Hey, I just made a Pull Request!
version-bridgepackages' internal store utility.✔️ Checklist