feat: --gif mode (frames → inline animated GIF)#33
Merged
Conversation
filepath.Base has no separator to strip from a bare "." or "..", so it passed them through unchanged and Join resolved outside the temp dir. Fall back to the default name for those two literal cases and pin the fix with a regression test.
filepath.Base of an all-separator path returns a single separator unchanged (nothing to strip), so a "/" name passed the existing "."/ ".." guard and Join collapsed it onto tmpDir itself, failing the write with an is-a-directory error instead of falling back to clip.gif. Extend the guard and pin it with a test case.
Wires --gif/--delay/--colors flags into runWithDeps and runUpload so the CLI can collapse ordered frame files into one animated GIF and upload that instead of the individual frames. --name now also labels the --gif output basename and is routed through validateName (not just assembleGIF's internal filepath.Base guard). --gif rejects stdin input, and --colors/--delay are validated at the boundary rather than silently coerced.
Council review flagged two real gaps: --delay had no upper bound (gifenc rounds it into a 16-bit GIF centisecond field, so values above 655350ms would silently wrap and corrupt playback speed instead of erroring), and --gif --name didn't require a .gif extension (comment.go picks inline-vs-link rendering purely from the uploaded basename's extension, so a mismatched name would defeat --gif's whole point). Three other findings from the same review were checked against the actual code and refuted — no fix needed, see task-3-report.md.
Adds the missing upper-boundary case for the --delay range check (655350ms accepted) so an off-by-one in the > vs >= comparison would be caught. The second council review round's other findings (claiming the .gif-extension guard on --name was reachable with an empty name, without --gif, or via stdin) were all refuted: the guard is nested inside opts.gif && opts.name != "", and --gif + stdin is already rejected earlier in the same function — both already covered by passing tests.
Renames the cap variable to captured across the added gif validation tests (the brief's own verbatim Step-1 test still uses cap and is left untouched). Also strengthens TestRun_GifMode_AcceptsMaxDelay to decode the produced GIF and assert the encoded delay is exactly 65535 centiseconds, via a gifBytesCapturingGitClient that reads the pushed file's bytes inside PushAttachments (assembleGIF's deferred cleanup removes the temp file before runWithDeps returns, so bytes can't be read back afterward).
The per-frame delay is written to the GIF stream as a uint16 by encoding/gif; only the low side was clamped, so a delay above 65535 centiseconds (e.g. doubled delay on the --size-ceiling re-encode path) silently truncated to a wrong, too-fast value instead of erroring or saturating.
Council review flagged the ignored gif.DecodeAll error: a malformed GIF would leave g nil and panic on the next line instead of failing cleanly. Declined the accompanying boundary-value (65535 vs 65536 cs) suggestion as out of scope for this fix.
Contributor
Author
AttachmentsDemo: 8 frames → --gif → this inline autoplay clip (pure Go, no ffmpeg)
|
…ncode comment Council review found README missing docs for --max-frames/--size-ceiling, inconsistent boundary validation (negative values silently treated as "disabled" instead of rejected like --colors/--delay), and an inaccurate comment claiming delay-doubling always preserves playback speed. Claude-Session: https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
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.
What
Adds
gh attach --gif: assembles ordered PNG/JPEG frames into a single animated GIF and uploads that one GIF inline via the existingrefs/uploads/issues/<N>path (a.gifis already inimageExtensions, so it renders inline and autoplays — an.mp4would only link).Encoding is pure Go —
image/gif+github.com/ericpauley/go-quantize(median-cut palette per frame + Floyd–Steinberg dither). No ffmpeg, no npm, no native binaries — exactly one new MIT dependency.Why
Foundation (slice C1) of a design to attach short video verification clips to frontend PRs: the house verify harness (Playwright) captures frames via CDP screencast, and this mode turns them into an inline autoplay clip on the PR — no ffmpeg (which breaks CI) and no JS deps.
Surface
--delayms per frame (rounded to GIF's 1/100s, clamped 2..65535);--colors2–256.--max-framesevenly samples down;--size-ceilingtriggers one reduced re-encode (fewer colors, half the frames). Out-of-range--colors/--delayare rejected, not coerced.--nameis gated byvalidateName.--gifonly collapses the frame list to one gif beforehand.Verification
go test ./...,go vet,golangci-lint(0 issues) green.--gif --key, uploaded, downloaded the blob back → valid GIF89a, 8 frames, loop=0, 480×240, correct delay; viewed it (legible text, clean gradient). Demo GIF posted below viagh attachitself.Follow-ups (non-blocking)
Tracked in #32 — JSON-mode warning field, re-encode-failure fallback, combined-guards test,
--nameextension UX nudge, shared name-sanitization helper. All Minor from the whole-branch review; none block this PR.https://claude.ai/code/session_01XGKirUaMS7kttgBwCzcJ3B