Extract pivot engine and React components into standalone npm workspace packages#19
Open
zodvik wants to merge 2 commits into
Open
Extract pivot engine and React components into standalone npm workspace packages#19zodvik wants to merge 2 commits into
zodvik wants to merge 2 commits into
Conversation
…ce packages - Add `@utils-foo/pivot-engine`: zero-dependency headless pivot engine with aggregators, sorters, PivotEngine class, and a plugin API for custom aggregation types (`registerAggregator`) - Add `@utils-foo/pivot-react`: React component library (PivotTable, PivotGrid, ConfigPanel, DataInput, FilterModal) with PapaParse CSV support - Set up npm workspaces with Vite aliases and tsconfig paths for dev-time source resolution (no separate build step) - Add 110 unit tests for engine (sorters, aggregators, PivotEngine) - Reduce `src/tools/pivot-table/index.tsx` to a thin wrapper around PivotTable https://claude.ai/code/session_01Gq6D8TLHT6eW3Gh8ZzFWa3
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 35 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pivot-react/src/ui/Button.tsx">
<violation number="1" location="packages/pivot-react/src/ui/Button.tsx:11">
P1: Set a safe default `type="button"` to avoid accidental form submission when this component is used inside forms.</violation>
</file>
<file name="packages/pivot-react/src/hooks/usePivotData.ts">
<violation number="1" location="packages/pivot-react/src/hooks/usePivotData.ts:10">
P1: The early return incorrectly treats "no rows and no cols" as empty, which suppresses valid grand-total results for 0-dimension pivots.</violation>
</file>
<file name="packages/pivot-engine/src/__tests__/aggregators.test.ts">
<violation number="1" location="packages/pivot-engine/src/__tests__/aggregators.test.ts:285">
P3: This test depends on a plugin registered in a different test, making it order-dependent. Register a plugin in this test (with a unique type) before asserting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Button: add default type="button" to prevent accidental form submission - usePivotData: remove erroneous rows+cols==0 early-return that suppressed valid grand-total-only pivot results - aggregators.test: make getRegisteredPlugins test self-contained (register its own plugin instead of depending on a plugin from a prior test) https://claude.ai/code/session_01Gq6D8TLHT6eW3Gh8ZzFWa3
Deploying utils-bar with
|
| Latest commit: |
aca6fb6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://5a2375b6.utils-bar.pages.dev |
| Branch Preview URL: | https://claude-pivot-engine-library.utils-bar.pages.dev |
📝 WalkthroughWalkthroughThis PR transforms the pivot-table implementation into a monorepo structure with npm workspaces, introducing two new workspace packages: Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Deploying utils-foo with
|
| Latest commit: |
aca6fb6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c03840ff.utils-foo.pages.dev |
| Branch Preview URL: | https://claude-pivot-engine-library.utils-foo.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
vite.config.ts (1)
8-13: Keep@uialias behavior consistent between Vite and TypeScript.Vite now resolves bare
@ui, but TypeScript paths (from the providedtsconfig.jsonsnippet) only define@ui/*. If someone follows the barrel import pattern (import { ... } from '@ui'), editor/TS resolution may fail while Vite works.💡 Suggested follow-up (`tsconfig.json`)
"paths": { + "@ui": ["./src/components/ui/index.ts"], "@ui/*": ["./src/components/ui/*"], "@utils-foo/pivot-engine": ["./packages/pivot-engine/src/index.ts"], "@utils-foo/pivot-react": ["./packages/pivot-react/src/index.ts"] }As per coding guidelines, "Import UI components from the barrel file at
src/components/ui."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vite.config.ts` around lines 8 - 13, Vite's alias for '@ui' is set in resolve.alias but TypeScript paths only map '@ui/*', causing editor/TS resolution to fail for imports like "from '@ui'"; update the TS config paths to include a bare '@ui' mapping that points to the same target as '@ui/*' (the barrel at src/components/ui) so both Vite (alias '@ui') and TypeScript resolve the barrel import consistently; ensure the same barrel export file (src/components/ui/index.ts) is referenced by the '@ui' path entry.packages/pivot-react/package.json (1)
11-11: Move@types/papaparsetodevDependencies.Type definitions are development-time dependencies and should be in
devDependenciesrather thandependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-react/package.json` at line 11, The package.json currently lists "@types/papaparse" under dependencies; move that entry into devDependencies instead. Remove the "@types/papaparse": "^5.5.2" line from the dependencies object and add the same key/value under the devDependencies object in the same package.json so type definitions are installed only for development.packages/pivot-engine/src/engine/analyzeData.ts (1)
13-16: Redundant null check after length guard.Line 13 already ensures
records.length > 0, sorecords[0]will always be defined (assuming the array wasn't mutated). Theif (!firstRecord)check on line 16 is unreachable under normal conditions.♻️ Proposed simplification
export function analyzeData(records: DataRecord[]): FieldInfo[] { if (records.length === 0) return [] const firstRecord = records[0] - if (!firstRecord) return [] const fieldNames = Object.keys(firstRecord)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-engine/src/engine/analyzeData.ts` around lines 13 - 16, The guard in analyzeData.ts redundantly checks firstRecord after already returning when records.length === 0; remove the unreachable if (!firstRecord) return [] and rely on the initial length check (keep usage of records and firstRecord as before) so code no longer performs the unnecessary null/undefined check for records[0].packages/pivot-react/src/ui/Modal.tsx (1)
31-63: Consider adding accessibility attributes.For better screen reader support, the modal dialog could benefit from:
role="dialog"andaria-modal="true"on the dialog container (line 39)aria-labelledbyreferencing the title when present- Focus trapping to keep keyboard focus within the modal
This is optional for an internal component but recommended if the package may be used externally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-react/src/ui/Modal.tsx` around lines 31 - 63, The modal lacks accessibility attributes and focus management; update the modal container (the element with ref={ref}) to include role="dialog" and aria-modal="true", add aria-labelledby that references the title element when title is provided (generate or use a stable id for the h2 rendered when title exists), and implement focus trapping and restore focus on close inside the Modal component (use overlayRef, ref, and onClose handlers to set initial focus to the modal or first focusable child, intercept Tab/Shift+Tab to keep focus inside, and return focus when the modal unmounts). Ensure these changes integrate with existing props, title, children, and onClose.packages/pivot-react/src/components/ConfigPanel.tsx (1)
164-173: Drop the memo/registry-size dependency here.
getAllAggregations()is cheap, andregisterAggregator()can overwrite an existing type without changing the map size. In that caseisDualFieldwill see the new plugin, butallAggregationscan stay cached with stale labels/options until the size changes.♻️ Suggested change
- const allAggregations = useMemo( - () => getAllAggregations(), - // Re-compute only if new plugins are registered (rare, so useMemo is stable) - // eslint-disable-next-line react-hooks/exhaustive-deps - [getRegisteredPlugins().size] - ) + const allAggregations = getAllAggregations()As per coding guidelines, "Use useMemo for expensive derived values"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-react/src/components/ConfigPanel.tsx` around lines 164 - 173, The memoization of allAggregations is unsafe because getAllAggregations() is cheap but can become stale when a plugin is overwritten without changing registry size; change the allAggregations declaration to call getAllAggregations() directly (remove useMemo and the size-based dependency and its eslint-disable comment) so labels/options update immediately, leaving isDualField logic (DUAL_FIELD_AGGREGATIONS, getRegisteredPlugins(), config.aggregation) unchanged.packages/pivot-react/src/ui/Checkbox.tsx (1)
26-33: Use a theme token for the checkmark color.
text-whitehardcodes the icon color inside a shared primitive, which makes the package harder to theme consistently.As per coding guidelines, "Use CSS variables as arbitrary Tailwind values (e.g.,
text-[var(--color-accent)]) instead of hardcoded colors"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-react/src/ui/Checkbox.tsx` around lines 26 - 33, The SVG in the Checkbox component hardcodes the checkmark color via the className "text-white"; update the SVG's className in packages/pivot-react/src/ui/Checkbox.tsx (the <svg> inside the Checkbox component) to use a theme token CSS variable instead (e.g., replace "text-white" with an arbitrary Tailwind value like "text-[var(--color-accent)]" or a project-standard variable such as "--color-foreground") so the icon color is themeable; ensure the chosen CSS variable exists in the shared theme and that the class keeps the rest of the utility classes (absolute inset-0 w-4 h-4 opacity-0 peer-checked:opacity-100 pointer-events-none transition-opacity).packages/pivot-react/tsconfig.json (1)
18-20: Remove unused@ui/*alias that weakens the package boundary.The
@ui/*mapping points to../../src/components/ui/*(outside this package) and is not used anywhere inpackages/pivot-react/src. This alias creates an unnecessary cross-package boundary violation. Removing it is safe and aligns with the pattern of using relative imports instead of aliases.♻️ Suggested change
"paths": { - "@utils-foo/pivot-engine": ["../pivot-engine/src/index.ts"], - "@ui/*": ["../../src/components/ui/*"] + "@utils-foo/pivot-engine": ["../pivot-engine/src/index.ts"] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-react/tsconfig.json` around lines 18 - 20, The tsconfig "paths" entry defines an unused cross-package alias "@ui/*" pointing to "../../src/components/ui/*" which weakens package boundaries; remove the "@ui/*" mapping from the "paths" section in packages/pivot-react/tsconfig.json so imports use relative paths and the package boundary is preserved, leaving the "@utils-foo/pivot-engine" mapping intact.packages/pivot-engine/src/__tests__/aggregators.test.ts (1)
244-269: Test plugin'sclone()implementation is buggy (though not exercised).The clone implementation at lines 253-259 has issues: the cloned aggregator's
pushstill mutates the outersumvariable, andvalue()returns the stale captureds. Since the test doesn't callclone()on this plugin, it passes, but this could mislead developers copying the pattern.♻️ Correct clone implementation for reference
clone() { - const s = sum - return { - push(v: unknown) { sum += Number(v) * 2 }, - value() { return s }, - clone() { return agg.clone() }, - } + let clonedSum = sum + const cloned: Aggregator = { + push(v: unknown) { clonedSum += Number(v) * 2 }, + value() { return clonedSum }, + clone() { return cloned.clone() }, + } + return cloned },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-engine/src/__tests__/aggregators.test.ts` around lines 244 - 269, The Aggregator clone implementation returned by the test's factory (symbols: registerAggregator, createAggregator, Aggregator, agg, clone, sum) incorrectly shares/mutates the outer `sum` and returns a stale value `s`; fix by making clone capture the current sum into a new local variable and return a new aggregator instance whose push/value/clone operate on that new local so the cloned aggregator is independent (its push updates the clone's own sum, value returns that current sum, and clone returns another independent copy).packages/pivot-react/src/lib/parseCsv.ts (1)
16-20: Type parameter inconsistent withdynamicTyping: true.The generic
Record<string, string>doesn't reflect thatdynamicTyping: trueconverts numeric strings tonumberand boolean strings toboolean. This creates a type mismatch, though it's mitigated by the cast on line 34.♻️ Consider using a more accurate type
- const result = Papa.parse<Record<string, string>>(csvText.trim(), { + const result = Papa.parse<Record<string, unknown>>(csvText.trim(), { header: true, skipEmptyLines: true, dynamicTyping: true, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-react/src/lib/parseCsv.ts` around lines 16 - 20, The generic type passed to Papa.parse (currently Papa.parse<Record<string, string>> in parseCsv) conflicts with dynamicTyping: true because parsed field values may be number/boolean/null; update the parse generic to a broader type such as Record<string, string|number|boolean|null> or Record<string, unknown> and adjust/remove the subsequent cast that masks the mismatch (the cast used after result parsing). Ensure the change is applied where Papa.parse is invoked and any downstream uses of the parsed row type (e.g., variables referencing result.data or the parseCsv return) are updated to the new, accurate type.packages/pivot-engine/src/engine/aggregators.ts (1)
325-333: Minor inconsistency in property checking approach.Line 326 uses
plugin.type in AGGREGATOR_FACTORIESwhilecreateAggregator(line 346) usesObject.prototype.hasOwnProperty.call(). Both work correctly for this plain object, but for consistency, consider using the same approach in both places.♻️ Optional: Align property checking style
export function registerAggregator(plugin: AggregatorPlugin): void { - if (plugin.type in AGGREGATOR_FACTORIES) { + if (Object.prototype.hasOwnProperty.call(AGGREGATOR_FACTORIES, plugin.type)) { throw new Error(`[pivot-engine] Cannot override built-in aggregation type: "${plugin.type}"`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pivot-engine/src/engine/aggregators.ts` around lines 325 - 333, In registerAggregator replace the property check that uses the "in" operator with the same hasOwnProperty style used in createAggregator: use Object.prototype.hasOwnProperty.call(AGGREGATOR_FACTORIES, plugin.type) to test for built-in types before throwing; keep the rest of the function (console.warn and pluginRegistry.set) unchanged and ensure you reference registerAggregator, AGGREGATOR_FACTORIES, and plugin.type when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/pivot-react/package.json`:
- Around line 9-13: The package is missing runtime deps used by
packages/pivot-react/src/lib/utils.ts: add "clsx" and "tailwind-merge" to the
"dependencies" section of packages/pivot-react/package.json so external
consumers can resolve those imports; update the dependencies object to include
"clsx": "<appropriate-version>" and "tailwind-merge": "<appropriate-version>"
(choose compatible versions consistent with the monorepo/root) and run install
to verify no unresolved imports remain.
In `@packages/pivot-react/src/components/FilterModal.tsx`:
- Around line 104-109: The two action buttons in FilterModal.tsx (the elements
wired to handleSelectAll and handleSelectNone) are missing explicit types and
will default to type="submit"; update both button elements to include
type="button" to prevent accidental form submission when this component is
rendered inside a form, ensuring the buttons remain plain action triggers tied
to handleSelectAll and handleSelectNone.
In `@packages/pivot-react/src/ui/Modal.tsx`:
- Around line 50-57: The close button in Modal (the button element rendering the
SVG in Modal.tsx) is missing an explicit type and will act as type="submit"
inside forms; update the close button in the Modal component to include
type="button" so it does not trigger form submission when clicked (apply the
same fix used for the Button component).
In `@packages/pivot-react/src/ui/Toggle.tsx`:
- Around line 4-6: ToggleProps currently allows rendering a checkbox without an
accessible name; change ToggleProps to a discriminated union that requires at
least one accessible name: either label (string) OR aria-label (string) OR
aria-labelledby (string). Specifically, replace the single interface with a
union like (props with label: string) | (props without label but with required
'aria-label') | (props without label but with required 'aria-labelledby'), keep
Omit<InputHTMLAttributes<HTMLInputElement>, 'type'> as the base for each branch,
and ensure the Toggle component's props type uses this new ToggleProps so
TypeScript enforces callers provide one of those name properties. Also update
any usages that fail the new type to pass label or an aria prop accordingly.
---
Nitpick comments:
In `@packages/pivot-engine/src/__tests__/aggregators.test.ts`:
- Around line 244-269: The Aggregator clone implementation returned by the
test's factory (symbols: registerAggregator, createAggregator, Aggregator, agg,
clone, sum) incorrectly shares/mutates the outer `sum` and returns a stale value
`s`; fix by making clone capture the current sum into a new local variable and
return a new aggregator instance whose push/value/clone operate on that new
local so the cloned aggregator is independent (its push updates the clone's own
sum, value returns that current sum, and clone returns another independent
copy).
In `@packages/pivot-engine/src/engine/aggregators.ts`:
- Around line 325-333: In registerAggregator replace the property check that
uses the "in" operator with the same hasOwnProperty style used in
createAggregator: use Object.prototype.hasOwnProperty.call(AGGREGATOR_FACTORIES,
plugin.type) to test for built-in types before throwing; keep the rest of the
function (console.warn and pluginRegistry.set) unchanged and ensure you
reference registerAggregator, AGGREGATOR_FACTORIES, and plugin.type when making
the change.
In `@packages/pivot-engine/src/engine/analyzeData.ts`:
- Around line 13-16: The guard in analyzeData.ts redundantly checks firstRecord
after already returning when records.length === 0; remove the unreachable if
(!firstRecord) return [] and rely on the initial length check (keep usage of
records and firstRecord as before) so code no longer performs the unnecessary
null/undefined check for records[0].
In `@packages/pivot-react/package.json`:
- Line 11: The package.json currently lists "@types/papaparse" under
dependencies; move that entry into devDependencies instead. Remove the
"@types/papaparse": "^5.5.2" line from the dependencies object and add the same
key/value under the devDependencies object in the same package.json so type
definitions are installed only for development.
In `@packages/pivot-react/src/components/ConfigPanel.tsx`:
- Around line 164-173: The memoization of allAggregations is unsafe because
getAllAggregations() is cheap but can become stale when a plugin is overwritten
without changing registry size; change the allAggregations declaration to call
getAllAggregations() directly (remove useMemo and the size-based dependency and
its eslint-disable comment) so labels/options update immediately, leaving
isDualField logic (DUAL_FIELD_AGGREGATIONS, getRegisteredPlugins(),
config.aggregation) unchanged.
In `@packages/pivot-react/src/lib/parseCsv.ts`:
- Around line 16-20: The generic type passed to Papa.parse (currently
Papa.parse<Record<string, string>> in parseCsv) conflicts with dynamicTyping:
true because parsed field values may be number/boolean/null; update the parse
generic to a broader type such as Record<string, string|number|boolean|null> or
Record<string, unknown> and adjust/remove the subsequent cast that masks the
mismatch (the cast used after result parsing). Ensure the change is applied
where Papa.parse is invoked and any downstream uses of the parsed row type
(e.g., variables referencing result.data or the parseCsv return) are updated to
the new, accurate type.
In `@packages/pivot-react/src/ui/Checkbox.tsx`:
- Around line 26-33: The SVG in the Checkbox component hardcodes the checkmark
color via the className "text-white"; update the SVG's className in
packages/pivot-react/src/ui/Checkbox.tsx (the <svg> inside the Checkbox
component) to use a theme token CSS variable instead (e.g., replace "text-white"
with an arbitrary Tailwind value like "text-[var(--color-accent)]" or a
project-standard variable such as "--color-foreground") so the icon color is
themeable; ensure the chosen CSS variable exists in the shared theme and that
the class keeps the rest of the utility classes (absolute inset-0 w-4 h-4
opacity-0 peer-checked:opacity-100 pointer-events-none transition-opacity).
In `@packages/pivot-react/src/ui/Modal.tsx`:
- Around line 31-63: The modal lacks accessibility attributes and focus
management; update the modal container (the element with ref={ref}) to include
role="dialog" and aria-modal="true", add aria-labelledby that references the
title element when title is provided (generate or use a stable id for the h2
rendered when title exists), and implement focus trapping and restore focus on
close inside the Modal component (use overlayRef, ref, and onClose handlers to
set initial focus to the modal or first focusable child, intercept Tab/Shift+Tab
to keep focus inside, and return focus when the modal unmounts). Ensure these
changes integrate with existing props, title, children, and onClose.
In `@packages/pivot-react/tsconfig.json`:
- Around line 18-20: The tsconfig "paths" entry defines an unused cross-package
alias "@ui/*" pointing to "../../src/components/ui/*" which weakens package
boundaries; remove the "@ui/*" mapping from the "paths" section in
packages/pivot-react/tsconfig.json so imports use relative paths and the package
boundary is preserved, leaving the "@utils-foo/pivot-engine" mapping intact.
In `@vite.config.ts`:
- Around line 8-13: Vite's alias for '@ui' is set in resolve.alias but
TypeScript paths only map '@ui/*', causing editor/TS resolution to fail for
imports like "from '@ui'"; update the TS config paths to include a bare '@ui'
mapping that points to the same target as '@ui/*' (the barrel at
src/components/ui) so both Vite (alias '@ui') and TypeScript resolve the barrel
import consistently; ensure the same barrel export file
(src/components/ui/index.ts) is referenced by the '@ui' path entry.
🪄 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: 7f075bf6-9c2a-48a3-9f01-2bcd67147b2e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (34)
package.jsonpackages/pivot-engine/package.jsonpackages/pivot-engine/src/__tests__/PivotEngine.test.tspackages/pivot-engine/src/__tests__/aggregators.test.tspackages/pivot-engine/src/__tests__/sorters.test.tspackages/pivot-engine/src/engine/PivotEngine.tspackages/pivot-engine/src/engine/aggregators.tspackages/pivot-engine/src/engine/analyzeData.tspackages/pivot-engine/src/engine/sorters.tspackages/pivot-engine/src/index.tspackages/pivot-engine/src/types.tspackages/pivot-engine/tsconfig.jsonpackages/pivot-engine/vitest.config.tspackages/pivot-react/package.jsonpackages/pivot-react/src/components/ConfigPanel.tsxpackages/pivot-react/src/components/DataInput.tsxpackages/pivot-react/src/components/FilterModal.tsxpackages/pivot-react/src/components/PivotGrid.tsxpackages/pivot-react/src/components/PivotTable.tsxpackages/pivot-react/src/hooks/usePivotData.tspackages/pivot-react/src/index.tspackages/pivot-react/src/lib/parseCsv.tspackages/pivot-react/src/lib/utils.tspackages/pivot-react/src/ui/Button.tsxpackages/pivot-react/src/ui/Card.tsxpackages/pivot-react/src/ui/Checkbox.tsxpackages/pivot-react/src/ui/EmptyState.tsxpackages/pivot-react/src/ui/Modal.tsxpackages/pivot-react/src/ui/Toggle.tsxpackages/pivot-react/tsconfig.jsonsrc/tools/pivot-table/hooks/usePivotData.tssrc/tools/pivot-table/index.tsxtsconfig.jsonvite.config.ts
💤 Files with no reviewable changes (1)
- src/tools/pivot-table/hooks/usePivotData.ts
Comment on lines
+9
to
+13
| "dependencies": { | ||
| "@utils-foo/pivot-engine": "*", | ||
| "@types/papaparse": "^5.5.2", | ||
| "papaparse": "^5.5.3" | ||
| }, |
There was a problem hiding this comment.
Missing clsx and tailwind-merge dependencies.
packages/pivot-react/src/lib/utils.ts imports clsx and tailwind-merge, but neither is declared in this package's dependencies. While hoisting from the root package.json works during local development, external consumers of this package would encounter missing module errors.
🐛 Proposed fix
"dependencies": {
"@utils-foo/pivot-engine": "*",
- "@types/papaparse": "^5.5.2",
- "papaparse": "^5.5.3"
+ "clsx": "^2.1.1",
+ "papaparse": "^5.5.3",
+ "tailwind-merge": "^3.5.0"
+ },
+ "devDependencies": {
+ "@types/papaparse": "^5.5.2"
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pivot-react/package.json` around lines 9 - 13, The package is
missing runtime deps used by packages/pivot-react/src/lib/utils.ts: add "clsx"
and "tailwind-merge" to the "dependencies" section of
packages/pivot-react/package.json so external consumers can resolve those
imports; update the dependencies object to include "clsx":
"<appropriate-version>" and "tailwind-merge": "<appropriate-version>" (choose
compatible versions consistent with the monorepo/root) and run install to verify
no unresolved imports remain.
Comment on lines
+104
to
109
| <button onClick={handleSelectAll} className="text-[var(--color-accent)] hover:underline"> | ||
| Select All | ||
| </button> | ||
| <span className="text-[var(--color-ink-muted)]">|</span> | ||
| <button | ||
| onClick={handleSelectNone} | ||
| className="text-[var(--color-accent)] hover:underline" | ||
| > | ||
| <button onClick={handleSelectNone} className="text-[var(--color-accent)] hover:underline"> | ||
| Select None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine the relevant section
head -120 packages/pivot-react/src/components/FilterModal.tsx | tail -30Repository: midplane/utils.foo
Length of output: 1531
🏁 Script executed:
# Also check the full context around lines 104-109 with more lines above/below
sed -n '90,115p' packages/pivot-react/src/components/FilterModal.tsxRepository: midplane/utils.foo
Length of output: 1234
🏁 Script executed:
# Check the beginning of the file to understand structure and if it's in a form
head -70 packages/pivot-react/src/components/FilterModal.tsxRepository: midplane/utils.foo
Length of output: 2037
🏁 Script executed:
# Check the Modal component implementation
cat packages/pivot-react/src/components/ui/Modal.tsxRepository: midplane/utils.foo
Length of output: 142
🏁 Script executed:
# Check if Button component is available and what parameters it accepts
cat packages/pivot-react/src/components/ui/Button.tsxRepository: midplane/utils.foo
Length of output: 143
🏁 Script executed:
# Find the correct path for UI components
find . -name "Modal*" -o -name "Button*" | head -20Repository: midplane/utils.foo
Length of output: 203
🏁 Script executed:
# List the structure of components directory
ls -la src/components/ui/ 2>/dev/null || find . -type d -name "ui" | head -5Repository: midplane/utils.foo
Length of output: 1900
🏁 Script executed:
# Check the actual file structure
git ls-files | grep -i "modal\|button" | grep -E "\.(tsx|ts)$"Repository: midplane/utils.foo
Length of output: 280
🏁 Script executed:
# Check the Modal component in the same package
cat packages/pivot-react/src/ui/Modal.tsxRepository: midplane/utils.foo
Length of output: 2336
🏁 Script executed:
# Check the Button component in the same package
cat packages/pivot-react/src/ui/Button.tsxRepository: midplane/utils.foo
Length of output: 1456
🏁 Script executed:
# Verify the import path in FilterModal
sed -n '1,10p' packages/pivot-react/src/components/FilterModal.tsxRepository: midplane/utils.foo
Length of output: 445
Add type="button" to these action buttons.
Lines 104 and 108 use raw <button> elements without explicit type attributes. Browsers default to type="submit", which will unintentionally submit any surrounding form if FilterModal is used inside a form context.
Suggested fix
- <button onClick={handleSelectAll} className="text-[var(--color-accent)] hover:underline">
+ <button type="button" onClick={handleSelectAll} className="text-[var(--color-accent)] hover:underline">
Select All
</button>
<span className="text-[var(--color-ink-muted)]">|</span>
- <button onClick={handleSelectNone} className="text-[var(--color-accent)] hover:underline">
+ <button type="button" onClick={handleSelectNone} className="text-[var(--color-accent)] hover:underline">
Select None
</button>📝 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.
Suggested change
| <button onClick={handleSelectAll} className="text-[var(--color-accent)] hover:underline"> | |
| Select All | |
| </button> | |
| <span className="text-[var(--color-ink-muted)]">|</span> | |
| <button | |
| onClick={handleSelectNone} | |
| className="text-[var(--color-accent)] hover:underline" | |
| > | |
| <button onClick={handleSelectNone} className="text-[var(--color-accent)] hover:underline"> | |
| Select None | |
| <button type="button" onClick={handleSelectAll} className="text-[var(--color-accent)] hover:underline"> | |
| Select All | |
| </button> | |
| <span className="text-[var(--color-ink-muted)]">|</span> | |
| <button type="button" onClick={handleSelectNone} className="text-[var(--color-accent)] hover:underline"> | |
| Select None |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pivot-react/src/components/FilterModal.tsx` around lines 104 - 109,
The two action buttons in FilterModal.tsx (the elements wired to handleSelectAll
and handleSelectNone) are missing explicit types and will default to
type="submit"; update both button elements to include type="button" to prevent
accidental form submission when this component is rendered inside a form,
ensuring the buttons remain plain action triggers tied to handleSelectAll and
handleSelectNone.
Comment on lines
+50
to
+57
| <button | ||
| onClick={onClose} | ||
| className="p-1 text-[var(--color-ink-muted)] hover:text-[var(--color-ink)] hover:bg-[var(--color-cream-dark)] rounded transition-colors" | ||
| > | ||
| <svg className="w-4 h-4" fill="none" stroke="currentColor" viewBox="0 0 24 24"> | ||
| <path strokeLinecap="round" strokeLinejoin="round" strokeWidth={2} d="M6 18L18 6M6 6l12 12" /> | ||
| </svg> | ||
| </button> |
There was a problem hiding this comment.
Add type="button" to prevent form submission.
The close button lacks an explicit type attribute. Inside a <form>, this defaults to type="submit", which could trigger unintended form submission. The PR's commit message mentions fixing this for the Button component—apply the same pattern here.
🐛 Proposed fix
<button
+ type="button"
onClick={onClose}
className="p-1 text-[var(--color-ink-muted)] hover:text-[var(--color-ink)] hover:bg-[var(--color-cream-dark)] rounded transition-colors"
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pivot-react/src/ui/Modal.tsx` around lines 50 - 57, The close button
in Modal (the button element rendering the SVG in Modal.tsx) is missing an
explicit type and will act as type="submit" inside forms; update the close
button in the Modal component to include type="button" so it does not trigger
form submission when clicked (apply the same fix used for the Button component).
Comment on lines
+4
to
+6
| export interface ToggleProps extends Omit<InputHTMLAttributes<HTMLInputElement>, 'type'> { | ||
| label?: string | ||
| } |
There was a problem hiding this comment.
Enforce an accessible name for unlabeled toggles.
label is optional, so this component can render a checkbox with no accessible name unless callers remember to pass aria-label/aria-labelledby. That’s an accessibility blocker for screen-reader flows.
♿ Suggested type-level guard
-export interface ToggleProps extends Omit<InputHTMLAttributes<HTMLInputElement>, 'type'> {
- label?: string
-}
+type ToggleAccessibleName =
+ | { label: string; 'aria-label'?: string; 'aria-labelledby'?: string }
+ | { label?: string; 'aria-label': string; 'aria-labelledby'?: string }
+ | { label?: string; 'aria-label'?: string; 'aria-labelledby': string }
+
+export type ToggleProps =
+ Omit<InputHTMLAttributes<HTMLInputElement>, 'type'> &
+ ToggleAccessibleNameAlso applies to: 8-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/pivot-react/src/ui/Toggle.tsx` around lines 4 - 6, ToggleProps
currently allows rendering a checkbox without an accessible name; change
ToggleProps to a discriminated union that requires at least one accessible name:
either label (string) OR aria-label (string) OR aria-labelledby (string).
Specifically, replace the single interface with a union like (props with label:
string) | (props without label but with required 'aria-label') | (props without
label but with required 'aria-labelledby'), keep
Omit<InputHTMLAttributes<HTMLInputElement>, 'type'> as the base for each branch,
and ensure the Toggle component's props type uses this new ToggleProps so
TypeScript enforces callers provide one of those name properties. Also update
any usages that fail the new type to pass label or an aria prop accordingly.
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
2 participants
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.
@utils-foo/pivot-engine: zero-dependency headless pivot engine withaggregators, sorters, PivotEngine class, and a plugin API for custom
aggregation types (
registerAggregator)@utils-foo/pivot-react: React component library (PivotTable, PivotGrid,ConfigPanel, DataInput, FilterModal) with PapaParse CSV support
source resolution (no separate build step)
src/tools/pivot-table/index.tsxto a thin wrapper around PivotTablehttps://claude.ai/code/session_01Gq6D8TLHT6eW3Gh8ZzFWa3
Summary by cubic
Extracted the pivot tool into
@utils-foo/pivot-engine(headless core with plugin API) and@utils-foo/pivot-react(React UI), and updated the app to use them via a thin wrapper. Adds data analysis helpers, heatmap utilities, and comprehensive Vitest coverage.Bug Fixes
Button: set default type="button" to avoid unintended form submits.usePivotData: allow grand‑total‑only pivots by removing the rows+cols==0 early return.Migration
@utils-foo/pivot-engineand@utils-foo/pivot-react.registerAggregator;ValueConfig.aggregationaccepts custom strings.getAllAggregations()/getAggregationLabel().@utils-foo/pivot-react:reactandlucide-react.npm run test --workspace=packages/pivot-engine(ortest:watch).Written for commit aca6fb6. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
pivot-engine) and React components (pivot-react).Tests
Chores