Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive rollup property type support across the dataview system, updates badge styling with subtle color variants, refactors filter/quantifier handling for display rollups, improves view components with className and layout options, implements server-side query key resolution, and adds a new SimpleDataViewProvider for in-memory data. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
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)
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 |
github-actions
bot
added
area: web
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (8)
packages/dataview/src/components/views/board-view/sticky-column-label.tsx (1)
165-165: Consider centralizing sticky z-index levels to avoid layering drift.At Line 165,
z-50now diverges from other sticky headers (z-40inpackages/dataview/src/components/views/table-view/data-table-sticky-header.tsx:143-152andpackages/dataview/src/components/views/sticky-group-label.tsx:42-57). Suggest using a shared z-index token/constant so stacking order is intentional and consistent across views.Based on learnings: "Applies to /{components,views}//*.{ts,tsx} : Prefer minimal Tailwind - avoid ad-hoc CSS".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/components/views/board-view/sticky-column-label.tsx` at line 165, The sticky column label uses an ad-hoc z-50 class (className="sticky z-50") which diverges from other sticky headers (z-40 in data-table-sticky-header and sticky-group-label); replace the literal Tailwind z value with a shared z-index token/export (e.g., STICKY_Z_INDEX or a shared class name) and update all usages (sticky-column-label.tsx, data-table-sticky-header.tsx, sticky-group-label.tsx) to import and use that token so stacking is consistent and centralized.packages/dataview/src/validators/valid-filter.ts (1)
48-54: Type casts are safe but could be cleaner.The
p as PropertyMetacasts on lines 51 and 75 are safe becausePropertyLikeincludes the fields thatisDisplayRollupandgetEffectiveTypeaccess (typeandconfig). However, consider whetherisDisplayRollupandgetEffectiveTypecould acceptPropertyLikeinstead ofPropertyMetato avoid the casts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/validators/valid-filter.ts` around lines 48 - 54, The cast noise can be removed by widening the parameter types of isDisplayRollup and getEffectiveType to accept PropertyLike instead of PropertyMeta; update those function signatures (isDisplayRollup(...) and getEffectiveType(...)) to use PropertyLike, ensure their internal code only accesses fields present on PropertyLike (type, config), then remove the explicit casts like in getDisplayRollupIds and any other callers so you call isDisplayRollup(p) and getEffectiveType(p) directly.packages/dataview/src/components/ui/pagination/page-pagination.tsx (1)
127-128: Consider adding aria-hidden to the sentinel element.The sentinel div is a visual detection mechanism and not meaningful content. Adding
aria-hidden="true"would prevent screen readers from announcing it.Suggested improvement
{/* Sentinel: when this is out of view, pagination is stuck to bottom */} - <div className="-mt-4" ref={sentinelRef} /> + <div aria-hidden="true" className="-mt-4" ref={sentinelRef} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/components/ui/pagination/page-pagination.tsx` around lines 127 - 128, The sentinel div used for intersection detection (referenced by sentinelRef in PagePagination) is nonsemantic and should be hidden from assistive tech; add aria-hidden="true" to the sentinel element (the <div ref={sentinelRef} />) so screen readers ignore it, optionally keeping className and other props unchanged.packages/dataview/src/parsers/filter.ts (1)
54-63: Non-primitive array values are silently dropped.When encoding array values, only primitives (string, number, boolean) are kept. Other value types (e.g., objects, nested arrays) are silently filtered out without warning. If this is intentional, consider adding a comment; if not, consider throwing or logging when unexpected types are encountered.
Optional: Add explicit handling for unexpected types
// Handle array values with parentheses wrapper if (Array.isArray(value)) { const primitiveArray = value.filter( (v): v is string | number | boolean => typeof v === "string" || typeof v === "number" || typeof v === "boolean" ); + // Note: Non-primitive values (objects, nested arrays) are intentionally + // excluded as the DSL only supports primitive array elements const arrayStr = encodeArrayValue(primitiveArray); encoded = `${baseParts.join(".")}.${arrayStr}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/parsers/filter.ts` around lines 54 - 63, The array-encoding branch in the Array.isArray(value) block (around the primitiveArray filtering before calling encodeArrayValue) silently drops non-primitive elements; detect those filtered-out items and surface a warning or error instead of silently ignoring them. Modify the code around the primitiveArray calculation (the filter that keeps typeof === "string"|"number"|"boolean") to compute nonPrimitives = value.filter(v => !(typeof v === "string"||typeof v === "number"||typeof v === "boolean")) and if nonPrimitives.length > 0 either console.warn (including baseParts join and a brief type list) or throw a descriptive Error; then continue to call encodeArrayValue(primitiveArray) as before. Ensure references to encodeArrayValue, primitiveArray and baseParts are used in the message so reviewers can locate the change.packages/dataview/src/components/views/table-view/index.tsx (1)
56-59:classNameis reaching each inner table, not the "table wrapper" from the prop contract.In grouped mode this gets applied once per expanded group's
DataTable; in flat mode it still bypasses the outer wrapper. Layout classes like padding, max-width, or margins will behave very differently from what the prop description promises. Consider puttingclassNameon the top-level container and using a separatetableClassNameif the inner table also needs styling.Also applies to: 328-347, 400-424
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/components/views/table-view/index.tsx` around lines 56 - 59, The prop `className` in the table-view component is being applied to each inner DataTable instead of the outer wrapper; change the prop contract so `className` is applied to the top-level container (the table wrapper) and introduce a new prop `tableClassName` (or similar) that is passed to the inner DataTable instances (both in flat and grouped rendering paths). Update the component props interface (replace/extend the current `className?: string` with the two props), replace usages where `className` is forwarded into `<DataTable>` (see the grouped render branch and flat render branch around the blocks previously at 328-347 and 400-424) to use `tableClassName`, and ensure the outermost wrapper element uses `className` so layout classes like padding/margins apply once. Also update any callers/tests that rely on the old behavior.packages/dataview/src/components/views/list-view/index.tsx (1)
26-29:classNamebehaves like a per-group content class, not a list-wrapper class.The new prop is documented as a wrapper class, but grouped rendering applies it inside each group's suspended content instead of on the outer list container. That makes spacing, border, and layout utilities repeat per group. Consider moving it to the top-level wrapper or renaming it to match the actual target.
Also applies to: 165-181, 215-243, 298-307
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/components/views/list-view/index.tsx` around lines 26 - 29, The prop "className" on the ListView component is documented as a list-wrapper class but is actually applied inside each group's suspended content; either move its usage to the top-level wrapper element in the ListView JSX so it applies once to the outer container (update the JSX that renders the root list wrapper to accept className and remove it from per-group suspended content), or rename the prop to something like "groupContentClassName" (and update the interface/type declaration and docs) to reflect that it targets each group's inner content; also update all places that currently apply className inside grouped render logic (the per-group suspended/content render functions) to use the new name if renaming.packages/dataview/src/types/filter.type.ts (1)
35-35: RenamequantifierValuesto match the repo constant convention.This is a new exported constant, so it's a good time to align it with the existing naming rule before the lower-camel name spreads further.
♻️ Suggested rename
-export const quantifierValues = ["any", "none", "every"] as const; +export const QUANTIFIER_VALUES = ["any", "none", "every"] as const; ... - quantifier: z.enum(quantifierValues).optional(), + quantifier: z.enum(QUANTIFIER_VALUES).optional(),As per coding guidelines, "Use UPPER_SNAKE_CASE for constants".
Also applies to: 126-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/types/filter.type.ts` at line 35, Rename the exported constant quantifierValues to UPPER_SNAKE_CASE (QUANTIFIER_VALUES) and update every usage/import to the new name; keep the value and "as const" typing intact (i.e., change export const quantifierValues = ["any","none","every"] as const; to export const QUANTIFIER_VALUES = ["any","none","every"] as const;), and similarly rename any other occurrences referenced in this file (and around line 126) so all references (including types or destructurings) point to QUANTIFIER_VALUES.packages/dataview/src/lib/providers/simple-data-view.tsx (1)
66-71: Prefer an asyncqueryFnhere.This works, but it drifts from the repo's async convention and makes the static query path harder to keep consistent with the rest of the providers.
♻️ Suggested cleanup
const dataQuery = useCallback( () => ({ queryKey, - queryFn: () => Promise.resolve(staticResponse), + queryFn: async () => staticResponse, initialData: staticResponse, }), [queryKey, staticResponse] );As per coding guidelines, "Any function returning a Promise must be
async."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/dataview/src/lib/providers/simple-data-view.tsx` around lines 66 - 71, The queryFn in the dataQuery useCallback is returning Promise.resolve(staticResponse) instead of being declared async; update the queryFn to be an async function that simply returns staticResponse (i.e., make queryFn async () => staticResponse) so it follows the repo convention for any function returning a Promise and stays consistent with dataQuery, queryKey, staticResponse, and initialData handling. Ensure signatures and types around dataQuery/useCallback remain unchanged.
🤖 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/dataview/src/components/ui/filter/condition-picker.tsx`:
- Around line 37-40: The selection of filter condition items silently falls back
when propertyType === "rollup" but property is undefined; update the logic that
assigns items (using property, propertyType, getFilterConditionsForProperty,
getFilterConditions) to explicitly guard this case and throw an error (or
assert) instead of calling getFilterConditions(propertyType); in practice, check
if propertyType === "rollup" and if !property then throw a clear error
mentioning the missing property, otherwise call
getFilterConditionsForProperty(property), and keep the existing branch that
calls getFilterConditions(propertyType) for non-rollup types.
In `@packages/dataview/src/components/ui/group-section.tsx`:
- Around line 101-132: Add upfront guards: verify config.scale is finite and
non-zero (e.g., check scale !== 0 && Number.isFinite(scale)) and early-return or
throw a descriptive error if it isn't before computing scaled; make the "number"
format an explicit case in the switch (instead of falling through to default),
and change the switch default to throw an Error for unsupported numberFormat
values; reference the existing variables numberFormat, scale, config and the
scaled calculation so the validation runs before computing scaled and before
formatting branches.
In `@packages/dataview/src/components/views/data-cell.tsx`:
- Around line 59-73: The current switch groups "status" with
"select"/"multiSelect" and renders rolled-up values using SelectProperty
(rollupConfig.type), which loses status-specific visuals and empty-state
handling; split "status" into its own case and render each item using the
StatusProperty component (instead of SelectProperty), passing the appropriate
props (value, any status options from rollupConfig.options, and flags like
showOriginal/showUnique) and preserving keys and empty-state behavior so
rolled-up statuses match regular StatusProperty rendering.
- Around line 140-142: The rollup rendering falls through to the default
TextProperty so email and phone lose their mailto:/tel: behavior; update the
switch in data-cell.tsx to add explicit "email" and "phone" cases (before the
default) that render the same components/interaction used for normal cells
(e.g., EmailProperty or PhoneProperty, or an anchor with "mailto:"/"tel:") using
items.join(", ") and the existing wrap prop, and ensure these cases respect
showOriginal/showUnique logic similar to other property types.
In `@packages/dataview/src/components/views/table-view/data-table.tsx`:
- Around line 218-226: The onClick handler currently casts e.target to
HTMLElement and calls .closest without ensuring e.target is an Element, which
can throw in edge cases; update the handler in the onClick callback (the block
that calls onRowClick?.(row.original)) to first check that e.target is an
Element (e.g., instanceof Element) before calling .closest, and only proceed to
lookup "input, button, a, [role='checkbox']" when that guard passes; if the
guard fails, skip the closest check and invoke onRowClick?.(row.original) as
intended.
In `@packages/dataview/src/lib/providers/index.ts`:
- Line 5: The new re-export of SimpleDataViewProvider from the barrel file
index.ts violates the no-barrel rule; remove the export line from
packages/dataview/src/lib/providers/index.ts and instead expose
SimpleDataViewProvider directly from its concrete module entrypoint (the module
that defines SimpleDataViewProvider, e.g., simple-data-view), then update any
callers to import SimpleDataViewProvider from that concrete module path
(./simple-data-view) rather than from the index barrel; ensure no other barrel
re-exports are added and that ESLint/TS rules remain enabled.
In `@packages/dataview/src/lib/providers/query-bridge.tsx`:
- Around line 466-483: The server queries normalize filter and sort via
idToKeyMap (built by buildIdToKeyMap) but do not normalize search.searchFields
or property references in group/column configs, so stale property IDs are sent;
update PageQueryBridge and InfiniteQueryBridge to map/resolve all
property-ID-bearing structures (search.searchFields, group, column) with
idToKeyMap before passing into groupQuery/columnQuery/runtime context—either add
a resolveSearchKeys (or reuse resolveX style helper) that converts
search.searchFields IDs to keys using idToKeyMap and likewise normalize group
and column property IDs, and then use those normalized objects in place of the
originals when calling the server helpers.
In `@packages/dataview/src/lib/providers/simple-data-view.tsx`:
- Around line 77-81: The hardcoded limit of 100 in mergedDefaults causes
pagination for in-memory datasets over 100 rows; update the mergedDefaults
construction in simple-data-view.tsx to set limit dynamically using the input
data length (e.g., use data.length) but clamp it to the supported max if
necessary so it never exceeds the provider's max page limit; modify the
mergedDefaults assignment (symbol: mergedDefaults, type: DefaultsConfig) to
compute limit = Math.min(data.length, MAX_LIMIT) or similar instead of limit:
100, leaving other defaults spread from defaults intact so DataViewProvider will
render all in-memory rows on a single page.
In `@packages/dataview/src/types/property.type.ts`:
- Around line 543-546: The RollupPropertyType currently inherits an optional key
from BaseProperty which allows code like const key = p.key ?? p.id (and
subsequent use in building server extras queries) to silently fall back to id;
change RollupPropertyType to require key (i.e., make key non-optional) so the
compiler forces callers to supply a real backend field path, update the type
definition for RollupPropertyType to include key: string (or the same key type
used in BaseProperty but required), and run the typechecker to catch any call
sites that must be updated (look for usages of RollupPropertyType, RollupConfig,
and the p.key ?? p.id pattern to fix places that relied on the optional key).
- Around line 474-478: Change the public API types to return ReactNode instead
of any: update the PropertyRenderFunction return type and the optional method
value? (both in types/property.type.ts) to ReactNode, remove the biome-ignore
lint suppression comments, and add the React import (e.g. import { ReactNode }
from 'react') so the types reference ReactNode.
- Around line 250-269: Replace the broad RollupConfig interface with a
discriminated union of specific variants so invalid combinations are impossible:
create a NumberRollupConfig (calculation in
{"sum","average","median","min","max","range"}, type: "number", allowed numeric
display fields decimalPlaces/numberFormat/scale/showAs), a DateRollupConfig
(calculation appropriate for dates, type: "date", allowed
dateFormat/timeFormat), a SelectRollupConfig (type: "select"|"status", allowed
options), an ArrayRollupConfig (for array results with its valid display
fields), and an OtherScalarRollupConfig for scalar passthroughs like
showOriginal/showUnique; keep RollupCalculation and PropertyType usages and
preserve the Exclude<PropertyType,"formula"|"button"|"rollup"> constraint on
each variant, removing fields that don’t apply to a variant so the type system
enforces valid combinations.
In `@packages/dataview/src/utils/badge-colors.ts`:
- Around line 14-22: The maps produced by buildMap currently use
Record<string,string>, losing exhaustiveness and allowing getters to fall back
to gray; change buildMap usage so that COLOR_CLASSES, BG_CLASSES, BG_VARS and
FOREGROUND_VARS are declared with "satisfies Record<BadgeColor, string>"
(matching the pattern used by BG_TRANSPARENT_CLASSES and TEXT_COLORS) to enforce
completeness, adjust buildMap's typing as needed to produce a BadgeColor-keyed
map, and then update the badge getter functions to throw a descriptive error
when a requested BadgeColor key is missing instead of using the "??" gray
fallback; reference buildMap, COLOR_CLASSES, BG_CLASSES, BG_VARS,
FOREGROUND_VARS, BG_TRANSPARENT_CLASSES, TEXT_COLORS and the badge getter
functions in your changes.
In `@packages/dataview/src/utils/filter.ts`:
- Around line 42-45: The code currently treats a bare "rollup" type as text by
returning DATA_TABLE_CONFIG.textConditions; instead, update the switch branch
that handles case "rollup" (the branch that runs when you don't have a full
property) to fail fast: throw a clear error (e.g., "rollup property context
required") rather than silently returning text conditions, and do the same for
the other analogous branch that currently defaults to
DATA_TABLE_CONFIG.textConditions; callers that have full property metadata
should continue to use getFilterConditionsForProperty(), and downstream
validators like isValidConditionForPropertyType(...) will then only receive
properly-typed conditions.
---
Nitpick comments:
In `@packages/dataview/src/components/ui/pagination/page-pagination.tsx`:
- Around line 127-128: The sentinel div used for intersection detection
(referenced by sentinelRef in PagePagination) is nonsemantic and should be
hidden from assistive tech; add aria-hidden="true" to the sentinel element (the
<div ref={sentinelRef} />) so screen readers ignore it, optionally keeping
className and other props unchanged.
In `@packages/dataview/src/components/views/board-view/sticky-column-label.tsx`:
- Line 165: The sticky column label uses an ad-hoc z-50 class (className="sticky
z-50") which diverges from other sticky headers (z-40 in
data-table-sticky-header and sticky-group-label); replace the literal Tailwind z
value with a shared z-index token/export (e.g., STICKY_Z_INDEX or a shared class
name) and update all usages (sticky-column-label.tsx,
data-table-sticky-header.tsx, sticky-group-label.tsx) to import and use that
token so stacking is consistent and centralized.
In `@packages/dataview/src/components/views/list-view/index.tsx`:
- Around line 26-29: The prop "className" on the ListView component is
documented as a list-wrapper class but is actually applied inside each group's
suspended content; either move its usage to the top-level wrapper element in the
ListView JSX so it applies once to the outer container (update the JSX that
renders the root list wrapper to accept className and remove it from per-group
suspended content), or rename the prop to something like "groupContentClassName"
(and update the interface/type declaration and docs) to reflect that it targets
each group's inner content; also update all places that currently apply
className inside grouped render logic (the per-group suspended/content render
functions) to use the new name if renaming.
In `@packages/dataview/src/components/views/table-view/index.tsx`:
- Around line 56-59: The prop `className` in the table-view component is being
applied to each inner DataTable instead of the outer wrapper; change the prop
contract so `className` is applied to the top-level container (the table
wrapper) and introduce a new prop `tableClassName` (or similar) that is passed
to the inner DataTable instances (both in flat and grouped rendering paths).
Update the component props interface (replace/extend the current `className?:
string` with the two props), replace usages where `className` is forwarded into
`<DataTable>` (see the grouped render branch and flat render branch around the
blocks previously at 328-347 and 400-424) to use `tableClassName`, and ensure
the outermost wrapper element uses `className` so layout classes like
padding/margins apply once. Also update any callers/tests that rely on the old
behavior.
In `@packages/dataview/src/lib/providers/simple-data-view.tsx`:
- Around line 66-71: The queryFn in the dataQuery useCallback is returning
Promise.resolve(staticResponse) instead of being declared async; update the
queryFn to be an async function that simply returns staticResponse (i.e., make
queryFn async () => staticResponse) so it follows the repo convention for any
function returning a Promise and stays consistent with dataQuery, queryKey,
staticResponse, and initialData handling. Ensure signatures and types around
dataQuery/useCallback remain unchanged.
In `@packages/dataview/src/parsers/filter.ts`:
- Around line 54-63: The array-encoding branch in the Array.isArray(value) block
(around the primitiveArray filtering before calling encodeArrayValue) silently
drops non-primitive elements; detect those filtered-out items and surface a
warning or error instead of silently ignoring them. Modify the code around the
primitiveArray calculation (the filter that keeps typeof ===
"string"|"number"|"boolean") to compute nonPrimitives = value.filter(v =>
!(typeof v === "string"||typeof v === "number"||typeof v === "boolean")) and if
nonPrimitives.length > 0 either console.warn (including baseParts join and a
brief type list) or throw a descriptive Error; then continue to call
encodeArrayValue(primitiveArray) as before. Ensure references to
encodeArrayValue, primitiveArray and baseParts are used in the message so
reviewers can locate the change.
In `@packages/dataview/src/types/filter.type.ts`:
- Line 35: Rename the exported constant quantifierValues to UPPER_SNAKE_CASE
(QUANTIFIER_VALUES) and update every usage/import to the new name; keep the
value and "as const" typing intact (i.e., change export const quantifierValues =
["any","none","every"] as const; to export const QUANTIFIER_VALUES =
["any","none","every"] as const;), and similarly rename any other occurrences
referenced in this file (and around line 126) so all references (including types
or destructurings) point to QUANTIFIER_VALUES.
In `@packages/dataview/src/validators/valid-filter.ts`:
- Around line 48-54: The cast noise can be removed by widening the parameter
types of isDisplayRollup and getEffectiveType to accept PropertyLike instead of
PropertyMeta; update those function signatures (isDisplayRollup(...) and
getEffectiveType(...)) to use PropertyLike, ensure their internal code only
accesses fields present on PropertyLike (type, config), then remove the explicit
casts like in getDisplayRollupIds and any other callers so you call
isDisplayRollup(p) and getEffectiveType(p) directly.
🪄 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: 0d2a3879-81e4-4ee8-9dbc-e1d87327e93c
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (45)
apps/web/src/components/client-providers/index.tsxapps/web/src/modules/dataview/product-list-properties.tsxapps/web/src/modules/dataview/product-properties.tsxapps/web/src/modules/dataview/product-table-properties.tsxpackage.jsonpackages/dataview/src/components/ui/badge.tsxpackages/dataview/src/components/ui/filter/advanced-filter-rule.tsxpackages/dataview/src/components/ui/filter/condition-picker.tsxpackages/dataview/src/components/ui/filter/properties/status-filter.tsxpackages/dataview/src/components/ui/filter/properties/text-filter.tsxpackages/dataview/src/components/ui/filter/quantifier-picker.tsxpackages/dataview/src/components/ui/filter/simple-filter-editor.tsxpackages/dataview/src/components/ui/group-section.tsxpackages/dataview/src/components/ui/pagination/page-pagination.tsxpackages/dataview/src/components/ui/properties/formula-property.tsxpackages/dataview/src/components/ui/properties/multi-select-property.tsxpackages/dataview/src/components/ui/properties/number-property.tsxpackages/dataview/src/components/ui/properties/select-property.tsxpackages/dataview/src/components/ui/properties/status-property.tsxpackages/dataview/src/components/views/board-view/index.tsxpackages/dataview/src/components/views/board-view/sticky-column-label.tsxpackages/dataview/src/components/views/data-card.tsxpackages/dataview/src/components/views/data-cell.tsxpackages/dataview/src/components/views/gallery-view/index.tsxpackages/dataview/src/components/views/list-view/index.tsxpackages/dataview/src/components/views/list-view/list-row.tsxpackages/dataview/src/components/views/skeleton-widths.tspackages/dataview/src/components/views/sticky-group-label.tsxpackages/dataview/src/components/views/table-view/data-table.tsxpackages/dataview/src/components/views/table-view/index.tsxpackages/dataview/src/lib/property-icons.tspackages/dataview/src/lib/providers/index.tspackages/dataview/src/lib/providers/query-bridge.tsxpackages/dataview/src/lib/providers/simple-data-view.tsxpackages/dataview/src/parsers/filter.tspackages/dataview/src/types/filter.type.tspackages/dataview/src/types/index.tspackages/dataview/src/types/property.type.tspackages/dataview/src/utils/badge-colors.tspackages/dataview/src/utils/filter-builder.tspackages/dataview/src/utils/filter-variant.tspackages/dataview/src/utils/filter.tspackages/dataview/src/utils/get-badge-variant.tspackages/dataview/src/utils/transform-data.tspackages/dataview/src/validators/valid-filter.ts
💤 Files with no reviewable changes (1)
- packages/dataview/src/components/views/sticky-group-label.tsx
| const items = | ||
| property && property.type === "rollup" | ||
| ? getFilterConditionsForProperty(property) | ||
| : getFilterConditions(propertyType); |
There was a problem hiding this comment.
Fail fast when propertyType is rollup but property is missing.
At Line 40, rollup can silently fall back to getFilterConditions(propertyType) when property is undefined, which can surface incorrect condition sets. Add an explicit guard for this state.
Suggested fix
interface ConditionPickerProps {
@@
propertyType: PropertyType;
}
+function getConditionItems(
+ propertyType: PropertyType,
+ property?: PropertyMeta
+) {
+ if (propertyType === "rollup") {
+ if (!property) {
+ throw new Error(
+ "ConditionPicker requires `property` when propertyType is 'rollup'."
+ );
+ }
+ return getFilterConditionsForProperty(property);
+ }
+ return getFilterConditions(propertyType);
+}
+
function ConditionPicker({
@@
}: ConditionPickerProps) {
const [open, setOpen] = useState(false);
- const items =
- property && property.type === "rollup"
- ? getFilterConditionsForProperty(property)
- : getFilterConditions(propertyType);
+ const items = getConditionItems(propertyType, property);
const selected = items.find((item) => item.value === condition);As per coding guidelines: No silent fallbacks - code must fail immediately when conditions aren't met and Handle all known values explicitly in data mapping - throw for unknown values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/components/ui/filter/condition-picker.tsx` around lines
37 - 40, The selection of filter condition items silently falls back when
propertyType === "rollup" but property is undefined; update the logic that
assigns items (using property, propertyType, getFilterConditionsForProperty,
getFilterConditions) to explicitly guard this case and throw an error (or
assert) instead of calling getFilterConditions(propertyType); in practice, check
if propertyType === "rollup" and if !property then throw a clear error
mentioning the missing property, otherwise call
getFilterConditionsForProperty(property), and keep the existing branch that
calls getFilterConditions(propertyType) for non-rollup types.
| const { | ||
| numberFormat = "number", | ||
| decimalPlaces = 0, | ||
| scale = 1, | ||
| } = config ?? {}; | ||
| const scaled = scale === 1 ? value : value / scale; | ||
|
|
||
| switch (numberFormat) { | ||
| case "numberWithCommas": | ||
| return value.toLocaleString(undefined, { | ||
| return scaled.toLocaleString(undefined, { | ||
| minimumFractionDigits: decimalPlaces, | ||
| maximumFractionDigits: decimalPlaces, | ||
| }); | ||
| case "percentage": | ||
| return `${value.toFixed(decimalPlaces)}%`; | ||
| return `${scaled.toFixed(decimalPlaces)}%`; | ||
| case "dollar": | ||
| return `$${value.toLocaleString(undefined, { | ||
| return `$${scaled.toLocaleString(undefined, { | ||
| minimumFractionDigits: decimalPlaces, | ||
| maximumFractionDigits: decimalPlaces, | ||
| })}`; | ||
| case "euro": | ||
| return `€${value.toLocaleString(undefined, { | ||
| return `€${scaled.toLocaleString(undefined, { | ||
| minimumFractionDigits: decimalPlaces, | ||
| maximumFractionDigits: decimalPlaces, | ||
| })}`; | ||
| case "pound": | ||
| return `£${value.toLocaleString(undefined, { | ||
| return `£${scaled.toLocaleString(undefined, { | ||
| minimumFractionDigits: decimalPlaces, | ||
| maximumFractionDigits: decimalPlaces, | ||
| })}`; | ||
| default: | ||
| return value.toFixed(decimalPlaces); | ||
| return scaled.toFixed(decimalPlaces); |
There was a problem hiding this comment.
Fail fast on invalid number formatting config.
scale can now produce Infinity/NaN labels when it is 0 or non-finite, and the default branch still silently treats unknown numberFormat values as plain numbers. Add a guard for scale, make "number" explicit, and throw for unsupported formats.
Proposed fix
function formatNumber(value: number, config?: NumberConfig): string {
const {
numberFormat = "number",
decimalPlaces = 0,
scale = 1,
} = config ?? {};
+ if (!Number.isFinite(scale) || scale <= 0) {
+ throw new Error(`Invalid number scale: ${String(scale)}`);
+ }
const scaled = scale === 1 ? value : value / scale;
switch (numberFormat) {
+ case "number":
+ return scaled.toFixed(decimalPlaces);
case "numberWithCommas":
return scaled.toLocaleString(undefined, {
minimumFractionDigits: decimalPlaces,
maximumFractionDigits: decimalPlaces,
});
@@
- default:
- return scaled.toFixed(decimalPlaces);
+ default:
+ throw new Error(`Unsupported number format: ${String(numberFormat)}`);
}
}As per coding guidelines, "Handle errors up-front - use guard clauses and early returns", "No silent fallbacks - code must fail immediately when conditions aren't met", and "Handle all known values explicitly in data mapping - throw for unknown values".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/components/ui/group-section.tsx` around lines 101 -
132, Add upfront guards: verify config.scale is finite and non-zero (e.g., check
scale !== 0 && Number.isFinite(scale)) and early-return or throw a descriptive
error if it isn't before computing scaled; make the "number" format an explicit
case in the switch (instead of falling through to default), and change the
switch default to throw an Error for unsupported numberFormat values; reference
the existing variables numberFormat, scale, config and the scaled calculation so
the validation runs before computing scaled and before formatting branches.
| switch (rollupConfig.type) { | ||
| case "select": | ||
| case "multiSelect": | ||
| case "status": | ||
| return ( | ||
| <div className="flex flex-wrap gap-1"> | ||
| {items.map((v, i) => ( | ||
| <SelectProperty | ||
| config={{ options: rollupConfig.options ?? [] }} | ||
| key={`${String(v)}-${i}`} | ||
| value={v as string | null} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
status rollups bypass the dedicated status renderer.
Regular status cells go through StatusProperty, but showOriginal/showUnique currently funnel status through SelectProperty. That drops any status-specific visuals or empty-state handling from rolled-up values. Please split status into its own branch here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/components/views/data-cell.tsx` around lines 59 - 73,
The current switch groups "status" with "select"/"multiSelect" and renders
rolled-up values using SelectProperty (rollupConfig.type), which loses
status-specific visuals and empty-state handling; split "status" into its own
case and render each item using the StatusProperty component (instead of
SelectProperty), passing the appropriate props (value, any status options from
rollupConfig.options, and flags like showOriginal/showUnique) and preserving
keys and empty-state behavior so rolled-up statuses match regular StatusProperty
rendering.
| // text, email, phone — render as comma-separated text | ||
| default: | ||
| return <TextProperty value={items.join(", ")} wrap={wrap} />; |
There was a problem hiding this comment.
Email and phone rollups become plain text here.
For showOriginal/showUnique, email and phone fall through to TextProperty, so mailto: / tel: behavior disappears for rolled-up values even though normal cells keep it. Add explicit email and phone cases to preserve the same interaction model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/components/views/data-cell.tsx` around lines 140 - 142,
The rollup rendering falls through to the default TextProperty so email and
phone lose their mailto:/tel: behavior; update the switch in data-cell.tsx to
add explicit "email" and "phone" cases (before the default) that render the same
components/interaction used for normal cells (e.g., EmailProperty or
PhoneProperty, or an anchor with "mailto:"/"tel:") using items.join(", ") and
the existing wrap prop, and ensure these cases respect showOriginal/showUnique
logic similar to other property types.
| onClick={(e) => { | ||
| const el = (e.target as HTMLElement).closest( | ||
| "input, button, a, [role='checkbox']" | ||
| ); | ||
| if (el) { | ||
| return; | ||
| } | ||
| onRowClick?.(row.original); | ||
| }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify where HTMLElement-cast + closest is used without explicit Element narrowing.
rg -n -C2 '\(e\.target as HTMLElement\)\.closest' --type=tsx
rg -n -C2 'instanceof Element' packages/dataview/src/components/views/table-view/data-table.tsxRepository: jingerpie/ocean-dataview
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Verify the actual code and search patterns with corrected rg syntax
# Read the specific file and lines to confirm the code
echo "=== Checking lines 218-226 in data-table.tsx ==="
sed -n '218,226p' packages/dataview/src/components/views/table-view/data-table.tsx
echo ""
echo "=== Search for HTMLElement cast + closest pattern (no type filter) ==="
rg -n -C2 'as HTMLElement.*closest'
echo ""
echo "=== Search for instanceof Element checks in data-table.tsx ==="
rg -n 'instanceof Element' packages/dataview/src/components/views/table-view/data-table.tsxRepository: jingerpie/ocean-dataview
Length of output: 1870
Guard e.target before calling .closest().
This cast assumes e.target is always an HTMLElement. Add an Element guard first to avoid edge-case runtime failures.
Suggested fix
- onClick={(e) => {
- const el = (e.target as HTMLElement).closest(
- "input, button, a, [role='checkbox']"
- );
- if (el) {
- return;
- }
- onRowClick?.(row.original);
- }}
+ onClick={(e) => {
+ const target = e.target;
+ if (!(target instanceof Element)) {
+ return;
+ }
+ const el = target.closest(
+ "input, button, a, [role='checkbox']"
+ );
+ if (el) {
+ return;
+ }
+ onRowClick?.(row.original);
+ }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/components/views/table-view/data-table.tsx` around
lines 218 - 226, The onClick handler currently casts e.target to HTMLElement and
calls .closest without ensuring e.target is an Element, which can throw in edge
cases; update the handler in the onClick callback (the block that calls
onRowClick?.(row.original)) to first check that e.target is an Element (e.g.,
instanceof Element) before calling .closest, and only proceed to lookup "input,
button, a, [role='checkbox']" when that guard passes; if the guard fails, skip
the closest check and invoke onRowClick?.(row.original) as intended.
| export interface RollupConfig { | ||
| /** How to aggregate the related values */ | ||
| calculation: RollupCalculation; | ||
| /** Date display: format */ | ||
| dateFormat?: DateConfig["dateFormat"]; | ||
| /** Number display: decimal places (0-10) */ | ||
| decimalPlaces?: NumberConfig["decimalPlaces"]; | ||
| /** Number display: format */ | ||
| numberFormat?: NumberConfig["numberFormat"]; | ||
| /** Select/status display: option definitions for colored badges */ | ||
| options?: SelectOption[]; | ||
| /** Number display: divide stored value for display */ | ||
| scale?: NumberConfig["scale"]; | ||
| /** Number display: bar/ring visualization */ | ||
| showAs?: NumberConfig["showAs"]; | ||
| /** Date display: time format */ | ||
| timeFormat?: DateConfig["timeFormat"]; | ||
| /** Underlying field type in the related table */ | ||
| type: Exclude<PropertyType, "formula" | "button" | "rollup">; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the size and structure of the property.type.ts file
wc -l packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 116
🏁 Script executed:
# Read the relevant sections of the file to understand RollupConfig and related types
cat -n packages/dataview/src/types/property.type.ts | sed -n '200,300p'Repository: jingerpie/ocean-dataview
Length of output: 3504
🏁 Script executed:
# Also check the RollupCalculation type definition
cat -n packages/dataview/src/types/property.type.ts | sed -n '1,100p'Repository: jingerpie/ocean-dataview
Length of output: 3828
🏁 Script executed:
# Search for RollupConfig usage
rg "RollupConfig" --type ts --type tsx -B 2 -A 2Repository: jingerpie/ocean-dataview
Length of output: 95
🏁 Script executed:
# Search for rollup validation or constraint checking
rg -i "rollup.*calculation\|calculation.*type" --type ts --type tsx -B 2 -A 2 | head -100Repository: jingerpie/ocean-dataview
Length of output: 95
🏁 Script executed:
# Look for any validation or helper functions related to rollups
rg "isValidRollup|validateRollup|checkRollup" --type ts --type tsxRepository: jingerpie/ocean-dataview
Length of output: 95
🏁 Script executed:
# Search for RollupConfig usage with glob patterns
rg "RollupConfig" -g "*.ts" -g "*.tsx" -B 2 -A 2Repository: jingerpie/ocean-dataview
Length of output: 3870
🏁 Script executed:
# Search for rollup-related validation or constraint logic
rg "rollup" -g "*.ts" -g "*.tsx" --ignore-case -l | head -20Repository: jingerpie/ocean-dataview
Length of output: 868
🏁 Script executed:
# Look at the RollupPropertyType definition to see how RollupConfig is used
cat -n packages/dataview/src/types/property.type.ts | sed -n '500,600p'Repository: jingerpie/ocean-dataview
Length of output: 3557
🏁 Script executed:
# Look for more rollup helper functions and validation
rg "getScalarRollups|getRollupOutputType|calculation" packages/dataview/src/types/property.type.ts -B 2 -A 5Repository: jingerpie/ocean-dataview
Length of output: 3139
🏁 Script executed:
# Search for any validation or check functions that validate calculation/type combinations
rg "function.*[Rr]ollup|const.*[Rr]ollup" packages/dataview/src -g "*.ts" -g "*.tsx" -A 3Repository: jingerpie/ocean-dataview
Length of output: 3908
🏁 Script executed:
# Check data-cell.tsx to see how it handles rollup rendering
cat -n packages/dataview/src/components/views/data-cell.tsx | grep -A 50 "case \"rollup\""Repository: jingerpie/ocean-dataview
Length of output: 1633
🏁 Script executed:
# Check if there's any runtime validation that catches invalid calculation/type combinations
rg "throw|Error|invalid|Invalid" packages/dataview/src/types/property.type.ts | head -20Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Look at the RollupArrayDisplay function to see what display fields it uses
cat -n packages/dataview/src/components/views/data-cell.tsx | sed -n '200,260p'Repository: jingerpie/ocean-dataview
Length of output: 1850
🏁 Script executed:
# Search for where RollupConfig is created/validated to see if there's any runtime protection
rg "calculation.*:" packages/dataview/src -g "*.ts" -g "*.tsx" -B 2 -A 2 | grep -A 5 -B 5 "type.*:" | head -50Repository: jingerpie/ocean-dataview
Length of output: 1993
🏁 Script executed:
# Check if there are any test files that show valid/invalid rollup combinations
fd "rollup" packages/dataview/src -g "*.test.*" -g "*.spec.*"Repository: jingerpie/ocean-dataview
Length of output: 177
🏁 Script executed:
# See the complete RollupArrayDisplay implementation to understand what display fields it uses
rg "RollupArrayDisplay" packages/dataview/src/components/views/data-cell.tsx -A 50 | head -70Repository: jingerpie/ocean-dataview
Length of output: 1858
🏁 Script executed:
# Check if there's validation happening at server integration or data transformation level
rg "RollupConfig" packages/dataview/src/utils -g "*.ts" -g "*.tsx" -B 2 -A 2Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Look for where RollupConfig objects are constructed/created to see if validation happens
rg "type.*rollup\|config.*calculation" packages/dataview/src -g "*.ts" -g "*.tsx" -lRepository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Let me check if there's any schema validation or data normalization that validates rollup configs
rg "normaliz\|validat\|schema" packages/dataview/src -g "*.ts" -g "*.tsx" -l | head -10Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Check if there are any JSDoc examples that show all the valid combinations
cat -n packages/dataview/src/types/property.type.ts | sed -n '220,270p'Repository: jingerpie/ocean-dataview
Length of output: 1983
🏁 Script executed:
# Let me check how RollupConfig is actually used in the codebase - are configs hardcoded or loaded from data?
rg "config.*{.*calculation" packages/dataview/src -g "*.ts" -g "*.tsx" -B 2 -A 2Repository: jingerpie/ocean-dataview
Length of output: 181
🏁 Script executed:
# Check if there are any zod/validation schemas that validate the config
fd "schema\|zod\|validation" packages/dataview/src --type f -g "*.ts"Repository: jingerpie/ocean-dataview
Length of output: 118
🏁 Script executed:
# Look at the complete RollupArrayDisplay to see all switch cases and what happens with invalid type/calculation combos
sed -n '/function RollupArrayDisplay/,/^}/p' packages/dataview/src/components/views/data-cell.tsx | head -80Repository: jingerpie/ocean-dataview
Length of output: 135
🏁 Script executed:
# Let me search more carefully for RollupConfig usage
rg "RollupConfig" packages/dataview/src -g "*.ts" --max-count 20Repository: jingerpie/ocean-dataview
Length of output: 660
🏁 Script executed:
# Check what happens when RollupArrayDisplay gets an unexpected type
sed -n '300,400p' packages/dataview/src/components/views/data-cell.tsxRepository: jingerpie/ocean-dataview
Length of output: 248
🏁 Script executed:
# Let me look for any type guards or runtime checks that validate rollup configurations
rg "isNumber\|isDate\|isText" packages/dataview/src/types/property.type.ts -B 2 -A 2Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Check if there's any server-side code that validates rollup configurations
find packages -name "*.ts" -path "*/trpc/*" -o -path "*/server/*" | head -20Repository: jingerpie/ocean-dataview
Length of output: 377
🏁 Script executed:
# Let's look for actual usage of RollupConfig in examples or tests to see if invalid combos are created
rg "type.*number\|type.*text\|type.*date\|type.*select" packages/dataview/src -g "*.ts" -B 3 -A 3 | grep -A 5 -B 5 "calculation"Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Check the complete RollupArrayDisplay to understand all type cases it handles
cat -n packages/dataview/src/components/views/data-cell.tsx | sed -n '100,200p'Repository: jingerpie/ocean-dataview
Length of output: 3389
🏁 Script executed:
# Check if there's any Zod or schema validation in packages/trpc
cat packages/trpc/src/lib/schemas.ts | head -50Repository: jingerpie/ocean-dataview
Length of output: 1386
🏁 Script executed:
# Check if there's a RollupConfig schema defined anywhere
rg "RollupConfig" packages/trpc/src -g "*.ts" -B 2 -A 5Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Check if the validation happens at API boundary when RollupConfig is received
rg "schema.*rollup\|rollup.*schema" packages/trpc/src -g "*.ts" -iRepository: jingerpie/ocean-dataview
Length of output: 50
Use discriminated unions to encode valid rollup configurations in the type system.
RollupConfig currently accepts invalid combinations like { type: "text", calculation: "sum" } or applying number display options to array rollups. Although JSDoc comments note that math calculations are "only valid when underlying type is 'number'", nothing prevents these states at compile time. Split into discriminated variants so math operations (sum, average, median, min, max, range) require type: "number", display-only rollups (showOriginal, showUnique) constrain which display fields are valid, and scalar rollups use appropriate config subsets.
As per coding guidelines, "Use discriminated unions for mutually exclusive states".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/types/property.type.ts` around lines 250 - 269, Replace
the broad RollupConfig interface with a discriminated union of specific variants
so invalid combinations are impossible: create a NumberRollupConfig (calculation
in {"sum","average","median","min","max","range"}, type: "number", allowed
numeric display fields decimalPlaces/numberFormat/scale/showAs), a
DateRollupConfig (calculation appropriate for dates, type: "date", allowed
dateFormat/timeFormat), a SelectRollupConfig (type: "select"|"status", allowed
options), an ArrayRollupConfig (for array results with its valid display
fields), and an OtherScalarRollupConfig for scalar passthroughs like
showOriginal/showUnique; keep RollupCalculation and PropertyType usages and
preserve the Exclude<PropertyType,"formula"|"button"|"rollup"> constraint on
each variant, removing fields that don’t apply to a variant so the type system
enforces valid combinations.
| value?( | ||
| property: PropertyRenderFunction, | ||
| item: T | ||
| // biome-ignore lint/suspicious/noExplicitAny: Flexible signature to avoid type inference issues with union types | ||
| ): any; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 112
🏁 Script executed:
cat -n packages/dataview/src/types/property.type.ts | head -100Repository: jingerpie/ocean-dataview
Length of output: 3828
🏁 Script executed:
wc -l packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 116
🏁 Script executed:
sed -n '474,478p' packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 255
🏁 Script executed:
rg "PropertyRenderFunction" packages/dataview/src/types/property.type.ts -A 2 -B 2Repository: jingerpie/ocean-dataview
Length of output: 427
🏁 Script executed:
rg "any" packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 702
🏁 Script executed:
rg "ReactNode" packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 338
🏁 Script executed:
rg "PropertyRenderFunction" packages/dataview/src -lRepository: jingerpie/ocean-dataview
Length of output: 180
🏁 Script executed:
rg "PropertyRenderFunction\b" packages/dataview/src --type ts --type tsx -B 2 -A 5Repository: jingerpie/ocean-dataview
Length of output: 95
🏁 Script executed:
rg "PropertyRenderFunction" packages/dataview/src -B 3 -A 5Repository: jingerpie/ocean-dataview
Length of output: 3618
🏁 Script executed:
cat -n packages/dataview/src/components/ui/properties/formula-property.tsx | head -100Repository: jingerpie/ocean-dataview
Length of output: 2164
🏁 Script executed:
rg "PropertyRenderFunction" packages/dataview/src/components/ui/properties/formula-property.tsx -B 5 -A 10Repository: jingerpie/ocean-dataview
Length of output: 966
🏁 Script executed:
sed -n '200,230p' packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 778
🏁 Script executed:
rg "value\(" packages/dataview/src/components -B 2 -A 5 | head -60Repository: jingerpie/ocean-dataview
Length of output: 696
🏁 Script executed:
grep -n "import.*ReactNode\|from.*react" packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 123
🏁 Script executed:
sed -n '325,385p' packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 1923
🏁 Script executed:
rg "export type FormulaPropConfig|export interface FormulaProperty" packages/dataview/src/types/property.type.ts -A 20Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
rg "export type FormulaPropertyType|export interface FormulaProp" packages/dataview/src/types/property.type.ts -A 25Repository: jingerpie/ocean-dataview
Length of output: 970
🏁 Script executed:
sed -n '440,500p' packages/dataview/src/types/property.type.tsRepository: jingerpie/ocean-dataview
Length of output: 1957
🏁 Script executed:
rg "PropertyRenderFunction" packages/dataview/src -B 2 -A 2Repository: jingerpie/ocean-dataview
Length of output: 2197
🏁 Script executed:
cat -n packages/dataview/src/types/property.type.ts | sed -n '1,5p'Repository: jingerpie/ocean-dataview
Length of output: 267
Replace any with ReactNode in both PropertyRenderFunction and the value? method, and remove the Biome suppressions.
This is part of the public formula-property API where any leaks directly through the type surface. The documentation already specifies ReactNode returns, and the actual implementation (formula-property.tsx) only returns null or JSX elements—both valid ReactNode. Update the import and type definitions:
Suggested fix
-import type { ComponentType, SVGProps } from "react";
+import type { ComponentType, ReactNode, SVGProps } from "react";-// biome-ignore lint/suspicious/noExplicitAny: Returns ReactNode but using any for simpler type inference
-export type PropertyRenderFunction = (id: string) => any;
+export type PropertyRenderFunction = (id: string) => ReactNode; value?(
property: PropertyRenderFunction,
item: T
- // biome-ignore lint/suspicious/noExplicitAny: Flexible signature to avoid type inference issues with union types
- ): any;
+ ): ReactNode;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/types/property.type.ts` around lines 474 - 478, Change
the public API types to return ReactNode instead of any: update the
PropertyRenderFunction return type and the optional method value? (both in
types/property.type.ts) to ReactNode, remove the biome-ignore lint suppression
comments, and add the React import (e.g. import { ReactNode } from 'react') so
the types reference ReactNode.
| export type RollupPropertyType<T> = BaseProperty<T> & { | ||
| type: "rollup"; | ||
| config: RollupConfig; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the property.type.ts file
find . -name "property.type.ts" -type fRepository: jingerpie/ocean-dataview
Length of output: 114
🏁 Script executed:
# Read the file around the specified lines (543-546)
cat -n packages/dataview/src/types/property.type.ts | sed -n '535,555p'Repository: jingerpie/ocean-dataview
Length of output: 753
🏁 Script executed:
# Also check the BaseProperty definition and line 688
cat -n packages/dataview/src/types/property.type.ts | sed -n '1,50p'Repository: jingerpie/ocean-dataview
Length of output: 1767
🏁 Script executed:
# Check line 688 to see the fallback behavior mentioned
cat -n packages/dataview/src/types/property.type.ts | sed -n '680,700p'Repository: jingerpie/ocean-dataview
Length of output: 780
🏁 Script executed:
# Read more of BaseProperty to find the key field definition
cat -n packages/dataview/src/types/property.type.ts | sed -n '26,150p'Repository: jingerpie/ocean-dataview
Length of output: 4739
🏁 Script executed:
# Search for where key is defined in BaseProperty
rg "key\?" packages/dataview/src/types/property.type.ts -A 2 -B 2 | head -50Repository: jingerpie/ocean-dataview
Length of output: 386
🏁 Script executed:
# Check all property type definitions to see if any override the key field
rg "PropertyType<T>" packages/dataview/src/types/property.type.ts -A 3 | head -80Repository: jingerpie/ocean-dataview
Length of output: 1915
🏁 Script executed:
# Look for rollup property definitions in tests or examples
find . -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.json" \) -exec rg -l "rollup" {} \; | grep -E "(test|spec|example)" | head -10Repository: jingerpie/ocean-dataview
Length of output: 50
🏁 Script executed:
# Check if there are existing rollup property definitions that might be affected
rg '"rollup"' packages/dataview -B 5 -A 5 | head -100Repository: jingerpie/ocean-dataview
Length of output: 6938
🏁 Script executed:
# Search for rollup property definitions/instantiations in the codebase
rg "type:\s*['\"]rollup" packages/dataview -B 3 -A 3 | head -150Repository: jingerpie/ocean-dataview
Length of output: 1415
🏁 Script executed:
# Check if there's any processing that creates rollup properties without key
rg "RollupPropertyType" packages/dataview -B 3 -A 3Repository: jingerpie/ocean-dataview
Length of output: 1568
🏁 Script executed:
# Look at the context around line 688 more broadly to understand the function
cat -n packages/dataview/src/types/property.type.ts | sed -n '670,700p'Repository: jingerpie/ocean-dataview
Length of output: 1235
Enforce required key for rollup properties to prevent silent fallback bugs.
Rollup properties inherit an optional key from BaseProperty, allowing type-unsafe definitions to pass checks. At line 688, the code falls back to id when key is missing (const key = p.key ?? p.id), then uses this potentially-wrong value to build the server extras query. This violates the coding guideline requiring immediate failure when conditions aren't met. Since rollups require a real backend field path (unlike display properties), RollupPropertyType should override key to be required.
Suggested fix
export type RollupPropertyType<T> = BaseProperty<T> & {
+ key: string;
type: "rollup";
config: RollupConfig;
};📝 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.
| export type RollupPropertyType<T> = BaseProperty<T> & { | |
| type: "rollup"; | |
| config: RollupConfig; | |
| }; | |
| export type RollupPropertyType<T> = BaseProperty<T> & { | |
| key: string; | |
| type: "rollup"; | |
| config: RollupConfig; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/types/property.type.ts` around lines 543 - 546, The
RollupPropertyType currently inherits an optional key from BaseProperty which
allows code like const key = p.key ?? p.id (and subsequent use in building
server extras queries) to silently fall back to id; change RollupPropertyType to
require key (i.e., make key non-optional) so the compiler forces callers to
supply a real backend field path, update the type definition for
RollupPropertyType to include key: string (or the same key type used in
BaseProperty but required), and run the typechecker to catch any call sites that
must be updated (look for usages of RollupPropertyType, RollupConfig, and the
p.key ?? p.id pattern to fix places that relied on the optional key).
| function buildMap(fn: (base: string) => string): Record<string, string> { | ||
| const map: Record<string, string> = {}; | ||
| for (const base of BASE_COLORS) { | ||
| const value = fn(base); | ||
| map[base] = value; | ||
| map[`${base}-subtle`] = value; | ||
| } | ||
| return map; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/dataview/src/utils/badge-colors.tsRepository: jingerpie/ocean-dataview
Length of output: 4234
🏁 Script executed:
grep -r "type BadgeColor" packages/dataview/src/types/ -A 10Repository: jingerpie/ocean-dataview
Length of output: 738
🏁 Script executed:
grep -r "type BadgeColor" packages/dataview/src/types/ -A 20Repository: jingerpie/ocean-dataview
Length of output: 1362
Make badge color lookups exhaustive instead of silently falling back to gray.
The getter functions (lines 74-95) all use ?? fallbacks to gray values, which masks unmapped colors and violates the coding guidelines requiring fail-fast behavior and explicit error handling. The maps created by buildMap() return Record<string, string>, which loses exhaustiveness checking. Use satisfies Record<BadgeColor, string> on COLOR_CLASSES, BG_CLASSES, BG_VARS, and FOREGROUND_VARS (like BG_TRANSPARENT_CLASSES and TEXT_COLORS already do) to enforce completeness, then throw an error in the getters instead of falling back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/utils/badge-colors.ts` around lines 14 - 22, The maps
produced by buildMap currently use Record<string,string>, losing exhaustiveness
and allowing getters to fall back to gray; change buildMap usage so that
COLOR_CLASSES, BG_CLASSES, BG_VARS and FOREGROUND_VARS are declared with
"satisfies Record<BadgeColor, string>" (matching the pattern used by
BG_TRANSPARENT_CLASSES and TEXT_COLORS) to enforce completeness, adjust
buildMap's typing as needed to produce a BadgeColor-keyed map, and then update
the badge getter functions to throw a descriptive error when a requested
BadgeColor key is missing instead of using the "??" gray fallback; reference
buildMap, COLOR_CLASSES, BG_CLASSES, BG_VARS, FOREGROUND_VARS,
BG_TRANSPARENT_CLASSES, TEXT_COLORS and the badge getter functions in your
changes.
| case "rollup": | ||
| // Default to text conditions when property context isn't available. | ||
| // Use getFilterConditionsForProperty() when you have the full property. | ||
| return DATA_TABLE_CONFIG.textConditions; |
There was a problem hiding this comment.
Don't silently treat bare rollups as text filters.
The new property-aware helpers are fine, but any caller that still validates by property.type will now classify every rollup as text. That means numeric/date/select rollups can default correctly here and then fail later in isValidConditionForPropertyType(...) or other type-only paths. I'd rather fail fast for bare "rollup" or add a property-aware validation helper and migrate the old API.
As per coding guidelines, "No silent fallbacks - code must fail immediately when conditions aren't met."
Also applies to: 84-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/dataview/src/utils/filter.ts` around lines 42 - 45, The code
currently treats a bare "rollup" type as text by returning
DATA_TABLE_CONFIG.textConditions; instead, update the switch branch that handles
case "rollup" (the branch that runs when you don't have a full property) to fail
fast: throw a clear error (e.g., "rollup property context required") rather than
silently returning text conditions, and do the same for the other analogous
branch that currently defaults to DATA_TABLE_CONFIG.textConditions; callers that
have full property metadata should continue to use
getFilterConditionsForProperty(), and downstream validators like
isValidConditionForPropertyType(...) will then only receive properly-typed
conditions.
1 participant
Summary
sum,average,showOriginal,showUnique, etc.) and array-filter quantifiers (any/none/every).id(stable view key) fromkey(server field), renamelabel→name, add per-propertyshowName/wrap/sizedisplay overrides and compact card layouts.combineGroupFilter,buildGroupWhere), quantifier picker UI, advanced filter rule tweaks.sm/md/lgsizes propagated toselect/multiSelect/status/numberproperties.SimpleDataViewProviderfor lightweight embeds; query bridge now resolves property ids → keys for server queries..claudeskills reshuffled (shadcn-designer removed; shadcn, turborepo, ultracite, get-pr-code-review added); docs moved to.docs/.Why
This consolidates a long-running refactor of dataview's property model, filter pipeline, and view rendering. Separating
id/keyunblocks formula and rollup fields; moving group filtering server-side fixes correctness on large datasets; sticky headers via CSS simplify the layout code that previously relied on portals. The rollup + quantifier work completes the filter surface for related-table data.Test Plan
any/none/everyquantifiers on rollup properties.dataview-themestorage key.bun run check && bun run check-types && bun run build.Summary by CodeRabbit
Release Notes
New Features
Enhancements
Dependencies