Forbedre visning av arv på enkelt action - #1971
Conversation
📝 WalkthroughWalkthroughConsolidates inheritance metadata into a new internal chip-right shape, updates mapping and chip construction to carry Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx (1)
120-147:⚠️ Potential issue | 🟡 MinorInclude
toPartyin the dependency list to avoid stale inheritance context.
toPartyis used to computeinheritedStatusand to populate the chip payload, but the effect won’t rerun if onlytoPartychanges without other dependencies. This can leave stale user/via context in the UI.🔧 Suggested fix
- }, [delegationCheckedActions, resource.identifier, hasAccess, currentRights, resourceRights]); + }, [ + delegationCheckedActions, + resource.identifier, + hasAccess, + currentRights, + resourceRights, + toParty, + ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx` around lines 120 - 147, The effect inside useRightsSection is missing toParty from its dependency array, causing stale inheritedStatus and chip payloads when toParty changes; update the dependency list for the useEffect that setsMissingAccess and setRights (the block that maps delegationCheckedActions -> mapRightsToChipRights and computes inheritedStatus via findInheritedRight) to include toParty so the effect re-runs whenever toParty changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx`:
- Around line 120-147: The effect inside useRightsSection is missing toParty
from its dependency array, causing stale inheritedStatus and chip payloads when
toParty changes; update the dependency list for the useEffect that
setsMissingAccess and setRights (the block that maps delegationCheckedActions ->
mapRightsToChipRights and computes inheritedStatus via findInheritedRight) to
include toParty so the effect re-runs whenever toParty changes.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/features/amUI/common/DelegationModal/SingleRights/hooks/rightsUtils.tsxsrc/features/amUI/common/DelegationModal/SingleRights/hooks/useRightChips.tsxsrc/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsxsrc/localizations/en.jsonsrc/localizations/no_nb.jsonsrc/localizations/no_nn.json
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx (2)
107-118:findInheritedRightcloses overtoPartybut is missing from theuseEffectdependency array.
findInheritedRightis a plain (unstable) function defined in the component body that capturestoPartyvia closure. It's called inside theuseEffectat line 131, but it isn't listed as a dependency. React'sexhaustive-depsrule will flag this.Functionally it's correct today — because
toPartyis already in the deps array — but it's fragile: any future change to the closure that adds a new captured variable won't be caught automatically.Cleanest fix is to move the helper inside the effect (it's only used there):
♻️ Move helper inside the useEffect
- const findInheritedRight = (right: ResourceRight, ruleKey: string) => { - const rule = right.indirectRules.find((r) => r.rule.key === ruleKey); - // if rule is found, this is inherited. - if (rule) { - return getInheritedStatus({ - permissions: rule?.permissions, - toParty: toParty, - }); - } - return []; - }; useEffect(() => { if (delegationCheckedActions) { setMissingAccess(getMissingAccessMessage(delegationCheckedActions)); + + const findInheritedRight = (right: ResourceRight, ruleKey: string) => { + const rule = right.indirectRules.find((r) => r.rule.key === ruleKey); + if (rule) { + return getInheritedStatus({ permissions: rule.permissions, toParty }); + } + return []; + }; if (hasAccess && resourceRights) {Also note line 113:
rule?.permissions— the optional chain is redundant inside theif (rule)guard;rule.permissionsis sufficient.Also applies to: 147-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx` around lines 107 - 118, findInheritedRight currently closes over toParty and is used only inside the useEffect, but it isn’t included in the deps array which is fragile; move the findInheritedRight helper into the same useEffect where it’s used (the effect that reads rights and calls getInheritedStatus) so it no longer closes over toParty, and update its body to use rule.permissions (drop the redundant optional chaining rule?.permissions inside the if (rule) guard); apply the same change for the similar helper usage around lines 147-154 to avoid unstable closures and dependency issues.
130-130: Minor: preferArray.includesoverArray.somefor simple equality checks.♻️ Readability nit
- isChecked: currentRights.some((key) => key === right.rule.key), + isChecked: currentRights.includes(right.rule.key),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx` at line 130, Replace the equality check using Array.some with Array.includes for readability: in useRightsSection.tsx update the isChecked assignment that currently uses currentRights.some((key) => key === right.rule.key) to use currentRights.includes(right.rule.key) instead (references: currentRights, right.rule.key, isChecked).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx`:
- Around line 107-118: findInheritedRight currently closes over toParty and is
used only inside the useEffect, but it isn’t included in the deps array which is
fragile; move the findInheritedRight helper into the same useEffect where it’s
used (the effect that reads rights and calls getInheritedStatus) so it no longer
closes over toParty, and update its body to use rule.permissions (drop the
redundant optional chaining rule?.permissions inside the if (rule) guard); apply
the same change for the similar helper usage around lines 147-154 to avoid
unstable closures and dependency issues.
- Line 130: Replace the equality check using Array.some with Array.includes for
readability: in useRightsSection.tsx update the isChecked assignment that
currently uses currentRights.some((key) => key === right.rule.key) to use
currentRights.includes(right.rule.key) instead (references: currentRights,
right.rule.key, isChecked).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx (1)
108-150:⚠️ Potential issue | 🟡 MinorAdd
getMissingAccessMessageto the effect dependencies.When
t,reportee?.name, orresource?.resourceOwnerNamechange, the callback is recreated but the effect won't re-run, causing stale messages to display. Include the callback in the dependency array to ensure the effect executes whenever the callback updates.🔧 Suggested fix
}, [ delegationCheckedActions, resource.identifier, hasAccess, currentRights, resourceRights, toParty, + getMissingAccessMessage, ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx` around lines 108 - 150, The effect updating missing access and rights should include the getMissingAccessMessage callback in its dependency array so it re-runs when the callback (which depends on t, reportee?.name, or resource?.resourceOwnerName) changes; update the useEffect dependencies to include getMissingAccessMessage alongside delegationCheckedActions, resource.identifier, hasAccess, currentRights, resourceRights, and toParty so that setMissingAccess(getMissingAccessMessage(...)) always uses the current callback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx`:
- Around line 108-150: The effect updating missing access and rights should
include the getMissingAccessMessage callback in its dependency array so it
re-runs when the callback (which depends on t, reportee?.name, or
resource?.resourceOwnerName) changes; update the useEffect dependencies to
include getMissingAccessMessage alongside delegationCheckedActions,
resource.identifier, hasAccess, currentRights, resourceRights, and toParty so
that setMissingAccess(getMissingAccessMessage(...)) always uses the current
callback.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/features/amUI/common/DelegationModal/SingleRights/hooks/useRightsSection.tsx
|
@mgunnerud |
2 participants
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Localization