UN-3102 [FIX] confirmation alert alway asking issue fix #1711
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdded internal flags to ConfigureConnectorModal to track schema/session initialization and Rule Engine dirty state; capture baseline form/connector after schema-defaults settle; update hasUnsavedChanges to be mode-specific (API: RuleEngine-only; others: form/connector/or rule engine); wire RuleEngine onDirtyStateChange in API and DATABASE modes. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Modal as ConfigureConnectorModal
participant Schema as Schema Loader
participant Form as Form Component
participant RuleEngine as RuleEngine
participant State as Internal State
User->>Modal: Open modal
Modal->>State: reset init/session/dirty flags
Modal->>Schema: fetch schema (or mark loaded if no endpoint)
Schema-->>Modal: schema loaded
Modal->>Form: apply defaults
Note over Modal,Form: brief delay to let defaults settle
Modal->>State: capture initial formDataConfig & connector id (hasInitializedFormData=true)
alt API or DATABASE (MANUALREVIEW)
Modal->>RuleEngine: render with onDirtyStateChange -> setRuleEngineHasChanges
User->>RuleEngine: edit rules
RuleEngine-->>Modal: onDirtyStateChange(true)
Modal->>State: ruleEngineHasChanges = true
end
rect rgb(220,230,255)
Note over Modal,State: Unsaved changes evaluation
alt API mode
Modal->>State: hasUnsavedChanges = ruleEngineHasChanges
else Other modes
Modal->>State: hasUnsavedChanges = formChanged ∨ connectorChanged ∨ ruleEngineHasChanges
end
end
User->>Modal: Save/Submit
Modal->>Modal: enable/perform save considering ruleEngineHasChanges
User->>Modal: Close modal
Modal->>State: reset init/session/dirty flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to Reviews > Disable Cache setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx (1)
⏰ 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). (1)
🔇 Additional comments (6)
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 |
vishnuszipstack
requested review from
Deepak-Kesavan,
athul-rs,
jagadeeswaran-zipstack and
jaseemjaskp
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx (1)
657-663: Verify that RuleEngine supports onDirtyStateChange in DATABASE mode.Same concern as the API mode usage above: ensure that the
RuleEnginecomponent properly handles theonDirtyStateChangecallback for the DATABASE/MANUALREVIEW mode. The verification script in the previous comment will check both usages.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx (2)
frontend/src/components/agency/ds-settings-card/DsSettingsCard.jsx (3)
connMode(30-30)formDataConfig(33-33)connDetails(32-32)frontend/src/components/settings/users/Users.jsx (1)
open(33-33)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (6)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx (6)
73-75: LGTM! Clear state flag declarations.The three new state flags appropriately track initialization lifecycle and RuleEngine dirty state with clear, self-documenting names.
106-107: LGTM! Consistent schema loading lifecycle tracking.Correctly marks schema as loaded when no endpoint exists, allowing the initialization flow to proceed.
283-294: LGTM! Improved unsaved changes detection.The logic correctly handles API mode separately and normalizes connector ID comparison to avoid false positives from
undefinedvsnullmismatch. The integration ofruleEngineHasChangesfor DATABASE mode ensures rule changes are detected.
442-450: LGTM! Comprehensive cleanup on modal close.Properly resets all initialization flags and baseline values when the modal closes, ensuring a fresh state for the next session.
623-627: Let me revise my comment.Actually, I already generated the comment for 623-627. I should just add another comment for 657-663 that references the same concern but notes it's for DATABASE mode.
Hmm, but the guideline says "Don't create multiple review comments for the same issue within the same code segment."
But these are NOT the same code segment - they're in different parts of the file.
Let me just add a note to my existing comment that both usages need verification.
Actually, I'll modify my comment to include both line ranges.
[scratchpad_end] -->Also applies to: 657-663
623-627: Verify that RuleEngine supports the onDirtyStateChange prop.The
RuleEnginecomponent is loaded dynamically from../../../plugins/manual-review/rule-engine, but this plugin directory does not exist in the repository. The component is invoked with anonDirtyStateChangecallback in two places (API mode and DATABASE mode), but without access to the plugin implementation, verify that:
- The plugin is properly available at runtime (external dependency, build artifact, or generated code).
- The
RuleEnginecomponent actually accepts and uses theonDirtyStateChangeprop.- The callback is invoked with a boolean value when rules are modified.
If the plugin is missing or doesn't support this prop, the unsaved-changes detection will fail silently.
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx
Show resolved
Hide resolved
…odel-always-asking-close-without-saving-confirmation-alert-coming-pfa
|
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 (2)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx (2)
391-398: Modal may close with unsaved RuleEngine changes.When "Save and Close" is clicked from the unsaved changes dialog (lines 754-761),
handleSave()returnstrueeven when it doesn't persist RuleEngine changes. This causes the modal to close (line 396) while leaving RuleEngine changes unsaved, which contradicts user expectations.This is related to the issue flagged in lines 299-352.
299-352: Save button remains enabled after saving, even when only RuleEngine changes exist—these changes are not persisted.The Save button is enabled when
hasUnsavedChanges()returns true (line 542), which includesruleEngineHasChangesfor DATABASE mode (line 296). However,handleValidateAndSubmit()only persists form configuration and connector changes (lines 307–315); it does not include RuleEngine changes in theupdatePayload.After a successful save,
initialFormDataConfigandinitialConnectorIdare reset (lines 318–319), butruleEngineHasChangesis not, leaving the Save button permanently enabled if only RuleEngine changes exist. Additionally,ruleEngineHasChangesis only reset when the modal closes (line 451), not after save.This creates confusing UX in DATABASE mode:
- User modifies a rule → Save button enables
- User clicks Save → only form/connector config persists; RuleEngine changes are not sent to backend
- Save button remains enabled because
ruleEngineHasChangeswas never resetClarify whether RuleEngine persists its own changes independently, or if the Save button should trigger RuleEngine persistence. If RuleEngine saves independently, either exclude
ruleEngineHasChangesfromhasUnsavedChanges()for DATABASE mode, or provide clearer UX (separate save controls or messaging). If RuleEngine changes must be persisted via the endpoint update, include them in the payload and reset the state after successful save.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx (1)
frontend/src/components/agency/ds-settings-card/DsSettingsCard.jsx (3)
connMode(30-30)formDataConfig(33-33)connDetails(32-32)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (6)
frontend/src/components/agency/configure-connector-modal/ConfigureConnectorModal.jsx (6)
73-75: Good addition of initialization state flags.The three new state flags properly track the initialization lifecycle and RuleEngine dirty state, enabling precise detection of actual changes versus false positives from schema defaults.
107-132: Schema loading lifecycle properly tracked.The logic correctly marks the schema as loaded both when no endpoint exists (line 109-111) and after the schema fetch completes (line 130), ensuring the initialization effect can proceed appropriately in both scenarios.
285-297: Unsaved changes detection is now correct and mode-aware.The normalization of connector IDs at line 293 (
connDetails?.id || null) properly fixes the undefined vs. null mismatch bug. The mode-specific logic correctly returns only RuleEngine changes for API mode, while checking all sources for DATABASE mode.
444-453: Proper cleanup of initialization state on modal close.All initialization flags and baseline values are correctly reset when the modal closes, ensuring a clean state for the next open.
362-389: RuleEngine state not reverted when discarding changes.When the user chooses "Close without Saving" (line 749-753), form data and connector selection are reverted to their initial values (lines 367-384), but RuleEngine changes are not reverted. If RuleEngine maintains its own internal state, this could leave unsaved rule changes in place after the modal closes and reopens.
Verify whether RuleEngine needs explicit state reset when discarding changes. If RuleEngine re-initializes from
workflowDetailson each render, this may not be an issue. Otherwise, consider resetting RuleEngine state or calling a cleanup method here.
626-630: Verify RuleEngine plugin supportsonDirtyStateChangeprop.The RuleEngine component is loaded from an external plugin not included in this repository. Cannot verify that the
onDirtyStateChangeprop is supported by the plugin. Confirm this prop is documented and supported in the rule-engine plugin implementation before merging.
jaseemjaskp
deleted the
UN-3102-bug-workflow-destination-connector-model-always-asking-close-without-saving-confirmation-alert-coming-pfa
branch
What
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
-https://github.com/Zipstack/unstract-cloud/pull/1184
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.