Conversation
WalkthroughThis change adds new English localization keys in Possibly related PRs
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
dosubot
bot
added
the
🧹 Improvements
|
Related Documentation No published documentation to review for changes on this repository. How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/web/public/static/locales/en/common.json (2)
2736-2740: Normalize capitalization for URL-based labelsKeep labels consistent with existing strings like “Cancel URL” and “No-show Fee”.
Apply this diff to unify capitalization and hyphenation:
- "rating_url_variable": "Rating url", + "rating_url_variable": "Rating URL", - "no_show_url_variable": "No Show url", + "no_show_url_variable": "No-show URL",
2928-2932: Consistent casing for attendee timezone variablesElsewhere we use sentence case for labels (e.g., “Event end time”). Consider matching that style to avoid mixed casing.
- "event_start_time_in_attendee_timezone_variable": "Event Start Time In Attendee Timezone", + "event_start_time_in_attendee_timezone_variable": "Event start time in attendee timezone", - "event_end_time_in_attendee_timezone_variable": "Event End Time In Attendee Timezone", + "event_end_time_in_attendee_timezone_variable": "Event end time in attendee timezone",packages/ui/components/editor/plugins/AddVariablesDropdown.tsx (4)
18-19: Localize aria-label and fix inert items-center
- aria-label should be localized.
- items-center has no effect without display:flex.
- <DropdownMenuTrigger aria-label="Add variable" className="focus:bg-muted pt-[6px]"> - <div className="items-center"> + <DropdownMenuTrigger aria-label={t("add_variable")} className="focus:bg-muted pt-[6px]"> + <div className="flex items-center">
45-45: Width override may not win against default w-[220px] in DropdownMenuContentDropdownMenuContent sets a default width "w-[220px]" internally. Without tw-merge, adding "w-96" here is not guaranteed to override due to Tailwind generation order. Use an explicit override or update the base component to twMerge class collisions.
Minimal local fix:
- <DropdownMenuContent className="w-96"> + <DropdownMenuContent className="!w-96">Preferred global fix in packages/ui/components/dropdown/Dropdown.tsx (outside this hunk) to ensure downstream overrides win:
// Replace classNames with tailwind-merge to de-dupe conflicting utilities import { twMerge } from "tailwind-merge"; // ... className={twMerge( "shadow-dropdown bg-default border-subtle relative z-10 origin-top-right space-y-[1px] rounded-xl border p-1 text-sm", "w-[220px]", props.className )}
53-67: Avoid nested interactive elements; use DropdownMenuItem onSelect or asChildRadix DropdownMenu.Item is interactive. Nesting a button inside it can confuse focus/ARIA and duplicate hover/focus states. Prefer binding selection to the item, or use asChild to render the button as the item.
Option A (bind to item, remove inner button):
- <DropdownMenuItem key={variable} className="hover:ring-0"> - <button - key={variable} - type="button" - className="hover:bg-muted w-full rounded-md px-3 py-2 text-left transition-colors" - onClick={() => - props.addVariable(`{${t(`${variable}_variable`).toUpperCase().replace(/ /g, "_")}}`) - }> - <div className="flex flex-col space-y-1"> + <DropdownMenuItem + key={variable} + className="hover:ring-0 px-3 py-2" + onSelect={() => + props.addVariable(`{${t(`${variable}_variable`).toUpperCase().replace(/ /g, "_")}}`) + }> + <div className="flex flex-col space-y-1 w-full"> ... - </div> - </button> - </DropdownMenuItem> + </div> + </DropdownMenuItem>Option B (asChild) if you want the button element semantics preserved.
58-64: Minor: avoid duplicate t() work and keep the token logic DRYYou call t(
${variable}_variable) twice (display and insertion). Precompute once per item or extract a tiny helper for token formatting.Example:
const label = t(`${variable}_variable`); const token = `{${label.toUpperCase().replace(/ /g, "_")}}`;Then use token for both display and insertion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/web/public/static/locales/en/common.json(5 hunks)packages/ui/components/editor/plugins/AddVariablesDropdown.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/ui/components/editor/plugins/AddVariablesDropdown.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/ui/components/editor/plugins/AddVariablesDropdown.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-15T00:27:33.099Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/components/AgentConfigurationSheet.tsx:348-371
Timestamp: 2025-08-15T00:27:33.099Z
Learning: In calcom/cal.com, variable insertion in both WorkflowStepContainer.tsx (addVariableEmailSubject) and AgentConfigurationSheet.tsx (addVariableToGeneralPrompt) follows the identical pattern: `{${variable.toUpperCase().replace(/ /g, "_")}}`. This is the established, consistent approach across the codebase for transforming variable labels into tokens, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/ui/components/editor/plugins/AddVariablesDropdown.tsx
🧬 Code Graph Analysis (1)
packages/ui/components/editor/plugins/AddVariablesDropdown.tsx (1)
packages/ui/components/dropdown/Dropdown.tsx (2)
DropdownMenuContent(34-51)DropdownMenuItem(63-71)
⏰ 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). (2)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
🔇 Additional comments (2)
apps/web/public/static/locales/en/common.json (2)
1605-1611: *New _variable entries align with UI usage in AddVariablesDropdownKeys for location_variable, additional_notes_variable, and timezone_variable look correct and have matching *_info counterparts. These will resolve cleanly in the dropdown rendering. No issues.
Also applies to: 1719-1722
3333-3333: PBAC description change is fineThe updated copy for pbac_desc_view_workflows reads well and is clearer. No technical impact.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/ui/components/editor/plugins/AddVariablesDropdown.test.tsx (2)
35-41: Strengthen the interaction test by asserting pre-state and using findByTextMake the test actually validate the open interaction by asserting the menu is absent before the click and present after. Using findByText also removes the need for waitFor.
Apply this diff:
it("opens dropdown on click", async () => { render(<AddVariablesDropdown addVariable={mockAddVariable} variables={variables} />); - fireEvent.click(screen.getByText("add_variable")); - await waitFor(() => { - expect(screen.getByText("add_dynamic_variables")).toBeInTheDocument(); - }); + expect(screen.queryByText("add_dynamic_variables")).not.toBeInTheDocument(); + fireEvent.click(screen.getByText("add_variable")); + expect(await screen.findByText("add_dynamic_variables")).toBeInTheDocument(); });
55-62: Tighten assertion on callback and consider isolating mock stateAdd a call-count assertion to ensure a single invocation from the click. Optionally, clear the mock before each test to avoid cross-test leakage.
Apply this diff:
await waitFor(() => { fireEvent.click(screen.getByText((content) => content.includes("{VAR1_VARIABLE}"))); }); + expect(mockAddVariable).toHaveBeenCalledTimes(1); expect(mockAddVariable).toHaveBeenCalledWith("var1_variable");Additionally (outside the selected lines), consider adding this near Line 24 (inside describe) to isolate tests:
beforeEach(() => { mockAddVariable.mockClear(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/ui/components/editor/plugins/AddVariablesDropdown.test.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.tsx
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/ui/components/editor/plugins/AddVariablesDropdown.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/ui/components/editor/plugins/AddVariablesDropdown.test.tsx
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/ui/components/editor/plugins/AddVariablesDropdown.test.tsx (1)
39-39: LGTM: Assertion updated to match CSS-driven capitalizationSwitching the expectation to plain key "add_dynamic_variables" (instead of uppercasing) aligns with the component’s CSS-based uppercase styling. This keeps the test resilient against locale/token-case changes implemented via styles.
E2E results are ready! |
5 participants
What does this PR do?
Before:
After:
