Skip to content

New: insights screen - #431

Open
ramiy wants to merge 9 commits into
core-betafrom
feat/insights/core
Open

New: insights screen#431
ramiy wants to merge 9 commits into
core-betafrom
feat/insights/core

Conversation

@ramiy

@ramiy ramiy commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@code-snippets-bot

Copy link
Copy Markdown
Contributor
  • Medium — src/php/Admin/Menus/Insights/Insights_Summary.php:35: Reading the table directly bypasses the existing snippet model path and its code_snippets/get_snippets filter, so extensions that supply or filter snippets will show incorrect Insights totals. Aggregate the returned models from get_snippets() instead.

  • Medium — tests/e2e/code-snippets-insights.spec.ts:89: This fixture creates an unattached condition, but Insights_Summary counts a condition as active only when an active snippet references it, so the chart contains 40%, not 60%, and the required Playwright run will fail. Link the condition fixture to an active snippet or assert 40%.

  • Low — src/php/REST_API/Snippets/Preferences_REST_Controller.php:47: The new code_snippets_insights_chart_views option is persisted but never removed by complete uninstall, leaving plugin data behind on every affected site. Delete it in Uninstaller::uninstall_current_site() and cover that cleanup path.

@sheabunge sheabunge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I want to hold off merging this until 3.10 is out of the beta branches, but I reckon this should be good for 4.0 if you're keen to get it out ASAP.

Comment thread src/js/components/InsightsMenu/InsightsCharts.tsx Outdated
Comment thread src/js/components/InsightsMenu/InsightsCharts.tsx Outdated
Comment thread src/js/components/InsightsMenu/InsightsCharts.tsx Outdated
Comment thread src/js/components/InsightsMenu/InsightsCharts.tsx Outdated
Comment thread src/js/components/InsightsMenu/InsightsDashboard.tsx Outdated
Comment thread src/js/hooks/useInsightsChartViews.ts Outdated
const [chartViews, setChartViews] = useState<InsightsChartViews>(
() => window.CODE_SNIPPETS?.insightsChartViews ?? DEFAULT_INSIGHTS_CHART_VIEWS
)
const chartViewsRef = useRef(chartViews)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use useState instead of useRef for persisting data.

Comment thread src/js/hooks/useInsightsChartViews.ts Outdated
Comment thread src/php/Admin/Menus/Insights/Insights_Menu_Assets.php Outdated
* @return array<string, string>
*/
private function get_location_labels(): array {
return [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth just returning the label and offloading translation to the front-end, where we are already including these strings. Otherwise, updating or amending this list now has two places where it needs to be changed.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants