remove deprecated Apollo onCompleted/onError callbacks#3845
Open
akanshaaa19 wants to merge 17 commits intomasterfrom
Open
remove deprecated Apollo onCompleted/onError callbacks#3845akanshaaa19 wants to merge 17 commits intomasterfrom
akanshaaa19 wants to merge 17 commits intomasterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces many GraphQL mutation lifecycle callbacks (onCompleted/onError) with explicit async/await handlers and try/catch error handling, moves some UI state to be derived from query results, adds/renames async handlers for upload/import/export/pin/delete flows, introduces additional data queries in several components (languages, quality rating), adjusts test MockedProvider usage, and adds three i18n keys. No public/exported function signatures were intentionally changed except internalizing mutation handlers and making some local handlers async. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
50 tasks
|
🚀 Deployed on https://deploy-preview-3845--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: 3
🧹 Nitpick comments (1)
src/containers/Flow/FlowList/FlowList.tsx (1)
118-128: Parameter mismatch with ImportButton.
handleImportaccepts one parameter(result: string), butImportButtonpasses two:(fileReader.result, media). While the extramediaparameter is ignored without error, other handlers in the codebase (e.g.,HSMList.tsx:354) accept both parameters. For consistency with theImportButtonAPI, consider updatinghandleImportto accept both parameters:const handleImport = async (result: string, media: any) => { try { const { data } = await importFlow({ variables: { flow: result } }); const { status } = data.importFlow; setImportStatus(status); } catch { setNotification('An error occured while importing the flow', 'warning'); } finally { setImporting(false); } };Also applies to: 233-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Flow/FlowList/FlowList.tsx` around lines 118 - 128, The handleImport function currently declared as handleImport(result: string) should be updated to accept both parameters provided by ImportButton (e.g., handleImport(result: string, media: any)) to match the ImportButton API; change the signature in FlowList.tsx for handleImport and any other duplicate handler definitions in this file to (result: string, media: any) and keep the same body (you can ignore or use media as needed) so the call sites that pass (fileReader.result, media) align with HSMList.tsx style and avoid parameter mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Flow/FlowList/FlowList.tsx`:
- Around line 130-139: The handlePin function lacks error handling around the
async updatePinned call; wrap the updatePinned invocation in a try/catch inside
handlePin, only call setRefreshList and setNotification on success, and in the
catch log the error (or use existing logger) and set a failure notification
(e.g., 'Failed to pin/unpin flow') so unhandled promise rejections are avoided;
update references: handlePin, updatePinned, setRefreshList, setNotification.
In `@src/containers/Form/FormLayout.tsx`:
- Around line 288-294: The computed languageId can throw if fetchedItem exists
but fetchedItem.language is null; update the ternary that sets languageId to
guard fetchedItem.language (e.g., check fetchedItem.language &&
fetchedItem.language.id) before accessing .id so it falls back to null/empty as
intended when language is missing; locate the languageId assignment in
FormLayout.tsx (symbols: languageId, fetchedItem, languageSupport) and apply a
null-safe access to fetchedItem.language (or use optional chaining) to prevent
runtime errors.
In `@src/containers/Trigger/Trigger.tsx`:
- Around line 251-266: handleFlowChange currently awaits validateTriggerFlow
without error handling; wrap the async call in a try/catch inside
handleFlowChange (around the validateTriggerFlow invocation) to catch
network/mutation errors, call setTriggerFlowWarning with a generic or
error.message value on failure, and optionally log the error (e.g.,
console.error or existing logger). Ensure existing logic that reads
result.data.validateTrigger remains in the try block and that
setTriggerFlowWarning(undefined) stays at the start of handleFlowChange.
---
Nitpick comments:
In `@src/containers/Flow/FlowList/FlowList.tsx`:
- Around line 118-128: The handleImport function currently declared as
handleImport(result: string) should be updated to accept both parameters
provided by ImportButton (e.g., handleImport(result: string, media: any)) to
match the ImportButton API; change the signature in FlowList.tsx for
handleImport and any other duplicate handler definitions in this file to
(result: string, media: any) and keep the same body (you can ignore or use media
as needed) so the call sites that pass (fileReader.result, media) align with
HSMList.tsx style and avoid parameter mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35dde555-b94c-4bf6-85b4-167d3d858c2f
📒 Files selected for processing (6)
src/components/UI/Layout/Navigation/SideMenus/SideMenus.tsxsrc/containers/Flow/FlowList/FlowList.tsxsrc/containers/Form/FormLayout.tsxsrc/containers/HSM/HSM.tsxsrc/containers/List/List.tsxsrc/containers/Trigger/Trigger.tsx
Glific
|
||||||||||||||||||||||||||||||||||||||||
| Project |
Glific
|
| Branch Review |
apollo_v4/batch_1
|
| Run status |
|
| Run duration | 13m 28s |
| Commit |
|
| Committer | Akansha Sakhre |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
97
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/filesearch/Filesearch.spec.ts • 1 failed test
| Test | Artifacts | |
|---|---|---|
| File search > should create a new assistant with file upload |
Test Replay
Screenshots
|
|
cypress/e2e/chat/ChatCollection.spec.ts • 1 flaky test
| Test | Artifacts | |
|---|---|---|
| ChatCollection > should send the emoji to collection |
Test Replay
Screenshots
|
|
…r management in HSMList and Providers components
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/HSM/HSMList/HSMList.tsx`:
- Line 208: The notification strings in HSMList are hard-coded; update the calls
to setNotification to use i18next translation keys (e.g., import/use the t
function and call setNotification(t('hsm.upload.invalidCsv'), 'warning') and
similarly for the other occurrence), add the new keys to your translation
resource files (or default en.json), and run the translation extractor (yarn
extract-translations) so Lokalise picks them up; refer to the setNotification
usage and the HSMList component to locate both places to change.
- Around line 192-197: The code checks the wrong property for GraphQL errors —
replace the check for data.errors with a check for data.syncHsmTemplate.errors
(or destructure syncHsmTemplate from the response) in the async block that calls
syncHsmTemplates(); update the conditional that calls setNotification so it
reads something like: if (data && data.syncHsmTemplate &&
data.syncHsmTemplate.errors) { setNotification(... 'warning') } else {
setNotification(... 'success') } to ensure resolver errors trigger the warning
notification.
- Around line 212-216: The current handling after calling bulkApplyTemplates({
variables: { data: result } }) assumes any non-null payload is a success;
instead inspect data.bulkApplyTemplates.errors and only treat as success when
errors is empty. Update the logic in HSMList.tsx (the bulkApplyTemplates
response handling) to: if data?.bulkApplyTemplates &&
data.bulkApplyTemplates.errors?.length === 0 then call
exportCsvFile(data.bulkApplyTemplates.csv_rows, 'result') and
setNotification(t('Templates applied successfully. Please check the csv file for
the results')); otherwise handle the failure path by exporting the returned
csv_rows (if present) and calling setNotification with an error message (e.g.
include the first error message or count of errors via t) and do not show the
success toast. Ensure you reference bulkApplyTemplates,
data.bulkApplyTemplates.errors, exportCsvFile, and setNotification/t in the
change.
In `@src/containers/SettingList/Providers/Providers.tsx`:
- Around line 200-204: The code calls setCredential(credentialData) inside the
providers iteration even though credentialData is computed from
credential?.credential?.credential and does not depend on the current provider;
move the setCredential call out of the providers.forEach loop (compute
credentialData as const credentialData = credential?.credential?.credential and
then, after the loop ends, call setCredential(credentialData) only once) so you
avoid redundant state updates during the providers iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46f1e1f2-337b-4ab4-b35a-ab7b97f4f131
📒 Files selected for processing (2)
src/containers/HSM/HSMList/HSMList.tsxsrc/containers/SettingList/Providers/Providers.tsx
…gTier from GET_QUALITY_RATING query
…and Trigger components; update language ID fallback in FormLayout; improve Providers component logic; add new translations for CSV validation messages
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/containers/SettingList/Organization/Organisation.test.tsx (1)
61-68: Consider extracting a render helper to reduce duplication.Multiple tests render similar provider-wrapped structures. A local helper would consolidate this pattern.
const renderOrganization = (mocks = ORGANIZATION_MOCKS) => render( <MockedProvider mocks={mocks}> <Router> <Organization /> </Router> </MockedProvider> );As per coding guidelines: "When repeatedly rendering the same provider-wrapped component in tests, extract a local
renderXxxhelper that accepts optionalmocksandpropsoverrides."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/SettingList/Organization/Organisation.test.tsx` around lines 61 - 68, Extract a local test helper named renderOrganization that wraps Organization with MockedProvider and Router to avoid repeating the render(...) pattern; implement it to accept an optional mocks parameter defaulting to ORGANIZATION_MOCKS (and optional props if needed), use MockedProvider, Router and render inside the helper, then replace direct calls to render(...) in tests (the ones importing render, MockedProvider, Router and rendering Organization) with calls to renderOrganization(mocks) to consolidate duplication.src/containers/SettingList/Organization/Organization.tsx (1)
201-201: Prefer a nullish check forskipinstead of a broad falsy check.Line 201 currently uses
!qualityRatingTier, which also hides the field for valid falsy values like0or''. An explicitnull/undefinedcheck is safer.Suggested diff
- skip: !qualityRatingTier, + skip: qualityRatingTier === null || qualityRatingTier === undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/SettingList/Organization/Organization.tsx` at line 201, The skip flag currently uses a broad falsy check (!qualityRatingTier) which hides valid falsy values; update the skip assignment to only skip when qualityRatingTier is null or undefined (e.g., use qualityRatingTier == null or qualityRatingTier === undefined) in the Organization component where skip: !qualityRatingTier is set so 0 or '' remain valid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Form/FormLayout.tsx`:
- Around line 259-271: The query is being skipped when item routes use a
credential shortcode because skip is tied only to itemId; update the useQuery
call for getItemQuery so its skip condition accounts for the credential
shortcode path (i.e., consider the computed variables or listItem ===
'credential' along with itemId). Locate the variables construction (variables,
listItem, itemId, params.type) and change the skip from skip: !itemId to a
condition that is false when variables contains the shortcode (for example skip
only when variables is falsy or when neither itemId nor credential shortcode is
present) so the credential shortcode route triggers the query and initializes
itemData/refetch correctly.
In `@src/containers/HSM/HSMList/HSMList.tsx`:
- Around line 237-248: The try/catch block in the import flow uses direct
destructuring of data.importTemplates which can throw if data is null/undefined
— update the handling in the function using importTemplatesMutation to use
optional chaining when accessing data.importTemplates (e.g., const errors =
data?.importTemplates?.errors) and keep the existing error notification path;
also mirror handleBulkApply by adding a success notification via
setNotification(t('Templates imported successfully'), 'success') when there are
no errors; ensure setImporting(false) remains in the finally block.
In `@src/containers/Trigger/Trigger.tsx`:
- Line 267: Replace the hardcoded error string passed to setTriggerFlowWarning
with an i18next translation key; locate the call in Trigger component
(setTriggerFlowWarning('Failed to validate flow. Please try again.')) and change
it to use the t(...) helper (e.g., t('trigger.validationFailedFallback')) so the
message is extracted, add the corresponding translation key to your source (or
en.json) and run yarn extract-translations to push to Lokalise.
---
Nitpick comments:
In `@src/containers/SettingList/Organization/Organisation.test.tsx`:
- Around line 61-68: Extract a local test helper named renderOrganization that
wraps Organization with MockedProvider and Router to avoid repeating the
render(...) pattern; implement it to accept an optional mocks parameter
defaulting to ORGANIZATION_MOCKS (and optional props if needed), use
MockedProvider, Router and render inside the helper, then replace direct calls
to render(...) in tests (the ones importing render, MockedProvider, Router and
rendering Organization) with calls to renderOrganization(mocks) to consolidate
duplication.
In `@src/containers/SettingList/Organization/Organization.tsx`:
- Line 201: The skip flag currently uses a broad falsy check
(!qualityRatingTier) which hides valid falsy values; update the skip assignment
to only skip when qualityRatingTier is null or undefined (e.g., use
qualityRatingTier == null or qualityRatingTier === undefined) in the
Organization component where skip: !qualityRatingTier is set so 0 or '' remain
valid values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b4def55-7125-4eb5-a44c-eb6e9bd3fea1
📒 Files selected for processing (8)
src/containers/Flow/FlowList/FlowList.tsxsrc/containers/Form/FormLayout.tsxsrc/containers/HSM/HSMList/HSMList.tsxsrc/containers/SettingList/Organization/Organisation.test.tsxsrc/containers/SettingList/Organization/Organization.tsxsrc/containers/SettingList/Providers/Providers.tsxsrc/containers/Trigger/Trigger.tsxsrc/i18n/en/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/containers/SettingList/Providers/Providers.tsx
- src/containers/Flow/FlowList/FlowList.tsx
…essages in HSMList and Trigger components; add missing translation for flow validation error
…flow parameter in Trigger component
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3845 +/- ##
==========================================
- Coverage 82.11% 82.06% -0.06%
==========================================
Files 311 311
Lines 13126 13122 -4
Branches 3039 3040 +1
==========================================
- Hits 10778 10768 -10
- Misses 1397 1405 +8
+ Partials 951 949 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/containers/HSM/HSMList/HSMList.tsx (1)
237-245:⚠️ Potential issue | 🟡 MinorSuccessful imports still complete silently.
The error path toasts, but the happy path just drops the loading screen. Please restore a success notification so imports behave like bulk apply and sync.
💡 Suggested fix
try { const { data } = await importTemplatesMutation({ variables: { data: result } }); - const errors = data?.importTemplates?.errors; - if (errors && errors.length > 0) { + const response = data?.importTemplates; + const errors = response?.errors; + if (errors?.length) { setNotification(t('Error importing templates'), 'warning'); + } else if (response) { + setNotification(t('Templates imported successfully'), 'success'); } } catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/HSM/HSMList/HSMList.tsx` around lines 237 - 245, The importTemplatesMutation success branch currently does not notify the user; update the try block in HSMList (where importTemplatesMutation is awaited) to call setNotification with a success message (e.g. t('Templates imported successfully')) when data?.importTemplates?.errors is empty or undefined, keeping the existing warning call when errors exist and preserving the catch warning behavior; ensure you use the same setNotification and i18n t() pattern as elsewhere so the UI shows a success toast on a successful import.src/containers/Form/FormLayout.tsx (1)
343-355:⚠️ Potential issue | 🟠 MajorCredential shortcode routes are still skipped here.
variablesswitches to{ shortcode: params.type }for credentials, butskipis still tied toitemId. On shortcode-only routes the query never runs, sofetchedItemstays null and the form falls through the create path.💡 Suggested fix
- let variables: any = itemId ? { [idType]: itemId } : false; + let variables: any = itemId ? { [idType]: itemId } : undefined; if (listItem === 'credential') { - variables = params.type ? { shortcode: params.type } : false; + variables = params.type ? { shortcode: params.type } : undefined; } + const shouldFetchItem = Boolean(variables); const { loading, error, data: itemData, refetch } = useQuery(getItemQuery, { variables, - skip: !itemId, + skip: !shouldFetchItem, fetchPolicy: getQueryFetchPolicy, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Form/FormLayout.tsx` around lines 343 - 355, The query is being skipped on shortcode-only credential routes because skip is tied only to itemId; update the logic that builds variables and the skip condition so getItemQuery runs when either itemId exists or when listItem === 'credential' and params.type (shortcode) is present. Specifically, ensure the variables assignment (variables = itemId ? { [idType]: itemId } : false; and the credential branch variables = params.type ? { shortcode: params.type } : false;) is preserved and change the useQuery option skip from skip: !itemId to skip: !(itemId || (listItem === 'credential' && params?.type)), so the query executes for shortcode routes and fetchedItem is populated.
🧹 Nitpick comments (1)
src/containers/HSM/HSMList/HSMList.tsx (1)
372-384: Restrict the picker to CSVs too.
ImportButtonalready supportsfileType, so passing.csvhere prevents unsupported files beforeFileReaderruns and avoids the loading flash for obviously invalid selections.💡 Suggested refactor
- <ImportButton title={t('Bulk apply')} onImport={() => setImporting(true)} afterImport={handleBulkApply} /> + <ImportButton + title={t('Bulk apply')} + fileType=".csv" + onImport={() => setImporting(true)} + afterImport={handleBulkApply} + /> ... <ImportButton title={t('Import templates')} + fileType=".csv" onImport={() => setImporting(true)} afterImport={handleImportTemplates} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/HSM/HSMList/HSMList.tsx` around lines 372 - 384, The ImportButton usage for template import should restrict selectable files to CSVs; update the ImportButton component(s) (e.g., the instance with title 'Import templates' in HSMList where setImporting and handleImportTemplates are used) to pass fileType=".csv" (and also add fileType=".csv" to any other ImportButton instances used for importing templates such as the 'Bulk apply' ImportButton if applicable) so the file picker only allows .csv files and prevents unsupported files before FileReader runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Form/FormLayout.tsx`:
- Around line 223-243: The new user-facing strings inside the FormLayout
success/error branch should be localized with i18next: replace hard-coded
messages such as "Failed to compile code. Please check again", the success
message constructed from capitalListItemName ("${capitalListItemName} edited
successfully!"), the copyNotification value usage, and "Your changes have been
autosaved" by wrapping them in t(...) and use interpolation for
capitalListItemName (e.g., t('...',{ name: capitalListItemName })); update
usages around updatedItem.isValid/customError.setErrors, the success branch that
calls setLink/additionalQuery/setFormSubmitted/setNotification, and any similar
strings at the other occurrences mentioned (lines ~264-279 and ~618) so all
user-facing messages are extracted for translation.
- Around line 348-376: The create form is mounting before the USER_LANGUAGES
query (organization) is resolved which causes languageId to start as '' then
flip and can wipe Formik state; update the logic around the organization
useQuery and form initialization so the form (or any Formik reinitialization
that relies on languageId) is gated until organization.data is loaded or
errored: use the existing USER_LANGUAGES query result (the organization const)
to compute a stable isLoadedOrg flag and only initialize the form/state (and
call setStates) when isLoadedOrg is true, surface organization.error to the
UI/error handling path, and ensure languageId returns null (or undefined) rather
than '' before org resolves so Number('') is not sent as 0; adjust the
languageId computation and the useEffect that calls setStates (and any
isLoadedData logic) to depend on organization.data/loading/error and fetchedItem
appropriately.
---
Duplicate comments:
In `@src/containers/Form/FormLayout.tsx`:
- Around line 343-355: The query is being skipped on shortcode-only credential
routes because skip is tied only to itemId; update the logic that builds
variables and the skip condition so getItemQuery runs when either itemId exists
or when listItem === 'credential' and params.type (shortcode) is present.
Specifically, ensure the variables assignment (variables = itemId ? { [idType]:
itemId } : false; and the credential branch variables = params.type ? {
shortcode: params.type } : false;) is preserved and change the useQuery option
skip from skip: !itemId to skip: !(itemId || (listItem === 'credential' &&
params?.type)), so the query executes for shortcode routes and fetchedItem is
populated.
In `@src/containers/HSM/HSMList/HSMList.tsx`:
- Around line 237-245: The importTemplatesMutation success branch currently does
not notify the user; update the try block in HSMList (where
importTemplatesMutation is awaited) to call setNotification with a success
message (e.g. t('Templates imported successfully')) when
data?.importTemplates?.errors is empty or undefined, keeping the existing
warning call when errors exist and preserving the catch warning behavior; ensure
you use the same setNotification and i18n t() pattern as elsewhere so the UI
shows a success toast on a successful import.
---
Nitpick comments:
In `@src/containers/HSM/HSMList/HSMList.tsx`:
- Around line 372-384: The ImportButton usage for template import should
restrict selectable files to CSVs; update the ImportButton component(s) (e.g.,
the instance with title 'Import templates' in HSMList where setImporting and
handleImportTemplates are used) to pass fileType=".csv" (and also add
fileType=".csv" to any other ImportButton instances used for importing templates
such as the 'Bulk apply' ImportButton if applicable) so the file picker only
allows .csv files and prevents unsupported files before FileReader runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 04f01fed-d61b-4ac1-b683-5d8a5696c020
📒 Files selected for processing (4)
src/containers/Form/FormLayout.tsxsrc/containers/HSM/HSMList/HSMList.tsxsrc/containers/Trigger/Trigger.tsxsrc/i18n/en/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/en/en.json
- src/containers/Trigger/Trigger.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/containers/Form/FormLayout.tsx (3)
223-242:⚠️ Potential issue | 🟡 MinorLocalize the new toast and validation copy.
The literals at Lines 225, 236-242, 266-278, and 623 bypass
t(...), so they will not be extracted or translated. Please wrap the local strings int(...)and interpolatecapitalListItemNameinstead of building English messages inline.As per coding guidelines, "Use i18next for internationalization with string extraction to Lokalise via
yarn extract-translations."Also applies to: 264-279, 623-623
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Form/FormLayout.tsx` around lines 223 - 242, The hard-coded user-facing strings in the success/validation paths need to be wrapped with the i18next translator and use interpolation instead of concatenation; update the branches around updatedItem handling (references: customError.setErrors, setLink, additionalQuery, setFormSubmitted, setNotification, copyNotification, capitalListItemName) to call t(...) for messages and pass capitalListItemName as an interpolation variable, and replace the static codeErrors/code message with t(...) as well so all toast and validation text is extractable for localization.
343-360:⚠️ Potential issue | 🟠 MajorShortcode-based credential forms are still skipped.
variablesalready supports{ shortcode: params.type }, but Line 359 still ties the query toitemId. On shortcode-only routes this query never runs, so the form never hydrates from the existing credential.💡 Suggested fix
- let variables: any = itemId ? { [idType]: itemId } : false; + let variables: any = itemId ? { [idType]: itemId } : undefined; if (listItem === 'credential') { - variables = params.type ? { shortcode: params.type } : false; + variables = params.type ? { shortcode: params.type } : undefined; } + const shouldFetchItem = Boolean(variables); const { loading, error, data: itemData, refetch, } = useQuery(getItemQuery, { variables, - skip: !itemId, + skip: !shouldFetchItem, fetchPolicy: getQueryFetchPolicy, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Form/FormLayout.tsx` around lines 343 - 360, The query for getItemQuery is being skipped when itemId is falsy even though credential forms can be identified by params.type (shortcode); update the variables and skip logic so the useQuery runs for shortcode-based credentials: keep the existing variables assignment (variables = itemId ? { [idType]: itemId } : false; and variables = params.type ? { shortcode: params.type } : false when listItem === 'credential') and change the useQuery option skip from skip: !itemId to skip: !itemId && !(listItem === 'credential' && params.type), ensuring getItemQuery executes when params.type is present even if itemId is absent.
348-381:⚠️ Potential issue | 🟠 MajorHold create-mode rendering until
USER_LANGUAGESresolves.On create routes Line 381 initializes
languageIdto''and later replaces it with the org default. WithenableReinitializeon Line 389, that can wipe in-progress edits, and the empty state can still be serialized as0on submit.organization.erroris also ignored right now.💡 One way to stabilize initialization
const organization = useQuery(USER_LANGUAGES, { skip: !languageSupport, }); + const isOrganizationLoading = languageSupport && !itemId && organization.loading; const languageId = fetchedItem ? languageSupport ? (fetchedItem.language?.id ?? '') : null : !itemId && organization.data ? organization.data.currentUser.user.organization.defaultLanguage.id - : ''; + : undefined; ... - if (loading) return <Loading />; + if (loading || isOrganizationLoading) return <Loading />; + if (organization.error) { + setErrorMessage(organization.error); + return null; + }I’d also avoid serializing
languageIduntil it is a real value; otherwise''becomes0andundefinedbecomesNaN.Also applies to: 383-389, 451-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/containers/Form/FormLayout.tsx` around lines 348 - 381, The create-mode can initialize languageId to an empty string before USER_LANGUAGES resolves which, combined with enableReinitialize, can wipe edits and serialize to invalid values; change the logic around USER_LANGUAGES/organization and languageId so the form waits for organization to finish (check organization.loading and organization.error) before providing a default language, only call setStates/fetch initialization when fetchedItem or organization is settled, and keep languageId undefined (not '') until a real id is available; update uses of languageId, the useEffect that calls setStates, and any enableReinitialize gating to only run reinitialization when organization.data or fetchedItem is a real value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/containers/Form/FormLayout.tsx`:
- Around line 297-299: The submit path currently uses isLoadedData (set to
Boolean(fetchedItem)) to decide update vs create, allowing a missing edit record
to fall through to createItem; change this by introducing a distinct "query
resolved for edit" guard (e.g., isEditResolved or hasFetchedItem) and use it in
performTask to block submit when itemId is present but fetchedItem is
null/undefined: if itemId exists and the edit-query has resolved without a
fetchedItem, return or surface a not-found error instead of calling createItem;
ensure the same clear guard replaces uses of isLoadedData in the edit-flow
(including the logic around fetchedItem at lines referenced) so updateItem is
only attempted when fetchedItem exists and createItem only when itemId is
absent.
---
Duplicate comments:
In `@src/containers/Form/FormLayout.tsx`:
- Around line 223-242: The hard-coded user-facing strings in the
success/validation paths need to be wrapped with the i18next translator and use
interpolation instead of concatenation; update the branches around updatedItem
handling (references: customError.setErrors, setLink, additionalQuery,
setFormSubmitted, setNotification, copyNotification, capitalListItemName) to
call t(...) for messages and pass capitalListItemName as an interpolation
variable, and replace the static codeErrors/code message with t(...) as well so
all toast and validation text is extractable for localization.
- Around line 343-360: The query for getItemQuery is being skipped when itemId
is falsy even though credential forms can be identified by params.type
(shortcode); update the variables and skip logic so the useQuery runs for
shortcode-based credentials: keep the existing variables assignment (variables =
itemId ? { [idType]: itemId } : false; and variables = params.type ? {
shortcode: params.type } : false when listItem === 'credential') and change the
useQuery option skip from skip: !itemId to skip: !itemId && !(listItem ===
'credential' && params.type), ensuring getItemQuery executes when params.type is
present even if itemId is absent.
- Around line 348-381: The create-mode can initialize languageId to an empty
string before USER_LANGUAGES resolves which, combined with enableReinitialize,
can wipe edits and serialize to invalid values; change the logic around
USER_LANGUAGES/organization and languageId so the form waits for organization to
finish (check organization.loading and organization.error) before providing a
default language, only call setStates/fetch initialization when fetchedItem or
organization is settled, and keep languageId undefined (not '') until a real id
is available; update uses of languageId, the useEffect that calls setStates, and
any enableReinitialize gating to only run reinitialization when
organization.data or fetchedItem is a real value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a6aec6a8-6f49-48d5-8638-8d5307fe043b
📒 Files selected for processing (2)
src/containers/Form/FormLayout.tsxsrc/containers/Trigger/Trigger.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/containers/Trigger/Trigger.tsx
…ist component by removing unused import and import templates logic; fix formatting in SettingList test helper
… ProviderContext import in HSMList
…arations in HSMList
…rt, and pin flow operations; update mocks and rendering logic
…Click parameter; update notification handling in FormLayout
mdshamoon
approved these changes
Apr 1, 2026
Member
mdshamoon
left a comment
mdshamoon
left a comment
There was a problem hiding this comment.
Please check the following comments. Rest looks good
| const handleImport = async (result: string) => { | ||
| try { | ||
| const { data } = await importFlow({ variables: { flow: result } }); | ||
| const { status } = data.importFlow; |
Member
There was a problem hiding this comment.
Are we sure status will always be there?
|
|
||
| const handleUpdateCompleted = (data: any, isSaveClick: boolean) => { | ||
| setShowConfirmationDialog(false); | ||
| let itemUpdatedObject: any = Object.keys(data)[0]; |
Member
There was a problem hiding this comment.
Are we sure in all cases we will get it in the first key. Sometimes we also get __typename
Member
Author
There was a problem hiding this comment.
this is how it was done earlier also see line 353 in the old code
Comment on lines
+229
to
+253
| if (errors) { | ||
| if (customHandler) { | ||
| customHandler(errors); | ||
| } else { | ||
| setErrorMessage(errors[0]); | ||
| } | ||
| } else if (updatedItem && typeof updatedItem.isValid === 'boolean' && !updatedItem.isValid) { | ||
| if (customError) { | ||
| const codeErrors = { code: 'Failed to compile code. Please check again' }; | ||
| customError.setErrors(codeErrors); | ||
| } | ||
| } else { | ||
| if (type === 'copy') setLink(updatedItem[linkParameter]); | ||
| if (additionalQuery) { | ||
| additionalQuery(itemId); | ||
| } | ||
|
|
||
| if (saveOnPageChange || isSaveClick) { | ||
| setFormSubmitted(true); | ||
| let notificationMessage = `${capitalListItemName} edited successfully!`; | ||
| if (type === 'copy') { | ||
| notificationMessage = copyNotification; | ||
| } | ||
| setNotification(notificationMessage); | ||
| } else { |
Member
There was a problem hiding this comment.
We havent touched this code base in a long time. See if these conditions can be optimized.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 1 medium 1 high |
| ErrorProne | 1 high |
🟢 Metrics 1 complexity
Metric Results Complexity 1
TIP This summary will be updated as you push new changes. Give us feedback
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
2 participants
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.
Following Apollo's deprecation of
onCompleted/onErroron query and mutation hooks, this PR refactors the affected files to use the recommended patterns:Files changed
FormLayout.tsxlanguageId/isLoadedDatastate; derived from query data. RemovedonCompletedfromUSER_LANGUAGESqueryList.tsxdeleteItemmutation callbacks to asyncdeleteHandlerSideMenus.tsxnotificationCountfromuseLazyQuerydata instead of stateHSM.tsxuploadMediacallbacks to asynchandleFileUploadTrigger.tsxvalidateTriggerFlowcallback to asynchandleFlowChangeFlowList.tsxflowNamestateHSMList.tsxTest plan
Summary by CodeRabbit
Bug Fixes
Refactor
Enhancement
Localization
Tests