docs(agent): pistes d'amélioration de l'agent d'édition, mesurées - #217
Open
EtienneLescot wants to merge 11 commits into
Open
docs(agent): pistes d'amélioration de l'agent d'édition, mesurées#217EtienneLescot wants to merge 11 commits into
EtienneLescot wants to merge 11 commits into
Conversation
|
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:
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 |
4 tasks
EtienneLescot
force-pushed
the
claude/agent-improvement-leads
branch
from
July 31, 2026 23:47
9ee0ba5 to
0d2dbcb
Compare
EtienneLescot
force-pushed
the
release/v1.8.0
branch
from
August 1, 2026 01:04
4afa3d4 to
2200c7c
Compare
v7 gave trims a `clipId`, but three READERS still answered "which clip is this?" from `(assetId, sourceTime)` — or from POSITION IN THE ARRAY. Source time is per asset: the moment two clips draw on the same media, it names two places at once. One cause, the two reported symptoms, and two adjacent defects found while verifying. --- Playback stops on reaching the second clip `isWithinClipBounds` gave a clip an EXCLUSIVE closing edge unless it was the LAST ELEMENT OF THE ARRAY, and the `preferredClipId` guard shared that edge. In its final ~50 ms — exactly when the rAF loop decides what comes next — a clip disowned its own last frames and the ambiguous scan handed them to its twin, placing the playhead near the END of that twin. `reachedClipEnd` then fired on a clip nothing follows: pause, playhead parked at the end of the timeline. Because the exception keyed off array position, the bug DEPENDED ON CLIP ORDER, which is what the reporter's experiments isolated: `A1 -> A2 -> C3` did not bug (the last clip belongs to another asset, the filter excludes it, the scan returns null, and the timeline-order fallback did the right thing by accident); `A1 -> C3 -> A2`, `C3 -> A1 -> A2` and any pair of twins did. All six layouts are in the test. An INCLUSIVE bound when the clip is named — identity beats proximity, the exclusive bound exists only to break ties in the scan and a named clip has no tie to break — plus a two-pass scan: strict containment first, closing edge only if nobody claims the instant. Same answer on a single-asset timeline, reached without consulting the order. --- "Automatic zooms" only decorates the first clip Cursor telemetry is recorded against the ORIGINAL file: `timeMs` is the asset's SOURCE time (the axis `cursor-track.ts` maps through `locateSourcePosition`). Zooms are authored in RAW timeline ms. The two axes coincide for one layout only: a single clip, at 0, covering the whole recording. Anywhere else, suggestions landed where `[0, asset duration]` falls on the ruler — the first clip. Telemetry was also read only for `videoSources[0]`, so a second recording was never consulted. `buildAutoZoomSuggestionsForClips` does the projection, per clip and by a plain shift (a raw clip is identity between its source and raw-virtual time). A dwell replayed by two clips therefore yields one zoom on EACH. Each clip sees only the samples in its own source window: a dwell a cut splits is no longer one dwell, which is right — the cursor did not sit still across the cut on the timeline being watched. --- Two adjacent defects, found while verifying A cut was never skipped during playback if a twin kept the stretch: "am I inside a cut?" scanned every segment by asset. `locateKeptSegment` narrows the question to the segments of the clip being played — a source time none of them covers is inside THAT clip's cut, however many other clips keep it. `findNextKeptSegment`'s source-clock fallback compared source positions across a whole asset. "Later in source time" only means something within one clip: with a late slice laid down BEFORE a trimmed early slice, entering that cut answered the first clip — raw start 0 — so playback jumped to the top of the timeline, fell into the same cut, and looped. Pre-existing, but the fix above makes that path far more reachable: leaving it would have traded one bug for a worse one. --- What is NOT touched `totalVirtualDuration` (`clips.at(-1)`) and `locateVirtualPosition`'s `index === clips.length - 1` still depend on array order. `resequenceClips` maintains that order, so neither is reachable today, and `locateVirtualPosition` has five callers: that is a separate change, not something to smuggle into a bugfix. Tests: every fix was verified FAILING without it. The integration test reproduces the symptom end to end on the pre-fix code (`pause()` at 9.96 s). No existing test covered the closing edge of a clip with a twin over the same media.
The three new AxcutTrimRange fixtures omitted `reason` / `origin`. Invisible to `tsc --noEmit`, which excludes `**/*.test.ts(x)` — `tsconfig.test.json` exists precisely for what the main config leaves out, and it is the gate CI runs. Caught by review, not by me: I checked the wrong one of the two typecheck jobs. Two review points taken while here: - `isWithinClipBounds` defaulted a missing `sourceEndSec` to 0 (as it always had), where `resolvePlaybackSegments` reads the same absent field as a zero-width window at the in-point. `locateKeptSegment` now feeds this function that very output, so the two sit in series and must not read one field two ways. No behaviour changes: a clip only awaits probing with `sourceStartSec === 0`, where both defaults coincide. A divergence removed, not a bug. - The per-asset telemetry fetches ran one IPC round trip after another. Nothing in the loop depends on visit order (`existingRegions` is read up front), so they now run concurrently; `Promise.all` preserves order, so the output is identical.
Le repli logiciel existait déjà sur Linux, mais par accident : `create_backend` ignorait son paramètre et wgpu rendait lavapipe de lui-même quand c'était le seul ICD. Ça marche — et c'est précisément le problème, parce que rien ne pouvait l'exercer, rien ne le signalait et rien ne le garantissait. - `create_backend` honore enfin son paramètre. `Backend::Cpu` passe par `force_fallback_adapter` ; `Backend::Hardware` rejette explicitement un adaptateur logiciel, ce qui rend `create` réellement matériel strict (un golden mesuré sur llvmpipe passait jusqu'ici pour une mesure GPU). `create_auto` gagne le repli explicite Hardware -> Cpu, comme côté Windows. - `OPENSCREEN_COMPOSITOR_BACKEND=hardware|cpu` force le choix. `VK_DRIVER_FILES` ferait la même chose au niveau du loader Vulkan, mais s'applique au processus entier : sous Electron il prive aussi Chromium de son GPU, qui rastérise alors toute son UI sur CPU et sature la machine. Le chemin CPU n'était donc pas testable sans casser la session. Même motif que `OPENSCREEN_EXPORT_ENCODER`. - L'adaptateur retenu est journalisé. Windows loggue son repli, Linux ne loggait rien : un hôte tombé sur lavapipe rendait à quelques fps sans que rien ne permette de l'établir à distance. - `diagnose()` sépare « aucun ICD Vulkan installé » du reste et nomme le paquet à installer. C'est la seule panne de cette famille que l'utilisateur peut réparer lui-même, et elle s'affichait en « Aperçu indisponible sur cette machine ». - `classify` s'appuie sur `DeviceType::Cpu` — ce que l'ICD déclare — plutôt que sur une sous-chaîne du nom ; le nom reste en filet. - Le .deb et le pacman déclarent Mesa (`mesa-vulkan-drivers` / `vulkan-swrast`). Aucune dépendance n'était déclarée, donc rien ne garantissait qu'un ICD existe. - Nouveau job CI `Rust test (Linux compositor)`. Le Rust Linux n'était compilé nulle part : la CI couvrait macOS et Windows, et les 2154 lignes du moteur wgpu ne passaient que par le poste des contributeurs. Le job installe lavapipe, donc il exerce pour de vrai le backend CPU sur un runner sans GPU. Deux corrections que ce job a révélées, et sans lesquelles il ne peut pas exister : - `export_timing.rs` et `output_geometry_golden.rs` ne compilaient pas sous Linux (`probe_frame_count` / `readback_resized` n'existent que côté Windows et macOS). Les fichiers de `tests/` étant compilés quelle que soit la plateforme, le crate entier était incompilable en `--tests` sur Linux, en silence. Gardés en `cfg(not(target_os = "linux"))` plutôt qu'en `cfg(windows)`, pour ne pas les retirer du job macOS qui les compile aujourd'hui. - L'export GIF construisait son device avec `Gpu::create` (matériel strict) alors que son propre commentaire dit suivre l'export MP4, qui prend `create_auto`. Un hôte sans GPU exportait donc un MP4 mais pas un GIF — sur le seul chemin où le backend CPU existe précisément pour que l'export aboutisse. Le rendre strict sur Linux sans ce correctif aurait cassé le GIF sur les machines lavapipe qui fonctionnent aujourd'hui.
…Linux `find | head -1` prenait le premier résultat dans l'ordre de parcours du système de fichiers, qui n'est pas trié. L'image ubuntu-latest embarque plusieurs LLVM : si l'un d'eux a un paquet -dev préinstallé, on pouvait sélectionner une version autre que celle qu'apt vient d'installer. Les headers built-in de clang étant liés à la version de libclang, le symptôme aurait été `stddef.h file not found` — une erreur qui ne désigne pas sa cause. `sort -V | tail -1` prend la plus récente, de façon déterministe. Remonté par CodeRabbit sur #223.
…tions on it Clicking "Smart cuts with AI" was the natural first move in the editor and the one that could not work: nothing produced a transcript until the user found the Media tab or the transcript pane and asked for one. Recognition is local and free, so the editor now produces transcripts by itself, and every affordance that needs one says what it is waiting for instead of doing nothing. - `store/transcriptionStore.ts` owns the queue: one asset at a time (whisper-server is a single process and audio extraction holds decoded frames in renderer memory), auto-enqueued from `useAutoTranscription()` on any document change. The transcript itself still lives on the document — the store only owns the job. - The auto pass cannot loop: an asset is enqueued only when it has no transcript, no job entry (queued / running / failed alike) and no persisted failure, and a job is dropped only after the save carrying its transcript resolved. Runs are stamped with a run id so a manual regenerate can supersede one mid-flight without the outgoing run clearing its successor. - `transcription/status.ts` holds the vocabulary: `deriveAssetStatus` folds job + document into one status (a stored transcript outranks a failed retry — the previous transcript is still usable), `resolveTranscriptGate` folds a set of those into ready / pending / blocked, resolved over the assets the TIMELINE plays rather than over `primaryAssetId` (which in a recording project is the screen capture, routinely the silent one). - Silent media are remembered: a container with no audio track fails the same way every time, so the verdict lands on `asset.transcriptionFailure` (additive schema field, no version bump) and the auto pass stops re-extracting its audio on every project open. Transient failures stay in memory and retry next load. - No local STT engine (browser preview, e2e shim) means no background pass; a manual request still runs. - Removes `projectStore.setTranscript` (it duplicated `withTranscript`) and the `assetStatuses` / `onRegenerateAsset` / `onTranscribe` / `isTranscribing` prop chains — the panes read the store. 25 tests cover the pure logic, the queue (loop guard, supersede, project switch, no engine, silent-verdict persistence) and the rendered gating.
…download The 253 MB model download happens inside the `stt:transcribe` IPC — i.e. inside a run the store has already marked `running` — so it correctly reads as one longer "Transcribing…" phase, with no separate step and nothing falsely clickable. The edges of that first run did not hold up, though: - `SttManager.init` cached a REJECTED `prepare()`, so one dropped connection during the download failed every later transcription in the session: the remaining assets in the queue flipped red in the same frame, and the retry the editor offers was a dead control until the app was restarted — reconnecting changed nothing. The slot is cleared on failure now. - A transient failure now stops the queue: the still-queued assets inherit the verdict instead of each spending a full retry budget and stacking an identical toast. It is the engine that failed, not their media. - The transcript pane's read-only state is scoped PER ASSET again. Widening it to the timeline-wide gate made every other clip's word stream swallow Backspace and hover-bin clicks for the whole background pass, with nothing on screen to say why — the exact "looks live, ignores you" failure mode. A block being rewritten now shows a spinner + "Transcribing…" and dims its stream, and the per-clip empty line no longer tells the user to regenerate an asset that is mid-run. - `mixToMono` hoists its channel arrays out of the sample loop. One WebIDL call per sample per channel (~57 M for a ten-minute stereo recording) froze the window — spinners included — for seconds, which was survivable while the pass was user-triggered and is not now that it is automatic. Found by an adversarial review of the first-run sequence; 4 new tests cover the init retry, the queue stop and the per-asset read-only scoping.
`tsc -p tsconfig.test.json` (its own CI job) rejected the two held-open-run tests: assigning the resolver inside a Promise executor leaves the outer `let releaseFirst: (() => void) | null` narrowed to `null`, so `releaseFirst?.()` is a call on `never`. A small `deferred()` helper hands back a stable 0-arg release instead. Also fixes the instruction that let this reach CI: AGENTS.md said `npx tsc --noEmit` was what CI runs, and that config does not see test files at all.
…warning `.claude/` is gitignored (while `launch.json` is tracked), so its local desktop-dev tweak slipped in through `git add -u` — reverted here, it stays a local change. Also puts a comment in `deferred()`'s placeholder so it stops tripping noEmptyBlockStatements.
Document de travail: chaque piste porte la mesure qui la justifie et la contre-mesure qui la départagera. Rien n'est appliqué ici.
…pels par lot Le track n'était pas la cause des échecs: les deux tours réussis prenaient 117 s et 112 s pour un couperet à 120 s. Ce que révèle la mesure, c'est que le tour émet 19 appels d'outils en série — six addTrim et neuf addZoom un par un.
EtienneLescot
force-pushed
the
claude/agent-improvement-leads
branch
from
August 1, 2026 13:48
0d2dbcb to
64b2f74
Compare
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.
Pistes d'amélioration de l'agent d'édition, appuyées sur les mesures du workbench. Rien à fusionner ici — cette PR est un document de travail, à traiter plus tard.
Chaque piste porte la mesure qui la justifie et, quand elle existe, la contre-mesure qui la départagera. L'ordre est celui où je les traiterais.
1. Le poids du track fait échouer un tour sur deux
Mesuré. Sur la prise réelle de 66 s,
getCursorTrackrend 356 points pour 24 238 caractères. La requête suivante passe à ~45 000 caractères. Sur 5 répétitions du prompt wizard, 3 ont expiré à 120 s, toujours au même endroit : juste après l'appel à l'outil. Les 2 qui aboutissent produisent un montage correct.Ce n'est pas un défaut du modèle : lui donner la donnée le fait échouer.
Pistes, de la moins à la plus intrusive :
virtualSecquand il est égal àatSec. 28 % du payload, strictement redondant tant qu'aucune coupe n'existe. Un champvirtualEqualsSource: trueen tête suffirait. Gain immédiat, aucune perte d'information.Le plafond de
buildCursorTrackest par ailleurs mou :DEFAULT_MAX_TRACK_POINTSborne la grille, mais les points gardés pour un changement de forme s'ajoutent par-dessus sans quetruncatedle signale. Ici 356 pour 400, sans conséquence — une capture riche en changements de pointeur dépasserait silencieusement.2. Le modèle place ses zooms d'après le transcript, pas d'après la trajectoire
Mesuré. Il appelle bien
getCursorTrack. Mais en comparant lefocusqu'il choisit à la position réelle du curseur dans sa propre fenêtre de zoom : 7 sur 9 sont faux, trois de plus d'un tiers d'image. Le pire vise(0.33, 0.09)— haut de l'écran — quand le curseur est à(0.38, 0.60).Son récit le trahit : il annonce un zoom sur « Iceman, Views » cinq secondes avant que ces mots soient prononcés. Il raconte une lecture de la trajectoire qu'il n'a pas faite.
Rappel 6/6 zones annotées, mais précision 0,41 — il zoome 38 % de la vidéo. Toucher toutes les zones en arrosant n'est pas de la détection.
Pistes :
addZoompourrait renvoyer la position réelle du curseur sur la fenêtre demandée, à côté dufocusreçu. Le modèle apprend l'écart au premier appel, sans qu'on lui impose quoi que ce soit. C'est la piste que je préfère : elle informe au lieu de contraindre.{atSec, cx, cy}sont peut-être trop plates pour qu'il y corrèle une fenêtre temporelle. À tester en réduisant d'abord le bruit (piste 1), pas en changeant la forme.3.
customScalerenddepthinopérant en silenceMesuré.
describe-zoomsest passé de 60 % à 98 % après correction de la table depth→échelle.describe-zooms-migratedreste à 33 % : quand un zoom porte uncustomScale, ledepthne rend plus rien et aucun champ ne le dit au modèle.Piste. Le snapshot expose déjà
depthIsOverridden. Reste à vérifier qu'il atteint le modèle dans tous les chemins, et quesetZoomdit clairement que passerdepthefface lecustomScale.4. Un patron récurrent : l'absence traitée comme un non-événement
Trois occurrences rencontrées en pilotant l'app, sans rapport entre elles :
Le troisième mérite un correctif, et le patron mérite d'être nommé quelque part : distinguer « je n'ai pas trouvé » de « il n'y a rien » est la même discipline côté UI et côté agent.
5. Le banc : ce qui manque encore
noa déjà matché danscannot, accusant de mensonge une réponse honnête. Et « pas de signal » compte comme une réussite, donc une réponse en français passerait au vert sans rien vérifier. Ce qui se calcule doit rester déterministe ; ce qui demande de lire du sens doit passer à un juge, sur les tours persistés, avec verdicts conforme / fautif / indéterminé.workbench/fixtures/README.md.