fix(web): add placeholder for no model on overview page #1309
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request enhance the functionality and internationalization of the Re:Earth CMS application. Key modifications include improved test cases for model creation and deletion, updates to the Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
web/e2e/project/overview.spec.ts (2)
16-17: LGTM! Consider making button selector more specificThe changes properly verify the placeholder visibility both before model creation and after deletion. However, using
.first()for button selection suggests multiple "New Model" buttons exist, which could make the test brittle.Consider using a more specific selector to target the exact button:
-await page.getByRole("button", { name: "plus New Model" }).first().click(); +await page.getByRole("button", { name: "plus New Model" }).filter({ hasText: "New Model" }).click();Also applies to: 48-48
Line range hint
6-13: Improve test isolation in setup/teardownThe current setup doesn't ensure a clean state for model testing. Consider enhancing the test isolation:
- Ensure no models exist in the beforeEach hook
- Clean up any created models in the afterEach hook
- Add test isolation flags to prevent parallel execution
test.beforeEach(async ({ reearth, page }) => { await reearth.goto("/", { waitUntil: "domcontentloaded" }); await createProject(page); + // Ensure no models exist + await expect(page.getByText("No Models yet")).toBeVisible(); }); test.afterEach(async ({ page }) => { + // Clean up any created models + const modelExists = await page.getByRole("menuitem").count() > 0; + if (modelExists) { + await page.getByRole("menuitem").click(); + await page.getByText("Delete").click(); + await page.getByRole("button", { name: "Delete Model" }).click(); + await closeNotification(page); + } await deleteProject(page); }); +// Prevent parallel execution +test.describe.configure({ mode: 'serial' });web/src/components/molecules/ProjectOverview/index.tsx (2)
74-81: Consider consolidating duplicate "New Model" buttonsThere are two "New Model" buttons with identical functionality - one in the header and one in the placeholder. This might confuse users and violates the DRY principle.
Consider showing only the placeholder button when there are no models, and only the header button when models exist:
-headerActions={ - <Button - type="primary" - icon={<Icon icon="plus" />} - onClick={onModelModalOpen} - disabled={!hasCreateRight}> - {t("New Model")} - </Button> -} +headerActions={models?.length ? ( + <Button + type="primary" + icon={<Icon icon="plus" />} + onClick={onModelModalOpen} + disabled={!hasCreateRight}> + {t("New Model")} + </Button> +) : null}
103-128: Enhance styling consistencyWhile the styled components are well-structured, there are a few improvements that could be made for better maintainability:
- Consider using design system tokens for consistent spacing and colors:
const Content = styled.div` display: flex; flex-direction: column; align-items: center; - gap: 12px; - color: rgba(0, 0, 0, 0.45); + gap: ${theme.spacing.md}; + color: ${theme.colors.textSecondary}; `;
- Consider extracting common spacing values into theme constants for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
web/e2e/project/overview.spec.ts(2 hunks)web/src/components/molecules/Integration/IntegrationTable/index.tsx(2 hunks)web/src/components/molecules/MyIntegrations/Webhook/WebhookList/index.tsx(2 hunks)web/src/components/molecules/ProjectList/ProjectList.tsx(2 hunks)web/src/components/molecules/ProjectOverview/index.tsx(3 hunks)web/src/i18n/translations/en.yml(2 hunks)web/src/i18n/translations/ja.yml(2 hunks)
🔇 Additional comments (10)
web/src/components/molecules/MyIntegrations/Webhook/WebhookList/index.tsx (1)
6-6: LGTM!
The addition of the Trans import alongside useT is appropriate for the enhanced translation implementation.
web/src/components/molecules/ProjectList/ProjectList.tsx (2)
8-8: LGTM: Clean import statement addition
The addition of the Trans component import is clean and properly aligned with the internationalization improvements.
45-45:
Add the missing documentation URL
The anchor tag's href attribute is empty, which makes the documentation link non-functional. Please add the appropriate documentation URL.
- <Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />
+ <Trans i18nKey="readDocument" components={{ l: <a href="https://docs.reearth.io/cms/..." target="_blank" rel="noopener noreferrer" /> }} />Let's verify if the documentation URL exists in other files:
web/src/components/molecules/ProjectOverview/index.tsx (1)
8-8: LGTM: Import changes are appropriate
The addition of the Trans component import is correct and aligns with its usage in the documentation link section.
web/src/components/molecules/Integration/IntegrationTable/index.tsx (2)
18-18: LGTM: Clean import addition
The addition of the Trans component import alongside useT is a good practice for handling translations with embedded components.
206-208: Verify the PR objective alignment
The changes appear to be updating the empty state in the IntegrationTable component, but the PR objective mentions "add placeholder for no model on overview page". Could you verify if:
- These changes are intended for this component
- Additional changes are needed in the overview page
- The PR description needs to be updated
web/src/i18n/translations/en.yml (2)
275-276: LGTM! Translation keys align with PR objectives
The new translation keys "No Models yet" and "Create a new model" provide clear messaging for the empty state, directly addressing the PR objective of adding a placeholder for no model on the overview page.
212-212: Verify HTML tag rendering implementation
The translation key uses an HTML-like <l> tag. Ensure that the component rendering this translation properly handles these tags.
✅ Verification successful
HTML tag rendering is properly implemented
The <l> tag is correctly handled by the Trans component across multiple components. The implementation shows consistent usage where the tag is properly mapped to an anchor element:
ProjectOverview/index.tsx:<Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />IntegrationTable/index.tsx:<Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />ProjectList/ProjectList.tsx:<Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />WebhookList/index.tsx:<Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Trans component usage with readDocument key
rg -A 2 "readDocument" --type ts --type tsx
Length of output: 71
Script:
#!/bin/bash
# Search for readDocument key usage in all files
rg -A 2 "readDocument"
# Search for components that might handle <l> tags
rg -A 5 "<l>" --type ts
rg -A 5 "<l>" --type typescript
Length of output: 5074
web/src/i18n/translations/ja.yml (2)
275-276: LGTM! Translations align with PR objectives.
The translations for "No Models yet" and "Create a new model" are grammatically correct and consistent with the application's terminology. These additions properly support the PR's objective of adding placeholders for no model scenarios.
212-212: LGTM! Verify tag handling in the translation system.
The translation is correct, but ensure that the <l> tags are properly handled by the translation system and rendered correctly in the UI.
✅ Verification successful
Based on the search results, I can see that the <l> tags are properly handled by the translation system using the Trans component from react-i18next. The usage is consistent across files, and there are three instances where it's used with the "readDocument" translation key:
web/src/components/molecules/ProjectOverview/index.tsx:<Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />web/src/components/molecules/ProjectList/ProjectList.tsx:<Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />web/src/components/molecules/Integration/IntegrationTable/index.tsx:<Trans i18nKey="readDocument" components={{ l: <a href="" /> }} />
The translation system is properly set up with i18next and react-i18next, and the <l> tags are correctly transformed into anchor (<a>) elements in the UI.
The translation and tag handling are properly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of <l> tags in translations
# Test: Search for similar tag usage in other translation files
rg -g '*.yml' '<l>'
Length of output: 197
Script:
#!/bin/bash
# Check how these <l> tags are processed in the codebase
ast-grep --pattern 'const $_ = $$.replace(/<l>.*?<\/l>/g, $_)'
# Also check for any component or utility that handles these tags
rg -g '*.{ts,tsx,js,jsx}' '<l>' -B 2 -A 2
Length of output: 108
Script:
#!/bin/bash
# Look for translation handling components or utilities
rg -g '*.{ts,tsx,js,jsx}' -i 'translate|i18n' -B 2 -A 2
# Check for any link or text processing utilities
fd -e ts -e tsx -e js -e jsx --exec grep -l -i "link\|text\|format"
Length of output: 101197
web/src/components/molecules/MyIntegrations/Webhook/WebhookList/index.tsx
Show resolved
Hide resolved
Overview
This PR adds the placeholder for no model on overview page.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Transcomponent for better translation handling in various components.Bug Fixes