fix: Send returnUrl to notes app when shared drive recipient#3746
Merged
fix: Send returnUrl to notes app when shared drive recipient#3746
Conversation
zatteo
requested review from
JF-Cozy,
doubleface,
lethemanh and
rezk2ll
as code owners
WalkthroughPublicNoteRedirect now reads the location search string (via useLocation), extracts a Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/modules/navigation/PublicNoteRedirect.tsx (1)
26-60: Consider addingsearchto the useEffect dependency array.The
searchvariable is used inside the effect (line 32) but isn't listed in the dependency array. While this is likely fine sincesearchwon't change during this redirect flow, adding it would satisfy the exhaustive-deps rule and prevent potential stale closure issues if the component's usage changes.♻️ Add search to dependencies
- }, [fileId, driveId, client]) + }, [fileId, driveId, client, search])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/PublicNoteRedirect.tsx` around lines 26 - 60, The useEffect in PublicNoteRedirect (the effect that defines fetchNoteUrl) references the variable search but does not include it in the dependency array; update the dependency list for that useEffect to include search (so the effect depends on [fileId, driveId, client, search]) to satisfy exhaustive-deps and avoid stale closures when search changes.src/modules/shareddrives/helpers.ts (1)
43-57: Non-null assertion onfile.driveIdcould mask runtime issues.The function uses
file.driveId!but the typeIOCozyFiledoesn't guaranteedriveIdis present. While the caller incomputePathchecks fordriveIdbefore calling, this function doesn't enforce that precondition.Consider either:
- Adding a runtime check with a descriptive error
- Using a more specific type that guarantees
driveId🛡️ Suggested defensive check
export const makeSharedDriveNoteReturnUrl = ( client: CozyClient, file: IOCozyFile ): string => { + if (!file.driveId) { + throw new Error('driveId is required to generate shared drive return URL') + } return generateWebLink({ slug: 'drive', searchParams: [], // eslint-disable-next-line `@typescript-eslint/no-unsafe-member-access` cozyUrl: client.getStackClient().uri as string, // eslint-disable-next-line `@typescript-eslint/no-unsafe-assignment` subDomainType: client.getInstanceOptions().subdomain, pathname: '', - hash: `/shareddrive/${file.driveId!}/${file.dir_id}` + hash: `/shareddrive/${file.driveId}/${file.dir_id}` }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/shareddrives/helpers.ts` around lines 43 - 57, The helper makeSharedDriveNoteReturnUrl is using a non-null assertion on file.driveId which can mask runtime errors; update it to either (a) perform a runtime check at the start of makeSharedDriveNoteReturnUrl and throw a descriptive error if file.driveId is missing, or (b) tighten the function signature to accept a file type that guarantees driveId (e.g., IOCozyFile & { driveId: string }) so callers like computePath must pass a file with driveId; ensure references to client.getStackClient(), client.getInstanceOptions(), and the hash construction remain unchanged.src/modules/navigation/hooks/helpers.ts (1)
120-130: EmptyreturnUrlparameter appended when client is null.When
driveIdexists butclientis null, the path becomes/note/${driveId}/${file._id}?returnUrl=with an empty value. Consider omitting the query parameter entirely whenreturnUrlis empty to produce cleaner URLs.♻️ Cleaner URL when returnUrl is empty
case 'public-note': if (driveId) { const returnUrl = client ? makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) : '' - return `/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent( - returnUrl - )}` + const basePath = `/note/${driveId}/${file._id}` + return returnUrl + ? `${basePath}?returnUrl=${encodeURIComponent(returnUrl)}` + : basePath } else { return `/note/${file._id}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/hooks/helpers.ts` around lines 120 - 130, The generated URL includes an empty returnUrl query when client is null; in the block that constructs the shared-drive note path (inside src/modules/navigation/hooks/helpers.ts around the code using driveId, file._id and makeSharedDriveNoteReturnUrl), only append "?returnUrl=..." when returnUrl is non-empty/truthy — i.e., compute returnUrl with makeSharedDriveNoteReturnUrl(client, file) and then branch to return `/note/${driveId}/${file._id}` if returnUrl is falsy, otherwise return `/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent(returnUrl)}` so no empty query parameter is emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/navigation/PublicNoteRedirect.tsx`:
- Line 63: Remove the debug console.log call that prints noteUrl in
PublicNoteRedirect.tsx (the line console.log('🔴 noteUrl', noteUrl)); locate it
inside the PublicNoteRedirect component where noteUrl is created/used and delete
that statement, or replace it with a proper structured logging call if
persistent metrics are needed (e.g., use the app logger or debug flag) so no
debug console output remains in production.
In `@src/modules/shareddrives/helpers.ts`:
- Line 55: The hash generation uses file.dir_id directly which can be undefined
and produce "undefined" in the URL; update the code that builds the hash (the
template literal where hash: `/shareddrive/${file.driveId!}/${file.dir_id}`) to
validate file.dir_id first and use a safe fallback (e.g., empty string or a
designated "root" token) or conditionally omit the segment so the produced path
never contains the literal "undefined"; ensure the same validation pattern is
applied wherever file.dir_id is used for URL/path construction.
---
Nitpick comments:
In `@src/modules/navigation/hooks/helpers.ts`:
- Around line 120-130: The generated URL includes an empty returnUrl query when
client is null; in the block that constructs the shared-drive note path (inside
src/modules/navigation/hooks/helpers.ts around the code using driveId, file._id
and makeSharedDriveNoteReturnUrl), only append "?returnUrl=..." when returnUrl
is non-empty/truthy — i.e., compute returnUrl with
makeSharedDriveNoteReturnUrl(client, file) and then branch to return
`/note/${driveId}/${file._id}` if returnUrl is falsy, otherwise return
`/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent(returnUrl)}` so no
empty query parameter is emitted.
In `@src/modules/navigation/PublicNoteRedirect.tsx`:
- Around line 26-60: The useEffect in PublicNoteRedirect (the effect that
defines fetchNoteUrl) references the variable search but does not include it in
the dependency array; update the dependency list for that useEffect to include
search (so the effect depends on [fileId, driveId, client, search]) to satisfy
exhaustive-deps and avoid stale closures when search changes.
In `@src/modules/shareddrives/helpers.ts`:
- Around line 43-57: The helper makeSharedDriveNoteReturnUrl is using a non-null
assertion on file.driveId which can mask runtime errors; update it to either (a)
perform a runtime check at the start of makeSharedDriveNoteReturnUrl and throw a
descriptive error if file.driveId is missing, or (b) tighten the function
signature to accept a file type that guarantees driveId (e.g., IOCozyFile & {
driveId: string }) so callers like computePath must pass a file with driveId;
ensure references to client.getStackClient(), client.getInstanceOptions(), and
the hash construction remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d81f709-9055-4f6c-80b6-784720d35bea
📒 Files selected for processing (4)
src/modules/navigation/PublicNoteRedirect.tsxsrc/modules/navigation/hooks/helpers.tssrc/modules/navigation/hooks/useFileLink.tsxsrc/modules/shareddrives/helpers.ts
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| subDomainType: client.getInstanceOptions().subdomain, | ||
| pathname: '', | ||
| hash: `/shareddrive/${file.driveId!}/${file.dir_id}` |
There was a problem hiding this comment.
file.dir_id is used without validation.
Similar to driveId, dir_id could be undefined (e.g., for root-level items), which would result in a malformed URL containing the literal string "undefined".
🛡️ Add validation for dir_id
export const makeSharedDriveNoteReturnUrl = (
client: CozyClient,
file: IOCozyFile
): string => {
+ if (!file.driveId || !file.dir_id) {
+ throw new Error('driveId and dir_id are required to generate shared drive return URL')
+ }
return generateWebLink({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/shareddrives/helpers.ts` at line 55, The hash generation uses
file.dir_id directly which can be undefined and produce "undefined" in the URL;
update the code that builds the hash (the template literal where hash:
`/shareddrive/${file.driveId!}/${file.dir_id}`) to validate file.dir_id first
and use a safe fallback (e.g., empty string or a designated "root" token) or
conditionally omit the segment so the produced path never contains the literal
"undefined"; ensure the same validation pattern is applied wherever file.dir_id
is used for URL/path construction.
f917fbd to
936dc4c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
zatteo
force-pushed
the
fix/shared-drive-recipient-note-return-url
branch
from
936dc4c to
14e2a5f
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/modules/shareddrives/helpers.ts (1)
43-57:⚠️ Potential issue | 🟡 MinorValidate
driveIdanddir_idbefore constructing the URL.Both
file.driveIdandfile.dir_idare used without validation. WhiledriveIduses a non-null assertion (!), and the caller incomputePathchecks fordriveId,dir_idcould still be undefined (e.g., for root-level items), resulting in a malformed URL containing the literal string "undefined".🛡️ Add validation for both properties
export const makeSharedDriveNoteReturnUrl = ( client: CozyClient, file: IOCozyFile ): string => { + if (!file.driveId || !file.dir_id) { + throw new Error('driveId and dir_id are required to generate shared drive return URL') + } return generateWebLink({ slug: 'drive', searchParams: [], // eslint-disable-next-line `@typescript-eslint/no-unsafe-member-access` cozyUrl: client.getStackClient().uri as string, // eslint-disable-next-line `@typescript-eslint/no-unsafe-assignment` subDomainType: client.getInstanceOptions().subdomain, pathname: '', - hash: `/shareddrive/${file.driveId!}/${file.dir_id}` + hash: `/shareddrive/${file.driveId}/${file.dir_id}` }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/shareddrives/helpers.ts` around lines 43 - 57, In makeSharedDriveNoteReturnUrl validate that file.driveId and file.dir_id are defined before building the hash: if driveId is missing throw/return an explicit error (do not rely on the non-null assertion), and for dir_id handle root-level items by using an explicit fallback (e.g. empty string or a canonical "root" token) so the generated hash never contains the literal "undefined"; update the hash construction (inside generateWebLink call in makeSharedDriveNoteReturnUrl) to use the validated driveId and a safe dirId fallback.
🧹 Nitpick comments (1)
src/modules/navigation/PublicNoteRedirect.tsx (1)
32-33: Consider validatingreturnUrlbefore passing tofetchURLto prevent potential open redirect vulnerabilities.Although
returnUrltypically originates from the controlledmakeSharedDriveNoteReturnUrl()function, it's extracted directly from URL search parameters which users can modify. The parameter is passed tofetchURLwithout validation. Consider adding a check to ensurereturnUrlpoints to a legitimate Cozy instance before passing it along.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/PublicNoteRedirect.tsx` around lines 32 - 33, Validate the extracted returnUrl from searchParams before passing it to fetchURL in PublicNoteRedirect: parse returnUrl using the URL constructor (or a safe parser), ensure its origin/host matches an allowed Cozy instance pattern (e.g., exact trusted host(s), allowed host suffix, or match window.location.origin if appropriate), and only call fetchURL(returnUrl) when the check passes; otherwise fall back to a safe internal URL or abort the redirect. Update the logic around searchParams/returnUrl and any call sites of fetchURL to perform this validation so untrusted user-supplied returnUrl values cannot trigger open redirects.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/navigation/hooks/helpers.ts`:
- Around line 120-130: The URL builder in the block referencing driveId, client,
makeSharedDriveNoteReturnUrl and file currently always appends a returnUrl query
parameter (empty when client is null); change the logic so you only add
?returnUrl=... when returnUrl is a non-empty string: compute returnUrl via
makeSharedDriveNoteReturnUrl(client, file) only if client exists, then build
`/note/${driveId}/${file._id}` and append
`?returnUrl=${encodeURIComponent(returnUrl)}` only when returnUrl is truthy
(omit the entire query string when it is empty or undefined).
---
Duplicate comments:
In `@src/modules/shareddrives/helpers.ts`:
- Around line 43-57: In makeSharedDriveNoteReturnUrl validate that file.driveId
and file.dir_id are defined before building the hash: if driveId is missing
throw/return an explicit error (do not rely on the non-null assertion), and for
dir_id handle root-level items by using an explicit fallback (e.g. empty string
or a canonical "root" token) so the generated hash never contains the literal
"undefined"; update the hash construction (inside generateWebLink call in
makeSharedDriveNoteReturnUrl) to use the validated driveId and a safe dirId
fallback.
---
Nitpick comments:
In `@src/modules/navigation/PublicNoteRedirect.tsx`:
- Around line 32-33: Validate the extracted returnUrl from searchParams before
passing it to fetchURL in PublicNoteRedirect: parse returnUrl using the URL
constructor (or a safe parser), ensure its origin/host matches an allowed Cozy
instance pattern (e.g., exact trusted host(s), allowed host suffix, or match
window.location.origin if appropriate), and only call fetchURL(returnUrl) when
the check passes; otherwise fall back to a safe internal URL or abort the
redirect. Update the logic around searchParams/returnUrl and any call sites of
fetchURL to perform this validation so untrusted user-supplied returnUrl values
cannot trigger open redirects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dbbfeef-f03f-4ec8-8e01-e169937fe304
📒 Files selected for processing (4)
src/modules/navigation/PublicNoteRedirect.tsxsrc/modules/navigation/hooks/helpers.tssrc/modules/navigation/hooks/useFileLink.tsxsrc/modules/shareddrives/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/navigation/hooks/useFileLink.tsx
Comment on lines
+120
to
+130
| if (driveId) { | ||
| const returnUrl = client | ||
| ? makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) | ||
| : '' | ||
|
|
||
| return `/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent( | ||
| returnUrl | ||
| )}` | ||
| } else { | ||
| return `/note/${file._id}` | ||
| } |
There was a problem hiding this comment.
Empty returnUrl query parameter when client is null.
When driveId exists but client is null, returnUrl becomes an empty string, resulting in a URL like /note/{driveId}/{fileId}?returnUrl=. This empty query parameter may be unintended and could cause issues in the notes app when parsing.
Consider only appending the returnUrl parameter when it has a value.
🔧 Conditionally append returnUrl
case 'public-note':
if (driveId) {
const returnUrl = client
? makeSharedDriveNoteReturnUrl(client, file as IOCozyFile)
: ''
- return `/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent(
- returnUrl
- )}`
+ const basePath = `/note/${driveId}/${file._id}`
+ return returnUrl
+ ? `${basePath}?returnUrl=${encodeURIComponent(returnUrl)}`
+ : basePath
} else {
return `/note/${file._id}`
}📝 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.
Suggested change
| if (driveId) { | |
| const returnUrl = client | |
| ? makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) | |
| : '' | |
| return `/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent( | |
| returnUrl | |
| )}` | |
| } else { | |
| return `/note/${file._id}` | |
| } | |
| if (driveId) { | |
| const returnUrl = client | |
| ? makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) | |
| : '' | |
| const basePath = `/note/${driveId}/${file._id}` | |
| return returnUrl | |
| ? `${basePath}?returnUrl=${encodeURIComponent(returnUrl)}` | |
| : basePath | |
| } else { | |
| return `/note/${file._id}` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modules/navigation/hooks/helpers.ts` around lines 120 - 130, The URL
builder in the block referencing driveId, client, makeSharedDriveNoteReturnUrl
and file currently always appends a returnUrl query parameter (empty when client
is null); change the logic so you only add ?returnUrl=... when returnUrl is a
non-empty string: compute returnUrl via makeSharedDriveNoteReturnUrl(client,
file) only if client exists, then build `/note/${driveId}/${file._id}` and
append `?returnUrl=${encodeURIComponent(returnUrl)}` only when returnUrl is
truthy (omit the entire query string when it is empty or undefined).
BundleMonFiles updated (6)
Unchanged files (15)
Total files change +163B 0% Groups updated (1)
Unchanged groups (2)
Final result: ✅ View report in BundleMon website ➡️ |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/modules/navigation/hooks/helpers.ts (1)
120-130:⚠️ Potential issue | 🟡 MinorEmpty
returnUrlquery parameter whenclientis null.When
driveIdexists butclientis null, the code appends an emptyreturnUrlparameter (e.g.,/note/drive456/note123?returnUrl=). This could cause issues in the notes app when parsing the URL.Consider only appending the parameter when
returnUrlhas a value:🔧 Conditionally append returnUrl
case 'public-note': if (driveId) { const returnUrl = client ? makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) : '' - return `/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent( - returnUrl - )}` + const basePath = `/note/${driveId}/${file._id}` + return returnUrl + ? `${basePath}?returnUrl=${encodeURIComponent(returnUrl)}` + : basePath } else { return `/note/${file._id}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/hooks/helpers.ts` around lines 120 - 130, The code builds a note URL that includes a returnUrl query param even when client is null, producing an empty `returnUrl=`; update the logic in the block that checks `driveId` (the function using `driveId`, `client`, `makeSharedDriveNoteReturnUrl`, and `file._id`) to only append `?returnUrl=...` when `returnUrl` is non-empty: compute `returnUrl` via `makeSharedDriveNoteReturnUrl(client, file)` only if `client` is truthy, then conditionally add the `?returnUrl=encodeURIComponent(returnUrl)` suffix to `/note/${driveId}/${file._id}` only when `returnUrl` has a value; otherwise return the base `/note/${driveId}/${file._id}` without any empty query parameter.src/modules/shareddrives/helpers.ts (1)
43-57:⚠️ Potential issue | 🟡 MinorValidate
driveIdanddir_idbefore use to prevent malformed URLs.The static analysis flagged the non-null assertion on
file.driveId!(line 55). While the caller incomputePathchecks fordriveIdexistence,dir_idcould still be undefined for root-level items, resulting in a URL containing the literal string "undefined".Consider adding validation at the function boundary for defensive programming:
🛡️ Proposed fix with validation
export const makeSharedDriveNoteReturnUrl = ( client: CozyClient, file: IOCozyFile ): string => { + if (!file.driveId || !file.dir_id) { + throw new Error('driveId and dir_id are required to generate shared drive return URL') + } return generateWebLink({ slug: 'drive', searchParams: [], // eslint-disable-next-line `@typescript-eslint/no-unsafe-member-access` cozyUrl: client.getStackClient().uri as string, // eslint-disable-next-line `@typescript-eslint/no-unsafe-assignment` subDomainType: client.getInstanceOptions().subdomain, pathname: '', - hash: `/shareddrive/${file.driveId!}/${file.dir_id}` + hash: `/shareddrive/${file.driveId}/${file.dir_id}` }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/shareddrives/helpers.ts` around lines 43 - 57, The makeSharedDriveNoteReturnUrl function uses file.driveId! and file.dir_id directly, which can produce malformed URLs if either is undefined; update this function to validate file.driveId and file.dir_id at the start (e.g., if (!file.driveId || !file.dir_id) throw a descriptive Error or normalize dir_id to ''/root as appropriate for your app), remove the non-null assertion on file.driveId, and then build the hash using the validated/normalized values when calling generateWebLink so the URL never contains the literal "undefined".
🧹 Nitpick comments (1)
src/modules/navigation/hooks/helpers.spec.js (1)
237-242: Consider adding test coverage for the case whenclientis provided.This test validates the behavior when
clientis not provided (emptyreturnUrl). Adding a test case that mocks aCozyClientand verifies the actual return URL generation would improve coverage of the happy path.💡 Suggested additional test case
it('should return correct path for public-note with driveId and client', () => { const file = { _id: 'note123', driveId: 'drive456', dir_id: 'folder789' } const mockClient = { getStackClient: () => ({ uri: 'https://example.mycozy.cloud' }), getInstanceOptions: () => ({ subdomain: 'nested' }) } const result = computePath(file, { type: 'public-note', pathname: '/public', client: mockClient }) expect(result).toMatch(/^\/note\/drive456\/note123\?returnUrl=/) expect(result).toContain(encodeURIComponent('https://')) })Note: You may need to mock
generateWebLinkfromcozy-clientfor this test to work properly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/hooks/helpers.spec.js` around lines 237 - 242, Add a unit test that exercises computePath when a CozyClient-like `client` is provided: create a file object with _id, driveId (and optional dir_id), mock a client exposing getStackClient and getInstanceOptions (or stub generateWebLink from cozy-client), call computePath(file, { type: 'public-note', pathname: '/public', client: mockClient }) and assert the returned path still matches the /note/:driveId/:id?returnUrl=... pattern and that the query contains a non-empty encoded returnUrl (e.g., contains "https://" when decoded or contains encodeURIComponent('https://')), ensuring the test covers the happy path with client present; use the same test file and helper names (computePath, generateWebLink) to locate code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/modules/navigation/hooks/helpers.ts`:
- Around line 120-130: The code builds a note URL that includes a returnUrl
query param even when client is null, producing an empty `returnUrl=`; update
the logic in the block that checks `driveId` (the function using `driveId`,
`client`, `makeSharedDriveNoteReturnUrl`, and `file._id`) to only append
`?returnUrl=...` when `returnUrl` is non-empty: compute `returnUrl` via
`makeSharedDriveNoteReturnUrl(client, file)` only if `client` is truthy, then
conditionally add the `?returnUrl=encodeURIComponent(returnUrl)` suffix to
`/note/${driveId}/${file._id}` only when `returnUrl` has a value; otherwise
return the base `/note/${driveId}/${file._id}` without any empty query
parameter.
In `@src/modules/shareddrives/helpers.ts`:
- Around line 43-57: The makeSharedDriveNoteReturnUrl function uses
file.driveId! and file.dir_id directly, which can produce malformed URLs if
either is undefined; update this function to validate file.driveId and
file.dir_id at the start (e.g., if (!file.driveId || !file.dir_id) throw a
descriptive Error or normalize dir_id to ''/root as appropriate for your app),
remove the non-null assertion on file.driveId, and then build the hash using the
validated/normalized values when calling generateWebLink so the URL never
contains the literal "undefined".
---
Nitpick comments:
In `@src/modules/navigation/hooks/helpers.spec.js`:
- Around line 237-242: Add a unit test that exercises computePath when a
CozyClient-like `client` is provided: create a file object with _id, driveId
(and optional dir_id), mock a client exposing getStackClient and
getInstanceOptions (or stub generateWebLink from cozy-client), call
computePath(file, { type: 'public-note', pathname: '/public', client: mockClient
}) and assert the returned path still matches the
/note/:driveId/:id?returnUrl=... pattern and that the query contains a non-empty
encoded returnUrl (e.g., contains "https://" when decoded or contains
encodeURIComponent('https://')), ensuring the test covers the happy path with
client present; use the same test file and helper names (computePath,
generateWebLink) to locate code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d75d4cf-fce0-4c4b-87d6-b5b090a0152d
📒 Files selected for processing (5)
src/modules/navigation/PublicNoteRedirect.tsxsrc/modules/navigation/hooks/helpers.spec.jssrc/modules/navigation/hooks/helpers.tssrc/modules/navigation/hooks/useFileLink.tsxsrc/modules/shareddrives/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/navigation/PublicNoteRedirect.tsx
doubleface
approved these changes
Mar 18, 2026
zatteo
force-pushed
the
fix/shared-drive-recipient-note-return-url
branch
from
14e2a5f to
7eb3a8a
Compare
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/modules/navigation/hooks/helpers.ts (1)
120-127:⚠️ Potential issue | 🟡 MinorAvoid emitting an empty
returnUrlquery paramWhen
clientisnull, this still generates?returnUrl=. Build the base note URL first, and only appendreturnUrlwhen it is non-empty.Suggested fix
case 'public-note': if (driveId) { const returnUrl = client ? makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) : '' - return `/note/${driveId}/${file._id}?returnUrl=${encodeURIComponent( - returnUrl - )}` + const basePath = `/note/${driveId}/${file._id}` + return returnUrl + ? `${basePath}?returnUrl=${encodeURIComponent(returnUrl)}` + : basePath } else { return `/note/${file._id}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/navigation/hooks/helpers.ts` around lines 120 - 127, The URL builder emits an empty returnUrl query param when client is null; update the logic in the block that uses driveId, file, and makeSharedDriveNoteReturnUrl so you first construct the base path `/note/${driveId}/${file._id}` and then, only if client is truthy and makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) returns a non-empty string, append `?returnUrl=${encodeURIComponent(returnUrl)}`; ensure you do not append `?returnUrl=` when returnUrl is empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/modules/navigation/hooks/helpers.ts`:
- Around line 120-127: The URL builder emits an empty returnUrl query param when
client is null; update the logic in the block that uses driveId, file, and
makeSharedDriveNoteReturnUrl so you first construct the base path
`/note/${driveId}/${file._id}` and then, only if client is truthy and
makeSharedDriveNoteReturnUrl(client, file as IOCozyFile) returns a non-empty
string, append `?returnUrl=${encodeURIComponent(returnUrl)}`; ensure you do not
append `?returnUrl=` when returnUrl is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a476cbe-bbec-4360-811c-fdb03d985060
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.jsonsrc/modules/navigation/PublicNoteRedirect.tsxsrc/modules/navigation/hooks/helpers.spec.jssrc/modules/navigation/hooks/helpers.tssrc/modules/navigation/hooks/useFileLink.tsxsrc/modules/shareddrives/helpers.ts
✅ Files skipped from review due to trivial changes (2)
- src/modules/navigation/hooks/helpers.spec.js
- package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- src/modules/navigation/PublicNoteRedirect.tsx
- src/modules/shareddrives/helpers.ts
- src/modules/navigation/hooks/useFileLink.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Required
linagora/cozy-notes#501
linagora/cozy-client#1677
Summary by CodeRabbit
New Features
Bug Fixes