Main #616
Main #616
Conversation
- Updated package.json to include Vitest and Testing Library dependencies. - Created GitHub Actions workflow for CI with linting, type checking, building, and testing jobs. - Added unit tests for environment configuration and root layout component. - Configured Vitest with appropriate settings and setup files.
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
@Nitesh-Badgujar-28906 is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds CI for lint/build/test via GitHub Actions, introduces Vitest setup for the web app (config, setup file), adds basic env and RootLayout tests, and updates package scripts and dependencies to support testing. Includes artifact upload and coverage steps in CI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as GitHub
participant CI as GitHub Actions
participant Build as Job: build-test
participant Tests as Job: tests
participant Art as Artifacts
Dev->>CI: push / pull_request to main
CI->>Build: Start build-test
Build->>Build: Setup Bun, restore Turbo cache
Build->>Build: Install deps (bun install)
Build->>Build: Lint, Type-check, Build
Build-->>Art: Upload web standalone artifact
Build-->>CI: Success/Failure
alt on success
CI->>Tests: Start tests (needs: build-test)
Tests->>Tests: Setup Bun, install deps
Tests->>Tests: bun run test (vitest)
Tests-->>Art: Upload coverage
Tests-->>CI: Status
else on failure
CI-->>Dev: build-test failed, tests skipped
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/package.json (1)
76-91: Declare test dependencies in apps/web
Tests in this package import@testing-library/reactand@testing-library/jest-dom, but they’re only listed in the root. Add them toapps/web’sdevDependencies(e.g.@testing-library/react: ^16.0.1,@testing-library/jest-dom: ^6.6.3) so CI and local test runs install them correctly.
🧹 Nitpick comments (13)
apps/web/vitest.config.ts (2)
2-9: Use ESM-safe dirname resolution in Vite/Vitest configUsing __dirname can break under ESM. Prefer import.meta.url + fileURLToPath to resolve paths reliably.
Apply this diff:
-import path from 'node:path'; +import { resolve, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); @@ resolve: { alias: { - '@': path.resolve(__dirname, './src'), + '@': resolve(__dirname, './src'), }, },
14-16: Optional: Broaden test discovery patternsIf you also use .spec.{ts,tsx} files, include them now to avoid future misses.
Apply this diff:
- include: ['src/**/*.test.{ts,tsx}'], + include: ['src/**/*.{test,spec}.{ts,tsx}'],apps/web/vitest.setup.ts (1)
1-4: Prefer Vitest-specific Jest-DOM entry for better TS typingsUsing '@testing-library/jest-dom/vitest' provides matcher typings for Vitest without additional tsconfig tweaks.
Apply this diff:
-import '@testing-library/jest-dom'; +import '@testing-library/jest-dom/vitest';apps/web/src/components/__tests__/root-layout.test.tsx (3)
2-2: Remove React import in testsPer guidelines, don’t import React in .tsx. It’s unnecessary with the automatic JSX runtime.
Apply this diff:
-import React from 'react';As per coding guidelines
4-4: Use path alias for consistencyPrefer '@/app/layout' to leverage your configured alias and avoid fragile relative paths.
Apply this diff:
-import OriginalRootLayout from '../../app/layout'; +import OriginalRootLayout from '@/app/layout';
16-20: Make the smoke test actually render the layoutRender the component and assert children presence. This also removes the unused import risk if you keep render.
Apply this diff:
-describe('RootLayout', () => { - it('is a defined component', () => { - expect(OriginalRootLayout).toBeTypeOf('function'); - }); -}); +describe('RootLayout', () => { + it('renders without crashing and shows children', () => { + const { getByTestId } = render( + <OriginalRootLayout> + <div data-testid="child" /> + </OriginalRootLayout>, + ); + expect(getByTestId('child')).toBeTruthy(); + }); +});apps/web/package.json (1)
36-36: Upgrade botid to ^1.5.x. Verified imports from "botid/client" remain correct.apps/web/src/app/env.test.ts (1)
19-19: Optional: simplify relative import path.From apps/web/src/app to apps/web/src/env.ts, ../env is sufficient. Not required after dynamic import change, but keep paths minimal for readability.
package.json (2)
24-27: Use pinned local CLI instead of npx latest for reproducibility.npx ultracite@latest ignores your devDependency pin and can cause drift in CI. Prefer the locally installed version.
Apply this diff:
- "lint": "npx ultracite@latest lint", - "format": "npx ultracite@latest format", + "lint": "ultracite lint", + "format": "ultracite format",
10-14: Check if all Testing Library packages are needed at root.If tests only run in apps/web, consider scoping these devDeps to that workspace to reduce root install surface.
.github/workflows/ci.yml (3)
31-40: Turbo cache key should hash Bun’s lockfile.You’re hashing bun.lock (nonexistent) instead of bun.lockb. Include turbo.json too for better cache busting.
Apply this diff:
- key: turbo-${{ github.workflow }}-${{ github.job }}-${{ github.ref }}-${{ hashFiles('**/package.json', 'bun.lock') }} + key: turbo-${{ github.workflow }}-${{ github.job }}-${{ github.ref }}-${{ hashFiles('**/package.json', 'bun.lockb', 'turbo.json') }} restore-keys: | turbo-${{ github.workflow }}-${{ github.job }}-${{ github.ref }}- turbo-${{ github.workflow }}-${{ github.job }}- turbo-${{ github.workflow }}-
61-69: Run tests independently of build to get faster feedback.Chaining tests on build means tests won’t run if build fails. Consider removing needs: build-test so tests run in parallel.
13-22: Harden workflow token permissions (principle of least privilege).Set default read-only permissions at the workflow level.
Apply this diff:
name: CI on: push: branches: [main] pull_request: branches: [main] +permissions: + contents: read + concurrency:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
.github/tests:(1 hunks).github/workflows/ci.yml(1 hunks)apps/web/package.json(2 hunks)apps/web/src/app/env.test.ts(1 hunks)apps/web/src/components/__tests__/root-layout.test.tsx(1 hunks)apps/web/vitest.config.ts(1 hunks)apps/web/vitest.setup.ts(1 hunks)package.json(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{test,spec}.{js,jsx,ts,tsx}: Don't use export or module.exports in test files.
Don't use focused tests.
Make sure the assertion function, like expect, is placed inside an it() function call.
Don't use disabled tests.
Don't nest describe() blocks too deeply in test files.
Don't use callbacks in asynchronous tests and hooks.
Don't have duplicate hooks in describe blocks.
Files:
apps/web/src/app/env.test.tsapps/web/src/components/__tests__/root-layout.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{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 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.
Don't use empty type parameters in type aliases and interfaces.
Don't use any or unknown as type constraints.
Don't use the TypeScript directive @ts-ignore.
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 primitive type aliases or misleading types
Don't use the TypeScript directive @ts-ignore
Don't use TypeScript enums
Use either T[] or Array consistently
Don't use the any type
Files:
apps/web/src/app/env.test.tsapps/web/vitest.setup.tsapps/web/vitest.config.tsapps/web/src/components/__tests__/root-layout.test.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,ts,tsx}: Don't use the return value of React.render.
Don't use consecutive spaces in regular expression literals.
Don't use theargumentsobject.
Don't use primitive type aliases or misleading types.
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...
Files:
apps/web/src/app/env.test.tsapps/web/vitest.setup.tsapps/web/vitest.config.tsapps/web/src/components/__tests__/root-layout.test.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{ts,tsx,js,jsx}: Don't use the comma operator
Use for...of statements instead of Array.forEach
Don't initialize variables to undefined
Use .flatMap() instead of map().flat() when possible
Don't assign a value to itself
Avoid unused imports and variables
Don't use await inside loops
Don't hardcode sensitive data like API keys and tokens
Don't use the delete operator
Don't use global eval()
Use String.slice() instead of String.substr() and String.substring()
Don't use else blocks when the if block breaks early
Put default function parameters and optional function parameters last
Use new when throwing an error
Use String.trimStart() and String.trimEnd() over String.trimLeft() and String.trimRight()
Files:
apps/web/src/app/env.test.tsapps/web/vitest.setup.tsapps/web/vitest.config.tsapps/web/src/components/__tests__/root-layout.test.tsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{jsx,tsx}: Don't useaccessKeyattribute on any HTML element.
Don't setaria-hidden="true"on focusable elements.
Don't add ARIA roles, states, and properties to elements that don't support them.
Don't use distracting elements like<marquee>or<blink>.
Only use thescopeprop on<th>elements.
Don't assign non-interactive ARIA roles to interactive HTML elements.
Make sure label elements have text content and are associated with an input.
Don't assign interactive ARIA roles to non-interactive HTML elements.
Don't assigntabIndexto non-interactive HTML elements.
Don't use positive integers fortabIndexproperty.
Don't include "image", "picture", or "photo" in img alt prop.
Don't use explicit role property that's the same as the implicit/default role.
Make static elements with click handlers use a valid role attribute.
Always include atitleelement for SVG elements.
Give all elements requiring alt text meaningful information for screen readers.
Make sure anchors have content that's accessible to screen readers.
AssigntabIndexto non-interactive HTML elements witharia-activedescendant.
Include all required ARIA attributes for elements with ARIA roles.
Make sure ARIA properties are valid for the element's supported roles.
Always include atypeattribute for button elements.
Make elements with interactive roles and handlers focusable.
Give heading elements content that's accessible to screen readers (not hidden witharia-hidden).
Always include alangattribute on the html element.
Always include atitleattribute for iframe elements.
AccompanyonClickwith at least one of:onKeyUp,onKeyDown, oronKeyPress.
AccompanyonMouseOver/onMouseOutwithonFocus/onBlur.
Include caption tracks for audio and video elements.
Use semantic elements instead of role attributes in JSX.
Make sure all anchors are valid and navigable.
Ensure all ARIA properties (aria-*) are valid.
Use valid, non-abstract ARIA roles for elements with...
Files:
apps/web/src/components/__tests__/root-layout.test.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/ultracite.mdc)
**/*.{tsx,jsx}: Always include a title element for icons unless there's text beside the icon
Always include a type attribute for button elements
Accompany onClick with at least one of: onKeyUp, onKeyDown, or onKeyPress
Accompany onMouseOver/onMouseOut with onFocus/onBlur
Don't import React itself
Don't define React components inside other components
Don't use both children and dangerouslySetInnerHTML on the same element
Don't insert comments as text nodes
Use <>...</> instead of ...
Files:
apps/web/src/components/__tests__/root-layout.test.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-09T09:03:49.797Z
Learnt from: CR
PR: OpenCut-app/OpenCut#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-09T09:03:49.797Z
Learning: Applies to **/*.{test,spec}.{js,jsx,ts,tsx} : Make sure the assertion function, like expect, is placed inside an it() function call.
Applied to files:
apps/web/vitest.setup.ts
🧬 Code graph analysis (1)
apps/web/src/app/env.test.ts (1)
apps/web/src/env.ts (1)
env(7-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Vercel Agent Review
🔇 Additional comments (3)
apps/web/src/components/__tests__/root-layout.test.tsx (1)
7-9: No changes needed for BotIdClient mock
The test’svi.mock('botid/client')aligns with the actual import in apps/web/src/app/layout.tsx.apps/web/package.json (1)
13-14: Ensure Vitest is resolvable in apps/web
Vitest is declared only in the root package.json (v^2.1.3); confirm your package manager hoists root devDependencies into this workspace or add vitest to apps/web/devDependencies..github/workflows/ci.yml (1)
53-59: Verify Next.js standalone artifact path.apps/web/.next/standalone exists only with output: 'standalone' configured. If not set, this artifact step is a no-op. Confirm next.config.js.
| tests: | ||
| name: Tests | ||
| runs-on: ubuntu-latest | ||
| needs: build-test | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: oven-sh/setup-bun@v2 | ||
| - run: bun install --frozen-lockfile | ||
| - run: bun run test -- --reporter=dot No newline at end of file |
There was a problem hiding this comment.
Invalid workflow file (wrong location and incomplete syntax)
This file is not under .github/workflows and lacks required top-level keys (name, on, jobs). It won’t run and may confuse contributors. You already have .github/workflows/ci.yml with a tests job.
Recommended: remove this file to avoid duplication.
- tests:
- name: Tests
- runs-on: ubuntu-latest
- needs: build-test
- steps:
- - uses: actions/checkout@v4
- - uses: oven-sh/setup-bun@v2
- - run: bun install --frozen-lockfile
- - run: bun run test -- --reporter=dotAlternatively, if you want a standalone workflow, place it at .github/workflows/tests.yml and add a proper trigger:
name: tests
on:
pull_request:
push:
branches: [main]
jobs:
tests:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: oven-sh/setup-bun@v2
- run: bun install --frozen-lockfile
- run: bun run test -- --reporter=dot🤖 Prompt for AI Agents
.github/tests (lines 1-9): This file is in the wrong location and has incomplete
workflow syntax so it will never run; either delete this file to avoid
duplication with .github/workflows/ci.yml, or move it to
.github/workflows/tests.yml and convert it into a valid GitHub Actions workflow
by adding the required top-level keys (name, on, jobs) and wrapping the current
job under jobs.tests with triggers (for example on: pull_request and push to
main) so the steps remain the same.
| env: | ||
| TURBO_TELEMETRY_DISABLED: 1 | ||
| # Prevent accidental production secrets usage | ||
| NODE_ENV: development | ||
| steps: |
There was a problem hiding this comment.
Prevent env validation from breaking builds in CI.
apps/web/src/env.ts enforces required server env vars at import-time. Without secrets in CI, next build may fail. Set SKIP_ENV_VALIDATION for the build job.
Apply this diff:
env:
TURBO_TELEMETRY_DISABLED: 1
# Prevent accidental production secrets usage
NODE_ENV: development
+ SKIP_ENV_VALIDATION: "1"📝 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.
| env: | |
| TURBO_TELEMETRY_DISABLED: 1 | |
| # Prevent accidental production secrets usage | |
| NODE_ENV: development | |
| steps: | |
| env: | |
| TURBO_TELEMETRY_DISABLED: 1 | |
| # Prevent accidental production secrets usage | |
| NODE_ENV: development | |
| SKIP_ENV_VALIDATION: "1" | |
| steps: |
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 18-22, CI builds fail because
apps/web/src/env.ts validates required server env vars at import-time; set
SKIP_ENV_VALIDATION in the build job's env to bypass that validation in CI. Add
SKIP_ENV_VALIDATION: 1 (or "true") to the env block for the build job (where
TURBO_TELEMETRY_DISABLED and NODE_ENV are defined) so the Next.js build won't
error when secrets are absent, leaving other job-level envs unchanged.
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
New Features
Tests
Chores
Scripts