Fix: markdown download cancellation logic#3348
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughA one-line guard is added to the server view markdown ChangesMarkdown Download Fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/ui/main/serverView/index.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ui/main/serverView/index.ts (1)
176-190: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explanatory comment for the URL pattern logic.
The code distinguishes between viewing and downloading markdown files based on URL patterns, but there's no explanation of why this specific pattern (
endsWith('download=')) is used or what it represents in the Rocket.Chat server API.📝 Suggested comment
// Intercept markdown file downloads and open in document viewer webviewSession.on('will-download', (_event, item) => { if (item.getFilename().endsWith('.md')) { const downloadUrl = item.getURL(); + // Only intercept for viewer URLs; allow normal downloads to proceed. + // Rocket.Chat server appends 'download=' for view actions vs. download actions. if (!downloadUrl.endsWith('download=')) return; item.cancel();As per coding guidelines, avoid unnecessary comments and write self-documenting code, but this is a necessary comment because the URL pattern distinction is non-obvious and depends on external server API behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/main/serverView/index.ts` around lines 176 - 190, Add a brief explanatory comment above the webviewSession.on('will-download', ...) handler explaining the Rocket.Chat URL pattern: that markdown file links without query "download=" open in-browser while links that end with "download=" are the server's explicit "download" endpoints and should be intercepted/cancelled and opened in our document viewer; reference the check using item.getURL().endsWith('download=') and mention that when matched we cancel the item and dispatch SERVER_DOCUMENT_VIEWER_OPEN_URL with documentFormat 'markdown' so future readers understand the non-obvious URL-based routing decision.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/main/serverView/index.ts`:
- Line 179: Replace the brittle suffix check that tests
downloadUrl.endsWith('download=') with URL parsing: call new URL(item.getURL()),
then inspect url.searchParams (e.g., if (!url.searchParams.has('download'))
return; or check a specific value with url.searchParams.get('download') if
Rocket.Chat uses a value like 'true'/'1' or empty string for the download flag);
update the logic around the downloadUrl/item.getURL() handling and
SERVER_DOCUMENT_VIEWER_OPEN_URL usage to rely on url.searchParams so additional
query params or fragments won’t break the detection.
---
Outside diff comments:
In `@src/ui/main/serverView/index.ts`:
- Around line 176-190: Add a brief explanatory comment above the
webviewSession.on('will-download', ...) handler explaining the Rocket.Chat URL
pattern: that markdown file links without query "download=" open in-browser
while links that end with "download=" are the server's explicit "download"
endpoints and should be intercepted/cancelled and opened in our document viewer;
reference the check using item.getURL().endsWith('download=') and mention that
when matched we cancel the item and dispatch SERVER_DOCUMENT_VIEWER_OPEN_URL
with documentFormat 'markdown' so future readers understand the non-obvious
URL-based routing decision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75eefd3d-caf9-42dc-b920-0f32da1d98c8
📒 Files selected for processing (1)
src/ui/main/serverView/index.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/ui/main/serverView/index.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/ui/main/serverView/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/ui/main/serverView/index.ts
| webviewSession.on('will-download', (_event, item) => { | ||
| if (item.getFilename().endsWith('.md')) { | ||
| const downloadUrl = item.getURL(); | ||
| if (!downloadUrl.endsWith('download=')) return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for URL construction patterns related to markdown downloads in the codebase
echo "=== Searching for download URL patterns ==="
rg -n -C5 'download=' --type ts --type tsx --type js --type jsx
echo -e "\n=== Searching for markdown-related download handling ==="
rg -n -C5 '\.md|markdown' --type ts --type tsx | rg -C3 'download|Download'
echo -e "\n=== Searching for document viewer URL construction ==="
rg -n -C5 'DOCUMENT_VIEWER.*URL' --type ts --type tsxRepository: RocketChat/Rocket.Chat.Electron
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/ui/main/serverView/index.ts"
echo "=== Code around line 179 in $FILE ==="
nl -ba "$FILE" | sed -n '150,210p'
echo
echo "=== Search for endsWith('download=') usage ==="
rg -n "endsWith\\('download='\\)" "$FILE" || true
echo
echo "=== Search for downloadUrl variable and its origin in $FILE ==="
rg -n "downloadUrl" "$FILE" -S
echo
echo "=== Search repository for 'download=' query construction patterns (ts/js only) ==="
rg -n -S "download=" --type-add "ts:*.ts" --type ts --type-add "js:*.js" --type js || true
echo
echo "=== Search repository for will-download handler ==="
rg -n -S "will-download" "$FILE" || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/ui/main/serverView/index.ts"
echo "=== Code around line ~179 in $FILE ==="
awk 'NR>=150 && NR<=210 {printf "%d\t%s\n", NR, $0}' "$FILE" | sed -n '1,200p'
echo
echo "=== Search for endsWith('download=') usage in $FILE ==="
rg -n "endsWith\\('download='\\)" "$FILE" || true
echo
echo "=== Search for will-download handler in $FILE ==="
rg -n "will-download" "$FILE" || true
echo
echo "=== Search for downloadUrl variable and its origin in $FILE ==="
rg -n "downloadUrl" "$FILE" || true
echo
echo "=== Search for download query construction patterns (ts/js only) ==="
rg -n -S "download=" --type-add "ts:*.ts" --type-add "js:*.js" --type ts --type js || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 2732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search repo for 'download=' literal usages (ts/js only) ==="
rg -n -S "download=" --type-add "ts:*.ts" --type-add "js:*.js" --type ts --type js || true
echo
echo "=== Search repo for SERVER_DOCUMENT_VIEWER_OPEN_URL ==="
rg -n "SERVER_DOCUMENT_VIEWER_OPEN_URL" --type-add "ts:*.ts" --type-add "tsx:*.tsx" --type-add "js:*.js" --type-add "jsx:*.jsx" --type ts --type tsx --type js --type jsx || true
echo
echo "=== Search repo for documentFormat: 'markdown' usage ==="
rg -n "documentFormat:\s*'markdown'" --type-add "ts:*.ts" --type-add "tsx:*.tsx" --type-add "js:*.js" --type-add "jsx:*.jsx" --type ts --type tsx --type js --type jsx || true
echo
echo "=== Search repo for 'download' query param parsing ==="
rg -n -S "searchParams\.get\\('download'\\)|searchParams\.has\\('download'\\)|\\bdownload\\b.*(query|string)" --type-add "ts:*.ts" --type-add "tsx:*.tsx" --type-add "js:*.js" --type-add "jsx:*.jsx" --type ts --type tsx --type js --type jsx || trueRepository: RocketChat/Rocket.Chat.Electron
Length of output: 1452
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect small relevant files/sections
sed -n '1,120p' src/servers/actions.ts
echo "----"
sed -n '240,330p' src/servers/reducers.ts
echo "----"
sed -n '1,120p' src/documentViewer/ipc.ts
echo "----"
# show the viewer open handler usage context
sed -n '150,260p' src/ui/components/ServersView/ServerPane.tsx
echo "----"
sed -n '1,80p' src/ui/main/serverView/index.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 9679
Harden the markdown download intercept URL check (line 179)
if (!downloadUrl.endsWith('download=')) return;is an exact suffix match; it won’t trigger ifdownloadisn’t literally the final characters (e.g., additional query params/values, fragments, or different server URL shape).- Replace the suffix logic with robust URL parsing (
new URL(item.getURL())+searchParams) and document what Rocket.Chat generates for the “open in document viewer vs allow download” case. - Repo search shows this
download=literal is only used here;SERVER_DOCUMENT_VIEWER_OPEN_URLjust forwardsdocumentUrlunchanged, so correctness depends entirely on what Electron reports initem.getURL().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/main/serverView/index.ts` at line 179, Replace the brittle suffix
check that tests downloadUrl.endsWith('download=') with URL parsing: call new
URL(item.getURL()), then inspect url.searchParams (e.g., if
(!url.searchParams.has('download')) return; or check a specific value with
url.searchParams.get('download') if Rocket.Chat uses a value like 'true'/'1' or
empty string for the download flag); update the logic around the
downloadUrl/item.getURL() handling and SERVER_DOCUMENT_VIEWER_OPEN_URL usage to
rely on url.searchParams so additional query params or fragments won’t break the
detection.
1 participant
Closes #3345
Summary by CodeRabbit