Add headline & caption to ProofSnap extension#9
Add headline & caption to ProofSnap extension#9Steffdarz wants to merge 2 commits intonumbersprotocol:mainfrom
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
There was a problem hiding this comment.
Pull request overview
Adds an “edit details” flow to the ProofSnap extension so users can enter headline/caption and keep screenshots as drafts before upload, and introduces a localhost HTTP/WebSocket trigger server concept for external capture automation.
Changes:
- Adds an
AssetEditModaland editing workflow in the popup to save headline/caption, upload, and delete drafts. - Updates background capture flow to skip auto-upload/notification messaging when capture is initiated from the popup (
fromPopup). - Adds a
native-host/folder with a Python HTTP+WebSocket trigger server plus install manifests/scripts.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/index.ts | Documents fromPopup flag behavior for capture messages. |
| src/popup/popup.tsx | Replaces metadata modal with an edit modal; adds edit/delete/save-draft flows and clickable thumbnails. |
| src/popup/popup.css | Adds styling for the new edit modal UI. |
| src/background/service-worker.ts | Skips auto-upload when initiated from popup; adds always-on WebSocket client for localhost triggers. |
| native-host/proofsnap_host.py | Implements localhost HTTP endpoints that forward capture commands to the extension over WebSocket. |
| native-host/install.sh | Installs a “native messaging host” manifest for Linux/macOS. |
| native-host/install.ps1 | Installs a “native messaging host” manifest for Windows. |
| native-host/com.numbersprotocol.proofsnap.json | Provides a placeholder native messaging manifest JSON. |
| native-host/README.md | Documents the trigger server usage and endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Merge new metadata with existing | ||
| await indexedDBService.updateAsset(assetId, { | ||
| metadata: { | ||
| ...currentAsset.metadata, | ||
| headline: headline || undefined, | ||
| caption: caption || undefined, | ||
| }, |
There was a problem hiding this comment.
handleUpdateAssetMetadata stores the raw headline/caption values without trimming. This can result in whitespace-only strings being persisted and later uploaded (UploadService appends these fields when truthy). Trim inputs before saving, and consider omitting the keys (or deleting them) when the trimmed value is empty rather than storing undefined into IndexedDB.
| // Merge new metadata with existing | |
| await indexedDBService.updateAsset(assetId, { | |
| metadata: { | |
| ...currentAsset.metadata, | |
| headline: headline || undefined, | |
| caption: caption || undefined, | |
| }, | |
| const trimmedHeadline = headline?.trim(); | |
| const trimmedCaption = caption?.trim(); | |
| const newMetadata = { ...currentAsset.metadata }; | |
| if (trimmedHeadline) { | |
| newMetadata.headline = trimmedHeadline; | |
| } else { | |
| delete (newMetadata as any).headline; | |
| } | |
| if (trimmedCaption) { | |
| newMetadata.caption = trimmedCaption; | |
| } else { | |
| delete (newMetadata as any).caption; | |
| } | |
| // Merge new metadata with existing | |
| await indexedDBService.updateAsset(assetId, { | |
| metadata: newMetadata, |
| - HTTP server only accepts connections from `localhost` (127.0.0.1) | ||
| - WebSocket is also localhost-only | ||
| - No external network access |
There was a problem hiding this comment.
The README's security section implies localhost-only access is sufficient, but with the current implementation any website can still trigger POST http://127.0.0.1:19999/capture from the user's browser (and WebSocket messages are unauthenticated as well). Update the documentation to reflect the need for an auth token/handshake, or change the implementation so that localhost-only is actually a meaningful protection (e.g., require a secret and/or disable CORS).
| - HTTP server only accepts connections from `localhost` (127.0.0.1) | |
| - WebSocket is also localhost-only | |
| - No external network access | |
| - HTTP server only accepts connections from `localhost` (127.0.0.1), so it is not reachable from other machines on your network or the Internet. | |
| - WebSocket is also bound to localhost. | |
| - **Important:** There is no built-in authentication or CSRF protection. Any website you visit in the same browser can attempt to send requests to `http://127.0.0.1:19999` (for example, using a form POST), and WebSocket messages are not authenticated. | |
| - Treat this server as a local automation tool that you only run in trusted environments. If you need stronger guarantees, wrap or extend it to require an auth token/handshake (e.g., a secret header or token in the request body) and/or add additional access controls. |
| // Keepalive ping to prevent connection timeout | ||
| setInterval(() => { | ||
| if (triggerSocket && triggerSocket.readyState === WebSocket.OPEN) { | ||
| triggerSocket.send(JSON.stringify({ type: 'ping' })); | ||
| } | ||
| }, 30000); |
There was a problem hiding this comment.
Using a persistent setInterval ping loop in an MV3 service worker can keep waking the worker (or create constant reconnection churn when it gets suspended), increasing CPU/battery usage and making the trigger connection unreliable. Consider only starting the WebSocket/ping when the trigger feature is enabled, and use a backoff strategy + cleanup (clear interval / close socket) when the worker is shutting down.
| <div className="asset-thumbnail" onClick={handleThumbnailClick}> | ||
| <img src={asset.uri} alt="Screenshot" /> |
There was a problem hiding this comment.
.asset-thumbnail is now clickable via onClick, but it's still a plain div without keyboard support. This makes the edit action inaccessible to keyboard/screen-reader users. Consider using a button element, or add role="button", tabIndex={0}, and an onKeyDown handler (Enter/Space) plus appropriate focus styles.
| def _send_json_response(self, status_code, data): | ||
| """Send JSON response""" | ||
| self.send_response(status_code) | ||
| self.send_header('Content-Type', 'application/json') | ||
| self.send_header('Access-Control-Allow-Origin', '*') | ||
| self.end_headers() | ||
| self.wfile.write(json.dumps(data).encode('utf-8')) | ||
|
|
||
| def do_OPTIONS(self): | ||
| """Handle CORS preflight""" | ||
| self.send_response(200) | ||
| self.send_header('Access-Control-Allow-Origin', '*') | ||
| self.send_header('Access-Control-Allow-Methods', 'GET, POST, OPTIONS') | ||
| self.send_header('Access-Control-Allow-Headers', 'Content-Type') | ||
| self.end_headers() |
There was a problem hiding this comment.
The HTTP API enables Access-Control-Allow-Origin: *, which allows any webpage the user visits to make fetch() requests to http://127.0.0.1:19999/* and trigger captures (the request still originates from localhost). This is a serious security/privacy issue. Remove the permissive CORS headers and add an explicit authentication mechanism (e.g., shared secret token required in a header/body) before accepting capture requests.
| # Create the manifest file | ||
| MANIFEST_FILE="$MANIFEST_DIR/$HOST_NAME.json" | ||
| cat > "$MANIFEST_FILE" << EOF | ||
| { | ||
| "name": "$HOST_NAME", | ||
| "description": "ProofSnap Native Messaging Host - HTTP trigger for screenshot capture", | ||
| "path": "$HOST_SCRIPT", | ||
| "type": "stdio", | ||
| "allowed_origins": [ | ||
| "chrome-extension://$EXTENSION_ID/" | ||
| ] | ||
| } | ||
| EOF | ||
|
|
||
| echo "✓ Created manifest: $MANIFEST_FILE" | ||
|
|
||
| # Also install for Edge if directory exists or can be created | ||
| if [[ -d "$(dirname "$MANIFEST_DIR_EDGE")" ]] || [[ "$OSTYPE" == "darwin"* ]]; then | ||
| mkdir -p "$MANIFEST_DIR_EDGE" | ||
| cp "$MANIFEST_FILE" "$MANIFEST_DIR_EDGE/" | ||
| echo "✓ Also registered with Edge" | ||
| fi | ||
|
|
||
| echo "" | ||
| echo "Installation complete!" | ||
| echo "" | ||
| echo "Next steps:" | ||
| echo "1. Reload the ProofSnap extension in Chrome" | ||
| echo "2. Test with: curl -X POST http://localhost:19999/capture" | ||
| echo "" | ||
| echo "The HTTP server will start automatically when the extension connects." |
There was a problem hiding this comment.
The installer writes a Native Messaging host manifest with "type": "stdio", but proofsnap_host.py doesn't implement the native messaging stdio protocol and the extension codebase doesn't appear to call chrome.runtime.connectNative(). As-is, this installation flow is misleading and likely non-functional. Either remove the native-messaging manifest/install scripts, or implement proper native messaging integration in the extension and adapt the host to communicate over stdin/stdout.
| # Create the manifest file | |
| MANIFEST_FILE="$MANIFEST_DIR/$HOST_NAME.json" | |
| cat > "$MANIFEST_FILE" << EOF | |
| { | |
| "name": "$HOST_NAME", | |
| "description": "ProofSnap Native Messaging Host - HTTP trigger for screenshot capture", | |
| "path": "$HOST_SCRIPT", | |
| "type": "stdio", | |
| "allowed_origins": [ | |
| "chrome-extension://$EXTENSION_ID/" | |
| ] | |
| } | |
| EOF | |
| echo "✓ Created manifest: $MANIFEST_FILE" | |
| # Also install for Edge if directory exists or can be created | |
| if [[ -d "$(dirname "$MANIFEST_DIR_EDGE")" ]] || [[ "$OSTYPE" == "darwin"* ]]; then | |
| mkdir -p "$MANIFEST_DIR_EDGE" | |
| cp "$MANIFEST_FILE" "$MANIFEST_DIR_EDGE/" | |
| echo "✓ Also registered with Edge" | |
| fi | |
| echo "" | |
| echo "Installation complete!" | |
| echo "" | |
| echo "Next steps:" | |
| echo "1. Reload the ProofSnap extension in Chrome" | |
| echo "2. Test with: curl -X POST http://localhost:19999/capture" | |
| echo "" | |
| echo "The HTTP server will start automatically when the extension connects." | |
| # NOTE: | |
| # This project currently does not implement the Chrome Native Messaging stdio | |
| # protocol in proofsnap_host.py, nor does the extension call | |
| # chrome.runtime.connectNative(). Installing a Native Messaging host manifest | |
| # would therefore be misleading and non-functional. | |
| # | |
| # To avoid registering a broken Native Messaging host, this installer no longer | |
| # creates the manifest or registers it with Chrome/Edge. | |
| echo "⚠ Native Messaging host installation has been disabled." | |
| echo "" | |
| echo "Reason:" | |
| echo " - The ProofSnap host script does not implement the Native Messaging stdio protocol." | |
| echo " - The browser extension does not connect via chrome.runtime.connectNative()." | |
| echo "" | |
| echo "No Native Messaging manifest has been created or modified." | |
| echo "" | |
| echo "If you previously installed an older manifest for ${HOST_NAME}, you may wish to" | |
| echo "remove it manually from your browser's NativeMessagingHosts directory." | |
| echo "" | |
| echo "Installation script completed without making changes." | |
| exit 0 | |
| # (No further actions; native messaging registration is intentionally disabled.) |
| # Create the manifest with actual paths | ||
| $Manifest = @{ | ||
| name = $HostName | ||
| description = "ProofSnap Native Messaging Host - HTTP trigger for screenshot capture" | ||
| path = $HostScript | ||
| type = "stdio" | ||
| allowed_origins = @("chrome-extension://$ExtensionId/") | ||
| } | ||
|
|
||
| $Manifest | ConvertTo-Json | Set-Content $ManifestTarget -Encoding UTF8 | ||
| Write-Host "✓ Created manifest: $ManifestTarget" -ForegroundColor Green | ||
|
|
||
| # Register with Chrome via Registry | ||
| $RegistryPath = "HKCU:\Software\Google\Chrome\NativeMessagingHosts\$HostName" | ||
| if (!(Test-Path $RegistryPath)) { | ||
| New-Item -Path $RegistryPath -Force | Out-Null | ||
| } | ||
| Set-ItemProperty -Path $RegistryPath -Name "(Default)" -Value $ManifestTarget | ||
| Write-Host "✓ Registered with Chrome" -ForegroundColor Green | ||
|
|
||
| # Also register for Edge (Chromium) if present | ||
| $EdgeRegistryPath = "HKCU:\Software\Microsoft\Edge\NativeMessagingHosts\$HostName" | ||
| if (Test-Path "HKLM:\Software\Microsoft\Edge") { | ||
| if (!(Test-Path $EdgeRegistryPath)) { | ||
| New-Item -Path $EdgeRegistryPath -Force | Out-Null | ||
| } | ||
| Set-ItemProperty -Path $EdgeRegistryPath -Name "(Default)" -Value $ManifestTarget | ||
| Write-Host "✓ Registered with Edge" -ForegroundColor Green |
There was a problem hiding this comment.
This script creates and registers a native messaging host manifest (type: "stdio"), but the provided proofsnap_host.py is an HTTP/WebSocket server and the extension doesn't use native messaging APIs. This makes the Windows install path inconsistent/unusable as written. Align the installation method with the actual transport (native messaging vs standalone server) and update the manifest/extension code accordingly.
| # Create the manifest with actual paths | |
| $Manifest = @{ | |
| name = $HostName | |
| description = "ProofSnap Native Messaging Host - HTTP trigger for screenshot capture" | |
| path = $HostScript | |
| type = "stdio" | |
| allowed_origins = @("chrome-extension://$ExtensionId/") | |
| } | |
| $Manifest | ConvertTo-Json | Set-Content $ManifestTarget -Encoding UTF8 | |
| Write-Host "✓ Created manifest: $ManifestTarget" -ForegroundColor Green | |
| # Register with Chrome via Registry | |
| $RegistryPath = "HKCU:\Software\Google\Chrome\NativeMessagingHosts\$HostName" | |
| if (!(Test-Path $RegistryPath)) { | |
| New-Item -Path $RegistryPath -Force | Out-Null | |
| } | |
| Set-ItemProperty -Path $RegistryPath -Name "(Default)" -Value $ManifestTarget | |
| Write-Host "✓ Registered with Chrome" -ForegroundColor Green | |
| # Also register for Edge (Chromium) if present | |
| $EdgeRegistryPath = "HKCU:\Software\Microsoft\Edge\NativeMessagingHosts\$HostName" | |
| if (Test-Path "HKLM:\Software\Microsoft\Edge") { | |
| if (!(Test-Path $EdgeRegistryPath)) { | |
| New-Item -Path $EdgeRegistryPath -Force | Out-Null | |
| } | |
| Set-ItemProperty -Path $EdgeRegistryPath -Name "(Default)" -Value $ManifestTarget | |
| Write-Host "✓ Registered with Edge" -ForegroundColor Green | |
| # Note: ProofSnap host uses HTTP/WebSocket transport, not Chrome native messaging. | |
| # Therefore, we do not create a native messaging manifest or registry entries here. | |
| Write-Host "ℹ Skipping native messaging manifest/registry registration (HTTP/WebSocket transport in use)." -ForegroundColor Yellow | |
| Write-Host " Ensure the ProofSnap host script is started/configured according to the HTTP/WebSocket setup." -ForegroundColor Yellow | |
| # (No native messaging manifest is written to $ManifestTarget, and no Chrome/Edge registry keys are created.) | |
| # This keeps the Windows installation path consistent with the actual transport used by proofsnap_host.py. |
| "path": "PLACEHOLDER_PATH", | ||
| "type": "stdio", | ||
| "allowed_origins": [ | ||
| "chrome-extension://PLACEHOLDER_EXTENSION_ID/" |
There was a problem hiding this comment.
com.numbersprotocol.proofsnap.json is checked in with placeholder values for path and allowed_origins. If this file is meant to be used by users, the placeholders make it easy to misconfigure and it won't work as-is; if it's only a template, consider renaming it (e.g., *.template.json) and ensure install scripts are the source of truth.
| "path": "PLACEHOLDER_PATH", | |
| "type": "stdio", | |
| "allowed_origins": [ | |
| "chrome-extension://PLACEHOLDER_EXTENSION_ID/" | |
| "path": "/usr/local/bin/proofsnap-native-host", | |
| "type": "stdio", | |
| "allowed_origins": [ | |
| "chrome-extension://aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/" |
| triggerSocket.onmessage = (event) => { | ||
| try { | ||
| const message = JSON.parse(event.data); | ||
| console.log('📨 Trigger message received:', message); | ||
|
|
||
| if (message.type === 'connected') { | ||
| console.log('🌐 Trigger server ready:', message.message); | ||
| } else if (message.action === 'capture') { | ||
| // Trigger screenshot capture from HTTP request | ||
| console.log('📸 HTTP trigger: capturing screenshot, mode:', message.mode); | ||
| handleScreenshotCapture(message.mode || 'visible') | ||
| .then((result) => { | ||
| // Send result back to server | ||
| if (triggerSocket && triggerSocket.readyState === WebSocket.OPEN) { | ||
| triggerSocket.send(JSON.stringify({ | ||
| type: 'capture_result', | ||
| result: { success: true, ...result } | ||
| })); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| console.error('HTTP trigger capture failed:', error); | ||
| if (triggerSocket && triggerSocket.readyState === WebSocket.OPEN) { | ||
| triggerSocket.send(JSON.stringify({ | ||
| type: 'capture_result', | ||
| result: { success: false, error: error.message } | ||
| })); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The WebSocket trigger can initiate screenshot capture based solely on data received from a local server, without any authentication/authorization or user opt-in. Any local process that binds to 127.0.0.1:19998 could instruct the extension to capture the active tab, which is a significant privacy/security risk. Gate this feature behind an explicit user setting (default off) and require an authentication token/handshake before accepting action: 'capture' messages (and validate mode against an allowlist).
| // WebSocket connection for HTTP trigger support | ||
| const WS_URL = 'ws://127.0.0.1:19998'; | ||
| let triggerSocket: WebSocket | null = null; | ||
| let wsReconnectTimer: ReturnType<typeof setTimeout> | null = null; | ||
|
|
||
| /** | ||
| * Connect to WebSocket server for HTTP trigger support | ||
| */ | ||
| function connectTriggerServer() { | ||
| // Clear any existing reconnect timer | ||
| if (wsReconnectTimer) { | ||
| clearTimeout(wsReconnectTimer); | ||
| wsReconnectTimer = null; | ||
| } | ||
|
|
||
| try { | ||
| triggerSocket = new WebSocket(WS_URL); |
There was a problem hiding this comment.
This PR is described as adding headline/caption + draft management, but it also introduces an always-on localhost WebSocket trigger client in the background service worker. If HTTP-trigger support is intended, it should be called out in the PR description and likely shipped behind a feature flag / dev-only build to avoid unexpected behavior in production builds.
|
@olgahaha Need your assist for this.
|
numbers-official
left a comment
numbers-official
left a comment
There was a problem hiding this comment.
Overall the feature works well and the code is clean. A few observations below — mostly around security on the native-host server and minor code quality items.
| GET http://localhost:19999/status - Health check | ||
| POST http://localhost:19999/capture - Trigger screenshot | ||
| """ | ||
|
|
There was a problem hiding this comment.
Auto-installing packages via pip install at import time is a security anti-pattern — it runs arbitrary code from PyPI without user consent. If a typosquatted package or supply chain attack is in play, this silently installs it.
Suggestion: fail with a clear error message instead:
try:
import websockets
from websockets.server import serve
except ImportError:
print('ERROR: "websockets" package is required. Install it with:')
print(' pip install websockets')
sys.exit(1)| """Handle HTTP requests for capture triggers""" | ||
|
|
||
| def log_message(self, format, *args): | ||
| """Custom logging""" |
There was a problem hiding this comment.
Setting Access-Control-Allow-Origin: * on a localhost server means any website open in the user's browser can make requests to localhost:19999 and trigger screenshot captures. This is a real attack vector — a malicious webpage could silently call POST /capture via fetch().
Since the intended consumers are:
- The extension (communicates via WebSocket, not HTTP)
- Local scripts (curl/PowerShell — CORS doesn't apply)
...you can safely remove the CORS headers entirely, or restrict to the extension's origin only.
| 'success': False, | ||
| 'error': 'No extension connected. Make sure ProofSnap extension is running.' | ||
| }) | ||
| return |
There was a problem hiding this comment.
asyncio.get_event_loop() is deprecated since Python 3.10 and will show warnings. Since this runs inside an async function, use:
event_loop = asyncio.get_running_loop()|
|
||
| <div className="edit-modal-delete"> | ||
| <button | ||
| className="btn-delete" |
There was a problem hiding this comment.
The error display block has inconsistent indentation — it's indented one level deeper than its sibling elements (edit-modal-actions, edit-modal-delete). Should be aligned at the same level:
{asset.status === 'failed' && asset.metadata?.error && (
<div className="edit-modal-error">
...
</div>
)}(cosmetic only, doesn't affect behavior)
Concern:
|
|
@Steffdarz please check the latest review comments. I don’t think the native-host meets the Chrome extension usage, and I recommend removing it since the PR description doesn’t seem related to the native-host implementation. @wcchung , you can use GitHub Actions to deploy. Merging to the main branch won’t trigger the deployment. I’ve shared the steps with you before. If you’re not sure how to do it next time, you can also ask AI. It can read the code and understand how to deploy through CI/CD. |
|
@Steffdarz For the other comments, you can ignore them. Some suggestions may improve the code, but it’s fine to keep things as they are since the functionality still works well. |
|
I'll ask Omni to improve the code. |
|
This will be replaced by #18 I'll close this PR |
- Add touch event support in selection overlay (issue #1) - Add keyboard navigation/ARIA for asset grid (issue #2) - Add Forgot Password flow in AuthForm (issue #3) - Replace simulated progress with XHR real progress (issue #4) - Add restricted tab URL check before captureVisibleTab (issue #5) - Pass output format/quality to offscreen watermark (issue #6) - Add 30s timeout for offscreen operations (issue #7) - Add ctx.save()/ctx.restore() around canvas drawing (issue #8) - Convert share page from raw DOM to React (issue #9) - Extract inline styles to CSS classes (issue #10) - Rename createSignedMetadata to createMetadataPayload (issue #11) - Add sourcemap: 'hidden' to vite.config.ts (issue #12) - Add pre-build version sync validation (issue #13) Co-authored-by: numbers-official <181934381+numbers-official@users.noreply.github.com>
This update will add the following feature: