Open Service: Add internal property to control visibility#35057
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
internal property to control visibility
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds internal visibility control to the open-service framework. Services and individual operations can be marked ChangesInternal visibility and naming enforcement
🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/shared/open-service/service-registration.test.ts (1)
228-230: ⚡ Quick winStrengthen
_resetruntime-access assertion with an actual state transition.This currently passes even if
_resetis a no-op, because state is already0. Set a non-zero value first, then assert_resetbrings it back to0.Suggested test adjustment
- expect(service.queries._getInternalValue(undefined)).toBe(0); - await service.commands._reset(undefined); - expect(service.queries.getValue(undefined)).toBe(0); + await service.commands.setValue(5); + expect(service.queries._getInternalValue(undefined)).toBe(5); + await service.commands._reset(undefined); + expect(service.queries.getValue(undefined)).toBe(0);Based on learnings, tests should validate observable side effects rather than relying on implementation-neutral initial state.
🤖 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 `@code/core/src/shared/open-service/service-registration.test.ts` around lines 228 - 230, The test currently verifies _reset by observing state already 0, so it would pass if _reset is a no-op; modify the test to first change the service state to a non-zero value using an available command (e.g., call the public command that mutates state such as service.commands.increment or service.commands.setValue — whichever exists in this service) and assert the internal value is non-zero via service.queries._getInternalValue (or getValue) before invoking service.commands._reset(undefined); then assert service.queries.getValue(undefined) (and _getInternalValue) equals 0 after _reset to ensure a real state transition occurred.
🤖 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 `@code/core/src/shared/open-service/service-registration.test.ts`:
- Around line 228-230: The test currently verifies _reset by observing state
already 0, so it would pass if _reset is a no-op; modify the test to first
change the service state to a non-zero value using an available command (e.g.,
call the public command that mutates state such as service.commands.increment or
service.commands.setValue — whichever exists in this service) and assert the
internal value is non-zero via service.queries._getInternalValue (or getValue)
before invoking service.commands._reset(undefined); then assert
service.queries.getValue(undefined) (and _getInternalValue) equals 0 after
_reset to ensure a real state transition occurred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f844b2f0-05db-46dc-a2c4-02888f17f71a
📒 Files selected for processing (7)
code/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/fixtures.tscode/core/src/shared/open-service/index.test-d.tscode/core/src/shared/open-service/service-definition.tscode/core/src/shared/open-service/service-registration.test.tscode/core/src/shared/open-service/service-registration.tscode/core/src/shared/open-service/types.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
There was a problem hiding this comment.
I'd say keep both internal and underscore prefix
- Using
internalas property is good for dropping such services/queries from runtime code. Checking for property is faster than doingname.startsWith("_") - Enforcing
_prefix gives clear intention of the query. Personally I don't care if it's underscore or what, as long as there is something that implies its intention. Too bad#isn't allowed outside classes.
I would even go as far as drop internal queries from typings of services that are discoverable by user.
This probably won't have any practical impact unless we have 20K services.
I had the same thought initially, but I feared it would be to cumbersome for the code that would actually consume these private operations. They would have to do type casts, which I would fear could hide issues from TS if they ever drift. |
Use explicit state casts so registration handlers can assign string values to nullable state fields. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve open-service conflicts by porting internal visibility filtering into service-registry.ts after next replaced service-registration.ts. Co-authored-by: Cursor <cursoragent@cursor.com>
Closes #
What I did
Added
internalvisibility metadata to open-service definitions so services can be hidden fromlistServices()and queries/commands can be hidden fromdescribeService()without disabling runtime access or typings.Also added TypeScript-only
_prefix enforcement for internal operations and updated open-service registration/type tests plus maintainer docs.Alternatively, we could also skip the
internalflag completely and rely solely on the operation name. So if the query/command starts with_, it is implicitly hidden from descriptions. For services as a whole, if theiridis prefixed_the same could happen. Open to both solutions.Example
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
No manual test is necessary. Maintainers can verify with:
yarn test open-serviceDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
_prefix naming convention.Documentation
Tests