docs(ai-edition): merge plan, inventories, and collision analysis#35
Open
EtienneLescot wants to merge 2 commits into
Open
docs(ai-edition): merge plan, inventories, and collision analysis#35EtienneLescot wants to merge 2 commits into
EtienneLescot wants to merge 2 commits into
Conversation
Four-doc foundation for the OpenScreen x Axcut AI-edition merge. - ai-edition-merge-plan.md: the 10-phase plan. SSOT is AxcutDocument; Python worker + Fastify server are dropped. Phase 0 vendors the schema and adds the migration; Phase 10 flips the feature flag default. - openscreen-inventory.md: catalog of the OpenScreen codebase as it stands for the merge (13 sections, file:line refs). - axcut-inventory.md: catalog of the axcut codebase (schema, services, providers, Python worker, UI). - ai-edition-collision-analysis.md: every schema/process/UX/feature collision with severity tags and resolution. 8 locked decisions recorded in section 9 with cross-refs to the merge plan section 5. Phase 0 (vendor axcut-schema, schema v3, bidirectional migrateDocument, feature flag) follows in a separate PR.
📝 WalkthroughWalkthroughAdds architecture docs plus the AI Edition schema, Electron bridge, renderer shell, and support modules needed to merge OpenScreen and Axcut into a local-first editor with chat, timeline, transcript, and export flows. ChangesAI Edition merge
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)electron/ai-edition/chat-service.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.21][ERROR]: unable to find a config; path electron/ai-edition/document-service.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.19][ERROR]: unable to find a config; path electron/ai-edition/document-service.test.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.27][ERROR]: unable to find a config; path
🔧 Stylelint (17.13.0)src/components/ai-edition/TranscriptEditor.module.cssError: ENOENT: no such file or directory, open '/.stylelintrc.json' src/components/ai-edition/TimelinePane.module.cssError: ENOENT: no such file or directory, open '/.stylelintrc.json' src/components/ai-edition/VirtualPreview.module.cssError: ENOENT: no such file or directory, open '/.stylelintrc.json' 🔧 markdownlint-cli2 (0.22.1)docs/architecture/ai-edition-merge-plan.mdmarkdownlint-cli2 wrapper config was not available before execution 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: 6
🤖 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 `@docs/architecture/ai-edition-collision-analysis.md`:
- Around line 191-194: The missing-file check in the VirtualPreview/export flow
currently uses fetch on asset.originalPath, but that path is a filesystem path,
not a URL. Replace the fetch-based 200/404 check with a filesystem or IPC-based
existence check in the rendering/recovery path, and keep the “Locate file” flow
tied to updateAsset and electron/dialog.showOpenDialog so the asset path can be
updated when the file is missing.
In `@docs/architecture/ai-edition-merge-plan.md`:
- Around line 100-111: The Phase 0 schema delta is incomplete in this section,
so expand the `AxcutDocument` plan to include the remaining locked changes from
the collision analysis: `cursorTelemetryPath`, `clipId` scoping,
`clip.cursorEnabled`, and removal of the singular `transcript` field. Update the
schema notes in `src/lib/ai-edition/schema/index.ts` and the surrounding
migration/export description so the Phase 0 scope matches all intended fields,
not just `annotations`, `zoomRanges`, and `legacyEditor`.
- Around line 288-295: Phase 9 is missing the planned transcription model
preference, so add `transcriptionModel` to the settings-sync scope alongside the
existing `userPreferences.ts` items. Update the plan to explicitly include
syncing the transcription model picker (`tiny/base/small/medium`) with its
size/speed hints, and make sure it is covered wherever the default
provider/default model/reasoning effort preferences are listed so the setting
has a clear implementation target.
- Around line 123-124: The exporter comparison in the architecture plan
overstates certainty with “we lose nothing in practice,” which conflicts with
the documented fallback and memory-risk caveats. Update the text in the
ai-edition-merge-plan section to use softer, conditional wording that reflects
the current evidence, and align it with the collision analysis and the WebCodecs
fallback path mentioned in src/lib/exporter/.
- Around line 39-50: The migration timing is inconsistent with the rollback
plan: it should not say existing EditorProjectData projects migrate on first
open, because the intended behavior is lazy migration on the first AI Edition
save with a v2 fallback/backup. Update the Migration section to describe that
trigger and keep the rollback-safe path explicit, using the existing
EditorProjectData and legacyEditor migration wording so the docs match the
implemented save-time flow.
In `@docs/architecture/openscreen-inventory.md`:
- Around line 692-699: Update the documentation inventory footer in the
openscreen inventory section so it no longer claims there are “no other .md
files” in the repo, since the merged tree now contains additional architecture
markdown files. Adjust the wording in the docs inventory entry to reflect the
expanded set of markdown documents while keeping the rest of the inventory text
consistent.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6fe19fa0-bac6-4516-8378-43131701e42f
📒 Files selected for processing (4)
docs/architecture/ai-edition-collision-analysis.mddocs/architecture/ai-edition-merge-plan.mddocs/architecture/axcut-inventory.mddocs/architecture/openscreen-inventory.md
Comment on lines
+191
to
+194
| ### 5.2 What if the recording file is moved/deleted? 🟠 | ||
| - axcut trusts absolute paths. If the file is moved, ops referencing `startSec/endSec` still work but rendering breaks (file not found). | ||
| - **Resolution:** on every render of VirtualPreview or export, check `await fetch(asset.originalPath)` for 200/404. On 404, show a "file missing" banner with a "Locate file" button. `electron/dialog.showOpenDialog` to pick the replacement. Update `asset.originalPath` via `updateAsset`. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t use fetch() for missing-file detection.
asset.originalPath is a filesystem path, not a URL; fetch(asset.originalPath) will not reliably hit the media file, so the “Locate file” recovery won’t trigger correctly. This needs a filesystem/IPC check instead.
🤖 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 `@docs/architecture/ai-edition-collision-analysis.md` around lines 191 - 194,
The missing-file check in the VirtualPreview/export flow currently uses fetch on
asset.originalPath, but that path is a filesystem path, not a URL. Replace the
fetch-based 200/404 check with a filesystem or IPC-based existence check in the
rendering/recovery path, and keep the “Locate file” flow tied to updateAsset and
electron/dialog.showOpenDialog so the asset path can be updated when the file is
missing.
Comment on lines
+39
to
+50
| **Migration:** existing `EditorProjectData` projects (v2) get a one-shot migration on first open in AI Edition: | ||
|
|
||
| | Legacy field | Maps to | | ||
| |---|---| | ||
| | `media.screenVideoPath` / `media.cameraVideoPath` | a single `asset` + a single clip spanning its full duration | | ||
| | `editor.trimRegions[]` | the inverse — gaps in the timeline, expressed as `skipRanges` on the asset | | ||
| | `editor.speedRegions[]` | timeline.speedRanges | | ||
| | `editor.zoomRegions[]` | timeline (modeled as `captionRanges` annotation + a new `zoomRanges[]` field added to the schema — see §2.4) | | ||
| | `editor.annotationRegions[]` | a new `annotations[]` field on the schema | | ||
| | `editor.{wallpaper, aspectRatio, cursor, padding, crop, webcam*, motionBlur, borderRadius}` | `editor.*` stays; rename legacy `editor` to `legacyEditor` on migration and apply those fields through the export pipeline the same way as today | | ||
|
|
||
| **Single source of truth, two views.** The renderer keeps a local store of `AxcutDocument` (via Zustand or plain context, TBD in Phase 1); the main process persists it to `app.getPath('userData')/projects/<id>.axcut` and SQLite for sessions/messages/checkpoints/jobs. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the migration trigger with the rollback story.
This says v2 projects migrate on first open, but the collision analysis locks lazy migration on first AI-edition save plus a v2 fallback/backup so users can toggle the flag back off safely. As written, the “first open” wording makes that rollback path impossible.
🤖 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 `@docs/architecture/ai-edition-merge-plan.md` around lines 39 - 50, The
migration timing is inconsistent with the rollback plan: it should not say
existing EditorProjectData projects migrate on first open, because the intended
behavior is lazy migration on the first AI Edition save with a v2
fallback/backup. Update the Migration section to describe that trigger and keep
the rollback-safe path explicit, using the existing EditorProjectData and
legacyEditor migration wording so the docs match the implemented save-time flow.
Comment on lines
+100
to
+111
| Two additions to `AxcutDocument` to keep OpenScreen's existing UX (zoom, annotations, appearance): | ||
|
|
||
| ```ts | ||
| // added to documentSchema in src/lib/ai-edition/schema/index.ts | ||
| annotations: z.array(annotationRegionSchema).default([]), // mirrors editor.annotationRegions | ||
| zoomRanges: z.array(zoomRegionSchema).default([]), // mirrors editor.zoomRegions | ||
| legacyEditor: legacyEditorSchema.nullable().default(null), // migration target for old projects | ||
| ``` | ||
|
|
||
| Legacy fields (`wallpaper`, `aspectRatio`, `cursor`, `webcam*`, `motionBlur`, `borderRadius`, `cropRegion`, `padding`) live under `legacyEditor` and are applied at export time exactly as today. Renderer reads them; main process reads them at export. | ||
|
|
||
| Schema version bumps to **3**. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fold the rest of the Phase 0 schema delta into this section.
This only budgets annotations, zoomRanges, and legacyEditor, but the collision analysis also locks cursorTelemetryPath, clipId scoping, clip.cursorEnabled, and dropping the singular transcript field. As written, Phase 0 is under-scoped.
🤖 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 `@docs/architecture/ai-edition-merge-plan.md` around lines 100 - 111, The Phase
0 schema delta is incomplete in this section, so expand the `AxcutDocument` plan
to include the remaining locked changes from the collision analysis:
`cursorTelemetryPath`, `clipId` scoping, `clip.cursorEnabled`, and removal of
the singular `transcript` field. Update the schema notes in
`src/lib/ai-edition/schema/index.ts` and the surrounding migration/export
description so the Phase 0 scope matches all intended fields, not just
`annotations`, `zoomRanges`, and `legacyEditor`.
Comment on lines
+288
to
+295
| ### Phase 9 — Polish | ||
|
|
||
| - 13-locale i18n pass on every new string (`npm run i18n:check`). | ||
| - Settings sync (`userPreferences.ts`): AI Edition on/off, default provider, default model, default reasoning effort. | ||
| - ROADMAP update: the "AI Edition" section turns from "direction" into "shipped phases X-Y; remaining Z". | ||
| - Empty states, error toasts (sonner already in deps), keyboard shortcuts help. | ||
| - Permission pass on native-bridge changes (record → asset adds a path to the document, which gets rendered back via `file://` — must use the same `assetBaseUrl` pattern already in `transcribe.ts`). | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the transcription model setting here.
Locked decision #5 requires a transcriptionModel picker (tiny/base/small/medium) with size/speed hints, but this phase list only syncs provider/default-model/reasoning-effort preferences. Without a planned home for that setting, the model-default decision has nowhere to land.
🧰 Tools
🪛 LanguageTool
[grammar] ~293-~293: Ensure spelling is correct
Context: ...ining Z". - Empty states, error toasts (sonner already in deps), keyboard shortcuts he...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/architecture/ai-edition-merge-plan.md` around lines 288 - 295, Phase 9
is missing the planned transcription model preference, so add
`transcriptionModel` to the settings-sync scope alongside the existing
`userPreferences.ts` items. Update the plan to explicitly include syncing the
transcription model picker (`tiny/base/small/medium`) with its size/speed hints,
and make sure it is covered wherever the default provider/default
model/reasoning effort preferences are listed so the setting has a clear
implementation target.
Comment on lines
+692
to
+699
| ### Documentation files (`docs/`) | ||
| - `docs/architecture/ai-edition-merge-plan.md` — the merge plan itself. | ||
| - `docs/architecture/native-bridge.md` (39 lines) — describes the versioned IPC pattern. | ||
| - `docs/tests/writing-tests.md`. | ||
| - `docs/testing/windows-native-cursor.md`, `docs/testing/macos-native-cursor.md` — native-helper smoke-test notes. | ||
| - `docs/engineering/windows-native-recorder-roadmap.md`, `docs/engineering/macos-native-recorder-roadmap.md`. | ||
| - Project root also has `README.md` (7976 bytes), `CONTRIBUTING.md`, `LICENSE`, `AGENTS.md` (the canonical agents guide referenced by merge plan). No other `.md` files in the repo (`grep -r '\.md$' --include=*` returns only those). | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the docs inventory footer.
This sentence is now false in the merged tree: this PR adds more architecture markdown files, so the inventory should not say there are “no other .md files” in the repo.
🤖 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 `@docs/architecture/openscreen-inventory.md` around lines 692 - 699, Update the
documentation inventory footer in the openscreen inventory section so it no
longer claims there are “no other .md files” in the repo, since the merged tree
now contains additional architecture markdown files. Adjust the wording in the
docs inventory entry to reflect the expanded set of markdown documents while
keeping the rest of the inventory text consistent.
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (2)
src/components/ai-edition/TranscriptEditor.tsx (1)
78-81: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
isInRangemakes word rendering O(n²).
isInRangecallsfindIndexthree times per word over the fullwordsarray, so rendering the transcript is quadratic in word count. For long transcripts this re-runs on every render and will stutter. Precompute the selected id set once (you already deriveselectedRange); compute the[from, to]index window in auseMemoand do an O(1) membership check per word.🤖 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/ai-edition/TranscriptEditor.tsx` around lines 78 - 81, The word selection logic in TranscriptEditor is doing an O(n²) check because `isInRange` repeatedly scans `transcript.words` for every rendered word. Refactor the range selection path in `TranscriptEditor` to precompute the selected index window or selected id set once with `useMemo` (using the existing `selectedRange`/anchor/focus state), then replace the per-word `isInRange` call with an O(1) membership check inside the word render loop.src/lib/ai-edition/exporter/documentExporter.ts (1)
218-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this type-only import to the top with the other imports.
import type { ExportResult }sits at the bottom of the file. It works via hoisting, but placing it with the other imports improves readability and avoids surprising future readers.🤖 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/ai-edition/exporter/documentExporter.ts` at line 218, The type-only import for ExportResult is separated from the rest of the imports, so move it into the main import block at the top of documentExporter.ts. Keep the existing import grouping consistent by placing import type { ExportResult } alongside the other import statements near the file header, without changing any runtime behavior.
🤖 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/ai-edition/document-service.ts`:
- Line 51: The SUPPORTED_VIDEO_EXTENSIONS set in document-service.ts includes a
duplicate .mp4 entry, so one intended video extension is likely missing. Update
the set to replace the duplicate with the correct missing extension and keep it
aligned with the accepted types in handlers.ts so both the picker and validator
allow the same video formats.
In `@src/components/ai-edition/ChatPanel.tsx`:
- Around line 33-52: The ChatPanel.handleSend flow clears the input and only
appends result.assistantMessage, so the user’s sent prompt never appears in
messages. Update the send path to either optimistically append the trimmed user
text to the messages state before calling nativeBridgeClient.aiEdition.chatRun,
or trigger loadHistory after a successful send so the thread reflects both
sides. Keep the fix localized to handleSend and the messages state update logic
in ChatPanel.
- Around line 29-31: The auto-scroll effect in ChatPanel only runs on mount
because the useEffect dependency array is empty, so it won’t fire after
loadHistory resolves or when new messages are added. Update the effect in
ChatPanel to depend on the message list or whatever state/prop drives the
rendered chat items, so the scrollTo call reruns whenever the chat content
changes.
In `@src/components/ai-edition/EditorSettings.tsx`:
- Around line 298-315: The speed edit handlers are writing to the wrong legacy
field, so changes are saved under a key that neither the reader nor the exporter
uses. In EditorSettings.tsx, update handleSpeedChange and handleSpeedDelete to
persist the same property that is read as sr from the legacy blob (the
speedRegions data), and keep commitToDisk behavior unchanged after the delete
path. Make sure the state update and any legacy write in these callbacks use the
same symbol/shape as the speedRegions reader so updates survive reload and flow
into documentExporter.ts.
In `@src/components/ai-edition/TimelinePane.tsx`:
- Around line 497-500: The drag logic in TimelinePane still relies only on
pointer-end callbacks to remove global listeners and the body class, so
unmounting mid-drag can leave stale handlers attached. Update the resize/drag
setup around the move/end handlers to store the active cleanup in a ref, clear
the window listeners and remove the "timeline-resizing-cut" class from that
cleanup, and invoke it from a useEffect unmount cleanup so any in-progress drag
is safely torn down.
- Around line 706-718: The timeline resize and navigator controls in
TimelinePane are pointer-only, so add keyboard accessibility to the handle
elements and the related cut-adjustment controls. Update the navigator handle
spans and the corresponding resize controls to expose appropriate ARIA semantics
and keyboard event handling so users can move/resize them with keys as well as
pointer input. Reuse the existing startNavigatorDrag flow (and the related cut
adjustment handlers in the other affected block) by wiring keyboard actions to
the same state updates, and ensure the focusable elements are reachable and
announced correctly.
In `@src/components/ai-edition/TranscriptEditor.tsx`:
- Around line 76-99: The word items rendered in TranscriptEditor are mouse-only
because each transcript word is a span with only an onClick handler. Update the
word rendering in the transcript.words.map loop to make each word
keyboard-focusable and activatable, using the existing handleWordClick behavior.
Add the appropriate button-like accessibility semantics and keyboard handling so
users can focus and select words without a mouse, while preserving the current
selection/current-state styling and behavior.
In `@src/components/ai-edition/VirtualPreview.tsx`:
- Around line 216-225: The scrubber input in VirtualPreview is missing an
accessible name, so add a label or aria-label to the range control used for
seekToVirtualTime. Update the input rendered in VirtualPreview.tsx to expose a
clear accessible name that describes the timeline/scrubber behavior, while
keeping the existing slider behavior, disabled state, and value handling
unchanged.
- Around line 37-42: Calling onVideoElement during VirtualPreview render can
cause React state अपडेट warnings and render loops; move this reporting logic out
of the render path in VirtualPreview and into an effect tied to the video
element lifecycle. Use the existing videoRef and onVideoElement callback, and
ensure the parent is notified when the element becomes available and cleared on
unmount or source change.
In `@src/lib/ai-edition/document/migrate.ts`:
- Around line 76-94: The migration logic in migrate() is dropping legacy
webcam-specific media, so preserve it when converting to v3 and back. Update the
forward path selection and asset creation to carry both screen and webcam video
information, and include cursorCaptureMode in the migrated media model; then
adjust the reverse migration to reconstruct webcam media as well as screen media
instead of only using screenVideoPath. Use the existing migrate() flow and
asset-building logic to locate the changes.
- Around line 281-284: The merge in migrate() is overwriting timeline-derived
editor state from document.timeline.skipRanges when legacyEditor is applied,
which can drop v3 skip range edits on return to v2. Update the legacy merge
logic so Object.assign(editor, legacy) happens first, then reapply the derived
timeline fields afterward (especially trimRegions) so current skip ranges always
win over legacyEditor.trimRegions.
In `@src/lib/ai-edition/document/transcribe.ts`:
- Around line 44-55: The transcript ID generation in transcribe.ts is not
namespaced, so segId and wordId restart at seg_1/word_1 for every asset and can
collide across documents. Update the ID creation logic in the transcription loop
to include the current asset namespace (for example the assetId or another
stable per-asset prefix) for both segment and word identifiers, and ensure any
downstream references like clip.wordRefs use the same namespaced IDs
consistently.
In `@src/lib/ai-edition/exporter/documentExporter.ts`:
- Around line 56-93: clipsToTrimRanges is building a synthetic AxcutDocument
with assets: [] and primaryAssetId: "", which makes timelineIntervals compute a
zero primary duration and drop all clip intervals. Fix this by giving the
synthetic document a real source duration via the asset metadata used by
timelineIntervals, or by computing the trim ranges directly from clips without
relying on primaryAssetDuration. Keep the logic in clipsToTrimRanges and the
timelineIntervals call aligned so the returned intervals reflect the actual
source length instead of defaulting to the full video trim.
In `@src/lib/ai-edition/schema/index.test.ts`:
- Around line 170-188: The `documentSchema rejects v2 doc without v3 envelopes`
test in `documentSchema.parse` is inconsistent with the actual assertion, since
it currently expects the input to be accepted via defaults. Either rename the
test to reflect the intended defaulting behavior if `documentSchema` should
backfill missing v3 envelopes, or change the assertion/schema behavior so
`documentSchema.parse` throws when those v3 envelope fields are absent. Make
sure the test name and the `documentSchema` contract match.
In `@src/lib/ai-edition/schema/index.ts`:
- Around line 19-34: Update the interval validation in wordSchema and
transcriptSegmentSchema so they reject inverted ranges, not just negative
values. Add an endSec >= startSec refinement to the interval-bearing schemas
used by the timeline/export flow, and apply the same ordered-range check
anywhere clip/timeline source ranges are validated while keeping sourceEndSec
optional when it is not present. Use the existing schema definitions in index.ts
as the entry point and preserve the current field shapes.
In `@src/native/browserShim.ts`:
- Around line 76-112: The create flow in browserShim’s create function returns a
new document but never updates currentDoc, so subsequent get, addAsset, and
removeAsset calls can read stale state. Update currentDoc to the newly created
doc inside create before saving the list and returning success, and ensure the
browser-mode document helpers continue to operate on that same currentDoc
reference.
---
Nitpick comments:
In `@src/components/ai-edition/TranscriptEditor.tsx`:
- Around line 78-81: The word selection logic in TranscriptEditor is doing an
O(n²) check because `isInRange` repeatedly scans `transcript.words` for every
rendered word. Refactor the range selection path in `TranscriptEditor` to
precompute the selected index window or selected id set once with `useMemo`
(using the existing `selectedRange`/anchor/focus state), then replace the
per-word `isInRange` call with an O(1) membership check inside the word render
loop.
In `@src/lib/ai-edition/exporter/documentExporter.ts`:
- Line 218: The type-only import for ExportResult is separated from the rest of
the imports, so move it into the main import block at the top of
documentExporter.ts. Keep the existing import grouping consistent by placing
import type { ExportResult } alongside the other import statements near the file
header, without changing any runtime behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 079c0959-64db-40fa-8441-709fa9fd7dd8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
docs/architecture/ai-edition-merge-plan.mdelectron/ai-edition/chat-service.tselectron/ai-edition/document-service.test.tselectron/ai-edition/document-service.tselectron/ai-edition/llm-config-store.tselectron/ai-edition/provider-registry.tselectron/ipc/handlers.tselectron/ipc/nativeBridge.tselectron/native-bridge/services/aiEditionService.tspackage.jsonsrc/App.tsxsrc/components/ai-edition/AiEditionShell.tsxsrc/components/ai-edition/ChatPanel.tsxsrc/components/ai-edition/EditorSettings.tsxsrc/components/ai-edition/IconRail.tsxsrc/components/ai-edition/NewEditorShell.tsxsrc/components/ai-edition/ProjectPanel.tsxsrc/components/ai-edition/TimelinePane.module.csssrc/components/ai-edition/TimelinePane.tsxsrc/components/ai-edition/TranscriptEditor.module.csssrc/components/ai-edition/TranscriptEditor.tsxsrc/components/ai-edition/VirtualPreview.module.csssrc/components/ai-edition/VirtualPreview.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/featureFlags.tssrc/lib/ai-edition/document/ids.tssrc/lib/ai-edition/document/migrate.test.tssrc/lib/ai-edition/document/migrate.tssrc/lib/ai-edition/document/timeline.test.tssrc/lib/ai-edition/document/timeline.tssrc/lib/ai-edition/document/transcribe.tssrc/lib/ai-edition/exporter/documentExporter.tssrc/lib/ai-edition/schema/index.test.tssrc/lib/ai-edition/schema/index.tssrc/lib/ai-edition/store/projectStore.test.tssrc/lib/ai-edition/store/projectStore.tssrc/lib/ai-edition/timeline/virtual-preview.test.tssrc/lib/ai-edition/timeline/virtual-preview.tssrc/native/browserShim.tssrc/native/client.tssrc/native/contracts.tsvite.config.ts
✅ Files skipped from review due to trivial changes (4)
- package.json
- src/lib/ai-edition/document/ids.ts
- src/lib/ai-edition/store/projectStore.test.ts
- src/components/ai-edition/VirtualPreview.module.css
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture/ai-edition-merge-plan.md
| } | ||
| } | ||
|
|
||
| const SUPPORTED_VIDEO_EXTENSIONS = new Set([".mp4", ".mov", ".m4v", ".webm", ".mkv", ".mp4"]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Duplicate .mp4 in the supported-extensions set.
.mp4 appears twice; the Set silently dedupes it, so one entry is dead and a likely-intended extension is missing. The file picker in handlers.ts also accepts mov, avi, mkv, m4v, wmv, flv, ts — confirm which extension this duplicate was meant to be so accepted assets stay consistent across the picker and this validator.
🐛 Proposed fix (drop the duplicate)
-const SUPPORTED_VIDEO_EXTENSIONS = new Set([".mp4", ".mov", ".m4v", ".webm", ".mkv", ".mp4"]);
+const SUPPORTED_VIDEO_EXTENSIONS = new Set([".mp4", ".mov", ".m4v", ".webm", ".mkv"]);📝 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.
Suggested change
| const SUPPORTED_VIDEO_EXTENSIONS = new Set([".mp4", ".mov", ".m4v", ".webm", ".mkv", ".mp4"]); | |
| const SUPPORTED_VIDEO_EXTENSIONS = new Set([".mp4", ".mov", ".m4v", ".webm", ".mkv"]); |
🤖 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/ai-edition/document-service.ts` at line 51, The
SUPPORTED_VIDEO_EXTENSIONS set in document-service.ts includes a duplicate .mp4
entry, so one intended video extension is likely missing. Update the set to
replace the duplicate with the correct missing extension and keep it aligned
with the accepted types in handlers.ts so both the picker and validator allow
the same video formats.
Comment on lines
+29
to
+31
| useEffect(() => { | ||
| scrollRef.current?.scrollTo({ top: scrollRef.current.scrollHeight, behavior: "smooth" }); | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Auto-scroll never fires after the first render.
This effect has an empty dependency array, so it only runs once on mount when the list is empty. It won't scroll when loadHistory resolves or when a new message is appended.
🐛 Depend on messages
- useEffect(() => {
- scrollRef.current?.scrollTo({ top: scrollRef.current.scrollHeight, behavior: "smooth" });
- }, []);
+ useEffect(() => {
+ scrollRef.current?.scrollTo({ top: scrollRef.current.scrollHeight, behavior: "smooth" });
+ }, [messages]);📝 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.
Suggested change
| useEffect(() => { | |
| scrollRef.current?.scrollTo({ top: scrollRef.current.scrollHeight, behavior: "smooth" }); | |
| }, []); | |
| useEffect(() => { | |
| scrollRef.current?.scrollTo({ top: scrollRef.current.scrollHeight, behavior: "smooth" }); | |
| }, [messages]); |
🤖 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/ai-edition/ChatPanel.tsx` around lines 29 - 31, The
auto-scroll effect in ChatPanel only runs on mount because the useEffect
dependency array is empty, so it won’t fire after loadHistory resolves or when
new messages are added. Update the effect in ChatPanel to depend on the message
list or whatever state/prop drives the rendered chat items, so the scrollTo call
reruns whenever the chat content changes.
Comment on lines
+33
to
+52
| const handleSend = useCallback(async () => { | ||
| if (!projectId || !input.trim() || busy) return; | ||
| const text = input.trim(); | ||
| setInput(""); | ||
| setBusy(true); | ||
| try { | ||
| const result = await nativeBridgeClient.aiEdition.chatRun(projectId, text); | ||
| if (result.success && result.assistantMessage) { | ||
| setMessages((prev) => [...prev, result.assistantMessage!]); | ||
| } else { | ||
| toast.error(result.error ?? "Chat failed"); | ||
| } | ||
| } catch (err) { | ||
| toast.error("Chat failed", { | ||
| description: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| } finally { | ||
| setBusy(false); | ||
| } | ||
| }, [projectId, input, busy]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sent user message is not shown in the thread.
On send, the input is cleared but the user's message is never appended to messages; only result.assistantMessage is added, and loadHistory isn't re-invoked. The user's own prompt vanishes from the UI until the panel reloads history. Append the user message optimistically (or re-load history after a successful run).
🐛 Optimistically append the user message
const text = input.trim();
setInput("");
setBusy(true);
+ const userMessage: AiEditionChatMessage = {
+ id: `local-${Date.now()}`,
+ role: "user",
+ content: text,
+ createdAt: new Date().toISOString(),
+ };
+ setMessages((prev) => [...prev, userMessage]);
try {
const result = await nativeBridgeClient.aiEdition.chatRun(projectId, text);📝 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.
Suggested change
| const handleSend = useCallback(async () => { | |
| if (!projectId || !input.trim() || busy) return; | |
| const text = input.trim(); | |
| setInput(""); | |
| setBusy(true); | |
| try { | |
| const result = await nativeBridgeClient.aiEdition.chatRun(projectId, text); | |
| if (result.success && result.assistantMessage) { | |
| setMessages((prev) => [...prev, result.assistantMessage!]); | |
| } else { | |
| toast.error(result.error ?? "Chat failed"); | |
| } | |
| } catch (err) { | |
| toast.error("Chat failed", { | |
| description: err instanceof Error ? err.message : String(err), | |
| }); | |
| } finally { | |
| setBusy(false); | |
| } | |
| }, [projectId, input, busy]); | |
| const handleSend = useCallback(async () => { | |
| if (!projectId || !input.trim() || busy) return; | |
| const text = input.trim(); | |
| setInput(""); | |
| setBusy(true); | |
| const userMessage: AiEditionChatMessage = { | |
| id: `local-${Date.now()}`, | |
| role: "user", | |
| content: text, | |
| createdAt: new Date().toISOString(), | |
| }; | |
| setMessages((prev) => [...prev, userMessage]); | |
| try { | |
| const result = await nativeBridgeClient.aiEdition.chatRun(projectId, text); | |
| if (result.success && result.assistantMessage) { | |
| setMessages((prev) => [...prev, result.assistantMessage!]); | |
| } else { | |
| toast.error(result.error ?? "Chat failed"); | |
| } | |
| } catch (err) { | |
| toast.error("Chat failed", { | |
| description: err instanceof Error ? err.message : String(err), | |
| }); | |
| } finally { | |
| setBusy(false); | |
| } | |
| }, [projectId, input, busy]); |
🤖 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/ai-edition/ChatPanel.tsx` around lines 33 - 52, The
ChatPanel.handleSend flow clears the input and only appends
result.assistantMessage, so the user’s sent prompt never appears in messages.
Update the send path to either optimistically append the trimmed user text to
the messages state before calling nativeBridgeClient.aiEdition.chatRun, or
trigger loadHistory after a successful send so the thread reflects both sides.
Keep the fix localized to handleSend and the messages state update logic in
ChatPanel.
Comment on lines
+298
to
+315
| const handleSpeedChange = useCallback( | ||
| (speed: PlaybackSpeed) => { | ||
| if (!document || !selectedSpeedId) return; | ||
| const next = sr.map((r) => (r.id === selectedSpeedId ? { ...r, speed } : r)); | ||
| updateLegacy({ sr: next }); | ||
| }, | ||
| [document, selectedSpeedId, sr, updateLegacy], | ||
| ); | ||
|
|
||
| const handleSpeedDelete = useCallback( | ||
| (id: string) => { | ||
| if (!document) return; | ||
| updateLegacy({ sr: sr.filter((r) => r.id !== id) }); | ||
| setSelectedSpeedId(null); | ||
| void commitToDisk(); | ||
| }, | ||
| [document, sr, updateLegacy, commitToDisk], | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Speed edits are persisted under the wrong key and silently lost.
sr is read from the legacy blob under key "speedRegions" (Line 139), but handleSpeedChange/handleSpeedDelete write back via updateLegacy({ sr: ... }), persisting to key "sr" instead. On the next read the change isn't visible, and documentExporter.ts also reads "speedRegions" (Line 143), so speed regions never reach export.
🐛 Proposed fix: write to the same key it reads from
const handleSpeedChange = useCallback(
(speed: PlaybackSpeed) => {
if (!document || !selectedSpeedId) return;
const next = sr.map((r) => (r.id === selectedSpeedId ? { ...r, speed } : r));
- updateLegacy({ sr: next });
+ updateLegacy({ speedRegions: next });
},
[document, selectedSpeedId, sr, updateLegacy],
);
const handleSpeedDelete = useCallback(
(id: string) => {
if (!document) return;
- updateLegacy({ sr: sr.filter((r) => r.id !== id) });
+ updateLegacy({ speedRegions: sr.filter((r) => r.id !== id) });
setSelectedSpeedId(null);
void commitToDisk();
},
[document, sr, updateLegacy, commitToDisk],
);📝 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.
Suggested change
| const handleSpeedChange = useCallback( | |
| (speed: PlaybackSpeed) => { | |
| if (!document || !selectedSpeedId) return; | |
| const next = sr.map((r) => (r.id === selectedSpeedId ? { ...r, speed } : r)); | |
| updateLegacy({ sr: next }); | |
| }, | |
| [document, selectedSpeedId, sr, updateLegacy], | |
| ); | |
| const handleSpeedDelete = useCallback( | |
| (id: string) => { | |
| if (!document) return; | |
| updateLegacy({ sr: sr.filter((r) => r.id !== id) }); | |
| setSelectedSpeedId(null); | |
| void commitToDisk(); | |
| }, | |
| [document, sr, updateLegacy, commitToDisk], | |
| ); | |
| const handleSpeedChange = useCallback( | |
| (speed: PlaybackSpeed) => { | |
| if (!document || !selectedSpeedId) return; | |
| const next = sr.map((r) => (r.id === selectedSpeedId ? { ...r, speed } : r)); | |
| updateLegacy({ speedRegions: next }); | |
| }, | |
| [document, selectedSpeedId, sr, updateLegacy], | |
| ); | |
| const handleSpeedDelete = useCallback( | |
| (id: string) => { | |
| if (!document) return; | |
| updateLegacy({ speedRegions: sr.filter((r) => r.id !== id) }); | |
| setSelectedSpeedId(null); | |
| void commitToDisk(); | |
| }, | |
| [document, sr, updateLegacy, commitToDisk], | |
| ); |
🤖 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/ai-edition/EditorSettings.tsx` around lines 298 - 315, The
speed edit handlers are writing to the wrong legacy field, so changes are saved
under a key that neither the reader nor the exporter uses. In
EditorSettings.tsx, update handleSpeedChange and handleSpeedDelete to persist
the same property that is read as sr from the legacy blob (the speedRegions
data), and keep commitToDisk behavior unchanged after the delete path. Make sure
the state update and any legacy write in these callbacks use the same
symbol/shape as the speedRegions reader so updates survive reload and flow into
documentExporter.ts.
Comment on lines
+497
to
+500
| globalThis.document.body.classList.add("timeline-resizing-cut"); | ||
| globalThis.window.addEventListener("pointermove", move); | ||
| globalThis.window.addEventListener("pointerup", end, { once: true }); | ||
| globalThis.window.addEventListener("pointercancel", end, { once: true }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up drag listeners on unmount.
These global listeners/body classes are only removed by the pointer-end callbacks. If the shell unmounts while dragging, listeners remain attached and can keep dispatching into stale component state. Track the active cleanup in a ref and run it from a useEffect unmount cleanup.
Also applies to: 522-524, 552-554, 640-642
🤖 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/ai-edition/TimelinePane.tsx` around lines 497 - 500, The drag
logic in TimelinePane still relies only on pointer-end callbacks to remove
global listeners and the body class, so unmounting mid-drag can leave stale
handlers attached. Update the resize/drag setup around the move/end handlers to
store the active cleanup in a ref, clear the window listeners and remove the
"timeline-resizing-cut" class from that cleanup, and invoke it from a useEffect
unmount cleanup so any in-progress drag is safely torn down.
Comment on lines
+44
to
+55
| for (let segIndex = 0; segIndex < result.segments.length; segIndex++) { | ||
| const seg = result.segments[segIndex]; | ||
| const segId = `seg_${segIndex + 1}`; | ||
| const wordIds: string[] = []; | ||
|
|
||
| const tokens = seg.text.trim().split(/\s+/).filter(Boolean); | ||
| if (tokens.length === 0) continue; | ||
|
|
||
| const wordDuration = (seg.endSec - seg.startSec) / tokens.length; | ||
| for (let w = 0; w < tokens.length; w++) { | ||
| const wordId = `word_${words.length + 1}`; | ||
| const startSec = seg.startSec + w * wordDuration; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope generated transcript IDs by asset.
Every transcription restarts at seg_1 / word_1, but document-level operations and clip.wordRefs only carry bare word IDs. A second asset transcript can make word references ambiguous; include the assetId or another stable namespace in generated IDs.
Suggested ID namespacing
+ const idNamespace = assetId.replace(/[^A-Za-z0-9_-]/g, "_");
+
for (let segIndex = 0; segIndex < result.segments.length; segIndex++) {
const seg = result.segments[segIndex];
- const segId = `seg_${segIndex + 1}`;
+ const segId = `seg_${idNamespace}_${segIndex + 1}`;
const wordIds: string[] = [];
@@
const wordDuration = (seg.endSec - seg.startSec) / tokens.length;
for (let w = 0; w < tokens.length; w++) {
- const wordId = `word_${words.length + 1}`;
+ const wordId = `word_${idNamespace}_${words.length + 1}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (let segIndex = 0; segIndex < result.segments.length; segIndex++) { | |
| const seg = result.segments[segIndex]; | |
| const segId = `seg_${segIndex + 1}`; | |
| const wordIds: string[] = []; | |
| const tokens = seg.text.trim().split(/\s+/).filter(Boolean); | |
| if (tokens.length === 0) continue; | |
| const wordDuration = (seg.endSec - seg.startSec) / tokens.length; | |
| for (let w = 0; w < tokens.length; w++) { | |
| const wordId = `word_${words.length + 1}`; | |
| const startSec = seg.startSec + w * wordDuration; | |
| const idNamespace = assetId.replace(/[^A-Za-z0-9_-]/g, "_"); | |
| for (let segIndex = 0; segIndex < result.segments.length; segIndex++) { | |
| const seg = result.segments[segIndex]; | |
| const segId = `seg_${idNamespace}_${segIndex + 1}`; | |
| const wordIds: string[] = []; | |
| const tokens = seg.text.trim().split(/\s+/).filter(Boolean); | |
| if (tokens.length === 0) continue; | |
| const wordDuration = (seg.endSec - seg.startSec) / tokens.length; | |
| for (let w = 0; w < tokens.length; w++) { | |
| const wordId = `word_${idNamespace}_${words.length + 1}`; | |
| const startSec = seg.startSec + w * wordDuration; |
🤖 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/ai-edition/document/transcribe.ts` around lines 44 - 55, The
transcript ID generation in transcribe.ts is not namespaced, so segId and wordId
restart at seg_1/word_1 for every asset and can collide across documents. Update
the ID creation logic in the transcription loop to include the current asset
namespace (for example the assetId or another stable per-asset prefix) for both
segment and word identifiers, and ensure any downstream references like
clip.wordRefs use the same namespaced IDs consistently.
Comment on lines
+56
to
+93
| function clipsToTrimRanges( | ||
| sourceDurationSec: number, | ||
| clips: AxcutDocument["timeline"]["clips"], | ||
| ): TrimRegion[] { | ||
| const intervals = timelineIntervals({ | ||
| ...({} as AxcutDocument), | ||
| timeline: { | ||
| clips, | ||
| gaps: [], | ||
| skipRanges: [], | ||
| muteRanges: [], | ||
| speedRanges: [], | ||
| captionRanges: [], | ||
| }, | ||
| assets: [], | ||
| project: { primaryAssetId: "" }, | ||
| } as unknown as AxcutDocument); | ||
| const trims: TrimRegion[] = []; | ||
| let cursor = 0; | ||
| for (const interval of intervals) { | ||
| if (interval.startSec > cursor) { | ||
| trims.push({ | ||
| id: `trim_${trims.length + 1}`, | ||
| startMs: Math.round(cursor * 1000), | ||
| endMs: Math.round(interval.startSec * 1000), | ||
| }); | ||
| } | ||
| cursor = Math.max(cursor, interval.endSec); | ||
| } | ||
| if (cursor < sourceDurationSec) { | ||
| trims.push({ | ||
| id: `trim_${trims.length + 1}`, | ||
| startMs: Math.round(cursor * 1000), | ||
| endMs: Math.round(sourceDurationSec * 1000), | ||
| }); | ||
| } | ||
| return trims; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
clipsToTrimRanges discards all clips and trims the entire video.
The synthetic document passes assets: [] and primaryAssetId: "", so timelineIntervals → primaryAssetDuration returns 0. normalizeIntervals(0, …) then clamps every clip interval to {startSec:0, endSec:0} and filters them out, yielding intervals = []. The loop produces nothing and the trailing guard pushes a single trim spanning [0, sourceDurationSec] — i.e. the export trims away the whole source. Pass the real duration through the synthetic asset (or compute intervals directly from clips).
🐛 Proposed fix: give the synthetic doc a real duration
const intervals = timelineIntervals({
...({} as AxcutDocument),
timeline: {
clips,
gaps: [],
skipRanges: [],
muteRanges: [],
speedRanges: [],
captionRanges: [],
},
- assets: [],
- project: { primaryAssetId: "" },
+ assets: [{ id: "src", durationSec: sourceDurationSec } as AxcutDocument["assets"][number]],
+ project: { primaryAssetId: "src" },
} as unknown as AxcutDocument);📝 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.
Suggested change
| function clipsToTrimRanges( | |
| sourceDurationSec: number, | |
| clips: AxcutDocument["timeline"]["clips"], | |
| ): TrimRegion[] { | |
| const intervals = timelineIntervals({ | |
| ...({} as AxcutDocument), | |
| timeline: { | |
| clips, | |
| gaps: [], | |
| skipRanges: [], | |
| muteRanges: [], | |
| speedRanges: [], | |
| captionRanges: [], | |
| }, | |
| assets: [], | |
| project: { primaryAssetId: "" }, | |
| } as unknown as AxcutDocument); | |
| const trims: TrimRegion[] = []; | |
| let cursor = 0; | |
| for (const interval of intervals) { | |
| if (interval.startSec > cursor) { | |
| trims.push({ | |
| id: `trim_${trims.length + 1}`, | |
| startMs: Math.round(cursor * 1000), | |
| endMs: Math.round(interval.startSec * 1000), | |
| }); | |
| } | |
| cursor = Math.max(cursor, interval.endSec); | |
| } | |
| if (cursor < sourceDurationSec) { | |
| trims.push({ | |
| id: `trim_${trims.length + 1}`, | |
| startMs: Math.round(cursor * 1000), | |
| endMs: Math.round(sourceDurationSec * 1000), | |
| }); | |
| } | |
| return trims; | |
| } | |
| function clipsToTrimRanges( | |
| sourceDurationSec: number, | |
| clips: AxcutDocument["timeline"]["clips"], | |
| ): TrimRegion[] { | |
| const intervals = timelineIntervals({ | |
| ...({} as AxcutDocument), | |
| timeline: { | |
| clips, | |
| gaps: [], | |
| skipRanges: [], | |
| muteRanges: [], | |
| speedRanges: [], | |
| captionRanges: [], | |
| }, | |
| assets: [{ id: "src", durationSec: sourceDurationSec } as AxcutDocument["assets"][number]], | |
| project: { primaryAssetId: "src" }, | |
| } as unknown as AxcutDocument); | |
| const trims: TrimRegion[] = []; | |
| let cursor = 0; | |
| for (const interval of intervals) { | |
| if (interval.startSec > cursor) { | |
| trims.push({ | |
| id: `trim_${trims.length + 1}`, | |
| startMs: Math.round(cursor * 1000), | |
| endMs: Math.round(interval.startSec * 1000), | |
| }); | |
| } | |
| cursor = Math.max(cursor, interval.endSec); | |
| } | |
| if (cursor < sourceDurationSec) { | |
| trims.push({ | |
| id: `trim_${trims.length + 1}`, | |
| startMs: Math.round(cursor * 1000), | |
| endMs: Math.round(sourceDurationSec * 1000), | |
| }); | |
| } | |
| return trims; | |
| } |
🤖 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/ai-edition/exporter/documentExporter.ts` around lines 56 - 93,
clipsToTrimRanges is building a synthetic AxcutDocument with assets: [] and
primaryAssetId: "", which makes timelineIntervals compute a zero primary
duration and drop all clip intervals. Fix this by giving the synthetic document
a real source duration via the asset metadata used by timelineIntervals, or by
computing the trim ranges directly from clips without relying on
primaryAssetDuration. Keep the logic in clipsToTrimRanges and the
timelineIntervals call aligned so the returned intervals reflect the actual
source length instead of defaulting to the full video trim.
Comment on lines
+170
to
+188
| it("documentSchema rejects v2 doc without v3 envelopes", () => { | ||
| expect(() => | ||
| documentSchema.parse({ | ||
| schemaVersion: 3, | ||
| project: { | ||
| id: "p", | ||
| title: "t", | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| }, | ||
| assets: [], | ||
| transcript: null, | ||
| timeline: {}, | ||
| agent: {}, | ||
| preview: {}, | ||
| export: {}, | ||
| history: {}, | ||
| }), | ||
| ).not.toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align this test with the intended schema contract.
The test name says v2-shaped docs should be rejected, but the assertion verifies they are accepted via defaults. Rename the test if defaulting is intended, or change the assertion/schema if v3 envelopes must be required.
Suggested rename if defaulting is intended
- it("documentSchema rejects v2 doc without v3 envelopes", () => {
+ it("documentSchema defaults missing v3 envelopes", () => {📝 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.
Suggested change
| it("documentSchema rejects v2 doc without v3 envelopes", () => { | |
| expect(() => | |
| documentSchema.parse({ | |
| schemaVersion: 3, | |
| project: { | |
| id: "p", | |
| title: "t", | |
| createdAt: new Date().toISOString(), | |
| updatedAt: new Date().toISOString(), | |
| }, | |
| assets: [], | |
| transcript: null, | |
| timeline: {}, | |
| agent: {}, | |
| preview: {}, | |
| export: {}, | |
| history: {}, | |
| }), | |
| ).not.toThrow(); | |
| it("documentSchema defaults missing v3 envelopes", () => { | |
| expect(() => | |
| documentSchema.parse({ | |
| schemaVersion: 3, | |
| project: { | |
| id: "p", | |
| title: "t", | |
| createdAt: new Date().toISOString(), | |
| updatedAt: new Date().toISOString(), | |
| }, | |
| assets: [], | |
| transcript: null, | |
| timeline: {}, | |
| agent: {}, | |
| preview: {}, | |
| export: {}, | |
| history: {}, | |
| }), | |
| ).not.toThrow(); |
🤖 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/ai-edition/schema/index.test.ts` around lines 170 - 188, The
`documentSchema rejects v2 doc without v3 envelopes` test in
`documentSchema.parse` is inconsistent with the actual assertion, since it
currently expects the input to be accepted via defaults. Either rename the test
to reflect the intended defaulting behavior if `documentSchema` should backfill
missing v3 envelopes, or change the assertion/schema behavior so
`documentSchema.parse` throws when those v3 envelope fields are absent. Make
sure the test name and the `documentSchema` contract match.
Comment on lines
+19
to
+34
| export const wordSchema = z.object({ | ||
| id: z.string().min(1), | ||
| segmentId: z.string().min(1), | ||
| startSec: z.number().nonnegative(), | ||
| endSec: z.number().nonnegative(), | ||
| text: z.string(), | ||
| }); | ||
|
|
||
| export const transcriptSegmentSchema = z.object({ | ||
| id: z.string().min(1), | ||
| kind: z.enum(["speech", "silence"]), | ||
| startSec: z.number().nonnegative(), | ||
| endSec: z.number().nonnegative(), | ||
| text: z.string(), | ||
| wordIds: z.array(z.string().min(1)).default([]), | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline ==\n'
ast-grep outline src/lib/ai-edition/schema/index.ts --view expanded || true
printf '\n== file excerpt ==\n'
nl -ba src/lib/ai-edition/schema/index.ts | sed -n '1,260p'
printf '\n== search interval fields ==\n'
rg -n --no-heading -S \
'timelineStartSec|timelineEndSec|sourceStartSec|sourceEndSec|startSec:|endSec:|wordIds|clipSchema|transcriptSegmentSchema|wordSchema|skipRangeSchema|operationSchema' \
src/lib/ai-edition || trueRepository: getopenscreen/openscreen
Length of output: 3786
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== schema excerpt ==\n'
awk 'NR>=1 && NR<=260 { printf "%4d: %s\n", NR, $0 }' src/lib/ai-edition/schema/index.ts
printf '\n== refinements in schema file ==\n'
rg -n --no-heading '\.refine\(|\.superRefine\(' src/lib/ai-edition/schema/index.ts || true
printf '\n== interval field consumers ==\n'
rg -n --no-heading -S 'timelineStartSec|timelineEndSec|sourceStartSec|sourceEndSec|startSec|endSec' src/lib/ai-edition src/lib || trueRepository: getopenscreen/openscreen
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== timeline core ==\n'
awk 'NR>=1 && NR<=190 { printf "%4d: %s\n", NR, $0 }' src/lib/ai-edition/document/timeline.ts
printf '\n== exporter interval use ==\n'
awk 'NR>=60 && NR<=95 { printf "%4d: %s\n", NR, $0 }' src/lib/ai-edition/exporter/documentExporter.ts
printf '\n== schema tests ==\n'
awk 'NR>=1 && NR<=140 { printf "%4d: %s\n", NR, $0 }' src/lib/ai-edition/schema/index.test.tsRepository: getopenscreen/openscreen
Length of output: 11736
Reject inverted time ranges in the schema.
startSec/endSec and the clip/timeline source ranges only enforce nonnegative values, so malformed docs/ops with end < start still pass validation. The timeline/export path assumes ordered intervals, so add end >= start refinements for the interval-bearing schemas, keeping sourceEndSec optional when absent.
🤖 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/ai-edition/schema/index.ts` around lines 19 - 34, Update the interval
validation in wordSchema and transcriptSegmentSchema so they reject inverted
ranges, not just negative values. Add an endSec >= startSec refinement to the
interval-bearing schemas used by the timeline/export flow, and apply the same
ordered-range check anywhere clip/timeline source ranges are validated while
keeping sourceEndSec optional when it is not present. Use the existing schema
definitions in index.ts as the entry point and preserve the current field
shapes.
Comment on lines
+76
to
+112
| create: (title?: string) => { | ||
| const doc = { | ||
| schemaVersion: 3, | ||
| project: { | ||
| id: `proj_${Math.random().toString(36).slice(2, 10)}`, | ||
| title: title || "Untitled Project", | ||
| createdAt: new Date().toISOString(), | ||
| updatedAt: new Date().toISOString(), | ||
| }, | ||
| assets: [], | ||
| transcript: null, | ||
| transcripts: [], | ||
| timeline: { | ||
| clips: [], | ||
| gaps: [], | ||
| skipRanges: [], | ||
| muteRanges: [], | ||
| speedRanges: [], | ||
| captionRanges: [], | ||
| }, | ||
| annotations: [], | ||
| zoomRanges: [], | ||
| legacyEditor: null, | ||
| agent: { pendingQuestions: [], suggestions: [], lastAppliedOperations: [] }, | ||
| preview: { strategy: "seek", revision: 0 }, | ||
| export: { preset: "final-balanced", lastJobId: null }, | ||
| history: { revisions: [] }, | ||
| }; | ||
| list.unshift({ | ||
| id: doc.project.id, | ||
| title: doc.project.title, | ||
| updatedAt: doc.project.updatedAt, | ||
| assetCount: 0, | ||
| }); | ||
| saveList(); | ||
| return Promise.resolve({ success: true, document: doc }); | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
create never sets currentDoc, so get/addAsset/removeAsset return a stale document.
create builds the new doc and returns it to the caller, but doesn't update currentDoc. After creating a project, get() returns the previously loaded (initially null) document, and addAsset/removeAsset (lines 119-121) return that stale value too. Browser-mode create→edit flows will operate on the wrong document.
🐛 Proposed fix
saveList();
+ currentDoc = doc;
+ saveToStorage(doc);
return Promise.resolve({ success: true, document: doc });📝 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.
Suggested change
| create: (title?: string) => { | |
| const doc = { | |
| schemaVersion: 3, | |
| project: { | |
| id: `proj_${Math.random().toString(36).slice(2, 10)}`, | |
| title: title || "Untitled Project", | |
| createdAt: new Date().toISOString(), | |
| updatedAt: new Date().toISOString(), | |
| }, | |
| assets: [], | |
| transcript: null, | |
| transcripts: [], | |
| timeline: { | |
| clips: [], | |
| gaps: [], | |
| skipRanges: [], | |
| muteRanges: [], | |
| speedRanges: [], | |
| captionRanges: [], | |
| }, | |
| annotations: [], | |
| zoomRanges: [], | |
| legacyEditor: null, | |
| agent: { pendingQuestions: [], suggestions: [], lastAppliedOperations: [] }, | |
| preview: { strategy: "seek", revision: 0 }, | |
| export: { preset: "final-balanced", lastJobId: null }, | |
| history: { revisions: [] }, | |
| }; | |
| list.unshift({ | |
| id: doc.project.id, | |
| title: doc.project.title, | |
| updatedAt: doc.project.updatedAt, | |
| assetCount: 0, | |
| }); | |
| saveList(); | |
| return Promise.resolve({ success: true, document: doc }); | |
| }, | |
| create: (title?: string) => { | |
| const doc = { | |
| schemaVersion: 3, | |
| project: { | |
| id: `proj_${Math.random().toString(36).slice(2, 10)}`, | |
| title: title || "Untitled Project", | |
| createdAt: new Date().toISOString(), | |
| updatedAt: new Date().toISOString(), | |
| }, | |
| assets: [], | |
| transcript: null, | |
| transcripts: [], | |
| timeline: { | |
| clips: [], | |
| gaps: [], | |
| skipRanges: [], | |
| muteRanges: [], | |
| speedRanges: [], | |
| captionRanges: [], | |
| }, | |
| annotations: [], | |
| zoomRanges: [], | |
| legacyEditor: null, | |
| agent: { pendingQuestions: [], suggestions: [], lastAppliedOperations: [] }, | |
| preview: { strategy: "seek", revision: 0 }, | |
| export: { preset: "final-balanced", lastJobId: null }, | |
| history: { revisions: [] }, | |
| }; | |
| list.unshift({ | |
| id: doc.project.id, | |
| title: doc.project.title, | |
| updatedAt: doc.project.updatedAt, | |
| assetCount: 0, | |
| }); | |
| saveList(); | |
| currentDoc = doc; | |
| saveToStorage(doc); | |
| return Promise.resolve({ success: true, document: doc }); | |
| }, |
🤖 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/native/browserShim.ts` around lines 76 - 112, The create flow in
browserShim’s create function returns a new document but never updates
currentDoc, so subsequent get, addAsset, and removeAsset calls can read stale
state. Update currentDoc to the newly created doc inside create before saving
the list and returning success, and ensure the browser-mode document helpers
continue to operate on that same currentDoc reference.
…tures) Implements the OpenScreen x Axcut merge end-to-end. The new editing model (multi-asset / clips / skip-ranges / transcript / virtual-time preview) is the new default editor for all users. AI features (LLM provider config + chat scaffold) are opt-in behind AI_FEATURES_ENABLED. Phase 0 (Foundation): - v3 AxcutDocument schema with annotations, zoomRanges, legacyEditor envelope - Bidirectional migration v2 EditorProjectData <-> v3 AxcutDocument - AI_FEATURES_ENABLED flag (renamed from AI_EDITION_ENABLED; now gates only LLM/agent UI per the two-layer framing) Phase 1 (Core merge - PR 1.1, 1.2, 1.3): - Main-process DocumentService for v3 projects (list, get, create, save, addAsset, removeAsset) - IPC bridge for aiEdition domain (document.*, llm.*, chat.*) - AiEditionService adapter for the native bridge - Renderer Zustand store (projectStore) with timeline ops (replaceTimeline, restoreFullTimeline, setTranscript) - ProjectPanel (left rail content) - TimelinePane ported from axcut (kept/cut segments, ruler, playhead, zoom/pan) - VirtualPreview ported from axcut (virtual-time seeking, cross-clip playback) - TranscriptEditor (click/shift-click word range -> Cut) - ChatPanel (AI chat with in-memory history) - EditorSettings bridge wrapping the original OpenScreen SettingsPanel - NewEditorShell with axcut-inspired layout: left rail (Project/Chat) + center (video + timeline) + right rail (Transcript/Background/Video effects/Camera/Cursor/Crop/Export) - IconRail component with collapse/expand Phase 3 (Exporter - document-driven): - documentExporter adapter feeding AxcutDocument into the existing VideoExporter / GifExporter (annotations, zoom, trim, speed, cursor, webcam, wallpaper all wired through) Phase 4 (Transcription - local Whisper): - transcribeAsset reusing the existing transformers.js pipeline - TranscriptEditor with click-to-cut words Phase 6-8 (AI features - scaffolded): - Provider registry (8 providers: anthropic, openai, google, mistral, openrouter, openai-compatible, openai-oauth, copilot-proxy) - LlmConfigStore with safeStorage (OS keychain) per locked decision 4 - ChatService (in-memory history; LLM call is a stub needing @langchain/* deps) - IPC contracts for llm.getSnapshot, llm.setConfig, llm.setApiKey, llm.removeApiKey, chat.run, chat.history Phase 9 (Polish - partial): - i18n: kept existing OpenScreen strings; no new translation files (premature for the current scaffold) - Settings: AI_FEATURES_ENABLED toggle + feature flag wiring Browser mode (developer convenience): - browserShim that stubs window.electronAPI + nativeBridgeClient for rapid iteration in a plain browser at http://localhost:5173/?windowType=editor - localStorage-backed project/document persistence Spec updates (docs/architecture/ai-edition-merge-plan.md): - New section 0: Framing - two layers (new editing model = default, AI features = opt-in) - Updated section 5: locked decisions (renamed flag, framing change) - Updated section 10: cut-over (no editor cut-over; only AI features opt-in) - Renamed throughout Tests: 313/313 passing (39 test files). tsc clean. lint clean.
EtienneLescot
force-pushed
the
docs/ai-edition-plan
branch
from
41e0d6d to
cf25858
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ai-edition/document-service.test.ts`:
- Around line 122-160: The removeAsset cascade test only verifies assets, clips,
and skipRanges, but it misses the primary asset cleanup path. In the
document-service.test.ts case around service.removeAsset, add an assertion on
after.project.primaryAssetId to ensure it is cleared when the last/primary asset
is deleted, using the existing createProject/addAsset/saveProject/removeAsset
flow to locate the test.
In `@electron/ai-edition/document-service.ts`:
- Around line 193-209: The project update in the document removal flow is
preserving a deleted primary asset because `...doc.project` carries the old
value forward and the conditional spread in the `next` object only updates
`primaryAssetId` when truthy. In the `document-service.ts` asset removal path,
update the `next.project` construction so `primaryAssetId` is always explicitly
set from the computed `primaryAssetId` value, even when it is `undefined`, while
keeping the existing `updatedAt` and other fields unchanged.
In `@electron/ai-edition/llm-config-store.ts`:
- Around line 34-42: `LlmConfigStore` currently only populates persisted
`config` and credentials inside `load()`, so `getConfig()` and `getApiKey()` can
return empty state when called on a fresh instance. Update `LlmConfigStore` so
it either lazily loads on first read or otherwise guarantees `load()` has run
before `getConfig()`/`getApiKey()` are used, and make sure the relevant call
sites in `runAiEditionChat` and `getAiEditionLlmConfig` rely on that behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb592b1d-105d-4d19-a58b-90a2f981efbe
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
docs/architecture/ai-edition-merge-plan.mdelectron/ai-edition/chat-service.tselectron/ai-edition/document-service.test.tselectron/ai-edition/document-service.tselectron/ai-edition/llm-config-store.tselectron/ai-edition/provider-registry.tselectron/ipc/handlers.tselectron/ipc/nativeBridge.tselectron/native-bridge/services/aiEditionService.tspackage.jsonsrc/App.tsxsrc/components/ai-edition/AiEditionShell.tsxsrc/components/ai-edition/ChatPanel.tsxsrc/components/ai-edition/EditorSettings.tsxsrc/components/ai-edition/IconRail.tsxsrc/components/ai-edition/NewEditorShell.tsxsrc/components/ai-edition/ProjectPanel.tsxsrc/components/ai-edition/TimelinePane.module.csssrc/components/ai-edition/TimelinePane.tsxsrc/components/ai-edition/TranscriptEditor.module.csssrc/components/ai-edition/TranscriptEditor.tsxsrc/components/ai-edition/VirtualPreview.module.csssrc/components/ai-edition/VirtualPreview.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/featureFlags.tssrc/lib/ai-edition/document/ids.tssrc/lib/ai-edition/document/migrate.test.tssrc/lib/ai-edition/document/migrate.tssrc/lib/ai-edition/document/timeline.test.tssrc/lib/ai-edition/document/timeline.tssrc/lib/ai-edition/document/transcribe.tssrc/lib/ai-edition/exporter/documentExporter.tssrc/lib/ai-edition/schema/index.test.tssrc/lib/ai-edition/schema/index.tssrc/lib/ai-edition/store/projectStore.test.tssrc/lib/ai-edition/store/projectStore.tssrc/lib/ai-edition/timeline/virtual-preview.test.tssrc/lib/ai-edition/timeline/virtual-preview.tssrc/native/browserShim.tssrc/native/client.tssrc/native/contracts.tsvite.config.ts
✅ Files skipped from review due to trivial changes (2)
- src/components/ai-edition/VirtualPreview.module.css
- src/components/ai-edition/TimelinePane.module.css
🚧 Files skipped from review as they are similar to previous changes (34)
- src/components/ai-edition/AiEditionShell.tsx
- src/lib/ai-edition/document/ids.ts
- src/lib/ai-edition/store/projectStore.test.ts
- vite.config.ts
- electron/ai-edition/chat-service.ts
- src/lib/ai-edition/timeline/virtual-preview.test.ts
- package.json
- src/components/video-editor/featureFlags.ts
- electron/ai-edition/provider-registry.ts
- src/components/video-editor/SettingsPanel.tsx
- src/lib/ai-edition/timeline/virtual-preview.ts
- src/lib/ai-edition/document/migrate.test.ts
- src/components/ai-edition/TranscriptEditor.module.css
- src/lib/ai-edition/store/projectStore.ts
- src/lib/ai-edition/document/timeline.test.ts
- src/components/ai-edition/ProjectPanel.tsx
- src/components/ai-edition/ChatPanel.tsx
- src/native/client.ts
- src/App.tsx
- src/lib/ai-edition/schema/index.test.ts
- src/native/browserShim.ts
- src/native/contracts.ts
- src/components/ai-edition/IconRail.tsx
- src/lib/ai-edition/exporter/documentExporter.ts
- src/lib/ai-edition/document/timeline.ts
- src/components/ai-edition/TranscriptEditor.tsx
- src/lib/ai-edition/schema/index.ts
- electron/native-bridge/services/aiEditionService.ts
- src/lib/ai-edition/document/migrate.ts
- electron/ipc/nativeBridge.ts
- src/components/ai-edition/NewEditorShell.tsx
- src/components/ai-edition/TimelinePane.tsx
- src/components/ai-edition/VirtualPreview.tsx
- src/components/ai-edition/EditorSettings.tsx
Comment on lines
+122
to
+160
| it("removes the asset and cascades clips + skipRanges", async () => { | ||
| const doc = await service.createProject("P"); | ||
| const withAsset = await service.addAsset(doc.project.id, { path: "/tmp/a.mp4" }); | ||
| const assetId = withAsset.assets[0]?.id; | ||
| expect(assetId).toBeTruthy(); | ||
|
|
||
| // Manually add a clip + skipRange so we can verify cascade | ||
| const docWithTimeline = await service.saveProject({ | ||
| ...withAsset, | ||
| timeline: { | ||
| ...withAsset.timeline, | ||
| clips: [ | ||
| { | ||
| id: "clip_1", | ||
| assetId, | ||
| sourceStartSec: 0, | ||
| timelineStartSec: 0, | ||
| timelineEndSec: 1, | ||
| wordRefs: [], | ||
| origin: "system", | ||
| }, | ||
| ], | ||
| skipRanges: [ | ||
| { | ||
| id: "skip_1", | ||
| assetId, | ||
| startSec: 0, | ||
| endSec: 1, | ||
| origin: "user", | ||
| }, | ||
| ], | ||
| }, | ||
| }); | ||
|
|
||
| const after = await service.removeAsset(docWithTimeline.project.id, assetId ?? ""); | ||
| expect(after.assets).toHaveLength(0); | ||
| expect(after.timeline.clips).toHaveLength(0); | ||
| expect(after.timeline.skipRanges).toHaveLength(0); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Coverage gap: removing the primary/last asset doesn't assert primaryAssetId is cleared.
This cascade test removes the only asset but never checks after.project.primaryAssetId. As written it would pass even though the source leaves a stale primaryAssetId pointing at the deleted asset (see comment on document-service.ts removeAsset). Add an assertion to lock the intended behavior.
expect(after.project.primaryAssetId).toBeUndefined();🤖 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/ai-edition/document-service.test.ts` around lines 122 - 160, The
removeAsset cascade test only verifies assets, clips, and skipRanges, but it
misses the primary asset cleanup path. In the document-service.test.ts case
around service.removeAsset, add an assertion on after.project.primaryAssetId to
ensure it is cleared when the last/primary asset is deleted, using the existing
createProject/addAsset/saveProject/removeAsset flow to locate the test.
Comment on lines
+193
to
+209
| const primaryAssetId = | ||
| doc.project.primaryAssetId === assetId | ||
| ? (assets[0]?.id ?? undefined) | ||
| : doc.project.primaryAssetId; | ||
| const next: AxcutDocument = { | ||
| ...doc, | ||
| assets, | ||
| timeline: { | ||
| ...doc.timeline, | ||
| clips: doc.timeline.clips.filter((c) => c.assetId !== assetId), | ||
| skipRanges: doc.timeline.skipRanges.filter((r) => r.assetId !== assetId), | ||
| }, | ||
| project: { | ||
| ...doc.project, | ||
| ...(primaryAssetId ? { primaryAssetId } : {}), | ||
| updatedAt: new Date().toISOString(), | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## file list\n'
git ls-files 'electron/ai-edition/document-service.ts' '*AxcutDocument*' '*document*service*' '*primaryAssetId*' | sed 's#^`#-` #'
printf '\n## outline document-service\n'
ast-grep outline electron/ai-edition/document-service.ts --view expanded || true
printf '\n## search primaryAssetId and AxcutDocument\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'primaryAssetId|type AxcutDocument|interface AxcutDocument|project:' electron . || trueRepository: getopenscreen/openscreen
Length of output: 10347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## document-service removeAsset slice\n'
sed -n '187,215p' electron/ai-edition/document-service.ts
printf '\n## schema slice\n'
sed -n '340,365p' src/lib/ai-edition/schema/index.ts
printf '\n## removeAsset tests\n'
sed -n '150,190p' electron/ai-edition/document-service.test.tsRepository: getopenscreen/openscreen
Length of output: 3782
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## save/writeProject slice\n'
sed -n '214,260p' electron/ai-edition/document-service.ts
printf '\n## getProject/saveProject slice\n'
sed -n '109,150p' electron/ai-edition/document-service.ts
printf '\n## project schema helper references\n'
rg -n 'JSON.stringify|writeFile|ensureDocument\(|documentSchema.parse|primaryAssetId' electron/ai-edition/document-service.ts src/lib/ai-edition/schema/index.ts src/lib/ai-edition/document/migrate.ts src/lib/ai-edition/store/projectStore.tsRepository: getopenscreen/openscreen
Length of output: 3911
Overwrite primaryAssetId unconditionally. When the last asset is removed, ...(primaryAssetId ? { primaryAssetId } : {}) leaves the old doc.project.primaryAssetId in place via ...doc.project, so the project still points at a deleted asset. primaryAssetId is optional, so assigning undefined here is fine.
🤖 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/ai-edition/document-service.ts` around lines 193 - 209, The project
update in the document removal flow is preserving a deleted primary asset
because `...doc.project` carries the old value forward and the conditional
spread in the `next` object only updates `primaryAssetId` when truthy. In the
`document-service.ts` asset removal path, update the `next.project` construction
so `primaryAssetId` is always explicitly set from the computed `primaryAssetId`
value, even when it is `undefined`, while keeping the existing `updatedAt` and
other fields unchanged.
Comment on lines
+34
to
+42
| async load(): Promise<void> { | ||
| try { | ||
| const raw = await fs.readFile(this.configPath, "utf8"); | ||
| this.config = JSON.parse(raw); | ||
| } catch { | ||
| this.config = null; | ||
| } | ||
| await this.loadCredentials(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
load() must be called before getConfig()/getApiKey() return persisted state.
Both config and credentials start empty and are only populated by load(). Callers that construct a store and immediately read from it (see handlers.ts runAiEditionChat/getAiEditionLlmConfig) will see config === null. Consider lazily loading on first read, or document the required load() call. Root cause + fix tracked in the handlers.ts comment.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 35-35: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFile(this.configPath, "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-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/ai-edition/llm-config-store.ts` around lines 34 - 42,
`LlmConfigStore` currently only populates persisted `config` and credentials
inside `load()`, so `getConfig()` and `getApiKey()` can return empty state when
called on a fresh instance. Update `LlmConfigStore` so it either lazily loads on
first read or otherwise guarantees `load()` has run before
`getConfig()`/`getApiKey()` are used, and make sure the relevant call sites in
`runAiEditionChat` and `getAiEditionLlmConfig` rely on that behavior.
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.
Summary
Four-doc foundation for the OpenScreen x Axcut AI-edition merge. No code changes — planning only. Phase 0 (vendor
axcut-schema, schema v3,migrateDocument, feature flag) follows in a separate PR.ai-edition-merge-plan.mdAxcutDocument; Python worker + Fastify server are dropped.openscreen-inventory.mdfile:linerefs).axcut-inventory.mdai-edition-collision-analysis.mdWhy this isn't a code PR
The merge touches 100+ files across the renderer, main process, native helpers, and i18n. Shipping the planning docs as PR #1 lets reviewers agree on:
AxcutDocumentas SSOT)…before any code lands. The next PR (Phase 0) becomes much smaller and reviewable.
Locked decisions (in §5 / §9)
@tanstack/react-query(Phase 6 leans on it).safeStorage(no plain JSON).StreamingVideoDecoder. Documented limitation: known scrubbing lag on >30 min recordings — revival path in §8.1..openscreenfile extension.Known limitations
See
ai-edition-merge-plan.md§8 — the main one is §8.1 (proxy MP4 dropped despite user reports of scrubbing lag on long recordings).Reviewers
This is a docs PR. Suggested reviewers: anyone who'll write Phase 0 code, plus the project maintainers.
Related
Phase 0 PR will follow once these land.
Summary by CodeRabbit