Add missing view after account is deleted #1611
Conversation
WalkthroughAdds a new Next.js account-deleted page rendering a confirmation screen, scopes a Create-button lookup in a draft system test, and replaces direct Escape + hidden-state waits with retrying expect(...).toPass(...) wrappers in chatbot conversation tests (15s timeout). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
services/main-frontend/src/pages/account-deleted.tsx (3)
9-10: Simplify component type declaration.The component doesn't use props or children, so
React.PropsWithChildren<unknown>is unnecessary. Consider simplifying:🔎 Suggested simplification
-const AccountDeletedPage: React.FC<React.PropsWithChildren<unknown>> = () => { +const AccountDeletedPage: React.FC = () => {Or simply use a plain function declaration:
-const AccountDeletedPage: React.FC<React.PropsWithChildren<unknown>> = () => { +const AccountDeletedPage = () => {
19-21: Consider usingprimaryFontfrom theme.As per coding guidelines, if a component uses fonts, prefer theme fonts. The h1 could include the primary font family for consistency.
🔎 Suggested change
import Button from "@/shared-module/common/components/Button" -import { baseTheme } from "@/shared-module/common/styles" +import { baseTheme, primaryFont } from "@/shared-module/common/styles"h1 { font-size: 2rem; + font-family: ${primaryFont}; }
25-30: Addaria-hiddento decorative icon.The Envelope icon is decorative since the adjacent text conveys the message. Adding
aria-hidden="true"improves screen reader experience.🔎 Suggested change
<Envelope + aria-hidden="true" className={css` color: ${baseTheme.colors.green[500]}; margin-right: 0.5rem; `} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/main-frontend/src/pages/account-deleted.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Make sure to use theuseTranslationhook for all text that is visible to the user.
If a component uses colors, use colors from the theme if possible. Example: import { baseTheme } from "@/shared-module/common/styles" ... background: ${baseTheme.colors.clear[100]};
If a component uses media queries, use the respondToOrLarger function. Example: import { respondToOrLarger } from "@/shared-module/common/styles/respond" ... ${respondToOrLarger.sm} { ... }
If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks.
If there is a form of any kind, make sure it uses useForm from react-hook-form, and make sure the form is integrated with the useToastMutation and useQuery hooks.
For loading and error states, use the Spinner and ErrorBanner components from the shared module.
We have better replacements for window.confirm, window.alert, and window.prompt, use the DialogProvider and the useDialog hook from the shared module instead.
For calling the React hooks useMemo and useEffect, don't use the React. prefix.
If a component uses fonts, use fonts from the theme if possible. Example: import { primaryFont } from "@/shared-module/common/styles" ... font-family: ${primaryFont};
Files:
services/main-frontend/src/pages/account-deleted.tsx
🧬 Code graph analysis (1)
services/main-frontend/src/pages/account-deleted.tsx (1)
shared-module/packages/common/src/styles/theme.ts (1)
baseTheme(11-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: code-style
- GitHub Check: headless_lms
- GitHub Check: build-and-system-tests
🔇 Additional comments (1)
services/main-frontend/src/pages/account-deleted.tsx (1)
35-44: LGTM!Good use of the
Linkcomponent for client-side navigation and proper translation hook usage for the button text. The default export is correct for a Next.js page.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
system-tests/src/tests/chatbot-conversation.spec.ts (1)
100-105: Retry pattern appropriately handles popover closure flakiness.The
toPass()wrapper with retry logic is a good approach for handling unstable popover behavior during viewport resizing for screenshots. However, the 15-second timeout seems generous.A few minor considerations:
- If the popover is already closed but the check fails, pressing Escape repeatedly during retries might have unintended side effects (e.g., closing other UI elements)
- Consider reducing the timeout to 10 seconds if this proves sufficient in practice
- If flakiness persists, investigate whether the root cause is animation timing or event propagation delays
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
system-tests/src/tests/chatbot-conversation.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
system-tests/**/*
⚙️ CodeRabbit configuration file
system-tests/**/*: For Playwright system tests, make sure each test that triggers a backend mutation (e.g., clicking a save button) waits for a UI confirmation element that proves the request completed successfully before proceeding. Otherwise, the test might continue before the operation finishes.
Also if someone navigates to a course page, and clicks continue on a dialog straight away, make sure its done with theselectCourseInstanceIfPromptedfunction.
If someone write a tests that depends on a toast staying on the screen, make sure to use theshowNextToastsInfinitely, andshowToastsNormallyfunctions. If those are used in conjunction withexpectScreenshotsToMatchSnapshots, you should set its argument ofdontWaitForSpinnersToDisappearto true.
For dialog interactions, use the helper functions fromsystem-tests/src/utils/dialogs.ts: - UsedismissAlertDialogfor alert dialogs - UserespondToConfirmDialogfor confirm dialogs (Yes/No) - UsefillPromptDialogfor prompt dialogs with input fields
NEVER select elements by auto-generated CSS classes (e.g.,.locator(".css-xknul2-TopBar")). Instead, use semantic selectors like data-testid attributes, role selectors, text content, or accessible names. Usepage.getByTestId(),page.getByRole(),page.getByText(), orpage.getByLabel()instead.
Files:
system-tests/src/tests/chatbot-conversation.spec.ts
🧠 Learnings (1)
📚 Learning: 2025-09-30T06:35:05.964Z
Learnt from: akirataguchi115
Repo: rage/secret-project-331 PR: 1561
File: system-tests/src/tests/material-reference.spec.ts:189-200
Timestamp: 2025-09-30T06:35:05.964Z
Learning: In Playwright tests, `.hover()` and other action methods automatically wait for elements to be visible and stable before performing the action. Explicit visibility waits before hover actions are not necessary.
Applied to files:
system-tests/src/tests/chatbot-conversation.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-deploy
- GitHub Check: build-and-system-tests
- GitHub Check: code-style
- GitHub Check: headless_lms
🔇 Additional comments (1)
system-tests/src/tests/chatbot-conversation.spec.ts (1)
200-205: Consistent application of retry pattern.Good consistency with the same pattern applied at lines 100-105. The same considerations about timeout duration and multiple Escape presses apply here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/main-frontend/src/pages/account-deleted.tsx (1)
14-22: Consider using theme values for sizing and spacing.The component uses hardcoded values for font-size (
2rem), padding (3rem), and margin (1rem). ThebaseThemeprovidesfontSizesandspacescales that could be used for consistency across the app.Example using theme values:
className={css` background-color: ${baseTheme.colors.green[100]}; - padding: 3rem; - margin-bottom: 1rem; + padding: ${baseTheme.space[6]}; + margin-bottom: ${baseTheme.space[4]}; h1 { - font-size: 2rem; + font-size: ${baseTheme.fontSizes[5]}; } `}Note: Adjust the scale indices based on your theme's actual values.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/main-frontend/src/pages/account-deleted.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Make sure to use theuseTranslationhook for all text that is visible to the user.
If a component uses colors, use colors from the theme if possible. Example: import { baseTheme } from "@/shared-module/common/styles" ... background: ${baseTheme.colors.clear[100]};
If a component uses media queries, use the respondToOrLarger function. Example: import { respondToOrLarger } from "@/shared-module/common/styles/respond" ... ${respondToOrLarger.sm} { ... }
If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks.
If there is a form of any kind, make sure it uses useForm from react-hook-form, and make sure the form is integrated with the useToastMutation and useQuery hooks.
For loading and error states, use the Spinner and ErrorBanner components from the shared module.
We have better replacements for window.confirm, window.alert, and window.prompt, use the DialogProvider and the useDialog hook from the shared module instead.
For calling the React hooks useMemo and useEffect, don't use the React. prefix.
If a component uses fonts, use fonts from the theme if possible. Example: import { primaryFont } from "@/shared-module/common/styles" ... font-family: ${primaryFont};
Files:
services/main-frontend/src/pages/account-deleted.tsx
🧬 Code graph analysis (1)
services/main-frontend/src/pages/account-deleted.tsx (1)
shared-module/packages/common/src/styles/theme.ts (1)
baseTheme(11-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-deploy
- GitHub Check: build-and-system-tests
- GitHub Check: headless_lms
- GitHub Check: code-style
🔇 Additional comments (1)
services/main-frontend/src/pages/account-deleted.tsx (1)
9-42: Implementation looks good.The component correctly follows the coding guidelines: uses
useTranslationfor user-facing text, leverages theme colors, and has a clean, straightforward structure appropriate for a confirmation page.
| <Envelope | ||
| className={css` | ||
| color: ${baseTheme.colors.green[500]}; | ||
| margin-right: 0.5rem; | ||
| `} | ||
| /> |
There was a problem hiding this comment.
Add aria-hidden="true" to decorative icon.
The Envelope icon is decorative within the heading. For better accessibility, mark it as hidden from screen readers since the text already conveys the meaning.
🔎 Apply this diff:
<Envelope
+ aria-hidden="true"
className={css`
color: ${baseTheme.colors.green[500]};
margin-right: 0.5rem;
`}
/>📝 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.
| <Envelope | |
| className={css` | |
| color: ${baseTheme.colors.green[500]}; | |
| margin-right: 0.5rem; | |
| `} | |
| /> | |
| <Envelope | |
| aria-hidden="true" | |
| className={css` | |
| color: ${baseTheme.colors.green[500]}; | |
| margin-right: 0.5rem; | |
| `} | |
| /> |
🤖 Prompt for AI Agents
In services/main-frontend/src/pages/account-deleted.tsx around lines 25 to 30,
the Envelope icon is decorative but not hidden from assistive technology; add
aria-hidden="true" to the Envelope component (or role="presentation") so screen
readers ignore it while keeping the visible markup unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
system-tests/src/tests/draft.spec.ts (1)
118-121: Good improvement, but consider applying the same pattern to other Create button clicks.The scoped selection using
getByRole("region")followed bygetByRole("button")with exact matching is more robust than the generic selectors used elsewhere in this file.However, there's an inconsistency:
- Line 70 uses
button:text("Create")(generic, unscoped)- Lines 149 and 174 use
.getByRole("button", { name: "Create" }).first()(relies on ordering)Consider applying the same scoped approach to these other Create button clicks for consistency and to avoid potential flakiness if multiple Create buttons exist on those pages.
🔎 View suggested refactor for line 70
- await page.click(`button:text("Create")`) + await page + .getByRole("region", { name: "Courses:" }) + .getByRole("button", { name: "Create", exact: true }) + .click()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
system-tests/src/tests/draft.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
system-tests/**/*
⚙️ CodeRabbit configuration file
system-tests/**/*: For Playwright system tests, make sure each test that triggers a backend mutation (e.g., clicking a save button) waits for a UI confirmation element that proves the request completed successfully before proceeding. Otherwise, the test might continue before the operation finishes.
Also if someone navigates to a course page, and clicks continue on a dialog straight away, make sure its done with theselectCourseInstanceIfPromptedfunction.
If someone write a tests that depends on a toast staying on the screen, make sure to use theshowNextToastsInfinitely, andshowToastsNormallyfunctions. If those are used in conjunction withexpectScreenshotsToMatchSnapshots, you should set its argument ofdontWaitForSpinnersToDisappearto true.
For dialog interactions, use the helper functions fromsystem-tests/src/utils/dialogs.ts: - UsedismissAlertDialogfor alert dialogs - UserespondToConfirmDialogfor confirm dialogs (Yes/No) - UsefillPromptDialogfor prompt dialogs with input fields
NEVER select elements by auto-generated CSS classes (e.g.,.locator(".css-xknul2-TopBar")). Instead, use semantic selectors like data-testid attributes, role selectors, text content, or accessible names. Usepage.getByTestId(),page.getByRole(),page.getByText(), orpage.getByLabel()instead.
Files:
system-tests/src/tests/draft.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-and-deploy
- GitHub Check: build-and-system-tests
- GitHub Check: headless_lms
- GitHub Check: code-style
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.