feat: added Check for embedded CSS and the correct stylesheet in Typescript files #3261
Conversation
WalkthroughThe pull request introduces enhancements to the GitHub Actions workflow for handling pull requests, specifically focusing on CSS and TypeScript file validation. A new Python script Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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: 3
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
43-50: Consider adding exclusions for generated TypeScript files.The current file pattern might include generated TypeScript files that shouldn't be checked for CSS violations.
- name: Get changed TypeScript files id: changed-ts-files uses: tj-actions/changed-files@v45 with: files: | **/*.ts **/*.tsx + files_ignore: | + **/*.generated.ts + **/*.gen.ts + **/dist/**
📜 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
📓 Learnings (1)
📓 Common learnings
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.
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/css_check.py
[warning] 1-1:
File ignored by default.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
.github/workflows/css_check.py (2)
9-21: LGTM! Function correctly implements CSS color code detection.The implementation aligns with the project requirements for detecting hex color codes.
59-59: Reconsider excluding test files from CSS validation.The current implementation skips files in test directories, but CSS violations in test files could still impact the application.
|
@palisadoes i have updated the code to run for files changed in the PR... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3261 +/- ##
=====================================================
+ Coverage 10.74% 89.72% +78.98%
=====================================================
Files 309 334 +25
Lines 7802 8626 +824
Branches 1729 1919 +190
=====================================================
+ Hits 838 7740 +6902
+ Misses 6900 624 -6276
- Partials 64 262 +198
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Cioppolo14
requested review from
noman2002 and
beingnoble03
and removed request for
palisadoes
IITI-tushar
force-pushed
the
added-app.module.css-check
branch
from
dd77fc9 to
0681a9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/css_check.py (2)
14-26: Add specific return type hint.The function correctly implements the required hex color pattern check, but could benefit from a more specific return type hint.
-def check_embedded_css(content: str) -> list: +def check_embedded_css(content: str) -> list[str]:🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
71-73: Improve CSS import detection regex pattern.The current pattern might miss valid imports with:
- Template literals:
import styles from `./styles.css`- Multi-line imports
- Comments
- css_imports = re.findall(r'import\s+.*?["\'](.+?\.css)["\']', content) + css_imports = re.findall(r'import\s+.*?[`"\']([^`"\']+\.css)[`"\']', content)🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/css_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-request.yml
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
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.
🪛 GitHub Actions: PR Workflow
.github/workflows/css_check.py
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (4)
.github/workflows/css_check.py (4)
8-13: Well-structured data models using namedtuples!Good use of namedtuples for type safety and immutability.
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
97-167: Well-implemented CLI with clear error handling!Good job on:
- Making success messages optional with --show_success
- Providing clear next steps in error messages
- Using appropriate exit codes
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
1-7: Address pipeline failure for sensitive file modification.The pipeline indicates this file is in a sensitive location (
.github/workflows). To proceed:
- Ensure the changes follow the project's workflow modification guidelines
- Request the 'ignore-sensitive-files-pr' label to be added to the PR
✅ Verification successful
Workflow file addition appears legitimate - proceed with label request
The file is appropriately placed alongside similar validation scripts (e.g., check-tsdoc.js) with consistent permissions. Since this is a new workflow file:
- Request the 'ignore-sensitive-files-pr' label to be added
- Ensure the PR description documents the purpose of this CSS validation workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if similar workflow files exist and their permissions ls -la .github/workflows/Length of output: 993
Script:
#!/bin/bash # Check git history for css_check.py git log --format="%h %s" .github/workflows/css_check.py # Check if file was recently added git log --diff-filter=A --format="%h %s" .github/workflows/css_check.pyLength of output: 0
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
64-69: Verify file read security implications.Since this script runs in GitHub Actions with workflow permissions, ensure:
- File reading is limited to the repository context
- Large files don't cause memory issues
✅ Verification successful
File read implementation is secure
The implementation has proper security controls:
- Path traversal protection via path normalization and controlled directory traversal
- Memory-safe file handling with proper resource cleanup
- Appropriate error handling for IO and encoding issues
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential large files that could cause issues find . -type f -name "*.ts" -o -name "*.tsx" -size +1MLength of output: 69743
Script:
#!/bin/bash # Check the full css_check.py script to understand file_path handling cat .github/workflows/css_check.pyLength of output: 6433
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/css_check.py (1)
73-82:⚠️ Potential issueImprove CSS import path resolution.
The current path resolution might fail for imports using parent directory references.
- css_imports = re.findall(r'import\s+.*?["\'](.+?\.css)["\']', content) + # Handle both quote types and template literals + css_imports = re.findall(r'import\s+.*?[`"\']([^`"\']+?\.css)[`"\']', content) for css_file in css_imports: - # Try to find the CSS file - css_file_path = os.path.join(os.path.dirname(file_path), css_file) - if not os.path.exists(css_file_path): - # If not found, try to find it relative to the src directory - src_dir = os.path.abspath(directory) - css_file_path = os.path.join(src_dir, css_file) + # Resolve the CSS file path + base_path = os.path.dirname(file_path) + css_file_path = os.path.normpath(os.path.join(base_path, css_file)) + if not os.path.exists(css_file_path): + # Try relative to the project root + css_file_path = os.path.normpath(os.path.join(directory, css_file))🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
🧹 Nitpick comments (5)
.github/workflows/css_check.py (5)
10-14: Add documentation and type hints to namedtuples.Consider adding docstrings and type hints to improve maintainability:
# Define namedtuples for storing results -Violation = namedtuple('Violation', ['file_path', 'css_file', 'reason']) -CorrectImport = namedtuple('CorrectImport', ['file_path', 'css_file']) -EmbeddedViolation = namedtuple('EmbeddedViolation', ['file_path', 'css_codes']) -CSSCheckResult = namedtuple('CSSCheckResult', ['violations', 'correct_imports', 'embedded_violations']) +from typing import List, NamedTuple + +class Violation(NamedTuple): + """Represents a CSS import violation.""" + file_path: str + css_file: str + reason: str + +class CorrectImport(NamedTuple): + """Represents a valid CSS import.""" + file_path: str + css_file: str + +class EmbeddedViolation(NamedTuple): + """Represents embedded CSS violations.""" + file_path: str + css_codes: List[str] + +class CSSCheckResult(NamedTuple): + """Contains the results of CSS validation.""" + violations: List[Violation] + correct_imports: List[CorrectImport] + embedded_violations: List[EmbeddedViolation]🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
16-28: Enhance embedded CSS detection with line numbers.The function correctly implements the hex color code check, but it could be more helpful by:
- Including line numbers for each violation
- Handling case-insensitive matches
-def check_embedded_css(content: str) -> list: +def check_embedded_css(content: str) -> List[tuple[str, int]]: """ Check for embedded CSS in the content. Args: content: The content of the file to check. Returns: - A list of embedded CSS violations found. + A list of tuples containing (color_code, line_number). """ - embedded_css_pattern = r"#([0-9a-fA-F]{3}){1,2}" # Matches CSS color codes - return re.findall(embedded_css_pattern, content) + embedded_css_pattern = r"#([0-9a-fA-F]{3}){1,2}" + violations = [] + for i, line in enumerate(content.splitlines(), 1): + matches = re.finditer(embedded_css_pattern, line, re.IGNORECASE) + violations.extend((match.group(), i) for match in matches) + return violations🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
65-65: Make test directory exclusion configurable.The hardcoded
"test"directory exclusion should be configurable through CLI arguments.- if file.endswith((".ts", ".tsx")) and "test" not in root: + if file.endswith((".ts", ".tsx")) and not any(test_dir in root for test_dir in args.exclude_test_dirs):Add to CLI arguments:
parser.add_argument( "--exclude_test_dirs", nargs="*", default=["test", "tests", "__tests__"], help="Test directories to exclude from analysis.", )🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
69-71: Enhance error handling with line information.Provide more context in error messages to help users identify issues.
- except (IOError, UnicodeDecodeError) as e: - print(f"Error reading file {file_path}: {e}") + except UnicodeDecodeError as e: + print(f"Error: File {file_path} is not UTF-8 encoded: {e}") + except IOError as e: + print(f"Error: Cannot read file {file_path}: {e}") + if e.errno == 13: # Permission denied + print("Tip: Check file permissions")🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
137-164: Improve output formatting and documentation.Consider structuring the output in a more readable format and documenting exit codes.
+ # Define exit codes + EXIT_SUCCESS = 0 + EXIT_FAILURE = 1 + output = [] - exit_code = 0 + exit_code = EXIT_SUCCESS + + def format_violation(v: Violation) -> str: + """Format a violation message.""" + return f" • {os.path.relpath(v.file_path)}\n ↳ {v.css_file} ({v.reason})" + if result.violations: - output.append("CSS Import Violations:") - for violation in result.violations: - output.append(f"- {violation.file_path}: {violation.css_file} ({violation.reason})") - exit_code = 1 + output.extend([ + "CSS Import Violations:", + *[format_violation(v) for v in result.violations], + "" + ]) + exit_code = EXIT_FAILURE🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/css_check.py(1 hunks).github/workflows/pull-request.yml(1 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx
- .github/workflows/pull-request.yml
🧰 Additional context used
📓 Learnings (1)
📓 Common learnings
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.
🪛 GitHub Actions: PR Workflow
.github/workflows/css_check.py
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
.github/workflows/css_check.py (1)
1-169: Address sensitive file modification warning.The pipeline indicates this is a sensitive file modification. Please:
- Request the
ignore-sensitive-files-prlabel to be added to the PR- Ensure you have the necessary permissions to modify workflow files
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
|
|
palisadoes
added
the
ignore-sensitive-files-pr
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/css_check.py (3)
16-28: Enhance CSS detection patterns.While the current implementation catches basic hex colors, consider expanding the detection to include:
- 8-digit hex colors with alpha channel (#RRGGBBAA)
- Other color formats (rgb, rgba, hsl)
- Other embedded CSS properties
- embedded_css_pattern = r"#([0-9a-fA-F]{3}){1,2}" # Matches CSS color codes + patterns = [ + r"#([0-9a-fA-F]{3}){1,2}", # Basic hex colors + r"#[0-9a-fA-F]{8}", # Hex with alpha + r"rgb\s*\([^)]+\)", # RGB + r"rgba\s*\([^)]+\)", # RGBA + r"hsl\s*\([^)]+\)", # HSL + ] + results = [] + for pattern in patterns: + results.extend(re.findall(pattern, content)) + return results🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code formatting does not comply with Black style guide. File needs to be reformatted.
29-98: Add type hints and split the function for better maintainability.The function would benefit from:
- Type hints for better code maintainability
- Breaking down into smaller, focused functions
+from typing import List, Set, Tuple + +def normalize_paths(paths: List[str]) -> Set[str]: + """Normalize file paths.""" + return set(os.path.abspath(path) for path in paths) + +def process_file( + file_path: str, + allowed_patterns: List[str], + base_dir: str +) -> Tuple[List[Violation], List[CorrectImport], List[EmbeddedViolation]]: + """Process a single file for violations.""" + def check_files( - directory: str, exclude_files: list, exclude_directories: list, allowed_css_patterns: list + directory: str, + exclude_files: List[str], + exclude_directories: List[str], + allowed_css_patterns: List[str] ) -> CSSCheckResult:🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code formatting does not comply with Black style guide. File needs to be reformatted.
100-166: Enhance error messages with line numbers.Add line numbers to violation reports to help users quickly locate issues.
- output.append(f"- {violation.file_path}: {violation.css_file} ({violation.reason})") + with open(violation.file_path, 'r') as f: + for i, line in enumerate(f, 1): + if violation.css_file in line: + output.append(f"- {violation.file_path}:{i}: {violation.css_file} ({violation.reason})") + break🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code formatting does not comply with Black style guide. File needs to be reformatted.
📜 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
📓 Learnings (1)
📓 Common learnings
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.
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/css_check.py
[warning] 1-1:
File ignored by default.
🪛 GitHub Actions: PR Workflow
.github/workflows/css_check.py
[warning] Code formatting does not comply with Black style guide. File needs to be reformatted.
.github/workflows/pull-request.yml
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
🪛 actionlint (1.7.4)
.github/workflows/pull-request.yml
55-55: shellcheck reported issue in this script: SC2086:info:16:66: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/css_check.py (2)
1-14: Well-structured script setup!Good use of namedtuples for organizing results and proper module imports.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.🪛 GitHub Actions: PR Workflow
[warning] Code formatting does not comply with Black style guide. File needs to be reformatted.
168-170: LGTM!Standard Python script entry point implementation.
🧰 Tools
🪛 GitHub Actions: PR Workflow
[warning] Code formatting does not comply with Black style guide. File needs to be reformatted.
.github/workflows/pull-request.yml (1)
43-76:⚠️ Potential issueOptimize CSS check execution and handle authorization.
- Running the script for each file individually is inefficient. Process all files in a single run.
- This workflow file is marked as sensitive. The PR needs the 'ignore-sensitive-files-pr' label.
- # Process each modified TypeScript file - echo "${{ steps.changed-ts-files.outputs.modified_files }}" | tr ' ' '\n' | while read -r file; do - if [ ! -f "$file" ]; then - echo "Error: File not found: $file" - exit 1 - fi - - # Check for CSS violations on the modified file - # Run the Python script with the specific file - echo "Running CSS check for: $file" - python ./.github/workflows/css_check.py --directory "$(dirname $file)" --allowed_css_patterns app.module.css || { - echo "Error: CSS check failed for $file" - exit 1 - } - done + # Process all modified TypeScript files at once + python ./.github/workflows/css_check.py --directory . --exclude_files $(echo "${{ steps.changed-ts-files.outputs.modified_files }}" | tr ' ' ',') --allowed_css_patterns app.module.css || { + echo "Error: CSS check failed" + exit 1 + }✅ Verification successful
Optimize CSS checks but maintain sensitive file controls
The workflow file is indeed protected by the repository's sensitive files policy. Your changes require:
- The 'ignore-sensitive-files-pr' label for authorization (confirmed by repository policy)
- Consider the suggested performance optimization by processing all files in a single run
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the PR has the required label gh pr view ${{ github.event.pull_request.number }} --json labels -q '.labels[].name' | grep -q 'ignore-sensitive-files-pr' || { echo "Error: PR requires 'ignore-sensitive-files-pr' label for modifying workflow files" exit 1 }Length of output: 352
Script:
#!/bin/bash # Check for CODEOWNERS file echo "=== Checking CODEOWNERS ===" fd "CODEOWNERS" .github/ # Check for workflow files that might contain sensitive file patterns echo -e "\n=== Checking workflow files for sensitive patterns ===" rg -l "sensitive|protected|restricted" .github/workflows/ # Check for documentation about sensitive files echo -e "\n=== Checking documentation for sensitive files info ===" rg -l "sensitive.*files?|protected.*files?|restricted.*files?" -g "*.md"Length of output: 553
Script:
#!/bin/bash echo "=== Checking context of sensitive patterns in pull-request.yml ===" rg -C 5 "sensitive|protected|restricted" .github/workflows/pull-request.yml echo -e "\n=== Checking context of sensitive patterns in pull-request-target.yml ===" rg -C 5 "sensitive|protected|restricted" .github/workflows/pull-request-target.ymlLength of output: 2200
🧰 Tools
🪛 actionlint (1.7.4)
55-55: shellcheck reported issue in this script: SC2086:info:16:66: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 GitHub Actions: PR Workflow
[error] Unauthorized modification or deletion of sensitive file. Apply 'ignore-sensitive-files-pr' label to override.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/css_check.py (5)
19-31: Enhance hex color code detection.The current regex pattern might match non-CSS hex codes (e.g., in comments or strings) and is case-sensitive. Consider improving it:
- embedded_css_pattern = r"#([0-9a-fA-F]{3}){1,2}" # Matches CSS color codes + embedded_css_pattern = r'(?<![\w"])#([0-9a-fA-F]{3}){1,2}(?![\w"])' # Matches standalone CSS color codesThe improved pattern:
- Uses negative lookbehind/lookahead to ensure it's not part of a larger word/string
- Still matches both 3 and 6 digit hex codes
33-38: Add type hints for better maintainability.Consider adding type hints for the lists to improve code maintainability:
- exclude_files: list, - exclude_directories: list, - allowed_css_patterns: list, + exclude_files: list[str], + exclude_directories: list[str], + allowed_css_patterns: list[str],
83-90: Improve CSS file path resolution logic.The current path resolution might miss files in node_modules or other common locations. Consider checking additional paths:
# Try to find the CSS file base_path = os.path.dirname(file_path) css_file_path = os.path.normpath(os.path.join(base_path, css_file)) + possible_paths = [ + css_file_path, + os.path.join(src_dir, css_file), + os.path.join(src_dir, 'node_modules', css_file), + ] - if not os.path.exists(css_file_path): - # If not found, try to find it relative to the src directory - src_dir = os.path.abspath(directory) - css_file_path = os.path.join(src_dir, css_file) + for path in possible_paths: + if os.path.exists(path): + css_file_path = path + break
154-162: Define exit codes as constants.For better maintainability and clarity, consider defining exit codes as constants:
+# Exit codes +SUCCESS = 0 +VIOLATION_FOUND = 1 + def main(): # ... existing code ... output = [] - exit_code = 0 + exit_code = SUCCESS if result.violations: output.append("CSS Import Violations:") for violation in result.violations: output.append( f"- {violation.file_path}: {violation.css_file} ({violation.reason})" ) - exit_code = 1 + exit_code = VIOLATION_FOUND
121-139: Improve help messages for CLI arguments.The current help messages could be more descriptive to better guide users:
parser.add_argument( "--exclude_files", nargs="*", default=[], - help="Specific files to exclude from analysis.", + help="Space-separated list of files to exclude from analysis (e.g., 'src/legacy.ts').", ) parser.add_argument( "--exclude_directories", nargs="*", default=[], - help="Directories to exclude from analysis.", + help="Space-separated list of directories to exclude from analysis (e.g., 'src/generated').", ) parser.add_argument( "--allowed_css_patterns", nargs="*", default=["app.module.css"], - help="Allowed CSS file patterns.", + help="Space-separated list of allowed CSS file patterns (default: 'app.module.css'). Use to restrict CSS imports.", )
📜 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)
📓 Common learnings
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.
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/css_check.py
[warning] 1-1:
File ignored by default.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (2)
.github/workflows/css_check.py (2)
1-16: Well-structured script setup with clear data models!The script follows Python best practices with proper shebang, encoding declaration, and clear docstring. The use of namedtuples provides a clean and immutable data structure for results.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
1-190: Overall implementation aligns well with requirements!The script successfully implements:
- Checking for embedded CSS hex color codes
- Validating CSS imports against allowed patterns
- Providing clear feedback for violations
The implementation follows the project's requirements and incorporates feedback from previous reviews.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
|
Move your workflows/scripts/ to the scripts folder |
IITI-tushar
force-pushed
the
added-app.module.css-check
branch
from
7d3128e to
1e5934b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.github/workflows/scripts/css_check.py (3)
33-50: Consider breaking down the complex function into smaller, focused functions.The function has multiple responsibilities which makes it harder to maintain and test. Consider splitting it into:
find_typescript_files(directory, exclusions)validate_css_imports(file_path, content, allowed_patterns)
74-78: Improve file reading performance and error handling.Reading the entire file into memory could be problematic for large files. Consider using a buffered reader and more specific error handling.
- with open(file_path, "r", encoding="utf-8") as f: - content = f.read() - except (IOError, UnicodeDecodeError) as e: - print(f"Error reading file {file_path}: {e}") + content = "" + with open(file_path, "r", encoding="utf-8") as f: + for line in f: + content += line + if len(content) > MAX_FILE_SIZE: + print(f"Warning: {file_path} exceeds maximum size limit") + break + except IOError as e: + print(f"IO Error reading {file_path}: {e}") + except UnicodeDecodeError as e: + print(f"Unicode decode error in {file_path}: {e}")
137-138: Expand default allowed CSS patterns.The default pattern is too restrictive. Consider adding more common CSS module patterns.
- default=["app.module.css"], + default=[ + "module.css", + "module.scss", + "styles.css", + "styles.scss" + ],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/css_check.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-request.yml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
.github/workflows/scripts/css_check.py (3)
1-16: Well-structured file setup with secure imports!Good use of named tuples for type safety and immutable data structures.
189-190: Clean and standard entry point implementation!
1-190: Add test coverage for the CSS checking functionality.While the implementation meets the PR objectives, it would benefit from unit tests to verify:
- CSS pattern matching
- File traversal logic
- Error handling scenarios
Would you like me to help create a test suite for this script?
There was a problem hiding this comment.
You have not applied this as suggested previously
The logic must be:
- Inspect the files provided in the CLI for the embedded CSS and fail if found
- Fail if the files provided do not have a single import of the validated path to
app.module.css - Fail if a css file is found in the same directory as the submitted file
- Exclude directories provided in the CLI
- Only check files that are part of the single root directory provided in the CLI
You also have a --directories CLI option that only takes filenames. This is confusing. Please fix.
|
@palisadoes should I remove --directories flag and add a --files flag?? |
Yes. You should also add a flag stating what the starting point should be to determine whether the CSS import is correct. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/scripts/css_check.py (2)
61-61: Improve CSS import detection regex.The current regex might miss some valid import patterns. Consider using a more comprehensive pattern.
- css_imports = re.findall(r'import\s+.*?["\'](.+?\.css)["\']', content) + css_imports = re.findall(r'(?:import\s+[^;]*?|import\s*\([^)]*?\))\s*from\s*["\'](.+?\.css)["\']', content)
182-194: Enhance directory validation error messages.Consider providing more specific error messages to help users understand and fix validation failures.
- raise ValueError( - f"Invalid path: {path}. Must be an existing file or directory." - ) + raise ValueError( + f"Path '{path}' is invalid. " + "If it's a directory, ensure it exists and you have read permissions. " + "If it's a file, ensure it exists and has a valid TypeScript extension (.ts, .tsx)." + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pull-request.yml(1 hunks).github/workflows/scripts/css_check.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pull-request.yml
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/css_check.py
[warning] 1-1:
File ignored by default.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (6)
.github/workflows/scripts/css_check.py (6)
1-16: Well-structured file setup with clear data models!Good use of namedtuples for type safety and readability in storing results.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
19-30: Enhance embedded CSS detection coverage.The current regex pattern only catches hex colors within curly braces. Consider expanding the detection to include:
- RGB/RGBA colors
- Named colors
- Inline styles
- Style attributes
- embedded_css_pattern = r"\{[^{}]*#[0-9a-fA-F]{3,6}\b.*?}" + patterns = [ + r"\{[^{}]*#[0-9a-fA-F]{3,6}\b.*?}", # Hex colors in curly braces + r"style\s*=\s*['\"][^'\"]*['\"]", # Style attributes + r"<style[^>]*>[^<]*</style>", # Style tags + r"\{[^{}]*(?:rgb|rgba)\s*\([^}]+\)[^}]*\}", # RGB/RGBA in curly braces + r"\{[^{}]*(?:red|blue|green|yellow|black|white|gray)[^}]*\}" # Named colors + ] + embedded_css = [] + for pattern in patterns: + embedded_css.extend(re.findall(pattern, content, re.DOTALL)) + return embedded_css
142-142: Improve test file detection logic.Using
"test" not in rootis a brittle way to exclude test files.-if file.endswith((".ts", ".tsx")) and "test" not in root: +if file.endswith((".ts", ".tsx")) and not any( + pattern in root + for pattern in ["__tests__", ".test.", ".spec.", "test/", "tests/"] +):
122-125: Potential security risk with path normalization.Using
os.path.abspathwithout sanitization could lead to path traversal vulnerabilities.- exclude_files = set(os.path.abspath(file) for file in exclude_files) - exclude_directories = set( - os.path.abspath(dir) for dir in exclude_directories - ) + base_dir = os.path.realpath(os.getcwd()) + exclude_files = set( + path for file in exclude_files + if (path := os.path.realpath(file)).startswith(base_dir) + ) + exclude_directories = set( + path for dir in exclude_directories + if (path := os.path.realpath(dir)).startswith(base_dir) + )
197-320: Well-implemented main execution flow!The implementation features:
- Clear argument parsing
- Helpful error messages
- Proper exit code handling
81-84: Verify the strict requirement for app.module.css.The current implementation requires every TypeScript file to import app.module.css. This might be too restrictive for components that don't need styling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
.github/workflows/scripts/css_check.py (8)
11-16: Add type hints to namedtuples for better code maintainability.Consider adding type hints to the namedtuple fields for better code maintainability and IDE support.
-Violation = namedtuple("Violation", ["file_path", "css_file", "reason"]) -CorrectImport = namedtuple("CorrectImport", ["file_path", "css_file"]) -EmbeddedViolation = namedtuple("EmbeddedViolation", ["file_path", "css_codes"]) +from typing import List + +Violation = namedtuple("Violation", ["file_path: str", "css_file: str", "reason: str"]) +CorrectImport = namedtuple("CorrectImport", ["file_path: str", "css_file: str"]) +EmbeddedViolation = namedtuple("EmbeddedViolation", ["file_path: str", "css_codes: List[str]"])
28-29: Enhance embedded CSS detection patterns.The current regex pattern could miss some common CSS patterns. Consider adding patterns for:
- RGB/RGBA colors
- HSL/HSLA colors
- Named colors
- CSS properties with units (px, em, rem)
- embedded_css_pattern = r"\{[^{}]*#[0-9a-fA-F]{3,6}\b.*?}" - return re.findall(embedded_css_pattern, content, re.DOTALL) + patterns = [ + r"\{[^{}]*#[0-9a-fA-F]{3,6}\b.*?}", # Hex colors + r"\{[^{}]*\b\d+(?:px|em|rem|%)\b.*?}", # Units + r"\{[^{}]*\b(?:rgb|rgba|hsl|hsla)\([^)]+\).*?}", # Color functions + r"\{[^{}]*\b(?:red|blue|green|yellow|white|black)\b.*?}", # Named colors + ] + embedded_css = set() + for pattern in patterns: + embedded_css.update(re.findall(pattern, content, re.DOTALL)) + return list(embedded_css)
54-58: Improve memory efficiency for large files.Reading the entire file into memory could be inefficient for large files. Consider using a buffered approach for large files.
- with open(file_path, "r", encoding="utf-8") as f: - content = f.read() + content = "" + with open(file_path, "r", encoding="utf-8") as f: + # Read file in chunks of 1MB + chunk_size = 1024 * 1024 + while chunk := f.read(chunk_size): + content += chunk
64-69: Improve CSS import path resolution.The current path resolution might miss CSS files in node_modules or other common locations. Consider checking additional paths.
base_path = os.path.dirname(file_path) css_file_path = os.path.normpath(os.path.join(base_path, css_file)) + # List of potential base paths to check + base_paths = [ + base_path, + os.path.abspath(directory), + os.path.join(os.path.abspath(directory), "node_modules"), + os.path.join(os.path.dirname(os.path.abspath(directory)), "node_modules"), + ] + if not os.path.exists(css_file_path): - src_dir = os.path.abspath(directory) - css_file_path = os.path.join(src_dir, css_file) + for path in base_paths: + temp_path = os.path.normpath(os.path.join(path, css_file)) + if os.path.exists(temp_path): + css_file_path = temp_path + break
94-101: Add parallel processing support for large directories.For repositories with many files, sequential processing could be slow. Consider adding parallel processing support.
+from concurrent.futures import ThreadPoolExecutor +from functools import partial + def check_files( directories: list, files: list, exclude_files: list, exclude_directories: list, allowed_css_patterns: list, + max_workers: int = 4, ) -> CSSCheckResult:And update the processing logic:
+ def process_file(file_path, directory): + local_violations = [] + local_correct_imports = [] + local_embedded_violations = [] + process_typescript_file( + file_path, + directory, + allowed_css_patterns, + local_violations, + local_correct_imports, + local_embedded_violations, + ) + return (local_violations, local_correct_imports, local_embedded_violations) + + with ThreadPoolExecutor(max_workers=max_workers) as executor: + futures = [] + for directory in directories: + # ... existing directory walking logic ... + futures.append(executor.submit(process_file, file_path, directory)) + + for future in futures: + result = future.result() + violations.extend(result[0]) + correct_css_imports.extend(result[1]) + embedded_css_violations.extend(result[2])
130-136: Add progress reporting for better user experience.For large repositories, it would be helpful to show progress during the scan.
+from tqdm import tqdm + for directory in directories: directory = os.path.abspath(directory) + total_files = sum(1 for _ in os.walk(directory)) + progress_bar = tqdm( + total=total_files, + desc=f"Scanning {directory}", + unit="files" + ) for root, _, files_in_dir in os.walk(directory): + progress_bar.update(1) if any( root.startswith(exclude_dir) for exclude_dir in exclude_directories ): continue + progress_bar.close()
191-214: Improve path validation and security.Consider converting paths to absolute paths and resolving symlinks for better security and consistency.
validated_dirs = [] for path in input_directories: + # Convert to absolute path and resolve symlinks + path = os.path.realpath(os.path.abspath(path)) + if os.path.isdir(path): + # Check if directory is readable + if not os.access(path, os.R_OK): + raise ValueError(f"Directory not readable: {path}") validated_dirs.append(path) elif os.path.isfile(path): + # Check if file is readable + if not os.access(path, os.R_OK): + raise ValueError(f"File not readable: {path}") validated_dirs.append(os.path.dirname(path)) else: raise ValueError( f"Invalid path: {path}. Must be an existing file or directory." ) + + # Remove duplicates while preserving order + return list(dict.fromkeys(validated_dirs))
293-329: Add JSON output option for better CI/CD integration.Consider adding a JSON output option for better integration with CI/CD pipelines and other tools.
+import json + + parser.add_argument( + "--json", + action="store_true", + help="Output results in JSON format", + ) + output = [] exit_code = 0 + + if args.json: + json_output = { + "violations": [v._asdict() for v in result.violations], + "embedded_violations": [v._asdict() for v in result.embedded_violations], + "correct_imports": [i._asdict() for i in result.correct_imports], + "exit_code": exit_code + } + print(json.dumps(json_output, indent=2)) + sys.exit(exit_code)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/css_check.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/css_check.py
[warning] 1-1:
File ignored by default.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
.github/workflows/scripts/css_check.py (1)
339-340: LGTM!The entry point is correctly implemented.
|
@palisadoes sir here's an example of embedded css warning which the script will show...
is it okay?? |
|
The script just needs to print the file name, the color code and the line on which the embedded code was found. One instance per line. No multi-line errors per violation, it makes it harder to read. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/pull-request.yml (1)
46-46: Remove trailing spaces.The line contains trailing spaces which should be removed to maintain clean YAML syntax.
- +🧰 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 (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/pull-request.yml
[error] 46-46: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
| - name: Check for CSS violations and print correct imports | ||
| if: ${{ steps.changed-files.outputs.any_changed == 'true' }} | ||
| run: | | ||
| # Pass all modified files as arguments to the script | ||
| python3 ./.github/workflows/scripts/css_check.py \ | ||
| --files ${{ steps.changed-files.outputs.modified_files }} \ | ||
| --allowed_css_patterns app.module.css |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve robustness of the CSS check step.
The step needs improvements in error handling and setup:
- The script path should be
.github/workflows/scripts/css_check.pyto match the project structure - Python setup and script validation should be added
# Run the CSS import check script on changed .ts and .tsx files
- name: Check for CSS violations and print correct imports
if: ${{ steps.changed-files.outputs.any_changed == 'true' }}
run: |
+ # Ensure Python is available
+ if ! command -v python3 &> /dev/null; then
+ echo "Error: Python 3 is required but not installed"
+ exit 1
+ fi
+
+ # Validate script exists
+ if [ ! -f .github/workflows/scripts/css_check.py ]; then
+ echo "Error: CSS check script not found"
+ exit 1
+ fi
+
+ # Make script executable
+ chmod +x .github/workflows/scripts/css_check.py
+
# Pass all modified files as arguments to the script
- python3 ./.github/workflows/scripts/css_check.py \
+ if ! python3 .github/workflows/scripts/css_check.py \
--files ${{ steps.changed-files.outputs.modified_files }} \
- --allowed_css_patterns app.module.css
+ --allowed_css_patterns app.module.css; then
+ echo "Error: CSS check failed"
+ exit 1
+ fi📝 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.
| - name: Check for CSS violations and print correct imports | |
| if: ${{ steps.changed-files.outputs.any_changed == 'true' }} | |
| run: | | |
| # Pass all modified files as arguments to the script | |
| python3 ./.github/workflows/scripts/css_check.py \ | |
| --files ${{ steps.changed-files.outputs.modified_files }} \ | |
| --allowed_css_patterns app.module.css | |
| - name: Check for CSS violations and print correct imports | |
| if: ${{ steps.changed-files.outputs.any_changed == 'true' }} | |
| run: | | |
| # Ensure Python is available | |
| if ! command -v python3 &> /dev/null; then | |
| echo "Error: Python 3 is required but not installed" | |
| exit 1 | |
| fi | |
| # Validate script exists | |
| if [ ! -f .github/workflows/scripts/css_check.py ]; then | |
| echo "Error: CSS check script not found" | |
| exit 1 | |
| fi | |
| # Make script executable | |
| chmod +x .github/workflows/scripts/css_check.py | |
| # Pass all modified files as arguments to the script | |
| if ! python3 .github/workflows/scripts/css_check.py \ | |
| --files ${{ steps.changed-files.outputs.modified_files }} \ | |
| --allowed_css_patterns app.module.css; then | |
| echo "Error: CSS check failed" | |
| exit 1 | |
| fi |
There was a problem hiding this comment.
@palisadoes should i add these changes or not??
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Please make coderabbit.ai approve your changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/scripts/css_check.py (4)
63-65: Enhance error handling with specific exceptions.The current error handling is too broad. Consider handling specific exceptions separately and providing more detailed error messages.
- except (IOError, UnicodeDecodeError) as e: - print(f"Error reading file {file_path}: {e}") - return + except IOError as e: + print(f"Failed to read file {file_path}: {e}") + return + except UnicodeDecodeError as e: + print(f"File {file_path} has invalid encoding: {e}") + return
101-196: Optimize performance for large codebases.Consider these performance improvements:
- Use set operations for faster lookups
- Process files in parallel for large codebases
+from concurrent.futures import ThreadPoolExecutor +from functools import partial def check_files(...): - exclude_files = set(os.path.abspath(file) for file in exclude_files) - exclude_directories = set(os.path.abspath(dir) for dir in exclude_directories) + # Convert to sets once for faster lookups + exclude_files_set = {os.path.abspath(file) for file in exclude_files} + exclude_directories_set = {os.path.abspath(dir) for dir in exclude_directories} + test_patterns_set = {"__tests__", ".test.", ".spec.", "test/", "tests/"} + + def process_file(file_path, directory): + if file_path in exclude_files_set: + return None + + if any(pattern in file_path for pattern in test_patterns_set): + return None + + return process_typescript_file(...) + + with ThreadPoolExecutor() as executor: + # Process files in parallel + futures = [] + for directory in directories: + for root, _, files in os.walk(directory): + if any(root.startswith(d) for d in exclude_directories_set): + continue + + for file in files: + if file.endswith((".ts", ".tsx")): + file_path = os.path.abspath(os.path.join(root, file)) + futures.append( + executor.submit(process_file, file_path, directory) + ) + + # Collect results + for future in futures: + result = future.result() + if result: + violations.extend(result.violations) + correct_imports.extend(result.correct_imports) + embedded_violations.extend(result.embedded_violations)
198-222: Add additional path validation checks.Consider adding checks for:
- Symbolic link loops
- Read permissions
- Hidden directories
def validate_directories_input(input_directories): + def is_valid_path(path): + """Check if path is valid and accessible.""" + try: + # Resolve any symbolic links and check for loops + real_path = os.path.realpath(path) + # Check read permissions + if not os.access(real_path, os.R_OK): + return False, "Read permission denied" + # Skip hidden directories + if os.path.basename(real_path).startswith('.'): + return False, "Hidden directory" + return True, None + except OSError as e: + return False, str(e) + validated_dirs = [] for path in input_directories: - if os.path.isdir(path): + is_valid, error = is_valid_path(path) + if not is_valid: + raise ValueError(f"Invalid path {path}: {error}") + + if os.path.isdir(path): validated_dirs.append(path) elif os.path.isfile(path): validated_dirs.append(os.path.dirname(path))
224-347: Add structured logging for better debugging.Consider adding structured logging to help with debugging and monitoring.
+import logging +import json + def main(): + # Configure logging + logging.basicConfig( + format='%(asctime)s - %(levelname)s - %(message)s', + level=logging.INFO + ) + parser = argparse.ArgumentParser(...) try: directories = validate_directories_input(...) + logging.info("Validated directories: %s", json.dumps(directories)) except ValueError as e: + logging.error("Directory validation failed: %s", str(e)) print(f"Error: {e}") sys.exit(1) result = check_files(...) + logging.info( + "Check completed - Violations: %d, Correct imports: %d, Embedded violations: %d", + len(result.violations), + len(result.correct_imports), + len(result.embedded_violations) + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/scripts/css_check.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
.github/workflows/scripts/css_check.py
[warning] 1-1:
File ignored by default.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (4)
.github/workflows/scripts/css_check.py (4)
1-17: Well-structured file setup with clear organization!Good use of namedtuples for type safety and immutability.
🧰 Tools
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[warning] 1-1:
File ignored by default.
349-350: LGTM! Proper entry point implementation.
19-37: 🛠️ Refactor suggestionEnhance embedded CSS detection coverage.
The current implementation only detects hex color codes. Consider expanding the detection to include:
- RGB/RGBA colors
- HSL/HSLA colors
- Named colors
- Style attributes
- Inline styles
def check_embedded_css(content: str) -> list: - color_code_pattern = r"#(?:[0-9a-fA-F]{3}|[0-9a-fA-F]{6})\b" - violations = [] + patterns = { + 'hex': r"#(?:[0-9a-fA-F]{3}|[0-9a-fA-F]{6})\b", + 'rgb': r"rgb\(\s*\d+\s*,\s*\d+\s*,\s*\d+\s*\)", + 'rgba': r"rgba\(\s*\d+\s*,\s*\d+\s*,\s*\d+\s*,\s*[\d.]+\s*\)", + 'hsl': r"hsl\(\s*\d+\s*,\s*[\d.]+%?\s*,\s*[\d.]+%?\s*\)", + 'hsla': r"hsla\(\s*\d+\s*,\s*[\d.]+%?\s*,\s*[\d.]+%?\s*,\s*[\d.]+\s*\)", + 'named_colors': r"\b(red|blue|green|yellow|purple|black|white|gray)\b", + 'style_attr': r"style\s*=\s*['\"][^'\"]*['\"]", + 'inline_style': r"<style[^>]*>[^<]*</style>" + } + violations = [] for line_number, line in enumerate(content.splitlines(), start=1): - matches = re.findall(color_code_pattern, line) - for match in matches: - violations.append((line_number, match)) + for pattern_type, pattern in patterns.items(): + matches = re.findall(pattern, line) + for match in matches: + if isinstance(match, tuple): # For patterns with groups + match = match[0] + violations.append((line_number, f"{pattern_type}: {match}"))Likely invalid or redundant comment.
71-76:⚠️ Potential issueImprove path traversal security.
The current implementation might be vulnerable to path traversal attacks. Consider using
os.path.realpathand validating that the resolved path is within the intended directory.- base_path = os.path.dirname(file_path) - css_file_path = os.path.normpath(os.path.join(base_path, css_file)) - if not os.path.exists(css_file_path): - src_dir = os.path.abspath(directory) - css_file_path = os.path.join(src_dir, css_file) + base_path = os.path.realpath(os.path.dirname(file_path)) + src_dir = os.path.realpath(directory) + css_file_path = os.path.realpath(os.path.join(base_path, css_file)) + + # Ensure the resolved path is within allowed directories + if not (css_file_path.startswith(base_path) or css_file_path.startswith(src_dir)): + violations.append(Violation(file_path, css_file, "Path traversal attempt detected")) + continueLikely invalid or redundant comment.
i have fixed this... the embedded css violations will look like this now.. |
The less questions reviewers have to answer, the better |
|
|
@palisadoes sir is it okay..? |
|
Yes |
palisadoes
merged commit 45b332c
into
PalisadoesFoundation:develop-postgres
|
@palisadoes sir i still have to commit one thing in css_check.py. |
|
Yes |
What kind of change does this PR introduce?
feature
Issue Number:#2907
Fixes #2907
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Other information
Have you read the contributing guide?
yes
Summary by CodeRabbit
Release Notes
New Features
Documentation