Add dataset filtering, inline name editing, and toast notifications#126
Open
MabudAlam wants to merge 7 commits into
Open
Add dataset filtering, inline name editing, and toast notifications#126MabudAlam wants to merge 7 commits into
MabudAlam wants to merge 7 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis pull request introduces three complementary features to the dataset page: (1) a global toast notification system using sonner with automatic theme synchronization, (2) client and server-side dataset row filtering with a popover UI and column-value matching, and (3) inline dataset name editing for owners with mutation-based persistence. The filtering feature integrates deeply into the page by computing a Sequence Diagram(s)These changes do not require sequence diagrams. The toast infrastructure is a straightforward provider setup; the filtering feature is a unidirectional state-driven UI with conditional query execution; and name editing is a simple mutation-triggered flow. None involve complex multi-component interactions or sequential workflows that would benefit from visualization. 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 |
MabudAlam
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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)
463-466:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse total dataset rows in the destructive confirmation modal.
Line 463 checks unfiltered
rows.lengthto decide whether destructive confirmation is needed, but Line 540 displaysdisplayRows.length. With an active filter, the modal can understate how many rows will be deleted.💡 Suggested fix
{confirmPopulate && ( <ConfirmPopulateModal - rowCount={displayRows.length} + rowCount={rows.length} onConfirm={() => { setConfirmPopulate(false); handlePopulate(); }}Also applies to: 539-541
🤖 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 463 - 466, The confirmation flow is inconsistent: the decision to show the destructive populate modal checks unfiltered rows.length but the modal text shows displayRows.length (filtered), which can understate deletions when a filter is active. Update the modal rendering(s) that currently reference displayRows.length to use the total dataset count (rows.length or a clear totalRows variable) so the confirmation message and the setConfirmPopulate/handlePopulate decision use the same total-row value; ensure both occurrences mentioned (the modal text and any other spots at the other referenced lines) are changed to reference rows.length.
🤖 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:
- Line 339: exportDisabled currently uses rows.length but exports act on
displayRows; change the condition to use displayRows.length (i.e., const
exportDisabled = exporting !== null || displayRows.length === 0) and update any
other checks around export logic that reference rows (see the export-related
block around displayRows usage) so the export button disables when
filtered/displayed rows are empty.
In `@frontend/components/Toaster.tsx`:
- Around line 25-39: The MutationObserver for data-theme currently calls
readTheme() which only checks localStorage/matchMedia and ignores direct changes
to document.documentElement; update the observer callback in the useEffect (the
block that creates observer and calls setTheme) to first read
document.documentElement.getAttribute("data-theme") and, if that value is
"light" or "dark", call setTheme with it, otherwise fall back to calling
readTheme() as before; ensure readTheme remains unchanged and only the observer
callback logic (and its call to setTheme) is modified so direct updates to
data-theme keep the Toaster in sync.
In `@frontend/convex/scripts.ts`:
- Around line 44-48: The idempotency check builds userSeedKeys from
existingUserDatasets but never updates it when you copy a dataset, so duplicate
seedKey values in seedDatasets can still be inserted; update userSeedKeys (or
maintain a local seen set) by adding the seedKey after a successful copy inside
the copy loop (the code that processes seedDatasets and calls the copy/insert
logic) and reuse that same set for subsequent checks (also ensure the same
update is applied in the other copy path referenced near the later seed
handling), referencing userSeedKeys, seedDatasets, and existingUserDatasets to
locate where to add seedKey post-success.
- Around line 35-37: The early-return in copySeedDatasetsToUser returns a
different shape (includes message, omits total); change it to return the same
contract as the normal path by returning { copied: 0, skipped: 0, total: 0 }
instead of the message object, and ensure the final return in
copySeedDatasetsToUser also always includes copied, skipped and total (no
message) so consumers receive a consistent result shape.
- Around line 79-80: The new dataset's rowCount (currently set from
seedDataset.rowCount) can be stale when no rows are copied; update rowCount from
the actual copied rows instead: use seedRows.length (or 0) when constructing the
dataset object and/or when calling the patch logic that currently runs only
inside the seedRows.length > 0 branch so the dataset's rowCount always reflects
actual rows; key symbols to change: rowCount, seedDataset, seedRows, and the
patch/update call that adjusts the dataset after seeding.
- Around line 140-147: The handler in deleteAllUserDatasets currently allows
deletion for any userId and can be invoked with SYSTEM_OWNER_ID; add a guard at
the start of the handler to detect if args.userId === SYSTEM_OWNER_ID (or any
other system owner constant) and abort by throwing a clear error (or returning a
non-destructive response). Update the deleteAllUserDatasets handler to perform
this check before querying or deleting datasets to ensure system/canonical seed
datasets cannot be removed.
---
Outside diff comments:
In `@frontend/app/dataset/`[id]/page.tsx:
- Around line 463-466: The confirmation flow is inconsistent: the decision to
show the destructive populate modal checks unfiltered rows.length but the modal
text shows displayRows.length (filtered), which can understate deletions when a
filter is active. Update the modal rendering(s) that currently reference
displayRows.length to use the total dataset count (rows.length or a clear
totalRows variable) so the confirmation message and the
setConfirmPopulate/handlePopulate decision use the same total-row value; ensure
both occurrences mentioned (the modal text and any other spots at the other
referenced lines) are changed to reference rows.length.
🪄 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: d1887bb3-1105-4270-b19c-c517cbb6c407
⛔ Files ignored due to path filters (1)
frontend/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
frontend/app/dataset/[id]/page.tsxfrontend/app/layout.tsxfrontend/components/Toaster.tsxfrontend/components/ToasterProvider.tsxfrontend/components/dataset/FilterPopover.tsxfrontend/convex/datasetRows.tsfrontend/convex/datasets.tsfrontend/convex/scripts.tsfrontend/package.json
| @@ -269,7 +337,7 @@ export default function DatasetPage() { | |||
| // the "Dataset not found" UI. | |||
|
|
|||
| const exportDisabled = exporting !== null || rows.length === 0; | |||
Contributor
There was a problem hiding this comment.
Disable Export based on filtered rows, not unfiltered rows.
Line 339 uses rows.length for exportDisabled, but export actually operates on displayRows (Lines 151-155). With an active filter yielding 0 rows, the button stays enabled and then silently no-ops.
💡 Suggested fix
- const exportDisabled = exporting !== null || rows.length === 0;
+ const exportDisabled = exporting !== null || displayRows.length === 0;Also applies to: 151-155
🤖 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 at line 339, exportDisabled currently
uses rows.length but exports act on displayRows; change the condition to use
displayRows.length (i.e., const exportDisabled = exporting !== null ||
displayRows.length === 0) and update any other checks around export logic that
reference rows (see the export-related block around displayRows usage) so the
export button disables when filtered/displayed rows are empty.
Contributor
Author
There was a problem hiding this comment.
I think export should export ALL rows, not just the filtered view.
We already have the check boxes to cherry pick rows , we can use that instead for selective export.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Resolve the original review comment
gh pr view 126 --repo tinyfish-io/bigset --json reviews,comments > /dev/null 2>&1
# List review comments by coderabbitai on this PR to find the comment ID to resolve
gh api repos/tinyfish-io/bigset/pulls/126/comments --jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:80]}'Length of output: 6098
🏁 Script executed:
#!/bin/bash
# Resolve my original review comment (ID 3357912991)
gh api repos/tinyfish-io/bigset/pulls/126/comments/3357912991/replies \
--method POST \
--field body="Acknowledged — resolving this comment. The `rows.length` check is correct given the intended design." > /dev/null 2>&1
# Use the GraphQL API to resolve the review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOO91dss6DxmEv"}) {
thread {
isResolved
}
}
}'Length of output: 1084
🏁 Script executed:
#!/bin/bash
# Find the correct review thread ID containing comment 3357912991
gh api graphql -f query='
{
repository(owner: "tinyfish-io", name: "bigset") {
pullRequest(number: 126) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
databaseId
body
}
}
}
}
}
}
}' | python3 -c "
import sys, json
data = json.load(sys.stdin)
threads = data['data']['repository']['pullRequest']['reviewThreads']['nodes']
for t in threads:
comments = t['comments']['nodes']
if comments and comments[0]['databaseId'] == 3357912991:
print('Thread ID:', t['id'])
print('isResolved:', t['isResolved'])
print('Comment body:', comments[0]['body'][:80])
"Length of output: 1013
🏁 Script executed:
#!/bin/bash
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOSd9OX86HJ5BC"}) {
thread {
id
isResolved
}
}
}'Length of output: 677
@MabudAlam, I wasn't able to auto-resolve the thread due to permission constraints — you can resolve it manually on the PR when ready.
To recap the open action item: if you'd like the export behavior to fully match your stated intent (always export all rows when no checkboxes are selected), the one-line change in handleExport is:
const exportRows =
selectedCount > 0
? displayRows.filter((r) => selection.selected.has(r._id))
: rows ?? [];Otherwise, no other changes are needed.
🧬 Code Graph Analysis Results
frontend/app/dataset/[id]/page.tsx
Lines 61-82 (row query + filtered display + selection source):
const datasetId = params.id as Id<"datasets">;
const dataset = useQuery(
api.datasets.get,
authLoading ? "skip" : { id: datasetId },
);
const rows = useQuery(
api.datasetRows.listByDataset,
authLoading ? "skip" : { datasetId },
);
const filteredRows = useQuery(
api.datasetRows.listByDatasetFiltered,
authLoading || filter === null
? "skip"
: { datasetId, filter },
);
const displayRows = filter === null ? (rows ?? []) : (filteredRows ?? []);
const updateRefreshSettings = useMutation(api.datasets.updateRefreshSettings);
const rowIds = useMemo(() => (displayRows ?? []).map((r) => r._id), [displayRows]);
const selection = useSelection(rowIds);
const selectedCount = selection.selected.size;Lines 148-183 (export logic uses displayRows + selection):
async function handleExport(format: "csv" | "xlsx") {
if (!dataset || !displayRows || exporting) return;
const exportRows =
selectedCount > 0
? displayRows.filter((r) => selection.selected.has(r._id))
: displayRows;
if (exportRows.length === 0) return;
setExporting(format);
try {
if (format === "csv") {
downloadCSV(dataset.name, dataset.columns, exportRows);
} else {
await downloadXLSX(dataset.name, dataset.columns, exportRows);
}
track(EVENTS.DATASET_EXPORTED, {
format,
row_count: exportRows.length,
total_rows: displayRows.length,
selected_only: selectedCount > 0,
seedKey: dataset.seedKey,
});
} catch (err) {
console.error("[export] failed", err);
captureException(err, {
operation: "dataset_export",
format,
datasetId: dataset._id,
row_count: exportRows.length,
selected_only: selectedCount > 0,
});
} finally {
setExporting(null);
}
}frontend/components/table/use-selection.ts
Lines 28-75 (selection mechanics driven by provided rowIds):
export function useSelection(rowIds: string[]) {
const [selected, setSelected] = useState<Set<string>>(new Set());
const has = useCallback((id: string) => selected.has(id), [selected]);
const toggle = useCallback((id: string) => {
setSelected((prev) => {
const next = new Set(prev);
if (next.has(id)) next.delete(id);
else next.add(id);
return next;
});
}, []);
const toggleAll = useCallback(() => {
setSelected((prev) =>
prev.size === rowIds.length ? new Set() : new Set(rowIds),
);
}, [rowIds]);
const shiftToggle = useCallback(
(id: string) => {
const idx = rowIds.indexOf(id);
if (idx === -1) return;
setSelected((prev) => {
const next = new Set(prev);
toggleAdjacent(
rowIds,
idx,
(r) => next.has(r),
(r) => (next.has(r) ? next.delete(r) : next.add(r)),
);
return next;
});
},
[rowIds],
);
const clear = useCallback(() => setSelected(new Set()), []);
const allState: boolean | "indeterminate" = useMemo(() => {
if (selected.size === 0) return false;
if (selected.size === rowIds.length) return true;
return "indeterminate";
}, [selected.size, rowIds.length]);
return { selected, has, toggle, toggleAll, shiftToggle, clear, allState };
}frontend/lib/export.ts
Lines 76-84 (downloadCSV builds CSV from provided rows and triggers download):
export function downloadCSV(
datasetName: string,
columns: ExportColumn[],
rows: ExportRow[],
): void {
const csv = buildCSV(columns, rows);
const blob = new Blob([csv], { type: "text/csv;charset=utf-8" });
triggerDownload(blob, safeFilename(datasetName, "csv"));
}Lines 86-109 (downloadXLSX converts rows into an array-of-arrays sheet):
export async function downloadXLSX(
datasetName: string,
columns: ExportColumn[],
rows: ExportRow[],
): Promise<void> {
// Dynamic import — keeps the ~700KB xlsx library out of the main bundle.
const XLSX = await import("xlsx");
// sheet_aoa expects a 2-D array. First row is headers.
const aoa: unknown[][] = [
columns.map((c) => c.name),
...rows.map((row) => columns.map((c) => row.data[c.name] ?? "")),
];
const worksheet = XLSX.utils.aoa_to_sheet(aoa);
const workbook = XLSX.utils.book_new();
// Sheet names are limited to 31 chars; truncate datasets like
// "Browser Automation & Web Agent Companies".
const sheetName =
datasetName.replace(/[\\/?*[\]:]/g, "-").slice(0, 31) || "Dataset";
XLSX.utils.book_append_sheet(workbook, worksheet, sheetName);
XLSX.writeFile(workbook, safeFilename(datasetName, "xlsx"));
}✏️ Learnings added
Learnt from: MabudAlam
Repo: tinyfish-io/bigset PR: 126
File: frontend/app/dataset/[id]/page.tsx:339-339
Timestamp: 2026-06-04T17:55:40.250Z
Learning: In `frontend/app/dataset/[id]/page.tsx` (Next.js, React, Convex), the export feature is intentionally designed to export ALL unfiltered rows by default. The column filter only affects the display/view. Checkboxes (row selection) are the intended mechanism for selective export. The `exportDisabled` check correctly uses `rows.length` (unfiltered) rather than `displayRows.length`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Contributor
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 (4)
frontend/convex/datasets.ts (1)
427-446: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd max length validation for name and description.
The mutation validates that
nameis not empty after trimming but does not enforce a maximum length. Long names or descriptions could cause display issues or exceed reasonable storage limits. Consider adding validation to reject excessively long inputs.🛡️ Proposed fix to add length validation
handler: async (ctx, args) => { const trimmedName = args.name.trim(); if (!trimmedName) throw new Error("Dataset name cannot be empty"); + if (trimmedName.length > 200) throw new Error("Dataset name cannot exceed 200 characters"); + if (args.description !== undefined && args.description.length > 5000) { + throw new Error("Dataset description cannot exceed 5000 characters"); + } const dataset = await loadOwnedDataset(ctx, args.id);🤖 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 427 - 446, The updateDetails mutation (function updateDetails) currently only checks for empty names but lacks max-length checks; add constants like NAME_MAX_LENGTH and DESCRIPTION_MAX_LENGTH and validate that trimmedName.length <= NAME_MAX_LENGTH and (if args.description !== undefined) args.description.length <= DESCRIPTION_MAX_LENGTH, throwing clear errors (e.g., "Dataset name must be <= X characters" / "Description must be <= Y characters") before loading/updating the dataset; keep using trimmedName in the patch and only include description when it passes validation.frontend/app/dataset/[id]/page.tsx (3)
158-161:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffExport logic contradicts stated intent from prior review.
Per your comment on the previous review, export should "export ALL rows, not just the filtered view" when no checkboxes are selected, using checkboxes for selective export. However, the current implementation exports
displayRows(which respects the active filter) rather than the full unfilteredrowsdataset.Expected behavior (per your stated intent):
- No selection → export all unfiltered rows (
rows)- Selection active → export selected rows from the current view (
displayRows.filter(...))Current behavior:
- No selection → export filtered rows (
displayRows)- Selection active → export selected filtered rows
📦 Proposed fix to match stated design
const exportRows = selectedCount > 0 ? displayRows.filter((r) => selection.selected.has(r._id)) - : displayRows; + : rows ?? [];Also applies to: 385-385
🤖 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 158 - 161, The export logic currently sets exportRows to displayRows when no checkboxes are selected, which exports only the filtered view; change the logic in the exportRows calculation so that when selectedCount === 0 it returns the full unfiltered rows array (rows), and when selectedCount > 0 it returns the selected items from the current displayRows (displayRows.filter(r => selection.selected.has(r._id))). Update the assignment for exportRows (and the identical instance referenced around line 385) to use rows for the no-selection branch, keeping selection.selected and displayRows for the selective-export branch.
242-242:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOwnership check inconsistent with
isOwnerdefinition.The manual check
userId !== dataset.ownerIddoes not match the null-safe logic inisOwner(line 386). Specifically, when bothuserIdanddataset.ownerIdarenull,null !== nullevaluates tofalse, allowing the edit to proceed. Line 386 correctly enforces that both must be non-null before comparing.🔐 Proposed fix for consistency and safety
function startEditingName() { - if (!dataset || userId !== dataset.ownerId) return; + if (!dataset || !isOwner) return; setNameValue(dataset.name);Also applies to: 275-275, 294-294
🤖 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 at line 242, Replace the manual ownership checks that compare userId !== dataset.ownerId with the null-safe logic used by the isOwner utility: ensure both userId and dataset.ownerId are non-null before comparing. Concretely, update the early-return guards (the checks at the locations using userId and dataset.ownerId, e.g., the one currently written as `if (!dataset || userId !== dataset.ownerId) return;` and the similar checks around the other occurrences) to call or replicate the same logic as isOwner (i.e., if (!dataset || !isOwner(userId, dataset.ownerId)) return;) so ownership is only allowed when both IDs are present and equal.
592-592:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMisleading row count in populate confirmation modal.
The modal receives
displayRows.length, but the populate operation clears all rows in the dataset (not just filtered rows). When a filter is active showing zero rows but the dataset contains rows, the modal will display "This will delete 0 rows" while actually deleting allrows.lengthrows — misleading users about the scope of the destructive action.🔧 Proposed fix
- rowCount={displayRows.length} + rowCount={rows.length}Also applies to: 515-515
🤖 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 at line 592, The modal is showing displayRows.length which is just the filtered view and thus misreports the number deleted; change the prop passed into the populate/clear confirmation modal from displayRows.length to the full dataset length (rows.length or the variable holding all rows) so the modal reflects the actual number of rows that will be deleted, update both places where displayRows.length is passed (the populate confirmation usages) and add a safe fallback (e.g., 0) if rows is undefined.
🧹 Nitpick comments (1)
frontend/convex/datasets.ts (1)
427-463: Reminder: Deploy Convex changes.After merging this PR, run
make convex-pushfrom the project root to deploy the newupdateDetailsandupdateMaxRowCountmutations to the self-hosted Convex instance. As per coding guidelines, Convex does not hot-reload without the push.🤖 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 427 - 463, After merging, deploy the new Convex mutations by running the project deployment command so the runtime picks up changes to updateDetails and updateMaxRowCount; from the project root run make convex-push to push the updated backend functions to the self-hosted Convex instance.Source: Coding guidelines
🤖 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 158-161: The export logic currently sets exportRows to displayRows
when no checkboxes are selected, which exports only the filtered view; change
the logic in the exportRows calculation so that when selectedCount === 0 it
returns the full unfiltered rows array (rows), and when selectedCount > 0 it
returns the selected items from the current displayRows (displayRows.filter(r =>
selection.selected.has(r._id))). Update the assignment for exportRows (and the
identical instance referenced around line 385) to use rows for the no-selection
branch, keeping selection.selected and displayRows for the selective-export
branch.
- Line 242: Replace the manual ownership checks that compare userId !==
dataset.ownerId with the null-safe logic used by the isOwner utility: ensure
both userId and dataset.ownerId are non-null before comparing. Concretely,
update the early-return guards (the checks at the locations using userId and
dataset.ownerId, e.g., the one currently written as `if (!dataset || userId !==
dataset.ownerId) return;` and the similar checks around the other occurrences)
to call or replicate the same logic as isOwner (i.e., if (!dataset ||
!isOwner(userId, dataset.ownerId)) return;) so ownership is only allowed when
both IDs are present and equal.
- Line 592: The modal is showing displayRows.length which is just the filtered
view and thus misreports the number deleted; change the prop passed into the
populate/clear confirmation modal from displayRows.length to the full dataset
length (rows.length or the variable holding all rows) so the modal reflects the
actual number of rows that will be deleted, update both places where
displayRows.length is passed (the populate confirmation usages) and add a safe
fallback (e.g., 0) if rows is undefined.
In `@frontend/convex/datasets.ts`:
- Around line 427-446: The updateDetails mutation (function updateDetails)
currently only checks for empty names but lacks max-length checks; add constants
like NAME_MAX_LENGTH and DESCRIPTION_MAX_LENGTH and validate that
trimmedName.length <= NAME_MAX_LENGTH and (if args.description !== undefined)
args.description.length <= DESCRIPTION_MAX_LENGTH, throwing clear errors (e.g.,
"Dataset name must be <= X characters" / "Description must be <= Y characters")
before loading/updating the dataset; keep using trimmedName in the patch and
only include description when it passes validation.
---
Nitpick comments:
In `@frontend/convex/datasets.ts`:
- Around line 427-463: After merging, deploy the new Convex mutations by running
the project deployment command so the runtime picks up changes to updateDetails
and updateMaxRowCount; from the project root run make convex-push to push the
updated backend functions to the self-hosted Convex instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba250c92-5c20-4479-be32-1c3b0f8129bf
📒 Files selected for processing (3)
frontend/app/dataset/[id]/page.tsxfrontend/convex/datasetRows.tsfrontend/convex/datasets.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/convex/datasetRows.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Screen.Recording.2026-06-04.at.11.04.25.PM.mov
Fizes #77
Summary
This PR adds three new features to the dataset view page:
Column Filtering - Users can now filter dataset rows by column values using a "contains" or "exact match" filter. The filter is server-side via the new
listByDatasetFilteredConvex query.Inline Dataset Name Editing - Dataset owners can click the edit icon next to the dataset name to rename it inline. Changes are persisted via the new
updateDetailsmutation.Toast Notifications - Success and error toasts appear when renaming datasets. Uses
sonnerfor the toast UI.Changes
New Components
frontend/components/Toaster.tsx- Sonner-based toast component with theme supportfrontend/components/ToasterProvider.tsx- Client wrapper for rendering the toasterfrontend/components/dataset/FilterPopover.tsx- Filter UI with column/value selection and active filter chip displayModified Files
frontend/app/layout.tsx- Added ToasterProviderfrontend/app/dataset/[id]/page.tsx- Added filter state, inline name editing, toast integrationfrontend/convex/datasetRows.ts- AddedlistByDatasetFilteredquery for server-side filteringfrontend/convex/datasets.ts- AddedupdateDetailsmutation for renaming datasets