fix: Allow to pass a returnUrl when fetching public note URL#1677
fix: Allow to pass a returnUrl when fetching public note URL#1677
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe note model's 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cozy-client/src/models/note.js (1)
46-52: Add a regression test forfetchURLpublic-linkreturnUrlbehavior.Lines 50-51 introduce new branch logic, but the provided tests only demonstrate
returnUrlhandling forgeneratePrivateUrl(not thefetchURLsharecode path). Please add a focused test to lock this behavior (including encoding expectations) and prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-client/src/models/note.js` around lines 46 - 52, Add a regression test that verifies fetchURL's public-link (sharecode) path correctly includes and URL-encodes options.returnUrl: exercise the fetchURL (or the public-link branch that reads sharecode) code path with a returnUrl containing characters that require encoding, assert the generated URL has a returnUrl query parameter with the expected percent-encoding, and also test behavior when public_name is present so the searchParams branch (username/public_name) and returnUrl coexist; mirror the style of the existing generatePrivateUrl tests (same test harness/setup) to lock this behavior and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cozy-client/src/models/note.js`:
- Around line 46-52: Add a regression test that verifies fetchURL's public-link
(sharecode) path correctly includes and URL-encodes options.returnUrl: exercise
the fetchURL (or the public-link branch that reads sharecode) code path with a
returnUrl containing characters that require encoding, assert the generated URL
has a returnUrl query parameter with the expected percent-encoding, and also
test behavior when public_name is present so the searchParams branch
(username/public_name) and returnUrl coexist; mirror the style of the existing
generatePrivateUrl tests (same test harness/setup) to lock this behavior and
prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7eed5708-a084-421e-b0e5-a4ba0d6c2a3d
📒 Files selected for processing (1)
packages/cozy-client/src/models/note.js
zatteo
force-pushed
the
fix/allow-return-url-in-fetch-note
branch
from
4e28f92 to
49cbad7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cozy-client/types/models/note.d.ts`:
- Around line 3-7: Update the fetchURL type signature so the options properties
match implementation: mark pathname, driveId and returnUrl as optional in the
options object for export function fetchURL(client: object, file: object,
options?: { ... }) to prevent TS errors when callers omit them; locate the
fetchURL declaration in the note.d.ts and change those property types from
required string to optional (e.g. pathname?: string) so the declaration aligns
with the runtime checks (e.g. if (options.returnUrl)).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4e9cea3-4a55-41fe-9374-d08cbb9dcc79
📒 Files selected for processing (3)
docs/api/cozy-client/modules/models.note.mdpackages/cozy-client/src/models/note.jspackages/cozy-client/types/models/note.d.ts
✅ Files skipped from review due to trivial changes (1)
- docs/api/cozy-client/modules/models.note.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cozy-client/src/models/note.js (1)
8-8: Clarify thatreturnUrlcurrently applies only to share/public URLs.
returnUrlis documented as a generic option, but it is only used whensharecodeexists. Please clarify this in JSDoc (or add support in the non-sharecodebranch if that was intended).📝 Suggested clarification
- * `@property` {string} [returnUrl] - Return URL to add in query string + * `@property` {string} [returnUrl] - Return URL to add in query string (share/public URL flow)Also applies to: 58-60, 70-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cozy-client/src/models/note.js` at line 8, Update the JSDoc for the `returnUrl` property to state it is only applied when a `sharecode` (public/share URL) is present, or alternatively implement support for `returnUrl` in the non-`sharecode` branch if that was intended; locate references to `returnUrl` and the `sharecode` check in this module (the `returnUrl` property JSDoc and the URL-building code that checks `sharecode` around the other usages mentioned) and either change the comment to explicitly say "applies only to share/public URLs when `sharecode` is provided" or add the logic to append `returnUrl` in the non-`sharecode` path so behavior matches the existing generic documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cozy-client/src/models/note.js`:
- Line 7: Update the JSDoc for the driveId property in
packages/cozy-client/src/models/note.js: locate the `@property` entry for driveId
and correct the description text from "used to fetched the URL" to "used to
fetch the URL" so the grammar is fixed while preserving the rest of the comment.
---
Nitpick comments:
In `@packages/cozy-client/src/models/note.js`:
- Line 8: Update the JSDoc for the `returnUrl` property to state it is only
applied when a `sharecode` (public/share URL) is present, or alternatively
implement support for `returnUrl` in the non-`sharecode` branch if that was
intended; locate references to `returnUrl` and the `sharecode` check in this
module (the `returnUrl` property JSDoc and the URL-building code that checks
`sharecode` around the other usages mentioned) and either change the comment to
explicitly say "applies only to share/public URLs when `sharecode` is provided"
or add the logic to append `returnUrl` in the non-`sharecode` path so behavior
matches the existing generic documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a3d9b83-be46-41c8-a4c8-8bc2b6d9f595
📒 Files selected for processing (2)
packages/cozy-client/src/models/note.jspackages/cozy-client/types/models/note.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cozy-client/types/models/note.d.ts
zatteo
force-pushed
the
fix/allow-return-url-in-fetch-note
branch
from
3fa7300 to
f83be6b
Compare
zatteo
force-pushed
the
fix/allow-return-url-in-fetch-note
branch
from
f83be6b to
3abaf83
Compare
2 participants
Summary by CodeRabbit
New Features
Documentation