-
Notifications
You must be signed in to change notification settings - Fork 122
Schema proposals beginnings #6836
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
🚀 Snapshot Release (
|
| Package | Version | Info |
|---|---|---|
@graphql-hive/cli |
0.57.0-alpha-20251210002603-00cddfa4df7c30b57b388c817428598d24c44c66 |
npm ↗︎ unpkg ↗︎ |
hive |
8.13.0-alpha-20251210002603-00cddfa4df7c30b57b388c817428598d24c44c66 |
npm ↗︎ unpkg ↗︎ |
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-6836.hive-storybook.pages.dev |
💻 Website PreviewThe latest changes are available as preview in: https://pr-6836.hive-landing-page.pages.dev |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
|
Will look tomorrow! |
jasonkuhrt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Left a few odd thoughts on the data model fwiw 🙌
packages/migrations/src/actions/2025.05.29T00.00.00.schema-proposals.ts
Outdated
Show resolved
Hide resolved
packages/migrations/src/actions/2025.05.29T00.00.00.schema-proposals.ts
Outdated
Show resolved
Hide resolved
packages/migrations/src/actions/2025.05.29T00.00.00.schema-proposals.ts
Outdated
Show resolved
Hide resolved
packages/migrations/src/actions/2025.05.29T00.00.00.schema-proposals.ts
Outdated
Show resolved
Hide resolved
| CREATE TABLE IF NOT EXISTS "schema_proposal_comments" | ||
| ( | ||
| id UUID PRIMARY KEY DEFAULT uuid_generate_v4 () | ||
| , user_id UUID REFERENCES users (id) ON DELETE SET NULL | ||
| , body TEXT NOT NULL | ||
| , created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| , updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW() | ||
| , schema_proposal_review_id UUID REFERENCES schema_proposal_reviews (id) ON DELETE CASCADE | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we bake in more semantics here rather than just free form? For example maybe a category enum.
type: kudos | confused | disagree | idea
The thinking is that structured semantics here would allow some interesting UX downstream, such as us being able to present the user with triaged comments e.g. "blocking" | "optional" etc. to reduce review process overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great idea but it might belong on the review object and is better suited to a v2.
For more context -- One constraint of the current table design is that every comment must be tied to a review. This is because I opted to use sql and I wanted to avoid complicated merging + pagination logic in the UI to display multiple sets or chronological data at the same time.
If using nosql then this could all be in the same table and have different structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was in the midst of factoring it out for a followup PR, but based on our discussions we want to move forward as is and adjust.
I'll add the import back
| function defaultSchemaProposalIdentity( | ||
| args: { schemaProposalId: string | null } & Parameters<typeof defaultTargetIdentity>[0], | ||
| ) { | ||
| const ids = defaultTargetIdentity(args); | ||
|
|
||
| if (args.schemaProposalId !== null) { | ||
| ids.push(`target/${args.targetId}/schemaProposal/${args.schemaProposalId}`); | ||
| } | ||
|
|
||
| return ids; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we introduce a new permission level here? This implies that we allow users to modify and view individual schema proposals within a target. 🤔
E.g. he that someone can see one specific schema proposal, isn't this permission on the target good enough for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
I was following what app deployments did without thinking. I've adjusted this.
Background
https://linear.app/the-guild/issue/CONSOLE-1245/collaborative-schema-designerproposer
Our goal is to build a space where developers/teams can come together and collaborate on schema. "Schema Proposals", as it's currently named, is a feature that would do just this. By allowing users to submit a proposed change and others to review and comment.
Description
This is not the full feature, but it lays some of the groundwork for this feature.
I have a proposed database structure, initial graphql API, and am gradually adding additional elements to this feature branch.