Site: swap rehype-autolink-headings for heading-anchors, use ToggleMenu from matterviz, improve DynamicScatter with interactive axes labels
#315
Conversation
- Use heading_ids() preprocessor to add IDs to headings at build time - Use heading_anchors() attachment in layout for runtime anchor icons - Add :target CSS styling for headings when navigated to - Fix link fragment in WBM readme (#links-to-wbm-files) - Disable MD051 markdownlint rule (link defined at runtime)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
site/src/lib/DynamicScatter.svelte (1)
467-470: Potential null reference in tooltip.The
axesobject derivesxandyfromoptions_by_key[selected.x]andoptions_by_key[selected.y]. Ifdata_loaderis called with an invalid key not present inoptions_by_key,axes.xoraxes.ywould beundefined, causing a runtime error when accessing.labelor.key.Consider adding optional chaining or guards consistent with how
color_valueandsize_valueare handled:Suggested fix
- {`@html` axes.x.label}: {x_formatted} - {`#if` axes.x.key === `date_added` && metadata.days_ago} + {`@html` axes.x?.label}: {x_formatted} + {`#if` axes.x?.key === `date_added` && metadata.days_ago} <small>({metadata.days_ago} days ago)</small>{/if}<br /> - {`@html` axes.y.label}: {y_formatted}<br /> + {`@html` axes.y?.label}: {y_formatted}<br />site/package.json (1)
47-49: Remove unused rehype dependencies from package.json.The packages
rehype-autolink-headingsandrehype-slugare no longer used in the codebase. They have been replaced bysvelte-multiselect'sheading_ids()function (seesite/svelte.config.js). Removing them will reduce unnecessary dependencies.
🤖 Fix all issues with AI agents
In `@site/src/lib/DynamicScatter.svelte`:
- Around line 127-138: The data_loader function accepts axis values `x | y | y2`
but only updates selected.x and selected.y; update logic to handle the `y2` case
(e.g., set selected.y2 = key) so selected state reflects the chosen axis, then
await tick() and return the same series/axis_label as done now; alternatively,
if y2 is not supported, change the function signature to remove `y2` or add a
clear comment explaining it's intentionally unsupported. Ensure references to
data_loader, selected, options_by_key, series, and tick are updated accordingly.
- Around line 237-244: The template contains a redundant expression
`${prop.label ?? prop.label}` inside the children snippet rendered via
children({ option: prop }) and format_property_path; replace the second
prop.label with the property identifier intended (use prop.key) so the path
expression becomes `${prop.path ?? ``}.${prop.key}` (matching the pattern used
elsewhere such as line 100) and keep the surrounding format_property_path and
model_counts_by_prop[prop.key] usage unchanged.
In `@site/src/lib/MetricsTable.svelte`:
- Around line 164-178: The current unsafe cast to IconName in the
affiliation_cell snippet risks runtime failure if org_logos contains an unknown
icon; instead, validate icon names where logos are constructed (e.g., in
get_org_logo()) against the set of valid matterviz IconName values (or the
labels.ts list), store a validated/icon-safe field on each logo entry, and
update affiliation_cell to use that validated field (or fall back to rendering
the img or skipping the icon) rather than using a blind `as IconName` cast;
ensure Icon and IconName references are preserved and validation logic maps
unknown names to a safe fallback or omits the icon.
In `@site/src/lib/TableColumnToggleMenu.svelte`:
- Line 2: The deleted test coverage for TableColumnToggleMenu.svelte
(site/tests/lib/TableColumnToggleMenu.svelte.test.ts) leaves
TableControls.svelte using an untested component; either restore the original
test file to re-enable coverage for TableColumnToggleMenu.svelte and ensure it
runs in CI, or create a tracking GitHub issue referencing the TODO in
TableColumnToggleMenu.svelte that documents the dependency on the matterviz
ToggleMenu release, target release/version and a removal date, and link that
issue from the TODO comment; update the test suite or CI config to reflect the
restored test or add the issue number to the TODO so the replacement isn't
forgotten.
In `@site/src/routes/models/`[slug]/+page.svelte:
- Around line 533-535: The scoped Svelte style uses :root which is outside the
component, so change the selector to use Svelte's global wrapper — replace the
current :root[data-theme='light'] rule with :global(:root[data-theme='light'])
so the --logo-brightness CSS variable (and any other theme root rules in this
block) apply at the document root (see the MetricsTable.svelte pattern for
reference).
🧹 Nitpick comments (4)
site/src/lib/DynamicScatter.svelte (2)
315-348: The{#iftrue}scoping pattern is functional but unconventional.Using
{#iftrue}to create a scope for@constdeclarations is valid Svelte but may confuse maintainers. Consider extracting the extent calculations into a derived state or helper function for clarity, though this is optional.The log-scale heuristic (requiring min > 0 and at least 100× range) is reasonable.
299-299: Unconventional spread syntax fordata_loader.
{...{ data_loader }}is functionally equivalent todata_loader={data_loader}. While both work, the direct prop assignment is more idiomatic and easier to read.Suggested simplification
- {...{ data_loader }} + data_loader={data_loader}.github/workflows/gh-pages.yml (1)
24-26: Consider pinning matterviz to a specific commit or tag for reproducible builds.Cloning
mainwithout a pinned commit hash means:
- Builds may break unexpectedly if matterviz introduces breaking changes
- Builds are not reproducible — the same workflow could produce different results on different days
- Harder to debug issues that arise from upstream changes
♻️ Suggested improvement
- git clone --depth 1 https://github.com/janosh/matterviz.git ../matterviz + git clone --depth 1 --branch v0.2.3 https://github.com/janosh/matterviz.git ../mattervizOr use a specific commit:
- git clone --depth 1 https://github.com/janosh/matterviz.git ../matterviz + git clone https://github.com/janosh/matterviz.git ../matterviz + cd ../matterviz && git checkout abc1234 && cd ../sitesite/src/routes/models/[slug]/+page.svelte (1)
273-284: Consider extracting sharedOrgLogocomponent to reduce duplication.The logo rendering logic here is identical to
MetricsTable.svelte(lines 165-178), and both files also duplicate the.org-logoCSS styling. Extracting this into a sharedOrgLogo.sveltecomponent would centralize the icon vs. image conditional logic and the associated styles.♻️ Example shared component
<!-- $lib/OrgLogo.svelte --> <script lang="ts"> import { Icon, type IconName } from 'matterviz' let { logo }: { logo: { id?: string; src?: string; name: string } } = $props() </script> {`#if` logo.id?.startsWith(`icon:`)} <span title={logo.name} class="org-logo"> <Icon icon={logo.id.replace(`icon:`, ``) as IconName} /> </span> {:else if logo.src} <img src={logo.src} alt="{logo.name} logo" title={logo.name} class="org-logo" /> {/if} <style> .org-logo { filter: grayscale(100%) brightness(var(--logo-brightness, 1)); height: 1em; width: auto; vertical-align: middle; margin: 0; } :global(:root[data-theme='light']) { --logo-brightness: 0.5; } </style>
The reusable deno-gh-pages workflow only supports pre-build and post-build inputs, not post-install. Move matterviz symlink commands into pre-build. Fix DynamicScatter and MetricsTable test selectors and async handling - Update DynamicScatter tests to use .controls-row instead of .controls-grid - Fix checkbox indexing to account for log-toggle checkboxes in DOM order - Add tick() calls after mount in MetricsTable tests for proper rendering - Simplify heatmap toggle test assertions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@site/tests/lib/DynamicScatter.test.svelte.ts`:
- Around line 92-106: Replace usages of the toBeDefined() matcher on DOM queries
with null-safe assertions: for variables like controls_row, plot_container,
fullscreen_button, settings_button (and the other occurrences noted at lines
112, 160, 337–344, 377–379) change expect(<query>) .toBeDefined() to
expect(<query>) .not.toBeNull() so that querySelector() returning null will fail
the test; update each assertion referencing the querySelector results
accordingly to use not.toBeNull().
🧹 Nitpick comments (2)
site/tests/lib/DynamicScatter.test.svelte.ts (2)
242-253: Avoid index-based checkbox selection.
Offsetting intoall_checkboxesmakes the tests brittle if toggle order changes. Consider selecting byaria-label,name, or addingdata-testidper checkbox so assertions stay stable when UI evolves.Also applies to: 350-366
346-348: Await DOM flush after opening the settings pane.
The pane render can be async; querying immediately after the click can be flaky. Considerawait tick()(orwaitFor) beforedoc_query.🧩 Suggested tweak
-import { mount } from 'svelte' +import { mount, tick } from 'svelte' ... - await settings_button?.click() - const pane = doc_query(pane_selector) + await settings_button?.click() + await tick() + const pane = doc_query(pane_selector)
Now that matterviz is installed from main, use the ToggleMenu component from matterviz instead of maintaining a local copy. Also adds SvelteKit sync step to the CI workflow to ensure types are generated before packaging.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@site/src/lib/TableControls.svelte`:
- Around line 2-3: The import of ToggleMenu from matterviz is invalid; remove
ToggleMenu from the import list in TableControls.svelte and delete the
corresponding <ToggleMenu bind:columns /> usage in the component (or replace it
with a valid component exported by matterviz such as one of
PeriodicTable/Structure/Composition/Trajectory/Heatmap/Scatter/ElementDetail or
a custom menu component) and update any references to the columns binding
accordingly (e.g., wire the columns state to your replacement or remove its
usage). Ensure the import line only imports valid symbols (e.g., Icon) from
matterviz and run type-check/build to confirm the error is resolved.
- Extract Logo component from MetricsTable and [slug]/+page.svelte - Add icon name validation in labels.ts for matterviz icons - Remove unused rehype-autolink-headings and rehype-slug packages - Fix DynamicScatter type errors (remove AxisConfig casts, add y2 to data_loader) - Add optional chaining to axes.x/y in tooltip for null safety - Update test selectors to use label titles instead of index-based queries - Change toBeDefined() to not.toBeNull() for querySelector results
janosh
changed the title
rehype-autolink-headings for heading-anchors, use ToggleMenu from matterviz, improve DynamicScatter with interactive axes labels
- Use importOriginal in matterviz mocks to preserve ICON_DATA export - Save original createElement before mocking to prevent infinite recursion
- Add property field to Label type for cases where key differs from property name (e.g. geo_opt symmetry metrics) - Update METADATA_COLS, HYPERPARAMS to use actual property names as keys - Extract get_label_path/get_label_value helpers in DynamicScatter - Add data_loader to color bar for dynamic property switching - Update test expectations for new label structure
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughIntegrates matterviz into CI and tests, replaces mdsvex/rehype heading plugins with svelte-multiselect anchors, refactors logo rendering and related exports/types, restructures DynamicScatter controls/axis logic, renames several metadata keys, and updates many tests to match the new components and keys. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
- Add matterviz/table mock for ToggleMenu component - Update metadata_cols test to use keys for filtering, labels for header checks - Update metrics test to use graph_construction_radius key
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 44-49: The workflow currently clones matterviz from main which is
non-deterministic; update the install-cmd step to pin the clone to a specific
tag or commit (e.g., use git clone --branch v0.2.2 --depth 1
https://github.com/janosh/matterviz.git ../matterviz or clone then git checkout
<commit>) so the build is reproducible; keep the subsequent commands (deno
install, deno run -A npm:`@sveltejs/kit` sync, deno task package, rm -rf ..., ln
-s ...) unchanged.
In `@site/tests/lib/table-export.test.ts`:
- Around line 48-50: The top-level binding const original_create_element =
document.createElement.bind(document) can throw in non‑DOM environments (Deno)
before describe.skipIf(IS_DENO) runs; change this to guard or lazily bind inside
a setup hook: either wrap the access with if (typeof document !== 'undefined')
before assigning, or move the bind into a beforeAll/beforeEach that runs under
the describe.skipIf(IS_DENO) block so original_create_element is only created
when document exists; apply the same change for the other top‑level document
usages around the 59-70 region to avoid import-time failures.
♻️ Duplicate comments (2)
site/src/lib/TableControls.svelte (1)
2-5: Verify ToggleMenu availability via thematterviz/tablesubpath.
@ts-expect-errorhides the type issue, but if the subpath isn’t shipped in the published package, bundlers/runtime will fail. Please confirm the package exports include it or provide a fallback.#!/bin/bash # Verify matterviz/table exists in the installed package pkg_dir=$(fd -t d '^matterviz$' node_modules | head -1) if [ -z "$pkg_dir" ]; then echo "matterviz not found in node_modules (run install first)" exit 1 fi python - <<'PY' import json, pathlib, sys pkg = pathlib.Path(sys.argv[1]) print("package:", pkg) print("exports:", json.loads(pkg.joinpath("package.json").read_text()).get("exports")) PY "$pkg_dir" ls -la "$pkg_dir/table" 2>/dev/null || find "$pkg_dir" -maxdepth 2 -type d -name 'table'Also applies to: 124-124
site/src/lib/DynamicScatter.svelte (1)
10-10: Confirmy2is never requested by the axis loader.
data_loaderacceptsy2but doesn’t update selection state for it. IfScatterPlotcan requesty2, consider handling it or narrowing the type.Also applies to: 142-153
🧹 Nitpick comments (3)
site/src/app.css (1)
245-265: Show heading anchors on keyboard focus, not just hover.
Consider revealing the anchor for:focus-visible/:focus-withinto keep keyboard navigation discoverable.♿ Suggested CSS tweak
:where(h2, h3, h4, h5, h6, strong):hover a[aria-hidden='true'] { opacity: 1; } +:where(h2, h3, h4, h5, h6, strong):focus-within a[aria-hidden='true'], +a[aria-hidden='true']:focus-visible { + opacity: 1; +}.github/workflows/gh-pages.yml (1)
24-27: Consider pinning the matterviz clone for reproducible CI builds.Cloning
maincan introduce non-deterministic builds if upstream changes land. If you don’t need moving-target builds, pin to a tag/commit or make the ref configurable.site/src/lib/MetricsTable.svelte (1)
2-2: Per-logo rendering looks good; consider a stronger key fallback.
If a logo has neitheridnorsrc(e.g., icon-only entries), the key can becomeundefinedand cause duplicate-key churn. A last-resort fallback to the loop index keeps keys stable.♻️ Optional key fallback
-{`#each` (row as ModelData).org_logos ?? [] as logo (logo.id ?? logo.src)} +{`#each` (row as ModelData).org_logos ?? [] as logo, i (logo.id ?? logo.src ?? i)} <Logo {logo} /> {/each}Also applies to: 164-167
| // Save original createElement before any mocks | ||
| const original_create_element = document.createElement.bind(document) | ||
|
|
There was a problem hiding this comment.
Guard top‑level document access to avoid Deno import failure.
document.createElement is bound at module load time, so in Deno (or any non‑DOM env) this can throw before describe.skipIf(IS_DENO) takes effect.
✅ Suggested fix
-// Save original createElement before any mocks
-const original_create_element = document.createElement.bind(document)
+// Save original createElement before any mocks (guard for non-DOM envs)
+const original_create_element =
+ typeof document !== `undefined`
+ ? document.createElement.bind(document)
+ : null- .mockImplementation((tag: string) =>
+ .mockImplementation((tag: string) =>
tag === `a`
? ({
href: ``,
download: ``,
click: mock_click,
} as unknown as HTMLAnchorElement)
- : original_create_element(tag)
+ : original_create_element?.(tag)
)Also applies to: 59-70
🤖 Prompt for AI Agents
In `@site/tests/lib/table-export.test.ts` around lines 48 - 50, The top-level
binding const original_create_element = document.createElement.bind(document)
can throw in non‑DOM environments (Deno) before describe.skipIf(IS_DENO) runs;
change this to guard or lazily bind inside a setup hook: either wrap the access
with if (typeof document !== 'undefined') before assigning, or move the bind
into a beforeAll/beforeEach that runs under the describe.skipIf(IS_DENO) block
so original_create_element is only created when document exists; apply the same
change for the other top‑level document usages around the 59-70 region to avoid
import-time failures.
The column toggle functionality is now provided by matterviz's ToggleMenu component which is mocked in tests. Skip tests that depend on its internal DOM structure (.column-toggles summary).
…gleMenu` from `matterviz`, improve `DynamicScatter` with interactive axes labels (#315) * Replace rehype-autolink-headings with svelte-multiselect heading_anchors - Use heading_ids() preprocessor to add IDs to headings at build time - Use heading_anchors() attachment in layout for runtime anchor icons - Add :target CSS styling for headings when navigated to - Fix link fragment in WBM readme (#links-to-wbm-files) - Disable MD051 markdownlint rule (link defined at runtime) * Fix gh-pages workflow to use valid pre-build input The reusable deno-gh-pages workflow only supports pre-build and post-build inputs, not post-install. Move matterviz symlink commands into pre-build. Fix DynamicScatter and MetricsTable test selectors and async handling - Update DynamicScatter tests to use .controls-row instead of .controls-grid - Fix checkbox indexing to account for log-toggle checkboxes in DOM order - Add tick() calls after mount in MetricsTable tests for proper rendering - Simplify heatmap toggle test assertions * Delete local TableColumnToggleMenu, import ToggleMenu from matterviz Now that matterviz is installed from main, use the ToggleMenu component from matterviz instead of maintaining a local copy. Also adds SvelteKit sync step to the CI workflow to ensure types are generated before packaging. * Fix gh-pages workflow: run matterviz build in subshell to preserve working directory * Install matterviz from main in vitest CI job * Refactor org logos, fix DynamicScatter types, improve test stability - Extract Logo component from MetricsTable and [slug]/+page.svelte - Add icon name validation in labels.ts for matterviz icons - Remove unused rehype-autolink-headings and rehype-slug packages - Fix DynamicScatter type errors (remove AxisConfig casts, add y2 to data_loader) - Add optional chaining to axes.x/y in tooltip for null safety - Update test selectors to use label titles instead of index-based queries - Change toBeDefined() to not.toBeNull() for querySelector results * Fix test failures: matterviz mocks and table-export recursion - Use importOriginal in matterviz mocks to preserve ICON_DATA export - Save original createElement before mocking to prevent infinite recursion * Refactor label keys to use property names for data access - Add property field to Label type for cases where key differs from property name (e.g. geo_opt symmetry metrics) - Update METADATA_COLS, HYPERPARAMS to use actual property names as keys - Extract get_label_path/get_label_value helpers in DynamicScatter - Add data_loader to color bar for dynamic property switching - Update test expectations for new label structure * Fix test failures from label key refactoring - Add matterviz/table mock for ToggleMenu component - Update metadata_cols test to use keys for filtering, labels for header checks - Update metrics test to use graph_construction_radius key * Skip tests for ToggleMenu component now from matterviz The column toggle functionality is now provided by matterviz's ToggleMenu component which is mocked in tests. Skip tests that depend on its internal DOM structure (.column-toggles summary). Move document binding inside describe block for Deno compatibility The top-level document.createElement.bind(document) was throwing in non-DOM environments (Deno) before describe.skipIf(IS_DENO) could skip the tests. Now bound lazily in beforeAll inside the skip block. Increase timeout for flaky MetricsTable tests
Summary
rehype-autolink-headingsandrehype-slugwithheading_ids()preprocessor andheading_anchors()attachment from svelte-multiselectChanges
<main>:targetstyling for headings, update commentTest plan
pnpm svelte-checkpassespnpm vite buildpassesSummary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores / Tests
✏️ Tip: You can customize this high-level summary in your review settings.