Fix auth refresh, nested folder lookup, and Mail.ReadWrite scope - #62
Fix auth refresh, nested folder lookup, and Mail.ReadWrite scope#62siriomaquea wants to merge 5 commits into
Conversation
Previously handleCheckAuthStatus used the sync loadTokenCache helper, which returns null when the access_token is past expires_at. This caused the tool to report 'Not authenticated' for any client gating on it 1h after auth, even with a valid refresh_token sitting in the same file. Use ensureAuthenticated() (same path the other tools take) so the refresh_token grant runs when needed. Lazy-require ./index to avoid a circular dependency with auth/index.js.
me/mailFolders only returns top-level folders, so moving emails to
common destinations like 'To Delete' (under Inbox) or 'Archive/2024'
failed with 'folder not found'.
After the top-level search misses, iterate over folders with
childFolderCount > 0 and look up children via
me/mailFolders/{id}/childFolders. Also $select id, displayName,
childFolderCount on the top-level fetch to keep the response small.
config.js already lists Mail.ReadWrite among the required scopes for mail-write operations (move, delete drafts, etc.), but the OAuth server was issuing tokens without it. Resulting tokens hit 403 on any write endpoint. Add Mail.ReadWrite to AUTH_CONFIG.scopes so users have to re-consent once and then write operations work as documented.
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
📝 WalkthroughWalkthroughAdds Mail.ReadWrite to Graph scopes; replaces auth token-cache inspection with a lazy call to ensureAuthenticated() in the auth status handler; extends folder lookup to support slash-separated paths, escapes single quotes in filters, and adds case-insensitive top-level and one-level child-folder search. ChangesMail Auth and Folder Resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the authentication status check by utilizing ensureAuthenticated to handle token refreshes and adds the Mail.ReadWrite scope to the configuration. It also enhances folder lookup by searching one level deep into subfolders. Feedback was provided to optimize the subfolder search by performing API calls in parallel using Promise.all and applying a $select clause to minimize data transfer.
| const foldersWithChildren = allFoldersResponse.value.filter(f => f.childFolderCount > 0); | ||
| for (const parentFolder of foldersWithChildren) { | ||
| try { | ||
| const childResponse = await callGraphAPI( | ||
| accessToken, | ||
| 'GET', | ||
| `me/mailFolders/${parentFolder.id}/childFolders`, | ||
| null, | ||
| { $top: 100 } | ||
| ); | ||
| if (childResponse.value) { | ||
| const childMatch = childResponse.value.find( | ||
| f => f.displayName.toLowerCase() === lowerFolderName | ||
| ); | ||
| if (childMatch) { | ||
| console.error(`Found child folder "${folderName}" under "${parentFolder.displayName}" with ID: ${childMatch.id}`); | ||
| return childMatch.id; | ||
| } | ||
| } | ||
| } catch (err) { | ||
| console.error(`Error searching child folders of "${parentFolder.displayName}": ${err.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation performs sequential API calls for each folder that has children, which can lead to significant performance degradation if a user has many top-level folders (up to 100 based on the $top parameter). Using Promise.all to fetch child folders in parallel is much more efficient. Additionally, adding a $select clause to the child folder query will reduce the payload size and improve performance, consistent with the optimization made at line 92. Note that this implementation only searches one level deep into subfolders; if deeper nesting support is required (e.g., Inbox/Sub1/Sub2), a recursive approach would be necessary.
const foldersWithChildren = allFoldersResponse.value.filter(f => f.childFolderCount > 0);
const childResults = await Promise.all(foldersWithChildren.map(async (parent) => {
try {
const res = await callGraphAPI(
accessToken,
'GET',
`me/mailFolders/${parent.id}/childFolders`,
null,
{ $top: 100, $select: 'id,displayName,childFolderCount' }
);
return { parent, children: res.value || [] };
} catch (err) {
console.error(`Error searching child folders of "${parent.displayName}": ${err.message}`);
return { parent, children: [] };
}
}));
for (const { parent, children } of childResults) {
const match = children.find(f => f.displayName.toLowerCase() === lowerFolderName);
if (match) {
console.error(`Found child folder "${folderName}" under "${parent.displayName}" with ID: ${match.id}`);
return match.id;
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@auth/tools.js`:
- Around line 64-80: The auth check in the try/catch around
ensureAuthenticated() (log tag '[CHECK-AUTH-STATUS]') returns a plain "Not
authenticated" payload on both the no-token and exception branches; change both
failure return paths to return the standard UNAUTHORIZED error response (i.e.,
replace the content payload with the repo's UNAUTHORIZED error message/object)
so callers can rely on the repo's auth error contract when ensureAuthenticated()
returns falsy or throws.
In `@email/folder-utils.js`:
- Around line 106-123: The current logic filters foldersWithChildren and
compares each childResponse.value displayName to lowerFolderName (derived from
folderName), which fails for path-style inputs like "Archive/2024"; instead
split folderName into path segments (e.g., parts = folderName.split('/') with
trimming), then for each parentFolder in foldersWithChildren start from the
appropriate root (e.g., match parentFolder.displayName to the first segment) and
iteratively call callGraphAPI('GET', `me/mailFolders/${currentId}/childFolders`,
...) to walk each segment, matching child.displayName (case-insensitive) to the
next segment until you either find the final folder id or stop; update the logic
around parentFolder, childResponse and childMatch to perform segment-by-segment
traversal rather than a single full-string comparison.
🪄 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
Run ID: b3b659f4-6c0c-47a6-805c-4fa209ad3d68
📒 Files selected for processing (3)
auth/tools.jsemail/folder-utils.jsoutlook-auth-server.js
Both the no-token and exception branches were emitting 'Not authenticated', which deviates from the repo's standard auth error contract (UNAUTHORIZED). Align both failure returns so callers can rely on the contract. Addresses CodeRabbit review feedback on PR ryaker#62.
Two improvements to getFolderIdByName based on review feedback: 1. Path-style input (e.g. 'Archive/2024', 'Inbox/To Delete') is now detected and walked segment-by-segment via a new helper, getFolderIdByPath. The previous implementation compared each child folder's displayName against the full input, so any value containing '/' never matched. 2. Child folder lookups now run in parallel via Promise.all and pass \$select=id,displayName,childFolderCount to keep payloads small. The previous sequential for-await loop became slow for users with many top-level folders. Also escape single quotes when building the displayName \$filter so folder names containing apostrophes don't break the query. Addresses CodeRabbit and Gemini review feedback on PR ryaker#62.
|
Thanks for the reviews! I've addressed the feedback in two follow-up commits:
Also escaped single quotes in the Tested locally against my mailbox — single names, nested paths, and the case-insensitive fallback all resolve correctly. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@email/folder-utils.js`:
- Around line 72-77: The split/trim logic creates a normalized segment but when
segments.length === 1 the code continues to use the original folderName (e.g.,
"Archive/") for matching; update the code paths that do exact/case-insensitive
comparisons (the logic around segments, getFolderIdByPath, and the comparisons
at the other occurrences) to use a normalizedName = segments[0] when
segments.length === 1 so all match checks (exact and case-insensitive) operate
on the trimmed single segment; apply the same normalization change to the other
comparison sites referenced around the same block (the checks near the other
occurrences) so single-segment inputs are consistently normalized before
matching.
🪄 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
Run ID: 538ef9bd-7010-46cc-b3cc-0cbf97fcb160
📒 Files selected for processing (2)
auth/tools.jsemail/folder-utils.js
| // Detect path-style input (e.g. "Archive/2024", "Inbox/To Delete") and | ||
| // walk it segment-by-segment instead of treating the whole string as one name. | ||
| const segments = folderName.split('/').map(s => s.trim()).filter(Boolean); | ||
| if (segments.length > 1) { | ||
| return await getFolderIdByPath(accessToken, segments); | ||
| } |
There was a problem hiding this comment.
Normalize single-segment path input before matching.
After splitting/trimming segments, single-segment values like "Archive/" still use raw folderName ("Archive/") for exact and case-insensitive comparison, which can miss valid folders.
Suggested fix
- const segments = folderName.split('/').map(s => s.trim()).filter(Boolean);
+ const segments = folderName.split('/').map(s => s.trim()).filter(Boolean);
+ if (segments.length === 0) {
+ return null;
+ }
+ const normalizedFolderName = segments[0];
if (segments.length > 1) {
return await getFolderIdByPath(accessToken, segments);
}
// Single-segment: exact match filter first
@@
- { $filter: `displayName eq '${folderName.replace(/'/g, "''")}'` }
+ { $filter: `displayName eq '${normalizedFolderName.replace(/'/g, "''")}'` }
@@
- const lowerFolderName = folderName.toLowerCase();
+ const lowerFolderName = normalizedFolderName.toLowerCase();Also applies to: 85-85, 104-104
🤖 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 `@email/folder-utils.js` around lines 72 - 77, The split/trim logic creates a
normalized segment but when segments.length === 1 the code continues to use the
original folderName (e.g., "Archive/") for matching; update the code paths that
do exact/case-insensitive comparisons (the logic around segments,
getFolderIdByPath, and the comparisons at the other occurrences) to use a
normalizedName = segments[0] when segments.length === 1 so all match checks
(exact and case-insensitive) operate on the trimmed single segment; apply the
same normalization change to the other comparison sites referenced around the
same block (the checks near the other occurrences) so single-segment inputs are
consistently normalized before matching.
1 participant
Summary
Three independent fixes found running this MCP in production:
loadTokenCachewhich returns null whenexpires_atis in the past, causing it to reportNot authenticatedwhile a validrefresh_tokenwas sitting in the same file. Switched toensureAuthenticated()(same code path the other tools already use).getFolderIdByNamewalks into subfolders.me/mailFoldersonly returns top-level folders, so moving to common destinations likeTo Delete(under Inbox) orArchive/2024failed withfolder not found. After the top-level miss, iterate folders withchildFolderCount > 0and look up children.Mail.ReadWrite.config.jsalready lists it butoutlook-auth-server.jsdidn't, so issued tokens couldn't move/delete drafts. Add the scope so users re-consent once and writes work.Each fix is a separate commit for easier review.
Test plan
check-auth-statusafter token expiry → returnsAuthenticated and ready(refresh fires).move-emailstoTo Delete(subfolder of Inbox) → resolves correctly.move-emailsactually moves (no 403).npm testsuite continues to pass.Notes
Users will need to re-authenticate once after upgrading so the new
Mail.ReadWritescope is granted. Worth calling out in the release notes.Summary by CodeRabbit
Bug Fixes
New Features