-
Notifications
You must be signed in to change notification settings - Fork 579
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
Open
Nithishvb
wants to merge
2
commits into
onlook-dev:main
Choose a base branch
from
Nithishvb:feat/restore-unchanged
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
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.
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/.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.