Skip to content

Conversation

@LironMShemen
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@asafkorem asafkorem left a comment

Choose a reason for hiding this comment

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

Work of art 💯

Comment on lines 68 to 70
logger
.labeled("CODE")
.info(`Running code:`, matchingEntry.value.code);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

also, we might want to extract the log to some function (const logCodeExec = (code: string) => { ...), since we're using this on line 117 as well. Also I think we should color the code in grey so it won't be very annoying on the logs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't this log enough? https://github.com/wix-incubator/pilot/blob/master/packages/core/src/common/CodeEvaluator.ts#L20-L24

It is needed in cases where the code fails to execute and there is no indication of what code was actually trying to run.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also, we might want to extract the log to some function (const logCodeExec = (code: string) => { ...), since we're using this on line 117 as well. Also I think we should color the code in grey so it won't be very annoying on the logs

image

],

logLevel: "info",
logLevel: "warn",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

info is for logging the view hierarchy. I don't think it's needed.

Comment on lines 83 to 84
const tempImagePath = path.join(__dirname, "temp_screenshot.png");
writeFileSync(tempImagePath, base64Image, { encoding: "base64" });
Copy link
Collaborator

Choose a reason for hiding this comment

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

it saves the temp file in the current directory, always under the same name.
it will be a problem when running in parallel, also it should be stored under the temp dir (os.tmpdir()). and use a unique name for each snapshot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

futhermore, it might be better to use the existing method for taking snapshot from the testing framework driver. WDYT?

Comment on lines +41 to +44
/**
* Removes near-duplicate coordinates based on a small epsilon threshold
*/
function deduplicateCoordinates(
Copy link
Collaborator

Choose a reason for hiding this comment

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

was it needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In case of 2 images returns the same location.

Copy link
Collaborator

@asafkorem asafkorem left a comment

Choose a reason for hiding this comment

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

GREAT GREAT JOB!
image

@asafkorem asafkorem merged commit 272f45f into master Jun 25, 2025
3 checks passed
@asafkorem asafkorem deleted the ocr-tool branch June 25, 2025 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants