-
Notifications
You must be signed in to change notification settings - Fork 134
[Graphite MQ] Draft PR GROUP:spec_a0e79b (PRs 3557, 3558, 3556) #3582
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
[Graphite MQ] Draft PR GROUP:spec_a0e79b (PRs 3557, 3558, 3556) #3582
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Pull Request ReviewThis PR consolidates three changes related to frontend improvements for API token management and environment configuration. Overall, the code quality is good, but I've identified several issues that should be addressed. Critical Issues1. Date Handling Bug in convertDurationToExpiresAtLocation: frontend/src/app/dialogs/create-api-token-frame.tsx:20-51 The function mutates the Date object directly, which can lead to incorrect calculations across time boundaries (DST, month boundaries, leap years). The current implementation using setMinutes, setHours, etc. is problematic. Recommendation: Use milliseconds-based calculations or a date library like date-fns for more accurate date arithmetic, especially for year calculations. 2. Missing Error Handling in Token CreationLocation: frontend/src/app/dialogs/create-api-token-frame.tsx:99-105 The form submission doesn't handle API errors. If the token creation fails, users won't see an error message. Recommendation: Add try-catch with user-facing error messages or toast notifications. 3. Sensitive Token Exposure RiskLocation: frontend/src/app/dialogs/create-api-token-frame.tsx:82 Ensure the token is cleared from memory/state when the dialog closes to minimize exposure. Medium Priority Issues4. Inconsistent IndentationLocation: frontend/src/app/dialogs/api-tokens-frame.tsx:37-38 There's an extra indentation level on line 38. 5. Missing Input ValidationLocation: frontend/src/app/forms/create-api-token-form.tsx:23 The expiresIn field is optional but doesn't validate the duration format. Add regex validation. 6. Revoke Action Without ConfirmationLocation: frontend/src/app/dialogs/api-tokens-frame.tsx:156 Revoking a token is a destructive action that should require confirmation dialog. 7. Hardcoded Clerk Key in .envLocation: frontend/.env:4 The Clerk publishable key is committed to the repository. Ensure production keys are never committed. Low Priority Issues8. Magic String cloud_api_Location: frontend/src/app/dialogs/api-tokens-frame.tsx:145 Consider extracting this prefix to a constant for consistency. 9. Accessibility: Table Width PropLocation: frontend/src/app/dialogs/api-tokens-frame.tsx:65 The prop w="min" is non-standard. Verify this is supported by your Table component. 10. Timezone Display IssueLocation: frontend/src/app/dialogs/api-tokens-frame.tsx:135-138 toLocaleDateString() displays in the user's timezone without showing which timezone. Consider adding time information too. Positive Observations✅ Good separation of concerns with dedicated form components Security Considerations
Performance Considerations
Testing Recommendations
SummaryThe implementation is solid overall with good patterns and user experience. The main concerns are:
All other issues are minor and can be addressed in follow-up PRs if time is constrained. |
This draft PR was created by the Graphite merge queue.
Trunk will be fast forwarded to the HEAD of this PR when CI passes, and the original PRs will be closed.
The following PRs are included in this draft PR: