feat(web): SavedFilter model, uninstall purge, and OSearch favorites - #263
Merged
Conversation
- Declare web service entry and handwritten SavedFilter with effective ModelId, IsDefault exclusivity, and SF11 shared ACL. - Purge web_saved_filter on module uninstall only when no surviving MetaModel remains for the logical model (IMD-safe). - Wire OSearch Favorites via useSavedFilters, merge server defaults over code presets, and apply first-frame filters on Kanban/Chart. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds saved-filter persistence and cleanup, web-shell planning controls, cross-application data loading, validation metadata propagation, bootstrap updates, and backend test identity support. It also updates search views, module dependencies, installation flows, and related tests. ChangesSaved filter lifecycle and search integration
Web-shell planning and lifecycle execution
Platform data, validation, and test identity
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant OSearchView
participant OSearch
participant useSavedFilters
participant SavedFilter
User->>OSearchView: open search view
OSearchView->>useSavedFilters: load saved-filter defaults
useSavedFilters->>SavedFilter: query active private and shared defaults
SavedFilter-->>useSavedFilters: saved-filter rows
useSavedFilters-->>OSearchView: merged defaults
OSearchView->>OSearch: provide defaults and await initial query
User->>OSearch: save or apply favorite
OSearch->>useSavedFilters: create, apply, or delete saved filter
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨No code suggestions found for the PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
modules/web/service/models/saved_filter.ts (1)
219-239: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftBack the name uniqueness with a database constraint.
_assertUniqueNamereads and then the caller writes. Two concurrent creates can both pass the check and insert the same name. The check also runs throughthis.Search, so record rules can hide a conflicting row and let a duplicate through.Add a unique index on
(Application, ModelName, UserId, Name)and keep this check for the friendly error message. Note that most engines treatNULLvalues as distinct in a unique index, so shared rows need a normalized sentinel or a partial index to stay covered.🤖 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 `@modules/web/service/models/saved_filter.ts` around lines 219 - 239, Add a database-level unique constraint for Application, ModelName, UserId, and Name, using the project’s schema or migration mechanism. Ensure rows with null UserId are covered by normalizing them to a shared sentinel or using an appropriate partial-index strategy, while retaining _assertUniqueName for the friendly AlreadyExists error.modules/web/service/tests/saved_filter.test.ts (1)
168-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for cross-bucket defaults and for a foreign
UserId.Two behaviors of the model stay untested:
_clearOtherDefaultsscopes the clear to one user bucket. Create a shared default and a private default for the sameApplication/ModelName, then assert both keepIsDefault = true. Today only same-bucket exclusivity is asserted.Createaccepts a caller-suppliedUserId. Add a test that passes another user's id and asserts the expected outcome. This test locks in the fix for the authorization gap raised inmodules/web/service/models/saved_filter.ts.🤖 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 `@modules/web/service/tests/saved_filter.test.ts` around lines 168 - 201, Extend the saved-filter tests to cover cross-bucket defaults by creating shared and private defaults with the same Application and ModelName, then assert both retain IsDefault = true after creation. Add a separate Create test supplying a different user’s UserId and assert the authorization behavior expected by the SavedFilter model.modules/web/service/models/_resolve_effective_model.ts (1)
87-105: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
Idas a stable tiebreaker to the pagination order.
Searchaccepts anOrderByarray. UseUpdatedAt descfollowed byId descto keep offset pages stable when rows share the sameUpdatedAt.🤖 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 `@modules/web/service/models/_resolve_effective_model.ts` around lines 87 - 105, Update the Search call in the pagination loop to use the supported OrderBy array, ordering first by UpdatedAt descending and then by Id descending. Keep the existing pageSize/offset pagination behavior unchanged.
🤖 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 `@internal/module/lifecycle/uninstaller.go`:
- Around line 128-130: Update the uninstaller cleanup flow around the
webSavedFilterTable check to use db.Migrator().GetTables(), propagate its
failure as a wrapped error, and return nil only when web_saved_filter is
confirmed absent. Preserve the existing cleanup behavior when the table exists.
In `@modules/web/service/models/saved_filter.ts`:
- Around line 329-333: Update the UserId normalization block in the saved-filter
update flow to enforce the same owner restriction as the create path before
accepting a caller-supplied value. Reject reassignment of a private favorite to
another user and reject promotion to a shared row, while preserving valid owner
assignments and the existing null/empty normalization.
- Around line 267-276: Unvalidated UserId values can let non-administrators
assign saved filters to other users. In
modules/web/service/models/saved_filter.ts#L267-L276 and `#L329-L333`, add one
shared owner-normalization helper that accepts null or the current actor,
permits other owner IDs only for system administrators, and use it in both
_prepareCreate and _prepareUpdate before assigning values.UserId; replace the
direct String(raw).trim() handling with this helper.
- Around line 419-430: The Delete method must delete only the rows validated by
_assertCanMutateShared: search with required fields overriding options.fields,
validate each returned row, then build a new condition using only their IDs and
the lowercase 'in' operator before calling super.Delete. Return 0 when no rows
pass validation, and avoid reusing the original condition for deletion.
- Around line 385-399: Update the static Update method’s pre-read call to Search
so it passes only the fixed fields selection { fields: ['Id'] }, without
spreading options. Preserve forwarding options to UpdateById for the actual
updates.
In `@modules/web/web/components/view/OKanbanView.vue`:
- Around line 444-449: Update OKanbanView’s onMounted/controller.apply flow to
wait for the first query-update payload whenever searchView is rendered,
ensuring SavedFilter defaults are applied before the initial query. Preserve
immediate mount-time application when no search view exists, and keep onSearch
responsible for recording the first payload without suppressing its emit.
In `@modules/web/web/components/view/search/OSearch.vue`:
- Around line 92-110: Update OSearch’s Favorites menu to expose an authorized
delete control for each favorite, wiring it to the remove operation from
useSavedFilters and reporting any removal failures through the component’s
existing error-handling mechanism. Preserve the current favorite application
behavior and only allow deletion when the user is authorized.
---
Nitpick comments:
In `@modules/web/service/models/_resolve_effective_model.ts`:
- Around line 87-105: Update the Search call in the pagination loop to use the
supported OrderBy array, ordering first by UpdatedAt descending and then by Id
descending. Keep the existing pageSize/offset pagination behavior unchanged.
In `@modules/web/service/models/saved_filter.ts`:
- Around line 219-239: Add a database-level unique constraint for Application,
ModelName, UserId, and Name, using the project’s schema or migration mechanism.
Ensure rows with null UserId are covered by normalizing them to a shared
sentinel or using an appropriate partial-index strategy, while retaining
_assertUniqueName for the friendly AlreadyExists error.
In `@modules/web/service/tests/saved_filter.test.ts`:
- Around line 168-201: Extend the saved-filter tests to cover cross-bucket
defaults by creating shared and private defaults with the same Application and
ModelName, then assert both retain IsDefault = true after creation. Add a
separate Create test supplying a different user’s UserId and assert the
authorization behavior expected by the SavedFilter model.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fde628d-e510-4c88-aeed-70f6ca426244
📒 Files selected for processing (19)
internal/module/lifecycle/uninstaller.gointernal/module/lifecycle/uninstaller_saved_filter_test.gointernal/testing/backend/backend.gomodules/web/package.jsonmodules/web/service/i18n.tsmodules/web/service/index.tsmodules/web/service/models/_resolve_effective_model.tsmodules/web/service/models/index.tsmodules/web/service/models/saved_filter.tsmodules/web/service/tests/saved_filter.test.tsmodules/web/web/components/view/OChartView.vuemodules/web/web/components/view/OKanbanView.vuemodules/web/web/components/view/OSearchView.vuemodules/web/web/components/view/search/OSearch.vuemodules/web/web/composables/search/index.tsmodules/web/web/composables/search/savedFilterDefaults.test.tsmodules/web/web/composables/search/savedFilterDefaults.tsmodules/web/web/composables/search/useSavedFilters.tsmodules/web/web/controllers/chartController.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 19 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
- Keep SF11 write/delete ACL on Update/Delete overrides; use ctx.mode for create because Id is pre-assigned before validation. - Relax ModelId/CreateUid kernel notNull so Constraint writeback can run after required checks. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…z seeds - Auto-include the web SPA shell when planned modules declare entryPoints.web, with CLI --no-web to opt out. - Allow cross-app data seeding so domain modules can own authz packs; keep platform FieldDefault/AppSetting logical defaults in auth and SavedFilter RR in web. - Retarget bootstrap minimal install to meta, harden module depends (web→document/meta, partner*→auth, drop unused task→base), and stabilize unit/e2e harness identity and fixtures. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/module/evolution/data/loader.go (1)
231-234: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winStore the trimmed explicit application value.
Line 231 trims
rec.Application, but Lines 232-234 only assign the normalized value when it is empty. A record with"application": " auth "keeps the whitespace inModelData.Application. This can create an invalid cross-application mapping.Proposed fix
app := strings.TrimSpace(rec.Application) if app == "" { rec.Application = rules.OwnerApp +} else { + rec.Application = app }🤖 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 `@internal/module/evolution/data/loader.go` around lines 231 - 234, Update the application normalization in the record-loading flow to assign the trimmed app value back to rec.Application when it is non-empty, while retaining rules.OwnerApp as the fallback for empty values. Ensure ModelData.Application receives the normalized value for explicit applications.
🧹 Nitpick comments (1)
modules/web/service/models/saved_filter.ts (1)
177-197: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueUniqueness is enforced only in application code.
_assertUniqueNameperforms a read and then the write proceeds. Two concurrent creates with the sameApplication,ModelName,UserId, andNamecan both pass the check. No database unique index backs the rule. Consider adding a unique index over the four columns so the database rejects the duplicate.🤖 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 `@modules/web/service/models/saved_filter.ts` around lines 177 - 197, Add a database-level unique index covering Application, ModelName, UserId, and Name for SavedFilter, while retaining _assertUniqueName for user-facing validation. Ensure the index migration/schema change is included so concurrent writes are rejected by the database.
🤖 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 `@internal/module/artifact/pipeline/pipeline.go`:
- Around line 1065-1103: The ensure-install pipeline must preserve installed
EnsureOrder module names for post-install lifecycle finalization. In
internal/module/artifact/pipeline/pipeline.go lines 1065-1103, carry the ensured
names into the upgrade lifecycle state while retaining existing install
behavior; in internal/module/lifecycle/modulemanager.go lines 1559-1562, process
a stable, deduplicated EnsureOrder plus ModuleOrder sequence for phase-end hooks
and refreshModuleIndexForLocalModules. Add a lifecycle regression test
confirming an ensured module receives both operations.
In `@modules/auth/e2e/switch_company_scope_acceptance.spec.ts`:
- Around line 307-317: Update the polling callback around readAuthTokens and
extractCompanyScopeFromToken so it only succeeds when activeCompanyId is
non-empty and differs from scopeA.activeCompanyId. Preserve the existing
30-second timeout and ensure transient missing-token results do not satisfy the
poll.
In `@modules/auth/service/tests/bootstrap_gift_pack.test.ts`:
- Line 164: Remove the duplicate expected declaration in the bootstrap gift pack
test, keeping a single Array<{ name: string; model: string }> binding within the
block so the module compiles.
In `@pkg/jsengine/scripts/choysumtest/choysumtest.js`:
- Around line 452-490: Update applyDefaultUnitIdentity to assign jsCtx.req.depth
to 0 unconditionally, replacing the conditional numeric-preservation check so
every test starts with the default request depth. Extend
TestChoysumTestScriptReappliesDefaultUnitIdentity to set a nonzero depth in its
first case and assert depth is 0 in the second case.
---
Outside diff comments:
In `@internal/module/evolution/data/loader.go`:
- Around line 231-234: Update the application normalization in the
record-loading flow to assign the trimmed app value back to rec.Application when
it is non-empty, while retaining rules.OwnerApp as the fallback for empty
values. Ensure ModelData.Application receives the normalized value for explicit
applications.
---
Nitpick comments:
In `@modules/web/service/models/saved_filter.ts`:
- Around line 177-197: Add a database-level unique index covering Application,
ModelName, UserId, and Name for SavedFilter, while retaining _assertUniqueName
for user-facing validation. Ensure the index migration/schema change is included
so concurrent writes are rejected by the database.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c7cadf6-0193-4a3d-a58c-78d0dc3962f4
📒 Files selected for processing (38)
.cursor/rules/module-data-seeds.mdcAGENTS.mdcmd/cmd_install.gocmd/cmd_upgrade.gointernal/bootstrap/service/coordinator.gointernal/module/artifact/pipeline/pipeline.gointernal/module/evolution/data/loader.gointernal/module/evolution/data/loader_test.gointernal/module/lifecycle/install_module.gointernal/module/lifecycle/install_prefetch.gointernal/module/lifecycle/modulemanager.gointernal/module/lifecycle/operation_options.gointernal/module/lifecycle/service.gointernal/module/plan/options.gointernal/module/plan/plan.gointernal/module/plan/planner.gointernal/module/plan/planner_test.gointernal/testing/backend/backend.gointernal/testing/backend/unit_identity.gointernal/testing/backend/unit_identity_test.gointernal/testing/e2e/runner.gomodules/README.mdmodules/auth/e2e/switch_company_scope_acceptance.spec.tsmodules/auth/package.jsonmodules/auth/service/tests/bootstrap_gift_pack.test.tsmodules/base/demo/demo.jsonmodules/base/package.jsonmodules/meta/package.jsonmodules/partner/package.jsonmodules/partner_bank/package.jsonmodules/partner_commercial/package.jsonmodules/task/package.jsonmodules/web/data/bootstrap.jsonmodules/web/package.jsonmodules/web/service/models/saved_filter.tsmodules/web/service/tests/saved_filter.test.tspkg/jsengine/scripts/choysumtest/choysumtest.jspkg/jsengine/scripts/choysumtest/script_test.go
💤 Files with no reviewable changes (1)
- modules/meta/package.json
- Restrict SavedFilter UserId to the actor or shared null, and preserve ChoysumError statuses from constraints. - Resolve browser actor identity via auth store; convert nested QueryCondition favorites; include keyword metadata when saving. - Wait for OSearchView defaults before Kanban first load, add favorite delete/retry UI, and harden related harness/e2e edge cases. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 49 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…atus meta - Run upgrade phase-end hooks and module-index refresh for EnsureOrder plus ModuleOrder. - Keep repository validation_failed wrapping while storing ChoysumError cause/gRPC on constraint issues. - Persist trimmed seed application values and avoid CodeQL len-sum allocation capacity. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
You’re at about 92% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/core/service/runtime/validation/engine.ts">
<violation number="1" location="modules/core/service/runtime/validation/engine.ts:584">
P1: Domain constraint responses lose their intended gRPC status and cause code whenever a kernel/platform or earlier constraint issue precedes them. Consider selecting the first status-bearing issue when wrapping the pipeline (and preserving its cause metadata), rather than consulting only `primaryIssue`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
- Grant base.user RoleMethodAccess and RoleFieldRule for web.SavedFilter alongside existing record rules. - Skip web-shell ensuring on uninstall; clear shared defaults without sudo and reject uncleared foreign defaults. - Fix favorites delete ownership/confirm/a11y, refresh server default winner after mutations, and fail closed on unit-identity lookup errors. Co-authored-by: Cursor <cursoragent@cursor.com>
- Replace identity/ctx/req with fresh minimal objects so allowlists and other case-local fields cannot leak across tests. Co-authored-by: Cursor <cursoragent@cursor.com>
…ysumError - Select the first error issue with meta.grpcCode when wrapping repository validation failures so Unauthenticated is not masked by earlier kernel errors. - Break the constraint handler loop after recording a domain ChoysumError so later handlers cannot run side effects. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
All reported issues were addressed across 25 files (changes from recent commits).
You’re at about 93% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Add unit tests for unit identity, web-shell planning, EnsureOrder pipeline, uninstall purge, and bootstrap meta install helpers. - Cover useSavedFilters/OSearch/OSearchView/actorUserId and SavedFilter BE edge cases so Codecov patch lines are exercised. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
modules/web/service/models/_resolve_effective_model.test.ts (1)
7-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the empty factory state.
When
originalisundefined, call an exportedunregisterServiceFactorythat removesmeta.MetaModelfrom the registry.🤖 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 `@modules/web/service/models/_resolve_effective_model.test.ts` around lines 7 - 12, Update withMockedMetaSearch to call the exported unregisterServiceFactory for meta.MetaModel when original is undefined, while retaining the existing registerServiceFactory restoration when original exists. Ensure the mock cleanup restores the registry to its prior empty state.internal/module/lifecycle/install_module_test.go (1)
89-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
SkipWebShellbehavior observable.Both tests pass for any failure. They can pass if
SkipWebShellis lost and planning fails while resolvingweb.
internal/module/lifecycle/install_module_test.go#L89-L97: makeweborigin access observable, then assert that the install reaches the intended later failure without accessingweb.internal/module/lifecycle/service_skip_webshell_test.go#L15-L18: use an observable service or planner path and assert that each request preservesSkipWebShell.🤖 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 `@internal/module/lifecycle/install_module_test.go` around lines 89 - 97, Make SkipWebShell assertions observable in both affected tests: in internal/module/lifecycle/install_module_test.go:89-97, instrument the web origin access and assert it is not accessed while InstallModule reaches the intended later failure; in internal/module/lifecycle/service_skip_webshell_test.go:15-18, use an observable service or planner path and assert every request preserves SkipWebShell. Do not accept arbitrary failures as passing.internal/bootstrap/service/coordinator_minimal_install_test.go (1)
43-61: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the expected failure deterministic.
This test calls the real
defaultInstallMinimalModulesand relies on the executor or the install failing in the harness. That failure is environment-dependent. If executor construction later succeeds, the test starts a real module install, waits for the install timeout, and then fails.Override
newMinimalInstallExecutorandinstallMinimalModulesFnwith a stub that returns a fixed error, asTestDefaultInstallMinimalModulesClassifiesInstallErrordoes. Keep the assertion onsnap.StageDetail.🤖 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 `@internal/bootstrap/service/coordinator_minimal_install_test.go` around lines 43 - 61, Make TestDefaultInstallMinimalModulesMarksMetaStage deterministic by overriding newMinimalInstallExecutor and installMinimalModulesFn with a stub that returns a fixed error, matching the setup used in TestDefaultInstallMinimalModulesClassifiesInstallError. Ensure defaultInstallMinimalModules receives the stubbed failure without running a real install, while preserving the existing snap.StageDetail assertion.modules/core/service/runtime/validation/engine.ts (1)
574-593: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated and unnarrowed
ChoysumErrorhandling in both constraint catch blocks. The static-handler and instance-handler catch blocks contain identical code. Both attach cause metadata and thenbreakfor everyChoysumError, although the comment states the intent is only domain auth/status failures. A plain domainInvalidArgumenttherefore hides the remaining constraint issues, and any future change must be applied twice.
modules/core/service/runtime/validation/engine.ts#L574-L593: extract a shared helper, for examplebuildConstraintFailureIssue(handler, error), that returns the issue and a stop flag. Set the stop flag only forGrpcCode.UnauthenticatedandGrpcCode.PermissionDenied.modules/core/service/runtime/validation/engine.ts#L636-L655: replace this block with a call to the same helper so both paths stay in sync.🤖 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 `@modules/core/service/runtime/validation/engine.ts` around lines 574 - 593, In modules/core/service/runtime/validation/engine.ts, extract the duplicated failure construction from the constraint catch blocks at lines 574-593 and 636-655 into a shared helper such as buildConstraintFailureIssue(handler, error) that returns the issue and stop flag. Preserve ChoysumError metadata, but set the stop flag only for GrpcCode.Unauthenticated or GrpcCode.PermissionDenied; plain domain errors must allow remaining handlers to run. Replace both catch-block implementations with the shared helper.
🤖 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 `@modules/core/service/orm/repository/validation/error_helpers.ts`:
- Around line 9-29: Update issueGrpcCode so zero-valued grpcCode metadata is
treated as absent, allowing wrapRepositoryValidationError to fall back to
GrpcCode.InvalidArgument; continue accepting only finite, nonzero numeric status
codes.
In `@modules/web/service/tests/saved_filter.test.ts`:
- Around line 154-160: Update the error assertion helper around
collectErrorCodes so messageHint is checked only after the expected code is
present, never as an alternative. Require codes.includes(code) first, then
validate messageHint when provided, while preserving the existing failure
message for missing or mismatched errors.
---
Nitpick comments:
In `@internal/bootstrap/service/coordinator_minimal_install_test.go`:
- Around line 43-61: Make TestDefaultInstallMinimalModulesMarksMetaStage
deterministic by overriding newMinimalInstallExecutor and
installMinimalModulesFn with a stub that returns a fixed error, matching the
setup used in TestDefaultInstallMinimalModulesClassifiesInstallError. Ensure
defaultInstallMinimalModules receives the stubbed failure without running a real
install, while preserving the existing snap.StageDetail assertion.
In `@internal/module/lifecycle/install_module_test.go`:
- Around line 89-97: Make SkipWebShell assertions observable in both affected
tests: in internal/module/lifecycle/install_module_test.go:89-97, instrument the
web origin access and assert it is not accessed while InstallModule reaches the
intended later failure; in
internal/module/lifecycle/service_skip_webshell_test.go:15-18, use an observable
service or planner path and assert every request preserves SkipWebShell. Do not
accept arbitrary failures as passing.
In `@modules/core/service/runtime/validation/engine.ts`:
- Around line 574-593: In modules/core/service/runtime/validation/engine.ts,
extract the duplicated failure construction from the constraint catch blocks at
lines 574-593 and 636-655 into a shared helper such as
buildConstraintFailureIssue(handler, error) that returns the issue and stop
flag. Preserve ChoysumError metadata, but set the stop flag only for
GrpcCode.Unauthenticated or GrpcCode.PermissionDenied; plain domain errors must
allow remaining handlers to run. Replace both catch-block implementations with
the shared helper.
In `@modules/web/service/models/_resolve_effective_model.test.ts`:
- Around line 7-12: Update withMockedMetaSearch to call the exported
unregisterServiceFactory for meta.MetaModel when original is undefined, while
retaining the existing registerServiceFactory restoration when original exists.
Ensure the mock cleanup restores the registry to its prior empty state.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b1f38ec-2471-41d0-9554-f503479c32cd
📒 Files selected for processing (54)
AGENTS.mdinternal/bootstrap/service/coordinator.gointernal/bootstrap/service/coordinator_minimal_install_test.gointernal/module/artifact/pipeline/pipeline_test.gointernal/module/evolution/data/loader.gointernal/module/evolution/data/loader_test.gointernal/module/lifecycle/install_module_test.gointernal/module/lifecycle/merge_unique_module_names_test.gointernal/module/lifecycle/modulemanager.gointernal/module/lifecycle/modulemanager_logging_test.gointernal/module/lifecycle/operation_options_test.gointernal/module/lifecycle/service_skip_webshell_test.gointernal/module/lifecycle/uninstaller.gointernal/module/lifecycle/uninstaller_saved_filter_test.gointernal/module/plan/options_test.gointernal/module/plan/planner.gointernal/module/plan/planner_test.gointernal/testing/backend/backend.gointernal/testing/backend/unit_identity.gointernal/testing/backend/unit_identity_test.gomodules/auth/e2e/switch_company_scope_acceptance.spec.tsmodules/core/service/orm/repository/validation/bridge.tsmodules/core/service/orm/repository/validation/error_helpers.tsmodules/core/service/orm/repository/validation/index.tsmodules/core/service/orm/repository/validation/tests/error_helpers.test.tsmodules/core/service/runtime/validation/engine.test.tsmodules/core/service/runtime/validation/engine.tsmodules/web/data/bootstrap.jsonmodules/web/service/models/_resolve_effective_model.test.tsmodules/web/service/models/_resolve_effective_model.tsmodules/web/service/models/saved_filter.tsmodules/web/service/tests/saved_filter.test.tsmodules/web/web/components/view/OChartView.vuemodules/web/web/components/view/OKanbanView.firstframe.test.tsmodules/web/web/components/view/OKanbanView.readonly.test.tsmodules/web/web/components/view/OKanbanView.vuemodules/web/web/components/view/OSearchView.test.tsmodules/web/web/components/view/OSearchView.vuemodules/web/web/components/view/kanbanFirstFrame.test.tsmodules/web/web/components/view/kanbanFirstFrame.tsmodules/web/web/components/view/search/OSearch.behavior.test.tsmodules/web/web/components/view/search/OSearch.vuemodules/web/web/composables/search/actorUserId.test.tsmodules/web/web/composables/search/actorUserId.tsmodules/web/web/composables/search/savedFilterDefaults.test.tsmodules/web/web/composables/search/savedFilterDefaults.tsmodules/web/web/composables/search/useSavedFilters.test.tsmodules/web/web/composables/search/useSavedFilters.tsmodules/web/web/controllers/chartController.test.tsmodules/web/web/controllers/chartController.tsmodules/web/web/query/utils/filter/structures.test.tsmodules/web/web/query/utils/filter/structures.tspkg/jsengine/scripts/choysumtest/choysumtest.jspkg/jsengine/scripts/choysumtest/script_test.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/module/lifecycle/uninstaller.go
- modules/auth/e2e/switch_company_scope_acceptance.spec.ts
- modules/web/service/models/_resolve_effective_model.ts
- internal/testing/backend/unit_identity.go
- AGENTS.md
- pkg/jsengine/scripts/choysumtest/choysumtest.js
- modules/web/web/composables/search/savedFilterDefaults.test.ts
- internal/module/evolution/data/loader.go
- modules/web/web/controllers/chartController.ts
- modules/web/web/components/view/OSearchView.vue
- modules/web/web/composables/search/savedFilterDefaults.ts
- modules/web/web/components/view/OChartView.vue
- internal/module/evolution/data/loader_test.go
- modules/web/service/models/saved_filter.ts
- modules/web/web/composables/search/useSavedFilters.ts
- modules/web/web/components/view/search/OSearch.vue
- pkg/jsengine/scripts/choysumtest/script_test.go
There was a problem hiding this comment.
All reported issues were addressed across 31 files (changes from recent commits).
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
- Always rebuild choysumtest identity/ctx for auth-free cases and drop dead Symbol cleanup. - Preflight shared-default clear ACL, split SavedFilter RMAs by method, and treat missing auth.User as not found. - Reject non-canonical meta.grpcCode for status selection; probe uninstall table existence without swallowing DB errors. - Deduplicate Kanban first search, race-guard OSearchView default loads, reuse mergeSavedFilterDefaults, and style delete confirm as danger. Co-authored-by: Cursor <cursoragent@cursor.com>
- Stub minimal meta install stage marking and assert SkipWebShell skips web resolve. - Require expectCode to match error codes before message hints; unregister mocked MetaModel factories. - Key identity/company_main and planner ensure-load failures off intent, not call counts; assert OSearch codeDefaults shapes. Co-authored-by: Cursor <cursoragent@cursor.com>
 |
|
Overall Grade Focus Area: Reliability |
Security  Reliability  Complexity Hygiene |
Feedback
Type looseness in tests
- The
anyand non-null assertions are both ways the tests sidestep the type system, spread across a lot of files. - Because these tests sit around the new saved filter / search behavior, making them stricter would give you better confidence that the behavior holds up as the feature evolves.
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Go |  |
Aug 10, 2026 4:20a.m. | Review ↗ |
| JavaScript | |
Aug 10, 2026 4:20a.m. | Review ↗ |
| Python |  |
Aug 10, 2026 4:20a.m. | Review ↗ |
| Shell | |
Aug 10, 2026 4:20a.m. | Review ↗ |
| Secrets | |
Aug 10, 2026 4:20a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
- Require error code before message hints and throw descriptive failures (choysumtest expect ignores a second arg). - Match SF11 stranger write/delete assertions to "violates record rule" instead of the old OR-hint "record_rule_denied". Co-authored-by: Cursor <cursoragent@cursor.com>
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
- Rename clear helper that shadowed the predeclared clear identifier. - Mark unused callback parameters as blank identifiers in planner/pipeline EnsureOrder tests. - Stop passing nil into WithOperationOptions while keeping an explicit nil FromContext contract check. Co-authored-by: Cursor <cursoragent@cursor.com>
- Rename unused fakeResolver peek/load name parameters to _ in uninstall and dependency error cases. Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover web_saved_filter probe/non-table DB errors and Upgrade SkipWebShell option wiring. - Hit Upgrade module-index refresh failure, auth.User lookup wrap, and unit identity context errors in RunOneAppBackendTests. Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover Kanban first-frame lastSearchPayload skip and falsy onSearch. - Cover chartController appliedFilters clear-flag branch matrix. - Extend SavedFilter BE cases for whitespace UserId, shared-default replace, and rename uniqueness. - Fill OSearchView, OSearch, useSavedFilters, defaults, structures, and effective-model partials. Co-authored-by: Cursor <cursoragent@cursor.com>
- Stub clearOtherDefaults sudo paths and exercise constraint null defaults. - Cover useSavedFilters/OSearch/OSearchView || and stale-gen arms. - Yield before empty app/model clear so superseding loads are race-safe. - Hit resolveEffectiveModel missing timestamps and null Search pages. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (5)
internal/module/lifecycle/service_upgrade_test.go (1)
17-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
SkipWebShellreaches the operation context.This test passes if
UpgradeignoresSkipWebShell. The missing-module error does not expose the applied operation options. Capture the planner or lifecycle context and assertOperationOptions.SkipWebShellistrue.🤖 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 `@internal/module/lifecycle/service_upgrade_test.go` around lines 17 - 24, Update the Upgrade test to capture the planner or lifecycle context used by svc.Upgrade, then assert the captured OperationOptions.SkipWebShell value is true. Preserve the existing missing-module failure assertion while ensuring the test verifies that SkipWebShell reaches the operation context rather than merely being accepted by the request.modules/web/web/components/view/OSearchView.test.ts (1)
259-279: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake this race assertion discriminating.
Line 260 uses
mockResolvedValue, so every load returns the sameKeeprow. The assertion at line 278 then passes in two different cases: the supersede guard skipped the empty-app clear, or the clear happened and the newer load re-fetched the identical row. The test cannot detect a regression in the guard.Return a distinct row for the newer load, and assert that value.
♻️ Distinguish the two loads
- sfSearch.mockResolvedValue([ - { Id: 'p1', Name: 'Keep', Condition: {}, IsDefault: true, UserId: 'me' }, - ]); + sfSearch.mockResolvedValueOnce([ + { Id: 'p1', Name: 'Keep', Condition: {}, IsDefault: true, UserId: 'me' }, + ]); const store = makeStore(); @@ store.application = 'demo'; + sfSearch.mockResolvedValueOnce([ + { Id: 'p2', Name: 'Newer', Condition: {}, IsDefault: true, UserId: 'me' }, + ]); await wrapper.find('.emit-defaults-ready').trigger('click'); await flushPromises(); await nextTick(); - expect(JSON.parse(wrapper.find('.defaults').text())[0].name).toBe('Keep'); + expect(JSON.parse(wrapper.find('.defaults').text())[0].name).toBe('Newer');🤖 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 `@modules/web/web/components/view/OSearchView.test.ts` around lines 259 - 279, Update the sfSearch mock in the “skips clearing defaults when a newer load supersedes empty app/model” test to return the existing Keep row for the initial load and a distinct row for the newer load, using sequential results or an equivalent implementation. Change the final defaults assertion to require the newer row’s distinct value, so the test fails if the empty-app clear is incorrectly allowed to win.modules/web/web/components/view/search/OSearch.behavior.test.ts (1)
624-643: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThis test cannot detect a swapped checkbox binding.
Lines 633-634 set both checkboxes to
true, and line 638 asserts bothisDefaultandsharedaretrue. If the template swaps theisDefaultandsharedbindings, the payload is identical and the test still passes. Line 632 also confirms only the count, not the identity, of each checkbox.Toggle one checkbox at a time, or select each checkbox by a stable class instead of by index.
♻️ Assert each checkbox binding separately
const checks = wrapper.findAll('input[type="checkbox"]'); expect(checks.length).toBeGreaterThanOrEqual(2); await checks[0]!.setValue(true); - await checks[1]!.setValue(true); const saveBtn = wrapper.findAll('.el-btn').find(b => b.text() === 'Save'); await saveBtn!.trigger('click'); await flushPromises(); expect(savedFiltersApi.saveCurrent).toHaveBeenCalledWith({ name: 'SharedDef', isDefault: true, - shared: true, + shared: false, });Add a second test that sets only
checks[1]and asserts{ isDefault: false, shared: true }.🤖 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 `@modules/web/web/components/view/search/OSearch.behavior.test.ts` around lines 624 - 643, Strengthen the checkbox binding coverage in the test around “saves with isDefault and shared checkboxes enabled” by adding a separate case that enables only the second checkbox and expects the save payload to contain isDefault: false and shared: true. Prefer stable checkbox selectors when available, or otherwise preserve the existing checkbox targeting while ensuring the test distinguishes the two bindings.modules/web/web/components/view/OKanbanView.firstframe.test.ts (1)
150-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnmount every wrapper returned by
mount.These tests leave four
OKanbanViewinstances mounted. The third test also leavesawaitFieldSelectionMockpending indefinitely. Store each wrapper and callwrapper.unmount()after its assertions.beforeEachalready resetsdeferStateand clears the shared mocks.🤖 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 `@modules/web/web/components/view/OKanbanView.firstframe.test.ts` around lines 150 - 204, Update the affected OKanbanView tests to retain each wrapper returned by mount and call wrapper.unmount() after assertions, including the cases using SyncEmitSearch and FalsyEmitSearch. Ensure the test with the indefinitely pending awaitFieldSelectionMock is unmounted so its component instance and pending work are cleaned up.modules/web/service/tests/saved_filter.test.ts (1)
917-976: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore inherited static methods without binding them
sudoandUpdateare inherited fromBaseModel. The bound-function assignments infinallycreate own properties onSavedFilterand pinthistoSavedFilter. Preserve the original ownership state, then delete the stub properties when the methods were inherited.🤖 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 `@modules/web/service/tests/saved_filter.test.ts` around lines 917 - 976, Update the test’s cleanup around SavedFilter.sudo and SavedFilter.Update to preserve whether each method was originally inherited from BaseModel rather than binding inherited methods. Restore the original functions without binding, and delete the temporary own-property stubs in finally when the methods were not originally defined directly on SavedFilter.
🤖 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.
Nitpick comments:
In `@internal/module/lifecycle/service_upgrade_test.go`:
- Around line 17-24: Update the Upgrade test to capture the planner or lifecycle
context used by svc.Upgrade, then assert the captured
OperationOptions.SkipWebShell value is true. Preserve the existing
missing-module failure assertion while ensuring the test verifies that
SkipWebShell reaches the operation context rather than merely being accepted by
the request.
In `@modules/web/service/tests/saved_filter.test.ts`:
- Around line 917-976: Update the test’s cleanup around SavedFilter.sudo and
SavedFilter.Update to preserve whether each method was originally inherited from
BaseModel rather than binding inherited methods. Restore the original functions
without binding, and delete the temporary own-property stubs in finally when the
methods were not originally defined directly on SavedFilter.
In `@modules/web/web/components/view/OKanbanView.firstframe.test.ts`:
- Around line 150-204: Update the affected OKanbanView tests to retain each
wrapper returned by mount and call wrapper.unmount() after assertions, including
the cases using SyncEmitSearch and FalsyEmitSearch. Ensure the test with the
indefinitely pending awaitFieldSelectionMock is unmounted so its component
instance and pending work are cleaned up.
In `@modules/web/web/components/view/OSearchView.test.ts`:
- Around line 259-279: Update the sfSearch mock in the “skips clearing defaults
when a newer load supersedes empty app/model” test to return the existing Keep
row for the initial load and a distinct row for the newer load, using sequential
results or an equivalent implementation. Change the final defaults assertion to
require the newer row’s distinct value, so the test fails if the empty-app clear
is incorrectly allowed to win.
In `@modules/web/web/components/view/search/OSearch.behavior.test.ts`:
- Around line 624-643: Strengthen the checkbox binding coverage in the test
around “saves with isDefault and shared checkboxes enabled” by adding a separate
case that enables only the second checkbox and expects the save payload to
contain isDefault: false and shared: true. Prefer stable checkbox selectors when
available, or otherwise preserve the existing checkbox targeting while ensuring
the test distinguishes the two bindings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e680d2d-033f-4aa2-adba-abcae8c084c9
📒 Files selected for processing (16)
internal/module/lifecycle/module_index_sync_test.gointernal/module/lifecycle/service_upgrade_test.gointernal/module/lifecycle/uninstaller_saved_filter_test.gointernal/module/plan/planner_test.gointernal/testing/backend/backend_injected_test.gointernal/testing/backend/unit_identity_test.gomodules/web/service/models/_resolve_effective_model.test.tsmodules/web/service/tests/saved_filter.test.tsmodules/web/web/components/view/OKanbanView.firstframe.test.tsmodules/web/web/components/view/OSearchView.test.tsmodules/web/web/components/view/OSearchView.vuemodules/web/web/components/view/search/OSearch.behavior.test.tsmodules/web/web/composables/search/savedFilterDefaults.test.tsmodules/web/web/composables/search/useSavedFilters.test.tsmodules/web/web/controllers/chartController.test.tsmodules/web/web/query/utils/filter/structures.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- modules/web/web/query/utils/filter/structures.test.ts
- modules/web/web/composables/search/savedFilterDefaults.test.ts
- modules/web/web/components/view/OSearchView.vue
- internal/module/lifecycle/uninstaller_saved_filter_test.go
- internal/module/plan/planner_test.go
- internal/testing/backend/unit_identity_test.go
- Exercise falsy create Id so currentId uses the empty-string fallback. - Race empty-app defaults clear without awaiting the first click. - Cover private canDelete with falsy non-null UserId via || ''. Co-authored-by: Cursor <cursoragent@cursor.com>
- Observe SkipWebShell on Upgrade Peek context instead of a vacuous miss. - Tighten OSearchView stale-clear race and OSearch checkbox bindings. - Unmount Kanban first-frame wrappers and restore SavedFilter stubs cleanly. - Use 0750 for the module-index upgrade test mkdir DeepSource flagged. Co-authored-by: Cursor <cursoragent@cursor.com>
- Drop unused receiver names on stub OriginCoordinator methods. Co-authored-by: Cursor <cursoragent@cursor.com>
- Drop the lazy metaModelService wrapper; web already depends on meta. - Use createServiceByModel<typeof MetaModel> at module scope like auth. - Stub MetaModel.Search in unit tests instead of swapping the factory. Co-authored-by: Cursor <cursoragent@cursor.com>
- Drop the one-line _actorId helper; trim this.userId at call sites. Co-authored-by: Cursor <cursoragent@cursor.com>
- Replace Go LookupEffectiveModel with First by application+name at loader, bootstrap, and unit identity call sites. - Inline MetaModel/MetaApplication Search limit 1 in auth ACL and SavedFilter; delete TS resolveEffective copies and pick-matrix tests. Co-authored-by: Cursor <cursoragent@cursor.com>
- Skip syncing draft active/enabled from JWT metadata while the switcher popover is open so refreshToken cannot clear a pending selection. - Harden Apply readiness assertions in company-switch E2E helpers. Co-authored-by: Cursor <cursoragent@cursor.com>
- Add ScopeKey (normalized path) so Name uniqueness and IsDefault mutex are per route bucket. - Prefill favorite Name from stable term src and filter list/create/defaults by current ScopeKey. Co-authored-by: Cursor <cursoragent@cursor.com>
- Cover normalizeScopeKey, default favorite name, and OSearch/OSearchView ScopeKey wiring. - Add OSwitchCompany open-panel draft guard and method-access meta lookup edge cases. Co-authored-by: Cursor <cursoragent@cursor.com>
- Extract trySetupHook for full branch coverage and cover defaultFavoriteName ||/?? edges. - Exercise SavedFilter empty-string owner bucket and ScopeKey merge on update. Co-authored-by: Cursor <cursoragent@cursor.com>
- Call _clearOtherDefaults with no identity userId so this.userId || '' is covered. Co-authored-by: Cursor <cursoragent@cursor.com>
This was referenced Aug 10, 2026
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.
User description
Summary
modules/webentryPoints.serviceand add handwrittenweb.SavedFilter(effectiveModelId, IsDefault exclusivity, SF11 shared write/delete). SF13 expects emptyweb_field_default/web_app_settingafter install.cleanModels, hard-deleteweb_saved_filteronly when a logical model has no remaining livemeta_model(IMD-safe; table missing = no-op).useSavedFilters/ Save dialog, merge defaults (private > shared > code), and stop Kanban/Chart from dropping first-frame filter applies.Test plan
go test ./internal/module/lifecycle/ -run 'SavedFilter|PurgesSaved|KeepsSaved|MissingTable'./choysum test unit web --be --pattern 'SavedFilter|SF13|SF11'./choysum test unit web --fe --pattern 'savedFilterDefaults|OSearch'./choysum test typecheck webweb, confirm tablesweb_saved_filter+web_field_default+web_app_setting; save/apply a Favorite in List searchMade with Cursor
PR Type
Enhancement, Tests
Description
Go Core Changes: Add
purgeSavedFiltersForGoneModelsinuninstaller.goto purgeweb_saved_filterrows when no survivingmeta_modelremains upon module uninstallation.TypeScript Module Changes (
modules/web): DeclareentryPoints.serviceand implement handwrittenweb.SavedFiltermodel featuring dynamicModelIdresolution,IsDefaultexclusivity, and shared favorite ACLs (SF11/SF12/SF13).Web UI & Search Component: Wire OSearch favorites with
useSavedFilters, merging server-defined defaults over code presets, and fix Kanban and Chart views from dropping initial frame filter applies.License & SPDX Compliance: All new TypeScript source files under
modules/web/service/andmodules/web/web/composables/search/includeApache-2.0SPDX headers; new Go test file includesLGPL-3.0-or-laterheader.Test Coverage: Added Go lifecycle uninstall tests (
internal/module/lifecycle/uninstaller_saved_filter_test.go), TypeScript backend tests (modules/web/service/tests/saved_filter.test.ts), and Vitest frontend unit tests (savedFilterDefaults.test.ts).File Walkthrough
13 files
Purge saved filter entries on module uninstall when models are removedAdd i18n translation helper for web serviceExport web service model definitionsResolve effective MetaModel ID from application and model nameRe-export SavedFilter model in web serviceImplement SavedFilter model with ACL and default exclusivity logicPass applied search filters to chart controllerLoad server-merged filter defaults before initial search emitAdd favorites dropdown menu and save filter modal dialogExport saved filters composables and default filter helpersImplement merge logic for private, shared, and code default filtersAdd composable for managing saved favorites lifecycleSupport appliedFilters parameter override in chart controller4 files
Add Go lifecycle tests for saved filter cleanup on uninstallEnsure meta module installation during web backend testsAdd TS backend unit tests for SavedFilter operationsAdd Vitest unit tests for saved filter default merging1 files
Register service entrypoint for web module1 files
Prevent Kanban view from ignoring first-frame search appliesSummary by CodeRabbit
--no-weboptions for installation and upgrades.