Skip to content
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

feat(config): add pause assistant #2204

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat(config): add pause assistant #2204

wants to merge 12 commits into from

Conversation

smalluban
Copy link
Collaborator

@smalluban smalluban commented Jan 22, 2025 •
edited
Loading

Fixes #2226. To test:

  1. Open Settings page
  2. Unhide the dev-tools section (Remote configuration)
  3. Add the pause-assistant flag
  4. Add the testing domain with the pause-assistant action
  5. Open testing domain and see the "Pause notification"
  6. After testing pausing, open the settings page as before, click "force sync", and add the flag again (it will clear out domain info and bring back the flag)
  7. Open the testing domain and see the "Resume notification"

Progress:

  • Pause notification
  • Pausing/Dismiss
  • Feedback notification
  • Resume notification
  • How assistant should work in managed mode
  • Feedback signals to WTM

@smalluban smalluban force-pushed the feat-pause-assistant branch 4 times, most recently from a69ed5e to 3eef519 Compare January 29, 2025 09:09
@smalluban smalluban force-pushed the feat-pause-assistant branch 2 times, most recently from 51160fd to 7a75b4c Compare January 30, 2025 14:58
@smalluban smalluban marked this pull request as ready for review January 30, 2025 15:58
@smalluban smalluban requested a review from AdamGhst January 30, 2025 16:01
@smalluban smalluban added the package CI: create extension packages label Jan 30, 2025
@smalluban smalluban force-pushed the feat-pause-assistant branch from a4293ad to 14b5a20 Compare February 10, 2025 11:15
@ghostery ghostery deleted a comment from github-actions bot Feb 10, 2025
@smalluban
Copy link
Collaborator Author

smalluban commented Feb 10, 2025 •
edited
Loading

Updated copy according to the #2226 (comment)
Zrzut ekranu 2025-02-10 o 12 20 14
Zrzut ekranu 2025-02-10 o 12 20 53

chrmod
chrmod reviewed Feb 10, 2025
View reviewed changes
src/background/reporting/webrequest-reporter.js Outdated Show resolved Hide resolved
src/manifest.firefox.json
Comment on lines +88 to +90
"pages/notifications/pause-assistant.html",
"pages/notifications/pause-resume.html",
"pages/notifications/pause-feedback.html"
Copy link
Member

Choose a reason for hiding this comment

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

how about we put all those in a single page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then, we will have much more overcomplicated logic in JS, like using a router and many views. It is better to have three separate "views" rather than one with parameters.

Copy link
Member

Choose a reason for hiding this comment

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

It is JS logic in one place or configuration in three places. Question is what is more likely to break

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is easier to reason about what happens if each notification has a separate structure. We would still need almost the same amount of files, the only advantage is one html file, but with a more complex structure in an index JS file. It is not worth it.

@smalluban smalluban requested a review from chrmod February 12, 2025 08:47
chrmod
chrmod reviewed Feb 13, 2025
View reviewed changes
src/pages/settings/components/devtools.js
'Enter flags to test:',
[FLAG_PAUSE_ASSISTANT, FLAG_FIREFOX_CONTENT_SCRIPT_SCRIPTLETS].join(', '),
);
if (!flags) return;
Copy link
Member

Choose a reason for hiding this comment

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

There may be a bug here. What if I want to clear all flags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use "force sync". This prompt is for adding flags on top of existing ones, not to overwrite them.

src/pages/settings/components/devtools.js
flags: {
[flag]: { enabled: true },
},
flags: flags.split(',').reduce((acc, flag) => {
Copy link
Member

Choose a reason for hiding this comment

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

How about?

Suggested change
flags: flags.split(',').reduce((acc, flag) => {
[FLAG_PAUSE_ASSISTANT]: { enabled: flags.include(FLAG_PAUSE_ASSISTANT) },
[FLAG_FIREFOX_CONTENT_SCRIPT_SCRIPTLETS]: { enabled: flags.include(FLAG_FIREFOX_CONTENT_SCRIPT_SCRIPTLETS) },

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You will need to add here each new flag. We still use those IDs for prompt placeholder, but this looks overcomplicated.

src/store/options.js Outdated Show resolved Hide resolved
src/manifest.firefox.json
Comment on lines +88 to +90
"pages/notifications/pause-assistant.html",
"pages/notifications/pause-resume.html",
"pages/notifications/pause-feedback.html"
Copy link
Member

Choose a reason for hiding this comment

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

It is JS logic in one place or configuration in three places. Question is what is more likely to break

src/background/reporting/webrequest-reporter.js Outdated Show resolved Hide resolved
src/pages/notifications/pause-feedback.js
import { REVIEW_PAGE_URL } from '/utils/urls.js';
import { openTabWithUrl } from '/utils/tabs.js';

const close = setupNotificationPage(360);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const close = setupNotificationPage(360);
const close = () => setupNotificationPage(360);

so it can be called <ui-button type="success" onclick="${close}">

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. The setupNotificationPage() returns a function, you cannot wrap close as a function, you need a result of that function call. As the close takes an argument, you can't use it directly, so it's wrapped in an arrow function.

@smalluban smalluban force-pushed the feat-pause-assistant branch 2 times, most recently from 9ec404c to 92c7f64 Compare February 19, 2025 13:01
@ghostery ghostery deleted a comment from github-actions bot Feb 19, 2025
@ghostery ghostery deleted a comment from github-actions bot Feb 19, 2025
@ghostery ghostery deleted a comment from github-actions bot Feb 19, 2025
@github-actions GitHub Actions
Copy link

Builds for commit 92c7f64:

@smalluban smalluban requested a review from chrmod February 19, 2025 13:03
@smalluban smalluban force-pushed the feat-pause-assistant branch from 92c7f64 to 874e631 Compare February 19, 2025 13:36
@github-actions GitHub Actions
Copy link

Builds for commit 874e631:

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

@philipp-classen philipp-classen philipp-classen approved these changes

@AdamGhst AdamGhst Awaiting requested review from AdamGhst

@chrmod chrmod Awaiting requested review from chrmod

Assignees
No one assigned
Labels
package CI: create extension packages
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Pause Assitant
3 participants
@smalluban @chrmod @philipp-classen