Desktop: Fixes #15057: Stabilize heading checkbox hashes#15059
Closed
Sandesh13fr wants to merge 5 commits intolaurent22:devfrom
Closed
Desktop: Fixes #15057: Stabilize heading checkbox hashes#15059Sandesh13fr wants to merge 5 commits intolaurent22:devfrom
Sandesh13fr wants to merge 5 commits intolaurent22:devfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds heading normalisation that strips leading checkbox markers before slug generation; emits both canonical and legacy heading hashes/IDs, updates heading iteration and link/jump resolution to accept either form, and preserves legacy anchor IDs in rendered HTML for backward compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Editor as Editor (CodeMirror / ProseMirror)
participant ForEach as forEachHeading
participant Normalise as normalizeHeadingForHash
participant Slug as uslug/slugify
User->>Editor: Click internal link "#target"
Editor->>ForEach: Iterate headings (callback receives hashes[])
ForEach->>Normalise: normalise heading text
Normalise->>Slug: slugify(normalisedText) -> canonicalHash
ForEach->>Slug: slugify(originalText) -> legacyHash (if different)
ForEach-->>Editor: callback(hashes = [canonical, legacy?], pos)
Editor->>Editor: if hashes.includes(target) -> setSelection/scroll
Editor-->>User: Navigate to heading
sequenceDiagram
participant Renderer as Markdown Renderer
participant NormaliseR as normalizeHeadingForHash
participant SlugR as uslug/slugify
participant Output as HTML output
Renderer->>NormaliseR: normalise heading inline text
NormaliseR->>SlugR: slugify(normalisedText) -> canonicalId
Renderer->>SlugR: slugify(originalText) -> legacyId
alt canonicalId != legacyId
Renderer->>Output: emit heading id=canonicalId
Renderer->>Output: emit empty anchor id=legacyId class="joplin-heading-legacy-anchor"
else
Renderer->>Output: emit heading id=canonicalId
end
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
coderabbitai
bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/editor/ProseMirror/commands/commands.test.ts (1)
86-107: Refactor repeated hash assertions into a table-driven loop.This block repeats the same assertion structure five times; a compact table keeps intent identical with less duplication.
♻️ Proposed refactor
test('jumpToHash should resolve canonical and legacy hashes for task-marker headings', () => { const editor = createTestEditor({ html: '<h2>[ ] title</h2><h2>[x] checked title</h2><h2>[X] done title</h2>' }); const jumpToHash = (hash: string) => { return commands[EditorCommandType.JumpToHash](editor.state, editor.dispatch, editor, [hash]); }; - expect(jumpToHash('title')).toBe(true); - expect(editor.state.selection.$anchor.parent.textContent).toBe('[ ] title'); - - expect(jumpToHash('checked-title')).toBe(true); - expect(editor.state.selection.$anchor.parent.textContent).toBe('[x] checked title'); - - expect(jumpToHash('x-checked-title')).toBe(true); - expect(editor.state.selection.$anchor.parent.textContent).toBe('[x] checked title'); - - expect(jumpToHash('done-title')).toBe(true); - expect(editor.state.selection.$anchor.parent.textContent).toBe('[X] done title'); - - expect(jumpToHash('x-done-title')).toBe(true); - expect(editor.state.selection.$anchor.parent.textContent).toBe('[X] done title'); + const cases = [ + ['title', '[ ] title'], + ['checked-title', '[x] checked title'], + ['x-checked-title', '[x] checked title'], + ['done-title', '[X] done title'], + ['x-done-title', '[X] done title'], + ] as const; + + for (const [hash, expectedHeading] of cases) { + expect(jumpToHash(hash)).toBe(true); + expect(editor.state.selection.$anchor.parent.textContent).toBe(expectedHeading); + } });As per coding guidelines: "Avoid duplicating code in tests; use
test.eachor shared helpers instead of repeating similar test blocks".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editor/ProseMirror/commands/commands.test.ts` around lines 86 - 107, The test 'jumpToHash should resolve canonical and legacy hashes for task-marker headings' repeats the same assertion pattern; refactor it to a table-driven test using test.each (or a simple loop) to supply pairs of hash and expected heading text, keep the editor setup via createTestEditor and the jumpToHash helper that calls commands[EditorCommandType.JumpToHash], and for each row assert the command returns true and that editor.state.selection.$anchor.parent.textContent equals the expected string (e.g. '[ ] title', '[x] checked title', '[X] done title').
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/editor/ProseMirror/utils/forEachHeading.ts`:
- Around line 21-37: The duplicate-hash bug comes from using two separate sets
(seenCanonicalHashes and seenLegacyHashes) so a legacy hash can collide with
another node's canonical hash; replace both with a single shared Set (e.g.,
seenHashes) and use that same set when calling uniqueHash for canonicalHash and
legacyHash inside the doc.descendants callback (in forEachHeading.ts), ensuring
you still push legacyHash only if it differs from canonicalHash but that both
aliases are reserved in the shared set.
In `@packages/renderer/MdToHtml.ts`:
- Around line 652-662: The legacy anchor generation uses
uslug(headingTextToken.content) directly which diverges from the canonical ID
logic; update the legacy alias generation to use
uslug(normalizeHeadingForHash(headingText)) (i.e. call normalizeHeadingForHash
on headingTextToken.content before uslug) and add the same deduplication
mechanism used in forEachHeading.ts to ensure uniqueness (check existing
IDs/counters and append suffixes as needed) so the produced legacyHeadingId both
matches canonical normalization and avoids duplicate anchors; adjust references
around canonicalHeadingId, headingTextToken and the joplin-heading-legacy-anchor
output accordingly.
---
Nitpick comments:
In `@packages/editor/ProseMirror/commands/commands.test.ts`:
- Around line 86-107: The test 'jumpToHash should resolve canonical and legacy
hashes for task-marker headings' repeats the same assertion pattern; refactor it
to a table-driven test using test.each (or a simple loop) to supply pairs of
hash and expected heading text, keep the editor setup via createTestEditor and
the jumpToHash helper that calls commands[EditorCommandType.JumpToHash], and for
each row assert the command returns true and that
editor.state.selection.$anchor.parent.textContent equals the expected string
(e.g. '[ ] title', '[x] checked title', '[X] done title').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8cb65b1a-264a-4bd0-b82c-3c68c3111626
📒 Files selected for processing (10)
packages/editor/CodeMirror/editorCommands/jumpToHash.test.tspackages/editor/CodeMirror/editorCommands/jumpToHash.tspackages/editor/CodeMirror/extensions/links/utils/findLineMatchingLink.test.tspackages/editor/CodeMirror/extensions/links/utils/findLineMatchingLink.tspackages/editor/ProseMirror/commands/commands.test.tspackages/editor/ProseMirror/utils/forEachHeading.tspackages/editor/ProseMirror/utils/jumpToHash.tspackages/editor/utils/normalizeHeadingForHash.tspackages/renderer/MdToHtml.test.tspackages/renderer/MdToHtml.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/editor/ProseMirror/utils/forEachHeading.ts (1)
15-25: DuplicateduniqueHashfunction.This function is duplicated in
packages/renderer/MdToHtml.ts(lines 91-101). Consider extracting to a shared utility (e.g.,packages/editor/utils/uniqueHash.tsor a package both can depend on).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/editor/ProseMirror/utils/forEachHeading.ts` around lines 15 - 25, The duplicated uniqueHash function (uniqueHash(baseHash: string, seenHashes: Set<string>)) should be extracted into a shared utility module and both callers updated to import it; create a new utility (e.g., uniqueHash in a shared utils package or packages/editor/utils/uniqueHash.ts), move the implementation there (preserving behavior with seenHashes parameter and return), then replace the local definitions in forEachHeading.ts and MdToHtml.ts with imports of the shared uniqueHash and remove the duplicated code from both files.packages/renderer/MdToHtml.ts (3)
654-654: UseT[]array type syntax.ESLint flags the use of
Array<T>syntax. Per project conventions, useT[]instead.🔧 Suggested fix
- const initializeSeenHeadingHashes = (tokens: Array<{ type: string; attrGet: (attrName: string)=> string|null }>) => { + const initializeSeenHeadingHashes = (tokens: { type: string; attrGet: (_attrName: string)=> string|null }[]) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/MdToHtml.ts` at line 654, The function signature for initializeSeenHeadingHashes uses the Array<T> syntax; change the parameter type from Array<{ type: string; attrGet: (attrName: string) => string | null }> to the bracket form ({ type: string; attrGet: (attrName: string) => string | null })[] so the parameter tokens in initializeSeenHeadingHashes uses T[] per project conventions; update only the type annotation for tokens to the bracket-style array type.
87-89: DuplicatednormalizeHeadingForHashimplementation.This function duplicates the logic in
packages/editor/utils/normalizeHeadingForHash.ts. Consider importing from that shared utility to maintain a single source of truth, or if the renderer package cannot depend on the editor package, extract this to a shared package (e.g.,@joplin/utils).As per coding guidelines: "When duplicating a substantial block of code, add a comment above it noting the duplication and referencing the original location."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/MdToHtml.ts` around lines 87 - 89, The function normalizeHeadingForHash in packages/renderer/MdToHtml.ts duplicates logic from packages/editor/utils/normalizeHeadingForHash.ts; replace the duplicate by importing and using the shared normalizeHeadingForHash utility (or move the implementation to a shared package like `@joplin/utils` and import it) so there is a single source of truth; if the renderer cannot depend on the editor package immediately, add a clear comment above the local normalizeHeadingForHash noting the duplication and referencing the original file path (packages/editor/utils/normalizeHeadingForHash.ts) and create a follow-up TODO to extract it to a shared module.
91-101: DuplicateduniqueHashimplementation.This function is identical to the one in
packages/editor/ProseMirror/utils/forEachHeading.ts(lines 15-25). Consider extracting it to a shared utility module to avoid maintaining two copies.As per coding guidelines: "When duplicating a substantial block of code, add a comment above it noting the duplication and referencing the original location."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/renderer/MdToHtml.ts` around lines 91 - 101, The function uniqueHash in MdToHtml.ts is duplicated elsewhere; extract this logic into a single exported utility (e.g., a shared util module exposing uniqueHash), replace the local uniqueHash implementation in MdToHtml.ts and the other file with an import from that utility, and update references to call the imported uniqueHash; if you cannot extract now, add a comment above the duplicate implementation in MdToHtml.ts noting the duplication and referencing the other implementation and its function name so future maintainers can consolidate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/editor/ProseMirror/utils/forEachHeading.ts`:
- Around line 15-25: The duplicated uniqueHash function (uniqueHash(baseHash:
string, seenHashes: Set<string>)) should be extracted into a shared utility
module and both callers updated to import it; create a new utility (e.g.,
uniqueHash in a shared utils package or packages/editor/utils/uniqueHash.ts),
move the implementation there (preserving behavior with seenHashes parameter and
return), then replace the local definitions in forEachHeading.ts and MdToHtml.ts
with imports of the shared uniqueHash and remove the duplicated code from both
files.
In `@packages/renderer/MdToHtml.ts`:
- Line 654: The function signature for initializeSeenHeadingHashes uses the
Array<T> syntax; change the parameter type from Array<{ type: string; attrGet:
(attrName: string) => string | null }> to the bracket form ({ type: string;
attrGet: (attrName: string) => string | null })[] so the parameter tokens in
initializeSeenHeadingHashes uses T[] per project conventions; update only the
type annotation for tokens to the bracket-style array type.
- Around line 87-89: The function normalizeHeadingForHash in
packages/renderer/MdToHtml.ts duplicates logic from
packages/editor/utils/normalizeHeadingForHash.ts; replace the duplicate by
importing and using the shared normalizeHeadingForHash utility (or move the
implementation to a shared package like `@joplin/utils` and import it) so there is
a single source of truth; if the renderer cannot depend on the editor package
immediately, add a clear comment above the local normalizeHeadingForHash noting
the duplication and referencing the original file path
(packages/editor/utils/normalizeHeadingForHash.ts) and create a follow-up TODO
to extract it to a shared module.
- Around line 91-101: The function uniqueHash in MdToHtml.ts is duplicated
elsewhere; extract this logic into a single exported utility (e.g., a shared
util module exposing uniqueHash), replace the local uniqueHash implementation in
MdToHtml.ts and the other file with an import from that utility, and update
references to call the imported uniqueHash; if you cannot extract now, add a
comment above the duplicate implementation in MdToHtml.ts noting the duplication
and referencing the other implementation and its function name so future
maintainers can consolidate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e19592c3-34fe-42aa-89b8-9cc9b2dc618d
📒 Files selected for processing (4)
packages/editor/ProseMirror/commands/commands.test.tspackages/editor/ProseMirror/utils/forEachHeading.tspackages/renderer/MdToHtml.test.tspackages/renderer/MdToHtml.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/ProseMirror/commands/commands.test.ts
- packages/renderer/MdToHtml.test.ts
Owner
|
Thanks for the contribution. We’re closing this PR because it does not follow the project’s contribution policy: it is not based on a triaged, accepted issue and does not address a concrete problem agreed upon by maintainers. While the change itself is not necessarily incorrect, reviewing and maintaining such PRs takes time away from higher-priority work. Please see our Contribution Scope Policy for details on what kinds of contributions we accept. If you intend to participate in GSoC, please make sure you read our GSoC guidelines. |
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
2 participants
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.
Summary
This PR fixes heading hash instability for task-marker headings so internal links remain valid when the checkbox state changes.
Problem
Headings such as:
## [ ] titleand:
## [x] titlepreviously produced different hashes. As a result, internal links could break after toggling the checkbox state in the heading.
Root cause
Heading hash generation and hash matching used raw heading text, so leading task markers affected the computed slug. Existing links using older raw-hash forms were also not consistently resolved after checkbox state changes.
What changed
x-titleWhy this is safe
Testing
Added/updated tests for:
[ ],[x], and[X]x-...hash compatibilityRan:
yarn workspace @joplin/renderer test --runInBand packages/renderer/MdToHtml.test.tsyarn workspace @joplin/editor test --runInBand findLineMatchingLink.test.ts jumpToHash.test.ts commands.test.tsyarn workspace @joplin/editor tsc --noEmityarn workspace @joplin/renderer tsc --noEmitFixes #15057