-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: restore unmodified files from cache during cleanup #1818
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
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.
Caution
Changes requested ❌
Reviewed everything up to e07a04f in 3 minutes and 14 seconds. Click for details.
- Reviewed
282
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
9
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/studio/electron/main/code/files.ts:87
- Draft comment:
Good use of crypto to generate hash. Consider adding a comment explaining the purpose of createHash for clarity. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/studio/electron/main/run/cleanup.ts:76
- Draft comment:
checkIfFileChanged properly reads and compares hashes; ensure filePath keys in JSON are consistent (absolute/relative) to avoid mismatches. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
3. apps/studio/electron/main/run/index.ts:208
- Draft comment:
Calling removeCacheDirectory during cleanProjectDir is clear. Ensure that cleanup order matches intended flow. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. apps/studio/electron/main/run/setup.ts:190
- Draft comment:
Using path.basename(filePath) for the cache file name may lead to collisions if different files share the same name. Consider using a hash of the full path or a directory structure. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code already handles uniqueness through the hashes.json mapping where full filepaths are used as keys. Even if two files have the same basename, their content and location are tracked separately in the JSON. The cache files are just temporary storage, and the mapping ensures correct retrieval. The suggestion would actually make the cache files harder to debug without adding real value. The comment raises a valid concern about filename collisions in general. Maybe there are edge cases where the JSON mapping isn't sufficient? The hashes.json provides a robust mapping system using full filepaths as keys, making the suggested change unnecessary complexity. The current system is both functional and more maintainable. The comment should be removed as the existing system already handles file uniqueness appropriately through the hashes.json mapping, and the suggested change would add unnecessary complexity.
5. apps/studio/electron/main/run/setup.ts:223
- Draft comment:
Similarly, in generateAndStoreHash, using path.basename(filePath) for cacheFilePath risks collisions; consider a safer naming scheme. - Reason this comment was not posted:
Marked as duplicate.
6. packages/models/src/cache/index.ts:1
- Draft comment:
The cache model is clear; ensure that keys used are unique to prevent cache collisions as noted in related functions. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
7. apps/studio/electron/main/run/setup.ts:223
- Draft comment:
The same basename-based approach is used in generateAndStoreHash to set the cache file path. This may lead to conflicts if different files share the same name. Consider revising this to produce unique cache file names. - Reason this comment was not posted:
Marked as duplicate.
8. apps/studio/electron/main/run/cleanup.ts:87
- Draft comment:
In checkIfFileChanged, if reading hashes.json fails, the function logs an error and returns false, which skips cleanup. Confirm that this fallback behavior is intentional, as it may leave transformed files unchanged if the cache cannot be read. - Reason this comment was not posted:
Comment looked like it was already resolved.
9. apps/studio/electron/main/run/setup.ts:69
- Draft comment:
Typographical error: the methods 't.jSXAttribute' and 't.jSXIdentifier' should be renamed to 't.jsxAttribute' and 't.jsxIdentifier' to match the standard naming convention in Babel types. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_e7Rd26OZei45qMMH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Hello, I am migrating the desktop app to a new repository. This repository will now focus on the web version. This PR has been migrated to the new repository: onlook-dev#8. Sorry for the inconvenience, rest assure your work here will also be adapted for the web version (if it hasn't already). |
Description
This PR addresses the issue of large, noisy commits caused by unnecessary transformations of all
.jsx/.tsx
files. Now, during the cleanup process, any file that remains unchanged since the initial transform will be restored from its cached original.Related Issues
Closes #1738
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Restores unmodified
.jsx/.tsx
files from cache during cleanup by comparing file hashes, reducing unnecessary transformations..jsx/.tsx
files are restored from cache instead of being transformed again.checkIfFileChanged()
incleanup.ts
compares file hashes to determine if a file has changed.cacheFile()
andgenerateAndStoreHash()
insetup.ts
store original file content and hashes in.onlook/cache
.removeCacheDirectory()
infiles.ts
deletes the cache directory during cleanup.HashesJson
type incache/index.ts
to store file hashes and cache paths.IGNORED_DIRECTORIES
inhelpers.ts
to include.onlook
.This description was created by
for e07a04f. You can customize this summary. It will automatically update as commits are pushed.