code-review

name

code-review

description

THIS IS THE PR REVIEW + CODE REVIEW SKILL. Invoke for any review intent — single file, diff, branch, or full pull request — against Texty's standards (WordPress Coding Standards on PHP, the 5 frontend rules in `frontend-dev`). Covers line-level violations, backward-compatibility breaks, fatal-error risks, and PR-level merge readiness. TRIGGER — invoke immediately when the user says ANY of these (case-insensitive, with or without a leading slash): - "pr review", "PR review", "review pr", "review PR", "review this pr", "review the pr" - "code review", "review this code", "review the code", "code review this" - "review #123", "review pr #123", "review PR-123", "review my changes", "review my branch", "review this branch", "review this diff" - "check this pr", "audit this pr", "audit this code", "audit this file", "audit this diff", "audit this branch" - "is this ready to merge", "is this safe to merge", "approve this pr", "approve this", "ship review" - "find issues in this branch", "what's wrong with X", "any problems with this" - User pastes a GitHub PR URL, a PR number (`#123`), or a diff - Task involves: PHPCS / phpcs warnings, security audit, backward-compatibility check, fatal-error audit, architecture compliance, merge readiness, changelog verification, label triage This is the ONLY review skill in the project — there is no separate `pr-review`. Both "pr review" and "code review" mean: invoke `code-review`. SKIP (do NOT invoke): - Active development on a fresh feature (use `backend-dev` / `frontend-dev` — those guide writing, not auditing) - Pure git operations (branch / commit / push) without a review intent

Texty Code Review

Audits code against project standards. Invoke for any review intent — single file, diff, branch, or full PR. Consult backend-dev and frontend-dev for the underlying rules; this skill is the audit checklist + PR-readiness gate.

When the input is a single file or snippet, skip the PR-level checks section. When the input is a full PR (description, branch, multiple files), run the whole skill.

---

1. Critical violations to flag (line-level)

Backend PHP — security (CRITICAL)

• Missing permission_callback on a register_rest_route call. Mandatory; never omit. Use [ $this, 'admin_permissions_check' ] for admin-only endpoints (inherited from Api\Base).
• Unsanitized $request input flowing into options, meta, queries, or messages. Every param: sanitize_text_field, absint, sanitize_email, sanitize_key, wp_kses_post — pick the right one for the type.
• Unescaped output — esc_html, esc_attr, esc_url, wp_kses_post in templates and direct output. (phpcs.xml disables WordPress.Security.EscapeOutput.OutputNotEscaped, so the linter won't catch this — humans must.)
• Direct SQL without $wpdb->prepare() for any dynamic value. String interpolation into a query is never acceptable.
• Missing nonce verification on non-REST form submissions (wp_verify_nonce, check_admin_referer).
• Capability check at the wrong level — admin endpoints must require manage_options, not a lower cap.

Backend PHP — coding standards (ERROR)

These will fail composer phpcs:

• Loose comparison (==, !=) — must be strict (===, !==). Rule: Universal.Operators.StrictComparisons.
• in_array() without true — must pass true as third argument. Rule: WordPress.PHP.StrictInArray.MissingTrueStrict.
• Wrong text domain — every translation function must end with 'texty'. Rule: WordPress.WP.I18n.
• Missing translator comment before sprintf() with placeholders.
• Concatenating translated strings — __('Hello ', 'texty') . $name is wrong; use sprintf( __('Hello %s', 'texty'), $name ).

Backend PHP — architecture (ERROR)

• Inline fully-qualified class names — \WP_Error, \Exception, \DateTimeImmutable, \WP_REST_Response in code or PHPDoc. Every external class gets a use ClassName; at the top of the file; references in the body and docblocks use the short name. One use per line, no inline FQCN dodge. See backend-dev: Class imports.
• Direct instantiation of registry classes — new Gateways(), new Settings(), new Notifications(). Use texty()->gateways(), ->settings(), ->notifications().
• REST controller missing from Api::__construct array — routes silently won't register.
• REST controller not extending Api\Base — must inherit admin_permissions_check.
• Hook name without texty_ prefix — apply_filters('my_event', ...) is wrong; must be texty_my_event. Collision risk with other plugins.
• WC notification overriding send() without re-applying _texty_{id} order meta — will fire duplicate SMSs on order status flips.
• New gateway not registered via texty_register_gateways action.
• Integration constructed without class_exists() guard for the host plugin's main class.
• New table column added to schema without updating SmsStat model ($data, $casts, getter/setter).
• Package added to Mozart packages list without a real namespace-collision reason — most modern composer packages don't need prefixing.

Backend PHP — documentation (WARNING)

• Missing PHPDoc on public/protected methods.
• Missing @since on new public/protected methods or new hooks the pro plugin may rely on.
• Missing hook docblock above apply_filters() / do_action() calls — consumers need to know each param.
• Missing @deprecated on a method/hook scheduled for removal — must note the version it became deprecated and the replacement.

Frontend (React/TypeScript)

The 7 rules in frontend-dev — flag any violation:

1. Wrong text domain — every __() / _n() / _x() ends with 'texty'. Empty domain or wrong slug = ERROR.
2. function Foo() declarations for components, handlers, or helpers — must be arrow functions assigned to const. ERROR.
3. Bare await without try / catch — every async boundary needs try / catch / finally with errors surfaced via toast.error (not just console.error). ERROR.
4. Implicit any / untyped props / untyped event handlers — every component has a Props type, every function has explicit param types. ERROR.
5. Inline style={{...}} (except genuinely dynamic chart-color/transform values), hand-rolled CSS classes for component styling, OR template-literal / string concat / classnames lib for dynamic className composition. Use Tailwind utilities + cn from @wedevs/plugin-ui for any conditional class. ERROR.
6. Icons not from lucide-react — @wordpress/icons, dashicons, inline SVG (except the brand logo in Header.tsx), or emoji-as-icon are all wrong. Use named imports from lucide-react with className="size-N" for sizing. ERROR.
7. + string concatenation for interpolation ('/api/' + path or 'Hello ' + name) — use a template literal (`/api/${path}`, `Hello ${name}`). Plain single-string literals are fine; only the interpolation case is enforced. ERROR.

Plus the framework-specific traps:

• React.FormEvent / React.ChangeEvent namespace types — deprecated in React 19 (TS6385). Use named imports: import { type FormEvent } from 'react'.
• asChild prop on plugin-ui triggers — wrong; plugin-ui is base-ui, use render={<button .../>}.
• TooltipProvider delayDuration={...} — wrong prop and wrong nesting; Tooltip self-wraps, and the prop is delay.
• react-toastify import — removed from this project; use import { toast } from '@wedevs/plugin-ui'.
• Direct recharts use without ChartContainer — wraps it so theme tokens (var(--color-<key>)) flow.
• JS hook name not snake_case + texty_ prefix — applyFilters('texty.routes', ...) / applyFilters('textyRoutes', ...) / applyFilters('routes', ...) are all wrong. Must be texty_routes. Same convention as PHP hooks; see frontend-dev: Hook names. ERROR.
• useEffect(async () => ...) — effect callbacks must not be async. Use inner async function plus cancelled flag.
• Foo.js left in place after creating Foo.tsx — webpack resolves .js first, so the new file is shadowed.
• Missing route ↔ submenu sync — adding routes.tsx entry without updating Admin\Menu::register_menu (or vice versa). Submenu id must equal route path without leading slash.

---

2. Backward compatibility (BLOCKING)

A break here is CRITICAL — pro plugin or live sites will fail on update. Treat any "yes" without an explicit migration plan as Request changes.

• No public/protected method renamed on an extensible class (gateways, notifications, registries, REST controllers).
• No required parameter added to an existing public/protected method.
• No method visibility tightened (public → protected, protected → private).
• No texty_* hook renamed. New hook added alongside the old one is fine; replacing is not.
• No hook parameter list shrunk or reordered. Adding a new param at the end is fine.
• No filter return-type change (e.g., previously returned array, now returns string).
• No option key renamed (texty_settings, texty_notifications, etc.).
• No meta key renamed (texty_phone, _texty_{id}).
• No column dropped from wp_texty_sms_stat.
• No REST response field removed or renamed. Adding new fields is fine.
• No args schema tightened (narrower enum, lower maxLength) — existing clients will get 400s.
• No window.texty localized-data field removed or renamed (Pro JS reads this).

If any item is "no" with a justification, the PR description must include:

• The replacement (new name / new shape).
• A deprecation shim using _deprecated_function() / apply_filters_deprecated() / do_action_deprecated() for at least one release.
• A note in the changelog calling out the breaking-change window.

---

3. Fatal error prevention

A fatal in a published WordPress plugin = white screen for users. Verify before approving:

• defined( 'ABSPATH' ) || exit; at the top of every new PHP file under includes/.
• Every reference to a third-party class (WooCommerce, WeDevs_Dokan, WC_Subscriptions, etc.) is class_exists()-guarded before the reference, not after.
• Every texty() registry call result that can be falsy is guarded — texty()->settings()->gateway() may return '' or false.
• Every WP/WC API call that can return WP_Error is wrapped in is_wp_error() before treating the result as success.
• DataLayerFactory::make_store() result is null-checked before use.
• Activation hook (Texty::activate) work is wrapped in try/catch — a throw here locks users out of the plugin.
• Schema migration is idempotent — dbDelta is safe to re-run; custom ALTER TABLE calls must check INFORMATION_SCHEMA first.
• No PHP 8+ syntax in code that must run on PHP 7.4 (constructor property promotion, named args, enums, readonly, match, mixed type) — Texty's plugin header says Requires PHP: 7.4.
• No translation calls at top-level file load (before init) — triggers the WP 6.7 just-in-time _load_textdomain_just_in_time notice.
• No new top-level code in texty.php or includes/*.php that runs DB queries before plugins_loaded.

If the diff touches the activation flow, the send pipeline, or the REST layer, the author should manually test on a fresh WordPress install (deactivate → reactivate → fire the change path). Verify they did.

---

4. PR-level checks (when reviewing a full PR)

Skip this section when reviewing a single file or snippet.

Branch hygiene

• Base branch is develop. PRs against main / master should be rejected unless it's a hotfix/release.
• Feature branches branch off develop and should be rebased on the latest develop before merge — no merge commits from develop polluting history.
• Single concern per PR — if the diff touches the dashboard refactor and a gateway fix and a doc tweak, ask the author to split. Reviewable PRs are small.
• No unrelated formatting churn — Prettier/PHPCBF "while I was here" reformats of untouched files make the diff unreadable. Push back.

PR description checklist

A reviewable Texty PR description should answer:

1. What changes? — One sentence summary.
2. Why? — Linked issue, bug report, or feature spec.
3. How was it tested? — Specific manual steps. There is no PHP test suite, so manual verification is mandatory for backend changes. For UI changes, a browser test (golden path + an edge case) is the bar.
4. Screenshots / screen recording — required for any visual change.
5. Backwards compatibility notes — does the change rename a hook, change an option shape, alter a REST response? If yes, the PR description must say so explicitly.
6. Pro-plugin impact — if the change adds/removes/renames a texty_* hook or REST field, the description must call out whether the pro plugin needs an update.

If any of 1–4 is missing, the verdict is request changes — not because the code is wrong, but because the PR isn't reviewable yet.

Pre-merge gates

Run / confirm these locally (CI runs release.yml on tag push, not on PR — so you can't lean on CI):

# PHP
composer phpcs              # WordPress Coding Standards — must be clean
php -l includes/**/*.php    # syntax check (or rely on phpcs)

# Frontend
npm run typecheck           # tsc --noEmit, strict mode
npm run build               # webpack — only the standard "asset size" warnings allowed

# Build artifact (release-candidate PRs only)
bin/build.sh                # produces build/texty.zip

PHPCS / typecheck failures block the review — fix-or-document first, before line-by-line.

Specific things to verify

Backend changes:

• Does any new register_rest_route have a permission_callback?
• New REST controller? Did the author append it to Api::__construct and confirm the route shows up at wp-json/texty/v1?
• New gateway? Tested with at least one real send (or Fake gateway when WP_DEBUG)?
• New notification? Manually triggered the source event (user register, comment, order status change) and confirmed delivery?
• Schema change to wp_texty_sms_stat? Author bumped TEXTY_VERSION so Install::run() re-runs dbDelta on activation? Existing rows survive the migration?
• New texty_* hook? Has a docblock with each param described.
• Mozart packages list grew? Genuinely needed, or could the dependency be loaded normally?

Frontend changes:

• Build clean? (typecheck + webpack)
• New page added? Three-place edit confirmed: src/pages/<name>/index.tsx, src/routing/routes.tsx, includes/Admin/Menu.php $submenus?
• New REST call wrapped in try / catch with errors surfaced via toast.error?
• Components migrated from .js → .tsx? Old .js files deleted, not left as shadowed dead code?
• Tailwind classes only — no new style={{}} (except recharts SVG props), no new .css files?
• All strings go through __('...', 'texty')? POT/JSON regenerated if user-facing strings changed?

Cross-cutting:

• Any composer.lock / package-lock.json changes? Did the lockfile drift from a real composer.json / package.json change, or is it accidental?
• Any new env var, build flag, or config file? Documented in CLAUDE.md or README.md?
• Any commit with hooks bypassed (--no-verify)? Block — investigate why a hook was failing instead.

---

5. Review approach

1. Determine scope. Is this a single file, a diff, or a full PR? Skip section 4 for the first two.
2. Generate the diff.

   git diff --stat develop...HEAD
   git diff develop...HEAD -- 'includes/**/*.php'
   git diff develop...HEAD -- 'src/**'
   

3. Run the gates (full PR only) — composer phpcs, npm run typecheck, npm run build, php -l <changed.php>.
4. Read the PR description first (full PR only). If it's incomplete, request changes immediately and don't read the diff yet — wait for the author to fill it in.
5. Scan for critical violations (section 1) — security first, then architecture.
6. Walk the BC checklist (section 2). Any unjustified break = blocker.
7. Walk the fatal-prevention checklist (section 3). Any unguarded third-party class / unwrapped WP_Error / activation throw = blocker.
8. Verify REST patterns when controllers changed: permission_callback, sanitization, schema validation, WP_Error returns, controller registration.
9. Check extensibility — does the change expose filters/actions where the pro plugin or third parties will need to hook in? texty_* prefix on every name?
10. Review tests / sanity checks. No PHP test suite in this repo, so manual verification through Texty > Tools > Send Test Message (for send-pipeline changes) and browser testing (for UI) is the bar. Push back if missing.
11. Synthesize a verdict.

---

6. Output format

For each finding:

[severity]: [Specific problem]
Location: path/to/file.php:42  (or src/pages/dashboard/index.tsx:88)
Standard: [Reference — e.g. "code-review §1: REST API > Permission Callbacks", or "frontend-dev: Rule 3 try/catch"]
Fix: [Brief explanation or correct code snippet]

Severity levels

• CRITICAL — Security issue, data loss risk, broken functionality, BC break without deprecation, unguarded fatal risk.
• ERROR — Standards violation, build will fail, or required pattern missing.
• WARNING — Suboptimal approach, missing best practice, or extensibility gap.
• SUGGESTION — Improvement opportunity, not a violation.

Verdict criteria

Verdict	When
Approve	All gates green, CRITICAL/ERROR list empty, BC checklist clean, fatal-prevention checklist clean, PR description complete, scope single-concern.
Request changes	Any CRITICAL or ERROR. Any unjustified BC break (renamed hook/method/option/column, removed REST field, tightened schema). Any unguarded third-party class reference. Missing PR description elements. Failing PHPCS / typecheck. Mixed-concern diff that should be split.
Needs discussion	Architectural choice that warrants a second opinion (new dependency, new data model, intentional breaking hook signature with migration plan, raising minimum PHP version). Don't approve or block unilaterally — escalate.
DO NOT MERGE	Touches credentials/secrets, adds a destructive/irreversible migration, drops a database column, removes a public method without deprecation, or relies on infra that isn't ready. State the blocker in plain English.

End with a one-line verdict.

Output template (full PR review)

## Verdict: [Approve | Request changes | Needs discussion | DO NOT MERGE]

**Summary:** One sentence on what this PR does and whether it's ready.

### Blockers
- [CRITICAL/ERROR] file:line — issue + fix.

### Improvements
- [WARNING] file:line — issue + suggested change.

### Nice-to-have
- [SUGGESTION] file:line — optional refinement.

### Backward compatibility
- [ ] No renamed/removed public methods, hooks, options, meta keys, columns, REST fields
- [ ] Any deprecation uses `_deprecated_function()` / `apply_filters_deprecated()` / `do_action_deprecated()` shim for ≥1 release
- [ ] Pro-plugin impact noted in description (or N/A explicitly)

### Fatal error prevention
- [ ] All third-party class references `class_exists()`-guarded
- [ ] `WP_Error` results checked before use
- [ ] `texty()` registry results null/empty-checked
- [ ] Activation hook wrapped in `try/catch`
- [ ] No PHP 8+ syntax (still PHP 7.4)
- [ ] No top-level i18n / DB calls before `init` / `plugins_loaded`

### PR description / process
- [ ] Description complete (what / why / how tested)
- [ ] Screenshots for visual changes
- [ ] PHPCS green
- [ ] Typecheck green
- [ ] Build green

---

Reviewer principles

Correct — Does it do what the task says? Are the edge cases handled? Secure — Could a malicious request bypass the permission check? Every input sanitized, every output escaped? Could a SQL injection slip in? Readable — Will future-you understand this in six months? Are names self-explanatory? Is the file scoped to one concern? Elegant — Does it match the surrounding architecture, or does it add a new shape that breaks the pattern? Reversible — If this ships and breaks production, how hard is it to back out? Is there a feature flag, or does it require another deploy?

If a CRITICAL or ERROR is found, the verdict is Request changes regardless of how clean the WARNINGs/SUGGESTIONs are. A PR that's secure, correct, readable, and elegant but adds a hidden one-way migration (irreversible schema change without rollback) is not ready — push back.