Collapse duplicated code across the solution - #201
Merged
Conversation
A repo-wide clone scan plus a semantic pass found ~85 verified places where the same logic existed more than once. This removes them, mostly by routing callers through a helper or base type that already existed. Backend: - Five retention services shared a byte-identical ExecuteAsync loop; they now derive from LeaderGatedRetentionService. The broad catch stays in each service's RunIterationAsync, one level below the host-fatal boundary. - fileOperation/folderOperation share FileSystemOperationActivityBase; the marker-envelope PostProcess preamble is now one helper used by seven activities. - The AES-GCM envelope (version byte + nonce + ciphertext + tag) had two independent implementations; both now use SecretEnvelope. Header validation stays outside the metrics scope, as before. - Crypto call/latency instrumentation collapses into DataMetrics.MeasureCrypto. - The ExecRow projection, the long-running/queued-long collectors, the NotificationDispatcher send tail, CsvField, the trigger manual.* extraction and the folder-tree helpers each exist once now. - ScimProvisioningService now forwards audits through AuditEventForwarder instead of hand-rolling the ECS scope. This fixes a real inconsistency: it hard-coded event.category=iam where the shared classifier returns "configuration" for SCIM_GROUP_* codes. Clients: - ApiException, the response plumbing and the config.json read side move to NodePilot.Core.Clients, shared by the CLI and the MCP server. The DTO copies stay duplicated on purpose (ApiDtoParityTests). Frontend: - The AI chat message mutators and the thread picker were duplicated between the designer panel and the global chat page; both now share one module. - LoggingTelemetrySection carried a private fork of useSectionForm. The fork had drifted and was the better version (it re-sends the mapped payload after a 412), so that behaviour moved into the shared hook before the fork was removed. - useDashboardFeed/useOperationsFeed, the type-the-phrase confirm dialog and the node context menu now use shared implementations. Dead code: three unused DbErrorClassifier predicates, a deprecated cloneableConfigKeys alias, four unused frontend exports, two orphaned stress scripts and an unused npm dependency. Test doubles: FakeSmtpServer, CapturingLogger, StubHttpClientFactory, StubGlobalVariableStore and FakeEngine each existed in two places; they now live in TestCommons or a shared Helpers file. ActivityConfigReferenceTests learned to follow an executor's base class and helpers, so config keys that moved into shared types stay verified instead of being exempted. Net effect: 1407 fewer lines of code. Full backend suites (5486 tests) and the frontend suite (2596 tests) pass; the solution builds clean.
This was referenced Aug 14, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| return ParseFromDocument(doc, result); | ||
| } | ||
| => ParseFromReader(() => XmlReader.Create(new StringReader(xml), HardenedReaderSettings)); |
| public string? RedactAndCap(string? value, int maxChars) | ||
| { | ||
| if (string.IsNullOrEmpty(value)) return value; | ||
| var redacted = Redact(value) ?? value; |
Comment on lines
+64
to
+67
| catch (Exception ex) | ||
| { | ||
| return Error(ex.Message, json); | ||
| } |
| { | ||
| var configured = configuredRoot(); | ||
| return string.IsNullOrWhiteSpace(configured) | ||
| ? Path.Combine(AppContext.BaseDirectory, "knowledge", defaultFolderName) |
Comment on lines
+117
to
+120
| catch (Exception ex) | ||
| { | ||
| _sessionTcs.TrySetException(ex); | ||
| } |
| public async ValueTask DisposeAsync() | ||
| { | ||
| _cts.Cancel(); | ||
| try { _listener.Stop(); } catch { /* already stopped */ } |
| try { _listener.Stop(); } catch { /* already stopped */ } | ||
| if (_acceptLoop is not null) | ||
| { | ||
| try { await _acceptLoop; } catch { /* shutdown noise */ } |
Folding the three field pickers onto one PickerPopover passed the whole useSearchablePicker() result through as a prop. Inside the component that object is opaque, so the React Compiler treats every read on it as a ref access during render and eslint fails the build with seven "Cannot access refs during render" errors. Destructure once at the top of the component instead. The refs are then forwarded as plain values, which is what the pre-dedup code did and what AnchoredPickerPopover expects.
Both armed-trigger blackout tests split their evaluator verdict on "now + 30 seconds", while the fixture's cron fires at the top of the coming hour. Whenever the suite runs in the last half minute of an hour the cutoff lands past the fire time and the split inverts, so the flagged case reports no window and the not-flagged case reports one. CI hit exactly that at 19:00:20Z. Derive the cutoff from the hour boundary instead and place it midway, so it is strictly between "now" and the fire time at every wall-clock moment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A dedup sweep across the solution: repeated logic is pulled into one place, with the shared
helper living where the dependency graph already allows it (a new
Engine/Mail/SmtpTransportfor the SMTP paths, shared activity/query/path-guard helpers for the rest).
This is the first of three stacked branches. Merge order matters: this one, then
refactor/remove-dead-abstractions, thensecurity/harden-auth-boundary-and-trigger-scopes.Verification
Full backend suite (5612 tests across 6 projects) and full frontend unit suite (2657 tests,
202 files) pass on the tip of the third branch, which contains this commit. Solution builds
with 0 errors.