-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: enable cyclic deps check for UI packages #38658
base: release
Are you sure you want to change the base?
Conversation
WalkthroughThe pull request introduces an expanded scope for cyclic dependency checks in the GitHub workflow configuration. New directories within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
Update dpdm commands to include package dependencies
-npx dpdm ./src/* --circular --warning=false --tree=false +npx dpdm "./src/*" "./packages/**/src/*" --circular --warning=false --tree=false
- Update both dpdm commands in the workflow (PR branch and release branch steps)
- This change ensures cyclic dependency detection across all monitored directories
🔗 Analysis chain
Line range hint
1-89
: Verify the dpdm command configuration.The dpdm command only checks files in ./src/* but should also check the newly added package directories.
Run this script to verify the dpdm configuration:
After verification, update the dpdm command to include package directories:
-npx dpdm ./src/* --circular --warning=false --tree=false +npx dpdm "./src/*" "./packages/**/src/*" --circular --warning=false --tree=false🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if dpdm command includes all monitored directories # Get the dpdm command cmd=$(rg "npx dpdm" -A 1) echo "Current dpdm command:" echo "$cmd" # List all monitored directories echo -e "\nMonitored directories from changed-files action:" rg "files: \|" -A 10Length of output: 228
Script:
#!/bin/bash # Check workflow file content and package structure echo "=== Workflow File Content ===" cat .github/workflows/ci-client-cyclic-deps-check.yml echo -e "\n=== Package Directory Structure ===" fd . "app/client/packages" -t d -d 2 echo -e "\n=== Checking for src directories in packages ===" fd "src" "app/client/packages" -t dLength of output: 8420
🧹 Nitpick comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
36-40
: Consider adding package.json files to the watch list.While monitoring the source files is good, changes to dependencies in package.json files could also introduce cyclic dependencies.
Add this to the files list:
app/client/packages/ast/** app/client/packages/design-system/** app/client/packages/dsl/** app/client/packages/icons/** app/client/packages/utils/** +app/client/packages/*/package.json
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-client-cyclic-deps-check.yml
(1 hunks)app/client/src/index.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
🔇 Additional comments (1)
.github/workflows/ci-client-cyclic-deps-check.yml (1)
36-40
: LGTM! Good addition of package directories.The inclusion of these package directories will help catch cyclic dependencies across the UI component library ecosystem.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Number
or
Fixes
Issue URL
Warning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD f2a517d yet
Wed, 15 Jan 2025 07:56:57 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit