feat: restore unmodified files from cache during cleanup#1818
Closed
Nithishvb wants to merge 2 commits intoonlook-dev:mainfrom
Closed
feat: restore unmodified files from cache during cleanup#1818Nithishvb wants to merge 2 commits intoonlook-dev:mainfrom
Nithishvb wants to merge 2 commits intoonlook-dev:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to e07a04f in 3 minutes and 14 seconds. Click for details.
- Reviewed
282lines of code in7files - Skipped
0files when reviewing. - Skipped posting
9draft 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.
Contributor
|
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). |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
This PR addresses the issue of large, noisy commits caused by unnecessary transformations of all
.jsx/.tsxfiles. 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/.tsxfiles from cache during cleanup by comparing file hashes, reducing unnecessary transformations..jsx/.tsxfiles are restored from cache instead of being transformed again.checkIfFileChanged()incleanup.tscompares file hashes to determine if a file has changed.cacheFile()andgenerateAndStoreHash()insetup.tsstore original file content and hashes in.onlook/cache.removeCacheDirectory()infiles.tsdeletes the cache directory during cleanup.HashesJsontype incache/index.tsto store file hashes and cache paths.IGNORED_DIRECTORIESinhelpers.tsto include.onlook.This description was created by
for e07a04f. You can customize this summary. It will automatically update as commits are pushed.