Codex/storage optimization - #789
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds configurable workspace storage with migration, usage reporting, and temporary-file cleanup. It wires these operations through Electron IPC and the launch UI. Windows capture validates temporary storage and reports detailed failures. Windows packaging adds portable artifacts. Video editing preserves source-to-timeline mappings during clip edits. ChangesWorkspace storage and Windows distribution
Source-aware video timeline mapping
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant User
participant MorePopover
participant electronAPI
participant projectIPC as project IPC handlers
participant storageSettings
User->>MorePopover: Select workspace action
MorePopover->>electronAPI: chooseWorkspaceDirectory()
electronAPI->>projectIPC: invoke choose-workspace-directory
projectIPC->>storageSettings: migrateStorageData() and persistWorkspaceRoot()
storageSettings-->>projectIPC: migration result and workspace layout
projectIPC-->>electronAPI: action result and restart status
electronAPI-->>MorePopover: update workspace state
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (8)
.github/workflows/release.yml (1)
452-452: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the portable release artifact.
RELEASING.mddoes not mentionRecordly-windows-portable-x64.exe. Add it to the release output documentation and state thatRecordly-windows-setup-x64.exeremains the WinGet installer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release.yml at line 452, Update the release output documentation in RELEASING.md to list Recordly-windows-portable-x64.exe and explicitly identify Recordly-windows-setup-x64.exe as the WinGet installer.electron/storageSettings.ts (4)
375-387: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompare sizes before you unlink the source file.
The guard at line 381 confirms that both paths exist and are regular files. It does not confirm that the copy is complete.
unlinkis irreversible, and this loop deletes the user's only remaining copy of a recording.Add a size equality check. The cost is zero, because both
statresults are already available.🛡️ Proposed guard
const destinationStats = await fsPromises.stat(copiedFile.destination).catch(() => null); const sourceStats = await fsPromises.stat(copiedFile.source).catch(() => null); - if (!destinationStats?.isFile() || !sourceStats?.isFile()) { + if ( + !destinationStats?.isFile() || + !sourceStats?.isFile() || + destinationStats.size !== sourceStats.size + ) { continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/storageSettings.ts` around lines 375 - 387, Update the deletion loop over copiedFiles to compare destinationStats.size with sourceStats.size after confirming both are regular files, and skip unlinking when the sizes differ. Keep the existing unlink and counter updates only for matching file sizes.
125-126: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider flushing the settings file and removing the stale temporary file on failure.
The write-then-rename sequence is not durable. If the process loses power between
writeFileandrename, the filesystem can leave the temporary file empty. Ifrenamethrows, the.tmpfile stays behind forever.The failure is recoverable, because
readWorkspaceRootSynccatches the parse error and reverts to the default paths. Handle it only if you want the configured workspace to survive a crash.♻️ Proposed durability improvement
- await fsPromises.writeFile(temporaryPath, JSON.stringify(settings, null, 2), "utf-8"); - await fsPromises.rename(temporaryPath, settingsPath); + const handle = await fsPromises.open(temporaryPath, "w"); + try { + await handle.writeFile(JSON.stringify(settings, null, 2), "utf-8"); + await handle.sync(); + } finally { + await handle.close(); + } + try { + await fsPromises.rename(temporaryPath, settingsPath); + } catch (error) { + await fsPromises.rm(temporaryPath, { force: true }); + throw error; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/storageSettings.ts` around lines 125 - 126, Update the write-and-rename flow around fsPromises.writeFile and fsPromises.rename to flush the temporary settings file before renaming it, and remove temporaryPath when the operation fails. Preserve the existing recovery behavior while ensuring failed writes do not leave stale .tmp files behind.
139-159: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the recursion fan-out in
getPathSize.
getPathSizemaps every directory entry throughPromise.allat every depth. The fan-out is unbounded.getWorkspaceUsagecalls it onlayout.cache, which is the ElectronsessionDatadirectory. Chromium fills that directory with a large number of small files, so a singleget-storage-statusrequest can open a very large number of concurrentlstatoperations in the main process.Two options:
- Bound the concurrency, for example with a small worker pool over the entry list.
- Make the cache size report cheaper or lazy, so the status IPC does not walk the full Chromium cache on every call.
♻️ Sequential-per-directory variant that removes the fan-out
const entries = await fsPromises.readdir(targetPath, { withFileTypes: true }); - const sizes = await Promise.all( - entries.map((entry) => getPathSize(path.join(targetPath, entry.name))), - ); - return sizes.reduce((total, size) => total + size, 0); + let total = 0; + for (const entry of entries) { + total += await getPathSize(path.join(targetPath, entry.name)); + } + return total;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/storageSettings.ts` around lines 139 - 159, Update getPathSize to eliminate unbounded Promise.all recursion when traversing directory entries, using bounded concurrency or sequential processing per directory. Preserve existing handling for missing paths, symbolic links, files, and the summed directory size returned to getWorkspaceUsage.
245-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the mapping order dependency.
rewriteStoredPathsapplies the mappings in sequence throughreduce. The order is significant.source.projectscan be nested insidesource.recordings, as the test atelectron/storageSettings.test.tsline 95 sets up. The projects mapping must run first. If the recordings mapping ran first, a project file path would rewrite to<destination.recordings>/Projects/...instead of<destination.projects>/....The current order is correct. Add a comment so a later reordering does not break migration silently.
♻️ Proposed comment
+ // Order matters: `projects` can be nested inside `recordings`, so the more + // specific projects mapping must be applied before the recordings mapping. const rewritten = rewriteStoredPaths(project, [ { source: source.projects, destination: destination.projects }, { source: source.recordings, destination: destination.recordings }, ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/storageSettings.ts` around lines 245 - 248, Add a concise comment immediately above the mappings in the rewriteStoredPaths call explaining that order is significant: the projects mapping must precede recordings because the projects source may be nested under recordings and rewriteStoredPaths processes mappings sequentially. Preserve the existing mapping order.electron/main.ts (1)
1015-1025: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider running the temporary-file cleanup without blocking startup.
Line 1017 awaits the cleanup. The cleanup is housekeeping. Nothing later in startup depends on its result. On a machine with a large temporary directory, the scan and the per-artifact size walk delay the first window.
Detach the call and keep the error handler.
Note: the scan target itself carries a separate risk. See the comment on
cleanupRecordlyTempArtifactsinelectron/storageSettings.ts.♻️ Proposed non-blocking variant
await ensureStorageDirectories(); - try { - const cleanup = await cleanupRecordlyTempArtifacts(app.getPath("temp")); - if (cleanup.removedCount > 0) { - console.log( - `Removed ${cleanup.removedCount} stale Recordly temporary files (${cleanup.removedBytes} bytes)`, - ); - } - } catch (error) { - console.warn("Failed to clean stale Recordly temporary files:", error); - } + void cleanupRecordlyTempArtifacts(app.getPath("temp")) + .then((cleanup) => { + if (cleanup.removedCount > 0) { + console.log( + `Removed ${cleanup.removedCount} stale Recordly temporary files (${cleanup.removedBytes} bytes)`, + ); + } + }) + .catch((error) => { + console.warn("Failed to clean stale Recordly temporary files:", error); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/main.ts` around lines 1015 - 1025, Update the startup cleanup block after ensureStorageDirectories so cleanupRecordlyTempArtifacts runs without being awaited, while preserving its existing success logging and catch-based warning handling. Ensure startup proceeds immediately and cleanup errors remain handled.electron/storageSettings.test.ts (2)
92-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the conflict branch and the nested-destination guard.
The suite covers the successful migration path well, including the nested
Projectsexclusion and the path rewrite. Two user-reachable branches have no coverage:
skippedConflicts.migrateStorageDatarelies onCOPYFILE_EXCLto never overwrite. The dialog inelectron/ipc/register/project.tspromises exactly that to the user. A test should pre-create a file at the destination, then assertskippedConflictsis 1 and thatdeleteMigratedSourceFilesleaves that source file in place.- The guard at
electron/storageSettings.tslines 269 to 275. A test should assert thatmigrateStorageDatarejects whendestination.rootis insidesource.recordings.Do you want me to generate these two tests?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/storageSettings.test.ts` around lines 92 - 125, Extend the storage migration tests covering migrateStorageData and deleteMigratedSourceFiles with two cases: pre-create a destination file to verify COPYFILE_EXCL behavior reports skippedConflicts as 1 and leaves the conflicting source untouched, and use a destination whose root is nested under source.recordings to verify migrateStorageData rejects. Preserve the existing successful migration assertions.
50-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion on line 68 does not isolate
initializeWorkspaceStorage, and module state leaks across tests.
persistWorkspaceRootalready sets the module-levelactiveWorkspaceRootatelectron/storageSettings.tsline 127. Line 54 calls it. So the assertion on line 68 passes even ifinitializeWorkspaceStoragenever touched that state. The assertions on lines 66 and 67 are sound, becausepersistWorkspaceRootdoes not callsetPath.The same module state is never reset between tests.
activeWorkspaceRootandworkspaceInitializationErrorstay set for every later test in the file. No current test reads them, so the suite passes. A new test that asserts the default-workspace path would fail depending on the order.Add a reset in
afterEach. Consider also asserting the null-workspace branch, wherereadWorkspaceRootSyncfinds no settings file andinitializeWorkspaceStoragereturnsnullwithout callingsetPath.💚 Proposed isolation improvement
Reset the module state after each test. This needs a small exported test hook, or use an empty
userDataPathto driveinitializeWorkspaceStorageback to the null branch:afterEach(async () => { + // Reset module-level workspace state so tests do not depend on order. + initializeWorkspaceStorage( + { getPath: () => "", setPath: () => undefined }, + path.join(os.tmpdir(), "recordly-nonexistent-user-data"), + ); await Promise.all( temporaryRoots.splice(0).map((root) => fs.rm(root, { recursive: true, force: true })), ); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/storageSettings.test.ts` around lines 50 - 69, Isolate storage-settings tests from leaked module state and ensure the initialization test verifies initializeWorkspaceStorage itself. Add an afterEach reset for activeWorkspaceRoot and workspaceInitializationError via a small test hook, or otherwise drive the empty-settings null branch; update the test to assert the null workspace result and that setPath is not called when no workspace is configured.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron-builder.json5`:
- Around line 106-118: Update the portable-build configuration and update
handling so portable executables do not use NSIS-only electron-updater actions
such as auto-download, download-and-install, or quitAndInstall. Add a
portable-specific path that offers only downloading the replacement executable,
or disable electron-updater when the portable executable is detected, while
preserving the existing NSIS update flow.
In `@electron/electron-env.d.ts`:
- Around line 881-892: Update the electron API declarations for
chooseWorkspaceDirectory and openWorkspaceDirectory to include the handler
response fields message and path, respectively, while preserving their existing
optionality and response shapes.
In `@electron/ipc/register/project.ts`:
- Around line 445-452: Update the catch blocks for choose-workspace-directory
and cleanup-recordly-temporary-files in the IPC registration function to call
dialog.showMessageBox with an error message before returning the existing {
success: false, error, message } result. Match the dialog pattern used by their
success paths and preserve the current failure response structure.
In `@electron/ipc/register/recording.ts`:
- Around line 434-443: Reset windowsCaptureOutputBuffer to an empty value before
prepareWindowsCaptureTempDirectory runs in the recording setup try block. Keep
the existing captureOutput fallback and failure-reporting flow unchanged so
failures during temporary-directory preparation use the current exception rather
than stale output from a previous attempt.
In `@electron/ipc/utils.ts`:
- Around line 113-127: Update getRecordingsDir to prefer
getConfiguredWorkspaceLayout()?.recordings over customRecordingsDir, matching
the workspace-first precedence already used by getProjectsStorageDir. Retain
RECORDINGS_DIR as the final fallback and continue creating and returning the
selected target directory.
In `@electron/main.ts`:
- Around line 125-129: Update the startup logging around the five directory path
messages in the main-process initialization to avoid exposing usernames at info
level: either emit these paths through the application's debug-level logger or
reduce the output to only the support-required paths. Keep the existing
directory resolution behavior unchanged.
In `@electron/storageSettings.ts`:
- Around line 412-448: Scope cleanupRecordlyTempArtifacts to an app-private
Recordly temporary subdirectory instead of the shared system temp directory, and
update the capture/export callers to create and write artifacts there
consistently. Use the same path construction, such as the Recordly-specific
child of app.getPath("temp"), wherever temp artifacts are produced and where
main.ts invokes cleanup, preserving the existing prefix and age filtering within
that directory.
- Around line 233-255: In rewriteMigratedProjectFile, limit the “unknown JSON
file” fallback to read and JSON.parse failures only; after successful parsing,
let rewriteStoredPaths, writeFile, and rename errors propagate to
migrateStorageData. Preserve skipping unreadable or invalid files, while
ensuring migration stops on write/rename failure and does not silently leave
temporary files or stale paths.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Line 452: Update the release output documentation in RELEASING.md to list
Recordly-windows-portable-x64.exe and explicitly identify
Recordly-windows-setup-x64.exe as the WinGet installer.
In `@electron/main.ts`:
- Around line 1015-1025: Update the startup cleanup block after
ensureStorageDirectories so cleanupRecordlyTempArtifacts runs without being
awaited, while preserving its existing success logging and catch-based warning
handling. Ensure startup proceeds immediately and cleanup errors remain handled.
In `@electron/storageSettings.test.ts`:
- Around line 92-125: Extend the storage migration tests covering
migrateStorageData and deleteMigratedSourceFiles with two cases: pre-create a
destination file to verify COPYFILE_EXCL behavior reports skippedConflicts as 1
and leaves the conflicting source untouched, and use a destination whose root is
nested under source.recordings to verify migrateStorageData rejects. Preserve
the existing successful migration assertions.
- Around line 50-69: Isolate storage-settings tests from leaked module state and
ensure the initialization test verifies initializeWorkspaceStorage itself. Add
an afterEach reset for activeWorkspaceRoot and workspaceInitializationError via
a small test hook, or otherwise drive the empty-settings null branch; update the
test to assert the null workspace result and that setPath is not called when no
workspace is configured.
In `@electron/storageSettings.ts`:
- Around line 375-387: Update the deletion loop over copiedFiles to compare
destinationStats.size with sourceStats.size after confirming both are regular
files, and skip unlinking when the sizes differ. Keep the existing unlink and
counter updates only for matching file sizes.
- Around line 125-126: Update the write-and-rename flow around
fsPromises.writeFile and fsPromises.rename to flush the temporary settings file
before renaming it, and remove temporaryPath when the operation fails. Preserve
the existing recovery behavior while ensuring failed writes do not leave stale
.tmp files behind.
- Around line 139-159: Update getPathSize to eliminate unbounded Promise.all
recursion when traversing directory entries, using bounded concurrency or
sequential processing per directory. Preserve existing handling for missing
paths, symbolic links, files, and the summed directory size returned to
getWorkspaceUsage.
- Around line 245-248: Add a concise comment immediately above the mappings in
the rewriteStoredPaths call explaining that order is significant: the projects
mapping must precede recordings because the projects source may be nested under
recordings and rewriteStoredPaths processes mappings sequentially. Preserve the
existing mapping order.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bcf7ec3-a226-4d07-8e17-b33fba4db21c
📒 Files selected for processing (33)
.github/workflows/build.yml.github/workflows/release.yml.github/workflows/winget-releaser.ymlelectron-builder.json5electron/appPaths.tselectron/electron-env.d.tselectron/ipc/project/manager.tselectron/ipc/recording/prune.tselectron/ipc/recording/windows.test.tselectron/ipc/recording/windows.tselectron/ipc/register/project.tselectron/ipc/register/recording.tselectron/ipc/utils.tselectron/main.tselectron/native/wgc-capture/src/main.cppelectron/preload.tselectron/storageSettings.test.tselectron/storageSettings.tssrc/components/launch/LaunchWindow.tsxsrc/components/launch/hooks/useLaunchWindowSystemState.tssrc/components/launch/popovers/MorePopover.tsxsrc/hooks/useScreenRecorder.tssrc/i18n/locales/de/launch.jsonsrc/i18n/locales/en/launch.jsonsrc/i18n/locales/es/launch.jsonsrc/i18n/locales/fr/launch.jsonsrc/i18n/locales/it/launch.jsonsrc/i18n/locales/ko/launch.jsonsrc/i18n/locales/nl/launch.jsonsrc/i18n/locales/pt-BR/launch.jsonsrc/i18n/locales/ru/launch.jsonsrc/i18n/locales/zh-CN/launch.jsonsrc/i18n/locales/zh-TW/launch.json
| "nsis", | ||
| "portable" | ||
| ], | ||
| "icon": "icons/icons/win/icon.ico" | ||
| , | ||
| "executableName": "Recordly", | ||
| "artifactName": "${productName}-windows-${arch}.${ext}" | ||
| "icon": "icons/icons/win/icon.ico", | ||
| "executableName": "Recordly" | ||
| }, | ||
| "nsis": { | ||
| "oneClick": false, | ||
| "allowToChangeInstallationDirectory": true, | ||
| "artifactName": "${productName}-windows-setup-${arch}.${ext}" | ||
| }, | ||
| "portable": { | ||
| "artifactName": "${productName}-windows-portable-${arch}.${ext}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 \
'electron-updater|autoUpdater|checkForUpdates|checkForUpdatesAndNotify|PORTABLE_EXECUTABLE_(FILE|DIR)' \
--glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.mjs' --glob '*.json' --glob '*.json5' . || trueRepository: webadderallorg/Recordly
Length of output: 12176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- electron/updater.ts top-level and relevant blocks ---\n'
sed -n '1,100p' electron/updater.ts
sed -n '330,450p' electron/updater.ts
sed -n '560,660p' electron/updater.ts
printf '\n--- package manager files ---\n'
[ -f package.json ] && cat package.json | sed -n '1,80p'
[ -f electron-builder.json5 ] && sed -n '1,130p' electron-builder.json5
printf '\n--- files mentioning update initialization call / environment overrides ---\n'
rg -n -C 4 'initUpdater|RECORDLY_DISABLE_AUTO_UPDATES|AUTO_UPDATES_DISABLED|publishAutoUpdate|electronbuilder|publisher' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.json' --glob '*.json5' . || trueRepository: webadderallorg/Recordly
Length of output: 252
🌐 Web query:
electron-builder 26.7.0 portable target autoUpdater portable auto-updates manual download only
💡 Result:
In electron-builder, the portable target is explicitly designed for scenarios where no installation is desired, and as a result, it does not support the built-in auto-update mechanism provided by the electron-updater package [1][2]. Official documentation and maintainer guidance confirm that auto-updates are currently only supported for standard installer-based targets on Windows, such as the NSIS target [1][3][4]. Because the portable target does not produce the necessary metadata (such as latest.yml) required for the electron-updater to function, you cannot use the standard auto-update flow [3][5][2]. To achieve update functionality for a portable application, you must implement a custom solution [1][2]. Common approaches for implementing custom "manual download only" updates include: 1. Custom Update Check: Your application can periodically query a remote JSON file (hosted on your own server or GitHub releases) that contains version and download URL information [3][5]. 2. Manual Download & Replace: If a newer version is detected, you can prompt the user to download the new executable [6][7]. Since a portable executable cannot easily self-update (replace its own file while running), this process typically involves: - Directing the user to a download page via the browser (shell.openExternal). - Downloading the new version to a temporary directory. - Using a small helper script or a secondary process to replace the old executable with the new one upon next launch. If your requirements allow for standard installers, migrating to the NSIS target is the recommended way to gain automatic, seamless updates with minimal configuration [1][3][4].
Citations:
- 1: https://www.electron.build/docs/targets/
- 2: windows portable apps and automatic update electron-userland/electron-builder#1813
- 3: https://www.electron.build/docs/features/auto-update
- 4: https://mintlify.wiki/electron-userland/electron-builder/distribution/auto-update
- 5: https://github.com/electron-userland/electron-builder/blob/b01d5225631115f6f301cb113b044fd10ebb5256/docs/auto-update.md
- 6: https://www.electron.build/electron-updater.class.appupdater
- 7: https://github.com/electron-userland/electron-builder/blob/b06c5effd35051c6b953696aec90b68a67456bed/packages/electron-updater/src/AppUpdater.ts
🌐 Web query:
electron-updater 6.8.3 portable portableExecutables path auto-download quitAndInstall portable
💡 Result:
Electron-updater is designed primarily for applications installed via standard installers (like NSIS) [1][2]. It does not natively support an auto-update and "quitAndInstall" flow for applications built with the portable target [3]. Key technical details regarding your query: 1. Portable vs. Installer: The portable target in electron-builder creates a self-contained executable that does not require installation [3]. Because it lacks an underlying installation directory and registry presence, the standard quitAndInstall mechanism—which typically replaces files in an installation folder—is not compatible with the portable format [4][5]. 2. Auto-Download and quitAndInstall: While you can use electron-updater to check for and download updates in a portable app, the quitAndInstall() method will not perform an in-place update for a portable executable [4][5]. If you attempt to use it, the process will fail to replace the running executable because the file is locked by the operating system. 3. Managing Portable Updates: To update a portable application, the standard pattern is to download the new portable executable to a temporary location (e.g., using electron-updater's manual download features or a custom downloader) [6][1]. Your application must then launch the new executable, close itself, and allow the new version to overwrite or replace the old one, often facilitated by a small wrapper or updater script that handles the file replacement after the main process exits. 4. Pathing: Portable apps provide environment variables to help you locate your runtime context [3]: - PORTABLE_EXECUTABLE_DIR: The directory where your current portable executable is located. - PORTABLE_EXECUTABLE_FILE: The full path to the currently running portable executable. If you require seamless "quit and install" auto-updating, consider using the NSIS target instead, which is explicitly designed to handle file replacement and installation workflows [1][3].
Citations:
- 1: https://mintlify.wiki/electron-userland/electron-builder/guides/auto-update/setup
- 2: https://www.electron.build/docs/features/auto-update
- 3: https://mintlify.wiki/electron-userland/electron-builder/packaging/windows
- 4: https://github.com/electron-userland/electron-builder/blob/master/packages/electron-updater/src/BaseUpdater.ts
- 5: https://www.jsdocs.io/package/electron-updater
- 6: https://www.electron.build/electron-updater.class.appupdater
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- electron/updater.ts top-level and relevant blocks ---'
sed -n '1,100p' electron/updater.ts
sed -n '330,450p' electron/updater.ts
sed -n '560,660p' electron/updater.ts
printf '%s\n' ''
printf '%s\n' '--- package manager files ---'
if [ -f package.json ]; then cat package.json | sed -n '1,80p'; fi
if [ -f electron-builder.json5 ]; then sed -n '1,130p' electron-builder.json5; fi
printf '%s\n' ''
printf '%s\n' '--- files mentioning update initialization call / environment overrides ---'
rg -n -C 4 'initUpdater|RECORDLY_DISABLE_AUTO_UPDATES|AUTO_UPDATES_DISABLED|publishAutoUpdate|electronbuilder|publisher' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.json' --glob '*.json5' . || trueRepository: webadderallorg/Recordly
Length of output: 19831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'setupAutoUpdates|checkForAppUpdates|installDownloadedUpdateNow|downloadAvailableUpdate|isAutoUpdateFeatureEnabled|autoUpdates' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' --glob '*.mjs' . || true
printf '%s\n' ''
printf '%s\n' '--- appPaths.ts portable env usage ---'
if [ -f electron/appPaths.ts ]; then
sed -n '1,200p' electron/appPaths.ts
fiRepository: webadderallorg/Recordly
Length of output: 12970
Handle updates separately for portable builds.
electron-updater is initialized for all packaged releases and offers NSIS-style auto-download, download-and-install, and autoUpdater.quitAndInstall() paths. With publishAutoUpdate enabled and both nsis and portable targets added, portable builds can show update actions that target an installer-only flow. Add a portable-specific update path, such as offering only the portable download, or disable electron-updater when the portable executable is detected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron-builder.json5` around lines 106 - 118, Update the portable-build
configuration and update handling so portable executables do not use NSIS-only
electron-updater actions such as auto-download, download-and-install, or
quitAndInstall. Add a portable-specific path that offers only downloading the
replacement executable, or disable electron-updater when the portable executable
is detected, while preserving the existing NSIS update flow.
Source: MCP tools
| chooseWorkspaceDirectory: () => Promise<{ | ||
| success: boolean; | ||
| canceled?: boolean; | ||
| workspaceRoot?: string; | ||
| recordingsDir?: string; | ||
| projectsDir?: string; | ||
| tempDir?: string; | ||
| cacheDir?: string; | ||
| restartRequired?: boolean; | ||
| error?: string; | ||
| }>; | ||
| openWorkspaceDirectory: () => Promise<{ success: boolean; error?: string }>; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Add missing message and path fields to match the actual handler return shapes.
chooseWorkspaceDirectory's catch branch in electron/ipc/register/project.ts returns message: "Failed to configure RecordlyData", but this type has no message field. openWorkspaceDirectory's success branch returns path: targetPath, but this type has no path field at all.
Proposed fix
chooseWorkspaceDirectory: () => Promise<{
success: boolean;
canceled?: boolean;
workspaceRoot?: string;
recordingsDir?: string;
projectsDir?: string;
tempDir?: string;
cacheDir?: string;
restartRequired?: boolean;
+ message?: string;
error?: string;
}>;
- openWorkspaceDirectory: () => Promise<{ success: boolean; error?: string }>;
+ openWorkspaceDirectory: () => Promise<{ success: boolean; path?: string; error?: string }>;📝 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.
| chooseWorkspaceDirectory: () => Promise<{ | |
| success: boolean; | |
| canceled?: boolean; | |
| workspaceRoot?: string; | |
| recordingsDir?: string; | |
| projectsDir?: string; | |
| tempDir?: string; | |
| cacheDir?: string; | |
| restartRequired?: boolean; | |
| error?: string; | |
| }>; | |
| openWorkspaceDirectory: () => Promise<{ success: boolean; error?: string }>; | |
| chooseWorkspaceDirectory: () => Promise<{ | |
| success: boolean; | |
| canceled?: boolean; | |
| workspaceRoot?: string; | |
| recordingsDir?: string; | |
| projectsDir?: string; | |
| tempDir?: string; | |
| cacheDir?: string; | |
| restartRequired?: boolean; | |
| message?: string; | |
| error?: string; | |
| }>; | |
| openWorkspaceDirectory: () => Promise<{ success: boolean; path?: string; error?: string }>; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/electron-env.d.ts` around lines 881 - 892, Update the electron API
declarations for chooseWorkspaceDirectory and openWorkspaceDirectory to include
the handler response fields message and path, respectively, while preserving
their existing optionality and response shapes.
| } catch (error) { | ||
| return { | ||
| success: false, | ||
| error: String(error), | ||
| message: "Failed to configure RecordlyData", | ||
| }; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Show an error dialog on failure.
choose-workspace-directory and cleanup-recordly-temporary-files show dialog.showMessageBox on success, but their catch blocks only return {success: false, ...}. The renderer callbacks in useLaunchWindowSystemState.ts call console.error on failure and display nothing to the user. If a workspace migration or a temp-file cleanup fails, the user gets no indication that anything went wrong.
Show an error dialog before returning the failure result, matching the success-path pattern already used in this function.
Proposed fix
} catch (error) {
+ dialog.showErrorBox("Failed to configure RecordlyData", String(error));
return {
success: false,
error: String(error),
message: "Failed to configure RecordlyData",
};
}
}); } catch (error) {
+ dialog.showErrorBox("Failed to clean temporary files", String(error));
return { success: false, error: String(error) };
}
});Also applies to: 467-481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/ipc/register/project.ts` around lines 445 - 452, Update the catch
blocks for choose-workspace-directory and cleanup-recordly-temporary-files in
the IPC registration function to call dialog.showMessageBox with an error
message before returning the existing { success: false, error, message } result.
Match the dialog pattern used by their success paths and preserve the current
failure response structure.
| let captureOutput = ""; | ||
| const tempDirectory = app.getPath("temp"); | ||
| try { | ||
| const exePath = getWindowsCaptureExePath(); | ||
| const recordingsDir = await getRecordingsDir(); | ||
| await prepareWindowsCaptureTempDirectory(tempDirectory); | ||
| const timestamp = Date.now(); | ||
| const outputPath = path.join(recordingsDir, `recording-${timestamp}.mp4`); | ||
| tempVideoPath = path.join(app.getPath("temp"), `recordly-native-${timestamp}.mp4`); | ||
| tempVideoPath = path.join(tempDirectory, `recordly-native-${timestamp}.mp4`); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset windowsCaptureOutputBuffer before prepareWindowsCaptureTempDirectory runs, so stale output does not shadow the current failure.
captureOutput stays "" until wcProc.stdout/stderr emit data, which only happens after prepareWindowsCaptureTempDirectory succeeds and the helper process spawns. setWindowsCaptureOutputBuffer("") does not run until line 544, well after prepareWindowsCaptureTempDirectory at line 439.
If prepareWindowsCaptureTempDirectory throws (insufficient free space, write permission failure), the catch block at line 600 builds failureDetail from captureOutput || windowsCaptureOutputBuffer. Since captureOutput is falsy, it falls back to the global windowsCaptureOutputBuffer, which can still hold an unrelated ERROR/WARNING line left over from a previous failed attempt (this global is never cleared on failure paths). describeWindowsCaptureStartFailure prefers that stale helper line over the actual current error message, so the user sees a misleading, out-of-date diagnostic instead of the real cause (for example, a stale "Failed to initialize Media Foundation encoder" message masking a current disk-space error). The same stale value also flows into recordNativeCaptureDiagnostics's processOutput field at line 614.
Clear the buffer before the temp-directory preparation so any failure at that stage reports the current error, not a leftover one.
🐛 Proposed fix
let wcProc: ChildProcessWithoutNullStreams | null = null;
let tempVideoPath: string | null = null;
let tempSystemAudioPath: string | null = null;
let tempMicPath: string | null = null;
let captureOutput = "";
const tempDirectory = app.getPath("temp");
try {
+ setWindowsCaptureOutputBuffer("");
const exePath = getWindowsCaptureExePath();
const recordingsDir = await getRecordingsDir();
await prepareWindowsCaptureTempDirectory(tempDirectory);📝 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.
| let captureOutput = ""; | |
| const tempDirectory = app.getPath("temp"); | |
| try { | |
| const exePath = getWindowsCaptureExePath(); | |
| const recordingsDir = await getRecordingsDir(); | |
| await prepareWindowsCaptureTempDirectory(tempDirectory); | |
| const timestamp = Date.now(); | |
| const outputPath = path.join(recordingsDir, `recording-${timestamp}.mp4`); | |
| tempVideoPath = path.join(app.getPath("temp"), `recordly-native-${timestamp}.mp4`); | |
| tempVideoPath = path.join(tempDirectory, `recordly-native-${timestamp}.mp4`); | |
| let captureOutput = ""; | |
| const tempDirectory = app.getPath("temp"); | |
| try { | |
| setWindowsCaptureOutputBuffer(""); | |
| const exePath = getWindowsCaptureExePath(); | |
| const recordingsDir = await getRecordingsDir(); | |
| await prepareWindowsCaptureTempDirectory(tempDirectory); | |
| const timestamp = Date.now(); | |
| const outputPath = path.join(recordingsDir, `recording-${timestamp}.mp4`); | |
| tempVideoPath = path.join(tempDirectory, `recordly-native-${timestamp}.mp4`); | |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import type { ChildProcessWithoutNullStreams } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/ipc/register/recording.ts` around lines 434 - 443, Reset
windowsCaptureOutputBuffer to an empty value before
prepareWindowsCaptureTempDirectory runs in the recording setup try block. Keep
the existing captureOutput fallback and failure-reporting flow unchanged so
failures during temporary-directory preparation use the current exception rather
than stale output from a previous attempt.
| export async function getRecordingsDir() { | ||
| await loadRecordingsDirectorySetting(); | ||
| const targetDir = customRecordingsDir ?? RECORDINGS_DIR; | ||
| const targetDir = | ||
| customRecordingsDir ?? getConfiguredWorkspaceLayout()?.recordings ?? RECORDINGS_DIR; | ||
| await fs.mkdir(targetDir, { recursive: true }); | ||
| return targetDir; | ||
| } | ||
|
|
||
| export async function getProjectsStorageDir() { | ||
| const workspaceProjectsDir = getConfiguredWorkspaceLayout()?.projects; | ||
| const targetDir = | ||
| workspaceProjectsDir ?? path.join(await getRecordingsDir(), PROJECTS_DIRECTORY_NAME); | ||
| await fs.mkdir(targetDir, { recursive: true }); | ||
| return targetDir; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'utils\.ts|project\.ts$|storage|status|workspace' electron | sed 's#^\./##' | head -100
echo
echo "== git diff stat =="
git diff --stat || true
echo
echo "== relevant occurrences =="
rg -n "getRecordingsDir|getProjectsStorageDir|customRecordingsDir|recordings|projects|getWorkspaceUsage|get-storage-status|Choose recordings folder|recordingsDirectory|loading settings|store\.get" electron -SRepository: webadderallorg/Recordly
Length of output: 18283
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ipc/utils.ts relevant section =="
cat -n electron/ipc/utils.ts | sed -n '1,180p'
echo
echo "== ipc/state.ts relevant section =="
cat -n electron/ipc/state.ts | sed -n '1,80p;180,225p'
echo
echo "== storageSettings.ts relevant functions =="
cat -n electron/ipc/storageSettings.ts | sed -n '1,80p;170,240p;350,415p'
echo
echo "== registrar storage status/workspace layout section =="
cat -n electron/ipc/register/project.ts | sed -n '280,395p;450,495p'
echo
echo "== read-only precedence probe from source text =="
python3 - <<'PY'
from pathlib import Path
import re
src = Path("electron/ipc/utils.ts").read_text()
m = re.search(r"export async function getRecordingsDir\(\).*?\{(?P<body>.*?)\n\}", src, re.S)
print("getRecordingsDir body:")
if m:
body = re.sub(r"\s+", " ", m.group("body")).strip()
print(body)
print("customRecordingsDir first:", body.index("customRecordingsDir") < body.index("getConfiguredWorkspaceLayout"))
proj_src = Path("electron/ipc/utils.ts").read_text()
pm = re.search(r"export async function getProjectsStorageDir\(\).*?\{(?P<body>.*?)\n\}", proj_src, re.S)
print("getProjectsStorageDir body:")
if pm:
body = re.sub(r"\s+", " ", pm.group("body")).strip()
print(body)
print("workspace projects first:", "getConfiguredWorkspaceLayout()?.projects" in body and "workspaceProjectsDir" in body)
print("storage status imports workspace layout usage:", bool((Path("electron/ipc/register/project.ts").read_text()).count("getWorkspaceUsage")))
PYRepository: webadderallorg/Recordly
Length of output: 11616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== appPaths =="
cat -n electron/appPaths.ts | sed -n '1,90p'
echo
echo "== storage status and workspace registration =="
cat -n electron/ipc/register/project.ts | sed -n '280,420p'
echo
echo "== storage settings =="
cat -n electron/electron/ipc/storageSettings.ts 2>/dev/null || fd -a 'storageSettings|storage.*settings' electron | while read -r f; do echo "--- $f"; sed -n '1,140p;330,415p' "$f"; done
echo
echo "== precise precedence call sites =="
rg -n "getStorageStatus|getWorkspaceUsage|persistRecordingsDirectorySetting|choose-recordings-directory|get-recordings-directory|recordingsDir:" electron/ipc/register/project.ts electron/ipc/utils.ts electron/ipc/state.ts electron/appPaths.ts -SRepository: webadderallorg/Recordly
Length of output: 20495
Make getRecordingsDir match the workspace-first precedence.
choose-recordings-directory persists a legacy recordingsDir setting, but get-storage-status reports size from getWorkspaceUsage(workspace), which uses getConfiguredWorkspaceLayout().recordings and .projects. If a workspace is active and the legacy setting provides a different path, recordings can be written under the custom directory while reported storage usage, migration source paths, and project storage use the workspace layout. Swap the precedence so active layout paths win over the legacy recordings directory.
Proposed fix
export async function getRecordingsDir() {
await loadRecordingsDirectorySetting();
const targetDir =
- customRecordingsDir ?? getConfiguredWorkspaceLayout()?.recordings ?? RECORDINGS_DIR;
+ getConfiguredWorkspaceLayout()?.recordings ?? customRecordingsDir ?? RECORDINGS_DIR;
await fs.mkdir(targetDir, { recursive: true });
return targetDir;
}📝 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.
| export async function getRecordingsDir() { | |
| await loadRecordingsDirectorySetting(); | |
| const targetDir = customRecordingsDir ?? RECORDINGS_DIR; | |
| const targetDir = | |
| customRecordingsDir ?? getConfiguredWorkspaceLayout()?.recordings ?? RECORDINGS_DIR; | |
| await fs.mkdir(targetDir, { recursive: true }); | |
| return targetDir; | |
| } | |
| export async function getProjectsStorageDir() { | |
| const workspaceProjectsDir = getConfiguredWorkspaceLayout()?.projects; | |
| const targetDir = | |
| workspaceProjectsDir ?? path.join(await getRecordingsDir(), PROJECTS_DIRECTORY_NAME); | |
| await fs.mkdir(targetDir, { recursive: true }); | |
| return targetDir; | |
| } | |
| export async function getRecordingsDir() { | |
| await loadRecordingsDirectorySetting(); | |
| const targetDir = | |
| getConfiguredWorkspaceLayout()?.recordings ?? customRecordingsDir ?? RECORDINGS_DIR; | |
| await fs.mkdir(targetDir, { recursive: true }); | |
| return targetDir; | |
| } | |
| export async function getProjectsStorageDir() { | |
| const workspaceProjectsDir = getConfiguredWorkspaceLayout()?.projects; | |
| const targetDir = | |
| workspaceProjectsDir ?? path.join(await getRecordingsDir(), PROJECTS_DIRECTORY_NAME); | |
| await fs.mkdir(targetDir, { recursive: true }); | |
| return targetDir; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/ipc/utils.ts` around lines 113 - 127, Update getRecordingsDir to
prefer getConfiguredWorkspaceLayout()?.recordings over customRecordingsDir,
matching the workspace-first precedence already used by getProjectsStorageDir.
Retain RECORDINGS_DIR as the final fallback and continue creating and returning
the selected target directory.
| console.log("Recordings directory:", recordingsDir); | ||
| console.log("Projects directory:", projectsDir); | ||
| console.log("Temporary directory:", app.getPath("temp")); | ||
| console.log("Cache directory:", app.getPath("sessionData")); | ||
| console.log("User Data Path:", app.getPath("userData")); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
The startup log writes five absolute paths that contain the OS username.
Every one of these paths embeds the account name on all three platforms, for example /Users/<name>/... or C:\Users\<name>\.... This runs at info level on every startup. Main-process console output is commonly collected into diagnostic and crash bundles, so the username travels with them.
The previous code logged one path. This change logs five.
Log the paths at debug level, or log only the paths a support engineer needs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/main.ts` around lines 125 - 129, Update the startup logging around
the five directory path messages in the main-process initialization to avoid
exposing usernames at info level: either emit these paths through the
application's debug-level logger or reduce the output to only the
support-required paths. Keep the existing directory resolution behavior
unchanged.
| async function rewriteMigratedProjectFile( | ||
| projectPath: string, | ||
| source: WorkspaceMigrationSource, | ||
| destination: WorkspaceLayout, | ||
| ) { | ||
| if (!/[.](?:recordly|json)$/i.test(projectPath)) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const content = await fsPromises.readFile(projectPath, "utf-8"); | ||
| const project = JSON.parse(content) as unknown; | ||
| const rewritten = rewriteStoredPaths(project, [ | ||
| { source: source.projects, destination: destination.projects }, | ||
| { source: source.recordings, destination: destination.recordings }, | ||
| ]); | ||
| const temporaryPath = `${projectPath}.migration.tmp`; | ||
| await fsPromises.writeFile(temporaryPath, JSON.stringify(rewritten, null, 2), "utf-8"); | ||
| await fsPromises.rename(temporaryPath, projectPath); | ||
| } catch { | ||
| // Not every JSON file in Projects is a Recordly project. Leave unknown files unchanged. | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Narrow the catch so a failed path rewrite is not silent.
The try block at line 242 wraps the read, the parse, the write, and the rename. The comment explains that unknown JSON files must be left unchanged. That reasoning applies only to the read and parse steps. The catch also swallows writeFile and rename failures.
The consequence is a data-loss path. If the rewrite fails after a successful parse, the migrated project keeps absolute paths that point at the old location, and migrateStorageData still reports success. The choose-workspace-directory handler then offers to delete the copied originals. If the user accepts, the project references files that no longer exist, and no error was ever surfaced.
A failed rename also leaves a <project>.migration.tmp file inside the new Projects directory.
Separate the two concerns. Treat a read or parse failure as "not a Recordly project" and skip it. Let a write or rename failure propagate so the caller can stop before it offers deletion.
🐛 Proposed fix
async function rewriteMigratedProjectFile(
projectPath: string,
source: WorkspaceMigrationSource,
destination: WorkspaceLayout,
) {
if (!/[.](?:recordly|json)$/i.test(projectPath)) {
return;
}
+ let project: unknown;
try {
const content = await fsPromises.readFile(projectPath, "utf-8");
- const project = JSON.parse(content) as unknown;
- const rewritten = rewriteStoredPaths(project, [
- { source: source.projects, destination: destination.projects },
- { source: source.recordings, destination: destination.recordings },
- ]);
- const temporaryPath = `${projectPath}.migration.tmp`;
- await fsPromises.writeFile(temporaryPath, JSON.stringify(rewritten, null, 2), "utf-8");
- await fsPromises.rename(temporaryPath, projectPath);
+ project = JSON.parse(content) as unknown;
} catch {
// Not every JSON file in Projects is a Recordly project. Leave unknown files unchanged.
+ return;
}
+
+ const rewritten = rewriteStoredPaths(project, [
+ { source: source.projects, destination: destination.projects },
+ { source: source.recordings, destination: destination.recordings },
+ ]);
+ const temporaryPath = `${projectPath}.migration.tmp`;
+ try {
+ await fsPromises.writeFile(temporaryPath, JSON.stringify(rewritten, null, 2), "utf-8");
+ await fsPromises.rename(temporaryPath, projectPath);
+ } catch (error) {
+ await fsPromises.rm(temporaryPath, { force: true });
+ throw error;
+ }
}📝 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.
| async function rewriteMigratedProjectFile( | |
| projectPath: string, | |
| source: WorkspaceMigrationSource, | |
| destination: WorkspaceLayout, | |
| ) { | |
| if (!/[.](?:recordly|json)$/i.test(projectPath)) { | |
| return; | |
| } | |
| try { | |
| const content = await fsPromises.readFile(projectPath, "utf-8"); | |
| const project = JSON.parse(content) as unknown; | |
| const rewritten = rewriteStoredPaths(project, [ | |
| { source: source.projects, destination: destination.projects }, | |
| { source: source.recordings, destination: destination.recordings }, | |
| ]); | |
| const temporaryPath = `${projectPath}.migration.tmp`; | |
| await fsPromises.writeFile(temporaryPath, JSON.stringify(rewritten, null, 2), "utf-8"); | |
| await fsPromises.rename(temporaryPath, projectPath); | |
| } catch { | |
| // Not every JSON file in Projects is a Recordly project. Leave unknown files unchanged. | |
| } | |
| } | |
| async function rewriteMigratedProjectFile( | |
| projectPath: string, | |
| source: WorkspaceMigrationSource, | |
| destination: WorkspaceLayout, | |
| ) { | |
| if (!/[.](?:recordly|json)$/i.test(projectPath)) { | |
| return; | |
| } | |
| let project: unknown; | |
| try { | |
| const content = await fsPromises.readFile(projectPath, "utf-8"); | |
| project = JSON.parse(content) as unknown; | |
| } catch { | |
| // Not every JSON file in Projects is a Recordly project. Leave unknown files unchanged. | |
| return; | |
| } | |
| const rewritten = rewriteStoredPaths(project, [ | |
| { source: source.projects, destination: destination.projects }, | |
| { source: source.recordings, destination: destination.recordings }, | |
| ]); | |
| const temporaryPath = `${projectPath}.migration.tmp`; | |
| try { | |
| await fsPromises.writeFile(temporaryPath, JSON.stringify(rewritten, null, 2), "utf-8"); | |
| await fsPromises.rename(temporaryPath, projectPath); | |
| } catch (error) { | |
| await fsPromises.rm(temporaryPath, { force: true }); | |
| throw error; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/storageSettings.ts` around lines 233 - 255, In
rewriteMigratedProjectFile, limit the “unknown JSON file” fallback to read and
JSON.parse failures only; after successful parsing, let rewriteStoredPaths,
writeFile, and rename errors propagate to migrateStorageData. Preserve skipping
unreadable or invalid files, while ensuring migration stops on write/rename
failure and does not silently leave temporary files or stale paths.
| const RECORDLY_TEMP_ARTIFACT_PREFIXES = ["recordly-"]; | ||
|
|
||
| export async function cleanupRecordlyTempArtifacts( | ||
| tempDirectory: string, | ||
| minimumAgeMs = 60 * 60 * 1_000, | ||
| ) { | ||
| const now = Date.now(); | ||
| let removedBytes = 0; | ||
| let removedCount = 0; | ||
| let entries: fs.Dirent[]; | ||
|
|
||
| try { | ||
| entries = await fsPromises.readdir(tempDirectory, { withFileTypes: true }); | ||
| } catch { | ||
| return { removedBytes, removedCount }; | ||
| } | ||
|
|
||
| for (const entry of entries) { | ||
| if (!RECORDLY_TEMP_ARTIFACT_PREFIXES.some((prefix) => entry.name.startsWith(prefix))) { | ||
| continue; | ||
| } | ||
|
|
||
| const artifactPath = path.join(tempDirectory, entry.name); | ||
| const stats = await fsPromises.stat(artifactPath).catch(() => null); | ||
| if (!stats || now - stats.mtimeMs < minimumAgeMs) { | ||
| continue; | ||
| } | ||
|
|
||
| const artifactBytes = await getPathSize(artifactPath); | ||
| try { | ||
| await fsPromises.rm(artifactPath, { force: true, recursive: true }); | ||
| removedBytes += artifactBytes; | ||
| removedCount += 1; | ||
| } catch { | ||
| // The artifact may still be held by an encoder. Leave it for the next pass. | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Scope the cleanup to an app-private temporary directory.
electron/main.ts line 1017 calls this function with app.getPath("temp"). If the user has not configured a workspace, that path is the shared system temporary directory on Linux and macOS. The only scoping is the recordly- name prefix, so the function deletes any matching entry older than one hour that the process can write.
Two failure modes follow:
- A second Recordly instance writes
/tmp/recordly-native-*.mp4. Some encoders allocate the output file and write the payload only on finalize, so the mtime stays old during a long capture. This function then deletes an in-use output file. - On POSIX,
rmon an open file succeeds. The writing process keeps its descriptor on an unlinked inode, finishes without an error, and the output disappears. The comment at line 446 describes the Windows lock behavior, which does not protect POSIX.
Restrict the scan to a directory that only this application writes, for example path.join(app.getPath("temp"), "recordly"), and make the capture and export code write there. An age filter alone is not sufficient to prove that an artifact is abandoned.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/storageSettings.ts` around lines 412 - 448, Scope
cleanupRecordlyTempArtifacts to an app-private Recordly temporary subdirectory
instead of the shared system temp directory, and update the capture/export
callers to create and write artifacts there consistently. Use the same path
construction, such as the Recordly-specific child of app.getPath("temp"),
wherever temp artifacts are produced and where main.ts invokes cleanup,
preserving the existing prefix and age filtering within that directory.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/timeline/TimelineEditor.tsx (1)
228-238: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the committed containment rule for the drag preview.
Line 232 moves every overlapping zoom in the preview. The committed update in
src/components/video-editor/VideoEditor.tsxmoves only zooms contained within the old clip. A partially overlapping zoom moves during drag and snaps back on drop.Use the same containment test in both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 228 - 238, Update the drag-preview logic in the isMove branch of TimelineEditor to move only zoomRegions fully contained within oldClip, matching the committed update’s containment rule in VideoEditor. Replace the current partial-overlap condition while preserving the existing previewSpans delta updates for contained zooms.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 4140-4143: Update the clip-move handling near setZoomRegions and
setAnnotationRegions so setSpeedRegions and setAudioRegions do not apply the
timeline delta directly to source-coordinate intervals. Preserve speed and audio
regions in source coordinates, or explicitly convert them between source and
timeline coordinates before updating; only timeline-coordinate regions should
shift with the clip.
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 815-819: Update the muted-clip interval construction in the map
following the mute filter so startSec and endSec are converted from source time
to output time via sourceTimeToOutputTime using slices before
scheduleBufferThroughTimeline compares them with audibleRanges. Preserve the
existing nonnegative bounds and interval filtering behavior.
---
Outside diff comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 228-238: Update the drag-preview logic in the isMove branch of
TimelineEditor to move only zoomRegions fully contained within oldClip, matching
the committed update’s containment rule in VideoEditor. Replace the current
partial-overlap condition while preserving the existing previewSpans delta
updates for contained zooms.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f12d444c-3a47-4787-8f22-bcc4767634ce
📒 Files selected for processing (9)
src/components/video-editor/VideoEditor.tsxsrc/components/video-editor/audio/clipAudio.tssrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/model/timelineModel.test.tssrc/components/video-editor/timeline/model/timelineModel.tssrc/components/video-editor/types.test.tssrc/components/video-editor/types.tssrc/lib/exporter/audioEncoder.ts
| Boolean(clip.muted) && getClipSourceEndMs(clip) > getClipSourceStartMs(clip), | ||
| ) | ||
| .map((clip) => ({ | ||
| startSec: Math.max(0, clip.startMs / 1000), | ||
| endSec: Math.max(0, clip.endMs / 1000), | ||
| startSec: Math.max(0, getClipSourceStartMs(clip) / 1000), | ||
| endSec: Math.max(0, getClipSourceEndMs(clip) / 1000), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Convert muted clip bounds to output time before subtracting them.
Lines 817-819 create source-time mute intervals. scheduleBufferThroughTimeline compares them with output-time audibleRanges. After a reflow, a clip at source 8_000–13_000 can render at output 5_000–10_000, so the current logic mutes only the wrong output segment.
Map both bounds through sourceTimeToOutputTime with slices, or perform the mute intersection in source coordinates.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/exporter/audioEncoder.ts` around lines 815 - 819, Update the
muted-clip interval construction in the map following the mute filter so
startSec and endSec are converted from source time to output time via
sourceTimeToOutputTime using slices before scheduleBufferThroughTimeline
compares them with audibleRanges. Preserve the existing nonnegative bounds and
interval filtering behavior.
1 participant
Pull Request Template
Description
Motivation
Type of Change
Related Issue(s)
Screenshots / Video
Screenshot (if applicable):
Video (wherever possible):
Testing Guide
Checklist
Thank you for contributing!
Summary by CodeRabbit
New Features
Bug Fixes
Localization