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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (6)**/*.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{tsx,ts}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.spec.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.main.spec.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{spec.ts,main.spec.ts}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🔇 Additional comments (1)
WalkthroughA one-line guard is added to the server view markdown will-download handler: it reads the download URL and returns early unless the URL ends with ChangesMarkdown Download Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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
Documents the actual contract: the web client title-link builds its URL with URLSearchParams (serializes as ?download=), while the download button concatenates a bare ?download. The trailing = is the only signal distinguishing view intent from download intent.
|
Thanks for the fix @tyroneyeh — the
Follow-ups that make this interception a fallback rather than the primary path: #3354 exposes a |
jeanfbrito
left a comment
jeanfbrito
left a comment
There was a problem hiding this comment.
Verified against the server code: title-link URLs end with download= (URLSearchParams serialization in GenericFileAttachment.getExternalUrl), download button URLs end with bare ?download (AttachmentDownloadBase), so the guard correctly lets real downloads through while keeping the viewer working for title clicks. Unit tests added on the branch cover both forms.
2 participants
Closes #3345
Summary by CodeRabbit
Bug Fixes
Tests