-
Notifications
You must be signed in to change notification settings - Fork 5
Develop #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Develop #84
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,11 @@ | ||
| import { type GetConfigResult } from '@/models/v4/configs/getConfigResult'; | ||
| import { type GetSystemConfigResult } from '@/models/v4/configs/getSystemConfigResult'; | ||
|
|
||
| import { createCachedApiEndpoint } from '../common/cached-client'; | ||
| import { createApiEndpoint } from '../common'; | ||
|
|
||
| const getConfigApi = createCachedApiEndpoint('/Config/GetConfig', { | ||
| ttl: 60 * 1000 * 1440, // Cache for 1 days | ||
| enabled: false, | ||
| }); | ||
| const getConfigApi = createApiEndpoint('/Config/GetConfig'); | ||
|
|
||
| const getSystemConfigApi = createCachedApiEndpoint('/Config/GetSystemConfig', { | ||
| ttl: 60 * 1000 * 1440, // Cache for 1 days | ||
| enabled: false, | ||
| }); | ||
| const getSystemConfigApi = createApiEndpoint('/Config/GetSystemConfig'); | ||
|
Comment on lines
+6
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainVerify performance implications of removing caching. The removal of cached endpoint construction eliminates TTL-based caching for configuration data. Since config data typically changes infrequently, this could lead to unnecessary API calls if these endpoints are invoked frequently (e.g., on navigation or component remounts). Please verify:
Run the following script to identify usage patterns: 🏁 Script executed: #!/bin/bash
# Description: Find all usages of getConfig and getSystemConfig to assess call frequency
# Search for direct function calls
rg -nP --type=ts --type=tsx -C3 '\b(getConfig|getSystemConfig)\s*\(' -g '!**/node_modules/**' -g '!**/*.test.*'
# Search for imports to understand consumption patterns
rg -nP --type=ts --type=tsx 'from.*@/api/config' -g '!**/node_modules/**' -g '!**/*.test.*'Length of output: 55 🏁 Script executed: #!/bin/bash
# Description: Find all usages of getConfig and getSystemConfig to assess call frequency
# Using glob patterns instead of --type parameter to avoid filetype issues
# Search for direct function calls in ts and tsx files
rg -nP -C3 '\b(getConfig|getSystemConfig)\s*\(' -g '*.ts' -g '*.tsx' -g '!**/node_modules/**' -g '!**/*.test.*'
# Search for imports from the config module
rg -nP 'from.*@/api/config' -g '*.ts' -g '*.tsx' -g '!**/node_modules/**' -g '!**/*.test.*'Length of output: 1057 🏁 Script executed: #!/bin/bash
# Description: Examine the core-store implementation to understand state caching behavior
# View the initialization and fetchConfig methods
sed -n '75,205p' src/stores/app/core-store.ts
# Also check if there are any selectors or hooks that consume this store
rg -nP 'useAppStore|useShallow|subscribe' src/stores/app/core-store.ts -A2 -B2Length of output: 4407 🏁 Script executed: #!/bin/bash
# Description: Check for react-query usage and fetchConfig invocation patterns
# Search for react-query useQuery hooks related to config
rg -nP 'useQuery|useMutation' -g '*.ts' -g '*.tsx' | head -20
# Find all places where fetchConfig is called
rg -nP 'fetchConfig\s*\(' -g '*.ts' -g '*.tsx' -C2
# Check if react-query is in package.json
grep -i 'react-query\|@tanstack/react-query' package.jsonLength of output: 1312 🏁 Script executed: #!/bin/bash
# Description: Verify if there's any alternative caching/TTL mechanism in place
# Check the createApiEndpoint function to see if it has any built-in caching
rg -nP 'createApiEndpoint|createCachedApiEndpoint' -g '*.ts' -g '*.tsx' -B3 -A5 | head -40
# Look at the actual endpoint creation in the config file
cat src/api/config/index.tsLength of output: 3008 Restore caching for configuration endpoints or implement react-query hooks per coding guidelines. The removal of Zustand state caching alone does not prevent duplicate concurrent API calls. Additionally, your coding guidelines mandate using react-query for data fetching and caching, which is not currently applied here. Fix: Either restore 🤖 Prompt for AI Agents |
||
|
|
||
| export const getConfig = async (key: string) => { | ||
| const response = await getConfigApi.get<GetConfigResult>({ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Fix JSON syntax error and verify API compatibility.
There's a trailing comma after the last field in the JSON object (line 405), which will cause the jq command to fail.
Apply this fix:
PAYLOAD=$(jq -n \ --arg version "$VERSION" \ --arg date "$RELEASE_DATE" \ --argjson notes "$NOTES_JSON" \ --arg repo "${{ github.repository }}" \ --arg commit "${{ github.sha }}" \ --arg actor "${{ github.actor }}" \ --arg run_url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" \ '{ "version": $version, "title": ("Release v" + $version), - "content": $notes, + "content": $notes }')Additionally, the payload structure has been significantly simplified, removing metadata fields like
date,repo,commit,actor, andrun_url. Verify that the Changerawr API accepts this new format and doesn't require these fields.I'll help verify this review comment. Let me inspect the actual workflow file and validate the claims.
🏁 Script executed:
Length of output: 0
🏁 Script executed:
Length of output: 862
🏁 Script executed:
Length of output: 65
🏁 Script executed:
Length of output: 71
🏁 Script executed:
Length of output: 148
🏁 Script executed:
Length of output: 1576
Fix JSON syntax error and verify API compatibility.
The trailing comma on line 405 (after
"content": $notes,) is a confirmed syntax error that will cause jq to fail with a parse error. The diff provided is correct and necessary.Additionally, the metadata fields (
date,repo,commit,actor,run_url) are defined as jq arguments but excluded from the final JSON payload. Verify that the Changerawr API endpoint accepts this simplified payload format and does not require these fields for proper operation.PAYLOAD=$(jq -n \ --arg version "$VERSION" \ --arg date "$RELEASE_DATE" \ --argjson notes "$NOTES_JSON" \ --arg repo "${{ github.repository }}" \ --arg commit "${{ github.sha }}" \ --arg actor "${{ github.actor }}" \ --arg run_url "${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" \ '{ "version": $version, "title": ("Release v" + $version), - "content": $notes, + "content": $notes }')🤖 Prompt for AI Agents