Conversation
BlackYps
added
the
area: changelog management
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (293)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds documentation and site plumbing for changelog snippet creation and rendering: a GitHub Actions workflow that creates a snippet file from a template and marks the PR as draft, new snippet template, Jekyll plugins and styles for unit-based changelog rendering, dependency additions, and updated changelog docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Actions Runner
participant Repo as Repository (git working tree)
participant GH as GitHub (gh CLI / API)
Note over Runner,Repo: Workflow starts on PR event
Runner->>Repo: git config user.email/user.name
Runner->>Repo: copy template-snippet.md -> changelog/snippets/category.<PR>.md (replace XYZW with PR)
Runner->>Repo: git add & git commit ("Add snippet template")
Runner->>GH: gh pr ready <PR> --undo
GH-->>Runner: PR state updated to Draft (undo ready)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
docs/Gemfile (1)
9-11: Avoid pinning default Ruby gems unless runtime is explicitly pinned.
csvandbase64are default gems tied closely to Ruby runtime versions. Pinning them here without an explicit Ruby version for the docs toolchain can cause Bundler resolution mismatches across local/CI environments. Prefer relying on default gems, or add a strictrubyversion and validate lockfile consistency.#!/bin/bash # Read-only verification for Ruby/default-gem pinning risk in docs toolchain. set -euo pipefail echo "== Find Ruby version constraints ==" rg -n --iglob 'Gemfile*' --iglob '*.gemspec' --iglob '.ruby-version' '\bruby\b|^ruby-version|^RUBY_VERSION' echo echo "== Inspect docs Gemfile/Gemfile.lock entries for csv/base64 ==" fd -i 'Gemfile.lock|Gemfile' docs . | xargs -r rg -n '^\s{0,4}(csv|base64)\b|gem\s+"(csv|base64)"' echo echo "== Locate actual requires/usages in docs-related Ruby code ==" rg -n --type ruby -C2 'require\s+["'\''](csv|base64)["'\'']|CSV\b|Base64\b' docs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/Gemfile` around lines 9 - 11, The Gemfile currently pins default Ruby stdlib gems with lines gem "csv", "~> 3.3" and gem "base64", "~> 0.3.0", which can cause Bundler resolution mismatches; either remove these gem entries so the toolchain uses the runtime-provided stdlib versions, or instead add an explicit ruby version constraint (ruby "x.y.z") and update/commit a matching Gemfile.lock so the pinned versions are intentional and reproducible—change the Gemfile by deleting the gem "csv" and gem "base64" lines or add a strict ruby declaration and regenerate the lockfile, referencing the existing Gemfile entries (gem "csv"/gem "base64") to locate the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/auto-draft-pr.yaml:
- Around line 34-47: The workflow step that creates and commits a snippet is
missing a repository checkout, a push of the new commit, and uses an
unrecognized filename pattern; to fix it add an initial "actions/checkout@vX"
step before changing into fa/changelog/snippets, update FILE from category.${{
github.event.pull_request.number }}.md to one of the accepted patterns (e.g.
other.${{ github.event.pull_request.number }}.md or features.${{
github.event.pull_request.number }}.md) so the bundler will pick it up, and
after git commit run git push to push the new branch/commit; ensure the existing
commands that reference FILE, cp sections/template-snippet.md, sed -i
"s/XYZW/${{ github.event.pull_request.number }}/g" $FILE, git add ., and git
commit -m "Add snippet template" remain but run only after checkout and before
the git push.
In `@docs/_plugins/balance_change.rb`:
- Around line 8-10: The current regex in doc.output.gsub! (the
/([^<\s]+)\s*-->\s*([^<\s]+)/) only captures single tokens (no spaces or
parentheses), so multi-token values like "60 (225) --> 45 (225)" are missed;
update the pattern to allow any non-HTML content around the arrow (e.g. use lazy
captures like /([^<]+?)\s*-->\s*([^<]+?)/ or similar) and trim captured
groups before inserting them into the replacement span so leading/trailing
whitespace isn't preserved; keep the replacement logic in the same gsub! call
but switch to the broader capture groups (referencing doc.output.gsub! and the
regex literal) and ensure you still escape HTML-sensitive chars if needed.
In `@docs/_plugins/unit_block.rb`:
- Around line 13-16: The template injects raw `@unit_id` and name into HTML;
update the UnitBlock rendering to validate `@unit_id` against a strict
whitelist/regex (e.g., only [A-Za-z0-9_-]) and reject or normalize unexpected
values, and escape any user-provided text using an HTML-escaping helper (e.g.,
CGI.escapeHTML or Jekyll::Utils.escape_html) when outputting name and when
composing the img src attribute; locate the renderer method that outputs the div
with class "unit-header" (the code using `@unit_id` and name) and apply these
validations/escapes so the src="/assets/icons/#{`@unit_id`}.png" and the span
content use safe, escaped values.
In `@docs/development-changelog/balance-snippet.md`:
- Line 19: Add a fenced language specifier to the code block starting at the
opening ``` on line 19 by changing it to ```md, and fix the heading level by
replacing the "### Example snippets" heading with "## Example snippets" (update
the heading text referenced as "### Example snippets" to "## Example snippets")
so markdownlint no longer reports a missing fence language and a skipped heading
level.
- Around line 9-14: Remove the draft/internal notes lines in the balance-snippet
content (the brainstorming lines about "Unit or explanation first?", "The unitID
is also used...", etc.) so the page reads as polished guidance; also ensure the
md->lua converter strips the inline marker "{: .unit-change}" and removes the
trailing "(unitID)" tokens (including cases like "enhancements/faction/name")
from rendered output so no draft markers or raw unit IDs remain in published
docs.
In `@docs/development-changelog/other-snippet.md`:
- Around line 12-13: The markdown violates lint rules by skipping a heading
level and omitting a fence language: change the "### Example snippet" heading to
"## Example snippet" and add the language tag "md" to the opening code fence for
the example (the fenced block that contains the LOD formula line) so it reads as
a proper fenced code block with a declared language.
---
Nitpick comments:
In `@docs/Gemfile`:
- Around line 9-11: The Gemfile currently pins default Ruby stdlib gems with
lines gem "csv", "~> 3.3" and gem "base64", "~> 0.3.0", which can cause Bundler
resolution mismatches; either remove these gem entries so the toolchain uses the
runtime-provided stdlib versions, or instead add an explicit ruby version
constraint (ruby "x.y.z") and update/commit a matching Gemfile.lock so the
pinned versions are intentional and reproducible—change the Gemfile by deleting
the gem "csv" and gem "base64" lines or add a strict ruby declaration and
regenerate the lockfile, referencing the existing Gemfile entries (gem "csv"/gem
"base64") to locate the edits.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 700c6765-d17f-4090-9a14-9a88974a38aa
⛔ Files ignored due to path filters (2)
docs/Gemfile.lockis excluded by!**/*.lockdocs/assets/icons/XES0102.pngis excluded by!**/*.png
📒 Files selected for processing (10)
.github/workflows/auto-draft-pr.yamlchangelog/snippets/sections/template-snippet.mddocs/Gemfiledocs/_plugins/balance_change.rbdocs/_plugins/unit_block.rbdocs/_sass/custom/custom.scssdocs/development-changelog.mddocs/development-changelog/balance-snippet.mddocs/development-changelog/other-snippet.mddocs/documentation.md
Comment on lines
+34
to
+47
| - name: Add snippet | ||
| working-directory: fa/changelog/snippets | ||
| run: | | ||
| # Configure git | ||
| git config user.email "github@faforever.com" | ||
| git config user.name "FAForever Machine User" | ||
|
|
||
| FILE=category.${{ github.event.pull_request.number }}.md | ||
| cp sections/template-snippet.md $FILE | ||
| sed -i "s/XYZW/${{ github.event.pull_request.number }}/g" $FILE | ||
|
|
||
| git add . | ||
| git commit -m "Add snippet template" | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow signals:"
rg -n 'actions/checkout|working-directory|FILE=|git commit|git push' .github/workflows/auto-draft-pr.yaml
echo
echo "Bundler accepted snippet categories:"
rg -n 'process_snippets "' .github/workflows/scripts/bash/changelog-combine.shRepository: FAForever/fa
Length of output: 693
🏁 Script executed:
cat -n .github/workflows/auto-draft-pr.yaml | head -50Repository: FAForever/fa
Length of output: 2288
Workflow will fail: missing checkout, no push, and invalid filename pattern.
The job is missing a checkout step (line 34 tries to access fa/changelog/snippets without cloning the repo first), never pushes the commit (line 46), and uses category.<PR>.md which the bundler doesn't recognize—it only processes balance.*.md, features.*.md, fix.*.md, graphics.*.md, ai.*.md, performance.*.md, or other.*.md.
Suggested fix
+ - name: Checkout PR branch
+ uses: actions/checkout@v4
+ with:
+ ref: ${{ github.event.pull_request.head.ref }}
- name: Add snippet
working-directory: fa/changelog/snippets
run: |
# Configure git
git config user.email "github@faforever.com"
git config user.name "FAForever Machine User"
- FILE=category.${{ github.event.pull_request.number }}.md
+ FILE=other.${{ github.event.pull_request.number }}.md
cp sections/template-snippet.md $FILE
sed -i "s/XYZW/${{ github.event.pull_request.number }}/g" $FILE
- git add .
- git commit -m "Add snippet template"
+ git add "$FILE"
+ git commit -m "Add snippet template" || true
+ git push🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/auto-draft-pr.yaml around lines 34 - 47, The workflow step
that creates and commits a snippet is missing a repository checkout, a push of
the new commit, and uses an unrecognized filename pattern; to fix it add an
initial "actions/checkout@vX" step before changing into fa/changelog/snippets,
update FILE from category.${{ github.event.pull_request.number }}.md to one of
the accepted patterns (e.g. other.${{ github.event.pull_request.number }}.md or
features.${{ github.event.pull_request.number }}.md) so the bundler will pick it
up, and after git commit run git push to push the new branch/commit; ensure the
existing commands that reference FILE, cp sections/template-snippet.md, sed -i
"s/XYZW/${{ github.event.pull_request.number }}/g" $FILE, git add ., and git
commit -m "Add snippet template" remain but run only after checkout and before
the git push.
Comment on lines
+8
to
+10
| doc.output.gsub!( | ||
| /([^<\s]+)\s*-->\s*([^<\s]+)/, | ||
| '<span class="old">\1</span> → <span class="new">\2</span>' |
Contributor
There was a problem hiding this comment.
Regex misses common value formats with spaces/parentheses.
This pattern only matches single tokens, so examples like 60 (225) --> 45 (225) won’t be transformed.
🔧 Suggested patch
- /([^<\s]+)\s*-->\s*([^<\s]+)/,
+ /([^<]+?)\s*-->\s*([^<]+?)(?=\s*(?:<|$))/,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| doc.output.gsub!( | |
| /([^<\s]+)\s*-->\s*([^<\s]+)/, | |
| '<span class="old">\1</span> → <span class="new">\2</span>' | |
| doc.output.gsub!( | |
| /([^<]+?)\s*-->\s*([^<]+?)(?=\s*(?:<|$))/, | |
| '<span class="old">\1</span> → <span class="new">\2</span>' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/_plugins/balance_change.rb` around lines 8 - 10, The current regex in
doc.output.gsub! (the /([^<\s]+)\s*-->\s*([^<\s]+)/) only captures single
tokens (no spaces or parentheses), so multi-token values like "60 (225) --> 45
(225)" are missed; update the pattern to allow any non-HTML content around the
arrow (e.g. use lazy captures like /([^<]+?)\s*-->\s*([^<]+?)/ or similar)
and trim captured groups before inserting them into the replacement span so
leading/trailing whitespace isn't preserved; keep the replacement logic in the
same gsub! call but switch to the broader capture groups (referencing
doc.output.gsub! and the regex literal) and ensure you still escape
HTML-sensitive chars if needed.
Comment on lines
+13
to
+16
| <div class="unit-header" data-unit="#{@unit_id}"> | ||
| <img class="unit-icon" | ||
| src="/assets/icons/#{@unit_id}.png"> | ||
| <span class="unit-name">#{name}</span> |
Contributor
There was a problem hiding this comment.
Escape and validate interpolated HTML values.
@unit_id and name are injected raw into HTML. This should be escaped (and unit_id validated) before rendering.
🛡️ Suggested patch
+require "cgi"
+
module Jekyll
class UnitBlock < Liquid::Block
@@
def render(context)
- name = super.strip
+ name = CGI.escapeHTML(super.strip)
+ unit_id = `@unit_id.strip`
+ raise ArgumentError, "Invalid unit id" unless unit_id.match?(/\A[a-zA-Z0-9_\/-]+\z/)
+ safe_unit_id = CGI.escapeHTML(unit_id)
<<~HTML
- <div class="unit-header" data-unit="#{`@unit_id`}">
+ <div class="unit-header" data-unit="#{safe_unit_id}">
<img class="unit-icon"
- src="/assets/icons/#{`@unit_id`}.png">
+ src="/assets/icons/#{safe_unit_id}.png"
+ alt="">
<span class="unit-name">#{name}</span>
</div>
HTML🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/_plugins/unit_block.rb` around lines 13 - 16, The template injects raw
`@unit_id` and name into HTML; update the UnitBlock rendering to validate `@unit_id`
against a strict whitelist/regex (e.g., only [A-Za-z0-9_-]) and reject or
normalize unexpected values, and escape any user-provided text using an
HTML-escaping helper (e.g., CGI.escapeHTML or Jekyll::Utils.escape_html) when
outputting name and when composing the img src attribute; locate the renderer
method that outputs the div with class "unit-header" (the code using `@unit_id`
and name) and apply these validations/escapes so the
src="/assets/icons/#{`@unit_id`}.png" and the span content use safe, escaped
values.
Comment on lines
+9
to
+14
| The game just renders the text as is, so we need to keep that in mind. | ||
| Unit or explanation first? | ||
| The unitID is also used to infer if the snippet is supposed to go into the land, navy, air, structures or enhancements category. | ||
| if the change is an enhancement, the unitID is enhancements/faction/name | ||
| The md to lua converter should remove the {: .unit-change} and the (unitID) | ||
|
|
Contributor
There was a problem hiding this comment.
Remove draft notes before publishing this doc page.
These lines read like internal brainstorming/TODO notes and conflict with the otherwise polished guidance.
🧾 Suggested cleanup
-The game just renders the text as is, so we need to keep that in mind.
-Unit or explanation first?
-The unitID is also used to infer if the snippet is supposed to go into the land, navy, air, structures or enhancements category.
-if the change is an enhancement, the unitID is enhancements/faction/name
-The md to lua converter should remove the {: .unit-change} and the (unitID)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The game just renders the text as is, so we need to keep that in mind. | |
| Unit or explanation first? | |
| The unitID is also used to infer if the snippet is supposed to go into the land, navy, air, structures or enhancements category. | |
| if the change is an enhancement, the unitID is enhancements/faction/name | |
| The md to lua converter should remove the {: .unit-change} and the (unitID) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development-changelog/balance-snippet.md` around lines 9 - 14, Remove
the draft/internal notes lines in the balance-snippet content (the brainstorming
lines about "Unit or explanation first?", "The unitID is also used...", etc.) so
the page reads as polished guidance; also ensure the md->lua converter strips
the inline marker "{: .unit-change}" and removes the trailing "(unitID)" tokens
(including cases like "enhancements/faction/name") from rendered output so no
draft markers or raw unit IDs remain in published docs.
|
|
||
| The snippet file should be named `balance.<PR Number>.md`. | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Fix markdownlint issues in examples section.
Line 19 should specify a fenced language, and Line 38 should not jump heading levels.
🧹 Suggested patch
-```
+```md
@@
-### Example snippets
+## Example snippetsAlso applies to: 38-38
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development-changelog/balance-snippet.md` at line 19, Add a fenced
language specifier to the code block starting at the opening ``` on line 19 by
changing it to ```md, and fix the heading level by replacing the "### Example
snippets" heading with "## Example snippets" (update the heading text referenced
as "### Example snippets" to "## Example snippets") so markdownlint no longer
reports a missing fence language and a skipped heading level.
Comment on lines
+12
to
+13
| ### Example snippet | ||
| ``` |
Contributor
There was a problem hiding this comment.
Fix markdownlint violations in the example section.
Line 12 should not skip heading levels, and Line 13 should declare a fence language.
🧹 Suggested patch
-### Example snippet
-```
+## Example snippet
+```md
- Tweak the formula used to calculate the level of detail (LOD) for props.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ### Example snippet | |
| ``` | |
| ## Example snippet |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 12-12: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development-changelog/other-snippet.md` around lines 12 - 13, The
markdown violates lint rules by skipping a heading level and omitting a fence
language: change the "### Example snippet" heading to "## Example snippet" and
add the language tag "md" to the opening code fence for the example (the fenced
block that contains the LOD formula line) so it reads as a proper fenced code
block with a declared language.
Member
I think it may look inconsistent visually when you put it last though. The idea of it being first was originally to emphasize the development that happens behind the scenes, to lure in possible contributors with knowledge of GitHub to know how to look it up. This is also why these numbers are made clickable by I think @relent0r at the time: 
This appears to be used actively, see also the 
The highlighted is likely a reference to https://faforever.github.io/fa/. I did eventually stop putting pull request or issue numbers behind the names of contributors, as was a little skewed and this new flow would not support that either way. I do very much like the richer visuals for balance changes 😄 ! |
Contributor
Author
|
I'm not arguing to get rid of the PR numbers or the links for that matter. But I don't think it is critical for luring in contributors that the link comes first. If we think that we should encourage new people more I would rather do it by adding something to the release text. |
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.
Description of the proposed changes
We should reconsider the snippet formats a bit. I think it makes more sense when the PR number comes last. It is not that important that it needs to come first.
Additionally. we can bump the quality of the changelogs on the website a lot by adding some processing. This requires a little additional syntax in the balance snippets, but it's not too complicated.
Yanky first results:
Testing done on the proposed changes
Additional context
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes