refactor: core code cleanup and handler modularization#113
Open
sujitn wants to merge 5 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 58 minutes. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR consolidates handler implementations, removes calculation-engine sharding surface, simplifies linear algebra by delegating to nalgebra, adds a Deref trait to Date for ergonomics, and trims the validation test suite. All handlers migrate from analytics.rs to a centralized mod.rs module with shared AppState infrastructure. ChangesCore Refactoring and Handler Consolidation
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 (2)
crates/convex-engine/src/calc_graph.rs (1)
6-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the stale sharding rustdoc.
The module docs still advertise
CalculationGraph::with_sharding,ShardConfig, andowns_node, but those APIs were removed in this refactor. That leaves the public docs pointing at code that no longer exists.🤖 Prompt for 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. In `@crates/convex-engine/src/calc_graph.rs` around lines 6 - 30, The module docs reference removed APIs (CalculationGraph::with_sharding, ShardConfig, owns_node); remove or rewrite the sharding section so docs no longer mention these symbols. Update the top-level rustdoc in calc_graph.rs to either delete the entire "Sharding" block or replace it with current sharding guidance that references existing APIs (or a note that sharding was removed), and remove the example code snippets that call the nonexistent methods to keep public docs consistent with the refactor.crates/convex-server/src/handlers/analytics.rs (1)
2276-2285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReuse a single currency parser here.
This match table only recognizes a subset of the currencies accepted elsewhere and silently remaps the rest to
USD. For example, valid codes likeNZD,NOK, orSEKwill be misclassified in analytics requests. Please route this through the shared currency parsing path instead of maintaining a second mapping.🤖 Prompt for 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. In `@crates/convex-server/src/handlers/analytics.rs` around lines 2276 - 2285, The inline match in convert_portfolio_input duplicates currency mapping and silently maps unknown codes to USD; replace it with the shared currency parsing path (e.g., call the central parser such as parse_currency(&input.currency) or Currency::from_str(&input.currency)) and use its return value (handling Option/Result the same way other callers do) instead of the local match, falling back to Currency::USD only if the shared parser indicates an unknown/invalid code; keep the rest of convert_portfolio_input unchanged and reference the Currency type for the final assignment.
🤖 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 `@crates/convex-server/src/handlers/analytics.rs`:
- Line 92: There is a name collision because this module defines its own fn
parse_date while mod.rs exports pub(crate) fn parse_date; remove or rename the
local helper and replace calls with super::parse_date to avoid duplicate
definitions, ensuring all date parsing uses the shared function. Also update
convert_portfolio_input to stop using its limited inline currency match and
instead call super::parse_currency (the shared mapping) so currency
normalization is consistent across analytics handlers; change the local currency
logic to delegate to super::parse_currency and handle its return accordingly.
In `@crates/convex-server/src/handlers/mod.rs`:
- Around line 317-331: The handler delete_curve currently returns
(StatusCode::NO_CONTENT, Json({})) which sends a body with 204; change the
success branch to return just StatusCode::NO_CONTENT (no body) — e.g. return
StatusCode::NO_CONTENT; — and keep the NotFound branch unchanged; apply the same
removal of Json({}) for the other delete handlers referenced (the handlers
around the ranges 632-645 and 851-863) so all success 204 responses send no
body.
- Around line 566-585: The existence check using
state.bond_store.get_by_id(&bond.instrument_id).await currently treats Err(_)
the same as not-found and proceeds to upsert, which hides read failures; update
the logic in the create handler (around get_by_id and before
state.bond_store.upsert) to explicitly handle Err from get_by_id by returning an
appropriate error response (e.g., StatusCode::INTERNAL_SERVER_ERROR with a JSON
error) or propagating the error instead of continuing to upsert, and apply the
same fix to the other create path referenced (the block around lines 657-673) so
both code paths distinguish Ok(None), Ok(Some(_)) and Err(_) correctly.
- Around line 976-997: The parse_currency function currently coerces unknown
codes to Currency::USD which hides bad input; change parse_currency(s: &str) to
be fallible (e.g., return Result<Currency, ParseCurrencyError> or
Option<Currency>) instead of defaulting to USD, map recognized uppercase codes
to the corresponding Currency variants and return an Err/None for the _ case (do
not return Currency::USD), and update callers of parse_currency (wherever
parse_currency is invoked) to handle the error by returning a 400 Bad Request or
appropriate validation response. Ensure the new error type or None is
descriptive so handlers can produce a clear client-facing error message.
- Around line 354-362: The code currently maps unknown query.compounding values
to Compounding::Continuous which silently changes behavior and still echoes the
caller's original value; instead, validate the input and return a client error
for unsupported values. In the handler where you build `compounding` from
`query.compounding`, change the match to explicitly accept only the known
strings
("simple","annual","semiannual"/"semi_annual","quarterly","monthly","daily") and
for the `_` arm return an HTTP 400/BadRequest (or propagate a validation error)
indicating the unsupported compounding value; ensure any response uses the
parsed enum (Compounding::...) only when validation succeeds. Use the existing
identifiers `query.compounding` and `Compounding`/`compounding` to locate and
update the logic.
- Around line 140-168: get_bond_quote currently reads from
state.engine.reference_data().bonds.get_by_id(...) but the CRUD handlers
(create_bond, update_bond, batch_create_bonds) only modify state.bond_store, so
newly created/updated bonds aren't visible to the quote endpoint; fix by making
get_bond_quote first check state.bond_store for the InstrumentId (and return
that if present) and only fall back to
state.engine.reference_data().bonds.get_by_id(...) when not found, or
alternatively update the CRUD handlers to also write the same bond data into the
engine.reference_data() bond cache after persisting to state.bond_store so both
stores stay in sync (refer to get_bond_quote, create_bond, update_bond,
batch_create_bonds, state.bond_store and state.engine.reference_data().bonds).
---
Outside diff comments:
In `@crates/convex-engine/src/calc_graph.rs`:
- Around line 6-30: The module docs reference removed APIs
(CalculationGraph::with_sharding, ShardConfig, owns_node); remove or rewrite the
sharding section so docs no longer mention these symbols. Update the top-level
rustdoc in calc_graph.rs to either delete the entire "Sharding" block or replace
it with current sharding guidance that references existing APIs (or a note that
sharding was removed), and remove the example code snippets that call the
nonexistent methods to keep public docs consistent with the refactor.
In `@crates/convex-server/src/handlers/analytics.rs`:
- Around line 2276-2285: The inline match in convert_portfolio_input duplicates
currency mapping and silently maps unknown codes to USD; replace it with the
shared currency parsing path (e.g., call the central parser such as
parse_currency(&input.currency) or Currency::from_str(&input.currency)) and use
its return value (handling Option/Result the same way other callers do) instead
of the local match, falling back to Currency::USD only if the shared parser
indicates an unknown/invalid code; keep the rest of convert_portfolio_input
unchanged and reference the Currency type for the final assignment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 551892ce-fc00-47d0-ab87-bead9a1a9bed
📒 Files selected for processing (7)
crates/convex-core/src/types/date.rscrates/convex-core/src/validation_tests.rscrates/convex-engine/src/calc_graph.rscrates/convex-engine/src/lib.rscrates/convex-math/src/linear_algebra/mod.rscrates/convex-server/src/handlers/analytics.rscrates/convex-server/src/handlers/mod.rs
💤 Files with no reviewable changes (1)
- crates/convex-core/src/validation_tests.rs
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 7 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
…and database check safety
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.
This PR applies several code review cleanups to the codebase:
Summary by CodeRabbit
Release Notes