refactor(planner-data): switch to skygame-data cdn

[imnaiyar]

PR Type

Enhancement

---

Description

• Switch data source from sky-planner.com to unpkg.com CDN

• Update BASE_URL to use skygame-data npm package

• Simplify DATA_PATH from /assets/data to /assets

---

Diagram Walkthrough

flowchart LR
  old["sky-planner.com<br/>/assets/data"]
  new["unpkg.com/skygame-data<br/>/assets"]
  old -- "refactor" --> new

Loading

File Walkthrough

	Relevant files
Configuration changes	fetcher.ts Migrate to skygame-data npm package CDN                                    packages/constants/src/skygame-planner/fetcher.ts Updated BASE_URL from https://sky-planner.com to https://unpkg.com/skygame-data@latest Changed DATA_PATH from /assets/data to /assets Migrated data fetching to use npm package CDN instead of custom domain +2/-2

---

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments

Project	Deployment	Preview	Comments	Updated (UTC)
sky-helper-docs	Ignored	Preview		Nov 21, 2025 7:38pm
website-next	Ignored	Preview		Nov 21, 2025 7:38pm

[qodo-free-for-open-source-projects]

PR Compliance Guide 🔍

(Compliance updated until commit e91d6b6)

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Unsanitized input parsing Description: The parseGuidSet function splits user-controlled input without sanitization, which could lead to injection attacks if GUIDs contain malicious characters or are used in unsafe contexts downstream. data.service.ts [334-336] Referred Code static parseGuidSet(value?: string): Set<string> { if (!value || value.length === 0) return new Set(); return new Set(value.split(",").filter((s) => s.length > 0));
	Supply chain dependency risk Description: The new CDN source (unpkg.com) introduces a supply chain security risk as it relies on an external npm package that could be compromised or contain malicious code, affecting all users who fetch data from this source. fetcher.ts [1-1] Referred Code import type { SkyHelper } from "@/structures"; import { Collection } from "@discordjs/collection";
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
🟢	No codebase code duplication found New Components Detected (Top 5): - fetchSkyData - resolveData - transformEmojis - CostUtils - costToEmoji
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

---

Previous compliance checks

Compliance check up to commit a01921d

Security Compliance
🔴	Unpinned dependency version Description: Using @latest tag in production CDN URL creates supply chain risk as package updates are automatically pulled without version pinning, potentially introducing malicious code or breaking changes. fetcher.ts [23-23] Referred Code export const BASE_URL = `https://unpkg.com/skygame-data@latest`; const DATA_PATH = "/assets";
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
🟢	No codebase code duplication found No new components were introduced in the PR code
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: CDN Version Pinning: Using @latest tag in CDN URL may introduce supply chain risks as package updates are automatically fetched without validation. Referred Code export const BASE_URL = `https://unpkg.com/skygame-data@latest`; const DATA_PATH = "/assets"; Learn more about managing compliance generic rules or creating your own custom rules

[qodo-free-for-open-source-projects]

PR Code Suggestions ✨

Latest suggestions up to e1e938c

Category	Suggestion	Impact
Possible issue	Refund currencies when locking items Refund currencies when locking an item in toggleItemUnlock to maintain data consistency. packages/skyhelper/src/bot/handlers/planner/helpers/action.utils.ts [115-122] if (!unlock) { // Lock item and its nodes user.plannerData.unlocked = PlannerDataService.removeFromGuidString( user.plannerData.unlocked, item.guid, ...(item.nodes?.map((n) => n.guid) ?? []), ); + // Refund currencies for locked nodes + const { seasonGuid, eventGuid } = getCurrencyContext(undefined, item); + item.nodes?.forEach((node) => { + adjustCurrencies(user, node, true, seasonGuid, eventGuid); + }); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies a critical bug where locking an item does not refund the associated currencies, leading to an inconsistent user data state.	High
	Pin CDN URL to specific version Pin the version in the ITEMS_DATA_URL from 0.x.x to a specific version like 0.2.0 to prevent unexpected breaking changes from the CDN. scripts/sync.items.emojis.mts [12] -const ITEMS_DATA_URL = "https://unpkg.com/skygame-data@0.x.x/assets/items.json"; +const ITEMS_DATA_URL = "https://unpkg.com/skygame-data@0.2.0/assets/items.json"; [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential for future breakages by using a wildcard version in a CDN URL and proposes pinning it to a specific version, which improves the script's stability.	Medium
	Validate data parameter before processing Add a null check for the data parameter in resolveProgress to prevent potential runtime errors. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [53-55] static resolveProgress(data: ISkyData, userData?: UserPlannerData) { + if (!data) throw new Error('Invalid data: data cannot be null or undefined'); if (!userData) return data; const d = this.deepClone(data); Apply / Chat Suggestion importance[1-10]: 6 __ Why: This suggestion adds a useful defensive check to prevent potential runtime errors if null or undefined data is passed, improving the function's robustness.	Low
General	Trim whitespace from parsed GUIDs Trim whitespace from each GUID in parseGuidSet to prevent matching failures caused by extraneous spaces. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [349-352] static parseGuidSet(value?: string): Set<string> { if (!value || value.length === 0) return new Set(); - return new Set(value.split(",").filter((s) => s.length > 0)); + return new Set(value.split(",").map((s) => s.trim()).filter((s) => s.length > 0)); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: This is a valuable improvement for data integrity, as it prevents subtle bugs from whitespace in GUID strings, making the GUID parsing logic more robust.	Medium
Learned best practice	Validate non-empty input arrays Add an early return check if the nodes array is empty to avoid unnecessary processing and database updates. This prevents marking the user data as modified when no actual changes occur. packages/skyhelper/src/bot/handlers/planner/helpers/action.utils.ts [282-304] export function unlockAllTreeNodes(user: UserSchema, nodes: INode[]) { + if (nodes.length === 0) return; + user.plannerData ??= PlannerDataService.createEmpty(); const guidsToAdd: string[] = []; nodes.forEach((node) => { if (node.item?.unlocked) return; guidsToAdd.push(node.guid); if (node.item) guidsToAdd.push(node.item.guid); node.hiddenItems?.forEach((item) => guidsToAdd.push(item.guid)); const { seasonGuid, eventGuid } = getCurrencyContext(node, node.item); adjustCurrencies(user, node, false, seasonGuid, eventGuid); }); user.plannerData.unlocked = PlannerDataService.addToGuidString(user.plannerData.unlocked, ...guidsToAdd); user.plannerData.date = new Date().toISOString(); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - When unlocking nodes, validate that the nodes array is not empty before processing to prevent unnecessary database operations and ensure data integrity.	Low
	Handle fetch errors gracefully Wrap the fetch and parse operations in a try-catch block to handle network or parsing errors gracefully. On error, return the stale cache if available rather than throwing, and log the error for monitoring. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [23-33] if (!cachedData || cacheExpired || force) { - const fetchedData = await fetch(URL).then((res) => res.text()); - const parsed = SkyDataResolver.parse(fetchedData); - client.applicationEmojis = await client.api.applications - .getEmojis(process.env.CLIENT_ID) - .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); - cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); - lastFetchTime = now; + try { + const fetchedData = await fetch(URL).then((res) => res.text()); + const parsed = SkyDataResolver.parse(fetchedData); + client.applicationEmojis = await client.api.applications + .getEmojis(process.env.CLIENT_ID) + .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); + cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); + lastFetchTime = now; + } catch (error) { + console.error('Failed to fetch sky data:', error); + if (cachedData) { + console.warn('Returning stale cached data due to fetch failure'); + return cachedData; + } + throw new Error('Failed to fetch sky data and no cache available'); + } } return cachedData; Apply / Chat Suggestion importance[1-10]: 5 __ Why: Relevant best practice - When fetching critical data with caching, wrap the fetch operation in a try-catch block and implement proper error handling to prevent the cache from being left in an invalid state.	Low
More

---

Previous suggestions

Suggestions up to commit e1e938c

Category	Suggestion	Impact
Possible issue	Trim whitespace from parsed GUIDs In parseGuidSet, trim whitespace from each GUID after splitting the string to prevent lookup failures caused by spaces. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [349-352] static parseGuidSet(value?: string): Set<string> { if (!value || value.length === 0) return new Set(); - return new Set(value.split(",").filter((s) => s.length > 0)); + return new Set(value.split(",").map((s) => s.trim()).filter((s) => s.length > 0)); } Suggestion importance[1-10]: 6 __ Why: This is a valid suggestion that improves the robustness of GUID parsing by handling potential whitespace, preventing subtle bugs from malformed GUID strings.	Low
Learned best practice	Add error handling for data fetching Add error handling with retry logic for the fetch operation to handle transient network failures gracefully. Consider wrapping the fetch in a try-catch block and implementing exponential backoff for retries. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [23-32] if (!cachedData || cacheExpired || force) { - const fetchedData = await fetch(URL).then((res) => res.text()); - const parsed = SkyDataResolver.parse(fetchedData); - client.applicationEmojis = await client.api.applications - .getEmojis(process.env.CLIENT_ID) - .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); - cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); - lastFetchTime = now; + try { + const fetchedData = await fetch(URL).then((res) => { + if (!res.ok) throw new Error(`HTTP ${res.status}: ${res.statusText}`); + return res.text(); + }); + const parsed = SkyDataResolver.parse(fetchedData); + client.applicationEmojis = await client.api.applications + .getEmojis(process.env.CLIENT_ID) + .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); + cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); + lastFetchTime = now; + } catch (error) { + if (cachedData) { + console.error('Failed to refresh sky data, using cached version:', error); + } else { + throw new Error('Failed to fetch sky data and no cache available'); + } + } } Suggestion importance[1-10]: 6 __ Why: Relevant best practice - When fetching critical data with caching, implement error handling and retry logic to ensure the application can recover from transient network failures.	Low

Suggestions up to commit 08e7267

Category	Suggestion	Impact
Incremental [*]	Fix tree index selection logic Fix the tree index selection logic by using Math.max(treeIndex, 0) instead of Math.min(treeIndex, 0) to ensure a valid index is always selected. packages/skyhelper/src/bot/handlers/planner/spirits/spirits.ts [133-137] if (this.state.i?.startsWith("tree")) { const treeGuid = this.state.i.slice(4); const treeIndex = trees.findIndex((tt) => tt.tree.guid === treeGuid); - selected = Math.min(treeIndex, 0); + selected = Math.max(treeIndex, 0); } Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a logical error in calculating the selected index, which would cause incorrect behavior or errors, and provides the correct fix.	Medium
	Verify breaking export changes handled Verify that the removal of multiple exports from packages/utils/src/classes/index.ts does not break dependent code. packages/utils/src/classes/index.ts [1-2] export * from "./shardsUtil.js"; export * from "./SkytimesUtils.js"; +// Deprecated exports removed - ensure all imports are updated: +// - LeaderboardCard, GameWinnerCard, SpiritTreeRenderer, +// - SpiritTreeTierRenderer, PlannerStatsCard Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies that removing several exports from packages/utils/src/classes/index.ts is a significant breaking change. It rightly prompts the developer to verify that this refactoring has been handled correctly across the codebase, which is a crucial check for a large PR like this.	Medium
	Prevent mutation of default state In PlannerDataService.createEmpty(), return a deeply frozen object using Object.freeze to prevent accidental mutations of the default empty state. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [78] export class PlannerDataService { static createEmpty(): UserPlannerData { - return { - items: {}, - spirits: {}, - seasons: {}, - events: {}, - }; + return Object.freeze({ + items: Object.freeze({}), + spirits: Object.freeze({}), + seasons: Object.freeze({}), + events: Object.freeze({}), + }); } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential shared state mutation issue and proposes a robust solution using Object.freeze. While the createEmpty() method is used as a Mongoose default, which typically clones the object, freezing it is a good defensive programming practice.	Low
	Improve DateTime instance detection Refine the deepClone method to use a more robust check for DateTime instances by verifying the constructor name instead of using a fragile duck-typing check. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [14-27] private static deepClone<T>(obj: T, visited = new WeakMap()): T { // Handle primitives and null if (obj === null || typeof obj !== "object") return obj; // Preserve special class instances - // instanceof check may not hold true bcz data is from another package - // DateTime - if (typeof obj === "object" && "toMillis" in obj) return obj as T; + // Check for DateTime by constructor name since instanceof may not work across packages + if (typeof obj === "object" && obj.constructor?.name === "DateTime") return obj as T; if (obj instanceof Date) return new Date(obj.getTime()) as T; // Check if we've already cloned this object (circular reference) if (visited.has(obj as object)) { return visited.get(obj as object) as T; } ... } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a fragile duck-typing check for DateTime instances and proposes a more robust solution, improving the reliability of the deepClone utility.	Low
Possible issue	Prevent null return value Add a null check for cachedData before returning from fetchSkyData to ensure the function never returns null, or throw an error if the cache fails to initialize. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [20-34] export async function fetchSkyData(client: SkyHelper, force = false) { const now = Date.now(); const cacheExpired = now - lastFetchTime > CACHE_LIFETIME_MS; if (!cachedData || cacheExpired || force) { const fetchedData = await fetch(URL).then((res) => res.text()); const parsed = SkyDataResolver.parse(fetchedData); // refresh emojis too in-case any emoji was updated client.applicationEmojis = await client.api.applications .getEmojis(process.env.CLIENT_ID) .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); lastFetchTime = now; } + if (!cachedData) { + throw new Error('Failed to initialize sky data cache'); + } return cachedData; } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential null return that contradicts the function's implicit return type, improving robustness by ensuring the function always returns valid data or throws an error.	Medium
General	Optimize currency context retrieval In unlockAllTreeNodes, move the getCurrencyContext call out of the forEach loop to avoid redundant calls, as all nodes in a tree share the same context. packages/skyhelper/src/bot/handlers/planner/helpers/action.utils.ts [299] -const { seasonGuid, eventGuid } = getCurrencyContext(node, node.item); +export function unlockAllTreeNodes(user: UserSchema, nodes: INode[]) { + user.plannerData ??= PlannerDataService.createEmpty(); + const guidsToAdd: string[] = []; + + // Get currency context from the first node (all nodes in tree should have same context) + const firstNode = nodes[0]; + const { seasonGuid, eventGuid } = firstNode ? getCurrencyContext(firstNode, firstNode.item) : {}; + + nodes.forEach((node) => { + if (node.item?.unlocked) return; + + guidsToAdd.push(node.guid); + if (node.item) guidsToAdd.push(node.item.guid); + + // Add hidden items + node.hiddenItems?.forEach((item) => guidsToAdd.push(item.guid)); + + // Spend currency for this node + adjustCurrencies(user, node, false, seasonGuid, eventGuid); + }); + user.plannerData.unlocked = PlannerDataService.addToGuidString(user.plannerData.unlocked, ...guidsToAdd); + user.plannerData.date = new Date().toISOString(); +} + Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a performance regression where getCurrencyContext is called inside a loop, while the context could be determined once outside the loop, as was done in the previous version of the code.	Low
	Avoid mutating accumulator in reduce Refactor calculateRemainingCosts to avoid mutating the accumulator in reduce by returning a new object for each iteration. packages/skyhelper/src/bot/handlers/planner/helpers/cost.utils.ts [33-41] static calculateRemainingCosts(nodes: INode[]) { return nodes.reduce((acc, node) => { if (!(node.item?.unlocked ?? false)) { - Object.keys(acc).forEach((k) => ((acc as any)[k] += (node as any)[k] ?? 0)); + return { + c: acc.c + (node.c ?? 0), + h: acc.h + (node.h ?? 0), + sc: acc.sc + (node.sc ?? 0), + sh: acc.sh + (node.sh ?? 0), + ac: acc.ac + (node.ac ?? 0), + ec: acc.ec + (node.ec ?? 0), + }; } return acc; }, { ...Empty_Cost }); - } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly points out that mutating the accumulator in a reduce function is not a best practice and proposes a functionally equivalent, immutable approach, which improves code quality and predictability.	Low
	Validate items array is non-empty Add a validation check to ensure the items array from the fetched data is not empty before proceeding with processing. scripts/sync.items.emojis.mts [35-46] let data: ItemsData | null = null; try { const res = await fetch(ITEMS_DATA_URL); if (!res.ok) { const body = await res.text().catch(() => ""); throw new Error(`Failed to fetch items.json: HTTP ${res.status} ${res.statusText}${body ? ` - ${body.slice(0, 200)}` : ""}`); } const text = await res.text(); data = jsonc.parse(text) as ItemsData | null; - if (!data || !Array.isArray(data.items)) { - throw new Error("Parsed items.json is invalid or missing 'items' array"); + if (!data || !Array.isArray(data.items) || data.items.length === 0) { + throw new Error("Parsed items.json is invalid, missing 'items' array, or contains no items"); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly proposes adding a check for an empty items array, which is a good practice for robustness, but it's a minor improvement.	Low

✅ Suggestions up to commit e91d6b6

Category	Suggestion	Impact
Incremental [*]	Restore prerequisite node unlocking logic Restore the logic to unlock all prerequisite nodes when a node is unlocked. The current implementation only unlocks the selected node, not its parents in the tree. packages/skyhelper/src/bot/handlers/planner/helpers/action.utils.ts [164-174] if (!node.item?.unlocked) { guidsToAdd.push(node.guid); node.unlocked = true; // Spend currency for this node adjustCurrencies(user, node, false, seasonGuid, eventGuid); } if (node.item) toggleItemUnlock(user, node.item, true); // Unlock hidden items node.hiddenItems?.forEach((item) => toggleItemUnlock(user, item, true)); +// Also unlock previous nodes +let currentNode = node; +while (currentNode.prev) { + if (!currentNode.prev.item?.unlocked) { + guidsToAdd.push(currentNode.prev.guid); + currentNode.prev.unlocked = true; + adjustCurrencies(user, currentNode.prev, false, seasonGuid, eventGuid); + } + if (currentNode.prev.item) toggleItemUnlock(user, currentNode.prev.item, true); + currentNode.prev.hiddenItems?.forEach((item) => toggleItemUnlock(user, item, true)); + currentNode = currentNode.prev; +} + Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical regression where the logic to unlock prerequisite nodes was removed, which would break the spirit tree progression.	High
	Add error handling for fetch Add a try...catch block to the fetchSkyData function to handle potential errors during the fetch operation, returning cached data if available or throwing a specific error. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [20-34] export async function fetchSkyData(client: SkyHelper, force = false) { const now = Date.now(); const cacheExpired = now - lastFetchTime > CACHE_LIFETIME_MS; if (!cachedData || cacheExpired || force) { - const fetchedData = await fetch(URL).then((res) => res.text()); - const parsed = SkyDataResolver.parse(fetchedData); - // refresh emojis too in-case any emoji was updated - client.applicationEmojis = await client.api.applications - .getEmojis(process.env.CLIENT_ID) - .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); - cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); - lastFetchTime = now; + try { + const fetchedData = await fetch(URL).then((res) => res.text()); + const parsed = SkyDataResolver.parse(fetchedData); + // refresh emojis too in-case any emoji was updated + client.applicationEmojis = await client.api.applications + .getEmojis(process.env.CLIENT_ID) + .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); + cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); + lastFetchTime = now; + } catch (error) { + if (cachedData) { + console.error('Failed to fetch sky data, using cached version:', error); + return cachedData; + } + throw new Error('Failed to fetch sky data and no cache available'); + } } return cachedData; } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a lack of error handling for a network request, and the proposed change improves application stability by preventing unhandled promise rejections.	Medium
Possible issue	Prevent concurrent cache refresh operations Implement a promise-based locking mechanism for the cache to prevent race conditions from multiple concurrent requests trying to refresh the cache simultaneously. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [11-13] let cachedData: ISkyData | null = null; let lastFetchTime = 0; +let fetchPromise: Promise<ISkyData> | null = null; const CACHE_LIFETIME_MS = 24 * 60 * 60 * 1000; +export async function fetchSkyData(client: SkyHelper, force = false) { + const now = Date.now(); + const cacheExpired = now - lastFetchTime > CACHE_LIFETIME_MS; + + if (!cachedData || cacheExpired || force) { + if (fetchPromise) { + return fetchPromise; + } + + fetchPromise = (async () => { + const fetchedData = await fetch(URL).then((res) => res.text()); + const parsed = SkyDataResolver.parse(fetchedData); + client.applicationEmojis = await client.api.applications + .getEmojis(process.env.CLIENT_ID) + .then((cmds) => new Collection(cmds.items.map((c) => [c.id, c]))); + cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); + lastFetchTime = now; + fetchPromise = null; + return cachedData; + })(); + + return fetchPromise; + } + return cachedData; +} + Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential race condition with concurrent cache refreshes and proposes a valid promise-based locking mechanism to prevent it, significantly improving the robustness of the caching logic.	Medium
	Validate fetched data before parsing Add a validation check to ensure the fetched data is not empty before attempting to parse it, which prevents potential errors from invalid API responses. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [24-25] const fetchedData = await fetch(URL).then((res) => res.text()); +if (!fetchedData || fetchedData.trim().length === 0) { + throw new Error('Fetched data is empty or invalid'); +} const parsed = SkyDataResolver.parse(fetchedData); Suggestion importance[1-10]: 6 __ Why: The suggestion improves robustness by adding a check for empty or invalid data from the fetch request, preventing potential parsing errors.	Low
Learned best practice	Trim whitespace from parsed GUIDs Trim whitespace from each GUID after splitting to ensure correct parsing and prevent matching errors caused by leading or trailing spaces. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [334-337] static parseGuidSet(value?: string): Set<string> { if (!value || value.length === 0) return new Set(); - return new Set(value.split(",").filter((s) => s.length > 0)); + return new Set(value.split(",").map(s => s.trim()).filter((s) => s.length > 0)); } Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Validate and sanitize user-provided data before processing to prevent injection or parsing errors.	Low
General	Avoid mutating shared node objects Avoid directly mutating the node.unlocked property. State should be managed centrally and derived from user.plannerData to prevent unintended side effects. packages/skyhelper/src/bot/handlers/planner/helpers/action.utils.ts [164-170] if (!node.item?.unlocked) { guidsToAdd.push(node.guid); - node.unlocked = true; + // Note: node.unlocked mutation removed - state should be tracked via user.plannerData only // Spend currency for this node adjustCurrencies(user, node, false, seasonGuid, eventGuid); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a direct mutation of a derived data object, which is poor practice; the state should be managed centrally in user.plannerData.	Low
	Avoid mutating accumulator in reduce Refactor the calculateCosts method to avoid mutating the accumulator in the reduce function by creating a new object in each iteration. packages/skyhelper/src/bot/handlers/planner/helpers/cost.utils.ts [25-30] static calculateCosts(nodes: INode[]): NonNullable<ICost> { - return nodes.reduce((acc, node) => { - Object.keys(acc).forEach((k) => ((acc as any)[k] += (node as any)[k] ?? 0)); - return acc; - }, { ...Empty_Cost }); + return nodes.reduce((acc, node) => ({ + c: acc.c + (node.c ?? 0), + h: acc.h + (node.h ?? 0), + sc: acc.sc + (node.sc ?? 0), + sh: acc.sh + (node.sh ?? 0), + ac: acc.ac + (node.ac ?? 0), + ec: acc.ec + (node.ec ?? 0), + }), { ...Empty_Cost }); } Suggestion importance[1-10]: 5 __ Why: The suggestion improves code quality by adhering to functional programming principles, avoiding mutation of the accumulator in reduce, which enhances clarity and maintainability.	Low
	✅ Reduce memory usage in progress resolution Suggestion Impact: The suggestion directly impacted the commit. Instead of using proxies or lazy evaluation as suggested, the developer implemented a deep clone approach (lines 10-46) that clones the entire data structure once at the beginning (line 54), then mutates the cloned objects in-place (lines 72-144) rather than creating new shallow clones for each array element. This achieves the same goal of reducing memory usage by eliminating the repeated shallow cloning pattern that was creating new objects for every item in the arrays. code diff: + private static deepClone<T>(obj: T, visited = new WeakMap()): T { + // Handle primitives and null + if (obj === null || typeof obj !== "object") return obj; + + // Preserve special class instances + // instanceof check may not hold true bcz data is from another package + // DateTime + if (typeof obj === "object" && "toMillis" in obj) return obj as T; + if (obj instanceof Date) return new Date(obj.getTime()) as T; + + // Check if we've already cloned this object (circular reference) + if (visited.has(obj as object)) { + return visited.get(obj as object) as T; + } + + // Handle arrays + if (Array.isArray(obj)) { + const arrClone = [] as unknown as T; + visited.set(obj as object, arrClone); + (obj as unknown as unknown[]).forEach((item, index) => { + (arrClone as unknown[])[index] = this.deepClone(item, visited); + }); + return arrClone; + } + + // Handle objects + const cloned = {} as T; + visited.set(obj as object, cloned); + + for (const key in obj) { + if (Object.prototype.hasOwnProperty.call(obj, key)) { + cloned[key] = this.deepClone(obj[key], visited); + } + } + + return cloned; + } + /** Resolve unlocked/bought/gifted, etc base on user progress */ - static resolveProgress(d: ISkyData, userData?: UserPlannerData) { - if (!userData) return d; - + static resolveProgress(data: ISkyData, userData?: UserPlannerData) { + if (!userData) return data; + const d = this.deepClone(data); const unlockedSet = this.parseGuidSet(userData.unlocked); const wingedLightsSet = this.parseGuidSet(userData.wingedLights); const favouritesSet = this.parseGuidSet(userData.favourites); const giftedSet = this.parseGuidSet(userData.gifted); - // shallow cloning so that this data is unique for each user - // only clone things that are modified for performance and also structuredClone breaks custom class instances - // NOTE!: currently each data as only items prop, should this change, - // this will need to be revisited to preserve original structure by spreading the rest // items - const items = { - items: d.items.items.map((item) => ({ - ...item, - unlocked: unlockedSet.has(item.guid), - favourited: favouritesSet.has(item.guid), - })), - }; + d.items.items.forEach((item) => { + item.unlocked = unlockedSet.has(item.guid); + item.favourited = favouritesSet.has(item.guid); + }); // nodes - const nodes = { - items: d.nodes.items.map((node) => ({ - ...node, - unlocked: unlockedSet.has(node.guid), - })), - }; + d.nodes.items.forEach((node) => { + node.unlocked = unlockedSet.has(node.guid); + }); // iaps - const iaps = { - items: d.iaps.items.map((iap) => { - const isUnlocked = unlockedSet.has(iap.guid); - const isGifted = giftedSet.has(iap.guid); - return { - ...iap, - bought: isUnlocked && !isGifted, - gifted: isGifted, - }; - }), - }; + d.iaps.items.forEach((iap) => { + const isUnlocked = unlockedSet.has(iap.guid); + const isGifted = giftedSet.has(iap.guid); + + iap.bought = isUnlocked && !isGifted; + iap.gifted = isGifted; + }); // itemLists - const itemLists = { - items: d.itemLists.items.map((itemList) => ({ - ...itemList, - items: itemList.items.map((itemNode) => ({ - ...itemNode, - unlocked: unlockedSet.has(itemNode.guid), - item: { ...itemNode.item, unlocked: unlockedSet.has(itemNode.item.guid) }, - })), - })), - }; + d.itemLists.items.forEach((itemList) => { + itemList.items.forEach((itemNode) => { + itemNode.unlocked = unlockedSet.has(itemNode.guid); + itemNode.item.unlocked = unlockedSet.has(itemNode.item.guid); + }); + }); // winged lights - const wingedLights = { - items: d.wingedLights.items.map((wl) => ({ - ...wl, - unlocked: wingedLightsSet.has(wl.guid), - })), - }; - - return { - ...d, - items, - nodes, - iaps, - itemLists, - wingedLights, - }; + d.wingedLights.items.map((wl) => { + wl.unlocked = wingedLightsSet.has(wl.guid); + }); + + return d; Improve memory efficiency in resolveProgress by avoiding the creation of shallow clones of large data arrays for each user request, possibly by using proxies or lazy evaluation. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [10-16] static resolveProgress(d: ISkyData, userData?: UserPlannerData) { if (!userData) return d; const unlockedSet = this.parseGuidSet(userData.unlocked); const wingedLightsSet = this.parseGuidSet(userData.wingedLights); const favouritesSet = this.parseGuidSet(userData.favourites); const giftedSet = this.parseGuidSet(userData.gifted); + // Consider using a caching layer or Proxy-based lazy evaluation + // to avoid creating full clones for every request + [Suggestion processed] Suggestion importance[1-10]: 4 __ Why: The suggestion correctly points out a potential performance issue with high memory usage from cloning large arrays on every request, which is a valid concern for scalability.	Low

✅ Suggestions up to commit 8dfdfe2

Category	Suggestion	Impact
Incremental [*]	✅ Prevent mutation of shared constant Suggestion Impact: The suggestion was directly implemented. The commit changed Empty_Cost to { ...Empty_Cost } in the calculateCosts method (line 6) and also applied the same fix to another method calculateRemainingCosts (line 15), preventing mutation of the shared constant in both locations. code diff: - }, Empty_Cost); + }, { ...Empty_Cost }); } /** Calcualte the combined remaining cost of the nodes based on progress */ @@ -36,7 +36,8 @@ Object.keys(acc).forEach((k) => ((acc as any)[k] += (node as any)[k] ?? 0)); } return acc; - }, Empty_Cost); + }, { ...Empty_Cost }); Prevent mutation of the shared Empty_Cost constant in calculateCosts by providing a shallow copy ({ ...Empty_Cost }) as the initial value for the reduce operation. packages/skyhelper/src/bot/handlers/planner/helpers/cost.utils.ts [25-30] static calculateCosts(nodes: INode[]): NonNullable<ICost> { return nodes.reduce((acc, node) => { Object.keys(acc).forEach((k) => ((acc as any)[k] += (node as any)[k] ?? 0)); return acc; - }, Empty_Cost); + }, { ...Empty_Cost }); } [Suggestion processed] Suggestion importance[1-10]: 9 __ Why: This suggestion identifies a critical bug where a shared constant is mutated, which would lead to incorrect calculations and is a very subtle but impactful issue.	High
	Handle whitespace in GUID parsing In parseGuidSet, trim whitespace from each GUID after splitting the string to ensure correct parsing and prevent matching errors. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [303-306] static parseGuidSet(value?: string): Set<string> { if (!value || value.length === 0) return new Set(); - return new Set(value.split(",").filter((s) => s.length > 0)); + return new Set(value.split(",").map(s => s.trim()).filter((s) => s.length > 0)); } Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a potential data integrity issue where whitespace in GUID strings could lead to incorrect parsing and bugs in tracking user progress.	Medium
	Add error handling for fetch Add try-catch error handling to the fetchSkyData function to manage network failures, and return stale cached data as a fallback if the fetch operation fails. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [20-34] export async function fetchSkyData(client: SkyHelper, force = false) { const now = Date.now(); const cacheExpired = now - lastFetchTime > CACHE_LIFETIME_MS; if (!cachedData || cacheExpired || force) { - const fetchedData = await fetch(URL).then((res) => res.text()); - const parsed = SkyDataResolver.parse(fetchedData); - ... + try { + const fetchedData = await fetch(URL).then((res) => { + if (!res.ok) throw new Error(`Failed to fetch: ${res.status} ${res.statusText}`); + return res.text(); + }); + const parsed = SkyDataResolver.parse(fetchedData); + ... + cachedData = resolveData(parsed, [...client.applicationEmojis.values()]); + lastFetchTime = now; + } catch (error) { + console.error('Failed to fetch sky data:', error); + if (cachedData) { + console.warn('Returning stale cached data due to fetch failure'); + return cachedData; + } + throw error; + } } return cachedData; } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a lack of error handling for a network request and proposes a robust solution that improves application resilience by preventing crashes and falling back to stale cache.	Medium
	✅ Avoid expensive deep clone operation Suggestion Impact: The suggestion was implemented. The commit moved the !userData check to line 7 (before any cloning), and also went further by replacing the expensive structuredClone with a more efficient shallow cloning approach using spread operators and map operations. code diff: - const data = structuredClone(d); - if (!userData) return data; + if (!userData) return d; Optimize resolveProgress by moving the !userData check before the structuredClone call to avoid an expensive deep copy when no user data is provided. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [10-12] static resolveProgress(d: ISkyData, userData?: UserPlannerData) { + if (!userData) return d; const data = structuredClone(d); - if (!userData) return data; [Suggestion processed] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an unnecessary and expensive deep clone operation and proposes a valid optimization by checking for userData before cloning.	Medium
	Make cache lifetime configurable Make the CACHE_LIFETIME_MS constant configurable using an environment variable instead of being hardcoded. packages/skyhelper/src/bot/handlers/planner/fetcher.ts [13] -const CACHE_LIFETIME_MS = 24 * 60 * 60 * 1000; // 24 hours +const CACHE_LIFETIME_MS = process.env.PLANNER_CACHE_LIFETIME_MS + ? parseInt(process.env.PLANNER_CACHE_LIFETIME_MS, 10) + : 24 * 60 * 60 * 1000; // Default: 24 hours Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a hardcoded value and proposes making it configurable, which improves the application's flexibility for different environments.	Low
General	Extract helper function to class level Extract the addCostToInstance helper function from calculateCurrencyBreakdown and define it as a private static method to avoid repeated function creation. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [170-177] -static addCostToInstance = (instance: IBreakdownInstanceCost, cost: ICost) => { +private static addCostToInstance(instance: IBreakdownInstanceCost, cost: ICost) { instance.cost.c = (instance.cost.c ?? 0) + (cost.c ?? 0); instance.cost.h = (instance.cost.h ?? 0) + (cost.h ?? 0); instance.cost.sc = (instance.cost.sc ?? 0) + (cost.sc ?? 0); instance.cost.sh = (instance.cost.sh ?? 0) + (cost.sh ?? 0); instance.cost.ac = (instance.cost.ac ?? 0) + (cost.ac ?? 0); instance.cost.ec = (instance.cost.ec ?? 0) + (cost.ec ?? 0); -}; +} Suggestion importance[1-10]: 4 __ Why: The suggestion correctly points out that the function is redefined on each call and proposes extracting it, which is a good practice for performance and code organization.	Low
	Add performance optimization for large datasets Consider optimizing calculateCurrencyBreakdown for large datasets by adding options for pagination or limiting the calculation scope to prevent potential performance issues. packages/skyhelper/src/bot/handlers/planner/helpers/data.service.ts [153-160] -static calculateCurrencyBreakdown(data: ISkyData): IBreakdownData { +static calculateCurrencyBreakdown(data: ISkyData, options?: { maxItems?: number }): IBreakdownData { const createEmptyInstance = (): IBreakdownInstanceCost => ({ cost: { c: 0, h: 0, sc: 0, sh: 0, ac: 0, ec: 0 }, price: 0, nodes: [], listNodes: [], iaps: [], }); + // Add early termination logic if maxItems is specified Suggestion importance[1-10]: 2 __ Why: The suggestion raises a valid performance concern, but implementing it would require a significant architectural change, and it's unclear if a partial calculation is useful in the current context.	Low

Suggestions up to commit a01921d

Category	Suggestion	Impact
Possible issue	Pin data source to a specific version To ensure stability, pin the skygame-data package to a specific version in the BASE_URL instead of using the @latest tag. packages/constants/src/skygame-planner/fetcher.ts [23] -export const BASE_URL = `https://unpkg.com/skygame-data@latest`; +export const BASE_URL = `https://unpkg.com/skygame-data@<SPECIFIC_VERSION>`; // e.g. @0.2.1 Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical risk of using the @latest tag, which can lead to unexpected production failures. Pinning the version is a best practice for ensuring stability and reproducibility.	High

[cloudflare-workers-and-pages]

Deploying sky-utils with    Cloudflare Pages

Latest commit:	0d79e73
Status:	✅  Deploy successful!
Preview URL:	https://06b11290.sky-utils.pages.dev
Branch Preview URL:	https://ref-skydata-cdn.sky-utils.pages.dev

View logs