feat: added github action for Check for embedded CSS and the correct stylesheet in Typescript files #2907 #3041
Conversation
WalkthroughA new Python script Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
IITI-tushar
force-pushed
the
added-add.module.css-check-python-script
branch
from
66d0d88 to
acdd27a
Compare
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/css_check.py (2)
9-20: Expand Embedded CSS Detection Beyond Hex CodesRight now,
check_embedded_cssonly catches inline hex color usage (e.g., #FFF, #FFFFFF). If there are other forms of embedded styling (e.g., style attributes, inlined multi-line CSS, single-line style properties), they will remain undetected.You might consider broadening the regex pattern or introducing multiple detection rules to cover other CSS constructs inlined in TypeScript files.
23-74: Enhance CSS Import Handling & Validation
Currently, the check for valid CSS imports is limited to endings with
/app.module.css. You may want to allow for multiple valid module file names, for instanceapp.module.cssorstyles.module.css, based on your project guidelines or common naming conventions.Consider whether you need to detect imports for
.less,.scss, or.sassif the project supports them.If the repository might evolve to use or allow multiple .module.* variations, you can update the condition to allow them or make it configurable.
.github/workflows/pull-request.yml (1)
42-42: Remove Trailing WhitespaceThe lines added/modified at 42, 46, and 47 contain trailing spaces, as flagged by
yamllint. Removing trailing whitespace will help maintain a clean and consistent YAML file.Apply this diff:
42- 42+ 46- 46+ 47- 47+Also applies to: 46-46, 47-47
🧰 Tools
🪛 yamllint (1.35.1)
[error] 42-42: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/css_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
🔇 Additional comments (2)
.github/workflows/css_check.py (2)
1-2: File-level Documentation is Clear
Your module-level docstring concisely explains the purpose of the script. This is a great start for developers browsing the file.
77-116: Verify Edge Cases and Add Automated Tests
Your main() function, CLI argument parsing, and overall structure are concise. However:
- No mention of test coverage or unit tests for these checks.
- Edge case: Large directories, symlinks, or files with unusual text encoding might pose unexpected issues.
As a follow-up, consider adding a test suite or coverage checks to verify the script’s behavior under various scenarios (e.g., large file sets, unusual file encodings, different CSS naming conventions).
palisadoes
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
.github/workflows/css_check.py (2)
58-58: Make test file exclusion configurableThe hardcoded exclusion of test files reduces flexibility.
Consider making test file exclusion configurable through command-line arguments:
- if file.endswith((".ts", ".tsx")) and "test" not in root: + if file.endswith((".ts", ".tsx")) and not any(exclude in root for exclude in test_excludes):Add to the argument parser:
parser.add_argument( "--test_excludes", nargs="*", default=["test", "__tests__"], help="Patterns to identify test directories" )
99-117: Enhance error reporting and output formatThe current output format could be improved for better integration with CI tools.
Consider adding:
- JSON output format option
- More detailed error statistics
- Colored output for better readability
+ parser.add_argument( + "--output-format", + choices=["text", "json"], + default="text", + help="Output format (text or json)" + ) + if args.output_format == "json": + import json + result = { + "violations": violations, + "embedded_css": embedded_css_violations, + "correct_imports": correct_css_imports, + "statistics": { + "files_checked": files_checked, + "violations_count": len(violations), + "embedded_css_count": len(embedded_css_violations) + } + } + print(json.dumps(result, indent=2))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/css_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 42-42: trailing spaces
(trailing-spaces)
[error] 46-46: trailing spaces
(trailing-spaces)
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 48-48: trailing spaces
(trailing-spaces)
[error] 49-49: syntax error: expected , but found ''
(syntax)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/css_check.py (2)
11-23: Improve hex color code pattern matchingThe pattern might match hex codes in comments or strings, leading to false positives.
Consider updating the pattern to exclude matches in comments and strings:
- embedded_css_pattern = r"#([0-9a-fA-F]{3}){1,2}" # Matches CSS color codes + # Exclude matches in single-line comments, multi-line comments, and strings + embedded_css_pattern = r'(?<!\/\/[^\n]*?)(?<!\/\*[\s\S]*?)(?<![\'"`])#([0-9a-fA-F]{3}){1,2}(?![0-9a-fA-F])'
70-72: Improve CSS import pattern matchingThe current pattern might miss some valid import statements.
Consider updating the pattern to handle more import variations:
- r'import\s+.*?["\'](.*?\.css)["\'];', content + r'import\s+(?:(?:{[^}]*}|\*|\w+)\s+from\s+)?["\'](.*?\.css)["\'];', content
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/css_check.py(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.github/workflows/css_check.py (1)
Learnt from: IITI-tushar
PR: PalisadoesFoundation/talawa-admin#3041
File: .github/workflows/css_check.py:11-22
Timestamp: 2025-01-07T09:50:29.019Z
Learning: In the talawa-admin project, CSS validation in TypeScript files should only check for hex color codes using the pattern `#([0-9a-fA-F]{3}){1,2}` and ensure that files only reference the `src/style/app.module.css` stylesheet.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
.github/workflows/css_check.py (2)
1-9: LGTM! Well-structured script setup.The script has proper shebang, encoding declaration, and imports.
148-150: LGTM! Proper script entry point.The script uses the standard Python idiom for execution.
noman2002
merged commit d669de3
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
feature
Issue Number:
#2907
Fixes #2907
Did you add tests for your changes?
Snapshots/Videos:
I checked if the script is working as expected or not.
The screen shots are here..
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
maybe
Other information
Have you read the contributing guide?
yes
Summary by CodeRabbit
New Features
Chores