chore: delete pre-fix copies that survived their own fixes - #397
Merged
PathGao merged 1 commit intoAug 2, 2026
Merged
Conversation
Four already-fixed behaviours each left a copy behind, and every copy looks like a reusable shared implementation: - `markdown.ts::renderRichContent` (141 lines, exported, zero importers) lacks `rememberDiagramSource`, so anything that "switched to the shared version" would silently disable the light-theme Mermaid re-render that PDF and HTML export depend on. It also hand-rolls the theme choice instead of `resolveMermaidTheme` and carries an inline DOMPurify config instead of the shared contract. - `markdown.ts::getLanguage` and `::highlightColorMap` duplicate the live copies in the viewer. - `Editor.svelte::getEditorScrollMax` is the pre-sftwrdotdev#316 version, using `getScrollHeight()` where the live one uses `getContentHeight()`. They were invisible because the tests guarding those behaviours each read one hard-coded path. `scripts/singleImplementationConvention.test.ts` generalises the allowlist idea to the whole `src` tree: one row per fixed behaviour, scanned everywhere, plus a meta-test asserting every rule still matches something so a rename cannot turn a rule into a no-op. Also fixes a `$effect` cleanup in Toc.svelte that removed its listener from the current value of a prop rather than the node it attached to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PathGao
added a commit
that referenced
this pull request
Aug 2, 2026
#397 shipped the "YouTube links never become embedded frames" rule with one allowlist entry for `replaceWithYoutubeEmbed` in MarkdownViewer.svelte, an uncalled pre-fix leftover it could not delete because #388 owned that file. Its comment said to delete the copy and the entry together. #388 deleted the copy; the entry stayed, so the marker now matches nothing and the meta-test that catches stale rules fires - correctly. The rule was always meant to be "allowed nowhere", which is now true. Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
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.
A pure deletion — 186 lines — plus one guardrail so this cannot recur silently.
These are not idle leftovers
Four already-fixed behaviours each left a copy behind, and every copy looks like a reusable shared implementation. The danger is not that they take up space; it is that a future "the viewer is too big, let's use the shared version" reverts a merged fix, with no test going red.
markdown.ts::renderRichContent(141 lines, exported)grep -rn "utils/markdown" src scripts src-tauri/src— onlyprocessMarkdownHtmlis ever importedrememberDiagramSource. Without that attribute, the light-theme Mermaid re-render that PDF/HTML export depends on (#359) finds nothing to restore. It also hand-rollsisDark ? "dark" : "neutral"instead ofresolveMermaidTheme— whose own comment says it exists "so the two cannot drift" — and carries an inline DOMPurify config instead of the shared contractmarkdown.ts::getLanguagemarkdown.ts::highlightColorMapEditor.svelte::getEditorScrollMaxgetScrollHeight(); the livegetEditorContentScrollMaxusesgetContentHeight(). Reviving it re-introduces the editor's bottom padding into split scroll syncEditor.svelte::getViewState,::revealLineeditorPanetype declares neitherisYoutubeLink/getYoutubeIdare kept — still called insideprocessMarkdownHtml— but their now-pointlessexportis dropped.Why nothing caught them
youtubeExternalFallback.test.ts,taskToggleMemory.test.ts,mermaidPrintTheme.test.tsandpreviewScrollSync.test.tseachreadFileSyncone hard-coded path. A second copy living anywhere else is invisible to all of them.scripts/singleImplementationConvention.test.tsgeneralises the allowlist idea already used byrenderPipelineConvention.test.ts: a data-driven table where each row is{ marker, allowed paths, optional requires }, scanned across the wholesrctree. Adding a newly-fixed behaviour is a one-row append. Eight rules ship, including:mermaid.render(may appear only inMarkdownViewer.svelte/mermaidPrint.ts, and each such file must also containrememberDiagramSource— that is fix(print): keep PDF exports readable in all themes #359 pinnedgetScrollHeight() - …heightis allowed nowhere — Exclude editor bottom padding from split scroll sync #316 pinnedcreateElement('iframe'),from 'dompurify',renderRichContent,getLanguage,highlightColorMapeach pinned to their one legitimate ownerPlus a meta-test asserting every rule still matches something, so a renamed symbol cannot turn a rule into a silent no-op.
Negative control: with the two edited source files stashed, 7 of the 9 tests fail, then pass again. It catches precisely the copies this PR removes.
One unrelated fix rode along
Toc.svelte's scroll$effectreturned a cleanup that calledremoveEventListeneron the current value of a prop rather than the node it had attached to. The element is stable today so it does not misbehave, but if the prop ever changed the listener would leak, and if it becamenullthe cleanup would throw. Now captures the node.Checks
No test lost coverage — none of the deleted symbols had a test referencing it, and
previewScrollSync.test.tspins the livegetEditorContentScrollMax, untouched.One line in another test: #384's
DOMPurify.sanitizecall-site allowlist listedmarkdown.ts, whose only call site was inside the deadrenderRichContent. That allowlist must be exactly consumed, so deleting the call site correctly failed it. The entry is removed. This is the allowlist doing its job, not a workaround.Not deleted, and why
onDestroyinEditor.svelte— it is used; an earlier scan claimed otherwise and was wrong.exportHtml.ts::escapeHtmlText, a zero-incrementreturn escapeHtml(v)wrapper — it still has a live caller inexport.ts. Worth inlining separately.Editor.svelte'sgetValue/setValue/focus/restoreViewStatecomponent exports also appear to have no callers (every in-file hit is on the Monaco object, not the component export). Left alone to keep this a scoped deletion; worth a follow-up.MarkdownViewer.svelte—replaceWithYoutubeEmbedbuilds the very<iframe src="youtube.com/embed/…">that a merged fix replaced with a browser-opening thumbnail, andframe-srcis gone from the CSP so it could not even load. It is zero-caller, andyoutubeExternalFallback.test.tsasserts no iframe — but only againstmarkdown.ts, so this copy was invisible: exactly the failure mode this PR exists to close. It is handled in refactor(preview): drop the dead YouTube iframe copy from the viewer #388; until that lands, the iframe rule ships with that one path allowlisted under aKNOWN STALE COPYcomment telling the next person to delete the copy and the allowlist entry together.🤖 Generated with Claude Code