-
-
Notifications
You must be signed in to change notification settings - Fork 833
chore(webapp) adds shortcut key for admin area #2570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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. Tip 🧪 Early access (models): enabledWe are currently testing Sonnet 4.5 code review models, which should lead to better review quality. However, this model may result in higher noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/components/primitives/Buttons.tsx (1)
347-355
: Parity with Button: consider preventing default/propagationFor LinkButton’s shortcut action you don’t
preventDefault
/stopPropagation
, unlike Button. It’s usually harmless, but aligning behaviors reduces surprises if global handlers exist. Optionally:- action: () => { + action: (e) => { if (innerRef.current) { innerRef.current.click(); + e.preventDefault(); + e.stopPropagation(); } },apps/webapp/app/routes/admin.orgs.tsx (1)
120-124
: LGTM: Scoped shortcut only when a single result existsConditionally enabling mod+Enter for the single-org case reads well and avoids conflicts when multiple rows are present. Nice touch enabling on input elements so it works from the search field.
Minor: if you want screen readers to announce the shortcut, consider adding
aria-keyshortcuts
via Button/LinkButton whenshortcut
is set (e.g.,Meta+Enter
/Control+Enter
). I can draft a utility if helpful.apps/webapp/app/routes/admin._index.tsx (1)
147-151
: LGTM: Shortcut for single-user impersonationThe conditional
mod+Enter
shortcut improves flow and matches the orgs page behavior. Works with the new Button wiring.Optional: same a11y note about
aria-keyshortcuts
for discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/webapp/app/components/primitives/Buttons.tsx
(2 hunks)apps/webapp/app/routes/admin._index.tsx
(1 hunks)apps/webapp/app/routes/admin.orgs.tsx
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}
: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/routes/admin.orgs.tsx
apps/webapp/app/components/primitives/Buttons.tsx
apps/webapp/app/routes/admin._index.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/admin.orgs.tsx
apps/webapp/app/components/primitives/Buttons.tsx
apps/webapp/app/routes/admin._index.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/admin.orgs.tsx
apps/webapp/app/components/primitives/Buttons.tsx
apps/webapp/app/routes/admin._index.tsx
🧬 Code graph analysis (1)
apps/webapp/app/components/primitives/Buttons.tsx (1)
apps/webapp/app/hooks/useShortcutKeys.tsx (1)
useShortcutKeys
(28-57)
⏰ 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). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
apps/webapp/app/components/primitives/Buttons.tsx (1)
301-311
: No change needed:createKeysFromShortcut
already handles undefined
createKeysFromShortcut
returns an empty array when passedundefined
, so calling it unconditionally won’t crash. No update required.
Adds a shortcut key to the admin area if only 1 filter result is shown for faster access