Repository navigation
fix(skills): remove duplicated media-cleanup block in slideshow - #3218
imprimisxo wants to merge 2 commits into
Conversation
Lines 431-461 were repeated at 463-491 — the presenter-notes paragraph plus the whole of "Media cleanup on slide exit" and "Global nav mute". The copies were not identical. The second dropped the rule that custom media controls must listen to the same native element the player stops and update through media events rather than parallel state. Because it came last, an agent reading top-to-bottom would take the truncated copy as authoritative and lose that rule. Copy 1 is a strict superset, so the later copy is deleted outright.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving. I verified the dedup keeps the complete copy rather than the truncated one, which is the load-bearing question on a "remove the duplicate block" change.
The two copies were not identical: the earlier one carried a paragraph requiring custom media controls to sync through media events, and the later one did not. Counts at this head vs main:
main head
custom-controls paragraph 1 1 <- survives
Media cleanup on slide exit 2 1
Global nav mute 2 1
presenter-notes paragraph 2 1
file length 559 529 <- exactly 30
Code fences stay balanced (28, even). The only lines still repeated are { / } / </p> inside the example HTML, which is expected.
Worth saying the payoff is not tidiness: a skill file is read into context on every invocation, so a duplicated instruction block is paid for on each run, and two copies of the same rule that disagree is a correctness hazard for whatever reads it.
The 154-line skeleton extraction noted in the description looks like a reasonable follow-up but is rightly not in this PR.
— Rames Jusso
The `Skills: manifest in sync` check hashes skill content, so editing skills/slideshow/SKILL.md without regenerating leaves the committed hash stale (71368acf61074198 -> 6a24a84b0c1a75f9). Generator output only, no hand edits. — Rames Jusso (James's assistant)
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Clean dedup. The deleted block (lines 463-498) is the incomplete copy — missing the custom-controls paragraph that the surviving copy at lines 420-460 retains. Manifest hash update is generator output. LGTM.
Review by Miga
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head review at f5098b281a3bd599320707842791ca2eea31b47b.
skills/slideshow/SKILL.md:420-461 retains the complete media contract, including the sole custom-controls/native-event synchronization paragraph. The later 30-line copy is the incomplete duplicate: after deletion, the presenter-notes paragraph and both affected headings each occur exactly once, the custom-controls rule remains exactly once, all 28 fence markers remain balanced, and the section flows directly into the existing separator and standalone guidance.
skills-manifest.json:66 is the only generated delta and updates the slideshow hash; exact-head Skills: manifest in sync, project-native skill lint/mirror, skills tests, Format, and every required check are terminal green or skipped as intended. Existing reviews contain no unresolved correctness finding.
Verdict: APPROVE
Reasoning: The current head removes only the truncated duplicate, preserves the stricter surviving contract, keeps the generated manifest coherent, and is green on all required gates.
— Magi
The media-cleanup and global-nav-mute sections appeared twice in skills/slideshow/SKILL.md. The two copies were not identical: the later one was missing the paragraph requiring custom media controls to sync through media events. Deletes the incomplete copy, keeps the complete one. Originally authored in #3218. Carried here re-signed because the org requires signed commits and both changes regenerate skills-manifest.json, which would otherwise conflict between the two PRs. Co-authored-by: Lance Curtis <[email protected]>
|
Heads up @imprimisxo, and thank you for this one. Your change is blocked here by something that has nothing to do with its quality: the org enforces signed commits on all branches, and Rather than ask you to re-sign and force-push, I carried it into #3225 byte-identical, with I re-verified your dedup at the new tree rather than taking it on trust: 559 to 529 lines, 28 code fences balanced, each repeated section down to one, and the custom-controls paragraph still present exactly once at line 445. Keeping the complete copy and deleting the incomplete one was the right call, and it is the part of a dedup that is easy to get backwards. Your #3219 report is fixed in the same PR. Both files you flagged as orphaned are deleted. — Rames Jusso (James's assistant) |
…e slideshow (#3225) * fix(skills): stop embedded-captions shipping author-only paths The skill is distributed via `hyperframes skills` (--copy, so the installed bundle matches the published tree), but three shipped files pointed at directories that only exist on the original author's machine. - references/test-set.md: deleted. Nothing in the skill referenced it (zero inbound links across all 140 files), and its corpus lives at ~/Downloads/heygen_relevant_videos/, so it was neither reachable nor runnable. - SKILL.md / dna/README.md: the retired 54-template archive was stated as living at a ~/Downloads path. Keeps the fact, drops the false location. - themes/PORTING.md: marked maintainer-only. It authors new theme DNAs rather than using the skill, nothing links it, and its inputs (the cap_fx3 demos, the frame corpora, CONTRACT.md) are not distributed. The paths stay as the author's original layout, now explicitly labelled as names for the inputs. The 10 references to ~/Downloads/hyperframes are untouched: those are the documented last resort in a real chain (HYPERFRAMES_ROOT -> in-repo -> that path), and every file carrying it also carries the env var. Regenerates skills-manifest.json, which hashes the whole skill directory. Closes #3219 — Rames Jusso (James's assistant) * fix(skills): delete themes/PORTING.md instead of marking it internal Nothing in the skill referenced it, its inputs are not distributed, and git history keeps it recoverable. Marking a dead procedure still ships the dead procedure. — Rames Jusso (James's assistant) * fix(skills): remove duplicated media-cleanup block in slideshow The media-cleanup and global-nav-mute sections appeared twice in skills/slideshow/SKILL.md. The two copies were not identical: the later one was missing the paragraph requiring custom media controls to sync through media events. Deletes the incomplete copy, keeps the complete one. Originally authored in #3218. Carried here re-signed because the org requires signed commits and both changes regenerate skills-manifest.json, which would otherwise conflict between the two PRs. Co-authored-by: Lance Curtis <[email protected]> * chore(skills): regenerate manifest for slideshow + embedded-captions — Rames Jusso (James's assistant) * chore(skills): record the two embedded-captions deletions in the guard check-no-main-deletions.mjs deliberately has no blanket override, so each intentional deletion is named with its reason and shows up in review. — Rames Jusso (James's assistant) --------- Co-authored-by: Lance Curtis <[email protected]>
|
Closing this out: your change is now on It merged as part of #3225 in commit d3a4e90, carried over byte-identical with your authorship preserved on the commit. The reason for folding it in rather than merging this PR directly is in my earlier comment above, and it was not a reflection on the change: the org ruleset requires signed commits, and a second PR touching skills would have collided with this one on Thanks for catching this. The two copies of that block had drifted apart, and the one you deleted was the incomplete one, which is the part that actually mattered and the part that is easy to get backwards. — Rames Jusso (James's assistant) |
What
Deletes a 30-line duplicated block from
skills/slideshow/SKILL.md. Pure deletion — +0 −30, no content added or reworded.Why
Lines 431–461 are repeated at 463–491: the presenter-notes paragraph, then the whole of
### Media cleanup on slide exitand### Global nav mute. Eleven paragraphs, 2,843 bytes — about 7.7% of the file.The copies are not identical. The second is a strict subset that drops exactly one normative paragraph:
That matters because of ordering. An agent reading the file top-to-bottom hits the complete version first and the truncated one second — and the natural reading of a repeated section is that the later one supersedes. So the rule most likely to be lost is the one about custom controls syncing through media events, which is exactly the kind of thing that produces a deck where the play button and the actual
<video>disagree after a slide change.Everything else in the two copies is verbatim identical, so copy 1 is a strict superset and the later copy can go without merging anything.
How
Removed lines 463–492 (the duplicate plus its trailing blank), keeping the
---separator that follows. Nothing else touched — the diff is+0 −30.Verified on the result:
### Media cleanup on slide exit,### Global nav mute,## Wrapping component,## Running a slideshow standalone: one heading eachreferences/standalone-harness.mdpointers retained (the presenter-notes paragraph and the standalone section — two distinct, legitimate mentions)One separate observation — no change made
Lines 242–396 are a 154-line, 4.8 KB fenced block: the "Scene HTML (skeleton)" for the 3-slide worked example. It is by far the largest code block in the file; the next largest is under 25 lines.
It could ship as an asset the way
references/standalone-harness.mdalready does, which would remove ~13% of the file and give the agent something to copy rather than retype. But an illustrative skeleton is a weaker case for extraction than a complete runnable document, and it may genuinely be more useful inline where the surrounding prose annotates it. Raising it rather than assuming — happy to send it as a follow-up if you want it.Test plan
Found while auditing skill files for context-budget cost.