Refactor repository select to async Select2 #9
Conversation
📝 WalkthroughWalkthroughThis pull request refactors GitHub repository selection and loading with a comprehensive caching, throttling, and search system. Changes span backend API endpoints, console commands, frontend state management, cache persistence, and modal UX enhancements to address repository dropdown functionality issues in modals. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Modal as Modal UI
participant Frontend as Frontend JS
participant LocalStorage as Local Storage
participant Backend as Backend API
participant Cache as Laravel Cache
participant GitHub as GitHub API
User->>Modal: Open modal with repo select
Modal->>Frontend: githubEnsureRepositoryCache()
alt Cache exists & token matches
Frontend->>LocalStorage: Retrieve cached repos
LocalStorage-->>Frontend: Return cached data
else Cache miss or token changed
Frontend->>Backend: GET /github/repositories<br/>(skipCache)
alt Throttled response
Backend->>Cache: Check throttle window
Cache-->>Backend: Return retry_after
Backend-->>Frontend: 429 + retry_after
Frontend->>User: Show throttle warning
else Cache hit from backend
Backend->>Cache: Retrieve repositories
Cache-->>Backend: Return repos + metadata
else Cache miss, fetch from GitHub
Backend->>GitHub: Fetch repositories
GitHub-->>Backend: Repository list
Backend->>Cache: Store with timestamp
Cache-->>Backend: Confirm
end
Backend-->>Frontend: Repositories + source + fetched_at
Frontend->>LocalStorage: Persist with token_hash
end
Frontend->>Modal: Populate repository select
Modal->>User: Display selectable repositories
User->>Frontend: Type in search box
Frontend->>Frontend: Debounce search (custom transport)
Frontend->>Backend: POST /github/repositories/search<br/>(query)
Backend->>Cache: Search in cached repos
alt Throttled
Backend-->>Frontend: Throttle notice
Frontend->>User: Show retry_after
else Results available
Backend-->>Frontend: Filtered results + metadata
Frontend->>Modal: Update dropdown options
end
sequenceDiagram
participant User
participant UI as UI Button
participant Frontend as Frontend JS
participant Backend as Backend API
participant LocalStorage as Local Storage
participant Cache as Laravel Cache
User->>UI: Click "Refresh Repositories"
UI->>Frontend: githubRefreshRepositoryCache()
Frontend->>LocalStorage: Clear cached repos
Frontend->>Frontend: Reset in-memory cache flags
Frontend->>Backend: POST /github/repositories/refresh
Backend->>Cache: Clear repository cache
Cache-->>Backend: Confirmed
Backend-->>Frontend: Success response
Frontend->>Backend: GET /github/repositories<br/>(skipCache=true)
Backend->>GitHub: Fetch fresh repos
GitHub-->>Backend: Latest data
Backend->>Cache: Store new data
Cache-->>Backend: Confirmed
Backend-->>Frontend: Fresh repos + metadata
Frontend->>LocalStorage: Persist refreshed data
Frontend->>UI: Show success notification
UI->>User: Cache refreshed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
Http/Controllers/GithubController.php (1)
150-209: Repository search endpoint fits Select2 and cache model; consider light validationThe
searchRepositories()endpoint:
- Accepts
q/termplus an optionallimit, delegating search toRepositoryCache::search.- Formats results as
{ id, text, name, private, has_issues }, which matches typical Select2 expectations.- Returns useful
meta(count, source, fetched_at, throttled, retry_after) and handles throttling with HTTP 429 and clear errors otherwise.This is well-structured and matches the frontend async-search needs. As a minor hardening improvement, you might add basic validation on
limit(e.g., integer with sensible bounds) and perhaps a max length on the query to guard against pathological inputs, though the current casting and internal clamping already mitigate most risks.Support/RepositoryCache.php (1)
141-164: Unused$tokenparameter infetchAndCache()
fetchAndCache()accepts$tokenbut never uses it;GithubApiClient::getRepositories()resolves the token internally, so this parameter only adds noise and triggers static analysis warnings.You can simplify the API and silence the warning by dropping the argument:
- /** - * Fetch repositories from GitHub API and cache the results. - * - * @param string $token - * @param string $hash - * @return array - */ - private static function fetchAndCache(string $token, string $hash): array + /** + * Fetch repositories from GitHub API and cache the results. + * + * @param string $hash + * @return array + */ + private static function fetchAndCache(string $hash): array { $throttle = self::checkThrottle($hash); if ($throttle['status'] === 'throttled') { return $throttle; } self::markApiCall($hash); $result = GithubApiClient::getRepositories(); @@ Cache::forever(self::cacheKey($hash), $payload); return [ 'status' => 'success', 'data' => $repositories, 'fetched_at' => $payload['fetched_at'], ]; }And update call sites accordingly:
- [$token, $hash] = $tokenData['data']; - $fetchResult = self::fetchAndCache($token, $hash); + [$token, $hash] = $tokenData['data']; + $fetchResult = self::fetchAndCache($hash);- [$token, $hash] = $tokenData['data']; - $refreshResult = self::fetchAndCache($token, $hash); + [$token, $hash] = $tokenData['data']; + $refreshResult = self::fetchAndCache($hash);This preserves behavior while matching the actual dependencies of
fetchAndCache().Also applies to: 173-204
Public/js/module.js (3)
12-19: Frontend repository caching & token-hash gating look goodThe in-memory + localStorage caching (with a short token hash) and
githubEnsureRepositoryCache()coordination are well-structured and should prevent duplicate loads while supporting multiple callers.githubGetCachedRepositories()does good validation and invalidates stale/structurally-bad entries.One behavioral note: caches with an empty
repositoriesarray are treated as “no cache”, so environments with legitimately zero repos will keep hitting the backend (and rely on backend throttling). If that’s undesirable you could drop thelength === 0check and treat “zero repos” as a valid cached state, but the current behavior is acceptable if you want to re-check periodically.Also applies to: 23-65, 67-130, 714-737
299-315: Modal repo select + default handling with async Select2Deferring repository select setup until
githubEnsureRepositoryCache()completes, and then usinggithubSetupRepositorySelect()plusgithubSetDefaultRepository()to inject/select the default repo, is a good fix for the modal dropdown issues. TheusesAjaxbranch ingithubPopulateRepositories()correctly avoids bulk option rendering and only ensures the current/default option exists.One assumption to keep in mind:
githubSetDefaultRepository()expects the Select2 value (id) to be the repository full name (e.g.owner/repo), since that’s what downstream calls likegithubLoadRepositoryLabels()and issue creation use. Please make sure the backend search endpoint returns each repo withidequal to the repo full name to keep everything consistent.Also applies to: 327-337, 750-755, 784-817
452-576: githubLoadRepositories/githubRefreshRepositoryCache flow is consistent with backend cache
githubLoadRepositories()correctly prefers localStorage, falls back to the API, keeps the settings dropdowns in sync viagithubPopulateRepositories(), and exposes optionalonSuccess/onCompletecallbacks for reuse (e.g., fromgithubEnsureRepositoryCache).githubRefreshRepositoryCache()clearing both localStorage and in-memory cache, then forcing a fresh load, matches the semantics of the backendRepositoryCache::clear().If you ever need richer refresh behavior, you could extend
githubRefreshRepositoryCache()to accept callbacks and forward them into the finalgithubLoadRepositories({ skipCache: true })call, but for the current settings-only usage this is not strictly necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.gitignore(1 hunks)Console/InstallCommand.php(1 hunks)Http/Controllers/GithubController.php(3 hunks)Http/routes.php(1 hunks)Providers/GithubServiceProvider.php(4 hunks)Public/js/laroute.js(1 hunks)Public/js/module.js(10 hunks)README.md(1 hunks)Resources/views/partials/sidebar.blade.php(2 hunks)Support/RepositoryCache.php(1 hunks)start.php(0 hunks)
💤 Files with no reviewable changes (1)
- start.php
🧰 Additional context used
🧬 Code graph analysis (3)
Http/Controllers/GithubController.php (2)
Support/RepositoryCache.php (4)
RepositoryCache(9-320)clear(124-133)getRepositories(21-59)search(68-117)Services/GithubApiClient.php (1)
getRepositories(160-204)
Support/RepositoryCache.php (2)
Services/GithubApiClient.php (1)
GithubApiClient(9-601)Http/Controllers/GithubController.php (1)
getRepositories(89-126)
Providers/GithubServiceProvider.php (1)
Console/InstallCommand.php (1)
InstallCommand(9-69)
🪛 LanguageTool
README.md
[uncategorized] ~58-~58: The official name of this software platform is spelled with a capital “H”.
Context: ...cation cache** so Laravel registers the Github commands ```bash php artisan cach...
(GITHUB)
🪛 PHPMD (2.15.0)
Support/RepositoryCache.php
173-173: Avoid unused parameters such as '$token'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (15)
.gitignore (1)
3-3: Ignorevendordirectory – LGTMIgnoring
vendorin Git is appropriate for a Composer-based module and avoids committing dependencies.Resources/views/partials/sidebar.blade.php (2)
120-129: Create‑issue repository select is ready for async Select2Adding
data-select2-search="true"anddata-placeholder="{{ __('Select Repository') }}"while keeping the default repository option preserves existing behavior and cleanly enables the async Select2 integration. Markup looks correct and is backward compatible.
196-207: Link‑issue repository select aligned with async Select2 usageThe same Select2-friendly attributes and localized placeholder on the “Link Issue” repository select keep the UX consistent across modals and support the async search flow. No issues spotted.
Public/js/laroute.js (1)
11-18: Client-side routes for repository search/refresh look correctThe added route definitions for
github/repositories/searchandgithub/repositories/refreshmatch the server-side URIs and route names, enabling JS to resolve these endpoints cleanly.Http/routes.php (1)
24-27: New repository search and refresh routes are well-scopedThe
github.repositories.search(GET) andgithub.repositories.refresh(POST) routes are properly added under the existing web/auth/roles group and mapped to the intended controller methods. This keeps repository search/refresh behind authentication and consistent with the rest of the module.Console/InstallCommand.php (1)
16-68: Install command implementation is solidThe
freescout-github:installcommand correctly:
- Verifies the
Githubmodule exists before proceeding.- Wraps
module:migratewith an optional--forceflag.- Logs and reports failures, returning a non‑zero exit code on error.
This is safe to run repeatedly and aligns with the README instructions.
Providers/GithubServiceProvider.php (1)
6-6: Command registration and asset loading safeguards look good
- Registering
InstallCommandonly whenrunningInConsole()avoids unnecessary wiring in HTTP requests.- Styles/JS filters now verify asset existence with
file_existsand log meaningful warnings instead of blindly adding paths, which should reduce 404s and ease debugging.Overall, these changes are a nice robustness upgrade with no obvious regressions.
Also applies to: 27-29, 95-100, 105-118, 211-221
Http/Controllers/GithubController.php (3)
13-47: toBoolean helper centralizes tolerant boolean parsingThe new
toBooleanhelper cleanly normalizes mixed request values (null, bool, numeric, and common string forms) with a configurable default, which is ideal for handlingrefreshflags from various clients/older Laravel behavior. Logic looks correct and side-effect free.
92-119: Repository loading now properly respects cache and throttling
getRepositories()now:
- Optionally clears the cache when
refreshevaluates to true viatoBoolean.- Delegates to
RepositoryCache::getRepositories(true)and returns repositories along withsourceandfetched_atmetadata.- Handles throttled responses explicitly with HTTP 429 and a
retry_afterhint, and falls back to a clear error payload otherwise.This is a solid improvement over direct API calls and should help with both performance and UX.
128-148: Explicit cache-clear endpoint is straightforward
refreshRepositories()simply clears the repository cache and returns a status JSON, with errors logged and surfaced as HTTP 500. Given the routes are already behind auth middleware, this is a safe and useful maintenance endpoint.Support/RepositoryCache.php (3)
21-59: Cache-first getRepositories() flow looks correctToken resolution, cache-first lookup, and optional hydration are wired cleanly, and throttling is delegated to
fetchAndCache(). No functional issues spotted here.
68-117: Search behavior is well-structured and throttle-awareThe
search()method correctly reusesgetRepositories(), falls back to an API refresh only when needed (non-empty query, empty matches, and non-API source), and exposesthrottled/retry_aftermetadata for callers. This is a solid design for the async Select2 use case.
212-232: Repository sanitization, filtering, and throttling are solidThe sanitization step enforces the expected shape and excludes repos without
name/full_nameor withhas_issues === false, which aligns with issue-centric UX. Query filtering is straightforward and case-insensitive, and the throttle helper cleanly encapsulates the 30s window with a clearretry_aftervalue. No changes needed here.Also applies to: 242-260, 268-297
Public/js/module.js (2)
183-199: Settings integration with cache persistence/refresh is appropriatePersisting repositories after a successful “Test connection” and wiring the “Refresh repositories” button through
githubRefreshRepositoryCache()ties the settings UI into the new caching model cleanly. Just ensure thegithub.test_connectionendpoint continues to return a top-levelresponse.repositoriesarray so these hooks stay effective.
578-712: Async Select2 repository search with debounce and throttlingThe
githubSetupRepositorySelect()configuration looks solid: it scopes the dropdown to the modal, debounces requests per-select viarepoSearchTimers, cancels in-flight XHRs throughactiveRepoRequests, and surfaces backend throttling via user-friendly alerts. Clearing local cache and forcing a backgroundgithubLoadRepositories({ skipCache: true })whenmeta.source === 'api'is a nice touch to keep settings in sync with fresh server data.No functional issues stand out here; this should address the original “repository dropdown in modal” problem effectively.
|
This is ready for review @jack-arturo |
fixes #5
fixes #6
This PR updates the repository select element from the modal to a Select2 async select and adds better repository cache management.