Redaction: solid-block default (drop gaussian, warn mosaic)#66
Conversation
Summary
Checks
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fbea7e5b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ound, e2e robustness, i18n Codex cloud review + execution-audit follow-ups on the blur-enable PR (#66): - timelineClipboardUtils: clamp the duplicated annotation/blur position to the canvas (getDuplicatedAnnotationPosition) so a full-frame or edge blur can't be nudged off-canvas and briefly expose an unredacted strip (Codex P2). - timelineCycleSelection: use an exclusive endMs bound so Tab-cycle selection matches the renderer/exporter active window (currentTimeMs < endMs) (Codex P3). - blur-redaction e2e: honor process.env.HEADLESS (default "true") instead of forcing headed runs; degrade gracefully when ffmpeg is absent (structural checks still run); remove the temp export dir in finally (Codex P2/P2/P3). - BlurSettingsPanel: replace the hardcoded "Duplicate" label with t("annotation.duplicateAnnotation"); add the key to all 13 locales. - Tests: cover cycle empty/single/edge cases and the duplicate clamp for full-frame and edge blurs. Full suite 405/405, tsc clean, biome clean.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9e118374f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex round-2 finding (blurEffects.ts:19 thread): BlurSettingsPanel's
shape/color/block-size handlers hard-coded blurData.type = "mosaic", so editing
any setting silently converted an imported gaussian ("blur") region back to
mosaic — losing the effect the renderer/exporter/persistence already support and
test (annotationRenderer.ts:245, annotationRenderer.test.ts:356/410-412,
projectPersistence.ts:400).
- blurEffects: add withBlurDataPatch(current, patch) — merges an edit while
preserving the existing blur type (mosaic default for a new region).
- BlurSettingsPanel: route all three edit handlers through it; no more type
clobbering.
- blurEffects.test: gaussian type preserved across shape/color/blockSize edits,
mosaic default for new regions, patched field applied.
Full suite 408/408, tsc clean, biome clean. Note: there is still no UI to
create/switch to gaussian, and gaussian is a weaker redaction than the mosaic
default — selector-vs-drop is a separate product decision, not this round.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7512f61bd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…arn mosaic, fix EOF
Reworks the blur feature into a redaction feature with a safe default. Research
(Hill et al. PoPETs 2016; Bishop Fox Unredacter; NSA) shows mosaic and gaussian blur
are recoverable for text; only a solid opaque block, burned into the export, is safe.
- New obscuring type "solid": 100% opaque color fill of the region, burned into the
MP4/GIF export and shown in the live preview. Now the default for new regions.
- Drop gaussian ("blur" type): renderer branch, preview branch, intensity slider, and
its tests. Removes the round-3 gaussian half-wiring findings at the root.
- Keep mosaic as a non-default, warned "not secure for text" option; add a Solid/Mosaic
type selector to the inspector; gate the block-size control behind mosaic.
- Migrate persisted type "blur" -> "solid" on load (normalizeBlurType); never reveals.
- Fix EOF zero-length blur: getPlayheadRegionSpan clamps the start so a blur added at the
video end keeps full duration (a zero-length region never renders/exports: < endMs).
- i18n: blurTypeSolid + mosaicSecurityWarning across all 13 locales.
Tests: solid render + full-opacity proof (inside pixel == opaque fill), normalizeBlurType
migration, getSolidFillColor, getPlayheadRegionSpan EOF clamp; gaussian tests removed with
the feature. Full suite 412/412, tsc + biome clean.
Addresses Codex round-3 (#66): EOF zero-length, gaussian-intensity, freehand-import-only.
Freehand stays renderer-honored for imported data; a freehand drawing tool is a follow-up.
My-Denia
changed the title
|
@codex review |
|
@claude review |
|
Claude finished @My-Denia's task in 2m 46s —— View job Review complete
No CLAUDE.md exists in this repo, so no repo-specific conventions to check against. I was not able to run Round-4 fixes verified as actually appliedAll four previously-flagged Codex items from earlier rounds check out in the current tree, not just in commit messages:
Core redaction logic
Minor / non-blocking
OverallThe redaction rework is sound: the solid path performs a genuine opaque replacement with no source sampling, the gaussian removal is clean (no dangling intensity UI or renderer branch), migration is safety-forward, and all four issues raised in the last Codex review round are verifiably fixed in code (not just asserted in commit messages). The two "minor" items above are cleanup, not correctness or security concerns — happy to see this merged as-is, with the orphaned i18n keys/intensity fields as an optional fast-follow. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8310ea7be9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex round-4: AnnotationOverlay forced solid opacity to 1 unconditionally. Since freehand blur regions span the full video surface and shapeMaskStyle drops the clip while isFreehandDrawing, a solid freehand region became a full-screen opaque panel during redraw, hiding the frame and the path being traced. Gate solid opacity with the same shouldShowFreehandBlurFill guard mosaic already uses (1 for rect/oval and for a settled freehand, 0 only while actively drawing).
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Redaction rework
Reworks the blur feature into a redaction feature with a safe default.
Deep research (Hill et al. PoPETs 2016; Bishop Fox Unredacter; NSA Redacting with Confidence; Signal/The Register) established that mosaic and gaussian blur are recoverable for structured text (passwords, IDs), and that the only method endorsed for redaction is a solid opaque block burned into the export. Video is higher-risk than screenshots (multi-frame).
Changes
blurData.type === "blur"): renderer branch, preview branch, intensity control, and its tests. Removes the round-3 gaussian half-wiring at the root, with no security loss (gaussian was the weakest method).type: "blur"(legacy gaussian) loads assolid(normalizeBlurType) — safety-forward, never reveals content.getPlayheadRegionSpanclamps the start so a blur added at the video end keeps its full duration — a zero-length region never renders/exports (renderAnnotationsfilters withcurrentTimeMs < endMs).blurTypeSolid+mosaicSecurityWarningacross all 13 locales.Verification
Full suite 412/412,
tsc --noEmit+ biome clean. The solid path is proven fully opaque at the pixel level (an inside pixel equals the opaque fill — the original frame is entirely replaced). Migration, EOF clamp, andgetSolidFillColorare unit-tested; gaussian tests were removed together with the feature.Codex round-3 disposition