CloudTrayMenu: Allow dev tokens #3459
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR refactors CloudTrayMenu token management by separating bag versus file token cleanup, preserves file-based dev tokens on validation failure, ensures legacy bag tokens are always cleared, pre-cleans the file token before updates, and adds proper awaiting for filebrowser HTTP operations. Sequence diagram for updated MajorTom token validation and cleanupsequenceDiagram
participant CloudTrayMenu
participant Bag
participant Filebrowser
CloudTrayMenu->>Bag: getData('major_tom')
CloudTrayMenu->>CloudTrayMenu: isMajorTomTokenValid(tokenToUse, true)
alt Token invalid
CloudTrayMenu->>CloudTrayMenu: Do not remove file token (preserve dev token)
CloudTrayMenu->>CloudTrayMenu: Set tokenToUse = undefined
else Token valid and differs from file token
CloudTrayMenu->>CloudTrayMenu: updateMajorTomToken(tokenToUse)
CloudTrayMenu->>Filebrowser: cleanMajorTomFileToken()
CloudTrayMenu->>Filebrowser: createFile(MAJOR_TOM_CLOUD_TOKEN_FILE.path, true)
CloudTrayMenu->>Filebrowser: writeToFile(MAJOR_TOM_CLOUD_TOKEN_FILE.path, token)
end
CloudTrayMenu->>Bag: cleanMajorTomBagToken() (always)
CloudTrayMenu->>Bag: setData('major_tom', { ...data })
CloudTrayMenu->>CloudTrayMenu: Set local_token
Class diagram for updated CloudTrayMenu token management methodsclassDiagram
class CloudTrayMenu {
+async cleanMajorTomBagToken(): Promise<void>
+async cleanMajorTomFileToken(): Promise<void>
+async fetchMajorTomData(): Promise<InstalledExtensionData>
+async updateMajorTomToken(token: string): Promise<void>
}
class Filebrowser {
+async createFile(folder_path: string, override: Boolean = false): Promise<void>
+async writeToFile(file: string, content: string): Promise<void>
+async deleteFile(file: FilebrowserFile): Promise<void>
}
CloudTrayMenu --> Filebrowser : uses
Flow diagram for MajorTom token update processflowchart TD
A[Start token update] --> B{Is token valid?}
B -- No --> C[Preserve file token]
C --> D[Remove bag token]
D --> E[Set local_token to empty]
B -- Yes --> F{Does token differ from file token?}
F -- Yes --> G[Clean file token]
G --> H[Create file token]
H --> I[Write new token]
I --> D
F -- No --> D
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
joaomariolago
changed the title
joaomariolago
force-pushed
the
allow-dev-tokens-cloud
branch
2 times, most recently
from
ff6ec0c to
49947eb
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider logging errors in the catch block around cleanMajorTomFileToken so you can track file delete failures instead of silently swallowing them.
- It would be safer to add explicit error handling or response status checks in your Filebrowser methods (createFile/writeToFile/deleteFile) to avoid unhandled axios failures.
- You could refactor the bag and file token cleanup into a shared utility to reduce duplication between cleanMajorTomBagToken and cleanMajorTomFileToken.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider logging errors in the catch block around cleanMajorTomFileToken so you can track file delete failures instead of silently swallowing them.
- It would be safer to add explicit error handling or response status checks in your Filebrowser methods (createFile/writeToFile/deleteFile) to avoid unhandled axios failures.
- You could refactor the bag and file token cleanup into a shared utility to reduce duplication between cleanMajorTomBagToken and cleanMajorTomFileToken.
## Individual Comments
### Comment 1
<location> `core/frontend/src/components/cloud/CloudTrayMenu.vue:289` </location>
<code_context>
this.operation_error_message = String(error)
},
- async cleanMajorTomToken(): Promise<void> {
+ async cleanMajorTomBagToken(): Promise<void> {
const data = await bag.getData('major_tom')
if (data && data.token) {
</code_context>
<issue_to_address>
Consider handling the case where bag.getData('major_tom') returns null or undefined.
Spreading a null value into setData may cause runtime errors; ensure data is always an object before spreading.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
const data = await bag.getData('major_tom')
if (data && data.token) {
delete data.token
}
await bag.setData('major_tom', { ...data })
=======
const data = await bag.getData('major_tom') || {}
if (data.token) {
delete data.token
}
await bag.setData('major_tom', { ...data })
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `core/frontend/src/components/cloud/CloudTrayMenu.vue:336` </location>
<code_context>
this.local_token = tokenToUse ?? ''
},
async updateMajorTomToken(token: string): Promise<void> {
+ try {
+ await this.cleanMajorTomFileToken()
+ } catch {
+ /** Do nothing */
+ }
</code_context>
<issue_to_address>
Catching all errors without logging may obscure underlying issues.
Consider logging the exception or adding a comment to clarify why silent failure is appropriate in this context.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
async updateMajorTomToken(token: string): Promise<void> {
+ try {
+ await this.cleanMajorTomFileToken()
+ } catch {
+ /** Do nothing */
+ }
=======
async updateMajorTomToken(token: string): Promise<void> {
try {
await this.cleanMajorTomFileToken()
} catch (err) {
// Log the error to help diagnose issues with token cleanup.
// Silent failure is appropriate here because token cleanup is non-critical.
console.error('Failed to clean Major Tom file token:', err)
}
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
joaomariolago
force-pushed
the
allow-dev-tokens-cloud
branch
from
9f3fc71 to
1b176ee
Compare
joaomariolago
requested review from
Williangalvani,
joaoantoniocardoso,
patrickelectric and
voorloopnul
|
I cant connect to cloud either on master or in this PR... are we not supporting outdated major tom versions? I'm on 2025-04-10 |
Good point, @joaomariolago maybe we should preserve the token in the bag and let majortom remove it when he finds a token in the filesystem. |
I think this was the original behavior of the PR, we decided on other discussions to remove the token from bag always, but if we think is better I can revert to keep in both bag and file system |
I think it's a valid behavior, older versions of major tom should still be able to use from the bag, and once majortom get updated it can remove the token from the bag and consume from the filesystem. |
* Do not remove a file token if it is invalid to allow other services that may use dev tokens to keep working * make sure token is always available at bag and file system so backend service can decide where to fetch and clean from bag if it supports new file system token approach
* Fix CRUD operations declared as asynced but not awaited correctly creating sync issues in other codes that uses these operations
joaomariolago
force-pushed
the
allow-dev-tokens-cloud
branch
from
1b176ee to
4b0d513
Compare
voorloopnul
approved these changes
Sep 10, 2025
|
@Williangalvani can you re-check this PR after last changes to check if it is working for you now |
patrickelectric
approved these changes
Sep 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Improve CloudTrayMenu token management by separating bag and file token cleanup, preserving developer tokens stored in files, always clearing legacy bag tokens, and ensuring filebrowser operations are properly awaited
Bug Fixes:
Enhancements: