Skip to content

feat(frontend): add script image workflow and no-capture toggle UI - #4

Closed
Builder106 wants to merge 3 commits into
ArunNGun:mainfrom
SankofaForge:pr/3-frontend-script-images
Closed

Builder106 wants to merge 3 commits into
ArunNGun:mainfrom
SankofaForge:pr/3-frontend-script-images

Conversation

@Builder106

Copy link
Copy Markdown
Contributor

Summary

Adds script image support and no-capture UI controls in the renderer.

Changes

  • script image parsing/serialization, paste/drop insertion, preview/removal
  • read-view image rendering updates
  • no-capture button UI and state wiring
  • related HTML/CSS updates

Note

Intended as PR 3 of 3 in split sequence.

@Builder106
Builder106 requested a review from ArunNGun as a code owner March 25, 2026 01:48

@ArunNGun ArunNGun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Code Review

@ArunNGun ArunNGun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Code Review

✅ What's good

Image serialization approach (parseStoredScript / serializeScriptText) is clever — embedding base64 image data as a tagged footer in the script text avoids breaking the existing storage format. The atob/btoa with JSON is clean and has a try/catch fallback.

insertAtCursor is well-written — correctly handles selection ranges and updates stats/preview after insert.

Rust side (lib.rs) is clean and minimal. set_content_protected for the no-capture toggle is the right Tauri API for this. Error handling is consistent.

CSS is tidy. The image-only fullscreen centering with flexbox is a nice touch for image-only scripts.


⚠️ Things to flag

1. buildScript receives scriptInput.value — worth a comment
scriptInput.value only contains human-readable text (images are in state.currentScriptImages), so the [[__images__:...]] footer never appears here. This is correct but subtle — a short comment explaining the separation would help future readers.

2. Image IDs — potential collision under rapid paste

const imageId = `img_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`

Only 6 chars of randomness. Low risk, but pasting multiple images in quick succession could theoretically collide. Consider crypto.randomUUID() — available in modern browsers and Tauri.

3. No image size limit on paste/drop
Large images get stored as full base64 data URLs in localStorage. A single high-res screenshot could be 2–5MB+ encoded, and localStorage is typically capped at 5–10MB total. A silent failure here could corrupt stored scripts. Worth adding a file size check with a user-visible warning.

4. Unnecessary setHideFromCapture call on startup when value is false

const initialHideCapture = localStorage.getItem(HIDE_CAPTURE_KEY) === '1'
applyHideFromCapture(initialHideCapture)

When initialHideCapture is false (the default), this still invokes API.setHideFromCapture(false) on every startup. Minor, but could be guarded with if (initialHideCapture).

5. package-lock.json added without visible package.json changes
The lock file is 232 lines but no package.json diff is shown. Was the lock file missing before? Worth clarifying — if @tauri-apps/cli was already a dep, the lock should have existed.

6. .gitignore un-ignore for src-tauri/src/
The !src-tauri/src/ lines look like they're fixing an accidental exclusion of Rust source files. Fine, but a brief PR note on why would help.


Summary

Blocking issues None
Recommended crypto.randomUUID(), image size limit before storing
Nice to have Comment on serialization format, startup guard for hide-capture

Overall solid work — the image workflow is well thought out end-to-end. Happy to approve once the size limit concern is addressed or acknowledged.

@akeslo

akeslo commented Apr 21, 2026

Copy link
Copy Markdown

Code review

Found 3 issues:

  1. src-tauri/src/lib.rs will conflict with (or overwrite) the existing 858-line Rust backend (bug — PR is 60 commits behind main; lib.rs didn't exist at branch point but does now)

The PR's 52-line lib.rs was added before the current full Rust backend existed on main. Merging will produce a conflict — and if manually resolved by taking the PR version, all existing Tauri commands (notch window management, config persistence, scripts storage, global shortcuts, audio, tray, etc.) would be lost.

use tauri::Manager;
fn configure_main_window_overlay(app: &tauri::AppHandle) -> Result<(), String> {
let window = app
.get_webview_window("main")
.ok_or_else(|| "main window not found".to_string())?;
window.set_always_on_top(true).map_err(|e| e.to_string())?;
#[cfg(target_os = "macos")]
{
window
.set_visible_on_all_workspaces(true)
.map_err(|e| e.to_string())?;
}
Ok(())
}
#[tauri::command]
fn set_hide_from_capture(app: tauri::AppHandle, hide: bool) -> Result<(), String> {
let window = app
.get_webview_window("main")
.ok_or_else(|| "main window not found".to_string())?;
window
.set_content_protected(hide)
.map_err(|e| e.to_string())?;
Ok(())
}
#[tauri::command]
fn refresh_overlay_behavior(app: tauri::AppHandle) -> Result<(), String> {
configure_main_window_overlay(&app)?;
Ok(())
}
#[cfg_attr(mobile, tauri::mobile_entry_point)]
pub fn run() {
tauri::Builder::default()
.plugin(tauri_plugin_fs::init())
.plugin(tauri_plugin_global_shortcut::Builder::new().build())
.plugin(tauri_plugin_positioner::init())
.setup(|app| {
if let Err(err) = configure_main_window_overlay(&app.handle()) {
eprintln!("overlay setup failed: {err}");
}
Ok(())
})
.invoke_handler(tauri::generate_handler![
set_hide_from_capture,
refresh_overlay_behavior
])
.run(tauri::generate_context!())
.expect("error while running tauri application");
}

  1. tauri.conf.json resets version and strips build pipeline (bug — will break CI and dev build)

The PR's version resets version from 3.0.01.0.0 and drops the full build config (beforeDevCommand, devUrl, beforeBuildCommand, frontendDist: ../dist) down to only frontendDist: ../frontend/renderer. This breaks the React frontend build pipeline.

{
"$schema": "https://schema.tauri.app/config/2",
"productName": "OpenTeleprompter",
"version": "1.0.0",
"identifier": "com.openteleprompter.teleprompter",
"build": {
"frontendDist": "../frontend/renderer"
},
"app": {
"withGlobalTauri": true,
"windows": [
{
"label": "main",
"title": "OpenTeleprompter",
"width": 1200,
"height": 800,
"alwaysOnTop": true,
"resizable": true
}
],
"security": {
"csp": null,
"dangerousDisableAssetCspModification": true
},
"macOSPrivateApi": true
},
"bundle": {
"active": true,

  1. No image size limit before storing to localStorage (bug — silent data loss risk)

insertImageFiles stores full base64 data URLs without a size check. A single hi-res screenshot can be 3-5 MB encoded; localStorage is typically capped at 5-10 MB total. Hitting the cap throws a QuotaExceededError that is currently uncaught, which can silently corrupt stored scripts.

function insertAtCursor(el, value) {
const start = el.selectionStart ?? el.value.length
const end = el.selectionEnd ?? el.value.length
const before = el.value.slice(0, start)
const after = el.value.slice(end)
el.value = before + value + after
const caret = start + value.length
el.selectionStart = caret
el.selectionEnd = caret
updateStats(el.value)

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Builder106

Builder106 commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

Blocker: PR targets a frontend tree that's no longer active on main

This PR's base (0cd37b9) predates the v2.0 / v3.0 Tauri rewrite. After investigating, the PR can't merge as-is:

  1. frontend/renderer/app.js is no longer the live frontend. main's vite.config.js builds index.html + settings.html from the project root, sourced from the React app under src/ (App.jsx, views/EditView.jsx, views/ReadView.jsx, etc.). tauri.conf.json on main ships frontendDist: ../dist (Vite output). The frontend/renderer/ tree still exists in the repo but is no longer wired up — modifying app.js there has no runtime effect in v3.0 builds.

  2. It depends on PR feat(tauri): configure main window overlay and capture controls #3, which is itself obsolete. This branch includes commit b7ab211 (the Tauri overlay/capture commit). I posted a blocker comment on feat(tauri): configure main window overlay and capture controls #3 explaining that set_hide_from_capture duplicates main's already-shipped Config.screenshare_hidden / apply_screenshare_mode() path, and that the tauri.conf.json changes there would break the build.

  3. The image-paste/drop workflow itself is still a worthwhile feature. The serialization approach (base64 footer in script text) and the renderer logic are reasonable. But porting it to current main means re-implementing it in React (EditView.jsx for the editor, ReadView.jsx for image rendering) on top of the TipTap editor that's now in use, plus the previously raised review feedback (crypto.randomUUID() for image IDs, localStorage quota guard with user-visible error, comment on the [[__images__:...]] separation, startup-guard for hide-capture).

Recommendation

Close this PR (and #3) and reopen the script-image workflow as a new PR branched from current main, targeting the React/TipTap editor in src/views/EditView.jsx and the read view in src/views/ReadView.jsx. The unaddressed review notes from this thread (size cap, UUID, etc.) should fold into that PR.

Modifying the dead frontend/renderer/ tree wouldn't actually deliver the feature in shipped builds, and re-targeting to React is a re-implementation, not a rebase.

@Builder106

Copy link
Copy Markdown
Contributor Author

Closing as superseded by the v3.0.0 rewrite. The overlay/no-capture pieces here duplicate what's already in lib.rs (see #3), and the script-image workflow was written against the old vanilla-JS frontend/renderer/app.js, which v3.0.0 replaced with the React/Tiptap editor in src/. The image feature has been reimplemented cleanly against the React editor (as a first-class Tiptap image node, not the [[image:…]] base64-in-textarea hack) in #5.

@Builder106 Builder106 closed this Jun 10, 2026
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.

3 participants