mirror of
https://github.com/webstudio-is/webstudio.git
synced 2026-11-06 12:04:00 +09:00
Oleg Isonen
d4b5b328a4
## What this fixes This PR fixes the bugs from #6633 that we could reproduce: - Spacing reset no longer triggers canvas Delete; autocomplete selection is readable in both themes. - Breakpoint widths save on Enter or blur, without reordering while someone types. Typed addresses no longer reuse an old history suggestion. - Redirect edits use saved project state, and unchanged old redirects do not block unrelated edits. Client-side links now follow project redirects, including sites with a custom 404 page. A new publish keeps polling past an old failed build. - Editors can open older projects and edit supported Markdown bindings without changing protected styles or permissions. Older Code Text edits now survive save, reload, and a later Builder visit; Editors cannot replace Code Text bindings. - Form writes are sent once per submission, even when a response fails. Safe response redirects still work. - Published pages and Builder Preview restore scroll without unwanted smooth animation. Preview keeps smooth anchors, repeated anchors, Back/Forward positions, and page entry animations. - Large-page selection and paste avoid rerendering unchanged Navigator rows and rescanning unchanged asset references. Keyboard focus now follows a moved row when its new parent expands. ## Checklist - [x] Third deep review: verify the pending legacy Editor permission and remounted scroll-restoration fixes; simplify the 11 modified files and deleted hook; demonstrate regression failures and passes. - [x] Third review: review documentation and fixture impact; run affected tests, typechecks, lint, formatting, and package boundaries. - [x] Third review: commit only the pending changes, push to this PR, review hosted results, and pass required CI. - [x] Second deep review: check save permissions, redirects, scroll, and UI state; fix reproduced problems and rerun affected checks. - [x] Deep-review the full PR, simplify repeated code, fix verified problems, and rerun affected checks. - [x] Make project redirects work on client-side link clicks, including sites with a custom 404 page; add a failing regression test, verify both runtimes, and review documentation impact. - [x] Reproduce the changed behaviors; show regression tests fail and pass. - [x] Check Editor, Preview, Navigator, Form, redirect, publish, and theme behavior in focused tests and browser workflows. - [x] Review the full diff for correctness, duplication, and needless complexity. Simplify redirect validation and strengthen the cross-page anchor test. - [x] Inspect light and dark autocomplete screenshots and approve the one intended new visual story. - [x] Review documentation impact. Update the Form guide for uncertain delivery responses; other changes restore expected behavior. - [x] Complete independent review, package typechecks, lint, boundaries, generated API/docs checks, and fixture build. - [x] Run the approved full agent evaluation suite. Eight of nine scenarios passed; the remaining evaluator and baseline limits are below. - [x] Pass required CI on the earlier review commits; the third-review head is tracked above. ## Implementation notes Preview uses one navigation request between the Builder and canvas. It records scroll before a page changes and restores it before entry animations. Router keys are observed only in Preview. This adds 211 net production lines because shorter approaches failed repeated anchors, delayed navigation, and returns from Design. Navigator and focus changes add 87 lines; the shared published-page hook adds 40. The earlier cleanup added 427 net production lines; this review adds 43 net non-test lines. Legacy document bindings are normalized when loading a snapshot and checking Editor capabilities. Older Code Text content is normalized on reads without saving; the first edit removes its stale property in the same version-checked save. Direct Editor patches cannot add or replace Code Text expressions. Editor reads do not migrate the database. Read-only bindings and template styles remain protected. No schema, package, icon, or product copy changes are included. The visual harness has an opt-in focus setting for the autocomplete story. Its default blur behavior is unchanged. The test now mounts scroll restoration after the route content, matching the production Body; it checks an anchor found only on the destination page. ## Review follow-up The review found two reproducible bugs. Older Code Text edited by an Editor could be overwritten by the legacy `code` property on a later Builder visit. A Navigator row moved into a collapsed parent could lose keyboard focus. Both are fixed with regression tests. The review also removed one duplicate parser, a repeated autocomplete color check, and an unnecessary prop scan on page-only saves. The Remix scroll test now mounts restoration in the same order as production. The legacy Code Text test covers literal and expression content, direct patch permissions, save retries, undo/redo, and Editor → reload → Builder → reload. The local end-to-end test passed. The other focused tests, affected package typechecks, lint, formatting, package boundaries, generated API/docs checks, and fixture build passed. A fixture sync picked up unrelated remote sitemap data; that generated drift was removed from this PR. The full agent evaluation was not rerun for this follow-up. ## Client-side redirects The root loader now checks redirects when the path changes, even when a custom 404 route matches. A generated yes/no flag in its own file avoids extra navigation requests on projects without redirects; the browser does not receive the redirect list. Projects with redirects pay for a root-loader check on each path change. Query-only navigation keeps the router's normal revalidation behavior. The new test reproduced the custom-404 failure before the fix and passes afterward. Both React Router and Remix templates, generated fixtures, typechecks, production builds, and the redirect-settings browser test pass. Existing redirect docs already describe this expected behavior, so no docs change is needed. ## Performance With 3,000 styled elements, median selection fell from 986 ms to 413 ms in a controlled local browser test. Full page paste including save measured 3.4 seconds afterward; the earlier run varied from 4 to 15 seconds, so that is not a reliable whole-paste speed multiplier. Skipping an unchanged asset-reference rewrite reduced that isolated step from 1,500 ms to 7 ms with 200 assets, 3,000 properties, and 12,000 styles. The browser still downloads and syncs project data, traverses the tree, validates and imports assets, inserts content, renders, and saves. Cross-project imports can still transfer assets. ## Second review follow-up Changing a breakpoint from Min width to Max width (or back) now saves the type and width together. A mixed Builder/Editor save now checks each Editor change against the Builder changes before it, including older Code Text bindings and changes to Content block boundaries. If an earlier save fails, later entries do not run against a state that was never saved. A full page reload now restores its saved scroll position without animating through the page. Client navigation still uses the existing hook. The two early scripts add no network request; they run next to the router's restore script, then restore the page's smooth-scroll setting. A nonce on Body is passed to all three scripts. Rebuilding Editor permissions costs a fresh scan only for mixed-permission batches; all-Builder batches keep the fast path. Focused checks passed: 40 patch authorization/save tests, five breakpoint browser tests, two early-scroll browser tests, 12 router navigation browser tests, four package typechecks, lint, formatting, and diff checks. The real SSR test also passed with delayed hydration and a nonce-only script policy, with no scroll animation or hydration warning. The full agent evaluation was not rerun; the earlier 8/9 result and its known evaluator limit remain unchanged. Documentation impact was reviewed again; these changes restore expected behavior and need no new user guide. On commit `b61c3aaf07`, the main workflow, all ten Builder browser shards, lint, submodule checks, and the approved visual rerun passed. The first visual comparison still records the already reviewed autocomplete difference; its approval rerun passed. ## Third review follow-up The pending changes close two Editor permission gaps. IDs hidden by read-time Code Text normalization remain reserved, so an Editor cannot reuse one to overwrite a protected saved prop. New Code Text accepts literal content but cannot introduce expressions. A Builder can deliberately replace a legacy prop and leave the replacement editable. Scroll preparation now runs after the destination DOM is inserted and before the router restores scroll. This preserves smooth anchors when restoration remounts with a page. The shared component replaces the separate hook. The navigation test now covers all six cases with and without remounting, removing the duplicate remount setup. The patch adds 51 net non-test lines, close to the 50-line estimate; the extra state records hidden saved IDs, and the extra component orders the three layout effects. Regression checks reproduced the hidden-ID overwrite, new expression insertion, copied-prop ID reuse, and instant remounted anchor before the fixes. They pass with the fixes. Local checks passed: all 2,677 project-build tests, 48 patch tests, the three SDK package suites (including 219 SDK React tests with one existing skip and 46 React Router tests), all five affected package typechecks, lint, formatting, package boundaries, generated API/docs checks, and fixture regeneration. Two Form tests timed out in the first parallel SDK run; both passed in isolation, and the complete SDK suite passed with one worker. Fixture regeneration used the existing local snapshots and downloaded required assets; it produced no tracked fixture changes. Remote fixture synchronization was not rerun. The Code Text, Content Block, share-link, and Link docs already describe the intended behavior, so no docs change is needed. The full agent evaluation was not rerun; its earlier 8/9 result and baseline limits still apply. A separate local model review was unavailable. Hosted automatic code and security reviews completed on `472dbc103e`; neither posted findings, and there are no unresolved review threads. Committed and pushed as `472dbc103e`. The [main workflow](https://github.com/webstudio-is/webstudio/actions/runs/42071262279) passed, including all ten Builder end-to-end shards, both package/CLI/core/app environments, typechecks, lint, generated checks, migrations, and size checks. Development preview fixture checks passed on Ubuntu and Windows. The [visual approval rerun](https://github.com/webstudio-is/webstudio/actions/runs/42071810156) passed. Its initial run flagged only the previously approved autocomplete story; all 267 stories were compared, with no other differences. The story was unchanged by this commit, so its existing approval was reapplied. The worktree is clean, and the remote branch matches the local commit. The excluded issues and other worktrees were not changed. No production deployment was initiated. ## Remaining follow-ups - #6645: The measured asset-remapping slowdown is fixed here, but the reported 30-second end-to-end paste remains unverified. Closed as not planned until the reporter shares a reproducer. - #6646: MCP can now inspect Global root defaults, but the reporter's original AI run remains unverified. - #5379: The broader proposal to remove the root grid remains open; this PR fixes the narrower overflow bug #6070. - #6644 was closed as not planned after local tests passed. The bad SSL report and unrelated feature requests are outside this PR. No shipped-fix announcement or production release is included. ## Verification The previous commit passed the main workflow, all ten browser shards, lint, submodule checks, and the approved visual rerun. The review commit passed the main workflow, all ten Builder browser shards, lint, submodule checks, and the visual-approval run. The initial visual check flagged the already-reviewed new autocomplete story; the approval run passed without a visual code change. An earlier browser run caught a generated-file format change; the follow-up kept that format intact and passed the same test. The full agent evaluation suite was not rerun for this redirect follow-up; Oleg asked to run it once all remaining work is fixed. Local: 2,675 project-build tests, 1,522 Builder core tests (one existing skip), 40 React Router tests, eight affected-package typechecks, lint, format, boundaries, generated API/docs checks, and fixture build passed. The earlier head also passed the Builder app, SDK, component, CLI, Editor/Preview/Navigator browser tests. On the final commit, all ten hosted browser groups, package and app checks, fixtures, migrations, and the approved visual run passed. The first Builder core job hit its seven-minute CI limit without an assertion failure; its single-job retry passed. The first visual job listed the already reviewed new autocomplete story; its approval rerun passed. The complete Sol/high agent suite finished with **8/9 scenarios passing**. The remaining native Form authoring evaluator rejects a plain HTML action URL as though it were a managed Webstudio Action. That fixture did not test actual browser submission, so no runtime claim is made. The comparison gate remains non-green: five baselines use another model and four fixtures have no accepted baseline. No baseline or validator was changed to hide this. The MCP Form result fix for #6640 is now included below. A fast redirect no longer leaves MCP without the Form result. The older local source failure helped reproduce this case. Closes #6633 Closes #6640 Closes #2439 Closes #6642 Ref #6645 Closes #6641 Closes #6643 Ref #6644 Ref #6646 Closes #6647 Ref #5379 Closes #6070 Ref #980 Ref #4309 Ref #6573 ### Consolidation of follow-up fixes - [x] Include and verify the Resource body, expression Tab, and 404 starter fixes from #6649, #6651, and #6650. - [x] Include and verify the root overflow, Navigator lag, and asset-paste fixes from #6652, #6653, and #6654. - [x] Include the Global root MCP inspection fix from #6655, with its discoverable example. - [x] Review `docs/` and fixture impact for the combined changes. - [x] Pass final CI on the combined branch. - [x] Close the superseded PRs after their changes were verified here. The 30-second end-to-end paste report is closed in #6645 until the reporter shares a reproducer. The original MCP agent outcome in #6646 is still unverified. Oleg approved keeping the public MCP example. The fix from #6656 is now included in this PR. The combined branch passed 236 focused tests, Builder/SDK/project-build typechecks, lint, formatting, package boundaries, generated API and MCP-doc checks, and the fixture pipeline. The fixture sync fetched an unrelated live-project change; rebuilding from the checked-in data restored a clean worktree. All nine agent evaluation scenarios passed, but the comparison gate remains non-green because saved baselines are missing or use a different model. The [main workflow](https://github.com/webstudio-is/webstudio/actions/runs/42090765144) passed all jobs, including ten Builder browser shards. The [approved visual rerun](https://github.com/webstudio-is/webstudio/actions/runs/42091462930) passed; its first run flagged only the already-reviewed new autocomplete story. PRs #6649–#6655 were closed as superseded, with their changes retained here. ### Final MCP Form consolidation - [x] Include the tested MCP Form redirect fix from #6656 without changing submission count or result limits. - [x] Run focused Vision/CLI tests and review the combined diff. - [x] Review `docs/` and fixture impact for this fix. The change is limited to Vision's Form result capture and tests; no docs or fixture update is needed. - [x] Pass CI on the new combined head. - [x] Close #6656 once its change is verified here. The user clarified that all bug-fix PRs from this work should be one PR. The MCP Form redirect fix belongs here too. The two Vision files match #6656 exactly. All 121 Vision tests, Vision and CLI typechecks, formatting, and the 59 focused Form capture tests passed. In the full local CLI run, 1,304 tests passed and two unrelated tests failed under parallel load; both passed in a single-worker rerun. The [main CI run](https://github.com/webstudio-is/webstudio/actions/runs/42093902295) passed, including all ten Builder browser shards. The [approved visual run](https://github.com/webstudio-is/webstudio/actions/runs/42095225165) passed. The initial visual run flagged only the previously approved autocomplete story. #6656 is closed as superseded by this PR. No production release was started. ### Large-project Navigator follow-up - [x] Import the provided large project into an isolated local Builder and measure selection-to-Style-Panel time with the tree expanded. - [x] Measure Navigator scrolling and confirm whether the current scroll path is native. - [x] Fix only the reproduced bottlenecks, with regression checks that fail before the change and pass afterward. - [x] Review `docs/` and fixture impact; run focused tests, typecheck, lint, and browser checks. - [x] Open the fixed local Builder for Oleg's manual check, then pass final CI. This follows the original large-page Navigator report in #6641. The separate 30-second paste report #6645 remains closed because it describes a different action and has no paste reproduction yet. The imported project has about 1,244 visible Navigator rows on the measured page. With the tree expanded, selection took roughly 0.9–2.2 seconds before this change and roughly 0.5–1.3 seconds afterward in local browser runs. Browser style recalculation fell from about 0.4–1.0 seconds to about 0.1–0.3 seconds per selection; times vary by run. The Navigator now uses native scrolling and lets the browser skip layout for rows off-screen. Horizontal wheel scrolling was restored and checked after Oleg spotted a regression. A focused regression test failed before the change and passes now. The Navigator tests, Builder typecheck, lint, formatting, diff check, and local browser selection/vertical/horizontal scrolling passed. The `docs/` guide and fixtures need no change because the interaction stays the same. Oleg reports that performance feels good in the local Builder. The [main CI run](https://github.com/webstudio-is/webstudio/actions/runs/42103807815) passed, including all ten Builder browser shards. One unrelated design-system browser test timed out on the first run and passed on retry. The [visual approval run](https://github.com/webstudio-is/webstudio/actions/runs/42105155951) passed after review of tiny Navigator text-pixel differences and the previously approved autocomplete story. No production release was started. ### Final review: Navigator drop marker - [x] Reproduce the clipped marker with a focused regression test. - [x] Keep the marker visible without losing off-screen-row performance. - [x] Review `docs/` and fixture impact, run focused tests, typecheck, lint, formatting, and browser checks. - [x] Push this fix to the same PR and pass required CI. The regression test failed before the fix (`auto` instead of `visible`) and passed afterward. A sortable row now marks when it has a drop target; only that row skips the off-screen containment rule, so its marker can cross the row edge. Other rows retain containment. Navigator and tree browser tests, both typechecks, lint, formatting, and diff checks passed locally. The Navigator guide still describes the same drag-and-drop behavior; no docs or fixture update is needed. This adds two net non-test lines, matching the small-change estimate. The [main CI run](https://github.com/webstudio-is/webstudio/actions/runs/42109032555) passed all 23 jobs. Two MCP Form browser shards first returned an unexpected `unknown` form status, then passed on rerun without a code change. The [visual approval run](https://github.com/webstudio-is/webstudio/actions/runs/42109658799) also passed. No production release was started. ### Drop marker color follow-up - [x] Remove the white outline and white fill so the marker is blue with a transparent center in both themes. - [x] Add a browser regression check that fails before the fix and passes after it. - [x] Review `docs/` and fixture impact; run focused tests, typecheck, lint, and formatting. - [x] Push to this PR and pass required CI. The old styles failed the new light/dark browser test because the line had a white outline. After removing the outline and the white circle backing, all 20 focused indicator/tree tests pass. Design-system typecheck, lint, formatting, and diff checks pass. The Navigator guide describes behavior, not marker colors, so docs need no update. No fixture input or output changes. The [main CI run](https://github.com/webstudio-is/webstudio/actions/runs/42115118863) passed all 23 jobs. Three MCP Form browser shards first returned an unexpected `unknown` status and passed on rerun without a code change. The [approved visual run](https://github.com/webstudio-is/webstudio/actions/runs/42115696559) passed after GitHub cancelled its first attempt. The intentional indicator difference and the two previously reviewed Navigator text differences were approved. No production release was started. ### MCP Form status follow-up - [x] Review the old first-attempt failures and compare the new first-pass Form CI results; the old reports did not expose which layer failed. - [x] Stop masking pre-submission browser errors as uncertain Form outcomes; confirm the regression test fails before and passes after. - [x] Review `docs/` and fixture impact; run focused tests and package checks. - [x] Push the fix to this PR and pass required CI. The CLI had returned `unknown` for every browser error, including startup failures before any Form submission. It now lets those errors surface. The browser observer still returns `unknown` if a submission may have happened, and neither path retries it. This removes 8 net production lines. The regression test failed before the fix and passes afterward. All 50 CLI preview tests and 59 Vision browser tests passed, as did CLI/Vision typechecks, lint, formatting, and the diff check. No docs or fixtures changed. The approved full agent evaluation ran: 8/9 scenarios passed; the existing managed-Form authoring `contactPage` check failed, while the Form runtime scenario passed. The comparison gate is also non-green because saved baselines are missing or incompatible. Commit: `a2ff0822ad`. The intermittent CI cause still needs a first-attempt check. [Main CI on the new head](https://github.com/webstudio-is/webstudio/actions/runs/42123770480) passed all jobs, including all ten Builder browser shards on their first attempt. The Form-heavy shards 1, 5, 6, 9, and 10 passed. The [approved visual run](https://github.com/webstudio-is/webstudio/actions/runs/42124446769) passed; its initial run showed only the four previously reviewed UI differences, none from this CLI-only patch. The older flaky `unknown` results cannot be attributed to startup versus post-dispatch observation from their logs, so this fix addresses a proven masking bug and adds regression coverage, but does not prove every possible intermittent observer failure is gone. No production release was started.