Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
capJavert
left a comment
capJavert
left a comment
There was a problem hiding this comment.
Reviewed alongside daily-api#4572 and daily#2182.
- Merge order holds. The consent page lists only scopes it has a description for. When nothing is unticked it posts no
scope, so the server grants everything the client asked for, hidden ones included.reading_historytherefore has to be known here before the API advertises it. The API PR should wait for this to be deployed, not only merged. - Follow-up (non-blocking): the page could fail closed, so the next scope doesn't need the same merge order. For example, always post
scopewith only the scopes it showed and the user kept (grantedScopes.filter((scope) => scopeDescriptions[scope])), or disable Allow when the request has a scope the page can't describe. - CI:
test_webappfails on__tests__/BookmarksPage.tsx, which this PR doesn't touch. The same test fails onmainat d5e6af0.
Reviewed by AI.
| 'offline_access', | ||
| 'read', | ||
| 'write', | ||
| 'reading_history', |
There was a problem hiding this comment.
Blocking (one-line fix): the new scope is missing from scopeLabels in packages/webapp/components/settings/OAuthApps.tsx, which only labels read and write. The Connected apps list builds each app's scope summary from that map and drops any scope without a label, so an app granted reading history still shows only "Read, Write". Users review and revoke access on that page, so it should show this scope. Please add a label in this PR, e.g. reading_history: 'Reading history'.
Reviewed by AI.
There was a problem hiding this comment.
Fixed in 9292fb9: scopeLabels now has reading_history: 'Reading history', so Connected apps shows it.
| import { noindexSeoProps } from '../../next-seo'; | ||
|
|
||
| const OPTIONAL_SCOPES = ['write', 'offline_access']; | ||
| const OPTIONAL_SCOPES = ['write', 'reading_history', 'offline_access']; |
There was a problem hiding this comment.
Question: reading_history starts ticked, like every optional scope, because declinedScopes starts empty. With daily-api#4572, the MCP challenge and scopes_supported include reading_history, so every new MCP or OAuth connection will ask for it. Most users will grant it just by leaving the box ticked. The read description deliberately lists the personal data it covers and leaves reading history out, so this is the most sensitive optional scope. Is opt-out intended? If it should be opt-in, a small change here is to start with it declined (useState<string[]>(['reading_history'])).
Reviewed by AI.
There was a problem hiding this comment.
Made it opt-in in 9292fb9: declinedScopes now starts as ['reading_history'], so the box starts unticked. write and offline_access still start ticked.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Review round 1 addressed in 9292fb9 (scope label, |
Adds the new optional
reading_historyOAuth scope to the consent page (description + optional checkbox, ticked by default likewrite) and to the scope list on settings/api.Merge before the daily-api PR. The consent page hides scopes it doesn't know but still grants them, so if daily-api shipped first users would grant
reading_historywithout seeing it.🤖 Generated with Claude Code
Preview domain
https://public-api-reading-history.preview.app.daily.dev