feat: remove Unraid API log download functionality#1793
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoved the API logs download feature across backend and frontend: deleted the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant RestController
participant RestService
participant ApiReportService
participant Filesystem
Note over Client,RestController: Previous flow (before removal)
Client->>RestController: GET /graphql/api/logs
RestController->>RestService: getLogs()
RestService->>ApiReportService: generateReport() / run execa to tar/zip
ApiReportService-->>RestService: report stream / path
RestService->>Filesystem: read stream
RestService-->>RestController: ReadStream
RestController-->>Client: 200 application/x-gtar (stream)
sequenceDiagram
autonumber
participant Client
participant RestController
Note over Client,RestController: New flow (after removal)
Client->>RestController: GET /graphql/api/logs
RestController--xClient: 404 / route not present (request won't reach controller)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (21)
💤 Files with no reviewable changes (16)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (6)📚 Learning: 2025-04-23T20:19:42.542ZApplied to files:
📚 Learning: 2025-04-23T20:19:42.542ZApplied to files:
📚 Learning: 2025-08-09T01:03:29.676ZApplied to files:
📚 Learning: 2025-01-29T16:36:04.777ZApplied to files:
📚 Learning: 2025-01-29T16:35:43.699ZApplied to files:
📚 Learning: 2024-11-15T16:22:03.485ZApplied to files:
🔇 Additional comments (10)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { RCloneModule } from '@app/unraid-api/graph/resolvers/rclone/rclone.module.js'; | ||
| import { SsoModule } from '@app/unraid-api/graph/resolvers/sso/sso.module.js'; | ||
| import { RestController } from '@app/unraid-api/rest/rest.controller.js'; | ||
| import { RestService } from '@app/unraid-api/rest/rest.service.js'; | ||
|
|
||
| @Module({ | ||
| imports: [RCloneModule, CliServicesModule, SsoModule], | ||
| imports: [RCloneModule, SsoModule], | ||
| controllers: [RestController], |
There was a problem hiding this comment.
RestModule now imports only the rclone and SSO modules (lines 3–10), so ApiReportService is no longer provided. The existing integration suites in api/src/unraid-api/rest/__test__/rest-module.integration.test.ts and rest-module-dependencies.test.ts still call Test.createTestingModule({ imports: [RestModule] }) and then retrieve ApiReportService, which will now fail with a missing provider error. Running the test suite will therefore fail immediately; either reintroduce the CliServicesModule provider/export chain or update those tests to reflect the removal.
Useful? React with 👍 / 👎.
| export class RestService { | ||
| protected logger = new Logger(RestService.name); | ||
|
|
||
| constructor(private readonly apiReportService: ApiReportService) {} | ||
|
|
||
| async saveApiReport(pathToReport: string) { | ||
| try { | ||
| const apiReport = await this.apiReportService.generateReport(); | ||
| this.logger.debug('Report object %o', apiReport); | ||
| await writeFile(pathToReport, JSON.stringify(apiReport, null, 2), 'utf-8'); | ||
| } catch (error) { | ||
| this.logger.warn('Could not generate report for zip with error %o', error); | ||
| } | ||
| } | ||
|
|
||
| async getLogs(): Promise<ReadStream> { | ||
| const logPath = getters.paths()['log-base']; | ||
| const graphqlApiLog = '/var/log/graphql-api.log'; | ||
|
|
||
| try { | ||
| await this.saveApiReport(join(logPath, 'report.json')); | ||
| } catch (error) { | ||
| this.logger.warn('Could not generate report for zip with error %o', error); | ||
| } | ||
| const zipToWrite = join(logPath, '../unraid-api.tar.gz'); | ||
|
|
||
| const logPathExists = Boolean(await stat(logPath).catch(() => null)); | ||
| if (logPathExists) { | ||
| try { | ||
| // Build tar command arguments | ||
| const tarArgs = ['-czf', zipToWrite, logPath]; | ||
|
|
||
| // Check if graphql-api.log exists and add it to the archive | ||
| const graphqlLogExists = Boolean(await stat(graphqlApiLog).catch(() => null)); | ||
| if (graphqlLogExists) { | ||
| tarArgs.push(graphqlApiLog); | ||
| this.logger.debug('Including graphql-api.log in archive'); | ||
| } | ||
|
|
||
| // Execute tar with timeout and capture output | ||
| await execa('tar', tarArgs, { | ||
| timeout: 60000, // 60 seconds timeout for tar operation | ||
| reject: true, // Throw on non-zero exit (default behavior) | ||
| }); | ||
|
|
||
| const tarFileExists = Boolean(await stat(zipToWrite).catch(() => null)); | ||
|
|
||
| if (tarFileExists) { | ||
| return createReadStream(zipToWrite); | ||
| } else { | ||
| throw new Error( | ||
| 'Failed to create log zip - tar file not found after successful command' | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| // Build detailed error message with execa's built-in error info | ||
| let errorMessage = 'Failed to create logs archive'; | ||
|
|
||
| if (error && typeof error === 'object' && 'command' in error) { | ||
| const execaError = error as ExecaError; | ||
|
|
||
| if (execaError.timedOut) { | ||
| errorMessage = `Tar command timed out after 60 seconds. Command: ${execaError.command}`; | ||
| } else if (execaError.exitCode !== undefined) { | ||
| errorMessage = `Tar command failed with exit code ${execaError.exitCode}. Command: ${execaError.command}`; | ||
| } | ||
|
|
||
| // Add stderr/stdout if available | ||
| if (execaError.stderr) { | ||
| errorMessage += `. Stderr: ${execaError.stderr}`; | ||
| } | ||
| if (execaError.stdout) { | ||
| errorMessage += `. Stdout: ${execaError.stdout}`; | ||
| } | ||
|
|
||
| // Include the short message from execa | ||
| if (execaError.shortMessage) { | ||
| errorMessage += `. Details: ${execaError.shortMessage}`; | ||
| } | ||
| } else if (error instanceof Error) { | ||
| errorMessage += `: ${error.message}`; | ||
| } | ||
|
|
||
| this.logger.error(errorMessage, error); | ||
| throw new Error(errorMessage); | ||
| } | ||
| } else { | ||
| throw new Error('No logs to download'); | ||
| } | ||
| } | ||
|
|
||
| async getCustomizationPath(type: 'banner' | 'case'): Promise<string | null> { | ||
| switch (type) { | ||
| case 'banner': |
There was a problem hiding this comment.
RestService now only exposes customization helpers (lines 8–23) and no longer defines saveApiReport. The api/src/unraid-api/rest/__test__/rest.service.test.ts suite still calls restService.saveApiReport(...), which now throws restService.saveApiReport is not a function, causing that test file to fail whenever the suite runs. Reintroducing the method or updating the dependent tests/callers is needed to keep the tests passing.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/src/unraid-api/rest/rest.service.ts (1)
1-25: RestService customization methods look good; adjust import formatting for PrettierThe
getCustomizationPath/getCustomizationStreamlogic is simple and correct, and the type restriction to'banner' | 'case'matches the controller validation.The Test API check is complaining about the import formatting on line 5; to satisfy Prettier you can expand this named import:
-import { getBannerPathIfPresent, getCasePathIfPresent } from '@app/core/utils/images/image-file-helpers.js'; +import { + getBannerPathIfPresent, + getCasePathIfPresent, +} from '@app/core/utils/images/image-file-helpers.js';api/src/unraid-api/rest/rest.service.spec.ts (1)
4-4: Vitest import usage is consistent and sufficient for this specUsing
beforeEach,describe,expect, anditfrom Vitest is aligned with the repo’s test setup, and there’s no need to importvihere since this spec doesn’t mock anything. Given behavior tests live inrest.service.test.ts, this lightweight “service is defined” spec is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/src/unraid-api/rest/rest.controller.test.ts(0 hunks)api/src/unraid-api/rest/rest.controller.ts(1 hunks)api/src/unraid-api/rest/rest.module.ts(1 hunks)api/src/unraid-api/rest/rest.service.spec.ts(1 hunks)api/src/unraid-api/rest/rest.service.test.ts(1 hunks)api/src/unraid-api/rest/rest.service.ts(1 hunks)web/__test__/components/DownloadApiLogs.test.ts(0 hunks)web/__test__/components/Wrapper/component-registry.test.ts(0 hunks)web/__test__/components/component-registry.test.ts(0 hunks)web/_webGui/testWebComponents.page(0 hunks)web/src/components/ConnectSettings/ConnectSettings.standalone.vue(0 hunks)web/src/components/DownloadApiLogs.standalone.vue(0 hunks)web/src/components/Wrapper/component-registry.ts(0 hunks)web/src/pages/index.vue(0 hunks)web/test-pages/pages/api-developer.njk(0 hunks)web/types/window.d.ts(0 hunks)
💤 Files with no reviewable changes (11)
- web/src/components/Wrapper/component-registry.ts
- api/src/unraid-api/rest/rest.controller.test.ts
- web/test/components/component-registry.test.ts
- web/types/window.d.ts
- web/test/components/Wrapper/component-registry.test.ts
- web/test/components/DownloadApiLogs.test.ts
- web/src/pages/index.vue
- web/src/components/DownloadApiLogs.standalone.vue
- web/src/components/ConnectSettings/ConnectSettings.standalone.vue
- web/test-pages/pages/api-developer.njk
- web/_webGui/testWebComponents.page
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
Repo: unraid/api PR: 1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/unraid-api/rest/rest.service.test.tsapi/src/unraid-api/rest/rest.service.spec.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
Repo: unraid/api PR: 942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/rest/rest.controller.ts
🪛 GitHub Actions: CI - Main (API)
api/src/unraid-api/rest/rest.controller.ts
[error] 2-2: prettier/prettier: Delete import escapeHtml from 'escape-html'; (eslint config with prettier).
🪛 GitHub Check: Test API
api/src/unraid-api/rest/rest.service.test.ts
[failure] 8-8:
Replace ·getBannerPathIfPresent,·getCasePathIfPresent· with ⏎····getBannerPathIfPresent,⏎····getCasePathIfPresent,⏎
api/src/unraid-api/rest/rest.controller.ts
[failure] 2-2:
Delete import·escapeHtml·from·'escape-html';⏎
api/src/unraid-api/rest/rest.service.ts
[failure] 5-5:
Replace ·getBannerPathIfPresent,·getCasePathIfPresent· with ⏎····getBannerPathIfPresent,⏎····getCasePathIfPresent,⏎
🔇 Additional comments (3)
api/src/unraid-api/rest/rest.controller.ts (1)
2-2: Keep theescapeHtmlimport; it is actively usedThe verification confirms that
escapeHtmlis used on line 97 to safely escape user-controlled values in the invalidredirect_urierror message. The import is necessary and should not be removed.api/src/unraid-api/rest/rest.module.ts (1)
8-12: RestModule correctly isolates CliServicesModule removal; no dangling referencesVerification confirms the removal of
CliServicesModulefromRestModuleimports is complete and isolated. The remaining references toCliServicesModuleare only within the CLI module itself (cli-services.module.ts) and its test file (cli.module.spec.ts), which is expected and correct. The change poses no risk to other modules.api/src/unraid-api/rest/rest.service.test.ts (1)
2-83: The review comment does not match the actual code in this file.The review comment references tests for
getCustomizationPath()andgetCustomizationStream()with imports forgetBannerPathIfPresent,getCasePathIfPresent,ReadStream, andReadable. However, the actual file atapi/src/unraid-api/rest/rest.service.test.tscontains tests forsaveApiReport()with entirely different mocking and imports (includingApiReportService).The provided code snippet appears to be from a different version, branch, or file than what currently exists in the repository. The suggested Prettier formatting fix is not applicable to the current code.
Verify that the review is being applied to the correct file and branch, or retrieve the correct review comment for the current test file.
Likely an incorrect or invalid review comment.
elibosley
changed the title
- Updated `rest.service.spec.ts` to include new tests for `getCustomizationPath` and `getCustomizationStream` methods, ensuring proper handling of banner and case paths. - Removed obsolete test files: `rest.service.test.ts`, `rest-module-dependencies.test.ts`, and `rest-module.integration.test.ts` to streamline the test suite. - Added necessary mocks for file system operations and image file helper functions to enhance test reliability.
- Integrated `ConfigModule` into the OIDC base module to enhance configuration management capabilities. - This addition supports better handling of environment variables and application settings.
- Added `ConfigModule` to the imports in the module dependencies integration test to enhance configuration management during testing. - This change supports better handling of environment variables and application settings in the test environment.
elibosley
force-pushed
the
codex/remove-download-unraid-api-logs-helper
branch
from
54dc3b6 to
f49fdf0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1793 +/- ##
==========================================
- Coverage 52.28% 52.15% -0.14%
==========================================
Files 874 873 -1
Lines 50576 50413 -163
Branches 5063 5032 -31
==========================================
- Hits 26445 26294 -151
+ Misses 24056 24044 -12
Partials 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
🤖 I have created a release *beep* *boop* --- ## [4.27.0](v4.26.2...v4.27.0) (2025-11-19) ### Features * remove Unraid API log download functionality ([#1793](#1793)) ([e4a9b82](e4a9b82)) ### Bug Fixes * auto-uninstallation of connect api plugin ([#1791](#1791)) ([e734043](e734043)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 participant
Summary
Testing
Codex Task
Summary by CodeRabbit
Removed Features
Chores