feat(file): Add permission checks for directory scanning #165
Conversation
|
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a permission check mechanism in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FileSearch
participant PermissionCheck
User->>FileSearch: searchFiles(rootDir)
FileSearch->>PermissionCheck: checkDirectoryPermissions(rootDir)
alt Permission granted
PermissionCheck-->>FileSearch: PermissionCheckResult
FileSearch->>FileSearch: Perform file search
else Permission denied
PermissionCheck-->>FileSearch: PermissionError
FileSearch->>User: Throw PermissionError
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![standard-input[bot]](https://avatars.githubusercontent.com/in/1011034?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/core/file/fileSearch.ts (1)
39-47: LGTM: Good error handling for globbyThe error handling for permission-related errors is well-implemented. Consider adding debug logging for better troubleshooting.
Add debug logging before throwing the error:
if (error.code === 'EPERM' || error.code === 'EACCES') { + logger.debug(`Permission error encountered while scanning ${rootDir}: ${error.code}`); throw new PermissionError( 'Permission denied while scanning directory. Please check folder access permissions for your terminal app.', rootDir, ); }src/core/file/permissionCheck.ts (2)
42-56: Refactor permission checks to reduce code duplicationThe repeated try-catch blocks for checking read, write, and execute permissions can be consolidated into a loop. This enhances maintainability by reducing code duplication.
Apply this diff to refactor the permission checks:
- try { - await fsPromises.access(dirPath, constants.R_OK); - details.read = true; - } catch {} - - try { - await fsPromises.access(dirPath, constants.W_OK); - details.write = true; - } catch {} - - try { - await fsPromises.access(dirPath, constants.X_OK); - details.execute = true; - } catch {} + const permissionTypes = [ + { key: 'read', flag: constants.R_OK }, + { key: 'write', flag: constants.W_OK }, + { key: 'execute', flag: constants.X_OK }, + ]; + + for (const { key, flag } of permissionTypes) { + try { + await fsPromises.access(dirPath, flag); + details[key] = true; + } catch {} + }
30-93: Add unit tests forcheckDirectoryPermissionsfunctionIncluding unit tests for the
checkDirectoryPermissionsfunction will help ensure its correct behavior across different scenarios and prevent future regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/core/file/fileSearch.ts(2 hunks)src/core/file/permissionCheck.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Test (macos-latest, 16.x)
src/core/file/fileSearch.ts
[failure] 16-16: tests/core/file/fileSearch.test.ts > fileSearch > filterFiles > should call globby with correct parameters
Error: Cannot access directory /mock/root: undefined
❯ Module.searchFiles src/core/file/fileSearch.ts:16:11
❯ tests/core/file/fileSearch.test.ts:140:7
[failure] 16-16: tests/core/file/fileSearch.test.ts > fileSearch > filterFiles > Honor .gitignore files in subdirectories
Error: Cannot access directory /mock/root: undefined
❯ Module.searchFiles src/core/file/fileSearch.ts:16:11
❯ tests/core/file/fileSearch.test.ts:196:22
[failure] 16-16: tests/core/file/fileSearch.test.ts > fileSearch > filterFiles > should not apply .gitignore when useGitignore is false
Error: Cannot access directory /mock/root: undefined
❯ Module.searchFiles src/core/file/fileSearch.ts:16:11
❯ tests/core/file/fileSearch.test.ts:220:22
🪛 GitHub Check: Test (ubuntu-latest, 16.x)
src/core/file/fileSearch.ts
[failure] 16-16: tests/core/file/fileSearch.test.ts > fileSearch > filterFiles > should call globby with correct parameters
Error: Cannot access directory /mock/root: undefined
❯ Module.searchFiles src/core/file/fileSearch.ts:16:11
❯ tests/core/file/fileSearch.test.ts:140:7
[failure] 16-16: tests/core/file/fileSearch.test.ts > fileSearch > filterFiles > Honor .gitignore files in subdirectories
Error: Cannot access directory /mock/root: undefined
❯ Module.searchFiles src/core/file/fileSearch.ts:16:11
❯ tests/core/file/fileSearch.test.ts:196:22
[failure] 16-16: tests/core/file/fileSearch.test.ts > fileSearch > filterFiles > should not apply .gitignore when useGitignore is false
Error: Cannot access directory /mock/root: undefined
❯ Module.searchFiles src/core/file/fileSearch.ts:16:11
❯ tests/core/file/fileSearch.test.ts:220:22
🔇 Additional comments (2)
src/core/file/fileSearch.ts (2)
6-6: LGTM: Clean import addition
The new import statement correctly brings in the required permission-checking functionality.
55-64: LGTM: Well-structured error handling
The catch block properly differentiates between PermissionError and other errors, maintaining the error context and providing clear error messages.
yamadashy
force-pushed
the
feature/dir-permission
branch
from
0d1c212 to
5d9f7e7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #165 +/- ##
==========================================
- Coverage 77.50% 75.63% -1.87%
==========================================
Files 36 37 +1
Lines 1769 1859 +90
Branches 329 336 +7
==========================================
+ Hits 1371 1406 +35
- Misses 398 453 +55 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/core/file/fileSearch.ts (2)
39-47: Enhance error handling and add test coverage.The permission error handling for globby is good, but could be improved:
- Add logging before throwing the error to help with debugging
- Consider handling other permission-related error codes (e.g., ENOENT)
Consider applying this enhancement:
}).catch((error) => { // Handle EPERM errors specifically if (error.code === 'EPERM' || error.code === 'EACCES') { + logger.error(`Permission error scanning directory ${rootDir}:`, error.message); throw new PermissionError( 'Permission denied while scanning directory. Please check folder access permissions for your terminal app.', rootDir, ); } + // Handle non-existent directory + if (error.code === 'ENOENT') { + logger.error(`Directory not found ${rootDir}:`, error.message); + throw new Error(`Directory not found: ${rootDir}`); + } throw error; });🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 41-47: src/core/file/fileSearch.ts#L41-L47
Added lines #L41 - L47 were not covered by tests
55-64: Improve error type handling and logging.The error handling is good but could be enhanced with better TypeScript practices and logging:
Consider applying these improvements:
// Re-throw PermissionError as is if (error instanceof PermissionError) { + logger.debug('Re-throwing permission error:', error.message); throw error; } - if (error instanceof Error) { + const err = error as Error; + logger.error('Error filtering files:', err.message); + throw new Error(`Failed to filter files in directory ${rootDir}. Reason: ${err.message}`); - } - - logger.error('An unexpected error occurred:', error); - throw new Error('An unexpected error occurred while filtering files.');This change:
- Adds debug logging for permission errors
- Uses type assertion for better error handling
- Simplifies the error handling flow
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 56-58: src/core/file/fileSearch.ts#L56-L58
Added lines #L56 - L58 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/core/file/fileSearch.ts(2 hunks)src/core/file/permissionCheck.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/core/file/fileSearch.ts
[warning] 13-17: src/core/file/fileSearch.ts#L13-L17
Added lines #L13 - L17 were not covered by tests
[warning] 41-47: src/core/file/fileSearch.ts#L41-L47
Added lines #L41 - L47 were not covered by tests
[warning] 56-58: src/core/file/fileSearch.ts#L56-L58
Added lines #L56 - L58 were not covered by tests
src/core/file/permissionCheck.ts
[warning] 18-24: src/core/file/permissionCheck.ts#L18-L24
Added lines #L18 - L24 were not covered by tests
[warning] 57-61: src/core/file/permissionCheck.ts#L57-L61
Added lines #L57 - L61 were not covered by tests
[warning] 68-89: src/core/file/permissionCheck.ts#L68-L89
Added lines #L68 - L89 were not covered by tests
[warning] 92-94: src/core/file/permissionCheck.ts#L92-L94
Added lines #L92 - L94 were not covered by tests
[warning] 109-109: src/core/file/permissionCheck.ts#L109
Added line #L109 was not covered by tests
[warning] 111-112: src/core/file/permissionCheck.ts#L111-L112
Added lines #L111 - L112 were not covered by tests
🔇 Additional comments (5)
src/core/file/permissionCheck.ts (5)
1-4: Simplify imports using fs.promises directly
The imports can be simplified by using fs.promises directly instead of separate imports.
As suggested in the previous review, apply this change:
- import { constants } from 'node:fs';
- import * as fs from 'node:fs/promises';
+ import { constants, promises as fsPromises } from 'node:fs';6-14: LGTM! Well-structured interface definition
The PermissionCheckResult interface is well-designed with clear optional properties and nested types.
16-25: Add tests and consider type safety for error codes
The PermissionError class implementation looks good, but:
- Test coverage is missing for this class
- Error codes could be typed for better type safety
Consider adding type safety for error codes:
+ type PermissionErrorCode = 'EPERM' | 'EACCES' | 'EISDIR';
export class PermissionError extends Error {
constructor(
message: string,
public readonly path: string,
- public readonly code?: string,
+ public readonly code?: PermissionErrorCode,
) {Would you like me to help generate unit tests for this class? Here's what we should test:
- Constructor properly sets message, path, and code
- Error name is correctly set to 'PermissionError'
- Inheritance from Error class works as expected
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 18-24: src/core/file/permissionCheck.ts#L18-L24
Added lines #L18 - L24 were not covered by tests
92-112: Consider i18n support and add tests
The function provides excellent user guidance, but consider these improvements:
- Missing test coverage for both macOS and non-macOS paths
- Messages could be externalized for future i18n support
- The error message contains hard-coded UI paths that might change with macOS updates
Consider externalizing messages:
const PERMISSION_MESSAGES = {
darwin: {
header: 'Permission denied: Cannot access \'{path}\', error code: {code}.',
instructions: `
1. Open System Settings
2. Navigate to Privacy & Security > Files and Folders
...`
},
default: 'Permission denied: Cannot access \'{path}\''
};Let's check for similar message patterns:
#!/bin/bash
# Look for other platform-specific messages
rg "platform\(\) === '(\w+)'" -t tsWould you like me to help:
- Generate unit tests for both macOS and non-macOS scenarios?
- Set up a basic i18n structure for message externalization?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 92-94: src/core/file/permissionCheck.ts#L92-L94
Added lines #L92 - L94 were not covered by tests
[warning] 109-109: src/core/file/permissionCheck.ts#L109
Added line #L109 was not covered by tests
[warning] 111-112: src/core/file/permissionCheck.ts#L111-L112
Added lines #L111 - L112 were not covered by tests
27-90: 🛠️ Refactor suggestion
Refactor for better maintainability and add comprehensive tests
While the implementation is functionally correct, there are several improvements that could be made:
- The function is quite complex with multiple responsibilities
- Test coverage is missing for critical paths
- Error handling could be more modular
Consider breaking down the function into smaller, focused functions:
async function checkBasicAccess(dirPath: string): Promise<void> {
await fsPromises.readdir(dirPath);
}
async function checkSpecificPermissions(dirPath: string): Promise<PermissionDetails> {
const details = { read: false, write: false, execute: false };
// ... permission checking logic ...
return details;
}
async function handlePermissionError(error: unknown, dirPath: string): Promise<PermissionCheckResult> {
// ... error handling logic ...
}Let's verify the error handling paths:
Would you like me to help generate comprehensive tests covering:
- Successful permission checks
- Various permission combinations
- Different error scenarios (EPERM, EACCES, EISDIR)
- Platform-specific behavior
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 57-61: src/core/file/permissionCheck.ts#L57-L61
Added lines #L57 - L61 were not covered by tests
[warning] 68-89: src/core/file/permissionCheck.ts#L68-L89
Added lines #L68 - L89 were not covered by tests
This PR adds permission checking before scanning directories in the searchFiles function.
related: #162