Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 27 minutes and 0 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughSign-in callbacks no longer accept a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run journeys-e2e:e2e |
❌ Failed | 36s | View ↗ |
nx run journeys-admin-e2e:e2e |
✅ Succeeded | 28s | View ↗ |
nx run watch-e2e:e2e |
✅ Succeeded | 22s | View ↗ |
nx run resources-e2e:e2e |
✅ Succeeded | 16s | View ↗ |
nx run videos-admin-e2e:e2e |
✅ Succeeded | 4s | View ↗ |
nx run-many --target=vercel-alias --projects=watch |
✅ Succeeded | 2s | View ↗ |
nx run-many --target=vercel-alias --projects=re... |
✅ Succeeded | 2s | View ↗ |
nx run-many --target=vercel-alias --projects=jo... |
✅ Succeeded | 2s | View ↗ |
Additional runs (12) |
✅ Succeeded | ... | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-04-24 01:59:02 UTC
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccountCheckDialog.tsx (1)
54-71: Optional: passhandleSignIndirectly.Now that both buttons invoke the same no-arg callback, the
() => handleSignIn()wrapper can be simplified toonClick={handleSignIn}(MUI Button'sMouseEventarg will simply be ignored by a() => voidhandler).♻️ Proposed simplification
- onClick={() => handleSignIn()} + onClick={handleSignIn} ... - onClick={() => handleSignIn()} + onClick={handleSignIn}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccountCheckDialog.tsx` around lines 54 - 71, Both MUI Buttons in AccountCheckDialog currently use an unnecessary arrow wrapper on the click handler; replace onClick={() => handleSignIn()} with onClick={handleSignIn} on the Button elements (the startIcon/props stay the same) so the no-arg callback handleSignIn is passed directly and avoids creating extra closures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@libs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccountCheckDialog.tsx`:
- Around line 54-71: Both MUI Buttons in AccountCheckDialog currently use an
unnecessary arrow wrapper on the click handler; replace onClick={() =>
handleSignIn()} with onClick={handleSignIn} on the Button elements (the
startIcon/props stay the same) so the no-arg callback handleSignIn is passed
directly and avoids creating extra closures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e7dce83e-25f0-4726-9abc-84d3a34b1e37
📒 Files selected for processing (7)
apps/journeys-admin/src/components/SignIn/SignInTabs/SignInTabs.tsxlibs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccountCheckDialog.tsxlibs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccoutCheckDialog.spec.tsxlibs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.spec.tsxlibs/journeys/ui/src/components/TemplateView/CreateJourneyButton/CreateJourneyButton.tsxlibs/journeys/ui/src/components/TemplateView/UseThisTemplateButton/UseThisTemplateButton.spec.tsxlibs/journeys/ui/src/components/TemplateView/UseThisTemplateButton/UseThisTemplateButton.tsx
|
The latest updates on your projects.
|
|
Found 1 test failure on Blacksmith runners: Failure
|
jianwei1
left a comment
jianwei1
left a comment
There was a problem hiding this comment.
Review summary
Nice, focused fix — the useEffect([router]) root cause is called out cleanly and the callback simplification propagates nicely through callers + tests. No blocking issues. Posting as a comment review; a few items to consider before merge (see inline).
Concerns:
- No regression test for the reported bug — the solution doc itself lists the two tests that would guard it.
- Solution doc frontmatter (
components:) and prose reference wrong paths forAccountCheckDialog/CreateJourneyButton/UseThisTemplateButton(they live underlibs/journeys/ui/src/components/TemplateView/, notapps/journeys-admin/src/components/). onChangehandler went from namedhandleTabChangeto an inline anonymous arrow — minor backslide vsapps/journeys-admin/AGENTS.md"event handlers prefix withhandle".AccountCheckDialog"Login with my account" no longer pre-selects the Log In tab — intentional per the doc, but worth confirming with product/QA on NES-1599 so it isn't later flagged as a regression.
FYI: Playwright E2E (journeys) failed on this run — likely unrelated, worth a re-run before merge.
- SignInTabs: extract named handleTabChange function
- AccountCheckDialog: simplify onClick to onClick={handleSignIn}
- AccoutCheckDialog.spec: drop redundant toHaveBeenCalledWith() assertions
- SignInTabs.spec: add regression tests for tab stability after re-render
- Solution doc: correct component paths in frontmatter and Investigation prose
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/SignIn/SignInTabs/SignInTabs.spec.tsx (1)
22-41: Good regression coverage for local tab state.These two re-render tests correctly guard against a regression where a router-driven effect could reset the selected tab. Since
rerenderre-invokes the component (and thus anyuseEffects), a reintroduction of the oldrouter.query-syncing effect would make these tests fail, which is exactly the behavior you want to lock in.One small observation: the two cases are functionally equivalent (a single
rerenderalready re-runs effects just as well as three consecutive ones), so the second test doesn't add much signal beyond the first. If you want to preserve both intents, consider collapsing them or explicitly simulating the scenario the second test is named after (e.g., a shallow route update) — otherwise the "repeated re-renders" case is essentially a duplicate. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/journeys-admin/src/components/SignIn/SignInTabs/SignInTabs.spec.tsx` around lines 22 - 41, The two tests in SignInTabs.spec.tsx ("should retain tab selection after re-render (regression: router effect reset)" and "should not reset tab selection on repeated re-renders (regression: shallow route update)") are functionally duplicate because repeated rerender() already re-runs effects; either remove the second test or replace it to explicitly simulate a shallow route update: keep the first test as-is using render/ fireEvent/ rerender on SignInTabs, and for the second test mock the router (next/router) and perform a shallow route update (e.g., call router.replace or update router.query with shallow: true or dispatch the appropriate router event) then assert the selected tab remains "Log In" so the test actually verifies router-driven shallow updates rather than redundant rerenders.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/journeys-admin/src/components/SignIn/SignInTabs/SignInTabs.spec.tsx`:
- Around line 22-41: The two tests in SignInTabs.spec.tsx ("should retain tab
selection after re-render (regression: router effect reset)" and "should not
reset tab selection on repeated re-renders (regression: shallow route update)")
are functionally duplicate because repeated rerender() already re-runs effects;
either remove the second test or replace it to explicitly simulate a shallow
route update: keep the first test as-is using render/ fireEvent/ rerender on
SignInTabs, and for the second test mock the router (next/router) and perform a
shallow route update (e.g., call router.replace or update router.query with
shallow: true or dispatch the appropriate router event) then assert the selected
tab remains "Log In" so the test actually verifies router-driven shallow updates
rather than redundant rerenders.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ade6a4d-c97a-400c-928f-d61c7aad078f
📒 Files selected for processing (5)
apps/journeys-admin/src/components/SignIn/SignInTabs/SignInTabs.spec.tsxapps/journeys-admin/src/components/SignIn/SignInTabs/SignInTabs.tsxdocs/solutions/ui-bugs/mui-tabs-snapping-to-first-tab-router-sync.mdlibs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccountCheckDialog.tsxlibs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccoutCheckDialog.spec.tsx
✅ Files skipped from review due to trivial changes (2)
- libs/journeys/ui/src/components/TemplateView/AccountCheckDialog/AccoutCheckDialog.spec.tsx
- docs/solutions/ui-bugs/mui-tabs-snapping-to-first-tab-router-sync.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/journeys-admin/src/components/SignIn/SignInTabs/SignInTabs.tsx
|
Regression tests: Added two tests in Tab pre-selection change: Confirmed intentional — documented in the solution doc as a deliberate tradeoff. The tabs don't change the underlying sign-in flow (real branching happens post-email-submit via |
2 participants
Summary by CodeRabbit
Refactor
loginquery parameter from sign-in redirects.Bug Fix / Behavior
Tests
loginquery checks and assert the no-arg handler is invoked exactly once.Documentation