Attempt to add Automation section and WebDriver support #265

kenchris · 2024-04-18T14:34:39Z

index.html

rakuco

Apologies for taking so long to review this pull request. Supporting workers (shared workers in particular) presents additional challenges. I was also trying to take the DevTools use case into consideration; even though it's orthogonal to the spec, having the WebDriver and DevTools models be similar helps us enormously.

What we've done in Generic Sensors, Device Orientation and Device Posture is plug any overridden data into a top-level navigable, so that essentially all frames in a tab use the same override, which also persists across navigations (since it's basically tied to the tab, not the main frame). This works great for features exposed to Window, and possibly for dedicated workers too, but shared workers are tied to an origin, not a tab, and can even outlive it.

Option 1: tracking origins

One approach would be tying the pressure override information to an origin rather than a top-level navigable like the Permissions API does. Since we need more endpoints than the Permissions API, perhaps we could draw some inspiration from WebAuthn and keep a mapping of source type to a list of domains that could be referred to by an id. The pseudocode would look like this in JS:

const id = await add_override("cpu", ["kde.org", "www.foo.bar:8443"]);
await set_pressure(id, { state: "critical" });
await remove_override(id);

This works great for WebDriver and web tests but not so much for DevTools, where the effects are only supposed to last while the UI is open and only affect the current tab. We'd probably need separate CDP commands for this use case and error out when both modes are being used at the same time, and optionally just not support shared workers in this case at all.

Option 2: storing the data in the top-level traversable

The usual approach. Works great with DevTools and WebDriver, except in the shared workers case: what should happen when the top-level traversable is destroyed but the shared worker's owner set remains non-empty?

This is the easiest approach to spec and implement though.

General comments

Before reviewing this PR line by line and regardless of the issue above, some bigger picture points need to be settled:

It is generally a good idea to use WebDriver to provide fake/virtual/mock versions of the lowest level of your stack so that more of the real one is exercised.
- Specifically, I am not convinced you need a "mock platform collector" on top of a virtual pressure source; instead, the spec needs to make a regular platform collector connect to a virtual pressure source when desired.
Related to the above, there needs to be a closer integration into the existing algorithms. For example:
- What does "using a virtual source type" mean exactly?
- Which parts of https://w3c.github.io/compute-pressure/#processing-model and https://w3c.github.io/compute-pressure/#the-pressureobserver-object (and others) need to be aware of the automation concepts and use them?
What happens when a virtual source type is removed while one or more observers are active? Should they just stop receiving callbacks?
The data delivery model: should an update immediately notify the platform collector, or should it be stored and picked up by the platform collector at its sampling rate? This is normally an implementation detail, but with this API we have the problem of a potentially too slow sampling rate in production that the former option could help with.

index.html

rakuco · 2024-05-01T12:44:50Z

index.html

@@ -161,6 +161,10 @@ <h3>Supported sources</h3>
      <em>global system thermals</em> and the <em>central [=processing unit=]</em>, also know as the CPU.
      Future levels of this specification MAY introduce additional [=source types=].
    </p>
+    <p>
+      If a [=virtual pressure source=] exists, the [=supported source types=] is equal to the


Hmm, this is an interesting issue. As this is written, it means if an implementation supports both "thermals" and "cpu", then calling "create virtual pressure source" with "cpu" will make knownSources return "cpu" (but not "thermals").

Additionally:

Having this text here and not in an actual algorithm makes it confusing from an implementation perspective, as it is not totally clear when this change should take place.

The "supported source types" concept is a per-user agent list, whereas virtual pressure is tied to one top-level traversable.

I think what you need is to change knownSources' attribute getter steps so that it returns the union of the user agent's supported source types and whatever virtual pressure sources are set for the corresponding top-level traversable.

OK removed this, this was before the knownSources change.

Doesn't the problem persist regardless of the knownSources change? In other words, should knownSources be oblivious to changes in the supported source types caused by the usage of virtual pressure sources?

I am unsure. For now I am leaning that way, but I am open to be convinced otherwise :-)

One other approach would be to only accept type values in "create virtual pressure source" that are also in https://w3c.github.io/compute-pressure/#dfn-supported-source-types -- it'd be a way of saying that one can send fake pressure updates for types known to this implementation.

index.html

rakuco · 2024-05-01T14:12:37Z

index.html

+    This [=extension command=] deletes a given [=source type=] of [=virtual pressure source=] and returns
+    pressure source updates back to hardware.


What does "return pressure source updates back to hardware" mean? For example, what should happen to an active observer for a given source type that is not supported by the platform once the virtual pressure source is removed?

The way it is spec'ed now they just wont receive any updates. But I do need to make sure that observers don't fail if a source is not supported by hardware.

I'm marking this unresolved just so that it's properly tracked -- I think this text needs to be clarified. If you can lay out some clear steps covering what must be done, great; otherwise, at least a note explaining what you mean here is important.

index.html

- there is one platform collector per global object (GO) - a platform collector will use virtual sources for non-worker GOs - virtual sources update data at a given sample rate set when created - pressure observers collect data at their own given sample rate

arskama · 2024-05-02T14:25:10Z

I find it a bit strange that the creation of the virtual source also defines the sample array.
Should we restrict the creation to only create the virtual source? That would allow the user to control when the updates actually happen.
Also we don't have a getter version, so we cannot check what was the set samplingInterval at creation.
Should we have one Getter returning the existing virtual sources and their set samplingInterval?
Should we have an example to show what needs to be done to for example set a new samplingInterval to a virtual source? Since apparently the virtual source needs to be deleted first? (right?)

rakuco

In addition to the inline comments and discussions, let's summarize some pending items so we don't forget about them:

@JuhaVainio's point that it should be possible for a virtual pressure source to report itself as non-existent, so that observe() rejecting with NotSupportedError can be tested.
When an update's timestamp should be set when a virtual pressure source is being used.
What to do when "update virtual pressure source" is called before any observers are observing. The simplest option is to just ignore these updates; another option is to save either all or the latest and report it once the first observer starts observing.

index.html

rakuco · 2024-05-13T14:52:37Z

index.html

+      <a data-cite="!WEBDRIVER2#dfn-getting-properties">get a property</a> "sampleInterval" from |parameters|.
+    </li>
+    <li>
+      If |sampleInterval| is not an Number, return [=error=] with [=error code|WebDriver error code=] [=invalid argument=].


You also need to check that it's a valid number (i.e. > 0 in this case).

index.html

rakuco · 2024-05-13T15:08:51Z

index.html

+  </table>
+  <p>
+    This [=extension command=] allows updating the state of a [=virtual pressure source=] by pushing
+    additional [=pending samples=].


Suggested change

additional [=pending samples=].

[=pending samples=].

rakuco · 2024-05-13T15:12:05Z

index.html

+      If |samples| is not an Array, return [=error=] with [=error code|WebDriver error code=] [=invalid argument=].
+    </li>
+    <li>
+      For each |sample| in |samples|:


rakuco · 2024-05-13T15:12:16Z

index.html

+          If |sample| is not a [=string=] and a valid {{PressureState}}, return [=error=] with [=error code|WebDriver error code=] [=invalid argument=].
+        </li>
+      </ol>
+      For each |sample| in |samples|:


rakuco · 2024-05-13T15:59:33Z

index.html

+      Let |virtualPressureSource|'s [=sample update interval=] be |sampleInterval|.
+    </li>
+    <li>
+      Run the following steps [=in parallel=] at the |sampleInterval| time interval:


I was thinking about this again, and I believe this shouldn't be part of these steps in the first place.

As the steps are currently written, this will start draining the queue as soon as "update virtual pressure source" is called (which could be before the first call to observe(), for example), and in general adding and removing data from the "pending samples" in these WebDriver steps while consuming the data in "data collection" has the potential to be very flaky.

I think there are two separate things that this change is trying to address:

One needs a way to set a platform collector's sampling rate (mostly for web-platform-tests purposes, I think?).

Assuming there really is a need to be able to specify more than one update at a time, they need to be consumed in order.

One option to tackle both is to leave more of it up to users. For example:

Take one update value in "update virtual pressure source" and invoke the data collection steps for all globals belonging to a given top-level traversable.

Users (including devtools-frontend in Chromium's case) can then implement whatever logic they want to call "update virtual pressure source" with different values at a rate they want.

This effectively makes the sampling rate up to the users in the WebDriver case, which I'm not sure is something desirable or not. It'd make the spec simpler, though.

Other ideas:

Fix 1) by specifying more clearly how the "data collection" algorithm uses https://w3c.github.io/compute-pressure/#dfn-sampling-rate and make sampleInterval in the "create virtual pressure source" steps optionally (i.e. make it an optional parameter) set the sampling rate value for all platform collectors under a given top-level traversable. You'd somehow need to undo this once the virtual pressure source is removed.

Fix 1) by mandating a somewhat high sampling rate when the platform collector is connected to a virtual pressure source.

Fix 2) by consuming the pending samples queue in the "data collection" steps.

rakuco · 2024-05-13T16:16:56Z

I find it a bit strange that the creation of the virtual source also defines the sample array.
Should we restrict the creation to only create the virtual source? That would allow the user to control when the updates actually happen.

This ended up being fixed a while ago.

Also we don't have a getter version, so we cannot check what was the set samplingInterval at creation.
Should we have one Getter returning the existing virtual sources and their set samplingInterval?

Getters are tricky because the information they return might be outdated by the time it reaches users. Unless really needed, I don't think we need to provide ways to access this information (which should be available to users anyway given that they made the WebDriver calls).

Should we have an example to show what needs to be done to for example set a new samplingInterval to a virtual source? Since apparently the virtual source needs to be deleted first? (right?)

If we take the "let users handle the sampling rate" approach I mentioned in one of the inline comments, this might end up becoming a non-issue as a side-effect.

index.html

rakuco · 2024-05-27T07:56:30Z

index.html

-    represents the results of the data fusion process. This may happen in user space or in kernel space.
+    For automation purposes, a [=platform collector=] must have the ability to connect to
+    [=virtual pressure sources=] and user their simulated data, as already [=adjusted pressure states=]
+    instead of actual platform data.


s/actual/raw/? Or maybe "unprocessed"?

"unprocessed platform data" or "unprocess platform metrics"?

rakuco · 2024-05-27T07:59:16Z

index.html

@@ -243,26 +243,23 @@ <h3>Sampling and Reporting Rate</h3>

 <section> <h2>Platform primitives</h2>
  <p>
-    The [=platform collector=] refers to a platform interface, with which the [=user agent=] interacts to
-    obtain the telemetry readings required by this specification.
+    The [=platform collector=] refers to an abstract platform interface, with which the


Shouldn't the <dfn> for "platform collector" be here instead?

rakuco · 2024-05-27T08:00:42Z

index.html

@@ -361,7 +358,7 @@ <h3>
        a <dfn>registered observer list</dfn> per supported [=source type=], which is initially empty.
      </li>
      <li>
-        a reference to an underlying <dfn>platform collector</dfn> as detailed in [[[#platform-primitives]]].
+        a <dfn>platform collector</dfn> as detailed in [[[#platform-primitives]]].


There can be more than one platform collector because of the different source types, right?

index.html

rakuco · 2024-05-27T11:33:32Z

index.html

+      Set |virtualPressureSource|'s [=pending sample=] to |sample|.
+    </li>
+    <li>
+      In an [=implementation-defined=] way, run the [=data collection=] steps.


The data collection algorithm takes two parameters that you need to provide here.

index.html

It was not being used anywhere; the source type is always when manipulating the virtual pressure source mapping, which uses source types as keys.

This makes it more clear when writing the spec that this is a concept related to a virtual pressure source rather than a general one.

…te algorithm This makes it possible to invoke it from multiple locations in preparation for supporting adding a virtual pressure source that reports itself as non-existent. While here: add `class="algorithm"` to the "data collection" algorithm.

This flag can be set in the "create virtual pressure source" endpoint, where it is set to true by default. When false, it causes calls to PressureObserver.observe() to reject with NotSupportedError when attempting to use the given virtual pressure source.

Merely for readability; if we reach the assertion, it means there must be exactly one element in the owning set (only shared workers would have more than one owning document).

- Get rid of `|data|` altogether. Specifying a return variable in the algorithm declaration is at the very least unusual. Furthermore, `|data|` and `|state|` were serving the same purpose in the virtual pressure source case while not working correctly in the non-virtual one at all: in the non-virtual case, `|data|` would always be null and the entire algorithm would abort. Furthermore, even if it did not, the subsequent step was reading `|data|` without ever writing to it first. - Stop setting a virtual platform sensor's "pending sample" to null after reading it. It is not necessary; if the same sample is sent on a later call, the other algorithms (like "has change in data") should take care of ignoring it instead. - Formatting: add missing `<p></p>` tags to the `<aside>` block.

arskama · 2024-05-31T08:17:44Z

index.html

@@ -258,7 +258,7 @@ <h3>Sampling and Reporting Rate</h3>
  </p>
  <p>
    For automation purposes, a [=platform collector=] must have the ability to connect to
-    [=virtual pressure sources=] and user their simulated data, as already [=adjusted pressure states=]
+    [=virtual pressure sources=] and user their simulated data as already [=adjusted pressure states=]


Is it me, or the whole sentence is hard to understand?

skipping the repetition of the verb makes the second part of the sentence difficut to understand.
"and user their simulated data as already [=adjusted pressure states=] instead of actual platform data."

arskama

LGTM

arskama

LGTM

arskama

LGTM

index.html

arskama

LGTM

…tion/deactivation This addresses w3c#265 (comment) by properly defining several concepts in one change: - "Pressure source" is now a proper `<dfn>`. It is similar to a "device sensor" in the Generic Sensor API spec, in that it works as the concept closest to the OS/hardware and retrieves some abstract telemetry data that is fed to one or more platform collectors. A virtual pressure source is a pressure source. - Platform collector's granularity has been fixed: there isn't one per global, but rather one per source type. To make it easier to follow, a global now has a "platform collector mapping" rather than just an associated platform collector. - Each platform collector now has some associated data: an "associated pressure source", which is the pressure source it retrieves data from, as well as an "activated" flag that is used when activating/deactivating data collection. With these new building blocks, we can fix and/or properly define some concepts that have been unclear so far: - A null associated pressure source indicates that a given pressure source is not available. This is used, for example, in `PressureObserver.observe()`, which is now responsible for creating a platform collector when one is not available in its global and attempting to set the platform collector's associated pressure source to either a virtual pressure source or a real one. When neither can be found, it remains null, which signals that `observe()` must reject with NotSupportedError. Previously (see w3c#273), the `observe()` steps simply checked if `|source|` was a "valid source type", which did not properly reflect the potentially asynchronous checks involved in the real pressure source case. - The absence of an entry for a given source type in the "platform collector mapping" indicates that `PressureObserver.observe()` must attempt to create a new platform collector as described above. An existing "platform collector mapping" entry is removed only by calls to `disconnect()` and `unobserve()` as well as when a document is no longer fully active or a worker's closing flag is set to true. Conceptually, this indicates more clearly that once a platform collector is created in `observe()` it will continue using the same associated pressure source. In other words, if `observe()` is called before "create virtual pressure source", all observers for the same source type will continue using a real pressure source until they all disconnect (and vice-versa). - A virtual pressure source keeps track of connected platform collectors so that it can set their associated pressure source to null when it is removed before they disconnect. This is used in the new data collection algorithm described below. - The data collection steps have been revamped. - "Data collection" itself has not changed much: it tries to get a platform collector's pressure source if one exists, read data from it and dispatch it to observers. - We finally have proper definitions for what it means to activate and deactivate data collection. The steps themselves do not do much besides making sure 1) a platform collector for the desired source exists and 2) these steps cannot run more than once for a platform collector. The "deactivate data collection" steps also takes care of removing a collector from a virtual pressure source's connected platform collectors set.

rakuco · 2024-06-04T11:38:04Z

@kenchris @arskama @JuhaVainio I've just pushed a new big commit, c01a9a9, that implements the remaining feedback from a comment I posted a while ago. The commit message links to the specific comment I'm referring to and describes what's being done.

In short:

"Pressure source" is now a proper <dfn>.
Platform collector's granularity has been properly specified (there are per-source type platform collectors per global).
"Activate" and "deactivate" data collection have been <dfn>'ed as algorithms.
It is now part of the spec that a pressure source for a platform collector can change only if there are no observing PressureObserver instances (i.e. if a PressureObserver is fetching real platform data and "Create virtual pressure source" is called, the latter will not have effect on it or any other observers for the same source type until all have been disconnected).

The latter behavior is not set in stone, but it is the easiest/most conservative option, so we can use it as a starting point (the previous behavior was just unspecified, which is worse IMO).

Please take a look at the recent changes; if all goes well the only thing left is properly handling a sample's timestamp, which also depends on #274.

index.html

kenchris · 2024-06-04T13:24:55Z

index.html

+    interface to hardware counters or an underlying telemetry framework that
+    provides information about a <dfn>source types</dfn>
+    defined by {{PressureSource}}. A [=pressure source=] can make use of data
+    fusion with data from additional sources if that provides better results.


more precise results?

kenchris · 2024-06-04T13:26:02Z

index.html

-    A [=platform collector=] can be defined by the underlying platform (e.g. in a native telemetry
-    framework) or by the [=user agent=], if it has a direct access to hardware counters.
+    A <dfn>platform collector</dfn> is an abstract interface responsible for
+    obtaining telemetry samples from a [=pressure source=], translating them


Some places we say data, then telemetry or information. Not sure if we should make these more consistent or not

kenchris · 2024-06-04T13:26:22Z

index.html

-    framework) or by the [=user agent=], if it has a direct access to hardware counters.
+    A <dfn>platform collector</dfn> is an abstract interface responsible for
+    obtaining telemetry samples from a [=pressure source=], translating them
+    into [=pressure states=] and providing them to the [=user agent=].


kenchris · 2024-06-04T13:27:19Z

index.html

  </p>
  <p>
-    A [=platform collector=] can support telemetry for different <dfn>source types</dfn> of computing
-    devices defined by {{PressureSource}}, or there can be multiple [=platform collectors=].
+    A [=platform collector=] has the following associated data:


Maybe we can find a better word for data here. Values?

kenchris · 2024-06-04T13:28:02Z

index.html

+      source</dfn>, which is a [=pressure source=] or null.
+    </li>
+    <li>
+      an <dfn data-dfn-for="platform collector">activated</dfn> boolean flag,


maybe leave our "flag" - I am not sure that is generally understood. It is really just a boolean

kenchris · 2024-06-04T13:29:02Z

index.html

-    counter readings are a product of data fusion performed in software, the [=platform collector=]
-    represents the results of the data fusion process. This may happen in user space or in kernel space.
+    For automation purposes, a [=platform collector=] must have the ability to connect to
+    [=virtual pressure sources=] and use their simulated data as already [=adjusted pressure states=]


already states sounds weird.

kenchris · 2024-06-04T13:30:18Z

index.html

+                    <li>
+                      Let |realPressureSource| be an [=implementation-defined=]
+                      [=pressure source=] that provides telemetry information
+                      about |source|, or null if none can be found.


kenchris · 2024-06-04T13:32:09Z

index.html

+              If |owningDocuments| is empty, return null.
+            </li>
+            <li>
+              [=Assert=]: |owningDocuments|'s [=set/size=] is 1.


What is returned if the assert fails?

kenchris · 2024-06-04T13:35:10Z

index.html

+          <p>
+            This step givens implementations leeway to collect telemetry data
+            via polling or by subscribing to platform- or OS-specific
+            notifications.


kenchris · 2024-06-04T13:35:24Z

index.html

+          Otherwise:
+          <ol>
+            <li>
+              Set |state| to an [=adjusted pressure state=] calculated from


I guess it is also calculated / computed in an implementation defined manner

arskama

LGTM

arskama reviewed Apr 18, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

arskama reviewed Apr 19, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

arskama reviewed Apr 19, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

JuhaVainio reviewed Apr 25, 2024

View reviewed changes

index.html Show resolved Hide resolved

rakuco reviewed Apr 26, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

index.html Outdated Show resolved Hide resolved

kenchris closed this Apr 29, 2024

kenchris force-pushed the main branch from 798f314 to 7340ca8 Compare April 29, 2024 10:31

kenchris reopened this Apr 29, 2024

rakuco reviewed May 1, 2024

View reviewed changes

kenchris added 7 commits May 2, 2024 11:31

Attempt to add Automation section and WebDriver support

18ed9ce

Some fixes

da611a0

Fixed Arnaud's comments, add support for mocking sources

ebb89c3

Resolve comments from Raphael

109fd78

WIP: Get rid of mock platform collector

ae4875c

Attempt to fix build

c8f8afc

More fixes from comments

ba6ccbd

kenchris force-pushed the main branch from 29b348e to ba6ccbd Compare May 6, 2024 13:49

Support dedicated workers

ebb75c8

rakuco reviewed May 13, 2024

View reviewed changes

Make the automation update only set one sample

4521ec4

rakuco reviewed May 27, 2024

View reviewed changes

rakuco added 6 commits May 30, 2024 14:00

Fix [=map/Remove=] syntax

3dbb088

Remove needless comma

c92a958

Remove virtual pressure source's source type property

1768ef5

It was not being used anywhere; the source type is always when manipulating the virtual pressure source mapping, which uses source types as keys.

Use data-dfn-for for concepts scoped to virtual pressure source

417dcb4

This makes it more clear when writing the spec that this is a concept related to a virtual pressure source rather than a general one.

rakuco added 3 commits May 30, 2024 17:02

get virtual pressure source: Add assertion for owningDocument's size

8e9809c

Merely for readability; if we reach the assertion, it means there must be exactly one element in the owning set (only shared workers would have more than one owning document).

Add missing apostrophe

4a5757a

arskama reviewed May 31, 2024

View reviewed changes

JuhaVainio reviewed May 31, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

arskama reviewed May 31, 2024

View reviewed changes

Fix typo: user -> use

4f79b62

rakuco mentioned this pull request Jun 4, 2024

Properly determine bfcache integration strategy #275

Open

rakuco added 2 commits June 4, 2024 12:45

Merge remote-tracking branch 'origin/main' into webdriver

df35c2b

arskama reviewed Jun 4, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

rakuco reviewed Jun 4, 2024

View reviewed changes

index.html Outdated Show resolved Hide resolved

rakuco added 2 commits June 4, 2024 14:09

Define "source type" (singular)

9e45f07

Add a note explaining when virtual pressure sources do have an effect

126995b

kenchris commented Jun 4, 2024

View reviewed changes

arskama reviewed Jun 6, 2024

View reviewed changes

		This [=extension command=] deletes a given [=source type=] of [=virtual pressure source=] and returns
		pressure source updates back to hardware.

	For each \|sample\| in \|samples\|:
	[=list/For each=] \|sample\| in \|samples\|:

Attempt to add Automation section and WebDriver support #265

Are you sure you want to change the base?

Attempt to add Automation section and WebDriver support #265

Conversation

kenchris commented Apr 18, 2024 • edited by pr-preview bot

rakuco left a comment

Choose a reason for hiding this comment

Option 1: tracking origins

Option 2: storing the data in the top-level traversable

General comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arskama commented May 2, 2024 • edited

rakuco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakuco commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arskama left a comment

Choose a reason for hiding this comment

arskama left a comment

Choose a reason for hiding this comment

arskama left a comment

Choose a reason for hiding this comment

arskama left a comment

Choose a reason for hiding this comment

rakuco commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arskama left a comment

Choose a reason for hiding this comment

kenchris commented Apr 18, 2024 •

edited by pr-preview bot

arskama commented May 2, 2024 •

edited