fix: include app version in private app install redirect #38286
Conversation
fix(admin): make Custom HTML card responsive on mobile
…n-loader fix(ui): add skeleton loading states to Room Contextual Bar views
…ptions fix: prevent startup crash on invalid MONGO_OPTIONS JSON
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughFixes sidebar removal safety, streamlines profile form layout/validation, ensures marketplace install redirects include app version, adds a contextual list skeleton component, improves Mongo config parsing and options, adds copy-to-clipboard for code blocks, and simplifies BaseRaw data-access surface and deletion rollback. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/packages/rocketchat-mongo-config/server/index.js (1)
20-29: PotentialTypeErrorifMONGO_URLis undefined.Line 22 calls
process.env.MONGO_URL.includes(...)without first checking ifMONGO_URLis defined. If it's undefined, this will throw aTypeError: Cannot read properties of undefined (reading 'includes').🐛 Proposed fix
const mongoConnectionOptions = { // add retryWrites=false if not present in MONGO_URL - ...(!process.env.MONGO_URL.includes('retryWrites') && { retryWrites: false }), + ...(!process.env.MONGO_URL?.includes('retryWrites') && { retryWrites: false }), ignoreUndefined: false,Alternatively, if
MONGO_URLis expected to always be set when this package loads, consider adding a guard at the top of the file that throws a descriptive error if it's missing.apps/meteor/client/views/account/profile/AccountProfileForm.tsx (1)
75-86: Remove unused email confirmation code.
mutateConfirmationEmailandhandleSendConfirmationEmailare dead code—they are defined but never called anywhere in the application. Remove both the mutation setup (lines 75-79) and the callback handler (lines 81-86).packages/models/src/models/BaseRaw.ts (1)
172-195: PropagateFindOneAndDeleteOptionsthrough the trash path and remove section banner comments.The trash path ignores
options(e.g.,session,collation,hint,projection), which breaks transactional correctness and can return a different document than the delete would have targeted. Thread the relevant options intofindOne,trash.updateOne, anddeleteOne.Additionally, remove the section banner comments (
/* ===================== FIXED METHOD ===================== */and/* ======================================================== */) as they violate the coding guideline to avoid code comments in TypeScript/JavaScript implementation.🔧 Suggested fix
- const doc = await this.col.findOne(filter); + const { session, projection, collation, hint } = options || {}; + const doc = await this.col.findOne(filter, { session, projection, collation, hint }); const { _id, ...record } = doc; const trash: TDeleted = { ...record, _deletedAt: new Date(), __collection__: this.name, } as unknown as TDeleted; - await this.trash.updateOne({ _id } as Filter<TDeleted>, { $set: trash } as UpdateFilter<TDeleted>, { - upsert: true, - }); + await this.trash.updateOne({ _id } as Filter<TDeleted>, { $set: trash } as UpdateFilter<TDeleted>, { + upsert: true, + session, + }); try { - await this.col.deleteOne({ _id } as Filter<T>); + await this.col.deleteOne({ _id } as Filter<T>, { session, collation, hint });
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx`:
- Line 211: The StatusText, Nickname, and Bio input fields currently call
field.onChange(e.target.value.trim()) which prevents typing spaces; change each
field to call field.onChange(e.target.value) on onChange and perform the
trimming on onBlur instead (e.g., onBlur ->
field.onChange(e.target.value.trim()) or call a trim handler) so users can type
multi-word values; update the JSX for the StatusText, Nickname, and Bio fields
accordingly to move trim from onChange to onBlur.
- Line 170: The current onChange handler in AccountProfileForm.tsx calls
field.onChange(e.target.value.trim()), which strips trailing spaces and prevents
typing multi-word values; change the onChange to call
field.onChange(e.target.value) (preserving user input) and perform trimming
either in an onBlur handler for the same input (e.g., onBlur={() =>
field.onChange(currentValue.trim())}) or centrally in the form submit handler
(e.g., when building data in handleSubmit, call .trim() on the relevant fields
before sending), keeping references to the same field.onChange and the form's
submit logic so behavior is consistent.
- Line 266: The email field's validation uses validateEmail which returns only a
boolean so react-hook-form has no message to display; update the rules for the
email input (the rules prop containing validate: validateEmail) to return a
string on failure (e.g., replace validate: validateEmail with validate: (v) =>
validateEmail(v) || t('Invalid_email')) and also ensure the TextInput for the
email displays errors by passing error={errors.email?.message} (mirror how name
uses errors.name?.message) so users see a descriptive "Invalid email" message
when validation fails.
In `@apps/meteor/client/views/marketplace/hooks/useInstallApp.tsx`:
- Around line 45-60: The onSuccess callback in useInstallApp.tsx currently
returns silently when data.app.version is falsy; instead, call the hook's
existing install error handler (the same handler used for other install
failures—e.g., setInstallError or onError within this hook) with a clear message
or Error object like "Missing app version" and then return; replace the silent
return in onSuccess (which destructures id and version from data.app) with a
call to that install error handler so the user receives feedback before exiting
the function.
In `@apps/meteor/packages/rocketchat-mongo-config/server/index.js`:
- Around line 37-41: In the catch block that handles parsing of MONGO_OPTIONS
(the code using mongoOptionStr), stop logging the raw mongoOptionStr to avoid
exposing secrets; replace the console.error line that prints the provided value
with a safe indicator such as the string length or a masked/sanitized summary
(e.g., "Value length: " + mongoOptionStr.length or a hash/partial mask), keep
the parse error message (error.message) and the contextual log lines, and ensure
you only reference mongoOptionStr for non-sensitive metadata rather than its
full content.
In `@packages/gazzodown/src/code/CodeBlock.tsx`:
- Around line 29-37: The handleCopy function sets a timeout that calls
setCopied(false) after 1500ms, which can trigger a state update after the
component unmounts; to fix it, store the timeout id in a ref (e.g., timeoutRef)
inside the component and clear that timeout in a useEffect cleanup, or track
mounted state with an isMountedRef and check it before calling setCopied(false);
update the handleCopy callback to save the timer id to timeoutRef and ensure
useEffect returns a cleanup that clearsTimeout(timeoutRef.current) and/or sets
isMountedRef.current = false so no state updates occur after unmount.
In `@packages/models/src/models/BaseRaw.ts`:
- Around line 195-216: The rollback code in BaseRaw.findOneAndDelete can remove
the last copy if deleteOne failed after the server already deleted the original;
to fix it, extract session from the passed options and ensure you pass the
options (including session) to all MongoDB calls (the initial findOneAndDelete,
the insertion to trash and the rollback this.trash.deleteOne), and before
deleting the trash record check whether the original document still exists
(e.g., a findOne({_id}, {session}) or count)—only delete the trash if the
original is confirmed missing; also propagate options/session in all relevant
operations to support transactions.
🧹 Nitpick comments (3)
packages/gazzodown/src/code/CodeBlock.tsx (2)
49-52: Minor: Unnecessary fragment wrapper.The fragment around
headon line 51 is redundant.Suggested simplification
return ( <> - <>{head}</> + {head} {chunks.map((chunk, index) =>
81-96: Consider extracting inline styles and adding focus/hover states.The inline styles reduce maintainability and the button lacks visible focus/hover states for keyboard navigation accessibility. Consider using a CSS class or the project's styling solution (e.g.,
@rocket.chat/fuselagecomponents if available) to handle these styles and states consistently.packages/models/src/models/BaseRaw.ts (1)
170-170: Remove section banner comments in implementation.These banner comments violate the “no implementation comments” guideline for TS/JS. As per coding guidelines, please remove them.
🧹 Suggested cleanup
- /* ===================== FIXED METHOD ===================== */ + @@ - /* ======================================================== */ +Also applies to: 222-222
| <TextInput | ||
| {...field} | ||
| id={nameId} | ||
| onChange={(e) => field.onChange(e.target.value.trim())} |
There was a problem hiding this comment.
Trimming on onChange prevents typing multi-word values.
Calling trim() on every change removes trailing spaces immediately, making it impossible for users to type multi-word names like "John Doe". When the user types "John " and presses space, the value is trimmed to "John", so the next character appends directly without a space.
Consider trimming on blur or on submit instead:
🐛 Proposed fix
<TextInput
{...field}
id={nameId}
- onChange={(e) => field.onChange(e.target.value.trim())}
+ onChange={(e) => field.onChange(e.target.value)}
+ onBlur={(e) => { field.onChange(e.target.value.trim()); field.onBlur(); }}
error={errors.name?.message}
disabled={!allowRealNameChange}
/>📝 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.
| onChange={(e) => field.onChange(e.target.value.trim())} | |
| <TextInput | |
| {...field} | |
| id={nameId} | |
| onChange={(e) => field.onChange(e.target.value)} | |
| onBlur={(e) => { field.onChange(e.target.value.trim()); field.onBlur(); }} | |
| error={errors.name?.message} | |
| disabled={!allowRealNameChange} | |
| /> |
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` at line 170,
The current onChange handler in AccountProfileForm.tsx calls
field.onChange(e.target.value.trim()), which strips trailing spaces and prevents
typing multi-word values; change the onChange to call
field.onChange(e.target.value) (preserving user input) and perform trimming
either in an onBlur handler for the same input (e.g., onBlur={() =>
field.onChange(currentValue.trim())}) or centrally in the form submit handler
(e.g., when building data in handleSubmit, call .trim() on the relevant fields
before sending), keeping references to the same field.onChange and the form's
submit logic so behavior is consistent.
| {...field} | ||
| id={statusTextId} | ||
| error={errors?.statusText?.message} | ||
| onChange={(e) => field.onChange(e.target.value.trim())} |
There was a problem hiding this comment.
Same trim issue affects StatusText, Nickname, and Bio fields.
These fields also use trim() on onChange, which will prevent users from typing multi-word values. Apply the same fix (trim on blur instead of change) to these fields.
Lines affected:
- Line 211: StatusText
- Line 235: Nickname
- Line 252: Bio
Also applies to: 235-235, 252-252
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` at line 211,
The StatusText, Nickname, and Bio input fields currently call
field.onChange(e.target.value.trim()) which prevents typing spaces; change each
field to call field.onChange(e.target.value) on onChange and perform the
trimming on onBlur instead (e.g., onBlur ->
field.onChange(e.target.value.trim()) or call a trim handler) so users can type
multi-word values; update the JSX for the StatusText, Nickname, and Bio fields
accordingly to move trim from onChange to onBlur.
| required: t('Required_field', { field: t('Email') }), | ||
| validate: { validateEmail: (email) => (validateEmail(email) ? undefined : t('error-invalid-email-address')) }, | ||
| }} | ||
| rules={{ required: t('Required_field', { field: t('Email') }), validate: validateEmail }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find validateEmail in the tools package
find . -path "*/packages/tools/src/*" -name "*.ts" -o -name "*.js" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1010
🏁 Script executed:
# Search for validateEmail definition
rg -n "validateEmail" packages/tools/src/ -A 5 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 882
🏁 Script executed:
# Also check if there's an index file that exports it
fd -e ts -e js "index" packages/tools/src/ | xargs grep -l "validateEmail" 2>/dev/null || echo "Not found in index files"Repository: RocketChat/Rocket.Chat
Length of output: 93
🏁 Script executed:
# Read the complete validateEmail function
cat packages/tools/src/validateEmail.tsRepository: RocketChat/Rocket.Chat
Length of output: 527
🏁 Script executed:
# Check the AccountProfileForm.tsx file around line 266 and surrounding validation logic
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | sed -n '250,280p'Repository: RocketChat/Rocket.Chat
Length of output: 1195
🏁 Script executed:
# Check if there's any custom error message handling or field-level error display
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | sed -n '1,50p'Repository: RocketChat/Rocket.Chat
Length of output: 2204
🏁 Script executed:
# Check if there's error rendering for the email field and how FieldError is used
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | sed -n '260,290p'Repository: RocketChat/Rocket.Chat
Length of output: 1052
🏁 Script executed:
# Search for FieldError usage in this file to understand error display pattern
rg -n "FieldError" apps/meteor/client/views/account/profile/AccountProfileForm.tsx -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 137
🏁 Script executed:
# Check other field validations in the form to see the error handling pattern
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | grep -A 10 "FieldError" | head -30Repository: RocketChat/Rocket.Chat
Length of output: 369
🏁 Script executed:
# Look for all Controller usages to see if any show error display pattern
ast-grep --pattern 'Controller
control={$_}
name={$_}
rules={$_}
render={({ field, fieldState }) => (
$$$
)}
/'Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search more broadly for error handling in the Controller renders
rg -n "fieldState|formState" apps/meteor/client/views/account/profile/AccountProfileForm.tsx -B 5 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 356
🏁 Script executed:
# Check if there are other fields showing error messages with FieldError
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | sed -n '100,200p'Repository: RocketChat/Rocket.Chat
Length of output: 3633
🏁 Script executed:
# Look at a larger section to see if any field uses FieldError to display validation errors
rg -n "FieldError" apps/meteor/client/views/account/profile/AccountProfileForm.tsx -B 10 -A 3Repository: RocketChat/Rocket.Chat
Length of output: 298
🏁 Script executed:
# Check validateUsername to see if it has similar issues
rg -n "validateUsername" packages/tools/src/ -A 5 -B 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if there's a pattern in how other fields handle validation with boolean returns
rg -n "validate:" apps/meteor/client/views/account/profile/AccountProfileForm.tsx -B 2 -A 5Repository: RocketChat/Rocket.Chat
Length of output: 752
🏁 Script executed:
# Check if there's global error handling or if errors are displayed elsewhere
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | sed -n '260,290p'Repository: RocketChat/Rocket.Chat
Length of output: 1052
🏁 Script executed:
# Look at the entire render to understand if email field error is displayed somewhere
rg -n "email" apps/meteor/client/views/account/profile/AccountProfileForm.tsx | grep -i errorRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check react-hook-form behavior when validate returns false without custom message
# by looking at how the form uses formState errors
cat -n apps/meteor/client/views/account/profile/AccountProfileForm.tsx | sed -n '60,75p'Repository: RocketChat/Rocket.Chat
Length of output: 594
Provide a descriptive error message for invalid emails.
The validate callback for the email field uses validateEmail which returns only a boolean. When validation fails, react-hook-form has no error message to display. Additionally, the email field's TextInput doesn't have an error prop to show validation feedback, unlike the name field which displays errors.name?.message. Users won't know why their email was rejected.
Consider either:
- Wrapping
validateEmailto return a string error message (e.g.,(email) => validateEmail(email) || t('Invalid_email')) - Adding error display to the email TextInput with
error={errors.email?.message}
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/account/profile/AccountProfileForm.tsx` at line 266,
The email field's validation uses validateEmail which returns only a boolean so
react-hook-form has no message to display; update the rules for the email input
(the rules prop containing validate: validateEmail) to return a string on
failure (e.g., replace validate: validateEmail with validate: (v) =>
validateEmail(v) || t('Invalid_email')) and also ensure the TextInput for the
email displays errors by passing error={errors.email?.message} (mirror how name
uses errors.name?.message) so users see a descriptive "Invalid email" message
when validation fails.
| onSuccess: (data: { app: App }) => { | ||
| const { id, version } = data.app; | ||
|
|
||
| if (!version) { | ||
| return; | ||
| } | ||
|
|
||
| router.navigate({ | ||
| name: 'marketplace', | ||
| params: { | ||
| context: 'private', | ||
| page: 'info', | ||
| id: data.app.id, | ||
| id, | ||
| version, | ||
| tab: 'details', | ||
| }, |
There was a problem hiding this comment.
Handle missing version with user feedback.
If version is absent, the flow silently returns, which can leave the user without guidance. Consider surfacing an error via the existing install error handler so the UX isn’t a no-op.
Proposed fix
- if (!version) {
- return;
- }
+ if (!version) {
+ handleInstallError(new Error('Installed app version missing'));
+ return;
+ }📝 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.
| onSuccess: (data: { app: App }) => { | |
| const { id, version } = data.app; | |
| if (!version) { | |
| return; | |
| } | |
| router.navigate({ | |
| name: 'marketplace', | |
| params: { | |
| context: 'private', | |
| page: 'info', | |
| id: data.app.id, | |
| id, | |
| version, | |
| tab: 'details', | |
| }, | |
| onSuccess: (data: { app: App }) => { | |
| const { id, version } = data.app; | |
| if (!version) { | |
| handleInstallError(new Error('Installed app version missing')); | |
| return; | |
| } | |
| router.navigate({ | |
| name: 'marketplace', | |
| params: { | |
| context: 'private', | |
| page: 'info', | |
| id, | |
| version, | |
| tab: 'details', | |
| }, |
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/marketplace/hooks/useInstallApp.tsx` around lines 45
- 60, The onSuccess callback in useInstallApp.tsx currently returns silently
when data.app.version is falsy; instead, call the hook's existing install error
handler (the same handler used for other install failures—e.g., setInstallError
or onError within this hook) with a clear message or Error object like "Missing
app version" and then return; replace the silent return in onSuccess (which
destructures id and version from data.app) with a call to that install error
handler so the user receives feedback before exiting the function.
| } catch (error) { | ||
| console.error('Failed to parse MONGO_OPTIONS environment variable'); | ||
| console.error('Provided value:', mongoOptionStr); | ||
| console.error('Error:', error.message); | ||
| console.error('Please ensure MONGO_OPTIONS contains valid JSON'); |
There was a problem hiding this comment.
Avoid logging potentially sensitive configuration values.
Line 39 logs the raw MONGO_OPTIONS value which could contain sensitive information such as credentials, API keys, or connection strings. This could expose secrets in logs.
🔒 Proposed fix
} catch (error) {
console.error('Failed to parse MONGO_OPTIONS environment variable');
- console.error('Provided value:', mongoOptionStr);
console.error('Error:', error.message);
console.error('Please ensure MONGO_OPTIONS contains valid JSON');If debugging is needed, consider logging only the length or a sanitized indicator (e.g., console.error('Value length:', mongoOptionStr.length)).
📝 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.
| } catch (error) { | |
| console.error('Failed to parse MONGO_OPTIONS environment variable'); | |
| console.error('Provided value:', mongoOptionStr); | |
| console.error('Error:', error.message); | |
| console.error('Please ensure MONGO_OPTIONS contains valid JSON'); | |
| } catch (error) { | |
| console.error('Failed to parse MONGO_OPTIONS environment variable'); | |
| console.error('Error:', error.message); | |
| console.error('Please ensure MONGO_OPTIONS contains valid JSON'); |
🤖 Prompt for AI Agents
In `@apps/meteor/packages/rocketchat-mongo-config/server/index.js` around lines 37
- 41, In the catch block that handles parsing of MONGO_OPTIONS (the code using
mongoOptionStr), stop logging the raw mongoOptionStr to avoid exposing secrets;
replace the console.error line that prints the provided value with a safe
indicator such as the string length or a masked/sanitized summary (e.g., "Value
length: " + mongoOptionStr.length or a hash/partial mask), keep the parse error
message (error.message) and the contextual log lines, and ensure you only
reference mongoOptionStr for non-sensitive metadata rather than its full
content.
| const handleCopy = useCallback(async () => { | ||
| try { | ||
| await navigator.clipboard.writeText(code); | ||
| setCopied(true); | ||
| setTimeout(() => setCopied(false), 1500); | ||
| } catch (error) { | ||
| console.error('Failed to copy code block', error); | ||
| } | ||
| }, [code]); |
There was a problem hiding this comment.
Potential state update on unmounted component.
The setTimeout in handleCopy isn't cleared if the component unmounts before the 1500ms elapses. This can cause a React warning and a no-op state update.
Proposed fix using a ref to track mounted state
const CodeBlock = ({ lines = [], language }: CodeBlockProps): ReactElement => {
const ref = useRef<HTMLElement>(null);
const [copied, setCopied] = useState(false);
+ const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const { highlightRegex } = useContext(MarkupInteractionContext);
const code = useMemo(() => lines.map((line) => line.value.value).join('\n'), [lines]);
const handleCopy = useCallback(async () => {
try {
await navigator.clipboard.writeText(code);
setCopied(true);
- setTimeout(() => setCopied(false), 1500);
+ timeoutRef.current = setTimeout(() => setCopied(false), 1500);
} catch (error) {
console.error('Failed to copy code block', error);
}
}, [code]);
+
+ useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+ }, []);🤖 Prompt for AI Agents
In `@packages/gazzodown/src/code/CodeBlock.tsx` around lines 29 - 37, The
handleCopy function sets a timeout that calls setCopied(false) after 1500ms,
which can trigger a state update after the component unmounts; to fix it, store
the timeout id in a ref (e.g., timeoutRef) inside the component and clear that
timeout in a useEffect cleanup, or track mounted state with an isMountedRef and
check it before calling setCopied(false); update the handleCopy callback to save
the timer id to timeoutRef and ensure useEffect returns a cleanup that
clearsTimeout(timeoutRef.current) and/or sets isMountedRef.current = false so no
state updates occur after unmount.
| } catch (originalError) { | ||
| try { | ||
| const rollbackResult = await this.trash.deleteOne({ _id } as Filter<TDeleted>); | ||
|
|
||
| if (!rollbackResult || rollbackResult.deletedCount !== 1) { | ||
| console.error('BaseRaw.findOneAndDelete: Rollback failed – orphaned trash record', { | ||
| collection: this.name, | ||
| _id, | ||
| rollbackResult, | ||
| originalError, | ||
| }); | ||
| } | ||
| } catch (rollbackError) { | ||
| console.error('BaseRaw.findOneAndDelete: Rollback threw exception', { | ||
| collection: this.name, | ||
| _id, | ||
| rollbackError, | ||
| originalError, | ||
| }); | ||
| } | ||
| return this.col.deleteMany(filter); | ||
| } | ||
|
|
||
| const cursor = this.find<ResultFields<T, C>>(filter, { session: options?.session }); | ||
|
|
||
| const ids: T['_id'][] = []; | ||
| for await (const doc of cursor) { | ||
| const { _id, ...record } = doc as T; | ||
|
|
||
| const trash: TDeleted = { | ||
| ...record, | ||
| _deletedAt: new Date(), | ||
| __collection__: this.name, | ||
| } as unknown as TDeleted; | ||
|
|
||
| ids.push(_id as T['_id']); | ||
|
|
||
| // since the operation is not atomic, we need to make sure that the record is not already deleted/inserted | ||
| await this.trash?.updateOne({ _id } as Filter<TDeleted>, { $set: trash } as UpdateFilter<TDeleted>, { | ||
| upsert: true, | ||
| session: options?.session, | ||
| }); | ||
|
|
||
| void options?.onTrash?.(doc); | ||
| } | ||
|
|
||
| if (options) { | ||
| return this.col.deleteMany({ _id: { $in: ids } } as unknown as Filter<T>, options); | ||
| } | ||
| return this.col.deleteMany({ _id: { $in: ids } } as unknown as Filter<T>); | ||
| } | ||
|
|
||
| // Trash | ||
| trashFind<P extends TDeleted>( | ||
| query: Filter<TDeleted>, | ||
| options?: FindOptions<P extends TDeleted ? TDeleted : P>, | ||
| ): FindCursor<WithId<TDeleted>> | undefined { | ||
| if (!this.trash) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (options) { | ||
| return this.trash.find( | ||
| { | ||
| __collection__: this.name, | ||
| ...query, | ||
| }, | ||
| options, | ||
| ); | ||
| throw originalError; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file
fd -t f "BaseRaw.ts" --exactRepository: RocketChat/Rocket.Chat
Length of output: 290
🏁 Script executed:
# Look at the full findOneAndDelete method with context
sed -n '150,250p' packages/models/src/models/BaseRaw.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 3686
🏁 Script executed:
# Also check the file structure to understand the class definition
head -100 packages/models/src/models/BaseRaw.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 3593
🏁 Script executed:
# Check the type definition for FindOneAndDeleteOptions
rg "FindOneAndDeleteOptions" -A 3 --type tsRepository: RocketChat/Rocket.Chat
Length of output: 1438
🏁 Script executed:
# Look for session usage patterns in the file
rg "session" packages/models/src/models/BaseRaw.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if options parameter contains session elsewhere
rg "options.*session" packages/models/src/models/BaseRaw.ts -B 1 -A 1Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check MongoDB type definition - see if FindOneAndDeleteOptions has session
rg "interface FindOneAndDeleteOptions|type FindOneAndDeleteOptions" -B 2 -A 10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check how other methods use session in this file, search broader
grep -n "session" packages/models/src/models/BaseRaw.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Let's check the MongoDB driver's FindOneAndDeleteOptions type
# by looking at node_modules or type stubs
find . -name "*.d.ts" -type f -exec grep -l "FindOneAndDeleteOptions" {} \; 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the actual usage of options parameter in the method
sed -n '23,71p' packages/models/src/models/BaseRaw.ts | grep -n "options"Repository: RocketChat/Rocket.Chat
Length of output: 374
🏁 Script executed:
# Look at MongoDB's FindOneAndDeleteOptions to understand session support
# Check if there are type definitions in node_modules
ls -la node_modules/mongodb/lib/typings/*.d.ts 2>/dev/null | head -10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Try to find where FindOneAndDeleteOptions is defined
fd "index.d.ts" node_modules/mongodb --type f 2>/dev/null | head -1 | xargs grep -A 20 "interface FindOneAndDeleteOptions"Repository: RocketChat/Rocket.Chat
Length of output: 50
🏁 Script executed:
# Look at how deleteOne is called with options in the method
sed -n '40,46p' packages/models/src/models/BaseRaw.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 279
🏁 Script executed:
# Check if there are other methods in the file that pass options to MongoDB operations
rg "await this\.(col|trash)\." packages/models/src/models/BaseRaw.ts -A 1 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 418
🏁 Script executed:
# Search for how session is passed in other operations
rg "\{ session" packages/models/src/models/BaseRaw.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the actual MongoDB package.json to understand version
cat package.json | grep -A 2 '"mongodb"'Repository: RocketChat/Rocket.Chat
Length of output: 117
🏁 Script executed:
# Check MongoDB 6.10.0 types - specifically FindOneAndDeleteOptions
cd node_modules/mongodb && find . -name "*.d.ts" -type f | xargs grep -l "FindOneAndDeleteOptions" | head -1 | xargs cat | grep -A 30 "interface FindOneAndDeleteOptions"Repository: RocketChat/Rocket.Chat
Length of output: 136
🏁 Script executed:
# Let's verify the actual concern - can a deleteOne fail after succeeding on server?
# Check MongoDB documentation references in comments or issues
rg "deleteOne|delete.*error" packages/models/src/models/BaseRaw.ts -B 1 -A 1Repository: RocketChat/Rocket.Chat
Length of output: 246
🏁 Script executed:
# Check if there are any tests for this method
fd -t f "*.test.ts" -o "*.spec.ts" | xargs grep -l "findOneAndDelete" 2>/dev/null | head -3Repository: RocketChat/Rocket.Chat
Length of output: 203
🏁 Script executed:
# Find test files for this class
fd -type f -name "*.test.*" | xargs grep -l "findOneAndDelete" 2>/dev/null | head -3Repository: RocketChat/Rocket.Chat
Length of output: 236
🏁 Script executed:
# Check if there's MongoDB documentation or comments about deleteOne error handling
rg "transient|network|stepdown|race condition" packages/models/src/models/ -iRepository: RocketChat/Rocket.Chat
Length of output: 189
🏁 Script executed:
# Let's verify the suggested fix - check if options parameter can have session
# by looking at how deleteOne is called elsewhere
rg "deleteOne.*options" packages/models/src/models/ -B 1 -A 1Repository: RocketChat/Rocket.Chat
Length of output: 649
🏁 Script executed:
# Check the actual line numbers mentioned in the review to match with current code
wc -l packages/models/src/models/BaseRaw.tsRepository: RocketChat/Rocket.Chat
Length of output: 107
🏁 Script executed:
# The file has 241 lines total. Let's look at the actual lines 195-216 mentioned in the review
sed -n '185,225p' packages/models/src/models/BaseRaw.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 1428
🏁 Script executed:
# Check the full findOneAndDelete method implementation to see how options are used
sed -n '165,225p' packages/models/src/models/BaseRaw.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 2226
🏁 Script executed:
# Check MongoDB documentation about session support in DeleteResult
# and verify what the race condition scenario looks like
rg "options" packages/models/src/models/BaseRaw.ts | grep -A 2 -B 2 "deleteOne\|findOne"Repository: RocketChat/Rocket.Chat
Length of output: 1419
🏁 Script executed:
# Verify how the method should pass options to the MongoDB operations
rg "col\.deleteOne|trash\.deleteOne" packages/models/src/models/ -B 1 -A 1Repository: RocketChat/Rocket.Chat
Length of output: 1131
Rollback can drop the last copy on ambiguous delete errors.
If deleteOne throws after the server actually deleted the document (e.g., transient network/primary stepdown), the rollback deletes the trash record and data is lost. Additionally, the method receives an options parameter but never passes it to any MongoDB operations, preventing session/transaction support. Consider verifying the document still exists before rolling back, passing options to all operations, and extracting the session from options if available.
🔧 Suggested approach
Check that the document still exists before deleting the trash record. Extract session from the options parameter and pass it to MongoDB operations:
try {
- await this.col.deleteOne({ _id } as Filter<T>);
+ await this.col.deleteOne({ _id } as Filter<T>, options || {});
} catch (originalError) {
try {
+ const stillExists = await this.col.findOne({ _id } as Filter<T>, options || {});
+ if (stillExists) {
- const rollbackResult = await this.trash.deleteOne({ _id } as Filter<TDeleted>);
+ const rollbackResult = await this.trash.deleteOne({ _id } as Filter<TDeleted>);
-
- if (!rollbackResult || rollbackResult.deletedCount !== 1) {
- console.error('BaseRaw.findOneAndDelete: Rollback failed – orphaned trash record', {
+ if (!rollbackResult || rollbackResult.deletedCount !== 1) {
+ console.error('BaseRaw.findOneAndDelete: Rollback failed – orphaned trash record', {
+ collection: this.name,
+ _id,
+ rollbackResult,
+ originalError,
+ });
+ }
+ } else {
+ console.error('BaseRaw.findOneAndDelete: Delete outcome uncertain; keeping trash record', {
collection: this.name,
_id,
- rollbackResult,
originalError,
});
+ }🤖 Prompt for AI Agents
In `@packages/models/src/models/BaseRaw.ts` around lines 195 - 216, The rollback
code in BaseRaw.findOneAndDelete can remove the last copy if deleteOne failed
after the server already deleted the original; to fix it, extract session from
the passed options and ensure you pass the options (including session) to all
MongoDB calls (the initial findOneAndDelete, the insertion to trash and the
rollback this.trash.deleteOne), and before deleting the trash record check
whether the original document still exists (e.g., a findOne({_id}, {session}) or
count)—only delete the trash if the original is confirmed missing; also
propagate options/session in all relevant operations to support transactions.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
2 participants
Proposed changes (including videos or screenshots)
This PR fixes an incorrect redirection that occurs after installing a private app.
Previously, the user was redirected to a marketplace URL that did not include the
app version, causing the router to misinterpret subsequent tab navigation (e.g.
Settings being treated as the version parameter).
The fix ensures that the redirect includes the app version and a default tab,
allowing all marketplace tabs (Details, Settings, etc.) to work correctly.
Issue(s)
Fixes #38215
Steps to test or reproduce
Further comments
This change is limited to the post-install navigation logic and does not affect
the app installation process itself. It provides a safe and minimal fix to ensure
correct routing behavior in the marketplace UI.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.