Skip to content

fix(data-views): handle record merging - #8624

Merged
CarolineDenis merged 3 commits into
mainfrom
issue-8623-mergefix
Oct 5, 2026
Merged

CarolineDenis merged 3 commits into
mainfrom
issue-8623-mergefix

Conversation

@grantfitzsimmons

@grantfitzsimmons grantfitzsimmons commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Fixes #8623

This PR solves the issue introduced with Data Views when merging records. It should now:

  • Clear selected record IDs and the record preview before refreshing after a merge.
  • Added a dedicated merge callback through the query results (in both data views and query builder).

Testing instructions

  • Open Data Views
    • Select Agent.
    • Select two or more Agent records.
    • Click Merge Records and complete the merge.
  • Confirm the Data View query reruns without crashing.

Summary by CodeRabbit

  • Bug Fixes
    • After records are merged from query results, the results refresh automatically and the previous selection and preview are cleared.
    • When records are deleted, they are removed from the current results, and the selection and preview are reset to keep the view in sync.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6f529568-27c2-464b-a49f-42bd5c917d5c
📥 Commits

Reviewing files that changed from the base of the PR and between d9dc704 and 34deda5.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 2a2a2e42-42aa-4818-b520-773883b2f7db

📥 Commits

Reviewing files that changed from the base of the PR and between f736d54 and d9dc704.

📒 Files selected for processing (3)
  • specifyweb/frontend/js_src/lib/components/DataViews/index.tsx
  • specifyweb/frontend/js_src/lib/components/QueryBuilder/Results.tsx
  • specifyweb/frontend/js_src/lib/components/QueryBuilder/ResultsWrapper.tsx

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Query results now report record deletions and merge completion through optional callbacks. Data Views clears selected record IDs and resets the preview index after these events. After a merge, it also refreshes query results.

Changes

Data Views merge refresh

Layer / File(s) Summary
Merge callback and Data Views refresh
specifyweb/frontend/js_src/lib/components/QueryBuilder/Results.tsx, specifyweb/frontend/js_src/lib/components/QueryBuilder/ResultsWrapper.tsx, specifyweb/frontend/js_src/lib/components/DataViews/index.tsx
Query results report deleted records and merge completion through optional callbacks. Data Views clears selection and resets the preview index after deletion. After a merge, it clears selection, resets the preview index, and refreshes query results.

Suggested reviewers: carolinedenis, kwhuber

Priority: ➖ Normal

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to d9dc7

The merge flow now reconciles removed records and refreshes Data Views without an identified remaining merge risk.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d9dc7

The change is narrowly scoped to keeping selection and preview state consistent with record changes. No new access or mutation capability was identified, but recovery after interrupted or unsuccessful merges remains only partially established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The new notification's direct effect is bounded to the mounted Data View's selection and preview. The deletion subscriber accepts resource events without filtering by table, so clearing can occur for an unrelated deletion event, but the added consumer only clears local state and does not mutate that other resource.

Trust Boundaries and Controls

  • observed — The results UI retains its canMerge(table) rendering gate. Callback execution performs notification and state cleanup rather than an authorization decision. This frontend gate does not establish server-side authorization or tenant isolation.

Resilience and Maintainability Implications

  • observed — The existing status dialog uses the same close callback for success, failure, aborted status, and completion of an abort request. That callback emits deletion events for every clone before closing. Consequently, these notifications are client invalidation signals, not proof of successful deletion or backend rollback. The new selection cleanup does not resolve the underlying recovery uncertainty.
🚥 Pre-merge checks | ✅ 4 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Automatic Tests ⚠️ Warning Automatic tests are necessary for this behavior change. The PR changes merge and deletion callback flow across RecordMergingLink, QueryResults, QueryResultsWrapper, and LoadedDataViewFromTable… Add automatic Jest/React Testing Library regression tests. Verify that a merge clears the Data Views selected IDs and preview state before the refreshed results arrive, triggers the refresh callback, and that deleted records invoke local re…
Testing Instructions ⚠️ Warning The instructions clearly cover the Data Views merge flow, but they do not cover the Query Builder path. The PR changes the shared QueryResults and QueryResultsWrapper components, and `QueryBuilder… Add a Query Builder test. Open Query Builder, run an Agent query, select at least two records, merge the records, and confirm that the results rerun without an error. Also confirm that the selected rows and record preview clear after the me…
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing record merging in Data Views.
Linked Issues check ✅ Passed Issue #8623 requires Data Views to rerun the query after merging records without loading deleted records. LoadedDataViewFromTable.handleMerged clears selectedIds, resets selectedIndex, and calls…
Out of Scope Changes check ✅ Passed The changes stay within issue #8623. The callback plumbing supports merge handling in Data Views. The onDeleted plumbing clears stale Data Views selection after deletion and supports the same record…
Full details: Automatic Tests

Explanation

Automatic tests are necessary for this behavior change. The PR changes merge and deletion callback flow across RecordMergingLink, QueryResults, QueryResultsWrapper, and LoadedDataViewFromTable to prevent a Data Views crash. The review-scoped diff contains only three production files and no test additions. Existing Jest and React Testing Library tests are available for the Data Views and Query Builder components, but no existing test covers this merge regression.

Resolution

Add automatic Jest/React Testing Library regression tests. Verify that a merge clears the Data Views selected IDs and preview state before the refreshed results arrive, triggers the refresh callback, and that deleted records invoke local result removal plus the parent deletion callback. Also verify that QueryResults retains the onReRun fallback when onMerged is not provided.

Full details: Testing Instructions

Explanation

The instructions clearly cover the Data Views merge flow, but they do not cover the Query Builder path. The PR changes the shared QueryResults and QueryResultsWrapper components, and QueryBuilderResults uses this path. The PR description also states that the callback applies to both Data Views and Query Builder.

Resolution

Add a Query Builder test. Open Query Builder, run an Agent query, select at least two records, merge the records, and confirm that the results rerun without an error. Also confirm that the selected rows and record preview clear after the merge in both Data Views and Query Builder.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

@kwhuber kwhuber left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Open Data Views
    • Select Agent.
    • Select two or more Agent records.
    • Click Merge Records and complete the merge.
  • Confirm the Data View query reruns without crashing.

Still receiving the same crash after merge on ojsmnh and auburn.

Specify 7 Crash Report - 2026-10-02T16_23_50.698Z.txt

Image

@grantfitzsimmons

Copy link
Copy Markdown
Member Author

@kwhuber Thanks for testing! Your crash shows that the agent request was happening during the deletion but before the post-merge request happens... Data Views now clears its selection immediately! Should be fixed 🤞

@grantfitzsimmons
grantfitzsimmons requested review from a team October 2, 2026 19:12

@JDAM2k4 JDAM2k4 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Testing instructions

  • Open Data Views
    • Select Agent.
    • Select two or more Agent records.
    • Click Merge Records and complete the merge.
  • Confirm the Data View query reruns without crashing.

This didn't work with naturkundemuseum, as it gave me the error Unknown column 'spmerging.status' in 'SELECT both with and without the auto-populate checkbox selected. I think that's due to a separate error though.

Specify 7 Crash Report - 2026-10-02T19_19_30.765Z.txt

I tried this again in the pri database and it worked fine, regardless of the auto-populate checkbox.

@grantfitzsimmons

Copy link
Copy Markdown
Member Author

error Unknown column 'spmerging.status' in 'SELECT both with and without the auto-populate checkbox selected

@JDAM2k4 That is a migration error, correct! I would stop using that database in favor of one that is currently up-to-date with all migrations.

@grantfitzsimmons
grantfitzsimmons requested a review from a team October 2, 2026 19:39

@gabek96 gabek96 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Testing instructions

  • Open Data Views
    • Select Agent.
    • Select two or more Agent records.
    • Click Merge Records and complete the merge.
  • Confirm the Data View query reruns without crashing.

Works great!

@CarolineDenis
CarolineDenis merged commit 02457b0 into main Oct 5, 2026
20 checks passed
@CarolineDenis
CarolineDenis deleted the issue-8623-mergefix branch October 5, 2026 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅Done

Development

Successfully merging this pull request may close these issues.

Merging records in Data Views causes a crash

5 participants