Skip to content

Comments

fix: nits addressed from PBAC walk through with Unkey#24244

Merged
keithwillcode merged 1 commit intomainfrom
fix/pbac-unkey-nits
Oct 3, 2025
Merged

fix: nits addressed from PBAC walk through with Unkey#24244
keithwillcode merged 1 commit intomainfrom
fix/pbac-unkey-nits

Conversation

@sean-brydon
Copy link
Member

What does this PR do?

Walked @chronark through PBAC as a new user - UX issues found upon testing

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 3, 2025

Walkthrough

  • Updated RoleSheet.tsx to enforce a 50-character limit on role name input, add an id for the advanced mode checkbox, and replace a span with a label linked via htmlFor for accessibility.
  • Added en/common.json string role_name_required and modified confirm_delete_role to include a {{roleName}} placeholder.
  • Extended server-side role name validation in pbac router to require 1–50 characters.

Possibly related PRs

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title refers to internal process details and uses non-descriptive terms like “nits” and “PBAC walk through,” but it does not summarize the primary changes made, such as enforcing input limits or improving accessibility, making it too vague to convey the main content of the PR. Please revise the title to clearly and concisely describe the key change, for example “Enforce 50-char limit on role names and improve advanced mode accessibility.”
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed The description, while brief, clearly states that the PR addresses UX issues discovered during a PBAC walkthrough, which is directly related to the changes made in the code.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/pbac-unkey-nits

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c11814c and 808954e.

📒 Files selected for processing (3)
  • apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx (3 hunks)
  • apps/web/public/static/locales/en/common.json (2 hunks)
  • packages/trpc/server/routers/viewer/pbac/_router.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:

  • packages/trpc/server/routers/viewer/pbac/_router.tsx
  • apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.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:

  • packages/trpc/server/routers/viewer/pbac/_router.tsx
  • apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
**/*.{ts,tsx,js,jsx}

⚙️ CodeRabbit configuration file

Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.

Files:

  • packages/trpc/server/routers/viewer/pbac/_router.tsx
  • apps/web/app/(use-page-wrapper)/settings/(settings-layout)/organizations/roles/_components/RoleSheet.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Install dependencies / Yarn install & cache

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@graphite-app graphite-app bot requested a review from a team October 3, 2025 10:33
@keithwillcode keithwillcode added consumer core area: core, team members only labels Oct 3, 2025
@dosubot dosubot bot added the 🧹 Improvements Improvements to existing features. Mostly UX/UI label Oct 3, 2025
const roleInputSchema = z.object({
teamId: z.number(),
name: z.string().min(1),
name: z.string().min(1).max(50),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image

Stops people from doing stuid things :D

Copy link
Contributor

@chronark chronark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks <3

@keithwillcode keithwillcode enabled auto-merge (squash) October 3, 2025 10:39
@keithwillcode keithwillcode merged commit 3e31945 into main Oct 3, 2025
97 of 105 checks passed
@keithwillcode keithwillcode deleted the fix/pbac-unkey-nits branch October 3, 2025 11:03
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2025

E2E results are ready!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

consumer core area: core, team members only 🧹 Improvements Improvements to existing features. Mostly UX/UI ready-for-e2e size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants