[codex] cache profile analyses#17
Open
saurabhhhcodes wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR integrates SWR for profile analysis fetching. A new ChangesSWR Integration for Profile Analysis
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify 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.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a73908d-ede6-49a6-93a7-6349d92b2985
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/app/[username]/ProfileClient.tsxsrc/lib/profile-analysis.ts
Comment on lines
+50
to
+56
| 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), |
Contributor
There was a problem hiding this comment.
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 Agents
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/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.
Comment on lines
+98
to
105
| const refreshAnalysis = useCallback( | ||
| async (options: { force?: boolean; nosave?: boolean } = {}) => { | ||
| setIsRefreshing(true); | ||
|
|
||
| try { | ||
| const nextData = await fetchProfileAnalysis(username, options); | ||
| await mutate(nextData, { revalidate: false }); | ||
|
|
Contributor
There was a problem hiding this comment.
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 Agents
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/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.
Comment on lines
+113
to
+127
| } 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", | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
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 Agents
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/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.
Comment on lines
+38
to
+41
| const res = await fetch(buildProfileUrl(username, options), { | ||
| cache: "no-store", | ||
| }); | ||
| const result = await res.json().catch(() => ({})); |
Contributor
There was a problem hiding this comment.
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(() => ({}));📝 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.
Suggested change
| 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(() => ({})); |
🤖 Prompt for AI Agents
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.
Comment on lines
+43
to
+48
| if (res.status === 403 && result?.error === "Star required") { | ||
| throw new ProfileAnalysisError( | ||
| result?.message || "Star required", | ||
| 403, | ||
| "STAR_REQUIRED", | ||
| ); |
Contributor
There was a problem hiding this comment.
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",
);
}📝 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.
Suggested change
| 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", | |
| ); | |
| } |
🤖 Prompt for AI Agents
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Why
Validation
Summary by CodeRabbit