Skip to content

Conversation

@KCSAbeywickrama
Copy link
Contributor

@KCSAbeywickrama KCSAbeywickrama commented Nov 29, 2025

Purpose

Fixes wso2/product-ballerina-integrator#2025

Summary by CodeRabbit

  • Refactor
    • Simplified the reset handler architecture by internalizing reset logic and removing the public reset callback from the data mapper API.

✏️ Tip: You can customize this high-level summary in your review settings.

@KCSAbeywickrama KCSAbeywickrama requested review from madushajg and removed request for gigara and hevayo November 29, 2025 09:02
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 29, 2025

Walkthrough

This change relocates reset handling from external prop control to internal component logic. The onReset prop is removed from the public DataMapperEditorProps interface and from DataMapperView, while DataMapperEditor now implements its own internal handleOnReset handler that computes the reset payload and triggers deletion.

Changes

Cohort / File(s) Summary
Public API Update
workspaces/ballerina/data-mapper/src/index.tsx
Removed onReset: () => Promise<void> property from DataMapperEditorProps interface
Internal Reset Logic
workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx
Added internal handleOnReset handler that derives target field from last view, computes output by removing trailing '0' components, and calls deleteMapping; replaced external onReset prop with internal handler passed to DataMapperHeader
Handler Removal
workspaces/ballerina/ballerina-visualizer/src/views/DataMapper/DataMapperView.tsx
Removed onDMReset handler and onReset prop passed to DataMapper component

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

  • Public API change: Verify all consumers of DataMapperEditor have been updated to remove onReset prop usage
  • Reset logic migration: Confirm the internal handleOnReset implementation produces equivalent behavior to the previous external handler
  • View integration: Ensure removal of reset triggering in DataMapperView doesn't break existing workflows

Suggested reviewers

  • hevayo
  • gigara

Poem

🐰 A reset that lives within, not without,
No crashing views when mappings clear out.
Logic moves home, the prop fades away,
Internal harmony saves the day! ✨

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is incomplete and lacks most required template sections including Goals, Approach, Testing, Documentation, and other critical details. Expand the description to include Goals (how the crash is prevented), Approach (implementation details), test coverage, documentation updates, and other relevant template sections.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the fix for a crash when clearing all mappings in a focused view, directly matching the core issue being addressed.
Linked Issues check ✅ Passed The PR addresses the linked issue by moving reset handling internally and removing the problematic onReset prop that caused crashes in focused views.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the crash by refactoring reset handling from the DataMapperView to DataMapperEditor and removing the public onReset prop.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 34ba47b and 7ed6cd3.

📒 Files selected for processing (3)
  • workspaces/ballerina/ballerina-visualizer/src/views/DataMapper/DataMapperView.tsx (0 hunks)
  • workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx (2 hunks)
  • workspaces/ballerina/data-mapper/src/index.tsx (0 hunks)
💤 Files with no reviewable changes (2)
  • workspaces/ballerina/ballerina-visualizer/src/views/DataMapper/DataMapperView.tsx
  • workspaces/ballerina/data-mapper/src/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a830927 and 7b4fede.

📒 Files selected for processing (4)
  • workspaces/ballerina/ballerina-visualizer/src/views/DataMapper/DataMapperView.tsx (0 hunks)
  • workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx (2 hunks)
  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx (2 hunks)
  • workspaces/ballerina/data-mapper/src/index.tsx (0 hunks)
💤 Files with no reviewable changes (2)
  • workspaces/ballerina/data-mapper/src/index.tsx
  • workspaces/ballerina/ballerina-visualizer/src/views/DataMapper/DataMapperView.tsx
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/components/**/*.tsx : Use React 18.2.0 features including concurrent rendering and automatic batching; avoid class components in favor of functional components with hooks

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx
  • workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx
📚 Learning: 2025-11-26T07:49:56.428Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:136-141
Timestamp: 2025-11-26T07:49:56.428Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBackButton() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx
  • workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx
📚 Learning: 2025-11-25T06:34:10.812Z
Learnt from: CR
Repo: wso2/vscode-extensions PR: 0
File: workspaces/ballerina/component-diagram/AGENTS.md:0-0
Timestamp: 2025-11-25T06:34:10.812Z
Learning: Applies to workspaces/ballerina/component-diagram/src/resources/icons/**/*.tsx : Create separate SVG icon components in src/resources/icons/ for all diagram icons and import them as React components

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx
📚 Learning: 2025-11-27T07:58:16.698Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 897
File: workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx:102-107
Timestamp: 2025-11-27T07:58:16.698Z
Learning: In workspaces/ballerina/data-mapper/src/components/Diagram/Label/MappingOptionsWidget.tsx, the `inProgress` state in `wrapWithProgress` intentionally doesn't reset to `false`. The component unmounts when the onClick operation finishes, and resetting the state would cause the mapping options menu to briefly reappear while the data mapper loads new content from file changes.

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx
  • workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx
📚 Learning: 2025-11-26T06:34:09.752Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 653
File: workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts:128-134
Timestamp: 2025-11-26T06:34:09.752Z
Learning: In workspaces/bi/bi-extension/src/test/e2e-playwright-tests/data-mapper/DataMapperUtils.ts, the goPrevViewBreadcrumb() method is only called when in a focused view, ensuring breadcrumbs are always present. No guard for empty breadcrumbs is needed.

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx
📚 Learning: 2025-11-24T14:51:49.267Z
Learnt from: KCSAbeywickrama
Repo: wso2/vscode-extensions PR: 998
File: workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx:113-132
Timestamp: 2025-11-24T14:51:49.267Z
Learning: In workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ExpressionBar.tsx, if `textFieldRef.current` is not undefined, `textFieldRef.current.inputElement` is guaranteed to exist. If `inputElement` doesn't exist when `current` exists, it's a fatal error that should reach the error boundary rather than being handled with defensive null checks.

Applied to files:

  • workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx
  • workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx
🧬 Code graph analysis (2)
workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx (1)
workspaces/ballerina/data-mapper/src/components/DataMapper/Header/ActionIconButton.tsx (1)
  • ActionIconButton (28-48)
workspaces/ballerina/data-mapper/src/components/DataMapper/DataMapperEditor.tsx (2)
workspaces/ballerina/ballerina-extension/src/rpc-managers/data-mapper/rpc-manager.ts (1)
  • deleteMapping (259-279)
workspaces/ballerina/ballerina-extension/src/core/extended-language-client.ts (1)
  • deleteMapping (825-827)
🔇 Additional comments (1)
workspaces/ballerina/data-mapper/src/components/DataMapper/Header/DataMapperHeader.tsx (1)

69-81: Refresh button wiring and spacing look solid

The new ActionIconButton for “Refresh all mappings” is correctly typed against onRefresh: () => Promise<void> and fits the existing async/spinner pattern. The small gap: 2px in ActionGroupContaner cleanly separates reset vs refresh without behavioral side effects. No changes requested.

Also applies to: 138-141

KavinduZoysa
KavinduZoysa previously approved these changes Nov 29, 2025
@KCSAbeywickrama KCSAbeywickrama dismissed KavinduZoysa’s stale review November 29, 2025 14:16

The merge-base changed after approval.

@KCSAbeywickrama KCSAbeywickrama merged commit a307e9d into wso2:main Nov 29, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Data Mapper] Crash when clear all mappings while in inside a focused view

2 participants