Implement cohesive Storybook for staff + community UI portal application package Fixes #266#283
Open
rohit-r-kumar wants to merge 266 commits into
Open
Implement cohesive Storybook for staff + community UI portal application package Fixes #266#283rohit-r-kumar wants to merge 266 commits into
rohit-r-kumar wants to merge 266 commits into
Conversation
…ify ADR entry in readme
… into rohit-r-kumar/issue212
Agent-Logs-Url: https://github.com/CellixJs/cellixjs/sessions/b332a369-259e-49fd-b45e-84b75c4b82b5 Co-authored-by: ttrang-nguyen <126544378+ttrang-nguyen@users.noreply.github.com>
ui-staff-route-shared -> ui-staff-shared ui-components -> ui-shared
Agent-Logs-Url: https://github.com/CellixJs/cellixjs/sessions/3d00d38e-39b6-480b-8f35-9451c37a4c08 Co-authored-by: rohit-r-kumar <175348946+rohit-r-kumar@users.noreply.github.com>
- Add StaffAppRoles constants (Staff.TechAdmin, Staff.Finance, Staff.ServiceLineOwner, Staff.CaseManager) - Add centralized staffRouteRoles mapping route paths to required roles - Extract extractRoles helper to staff-app-roles.ts (shared utility) - Add RequireRole component that reads StaffAuthContext and redirects to /unauthorized when user lacks the required role - Wrap each /staff/* route in App.tsx with RequireRole using the centralized mapping - Filter nav links in StaffRouteShell to only show sections the user can access based on their roles - Add react-router-dom and react-dom to ui-staff-shared dependencies - Add unit tests for StaffAppRoles, staffRouteRoles, extractRoles, and RequireRole component Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t-r-kumar/issue214
…a app roles (#214) Tests cover every role × route combination: - Staff.TechAdmin: /staff/tech allowed, all others blocked - Staff.Finance: /staff/finance allowed, all others blocked - Staff.CaseManager: /staff/community + /staff/users allowed, others blocked - Staff.ServiceLineOwner: /staff/community + /staff/users allowed, others blocked - No roles: all /staff/* routes blocked Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove antd and react-dom from dependencies where unused, and storybook from devDependencies across all ui-staff-route-* packages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…zation Covers all 4 Entra app-role × 4 route combinations (20 access tests) plus 3 nav-link visibility tests = 23 tests total. Strategy: inject a fake OIDC session into sessionStorage via page.addInitScript() before the app loads. oidc-client-ts restores stored users without re-validating the JWT signature, so each test can control the user's roles independently without restarting the mock auth server. Test matrix: - Staff.TechAdmin: /tech ✓, /finance ✗, /community ✗, /users ✗ - Staff.Finance: /finance ✓, /tech ✗, /community ✗, /users ✗ - Staff.CaseManager: /community ✓, /users ✓, /finance ✗, /tech ✗ - Staff.ServiceLineOwner:/community ✓, /users ✓, /finance ✗, /tech ✗ - No roles: all 4 routes ✗ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…enityJS feat: add new test commands for end-to-end and acceptance tests in turbo.json style: format vitest.config.ts for consistency in quotes and indentation
Contributor
Reviewer's GuideAdds a cohesive Storybook setup for the staff UI portal app, wires it into the monorepo tooling, and adjusts auth/permissions handling and story naming to better support SSR, Storybook, and future role-based behaviour. Sequence diagram for Storybook staff story rendering with global decoratorssequenceDiagram
participant Storybook
participant Decorator as GlobalDecorator
participant Helmet as HelmetProvider
participant Auth as AuthProvider
participant Theme as ThemeProvider
participant Apollo as MockedProvider
participant Router as MemoryRouter
participant Story
Storybook->>Decorator: render(Story, context)
Decorator->>Decorator: read context.parameters
Decorator->>Helmet: wrap children
Helmet->>Auth: wrap children with mockOidcConfig
Auth->>Theme: wrap children
Theme->>Apollo: wrap children with mocks, defaultOptions
Apollo->>Router: wrap children with initialEntries
Router->>Story: render
Story-->>Storybook: rendered staff UI story
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
.storybook/preview.tsx, when buildingdefaultOptionsforMockedProvideryou overwritewatchQueryandquerycompletely; consider shallow-merging those nested objects with anyapolloParams.defaultOptionspassed in so story-specific cache/policy configuration isn’t lost. - In
.storybook/main.ts, most addons are resolved viagetAbsolutePathbutstorybook-addon-apollo-clientis left as a bare string; for consistency and to avoid resolution issues in the monorepo/PnP setup, you may want to resolve that addon viagetAbsolutePathas well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `.storybook/preview.tsx`, when building `defaultOptions` for `MockedProvider` you overwrite `watchQuery` and `query` completely; consider shallow-merging those nested objects with any `apolloParams.defaultOptions` passed in so story-specific cache/policy configuration isn’t lost.
- In `.storybook/main.ts`, most addons are resolved via `getAbsolutePath` but `storybook-addon-apollo-client` is left as a bare string; for consistency and to avoid resolution issues in the monorepo/PnP setup, you may want to resolve that addon via `getAbsolutePath` as well.
## Individual Comments
### Comment 1
<location path="packages/ocom/ui-staff-shared/src/index.tsx" line_range="27-28" />
<code_context>
const resolvedPermissions = React.useMemo(() => {
if (explicitRoles && explicitRoles.length > 0) return explicitRoles;
const perms = auth?.permissions;
- if (!perms) return [];
- return Object.entries(perms)
- .filter(([, isEnabled]) => isEnabled === true)
- .map(([permKey]) => permKey);
+ if (perms) {
+ return Object.entries(perms)
+ .filter(([, isEnabled]) => isEnabled === true)
</code_context>
<issue_to_address>
**issue (bug_risk):** Fallback to roles is skipped when permissions exist but have no enabled entries.
With this change, `auth.roles` is only used when `auth.permissions` is falsy. If the backend sets `permissions` to an object (possibly empty, or with all `false` values) while resolving, `perms` is truthy and you’ll return an empty array instead of falling back to `auth.roles`, which can under-authorize users. It’d be safer to detect the "no enabled permissions" case (e.g., after filtering enabled entries) and then fall back to `auth.roles` instead of returning an empty list.
</issue_to_address>
### Comment 2
<location path="apps/ui-staff/.storybook/preview.tsx" line_range="31" />
<code_context>
+ const initialEntries = context.parameters?.memoryRouter?.initialEntries ?? ['/'];
+ const apolloParams = context.parameters?.apolloClient ?? {};
+ const mocks = apolloParams.mocks ?? apolloMocks;
+ const { defaultOptions } = apolloParams;
+
+ return (
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Merging Apollo `defaultOptions` overwrites any existing `watchQuery`/`query` options from stories.
In `MockedProvider`, the current spread:
```ts
defaultOptions={{
...defaultOptions,
watchQuery: { fetchPolicy: 'network-only' },
query: { fetchPolicy: 'network-only' },
}}
```
overwrites any `watchQuery`/`query` options from `parameters.apolloClient.defaultOptions` (e.g. `errorPolicy`, `pollInterval`). To preserve those while enforcing the fetch policy, deep-merge the sub-objects instead:
```ts
defaultOptions={{
...defaultOptions,
watchQuery: { ...(defaultOptions?.watchQuery ?? {}), fetchPolicy: 'network-only' },
query: { ...(defaultOptions?.query ?? {}), fetchPolicy: 'network-only' },
}}
```
</issue_to_address>
### Comment 3
<location path="apps/ui-staff/.storybook/preview.tsx" line_range="78-82" />
<code_context>
+ },
+ },
+
+ a11y: {
+ // 'todo' - show a11y violations in the test UI only
+ // 'error' - fail CI on a11y violations
+ // 'off' - skip a11y checks entirely
+ test: 'todo',
+ },
+};
</code_context>
<issue_to_address>
**question (bug_risk):** Use of `a11y.test` option may not align with addon-a11y’s expected configuration.
The `a11y` block is using `test: 'todo'`, but `@storybook/addon-a11y` doesn’t expose a `test` option—its config is usually via `config`, `manual`, etc. Unless you have custom wiring that consumes `a11y.test`, this setting will be ignored and you won’t get the intended “todo/error/off” behavior. Please verify the addon’s current API and either update this config or add an adapter that maps `'todo' | 'error' | 'off'` to the real addon options.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
27
to
+28
| const perms = auth?.permissions; | ||
| if (!perms) return []; | ||
| return Object.entries(perms) | ||
| .filter(([, isEnabled]) => isEnabled === true) | ||
| .map(([permKey]) => permKey); | ||
| if (perms) { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Fallback to roles is skipped when permissions exist but have no enabled entries.
With this change, auth.roles is only used when auth.permissions is falsy. If the backend sets permissions to an object (possibly empty, or with all false values) while resolving, perms is truthy and you’ll return an empty array instead of falling back to auth.roles, which can under-authorize users. It’d be safer to detect the "no enabled permissions" case (e.g., after filtering enabled entries) and then fall back to auth.roles instead of returning an empty list.
| const initialEntries = context.parameters?.memoryRouter?.initialEntries ?? ['/']; | ||
| const apolloParams = context.parameters?.apolloClient ?? {}; | ||
| const mocks = apolloParams.mocks ?? apolloMocks; | ||
| const { defaultOptions } = apolloParams; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Merging Apollo defaultOptions overwrites any existing watchQuery/query options from stories.
In MockedProvider, the current spread:
defaultOptions={{
...defaultOptions,
watchQuery: { fetchPolicy: 'network-only' },
query: { fetchPolicy: 'network-only' },
}}overwrites any watchQuery/query options from parameters.apolloClient.defaultOptions (e.g. errorPolicy, pollInterval). To preserve those while enforcing the fetch policy, deep-merge the sub-objects instead:
defaultOptions={{
...defaultOptions,
watchQuery: { ...(defaultOptions?.watchQuery ?? {}), fetchPolicy: 'network-only' },
query: { ...(defaultOptions?.query ?? {}), fetchPolicy: 'network-only' },
}}
Comment on lines
+78
to
+82
| a11y: { | ||
| // 'todo' - show a11y violations in the test UI only | ||
| // 'error' - fail CI on a11y violations | ||
| // 'off' - skip a11y checks entirely | ||
| test: 'todo', |
Contributor
There was a problem hiding this comment.
question (bug_risk): Use of a11y.test option may not align with addon-a11y’s expected configuration.
The a11y block is using test: 'todo', but @storybook/addon-a11y doesn’t expose a test option—its config is usually via config, manual, etc. Unless you have custom wiring that consumes a11y.test, this setting will be ignored and you won’t get the intended “todo/error/off” behavior. Please verify the addon’s current API and either update this config or add an adapter that maps 'todo' | 'error' | 'off' to the real addon options.
added 2 commits
June 17, 2026 23:15
rohit-r-kumar
changed the title
nnoce14
requested changes
Jun 22, 2026
…cellixjs into rohit-r-kumar/issue266
…aml, Also update staff-roles and staff-users storybook
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #266
Summary by Sourcery
Add a cohesive Storybook setup for the staff UI portal and align shared component stories to the new structure.
New Features:
Bug Fixes:
Enhancements:
Build: