fix: address review feedback on eventstore/timeline/dlq service modules#139
Merged
intel352 merged 2 commits intofeat/issue-90-domain-servicesfrom Feb 23, 2026
Merged
Conversation
…t64 config handling, and test fix Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Extract EventStore, Timeline, and DLQ as self-registering modules
fix: address review feedback on eventstore/timeline/dlq service modules
Feb 23, 2026
intel352
added a commit
that referenced
this pull request
Feb 23, 2026
…es (#90) (#125) * feat: extract EventStore, Timeline, and DLQ as self-registering modules (#90) Follow the feature flag module pattern (PR #56) to extract three domain services into self-registering module types: - eventstore.service: wraps SQLiteEventStore with config-driven setup - timeline.service: wraps TimelineHandler and ReplayHandler - dlq.service: wraps DLQHandler Each module implements ProvidesServices() for auto-discovery and is declared in admin/config.yaml. Hardcoded wiring removed from main.go with backward-compatible fallbacks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: address review feedback on eventstore/timeline/dlq service modules (#139) * Initial plan * Apply PR review feedback: fix timeline/DLQ discovery, nil check, float64 config handling, and test fix Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
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
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.
Addresses reviewer feedback from the domain-services extraction PR. Fixes several correctness issues in the service discovery logic and config handling.
Dead type-assertion loops removed (
cmd/server/main.go)Both
svc.(*module.TimelineServiceModule)andsvc.(*module.DLQServiceModule)assertions could never succeed — those modules register their muxes/store as services viaProvidesServices(), not themselves. Replaced with single-pass loops over the registry that collect services by suffix.Partial-discovery bugs fixed (
cmd/server/main.go)timelineDiscoveredwas set totrueas soon as.timelinewas found, skipping the fallback even when.replayor.backfillwere still nil. Now all three must be non-nil before marking discovered.dlqDiscoveredwas set when the.adminhandler was found, butdlqStorecould still be nil. Now both mux and store must be found together.float64 config handling (
plugins/dlq,plugins/eventstore)YAML numeric values decode as
float64; integer-only assertions silently droppedmax_retriesandretention_days. Addedfloat64branches consistent with the rest of the codebase (seeplugins/api/plugin.go:getIntConfig).Nil guard for EventStore (
module/timeline_service.go)NewTimelineServiceModulenow panics immediately with a clear message ifeventStoreis nil, rather than panicking on the first HTTP request.Unimplemented config fields documented (
module/dlq_service.go,module/eventstore_service.go)max_retries,retention_days(DLQ), andretention_days(EventStore) are stored and exposed via accessors but not yet wired to the underlying stores. Added doc comments marking them as reserved for future implementation.Test fix (
module/eventstore_service_test.go)TestEventStoreServiceModule_DefaultDBPathwas passing a non-empty path, contradicting its stated purpose. Updated to pass an emptyDBPathand verify the store initializes via the default path.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.