Skip to content

feat: implement threads service#3

Merged
rowan-stein merged 2 commits intomainfrom
noa/issue-1-work
Mar 13, 2026
Merged

feat: implement threads service#3
rowan-stein merged 2 commits intomainfrom
noa/issue-1-work

Conversation

@casey-brooks
Copy link
Contributor

Summary

  • implement the Threads gRPC service with migrations, store, notifier, and handlers
  • add buf generation config, Dockerfile, Makefile, and CI/release workflows
  • add Helm chart for threads deployment

Testing

  • go vet ./...
  • go test ./...
  • go build ./...

Issue

Closes #1

@casey-brooks
Copy link
Contributor Author

Test & Lint Summary

  • go vet ./...
  • go test ./...
  • go build ./...

Tests: passed 0, failed 0, skipped 0
Lint: go vet ./... (no issues)

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Good overall implementation — the service structure, gRPC handler patterns, store layer, and Helm chart closely follow the reference repos. The domain logic is sound and the data model matches the architecture spec.

There are several issues to address before merging:

Build-breaking (2):

  • Dockerfile is missing protobuf generation — Docker builds will fail since gen/ is gitignored
  • publish-edge workflow has the same problem — no proto generation before docker build

Code quality (3):

  • Duplicated pagination token functions (EncodeThreadMessagePageToken / EncodeUnackedMessagePageToken are identical)
  • Duplicated row-scanning logic in ListMessages / ListUnackedMessages — extract a shared helper
  • toProtoThreadStatus silently returns UNSPECIFIED for unknown values instead of failing loudly

Minor (3):

  • schema_migrations table defined in both migration SQL and Go runner
  • grpc.DialContext is deprecated — use grpc.NewClient
  • Unnecessary if len(participants) > 0 guard in converter
  • N+1 participant queries in ListThreads

Please address the major items. The implementation is close to ready.

@casey-brooks
Copy link
Contributor Author

Summary

  • make Dockerfile self-contained with buf generation and remove schema_migrations from SQL
  • consolidate message pagination/scan logic and batch thread participants
  • enforce invariant conversions and update grpc client usage

Test & Lint Summary

  • go vet ./...
  • go test ./...

Tests: passed 0, failed 0, skipped 0
Lint: go vet ./... (no issues)

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

All 9 review comments verified as resolved in the code:

  1. Dockerfile — buf installed and proto generation added in builder stage; self-contained builds
  2. migrations/0001_init.sqlschema_migrations creation removed; runner handles it
  3. pagination.go — Duplicates consolidated into EncodeMessagePageToken / DecodeMessagePageToken
  4. converter.gotoProtoThreadStatus default case now panics on invariant violation
  5. converter.go — Unnecessary if len(participants) > 0 guard removed
  6. main.gogrpc.DialContext replaced with grpc.NewClient
  7. messages.goscanMessagePage helper extracted, eliminating duplication
  8. threads.go/store.go — N+1 replaced with batched loadParticipantsByThreadIDs
  9. messages.gomath.MaxInt32 used instead of obscure bit manipulation

Clean implementation. Good to merge.

@rowan-stein rowan-stein merged commit 693b0d8 into main Mar 13, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants