fix: style_cell and hover_template lost when sorting in descending order#8915
Open
ManasVardhan wants to merge 2 commits intomarimo-team:mainfrom
Open
fix: style_cell and hover_template lost when sorting in descending order#8915ManasVardhan wants to merge 2 commits intomarimo-team:mainfrom
ManasVardhan wants to merge 2 commits intomarimo-team:mainfrom
Conversation
When sorting by any column, _style_cells() and _hover_cells() generated sequential row IDs (or reversed sequential IDs for descending) that did not match the actual _marimo_row_id values of the displayed rows. This caused the frontend to discard all styles/hover texts because the dict keys did not match its row ID lookups. The fix introduces _get_page_row_ids() which reads actual _marimo_row_id values from the sorted searched manager data for the current page. This ensures style/hover dict keys always match what the frontend expects, regardless of sort column or direction. The descending parameter is removed from _style_cells() and _hover_cells() since it is no longer needed. Also fixes hover_template with sorted data (previously marked xfail) and fixes the pre-existing test bug where sort arg was passed as a single SortArgs instead of a list. Closes marimo-team#8847
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ |
for more information, see https://pre-commit.ci
Author
|
I have read the CLA Document and I hereby sign the CLA |
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.
Summary
Fixes #8847.
style_cellstyles (andhover_templatetexts) were lost when sorting a table in descending order.Root Cause
_style_cells()and_hover_cells()generated row IDs assuming they correlated with the sort order:range(skip, skip + take)range(total - 1 - skip, ..., -1)This only worked when sorting by the index column. When sorting by any other column, the actual
_marimo_row_idvalues on the displayed page are arbitrary and do not follow sequential or reversed-sequential patterns. The frontend looks up styles by_marimo_row_id, so the mismatched keys caused all styles to be silently discarded.Fix
Introduces
_get_page_row_ids()which reads the actual_marimo_row_idvalues from the sorted_searched_managerdata for the current page. This ensures style/hover dict keys always match what the frontend uses for lookup, regardless of sort column or direction.For tables without stable row IDs (list/dict data), positional indices are returned since both backend and frontend use positional indexing.
The
descendingparameter is removed from_style_cells()and_hover_cells()since it is no longer needed.Additional fixes
xfailfromtest_cell_search_df_hover_texts_sorted(hover with sorted data now works)sortwas passed as a bareSortArgsinstead of[SortArgs(...)]Tests
test_cell_styles_descending_non_index_column: 4-row table sorted by Score (not index) descending. Verifies all row IDs present with correct style values.test_cell_styles_sorted_by_value_with_pagination: 20-row table sorted by Value descending with page_size=5. Verifies page 0 contains the correct (non-sequential) row IDs.