Skip to content

Warn for non-standard datetime values - #378

Open
ryux1 wants to merge 3 commits into
github:mainfrom
ryux1:fix/strict-datetime-validation
Open

ryux1 wants to merge 3 commits into
github:mainfrom
ryux1:fix/strict-datetime-validation

Conversation

@ryux1

@ryux1 ryux1 commented Sep 7, 2026

Copy link
Copy Markdown

Summary

  • validate non-empty datetime attributes against the ECMAScript date-time string format
  • warn for malformed or implementation-dependent values while preserving the existing parsing behavior
  • cover valid date-only, date-time, offset, expanded-year, and midnight forms

Closes #266

Testing

  • npm test (636 Chromium tests)
  • TypeScript and ESLint checks run as part of the test build
  • npx prettier --check src/relative-time-element.ts test/relative-time.js
  • git diff --check

@ryux1
ryux1 requested a review from a team as a code owner September 7, 2026 17:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Validation remains partly implementation-dependent, and the new test assumes V8-specific parsing behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Adds warnings for non-standard datetime values while preserving native parsing behavior.

Changes:

  • Adds ECMAScript date-time validation and warnings.
  • Adds coverage for valid and malformed values.
File Description
src/​relative-time-element.ts Adds validation and warning logic.
test/​relative-time.js Tests warnings and accepted formats.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +22 to +23
const dateTimeFormatPattern =
/^(?:\d{4}|(?!-000000)[+-]\d{6})(?:-(?:0[1-9]|1[0-2])(?:-(?:0[1-9]|[12]\d|3[01]))?)?(?:T(?:(?:[01]\d|2[0-3]):[0-5]\d(?::[0-5]\d(?:\.\d{3})?)?|24:00(?::00(?:\.000)?)?)(?:Z|[+-](?:[01]\d|2[0-3]):[0-5]\d)?)?$/
Comment thread test/relative-time.js Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The regex accepts incomplete date-time forms such as 2024T03:04, preventing the intended warning.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Require complete dates before accepting a time component

src/​relative-time-element.ts:23

The time portion is optional independently of the month/day portion, so non-standard values such as 2024T03:04 and 2024-01T03:04 match this regex. V8's Date.parse also accepts both, which means Chromium emits no warning for exactly the implementation-dependent fallback this validation is intended to detect. Nest the T... group under the day component (and add these values to the malformed cases) so a time is only allowed after a complete date.

@francinelucca francinelucca left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this contribution @ryux1 🙏🏽, can you take a look at copilot's review comments above? thanks!

This branch has not been deployed

No deployments
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.

Stricter datetime validation

4 participants