Skip to content

Merge pull request #185 from Open-VCS/codex/github-mention-build-script-issue Make Tauri pre-build/dev hooks resilient to script location#186

Merged
Jordonbc merged 4 commits intostablefrom
Dev
Mar 19, 2026
Merged

Merge pull request #185 from Open-VCS/codex/github-mention-build-script-issue Make Tauri pre-build/dev hooks resilient to script location#186
Jordonbc merged 4 commits intostablefrom
Dev

Conversation

@Jordonbc
Copy link
Collaborator

No description provided.

@Jordonbc Jordonbc merged commit 51ddcde into stable Mar 19, 2026
5 checks passed
@github-actions
Copy link

Code Review: PR #185

Summary

This PR makes Tauri pre-build/dev hooks resilient to script location by replacing simple node commands with inline Node.js scripts that search multiple candidate paths.

Issues Found

1. Code Duplication (Medium)
The inline scripts are identical except for the argument (dev vs build). This violates DRY - any bug fix requires changes in two places. Consider extracting to a helper script.

2. Missing Semicolons (Low)
The inline script lacks semicolons, which contradicts the project's stated convention: "Use semicolons at statement ends" (AGENTS.md).

3. Unhandled Exception (Medium)
If spawnSync throws (e.g., process.execPath not found), the script crashes unhandled. Wrap in try/catch:

try {
  const result = spawnSync(process.execPath, [script, 'dev'], { stdio: 'inherit' });
  process.exit(result.status ?? 1);
} catch (e) {
  console.error('Failed to spawn script:', e);
  process.exit(1);
}

4. Signal Handling (Low)
When a process is killed by a signal, result.status is null but result.signal is set. Consider:

process.exit(result.status ?? (result.signal ? 1 : 1));

5. Hardcoded Candidates (Low)
The candidate paths list is fragile. If the project structure changes, this needs updating.

Positive Aspects

  • Successfully resolves the original issue (finding scripts from different CWDs)
  • Provides clear error messages when script is missing
  • Properly forwards exit codes

Recommendation

Approve with suggestions to extract a helper script to avoid duplication and add error handling. The core functionality is correct.

New%20session%20-%202026-03-19T04%3A10%3A40.075Z
opencode session  |  github run

@github-actions
Copy link

Code Review: PR #185

Summary

Makes Tauri pre-build/dev hooks resilient to script location by replacing hardcoded paths with inline Node.js scripts that search multiple candidate paths.

Issues

1. Hardcoded candidate paths may not cover all scenarios

  • Candidate paths are relative to process.cwd(), but the Tauri CLI might be invoked from different directories (workspace root, Backend/, or even nested directories).
  • The list ['scripts/...','Backend/scripts/...','../Backend/scripts/...','../../scripts/...'] doesn't cover cases like ../../Backend/scripts/... or absolute paths.

2. No logging when script is found

  • When the script is successfully located, there's no indication which path was chosen. This could make debugging confusing if the wrong script is accidentally picked up (e.g., if someone has a stale scripts/ elsewhere).

3. Error message could be clearer

'Unable to locate Backend/scripts/run-tauri-before-command.js'

But it's checking paths relative to CWD, not just Backend/scripts/. The error implies it only looks in Backend/scripts/ when it actually checks 4 different relative paths.

Suggestions

  1. Add debug logging: Log which candidate path was found, e.g., console.log('Using script: ' + script) before execution.

  2. Document expected CWD: Add a comment explaining from which directories Tauri commands are typically invoked.

  3. Consider path normalization: Use path.normalize() on the resolved paths for consistency.

  4. Testing gap: The PR only tested from one CWD. Consider documenting which working directories the build/dev commands are expected to run from.

Verdict

Functional improvement with minor polish opportunities. The core approach is sound—searching multiple candidates handles the main use case. The inline script is verbose but acceptable for a configuration file where shell portability is limited.

New%20session%20-%202026-03-19T04%3A10%3A54.555Z
opencode session  |  github run

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.

1 participant