Add workaround for CSS HEX colors in markdown. #865
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Markdown processor to avoid converting CSS hex color codes into issue links and adds tests and CI adjustments to validate that behavior.
- Updates
markdownWithRepositoryContextto detect and skip CSS hex colors when linking issue references. - Introduces extensive Vitest tests to cover hex-color vs. issue-number scenarios.
- Adds a Vitest configuration file and refines the GitHub Actions workflow to run tests.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vitest.config.ts | New Vitest config for running tests in Node environment |
| tests/markdown.test.ts | Test suite covering issue linkification and CSS hex skips |
| src/tools/markdown.ts | Regex and callback logic updated to avoid CSS hex matches |
| .github/workflows/test.yml | Renamed workflow, added a dedicated test job |
Comments suppressed due to low confidence (3)
src/tools/markdown.ts:34
- The comment mentions skipping code blocks, but the implementation only checks CSS contexts. Consider either implementing code-block detection (e.g., ignore content within ``` fences) or updating the comment to accurately reflect current behavior.
// Add references to issues and PRs (avoid CSS hex colors and code blocks)
tests/markdown.test.ts:19
- We should add a test case for uppercase hex codes (e.g.,
#ABCDEF) to ensure that the CSS skip logic handles both lowercase and uppercase hex values as intended.
it("should NOT convert CSS color codes", () => {
.github/workflows/test.yml:43
- [nitpick] The
build_frontendjob is named "Test build", which can be misleading. Consider renaming it to something like "Build Frontend" to accurately reflect its purpose.
name: Test build
📝 Walkthrough""" WalkthroughThe changes introduce a new test job to the GitHub Actions workflow, enhance the markdown issue reference conversion to avoid mangling CSS hex color codes, add comprehensive tests for this logic, and include a Vitest configuration for running the tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant Node.js
participant Vitest
Developer->>GitHub Actions: Push code / PR
GitHub Actions->>Node.js: Set up Node.js 18
GitHub Actions->>Node.js: Install dependencies (script/bootstrap)
GitHub Actions->>Vitest: Run tests (npx vitest run)
Vitest-->>GitHub Actions: Test results
GitHub Actions-->>Developer: Report build and test status
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/tools/markdown.ts (1)
44-44: Consider expanding CSS context detection or making it more specific.The current fallback pattern
:\s*$might be too broad and could incorrectly skip valid issue references in non-CSS contexts (e.g., "Note: #123 is important").Consider a more comprehensive CSS property list or a more specific pattern:
- if (/(?:color|background|border):\s*$/.test(beforeMatch) || /:\s*$/.test(beforeMatch)) { + if (/(?:color|background|border|fill|stroke|outline|box-shadow|text-shadow):\s*$/.test(beforeMatch)) {Or use a more specific pattern that looks for CSS-like contexts:
- if (/(?:color|background|border):\s*$/.test(beforeMatch) || /:\s*$/.test(beforeMatch)) { + if (/(?:color|background|border):\s*$/.test(beforeMatch) || /[a-z-]+:\s*$/.test(beforeMatch)) {tests/markdown.test.ts (2)
5-10: Consider completing the mock repository object.The mock repository uses
as RepositoryInfotype assertion, which might hide missing required fields. Consider providing all required fields or using a more type-safe approach.const mockRepository: RepositoryInfo = { id: "123", full_name: "awesome/repo", default_branch: "main", available_version: "v1.0.0", + additional_info: "", + hide_default_branch: false, + issues: 0, + releases: [], + ref: "main", + selected_tag: null, + version_or_commit: "version", + // ... other required fields from RepositoryInfo -} as RepositoryInfo; +};
96-118: Consider reducing the performance test scope.The performance test with 25,000 iterations might be excessive for unit tests and could slow down the CI pipeline. Consider reducing the number or moving it to a separate performance test suite.
- for (let i = 0; i < 25_000; i++) { + for (let i = 0; i < 1_000; i++) { parts.push(`Issue #${i + 1} was fixed. Use color:#${String(i).padStart(3, "0")}; in CSS.`); }Or create a separate performance test:
- it("should handle large text efficiently", () => { + it.skip("should handle large text efficiently", () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
.github/workflows/test.yml(2 hunks)src/tools/markdown.ts(1 hunks)tests/markdown.test.ts(1 hunks)vitest.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/markdown.test.ts (2)
src/data/repository.ts (1)
RepositoryInfo(41-50)src/tools/markdown.ts (1)
markdownWithRepositoryContext(6-60)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests
- GitHub Check: Test build
🔇 Additional comments (4)
vitest.config.ts (1)
1-8: LGTM! Clean and appropriate Vitest configuration.The configuration is well-structured with:
- Correct environment setting for Node.js
- Appropriate test file inclusion pattern
- Minimal and focused setup
.github/workflows/test.yml (2)
1-1: LGTM! Good workflow naming update.The name change from "Test build" to "Test and Build" better reflects the workflow's purpose.
21-40: LGTM! Well-structured test job.The test job configuration follows GitHub Actions best practices:
- Uses latest Ubuntu runner
- Proper Node.js version pinning
- Yarn caching for performance
- Standard dependency installation and test execution
tests/markdown.test.ts (1)
12-119: LGTM! Comprehensive test coverage.The test suite covers the main scenarios well:
- Basic issue reference conversion
- CSS hex color exclusion
- Mixed content handling
- Cross-repository references
- Edge cases with non-hex digits
The tests demonstrate the intended functionality effectively.
1 participant
Closes hacs/integration#4688