Skip to content

[Reviewed] fix: correct commands path in deploy-commands.js (#1)#23

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

[Reviewed] fix: correct commands path in deploy-commands.js (#1)#23
hanwucui wants to merge 1 commit into
IN3PIRE:mainfrom
hanwucui:fix/deploy-commands-path

Conversation

@hanwucui
Copy link
Copy Markdown

Fixes issue #1 from #21

Changes

  • Fixed path: Changed 'commands' to 'slashcommands' in deploy-commands.js:7 to match the actual directory structure
  • Added safety check: Added s.existsSync() guard to gracefully handle missing slashcommands directory

Problem

deploy-commands.js referenced src/commands but the actual folder is src/slashcommands, which breaks the deployment script completely.

Testing

  • Verified the slashcommands directory exists with the expected command files (avatar.js, ping.js, server.js, uptime.js, user.js)

- deploy-commands.js referenced 'src/commands' but the actual folder is 'src/slashcommands'
- Added existsSync check to gracefully handle missing directory
- Fixes IN3PIRE#21 issue IN3PIRE#1 (Critical: breaks deployment script)
@TrivCodez
Copy link
Copy Markdown
Contributor

TrivCodez commented Apr 29, 2026

Pull Request Review — Issue #1 (from #21)

✅ Requirements — MET

Requirement Status
Bug fix correctness
Code quality 🟢
No breaking changes
Defensive programming
Documentation

🔴 Critical — BLOCKING MERGE

This PR removes global command deployment logic that exists in the base branch:

- await rest.put(Routes.applicationCommands(process.env.CLIENT_ID), { body: commands });
+ const route = process.env.GUILD_ID
+ ? Routes.applicationGuildCommands(process.env.CLIENT_ID, process.env.GUILD_ID)
+ : Routes.applicationCommands(process.env.CLIENT_ID);
+ await rest.put(route, { body: commands });

The base implementation only supports global commands. Your change switches to guild-only when GUILD_ID is present, but completely removes global command support when GUILD_ID is missing by adding process.exit(1).


🟡 Warnings

  1. Duplicate effort — PRs [Reviewed] refactor(commands): update commands path and enhance uptime command #24, fix: correct slashcommands directory path (Fixes #1) #25, [Reviewed] fix: update commands path in deploy-commands.js (Item 1) #32 also fix the same path issue.

  2. Overly aggressive error handling — Exiting the process on missing directory prevents the script from being used in setups where commands are deployed globally without a local slashcommands directory (e.g., CI/CD pipelines).

  3. Inconsistent pattern — Other open PRs ([Reviewed] refactor(commands): update commands path and enhance uptime command #24, fix: correct slashcommands directory path (Fixes #1) #25) keep the existing global command logic intact.


✅ Code Quality Highlights

  • ✅ Good use of fs.existsSync for defensive programming
  • ✅ Clear error message with path information

📋 Recommendation: REQUEST CHANGES

Please modify to preserve existing functionality:

const commandsPath = path.join(__dirname, 'slashcommands');

if (!fs.existsSync(commandsPath)) {
 console.warn(`Warning: Commands directory not found: ${commandsPath}`);
 console.warn('No commands will be deployed.');
 process.exit(0); // Exit gracefully, not with error code
}

// Keep original logic
await rest.put(Routes.applicationCommands(process.env.CLIENT_ID), { body: commands });

Alternative: If you want to add guild support, follow the pattern from PR #24 which adds it without breaking global command deployment.

Given the duplication with other PRs, consider closing this PR and letting #25 (minimal fix) or #24 (enhanced fix) proceed instead.

@TrivCodez TrivCodez changed the title fix: correct commands path in deploy-commands.js (#1) [Reviewed] fix: correct commands path in deploy-commands.js (#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