refactor(api): optimize api-handlers.ts and client.ts#11
Merged
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds shared safe-transform helpers, replaces local helpers with imports in client and API handlers, applies safe JSON parsing across memory list/search/delete/backfill flows, updates tag-to-shard selection, refactors migration progress into a class, and adds ChangesSafe Transformation Utilities and Refactoring
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/api-handlers.ts`:
- Around line 966-975: The toJSON() method currently returns a live reference to
this.errors which allows later mutations from handleRunTagMigrationBatch (which
pushes into this.errors) to modify any stored snapshots (e.g., used by
handleGetTagMigrationProgress); change toJSON() to return a shallow copy of the
errors array (e.g., [...this.errors] or this.errors.slice()) so callers receive
an immutable snapshot while leaving the internal this.errors modifiable.
- Around line 337-350: The code parses memory.metadata into metadata once but
calls safeJSONParse(memory.metadata) again when building the return value; reuse
the existing metadata variable instead. Update the return object's deletedPrompt
expression to reference the already-parsed metadata (cast to Record<string,
unknown>)?.promptId rather than calling safeJSONParse a second time, leaving the
earlier logic that deletes the linked prompt via
userPromptManager.deletePrompt(linkedPromptId) and the existing calls to
vectorSearch.deleteVector(db, id, shard) and
shardManager.decrementVectorCount(shard.id) unchanged.
- Around line 614-625: Extract the parsed metadata into a local variable to
avoid calling safeJSONParse(m.metadata) twice inside the object literal: before
building the object, assign const parsedMetadata = safeJSONParse(m.metadata) as
Record<string, unknown> and then use parsedMetadata for the metadata field and
for linkedPromptId ((parsedMetadata)?.promptId as string | undefined); update
references in the object (metadata, linkedPromptId) accordingly and ensure types
remain the same.
In `@src/services/utils/safe-transforms.ts`:
- Line 11: The ternary assigning numValue is redundant because both branches
call Number(timestamp); replace the ternary expression in the assignment to
numValue with a single Number(timestamp) call (i.e., set numValue =
Number(timestamp)); update references around the assignment in
safe-transforms.ts (look for the numValue variable and the timestamp parameter)
so the code is simpler and avoids the dead branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e60afdae-5879-4dd7-a0fc-1e29b7b32f54
📒 Files selected for processing (4)
.coderabbit.yamlsrc/services/api-handlers.tssrc/services/client.tssrc/services/utils/safe-transforms.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/utils/safe-transforms.ts`:
- Around line 6-21: safeToISOString currently converts non-number string
timestamps to NaN and silently returns the current time, corrupting valid string
timestamps from raw DB results; update safeToISOString to detect when timestamp
is a string and attempt to parse it as a Date (e.g., new Date(timestamp)) and if
that produces a valid date return its toISOString(), otherwise keep existing
fallbacks (numeric conversion, negative check, catch-all current time).
Reference the function safeToISOString and the local variable timestamp when
adding the string-type guard and parsed Date validity check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 256c50f4-4d74-48a4-b9ea-ec2e9b49f56c
📒 Files selected for processing (2)
src/services/api-handlers.tssrc/services/utils/safe-transforms.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/utils/safe-transforms.ts`:
- Around line 7-26: The outer try-catch in safeToISOString is unnecessary and
should be removed: delete the surrounding try { ... } catch { return new
Date().toISOString(); } so the function relies on the existing inner checks
(null/undefined guard, Number() -> isNaN/negative check, string parse via new
Date(...).getTime()) and returns new Date().toISOString() for invalid inputs;
keep all current branches that handle numeric, string, and invalid timestamps
unchanged and ensure the final return uses new Date(numValue).toISOString() for
valid numeric inputs.
- Line 13: The check "numValue < 0" in the numeric timestamp validation inside
safe-transforms.ts incorrectly rejects valid pre-1970 timestamps and causes
silent fallback/corruption; remove the "< 0" condition so the branch only checks
isNaN(numValue), i.e., change the conditional from "isNaN(numValue) || numValue
< 0" to just "isNaN(numValue)", keep the existing string-parsing fallback
behavior intact (so negative numeric timestamps are accepted and parsed as
dates), and run/update any tests around the function that uses numValue to
ensure historical dates are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf62a3a6-a7cb-4c44-82a7-a05aeae36e46
📒 Files selected for processing (1)
src/services/utils/safe-transforms.ts
Owner
Author
|
@coderabbitai review I pushed commit 39dab05 to address the remaining safeToISOString findings: removed the outer try/catch, stopped rejecting negative numeric timestamps, preserved valid string timestamp parsing, and kept invalid values falling back safely. Please re-review PR #11. |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
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
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.
Changes
safeToISOString()andsafeJSONParse()from bothapi-handlers.tsandclient.tsintosrc/services/utils/safe-transforms.ts— eliminates 60+ lines of duplicationgetProjectPathFromTag()now usesextractScopeFromTag()to scope shard search instead of iterating all project shardslet migrationProgresswithMigrationProgressTrackerclass — proper encapsulation, thread-safety, JSON serializationImpact
Testing
Requesting @coderabbitai full review for additional optimization opportunities in this module and across the codebase.
Summary by CodeRabbit
Refactor
Chores