Fix optional env #679
Fix optional env #679
Conversation
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
@mainertoo is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow to build and push the OpenCut Web Docker image to GHCR, relaxes several server-side environment variables to optional, and replaces the upload URL endpoint: the API now exposes a GET handler that queries the Freesound API and no longer generates R2 presigned URLs or requires transcription/R2 runtime checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant Freesound as Freesound API
Client->>Server: GET /api/get-upload-url?query=...&page=...&page_size=...&sort=...
alt Missing Freesound config
Server-->>Client: 503 {"error":"FREESOUND config missing"}
else Freesound config present
Server->>Freesound: GET /search/text with mapped query params & API key
Freesound-->>Server: 200 OK with search results
Server-->>Client: 200 OK with Freesound response data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/build-opencut-web.yml:
- Around line 58-60: The workflow is exposing FREESOUND_CLIENT_ID and
FREESOUND_API_KEY via Docker build-args which bakes secrets into image layers;
remove FREESOUND_CLIENT_ID and FREESOUND_API_KEY from the build-args block in
build-opencut-web.yml and instead provide them as runtime environment variables
(e.g., via your deployment/Kubernetes/Docker Compose) or, if absolutely needed
during build, switch to BuildKit secrets via the action's secrets: input and
mount them in the Dockerfile; ensure apps/web/src/env.ts continues to read these
as optional runtime env vars rather than relying on build-time args.
In @apps/web/src/env.ts:
- Around line 16-22: The module currently constructs Redis (new Redis) and
exports baseRateLimit (new Ratelimit) at load time using
env.UPSTASH_REDIS_REST_URL and env.UPSTASH_REDIS_REST_TOKEN which can be
undefined; change rate-limit.ts to check those two env vars first and only
instantiate Redis and Ratelimit when both are present, otherwise export a no-op
fallback with the same public API as Ratelimit (e.g., an object exposing the
methods your routes call, such as `.limit()`/`.consume()` or whatever your code
uses) so callers can operate without a running Redis instance; reference symbols
to update: env.UPSTASH_REDIS_REST_URL, env.UPSTASH_REDIS_REST_TOKEN, Redis
(constructor usage), and baseRateLimit / Ratelimit (export) so initialization is
conditional and safe.
- Around line 24-28: env.FREESOUND_API_KEY is optional in the env schema but
route.ts uses env.FREESOUND_API_KEY directly, causing "undefined" to be sent to
Freesound; update the search route handler
(apps/web/src/app/api/sounds/search/route.ts) to check for env.FREESOUND_API_KEY
at the start of the request handler and if missing return a proper HTTP error
(e.g., 503 with message like "Freesound integration not configured" or similar),
or alternatively change the env schema to make FREESOUND_API_KEY required if the
integration must always be available—choose one approach and implement the
corresponding guard and error response in the function that calls the Freesound
API.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build-opencut-web.ymlapps/web/src/env.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of for loops when you don't need initializer and update expressions.
Don't reassign const variables....
Files:
apps/web/src/env.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and...
Files:
apps/web/src/env.ts
🧠 Learnings (1)
📚 Learning: 2025-07-27T22:14:46.402Z
Learnt from: CR
Repo: OpenCut-app/OpenCut PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-27T22:14:46.402Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Don't hardcode sensitive data like API keys and tokens.
Applied to files:
apps/web/src/env.ts
🔇 Additional comments (6)
.github/workflows/build-opencut-web.yml (3)
1-17: Workflow configuration looks good.The workflow name, triggers, path filters, and permissions are properly configured following GitHub Actions best practices.
18-48: Docker setup steps are properly configured.The checkout, QEMU, Buildx setup, GHCR login, and metadata generation steps follow best practices for Docker image builds in GitHub Actions.
55-55: Verify single-platform build is intentional.The workflow sets up QEMU (line 26) for multi-architecture builds but only targets
linux/amd64. If you plan to deploy on ARM-based infrastructure (e.g., AWS Graviton, Apple Silicon), consider addinglinux/arm64:platforms: linux/amd64,linux/arm64If amd64-only is intentional, you can remove the QEMU setup step to speed up the workflow.
apps/web/src/env.ts (3)
13-13: Good addition of NEXT_RUNTIME validation.Adding validation for Vercel's
NEXT_RUNTIMEenvironment variable with the correct enum values (nodejs|edge) ensures type safety while keeping it optional for non-Vercel deployments.
39-42: Code correctly validates transcription configuration before use.Verified that both
POST /api/transcribeandPOST /api/get-upload-urlcallisTranscriptionConfigured()early to check forMODAL_TRANSCRIPTION_URLpresence before attempting to use it. If the URL is missing, a 503 error is returned with a clear message listing which environment variables are required. No action needed.
30-37: R2 configuration is properly validated as a complete set.The codebase correctly handles this through the
isTranscriptionConfigured()utility function inapps/web/src/lib/transcription-utils.ts, which checks that all R2 variables (CLOUDFLARE_ACCOUNT_ID, R2_ACCESS_KEY_ID, R2_SECRET_ACCESS_KEY, R2_BUCKET_NAME) are present together. The transcription feature is only enabled when all required variables exist, preventing partial configurations that would cause runtime errors.
| build-args: | | ||
| FREESOUND_CLIENT_ID=${{ secrets.FREESOUND_CLIENT_ID }} | ||
| FREESOUND_API_KEY=${{ secrets.FREESOUND_API_KEY }} |
There was a problem hiding this comment.
Critical security vulnerability: Secrets exposed in Docker build arguments.
Docker build arguments are baked into image layers and can be extracted by anyone with access to the image using docker history or by inspecting the image manifest. This exposes the Freesound API credentials.
Solutions:
- Preferred: Pass these as runtime environment variables instead of build args, then configure them in your deployment environment (Kubernetes secrets, Docker Compose env vars, etc.)
- Alternative: Use Docker BuildKit secret mounts which don't persist in layers:
- name: Build and push
uses: docker/build-push-action@v6
with:
context: .
file: apps/web/Dockerfile
push: true
platforms: linux/amd64
tags: ${{ steps.meta.outputs.tags }}
labels: ${{ steps.meta.outputs.labels }}
secrets: |
FREESOUND_CLIENT_ID=${{ secrets.FREESOUND_CLIENT_ID }}
FREESOUND_API_KEY=${{ secrets.FREESOUND_API_KEY }}Then in your Dockerfile, mount them during build:
RUN --mount=type=secret,id=FREESOUND_CLIENT_ID \
--mount=type=secret,id=FREESOUND_API_KEY \
FREESOUND_CLIENT_ID=$(cat /run/secrets/FREESOUND_CLIENT_ID) \
FREESOUND_API_KEY=$(cat /run/secrets/FREESOUND_API_KEY) \
# your build commandHowever, since apps/web/src/env.ts shows these are now optional runtime variables, the best approach is to remove them from build args entirely and provide them at runtime.
Based on learnings, don't hardcode sensitive data like API keys and tokens.
🤖 Prompt for AI Agents
In @.github/workflows/build-opencut-web.yml around lines 58 - 60, The workflow
is exposing FREESOUND_CLIENT_ID and FREESOUND_API_KEY via Docker build-args
which bakes secrets into image layers; remove FREESOUND_CLIENT_ID and
FREESOUND_API_KEY from the build-args block in build-opencut-web.yml and instead
provide them as runtime environment variables (e.g., via your
deployment/Kubernetes/Docker Compose) or, if absolutely needed during build,
switch to BuildKit secrets via the action's secrets: input and mount them in the
Dockerfile; ensure apps/web/src/env.ts continues to read these as optional
runtime env vars rather than relying on build-time args.
| /** | ||
| * Redis (Upstash-compatible REST) | ||
| * If you are not using Upstash/serverless-redis-http yet, keep these optional. | ||
| * The app should handle "redis disabled" gracefully once we patch the routes. | ||
| */ | ||
| UPSTASH_REDIS_REST_URL: z.string().url().optional(), | ||
| UPSTASH_REDIS_REST_TOKEN: z.string().optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the env.ts file to see the actual changes
cat -n apps/web/src/env.ts | head -30Repository: OpenCut-app/OpenCut
Length of output: 1261
🏁 Script executed:
# Search for Redis client imports and initialization
rg -n "Upstash|upstash|Redis" --type ts --type tsx -C 2 | head -100Repository: OpenCut-app/OpenCut
Length of output: 90
🏁 Script executed:
# Search for environment variable usage in routes/API handlers
find apps/web/src -type f \( -name "*.ts" -o -name "*.tsx" \) | xargs grep -l "UPSTASH_REDIS" 2>/dev/nullRepository: OpenCut-app/OpenCut
Length of output: 113
🏁 Script executed:
# Look for Redis client instantiation and error handling
rg -n "new Redis|createClient|getClient" --type ts --type tsx -C 3Repository: OpenCut-app/OpenCut
Length of output: 90
🏁 Script executed:
# Examine the rate-limit.ts file to see how Redis is handled
cat -n apps/web/src/lib/rate-limit.tsRepository: OpenCut-app/OpenCut
Length of output: 594
🏁 Script executed:
# Search for all uses of the rate limiter in route handlers
rg -n "rate.limit|rateLimit|Ratelimit" --type ts -C 3Repository: OpenCut-app/OpenCut
Length of output: 3486
🏁 Script executed:
# Check for error handling around Redis/rate limiting
rg -n "catch|try|error|Error|redis|Redis" apps/web/src/lib/rate-limit.ts -C 2Repository: OpenCut-app/OpenCut
Length of output: 484
🏁 Script executed:
# Check one of the routes to see how it uses the rate limiter
head -30 apps/web/src/app/api/transcribe/route.tsRepository: OpenCut-app/OpenCut
Length of output: 1026
🏁 Script executed:
# Check if there's any try-catch or error handling in the route handlers
rg -n "try|catch|error|graceful|disabled" apps/web/src/app/api/transcribe/route.ts -C 2Repository: OpenCut-app/OpenCut
Length of output: 3105
Fix rate-limit.ts to handle missing Redis configuration—module instantiation will fail if env vars are undefined.
The current implementation instantiates the Redis client and Ratelimit at module load time without checking if UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN are defined. If these are undefined, the module will fail to load, preventing the entire application from starting. Routes cannot gracefully handle a missing rate limiter if initialization happens at the module level.
Modify apps/web/src/lib/rate-limit.ts to conditionally initialize the Ratelimit instance or provide a no-op fallback when Redis configuration is missing.
Current code at apps/web/src/lib/rate-limit.ts
const redis = new Redis({
url: env.UPSTASH_REDIS_REST_URL,
token: env.UPSTASH_REDIS_REST_TOKEN,
});
export const baseRateLimit = new Ratelimit({
redis,
limiter: Ratelimit.slidingWindow(100, "1 m"),
analytics: true,
prefix: "rate-limit",
});
🤖 Prompt for AI Agents
In @apps/web/src/env.ts around lines 16 - 22, The module currently constructs
Redis (new Redis) and exports baseRateLimit (new Ratelimit) at load time using
env.UPSTASH_REDIS_REST_URL and env.UPSTASH_REDIS_REST_TOKEN which can be
undefined; change rate-limit.ts to check those two env vars first and only
instantiate Redis and Ratelimit when both are present, otherwise export a no-op
fallback with the same public API as Ratelimit (e.g., an object exposing the
methods your routes call, such as `.limit()`/`.consume()` or whatever your code
uses) so callers can operate without a running Redis instance; reference symbols
to update: env.UPSTASH_REDIS_REST_URL, env.UPSTASH_REDIS_REST_TOKEN, Redis
(constructor usage), and baseRateLimit / Ratelimit (export) so initialization is
conditional and safe.
| /** | ||
| * Freesound (optional) | ||
| */ | ||
| FREESOUND_CLIENT_ID: z.string().optional(), | ||
| FREESOUND_API_KEY: z.string().optional(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find Freesound references and their usage patterns
# Search for FREESOUND variable usage in TypeScript/TSX files
echo "=== FREESOUND Variable Usage ==="
rg -n "FREESOUND" --type ts --type tsx -C 3
echo -e "\n=== Workflow file content (build args section) ==="
# Check the workflow file for build args
if [ -f ".github/workflows/build-opencut-web.yml" ]; then
cat -n ".github/workflows/build-opencut-web.yml" | grep -A 10 -B 5 "FREESOUND\|build.*args\|build-arg" | head -50
else
echo "Workflow file not found at .github/workflows/build-opencut-web.yml"
# Try to find it
find . -name "*build*" -type f | grep -i workflow
fi
echo -e "\n=== Env.ts file content (relevant section) ==="
# Show the env.ts file to see the full context
if [ -f "apps/web/src/env.ts" ]; then
cat -n "apps/web/src/env.ts" | head -50
fiRepository: OpenCut-app/OpenCut
Length of output: 2602
🏁 Script executed:
#!/bin/bash
# More comprehensive search for Freesound usage
echo "=== Searching ALL files for FREESOUND ==="
rg "FREESOUND" -i --type-list | head -5
rg "FREESOUND" -i -C 2
echo -e "\n=== Check Dockerfile usage ==="
if [ -f "apps/web/Dockerfile" ]; then
cat -n "apps/web/Dockerfile" | head -80
else
find . -name "Dockerfile*" -type f | head -5
ls -la apps/web/ | grep -i docker
fi
echo -e "\n=== Check if env vars are exported/used in Next.js config ==="
if [ -f "apps/web/next.config.js" ] || [ -f "apps/web/next.config.ts" ]; then
find apps/web -maxdepth 1 -name "next.config.*" -exec cat -n {} \;
fi
echo -e "\n=== Complete env.ts to see runtimeEnv mappings ==="
if [ -f "apps/web/src/env.ts" ]; then
cat -n "apps/web/src/env.ts" | tail -30
fiRepository: OpenCut-app/OpenCut
Length of output: 11621
🏁 Script executed:
#!/bin/bash
# Check the actual Freesound API route to see how it handles optional variables
echo "=== Freesound API Route Implementation ==="
if [ -f "apps/web/src/app/api/sounds/search/route.ts" ]; then
cat -n "apps/web/src/app/api/sounds/search/route.ts"
else
find . -name "*search*route*" -type f | head -5
fiRepository: OpenCut-app/OpenCut
Length of output: 9975
Missing optional variable validation in Freesound API route.
Line 153 in apps/web/src/app/api/sounds/search/route.ts uses env.FREESOUND_API_KEY directly without checking if it's defined. Since FREESOUND_API_KEY is declared as optional in env.ts, the endpoint should handle the case where it's missing. Currently, if the key is undefined, it gets sent to the Freesound API as the literal string "undefined", causing an authentication error with a misleading "Failed to search sounds" message.
The endpoint should either:
- Check if
env.FREESOUND_API_KEYexists before attempting the API call, and return an appropriate error (e.g., 503 with "Freesound integration not configured") - Or make
FREESOUND_API_KEYrequired instead of optional if the feature is essential
🤖 Prompt for AI Agents
In @apps/web/src/env.ts around lines 24 - 28, env.FREESOUND_API_KEY is optional
in the env schema but route.ts uses env.FREESOUND_API_KEY directly, causing
"undefined" to be sent to Freesound; update the search route handler
(apps/web/src/app/api/sounds/search/route.ts) to check for env.FREESOUND_API_KEY
at the start of the request handler and if missing return a proper HTTP error
(e.g., 503 with message like "Freesound integration not configured" or similar),
or alternatively change the env schema to make FREESOUND_API_KEY required if the
integration must always be available—choose one approach and implement the
corresponding guard and error response in the function that calls the Freesound
API.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/get-upload-url/route.ts (1)
54-78: Remove the redundant environment variable validation at lines 64–78.The
isTranscriptionConfigured()check at lines 36–52 already validates all four variables checked again at lines 64–78 (R2_ACCESS_KEY_ID,R2_SECRET_ACCESS_KEY,R2_BUCKET_NAME,CLOUDFLARE_ACCOUNT_ID). The second check is unreachable at runtime if the first check passes, making it unnecessary defensive code.If TypeScript type narrowing is the sole purpose (as the comment suggests), consider using a type assertion instead of re-validating at runtime:
const r2AccessKeyId = env.R2_ACCESS_KEY_ID as string; const r2SecretAccessKey = env.R2_SECRET_ACCESS_KEY as string; const r2BucketName = env.R2_BUCKET_NAME as string; const cloudflareAccountId = env.CLOUDFLARE_ACCOUNT_ID as string;Alternatively, remove this block entirely and rely on the first validation.
🧹 Nitpick comments (2)
apps/web/src/app/api/get-upload-url/route.ts (2)
32-52: Consider simplifying the explanatory comments.The comments at lines 32-35 appear to address a code reviewer ("This is your existing check; it produces the helpful error message") rather than documenting the code for future maintainers. Consider either removing these or condensing them to a brief comment about the runtime guard's purpose.
♻️ Suggested simplification
- /** - * Check transcription configuration (runtime guard) - * This is your existing check; it produces the helpful error message. - */ + // Check transcription configuration before proceeding const transcriptionCheck = isTranscriptionConfigured();
135-135: Optional: Consider structured logging.While the formatting changes are fine, note that coding guidelines discourage
consoleusage. However, for server-side API routes,console.erroris standard practice for error logging. If the project grows, consider adopting a structured logging library for better observability (log levels, structured data, log aggregation support).Also applies to: 149-149
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/app/api/get-upload-url/route.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of for loops when you don't need initializer and update expressions.
Don't reassign const variables....
Files:
apps/web/src/app/api/get-upload-url/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and...
Files:
apps/web/src/app/api/get-upload-url/route.ts
🔇 Additional comments (1)
apps/web/src/app/api/get-upload-url/route.ts (1)
104-105: LGTM! Correctly using type-guarded variables.The R2 client initialization and URL construction properly use the guarded local variables (
r2AccessKeyId,r2SecretAccessKey,r2BucketName,cloudflareAccountId) instead of direct environment access. This resolves TypeScript's type safety concerns while maintaining runtime correctness.Also applies to: 114-114
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/web/src/app/api/get-upload-url/route.ts:
- Around line 50-55: The nested ternary that assigns sortParam (const sortParam
= sort === "relevance" ? "score" : sort === "created" ? "created" :
`${sort}_desc`) should be replaced with a clearer construct; either implement a
small mapping object (e.g., const SORT_MAP = { relevance: "score", created:
"created" }; const sortParam = SORT_MAP[sort] ?? `${sort}_desc`) or use an
if/else chain (if (sort === "relevance") sortParam = "score"; else if (sort ===
"created") sortParam = "created"; else sortParam = `${sort}_desc`) to eliminate
the nested ternary and improve readability where sortParam is defined in
route.ts.
- Line 97: Replace the console.error call that reads "console.error(\"Error in
sounds search route:\", error);" with the project's structured logger (e.g.,
call processLogger.error or appLogger.error) and include the error object and
contextual message; if no logger is available in this module, import the shared
logger used across the app or add a TODO comment and use a temporary logger
wrapper, ensuring you do not leave any console.* calls in route.ts and that the
log call provides both the descriptive text and the error details.
- Around line 14-16: Update isFreesoundConfigured to return
!!env.FREESOUND_API_KEY instead of Boolean(env.FREESOUND_API_KEY) and ensure it
only checks FREESOUND_API_KEY (remove any reference to FREESOUND_CLIENT_ID);
also remove the console.error call referenced in the get-upload-url route
(replace with appropriate logger or silent handling per guidelines) so no direct
console.error remains in that handler.
🧹 Nitpick comments (3)
apps/web/src/app/api/get-upload-url/route.ts (3)
58-58: Consider removing the type assertion.While the type assertion is safe due to the configuration guard on line 21, it could be avoided by refactoring
isFreesoundConfiguredto be a type guard function.♻️ Optional refactor using a type guard
-function isFreesoundConfigured() { - return Boolean(env.FREESOUND_API_KEY); -} +function isFreesoundConfigured(): env is typeof env & { FREESOUND_API_KEY: string } { + return Boolean(env.FREESOUND_API_KEY); +}Then the type assertion on line 58 becomes unnecessary as TypeScript will narrow the type automatically.
70-80: Add timeout configuration for the external API call.The fetch call to the Freesound API lacks timeout configuration, which could lead to hanging requests if the external service is slow or unresponsive. This impacts reliability and user experience.
⏱️ Proposed timeout implementation
const response = await fetch( `https://freesound.org/apiv2/search/text/?${params.toString()}`, { method: "GET", headers: { Accept: "application/json", }, // avoid caching in builds/edge contexts cache: "no-store", + signal: AbortSignal.timeout(10000), // 10 second timeout } );Note:
AbortSignal.timeout()is available in Node.js 18+ and modern browsers.
18-107: Consider adding rate limiting for this public endpoint.This endpoint proxies requests to the Freesound API without apparent rate limiting (the AI summary notes rate limiting was removed). A public-facing endpoint that calls external APIs without rate limiting could:
- Exhaust Freesound API quotas or hit their rate limits
- Be abused as a proxy to bypass Freesound's rate limiting
- Lead to service degradation or unexpected costs
Consider implementing rate limiting using the available
UPSTASH_REDIS_REST_URLandUPSTASH_REDIS_REST_TOKEN(marked as optional in env.ts), or document that this endpoint should be protected at the infrastructure level (e.g., via Vercel rate limiting, API gateway, or firewall rules).If rate limiting via Redis was intentionally removed to support deployments without Redis, ensure that:
- Alternative rate limiting is configured at the infrastructure level
- This endpoint is documented as requiring protection
- The Freesound API has sufficient quotas for expected usage
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/app/api/get-upload-url/route.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use the comma operator.
Don't write functions that exceed a given Cognitive Complexity score.
Don't use unnecessary boolean casts.
Don't use unnecessary callbacks with flatMap.
Use for...of statements instead of Array.forEach.
Don't create classes that only have static members (like a static namespace).
Don't use this and super in static contexts.
Don't use unnecessary catch clauses.
Don't use unnecessary constructors.
Don't use unnecessary continue statements.
Don't export empty modules that don't change anything.
Don't use unnecessary escape sequences in regular expression literals.
Don't use unnecessary labels.
Don't use unnecessary nested block statements.
Don't rename imports, exports, and destructured assignments to the same name.
Don't use unnecessary string or template literal concatenation.
Don't use String.raw in template literals when there are no escape sequences.
Don't use useless case statements in switch statements.
Don't use ternary operators when simpler alternatives exist.
Don't use uselessthisaliasing.
Don't initialize variables to undefined.
Don't use the void operators (they're not familiar).
Use arrow functions instead of function expressions.
Use Date.now() to get milliseconds since the Unix Epoch.
Use .flatMap() instead of map().flat() when possible.
Use literal property access instead of computed property access.
Don't use parseInt() or Number.parseInt() when binary, octal, or hexadecimal literals work.
Use concise optional chaining instead of chained logical expressions.
Use regular expression literals instead of the RegExp constructor when possible.
Don't use number literal object member names that aren't base 10 or use underscore separators.
Remove redundant terms from logical expressions.
Use while loops instead of for loops when you don't need initializer and update expressions.
Don't reassign const variables....
Files:
apps/web/src/app/api/get-upload-url/route.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Don't use primitive type aliases or misleading types.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't return a value from a function with the return type 'void'.
Don't use the TypeScript directive @ts-ignore.
Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and type annotations.
Use eitherT[]orArray<T>consistently.
Initialize each enum member value explicitly.
Useexport typefor types.
Useimport typefor types.
Make sure all enum members are literal values.
Don't use TypeScript const enum.
Don't declare empty interfaces.
Don't let variables evolve into any type through reassignments.
Don't use the any type.
Don't misuse the non-null assertion operator (!) in TypeScript files.
Don't use implicit any type on variable declarations.
Don't merge interfaces and classes unsafely.
Don't use overload signatures that aren't next to each other.
Use the namespace keyword instead of the module keyword to declare TypeScript namespaces.
Use consistent accessibility modifiers on class properties and methods.
Use function types instead of object types with call signatures.
Don't use void type outside of generic or return types.
**/*.{ts,tsx}: Don't use TypeScript enums.
Don't export imported variables.
Don't add type annotations to variables, parameters, and class properties that are initialized with literal expressions.
Don't use TypeScript namespaces.
Don't use non-null assertions with the!postfix operator.
Don't use parameter properties in class constructors.
Don't use user-defined types.
Useas constinstead of literal types and...
Files:
apps/web/src/app/api/get-upload-url/route.ts
🧬 Code graph analysis (1)
apps/web/src/app/api/get-upload-url/route.ts (1)
apps/web/src/env.ts (1)
env(7-63)
| function isFreesoundConfigured() { | ||
| return Boolean(env.FREESOUND_API_KEY); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/web/src/app/api/get-upload-url/route.tsRepository: OpenCut-app/OpenCut
Length of output: 4006
🌐 Web query:
Freesound API v2 authentication requirements - does it require both client_id and api_key or token?
💡 Result:
Short answer: No — you use one strategy or the other.
- Token (API key) auth: add the api key (called "client secret/Api key" in the dashboard) as a token GET param or an Authorization: Token header. [1]
- OAuth2: use your client_id and client_secret to perform the OAuth2 flow and obtain an access token, then send requests with Authorization: Bearer ACCESS_TOKEN. [1][2]
So you do not need to supply client_id plus api_key on every request — either use the token (api key) method or the OAuth2 (client_id/secret → access token) method depending on what the endpoint requires. [1][2]
Remove unnecessary Boolean cast on line 15.
The isFreesoundConfigured() function uses an unnecessary Boolean() cast. Use !!env.FREESOUND_API_KEY instead. Additionally, the function only needs to check FREESOUND_API_KEY (not FREESOUND_CLIENT_ID) since the implementation uses Freesound's token authentication method, which requires only the API key.
Also remove the console.error() call on line 97 per coding guidelines.
🤖 Prompt for AI Agents
In @apps/web/src/app/api/get-upload-url/route.ts around lines 14 - 16, Update
isFreesoundConfigured to return !!env.FREESOUND_API_KEY instead of
Boolean(env.FREESOUND_API_KEY) and ensure it only checks FREESOUND_API_KEY
(remove any reference to FREESOUND_CLIENT_ID); also remove the console.error
call referenced in the get-upload-url route (replace with appropriate logger or
silent handling per guidelines) so no direct console.error remains in that
handler.
| const sortParam = | ||
| sort === "relevance" | ||
| ? "score" | ||
| : sort === "created" | ||
| ? "created" | ||
| : `${sort}_desc`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid nested ternary expressions.
The sort parameter mapping uses nested ternary expressions, which violates the coding guidelines: "Don't use nested ternary expressions."
As per coding guidelines, refactor to use if-else statements or a mapping object:
♻️ Proposed refactor using if-else statements
- const sortParam =
- sort === "relevance"
- ? "score"
- : sort === "created"
- ? "created"
- : `${sort}_desc`;
+ let sortParam: string;
+ if (sort === "relevance") {
+ sortParam = "score";
+ } else if (sort === "created") {
+ sortParam = "created";
+ } else {
+ sortParam = `${sort}_desc`;
+ }Or using a mapping object:
- const sortParam =
- sort === "relevance"
- ? "score"
- : sort === "created"
- ? "created"
- : `${sort}_desc`;
+ const sortMap: Record<string, string> = {
+ relevance: "score",
+ created: "created",
+ };
+ const sortParam = sortMap[sort] ?? `${sort}_desc`;📝 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.
| const sortParam = | |
| sort === "relevance" | |
| ? "score" | |
| : sort === "created" | |
| ? "created" | |
| : `${sort}_desc`; | |
| const sortMap = { | |
| relevance: "score", | |
| created: "created", | |
| }; | |
| const sortParam = sortMap[sort] ?? `${sort}_desc`; |
🤖 Prompt for AI Agents
In @apps/web/src/app/api/get-upload-url/route.ts around lines 50 - 55, The
nested ternary that assigns sortParam (const sortParam = sort === "relevance" ?
"score" : sort === "created" ? "created" : `${sort}_desc`) should be replaced
with a clearer construct; either implement a small mapping object (e.g., const
SORT_MAP = { relevance: "score", created: "created" }; const sortParam =
SORT_MAP[sort] ?? `${sort}_desc`) or use an if/else chain (if (sort ===
"relevance") sortParam = "score"; else if (sort === "created") sortParam =
"created"; else sortParam = `${sort}_desc`) to eliminate the nested ternary and
improve readability where sortParam is defined in route.ts.
| return NextResponse.json(data); | ||
| } catch (error) { | ||
| console.error("Error generating upload URL:", error); | ||
| console.error("Error in sounds search route:", error); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace console.error with proper logging.
The coding guidelines specify: "Don't use console." Use a proper logging library or a structured logging approach instead.
As per coding guidelines, consider using a logging library or framework-specific logging mechanism:
♻️ Suggested alternatives
If using a logging library like pino or winston:
- console.error("Error in sounds search route:", error);
+ logger.error({ error, route: "/api/get-upload-url" }, "Error in sounds search route");Or if logging infrastructure isn't set up yet, consider adding a TODO:
- console.error("Error in sounds search route:", error);
+ // TODO: Replace with proper logging infrastructure
+ // Temporary console usage for error tracking
+ console.error("Error in sounds search route:", error);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/src/app/api/get-upload-url/route.ts at line 97, Replace the
console.error call that reads "console.error(\"Error in sounds search route:\",
error);" with the project's structured logger (e.g., call processLogger.error or
appLogger.error) and include the error object and contextual message; if no
logger is available in this module, import the shared logger used across the app
or add a TODO comment and use a temporary logger wrapper, ensuring you do not
leave any console.* calls in route.ts and that the log call provides both the
descriptive text and the error details.
1 participant
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Screenshots (if applicable)
Add screenshots to help explain your changes.
Checklist:
Additional context
Add any other context about the pull request here.
Summary by CodeRabbit
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.