feat(trees): sort Chronostrat tree by period instead of name, add ranges#8254
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes add GeologicTimePeriod start/end period data and biostrat filtering through the backend tree API, frontend tree rendering, and search controls. A new preference and localization strings control chrono-period display, and QueryComboBox adds BioStrat-specific filtering plus text normalization. ChangesGeologicTimePeriod tree view
BioStrat query combo filtering
Compact metadata
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (3 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 matches the behavior from Specify 6
CarolineDenis
approved these changes
Jul 6, 2026
emenslin
approved these changes
Jul 6, 2026
emenslin
left a comment
emenslin
left a comment
Collaborator
There was a problem hiding this comment.
- Open the Chronostratigraphy tree
- Expand nodes at various levels and verify that children are ordered by
startPeriod(ascending geologic time). - Verify the displayed time ranges match the chart (e.g.,
Campanian (83.6 ± 0.2 – 72.2 ± 0.2)). - Hover over a time range and verify the tooltip shows the schema field labels with values.
- Toggle the Show start and end periods preference off and verify the ranges disappear.
- Change the Tree Editor → Behavior → Sort by field preference to different values (
name,fullName,rankId,nodeNumber). - Verify that the Chronostrat tree is unaffected by this preference change since it always sorts by
startPeriod. - Open a different tree (Taxon, Storage, Geography, etc.) and verify the
orderByFieldpreference still works correctly there
Looks good, I didn't run into any issues!
feat(trees): add biostrat filter to chronostrat
Member
Author
|
@copilot resolve the merge conflicts in this pull request (let's see how this goes) |
Triggered by 56be873 on branch refs/heads/issue-6049
Contributor
Resolved. I merged |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specifyweb/backend/trees/views.py (1)
155-186: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSort-by-period guarantee for GeologicTimePeriod is enforced only on the client.
The fix for
#6049(always sort chrono tree bystartPeriod) is implemented solely by the frontend passingsortfield='startPeriod'in the URL (seeTreeView/index.tsx).get_tree_rows/tree_viewstill blindly trust thesortfieldquery parameter for every tree, includinggeologictimeperiod. Any other API consumer (or a future frontend regression) could passsortfield=nameand silently reintroduce the original bug. Consider forcingsortfieldto'startPeriod'server-side whenevertree == 'geologictimeperiod', so the ordering guarantee doesn't depend on client cooperation.🤖 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/views.py` around lines 155 - 186, The sort-order fix for geologictimeperiod is only enforced by the client, while tree_view/get_tree_rows still accept any sortfield from the request. Update tree_view (or get_tree_rows) to override sortfield to startPeriod whenever tree is geologictimeperiod, so the ordering guarantee cannot be bypassed by other callers. Keep the change localized around tree_view, get_tree_rows, and the orderby lookup.
♻️ Duplicate comments (1)
specifyweb/backend/trees/views.py (1)
167-169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTruthiness bug still present:
includestartendperiodsparam is always truthy.
request.GET.get('includestartendperiods', False)returns the raw string when present, so"false"(or any non-empty string) is still truthy. This meansinclude_start_end_periodsis effectively alwaysTruefortree == 'geologictimeperiod', defeating the point of togglingdisplayChronoPeriodsoff — this was already flagged on a prior commit and remains unfixed.🐛 Proposed fix
- include_start_end_periods = request.GET.get('includestartendperiods', False) and tree == 'geologictimeperiod' + include_start_end_periods = request.GET.get('includestartendperiods', 'false').lower() == 'true' and tree == 'geologictimeperiod'🤖 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/views.py` around lines 167 - 169, `include_start_end_periods` in `trees.views` is still using the raw `request.GET.get('includestartendperiods', False)` value, so any non-empty string like "false" evaluates truthy. Update the parsing in the view logic around `include_author` and `include_start_end_periods` to explicitly interpret the query param as a boolean (for example, by checking for an allowed true value) before combining it with `tree == 'geologictimeperiod'`. This should be fixed in the same request-handling block where `biostrat` is read.
🧹 Nitpick comments (2)
specifyweb/frontend/js_src/lib/components/TreeView/__tests__/biostratFilter.test.ts (1)
14-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest duplicates logic instead of exercising the real implementation.
filterBio/filterChrono/filterAllare reimplemented locally rather than imported fromSearch.tsx, so this test provides no protection against the actual filter logic regressing. Consider extracting the predicate into a small exported helper used by bothSearch.tsxand this test.🤖 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/__tests__/biostratFilter.test.ts` around lines 14 - 20, The biostrat filter test is duplicating predicate logic locally instead of validating the real implementation. Extract the filter predicate(s) from Search.tsx into a small exported helper and use that helper in both Search.tsx and biostratFilter.test.ts so the test exercises the actual shared logic. Update the test to import the helper instead of defining filterBio, filterChrono, and filterAll inline.specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx (1)
595-609: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHardcoded
'1'discards the condition's actualstartValue.Every sibling branch in this mapper (
rankId,nodeNumber,collectionRelTypeId,taxonTreeDefId/definitionId) forwards the realstartValuefrom the serialized condition. This new branch instead hardcodesvalue: '1', silently ignoring whateverhelpers.tsactually computed ('true'). It works today only because helpers.ts never emits anything else forisBioStrat, but the coupling is implicit and would break silently if that ever changes.♻️ Suggested fix: reuse startValue for consistency
: fieldName === 'isBioStrat' ? { field: 'isBioStrat', isRelationship: false, operation: 'in', isNot: false, - value: '1', + value: startValue === 'true' ? '1' : '0', }Also worth confirming the backend/
extraFiltersconsumer actually expects'1'/'0'(vs'true'/'false') for booleaninfilters, since that contract lives outside this file.🤖 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/QueryComboBox/index.tsx` around lines 595 - 609, The isBioStrat branch in QueryComboBox’s extended filter mapper is hardcoding a value instead of using the serialized condition’s startValue. Update the isBioStrat case to follow the same pattern as the sibling branches (rankId, nodeNumber, collectionRelTypeId, taxonTreeDefId/definitionId) by forwarding startValue into the returned filter object, and verify the extraFilters/backend contract for boolean in values while keeping the mapping consistent in the QueryComboBox logic.
🤖 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/frontend/js_src/lib/components/TreeView/index.tsx`:
- Around line 429-436: The new biostrat filter Select in TreeView is missing the
same accessibility attributes used by the other Select controls. Update the
Select component in TreeView to include a descriptive aria-label and title,
matching the pattern used by the existing Select around the other filter
controls, so screen readers can identify it consistently.
---
Outside diff comments:
In `@specifyweb/backend/trees/views.py`:
- Around line 155-186: The sort-order fix for geologictimeperiod is only
enforced by the client, while tree_view/get_tree_rows still accept any sortfield
from the request. Update tree_view (or get_tree_rows) to override sortfield to
startPeriod whenever tree is geologictimeperiod, so the ordering guarantee
cannot be bypassed by other callers. Keep the change localized around tree_view,
get_tree_rows, and the orderby lookup.
---
Duplicate comments:
In `@specifyweb/backend/trees/views.py`:
- Around line 167-169: `include_start_end_periods` in `trees.views` is still
using the raw `request.GET.get('includestartendperiods', False)` value, so any
non-empty string like "false" evaluates truthy. Update the parsing in the view
logic around `include_author` and `include_start_end_periods` to explicitly
interpret the query param as a boolean (for example, by checking for an allowed
true value) before combining it with `tree == 'geologictimeperiod'`. This should
be fixed in the same request-handling block where `biostrat` is read.
---
Nitpick comments:
In `@specifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsx`:
- Around line 595-609: The isBioStrat branch in QueryComboBox’s extended filter
mapper is hardcoding a value instead of using the serialized condition’s
startValue. Update the isBioStrat case to follow the same pattern as the sibling
branches (rankId, nodeNumber, collectionRelTypeId, taxonTreeDefId/definitionId)
by forwarding startValue into the returned filter object, and verify the
extraFilters/backend contract for boolean in values while keeping the mapping
consistent in the QueryComboBox logic.
In
`@specifyweb/frontend/js_src/lib/components/TreeView/__tests__/biostratFilter.test.ts`:
- Around line 14-20: The biostrat filter test is duplicating predicate logic
locally instead of validating the real implementation. Extract the filter
predicate(s) from Search.tsx into a small exported helper and use that helper in
both Search.tsx and biostratFilter.test.ts so the test exercises the actual
shared logic. Update the test to import the helper instead of defining
filterBio, filterChrono, and filterAll inline.
🪄 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: 6bb85ccc-f91c-45ea-a2f4-6ee7bec57ee3
📒 Files selected for processing (17)
config/common/schema_localization_en.jsonspecifyweb/backend/context/data/schemalocalization.jsonspecifyweb/backend/trees/tests/test_trees.pyspecifyweb/backend/trees/views.pyspecifyweb/frontend/js_src/lib/components/QueryComboBox/__tests__/biostratConditions.test.tsspecifyweb/frontend/js_src/lib/components/QueryComboBox/helpers.tsspecifyweb/frontend/js_src/lib/components/QueryComboBox/index.tsxspecifyweb/frontend/js_src/lib/components/TreeView/Row.tsxspecifyweb/frontend/js_src/lib/components/TreeView/Search.tsxspecifyweb/frontend/js_src/lib/components/TreeView/Tree.tsxspecifyweb/frontend/js_src/lib/components/TreeView/__tests__/biostratFilter.test.tsspecifyweb/frontend/js_src/lib/components/TreeView/helpers.tsspecifyweb/frontend/js_src/lib/components/TreeView/index.tsxspecifyweb/frontend/js_src/lib/localization/tree.tsspecifyweb/frontend/js_src/lib/tests/ajax/static/context/schema_localization.json/lang=en.jsonspecifyweb/frontend/js_src/lib/utils/cache/definitions.tsspecifyweb/specify/fixtures/empty_db.json
✅ Files skipped from review due to trivial changes (3)
- specifyweb/specify/fixtures/empty_db.json
- specifyweb/frontend/js_src/lib/tests/ajax/static/context/schema_localization.json/lang=en.json
- config/common/schema_localization_en.json
🚧 Files skipped from review as they are similar to previous changes (3)
- specifyweb/frontend/js_src/lib/localization/tree.ts
- specifyweb/frontend/js_src/lib/components/TreeView/Row.tsx
- specifyweb/frontend/js_src/lib/components/TreeView/helpers.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
g1rly-c0d3r
approved these changes
Jul 7, 2026
g1rly-c0d3r
left a comment
g1rly-c0d3r
left a comment
Collaborator
There was a problem hiding this comment.
- Open the Chronostratigraphy tree
- Expand nodes at various levels and verify that children are ordered by
startPeriod(ascending geologic time). - Verify the displayed time ranges match the chart (e.g.,
Campanian (83.6 ± 0.2 – 72.2 ± 0.2)). - Hover over a time range and verify the tooltip shows the schema field labels with values.
- Toggle the Show start and end periods preference off and verify the ranges disappear.
- Change the Tree Editor → Behavior → Sort by field preference to different values (
name,fullName,rankId,nodeNumber). - Verify that the Chronostrat tree is unaffected by this preference change since it always sorts by
startPeriod. - Open a different tree (Taxon, Storage, Geography, etc.) and verify the
orderByFieldpreference still works correctly there
All good! Timestamps appear correctly.
Collaborator
Updated: Everything looks good. Didn't run into any issues! |
rijulpoudel
approved these changes
Jul 7, 2026
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…None' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
github-project-automation
Bot
moved this from Dev Attention Needed
to ✅Done
in General Tester Board
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
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