Skip to content

Conversation

@yuhgto
Copy link
Contributor

@yuhgto yuhgto commented May 8, 2025

Added sample context for developers writing code with LLM assistance. It makes it more likely for AI tools like Copilot or Cursor to write correct code for our SDKs and API.

@RomKadria Can you review these changes? This is my first time contributing to the API project. Happy to hear any feedback.

Description of changes -

  • Created new packages/llm-context folder
  • Added README.md
  • Added 2 rule files to it:
    • GraphQL API best practices
    • API client best practices

@yuhgto
Copy link
Contributor Author

yuhgto commented May 8, 2025

Validation that failed is unrelated to my changes AFAIK. npm run install works fine in my local environment.

Run yarn install
yarn install v1.22.21
[1/[4](https://github.com/mondaycom/monday-graphql-api/actions/runs/14912182160/job/41889042058?pr=73#step:5:5)] Resolving packages...
[2/4] Fetching packages...
error Error: https://registry.npmjs.org/text-table/-/text-table-0.2.0.tgz: Request failed "[5](https://github.com/mondaycom/monday-graphql-api/actions/runs/14912182160/job/41889042058?pr=73#step:5:6)00 Internal Server Error"
    at ResponseError.ExtendableBuiltin (/opt/hostedtoolcache/node/18.18.1/x[6](https://github.com/mondaycom/monday-graphql-api/actions/runs/14912182160/job/41889042058?pr=73#step:5:7)4/lib/node_modules/yarn/lib/cli.js:696:66)
    at new ResponseError (/opt/hostedtoolcache/node/18.18.1/x64/lib/node_modules/yarn/lib/cli.js:802:124)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
    at Request.<anonymous> (/opt/hostedtoolcache/node/18.18.1/x64/lib/node_modules/yarn/lib/cli.js:66218:16)
    at Request.emit (node:events:51[7](https://github.com/mondaycom/monday-graphql-api/actions/runs/14912182160/job/41889042058?pr=73#step:5:8):28)
    at module.exports.Request.onRequestResponse (/opt/hostedtoolcache/node/1[8](https://github.com/mondaycom/monday-graphql-api/actions/runs/14912182160/job/41889042058?pr=73#step:5:9).18.1/x64/lib/node_modules/yarn/lib/cli.js:141751:10)
    at ClientRequest.emit (node:events:517:28)
    at HTTPParser.parserOnIncomingClient (node:_http_client:700:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:11[9](https://github.com/mondaycom/monday-graphql-api/actions/runs/14912182160/job/41889042058?pr=73#step:5:10):17)
    at TLSSocket.socketOnData (node:_http_client:541:22)
    at TLSSocket.emit (node:events:517:28)
Error: Process completed with exit code 1.

@yuhgto
Copy link
Contributor Author

yuhgto commented May 12, 2025

@RomKadria I moved the rules to the root of the repo. Can you approve this? ⭐

data: {
loading: boolean; // Query-specific loading state
error: APIError | null; // Query-specific errors
data: T; // Your query results
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need it? the type are auto generated here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my testing, seems like Cursor only looks at the types exposed by a library when fixing bugs (rather than generating code). Explicitly declaring the shape should make it more reliable.

The main thing I wanted to document is that the return object has nesting – which is hard to infer from the package source because it uses a generic return type T.

// Structured error type
type APIError = {
message: string;
status: number;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are already exporting the error ourselves as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment. I'm willing to remove this but in my testing it resulted in worse code (Cursor would try to access the data in the API error wrong or hallucinate the return value)

@yuhgto
Copy link
Contributor Author

yuhgto commented May 16, 2025

@RomKadria Implemented all your feedback except the types one. Can we merge on Sunday/Monday?

@RomKadria RomKadria merged commit f3e7299 into mondaycom:main May 29, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants