Skip to content

Comments

Re-land #76 safely: resilient /skills count + globalStats fallbacks#375

Merged
steipete merged 3 commits intomainfrom
reland-pr76-safe
Feb 16, 2026
Merged

Re-land #76 safely: resilient /skills count + globalStats fallbacks#375
steipete merged 3 commits intomainfrom
reland-pr76-safe

Conversation

@steipete
Copy link
Collaborator

@steipete steipete commented Feb 16, 2026

Re-lands the #76 skills-count feature that was reverted in #359, with migration-safe behavior.

What changed

  • Re-applies the original /skills total count UI + globalStats table/cron.
  • Re-applies follow-up refactors for public-visibility/count consistency.
  • Adds hard fallbacks so /skills never errors if globalStats table/index/row is missing:
    • skills.countPublicSkills returns precomputed count when available.
    • If global stats storage is unavailable or uninitialized, it falls back to a live public-skill count.
  • Guards globalStats write paths so missing storage does not fail unrelated skill mutations.
  • Adds regression tests for countPublicSkills fallback paths.

Verification

  • bun run lint
  • bun run build
  • bun run test

Fixes #357

Greptile Summary

Re-lands the #76 skills-count feature (reverted in #359) with migration-safe fallbacks. Adds a globalStats table with an hourly cron for full reconciliation, incremental count adjustments on every skill mutation path, and a countPublicSkills query that prefers precomputed counts but falls back to a live scan if the table isn't ready. Centralizes public-visibility checks from toPublicSkill into a reusable isPublicSkillDoc function, removes a redundant client-side default-sort useEffect, and shares Convex React test mocks across the skills test suite.

  • The globalStats error-detection heuristic (isGlobalStatsStorageNotReadyError) is string-based, which is pragmatic for migration safety but may need updating if Convex changes its error message format.
  • All skill mutation sites consistently apply the {...skill, ...patch} pattern to compute before/after visibility deltas — no double-counting was found in the insertVersion flow.
  • The live-count fallback in the countPublicSkills query paginates through all skills, which could be expensive on large datasets, but this only triggers when globalStats hasn't been initialized yet and is quickly resolved by the hourly cron.
  • Test coverage is solid: three fallback scenarios for countPublicSkills and updated rate-limit tests to handle the new globalStats queries.

Confidence Score: 5/5

  • This PR is safe to merge — all mutation paths are properly guarded with fallbacks, and the feature degrades gracefully if the new table isn't deployed yet.
  • Score of 5 reflects: (1) consistent and thorough integration of count adjustments at all skill mutation sites, (2) robust error handling that prevents the new feature from breaking existing functionality even during migration, (3) proper test coverage for all fallback paths, (4) clean code organization with centralized visibility logic, and (5) no logical errors or security issues found after thorough review.
  • No files require special attention.

Last reviewed commit: 89129a5

Context used:

  • Context from dashboard - AGENTS.md (source)

rknoche6 and others added 3 commits February 17, 2026 00:21
Co-authored-by: rknoche <richard.knoche@holidaycheck.com>
Co-authored-by: Peter Steinberger <steipete@gmail.com>
@vercel
Copy link
Contributor

vercel bot commented Feb 16, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
clawhub Ready Ready Preview, Comment Feb 16, 2026 11:24pm

@cursor
Copy link

cursor bot commented Feb 16, 2026

PR Summary

Medium Risk
Touches multiple moderation/publish/delete code paths to maintain an incremental global counter; mistakes could skew counts or add edge-case DB errors, though the code includes explicit fallbacks for missing schema/rows.

Overview
Adds a new globalStats storage layer (schema + hourly cron + maintenance mutation) to keep a precomputed count of publicly visible skills, plus a skills.countPublicSkills query that returns the cached value and falls back to a live count when the table/index/row isn’t available.

Centralizes public-skill visibility rules in convex/lib/globalStats.ts and updates many skill-mutation paths (publish, delete/undelete, moderation/VT actions, ban/unban flows, hard-delete prep) to incrementally adjust the global count when a skill’s public visibility changes, while guarding writes so missing storage doesn’t break unrelated mutations.

Updates the /skills UI to fetch and display the total skill count next to the page title, removes a client-side default-sort redirect in favor of route beforeLoad, and refactors browse tests to share Convex React mocks; adds new unit tests covering the count query’s fallback behavior.

Written by Cursor Bugbot for commit 89129a5. This will update automatically on new commits. Configure here.

@steipete steipete merged commit 8126413 into main Feb 16, 2026
6 checks passed
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

key: v.string(),
activeSkillsCount: v.number(),
updatedAt: v.number(),
}).index('by_key', ['key'])
Copy link

Choose a reason for hiding this comment

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

Global stats key allows duplicate rows

Medium Severity

globalStats uses a plain by_key index, but writes and reads assume a single row via .unique(). Concurrent initialization paths can insert multiple key: 'default' rows, after which .unique() can throw and break countPublicSkills and mutations that call adjustGlobalPublicSkillsCount.

Additional Locations (2)

Fix in Cursor Fix in Web

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 89129a5d97

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +1660 to +1661
const statsCount = await readGlobalPublicSkillsCount(ctx)
if (typeof statsCount === 'number') return statsCount

Choose a reason for hiding this comment

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

P2 Badge Avoid serving stale global count from partially maintained cache

countPublicSkills returns globalStats.activeSkillsCount whenever that row exists, but the new delta-maintenance wiring in this commit only covers mutations in convex/skills.ts. I checked other visibility-changing writers (evictSquatterSkillForRestoreInternal in convex/githubRestoreMutations.ts:34-40 and applyEmptySkillCleanupInternal in convex/maintenance.ts:1012-1027), and they still soft-delete/hide skills without updating globalStats, so /skills can report an inflated count until the hourly cron corrects it. Please update those paths (or force a recount here when cache freshness is uncertain) so this query stays consistent.

Useful? React with 👍 / 👎.

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.

Last pr (mine) broke the skills list

2 participants