feat(scoped-query): introduce useScopedQuery for scope-based API access control #5754
Conversation
This file contains 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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 5 Skipped Deployments
|
|
🎉 @WANZARGEN and @seungyeoneeee have been randomly selected as the reviewers! Please review. 🙏 |
There was a problem hiding this comment.
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
apps/web/src/services/dashboards/composables/use-dashboard-query.ts:70
- [nitpick] Removing explicit generic type parameters may affect type inference. Please verify that the new useScopedQuery implementation correctly infers types for queries.
const publicDashboardListQuery = useScopedQuery({
apps/web/src/services/dashboards/composables/use-dashboard-query.ts:79
- [nitpick] Ensure that removing explicit generic parameters does not lead to issues with type inference for the private dashboard query.
const privateDashboardListQuery = useScopedQuery({
apps/web/src/query/composables/tests/use-scoped-query.test.ts:24
- The test mock sets globalGrantLoading to true, causing isAppReady to be false and disabling the query. To test enabled queries properly, consider setting globalGrantLoading to false.
useAppContextStore: () => ({ getters: { globalGrantLoading: true } }),
apps/web/src/api-clients/_common/composables/use-scoped-query.ts:1
- Ensure that all external references to the old useScopedQuery have been updated to the new implementation to prevent potential runtime errors.
// Removal of the old useScopedQuery file
WANZARGEN
requested changes
Apr 8, 2025
| const { | ||
| queryKey, enabled, currentScope, requiredScopes, isAppReady, | ||
| } = params; | ||
| if (!enabled || !isAppReady || !currentScope) return; |
There was a problem hiding this comment.
issue: It doesn't warn in case that scope is invalid.
WANZARGEN
reviewed
Apr 8, 2025
|
|
||
| if (_warnedKeys.has(key)) return; | ||
|
|
||
| _warnedKeys.add(key); |
There was a problem hiding this comment.
suggestion: Using Symbol for Warning Keys Management
Summary
Consider using Symbol to manage warning keys separately per context instead of using a global Set.
Current Implementation
const _warnedKeys = new Set<string>();Suggested Implementation
const createWarnedKeysSymbol = () => Symbol('warnedKeys');
export const useScopedQuery = <T,...>(...) => {
const WARNED_KEYS = createWarnedKeysSymbol();
const warnedKeys = new Set<string>();
(globalThis as any)[WARNED_KEYS] = warnedKeys;
// ... rest of the implementation
}Benefits
- Isolates warning state per hook instance
- Prevents warning key collisions across different components/contexts
- More accurate warning tracking for identical query keys used in different contexts
- Better memory management as warning sets can be garbage collected with their contexts
Considerations
- Slightly increased memory usage due to multiple Sets
- Need to ensure proper cleanup of global Symbol references
- May want to consider WeakMap as an alternative for automatic garbage collection
Impact
Low risk, medium effort change that improves debugging accuracy and maintainability.
Testing Recommendations
- Verify warnings work independently across different components
- Check memory usage patterns
- Ensure no warning leaks between different hook instances
What are your thoughts on this approach?
There was a problem hiding this comment.
[Korean]
경고 키(Warning Keys) 관리를 위한 Symbol 사용 제안
요약
현재 전역 Set으로 관리되는 경고 키를 Symbol을 사용하여 컨텍스트별로 분리하여 관리하는 방안을 제안드립니다.
현재 구현
const _warnedKeys = new Set<string>();제안 구현
const createWarnedKeysSymbol = () => Symbol('warnedKeys');
export const useScopedQuery = <T,...>(...) => {
const WARNED_KEYS = createWarnedKeysSymbol();
const warnedKeys = new Set<string>();
(globalThis as any)[WARNED_KEYS] = warnedKeys;
// ... 나머지 구현
}장점
- 훅 인스턴스별로 경고 상태 격리
- 서로 다른 컴포넌트/컨텍스트 간 경고 키 충돌 방지
- 동일한 쿼리 키가 다른 컨텍스트에서 사용될 때 더 정확한 경고 추적 가능
- 컨텍스트와 함께 가비지 컬렉션이 가능하여 메모리 관리가 개선됨
고려사항
- 여러 Set 사용으로 인한 약간의 메모리 사용량 증가
- 전역 Symbol 참조에 대한 적절한 정리 필요
- 자동 가비지 컬렉션을 위해 WeakMap 사용도 대안으로 고려 가능
영향도
위험도는 낮고, 구현 노력은 중간 정도이며, 디버깅 정확성과 유지보수성이 향상됩니다.
테스트 권장사항
- 서로 다른 컴포넌트에서 경고가 독립적으로 작동하는지 확인
- 메모리 사용 패턴 확인
- 서로 다른 훅 인스턴스 간 경고 누수가 없는지 확인
이 접근 방식에 대해 어떻게 생각하시나요?
WANZARGEN
requested changes
Apr 8, 2025
|
|
||
| if (_warnedKeys.has(key)) return; | ||
|
|
||
| _warnedKeys.add(key); |
There was a problem hiding this comment.
[Korean]
경고 키(Warning Keys) 관리를 위한 Symbol 사용 제안
요약
현재 전역 Set으로 관리되는 경고 키를 Symbol을 사용하여 컨텍스트별로 분리하여 관리하는 방안을 제안드립니다.
현재 구현
const _warnedKeys = new Set<string>();제안 구현
const createWarnedKeysSymbol = () => Symbol('warnedKeys');
export const useScopedQuery = <T,...>(...) => {
const WARNED_KEYS = createWarnedKeysSymbol();
const warnedKeys = new Set<string>();
(globalThis as any)[WARNED_KEYS] = warnedKeys;
// ... 나머지 구현
}장점
- 훅 인스턴스별로 경고 상태 격리
- 서로 다른 컴포넌트/컨텍스트 간 경고 키 충돌 방지
- 동일한 쿼리 키가 다른 컨텍스트에서 사용될 때 더 정확한 경고 추적 가능
- 컨텍스트와 함께 가비지 컬렉션이 가능하여 메모리 관리가 개선됨
고려사항
- 여러 Set 사용으로 인한 약간의 메모리 사용량 증가
- 전역 Symbol 참조에 대한 적절한 정리 필요
- 자동 가비지 컬렉션을 위해 WeakMap 사용도 대안으로 고려 가능
영향도
위험도는 낮고, 구현 노력은 중간 정도이며, 디버깅 정확성과 유지보수성이 향상됩니다.
테스트 권장사항
- 서로 다른 컴포넌트에서 경고가 독립적으로 작동하는지 확인
- 메모리 사용 패턴 확인
- 서로 다른 훅 인스턴스 간 경고 누수가 없는지 확인
이 접근 방식에 대해 어떻게 생각하시나요?
…ss control Signed-off-by: piggggggggy <[email protected]>
Signed-off-by: piggggggggy <[email protected]>
Signed-off-by: piggggggggy <[email protected]>
Signed-off-by: piggggggggy <[email protected]>
Signed-off-by: piggggggggy <[email protected]>
Signed-off-by: piggggggggy <[email protected]>
Signed-off-by: piggggggggy <[email protected]>
piggggggggy
force-pushed
the
feature-scoped-query-composable
branch
from
74d8915 to
0520c37
Compare
…rotask Signed-off-by: piggggggggy <[email protected]>
piggggggggy
commented
Apr 12, 2025
There was a problem hiding this comment.
Revise dev warning logger for useScopedQuery, addressing both DX and conceptual clarity.
- Why not use getCurrentInstance()
- getCurrentInstance() is designed for accessing Vue’s internal component context.
- It does not reflect a query’s declaration location, making it conceptually unsuitable.
- If the goal were to deduplicate logs per invocation, Symbol would be a more appropriate and consistent mechanism.
- Why Symbol wasn’t selected
- Symbol-based logging fits a “per-call” model (each invocation is distinct),
- But our goal was to log only once per declaration, not every time the query runs.
- Final approach: Error.stack + microtask tick
- Uses Error().stack to infer the exact declaration point (file + line)
- Uses queueMicrotask() to cache the warning for one tick
- ✅ Prevents noisy duplicates
- ✅ Allows re-logging on future re-entries
- ✅ Aligns with a declarative model of usage
useScopedQuery 내 개발용 경고 로그 시스템을 다음과 같은 이유로 개선하였습니다:
- 인스턴스 기반 방식 (getCurrentInstance) — ❌ 사용하지 않음
- 초기에는 getCurrentInstance()를 활용하여 컴포넌트 기준으로 로깅을 캐싱하려 했지만,
- 이 API는 Vue의 내부 렌더링 컨텍스트나 라이프사이클 접근용이며,
- 선언 위치 기반 로깅이라는 목적과는 거리가 있습니다.
- 같은 목적이라면 Symbol을 통한 호출 단위 캐싱이 더 일관된 선택입니다.
- Symbol 기반 (시도) — ❌ 채택하지 않음
- Symbol은 호출마다 고유한 값이 생성되므로, **“호출 시마다 로깅”**에 적합하지만,
- 우리가 의도한 것은 **“선언 위치당 1회만 로깅”**하는 구조였기 때문에 부합하지 않았습니다.
- 최종 선택: Error.stack + tick 기반 캐싱 — ✅ 채택
- Error().stack을 통해 쿼리 선언 위치를 추출해 고유 key 생성
- queueMicrotask()를 이용해 이벤트 루프 단위로 로그 캐싱
- → 동일 tick 내 중복 로그 방지
- → 다음 tick에서 다시 로깅 가능 (페이지 재진입 대응)
There was a problem hiding this comment.
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
apps/web/src/api-clients/_common/composables/use-scoped-query.ts:1
- Ensure that no parts of the codebase rely on the old useScopedQuery import path; all references should be updated to use '@/query/composables/use-scoped-query'.
// Deprecated useScopedQuery implementation removed
apps/web/src/query/query-key/_types/query-key-type.ts:4
- Verify that treating QueryKeyArray as immutable does not break any parts of the code that expect to modify the query key arrays.
export type QueryKeyArray = readonly unknown[];
apps/web/src/services/dashboards/composables/use-dashboard-query.ts:70
- [nitpick] Consider providing explicit generic type arguments for useScopedQuery if TypeScript inference does not correctly capture the desired types for the query function.
const publicDashboardListQuery = useScopedQuery({
Signed-off-by: samuel.park <[email protected]>
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.
Skip Review (optional)
style,chore,ci,test,docs)Description (optional)
Summary
This PR introduces a new composable useScopedQuery, a wrapper around useQuery that enforces scope-based API execution logic for our product.
Why this is needed
Our product uses API-level scopes to control access permissions. However, these scopes are only loosely coupled with route-level contexts.
Without explicit enforcement, queries may execute before the correct scope is granted, leading to authorization errors or broken UX.
To solve this, useScopedQuery ensures:
Features
요약
스코프 기반 API 실행 제어를 강제하는 useQuery 래퍼 훅인 useScopedQuery를 도입합니다.
왜 필요한가요?
우리 서비스는 API Scope을 통해 리소스 접근 권한을 제어합니다. 하지만 이 Scope는 Route Scope와 약하게만 연결되어 있어,
올바른 Scope가 세팅되기 전에 Query가 먼저 실행되는 시점 문제가 발생할 수 있습니다.
이를 방지하기 위해 useScopedQuery는 다음을 보장합니다:
주요 기능
Things to Talk About (optional)