[codex] cache profile analyses - #17
Conversation
Sorry, something went wrong.
WalkthroughThis PR integrates SWR for profile analysis fetching. A new profile-analysis.ts module exports ProfileAnalysisError, ProfileAnalysisOptions, and fetchProfileAnalysis() function. ProfileClient now uses useSWR instead of manual state, gated by username validation, with refresh callbacks and error/modal handling. ChangesSWR Integration for Profile Analysis
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem🚥 Pre-merge checks | ✅ 4 | ❌ 1 ❌ Failed checks (1 warning)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agentsVerify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`[username]/ProfileClient.tsx:
- Around line 98-105: The refreshAnalysis function is mutating the shared SWR
cache even for temporary "nosave" responses; change refreshAnalysis (and the
other occurrence) so that when options.nosave === true you DO NOT call
mutate(...) on the shared ["profile-analysis", username] entry—instead store the
fetched result in a separate local preview state (e.g., previewAnalysis state)
and use that for immediate UI rendering; keep the existing mutate(...) call only
for non-nosave responses so the shared cache remains the locked/historical
snapshot preserved by fetchProfileAnalysis.
- Around line 50-56: The SWR key and fetcher must use the normalized
safeUsername so cache entries for "/Foo" and "/foo" are the same: change the
useSWR call to use isInvalidUsername ? null : ["profile-analysis", safeUsername]
and make the fetcher call fetchProfileAnalysis(safeUsername) (or simply use ()
=> fetchProfileAnalysis(safeUsername)); likewise update refreshAnalysis (and any
other manual fetches around lines 98-104 that call
fetchProfileAnalysis(username)) to call fetchProfileAnalysis(safeUsername) or
call mutate() without raw username so all reads/writes use the same normalized
safeUsername.
- Around line 113-127: The catch block that handles refresh failures currently
calls setError (and thus triggers the full-page error UI) even when stale
analysis exists; change this so manual-refresh failures do not overwrite the
top-level error state when there is already cached analysis: in the catch for
refreshError (the branch checking ProfileAnalysisError && code ===
"STAR_REQUIRED" can remain), avoid calling setError(...) directly—instead
introduce or use a separate refresh-specific state (e.g., refreshError or
refreshStatus) to surface transient refresh failures, or only call setError when
there is no existing analysis/cached data; keep setShowStarModal behavior as-is.
Ensure you update any rendering logic that shows the full-page error (the code
that checks error) to only switch to full-page error when error is set and there
is no cached analysis, or rely on the new refresh-specific state to display a
non-blocking inline error.
In `@src/lib/profile-analysis.ts`:
- Around line 38-41: Wrap the fetch call in a try/catch so network/transport
rejections are converted into a ProfileAnalysisError: replace the direct await
fetch(buildProfileUrl(...)) with a try block that awaits fetch, and in the catch
create and throw a new ProfileAnalysisError (including the original error as
cause or in the message and any useful context like the username/URL). Keep the
existing logic that parses res.json() but ensure transport failures never escape
as native errors by always throwing ProfileAnalysisError from the catch. Use the
existing buildProfileUrl and ProfileAnalysisError symbols to locate where to
change.
- Around line 43-48: The current 403 branch is brittle because it checks
result?.error === "Star required"; update that conditional in the code around
the ProfileAnalysisError throw so it detects the star-gate more robustly: keep
the res.status === 403 guard but replace the strict equality with a normalized,
defensive check such as checking for a structured error code (e.g. result?.code
=== "STAR_REQUIRED") or, if absent, a case-insensitive substring match like
(result?.error || result?.message || "").toLowerCase().includes("star"); then
throw ProfileAnalysisError as before (preserving message and code) so the modal
flow in ProfileClient still triggers even if backend wording changes.
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a73908d-ede6-49a6-93a7-6349d92b2985
📥 CommitsReviewing files that changed from the base of the PR and between 387a102 and 497ba85.
⛔ Files ignored due to path filters (1)- package-lock.json is excluded by !**/package-lock.json
- package.json
- src/app/[username]/ProfileClient.tsx
- src/lib/profile-analysis.ts
Sorry, something went wrong.
| const safeUsername = (username || "").toLowerCase(); | ||
| const isInvalidUsername = | ||
| !username || safeUsername === "undefined" || safeUsername === "null"; | ||
|
|
||
| const { data, error: fetchError, mutate } = useSWR( | ||
| isInvalidUsername ? null : ["profile-analysis", username], | ||
| ([, activeUsername]) => fetchProfileAnalysis(activeUsername), |
There was a problem hiding this comment.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Normalize the SWR key and manual fetch to the same username form.
The component already derives safeUsername, but the SWR key and refreshAnalysis() still use raw username. Visiting /Foo and /foo will create different cache entries and miss the reuse this PR is trying to add. Use one normalized value for both the key and fetchProfileAnalysis(...) so the cache stays coherent.
Also applies to: 98-104
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[username]/ProfileClient.tsx around lines 50 - 56, The SWR key and fetcher must use the normalized safeUsername so cache entries for "/Foo" and "/foo" are the same: change the useSWR call to use isInvalidUsername ? null : ["profile-analysis", safeUsername] and make the fetcher call fetchProfileAnalysis(safeUsername) (or simply use () => fetchProfileAnalysis(safeUsername)); likewise update refreshAnalysis (and any other manual fetches around lines 98-104 that call fetchProfileAnalysis(username)) to call fetchProfileAnalysis(safeUsername) or call mutate() without raw username so all reads/writes use the same normalized safeUsername.
Sorry, something went wrong.
| const refreshAnalysis = useCallback( | ||
| async (options: { force?: boolean; nosave?: boolean } = {}) => { | ||
| setIsRefreshing(true); | ||
|
|
||
| try { | ||
| const nextData = await fetchProfileAnalysis(username, options); | ||
| await mutate(nextData, { revalidate: false }); | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Don’t write nosave responses into the main profile cache.
See Latest Analysis explicitly requests nosave: true, but refreshAnalysis() still mutates the shared ["profile-analysis", ...] entry with that response. After one click, navigating away and back will keep showing the unsaved live analysis from SWR instead of the locked/historical snapshot the server contract was preserving. Keep nosave results out of the shared cache, or store them in separate local preview state.
This follows the upstream src/lib/profile-analysis.ts contract, where nosave is intentionally carried through to /api/analyze as a distinct request mode.
Also applies to: 276-279
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[username]/ProfileClient.tsx around lines 98 - 105, The refreshAnalysis function is mutating the shared SWR cache even for temporary "nosave" responses; change refreshAnalysis (and the other occurrence) so that when options.nosave === true you DO NOT call mutate(...) on the shared ["profile-analysis", username] entry—instead store the fetched result in a separate local preview state (e.g., previewAnalysis state) and use that for immediate UI rendering; keep the existing mutate(...) call only for non-nosave responses so the shared cache remains the locked/historical snapshot preserved by fetchProfileAnalysis.
Sorry, something went wrong.
| } catch (refreshError) { | ||
| if ( | ||
| refreshError instanceof ProfileAnalysisError && | ||
| refreshError.code === "STAR_REQUIRED" | ||
| ) { | ||
| setShowStarModal(true); | ||
| setError(null); | ||
| } else { | ||
| setShowStarModal(false); | ||
| setError( | ||
| refreshError instanceof Error | ||
| ? refreshError.message | ||
| : "NETWORK_FAILURE", | ||
| ); | ||
| } |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Preserve the current analysis when a manual refresh fails.
This catch path promotes refresh failures into the same top-level error state used for initial-load failures. Because the component hard-switches to the full-page error UI at Line 160 whenever error is set, one transient refresh error hides already-cached analysis and leaves the user with only “Return to Hub”. Keep stale data visible on refresh failures and surface the refresh error separately.
Also applies to: 160-192
🤖 Prompt for AI AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[username]/ProfileClient.tsx around lines 113 - 127, The catch block that handles refresh failures currently calls setError (and thus triggers the full-page error UI) even when stale analysis exists; change this so manual-refresh failures do not overwrite the top-level error state when there is already cached analysis: in the catch for refreshError (the branch checking ProfileAnalysisError && code === "STAR_REQUIRED" can remain), avoid calling setError(...) directly—instead introduce or use a separate refresh-specific state (e.g., refreshError or refreshStatus) to surface transient refresh failures, or only call setError when there is no existing analysis/cached data; keep setShowStarModal behavior as-is. Ensure you update any rendering logic that shows the full-page error (the code that checks error) to only switch to full-page error when error is set and there is no cached analysis, or rely on the new refresh-specific state to display a non-blocking inline error.
Sorry, something went wrong.
| const res = await fetch(buildProfileUrl(username, options), { | ||
| cache: "no-store", | ||
| }); | ||
| const result = await res.json().catch(() => ({})); |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Normalize transport failures into ProfileAnalysisError (Line 38).
fetch() rejections (offline, DNS, timeout, aborted request) currently throw native errors, so this helper does not fully normalize error shape as intended for shared consumers.
Suggested patch export async function fetchProfileAnalysis(
username: string,
options: ProfileAnalysisOptions = {},
): Promise<AnalysisResult> {
- const res = await fetch(buildProfileUrl(username, options), {
- cache: "no-store",
- });
+ let res: Response;
+ try {
+ res = await fetch(buildProfileUrl(username, options), {
+ cache: "no-store",
+ });
+ } catch (error) {
+ throw new ProfileAnalysisError(
+ error instanceof Error ? error.message : "Network request failed",
+ 0,
+ "NETWORK_ERROR",
+ );
+ }
const result = await res.json().catch(() => ({}));‼️ 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.
| const res = await fetch(buildProfileUrl(username, options), { | |
| cache: "no-store", | |
| }); | |
| const result = await res.json().catch(() => ({})); | |
| let res: Response; | |
| try { | |
| res = await fetch(buildProfileUrl(username, options), { | |
| cache: "no-store", | |
| }); | |
| } catch (error) { | |
| throw new ProfileAnalysisError( | |
| error instanceof Error ? error.message : "Network request failed", | |
| 0, | |
| "NETWORK_ERROR", | |
| ); | |
| } | |
| const result = await res.json().catch(() => ({})); |
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/profile-analysis.ts` around lines 38 - 41, Wrap the fetch call in a try/catch so network/transport rejections are converted into a ProfileAnalysisError: replace the direct await fetch(buildProfileUrl(...)) with a try block that awaits fetch, and in the catch create and throw a new ProfileAnalysisError (including the original error as cause or in the message and any useful context like the username/URL). Keep the existing logic that parses res.json() but ensure transport failures never escape as native errors by always throwing ProfileAnalysisError from the catch. Use the existing buildProfileUrl and ProfileAnalysisError symbols to locate where to change.
Sorry, something went wrong.
| if (res.status === 403 && result?.error === "Star required") { | ||
| throw new ProfileAnalysisError( | ||
| result?.message || "Star required", | ||
| 403, | ||
| "STAR_REQUIRED", | ||
| ); |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Avoid brittle star-gate matching on literal backend text (Line 43).
The 403 star-gate path is keyed to result?.error === "Star required" only. If backend wording changes (while still signaling the same condition), the modal flow in ProfileClient will stop triggering.
Suggested patch- if (res.status === 403 && result?.error === "Star required") {
+ const errorToken =
+ typeof result?.error === "string"
+ ? result.error
+ : typeof result?.code === "string"
+ ? result.code
+ : "";
+
+ if (
+ res.status === 403 &&
+ (errorToken === "Star required" || errorToken === "STAR_REQUIRED")
+ ) {
throw new ProfileAnalysisError(
result?.message || "Star required",
403,
"STAR_REQUIRED",
);
}‼️ 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.
| if (res.status === 403 && result?.error === "Star required") { | |
| throw new ProfileAnalysisError( | |
| result?.message || "Star required", | |
| 403, | |
| "STAR_REQUIRED", | |
| ); | |
| const errorToken = | |
| typeof result?.error === "string" | |
| ? result.error | |
| : typeof result?.code === "string" | |
| ? result.code | |
| : ""; | |
| if ( | |
| res.status === 403 && | |
| (errorToken === "Star required" || errorToken === "STAR_REQUIRED") | |
| ) { | |
| throw new ProfileAnalysisError( | |
| result?.message || "Star required", | |
| 403, | |
| "STAR_REQUIRED", | |
| ); | |
| } |
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/profile-analysis.ts` around lines 43 - 48, The current 403 branch is
brittle because it checks result?.error === "Star required"; update that
conditional in the code around the ProfileAnalysisError throw so it detects the
star-gate more robustly: keep the res.status === 403 guard but replace the
strict equality with a normalized, defensive check such as checking for a
structured error code (e.g. result?.code === "STAR_REQUIRED") or, if absent, a
case-insensitive substring match like (result?.error || result?.message ||
"").toLowerCase().includes("star"); then throw ProfileAnalysisError as before
(preserving message and code) so the modal flow in ProfileClient still triggers
even if backend wording changes.
Sorry, something went wrong.
What changed
Why
Validation
Summary by CodeRabbit