Skip to content
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.

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

feat: add total counts to start page #215

Merged
merged 1 commit into from
Jan 19, 2025
Merged

Conversation

flaming-codes
Copy link
Owner

@flaming-codes flaming-codes commented Jan 19, 2025

Important

Add total counts of authors and packages to the start page with caching in author.service.ts and package.service.ts.

  • Behavior:
    • Adds getTotalAuthorsCount() in AuthorService and getTotalPackagesCount() in PackageService to fetch and cache total counts.
    • Updates loader in _index.tsx to fetch total counts of authors and packages and format them for display.
  • Caching:
    • Extends CacheKey and CacheValue types in author.service.ts and package.service.ts to include counts.
    • Caches total counts in TTLCache in both services.
  • UI:
    • Displays total counts of authors and packages in the start page description in _index.tsx.

This description was created by Ellipsis for c57c87f. It will automatically update as commits are pushed.

@flaming-codes flaming-codes merged commit 7e516bc into main Jan 19, 2025
@flaming-codes flaming-codes deleted the feat-total-counts branch January 19, 2025 09:11
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to c57c87f in 1 minute and 52 seconds

More details
  • Looked at 161 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. web/app/data/author.service.ts:12
  • Draft comment:
    Cache keys should be unique across different services to avoid potential collisions. Consider prefixing the keys with the service name, e.g., "author-count-authors".
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The cache is a private static field within AuthorService. It's not accessible from other services. The cache keys are already unique within this service. The suggestion seems to be solving a non-existent problem since there's no possibility of key collisions across services.
    Maybe there could be inheritance or composition patterns I'm not seeing where the cache could be shared? Maybe there are plans to refactor this into a shared cache in the future?
    Even if the cache were to be shared in the future, that would be a significant architectural change requiring many other changes. We should not complicate the current code for hypothetical future changes.
    Delete this comment. The suggestion is solving a non-existent problem since the cache is private to this service and cannot have collisions with other services.
2. web/app/data/package.service.ts:18
  • Draft comment:
    Cache keys should be unique across different services to avoid potential collisions. Consider prefixing the keys with the service name, e.g., "package-count-packages".
  • Reason this comment was not posted:
    Marked as duplicate.
3. web/app/data/package.service.ts:83
  • Draft comment:
    This is duplicate counting logic. Consider creating a shared utility function that takes table name and cache key as parameters.

  • function getTotalAuthorsCount (author.service.ts)

  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_EOwW2ZorZ6Rvo2pZ


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@@ -196,6 +196,25 @@ export class AuthorService {
return sitemapItems;
}

static async getTotalAuthorsCount(): Promise<number> {
Copy link

Choose a reason for hiding this comment

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

Consider consolidating this with the existing count utility in PackageService by creating a shared function that takes the table name as a parameter.

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.

1 participant