Datatables - #11
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a datatable subsystem: type-safe Column and ColumnFilter models with FilterState and SortDirection; TableDef builder and derivation macros; runtime TableState with TableUpdate messages, CSV export; Tyrian renderers (Bulma/Bootstrap/Raw); examples, tests, snapshot tooling, and docs. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as UI/Component
participant State as TableState
participant Renderer as Renderer
participant Browser as Browser
User->>UI: interact (filter / sort / page / select / export)
UI->>UI: create TableUpdate message
UI->>State: update(TableUpdate)
State->>State: apply filtering, sorting, paging, selection
State-->>UI: return updated TableState
UI->>Renderer: renderTable(state)
Renderer->>State: read displayData & definition.columns
Renderer-->>UI: Html[TableUpdate]
UI->>Browser: mount HTML
Browser->>User: updated view
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@DATATABLE_IMPLEMENTATION_PLAN.md`:
- Around line 997-1000: The code references a non-existent TableExport.toJSON in
the Msg.ExportJSON handler; either implement toJSON on the TableExport object
(mirroring toCSV/selectedToCSV logic to serialize model.tableState into a JSON
string or blob and trigger download) or remove/replace the Msg.ExportJSON
branch. Locate the Msg.ExportJSON case and the TableExport object (which
contains toCSV and selectedToCSV) and either add a toJSON(model.tableState)
method that returns the JSON export payload and any filename/headers, or delete
the call and corresponding UI/Msg.ExportJSON usage so only supported exports
remain.
- Around line 1063-1070: Add an exportable flag and noExport helper on the
Column type and wire it into the exportableColumns logic: in the Column class
add a boolean field (exportable = true by default) and implement a noExport
method that returns a new/modified Column with exportable set to false; update
any code that computes exportableColumns (e.g., TableDefBuilder or
TableDef.exportableColumns) to filter columns by column.exportable; ensure
method/field names match Column and TableDefBuilder/TableDef symbols used in the
diff so .noExport calls in user docs compile.
- Around line 576-603: The toCSV function references
state.definition.exportableColumns which doesn't exist; fix by adding export
support to TableDef and Column: add an exportable: Boolean field (default true)
and a noExport helper to the Column case class, then implement
TableDef.exportableColumns to return columns.filter(_.exportable). Update
references in toCSV (and any other exporters) to use
state.definition.exportableColumns; look for symbols Column, TableDef,
exportableColumns, toCSV, and noExport to place the changes.
- Around line 175-177: The MultiSelectFilter.matches method references an
undefined identifier `selected`; update the method to use the selected set from
the passed State (i.e., replace `selected` with `state.selected`) so it reads
something like checking membership via `state.selected.contains(render(value))`;
locate this in the MultiSelectFilter class's matches method and ensure there are
no other references to the undefined variable.
- Around line 188-196: In DateRangeFilter.matches, replace the undefined local
references to from and to with the fields on the passed State (i.e., use
state.from and state.to) so the method checks the filter bounds from the
provided State; update the checks that compute afterFrom and beforeTo to call
state.from.forall(...) and state.to.forall(...) respectively inside the matches
function of DateRangeFilter.
- Around line 146-150: The TextFilter.matches method incorrectly references an
undefined variable `search`; update matches (in class TextFilter) to extract the
search string from the passed-in state (which is a FilterState.TextValue)
instead of using `search` — e.g., pattern-match or cast `state` to
FilterState.TextValue to get its `value`, then use that extracted string (apply
toLowerCase when caseInsensitive) with the rendered value from render(value) to
perform the contains check.
- Around line 237-245: CustomFilter lacks the required type member for the
Filter trait: add a type alias binding the generic S to the member type so the
case class satisfies the trait contract; i.e., inside CustomFilter declare the
type member (type State = S) so methods like matches use the declared State type
and the class conforms to ColumnFilter/FilterState expectations.
- Around line 163-165: SelectFilter.matches currently uses an undefined variable
selected; update the method to reference state.selected (an Option) and compare
safely to the rendered value produced by render(value). Specifically, inside
SelectFilter.matches, fetch state.selected and handle the Option (e.g., pattern
match or use map/contains) to compare the rendered value (render(value)) with
the selected value only when present, returning false when state.selected is
None.
- Around line 965-984: The code calls .withTitle("Employee Directory") on
TableDef but TableDef lacks a title field and TableDefBuilder has no withTitle;
add an optional title: Option[String] to the TableDef data structure (and its
constructor/fields) and implement a fluent builder method
TableDefBuilder.withTitle(title: String): TableDefBuilder[...] that stores the
title on the builder and ensures build(...) populates the TableDef.title; update
any creation paths (e.g., the build(...) call used by withSelection) so the
resulting TableDef includes the provided title.
- Around line 202-218: NumberRangeFilter is missing its State type alias and the
matches parameter is using the concrete FilterState type; add a type declaration
(e.g. type State = FilterState.NumberRangeValue) to NumberRangeFilter and change
the matches signature to use that State (matches(value: V, state: State):
Boolean), then adjust the pattern match to match on
FilterState.NumberRangeValue(min, max) as before so the implementation remains
the same but types align with other filters.
- Around line 223-232: BooleanFilter is missing an explicit type alias for its
state and the matches method uses the concrete FilterState instead of the filter
instance's State type; add "type State = FilterState" (or the correct state
subtype) to the BooleanFilter declaration and change the matches signature to
"matches(value: V, state: State): Boolean" so the class uses its own State alias
consistently (referencing BooleanFilter, type State, and matches).
- Around line 849-916: Update the TableRenderer implementations to include
comprehensive accessibility: ensure the TableRenderer trait implementations
(e.g., RawTableRenderer, BulmaTableRenderer, BootstrapTableRenderer) render
semantic table HTML (use <table>, <caption>, <th scope="col">), expose sort
state via aria-sort on header controls and make sortable headers
keyboard-focusable buttons, provide visible or programmatic labels
(aria-label/aria-labelledby) for all filter inputs rendered by
renderFilterInput, ensure pagination and filter controls are keyboard operable
and reachable via Tab, add ARIA live regions (aria-live or role="status")
updated by renderInfo/renderFilters to announce result counts and changes, avoid
relying on color alone for state (include text/icons), and update
implementations to maintain predictable focus behavior during dynamic updates;
follow WAI-ARIA Authoring Practices and WCAG 2.1 when updating RawTableRenderer,
BulmaTableRenderer, BootstrapTableRenderer and the methods renderTable,
renderHeader, renderBody, renderFilters, renderPagination, renderInfo,
renderExportControls, and renderFilterInput.
In `@forms4s-core/src/main/scala/forms4s/datatable/TableExport.scala`:
- Around line 69-73: The escapeCSV function hardcodes checking for commas so
values containing a custom delimiter (e.g., ";") are not escaped; change
escapeCSV(value: String) to accept the configured delimiter (e.g.,
escapeCSV(value: String, delimiter: Char)) and update its logic to check for
value.contains(delimiter) in addition to quotes and newlines, adjust the
replacement/quoting behavior to use the same delimiter-aware check, and update
all call sites (including toCSV) to pass the delimiter through so the CSV export
correctly escapes values for any configured delimiter.
In `@forms4s-core/src/main/scala/forms4s/datatable/TableState.scala`:
- Around line 181-185: selectedItems currently looks up indices against
filteredData but selection indices (and renderBody's globalIdx) are into the
filtered+sorted view; change selectedItems to use the sorted view instead. In
TableState.selectedItems, replace the lookup over filteredData with lookups into
sortedData (the filtered+sorted view used by renderBody/globalIdx) so
selection.toVector.sorted.flatMap(i => sortedData.lift(i)) is used (keeping the
same selection/ordering behavior).
- Around line 163-164: The SelectAll branch currently builds selection as (0
until totalFilteredItems) which are filtered-list positions, but the selection
model expects global indices; change TableUpdate.SelectAll to compute the set of
global indices of the currently filtered/visible rows and store those globals in
the selection. Concretely, locate the code/path that produces the filtered view
(the same logic used by renderBody where globalIdx = state.page.offset + idx or
any filteredRows/visibleRows list), map those visible rows to their original
data indices (e.g., via zipWithIndex on data or using the existing
filteredIndices collection) and call copy(selection =
visibleGlobalIndices.toSet) so selection contains global indices, not 0..N-1.
♻️ Duplicate comments (1)
DATATABLE_IMPLEMENTATION_PLAN.md (1)
609-627: Reference to undefinedexportableColumnsmethod.Line 610 also references
state.definition.exportableColumns, which is not defined. See the previous comment for suggested fixes.
🟡 Minor comments (11)
DATATABLE_IMPLEMENTATION_PLAN.md-1151-1152 (1)
1151-1152: Remove extraneous fenced code block marker.Line 1152 contains a solo closing code fence (```) that doesn't match an opening fence. This appears to be a typo and should be removed.
Based on static analysis findings.
🐛 Proposed fix
case Msg.TableMsg(msg) => model.copy(tableState = model.tableState.update(msg)) -```</details> </blockquote></details> <details> <summary>forms4s-core/src/main/scala/forms4s/datatable/derivation/TableDerivationMacros.scala-11-45 (1)</summary><blockquote> `11-45`: **Reject nested selectors in `extractFieldNameImpl` at compile time.** The current code accepts nested selectors like `_.address.street` and extracts only the leaf field name ("street"), which won't match the derived column id. Add a guard to restrict `Select` to direct field access only: <details> <summary>Guard against nested selectors</summary> ```diff def extractFromTerm(term: Term): Option[String] = term match { - case Select(_, fieldName) => Some(fieldName) + case Select(Ident(_), fieldName) => Some(fieldName) + case Select(This(_), fieldName) => Some(fieldName) + case Select(_, _) => None // nested select; reject case Inlined(_, _, inner) => extractFromTerm(inner) case Block(_, inner) => extractFromTerm(inner) case Lambda(_, body) => extractFromTerm(body) case _ => None }forms4s-examples/src/test/scala/forms4s/testing/SnapshotTest.scala-26-39 (1)
26-39: Redundant path resolution intestSnapshot.Line 29 resolves the path again when
filePathis already computed on line 27.Proposed fix
def testSnapshot(content: String, path: String): Unit = { val filePath = testResourcesPath.resolve(path) val existingOpt = Option.when(Files.exists(filePath)) { - Files.readString(testResourcesPath.resolve(path)) + Files.readString(filePath) } val isOk = existingOpt.contains(content) if (!isOk) { Files.createDirectories(filePath.getParent) Files.writeString(filePath, content) Assertions.fail(s"Snapshot $path was not matching. A new value has been written to $filePath.") } }forms4s-examples/src/test/scala/forms4s/example/datatable/DatatableSnapshotTest.scala-77-83 (1)
77-83: Raw string literal doesn't produce actual newline character.In the
"All Combined"test case,\ninside a raw string literal ("""...""") is interpreted as two literal characters (backslash + n), not a newline. This means the test doesn't validate newline escaping as intended.Proposed fix
Use string concatenation or regular string escaping:
- Record("All Combined", """Comma, Quote", Newline\n"""), + Record("All Combined", "Comma, Quote\", Newline\n"),Or use string interpolation with explicit newline:
- Record("All Combined", """Comma, Quote", Newline\n"""), + Record("All Combined", s"""Comma, Quote", Newline${"\n"}"""),forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BulmaTableRenderer.scala-176-199 (1)
176-199: SametoIntissue as in other renderers.Line 184 uses
v.toIntwhich can throw. Apply the same defensive fix.forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BootstrapTableRenderer.scala-295-330 (1)
295-330: SameLocalDate.parseissue as in RawTableRenderer.Lines 309 and 325 use
java.time.LocalDate.parse(v)without error handling. Apply the sameTry(...).toOptionpattern.forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BulmaTableRenderer.scala-316-357 (1)
316-357: SameLocalDate.parseissue as in other renderers.Lines 331 and 351 use
java.time.LocalDate.parse(v)without error handling.forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/RawTableRenderer.scala-105-116 (1)
105-116: Potential runtime exception on invalid page size input.
v.toInton line 107 will throw aNumberFormatExceptionif the user manipulates the DOM or the value is somehow invalid. Consider usingtoIntOptionwith a fallback.🛡️ Defensive fix
Html.select( - onChange(v => TableUpdate.SetPageSize(v.toInt)) + onChange(v => TableUpdate.SetPageSize(v.toIntOption.getOrElse(state.page.pageSize))) )(forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/RawTableRenderer.scala-119-131 (1)
119-131: Info text displays "Showing 1 to 0" when data is empty.When
totalFilteredItemsis 0,startbecomes 1 (offset 0 + 1), butendis 0, resulting in "Showing 1 to 0 of 0 entries". Consider handling the empty case explicitly.🐛 Proposed fix
override def renderInfo[T](state: TableState[T]): Html[TableUpdate] = { - val start = state.page.offset + 1 + val start = if (state.totalFilteredItems == 0) 0 else state.page.offset + 1 val end = math.min(state.page.offset + state.page.pageSize, state.totalFilteredItems)forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BootstrapTableRenderer.scala-167-183 (1)
167-183: SametoIntissue as in RawTableRenderer.Line 172 uses
v.toIntwhich can throw on invalid input. Apply the same defensive fix suggested for RawTableRenderer.🛡️ Defensive fix
Html.select( className := "form-select form-select-sm", - onChange(v => TableUpdate.SetPageSize(v.toInt)) + onChange(v => TableUpdate.SetPageSize(v.toIntOption.getOrElse(state.page.pageSize))) )(forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/RawTableRenderer.scala-223-256 (1)
223-256: UncaughtDateTimeParseExceptionon invalid date input.
java.time.LocalDate.parse(v)on lines 236 and 251 can throw if the browser sends an unexpected format. Wrap in aTryor use a safe parsing method.🛡️ Defensive fix for date parsing
+import scala.util.Try + +// In renderFilterInput, DateRange case: onInput(v => TableUpdate.SetFilter( column.id, FilterState.DateRangeValue( - if (v.isEmpty) None else Some(java.time.LocalDate.parse(v)), + if (v.isEmpty) None else Try(java.time.LocalDate.parse(v)).toOption, toVal ) ) )
🧹 Nitpick comments (11)
website/README.md (1)
21-35: Consider cleaningexample-distbefore copying new assets.This prevents stale assets from lingering between builds.
♻️ Suggested doc tweak
# 2. Copy built assets to website mkdir -p ../website/static/example-dist/ +rm -rf ../website/static/example-dist/* cp dist/* ../website/static/example-dist/Also applies to: 37-37
DATATABLE_IMPLEMENTATION_PLAN.md (3)
9-33: Add language specifier to fenced code block.The fenced code block should specify a language for proper syntax highlighting. Since this is a file structure tree, you can use
textas the language specifier.Based on static analysis findings.
📝 Proposed fix
-``` +```text forms4s-core/src/main/scala/forms4s/datatable/ ├── Column.scala # Column definition with flexible configuration
358-375: Consider safer alternative to type casting in filter matching.Line 370 uses
asInstanceOf[ColumnFilter[Any]]to work around type erasure. While this will likely work in practice, it's not type-safe and could cause runtime errors if the filter's state type doesn't match.Consider restructuring the filter matching to avoid the cast. One approach is to have the
Columnclass handle the filtering internally:💡 Suggested refactor
Add a method to
Column:case class Column[T, V]( // ... existing fields ... ) { // ... existing methods ... def matchesFilter(row: T, filterState: FilterState): Boolean = { filter match { case None => true case Some(f) => val value = extract(row) f.matches(value, filterState.asInstanceOf[f.State]) } } }Then in
filteredData:definition.columns.forall { col => filters.get(col.id) match { case None | Some(state) if state.isEmpty => true case Some(state) => col.matchesFilter(row, state) } }
378-397: Consider safer alternative to type casting in sorting.Line 387 uses
asInstanceOf[Ordering[Any]]which could lead to runtime errors. Consider encapsulating the sorting logic within theColumnclass to maintain type safety.💡 Suggested refactor
Add a method to
Column:case class Column[T, V]( // ... existing fields ... ) { // ... existing methods ... def compareRows(row1: T, row2: T): Int = { sortBy match { case Some(ord) => ord.compare(extract(row1), extract(row2)) case None => render(extract(row1)).compareTo(render(extract(row2))) } } }Then in
sortedData:val sorted = filteredData.sortWith((r1, r2) => col.compareRows(r1, r2) < 0)forms4s-core/src/main/scala/forms4s/datatable/derivation/TableDerivationMacros.scala (1)
88-89: Optional: preserve acronyms in labels.
capitalizelowercases the rest of the string (e.g.,URLValue→Urlvalue). If you want to preserve acronyms, consider title-casing each word without lowercasing.♻️ Possible tweak
- private def camelToTitle(s: String): String = - s.replaceAll("([a-z])([A-Z])", "$1 $2").capitalize + private def camelToTitle(s: String): String = + s + .replaceAll("([a-z])([A-Z])", "$1 $2") + .split(" ") + .filter(_.nonEmpty) + .map(w => w.head.toUpper + w.tail) + .mkString(" ")forms4s-core/src/main/scala/forms4s/datatable/FilterState.scala (1)
35-40: Prefervalfor empty instances to avoid repeated allocations.
These are immutable constants; usingvalmakes intent explicit and avoids new instances per call.♻️ Proposed tweak
- def emptyText: TextValue = TextValue("") - def emptySelect: SelectValue = SelectValue(None) - def emptyMultiSelect: MultiSelectValue = MultiSelectValue(Set.empty) - def emptyDateRange: DateRangeValue = DateRangeValue(None, None) - def emptyNumberRange: NumberRangeValue = NumberRangeValue(None, None) - def emptyBoolean: BooleanValue = BooleanValue(None) + val emptyText: TextValue = TextValue("") + val emptySelect: SelectValue = SelectValue(None) + val emptyMultiSelect: MultiSelectValue = MultiSelectValue(Set.empty) + val emptyDateRange: DateRangeValue = DateRangeValue(None, None) + val emptyNumberRange: NumberRangeValue = NumberRangeValue(None, None) + val emptyBoolean: BooleanValue = BooleanValue(None)forms4s-core/src/main/scala/forms4s/datatable/TableExport.scala (1)
49-67: Consider extracting shared CSV generation logic.
selectedToCSVduplicates most of the row-rendering logic fromtoCSV. A private helper could reduce duplication.Suggested refactor
+ private def buildCSV[T]( + columns: List[Column[T, ?]], + data: Vector[T], + includeHeaders: Boolean, + delimiter: String + ): String = { + val sb = new StringBuilder + if (includeHeaders) { + sb.append(columns.map(c => escapeCSV(c.label, delimiter)).mkString(delimiter)) + sb.append("\n") + } + data.foreach { row => + val values = columns.map { col => + val value = col.extract(row) + escapeCSV(col.render(value), delimiter) + } + sb.append(values.mkString(delimiter)) + sb.append("\n") + } + sb.toString + } def selectedToCSV[T](state: TableState[T], delimiter: String = ","): String = - val columns = state.definition.columns - val data = state.selectedItems - ... + buildCSV(state.definition.columns, state.selectedItems, includeHeaders = true, delimiter)forms4s-examples/src/main/scala/forms4s/example/docs/DatatableTyrianExample.scala (1)
34-34: Router always clears filters on any navigation.The router emits
ClearAllFiltersfor every location change, which seems unintentional for a documentation example. Consider using a no-op or documenting this as a placeholder pattern.Alternative approaches
If no routing is needed:
def router: Location => TableMsg = _ => TableMsg.Update(TableUpdate.SetPage(0)) // no-op effectivelyOr add a comment explaining this is a simplified example:
// In a real app, parse location and dispatch appropriate updates def router: Location => TableMsg = _ => TableMsg.Update(TableUpdate.ClearAllFilters)forms4s-core/src/main/scala/forms4s/datatable/derivation/TableDefBuilder.scala (2)
62-72: Consider adding validation for unknown column IDs in exclusions/renames/modifications.If a user specifies an exclusion, rename, or modification for a field that doesn't exist (e.g., typo in the lambda), the builder silently ignores it. Consider logging a warning or providing a strict mode that fails on unknown column IDs.
💡 Optional: Add validation in build()
inline def build(id: String): TableDef[T] = { val baseColumns = deriveColumnsWithMacro[T] + val columnIds = baseColumns.map(_.id).toSet + val unknownExclusions = exclusions -- columnIds + val unknownRenames = renames.keySet -- columnIds + val unknownModifications = modifications.keySet -- columnIds + if (unknownExclusions.nonEmpty || unknownRenames.nonEmpty || unknownModifications.nonEmpty) { + // Could log warning or throw in strict mode + } val filteredColumns = baseColumns.filterNot(c => exclusions.contains(c.id)) // ... rest unchanged }
40-43: Type erasure cast is acceptable but warrants documentation.The
asInstanceOf[Column[T, ?] => Column[T, ?]]cast is necessary to store the modification function in a type-erased map. This is safe because the macro ensures the correct field type at the call site, but a brief inline comment would help future maintainers understand why this is safe.forms4s-core/src/main/scala/forms4s/datatable/TableState.scala (1)
39-58: Consider memoizingfilteredDatato avoid repeated recomputation.
filteredDatais called multiple times per render cycle (viasortedData,totalFilteredItems,totalPages,uniqueValuesFor,selectedItems). For large datasets, this O(n*m) operation (n rows, m columns) could be expensive. Consider caching the result or making it a lazy val in a wrapper.Since
TableStateis a case class and immutable, the filtered data is stable per instance. A pattern like computing it once and storing it could help, though it changes the data model.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@forms4s-core/src/main/scala/forms4s/datatable/ColumnFilter.scala`:
- Around line 53-66: SelectFilter currently accepts includeBlank but never uses
it; either remove the parameter or propagate it into filtering/rendering so it
controls the "All"/blank option. If you choose to keep it, update SelectFilter
to store includeBlank and use it when producing the empty state and option list
(e.g., change emptyState from FilterState.emptySelect to a Select-specific empty
state that captures includeBlank, and ensure the UI/renderer that builds option
lists for SelectFilter reads SelectFilter.includeBlank to include or omit the
blank/"All" option), while leaving matches logic (matches and
FilterState.SelectValue handling) unchanged; alternatively remove the
includeBlank parameter and any references to it to avoid a misleading API.
In
`@forms4s-examples/src/main/scala/forms4s/example/docs/DatatableTyrianExample.scala`:
- Around line 17-18: Replace the placeholder definitions for tableDef:
TableDef[Employee] and data: Vector[Employee] (which currently use ??? and will
throw NotImplementedError at runtime) with concrete values — e.g., assign
tableDef to the sample TableDef used elsewhere in the example (sampleTable or
similar) and assign data to the sample Vector of Employee instances
(sampleEmployees or similar) so init can consume real data; locate the
definitions of tableDef and data in DatatableTyrianExample.scala and wire them
to the existing sample table/data symbols used in the examples.
In `@forms4s-examples/src/main/scala/forms4s/example/TyrianExample.scala`:
- Around line 90-98: The tab anchors in renderTabs are non-focusable anchors
without href; replace the a(...) elements with button elements (e.g.,
button(type := "button", className := "...")(...)) so the Tab controls are
keyboard-focusable and announce as interactive controls; keep the existing
onClick handlers (Msg.SwitchTab(Tab.Forms) / Msg.SwitchTab(Tab.Datatable)),
preserve the is-active class logic on the li and transfer any visual classes
(adjust to something like "is-ghost" or other button styling) to the button to
maintain appearance, and ensure the semantics still reference Tab and
Msg.SwitchTab as before.
In
`@forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BootstrapTableRenderer.scala`:
- Around line 237-262: The checkbox id generation in
BootstrapTableRenderer.scala's FilterType.MultiSelect uses raw values (id :=
s"${column.id}-$opt") which can produce invalid or colliding HTML ids; change
this to produce HTML-safe, unique ids by either sanitizing opt (e.g.,
slugify/remove unsafe chars) or append a stable index from options/map (use
options.zipWithIndex and id := s"${column.id}-${idx}") and ensure the label
htmlFor uses the same generated id; update references to column.id, opt,
options, selectedValues and TableUpdate.SetFilter accordingly.
- Around line 106-131: The pagination uses anchor elements with href="#" which
can cause unwanted navigation; update renderPagination and
renderPaginationNumbers to use <button> elements (styled with Bootstrap's
"page-link" class) instead of a(...) anchors, wiring the same onClick handlers
(TableUpdate.PrevPage, TableUpdate.NextPage and the page-number updates) so
clicks no longer trigger default browser navigation—mirror the approach used in
RawTableRenderer and apply the same change in BulmaTableRenderer.
In
`@forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BulmaTableRenderer.scala`:
- Around line 73-81: The interactive elements in BulmaTableRenderer are
currently non-semantic (th/a/tr/span) with click-only handlers; replace them
with proper interactive controls: for sortable headers (where
TableUpdate.ToggleSort(col.id) is used) render a button inside the th (or
replace the th content with a button) and keep the onClick on that button, add
aria-sort on the th and aria-pressed/aria-label on the button; for pagination
links replace anchor elements with buttons and keep their onClick handlers; for
row selection move the onClick from tr into a checkbox input or a dedicated
selection button column (update the row rendering function that attaches the
selection handler) so keyboard users can toggle selection; for multi-select tags
replace clickable spans with buttons and preserve existing handler logic. Also
ensure visual styles (cursor, user-select) are transferred to the new buttons
and add appropriate aria attributes (aria-label, role if needed) so behavior and
a11y are preserved.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@forms4s-core/src/main/scala/forms4s/datatable/TableState.scala`:
- Around line 89-91: The selection set currently mixes two index spaces:
ToggleRowSelection handlers use globalIdx = page.offset + idx into sortedData
while SelectAll and selectedItems use indices into the original data vector
(filteredIndices and data.lift), causing wrong selections; fix by normalizing
all selection indices to the original data index space in TableState — update
ToggleRowSelection (and any renderer logic that computes globalIdx from
page.offset + idx) to map the rendered row to its original data index (e.g., via
a mapping from sortedData positions to original data indices stored on
TableState or by using data.indexOf(row) when available), and ensure SelectAll,
filteredIndices, selectedItems, and any methods that read/write the selection
set (refer to filteredIndices, sortedData, selectedItems, ToggleRowSelection,
SelectAll) all add/remove indices in that same original-data index space.
In
`@forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BootstrapTableRenderer.scala`:
- Around line 306-314: The current onInput handler calls
java.time.LocalDate.parse(v) directly (inside TableUpdate.SetFilter creating
FilterState.DateRangeValue), which can throw DateTimeParseException for
malformed strings; update the onInput logic in the DateRange handlers (the block
that builds TableUpdate.SetFilter / FilterState.DateRangeValue) to defensively
parse the string into an Option[LocalDate] by catching/parsing failures (e.g.
wrap parse in a try/catch or use scala.util.Try/Option and map to None on
failure) for both the start and end date branches so no exception is thrown on
invalid input and the filter receives None instead of crashing.
In
`@forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BulmaTableRenderer.scala`:
- Around line 330-337: BulmaTableRenderer currently calls
java.time.LocalDate.parse(v) inside the onInput handler when building
TableUpdate.SetFilter with FilterState.DateRangeValue, which can throw on
malformed input; update the handler to defensively parse user input by wrapping
LocalDate.parse(...) in scala.util.Try(...).toOption (for both the "from" and
"to" values) and pass the resulting Option[LocalDate] into
FilterState.DateRangeValue; apply the same defensive change to the similar
parsing occurrences around the other block (lines referenced in the review,
e.g., the 349-356 section) so parsing never throws.
♻️ Duplicate comments (1)
forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BulmaTableRenderer.scala (1)
73-81: Sortable headers and MultiSelect tags still lack keyboard accessibility.As noted in a previous review, sortable column headers (using
thwithonClick) and MultiSelect tag chips (usingspanwithonClick) are not keyboard-accessible. Consider wrapping interactive content inbuttonelements.Also applies to: 263-276
🧹 Nitpick comments (2)
forms4s-core/src/main/scala/forms4s/datatable/TableState.scala (2)
37-56: Duplicated filter-matching logic betweenfilteredDataandfilteredIndices.The filter-matching predicate is duplicated verbatim in both
filteredData(lines 41-55) andfilteredIndices(lines 97-112). Consider extracting a sharedmatchesAllFilters(row: T): Booleanhelper to eliminate duplication and reduce maintenance burden.♻️ Suggested refactor
+ private def matchesAllFilters(row: T): Boolean = + definition.columns.forall { col => + col.filter match { + case None => true + case Some(filter) => + filters.get(col.id) match { + case None => true + case Some(state) if state.isEmpty => true + case Some(state) => + val value = col.extract(row) + filter.asInstanceOf[ColumnFilter[Any]].matches(value, state) + } + } + } + /** Apply all filters to the data */ def filteredData: Vector[T] = { if (filters.isEmpty || filters.values.forall(_.isEmpty)) data - else - data.filter { row => - definition.columns.forall { col => - col.filter match { - case None => true - case Some(filter) => - filters.get(col.id) match { - case None => true - case Some(state) if state.isEmpty => true - case Some(state) => - val value = col.extract(row) - filter.asInstanceOf[ColumnFilter[Any]].matches(value, state) - } - } - } - } + else data.filter(matchesAllFilters) }Also applies to: 94-113
37-91: Consider caching derived data views for large datasets.
filteredData,sortedData, andpagedDataare alldefmethods that recompute on every access. In a render cycle that accessestotalFilteredItems,totalPages, anddisplayData, filtering/sorting may run multiple times. For small datasets this is fine, but for larger tables consider memoization orlazy val(with the trade-off of holding references).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/RawTableRenderer.scala`:
- Around line 221-254: In FilterType.DateRange rendering (inside
RawTableRenderer), avoid calling java.time.LocalDate.parse(v) directly because
it can throw DateTimeParseException; instead, defensively parse both the "from"
and "to" inputs used to build FilterState.DateRangeValue for
TableUpdate.SetFilter (referenced by column.id and FilterState.DateRangeValue)
by catching DateTimeParseException (or using a safe parse helper) and treating
malformed/non-parseable values as None before creating the
FilterState.DateRangeValue so the component never throws on bad input.
♻️ Duplicate comments (2)
forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BulmaTableRenderer.scala (1)
318-359: Date parsing can throw on malformed input.Same issue as in
BootstrapTableRendererandRawTableRenderer:LocalDate.parse(v)will throw on malformed input.🛡️ Defensive parsing suggestion
onInput(v => TableUpdate.SetFilter( column.id, FilterState.DateRangeValue( - if (v.isEmpty) None else Some(java.time.LocalDate.parse(v)), + if (v.isEmpty) None else scala.util.Try(java.time.LocalDate.parse(v)).toOption, toVal, ), ), ),Apply the same fix to both date inputs (lines 333 and 353).
forms4s-tyrian/src/main/scala/forms4s/tyrian/datatable/BootstrapTableRenderer.scala (1)
295-330: Date parsing can throw on malformed input.
LocalDate.parse(v)will throwDateTimeParseExceptionif the input is non-empty but malformed. This was flagged in a previous review and remains unaddressed.🛡️ Defensive parsing suggestion
onInput(v => TableUpdate.SetFilter( column.id, FilterState.DateRangeValue( - if (v.isEmpty) None else Some(java.time.LocalDate.parse(v)), + if (v.isEmpty) None else scala.util.Try(java.time.LocalDate.parse(v)).toOption, toVal, ), ), ),Apply the same fix to both date inputs (lines 309 and 325).
🧹 Nitpick comments (1)
forms4s-core/src/test/scala/forms4s/datatable/TableExportSpec.scala (1)
88-95: Consider a more robust assertion for newline escaping test.Using
split("\n")on CSV containing embedded newlines and then checkingstartsWithis fragile. A regex or direct string check would be more reliable.💡 Alternative assertion
"escapes newlines in values" in { val dataWithNewline = Vector(Person("Alice\nBob", "New York", 50000)) val stateWithNewline = TableState(tableDef, dataWithNewline) val csv = TableExport.toCSV(stateWithNewline) - val lines = csv.split("\n") - // Value with newline should be quoted - assert(lines(1).startsWith("\"Alice")) + // Value with newline should be quoted - check the raw CSV contains the quoted field + assert(csv.contains("\"Alice\nBob\"")) }
Merged
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.
Solves #9
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.