Fix code quality violations and rename select input components
Move small tasks from wishlist to todo, update npm packages Replace #[Auth] attributes with manual auth checks and code quality rule Remove on_jqhtml_ready lifecycle method from framework Complete ACL system with 100-based role indexing and /dev/acl tester WIP: ACL system implementation with debug instrumentation Convert rsx:check JS linting to RPC socket server Clean up docs and fix $id→$sid in man pages, remove SSR/FPC feature Reorganize wishlists: priority order, mark sublayouts complete, add email Update model_fetch docs: mark MVP complete, fix enum docs, reorganize Comprehensive documentation overhaul: clarity, compression, and critical rules Convert Contacts/Projects CRUD to Model.fetch() and add fetch_or_null() Add JS ORM relationship lazy-loading and fetch array handling Add JS ORM relationship fetching and CRUD documentation Fix ORM hydration and add IDE resolution for Base_* model stubs Rename Json_Tree_Component to JS_Tree_Debug_Component and move to framework Enhance JS ORM infrastructure and add Json_Tree class name badges 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
root
@@ -27,7 +27,6 @@ class LifecycleMethodsStatic_CodeQualityRule extends CodeQualityRule_Abstract
|
||||
'on_app_modules_init',
|
||||
'on_app_init',
|
||||
'on_app_ready',
|
||||
'on_jqhtml_ready',
|
||||
];
|
||||
|
||||
public function get_id(): string
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
namespace App\RSpade\CodeQuality\Rules\JavaScript;
|
||||
|
||||
use App\RSpade\CodeQuality\Rules\CodeQualityRule_Abstract;
|
||||
use App\RSpade\CodeQuality\Support\Js_CodeQuality_Rpc;
|
||||
|
||||
/**
|
||||
* JavaScript 'this' Usage Rule
|
||||
@@ -94,8 +95,7 @@ class ThisUsage_CodeQualityRule extends CodeQualityRule_Abstract
|
||||
}
|
||||
|
||||
/**
|
||||
* Use Node.js with acorn to parse JavaScript and find violations
|
||||
* Uses external parser script stored in resources directory
|
||||
* Analyze JavaScript file for 'this' usage violations via RPC server
|
||||
*/
|
||||
private function parse_with_acorn(string $file_path): array
|
||||
{
|
||||
@@ -125,33 +125,8 @@ class ThisUsage_CodeQualityRule extends CodeQualityRule_Abstract
|
||||
}
|
||||
}
|
||||
|
||||
// Path to the parser script
|
||||
$parser_script = __DIR__ . '/resource/this-usage-parser.js';
|
||||
|
||||
if (!file_exists($parser_script)) {
|
||||
// Parser script missing - fatal error
|
||||
throw new \RuntimeException("JS-THIS parser script missing: {$parser_script}");
|
||||
}
|
||||
|
||||
// Run Node.js parser with the external script
|
||||
$output = shell_exec("cd /tmp && node " . escapeshellarg($parser_script) . " " . escapeshellarg($file_path) . " 2>&1");
|
||||
|
||||
if (!$output) {
|
||||
return [];
|
||||
}
|
||||
|
||||
$result = json_decode($output, true);
|
||||
if (!$result) {
|
||||
return [];
|
||||
}
|
||||
|
||||
// Check for errors from the parser
|
||||
if (isset($result['error'])) {
|
||||
// Parser encountered an error but it's not fatal for the rule
|
||||
return [];
|
||||
}
|
||||
|
||||
$violations = $result['violations'] ?? [];
|
||||
// Analyze via RPC server (lazy starts if not running)
|
||||
$violations = Js_CodeQuality_Rpc::analyze_this($file_path);
|
||||
|
||||
// Cache the result
|
||||
file_put_contents($cache_file, json_encode($violations));
|
||||
|
||||
@@ -1,378 +0,0 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* JavaScript 'this' usage parser for code quality checks
|
||||
*
|
||||
* PURPOSE: Parse JavaScript files to find 'this' usage violations
|
||||
* according to RSpade coding standards.
|
||||
*
|
||||
* USAGE: node this-usage-parser.js <filepath>
|
||||
* OUTPUT: JSON with violations array
|
||||
*
|
||||
* RULES:
|
||||
* - Anonymous functions can use: const $var = $(this) as first line
|
||||
* - Instance methods must use: const that = this as first line
|
||||
* - Static methods should never use 'this', use ClassName instead
|
||||
* - Arrow functions are ignored (they inherit 'this')
|
||||
*
|
||||
* @FILENAME-CONVENTION-EXCEPTION - Node.js utility script
|
||||
*/
|
||||
|
||||
const fs = require('fs');
|
||||
const acorn = require('acorn');
|
||||
const walk = require('acorn-walk');
|
||||
|
||||
// Known jQuery callback methods - used for better remediation messages
|
||||
const JQUERY_CALLBACKS = new Set([
|
||||
'click', 'dblclick', 'mouseenter', 'mouseleave', 'mousedown', 'mouseup',
|
||||
'mousemove', 'mouseover', 'mouseout', 'change', 'submit', 'focus', 'blur',
|
||||
'keydown', 'keyup', 'keypress', 'resize', 'scroll', 'select', 'load',
|
||||
'on', 'off', 'one', 'each', 'map', 'filter',
|
||||
'fadeIn', 'fadeOut', 'slideDown', 'slideUp', 'animate',
|
||||
'done', 'fail', 'always', 'then', 'ready', 'hover'
|
||||
]);
|
||||
|
||||
function analyzeFile(filePath) {
|
||||
try {
|
||||
const code = fs.readFileSync(filePath, 'utf8');
|
||||
const lines = code.split('\n');
|
||||
|
||||
let ast;
|
||||
try {
|
||||
ast = acorn.parse(code, {
|
||||
ecmaVersion: 2020,
|
||||
sourceType: 'module',
|
||||
locations: true
|
||||
});
|
||||
} catch (e) {
|
||||
// Parse error - return empty violations
|
||||
return { violations: [], error: `Parse error: ${e.message}` };
|
||||
}
|
||||
|
||||
const violations = [];
|
||||
const classInfo = new Map(); // Track class info
|
||||
|
||||
// First pass: identify all classes and their types
|
||||
walk.simple(ast, {
|
||||
ClassDeclaration(node) {
|
||||
const hasStaticInit = node.body.body.some(member =>
|
||||
member.static && member.key?.name === 'init'
|
||||
);
|
||||
classInfo.set(node.id.name, {
|
||||
isStatic: hasStaticInit
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
// Helper to check if first line of function has valid pattern
|
||||
function checkFirstLinePattern(funcNode) {
|
||||
if (!funcNode.body || !funcNode.body.body || funcNode.body.body.length === 0) {
|
||||
return null;
|
||||
}
|
||||
|
||||
let checkIndex = 0;
|
||||
const firstStmt = funcNode.body.body[0];
|
||||
|
||||
// Check if first statement is e.preventDefault() or similar
|
||||
if (firstStmt.type === 'ExpressionStatement' &&
|
||||
firstStmt.expression?.type === 'CallExpression' &&
|
||||
firstStmt.expression?.callee?.type === 'MemberExpression' &&
|
||||
firstStmt.expression?.callee?.property?.name === 'preventDefault') {
|
||||
// First line is preventDefault, check second line for pattern
|
||||
checkIndex = 1;
|
||||
if (funcNode.body.body.length <= 1) {
|
||||
return null; // No second statement
|
||||
}
|
||||
}
|
||||
|
||||
const targetStmt = funcNode.body.body[checkIndex];
|
||||
if (targetStmt.type !== 'VariableDeclaration') {
|
||||
return null;
|
||||
}
|
||||
|
||||
const firstDecl = targetStmt.declarations[0];
|
||||
if (!firstDecl || !firstDecl.init) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const varKind = targetStmt.kind; // 'const', 'let', or 'var'
|
||||
|
||||
// Check for 'that = this' pattern
|
||||
if (firstDecl.id.name === 'that' &&
|
||||
firstDecl.init.type === 'ThisExpression') {
|
||||
if (varKind !== 'const') {
|
||||
return 'that-pattern-wrong-kind';
|
||||
}
|
||||
return 'that-pattern';
|
||||
}
|
||||
|
||||
// Check for 'CurrentClass = this' pattern (for static polymorphism)
|
||||
if (firstDecl.id.name === 'CurrentClass' &&
|
||||
firstDecl.init.type === 'ThisExpression') {
|
||||
if (varKind !== 'const') {
|
||||
return 'currentclass-pattern-wrong-kind';
|
||||
}
|
||||
return 'currentclass-pattern';
|
||||
}
|
||||
|
||||
// Check for '$var = $(this)' pattern
|
||||
if (firstDecl.id.name.startsWith('$') &&
|
||||
firstDecl.init.type === 'CallExpression' &&
|
||||
firstDecl.init.callee.name === '$' &&
|
||||
firstDecl.init.arguments.length === 1 &&
|
||||
firstDecl.init.arguments[0].type === 'ThisExpression') {
|
||||
if (varKind !== 'const') {
|
||||
return 'jquery-pattern-wrong-kind';
|
||||
}
|
||||
return 'jquery-pattern';
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
// Helper to detect if we're in a jQuery callback (best effort)
|
||||
function isLikelyJQueryCallback(ancestors) {
|
||||
for (let i = ancestors.length - 1; i >= 0; i--) {
|
||||
const node = ancestors[i];
|
||||
if (node.type === 'CallExpression' && node.callee.type === 'MemberExpression') {
|
||||
const methodName = node.callee.property.name;
|
||||
if (JQUERY_CALLBACKS.has(methodName)) {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
// Walk the AST looking for 'this' usage
|
||||
walk.ancestor(ast, {
|
||||
ThisExpression(node, ancestors) {
|
||||
// Skip arrow functions - they inherit 'this'
|
||||
for (const ancestor of ancestors) {
|
||||
if (ancestor.type === 'ArrowFunctionExpression') {
|
||||
return; // Skip - arrow functions inherit context
|
||||
}
|
||||
}
|
||||
|
||||
// Find containing function and class
|
||||
let containingFunc = null;
|
||||
let containingClass = null;
|
||||
let isAnonymousFunc = false;
|
||||
let isStaticMethod = false;
|
||||
let isConstructor = false;
|
||||
let isInstanceMethod = false;
|
||||
let hasMethodDefinition = false;
|
||||
|
||||
// First pass: check if we're in a MethodDefinition
|
||||
for (let i = ancestors.length - 1; i >= 0; i--) {
|
||||
const ancestor = ancestors[i];
|
||||
if (ancestor.type === 'MethodDefinition') {
|
||||
hasMethodDefinition = true;
|
||||
isStaticMethod = ancestor.static;
|
||||
isConstructor = ancestor.kind === 'constructor';
|
||||
isInstanceMethod = !ancestor.static && ancestor.kind !== 'constructor';
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
// Second pass: find function and class
|
||||
for (let i = ancestors.length - 1; i >= 0; i--) {
|
||||
const ancestor = ancestors[i];
|
||||
|
||||
if (!containingFunc && (
|
||||
ancestor.type === 'FunctionExpression' ||
|
||||
ancestor.type === 'FunctionDeclaration'
|
||||
)) {
|
||||
containingFunc = ancestor;
|
||||
// Only mark as anonymous if NOT inside a MethodDefinition
|
||||
isAnonymousFunc = ancestor.type === 'FunctionExpression' && !hasMethodDefinition;
|
||||
}
|
||||
|
||||
if (!containingClass && (
|
||||
ancestor.type === 'ClassDeclaration' ||
|
||||
ancestor.type === 'ClassExpression'
|
||||
)) {
|
||||
containingClass = ancestor;
|
||||
}
|
||||
}
|
||||
|
||||
if (!containingFunc) {
|
||||
return; // Not in a function
|
||||
}
|
||||
|
||||
// Skip constructors - 'this' is allowed for property assignment
|
||||
if (isConstructor) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Skip instance methods - 'this' is allowed directly in instance methods
|
||||
// Only enforce aliasing for anonymous functions and static methods
|
||||
if (isInstanceMethod) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Check if this is part of the allowed first-line pattern with const
|
||||
const parent = ancestors[ancestors.length - 2];
|
||||
const firstStmt = containingFunc.body?.body?.[0];
|
||||
let checkIndex = 0;
|
||||
|
||||
// Check if first statement is preventDefault
|
||||
const hasPreventDefault = firstStmt?.type === 'ExpressionStatement' &&
|
||||
firstStmt?.expression?.type === 'CallExpression' &&
|
||||
firstStmt?.expression?.callee?.type === 'MemberExpression' &&
|
||||
firstStmt?.expression?.callee?.property?.name === 'preventDefault';
|
||||
|
||||
if (hasPreventDefault) {
|
||||
checkIndex = 1;
|
||||
}
|
||||
|
||||
const targetStmt = containingFunc.body?.body?.[checkIndex];
|
||||
const isTargetConst = targetStmt?.type === 'VariableDeclaration' && targetStmt?.kind === 'const';
|
||||
|
||||
// Check if this 'this' is inside $(this) on the first or second line
|
||||
// For jQuery pattern: const $var = $(this)
|
||||
if (parent && parent.type === 'CallExpression' && parent.callee?.name === '$') {
|
||||
// This is $(this) - check if it's in the right position with const
|
||||
if (isTargetConst &&
|
||||
targetStmt?.declarations?.[0]?.init === parent &&
|
||||
targetStmt?.declarations?.[0]?.id?.name?.startsWith('$')) {
|
||||
return; // This is const $var = $(this) in correct position
|
||||
}
|
||||
}
|
||||
|
||||
// Check if this 'this' is the 'const that = this' in correct position
|
||||
if (parent && parent.type === 'VariableDeclarator' &&
|
||||
parent.id?.name === 'that' &&
|
||||
isTargetConst &&
|
||||
targetStmt?.declarations?.[0]?.id?.name === 'that') {
|
||||
return; // This is 'const that = this' in correct position
|
||||
}
|
||||
|
||||
// Check if this 'this' is the 'const CurrentClass = this' in correct position
|
||||
if (parent && parent.type === 'VariableDeclarator' &&
|
||||
parent.id?.name === 'CurrentClass' &&
|
||||
isTargetConst &&
|
||||
targetStmt?.declarations?.[0]?.id?.name === 'CurrentClass') {
|
||||
return; // This is 'const CurrentClass = this' in correct position
|
||||
}
|
||||
|
||||
// Check what pattern is used
|
||||
const pattern = checkFirstLinePattern(containingFunc);
|
||||
|
||||
// Determine the violation and remediation
|
||||
let message = '';
|
||||
let remediation = '';
|
||||
const lineNum = node.loc.start.line;
|
||||
const codeSnippet = lines[lineNum - 1].trim();
|
||||
const className = containingClass?.id?.name || 'unknown';
|
||||
const isJQueryContext = isLikelyJQueryCallback(ancestors);
|
||||
|
||||
// Anonymous functions take precedence - even if inside a static method
|
||||
if (isAnonymousFunc) {
|
||||
if (!pattern) {
|
||||
// Check if there's a preventDefault on the first line
|
||||
const firstStmt = containingFunc.body?.body?.[0];
|
||||
const hasPreventDefault = firstStmt?.type === 'ExpressionStatement' &&
|
||||
firstStmt?.expression?.type === 'CallExpression' &&
|
||||
firstStmt?.expression?.callee?.type === 'MemberExpression' &&
|
||||
firstStmt?.expression?.callee?.property?.name === 'preventDefault';
|
||||
|
||||
if (isJQueryContext) {
|
||||
message = `'this' in jQuery callback should be aliased for clarity.`;
|
||||
if (hasPreventDefault) {
|
||||
remediation = `Add 'const $element = $(this);' as the second line (after preventDefault), then use $element instead of 'this'.`;
|
||||
} else {
|
||||
remediation = `Add 'const $element = $(this);' as the first line of this function, then use $element instead of 'this'.`;
|
||||
}
|
||||
} else {
|
||||
message = `Ambiguous 'this' usage in anonymous function.`;
|
||||
if (hasPreventDefault) {
|
||||
remediation = `If this is a jQuery callback: Add 'const $element = $(this);' as the second line (after preventDefault).\n` +
|
||||
`If this is an instance context: Add 'const that = this;' as the second line.\n` +
|
||||
`Then use the aliased variable instead of 'this'.`;
|
||||
} else {
|
||||
remediation = `If this is a jQuery callback: Add 'const $element = $(this);' as first line.\n` +
|
||||
`If this is an instance context: Add 'const that = this;' as first line.\n` +
|
||||
`Then use the aliased variable instead of 'this'.`;
|
||||
}
|
||||
}
|
||||
} else if (pattern === 'that-pattern' || pattern === 'jquery-pattern') {
|
||||
message = `'this' used after aliasing. Use the aliased variable instead.`;
|
||||
// Find the variable declaration (might be first or second statement)
|
||||
let varDeclIndex = 0;
|
||||
const firstStmt = containingFunc.body?.body?.[0];
|
||||
if (firstStmt?.type === 'ExpressionStatement' &&
|
||||
firstStmt?.expression?.callee?.property?.name === 'preventDefault') {
|
||||
varDeclIndex = 1;
|
||||
}
|
||||
const varName = containingFunc.body?.body?.[varDeclIndex]?.declarations?.[0]?.id?.name;
|
||||
remediation = pattern === 'jquery-pattern'
|
||||
? `You already have 'const ${varName} = $(this)'. Use that variable instead of 'this'.`
|
||||
: `You already have 'const that = this'. Use 'that' instead of 'this'.`;
|
||||
} else if (pattern === 'that-pattern-wrong-kind') {
|
||||
message = `Instance alias must use 'const', not 'let' or 'var'.`;
|
||||
remediation = `Change to 'const that = this;' - the instance reference should never be reassigned.`;
|
||||
} else if (pattern === 'jquery-pattern-wrong-kind') {
|
||||
message = `jQuery element alias must use 'const', not 'let' or 'var'.`;
|
||||
remediation = `Change to 'const $element = $(this);' - jQuery element references should never be reassigned.`;
|
||||
}
|
||||
} else if (isStaticMethod) {
|
||||
if (!pattern) {
|
||||
message = `Static method in '${className}' should not use naked 'this'.`;
|
||||
remediation = `Static methods have two options:\n` +
|
||||
`1. If you need the exact class (no polymorphism): Replace 'this' with '${className}'\n` +
|
||||
`2. If you need polymorphism (inherited classes): Add 'const CurrentClass = this;' as first line\n` +
|
||||
` Then use CurrentClass.name or CurrentClass.property for polymorphic access.\n` +
|
||||
`Consider: Does this.name need to work for child classes? If yes, use CurrentClass pattern.`;
|
||||
} else if (pattern === 'currentclass-pattern') {
|
||||
message = `'this' used after aliasing to CurrentClass. Use 'CurrentClass' instead.`;
|
||||
remediation = `You already have 'const CurrentClass = this;'. Use 'CurrentClass' for all access, not naked 'this'.`;
|
||||
} else if (pattern === 'currentclass-pattern-wrong-kind') {
|
||||
message = `CurrentClass pattern must use 'const', not 'let' or 'var'.`;
|
||||
remediation = `Change to 'const CurrentClass = this;' - the CurrentClass reference should never be reassigned.`;
|
||||
} else if (pattern === 'jquery-pattern') {
|
||||
// jQuery pattern in static method's anonymous function is OK
|
||||
return;
|
||||
} else if (pattern === 'jquery-pattern-wrong-kind') {
|
||||
message = `jQuery element alias must use 'const', not 'let' or 'var'.`;
|
||||
remediation = `Change to 'const $element = $(this);' - jQuery element references should never be reassigned.`;
|
||||
}
|
||||
|
||||
if (isAnonymousFunc && !pattern) {
|
||||
remediation += `\nException: If this is a jQuery callback, add 'const $element = $(this);' as the first line.`;
|
||||
}
|
||||
}
|
||||
// NOTE: Instance methods are exempt from this rule - they can use 'this' directly
|
||||
// The check returns early for instance methods, so this else block is unreachable for them
|
||||
|
||||
if (message) {
|
||||
violations.push({
|
||||
line: lineNum,
|
||||
message: message,
|
||||
codeSnippet: codeSnippet,
|
||||
remediation: remediation
|
||||
});
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
return { violations: violations };
|
||||
|
||||
} catch (error) {
|
||||
return { violations: [], error: error.message };
|
||||
}
|
||||
}
|
||||
|
||||
// Main execution
|
||||
const filePath = process.argv[2];
|
||||
if (!filePath) {
|
||||
console.log(JSON.stringify({ violations: [], error: 'No file path provided' }));
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
if (!fs.existsSync(filePath)) {
|
||||
console.log(JSON.stringify({ violations: [], error: `File not found: ${filePath}` }));
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
const result = analyzeFile(filePath);
|
||||
console.log(JSON.stringify(result));
|
||||
Reference in New Issue
Block a user