Skip to content

fix: resolve remaining PR 496 review issues #501

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jeffscottward wants to merge 1 commit into from
Closed

fix: resolve remaining PR 496 review issues #501

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 5 additions & 3 deletions src/renderer/components/CsvTableRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,9 @@ function isNumericValue(value: string): boolean {
const trimmed = value.trim();
if (trimmed === '') return false;
// Match: optional currency/sign prefix, digits with optional commas, optional decimal, optional suffix
return /^[($\-]*[\d,]+(\.\d+)?[%)]*$/.test(trimmed);
return /^(?:-?\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?%?|\(\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?\)%?)$/.test(
trimmed
);
Comment on lines +85 to +87
Copy link

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/renderer/components/CsvTableRenderer.tsx | head -200

Repository: RunMaestro/Maestro

Length of output: 6628

🏁 Script executed:

# Verify the logic issue with a quick trace through compareValues
cat -n src/renderer/components/CsvTableRenderer.tsx | sed -n '120,140p'

Repository: RunMaestro/Maestro

Length of output: 859

🏁 Script executed:

# Double-check the indentation style to ensure it matches the guidelines
head -20 src/renderer/components/CsvTableRenderer.tsx | cat -A

Repository: RunMaestro/Maestro

Length of output: 545

🏁 Script executed:

# Verify the exact regex pattern at lines 85-87 matches parenthesized values
sed -n '85,87p' src/renderer/components/CsvTableRenderer.tsx | cat -A

Repository: RunMaestro/Maestro

Length of output: 194

Parenthesized negatives are detected but sorted with the wrong sign

Line 85 now classifies values like (123) and ($45.67) as numeric, but compareValues (lines 130–131) strips () without converting to negative, so these values sort as positive numbers.

Proposed fix 
 function isNumericValue(value: string): boolean {
 	const trimmed = value.trim();
 	if (trimmed === '') return false;
 	// Match: optional currency/sign prefix, digits with optional commas, optional decimal, optional suffix
 	return /^(?:-?\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?%?|\(\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?\)%?)$/.test(
 		trimmed
 	);
 }
+
+function parseNumericValue(value: string): number | null {
+	const trimmed = value.trim();
+	if (!isNumericValue(trimmed)) return null;
+
+	const isParenNegative = /^\(.*\)$/.test(trimmed);
+	const normalized = trimmed.replace(/[,$%()]/g, '');
+	const num = Number(normalized);
+	if (Number.isNaN(num)) return null;
+
+	return isParenNegative ? -num : num;
+}
 
 function compareValues(a: string, b: string, direction: SortDirection): number {
 	const aVal = a.trim();
 	const bVal = b.trim();
@@
-	const aNum = parseFloat(aVal.replace(/[,$%()]/g, ''));
-	const bNum = parseFloat(bVal.replace(/[,$%()]/g, ''));
-
-	if (!isNaN(aNum) && !isNaN(bNum)) {
+	const aNum = parseNumericValue(aVal);
+	const bNum = parseNumericValue(bVal);
+	if (aNum !== null && bNum !== null) {
 		return direction === 'asc' ? aNum - bNum : bNum - aNum;
 	}
📝 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.

Suggested change
return /^(?:-?\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?%?|\(\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?\)%?)$/.test(
trimmed
);
function isNumericValue(value: string): boolean {
const trimmed = value.trim();
if (trimmed === '') return false;
// Match: optional currency/sign prefix, digits with optional commas, optional decimal, optional suffix
return /^(?:-?\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?%?|\(\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?\)%?)$/.test(
trimmed
);
}
function parseNumericValue(value: string): number | null {
const trimmed = value.trim();
if (!isNumericValue(trimmed)) return null;
const isParenNegative = /^\(.*\)$/.test(trimmed);
const normalized = trimmed.replace(/[,$%()]/g, '');
const num = Number(normalized);
if (Number.isNaN(num)) return null;
return isParenNegative ? -num : num;
}
function compareValues(a: string, b: string, direction: SortDirection): number {
const aVal = a.trim();
const bVal = b.trim();
const aNum = parseNumericValue(aVal);
const bNum = parseNumericValue(bVal);
if (aNum !== null && bNum !== null) {
return direction === 'asc' ? aNum - bNum : bNum - aNum;
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/renderer/components/CsvTableRenderer.tsx` around lines 85 - 87, The
isNumeric regex correctly identifies parenthesized negatives like "(123)" and
"($45.67)" but compareValues currently just strips "()" instead of converting
them to negative numbers; update the numeric normalization in compareValues (or
the shared parsing helper used by compareValues) to detect values that start
with "(" and end with ")" and, after removing surrounding parentheses, prepend a
"-" before removing currency symbols/commas and parsing so "(123)" and
"($45.67)" parse as -123 and -45.67 respectively; keep existing handling for $ ,
and % and ensure percent and negative signs are handled consistently.

}

/**
Expand Down Expand Up @@ -149,8 +151,8 @@ function highlightMatches(text: string, query: string, accentColor: string): Rea
// Use running character offset as key to guarantee uniqueness across
// identical substrings appearing at different positions.
let offset = 0;
return parts.map((part) => {
const key = offset;
return parts.map((part, index) => {
const key = `${offset}-${index}`;
Comment on lines 151 to +155
Copy link

@greptile-apps greptile-apps bot Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale comment + overly complex key

The comment at line 151–152 still says the offset alone "guarantee[s] uniqueness", but the whole reason this bug existed is that offset was not unique — the empty string preceding the first match and the match itself both had offset = 0, causing the duplicate key.

Since .map((part, index) => …) already gives you a unique index per slot, you can simplify the key to just index and update the comment accordingly:

Suggested change
// Use running character offset as key to guarantee uniqueness across
// identical substrings appearing at different positions.
let offset = 0;
return parts.map((part) => {
const key = offset;
return parts.map((part, index) => {
const key = `${offset}-${index}`;
// index is unique within this map call, so use it directly as the React key.
return parts.map((part, index) => {
const key = index;
offset += part.length;

The compound ${offset}-${index} is not incorrect, but it implies the offset contributes uniqueness when it doesn't — only index does.

offset += part.length;
return regex.test(part) ? (
<mark
Expand Down