fix: address code review issues in conditional logic

- Add explicit boolean handling for toggle equals/not_equals
- Fix toNum to return NaN instead of 0 for empty values
- Use NaN guards in greater_than/less_than comparisons
- Add cleanupStaleConditions after field reorder/delete
- Clean up empty conditions objects before save

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Luca Oelfke 2026-02-13 20:46:56 +01:00
parent 5efc6a557d
commit fbc2ada08b
2 changed files with 40 additions and 5 deletions

View file

@ -61,6 +61,20 @@ export class FormBuilderModal extends Modal {
this.contentEl.empty(); this.contentEl.empty();
} }
private cleanupStaleConditions(): void {
for (let i = 0; i < this.draft.fields.length; i++) {
const field = this.draft.fields[i];
if (!field.conditions) continue;
const precedingIds = new Set(this.draft.fields.slice(0, i).map((f) => f.id));
field.conditions.rules = field.conditions.rules.filter((r) =>
precedingIds.has(r.fieldId),
);
if (field.conditions.rules.length === 0) {
field.conditions = undefined;
}
}
}
private pushSnapshot(): void { private pushSnapshot(): void {
// Truncate any forward history // Truncate any forward history
this.history = this.history.slice(0, this.historyIndex + 1); this.history = this.history.slice(0, this.historyIndex + 1);
@ -272,6 +286,12 @@ export class FormBuilderModal extends Modal {
new Notice('Form name cannot be empty.'); new Notice('Form name cannot be empty.');
return; return;
} }
// Clean up empty conditions before saving
for (const f of this.draft.fields) {
if (f.conditions && f.conditions.rules.length === 0) {
f.conditions = undefined;
}
}
this.onSave(this.draft); this.onSave(this.draft);
this.close(); this.close();
}); });
@ -317,6 +337,7 @@ export class FormBuilderModal extends Modal {
this.draft.fields[i], this.draft.fields[i],
this.draft.fields[i - 1], this.draft.fields[i - 1],
]; ];
this.cleanupStaleConditions();
this.render(); this.render();
}); });
} }
@ -334,6 +355,7 @@ export class FormBuilderModal extends Modal {
this.draft.fields[i + 1], this.draft.fields[i + 1],
this.draft.fields[i], this.draft.fields[i],
]; ];
this.cleanupStaleConditions();
this.render(); this.render();
}); });
} }
@ -347,6 +369,7 @@ export class FormBuilderModal extends Modal {
deleteBtn.addEventListener('click', () => { deleteBtn.addEventListener('click', () => {
this.pushSnapshot(); this.pushSnapshot();
this.draft.fields.splice(i, 1); this.draft.fields.splice(i, 1);
this.cleanupStaleConditions();
this.render(); this.render();
}); });
@ -398,6 +421,7 @@ export class FormBuilderModal extends Modal {
if (this.dragSourceIndex < i) targetIndex--; if (this.dragSourceIndex < i) targetIndex--;
this.draft.fields.splice(targetIndex, 0, moved); this.draft.fields.splice(targetIndex, 0, moved);
this.dragSourceIndex = null; this.dragSourceIndex = null;
this.cleanupStaleConditions();
this.render(); this.render();
}); });

View file

@ -30,17 +30,27 @@ function evaluateRule(
case 'is_not_empty': case 'is_not_empty':
return !isEmpty(value); return !isEmpty(value);
case 'equals': case 'equals':
if (typeof value === 'boolean') {
return value === (rule.value === 'true' || rule.value === true);
}
return String(value ?? '') === String(rule.value ?? ''); return String(value ?? '') === String(rule.value ?? '');
case 'not_equals': case 'not_equals':
if (typeof value === 'boolean') {
return value !== (rule.value === 'true' || rule.value === true);
}
return String(value ?? '') !== String(rule.value ?? ''); return String(value ?? '') !== String(rule.value ?? '');
case 'contains': case 'contains':
return checkContains(value, rule.value); return checkContains(value, rule.value);
case 'not_contains': case 'not_contains':
return !checkContains(value, rule.value); return !checkContains(value, rule.value);
case 'greater_than': case 'greater_than': {
return toNum(value) > toNum(rule.value); const a = toNum(value), b = toNum(rule.value);
case 'less_than': return !isNaN(a) && !isNaN(b) && a > b;
return toNum(value) < toNum(rule.value); }
case 'less_than': {
const a = toNum(value), b = toNum(rule.value);
return !isNaN(a) && !isNaN(b) && a < b;
}
default: default:
return true; return true;
} }
@ -61,7 +71,8 @@ function checkContains(value: unknown, search: unknown): boolean {
} }
function toNum(value: unknown): number { function toNum(value: unknown): number {
return parseFloat(String(value ?? '')) || 0; if (value === null || value === undefined || value === '') return NaN;
return parseFloat(String(value));
} }
/** /**