Implement IndexedDB for flow definitions and add revision handling logic #3380
Conversation
WalkthroughAdds IndexedDB persistence for flow definitions with exported async helpers: getFlowDefinition, deleteFlowDefinition, fetchLatestRevision, and postLatestRevision. Introduces DB initialization and upgrade logic (object store keyed by Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
🚀 Deployed on https://deploy-preview-3380--glific-frontend.netlify.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/components/floweditor/FlowEditor.tsx (1)
220-225: Consider improving type safety instead of using @ts-ignoreWhile @ts-ignore works for suppressing TypeScript errors, consider properly typing the
filesobject returned fromloadfiles()to avoid these suppressions.You could define a proper type for the files object:
type LoadedFiles = Record<string, HTMLScriptElement | HTMLLinkElement>;Then update the cleanup code:
- Object.keys(files).forEach((node) => { - // @ts-ignore - if (files[node] && document.body.contains(files[node])) { - // @ts-ignore - document.body.removeChild(files[node]); + Object.keys(files).forEach((node) => { + const element = files[node as keyof typeof files]; + if (element && document.body.contains(element)) { + document.body.removeChild(element); } });src/components/floweditor/FlowEditor.helper.tsx (1)
196-196: Unnecessary async declarationThe function was changed to async but doesn't contain any await statements or return a Promise.
Unless you plan to add async operations to this function, consider reverting this change:
-export const loadfiles = async (startFlowEditor: any) => { +export const loadfiles = (startFlowEditor: any) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/floweditor/FlowEditor.helper.tsx(2 hunks)src/components/floweditor/FlowEditor.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/floweditor/FlowEditor.tsxsrc/components/floweditor/FlowEditor.helper.tsx
🧬 Code Graph Analysis (2)
src/components/floweditor/FlowEditor.tsx (2)
src/components/floweditor/FlowEditor.helper.tsx (3)
fetchLatestRevision(67-77)getFlowDefinition(44-65)postLatestRevision(79-92)src/mocks/Flow.tsx (1)
publishFlow(423-438)
src/components/floweditor/FlowEditor.helper.tsx (1)
src/config/index.ts (1)
FLOW_EDITOR_API(30-30)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
- GitHub Check: build
- GitHub Check: CI
🔇 Additional comments (1)
src/components/floweditor/FlowEditor.tsx (1)
22-33: LGTM!The imports are appropriate for the new IndexedDB and revision handling functionality.
…ndexedDB mock for tests
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/floweditor/FlowEditor.helper.tsx(2 hunks)src/components/floweditor/FlowEditor.test.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/floweditor/FlowEditor.helper.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
src/components/floweditor/FlowEditor.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
- GitHub Check: CI
- GitHub Check: build
🔇 Additional comments (1)
src/components/floweditor/FlowEditor.test.tsx (1)
54-60: LGTM! Axios mock is appropriate for testing.The axios.get mock implementation correctly provides a consistent response structure that aligns with the expected API response format, supporting the new asynchronous API calls in the FlowEditor components.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3380 +/- ##
==========================================
- Coverage 82.99% 82.63% -0.37%
==========================================
Files 340 340
Lines 11241 11328 +87
Branches 2379 2396 +17
==========================================
+ Hits 9330 9361 +31
- Misses 1202 1249 +47
- Partials 709 718 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Glific
|
||||||||||||||||||||||||||||
| Project |
Glific
|
| Branch Review |
enhancements/save-revision-before-publish
|
| Run status |
|
| Run duration | 28m 13s |
| Commit |
|
| Committer | Akansha Sakhre |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
182
|
| View all changes introduced in this branch ↗︎ | |
Screen.Recording.2025-07-07.at.3.44.37.PM.movWhen i was trying to publish a flow, got logged out. Happening for newly created and existing flow. 
|
I am able to reproduce when if the floweditor is idle for sometime, and then i try to publish this happens. The authentication error happens when we try to to do GET revisions just before publish. We are not sending correct token for this req, since rest of the graphql queries are working fine. I think when we renew the token, the new one is not send with these revision apis. |
|
Should we cleanup the indexedDB when we are out of the floweditor? |
But if someone's changes are not saved and they didn't publish, when they come back the next day and publish again the changes are lost again. But we should decide when to clean it up |
…ing; update flow definition timestamp comparison logic
|
@madclaws the logout issue is resolved |
kurund
left a comment
kurund
left a comment
There was a problem hiding this comment.
I am wondering if we should clear the index DB once the flow is successfully published.
…ndexedDB mock for tests
…ing; update flow definition timestamp comparison logic
… publish completion
…m:glific/glific-frontend into enhancements/save-revision-before-publish
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/components/floweditor/FlowEditor.helper.tsx (3)
91-114: Critical: Missing save function still unaddressedWhile the fetch function implementation looks good with proper error handling and authentication, the IndexedDB implementation is still incomplete without a save function. The
getFlowDefinitionwill always return null unless data is saved elsewhere.You still need to implement a
saveFlowDefinitionfunction to make the IndexedDB functionality complete:export const saveFlowDefinition = async ( uuid: string, definition: any, timeStamp: string ): Promise<boolean> => { const db = dbInstance || (await initDB()); if (!db) { setLogs('Database not initialized. Call initDB() first.', 'error'); return false; } return new Promise((resolve, reject) => { const transaction = db.transaction([STORE_NAME], 'readwrite'); const store = transaction.objectStore(STORE_NAME); const request = store.put({ uuid, definition, timeStamp }); request.onsuccess = () => resolve(true); request.onerror = () => reject(new Error('Failed to save flow definition')); }); };
104-106: Inefficient sorting implementation remainsThe reduce approach for finding the latest revision is still inefficient and could be error-prone. As mentioned in past comments, sorting would be more reliable.
Consider this more efficient approach:
- if (data.results.length > 0) { - latestRevision = data.results.reduce((latest: any, current: any) => { - return new Date(latest.created_on) > new Date(current.created_on) ? latest : current; - }); - } + if (data.results.length > 0) { + latestRevision = data.results.sort((a: any, b: any) => + new Date(b.created_on).getTime() - new Date(a.created_on).getTime() + )[0]; + }
116-117: Type safety concerns persistThe function still uses
anytype for the definition parameter, which reduces type safety as noted in past reviews.Consider improving type safety:
-export const postLatestRevision = async (uuid: string, definition: any) => { +interface FlowDefinition { + [key: string]: any; // or more specific structure if known +} + +export const postLatestRevision = async (uuid: string, definition: FlowDefinition): Promise<boolean> => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/floweditor/FlowEditor.helper.tsx(2 hunks)src/components/floweditor/FlowEditor.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/floweditor/FlowEditor.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit Configuration File
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
src/components/floweditor/FlowEditor.helper.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CI
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
🔇 Additional comments (6)
src/components/floweditor/FlowEditor.helper.tsx (6)
7-8: LGTM: Authentication service imports addedThe addition of
getAuthSessionimport aligns with the authentication token handling mentioned in the PR objectives and addresses the authentication issues that were previously reported.
12-43: IndexedDB initialization looks solidThe database initialization follows IndexedDB best practices with proper error handling, singleton pattern, and schema management. The upgrade handling correctly creates the object store and index.
45-66: LGTM: Proper error handling in getFlowDefinitionThe function correctly initializes the database if needed and handles both success and error cases appropriately.
68-89: Good implementation of delete functionalityThe delete function properly handles database initialization and includes appropriate error handling with descriptive error messages.
94-99: Authentication token handling improvedGood to see the authentication token is now properly retrieved and included in the headers. This should address the logout issues mentioned in the PR comments.
116-138: Good error handling in postLatestRevisionThe function properly handles authentication tokens and includes comprehensive error handling. The use of
fetchinstead ofaxiosis consistent with the fetchLatestRevision function.
| }; | ||
|
|
||
| export const loadfiles = (startFlowEditor: any) => { | ||
| export const loadfiles = async (startFlowEditor: any) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Function signature change needs verification
The loadfiles function has been changed from synchronous to asynchronous, but the internal implementation remains synchronous. This could break existing callers.
Verify that all callers of loadfiles have been updated to handle the async nature:
🏁 Script executed:
#!/bin/bash
# Search for all usages of loadfiles function
rg -A 3 -B 3 "loadfiles\(" --type ts --type tsxLength of output: 77
🏁 Script executed:
#!/bin/bash
# Search for all usages of loadfiles across TS/TSX files
rg -A 3 -B 3 "loadfiles\(" -g "*.ts" -g "*.tsx" || echo "No occurrences found."Length of output: 535
Ensure callers handle loadfiles’ async return
The loadfiles function now returns a Promise, but its caller in FlowEditor.tsx still treats it as synchronous:
• File: src/components/floweditor/FlowEditor.tsx
Around the useEffect:
useEffect(() => {
if (flowId) {
- const files = loadfiles(() => {
+ (async () => {
+ const files = await loadfiles(() => {
getFreeFlow({ variables: { id: flowId } });
});
+ // …use files here…
+ })();
}
}, [flowId]);Please update this (and any other) call site to either await the async function or chain with .then(...), and adjust the useEffect body accordingly so you’re working with the resolved value instead of a Promise.
🤖 Prompt for AI Agents
In src/components/floweditor/FlowEditor.tsx near the useEffect hook, the call to
the async function loadfiles from FlowEditor.helper.tsx is currently treated
synchronously. Update the useEffect callback to either use an async function
with await when calling loadfiles or use loadfiles(...).then(...) to handle the
Promise resolution properly. This ensures the code works with the resolved value
instead of the Promise itself.
…m:glific/glific-frontend into enhancements/save-revision-before-publish
…cements/save-revision-before-publish
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/floweditor/FlowEditor.helper.tsx(1 hunks)src/components/floweditor/FlowEditor.test.tsx(6 hunks)src/components/floweditor/FlowEditor.tsx(4 hunks)src/mocks/Flow.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mocks/Flow.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
src/components/floweditor/FlowEditor.helper.tsxsrc/components/floweditor/FlowEditor.test.tsxsrc/components/floweditor/FlowEditor.tsx
🧬 Code graph analysis (3)
src/components/floweditor/FlowEditor.helper.tsx (2)
src/config/index.ts (1)
FLOW_EDITOR_API(30-30)src/services/AuthService.tsx (1)
getAuthSession(26-51)
src/components/floweditor/FlowEditor.test.tsx (1)
src/mocks/Flow.tsx (1)
publishFlowWithSuccess(440-455)
src/components/floweditor/FlowEditor.tsx (1)
src/components/floweditor/FlowEditor.helper.tsx (4)
deleteFlowDefinition(68-89)fetchLatestRevision(91-114)getFlowDefinition(45-66)postLatestRevision(116-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: CI
- GitHub Check: glific (1.18.3-otp-27, 27.3.3)
| const response = await fetch(`${glificBase}revisions/${uuid}?version=13.2`, { | ||
| headers: { | ||
| authorization: token, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Include Bearer prefix on Authorization header
The flow-editor API expects standard bearer auth. Here we send the raw token (authorization: token), which the backend treats as missing credentials, returning 401. This regresses publishing after idle periods—the exact bug reported in PR comments. Prefix the header with Bearer (and bail early if token is falsy) so the request reuses our auth session correctly.
- const response = await fetch(`${glificBase}revisions/${uuid}?version=13.2`, {
- headers: {
- authorization: token,
- },
- });
+ if (!token) {
+ throw new Error('Missing access token for revision fetch');
+ }
+
+ const response = await fetch(`${glificBase}revisions/${uuid}?version=13.2`, {
+ headers: {
+ authorization: `Bearer ${token}`,
+ },
+ });📝 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.
| const response = await fetch(`${glificBase}revisions/${uuid}?version=13.2`, { | |
| headers: { | |
| authorization: token, | |
| }, | |
| }); | |
| if (!token) { | |
| throw new Error('Missing access token for revision fetch'); | |
| } | |
| const response = await fetch(`${glificBase}revisions/${uuid}?version=13.2`, { | |
| headers: { | |
| authorization: `Bearer ${token}`, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In src/components/floweditor/FlowEditor.helper.tsx around lines 96 to 100, the
fetch call sends the raw token in the authorization header which the API expects
as a Bearer token; update the code to early-return or throw if token is falsy,
and set the header to "Authorization": `Bearer ${token}` (i.e., prefix the token
with "Bearer " and use the proper header key casing) so the backend recognizes
the credentials and avoids 401s after idle periods.
| const checkLatestRevision = async () => { | ||
| let revisionSaved = false; | ||
| if (uuid) { | ||
| const latestRevision = await fetchLatestRevision(uuid); | ||
| const flowDefinition = await getFlowDefinition(uuid); | ||
|
|
||
| if (latestRevision && flowDefinition) { | ||
| const latestRevisionTime = dayjs(latestRevision.created_on); | ||
| const flowDefinitionTime = dayjs(flowDefinition.timestamp); | ||
|
|
||
| if (flowDefinitionTime.isAfter(latestRevisionTime)) { | ||
| const timeDifferenceSeconds = flowDefinitionTime.diff(latestRevisionTime, 'seconds'); | ||
| revisionSaved = | ||
| timeDifferenceSeconds > 300 ? await postLatestRevision(uuid, flowDefinition.definition) : true; | ||
| } else { | ||
| revisionSaved = true; | ||
| } | ||
| } else if (!flowDefinition) { | ||
| setLogs(`Local Flow definition not found ${uuid}`, 'info'); | ||
|
|
||
| // If flowDefinition is not found, we assume the revision is saved | ||
| revisionSaved = true; | ||
| } | ||
| } | ||
| return revisionSaved; | ||
| }; |
There was a problem hiding this comment.
Surface failure when revisions can't be synced
When fetchLatestRevision/getFlowDefinition returns null we bail with false, but nothing informs the user why publish didn't proceed; dialog just spins. Add error notification + reset loading.
🤖 Prompt for AI Agents
In src/components/floweditor/FlowEditor.tsx around lines 288-313, when
fetchLatestRevision or getFlowDefinition returns null the function currently
proceeds without surfacing an error and the publish dialog can continue
spinning; update the null/failed branches to log an error (use setLogs with an
error message including the uuid and which call failed) and reset the
publishing/loading state (e.g., call setIsPublishing(false) or the component's
equivalent) before returning false so the UI is notified and the dialog stops.
|
putting this on hold because we are already working on floweditor auth here which might be a potential issue for this too |
target issue is #3344
Follow these steps to test it locally- https://docs.google.com/document/d/1g28JMBjrUhtdXrY8AGZBUJMHaasE5aD0gNDgt_aT41Q/edit?tab=t.0
Summary by CodeRabbit
New Features
Added local storage for flow definitions using IndexedDB, enabling offline access and improved data persistence.
Introduced automatic revision checks to ensure the latest flow definition is saved before publishing.
Enhanced flow publishing process with a consistency check to keep backend data up to date.
Improvements
Improved reliability of flow publishing by synchronizing recent changes within a 5-minute window.
Summary by CodeRabbit
New Features
Bug Fixes
Tests