feat(PermissionSessionContext): extract, export, and self-resolve from permissionKey - #1774
feat(PermissionSessionContext): extract, export, and self-resolve from permissionKey#1774rishiraj38 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe change adds the reusable ChangesPermission context UI
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PermissionShield
participant PermissionSessionContext
participant PermissionResolver
PermissionShield->>PermissionSessionContext: Render tooltip content
PermissionSessionContext->>PermissionResolver: Resolve permission when needed
PermissionResolver-->>PermissionSessionContext: Return permission data
PermissionSessionContext-->>PermissionShield: Render permission details
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.tsx (1)
76-89: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
PermissionSessionContextto the custom domain index and bundle it.
src/custom/index.tsexports only the existing custom components and does not include custom permissions. AddPermissionSessionContextandtype PermissionSessionContextPropsto that export, then rebuild sodist/index.d.tsexposes them for consumers importing@sistent/sistent.🤖 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/index.tsx` around lines 76 - 89, Add PermissionSessionContext and type PermissionSessionContextProps to the exports in src/custom/index.ts, then rebuild the package so the generated dist/index.d.ts also exposes both symbols to `@sistent/sistent` consumers.Source: Coding guidelines
🧹 Nitpick comments (4)
src/custom/permissions.tsx (4)
504-506: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
text.secondaryovertext.disabledfor this message in thecardvariant.
palette.subtlemaps totheme.palette.text.disabledfor thecardvariant. That token is intended for disabled affordances and carries low contrast. In thecardvariant this line is informational content on a full page, so low contrast reduces readability. Confirm the contrast ratio of the Sistenttext.disabledtoken againstbackground.paperin both light and dark modes, or switch this line totheme.palette.text.secondary.As per coding guidelines: "Theme-aware UI must use Sistent theme exports and semantic palette tokens rather than raw MUI defaults."
🤖 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/custom/permissions.tsx` around lines 504 - 506, Update the message color in the card variant near the font styling to use the Sistent semantic text.secondary theme token instead of palette.subtle, which resolves to text.disabled. Preserve the existing typography and ensure the color remains theme-aware in both light and dark modes.Source: Coding guidelines
543-554: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMove the event dispatch out of the
setOpenupdater.React may invoke a state updater more than once, including under Strict Mode double-invocation. The
window.dispatchEventcall at line 548 then fires more than once per click. Compute the next value first, then dispatch outside the setter.♻️ Proposed refactor
const handleToggle = (e: React.MouseEvent) => { e.stopPropagation(); - setOpen((prev) => { - const next = !prev; - if (next) { - window.dispatchEvent( - new CustomEvent('permission-shield-opened', { detail: { id: uniqueId } }) - ); - } - return next; - }); + const next = !open; + setOpen(next); + if (next) { + window.dispatchEvent( + new CustomEvent('permission-shield-opened', { detail: { id: uniqueId } }) + ); + } };🤖 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/custom/permissions.tsx` around lines 543 - 554, Update handleToggle so it computes the next open state before calling setOpen, then dispatches permission-shield-opened outside the state updater only when opening. Keep stopPropagation and the existing uniqueId event detail unchanged.Source: Linters/SAST tools
577-598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLet
PermissionShielddelegate key resolution toPermissionSessionContext.
PermissionShieldduplicates the logic already implemented byPermissionSessionContextwhenpermissionKeyis provided. PasspermissionKeyto the child, remove the locals, and remove theuseUnmetPermissionKeys(permissionKey)call at line 537 if it has no other reference in this file.🤖 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/custom/permissions.tsx` around lines 577 - 598, Update PermissionShield so PermissionSessionContext receives permissionKey and performs key resolution itself; remove the duplicated declaredKeys, displayedKeys, combinator, keyNames, subtitle, categories, and subcategories locals. Remove the useUnmetPermissionKeys(permissionKey) call if it has no remaining references in the file, while preserving the existing tooltip rendering.
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
useThemefrom the Sistent theme barrel.
src/theme/index.tsxexportsuseTheme, and nearby custom components import it from that barrel. Keep this component in line by usingimport { useTheme } from '../theme';instead of@mui/material/styles.🤖 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/custom/permissions.tsx` at line 5, Update the useTheme import in the permissions component to use the Sistent theme barrel at ../theme instead of `@mui/material/styles`, matching the import convention used by nearby custom components.Source: Coding guidelines
🤖 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/custom/permissions.tsx`:
- Line 309: Coerce the combined length check in the hasKeys rendering guard to a
boolean so empty categories and subcategories do not render a visible 0.
Preserve rendering when either array contains entries and keep the existing
hasKeys requirement.
- Around line 254-274: Update the key copy control in the map callback to guard
navigator.clipboard availability, handle writeText rejections without unhandled
errors, and make the Box keyboard-operable with appropriate focus and keyboard
activation behavior. Store the pending reset timeout via the suggested copy
timer identifier, clear it before scheduling a new reset, and preserve the
copied-state feedback for successful copies.
---
Outside diff comments:
In `@src/index.tsx`:
- Around line 76-89: Add PermissionSessionContext and type
PermissionSessionContextProps to the exports in src/custom/index.ts, then
rebuild the package so the generated dist/index.d.ts also exposes both symbols
to `@sistent/sistent` consumers.
---
Nitpick comments:
In `@src/custom/permissions.tsx`:
- Around line 504-506: Update the message color in the card variant near the
font styling to use the Sistent semantic text.secondary theme token instead of
palette.subtle, which resolves to text.disabled. Preserve the existing
typography and ensure the color remains theme-aware in both light and dark
modes.
- Around line 543-554: Update handleToggle so it computes the next open state
before calling setOpen, then dispatches permission-shield-opened outside the
state updater only when opening. Keep stopPropagation and the existing uniqueId
event detail unchanged.
- Around line 577-598: Update PermissionShield so PermissionSessionContext
receives permissionKey and performs key resolution itself; remove the duplicated
declaredKeys, displayedKeys, combinator, keyNames, subtitle, categories, and
subcategories locals. Remove the useUnmetPermissionKeys(permissionKey) call if
it has no remaining references in the file, while preserving the existing
tooltip rendering.
- Line 5: Update the useTheme import in the permissions component to use the
Sistent theme barrel at ../theme instead of `@mui/material/styles`, matching the
import convention used by nearby custom components.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e3ec21e2-3fd9-4c74-b553-fd6ffa08a5c5
📒 Files selected for processing (2)
src/custom/permissions.tsxsrc/index.tsx
efb30f7 to
4b8ba8c
Compare
| <PermissionSessionContext | ||
| variant="tooltip" | ||
| displayedKeys={displayedKeys} | ||
| subtitle={subtitle} | ||
| categories={categories} | ||
| subcategories={subcategories} | ||
| /> |
There was a problem hiding this comment.
Now that PermissionSessionContext can self-resolve from permissionKey, should PermissionShield pass permissionKey here instead of re-deriving displayedKeys / subtitle / categories so the tooltip and card paths stay in sync?
There was a problem hiding this comment.
Good catch; yes, that was redundant. Updated PermissionShield to pass permissionKey directly to PermissionSessionContext instead of manually deriving everything. Both the tooltip and card paths now go through the same self-resolving logic, so they'll stay in sync automatically.
…m permissionKey - Extract PermissionSessionContext from PermissionShield into a standalone exported component for reuse in card/full-page layouts (e.g. 403 error pages). - Add self-resolving mode via permissionKey prop — resolves displayedKeys, subtitle, categories, and subcategories internally using useUnmetPermissionKeys, getPermissionKeys, and getPermissionKeyCombinator. - Refactor PermissionShield to delegate key resolution to PermissionSessionContext instead of duplicating it, keeping tooltip and card rendering paths in sync. - Fix accessibility: copy-key control now has role=button, tabIndex=0, and keyboard handler (Enter/Space). - Fix boolean coercion: wrap chip-container length checks with Boolean() to prevent rendering literal '0'. - Export PermissionSessionContext and PermissionSessionContextProps from the root barrel file (src/index.tsx). Signed-off-by: Rishi Raj <rishiraj824@gmail.com> Signed-off-by: Rishi Raj <rishiraj438gt@gmail.com>
4b8ba8c to
c621514
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
rishiraj38
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/custom/permissions.tsx (2)
244-254: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSkip the copy action when the key has no id.
idbecomes''whenkey.idis absent. The handler then writes an empty string and shows "Copied!". Return early instead.♻️ Proposed change
const copyKeyId = (e: React.SyntheticEvent) => { e.stopPropagation(); - const id = key.id || ''; + const id = key.id; + if (!id) return; void navigator.clipboard ?.writeText(id) .then(() => { - setCopiedKeyId(key.id || `#${index}`); + setCopiedKeyId(id); setTimeout(() => setCopiedKeyId(null), 1500); }) .catch(() => undefined); };🤖 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/custom/permissions.tsx` around lines 244 - 254, Update the copyKeyId handler to return immediately when key.id is absent or empty, before invoking navigator.clipboard.writeText or updating copied state. Preserve the existing copy and confirmation behavior for keys with a valid id.
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the Sistent palette token for the card background.
The card background should use the Sistent theme palette rather than the raw MUI
background.paperslot, e.g.theme.palette.background.cardortheme.palette.background.surfaces, so the UI uses the semantic palette token.🤖 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/custom/permissions.tsx` at line 5, Update the card styling in the permissions component to use the Sistent semantic palette token, such as theme.palette.background.card or theme.palette.background.surfaces, instead of the raw MUI theme.palette.background.paper value; preserve the existing card layout and styling behavior.Source: Coding guidelines
🤖 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/custom/permissions.tsx`:
- Around line 552-563: Update handleToggle so it computes the next open state
from the current state before calling setOpen, then dispatches
permission-shield-opened outside the state updater only when opening. Keep
stopPropagation and the existing event detail unchanged.
---
Nitpick comments:
In `@src/custom/permissions.tsx`:
- Around line 244-254: Update the copyKeyId handler to return immediately when
key.id is absent or empty, before invoking navigator.clipboard.writeText or
updating copied state. Preserve the existing copy and confirmation behavior for
keys with a valid id.
- Line 5: Update the card styling in the permissions component to use the
Sistent semantic palette token, such as theme.palette.background.card or
theme.palette.background.surfaces, instead of the raw MUI
theme.palette.background.paper value; preserve the existing card layout and
styling behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cbfa91f-605f-4cdb-8f15-f8bc5b956a42
📒 Files selected for processing (2)
src/custom/permissions.tsxsrc/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.tsx
| const handleToggle = (e: React.MouseEvent) => { | ||
| e.stopPropagation(); | ||
| setOpen((prev) => { | ||
| const next = !prev; | ||
| if (next) { | ||
| window.dispatchEvent( | ||
| new CustomEvent('permission-shield-opened', { detail: { id: uniqueId } }) | ||
| ); | ||
| } | ||
| return next; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Move the event dispatch out of the state updater.
React can invoke the updater more than once. The permission-shield-opened event then fires more than once for one toggle. Compute the next value first, then dispatch outside the setter.
🐛 Proposed fix
const handleToggle = (e: React.MouseEvent) => {
e.stopPropagation();
- setOpen((prev) => {
- const next = !prev;
- if (next) {
- window.dispatchEvent(
- new CustomEvent('permission-shield-opened', { detail: { id: uniqueId } })
- );
- }
- return next;
- });
+ const next = !open;
+ setOpen(next);
+ if (next) {
+ window.dispatchEvent(
+ new CustomEvent('permission-shield-opened', { detail: { id: uniqueId } })
+ );
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleToggle = (e: React.MouseEvent) => { | |
| e.stopPropagation(); | |
| setOpen((prev) => { | |
| const next = !prev; | |
| if (next) { | |
| window.dispatchEvent( | |
| new CustomEvent('permission-shield-opened', { detail: { id: uniqueId } }) | |
| ); | |
| } | |
| return next; | |
| }); | |
| }; | |
| const handleToggle = (e: React.MouseEvent) => { | |
| e.stopPropagation(); | |
| const next = !open; | |
| setOpen(next); | |
| if (next) { | |
| window.dispatchEvent( | |
| new CustomEvent('permission-shield-opened', { detail: { id: uniqueId } }) | |
| ); | |
| } | |
| }; |
🧰 Tools
🪛 React Doctor (0.9.3)
[error] 557-557: This side-effecting call runs inside a state updater, which React may invoke more than once. Move it outside the setter after computing the next state.
React may replay a state updater, so callbacks, analytics, and persistence inside it can run more than once. Compute state purely, then perform the side effect outside the setter.
(no-side-effect-in-state-updater-function)
🤖 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/custom/permissions.tsx` around lines 552 - 563, Update handleToggle so it
computes the next open state from the current state before calling setOpen, then
dispatches permission-shield-opened outside the state updater only when opening.
Keep stopPropagation and the existing event detail unchanged.
Source: Linters/SAST tools
|
Really liked the direction of this refactor. one thing I was curious about while reading it: pages in Meshery still perform their own useHasPermission(...) checks before rendering the error state. so will we continue with it to stay as the current pattern, or do you think it could eventually be abstracted further? that was one of the things I saw earlier, so wanted to understand/ learn from how you think about the long-term architectures. |
3 participants
Description
Extracts
PermissionSessionContextfromPermissionShieldinto a standalone, exported component so that permission failure details (required keys, function names, descriptions, and user/org/role context) can be rendered in any layout — tooltips, cards, and full-page 403 error pages — from a single source of truth.What changed
Self-resolving
permissionKeypropPermissionSessionContextnow accepts an optionalpermissionKeyprop. When provided, the component internally resolvesdisplayedKeys,subtitle,categories, andsubcategoriesusing the same hooks asPermissionShield(useUnmetPermissionKeys,getPermissionKeys,getPermissionKeyCombinator). This eliminates the need for callers to manually derive and pass these values.PermissionShieldrefactored to delegatePermissionShieldpreviously duplicated the full key-resolution logic before passing pre-resolved props toPermissionSessionContext. It now passespermissionKeydirectly, keeping the tooltip and card rendering paths in sync through a single resolution path.Accessibility improvements
role="button",tabIndex={0}, and keyboard handler (Enter/Space).navigator.clipboard.writeText()with.catch()fallback.Boolean coercion fix
Boolean()to prevent React from rendering the literal string"0".Barrel export
PermissionSessionContextandPermissionSessionContextPropsexported fromsrc/index.tsx.Two usage modes
permissionKey={Keys.X}PermissionShieldtooltip — pass the key, get the full UIdisplayedKeys={[...]}Related Issues / PRs
PermissionSessionContextinDefaultError403 pagesType of Change
Screen.Recording.2026-08-05.at.2.30.03.PM.mov