Skip to content

Conversation

@PrzemyslawKlys
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings December 14, 2025 18:16
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request updates the DataTables ColumnHighlighter JavaScript library to version 0.1.10 and adds comprehensive date/time parsing and formatting support. The changes improve compatibility with PowerShell/.NET date formats, add Bootstrap 5.3 table striping support, enhance column name matching tolerance, and improve locale-aware number parsing.

Key Changes

  • Enhanced date/time parsing with support for .NET/PowerShell format tokens (dd/MM/yyyy, etc.) and automatic conversion to Moment.js formats, with fallback parsing when Moment.js is unavailable
  • Improved number parsing to handle both comma and dot decimal separators for international locale support
  • Fixed Bootstrap 5.3 table striping compatibility by using box-shadow overlays instead of simple background colors
  • Added tolerant column name matching that ignores case, spacing, and punctuation differences

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
Resources/JS/dataTables.columnHighlighter.js Complete rewrite with new date parsing functions (dotNetToMomentFormat, parseDateByFormatHint, parseDateValue), locale-aware number parsing (parseLocaleNumber), tolerant column matching (normalizeName), and Bootstrap 5.3 compatibility fixes for cell highlighting
Private/Parameters.Configuration.ps1 Updated CDN link from version 0.1.2 to 0.1.10 and removed trailing whitespace
Examples/Example-Table/Example-TableDateTimeConditionsFormats.ps1 New example demonstrating date comparisons with dd/MM/yyyy format in both HTML and JavaScript DataStore modes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +25 to +28
New-HTMLTableOption -DataStore JavaScript -DateTimeFormat 'dd/MM/yyyy HH:mm:ss'
New-HTMLTable -DataTable $AdminsDisabled -HideFooter -DisablePaging -DateTimeSortingFormat 'DD/MM/YYYY HH:mm:ss' {
New-HTMLTableHeader -Title 'JavaScript: Highlight RefreshTokenDate < threshold'
New-HTMLTableCondition -Name 'RefreshTokenDate' -ComparisonType 'date' -Operator lt -Value $DateDeleteCheck -BackgroundColor Red -Color Black -FailBackgroundColor LightGreen -FailColor Black -DateTimeFormat 'dd/MM/YYYY HH:mm:ss'
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same inconsistency in date format case conventions as lines 17-20. The New-HTMLTableOption uses lowercase tokens (dd/MM/yyyy) while New-HTMLTable and New-HTMLTableCondition use uppercase tokens (DD/MM/YYYY). Consider standardizing on one convention for clarity.

Copilot uses AI. Check for mistakes.
if (nums.length > 6) {
// Use the first 3 digits of the next group as milliseconds (covers 3/6/7 digit fractions from .NET)
var msStr = '' + numsRaw[6];
ms = parseInt(msStr.substring(0, 3), 10);
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The millisecond parsing logic has a potential issue. When the fractional seconds in the date string have fewer than 3 digits, the substring operation will correctly get those digits, but the resulting number will not properly represent milliseconds. For example, if the fractional part is "5" (representing 500ms), parseInt("5".substring(0, 3), 10) returns 5 (not 500). To fix this, pad the string to 3 characters before parsing, or multiply by the appropriate power of 10 based on the actual length.

Suggested change
ms = parseInt(msStr.substring(0, 3), 10);
ms = parseInt(msStr.substring(0, 3).padEnd(3, '0'), 10);

Copilot uses AI. Check for mistakes.
Comment on lines +16 to +28
New-HTMLSection -HeaderText 'HTML DataStore (dd/MM/yyyy)' {
New-HTMLTableOption -DataStore HTML -DateTimeFormat 'dd/MM/yyyy HH:mm:ss'
New-HTMLTable -DataTable $AdminsDisabled -HideFooter -DisablePaging -DateTimeSortingFormat 'DD/MM/YYYY HH:mm:ss' {
New-HTMLTableHeader -Title 'HTML: Highlight RefreshTokenDate < threshold'
New-HTMLTableCondition -Name 'RefreshTokenDate' -ComparisonType 'date' -Operator lt -Value $DateDeleteCheck -BackgroundColor Red -Color Black -FailBackgroundColor LightGreen -FailColor Black -DateTimeFormat 'dd/MM/YYYY HH:mm:ss'
}
}

New-HTMLSection -HeaderText 'JavaScript DataStore (dd/MM/yyyy)' {
New-HTMLTableOption -DataStore JavaScript -DateTimeFormat 'dd/MM/yyyy HH:mm:ss'
New-HTMLTable -DataTable $AdminsDisabled -HideFooter -DisablePaging -DateTimeSortingFormat 'DD/MM/YYYY HH:mm:ss' {
New-HTMLTableHeader -Title 'JavaScript: Highlight RefreshTokenDate < threshold'
New-HTMLTableCondition -Name 'RefreshTokenDate' -ComparisonType 'date' -Operator lt -Value $DateDeleteCheck -BackgroundColor Red -Color Black -FailBackgroundColor LightGreen -FailColor Black -DateTimeFormat 'dd/MM/YYYY HH:mm:ss'
Copy link

Copilot AI Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's an inconsistency in date format case conventions. The New-HTMLTableOption uses lowercase format tokens (dd/MM/yyyy) while New-HTMLTable's DateTimeSortingFormat and New-HTMLTableCondition's DateTimeFormat use uppercase tokens (DD/MM/YYYY). While the JavaScript code handles both through the dotNetToMomentFormat converter, this inconsistency can be confusing for users. Consider standardizing on one convention throughout the example, preferably using the Moment.js convention (uppercase DD/MM/YYYY) since that's what the sorting format requires.

Suggested change
New-HTMLSection -HeaderText 'HTML DataStore (dd/MM/yyyy)' {
New-HTMLTableOption -DataStore HTML -DateTimeFormat 'dd/MM/yyyy HH:mm:ss'
New-HTMLTable -DataTable $AdminsDisabled -HideFooter -DisablePaging -DateTimeSortingFormat 'DD/MM/YYYY HH:mm:ss' {
New-HTMLTableHeader -Title 'HTML: Highlight RefreshTokenDate < threshold'
New-HTMLTableCondition -Name 'RefreshTokenDate' -ComparisonType 'date' -Operator lt -Value $DateDeleteCheck -BackgroundColor Red -Color Black -FailBackgroundColor LightGreen -FailColor Black -DateTimeFormat 'dd/MM/YYYY HH:mm:ss'
}
}
New-HTMLSection -HeaderText 'JavaScript DataStore (dd/MM/yyyy)' {
New-HTMLTableOption -DataStore JavaScript -DateTimeFormat 'dd/MM/yyyy HH:mm:ss'
New-HTMLTable -DataTable $AdminsDisabled -HideFooter -DisablePaging -DateTimeSortingFormat 'DD/MM/YYYY HH:mm:ss' {
New-HTMLTableHeader -Title 'JavaScript: Highlight RefreshTokenDate < threshold'
New-HTMLTableCondition -Name 'RefreshTokenDate' -ComparisonType 'date' -Operator lt -Value $DateDeleteCheck -BackgroundColor Red -Color Black -FailBackgroundColor LightGreen -FailColor Black -DateTimeFormat 'dd/MM/YYYY HH:mm:ss'
New-HTMLSection -HeaderText 'HTML DataStore (DD/MM/YYYY)' {
New-HTMLTableOption -DataStore HTML -DateTimeFormat 'DD/MM/YYYY HH:mm:ss'
New-HTMLTable -DataTable $AdminsDisabled -HideFooter -DisablePaging -DateTimeSortingFormat 'DD/MM/YYYY HH:mm:ss' {
New-HTMLTableHeader -Title 'HTML: Highlight RefreshTokenDate < threshold'
New-HTMLTableCondition -Name 'RefreshTokenDate' -ComparisonType 'date' -Operator lt -Value $DateDeleteCheck -BackgroundColor Red -Color Black -FailBackgroundColor LightGreen -FailColor Black -DateTimeFormat 'DD/MM/YYYY HH:mm:ss'
}
}
New-HTMLSection -HeaderText 'JavaScript DataStore (DD/MM/YYYY)' {
New-HTMLTableOption -DataStore JavaScript -DateTimeFormat 'DD/MM/YYYY HH:mm:ss'
New-HTMLTable -DataTable $AdminsDisabled -HideFooter -DisablePaging -DateTimeSortingFormat 'DD/MM/YYYY HH:mm:ss' {
New-HTMLTableHeader -Title 'JavaScript: Highlight RefreshTokenDate < threshold'
New-HTMLTableCondition -Name 'RefreshTokenDate' -ComparisonType 'date' -Operator lt -Value $DateDeleteCheck -BackgroundColor Red -Color Black -FailBackgroundColor LightGreen -FailColor Black -DateTimeFormat 'DD/MM/YYYY HH:mm:ss'

Copilot uses AI. Check for mistakes.
@claude
Copy link

claude bot commented Dec 14, 2025

Pull Request Review - PR #514

Overview

This PR updates the DataTables ColumnHighlighter JavaScript library from v0.1.2 to v0.1.12, adds a new example demonstrating DateTime condition formatting, and bumps the module version to 1.40.0.


Code Quality & Best Practices

Strengths

  1. Well-structured example (Example-TableDateTimeConditionsFormats.ps1):

    • Clear, self-documenting code with helpful comments
    • Tests both HTML and JavaScript DataStore modes
    • Demonstrates edge cases (null values, boundary conditions)
    • Follows existing example patterns in the repository
  2. JavaScript improvements:

    • Comprehensive date/number parsing with locale support
    • Robust .NET to Moment.js format conversion
    • Better header name normalization with fuzzy matching
    • Improved error handling with try-catch blocks
  3. Version management:

    • Appropriate version bump (1.39.0 → 1.40.0)
    • Updated dependency (PSSharedGoods 0.0.310 → 0.0.312)
    • Consistent CDN URL update across configuration

Potential Issues & Concerns

⚠️ Medium Priority

  1. JavaScript file size explosion (Resources/JS/dataTables.columnHighlighter.js:1-715):

    • File grew from ~40 lines to ~715 lines (18x increase)
    • This is a significant bundle size increase for users
    • Recommendation: Consider:
      • Minifying the production version
      • Documenting the size increase in CHANGELOG
      • Evaluating if all features are necessary
  2. Date parsing complexity (Resources/JS/dataTables.columnHighlighter.js:56-284):

    • Multiple parsing strategies with fallbacks
    • Complex format conversion logic
    • Concern: May have edge cases with unusual date formats
    • Recommendation: Add comprehensive test cases for date parsing
  3. Missing test coverage:

    • No tests for the new DateTime condition functionality
    • Existing tests don't cover New-HTMLTableCondition with dates
    • Recommendation: Add tests to Tests/New-HTMLTable.Tests.ps1:
      It 'Should apply date conditions correctly' {
          # Test date comparisons with various formats
      }

📝 Low Priority (Code Style)

  1. Inconsistent date format notation (Example-TableDateTimeConditionsFormats.ps1:18,20,26,28):

    • PowerShell uses lowercase (dd/MM/yyyy)
    • JavaScript/Moment uses uppercase (DD/MM/YYYY)
    • While this works, it could confuse maintainers
    • Recommendation: Add a comment explaining the format difference
  2. Module metadata formatting (PSWriteHTML.psd1:14-17):

    • Changed from inline to multi-line format
    • Minor: Inconsistent with the rest of the file
    • Recommendation: Keep consistent formatting (minor cosmetic issue)

Performance Considerations

Good

  • Caching mechanism for header lookups reduces repeated DOM queries
  • Early returns in parsing functions prevent unnecessary work

⚠️ Potential Impact

  • Large JavaScript file will increase page load time
  • Complex parsing on every table render could impact tables with many rows
  • Recommendation:
    • Profile performance with large datasets (1000+ rows)
    • Consider lazy loading the library only when conditions are used

Security Concerns

No Critical Issues Found

  1. Input sanitization: Number/date parsing appears safe, no eval() or innerHTML usage
  2. XSS protection: jQuery's .text() used instead of .html() for header names
  3. Injection risks: No obvious SQL/command injection vectors

📝 Minor Observations

  • Regular expressions in normalizeName() are safe
  • Date parsing fallbacks to native Date() constructor could theoretically accept unexpected formats, but this is expected behavior

Missing Elements

  1. CHANGELOG.MD update: Should document:

    • Breaking changes (if any in date format handling)
    • New features (improved date/number parsing)
    • Library version update
  2. Documentation:

    • No update to docs for new DateTimeFormat parameter behavior
    • Consider updating Docs/New-HTMLTableCondition.md
  3. Test Coverage:

    • No automated tests for the new example
    • No regression tests for date parsing edge cases

Recommendations

Before Merge:

  1. Add CHANGELOG entry documenting the library update
  2. Add test coverage for DateTime conditions
  3. Document the file size increase and performance implications
  4. ⚠️ Consider minification for the JavaScript file

Optional Improvements:

  1. Add JSDoc comments to JavaScript functions for maintainability
  2. Profile performance with large datasets
  3. Add example output screenshots to the example file comments

Overall Assessment

Status: Approve with Recommendations

This is a solid enhancement that significantly improves date and number handling in table conditions. The JavaScript improvements are well-implemented with good error handling. The main concern is the significant file size increase, which should be documented and potentially mitigated with minification.

The example is well-written and demonstrates the new capabilities clearly. With test coverage and documentation updates, this PR will be ready for production.

Estimated Risk: Low to Medium

  • Low risk for functionality (well-tested externally, comprehensive error handling)
  • Medium risk for performance (file size increase needs monitoring)

Review generated with Claude Code - Focusing on production-ready code quality

@PrzemyslawKlys PrzemyslawKlys merged commit bc4bfb6 into master Dec 14, 2025
6 checks passed
@PrzemyslawKlys PrzemyslawKlys deleted the fix/htmlextensions-columnhighlighter-v0.1.10 branch December 14, 2025 19:40
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.

2 participants