Daily RC #532
Daily RC #532
Conversation
…ality - Removed unused pagination icons and integrated a new pagination structure using the `Pagination` component. - Implemented a `getVisiblePageRange` function to dynamically calculate visible page numbers and ellipses based on the current page index and total page count. - Updated the layout for improved responsiveness and user experience, including better spacing and alignment of pagination controls.
- Implemented a new DELETE route for campaigns, allowing for the removal of campaigns and associated data through the API. - Enhanced the campaign settings UI with a "Danger Zone" tab, featuring a delete confirmation dialog for irreversible actions. - Integrated project context and error handling for the delete operation, ensuring proper user feedback and navigation post-deletion. - Added tests to validate the campaign deletion process and ensure compliance with RLS policies.
WalkthroughThis pull request reorganizes package manifests across a monorepo, refactors empty-state UI components, introduces campaign deletion functionality with a new API route and settings UI, enhances data-table pagination with ellipsis support, and adds RLS testing for database campaigns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
5 Skipped Deployments
|
- Replaced the deprecated EmptyWelcome component with a new Empty component across multiple pages, enhancing the visual presentation of empty states. - Updated the CampaignsPage and WelcomeNewPage to utilize the new Empty component, providing a more cohesive user experience. - Modified the EmptyRowsRenderer in the grid scaffold to align with the new empty state design, ensuring consistent messaging and styling.
…erience - Replaced existing empty state implementations with a new unified Empty component across various pages, including Campaigns, Portal Presets, Tags, and Documents. - Enhanced visual presentation and messaging for empty states, ensuring a cohesive design throughout the application. - Updated related UI elements to utilize the new Empty component, improving overall usability and aesthetics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
editor/app/(workbench)/[org]/[proj]/(console)/(resources)/customers/page.tsx (1)
186-186:⚠️ Potential issue | 🟡 MinorTypo:
font-norma→font-normal.Pre-existing, but worth fixing while you're in this file.
- className="text-sm font-norma text-muted-foreground" + className="text-sm font-normal text-muted-foreground"editor/app/(workbench)/[org]/[proj]/(console)/(resources)/campaigns/page.tsx (1)
40-40:⚠️ Potential issue | 🟡 MinorRemove leftover
console.logstatement.This debug log will emit campaign data to the browser console in production.
🧹 Proposed fix
- console.log("campaigns", campaigns);.legacy/editor-packages/editor-preview-pip/package.json (1)
1-80:⚠️ Potential issue | 🟡 MinorChanges to
.legacy/directory should be avoided.As per coding guidelines, the
.legacydirectory should be fully ignored — no contributions should target it. Consider whether these package.json changes are intentional or if they were accidentally included in this PR.As per coding guidelines: "Fully ignore the
.legacydirectory - do not contribute to this directory."
🤖 Fix all issues with AI agents
In `@apps/docs/package.json`:
- Line 11: The "dev" script in package.json is invoking content:watch as if it
were a system binary; update the "dev" script entry so the npm script
"content:watch" is invoked via the package manager (e.g., prefix with "pnpm" or
"npm run") — locate the "dev" script string in package.json and change the
command that references "content:watch" so it runs as a script (for example
replacing content:watch with pnpm content:watch or npm run content:watch) while
keeping the rest of the command (e.g., "& pnpm start") intact.
In `@crates/math2/package.json`:
- Line 4: Update the package.json "description" field to accurately describe the
`@crates/math2` package (currently incorrectly "turbo ci rust wrapper"); open the
package.json and change the "description" value to a short, accurate summary
such as indicating it provides geometry and common math operations for Rust
(reference the package name "@crates/math2" and the "description" key to locate
the field).
In `@desktop/package.json`:
- Line 17: The "lint" npm script in package.json uses the removed --ext flag
(script name "lint"), which fails under ESLint v9; replace it with a glob-based
invocation or migrate to flat config: update the "lint" script to call eslint
with file globs that match your TypeScript files (e.g., patterns for **/*.ts and
**/*.tsx) or, if you intend to use legacy config, explicitly set ES
LINT_USE_FLAT_CONFIG=false and adjust the script, and if migrating to flat
config instead, move your file globs into the new flat config's "files" entries
and change the "lint" script to call eslint without --ext.
In
`@editor/app/`(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/_components/settings.tsx:
- Around line 664-678: The onDelete async handler for deleting a campaign (the
anonymous async function passed to onDelete that calls
fetch(`/private/west/campaigns/${campaign_id}`) using project.id in headers)
does not catch fetch rejections; wrap the fetch+response handling in a try/catch
so network errors are caught, call toast.error with a useful message (include
error.message when available), and return false on failure; keep the existing
JSON parse fallback for non-ok responses but ensure the catch block also returns
false after logging the error.
In `@editor/components/data-table/data-table-pagination.tsx`:
- Around line 23-49: The getVisiblePageRange function can omit the first (0) or
last (pageCount-1) page when left === 1 or right === pageCount-2; change the
logic so page 0 and pageCount-1 are always included and the ellipsis markers are
only inserted when there is a gap >1 between the edge pages and the central
range. Concretely, in getVisiblePageRange compute left and right as now, but
iterate the central loop from Math.max(1, left) to Math.min(pageCount - 2,
right), always push 0 at the start and pageCount - 1 at the end, and only push
"ellipsis-left" if left > 1 and "ellipsis-right" if right < pageCount - 2 so the
first/last pages are never dropped (refer to getVisiblePageRange, left, right,
showLeftEllipsis, showRightEllipsis).
In `@packages/grida-mixed-properties/package.json`:
- Line 10: Replace the misspelled dependency key "jsonsschema" with the correct
package name "jsonschema" in the package.json dependencies list (look for the
string "jsonsschema" in packages/grida-mixed-properties/package.json) so the
project depends on the correct npm package; after updating, run your package
manager install to verify resolution.
In `@packages/react-p-queue/package.json`:
- Around line 33-38: The package currently builds both CJS and ESM via the
"build" and "dev" tsup scripts which will emit require('p-queue') for CJS and
break at runtime because p-queue v7 is ESM-only; fix by either removing the CJS
target from the scripts (stop emitting cjs in the tsup command) or configure
tsup to bundle p-queue into outputs by adding a tsup config with noExternal:
['p-queue'] (or similar) so the CJS output does not require('p-queue') at
runtime—update the "build"/"dev" scripts or add tsup.config.ts accordingly and
keep dependency "p-queue" unchanged.
In `@supabase/tests/grida_west_referral_campaign_rls_test_test.sql`:
- Line 2: Add two pgTAP DELETE isolation tests to
supabase/tests/grida_west_referral_campaign_rls_test_test.sql that assert
insider cannot delete acme's campaign and alice cannot delete local's campaign,
and update SELECT plan(...) from 10 to 12; locate the test file and add two new
tests (following the existing INSERT/UPDATE test style and using the same
user-switching/helper pattern used by the existing tests) that attempt the
DELETE operations and expect them to fail under RLS.
🧹 Nitpick comments (4)
editor/app/(workbench)/[org]/[proj]/[id]/data/(data)/table/~new/page.tsx (1)
37-47: Remove redundant icon sizing class for consistency.
TableIconhas an explicitclassName="size-6", butEmptyMedia variant="icon"is designed to automatically applysize-6to SVG children without an explicit size class via the CSS selector[&_svg:not([class*='size-'])]:size-6. Explicit sizing is unnecessary here since the component's variant already handles it.♻️ Suggested fix
<EmptyMedia variant="icon"> - <TableIcon className="size-6" /> + <TableIcon /> </EmptyMedia>editor/app/(api)/private/west/campaigns/[campaign_id]/route.ts (1)
60-66: Consider logging thecampaign_idandproject_idon failure for operational debugging.The current log on Line 61 only includes the Supabase error object. Adding the identifiers would help trace issues in production.
Proposed tweak
- console.error("Failed to delete campaign", docErr); + console.error("Failed to delete campaign", { campaign_id, project_id, error: docErr });supabase/tests/grida_west_referral_campaign_rls_test_test.sql (1)
76-81:set_configwithfalse(non-local) — intentional but worth noting.Using
set_config(..., false)makes these GUCs session-scoped rather than transaction-local. This works here because the entire script runs inside a singleBEGIN/ROLLBACKblock, but if the test harness ever changes to not roll back, these would leak. Thetrue(local) flag used intest_set_authis the safer pattern — consider using it here too for consistency.packages/grida-tokens/package.json (1)
34-38: The./kexport alias maps tolocales— consider a comment or README note.The export path
"./k"resolving todist/locales.*is non-obvious to consumers. Ifkis a project convention (e.g., for i18n keys), a brief note in the package README would help discoverability.
| "postinstall": "pnpm content:setup", | ||
| "content:watch": "node ./scripts/watch-docs.js", | ||
| "deploy": "docusaurus deploy", | ||
| "dev": "content:watch & pnpm start", |
There was a problem hiding this comment.
content:watch script invocation is missing the pnpm prefix — the dev script will fail.
content:watch is not a system binary; it's an npm script defined in this same package.json. Without pnpm (or npm run) it won't be found at runtime.
Proposed fix
- "dev": "content:watch & pnpm start",
+ "dev": "pnpm content:watch & pnpm start",📝 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.
| "dev": "content:watch & pnpm start", | |
| "dev": "pnpm content:watch & pnpm start", |
🤖 Prompt for AI Agents
In `@apps/docs/package.json` at line 11, The "dev" script in package.json is
invoking content:watch as if it were a system binary; update the "dev" script
entry so the npm script "content:watch" is invoked via the package manager
(e.g., prefix with "pnpm" or "npm run") — locate the "dev" script string in
package.json and change the command that references "content:watch" so it runs
as a script (for example replacing content:watch with pnpm content:watch or npm
run content:watch) while keeping the rest of the command (e.g., "& pnpm start")
intact.
| "name": "@crates/math2", | ||
| "description": "turbo ci rust wrapper", | ||
| "private": true, | ||
| "description": "turbo ci rust wrapper", |
There was a problem hiding this comment.
Incorrect package description.
The description "turbo ci rust wrapper" doesn't match the package name @crates/math2. Based on learnings, the math2 crate is used for geometry and common math operations, not for CI/turbo tooling.
📝 Suggested description fix
- "description": "turbo ci rust wrapper",
+ "description": "Geometry and common math operations library",📝 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.
| "description": "turbo ci rust wrapper", | |
| "description": "Geometry and common math operations library", |
🤖 Prompt for AI Agents
In `@crates/math2/package.json` at line 4, Update the package.json "description"
field to accurately describe the `@crates/math2` package (currently incorrectly
"turbo ci rust wrapper"); open the package.json and change the "description"
value to a short, accurate summary such as indicating it provides geometry and
common math operations for Rust (reference the package name "@crates/math2" and
the "description" key to locate the field).
| "scripts": { | ||
| "dev": "electron-forge start", | ||
| "package": "electron-forge package", | ||
| "lint": "eslint --ext .ts,.tsx .", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ESLint 9 --ext flag removed flat config
💡 Result:
In ESLint v9, flat config is the default, and the CLI flag --ext is not supported with flat config. Instead of telling ESLint which extensions to include via --ext, you must define what files ESLint should lint using files globs in eslint.config.* (or pass explicit globs on the command line). [1], [2]
What to do instead of --ext .ts,.tsx:
// eslint.config.js
export default [
{
files: ["**/*.ts", "**/*.tsx"],
},
];ESLint uses the files entries in your flat config to decide which files are linted. [2]
If you must keep using --ext: switch back to legacy eslintrc mode by setting ESLINT_USE_FLAT_CONFIG=false (flat config is default in v9). [3]
Sources:
[1] ESLint config migration guide (CLI flags not supported with flat config) (eslint.org)
[2] Migration guide section explaining --ext replacement with files globs (runebook.dev)
[3] ESLint v9 migration docs (flat config default; env var to use eslintrc) (eslint.org)
Citations:
- 1: https://eslint.org/docs/latest/use/configure/migration-guide?utm_source=openai
- 2: https://runebook.dev/en/docs/eslint/use/configure/migration-guide?utm_source=openai
- 3: https://eslint.org/docs/latest/use/migrate-to-9.0.0?utm_source=openai
🏁 Script executed:
# Check the ESLint version in desktop/package.json
cat desktop/package.json | grep -A 20 "devDependencies"Repository: gridaco/grida
Length of output: 936
🏁 Script executed:
# Get full package.json devDependencies to see ESLint version
cat desktop/package.json | grep -A 50 "devDependencies" | grep eslint
# Check for ESLint config files in desktop directory
ls -la desktop/eslint.config.* 2>/dev/null || echo "No eslint.config files found"
# Check if there's an .eslintrc in desktop
ls -la desktop/.eslintrc* 2>/dev/null || echo "No .eslintrc files found"Repository: gridaco/grida
Length of output: 310
🏁 Script executed:
# Read the ESLint config file to understand if flat config or legacy mode
cat desktop/.eslintrc.jsonRepository: gridaco/grida
Length of output: 408
--ext flag is not supported in ESLint v9.
ESLint 9 defaults to flat config, which removed support for the --ext CLI flag. This lint script will fail or ignore the extension filter. Even with the legacy .eslintrc.json present, you need to explicitly set ESLINT_USE_FLAT_CONFIG=false to use it, which defeats the purpose of upgrading.
Either use glob patterns directly or migrate to flat config with files globs:
Using glob patterns (simpler)
- "lint": "eslint --ext .ts,.tsx .",
+ "lint": "eslint '**/*.{ts,tsx}'",📝 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.
| "lint": "eslint --ext .ts,.tsx .", | |
| "lint": "eslint '**/*.{ts,tsx}'", |
🤖 Prompt for AI Agents
In `@desktop/package.json` at line 17, The "lint" npm script in package.json uses
the removed --ext flag (script name "lint"), which fails under ESLint v9;
replace it with a glob-based invocation or migrate to flat config: update the
"lint" script to call eslint with file globs that match your TypeScript files
(e.g., patterns for **/*.ts and **/*.tsx) or, if you intend to use legacy
config, explicitly set ES LINT_USE_FLAT_CONFIG=false and adjust the script, and
if migrating to flat config instead, move your file globs into the new flat
config's "files" entries and change the "lint" script to call eslint without
--ext.
| onDelete={async () => { | ||
| const res = await fetch( | ||
| `/private/west/campaigns/${campaign_id}`, | ||
| { | ||
| method: "DELETE", | ||
| headers: { | ||
| "x-grida-editor-user-current-project-id": | ||
| project.id.toString(), | ||
| }, | ||
| } | ||
| ); | ||
| if (!res.ok) { | ||
| const body = await res.json().catch(() => ({})); | ||
| toast.error(body.error ?? "Failed to delete campaign"); | ||
| return false; |
There was a problem hiding this comment.
Unhandled network errors in the delete handler.
If fetch itself rejects (e.g., network failure), the error propagates as an unhandled rejection since there's no try/catch around the fetch call. The .catch(() => ({})) on Line 676 only guards the JSON parse, not the fetch.
Proposed fix
onDelete={async () => {
+ try {
const res = await fetch(
`/private/west/campaigns/${campaign_id}`,
{
method: "DELETE",
headers: {
"x-grida-editor-user-current-project-id":
project.id.toString(),
},
}
);
if (!res.ok) {
const body = await res.json().catch(() => ({}));
toast.error(body.error ?? "Failed to delete campaign");
return false;
}
toast.success("Campaign deleted");
router.push(
`/${project.organization_name}/${project.name}/campaigns`
);
return true;
+ } catch {
+ toast.error("Network error — please try again");
+ return false;
+ }
}}🤖 Prompt for AI Agents
In
`@editor/app/`(workbench)/[org]/[proj]/(console)/(campaign)/campaigns/[campaign]/_components/settings.tsx
around lines 664 - 678, The onDelete async handler for deleting a campaign (the
anonymous async function passed to onDelete that calls
fetch(`/private/west/campaigns/${campaign_id}`) using project.id in headers)
does not catch fetch rejections; wrap the fetch+response handling in a try/catch
so network errors are caught, call toast.error with a useful message (include
error.message when available), and return false on failure; keep the existing
JSON parse fallback for non-ok responses but ensure the catch block also returns
false after logging the error.
| function getVisiblePageRange( | ||
| pageIndex: number, | ||
| pageCount: number, | ||
| siblingCount = 1 | ||
| ): (number | "ellipsis-left" | "ellipsis-right")[] { | ||
| // When empty (0 rows), still show page 1 | ||
| if (pageCount <= 0) return [0]; | ||
| const totalVisible = siblingCount * 2 + 3; // e.g. 1 ... 4 5 6 ... 10 | ||
| if (pageCount <= totalVisible) { | ||
| return Array.from({ length: pageCount }, (_, i) => i); | ||
| } | ||
| const left = Math.max(0, pageIndex - siblingCount); | ||
| const right = Math.min(pageCount - 1, pageIndex + siblingCount); | ||
| const showLeftEllipsis = left > 1; | ||
| const showRightEllipsis = right < pageCount - 2; | ||
| const pages: (number | "ellipsis-left" | "ellipsis-right")[] = []; | ||
| if (showLeftEllipsis) { | ||
| pages.push(0, "ellipsis-left"); | ||
| } | ||
| for (let i = left; i <= right; i++) { | ||
| pages.push(i); | ||
| } | ||
| if (showRightEllipsis) { | ||
| pages.push("ellipsis-right", pageCount - 1); | ||
| } | ||
| return pages; | ||
| } |
There was a problem hiding this comment.
Bug: first and/or last page can be omitted from the visible range.
When left === 1, showLeftEllipsis is false (since 1 > 1 is false), but the loop starts at 1, so page 0 is never emitted. Symmetrically, when right === pageCount - 2, showRightEllipsis is false, but the loop ends before pageCount - 1, so the last page is dropped.
Example: pageCount=6, pageIndex=2 → left=1, right=3, no left ellipsis, loop produces [1,2,3, "…", 5] — page 0 is missing.
Example: pageCount=6, pageIndex=3 → left=2, right=4, no right ellipsis, output is [0, "…", 2,3,4] — page 5 is missing.
The fix is to always include page 0 and pageCount - 1, and only insert ellipsis markers between them:
🐛 Proposed fix
function getVisiblePageRange(
pageIndex: number,
pageCount: number,
siblingCount = 1
): (number | "ellipsis-left" | "ellipsis-right")[] {
if (pageCount <= 0) return [0];
const totalVisible = siblingCount * 2 + 3;
if (pageCount <= totalVisible) {
return Array.from({ length: pageCount }, (_, i) => i);
}
const left = Math.max(0, pageIndex - siblingCount);
const right = Math.min(pageCount - 1, pageIndex + siblingCount);
const showLeftEllipsis = left > 1;
const showRightEllipsis = right < pageCount - 2;
const pages: (number | "ellipsis-left" | "ellipsis-right")[] = [];
- if (showLeftEllipsis) {
- pages.push(0, "ellipsis-left");
- }
- for (let i = left; i <= right; i++) {
- pages.push(i);
- }
- if (showRightEllipsis) {
- pages.push("ellipsis-right", pageCount - 1);
- }
+ // Always include first page
+ pages.push(0);
+ if (showLeftEllipsis) {
+ pages.push("ellipsis-left");
+ }
+ for (let i = Math.max(left, 1); i <= Math.min(right, pageCount - 2); i++) {
+ pages.push(i);
+ }
+ if (showRightEllipsis) {
+ pages.push("ellipsis-right");
+ }
+ // Always include last page
+ if (pageCount > 1) {
+ pages.push(pageCount - 1);
+ }
return pages;
}📝 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.
| function getVisiblePageRange( | |
| pageIndex: number, | |
| pageCount: number, | |
| siblingCount = 1 | |
| ): (number | "ellipsis-left" | "ellipsis-right")[] { | |
| // When empty (0 rows), still show page 1 | |
| if (pageCount <= 0) return [0]; | |
| const totalVisible = siblingCount * 2 + 3; // e.g. 1 ... 4 5 6 ... 10 | |
| if (pageCount <= totalVisible) { | |
| return Array.from({ length: pageCount }, (_, i) => i); | |
| } | |
| const left = Math.max(0, pageIndex - siblingCount); | |
| const right = Math.min(pageCount - 1, pageIndex + siblingCount); | |
| const showLeftEllipsis = left > 1; | |
| const showRightEllipsis = right < pageCount - 2; | |
| const pages: (number | "ellipsis-left" | "ellipsis-right")[] = []; | |
| if (showLeftEllipsis) { | |
| pages.push(0, "ellipsis-left"); | |
| } | |
| for (let i = left; i <= right; i++) { | |
| pages.push(i); | |
| } | |
| if (showRightEllipsis) { | |
| pages.push("ellipsis-right", pageCount - 1); | |
| } | |
| return pages; | |
| } | |
| function getVisiblePageRange( | |
| pageIndex: number, | |
| pageCount: number, | |
| siblingCount = 1 | |
| ): (number | "ellipsis-left" | "ellipsis-right")[] { | |
| // When empty (0 rows), still show page 1 | |
| if (pageCount <= 0) return [0]; | |
| const totalVisible = siblingCount * 2 + 3; // e.g. 1 ... 4 5 6 ... 10 | |
| if (pageCount <= totalVisible) { | |
| return Array.from({ length: pageCount }, (_, i) => i); | |
| } | |
| const left = Math.max(0, pageIndex - siblingCount); | |
| const right = Math.min(pageCount - 1, pageIndex + siblingCount); | |
| const showLeftEllipsis = left > 1; | |
| const showRightEllipsis = right < pageCount - 2; | |
| const pages: (number | "ellipsis-left" | "ellipsis-right")[] = []; | |
| // Always include first page | |
| pages.push(0); | |
| if (showLeftEllipsis) { | |
| pages.push("ellipsis-left"); | |
| } | |
| for (let i = Math.max(left, 1); i <= Math.min(right, pageCount - 2); i++) { | |
| pages.push(i); | |
| } | |
| if (showRightEllipsis) { | |
| pages.push("ellipsis-right"); | |
| } | |
| // Always include last page | |
| if (pageCount > 1) { | |
| pages.push(pageCount - 1); | |
| } | |
| return pages; | |
| } |
🤖 Prompt for AI Agents
In `@editor/components/data-table/data-table-pagination.tsx` around lines 23 - 49,
The getVisiblePageRange function can omit the first (0) or last (pageCount-1)
page when left === 1 or right === pageCount-2; change the logic so page 0 and
pageCount-1 are always included and the ellipsis markers are only inserted when
there is a gap >1 between the edge pages and the central range. Concretely, in
getVisiblePageRange compute left and right as now, but iterate the central loop
from Math.max(1, left) to Math.min(pageCount - 2, right), always push 0 at the
start and pageCount - 1 at the end, and only push "ellipsis-left" if left > 1
and "ellipsis-right" if right < pageCount - 2 so the first/last pages are never
dropped (refer to getVisiblePageRange, left, right, showLeftEllipsis,
showRightEllipsis).
| "aggregator", | ||
| "grida", | ||
| "json", | ||
| "jsonsschema", |
There was a problem hiding this comment.
Typo in keyword: "jsonsschema" → "jsonschema".
Proposed fix
- "jsonsschema",
+ "jsonschema",📝 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.
| "jsonsschema", | |
| "jsonschema", |
🤖 Prompt for AI Agents
In `@packages/grida-mixed-properties/package.json` at line 10, Replace the
misspelled dependency key "jsonsschema" with the correct package name
"jsonschema" in the package.json dependencies list (look for the string
"jsonsschema" in packages/grida-mixed-properties/package.json) so the project
depends on the correct npm package; after updating, run your package manager
install to verify resolution.
| "build": "tsup index.tsx --format cjs,esm --dts", | ||
| "dev": "tsup index.tsx --format cjs,esm --dts --watch", | ||
| "test": "vitest run" | ||
| }, | ||
| "dependencies": { | ||
| "p-queue": "^7.4.1" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
p-queue version 7 ESM only package
💡 Result:
Yes—p-queue v7 is ESM-only (“native ESM” / “no longer provides a CommonJS export”). [1], [2] The v7.0.0 release notes explicitly call it a breaking change: “This package is now pure ESM.” [3]
If you need CommonJS, your options are:
- Stay on p-queue v6 (last CJS-compatible major), or use dynamic
import()in CJS. [1] - Use a compatibility/fork package such as p-queue-compat or @esm2cjs/p-queue (ESM + CJS). [4], [5]
Sources: [1] [2] [3] [4] [5]
🌐 Web query:
tsup externalize dependencies default behavior
💡 Result:
tsup’s default behavior is to treat packages listed in your package.json dependencies as external—i.e., they are not bundled into the output. You only need to use external if you want to externalize additional imports beyond what’s in dependencies. [1]
When you enable bundling of node_modules (e.g., --bundle), dependencies are still always excluded/externalized. [2]
Sources: [1] [2]
🌐 Web query:
tsup CJS build with ESM-only dependency bundling
💡 Result:
If your CJS output ends up doing require("some-esm-only-dep"), it will fail because require() treats targets as CommonJS and can’t load ESM the normal way (Node recommends using import() instead). [1]
Make tsup bundle the ESM-only dependency into your CJS build
Mark the ESM-only package as not external (i.e., force-bundle it) with noExternal:
// tsup.config.ts
import { defineConfig } from "tsup";
export default defineConfig({
entry: ["src/index.ts"],
format: ["cjs"],
// keep most node_modules external, but bundle the ESM-only one(s)
skipNodeModulesBundle: true,
noExternal: ["esm-only-dep"],
});noExternal= “Always bundle modules matching given patterns” [2]skipNodeModulesBundle= “Skip node_modules bundling (will still bundle modules matchingnoExternal)” [2]- If you instead mark something as external, esbuild will preserve the import and emit a
require(...)in CJS, which is exactly what you don’t want for ESM-only deps. [3]
If bundling isn’t possible
If the dependency can’t be bundled (native addons, runtime resolution tricks, etc.), the fallback is to keep it external and load it from CJS via import() (async) as Node suggests. [1]
Sources:
[1] Node.js ESM docs on require vs import() https://nodejs.org/download/release/v17.7.1/docs/api/esm.html
[2] tsup Options (noExternal, skipNodeModulesBundle) https://www.jsdocs.io/package/tsup
[3] esbuild external behavior (preserves import; uses require for CJS) https://esbuild.github.io/api/
p-queue v7 is ESM-only — CJS build will break at runtime.
p-queue ≥ 7 ships only ESM. By default, tsup externalizes dependencies, so the CJS output will emit require('p-queue') which fails at runtime. Either bundle p-queue into the output via noExternal in a tsup.config.ts, or drop the CJS format since this is a React library.
🤖 Prompt for AI Agents
In `@packages/react-p-queue/package.json` around lines 33 - 38, The package
currently builds both CJS and ESM via the "build" and "dev" tsup scripts which
will emit require('p-queue') for CJS and break at runtime because p-queue v7 is
ESM-only; fix by either removing the CJS target from the scripts (stop emitting
cjs in the tsup command) or configure tsup to bundle p-queue into outputs by
adding a tsup config with noExternal: ['p-queue'] (or similar) so the CJS output
does not require('p-queue') at runtime—update the "build"/"dev" scripts or add
tsup.config.ts accordingly and keep dependency "p-queue" unchanged.
| @@ -0,0 +1,262 @@ | |||
| BEGIN; | |||
| SELECT plan(10); | |||
There was a problem hiding this comment.
Missing DELETE isolation tests — coding guidelines require insert/update/delete coverage.
The plan covers READ (6), INSERT (2), and UPDATE (2) isolation, but DELETE isolation is absent. Add at least two tests verifying that insider cannot delete acme's campaign and alice cannot delete local's campaign, then bump the plan count accordingly.
As per coding guidelines, "Add pgTAP test coverage for every table that alters RLS, permissions, or tenant boundaries, including at minimum read isolation tests and write isolation tests (insert/update/delete)".
🤖 Prompt for AI Agents
In `@supabase/tests/grida_west_referral_campaign_rls_test_test.sql` at line 2, Add
two pgTAP DELETE isolation tests to
supabase/tests/grida_west_referral_campaign_rls_test_test.sql that assert
insider cannot delete acme's campaign and alice cannot delete local's campaign,
and update SELECT plan(...) from 10 to 12; locate the test file and add two new
tests (following the existing INSERT/UPDATE test style and using the same
user-switching/helper pattern used by the existing tests) that attempt the
DELETE operations and expect them to fail under RLS.
1 participant
New Features
Documentation
Tests