Skip to content

Fix restrictions#53

Merged
Stigmatoz merged 2 commits intoStigmatoz:masterfrom
tschettler:fix-restrictions
Oct 10, 2021
Merged

Fix restrictions#53
Stigmatoz merged 2 commits intoStigmatoz:masterfrom
tschettler:fix-restrictions

Conversation

@tschettler
Copy link
Contributor

@tschettler tschettler commented Oct 8, 2021

Fixes two issues with restrictions:

  1. Issue where an added restriction without a path did not match a url with a path.
  2. Race condition loading restriction list and setting restriction icon on charts

Ideally, we would add a restriction icon for urls that are restricted by path as well, maybe this could be a future enhancement.

@Stigmatoz Stigmatoz merged commit cb82c9e into Stigmatoz:master Oct 10, 2021
@Stigmatoz
Copy link
Owner

Thank you, Travis.
Now I have fixed one small error with a missing icon of the current domain.

And now we have the same problem with restrictions. This is reproduced when we set the path in the list of constraints, for example, "github.com ", and then open the page with the URL "github.com/web-activity-time-tracker ".
We get the current tab from the storage with the current path, but now we store each tab with different URLs within the current domain.
We need to block the site if the current path is nested in a path from the list of restrictions.

Perhaps we need to make a new flag in the list of restrictions indicating "the entire domain" and "Only the current page with all nested paths". This can work more accurately with a list of restrictions.

What do you think ?

@tschettler
Copy link
Contributor Author

Thanks for the feedback, let me take a look and try to understand the issue, because setting restrictions at the host level should still be working.

@tschettler tschettler deleted the fix-restrictions branch October 11, 2021 03:09
@tschettler
Copy link
Contributor Author

It turns out this was caused by a pre-existing race condition, where currentTab was being cleared before the chart was displayed due to how chrome.windows.getLastFocused works. No need for your latest change so I reverted it. Fixed via #54.

@Stigmatoz
Copy link
Owner

Thanks for the feedback, let me take a look and try to understand the issue, because setting restrictions at the host level should still be working.

Hi, Travis.
We have a restriction error in the method isLimitExceeded(domain, tab). We pass only the current tab to this method. When we open two or more tabs in the current domain with different URLs and different nested paths, we now check only the time for the current tab, and we need to check the time for all tabs with a URL that includes a restriction URL.

@tschettler
Copy link
Contributor Author

tschettler commented Nov 10, 2021

Hi, Travis.
We have a restriction error in the method isLimitExceeded(domain, tab). We pass only the current tab to this method. When we open two or more tabs in the current domain with different URLs and different nested paths, we now check only the time for the current tab, and we need to check the time for all tabs with a URL that includes a restriction URL.

Shouldn't it only be checking for the current tab? Maybe I'm not understanding, are you saying the expectation is that the timer will increment for tabs that are not active? So if I have two tabs open for different restricted Urls and I'm only active on one of them, the extension will increment the activity for each tab and ultimately block both, even if I never go to the other tab?

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