Skip to content

[SAFE-EMAIL-TEST-1] PLU-386: setup infra to display app-specific tooltips in check step#1645

Open
ogp-weeloong wants to merge 1 commit into
develop-v2from
feat/ses/safe-test-emails/tooltip
Open

[SAFE-EMAIL-TEST-1] PLU-386: setup infra to display app-specific tooltips in check step#1645
ogp-weeloong wants to merge 1 commit into
develop-v2from
feat/ses/safe-test-emails/tooltip

Conversation

@ogp-weeloong
Copy link
Copy Markdown
Contributor

@ogp-weeloong ogp-weeloong commented May 26, 2026

Problem

When testing email, we want to always send the email only to the pipe owner. This prevents them from accidentally sending a real email to their target audience when testing.

Solution

High level: when calling postman's API, clear CC list and force recipient to pipe owner if the run is a test run.

We want to add a UX signal to let the pipe owner know that when they click test step in postman, the email will be sent to them.

I decided on showing a tooltip. However, I'm not 100% sure if tooltip is good enough - we may want to do a NUX / first time modal instead.

This PR

This sets up the infra to enable apps to extend the front end.

High level approach
We do something similar to apps in backend.

  1. We add hooks into our frontend for which apps can extend the functionality of.
  2. These per-app extensions are stored in frontend/src/app-extensions/<appName>
  3. Apps register their extensions by adding to the APP_EXTENSIONS map in the app-extensions index file.

This PR implements this approach and sets up the first extension site - the check step button.
BONUS: We can move the FormSG "check step again" button to this approach.

IMPORTANT: Feel free to leave comments if you have better idea, code is easy to change.

Before & After Screenshots

No-op, so here's some screens showing existing tooltips still work

Screenshot 2026-05-26 at 10 41 07 AM Screenshot 2026-05-26 at 10 41 48 AM

Tests

Generally regression tests on test step tooltips since I refactored it a bit.

  • Check that existing tooltips (e.g. unpublish / remove variables from deleted step etc) still work

@ogp-weeloong ogp-weeloong requested a review from a team as a code owner May 26, 2026 02:41
@linear
Copy link
Copy Markdown

linear Bot commented May 26, 2026

PLU-386

@ogp-weeloong ogp-weeloong changed the title PLU-386: create testStepTooltip property PLU-386: setup infra to display app-specific tooltips in check step May 26, 2026
@ogp-weeloong ogp-weeloong marked this pull request as draft May 26, 2026 03:07
@ogp-weeloong ogp-weeloong force-pushed the feat/ses/safe-test-emails/tooltip branch 2 times, most recently from f4b9e29 to c847a08 Compare May 26, 2026 15:56
@ogp-weeloong ogp-weeloong marked this pull request as ready for review May 27, 2026 05:05

if (isFormSgTrigger) {
return <FormSGCheckAgainButton {...props} />
return <FormSGCheckAgainButton ref={ref} {...props} />
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will migrate this to extension approach in a later PR

</Tooltip>
)
}
const CheckStepTooltip = forwardRef<HTMLDivElement, CheckStepTooltipProps>(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the forwardRef is needed here because eventually I'll wrap this in another tooltip, which requires its child to support refs

Copy link
Copy Markdown
Contributor Author

ogp-weeloong commented May 27, 2026


How to use the Graphite Merge Queue

Add the label lfg to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@ogp-weeloong ogp-weeloong changed the title PLU-386: setup infra to display app-specific tooltips in check step [SAFE-EMAIL-TEST-1] PLU-386: setup infra to display app-specific tooltips in check step May 28, 2026
@kevinkim-ogp
Copy link
Copy Markdown
Contributor

Code review

Architectural question: should app-extensions live in the frontend or be driven by the backend?

Frontend-only is the right call here. The reasoning:

  1. The extension is pure UI. CheckStepButton is a React component; the backend cannot send component code over the wire. A backend-driven approach could only send metadata flags (e.g. showSafeEmailTooltip: true), and the frontend would still own all the rendering logic — adding a round trip for no gain.

  2. The backend already sends the discriminating data. step.appKey and step.key arrive via GraphQL on IStep. No new API needed.

  3. Consistent with existing patterns. App-specific frontend logic keyed on appKey/stepKey already exists throughout the codebase (CheckAgainButton, MrfContext, FlowStepGroup, etc.). This registry is a cleaner, more scalable version of those hardcoded if branches.

  4. The setupMessage precedent confirms the boundary. Backend-driven UI (e.g. setupMessage) is appropriate when the backend has knowledge the frontend lacks. A "this sends a real test email" tooltip is static content the frontend can own entirely.


No critical bugs found. One latent concern worth noting (below the threshold for a blocking finding):

CheckStepButtonExtension is assigned inside the function body without useMemo:

https://github.com/ogp-plumber/plumber/blob/c847a081a3987392e5f6e3512a66b3eea88b38eb/packages/frontend/src/components/FlowStepTestController/index.tsx#L266-L270

When shouldAllowCheckStep toggles, the JSX element type switches between appExtension.CheckStepButton and NoOpCheckStepButtonExtension. React treats a type change as a full unmount/remount of the child subtree. This is dormant today (APP_EXTENSIONS is empty), but will cause the CheckAgainButton tree to remount on save/validation-change once a real extension is registered. A useMemo([shouldAllowCheckStep, appExtension]) would prevent this.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Copy link
Copy Markdown
Contributor

@kevinkim-ogp kevinkim-ogp left a comment

Choose a reason for hiding this comment

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

lgtm, approach makes sense and tested no regressions 🫡

think can ignore the useMemo from the claude review

We want to allow apps to extend our front end (e.g. with tooltips)

This creates a new "app-extensions" system in front end to enable
this. The idea is to allow apps to provide react components that we
will render in appropriate parts of the frontend.

This kicks it off by extending the "check step" button

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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