feat(effects): add @opencut/effects package with 24 effects and presets#750
Open
doananh234 wants to merge 10 commits intoOpenCut-app:mainfrom
Open
feat(effects): add @opencut/effects package with 24 effects and presets#750doananh234 wants to merge 10 commits intoOpenCut-app:mainfrom
doananh234 wants to merge 10 commits intoOpenCut-app:mainfrom
Conversation
Add a standalone effects package that extracts effect definitions, GLSL shaders, and registry from @opencut/web into a reusable package. ## Package (@opencut/effects) - Registry: register/get/has/getAll effects with Map-based lookup - Categories: COLOR_TONE, ARTISTIC, BEAUTY with display metadata - Types: Effect, EffectDefinition, EffectContext, WebGLEffectPass ### Effects (24 total) **Color & Tone (8):** brightness, contrast, saturation, hue-shift, temperature, exposure, gamma, color-balance **Artistic (10):** blur, vignette, film-grain, sharpen, pixelate, chromatic-aberration, glitch (2-pass), neon-glow (2-pass), sketch (2-pass), oil-paint (2-pass) **Beauty (6):** skin-smooth (bilateral filter), face-brighten, eye-enhance, teeth-whiten, blush, slim-face ### Presets (12) - Instagram: clarendon, juno, lark, gingham, valencia, nashville - Cinematic: teal-and-orange, vintage-film, noir, bleach-bypass - Beauty: soft-glow, portrait ## Web App Integration - Migrate blur effect from web app to package - Update imports via re-export pattern (backward compatible) - Pass time param for animated effects (film-grain, glitch) - Add face-mesh service using MediaPipe Face Mesh (lazy-loaded) - Beauty effects auto-detect faces and pass EffectContext - Async renderVisual for face detection pipeline ## Tests 152 unit tests covering registry, all effect definitions, and presets
|
Someone is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant VisualNode
participant FaceMesh as Face Mesh Service
participant EffectsPkg as `@opencut/effects`
participant WebGL
Client->>VisualNode: render(frame, timelineTime)
VisualNode->>VisualNode: gather enabled effects
alt any enabled effect.category == BEAUTY
VisualNode->>FaceMesh: detectFace(canvas)
FaceMesh-->>VisualNode: EffectContext (cheek/jaw/faceDetected)
else
VisualNode->>VisualNode: context = undefined
end
VisualNode->>EffectsPkg: getEffect(effectType)
EffectsPkg-->>VisualNode: EffectDefinition
VisualNode->>WebGL: render passes with uniforms { effectParams, width, height, time, context? }
WebGL-->>VisualNode: rendered output
VisualNode-->>Client: render complete (async)
sequenceDiagram
participant App
participant WebAppEffects as Web App Effects Module
participant EffectsPkg as `@opencut/effects`
participant Registry
App->>WebAppEffects: import registerDefaultEffects
App->>WebAppEffects: registerDefaultEffects()
WebAppEffects->>EffectsPkg: registerAllEffects()
EffectsPkg->>Registry: registerEffect(definition)...
Registry-->>EffectsPkg: registered
App->>WebAppEffects: getEffect(type)
WebAppEffects->>EffectsPkg: getEffect(type)
EffectsPkg->>Registry: lookup
Registry-->>EffectsPkg: EffectDefinition
WebAppEffects-->>App: EffectDefinition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…en, and vignette effects - Implement tests for chromatic aberration effect definition, verifying type, name, category, parameters, and renderer behavior. - Add tests for glitch effect definition, covering type, name, parameters, and rendering logic. - Create tests for sharpen effect definition, ensuring correct parameter defaults and rendering. - Introduce tests for vignette effect definition, validating parameters and rendering behavior. These tests enhance coverage for the new effects introduced in the @opencut/effects package.
doananh234
force-pushed
the
feat/effects-package
branch
from
4081a7d to
7e40be9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (14)
packages/effects/package.json (1)
15-16: Consider pinning@types/bunto a specific version.Using
"latest"for@types/buncan lead to non-reproducible builds if the types package introduces breaking changes. Consider pinning to a specific version or using a caret range.♻️ Suggested change
"devDependencies": { - "@types/bun": "latest", + "@types/bun": "^1.2.0", "tsup": "^8.4.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/package.json` around lines 15 - 16, Replace the floating "latest" version for the `@types/bun` devDependency with a pinned or caret-ranged version to ensure reproducible builds; edit the "devDependencies" entry for "@types/bun" in package.json (the dependency key "@types/bun") and set it to a specific semver like "0.4.0" or a caret range like "^0.4.0" instead of "latest", then run your lockfile update (npm/yarn/pnpm install) to persist the fixed version.packages/effects/src/effects/temperature/index.ts (1)
26-28: Consider extracting the normalization factor.The magic number
400could be a named constant for improved readability, though this is a minor suggestion.♻️ Optional: Extract normalization constant
+const TEMPERATURE_NORMALIZATION_FACTOR = 400; + export const temperatureEffectDefinition: EffectDefinition = { // ... renderer: { type: "webgl", passes: [ { fragmentShader, uniforms: ({ effectParams }) => ({ - u_temperature: (effectParams.warmth as number) / 400, + u_temperature: (effectParams.warmth as number) / TEMPERATURE_NORMALIZATION_FACTOR, }), }, ], }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/effects/temperature/index.ts` around lines 26 - 28, The division by the magic number 400 in the uniforms function should be replaced with a named constant for clarity; introduce a constant (e.g., TEMPERATURE_NORMALIZATION = 400) near the top of the module or file and use it in uniforms: ({ effectParams }) => ({ u_temperature: (effectParams.warmth as number) / TEMPERATURE_NORMALIZATION }), updating any import/exports if needed and keeping the symbol names uniforms, effectParams, and u_temperature unchanged.packages/effects/tests/beauty-effects.test.ts (2)
21-40: Use full identifier names in the shared validation helper.Short names like
defandpmake this generic test helper harder to maintain.🧹 Suggested rename refactor
-function validateBeautyEffect(def: EffectDefinition, type: string) { - test(`${type}: correct type and BEAUTY category`, () => { - expect(def.type).toBe(type); - expect(def.category).toBe(EffectCategory.BEAUTY); +function validateBeautyEffect(effectDefinition: EffectDefinition, effectType: string) { + test(`${effectType}: correct type and BEAUTY category`, () => { + expect(effectDefinition.type).toBe(effectType); + expect(effectDefinition.category).toBe(EffectCategory.BEAUTY); }); - test(`${type}: uniforms work without context (graceful fallback)`, () => { + test(`${effectType}: uniforms work without context (graceful fallback)`, () => { const defaults: Record<string, number | string | boolean> = {}; - for (const p of def.params) defaults[p.key] = p.default; - for (const pass of def.renderer.passes) { - const result = pass.uniforms({ effectParams: defaults, ...defaultArgs }); + for (const parameter of effectDefinition.params) defaults[parameter.key] = parameter.default; + for (const renderPass of effectDefinition.renderer.passes) { + const result = renderPass.uniforms({ effectParams: defaults, ...defaultArgs }); expect(typeof result).toBe("object"); } }); - test(`${type}: uniforms work with face context`, () => { + test(`${effectType}: uniforms work with face context`, () => { const defaults: Record<string, number | string | boolean> = {}; - for (const p of def.params) defaults[p.key] = p.default; - for (const pass of def.renderer.passes) { - const result = pass.uniforms({ effectParams: defaults, ...defaultArgs, context: mockContext }); + for (const parameter of effectDefinition.params) defaults[parameter.key] = parameter.default; + for (const renderPass of effectDefinition.renderer.passes) { + const result = renderPass.uniforms({ effectParams: defaults, ...defaultArgs, context: mockContext }); expect(typeof result).toBe("object"); } }); }As per coding guidelines, "Never abbreviate variable and parameter names. Use
eventnote,elementnotel. Prefer full, readable names that are easy for AI agents to understand and modify".Also applies to: 63-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/tests/beauty-effects.test.ts` around lines 21 - 40, The helper function validateBeautyEffect uses abbreviated identifiers (def, p, pass) which reduces readability; rename them to full, descriptive names across the function—e.g., rename parameter def to effectDefinition, loop variable p to param or parameterDefinition, pass to rendererPass, and defaults to defaultValues—update all usages including effectDefinition.params, effectDefinition.renderer.passes, and the uniforms calls (pass.uniforms -> rendererPass.uniforms) and similarly in the face-context test; apply the same renames in the other shared validation helper occurrences mentioned so identifiers are consistent and self-descriptive.
27-33: Harden uniform return assertions to rejectnull.
typeof nullis"object", so these assertions can pass for invalid returns.✅ Suggested test hardening
- expect(typeof result).toBe("object"); + expect(result).not.toBeNull(); + expect(typeof result).toBe("object");- expect(typeof result).toBe("object"); + expect(result).not.toBeNull(); + expect(typeof result).toBe("object");Also applies to: 36-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/tests/beauty-effects.test.ts` around lines 27 - 33, The test currently asserts uniforms returns an "object" using typeof which allows null; change the assertion in the test `${type}: uniforms work without context (graceful fallback)` (and the similar block covering def.renderer.passes) to explicitly reject null and ensure a plain object, e.g. assert that result !== null && typeof result === "object" or use expect(result).not.toBeNull() followed by expect(typeof result).toBe("object"); locate the loop that calls pass.uniforms({ effectParams: defaults, ...defaultArgs }) and update its assertions accordingly.packages/effects/src/effects/vignette/index.ts (1)
22-22: Replace the approximated constant with a standard constant.
0.707is a magic approximation and can drift semantically over time. UseMath.SQRT1_2to make intent explicit.♻️ Suggested change
- u_radius: (effectParams.radius as number) / 100 * 0.707, + u_radius: ((effectParams.radius as number) / 100) * Math.SQRT1_2,As per coding guidelines, "Use standard constants instead of approximated literals".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/effects/vignette/index.ts` at line 22, The literal 0.707 in the u_radius assignment is a magic number; update the expression that sets u_radius (currently using (effectParams.radius as number) / 100 * 0.707) to use the standard constant Math.SQRT1_2 instead of 0.707 so the intent is explicit and less error-prone.packages/effects/src/presets/registry.ts (1)
23-25: Use descriptive variable name in filter callback.The parameter
pshould be renamed topresetfor consistency with coding guidelines.♻️ Proposed fix
export function getPresetsByCategory(category: PresetCategory): EffectPreset[] { - return getAllPresets().filter((p) => p.category === category); + return getAllPresets().filter((preset) => preset.category === category); }As per coding guidelines: "Never abbreviate variable and parameter names... Prefer full, readable names."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/presets/registry.ts` around lines 23 - 25, In getPresetsByCategory, rename the filter callback parameter from the single-letter p to a descriptive name (e.g., preset) for readability and guideline consistency; update the lambda in getAllPresets().filter((p) => p.category === category) to use getAllPresets().filter((preset) => preset.category === category) so the callback and any inline references use the full name.apps/web/src/services/renderer/nodes/visual-node.ts (1)
132-140: Use descriptive variable names instead of abbreviations.The callback uses abbreviated names
eanddefwhich violates the coding guidelines requiring full, readable names.♻️ Proposed fix
// Detect face once per frame if any beauty effect is active - const hasBeautyEffect = enabledEffects.some((e) => { - const def = getEffect({ effectType: e.type }); - return def.category === EffectCategory.BEAUTY; + const hasBeautyEffect = enabledEffects.some((effect) => { + const definition = getEffect({ effectType: effect.type }); + return definition.category === EffectCategory.BEAUTY; });As per coding guidelines: "Never abbreviate variable and parameter names. Use
eventnote,elementnotel. Prefer full, readable names."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/renderer/nodes/visual-node.ts` around lines 132 - 140, Replace the abbreviated callback parameter names in the beauty-effect detection logic: in the enabledEffects.some callback change the short names `e` and `def` to descriptive names like `effect` and `effectDefinition` (or `effectDef`) and update the getEffect call accordingly (getEffect({ effectType: effect.type })); keep the rest of the logic intact that checks EffectCategory.BEAUTY, and ensure variables around this block (hasBeautyEffect, faceContext, detectFace, elementCanvas) are referenced consistently.packages/effects/src/presets/types.ts (1)
2-17: Preferexport typefor these exported type shapes.This file exports type-only contracts, so switch
interfaceexports toexport typeto align with repository conventions.♻️ Suggested change
-export interface EffectPresetEntry { +export type EffectPresetEntry = { /** Must match a registered effect type (e.g., "brightness") */ effectType: string; params: Record<string, number | string | boolean>; -} +}; export type PresetCategory = "instagram" | "cinematic" | "beauty" | "custom"; /** Curated combination of effects with preset parameter values */ -export interface EffectPreset { +export type EffectPreset = { type: string; name: string; category: PresetCategory; description: string; effects: EffectPresetEntry[]; -} +};Based on learnings: Applies to **/*.{ts,tsx} : Use
export typefor types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/presets/types.ts` around lines 2 - 17, Replace the exported interfaces with exported type aliases to follow repo convention: change EffectPresetEntry and EffectPreset from "export interface" to "export type" keeping the same property shapes and JSDoc comments (retain effectType, params: Record<string, number | string | boolean>, and for EffectPreset keep type, name, category: PresetCategory, description, effects: EffectPresetEntry[]); leave PresetCategory as-is (it's already an exported type). Ensure the exported symbol names (EffectPresetEntry, PresetCategory, EffectPreset) remain unchanged so other modules keep importing them.packages/effects/src/effects/blur/blur.test.ts (1)
59-82: Add a malformed-input test forintensity.Current tests only verify numeric values. Add one case for invalid
intensity(e.g.,"abc"orundefined) to lock in finiteu_sigmabehavior and prevent NaN regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/effects/blur/blur.test.ts` around lines 59 - 82, Add a test in blur.test.ts that calls blurEffectDefinition.renderer.passes[0].uniforms with malformed intensity inputs (e.g., intensity: "abc" and intensity: undefined) and assert that the returned uniforms.u_sigma is a finite number (not NaN) and falls back to the minimum value (e.g., 0.001) so invalid inputs cannot produce NaN; reuse the existing pass1.uniforms invocation pattern and assertions similar to the "sigma is never zero" test to lock in this behavior.packages/effects/tests/registry.test.ts (1)
45-49: Use full variable namedefinitioninstead of abbreviateddef.Per coding guidelines, variable names should not be abbreviated. The abbreviation
defshould be expanded todefinitionfor clarity.Proposed fix
test("registerEffect adds effect to registry", () => { - const def = createStubDefinition("blur"); - registerEffect({ definition: def }); + const definition = createStubDefinition("blur"); + registerEffect({ definition }); expect(hasEffect({ effectType: "blur" })).toBe(true); });This pattern should be applied consistently throughout the file (lines 46, 56, 70-71, 79-85, 92-93). As per coding guidelines: "Never abbreviate variable and parameter names."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/tests/registry.test.ts` around lines 45 - 49, Rename the abbreviated variable def to the full name definition in the tests so they follow the coding guideline (e.g., in the test that calls createStubDefinition("blur") and then registerEffect({ definition: def }) change both the variable and its use to definition), and apply the same rename for all occurrences in this file (the tests referencing createStubDefinition, registerEffect, and hasEffect across the noted blocks) so signatures and assertions use definition instead of def.packages/effects/src/presets/index.ts (1)
27-30: Consider adding explicit return type annotation.The
getAllBundledPresetsfunction could benefit from an explicit return type for better API documentation and type safety at call sites.Proposed improvement
+import type { EffectPreset } from "./types"; + /** Get all bundled presets without registering */ -export function getAllBundledPresets() { +export function getAllBundledPresets(): EffectPreset[] { return [...allPresets]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/presets/index.ts` around lines 27 - 30, Add an explicit return type to the exported function getAllBundledPresets so callers see the API shape and get compile-time safety; change its signature to declare the actual array element type (matching the type of allPresets) — e.g., export function getAllBundledPresets(): PresetType[] — and ensure PresetType (or the correct existing type) is imported/referenced so the function returns [...allPresets] with a matching annotated return type.apps/web/src/services/face-mesh/face-mesh-provider.ts (1)
42-43: Use the app logger instead ofconsole.warn.This bypasses the normal logging pipeline in a new shared service. As per coding guidelines, "Don't use console."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/face-mesh/face-mesh-provider.ts` around lines 42 - 43, Replace the console.warn call in the catch block that logs "[face-mesh] Failed to load MediaPipe:" and the err variable with the application's logger so the message flows through the shared logging pipeline; locate the catch block where err is logged and swap console.warn for the module's logger method (e.g., logger.warn or logger.error) while preserving the same message and including the err details, and ensure the appropriate logger is imported or obtained in this module before use.packages/effects/src/index.ts (1)
49-77: Use one source of truth for bundled effect definitions.Each effect is maintained twice here: once in the named export section and again in
allEffectDefinitions. That drift is easy to miss, and the subtle failure mode is that a new effect becomes importable but never registers viaregisterAllEffects(). Consider deriving the registration list from one shared category array or map.Also applies to: 86-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/index.ts` around lines 49 - 77, The file duplicates effect metadata by exporting each effect (e.g., blurEffectDefinition, skinSmoothEffectDefinition, brightnessEffectDefinition) and also listing them in allEffectDefinitions/registerAllEffects; consolidate to a single source of truth by creating per-category arrays or a single map of effect definitions (e.g., ARTISTIC_EFFECTS, BEAUTY_EFFECTS, COLOR_TONE_EFFECTS or EFFECT_DEFINITIONS_MAP) and export those arrays/maps, then derive allEffectDefinitions and have registerAllEffects iterate over that single collection; update any imports/usages to reference the exported arrays/map so adding a new effect only requires updating one place (the effect export file or the category array).packages/effects/src/types.ts (1)
55-72: Make face-coordinate fields fixed-size tuples.
cheekLeft,cheekRight, andjawPointshave fixed shapes, butnumber[]accepts any length. Since this is now the shared contract between the renderer and effects package, tuple types would catch malformed contexts before they reach shader code. Based on learnings, "Use type definitions from apps/web/src/types for type safety and consistency across the codebase."Suggested change
- cheekLeft?: number[]; + cheekLeft?: [number, number]; - cheekRight?: number[]; + cheekRight?: [number, number]; ... - jawPoints?: number[]; + jawPoints?: [number, number, number, number, number, number];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/types.ts` around lines 55 - 72, The effect context uses open-ended arrays for coordinate fields which allows malformed lengths; update EffectContext so cheekLeft and cheekRight are fixed 2-tuples ([number, number]) and replace jawPoints with the corresponding fixed-length tuple type used by the app (import the shared types from apps/web/src/types — e.g., Vec2 for 2D points and the JawPointsTuple or equivalent exported type) and use those types for cheekLeft, cheekRight and jawPoints in the EffectContext interface so the renderer/effects contract is type-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/package.json`:
- Around line 27-28: The package.json currently pins "@mediapipe/face_mesh" to
"0.4.1633559619"; update this dependency entry to the latest published version
"0.4.1657299874" so the project receives the fixes/improvements—open the
apps/web/package.json, locate the "@mediapipe/face_mesh" entry and replace the
version string, then run package manager install (npm/yarn/pnpm) and verify the
lockfile is updated and the app still builds/tests pass.
In `@apps/web/src/services/face-mesh/face-mesh-provider.ts`:
- Around line 23-27: The locateFile callback for the FaceMesh constructor
currently uses an unversioned CDN URL which can cause runtime version drift;
update the locateFile return value in the FaceMesh instantiation to include the
exact package version (e.g., change
"https://cdn.jsdelivr.net/npm/@mediapipe/face_mesh/${file}" to
"https://cdn.jsdelivr.net/npm/@mediapipe/face_mesh@0.4.1633559619/${file}" or
source the version from config/env) so runtime assets match the installed
dependency, and replace the use of console.warn in this module with the
project’s standard error handling/logging mechanism (do not use console.warn;
use the configured logger or throw/handle the error in the FaceMesh
initialization flow).
In `@packages/effects/src/categories.ts`:
- Around line 1-6: Replace the TypeScript enum EffectCategory with a const
object + union type: define an exported const (e.g., EFFECT_CATEGORY = {
COLOR_TONE: "color-tone", ARTISTIC: "artistic", BEAUTY: "beauty" } as const) and
export a type alias export type EffectCategory = typeof EFFECT_CATEGORY[keyof
typeof EFFECT_CATEGORY]; then update any usage that relied on the enum members
to use the const keys/values and ensure the existing Record<EffectCategory,
EffectCategoryMeta> works (it will now use the union string type); update
imports/refs to use the new EffectCategory type or the const values where
appropriate.
In `@packages/effects/src/effects/blur/index.ts`:
- Around line 27-33: Extract a small helper (e.g., parseIntensity) to centralize
parsing/validation of effectParams.intensity and use it in both shader pass
uniforms; implement it to coerce the value to Number via
Number.parseFloat(String(effectParams.intensity)), then check
Number.isFinite(value) and if not finite default to 0, finally clamp the result
to the range [0, 100] before returning; replace the inline parsing in the
uniforms functions (where intensity is computed and used to produce u_sigma)
with calls to this helper to avoid duplication and prevent NaN from propagating
to u_sigma.
In `@packages/effects/src/effects/color-balance/index.ts`:
- Around line 12-20: Update the color-balance effect parameter keys to use full
names: replace shadowsR/shadowsG/shadowsB with
shadowsRed/shadowsGreen/shadowsBlue; replace midsR/midsG/midsB with
midtonesRed/midtonesGreen/midtonesBlue; replace
highlightsR/highlightsG/highlightsB with
highlightsRed/highlightsGreen/highlightsBlue; then update the renderer uniforms
mapping for the ColorBalance effect (the uniforms mapping object that reads
these parameter keys) to reference the new keys, and update cinematic-presets.ts
preset objects to use the new keys (or add a small compatibility mapping when
loading presets so legacy keys are converted to the new keys to avoid breaking
existing saved presets/projects).
In `@packages/effects/src/effects/neon-glow/index.ts`:
- Around line 23-31: The neon-glow uniforms currently cast values unsafely,
which can produce NaN or invalid colors in shaders; update the uniforms factory
for both passes to parse numbers via Number.parseFloat with sensible fallback
defaults (e.g., for threshold and intensity) and validate the color string
before calling hexToRgb (fall back to a default hex like "#ffffff" if invalid).
Specifically modify the uniforms functions that read effectParams to: parse
effectParams.threshold and effectParams.intensity with Number.parseFloat and use
a fallback when parse returns NaN, and check/normalize effectParams.color
(validate hex format) before passing to hexToRgb so u_glowColor always receives
a valid RGB value. Ensure you update references to u_threshold, u_intensity and
u_glowColor in the two uniforms blocks shown.
In `@packages/effects/src/effects/sketch/index.ts`:
- Around line 21-29: The uniforms functions for the sketch effect currently use
TypeScript assertions (effectParams.lineWidth as number and
effectParams.threshold as number) which don't validate runtime types; update the
uniforms callbacks (the functions that return u_lineWidth and u_threshold) to
parse and validate values at runtime by using Number.parseFloat on
effectParams.lineWidth and effectParams.threshold with a numeric fallback (e.g.,
defaultLineWidth/defaultThreshold) and then apply bounds via explicit if/else
conditional checks (not Math.min/Math.max) to clamp into the allowed range
before returning u_lineWidth and u_threshold to the shaders.
In `@packages/effects/src/effects/slim-face/index.ts`:
- Around line 19-22: The uniforms generator in slim-face (uniforms function
using effectParams and context) must treat missing jaw landmarks as a no-op:
when context?.jawPoints is absent or empty, set u_intensity to 0 (instead of
(effectParams.intensity as number) / 100) so no pinch occurs; you may still
supply a fallback u_jawCenter but ensure intensity is zero until real jawPoints
exist. Update the uniforms return logic for u_intensity and guard any use of
context.jawPoints in the slim-face effect to rely on that zeroed intensity.
In `@packages/effects/src/effects/teeth-whiten/index.ts`:
- Around line 19-21: The uniforms function for the teeth-whiten effect currently
only accepts effectParams and returns u_intensity; change the uniforms signature
(the uniforms function in packages/effects/src/effects/teeth-whiten/index.ts) to
accept ( { effectParams, context } ) like skin-smooth and blush, read face
context (e.g., context.faceMask or context.faceRegions) and expose a
mask-related uniform (e.g., u_faceMaskEnabled boolean and/or u_faceMask texture)
alongside u_intensity; then update the fragment shader used by the teeth-whiten
effect to take the new mask uniform(s) and gate the desaturation/brightening
logic so whitening is applied only where the face/teeth mask indicates (use
u_faceMask or u_faceMaskEnabled to mix original and whitened color).
In `@packages/effects/src/utils/color.ts`:
- Around line 2-8: The hexToRgb function uses an abbreviated variable and
substring(); update it to use a descriptive variable name (hexString) and
String.slice() calls, and change the function signature return type to a fixed
tuple [number, number, number] for better type safety; specifically, in hexToRgb
replace h with hexString = hex.replace("#", ""), use hexString.slice(0,2),
slice(2,4), slice(4,6) when parsing with Number.parseInt(..., 16) and return a
typed tuple [r, g, b] where each channel is divided by 255.
In `@packages/effects/tests/artistic-effects.test.ts`:
- Around line 141-145: The test name states "radius max is capped at 5 for
performance" but the assertion checks <= 10; update the assertion to enforce the
contract by changing the expectation for
oilPaintEffectDefinition.params.find(...) (radiusParam) so that radiusParam.max
is asserted to be <= 5 (or alternatively rename the test to match the existing
limit if the intended cap is actually 10); locate the radiusParam reference and
adjust the expect(...) matcher accordingly to toBeLessThanOrEqual(5) to match
the test title.
---
Nitpick comments:
In `@apps/web/src/services/face-mesh/face-mesh-provider.ts`:
- Around line 42-43: Replace the console.warn call in the catch block that logs
"[face-mesh] Failed to load MediaPipe:" and the err variable with the
application's logger so the message flows through the shared logging pipeline;
locate the catch block where err is logged and swap console.warn for the
module's logger method (e.g., logger.warn or logger.error) while preserving the
same message and including the err details, and ensure the appropriate logger is
imported or obtained in this module before use.
In `@apps/web/src/services/renderer/nodes/visual-node.ts`:
- Around line 132-140: Replace the abbreviated callback parameter names in the
beauty-effect detection logic: in the enabledEffects.some callback change the
short names `e` and `def` to descriptive names like `effect` and
`effectDefinition` (or `effectDef`) and update the getEffect call accordingly
(getEffect({ effectType: effect.type })); keep the rest of the logic intact that
checks EffectCategory.BEAUTY, and ensure variables around this block
(hasBeautyEffect, faceContext, detectFace, elementCanvas) are referenced
consistently.
In `@packages/effects/package.json`:
- Around line 15-16: Replace the floating "latest" version for the `@types/bun`
devDependency with a pinned or caret-ranged version to ensure reproducible
builds; edit the "devDependencies" entry for "@types/bun" in package.json (the
dependency key "@types/bun") and set it to a specific semver like "0.4.0" or a
caret range like "^0.4.0" instead of "latest", then run your lockfile update
(npm/yarn/pnpm install) to persist the fixed version.
In `@packages/effects/src/effects/blur/blur.test.ts`:
- Around line 59-82: Add a test in blur.test.ts that calls
blurEffectDefinition.renderer.passes[0].uniforms with malformed intensity inputs
(e.g., intensity: "abc" and intensity: undefined) and assert that the returned
uniforms.u_sigma is a finite number (not NaN) and falls back to the minimum
value (e.g., 0.001) so invalid inputs cannot produce NaN; reuse the existing
pass1.uniforms invocation pattern and assertions similar to the "sigma is never
zero" test to lock in this behavior.
In `@packages/effects/src/effects/temperature/index.ts`:
- Around line 26-28: The division by the magic number 400 in the uniforms
function should be replaced with a named constant for clarity; introduce a
constant (e.g., TEMPERATURE_NORMALIZATION = 400) near the top of the module or
file and use it in uniforms: ({ effectParams }) => ({ u_temperature:
(effectParams.warmth as number) / TEMPERATURE_NORMALIZATION }), updating any
import/exports if needed and keeping the symbol names uniforms, effectParams,
and u_temperature unchanged.
In `@packages/effects/src/effects/vignette/index.ts`:
- Line 22: The literal 0.707 in the u_radius assignment is a magic number;
update the expression that sets u_radius (currently using (effectParams.radius
as number) / 100 * 0.707) to use the standard constant Math.SQRT1_2 instead of
0.707 so the intent is explicit and less error-prone.
In `@packages/effects/src/index.ts`:
- Around line 49-77: The file duplicates effect metadata by exporting each
effect (e.g., blurEffectDefinition, skinSmoothEffectDefinition,
brightnessEffectDefinition) and also listing them in
allEffectDefinitions/registerAllEffects; consolidate to a single source of truth
by creating per-category arrays or a single map of effect definitions (e.g.,
ARTISTIC_EFFECTS, BEAUTY_EFFECTS, COLOR_TONE_EFFECTS or EFFECT_DEFINITIONS_MAP)
and export those arrays/maps, then derive allEffectDefinitions and have
registerAllEffects iterate over that single collection; update any
imports/usages to reference the exported arrays/map so adding a new effect only
requires updating one place (the effect export file or the category array).
In `@packages/effects/src/presets/index.ts`:
- Around line 27-30: Add an explicit return type to the exported function
getAllBundledPresets so callers see the API shape and get compile-time safety;
change its signature to declare the actual array element type (matching the type
of allPresets) — e.g., export function getAllBundledPresets(): PresetType[] —
and ensure PresetType (or the correct existing type) is imported/referenced so
the function returns [...allPresets] with a matching annotated return type.
In `@packages/effects/src/presets/registry.ts`:
- Around line 23-25: In getPresetsByCategory, rename the filter callback
parameter from the single-letter p to a descriptive name (e.g., preset) for
readability and guideline consistency; update the lambda in
getAllPresets().filter((p) => p.category === category) to use
getAllPresets().filter((preset) => preset.category === category) so the callback
and any inline references use the full name.
In `@packages/effects/src/presets/types.ts`:
- Around line 2-17: Replace the exported interfaces with exported type aliases
to follow repo convention: change EffectPresetEntry and EffectPreset from
"export interface" to "export type" keeping the same property shapes and JSDoc
comments (retain effectType, params: Record<string, number | string | boolean>,
and for EffectPreset keep type, name, category: PresetCategory, description,
effects: EffectPresetEntry[]); leave PresetCategory as-is (it's already an
exported type). Ensure the exported symbol names (EffectPresetEntry,
PresetCategory, EffectPreset) remain unchanged so other modules keep importing
them.
In `@packages/effects/src/types.ts`:
- Around line 55-72: The effect context uses open-ended arrays for coordinate
fields which allows malformed lengths; update EffectContext so cheekLeft and
cheekRight are fixed 2-tuples ([number, number]) and replace jawPoints with the
corresponding fixed-length tuple type used by the app (import the shared types
from apps/web/src/types — e.g., Vec2 for 2D points and the JawPointsTuple or
equivalent exported type) and use those types for cheekLeft, cheekRight and
jawPoints in the EffectContext interface so the renderer/effects contract is
type-safe.
In `@packages/effects/tests/beauty-effects.test.ts`:
- Around line 21-40: The helper function validateBeautyEffect uses abbreviated
identifiers (def, p, pass) which reduces readability; rename them to full,
descriptive names across the function—e.g., rename parameter def to
effectDefinition, loop variable p to param or parameterDefinition, pass to
rendererPass, and defaults to defaultValues—update all usages including
effectDefinition.params, effectDefinition.renderer.passes, and the uniforms
calls (pass.uniforms -> rendererPass.uniforms) and similarly in the face-context
test; apply the same renames in the other shared validation helper occurrences
mentioned so identifiers are consistent and self-descriptive.
- Around line 27-33: The test currently asserts uniforms returns an "object"
using typeof which allows null; change the assertion in the test `${type}:
uniforms work without context (graceful fallback)` (and the similar block
covering def.renderer.passes) to explicitly reject null and ensure a plain
object, e.g. assert that result !== null && typeof result === "object" or use
expect(result).not.toBeNull() followed by expect(typeof result).toBe("object");
locate the loop that calls pass.uniforms({ effectParams: defaults,
...defaultArgs }) and update its assertions accordingly.
In `@packages/effects/tests/registry.test.ts`:
- Around line 45-49: Rename the abbreviated variable def to the full name
definition in the tests so they follow the coding guideline (e.g., in the test
that calls createStubDefinition("blur") and then registerEffect({ definition:
def }) change both the variable and its use to definition), and apply the same
rename for all occurrences in this file (the tests referencing
createStubDefinition, registerEffect, and hasEffect across the noted blocks) so
signatures and assertions use definition instead of def.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b879552f-1afa-45bf-b127-fdf35bbfc909
⛔ Files ignored due to path filters (32)
bun.lockis excluded by!**/*.lockpackages/effects/src/effects/blur/blur.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/blush/blush.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/brightness/brightness.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/chromatic-aberration/chromatic-aberration.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/color-balance/color-balance.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/contrast/contrast.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/exposure/exposure.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/eye-enhance/eye-enhance.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/face-brighten/face-brighten.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/film-grain/film-grain.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/gamma/gamma.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/glitch/glitch-pass1.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/glitch/glitch-pass2.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/hue-shift/hue-shift.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/neon-glow/neon-glow-pass1.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/neon-glow/neon-glow-pass2.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/oil-paint/oil-paint-pass1.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/oil-paint/oil-paint-pass2.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/pixelate/pixelate.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/saturation/saturation.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/sharpen/sharpen.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/sketch/sketch-pass1.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/sketch/sketch-pass2.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/skin-smooth/skin-smooth-pass1.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/skin-smooth/skin-smooth-pass2.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/slim-face/slim-face.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/teeth-whiten/teeth-whiten.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/temperature/temperature.frag.glslis excluded by!**/*.glslpackages/effects/src/effects/vignette/vignette.frag.glslis excluded by!**/*.glslpackages/effects/src/shaders/common.glslis excluded by!**/*.glslpackages/effects/src/shaders/effect.vert.glslis excluded by!**/*.glsl
📒 Files selected for processing (65)
apps/web/package.jsonapps/web/src/lib/effects/definitions/blur.tsapps/web/src/lib/effects/definitions/index.tsapps/web/src/lib/effects/index.tsapps/web/src/lib/effects/registry.tsapps/web/src/services/face-mesh/face-mesh-provider.tsapps/web/src/services/face-mesh/index.tsapps/web/src/services/renderer/nodes/effect-layer-node.tsapps/web/src/services/renderer/nodes/image-node.tsapps/web/src/services/renderer/nodes/sticker-node.tsapps/web/src/services/renderer/nodes/video-node.tsapps/web/src/services/renderer/nodes/visual-node.tsapps/web/src/services/renderer/webgl-utils.tsapps/web/src/types/effects.tsapps/web/tsconfig.tsbuildinfopackages/effects/.gitignorepackages/effects/package.jsonpackages/effects/src/categories.tspackages/effects/src/effects/blur/blur.test.tspackages/effects/src/effects/blur/index.tspackages/effects/src/effects/blush/index.tspackages/effects/src/effects/brightness/index.tspackages/effects/src/effects/chromatic-aberration/chromatic-aberration.test.tspackages/effects/src/effects/chromatic-aberration/index.tspackages/effects/src/effects/color-balance/index.tspackages/effects/src/effects/contrast/index.tspackages/effects/src/effects/exposure/index.tspackages/effects/src/effects/eye-enhance/index.tspackages/effects/src/effects/face-brighten/index.tspackages/effects/src/effects/film-grain/index.tspackages/effects/src/effects/gamma/index.tspackages/effects/src/effects/glitch/glitch.test.tspackages/effects/src/effects/glitch/index.tspackages/effects/src/effects/hue-shift/index.tspackages/effects/src/effects/neon-glow/index.tspackages/effects/src/effects/oil-paint/index.tspackages/effects/src/effects/pixelate/index.tspackages/effects/src/effects/saturation/index.tspackages/effects/src/effects/sharpen/index.tspackages/effects/src/effects/sharpen/sharpen.test.tspackages/effects/src/effects/sketch/index.tspackages/effects/src/effects/skin-smooth/index.tspackages/effects/src/effects/slim-face/index.tspackages/effects/src/effects/teeth-whiten/index.tspackages/effects/src/effects/temperature/index.tspackages/effects/src/effects/vignette/index.tspackages/effects/src/effects/vignette/vignette.test.tspackages/effects/src/glsl.d.tspackages/effects/src/index.tspackages/effects/src/presets/beauty-presets.tspackages/effects/src/presets/cinematic-presets.tspackages/effects/src/presets/index.tspackages/effects/src/presets/instagram-filters.tspackages/effects/src/presets/registry.tspackages/effects/src/presets/types.tspackages/effects/src/registry.tspackages/effects/src/types.tspackages/effects/src/utils/color.tspackages/effects/tests/artistic-effects.test.tspackages/effects/tests/beauty-effects.test.tspackages/effects/tests/color-tone-effects.test.tspackages/effects/tests/presets.test.tspackages/effects/tests/registry.test.tspackages/effects/tsconfig.jsonpackages/effects/tsup.config.ts
💤 Files with no reviewable changes (1)
- apps/web/src/lib/effects/definitions/blur.ts
Comment on lines
+19
to
+21
| uniforms: ({ effectParams }) => ({ | ||
| u_intensity: (effectParams.intensity as number) / 200, | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether teeth-whiten shader/effect pipeline supports face-aware masking uniforms.
fd 'teeth-whiten.frag.glsl' packages/effects/src --exec sed -n '1,240p' {}
rg -nP --type=ts 'teeth-whiten|faceDetected|faceLandmark|u_faceMaskEnabled|context' packages/effects/src apps/web/src/services/renderer/nodes -C 3Repository: OpenCut-app/OpenCut
Length of output: 21562
Teeth-whiten effect is not face-aware; the shader applies whitening globally.
The effect should consume face context when available, but the uniforms function at lines 19–21 only accepts effectParams. Additionally, the shader itself only supports u_intensity and has no masking logic, so integrating face detection requires both:
- Accept
contextin the uniforms function (as shown in skin-smooth and blush) - Add mask-aware logic to the shader fragment code
Shader limitations
The fragment shader currently applies desaturation and brightening uniformly across all pixels. To be teeth-aware, the shader would need to receive and apply a face mask uniform (e.g., u_faceMaskEnabled or u_faceMask) to gate the effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/effects/src/effects/teeth-whiten/index.ts` around lines 19 - 21, The
uniforms function for the teeth-whiten effect currently only accepts
effectParams and returns u_intensity; change the uniforms signature (the
uniforms function in packages/effects/src/effects/teeth-whiten/index.ts) to
accept ( { effectParams, context } ) like skin-smooth and blush, read face
context (e.g., context.faceMask or context.faceRegions) and expose a
mask-related uniform (e.g., u_faceMaskEnabled boolean and/or u_faceMask texture)
alongside u_intensity; then update the fragment shader used by the teeth-whiten
effect to take the new mask uniform(s) and gate the desaturation/brightening
logic so whitening is applied only where the face/teeth mask indicates (use
u_faceMask or u_faceMaskEnabled to mix original and whitened color).
…en, and vignette effects - Implement tests for chromatic aberration effect definition, verifying type, name, category, parameters, and renderer behavior. - Add tests for glitch effect definition, covering type, name, parameters, and rendering logic. - Create tests for sharpen effect definition, ensuring correct parameter defaults and rendering. - Introduce tests for vignette effect definition, validating parameters and rendering behavior. These tests enhance coverage for the new effects introduced in the @opencut/effects package.
…into feat/effects-package # Conflicts: # apps/web/tsconfig.tsbuildinfo
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/web/src/services/face-mesh/face-mesh-provider.ts (1)
68-72: Use descriptive variable names instead of abbreviations.The variable names
lc,rc,jawL,jawRshould be expanded for readability.✏️ Suggested refactor
- const lc = landmarks[LANDMARK_INDICES.leftCheek]; - const rc = landmarks[LANDMARK_INDICES.rightCheek]; - const jaw = landmarks[LANDMARK_INDICES.jawBottom]; - const jawL = landmarks[LANDMARK_INDICES.jawLeft]; - const jawR = landmarks[LANDMARK_INDICES.jawRight]; + const leftCheek = landmarks[LANDMARK_INDICES.leftCheek]; + const rightCheek = landmarks[LANDMARK_INDICES.rightCheek]; + const jawBottom = landmarks[LANDMARK_INDICES.jawBottom]; + const jawLeft = landmarks[LANDMARK_INDICES.jawLeft]; + const jawRight = landmarks[LANDMARK_INDICES.jawRight]; // Estimate cheek radius from face width - const faceWidth = Math.abs(rc.x - lc.x); + const faceWidth = Math.abs(rightCheek.x - leftCheek.x); const cheekRadius = faceWidth * 0.15; return { faceDetected: true, - cheekLeft: [lc.x, lc.y], - cheekRight: [rc.x, rc.y], + cheekLeft: [leftCheek.x, leftCheek.y], + cheekRight: [rightCheek.x, rightCheek.y], cheekRadius, - jawPoints: [jaw.x, jaw.y, jawL.x, jawL.y, jawR.x, jawR.y], + jawPoints: [jawBottom.x, jawBottom.y, jawLeft.x, jawLeft.y, jawRight.x, jawRight.y], };As per coding guidelines: "Never abbreviate variable and parameter names."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/face-mesh/face-mesh-provider.ts` around lines 68 - 72, Replace the abbreviated landmark variables (lc, rc, jawL, jawR) with descriptive names to match coding guidelines: e.g., leftCheek, rightCheek, jawBottom (already named jaw), jawLeft, jawRight; update the assignments that use landmarks[LANDMARK_INDICES.*] and rename all subsequent usages in the same module (functions/methods referencing these symbols in face-mesh-provider.ts) so references remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/services/face-mesh/face-mesh-provider.ts`:
- Around line 44-46: Remove the console.warn call in the catch block (the catch
(err) in face-mesh-provider.ts) and either call the project logging utility
(e.g., processLogger.warn or logger.warn) with the error or omit logging
entirely while keeping the existing return null; update the code path that
currently does console.warn("[face-mesh] Failed to load MediaPipe:", err) to use
the standard logger method or no console call.
- Around line 36-41: The current fm.onResults handler only resolves
pendingResolve and never handles errors or timeouts, so update the
pendingDetection creation and fm.send usage to add a rejection path and timeout:
when creating pendingDetection, set up a timeout (e.g., 5s) that calls
pendingReject(new Error("Face detection timeout")) and ensure both
pendingResolve and pendingReject clear that timeout before resolving/rejecting;
wrap the fm.send(...) call in try-catch and call pendingReject(err) on sync
errors; ensure fm.onResults uses pendingResolve to clear timeout and resolve and
that pendingReject is assigned so it can be invoked from the timeout or catch
path (references: pendingDetection, pendingResolve, pendingReject, fm.send,
fm.onResults).
- Line 112: The call fm.send({ image: source as HTMLCanvasElement }) uses an
unsafe cast; update detectFace (and its caller) so source is typed as
HTMLImageElement | HTMLCanvasElement | HTMLVideoElement, remove the forced cast,
and add a runtime type guard before calling fm.send to ensure source is one of
those allowed types; if you may receive an OffscreenCanvas (from
createOffscreenCanvas()), convert it to a real HTMLCanvasElement (e.g., by
creating an HTMLCanvasElement, drawing the OffscreenCanvas/ImageBitmap into it)
and pass that converted canvas to fm.send to avoid runtime errors.
---
Nitpick comments:
In `@apps/web/src/services/face-mesh/face-mesh-provider.ts`:
- Around line 68-72: Replace the abbreviated landmark variables (lc, rc, jawL,
jawR) with descriptive names to match coding guidelines: e.g., leftCheek,
rightCheek, jawBottom (already named jaw), jawLeft, jawRight; update the
assignments that use landmarks[LANDMARK_INDICES.*] and rename all subsequent
usages in the same module (functions/methods referencing these symbols in
face-mesh-provider.ts) so references remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f1e4562-f895-4acf-ba5c-bd77a65af0a6
📒 Files selected for processing (10)
apps/web/package.jsonapps/web/src/services/face-mesh/face-mesh-provider.tsapps/web/tsconfig.tsbuildinfopackages/effects/src/categories.tspackages/effects/src/effects/blur/index.tspackages/effects/src/effects/color-balance/index.tspackages/effects/src/effects/slim-face/index.tspackages/effects/src/presets/cinematic-presets.tspackages/effects/src/utils/color.tspackages/effects/tests/artistic-effects.test.ts
✅ Files skipped from review due to trivial changes (2)
- apps/web/package.json
- packages/effects/src/presets/cinematic-presets.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/effects/src/effects/color-balance/index.ts
- packages/effects/src/categories.ts
- packages/effects/src/effects/blur/index.ts
- packages/effects/tests/artistic-effects.test.ts
- packages/effects/src/effects/slim-face/index.ts
chore: remove logger Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into feat/effects-package
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/effects/src/effects/neon-glow/index.ts (1)
31-33:⚠️ Potential issue | 🟠 MajorValidate
colorbeforehexToRgbto avoidNaNshader uniforms.
getStringParamguarantees a string, not a valid hex color. If malformed input reacheshexToRgb,u_glowColorcan become[NaN, NaN, NaN].🔧 Proposed fix
import pass1Shader from "./neon-glow-pass1.frag.glsl"; import pass2Shader from "./neon-glow-pass2.frag.glsl"; +const HEX_COLOR_PATTERN = /^#[0-9a-fA-F]{6}$/; + /** Neon glow — 2-pass: extract bright areas, blur + tint */ export const neonGlowEffectDefinition: EffectDefinition = { @@ { fragmentShader: pass2Shader, - uniforms: ({ effectParams }) => ({ - u_intensity: getNumberParam(effectParams, "intensity") / 50, - u_glowColor: hexToRgb(getStringParam(effectParams, "color", "#00ff88")), - }), + uniforms: ({ effectParams }) => { + const glowColorHex = getStringParam(effectParams, "color", "#00ff88"); + const safeGlowColorHex = HEX_COLOR_PATTERN.test(glowColorHex) + ? glowColorHex + : "#00ff88"; + + return { + u_intensity: getNumberParam(effectParams, "intensity") / 50, + u_glowColor: hexToRgb(safeGlowColorHex), + }; + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/effects/src/effects/neon-glow/index.ts` around lines 31 - 33, The color string from getStringParam may be malformed before passing to hexToRgb causing u_glowColor to become [NaN,NaN,NaN]; fix by reading the color into a local variable (e.g., const color = getStringParam(effectParams, "color", "#00ff88")), validate it with a simple hex regex or normalization routine, and only call hexToRgb(color) when valid—otherwise fall back to a safe default like "#00ff88" (or precomputed rgb) so u_glowColor is never set to NaNs; update the code around getStringParam/hexToRgb and u_glowColor to use this validated value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/effects/src/effects/neon-glow/index.ts`:
- Around line 31-33: The color string from getStringParam may be malformed
before passing to hexToRgb causing u_glowColor to become [NaN,NaN,NaN]; fix by
reading the color into a local variable (e.g., const color =
getStringParam(effectParams, "color", "#00ff88")), validate it with a simple hex
regex or normalization routine, and only call hexToRgb(color) when
valid—otherwise fall back to a safe default like "#00ff88" (or precomputed rgb)
so u_glowColor is never set to NaNs; update the code around
getStringParam/hexToRgb and u_glowColor to use this validated value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e33efb8-a875-4d3d-8901-c8ce4c2d3b29
📒 Files selected for processing (26)
apps/web/src/services/face-mesh/face-mesh-provider.tsapps/web/tsconfig.tsbuildinfopackages/effects/src/effects/blush/index.tspackages/effects/src/effects/brightness/index.tspackages/effects/src/effects/chromatic-aberration/index.tspackages/effects/src/effects/color-balance/index.tspackages/effects/src/effects/contrast/index.tspackages/effects/src/effects/exposure/index.tspackages/effects/src/effects/eye-enhance/index.tspackages/effects/src/effects/face-brighten/index.tspackages/effects/src/effects/film-grain/index.tspackages/effects/src/effects/gamma/index.tspackages/effects/src/effects/glitch/index.tspackages/effects/src/effects/hue-shift/index.tspackages/effects/src/effects/neon-glow/index.tspackages/effects/src/effects/oil-paint/index.tspackages/effects/src/effects/pixelate/index.tspackages/effects/src/effects/saturation/index.tspackages/effects/src/effects/sharpen/index.tspackages/effects/src/effects/sketch/index.tspackages/effects/src/effects/skin-smooth/index.tspackages/effects/src/effects/slim-face/index.tspackages/effects/src/effects/teeth-whiten/index.tspackages/effects/src/effects/temperature/index.tspackages/effects/src/effects/vignette/index.tspackages/effects/src/utils/params.ts
✅ Files skipped from review due to trivial changes (10)
- packages/effects/src/effects/brightness/index.ts
- packages/effects/src/effects/face-brighten/index.ts
- packages/effects/src/effects/pixelate/index.ts
- packages/effects/src/effects/skin-smooth/index.ts
- packages/effects/src/effects/exposure/index.ts
- packages/effects/src/effects/film-grain/index.ts
- packages/effects/src/effects/sketch/index.ts
- packages/effects/src/effects/sharpen/index.ts
- packages/effects/src/effects/teeth-whiten/index.ts
- packages/effects/src/effects/chromatic-aberration/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/effects/src/effects/saturation/index.ts
- packages/effects/src/effects/hue-shift/index.ts
- packages/effects/src/effects/temperature/index.ts
- packages/effects/src/effects/gamma/index.ts
- packages/effects/src/effects/contrast/index.ts
- packages/effects/src/effects/vignette/index.ts
- packages/effects/src/effects/blush/index.ts
- packages/effects/src/effects/oil-paint/index.ts
- packages/effects/src/effects/eye-enhance/index.ts
- apps/web/src/services/face-mesh/face-mesh-provider.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a feature:
Features for: Feature Effects #713
Pre-approval Confirmation
Summary
Introduces
@opencut/effects, a standalone monorepo package that provides a comprehensive video effects system with 24 WebGL shader effects, a preset system, and MediaPipe face detection integration for beauty effects.@opencut/webinto reusable@opencut/effectspackageMotivation
The existing effects system had only a blur effect hardcoded inside
@opencut/web. This PR:What's Included
Package:
packages/effects/Registry & Types
Effect,EffectDefinition,EffectContexttype systemregisterEffect(),getEffect(),getAllEffects()EffectCategoryenum:COLOR_TONE,ARTISTIC,BEAUTYWebGLEffectPasswith optionaltime(animated effects) andcontext(face data)Color & Tone Effects (8) — single-pass shaders
Artistic Effects (10) — single and multi-pass shaders
Beauty Effects (6) — face-aware with MediaPipe integration
Preset System (12 presets)
Each preset is a curated combination of 2-5 effects with tuned parameters.
Web App Integration (
apps/web/)@/types/effectsand@/lib/effectsimports still workvisual-node.ts: passestimefor animated effects, auto-detects beauty effects for face detectioneffect-layer-node.ts: passestimeto uniforms factoryservices/face-mesh/service: lazy-loads MediaPipe, promise-based detection (no race conditions)definitions/blur.ts,blur.frag.glsl,effect.vert.glslAdding a New Effect
packages/effects/src/effects/{name}/index.ts(definition)packages/effects/src/effects/{name}/{name}.frag.glsl(shader)packages/effects/src/index.tsTest Plan
bun testinpackages/effects/)Breaking Changes
None. All existing imports continue to work via re-export shims.
Screen.Recording.2026-03-23.at.2.27.12.pm.mov
Summary by CodeRabbit