feat(trees): sort Chronostrat tree by period instead of name, add ranges#8254
Open
grantfitzsimmons wants to merge 12 commits into
Open
feat(trees): sort Chronostrat tree by period instead of name, add ranges#8254grantfitzsimmons wants to merge 12 commits into
grantfitzsimmons wants to merge 12 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThe changes add GeologicTimePeriod support for carrying start/end period data from the backend through tree-row fetching and into row rendering, with a new preference and localized labels controlling chrono-period display. ChangesGeologicTimePeriod chrono-period display
Compact metadata
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specifyweb/frontend/js_src/lib/components/TreeView/index.tsx (1)
158-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate stale comment to reflect the new conditional sort logic.
The comment still says name is unconditionally hard-coded, but the code below now branches to
startPeriodforGeologicTimePeriod. Worth a quick update so future readers aren't misled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/frontend/js_src/lib/components/TreeView/index.tsx` around lines 158 - 174, The comment in TreeView index.tsx is stale and still describes the sort field as always hard-coded to name, but the nearby sorting logic now conditionally uses startPeriod for GeologicTimePeriod. Update the comment around the tree row fetch/sort handling to match the actual behavior in the relevant TreeView code path, and keep the reference to userPreferences.get if you mention the preference fallback so future readers aren’t misled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/components/TreeView/index.tsx`:
- Around line 158-174: The comment in TreeView index.tsx is stale and still
describes the sort field as always hard-coded to name, but the nearby sorting
logic now conditionally uses startPeriod for GeologicTimePeriod. Update the
comment around the tree row fetch/sort handling to match the actual behavior in
the relevant TreeView code path, and keep the reference to userPreferences.get
if you mention the preference fallback so future readers aren’t misled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 39f17028-fb57-49b5-ad16-7236ba31ad2e
📒 Files selected for processing (2)
specifyweb/frontend/js_src/lib/components/TreeView/Row.tsxspecifyweb/frontend/js_src/lib/components/TreeView/index.tsx
grantfitzsimmons
changed the title
Triggered by 0c5d1b4 on branch refs/heads/issue-6049
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specifyweb/backend/trees/views.py`:
- Line 168: The query flag handling in the tree view is treating a present
string value as truthy, so the `include_start_end_periods` path in
`trees/views.py` is enabled even when the request sends `"false"`. Update the
`include_start_end_periods` assignment in the view logic to explicitly parse the
GET parameter value into a real boolean before combining it with the `tree ==
'geologictimeperiod'` check, using the same pattern as other request-flag
handling in this view so `fetchRows` can actually disable the option.
In `@specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx`:
- Around line 289-348: The visible label in Row’s GeologicTimePeriod rendering
always prints the range separator even when `row.endPeriod` is missing, causing
malformed text like a dangling dash. Update the JSX in `Row.tsx` so the span
content branches the same way as the `title` logic: use the range format only
when `typeof row.endPeriod === 'string'`, otherwise render a single-period label
with no separator or empty end value. Keep the `trimDecimal`,
`treeText.geologicTimePeriodRange`, and `treeText.geologicTimePeriodSingle`
usage aligned with that check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 91121783-ea90-4997-8e25-a8a70222d748
📒 Files selected for processing (7)
specifyweb/backend/trees/views.pyspecifyweb/frontend/js_src/lib/components/Preferences/UserDefinitions.tsxspecifyweb/frontend/js_src/lib/components/TreeView/Row.tsxspecifyweb/frontend/js_src/lib/components/TreeView/helpers.tsspecifyweb/frontend/js_src/lib/components/TreeView/index.tsxspecifyweb/frontend/js_src/lib/localization/preferences.tsspecifyweb/frontend/js_src/lib/localization/tree.ts
✅ Files skipped from review due to trivial changes (2)
- specifyweb/frontend/js_src/lib/localization/tree.ts
- specifyweb/frontend/js_src/lib/localization/preferences.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- specifyweb/frontend/js_src/lib/components/TreeView/index.tsx
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Triggered by 241ad64 on branch refs/heads/issue-6049
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specifyweb/backend/trees/tests/test_trees.py (1)
340-403: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM on signature update, but consider testing the new flag's actual effect.
Both
get_tree_rowscalls correctly add the newinclude_start_end_periodspositional argument to match the updated signature inviews.py. However, both calls passFalse, so there's no test coverage confirming thatstart_period/end_period/etc. are actually populated (vs."NULL") when the flag isTruefor aGeologicTimePeriodtree — which is the actual feature this PR introduces.Consider adding a test case using a
GeologicTimePeriodtree withinclude_start_end_periods=Trueto verify the projected columns contain real values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/trees/tests/test_trees.py` around lines 340 - 403, The updated get_tree_rows signature is covered, but the new include_start_end_periods behavior is not verified because both existing calls pass False. Add a test in test_trees using a GeologicTimePeriod tree and call get_tree_rows with include_start_end_periods=True, then assert the returned rows populate start_period/end_period and related fields with real values instead of "NULL". Use the existing get_tree_rows helper and the tree setup patterns in this test module to locate the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specifyweb/backend/trees/tests/test_trees.py`:
- Around line 340-403: The updated get_tree_rows signature is covered, but the
new include_start_end_periods behavior is not verified because both existing
calls pass False. Add a test in test_trees using a GeologicTimePeriod tree and
call get_tree_rows with include_start_end_periods=True, then assert the returned
rows populate start_period/end_period and related fields with real values
instead of "NULL". Use the existing get_tree_rows helper and the tree setup
patterns in this test module to locate the new case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6bb22da-ad2a-457b-a410-4c6746d5f887
📒 Files selected for processing (3)
specifyweb/backend/trees/tests/test_trees.pyspecifyweb/frontend/js_src/lib/components/TreeView/Row.tsxspecifyweb/frontend/js_src/lib/localization/tree.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx
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
1 participant
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.
Fixes #6049
This PR hardcodes the Chronostratigraphy (
GeologicTimePeriod) tree to always sort nodes bystartPeriod, independent of the user'sorderByFieldpreference under User Preferences → Tree Editor → Behavior. This a pretty big QoL update for paleo and geo users.Currently, the
orderByFieldpreference controls sorting for all tree types. However, Chronostratigraphy should be using chronological ordering bystartPeriod, which should take precedence over user-configurable sort fields.This also adds a new Show start and end periods toggle preference has been added under User Preferences → Tree Editor → Chronostratigraphy (enabled by default). When enabled, each node displays its geologic time range in ICS format:
(298.9 ± 0.15 – 251.902 ± 0.024). Trailing zeros are trimmed. Hovering over the range shows a tooltip with full details using schema field labels.Testing instructions
Compare against the Chronostrat chart: https://stratigraphy.org/ICSchart/ChronostratChart2026-06.pdf
startPeriod(ascending geologic time).Campanian (83.6 ± 0.2 – 72.2 ± 0.2)).name,fullName,rankId,nodeNumber).startPeriod.orderByFieldpreference still works correctly thereSummary by CodeRabbit