Repository navigation
feat: Improved color scales + example app - #565
Conversation
Signed-off-by: Ilya Boyandin <[email protected]>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 28 minutes and 17 seconds. ⌛ 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 (3)
📝 WalkthroughWalkthroughThis PR introduces a new Vite + React example application demonstrating color scale legends, adds five specialized legend rendering components (continuous, stepped, categorical with tick positioning), updates the Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser/UI
participant Room as Room Component
participant RoomStore as Room Store
participant DuckDB as DuckDB
participant LegendBuilder as Legend Builder
participant Legends as Legend Components
Browser->>Room: Mount Room component
Room->>RoomStore: Initialize store with cars.parquet
RoomStore->>DuckDB: Load parquet dataset
DuckDB-->>RoomStore: cars table ready
Room->>DuckDB: Query cars table
DuckDB-->>Room: Query results
Room->>LegendBuilder: Build color scale legends<br/>(sequential, diverging, etc.)
LegendBuilder-->>Room: Resolved legend configs
Room->>Legends: Render ColorScaleLegend<br/>+ ContinuousLegend<br/>+ SteppedLegend<br/>+ CategoricalLegend
Legends->>Legends: Position ticks,<br/>detect overlap,<br/>build swatches
Legends-->>Browser: Render legend UI +<br/>colored sample rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 5
🧹 Nitpick comments (4)
examples/color-scales/README.md (1)
17-20: Consider documentingpnpminstead ofnpmfor in-repo usage.This monorepo uses pnpm with
workspace:*links, and per the repo's guidance the example must be run afterpnpm buildfrom the repo root (e.g.,pnpm dev color-scales-example). Runningnpm installinsideexamples/color-scaleswill not resolve those workspace dependencies the same way. Other examples in the repo also documentpnpm install && pnpm devfor the in-repo flow.If this README is intended for users scaffolding a standalone copy via
giget(where deps are real published versions), the npm form is fine — in that case it might be worth a one-line note clarifying the two contexts.📝 Suggested doc tweak
## Running locally +From the repo root (workspace dev flow): + +```sh +pnpm install +pnpm build +pnpm dev color-scales-example +``` + +Or, if you've scaffolded this example as a standalone project: + ```sh npm install npm run dev</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@examples/color-scales/README.mdaround lines 17 - 20, Update the README in
examples/color-scales to document the in-repo pnpm workflow: replace or augment
the existing npm snippet so it instructs users to run "pnpm install" at the repo
root (or in the example), then "pnpm build" from the repo root, and then "pnpm
dev color-scales-example" to run the example using workspace:* dependencies;
also keep a short one-line alternative explaining that if the example was
scaffolded standalone (via giget) they can use the existing "npm install" / "npm
run dev" commands.</details> </blockquote></details> <details> <summary>packages/color-scales/src/legend/LegendTicks.tsx (2)</summary><blockquote> `15-17`: **Hard‑coded glyph width is fragile.** `label.length * 5.6 + 8` assumes 10px tabular-nums Latin glyphs. It will under/over-estimate for non‑Latin labels (e.g., categorical with CJK characters) and any future font/size change in `LegendTicks`, causing either overlapping labels or unnecessary thinning. Consider measuring with a hidden canvas/`measureText` once per render, or accepting it as a known limitation in a comment. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/color-scales/src/legend/LegendTicks.tsxaround lines 15 - 17, The
hard-coded width heuristic in estimateTickLabelWidth is fragile for non-Latin
glyphs and font changes; replace the multiplication with a real text measurement
using a Canvas 2D context: create or reuse a hidden/offscreen canvas context in
LegendTicks (e.g., per render or memoized), call context.font to match the
legend label font and use context.measureText(label).width for the width
(optionally add a small padding like +8), and cache measurements per-label to
avoid repeated work; update estimateTickLabelWidth to use this measured width so
labels don't overlap across scripts and font changes.</details> --- `34-50`: **Greedy filtering can drop the domain‑max tick.** `getVisibleTicks` walks ticks in ascending offset and drops any that overlap. When labels are wide (e.g., long formatted numbers) the trailing tick at offset 100 — which conveys the domain maximum — can be the one filtered out, while a less informative mid‑range tick is kept. Consider always preserving the first and last ticks (endpoints) and only thinning interior ticks, or running the greedy pass from both ends. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@packages/color-scales/src/legend/LegendTicks.tsxaround lines 34 - 50,
getVisibleTicks currently greedily removes overlapping ticks by scanning sorted
offsets and can drop important endpoints; modify getVisibleTicks (which uses
getTickBounds, LegendTick and width) to always preserve the first and last tick:
compute sortedTicks, always include sortedTicks[0] and sortedTicks[last], then
run the greedy overlap removal only on the interior ticks (or alternatively
perform two-pass greedy from both ends) so that domain-min and domain-max ticks
are never removed while still thinning overlapping interior labels.</details> </blockquote></details> <details> <summary>examples/color-scales/src/room.tsx (1)</summary><blockquote> `183-188`: **Duplicates the `rgba` helper from the package.** The same `rgba(...)` formatting (with `color[3] / 255`) already lives in `packages/color-scales/src/legend/utils.ts` and is used by `CategoricalLegend`/`SteppedLegend`. Consider exporting that helper from `@sqlrooms/color-scales` so example/consumer code doesn't have to reimplement the alpha-normalization rule. <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@examples/color-scales/src/room.tsxaround lines 183 - 188, This duplicates
the inline RGBA formatting; instead import and use the shared helper (export the
helper from packages/color-scales/src/legend/utils.ts and consume it in this
file) rather than reimplementing alpha normalization—replace the inline style
construction for backgroundColor in room.tsx with a call to the exported helper
(the same function used by CategoricalLegend/SteppedLegend) so the example
relies on the package utility.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@examples/color-scales/src/index.css:
- Around line 5-8: The
@sourceentry "'src//*.{ts,tsx}'" is wrong because
paths are resolved relative to this CSS file; replace it with a relative pattern
that points to the files in the same directory (e.g. ".//.{ts,tsx}") so
Tailwind picks up the local TypeScript/TSX sources; update the@sourceline in
this file to use "./**/.{ts,tsx}" (or "./.{ts,tsx}" if you only want top-level
files) and remove the erroneous 'src/**/.{ts,tsx}' entry.In
@examples/color-scales/src/room.tsx:
- Line 178: The key generation for the list item uses
${colorScale.field}-${row.Car}which can collide whenrow.Carvalues repeat;
update the key used in the mapped element (the expression producing
key={${colorScale.field}-${row.Car}``) to append a stable per-item identifier
such as the iteration index or a unique id (e.g.,
${colorScale.field}-${row.Car}-${index}) so keys are guaranteed unique across
rendered rows.- Around line 229-245: The code ignores queryResult.error causing silent
failures; update the component after the useSql/queryResult usage to handle
errors by checking queryResult.error and rendering an error state or at minimum
logging the error (e.g., when !isTableReady is false && queryResult.isLoading is
false && queryResult.error is set). Locate the queryResult reference returned by
useSql and the existing loading branch that checks isTableReady and
queryResult.isLoading; add a conditional branch that renders a simple error
message or a console.error including queryResult.error.message (or error object)
before rendering the gallery so failures are visible.In
@packages/color-scales/src/ColorScaleLegend.tsx:
- Around line 26-29: The current key for the legend items in ColorScaleLegend
(inside the legends.map(...) JSX) uses${legend.title}-${legend.type}which
can collide when multiple legends share the same title and type; update the map
callback to include the array index (e.g., map((legend, index) => ...) and set
the key to include the index like${legend.title}-${legend.type}-${index}) so
each renderedis unconditionally unique.In
@packages/color-scales/src/legend/SteppedLegend.tsx:
- Around line 25-29: The spacer used when legend.ticks is empty in SteppedLegend
doesn't match the height of LegendTicks (h-2 vs h-8) causing misalignment;
update the empty branch in SteppedLegend to use the same height as LegendTicks
(h-8) or extract a shared constant (e.g., TICKS_HEIGHT) used by both LegendTicks
and SteppedLegend so both render the same vertical space when ticks are
absent/present.
Nitpick comments:
In@examples/color-scales/README.md:
- Around line 17-20: Update the README in examples/color-scales to document the
in-repo pnpm workflow: replace or augment the existing npm snippet so it
instructs users to run "pnpm install" at the repo root (or in the example), then
"pnpm build" from the repo root, and then "pnpm dev color-scales-example" to run
the example using workspace:* dependencies; also keep a short one-line
alternative explaining that if the example was scaffolded standalone (via giget)
they can use the existing "npm install" / "npm run dev" commands.In
@examples/color-scales/src/room.tsx:
- Around line 183-188: This duplicates the inline RGBA formatting; instead
import and use the shared helper (export the helper from
packages/color-scales/src/legend/utils.ts and consume it in this file) rather
than reimplementing alpha normalization—replace the inline style construction
for backgroundColor in room.tsx with a call to the exported helper (the same
function used by CategoricalLegend/SteppedLegend) so the example relies on the
package utility.In
@packages/color-scales/src/legend/LegendTicks.tsx:
- Around line 15-17: The hard-coded width heuristic in estimateTickLabelWidth is
fragile for non-Latin glyphs and font changes; replace the multiplication with a
real text measurement using a Canvas 2D context: create or reuse a
hidden/offscreen canvas context in LegendTicks (e.g., per render or memoized),
call context.font to match the legend label font and use
context.measureText(label).width for the width (optionally add a small padding
like +8), and cache measurements per-label to avoid repeated work; update
estimateTickLabelWidth to use this measured width so labels don't overlap across
scripts and font changes.- Around line 34-50: getVisibleTicks currently greedily removes overlapping
ticks by scanning sorted offsets and can drop important endpoints; modify
getVisibleTicks (which uses getTickBounds, LegendTick and width) to always
preserve the first and last tick: compute sortedTicks, always include
sortedTicks[0] and sortedTicks[last], then run the greedy overlap removal only
on the interior ticks (or alternatively perform two-pass greedy from both ends)
so that domain-min and domain-max ticks are never removed while still thinning
overlapping interior labels.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `7f6e8bf9-e3e1-4fe4-8411-9791bb05c4be` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 1dd3153efb958b57227f8702967c3ed967a90375 and 93fc5fdb8ecd926cc8f205d60d8900b58e63092d. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (21)</summary> * `examples/color-scales/README.md` * `examples/color-scales/eslint.config.js` * `examples/color-scales/index.html` * `examples/color-scales/package.json` * `examples/color-scales/src/index.css` * `examples/color-scales/src/main.tsx` * `examples/color-scales/src/room.tsx` * `examples/color-scales/src/vite-env.d.ts` * `examples/color-scales/tsconfig.app.json` * `examples/color-scales/tsconfig.json` * `examples/color-scales/tsconfig.node.json` * `examples/color-scales/vite.config.ts` * `packages/color-scales/README.md` * `packages/color-scales/src/ColorScaleLegend.tsx` * `packages/color-scales/src/config.ts` * `packages/color-scales/src/legend/CategoricalLegend.tsx` * `packages/color-scales/src/legend/ContinuousLegend.tsx` * `packages/color-scales/src/legend/LegendTicks.tsx` * `packages/color-scales/src/legend/SteppedLegend.tsx` * `packages/color-scales/src/legend/utils.ts` * `packages/color-scales/src/scale.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| @source '../index.html'; | ||
| @source 'src/**/*.{ts,tsx}'; | ||
| @source '../../../packages/*/src/**/*.{ts,tsx}'; | ||
| @source '../node_modules/@sqlrooms/*/dist/'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Tailwind CSS v4 @source directive path resolution relative to CSS file
💡 Result:
In Tailwind CSS v4, the @source directive resolves paths relative to the CSS file containing the directive. This applies even in imported CSS files, where paths are correctly resolved relative to the importing file's location due to Tailwind's core handling of @import statements, which preserves the base path for each CSS file during processing. For example, in ./src/input.css: @import "tailwindcss"; @source "../resources/views//.blade.php"; The path "../resources/views//.blade.php" is relative to ./src/input.css. This behavior is confirmed in official Tailwind CSS documentation and development pull requests. Paths support glob patterns and work across different operating systems (tested on Linux and Windows). Quoted paths are required for globs. Note: Absolute paths are not supported; paths must be relative. For node_modules or external libs, use relative paths like "../node_modules/@my-company/ui-lib".
Citations:
- 1: Add
@sourcesupport tailwindlabs/tailwindcss#14078 - 2: https://tailwindcss.com/docs/detecting-classes-in-source-files
- 3: https://tailwindcss.com/docs/functions-and-directives
- 4: Handle new
source(…)tailwindlabs/tailwindcss#14767
🏁 Script executed:
# Check the structure of examples/color-scales directory
find examples/color-scales -type f -name '*.ts' -o -name '*.tsx' | head -20Repository: sqlrooms/sqlrooms
Length of output: 208
🏁 Script executed:
# List the directory structure
tree -L 3 examples/color-scales/ 2>/dev/null || find examples/color-scales -type d | head -20Repository: sqlrooms/sqlrooms
Length of output: 340
🏁 Script executed:
# Check if src/src exists
ls -la examples/color-scales/src/ 2>/dev/null || echo "Directory not found"Repository: sqlrooms/sqlrooms
Length of output: 411
Incorrect @source path on line 6.
In Tailwind v4, @source paths are resolved relative to the CSS file. Since this file lives at examples/color-scales/src/index.css, 'src/**/*.{ts,tsx}' resolves to examples/color-scales/src/src/**/*.{ts,tsx}, which doesn't exist. The actual TypeScript files are in examples/color-scales/src/ itself. Tailwind's automatic content detection will likely still pick up the local sources, but the explicit directive is dead.
🛠️ Proposed fix
`@source` '../index.html';
-@source 'src/**/*.{ts,tsx}';
+@source './**/*.{ts,tsx}';
`@source` '../../../packages/*/src/**/*.{ts,tsx}';
`@source` '../node_modules/@sqlrooms/*/dist/';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @source '../index.html'; | |
| @source 'src/**/*.{ts,tsx}'; | |
| @source '../../../packages/*/src/**/*.{ts,tsx}'; | |
| @source '../node_modules/@sqlrooms/*/dist/'; | |
| `@source` '../index.html'; | |
| `@source` './**/*.{ts,tsx}'; | |
| `@source` '../../../packages/*/src/**/*.{ts,tsx}'; | |
| `@source` '../node_modules/@sqlrooms/*/dist/'; |
🧰 Tools
🪛 Stylelint (17.9.0)
[error] 5-5: Unexpected unknown at-rule "@source" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 6-6: Unexpected unknown at-rule "@source" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 7-7: Unexpected unknown at-rule "@source" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
[error] 8-8: Unexpected unknown at-rule "@source" (scss/at-rule-no-unknown)
(scss/at-rule-no-unknown)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/color-scales/src/index.css` around lines 5 - 8, The `@source` entry
"'src/**/*.{ts,tsx}'" is wrong because paths are resolved relative to this CSS
file; replace it with a relative pattern that points to the files in the same
directory (e.g. "./**/*.{ts,tsx}") so Tailwind picks up the local TypeScript/TSX
sources; update the `@source` line in this file to use "./**/*.{ts,tsx}" (or
"./*.{ts,tsx}" if you only want top-level files) and remove the erroneous
'src/**/*.{ts,tsx}' entry.
| const queryResult = useSql<CarRow>({ | ||
| query: `SELECT * FROM cars ORDER BY Model, Car`, | ||
| enabled: isTableReady, | ||
| }); | ||
| const rows = useMemo( | ||
| () => queryResult.data?.toArray() ?? [], | ||
| [queryResult.data], | ||
| ); | ||
| const legends = useMemo(() => buildLegends(rows), [rows]); | ||
|
|
||
| if (!isTableReady || queryResult.isLoading) { | ||
| return ( | ||
| <div className="flex h-full w-full items-center justify-center"> | ||
| <Spinner /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
queryResult.error is silently ignored.
If the parquet fetch or SQL query fails, isLoading becomes false but queryResult.data stays undefined → the gallery renders an empty grid with no feedback. For an example app this is acceptable, but a small error state (or at least logging the error) would make the demo more obvious to debug when network/CORS issues arise.
Suggested addition
if (!isTableReady || queryResult.isLoading) {
return (
<div className="flex h-full w-full items-center justify-center">
<Spinner />
</div>
);
}
+
+ if (queryResult.error) {
+ return (
+ <div className="text-destructive flex h-full w-full items-center justify-center text-sm">
+ Failed to load cars dataset: {String(queryResult.error)}
+ </div>
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const queryResult = useSql<CarRow>({ | |
| query: `SELECT * FROM cars ORDER BY Model, Car`, | |
| enabled: isTableReady, | |
| }); | |
| const rows = useMemo( | |
| () => queryResult.data?.toArray() ?? [], | |
| [queryResult.data], | |
| ); | |
| const legends = useMemo(() => buildLegends(rows), [rows]); | |
| if (!isTableReady || queryResult.isLoading) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center"> | |
| <Spinner /> | |
| </div> | |
| ); | |
| } | |
| const queryResult = useSql<CarRow>({ | |
| query: `SELECT * FROM cars ORDER BY Model, Car`, | |
| enabled: isTableReady, | |
| }); | |
| const rows = useMemo( | |
| () => queryResult.data?.toArray() ?? [], | |
| [queryResult.data], | |
| ); | |
| const legends = useMemo(() => buildLegends(rows), [rows]); | |
| if (!isTableReady || queryResult.isLoading) { | |
| return ( | |
| <div className="flex h-full w-full items-center justify-center"> | |
| <Spinner /> | |
| </div> | |
| ); | |
| } | |
| if (queryResult.error) { | |
| return ( | |
| <div className="text-destructive flex h-full w-full items-center justify-center text-sm"> | |
| Failed to load cars dataset: {String(queryResult.error)} | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/color-scales/src/room.tsx` around lines 229 - 245, The code ignores
queryResult.error causing silent failures; update the component after the
useSql/queryResult usage to handle errors by checking queryResult.error and
rendering an error state or at minimum logging the error (e.g., when
!isTableReady is false && queryResult.isLoading is false && queryResult.error is
set). Locate the queryResult reference returned by useSql<CarRow> and the
existing loading branch that checks isTableReady and queryResult.isLoading; add
a conditional branch that renders a simple error message or a console.error
including queryResult.error.message (or error object) before rendering the
gallery so failures are visible.
Summary by CodeRabbit
New Features
Documentation