fix: improve project-switcher data-testid for uniqueness and special chars#578
Conversation
…chars
The data-testid generation was using only the sanitized project name which
could produce collisions and didn't handle special characters properly.
Changes:
- Combine stable project.id with sanitized name: project-switcher-{id}-{name}
- Expand sanitization to remove non-alphanumeric chars (except hyphens)
- Collapse multiple hyphens and trim leading/trailing hyphens
- Update E2E tests to use ends-with selector for matching
This ensures test IDs are deterministic, unique, and safe for CSS selectors.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe changes refactor the project switcher item's test ID generation to include both a project ID and a sanitized version of the project name, creating a more unique identifier ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @stefandevo, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request improves the data-testid for project switcher items by including a sanitized project name, which is a great step for test stability. The implementation in ProjectSwitcherItem is solid. However, the E2E tests have been updated in a way that will fail if project names contain special characters, uppercase letters, or spaces, as they don't apply the same sanitization logic. I've provided suggestions to fix this in the test files by replicating the sanitization logic. For better long-term maintainability, I also recommend extracting this sanitization logic into a shared utility function.
Address PR review comments by: - Creating shared sanitizeForTestId utility in apps/ui/src/lib/utils.ts - Updating ProjectSwitcherItem to use the shared utility - Adding matching helper to test utils for E2E tests - Updating all E2E tests to use the sanitization helper This ensures the component and tests use identical sanitization logic, making tests robust against project names with special characters.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/tests/projects/open-existing-project.spec.ts (1)
85-111: Stabilize the settings route handler to avoid disposed-response failures.The pipeline error (
apiResponse.json: Response has been disposed) points at theroute.fetch()→response.json()flow. Add a defensive fallback so the test doesn’t hard-fail if the response is disposed or aborted.💡 Proposed fix
await page.route('**/api/settings/global', async (route) => { - const response = await route.fetch(); - const json = await response.json(); + const response = await route.fetch(); + let json; + try { + json = await response.json(); + } catch { + await route.continue(); + return; + } if (json.settings) { // Remove currentProjectId to prevent restoring a project json.settings.currentProjectId = null; @@ await route.fulfill({ status: response.status(), headers: response.headers(), json, }); });
🤖 Fix all issues with AI agents
In
`@apps/ui/src/components/layout/project-switcher/components/project-switcher-item.tsx`:
- Around line 40-55: The sanitizedName computation can produce an empty string
(e.g., project.name contains only symbols/whitespace), causing testId
(`project-switcher-${project.id}-${sanitizedName}`) to end with a trailing
hyphen; update the code around sanitizedName and testId to handle that case by
using a fallback: after computing sanitizedName, if it's empty either set a
default token (e.g., 'untitled' or 'unknown') or build testId conditionally so
you only append `-${sanitizedName}` when sanitizedName.length > 0; adjust the
variables named sanitizedName and testId in project-switcher-item.tsx (and the
analogous occurrence at the other location) accordingly.
♻️ Duplicate comments (2)
apps/ui/tests/projects/new-project-creation.spec.ts (1)
81-83: Align selector with sanitized test-id format.Same concern as the open-existing test: the selector should use the sanitized name to match
project-switcher-{id}-{sanitizedName}.apps/ui/tests/features/feature-manual-review-flow.spec.ts (1)
134-136: Align selector with sanitized test-id format.Same concern as the other updated tests: use a sanitized project name (or shared helper) when building the ends-with selector.
Summary
project.idwith sanitized name for unique, deterministic test IDsThis addresses the CodeRabbitAI feedback on PR #574 regarding potential test-id collisions and special character handling.
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.