feat: add config default sorting for proposal and dataset tables#2249
Open
feat: add config default sorting for proposal and dataset tables#2249
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
datasets.effects, themaphandler mutates theparamsobject from the store (params.limits.orderis set in place); consider cloningparams/limitsbefore modifying to preserve state immutability guarantees. - The logic for deriving default sort column/direction (including handling comma-separated column names) is now duplicated and slightly inconsistent between proposals and datasets; consider extracting a shared helper to compute the
ordervalue from config to keep behavior aligned and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `datasets.effects`, the `map` handler mutates the `params` object from the store (`params.limits.order` is set in place); consider cloning `params`/`limits` before modifying to preserve state immutability guarantees.
- The logic for deriving default sort column/direction (including handling comma-separated column names) is now duplicated and slightly inconsistent between proposals and datasets; consider extracting a shared helper to compute the `order` value from config to keep behavior aligned and easier to maintain.
## Individual Comments
### Comment 1
<location path="src/app/state-management/effects/proposals.effects.ts" line_range="65-69" />
<code_context>
- if (sortColumn && sortDirection) {
- limitsParam.order = `${sortColumn}:${sortDirection}`;
- }
+ const column = sortColumn
+ ? sortColumn.includes(",")
+ ? sortColumn.split(",")[0]
+ : sortColumn
+ : defaultColumn;
</code_context>
<issue_to_address>
**suggestion:** Trim the extracted column name when splitting on commas to avoid accidental whitespace issues.
Since `sortColumn` may contain multiple comma-separated names from config (e.g. `"pi_lastname, pi_firstname"`), the first element after `split(',')` might include leading/trailing spaces. Please trim it, e.g. `sortColumn.split(',')[0].trim()`, so the selected column is stable even if config spacing changes.
```suggestion
const column = sortColumn
? sortColumn.includes(",")
? sortColumn.split(",")[0].trim()
: sortColumn
: defaultColumn;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
nitrosx
requested changes
Mar 9, 2026
Member
nitrosx
left a comment
There was a problem hiding this comment.
Overall is fine, can you please add some documentation on how to configure it?
Member
|
I think the sourcery comment makes sense |
Member
Author
Documentation for it has been added to this PR: SciCatProject/backend#2587 |
Junjiequan
approved these changes
Mar 16, 2026
nitrosx
requested changes
Mar 17, 2026
Member
nitrosx
left a comment
There was a problem hiding this comment.
Can you add some documentation and also the changes that needs to happen on the frontend config files to use this feature?
If I am not mistaken, you need to add the key sort to the column that you want to use for sorting as a default, correct?
0f70164 to
07e067e
Compare
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.
Description
This PR adds support for configurable default sorting in datasets and proposals tables through the backend configuration file.
Motivation
Background on use case, changes needed
Fixes:
Please provide a list of the fixes implemented in this PR
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Add support for configurable default sorting of proposals and datasets tables based on backend configuration, and cover this behavior with end-to-end tests.
New Features:
Tests: