Added app.module.css check #3536
Conversation
WalkthroughThis pull request adds a new step to the GitHub Actions workflow that conditionally checks for CSS violations in modified TypeScript files. It introduces a Python script ( Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant GH as GitHub Actions
participant CS as CSS Check Script
PR->>GH: Trigger pull request event
GH->>GH: Determine if TypeScript files were modified
GH->>CS: Run css_check.py with file paths & allowed CSS patterns
CS->>CS: Process each TypeScript file (regex check for CSS issues)
CS-->>GH: Return validation results (violations/success)
GH->>PR: Report CSS check outcome in the workflow
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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
|
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: 1
🧹 Nitpick comments (2)
src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx (1)
11-11: Remove extra period in doc commentThere is a small typo with two successive periods after the word “component” (line 11). Remove the extra dot for cleaner documentation.
- * Props for the `addOnEntry` component.. + * Props for the `addOnEntry` component..github/workflows/pull-request.yml (1)
46-46: Remove trailing whitespaceThere are trailing spaces at line 46. Removing them helps maintain consistent formatting.
- +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/css_check.py(1 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 46-46: trailing spaces
(trailing-spaces)
🪛 Ruff (0.8.2)
.github/workflows/scripts/css_check.py
320-320: Loop control variable line_number not used within loop body
(B007)
320-320: Loop control variable css_code not used within loop body
(B007)
🔇 Additional comments (2)
.github/workflows/scripts/css_check.py (1)
308-316: Script does not fail on CSS import violationsCurrently, the script sets
exit_codeto 0 even when CSS import violations are found (lines 308–316). If allowing these violations to pass is only a temporary measure, consider adding a comment or a TODO to revisit this behavior later in order to enforce stricter compliance with required CSS imports down the road..github/workflows/pull-request.yml (1)
47-55: New step for CSS checks looks goodThese lines add a new step to run
css_check.pyon changed TypeScript files. The logic appears sound and integrates nicely with the existing workflow.
| for violation in result.embedded_violations: | ||
| for line_number, css_code in zip( | ||
| violation.line_numbers, violation.css_codes | ||
| ): | ||
| relative_file_path = os.path.relpath(violation.file_path) | ||
| for css_code in violation.css_codes: | ||
| output.append( | ||
| f"- File: {relative_file_path}, Line: {line_number}: " | ||
| f"Embedded color code `{css_code}` detected.Please replace" | ||
| f" it with a CSS variable in `src/style/app.module.css`." | ||
| ) | ||
| exit_code = 1 |
There was a problem hiding this comment.
Fix unused loop variables and unify the embedded CSS logic
Within the first loop (lines 320–324), the line_number and css_code variables are never used. Then, there is a second loop at lines 324–329 that also references line_number but never correctly reassigns it. Combining these two loops into a single loop is more concise and prevents logic errors:
-for line_number, css_code in zip(
- violation.line_numbers, violation.css_codes
-):
- relative_file_path = os.path.relpath(violation.file_path)
-for css_code in violation.css_codes:
+for line_number, css_code in zip(
+ violation.line_numbers, violation.css_codes
+):
relative_file_path = os.path.relpath(violation.file_path)
output.append(
f"- File: {relative_file_path}, Line: {line_number}: "
f"Embedded color code `{css_code}` detected."
"Please replace it with a CSS variable in"
" `src/style/app.module.css`."
)📝 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.
| for violation in result.embedded_violations: | |
| for line_number, css_code in zip( | |
| violation.line_numbers, violation.css_codes | |
| ): | |
| relative_file_path = os.path.relpath(violation.file_path) | |
| for css_code in violation.css_codes: | |
| output.append( | |
| f"- File: {relative_file_path}, Line: {line_number}: " | |
| f"Embedded color code `{css_code}` detected.Please replace" | |
| f" it with a CSS variable in `src/style/app.module.css`." | |
| ) | |
| exit_code = 1 | |
| for violation in result.embedded_violations: | |
| for line_number, css_code in zip( | |
| violation.line_numbers, violation.css_codes | |
| ): | |
| relative_file_path = os.path.relpath(violation.file_path) | |
| output.append( | |
| f"- File: {relative_file_path}, Line: {line_number}: " | |
| f"Embedded color code `{css_code}` detected." | |
| "Please replace it with a CSS variable in" | |
| " `src/style/app.module.css`." | |
| ) | |
| exit_code = 1 |
🧰 Tools
🪛 Ruff (0.8.2)
320-320: Loop control variable line_number not used within loop body
(B007)
320-320: Loop control variable css_code not used within loop body
(B007)
|
@palisadoes Sir as we discussed earlier
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
47-47: Remove Trailing Spaces
There are trailing spaces on this blank line (line 47) which can be cleaned up for consistency and to avoid YAML lint errors.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 47-47: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/pull-request.yml
[error] 47-47: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
48-56: New CSS Check Step Integration
This new step correctly integrates thecss_check.pyscript into the workflow. It conditionally runs when any TypeScript file is modified, passes the modified file paths, and restricts allowed CSS patterns toapp.module.css.
Please verify if the allowed CSS pattern parameter should include the full path (e.g.,src/style/app.module.css) as mentioned in the PR objectives, to ensure the check precisely matches the intended file.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3536 +/- ##
=================================================
Coverage 88.61% 88.61%
=================================================
Files 341 341
Lines 8627 8627
Branches 1925 1925
=================================================
Hits 7645 7645
Misses 638 638
Partials 344 344 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
- Once again. Please remove all references to this
allowed_css_patternslogic. We don't want to verify whether the CSS file is imported. - We only want to ensure that embedded CSS is not found.
- The file app.module.css will soon be imported on app startup for all modules to access. We don't need the check. It is out of scope of the issue given the new circumstances.
What kind of change does this PR introduce?
added a script css_check.py,
It will:
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
yes
Summary by CodeRabbit