[codex] Fix project health checks#531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces ChangesUUID Modernization & Security
i18n Locale Restructuring
useScreenRecorder Hook Optimization
Test Infrastructure & E2E Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/i18n/__tests__/tutorialHelpTranslations.test.ts (1)
37-46: 💤 Low valuenit: key order in
dialogsByLocalediverges fromSUPPORTED_LOCALES
zh-TWandja-JPare appended at the end instead of matching the canonical order inSUPPORTED_LOCALES. not a bug — thesatisfiesconstraint catches any gaps at compile time — but keeping them in sync makes future diffs easier to grok.✨ Optional ordering alignment
const dialogsByLocale = { en: enDialogs, "zh-CN": zhCNDialogs, + "zh-TW": zhTWDialogs, es: esDialogs, fr: frDialogs, tr: trDialogs, "ko-KR": koKRDialogs, - "zh-TW": zhTWDialogs, "ja-JP": jaJPDialogs, } satisfies Record<Locale, { tutorial: Record<string, unknown> }>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/__tests__/tutorialHelpTranslations.test.ts` around lines 37 - 46, The object literal dialogsByLocale is not in the same key order as the canonical SUPPORTED_LOCALES, which makes diffs harder to read; reorder the entries in dialogsByLocale to exactly match the sequence in SUPPORTED_LOCALES (use the same locale strings and order), keeping the same values (enDialogs, zhCNDialogs, zhTWDialogs, esDialogs, frDialogs, trDialogs, koKRDialogs, jaJPDialogs) so the satisfies Record<Locale, { tutorial: Record<string, unknown> }> check still passes.src/hooks/useScreenRecorder.ts (1)
411-417: 💤 Low valueLGTM — clean fix for the stale-closure + excess re-run problem.
Memoizing
safeHideCountdownOverlaywithuseCallback(…, [])is exactly right. Before this, a plainconstwas recreated every render; including it in the effect's dep array (line 461) would have re-registered theonStopRecordingFromTraylistener on every render, and omitting it would have been a stale-closure lint violation. Both hazards gone now.One nit:
window.electronAPIis accessed without optional chaining on line 413, while several other call sites in the same file usewindow.electronAPI?.xxx(e.g. lines 305, 422). Thetry/catchmakes it safe, but it's kinda inconsistent. Up to you whether you want to unify the style.🧹 Optional: align with the optional-chaining pattern used elsewhere
- await window.electronAPI.hideCountdownOverlay(runId); + await window.electronAPI?.hideCountdownOverlay(runId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useScreenRecorder.ts` around lines 411 - 417, Update safeHideCountdownOverlay to use optional chaining when calling the Electron API to match the style elsewhere: in the useCallback for safeHideCountdownOverlay, replace the direct access window.electronAPI.hideCountdownOverlay(runId) with a guarded call (e.g. window.electronAPI?.hideCountdownOverlay) or an early-return if window.electronAPI is undefined, so the call site is consistent with other uses like the ones around lines referencing window.electronAPI?.xxx; keep the existing try/catch and behaviour otherwise.src/components/video-editor/timeline/TimelineEditor.tsx (1)
833-838: Move UUID generation outside the state updater to avoid double-invocation in StrictMode
crypto.randomUUID()is called inside thesetKeyframesfunctional updater. In React 18 + StrictMode (development), React double-invokes state updaters and discards the first result — so two UUIDs get generated but only one is used. No bug in production, but cleaner to generate the ID outside the updater and capture it in the closure:✨ Suggested fix
- setKeyframes((prev) => [...prev, { id: crypto.randomUUID(), time }]); + const id = crypto.randomUUID(); + setKeyframes((prev) => [...prev, { id, time }]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 833 - 838, The addKeyframe callback currently calls crypto.randomUUID() inside the setKeyframes functional updater which can produce duplicate/extra UUIDs in React 18 StrictMode; fix by generating the id before calling setKeyframes (e.g., const id = crypto.randomUUID()) and then use that id inside the updater when appending the new keyframe, keeping the existing time calculation and duplicate-time guard; refer to addKeyframe, setKeyframes, keyframes and crypto.randomUUID when making the change.tests/e2e/gif-export.spec.ts (1)
97-104: ⚡ Quick winDon't swallow non-teardown failures here.
This catch eats real failures from
setCurrentVideoPath()/switchToEditor()too, so a broken IPC path turns into a generic 15s window timeout and the root cause disappears. I'd only ignore the expected context-loss case and rethrow the rest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/gif-export.spec.ts` around lines 97 - 104, The try/catch around hudWindow.evaluate is swallowing all failures from window.electronAPI.setCurrentVideoPath() and switchToEditor(); update the catch to only ignore the expected “page/context torn down” errors and rethrow any other errors. Concretely, inside the catch for the hudWindow.evaluate call inspect the caught error (from the evaluate call) and if its message indicates a destroyed execution context or closed target (e.g. contains substrings like "Execution context was destroyed", "Target closed", or "page is closed") then swallow it, otherwise rethrow the error so real IPC/logic failures from setCurrentVideoPath or switchToEditor surface in the test failure..github/workflows/ci.yml (1)
43-60: ⚡ Quick winWe're still leaving two health checks manual.
The PR validation list includes
npm audit --omit=devandgit diff --check, but this workflow still won't enforce either. If the point here is "project health checks", I'd wire those in too so green CI actually means the full checklist passed.possible follow-up
test: name: Test runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: node-version: 22 cache: npm - run: npm ci + - run: npm audit --omit=dev + - run: git diff --check - run: npm run i18n:check - run: npm run test - run: npm run test:browser:install - run: npm run test:browser🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 43 - 60, The workflow is missing enforcement of two project health checks: run the commands npm audit --omit=dev and git diff --check as CI steps so failures block merges; add these steps (using the literal commands npm audit --omit=dev and git diff --check) after the existing npm ci step in both the primary job's steps (the earlier npm ci run) and the e2e job (after the e2e job's npm ci) so the workflow will fail when those checks fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vitest.config.ts`:
- Line 9: The test.exclude assignment overrides Vitest's built-in excludes (like
/node_modules/ and /.git/), so keep those defaults and add your browser-only
pattern instead: use configDefaults.exclude and merge or append the glob
"src/**/*.browser.test.{ts,tsx}" rather than replacing test.exclude in the
vitest config (refer to test.exclude, configDefaults.exclude and the browser
glob to locate where to change).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 43-60: The workflow is missing enforcement of two project health
checks: run the commands npm audit --omit=dev and git diff --check as CI steps
so failures block merges; add these steps (using the literal commands npm audit
--omit=dev and git diff --check) after the existing npm ci step in both the
primary job's steps (the earlier npm ci run) and the e2e job (after the e2e
job's npm ci) so the workflow will fail when those checks fail.
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 833-838: The addKeyframe callback currently calls
crypto.randomUUID() inside the setKeyframes functional updater which can produce
duplicate/extra UUIDs in React 18 StrictMode; fix by generating the id before
calling setKeyframes (e.g., const id = crypto.randomUUID()) and then use that id
inside the updater when appending the new keyframe, keeping the existing time
calculation and duplicate-time guard; refer to addKeyframe, setKeyframes,
keyframes and crypto.randomUUID when making the change.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 411-417: Update safeHideCountdownOverlay to use optional chaining
when calling the Electron API to match the style elsewhere: in the useCallback
for safeHideCountdownOverlay, replace the direct access
window.electronAPI.hideCountdownOverlay(runId) with a guarded call (e.g.
window.electronAPI?.hideCountdownOverlay) or an early-return if
window.electronAPI is undefined, so the call site is consistent with other uses
like the ones around lines referencing window.electronAPI?.xxx; keep the
existing try/catch and behaviour otherwise.
In `@src/i18n/__tests__/tutorialHelpTranslations.test.ts`:
- Around line 37-46: The object literal dialogsByLocale is not in the same key
order as the canonical SUPPORTED_LOCALES, which makes diffs harder to read;
reorder the entries in dialogsByLocale to exactly match the sequence in
SUPPORTED_LOCALES (use the same locale strings and order), keeping the same
values (enDialogs, zhCNDialogs, zhTWDialogs, esDialogs, frDialogs, trDialogs,
koKRDialogs, jaJPDialogs) so the satisfies Record<Locale, { tutorial:
Record<string, unknown> }> check still passes.
In `@tests/e2e/gif-export.spec.ts`:
- Around line 97-104: The try/catch around hudWindow.evaluate is swallowing all
failures from window.electronAPI.setCurrentVideoPath() and switchToEditor();
update the catch to only ignore the expected “page/context torn down” errors and
rethrow any other errors. Concretely, inside the catch for the
hudWindow.evaluate call inspect the caught error (from the evaluate call) and if
its message indicates a destroyed execution context or closed target (e.g.
contains substrings like "Execution context was destroyed", "Target closed", or
"page is closed") then swallow it, otherwise rethrow the error so real IPC/logic
failures from setCurrentVideoPath or switchToEditor surface in the test failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bb9bd07-eb2b-44a9-bb20-3ac077136f6a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/ci.ymlelectron/windows.tspackage.jsonsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/hooks/useScreenRecorder.tssrc/i18n/__tests__/tutorialHelpTranslations.test.tssrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/blurEffects.test.tstests/e2e/gif-export.spec.tsvitest.config.ts
💤 Files with no reviewable changes (5)
- electron/windows.ts
- package.json
- src/i18n/locales/zh-TW/settings.json
- src/i18n/locales/tr/settings.json
- src/i18n/locales/fr/settings.json
| globals: true, | ||
| environment: "jsdom", | ||
| include: ["src/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}"], | ||
| exclude: ["src/**/*.browser.test.{ts,tsx}"], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Vitest 4.x config: does setting test.excludeindefineConfigreplace the default exclude list, and isconfigDefaults.exclude the supported way to extend it?
💡 Result:
Yes, setting test.exclude in defineConfig replaces the default exclude list. The official Vitest documentation explicitly states: "manually setting exclude in your config will replace the default value." The default is ['/node_modules/', '/.git/']. Yes, configDefaults.exclude is the supported way to extend it. The docs provide this exact example: import { configDefaults, defineConfig } from 'vitest/config' export default defineConfig({ test: { exclude: [ ...configDefaults.exclude, 'packages/template/*', './temp/', ], }, }) This behavior applies to Vitest 4.x, as confirmed by the current official documentation at vitest.dev/config/exclude (published 2026-03-04) and v4.vitest.dev/config/exclude.
Citations:
- 1: https://v4.vitest.dev/config/exclude
- 2: https://vitest.dev/config/exclude
- 3: https://vitest.dev/config/
Preserve Vitest's default excludes here.
Setting test.exclude manually replaces Vitest's default exclude list (which includes /node_modules/ and /.git/), so this drops the built-in patterns and can make the test runner pick up folders it shouldn't. lowkey risky for CI noise/perf. Fold your browser glob into configDefaults.exclude instead per the official docs.
nit: cleaner fix
-import { defineConfig } from "vitest/config";
+import { configDefaults, defineConfig } from "vitest/config";
export default defineConfig({
test: {
globals: true,
environment: "jsdom",
include: ["src/**/*.{test,spec}.{js,mjs,cjs,ts,mts,cts,jsx,tsx}"],
- exclude: ["src/**/*.browser.test.{ts,tsx}"],
+ exclude: [...configDefaults.exclude, "src/**/*.browser.test.{ts,tsx}"],
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vitest.config.ts` at line 9, The test.exclude assignment overrides Vitest's
built-in excludes (like /node_modules/ and /.git/), so keep those defaults and
add your browser-only pattern instead: use configDefaults.exclude and merge or
append the glob "src/**/*.browser.test.{ts,tsx}" rather than replacing
test.exclude in the vitest config (refer to test.exclude, configDefaults.exclude
and the browser glob to locate where to change).
1 participant
Summary
uuiddependency, re-enable Electron web security for the editor window, and stabilize Electron e2e teardownValidation
npm run lintnpx tsc --noEmitnpm run testnpm run test:browsernpm run i18n:checknpm audit --omit=devnpm run build-vitenpm run test:e2egit diff --checkSummary by CodeRabbit
Tests
Internationalization
Bug Fixes
Chores