Conversation
|
Warning Rate limit exceeded@peterzimon has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 48 seconds before requesting another review. ⌛ 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 (3)
WalkthroughUpdated filter UI and behavior in Shade and Stats. In Shade (apps/shade/src/components/ui/filters.tsx): cleared i18n defaults for addFilter/addFilterTitle, adjusted small input padding, changed non-editable operator border to use a full border with zero-right width, updated popover width selection to prefer selectedFieldForOptions?.className then popoverContentClassName with a w-[200px] fallback, added field-level option autoCloseOnSelect, and made SelectOptionsPopover clear search on close. In Stats: converted UTM/source badges to compact counts and hid selected options, set audience defaultOperator to "is any of" with hideOperatorSelect and autoCloseOnSelect, constrained post popover width and changed icon to PenLine, updated Filters usage (FunnelPlus, empty add text, size sm), and adjusted header vertical padding. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 1
🧹 Nitpick comments (3)
apps/stats/src/views/Stats/components/stats-filter.tsx (1)
489-489: Simplify the ternary expression.The ternary operator evaluates to an empty string in both branches. This can be simplified to just an empty string.
Apply this diff:
- addButtonText={filters.length ? '' : ''} + addButtonText=''apps/shade/src/components/ui/filters.tsx (1)
944-944: Use idiomatic Tailwind utility for border removal.The arbitrary value
border-r-[0px]should be replaced with the built-in Tailwind utilityborder-r-0for better maintainability and consistency with Tailwind conventions.Apply this diff:
- <div className="flex items-center self-stretch border border-r-[0px] px-3 text-sm text-muted-foreground"> + <div className="flex items-center self-stretch border border-r-0 px-3 text-sm text-muted-foreground">Based on learnings, ensure Tailwind code follows framework conventions.
apps/stats/src/views/Stats/layout/stats-header.tsx (1)
101-105: Remove commented code or add tracking reference.The Locations menu item is commented out. If this removal is permanent, please delete the commented block. If it's temporary, add a TODO comment with a tracking reference.
If permanent, apply this diff:
- {/* {appSettings?.analytics.webAnalytics && ( - <PageMenuItem value="/analytics/locations/" onClick={() => { - navigate('/analytics/locations/'); - }}>Locations</PageMenuItem> - )} */}If temporary, add context:
- {/* {appSettings?.analytics.webAnalytics && ( + {/* TODO(NY-768): Temporarily hidden pending design review + {appSettings?.analytics.webAnalytics && ( <PageMenuItem value="/analytics/locations/" onClick={() => { navigate('/analytics/locations/'); }}>Locations</PageMenuItem> )} */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/shade/src/components/ui/filters.tsx(4 hunks)apps/shade/src/components/ui/pagemenu.tsx(3 hunks)apps/stats/src/views/Stats/components/stats-filter.tsx(4 hunks)apps/stats/src/views/Stats/layout/stats-header.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/shade/src/components/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
Atomic UI components should be placed in
src/components/ui/*and each component must have a corresponding*.stories.tsxfile next to it for Storybook documentation
Files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
apps/shade/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
apps/shade/src/components/**/*.{ts,tsx}: UsePascalCasefor component identifiers in filenames while keeping ShadCN-generated files in kebab-case
Always forward and mergeclassNameprop withcn(...)utility function
Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Prefer compound subcomponents (e.g.,Header.Title,Header.Meta,Header.Actions) for multi-region components instead of many props
Use Tailwind CSS scoped via.shadeclass; dark mode uses.dark
Files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
apps/shade/src/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
apps/shade/src/**/*.{ts,tsx,js}: UsecamelCasefor function and variable names
Use the@alias for internal imports (e.g.,@/lib/utils)
Files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
apps/shade/{src,test}/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
apps/shade/{src,test}/**/*.{ts,tsx,js}: Runyarn lintafter making changes to fix any ESLint errors and warnings before committing
Follow ESLint andtailwindcss/*plugin rules when writing styles
Files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use Tailwind CSS scoped via `.shade` class; dark mode uses `.dark`
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Use `shade` design system for new components in Admin UI (avoid legacy `admin-x-design-system`)
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Prefer compound subcomponents (e.g., `Header.Title`, `Header.Meta`, `Header.Actions`) for multi-region components instead of many props
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/ui/**/*.{ts,tsx} : Atomic UI components should be placed in `src/components/ui/*` and each component must have a corresponding `*.stories.tsx` file next to it for Storybook documentation
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Applied to files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
Applied to files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use `PascalCase` for component identifiers in filenames while keeping ShadCN-generated files in kebab-case
Applied to files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use Tailwind CSS scoped via `.shade` class; dark mode uses `.dark`
Applied to files:
apps/shade/src/components/ui/filters.tsxapps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
apps/shade/src/components/ui/filters.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/layout/**/*.{ts,tsx} : Reusable layout containers (Page, Heading, Header, ViewHeader, ErrorPage) should be placed in `src/components/layout/*`
Applied to files:
apps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Prefer compound subcomponents (e.g., `Header.Title`, `Header.Meta`, `Header.Actions`) for multi-region components instead of many props
Applied to files:
apps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/ui/**/*.{ts,tsx} : Atomic UI components should be placed in `src/components/ui/*` and each component must have a corresponding `*.stories.tsx` file next to it for Storybook documentation
Applied to files:
apps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/features/**/*.{ts,tsx} : Higher-level, opinionated components (e.g., PostShareModal, SourceTabs) should be placed in `src/components/features/*`
Applied to files:
apps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/lib/utils.ts : Shared utilities (class merging, formatting, chart helpers) should be centralized in `src/lib/utils.ts`
Applied to files:
apps/shade/src/components/ui/pagemenu.tsx
📚 Learning: 2025-11-25T11:58:51.652Z
Learnt from: ibalosh
Repo: TryGhost/Ghost PR: 25525
File: apps/shade/src/shade-app.tsx:4-4
Timestamp: 2025-11-25T11:58:51.652Z
Learning: In apps/shade, the app wrapper file should be named `src/shade-app.tsx` (kebab-case) while the component itself is exported as `ShadeApp` (PascalCase). Context providers should be placed in `src/providers/*` using kebab-case filenames.
Applied to files:
apps/shade/src/components/ui/pagemenu.tsx
🧬 Code graph analysis (2)
apps/shade/src/components/ui/pagemenu.tsx (1)
apps/shade/src/index.ts (1)
cn(82-82)
apps/stats/src/views/Stats/layout/stats-header.tsx (2)
apps/shade/src/components/ui/navbar.tsx (1)
Navbar(46-46)apps/shade/src/components/ui/pagemenu.tsx (2)
PageMenu(227-227)PageMenuItem(227-227)
⏰ 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). (5)
- GitHub Check: ActivityPub tests
- GitHub Check: Admin-X Settings tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (5)
apps/stats/src/views/Stats/components/stats-filter.tsx (1)
447-449: LGTM! Configuration enhancements improve UX.The addition of
defaultOperatorandhideOperatorSelectstreamlines the audience filter UI by establishing a clear default and removing unnecessary operator selection controls.apps/shade/src/components/ui/filters.tsx (1)
239-239: LGTM! Padding adjustment improves visual consistency.The horizontal padding increase for the small variant aligns with the filter UI refinement objectives.
apps/shade/src/components/ui/pagemenu.tsx (1)
51-51: LGTM - Consistent spacing refinement.The gap value updates are synchronized across the constant (line 51), layout calculations, and all container utilities. The change from
gap-1.5(6px) togap-2(8px) provides slightly more breathing room between menu items.Also applies to: 145-145, 147-147
apps/stats/src/views/Stats/layout/stats-header.tsx (2)
67-67: LGTM - Refined navbar spacing.The padding adjustment from uniform
py-8to asymmetricpb-6 pt-9provides better visual balance in the header layout.
69-99: LGTM - Consistent icon enhancements.The addition of icons (Gauge, Globe, Mail, Sprout) to the navigation menu items enhances visual clarity and provides better wayfinding. The implementation is consistent across all items and properly preserves conditional rendering based on app settings.
peterzimon
force-pushed
the
analytics-filtering-refinements
branch
from
47d31e0 to
8e26e75
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/stats/src/views/Stats/components/stats-filter.tsx (1)
491-491: Simplify redundant ternary expression.Both branches of the ternary return the same value (empty string), making the condition unnecessary.
Apply this diff to simplify:
- addButtonText={filters.length ? '' : ''} + addButtonText=''
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/shade/src/components/ui/filters.tsx(5 hunks)apps/stats/src/views/Stats/components/stats-filter.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/shade/src/components/ui/filters.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use Tailwind CSS scoped via `.shade` class; dark mode uses `.dark`
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Use `shade` design system for new components in Admin UI (avoid legacy `admin-x-design-system`)
📚 Learning: 2025-11-12T22:26:48.725Z
Learnt from: troyciesco
Repo: TryGhost/Ghost PR: 25271
File: apps/stats/src/views/Stats/Growth/Growth.tsx:199-204
Timestamp: 2025-11-12T22:26:48.725Z
Learning: In apps/stats/src/views/Stats/Growth/Growth.tsx, the loading skeleton for the top posts/pages table intentionally uses colSpan={1} to only span the first (wider) column, creating a visual effect where the skeleton appears only in the main content column (which contains post/page titles with dates).
Applied to files:
apps/stats/src/views/Stats/components/stats-filter.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). (5)
- GitHub Check: Admin-X Settings tests
- GitHub Check: ActivityPub tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (3)
apps/stats/src/views/Stats/components/stats-filter.tsx (3)
447-449: LGTM! Audience field configuration improvements.The addition of
defaultOperator: 'is any of'andhideOperatorSelect: trueappropriately simplifies the UI for the multiselect audience field by hiding the operator selector when only one operator is semantically relevant.
463-464: LGTM! Explicit width control for post selector.The fixed width classes (
w-80= 320px) provide consistent layout for the post/page selector and its popover content, which is appropriate for displaying post titles clearly.
490-490: LGTM! UI refinements for the Filters component.The icon change to
FunnelPlus, button ordering via[&>button]:order-last, and small size variant appropriately refine the filter UI presentation.Also applies to: 492-492, 496-496
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/stats/src/views/Stats/components/stats-filter.tsx (1)
497-497: Simplify redundant ternary.Both branches of the ternary return the same empty string. Simplify to just
addButtonText=''or remove if empty string is the default.- addButtonText={filters.length ? '' : ''} + addButtonText=''
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/stats/src/views/Stats/components/stats-filter.tsx(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use Tailwind CSS scoped via `.shade` class; dark mode uses `.dark`
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/lib/utils.ts : Shared utilities (class merging, formatting, chart helpers) should be centralized in `src/lib/utils.ts`
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Use `shade` design system for new components in Admin UI (avoid legacy `admin-x-design-system`)
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Prefer compound subcomponents (e.g., `Header.Title`, `Header.Meta`, `Header.Actions`) for multi-region components instead of many props
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use `PascalCase` for component identifiers in filenames while keeping ShadCN-generated files in kebab-case
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/features/**/*.{ts,tsx} : Higher-level, opinionated components (e.g., PostShareModal, SourceTabs) should be placed in `src/components/features/*`
Learnt from: ibalosh
Repo: TryGhost/Ghost PR: 25525
File: apps/shade/src/shade-app.tsx:4-4
Timestamp: 2025-11-25T11:58:51.652Z
Learning: In apps/shade, the app wrapper file should be named `src/shade-app.tsx` (kebab-case) while the component itself is exported as `ShadeApp` (PascalCase). Context providers should be placed in `src/providers/*` using kebab-case filenames.
⏰ 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: Setup
- GitHub Check: Setup
- GitHub Check: Build & Push
🔇 Additional comments (6)
apps/stats/src/views/Stats/components/stats-filter.tsx (6)
93-95: LGTM! Consistent badge styling across option types.The compact monospace styling for visit counts provides better visual hierarchy and consistency across UTM and source options.
Also applies to: 159-161
387-387: Verify that hiding selected options doesn't hurt discoverability.Adding
selectedOptionsClassName: 'hidden'across UTM and source fields will hide selected options in the dropdown. Ensure users can still clearly identify what's currently selected, particularly when reopening the filter to modify selections. The selected values should be visible in the filter chips/tags.Also applies to: 400-400, 413-413, 426-426, 439-439, 483-483
452-454: LGTM! Appropriate defaults for multiselect audience field.Setting
defaultOperator: 'is any of'and hiding the operator select makes sense for a multiselect field where this is the natural operator.
460-460: LGTM! Better icon choice for posts.
PenLinemore accurately represents writing/content than the genericFileicon.
468-469: LGTM! Wider popover improves readability.Setting both
classNameandpopoverContentClassNametow-80provides more space for post titles, improving readability of the searchable dropdown.
496-496: LGTM! Appropriate UI refinements for filter component.The
FunnelPlusicon, compactsmsize, and layout-adjusted className all contribute to a more polished filter interface.Also applies to: 498-498, 502-502
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/shade/src/components/ui/filters.tsx (1)
944-949: Border styling change looks correct but differs from CVA-managed variants.The non-editable operator display now uses inline styles (
border border-r-[0px]) rather than leveraging the existing CVA variants. This works but creates inconsistency with how other filter segments manage borders.Consider extracting this into a CVA variant for consistency:
- <div className="flex items-center self-stretch border border-r-[0px] px-3 text-sm text-muted-foreground"> + <div className={cn( + "flex items-center self-stretch px-3 text-sm text-muted-foreground", + filterOperatorVariants({variant: context.variant, size: context.size}), + "cursor-default hover:bg-transparent hover:text-muted-foreground" + )}>apps/stats/src/views/Stats/components/stats-filter.tsx (1)
497-499: EmptyaddButtonTextfor both states is redundant.The ternary
filters.length ? '' : ''always evaluates to an empty string. This can be simplified.Apply this diff:
- addButtonText={filters.length ? '' : ''} + addButtonText=''
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/shade/src/components/ui/filters.tsx(9 hunks)apps/stats/src/views/Stats/components/stats-filter.tsx(12 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/shade/src/components/ui/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
Atomic UI components should be placed in
src/components/ui/*and each component must have a corresponding*.stories.tsxfile next to it for Storybook documentation
Files:
apps/shade/src/components/ui/filters.tsx
apps/shade/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
apps/shade/src/components/**/*.{ts,tsx}: UsePascalCasefor component identifiers in filenames while keeping ShadCN-generated files in kebab-case
Always forward and mergeclassNameprop withcn(...)utility function
Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Prefer compound subcomponents (e.g.,Header.Title,Header.Meta,Header.Actions) for multi-region components instead of many props
Use Tailwind CSS scoped via.shadeclass; dark mode uses.dark
Files:
apps/shade/src/components/ui/filters.tsx
apps/shade/src/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
apps/shade/src/**/*.{ts,tsx,js}: UsecamelCasefor function and variable names
Use the@alias for internal imports (e.g.,@/lib/utils)
Files:
apps/shade/src/components/ui/filters.tsx
apps/shade/{src,test}/**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (apps/shade/AGENTS.md)
apps/shade/{src,test}/**/*.{ts,tsx,js}: Runyarn lintafter making changes to fix any ESLint errors and warnings before committing
Follow ESLint andtailwindcss/*plugin rules when writing styles
Files:
apps/shade/src/components/ui/filters.tsx
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use Tailwind CSS scoped via `.shade` class; dark mode uses `.dark`
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use CVA (Class Variance Authority) for component variants when useful instead of heavy prop configuration
Applied to files:
apps/shade/src/components/ui/filters.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/components/**/*.{ts,tsx} : Use Tailwind CSS scoped via `.shade` class; dark mode uses `.dark`
Applied to files:
apps/shade/src/components/ui/filters.tsx
📚 Learning: 2025-11-26T11:05:59.314Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: apps/shade/AGENTS.md:0-0
Timestamp: 2025-11-26T11:05:59.314Z
Learning: Applies to apps/shade/src/index.ts : Place new UI components under `src/components/ui` and export them from `src/index.ts`
Applied to files:
apps/shade/src/components/ui/filters.tsx
📚 Learning: 2025-11-25T14:28:50.351Z
Learnt from: CR
Repo: TryGhost/Ghost PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T14:28:50.351Z
Learning: Applies to ghost/i18n/locales/en/ghost.json : Add UI translations to `ghost/i18n/locales/en/ghost.json` for Admin UI features
Applied to files:
apps/shade/src/components/ui/filters.tsx
🧬 Code graph analysis (1)
apps/shade/src/components/ui/filters.tsx (3)
ghost/core/core/server/services/koenig/node-renderers/header-v1-renderer.js (1)
div(55-55)apps/shade/src/components/ui/popover.tsx (1)
PopoverContent(31-31)apps/shade/src/index.ts (1)
cn(82-82)
⏰ 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). (5)
- GitHub Check: Admin-X Settings tests
- GitHub Check: ActivityPub tests
- GitHub Check: Unit tests (Node 22.18.0)
- GitHub Check: Lint
- GitHub Check: Build & Push
🔇 Additional comments (12)
apps/shade/src/components/ui/filters.tsx (6)
239-239: LGTM!The padding adjustment from
px-1.5topx-2for thesmvariant provides more consistent horizontal spacing.
750-751: LGTM!The new
autoCloseOnSelectfield option is well-documented and provides useful control for auto-closing dropdowns on selection, even for multiselect types.
1049-1052: Delayed search clearing aligns with popover close animation.The 200ms delay before clearing the search input allows the popover close animation to complete before resetting state, preventing a visual flash.
1134-1137: LGTM!The auto-close behavior for inline
SelectOptionsPopovercorrectly checksautoCloseOnSelectbefore invokingonClose, maintaining compatibility with existing multiselect behavior.
2074-2074: Potential confusion betweenclassNameandpopoverContentClassName.The logic
selectedFieldForOptions?.className || popoverContentClassName || 'w-[200px]'prioritizesclassNameoverpopoverContentClassName. However,classNameis typically used for the field's display styling whilepopoverContentClassNameis specifically for the popover. This could cause unexpected width inheritance.Confirm this prioritization is intentional. The summary indicates "use
selectedFieldForOptions?.classNamethenpopoverContentClassName", but mixing general className with popover-specific className may be confusing for consumers.
105-105: Ensure add filter button displays intended text or iconSetting
addFilterandaddFilterTitleto empty strings inDEFAULT_I18N(lines 105, 122) means the "Add Filter" button will render with no visible text unlessaddButtonTextis explicitly provided. The current fallback logic at line 2070 ({addButtonText || mergedI18n.addFilter}) will display an empty string ifaddButtonTextis undefined. The Storybook stories and any consumer implementations that rely on the default will show a button with only an icon and no label, potentially creating confusion about the button's purpose.Verify that all usages either (1) provide explicit
addButtonTextprops, or (2) intended the button to display without text. If text labels are desired, consider restoring meaningful default strings foraddFilterandaddFilterTitle.apps/stats/src/views/Stats/components/stats-filter.tsx (6)
93-96: LGTM!The
order-2class on the count badge spans ensures they appear at the end of the option row, improving visual layout for UTM and source options.Also applies to: 158-162
386-387: LGTM!Adding
selectedOptionsClassName: 'hidden'to UTM and source fields hides selected options from the dropdown display, providing a cleaner UI when filters are applied.Also applies to: 399-400, 412-413, 425-426, 438-439, 483-484
452-455: LGTM!The audience field configuration with
defaultOperator: 'is any of',hideOperatorSelect: true, andautoCloseOnSelect: trueprovides a streamlined multi-select experience that closes immediately after selection.
461-461: LGTM!The
PenLineicon is more semantically appropriate for "Post or page" than the previous icon.
469-470: LGTM!Setting both
className: 'w-80'andpopoverContentClassName: 'w-80'ensures consistent width for the post filter popover, accommodating longer post titles.
503-503: LGTM!Using
size='sm'aligns with the compact filter presentation goal of this PR.
peterzimon
force-pushed
the
analytics-filtering-refinements
branch
from
e3ed0cd to
998a7a8
Compare
1 participant
ref https://linear.app/ghost/issue/NY-768/analytics-filter-ui-refinements