Enhance dataset view: inline name editing, cell detail side sheet, column filter#78
Enhance dataset view: inline name editing, cell detail side sheet, column filter#78MabudAlam wants to merge 11 commits into
Conversation
…pansion functionality
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a Convex query listByDatasetFiltered and an updateDetails mutation; UI components BigSetToaster, SideSheet/CellDetail, and FilterPopover/ActiveFilter; table cell expansion wiring (onCellExpand) and DatasetTable/DataRow changes; inline dataset name editing on the dataset page; slide-in CSS and layout wiring; and dependency additions (lucide-react, sonner). Sequence DiagramssequenceDiagram
participant User
participant FilterPopover as FilterPopover
participant DatasetPage as DatasetPage
participant Convex as listByDatasetFiltered
User->>FilterPopover: open and select column/value
User->>FilterPopover: click Apply
FilterPopover->>DatasetPage: onFilter(column, value, matchType)
DatasetPage->>Convex: query(datasetId, filter)
Convex-->>DatasetPage: filtered rows
DatasetPage->>User: render filtered rows + ActiveFilter
sequenceDiagram
participant User
participant DataRow as DataRow Cell
participant DatasetPage as DatasetPage
participant SideSheet as SideSheet
participant CellDetail as CellDetail
participant BigSetToaster as Toaster
User->>DataRow: click expand button
DataRow->>DatasetPage: onCellExpand(column, value, rowId)
DatasetPage->>SideSheet: open with CellDetail props
SideSheet->>SideSheet: lock body scroll / trap focus
User->>CellDetail: click Copy
CellDetail->>navigator.clipboard: write(value)
CellDetail->>BigSetToaster: show toast success/error
User->>SideSheet: close
SideSheet->>SideSheet: unlock body scroll
Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
frontend/app/globals.css (1)
99-101: ⚡ Quick winRespect reduced-motion preference for the slide-in utility.
Add a reduced-motion override so the panel opens without animation when users request less motion.
Proposed fix
.animate-slide-in { animation: slide-in 0.3s ease-out; } + +@media (prefers-reduced-motion: reduce) { + .animate-slide-in { + animation: none; + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/globals.css` around lines 99 - 101, Update the .animate-slide-in rule to respect the user's reduced-motion preference by adding a prefers-reduced-motion media query override that disables the animation (set animation: none and remove transitions) when prefers-reduced-motion: reduce; ensure the selector .animate-slide-in and the keyframe name slide-in are targeted so the panel opens without animation for users who request less motion.frontend/components/table/DataRow.tsx (1)
86-88: ⚡ Quick winMake the expand control keyboard-discoverable and contextual.
The button is only shown on hover; add focus visibility and include column context in
aria-label.Proposed fix
- className="absolute right-1 top-1/2 -translate-y-1/2 opacity-0 group-hover:opacity-100 p-0.5 rounded bg-foreground/5 hover:bg-foreground/10 text-muted hover:text-foreground transition-all" - aria-label="Expand cell" + className="absolute right-1 top-1/2 -translate-y-1/2 opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 focus:opacity-100 p-0.5 rounded bg-foreground/5 hover:bg-foreground/10 text-muted hover:text-foreground transition-all" + aria-label={`Expand cell in column ${col.name}`}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/table/DataRow.tsx` around lines 86 - 88, Update the expand button in DataRow.tsx so it becomes keyboard-discoverable and provides column context: add focus and focus-visible classes (e.g., focus:opacity-100 and/or focus-visible:outline/focus-visible:ring) to the existing className that uses group-hover:opacity-100 so the control becomes visible on keyboard focus, and change the static aria-label="Expand cell" to a contextual label that includes the column identifier (e.g., aria-label={`Expand ${column.title || column.id} cell`}) using the DataRow props (column, column.title or column.id) so screen-reader users know which column is being expanded.frontend/components/SideSheet.tsx (1)
16-24: ⚡ Quick winPreserve prior body overflow when closing the sheet.
Resetting to
""can override an existing overflow lock from another overlay and re-enable scrolling unexpectedly.Proposed fix
useEffect(() => { - if (open) { - document.body.style.overflow = "hidden"; - } else { - document.body.style.overflow = ""; - } - return () => { - document.body.style.overflow = ""; - }; + if (!open) return; + const previousOverflow = document.body.style.overflow; + document.body.style.overflow = "hidden"; + return () => { + document.body.style.overflow = previousOverflow; + }; }, [open]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/components/SideSheet.tsx` around lines 16 - 24, The effect that toggles document.body.style.overflow uses a hard reset to "" which can overwrite another overlay's lock; modify the useEffect in SideSheet to save the previous overflow value (e.g., const prevOverflowRef = useRef<string | null>(null)) when open becomes true, set overflow to "hidden", and on close/unmount restore document.body.style.overflow = prevOverflowRef.current ?? "" (or delete the inline style if prev was null) so the prior overflow state is preserved; update the effect cleanup and the else branch to use this stored previous value rather than always assigning "".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/app/dataset/`[id]/page.tsx:
- Around line 331-335: FilterPopover is receiving filtered or empty data because
you're passing allRows which is set to undefined when filter !== null; change
the rows prop to always receive the unfiltered source rows (e.g., preserve and
pass the originalRows/initialRows or dataset.rows) so the exact-match value list
is built from the full source set regardless of active filters; locate the
FilterPopover usage and replace rows={allRows ?? []} with rows={originalRows ??
dataset.rows ?? []} (or a preserved constant created before applying filters) so
the popover always gets the complete source rows.
- Around line 252-283: The rename UI (editingName branch and the edit-button
that calls startEditingName) is rendered for all viewers; restrict it to owners
by checking an ownership flag (e.g., compute isOwner = currentUser?.id ===
dataset.ownerId or derive from session) and only render the input/button when
isOwner is true; otherwise render a plain read-only <h1> showing dataset.name.
Also harden startEditingName/saveName/cancelNameEdit so they no-op if the caller
is not owner to avoid any accidental invocation.
In `@frontend/components/dataset/FilterPopover.tsx`:
- Around line 68-77: The current colValues computation uses the possibly
filtered rows array (rows) which makes exact-match options shrink when a filter
is active; change the source for exact options to an unfiltered dataset (e.g.,
add/consume a prop like unfilteredRows or distinctValues from the backend) and
compute colValues from that unfiltered source instead of rows; update the logic
around selectedColDef and colValues to use the new prop (or fetch distinct
values from the backend if provided) so options remain stable while filters are
applied.
In `@frontend/components/SideSheet.tsx`:
- Around line 68-74: The copy handler currently uses displayValue (which maps
null/empty to "—") so handleCopy writes the fallback glyph to the clipboard;
change handleCopy to write the raw value instead (use value or String(value)
only when value is non-null/defined) and keep displayValue for rendering only;
update the call to navigator.clipboard.writeText to use the raw source (e.g.,
value ?? "" or String(value) when value is defined) so empty/null copies an
empty string or the actual underlying value rather than "—".
In `@frontend/convex/datasets.ts`:
- Around line 134-149: The updateDetails mutation unconditionally writes
args.description and can clobber a concurrent change; in the handler
(updateDetails) compare args.description to the loaded dataset.description and
only include description in the ctx.db.patch payload when it actually differs
(or omit it when undefined), i.e. build the patch object conditionally (e.g.,
always set name to trimmedName, and only add description if args.description !==
dataset.description) before calling ctx.db.patch(dataset._id, patch).
---
Nitpick comments:
In `@frontend/app/globals.css`:
- Around line 99-101: Update the .animate-slide-in rule to respect the user's
reduced-motion preference by adding a prefers-reduced-motion media query
override that disables the animation (set animation: none and remove
transitions) when prefers-reduced-motion: reduce; ensure the selector
.animate-slide-in and the keyframe name slide-in are targeted so the panel opens
without animation for users who request less motion.
In `@frontend/components/SideSheet.tsx`:
- Around line 16-24: The effect that toggles document.body.style.overflow uses a
hard reset to "" which can overwrite another overlay's lock; modify the
useEffect in SideSheet to save the previous overflow value (e.g., const
prevOverflowRef = useRef<string | null>(null)) when open becomes true, set
overflow to "hidden", and on close/unmount restore document.body.style.overflow
= prevOverflowRef.current ?? "" (or delete the inline style if prev was null) so
the prior overflow state is preserved; update the effect cleanup and the else
branch to use this stored previous value rather than always assigning "".
In `@frontend/components/table/DataRow.tsx`:
- Around line 86-88: Update the expand button in DataRow.tsx so it becomes
keyboard-discoverable and provides column context: add focus and focus-visible
classes (e.g., focus:opacity-100 and/or
focus-visible:outline/focus-visible:ring) to the existing className that uses
group-hover:opacity-100 so the control becomes visible on keyboard focus, and
change the static aria-label="Expand cell" to a contextual label that includes
the column identifier (e.g., aria-label={`Expand ${column.title || column.id}
cell`}) using the DataRow props (column, column.title or column.id) so
screen-reader users know which column is being expanded.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 441b8f5d-6f7c-4def-8281-ec87da2f12a2
📒 Files selected for processing (11)
frontend/app/dataset/[id]/page.tsxfrontend/app/globals.cssfrontend/app/layout.tsxfrontend/components/SideSheet.tsxfrontend/components/Toaster.tsxfrontend/components/dataset/FilterPopover.tsxfrontend/components/table/DataRow.tsxfrontend/components/table/DatasetTable.tsxfrontend/convex/datasetRows.tsfrontend/convex/datasets.tsfrontend/package.json
| const colValues = selectedColDef | ||
| ? Array.from( | ||
| new Set( | ||
| rows | ||
| .map((r) => r.data[selectedColDef.name]) | ||
| .map((v) => String(v ?? "")) | ||
| .filter(Boolean), | ||
| ), | ||
| ).sort() | ||
| : []; |
There was a problem hiding this comment.
Exact-match options should not be derived from potentially filtered rows.
When the parent provides already-filtered rows, this list shrinks to the current subset, which can prevent users from switching exact filters to values no longer present in that subset.
Use an unfiltered source for exact options (e.g., distinct values from backend or a dedicated unfiltered prop) so option availability is stable while a filter is active.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/components/dataset/FilterPopover.tsx` around lines 68 - 77, The
current colValues computation uses the possibly filtered rows array (rows) which
makes exact-match options shrink when a filter is active; change the source for
exact options to an unfiltered dataset (e.g., add/consume a prop like
unfilteredRows or distinctValues from the backend) and compute colValues from
that unfiltered source instead of rows; update the logic around selectedColDef
and colValues to use the new prop (or fetch distinct values from the backend if
provided) so options remain stable while filters are applied.
…eSheet overflow handling
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/app/dataset/[id]/page.tsx (1)
61-70:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFiltered mode still subscribes to the full unfiltered row set.
When a filter is active, this page keeps
listByDatasetandlistByDatasetFilteredlive simultaneously. That preserves full-row transfer/client memory and weakens the server-side filtering payoff for large datasets. Consider replacingallRowsusage with a lightweight backend endpoint for exact-filter options (e.g., distinct values per column) or gating full-row subscription strictly to when it is needed.Also applies to: 336-339
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/app/dataset/`[id]/page.tsx around lines 61 - 70, The page currently keeps both allRows (api.datasetRows.listByDataset) and filteredRows (api.datasetRows.listByDatasetFiltered) live at the same time, causing full-row subscription even when a filter is applied; change the logic so allRows is skipped whenever filter !== null (i.e., only subscribe to listByDataset when no filter is active) OR replace uses of allRows for filter UI with a lightweight backend call that returns distinct/summary values (e.g., a new api.datasetRows.distinctValues endpoint) and only subscribe to full list when the UI explicitly requires the full row set; apply the same gating/endpoint replacement to the other occurrence referenced (lines ~336-339) so full-row transfer is prevented when filteredRows is used.
♻️ Duplicate comments (1)
frontend/convex/datasets.ts (1)
135-152:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPrevent stale-field clobber in
updateDetails.This still overwrites fields from stale client state in concurrent edits because both fields are required and the mutation can’t infer intent. Make
name/descriptionoptional and only patch fields explicitly provided by the caller.Suggested minimal direction
export const updateDetails = mutation({ args: { id: v.id("datasets"), - name: v.string(), - description: v.string(), + name: v.optional(v.string()), + description: v.optional(v.string()), }, handler: async (ctx, args) => { - const trimmedName = args.name.trim(); - if (!trimmedName) throw new Error("Dataset name cannot be empty"); const dataset = await loadOwnedDataset(ctx, args.id); - - // Skip entirely if neither field actually changed. - const nameChanged = trimmedName !== dataset.name; - const descChanged = args.description !== dataset.description; - if (!nameChanged && !descChanged) return; - - const patch: Record<string, unknown> = { name: trimmedName }; - if (descChanged) patch.description = args.description; + const patch: { name?: string; description?: string } = {}; + if (args.name !== undefined) { + const trimmedName = args.name.trim(); + if (!trimmedName) throw new Error("Dataset name cannot be empty"); + if (trimmedName !== dataset.name) patch.name = trimmedName; + } + if ( + args.description !== undefined && + args.description !== dataset.description + ) { + patch.description = args.description; + } + if (Object.keys(patch).length === 0) return; await ctx.db.patch(dataset._id, patch); }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/convex/datasets.ts` around lines 135 - 152, Change the mutation to treat name and description as optional and only patch fields the client actually provided: make args.name and args.description optional, check for their presence (e.g. args.hasOwnProperty('name') / args.name !== undefined) before trimming/validating and before adding to the patch, validate name only when provided (trim and error if empty), use loadOwnedDataset to get current values but avoid using dataset values to overwrite unspecified fields, and call ctx.db.patch(dataset._id, patch) with a patch object that only contains the keys for fields that were explicitly sent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/app/dataset/`[id]/page.tsx:
- Around line 61-70: The page currently keeps both allRows
(api.datasetRows.listByDataset) and filteredRows
(api.datasetRows.listByDatasetFiltered) live at the same time, causing full-row
subscription even when a filter is applied; change the logic so allRows is
skipped whenever filter !== null (i.e., only subscribe to listByDataset when no
filter is active) OR replace uses of allRows for filter UI with a lightweight
backend call that returns distinct/summary values (e.g., a new
api.datasetRows.distinctValues endpoint) and only subscribe to full list when
the UI explicitly requires the full row set; apply the same gating/endpoint
replacement to the other occurrence referenced (lines ~336-339) so full-row
transfer is prevented when filteredRows is used.
---
Duplicate comments:
In `@frontend/convex/datasets.ts`:
- Around line 135-152: Change the mutation to treat name and description as
optional and only patch fields the client actually provided: make args.name and
args.description optional, check for their presence (e.g.
args.hasOwnProperty('name') / args.name !== undefined) before
trimming/validating and before adding to the patch, validate name only when
provided (trim and error if empty), use loadOwnedDataset to get current values
but avoid using dataset values to overwrite unspecified fields, and call
ctx.db.patch(dataset._id, patch) with a patch object that only contains the keys
for fields that were explicitly sent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9733006f-fe45-48cb-bda9-760edf246db4
📒 Files selected for processing (5)
frontend/app/dataset/[id]/page.tsxfrontend/app/globals.cssfrontend/components/SideSheet.tsxfrontend/components/table/DataRow.tsxfrontend/convex/datasets.ts
|
Hey @MabudAlam This is super cool, thanks for the PR! I'll take a look at this one and get back to you shortly! |
Jaredee123
left a comment
Jaredee123
left a comment
There was a problem hiding this comment.
Code Review
Went through the full diff. No auth bypass or injection vulnerabilities — the authorization architecture is correct (updateDetails goes through loadOwnedDataset server-side, ownerId is never accepted from the client). Good work there.
However there are 3 issues that need to be fixed before this can merge, plus some medium-severity ones to address before production.
🔴 Must fix before merge
1. Merge conflicts will break the build
The PR was authored against an older commit. Three files now conflict with main:
DataRow.tsx—DataRowDatais missingisBuilding: boolean(added by a later PR). TypeScript will fail to compile.DatasetTable.tsx— theuseMemodep array dropsisBuilding, so ghost rows won't animate during dataset population.datasets.ts— the PR adds agetInternalexport that already exists onmain. Duplicate export → Convex push fails entirely.
When resolving the datasets.ts conflict, keep main's version of getInternal — it returns null on not-found, which the backend populate route depends on. The PR version throws, which would break the delete-race protection logic.
2. Enter key triggers saveName twice (app/dataset/[id]/page.tsx)
onBlur={saveName}
onKeyDown={(e) => {
if (e.key === "Enter") saveName(); // ← fires once
// input then loses focus → onBlur fires saveName() again
}}Pressing Enter calls saveName() directly, then the browser blurs the input, firing onBlur → second concurrent mutation. Fix:
onKeyDown={(e) => {
if (e.key === "Enter") {
e.preventDefault();
nameInputRef.current?.blur(); // blur triggers onBlur → saveName() once
}
if (e.key === "Escape") { ... }
}}3. saveName silently clobbers concurrent description edits (app/dataset/[id]/page.tsx)
await updateDetails({
id: dataset._id,
name: trimmed,
description: dataset.description, // ← stale snapshot from page load
});If the description was updated in another tab between when this page loaded and when the user saves the name, the old description value wins. The fix is to not send description in saveName at all — either split updateDetails into updateName / updateDescription, or change the mutation to only patch name when that's all that changed.
🟡 Fix before production
4. Cell values unmasked in PostHog session replays (SideSheet.tsx)
Table cells have data-ph-mask-text="true" so values are redacted in session recordings. The expanded value in the side sheet does not:
<p className="text-sm text-foreground break-all whitespace-pre-wrap">
{displayValue} {/* visible in PostHog replays */}
</p>Add data-ph-mask-text="true" to this element and the copy button wrapper.
5. handleCellExpand not memoized — defeats all DataRow memoization (app/dataset/[id]/page.tsx)
handleCellExpand is declared as a plain function (not useCallback), so it gets a new reference on every render. It's passed into itemData's useMemo, which then invalidates on every render, forcing every visible row to re-render whenever any state changes (filter, side sheet open/close, etc.). The existing toggleRow is wrapped in useCallback precisely to avoid this — handleCellExpand should be too:
const handleCellExpand = useCallback((columnName: string, value: unknown, rowId: string) => {
...
}, []);6. SideSheet scroll-lock useEffect double-restores body overflow (SideSheet.tsx)
When open transitions true → false, React runs the previous effect's cleanup (restores overflow, nulls the ref), then runs the new effect's else branch (reads the now-null ref, sets overflow to ""). The second write overwrites the restored value. If the page had body { overflow: auto }, closing the sheet breaks scroll permanently. Remove the else branch — the cleanup return handles restoration.
7. colValues computed O(N) on every FilterPopover render (FilterPopover.tsx)
const colValues = selectedColDef
? Array.from(new Set(rows.map(r => r.data[selectedColDef.name]))).sort()
: [];This runs synchronously on every render — including every keystroke in the filter search box. For large datasets this causes noticeable input lag. Wrap in useMemo([rows, selectedColDef]).
🔵 Low / polish
updateDetailsusesRecord<string, unknown>for the patch object — loses Convex's generated type safety. UsePartial<Doc<"datasets">>instead.filter: nullpath inlistByDatasetFilteredis dead code — the client always passes"skip"when there's no filter, sonullis never sent.- Two "Clear filter" affordances render simultaneously when a filter is active (the
×insideActiveFilterand a separate "Clear" text button). Pick one. SideSheetis missingrole="dialog",aria-modal="true", a focus trap, and an Escape key handler. Screen readers can't determine the dialog boundary and Tab escapes the sheet.
Overall the feature logic is solid and the auth story is correct. Main blockers are the merge conflicts and the double-mutation on Enter. Happy to help resolve once those are addressed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/convex/datasetRows.ts (1)
236-244: ⚡ Quick winPrefer cached
datasets.rowCountbefore full row scan incountByDataset.
countByDatasetalways doescollect().length, which scales with dataset size. This file already maintainsdatasets.rowCounton insert/remove/clear, so read that first and only fall back to a scan for legacy docs missing the field.♻️ Proposed refactor
export const countByDataset = internalQuery({ args: { datasetId: v.id("datasets") }, handler: async (ctx, args) => { - const rows = await ctx.db - .query("datasetRows") - .withIndex("by_dataset", (q) => q.eq("datasetId", args.datasetId)) - .collect(); - return rows.length; + const dataset = await ctx.db.get(args.datasetId); + if (!dataset) return 0; + if (typeof dataset.rowCount === "number") return dataset.rowCount; + + const rows = await ctx.db + .query("datasetRows") + .withIndex("by_dataset", (q) => q.eq("datasetId", args.datasetId)) + .collect(); + return rows.length; }, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/convex/datasetRows.ts` around lines 236 - 244, Update the countByDataset handler to read the dataset document first (use the datasets record for args.datasetId) and if that document has a numeric datasets.rowCount, return it immediately; only if rowCount is missing/undefined fall back to the existing ctx.db.query("datasetRows").withIndex("by_dataset", q => q.eq("datasetId", args.datasetId)).collect() and return rows.length. Keep the existing internalQuery signature and ensure you reference countByDataset and the datasets.rowCount field when implementing the short-circuit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@frontend/convex/datasetRows.ts`:
- Around line 236-244: Update the countByDataset handler to read the dataset
document first (use the datasets record for args.datasetId) and if that document
has a numeric datasets.rowCount, return it immediately; only if rowCount is
missing/undefined fall back to the existing
ctx.db.query("datasetRows").withIndex("by_dataset", q => q.eq("datasetId",
args.datasetId)).collect() and return rows.length. Keep the existing
internalQuery signature and ensure you reference countByDataset and the
datasets.rowCount field when implementing the short-circuit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6dbdec6-14dd-49b7-ab1a-551c5f891fc0
📒 Files selected for processing (3)
frontend/app/dataset/[id]/page.tsxfrontend/convex/datasetRows.tsfrontend/package.json
✅ Files skipped from review due to trivial changes (1)
- frontend/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/app/dataset/[id]/page.tsx
…formance; update dataset mutation to handle optional description
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/convex/datasetRows.ts (1)
87-87:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't collapse empty-string filters into the unfiltered path.
!args.filter.valuemakes{ matchType: "exact", value: "" }return every row, so blank-cell exact filters can never work. Onlynullshould skip filtering here;"contains"with""already naturally returns all rows via.includes("").Suggested fix
- if (!args.filter || !args.filter.value) return rows; + if (!args.filter) return rows;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/convex/datasetRows.ts` at line 87, The current guard in datasetRows.ts uses if (!args.filter || !args.filter.value) return rows; which wrongly treats empty string ("") as "no filter" and prevents exact-empty filtering; change the second check to only skip when the filter value is null/undefined (e.g., if (!args.filter || args.filter.value == null) return rows) so that empty-string values are passed through to filtering logic (this preserves existing behavior for contains since "".includes("") already returns true).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/convex/datasetRows.ts`:
- Line 87: The current guard in datasetRows.ts uses if (!args.filter ||
!args.filter.value) return rows; which wrongly treats empty string ("") as "no
filter" and prevents exact-empty filtering; change the second check to only skip
when the filter value is null/undefined (e.g., if (!args.filter ||
args.filter.value == null) return rows) so that empty-string values are passed
through to filtering logic (this preserves existing behavior for contains since
"".includes("") already returns true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9f94645-198f-4296-a55d-4dd6aaf1c905
📒 Files selected for processing (5)
frontend/app/dataset/[id]/page.tsxfrontend/components/SideSheet.tsxfrontend/components/dataset/FilterPopover.tsxfrontend/convex/datasetRows.tsfrontend/convex/datasets.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/app/dataset/[id]/page.tsx
- frontend/components/SideSheet.tsx
- frontend/components/dataset/FilterPopover.tsx
|
Actionable comments posted: 0 |
…d settings dropdowns, and improve filter handling
3 participants
Fixed : #77
Screen.Recording.2026-05-23.at.11.57.51.PM.mov
Summary
Improves the dataset view page (
/dataset/[id]) with three new features:1. Inline Dataset Name Editing
frontend/convex/datasets.ts,frontend/app/dataset/[id]/page.tsx2. Cell Detail Side Sheet
frontend/components/SideSheet.tsx,frontend/components/table/DataRow.tsx,frontend/components/table/DatasetTable.tsx,frontend/app/globals.css3. Column Filter
column: *value*orcolumn: "value")listByDatasetFiltered) handles filtering to avoid sending unfiltered rows to the client