Conversation
Co-authored-by: Jurie <smit.jurie@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
WalkthroughThis PR adds two new utility module namespaces to packages/utils: "formatting" and "validation". The formatting module provides utilities for date, number, and string formatting. The validation module offers URL validation functionality. Package exports and main index re-exports are updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (1)
packages/utils/src/formatting/string.ts (1)
73-85: Consider handling consecutive underscores.The function splits on
"_"but doesn't handle consecutive underscores, which would create empty components. For example,"foo__bar"would result in an empty string in the components array, potentially causing unexpected output.🔎 Suggested enhancement
export function snakeToCamel( value: string, capitalizeFirst: boolean = false ): string { - const components = value.split("_"); + const components = value.split("_").filter(c => c.length > 0); + if (components.length === 0) { + return ""; + } const first = capitalizeFirst ? components[0].charAt(0).toUpperCase() + components[0].slice(1) : components[0];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/utils/package.jsonpackages/utils/src/formatting/date.tspackages/utils/src/formatting/index.tspackages/utils/src/formatting/number.tspackages/utils/src/formatting/string.tspackages/utils/src/index.tspackages/utils/src/validation/index.tspackages/utils/src/validation/url.ts
🧰 Additional context used
🪛 ast-grep (0.40.3)
packages/utils/src/formatting/string.ts
[warning] 48-48: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${separator}+, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 49-49: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${separator}|${separator}$, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (13)
packages/utils/package.json (2)
17-25: LGTM! Clean export structure.The new exports for validation and formatting follow the existing pattern and integrate well with the package structure. The export paths correctly point to the compiled JavaScript and TypeScript declaration files.
58-60: LGTM! Keywords appropriately updated.The keywords have been correctly updated to reflect the new validation and formatting capabilities.
packages/utils/src/validation/url.ts (3)
5-8: LGTM! Clean interface design.The
UrlValidationResultinterface provides a clear contract with an optional error field for detailed feedback.
52-54: LGTM! Useful convenience function.The
isValidUrlfunction provides a clean boolean interface for simple validation checks.
17-43: Scheme comparison is already case-insensitive.The URL constructor automatically normalizes protocols to lowercase, regardless of input case. Testing confirms that
new URL('HTTP://example.com').protocolreturns'http:', making the case-sensitive comparison at line 29 effectively case-insensitive. The suggested.toLowerCase()normalization is unnecessary.Likely an incorrect or invalid review comment.
packages/utils/src/validation/index.ts (1)
1-5: LGTM! Clean module aggregation.The validation index properly re-exports URL utilities, providing a clear entry point for validation functionality.
packages/utils/src/index.ts (1)
8-12: LGTM! Public API cleanly expanded.The re-exports for validation and formatting follow the existing pattern and properly expose the new utility modules.
packages/utils/src/formatting/index.ts (1)
5-7: LGTM! Clean formatting module aggregation.The formatting index properly aggregates date, number, and string utilities into a cohesive module.
packages/utils/src/formatting/date.ts (1)
34-60: Note: Relative time uses approximate calculations.The month and year calculations use fixed approximations (30 days/month, 365 days/year) which may not match calendar expectations. For instance, "1 month ago" will be shown after exactly 30 days rather than calendar months. This is acceptable for display purposes but worth noting.
packages/utils/src/formatting/number.ts (3)
14-26: Consider locale handling approach.The function hardcodes
"en-US"locale then replaces separators, which works but may feel indirect. This approach ensures consistent decimal formatting before replacement, though users might expect the custom separators to be applied directly.
35-54: LGTM! Correct byte formatting implementation.The function properly handles binary vs. decimal units with appropriate base conversion and edge cases like zero bytes.
63-66: LGTM! Smart percentage handling.The auto-detection of fractional (0.0-1.0) vs. whole number (0-100) ranges is convenient, though callers should be aware of this behavior to avoid confusion.
packages/utils/src/formatting/string.ts (1)
59-64: LGTM! Clean camelCase to snake_case conversion.The function correctly handles uppercase letters and removes leading underscores.
| export function formatDateTime(date: Date, format: string = "default"): string { | ||
| if (format === "iso") { | ||
| return date.toISOString(); | ||
| } | ||
|
|
||
| const year = date.getFullYear(); | ||
| const month = String(date.getMonth() + 1).padStart(2, "0"); | ||
| const day = String(date.getDate()).padStart(2, "0"); | ||
| const hours = String(date.getHours()).padStart(2, "0"); | ||
| const minutes = String(date.getMinutes()).padStart(2, "0"); | ||
| const seconds = String(date.getSeconds()).padStart(2, "0"); | ||
|
|
||
| return `${year}-${month}-${day} ${hours}:${minutes}:${seconds}`; | ||
| } |
There was a problem hiding this comment.
Clarify format parameter documentation.
The JSDoc at line 9 mentions "custom format" but the implementation only supports "iso" and a hardcoded default format. This could mislead users expecting format string capabilities (like "YYYY-MM-DD" patterns).
🔎 Suggested documentation fix
/**
* Format datetime to string.
*
* @param date - Date object to format
- * @param format - Format string ('iso' for ISO 8601, or custom format)
+ * @param format - Format type ('iso' for ISO 8601, 'default' for 'YYYY-MM-DD HH:mm:ss')
* @returns Formatted datetime string
*/Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/utils/src/formatting/date.ts around lines 12 to 25, the JSDoc claims
a "custom format" but the function only supports "iso" and a hardcoded default;
update documentation to accurately describe supported values and behavior:
change the JSDoc to state that format accepts either "iso" (returns
date.toISOString()) or "default" (returns "YYYY-MM-DD HH:mm:ss") and that other
strings are ignored/treated as "default" (or alternatively validate and throw
for unknown formats). Also add a brief runtime guard to coerce/validate the
format parameter (e.g., if format !== "iso" && format !== "default" then set to
"default" or throw) and document that choice in the JSDoc.
| export function truncateString( | ||
| value: string, | ||
| maxLength: number, | ||
| suffix: string = "...", | ||
| preserveWords: boolean = true | ||
| ): string { | ||
| if (value.length <= maxLength) { | ||
| return value; | ||
| } | ||
|
|
||
| if (preserveWords) { | ||
| const truncated = value.substring(0, maxLength - suffix.length); | ||
| const lastSpace = truncated.lastIndexOf(" "); | ||
| if (lastSpace > maxLength * 0.5) { | ||
| return truncated.substring(0, lastSpace) + suffix; | ||
| } | ||
| return truncated + suffix; | ||
| } | ||
|
|
||
| return value.substring(0, maxLength - suffix.length) + suffix; | ||
| } |
There was a problem hiding this comment.
Fix word boundary heuristic logic.
Line 27 compares lastSpace (an absolute character index) with maxLength * 0.5 (a threshold). This logic seems incorrect—it should likely check if the found space is reasonably close to the desired truncation point. For example, if maxLength=100 and the last space is at index 60, it would be accepted (60 > 50), but if lastSpace=40 it would be rejected, which may not reflect the intended behavior.
🔎 Suggested fix
if (preserveWords) {
const truncated = value.substring(0, maxLength - suffix.length);
const lastSpace = truncated.lastIndexOf(" ");
- if (lastSpace > maxLength * 0.5) {
+ const minAcceptableLength = (maxLength - suffix.length) * 0.5;
+ if (lastSpace > minAcceptableLength) {
return truncated.substring(0, lastSpace) + suffix;
}
return truncated + suffix;🤖 Prompt for AI Agents
In packages/utils/src/formatting/string.ts around lines 14 to 34, the heuristic
compares lastSpace (an absolute index in the truncated substring) to maxLength *
0.5 which is incorrect; change the comparison to use the truncated substring
length (or the effective limit maxLength - suffix.length) so you check proximity
relative to the actual truncated portion. Specifically, compute truncated =
value.substring(0, maxLength - suffix.length), then only accept the lastSpace if
lastSpace > truncated.length * 0.5 (and lastSpace !== -1) and return
truncated.substring(0, lastSpace) + suffix; otherwise fall back to truncated +
suffix.
| export function slugify(value: string, separator: string = "-"): string { | ||
| return value | ||
| .toLowerCase() | ||
| .trim() | ||
| .replace(/[\s_]+/g, separator) | ||
| .replace(/[^\w\-]+/g, "") | ||
| .replace(new RegExp(`${separator}+`, "g"), separator) | ||
| .replace(new RegExp(`^${separator}|${separator}$`, "g"), ""); | ||
| } |
There was a problem hiding this comment.
Address ReDoS vulnerability in dynamic RegExp construction.
Lines 49-50 construct regular expressions from the separator variable without escaping special regex characters. If a caller passes a separator containing regex metacharacters (e.g., ".", "*", "+"), this could cause incorrect behavior or ReDoS attacks.
As per static analysis hints.
🔎 Recommended fix to escape separator
+function escapeRegExp(str: string): string {
+ return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+}
+
export function slugify(value: string, separator: string = "-"): string {
+ const escapedSeparator = escapeRegExp(separator);
return value
.toLowerCase()
.trim()
.replace(/[\s_]+/g, separator)
.replace(/[^\w\-]+/g, "")
- .replace(new RegExp(`${separator}+`, "g"), separator)
- .replace(new RegExp(`^${separator}|${separator}$`, "g"), "");
+ .replace(new RegExp(`${escapedSeparator}+`, "g"), separator)
+ .replace(new RegExp(`^${escapedSeparator}|${escapedSeparator}$`, "g"), "");
}🧰 Tools
🪛 ast-grep (0.40.3)
[warning] 48-48: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${separator}+, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 49-49: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^${separator}|${separator}$, "g")
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In packages/utils/src/formatting/string.ts around lines 43 to 51, the code
builds RegExp objects directly from the separator string which can contain regex
metacharacters and lead to incorrect behavior or ReDoS; fix by first escaping
all regex-special characters in the separator (e.g. replace characters like
-\/\\^$*+?.()|[]{} with a backslash-escaped version) and then use the escaped
separator when creating the RegExp instances (keep the existing "g" flag and
existing patterns but substitute the escaped separator), ensuring you don't
change other logic or return value.
1 participant
Summary
Migrated utility functions from orchestration to respective repositories:
Added new utility modules in
packages/utils:formatting: Date, number, and string formatting utilitiesvalidation: URL, email, and input validation utilitiescommon: Retry, rate limiting, and error handling utilitiesUpdated
package.jsonto include new export paths for:./validation./formattingSynchronized Python utility package with new TypeScript utilities
codeflow-utils-pythonRelated Pull Requests
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.