updated css_check.py #3422
Closed
updated css_check.py #3422
Conversation
This file contains 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
WalkthroughThis pull request introduces modifications to three files: a Python script for CSS violation checking, a TypeScript component for sign-out functionality, and its corresponding documentation. The changes focus on enhancing the embedded CSS violation reporting mechanism by adding line number tracking and updating the logout function's declaration to be asynchronous. The modifications aim to improve code structure and provide more detailed violation reporting. Changes
Possibly related PRs
Suggested labels
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
|
palisadoes
previously approved these changes
Jan 25, 2025
|
Please fix the failing tests |
![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
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/scripts/css_check.py(3 hunks)docs/docs/auto-docs/components/SignOut/SignOut/functions/default.md(1 hunks)src/components/SignOut/SignOut.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/docs/auto-docs/components/SignOut/SignOut/functions/default.md
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
.github/workflows/scripts/css_check.py
[error] Unauthorized file modification/deletion detected. This file is protected and requires the 'ignore-sensitive-files-pr' label to override.
src/components/SignOut/SignOut.tsx
[error] 25-25: Missing return type on function (@typescript-eslint/explicit-function-return-type)
🪛 eslint
src/components/SignOut/SignOut.tsx
[error] 25-25: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
src/components/SignOut/SignOut.tsx
[failure] 25-25:
Missing return type on function
🔇 Additional comments (4)
.github/workflows/scripts/css_check.py (3)
98-101: LGTM! Enhanced violation reporting with line numbers.The addition of line numbers to embedded CSS violations improves the developer experience by making it easier to locate and fix violations.
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized file modification/deletion detected. This file is protected and requires the 'ignore-sensitive-files-pr' label to override.
318-325: LGTM! Improved violation output format.The new output format clearly shows both the file path and line number for each violation, making it easier for developers to locate and fix issues.
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized file modification/deletion detected. This file is protected and requires the 'ignore-sensitive-files-pr' label to override.
13-15:⚠️ Potential issueProtected file modification detected.
This file is protected and requires the 'ignore-sensitive-files-pr' label to proceed. Please add the required label to the PR or consult with maintainers for guidance.
✅ Verification successful
Protected file modification verified.
The file
.github/workflows/scripts/css_check.pyis indeed protected as it resides in the.github/**path. The PR requires the 'ignore-sensitive-files-pr' label to proceed with modifications to protected files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the PR has the required label gh pr view 3422 --json labels -q '.labels[].name' | grep -q 'ignore-sensitive-files-pr' && echo "Label found" || echo "Label missing"Length of output: 213
Script:
#!/bin/bash # Search for protection patterns and documentation echo "=== Searching for protection patterns ===" rg -l "ignore-sensitive-files-pr|protected.*file" .github/ echo -e "\n=== Checking for documentation ===" fd "README|CONTRIBUTING" -e md -x cat {} 2>/dev/null echo -e "\n=== Checking workflow files for protection rules ===" fd "^.*\.ya?ml$" .github/workflows -x cat {} 2>/dev/nullLength of output: 55819
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized file modification/deletion detected. This file is protected and requires the 'ignore-sensitive-files-pr' label to override.
src/components/SignOut/SignOut.tsx (1)
24-39: LGTM! Improved code organization.Moving the
logoutfunction inside the component improves encapsulation and follows React best practices.🧰 Tools
🪛 eslint
[error] 25-25: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
🪛 GitHub Check: Performs linting, formatting, type-checking, checking for different source and target branch
[failure] 25-25:
Missing return type on function🪛 GitHub Actions: PR Workflow
[error] 25-25: Missing return type on function (@typescript-eslint/explicit-function-return-type)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/SignOut/SignOut.tsx(1 hunks)
🧰 Additional context used
🪛 eslint
src/components/SignOut/SignOut.tsx
[error] 25-25: Insert ·
(prettier/prettier)
[error] 25-25: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
[error] 26-26: Insert ·
(prettier/prettier)
[error] 27-27: Insert ·
(prettier/prettier)
[error] 28-28: Insert ·
(prettier/prettier)
[error] 29-29: Insert ·
(prettier/prettier)
[error] 31-31: Insert ·
(prettier/prettier)
[error] 32-32: Insert ·
(prettier/prettier)
[error] 33-33: Insert ·
(prettier/prettier)
[error] 34-34: Insert ·
(prettier/prettier)
[error] 35-35: Insert ·
(prettier/prettier)
[error] 36-36: Insert ·
(prettier/prettier)
[error] 37-37: Insert ·
(prettier/prettier)
[error] 38-38: Insert ·
(prettier/prettier)
[error] 39-39: Insert ·
(prettier/prettier)
🪛 GitHub Actions: PR Workflow
src/components/SignOut/SignOut.tsx
[warning] Code style issues found. Prettier formatting check failed. Run Prettier with --write to fix.
🔇 Additional comments (1)
src/components/SignOut/SignOut.tsx (1)
25-39: Fix formatting issues.The PR is failing due to code style issues. Please run Prettier to fix the formatting:
npx prettier --write "src/components/SignOut/SignOut.tsx"🧰 Tools
🪛 eslint
[error] 25-25: Insert
·(prettier/prettier)
[error] 25-25: Missing return type on function.
(@typescript-eslint/explicit-function-return-type)
[error] 26-26: Insert
·(prettier/prettier)
[error] 27-27: Insert
·(prettier/prettier)
[error] 28-28: Insert
·(prettier/prettier)
[error] 29-29: Insert
·(prettier/prettier)
[error] 31-31: Insert
·(prettier/prettier)
[error] 32-32: Insert
·(prettier/prettier)
[error] 33-33: Insert
·(prettier/prettier)
[error] 34-34: Insert
·(prettier/prettier)
[error] 35-35: Insert
·(prettier/prettier)
[error] 36-36: Insert
·(prettier/prettier)
[error] 37-37: Insert
·(prettier/prettier)
[error] 38-38: Insert
·(prettier/prettier)
[error] 39-39: Insert
·(prettier/prettier)
🪛 GitHub Actions: PR Workflow
[warning] Code style issues found. Prettier formatting check failed. Run Prettier with --write to fix.
IITI-tushar
force-pushed
the
updating-css_check.py
branch
from
409c038 to
e36724d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3422 +/- ##
=====================================================
+ Coverage 6.36% 89.81% +83.45%
=====================================================
Files 314 336 +22
Lines 8197 8664 +467
Branches 1843 1925 +82
=====================================================
+ Hits 522 7782 +7260
+ Misses 7615 621 -6994
- Partials 60 261 +201
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@palisadoes sir i have updated the css_check.py... |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/SignOut/SignOut.tsx (1)
24-29: Consider moving handleSignOut outside logout.The
handleSignOutfunction is recreated on every render since it's defined insidelogout. Moving it outside would optimize performance.+const handleSignOut = (): void => { + localStorage.clear(); + endSession(); + navigate('/'); +}; + const logout = async (): Promise<void> => { - const handleSignOut = (): void => { - localStorage.clear(); - endSession(); - navigate('/'); - };🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-28: src/components/SignOut/SignOut.tsx#L24-L28
Added lines #L24 - L28 were not covered by tests.github/workflows/scripts/css_check.py (1)
318-325: Improve error message formatting.The error message for embedded CSS violations has formatting issues:
- Missing space after "Please replace"
- Long line could be split for better readability
- f"Embedded color code `{css_code}` detected. Please replace" - f"it with a CSS variable in `src/style/app.module.css`." + f"Embedded color code `{css_code}` detected. Please replace " + f"it with a CSS variable in `src/style/app.module.css`."🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized file modification/deletion detected. This file is protected and requires the '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/scripts/css_check.py(3 hunks)docs/docs/auto-docs/components/SignOut/SignOut/functions/default.md(1 hunks)src/components/SignOut/SignOut.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/components/SignOut/SignOut.tsx
[warning] 24-28: src/components/SignOut/SignOut.tsx#L24-L28
Added lines #L24 - L28 were not covered by tests
[warning] 31-33: src/components/SignOut/SignOut.tsx#L31-L33
Added lines #L31 - L33 were not covered by tests
[warning] 35-35: src/components/SignOut/SignOut.tsx#L35
Added line #L35 was not covered by tests
[warning] 37-37: src/components/SignOut/SignOut.tsx#L37
Added line #L37 was not covered by tests
🪛 GitHub Actions: PR Workflow
.github/workflows/scripts/css_check.py
[error] Unauthorized file modification/deletion detected. This file is protected and requires the 'ignore-sensitive-files-pr' label to override.
🔇 Additional comments (2)
src/components/SignOut/SignOut.tsx (1)
24-39: Add test coverage for error handling scenarios.The error handling paths in the
logoutfunction are not covered by tests. This includes:
- Token revocation failure scenario
- Local sign out fallback behavior
Please add test cases to cover these scenarios. Here's a suggested test structure:
it('should handle token revocation failure gracefully', async () => { const mockError = new Error('Token revocation failed'); revokeRefreshToken.mockRejectedValueOnce(mockError); await logout(); expect(console.error).toHaveBeenCalledWith('Error revoking refresh token:', mockError); expect(localStorage.clear).toHaveBeenCalled(); expect(endSession).toHaveBeenCalled(); expect(navigate).toHaveBeenCalledWith('/'); });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 24-28: src/components/SignOut/SignOut.tsx#L24-L28
Added lines #L24 - L28 were not covered by tests
[warning] 31-33: src/components/SignOut/SignOut.tsx#L31-L33
Added lines #L31 - L33 were not covered by tests
[warning] 35-35: src/components/SignOut/SignOut.tsx#L35
Added line #L35 was not covered by tests
[warning] 37-37: src/components/SignOut/SignOut.tsx#L37
Added line #L37 was not covered by testsdocs/docs/auto-docs/components/SignOut/SignOut/functions/default.md (1)
9-9: LGTM!The documentation has been correctly updated to reflect the new line number where the function is defined.
Comment on lines
+13
to
+15
| EmbeddedViolation = namedtuple( | ||
| "EmbeddedViolation", ["file_path", "css_codes", "line_numbers"] | ||
| ) |
There was a problem hiding this comment.
Protected file modification detected.
This file is protected and requires the 'ignore-sensitive-files-pr' label to proceed with modifications.
Please:
- Add the 'ignore-sensitive-files-pr' label to this PR
- Get approval from maintainers before modifying this protected file
🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] Unauthorized file modification/deletion detected. This file is protected and requires the 'ignore-sensitive-files-pr' label to override.
palisadoes
added
the
ignore-sensitive-files-pr
|
palisadoes
requested changes
Jan 25, 2025
palisadoes
requested changes
Jan 25, 2025
There was a problem hiding this comment.
- There have been repeated failures of the script, especially when detecting whether the CSS file importation is correct.
- Please remove the check for the CSS file, and just focus on embedded CSS
I had to revert changes to pull-request.yml to ensure others could merge code.
|
@palisadoes sir this is the current logs when script is ran for src directory |
@palisadoes sir should i update the code so that:
Please suggest any other changes which will fix this... |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What kind of change does this PR introduce?
updating css_check.py
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
I forget to add a final commit in my recent PR so doing it here....
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
yes
Summary by CodeRabbit
Improvements
Refactor
Documentation