Address chart lab review debt#13
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
![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 enhances the chart explorer by adding support for intraday data through improved time parsing and Unix timestamp handling. Key changes include refactoring endpoint normalization using the URL API, removing the Tesouro bonds history endpoint, and updating the chart's time scale visibility based on the presence of intraday data. Additionally, the logic for selecting candlestick charts was refined, and data sorting was updated to handle numeric timestamps correctly. I have no feedback to provide as there were no review comments.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a8444bb10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (typeof value === "number") { | ||
| const date = new Date(value > 1e11 ? value : value * 1000); | ||
| return timestampFromDate(date); |
There was a problem hiding this comment.
The new numeric fast-path treats all numbers as Unix timestamps (value * 1000 or ms), which silently misparses common period codes like 202401/202312 into 1970 dates. Before this commit, numeric values were stringified and could match the YYYYMM parser branch, so monthly series with numeric periodo values would now render with incorrect time axes and corrupted ordering.
Useful? React with 👍 / 👎.
| if (typeof a.time === "number" && typeof b.time === "number") { | ||
| return a.time - b.time; | ||
| } | ||
| return String(a.time).localeCompare(String(b.time)); |
There was a problem hiding this comment.
When one point has numeric time and another has date-string time, the comparator falls back to String(...).localeCompare(...), which is lexicographic rather than chronological across types. That can invert points (e.g., Unix 1714521600 vs 2024-05-01), causing non-monotonic series input and incorrect chart rendering for datasets mixing day-only and timestamped records.
Useful? React with 👍 / 👎.
1 participant
Summary
/bcb/series/../../openapi.json./tesouro/bonds/historyfrom the Labs allowlist until the chart parser supports Tesouro date fields.Review feedback addressed
parseTime.Validation
node --check src/findata/web/static/chart-explorer.js.venv/bin/python -m pytest tests/test_api.py -q.venv/bin/ruff format --check src/ tests/ scripts/.venv/bin/ruff check src/ tests/ scripts/.venv/bin/python -m mypy src/findata.venv/bin/python -m pytest tests/ -qgit diff --checkhttp://127.0.0.1:8765/charts: default Selic chart loads, no console errors,/bcb/series/../../openapi.jsonis rejected after URL canonicalization, and Tesouro history is rejected while unsupported.Merge note
Do not merge this PR until review status, bot/human comments, and pending improvements are summarized and Roberto explicitly approves merge in the current conversation.