Refactor workers #4
Conversation
|
@reynaldichernando is attempting to deploy a commit to the JS Mastery Pro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
additional notes:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/puter.worker.js`:
- Around line 179-196: The /api/projects/clear route handler
(router.post("/api/projects/clear")) is missing a try/catch like the other
endpoints, so wrap the async handler body in a try block and move the existing
logic (checking user.puter, computing userDeleted via deleteKvByPattern and
publicDeleted via deleteKvByPattern, returning jsonOk) into it; add a catch that
logs or handles the error and returns jsonError(500, "Internal server error")
(or include err.message) so any exception from deleteKvByPattern is handled
consistently with /api/projects/list, /api/projects/get, and /api/projects/save.
- Around line 185-190: The deletion pattern for public projects is too broad:
change the call to deleteKvByPattern(mePuter.kv, `${PUBLIC_PREFIX}*`) so it only
targets the current user's public entries (e.g., `${PUBLIC_PREFIX}${userId}_*`);
update the code in the block that uses userPuter, mePuter, deleteKvByPattern,
and PUBLIC_PREFIX to build a scopedPattern using the current userId (ensuring
userId is present/validated) and pass that scopedPattern to deleteKvByPattern so
only the caller's public projects are removed.
- Around line 79-117: The route handler in router.get("/api/projects/get")
incorrectly checks scope === "private", making the private-project branch
unreachable because the frontend sends scope="user" for private projects; update
the condition in that handler to check scope === "user" so the user/private
lookup using userPuter.kv.get(PROJECT_PREFIX + id) runs when appropriate (ensure
the variables mePuter, userPuter, id, scope and the PRIVATE branch logic remain
unchanged).
🧹 Nitpick comments (1)
lib/puter.worker.js (1)
138-156: Redundantauth.getUser()call in the public save path.
getUserId(userPuter)at line 138 already callsuserPuter.auth.getUser(). Then line 155 callsuserPuter.auth.getUser()again to get the username. Consider reusing the result to avoid a duplicate network round-trip.♻️ Proposed refactor to avoid duplicate call
- const userId = await getUserId(userPuter); - if (!userId) return jsonError(401, "User id required"); + const userInfo = await userPuter.auth.getUser(); + const userId = userInfo?.uuid || null; + if (!userId) return jsonError(401, "User id required"); if (scope === "private") { // PRIVATE PROJECT ... } else { // PUBLIC PROJECT const publicKey = getPublicKey(userId, project.id); - const userInfo = await userPuter.auth.getUser(); let username = userInfo?.username || userInfo?.name || null;
| router.get("/api/projects/get", async ({ request, user }) => { | ||
| try { | ||
| const mePuter = me.puter; | ||
| const userPuter = user.puter; | ||
|
|
||
| if (!id) return jsonError(400, "Project id required"); | ||
| if (!userPuter) return jsonError(401, "Authentication required"); | ||
|
|
||
| if (scope === "public") { | ||
| const mePuter = getMePuter(me); | ||
| if (!mePuter) return jsonError(500, "Missing deployer Puter context."); | ||
| const url = new URL(request.url); | ||
| const id = url.searchParams.get("id"); | ||
| const scope = url.searchParams.get("scope") || "user"; | ||
| const ownerId = url.searchParams.get("ownerId"); | ||
|
|
||
| const publicKey = ownerId | ||
| ? getPublicKey(ownerId, id) | ||
| : await findPublicKeyByProjectId(mePuter, id); | ||
| if (!id) return jsonError(400, "Project id required"); | ||
|
|
||
| if (!publicKey) return jsonError(404, "Project not found"); | ||
| if (scope === "private") { | ||
| // PRIVATE PROJECT | ||
| const key = `${PROJECT_PREFIX}${id}`; | ||
| const project = await userPuter.kv.get(key); | ||
| if (!project) return jsonError(404, "Project not found"); | ||
|
|
||
| const project = await kvGetProject(mePuter.kv, publicKey); | ||
| if (!project) return jsonError(404, "Project not found"); | ||
| return { project }; | ||
| } else { | ||
| // PUBLIC PROJECT | ||
| const publicKey = ownerId | ||
| ? getPublicKey(ownerId, id) | ||
| : await findPublicKeyByProjectId(mePuter, id); | ||
| if (!publicKey) return jsonError(404, "Project not found"); | ||
|
|
||
| return { project }; | ||
| } | ||
| const project = await mePuter.kv.get(publicKey); | ||
| if (!project) return jsonError(404, "Project not found"); | ||
|
|
||
| const userPuter = getUserPuter(user); | ||
| if (!userPuter) return jsonError(401, "Authentication required"); | ||
|
|
||
| const key = `${PROJECT_PREFIX}${id}`; | ||
| const project = await kvGetProject(userPuter.kv, key); | ||
| if (!project) return jsonError(404, "Project not found"); | ||
| return { project }; | ||
| return { project }; | ||
| } | ||
| } catch (error) { | ||
| return jsonError(500, "Failed to get project", { | ||
| message: error?.message || "Unknown error", | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -A 10 'getProjectById' --type=ts --type=jsRepository: JavaScript-Mastery-Pro/roomify
Length of output: 1905
🏁 Script executed:
sed -n '106,130p' lib/puter.action.tsRepository: JavaScript-Mastery-Pro/roomify
Length of output: 743
Scope mismatch prevents private projects from being retrieved.
The frontend (visualizer.$id.tsx line 38) sends scope="user" for private projects and scope="public" for public projects via getProjectById (confirmed in lib/puter.action.ts line 125: &scope=${scope}). However, the backend endpoint at line 93 checks if (scope === "private"), which will never be true. This means the private project branch (lines 94–99) is unreachable, and all private project requests incorrectly fall through to the public lookup, causing them to fail.
Change line 93 from if (scope === "private") to if (scope === "user").
🤖 Prompt for AI Agents
In `@lib/puter.worker.js` around lines 79 - 117, The route handler in
router.get("/api/projects/get") incorrectly checks scope === "private", making
the private-project branch unreachable because the frontend sends scope="user"
for private projects; update the condition in that handler to check scope ===
"user" so the user/private lookup using userPuter.kv.get(PROJECT_PREFIX + id)
runs when appropriate (ensure the variables mePuter, userPuter, id, scope and
the PRIVATE branch logic remain unchanged).
There was a problem hiding this comment.
hmm, this might be true, although its a bit weird we have 2 different flags to determine private and public
like in our save, we have visibility private and public, while in get, we have public and user? @sujatagunale what do you think, should we just unify it?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
i'll let you decide on this 🙏
sujatagunale
merged commit 82c1408
into
JavaScript-Mastery-Pro:main
2 participants
workers:
listKvValueslogic, i notice we aren't using the pagination, so we should just use the simple version of the functiondeleteKvByPattern, we somehow are doing extra manual pattern logic (?) which is not needed, since thelistKvValuesalready accepts patternmefrom router param and just use it directly, this is a global param, and not router (like user), plus its going to be always available/api/projects/list, some logic are just unnecessary/api/projects/get, removed thekvGetProjectwrapper, as we don't need this, the puter.kv.get is already a one liner/api/projects/save, adding minor comment + returning thesharedByso it doesn't show as unknown when clicking share in the frontend/api/hosting/reset, hosting lives in user context, so there's no point clearing it in the workers context, since it doesn't exist in the first placefrontend:
Visualizercomponentnotes:
Summary by CodeRabbit