Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 80 additions & 25 deletions src/components/CategoryEditModal.vue
Original file line number Diff line number Diff line change
Expand Up @@ -6,29 +6,43 @@ b-modal(id="edit" ref="edit" title="Edit category" @show="resetModal" @hidden="h
b-form-input(v-model="editing.name")
b-input-group(prepend="Parent")
b-select(v-model="editing.parent", :options="allCategories")
//| ID: {{editing.id}}

hr
div.my-1
b Rule
b-input-group.my-1(prepend="Type")
b-select(v-model="editing.rule.type", :options="allRuleTypes")
div(v-if="editing.rule.type === 'regex'")
b-input-group.my-1(prepend="Pattern")
b-form-input(v-model="editing.rule.regex")
div.d-flex
// Multi-pattern list — each pattern is one line with its own remove button
div.my-1(v-for="(pattern, index) in editing.rulePatterns" :key="index")
b-input-group
b-input-group-prepend
span.input-group-text Pattern {{ index + 1 }}
b-form-input(v-model="editing.rulePatterns[index]" @input="validateSinglePattern")
b-input-group-append
b-btn(variant="outline-danger" @click="removePattern(index)" :disabled="editing.rulePatterns.length <= 1")
icon(name="trash")
div.d-flex.align-items-center
div.flex-grow-1
b-btn.mt-1(size="sm" variant="outline-primary" @click="addPattern()")
icon.mr-1(name="plus")
| Add pattern
div.flex-grow-1.text-right
small.text-muted {{ editing.rulePatterns.length }} pattern(s)
div.d-flex.mt-2
div.flex-grow-1
b-form-checkbox(v-model="editing.rule.ignore_case" switch)
| Case insensitive
div.flex-grow-1
small.text-right
div.text-danger(v-if="!validPattern") Invalid pattern
div.text-warning(v-if="validPattern && broad_pattern") Pattern too broad
div.text-danger(v-if="!validPattern") Invalid pattern(s)
div.text-warning(v-if="validPattern && broad_pattern && !patternErrors.length") Pattern(s) too broad
div.text-warning(v-if="patternErrors.length > 0")
| Pattern(s) {{ patternErrors.join(', ') }} invalid
Comment on lines +38 to +41

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.

P1 Duplicate error messages shown simultaneously

Both !validPattern (red "Invalid pattern(s)") and patternErrors.length > 0 (yellow "Pattern(s) X invalid") fire for exactly the same condition — any invalid entry makes validPattern false and also populates patternErrors. A user typing a broken regex sees both messages at once, which is redundant and visually contradictory (red danger + yellow warning for the same fact). The generic red message should only show when patternErrors is empty but validation still fails (i.e. !validPattern && !patternErrors.length).

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


hr
div.my-1
b Color

b-form-checkbox(v-model="editing.inherit_color" switch)
| Inherit parent color
div.mt-1(v-show="!editing.inherit_color")
Expand All @@ -54,9 +68,15 @@ import _ from 'lodash';
import ColorPicker from '~/components/ColorPicker.vue';
import { useCategoryStore } from '~/stores/categories';
import { mapState } from 'pinia';
import { validateRegex, isRegexBroad } from '~/util/validate';
import {
validateRegex,
isRegexBroad,
splitRegexPipe,
joinRegexPipe,
} from '~/util/validate';

import 'vue-awesome/icons/trash';
import 'vue-awesome/icons/plus';

export default {
name: 'CategoryEditModal',
Expand All @@ -71,15 +91,17 @@ export default {
categoryStore: useCategoryStore(),

editing: {
id: 0, // FIXME: Use ID assigned to category in store, in order for saves to be uniquely targeted
id: 0,
name: null,
rule: {},
parent: [],
rule: {} as { type: string; regex?: string; ignore_case?: boolean },
rulePatterns: [] as string[], // UI-only: split view of rule.regex
parent: [] as string[],
inherit_color: true,
color: null,
color: null as string | null,
inherit_score: true,
score: null,
score: null as number | null,
},
patternErrors: [] as number[], // 1-based indices of invalid patterns
};
},
computed: {
Expand All @@ -90,17 +112,25 @@ export default {
return [
{ value: 'none', text: 'None' },
{ value: 'regex', text: 'Regular Expression' },
//{ value: 'glob', text: 'Glob pattern' },
];
},
valid: function () {
return this.editing.rule.type !== 'none' && this.validPattern;
},
validPattern: function () {
return this.editing.rule.type === 'regex' && validateRegex(this.editing.rule.regex || '');
if (this.editing.rule.type !== 'regex') return true;
const patterns = (this.editing.rulePatterns || [])
.map(p => (p || '').trim())
.filter(p => p.length > 0);
if (patterns.length === 0) return false;
return patterns.every(p => validateRegex(p));
},
broad_pattern: function () {
return this.editing.rule.type === 'regex' && isRegexBroad(this.editing.rule.regex || '');
if (this.editing.rule.type !== 'regex') return false;
const patterns = (this.editing.rulePatterns || [])
.map(p => (p || '').trim())
.filter(p => p.length > 0);
return patterns.some(p => isRegexBroad(p));
},
},
watch: {
Expand All @@ -123,44 +153,64 @@ export default {
this.$emit('hidden');
},
removeClass() {
// TODO: Show a confirmation dialog
// TODO: Remove children as well?
this.categoryStore.removeClass(this.categoryId);
},
checkFormValidity() {
// FIXME
return true;
},
handleOk(event) {
// Prevent modal from closing
event.preventDefault();
// Trigger submit handler
this.handleSubmit();
this.$emit('ok');
},
handleSubmit() {
// Exit when the form isn't valid
if (!this.checkFormValidity()) {
return;
}

// Save the category
// Join multi-pattern list into a single pipe-separated regex string
// so the backend receives the same format it always has.
const regex = joinRegexPipe(this.editing.rulePatterns);

const new_class = {
id: this.editing.id,
name: this.editing.parent.concat(this.editing.name),
rule: this.editing.rule.type !== 'none' ? this.editing.rule : { type: 'none' },
rule:
this.editing.rule.type !== 'none'
? {
type: 'regex',
regex,
ignore_case: this.editing.rule.ignore_case,
}
: { type: 'none' },
data: {
color: this.editing.inherit_color === true ? undefined : this.editing.color,
score: this.editing.inherit_score === true ? undefined : this.editing.score,
},
};
this.categoryStore.updateClass(new_class);

// Hide the modal manually
this.$nextTick(() => {
this.$refs.edit.hide();
});
},
addPattern() {
this.editing.rulePatterns.push('');
},
removePattern(index: number) {
if (this.editing.rulePatterns.length <= 1) return;
this.editing.rulePatterns.splice(index, 1);
this.validateSinglePattern();
},
validateSinglePattern() {
this.patternErrors = [];
this.editing.rulePatterns.forEach((p, i) => {
const trimmed = (p || '').trim();
if (trimmed.length > 0 && !validateRegex(trimmed)) {
this.patternErrors.push(i + 1); // 1-based for user display
}
});
},
resetModal() {
const cat = this.categoryStore.get_category_by_id(this.categoryId);
const color = cat.data ? cat.data.color : undefined;
Expand All @@ -171,12 +221,17 @@ export default {
id: cat.id,
name: cat.subname,
rule: _.cloneDeep(cat.rule),
rulePatterns:
cat.rule.type === 'regex' && cat.rule.regex
? splitRegexPipe(cat.rule.regex)
: [''],
parent: cat.parent ? cat.parent : [],
color,
inherit_color,
score,
inherit_score,
};
this.patternErrors = [];
},
},
};
Expand Down
11 changes: 9 additions & 2 deletions src/components/CategoryEditTree.vue
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
div
div.row.py-2.class
div.col-8.col-md-4
span(:style="{ marginLeft: (1.5 * depth) + 'em', cursor: _class.children.length > 0 ? 'pointer' : null}" @click="expanded = !expanded")
span(:style="{ marginLeft: (1.5 * depth) + 'em', cursor: _class.children.length > 0 ? 'pointer' : null }" @click="expanded = !expanded")
span(v-if="_class.children.length > 0" style="opacity: 0.8")
icon(:name="expanded ? 'regular/minus-square' : 'regular/plus-square'" scale="0.8")
span(v-else style="opacity: 0.6")
Expand All @@ -16,7 +16,10 @@ div

div.col-4.col-md-8
span.d-none.d-md-inline
span(v-if="_class.rule.type === 'regex'") Rule ({{_class.rule.type}}): #[code {{_class.rule.regex}}]
span(v-if="_class.rule.type === 'regex'")
| Rule ({{_class.rule.type}}):
div.rule-item(v-for="(pat, indx) in splitRegex(_class.rule.regex)" :key="indx")
code(:style='{ color: "#d63384 !important" }') {{ pat }}

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.

P1 !important inside a Vue :style object binding is silently ignored by browsers. Vue sets individual CSS properties via element.style.setProperty(prop, value) where the value "#d63384 !important" is treated as an invalid value and discarded, so the colour override will never take effect. Use a scoped CSS rule or the string form of :style if you need !important.

Suggested change
code(:style='{ color: "#d63384 !important" }') {{ pat }}
code.rule-code {{ pat }}

span.text-muted(v-else) No rule
span.float-right
b-btn.ml-1.border-0(size="sm", variant="outline-secondary", @click="showEditModal(_class.id)" pill)
Expand All @@ -42,6 +45,7 @@ import 'vue-awesome/icons/edit';

import CategoryEditModal from './CategoryEditModal.vue';
import { useCategoryStore } from '~/stores/categories';
import { splitRegexPipe } from '~/util/validate';

import _ from 'lodash';

Expand Down Expand Up @@ -100,6 +104,9 @@ export default {
hideEditModal: function () {
this.editingId = null;
},
splitRegex: function (regex: string): string[] {
return splitRegexPipe(regex);
},
},
};
</script>
Expand Down
83 changes: 83 additions & 0 deletions src/util/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,86 @@ export function isRegexBroad(re: string | RegExp) {
'THIS STRING SHOULD PROBABLY NOT MATCH: ' + alphabet + alphabet.toUpperCase() + numbers
);
}

/**
* Split a combined regex string into individual patterns by top-level `|`.
* Parenthesized groups like (…), (?:…), (?=…) are traversed with depth
* tracking so a pipe inside a group doesn't trigger a split.
*
* Users who need a literal | in a single rule can escape it with \|.
*/
export function splitRegexPipe(regex: string): string[] {
if (!regex) return [];
// Bail out of the smart split for pathological inputs — pipe-joined
// rule sets shouldn't realistically exceed 1 000 characters.
if (regex.length > 1000) {
return [regex];
}
const parts: string[] = [];
let depth = 0; // paren-nesting depth — skip | when inside a group
let escaping = false; // simple \-escape for \|
let start = 0;
for (let i = 0; i < regex.length; i++) {
const ch = regex[i];
if (escaping) {
escaping = false;
continue;
}
if (ch === '\\') {
escaping = true;
continue;
}
if (ch === '(') {
depth++;
continue;
}
if (ch === ')') {
if (depth > 0) depth--;
continue;
}
if (ch === '|' && depth === 0) {
parts.push(regex.slice(start, i));
start = i + 1;
}
}
Comment on lines +42 to +68

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.

P1 Character classes ([...]) are not tracked, so a pipe inside one (e.g. [a-z|0-9]) triggers a split at depth 0 and produces ["[a-z", "0-9]"] — two syntactically broken patterns. When a user re-opens a modal for a rule that contains such a pattern, they'll see false validation errors, and the intended regex is visually mangled (though the round-trip through joinRegexPipe happens to reconstruct the original string on save). Adding an inClass flag that mirrors the depth logic for ( / ) fixes this.

Suggested change
const parts: string[] = [];
let depth = 0; // paren-nesting depth — skip | when inside a group
let escaping = false; // simple \-escape for \|
let start = 0;
for (let i = 0; i < regex.length; i++) {
const ch = regex[i];
if (escaping) {
escaping = false;
continue;
}
if (ch === '\\') {
escaping = true;
continue;
}
if (ch === '(') {
depth++;
continue;
}
if (ch === ')') {
if (depth > 0) depth--;
continue;
}
if (ch === '|' && depth === 0) {
parts.push(regex.slice(start, i));
start = i + 1;
}
}
const parts: string[] = [];
let depth = 0; // paren-nesting depth — skip | when inside a group
let inClass = false; // inside a character class [...] — | is literal here
let escaping = false; // simple \-escape for \|
let start = 0;
for (let i = 0; i < regex.length; i++) {
const ch = regex[i];
if (escaping) {
escaping = false;
continue;
}
if (ch === '\\') {
escaping = true;
continue;
}
if (ch === '[' && !inClass) {
inClass = true;
continue;
}
if (ch === ']' && inClass) {
inClass = false;
continue;
}
if (inClass) continue; // any character inside [...] is literal
if (ch === '(') {
depth++;
continue;
}
if (ch === ')') {
if (depth > 0) depth--;
continue;
}
if (ch === '|' && depth === 0) {
parts.push(regex.slice(start, i));
start = i + 1;
}
}

parts.push(regex.slice(start));
return parts.filter(p => p.trim().length > 0);
}

/**
* Joins an array of regex patterns into a single pipe-separated string.
* Empty/whitespace-only entries are skipped.
*/
export function joinRegexPipe(patterns: string[]): string {
return patterns
.map(p => (p || '').trim())
.filter(p => p.length > 0)
.join('|');
}

/**
* Validates each pattern in a list and returns per-pattern results.
* `allValid` is true only when every pattern compiles successfully.
*/
export function validatePatternList(patterns: string[]): {
allValid: boolean;
results: { valid: boolean; broad: boolean }[];
} {
const results = patterns
.filter(p => (p || '').trim().length > 0)
.map(p => ({
valid: validateRegex(p.trim()),
broad: isRegexBroad(p.trim()),
}));
return {
allValid: results.every(r => r.valid),
results,
};
}

/**
* Returns true if *any* pattern in the list is overly broad.
*/
export function isPatternListBroad(patterns: string[]): boolean {
return patterns.some(p => isRegexBroad(p));
}
Comment on lines +88 to +109

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.

P2 validatePatternList and isPatternListBroad are exported but never called

Both functions were introduced in this PR but the modal uses inline logic (validateSinglePattern, validPattern computed, broad_pattern computed) instead. They are dead code as written. Either wire them into the component to consolidate the validation logic, or remove them to keep the surface clean.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!