fix(pds-table): add default sorting props #633
Conversation
github-actions
bot
added
bug
✅ Deploy Preview for pine-design-system ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR adds initial-sort support to PdsTable via two new public props: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @libs/core/src/components/pds-table/pds-table.tsx:
- Around line 94-123: applyDefaultSort calls sortTable immediately but sortTable
currently assumes a <pds-table-body> exists and will throw if it's missing;
update the sortTable method to first check for the body (e.g.,
this.el.querySelector('pds-table-body')) and return early/no-op if it is null so
callers like applyDefaultSort won't crash when only headers are rendered, and
keep applyDefaultSort's logic that still sets sortingColumn/sortingDirection and
calls setActiveSort on the header cell so the visual state is applied even when
the body is absent.
In @libs/core/src/components/pds-table/stories/pds-table.stories.js:
- Around line 254-264: DefaultSortTemplate is emitting literal "undefined" for
default-sort-column and default-sort-direction when Storybook args are cleared;
import and use the lit ifDefined directive (from lit/directives/if-defined.js)
and wrap args.defaultSortColumn and args.defaultSortDirection with ifDefined in
the pds-table attributes so the attributes are omitted when unset (also add the
ifDefined import at the top of the file and update the attribute bindings in
DefaultSortTemplate).
🧹 Nitpick comments (2)
libs/core/src/components.d.ts (1)
1708-1716: Docstring encourages a brittle contract (“must match text content”).If possible, prefer a stable identifier (e.g.,
columnKey/sortKey) rather than coupling default sorting toinnerText(which can change with i18n, icons, whitespace, visually-hidden text). Even if you keep the current API, consider softening the wording to “matches the column header label” and documenting edge-cases (icons, extra whitespace).Also applies to: 4495-4503
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
332-493: Nice coverage; consider making the “active header” assertion less timing-sensitive.If
setActiveSort()triggers async updates insidepds-table-head-cell, oneawait page.waitForChanges()may be borderline. Consider an additionalawait page.waitForChanges()(or waiting for the class to appear) before assertingtoHaveClass('is-active').
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/core/src/components.d.tslibs/core/src/components/pds-table/docs/pds-table.mdxlibs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.tsxlibs/core/src/components/pds-table/pds-table-head-cell/readme.mdlibs/core/src/components/pds-table/pds-table-head-cell/test/pds-table-head-cell.spec.tsxlibs/core/src/components/pds-table/pds-table.tsxlibs/core/src/components/pds-table/readme.mdlibs/core/src/components/pds-table/stories/pds-table.stories.jslibs/core/src/components/pds-table/test/pds-table.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
libs/core/src/components/pds-table/pds-table-head-cell/test/pds-table-head-cell.spec.tsx (1)
libs/react/src/components/proxies.ts (2)
PdsTableHeadCell(95-95)PdsTable(91-91)
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
libs/react/src/components/proxies.ts (6)
PdsTable(91-91)PdsTableHead(94-94)PdsTableHeadCell(95-95)PdsTableBody(92-92)PdsTableRow(96-96)PdsTableCell(93-93)
🔇 Additional comments (8)
libs/core/src/components/pds-table/readme.md (1)
10-19: Documentation looks good.The properties table accurately documents the new
defaultSortColumnanddefaultSortDirectionprops with correct types, descriptions, and default values.libs/core/src/components/pds-table/pds-table-head-cell/readme.md (1)
23-41: Documentation for newsetActiveSortmethod is clear and complete.The method signature, parameters, and return type are well documented, making it easy for consumers to understand the API.
libs/core/src/components/pds-table/pds-table-head-cell/pds-table-head-cell.tsx (2)
1-1: Import change is correct.The
Methoddecorator import is required for exposingsetActiveSortas a public method on the custom element.
62-73: No issues identified withsetActiveSortimplementation.The method correctly sets initial sort state on load. Unlike
toggleSort(which handles user interaction and must clear previous active states),setActiveSortis called only during initial load when no cells are yet active, so clearing other cells is unnecessary. The design appropriately differentiates between programmatic state setup and user interaction.libs/core/src/components/pds-table/docs/pds-table.mdx (1)
1322-1430: Excellent documentation for the Default Sort feature.The new section clearly explains:
- How to use
defaultSortColumnanddefaultSortDirectionprops- The requirement that
defaultSortColumnmust match the text content of a sortable column header- Both React and web component syntax examples
libs/core/src/components/pds-table/pds-table-head-cell/test/pds-table-head-cell.spec.tsx (1)
441-493: Good test coverage forsetActiveSortmethod.The tests appropriately cover:
- Setting active sort to descending direction
- Setting active sort to ascending direction
- Guard behavior for non-sortable columns
Test assertions verify both the
is-activeclass and direction-specific classes (sort-asc/sort-desc).libs/core/src/components.d.ts (1)
1773-1777:setActiveSort()typing looks good for programmatic activation.libs/core/src/components/pds-table/stories/pds-table.stories.js (1)
27-41: Story controls + DefaultSorting story wiring look good.Also applies to: 387-399
| const DefaultSortTemplate = (args) => html` | ||
| <pds-table | ||
| ?compact=${args.compact} | ||
| component-id="${args.componentId}" | ||
| ?fixed-column=${args.fixedColumn} | ||
| ?responsive=${args.responsive} | ||
| ?row-dividers=${args.rowDividers} | ||
| ?selectable=${args.selectable} | ||
| default-sort-column="${args.defaultSortColumn}" | ||
| default-sort-direction="${args.defaultSortDirection}" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the file exists and check its content around the specified lines
if [ -f "libs/core/src/components/pds-table/stories/pds-table.stories.js" ]; then
echo "File exists"
wc -l "libs/core/src/components/pds-table/stories/pds-table.stories.js"
else
echo "File not found"
fiRepository: Kajabi/pine
Length of output: 134
🏁 Script executed:
# Read the file content around lines 254-264
sed -n '250,270p' "libs/core/src/components/pds-table/stories/pds-table.stories.js"Repository: Kajabi/pine
Length of output: 683
🏁 Script executed:
# Check current imports at the top of the file
head -20 "libs/core/src/components/pds-table/stories/pds-table.stories.js"Repository: Kajabi/pine
Length of output: 546
🏁 Script executed:
# Search for any usage of ifDefined in this file
rg "ifDefined" "libs/core/src/components/pds-table/stories/pds-table.stories.js"Repository: Kajabi/pine
Length of output: 37
🏁 Script executed:
# Search for other instances of defaultSortColumn or defaultSortDirection in the file
rg "defaultSort(Column|Direction)" "libs/core/src/components/pds-table/stories/pds-table.stories.js"Repository: Kajabi/pine
Length of output: 274
🏁 Script executed:
# Check for similar patterns with string interpolation of args that might have undefined values
# Look for patterns like "${args.something}" in the entire file
rg '\$\{args\.' "libs/core/src/components/pds-table/stories/pds-table.stories.js" -A 1 -B 1Repository: Kajabi/pine
Length of output: 1933
🏁 Script executed:
# Check if there are other story templates in this file and how they handle optional attributes
grep -n "Template = " "libs/core/src/components/pds-table/stories/pds-table.stories.js"Repository: Kajabi/pine
Length of output: 279
🏁 Script executed:
# Check the argTypes definitions for defaultSortColumn and defaultSortDirection
sed -n '/defaultSortColumn/,/defaultSortDirection/p' "libs/core/src/components/pds-table/stories/pds-table.stories.js" | head -30Repository: Kajabi/pine
Length of output: 497
🏁 Script executed:
# Check the argTypes for componentId to compare with defaultSort attributes
rg "componentId" "libs/core/src/components/pds-table/stories/pds-table.stories.js" -A 5 -B 2Repository: Kajabi/pine
Length of output: 2733
🏁 Script executed:
# Look for the export default and its args to see default values
sed -n '/export default/,/args:/p' "libs/core/src/components/pds-table/stories/pds-table.stories.js"Repository: Kajabi/pine
Length of output: 13932
Use ifDefined() to avoid emitting default-sort-*="undefined" from Storybook args.
With lit templates, ${args.defaultSortColumn} and ${args.defaultSortDirection} can end up as the literal string "undefined" when Storybook controls are cleared, which feeds invalid values into the component. Use ifDefined() to omit the attribute when unset.
Proposed fix
import { html } from 'lit-html';
+import { ifDefined } from 'lit-html/directives/if-defined.js';
const DefaultSortTemplate = (args) => html`
<pds-table
?compact=${args.compact}
component-id="${args.componentId}"
?fixed-column=${args.fixedColumn}
?responsive=${args.responsive}
?row-dividers=${args.rowDividers}
?selectable=${args.selectable}
- default-sort-column="${args.defaultSortColumn}"
- default-sort-direction="${args.defaultSortDirection}"
+ default-sort-column=${ifDefined(args.defaultSortColumn)}
+ default-sort-direction=${ifDefined(args.defaultSortDirection)}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DefaultSortTemplate = (args) => html` | |
| <pds-table | |
| ?compact=${args.compact} | |
| component-id="${args.componentId}" | |
| ?fixed-column=${args.fixedColumn} | |
| ?responsive=${args.responsive} | |
| ?row-dividers=${args.rowDividers} | |
| ?selectable=${args.selectable} | |
| default-sort-column="${args.defaultSortColumn}" | |
| default-sort-direction="${args.defaultSortDirection}" | |
| > | |
| import { html } from 'lit-html'; | |
| import { ifDefined } from 'lit-html/directives/if-defined.js'; | |
| const DefaultSortTemplate = (args) => html` | |
| <pds-table | |
| ?compact=${args.compact} | |
| component-id="${args.componentId}" | |
| ?fixed-column=${args.fixedColumn} | |
| ?responsive=${args.responsive} | |
| ?row-dividers=${args.rowDividers} | |
| ?selectable=${args.selectable} | |
| default-sort-column=${ifDefined(args.defaultSortColumn)} | |
| default-sort-direction=${ifDefined(args.defaultSortDirection)} | |
| > |
🤖 Prompt for AI Agents
In @libs/core/src/components/pds-table/stories/pds-table.stories.js around lines
254 - 264, DefaultSortTemplate is emitting literal "undefined" for
default-sort-column and default-sort-direction when Storybook args are cleared;
import and use the lit ifDefined directive (from lit/directives/if-defined.js)
and wrap args.defaultSortColumn and args.defaultSortDirection with ifDefined in
the pds-table attributes so the attributes are omitted when unset (also add the
ifDefined import at the top of the file and update the attribute bindings in
DefaultSortTemplate).
There was a problem hiding this comment.
@coderabbitai Thanks for the catch! You're right that lit-html is used. However, ifDefined isn't used anywhere else in our stories, and the DefaultSortTemplate is dedicated to the DefaultSorting story which always has these args set.
There was a problem hiding this comment.
@pixelflips, that makes perfect sense! Since DefaultSortTemplate is dedicated to the DefaultSorting story and those args are always provided, and you're maintaining consistency with the existing pattern used throughout your stories, there's no need to add ifDefined here. Thanks for the clarification about your codebase conventions!
|
@coderabbitai i have addressed the previous comments, can you please review again? |
|
@pixelflips: I'll review the changes now that you've addressed the previous comments. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/core/src/components/pds-table/pds-table.tsx (1)
99-122: Minor: Redundant fallback for direction.Line 100 has a fallback
|| 'asc'butdefaultSortDirectionalready defaults to'asc'in the prop declaration (line 58). This is harmless but slightly redundant.The type assertion at line 119 is verbose but necessary given the dynamic nature of Stencil's
@Method()decorator. IfHTMLPdsTableHeadCellElementis generated withsetActiveSortincomponents.d.ts, consider using it directly for cleaner typing.🔧 Suggested simplification
private async applyDefaultSort() { - const direction = this.defaultSortDirection || 'asc'; + const direction = this.defaultSortDirection;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/core/src/components/pds-table/pds-table.tsxlibs/core/src/components/pds-table/test/pds-table.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
libs/react/src/components/proxies.ts (6)
PdsTable(91-91)PdsTableHead(94-94)PdsTableHeadCell(95-95)PdsTableBody(92-92)PdsTableRow(96-96)PdsTableCell(93-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-lint (18)
- GitHub Check: test-specs (20)
- GitHub Check: test-lint (20)
- GitHub Check: test-specs (22)
- GitHub Check: test-specs (18)
- GitHub Check: test-lint (22)
🔇 Additional comments (9)
libs/core/src/components/pds-table/pds-table.tsx (3)
47-58: LGTM!The new props are well-documented with clear JSDoc comments. The type definitions for
defaultSortColumnanddefaultSortDirectionare appropriate, and the default value fordefaultSortDirectionis sensible.
83-92: LGTM!The conditional invocation of
applyDefaultSort()incomponentDidLoadis clean. The ordering—responsive setup before default sort—is appropriate.
239-243: LGTM!Good defensive guard. This prevents runtime errors when
sortTableis called on a table without a body, which can occur during initial load withdefaultSortColumnset.libs/core/src/components/pds-table/test/pds-table.spec.tsx (6)
332-367: LGTM!Good test coverage for ascending default sort. The test data is well-chosen to verify alphabetical ordering.
369-404: LGTM!Good test for the descending sort direction. Complements the ascending test well.
406-435: LGTM!Good test verifying the visual active state is correctly applied to only the sorted column header.
437-460: LGTM!Good test for the warning path. The spy is correctly set up before page creation to capture warnings during
componentDidLoad, and properly cleaned up withmockRestore().
462-493: LGTM!Important negative test case verifying the component doesn't sort when
defaultSortColumnis not configured.
495-512: LGTM!Good edge case test validating the guard in
sortTable. It correctly verifies that:
- No exception is thrown when the table body is missing
- The header cell visual state is still applied
This ensures graceful degradation when the table structure is incomplete.
pixelflips
force-pushed
the
fix/default-table-sort
branch
from
8b0475a to
63fc2df
Compare
|
@coderabbitai i have also addressed the mentioned nitpick, please perform another review. |
|
@pixelflips: I'll review the changes now that you've addressed the nitpick. Looking forward to seeing the updates! ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
273-278: MissingmockRestore()for console spy.The
consoleWarnSpyis created but never restored, which can leak mock state into subsequent tests and cause flaky behavior.Proposed fix
page.rootInstance.sortTable('NonExistentColumn', 'asc'); expect(consoleWarnSpy).toHaveBeenCalledWith('Column "NonExistentColumn" not found.'); + consoleWarnSpy.mockRestore(); });libs/core/src/components/pds-table/pds-table.tsx (1)
265-274: Potential null pointer if row cells are missing.If a table row has fewer cells than expected (e.g., due to
colspanor malformed markup), thequerySelectorat lines 266-267 may returnnull, causing a crash when accessing.textContent.Proposed fix with null guards
// Sort the rows based on the content of the specified column tableRows.sort((a, b) => { - const valueA = a.querySelector(`pds-table-cell:nth-child(${columnIndex + 1})`).textContent.trim(); - const valueB = b.querySelector(`pds-table-cell:nth-child(${columnIndex + 1})`).textContent.trim(); + const cellA = a.querySelector(`pds-table-cell:nth-child(${columnIndex + 1})`); + const cellB = b.querySelector(`pds-table-cell:nth-child(${columnIndex + 1})`); + const valueA = cellA?.textContent?.trim() ?? ''; + const valueB = cellB?.textContent?.trim() ?? ''; if (direction === 'asc') { return valueA.localeCompare(valueB, undefined, { sensitivity: 'base' }); } else { return valueB.localeCompare(valueA, undefined, { sensitivity: 'base' }); } });
🧹 Nitpick comments (1)
libs/core/src/components/pds-table/pds-table.tsx (1)
87-91: Unawaited async call may cause timing issues.
applyDefaultSort()is anasyncmethod but is called withoutawait. While this works in tests due to Stencil'swaitForChanges(), in production the sort may complete aftercomponentDidLoadreturns, potentially causing race conditions if downstream code expects the table to be sorted immediately after the component loads.Proposed fix
- componentDidLoad() { + async componentDidLoad() { if (this.responsive) { this.setupResponsiveScrolling(); } // Apply default sort if specified if (this.defaultSortColumn) { - this.applyDefaultSort(); + await this.applyDefaultSort(); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/core/src/components/pds-table/pds-table.tsxlibs/core/src/components/pds-table/test/pds-table.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
libs/react/src/components/proxies.ts (6)
PdsTable(91-91)PdsTableHead(94-94)PdsTableHeadCell(95-95)PdsTableBody(92-92)PdsTableRow(96-96)PdsTableCell(93-93)
🔇 Additional comments (4)
libs/core/src/components/pds-table/test/pds-table.spec.tsx (1)
332-512: Solid test coverage for the default sorting feature.The new tests comprehensively cover:
- Ascending and descending default sort on initial load
- Active header cell state verification
- Warning emission when the specified column is not found
- Graceful handling when no
defaultSortColumnis provided- Edge case where table has no body
libs/core/src/components/pds-table/pds-table.tsx (3)
47-58: Well-documented new props with sensible defaults.The new
defaultSortColumnanddefaultSortDirectionprops are clearly documented and follow existing patterns in the component. The default value of'asc'fordefaultSortDirectionis intuitive.
99-123: Clean implementation ofapplyDefaultSort().The method correctly:
- Finds sortable header cells matching the specified column name
- Applies the sort via
sortTable()- Updates internal state
- Activates the visual state on the header cell
- Logs a warning if the column is not found
The type assertion on line 119 is appropriately documented.
242-243: Good defensive guard for missing table body.This early return prevents crashes when sorting is attempted on a table without a body, which aligns with the test case added for this scenario.
|
@coderabbitai addressed previous mentioned issues and nitpicks, please review again. |
|
@pixelflips: I'll review the changes now that you've addressed the previous issues and nitpicks. Thank you for the updates! ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/core/src/components/pds-table/pds-table.tsx (1)
110-116: Potential mismatch between column matching and sorting lookup.The matching logic on line 111 uses trimmed
textContent, butsortTableon line 116 uses the rawthis.defaultSortColumn. Meanwhile,sortTableinternally usesinnerText.trim()(line 256) for its lookup. This could cause subtle mismatches if:
- The prop value has different whitespace than the actual header text
- The header contains nested elements where
textContentandinnerTextdifferConsider using the matched cell's trimmed text content for the
sortTablecall to ensure consistency:Suggested improvement
if (matchingCell) { + const columnName = (matchingCell.textContent ?? '').trim(); // Sort the table data - this.sortTable(this.defaultSortColumn, direction); - this.sortingColumn = this.defaultSortColumn; + this.sortTable(columnName, direction); + this.sortingColumn = columnName; this.sortingDirection = direction;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/core/src/components/pds-table/pds-table.tsx
🔇 Additional comments (3)
libs/core/src/components/pds-table/pds-table.tsx (3)
47-58: LGTM!The new props are well-documented with clear JSDoc comments explaining their purpose and relationship. The typing is correct, and the default value for
defaultSortDirectionis appropriate.
83-94: LGTM!The promise rejection handling is correct. Using
voidwith.catch()properly handles the async call without leaving an unhandled promise rejection, and the warning provides useful debugging information.
241-246: LGTM!Good defensive addition. The early return guard prevents potential null reference errors when the table body is missing, which could happen during component initialization or with malformed table structures.
|
@coderabbitai addressed the last nitpick, please review again. |
|
@pixelflips: Thank you for addressing the last nitpick! I'll review the changes now. ✅ Actions performedReview triggered.
|
Description
Add
defaultSortColumnanddefaultSortDirectionprops topds-tableto allow tables to render with a pre-sorted column on initial load.Fixes DSS-70
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: