Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 30, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivet-inspector Ready Ready Preview Comment Dec 1, 2025 2:02pm
rivetkit-serverless Ready Ready Preview Comment Dec 1, 2025 2:02pm
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 1, 2025 2:02pm
rivet-site Ignored Ignored Preview Dec 1, 2025 2:02pm

@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless November 30, 2025 09:47 Inactive
Copy link
Member Author

NathanFlurry commented Nov 30, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 30, 2025

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3556

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3556

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3556

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3556

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3556

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3556

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3556

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3556

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3556

commit: b2e0ffb

@claude
Copy link

claude bot commented Nov 30, 2025

PR Review: feat(frontend): create api key

This PR adds Cloud API token management functionality to the frontend. Overall, the implementation is solid with good UX patterns. Here are my findings:

Strengths

  1. Security Best Practices

    • One-time token display after creation (lines 72-95 in create-api-token-frame.tsx)
    • Clear warning message about token visibility
    • Masked token display in list view (showing only last 4 characters)
    • Proper revocation functionality
  2. Good UX Patterns

    • Loading states with skeletons
    • Optimistic UI updates with query invalidation
    • Comprehensive token expiration options (15m to never)
    • Inline creation button in the token list table
    • Clear visual distinction for revoked tokens (opacity-50)
  3. Code Organization

    • Well-separated concerns (form, dialog, data provider)
    • Consistent patterns with existing codebase
    • TypeScript types properly defined

🐛 Potential Issues

1. Date Handling Bug in convertDurationToExpiresAt (High Priority)

Location: frontend/src/app/dialogs/create-api-token-frame.tsx:20-51

The date mutation methods used can produce incorrect results:

now.setDate(now.getDate() + value);  // Line 43
now.setFullYear(now.getFullYear() + value);  // Line 46

Issues:

  • setDate can cause month overflow issues (e.g., Jan 31 + 30 days = March 2/3 depending on year)
  • setFullYear doesn't account for leap years properly
  • Mutations can have unexpected side effects

Recommendation:

function convertDurationToExpiresAt(duration: string): string | undefined {
  if (duration === "never") {
    return undefined;
  }

  const now = Date.now();
  const match = duration.match(/^(\d+)([mhdy])$/);

  if (\!match) {
    return undefined;
  }

  const value = Number.parseInt(match[1], 10);
  const unit = match[2];

  const MS_PER_MINUTE = 60 * 1000;
  const MS_PER_HOUR = 60 * MS_PER_MINUTE;
  const MS_PER_DAY = 24 * MS_PER_HOUR;
  const MS_PER_YEAR = 365.25 * MS_PER_DAY; // Account for leap years

  let expiresAt: number;
  switch (unit) {
    case "m":
      expiresAt = now + value * MS_PER_MINUTE;
      break;
    case "h":
      expiresAt = now + value * MS_PER_HOUR;
      break;
    case "d":
      expiresAt = now + value * MS_PER_DAY;
      break;
    case "y":
      expiresAt = now + value * MS_PER_YEAR;
      break;
    default:
      return undefined;
  }

  return new Date(expiresAt).toISOString();
}

2. Missing Confirmation Dialog for Token Revocation (Medium Priority)

Location: frontend/src/app/dialogs/api-tokens-frame.tsx:156

Clicking "Revoke" immediately revokes the token without confirmation. This is a destructive action that could cause service disruptions.

Recommendation: Add a confirmation dialog before revoking.

3. Environment File Change Should Not Be Committed (Low Priority)

Location: frontend/.env:2

Changed from http://localhost:43708/api to http://localhost:3000. This appears to be a local development configuration that shouldn't be in the PR.

Recommendation: Revert this change or use .env.local for local overrides.

4. Missing Error Boundary (Low Priority)

The match(__APP_TYPE__) calls (e.g., lines 78-83 in tokens.tsx) throw errors for unknown contexts but lack error boundaries to gracefully handle this.

Recommendation: Add error boundaries or handle the error state in the UI.

🔍 Code Quality Observations

  1. Type Safety: Good use of TypeScript, proper inference from contexts
  2. Query Management: Proper use of React Query patterns with invalidation
  3. Accessibility: Some good practices (aria-labels on inputs at lines 175-205 in tokens.tsx)
  4. Styling: Consistent use of Tailwind classes and design system components

🧪 Testing Recommendations

The PR lacks visible test coverage. Consider adding:

  1. Unit tests for convertDurationToExpiresAt (especially edge cases)
  2. Integration tests for the token creation/revocation flow
  3. E2E tests for the complete user journey

📊 Performance Considerations

  1. Query Refetch Interval: Line 21 in publishable-token-code-group.tsx has refetchInterval: 5000. This might be excessive depending on how often runner configs change.
  2. Multiple Dialogs: The nested dialog pattern (CreateApiToken inside ApiTokens) is handled well with proper state management.

🔒 Security Assessment

✅ Tokens are properly masked
✅ One-time display for sensitive data
✅ Bearer token pattern for API calls
✅ No hardcoded secrets
⚠️ Consider rate limiting on the backend for token creation

📝 Documentation

The code includes good inline comments and clear variable names. The comprehensive code examples in the Cloud API Tokens section (lines 335-479 in tokens.tsx) are excellent for developer experience.

Minor Suggestions

  1. Inconsistent indentation at line 38 in api-tokens-frame.tsx (extra tab)
  2. Unused import: faCopy is imported but not used in tokens.tsx:1
  3. Consider extracting the date formatting logic to a shared utility
  4. The validation in the form schema (line 18-23 in create-api-token-form.tsx) could validate the duration format

🎯 Summary

This is a well-structured feature addition with good security practices. The main concern is the date calculation bug which should be fixed before merging. The missing revocation confirmation is also worth addressing to prevent accidental token deletions.

Recommendation: Request changes - Fix the date handling bug before merging.


Review generated by Claude Code

@NathanFlurry NathanFlurry force-pushed the 11-30-feat_frontend_create_api_key branch from 2508523 to ed9dd00 Compare November 30, 2025 23:53
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless November 30, 2025 23:53 Inactive
@NathanFlurry NathanFlurry changed the base branch from main to graphite-base/3556 December 1, 2025 05:47
@NathanFlurry NathanFlurry force-pushed the 11-30-feat_frontend_create_api_key branch from ed9dd00 to 6e53a16 Compare December 1, 2025 05:47
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 05:47 Inactive
@NathanFlurry NathanFlurry changed the base branch from graphite-base/3556 to 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page December 1, 2025 05:47
@NathanFlurry NathanFlurry marked this pull request as ready for review December 1, 2025 05:47
@NathanFlurry NathanFlurry force-pushed the 11-30-feat_frontend_create_api_key branch from 6e53a16 to 9f63d63 Compare December 1, 2025 05:57
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 05:57 Inactive
@graphite-app graphite-app bot force-pushed the 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page branch from 6348746 to 179a5c9 Compare December 1, 2025 06:01
@graphite-app graphite-app bot force-pushed the 11-30-feat_frontend_create_api_key branch from 9f63d63 to a9b55b1 Compare December 1, 2025 06:01
@vercel vercel bot temporarily deployed to Preview – rivetkit-serverless December 1, 2025 06:01 Inactive
@NathanFlurry NathanFlurry force-pushed the 11-30-fix_frontend_do_not_show_tokens_button_if_not_on_namespace_page branch from 179a5c9 to 2ac6633 Compare December 1, 2025 13:59
@NathanFlurry NathanFlurry force-pushed the 11-30-feat_frontend_create_api_key branch from a9b55b1 to b2e0ffb Compare December 1, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants