fix: cache invalidation and org priority bugs in forms#1613
fix: cache invalidation and org priority bugs in forms#1613borkarsaish65 wants to merge 2 commits into
Conversation
- Add cache invalidation on create to prevent stale fallback (default org) data - Fix read() priority to prefer user's org over default org (was using tenant_code, should be organization_code) - Fix readAllFormsVersion() to cache user org form over default org form when both exist Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe form service now invalidates cached entries after creating a form and adjusts organization selection logic in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/form.js (1)
252-259:⚠️ Potential issue | 🟠 MajorDefault-org entries written here are still invisible to normal
read()calls.When this branch falls back to a default-org form,
forms.set()writes the cache entry underdefaults.orgCode, butcacheHelper.forms.get()only looks up the caller'sorgCodebefore hitting the database. So default-only forms still are not actually prewarmed for user-org reads. Either pass the requester org into this flow and cache under that key, or teachforms.get()to consult the default-org key before querying the database.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/form.js` around lines 252 - 259, The code currently caches a fallback default-org form under defaults.orgCode which makes it invisible to reads from the requesting org; update the cache key so fallback default forms are stored under the requester org. Concretely, in the branch where you compute formData (using completeFormData and formData.sub_type) change the cacheHelper.forms.set call to use formData.organization_code || tenantCode (instead of defaults.orgCode) so the cached entry is available to cacheHelper.forms.get for the calling org; alternatively, if you prefer the other approach, make cacheHelper.forms.get consult defaults.orgCode when the caller key misses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/services/form.js`:
- Around line 252-259: The code currently caches a fallback default-org form
under defaults.orgCode which makes it invisible to reads from the requesting
org; update the cache key so fallback default forms are stored under the
requester org. Concretely, in the branch where you compute formData (using
completeFormData and formData.sub_type) change the cacheHelper.forms.set call to
use formData.organization_code || tenantCode (instead of defaults.orgCode) so
the cached entry is available to cacheHelper.forms.get for the calling org;
alternatively, if you prefer the other approach, make cacheHelper.forms.get
consult defaults.orgCode when the caller key misses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 353d5a2c-1639-43c6-9ed7-a04c0d8e50ef
📒 Files selected for processing (1)
src/services/form.js
|
|
||
| // Invalidate cache so stale fallback (default org) is not served | ||
| try { | ||
| if (bodyData.type && bodyData.sub_type) { | ||
| await cacheHelper.forms.delete(tenantCode, orgCode, bodyData.type, bodyData.sub_type) | ||
| } | ||
| } catch (cacheError) { | ||
| console.error('Failed to invalidate form cache:', cacheError) | ||
| } |
There was a problem hiding this comment.
Why do we need to delete the cache when a new form is created? It’s new, so it wouldn’t have been cached anyway. Are we caching a list somewhere? It doesn’t look like we’re clearing any list cache either.
| @@ -240,7 +249,10 @@ | |||
| ) | |||
There was a problem hiding this comment.
Why are we caching this? We’re not even using cache for the list.
Here we’re caching individual forms, but await form.getAllFormsVersion only returns the version without the rest of the data. That means reads will fail, because the cache ends up serving incomplete, unusable form data.
| await cacheHelper.forms.delete( | ||
| tenantCode, | ||
| originalForm.organization_code || orgCode, | ||
| originalForm.type, | ||
| originalForm.sub_type | ||
| ) | ||
| } |
There was a problem hiding this comment.
It looks like we’re only clearing the cache for the current org. What happens to other orgs when default org data is updated (same for delete, etc.)? Since default org data is shared across all orgs in the tenant, shouldn’t we be invalidating their caches as well?
|
OT |
Release Notes
read()method to prioritize user's organization form over default organization form by usingorganization_codeinstead oftenant_codefor comparisonreadAllFormsVersion()to cache user organization forms over default organization forms when both exist, preferring custom organization entriesLines Changed by Author