add eks deply configs and test for server and client - #72
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces a complete CI/CD and infrastructure setup: GitHub Actions workflow for building Docker images and deploying to AWS EKS, Terraform configuration provisioning S3 and ECR repositories, Kubernetes manifests for the EKS cluster and application deployments, containerization with health checks for both client and server, comprehensive test suites, and a server app refactoring using a factory pattern. ChangesInfrastructure & Deployment Pipeline
Application Code & Testing
Project Configuration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 21 minutes and 53 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (9)
infra/terraform/versions.tf (1)
2-2: ⚡ Quick winTighten
required_versionto prevent unintended Terraform major-version upgrades.
>= 1.6.0permits Terraform 2.x (and beyond), which could introduce breaking HCL semantics or provider compatibility issues. The pessimistic constraint operator is a better fit here:♻️ Proposed fix
- required_version = ">= 1.6.0" + required_version = "~> 1.6"
~> 1.6allows any1.x >= 1.6patch/minor release while blocking2.0+, matching the same intent applied to the provider versions below.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/terraform/versions.tf` at line 2, Update the Terraform required_version constraint to use a pessimistic operator to block Terraform 2.x: replace the existing required_version = ">= 1.6.0" with a constraint that pins the major version (e.g., "~> 1.6") so it allows 1.x >= 1.6 but prevents 2.0+; locate and modify the required_version entry in versions.tf accordingly.client/src/components/__tests__/Button.test.jsx (1)
27-43: 💤 Low valueConsider using
userEvent.setup()for click tests.
@testing-library/user-eventv14 recommends theuserEvent.setup()pattern over direct utility calls, as it maintains consistent pointer state and works correctly with the concurrency model. The convenience API still works, but it instantiates a newUserEventper call.♻️ Suggested refactor
it("invokes onClick when clicked", async () => { + const user = userEvent.setup(); const onClick = vi.fn(); render(<Button onClick={onClick}>Go</Button>); - await userEvent.click(screen.getByRole("button")); + await user.click(screen.getByRole("button")); expect(onClick).toHaveBeenCalledTimes(1); }); it("does not invoke onClick when disabled", async () => { + const user = userEvent.setup(); const onClick = vi.fn(); render( <Button onClick={onClick} disabled> Go </Button>, ); - await userEvent.click(screen.getByRole("button")); + await user.click(screen.getByRole("button")); expect(onClick).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/__tests__/Button.test.jsx` around lines 27 - 43, Replace direct calls to the convenience API userEvent.click with a single user-event instance created via userEvent.setup() in these tests: create const user = userEvent.setup() at the top of each test (or once in a beforeEach) and replace await userEvent.click(screen.getByRole("button")) with await user.click(screen.getByRole("button")) in the "invokes onClick when clicked" and "does not invoke onClick when disabled" tests so pointer state and concurrency are handled consistently.client/src/api/__tests__/auth.test.js (1)
15-21: 💤 Low valueRemove redundant
mockPost.mockReset()fromafterEach.Both
beforeEachandafterEachreset the same mock. ThebeforeEachreset already guarantees a clean state before every test, making theafterEachreset a no-op.♻️ Proposed fix
beforeEach(() => { mockPost.mockReset(); }); - -afterEach(() => { - mockPost.mockReset(); -});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/api/__tests__/auth.test.js` around lines 15 - 21, Remove the redundant mock reset in the afterEach block: keep the existing beforeEach that calls mockPost.mockReset() and delete the mockPost.mockReset() call inside afterEach so the mock is only reset once before each test; locate the test hooks named beforeEach and afterEach and remove the mockPost.mockReset() invocation from the afterEach function.server/__tests__/unit/utils.test.js (1)
29-37: 💤 Low value
logWarnandlogErrortests don't verify message forwarding.The
logIttest checks that the forwarded arguments contain the expected strings, butlogWarnandlogErroronly assert that the corresponding console method was called at all. A bug that dropped the arguments would go undetected.♻️ Proposed additions
it("logWarn forwards to console.warn", () => { logWarn("careful"); expect(warnSpy).toHaveBeenCalled(); + const args = warnSpy.mock.calls[0]; + expect(args.join(" ")).toContain("careful"); }); it("logError forwards to console.error", () => { logError("oops"); expect(errorSpy).toHaveBeenCalled(); + const args = errorSpy.mock.calls[0]; + expect(args.join(" ")).toContain("oops"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/__tests__/unit/utils.test.js` around lines 29 - 37, The tests for logWarn and logError currently only assert that console.warn/error were called; update them to assert the forwarded message content like the logIt test does: capture the spies (warnSpy and errorSpy) and replace the existing expect(warnSpy).toHaveBeenCalled() and expect(errorSpy).toHaveBeenCalled() with assertions that the spy was called with arguments containing the expected strings (e.g., expect(...).toHaveBeenCalledWith(expect.stringContaining("careful")) and expect(...).toHaveBeenCalledWith(expect.stringContaining("oops"))), so logWarn and logError are verified to forward the message text.infra/eks/cluster.yaml (1)
20-20: Kubernetes 1.30 is in EKS Extended Support and carries a 6× cost premium.EKS version 1.30 entered Extended Support as of July 23, 2025. Clusters on extended support incur a 500% higher control-plane cost ($0.60/cluster-hour vs $0.10 under standard support). There are 12 months from entry into extended support to migrate off v1.30 before AWS forcibly upgrades it.
Consider pinning to a version currently in standard support (1.32 or 1.33 are available per the EKS docs) to avoid unnecessary charges and the forced-upgrade risk before mid-2026.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/eks/cluster.yaml` at line 20, The cluster.yaml currently pins EKS to version "1.30" which is in Extended Support and incurs a large cost premium; update the version key in infra/eks/cluster.yaml from "1.30" to a currently standard-supported EKS minor version (for example "1.33" or "1.32") to avoid extended-support charges and forced upgrades—ensure you replace the value of the top-level version field and run any CI/validation that checks supported EKS versions.infra/k8s/secret.example.yaml (1)
13-13: 💤 Low valueConsider replacing
"change-me"with a more unambiguous placeholder.
JWT_SECRET: "change-me"is a functional-looking value that could slip into an accidentally applied secret. A clearly non-operable placeholder reduces the risk of an accidental deploy with a trivially guessable secret.💡 Suggested change
- JWT_SECRET: "change-me" + JWT_SECRET: "<REPLACE_WITH_A_RANDOM_256BIT_SECRET>"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/k8s/secret.example.yaml` at line 13, Replace the ambiguous JWT_SECRET value "change-me" with a clearly non-operable placeholder so it cannot be mistaken for a real secret; update the JWT_SECRET entry to a conspicuous token (e.g. "__REPLACE_WITH_SECURE_JWT_SECRET__" or "<CHANGE_ME_DO_NOT_COMMIT>") and add a short comment if needed to indicate it must be replaced before deployment.client/src/pages/__tests__/Login.test.jsx (1)
47-49: 💤 Low valueRemove the redundant
afterEach
loginUser.mockReset()inafterEachis already covered by thebeforeEachthat runs before each subsequent test. TheafterEachhas no effect on anything not already handled.♻️ Proposed cleanup
- afterEach(() => { - loginUser.mockReset(); - }); -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/__tests__/Login.test.jsx` around lines 47 - 49, The test contains a redundant afterEach that calls loginUser.mockReset(); remove the entire afterEach(() => { loginUser.mockReset(); }); block since loginUser is already reset in the existing beforeEach, leaving the beforeEach intact to handle mock resetting/clearing for each test.infra/k8s/server-service-lb.yaml (1)
12-22: Consider TLS termination on the NLB for the public API.The NLB operates at Layer 4 with no TLS configured, meaning auth credentials (passwords at signup/login) and JWT tokens are transmitted in plaintext over the public internet. AWS NLB supports TLS offload via the
service.beta.kubernetes.io/aws-load-balancer-ssl-certannotation.The same applies to
infra/k8s/client-service.yaml. For a dev/demo environment this may be acceptable, but for any production traffic serving real users it should be addressed before go-live.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/k8s/server-service-lb.yaml` around lines 12 - 22, Add TLS termination annotations to the NLB service so public traffic is encrypted: update the Service that uses service.beta.kubernetes.io/aws-load-balancer-type: nlb (and spec.type: LoadBalancer) to include service.beta.kubernetes.io/aws-load-balancer-ssl-cert with your ACM certificate ARN and service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"; change the exposed port from 80 to 443 (name https) and keep targetPort pointing to your existing backend (http port 80) so the NLB terminates TLS and forwards plain HTTP to pods; apply the same annotation/port changes to the client-service Service as well.infra/k8s/client-deployment.yaml (1)
58-75: ⚡ Quick win
emptyDir: {}is disk-backed, not tmpfs.The comment says these mounts are tmpfs, but Kubernetes only mounts a RAM-backed tmpfs when
emptyDir.mediumis set toMemory. As written,/var/cache/nginxand/tmpwill consume node ephemeral storage instead. If disk-backed scratch space is intentional, the comment should be updated to match. (kubernetes.io)Suggested manifest fix
volumes: - name: nginx-cache - emptyDir: {} + emptyDir: + medium: Memory - name: tmp - emptyDir: {} + emptyDir: + medium: Memory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@infra/k8s/client-deployment.yaml` around lines 58 - 75, The comment claims the nginx-cache and tmp mounts are tmpfs but the manifest uses emptyDir: {} which is disk-backed; update the emptyDir entries for the volumes named "nginx-cache" and "tmp" to include medium: Memory (emptyDir: { medium: Memory }) so they become RAM-backed tmpfs, or if disk-backed is intended, change the comment to remove the tmpfs claim and state they are node ephemeral storage; locate the volume definitions for "nginx-cache" and "tmp" and adjust either the emptyDir.medium or the comment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy_eks.yaml:
- Around line 3-19: The workflow can run concurrent deployments against the same
shared cluster (EKS_CLUSTER_NAME) causing race conditions; add a top-level
concurrency block in .github/workflows/deploy_eks.yaml to serialize runs that
target the same cluster by using a group key that includes the EKS cluster name
(e.g. use github.workflow or github.ref plus env.EKS_CLUSTER_NAME) and set
cancel-in-progress to false so only one run mutates the cluster at a time and
subsequent runs queue up instead of running in parallel.
- Around line 80-119: The CI currently runs the "Phase 2 - Terraform Apply" job
in infra/terraform against the local backend, losing state; add a remote backend
(e.g., S3 backend + DynamoDB lock or Terraform Cloud) in the Terraform
configuration and ensure terraform init is invoked with that backend before
plan/apply. Concretely: add a backend block in the Terraform root (or supply
-backend-config values) so state is stored in the designated S3 bucket and locks
in DynamoDB, create or provision that S3 bucket/DynamoDB table beforehand (or
supply credentials via the existing Configure AWS credentials step), and update
the workflow steps around "Terraform Init" / "Terraform Plan" / "Terraform
Apply" to run init with the backend config so subsequent runs reuse the remote
state and avoid orphaned resources and apply failures. Ensure the remote backend
config references the same project variables used for ECR and bucket naming so
resources remain consistent across runs.
In `@client/src/api/__tests__/auth.test.js`:
- Around line 82-86: The test title is misleading because when
error.response?.data is missing handleApiError rethrows the original Error —
update the test description to reflect that behavior (e.g., rename the it(...)
string to "rethrows original error when error response has no body") so it
matches the existing assertion using signupUser and mockPost rejecting with new
Error("network down"); do not change the assertion or error behavior, only the
test description referencing signupUser and handleApiError.
In `@infra/eks/cluster.yaml`:
- Line 43: The cluster spec currently sets privateNetworking: false which places
worker nodes in public subnets; change privateNetworking to true in the cluster
YAML (the privateNetworking field in the cluster/nodegroup spec) so node groups
launch into private subnets and do not receive public IPs, and if needed ensure
the VPC has private subnets and appropriate NAT or routing for outbound
connectivity and that security group and nodeGroup settings (node group
name/nodeGroup configuration) allow required access via bastion/NAT instead of
public IPs.
- Line 23: The template exposes a real AWS account ID in the IAM role ARN; find
occurrences of the serviceRoleARN entry (e.g., the value
"arn:aws:iam::541357644124:role/LabRole") and replace the 12-digit account
number with a placeholder (for example "arn:aws:iam::<ACCOUNT_ID>:role/LabRole"
or "arn:aws:iam::123456789012:role/LabRole") so no real account identifiers are
committed; update all matching occurrences in the file (search for
serviceRoleARN and the specific ARN string) and keep the role name "LabRole"
unchanged.
In `@infra/terraform/.gitignore`:
- Line 2: Remove the `.terraform.lock.hcl` entry from infra/terraform/.gitignore
so the lockfile is committed; after removing that line, run `terraform init` and
then create multi-platform provider checksums with `terraform providers lock`
for the required platforms (e.g., linux_amd64, darwin_arm64, darwin_amd64), then
add and commit the generated .terraform.lock.hcl to version control to ensure
reproducible provider versions.
In `@infra/terraform/main.tf`:
- Around line 35-43: The aws_s3_bucket_server_side_encryption_configuration
resource "artifacts" is using sse_algorithm = "AES256" (SSE-S3) but also sets
bucket_key_enabled, which only applies to SSE-KMS; remove the bucket_key_enabled
attribute to keep SSE-S3, or if you need bucket key functionality change
sse_algorithm to "aws:kms" and configure a KMS key (e.g., add kms_key_id) for
the resource instead; update the resource "artifacts" accordingly to eliminate
the configuration inconsistency.
- Around line 64-87: Update the two aws_ecr_repository resources
(aws_ecr_repository.server and aws_ecr_repository.client) to make image tags
immutable except for the CI `latest` tag: change image_tag_mutability to
"IMMUTABLE_WITH_EXCLUSION" and add an image_tag_mutability_exclusion_filter
block that sets filter = "latest" and filter_type = "WILDCARD" so SHA-based tags
remain immutable while allowing the latest tag to be mutable for workflows.
In `@server/__tests__/unit/validation-matches.test.js`:
- Around line 1-7: The refinement in the match validation schema (e.g., inside
createMatchSchema / any schema that checks endTime <= startTime) uses the
removed enum z.ZodIssueCode.custom; change that to the string literal "custom"
(replace code: z.ZodIssueCode.custom with code: "custom") so the Zod v4
refinement error is created correctly and runtime errors stop occurring.
---
Nitpick comments:
In `@client/src/api/__tests__/auth.test.js`:
- Around line 15-21: Remove the redundant mock reset in the afterEach block:
keep the existing beforeEach that calls mockPost.mockReset() and delete the
mockPost.mockReset() call inside afterEach so the mock is only reset once before
each test; locate the test hooks named beforeEach and afterEach and remove the
mockPost.mockReset() invocation from the afterEach function.
In `@client/src/components/__tests__/Button.test.jsx`:
- Around line 27-43: Replace direct calls to the convenience API userEvent.click
with a single user-event instance created via userEvent.setup() in these tests:
create const user = userEvent.setup() at the top of each test (or once in a
beforeEach) and replace await userEvent.click(screen.getByRole("button")) with
await user.click(screen.getByRole("button")) in the "invokes onClick when
clicked" and "does not invoke onClick when disabled" tests so pointer state and
concurrency are handled consistently.
In `@client/src/pages/__tests__/Login.test.jsx`:
- Around line 47-49: The test contains a redundant afterEach that calls
loginUser.mockReset(); remove the entire afterEach(() => {
loginUser.mockReset(); }); block since loginUser is already reset in the
existing beforeEach, leaving the beforeEach intact to handle mock
resetting/clearing for each test.
In `@infra/eks/cluster.yaml`:
- Line 20: The cluster.yaml currently pins EKS to version "1.30" which is in
Extended Support and incurs a large cost premium; update the version key in
infra/eks/cluster.yaml from "1.30" to a currently standard-supported EKS minor
version (for example "1.33" or "1.32") to avoid extended-support charges and
forced upgrades—ensure you replace the value of the top-level version field and
run any CI/validation that checks supported EKS versions.
In `@infra/k8s/client-deployment.yaml`:
- Around line 58-75: The comment claims the nginx-cache and tmp mounts are tmpfs
but the manifest uses emptyDir: {} which is disk-backed; update the emptyDir
entries for the volumes named "nginx-cache" and "tmp" to include medium: Memory
(emptyDir: { medium: Memory }) so they become RAM-backed tmpfs, or if
disk-backed is intended, change the comment to remove the tmpfs claim and state
they are node ephemeral storage; locate the volume definitions for "nginx-cache"
and "tmp" and adjust either the emptyDir.medium or the comment accordingly.
In `@infra/k8s/secret.example.yaml`:
- Line 13: Replace the ambiguous JWT_SECRET value "change-me" with a clearly
non-operable placeholder so it cannot be mistaken for a real secret; update the
JWT_SECRET entry to a conspicuous token (e.g.
"__REPLACE_WITH_SECURE_JWT_SECRET__" or "<CHANGE_ME_DO_NOT_COMMIT>") and add a
short comment if needed to indicate it must be replaced before deployment.
In `@infra/k8s/server-service-lb.yaml`:
- Around line 12-22: Add TLS termination annotations to the NLB service so
public traffic is encrypted: update the Service that uses
service.beta.kubernetes.io/aws-load-balancer-type: nlb (and spec.type:
LoadBalancer) to include service.beta.kubernetes.io/aws-load-balancer-ssl-cert
with your ACM certificate ARN and
service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443"; change the
exposed port from 80 to 443 (name https) and keep targetPort pointing to your
existing backend (http port 80) so the NLB terminates TLS and forwards plain
HTTP to pods; apply the same annotation/port changes to the client-service
Service as well.
In `@infra/terraform/versions.tf`:
- Line 2: Update the Terraform required_version constraint to use a pessimistic
operator to block Terraform 2.x: replace the existing required_version = ">=
1.6.0" with a constraint that pins the major version (e.g., "~> 1.6") so it
allows 1.x >= 1.6 but prevents 2.0+; locate and modify the required_version
entry in versions.tf accordingly.
In `@server/__tests__/unit/utils.test.js`:
- Around line 29-37: The tests for logWarn and logError currently only assert
that console.warn/error were called; update them to assert the forwarded message
content like the logIt test does: capture the spies (warnSpy and errorSpy) and
replace the existing expect(warnSpy).toHaveBeenCalled() and
expect(errorSpy).toHaveBeenCalled() with assertions that the spy was called with
arguments containing the expected strings (e.g.,
expect(...).toHaveBeenCalledWith(expect.stringContaining("careful")) and
expect(...).toHaveBeenCalledWith(expect.stringContaining("oops"))), so logWarn
and logError are verified to forward the message text.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d48bdd5-0df8-4d8f-8af2-fc7f1ed88d22
⛔ Files ignored due to path filters (2)
client/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlserver/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (40)
.github/workflows/deploy_eks.yaml.gitignorebiome.jsonclient/Dockerfileclient/nginx.confclient/package.jsonclient/src/api/__tests__/auth.test.jsclient/src/components/FeaturedCarousel.jsxclient/src/components/__tests__/Button.test.jsxclient/src/components/__tests__/Input.test.jsxclient/src/pages/__tests__/Login.test.jsxclient/src/test/setup.jsclient/vite.config.jsinfra/eks/cluster.yamlinfra/k8s/client-deployment.yamlinfra/k8s/client-service.yamlinfra/k8s/namespace.yamlinfra/k8s/secret.example.yamlinfra/k8s/server-deployment.yamlinfra/k8s/server-service-lb.yamlinfra/k8s/server-service.yamlinfra/terraform/.gitignoreinfra/terraform/main.tfinfra/terraform/outputs.tfinfra/terraform/variables.tfinfra/terraform/versions.tfserver/Dockerfileserver/__tests__/e2e/auth-flow.test.jsserver/__tests__/integration/auth.test.jsserver/__tests__/integration/health.test.jsserver/__tests__/setup/env.jsserver/__tests__/unit/jwt.test.jsserver/__tests__/unit/match-status.test.jsserver/__tests__/unit/utils.test.jsserver/__tests__/unit/validation-auth.test.jsserver/__tests__/unit/validation-matches.test.jsserver/jest.config.jsserver/package.jsonserver/src/app.jsserver/src/index.js
| it("falls back to default message when error response has no body", async () => { | ||
| mockPost.mockRejectedValue(new Error("network down")); | ||
|
|
||
| await expect(signupUser({})).rejects.toThrow("network down"); | ||
| }); |
There was a problem hiding this comment.
Inaccurate test description — code path rethrows, not "falls back to default message".
When error.response?.data is absent, handleApiError executes throw error (the original Error("network down")). No default message is involved. The description misleadingly implies the defaultMessage parameter is used here.
🐛 Proposed fix
-it("falls back to default message when error response has no body", async () => {
+it("rethrows the original error when response has no body", async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("falls back to default message when error response has no body", async () => { | |
| mockPost.mockRejectedValue(new Error("network down")); | |
| await expect(signupUser({})).rejects.toThrow("network down"); | |
| }); | |
| it("rethrows the original error when response has no body", async () => { | |
| mockPost.mockRejectedValue(new Error("network down")); | |
| await expect(signupUser({})).rejects.toThrow("network down"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/src/api/__tests__/auth.test.js` around lines 82 - 86, The test title
is misleading because when error.response?.data is missing handleApiError
rethrows the original Error — update the test description to reflect that
behavior (e.g., rename the it(...) string to "rethrows original error when error
response has no body") so it matches the existing assertion using signupUser and
mockPost rejecting with new Error("network down"); do not change the assertion
or error behavior, only the test description referencing signupUser and
handleApiError.
| version: "1.30" | ||
|
|
||
| iam: | ||
| serviceRoleARN: arn:aws:iam::541357644124:role/LabRole |
There was a problem hiding this comment.
Replace the real AWS account ID with a placeholder.
541357644124 is a real 12-digit account ID committed to a public repository. Even though the comment instructs readers to replace it, shipping a committed template with a real account number exposes account-level information to anyone browsing the repository.
🛡️ Proposed fix
iam:
- serviceRoleARN: arn:aws:iam::541357644124:role/LabRole
+ serviceRoleARN: arn:aws:iam::<YOUR_ACCOUNT_ID>:role/LabRole
withOIDC: false
...
iam:
- instanceRoleARN: arn:aws:iam::541357644124:role/LabRole
+ instanceRoleARN: arn:aws:iam::<YOUR_ACCOUNT_ID>:role/LabRoleAlso applies to: 45-45
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/eks/cluster.yaml` at line 23, The template exposes a real AWS account
ID in the IAM role ARN; find occurrences of the serviceRoleARN entry (e.g., the
value "arn:aws:iam::541357644124:role/LabRole") and replace the 12-digit account
number with a placeholder (for example "arn:aws:iam::<ACCOUNT_ID>:role/LabRole"
or "arn:aws:iam::123456789012:role/LabRole") so no real account identifiers are
committed; update all matching occurrences in the file (search for
serviceRoleARN and the specific ARN string) and keep the role name "LabRole"
unchanged.
| minSize: 2 | ||
| maxSize: 3 | ||
| volumeSize: 20 | ||
| privateNetworking: false |
There was a problem hiding this comment.
privateNetworking: false places worker nodes in public subnets with public IPs.
This increases the attack surface of the node group. Even for a learner lab, placing compute in private subnets and restricting inbound access via security groups is the standard posture. If the VPC created by eksctl has private subnets available, switching to true adds meaningful defence-in-depth at zero extra cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/eks/cluster.yaml` at line 43, The cluster spec currently sets
privateNetworking: false which places worker nodes in public subnets; change
privateNetworking to true in the cluster YAML (the privateNetworking field in
the cluster/nodegroup spec) so node groups launch into private subnets and do
not receive public IPs, and if needed ensure the VPC has private subnets and
appropriate NAT or routing for outbound connectivity and that security group and
nodeGroup settings (node group name/nodeGroup configuration) allow required
access via bastion/NAT instead of public IPs.
| import { describe, expect, it } from "@jest/globals"; | ||
| import { | ||
| createMatchSchema, | ||
| listMatchesQuerySchema, | ||
| matchIdParamSchema, | ||
| updateScoreSchema, | ||
| } from "../../src/validation/matches.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Zod v4 still export z.ZodIssueCode enum, or was it removed in the migration?
💡 Result:
No, Zod v4 does not export z.ZodIssueCode. The ZodIssueCode enum from v3 has been replaced by a union type of string literals for first-party issue codes, available as z.core.$ZodIssueCode (added initially in v3.25.21 and carried forward to v4). Issue types are now under z.core namespace (e.g., z.core.$ZodIssueInvalidType), and $ZodIssueCode is defined in the core as the union of codes from $ZodIssue. The migration guide confirms v3's z.Zod*Issue types are renamed/merged and no longer exported as z.ZodIssueCode.
Citations:
- 1: [v4] Replacement for
ZodIssueCodeenum? colinhacks/zod#4484 - 2: https://zod.dev/v4/changelog
- 3: https://v4.zod.dev/v4/changelog?id=updates-issue-formats
- 4: https://v4.zod.dev/packages/core?id=issues
- 5: https://github.com/colinhacks/zod/blob/main/packages/zod/src/v4/core/errors.ts
- 6: https://cdn.jsdelivr.net/npm/zod@4.0.14/v4/core/errors.d.ts
🏁 Script executed:
# Check the validation/matches.js file for ZodIssueCode usage
cat -n server/src/validation/matches.js | head -100Repository: codic-yeeshu/stathub
Length of output: 1411
🏁 Script executed:
# Check Zod version in package.json
rg "zod" server/package.json -A 2 -B 2Repository: codic-yeeshu/stathub
Length of output: 148
Fix z.ZodIssueCode.custom usage for Zod v4 compatibility.
The code at server/src/validation/matches.js:32 uses z.ZodIssueCode.custom, which no longer exists in Zod v4. The z.ZodIssueCode enum was removed and replaced with string literals. Replace code: z.ZodIssueCode.custom with code: "custom" to fix the runtime error when the validation triggers the endTime <= startTime check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/__tests__/unit/validation-matches.test.js` around lines 1 - 7, The
refinement in the match validation schema (e.g., inside createMatchSchema / any
schema that checks endTime <= startTime) uses the removed enum
z.ZodIssueCode.custom; change that to the string literal "custom" (replace code:
z.ZodIssueCode.custom with code: "custom") so the Zod v4 refinement error is
created correctly and runtime errors stop occurring.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Chores