Skip to content

Conversation

@Gitesh307
Copy link
Contributor

@Gitesh307 Gitesh307 commented Sep 30, 2025

Fixes #7445

This PR removes a set of legacy preferences previously defined in remotePrefs.ts and CollectionDefinitions.tsx that have either been migrated to the User Preferences system or are no longer relevant. By retiring these obsolete keys (e.g., tree-viewer sort fields, rank thresholds, and TaxonTreeEditor.DisplayAuthor), the codebase is simplified and avoids confusion around outdated settings. All remaining logic referencing these preferences has been stripped out, and the Tree Viewer components now rely solely on the modern User Preferences system for configuration.

Checklist

  • Self-review the PR after opening it to make sure the changes look good and
    self-explanatory (or properly documented)
  • Add relevant issue to release milestone

Testing instructions

  • Verify Tree Viewer functionality works correctly.
  • Load a tree view (e.g., Taxon) with the network panel open; expand a node whose rank is below the current threshold and confirm no …/stats/ request is issued, then expand a higher-rank node and confirm one is.
  • Confirm the app loads without errors and obsolete prefs like form.definition.columnSource are not used.

@CarolineDenis CarolineDenis requested a review from a team October 1, 2025 12:12
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

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

  • Verify Tree Viewer functionality works correctly.
  • In Preferences → Tree Editor, verify the new “Minimum rank for Collection Object counts” control appears under each tree section with the expected default of 0.
  • Load a tree view (e.g., Taxon) with the network panel open; expand a node whose rank is below the current threshold and confirm no …/stats/ request is issued, then expand a higher-rank node and confirm one is.
  • Confirm the app loads without errors and obsolete prefs like form.definition.columnSource are not used.

The only concern I have is if setting the minimum count rank to zero for every tree by default would have any performance issues? From what I tried I didn't run into anything major, however, I know there are some very large trees out there that could potentially be impacted.

@emenslin emenslin requested a review from a team October 3, 2025 21:33
@melton-jason
Copy link
Contributor

melton-jason commented Oct 7, 2025

The only concern I have is if setting the minimum count rank to zero for every tree by default would have any performance issues? From what I tried I didn't run into anything major, however, I know there are some very large trees out there that could potentially be impacted.

#7461 (review)

There definitely could be some performance issues. Primarily, this could lead to the same Issue as #3821 if the tree count queries execute past the time out threshold.

However, the scale of the database would likely need to be one we have not seen before. I ran some tests on a local copy of Herb RBGE with the minimum count rank preference set to 0. In 5 runs, the average time it took execute the tree count query on their whole Taxon tree was 6.394 seconds.
(Of course, my own local testing should be taken with a grain of salt: actual response times can vary depending on available resources to the reverse proxy and Specify 7, concurrent operations to the instance, the "distance" between the browser, Specify instance, and database etc.)

Most of the default timeout settings with NGINX (see nginx_http_core and nginx_http_proxy) are at 60 seconds. I believe this means unless the institution configured their reverse proxy to have a lower timeout, the response would need to take ~60 seconds before the problems mentioned in #3821 become apparent.

We can verify statistics on other databases (there are some examples in #3613 and #3821 (comment)) to see if any reasonably approach the timeout limit.

The stats queries generally happen concurrently to other tree usage, so the user should receive little other usage/performance impact. Although with HTTP 1.1, there is the 6-8 limit on "in-flight" requests (#2608 (comment)), so this does limit how many other requests the browser can make concurrently while the stats are being fetched.

@Gitesh307 Gitesh307 requested a review from a team October 14, 2025 02:54
@CarolineDenis CarolineDenis removed the request for review from melton-jason November 12, 2025 15:02
@CarolineDenis CarolineDenis added this to the 7.12.0 milestone Nov 12, 2025
Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

@Gitesh307 This removes the TreeEditor.Rank.Threshold. preference, but the PR that adds this functionality to User Preferences is not yet introduced. This PR, if merged, would remove that option for users entirely regardless of what is configured until #6844 is merged.

@CarolineDenis, should @Gitesh307 finish #6844 or should we add back that as a preference until a later time?

Testing instructions

  • Verify Tree Viewer functionality works correctly.
  • Confirm the app loads without errors and obsolete prefs like form.definition.columnSource are not used.

@github-project-automation github-project-automation bot moved this from 📋Back Log to Dev Attention Needed in General Tester Board Nov 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Dev Attention Needed

Development

Successfully merging this pull request may close these issues.

Retire and remove obsolete preferences

6 participants