onboarding widget backend changes - #3605
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds persistent client metadata: a ChangesClient configuration metadata support
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPHandler
participant RDBConfigStore
participant Database
Client->>HTTPHandler: POST /api/config/metadata {patch}
activate HTTPHandler
HTTPHandler->>RDBConfigStore: UpdateClientMetadata(patch)
activate RDBConfigStore
RDBConfigStore->>Database: SELECT metadata_json FOR UPDATE
Database-->>RDBConfigStore: existing metadata_json
RDBConfigStore->>RDBConfigStore: Merge patch (nil deletes), MarshalSorted
RDBConfigStore->>Database: UPDATE config_client SET metadata_json=...
Database-->>RDBConfigStore: update result
RDBConfigStore->>Database: COMMIT
deactivate RDBConfigStore
HTTPHandler-->>Client: { "success": true } or error
deactivate HTTPHandler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
akshaydeo
marked this pull request as ready for review
May 19, 2026 21:33
Contributor
Confidence Score: 5/5Safe to merge; the core read-modify-write path is correctly protected with FOR UPDATE and the metadata column is cleanly isolated from the config.json sync path. The locking strategy (dbForUpdate), transaction isolation, and the metadata-preservation logic in UpdateClientConfig are all correct. The two findings are a dead-code ErrNotFound guard in the GET handler and a slightly unconventional HTTP status code choice — neither affects data correctness or runtime safety. transports/bifrost-http/handlers/config.go — dead-code ErrNotFound guard and HTTP status code choice are worth a quick look before merge. Important Files Changed
Reviews (5): Last reviewed commit: "onboarding widget backend changes" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@framework/configstore/rdb.go`:
- Around line 551-552: The metadata merge is doing a read-modify-write without a
lock in UpdateClientMetadata (and similarly in UpdateClientConfig), so
concurrent calls can overwrite each other or silently be dropped when UPDATE
affects 0 rows; change the read to acquire a row lock (SELECT ... FOR UPDATE)
when loading the singleton row (use GORM row locking via
tx.Clauses(clause.Locking{Strength:"UPDATE"}).First(&existing) or equivalent) to
serialize merges, and after performing the UPDATE check RowsAffected and treat a
zero-row update as a retry/failure path (retry the read-with-lock and merge or
return a clear error) instead of silently proceeding. Ensure the fix is applied
in the code paths using tx.First(&existing) at the shown locations
(UpdateClientMetadata and the similar block at the other location).
- Around line 268-270: The current read of metadata_json into existing via
tx.Select("metadata_json").First(&existing) is not locked and can be stale;
change this to acquire the same row-level lock used elsewhere before copying
metadata (i.e., perform the SELECT with a FOR UPDATE / row-lock clause on
tables.TableClientConfig via the transaction tx when calling First on existing)
so the DELETE+CREATE in this transaction serializes with concurrent
UpdateClientMetadata writes and prevents overwriting newer metadata.
- Around line 570-571: The current code path that does
tx.Create(&tables.TableClientConfig{MetadataJSON: string(data)}) creates a
skeletal config_client row with only metadata_json, which breaks GetMCPConfig's
"no-row => use defaults" behavior; instead, before inserting check that the
primary client config row exists (the same row used by your client-config
initialization path) and only allow metadata-only writes if that main client
config is present, or populate the new tables.TableClientConfig with the same
default fields used by the normal client-config initialization routine so the
inserted row has full default values; update the code around tx.Create(...) to
either validate existence of the main client config or seed all default fields
(and reference GetMCPConfig) so downstream logic continues to use intended
defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d17d0b91-8067-407b-a90a-9b5032fe5907
📒 Files selected for processing (8)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/store.goframework/configstore/tables/clientconfig.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config_test.goui/lib/types/config.ts
akshaydeo
force-pushed
the
05-20-onboarding_widget_backend_changes
branch
from
May 20, 2026 04:59
99e8bd0 to
a027d50
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@framework/configstore/rdb.go`:
- Around line 551-554: The check in the dbForUpdate(...).First(&existing) error
path returns a plain fmt.Errorf which hides the standard "not found" sentinel;
change that return to wrap or return the package's ErrNotFound sentinel (e.g.
return fmt.Errorf("%w: client config must be initialized before metadata can be
updated", ErrNotFound) or simply return ErrNotFound with context) so callers of
the ConfigStore can detect the missing-config case; update the branch that
currently references existing/dbForUpdate to return ErrNotFound instead of a
plain error string.
In `@framework/configstore/tables/clientconfig.go`:
- Around line 203-207: When rehydrating cc.Metadata from cc.MetadataJSON in
TableClientConfig, reset cc.Metadata to an empty map or nil before calling
json.Unmarshal to avoid merging into a previously populated map and retaining
stale keys; update the block that checks cc.MetadataJSON (the code referencing
cc.MetadataJSON and cc.Metadata) to clear or reinitialize cc.Metadata right
before unmarshalling so the resulting map reflects only the JSON payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b0ce748-16b0-4633-b98f-451aa8bea290
📒 Files selected for processing (9)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/clientconfig.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config_test.goui/lib/types/config.ts
akshaydeo
force-pushed
the
05-20-onboarding_widget_backend_changes
branch
from
May 20, 2026 05:33
a027d50 to
51e6ecd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
framework/configstore/tables/clientconfig.go (1)
203-209:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClear
Metadataon emptyMetadataJSON.This hook still leaves a previously populated
cc.Metadataintact when the sameTableClientConfiginstance is reused for a row whosemetadata_jsonis empty, so later reads can surface stale metadata.Suggested fix
- if cc.MetadataJSON != "" { + if cc.MetadataJSON != "" { var metadata map[string]any if err := json.Unmarshal([]byte(cc.MetadataJSON), &metadata); err != nil { return err } cc.Metadata = metadata + } else { + cc.Metadata = nil }🤖 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 `@framework/configstore/tables/clientconfig.go` around lines 203 - 209, The current logic only unmarshals into cc.Metadata when cc.MetadataJSON != "" but does not clear cc.Metadata when MetadataJSON is empty, leaving stale data; update the same hook (the block referencing cc.MetadataJSON and cc.Metadata) to explicitly set cc.Metadata = nil (or an empty map) when cc.MetadataJSON == "" so previous metadata is cleared, and keep the existing json.Unmarshal path unchanged to populate cc.Metadata when JSON is present.
🤖 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 `@transports/bifrost-http/handlers/config.go`:
- Around line 194-198: The handler logs a warning for any error returned by
h.store.ConfigStore.GetClientMetadata (causing noisy logs when
configstore.ErrNotFound is returned on fresh installs); update the error
handling in the GetClientMetadata block to treat configstore.ErrNotFound as
expected (do not call logger.Warn for that case—either skip logging or use a
debug/trace log) and only call logger.Warn("failed to get client metadata...")
for other errors returned by GetClientMetadata.
---
Duplicate comments:
In `@framework/configstore/tables/clientconfig.go`:
- Around line 203-209: The current logic only unmarshals into cc.Metadata when
cc.MetadataJSON != "" but does not clear cc.Metadata when MetadataJSON is empty,
leaving stale data; update the same hook (the block referencing cc.MetadataJSON
and cc.Metadata) to explicitly set cc.Metadata = nil (or an empty map) when
cc.MetadataJSON == "" so previous metadata is cleared, and keep the existing
json.Unmarshal path unchanged to populate cc.Metadata when JSON is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfadbafc-2aab-448b-b5e8-88d827fba548
📒 Files selected for processing (10)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/clientconfig.goframework/configstore/tables/clientconfig_test.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config_test.goui/lib/types/config.ts
✅ Files skipped from review due to trivial changes (2)
- framework/configstore/tables/clientconfig_test.go
- ui/lib/types/config.ts
akshaydeo
force-pushed
the
05-20-onboarding_widget_backend_changes
branch
from
May 20, 2026 05:48
51e6ecd to
3da31f2
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@framework/configstore/rdb.go`:
- Around line 557-567: The current merge loop over `patch` only merges top-level
keys and replaces nested objects (variable `merged`), causing nested metadata
like `onboarding.dismissed` to be lost; implement a recursive helper (e.g.,
`mergeMetadataPatch(dst, patch map[string]any)`) that walks keys, deletes when
patch value is nil, and when both `dst[k]` and `patch[k]` are `map[string]any`
recurses to merge nested maps, otherwise sets `dst[k]=patch[k]`; call this
helper instead of the existing non-recursive loop to preserve nested metadata
during patching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4e778e8-9c72-434d-8cc2-0693f33fb414
📒 Files selected for processing (10)
framework/configstore/migrations.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/clientconfig.goframework/configstore/tables/clientconfig_test.goframework/configstore/tables/config.gotransports/bifrost-http/handlers/config.gotransports/bifrost-http/lib/config_test.goui/lib/types/config.ts
akshaydeo
force-pushed
the
05-20-onboarding_widget_backend_changes
branch
from
May 20, 2026 07:00
3da31f2 to
135a863
Compare
Contributor
Author
Merge activity
|
akshaydeo
added a commit
that referenced
this pull request
May 20, 2026
## Summary
Adds a `metadata_json` column to `config_client` that stores a free-form JSON blob of UI/admin preferences (e.g. `onboarding_dismissed`). This blob is intentionally excluded from the `ClientConfig` API struct so that `config.json` sync can never overwrite it. A new `POST /api/config/metadata` endpoint allows callers to merge patches into the blob, and `GET /api/config` now includes the metadata in its response.
## Changes
- Added `migrationAddClientConfigMetadataColumn` to create the `metadata_json` column on `config_client` with a rollback path.
- Added `MetadataJSON` (persisted) and `Metadata` (virtual) fields to `TableClientConfig`, with `BeforeSave`/`AfterFind` hooks to marshal/unmarshal the JSON blob.
- `UpdateClientConfig` now reads and preserves `MetadataJSON` inside its DELETE+CREATE transaction so config writes never clobber UI preferences.
- Added `GetClientMetadata` and `UpdateClientMetadata` to `RDBConfigStore`. `UpdateClientMetadata` performs a targeted `UPDATE` on `metadata_json` only, merging the patch and removing keys whose value is `nil`.
- Exposed both methods on the `ConfigStore` interface.
- Registered `POST /api/config/metadata` in the HTTP transport; `GET /api/config` now appends the metadata blob to the response when non-empty.
- Added `MetadataKeyOnboardingDismissed` constant as the first typed key in the metadata blob.
- Added `metadata` field to the `BifrostConfig` TypeScript type.
- Added stub implementations of `GetClientMetadata` and `UpdateClientMetadata` to `MockConfigStore` in tests.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [ ] Plugins
- [x] UI (React)
- [ ] Docs
## How to test
```sh
# Core/Transports
go test ./framework/configstore/... ./transports/bifrost-http/...
# Verify migration runs cleanly on a fresh or existing DB by starting the server
# and confirming the metadata_json column exists on config_client.
# Set a metadata key
curl -X POST http://localhost:8080/api/config/metadata \
-H "Content-Type: application/json" \
-d '{"onboarding_dismissed": true}'
# Expected: {"success": true}
# Read it back
curl http://localhost:8080/api/config
# Expected: response includes "metadata": {"onboarding_dismissed": true}
# Clear a key by passing null
curl -X POST http://localhost:8080/api/config/metadata \
-H "Content-Type: application/json" \
-d '{"onboarding_dismissed": null}'
# Expected: {"success": true}, metadata key removed
# UI
cd ui
pnpm i
pnpm build
```
## Breaking changes
- [ ] Yes
- [x] No
## Related issues
## Security considerations
The `POST /api/config/metadata` endpoint is gated by the same middleware chain as the rest of `/api/config`, so it inherits existing auth controls. The metadata blob is a free-form store; callers should avoid writing secrets or PII into it, as it is returned in plaintext via `GET /api/config`.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
Merged
11 tasks
18 tasks
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.
Summary
Adds a
metadata_jsoncolumn toconfig_clientthat stores a free-form JSON blob of UI/admin preferences (e.g.onboarding_dismissed). This blob is intentionally excluded from theClientConfigAPI struct so thatconfig.jsonsync can never overwrite it. A newPOST /api/config/metadataendpoint allows callers to merge patches into the blob, andGET /api/confignow includes the metadata in its response.Changes
migrationAddClientConfigMetadataColumnto create themetadata_jsoncolumn onconfig_clientwith a rollback path.MetadataJSON(persisted) andMetadata(virtual) fields toTableClientConfig, withBeforeSave/AfterFindhooks to marshal/unmarshal the JSON blob.UpdateClientConfignow reads and preservesMetadataJSONinside its DELETE+CREATE transaction so config writes never clobber UI preferences.GetClientMetadataandUpdateClientMetadatatoRDBConfigStore.UpdateClientMetadataperforms a targetedUPDATEonmetadata_jsononly, merging the patch and removing keys whose value isnil.ConfigStoreinterface.POST /api/config/metadatain the HTTP transport;GET /api/confignow appends the metadata blob to the response when non-empty.MetadataKeyOnboardingDismissedconstant as the first typed key in the metadata blob.metadatafield to theBifrostConfigTypeScript type.GetClientMetadataandUpdateClientMetadatatoMockConfigStorein tests.Type of change
Affected areas
How to test
Breaking changes
Related issues
Security considerations
The
POST /api/config/metadataendpoint is gated by the same middleware chain as the rest of/api/config, so it inherits existing auth controls. The metadata blob is a free-form store; callers should avoid writing secrets or PII into it, as it is returned in plaintext viaGET /api/config.Checklist
docs/contributing/README.mdand followed the guidelines