feat(note): event flows, split more persistence, perf, auth handling#153
Merged
KevinNitroG merged 24 commits intomainfrom Apr 8, 2026
Merged
feat(note): event flows, split more persistence, perf, auth handling#153KevinNitroG merged 24 commits intomainfrom
KevinNitroG merged 24 commits intomainfrom
Conversation
…event into separate model again
…integration, and workspaceevent Going to refactor the repo param, and public it all
going to refactor the params of repository
Member
Author
|
@CodeRabbit check my code please, Im gonna die |
|
Important Review skippedToo many files! This PR contains 180 files, which is 30 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (180)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai full review will this work? AI suggest me |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the note service’s persistence and event flow pipeline, moving away from Jet-based persistence and the previous pubsub setup toward a split read/write persistence approach (sqlc + repos/read-models) and a new outbox + event controller architecture, while also updating shared API event schemas and generated clients.
Changes:
- Reworked note-service infra wiring: introduce outbox forwarder, Kafka-based event controller, and Redis-based workspace event hub; remove legacy pubsub/integrationevent wiring.
- Split persistence into pgrepo (write/repos/uow) and pgreadmodel (read models), update sqlc queries (including dynamic filters + tracing), and remove Jet generation.
- Updated share OpenAPI schemas and regenerated Go/TS client types; adjusted Node services’ OTEL env behavior and CI branch patterns.
Reviewed changes
Copilot reviewed 170 out of 172 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates TODOs for note service event routing concerns. |
| pkg/api/share/share.gen.go | Regenerates share API models (note events, document committed event includes userId). |
| packages/api-gen/src/zod.gen.ts | Regenerates Zod schemas for share events (note + document committed). |
| packages/api-gen/src/types.gen.ts | Regenerates TS types for share events (note + document committed). |
| internal/note/wire.go | Fixes/normalizes imports and provider set wiring for note module. |
| internal/note/server.go | Refactors server startup to run migrations + start http/grpc/health/event/workspace hub/outbox. |
| internal/note/infra/workspaceevent/wire.go | Adds DI wiring for workspace event hub and Redis client. |
| internal/note/infra/workspaceevent/redis.go | Renames package and wraps go-redis client in a struct type. |
| internal/note/infra/wire.go | Rebuilds infra provider set around common/integrationpublisher/outbox/workspaceevent + sqlc/pgx. |
| internal/note/infra/service/authorization.go | Simplifies interceptor error wrapping and changes service interface returns to error. |
| internal/note/infra/pubsub/wire.go | Removes legacy pubsub provider wiring. |
| internal/note/infra/pubsub/outbox.go | Removes legacy placeholder pubsub file. |
| internal/note/infra/pubsub/integration.go | Removes legacy Kafka integration pubsub router/bus/processor implementation. |
| internal/note/infra/pubsub/common.go | Removes legacy Watermill logger + Kafka publisher + marshaler providers. |
| internal/note/infra/persistence/wire.go | Replaces old pg wiring with PGRepo + PGReadModel provider sets and new pg helpers. |
| internal/note/infra/persistence/pgsqlc/workspace.sql.go | Updates sqlc-generated workspace queries (split by id/slug, adds tracing, dynamic filters). |
| internal/note/infra/persistence/pgsqlc/workspace.sql | Splits GetWorkspace into GetWorkspaceByID/GetWorkspaceBySlug; adds conditional FOR UPDATE. |
| internal/note/infra/persistence/pgsqlc/workspace_read.sql.go | Adds sqlc-generated read-only workspace query implementations. |
| internal/note/infra/persistence/pgsqlc/workspace_read.sql | Adds read-only workspace queries (by slug, check slug exists). |
| internal/note/infra/persistence/pgsqlc/temp_tables.sql.go | Adds tracing to temp-table sqlc exec helpers. |
| internal/note/infra/persistence/pgsqlc/note.sql | Refactors note queries (GetNoteByID, dynamic filtering, adds GetWorkspaceIDsByNoteIDs). |
| internal/note/infra/persistence/pgsqlc/note_read.sql | Adds read-only note queries (by id/ids, workspace, folder ids, trash). |
| internal/note/infra/persistence/pgsqlc/note_link.sql.go | Removes some write-model queries, simplifies outgoing links query, adds tracing. |
| internal/note/infra/persistence/pgsqlc/note_link.sql | Simplifies outgoing links query; removes backlinks/workspace link queries from write file. |
| internal/note/infra/persistence/pgsqlc/note_link_read.sql.go | Adds sqlc-generated read-only note link queries (backlinks/outgoing/workspace links). |
| internal/note/infra/persistence/pgsqlc/note_link_read.sql | Adds read-only note link queries. |
| internal/note/infra/persistence/pgsqlc/folder.sql | Simplifies folder queries; adds workspace-scoped and dynamic list queries with conditional FOR UPDATE. |
| internal/note/infra/persistence/pgsqlc/folder_read.sql | Adds read-only folder queries (recursive tree, root ids, trash). |
| internal/note/infra/persistence/pgrepo/workspace.go | Adds new workspace repo implementation using sqlc (read + save + event publishing). |
| internal/note/infra/persistence/pgrepo/uow.go | Adds new pgx-based UnitOfWork with repo registry and publisher factory support. |
| internal/note/infra/persistence/pgrepo/runintx.go | Adds helper to run repo operations in a transaction (used by repos). |
| internal/note/infra/persistence/pgrepo/publisher.go | Introduces publisher + publisher-factory interfaces for outbox/event publishing. |
| internal/note/infra/persistence/pgrepo/err.go | Renames/moves pg error mapping helper into pgrepo. |
| internal/note/infra/persistence/pgrepo/common.go | Adds trashedBy mapping helpers between domain and sqlc enums. |
| internal/note/infra/persistence/pgreadmodel/workspacetree.go | Adds read model for workspace tree building from folders/notes queries. |
| internal/note/infra/persistence/pgreadmodel/workspacegraph.go | Adds read model for workspace graph (notes + links, optional orphan filtering). |
| internal/note/infra/persistence/pgreadmodel/workspacebyslug.go | Adds read model to fetch workspace by slug (read-only query path). |
| internal/note/infra/persistence/pgreadmodel/showtrash.go | Adds read model for trash listing (notes + folders). |
| internal/note/infra/persistence/pgreadmodel/notelinks.go | Adds read model for note backlinks/outgoing links with existence check. |
| internal/note/infra/persistence/pgreadmodel/notegraph.go | Adds read model for note-centric graph with BFS depth. |
| internal/note/infra/persistence/pgreadmodel/note.go | Adds placeholder read model for GetNote (currently stub). |
| internal/note/infra/persistence/pgreadmodel/graph.go | Adds graph builder helper used by graph read models. |
| internal/note/infra/persistence/pgreadmodel/err.go | Adds pg error mapping helper for read models. |
| internal/note/infra/persistence/pgreadmodel/checkslugexists.go | Adds read model for workspace slug existence. |
| internal/note/infra/persistence/pgjet/public/table/workspaces.go | Removes Jet-generated workspaces table builder. |
| internal/note/infra/persistence/pgjet/public/table/table_use_schema.go | Removes Jet schema setter helper. |
| internal/note/infra/persistence/pgjet/public/table/notes.go | Removes Jet-generated notes table builder. |
| internal/note/infra/persistence/pgjet/public/table/note_links.go | Removes Jet-generated note_links table builder. |
| internal/note/infra/persistence/pgjet/public/table/goose_db_version.go | Removes Jet-generated goose_db_version table builder. |
| internal/note/infra/persistence/pgjet/public/table/folders.go | Removes Jet-generated folders table builder. |
| internal/note/infra/persistence/pgjet/public/model/workspaces.go | Removes Jet-generated workspaces model. |
| internal/note/infra/persistence/pgjet/public/model/trashed_by.go | Removes Jet-generated trashed_by enum model. |
| internal/note/infra/persistence/pgjet/public/model/notes.go | Removes Jet-generated notes model. |
| internal/note/infra/persistence/pgjet/public/model/note_links.go | Removes Jet-generated note_links model. |
| internal/note/infra/persistence/pgjet/public/model/goose_db_version.go | Removes Jet-generated goose_db_version model. |
| internal/note/infra/persistence/pgjet/public/model/folders.go | Removes Jet-generated folders model. |
| internal/note/infra/persistence/pgjet/public/enum/trashed_by.go | Removes Jet-generated trashed_by enum helpers. |
| internal/note/infra/persistence/pg/workspace.go | Removes prior Jet-based workspace repo implementation. |
| internal/note/infra/persistence/pg/uow.go | Removes prior stdlib/sql-based unit of work implementation. |
| internal/note/infra/persistence/pg/pg.go | Removes old pg pool/queries/stdlib providers (moved to persistence/pg.go). |
| internal/note/infra/persistence/pg.go | Adds pgx pool creation + sqlc queries + stdlib adapter and keeps migrations/ping. |
| internal/note/infra/outbox/wire.go | Adds outbox DI wiring and binds PublisherFactory. |
| internal/note/infra/outbox/outbox.go | Adds SQL outbox forwarder runner using watermill-sql subscriber + forwarder. |
| internal/note/infra/outbox/forwarder.go | Adds SQL publisher factory and event metadata/topic publishing logic. |
| internal/note/infra/integrationpublisher/wire.go | Adds DI wiring for integration publisher. |
| internal/note/infra/integrationpublisher/integrationpublisher.go | Implements publishing of integration events (note created/updated/deleted) to Kafka topics. |
| internal/note/infra/common/wire.go | Adds common Kafka publisher wiring shared by outbox + integration publisher. |
| internal/note/infra/common/kafkapublisher.go | Adds Kafka publisher wrapper binding to both outbox and integration publisher interfaces. |
| internal/note/domain/workspacerepo.go | Switches repo interface methods from custom errs.Error to error. |
| internal/note/domain/workspace.go | Adjusts workspace event creation plumbing; changes internal event-add method name and BaseEvent construction. |
| internal/note/domain/uow.go | Switches UoW interface callback signature to error. |
| internal/note/domain/trashservice.go | Switches service methods to error and adapts nil/uuid.Nil semantics. |
| internal/note/domain/noteservice.go | Switches UpdateNoteSizeBasedOnContent to return error. |
| internal/note/domain/noterepo.go | Replaces builder-style params with a struct and switches interface returns to error. |
| internal/note/domain/folderrepo.go | Replaces builder-style params with a struct and switches interface returns to error. |
| internal/note/domain/folder.go | Refactors folder model: icon as string, parent id semantics, adds deleted flag + permanent delete event. |
| internal/note/domain/event.go | Adds GetUserID to Event, changes BaseEvent constructor return type, and changes icon fields to string. |
| internal/note/domain/common.go | Refactors TrashedBy into uint8 enum with validation and string mapping helpers. |
| internal/note/controller/wire.go | Replaces integrationevent controller wiring with new event controller wiring. |
| internal/note/controller/http/workspace.go | Routes command/query handling through new app struct fields; switches to WorkspaceEventHub subscribe API. |
| internal/note/controller/http/note.go | Updates handlers to new app struct fields and icon parameter behavior. |
| internal/note/controller/http/map.go | Updates API mapping logic for icon/tags/role conversion and weight handling. |
| internal/note/controller/http/http.go | Switches handler dependency from WorkspaceEventPubSub to WorkspaceEventHub. |
| internal/note/controller/http/folder.go | Updates handlers to new app struct fields and icon parameter behavior. |
| internal/note/controller/health/health.go | Updates health checks to use persistence Pg + Redis client ping instead of old pubsub check. |
| internal/note/controller/event/wire.go | Renames integrationevent package to event and updates provider. |
| internal/note/controller/event/notifyworkspaceitemsupdated.go | Adds handlers that translate domain-event metadata into app-level workspace-item update notifications. |
| internal/note/controller/event/event.go | Adds Kafka-based event controller wiring, handler registration, middleware, and topic mapping. |
| internal/note/controller/event/documentcommitted.go | Adds handler converting share DocumentCommittedEvent into app DocumentCommitted command. |
| internal/note/config/wire.go | Wires new Advanced config nesting (DomainEvent + WorkspaceEvent). |
| internal/note/config/config.go | Adds Advanced.DomainEvent + Advanced.WorkspaceEvent config and defaults. |
| internal/note/component/wire.go | Renames package and adds Watermill logger + marshaler providers. |
| internal/note/component/watermill.go | Adds Watermill JSON marshaler + slog logger adapter providers. |
| internal/note/component/validator.go | Renames package to component. |
| internal/note/component/domaineventtopic.go | Adds domain-event-to-topic mapping for internal note events. |
| internal/note/app/workspaceevent.go | Splits workspace event pubsub into publisher/subscriber hub interfaces and simplifies event type registry. |
| internal/note/app/wire.go | Splits wiring into Cmds/Events/Queries provider sets and removes integration pubsub provider set. |
| internal/note/app/svc_authorization.go | Switches authorization service interface to return error. |
| internal/note/app/q_showtrash.go | Switches query handler/readmodel interface to return error. |
| internal/note/app/q_model.go | Changes app query models: icon fields to string, GraphNode weight to float64, username to string. |
| internal/note/app/q_getworkspacetree.go | Adjusts query params (RootFolderID/Depth non-pointer) and switches interfaces to error. |
| internal/note/app/q_getworkspacemembers.go | Switches handler signature to error. |
| internal/note/app/q_getworkspacegraph.go | Switches handler/readmodel signature to error. |
| internal/note/app/q_getworkspacebyslug.go | Renames readmodel interface and switches signatures to error. |
| internal/note/app/q_getnotelinks.go | Switches handler/readmodel signature to error. |
| internal/note/app/q_getnotegraph.go | Switches handler/readmodel signature to error. |
| internal/note/app/q_getnote.go | Switches handler/readmodel signature to error. |
| internal/note/app/q_checkworkspaceexists.go | Switches handler/readmodel signature to error. |
| internal/note/app/persistence.go | Removes Persistence interface (server now uses persistence.Pg directly). |
| internal/note/app/internalpubsub.go | Removes unused placeholder file. |
| internal/note/app/integrationpubsub.go | Removes legacy IntegrationPubSub abstraction. |
| internal/note/app/integrationevent.go | Introduces integration event types + publisher interface. |
| internal/note/app/e_notifyworkspaceitemsupdated.go | Adds debounced workspace item update event publishing. |
| internal/note/app/e_notecreatedintegration.go | Adds a domain-to-integration event handler skeleton for note-created events. |
| internal/note/app/e_documentcommitted.go | Refactors document committed handler to use internal event struct fields. |
| internal/note/app/cmd_updateworkspacemembers.go | Updates handler wiring and signature; adds IntegrationPublisher dependency. |
| internal/note/app/cmd_unpublishworkspace.go | Switches command handler signature to error. |
| internal/note/app/cmd_unpublishnote.go | Switches command handler signature to error. |
| internal/note/app/cmd_trashworkspaceitems.go | Switches UoW callback signature and updates repo param usage to new structs. |
| internal/note/app/cmd_restoretrashworkspaceitems.go | Switches signatures and updates repo param usage to new structs. |
| internal/note/app/cmd_renameworkspace.go | Switches command handler signature to error. |
| internal/note/app/cmd_renamenote.go | Switches signature to error and notes missing transaction. |
| internal/note/app/cmd_renamefolder.go | Switches command handler signature to error. |
| internal/note/app/cmd_publishworkspace.go | Adds placeholder publish workspace command handler. |
| internal/note/app/cmd_publishnote.go | Switches command handler signature to error. |
| internal/note/app/cmd_permanentlydeleteworkspaceitems.go | Refactors handler to depend on UoW; leaves logic largely TODO/commented. |
| internal/note/app/cmd_permanentlydeletenote.go | Refactors permanent delete note to use UoW. |
| internal/note/app/cmd_permanentlydeletefolder.go | Refactors permanent delete folder to use UoW. |
| internal/note/app/cmd_moveworkspaceitems.go | Switches UoW callback signature and updates repo param usage to new structs. |
| internal/note/app/cmd_generatedailynote.go | Switches handler signature to error. |
| internal/note/app/cmd_deleteworkspace.go | Switches command handler signature to error. |
| internal/note/app/cmd_createworkspace.go | Uses uuid v7 for root folder, switches UoW callback signature, and updates folder creation. |
| internal/note/app/cmd_createnote.go | Changes icon handling to string and switches command handler signature to error. |
| internal/note/app/cmd_createfolder.go | Changes icon handling to string and switches command handler signature to error. |
| internal/note/app/app.go | Replaces CommandHandlers/QueryHandlers/IntegrationEventHandlers with Cmds/Queries/Events; removes pubsub lifecycle. |
| go.mod | Adds watermill-sql + debounce; removes Jet direct dependency and adjusts indirect deps. |
| cmd/note/sqlc.yaml | Switches to plugin-based sqlc-gen-go with dynamic filter + tracing options. |
| cmd/note/project.json | Removes Jet generation target from Nx workflow. |
| cmd/note/gen-jet.sh | Removes Jet generation script. |
| apps/search-worker/src/otel.ts | Changes OTEL instrumentation env setup logic. |
| apps/search-worker/.env.development-mon | Removes enabled-instrumentations env var (now set in code). |
| apps/document/src/otel.ts | Changes OTEL instrumentation env setup logic. |
| apps/document/.env.development-mon | Removes enabled-instrumentations env var (now set in code). |
| api/share/components/schemas/UserDeletedEvent.yaml | Removes redundant readOnly override. |
| api/share/components/schemas/NoteUpdatedEvent.yaml | Replaces $ref with explicit updated event schema. |
| api/share/components/schemas/NoteSearch.yaml | Replaces $ref fields with explicit uuid/string fields; removes examples. |
| api/share/components/schemas/NoteDeletedEvent.yaml | Replaces $ref with explicit uuid string schema. |
| api/share/components/schemas/NoteCreatedEvent.yaml | Replaces $ref with explicit uuid/string schema and nullable icon. |
| api/share/components/schemas/EventBase.yaml.bak | Removes backup schema file. |
| api/share/components/schemas/DocumentCommittedEvent.yaml | Adds userId requirement and adjusts examples. |
| api/redocly.yaml | Disables scalar-property-missing-example rule for share API linting. |
| .golangci.yaml | Adjusts errcheck exclusion pattern list (commented-out line). |
| .github/workflows/ci.yaml | Updates CI branch globbing from * to ** for push/PR. |
Comments suppressed due to low confidence (3)
internal/note/app/cmd_createworkspace.go:63
- The UnitOfWork callback is using the handler’s injected repos (
h.folderRepo/h.workspaceRepo) instead of the transaction-scoped repos from theRepoRegistry(r.Folder()/r.Workspace()). This breaks transactional guarantees and can bypass tx-scoped publisher/outbox wiring. Use the repos fromrinside this closure.
internal/note/app/cmd_permanentlydeletenote.go:64 note.Deleted()is a boolean getter, so this handler doesn’t mark the note as deleted before saving. Call the mutating domain method (e.g.,note.Delete(cmd.UserID)or the intended permanent-delete method) so the repo can persist the deletion and emit the correct event.
internal/note/app/cmd_permanentlydeletefolder.go:64folder.Deleted()is a boolean getter, so this handler doesn’t mark the folder as deleted before saving. Call the mutating domain method (e.g.,folder.PermanentlyDelete(cmd.UserID)) so the repo can persist the deletion and emit the correct event.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+37
to
+41
| tx, err := params.pgxPool.Begin(ctx) | ||
| if err != nil { | ||
| return errs.NewPersistenceInternal("failed to begin transaction", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
A transaction is started here, but the code later passes params.queries into the callback unchanged, so DB operations won’t run on this tx. Build a tx-backed pgsqlc.Queries from tx (and a tx-backed publisher) and pass those into the callback.
Comment on lines
+61
to
+64
| fnParams := &RunInTxFnparams{ | ||
| queries: params.queries, | ||
| publisher: params.publisher, | ||
| } |
There was a problem hiding this comment.
This is constructing callback params from params.queries / params.publisher, which are the non-tx values in the non-transaction branch. This can cause nil publisher panics when publishing events. Populate fnParams with tx-backed implementations in the non-transaction branch.
Comment on lines
+85
to
+87
| default: | ||
| panic("invalid workspace role") | ||
| } |
There was a problem hiding this comment.
Panicking in HTTP mapping code can crash the server if an unexpected role value appears (e.g., after adding a new enum value). Prefer returning a safe default or propagating an error instead of panicking.
internal/note/controller/http/map.go
Outdated
Comment on lines
+161
to
+165
| func toNoteLink(n *app.NoteLink) note.NoteLink { | ||
| id := n.ID | ||
| return note.NoteLink{ | ||
| Id: &id, | ||
| Id: &n.ID, | ||
| Name: n.Name, | ||
| Icon: n.Icon, | ||
| Icon: &n.Icon, |
There was a problem hiding this comment.
toNoteLink always sets a non-nil Icon pointer, even when the icon is an empty string, while other mappers treat empty strings as nil. For consistency and to avoid emitting empty-string icons, set Icon to nil when n.Icon == "".
Comment on lines
+14
to
+17
| func NewNoteCreatedDomainToIntegrationEventHandler( | ||
| integrationPublisher IntegrationPublisher, | ||
| noteRepo domain.NoteRepo, | ||
| ) *NoteCreatedDomainToIntegrationEventHandler { |
There was a problem hiding this comment.
noteRepo is accepted but never used in this constructor, which will be flagged by linters and can fail the build. Remove the parameter or store/use it (or rename to _ if it’s intentionally unused).
Comment on lines
+92
to
+96
| sql.TxFromPgx(pgxTx), | ||
| sql.PublisherConfig{ | ||
| SchemaAdapter: sql.DefaultPostgreSQLSchema{}, | ||
| AutoInitializeSchema: true, | ||
| }, |
There was a problem hiding this comment.
The config includes a required outbox_table_name, but this SQL publisher uses the default schema adapter/config and doesn’t apply the configured table name. Wire the configured table name into the publisher/subscriber schema adapter/config (or remove the config field).
Comment on lines
+28
to
+32
| PollInterval: time.Second, | ||
| InitializeSchema: true, | ||
| SchemaAdapter: sql.DefaultPostgreSQLSchema{}, | ||
| ConsumerGroup: "", // NOTE: If scale, we should care about this | ||
| }, |
There was a problem hiding this comment.
ConsumerGroup is set to an empty string. When running multiple instances, this can lead to duplicated forwarding because each instance may process the same outbox rows. Prefer a stable, non-empty consumer group name from config.
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Member
Author
@coderabbitai pause @coderabbitai stop Hey hey stop |
|
✅ Created PR with unit tests: #156 |
Member
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Member
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 177 out of 179 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (3)
internal/note/app/cmd_createworkspace.go:67
- Inside the UnitOfWork transaction, this handler is using the injected repos (h.folderRepo / h.workspaceRepo) instead of the transaction-scoped repos from the RepoRegistry (r.Folder()/r.Workspace()). This can bypass the transaction and lead to partial writes if the tx rolls back or if the repos are configured differently for tx vs non-tx operations. Use the repos from
rwithin the Execute callback.
internal/note/app/cmd_createworkspace.go:74 - If
CreateWorkspaceWithOwnershipfails, the workspace and root folder have already been committed to the DB, leaving a workspace without the intended ownership/ACL setup. Consider adding a compensating action (e.g., delete the workspace), or moving ownership creation into the same persistence transaction if possible (or at least documenting/handling the inconsistency explicitly).
internal/note/app/cmd_permanentlydeleteworkspaceitems.go:78 - This command handler currently does not perform any deletion and always returns nil (success). Since this handler is wired and invoked by the HTTP endpoint, it will report 204 even though nothing was deleted. Either implement the deletion logic (preferably via uow + repos) or return an explicit unimplemented/error response until it’s ready.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+22
to
+23
| func (h *Note) GetNote(ctx context.Context, q *app.GetNote) (*app.Note, error) { | ||
| return nil, nil |
There was a problem hiding this comment.
GetNote read model is currently a stub that returns (nil, nil). This will cause downstream callers to treat the request as successful and likely panic when dereferencing the result (e.g., the HTTP handler does toNote(*result)). Implement this method (or return a concrete error such as errs.NewUnimplemented()) before merging.
Comment on lines
+95
to
+99
| txQueries := pgsqlc.New(tx) | ||
| publisher, err := u.publisherFactory.Create(tx) | ||
| if err != nil { | ||
| return errs.NewPersistenceInternal("failed to create publisher in unit of work", err) | ||
| } |
There was a problem hiding this comment.
If publisherFactory.Create(tx) fails, the transaction opened by u.pool.Begin(ctx) is returned without being rolled back. This can leak an open transaction/connection back to the pool. Roll back the transaction before returning on this error path (and consider a general defer tx.Rollback(ctx) pattern that is cancelled by a successful commit).
Comment on lines
+65
to
+69
| queries := pgsqlc.New(tx) | ||
| publisher, err := r.publisherFactory.Create(tx) | ||
| if err != nil { | ||
| return errs.NewPersistenceInternal("failed to create publisher in RunInTx", err) | ||
| } |
There was a problem hiding this comment.
If publisherFactory.Create(tx) fails after starting a transaction, the function returns without rolling the transaction back. This can leave an open transaction and tie up a pooled connection. Roll back the tx on this error path (or use a defer tx.Rollback(ctx) that is cleared by commit).
Comment on lines
+19
to
+22
| type RunInTxFnparams struct { | ||
| queries *pgsqlc.Queries | ||
| publisher Publisher | ||
| } |
There was a problem hiding this comment.
The exported type name RunInTxFnparams has a casing typo (params vs Params). This will trip common Go linters and makes the API look inconsistent. Consider renaming to RunInTxFnParams (and updating the function signature accordingly).
Comment on lines
+5
to
+8
| userId: | ||
| type: string | ||
| format: number | ||
|
|
There was a problem hiding this comment.
userId is defined as type: string but uses format: number, which is not a valid/appropriate format for a string and conflicts with the generated clients (Go/TS expect a string userId). Update the schema to use a consistent string format (or omit format entirely).
Comment on lines
+28
to
+33
| sql.SubscriberConfig{ | ||
| PollInterval: time.Second, | ||
| InitializeSchema: true, | ||
| SchemaAdapter: schemaAdapter, | ||
| ConsumerGroup: "", // NOTE: If scale, we should care about this | ||
| }, |
There was a problem hiding this comment.
ConsumerGroup is configured as an empty string. With multiple instances, this typically results in each instance consuming/forwarding the same outbox rows (duplicate publishes). Please wire this to a stable, configurable consumer group (e.g., from config) and document the scaling behavior.
Comment on lines
+132
to
+143
| for _, childFolder := range folderMap { | ||
| if childFolder.ParentID != nil && *childFolder.ParentID == folderID { | ||
| childTree := h.buildFolderTree( | ||
| childFolder.ID, | ||
| childFolder.Name, | ||
| childFolder.Icon, | ||
| childFolder.UpdatedAt, | ||
| folderMap, | ||
| notesByFolder, | ||
| ) | ||
| result.Children = append(result.Children, childTree) | ||
| } |
There was a problem hiding this comment.
buildFolderTree scans the entire folderMap for children on every recursive call (for _, childFolder := range folderMap), which makes the runtime O(n^2) for large trees and also yields non-deterministic child ordering (map iteration order). Consider precomputing a parentID -> []child folder map (and optionally sorting) so each node only visits its direct children once.
KevinNitroG
changed the title
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.
No description provided.