Staging#213
Merged
Merged
Conversation
- the cache-aside wrapper is ready - the infrastructure (Kubernetes + Helm + CI sidecars) is all set up
feat(redis): Redis is now configured
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for system-craft-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Warning Review limit reached
More reviews will be available in 29 minutes and 39 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughAdds ChangesRedis Integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant HintRoute as /api/interview/[id]/hint
participant checkRateLimit
participant Redis
Client->>HintRoute: POST request + Firebase token
HintRoute->>HintRoute: authenticate Firebase user
HintRoute->>checkRateLimit: checkRateLimit(uid, 'ai-hint', 20, 3600)
checkRateLimit->>Redis: pipeline INCR rate:ai-hint:<uid>:<bucket>
checkRateLimit->>Redis: pipeline EXPIRE windowSeconds
Redis-->>checkRateLimit: [count, ok]
checkRateLimit-->>HintRoute: RateLimitResult {allowed, remaining, resetIn}
alt allowed = false
HintRoute-->>Client: 429 + X-RateLimit-Remaining, X-RateLimit-Reset
else allowed = true
HintRoute->>HintRoute: validate session id, generate hint
HintRoute-->>Client: 200 hint response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
src/lib/rateLimit.ts (1)
44-45: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAlign key TTL with the current window boundary.
Line 44 always sets
EXPIREtowindowSeconds, so keys created near the end of a bucket can live almost an extra full window. UseresetIn(seconds until next bucket) for expiry to reduce stale key buildup and keep TTL behavior consistent.♻️ Proposed adjustment
export async function checkRateLimit( @@ - // Bucket by current time window (e.g., current hour if windowSeconds is 3600) - const currentWindow = Math.floor(Date.now() / 1000 / windowSeconds); + // Bucket by current time window (e.g., current hour if windowSeconds is 3600) + const nowSeconds = Math.floor(Date.now() / 1000); + const currentWindow = Math.floor(nowSeconds / windowSeconds); + const nextWindowStart = (currentWindow + 1) * windowSeconds; + const resetIn = Math.max(1, nextWindowStart - nowSeconds); const key = `ratelimit:${action}:${identifier}:${currentWindow}`; @@ - pipeline.expire(key, windowSeconds); + pipeline.expire(key, resetIn); @@ - // Calculate reset time based on the end of the current window - const nextWindowStart = (currentWindow + 1) * windowSeconds; - const resetIn = Math.max(0, nextWindowStart - Math.floor(Date.now() / 1000)); - return { allowed, remaining, resetIn };Also applies to: 63-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/rateLimit.ts` around lines 44 - 45, The pipeline.expire call in the rateLimit.ts file currently uses windowSeconds as the TTL, which causes keys created near the end of a window to live almost an extra full window. Replace the windowSeconds parameter with resetIn (which represents seconds until the next bucket boundary) in both the pipeline.expire call at line 44 and the similar occurrences at lines 63-65 to ensure the TTL is aligned with the current window boundary and reduce stale key buildup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 132-139: Fix the Docker networking issue in the E2E CI workflow by
creating a dedicated Docker network that both containers can use for
communication. First, add a step to create a Docker network using docker network
create. Then, update both the Redis container startup (redis-e2e) and the
application container startup (systemcraft-e2e) to use the --network flag to
join this created network. Finally, update the REDIS_URL environment variable
from redis://host.docker.internal:6379 to redis://redis-e2e:6379 so the
application can reach Redis through Docker's internal DNS by using the Redis
container name instead of the host.docker.internal hostname which is unavailable
on Linux runners.
In `@app/api/health/route.ts`:
- Around line 6-8: The redisClient.ping() call in the health endpoint can block
indefinitely if Redis is experiencing latency or network issues, since the
ioredis retryStrategy only applies to connection retries, not individual command
timeouts. Add a timeout mechanism around the redisClient.ping() promise (for
example, using Promise.race with a timeout promise or a dedicated timeout
wrapper) to ensure the health endpoint remains responsive. Set an appropriate
timeout duration (e.g., 5 seconds) so that if the ping does not complete within
that time, it fails fast and returns false for the redisOk status.
In `@app/api/reference-architectures/analyze/route.ts`:
- Around line 16-17: The rate limit identity extraction at the checkRateLimit
call is using the raw x-forwarded-for header without proper handling. Replace
the unsanitized header value with a properly parsed and validated client IP
address. If using x-forwarded-for, split the comma-separated values and extract
only the first (leftmost) IP address since the header can contain multiple IPs.
Consider implementing a helper function to safely extract the client IP from
trusted headers, and ensure your ingress/proxy layer is configured to strip
client-supplied forwarding headers and only set trusted values.
In `@helm/systemcraft/templates/network-policy.yaml`:
- Around line 40-45: The Redis port configuration (lines 40-45 with protocol TCP
and port 6379) is currently controlled by both the redis.enabled check and an
implicit allowExternal condition, which prevents internal Redis communication
when external egress is disabled. Move the Redis port rule outside of any
allowExternal conditional block so that Redis traffic is allowed independently
based only on the redis.enabled flag, ensuring that intentionally disabling
external egress does not break internal Redis functionality.
In `@helm/systemcraft/values.yaml`:
- Around line 127-129: The redis.url field at line 129 hardcodes the Redis host
as systemcraft-redis, but the actual Redis Service name is templated using the
chart's fullname helper, causing connection failures when using non-default
release names. Replace the hardcoded systemcraft-redis hostname in the url with
a templated reference using the same fullname pattern as the Redis Service
(derive it from {{ include "systemcraft.fullname" . }}-redis), and ensure this
is configurable as a chart value so users can override it if needed rather than
relying on a silent fail-open behavior.
In `@kubernetes/network-policy.yaml`:
- Around line 40-43: The ports rule allowing TCP traffic on port 6379 is missing
a `to` selector, which means it permits egress to any endpoint on that port. Add
a `to` field under the ports section that specifies the destination pods or
service for Redis traffic, using either a podSelector to target Redis pods by
label or a namespaceSelector combined with podSelector to restrict egress only
to the intended Redis pods/service instead of allowing any TCP/6379 targets.
In `@kubernetes/redis-deployment.yaml`:
- Around line 17-28: The redis container in the deployment lacks security
hardening controls. Add a securityContext block to the container definition (at
the same indentation level as the name, image, ports, and resources fields) to
enforce non-root user execution and disable privilege escalation by setting
runAsNonRoot to true, allowPrivilegeEscalation to false, and specifying a
non-root user ID in runAsUser. Additionally, consider adding a securityContext
at the pod spec level (under the pod template spec) to apply broader security
policies across all containers in the pod.
In `@src/lib/referenceArchCache.test.ts`:
- Line 20: Replace the `as any` type assertions on lines 20, 34, and 54 in the
test file with proper type annotations. Instead of using the `any` type, use
specific type definitions or type casting that accurately represents the mock
object structure being created. This will satisfy the lint/typecheck
requirements without bypassing the type system.
---
Nitpick comments:
In `@src/lib/rateLimit.ts`:
- Around line 44-45: The pipeline.expire call in the rateLimit.ts file currently
uses windowSeconds as the TTL, which causes keys created near the end of a
window to live almost an extra full window. Replace the windowSeconds parameter
with resetIn (which represents seconds until the next bucket boundary) in both
the pipeline.expire call at line 44 and the similar occurrences at lines 63-65
to ensure the TTL is aligned with the current window boundary and reduce stale
key buildup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9452deb1-a6ab-4981-8d47-ffdec00b8569
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.github/workflows/ci.ymlapp/api/health/route.tsapp/api/interview/[id]/hint/route.tsapp/api/reference-architectures/analyze/route.tshelm/systemcraft/templates/deployment.yamlhelm/systemcraft/templates/network-policy.yamlhelm/systemcraft/templates/redis-deployment.yamlhelm/systemcraft/templates/redis-service.yamlhelm/systemcraft/values.yamlkubernetes/deployment.yamlkubernetes/network-policy.yamlkubernetes/redis-deployment.yamlkubernetes/redis-service.yamlpackage.jsonsrc/lib/rateLimit.test.tssrc/lib/rateLimit.tssrc/lib/redis.tssrc/lib/referenceArchCache.test.tssrc/lib/referenceArchCache.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Chores