Skip to content

[CORRUPTED] Synthetic Benchmark PR #5890 - Make various UI improvements #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
tomerqodo wants to merge 2 commits into
base: base_pr_5890_20251204_9037
Choose a base branch
from
Open

[CORRUPTED] Synthetic Benchmark PR #5890 - Make various UI improvements #90

tomerqodo wants to merge 2 commits into from

Conversation

@tomerqodo
Copy link

@tomerqodo tomerqodo commented Dec 4, 2025 •
edited by qodo-code-review bot
Loading

User description

Benchmark PR plausible#5890

Type: Corrupted (contains bugs)

Original PR Title: Make various UI improvements
Original PR Description: ### Changes

  • Fix invite modal z-index issues (search, button and avatar on top of overlay)
  • Improve invite modal design
Before After
CleanShot 2025-11-17 at 09 54 27@2x Localhost · 2 38pm · 11-17
  • Add hover state to stats bars on dashboard
CleanShot.2025-11-15.at.10.55.13.mp4
  • Improve feature gate design
Before After
CleanShot 2025-11-17 at 09 38 34@2x CleanShot 2025-11-17 at 09 38 15@2x
  • Improve trial upgrade CTA design
Before After
CleanShot 2025-11-15 at 10 27 40@2x CleanShot 2025-11-15 at 10 27 01@2x

Tests

  • This PR does not require tests

Dark mode

PR Type

Enhancement

Description

  • Improve feature gate overlay design with better visual hierarchy

  • Add hover state to dashboard stats bars with smooth transitions

  • Redesign invitation modal with cleaner layout and improved typography

  • Enhance trial upgrade notification in header with button component

  • Update upgrade call-to-action messaging for clarity and consistency

Diagram Walkthrough

flowchart LR
  A["Feature Gate<br/>Overlay"] -->|"Enhanced styling<br/>& typography"| B["Better Visual<br/>Hierarchy"]
  C["Stats Bars"] -->|"Add hover state<br/>& transitions"| D["Interactive<br/>Feedback"]
  E["Invitation<br/>Modal"] -->|"Redesign layout<br/>& spacing"| F["Improved<br/>UX"]
  G["Trial Notification"] -->|"Add button<br/>component"| H["Better<br/>CTA"]
  I["Upgrade Messages"] -->|"Clearer<br/>wording"| J["Consistent<br/>Copy"]
Loading

File Walkthrough
Relevant files
Enhancement
11 files
billing.ex
Redesign feature gate overlay and upgrade messaging           
+27/-16 
generic.ex
Add yellow button theme and improve disabled state             
+3/-1     
prima_modal.ex
Adjust modal positioning and add title styling                     
+3/-3     
sites.ex
Redesign invitation modal layout and messaging                     
+21/-28 
_header.html.heex
Improve trial notification design with button component   
+13/-6   
bar.js
Add rounded corners and transition effects to bars             
+1/-1     
conversions.js
Add hover state color to conversion stats bars                     
+1/-1     
props.js
Add hover state color to properties stats bars                     
+1/-1     
index.js
Add hover state colors to page stats bars                               
+3/-3     
source-list.js
Add hover state colors to source stats bars                           
+3/-3     
list.tsx
Add row hover effects and improve bar styling                       
+3/-3     
Tests
5 files
billing_test.exs
Update tests for feature gate overlay changes                       
+8/-12   
notice_test.exs
Update upgrade message text in tests                                         
+1/-1     
funnel_settings_test.exs
Update upgrade message assertions in tests                             
+2/-2     
props_settings_test.exs
Update upgrade message assertions in tests                             
+4/-4     
team_setup_test.exs
Update feature gate overlay test assertions                           
+3/-4     

sanne-san and others added 2 commits November 17, 2025 15:19
@sanne-san
Make various UI improvements
5e42cd7 
- Fix invite modal z-index issues
- Improve invite modal design
- Add hover state to stats bars on dashboard
- Improve feature gate design
- Improve trial upgrade CTA design
@tomerqodo
Apply changes for benchmark PR
5bec9e5
@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing Audit Logs: New UI flows that change upgrade prompts and CTA links do not include any logging of
critical user actions (e.g., clicking upgrade, accepting invitations), and it is unclear
from the diff whether such actions are audited elsewhere.

Referred Code 
  case Plans.get_subscription_plan(team && team.subscription) do
    %Plan{kind: :business} -> true
    %EnterprisePlan{} -> true
    _ -> false
  end

cond do
  not is_nil(assigns.current_role) and assigns.current_role not in [:owner, :billing] ->
    ~H"ask your team owner to upgrade their subscription."

  upgrade_assistance_required? ->
    ~H"""
    contact
    <.styled_link href="mailto:[email protected]" class="font-medium">
      [email protected]
    </.styled_link>
    to upgrade your subscription.
    """

  true ->
    ~H"""


 ... (clipped 8 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Missing Null Checks: UI changes rely on props like color and metrics without added guards or fallbacks in new
code paths (e.g., hover color usage), which may cause silent UI failures if undefined,
though handling may exist elsewhere.

Referred Code 
    </div>
  )
}

function renderRow(listItem: TListItem) {
  return (
    <div key={listItem.name} style={{ minHeight: ROW_HEIGHT }}>
      <div
        className="group flex w-full items-center hover:bg-gray-100/60 dark:hover:bg-gray-850 rounded-sm transition-colors duration-150"
        style={{ marginTop: ROW_GAP_HEIGHT }}
      >
        {renderBarFor(listItem)}
        {renderMetricValuesFor(listItem)}
      </div>
    </div>
  )
}

function renderBarFor(listItem: TListItem) {
  const lightBackground = color || 'bg-green-50 group-hover:bg-green-100'
  const metricToPlot = metrics.find((metric) => metric.meta.plot)?.key


 ... (clipped 9 lines)

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

qodo-code-review bot commented Dec 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Inconsistent hover effects on dashboard

The dashboard's new hover effects are inconsistent. The bar chart's color
changes on hover, but other row elements like text do not, creating a disjointed
experience that should be made more cohesive.

Examples:

assets/js/dashboard/stats/reports/list.tsx [250-271] 
          className="group flex w-full items-center hover:bg-gray-100/60 dark:hover:bg-gray-850 rounded-sm transition-colors duration-150"
          style={{ marginTop: ROW_GAP_HEIGHT }}
        >
          {renderBarFor(listItem)}
          {renderMetricValuesFor(listItem)}
        </div>
      </div>
    )
  }


 ... (clipped 12 lines)
assets/js/dashboard/stats/pages/index.js [56] 
      color="bg-orange-50 group-hover:bg-orange-100"

Solution Walkthrough:

Before:

// assets/js/dashboard/stats/reports/list.tsx

function ListReport({ color, ... }) {
  // ...
  function renderRow(listItem) {
    return (
      // The row has a hover effect
      <div className="group ... hover:bg-gray-100/60 dark:hover:bg-gray-850 ...">
        {renderBarFor(listItem)}
        {/* Other elements like metrics */}
      </div>
    )
  }

  function renderBarFor(listItem) {
    // The bar color also changes on hover, but dark mode bg is removed
    const lightBackground = color || 'bg-green-50 group-hover:bg-green-100';
    return <Bar bg={lightBackground} ... />;
  }
}

After:

// assets/js/dashboard/stats/reports/list.tsx

function ListReport({ color, ... }) {
  // ...
  function renderRow(listItem) {
    return (
      // The row is a group, but hover effect is removed to avoid conflict
      <div className="group ...">
        {renderBarFor(listItem)}
        {/* Other elements like metrics */}
      </div>
    )
  }

  function renderBarFor(listItem) {
    // The bar color changes on hover, and dark mode is handled correctly.
    // Text color could also be changed with `group-hover:text-...`
    const background = color || 'bg-green-50';
    const hoverBackground = 'group-hover:bg-green-100';
    const darkBackground = 'dark:bg-gray-500/15';
    return <Bar bg={`${background} ${hoverBackground} ${darkBackground}`} ... />;
  }
}
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a UX inconsistency in the new hover effects and a related bug where the bar's dark mode background is removed, making it a significant quality improvement.

Medium
Possible issue
Restore dark mode background class

Restore the dark mode background class for the Bar component to fix its
appearance in dark mode.

assets/js/dashboard/stats/reports/list.tsx [266-272]

 const lightBackground = color || 'bg-green-50 group-hover:bg-green-100'
 const metricToPlot = metrics.find((metric) => metric.meta.plot)?.key
 
 return (
   <div className="grow w-full overflow-hidden">
     <Bar
       maxWidthDeduction={undefined}
       count={listItem[metricToPlot]}
       all={state.list}
-      bg={lightBackground}
+      bg={`${lightBackground} dark:bg-gray-500/15`}
       plot={metricToPlot}
     >

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that removing the hardcoded dark mode class from the bg prop will likely cause a visual regression in dark mode, as no alternative dark mode styling is provided.

Low
Prevent page jump on button click

Replace the "Reject" <.button_link> with a <.button> component to prevent the page from jumping to the
top on click.

lib/plausible_web/live/sites.ex [747-756]

 <div class="flex flex-col sm:flex-row-reverse gap-3 p-5 sm:p-6">
    <.button
      :if={!(Map.get(@invitation, :exceeded_limits) || Map.get(@invitation, :no_plan))}
      mt?={false}
      class="w-full sm:w-auto sm:text-sm"
      data-method="post"
      data-csrf={Plug.CSRFProtection.get_csrf_token()}
      data-to={"/sites/invitations/#{@invitation.invitation.invitation_id}/accept"}
      data-autofocus
    >
      Accept and continue
    </.button>
    <.button_link
      :if={Map.get(@invitation, :exceeded_limits) || Map.get(@invitation, :no_plan)}
      mt?={false}
      href={Routes.billing_path(PlausibleWeb.Endpoint, :choose_plan)}
      class="w-full sm:w-auto sm:text-sm"
      data-autofocus
    >
      Upgrade
    </.button_link>
-   <.button_link
+   <.button
      mt?={false}
      class="w-full sm:w-auto sm:text-sm"
-     href="#"
      theme="secondary"
      data-method="post"
      data-csrf={Plug.CSRFProtection.get_csrf_token()}
      data-to={"/sites/invitations/#{@invitation.invitation.invitation_id}/reject"}
    >
      Reject
-   </.button_link>
+   </.button>
  </div>

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that using <.button_link> with href="#" for a POST action can cause an unwanted page jump and proposes a more semantically correct <.button> component, improving user experience.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tomerqodo @sanne-san