Conversation
![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 transitions the repository to a manifest-driven architecture, introducing plots_manifest.json as the single source of truth for the atlas inventory. The homepage, README, and dashboard have been refactored to be dynamically generated, while data schemas and visualization scripts across multiple categories were standardized. New automated validation tools for link integrity and accessibility were also added. Review feedback recommends centralizing hardcoded constants in the compute timeline generators, enhancing the Markdown link-parsing regex to support optional titles, and pruning dead code from the refactored validation script.
| year = row["year"] | ||
| if year < 1945: | ||
| return 1e2 |
There was a problem hiding this comment.
Similar to the static generator, these fallback values are hardcoded magic numbers. Centralizing these constants in a shared utility or documenting them would improve consistency between the static and interactive plots.
| year = row["year"] | |
| if year < 1945: | |
| return 1e2 | |
| if year < 2012: | |
| return 1e10 | |
| return 1e12 |
| return md_links | ||
|
|
||
|
|
||
| def _is_local_link(link: str) -> bool: |
There was a problem hiding this comment.
The regular expression used to extract Markdown links is too restrictive. It will fail to correctly parse links that include optional titles (e.g., [text](url "title")), as the title will be included in the captured URL group, leading to broken link reports during validation.
md_links = re.findall(r"!?\[[^\]]*\]\(([^)\s]+)(?:\s+["'][^"']*["'])?\)", text)| PLOTS = [ | ||
| { | ||
| "name": entry["id"], | ||
| "csv": str(Path(entry["data"]).relative_to(entry["id"])), | ||
| "meta": str(Path(entry["metadata"]).relative_to(entry["id"])), | ||
| "required_files": [ | ||
| str(Path(path).relative_to(entry["id"])) | ||
| for path in (entry["interactive"], entry["png"], entry["svg"], f"{entry['id']}/index.html") | ||
| if path | ||
| ], | ||
| **entry, | ||
| } | ||
| for entry in plot_entries(ROOT, published_only=True) | ||
| ] |
There was a problem hiding this comment.
The PLOTS list is defined but appears to be unused in the refactored main() function, which now relies on plot_entries() from manifest_utils. This dead code should be removed to improve maintainability.
![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: 7a97ff040b
ℹ️ 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".
| const tooltip = d3.select('body').append('div').attr('class', 'tooltip'); | ||
|
|
||
| const totalHeight = LANES.length * (LANE_HEIGHT + 20) + MARGIN.top + MARGIN.bottom; | ||
| const totalHeight = 8 * (LANE_HEIGHT + 20) + MARGIN.top + MARGIN.bottom; |
There was a problem hiding this comment.
The dashboard now loads lanes from plots_manifest.json, but totalHeight is hardcoded for exactly 8 lanes. If the number of published non-dashboard entries changes, rendered lanes and the x-axis can fall outside the declared viewBox (or leave unused space), so the manifest-driven layout breaks as soon as inventory changes. Compute height from the loaded entry count instead of a fixed constant.
Useful? React with 👍 / 👎.
| const raw = row.year || row.Year || row.Years_Ago || row.date; | ||
| if (!raw) return null; | ||
| if (row.Years_Ago) return 2026 - parseFloat(raw); | ||
| const value = parseFloat(String(raw).slice(0, 4)); |
There was a problem hiding this comment.
For rows using date values, parseYear truncates to the first four characters, collapsing all events in the same calendar year to the same x-position. In this dashboard each lane uses a single y-level, so same-year events overlap and later points can hide earlier ones, making portions of the timeline and tooltips effectively inaccessible (notably for dense model-size releases). Parse full dates to fractional years (or timestamps) rather than year-only truncation.
Useful? React with 👍 / 👎.
1 participant
No description provided.