Skip to content

Treat remote 404 as missing preview in readRemoteFile #197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
khaliqgant merged 1 commit into from
Jun 10, 2026
+40 −4
Merged

Treat remote 404 as missing preview in readRemoteFile #197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/main/integrations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,6 +953,25 @@ describe('IntegrationsManager', () => {
expect(mock.integrationMountManager.ensureMounted).not.toHaveBeenCalled()
})

it('returns a missing preview when an in-scope remote read 404s instead of rejecting', async () => {
// Historical provider records (e.g. the GitHub issue JSON synthesized from
// Linear sync metadata) are not downloaded locally, so an in-scope read can
// legitimately 404. It must degrade to a missing preview rather than reject
// the IPC handler.
mock.relayClient.readFile.mockImplementationOnce(async (_workspaceId: string, path: string) => {
mock.readFileCalls.push({ workspaceId: _workspaceId, path })
throw Object.assign(new Error('not found'), { status: 404, code: 'not_found' })
})

const manager = new IntegrationsManager()
const preview = await manager.readRemoteFile(
'project-1',
'/slack/channels/C123/messages/1713220123_001100.json'
)

expect(preview).toMatchObject({ kind: 'missing', size: 0 })
})
Comment on lines +956 to +973

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add test coverage for non-404 errors being re-thrown.

The new 404 test correctly verifies graceful degradation, but there's no test ensuring that non-404 errors (e.g., 500, network failures) are still thrown as expected (per line 1082 in integrations.ts). Without this coverage, a bug that swallows all errors could go undetected.

As per coding guidelines, integration notification changes should include regression tests beyond the happy path. Consider adding a test case that mocks relayClient.readFile to throw a non-404 error (e.g., { status: 500 }) and asserts that readRemoteFile rejects with that error.

🧪 Proposed test for non-404 error propagation 
  })

+ it('re-throws non-404 remote read errors instead of returning a missing preview', async () => {
+   mock.relayClient.readFile.mockImplementationOnce(async (_workspaceId: string, path: string) => {
+     mock.readFileCalls.push({ workspaceId: _workspaceId, path })
+     throw Object.assign(new Error('internal server error'), { status: 500 })
+   })
+
+   const manager = new IntegrationsManager()
+
+   await expect(
+     manager.readRemoteFile('project-1', '/slack/channels/C123/messages/1713220123_001100.json')
+   ).rejects.toThrow('internal server error')
+ })
+
  it('rejects targeted remote file reads outside the project integration scope', async () => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('returns a missing preview when an in-scope remote read 404s instead of rejecting', async () => {
// Historical provider records (e.g. the GitHub issue JSON synthesized from
// Linear sync metadata) are not downloaded locally, so an in-scope read can
// legitimately 404. It must degrade to a missing preview rather than reject
// the IPC handler.
mock.relayClient.readFile.mockImplementationOnce(async (_workspaceId: string, path: string) => {
mock.readFileCalls.push({ workspaceId: _workspaceId, path })
throw Object.assign(new Error('not found'), { status: 404, code: 'not_found' })
})
const manager = new IntegrationsManager()
const preview = await manager.readRemoteFile(
'project-1',
'/slack/channels/C123/messages/1713220123_001100.json'
)
expect(preview).toMatchObject({ kind: 'missing', size: 0 })
})
it('returns a missing preview when an in-scope remote read 404s instead of rejecting', async () => {
// Historical provider records (e.g. the GitHub issue JSON synthesized from
// Linear sync metadata) are not downloaded locally, so an in-scope read can
// legitimately 404. It must degrade to a missing preview rather than reject
// the IPC handler.
mock.relayClient.readFile.mockImplementationOnce(async (_workspaceId: string, path: string) => {
mock.readFileCalls.push({ workspaceId: _workspaceId, path })
throw Object.assign(new Error('not found'), { status: 404, code: 'not_found' })
})
const manager = new IntegrationsManager()
const preview = await manager.readRemoteFile(
'project-1',
'/slack/channels/C123/messages/1713220123_001100.json'
)
expect(preview).toMatchObject({ kind: 'missing', size: 0 })
})
it('re-throws non-404 remote read errors instead of returning a missing preview', async () => {
mock.relayClient.readFile.mockImplementationOnce(async (_workspaceId: string, path: string) => {
mock.readFileCalls.push({ workspaceId: _workspaceId, path })
throw Object.assign(new Error('internal server error'), { status: 500 })
})
const manager = new IntegrationsManager()
await expect(
manager.readRemoteFile('project-1', '/slack/channels/C123/messages/1713220123_001100.json')
).rejects.toThrow('internal server error')
})
🤖 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/main/integrations.test.ts` around lines 956 - 973, Add a test that
ensures non-404 errors from the relay are re-thrown: copy the pattern from the
existing 404 test in integrations.test.ts but have
mock.relayClient.readFile.mockImplementationOnce throw an Error/object with
status: 500 (or a network error), then call new
IntegrationsManager().readRemoteFile('project-1', '/slack/...json') and assert
the promise rejects with that same error (use expect(...).rejects.toThrow or
.rejects.toMatchObject to verify the error/status). Reference readRemoteFile,
IntegrationsManager and mock.relayClient.readFile when locating where to insert
the test.

Source: Coding guidelines


it('rejects targeted remote file reads outside the project integration scope', async () => {
const manager = new IntegrationsManager()

Expand Down
25 changes: 21 additions & 4 deletions src/main/integrations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,10 +1060,27 @@ export class IntegrationsManager {
throw new Error('Integration remote file is outside this project integration scope')
}

return this.withIntegrationRemoteHandle(async (handle) => {
const file = await handle.client().readFile(handle.workspaceId, path)
return remoteFileReadToPreview(file)
})
try {
return await this.withIntegrationRemoteHandle(async (handle) => {
const file = await handle.client().readFile(handle.workspaceId, path)
return remoteFileReadToPreview(file)
})
} catch (error) {
// A remote 404 means the file was never synced locally (e.g. historical
// provider records that are not downloaded) or was removed between a
// directory listing and this read. Mirror the local filesystem behavior
// (readTextPreview) by reporting it as a missing preview instead of
// rejecting the IPC handler, so best-effort enrichment readers degrade
// gracefully rather than logging a handler error.
if (isHttpStatus(error, 404)) {
return {
kind: 'missing',
content: error instanceof Error ? error.message : 'Remote file not found',
size: 0
}
}
throw error
}
}

async listRemoteDirectory(projectId: string, remotePath: string): Promise<filesystem.ExplorerEntry[]> {
Expand Down
Loading