Conversation
caugner
left a comment
caugner
left a comment
There was a problem hiding this comment.
Initial pass with mostly nits. I'll approve after another review round.
There was a problem hiding this comment.
[nit] Do these generated files need to be checked in?
There was a problem hiding this comment.
Not checking them in means adding python as a runtime dependency, and I'd like to avoid that
There was a problem hiding this comment.
Are you saying npm run glean:generate requires Python? I don't recall this from Yari.
There was a problem hiding this comment.
Yes, it uses https://github.com/mozilla/glean_parser
| if (res.status === 404) { | ||
| throw new NonSentryError( | ||
| `Error: We couldn't find "${key}" in the Browser Compatibility Data.`, | ||
| "404", |
There was a problem hiding this comment.
[nit] Should this error be more specific?
| "404", | |
| "compat_404", |
There was a problem hiding this comment.
No need: this string becomes silent_error.reason and the tool name is automatically added to silent_error.tool
| if (!res.ok) { | ||
| if (res.status === 404) { | ||
| throw new NonSentryError(`Error: We couldn't find ${path}`); | ||
| throw new NonSentryError(`Error: We couldn't find ${path}`, "404"); |
There was a problem hiding this comment.
dito
| throw new NonSentryError(`Error: We couldn't find ${path}`, "404"); | |
| throw new NonSentryError(`Error: We couldn't find ${path}`, "doc_404"); |
| if (!res.ok) { | ||
| throw new Error( | ||
| `${res.status}: ${res.statusText} for "${query}", perhaps try again.`, | ||
| ); |
There was a problem hiding this comment.
[nit] Should this be recorded as search_${res.status}?
There was a problem hiding this comment.
This string ends up displayed to the user, so no; and we don't submit it to glean, it's captured by Sentry (because our Search API really ought not to be erroring out!)
| const searchResponse = await res.json(); | ||
|
|
||
| /** @type {(SearchDocument | Doc)[]} */ | ||
| const docs = await Promise.all( | ||
| searchResponse.documents.map(async (searchDoc) => { | ||
| const { mdn_url } = searchDoc; | ||
| const metadataUrl = new URL( | ||
| mdn_url + "/metadata.json", | ||
| "https://developer.mozilla.org", | ||
| ); | ||
| try { | ||
| const metadataRes = await fetch(metadataUrl); | ||
| if (!metadataRes.ok) { | ||
| return searchDoc; | ||
| } | ||
| /** @type {Doc} */ | ||
| const doc = await metadataRes.json(); | ||
| return doc; | ||
| } catch { | ||
| return searchDoc; | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
[nit] Would an unexpectedly invalid search response crash the MCP server? If so, should this be caught, and recorded as search_invalid_response or similar?
There was a problem hiding this comment.
It shouldn't crash the server, just error out on that request, and Sentry would alert us
| "test": "node --test --experimental-test-coverage --test-coverage-lines 100 --test-coverage-exclude 'test/**/*' 'test/**/*.test.js'", | ||
| "test:watch": "node --test --watch 'test/**/*.test.js'" | ||
| "test:watch": "node --test --watch 'test/**/*.test.js'", | ||
| "glean:generate": "glean translate glean/metrics.yml -f javascript -o glean/generated/ && glean translate glean/metrics.yml -f typescript -o glean/generated/ && sed -i 's/ DO NOT COMMIT\\.//g' glean/generated/*" |
There was a problem hiding this comment.
[nit] If not checked in, this could run via postinstall.
|
|
||
| As this MCP is experimental it may be withdrawn at any time, in particular, the URL will change before it becomes production ready. | ||
|
|
||
| To opt out of first-party analytics, send the `X-Moz-1st-Party-Data-Opt-Out: 1` header along with requests to the MCP. |
There was a problem hiding this comment.
[nit] Maybe add a quick explanation of what the analytics is used for?
| @@ -10,6 +10,18 @@ | |||
| "moduleResolution": "nodenext", | |||
| // fix for badly-formed types in dependencies: | |||
| "skipLibCheck": true, | |||
There was a problem hiding this comment.
[nit] I guess this cannot be set to false due to some upstream issues?
There was a problem hiding this comment.
Yeah, on my todo list to attempt to turn it off and fix any errors
| "moduleResolution": "nodenext", | ||
| // fix for badly-formed types in dependencies: | ||
| "skipLibCheck": true, | ||
| // fix badly defined imports: |
There was a problem hiding this comment.
[nit] Can this be fixed upstream, and if so, is there a bug to link here?
There was a problem hiding this comment.
Probably, but we have to be on an old release of glean for it to support server-side pings, so I'm not sure it's worth it (there'd need to be a backport).
3 participants
Fixes #98
Blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=2020347 (now approved)