feat: harden operations and automate test releases - #233
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (19)
.github/workflows/release.yml (1)
201-222: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd
pipefailand clean the temporary directory.The step uses
set -uonly. Thesha256sum --checkresult is evaluated by theif, so the missing-eis intentional. Adding-o pipefailstill protects future pipelines in this step. Removingverify_diravoids leaving downloaded assets on the runner.♻️ Proposed cleanup
- set -u + set -uo pipefail verify_dir="$(mktemp -d)" + trap 'rm -rf "$verify_dir"' EXIT🤖 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 @.github/workflows/release.yml around lines 201 - 222, Update the “Verify existing release assets” step to enable Bash pipefail alongside nounset without adding errexit, preserving the current conditional verification flow. Add cleanup for the temporary verify_dir so downloaded assets are removed after the verification attempt, including when the step completes through either branch.scripts/release-version.test.mjs (2)
162-177: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winIsolate the fixture repository from global Git configuration.
The test runs
git commitwith the developer's global configuration. If a contributor setscommit.gpgsign=true,init.templateDir, or a global hooks path, the commit fails or waits for a passphrase. SettingGIT_CONFIG_GLOBALandGIT_CONFIG_SYSTEMto a path that does not exist makes the fixture deterministic.♻️ Proposed isolation
-function git(cwd, ...args) { +const isolatedGitEnv = { + ...process.env, + GIT_CONFIG_GLOBAL: '/dev/null', + GIT_CONFIG_SYSTEM: '/dev/null', +}; + +function git(cwd, ...args) { // Git is the fixture engine for this isolated temporary repository. // eslint-disable-next-line sonarjs/no-os-command-from-path - return execFileSync('git', args, { cwd, encoding: 'utf8' }).trim(); + return execFileSync('git', args, { cwd, encoding: 'utf8', env: isolatedGitEnv }).trim(); }Add
-c,commit.gpgsign=falseto the commit invocation if you prefer to keep the ambient environment.🤖 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 `@scripts/release-version.test.mjs` around lines 162 - 177, Isolate the temporary Git repository from system and global configuration in the test around the git commit setup. Set GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to nonexistent paths in the spawned Git environment, or disable signing directly on the commit invocation with commit.gpgsign=false, so global templates, hooks, signing, and passphrase prompts cannot affect the fixture.
179-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd narrow tests for the rejection paths.
The suite covers the success path of
writeGitHubOutputsonly through the integration test.assertOutputResultand theheadShaguard inplanReleaseprotect the release outputs, and neither guard has a direct test. Add small unit tests that assertplanReleasethrows for a short or uppercaseheadSha, and thatwriteGitHubOutputsthrows for a malformedactionand for an emptyoutputPath.This follows the guideline "Start with the narrowest tests that cover the changed 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 `@scripts/release-version.test.mjs` around lines 179 - 202, Add focused unit tests for the rejection paths in planRelease and writeGitHubOutputs: verify planRelease throws for short and uppercase headSha values, and verify writeGitHubOutputs throws when action is malformed or outputPath is empty. Keep these tests narrow and separate from the existing integration success-path test.Source: Coding guidelines
src/renderer/src/stores/collectionStore.ts (1)
153-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
scopeExcludeddefault out of the generic predicate.
buildFilterPredicateis a generic filter utility, but it hard-codes one Dynatrace field name. Any other collection that adds a boolean field with legacy null rows needs another special case here. Consider passing the defaulted fields throughCollectionQueryOptionsso the domain knowledge stays with the caller.🤖 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/renderer/src/stores/collectionStore.ts` around lines 153 - 161, Update buildFilterPredicate to remove the hard-coded scopeExcluded fallback and keep its predicate generic. Extend CollectionQueryOptions to carry fields requiring an undefined-to-false default, then have the relevant caller configure scopeExcluded there and apply that configuration when evaluating field values.src/main/handlers/cacheHandlers.test.ts (1)
241-256: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the rejection cases so failures identify the broken branch.
The test asserts three distinct rejection paths with two shared assertions. If only one path regresses, the failure does not show which input was accepted. Use
it.eachwith one case per row.🤖 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/handlers/cacheHandlers.test.ts` around lines 241 - 256, Split the combined test around rejects invalid query identities and memberships into an it.each table with one rejection input per row, covering the invalid read identity, invalid snapshot identity, and invalid record ID cases. Keep the shared expectations that readQueryMembership and writeQueryMembership are not called within each parameterized case so failures identify the specific rejected input.src/renderer/src/stores/collectionStoreRegistry.ts (1)
93-103: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the
storeRefplaceholder against a callback during construction.
entry.storeRefisundefineduntil line 122. The subscriber-count callback dereferences it at line 103 withentry.storeRef.deref(). The currentCollectionStoreconstructor does not call the callback, so no crash occurs today. If a later change makes the constructor report an initial subscriber count, this throws aTypeError. Optional chaining removes the hazard.🛡️ Proposed fix
- const retainedStore = entry.strongStore ?? entry.storeRef.deref(); + const retainedStore = entry.strongStore ?? entry.storeRef?.deref();Apply the same guard at lines 113, 131, 142, and inside the finalization callback at line 33.
🤖 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/renderer/src/stores/collectionStoreRegistry.ts` around lines 93 - 103, Guard every dereference of the placeholder storeRef with optional chaining, including the subscriber-count callback during CollectionStore construction, the references around lines 113, 131, and 142, and the finalization callback. Preserve the existing retained-store behavior while preventing callbacks from throwing before storeRef is initialized.src/renderer/src/tabs/dynatrace-problems.css (1)
321-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe two new button blocks duplicate the same declarations.
.dt-problems__history-pagination buttonand.dt-problem-note__ticket-actions buttonshareborder,border-radius,background,color,cursor,font, andfont-weight, plus an identical hover rule. Onlymin-height,padding, andfont-sizediffer.Extract a shared secondary-button class, then override the three differing properties per context.
Also applies to: 1059-1075
🤖 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/renderer/src/tabs/dynatrace-problems.css` around lines 321 - 337, Extract the shared button declarations and hover styling from .dt-problems__history-pagination button and .dt-problem-note__ticket-actions button into a reusable secondary-button class. Update both contexts to use that class, retaining context-specific overrides only for min-height, padding, and font-size.src/main/dynatrace/DynatraceProblemsClient.test.ts (2)
313-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
Retry-Afterheader path.This test only covers
error.retryAfterSecondsin the response body.retryAfterHeaderMillisecondsis checked first inretryAfterMillisecondsand handles both a numeric seconds value and an HTTP-date value. Neither branch is exercised.💚 Suggested added case
+ it('prefers the Retry-After header over the response body', async () => { + const fetchMock = vi.fn<typeof fetch>().mockResolvedValue( + new Response(JSON.stringify({ error: { retryAfterSeconds: 5 } }), { + status: 429, + headers: { 'Content-Type': 'application/json', 'Retry-After': '30' }, + }), + ); + const client = new DynatraceProblemsClient(fetchMock); + + const error = await client.testConnection(config).catch((caught) => caught); + + expect(getDynatraceRetryAfterMs(error)).toBe(30_000); + });🤖 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/dynatrace/DynatraceProblemsClient.test.ts` around lines 313 - 334, Extend the rate-limit coverage around the test named “preserves Dynatrace retry guidance on rate-limit errors” to exercise Retry-After response headers, including both numeric-seconds and HTTP-date values. Assert the resulting retry delay is preserved through getDynatraceRetryAfterMs, while keeping the existing response-body retryAfterSeconds coverage intact.
299-311: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe 10,000-record fixture makes this test expensive.
The test builds 10,000 problem objects, serializes them with
JSON.stringify, parses the JSON back, and runs the full ZodproblemSchemaarray parse. That is significant work for a single boolean assertion.Export
MAX_PROBLEMSfromDynatraceProblemsClient.tsand assert against a smaller derived limit, or make the maximum injectable so the test can use a small value.🤖 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/dynatrace/DynatraceProblemsClient.test.ts` around lines 299 - 311, Export the existing maximum-problems limit as MAX_PROBLEMS from DynatraceProblemsClient, then update the exact-limit test to derive its fixture size from that constant or use an injectable smaller limit. Preserve the assertion that a result exactly at the configured limit sets resultTruncated to true without a notification, while avoiding construction and schema parsing of 10,000 records.src/renderer/src/hooks/__tests__/useDynatraceProblems.test.ts (1)
85-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test only covers the empty-problem path.
The
useCollectionmock returnsdata: []forDYNATRACE_PROBLEMS_COLLECTION.loadedProblemIdsis therefore always empty, and the assertions pinvalues: []withenabled: false. The batching behavior that the change introduces, meaning non-emptyvaluesandenabled: true, is never exercised.The mock also returns
[state]and[historicalNote]for the related collections while those queries are disabled. A disabled query returns no data at runtime, so the fixture does not match real behavior.Add a case where the mock returns open and history problems, then assert that
batchedFilter.valuescontains the deduplicated problem IDs and thatenabledistrue.🤖 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/renderer/src/hooks/__tests__/useDynatraceProblems.test.ts` around lines 85 - 140, The test case around useDynatraceProblems currently covers only empty problem results and disabled related queries. Add a non-empty fixture for open and historical problems in the useCollection mock, ensure related collections reflect disabled-query runtime behavior, and assert that both related batchedFilter.values contain deduplicated problem IDs with enabled set to true.src/renderer/src/tabs/__tests__/DynatraceProblemsTab.test.tsx (1)
236-255: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm that
Load 100 morematchesHISTORY_PAGE_SIZE.The test hardcodes the button label
Load 100 more.HISTORY_PAGE_SIZEinsrc/renderer/src/hooks/useDynatraceProblems.tsis also100. If the constant changes, the component label changes and this test fails for a reason unrelated to the behavior under test.Import the page-size constant or match with a regular expression such as
/Load \d+ more/.Also applies to: 257-274
🤖 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/renderer/src/tabs/__tests__/DynatraceProblemsTab.test.tsx` around lines 236 - 255, Update the “loads more resolved history” test around the Load 100 more interaction to avoid hardcoding the current page size: import and use HISTORY_PAGE_SIZE when constructing the button matcher, or use a numeric matcher such as /Load \d+ more/. Apply the same change to the additional assertion range noted in the comment, while preserving the existing loadMoreHistory behavior check.src/main/dynatrace/DynatraceProblemsClient.ts (1)
369-375: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
numericCountaccepts an empty string as0.
Number('')returns0. An emptyproblemCountfield therefore reports a valid count of zero instead of a parse failure. IninspectAlertingProfileField,problemCount === 0forceshealthy: true, so a malformed response is reported as healthy scope metadata.♻️ Proposed guard
function numericCount(record: Record<string, unknown> | undefined, field: string): number | null { const value = record?.[field]; - const count = typeof value === 'string' ? Number(value) : value; + const count = typeof value === 'string' ? (value.trim() === '' ? NaN : Number(value)) : value; return typeof count === 'number' && Number.isFinite(count) && count >= 0 ? Math.floor(count) : null; }🤖 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/dynatrace/DynatraceProblemsClient.ts` around lines 369 - 375, Update numericCount to reject empty or whitespace-only string values before converting them with Number, returning null for those inputs. Preserve numeric handling and valid non-empty numeric strings so inspectAlertingProfileField does not treat malformed problemCount data as zero.src/renderer/src/components/settings/AdministrationSettings.test.tsx (1)
270-317: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the test to cover the post-apply behavior.
replaceProfilesinsrc/renderer/src/components/settings/administration/RelayServerPanel.tsxcloses the dialog and sets the feedback message whenresult.okis true. This test stops at theexecuteassertion, so a regression that leaves the dialog open would still pass.💚 Proposed additional assertions
fireEvent.click(within(dialog).getByRole('button', { name: 'Apply stored scope' })); await waitFor(() => expect(execute).toHaveBeenCalledWith({ command: 'administration.setting.replace', payload: { setting: 'dynatrace.alerting-profiles', value: { profiles: ['NOC Core', 'Retail Stores'] }, expectedRevision: 4, }, expectedRevision: null, }), ); + await waitFor(() => + expect( + screen.queryByRole('dialog', { name: 'Review stored problem scope' }), + ).not.toBeInTheDocument(), + ); + expect(screen.getByRole('status')).toHaveTextContent('Stored Dynatrace problem scope updated.'); });🤖 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/renderer/src/components/settings/AdministrationSettings.test.tsx` around lines 270 - 317, The test should also verify the successful post-apply behavior of replaceProfiles: after the Apply stored scope action resolves successfully, assert that the review dialog closes and the success feedback message is displayed. Keep the existing execute payload assertion unchanged.src/main/handlers/windowHandlers.test.ts (1)
483-492: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the rejection cases to the remaining
normalizeServiceDeskUrlguards.The table covers protocol, credentials, port, leading whitespace, and a newline. Two guards in
normalizeServiceDeskUrlstay untested: theMAX_EXTERNAL_URL_LENGTHbound and the non-string type check. The type check matters because IPC arguments are unvalidated at runtime.Per coding guidelines: "Start with the narrowest tests that cover the changed behavior."
💚 Proposed additional cases
it.each([ 'http://servicedesk.example.com/INC0012345', 'https://user:secret@servicedesk.example.com/INC0012345', 'https://servicedesk.example.com:8443/INC0012345', ' https://servicedesk.example.com/INC0012345', 'https://servicedesk.example.com/INC0012345\nignored', + `https://servicedesk.example.com/${'a'.repeat(2_100)}`, ])('blocks unsafe Service Desk URL %s', async (url) => { await expect(getHandler(IPC_CHANNELS.OPEN_SERVICE_DESK_URL)({}, url)).resolves.toBe(false); expect(shell.openExternal).not.toHaveBeenCalled(); }); + + it.each([undefined, null, 42, { href: 'https://servicedesk.example.com' }])( + 'blocks non-string Service Desk input %p', + async (url) => { + await expect(getHandler(IPC_CHANNELS.OPEN_SERVICE_DESK_URL)({}, url)).resolves.toBe(false); + expect(shell.openExternal).not.toHaveBeenCalled(); + }, + );🤖 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/handlers/windowHandlers.test.ts` around lines 483 - 492, Extend the unsafe Service Desk URL table in the test for getHandler(IPC_CHANNELS.OPEN_SERVICE_DESK_URL) with cases covering normalizeServiceDeskUrl’s MAX_EXTERNAL_URL_LENGTH limit and non-string inputs, asserting each resolves false and shell.openExternal is not called.Source: Coding guidelines
src/shared/urlSecurity.ts (1)
2-3: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider extending the rejected character class to format characters.
\p{Cc}matches C0 and C1 control characters only. It does not match\p{Cf}format characters such as U+200E, U+200F, and the bidi isolates U+2066-U+2069. An operator-supplied URL that carries bidi controls renders with a misleading direction in the UI label while pointing at a different host. The leading and trailing cases are already covered, becauseString.prototype.trimremoves U+2028 and U+2029 and the check at line 122 rejects padded input.🛡️ Proposed hardening
-const CONTROL_CHARACTER_PATTERN = /\p{Cc}/u; +const CONTROL_CHARACTER_PATTERN = /[\p{Cc}\p{Cf}]/u;Confirm this does not reject legitimate ticket URLs in your environment before you apply it.
🤖 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/shared/urlSecurity.ts` around lines 2 - 3, Extend CONTROL_CHARACTER_PATTERN to reject Unicode format characters alongside C0/C1 controls, covering bidi and directional formatting characters in URL validation. Verify legitimate ticket URLs remain accepted in the target environment, and keep the existing trimming and padded-input checks unchanged.src/main/dynatrace/DynatraceProblemsManager.ts (2)
561-592: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReconciliation updates run one record at a time over the full collection.
reconcileProblemScopeloads every problem record, then awaits oneupdatecall per changed record in sequence. History is retained for one year, so this list can be large. After an operator changes the alerting-profile scope, the first reconciliation can mark a large fraction of records, which makes the loop long and blocks the sync from completing.
upsertProblemsanddeleteProblemsWithRelatedRecordsalready use the shared-iterator worker pattern withUPSERT_CONCURRENCY. Apply the same pattern here.♻️ Proposed refactor to bound reconciliation latency
- const excludedAt = new Date().toISOString(); - let excludedCount = 0; - for (const problem of problems) { - const shouldExclude = Boolean( - selectedProfiles && - !problem.alertingProfiles?.some((profile) => selectedProfiles.has(profile)), - ); - if (shouldExclude) excludedCount += 1; - const missingExcludedAt = shouldExclude && parsedTimestamp(problem.scopeExcludedAt) === null; - const staleIncludedAt = !shouldExclude && Boolean(problem.scopeExcludedAt); - if (problem.scopeExcluded === shouldExclude && !missingExcludedAt && !staleIncludedAt) - continue; - await pb.collection(DYNATRACE_PROBLEMS_COLLECTION).update( - problem.id, - { - scopeExcluded: shouldExclude, - scopeExcludedAt: shouldExclude ? excludedAt : '', - }, - { requestKey: null }, - ); - } - return excludedCount; + const excludedAt = new Date().toISOString(); + let excludedCount = 0; + const pending: Array<{ id: string; shouldExclude: boolean }> = []; + for (const problem of problems) { + const shouldExclude = Boolean( + selectedProfiles && + !problem.alertingProfiles?.some((profile) => selectedProfiles.has(profile)), + ); + if (shouldExclude) excludedCount += 1; + const missingExcludedAt = shouldExclude && parsedTimestamp(problem.scopeExcludedAt) === null; + const staleIncludedAt = !shouldExclude && Boolean(problem.scopeExcludedAt); + if (problem.scopeExcluded === shouldExclude && !missingExcludedAt && !staleIncludedAt) + continue; + pending.push({ id: problem.id, shouldExclude }); + } + // See upsertProblems: one shared iterator, N workers, no index bookkeeping. + const queue = pending[Symbol.iterator](); + const worker = async () => { + for (const { id, shouldExclude } of queue) { + await pb.collection(DYNATRACE_PROBLEMS_COLLECTION).update( + id, + { + scopeExcluded: shouldExclude, + scopeExcludedAt: shouldExclude ? excludedAt : '', + }, + { requestKey: null }, + ); + } + }; + await Promise.all( + Array.from({ length: Math.min(UPSERT_CONCURRENCY, pending.length) }, () => worker()), + ); + return excludedCount;🤖 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/dynatrace/DynatraceProblemsManager.ts` around lines 561 - 592, Refactor reconcileProblemScope to update changed problem records through the shared bounded-concurrency iterator pattern used by upsertProblems and deleteProblemsWithRelatedRecords, using UPSERT_CONCURRENCY. Preserve the existing shouldExclude calculation, excludedCount result, update payload, and request key behavior while allowing updates to run concurrently with the configured limit.
369-387: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRetry delay does not grow with consecutive failures.
The failure path stores
consecutiveFailures, but the retry delay stays atmax(POLL_INTERVAL_MS, retryAfter)for every attempt. If Dynatrace stays unavailable and sends noRetry-Aftervalue, Relay keeps polling at the base interval indefinitely. Derive the delay from the new failure count so repeated failures back off, and keep the Dynatrace-provided value as a lower bound.♻️ Proposed refactor to add bounded backoff
const message = getErrorMessage(error); - const retryDelay = Math.max(POLL_INTERVAL_MS, getDynatraceRetryAfterMs(error) ?? 0); + const failures = (previousSync?.consecutiveFailures ?? 0) + 1; + const backoff = Math.min(POLL_INTERVAL_MS * 2 ** (failures - 1), MAX_RETRY_DELAY_MS); + const retryDelay = Math.max(backoff, getDynatraceRetryAfterMs(error) ?? 0); this.scheduledRetryAt = Date.now() + retryDelay;Add the bound next to the other interval constants:
const MAX_RETRY_DELAY_MS = 30 * 60_000;Note: the existing test at
src/main/dynatrace/DynatraceProblemsManager.test.tslines 361-452 advances timers by 61 seconds between attempts. Backoff would require that test to advance further.🤖 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/dynatrace/DynatraceProblemsManager.ts` around lines 369 - 387, Update the catch path in DynatraceProblemsManager to calculate consecutiveFailures before deriving retryDelay, then apply bounded exponential backoff from POLL_INTERVAL_MS while retaining getDynatraceRetryAfterMs(error) as a lower bound and capping the result with MAX_RETRY_DELAY_MS. Reuse that failure count in writeSyncState, add the bound alongside the interval constants, and adjust affected retry-timing tests to advance timers according to the backoff.src/renderer/src/components/settings/administration/RelayServerPanel.tsx (1)
28-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider de-duplicating the parsed profile names.
selectedProfileNameskeeps every non-empty line. If an operator repeats a profile name, the stored value contains a duplicate, andaddedProfileNamescan show that name twice in the review dialog. The manager builds aSetfrom the selection, so duplicates only affect the persisted value and the dialog copy.♻️ Proposed refactor
const selectedProfileNames = useMemo( () => - profileText - .split(/\r?\n/) - .map((value) => value.trim()) - .filter(Boolean), + [ + ...new Set( + profileText + .split(/\r?\n/) + .map((value) => value.trim()) + .filter(Boolean), + ), + ], [profileText], );🤖 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/renderer/src/components/settings/administration/RelayServerPanel.tsx` around lines 28 - 45, Update selectedProfileNames in the RelayServerPanel profile parsing flow to remove duplicate trimmed names before calculating addedProfileNames and persisting the selection. Preserve the existing trimming and empty-line filtering behavior, using a uniqueness-preserving collection or equivalent.src/renderer/src/tabs/DynatraceProblemsTab.tsx (1)
1467-1491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
canSyncDynatraceis computed twice from the same expression.Line 1471 computes the predicate inside
handleRefresh, and line 1490 computes the identical expression forrefreshControlCopy. The two copies can diverge, which would make the refresh button label describe an action the handler does not take. Hoist oneconstabovehandleRefreshand use it in both places.♻️ Proposed refactor
+ const canSyncDynatrace = relayMode === 'server' && globalThis.api?.runtime?.kind !== 'web'; + const handleRefresh = async () => { if (savingAction) return; setSavingAction('refresh'); try { - const canSyncDynatrace = relayMode === 'server' && globalThis.api?.runtime?.kind !== 'web'; if (canSyncDynatrace && sync?.state !== 'disabled') {const lastSyncLabel = getLastSyncLabel(sync); - const canSyncDynatrace = relayMode === 'server' && globalThis.api?.runtime?.kind !== 'web'; const refreshControl = refreshControlCopy(canSyncDynatrace, savingAction === 'refresh');🤖 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/renderer/src/tabs/DynatraceProblemsTab.tsx` around lines 1467 - 1491, Hoist the shared canSyncDynatrace predicate above handleRefresh, then reuse that constant both inside the refresh handler and when calling refreshControlCopy. Remove the duplicate local declaration while preserving the existing behavior.
🔇 Additional comments (23)
src/main/handlers/cloudStatus/dynatraceStatusProvider.ts (1)
160-163: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Enforce the payload limit while reading the response.
response.text()buffers the complete body before Line 161 checks its size. A response without a validContent-Lengthcan therefore allocate far more thanMAX_RESPONSE_BYTES.Read
response.bodythrough a reader. Count each chunk. Cancel and throw when the cumulative size exceeds the limit. Add a chunked-response test that verifies the reader stops at the limit.Proposed fix
+async function readResponseTextWithinLimit(response: Response): Promise<string> { + const reader = response.body?.getReader(); + if (!reader) return ''; + + const decoder = new TextDecoder(); + let bytes = 0; + let text = ''; + try { + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + bytes += value.byteLength; + if (bytes > MAX_RESPONSE_BYTES) { + await reader.cancel().catch(() => undefined); + throw new Error(`Dynatrace Status.io response exceeds ${MAX_RESPONSE_BYTES} bytes`); + } + text += decoder.decode(value, { stream: true }); + } + } finally { + reader.releaseLock(); + } + return text + decoder.decode(); +} + - const responseBody = await response.text(); - if (Buffer.byteLength(responseBody, 'utf8') > MAX_RESPONSE_BYTES) { - throw new Error(`Dynatrace Status.io response exceeds ${MAX_RESPONSE_BYTES} bytes`); - } + const responseBody = await readResponseTextWithinLimit(response);As per coding guidelines, start with the narrowest tests that cover the changed behavior.
src/renderer/src/utils/__tests__/cloudStatusDisplay.test.ts (1)
35-118: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Run focused validation before a readiness claim.
Run the changed cloud-status renderer tests first.
As per coding guidelines, “Start with the narrowest tests that cover the changed behavior” and run
npm audit --audit-level=high --omit=devbefore a readiness or push-to-test claim.src/renderer/src/utils/cloudStatus.ts (1)
5-5: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Remove the duplicate
CloudIssueCandidatedeclarations.Line 5 declares
CloudIssueCandidatethree times. TypeScript reports a duplicate identifier and stops the renderer type check. Keep one declaration.Proposed fix
type CloudIssueCandidate = { severity: CloudStatusSeverity; pubDate: string }; -type CloudIssueCandidate = { severity: CloudStatusSeverity; pubDate: string }; -type CloudIssueCandidate = { severity: CloudStatusSeverity; pubDate: string };.github/workflows/release.yml (1)
35-84: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the required checks come from the
github-actionsapp.Line 53 keeps only check runs whose
app.slugequalsgithub-actions. IfSonarQube quality gateorSnyk security gateis reported by the SonarQube or Snyk GitHub App instead of an Actions job, the filter removes it. The loop then waits the full 25 minutes and the release fails on everytestpush.Also confirm the three names match the workflow job
namevalues exactly, because the map is keyed on the check-run name.package.json (1)
16-16: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Check for stale references to the removed
releasescript.The
releasescript no longer exists. Any workflow, script, or document that callsnpm run releasenow fails.scripts/release-version.test.mjs (1)
15-15: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the pinned Node version supports
import.meta.dirname.
import.meta.dirnamewas added in Node 20.11 and 21.2. If.node-versionpins an earlier release, this value isundefinedandpath.resolvethrows.src/renderer/src/hooks/__tests__/useCollection.test.ts (1)
359-374: 📐 Maintainability & Code Quality | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that
globalThis.apiis reset between tests.These tests assign
globalThis.apidirectly. If noafterEachdeletes it, later tests inherit cache mocks from earlier tests and can pass for the wrong reason. Verify that a cleanup hook exists in this file or the shared setup.src/main/cache/OfflineCache.test.ts (1)
277-296: 📐 Maintainability & Code Quality | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that
Date.nowis restored if an assertion fails.The test restores the spy with
now.mockRestore()inside the test body. If a later write throws,Date.nowstays mocked at64for the rest of the suite. Confirm that the suite config setsrestoreMocks: true, or that the existingafterEachcallsvi.restoreAllMocks().♻️ Alternative that always restores
- const now = vi.spyOn(Date, 'now'); - for (let index = 0; index < 65; index += 1) { - now.mockReturnValue(index); + vi.useFakeTimers(); + for (let index = 0; index < 65; index += 1) { + vi.setSystemTime(index); cache.writeQueryMembership('dynatrace_problems', index.toString(16).padStart(16, '0'), { recordIds: [`problem-${index}`], totalItems: 1, complete: true, }); } - now.mockRestore(); + vi.useRealTimers();src/renderer/src/runtime/WebBridge.test.ts (1)
368-385: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Consider adding a
javascript:scheme case.The test covers HTTP and userinfo rejection. A scheme-injection case is the highest-risk input for an external-open path. If
src/shared/urlSecurity.tstests already coverjavascript:fornormalizeServiceDeskUrl, no change is needed here.src/main/pocketbase/schema/collectionCatalog.ts (1)
1005-1010: 🗄️ Data Integrity & Integration | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that existing
dynatrace_problemsrows resolvescopeExcludedtofalse.
src/renderer/src/hooks/useDynatraceProblems.tsfilters both queries withscopeExcluded=false.scopeExcludedis a newboolfield on an existing collection. If PocketBase leaves the column NULL for rows that already exist, the filter excludes them and every previously synced problem disappears from the Open and History views until the next sync rewrites the record.Confirm that the collection bootstrap applies the field with a
falsevalue for existing rows, or that the Dynatrace manager backfills it.src/main/dynatrace/DynatraceProblemsClient.ts (1)
349-356: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.
parseQueryResultthrows on in-progress poll payloads.
parseQueryResulttolerates onlynullandundefined. Any other value must satisfyqueryResultSchema, which requires arecordsarray.runQuerycallsparseQueryResult(response.result)on every loop iteration, includingRUNNINGresponses. If Dynatrace returns a non-null progress object withoutrecordswhile the query is still executing, the client throwsDynatrace returned an unexpected Grail query result.and abandons a query that would have succeeded.Parse the result only when the state is
SUCCEEDED, or treat a parse failure on a non-terminal state as "not ready yet".🐛 Proposed fix
-function parseQueryResult(result: unknown): QueryResult | null { - if (result == null) return null; - const parsed = queryResultSchema.safeParse(result); - if (!parsed.success) { - throw new Error('Dynatrace returned an unexpected Grail query result.'); - } - return parsed.data; -} +function parseQueryResult(result: unknown, terminal: boolean): QueryResult | null { + if (result == null) return null; + const parsed = queryResultSchema.safeParse(result); + if (!parsed.success) { + if (!terminal) return null; + throw new Error('Dynatrace returned an unexpected Grail query result.'); + } + return parsed.data; +}Then update the call site:
- const result = parseQueryResult(response.result); - if (response.state === 'SUCCEEDED' && result) return result; + const terminal = response.state === 'SUCCEEDED'; + const result = parseQueryResult(response.result, terminal); + if (terminal && result) return result;src/renderer/src/hooks/useDynatraceProblems.ts (4)
39-58: 🗄️ Data Integrity & Integration | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Two different collections share one
batchedFilter.key.
statesqueriesDYNATRACE_PROBLEM_STATES_COLLECTIONandnotesqueriesDYNATRACE_PROBLEM_NOTES_COLLECTION, but both passkey: 'dynatrace-loaded-problems'. IfcollectionStoreRegistryscopes batch state bykeyalone, the two queries share one batch entry and one result set. If it scopes by collection plus key, the shared key is safe.Confirm the scoping rule. If the key is global, give each query a distinct key.
35-58: 🚀 Performance & Scalability | 🏗️ Heavy lift
⚠️ Unverified finding
Sandbox verification was unavailable.History pagination re-batches every related query.
loadedProblemIdsis derived from the combined open and history list. EachloadMoreHistorycall adds up toHISTORY_PAGE_SIZE(100) IDs. That changesbatchedFilter.valuesfor bothstatesandnotes, withRELATED_PROBLEM_BATCH_SIZEof 40 per batch.If the batching layer refetches all batches when
valueschanges, page n costsceil(100n / 40)requests per related collection. Loading five history pages then issues 13 state requests and 13 note requests for that page alone, and re-reads all previously fetched IDs.Confirm that the batching layer fetches only the new IDs. If it does not, pass only the newly added IDs, or scope related lookups to the visible page.
105-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.
refetchchanges identity on every render.The dependency array lists the collection result objects, not their
refetchfunctions.useCollectionreturns a new object on each render, sorefetchis a new function on each render. Any consumer that putsrefetchin auseEffectdependency array runs an effect loop.Depend on the
refetchfunctions instead.♻️ Proposed fix
const refetch = useCallback(async () => { await Promise.all([ openProblems.refetch(), historyProblems.refetch(), states.refetch(), notes.refetch(), sync.refetch(), ]); - }, [historyProblems, notes, openProblems, states, sync]); + }, [ + historyProblems.refetch, + notes.refetch, + openProblems.refetch, + states.refetch, + sync.refetch, + ]);This helps only if
useCollectionreturns a stablerefetch. Confirm that it wrapsrefetchinuseCallback.
115-131: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.
loadingstays true if a disabled collection reports loading.
statesandnotesare created withenabled: loadedProblemIds.length > 0. On first render there are no problems, so both are disabled.loadingORsstates.loadingandnotes.loading. IfuseCollectionreportsloading: truefor a disabled query, the tab shows a loading state that never clears when the problem list is legitimately empty.The test mock in
src/renderer/src/hooks/__tests__/useDynatraceProblems.test.tsalways returnsloading: false, so this path is not covered.Confirm that a disabled
useCollectionreportsloading: false.src/renderer/src/tabs/__tests__/DynatraceProblemsTab.test.tsx (2)
174-183: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Remove the duplicated assignment and restore
runtimeafter the test.Two issues at this site:
- The assignment to
globalThis.api.runtimeappears twice in a row. The second statement is redundant.- The test sets
runtimeto{ kind: 'web' }and never restores it. Every test that runs after this one sees the web runtime. That changes which toolbar action the tab renders and whethersyncDynatraceProblemsis reachable.🐛 Proposed fix
it('reloads Relay data without requesting a privileged sync in Relay Web', async () => { if (!globalThis.api) throw new Error('Expected bridge fixture'); - (globalThis.api as unknown as { runtime: { kind: 'web' } }).runtime = { kind: 'web' }; - (globalThis.api as unknown as { runtime: { kind: 'web' } }).runtime = { kind: 'web' }; + const bridge = globalThis.api as unknown as { runtime: { kind: string } }; + const previousRuntime = bridge.runtime; + bridge.runtime = { kind: 'web' }; + onTestFinished(() => { + bridge.runtime = previousRuntime; + }); render(<DynatraceProblemsTab relayMode="server" />);
onTestFinishedis imported fromvitest. If abeforeEachalready rebuildsglobalThis.api, only the duplicated line needs removal.
205-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Each test spreads the mutated
mocks.hookValue, so fixture state accumulates.
mocks.hookValueis a module-level object. Every test here reassigns it with{ ...mocks.hookValue, ... }. Unless abeforeEachrestores the base fixture, state leaks forward:
- The truncation test at Line 221 inherits
state: 'syncing'from the test at Line 205.- The cached-history test at Line 257 inherits
hasMoreHistory: trueandtotalHistoryCount: 250from the test at Line 236.The assertions still pass because each test checks only its own field. The tests are therefore not independent, and a future assertion added to one of them can fail for an unrelated reason.
Build each fixture from a fresh base constant rather than from the current
mocks.hookValue.src/renderer/src/tabs/dynatrace-problems.css (2)
178-182: 📐 Maintainability & Code Quality | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the contrast ratio of the warning notice.
The rule sets
color: var(--color-warning)onbackground: var(--color-warning-subtle). A warning hue placed on its own tinted background often falls below the WCAG 2.1 AA requirement of 4.5:1 for body text. The test at Line 232 ofsrc/renderer/src/tabs/__tests__/DynatraceProblemsTab.test.tsxasserts this notice usesrole="alert", so the text must be readable.
649-649: 📐 Maintainability & Code Quality | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the fact-item count matches the three-column grid.
The grid changes from four columns to three. If the detail panel still renders four fact items, the fourth item wraps onto a second row and occupies one third of the width.
src/main/dynatrace/DynatraceProblemsManager.test.ts (1)
583-606: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the retry-guidance fixture matches the real client error shape.
The fixture passes
{ cause: 90_000 }, and the assertion expectsnextRetryAt90 seconds after the fake clock. This encodes the assumption thatgetDynatraceRetryAfterMsreads a plain number fromerror.cause. IfDynatraceProblemsClientattachesRetry-Afterin a different shape, this test passes while production retry guidance is discarded.src/renderer/src/components/settings/administration/RelayServerPanel.tsx (1)
217-223: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.The panel provides no way to clear the stored problem scope.
disabled={!profiles || selectedProfileNames.length === 0}blocks submission when the textarea is empty.DynatraceProblemsManager.prepareProfileScopetreats a null selection as unscoped and skips both the profile-health check andreconcileProblemScopeexclusion. After an operator sets any profile list, this control offers no path back to the unscoped state.Confirm the intended behavior. If operators must be able to remove the scope, allow an empty submission and map it to the unscoped value.
src/main/handlers/window/externalLinkHandlers.ts (1)
148-152: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm
describeUrlForLogtolerates non-string input.The
url: stringannotation is a compile-time claim only. IPC arguments arrive from the renderer at runtime, so a compromised or buggy caller can send a number, object, orundefined.normalizeServiceDeskUrlhandles that case and returnsnull. The rejection path then callsdescribeUrlForLog(url)with the same unchecked value. IfdescribeUrlForLogcalls a string method ornew URLwithout a guard, this handler rejects its promise instead of returningfalse.src/renderer/src/tabs/DynatraceProblemsTab.tsx (1)
1351-1359: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Optional chaining guards
apibut notopenServiceDeskUrl.
globalThis.api?.openServiceDeskUrl(url)only protects against a missingapiobject. In a runtime whereapiexists but does not exposeopenServiceDeskUrl, this line throwsTypeError: ... is not a function. The throw happens inside the async callback, so theshowToasterror path never runs and the operator sees no feedback.
handleOpenDynatraceat lines 1332-1340 has the same shape foropenExternal, but that method predates this change.openServiceDeskUrlis new, and this stack adds the Relay Web implementation in a separate file. Confirm every runtime bridge exposes it, or call it through an optional method access.🛡️ Proposed defensive fix
const handleOpenTicket = useCallback( async (reference: string) => { const url = getSafeTicketUrl(reference); - if (!url || !(await globalThis.api?.openServiceDeskUrl(url))) { + if (!url || !(await globalThis.api?.openServiceDeskUrl?.(url))) { showToast('Unable to open the Service Desk reference.', 'error'); } }, [showToast], );
🤖 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 `@docs/SECURITY.md`:
- Around line 108-113: Update the Service Desk security description to
explicitly state that its handler accepts any HTTPS host, subject only to the
documented URL shape, credential, port, sender-validation, rate-limit, and
logging checks; clarify that the general OPEN_EXTERNAL host allowlist does not
apply.
In `@scripts/release-version.mjs`:
- Around line 121-125: Update runGit to pass an explicit maxBuffer option to
execFileSync, sized to accommodate readCommits output for full repository
histories when the range is HEAD, while preserving the existing cwd and UTF-8
encoding behavior.
In `@src/renderer/src/components/settings/administration/RelayServerPanel.tsx`:
- Around line 245-258: Update the RelayServerPanel apply-confirmation flow
around replaceProfiles to track its in-flight state, disable the “Apply stored
scope” and “Cancel” footer buttons while the replacement command runs, and
preserve the existing reauthentication loading pattern. Ensure rapid clicks
cannot dispatch multiple replaceProfiles commands with the same revision.
In `@src/renderer/src/stores/collectionStore.ts`:
- Around line 713-732: Update fetchOfflineSnapshot so a successful current
cached read passes error: null to updateSnapshot, matching fetchOnlineSnapshot.
Preserve the existing cached data, pagination, and membership metadata behavior
while clearing any stale error alongside valid offline data.
In `@src/renderer/src/tabs/DynatraceProblemsTab.tsx`:
- Around line 1071-1092: Update the Service Desk ticket composer help text near
the “Service Desk ticket number” field to state that entering a full HTTPS
ticket URL enables the “Open ↗” action, while preserving the existing guidance
for ticket numbers.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 201-222: Update the “Verify existing release assets” step to
enable Bash pipefail alongside nounset without adding errexit, preserving the
current conditional verification flow. Add cleanup for the temporary verify_dir
so downloaded assets are removed after the verification attempt, including when
the step completes through either branch.
In `@scripts/release-version.test.mjs`:
- Around line 162-177: Isolate the temporary Git repository from system and
global configuration in the test around the git commit setup. Set
GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to nonexistent paths in the spawned Git
environment, or disable signing directly on the commit invocation with
commit.gpgsign=false, so global templates, hooks, signing, and passphrase
prompts cannot affect the fixture.
- Around line 179-202: Add focused unit tests for the rejection paths in
planRelease and writeGitHubOutputs: verify planRelease throws for short and
uppercase headSha values, and verify writeGitHubOutputs throws when action is
malformed or outputPath is empty. Keep these tests narrow and separate from the
existing integration success-path test.
In `@src/main/dynatrace/DynatraceProblemsClient.test.ts`:
- Around line 313-334: Extend the rate-limit coverage around the test named
“preserves Dynatrace retry guidance on rate-limit errors” to exercise
Retry-After response headers, including both numeric-seconds and HTTP-date
values. Assert the resulting retry delay is preserved through
getDynatraceRetryAfterMs, while keeping the existing response-body
retryAfterSeconds coverage intact.
- Around line 299-311: Export the existing maximum-problems limit as
MAX_PROBLEMS from DynatraceProblemsClient, then update the exact-limit test to
derive its fixture size from that constant or use an injectable smaller limit.
Preserve the assertion that a result exactly at the configured limit sets
resultTruncated to true without a notification, while avoiding construction and
schema parsing of 10,000 records.
In `@src/main/dynatrace/DynatraceProblemsClient.ts`:
- Around line 369-375: Update numericCount to reject empty or whitespace-only
string values before converting them with Number, returning null for those
inputs. Preserve numeric handling and valid non-empty numeric strings so
inspectAlertingProfileField does not treat malformed problemCount data as zero.
In `@src/main/dynatrace/DynatraceProblemsManager.ts`:
- Around line 561-592: Refactor reconcileProblemScope to update changed problem
records through the shared bounded-concurrency iterator pattern used by
upsertProblems and deleteProblemsWithRelatedRecords, using UPSERT_CONCURRENCY.
Preserve the existing shouldExclude calculation, excludedCount result, update
payload, and request key behavior while allowing updates to run concurrently
with the configured limit.
- Around line 369-387: Update the catch path in DynatraceProblemsManager to
calculate consecutiveFailures before deriving retryDelay, then apply bounded
exponential backoff from POLL_INTERVAL_MS while retaining
getDynatraceRetryAfterMs(error) as a lower bound and capping the result with
MAX_RETRY_DELAY_MS. Reuse that failure count in writeSyncState, add the bound
alongside the interval constants, and adjust affected retry-timing tests to
advance timers according to the backoff.
In `@src/main/handlers/cacheHandlers.test.ts`:
- Around line 241-256: Split the combined test around rejects invalid query
identities and memberships into an it.each table with one rejection input per
row, covering the invalid read identity, invalid snapshot identity, and invalid
record ID cases. Keep the shared expectations that readQueryMembership and
writeQueryMembership are not called within each parameterized case so failures
identify the specific rejected input.
In `@src/main/handlers/windowHandlers.test.ts`:
- Around line 483-492: Extend the unsafe Service Desk URL table in the test for
getHandler(IPC_CHANNELS.OPEN_SERVICE_DESK_URL) with cases covering
normalizeServiceDeskUrl’s MAX_EXTERNAL_URL_LENGTH limit and non-string inputs,
asserting each resolves false and shell.openExternal is not called.
In `@src/renderer/src/components/settings/administration/RelayServerPanel.tsx`:
- Around line 28-45: Update selectedProfileNames in the RelayServerPanel profile
parsing flow to remove duplicate trimmed names before calculating
addedProfileNames and persisting the selection. Preserve the existing trimming
and empty-line filtering behavior, using a uniqueness-preserving collection or
equivalent.
In `@src/renderer/src/components/settings/AdministrationSettings.test.tsx`:
- Around line 270-317: The test should also verify the successful post-apply
behavior of replaceProfiles: after the Apply stored scope action resolves
successfully, assert that the review dialog closes and the success feedback
message is displayed. Keep the existing execute payload assertion unchanged.
In `@src/renderer/src/hooks/__tests__/useDynatraceProblems.test.ts`:
- Around line 85-140: The test case around useDynatraceProblems currently covers
only empty problem results and disabled related queries. Add a non-empty fixture
for open and historical problems in the useCollection mock, ensure related
collections reflect disabled-query runtime behavior, and assert that both
related batchedFilter.values contain deduplicated problem IDs with enabled set
to true.
In `@src/renderer/src/stores/collectionStore.ts`:
- Around line 153-161: Update buildFilterPredicate to remove the hard-coded
scopeExcluded fallback and keep its predicate generic. Extend
CollectionQueryOptions to carry fields requiring an undefined-to-false default,
then have the relevant caller configure scopeExcluded there and apply that
configuration when evaluating field values.
In `@src/renderer/src/stores/collectionStoreRegistry.ts`:
- Around line 93-103: Guard every dereference of the placeholder storeRef with
optional chaining, including the subscriber-count callback during
CollectionStore construction, the references around lines 113, 131, and 142, and
the finalization callback. Preserve the existing retained-store behavior while
preventing callbacks from throwing before storeRef is initialized.
In `@src/renderer/src/tabs/__tests__/DynatraceProblemsTab.test.tsx`:
- Around line 236-255: Update the “loads more resolved history” test around the
Load 100 more interaction to avoid hardcoding the current page size: import and
use HISTORY_PAGE_SIZE when constructing the button matcher, or use a numeric
matcher such as /Load \d+ more/. Apply the same change to the additional
assertion range noted in the comment, while preserving the existing
loadMoreHistory behavior check.
In `@src/renderer/src/tabs/dynatrace-problems.css`:
- Around line 321-337: Extract the shared button declarations and hover styling
from .dt-problems__history-pagination button and
.dt-problem-note__ticket-actions button into a reusable secondary-button class.
Update both contexts to use that class, retaining context-specific overrides
only for min-height, padding, and font-size.
In `@src/renderer/src/tabs/DynatraceProblemsTab.tsx`:
- Around line 1467-1491: Hoist the shared canSyncDynatrace predicate above
handleRefresh, then reuse that constant both inside the refresh handler and when
calling refreshControlCopy. Remove the duplicate local declaration while
preserving the existing behavior.
In `@src/shared/urlSecurity.ts`:
- Around line 2-3: Extend CONTROL_CHARACTER_PATTERN to reject Unicode format
characters alongside C0/C1 controls, covering bidi and directional formatting
characters in URL validation. Verify legitimate ticket URLs remain accepted in
the target environment, and keep the existing trimming and padded-input checks
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
1 participant
What changed
testupdates using Conventional Commits.Release behavior
testSHA.v1.0.0, then derives major/minor/patch versions from commit history.Verification
npm run typechecknpm run lintnpm run format:checknpm test(5,907 passing, 1 existing skip)npm run buildnpm audit --audit-level=high --omit=dev(0 vulnerabilities)npm run test:electron(38 passing, 1 intentional screenshot skip; three timing-only failures each passed targeted rerun)git diff --checkSummary by CodeRabbit