Conversation
Create 11 new shared utility modules under src/utils/ that centralize duplicated functions from across the codebase: format, string, array, async, tokens, date, json, object, verbose, claude, and markdown. Expand fuzzy-match with levenshtein/similarity. Replace colors.ts function bodies with re-exports from format.ts.
Replace local implementations with imports from shared utils: - formatDuration (4 styles) in history, timely, ask/cli - formatBytes in files-to-prompt, ask/cli - formatRelativeTime in last-changes, azure-devops/utils - levenshtein/similarity in azure-devops/utils - htmlToMarkdown, slugify in azure-devops/utils - removeDiacritics in azure-devops/history (replaces DIACRITICS_MAP) - wrapArray in mcp-ripgrep, mcp-tsc - stripAnsi, escapeShellArg in mcp-ripgrep - Generic utils (retry, debounce, etc.) in ask/helpers Net: -411 lines across 12 files via re-exports and thin wrappers.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughCentralizes many local utilities into new shared modules under Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Precheck as PrecheckService
participant AD as AzureDevOpsAPI
participant Storage as LocalStorage
CLI->>Precheck: run precheck(entries)
Precheck->>AD: fetch workitem metadata (id or redirect)
AD-->>Precheck: return title, type, redirectedId?
Precheck->>Precheck: store precheckRedirected entries (original/redirected title/type)
Precheck->>CLI: return precheck results
CLI->>Storage: persist entries with _workitemTitle when available
CLI->>CLI: display dry-run/created logs including "#id Title" when titles exist
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello @genesiscz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the codebase by consolidating numerous duplicated utility functions into a new set of shared modules. The primary goal is to adhere to the DRY (Don't Repeat Yourself) principle, enhancing code maintainability, consistency, and reducing the overall footprint of redundant logic. This change streamlines development by providing a single source of truth for common operations, making future enhancements and bug fixes more efficient. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a large-scale refactoring that centralizes many duplicated utility functions into new shared modules under src/utils/. This is an excellent change that significantly improves code maintainability and consistency by reducing code duplication (DRY principle). The new utility modules for formatting, strings, async operations, and more are well-structured. The migration of existing files to use these new shared utilities appears to be handled carefully, with attention to preserving existing functionality through different formatting styles. I found one minor discrepancy in the output of a refactored function, which I've commented on. Overall, this is a high-quality refactoring that will benefit the codebase long-term.
| function formatDuration(minutes: number): string { | ||
| const hours = Math.floor(minutes / 60); | ||
| const mins = minutes % 60; | ||
| if (hours > 0) { | ||
| return `${hours}h ${mins}m`; | ||
| } | ||
| return `${mins}m`; | ||
| return _formatDuration(minutes, "min", "hm-smart"); | ||
| } |
There was a problem hiding this comment.
The refactoring to use the shared formatDuration with the hm-smart style introduces a subtle change in the output format compared to the original implementation.
The previous implementation would format a duration of exactly N hours (e.g., 120 minutes) as Nh 0m. The new hm-smart style will format this as Nh.
For example:
- Old:
formatDuration(120)->'2h 0m' - New:
_formatDuration(120, 'min', 'hm-smart')->'2h'
While this might be a desired improvement, it contradicts the goal of preserving the exact per-caller output mentioned in the pull request description. If the original format is important, you might consider adjusting the hm-smart style or using a different style.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp-ripgrep/index.ts (1)
18-22:⚠️ Potential issue | 🟠 MajorDebug log left in
processOutput;useColorsparameter is unused.Line 20 logs
"searchRoot"to stderr on every call—this looks like a leftover debug statement. Additionally, theuseColorsparameter is never read inside the function body, making it dead code. Callers passfalseoruseColorsbut the value is ignored.Proposed fix
-function processOutput(output: string, useColors: boolean): string { +function processOutput(output: string): string { if (!output) return output; - console.error("searchRoot", searchRoot); return searchRoot ? output.replaceAll(searchRoot, ".") : output; }If
useColorswas meant to gate ANSI stripping, the body should callstripAnsiwhen colors are unwanted. Otherwise, drop the parameter and update all call sites.
🤖 Fix all issues with AI agents
In `@src/utils/async.ts`:
- Around line 119-130: withTimeout currently creates a setTimeout that is never
cleared when the input promise settles, keeping the event loop alive; modify
withTimeout to store the timer handle returned by setTimeout and clear it
(clearTimeout) as soon as either the input promise or the timeout wins (i.e., on
settlement of the returned race), or restructure to use an AbortController tied
to the timer and abort/cleanup on settlement; update the function (withTimeout)
so the timer is cancelled in all resolution/rejection paths to prevent the timer
leak.
- Around line 82-91: The generic parameter constraint for debounce (function
debounce) and throttle (function throttle) is too narrow—using T extends
(...args: unknown[]) => void causes contravariance issues for callers; change
the constraint to use any (e.g., T extends (...args: any[]) => any or T extends
(...args: any[]) => void) so functions with concrete parameter types are
assignable, update the timeout/return types accordingly (keep ReturnType<typeof
setTimeout> for timeout) and ensure the returned function signature remains
(...args: Parameters<T>) => ReturnType<T> or void to preserve typing for
callers.
In `@src/utils/claude/index.ts`:
- Around line 30-34: Before calling createReadStream(filePath) in this module,
add an existence/access check for filePath (e.g., using fs.existsSync or
fs.promises.access) and handle the missing file case by throwing a clear error
or returning early; update the logic around fileStream and the readline
interface (rl / createInterface) to only create them after confirming the file
is present to avoid uncaught ENOENT from createReadStream.
In `@src/utils/date.ts`:
- Around line 28-34: The loop mutates local-time fields while parsing and
outputting UTC, causing off-by-one dates; fix by treating dates in UTC only:
parse since/upto into UTC-based Dates (or construct via Date.UTC using the
year/month/day parsed from the input strings), then when advancing use UTC
methods (e.g., current.setUTCDate(current.getUTCDate() + 1) or increment the
timestamp by 86400000 ms) and keep using toISOString().split("T")[0] for output;
update the variables referenced (since, upto, start, end, current, dates)
accordingly so all operations are performed in UTC.
In `@src/utils/format.ts`:
- Around line 23-76: The switch in formatDuration (function formatDuration, type
DurationStyle) is not exhaustive and can fall through returning undefined; add a
default/fallback that assigns the unhandled style to a never-typed variable to
force compile-time exhaustiveness (e.g., const _exhaustiveCheck: never = style)
and then return a safe fallback string (like "0ms" or an error string) so the
function always returns a string at runtime.
In `@src/utils/string.ts`:
- Around line 53-59: sanitizeOutput currently uses a limited regex that only
strips SGR sequences; update it to reuse the existing stripAnsi utility
(stripAnsi) instead of the narrower /\x1b\[[0-9;]*m/g so all CSI/ANSI escape
sequences are removed when removeANSI is true. Locate sanitizeOutput and replace
the removeANSI branch to call or invoke stripAnsi(sanitized) (keeping the
subsequent control-character removal) so callers get full ANSI stripping; ensure
you reference the existing stripAnsi symbol and preserve the sanitized variable
flow and return value.
- Around line 45-48: truncateText currently appends "..." even when maxLength ≤
3 resulting in output longer than maxLength; update truncateText to handle small
limits by first clamping maxLength to >=0 and if maxLength <= 3 return
text.substring(0, maxLength) (no ellipsis), otherwise keep existing behavior of
returning text.substring(0, maxLength - 3) + "..." so the final string never
exceeds maxLength; reference the truncateText(text: string, maxLength: number =
100) function and adjust its conditional logic accordingly.
🧹 Nitpick comments (7)
src/utils/json.ts (1)
8-14: Return type doesn't reflect fallback guarantee.When a
fallbackis provided, the return can never benull, yet the signature always includes| null. Consider overloads to give callers tighter types:Proposed overloads
-export function parseJSON<T>(text: string, fallback?: T): T | null { +export function parseJSON<T>(text: string, fallback: T): T; +export function parseJSON<T>(text: string): T | null; +export function parseJSON<T>(text: string, fallback?: T): T | null {src/utils/claude/index.ts (1)
17-19:getClaudeProjectsDir()is a trivial getter over a constant.Since
PROJECTS_DIRis already exported, the function adds no value unless it's intended to allow future dynamic resolution. Consider whether both exports are needed.src/utils/async.ts (1)
41-73: Consider replacing the explicitnew Promiseconstructor withasync/await+ sleep.The manual
resolve/rejectwiring (explicit construction anti-pattern) makes the retry logic harder to follow. Anasyncloop with asleephelper is equivalent and more readable:Suggested alternative
- return new Promise((resolve, reject) => { - let attempt = 0; - - const tryOperation = async () => { - try { - const result = await operation(); - resolve(result); - } catch (error) { - attempt++; - if (attempt >= maxAttempts || (shouldRetry && !shouldRetry(error))) { - reject(error); - return; - } - - let nextDelay: number; - switch (backoff) { - case "linear": - nextDelay = delay * attempt; - break; - case "fixed": - nextDelay = delay; - break; - default: // exponential - nextDelay = delay * 2 ** (attempt - 1); - } - - onRetry?.(attempt, nextDelay); - setTimeout(tryOperation, nextDelay); - } - }; - - tryOperation(); - }); + const sleep = (ms: number) => new Promise<void>((r) => setTimeout(r, ms)); + for (let attempt = 0; attempt < maxAttempts; attempt++) { + try { + return await operation(); + } catch (error) { + if (attempt + 1 >= maxAttempts || (shouldRetry && !shouldRetry(error))) { + throw error; + } + let nextDelay: number; + switch (backoff) { + case "linear": + nextDelay = delay * (attempt + 1); + break; + case "fixed": + nextDelay = delay; + break; + default: + nextDelay = delay * 2 ** attempt; + } + onRetry?.(attempt + 1, nextDelay); + await sleep(nextDelay); + } + } + throw new Error("Retry exhausted"); // unreachablesrc/ask/utils/cli.ts (1)
4-4: Inconsistent import path style: relative vs. alias.Other migrated files in this PR use the
@app/utils/formatpath alias (e.g.,history-search.ts,history.ts,entry-processor.ts,last-changes/index.ts), but here both the import (line 4) and re-export (line 329) use a relative path../../utils/format. Consider aligning with the alias for consistency.♻️ Suggested change
-import { formatDuration as _formatDuration } from "../../utils/format"; +import { formatDuration as _formatDuration } from "@app/utils/format";-export { formatBytes } from "../../utils/format"; +export { formatBytes } from "@app/utils/format";Also applies to: 329-329
src/utils/object.ts (1)
16-29:for...initerates inherited enumerable properties.If
sourceever has prototype pollution or inherits enumerable keys, they'll be merged into the result. Consider guarding withObject.hasOwn:Suggested hardening
for (const key in source) { + if (!Object.hasOwn(source, key)) continue; if (source[key] && isObject(source[key]) && isObject(result[key])) {Low risk given the
Partial<T>typing, but a good defensive habit for a shared utility.src/utils/tokens.ts (1)
19-26: Consider usingimport()instead ofrequire()for the optional dependency.
require()works at runtime in Bun but is a CJS pattern. A dynamicawait import("gpt-3-encoder")would be more idiomatic in an ESM/TypeScript codebase. This would require makingcountTokensasync (or caching the import at module level), so it's a trade-off.Not blocking—the try/catch fallback keeps it safe either way.
src/utils/string.ts (1)
34-40: Doc comment overstates coverage:removeDiacriticsonly strips the basic Combining Diacritical Marks block.The JSDoc says "Handles all Unicode combining marks", but the regex
[\u0300-\u036f]only covers the basic block (U+0300–U+036F). Extended combining marks (e.g., U+1AB0–U+1AFF, U+20D0–U+20FF) are not removed. This is fine for Latin-script use cases but the comment is misleading.
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicated helper logic across the CLI tools into shared src/utils/* modules and updates multiple consumers to import/re-export those shared utilities to reduce duplication and improve maintainability.
Changes:
- Added a set of shared utility modules under
src/utils/(formatting, strings, async, tokens, JSON, objects, dates, markdown, verbose, Claude helpers). - Migrated multiple tool implementations to use shared imports and added re-exports/thin wrappers for compatibility.
- Expanded fuzzy matching utilities with Levenshtein distance and similarity scoring.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/verbose.ts | Adds a scoped verbose logger factory. |
| src/utils/tokens.ts | Adds shared token estimation/counting/truncation helpers. |
| src/utils/string.ts | Adds consolidated string helpers (slugify, ANSI stripping, shell escaping, etc.). |
| src/utils/prompts/colors.ts | Removes local formatting helpers and re-exports shared format utilities. |
| src/utils/object.ts | Adds isObject and deepMerge utilities. |
| src/utils/markdown/html-to-md.ts | Centralizes HTML→Markdown conversion via Turndown + GFM. |
| src/utils/json.ts | Adds safe JSON parsing helper. |
| src/utils/fuzzy-match.ts | Extends fuzzy matching with Levenshtein + similarity score. |
| src/utils/format.ts | Adds shared formatting utilities (duration/bytes/cost/tokens/relative time/list/number). |
| src/utils/date.ts | Adds shared month/date-range utilities. |
| src/utils/claude/index.ts | Adds shared Claude projects path + JSONL transcript parsing. |
| src/utils/async.ts | Adds shared retry/debounce/throttle/timeout helpers. |
| src/utils/array.ts | Adds shared wrapArray. |
| src/timely/utils/entry-processor.ts | Switches duration formatting to shared formatDuration. |
| src/timely/utils/date.ts | Re-exports shared date helpers and maps duration formatting to shared formatter. |
| src/mcp-tsc/utils/helpers.ts | Re-exports shared wrapArray. |
| src/mcp-ripgrep/index.ts | Replaces local string/array helpers with shared implementations. |
| src/last-changes/index.ts | Switches relative time formatting to shared formatRelativeTime. |
| src/files-to-prompt/index.ts | Switches file-size formatting to shared formatBytes. |
| src/azure-devops/utils.ts | Replaces local markdown/string/fuzzy utilities with shared modules + re-exports. |
| src/azure-devops/history.ts | Switches diacritics normalization to shared removeDiacritics. |
| src/azure-devops/commands/history.ts | Switches duration formatting to shared formatDuration. |
| src/azure-devops/commands/history-search.ts | Switches duration formatting to shared formatDuration. |
| src/ask/utils/helpers.ts | Re-exports shared helpers instead of maintaining local copies. |
| src/ask/utils/cli.ts | Switches elapsed-time formatting to shared formatDuration and re-exports shared formatBytes. |
Comments suppressed due to low confidence (1)
src/mcp-ripgrep/index.ts:270
wrapArrayno longer drops empty-string values (it only returns[]for null/undefined). Ifargs.filePatternis"", this will produce['']and the loop will add-g ''to the ripgrep command, which can error or match unexpectedly. Consider filtering out empty/whitespace-only patterns before building the command.
const pattern = String(args.pattern || "");
const path = String(args.path);
const caseSensitive = typeof args.caseSensitive === "boolean" ? args.caseSensitive : undefined;
const filePattern = wrapArray(args.filePattern as string | string[] | undefined);
const maxResults = typeof args.maxResults === "number" ? args.maxResults : undefined;
const context = typeof args.context === "number" ? args.context : undefined;
const useColors = typeof args.useColors === "boolean" ? args.useColors : false;
if (!pattern) {
return {
isError: true,
content: [{ type: "text", text: "Error: Pattern is required" }],
};
}
// Resolve the search path relative to the configured root
const fullPath = nodePath.resolve(searchRoot, path);
// Build the rg command with flags
let command = "rg";
// Add case sensitivity flag if specified
if (caseSensitive === true) {
command += " -s"; // Case sensitive
} else if (caseSensitive === false) {
command += " -i"; // Case insensitive
}
// Add file pattern if specified
if (filePattern && filePattern.length > 0) {
for (const pattern of filePattern) {
command += ` -g ${escapeShellArg(pattern)}`;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/utils/async.ts
Outdated
| let timeout: ReturnType<typeof setTimeout>; | ||
| return (...args: Parameters<T>) => { | ||
| clearTimeout(timeout); |
There was a problem hiding this comment.
debounce declares let timeout: ReturnType<typeof setTimeout>; and calls clearTimeout(timeout) before timeout is assigned. Under strict TS this is flagged as “used before being assigned” (and at runtime the first call clears an uninitialized handle). Initialize the variable (e.g., to undefined) and guard the clearTimeout call.
| let timeout: ReturnType<typeof setTimeout>; | |
| return (...args: Parameters<T>) => { | |
| clearTimeout(timeout); | |
| let timeout: ReturnType<typeof setTimeout> | undefined; | |
| return (...args: Parameters<T>) => { | |
| if (timeout !== undefined) { | |
| clearTimeout(timeout); | |
| } |
| export function withTimeout<T>( | ||
| promise: Promise<T>, | ||
| timeoutMs: number, | ||
| timeoutError?: Error, | ||
| ): Promise<T> { | ||
| const timeoutPromise = new Promise<never>((_, reject) => { | ||
| setTimeout(() => { | ||
| reject(timeoutError || new Error(`Operation timed out after ${timeoutMs}ms`)); | ||
| }, timeoutMs); | ||
| }); | ||
| return Promise.race([promise, timeoutPromise]); | ||
| } |
There was a problem hiding this comment.
withTimeout starts a timer but never clears it when the wrapped promise settles. In CLIs this can keep the event loop alive unnecessarily and create stray timers. Store the timeout handle and clearTimeout it in a finally/settlement path.
| case "hm-smart": { | ||
| if (totalMinutes < 1) return "< 1m"; | ||
| const hours = Math.floor(totalMinutes / 60); | ||
| const mins = Math.round(totalMinutes % 60); | ||
| if (hours === 0) return `${mins}m`; | ||
| if (mins === 0) return `${hours}h`; | ||
| return `${hours}h ${mins}m`; | ||
| } |
There was a problem hiding this comment.
In hm-smart, rounding minutes can produce mins === 60 (e.g., 59.6m) which then formats as 60m or Xh 60m. Consider normalizing the carry (increment hours and set mins to 0) to avoid impossible minute values in the output.
src/utils/tokens.ts
Outdated
| ): { text: string; tokens: number; truncated: boolean } { | ||
| const tokens = countTokens(text); | ||
|
|
||
| if (!maxTokens || tokens <= maxTokens) { |
There was a problem hiding this comment.
if (!maxTokens || tokens <= maxTokens) treats maxTokens = 0 as “no limit” because 0 is falsy. If callers can pass 0, this will skip truncation unexpectedly. Prefer a nullish check (maxTokens == null) instead of a truthiness check.
| if (!maxTokens || tokens <= maxTokens) { | |
| if (maxTokens == null || tokens <= maxTokens) { |
src/utils/async.ts
Outdated
| export function throttle<T extends (...args: unknown[]) => void>( | ||
| func: T, | ||
| limit: number, | ||
| ): (...args: Parameters<T>) => void { | ||
| let inThrottle: boolean; | ||
| return (...args: Parameters<T>) => { | ||
| if (!inThrottle) { | ||
| func(...args); | ||
| inThrottle = true; | ||
| setTimeout(() => (inThrottle = false), limit); | ||
| } | ||
| }; |
There was a problem hiding this comment.
throttle declares let inThrottle: boolean; and reads it before assignment (if (!inThrottle)). This is a TS “used before being assigned” issue under strict and should be initialized (e.g., let inThrottle = false).
src/timely/utils/entry-processor.ts
Outdated
| return `${hours}h ${mins}m`; | ||
| } | ||
| return `${mins}m`; | ||
| return _formatDuration(minutes, "min", "hm-smart"); |
There was a problem hiding this comment.
This refactor changes output for whole-hour values: the previous implementation returned "2h 0m", but hm-smart returns "2h" when minutes are 0. That’s a behavior change and also contradicts the function’s doc comment ("Xh Ym"). Either keep the old formatting behavior here or add/use a duration style that preserves the old output.
| return _formatDuration(minutes, "min", "hm-smart"); | |
| return _formatDuration(minutes, "min", "hm"); |
src/utils/tokens.ts
Outdated
| export function countTokens(text: string): number { | ||
| try { | ||
| const { encode } = require("gpt-3-encoder"); | ||
| return encode(text).length; | ||
| } catch { | ||
| return Math.ceil(text.split(/\s+/).length * 1.3); |
There was a problem hiding this comment.
countTokens/limitToTokens use require("gpt-3-encoder"), but this repo is ESM (package.json#type: module, tsconfig#module: ESNext). In Node ESM there is no global require, so this will throw at runtime even when the package exists. Use an ESM-safe loader (e.g., createRequire(import.meta.url)) or switch to an import-based approach while keeping the lazy-load behavior.
src/utils/tokens.ts
Outdated
| const { encode } = require("gpt-3-encoder"); | ||
| return encode(text).length; | ||
| } catch { | ||
| return Math.ceil(text.split(/\s+/).length * 1.3); |
There was a problem hiding this comment.
The fallback heuristic Math.ceil(text.split(/\s+/).length * 1.3) counts an empty string as 1 “word” ("".split(...) => [""]), producing non-zero tokens for empty/whitespace-only input. Consider using text.trim() and returning 0 when the trimmed string is empty.
| return Math.ceil(text.split(/\s+/).length * 1.3); | |
| const trimmed = text.trim(); | |
| if (!trimmed) { | |
| return 0; | |
| } | |
| return Math.ceil(trimmed.split(/\s+/).length * 1.3); |
…--to and @me to timelog list - import/prepare-import: Display workitem titles in precheck redirects, dry-run entries, and real import success output - prepare-import: Store _workitemTitle in entries for list/totals display - timelog list: Add --from/--to as primary date flags (--since/--upto hidden) - timelog list: Resolve --user @me to configured defaultUser - skills: Update command references and document new features
…providers installServer() result was ignored — user rejecting the diff still marked the server as enabled. Now properly skips on rejection. Added feedback logging for WriteResult.NoChanges so providers like Codex and Cursor (where presence = enabled) show a message instead of producing zero output.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/azure-devops/commands/timelog/list.ts (1)
78-81:⚠️ Potential issue | 🟡 MinorStale error message still references
--sinceinstead of--from.Line 79 was not updated to match the new primary option names introduced in this PR.
Proposed fix
if (!fromDate) { - console.error("--since or --day is required for date queries"); + console.error("--from or --day is required for date queries"); process.exit(1); }
🤖 Fix all issues with AI agents
In `@plugins/genesis-tools/skills/azure-devops/SKILL.md`:
- Line 384: Update the wording in the sentence that currently reads "The `--user
`@me`` resolves to the configured default user name. Use `--from`/`--to` for date
ranges (`--since`/`--upto` also accepted as aliases)." to use the compound
"username" instead of "user name" (so it reads "...configured default
username."); search for the exact text containing `--user `@me`` in SKILL.md to
locate the sentence and replace only the phrase "user name" with "username" to
keep the rest of the wording and flags unchanged.
In `@src/azure-devops/commands/timelog/list.ts`:
- Around line 94-101: The code silently leaves "@me" as a literal when
config.timelog?.defaultUser?.userName is missing, causing empty results; update
the user resolution logic in the block that builds resolvedUsers (using
options.user and defaultUserName) to detect any "@me" that could not be resolved
and throw or return a clear error message instead of keeping the literal.
Specifically, after mapping to resolvedUsers (or during mapping), if any entry
equals "@me" and defaultUserName is falsy, raise a user-facing error (or exit)
explaining that "@me" cannot be resolved and instruct to configure
timelog.defaultUser.userName or pass an explicit user; keep the rest of the
filtering logic (userFilters and entries filter) unchanged.
In `@src/azure-devops/commands/timelog/prepare-import.ts`:
- Around line 154-161: The code uses workitemTitle derived from precheck.status
and precheck.redirectTitle, but when precheck.status === "redirect" and
redirectTitle is undefined while redirectId exists you silently store the
original title; update the logic that sets workitemTitle (and thus
storedEntry._workitemTitle) to detect this case and produce an explicit value
indicating a redirect (e.g., prefer redirectTitle, else if redirectId exists set
a composed title like "Redirected -> <redirectId>" or append a note to
originalTitle) so the stored entry clearly reflects that the item was redirected
even when redirectTitle is missing; adjust the assignment around workitemTitle
and storedEntry._workitemTitle accordingly.
🧹 Nitpick comments (3)
src/azure-devops/commands/timelog/import.ts (1)
231-233: Minor inconsistency in label formatting between dry-run and creation output.Dry-run (line 233) formats as
#${id}${titlePart}:(with a colon after the label), while creation (line 257–258) formats as#${id} ${title}without a colon, joined by" | ". This subtle inconsistency in the output style between the two code paths may confuse users comparing dry-run output against actual creation output.Consider aligning the two paths — e.g., using
wiLabel+ pipe-joined parts consistently in both places.src/azure-devops/commands/timelog/list.ts (2)
38-40:--daysilently overrides--from/--to— consider a conflict guard.If a user passes both
--day 2026-01-15and--from 2026-01-01 --to 2026-01-31,--daysilently wins for both bounds, discarding the explicit range. A brief validation emitting a warning (or error) when--dayis combined with--from/--towould prevent silent surprises.
90-92: Dead code —elsebranch is unreachable.The first branch covers
hasWorkItem && !hasDateFilter, the second covershasDateFilter || !hasWorkItem. Together they exhaust all four boolean combinations of(hasWorkItem, hasDateFilter), so thiselseis never entered.Proposed removal
entries = raw; - } else { - entries = []; }
…Gemini - async: widen debounce/throttle generic constraint to (...args: any[]) - async: initialize debounce timeout and throttle flag properly - async: clear timer in withTimeout to prevent event loop leak - format: add exhaustive default case to formatDuration switch - format: handle hm-smart rounding edge case (59.6min → 60m carry) - date: use UTC methods in getDatesInMonth to fix timezone mismatch - string: handle truncateText edge case when maxLength ≤ 3 - string: reuse stripAnsi in sanitizeOutput instead of narrower regex - claude: add existsSync guard before createReadStream in parseJsonlTranscript - tokens: use nullish check for maxTokens to handle 0 correctly - tokens: handle empty string in countTokens fallback heuristic - entry-processor: use hm-always style to preserve original "Xh Ym" output
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/utils/format.ts`:
- Around line 101-120: formatRelativeTime currently produces negative "X days
ago" for future dates because diffMs can be negative; add a guard near the top
of formatRelativeTime that checks if diffMs < 0 and handle future timestamps
(e.g., return "just now" for very near-future or compute absolute
diffMinutes/diffHours/diffDays and return a future-formatted string like "in X
minutes/hours/days") instead of falling through to the existing past logic;
update uses of diffMs/diffMinutes/diffHours/diffDays (and respect
rounding/fallbackFormat) so future cases are symmetric and do not produce
negative values.
In `@src/utils/string.ts`:
- Around line 11-18: The slugify function currently preserves original casing;
update slugify(title: string) to produce lowercase slugs by applying
.toLowerCase() to the normalized/replaced string (e.g., add .toLowerCase() in
the method chain before trimming/slicing) so outputs become all-lowercase and
consistent with the other local slugify implementation; ensure the inserted
.toLowerCase() is applied after diacritics removal and non-alphanumeric
replacement but before final .replace(/^-+|-+$/g, "") and .slice(0, 50) to keep
behavior unchanged except for casing.
🧹 Nitpick comments (4)
src/utils/async.ts (1)
41-73:retryuses the explicit-construction anti-pattern around an innerasyncfunction.Wrapping an
asyncfunction insidenew Promise(...)is a well-known anti-pattern. The innerasyncalready returns a promise, and if an unexpected throw occurs outside thetry/catch(e.g., a bug in the scheduling logic), the rejection would be unhandled rather than propagated to the caller. A recursive-async approach is simpler and avoids this risk.♻️ Suggested recursive-async rewrite
- return new Promise((resolve, reject) => { - let attempt = 0; - - const tryOperation = async () => { - try { - const result = await operation(); - resolve(result); - } catch (error) { - attempt++; - if (attempt >= maxAttempts || (shouldRetry && !shouldRetry(error))) { - reject(error); - return; - } - - let nextDelay: number; - switch (backoff) { - case "linear": - nextDelay = delay * attempt; - break; - case "fixed": - nextDelay = delay; - break; - default: // exponential - nextDelay = delay * 2 ** (attempt - 1); - } - - onRetry?.(attempt, nextDelay); - setTimeout(tryOperation, nextDelay); - } - }; - - tryOperation(); - }); + let attempt = 0; + const execute = async (): Promise<T> => { + try { + return await operation(); + } catch (error) { + attempt++; + if (attempt >= maxAttempts || (shouldRetry && !shouldRetry(error))) { + throw error; + } + let nextDelay: number; + switch (backoff) { + case "linear": + nextDelay = delay * attempt; + break; + case "fixed": + nextDelay = delay; + break; + default: + nextDelay = delay * 2 ** (attempt - 1); + } + onRetry?.(attempt, nextDelay); + await new Promise((r) => setTimeout(r, nextDelay)); + return execute(); + } + }; + return execute();src/utils/tokens.ts (1)
19-28: Consider caching the dynamicrequire("gpt-3-encoder")result.
limitToTokenstriggers up to two separaterequire("gpt-3-encoder")calls per invocation — once insidecountTokens(line 21) and once directly (line 46). While Node/Bun cachesrequireresults, you could avoid the repeated try/catch overhead and make the code clearer by caching the module at module scope:♻️ Suggested approach
+let _encoder: { encode: (t: string) => number[]; decode: (t: number[]) => string } | null = null; +let _encoderLoaded = false; + +function getEncoder() { + if (!_encoderLoaded) { + try { _encoder = require("gpt-3-encoder"); } catch { _encoder = null; } + _encoderLoaded = true; + } + return _encoder; +} + export function countTokens(text: string): number { - try { - const { encode } = require("gpt-3-encoder"); - return encode(text).length; - } catch { + const enc = getEncoder(); + if (enc) return enc.encode(text).length; + { const trimmed = text.trim(); if (!trimmed) return 0; return Math.ceil(trimmed.split(/\s+/).length * 1.3); } }This also makes the fallback path explicit rather than relying on catch-as-control-flow.
Also applies to: 45-63
src/utils/string.ts (1)
38-40: Doc says "all Unicode combining marks" but regex only covers the basic block.The regex
[\u0300-\u036f]covers the "Combining Diacritical Marks" block, which handles the vast majority of Latin-script diacritics. However, the JSDoc claim "Handles all Unicode combining marks" is slightly overstated — it misses supplemental blocks (e.g.,\u1AB0-\u1AFF,\u20D0-\u20FF). Consider softening the doc to "Handles common Latin diacritical marks" or using\p{M}with the unicode flag if broader coverage is needed.src/utils/format.ts (1)
175-180:formatNumberandformatTokensare near-identical — consider consolidating.
formatTokens(line 152-155) andformatNumber(line 175-179) differ only in thatformatNumberincludes a billions tier. If the distinction is intentional for semantic clarity, that's fine, but it's worth noting the duplication.
| export function formatRelativeTime(date: Date, options?: FormatRelativeTimeOptions): string { | ||
| const { maxDays = Infinity, fallbackFormat, rounding = "floor" } = options ?? {}; | ||
| const round = rounding === "round" ? Math.round : Math.floor; | ||
|
|
||
| const diffMs = Date.now() - date.getTime(); | ||
| const diffMinutes = round(diffMs / 60000); | ||
| const diffHours = round(diffMs / 3600000); | ||
| const diffDays = round(diffMs / 86400000); | ||
|
|
||
| if (maxDays !== Infinity && diffDays >= maxDays && fallbackFormat) { | ||
| return fallbackFormat(date); | ||
| } | ||
|
|
||
| if (diffMinutes < 1) return "just now"; | ||
| if (diffMinutes === 1) return "1 minute ago"; | ||
| if (diffMinutes < 60) return `${diffMinutes} minute${diffMinutes !== 1 ? "s" : ""} ago`; | ||
| if (diffHours === 1) return "1 hour ago"; | ||
| if (diffHours < 24) return `${diffHours} hour${diffHours !== 1 ? "s" : ""} ago`; | ||
| return `${diffDays} day${diffDays === 1 ? "" : "s"} ago`; | ||
| } |
There was a problem hiding this comment.
formatRelativeTime produces nonsensical output for future dates.
If date is in the future, diffMs is negative, causing all comparisons to fall through to line 119, which returns something like "-1 days ago". If this is only used for past dates, consider adding a guard:
♻️ Suggested guard
export function formatRelativeTime(date: Date, options?: FormatRelativeTimeOptions): string {
const { maxDays = Infinity, fallbackFormat, rounding = "floor" } = options ?? {};
const round = rounding === "round" ? Math.round : Math.floor;
const diffMs = Date.now() - date.getTime();
+ if (diffMs < 0) return "just now";
const diffMinutes = round(diffMs / 60000);🤖 Prompt for AI Agents
In `@src/utils/format.ts` around lines 101 - 120, formatRelativeTime currently
produces negative "X days ago" for future dates because diffMs can be negative;
add a guard near the top of formatRelativeTime that checks if diffMs < 0 and
handle future timestamps (e.g., return "just now" for very near-future or
compute absolute diffMinutes/diffHours/diffDays and return a future-formatted
string like "in X minutes/hours/days") instead of falling through to the
existing past logic; update uses of diffMs/diffMinutes/diffHours/diffDays (and
respect rounding/fallbackFormat) so future cases are symmetric and do not
produce negative values.
| export function slugify(title: string): string { | ||
| return title | ||
| .normalize("NFD") | ||
| .replace(/[\u0300-\u036f]/g, "") | ||
| .replace(/[^a-zA-Z0-9]+/g, "-") | ||
| .replace(/^-+|-+$/g, "") | ||
| .slice(0, 50); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'slugify' --type=ts --type=tsx -B2 -A2Repository: genesiscz/GenesisTools
Length of output: 93
🏁 Script executed:
rg -n 'slugify' --type=ts -B2 -A2Repository: genesiscz/GenesisTools
Length of output: 2509
slugify doesn't lowercase the output.
Slugs are conventionally all-lowercase. The current implementation preserves the original casing, which can produce slugs like My-Article-Title instead of the expected my-article-title. This is especially important since the function is used for filesystem operations in src/azure-devops/utils.ts (line 259), and the codebase already has a local slugify implementation in src/git-rebranch/index.ts that includes .toLowerCase().
♻️ Suggested fix
export function slugify(title: string): string {
return title
.normalize("NFD")
.replace(/[\u0300-\u036f]/g, "")
+ .toLowerCase()
.replace(/[^a-zA-Z0-9]+/g, "-")
.replace(/^-+|-+$/g, "")
.slice(0, 50);
}📝 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.
| export function slugify(title: string): string { | |
| return title | |
| .normalize("NFD") | |
| .replace(/[\u0300-\u036f]/g, "") | |
| .replace(/[^a-zA-Z0-9]+/g, "-") | |
| .replace(/^-+|-+$/g, "") | |
| .slice(0, 50); | |
| } | |
| export function slugify(title: string): string { | |
| return title | |
| .normalize("NFD") | |
| .replace(/[\u0300-\u036f]/g, "") | |
| .toLowerCase() | |
| .replace(/[^a-zA-Z0-9]+/g, "-") | |
| .replace(/^-+|-+$/g, "") | |
| .slice(0, 50); | |
| } |
🤖 Prompt for AI Agents
In `@src/utils/string.ts` around lines 11 - 18, The slugify function currently
preserves original casing; update slugify(title: string) to produce lowercase
slugs by applying .toLowerCase() to the normalized/replaced string (e.g., add
.toLowerCase() in the method chain before trimming/slicing) so outputs become
all-lowercase and consistent with the other local slugify implementation; ensure
the inserted .toLowerCase() is applied after diacritics removal and
non-alphanumeric replacement but before final .replace(/^-+|-+$/g, "") and
.slice(0, 50) to keep behavior unchanged except for casing.
- SKILL.md: fix "user name" → "username" wording - list: error early when @me cannot be resolved (no defaultUser configured) - prepare-import: show redirect indicator in title when redirectTitle is missing
- Fix critical stale cleanup scope bug (Thread #4/#11): project-scoped listing no longer deletes other projects' cache entries - Preserve matchSnippet when merging search results (Thread #19) - Fix rg-only results getting 0 slots when meta returns 20 (Thread #13) - Use rmSync with force flag instead of repetitive try/catch (Thread #1) - Add try/catch fallback for METADATA_VERSION (Thread #3/#6) - Add -- and -F flags to ripgrep commands (Thread #5/#17) - Reuse extractTextFromMessage in rgExtractSnippet (Thread #2) - Add 10MB file size cap + early exit in extraction (Thread #7/#18) - Add error handling for ripgrep functions (Thread #10/#16)
- Fix critical stale cleanup scope bug (Thread #4/#11): project-scoped listing no longer deletes other projects' cache entries - Preserve matchSnippet when merging search results (Thread #19) - Fix rg-only results getting 0 slots when meta returns 20 (Thread #13) - Use rmSync with force flag instead of repetitive try/catch (Thread #1) - Add try/catch fallback for METADATA_VERSION (Thread #3/#6) - Add -- and -F flags to ripgrep commands (Thread #5/#17) - Reuse extractTextFromMessage in rgExtractSnippet (Thread #2) - Add 10MB file size cap + early exit in extraction (Thread #7/#18) - Add error handling for ripgrep functions (Thread #10/#16)
#25) * fix(claude-history): full metadata extraction, rg search, auto-reindex - Read entire JSONL files for metadata (was limited to 50 lines/64KB) - Store full firstPrompt (was truncated to 120 chars) - Index ALL user messages into allUserText field (capped 5000 chars) - Summary/custom-title: latest wins (captures tail of file) - Add ripgrep full-content search fallback (rgSearchFiles/rgExtractSnippet) - Auto-reindex via MD5 hash of lib.ts+cache.ts (METADATA_VERSION) - Clean up stale cache entries for deleted session files - Rename DB from stats-cache.db to index.db - Show match snippets in claude-resume session picker - Search firstPrompt in matchByIdOrName * feat(claude-resume): show conversation excerpts in session picker Adds a second line below each session name showing additional context: summary (if name is from title), first prompt (if name is from summary), or match snippet (if from search). * fix(claude-resume): run metadata + rg search in parallel, merge results Previously rg only ran as fallback when metadata returned 0 results. Now both phases run concurrently via Promise.all. Results are deduped and merged: metadata matches first (ranked by relevance), then rg-only matches with snippets. Catches content in assistant messages and user text past the 5000-char metadata cap. * fix(claude-history): resolve dashed project names, add nerdy stats - extractProjectName: progressively resolve encoded dir against filesystem to recover dashed names (col-fe was showing as "fe") - Cache resolved project names to avoid repeated filesystem lookups - Show detailed stats in session picker: session count, project count, scope, indexed/stale/reindex status * feat(claude-resume): nerdy search diagnostics Show detailed breakdown of where results came from: - Index matches: count + what was searched (name/branch/project/prompt) - Content search: meta hits, rg total hits, overlap, rg-only unique hits * fix(claude-history): address PR #25 review comments - Fix critical stale cleanup scope bug (Thread #4/#11): project-scoped listing no longer deletes other projects' cache entries - Preserve matchSnippet when merging search results (Thread #19) - Fix rg-only results getting 0 slots when meta returns 20 (Thread #13) - Use rmSync with force flag instead of repetitive try/catch (Thread #1) - Add try/catch fallback for METADATA_VERSION (Thread #3/#6) - Add -- and -F flags to ripgrep commands (Thread #5/#17) - Reuse extractTextFromMessage in rgExtractSnippet (Thread #2) - Add 10MB file size cap + early exit in extraction (Thread #7/#18) - Add error handling for ripgrep functions (Thread #10/#16) * fix(claude-history): add cwd to early-exit check, use null for empty allUserText - Include cwd in early-exit condition to avoid skipping cwd extraction - Return null instead of "" for allUserText when no user text found - Anonymize path examples in comments
- Replace duplicate stripAnsi with import from utils/string (#18) - Fix wrapToWidth ANSI-aware truncation (#1/#3/#22) - Add await to recursive executeTool call (#9/#20) - Guard cursor when filtered list is empty (#5/#19) - Use basename() instead of split("/").pop() (#10/#16) - Validate --width NaN input (#14) - Re-display watch message after screen clear (#15)
- Replace duplicate stripAnsi with import from utils/string (#18) - Fix wrapToWidth ANSI-aware truncation (#1/#3/#22) - Add await to recursive executeTool call (#9/#20) - Guard cursor when filtered list is empty (#5/#19) - Use basename() instead of split("/").pop() (#10/#16) - Validate --width NaN input (#14) - Re-display watch message after screen clear (#15)
* fix: clean error formatting and fuzzy match for tools entry point * feat(tools): add discovery, introspection, and search-select modules - discovery.ts: scans src/ for tools, extracts descriptions from README.md - introspect.ts: runs --help on tools and parses Commander output into structured data - search-select.ts: single-select search prompt with onHighlight callback, modeled on search-multiselect * feat(tools): interactive browser with search, README preview, and subcommand explorer * feat(markdown): add render options and CLI flags (--watch, --width, --theme, --no-color) * fix: improve fuzzy matching to handle trailing dashes and segment matching "tools cli-" now correctly matches "markdown-cli" by stripping trailing dashes and checking dash-separated segments. * feat(markdown-cli): add demo subcommand with interactive template gallery * Revert "feat(markdown-cli): add demo subcommand with interactive template gallery" This reverts commit f5c84db. * feat(markdown): implement theme palette system (dark/light/minimal) Wire up the --theme flag to actual color palettes that change mermaid blocks, table borders, and alert colors per theme. * fix: address PR #28 code review feedback - Replace duplicate stripAnsi with import from utils/string (#18) - Fix wrapToWidth ANSI-aware truncation (#1/#3/#22) - Add await to recursive executeTool call (#9/#20) - Guard cursor when filtered list is empty (#5/#19) - Use basename() instead of split("/").pop() (#10/#16) - Validate --width NaN input (#14) - Re-display watch message after screen clear (#15) * fix(markdown-cli/tools): address PR #28 review feedback - Remove stray n8n.json (unrelated to PR scope) - fix(markdown-cli): reject invalid --theme values via Commander .choices() (Thread #23) - fix(tools): guard discoverTools against missing srcDir with existsSync check (Thread #24) - fix(markdown): use display width for emoji/wide chars in wrapToWidth (Thread #25)
- Use Promise.allSettled in showConfig to handle stale tokens (#14) - Clamp pct in renderBar to prevent RangeError (#17) - Clear firedThresholds on period reset in watch mode (#18) - Wrap entire getKeychainCredentials in try/catch for non-macOS (#19) - Escape sound parameter in AppleScript notifications (#20) - Fix import.meta.dir off-by-one in update tool (#21) - Use isCancelled() in withCancel() for light-mode cancel (#22) - Remove 100% cap on projected usage
- Use Promise.allSettled in showConfig to handle stale tokens (#14) - Clamp pct in renderBar to prevent RangeError (#17) - Clear firedThresholds on period reset in watch mode (#18) - Wrap entire getKeychainCredentials in try/catch for non-macOS (#19) - Escape sound parameter in AppleScript notifications (#20) - Fix import.meta.dir off-by-one in update tool (#21) - Use isCancelled() in withCancel() for light-mode cancel (#22) - Remove 100% cap on projected usage
Summary
src/utils/centralizing duplicated functions from across the codebaseNew modules
format.tsformatDuration(4 styles),formatBytes,formatCost,formatTokens,formatRelativeTime,formatList,formatNumberstring.tsslugify,stripAnsi,escapeShellArg,removeDiacritics,truncateText,sanitizeOutputarray.tswrapArrayasync.tsretry,debounce,throttle,withTimeoutfuzzy-match.tslevenshteinDistance,similarityScoretokens.tsestimateTokens,countTokens,limitToTokensdate.tsgetMonthDateRange,getDatesInMonthjson.tsparseJSONobject.tsisObject,deepMergeverbose.tscreateVerboseLoggerclaude/index.tsgetClaudeProjectsDir,parseJsonlTranscriptmarkdown/html-to-md.tshtmlToMarkdownMigrated consumers
azure-devops (utils, history, history-search), timely (date, entry-processor), ask (helpers, cli), files-to-prompt, last-changes, mcp-ripgrep, mcp-tsc, colors.ts
Key design decisions
formatDurationuses astyleparameter (tiered/hm-always/hm-smart/hms) to preserve exact per-caller outputformatRelativeTimeuses options (maxDays,fallbackFormat,rounding) for caller-specific behaviorTest plan
tsgo --noEmitpasses (no new type errors)Summary by CodeRabbit
Refactor
New Features
Documentation