Skip to content

refactor: utilizes anonymous gh access and then fallback to our token#2599

Merged
stalniy merged 1 commit intomainfrom
refactor/templates4
Jan 29, 2026
Merged

refactor: utilizes anonymous gh access and then fallback to our token#2599
stalniy merged 1 commit intomainfrom
refactor/templates4

Conversation

@stalniy
Copy link
Contributor

@stalniy stalniy commented Jan 29, 2026

Why

To fight against gh rate limiting

Summary by CodeRabbit

  • New Features

    • Added anonymous fetch fallback and a credentials fallback for GitHub template fetching.
  • Improvements

    • More detailed GitHub API error messages.
    • Centralized rate-limit tracking for API calls.
  • Tests

    • Updated/removed unit tests related to GitHub request tracking.

✏️ Tip: You can customize this high-level summary in your review settings.

@stalniy stalniy requested a review from a team as a code owner January 29, 2026 11:34
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 29, 2026

📝 Walkthrough

Walkthrough

Refactors GitHub credential handling: workflow now prefers secrets.TEMPLATES_GITHUB_PAT with a fallback; TemplateFetcherService now accepts an Octokit factory and creates both authenticated and anonymous Octokit instances, trying anonymous fetches first and falling back to authenticated calls, with rate-limit tracking moved to a fetch wrapper.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
.github/workflows/reusable-build-image.yml
GITHUB_PAT assignment changed to use secrets.TEMPLATES_GITHUB_PAT or fallback to secrets.GITHUB_TOKEN.
Template fetcher implementation
apps/api/src/template/services/template-fetcher/template-fetcher.service.ts
Constructor now accepts a createOctokit factory and required githubPAT option; initializes authenticated and anonymous Octokit instances; fetch wrapper centralizes rate-limit tracking; added anonymous-first fetch + authenticated fallback helpers; improved error typing; removed direct response-based rate-limit updates.
Template gallery integration
apps/api/src/template/services/template-gallery/template-gallery.service.ts
Now passes an Octokit factory (getOctokit) and githubPAT into TemplateFetcherService; getOctokit accepts optional fetch and defers PAT handling to caller.
Tests
apps/api/src/template/services/template-fetcher/template-fetcher.service.spec.ts
Updated to construct service with an Octokit provider function and options object containing githubPAT; removed test asserting githubRequestsRemaining header updates.

Sequence Diagram(s)

sequenceDiagram
    participant Service as TemplateFetcherService
    participant Anonymous as Anonymous Octokit
    participant Primary as Authenticated Octokit
    participant GH as GitHub API

    Service->>Service: fetchFileContent(path, ref)
    Service->>Anonymous: getContent(path, ref) (no auth)
    Anonymous->>GH: HTTP request (unauthenticated)
    alt Anonymous returns content
        GH-->>Anonymous: 200 + content
        Anonymous-->>Service: content (return)
    else Anonymous fails (404/403/etc.)
        GH-->>Anonymous: error
        Anonymous-->>Service: error
        Service->>Primary: getContent(path, ref) (with PAT)
        Primary->>GH: HTTP request (authenticated)
        GH-->>Primary: 200 + content / error
        Primary-->>Service: content or error
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • baktun14

Poem

🐇 I nibble at tokens, hop from auth to none,
I try anon first, then PAT if need be run,
Fetches kept nimble, rate limits softly traced,
Templates retrieved, no longer misplaced,
Hooray for resilient hopping — code done! 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: refactoring to use anonymous GitHub access with fallback to token for rate-limit mitigation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

ygrishajev
ygrishajev previously approved these changes Jan 29, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@apps/api/src/template/services/template-fetcher/template-fetcher.service.ts`:
- Around line 91-107: The early-return condition in private method
`#fetchFileContentAnonymously` is inverted: change the guard from "if
(!this.#isAnonymousOctokitFailed) return;" to "if
(this.#isAnonymousOctokitFailed) return;" so anonymous fetching is attempted
unless it previously failed; keep using
this.#fetchFileContentUsing(this.#anonymousOctokit, params) and preserve the
catch logic that sets this.#isAnonymousOctokitFailed = true and logs via
this.#logger.warn with the same event, owner, repo, path, ref, and error fields.
🧹 Nitpick comments (2)
apps/api/src/template/services/template-fetcher/template-fetcher.service.ts (1)

160-164: Avoid using any return type.

The return type Promise<any> violates coding guidelines. Based on usage, this should return Promise<Template | null>.

♻️ Proposed fix
  private async processTemplateSource(
    templateSource: TemplateSource,
    directoryItems: GithubDirectoryItem[],
    options: { includeConfigJson?: boolean }
-  ): Promise<any> {
+  ): Promise<Template | null> {

As per coding guidelines: "Never use type any or cast to type any. Always define the proper TypeScript types."

apps/api/src/template/services/template-fetcher/template-fetcher.service.spec.ts (1)

13-82: Consider adding tests for anonymous fallback behavior.

The new anonymous-first fetching strategy with fallback to authenticated requests is a key part of this PR. Consider adding tests that verify:

  1. Anonymous fetch is attempted first
  2. Fallback to authenticated occurs when anonymous fails
  3. Once anonymous fails, subsequent calls skip anonymous and go directly to authenticated

This would help catch issues like the inverted condition bug.

@codecov
Copy link

codecov bot commented Jan 29, 2026

Codecov Report

❌ Patch coverage is 79.16667% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 49.69%. Comparing base (ab74ab0) to head (4faa647).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...vices/template-fetcher/template-fetcher.service.ts 77.27% 4 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (79.16%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2599      +/-   ##
==========================================
- Coverage   50.01%   49.69%   -0.32%     
==========================================
  Files        1024     1014      -10     
  Lines       28976    28636     -340     
  Branches     6620     6562      -58     
==========================================
- Hits        14492    14231     -261     
+ Misses      14205    14025     -180     
- Partials      279      380     +101     
Flag Coverage Δ *Carryforward flag
api 78.78% <79.16%> (-0.07%) ⬇️
deploy-web 31.84% <ø> (ø) Carriedforward from ab74ab0
log-collector ?
notifications 87.94% <ø> (ø) Carriedforward from ab74ab0
provider-console 81.48% <ø> (ø) Carriedforward from ab74ab0
provider-proxy 84.35% <ø> (ø) Carriedforward from ab74ab0

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
...vices/template-gallery/template-gallery.service.ts 99.11% <100.00%> (+0.85%) ⬆️
...vices/template-fetcher/template-fetcher.service.ts 91.11% <77.27%> (-4.06%) ⬇️

... and 45 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stalniy stalniy merged commit 9e642b3 into main Jan 29, 2026
149 of 154 checks passed
@stalniy stalniy deleted the refactor/templates4 branch January 29, 2026 13:13
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.

2 participants

Comments