close
Skip to content

Fix some unsafe type assertions#2719

Merged
fredrikekelund merged 16 commits into
trunkfrom
f26d/fix-some-unsafe-type-assertions
Mar 11, 2026
Merged

Fix some unsafe type assertions#2719
fredrikekelund merged 16 commits into
trunkfrom
f26d/fix-some-unsafe-type-assertions

Conversation

@fredrikekelund
Copy link
Copy Markdown
Contributor

Related issues

How AI was used in this PR

#2694 (comment) prompted me to look into ESLint rules to avoid similar problems. I found @typescript-eslint/no-unsafe-type-assertion and tried enabling it, letting Codex fix the errors. It did a decent job, but ultimately, there were too many errors for a single PR. The stuff that made it into this PR was mostly written by hand, except for format-rtk-error.tsx.

Proposed Changes

See the previous section for some background… In short, I tried enabling @typescript-eslint/no-unsafe-type-assertion but found too many errors. This PR primarily fixes up some type assertions related to blueprints, but also in format-rtk-error.tsx, snapshot-slice.ts, and tools/common/lib/locale.ts.

Testing Instructions

  1. Code review is paramount
  2. Lightly smoke test creating a new site from a blueprint

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

Copy link
Copy Markdown
Contributor

@katinthehatsite katinthehatsite left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, I explored similar solutions but I think this one is slightly better. This approach uses structuredClone instead of spread and my approach used shallow spread + index-based assignment.

Do we also need to update the Blueprint type in studio/apps/studio/src/modules/add-site/lib/apply-blueprint-form-values.ts ?

@fredrikekelund
Copy link
Copy Markdown
Contributor Author

Do we also need to update the Blueprint type in studio/apps/studio/src/modules/add-site/lib/apply-blueprint-form-values.ts

We should. Good catch 👍 Came up when I merged trunk

@fredrikekelund
Copy link
Copy Markdown
Contributor Author

Looks like I still need to update this PR to fix the failing tests…

@wpmobilebot
Copy link
Copy Markdown
Collaborator

wpmobilebot commented Mar 9, 2026

📊 Performance Test Results

Comparing 373aaa6 vs trunk

site-editor

Metric trunk 373aaa6 Diff Change
load 1739.00 ms 1728.00 ms -11.00 ms ⚪ 0.0%

site-startup

Metric trunk 373aaa6 Diff Change
siteCreation 6088.00 ms 6080.00 ms -8.00 ms ⚪ 0.0%
siteStartup 3934.00 ms 3940.00 ms +6.00 ms ⚪ 0.0%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)

@fredrikekelund
Copy link
Copy Markdown
Contributor Author

I've fixed the tests. The problem was caused by importing the isStepDefinition function from @wp-playground/blueprints. That's a tiny function, but because of how the @wp-playground/blueprints exports are structured, Vite also pulled in the @php-wasm/node-polyfills package into the bundle. apps/studio/electron.vite.config.ts defines all @php-wasm/* packages as external, but that package still wasn't preserved in the packaged node_modules directory, since @wp-playground/blueprints is a devDependency. All this resulted in a fatal error when the Electron app started.

In short… A bit of a mess for such a small issue. I decided to just copy and inline the isStepDefinition function to get around the problem. I've also raised the fundamental issue with Orbit (p1773050695429429-slack-C09GMMV3T6J).

I'd appreciate another review, @katinthehatsite 🙏

@fredrikekelund
Copy link
Copy Markdown
Contributor Author

Please ignore the lint failures. They will be fixed when #2720 is merged into this branch.

Copy link
Copy Markdown
Contributor

@epeicher epeicher left a comment

Choose a reason for hiding this comment

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

Thanks for improving this @fredrikekelund! I have reviewed it, and I have just left a question for consideration. I have tested site creation using blueprints, and I have not found any issues. LGTM!

Comment thread apps/cli/lib/cli-args-sanitizer.ts Outdated
* Prepares data for Sentry by stringifying nested objects to avoid normalization limits.
*/
export function sanitizeRunCLIArgs( args: RunCLIArgs ): Record< string, unknown > {
const blueprint = args.blueprint && 'steps' in args.blueprint ? args.blueprint : undefined;
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.

This would return undefined for a blueprint that has no steps, but there are blueprints that can contain no steps, is this intentional?

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.

That's an excellent point! I was trying to appease TS here, but from a functionality perspective, you're right that this is lossy. I've updated the code and opted to use a type assertion after all.

@fredrikekelund
Copy link
Copy Markdown
Contributor Author

Thanks for the review, @epeicher 🙏

* Remove `AllowedPHPVersion` type

* Remove unused `setProviderConstants` action

* Fix type error

* Fix test
@fredrikekelund fredrikekelund merged commit 602d59a into trunk Mar 11, 2026
9 of 10 checks passed
@fredrikekelund fredrikekelund deleted the f26d/fix-some-unsafe-type-assertions branch March 11, 2026 12:21
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.

4 participants