Expose project members in project list modal#1258
Conversation
Motivation: Guests and members should be able to see who to contact for role changes without visiting settings. Modifications: - Add a "View members" action in the projects list - Show owners and members in a modal with styled lists and icons - Add an E2E test to verify the members modal Result: Project owners and members are visible from the project list.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ProjectOwnersModal and integrates it into the Projects table (View members button), introduces a Playwright test for project members, and updates an existing landing e2e test to wait for the login response and adjust navigation/search triggers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Frontend as Webapp
participant API as Server/API
User->>Browser: Click "View members"
Browser->>Frontend: request open modal with projectName
Frontend->>API: GET /api/v1/projects/{projectName} (metadata)
API-->>Frontend: project metadata (owners, members)
Frontend-->>Browser: render ProjectOwnersModal with lists
Browser-->>User: display "Project members" dialog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
|
Screenshots: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@webapp/e2e/project-owners.spec.ts`:
- Line 53: The timeout option is incorrectly passed as the second argument to
expect(...) instead of to the assertion; update the call that uses
dialog.getByTestId('project-member-login').first() so that expect(...) is called
without the timeout argument and the timeout is provided to toBeVisible (i.e.,
call expect(dialog.getByTestId('project-member-login').first()).toBeVisible({
timeout: 15000 })), keeping the same element selection and timeout value.
In `@webapp/src/dogma/features/project/ProjectOwnersModal.tsx`:
- Around line 46-49: The modal currently shows "Loading members..." forever
because it only checks isMetadataLoading or !ownersMetadata; update
ProjectOwnersModal to handle query failures from
useGetMetadataByProjectNameQuery by reading the isError (and optionally error)
return value and rendering an error state when isError is true (e.g., show
"Failed to load members" with error.message and a retry action). Replace the JSX
branch that uses {isMetadataLoading || !ownersMetadata ? ...} with logic that
first checks isMetadataLoading, then checks isError to display the error UI (and
call refetch on retry), and only then falls back to the empty owners message or
the loaded ownersMetadata.
In `@webapp/src/dogma/features/project/Projects.tsx`:
- Around line 143-163: The Members column uses columnHelper.accessor((row:
ProjectDto) => row.name) without an explicit id, causing potential TanStack
Table column ID conflicts; update the accessor call for the Members column (the
one returning the View members Button that calls setOwnersProjectName and
onOpen) to include a unique id (e.g., id: 'members') in the column definition so
the table won't derive a duplicate ID from the accessor; also scan the other
columns using the same accessor (the Name and Action columns) and add explicit
ids there as well if not already present.
🧹 Nitpick comments (5)
webapp/e2e/landing.spec.ts (1)
9-11: Inconsistent variable naming across test files.This variable is named
loginResponse, while the same pattern inproject-owners.spec.ts(line 9) usesloginResponsePromise. The latter is more accurate sincepage.waitForResponse()returns aPromise. Consider aligning the naming for consistency.Proposed fix
- const loginResponse = page.waitForResponse((response) => response.url().includes('/api/v1/login')); + const loginResponsePromise = page.waitForResponse((response) => response.url().includes('/api/v1/login')); await page.getByRole('button', { name: 'Login' }).click(); - await loginResponse; + await loginResponsePromise;webapp/e2e/project-owners.spec.ts (1)
3-12: Duplicated loginbeforeEachblock across test files.The login logic is identical to
landing.spec.ts. Consider extracting a shared login helper to reduce duplication if more E2E test files are planned.webapp/src/dogma/features/project/ProjectOwnersModal.tsx (2)
50-52: Redundantowners.length > 0guard.The outer ternary on line 48 already handles the
owners.length === 0case, so the{owners.length > 0 && (...)}check on line 52 is always true here. It can be removed to reduce nesting.
55-63: Consider using ChakraIconcomponent instead of inline styles.The
FaCrownandFaUserGroupicons use rawstyleobjects for layout/color. Chakra'sIconcomponent (or wrapping in aBox) with Chakra style props would be more consistent with the rest of the component's styling approach.Example
import { Icon } from '@chakra-ui/react'; <Icon as={FaCrown} mr={2} color="blue.500" verticalAlign="text-bottom" />Also applies to: 84-92
webapp/src/dogma/features/project/Projects.tsx (1)
66-66:columnHelpercreated every render defeatsuseMemo.
createColumnHelper<ProjectDto>()on line 66 returns a new object each render. Since it's in theuseMemodependency array (line 202), the columns are recomputed every render, negating the memoization. This is pre-existing, but worth noting — consider movingcolumnHelperoutside the component or to a module-level constant.Proposed fix
Move the helper outside the component:
+const columnHelper = createColumnHelper<ProjectDto>(); + export const Projects = () => { - const columnHelper = createColumnHelper<ProjectDto>(); const dispatch = useAppDispatch();Then remove
columnHelperfrom theuseMemodeps:- [columnHelper, onOpen, user], + [onOpen, user],Also applies to: 202-202
Motivation: Tighten table column ids, error handling, and test assertions. Modifications: - Add explicit column ids for Name/Members/Action - Fix expect timeout usage in E2E - Add error state with retry to members modal Result: Review feedback is addressed with safer UI and tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webapp/src/dogma/features/project/ProjectOwnersModal.tsx`:
- Around line 52-65: The current conditional returns "System administrators"
whenever owners.length === 0, which hides regular members; update the render
logic in ProjectOwnersModal (the JSX that uses isMetadataLoading, isError,
owners and members) so that it only shows the "System administrators" text when
both owners.length === 0 AND members.length === 0, and otherwise proceed to
render the members list (and any owners if present); keep existing error/retry
(isError/refetch) and loading (isMetadataLoading) branches unchanged.
🧹 Nitpick comments (3)
webapp/src/dogma/features/project/ProjectOwnersModal.tsx (2)
66-95: Redundantowners.length > 0guard.The outer ternary (line 63) already ensures
owners.length !== 0before reaching this branch, so theowners.length > 0check on line 67 is always true. Removing it reduces a layer of nesting.
70-78: Hardcoded hex colors bypass Chakra's theme tokens.
'#3182ce'and'#38a169'happen to match Chakra's defaultblue.500andgreen.500, but they won't follow theme overrides or color-mode changes. Consider using the theme tokens (e.g., viauseColorModeValueoruseToken) for consistency with thesxmarker colors already using'blue.500'/'green.500'on lines 86 and 115.Also applies to: 100-106
webapp/src/dogma/features/project/Projects.tsx (1)
89-206:columnHelperis recreated every render, defeatinguseMemo.
createColumnHelper<ProjectDto>()on line 66 produces a new object each render, socolumnHelperin the dependency array (line 205) always triggers recomputation. This is pre-existing, but now that the dependency list is being touched anyway, it might be worth hoisting it outside the component or wrapping it in its ownuseMemo.Hoist columnHelper outside the component
+const columnHelper = createColumnHelper<ProjectDto>(); + export const Projects = () => { - const columnHelper = createColumnHelper<ProjectDto>(); const dispatch = useAppDispatch();Then remove
columnHelperfrom theuseMemodeps on line 205.
Motivation: Tighten table column ids, error handling, and test assertions. Modifications: - Add explicit column ids for Name/Members/Action - Fix expect timeout usage in E2E - Add error state with retry to members modal Result: Review feedback is addressed with safer UI and tests.
…aldogma into codex/project-members-modal
| } | ||
|
|
||
| export const ProjectOwnersModal = ({ projectName, isOpen, onClose }: ProjectOwnersModalProps) => { | ||
| const { data, isLoading, isError, error } = useGetMetadataByProjectNameQuery(projectName ?? '', { |
There was a problem hiding this comment.
Question) This API seems to require @RequiresProjectRole(ProjectRole.MEMBER) - will GUEST/users who are not members of a project be able to view members?
There was a problem hiding this comment.
I missed that point. 😅
In production, GUEST is not an anonymous user but may be authenticated by a third party IdP such as Okta. Therefore, it makes sense to allow GUEST to access the project metadata.
There was a problem hiding this comment.
We might need to add another api that only provides the member information later. I think it's okay for now.
There was a problem hiding this comment.
Agreed. We can consider adding additional APIs when we have a chance.
| } | ||
|
|
||
| export const ProjectOwnersModal = ({ projectName, isOpen, onClose }: ProjectOwnersModalProps) => { | ||
| const { data, isLoading, isError, error } = useGetMetadataByProjectNameQuery(projectName ?? '', { |
There was a problem hiding this comment.
We might need to add another api that only provides the member information later. I think it's okay for now.
3 participants
Motivation:
Guests and members should be able to see who to contact for role changes
without visiting settings.
Modifications:
Result: