feat: allow setting token for different repository #319
Conversation
WalkthroughThe recent updates focus on enhancing localization, repository-specific token management, and UI improvements. Key modifications include the introduction of parameterized localization for error messages, the ability to use distinct GitHub tokens for different repositories, and styling adjustments for better UI presentation. These changes aim to provide a more flexible and user-friendly experience, especially for users managing multiple repositories or requiring localization support. Changes
Related issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
src/i18n/locales/en.jsonis excluded by:!**/*.jsonsrc/i18n/locales/fr.jsonis excluded by:!**/*.json
Files selected for processing (9)
- src/GitHub/branch.ts (2 hunks)
- src/commands/callback.ts (3 hunks)
- src/commands/file_menu.ts (4 hunks)
- src/main.ts (3 hunks)
- src/settings/interface.ts (1 hunks)
- src/settings/migrate.ts (4 hunks)
- src/settings/modals/manage_repo.ts (6 hunks)
- src/styles.css (1 hunks)
- src/utils/data_validation_test.ts (1 hunks)
Additional comments: 16
src/styles.css (1)
- 68-76: The CSS changes for modal elements and setting item controls are appropriate and align with the PR objectives of enhancing the user interface. The use of advanced CSS selectors like
:has()is noted for its precision, though it's important to consider browser compatibility and maintainability in future developments.src/commands/callback.ts (3)
- 86-86: The modification to pass
repo?.smartKeyas an argument toreloadOctokitis correctly implemented, leveraging optional chaining to handle cases whererepomight be null. This change aligns with the PR objectives and follows best practices for asynchronous operations.- 109-109: The adjustment to
reloadOctokitfunction calls, passingrepo?.smartKey, is correctly applied here as well, ensuring that repository-specific configurations are supported. The use of optional chaining and proper handling of asynchronous operations is noted and appreciated.- 235-235: Again, the use of
repo?.smartKeyas an argument forreloadOctokitis correctly implemented, supporting the functionality for repository-specific tokens. The consistent application of optional chaining and asynchronous patterns across these changes is commendable.src/main.ts (1)
- 162-165: The
reloadOctokitmethod now accepts an optionalrepoparameter, allowing it to reload Octokit with a repository-specific token. This is a significant improvement for handling different repository configurations. However, consider the following:
- Verify that the
loadTokenmethod is robust and handles all edge cases, asreloadOctokitrelies on it for token retrieval.- Ensure that the
apiSettings.tiersForApiandapiSettings.hostnameare validated or sanitized to prevent potential security issues.- It might be beneficial to add more detailed comments or documentation about the conditions under which an enterprise GitHub instance is used, for clarity and maintainability.
src/GitHub/branch.ts (1)
- 285-293: The localization of error messages using
i18next.tin thecheckRepositorymethod is a positive change, enhancing the user experience for non-English speakers. However, consider the following:
- Ensure that all error message keys (
commands.checkValidity.inRepo.error404, etc.) are correctly defined in the localization files.- It's good practice to log the original error object along with the user-friendly error message for debugging purposes.
src/settings/migrate.ts (3)
- 144-180: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [132-177]
The
migrateTokenfunction has been updated to handle repository-specific tokens. This is an important update for supporting more granular token management. Consider the following:
- Ensure that the
repoparameter is sanitized before use, especially when constructing file paths or JSON keys, to prevent injection vulnerabilities.- The logic for handling JSON and non-JSON token storage formats is well-implemented. However, ensure that documentation or comments are added to clarify the expected format of the token storage file for maintainability.
- Consider adding error handling for file operations (
mkdir,read,write) to gracefully handle potential filesystem issues.
- 215-219: The addition of transformation settings for link handling in the
migrateOtherRepositoryfunction is a thoughtful enhancement, allowing for more customization in how links are managed. However, ensure that:
- The default values (
toUri: false,slugify: "disable", etc.) are clearly documented and communicated to users, as these settings significantly impact how links are processed.- Consider providing examples or use cases in the documentation to help users understand how to utilize these new settings effectively.
- 326-330: In the
migrateOldSettingsfunction, the transformation settings for link handling (toUri: true,slugify: "lower") are set with sensible defaults. This is a good practice for ensuring backward compatibility and ease of use for existing users. However, similar to the previous comment, ensure that:
- These settings are well-documented, and users are informed about the impact of these defaults on their link handling.
- Consider adding validation or sanitization for these settings to prevent potential misuse or configuration errors.
src/commands/file_menu.ts (4)
- 19-19: The change to pass
repo?.smartKeytoreloadOctokitinshareFolderRepoaligns with the PR objectives to support repository-specific tokens. This should ensure that the correct GitHub client instance is used based on the repository context. Ensure thatreloadOctokitis updated to handle this parameter appropriately.- 149-154: In
addMenuFile, passinggetSharedKey?.smartKeytoreloadOctokitis a crucial update for supporting repository-specific tokens. This change ensures that actions performed within this function context are executed with the correct repository credentials. It's important to verify that all downstream effects of this change are accounted for, especially in howshareOneNotehandles the repository-specific client instance.- 217-217: The update in
subMenuCommandsFileto passotherRepo?.smartKeytoreloadOctokitis consistent with the goal of enabling repository-specific tokens. This modification is essential for ensuring that the GitHub client instance used is appropriate for the given repository. Similar to other changes, confirm thatreloadOctokitand any functions called thereafter correctly handle this new parameter.- 255-255: The addition of passing
item.smartKeytoreloadOctokitin thesubMenuCommandsFilefunction's callback forChooseRepoToRunis another step towards supporting repository-specific tokens. This ensures that user selections in the UI correctly influence which repository's token is used for GitHub operations. As with other changes, thorough testing is recommended to ensure seamless integration.src/utils/data_validation_test.ts (1)
- 400-404: The addition of new transformation options (
toUri,slugify,applyRegex) in thedefaultRepofunction for copying links is a valuable enhancement. This change allows for more flexible and customizable link handling, which can improve user experience and integration with different platforms or requirements. Ensure that these new options are well-documented and that their usage is clear to users or developers who might need to configure or extend the plugin.src/settings/modals/manage_repo.ts (2)
- 7-7: The import statement for
migrateTokenfrom "../migrate" is correctly added to support the new functionality for managing repository-specific tokens. This aligns with the PR objectives.- 330-337: The handling of the GitHub token involves decrypting an existing token and potentially migrating it. It's crucial to ensure that the token is securely handled throughout this process to prevent any security vulnerabilities.
Ensure that the
loadTokenandmigrateTokenmethods implement adequate security measures for handling sensitive information like tokens, especially regarding encryption, storage, and transmission.
| const desc_ghToken = document.createDocumentFragment(); | ||
| desc_ghToken.createEl("span", undefined, (span) => { | ||
| span.innerText = i18next.t("settings.github.ghToken.desc"); | ||
| span.createEl("a", undefined, (link) => { | ||
| link.innerText = `${i18next.t("common.here")}.`; | ||
| link.href = | ||
| "https://github.com/settings/tokens/new?scopes=repo,workflow"; | ||
| }); | ||
| }); | ||
| new Setting(contentEl) | ||
| .setName(i18next.t("common.ghToken")) | ||
| .setDesc(desc_ghToken) | ||
| .addText(async (text) => { | ||
| const decryptedToken: string = await this.plugin.loadToken(this.repository.smartKey); | ||
| text | ||
| .setPlaceholder("ghp_1234567890") | ||
| .setValue(this.repository.token ?? decryptedToken) | ||
| .onChange(async (value) => { | ||
| await migrateToken(this.plugin, value.trim(), this.repository.smartKey); | ||
| await this.plugin.saveSettings(); | ||
| }); |
There was a problem hiding this comment.
The implementation of the new ghToken setting in ModalEditingRepository is comprehensive, covering the description, input handling, and integration with the migrateToken function. However, ensure that error handling is robust around the asynchronous operations, especially for migrateToken and this.plugin.saveSettings(), to gracefully handle any potential failures.
Consider adding try-catch blocks around asynchronous operations to improve error handling and user feedback in case of failures.
src/settings/interface.ts
Outdated
| @@ -29,6 +29,7 @@ export interface Repository { | |||
| branch: string; | |||
| automaticallyMergePR: boolean; | |||
| verifiedRepo?: boolean; | |||
| token?: string, | |||
There was a problem hiding this comment.
The addition of the token field to the Repository interface aligns with the PR objectives. However, consider removing the trailing comma after the token field for consistency with TypeScript best practices.
- token?: string,
+ token?: stringCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| token?: string, | |
| token?: string |
| const tokenFile = await this.app.vault.adapter.read(`${tokenPath}`); | ||
| if (tokenPath.endsWith(".json")) { | ||
| const tokenJSON = JSON.parse(tokenFile); | ||
| return tokenJSON.GITHUB_PUBLISHER_TOKEN; | ||
| const defaultToken = tokenJSON.GITHUB_PUBLISHER_TOKEN; | ||
| if (repo) | ||
| return tokenJSON.GITHUB_PUBLISHER_REPOS[repo] ?? defaultToken; | ||
| return defaultToken; | ||
| } | ||
| if (tokenFile) { | ||
| return tokenFile.split("=")[1]; | ||
| const defaultToken=tokenFile.split("\n").find((line) => line.startsWith("GITHUB_TOKEN"))?.split("=")[1] ?? ""; | ||
| if (repo) return tokenFile.split("\n").find((line) => line.startsWith(`${repo}_TOKEN`))?.split("=")[1] ?? defaultToken; | ||
| return defaultToken; | ||
| } | ||
| } catch (e) { | ||
| notif({ settings: this.settings, e: true }, e); |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [131-150]
The loadToken method has been updated to support loading tokens specific to a repository. This is a crucial enhancement for managing repository-specific configurations. However, there are a few points to consider:
- Ensure that the
repoparameter is always sanitized before use, especially when used as a key in JSON objects or file paths, to prevent injection vulnerabilities. - The method now handles both JSON and non-JSON token storage formats. Ensure that this dual support does not introduce complexity or confusion in token management practices.
- Consider adding more detailed error logging or handling specific error types to improve maintainability and debugging.
- notif({ settings: this.settings, e: true }, e);
+ notif({ settings: this.settings, error: true }, e); // Ensure the error flag is clearly named for readability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
src/i18n/locales/en.jsonis excluded by:!**/*.jsonsrc/i18n/locales/fr.jsonis excluded by:!**/*.json
Files selected for processing (2)
- src/GitHub/branch.ts (2 hunks)
- src/settings/interface.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/GitHub/branch.ts
- src/settings/interface.ts
Summary by CodeRabbit