fix: address code review issues from PR #9#18
Open
numbers-official wants to merge 3 commits intomainfrom
Open
Conversation
- Add AssetEditModal component to popup for adding metadata before upload - Skip auto-upload when capturing from popup to allow user to fill metadata first - Preserve existing metadata when updating headline/caption - Add skipAutoUpload flag to capture message payload - Add CSS styling for the edit modal
- Add delete button in AssetEditModal with confirmation dialog - Add handleDeleteAsset function to remove screenshots from IndexedDB - Add CSS styling for delete button
- native-host/proofsnap_host.py
- Replace silent auto pip-install with a clear error message and sys.exit(1)
- Remove Access-Control-Allow-Origin: * and do_OPTIONS handler; CORS is
unnecessary since curl/scripts don't enforce it and browser-based
access from arbitrary websites is undesirable
- Use asyncio.get_running_loop() instead of deprecated get_event_loop()
- native-host: remove com.numbersprotocol.proofsnap.json, install.sh,
install.ps1 — these register a Chrome Native Messaging host that the
extension never uses (extension connects via raw WebSocket, not
chrome.runtime.connectNative), so they are dead and misleading code
- src/popup/popup.tsx: fix indentation of edit-modal-error block to
align with sibling elements
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
2 participants
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.
Summary
Applies targeted fixes to the code introduced in #9, based on review feedback:
proofsnap_host.py— replaced with a clear error message andsys.exit(1)to avoid silently runningpip installat import timeAccess-Control-Allow-Origin: *— the HTTP server is only consumed by local scripts (curl/PowerShell), which don't enforce CORS; the wildcard header was allowing any website to trigger capturesasyncio.get_event_loop()deprecation — replaced withasyncio.get_running_loop()(deprecated since Python 3.10)com.numbersprotocol.proofsnap.json,install.sh,install.ps1registered a Chrome Native Messaging host that the extension never uses (it connects via raw WebSocket, notchrome.runtime.connectNative())AssetEditModalerror display blockTest plan
python proofsnap_host.pywithoutwebsocketsinstalled — should print error and exitpython proofsnap_host.pywithwebsocketsinstalled — should start normallycurl -X POST http://localhost:19999/capturestill worksGenerated by Sherry Chung with Omni