Skip to content

Fix Date Parser Any Parsing Two-Digit Number as Year #4242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Apr 25, 2025
Merged

Fix Date Parser Any Parsing Two-Digit Number as Year #4242

merged 5 commits into from
Apr 25, 2025

Conversation

bytrangle
Copy link
Contributor

Description

When user creates a new task which includes short syntax for date, then following by a two-digit number, the number will be interpreted as year.

For example: "Task @14/4 90m". Implication: User schedules the task for 14/4 going forward and estimates the task as taking 90 minutes. Two problems:

  • This task will be scheduled for 14/4/1990 because Chrono understands 90 as 1990.
  • The task title will include the word "m" when it should have been escaped.

Changes I've made:

  • Write a custom parser that checks if the task title contains string in the format of DD/MM YY. The regex pattern is more complicated than that, because there are so many ways a user can write date informally. It doesn't take into account syntax like "August 8 90m".
  • The parser will set the year for the task to the current year or a year in the future
  • The parseScheduledDate is changed so that the word "m" won't be a part of the final task title.

Issues Resolved

Potentially fixes #4194

Check List

  • New functionality includes testing.
  • New functionality has been documented in the README if applicable.

@johannesjo johannesjo requested review from Copilot and johannesjo and removed request for Copilot April 10, 2025 21:05
Copilot
Copilot AI reviewed Apr 10, 2025
View reviewed changes
Copy link

@Copilot 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.

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/app/features/tasks/short-syntax.ts:321

  • [nitpick] The hacky check for a time estimate appended to the date might not handle all edge cases. A clearer separation of date and time estimate parsing would improve robustness.
if (inputDate.match(/ [0-9]{1,}m/g)) {

src/app/features/tasks/short-syntax.ts Outdated
if (matched) {
if (matched[matched.length - 1]) {
const twoDigits = matched[matched.length - 1];
result.text = text.replace(twoDigits, '');
Copy link
Preview

Copilot AI Apr 10, 2025

Choose a reason for hiding this comment

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

Replacing the two-digit year indiscriminately may remove matching digits elsewhere in the string. Consider targeting the exact location of the match or using a more precise replacement strategy.

Suggested change
result.text = text.replace(twoDigits, '');
const startIndex = matched.index + matched[0].lastIndexOf(twoDigits);
const endIndex = startIndex + twoDigits.length;
result.text = text.slice(0, startIndex) + text.slice(endIndex);

Copilot uses AI. Check for mistakes.

@bytrangle bytrangle marked this pull request as ready for review April 21, 2025 10:01
@bytrangle bytrangle changed the title Fix Date Parser Accidentally Parsing Two-Digit Number as Year Fix Date Parser Any Parsing Two-Digit Number as Year Apr 21, 2025
@johannesjo
Copy link
Owner

Hey @bytrangle ! Thank you for this PR!

Could you maybe also add some unit tests for this? Tbh I am a bit hesitant to merge this, since I am afraid of this causing new unintended problems with the new regex. Chrono node is well tested and used by many projects so we have to make sure to replace it with something reliable. Some unit tests for the different cases would give me more confidence :)

@bytrangle
Copy link
Contributor Author

Will do. I'm also scared of implementing this change because it may lead to some edge cases that I can't possibly imagine. No date parsing tool is better than Chrono in the Javascript ecosystem, but I feel like it has become too relaxing over time :)

@johannesjo
Copy link
Owner

@bytrangle I hope you don't mind I'm testing this

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Apr 24, 2025 •
edited
Loading

PR Reviewer Guide 🔍

(Review updated until commit b3ab685)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

4194 - Partially compliant

Compliant requirements:

  • Prevent two-digit numbers following a date from being parsed as a year.
  • Strip short date/time syntax from the task title.

Non-compliant requirements:

  • Duration-after-time syntax scenario is not covered by tests or implementation.
  • Time-after-duration syntax scenario is not covered by tests or implementation.
  • Numeric "DD/MM" date format handling is untested.

Requires further human verification:

  • Manual UI testing of quick-add for all supported syntax combinations.
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

String Replacement Bug

The hacky approach appends 'm' to inputDate then uses it in title.replace, which can produce incorrect patterns (e.g., "90mm") or fail to strip the intended substring. Extract and strip the exact matched text instead.

 
let inputDate = parsedDateResult.text;
// Hacky way to strip short syntax for time estimate that was
// accidentally included in the date parser
// For example: the task is "Task @14/4 90m" and we don't want "90m"
if (inputDate.match(/ [0-9]{1,}m/g)) {
  inputDate += 'm';
}
Limited Regex Coverage

The regex / [5-9][0-9]$/ only matches two-digit numbers from 50–99. Confirm that other date/duration formats (e.g., numeric "DD/MM" and various estimate patterns) are correctly parsed and not misinterpreted as years.

 
const regex = / [5-9][0-9]$/;
const yearIndex = text.search(regex);
// The year pattern in Chrono's source code is (?:[1-9][0-9]{0,3}\\s{0,2}(?:BE|AD|BC|BCE|CE)|[1-2][0-9]{3}|[5-9][0-9]|2[0-5]).
// This means any two-digit numeric value from 50 to 99 will be considered a year.
// Link: https://github.com/wanasit/chrono/blob/54e7ff12f9185e735ee860c25922b2ab2367d40b/src/locales/en/constants.ts#L234C30-L234C108
// When someone creates a task like "Test @25/4 90m", Chrono will return the year as 1990, which is an undesirable behaviour in most cases.
if (yearIndex !== -1) {

@johannesjo
Copy link
Owner

Thank you very much! Seems to work well as far as I can tell. I will merge this. If we encounter additional issues we can fix them in a separate PR.

@johannesjo johannesjo merged commit 78a41fb into johannesjo:master Apr 25, 2025
5 checks passed
@CodiumAI-Agent
Copy link

Persistent review updated to latest commit b3ab685

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@johannesjo johannesjo Awaiting requested review from johannesjo

Assignees
No one assigned
Labels
help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Date and time syntax not working as expected in quick-add (might be me)
3 participants
@bytrangle @johannesjo @CodiumAI-Agent