chore: remove + skip button in the chat panel (#182) - #187
Merged
Conversation
Drops the chat-panel '+ skip' control, its six i18n keys across all 13 locales, and the runAddTrim handler / useOptimisticTimelineOps hook that existed only for it (the hook had no other callers and is removed with the file). Refs #182.
After removing the chat-panel '+ skip' button (the only renderer-side caller of nativeBridgeClient.aiEdition.runTimelineOperation), the entire IPC chain is dead: the renderer method, the action variant in the IPC request union, the AiEditionService method, the bridge wiring, the IPC handler, and the 'timeline.run' case in the dispatcher. The backend runTimelineOperation in chat-service.ts is kept (it is a clean main-process API for any future caller). The chat-service.test.ts suite that exercises it is also kept. Refs #182.
The chat-panel '+ skip' removal (7e2bcaf) left the transcript-pane trim handlers (handleAddTrimRange / handleRemoveTrimRange) as the last user of the saveQueueRef pattern — and they were untested. The pattern is non-trivial: a serialised save queue where the doc read happens INSIDE the chain (after awaiting the previous save), otherwise two rapid Backspaces would race and clobber each other's edit. Extracts the pattern into src/lib/ai-edition/store/useSequentialTimelineOps.ts with the same three properties the inline version depended on: 1. Two concurrent apply() calls are serialised — op N+1 reads the doc op N committed, not the pre-op-N doc (race fix preserved). 2. A save rejection doesn't poison the queue; the next call still has a resolved promise to chain off (error swallow preserved). The original promise returned to the caller keeps its rejection. 3. The store-empty fallback (no project loaded) returns null instead of crashing. The hook is generic over the operation type (add_trim_range, remove_trim_range, future timeline ops), not hard-coded to trim ranges — the transcript-pane handlers are now a one-line apply() call each, and any future renderer op (e.g. a timeline ruler drag-to-trim) can use the same hook instead of reinventing the queue. External signatures of handleAddTrimRange / handleRemoveTrimRange are unchanged (RightPanes doesn't await the return value). Includes a 4-test suite covering all three properties. NewEditorShell.tsx: -53 / +18 lines.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
…satisfy biome - Add cameraTrack: null to the asset fixture (the asset schema requires it after the v3 -> v4 migrate moved cameraTrack onto each asset). This was a new error on PR #187 that pushed the test-typecheck baseline from 80 to 81. - Run biome check --write on the three files the lint job flagged (LeftPanel.tsx, NewEditorShell.tsx, useSequentialTimelineOps.test.ts) for the unsorted-imports and formatter errors. tsc, biome, vitest (1147/1148 — the one fail is the pre-existing Windows temp-dir race in document-service.test.ts, passes 23/23 in isolation), i18n and docs checks all stay clean.
EtienneLescot
added a commit
that referenced
this pull request
Jul 29, 2026
…ure flag Findings from the v1.8.0 ponytail audit (see also PR #187): - Delete \src/utils/getTestId.ts\: the TestId type and getTestId() helper are not imported anywhere; tests use hardcoded data-testid strings. - Delete \src/components/video-editor/featureFlags.ts\: the single constant \AI_FEATURES_ENABLED = true\ is referenced only by a doc comment and by \LeftPanel.tsx\ as a runtime gate. Inline the gate in the two sites that used it and update the architecture docs to match. The new editor ships as the default from Phase 1 PR 1.3 onward; an always-true flag was carrying its own explanation as cargo. - Drop the \MIN_DELTA\ and \VIEWPORT_SCALE\ exports from \src/components/video-editor/videoPlayback/constants.ts\: only the source file references either. - Demote \RenderableChatMessage\, \estimateTokens\, and \DEFAULT_CHAT_BUDGET_TOKENS\ in \src/components/ai-edition/chatBudget.ts\ to file-private — no external importer. - Delete \scripts/bench-export.mjs\ (retired harness, runner already removed per rendering-performance.md:371), \scripts/stt-dev-server.mjs\, \scripts/e2e-pipeline-smoke.mjs\, \scripts/e2e-stt-smoke.mjs\. None are wired into \package.json\ scripts. - Tidy the now-stale comment in \electron-builder.json5\ that explained the \ fmpeg.exe\ exclude via \�ench-export.mjs\. No behavior change: tsc clean, lint unchanged, 1144/1144 unit tests pass.
EtienneLescot
added a commit
that referenced
this pull request
Jul 31, 2026
…satisfy biome - Add cameraTrack: null to the asset fixture (the asset schema requires it after the v3 -> v4 migrate moved cameraTrack onto each asset). This was a new error on PR #187 that pushed the test-typecheck baseline from 80 to 81. - Run biome check --write on the three files the lint job flagged (LeftPanel.tsx, NewEditorShell.tsx, useSequentialTimelineOps.test.ts) for the unsorted-imports and formatter errors. tsc, biome, vitest (1147/1148 — the one fail is the pre-existing Windows temp-dir race in document-service.test.ts, passes 23/23 in isolation), i18n and docs checks all stay clean.
EtienneLescot
added a commit
that referenced
this pull request
Jul 31, 2026
…ure flag Findings from the v1.8.0 ponytail audit (see also PR #187): - Delete \src/utils/getTestId.ts\: the TestId type and getTestId() helper are not imported anywhere; tests use hardcoded data-testid strings. - Delete \src/components/video-editor/featureFlags.ts\: the single constant \AI_FEATURES_ENABLED = true\ is referenced only by a doc comment and by \LeftPanel.tsx\ as a runtime gate. Inline the gate in the two sites that used it and update the architecture docs to match. The new editor ships as the default from Phase 1 PR 1.3 onward; an always-true flag was carrying its own explanation as cargo. - Drop the \MIN_DELTA\ and \VIEWPORT_SCALE\ exports from \src/components/video-editor/videoPlayback/constants.ts\: only the source file references either. - Demote \RenderableChatMessage\, \estimateTokens\, and \DEFAULT_CHAT_BUDGET_TOKENS\ in \src/components/ai-edition/chatBudget.ts\ to file-private — no external importer. - Delete \scripts/bench-export.mjs\ (retired harness, runner already removed per rendering-performance.md:371), \scripts/stt-dev-server.mjs\, \scripts/e2e-pipeline-smoke.mjs\, \scripts/e2e-stt-smoke.mjs\. None are wired into \package.json\ scripts. - Tidy the now-stale comment in \electron-builder.json5\ that explained the \ fmpeg.exe\ exclude via \�ench-export.mjs\. No behavior change: tsc clean, lint unchanged, 1144/1144 unit tests pass.
EtienneLescot
added a commit
that referenced
this pull request
Aug 1, 2026
…satisfy biome - Add cameraTrack: null to the asset fixture (the asset schema requires it after the v3 -> v4 migrate moved cameraTrack onto each asset). This was a new error on PR #187 that pushed the test-typecheck baseline from 80 to 81. - Run biome check --write on the three files the lint job flagged (LeftPanel.tsx, NewEditorShell.tsx, useSequentialTimelineOps.test.ts) for the unsorted-imports and formatter errors. tsc, biome, vitest (1147/1148 — the one fail is the pre-existing Windows temp-dir race in document-service.test.ts, passes 23/23 in isolation), i18n and docs checks all stay clean.
EtienneLescot
added a commit
that referenced
this pull request
Aug 1, 2026
…ure flag Findings from the v1.8.0 ponytail audit (see also PR #187): - Delete \src/utils/getTestId.ts\: the TestId type and getTestId() helper are not imported anywhere; tests use hardcoded data-testid strings. - Delete \src/components/video-editor/featureFlags.ts\: the single constant \AI_FEATURES_ENABLED = true\ is referenced only by a doc comment and by \LeftPanel.tsx\ as a runtime gate. Inline the gate in the two sites that used it and update the architecture docs to match. The new editor ships as the default from Phase 1 PR 1.3 onward; an always-true flag was carrying its own explanation as cargo. - Drop the \MIN_DELTA\ and \VIEWPORT_SCALE\ exports from \src/components/video-editor/videoPlayback/constants.ts\: only the source file references either. - Demote \RenderableChatMessage\, \estimateTokens\, and \DEFAULT_CHAT_BUDGET_TOKENS\ in \src/components/ai-edition/chatBudget.ts\ to file-private — no external importer. - Delete \scripts/bench-export.mjs\ (retired harness, runner already removed per rendering-performance.md:371), \scripts/stt-dev-server.mjs\, \scripts/e2e-pipeline-smoke.mjs\, \scripts/e2e-stt-smoke.mjs\. None are wired into \package.json\ scripts. - Tidy the now-stale comment in \electron-builder.json5\ that explained the \ fmpeg.exe\ exclude via \�ench-export.mjs\. No behavior change: tsc clean, lint unchanged, 1144/1144 unit tests pass.
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
Removes the chat-panel
+ skipbutton and the now-dead code it left behind. The button was the only renderer-side caller ofnativeBridgeClient.aiEdition.runTimelineOperation; deleting it also lets us drop the entire IPC chain (renderer method, IPC handler, bridge wiring, dispatch case, type variant) and the backendrunTimelineOperationinchat-service.ts:454stays as a clean main-process API for any future caller. Finally, the trim queue that lived inline inNewEditorShell.tsx(a serialisedsaveQueueRefwith a doc read inside the chain to dodge a race) is nowuseSequentialTimelineOps, a generic hook any future renderer timeline op can reuse.3 commits, 23 files, +304/-333.
Related issue
Fixes #182
Type of change
Release impact
Desktop impact
Screenshots / video
N/A — the only visible change is the removal of a button that was never reachable from a usable UX (it asked the user to type
startSec-endSecby hand, while the actual flow is the transcript pane's click-word + Backspace).Testing
npx tsc --noEmit— cleannpm run i18n:check— PASSED (12 locales, 7 namespaces)npm run test— 1147/1148. The one failure is a pre-existing Windows temp-dir race indocument-service.test.ts > concurrent saves(ENOTEMPTY onC:\Users\camil\AppData\Local\Temp\openscreen-ai-edition-*);git diffshows zero changes to that file or its subject, and it passes 23/23 in isolation.src/lib/ai-edition/store/useSequentialTimelineOps.test.tscovering the queue race fix, error swallow, store-empty fallback, and saved-doc return value.Commits
chore: remove + skip button in the chat panel(the actual chore: remove + skip button in the chat panel #182 fix)chore: drop the now-unused aiEdition.runTimelineOperation IPC plumbing(follow-up cleanup — the IPC had no other callers)refactor: extract useSequentialTimelineOps hook from NewEditorShell(the inlinesaveQueueRefpattern was untested; the hook is generic overAxcutTimelineOperationso a future timeline op can reuse it)