Introduce EffectJS and Zod - #26
Conversation
Add Zod and Effect.js libraries to sanitize server-provided Block Context data for Bookmark, Reaction, Konomi, and UserProfile blocks. This ensures type-safe runtime validation of data before assigning it to the Interactivity Store.
This update prevents ESLint from checking the build-modules directory, streamlining the linting process.
Introduce Effect.js for effect handling and Zod for schema validation. Update tests to ensure proper sanitization and add new test cases.
📝 WalkthroughWalkthroughAdds schema sanitization utilities, Zod/effect module wiring, tests, build module manifests, PHP module registration, and updates multiple block stores to validate/merge sanitized context and state during initialization. Changes
Sequence Diagram(s)sequenceDiagram
participant Store as Block Store
participant Inter as `@wordpress/interactivity`
participant Effect as Effect runtime
participant Zod as zod
participant AppCtx as In-memory Context
Store->>Inter: getServerContext(storeName)
Inter-->>Store: rawContext
Store->>Effect: sanitizeContext(schema, storeName)
Effect->>Zod: parse/validate rawContext
Zod-->>Effect: parsed or ZodError
alt parsed
Effect->>AppCtx: merge sanitized context (Object.assign)
Effect-->>Store: succeed(null)
else error
Effect->>Store: fail(Cause/Error)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sources/Blocks/blocks-manifest.php (1)
1-3: This file is auto-generated and violates naming/structure guidelines—requires a build or architecture decision, not manual edits.This PHP file is generated by
wp-scripts build-blocks-manifestand should not be manually modified. However, it violates the coding guidelines by missingdeclare(strict_types=1);and theSpaghettiDojo\Konominamespace required for all PHP files insources/.Since the file is auto-generated and will be overwritten on rebuild, manual edits are not viable. Choose one:
- Document an exception for auto-generated manifests in
sources/(least preferred—creates inconsistency)- Relocate the manifest outside
sources/with a documented reason- Configure the generator (if
wp-scripts build-blocks-manifestsupports options) or create a wrapper script that regenerates with proper headersPer coding guidelines:
**/*.phprequires strict types andsources/**/*.phprequires namespaceSpaghettiDojo\Konomi.
🤖 Fix all issues with AI agents
In `@build-modules/konomi-schema.asset.php`:
- Line 1: Add the PHP strict types declaration to the top of the file by
inserting declare(strict_types=1); as the first non-blank, non-comment line
before the existing "<?php return array(...)" so the file begins with "<?php
declare(strict_types=1);" and then returns the array as currently defined (the
returned array with 'dependencies', 'version', 'type' remains unchanged).
In `@sources/Blocks/Bookmark/view/store.ts`:
- Around line 77-82: callbacks.init currently calls
Effect.runPromise(sanitizeContext(contextSchema, STORE_NAME)) without handling
rejections; update this to handle the returned Promise from Effect.runPromise by
attaching a .catch(...) to log or surface errors (or alternatively pass an
onError callback into sanitizeContext/Effect.runPromise if supported),
referencing callbacks.init, Effect.runPromise, sanitizeContext, contextSchema
and STORE_NAME so that any failure from sanitizeContext is caught and does not
produce an unhandled rejection.
In `@sources/Blocks/Konomi/view/schema.ts`:
- Around line 3-12: Keep the error field required on contextSchema (do not make
error optional) and create a single source of truth by exporting the inferred
Context type from this file: export type Context = z.infer<typeof
contextSchema>; then remove or replace the separately-declared Context type in
store.ts so store code imports the Context type from this schema file (follow
the UserProfile pattern of exporting inferred types from state/schema files and
ensure all usages still access error.code and error.message without null
checks).
In `@sources/Blocks/Konomi/view/store.ts`:
- Around line 58-61: The init method calls
Effect.runPromise(sanitizeContext(contextSchema, STORE_NAME)) but does not
handle its returned Promise; update init (in the init() function) to await or
attach a .catch handler to the Effect.runPromise call so rejections from
sanitizeContext(contextSchema, STORE_NAME) are handled—e.g., await the call
inside an async init or add .catch to log or handle the error via the store's
error path (referencing init, sanitizeContext, contextSchema, STORE_NAME, and
Effect.runPromise).
In `@sources/Blocks/UserProfile/view/store.ts`:
- Around line 43-48: The init function calls
Effect.runSync(sanitizeState(stateSchema, STORE_NAME)) which can throw if schema
validation fails because sanitizeState is invoked without an onError handler;
modify init to handle that failure by either passing an onError callback (e.g.,
use catchInitializationIssues as the onError argument to sanitizeState) so
sanitizeState returns a safe fallback instead of throwing, or wrap the
Effect.runSync call in a try/catch that logs the error (via your logger) and
returns/initializes a default safe state for STORE_NAME; reference init,
STORE_NAME, sanitizeState, stateSchema and Effect.runSync when applying the
change.
In `@tests/setup.ts`:
- Around line 6-11: The file uses `@ts-ignore` to silence TypeScript for
globalThis.wp in ensureGlobalWp (and the similar block at lines 15-19); replace
those ignores by declaring a proper global type and casting: add a minimal
declaration for the global interface (or a local typed helper) so you can safely
write (globalThis as unknown as { wp?: Record<string, any> }).wp or use a helper
function to set globalThis.wp = {} with an explicit type, and remove the
`@ts-ignore` comments; update both ensureGlobalWp and the other global assignment
to use the same typed approach so strict mode is satisfied.
In `@tests/unit/js/modules/schema/routines.spec.ts`:
- Around line 17-23: Add a beforeEach hook to clear jest mocks between tests by
inserting beforeEach(() => { jest.clearAllMocks() }) near the top of the
routines spec (after the jest.mock(...) block) so mocks like
getServerState/getServerContext/getContext/getConfig are reset between test
cases; this ensures test isolation for the sanitizer suites and uses the global
jest.clearAllMocks API.
- Around line 28-41: Replace the unsafe `as any` test fixtures with correctly
typed values: for valid fixtures returned from getServerState (used in the test
invoking sanitizeState with schema and storeName) use TypeScript's `satisfies
z.infer<typeof schema>` to assert they match the schema type, and for invalid
fixtures use the `unknown` type (or a typed `unknown` variable) to represent
values that should fail validation; update the mocked getServerState calls
(jest.mocked(getServerState).mockReturnValue(...)) to return these properly
typed fixtures so the tests compile under strict mode while still exercising
sanitizeState and the onError behavior.
In `@tests/unit/js/modules/schema/schema.spec.ts`:
- Around line 6-12: Add a test-level beforeEach that clears Jest mocks so mock
state from the jest.mock of '@wordpress/interactivity' (getConfig, getContext,
getServerContext, getServerState) is reset between tests; place this beforeEach
in the same spec file (tests/unit/js/modules/schema/schema.spec.ts) after the
jest.mock block and have it call Jest's clearAllMocks to ensure each test starts
with clean mock state.
🧹 Nitpick comments (10)
sources/Blocks/UserProfile/view/schema.ts (1)
10-10: Consider wrapping the inferred type withReadonly<>for immutability.As per coding guidelines,
Readonly<>should be used for immutable data structures in this directory.♻️ Suggested change
-export type UserProfileState = z.infer< typeof stateSchema >; +export type UserProfileState = Readonly< z.infer< typeof stateSchema > >;sources/Blocks/Reaction/view/schema.ts (1)
3-6: Consider exporting inferred types for consistency.The
UserProfile/view/schema.tsexports type aliases (UserProfileState,UserProfileStateSchema), while this file does not. Consider adding similar exports if consumers need the inferred type.♻️ Optional type exports
export const contextSchema = z.object( { isActive: z.boolean(), count: z.number(), } ); + +export type ReactionContextSchema = typeof contextSchema; +export type ReactionContext = Readonly< z.infer< typeof contextSchema > >;resources/modules/zod/index.ts (1)
1-2: Redundant re-export ofz.
export * from 'zod'already includes the named exportz, making line 2 redundant. This doesn't cause issues but can be simplified.♻️ Suggested simplification
export * from 'zod'; -export { z } from 'zod';sources/Artifacts/Module.php (1)
60-62: Silent skip on unreadable asset files may hinder debugging.When an asset file is unreadable, the code silently continues. Consider logging a warning to aid troubleshooting deployment issues.
♻️ Optional: Add debug logging
if (!is_readable($assetPath)) { + // Consider adding: error_log("Module asset not readable: {$assetPath}"); continue; }__mocks__/@external/effect-js.ts (2)
122-136: Type and value both namedCausemay cause confusion.While TypeScript allows the same name for types and values (separate namespaces), having both
export type Cause = unknownandexport const Cause = {}with the same name can be confusing for consumers.♻️ Optional: Clarify naming
-// Convenience re-exports to align with Effect style API -export type Cause = unknown; +// Convenience re-exports to align with Effect style API +export type CauseType = unknown; +export type Cause = CauseType; export const Effect = { succeed, fail, try: try_, runPromise, runSync, flatMap, tap, andThen, catchAllCause, }; -export const Cause = {}; +export const Cause: Record<string, never> = {};Alternatively, this naming mirrors the real Effect library's API where
Causeexists as both a type and a namespace, so keeping it as-is is also acceptable for API compatibility.
118-119: Consider stricter typing forpipe.The
anytype in the function array reduces type safety. For a test mock this is acceptable, but stricter typing would catch more issues.♻️ Optional: Stricter typing
-export const pipe = <T>(value: T, ...fns: Array<(input: any) => any>) => +export const pipe = <T, R = T>(value: T, ...fns: Array<(input: unknown) => unknown>): R => - fns.reduce((current, fn) => fn(current), value); + fns.reduce((current, fn) => fn(current), value as unknown) as R;This is a minor nit for a mock file where perfect typing isn't critical.
resources/modules/schema/index.ts (1)
54-55: Replaceanywithunknownto preserve strict typing.
anybypasses type safety;unknownkeeps the pipeline strict while still allowing schema validation.♻️ Suggested change
- Effect.flatMap( ( context: any ) => sanitize( context, schema ) ), + Effect.flatMap( ( context: unknown ) => sanitize( context, schema ) ), @@ - Effect.flatMap( ( config: any ) => sanitize( config, schema ) ), + Effect.flatMap( ( config: unknown ) => sanitize( config, schema ) ),As per coding guidelines:
**/*.{ts,tsx}: Use TypeScript with strict typingAlso applies to: 74-75
tests/unit/js/modules/schema/routines.spec.ts (1)
46-48: Track the TODOs for pub/sub assertions to avoid lingering gaps.Consider opening a follow-up issue so these assertions don’t get lost as tests evolve.
Also applies to: 81-82, 122-123
tests/unit/js/blocks/bookmarks/view/store.spec.ts (2)
41-45: Tighten callback typing to keep strict TS.
any[]weakens type-safety; considerunknown[](or a tighter signature) for callbacks/actions.♻️ Suggested typing tweak
- actions: Record<string, ( ...args: any[] ) => void>; - callbacks: Record<string, ( ...args: any[] ) => void>; + actions: Record<string, ( ...args: ReadonlyArray<unknown> ) => void>; + callbacks: Record<string, ( ...args: ReadonlyArray<unknown> ) => void>;As per coding guidelines, Use TypeScript with strict typing.
84-86: Avoidas anyin the sanitizeContext mock.
This should usually type-check without a cast; if it doesn’t, prefer a typed helper overany.♻️ Suggested cleanup
- jest.mocked( sanitizeContext ).mockReturnValue( - Effect.succeed( null ) as any, - ); + jest.mocked( sanitizeContext ).mockReturnValue( + Effect.succeed( null ), + );As per coding guidelines, Use TypeScript with strict typing.
| callbacks: { | ||
| init(): void { | ||
| Effect.runPromise( | ||
| sanitizeContext( contextSchema, STORE_NAME ) | ||
| ); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "store.ts" -path "*/Blocks/Bookmark/view/*" -type fRepository: Spaghetti-Dojo/konomi
Length of output: 104
🏁 Script executed:
# Read the specific file to see lines 77-82 and context
wc -l sources/Blocks/Bookmark/view/store.tsRepository: Spaghetti-Dojo/konomi
Length of output: 105
🏁 Script executed:
# Get the actual code at lines 77-82 with context
sed -n '70,90p' sources/Blocks/Bookmark/view/store.tsRepository: Spaghetti-Dojo/konomi
Length of output: 340
🏁 Script executed:
# Find sanitizeContext implementation
rg -n "export.*sanitizeContext|function sanitizeContext|const sanitizeContext" --type ts --type tsxRepository: Spaghetti-Dojo/konomi
Length of output: 92
🏁 Script executed:
# Check how Effect.runPromise is defined and whether it handles errors
rg -n "runPromise|Effect" sources/Blocks/Bookmark/view/store.ts -B 2 -A 2Repository: Spaghetti-Dojo/konomi
Length of output: 331
🏁 Script executed:
# Find sanitizeContext implementation
rg -n "export.*sanitizeContext" sources/ -A 10 --type tsRepository: Spaghetti-Dojo/konomi
Length of output: 47
🏁 Script executed:
# Also check in konomi schema module
find . -path "*schema*" -name "*.ts" -o -name "*.tsx" | head -20Repository: Spaghetti-Dojo/konomi
Length of output: 1003
🏁 Script executed:
# Check Effect.runPromise usage patterns in codebase
rg -n "Effect\.runPromise|runPromise\(" sources/ --type ts -B 2 -A 3Repository: Spaghetti-Dojo/konomi
Length of output: 1153
🏁 Script executed:
# Read the store.ts file completely to check for Konomi namespace usage
cat -n sources/Blocks/Bookmark/view/store.tsRepository: Spaghetti-Dojo/konomi
Length of output: 2599
🏁 Script executed:
# Read sanitizeContext implementation
cat -n resources/modules/schema/index.ts | head -100Repository: Spaghetti-Dojo/konomi
Length of output: 3699
🏁 Script executed:
# Check for sanitizeContext specifically
rg -n "sanitizeContext" resources/modules/schema/index.ts -A 15Repository: Spaghetti-Dojo/konomi
Length of output: 770
🏁 Script executed:
# Check Effect.runPromise in node_modules or external definition
find . -name "*.d.ts" -o -name "*.ts" | xargs grep -l "runPromise" 2>/dev/null | head -5Repository: Spaghetti-Dojo/konomi
Length of output: 265
🏁 Script executed:
# Look for Effect type definitions
rg -n "runPromise|Effect" sources/ --type ts -B 1 -A 1 | grep -A 3 -B 3 "runPromise"Repository: Spaghetti-Dojo/konomi
Length of output: 1088
🏁 Script executed:
# Check Konomi namespace usage in the codebase
rg -n "namespace Konomi|declare namespace Konomi" sources/ --type ts --type tsxRepository: Spaghetti-Dojo/konomi
Length of output: 92
🏁 Script executed:
# Check how Effect.runPromise behaves with failing Effects
cat -n ./__mocks__/@external/effect-js.tsRepository: Spaghetti-Dojo/konomi
Length of output: 4588
🏁 Script executed:
# Verify if there are other usages of sanitizeContext with onError callback
rg -n "sanitizeContext.*onError|onError.*:.*=>" sources/ --type tsRepository: Spaghetti-Dojo/konomi
Length of output: 47
🏁 Script executed:
# Check if the review comment's concern about Konomi namespace is also valid
rg -n "namespace Konomi|type Context.*Konomi" sources/Blocks/Bookmark/view/ --type tsRepository: Spaghetti-Dojo/konomi
Length of output: 47
Handle the Promise from Effect.runPromise to avoid unhandled rejections.
callbacks.init is sync, and sanitizeContext can reject via Effect.catchAllCause → Effect.fail. Without error handling, rejection goes unhandled. Add .catch() handler or pass onError callback to sanitizeContext.
🔧 Suggested patterns
Option 1 - Using .catch():
init(): void {
- Effect.runPromise(
- sanitizeContext( contextSchema, STORE_NAME )
- );
+ void Effect.runPromise(
+ sanitizeContext( contextSchema, STORE_NAME )
+ ).catch( () => {
+ // optionally log or report initialization failure
+ } );
},Option 2 - Using onError callback:
init(): void {
- Effect.runPromise(
- sanitizeContext( contextSchema, STORE_NAME )
- );
+ void Effect.runPromise(
+ sanitizeContext( contextSchema, STORE_NAME, ( error ) => {
+ // handle initialization error
+ } )
+ );
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| callbacks: { | |
| init(): void { | |
| Effect.runPromise( | |
| sanitizeContext( contextSchema, STORE_NAME ) | |
| ); | |
| }, | |
| callbacks: { | |
| init(): void { | |
| void Effect.runPromise( | |
| sanitizeContext( contextSchema, STORE_NAME ) | |
| ).catch( () => { | |
| // optionally log or report initialization failure | |
| } ); | |
| }, |
🤖 Prompt for AI Agents
In `@sources/Blocks/Bookmark/view/store.ts` around lines 77 - 82, callbacks.init
currently calls Effect.runPromise(sanitizeContext(contextSchema, STORE_NAME))
without handling rejections; update this to handle the returned Promise from
Effect.runPromise by attaching a .catch(...) to log or surface errors (or
alternatively pass an onError callback into sanitizeContext/Effect.runPromise if
supported), referencing callbacks.init, Effect.runPromise, sanitizeContext,
contextSchema and STORE_NAME so that any failure from sanitizeContext is caught
and does not produce an unhandled rejection.
There was a problem hiding this comment.
@copilot Would you be so kind to work on this? Consider
- Keep the
pipeflow - We don't need to differentiate between expected and unexpected errors, just prevent to log in the console
- For now we can simply use
@wordpress/hooksas a publish system, so that later on we can hook a subscriber - The error should be a standard object, I would start fresh with a easy
ErrorMessagewith two properties,message,type,severity. Themessageis human readable text and shall not come from the underline procedure, it shall be meaningfull since it will be printed to the user, so no technical terms no data information related to logic or code errors or stack.typeis to distinguish betweenexpectedandunexpected, if it's easier to catch those two errors separately, than I would avoid thetypeproperty.severity, this is more likeinfo,error,warning. I think make sense to align with PHP PSR Logging specification so that, if necessary we can send thisErrorMessageto the server for further processing and there since we run PHP we expect to have something PSR compatible. - Double check if there are other occurrencies of the same procedure, other
storefiles using the same strategy to sanitize the content and apply the same changes.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| export const contextSchema = z.object( { | ||
| id: z.number(), | ||
| type: z.string(), | ||
| isUserLoggedIn: z.boolean(), | ||
| loginRequired: z.boolean(), | ||
| error: z.object( { | ||
| code: z.string(), | ||
| message: z.string(), | ||
| } ), | ||
| } ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find usage of contextSchema in the Konomi block
rg -n "contextSchema" sources/Blocks/Konomi/ -A 3 -B 3
# Search for context initialization/creation patterns
rg -n "getServerContext|getContext|createContext" sources/Blocks/Konomi/ -A 5Repository: Spaghetti-Dojo/konomi
Length of output: 2936
🏁 Script executed:
# Check how the error field is used in the block
rg -n "error" sources/Blocks/Konomi/ -B 2 -A 2
# Look for similar schema patterns in other blocks
find sources/Blocks -name "schema.ts" -o -name "schema.js" | head -10Repository: Spaghetti-Dojo/konomi
Length of output: 3480
🏁 Script executed:
# Check if other schema files export inferred types
rg -n "export type.*=.*z.infer" sources/Blocks/ --type ts --type tsxRepository: Spaghetti-Dojo/konomi
Length of output: 92
🏁 Script executed:
# Check the Context type definition in store.ts more closely
sed -n '30,45p' sources/Blocks/Konomi/view/store.ts
# Check if Context is exported from schema.ts or defined elsewhere
rg -n "type Context" sources/Blocks/Konomi/ -B 2 -A 5
# Check if other blocks' schema files export inferred types
for f in sources/Blocks/*/view/schema.ts; do echo "=== $f ==="; head -20 "$f"; doneRepository: Spaghetti-Dojo/konomi
Length of output: 1710
Remove suggestion to make error optional; consider establishing a single source of truth for the Context type.
The error field must remain required. Server initialization (Context.php) always provides { code: '', message: '' }, and all client code accesses error.code and error.message without null checks.
However, the Context type is currently maintained separately in store.ts while contextSchema is defined in schema.ts—creating a potential source of drift. The UserProfile block shows a better pattern: exporting inferred types directly from the schema file (e.g., export type UserProfileState = z.infer< typeof stateSchema >;). Consider following this approach to keep schema and types in sync.
🤖 Prompt for AI Agents
In `@sources/Blocks/Konomi/view/schema.ts` around lines 3 - 12, Keep the error
field required on contextSchema (do not make error optional) and create a single
source of truth by exporting the inferred Context type from this file: export
type Context = z.infer<typeof contextSchema>; then remove or replace the
separately-declared Context type in store.ts so store code imports the Context
type from this schema file (follow the UserProfile pattern of exporting inferred
types from state/schema files and ensure all usages still access error.code and
error.message without null checks).
| init(): void { | ||
| Effect.runPromise( | ||
| sanitizeContext( contextSchema, STORE_NAME ) | ||
| ); |
There was a problem hiding this comment.
Handle the promise from Effect.runPromise to avoid unhandled rejections.
If sanitization fails, the promise rejects; currently it’s not caught.
🛠️ Suggested fix
- Effect.runPromise(
- sanitizeContext( contextSchema, STORE_NAME )
- );
+ void Effect.runPromise(
+ sanitizeContext( contextSchema, STORE_NAME )
+ ).catch( () => undefined );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| init(): void { | |
| Effect.runPromise( | |
| sanitizeContext( contextSchema, STORE_NAME ) | |
| ); | |
| init(): void { | |
| void Effect.runPromise( | |
| sanitizeContext( contextSchema, STORE_NAME ) | |
| ).catch( () => undefined ); | |
| } |
🤖 Prompt for AI Agents
In `@sources/Blocks/Konomi/view/store.ts` around lines 58 - 61, The init method
calls Effect.runPromise(sanitizeContext(contextSchema, STORE_NAME)) but does not
handle its returned Promise; update init (in the init() function) to await or
attach a .catch handler to the Effect.runPromise call so rejections from
sanitizeContext(contextSchema, STORE_NAME) are handled—e.g., await the call
inside an async init or add .catch to log or handle the error via the store's
error path (referencing init, sanitizeContext, contextSchema, STORE_NAME, and
Effect.runPromise).
| const STORE_NAME = 'konomiProfilePagination'; | ||
|
|
||
| // eslint-disable-next-line max-lines-per-function | ||
| export function init(): void { | ||
| const { state } = store< Store >( 'konomiProfilePagination', { | ||
| state: getServerState(), | ||
| const { state } = store< Store >( STORE_NAME, { | ||
| state: Effect.runSync( sanitizeState( stateSchema, STORE_NAME ) ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "catchInitializationIssues" resources/modules/schema/index.ts -A 10 -B 2Repository: Spaghetti-Dojo/konomi
Length of output: 1493
🏁 Script executed:
rg -n "sanitizeState" sources/Blocks/UserProfile/view/store.ts -A 5 -B 5Repository: Spaghetti-Dojo/konomi
Length of output: 843
🏁 Script executed:
rg -n "export.*sanitizeState" --type ts --type tsx -A 10Repository: Spaghetti-Dojo/konomi
Length of output: 92
🏁 Script executed:
cat -n sources/Blocks/UserProfile/view/store.tsRepository: Spaghetti-Dojo/konomi
Length of output: 4858
Add error handling to prevent uncaught exceptions from Effect.runSync in store initialization.
catchInitializationIssues does not prevent the Effect from failing—it invokes the optional callback, then returns Effect.fail( error ). Since sanitizeState is called without an onError handler on line 48, any schema validation failure will cause Effect.runSync to throw an uncaught exception, breaking store initialization. Either pass an onError callback to sanitizeState, wrap the runSync call in try-catch, or use an alternative Effect method that provides a safe fallback.
🤖 Prompt for AI Agents
In `@sources/Blocks/UserProfile/view/store.ts` around lines 43 - 48, The init
function calls Effect.runSync(sanitizeState(stateSchema, STORE_NAME)) which can
throw if schema validation fails because sanitizeState is invoked without an
onError handler; modify init to handle that failure by either passing an onError
callback (e.g., use catchInitializationIssues as the onError argument to
sanitizeState) so sanitizeState returns a safe fallback instead of throwing, or
wrap the Effect.runSync call in a try/catch that logs the error (via your
logger) and returns/initializes a default safe state for STORE_NAME; reference
init, STORE_NAME, sanitizeState, stateSchema and Effect.runSync when applying
the change.
| function ensureGlobalWp() { | ||
| // @ts-ignore | ||
| if ( typeof globalThis.wp === 'undefined' ) { | ||
| // @ts-ignore | ||
| globalThis.wp = {}; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n tests/setup.tsRepository: Spaghetti-Dojo/konomi
Length of output: 578
🏁 Script executed:
fd -e "d.ts" | head -20Repository: Spaghetti-Dojo/konomi
Length of output: 119
🏁 Script executed:
rg -n "declare global|interface.*Wp|globalThis\.wp" --type ts --type tsxRepository: Spaghetti-Dojo/konomi
Length of output: 92
🏁 Script executed:
rg -n "wp\s*:" -g "*.d.ts"Repository: Spaghetti-Dojo/konomi
Length of output: 93
🏁 Script executed:
cat -n `@types/index.d.ts`Repository: Spaghetti-Dojo/konomi
Length of output: 618
🏁 Script executed:
cat -n `@types/wp/index.d.ts` | head -50Repository: Spaghetti-Dojo/konomi
Length of output: 147
🏁 Script executed:
rg "globalThis\.wp" --type tsRepository: Spaghetti-Dojo/konomi
Length of output: 217
🏁 Script executed:
cat tsconfig.json | head -40Repository: Spaghetti-Dojo/konomi
Length of output: 1352
Replace @ts-ignore with explicit global typing to enforce strict TypeScript checks.
The @ts-ignore comments suppress type errors despite strict: true being enabled in tsconfig.json. Since globalThis.wp is used only in this file and does not have a type definition for the Node.js test environment, use a typed helper cast instead.
🔧 Suggested refactor (typed helper)
+type GlobalWithWp = typeof globalThis & {
+ wp?: {
+ apiFetch?: ( ( ...args: unknown[] ) => unknown ) & { use?: ( ...args: unknown[] ) => void };
+ };
+};
+
function ensureGlobalWp() {
- // `@ts-ignore`
- if ( typeof globalThis.wp === 'undefined' ) {
- // `@ts-ignore`
- globalThis.wp = {};
- }
+ const globalWithWp = globalThis as GlobalWithWp;
+ if ( typeof globalWithWp.wp === 'undefined' ) {
+ globalWithWp.wp = {};
+ }
}
// TODO Use __mocks__
function mockWpApiFetch() {
const apiFetch = () => void 0;
apiFetch.use = () => void 0;
- // `@ts-ignore`
- globalThis.wp['apiFetch'] = apiFetch;
+ const globalWithWp = globalThis as GlobalWithWp;
+ globalWithWp.wp = globalWithWp.wp ?? {};
+ globalWithWp.wp.apiFetch = apiFetch;
}Also applies to: lines 15-19
🤖 Prompt for AI Agents
In `@tests/setup.ts` around lines 6 - 11, The file uses `@ts-ignore` to silence
TypeScript for globalThis.wp in ensureGlobalWp (and the similar block at lines
15-19); replace those ignores by declaring a proper global type and casting: add
a minimal declaration for the global interface (or a local typed helper) so you
can safely write (globalThis as unknown as { wp?: Record<string, any> }).wp or
use a helper function to set globalThis.wp = {} with an explicit type, and
remove the `@ts-ignore` comments; update both ensureGlobalWp and the other global
assignment to use the same typed approach so strict mode is satisfied.
| jest.mock('@wordpress/interactivity', () => ({ | ||
| getServerState: jest.fn(), | ||
| getServerContext: jest.fn(), | ||
| getContext: jest.fn(), | ||
| getConfig: jest.fn(), | ||
| })); | ||
|
|
There was a problem hiding this comment.
Add beforeEach to clear mocks between tests.
This keeps test isolation consistent across all sanitizer suites.
✅ Suggested addition
jest.mock('@wordpress/interactivity', () => ({
getServerState: jest.fn(),
getServerContext: jest.fn(),
getContext: jest.fn(),
getConfig: jest.fn(),
}));
+
+beforeEach(() => {
+ jest.clearAllMocks();
+});As per coding guidelines, Clear mocks between tests with beforeEach(() => { jest.clearAllMocks() }) in TypeScript tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.mock('@wordpress/interactivity', () => ({ | |
| getServerState: jest.fn(), | |
| getServerContext: jest.fn(), | |
| getContext: jest.fn(), | |
| getConfig: jest.fn(), | |
| })); | |
| jest.mock('@wordpress/interactivity', () => ({ | |
| getServerState: jest.fn(), | |
| getServerContext: jest.fn(), | |
| getContext: jest.fn(), | |
| getConfig: jest.fn(), | |
| })); | |
| beforeEach(() => { | |
| jest.clearAllMocks(); | |
| }); |
🤖 Prompt for AI Agents
In `@tests/unit/js/modules/schema/routines.spec.ts` around lines 17 - 23, Add a
beforeEach hook to clear jest mocks between tests by inserting beforeEach(() =>
{ jest.clearAllMocks() }) near the top of the routines spec (after the
jest.mock(...) block) so mocks like
getServerState/getServerContext/getContext/getConfig are reset between test
cases; this ensures test isolation for the sanitizer suites and uses the global
jest.clearAllMocks API.
| it('returns sanitized state', async () => { | ||
| const serverState = { page: 1, perPage: 10 }; | ||
| jest.mocked(getServerState).mockReturnValue(serverState as any); | ||
|
|
||
| const result = await Effect.runPromise(sanitizeState(schema, storeName)); | ||
|
|
||
| expect(result).toEqual(serverState); | ||
| expect(getServerState).toHaveBeenCalledWith(storeName); | ||
| }); | ||
|
|
||
| it('fails on invalid state and calls onError', async () => { | ||
| const onError = jest.fn(); | ||
| jest.mocked(getServerState).mockReturnValue({ page: 'invalid' } as any); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the test file
fd -t f "routines.spec.ts" | head -5Repository: Spaghetti-Dojo/konomi
Length of output: 110
🏁 Script executed:
# Check the file size first
wc -l tests/unit/js/modules/schema/routines.spec.tsRepository: Spaghetti-Dojo/konomi
Length of output: 114
🏁 Script executed:
# Read the specific lines mentioned in the review (28-41)
sed -n '1,150p' tests/unit/js/modules/schema/routines.spec.ts | cat -nRepository: Spaghetti-Dojo/konomi
Length of output: 4471
🏁 Script executed:
# Look for the getServerState type definition
fd -t f -e "d.ts" -e "ts" | xargs rg -l "getServerState" | head -10Repository: Spaghetti-Dojo/konomi
Length of output: 241
🏁 Script executed:
# Check import statements and type definitions in the test file
head -30 tests/unit/js/modules/schema/routines.spec.tsRepository: Spaghetti-Dojo/konomi
Length of output: 858
🏁 Script executed:
# Check if there's a types directory or node_modules reference for `@wordpress/interactivity`
fd -t f "@wordpress" | head -20Repository: Spaghetti-Dojo/konomi
Length of output: 47
🏁 Script executed:
# Look for any TypeScript configuration or type declaration files
fd -t f -e "d.ts" | xargs rg -l "@wordpress/interactivity" 2>/dev/null | head -10Repository: Spaghetti-Dojo/konomi
Length of output: 47
🏁 Script executed:
# Check the project's package.json to understand dependencies
cat package.json | grep -A 20 -B 5 "@wordpress/interactivity"Repository: Spaghetti-Dojo/konomi
Length of output: 1416
🏁 Script executed:
# Check the tsconfig to understand strict typing requirements
cat tsconfig.json | head -50Repository: Spaghetti-Dojo/konomi
Length of output: 1494
🏁 Script executed:
# Also verify the actual behavior - check if there are any type errors currently
# Look for any error suppression comments near the as any usage
grep -n "as any" tests/unit/js/modules/schema/routines.spec.tsRepository: Spaghetti-Dojo/konomi
Length of output: 574
🏁 Script executed:
# Check if there's a `@types` directory or type definitions
fd -t d "@types" | head -10Repository: Spaghetti-Dojo/konomi
Length of output: 72
Use strict typing in test fixtures—remove as any casts.
The project enforces strict: true TypeScript compilation. Using as any bypasses type safety and can mask test setup errors. Replace with:
- Valid fixtures:
satisfieswith the schema type (e.g.,satisfies z.infer<typeof schema>) - Invalid fixtures:
unknowntype for values that should fail validation
🔧 Refactoring pattern
-const serverState = { page: 1, perPage: 10 };
-jest.mocked(getServerState).mockReturnValue(serverState as any);
+const serverState = { page: 1, perPage: 10 } satisfies z.infer<typeof schema>;
+jest.mocked(getServerState).mockReturnValue(serverState);
-jest.mocked(getServerState).mockReturnValue({ page: 'invalid' } as any);
+const invalidState: unknown = { page: 'invalid' };
+jest.mocked(getServerState).mockReturnValue(invalidState);Applies to lines 30, 40, 59, 60, 73, 97, 100, 113.
🤖 Prompt for AI Agents
In `@tests/unit/js/modules/schema/routines.spec.ts` around lines 28 - 41, Replace
the unsafe `as any` test fixtures with correctly typed values: for valid
fixtures returned from getServerState (used in the test invoking sanitizeState
with schema and storeName) use TypeScript's `satisfies z.infer<typeof schema>`
to assert they match the schema type, and for invalid fixtures use the `unknown`
type (or a typed `unknown` variable) to represent values that should fail
validation; update the mocked getServerState calls
(jest.mocked(getServerState).mockReturnValue(...)) to return these properly
typed fixtures so the tests compile under strict mode while still exercising
sanitizeState and the onError behavior.
| jest.mock( '@wordpress/interactivity', () => ( { | ||
| getConfig: jest.fn(), | ||
| getContext: jest.fn(), | ||
| getServerContext: jest.fn(), | ||
| getServerState: jest.fn(), | ||
| } ) ); | ||
|
|
There was a problem hiding this comment.
Add beforeEach to clear mocks between tests.
This keeps mock state isolated across tests and aligns with the test guidelines.
✅ Suggested addition
jest.mock( '@wordpress/interactivity', () => ( {
getConfig: jest.fn(),
getContext: jest.fn(),
getServerContext: jest.fn(),
getServerState: jest.fn(),
} ) );
+
+beforeEach( () => {
+ jest.clearAllMocks();
+} );As per coding guidelines, Clear mocks between tests with beforeEach(() => { jest.clearAllMocks() }) in TypeScript tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.mock( '@wordpress/interactivity', () => ( { | |
| getConfig: jest.fn(), | |
| getContext: jest.fn(), | |
| getServerContext: jest.fn(), | |
| getServerState: jest.fn(), | |
| } ) ); | |
| jest.mock( '@wordpress/interactivity', () => ( { | |
| getConfig: jest.fn(), | |
| getContext: jest.fn(), | |
| getServerContext: jest.fn(), | |
| getServerState: jest.fn(), | |
| } ) ); | |
| beforeEach( () => { | |
| jest.clearAllMocks(); | |
| } ); |
🤖 Prompt for AI Agents
In `@tests/unit/js/modules/schema/schema.spec.ts` around lines 6 - 12, Add a
test-level beforeEach that clears Jest mocks so mock state from the jest.mock of
'@wordpress/interactivity' (getConfig, getContext, getServerContext,
getServerState) is reset between tests; place this beforeEach in the same spec
file (tests/unit/js/modules/schema/schema.spec.ts) after the jest.mock block and
have it call Jest's clearAllMocks to ensure each test starts with clean mock
state.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.