feat: hide member filter for users with only MEMBER role#23097
feat: hide member filter for users with only MEMBER role#23097Udit-takkar merged 4 commits intomainfrom
Conversation
- Conditionally disable userId column filter based on isTeamAdminOrOwner flag - Prevents MEMBER-only users from encountering backend permission errors - Users with ADMIN/OWNER role in any team can still access the filter Co-Authored-By: eunjae@cal.com <hey@eunjae.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
WalkthroughThe member (userId) column filter in the bookings listing was changed from always enabled to enabled only for team admins/owners (enableColumnFilter now uses user?.isTeamAdminOrOwner ?? false). Playwright E2E tests were updated: a setup helper distinguishes admin vs non-admin flows, tests were refactored, and an admin-specific test verifies the admin-only Member filter. No other column configurations or exported declarations changed. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
bookings
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/18/25)1 reviewer was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/web/modules/bookings/views/bookings-listing-view.tsx (1)
134-150: GateuserIdsin the TRPC query when the viewer isn’t an admin/ownerPrevent backend permission errors when a non-admin user deep-links or loads a saved segment containing
userIdby only forwardinguserIdsifuser?.isTeamAdminOrOwneris true.Locations to update:
- apps/web/modules/bookings/views/bookings-listing-view.tsx at the TRPC query (around line 134)
Apply this diff:
--- a/apps/web/modules/bookings/views/bookings-listing-view.tsx +++ b/apps/web/modules/bookings/views/bookings-listing-view.tsx @@ -134,7 +134,7 @@ teamIds, - userIds, + userIds: user?.isTeamAdminOrOwner ? userIds : undefined, attendeeName, attendeeEmail, bookingUid,Follow-up tests:
- Load as a MEMBER-only user with
?userId=<some-id>or a saved segment includinguserId→ expect no backend error and results as ifuserIdwasn’t set.- Mixed-role users (Admin in one team, Member in another): ensure filtering by a user from a team where they’re only a Member doesn’t error (or constrain selectable values accordingly).
Optionally, clear any pre-existing
userIdfilter chips/URL params when the viewer isn’t permitted.
🧹 Nitpick comments (3)
apps/web/modules/bookings/views/bookings-listing-view.tsx (3)
289-289: Use strict equality for status comparisonUse
===for consistency and to avoid subtle coercion.- if (status === "recurring" || status == "unconfirmed" || status === "cancelled") { + if (status === "recurring" || status === "unconfirmed" || status === "cancelled") {
320-321: Fix missing useMemo deps to avoid stale “today” grouping and filteringBoth memos depend on
statusand/oruser?.timeZonebut don’t list them, which can produce stale computations when status or timezone changes.- }, [query.data]); + }, [query.data, status, user?.timeZone]);- }, [query.data]); + }, [query.data, user?.timeZone]);Also applies to: 339-339
286-305: Reduce repeat Day.js computations inside loopsHot paths filter/transform bookings per render and call
dayjs()repeatedly. Cache today once in the current timezone to cut work inside loops.Example tweak (outside the selected lines):
const todayStr = useMemo( () => dayjs().tz(user?.timeZone).format("YYYY-MM-DD"), [user?.timeZone] ); // Then compare in filters/maps: dayjs(booking.startTime).tz(user?.timeZone).format("YYYY-MM-DD") !== todayStr // and dayjs(booking.startTime).tz(user?.timeZone).format("YYYY-MM-DD") === todayStrAlso applies to: 322-329
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/modules/bookings/views/bookings-listing-view.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/modules/bookings/views/bookings-listing-view.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/modules/bookings/views/bookings-listing-view.tsx
🧬 Code Graph Analysis (1)
apps/web/modules/bookings/views/bookings-listing-view.tsx (1)
scripts/prepare-local-for-delegation-credentials-testing.js (1)
user(15-17)
🔇 Additional comments (1)
apps/web/modules/bookings/views/bookings-listing-view.tsx (1)
181-187: Member filter gated by isTeamAdminOrOwner — solid, targeted fixUsing
enableColumnFilter: user?.isTeamAdminOrOwner ?? falsecorrectly hides the Member filter for MEMBER-only users and keeps it visible for Admin/Owner users. Defaulting tofalseduring ME loading avoids flicker and accidental exposure.
E2E results are ready! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/playwright/bookings-list.e2e.ts (1)
573-575: Also cover the 'member' search term for non-adminsAs noted above, asserting that “member” yields no results for non-admins would directly validate the admin-only gating.
🧹 Nitpick comments (4)
apps/web/playwright/bookings-list.e2e.ts (4)
490-509: Make roles explicit in setup to avoid flakiness; emulate MEMBER-with-team for non-admin pathRight now, isAdmin=true creates a user with a team but doesn’t explicitly set the role; isAdmin=false creates a user without a team. Since the UI gating depends on isTeamAdminOrOwner, relying on fixture defaults can make these tests flaky and not perfectly representative of the “MEMBER-only across teams” scenario the PR targets.
- Ensure the admin path is deterministically ADMIN/OWNER.
- Ensure the non-admin path is deterministically MEMBER in a team (the real-world case that must hide the filter).
Apply this diff to make the role explicit:
- const user = isAdmin ? await users.create(undefined, { hasTeam: true }) : await users.create(); + const user = isAdmin + ? await users.create(undefined, { hasTeam: true, teamRole: MembershipRole.ADMIN }) + : await users.create(undefined, { hasTeam: true, teamRole: MembershipRole.MEMBER });Optional: reduce flakes by waiting for the “me” query to settle before opening the filter:
// before navigating const meGetResponse = page.waitForResponse((r) => /\/api\/trpc\/viewer\.me\.get/.test(r.url())); // after goto await Promise.all([meGetResponse, bookingsGetResponse]);
510-512: Add an explicit assertion that Member filter is hidden for non-adminThis directly asserts the core requirement for MEMBER-only users.
test("should show all filter items initially and after clearing search", async ({ page, users }) => { await setup({ isAdmin: false, page, users }); + await expect(getFilterItemLocator(page, "add-filter-item-userId")).toBeHidden();
544-546: Strengthen case-insensitive search by asserting all non-member items are hiddenYou already verify two items; complete the coverage for the rest to avoid silent regressions.
test("search should be case-insensitive", async ({ page, users }) => { await setup({ isAdmin: true, page, users }); @@ await expect(getFilterItemLocator(page, "add-filter-item-userId")).toBeVisible(); await expect(getFilterItemLocator(page, "add-filter-item-eventTypeId")).toBeHidden(); await expect(getFilterItemLocator(page, "add-filter-item-teamId")).toBeHidden(); + await expect(getFilterItemLocator(page, "add-filter-item-attendeeName")).toBeHidden(); + await expect(getFilterItemLocator(page, "add-filter-item-attendeeEmail")).toBeHidden(); + await expect(getFilterItemLocator(page, "add-filter-item-dateRange")).toBeHidden();
556-558: Add a dedicated negative test for non-admins searching “member”To explicitly guard the regression this PR addresses, add a non-admin test that proves the Member filter never appears—even when searching for it.
Example:
test("non-admin should not see Member filter (initially or when searching 'member')", async ({ page, users }) => { await setup({ isAdmin: false, page, users }); await expect(getFilterItemLocator(page, "add-filter-item-userId")).toBeHidden(); const searchInput = page.locator(searchInputSelector); await searchInput.fill("member"); await expect(getFilterItemLocator(page, "add-filter-item-userId")).toBeHidden(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/playwright/bookings-list.e2e.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/playwright/bookings-list.e2e.ts
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/playwright/bookings-list.e2e.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
apps/web/playwright/bookings-list.e2e.ts (1)
535-543: LGTM: Admin-only filter visibility coveredPositive path for admins is asserted correctly with a clear, focused check.
4 participants
feat: hide member filter for users with only MEMBER role
Summary
This PR conditionally hides the "Member" filter on the bookings page for users who only have MEMBER role across all their teams. The change prevents MEMBER-only users from encountering backend permission errors when attempting to filter bookings by members.
Key Changes:
userIdcolumn definition inbookings-listing-view.tsxto useenableColumnFilter: user?.isTeamAdminOrOwner ?? falseisTeamAdminOrOwnerflag from the me query to determine filter visibilityContext: The backend already throws permission errors for MEMBER-only users trying to filter by userIds (line 152 in
packages/trpc/server/routers/viewer/bookings/get.handler.ts). This frontend change provides better UX by hiding the filter entirely rather than letting users encounter an error.(↑ that "Member" filter is hidden when the current user is not an owner or an admin.)
Review & Testing Checklist for Human
enableColumnFilter: falseactually hides the Member filter column (not just disables it) for MEMBER-only usersRecommended Test Plan:
/bookingspage as each user typeDiagram
%%{ init : { "theme" : "default" }}%% graph TB BookingsView["apps/web/modules/bookings/views/<br/>bookings-listing-view.tsx"]:::major-edit MeQuery["packages/trpc/server/routers/viewer/<br/>me/get.handler.ts"]:::context BookingsHandler["packages/trpc/server/routers/viewer/<br/>bookings/get.handler.ts"]:::context UseMeQuery["packages/trpc/react/hooks/<br/>useMeQuery.ts"]:::context MeQuery -->|"returns isTeamAdminOrOwner flag"| UseMeQuery UseMeQuery -->|"provides user data"| BookingsView BookingsView -->|"conditionally enables userId filter"| BookingsView BookingsHandler -->|"throws error for MEMBER userIds filter"| BookingsView subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
isTeamAdminOrOwnerflag being computed correctly for all edge casesLink to Devin run: https://app.devin.ai/sessions/ad3dbd91dc8d48c4920f267908c420f0
Requested by: @eunjae-lee