Add DataGrip instructions to contributing README#2352
Add DataGrip instructions to contributing README#2352dimaMachina wants to merge 1 commit intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (2) 🟠⚠️
Inline Comments:
- 🟠 Major:
mdx-components.tsx:89Missing width/height on images causes CLS (Cumulative Layout Shift) - 🟠 Major:
index.mdx:9Missing prerequisite about runningpnpm setup-devbefore following the guide
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
mdx-components.tsx:91Missing fallback alt text whenprops.altis falsy - 🟡 Minor:
index.mdx:64Missing link to conceptual manage-database documentation explaining Dolt branches
💭 Consider (4) 💭
💭 1) index.mdx Novel image syntax pattern
Issue: The <></> syntax (fragment-wrapped markdown images with relative paths) is used exclusively in this file and doesn't appear anywhere else in the documentation.
Why: Other docs use the <Image src="/path" /> component with absolute paths from the public directory. This establishes a new undocumented pattern that may confuse future documentation authors.
Fix: Consider either (a) documenting this as the new preferred pattern for co-located images, or (b) using the established <Image> component pattern with images in public/images/.
Refs:
💭 2) index.mdx:5 Non-standard keywords frontmatter field
Issue: This file introduces a keywords frontmatter field that isn't present in any peer contributing docs.
Why: The other contributing docs (overview.mdx, manage-database.mdx, etc.) use only title, sidebarTitle, description, and icon. Adding keywords here creates inconsistency.
Fix: Either remove keywords to match peer conventions, or if SEO keywords are desired, add them consistently across contributing docs.
💭 3) index.mdx:56-62 "Go to DDL" step lacks explanation
Issue: Step 6 instructs users to click "Go to DDL" without explaining what DDL means or why this step is helpful.
Why: Contributors unfamiliar with DataGrip may not understand the purpose of this step in the workflow.
Fix: Add a brief explanation: "This opens the schema definition view where you can explore table structures and relationships."
💭 4) index.mdx Consider mentioning pnpm db:studio as alternative
Issue: The guide doesn't mention that pnpm db:studio exists as a lighter-weight alternative for quick database inspection.
Why: Contributors may not need a full IDE setup for simple queries. Mentioning the simpler option helps them choose the right tool.
Fix: Consider adding a note at the top: "For quick inspection, you can also use pnpm db:studio. This guide is for contributors who prefer a full SQL IDE with features like schema navigation and DDL viewing."
🚫 REQUEST CHANGES
Summary: Good documentation addition that complements the existing manage-database docs! The two major issues should be addressed before merging: (1) the mdx-components.tsx change needs width/height defaults to prevent layout shift, and (2) the guide needs a prerequisite callout about running pnpm setup-dev first. The minor and consider items would improve clarity but aren't blockers.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
mdx-components.tsx:91-98 |
Alt text duplication between alt attribute and figcaption |
Common pattern in documentation; screen readers handle this appropriately. The visible caption serves sighted users while alt serves assistive technology. |
index.mdx:71 |
Branch naming confusion with hyphens in project ID | After investigation, activities-planner appears to be a valid project ID format. The underscore delimiters are clear. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
3 | 0 | 0 | 0 | 2 | 0 | 1 |
pr-review-docs |
4 | 0 | 1 | 0 | 2 | 0 | 0 |
pr-review-consistency |
5 | 0 | 2 | 0 | 0 | 0 | 0 |
pr-review-product |
3 | 0 | 1 | 0 | 0 | 0 | 1 |
| Total | 15 | 0 | 4 | 0 | 4 | 0 | 2 |
| img(props) { | ||
| const ComponentToUse = typeof props.src === 'object' ? NextImage : 'img'; | ||
| const img = ( | ||
| <ComponentToUse {...props} className="rounded-lg border border-border w-auto mx-auto" /> |
There was a problem hiding this comment.
🟠 MAJOR: Missing width/height causes CLS
Issue: The new img handler doesn't pass through width/height props, unlike the Image component above (lines 79-80) which defaults to 1200×1200.
Why: Without explicit dimensions, images cause Cumulative Layout Shift (CLS) as they load. The new DataGrip guide has 8 images — users will see content jumping as each loads.
Fix:
| <ComponentToUse {...props} className="rounded-lg border border-border w-auto mx-auto" /> | |
| <ComponentToUse {...props} width={props.width ?? 1200} height={props.height ?? 1200} className="rounded-lg border border-border w-auto mx-auto" /> |
Refs:
| const img = ( | ||
| <ComponentToUse {...props} className="rounded-lg border border-border w-auto mx-auto" /> | ||
| ); | ||
| if (!props.alt) { |
There was a problem hiding this comment.
🟡 Minor: Missing fallback alt text
Issue: When props.alt is falsy, images render without any alt attribute. The Image component above (line 77) uses alt={props.alt ?? 'Image'} as a fallback.
Why: Images without alt are less accessible. While the DataGrip guide provides alt text for all images, future MDX authors may omit it.
Fix:
| if (!props.alt) { | |
| if (!props.alt) { | |
| return <ComponentToUse {...props} alt="" width={props.width ?? 1200} height={props.height ?? 1200} className="rounded-lg border border-border w-auto mx-auto" />; |
Refs:
| icon: LuDatabase | ||
| --- | ||
|
|
||
| <Steps> |
There was a problem hiding this comment.
🟠 MAJOR: Missing prerequisite — pnpm setup-dev
Issue: The guide jumps straight into DataGrip setup without mentioning that the local manage database must be running first.
Why: Contributors following this guide without running pnpm setup-dev will hit connection failures at Step 4 ("Test Connection") with no explanation. The Doltgres database on port 5432 only exists after setup-dev runs.
Fix: Add a prerequisites callout before Step 1:
<Note>
Before starting, ensure you've completed the [contributor setup](/community/contributing/overview) and run `pnpm setup-dev` to start the local databases.
</Note>
<Steps>Refs:
| <></> | ||
| </Step> | ||
| <Step> | ||
| ## Check out a Dolt branch and run a query |
There was a problem hiding this comment.
🟡 Minor: Missing link to conceptual documentation
Issue: This step introduces DOLT_CHECKOUT and the branch naming convention without linking to the existing manage-database.mdx page that explains why branch checkout is required.
Why: Contributors unfamiliar with Doltgres may not understand why this step is necessary. The conceptual page explains that each project has an isolated branch and connections default to main.
Fix: Add a brief explanation with a link:
## Check out a Dolt branch and run a query
The manage database uses [Doltgres branch isolation](/community/contributing/manage-database#branch-naming-convention). Each project has its own branch, so you must check out the correct branch before querying project data.
In the query console, check out the project branch first, then run your SQL.Refs:
Ito Test Report ✅17 test cases ran. 17 passed. This test run verified PR #2352 which adds a new DataGrip contributing guide page and refactors the global ✅ Passed (17)
📋 View Recording |
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
|
This pull request has been automatically closed due to inactivity. If you'd like to continue working on this, please:
Thank you for your understanding! |
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
|
This pull request has been automatically closed due to inactivity. If you'd like to continue working on this, please:
Thank you for your understanding! |
1 participant
No description provided.