-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat(widgets): fetch saved access token automatically #344
base: master
Are you sure you want to change the base?
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant LocalStorage
User->>App: Mount Component
App->>LocalStorage: Retrieve Access Token
LocalStorage-->>App: Return Token (if exists)
App->>User: Pre-fill Token Input (if exists)
User->>App: Update Token Input
App->>App: Enable/Disable Save Token Button
User->>App: Click Save Token
App->>LocalStorage: Save Token
User->>App: Click Clear Token
App->>LocalStorage: Remove Token
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 (
|
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: 1
🧹 Outside diff range and nitpick comments (2)
demo/App.jsx (2)
38-43
: LGTM! Consider adding error handling for localStorage access.The useEffect hook correctly retrieves the saved token on component mount. However, localStorage access can fail in certain scenarios (e.g., private browsing mode).
useEffect(() => { - const savedUserAccessToken = localStorage.getItem('webexWidgetsDemoAccessToken'); - if (savedUserAccessToken) { - setTokenInput(savedUserAccessToken); - } + try { + const savedUserAccessToken = localStorage.getItem('webexWidgetsDemoAccessToken'); + if (savedUserAccessToken) { + setTokenInput(savedUserAccessToken); + } + } catch (error) { + console.warn('Failed to retrieve saved access token:', error); + } }, []);
27-31
: Add error handling for localStorage operations.Token saving should handle potential localStorage failures gracefully.
const handleSaveToken = (event) => { event.preventDefault(); - setToken(tokenInput); - localStorage.setItem('webexWidgetsDemoAccessToken', tokenInput); + try { + localStorage.setItem('webexWidgetsDemoAccessToken', tokenInput); + setToken(tokenInput); + } catch (error) { + console.error('Failed to save access token:', error); + // Optionally show user feedback about the failure + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/demo.bundle.21d6dc76bb96ca9b5537.js.map
is excluded by!**/*.map
📒 Files selected for processing (2)
demo/App.jsx
(2 hunks)docs/demo.bundle.21d6dc76bb96ca9b5537.js.LICENSE.txt
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/demo.bundle.21d6dc76bb96ca9b5537.js.LICENSE.txt
[style] ~381-~381: ‘WITH REGARD TO’ might be wordy. Consider a shorter alternative.
Context: ...AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WAR...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[duplication] ~435-~435: Possible typo: you repeated a word
Context: ... this source tree. / /* * @license React * react.production.min.js * * Copyright (c) F...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~512-~512: Using many exclamation marks might seem excessive (in this case: 69 exclamation marks for a text that’s 11998 characters long)
Context: ...See LICENSE file. */ //! moment.js //! moment.js locale configuration
(EN_EXCESSIVE_EXCLAMATION)
🔇 Additional comments (2)
demo/App.jsx (1)
Line range hint 14-19
: LGTM! Token cleanup is properly implemented.
The function correctly clears both the state and localStorage.
docs/demo.bundle.21d6dc76bb96ca9b5537.js.LICENSE.txt (1)
1-512
: LGTM! Comprehensive license documentation.
The file properly documents all third-party licenses, ensuring legal compliance.
🧰 Tools
🪛 LanguageTool
[style] ~381-~381: ‘WITH REGARD TO’ might be wordy. Consider a shorter alternative.
Context: ...AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WAR...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
[duplication] ~435-~435: Possible typo: you repeated a word
Context: ... this source tree. / /* * @license React * react.production.min.js * * Copyright (c) F...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~512-~512: Using many exclamation marks might seem excessive (in this case: 69 exclamation marks for a text that’s 11998 characters long)
Context: ...See LICENSE file. */ //! moment.js //! moment.js locale configuration
(EN_EXCESSIVE_EXCLAMATION)
setTokenInput(event.target.value); | ||
const saveTokenButton = document.getElementById('saveTokenButtonId'); | ||
if (event.target.value.length === 0) saveTokenButton.disabled = true; | ||
else saveTokenButton.disabled = false; |
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.
Replace direct DOM manipulation with React state.
Direct DOM manipulation with getElementById
is an anti-pattern in React. Use state to manage the button's disabled state.
+const [isSaveButtonDisabled, setIsSaveButtonDisabled] = useState(true);
+
const handleUpdateInputToken = (event) => {
setTokenInput(event.target.value);
- const saveTokenButton = document.getElementById('saveTokenButtonId');
- if (event.target.value.length === 0) saveTokenButton.disabled = true;
- else saveTokenButton.disabled = false;
+ setIsSaveButtonDisabled(event.target.value.length === 0);
};
Then update the button props:
-<Button color="blue" type="submit" onClick={handleSaveToken} disabled={!!token} ariaLabel="Save Token">
+<Button color="blue" type="submit" onClick={handleSaveToken} disabled={!!token || isSaveButtonDisabled} ariaLabel="Save Token">
Committable suggestion skipped: line range outside the PR's diff.
Issue:
In every page refresh, developer has to provide access token for testing the demo app, which involves lot of context switching and time consuming.
Solution
** Provided a solution to fetch saved access token automatically from local storage**
Vidcast of testing:
Link:
Manual Tests Performed
Join Meeting
buttonSummary by CodeRabbit
New Features
Documentation