Skip to content

feat: Minimize site form layout shift - #295

Merged
dcalhoun merged 1 commit into
trunkfrom
feat/minimize-site-form-layout-shift
Jun 25, 2024
Merged

feat: Minimize site form layout shift#295
dcalhoun merged 1 commit into
trunkfrom
feat/minimize-site-form-layout-shift

Conversation

@dcalhoun

@dcalhoun dcalhoun commented Jun 24, 2024

Copy link
Copy Markdown
Member

Proposed Changes

Minimize site form layout shift
The "Add site" dialog relies upon asynchronous name generation, which
ultimately populates the site path input. Conditionally rendering the
site path input based upon the populated site path value meant the input
popped into place during the modal open animation. Conditionally
rendering the input based upon the event handler only ensures the input
is present throughout the modal open animation.

Testing Instructions

Tip

Observing the difference in the provided screen recordings below requires slowly dragging the video scrubber.

Before After
initial-open-before initial-open-after
initial-open-before.mov
initial-open-after.mov
repeat-open-after.mov

1. Add site dialog always displays site path input

  1. Click "Add site" in the sidebar.
  2. Verify the site path input is always in-place, even during the modal
    animation the first time the modal is opened.

2. Edit site name excludes site path input

  1. Select or add a site.
  2. Navigate to the Settings panel.
  3. Click "Edit" next to the site name.
  4. Verify the site path input is not displayed.

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
The "Add site" dialog relies upon asynchronous name generation, which
ultimately populates the site path input. Conditionally rendering the
site path input based upon the populated site path value meant the input
popped into place during the modal open animation. Conditionally
rendering the input based upon the event handler only ensures the input
is present throughout the modal open animation.
@dcalhoun dcalhoun self-assigned this Jun 24, 2024
@dcalhoun
dcalhoun marked this pull request as ready for review June 24, 2024 21:40
@dcalhoun
dcalhoun requested a review from a team June 24, 2024 21:40
@dcalhoun

Copy link
Copy Markdown
Member Author

I consider the proposed changes an easy, low-quality solution, but I feel they are sufficient for the time being.

A more ideal solution might involve rendering a placeholder until the asynchronous work is complete. We might also postpone rendering of the actual dialog content until after both the modal open animation and asynchronous name generation complete to achieve optimal FPS.

@katinthehatsite katinthehatsite left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes look good and fix the issue for me. Thanks for adding the fix!

@dcalhoun
dcalhoun merged commit 851c054 into trunk Jun 25, 2024
@dcalhoun
dcalhoun deleted the feat/minimize-site-form-layout-shift branch June 25, 2024 13:57
fluiddot pushed a commit that referenced this pull request Jun 27, 2024
The "Add site" dialog relies upon asynchronous name generation, which
ultimately populates the site path input. Conditionally rendering the
site path input based upon the populated site path value meant the input
popped into place during the modal open animation. Conditionally
rendering the input based upon the event handler only ensures the input
is present throughout the modal open animation.
wojtekn added a commit that referenced this pull request Jul 29, 2026
… path traversal alert #295 (#4384)

## Related issues

- Related to Dependabot alert
[#295](https://github.com/Automattic/studio/security/dependabot/295)

## How AI was used in this PR

Claude Code traced `@hono/node-server` through the tree (transitive,
pulled only by `@modelcontextprotocol/sdk` in the data-liberation-agent
package), determined that the SDK's older pin (`^1.19.9`) couldn't reach
a patched hono, and found that SDK 1.30.0 widened its range to `^1.19.9
|| ^2.0.5`. It bumped the SDK pin so npm could dedup hono up to the
patched 2.0.12, verified no direct-dependency pollution or override was
needed, rebuilt the checked-in MCP bundle, and confirmed the package
builds and that the only failing tests are pre-existing browser-fixture
failures identical on trunk. I reviewed the diff myself.

## Proposed Changes

Resolves the `@hono/node-server` path-traversal advisory
(encoded-backslash `%5C` directory escape in `serve-static` on Windows),
which reached the tree transitively through the data-liberation-agent's
MCP SDK dependency. Bumping `@modelcontextprotocol/sdk` from 1.29.0 to
1.30.0 lets the resolver move `@hono/node-server` to 2.0.12 — which also
clears a second, newer advisory (unauthenticated WebSocket-handshake
memory-leak DoS) that affects hono ≤2.0.9. The checked-in MCP bundle is
rebuilt to match, consistent with prior dependency-bump PRs.

No user-visible behavior change. The data-liberation-agent package
targets Node ≥22, well above hono 2.x's Node 18.14.1 minimum, so the
major bump carries no engine impact.

## Testing Instructions

- `npm ci` resolves cleanly.
- `npm ls @hono/node-server` reports a single copy at 2.0.12 (no
lingering 1.19.x, and hono is not added as a direct dependency).
- `npm run build -w data-liberation` succeeds and regenerates
`dist/mcp-server.bundle.mjs`.
- `npm audit` reports no `@hono/node-server` findings.

## Pre-merge Checklist

- [ ] Have you checked for TypeScript, React or other console errors?

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants