Create Rev79 Id input fields for Projects and Engagements#1776
Create Rev79 Id input fields for Projects and Engagements#1776
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Rev79 support: exposes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.graphql (2)
44-48: Consider fetchingcanReadforusesRev79to enable permission-aware rendering.Currently only
valueis fetched. If the user lacks permission to read this field, the UI won't be able to distinguish between "usesRev79 is false" and "user can't read usesRev79".parent { usesRev79 { + canRead value } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.graphql` around lines 44 - 48, The GraphQL fragment currently fetches only parent.usesRev79.value which prevents distinguishing between a false value and lack of read permission; update the fragment to also request the permission field (e.g., parent.usesRev79.canRead) so the UI can perform permission-aware rendering—locate the fragment that references usesRev79 and add fetching of canRead alongside value, then update any consuming code (renderers that read parent.usesRev79.value) to check canRead before interpreting the value.
49-55: Redundant inline fragment.The fragment is already defined
on LanguageEngagement, so the... on LanguageEngagementinline fragment is unnecessary.♻️ Proposed simplification
- ... on LanguageEngagement { - rev79CommunityId { - canRead - canEdit - value - } + rev79CommunityId { + canRead + canEdit + value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.graphql` around lines 49 - 55, The inline fragment "... on LanguageEngagement" is redundant because the surrounding selection is already on the LanguageEngagement type; remove the inline fragment wrapper and place the rev79CommunityId selection (canRead, canEdit, value) directly under the LanguageEngagement selection so the rev79CommunityId field is selected without the unnecessary "... on LanguageEngagement" wrapper.src/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.tsx (1)
176-189: Consider handling the case whereusesRev79is not readable.The conditional
engagement.parent.usesRev79.valueaccesses.valuedirectly without checkingcanRead. If the user doesn't have permission to readusesRev79, this could result in rendering nothing (which may be the intended behavior) or potentially accessing an undefined value depending on how the API returns non-readable fields.Consider adding a
canReadcheck for defensive coding:- {engagement.parent.usesRev79.value && ( + {engagement.parent.usesRev79.canRead && engagement.parent.usesRev79.value && (Note: The Biome lint warning about
childrenprop is a false positive here—DataButtonexplicitly acceptschildrenas a prop (either a render function or ReactNode) per the component's interface insrc/components/DataButton/DataButton.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.tsx` around lines 176 - 189, The conditional should guard reading the field by checking read permission before accessing .value: update the condition that currently uses engagement.parent.usesRev79.value to verify engagement.parent.usesRev79.canRead (and then .value) so you only access the value when readable; apply the same defensive pattern around rendering the DataButton (which uses rev79CommunityId and show) to ensure you don't dereference a non-readable field (leave DataButton's children usage as-is since it's an accepted prop).src/scenes/Projects/Update/UpdateProjectDialog.tsx (1)
88-97: Consider hidingusesRev79when user lacks edit permission rather than showing it disabled.The
usesRev79checkbox is disabled when!project.rev79ProjectId.canEdit, but it's still rendered. This differs from the pattern fordepartmentId(lines 98-102) which returnsnullwhen the user can't edit. For consistency and cleaner UX, consider not rendering the checkbox at all if the user can't edit Rev79 fields.♻️ Proposed fix
usesRev79: ({ project }) => ( + !project.rev79ProjectId.canEdit ? null : ( <CheckboxField name="usesRev79" label="Uses Rev79" - disabled={!project.rev79ProjectId.canEdit} /> + ) ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Projects/Update/UpdateProjectDialog.tsx` around lines 88 - 97, The usesRev79 checkbox is rendered disabled when the user lacks permission but should be hidden like departmentId; update the usesRev79 renderer (the usesRev79 function) to return null when project.rev79ProjectId.canEdit is false instead of rendering a disabled CheckboxField, matching the departmentId pattern and improving UX—keep the rev79ProjectId TextField behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scenes/Projects/Overview/ProjectOverview.tsx`:
- Around line 442-454: The DataButton for the Rev79 Project ID uses the wrong
permission field: change the secured prop from project.usesRev79 to
project.rev79ProjectId so read/redaction checks use the actual field; update the
secured prop on the DataButton inside ProjectOverview (component rendering Rev79
Project ID) to reference project.rev79ProjectId (keep other props like label,
redacted, children, and onClick unchanged).
---
Nitpick comments:
In
`@src/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.graphql`:
- Around line 44-48: The GraphQL fragment currently fetches only
parent.usesRev79.value which prevents distinguishing between a false value and
lack of read permission; update the fragment to also request the permission
field (e.g., parent.usesRev79.canRead) so the UI can perform permission-aware
rendering—locate the fragment that references usesRev79 and add fetching of
canRead alongside value, then update any consuming code (renderers that read
parent.usesRev79.value) to check canRead before interpreting the value.
- Around line 49-55: The inline fragment "... on LanguageEngagement" is
redundant because the surrounding selection is already on the LanguageEngagement
type; remove the inline fragment wrapper and place the rev79CommunityId
selection (canRead, canEdit, value) directly under the LanguageEngagement
selection so the rev79CommunityId field is selected without the unnecessary "...
on LanguageEngagement" wrapper.
In
`@src/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.tsx`:
- Around line 176-189: The conditional should guard reading the field by
checking read permission before accessing .value: update the condition that
currently uses engagement.parent.usesRev79.value to verify
engagement.parent.usesRev79.canRead (and then .value) so you only access the
value when readable; apply the same defensive pattern around rendering the
DataButton (which uses rev79CommunityId and show) to ensure you don't
dereference a non-readable field (leave DataButton's children usage as-is since
it's an accepted prop).
In `@src/scenes/Projects/Update/UpdateProjectDialog.tsx`:
- Around line 88-97: The usesRev79 checkbox is rendered disabled when the user
lacks permission but should be hidden like departmentId; update the usesRev79
renderer (the usesRev79 function) to return null when
project.rev79ProjectId.canEdit is false instead of rendering a disabled
CheckboxField, matching the departmentId pattern and improving UX—keep the
rev79ProjectId TextField behavior unchanged.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/scenes/Engagement/EditEngagement/EditEngagementDialog.tsxsrc/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.graphqlsrc/scenes/Engagement/LanguageEngagement/Header/LanguageEngagementHeader.tsxsrc/scenes/Engagement/LanguageEngagement/LanguageEngagementDetail.graphqlsrc/scenes/Projects/Overview/ProjectOverview.graphqlsrc/scenes/Projects/Overview/ProjectOverview.tsxsrc/scenes/Projects/Update/UpdateProjectDialog.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/scenes/Projects/Update/UpdateProjectDialog.tsx (1)
17-17: Use the canonicalLinkimport pathPlease import
Linkfrom~/components/Routing/Linkinstead of~/components/Routingto match the repo rule.As per coding guidelines: "Use
~/components/Routing/Linkfor link components — never MUI'sLinkor React Router'sLinkdirectly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scenes/Projects/Update/UpdateProjectDialog.tsx` at line 17, The import for Link in UpdateProjectDialog.tsx should use the canonical path; replace the current import "import { Link } from '~/components/Routing';" with a default or named import from '~/components/Routing/Link' so the UpdateProjectDialog component (referencing Link) uses the repo-standard Link export instead of importing from the barrel; update any references to Link in this file to match the exported shape from '~/components/Routing/Link'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scenes/Projects/Update/UpdateProjectDialog.tsx`:
- Around line 91-97: The usesRev79 CheckboxField in UpdateProjectDialog is being
disabled via project.rev79ProjectId.canEdit but is still rendered outside the
SecuredField pattern, allowing the field to be visible/editable when permissions
differ; wrap the usesRev79 field rendering with the SecuredField component (or
conditionally render it using the same permission check) so that
CheckboxField(name="usesRev79", label="Uses Rev79") is only mounted when
project.rev79ProjectId.canEdit is true, mirroring the secured-field pattern used
elsewhere in this file.
---
Nitpick comments:
In `@src/scenes/Projects/Update/UpdateProjectDialog.tsx`:
- Line 17: The import for Link in UpdateProjectDialog.tsx should use the
canonical path; replace the current import "import { Link } from
'~/components/Routing';" with a default or named import from
'~/components/Routing/Link' so the UpdateProjectDialog component (referencing
Link) uses the repo-standard Link export instead of importing from the barrel;
update any references to Link in this file to match the exported shape from
'~/components/Routing/Link'.
brentkulwicki
left a comment
brentkulwicki
left a comment
There was a problem hiding this comment.
I did not pull and test locally, but everything makes sense and I have no issues when looking at the code. ✅
2 participants
API PR: SeedCompany/cord-api-v3#3666
This adds a "uses Rev79" checkbox to the Project edit for Momentum Projects. If true, a Rev79 project id and engagement community id input will be rendered.