fix: resolve remaining PR 496 review issues#501
Closed
jeffscottward wants to merge 1 commit intoRunMaestro:mainfrom
Closed
fix: resolve remaining PR 496 review issues#501jeffscottward wants to merge 1 commit intoRunMaestro:mainfrom
jeffscottward wants to merge 1 commit intoRunMaestro:mainfrom
Conversation
📝 WalkthroughWalkthroughEnhanced numeric value detection in a CSV table renderer component to support additional numeric formats including negative signs, currency symbols, thousands separators, and parenthesized negatives. Fixed key generation in match highlighting to ensure uniqueness by incorporating both offset and index values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/components/CsvTableRenderer.tsx`:
- Around line 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.
Comment on lines
+85
to
+87
| return /^(?:-?\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?%?|\(\$?(?:\d+|\d{1,3}(?:,\d{3})+)(?:\.\d+)?\)%?)$/.test( | ||
| trimmed | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/renderer/components/CsvTableRenderer.tsx | head -200Repository: 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 -ARepository: 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 -ARepository: 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.
Greptile SummaryThis PR contains two small but correct fixes to
One minor observation: the running comment still claims the Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["isNumericValue(value)"] --> B{trimmed === ''}
B -- Yes --> C[return false]
B -- No --> D{Match new regex}
D -- "Positive form\n-?$? digit-block .decimal? %?" --> E["e.g. 1234, $1,234.56, -1,234%"]
D -- "Accounting negative\n($? digit-block .decimal?) %?" --> F["e.g. (1,234.56), ($500)"]
D -- No match --> G[return false]
E --> H[return true]
F --> H
subgraph "digit-block alternatives"
I["\\d+ (plain digits, no commas)"]
J["\\d{1,3}(?:,\\d{3})+ (proper comma grouping)"]
end
D -.uses.-> I
D -.uses.-> J
Last reviewed commit: c20ed24 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
151
to
+155
| // 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}`; |
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Note: task list file was created locally for tracking and not committed.
Summary by CodeRabbit
Bug Fixes
Improvements