Skip to content

fix: stop configurator clipping inside WP admin's layout chrome - #123

Merged
jackgranatowski merged 2 commits into
mainfrom
claude/configurator-sync-pr-460-461-g1f2w4
Jul 1, 2026
Merged

jackgranatowski merged 2 commits into
mainfrom
claude/configurator-sync-pr-460-461-g1f2w4

Conversation

@jackgranatowski

Copy link
Copy Markdown
Contributor

Summary

  • In WP admin, the Design Settings page showed visible whitespace above and to the left of the configurator, with content clipped off the right edge (reported with a screenshot of the mobile Colors panel).
  • Root cause: the configurator's root shell used viewport units (w-screen h-screen), correct for the standalone hosted app but wrong once mounted inside WP admin's normal document flow (.wrap / #wpcontent) — the viewport-sized element overflowed past the content area's right edge while the host's own layout chrome left a gap top-left.
  • Re-synced App.svelte from the framework fix (fix(configurator): stop full-viewport sizing from clipping inside WP admin SLASHED#463), which switches to w-full h-full when running embedded.
  • Added plugin-side CSS in class-token-page.php, scoped to the Tokens admin page only via a dedicated body class, that strips WP's wrap margin/padding around the app and gives its ancestor chain (#wpcontent, #wpbody, #wpbody-content, .wrap, #slashed-admin-app) an explicit height so the new h-full root has real dimensions to fill instead of collapsing to 0.
  • Rebuilt the admin SPA bundle (assets/admin-app/app.js).

Type

  • fix

Checklist

  • Conventional Commit messages (feat:, fix:, docs:, …)
  • npm test (not run — requires WP test harness not available in this environment; npm run lint and npm run verify were run instead)
  • npm run lint passes (stylelint + php -l)
  • npm run verify passes (version metadata in sync)
  • Generated artifacts not hand-edited (assets/admin-app/app.js regenerated via npm run build:admin-app, not hand-edited)
  • CHANGELOG.md updated under ## [Unreleased] — not done, let me know if you'd like this added
  • Built SPA assets committed (admin-app/src source changed via sync)

Notes

Depends on codeslash-dev/SLASHED#463 (framework half of this fix) — admin-app/src/App.svelte was synced from that branch via SLASHED_CONFIGURATOR_SRC=/path/to/SLASHED/configurator/src npm run sync. Once #463 merges to main, a normal GitHub-sourced re-sync will pick it up the same way.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SQXr34nocCi1jrGcp5TPVm


Generated by Claude Code

Re-sync App.svelte from the framework fix (w-full/h-full when
embedded instead of viewport units) and scope plugin-side CSS to the
Tokens admin page that strips WP's wrap margin/padding around the app
and gives its ancestor chain a real height, so the app fills the
content area instead of overflowing past its right edge with a gap
top-left.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQXr34nocCi1jrGcp5TPVm
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@jackgranatowski, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 53 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 616030a8-e4e9-4e37-9e78-fe7b24d257ea

📥 Commits

Reviewing files that changed from the base of the PR and between 553c8ff and 3d6662a.

📒 Files selected for processing (5)
  • SLASHED-for-WP/admin-app/.vendored-manifest.json
  • SLASHED-for-WP/admin-app/src/App.svelte
  • SLASHED-for-WP/admin-app/src/lib/persistence.ts
  • SLASHED-for-WP/assets/admin-app/app.js
  • SLASHED-for-WP/includes/class-token-page.php
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/configurator-sync-pr-460-461-g1f2w4

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix configurator sizing when embedded in WP admin Tokens page

🐞 Bug fix 🕐 40+ Minutes

Grey Divider

AI Description

• Switch embedded configurator shell from viewport sizing to container sizing.
• Scope WP-admin layout overrides to Tokens page and enforce full-height mount chain.
• Regenerate admin SPA bundle and update vendored sync manifest metadata.
Diagram

graph TD
  A["WP Admin (Browser)"] --> B["Tokens admin screen"] --> C["includes/class-token-page.php"] --> D["Inline CSS + enqueue assets"] --> E("#slashed-admin-app mount") --> F["Admin SPA bundle (app.js)"] --> G["Svelte App shell (App.svelte)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Embed configurator via iframe
  • ➕ Hard isolation from WP admin CSS/layout chrome
  • ➕ Viewport units remain correct inside the iframe
  • ➖ Adds iframe messaging complexity for persistence/live preview
  • ➖ More work for auth/context sharing and deep linking
2. CSS-only containment (keep w-screen/h-screen, constrain via max-width/overflow)
  • ➕ Avoids changing app sizing logic
  • ➕ Potentially less app-specific branching
  • ➖ Still brittle across WP admin breakpoints/sidebars
  • ➖ Harder to guarantee no clipping across all panels
3. Set explicit mount height using JS (measure available space)
  • ➕ Can account for dynamic admin bar height/viewport changes precisely
  • ➕ Avoids relying on ancestor-chain height rules
  • ➖ More runtime complexity; must handle resize events
  • ➖ Mixes layout concerns into JS and can regress performance

Recommendation: Current approach is the best trade-off: the app selects container sizing when embedded (correct semantic behavior), and the plugin supplies narrowly scoped CSS to ensure the container has real height. The iframe approach is more robust but materially increases integration complexity.

Files changed (4) +171 / -131

Bug fix (2) +42 / -2
App.svelteUse w-full/h-full when embedded to prevent WP admin clipping +9/-2

Use w-full/h-full when embedded to prevent WP admin clipping

• Imports embedded-detection from persistence and computes an 'embedded' flag. The root shell now uses container sizing (w-full/h-full) when embedded, while keeping viewport sizing for standalone hosting.

SLASHED-for-WP/admin-app/src/App.svelte

class-token-page.phpScope Tokens-page layout CSS and enforce full-height mount chain +33/-0

Scope Tokens-page layout CSS and enforce full-height mount chain

• Adds a Tokens-page-specific body class via admin_body_class and injects inline CSS scoped to that class. The CSS removes WP admin wrap padding/margins and sets explicit heights/overflow rules on the ancestor chain so the embedded app’s h-full layout has a real sizing context.

SLASHED-for-WP/includes/class-token-page.php

Other (2) +129 / -129
.vendored-manifest.jsonRecord local configurator sync source and timestamps +51/-51

Record local configurator sync source and timestamps

• Updates vendored manifest metadata to reflect a local sync source and refreshes per-file source references. This documents provenance for the synced configurator sources.

SLASHED-for-WP/admin-app/.vendored-manifest.json

app.jsRebuild admin SPA bundle with embedded sizing fix +78/-78

Rebuild admin SPA bundle with embedded sizing fix

• Regenerated built JS bundle to include the updated App.svelte behavior and any related build output changes. This is a generated artifact from the admin-app build process.

SLASHED-for-WP/assets/admin-app/app.js

@qodo-code-review

qodo-code-review Bot commented Jun 30, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 1 rule

Grey Divider


Action required

1. Notices clipped by overflow ✓ Resolved 🐞 Bug ☼ Reliability
Description
The Tokens page injects CSS that forces fixed height + overflow:hidden on
#wpbody-content/.wrap/#slashed-admin-app, so any admin notice rendered above .wrap can push the app
down while the app container remains fixed-height and gets clipped with no page scroll. This can
happen in-repo: the Bricks integration registers global admin_notices when Bricks isn’t active, so
the Tokens page can be affected.
Code

SLASHED-for-WP/includes/class-token-page.php[R135-147]

+		// The app shell fills its mount point edge-to-edge (it owns its own
+		// header/sidebar/scroll regions). Strip the WP admin page chrome's
+		// margin/padding around it and give its ancestor chain an explicit
+		// height, so the app's `w-full h-full` root actually has real
+		// dimensions to fill instead of overflowing past the right edge of
+		// the content area like raw viewport units would.
+		wp_add_inline_style(
+			'slashed-admin-app',
+			'body.slashed-tokens-page #wpcontent { padding-left: 0; }' .
+			'body.slashed-tokens-page .wrap { margin: 0; }' .
+			'body.slashed-tokens-page #wpbody-content { padding-bottom: 0; }' .
+			'body.slashed-tokens-page #wpcontent, body.slashed-tokens-page #wpbody, body.slashed-tokens-page #wpbody-content, body.slashed-tokens-page .wrap, body.slashed-tokens-page #slashed-admin-app { height: calc(100vh - var(--wp-admin--admin-bar--height, 32px)); overflow: hidden; }'
+		);
Relevance

⭐⭐ Medium

No prior suggestions found about WP admin notices being clipped by overflow:hidden + fixed-height
ancestor chain on token page.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The injected CSS applies fixed height + overflow hidden to containers that also contain (or precede)
.wrap, and the SPA mount is inside .wrap; additionally, the repo registers admin notices on
admin screens, which can appear above .wrap and thus trigger clipping under the new overflow
rules.

SLASHED-for-WP/includes/class-token-page.php[135-147]
SLASHED-for-WP/includes/class-token-page.php[377-394]
SLASHED-for-WP/integrations/bricks/slashed-bricks.php[304-308]
SLASHED-for-WP/integrations/bricks/slashed-bricks.php[345-352]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Tokens admin page injects inline CSS that sets `height: calc(100vh - ...)` and `overflow: hidden` on `#wpbody-content`, `.wrap`, and `#slashed-admin-app`. If any admin notices render above `.wrap`, the notice consumes vertical space but `.wrap` remains fixed-height, causing the app and/or notices to be clipped with no scroll.

### Issue Context
The SPA is mounted under `.wrap > #slashed-admin-app`, and other parts of this repo can emit `admin_notices` on admin pages.

### Fix Focus Areas
- SLASHED-for-WP/includes/class-token-page.php[135-147]

### Suggested fix direction
- Keep the “full-height app” goal but make the layout resilient to notices:
 - Make `#wpbody-content` a column flex container with fixed height.
 - Let notices take natural height.
 - Make `.wrap` flex to fill remaining space (`flex: 1; min-height: 0;`) and avoid forcing `.wrap` to the full viewport height.
 - Consider applying `overflow: hidden` only to the app mount (`#slashed-admin-app`) rather than the entire ancestor chain (or at least avoid `overflow:hidden` on `#wpbody-content`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Embedded sizing guard mismatch ✓ Resolved 🐞 Bug ≡ Correctness
Description
App.svelte decides between w-full/h-full vs w-screen/h-screen using isEmbedded(), but persistence.ts
defines isEmbedded() as “REST URL present” while loadInitialOverrides() treats the mere presence of
window.slashedApp as the embedded-mode boundary. If a host provides slashedApp boot data without
rest.url, the app will still behave as embedded for state, but will render with viewport sizing and
can overflow/clip in embedded layouts.
Code

SLASHED-for-WP/admin-app/src/App.svelte[R34-40]

+  // Embedded hosts (e.g. the WP admin page) mount us into a sized container in
+  // normal document flow, not the document body — w-screen/h-screen would then
+  // size to the viewport while still being offset by the host's own layout
+  // chrome, overflowing past its right edge. Standalone keeps viewport units
+  // since it owns the whole page.
+  const embedded = isEmbedded();
+
Relevance

⭐⭐ Medium

No historical review evidence about aligning isEmbedded() semantics between rest.url and
window.slashedApp boundaries.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The PR adds const embedded = isEmbedded(); to drive the root sizing class, but persistence.ts
shows isEmbedded() is keyed off rest.url while loadInitialOverrides() explicitly uses
wpBoot() presence (window.slashedApp) as the embedded boundary; these semantics are not aligned.

SLASHED-for-WP/admin-app/src/App.svelte[34-43]
SLASHED-for-WP/admin-app/src/lib/persistence.ts[177-200]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`App.svelte` uses `isEmbedded()` to decide whether to use container sizing (`w-full h-full`) vs viewport sizing (`w-screen h-screen`). But `isEmbedded()` currently means “WP REST persistence is configured” (rest.url exists), whereas other logic (`loadInitialOverrides`) treats the presence of `window.slashedApp` as the embedded boundary.

### Issue Context
This PR introduces layout behavior that depends on `isEmbedded()`.

### Fix Focus Areas
- SLASHED-for-WP/admin-app/src/App.svelte[34-40]
- SLASHED-for-WP/admin-app/src/lib/persistence.ts[177-200]

### Suggested fix direction
- Either:
 - Change `isEmbedded()` to reflect “embedded host present” (e.g., `Boolean(wpBoot())`) and keep REST detection internal to `saveOverrides`, or
 - Add a new exported helper (e.g., `hasEmbeddedHost()` / `hasWpBoot()`) used by `App.svelte` for sizing, while keeping `isEmbedded()` as “REST persistence mode”.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

3. Vendored App.svelte edited 📘 Rule violation § Compliance
Description
SLASHED-for-WP/admin-app/src/App.svelte is listed as a vendored file in
SLASHED-for-WP/admin-app/.vendored-manifest.json, but it is modified in this PR. This violates the
rule forbidding modifications to any file listed in the vendored manifest.
Code

SLASHED-for-WP/admin-app/src/App.svelte[11]

+  import { loadInitialOverrides, injectLivePreview, saveOverrides, isEmbedded } from './lib/persistence';
Relevance

⭐ Low

Repo regularly updates vendored admin-app/src via sync; PR #122 merged many vendored edits after
manifest added in #111.

PR-#111
PR-#122

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The compliance rule forbids modifying any file listed in .vendored-manifest.json. The manifest
explicitly lists src/App.svelte as vendored, and the PR changes that file (e.g., adding
isEmbedded usage and changing the root <div> sizing classes).

Rule 1514148: Do not modify vendored files listed in .vendored-manifest.json
SLASHED-for-WP/admin-app/.vendored-manifest.json[14-17]
SLASHED-for-WP/admin-app/src/App.svelte[11-11]
SLASHED-for-WP/admin-app/src/App.svelte[242-242]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR modifies a file (`SLASHED-for-WP/admin-app/src/App.svelte`) that is declared as vendored in `SLASHED-for-WP/admin-app/.vendored-manifest.json`, which is disallowed.

## Issue Context
Per the compliance rule, any file path present in `.vendored-manifest.json` must not be added/edited/deleted in this PR.

## Fix Focus Areas
- SLASHED-for-WP/admin-app/src/App.svelte[11-11]
- SLASHED-for-WP/admin-app/src/App.svelte[34-40]
- SLASHED-for-WP/admin-app/src/App.svelte[242-242]
- SLASHED-for-WP/admin-app/.vendored-manifest.json[14-17]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread SLASHED-for-WP/includes/class-token-page.php
Comment thread SLASHED-for-WP/admin-app/src/App.svelte
Re-sync App.svelte/persistence.ts (framework now uses hasWpBoot()
instead of overloading isEmbedded()'s REST-specific meaning for
layout sizing) and rebuild the admin SPA bundle.

Also fix the Tokens-page layout CSS: forcing a fixed height +
overflow:hidden on the whole #wpbody-content/.wrap/#slashed-admin-app
chain meant any admin notice rendered above .wrap (core update
notices, the Bricks integration's own admin_notices) would push the
app past the bottom edge with no way to scroll to it. Switch to a
flex column so notices keep their natural height and only .wrap
claims the remaining space.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SQXr34nocCi1jrGcp5TPVm
@jackgranatowski
jackgranatowski merged commit 24192d1 into main Jul 1, 2026
9 checks passed
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