vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

Fix infinite loop when creating filters #3061

Merged
konrad merged 1 commits from ThatHurleyGuy/frontend:fix-loop into main 2023-11-15 12:10:22 +00:00
Conversation 9 Commits 1 Files Changed 1 +2 -2
ThatHurleyGuy commented 2023-02-05 07:17:31 +00:00
Contributor
Copy Link

Preamble

Just setup an instance of Vikunja for my wife and I to keep track of our TODO lists. Really impressed with it so far, kudos for building out such an awesome app for this. I'm more of a systems/database engineer, so my frontend experience is pretty lacking. There may very well be a better solution to this.

System Environment

Firefox 109.0.1 osx 13.1, my timezone is America/Mountain (UTC-6).

Repro Steps

  1. Create a new saved filter
  2. Set the due date to now + 12h

Expected

The date field populates with the text as it's typed in

Observed Behavior

The browser tab hangs in an infinite loop. I checked out the code to run it locally and saw that in the console I got the real fun Maximum recursive updates exceeded in component <card>. This means you have a reactive effect that is mutating its own dependencies and thus recursively triggering itself. Possible sources include component template, render function, updated hook or watcher source function. message. What seems to be happening is:

  1. The From field is null, so it defaults to Jan 1 1970
  2. This value gets into the prepare function here, and because the date object is in the user's local timezone which is -6 it's technically the day before, Dec 31 1969
  3. The Dec 31 1969 value gets set, which triggers the various change watchers, and the cycle repeats again taking Dec 30 1969, repeat

Proposed Solution

Rather than putting in a truncated version of the date/time with startDate.getDate, use the iso formatted version which includes the timezone data. I have no idea if this has ramifications elsewhere in the app, but it solves the problems I was seeing.

# Preamble Just setup an instance of Vikunja for my wife and I to keep track of our TODO lists. Really impressed with it so far, kudos for building out such an awesome app for this. I'm more of a systems/database engineer, so my frontend experience is pretty lacking. There may very well be a better solution to this. ## System Environment Firefox 109.0.1 osx 13.1, my timezone is America/Mountain (UTC-6). ## Repro Steps 1. Create a new saved filter 2. Set the due date to `now + 12h` ## Expected The date field populates with the text as it's typed in ## Observed Behavior The browser tab hangs in an infinite loop. I checked out the code to run it locally and saw that in the console I got the real fun `Maximum recursive updates exceeded in component <card>. This means you have a reactive effect that is mutating its own dependencies and thus recursively triggering itself. Possible sources include component template, render function, updated hook or watcher source function.` message. What seems to be happening is: 1. The `From` field is null, so it defaults to Jan 1 1970 2. This value gets into the `prepare` function [here](https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/list/partials/filters.vue#L430), and because the date object is in the user's local timezone which is -6 it's technically the day before, Dec 31 1969 3. The Dec 31 1969 value gets set, which triggers the various change watchers, and the cycle repeats again taking Dec 30 1969, repeat ## Proposed Solution Rather than putting in a truncated version of the date/time with `startDate.getDate`, use the iso formatted version which includes the timezone data. I have no idea if this has ramifications elsewhere in the app, but it solves the problems I was seeing.
ThatHurleyGuy force-pushed fix-loop from 8ea821667b to 7e532ab680 2023-02-05 07:19:18 +00:00 Compare
ThatHurleyGuy commented 2023-02-05 07:21:10 +00:00
Author
Contributor
Copy Link

@konrad Sorry for the super verbose PR description, wanted to try and give plenty of the debugging info I ran into. Let me know if there's anything else I can help do try and fix this!

@konrad Sorry for the super verbose PR description, wanted to try and give plenty of the debugging info I ran into. Let me know if there's anything else I can help do try and fix this!
👍 1
ThatHurleyGuy requested review from konrad 2023-02-05 07:21:20 +00:00
frederick commented 2023-02-05 07:22:58 +00:00
Member
Copy Link

Hi ThatHurleyGuy!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3061-fix-loop--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi ThatHurleyGuy! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3061-fix-loop--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen reviewed 2023-02-07 13:33:29 +00:00
src/components/list/partials/filters.vue Outdated
@ -427,10 +427,10 @@ function prepareDate(filterName, variableName) {
const endDate = new Date(params.value.filter_value[foundDateEnd])
filters.value[variableName] = {
dateFrom: !isNaN(startDate)
dpschen commented 2023-02-07 13:33:29 +00:00
Member

Good catch with that error!

When looking at the mistake I think that the !isNaN(startDate) line is the broken one, because:

!isNaN(new Date(null)) === true

What that line should actually do is to check if the Date is a valid one, which it is not! (it is [!] by the ECMAScript definition because new Date() coerces to type Number if it's not a string—didn't know that myself)

We could check that with some helper function. E.g.

// inspired by https://stackoverflow.com/questions/1353684/detecting-an-invalid-date-date-instance-in-javascript/12372720#comment42479723_12372720
function isValidKebabDateString(str) {
	if (!str) {
		return false
	}
	const date = new Date(str)
	// An invalid date object returns NaN for getTime() and NaN is the only
	// object not strictly equal to itself.
	return date.getTime() === date.getTime() 
}

I established for myself the convention that a 'kebab date' is a date string in the format 2023-02-07. I only found out later that this is also a valid ISO date string that has only day precision and lacks a time zone. Still haven't found a better name though. Maybe ISODateStringWithDayPrecision 🤷‍♂️ ?

We want to replace that filter logic anyway (see that pull request. Again has to be rebased...)

For now I would be fine to replace the !isNaN-part with a better check like the one above.

@konrad Anything to add here?

Good catch with that error! When looking at the mistake I think that the `!isNaN(startDate)` line is the broken one, because: ```ts !isNaN(new Date(null)) === true ``` What that line should actually do is to check if the Date is a valid one, which it is not! (it is \[!] by the ECMAScript definition [because `new Date()` coerces to type Number if it's not a string](https://stackoverflow.com/questions/1353684/detecting-an-invalid-date-date-instance-in-javascript/12372720#comment42479723_12372720)—didn't know that myself) We could check that with some helper function. E.g. ```ts // inspired by https://stackoverflow.com/questions/1353684/detecting-an-invalid-date-date-instance-in-javascript/12372720#comment42479723_12372720 function isValidKebabDateString(str) { if (!str) { return false } const date = new Date(str) // An invalid date object returns NaN for getTime() and NaN is the only // object not strictly equal to itself. return date.getTime() === date.getTime() } ``` I established for myself the convention that a 'kebab date' is a date string in the format `2023-02-07`. I only found out later that this is also a valid ISO date string that has only day precision and lacks a time zone. Still haven't found a better name though. Maybe `ISODateStringWithDayPrecision` 🤷‍♂️ ? We want to replace that filter logic anyway (see [that pull request](https://kolaente.dev/vikunja/frontend/pulls/2474). Again has to be rebased...) For now I would be fine to replace the `!isNaN`-part with a better check like the one above. @konrad Anything to add here?
ThatHurleyGuy commented 2023-02-07 18:43:32 +00:00
Author
Contributor

Ah yeah that's a good idea, I can take a stab at changing the isNaN check to something like you suggested here later today

Ah yeah that's a good idea, I can take a stab at changing the `isNaN` check to something like you suggested here later today
ThatHurleyGuy commented 2023-02-08 04:19:48 +00:00
Author
Contributor

Actually, I take that back. I don't think the isNaN check is related here, although I think you're right there's a subtle bug lurking there. The real issue is the time parsing and how timezones impact that. When I'm seeing this infinite loop (in timezone America/Denver, UTC-7) the date is totally valid and not nan. The issue is caused by the timezone drift. Here's a simple example from my browser's javascript console:

date = new Date()
Date Tue Feb 07 2023 21:02:01 GMT-0700 (Mountain Standard Time)

date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration
Date Mon Feb 06 2023 17:00:00 GMT-0700 (Mountain Standard Time)

date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration
Date Sun Feb 05 2023 17:00:00 GMT-0700 (Mountain Standard Time)

Each time this page renders, the date gets set back one day earlier which retriggers the formatting, repeat. However, if I set my system's timezone to Germany this drift doesn't happen because it's a positive offset (probably why others haven't seen this occurring):

date = new Date()
Date Wed Feb 08 2023 05:05:56 GMT+0100 (Central European Standard Time)

date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration
Date Wed Feb 08 2023 01:00:00 GMT+0100 (Central European Standard Time)

date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration
Date Wed Feb 08 2023 01:00:00 GMT+0100 (Central European Standard Time)

So unless I'm missing something, the isNaN check is unrelated here, the issue is timezones (the bane of all programmers existence).

The only solutions I can think of are:

  1. Drop kebab case and what I proposed in this PR, including the timezone data in the formatted string to ensure the next time it's parsed there's no drift
  2. Manually split/parse the string:
// First check if it's kebab case or something
dateString = "2023-01-30"
"2023-01-30"

split = dateString.split(/\D/)
Array(3) [ "2023", "01", "30" ]

date = new Date(split[0], split[1]-1, split[2])
Date Mon Jan 30 2023 00:00:00 GMT-0700 (Mountain Standard Time)
  1. Just always use getUTC{FullYear,Month,Date} to ensure it's always the same timezone date used:
date = new Date()
Date Tue Feb 07 2023 21:15:05 GMT-0700 (Mountain Standard Time)

date = new Date(`${date.getUTCFullYear()}-${date.getUTCMonth() + 1}-${date.getUTCDate()}`) // Next iteration
Date Tue Feb 07 2023 17:00:00 GMT-0700 (Mountain Standard Time)

date = new Date(`${date.getUTCFullYear()}-${date.getUTCMonth() + 1}-${date.getUTCDate()}`) // Next iteration
Date Tue Feb 07 2023 17:00:00 GMT-0700 (Mountain Standard Time)
Actually, I take that back. I don't think the `isNaN` check is related here, although I think you're right there's a subtle bug lurking there. The real issue is the time parsing and how timezones impact that. When I'm seeing this infinite loop (in timezone America/Denver, UTC-7) the date is totally valid and *not* `nan`. The issue is caused by the timezone drift. Here's a simple example from my browser's javascript console: ``` date = new Date() Date Tue Feb 07 2023 21:02:01 GMT-0700 (Mountain Standard Time) date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration Date Mon Feb 06 2023 17:00:00 GMT-0700 (Mountain Standard Time) date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration Date Sun Feb 05 2023 17:00:00 GMT-0700 (Mountain Standard Time) ``` Each time this page renders, the date gets set back one day earlier which retriggers the formatting, repeat. However, if I set my system's timezone to Germany this drift doesn't happen because it's a positive offset (probably why others haven't seen this occurring): ``` date = new Date() Date Wed Feb 08 2023 05:05:56 GMT+0100 (Central European Standard Time) date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration Date Wed Feb 08 2023 01:00:00 GMT+0100 (Central European Standard Time) date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration Date Wed Feb 08 2023 01:00:00 GMT+0100 (Central European Standard Time) ``` So unless I'm missing something, the `isNaN` check is unrelated here, the issue is timezones (the bane of all programmers existence). The only solutions I can think of are: 1. Drop kebab case and what I proposed in this PR, including the timezone data in the formatted string to ensure the next time it's parsed there's no drift 2. Manually split/parse the string: ``` // First check if it's kebab case or something dateString = "2023-01-30" "2023-01-30" split = dateString.split(/\D/) Array(3) [ "2023", "01", "30" ] date = new Date(split[0], split[1]-1, split[2]) Date Mon Jan 30 2023 00:00:00 GMT-0700 (Mountain Standard Time) ``` 3. Just always use `getUTC{FullYear,Month,Date}` to ensure it's always the same timezone date used: ``` date = new Date() Date Tue Feb 07 2023 21:15:05 GMT-0700 (Mountain Standard Time) date = new Date(`${date.getUTCFullYear()}-${date.getUTCMonth() + 1}-${date.getUTCDate()}`) // Next iteration Date Tue Feb 07 2023 17:00:00 GMT-0700 (Mountain Standard Time) date = new Date(`${date.getUTCFullYear()}-${date.getUTCMonth() + 1}-${date.getUTCDate()}`) // Next iteration Date Tue Feb 07 2023 17:00:00 GMT-0700 (Mountain Standard Time) ```
👍 1
dpschen commented 2023-02-08 12:12:40 +00:00
Member

Interesting find. I like version 3. Mostly because it would align best with the current implementation.

For the future it would probably be best if we would use ISO dates everywhere, but for that it would make sense to check all the other parts of the app that use date. Might as well be that I'm missing some insight for my conclusion.

What do you think @konrad?

Interesting find. I like version 3. Mostly because it would align best with the current implementation. For the future it would probably be best if we would use ISO dates everywhere, but for that it would make sense to check all the other parts of the app that use date. Might as well be that I'm missing some insight for my conclusion. What do you think @konrad?
ThatHurleyGuy commented 2023-02-08 23:52:29 +00:00
Author
Contributor

Updated the PR to use UTC dates

Updated the PR to use UTC dates
konrad commented 2023-02-22 12:36:49 +00:00
Owner

@dpschen ISO dates are a good idea, would make it consistent with what the api does already.

@dpschen ISO dates are a good idea, would make it consistent with what the api does already.
konrad requested review from dpschen 2023-02-07 15:02:38 +00:00
dpschen was assigned by konrad 2023-02-07 15:02:45 +00:00
konrad commented 2023-02-07 15:08:03 +00:00
Owner
Copy Link

What version did you encounter the bug on? I just tried this on try (last unstable build), but didn't manage to reproduce it. I do remember though having fixed this error in the past.

What version did you encounter the bug on? I just tried this on try (last unstable build), but didn't manage to reproduce it. I do remember though having fixed this error in the past.
ThatHurleyGuy commented 2023-02-07 18:41:34 +00:00
Author
Contributor
Copy Link

@konrad This happens on every version I tried, including main/HEAD. What timezone did you test it in? Like I mentioned in the PR description, it consistently happens to me with any negative offset timezones

@konrad This happens on every version I tried, including main/HEAD. What timezone did you test it in? Like I mentioned in the PR description, it consistently happens to me with any negative offset timezones
ThatHurleyGuy force-pushed fix-loop from 7e532ab680 to 4527a66c83 2023-02-08 23:51:41 +00:00 Compare
konrad commented 2023-02-22 12:07:04 +00:00
Owner
Copy Link

@konrad This happens on every version I tried, including main/HEAD. What timezone did you test it in? Like I mentioned in the PR description, it consistently happens to me with any negative offset timezones

I'm in UTC+1 (Germany). I set the Browser to UTC-7 but could not reproduce your example:

date = new Date()
Wed Feb 22 2023 05:05:28 GMT-0700 (GMT-07:00)
date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration
Wed Feb 22 2023 00:00:00 GMT-0700 (GMT-07:00)
date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration
Wed Feb 22 2023 00:00:00 GMT-0700 (GMT-07:00)
date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration
Wed Feb 22 2023 00:00:00 GMT-0700 (GMT-07:00)
> @konrad This happens on every version I tried, including main/HEAD. What timezone did you test it in? Like I mentioned in the PR description, it consistently happens to me with any negative offset timezones I'm in UTC+1 (Germany). I set the Browser to UTC-7 but could not reproduce your example: ``` date = new Date() Wed Feb 22 2023 05:05:28 GMT-0700 (GMT-07:00) date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration Wed Feb 22 2023 00:00:00 GMT-0700 (GMT-07:00) date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration Wed Feb 22 2023 00:00:00 GMT-0700 (GMT-07:00) date = new Date(`${date.getFullYear()}-${date.getMonth() + 1}-${date.getDate()}`) // Next iteration Wed Feb 22 2023 00:00:00 GMT-0700 (GMT-07:00)
konrad commented 2023-05-31 14:15:56 +00:00
Owner
Copy Link

Is this still relevant?

Is this still relevant?
konrad commented 2023-10-22 17:28:51 +00:00
Owner
Copy Link

@ThatHurleyGuy Looks like the browser hanging is not an issue but the date input does not seem to do what it should.

Do you want to modify your PR?

@ThatHurleyGuy Looks like the browser hanging is not an issue but the date input does not seem to do what it should. Do you want to modify your PR?
ThatHurleyGuy force-pushed fix-loop from 4527a66c83 to e08b887048 2023-11-14 05:42:30 +00:00 Compare
ThatHurleyGuy force-pushed fix-loop from e08b887048 to 5c08442837 2023-11-14 05:50:01 +00:00 Compare
ThatHurleyGuy commented 2023-11-14 06:02:44 +00:00
Author
Contributor
Copy Link

@konrad Sorry for the slowww response. I just updated the PR to be up to date with main. Uploaded screenshots for what I'm seeing on main vs how it looks on my branch/machine. Main definitely still has the loop (in dev there's a guard that prevents it from looping more than 100 times and then spits out that message). The result is "End Date" with the verbose date of January 1st 1970 minus 100 iterations of the loop subrracting a day. I figured the compact version of the date in my PR would be easier to update.

What are you seeing when you say "date input does not seem to do what it should"? I also check the calendar date picker and it seemed to work as I'd expect

As a side note, I've been Vikunja daily for the past 9 months and have been loving it. Thanks for making this available!

@konrad Sorry for the slowww response. I just updated the PR to be up to date with main. Uploaded screenshots for what I'm seeing on main vs how it looks on my branch/machine. Main definitely still has the loop (in dev there's a guard that prevents it from looping more than 100 times and then spits out that message). The result is "End Date" with the verbose date of January 1st 1970 minus 100 iterations of the loop subrracting a day. I figured the compact version of the date in my PR would be easier to update. What are you seeing when you say "date input does not seem to do what it should"? I also check the calendar date picker and it seemed to work as I'd expect As a side note, I've been Vikunja daily for the past 9 months and have been loving it. Thanks for making this available!
branch.gif
127 KiB
main.gif
185 KiB
branch.gif main.gif
konrad commented 2023-11-15 12:05:07 +00:00
Owner
Copy Link

I'm seeing the same on main as in your branch.

What are you seeing when you say "date input does not seem to do what it should"? I also check the calendar date picker and it seemed to work as I'd expect

Very good question 🙃 might be related to this: https://community.vikunja.io/t/filter-setting-s/1791/3?u=kolaente

I'm seeing the same on `main` as in your branch. > What are you seeing when you say "date input does not seem to do what it should"? I also check the calendar date picker and it seemed to work as I'd expect Very good question 🙃 might be related to this: https://community.vikunja.io/t/filter-setting-s/1791/3?u=kolaente
konrad approved these changes 2023-11-15 12:09:35 +00:00
konrad merged commit 7e623d919e into main 2023-11-15 12:10:22 +00:00
konrad deleted branch fix-loop 2023-11-15 12:10:22 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
dpschen
4 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3061
No description provided.
Delete Branch "ThatHurleyGuy/frontend:fix-loop"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?