Skip to content

docs(rest-api): fix App-group follow-ups from review (ENG-40977) - #592

Open
pranavkamble-cometchat wants to merge 2 commits into
mainfrom
docs/restapi-mgmt-api-ENG-40977
Open

pranavkamble-cometchat wants to merge 2 commits into
mainfrom
docs/restapi-mgmt-api-ENG-40977

Conversation

@pranavkamble-cometchat

@pranavkamble-cometchat pranavkamble-cometchat commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #573 (ENG-40589), addressing @ketanyekale's review:

  • DELETE /apps/{appId}: replace the empty-"" wrapper + wrong deleteSchema with a direct $ref to deleteAppSchema ({success, scheduledToDeleteAt}), matching the example; response description "List Apps" -> "Delete App".
  • /apps/{appId}/usage: response description "Updated App" -> "App Usage"; example message "...processed for ." -> "...processed for .".
  • "serverslocated" -> "servers located" typo.

mint validates; still 40 paths, no unrelated endpoints touched.

Description

Related Issue(s)

Type of Change

  • Documentation correction/update
  • New documentation
  • Improvement to existing documentation
  • Typo fix
  • Other (please specify)

Checklist

  • I have read the CONTRIBUTING document
  • My branch name follows the naming convention
  • My changes follow the documentation style guide
  • I have checked for spelling and grammar errors
  • All links in my changes are valid and working
  • My changes are accurately described in this pull request

Additional Information

Screenshots (if applicable)

Follow-up to #573 (ENG-40589), addressing @ketanyekale's review:

- DELETE /apps/{appId}: replace the empty-"" wrapper + wrong deleteSchema
  with a direct $ref to deleteAppSchema ({success, scheduledToDeleteAt}),
  matching the example; response description "List Apps" -> "Delete App".
- /apps/{appId}/usage: response description "Updated App" -> "App Usage";
  example message "...processed for ." -> "...processed for <App Name>.".
- "serverslocated" -> "servers located" typo.

mint validates; still 40 paths, no unrelated endpoints touched.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mintlify

mintlify Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
cometchat 🟢 Ready View Preview Nov 3, 2026, 1:53 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

@ketanyekale

Copy link
Copy Markdown
Member

Review — docs(rest-api) App-group follow-ups (ENG-40977)

Checked against the OAS-source PR, cometchat-team/mgmt-api#927. The two are in parity and this is correct.

Verified

  • DELETE /apps/{appId} — data now $refs deleteAppSchema ({success, scheduledToDeleteAt}), which matches the response example (scheduledToDeleteAt: 1631203178); description "List Apps" -> "Delete App". deleteAppSchema already existed in components.schemas in this file, so nothing dangles, and deleteSchema is still referenced by 4 other operations, so it correctly stays.
  • /apps/{appId}/usage — description "Updated App" -> "App Usage"; example placeholder ...processed for . -> ...processed for <App Name>.. The data schema was already inline {success, message} and matches the example.
  • Typo — fixed in both places (POST /apps createApp and GET /apps listApps examples). Zero serverslocated left. Worth noting the GET /apps instance was stale here relative to the generated mgmt-api spec, which already read correctly, so this also re-syncs that drift.
  • Parity — jq-normalised diff of the two changed subtrees against mgmt-api#927 is empty except for this file's "operationId": "delete-app".
  • No build or link risk — only management-apis.json changed; no paths, methods or operationIds added, removed or renamed, so the explicit nav refs in docs.json (rest-api/management-apis/app/delete, .../app/list, …) still resolve and no redirects are needed. Path count unchanged at 40. No MDX prose references scheduledToDeleteAt or the old delete-response message field, so there is no contradicting hand-written content.

Non-blocking

Carried over from the OAS repo: the empty-"" wrapper this PR removes for delete-app still affects 130 other response schemas in this file — list-collaborators, list-extensions, the webhook operations, and the meta object on list-apps — so those response shapes still render wrong in the same way. Tracking that as a separate sweep rather than widening this PR; same note left on mgmt-api#927.

Minor: the checklist still has "I have checked for spelling and grammar errors" and "All links in my changes are valid and working" unticked. Both hold as far as I can tell — just tick them.

@pranavkamble-cometchat

Copy link
Copy Markdown
Contributor Author

@ketanyekale thanks for the review — confirming the docs side.

All three fixes mirror mgmt-api cometchat-team/mgmt-api#927 and the /apps/{appId} DELETE + /apps/{appId}/usage subtrees are in parity (identical apart from the docs-only operationId, as you noted):

  • DELETE → data.$ref → deleteAppSchema (matches the example), description: "List Apps" → "Delete App"
  • usage → description: "Updated App" → "App Usage", message …processed for <App Name>.
  • serverslocated → servers located

mint validates the spec; still 40 paths, no docs.json nav or redirect impact, and deleteSchema stays in use by the other delete ops.

On the two pre-existing items from the mgmt review: PUT /apps/{appId} is absent here too (it's dropped at generation time on the mgmt side via SORT_ORDER), and the empty-"" wrapper is systemic in this copy as well (~130 wrappers). Both will be tracked as their own tickets, not folded into this PR.

@ketanyekale

Copy link
Copy Markdown
Member

Re-review (second pass) — same head, 44075458

Second pass on this one, checking the new schema against the actual API implementation rather than just against the example. Everything in my first pass holds. One thing worth a line of prose, and one adjacent issue in this repo.

1. deleteAppSchema describes only one of the two real response shapes

Traced the implementation in mgmt-api (app/Http/Controllers/AppController.php on dev, ~L1046-1107). DELETE /apps/{appId} has two success paths:

  • First call (app not yet scheduled) — sets scheduledToDeleteAt = now + 30 days, state inactive, returns {"success": true, "scheduledToDeleteAt": <ts>}
  • Second call (app already scheduled) — permanently deletes the app, returns {"success": true} with no scheduledToDeleteAt

So the schema and example here cover the first path only, and as written they imply scheduledToDeleteAt is always returned.

This is still clearly the right change — the old {success, message} shape matched neither path, and message is never returned by this endpoint at all. Just worth a sentence in rest-api/management-apis/app/delete.mdx, whose description is currently the single line "Schedule a specific app for deletion.": calling DELETE again on an already-scheduled app deletes it permanently and returns only success. That's useful, non-obvious behaviour for anyone integrating against it.

2. Adjacent: error-codes.mdx documents an error that can't be returned

rest-api/management-apis/error-codes.mdx:14 lists:

ERR_APP_ARCHIVED — The app has been archived and will be permanently deleted after 30 days.

On dev that code is assigned in exactly one place (AppController.php:1032), inside the inner else of the delete flow — nested in a branch where scheduledToDeleteAt is already known to be empty, so it cannot be reached. The adjacent comment ("Deletes apps even if scheduledToDeleteAt key is present") suggests the behaviour was intentionally changed to hard-delete on the second call, which left the error dead and this row stale. Separate ticket, not this PR.

Confirmed, in addition to the first pass

  • Page bindings are by method+path (openapi: delete /apps/{appId}, openapi: post /apps/{appId}/usage), not by operationId or slug — so there is definitively no nav, slug or redirect impact from this change.
  • Only one copy of this spec exists in the repo, and docs.json registers only management-apis.json for it, so there is no versioned duplicate left unpatched.
  • No update/PUT page exists under rest-api/management-apis/app/ — consistent with PUT /apps/{appId} being missing from the upstream generated spec (noted on mgmt-api#927).

No blockers here from my side.

@ketanyekale ketanyekale left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving — docs side

Re-checked at 44075458. Nothing outstanding from either of my passes; both items I raised on the second pass were explicitly non-blocking.

Verified independently this time

  • Parity with cometchat-team/mgmt-api#927 — normalised comparison of paths["/apps/{appId}"].delete and paths["/apps/{appId}/usage"] between this branch and the mgmt-api PR head: identical apart from this file's operationId: delete-app. /apps/{appId}/usage is identical outright.
  • deleteAppSchema resolves in this file's components.schemas as {success: boolean, scheduledToDeleteAt: integer}, matching the example. deleteSchema is still referenced by other delete operations, so it correctly stays.
  • Typo — 0 serverslocated remaining, 2 correct servers located.
  • Example placeholder — 0 occurrences of processed for . left.
  • No build or nav risk — still 40 paths, spec parses clean, only management-apis.json changed. delete.mdx binds as openapi: delete /apps/{appId}, i.e. by method+path, so no slug, nav or redirect impact. All Mintlify checks green (deployment, link-rot, vale-spellcheck) plus validate-branch-name.
  • The upstream fix reproduces from source — on the mgmt-api side I ran the real generator end-to-end (swagger-php 4.11.1 per composer.lock, --legacy, repo processor + bootstrap, then format_oas_output.php). The DELETE /apps/{appId} and usage subtrees regenerate exactly as committed there, which is what this file mirrors. So this copy matches generated output, not just a hand edit.

Non-blocking, carried forward

  • The empty-"" wrapper still affects 130 other response schemas in this file (list-collaborators, list-extensions, the webhook operations, list-apps' meta). Tracked as ENG-41012 — correct not to widen this PR.
  • The delete.mdx sentence about the two-call behaviour (second DELETE on an already-scheduled app hard-deletes and returns only success) is still worth adding; I confirmed it again against AppController.php L1037-1118. Your call, and fine as a follow-up.
  • ENG-41013 covers the stale ERR_APP_ARCHIVED row in error-codes.mdx:14. Re-confirmed unreachable in the controller.
  • One thing to keep in mind for the ENG-41012 sweep: mgmt-api-commit.sh copies Mgmt-REST-APIs.json verbatim over this file when MINTIFY_REPO_PATH is set, but this file is not a verbatim copy today — 40 paths here vs 47 upstream, plus docs-only operationIds on the App group. A naive sync would overwrite those. Nav binds by method+path so it would not break pages, but the sweep's regen should account for it.

Still worth ticking the two unticked checklist boxes; both hold as far as I can tell.

Approving.

siva-cometchat
siva-cometchat previously approved these changes Nov 2, 2026

@ketanyekale ketanyekale left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes — retracting my approval, one line is wrong

Apologies for the churn: I approved this earlier today, then re-checked the fetch-usage example against the mgmt-api implementation and found my approval was premature. Everything else I verified still stands; this one line should not ship. Same note on cometchat-team/mgmt-api#927, where the OAS source lives.

The new usage example documents the wrong value

POST /apps/{appId}/usage's 200 example now reads:

Your usage request has been accepted and is being processed for <App Name>. You will receive an email with the details at <emailID>@example.com

That field is not an app name. In mgmt-api the template is app/Lib/AppResponse/Success.php:197 — ...processed for {{billingCycle}}... — and billingCycle is filled at WrapperController.php:258 from $successMsg, which is assigned in exactly two places in that file plus its initialiser:

source value
WrapperController.php:230 (default) current billing cycle (2026-10-01 - 2026-10-31)
WrapperController.php:216 (usePreviousBillingCycle: true) previous billing cycle (2026-09-01 - 2026-09-30)
WrapperController.php:66, no app subscription ''

An app name never appears there. The dangling processed for . this PR set out to fix was actually a faithful rendering of that third case — it looked broken but was not wrong, whereas the replacement reads cleanly and misinforms. For a PR whose purpose is fixing misleading response docs, that is the worse failure mode.

The preview makes it self-contradictory on the page itself. On fetch-usage the <App Name> example sits directly below a request body whose only property is usePreviousBillingCycle, described as choosing which billing cycle to fetch. Anyone reading top to bottom hits the contradiction immediately.

Requested change

One line in management-apis.json, under paths["/apps/{appId}/usage"].post.responses.200.content["application/json"].example:

-                    "message": "Your usage request has been accepted and is being processed for <App Name>. You will receive an email with the details at <emailID>@example.com"
+                    "message": "Your usage request has been accepted and is being processed for current billing cycle (2026-10-01 - 2026-10-31). You will receive an email with the details at <emailID>@example.com"

Any phrasing that conveys a billing-cycle range rather than an app name is fine; the above just matches what the code emits. Keep it in step with the mgmt-api side so the two copies stay in parity.

Everything else: confirmed, with stronger evidence this round

Redocly lint, main vs this branch — this PR fixes exactly 3 problems and introduces 0:

no-invalid-media-type-examples  #/paths/~1apps~1{appId}/delete/responses/200/.../example/data/success
no-invalid-media-type-examples  #/paths/~1apps~1{appId}/delete/responses/200/.../example/data/scheduledToDeleteAt
no-unused-components            #/components/schemas/deleteAppSchema  ("never used")

The old {success, message} schema provably contradicted its own example, and deleteAppSchema is provably no longer orphaned.

The delete page renders correctly on the preview — delete shows "Delete App" as the 200 description and { "success": true, "scheduledToDeleteAt": 1631203178 } as the example, with data as a plain object. That is the customer-facing defect from the original review, fixed and verified in the rendered output rather than only in the spec.

Also still true: 40 paths, spec parses clean, pages bind by method+path so no nav/slug/redirect impact, all Mintlify checks green, and parity with mgmt-api#927 holds apart from the docs-only operationId.

Non-blocking, unchanged: the empty-"" wrapper still affects 130 other response schemas here (ENG-41012); the delete.mdx sentence on the two-call delete behaviour is still worth adding; ERR_APP_ARCHIVED in error-codes.mdx:14 is ENG-41013. And the two unticked checklist boxes.

Re-approving as soon as the example line is corrected.

…ement API

Twelve endpoints carried copy-paste response descriptions that disagreed
with the operation they document, so the rendered response panel
announced the wrong action. Each new value matches that operation's own
summary and description:

- GET    /apps/{appId}/extensions                   Enabled/Disabled Extension -> List Extensions
- PUT    .../widget/v2/settings                     Create Chat Widget -> Update Chat Widget
- GET    .../widget/v2/settings                     Create Chat Widget -> List Chat Widgets
- PUT    .../gifs-giphy/v1/settings                 Create Chat Widget -> Update Giphy Settings
- GET    .../xss-filter/v1/settings                 Update XSS filters Settings -> Get XSS filters Settings
- DELETE .../image-moderation/v1/settings           Get Image Moderation Settings -> Delete Image Moderation Settings
- GET    .../sentiment-analysis/v1/settings         Update Sentiment analysis Settings -> Get Sentiment analysis Settings
- PUT    .../virus-malware-scanner/v1/settings      Create Chat Widget -> Update Virus Malware Scanner Settings
- POST   .../push-notification/v1/upload-certificate-p12  Query Certificate -> Upload Certificate
- GET    /apps/{appId}/collaborators                Created App -> List Collaborators
- DELETE /apps/{appId}/webhooks/{webhookId}/triggers     Enabled Triggers -> Remove Triggers
- POST   /tenants                                   Map Settings -> Add Tenant

Response descriptions only. operationId, summary and tags are untouched,
so every derived page slug is unchanged and no navigation entry or
redirect is affected. Still 40 paths, no dangling $refs.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

This branch was successfully deployed

1 active deployment
staging — 540cba77 Deployed Nov 3, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants