-
Notifications
You must be signed in to change notification settings - Fork 386
fix(encryption): update API doc links for plugin-encryption #4238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis change introduces the new Possibly related PRs
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
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/@webex/plugin-encryption/README.md (3)
1-5: Avoid duplicate top-level headings
The README begins with two H1 headings (# @webex/plugin-encryptionand then# Encryption plugin for the Cisco Webex JS SDK). This can confuse parsers and readers. Consider demoting the second heading to H2 or removing it to comply with the standard-readme spec.
22-30: Enhance CDN examples with SRI
To follow best practices and improve security, addintegrityandcrossoriginattributes to your<script>tags for both unpkg and jsDelivr.
57-64: Optional: Simplify promise handling
Instead of.then(async () => { … }), you could make the enclosing callbackasyncand use:await webex.cypher.register(); try { … } catch (error) { … }for clearer flow and error handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/index.html(1 hunks)packages/@webex/plugin-encryption/README.md(4 hunks)packages/@webex/plugin-encryption/package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Initialize Project
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (4)
packages/@webex/plugin-encryption/package.json (1)
17-17:✅ Verification successful
Confirm corrected docs output path
Thebuild:docsscript now writes to../../../docs/plugin-encryption, matching the renamed directory. Please verify there are no remaining references to the oldplugin-encrptionpath in CI/CD workflows or other scripts.Run this to catch any typos:
🏁 Script executed:
#!/bin/bash # Ensure no residual typo references to the old docs path rg -l "plugin-encrption"Length of output: 24
Docs output path confirmed
Ranrg -l plugin-encrptionwith no matches; there are no residual references to the old typo.docs/index.html (1)
100-100: Approve: Added Encryption API Reference link
The new<a href="./plugin-encryption">Encryption API Reference</a>aligns with the corrected docs directory and completes the API Reference section.packages/@webex/plugin-encryption/README.md (2)
17-19: Verify installation instructions
The Install section now directs users tonpm install --save webexinstead of the plugin package. Confirm that the encryption plugin ships with the corewebexbundle to prevent user confusion.
38-38: Approve: Updated usage links
The links to the developer portal and the specific Encryption API Reference (https://webex.github.io/webex-js-sdk/plugin-encryption/) are accurate and reflect the new docs structure.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
sreenara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused with the sample code snippet.
import Webex from 'webex/plugin-encryption';
Ideally, we should have 2 ways of a developer consuming this package.
import Webex from '@webex/plugin-encryption';
This would allow a developer to limit their package consumption to one package.
If a developer is already using webex or doesn't mind the entire package, then
import Webex from 'webex/encryption';
The statement in the README doesn't match the current folder structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/@webex/plugin-encryption/developer-quickstart.md (2)
87-87: Fix code block language tag
Remove the space beforetypescriptso syntax highlighting works correctly.
98-99: Improve example error handling and async flow
Relying on.then(async ...)without a.catchmay hide errors. Consider an immediately invoked async function with try/catch for clarity and proper error logging:(async () => { try { const webex = await initializeWebex(accessToken); await decryptFile(webex, attachmentURL, options, decryptedFileName, mimeType); } catch (error) { console.error('Error initializing or decrypting file:', error); } })();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
packages/@webex/plugin-encryption/README.md(5 hunks)packages/@webex/plugin-encryption/babel.config.js(1 hunks)packages/@webex/plugin-encryption/developer-quickstart.md(1 hunks)packages/@webex/plugin-encryption/jest.config.js(1 hunks)packages/@webex/plugin-encryption/package.json(2 hunks)packages/@webex/plugin-encryption/src/webex-config.ts(1 hunks)packages/@webex/plugin-encryption/src/webex.js(1 hunks)packages/webex-node/package.json(1 hunks)packages/webex-node/src/webex-node.js(1 hunks)packages/webex/package.json(0 hunks)webpack.config.js(1 hunks)
💤 Files with no reviewable changes (1)
- packages/webex/package.json
✅ Files skipped from review due to trivial changes (6)
- packages/webex-node/package.json
- packages/webex-node/src/webex-node.js
- webpack.config.js
- packages/@webex/plugin-encryption/src/webex-config.ts
- packages/@webex/plugin-encryption/package.json
- packages/@webex/plugin-encryption/src/webex.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-encryption/README.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Initialize Project
🔇 Additional comments (4)
packages/@webex/plugin-encryption/babel.config.js (3)
2-2: Good addition of package version injectionAdding the
@webex/babel-config-legacy/inject-package-versionplugin is a good practice that ensures the package version is automatically embedded in the built files. This is important for tracking and debugging purposes.
3-13: Improved preset configuration formattingThe reorganized preset configuration maintains the same functionality while improving readability with consistent formatting. The explicit node target specification in the
@babel/preset-envconfiguration is a good practice.
14-14: Good addition of source mapsEnabling source maps is beneficial for debugging transformed code, making it easier to trace issues back to the original source. This will improve the developer experience when working with this package.
packages/@webex/plugin-encryption/jest.config.js (1)
5-7: Correctly handling the uuid package transformationThe addition of this
transformIgnorePatternsconfiguration ensures that theuuidpackage is processed by Babel during testing, while other node modules are still ignored. This is important because:
- It allows the test environment to properly process ES modules from the uuid package
- It maintains test performance by still ignoring other node_modules
This change aligns with modern Jest testing practices for handling ES module dependencies.
sreenara
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicks in the comments. But everything else looks OK.
|
|
||
| webex.once('ready', () => { | ||
| webex.cypher.register().then(() => { | ||
| webex.cypher.register().then( async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| webex.cypher.register().then( async () => { | |
| webex.cypher.register().then(async () => { |
| ### Example Usage | ||
|
|
||
| ```typescript | ||
| ``` typescript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ``` typescript | |
| ```typescript |
COMPLETES: ADHOC
This pull request addresses
by making the following changes
Change Type
The following scenarios were tested
Tested locally and confirmed that the docs are generated
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.