Skip to content

Add an option to specify what column is used to sort the table on an initial page load#64

Open
matthewcornell wants to merge 1 commit into
mainfrom
mc/initial-sort-column/4
Open

Add an option to specify what column is used to sort the table on an initial page load#64
matthewcornell wants to merge 1 commit into
mainfrom
mc/initial-sort-column/4

Conversation

@matthewcornell
Copy link
Copy Markdown
Collaborator

Implements #4 : Adds initial_sort_column to initialize() options, with validation via _validateOptions() (checks type and that the name matches a known scores.csv column). Uses DataTables name-based ordering so sorting is column-position-independent. Documents the option and its column-name -> header mapping in README.md.

Reviewer notes:

  • adds _initialSortColumn var that's initialized in initialize() (defaulting to 'model_id') and used by updateTable() to initially order the table
  • we changed to DataTables name-based ordering so sorting is column-position-independent. this allowed name-based initialization from the options, and removed two old TODOs
  • added _validateOptions(), which checks 'initial_sort_column' if present, and also does a few extra checks that were missing
  • predevals.bundle.js were both auto-generated

… _validateOptions() (checks type and that the name matches a known scores.csv column). Uses DataTables name-based ordering so sorting is column-position-independent. Documents the option and its column-name -> header mapping in README.md.
@matthewcornell
Copy link
Copy Markdown
Collaborator Author

TODO: claude review:

PR #64 Review: initial_sort_column option

Overview

Adds an initial_sort_column option to App.initialize() that controls which column the scores table sorts on at load. It also introduces a _validateOptions() helper and migrates DataTables from index-based to name-based column ordering, cleaning up two old // TODO comments.


Correctness

Name-based DataTables ordering — the switch from order: [[0, 'asc']] to order: [{name: this._initialSortColumn, dir: 'asc'}] is correct and more robust. Adding name: columnName to every dtColumns entry is the right prerequisite.

columnDefs.targets fix (src/predevals.js:641) — good change from hardcoded [0] to scoreTableCols.indexOf('model_id'). One edge case: Array.indexOf returns -1 if model_id is absent, and DataTables interprets -1 as "all columns." Extremely unlikely to occur in practice but worth knowing.

Validation runs before state is set_validateOptions(options) accesses options['targets'] directly (not this.state.targets), so it correctly works before this.state.targets is assigned on the next line. No issue here.

Cross-target column validity gap_validateOptions() collects valid column names from all targets combined (src/predevals.js:284–289). A column valid for target A may not appear in target B's scores_table. When the user switches to target B, updateTable() rebuilds the table using _initialSortColumn, but that name may not match any column in the new table. DataTables' name-based ordering silently does nothing if the name isn't found — the sort order falls back to insertion order. This is non-breaking but could be surprising. Consider documenting it alongside the ascending-only limitation, or logging a warning in updateTable() when _initialSortColumn isn't in scoreTableCols.

_initialSortColumn is not really "initial" — every call to updateTable() (including target/eval-set changes) re-applies the sort from _initialSortColumn. A user who manually sorts by a different column and then changes the eval set will see the sort reset. This matches the old behavior (order: [[0, 'asc']] also always reset), so it's not a regression, but the naming implies it only applies once at startup. The README documents the ascending-only limitation but not the reset-on-update behavior.


Code Quality

showDialog() using window.alert() — adequate for a developer-facing init error, but alert dialogs are blocking and stylistically dated. Since your app already uses Bootstrap, a simple modal or console.error with a visible DOM message might integrate better. Not a blocker for this PR.

Error thrown as a string_validateOptions() throws plain strings, not Error objects. This means callers lose stack traces. Since the catch in initialize() immediately shows the message and returns, there's no practical impact here, but throw new Error(...) is the conventional pattern.

_validateOptions() is effectively stateless — it only uses options, not this, so it could be a module-level function. Keeping it on App is fine for cohesion; just noting it could be extracted.

The required-property check in _validateOptions() (src/predevals.js:272–278) checks targets and eval_sets but not task_id_text, which is also accessed later in initialize(). Minor inconsistency, arguably out of scope for this PR.


Suggestions

  1. Document the reset-on-target-change behavior in the README alongside the ascending-only limitation. Users may not expect manual sort state to be cleared on eval-set change.

  2. Warn or guard when _initialSortColumn is absent from the current table in updateTable():

    if (!scoreTableCols.includes(this._initialSortColumn)) {
        console.warn(`initial_sort_column '${this._initialSortColumn}' not in current table columns; defaulting to model_id`);
    }

    This would make the cross-target mismatch visible in the console rather than silently falling back.

  3. Throw Error instead of strings in _validateOptions() for conventional stack-trace behavior — low priority given this is dev-time init only.


Summary

The implementation is clean and correct for the primary use case. The two things worth addressing before merge are the silent cross-target column mismatch and documenting the "sort resets on update" behavior. Everything else is low priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant