feat(canvas): add Canvas LMS - #487
Open
Dhirenderchoudhary wants to merge 10 commits into
Open
Conversation
Contributor
|
@Dhirenderchoudhary is attempting to deploy a commit to the corsair Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
Greptile SummaryCanvas LMS integration now includes:
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Caller["Canvas endpoint caller"] --> Input["Validate operation input"]
Input --> BaseURL["Resolve plugin or account base URL"]
BaseURL --> HTTP["Canvas REST / GraphQL request"]
HTTP --> Output["Validate operation response"]
Output --> Result["Return typed result"]
Canvas["Canvas Live Event"] --> Match["Match event and tenant"]
Match --> Verify["Verify HMAC signature"]
Verify --> Trigger["Dispatch webhook trigger"]
Reviews (13): Last reviewed commit: "feat(canvas): add Canvas API response sc..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Plugin PR scorecard —
|
| Check | Status | Notes |
|---|---|---|
| R1 — Scope: plugin files only | ✅ | |
| R2 — Tests with assertions | ✅ | |
| R3 — Description complete | ✅ | |
| R3 — Linked issue / claim | ✅ | |
| R4 — Demo video / recording | ✅ |
Rules: PLUGIN_PR_RULES.md · re-runs on every push
|
Hey @Dhirenderchoudhary, thanks for the contribution! 🏴☠️ Before a maintainer reviews, please fix the items below — the review re-runs automatically on your next push. Must fix
Rule Used: Flag boilerplate residue from the plugin generator... (source)
Rule Used: Verify the implementation matches the PR descripti... (source)
Rule Used: Every endpoint must validate inputs and outputs wi... (source)
Rule Used: Every endpoint must validate inputs and outputs wi... (source)
Rule Used: Every endpoint must validate inputs and outputs wi... (source)
Rule Used: Flag boilerplate residue from the plugin generator... (source)
Rule Used: Flag Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Rule Used: Flag boilerplate residue from the plugin generator... (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Rule Used: Flag boilerplate residue from the plugin generator... (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! PR requirements (rules)
If anything remains after your next push, a bot commit will clean it up; a maintainer always does the final review and merge. |
github-actions
Bot
added
bot:round-2
|
Remaining findings are being fixed by a bot commit — it will be re-reviewed automatically. |
Maintainer review neededAutomated rounds are exhausted. Remaining findings:
Knowledge Base Used: The provider-plugin package pattern |
github-actions
Bot
added
the
needs-maintainer
Dhirenderchoudhary
removed
the
needs-maintainer
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a Canvas LMS provider with typed REST and GraphQL operations, authenticated requests, endpoint schemas, API-key and OAuth support, webhook verification and triggers, tenant resolution, packaging, tests, and provider registration. ChangesCanvas operation contracts
Canvas endpoint execution
Canvas plugin wiring and packaging
Canvas webhook support
Validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Dhirenderchoudhary
force-pushed
the
feat/canvas-plugin
branch
from
August 3, 2026 21:30
5624ff9 to
b0f22ad
Compare
Collaborator
Author
|
@greptile review |
Dhirenderchoudhary
removed
ci
Collaborator
Author
|
@greptile review |
![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
🧹 Nitpick comments (9)
packages/corsair/core/constants.ts (1)
220-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
'canvas'toAllProvidersfor autocomplete consistency.No exhaustive
AllProvidershandling exists, and(string & {})keeps the type open. The omission does not affect assignability or exhaustive switches.🤖 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 `@packages/corsair/core/constants.ts` around lines 220 - 316, Update the AllProviders type to include the literal provider value 'canvas' alongside the existing provider names, preserving the open (string & {}) fallback and current type behavior.packages/canvas/index.ts (1)
578-579: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
canvasOperationsimport to the top of the file.The import sits between declarations at Line 579. ES module imports hoist, so the behavior is correct. The placement still breaks the convention used by the other imports in this file and can confuse readers and tooling.
🤖 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 `@packages/canvas/index.ts` around lines 578 - 579, Move the canvasOperations import from its current position between declarations to the file’s top import section, alongside the other module imports. Do not change how canvasOperations is used.packages/canvas/endpoints/operations.ts (2)
619-628: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate
deleteAMessageoperation.
deleteAMessagedeclares the same method and the same path asdeleteConversationMessages. Two operation names for one Canvas endpoint increase the public surface without adding capability.♻️ Proposed removal
deleteConversationMessages: { method: 'POST', path: '/api/v1/conversations/{conversation_id}/remove_messages', description: 'Delete specific messages from a Canvas conversation', }, - deleteAMessage: { - method: 'POST', - path: '/api/v1/conversations/{conversation_id}/remove_messages', - description: 'Delete messages from a Canvas conversation', - },Also remove
deleteAMessagefrom theConversationsgroup inpackages/canvas/endpoints/index.tsand fromcanvasEndpointsNested.conversationsinpackages/canvas/index.ts.🤖 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 `@packages/canvas/endpoints/operations.ts` around lines 619 - 628, Remove the duplicate deleteAMessage operation from the endpoint definitions, and remove its references from the Conversations group and canvasEndpointsNested.conversations. Retain deleteConversationMessages as the sole operation for this method and path.
63-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that GraphQL operations require a caller-supplied
querydocument.Many entries map to
POST /api/graphqlwith the same method and path. The factory sends onlyinput.bodyto that path. ThereforegetAccountGraphQl,getAssignment2,createAssignmentGraphQl, andgetLegacyNodeare behaviorally identical passthroughs. The descriptions state that each operation retrieves or creates a specific resource. That is only true when the caller supplies the correct GraphQLqueryandvariablesin the body.Two options improve this:
- Add the GraphQL document to the operation metadata and merge it in the factory.
- Update the descriptions to state that the caller must supply
queryandvariables.Also applies to: 223-228, 244-248, 1210-1215
🤖 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 `@packages/canvas/endpoints/operations.ts` around lines 63 - 68, Update the descriptions for getAccountGraphQl, getAssignment2, createAssignmentGraphQl, and getLegacyNode to state that callers must supply the appropriate GraphQL query document and variables in input.body. Preserve their existing method and path metadata; do not claim resource-specific behavior is built in unless the factory also supplies the GraphQL document.packages/canvas/endpoints/factory.ts (1)
12-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the
base_urlkey getter instead of castingctx.keys.
canvasAuthConfigdeclaresaccount: ['base_url']for both auth types inpackages/canvas/index.ts(Lines 105-112). The generated key context should therefore expose thebase_urlgetter. The cast to{ get_base_url?: () => Promise<string | null | undefined> }hides that contract. If the generated getter name changes, the cast keeps compiling and the fallback silently returnsundefined.Use the typed
KeyBuilderContextshape for Canvas here, or export a small typed helper frompackages/canvas/index.ts.🤖 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 `@packages/canvas/endpoints/factory.ts` around lines 12 - 28, Update resolveCanvasBaseUrl to use the generated KeyBuilderContext type for ctx.keys, preserving the base_url getter contract declared by canvasAuthConfig instead of casting to an ad hoc getter shape. Reuse the existing Canvas typing from canvas/index.ts or export a focused typed helper there, and keep the current option/account fallback behavior unchanged.packages/canvas/jest.config.cjs (1)
5-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the copied patterns that do not apply to this package.
collectCoverageFromexcludesjest.config.ts, but this file isjest.config.cjs, so the exclusion never applies. The**/plugins/**and**/setup/**entries intestMatchtarget directories that this package does not contain. The**/*.test.tspattern already coversschema.test.ts.♻️ Proposed cleanup
testMatch: [ '**/*.test.ts', '**/tests/**/*.test.ts', - '**/plugins/**/*.test.ts', - '**/setup/**/*.test.ts', ], collectCoverageFrom: [ '**/*.ts', '!**/*.d.ts', '!**/node_modules/**', '!**/dist/**', - '!jest.config.ts', + '!tsup.config.ts', '!tests/**', ],🤖 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 `@packages/canvas/jest.config.cjs` around lines 5 - 18, Clean up the Jest configuration by removing the redundant `**/plugins/**/*.test.ts` and `**/setup/**/*.test.ts` entries from `testMatch`, and remove the ineffective `!jest.config.ts` exclusion from `collectCoverageFrom` because the config is CJS. Keep the broad `**/*.test.ts` matching and all applicable coverage exclusions unchanged.packages/canvas/endpoints/types.ts (1)
35-56: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUnresolved path placeholders reach the Canvas API. No layer enforces that a value exists for each
{placeholder}in an operation path: the generated input schema keepspathParamsfully optional, andresolvePathsubstitutes silently. A call such asgetSingleCourse({})therefore sends a request whose URL still contains the literal placeholder, and Canvas answers with a confusing error instead of a local validation failure.
packages/canvas/endpoints/types.ts#L35-L56: extract the placeholder names fromoperation.pathand require each one as a non-empty string in the generated input schema.packages/canvas/client.ts#L37-L47: replace all placeholders with a global regex and throw when a value is missing, so an unresolved placeholder cannot reach the request URL.🤖 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 `@packages/canvas/endpoints/types.ts` around lines 35 - 56, The generated schema in createRequestInputSchema must extract every placeholder from operation.path and require each corresponding pathParams value to be a non-empty string, while keeping unrelated parameters optional. In packages/canvas/endpoints/types.ts lines 35-56, add this per-operation validation. In packages/canvas/client.ts lines 37-47, update resolvePath to replace all occurrences using a global placeholder match and throw when any required value is missing, preventing unresolved placeholders from reaching the request URL.packages/canvas/tsconfig.json (1)
17-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExclude non-library files from the declaration project.
include: ["./**/*"]includesschema.test.tsandtsup.config.ts; exclude these files to prevent declaration output for them. Keepreferences: []; workspace package resolution handles thecorsairimports.🤖 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 `@packages/canvas/tsconfig.json` around lines 17 - 19, Update the declaration project configuration in packages/canvas/tsconfig.json to exclude schema.test.ts and tsup.config.ts alongside dist and node_modules, while preserving the existing include pattern and references: [] setting.packages/canvas/endpoints/index.ts (1)
4-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an exhaustiveness check for endpoint groups.
defineGrouppermits partial registration. Add a type-level or test-level assertion that everyCanvasOperationNameappears in exactly one endpoint group.🤖 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 `@packages/canvas/endpoints/index.ts` around lines 4 - 14, Add an exhaustiveness assertion alongside defineGroup and the endpoint-group declarations so every CanvasOperationName is registered exactly once: reject missing names and duplicate registrations at the type or test level. Keep defineGroup’s generated endpoint mapping behavior unchanged.
🤖 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 `@packages/canvas/client.ts`:
- Around line 12-22: Update normalizeCanvasBaseUrl to parse the trimmed URL with
new URL(), require the protocol to be exactly https:, and reject malformed or
non-HTTPS values before returning the normalized base URL. Preserve
trailing-slash removal and the existing required-value validation, using the
parsed URL result to ensure the base URL includes a valid host.
- Around line 65-86: Update the request preparation in the Canvas client around
the config and requestOptions objects to transform array-valued query parameters
into Canvas bracketed keys such as include[] before passing them to the shared
executor. Preserve the existing bearer-token configuration and ensure non-array
query parameters remain unchanged.
In `@packages/canvas/endpoints/factory.ts`:
- Around line 54-58: Update the completion logging around logEventFromContext in
the endpoint handler to avoid passing the full input object, which may contain
personal or access-token data. Construct and pass only non-sensitive metadata,
or redact input.body, pathParams, and query fields known to contain sensitive
values, while preserving the parsed response and completed-event behavior.
In `@packages/canvas/endpoints/types.ts`:
- Around line 65-75: Widen canvasResponseSchema in CanvasEndpointOutputSchemas
to accept successful responses with undefined, empty-string, and other string
bodies in addition to JSON objects and arrays. Preserve the existing object and
array validation so factory.ts parsing continues to validate structured
responses without turning 204 DELETE results or getRubricsUploadTemplate into
errors.
In `@packages/canvas/error-handlers.ts`:
- Around line 5-18: Restrict the fallback matching in RATE_LIMIT_ERROR.match so
“429” is recognized only when the error message clearly indicates rate limiting,
while preserving the existing ApiError status check and rate_limited matching.
Remove the broad substring behavior that treats unrelated identifiers containing
429 as retryable.
In `@packages/canvas/index.ts`:
- Around line 630-644: Update the OAuth configuration created by canvas() so
authUrl and tokenUrl use each tenant’s account base_url, matching
resolveCanvasBaseUrl in the endpoint factory, instead of resolving
options.baseUrl only once. If OAuth URLs cannot be resolved per tenant by the
framework, require options.baseUrl for oauth_2 and reject configuration without
it rather than falling back to the public Canvas host.
- Around line 581-601: Update CanvasOperation in operations.ts to support an
optional riskLevel, then modify buildEndpointMeta to prefer an operation’s
explicit riskLevel over HTTP-method inference. Mark deleteDiscussionEntry,
deleteDiscussionTopicGraphQl, deleteSubmissionDraft, and deleteOutcomeLinks as
destructive so PluginPermissionsConfig enforces the correct permission.
In `@packages/canvas/webhooks/types.ts`:
- Around line 91-99: Update verifyCanvasWebhookSignature to normalize
request.headers['x-canvas-signature'] by selecting the first element when it is
an array, while preserving the existing string and missing-header behavior. Use
the normalized string for subsequent signature verification instead of directly
casting the header value.
---
Nitpick comments:
In `@packages/canvas/endpoints/factory.ts`:
- Around line 12-28: Update resolveCanvasBaseUrl to use the generated
KeyBuilderContext type for ctx.keys, preserving the base_url getter contract
declared by canvasAuthConfig instead of casting to an ad hoc getter shape. Reuse
the existing Canvas typing from canvas/index.ts or export a focused typed helper
there, and keep the current option/account fallback behavior unchanged.
In `@packages/canvas/endpoints/index.ts`:
- Around line 4-14: Add an exhaustiveness assertion alongside defineGroup and
the endpoint-group declarations so every CanvasOperationName is registered
exactly once: reject missing names and duplicate registrations at the type or
test level. Keep defineGroup’s generated endpoint mapping behavior unchanged.
In `@packages/canvas/endpoints/operations.ts`:
- Around line 619-628: Remove the duplicate deleteAMessage operation from the
endpoint definitions, and remove its references from the Conversations group and
canvasEndpointsNested.conversations. Retain deleteConversationMessages as the
sole operation for this method and path.
- Around line 63-68: Update the descriptions for getAccountGraphQl,
getAssignment2, createAssignmentGraphQl, and getLegacyNode to state that callers
must supply the appropriate GraphQL query document and variables in input.body.
Preserve their existing method and path metadata; do not claim resource-specific
behavior is built in unless the factory also supplies the GraphQL document.
In `@packages/canvas/endpoints/types.ts`:
- Around line 35-56: The generated schema in createRequestInputSchema must
extract every placeholder from operation.path and require each corresponding
pathParams value to be a non-empty string, while keeping unrelated parameters
optional. In packages/canvas/endpoints/types.ts lines 35-56, add this
per-operation validation. In packages/canvas/client.ts lines 37-47, update
resolvePath to replace all occurrences using a global placeholder match and
throw when any required value is missing, preventing unresolved placeholders
from reaching the request URL.
In `@packages/canvas/index.ts`:
- Around line 578-579: Move the canvasOperations import from its current
position between declarations to the file’s top import section, alongside the
other module imports. Do not change how canvasOperations is used.
In `@packages/canvas/jest.config.cjs`:
- Around line 5-18: Clean up the Jest configuration by removing the redundant
`**/plugins/**/*.test.ts` and `**/setup/**/*.test.ts` entries from `testMatch`,
and remove the ineffective `!jest.config.ts` exclusion from
`collectCoverageFrom` because the config is CJS. Keep the broad `**/*.test.ts`
matching and all applicable coverage exclusions unchanged.
In `@packages/canvas/tsconfig.json`:
- Around line 17-19: Update the declaration project configuration in
packages/canvas/tsconfig.json to exclude schema.test.ts and tsup.config.ts
alongside dist and node_modules, while preserving the existing include pattern
and references: [] setting.
In `@packages/corsair/core/constants.ts`:
- Around line 220-316: Update the AllProviders type to include the literal
provider value 'canvas' alongside the existing provider names, preserving the
open (string & {}) fallback and current type behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6d72a4b-7ce9-4b84-ad90-8ffbe5279db5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
packages/canvas/client.tspackages/canvas/endpoints/factory.tspackages/canvas/endpoints/index.tspackages/canvas/endpoints/operations.tspackages/canvas/endpoints/types.tspackages/canvas/error-handlers.tspackages/canvas/index.tspackages/canvas/jest.config.cjspackages/canvas/package.jsonpackages/canvas/schema.test.tspackages/canvas/schema/index.tspackages/canvas/tsconfig.jsonpackages/canvas/tsup.config.tspackages/canvas/webhooks/index.tspackages/canvas/webhooks/oauth-tenant-link.tspackages/canvas/webhooks/tenant-matcher.tspackages/canvas/webhooks/triggers.tspackages/canvas/webhooks/types.tspackages/corsair/core/constants.ts
6 tasks
Collaborator
Author
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/canvas/endpoints/operations.ts (2)
207-210: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse
user_idin the last-attended route.Canvas defines this endpoint as
/api/v1/courses/{course_id}/users/{user_id}/last_attended, not an enrollment-scoped route.🤖 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 `@packages/canvas/endpoints/operations.ts` around lines 207 - 210, Update the addLastAttendedDate operation’s path to use the Canvas user-scoped route with the {user_id} placeholder instead of the enrollment-scoped {enrollment_id} route, while preserving the existing method and operation metadata.
492-495: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMark both no-body POST operations as
bodyless.
duplicateGroupDiscussionTopicneeds no body.assignUnassignedMembersToGroupCategoryonly has optionalsyncquery input. Withoutbodyless: true, valid path-only calls fail local validation.🤖 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 `@packages/canvas/endpoints/operations.ts` around lines 492 - 495, Mark both duplicateGroupDiscussionTopic and assignUnassignedMembersToGroupCategory operation definitions as bodyless: true, while preserving the existing method, paths, and optional sync query input.
🤖 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 `@packages/canvas/webhooks/oauth-tenant-link.ts`:
- Around line 59-64: Replace the accounts[0] fallback in the OAuth tenant-link
resolution with an explicit account identifier from the OAuth contract; when no
identifier exists or multiple accounts remain ambiguous, return null instead of
selecting an arbitrary account. Update the surrounding account-resolution flow
and add coverage for reordered multi-account responses and paginated results.
---
Outside diff comments:
In `@packages/canvas/endpoints/operations.ts`:
- Around line 207-210: Update the addLastAttendedDate operation’s path to use
the Canvas user-scoped route with the {user_id} placeholder instead of the
enrollment-scoped {enrollment_id} route, while preserving the existing method
and operation metadata.
- Around line 492-495: Mark both duplicateGroupDiscussionTopic and
assignUnassignedMembersToGroupCategory operation definitions as bodyless: true,
while preserving the existing method, paths, and optional sync query input.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5be6ea9b-b88e-4427-89c4-2c986f7af8a6
📒 Files selected for processing (6)
packages/canvas/api.test.tspackages/canvas/client.tspackages/canvas/endpoints/operations.tspackages/canvas/endpoints/types.tspackages/canvas/schema.test.tspackages/canvas/webhooks/oauth-tenant-link.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/canvas/client.ts
- packages/canvas/schema.test.ts
Collaborator
Author
|
@greptile review |
Collaborator
Author
|
@greptile review |
Collaborator
Author
|
@greptile review |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/canvas/endpoints/routes.ts (2)
53-59: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse a Map for route lookup.
canvasRoutesholds over 200 entries.getCanvasRouteperforms a linear scan on every call. Build aMap<CanvasOperationName, CanvasRoute>once at module load and read from it.♻️ Proposed refactor
+const canvasRouteByKey = new Map<CanvasOperationName, CanvasRoute>( + canvasRoutes.map((entry) => [entry.key, entry]), +); + export function getCanvasRoute(key: CanvasOperationName): CanvasRoute { - const route = canvasRoutes.find((entry) => entry.key === key); + const route = canvasRouteByKey.get(key); if (!route) { throw new Error(`[canvas] Unknown operation: ${key}`); } return route; }🤖 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 `@packages/canvas/endpoints/routes.ts` around lines 53 - 59, Replace the linear canvasRoutes.find lookup in getCanvasRoute with a module-level Map<CanvasOperationName, CanvasRoute> initialized once from canvasRoutes. Retrieve routes by key from the Map, preserve the existing unknown-operation error and return behavior, and avoid rebuilding the Map per call.
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pathParamsOfduplicatespathParamNamesinpackages/canvas/endpoints/types.ts.Both functions extract
{param}placeholders with the same regular expression. Export one helper and import it in the other module. This keeps the path-parameter contract in a single place.🤖 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 `@packages/canvas/endpoints/routes.ts` around lines 24 - 26, Remove the duplicate placeholder extraction from pathParamsOf and reuse the exported pathParamNames helper from the endpoints types module instead. Update the relevant import/export declarations so both call sites share the single implementation and preserve the existing readonly string-array behavior.packages/canvas/endpoints/response-schemas.ts (2)
449-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
expectsListResponsefrom the schema instead of repeating the branches.
expectsListResponseduplicates every branch ofcreateResponseSchema. If one function changes, the two can disagree, andpackages/canvas/api.test.tsbuilds mock responses fromexpectsListResponse. A test would then pass while the runtime schema rejects the real response.Compute the answer from the schema that
createResponseSchemareturns.♻️ Proposed refactor
export function expectsListResponse( name: CanvasOperationName, operation: CanvasOperation = canvasOperations[name], ): boolean { - if (operation.path === '/api/graphql') return false; - if (operation.method === 'DELETE') return false; - if (operation.path.includes('/upload')) return false; - const resource = resourceSchemaFor(name, operation); - if ( - resource === CanvasPermissionsSchema || - resource === CanvasUnreadCountSchema || - resource === CanvasQuotaSchema || - resource === CanvasSubmissionSummarySchema || - resource === CanvasJsonObjectSchema || - resource === CanvasJsonArraySchema - ) { - return resource === CanvasJsonArraySchema; - } - return isListOperation(name, operation); + return createResponseSchema(name, operation) instanceof z.ZodArray; }🤖 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 `@packages/canvas/endpoints/response-schemas.ts` around lines 449 - 468, Refactor expectsListResponse to derive its result from the response schema produced by createResponseSchema, rather than duplicating path, method, and resource-specific branches. Reuse the existing schema-generation symbols and determine whether the returned schema represents a list, keeping the boolean consistent with runtime validation.
13-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace
.passthrough()withz.looseObject()across this file. Zod 4 deprecates the method, andz.looseObject()preserves unknown keys.🤖 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 `@packages/canvas/endpoints/response-schemas.ts` around lines 13 - 17, Replace the deprecated .passthrough() usage on CanvasEntitySchema and any other schemas in this file with z.looseObject(), preserving each schema’s existing fields and unknown-key behavior.packages/canvas/endpoints/operations.ts (1)
68-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking read-only GraphQL operations as
read.
riskForinpackages/canvas/endpoints/routes.tsmaps any POST towrite. All GraphQL operations use POST, so query-only operations such asgetAccountGraphQl,getLegacyNode,getAuditLogs,getInternalSettings,getModuleItem, andgetAssignmentGroupare reported aswriterisk. The newriskLevelfield can correct this metadata for query-only GraphQL operations.Note that the GraphQL document is supplied by the caller in
input.body, so areadlabel is only accurate if callers are constrained to queries. Choose the label that matches the intended guarantee.🤖 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 `@packages/canvas/endpoints/operations.ts` around lines 68 - 73, Update the GraphQL endpoint metadata for getAccountGraphQl and the other query-only operations (getLegacyNode, getAuditLogs, getInternalSettings, getModuleItem, and getAssignmentGroup) to use the riskLevel field with a read value only if callers are constrained to query documents; otherwise retain write to reflect the caller-supplied GraphQL operation. Ensure each label matches the intended guarantee and riskFor consumes this metadata.
🤖 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 `@packages/canvas/endpoints/response-schemas.ts`:
- Around line 415-446: Update createResponseSchema to return operation-specific
wrapper schemas for getQuizStatistics, getOutcomeResults,
createBatchOverridesInACourse, and polling create operations returning polls,
poll_choices, poll_sessions, or poll_submissions. Define or reuse strict Zod
schemas matching each documented response shape, including the
assignment-overrides array, and select them by operation name before generic
resource/list handling; do not use a permissive fallback.
In `@packages/canvas/endpoints/types.ts`:
- Around line 62-67: Update CanvasEndpointInputs to derive body optionality from
each operation’s mutation and bodyless status, matching
createRequestInputSchema: require a non-empty body for mutations that are not
bodyless, while preserving optional body for bodyless or non-mutation
operations. Ensure the existing pathParams mapping remains unchanged.
In `@packages/canvas/webhooks/oauth-tenant-link.ts`:
- Around line 44-46: Update the singleton-row branch in the tenant-link
resolution function so it does not use the child row’s id as the tenant identity
when parent_account_id is set. Prefer a valid root_account_id from the row or
related account data, and return null when no valid root identity is available;
preserve the existing behavior for root accounts.
In `@packages/canvas/webhooks/tenant-matcher.ts`:
- Around line 5-8: The numericAccountId function in
packages/canvas/webhooks/tenant-matcher.ts:5-8 must iterate through all values
and return the first normalized string matching /^\d+$/, rather than validating
only firstString(values). In
packages/canvas/webhooks/oauth-tenant-link.ts:74-76, scan canvas_account_id,
account_id, and root_account_id for the first normalized numeric value before
checking UUID fields.
---
Nitpick comments:
In `@packages/canvas/endpoints/operations.ts`:
- Around line 68-73: Update the GraphQL endpoint metadata for getAccountGraphQl
and the other query-only operations (getLegacyNode, getAuditLogs,
getInternalSettings, getModuleItem, and getAssignmentGroup) to use the riskLevel
field with a read value only if callers are constrained to query documents;
otherwise retain write to reflect the caller-supplied GraphQL operation. Ensure
each label matches the intended guarantee and riskFor consumes this metadata.
In `@packages/canvas/endpoints/response-schemas.ts`:
- Around line 449-468: Refactor expectsListResponse to derive its result from
the response schema produced by createResponseSchema, rather than duplicating
path, method, and resource-specific branches. Reuse the existing
schema-generation symbols and determine whether the returned schema represents a
list, keeping the boolean consistent with runtime validation.
- Around line 13-17: Replace the deprecated .passthrough() usage on
CanvasEntitySchema and any other schemas in this file with z.looseObject(),
preserving each schema’s existing fields and unknown-key behavior.
In `@packages/canvas/endpoints/routes.ts`:
- Around line 53-59: Replace the linear canvasRoutes.find lookup in
getCanvasRoute with a module-level Map<CanvasOperationName, CanvasRoute>
initialized once from canvasRoutes. Retrieve routes by key from the Map,
preserve the existing unknown-operation error and return behavior, and avoid
rebuilding the Map per call.
- Around line 24-26: Remove the duplicate placeholder extraction from
pathParamsOf and reuse the exported pathParamNames helper from the endpoints
types module instead. Update the relevant import/export declarations so both
call sites share the single implementation and preserve the existing readonly
string-array behavior.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ef056dd4-115d-4e3a-adf9-d4531366220e
📒 Files selected for processing (17)
packages/canvas/api.test.tspackages/canvas/client.tspackages/canvas/endpoints/factory.tspackages/canvas/endpoints/index.tspackages/canvas/endpoints/operations.tspackages/canvas/endpoints/response-schemas.tspackages/canvas/endpoints/routes.tspackages/canvas/endpoints/types.tspackages/canvas/error-handlers.tspackages/canvas/index.tspackages/canvas/jest.config.cjspackages/canvas/schema.test.tspackages/canvas/tsconfig.jsonpackages/canvas/webhooks/oauth-tenant-link.tspackages/canvas/webhooks/tenant-matcher.tspackages/canvas/webhooks/types.tspackages/corsair/core/constants.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/canvas/tsconfig.json
- packages/canvas/endpoints/factory.ts
- packages/canvas/error-handlers.ts
- packages/canvas/webhooks/types.ts
- packages/canvas/client.ts
- packages/canvas/schema.test.ts
6 tasks
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
2 participants
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.
Description
This PR introduces a comprehensive Canvas LMS integration to Corsair.
Key Features:
canvasplugin with over 200+ REST and GraphQL endpoints.baseUrlresolution to accommodate both cloud and self-hosted Canvas instances.Closes #486
Checklist
Before submitting your PR, please verify the following:
pnpm lintand all checks pass (Note: passed for canvas package)pnpm typecheckand there are no TypeScript errorspnpm buildand all packages build successfullypnpm testand all tests pass (Canvas tests passing 5/5)Screenshots / Demos (if applicable)
Additional Notes
x-canvas-signature).Summary by CodeRabbit