#4464 temp render slice 1 #4584

BLoe · 2022-10-31T23:45:25Z

What does this PR do?

Closes Temporary pop-up Slice 1: Add the “Display Temporary Information” brick #4464

Reviewer Tips

Start by looking at the new runRendererPipeline field on BlockOptions in core.ts
Look at the implementation of this function in reducePipeline.ts
Look at the new brick DisplayTemporaryInfo
If you want to follow through all the wiring up to the sidebar from there, it's probably easier to check out the branch and use your IDE, but it follows the exact same pattern as the regular panels, just with a x close button in the sidebar tab header

Demo

https://www.loom.com/share/5d663eb547954b26b01d2e4f8eb25b2c

Future Work

I think I want to take a shot at untangling some of the type dependencies soon...

Checklist

Add tests
Run Storybook and manually confirm that all stories are working
Designate a primary reviewer - @BALEHOK

codecov · 2022-10-31T23:52:10Z

Codecov Report

Merging #4584 (98165f3) into main (5ca79ec) will increase coverage by 0.04%.
The diff coverage is 46.32%.

@@            Coverage Diff             @@
##             main    #4584      +/-   ##
==========================================
+ Coverage   50.87%   50.92%   +0.04%     
==========================================
  Files         916      917       +1     
  Lines       27183    27307     +124     
  Branches     5519     5533      +14     
==========================================
+ Hits        13830    13905      +75     
- Misses      12420    12465      +45     
- Partials      933      937       +4

Impacted Files	Coverage Δ
src/blocks/transformers/registerTransformers.ts	`0.00% <0.00%> (ø)`
...ponents/documentBuilder/render/DocumentContext.tsx	`63.63% <0.00%> (-14.15%)`	⬇️
src/contentScript/executor.ts	`0.00% <0.00%> (ø)`
src/contentScript/messenger/registration.ts	`0.00% <ø> (ø)`
src/contentScript/sidebarController.tsx	`9.70% <0.00%> (-0.96%)`	⬇️
src/core.ts	`100.00% <ø> (ø)`
src/development/headers.ts	`0.00% <0.00%> (ø)`
src/pageEditor/exampleExtensionConfig.ts	`0.00% <ø> (ø)`
src/sidebar/PanelBody.tsx	`65.21% <0.00%> (-1.95%)`	⬇️
src/sidebar/messenger/api.ts	`100.00% <ø> (ø)`
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

BLoe · 2022-10-31T23:52:46Z

src/runtime/reducePipeline.ts

+              rootOverride ?? root,
+              {
+                ...options,
+                headless: true,


This is the only important difference from the call in runPipeline above.

I would comment on this in the code. Helpful for people coming back to it later

twschiller · 2022-11-01T00:50:12Z

src/blocks/renderers/table.test.ts

@@ -32,6 +32,7 @@ describe("parse compile error", () => {
        root: null,
        logger: new ConsoleLogger(),
        runPipeline: neverPromise,
+        runRendererPipeline: neverPromise,


twschiller · 2022-11-01T00:50:20Z

src/blocks/transformers/FormData.test.ts

@@ -53,6 +53,7 @@ describe("FormData block", () => {
      logger: null,
      root: null,
      runPipeline: neverPromise,
+      runRendererPipeline: neverPromise,


twschiller · 2022-11-01T00:51:37Z

src/sidebar/types.ts

@@ -49,22 +49,33 @@ export type RendererError = {
 * @see PanelEntry
 * @see FormEntry
 */
-export type EntryType = "panel" | "form";
+export type EntryType = "panel" | "form" | "temporaryPanel";


twschiller · 2022-11-01T00:53:07Z

src/sidebar/sidebarSlice.ts

+        state.temporaryPanels,
+        (x) => x.extensionId === panel.extensionId
+      );
+


Not sure if it needs to be included in this PR or the next, but we need to call cancel on the other panels from the extension:

See equivalent behavior here for forms:
https://github.com/pixiebrix/pixiebrix-extension/pull/4584/files#diff-f38df102e6740716f2c8515f48e1aa4d043d02404c8b8e2cfbf4e4c0db041c8fR121

twschiller · 2022-11-01T00:53:37Z

src/sidebar/sidebarSlice.ts

@@ -43,6 +44,7 @@ export type SidebarState = SidebarEntries & {
 export const emptySidebarState: SidebarState = {
  panels: [],
  forms: [],
+  temporaryPanels: [],


twschiller · 2022-11-01T00:54:00Z

src/sidebar/protocol.tsx

@@ -113,3 +120,14 @@ export async function showForm(sequence: number, entry: FormEntry) {
 export async function hideForm(sequence: number, nonce: UUID) {
  runListeners("onHideForm", sequence, { nonce });
 }
+
+export async function showTemporaryPanel(


src/sidebar/Tabs.tsx

twschiller · 2022-11-01T00:56:20Z

src/sidebar/ConnectedSidebar.tsx

@@ -81,12 +98,15 @@ const ConnectedSidebar: React.VFC = () => {
          // Use ignoreApiError to avoid showing error on intermittent network issues or PixieBrix API degradation
          ignoreApiError
        >
-          {sidebarState.panels?.length || sidebarState.forms?.length ? (


twschiller · 2022-11-01T00:56:57Z

src/pageEditor/tabs/editTab/blockFilterHelpers.ts

@@ -56,6 +57,11 @@ export function getSubPipelineFlavor(
    return PipelineFlavor.NoEffect;
  }

+  if (parentNodeId === DisplayTemporaryInfo.BLOCK_ID) {
+    // Temporary Info renderer shouldn't have side effects


twschiller · 2022-11-01T00:57:20Z

src/development/headers.ts

@@ -21,7 +21,7 @@ import "./cryptoNodePolyfill.js";
 import blockRegistry from "@/blocks/registry";

 // Maintaining this number is a simple way to ensure bricks don't accidentally get dropped
-const EXPECTED_HEADER_COUNT = 104;
+const EXPECTED_HEADER_COUNT = 105;


src/pageEditor/exampleBlockConfigs.ts

twschiller · 2022-11-01T00:57:59Z

src/pageEditor/exampleBlockConfigs.ts

+  if (blockId === DisplayTemporaryInfo.BLOCK_ID) {
+    return {
+      title: "Example info",
+      body: makePipelineExpression([createNewBlock(DocumentRenderer.BLOCK_ID)]),


BALEHOK · 2022-11-02T14:58:58Z

src/runtime/reducePipeline.ts

+            );
+
+            // Already using errors for control flow, keeping in this way for conciseness
+            // noinspection ExceptionCaughtLocallyJS


Is this a WebStorm thing?
Can it be disabled in a config rather than in code?

It's to suppress an IntelliJ inspection. I can turn off the inspection in project settings instead I guess.

Actually, this is already used in sidebarExtension.ts - line 192 (where this logic is borrowed from), so I don't think it's that big a deal to leave it here as well.

BALEHOK · 2022-11-02T16:34:09Z

src/runtime/reducePipeline.ts

+
+            // Already using errors for control flow, keeping in this way for conciseness
+            // noinspection ExceptionCaughtLocallyJS
+            throw new BusinessError("Pipeline does not include a renderer");


I'm a little bit confused. Does it throw in case when reducePipelineExpression successfully completes?

Yes, because that means the pipeline does not have a renderer, and this pipeline expects a renderer. It's definitely a little weird to use errors for control flow like this (hence the IDE warning), but it's already how it's done for regular sidebar panels in sidebarExtension.ts, so I'm using the same logic here.

This reverts commit 1ce4396.

twschiller · 2022-11-03T12:04:54Z

src/blocks/transformers/temporaryInfo/DisplayTemporaryInfo.ts

+    } finally {
+      controller.abort();
+    }
+  }


I'd prefer returning {} instead of undefined. Not a big deal since neither can be used, so returning a value in the future is not backward incompatible

twschiller · 2022-11-03T12:07:34Z

src/blocks/transformers/temporaryInfo/temporaryPanelProtocol.ts

+ * Resolve some temporary panels' deferred promises
+ * @param nonces The nonces of the panels to resolve
+ */
+export async function resolveTemporaryPanels(nonces: UUID[]) {


NIT: I'd prefer this method name to refer to what the method does from a business perspective vs. the code perspective. E.g., what's the impact of resolving the temporary panels?

I think it's something to the effect of "closeTemporaryPanels" or "completeTemporaryPanels"?

"resolve" to me seems to only mean something in the world of promises

I guess this matches what we have for form though. So definitely just a thought/NIT...

Yeah, I guess I was just trying to follow the way the form one is named. I agree it's not the greatest name though. I can change both names to something better.

The form function is named that way to distinguish between resolve and cancel I think, which both "close" the form.

twschiller · 2022-11-03T12:08:21Z

src/blocks/transformers/temporaryInfo/temporaryPanelProtocol.ts

+
+  panels.set(nonce, registration);
+
+  return registration.promise;


twschiller · 2022-11-03T12:11:01Z

src/blocks/transformers/temporaryInfo/DisplayTemporaryInfo.test.ts

+  resolveTemporaryPanels: jest.fn(),
+}));
+
+describe("DisplayTemporaryInfo", () => {


twschiller

See comment on return value from the brick of undefined vs. {}

Excited to get this merged!

Ben Loe added 9 commits October 31, 2022 15:56

implement temp display info and fix id types

bf4b2f7

Merge branch 'main' into feature/4464-temp-render-slice-1

e9f8aa9

fix merge issue

0d67c08

add stub run functions

12351c7

revert type change

b7dd88b

revert import order changes

6438fb7

fix import orders

ad8319e

fix export

d9759db

add tests

2f9c893

BLoe requested review from twschiller and BALEHOK October 31, 2022 23:45

vercel bot deployed to Preview October 31, 2022 23:47 View deployment

BLoe commented Oct 31, 2022

View reviewed changes

fix import order and lint issue

e8897b2

vercel bot deployed to Preview November 1, 2022 00:18 View deployment

fix circular dep

9d2ea02

vercel bot deployed to Preview November 1, 2022 00:21 View deployment

twschiller reviewed Nov 1, 2022

View reviewed changes

src/sidebar/Tabs.tsx Outdated Show resolved Hide resolved

twschiller reviewed Nov 1, 2022

View reviewed changes

src/pageEditor/exampleBlockConfigs.ts Outdated Show resolved Hide resolved

twschiller reviewed Nov 1, 2022

View reviewed changes

vercel bot deployed to Preview November 1, 2022 01:27 View deployment

PR tweaks

c8c2f67

vercel bot deployed to Preview November 1, 2022 15:49 View deployment

block pipeline execution while temp panel is open

c2ac95e

vercel bot deployed to Preview November 1, 2022 22:52 View deployment

fix tests

dc59d4d

vercel bot deployed to Preview November 1, 2022 23:32 View deployment

BLoe requested a review from twschiller November 2, 2022 00:18

BALEHOK reviewed Nov 2, 2022

View reviewed changes

disable this idea inspection

1ce4396

vercel bot deployed to Preview November 2, 2022 20:34 View deployment

Ben Loe added 2 commits November 2, 2022 16:36

Revert "disable this idea inspection"

af91d9e

This reverts commit 1ce4396.

make comment more clear

6ff8457

vercel bot deployed to Preview November 2, 2022 20:40 View deployment

Merge branch 'main' into feature/4464-temp-render-slice-1

aa36d96

vercel bot deployed to Preview November 3, 2022 04:52 View deployment

twschiller reviewed Nov 3, 2022

View reviewed changes

twschiller approved these changes Nov 3, 2022

View reviewed changes

BALEHOK approved these changes Nov 3, 2022

View reviewed changes

twschiller added this to the 1.7.12 milestone Nov 3, 2022

Ben Loe added 2 commits November 3, 2022 15:59

Merge branch 'main' into feature/4464-temp-render-slice-1

3391db3

return an empty object from brick and rename functions

a322daa

vercel bot deployed to Preview November 3, 2022 20:13 View deployment

fix mock

98165f3

vercel bot deployed to Preview November 3, 2022 20:37 View deployment

BLoe merged commit f3aba14 into main Nov 3, 2022

BLoe deleted the feature/4464-temp-render-slice-1 branch November 3, 2022 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4464 temp render slice 1 #4584

#4464 temp render slice 1 #4584

BLoe commented Oct 31, 2022 •

edited

codecov bot commented Oct 31, 2022 •

edited

BLoe Oct 31, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022 •

edited

twschiller Nov 1, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022

twschiller Nov 1, 2022

BALEHOK Nov 2, 2022

BLoe Nov 2, 2022

BLoe Nov 2, 2022

BALEHOK Nov 2, 2022

BLoe Nov 2, 2022 •

edited

twschiller Nov 3, 2022

twschiller Nov 3, 2022 •

edited

BLoe Nov 3, 2022

BLoe Nov 3, 2022 •

edited

twschiller Nov 3, 2022

twschiller Nov 3, 2022

twschiller left a comment


		panels.set(nonce, registration);

		return registration.promise;

#4464 temp render slice 1 #4584

#4464 temp render slice 1 #4584

Conversation

BLoe commented Oct 31, 2022 • edited

What does this PR do?

Reviewer Tips

Demo

Future Work

Checklist

codecov bot commented Oct 31, 2022 • edited

Codecov Report

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

twschiller Nov 1, 2022 • edited

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

Choose a reason for hiding this comment

BLoe Nov 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Nov 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BLoe Nov 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller left a comment

Choose a reason for hiding this comment

BLoe commented Oct 31, 2022 •

edited

codecov bot commented Oct 31, 2022 •

edited

twschiller Nov 1, 2022 •

edited

BLoe Nov 2, 2022 •

edited

twschiller Nov 3, 2022 •

edited

BLoe Nov 3, 2022 •

edited