Skip to content

fix: use ratelimit name to determine counter id#4200

Merged
chronark merged 1 commit intomainfrom
10-29-fix_use_ratelimit_name_to_determine_counter_id
Oct 29, 2025
Merged

fix: use ratelimit name to determine counter id#4200
chronark merged 1 commit intomainfrom
10-29-fix_use_ratelimit_name_to_determine_counter_id

Conversation

@chronark
Copy link
Collaborator

@chronark chronark commented Oct 29, 2025

What does this PR do?

Adds the name field to the rate limiter key generation in the DurableRateLimiter class. This change modifies the key format from identifier::window::shard to identifier::window::name::shard.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Test that rate limiting works correctly with the new key format
  • Verify that rate limits are properly isolated by name parameter
  • Check that existing rate limits continue to function after the change

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Unkey Docs if changes were necessary

@vercel
Copy link

vercel bot commented Oct 29, 2025

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

Project Deployment Preview Comments Updated (UTC)
dashboard Ready Ready Preview Comment Oct 29, 2025 0:08am
engineering Ready Ready Preview Comment Oct 29, 2025 0:08am

@changeset-bot
Copy link

changeset-bot bot commented Oct 29, 2025

⚠️ No Changeset found

Latest commit: 91f9f55

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Collaborator Author

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

@chronark chronark marked this pull request as ready for review October 29, 2025 12:07
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

📝 Walkthrough

Walkthrough

Modified the cache key generation in DurableRateLimiter.getId to include req.name alongside existing components (identifier, window, shard). This changes how rate-limit buckets are keyed, potentially creating separate rate-limit buckets for identical identifiers with different names.

Changes

Cohort / File(s) Summary
Rate-limit cache key composition
apps/api/src/pkg/ratelimit/do_client.ts
Updated DurableRateLimiter.getId to include req.name in the cache key generation, making the cache lookup name-aware

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that including req.name in the cache key is intentional and correctly addresses the intended rate-limiting scoping
  • Confirm the cache key format change doesn't break existing rate-limit data or create unintended key collisions
  • Check if related code paths that depend on the old cache key format are updated accordingly

Possibly related PRs

Suggested labels

Bug, Needs Approval

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
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.
Description Check ⚠️ Warning The PR description provided is essentially a blank template with no substantive content filled in. The "What does this PR do?" section contains only placeholder text and no issue reference, the "Type of change" section has no checkbox selected, the "How should this be tested?" section contains only placeholder items ("Test A", "Test B"), and all required checklist items are marked as unchecked. While the PR objectives reveal the actual intent (adding the name field to rate limiter key generation in a bug fix), this information is not reflected in the PR description itself. The author appears to have submitted the template without completing any of the required sections. The author must complete the PR description by filling in all required sections: providing a real issue number or creating one, selecting "Bug fix" as the type of change, describing the specific changes and motivation, providing actual testing instructions with reproduction steps, and completing the required checklist items. The description should clearly explain that the change modifies the rate limiter cache key format to include the name parameter, isolating rate limits by name in addition to existing criteria.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "fix: use ratelimit name to determine counter id" is clear, concise, and directly summarizes the main change in the changeset. The title accurately reflects the modification to the cache key generation in DurableRateLimiter to include the name field, which aligns with the AI-generated summary and the PR description. The title is specific enough for a teammate to understand the primary change without ambiguity.
✨ 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 10-29-fix_use_ratelimit_name_to_determine_counter_id

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.

@vercel vercel bot temporarily deployed to Preview – engineering October 29, 2025 12:08 Inactive
@vercel vercel bot temporarily deployed to Preview – dashboard October 29, 2025 12:08 Inactive
@CLAassistant
Copy link

CLAassistant commented Oct 29, 2025

CLA assistant check
All committers have signed the CLA.

@chronark chronark added this pull request to the merge queue Oct 29, 2025
@graphite-app
Copy link

graphite-app bot commented Oct 29, 2025

SpongeBob gif. SpongeBob pretends to crank his fist like a jack-in-the-box, and his thumb rises and pops out for a thumbs up. He then gestures to his thumb like 'eh? What do you think?' (Added via Giphy)

@graphite-app
Copy link

graphite-app bot commented Oct 29, 2025

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (10/29/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

Merged via the queue into main with commit ce0505d Oct 29, 2025
25 checks passed
@chronark chronark deleted the 10-29-fix_use_ratelimit_name_to_determine_counter_id branch October 29, 2025 12:23
@coderabbitai coderabbitai bot mentioned this pull request Nov 4, 2025
19 tasks
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.

5 participants