ENG-1076 Variability in roam URLs #562

maparent · 2025-11-23T23:43:09Z

https://linear.app/discourse-graphs/issue/ENG-1076/variability-in-roam-urls

Summary by CodeRabbit

Bug Fixes
- Made Roam space URL construction deterministic, improving reliability when creating spaces and direct links.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

linear · 2025-11-23T23:43:12Z

supabase · 2025-11-23T23:43:16Z

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

maparent · 2025-11-23T23:44:17Z

@CodeRabbit review

coderabbitai · 2025-11-23T23:44:21Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-11-23T23:46:06Z

📝 Walkthrough

Walkthrough

Replaced dynamic getRoamUrl() calls with a deterministic ROAM_URL_PREFIX + spaceName concatenation in Roam URL construction inside supabaseContext.ts. No other logic or error handling was changed.

Changes

Cohort / File(s)	Change Summary
Roam URL construction `apps/roam/src/utils/supabaseContext.ts`	Replaced use of `getRoamUrl()` with a constant `ROAM_URL_PREFIX` and now build Roam space URLs by concatenating `ROAM_URL_PREFIX + spaceName`; URL formation changed from function call to deterministic concatenation.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Single-file, localized substitution of URL construction approach; review focus:
- Verify ROAM_URL_PREFIX value and expected format (trailing slash, protocol).
- Ensure no callers relied on getRoamUrl() side effects or dynamic behavior.

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'ENG-1076 Variability in roam URLs' directly references the issue and accurately summarizes the main change: addressing variability in how Roam URLs are handled by introducing deterministic URL construction.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 848581f and 0159498.

📒 Files selected for processing (1)

apps/roam/src/utils/supabaseContext.ts (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

apps/roam/src/utils/supabaseContext.ts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

apps/roam/src/utils/supabaseContext.ts (1)
57-58: Consider using URL parsing for more robust port normalization.

The verification confirms only port 3333 is currently used in the codebase. However, the hardcoded pattern is still somewhat fragile. If new development environments introduce different ports in the future, the normalization will miss them.

Using URL parsing would be more maintainable and automatically handle any port value without requiring code changes:
let url = getRoamUrl();
-url = url.replace("/?server-port=3333#/", "/#/");
+try {
+  const urlObj = new URL(url);
+  urlObj.searchParams.delete("server-port");
+  url = urlObj.toString();
+} catch (e) {
+  console.warn("Failed to parse Roam URL:", e);
+}
This is an optional improvement for better maintainability and forward-compatibility.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 18e7685 and 848581f.

📒 Files selected for processing (1)

apps/roam/src/utils/supabaseContext.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/config.toml:0-0
Timestamp: 2025-05-20T15:51:22.084Z
Learning: Wildcard patterns in additional_redirect_urls within Supabase configuration are acceptable in this codebase when needed for Vercel branch deployments, despite the small security risk they introduce.

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/api/supabase/rpc/search-content/route.ts:53-56
Timestamp: 2025-05-30T14:37:30.215Z
Learning: The search-content route (apps/website/app/api/supabase/rpc/search-content/route.ts) is intentionally designed to be platform-agnostic rather than Roam-specific. The generic "Platform" terminology (like subsetPlatformIds) is used because the route will support multiple platforms in the near future, not just Roam.

📚 Learning: 2025-05-30T14:37:30.215Z

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 182
File: apps/website/app/api/supabase/rpc/search-content/route.ts:53-56
Timestamp: 2025-05-30T14:37:30.215Z
Learning: The search-content route (apps/website/app/api/supabase/rpc/search-content/route.ts) is intentionally designed to be platform-agnostic rather than Roam-specific. The generic "Platform" terminology (like subsetPlatformIds) is used because the route will support multiple platforms in the near future, not just Roam.

Applied to files:

apps/roam/src/utils/supabaseContext.ts

📚 Learning: 2025-06-23T11:49:45.457Z

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 220
File: apps/roam/src/utils/conceptConversion.ts:11-40
Timestamp: 2025-06-23T11:49:45.457Z
Learning: In the DiscourseGraphs/discourse-graph codebase, direct access to `window.roamAlphaAPI` is the established pattern throughout the codebase. The team prefers to maintain this pattern consistently rather than making piecemeal changes, and plans to address dependency injection as a global refactor when scheduled.

Applied to files:

apps/roam/src/utils/supabaseContext.ts

maparent · 2025-11-23T23:52:54Z

@CodeRabbit: I thought of parsing, but I was trying to avoid impacting new patterns in an unforeseen way, so I made the code very tailored to the one known case.

coderabbitai · 2025-11-23T23:53:44Z

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# View the file mentioned in the AI summary to understand the implementation
cat apps/roam/src/utils/supabaseContext.ts

Length of output: 3687

@maparent, that's a sound approach for addressing the specific known issue. Your targeted string replacement is indeed safer than parsing when you want to avoid unintended side effects on other URL patterns.

The explicit replacement of "/?server-port=3333#/" with "/#/" makes the intent clear and won't accidentally transform URLs that might follow different patterns in the future. This is particularly important since the URL is used as an identifier when creating/fetching the space in the database.

The trade-off is that this won't handle variations like different port numbers (e.g., server-port=3334), but if you're specifically addressing a known case with port 3333, this focused approach aligns well with keeping the PR scope contained to ENG-1076.

✏️ Learnings added

Learnt from: maparent
URL: https://github.com/DiscourseGraphs/discourse-graph/pull/562

Timestamp: 2025-11-23T23:53:43.064Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used

Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-08-30T17:03:26.927Z
Learning: For maparent: Takes a focused approach to PR scope - acknowledges good suggestions that are out of scope for current PR rather than expanding scope.

mdroidian · 2025-11-24T04:28:09Z

Why do we need url? Why not just use platform and spaceName?

maparent · 2025-11-24T15:23:37Z

The assumption was that different spaces could be identified by a URL, which is thus the unique key.
The name is intended for display, not as a canonical identifier for the space. (Though it should be unique so there is no name confusion.)
In the case of roam, all URLs are the same except one part, which we also use as the name; but it would be an unsafe assumption that all platform's URLs can have a single part that serves as a unique identifier.

mdroidian · 2025-11-24T15:33:29Z

But for Roam, window.roamAlphaAPI.graph.name is what is unique, right? So url provides no additional value, is that correct? (because it is graph.name + some additional characters).

maparent · 2025-11-24T15:43:12Z

For Roam, true. For Intergraph, false.

mdroidian · 2025-11-24T15:56:32Z

What will Obsidian use as url?

maparent · 2025-11-24T18:23:30Z

As discussed: Obsidian should use a urn:uuid:.... URN (misnamed as URL)
Since Obsidian graphs are not generally available on the Web.
I think it would be nice to have access to Obsidian URLs when exported, but currently out of scope.

Since there is more variation in Roam URLs than initially observed, I took a different approach, generating a canonical URL rather than trying to capture the variation.

maparent · 2025-11-24T18:23:56Z

@CodeRabbit review

coderabbitai · 2025-11-24T18:24:02Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

sid597

LGTM

sid597 · 2025-11-24T15:13:35Z

apps/roam/src/utils/supabaseContext.ts

      const personName = getCurrentUserDisplayName();
-      const url = getRoamUrl();
+      let url = getRoamUrl();
+      url = url.replace("/?server-port=3333#/", "/#/");


I have seen a few examples of:

https://roamresearch.com/#/app/resultsgraph

https://roamresearch.com/#/app/resultsgraph/search
https://roamresearch.com/#/app/resultsgraph/graph

being added to supabase but it should only be
https://roamresearch.com/#/app/resultsgraph

eng-1076: normalize roam URL

848581f

github-project-automation bot added this to General Nov 23, 2025

coderabbitai bot reviewed Nov 23, 2025

View reviewed changes

maparent requested a review from sid597 November 23, 2025 23:53

generate Url vs parsing

0159498

sid597 approved these changes Nov 27, 2025

View reviewed changes

maparent merged commit eebf334 into main Nov 27, 2025
5 checks passed

maparent deleted the eng-1076-variability-in-roam-urls branch November 27, 2025 18:10

ENG-1076 Variability in roam URLs #562

ENG-1076 Variability in roam URLs #562

Uh oh!

Conversation

maparent commented Nov 23, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

linear bot commented Nov 23, 2025

Uh oh!

supabase bot commented Nov 23, 2025

Uh oh!

maparent commented Nov 23, 2025

Uh oh!

coderabbitai bot commented Nov 23, 2025

Uh oh!

coderabbitai bot commented Nov 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

maparent commented Nov 23, 2025

Uh oh!

coderabbitai bot commented Nov 23, 2025

Uh oh!

mdroidian commented Nov 24, 2025

Uh oh!

maparent commented Nov 24, 2025

Uh oh!

mdroidian commented Nov 24, 2025

Uh oh!

maparent commented Nov 24, 2025

Uh oh!

mdroidian commented Nov 24, 2025

Uh oh!

maparent commented Nov 24, 2025

Uh oh!

maparent commented Nov 24, 2025

Uh oh!

coderabbitai bot commented Nov 24, 2025

Uh oh!

sid597 left a comment

Choose a reason for hiding this comment

Uh oh!

sid597 Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maparent commented Nov 23, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 23, 2025 •

edited

Loading