refactor(platform): clean up route pending states and ability ref#541
Conversation
Remove unnecessary pendingComponent/pendingMs overrides from data table routes now that tables load their parts independently. Simplify the ability ref in dashboard layout to use a single ref object tracking both role and ability, and tighten defineAbilityFor parameter type.
Greptile SummaryCleaned up route pending states following PR #540's independent data table loading, and refactored the dashboard layout's ability ref to track both role and ability in a single object instead of using two separate refs.
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| services/platform/app/routes/dashboard/$id.tsx | Refactored ability ref to track both role and ability in single object, but has a TypeScript type error in ref initialization |
| services/platform/lib/permissions/ability.ts | Tightened parameter type from `string |
Last reviewed commit: e208153
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const abilityRef = useRef<AppAbility>(defineAbilityFor(memberContext?.role)); | ||
| const lastRoleRef = useRef<string | undefined>(memberContext?.role); | ||
| const abilityRef = useRef<{ role: string | null; ability: AppAbility }>(null); |
There was a problem hiding this comment.
type mismatch between generic parameter and initial value - ref type doesn't allow null but is initialized with null
| const abilityRef = useRef<{ role: string | null; ability: AppAbility }>(null); | |
| const abilityRef = useRef<{ role: string | null; ability: AppAbility } | null>(null); |
📝 WalkthroughWalkthroughThis pull request refactors role and permissions management in the dashboard root route by replacing per-context recomputation with a single ref object containing Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/platform/app/routes/dashboard/`$id.tsx:
- Around line 37-45: TypeScript may not narrow abilityRef.current after the
in-render assignment, so update the destructure to use a non-null assertion and
remove the unused role variable: replace "const { role, ability } =
abilityRef.current" with "const { ability } = abilityRef.current!" (or change
the useRef typing to be non-nullable), and compute hasRole from the available
value (e.g., "const hasRole = abilityRef.current!.role !== null" or "const
hasRole = !!ability") to avoid the potential null typing error and remove the
unused role binding; keep defineAbilityFor and the lazy assignment logic as-is.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
services/platform/app/routes/dashboard/$id.tsxservices/platform/app/routes/dashboard/$id/_knowledge/customers.tsxservices/platform/app/routes/dashboard/$id/_knowledge/products.tsxservices/platform/app/routes/dashboard/$id/_knowledge/tone-of-voice.tsxservices/platform/app/routes/dashboard/$id/_knowledge/vendors.tsxservices/platform/app/routes/dashboard/$id/_knowledge/websites.tsxservices/platform/app/routes/dashboard/$id/automations/index.tsxservices/platform/app/routes/dashboard/$id/custom-agents/index.tsxservices/platform/app/routes/dashboard/$id/settings/teams.tsxservices/platform/lib/permissions/ability.ts
💤 Files with no reviewable changes (8)
- services/platform/app/routes/dashboard/$id/_knowledge/vendors.tsx
- services/platform/app/routes/dashboard/$id/_knowledge/websites.tsx
- services/platform/app/routes/dashboard/$id/custom-agents/index.tsx
- services/platform/app/routes/dashboard/$id/settings/teams.tsx
- services/platform/app/routes/dashboard/$id/_knowledge/tone-of-voice.tsx
- services/platform/app/routes/dashboard/$id/_knowledge/products.tsx
- services/platform/app/routes/dashboard/$id/_knowledge/customers.tsx
- services/platform/app/routes/dashboard/$id/automations/index.tsx
| if (!abilityRef.current || abilityRef.current.role !== currentRole) { | ||
| abilityRef.current = { | ||
| role: currentRole, | ||
| ability: defineAbilityFor(currentRole), | ||
| }; | ||
| } | ||
|
|
||
| const hasRole = !!memberContext?.role; | ||
| const { role, ability } = abilityRef.current; | ||
| const hasRole = role !== null; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The during-render ref mutation pattern is valid, but confirm TypeScript narrows correctly after the assignment.
The lazy-initialization pattern (writing to a ref during render when the value is stale) is a supported React idiom. However, TypeScript does not narrow abilityRef.current through the ref assignment inside the if-block, so line 44 const { role, ability } = abilityRef.current may require a non-null assertion (abilityRef.current!) depending on whether the ref type includes null (see the useRef typing issue above).
Additionally, role on line 44 is only ever consumed to compute hasRole on line 45 and is never referenced elsewhere in this function. You can simplify:
♻️ Minor cleanup suggestion
- const { role, ability } = abilityRef.current;
- const hasRole = role !== null;
+ const { ability } = abilityRef.current!;
+ const hasRole = abilityRef.current!.role !== null;📝 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.
| if (!abilityRef.current || abilityRef.current.role !== currentRole) { | |
| abilityRef.current = { | |
| role: currentRole, | |
| ability: defineAbilityFor(currentRole), | |
| }; | |
| } | |
| const hasRole = !!memberContext?.role; | |
| const { role, ability } = abilityRef.current; | |
| const hasRole = role !== null; | |
| if (!abilityRef.current || abilityRef.current.role !== currentRole) { | |
| abilityRef.current = { | |
| role: currentRole, | |
| ability: defineAbilityFor(currentRole), | |
| }; | |
| } | |
| const { ability } = abilityRef.current!; | |
| const hasRole = abilityRef.current!.role !== null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/platform/app/routes/dashboard/`$id.tsx around lines 37 - 45,
TypeScript may not narrow abilityRef.current after the in-render assignment, so
update the destructure to use a non-null assertion and remove the unused role
variable: replace "const { role, ability } = abilityRef.current" with "const {
ability } = abilityRef.current!" (or change the useRef typing to be
non-nullable), and compute hasRole from the available value (e.g., "const
hasRole = abilityRef.current!.role !== null" or "const hasRole = !!ability") to
avoid the potential null typing error and remove the unused role binding; keep
defineAbilityFor and the lazy assignment logic as-is.
1 participant
Remove unnecessary pendingComponent/pendingMs overrides from data table routes now that tables load their parts independently. Simplify the ability ref in dashboard layout to use a single ref object tracking both role and ability, and tighten defineAbilityFor parameter type.
Summary by CodeRabbit