feat: add channel name resolution and canvas support#7
feat: add channel name resolution and canvas support#7brianevanmiller wants to merge 2 commits intoshaharia-lab:mainfrom
Conversation
- Add resolveChannel() to accept channel names (general, #general) in addition to IDs - Update conversations read and messages send to use channel name resolution - Add new canvases command group with list and read subcommands - Implement HTML to markdown conversion for canvas content using node-html-parser - Support fetching canvas comments via file thread system - Add SlackFile and SlackFileShare types New commands: slackcli canvases list [--channel <name>] [--limit <n>] slackcli canvases read <canvas-id> [--channel <name>] [--raw] [--include-comments] Channel names now work in: slackcli conversations read general slackcli messages send --recipient-id "#random" --message "Hello" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This is a Bun project that uses bun.lock for dependency tracking. The npm lockfile was accidentally generated and is unnecessary. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
shahariaazam
left a comment
There was a problem hiding this comment.
Required Before Merge
- Rebase on main - Currently based on v0.1.1, main is now v0.2.0
- Update CHANGELOG.md - Add v0.3.0 section with these features
- Update README.md - Document canvas commands and channel name resolution
- Complete test plan checklist
Key Issues
See inline comments for details on:
- Channel resolution performance (no caching)
- Type safety issues (
client: any) - Missing file size limits
- Input validation needed
Great features overall! Just needs documentation and some refinements.
shahariaazam
left a comment
There was a problem hiding this comment.
See inline comments for specific issues to address.
| const response = await this.listConversations({ | ||
| types: 'public_channel,private_channel', | ||
| limit: 200, | ||
| cursor, |
There was a problem hiding this comment.
Performance issue: No caching. Every command re-fetches all channels with pagination.
Add caching:
private channelCache = new Map<string, {id: string, expires: number}>();
async resolveChannel(channelOrName: string): Promise<string> {
const cached = this.channelCache.get(channelOrName);
if (cached && cached.expires > Date.now()) return cached.id;
// ... existing logic ...
this.channelCache.set(channelName, {
id: match.id,
expires: Date.now() + 300000
});
return match.id;
}| if (options.includeComments) { | ||
| await fetchAndDisplayComments(client, file, spinner); | ||
| } | ||
| } catch (err: any) { |
There was a problem hiding this comment.
Type safety: Change client: any to proper type.
import type { SlackClient } from '../lib/slack-client.ts';
async function fetchAndDisplayComments(
client: SlackClient,
file: SlackFile,
spinner: ReturnType<typeof ora>
): Promise<void>|
|
||
| // For browser auth, also add the cookie | ||
| if (this.config.auth_type === 'browser') { | ||
| const encodedXoxdToken = encodeURIComponent(this.config.xoxd_token); |
There was a problem hiding this comment.
Missing size limit: Large files can cause OOM.
const contentLength = response.headers.get('content-length');
const MAX_SIZE = 10 * 1024 * 1024; // 10MB
if (contentLength && parseInt(contentLength) > MAX_SIZE) {
throw new Error('Canvas too large (max: 10MB)');
}
return response.text();| let channelId: string | undefined; | ||
| if (options.channel) { | ||
| spinner.text = 'Resolving channel...'; | ||
| channelId = await client.resolveChannel(options.channel); |
There was a problem hiding this comment.
No input validation:
const limit = parseInt(options.limit);
if (isNaN(limit) || limit <= 0 || limit > 1000) {
throw new Error('Limit must be 1-1000');
}
shahariaazam
left a comment
There was a problem hiding this comment.
Additional review comments on documentation and testing.
| @@ -26,17 +26,17 @@ | |||
| "license": "MIT", | |||
| "dependencies": { | |||
| "@slack/web-api": "^7.11.0", | |||
There was a problem hiding this comment.
Dependency pinning: Using ^7.0.2 allows minor version updates that could introduce breaking changes.
For CLI tools, consider tighter pinning:
"node-html-parser": "~7.0.2"| } | ||
|
|
||
| function processUnorderedList(node: HTMLElement): string { | ||
| const items = node.querySelectorAll(':scope > li'); |
There was a problem hiding this comment.
Nested lists not supported: This only handles direct children with :scope > li.
Consider testing with canvases containing nested lists to see if recursion is needed.
| spinner.start(`Fetching comments from #${channelName}...`); | ||
|
|
||
| try { | ||
| const repliesResponse = await client.getConversationReplies(channelId, share.ts, { |
There was a problem hiding this comment.
Hard-coded limit: Why 100? Extract to constant:
const MAX_COMMENT_REPLIES = 100;Also consider what happens if there are >100 comments.
Documentation RequiredREADME.md - Missing documentation for:
CHANGELOG.md - Missing v0.3.0 section with:
Note: PR is based on v0.1.1, but main is now v0.2.0. Rebase needed. |
Summary
This PR adds two major features to slackcli:
1. Channel Name Resolution
general,#general) instead of requiring IDsconversations readandmessages sendcommands2. Canvas Support
New
canvasescommand group for reading Slack canvas documents:slackcli canvases list- List all canvases in the workspaceslackcli canvases read <id>- Read canvas content as markdown--channel <name>- Read a channel's associated canvas--raw- Output raw HTML instead of parsed markdown--include-comments- Include threaded comments from all sharesNote: The Slack API does not provide a public method to read canvas content directly. This implementation uses the files API to download the HTML export and converts it to markdown using
node-html-parser.New Usage Examples
Test plan
--include-commentsfetches threaded replies--rawoutputs HTML directly🤖 Generated with Claude Code