Conversation
📝 WalkthroughWalkthroughThis PR adds a new Sequence DiagramsequenceDiagram
participant User
participant ChangelogCommand
participant ChangelogRenderingService
participant RenderContext as ChangelogRenderContext
participant MarkdownRenderers
participant Output
User->>ChangelogCommand: Execute CLI with --dropdowns flag
ChangelogCommand->>ChangelogCommand: Parse dropdowns parameter
ChangelogCommand->>ChangelogRenderingService: RenderChangelogsArguments(dropdowns=true/false)
ChangelogRenderingService->>ChangelogRenderingService: SetupOutput() processes title/slug
ChangelogRenderingService->>RenderContext: Initialize with Dropdowns property
alt Dropdowns = true
ChangelogRenderingService->>MarkdownRenderers: Render each entry type
MarkdownRenderers->>MarkdownRenderers: Check context.Dropdowns
MarkdownRenderers->>MarkdownRenderers: Render as dropdown blocks
MarkdownRenderers->>Output: MyST dropdown syntax
else Dropdowns = false
ChangelogRenderingService->>MarkdownRenderers: Render each entry type
MarkdownRenderers->>MarkdownRenderers: Check context.Dropdowns
MarkdownRenderers->>MarkdownRenderers: Render as flattened bullets
MarkdownRenderers->>Output: Flattened Markdown format
end
Output->>User: Rendered changelog (dropdown or flat)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/Elastic.Changelog/Rendering/Markdown/MarkdownRendererBase.cs (1)
42-48: ⚡ Quick winConsider folding the parameters into a small record.
Both overloads now exceed the 4-parameter cap, and call sites pass
entryHideLinkspositionally next to a second boolindentForListItem. A smallPrIssueLinkOptions(ChangelogEntry Entry, string Repo, string Owner, bool HideLinks, bool IndentForListItem)would collapse this toRenderPrIssueLinks(sb, options)and remove the bool-adjacency at call sites.As per coding guidelines: "Method parameters should not exceed 4 in count. Use a record or options object for methods that require more than 4 parameters. Boolean parameters must be named at call sites."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/Elastic.Changelog/Rendering/Markdown/MarkdownRendererBase.cs` around lines 42 - 48, The RenderPrIssueLinks overloads exceed the 4-parameter guideline and pass two adjacent bools positionally; create a small record (e.g., PrIssueLinkOptions with properties ChangelogEntry Entry, string Repo, string Owner, bool HideLinks, bool IndentForListItem) and change both method signatures to RenderPrIssueLinks(StringBuilder sb, PrIssueLinkOptions options); update internal references inside MarkdownRendererBase to use options.* and update all call sites to pass a named PrIssueLinkOptions instance (ensuring bools are named via properties) so callers become RenderPrIssueLinks(sb, new PrIssueLinkOptions { Entry = entry, Repo = entryRepo, Owner = entryOwner, HideLinks = entryHideLinks, IndentForListItem = indentForListItem }); ensure overload forwarding is removed or adjusted accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Elastic.Changelog.Tests/Changelogs/Render/DropdownRenderTests.cs`:
- Around line 419-422: The test asserts AsciiDoc uses unconstrained strong
markers (“**Impact:**”, “**Action:**”) which is non-idiomatic; update the
DropdownRenderTests.cs assertions to expect idiomatic constrained strong markers
("*Impact:*", "*Action:*") or make the assertions tolerant (e.g., regex or
alternatives) to accept either single- or double-asterisk forms so the test
stays valid if AsciidocRendererBase.cs (where the renderer currently emits
"**Impact:**" / "**Action:**" around lines 91 and 97) is changed to emit the
idiomatic single-asterisk format.
---
Nitpick comments:
In `@src/services/Elastic.Changelog/Rendering/Markdown/MarkdownRendererBase.cs`:
- Around line 42-48: The RenderPrIssueLinks overloads exceed the 4-parameter
guideline and pass two adjacent bools positionally; create a small record (e.g.,
PrIssueLinkOptions with properties ChangelogEntry Entry, string Repo, string
Owner, bool HideLinks, bool IndentForListItem) and change both method signatures
to RenderPrIssueLinks(StringBuilder sb, PrIssueLinkOptions options); update
internal references inside MarkdownRendererBase to use options.* and update all
call sites to pass a named PrIssueLinkOptions instance (ensuring bools are named
via properties) so callers become RenderPrIssueLinks(sb, new PrIssueLinkOptions
{ Entry = entry, Repo = entryRepo, Owner = entryOwner, HideLinks =
entryHideLinks, IndentForListItem = indentForListItem }); ensure overload
forwarding is removed or adjusted accordingly.
🪄 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: Enterprise
Run ID: 0d9d1f38-3e96-48e5-a1c5-c0755a97fcaa
📒 Files selected for processing (11)
docs/cli/changelog/render.mdsrc/services/Elastic.Changelog/Rendering/ChangelogRenderContext.cssrc/services/Elastic.Changelog/Rendering/ChangelogRenderingService.cssrc/services/Elastic.Changelog/Rendering/Markdown/BreakingChangesMarkdownRenderer.cssrc/services/Elastic.Changelog/Rendering/Markdown/DeprecationsMarkdownRenderer.cssrc/services/Elastic.Changelog/Rendering/Markdown/HighlightsMarkdownRenderer.cssrc/services/Elastic.Changelog/Rendering/Markdown/KnownIssuesMarkdownRenderer.cssrc/services/Elastic.Changelog/Rendering/Markdown/MarkdownRendererBase.cssrc/tooling/docs-builder/Commands/ChangelogCommand.cstests/Elastic.Changelog.Tests/Changelogs/Render/DropdownRenderTests.cstests/Elastic.Changelog.Tests/Changelogs/Render/TitleTargetTests.cs
| // AsciiDoc should always use bullet format regardless of dropdowns flag | ||
| content.Should().Contain("* Breaking API change"); | ||
| content.Should().Contain("**Impact:** Existing API calls will fail"); | ||
| content.Should().Contain("**Action:** Update your code"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the AsciiDoc renderer for breaking changes
rg -n --type=cs -C5 "Impact|Action" --iglob "*Asciidoc*"
rg -n --type=cs -C5 "Impact|Action" --iglob "*AsciiDoc*"
rg -n --type=cs -C5 "\*\*Impact\|\\*Impact"Repository: elastic/docs-builder
Length of output: 21526
The test assertions are correct but validate non-idiomatic AsciiDoc formatting.
The renderer at AsciidocRendererBase.cs lines 91 and 97 deliberately emits **Impact:** and **Action:** (double asterisks). The test correctly verifies this output. However, AsciiDoc best practice for standalone words/phrases with surrounding space is constrained strong (*Impact:* with single asterisks), not unconstrained (**Impact:**). The test assertions would need updating if the renderer is fixed to use idiomatic AsciiDoc formatting, but the test logic itself is sound.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/Elastic.Changelog.Tests/Changelogs/Render/DropdownRenderTests.cs`
around lines 419 - 422, The test asserts AsciiDoc uses unconstrained strong
markers (“**Impact:**”, “**Action:**”) which is non-idiomatic; update the
DropdownRenderTests.cs assertions to expect idiomatic constrained strong markers
("*Impact:*", "*Action:*") or make the assertions tolerant (e.g., regex or
alternatives) to accept either single- or double-asterisk forms so the test
stays valid if AsciidocRendererBase.cs (where the renderer currently emits
"**Impact:**" / "**Action:**" around lines 91 and 97) is changed to emit the
idiomatic single-asterisk format.
There was a problem hiding this comment.
I am implementing the Asciidoc-specific fixes in a subsequent PR.
There was a problem hiding this comment.
@lcawl Understood, thanks for the heads-up. I'll keep an eye out for the follow-up PR addressing the AsciiDoc renderer formatting.
( - ω -)～♪
There was a problem hiding this comment.
@lcawl Thanks for the link! I'll pick up the review of the AsciiDoc formatting fixes in #3262.
1 participant
This PR relates to #3228
In particular, it adds the ability to turn off dropdowns in the "changelog render" output, same as in the changelog directive.
Summary
Users can now use
--dropdownsto render separated types (breaking changes, deprecations, known issues, highlights) as MyST dropdowns, or omit the flag for the new default flattened bulleted list format.Reviewer notes
AI implementation details
All functionality has been successfully implemented according to the plan:
🔹 CLI Command: Added
--dropdownsparameter withfalsedefault🔹 Service Layer: Extended with
Dropdownsproperty throughout the rendering pipeline🔹 All Renderers: Updated to support both dropdown and flattened modes
🔹 Documentation: Added comprehensive CLI documentation
🔹 Tests: Created full test coverage for both rendering modes
There was also an issue with the way content was rendered in the non-dropdown mode when the bulleted item spanned multiple paragraphs. The output is now fixed to work as described in https://elastic.github.io/docs-builder/syntax/lists/#content-within-a-list-item:
Before (broken):
After (fixed):
Tests
I tested with some made-up changelogs in the kibana repo as follows:
--dropdownsoption, for example:--dropdownscommand option. Preview and verify that it's properly formatted:Generative AI disclosure
Tool(s) and model(s) used: composer-2, claude-4-sonnet-thinking