fix: add GraphQL cache invalidation for programs and modules#3387
fix: add GraphQL cache invalidation for programs and modules#3387arkid15r merged 13 commits intoOWASP:mainfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a GraphQL resolver caching extension and top-level cache helpers, wires cache invalidation into program/module mutations via transaction.on_commit, updates frontend queries to use cache-and-network, and changes mutations to await refetchQueries to ensure fresh data is shown after writes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (1)
28-30: Missing refetch or state update after status mutation.The mutation doesn't include
refetchQueriesto refresh the data after a successful status update. While the toast indicates success, the displayed status on the page won't update becauseprogramremains stale in local state.Per the PR objectives, mutations should use
refetchQuerieswithawaitRefetchQueries: trueto ensure fresh data is fetched after writes.Suggested fix
const [updateProgram] = useMutation(UpdateProgramStatusDocument, { onError: handleAppError, + refetchQueries: [{ query: GetProgramAndModulesDocument, variables: { programKey } }], + awaitRefetchQueries: true, })Alternatively, you could optimistically update the local state in the
updateStatusfunction after a successful mutation:await updateProgram({ variables: { inputData: { key: program.key, name: program.name, status: newStatus, }, }, }) + + setProgram((prev) => (prev ? { ...prev, status: newStatus } : null))frontend/src/app/my/mentorship/programs/[programKey]/modules/create/page.tsx (1)
116-124: Add type guard or optional chaining for error handling.
errin catch blocks has typeunknown. Direct property access without a type guard could fail at runtime if the error isn't anErrorobject. The create program page uses safer optional chaining (err?.message).Suggested fix
} catch (err) { addToast({ title: 'Creation Failed', - description: err.message || 'Something went wrong while creating the module.', + description: err instanceof Error ? err.message : 'Something went wrong while creating the module.', color: 'danger', variant: 'solid', timeout: 4000, }) }frontend/src/app/my/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
35-53: Error handling may misclassify network errors as "not found".When
erroris present but not a "not found" error (e.g., network failure) andmoduleis null (no cached data), the UI shows "Module Not Found" (404) which is misleading. Consider distinguishing between actual 404s and other errors without cached data.💡 Suggested improvement
if (isLoading) return <LoadingSpinner /> + if (error && !error.message?.toLowerCase().includes('not found')) { + return ( + <ErrorDisplay + statusCode={500} + title="Error Loading Module" + message="An error occurred while loading the module. Please try again." + /> + ) + } + if (!module) { return ( <ErrorDisplay statusCode={404} title="Module Not Found" message="Sorry, the module you're looking for doesn't exist." /> ) }
🤖 Fix all issues with AI agents
In `@frontend/__tests__/unit/pages/CreateProgram.test.tsx`:
- Around line 162-178: The test currently only asserts awaitRefetchQueries on
mockCreateProgram and misses asserting refetchQueries; update the assertion for
mockCreateProgram (the expect(mockCreateProgram).toHaveBeenCalledWith(...)
block) to also include a refetchQueries property with the expected query (or at
least an array) so the mutation call contains both awaitRefetchQueries: true and
refetchQueries: [/* expected query or queries */]; adjust the
expect.objectContaining to include refetchQueries alongside variables and
awaitRefetchQueries to lock in cache-refresh behavior.
In `@frontend/src/app/mentorship/programs/`[programKey]/page.tsx:
- Around line 20-24: The current useQuery call (GetProgramAndModulesDocument
with variables: { programKey } and fetchPolicy: 'cache-and-network')
unconditionally shows a spinner when loading is true, which hides cached data;
update the render logic that checks loading (the spinner at the current page
component) to only show the spinner when loading is true AND data is falsy
(e.g., if (!data && loading) show spinner), so cached data renders while the
network request proceeds; apply the same change to the analogous modules page
component that uses useQuery there.
- Around line 30-34: Extract the 404 detection into a reusable helper named
isNotFoundError(error) (place it in a shared module like utils/errorUtils or
lib/graphql/errorUtils) and replace the inline checks that use
graphQLRequestError.message?.toLowerCase().includes('not found') in page.tsx and
modules/[moduleKey]/page.tsx with if (isNotFoundError(graphQLRequestError)) {
... } else { handleAppError(graphQLRequestError) }; implement isNotFoundError to
first inspect error.graphQLErrors[].extensions.code for 'NOT_FOUND' or
'RESOURCE_NOT_FOUND', then check error.networkError?.statusCode/response status
for 404, and finally fall back to a case-insensitive message substring match to
preserve current behavior. Ensure the helper is exported and imported where used
so both files share the same logic.
🧹 Nitpick comments (4)
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (1)
32-37: Consider refining the loading state to avoid UI flicker during background refetch.The
fetchPolicy: 'cache-and-network'addition aligns well with the PR's cache invalidation objectives. However, combined withnotifyOnNetworkStatusChange: true, theloadingboolean will betrueduring background refetches, which could cause theLoadingSpinnerto briefly replace existing content.Consider checking for cached data presence or using
networkStatusto distinguish initial load from background refetch:Suggested improvement
- const { data, loading: isQueryLoading } = useQuery(GetProgramAndModulesDocument, { + const { data, loading: isQueryLoading, networkStatus } = useQuery(GetProgramAndModulesDocument, { variables: { programKey }, skip: !programKey, fetchPolicy: 'cache-and-network', notifyOnNetworkStatusChange: true, }) - const isLoading = isQueryLoading + // Only show loading spinner on initial load, not background refetch + const isLoading = isQueryLoading && !dataThis ensures the spinner only appears when there's no cached data to display, preventing flicker during background network requests.
frontend/src/app/mentorship/programs/[programKey]/modules/[moduleKey]/page.tsx (1)
32-39: Consider extracting duplicateisNotFoundcomputation.The same
error.message?.toLowerCase().includes('not found')logic is computed both in theuseEffect(line 34) and in the render block (line 44). While the overhead is negligible, extracting this to a single derived value improves maintainability.♻️ Suggested refactor
+ const isNotFound = error?.message?.toLowerCase().includes('not found') + useEffect(() => { if (error) { - const isNotFound = error.message?.toLowerCase().includes('not found') if (!isNotFound) { handleAppError(error) } } - }, [error]) + }, [error, isNotFound]) if (isLoading) return <LoadingSpinner /> if (error) { - const isNotFound = error.message?.toLowerCase().includes('not found') return ( <ErrorDisplay statusCode={isNotFound ? 404 : 500}Also applies to: 43-56
backend/apps/common/graphql_cache.py (2)
10-10: Consider adding debug logging for cache invalidation.The logger is imported but unused. Adding debug-level logging in
invalidate_graphql_cachewould help diagnose cache issues in production.♻️ Optional: Add debug logging
def invalidate_graphql_cache(field_name: str, field_args: dict) -> bool: ... key = f"{field_name}:{json.dumps(field_args, sort_keys=True)}" cache_key = f"{settings.GRAPHQL_RESOLVER_CACHE_PREFIX}-{hashlib.sha256(key.encode()).hexdigest()}" - result = cache.delete(cache_key) - return result + deleted = cache.delete(cache_key) + logger.debug("Cache invalidation for %s: %s", field_name, "hit" if deleted else "miss") + return deleted
25-26: Minor: Simplify return statement.Per static analysis (RET504), the intermediate assignment is unnecessary.
♻️ Suggested simplification
- result = cache.delete(cache_key) - return result + return cache.delete(cache_key)
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
The CR's comments are unaddressed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/my/mentorship/programs/[programKey]/page.tsx (1)
93-114: Potential null reference error due to state sync timing.With
cache-and-networkandnotifyOnNetworkStatusChange: true, when cached data exists:
isLoading= true (background fetch),data= cached value, butprogram= null (useEffect hasn't run)- Line 93:
true && !cachedValue→ false (continues)- Line 95:
!null && !true→ false (continues)- Line 106:
program.status→ TypeErrorUnlike
ModuleDetailsPagewhich derivesprogramModuledirectly fromdata, this component stores it in state viauseEffect, creating a timing gap.🔧 Suggested fix: Guard against null program before accessing properties
- if (isLoading && !data) return <LoadingSpinner /> + if (isLoading && !program) return <LoadingSpinner /> if (!program && !isLoading) {Alternatively, derive
programdirectly fromdatalike the other page does:- const [program, setProgram] = useState<Program | null>(null) - const [modules, setModules] = useState<Module[]>([]) ... - useEffect(() => { - if (data?.getProgram) { - setProgram(data.getProgram) - setModules(data.getProgramModules || []) - } - }, [data]) + const program = data?.getProgram ?? null + const modules = data?.getProgramModules ?? []This eliminates the race condition by deriving state synchronously from
data.
There was a problem hiding this comment.
Hey, I tested this and the invalidation works.
However, the frontend throws an error after I visit a program.
Error:
Cannot read properties of null (reading 'status') in src/app/my/mentorship/programs/[programKey]/page.tsx (106:53) @ ProgramDetailsPage
Refreshing the page after this, loads the program. I was not able to reproduce it consistently, but this happens when I do the following:
- Visit any single program page
- Use breadcrumb "Mentorship" to go back to programs list page
- Visit any single program page (Errors)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/apps/common/extensions.py`:
- Around line 68-87: invalidate_program_cache and invalidate_module_cache
currently build cache key dicts using camelCase names (programKey, moduleKey)
which don't match the resolver kwargs produced by Strawberry (snake_case), so
cache invalidation never hits; update both methods to pass snake_case keys
(e.g., {"program_key": program_key} and {"module_key": module_key,
"program_key": program_key}) when calling
self.invalidate_cache("getProgram"/"getModule") so the generated keys match the
middleware's resolver kwargs.
🧹 Nitpick comments (1)
backend/apps/common/extensions.py (1)
105-106: Verify SchemaExtension can be instantiated without execution_context.
_cache_extension = CacheExtension()is created at import time. In Strawberry,SchemaExtensionoften expects anexecution_contextin__init__. If that’s required in v0.289.0, this will raise at startup. Consider refactoring invalidation helpers to avoid constructingSchemaExtensiondirectly (e.g., extract a standalone key helper), or confirm the constructor acceptsNone.
|
Hey @rudransh-shrivastava, please review this PR. It seems fine to me, but CodeRabbit is showing an error. Could you help me check if any changes are required? |
There was a problem hiding this comment.
Great work, looks good to me!
Please add relevant backend tests as per CONTRIBUTING.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/apps/mentorship/api/internal/mutations/module.py`:
- Around line 123-124: The cache invalidation call to invalidate_program_cache
is being invoked inside a transaction.atomic block; change this so invalidation
runs only after a successful commit by registering it with transaction.on_commit
(e.g., replace direct invalidate_program_cache(program.key) with
transaction.on_commit(lambda: invalidate_program_cache(program.key))) or move
the call outside the atomic block, ensuring you reference the same
invalidate_program_cache and the surrounding transaction.atomic context so
invalidation only occurs post-commit.
♻️ Duplicate comments (1)
backend/apps/mentorship/api/internal/mutations/module.py (1)
407-409: Same on-commit concern applies here.
Consider batching invalidation inside a singletransaction.on_commitcallback to prevent stale recache windows.🔧 Suggested fix
- invalidate_module_cache(old_module_key, module.program.key) - if module.key != old_module_key: - invalidate_module_cache(module.key, module.program.key) + def _invalidate(): + invalidate_module_cache(old_module_key, module.program.key) + if module.key != old_module_key: + invalidate_module_cache(module.key, module.program.key) + transaction.on_commit(_invalidate)
|
Hey @rudransh-shrivastava, |
|
@HarshitVerma109 I have already approved it. Also, you need to add the backend tests as I have mentioned in my previous review. |
|
Hey @rudransh-shrivastava, I just added a backend test. Please take a look and let me know if any changes are needed! |
kasya
left a comment
kasya
left a comment
There was a problem hiding this comment.
@HarshitVerma109 That's really cool! Thank you!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@backend/apps/api/internal/extensions/cache.py`:
- Around line 37-49: Update generate_key to use a defensive JSON encoder so
non-JSON-native types (e.g., datetime, UUID, Decimal) serialize correctly:
replace json.dumps(field_args, sort_keys=True) with json.dumps(field_args,
sort_keys=True, cls=DjangoJSONEncoder) and import DjangoJSONEncoder from
django.core.serializers.json; also add a unit test in TestGenerateKey
(backend/tests/apps/api/internal/extensions/cache_test.py) that passes args
containing a datetime and a UUID to generate_key to assert it returns a stable,
non-erroring string that includes settings.GRAPHQL_RESOLVER_CACHE_PREFIX.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3387 +/- ##
==========================================
+ Coverage 85.56% 85.61% +0.04%
==========================================
Files 461 461
Lines 14258 14254 -4
Branches 1907 1905 -2
==========================================
+ Hits 12200 12203 +3
+ Misses 1679 1672 -7
Partials 379 379
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
4 participants
Proposed change
Resolves #3348
This PR fixes the mentorship portal cache invalidation issue where updates to programs and modules were not visible immediately after saving. The backend GraphQL resolver cache was not being cleared after mutations, causing stale data to be served.
Backend Changes:
graphql_cache.pyutility to invalidate specific cache entriesFrontend Changes:
Checklist
make check-testlocally: all warnings addressed, tests passed