Implement GitHub API caching with TTL#50
Implement GitHub API caching with TTL#50kunal-bunkar wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
WalkthroughImplements a client-side Org Explorer: adds Tailwind and router deps, a stateful React UI for org search/refresh, a GitHub REST client with rate-limit guarding, and an IndexedDB+localStorage TTL cache layer for organization data. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant App as App (React)
participant Cache as Cache (IndexedDB + localStorage)
participant GH as GitHub API
User->>App: submit org login
App->>App: setLoading(true)
App->>Cache: getOrganization(login, forceRefresh=false)
alt cache valid
Cache->>Cache: check TTL (localStorage)
Cache->>Cache: read record (IndexedDB)
Cache-->>App: {org, source: "cache", cachedAt}
else missing/expired
Cache->>GH: fetch /orgs/{login} (with timeout)
GH-->>Cache: org JSON + rate-limit headers
Cache->>Cache: persist (IndexedDB) & update TTL (localStorage)
Cache-->>App: {org, source: "network", cachedAt}
end
App->>App: setLoading(false)
App-->>User: render org card
User->>App: click Refresh
App->>App: setRefreshing(true)
App->>Cache: refreshOrganization(login)
Cache->>GH: fetch /orgs/{login}
GH-->>Cache: fresh org JSON + headers
Cache->>Cache: update IndexedDB & TTL
Cache-->>App: refreshed result
App->>App: setRefreshing(false)
App-->>User: update display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/github.ts`:
- Around line 39-52: The setRateLimitWindow function currently calls
localStorage.setItem(RATE_LIMIT_KEY, ...) directly which can throw and turn a
valid GitHub response into a failure; make rate-limit persistence best-effort by
surrounding the localStorage.setItem call with a try/catch and swallowing or
logging storage errors (do not rethrow) so that failures to persist rate-limit
metadata do not affect the normal response flow; keep the rest of the function
(parsing headers, building the RateLimitWindow object) intact and only guard the
persistence step.
In `@src/App.tsx`:
- Around line 63-70: The search input currently lacks a persistent accessible
name; update the form to provide one by adding a visible <label> tied to the
input via htmlFor/id or by adding an aria-label/aria-labelledby to the input
(e.g., give the input an id like "search-input" and a label text such as
"Organization login" or add aria-label="Organization login"); ensure this change
is applied where handleSearch, query, setQuery, loading, and refreshing are used
and that the disabled prop remains unchanged so screen readers can always
identify the field.
- Around line 25-123: The component currently hard-codes all user-visible
strings in App.tsx (heading, subtitle, input placeholder, button labels
including "Searching..." and "Refreshing...", source display strings, "Last
fetched:" label, "Open on GitHub" and the fallback error messages "Failed to
fetch organization." / "Failed to refresh organization."); extract these into
your i18n resource files (e.g. keys like app.title, app.subtitle,
search.placeholder, button.search, button.searching, button.refresh,
button.refreshing, link.openOnGithub, meta.lastFetched, source.cache,
source.api, error.fetch, error.refresh) and replace the literals in the JSX and
the catch blocks with lookups via the project i18n API (e.g. import t from i18n
or useTranslation and call t('...')), preserving dynamic parts (e.g. include the
actual error message when err instanceof Error by combining t('error.fetch')
with err.message). Ensure you update both render strings and error/default
messages in handleSearch and handleRefresh and keep keys consistent with
existing locale structure.
In `@src/cache/orgCache.ts`:
- Around line 104-128: The cache layer must be best-effort: ensure readCached
failures don’t abort the function and writeCached/setMeta failures don’t
overwrite the successful response. Wrap the getMeta/readCached branch so that
any exception from getMeta or readCached is caught and treated as a cache miss
(proceed to call getOrganizationFromGitHub), and after obtaining org from
getOrganizationFromGitHub, call writeCached and setMeta inside their own
try/catch blocks that swallow/log errors but do not rethrow so the function
still returns the fresh org and source info. Refer to getMeta, readCached,
getOrganizationFromGitHub, writeCached, and setMeta when applying these changes.
In `@vite.config.ts`:
- Around line 1-7: The Vite config registers only the React plugin (defineConfig
and plugins: [react()]) but does not register the `@tailwindcss/vite` plugin while
package.json added `@tailwindcss/vite` and tailwindcss; either remove those
devDependencies or import and add the Tailwind Vite plugin to the plugins array.
Specifically, add an import for the Tailwind Vite plugin (from
"@tailwindcss/vite") and include its invocation alongside react() inside the
plugins array in the default export (or delete the unused `@tailwindcss/vite` and
tailwindcss entries from package.json if you prefer not to use Tailwind).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3d089fc-69d2-4f83-a540-a0167213e0a8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonsrc/App.csssrc/App.tsxsrc/api/github.tssrc/cache/orgCache.tsvite.config.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/cache/orgCache.ts (1)
139-146:⚠️ Potential issue | 🟡 MinorMove
setMetainside the cache-write try block for consistency.If
writeCachedfails, the metadata should not be updated. The current order can leave localStorage claiming valid cached data when IndexedDB actually failed to persist it—leading to repeated failed cache reads until TTL expires.♻️ Proposed fix
try { await writeCached(record); + setMeta(login, { cachedAt, expiresAt }); } catch { // Ignore cache write errors; return fresh data anyway. } - setMeta(login, { cachedAt, expiresAt }); return { org, source: "network", cachedAt };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cache/orgCache.ts` around lines 139 - 146, The setMeta call is currently executed regardless of whether writeCached(record) succeeds, which can leave metadata claiming cached data exists when IndexedDB failed; move the setMeta(login, { cachedAt, expiresAt }) call into the try block immediately after await writeCached(record) so metadata is only updated on successful cache writes, keep the catch block to ignore write errors, and ensure the subsequent return { org, source: "network", cachedAt } remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/github.ts`:
- Around line 81-100: The getOrganizationFromGitHub function currently calls
fetch with no timeout; wrap the request with an AbortController and a timeout
(e.g., 10-15s) so the request is aborted if it takes too long: create an
AbortController, start a setTimeout that calls controller.abort() after the
timeout, pass controller.signal into the fetch options, and clear the timer
after fetch completes (success or failure). Ensure aborts are handled
meaningfully: catch AbortError from the fetch and throw a clear timeout Error
(or map it into the same error flow used by parseError). Update
getOrganizationFromGitHub to use the controller and timer while preserving calls
to setRateLimitWindow(response.headers) and the existing error handling.
In `@src/App.css`:
- Around line 35-45: Update the .srOnly CSS rule to replace the deprecated clip
property with clip-path by using clip-path: inset(50%) while preserving existing
accessibility layout; locate the .srOnly class in App.css and remove or replace
clip: rect(0, 0, 0, 0) with clip-path: inset(50%); keep the other declarations
(position, width, height, padding, margin, overflow, white-space, border)
unchanged so the screen-reader-only behavior remains intact.
In `@src/cache/orgCache.ts`:
- Around line 51-64: openDatabase() returns an open IndexedDB connection but
readCached, writeCached, and removeCached never close it; wrap each call site
that awaits openDatabase() (the functions readCached, writeCached, removeCached)
in a try/finally and call db.close() in the finally block so the database
connection is always closed after the transaction completes or errors, ensuring
you still return/throw the transaction result inside the try and only close in
finally.
---
Duplicate comments:
In `@src/cache/orgCache.ts`:
- Around line 139-146: The setMeta call is currently executed regardless of
whether writeCached(record) succeeds, which can leave metadata claiming cached
data exists when IndexedDB failed; move the setMeta(login, { cachedAt, expiresAt
}) call into the try block immediately after await writeCached(record) so
metadata is only updated on successful cache writes, keep the catch block to
ignore write errors, and ensure the subsequent return { org, source: "network",
cachedAt } remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ab3bb37-8d94-425e-8e90-fad698562aaa
📒 Files selected for processing (6)
src/App.csssrc/App.tsxsrc/api/github.tssrc/cache/orgCache.tssrc/i18n/strings.tsvite.config.ts
… rate-limit safeguards, request timeout, and accessibility/i18n updates
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/github.ts`:
- Around line 30-35: In getRateLimitWindow(), stop trusting localStorage by
parsing into unknown and validating the shape before casting to RateLimitWindow:
parse raw with JSON.parse into an unknown, check that the resulting object has
the expected numeric fields (e.g., blockedUntil is a finite number and any other
timestamp/count fields used elsewhere), and return null if validation fails;
apply the same validation approach where the parsed value is later consumed (see
the logic around lines handling blockedUntil/wait message) so any corrupted or
tampered storage yields null instead of producing NaN or bad UX.
- Around line 67-80: The user-facing literals in getRateLimitGuardError (and the
earlier response status message returned by the error helper) must be moved to
i18n resources: replace the hardcoded string returned by getRateLimitGuardError
(including the dynamic waitMinutes interpolation) with a call to the
i18n/localization helper (e.g., t('github.rateLimit', { minutes: waitMinutes }))
and move the message template into the locale file; likewise replace the
fallback response.statusText string return in the error helper with an i18n key
(e.g., t('github.requestFailed', { status: response.status })) so all
user-visible messages (getRateLimitGuardError and the request error helper) are
externalized and formatted via the i18n API.
In `@src/App.css`:
- Around line 28-33: The CSS uses camelCase class selectors (e.g., .searchForm
and the other camelCase selectors at the referenced lines) which breaks project
naming consistency; rename these selectors to kebab-case (e.g., .search-form)
and provide a compatibility bridge by adding the original camelCase selectors as
aliases that reuse the new kebab-case rules (duplicate the rules or use
comma-separated selectors) so existing markup/components keep working; after
updating the CSS, search codebase for occurrences of the camelCase class names
(components, JSX/HTML) and update them to the new kebab-case names or keep them
until a follow-up refactor is scheduled.
- Around line 1-6: The .app selector uses min-height: 100vh which can jump on
mobile; update the .app rule (min-height) to provide a dynamic-viewport fallback
by adding a 100dvh declaration before the existing 100vh so browsers that
support dynamic viewport use 100dvh while older browsers fall back to 100vh
(modify the min-height in the .app rule accordingly).
In `@src/cache/orgCache.ts`:
- Line 126: The inline validation error in orgCache ("if (!login) throw new
Error('Please enter an organization login.')") must be replaced with a localized
string from the shared i18n resource; import the project's localization helper
(e.g., the i18n/t function used across the codebase) into src/cache/orgCache.ts
and throw the error using a resource key such as errors.orgLoginRequired (or the
established key pattern), e.g., throw new
Error(i18n.t('errors.orgLoginRequired')), and add the corresponding entry to the
i18n resource files so the message is externalized.
- Around line 158-164: The code calls setMeta(login, { cachedAt, expiresAt })
even if writeCached(record) fails, leaving TTL metadata pointing to non-existent
cache entries; update the logic so setMeta is executed only after a successful
write: move the setMeta call into the try block after await writeCached(record)
(or otherwise guard it behind a successful write result) and preserve the
current catch behavior to ignore write errors while avoiding writing metadata on
failure; reference functions/variables: writeCached, setMeta, record, login,
cachedAt, expiresAt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95ce68dc-c0c9-454d-a8b0-8ad251730b3f
📒 Files selected for processing (3)
src/App.csssrc/api/github.tssrc/cache/orgCache.ts
| function getRateLimitWindow(): RateLimitWindow | null { | ||
| try { | ||
| const raw = localStorage.getItem(RATE_LIMIT_KEY); | ||
| if (!raw) return null; | ||
| return JSON.parse(raw) as RateLimitWindow; | ||
| } catch { |
There was a problem hiding this comment.
Validate stored rate-limit JSON shape before using it.
JSON.parse(raw) as RateLimitWindow trusts untyped localStorage data. If storage is corrupted/tampered, blockedUntil can become invalid and produce bad UX (for example, NaN in the wait message on Line 79). Parse into unknown, validate numeric fields, and return null when invalid.
🔧 Proposed hardening
function getRateLimitWindow(): RateLimitWindow | null {
try {
const raw = localStorage.getItem(RATE_LIMIT_KEY);
if (!raw) return null;
- return JSON.parse(raw) as RateLimitWindow;
+ const parsed: unknown = JSON.parse(raw);
+ if (!parsed || typeof parsed !== "object") return null;
+ const obj = parsed as Record<string, unknown>;
+ const blockedUntil = Number(obj.blockedUntil);
+ const remainingRaw = obj.remaining;
+ const remaining =
+ remainingRaw === null ? null : Number(remainingRaw);
+
+ if (!Number.isFinite(blockedUntil)) return null;
+ if (remaining !== null && !Number.isFinite(remaining)) return null;
+
+ return { blockedUntil, remaining };
} catch {
return null;
}
}Also applies to: 70-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/github.ts` around lines 30 - 35, In getRateLimitWindow(), stop
trusting localStorage by parsing into unknown and validating the shape before
casting to RateLimitWindow: parse raw with JSON.parse into an unknown, check
that the resulting object has the expected numeric fields (e.g., blockedUntil is
a finite number and any other timestamp/count fields used elsewhere), and return
null if validation fails; apply the same validation approach where the parsed
value is later consumed (see the logic around lines handling blockedUntil/wait
message) so any corrupted or tampered storage yields null instead of producing
NaN or bad UX.
| return response.statusText || `Request failed (${response.status})`; | ||
| } | ||
|
|
||
| function getRateLimitGuardError(): string | null { | ||
| const rateLimit = getRateLimitWindow(); | ||
| if (!rateLimit || !rateLimit.blockedUntil) return null; | ||
| if (Date.now() >= rateLimit.blockedUntil) return null; | ||
|
|
||
| const waitMinutes = Math.max( | ||
| 1, | ||
| Math.ceil((rateLimit.blockedUntil - Date.now()) / 60000), | ||
| ); | ||
| return `GitHub rate limit reached. Try again in about ${waitMinutes} minute(s), or use refresh later.`; | ||
| } |
There was a problem hiding this comment.
Externalize user-facing API error strings to i18n resources.
Messages on Lines 67-80 and Line 100 are shown to end users via App.tsx error state. Move these literals to the i18n string resource and format dynamic parts (like waitMinutes) there.
As per coding guidelines "User-visible strings should be externalized to resource files (i18n)".
Also applies to: 100-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/github.ts` around lines 67 - 80, The user-facing literals in
getRateLimitGuardError (and the earlier response status message returned by the
error helper) must be moved to i18n resources: replace the hardcoded string
returned by getRateLimitGuardError (including the dynamic waitMinutes
interpolation) with a call to the i18n/localization helper (e.g.,
t('github.rateLimit', { minutes: waitMinutes })) and move the message template
into the locale file; likewise replace the fallback response.statusText string
return in the error helper with an i18n key (e.g., t('github.requestFailed', {
status: response.status })) so all user-visible messages (getRateLimitGuardError
and the request error helper) are externalized and formatted via the i18n API.
| .app { | ||
| min-height: 100vh; | ||
| display: grid; | ||
| place-items: center; | ||
| padding: 1.5rem; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use dynamic viewport unit fallback for mobile viewport stability.
At Line 2, 100vh can cause visible jump/cropping on mobile browser UI show/hide. Add 100dvh fallback for more stable layout behavior.
📱 Proposed fix
.app {
- min-height: 100vh;
+ min-height: 100vh;
+ min-height: 100dvh;
display: grid;
place-items: center;
padding: 1.5rem;
}As per coding guidelines, "Ensure that ... [the code] adheres to best practices recommended by lighthouse or similar tools for performance."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .app { | |
| min-height: 100vh; | |
| display: grid; | |
| place-items: center; | |
| padding: 1.5rem; | |
| } | |
| .app { | |
| min-height: 100vh; | |
| min-height: 100dvh; | |
| display: grid; | |
| place-items: center; | |
| padding: 1.5rem; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.css` around lines 1 - 6, The .app selector uses min-height: 100vh
which can jump on mobile; update the .app rule (min-height) to provide a
dynamic-viewport fallback by adding a 100dvh declaration before the existing
100vh so browsers that support dynamic viewport use 100dvh while older browsers
fall back to 100vh (modify the min-height in the .app rule accordingly).
| .searchForm { | ||
| margin-top: 1rem; | ||
| display: flex; | ||
| gap: 0.6rem; | ||
| flex-wrap: wrap; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Standardize selector naming convention (camelCase vs kebab-case).
Lines 28, 35, 47, and 103 use camelCase selectors while other selectors are lowercase single-word. This breaks naming consistency and makes stylesheet evolution harder. Consider migrating to kebab-case with a compatibility bridge.
♻️ Proposed migration-safe refactor
-.searchForm {
+.searchForm,
+.search-form {
margin-top: 1rem;
display: flex;
gap: 0.6rem;
flex-wrap: wrap;
}
-.srOnly {
+.srOnly,
+.sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip-path: inset(50%);
white-space: nowrap;
border: 0;
}
-.searchInput {
+.searchInput,
+.search-input {
flex: 1;
min-width: 220px;
padding: 0.7rem 0.8rem;
border-radius: 10px;
border: 1px solid `#3b3b3b`;
background: `#0f0f0f`;
color: `#f3f3f3`;
}
-.cardHeader {
+.cardHeader,
+.card-header {
display: flex;
gap: 0.85rem;
align-items: center;
}As per coding guidelines, "Ensure that ... [the code] adheres to similar naming conventions for classes, ids."
Also applies to: 35-35, 47-47, 103-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.css` around lines 28 - 33, The CSS uses camelCase class selectors
(e.g., .searchForm and the other camelCase selectors at the referenced lines)
which breaks project naming consistency; rename these selectors to kebab-case
(e.g., .search-form) and provide a compatibility bridge by adding the original
camelCase selectors as aliases that reuse the new kebab-case rules (duplicate
the rules or use comma-separated selectors) so existing markup/components keep
working; after updating the CSS, search codebase for occurrences of the
camelCase class names (components, JSX/HTML) and update them to the new
kebab-case names or keep them until a follow-up refactor is scheduled.
| options?: { forceRefresh?: boolean; ttlMs?: number }, | ||
| ): Promise<OrgResult> { | ||
| const login = rawLogin.trim().toLowerCase(); | ||
| if (!login) throw new Error("Please enter an organization login."); |
There was a problem hiding this comment.
Externalize validation error text to i18n resources.
The message on Line 126 is user-visible and should come from the shared localization resource instead of an inline literal.
As per coding guidelines "User-visible strings should be externalized to resource files (i18n)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cache/orgCache.ts` at line 126, The inline validation error in orgCache
("if (!login) throw new Error('Please enter an organization login.')") must be
replaced with a localized string from the shared i18n resource; import the
project's localization helper (e.g., the i18n/t function used across the
codebase) into src/cache/orgCache.ts and throw the error using a resource key
such as errors.orgLoginRequired (or the established key pattern), e.g., throw
new Error(i18n.t('errors.orgLoginRequired')), and add the corresponding entry to
the i18n resource files so the message is externalized.
| try { | ||
| await writeCached(record); | ||
| } catch { | ||
| // Ignore cache write errors; return fresh data anyway. | ||
| } | ||
| setMeta(login, { cachedAt, expiresAt }); | ||
|
|
There was a problem hiding this comment.
Only set TTL metadata after a successful IndexedDB write.
On Lines 158-164, setMeta is called even when writeCached fails. That can leave valid TTL metadata pointing to missing cache data, causing repeated avoidable cache-miss reads until expiration.
🔧 Proposed fix
try {
await writeCached(record);
+ setMeta(login, { cachedAt, expiresAt });
} catch {
// Ignore cache write errors; return fresh data anyway.
+ clearMeta(login);
}
- setMeta(login, { cachedAt, expiresAt });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cache/orgCache.ts` around lines 158 - 164, The code calls setMeta(login,
{ cachedAt, expiresAt }) even if writeCached(record) fails, leaving TTL metadata
pointing to non-existent cache entries; update the logic so setMeta is executed
only after a successful write: move the setMeta call into the try block after
await writeCached(record) (or otherwise guard it behind a successful write
result) and preserve the current catch behavior to ignore write errors while
avoiding writing metadata on failure; reference functions/variables:
writeCached, setMeta, record, login, cachedAt, expiresAt.
Addressed Issues:
Fixes #42
Screenshots/Recordings:
Additional Notes:
organizationsobject store)forceRefresh) to fetch latest data from GitHub.x-ratelimit-remainingandx-ratelimit-resetnpm run lint✅npm run build✅Checklist
Summary by CodeRabbit
New Features
Chores