feat(Sheets): open in new tab if not sheets

[stevekaplan123]

Description

Links in sheets will open in the current window if they are links to other sheets or to voices. Otherwise we open them in a new tab.

[gitvelocity-reviewer]

📊 Code Quality Score: 6/100

22 × 0.25 = 5.5, rounded to 6

Category	Score	Factors
🔭 Scope	4/20	One file (Sheet.jsx) is modified. The change touches one event handler (handleClick) and one prop binding in render(). No new files, no new API endpoints, no cross-subsystem changes.
🏗️ Architecture	2/20	No module boundary changed. URL-routing logic is added inline inside the existing handleClick method rather than extracted to a utility. The prop addition wires handleClick into a child component that previously did not receive it.
⚙️ Implementation	6/20	handleClick now constructs a URL object inside a try/catch to handle malformed URLs, tests url.pathname against /^\/sheets\/\d+/, compares url.hostname against the hostname returned by Sefaria.getModuleURL(Sefaria.VOICES_MODULE), and branches between window.location.href assignment and Sefaria.util.openInNewTab.
⚠️ Risk	7/20	Changes user-visible navigation behavior: links that previously always opened in a new tab now open in the same tab for sheet and Voices URLs. The simultaneous prop addition (handleClick) to the child component is a second behavioral change. No feature flag or rollback mechanism is present.
✅ Quality	2/15	No test file accompanies the change. The navigation branching logic (isSheetLink, isVoicesDomain, fallback) has no automated coverage.
🔒 Perf / Security	1/5	URL parsing uses the browser-native URL constructor with a try/catch. No performance or security work beyond that baseline.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

_{Scored by GitVelocity · How are scores calculated?}

[Copilot]

Pull request overview

This PR updates link navigation behavior within the Sheets UI so that links to other sheets (and to the Voices module) navigate in the current tab, while other links open in a new tab—aligning Sheets with the intended cross-module navigation UX.

Changes:

• Added URL parsing and routing logic in Sheets click-handling to decide between same-tab navigation vs. opening a new tab.
• Passed handleClick into the non-editor SheetContent path so the same link behavior applies in reader mode.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[dcschreiber]

(Claude writing on Daniel's behalf — these review comments were AI-generated and Daniel reviewed them before posting. Posted as one comment because a pending draft review blocks inline comments.)

The approach looks sound and the fallbacks (try/catch around new URL, defaulting to openInNewTab) are careful. Three comments on static/js/sheets/Sheet.jsx, ordered by importance:

1. isSheetLink checks only the pathname, not the hostname (line 61)

A link to https://someothersite.com/sheets/42 would match the regex and navigate the current window to the external site. There's an existing helper that does exactly the right check against all of our module domains in both languages:

const isSheetLink = Sefaria.isSefariaURL(url) && /^\/sheets\/\d+/.test(url.pathname);

2. Modifier clicks are overridden (line 65)

Because e.preventDefault() fires unconditionally, a cmd/ctrl-click (or shift-click) on a sheet/Voices link now navigates the current tab, contradicting the user's explicit "open in new tab" gesture. An early return before preventDefault() lets the browser handle it:

if (e.metaKey || e.ctrlKey || e.shiftKey) { return; }

(The old code hijacked everything into a new tab too, but it never forced a same-tab navigation, so this branch makes it newly noticeable.)

3. The Voices hostname comparison is interface-language-scoped (line 62)

Two edge cases, since getModuleURL resolves via domainModules[currentInterfaceLang]:

• A link to the Hebrew Voices domain clicked while in the English interface won't match, and opens a new tab instead of staying put.

Comparing against getDomainHostnames() (or both languages' Voices hostnames) would cover both. Fine to leave as-is if the mapping is guaranteed populated.

[dcschreiber]

These comments are small, but Claude should have left one or two comments that should actually be fixed.

[dcschreiber]

I think your indentation is off.

[stevekaplan123]

(Claude writing on Daniel's behalf — these review comments were AI-generated and Daniel reviewed them before posting. Posted as one comment because a pending draft review blocks inline comments.)

The approach looks sound and the fallbacks (try/catch around new URL, defaulting to openInNewTab) are careful. Three comments on static/js/sheets/Sheet.jsx, ordered by importance:

1. isSheetLink checks only the pathname, not the hostname (line 61)

A link to https://someothersite.com/sheets/42 would match the regex and navigate the current window to the external site. There's an existing helper that does exactly the right check against all of our module domains in both languages:

const isSheetLink = Sefaria.isSefariaURL(url) && /^\/sheets\/\d+/.test(url.pathname);

2. Modifier clicks are overridden (line 65)

Because e.preventDefault() fires unconditionally, a cmd/ctrl-click (or shift-click) on a sheet/Voices link now navigates the current tab, contradicting the user's explicit "open in new tab" gesture. An early return before preventDefault() lets the browser handle it:

if (e.metaKey || e.ctrlKey || e.shiftKey) { return; }

(The old code hijacked everything into a new tab too, but it never forced a same-tab navigation, so this branch makes it newly noticeable.)

3. The Voices hostname comparison is interface-language-scoped (line 62)

Two edge cases, since getModuleURL resolves via domainModules[currentInterfaceLang]:

• A link to the Hebrew Voices domain clicked while in the English interface won't match, and opens a new tab instead of staying put.

Comparing against getDomainHostnames() (or both languages' Voices hostnames) would cover both. Fine to leave as-is if the mapping is guaranteed populated.

Great points, thanks. I addressed each of these three issues.

[dcschreiber]

(Claude writing on Daniel's behalf — these review comments were AI-generated and Daniel reviewed them before posting.)

Re-reviewed. The three earlier points are properly fixed: the hostname check is real now, both languages' Voices domains match, and splitting getDomainHostnames into three getters is a clean way to get there. Both old callers were updated, so nothing else reads the old helper.

One thing I want to flag as checked rather than as a problem: this PR newly wires handleClick into reader mode, so a link inside sheet content no longer opens a new panel and does a full page navigation instead. I went looking for the reasoning and found it in sc-46102 — the stacking, uncloseable panels are the bug being fixed, and Mickey's acceptance criteria ask for exactly this. So no change wanted there.

Two small items inline, one of which I would like changed before merge.

[dcschreiber]

(Claude writing on Daniel's behalf)

The name reads as the opposite of what the function does. getCurrentLangHostnames returns the current language, so getCurrentModuleHostnames sounds like it returns the current module — but it takes a module as an argument and deliberately spans every language.

getModuleHostnames(module) would say it plainly, and the contrast with getCurrentLangHostnames then does real work: one is scoped to what the reader is using, the other is not scoped at all.

[dcschreiber]

(Claude writing on Daniel's behalf)

No change needed, just so you know it is belt-and-braces: ReaderApp already installs handleInAppClickWithModifiers as a capture-phase listener on document, and it calls stopImmediatePropagation() on any modifier click. Since document sits above React's root container, that fires first and the event never reaches this handler.

So this line is unreachable for Sheet as rendered from ReaderPanel. Harmless, and arguably worth keeping in case the component is ever mounted outside ReaderApp. Flagging it only so nobody later reads it as the thing that makes cmd-click work.

[gitvelocity-reviewer]

📊 Code Quality Score: 16/100

Base Score 40 × ESF 0.4 (Small tier, 85 effective lines, 3 files) = 16

Category	Score	Factors
🔭 Scope	8/20	sefaria.js gains three new exported methods and one private helper in place of one removed method. util.js updates one call site in getCookieDomain. Sheet.jsx replaces the body of handleClick and passes it down as a prop to the sheet body component. All three files are in the same frontend subsystem.
🏗️ Architecture	10/20	getDomainHostnames is removed and its responsibility is split across getCurrentLangHostnames (current language, all modules), getAllHostnames (all languages, all modules), and getCurrentModuleHostnames (all languages, one module), with _hostnamesFromModuleURLs as the shared inner loop. The split makes the current-language vs. all-languages distinction a named boundary rather than an implicit assumption, and Sheet.jsx now depends on the all-languages variants to handle cross-language links correctly.
⚙️ Implementation	9/20	_hostnamesFromModuleURLs wraps each new URL() call in try/catch and logs rather than throws on a bad entry. getCurrentModuleHostnames uses .map().filter() to skip languages that omit the requested module. The new handleClick body adds modifier-key passthrough, relative-URL resolution via new URL(fullUrl, window.location.href), and a two-branch routing decision using isSheetLink (hostname in getAllHostnames and pathname matching /^/sheets/\d+/) and isVoicesDomain (hostname in getCurrentModuleHostnames(Sefaria.VOICES_MODULE)).
⚠️ Risk	7/20	The isSefariaURL and getCookieDomain call-site renames from getDomainHostnames to getCurrentLangHostnames preserve existing semantics. The handleClick rewrite changes which links open in the current tab versus a new tab for all sheet content; a wrong hostname set or a regex mismatch would silently misroute links. No auth paths, no data persistence, and the change is fully reversible.
✅ Quality	4/15	No test file accompanies the change. The block comment above the three getters documents the domainModules data structure, the semantics of each getter, and a usage rule of thumb. Inline comments in handleClick explain the modifier-key passthrough and the cross-language hostname rationale. No test drives _hostnamesFromModuleURLs, getAllHostnames, getCurrentModuleHostnames, or the new handleClick routing branches.
🔒 Perf / Security	2/5	The try/catch in _hostnamesFromModuleURLs prevents a malformed entry in domainModules from propagating an exception to all three callers. The modifier-key passthrough in handleClick avoids intercepting metaKey, ctrlKey, and shiftKey clicks that the browser should handle natively.

Was this score accurate? 👍 Yes · 👎 No

How this was scored →

_{Scored by GitVelocity · How are scores calculated?}