Skip to content

[Reviewed] fix: update commands path in deploy-commands.js (Item 1)#32

Open
slendereater-sketch wants to merge 1 commit into
IN3PIRE:mainfrom
slendereater-sketch:fix/deploy-commands-path
Open

[Reviewed] fix: update commands path in deploy-commands.js (Item 1)#32
slendereater-sketch wants to merge 1 commit into
IN3PIRE:mainfrom
slendereater-sketch:fix/deploy-commands-path

Conversation

@slendereater-sketch
Copy link
Copy Markdown

Fixes Item 1 in Issue #21. The deployment script was referencing a non-existent 'commands' directory instead of 'slashcommands'.

Copy link
Copy Markdown
Contributor

@TrivCodez TrivCodez left a comment

Choose a reason for hiding this comment

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

🔍 Pull Request Review — Issue #1

Thanks for fixing the incorrect path, @slendereater-sketch. This is an important bug fix that resolves a core setup issue.


🔍 Changes Reviewed

  • Path fix: commandsslashcommands is correct and matches the actual directory structure
  • Files modified: src/deploy-commands.js (1 line change)

🚨 Critical — BLOCKING MERGE

Missing Directory Existence Check

  • If the slashcommands/ directory doesn't exist, fs.readdirSync() will throw ENOENT and crash the deployment script
  • This is especially important for a deployment script that new users might run before creating any commands
  • Fix Required: Add defensive error handling:
const commandsPath = path.join(__dirname, 'slashcommands');

+// Check if directory exists before reading
+if (!fs.existsSync(commandsPath)) {
+  console.warn(`[Warning]: ${commandsPath} does not exist. No commands to deploy.`);
+  return;
+}
+
const commandFiles = fs.readdirSync(commandsPath).filter(f => f.endsWith('.js'));

Alternative: Wrap in try/catch:

let commandFiles;
try {
  commandFiles = fs.readdirSync(commandsPath).filter(f => f.endsWith('.js'));
} catch (error) {
  console.error(`[Error]: Failed to read commands directory:`, error.message);
  process.exit(1);
}

🟡 Minor Issues

No newline at EOF

  • Ensures consistency across project files
  • Add final newline to avoid "\ No newline at end of file" marker

💡 Suggestions

Error Message Clarity

  • Current generic console.error(error) in catch block doesn't provide context
  • Improve to: console.error('[Error]: Failed to deploy commands:', error.message);

✅ Good

  • Simple, targeted fix that addresses the core issue
  • Clean diff with no unnecessary changes
  • Aligns with the project's bounty issue #21 Item #1

🔍 Verification Steps

  1. Run node src/deploy-commands.js when src/slashcommands/ exists → success
  2. Run node src/deploy-commands.js when src/slashcommands/ is missing → should warn, not crash

⚖️ Verdict: REQUEST CHANGES

The path correction is correct, but the script needs defensive error handling to prevent crashes on missing directories. This is critical for user experience, especially for new bot developers setting up the project for the first time.

@TrivCodez TrivCodez changed the title fix: update commands path in deploy-commands.js (Item 1) [Reviewed] fix: update commands path in deploy-commands.js (Item 1) Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants