Eng 418 create and store roam space password #245
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes introduce new utilities for managing block properties in Roam, including functions to retrieve raw and normalized block properties, and a new module to set and denormalize block properties. Additionally, the Supabase context is updated to include a "space password," which is retrieved or generated and stored in block properties. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BlockAPI as Roam Block API
Client->>BlockAPI: getRawBlockProps(uid)
BlockAPI-->>Client: rawProps
Client->>Client: normalizeProps(rawProps)
Client-->>Client: getBlockProps returns normalizedProps
sequenceDiagram
participant Client
participant BlockAPI as Roam Block API
Client->>BlockAPI: getRawBlockProps(uid)
BlockAPI-->>Client: baseProps
Client->>Client: deNormalizeProps(newProps)
Client->>Client: merge baseProps with deNormalized newProps
Client->>BlockAPI: update block properties
BlockAPI-->>Client: updatedProps
Client-->>Client: setBlockProps returns updatedProps
sequenceDiagram
participant Client
participant BlockAPI as Roam Block API
Client->>BlockAPI: getBlockProps(configPageUid)
BlockAPI-->>Client: configProps
alt password exists
Client-->>Client: use existing password
else password missing
Client->>Client: generate random UUID
Client->>BlockAPI: setBlockProps(configPageUid, {"space-user-password": uuid})
end
Client-->>Client: include spacePassword in SupabaseContext
Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/roam/src/utils/setBlockProps.ts (1)
27-38: Consider adding error handling for invalid UIDs.The function correctly merges properties and updates the block, but it doesn't validate whether the UID exists before attempting the update.
Consider adding basic validation:
const setBlockProps = (uid: string, newProps: Record<string, json>) => { + if (!uid) { + console.warn("setBlockProps: Invalid UID provided"); + return null; + } const baseProps = getRawBlockProps(uid); if (typeof baseProps === "object" && !Array.isArray(baseProps)) { const props = { ...(baseProps || {}), ...(deNormalizeProps(newProps) as Record<string, json>), } as Record<string, json>; - window.roamAlphaAPI.data.block.update({ block: { uid, props } }); + try { + window.roamAlphaAPI.data.block.update({ block: { uid, props } }); + } catch (error) { + console.error(`Failed to update block properties for UID ${uid}:`, error); + throw error; + } return props; } return baseProps; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/utils/getBlockProps.ts(1 hunks)apps/roam/src/utils/setBlockProps.ts(1 hunks)apps/roam/src/utils/supabaseContext.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/roam/src/utils/supabaseContext.ts (2)
packages/database/types.gen.ts (1)
Database(9-814)apps/roam/src/utils/renderNodeConfigPage.ts (1)
DISCOURSE_CONFIG_PAGE_TITLE(28-28)
apps/roam/src/utils/setBlockProps.ts (1)
apps/roam/src/utils/getBlockProps.ts (2)
json(1-7)getRawBlockProps(27-30)
🔇 Additional comments (4)
apps/roam/src/utils/getBlockProps.ts (1)
27-33: LGTM! Clean separation of concerns.The refactoring successfully separates raw data retrieval from normalization while maintaining backward compatibility. This design enables other modules to access raw properties when needed.
apps/roam/src/utils/supabaseContext.ts (1)
12-12: Verify crypto.randomUUID() browser support.The
crypto.randomUUID()method requires modern browser support and secure contexts (HTTPS). Ensure this aligns with your target browser compatibility.What is the browser support for crypto.randomUUID() and does it require HTTPS?apps/roam/src/utils/setBlockProps.ts (2)
9-25: LGTM! Correct inverse operation of normalization.The
deNormalizePropsfunction correctly reverses the normalization process by adding colons to keys, handling nested objects and arrays recursively. The implementation mirrors the logic innormalizeProps.
40-46: LGTM! Useful testing utility.The
testSetBlockPropsfunction provides a convenient way to set properties using page titles, properly handling the case where the page doesn't exist.
| const getOrCreateSpacePassword = () => { | ||
| const props = getBlockProps(settingsConfigPageUid); | ||
| const existing: string | unknown = props["space-user-password"]; | ||
| if (existing && typeof existing === "string") return existing; | ||
| // use a uuid as password, at least cryptographically safe | ||
| const password = crypto.randomUUID(); | ||
| setBlockProps(settingsConfigPageUid, { | ||
| "space-user-password": password, | ||
| }); | ||
| return password; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and consider error handling.
The function has a couple of areas for improvement:
- Type narrowing: The
existingvariable is typed asstring | unknownbut the code only handles the string case explicitly. - Error handling: No error handling for the
setBlockPropsoperation, which could fail silently.
Consider this improvement:
const getOrCreateSpacePassword = () => {
const props = getBlockProps(settingsConfigPageUid);
- const existing: string | unknown = props["space-user-password"];
- if (existing && typeof existing === "string") return existing;
+ const existing = props["space-user-password"];
+ if (typeof existing === "string" && existing.length > 0) return existing;
// use a uuid as password, at least cryptographically safe
const password = crypto.randomUUID();
- setBlockProps(settingsConfigPageUid, {
- "space-user-password": password,
- });
+ try {
+ setBlockProps(settingsConfigPageUid, {
+ "space-user-password": password,
+ });
+ } catch (error) {
+ console.error("Failed to store space password:", error);
+ // Consider whether to throw or handle gracefully
+ }
return password;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getOrCreateSpacePassword = () => { | |
| const props = getBlockProps(settingsConfigPageUid); | |
| const existing: string | unknown = props["space-user-password"]; | |
| if (existing && typeof existing === "string") return existing; | |
| // use a uuid as password, at least cryptographically safe | |
| const password = crypto.randomUUID(); | |
| setBlockProps(settingsConfigPageUid, { | |
| "space-user-password": password, | |
| }); | |
| return password; | |
| }; | |
| const getOrCreateSpacePassword = () => { | |
| const props = getBlockProps(settingsConfigPageUid); | |
| const existing = props["space-user-password"]; | |
| if (typeof existing === "string" && existing.length > 0) return existing; | |
| // use a uuid as password, at least cryptographically safe | |
| const password = crypto.randomUUID(); | |
| try { | |
| setBlockProps(settingsConfigPageUid, { | |
| "space-user-password": password, | |
| }); | |
| } catch (error) { | |
| console.error("Failed to store space password:", error); | |
| // Consider whether to throw or handle gracefully | |
| } | |
| return password; | |
| }; |
🤖 Prompt for AI Agents
In apps/roam/src/utils/supabaseContext.ts around lines 35 to 45, improve type
safety by refining the type of the `existing` variable to exclude `unknown` and
explicitly handle non-string cases or absence of the property. Add error
handling around the `setBlockProps` call by wrapping it in a try-catch block to
catch and handle potential failures, such as logging the error or rethrowing it,
to prevent silent failures.
|
@CodeRabbit the existence of this uid is an invariant |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@maparent I need to examine the code to understand which specific UID you're referring to as an invariant. Let me look at the implementation details. 🏁 Script executed: Length of output: 4522 @maparent Thank you for clarifying! I can see you're referring to the You're stating that the configuration page will always exist in the system, which explains why the code can safely use this UID directly in Thoughtful rabbit taking notes ✏️ Learnings added
|
maparent
force-pushed
the
ENG-520-supabase-in-frontend
branch
from
9cf8334 to
6cfbec9
Compare
maparent
force-pushed
the
ENG-418-create-and-store-roam-space-password
branch
from
0e6ff35 to
07cf6d2
Compare
maparent
force-pushed
the
ENG-520-supabase-in-frontend
branch
from
6cfbec9 to
93b7c94
Compare
maparent
force-pushed
the
ENG-418-create-and-store-roam-space-password
branch
from
07cf6d2 to
89162a9
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I won't block this, but I'm not sure deNormalizeProps is required
maparent
force-pushed
the
ENG-418-create-and-store-roam-space-password
branch
from
89162a9 to
3bd13a0
Compare
maparent
force-pushed
the
ENG-418-create-and-store-roam-space-password
branch
from
3bd13a0 to
67ecd10
Compare
Create it (as a UUID, which is cryptographicallly safe) and store it otherwise. Added an utility function to set Props.
maparent
force-pushed
the
ENG-418-create-and-store-roam-space-password
branch
from
67ecd10 to
12b91a4
Compare
Obtain a password from the roam props of the plugin settings, intended to use for the supabase anonymous user for the space.
Create it and store it otherwise.
Added an utility function to set Props.
Note that I'm basing it on 520, not because I need to yet, but because my next task will use both of those together, and unless the merges come really fast I'll be paralyzed.
Summary by CodeRabbit
New Features
Refactor