Scopes: Add `ScopesBridge` object by bfmatei · Pull Request #990 · grafana/scenes

bfmatei · 2024-12-04T13:43:59Z

Continuing the discussion from here: grafana/grafana#97176

So what this PR brings: allow SceneObjects to consume React Contexts.

📦 Published PR as canary version: 6.3.1--canary.990.13785017927.0

✨ Test out this PR locally via:

npm install @grafana/scenes-react@6.3.1--canary.990.13785017927.0
npm install @grafana/scenes@6.3.1--canary.990.13785017927.0
# or 
yarn add @grafana/scenes-react@6.3.1--canary.990.13785017927.0
yarn add @grafana/scenes@6.3.1--canary.990.13785017927.0

torkelo

I think this is really cool, but until we have more examples / stronger need for this for many contexts I would recommend a more simple specific implementation for the ScopeContext specifically (maybe a base class that can handle some generic stuff).

Like a ScopeContextConsumer scene object that you can add to your scene (we would add it to DashboardScene), maybe we add it as an optional state prop to EmbeddeddScene or SceneApp (DashboardSceneRenderer would have render the ScopeContextConsumer component to make the context => scene state active).

Adding this to all scene objects, and to ComponentWrapper feels a bit too drastic until we know how much this will be used

torkelo · 2024-12-04T15:15:16Z

But I do think something like ScopesContextConsumer or ScopesSceneBridge (whatever we call it), could be something we add to scenes lib (and it depends on the context in runtime). And then scene app plugins can use it

dprokop

Agree with Torkel overall, this is a neat addition, but maybe would be worth talking it trough more widely if there's really a library need for this. It introduces quite a bit of complexity, and I'm not sure if we should solve this problem in classic scenes, but rather in react-scenes.

dprokop · 2024-12-05T08:54:13Z

packages/scenes/src/core/SceneComponentWrapper.tsx

+function SceneComponentWrapperWithoutMemo<T extends SceneObject>({ model, ...otherProps }: SceneComponentProps<T>) {
+  return [
+    <ContextsConsumer key="contexts" model={model} />,
+    <ComponentRenderer key="component" model={model} {...otherProps} />,
+  ];
+}
+


Took me a bit to udnerstand this 🤔

bfmatei · 2024-12-05T16:13:18Z

@torkelo @dprokop

I refactored this to only use React Context for Scopes (added a bridge between scopes and scenes).

A couple of notes:

SceneApp seems the best place to place the bridge as it's top level, meaning that all nested scene objects can get it
SceneAppPage have a useScopes flag that control wether a page should or should not show the scopes selector
the bridge also handles URL sync as it's way better than controlling it via the service (I can change this tho')
added usage for SceneQueryRunner as well as adhoc variables and groupby variables. No more double queries and no more data/filters enriching needed
the context is defined here for testing purposes as it's easier than to link runtime to scopes and then scopes to grafana while doing development
the useScopes hook has to spread the context as simply returning the context won't take effect for use-cases like

useEffect(() => {
  console.log(context);
}, [context]);

the context has to be a service as it's way easier to offer interoperability between scopes dashboards, scopes selector and the scopes "facade"
we need to do something with Prettier. Apparently there are a lot of files which are not formatted correctly 😢

torkelo

I think this looking really good

I think the isScopesLoading code added to AdHocFilters and GroupByVarable feel a bit on the "too complex" / detail that it should not have to care so deeply about (ie subscribe to state etc).

Wonder if we can skip that for now, and if it becomes a problem find a less invasive solution (like a loading spinner on the whole scene until scopes have initialized)

packages/scenes/src/core/SceneScopesBridge.ts

packages/scenes/src/variables/adhoc/AdHocFiltersVariable.tsx

torkelo · 2024-12-06T05:27:44Z

packages/scenes/src/querying/SceneQueryRunner.ts

  private _onActivate() {
    if (this.isQueryModeAuto()) {
      const timeRange = sceneGraph.getTimeRange(this);
+      const scopesBridge = sceneGraph.getScopesBridge(this);


Long term I think it would be cool to generalize this concept with a special variable type that all query runners and variables depend on by default (so will block initial query execution), and the variable can then modify the query.

I tried to start a poc around it, a QueryEnhancerVariable but did not get very far.

But I think this implementation is also ok, just a shame it adds more complexity to this already complex class :)

I have something in mind regarding this. I'll start a PoC in the next weeks 🤔 .

bfmatei

I'll just continue doing testing for this and see how it goes. I also need to fix the code in core and test that out as well.

packages/scenes-app/src/components/SomeReactContext.tsx

bfmatei · 2024-12-06T09:10:19Z

packages/scenes/src/querying/SceneQueryRunner.ts

  private _onActivate() {
    if (this.isQueryModeAuto()) {
      const timeRange = sceneGraph.getTimeRange(this);
+      const scopesBridge = sceneGraph.getScopesBridge(this);


I have something in mind regarding this. I'll start a PoC in the next weeks 🤔 .

packages/scenes/src/core/SceneScopesBridge.ts

packages/scenes/src/variables/adhoc/AdHocFiltersVariable.tsx

torkelo · 2024-12-09T13:29:39Z

packages/scenes/src/components/SceneApp/types.ts

  layout?: PageLayoutType;
+
+  // Whether to use scopes for this page
+  useScopes?: boolean;


do we really need this as a page level prop?

I'm thinking that some pages might not want scopes to be shown for various reasons 🤔

dprokop

@bfmatei - this looks good now! Excellent job on bringing this to scenes. I've left just a few nits to consider.

packages/scenes/src/core/SceneScopesBridge.ts

packages/scenes/src/components/SceneApp/SceneAppPage.tsx

packages/scenes/src/core/SceneScopesBridge.ts

dprokop · 2024-12-09T13:28:10Z

packages/scenes/src/querying/SceneQueryRunner.ts

    }

-    this.runWithTimeRange(timeRange);
+    this.runWithTimeRangeAndScopes(timeRange, scopesBridge);


Let's add tests for making sure the SQR works as expected w/ scopes.

packages/scenes/src/core/SceneScopesBridge.ts

aocenas · 2025-02-18T10:14:32Z

packages/scenes/src/core/SceneScopesBridge.ts

+    if (this._pendingScopes && newContext) {
+      setTimeout(() => {
+        newContext?.changeScopes(this._pendingScopes!);
+        this._pendingScopes = null;


Should we just return here and end the update? It seems like the _pendingScopes have priority (cause they are from the URL) so we need to propagate them into the context, so it does not seem important what is the current newContext value.

Haven't tested out this case. At a first glance I would say we still need to save the new context to get access to other props (i.e. enabled or loading) even if scopes themselves were not loaded.

Something in the lines of:

load page with scopes in URL

trigger scopes loading in the context which results in setting the loading flag to true

the bridge prevents the query runner to perform queries given that scopes are loading

packages/scenes/src/core/SceneScopesBridge.ts

tskarhed · 2025-02-21T09:04:42Z

packages/scenes/src/core/SceneScopesBridge.ts

+
+  public updateContext(newContext: ScopesContextValue | undefined) {
+    if (this._pendingScopes && newContext) {
+      setTimeout(() => {


Why is this timeout needed?

IIRC it was a race condition between setting new scopes which triggers scopes loading hence setting the loading flag to true and pushing the new context value through _contextSubject.

tskarhed · 2025-02-25T16:19:59Z

packages/scenes/src/core/SceneScopesBridge.ts

+    this.context?.setReadOnly(readOnly);
+  }
+
+  public updateContext(newContext: ScopesContextValue | undefined) {


When setting new scopes, do I have to call this? Wouldn't it be easier with an updateScopes method?

updateScopes was deliberately skipped until we have a need to expose it publicly. This should never be called by plugins. However, marking it as private would prevent us from accessing it in the model.

An alternative would be to expose a SceneScopesBridgeLike interface that only contains public methods and use that as a return value for getScopesBridge or for the state prop in AppComponent.

We can't set this to private, as it is used when setting the context from useScope

mdvictor

This is amazing work! Small comment about removing the filter enriching call in adhocs

mdvictor · 2025-03-05T16:01:47Z

packages/scenes/src/variables/adhoc/AdHocFiltersVariable.tsx

      queries,
      timeRange,
+      scopes: this._scopesBridge?.getValue(),
      ...getEnrichedFiltersRequest(this),


Should we still have calls to getEnrichedFiltersRequest now that scopes are pulled directly here? Also other related filter enriching code? I'd reckon there's no use for them anymore, e.g.: packages/scenes/src/variables/getEnrichedFiltersRequest.ts

Since the filters enrichers have all been removed here that call won't do anything so I guess we could leave it there for when/if we will ever enrich filters that way again?

mdvictor

LGTM!

mdvictor · 2025-03-06T14:25:21Z

packages/scenes/src/core/SceneScopesBridge.ts

+          ([prevContext, newContext]) =>
+            [prevContext?.state.value ?? [], newContext?.state.value ?? []] as [Scope[], Scope[]]
+        ),
+        filter(([prevScopes, newScopes]) => !isEqual(prevScopes, newScopes))


Do we need this filtering here? I've not encountered a scenario where this might happen (e.g.: applying the same scopes from the selector would be one, I think)

But it does cause issues when trying to inject scope filters into adhoc. Basically when the dashboard loads, the scopes aren't yet loaded so they are not pushed to the adhoc, and when subscribeToValue triggers it compares prev and new scopes as being the same since it's the first value anyway. And because of this filtering we don't pass the first scopes values and the adhocs don't get populated

Can we get around this by not filtering for undefined context above? I think having this safeguard can prevent unnecessary accidental updates. Maybe we can do a check if it is the first update here? We can do a distinctUntilChanged before mapping to the values. So it will end up emitting [] and [], but at least it will emit a value when going from an undefined context and the loading has finished..

when subscribeToValue triggers it compares prev and new scopes as being the same since it's the first value anyway.

I think you can do something like this though right:

current = SceneScopesBridge.context SceneScopesBridge.subscribeToValue(() => do something with value)

I feel like both the pairwise and the filters would mess up a bit the BehaviourSubject contract here. pairwise seem to need 2 events until it emits something, if we filter out undefined and loading context values this needs 3 changes to emit first time.

So I would agree that removing the first filter probably makes sense as you want to know if it was undefined before or goes to undefined value just has to be mapped right. Also as this suggests https://stackoverflow.com/questions/50059622/rxjs-observable-which-emits-both-previous-and-current-value-starting-from-first I would start it off with some empty value see this actually emits something from the start.

scenes-repo-bot-access-token · 2025-03-11T10:06:38Z

🚀 PR was released in v6.3.1 🚀

bfmatei requested review from dprokop and torkelo December 4, 2024 13:43

bfmatei self-assigned this Dec 4, 2024

bfmatei mentioned this pull request Dec 4, 2024

Scopes: Refactor scopes to plain React and move it to runtime grafana/grafana#97176

Merged

3 tasks

torkelo reviewed Dec 4, 2024

View reviewed changes

dprokop reviewed Dec 5, 2024

View reviewed changes

torkelo reviewed Dec 6, 2024

View reviewed changes

bfmatei commented Dec 6, 2024

View reviewed changes

bfmatei mentioned this pull request Dec 6, 2024

Scopes: Add ScopesContext to @grafana/runtime grafana/grafana#97566

Merged

3 tasks

torkelo reviewed Dec 9, 2024

View reviewed changes

dprokop reviewed Dec 9, 2024

View reviewed changes

bfmatei force-pushed the bogdan/react-contexts-bindings-poc branch from 6265009 to c3a4b47 Compare December 9, 2024 14:40

bfmatei added patch Increment the patch version when merged release Create a release when this pr is merged labels Dec 9, 2024

bfmatei changed the title ~~ReactContexts: PoC~~ Scopes: Add ScopesBridge object Dec 9, 2024

bfmatei force-pushed the bogdan/react-contexts-bindings-poc branch from 6412637 to 6fbea37 Compare February 17, 2025 09:16

aocenas reviewed Feb 18, 2025

View reviewed changes

tskarhed reviewed Feb 21, 2025

View reviewed changes

tskarhed reviewed Feb 25, 2025

View reviewed changes

bfmatei added 9 commits February 27, 2025 11:43

ReactContexts: PoC

53d9fc4

Remove unused useContext hook

e296557

Refactor

b8ce0e7

Refactor

b376a02

Refactor

b05d05c

Refactor

f3782a7

Remove complexity

8c011dd

Update to latest ScopesContext

022b851

Refactors

6d0cc94

bfmatei and others added 4 commits February 27, 2025 11:44

Fix tests

3a5c446

Fix typecheck

76b0313

Remove unused ts-expect-error

b484f05

SceneQueryRunner: Add tests for scopes (#1061)

5411c17

bfmatei force-pushed the bogdan/react-contexts-bindings-poc branch from ea428ac to 5411c17 Compare February 27, 2025 09:44

bfmatei and others added 3 commits February 27, 2025 14:12

Add comments for updateContext

9a6f626

Fix

3b2c35c

Update snapshot

a1a96fd

tskarhed mentioned this pull request Mar 5, 2025

ScopesContext: Move runtime service to unstable grafana/grafana#101600

Merged

3 tasks

mdvictor reviewed Mar 6, 2025

View reviewed changes

This was referenced Mar 6, 2025

AdhocVariable: Inject scopes in baseFilters #1072

Closed

Remove scopes to adhoc conversion code grafana/grafana#101689

Closed

mdvictor approved these changes Mar 6, 2025

View reviewed changes

mdvictor mentioned this pull request Mar 6, 2025

Remove scopes to adhoc conversion code grafana/grafana#101699

Merged

mdvictor reviewed Mar 6, 2025

View reviewed changes

aocenas added this to Grafana Frontend Platform Mar 10, 2025

github-project-automation bot moved this to 🔍 In review in Grafana Frontend Platform Mar 10, 2025

aocenas and others added 3 commits March 10, 2025 12:06

Make sure subscribeToValue emits initial value

8288ebd

fix using url scopes only if they exist

055e2e6

revert latest commit. will be better addressed afterwards

373f7dd

aocenas merged commit 6ed48bf into main Mar 11, 2025
5 checks passed

aocenas deleted the bogdan/react-contexts-bindings-poc branch March 11, 2025 10:01

github-project-automation bot moved this from 🔍 In review to 🚀 Done in Grafana Frontend Platform Mar 11, 2025

scenes-repo-bot-access-token bot added the released label Mar 11, 2025

tskarhed mentioned this pull request Mar 12, 2025

Restore Scopes functionality once Scopes is available in grafana packages grafana/metrics-drilldown#45

Closed

Conversation

bfmatei commented Dec 4, 2024 • edited by scenes-repo-bot-access-token bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

torkelo left a comment

Choose a reason for hiding this comment

Uh oh!

torkelo commented Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dprokop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfmatei commented Dec 5, 2024

Uh oh!

torkelo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bfmatei left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dprokop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdvictor left a comment

Choose a reason for hiding this comment

Uh oh!

mdvictor Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdvictor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bfmatei commented Dec 4, 2024 •

edited by scenes-repo-bot-access-token bot

Loading

torkelo commented Dec 4, 2024 •

edited

Loading

mdvictor Mar 5, 2025 •

edited

Loading