Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use UID instead of handle to identify extensions in app-watcher #5150

Merged
merged 6 commits into from
Jan 13, 2025
Merged

Use UID instead of handle to identify extensions in app-watcher #5150

merged 6 commits into from
Jan 13, 2025

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Jan 3, 2025 •
edited
Loading

WHY are these changes introduced?

switch from using extension handles to UIDs as the primary identifier for extensions in the app watcher.

WHAT is this pull request doing?

  • Updates extension diffing to compare UIDs instead of handles
  • Modifies UUID generation to be deterministic based on the extension handle
  • Use UID in the whole app-watcher/dev-session flow.
  • Adds some code to detect folder movements.

How to test your changes?

  1. Create a new app with multiple extensions
  2. Start the dev server
  3. Try moving extension folders around (inside the extensions folder)
  4. Verify that extensions are properly tracked and rebuilt
  5. Test deleting extensions and confirm proper cleanup

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

@isaacroldan Graphite App
Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@isaacroldan isaacroldan marked this pull request as ready for review January 3, 2025 10:17
@isaacroldan isaacroldan requested a review from a team as a code owner January 3, 2025 10:17
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 3, 2025

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 3, 2025 •
edited
Loading

Coverage report

St.❔
 Category Percentage Covered / Total
🟡 Statements
75.15% (+0% 🔼)
 8872/11805
🟡 Branches
70.34% (+0.01% 🔼)
 4313/6132
🟡 Functions 75.08% 2320/3090
🟡 Lines
75.72% (+0.01% 🔼)
 8387/11076
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.83% (-1.23% 🔻)
86.49% (-2.7% 🔻)
 90.48% 98.61%

Test suite run success

2005 tests passing in 904 suites.

Report generated by 🧪jest coverage report action from e433cf6

@isaacroldan isaacroldan force-pushed the 01-03-use_uid_instead_of_handle_to_identify_extensions_in_app-watcher branch from 10ab3ae to 66b63f7 Compare January 7, 2025 15:44
gonzaloriestra
gonzaloriestra approved these changes Jan 10, 2025
View reviewed changes
packages/app/src/cli/models/extensions/extension-instance.ts
@@ -150,11 +150,12 @@
this.directory = options.directory
this.specification = options.specification
this.handle = this.buildHandle()
this.devUUID = `dev-${nonRandomUUID(this.handle)}`
const uuidFromHandle = nonRandomUUID(this.handle)
this.devUUID = `dev-${uuidFromHandle}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a valid UUID is not required here (because of the dev- prefix), why can't we just use the handle directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dev-console and the remote-ui tool expect a UUID in this format (dev-{UUID})

packages/app/src/cli/services/dev/app-events/file-watcher.ts Outdated
@@ -221,8 +221,11 @@ export class FileWatcher {
this.pushEvent({type: 'app_config_deleted', path, extensionPath, startTime})
} else if (isExtensionToml) {
// When a toml is deleted, we can consider every extension in that folder was deleted.
this.extensionPaths = this.extensionPaths.filter((extPath) => extPath !== extensionPath)
this.pushEvent({type: 'extension_folder_deleted', path: extensionPath, extensionPath, startTime})
// We need to wait in case this is actually just moving folders around, not deleting them.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if we don't wait? If it appears as removed and then as added, I don't think it's a big issue and also I guess it won't be very common. So maybe it's better to keep the code simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a request from the consistent-dev team, but it doesn't solve the issue completely, so I guess we can discard this change until I have a proper solution.

The goal is to detect folder movements and don't treat it as remove+add, because the deletion could be potentially dangerous if it reaches the server.

@isaacroldan isaacroldan force-pushed the 01-03-use_uid_instead_of_handle_to_identify_extensions_in_app-watcher branch from 66b63f7 to 8a2b013 Compare January 13, 2025 12:00
@isaacroldan isaacroldan force-pushed the 01-03-use_uid_instead_of_handle_to_identify_extensions_in_app-watcher branch from 8a2b013 to e433cf6 Compare January 13, 2025 12:03
@isaacroldan isaacroldan added this pull request to the merge queue Jan 13, 2025
Merged via the queue into main with commit e28e357 Jan 13, 2025
28 checks passed
@isaacroldan isaacroldan deleted the 01-03-use_uid_instead_of_handle_to_identify_extensions_in_app-watcher branch January 13, 2025 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gonzaloriestra gonzaloriestra gonzaloriestra approved these changes

Assignees
No one assigned
Labels
gsd:40977
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@isaacroldan @gonzaloriestra