Repository navigation
Production audit – Batch 1 (Security, Backend & API fixes) - #2
Merged
Merged
Conversation
Two rules, each reachable by more than one route, which is what most of this
change is about.
An admin creates MEMBERS, never admins. POST /org-admin/users offered
role=admin and honoured it. That is not just a policy preference: an admin with
no admin_departments rows is ORGANIZATION-WIDE by definition (managed_dept_ids
returns None for them), so a single-department admin could mint an admin
account with reach over the whole org and sign in as it, since they choose its
password. Department scoping was therefore decorative — one POST away from
being bypassed. Granting the admin role stays a superadmin action, where the
actor is already org-wide.
Blocking the create alone would have been a one-line detour, so PUT
/users/{id} with role=admin is refused too. Demotion is deliberately still
allowed: removing reach is not escalation, and an admin who has lost their post
needs to be demotable by whoever is on shift. The check is on the CHANGE, not
the requested value — the edit drawer sends every field on save, so keying on
role=admin alone would have made renaming an existing admin impossible.
An admin places people only in departments they administer. POST /users
validated the department against the ORG but never against the admin's scope,
so a Ward-only admin could create staff in Pharmacy. PUT /users/{id} had the
same hole and a worse consequence: moving an account to a department you do not
manage is a one-way trip, since the target then fails _load_org_user and the
admin cannot undo their own edit. Both now resolve through one helper, so a new
route inherits the check rather than having to remember it.
Out of scope answers 404, not 403, matching how this module already treats
another tenant's objects. 403 would let an admin enumerate the departments they
do not manage by watching which ids answer differently.
Scoping the writes alone would have left the console lying. GET /departments IS
the department dropdown in both forms, so it is scoped now — otherwise the
restriction only shows up as a 404 after the admin has filled the form in. The
dashboard counts are scoped for the same reason: "8 departments" beside a
listing of two reads as a broken listing, not as scope. total_conversations
stays org-wide because a conversation has participants, not a department, and a
per-department count would double-count cross-department threads.
The activity feed is filtered too, and that one is a confidentiality fix rather
than a cosmetic one: an audit row's `target` is the affected person's EMAIL, so
an unscoped feed handed a department-scoped admin the addresses of the very
people their user list was hiding. Filtered on actor, since audit_logs stores
who acted but not who was acted upon as an id. Own rows always survive the
filter — an admin must be able to see what they themselves did.
A scoped admin also cannot create departments. Not a security boundary but a
coherence one: the new department would fall outside their scope the instant it
existed, so it would vanish from their own listing and they could neither staff
it nor rename it. Auto-granting it to them would be worse — self-service scope
expansion.
Console: /auth/me and /auth/login now carry managed_departments for org admins,
so the UI can hide what the API would refuse instead of letting someone fill in
a form and collect an error. Attached to LOGIN as well as /me because
AuthContext sets the user straight from the login response, so a scoped admin
would otherwise be shown org-wide controls until their next page load. An empty
list means org-wide; the key is absent entirely for members and superadmins, so
nobody reads "no departments" as "manages nothing".
The create form drops its role picker and says what it does instead. The edit
drawer disables the admin option unless the target already is one, which is
what keeps demotion reachable. The users page names the departments it is
scoped to, so a short list reads as scope rather than as missing people.
Every org_admin in production today has no admin_departments rows and is
therefore org-wide, so nothing about their reach changes; only the role
restriction applies to them. That path is pinned by its own test.
12 new tests. 164 pass, and verified against the running stack with a real
scoped admin: all ten allow/deny cases behave, including the two-step move and
the save-unchanged no-op.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The iOS admin screen cannot create users — there is no POST /org-admin/users in RxHiveAPI — but it can promote and it can move someone between departments, the two update routes the previous commit locked down. The server refuses both now, so this is not an enforcement gap; the buttons just still looked live and would have failed with an unexplained error toast. The Admin segment is disabled unless the target already is one, matching the web drawer, and for the same reason: demotion is still allowed, so the current role needs a selected state to demote FROM. The department menu needed no change. It is fed by GET /org-admin/departments, which is now scoped server-side, so a department-scoped admin no longer sees departments the API would 404 on. Also corrects the caption. "Admins can see this screen and manage everyone in the organisation" stopped being true when a super admin could scope an admin to named departments. Verified by `swiftc -parse` only. The iOS target still cannot be built here — xcodebuild wants `sudo xcodebuild -license accept`, which needs a password I do not have — so this is unverified beyond syntax, and it sits in its own commit rather than alongside the backend and web work, which was tested end to end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everyone in an organisation may message everyone else in it, and may send any format at any size up to 2 GB. The super admin's Access Control page is gone, along with the engine behind it. Reachability. services/access.py and its deny-by-default rule evaluation are deleted. What replaces them is one line at conversations.get_or_create_direct: same organisation. That check is restated there rather than inherited, because it used to be the FIRST CLAUSE of can_converse — deleting the rules engine without it would have quietly opened direct conversations between tenants, which is a different feature (cross-org groups) with its own membership model. The per-send re-check in messaging.py goes entirely: membership of a conversation is now the permission, and the org check already sitting above it is what keeps tenants apart. Contacts and search lose their reachability filters and fall back to what their queries already said — everyone active in my org. Send policy. The per-category toggles and the document whitelist are gone from the claim path and from forwarding. GET /users/me/send-policy is gone; nothing resolves a policy any more. File types. The upload path had a SECOND, older restriction that had nothing to do with the access-control feature: a hard-coded extension allow-list in storage.py. Removing only the super admin page would have left .psd, .dwg, .rar and .exe rejected with "File type not supported", so classify() no longer rejects anything — the four extension sets are now purely a rendering hint deciding which bubble a file lands in, and anything unrecognised is a document. What that makes load-bearing is the content type. Media is served from a SAME-ORIGIN path with Content-Disposition: inline, so a stored type the browser renders is stored XSS on the app's own origin: an uploaded .html or .svg would execute as us. MIME_BY_EXT already defaulted to application/octet-stream, which makes the browser download instead, and that default is now the thing standing between "any format" and a scripting vector. There is a test pinning it, and a comment saying not to replace it with mimetypes.guess_type. Size. One ceiling of 2 GB instead of 16/200/100 MB per category. This could not just be a bigger number: the route accumulated the whole upload into a bytearray, so a 2 GB file meant 2 GB resident in the API process — for every concurrent upload, taking the box down for everyone. Starlette has already spooled the body to a temp file on disk past 1 MB, so the handler now hands that file object straight to S3 via a new storage.put_stream and never holds the payload. Reading bytes into memory survives in exactly one place, thumbnailing, which genuinely needs a buffer; past 64 MB the upload succeeds without a preview rather than risking the process. The honest remaining cost is disk, not memory: a 2 GB upload occupies the instance's temp space for the length of the request. Schema. chat_access_rules, send_policies and admin_departments are dropped, with the access_party_type enum. The migration reports the row counts it destroys, and downgrade() recreates the schema EMPTY — the rules themselves are not recoverable. Dropping admin_departments changes behaviour rather than merely removing an unused table, and the choice was made explicitly: managed_dept_ids read "no rows" as ORGANISATION-WIDE, so every org admin goes back to managing their whole organisation. Department scoping is no longer expressible. All the scope filters added with it — user list, department list, dashboard counts, activity feed — come back out. Kept, because it never depended on departments: an admin creates members, not admins, and cannot promote one either. Extended, because of a review finding that survives the removal: _load_org_user now refuses any target who is not a member. Without it "admins cannot create admins" was decorative — reset-password returns the new password in its own response body, so an admin who could not MINT an admin could simply TAKE one, and one call against a peer handed over a working login. Blocking creation while leaving seizure open protects nothing. Peer admins stay VISIBLE in the user list (you need to know who to escalate to) and an admin can still manage their own row; only mutating someone else's admin account is refused, which now includes demotion. That is deliberate: removing an admin joins granting one as a superadmin action. iOS needed no commit. Its send-policy awareness was the uncommitted Phase 7 work, so removing it left five files byte-identical to HEAD and SendPolicyStore.swift, which was never tracked, simply gone. Verified: 135 backend tests pass. The migration was run against a real Postgres (2 rules destroyed) and round-tripped down and up. Against the running stack: two users hold a conversation with zero rules in the database, .exe/.psd/.dwg upload and .html stores as octet-stream, a 12 MB file goes through the streaming path, creating an admin is still refused, and both removed endpoints 404. The built bundle contains no Access Control nav entry, route, send-policy fetch or access_changed handler, and the composer offers all three attach options with a document picker that accepts anything. iOS type-checks with zero errors across 51 files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Parity items 17-20, 22, 23. The timestamp and ticks used to be one shared block
emitted below every kind of content, which is why a one-word message was two
rows tall, why every photo carried a strip of empty bubble under it, and why a
260px audio card and a 280px document card were each followed by a full-width
band of green.
MessageFooter is extracted so it can be handed to four different places:
beside text — a sibling in a bottom-aligned row, so "hi 10:24 AM ✓" is one
line. Deliberately not flex-1: trailing alignment already comes from
the row's justify-end, and stretching it is what pushed every bubble
to the full column width on iOS.
overlaid photo/video — absolutely positioned in the media's bottom-right
inside a black/45 scrim. The scrim is not decoration: a photo can be
white exactly where the timestamp lands.
inline a CAPTIONED photo/video, and the unavailable case — there is nothing
to sit beside or on top of, so it keeps its own row.
passed audio/document — handed to the card as a prop and rendered on a row
the card already had.
Two things fell out of doing this that are worth naming separately.
ImageBubble's overlay wrapper carries a min-height. An overlaid footer is
positioned against that box, so a picture that renders with NO height — a 404, a
decode failure, a degenerate 1x1 — let the scrim escape the bubble and land on
the message below it. Caught in the first screenshot, because the test fixture
was a 1x1 PNG; a broken image in production would have done the same.
VideoBubble is new, and covers three items at once. It opens
FullscreenVideoViewer, which already existed and was already wired to the
gallery and to starred messages — the chat bubble was the one caller that never
reached it, so a video in the thread was the only medium with no filename, no
download and no jump-to-message (item 15). It generates a poster frame at 0.5s
client-side, because the upload service only thumbnails images and PDFs, so
`poster` was always undefined and the browser painted frame zero, which on a
phone recording is very often black (item 12). And it stamps a duration badge,
bottom-LEFT with a play glyph so "0:09" is not read as the send time (item 13).
The poster work is deferred to first intersection and cached by URL, so a thread
scrolled back through fifty clips does not open fifty decoders.
AudioPlayer's leading slot now swaps. Before first play it shows the sender's
initial with a mic badge; after, the speed pill. Offering "2x" on a note nobody
has heard is a control for a decision the listener has not had the chance to
make (item 5). hasPlayed is latched in the play handler and deliberately NOT
derived from position > 0, because reaching the end rewinds to zero and would
flip a fully-listened note back to looking unplayed.
Media that is missing now says so instead of rendering empty (item 22).
Previously ImageBubble returned null leaving a bubble containing only a
timestamp, audio got <AudioPlayer src="">, video got <video src=""> and the
document card rendered a dead download link named "Document".
Run spacing carries the grouping (item 23). showSenderName IS the run boundary —
already computed, already driving the avatar and the name — it just never
reached the vertical rhythm. Applied to DMs too, where the avatar and name are
suppressed entirely and spacing is the only grouping signal there is.
Verified with five geometry assertions in tests/bubble-layout.spec.js, under
Playwright rather than the embedded browser: the message list is react-virtuoso
and a virtualiser renders NOTHING in a viewport it cannot measure, which is what
a zero-height headless pane gives it. The assertions are comparisons of bounding
boxes — footer to the right of the text and sharing its baseline, footer inside
the image's box, footer inside the card's box — because "beside, not below" is a
geometry claim and nothing weaker actually tests it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… usable
Parity items 21 and 24.
The composer used to freeze for the whole of an upload. `disabled={uploading}`
sat on the textarea and the send button, so a large file on a slow connection
wedged the chat with no way out but reloading the page — and there was no cancel
anywhere: the tray's clear button and the per-tile remove were both disabled
too, and nothing passed an AbortSignal to axios.
Progress was one scalar shared by the whole batch, so the bar reset to zero for
each file and nothing said WHICH file was going. The status line read
"Sending 5 files…" for the entire batch even when four were already done,
because stagedFiles was not trimmed until the loop finished.
Now: the tray hands its batch off and closes immediately, the files become one
row each ABOVE a live composer, and the user keeps typing and sending while they
upload. Each row carries its own filename, its own determinate percentage and an
X that aborts the request through an AbortController — not one that merely hides
the row while the bytes keep going.
The determinate bar is deliberately kept. iOS shows an indeterminate one only
because APIClient.upload exposes no progress callback; axios does, so the web
has real byte progress and regressing to a spinner to "match" would be backwards.
Controllers live in a ref rather than on the job objects. The first version read
one back through a setUploadJobs updater, which does not run at call time —
React invokes it during the render pass — so the send loop would have got
`undefined` for every file and cancel would have silently done nothing. Caught
before it shipped; noting it because the code reads plausibly either way.
A cancel is not a failure: it skips the toast and does not re-stage the file. A
genuine failure still goes back into the tray so it can be retried, which is the
existing recovery path and worth keeping.
No "Compressing…" phase. iOS shows one because it re-encodes before uploading;
the web does not, so inventing the status would be a lie about what is happening.
Item 24: whatever is typed when a file is attached becomes the caption and the
box is cleared. Typing a sentence and then attaching used to strand it — the
user either lost it or sent it as a second message. Only seeded when the caption
is still empty, so a second batch cannot clobber a caption already written.
Verified: 5 bubble-layout and 3 messaging e2e tests pass against the running
stack. (An intermediate run failed at superadmin login with the seeding helper
throwing — that was the 10-per-window login limiter exhausted by my own test
volume, not a regression; it passes once the bucket recovers.)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tures The remaining iOS-parity items. Grouped by area. AUDIO (items 4-8) PeaksWaveform now draws a sent note's REAL envelope. It used to refuse anything that was not a blob: URL, on two grounds. One was sound and is kept: a FABRICATED envelope is a lie, and invented amplitude on a clinical recording invites scrubbing to a peak that is not there. Decoding the actual file is the opposite of that — it is the honest version, and it is what makes the seek surface aimable rather than a flat row you can drag but not aim. The other ground was cost, and that is answered rather than ignored: decoding is deferred to first PLAY (a thread scrolled past fifty notes decodes none), cached by URL, deduplicated per URL in flight, and by the time it runs the player is fetching the same bytes anyway. Peaks are NORMALISED to the clip's own loudest bar rather than scaled by a fixed 1.6x gain. Fixed gain renders a quietly-recorded note as a near-flat line, which is the uninformative row this was meant to replace — and quiet is the common case, since a phone held at arm's length in a noisy ward records low. The paused preview is now honest when it cannot play. This is NOT Safari-only, despite how it is usually described: pickAudioFormat puts audio/mp4 first because the backend classifies .webm as video, an MP4's moov atom is written on STOP, and current Chrome records MP4 too — so both browsers hand back a partial blob no player can open. Only Firefox, falling through to Ogg/Opus, previews mid-recording. AudioPlayer had no error handling at all, so the documented "degrades to cannot-preview" behaviour never happened; the user tapped play and nothing occurred. It now says review is available once the recording is finished, and Resume and Send still work. A minimum-duration guard (0.6s, matching iOS) plus a reason for every silent drop. Three paths in the hook returned to idle with no message, one of which — a sub-second pause — left an empty player beside a live Send button. Track `ended`/`mute` handling. A browser has no phone-call interruption, but it has the two events that matter: device unplugged or permission revoked, and input seized by another app. Without them the wall-clock tick keeps counting against a recorder that has stopped capturing and the user sends silence. PAUSES rather than cancels — discarding a half-finished message without asking is worse than handing it back. IMAGES (items 9, 10, 14) utils/mediaQuality: Standard/HD tiers with a real canvas re-encode, mirroring MediaQuality.swift including the three rules that make it safe — never upscale, keep the original when the re-encode comes out larger (an optimised JPEG routinely inflates), and rename to .jpg because the server derives MIME from the extension. EXIF rotation is baked in via imageOrientation: 'from-image', without which a portrait phone photo arrives on its side. HEIC is the one case a browser cannot match: Chrome and Firefox cannot decode it, so the original bytes go up untouched. A wasm decoder is not available — the CSP is script-src 'self' with no 'wasm-unsafe-eval', the same constraint that ruled out pdf.js. The size shown in the tray is MEASURED by running the real transcode, not estimated, because a number beside the send button that is not the number that arrives is worse than no number. Videos count at original size and say "original", exactly as iOS does. The avatar upload is routed through the same transcode. The brief flagged it and it was the worse of the two raw paths: no size check at all, so a 40 MB photo was pushed at the server and only bounced by the backend limit. Video duration is measured on send. It is CLIENT-supplied on message create — the server never probes the file — and the web only ever passed it for voice notes, so every video ever sent from the web has duration null forever and the badge added in the previous commit could never render for them. useZoomPan: wheel and two-pointer pinch, anchored at the cursor rather than the image centre, because zooming to the middle while the user points at a corner is what makes a homemade zoom feel broken. Panning is enabled only while zoomed, so click-to-close and swipe-to-page keep working untouched and the gesture arbitration iOS needs a single DragGesture for is avoided by not competing. SESSION (items 25-29) Replays are marked. A follower of a single-flight refresh replayed through client(originalRequest) with _retry unset, so a follower whose replay 401d again re-entered as a WINNER and fired a second refresh — a redundant rotation racing the first, against a backend that treats refresh-token reuse as a stolen cookie and burns the whole session family. A refresh generation counter. A request already on the wire when someone else refreshed comes back 401 against a cookie that merely predates the rotation; that is now replayed rather than triggering a second rotation. Sign-out says why. Both teardowns were bare navigations carrying nothing, and the one piece of expiry copy in the codebase (helpers.js) is dead — grep shows handleApiError is never called anywhere, so no session-expiry message could ever reach a user. The reason travels in sessionStorage because the teardown is a document navigation that destroys in-memory state and any toast on screen. Login also shows the server's own 401 detail now, since "Account is deactivated" is not a typo the user can fix by retyping their password. Jittered reconnect backoff. Every tab connected when the API restarts otherwise wakes on the same 1s/2s/4s schedule and the herd hits a server still coming up — and the web has the most sockets per user, one per tab. Realtime frames are dropped when the socket is down, not queued. The finding named typing and read receipts; the queue also carried every call-signalling frame, so a queued call:initiate would RING the callee after reconnect for a call already abandoned. Nothing here needs replay: chat already refuses to queue because the HTTP fallback owns it. TOUCH (items 1-3) useHoldToTalk: hold-to-talk, swipe-up-to-lock, slide-left-to-cancel — attached on COARSE POINTERS ONLY. The click toggle is kept for a mouse rather than replaced, because a mouse-hold is a poor primary interaction and on desktop the click flow already behaves like iOS's locked state. Three adaptations a browser forces. The first hold triggers getUserMedia, whose prompt steals the pointer and swallows pointerup, so the gesture arms only once the recorder is genuinely running. MediaRecorder start-up latency means a sub-300ms hold captures a zero-byte blob, so a tap is discarded with "Hold to record, release to send" rather than sent. And mobile Safari raises the selection callout on long press, so the surface sets touch-action: none. Lock is checked before cancel, so a diagonal drag locks rather than reading as a half-hearted cancel — the same ordering as iOS micGesture. NOT DONE, deliberately: item 11 (video re-encode), which the brief itself concludes is not portable and which the CSP independently blocks; item 16 (camera), which section 4 rules out as a mobile-app decision; and items 30-33, which the brief records so that nobody ports them. Verified: 135 backend tests and all 17 Playwright e2e tests pass, including the calling, chat-ui and render-check suites that exercise the rewritten components. Running the full e2e suite needs RXHIVE_RATE_LIMIT_LOGIN raised locally — the default 10/minute is far below what seeding 17 tests costs from one IP, and the limiter is documented as tunable for exactly this. Unset it when running pytest, or test_login_rate_limiter_fires correctly fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A pre-send editor on both clients, opened from the confirmation step: crop /
rotate / flip, freehand drawing with four pen widths and a colour strip, and
text boxes with independently controlled text and plate colour, type size and
wrap width.
The whole thing rests on one rule: a staged item keeps its ORIGINAL bytes for as
long as it is staged and carries an edit model beside them, and the uploaded
bytes are re-derived from original + model on every save. So re-opening the
editor resumes where the user left off, "revert to the original" is the empty
model rather than an inverse transform, and cropping twice never compounds JPEG
generations.
Annotations are stored against the ORIGINAL image, not the visible frame, so
crop / rotate / flip carry them along and a tighter crop genuinely cuts a stroke
in half. The pipeline order is fixed at crop -> flip -> rotate, and the preview
and the exporter go through the same transform so what is on screen is what is
sent. Text is the one exception: it stays glued to its point but is
counter-rotated so it never renders sideways or mirrored.
Web
utils/mediaEdit.js the model, the geometry and the canvas compositor
utils/videoEdit.js video crop, MP4-only on purpose (see below)
chat/editor/ the editor shell, crop stage, annotate stage, controls
MessageComposer.jsx a pencil per staged tile, an Edited badge, the wiring
StagedFilePreview.jsx the same affordance at full size
iOS
Media/Editor/ model, renderers, editor shell and the two stages
MediaQuality.swift video crop folded into the existing tier export, one
pass, and every failure path now refuses instead of
silently sending the uncropped original
MediaSendSheet.swift PendingMedia keeps its original and is mutated in place
RxHiveTests/ 9 tests pinning the transform against the point
converters, and text staying upright under every
rotation and flip
Deliberate limits, all documented at the call site: web video crop is MP4-only
and hidden where MP4 is unavailable, because AVFoundation cannot decode VP8/VP9
and a crop done in Firefox would be unplayable for every iPhone user; it is a
real-time encode, capped at three minutes, and needs the tab visible. Drawing
and text are photo-only. No free-angle straighten, because an arbitrary angle
stops the crop being expressible as a rectangle. Editing a GIF flattens it to a
still, and the editor says so before saving.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nce tests Work in progress that was sitting uncommitted in the tree, committed to this branch at the author's request so the attachment-editor PR could be untangled from it. Not my change — recorded here so the history says where it came from. The centrepiece is infra/livekit.yaml. `use_external_ip: true` makes the SFU STUN out and advertise the host's PUBLIC address in ICE candidates, which is right for a deployed server behind NAT and wrong for anything on a LAN: a phone on the same Wi-Fi cannot reach its own network's public IP, because home routers rarely hairpin. Signalling succeeds, the call shows as connected, and no media ever arrives — and nothing in any log says so, which is exactly the shape of the mobile-to-web failure under investigation. LIVEKIT_NODE_IP now selects between the two, substituted at start-up because livekit-server does not expand env vars inside its config. Also in here: app/services/call_deadlines.py, a CallConnectivityWatcher on the web, group-call joining (AddParticipantsModal, OngoingGroupCallBar), docs/CALLS.md, and new coverage in test_call_resilience.py, CallResilienceTests.swift and group-calling.spec.js. Two lint fixes of mine on top: ruff I001 import ordering in app/api/media.py and app/services/conversations.py. Deliberately NOT included, because they belong to the attachment-editor branch and including them is what made that PR unreviewable in the first place: the editor components and utils on both platforms, and the five shared files (MessageComposer.jsx, StagedFilePreview.jsx, MessageComposer.swift, MediaQuality.swift, MediaSendSheet.swift) whose working-tree contents are byte-identical to that branch. Also excluded: ios/build-device (884 MB of build output), infra/.env.bak.1785822698, and frontend/tests/_audit-a.spec.js. 169 backend tests pass; ruff clean on app and tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RESTORES ios/.../Editor/MediaEditHistory.swift, which I deleted. It existed only in the working tree, never committed and not on this branch; my pre-checkout backup enumerated the files that were ON THE BRANCH rather than the ones in the directory, so a locally-new file was never copied, and the rm -rf that followed took it. git had never seen it, there were no APFS snapshots, and build-device held only object code and an index record. Rebuilt from what did survive: MediaEditHistoryTests, which specifies every behaviour, and the index record, which gave the exact original surface — present, past, limit, canUndo, init(), init(_:), set(_:), apply(_:), commit(), undo(), reset(to:) — down to the stdlib calls it made (popLast, contains(where:), removeFirst(_:)). All ten of its tests pass, including the ones that pin the behaviour nobody would guess: a no-op commit must not eat an Undo press, a 25-sample drag is one undo step, the stack is bounded and drops OLDEST first, and a saved edit is the floor Undo cannot reach past. The only deviation from the original is that `present` and `past` are private(set): nothing outside assigns them — the view writes through set(_:) — and the tests only read. CodeRabbit, the four findings that touch this PR's diff: videoEdit.js — the `seeked` wait had no timeout and no abort path, so an engine that never fires it left renderCroppedVideo unsettled, the editor stuck in its saving state, and Cancel inert (the abort listener is registered further down). Now bounded and abortable. The timeout RESOLVES rather than rejecting: a seek that never reported completion has usually still landed, and rendering from wherever the playhead is beats refusing to render. CropStageView, AnnotateStageView, MediaEditorView — `.task` in a view body inherits the view's main actor, so a device-resolution composite and a full-resolution decode were running on the run loop. Moved to Task.detached(.userInitiated), with previewOutput still computed on the main actor because it reads UIScreen.main. MessageComposer.swift — same shape: MediaTranscoder.image is a synchronous ImageIO decode plus JPEG encode inside a @mainactor task, so it froze the composer for every send. Only the raster work moves; the uploads and toast mutations stay on the main actor. EditorControls / MediaSendSheet — one @mainactor EditorInsets.window instead of two byte-identical copies. Two cached `static let`s are also two caches, which can disagree after a scene change. The annotation is not enforced under Swift 5 language mode; it is there so Swift 6 is a no-op rather than an error in two files. The other 14 CodeRabbit threads are on base-branch files — client.js, websocket.js, DocumentBubble.jsx, MessageBubble.jsx, CallStore.swift, calls.py, Info.plist and the rest. They are real, but fixing them here would put non-editor changes back into a diff that was just cleaned of them, so they belong on a PR against access-control-and-chat-fixes. Also ignores ios/build-device (884 MB of Xcode output) and infra/.env.bak.*, both of which were sitting untracked and one `git add -A` away from the history. Verified: 33 iOS tests pass (10 MediaEditHistory, 9 MediaEditGeometry, 14 others); web lint 0 errors; web build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI runs `ruff check` AND `ruff format --check`; only the first was being run locally, so 19 files had drifted out of format and the backend job was already red on this branch before the attachment-editor PR existed. That PR touches no backend file at all, so it inherited a red check it could not have caused and could not fix from its own diff — the fix belongs here. Pure formatting: no logic changed, and the 19 files include several nobody has edited recently (admin.py, messages.py, test_auth.py), which is what confirms this is accumulated drift rather than anything introduced by recent work. 169 backend tests pass; ruff check and ruff format --check both clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The backend job ran postgres and redis but no S3, while the suite has always contained tests that upload for real — test_media_upload_and_authenticated_serving and test_voice_note_duration_round_trip both predate any of the recent work. They could only ever fail there. Nobody noticed because the `ruff format --check` step failed first and the job never reached Tests. Fixing the formatting in the previous commit is what exposed this; it is not new breakage, it is the next layer down. CI has not passed on main in at least the last twenty runs. Same image, credentials and health check as the e2e job, so there is one way to stand up MinIO in this file rather than two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`docker pull bitnami/minio:latest` returns "manifest unknown" — Bitnami withdrew their public Docker Hub catalogue. The job died at container start, before any step ran, so the previous commit's MinIO service could never have worked. Both jobs are switched, not just the backend one. The e2e job carried the same reference and would have failed identically the moment it ran; it is currently SKIPPED on this PR, which is the only reason it has not. minio/minio:edge-cicd is MinIO's own image for CI. It matters that its default command is `minio server /data`: a GitHub Actions service container cannot supply a command, which rules out the plain minio/minio tag. Same MINIO_ROOT_USER / MINIO_ROOT_PASSWORD variables, so nothing else changes. Verified by pulling the image and inspecting its entrypoint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ifespan `app.main` calls `ensure_bucket()` from its lifespan, but httpx's ASGITransport does not run lifespan events, so under pytest the bucket was never created. The four tests that really upload therefore failed with NoSuchBucket the moment CI finally had a MinIO to talk to. They passed locally for a reason worth naming: the dev container's app had already created that bucket in the shared MinIO, so the suite was silently depending on a side effect of something else having run. That is the same class of problem as the stale-bytecode failures earlier in this session — a green local run that proves less than it appears to. Placed beside the schema fixture because it is the same job for the object store that the schema fixture does for Postgres: the suite establishes its own preconditions instead of inheriting whatever happens to be running. Deliberately tolerant of no object store: most of the suite never touches one, and the few that do now fail with their own clear S3 error rather than being masked by a fixture failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Web + iOS: crop, draw and caption an attachment before sending
…tomic Three concurrency defects found auditing the realtime layer. registry.add() ran outside the try/finally that owns teardown. The guard did not open until the receive loop, ~30 lines later, and three of the awaits in between raise in ordinary operation: presence.mark_online talks to Redis, the last_seen_at block talks to Postgres, and the "connected" send_text fails outright for a tab that navigated away mid-handshake. Any of them escaping left the entry in LocalRegistry.connections and the user's Redis channel subscribed for the life of the process, because nothing else prunes them. A Redis blip during a reconnect storm therefore leaked a socket per failed handshake, left _reader fanning every subsequent event at a dead connection, drifted the ws_local_sockets gauge upward permanently, and skipped handle_user_link_down so a call the user was in never got its grace window. presence.mark_online issued SADD, EXPIRE and SCARD as separate round trips while the caller used the result as an edge trigger. Two clients connecting within the same few milliseconds both landed their SADD before either SCARD, so both read count == 2, both returned False, and no "online" event was ever published — the user stayed grey to every conversation partner until something unrelated forced a refetch. Both mark_online and mark_offline now run their commands in one MULTI. Uncontended behaviour is unchanged. get_or_create_direct was a check-then-act with nothing in the schema constraining one direct conversation per pair. Three paths reach it — the direct route, forward-to-contacts and call initiation — so Alice tapping call on Bob as Bob opens a chat with her is enough to create two conversations with the same person, each holding half the history, with no way for either client to merge them. Serialized with a transaction-scoped advisory lock per pair rather than a unique index, since the index would need a migration over data that may already contain duplicates. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…gating Password hashing ran on the event loop. bcrypt at cost 12 is ~170ms of uninterruptible CPU measured locally, spent inside async handlers, so a burst of sign-ins stalled every unrelated request behind it — WebSocket pings and the health probe included. hash_password/verify_password are now async and run in the default thread pool, where the GIL is released for the C hash. Passwords were unbounded above. bcrypt hashes only the first 72 bytes and discards the rest silently, so a 100-character password and its own 72-byte prefix authenticated identically. Verified against bcrypt 4.2.1. Capped at the boundary rather than pre-hashing, which would invalidate every stored hash. This rejects set-time input that was previously accepted and truncated; existing passwords are unaffected, since the policy only runs when a password is set. /api/auth/change-password had no rate limit. password_limiter existed and was wired to the org-admin reset but not to the self-service route, which verifies current_password and is therefore an online guessing surface for anyone sitting on a hijacked session. Changing your password revoked your own session. The comment said "every other session" and the test was named test_change_password_revokes_other_sessions, but the assertion required every token revoked, the caller's included — so the test locked in the bug its name described avoiding. The effect was a silent sign-out of the tab you had just used, up to access_token_minutes later with no visible cause, and the refresh that discovered it presented a revoked token with no successor, which rotation cannot distinguish from a replay: every password change also logged a "revoking that client's session family" theft warning against an account nobody had attacked. The caller's token is now preserved and the test asserts the contract its name claims. Deactivating a conversation did not revoke reading it. Setting is_active = False is the only lever a superadmin has to cut a tenant off from a cross-org group — both the delete and archive routes leave the participant rows in place — and no read path checked the flag. The group vanished from the conversation list while its full message history, search, starred, pinned and media tabs stayed readable to anyone holding the id. The login lookup could not use its index. users.email is CITEXT with a UNIQUE btree index, so equality is already case-insensitive, but the query wrapped it in lower(), which that index cannot serve. EXPLAIN ANALYZE over 50k rows: 6.62ms sequential scan discarding 49,999 rows, against 0.044ms index scan — 150x, on every sign-in including every failed one. Fixed at all four call sites. Push-endpoint validation blocked the event loop. The SSRF check resolves a fully caller-chosen hostname with socket.getaddrinfo, the blocking libc resolver, which takes no timeout of its own: any authenticated user could point it at a black-holing nameserver and freeze the worker for the tens of seconds resolv.conf allows. Moved to a worker thread under a 2s deadline. Also adds the negative tests for require_superadmin and require_org_admin. The suite had none: every admin-portal test signs in as a superadmin, so it proved the 31 admin routes work and never that they are shut — a guard that returned its argument unconditionally would have passed the whole suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p real mark-seen swallowed calls that had not happened yet. The UPDATE had no status filter, and the CallParticipant row is written at initiation, so it already exists while the phone is ringing: opening the Calls tab mid-ring stamped seen_at on the in-flight call, and when the ring then timed out into `missed` the badge stayed at zero. The one call the user most needed flagged was the one silently marked read. In-flight calls are now excluded. The added test fails against the previous code and passes against this one. Over-long names returned 500 instead of 400. Group name, scheduled-call title, org-admin user rename, department rename and org rename all wrote straight into String(200)/String(300) columns with no input bound, so anything longer reached Postgres and came back as a value-too-long DataError. Note the asymmetry this exposed: the create schemas already capped these fields and the update schemas did not, so the identical value was a clean 422 on create and a 500 on rename. Bounded at the column widths, which converts only the crashing input. RXHIVE_MAX_UPLOAD_BYTES did nothing. config.py carried a max_upload_bytes setting advertising 100 MB that nothing read, while the enforced ceiling was a hardcoded 2 GB literal in storage.py — so an operator who set that variable, or who read it to learn the limit, was wrong by 20x in the permissive direction. storage.py now reads the setting and the default stays 2 GB, so the effective ceiling is unchanged; the deliberate rationale for that number moves to config.py with it. test_upload_still_refuses_past_the_ceiling only ever asserted that the constant equalled 2 GB, so deleting the size check from the route entirely would not have failed it. It now lowers the ceiling for the duration and exercises the real rejection branch, both sides of it, without a test moving two gigabytes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds durable cross-platform call recovery, group-call invitations, media editing and upload workflows, access-control removal, password and upload limits, CI object storage setup, platform networking configuration, and expanded backend, web, and iOS validation. ChangesApplication foundations
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ios/RxHive/Features/Chat/GroupInfoView.swift (1)
992-1009: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSelections made under an earlier search query are silently dropped.
onAddnow receivescandidates.filter { selected.contains($0.id) }.candidatesderives fromcontacts, andcontactsis replaced on every debounced query byload(force: true).selectedis not cleared when the query changes.So this sequence loses a selection:
- The user types "an" and selects Anna.
- The user types "bo".
contactsnow holds only Bob, so Anna is no longer incandidates.- The user selects Bob. The button reads "Add 2".
onAddreceives Bob only. Anna is never added, and nothing tells the user.The previous code passed the raw
selectedid set, which did not have this failure. Hold the selected contacts by id so a selection survives a query change.🐛 Proposed fix to retain selections across queries
- `@State` private var selected: Set<String> = [] + /// Keyed by contact id so a selection survives the roster being replaced by the + /// next debounced query. A plain id set loses the contact itself, and a filter + /// over the current page then drops anyone picked under an earlier query. + `@State` private var selected: [String: Contact] = [:]Button { - if selected.contains(contact.id) { - selected.remove(contact.id) - } else { - selected.insert(contact.id) - } + if selected[contact.id] != nil { + selected[contact.id] = nil + } else { + selected[contact.id] = contact + } } label: {- await onAdd(candidates.filter { selected.contains($0.id) }) + await onAdd(Array(selected.values))
contactRowand the toolbar label read the same state:- Image(systemName: selected.contains(contact.id) ? "checkmark.circle.fill" : "circle") + Image(systemName: selected[contact.id] != nil ? "checkmark.circle.fill" : "circle") .font(.system(size: 20)) - .foregroundStyle(selected.contains(contact.id) ? Theme.Color.primary : Theme.Color.border2) + .foregroundStyle(selected[contact.id] != nil ? Theme.Color.primary : Theme.Color.border2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/RxHive/Features/Chat/GroupInfoView.swift` around lines 992 - 1009, Update the selection flow in GroupInfoView so selected contacts are retained by ID across debounced query changes instead of deriving the add list solely from the current candidates. Preserve selections from prior queries when handling the toolbar action and ensure contactRow and the toolbar label use the same persistent selection state, so onAdd receives every selected contact.backend/app/realtime/hub.py (1)
56-92: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the remaining subscribe/unsubscribe race in
add.
addreadsstr(user_id) not in self.connectionsoutsideself._lock, awaitssubscribe, and only then takes the lock to register the socket. The re-check added inremovedoes not cover every interleaving.Consider this order on one event loop:
addsees no existing bucket and awaitssubscribe, which completes.removetakes the lock, pops the last connection, releases the lock, and awaitsunsubscribe, which completes.removetakes the lock for its re-check.addhas not registered yet, so the bucket is still empty andremovedoes not resubscribe.addtakes the lock and registers the socket.The user now holds a live socket on an unsubscribed channel.
_readernever fans out to it, so the client appears online and receives nothing until it reconnects — the exact failure theremovere-check was added to prevent. Mid-call reconnects are the common trigger.Perform the membership check and the registration under one lock hold, and subscribe while still holding it.
🐛 Proposed fix to serialize the check, the subscribe, and the registration
async def add(self, user_id: uuid.UUID, conn_id: str, ws: WebSocket) -> None: - # SUBSCRIBE FIRST, then register the socket. + # SUBSCRIBE FIRST, then register the socket — both under one lock hold. # # The other order leaves a window in which this worker holds a live socket it # has not yet subscribed a channel for, so anything published to that user in # the meantime is dropped by the broker with no subscriber. It is a small # window, but a reconnecting client's first act is to be sent its resumed call # state, and losing that frame is precisely the failure this whole path exists # to prevent. Subscribing to a channel with no sockets yet is harmless: the # reader simply finds an empty bucket and moves on. - if self._pubsub is not None and str(user_id) not in self.connections: - with contextlib.suppress(Exception): - await self._pubsub.subscribe(user_channel(user_id)) - async with self._lock: - self.connections.setdefault(str(user_id), {})[conn_id] = ws + # + # The lock is held across the subscribe as well, so a concurrent `remove` + # cannot unsubscribe and re-check between our subscribe and our registration. + async with self._lock: + if self._pubsub is not None and str(user_id) not in self.connections: + with contextlib.suppress(Exception): + await self._pubsub.subscribe(user_channel(user_id)) + self.connections.setdefault(str(user_id), {})[conn_id] = ws
removemust then also hold the lock across itsunsubscribe, so the two operations cannot interleave at all:async def remove(self, user_id: uuid.UUID, conn_id: str) -> None: async with self._lock: bucket = self.connections.get(str(user_id), {}) bucket.pop(conn_id, None) last_for_user = not bucket if last_for_user: self.connections.pop(str(user_id), None) - if last_for_user and self._pubsub is not None: - with contextlib.suppress(Exception): - await self._pubsub.unsubscribe(user_channel(user_id)) + if last_for_user and self._pubsub is not None: + with contextlib.suppress(Exception): + await self._pubsub.unsubscribe(user_channel(user_id))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/realtime/hub.py` around lines 56 - 92, Update Hub.add so checking whether the user has connections, subscribing via _pubsub.subscribe, and registering the socket occur under one continuous self._lock hold; do not await subscribe before acquiring the lock or perform the membership check outside it. Update Hub.remove to retain the same lock through _pubsub.unsubscribe, preserving the existing last-connection logic so add and remove cannot interleave between subscription and registration.ios/RxHive/Realtime/RealtimeClient.swift (1)
272-292: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winA pending backoff task is not cancelled when a call forces a reconnect, so a duplicate socket is opened.
Two problems combine here:
hasLiveCall()is read once, at schedule time. A backoff scheduled before a call started keeps the 30-second chat ceiling for its whole wait, even though a call is now live.CallStore.accept()(Line 698 ofios/RxHive/Features/Calls/CallStore.swift) callsauth.realtime.connect()when the socket is down. During backoff the state is.reconnecting, soconnect()passes its guard and callsopenSocket()— but neitherconnect()noropenSocket()cancelsreconnectTask. The pending task then fires later and opens a second socket on top of the live one.That is the same orphan-socket condition the comment at Lines 359-364 describes for the foreground path, reached through the newly added accept flow.
🐛 Proposed fix
private func openSocket() { + // Any pending backoff is now moot: it would open a second socket on top of + // this one, and the orphan lingers server-side until the heartbeat timeout. + reconnectTask?.cancel(); reconnectTask = nil state = .connectingRe-read the ceiling at wake-up so a call that starts mid-wait shortens the remaining delay:
reconnectTask = Task { [weak self] in try? await Task.sleep(for: .seconds(delay)) guard let self, !Task.isCancelled, !self.intentionallyClosed else { return } self.openSocket() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/RxHive/Realtime/RealtimeClient.swift` around lines 272 - 292, Update scheduleReconnect so the reconnect task re-evaluates hasLiveCall() after waking and applies the two-second ceiling when a call starts during the wait. Ensure the manual reconnect path used by CallStore.accept, through connect/openSocket, cancels any pending reconnectTask before opening a socket, preventing the delayed task from creating a duplicate connection.
🟡 Minor comments (30)
README.md-157-157 (1)
157-157: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd blank lines around the troubleshooting table.
Markdownlint reports MD058 because the table is adjacent to list content. Add a blank line before and after the table. Indent the table under item 2 if it must remain part of that list item.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` at line 157, Add blank lines immediately before and after the troubleshooting table in README.md, and indent the table consistently under list item 2 if it remains part of that item, resolving the MD058 markdownlint violation.Source: Linters/SAST tools
docs/IOS_TO_WEB_PARITY.md-381-381 (1)
381-381: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winEscape the literal
<img>element.Wrap
<img>in backticks. Markdownlint currently parses it as an image element without alternate text.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/IOS_TO_WEB_PARITY.md` at line 381, Update the documentation text in the “What the web does today” section so the literal img element is enclosed in Markdown inline-code backticks, including the existing angle brackets, preventing Markdownlint from treating it as an image.Source: Linters/SAST tools
docs/IOS_TO_WEB_PARITY.md-320-320 (1)
320-320: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace workstation-specific absolute paths.
Use repository-relative paths such as
frontend/src/components/chat/ProfileDrawer.jsx. The current/Users/carinrowena/...paths do not work for other contributors and disclose local workstation details.Also applies to: 720-720, 822-822
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/IOS_TO_WEB_PARITY.md` at line 320, Replace the workstation-specific absolute paths in the referenced documentation findings with repository-relative paths, including frontend/src/components/chat/ProfileDrawer.jsx and the entries at the other referenced locations. Preserve the cited file and line information while removing the local /Users/carinrowena/... prefix.scripts/inspect-call.py-72-83 (1)
72-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack duplicate participants per room.
problemsis process-wide. If an earlier room has duplicate identities, Line 83 suppresses the acoustic-echo diagnosis for every later room.Use a
room_has_duplicatesflag for the current room. Keepproblemsonly for the final exit status.Proposed fix
for room in rooms: + room_has_duplicates = False print(f"\nroom {room.name} ({room.num_participants} participants)") @@ if len(identities) > 1: problems += 1 + room_has_duplicates = True @@ - if not problems and len(speakers) > 1: + if not room_has_duplicates and len(speakers) > 1:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/inspect-call.py` around lines 72 - 83, Update the room-processing loop around by_user and the speakers check to track duplicate identities with a per-room room_has_duplicates flag, resetting it for each room and using it to gate the acoustic-echo diagnosis. Continue incrementing problems for duplicate rooms so it remains the aggregate final exit-status counter.docs/CALLS.md-5-6 (1)
5-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the Android link fragment.
[Android](#android)does not match the## 5. Androidheading. Use#5-androidso the link resolves.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/CALLS.md` around lines 5 - 6, Update the Android link in the wire contract documentation to use the fragment `#5-android`, matching the “## 5. Android” heading and preserving the link text.Source: Linters/SAST tools
backend/tests/test_call_resilience.py-26-39 (1)
26-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the
captured_eventsdocstring.The fixture patches only
calls_service.publish_to_users. The tests callcalls_servicedirectly, so update the docstring to describe the single patched binding instead of claiming that the bus and API bindings are also patched.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_call_resilience.py` around lines 26 - 39, Update the captured_events fixture docstring to state that it patches only calls_service.publish_to_users and that tests invoke calls_service directly; remove the inaccurate claim about patching the bus and API import bindings.backend/tests/test_calls_and_media.py-245-260 (1)
245-260: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSecurity Misconfiguration (CWE-79): Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
Reachability: External · Exploitability: Theoretical
Assert the final object response headers.
/api/media/*returns a307; the API’snosniffheader does not protect the subsequent MinIO/S3 response. Because uploads are presigned withContent-Disposition: inline, test the final response and enforceX-Content-Type-Options: nosniffandContent-Disposition: attachmentfor unknown types. The current assertion covers only stored metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_calls_and_media.py` around lines 245 - 260, Extend test_unknown_types_are_stored_as_octet_stream to request each uploaded file through its /api/media/* endpoint, follow the redirect to the final object response, and assert the final response includes X-Content-Type-Options: nosniff and Content-Disposition: attachment. Retain the existing application/octet-stream metadata assertions while validating headers on the final response rather than the initial 307.frontend/tests/calling.spec.js-357-405 (1)
357-405: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet an explicit timeout, and re-check the "unavailable" assertion after the ring screen appears.
Two points on this test:
- It has no
test.setTimeout. Its own waits already allow 10s for the offline poll, 10s for the cancel button, 30s for the ringer, and 30s for audio, plus two UI logins. That exceeds the 60s default, so this test can fail on the timeout rather than on the behavior. Every other test added in this file sets an explicit budget.- Line 384 runs immediately after the click, so it passes before the server can answer at all. A
call:unavailabletoast that arrives 500ms later is not caught. Move the count check to after the ring screen is confirmed on line 385.♻️ Proposed change
test('a ring placed while the callee is offline arrives when they come back', async ({ browser, }) => { + test.setTimeout(180000); const aliceCtx = await browser.newContext({ permissions: ['camera', 'microphone'] }); @@ await aPage.getByTestId('header-voice-call-btn').click(); - // The caller must NOT be told the callee is unavailable — the ring is live, and - // the ringing screen (not a dead-call toast) is what proves it. - await expect(aPage.getByText(/User is unavailable/i)).toHaveCount(0); await expect(aPage.getByTestId('call-cancel-btn')).toBeVisible({ timeout: 10000 }); + // The caller must NOT be told the callee is unavailable — checked after the ring + // screen is up, so a late toast is still caught. + await expect(aPage.getByText(/User is unavailable/i)).toHaveCount(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/tests/calling.spec.js` around lines 357 - 405, Update the test case around “a ring placed while the callee is offline arrives when they come back” to set an explicit timeout sufficient for its offline, ringing, and audio waits plus login setup. Move the “User is unavailable” count assertion until after the ringing screen is confirmed via the call-cancel button or ring heading, so delayed unavailable notifications are detected.backend/app/realtime/hub.py-304-331 (1)
304-331: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReconcile the resume ordering with the service docstring.
resume_calls_foris called at Line 331, after_broadcast_presence(user, "online")at Line 312. The docstring ofresume_calls_forinbackend/app/services/calls.pystates it is called "before presence is published". Either move the resume call above the presence broadcast, or correct the docstring so it describes the real order. A reader debugging frame ordering will otherwise trust the wrong sequence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/realtime/hub.py` around lines 304 - 331, Align the call in the connection flow with the documented ordering of resume_calls_for: either invoke resume_calls_for before _broadcast_presence(user, "online"), or update resume_calls_for’s docstring to accurately describe its post-presence invocation. Keep the implementation and documentation consistent so frame-order debugging reflects the actual sequence.frontend/src/components/chat/editor/MediaEditor.jsx-375-380 (1)
375-380: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not carry
item.editedDurationwhen handing back the original file.When
hasEdits(model)is false,savereturns the untouchedoriginalbytes but still reportsduration: item.editedDuration ?? null.editedDurationwas measured from a previous rendered output, not fromoriginal. So this pairs original bytes with a duration taken from different bytes.The path is reachable: open a previously cropped video, press Revert, then press Done. The composer receives the original clip together with the stale duration from the earlier render.
Report
nullhere and let the consumer read the duration from the original file.🐛 Proposed fix
// Nothing to bake. Hand back the original so a save with no edits cannot // silently re-encode a photo — and so the item stops being marked as edited. + // `duration` is null rather than `item.editedDuration`: that value was measured + // from a previous render's output, and these are the ORIGINAL bytes. if (!hasEdits(model)) { - onSave({ file: original, edit: emptyEdit(), duration: item.editedDuration ?? null }); + onSave({ file: original, edit: emptyEdit(), duration: null }); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/editor/MediaEditor.jsx` around lines 375 - 380, Update the no-edits branch in save so the onSave payload for the original file reports duration as null instead of using item.editedDuration. Keep the existing original-file and empty-edit behavior unchanged, allowing the consumer to derive duration from the original bytes.ios/RxHive/Features/Media/Editor/EditorControls.swift-274-300 (1)
274-300: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the duplicated
EditorInsetsdocumentation.Keep one documentation block.
MediaSendSheet.windowInsetsalready delegates toEditorInsets.window, so retain that ownership statement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/RxHive/Features/Media/Editor/EditorControls.swift` around lines 274 - 300, Remove the duplicated documentation block associated with EditorInsets.window in EditorControls.swift, keeping a single concise documentation block for the shared implementation. Preserve the ownership statement that MediaSendSheet.windowInsets delegates to EditorInsets.window.frontend/src/components/chat/editor/MediaEditor.jsx-275-279 (1)
275-279: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRelease the previous source when
originalchanges, not only on unmount.The release effect has an empty dependency array, so
sourceRef.current?.release?.()runs only at unmount. The load effect at Line 211 depends on[original, isVideo]and setscleanup = nullafter ownership transfers tosource.release. Iforiginalchanges while a source is already set, the load effect teardown releases nothing and the previous source is never released. For the video branch that leaks a detached video element and an object URL; for the image branch it leaks the decoded bitmap.The comment at Line 272 states that
originalis stable for the lifetime of the editor. That holds only if the parent remounts the editor for each item. Releasing the previous source in the load effect makes the code correct regardless of how the parent mounts it.🐛 Proposed fix to release the previous source on every reload
useEffect(() => { let cancelled = false; let cleanup = null; + // Whatever was decoded for the PREVIOUS `original` is dead the moment this + // effect re-runs. Releasing it here rather than only at unmount means a + // parent that reuses this component for a different item does not leak a + // video element, an object URL, or a decoded bitmap. + const previous = sourceRef.current; + if (previous) { + previous.release?.(); + setSource(null); + } const load = async () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/editor/MediaEditor.jsx` around lines 275 - 279, Update the source cleanup logic near sourceRef and the load effect so the previously owned source is released whenever original or isVideo triggers a reload, not only during unmount. Reuse the existing sourceRef ownership and ensure cleanup occurs before replacing it, while preserving final unmount cleanup and avoiding release of the newly loaded source.backend/app/api/calls.py-280-295 (1)
280-295: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSensitive Data Exposure (CWE-525): Use of Web Browser Cache Containing Sensitive Information
Reachability: External · Exploitability: Moderate
Reachability path
● Entry backend/app/main.py:37 _call_deadline_sweeper: Fire ring timeouts and reconnect-grace expiries whoever scheduled them. The safety net that makes call timers survive a worker restart. P… │ ▼ ● Hop backend/app/services/calls.py:567 invite_to_call │ ▼ ● Sink backend/app/api/calls.pyPrevent caching of
/activeresponses.The response contains identity-specific call state and has no cache directive. Set
Cache-Control: no-storefor this endpoint or all authenticated API responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/api/calls.py` around lines 280 - 295, The active_call endpoint currently returns identity-specific state without preventing intermediary or browser caching. Update active_call to include a Cache-Control: no-store response header, using the project’s established response/header pattern if available; keep the existing response payload and service call unchanged.ios/RxHive/Features/Chat/MessageBubble.swift-205-213 (1)
205-213: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMatch image/video footer placement with the web client.
ios/RxHive/Features/Chat/MessageBubble.swift:210always overlays the footer for images and videos. The web client places it below content when media has a caption or is unavailable. Add an iOS placement for those cases. Audio and file messages with attachments already place the footer inside the card.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/RxHive/Features/Chat/MessageBubble.swift` around lines 205 - 213, Update the footerPlacement computed property to match web behavior for .image and .video messages: return the below-content placement when the media has a caption or is unavailable, and retain .overlaid otherwise. Keep the existing audio/file and text/system/unknown placement logic unchanged.Source: Path instructions
ios/RxHive/Features/Media/Editor/MediaEdit.swift-477-488 (1)
477-488: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
outputdoes not re-derive the scale from the rounded size, so an edge-flush export can keep a dark hairline.
displayOutputat lines 498-504 deliberately errs large and its comment explains why: erring small "leaves a transparent hairline down one edge."outputdoes not apply the same correction. It roundswandhbut returns the unroundedscale.When
round()rounds up, the surface is up to half a pixel wider than the pixels drawn into it. Everywhere except a crop flush with the source's right or bottom edge there is more image to clip, so nothing shows. Flush against that edge there is not, and the uncovered column keeps the opaque black thatMediaEditRenderer.composefills at lines 247-248 — a dark hairline down the edge of the sent photo.The web already fixed exactly this.
outputPixelSizeinfrontend/src/utils/mediaEdit.jslines 409-413 re-derives the scale from the rounded canvas against the unrounded crop and takes the larger axis. Line 43-45 of this file states the two sides must be changed together.🐛 Proposed fix
static func output(_ source: CGSize, _ edit: MediaEdit, maxEdge: CGFloat) -> Output { let cropped = croppedPixelSize(source, edit) - let scale = min(1, maxEdge / max(cropped.width, cropped.height)) - let w = max(1, (cropped.width * scale).rounded()) - let h = max(1, (cropped.height * scale).rounded()) + let fit = min(1, maxEdge / max(cropped.width, cropped.height)) + let w = max(1, (cropped.width * fit).rounded()) + let h = max(1, (cropped.height * fit).rounded()) + // Re-derived FROM the rounded surface against the UNROUNDED crop, and the + // larger axis wins — the same correction `displayOutput` makes, and for the + // same reason. Erring large costs under a pixel of overscan, which is clipped. + let c = crop(edit) + let exactWidth = max(1e-6, c.width * source.width) + let exactHeight = max(1e-6, c.height * source.height) + let scale = max(w / exactWidth, h / exactHeight) let unrotated = CGSize(width: w, height: h)As per path instructions: "This app must stay behaviorally aligned with frontend/ — see docs/IOS_TO_WEB_PARITY.md. If a change alters shared behavior on only one platform, say so."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/RxHive/Features/Media/Editor/MediaEdit.swift` around lines 477 - 488, Update MediaEdit.output to re-derive the returned scale from the rounded dimensions and unrounded cropped size, using the larger width- and height-based scale as frontend outputPixelSize does. Keep the rounded output dimensions and quarter-turn size handling intact, and preserve behavioral parity with the web implementation.Source: Path instructions
ios/RxHive/Features/Media/Editor/CropStageView.swift-291-298 (1)
291-298: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA folded rect keeps the pre-fold anchor, so a locked ratio re-anchors to the wrong edge.
Lines 293-294 fold the rect when the user drags a handle past the opposite edge, but
anchoris passed toclampFrameRectunchanged at line 297. After a west-handle drag folds, the user is effectively holding the east edge, yetclampFrameRectstill evaluatesanchor.holdsWestand pinsx = rect.maxX - w(seeios/RxHive/Features/Media/Editor/MediaEdit.swiftline 621). The rect then jumps away from the finger.Free crop is unaffected, because the re-anchor block only runs when
ratiois non-nil. The defect is visible only with a locked aspect preset.Mirror the anchor when the span folds.
🐛 Proposed fix
var next = start + var effective = anchor if anchor.holdsWest { next.origin.x = start.minX + dx; next.size.width = start.width - dx } if anchor.holdsEast { next.size.width = start.width + dx } if anchor.holdsNorth { next.origin.y = start.minY + dy; next.size.height = start.height - dy } if anchor.holdsSouth { next.size.height = start.height + dy } // Dragged past the opposite edge: fold the rect rather than letting a // negative span through, which would render inside-out. - if next.size.width < 0 { next.origin.x += next.size.width; next.size.width = -next.size.width } - if next.size.height < 0 { next.origin.y += next.size.height; next.size.height = -next.size.height } + if next.size.width < 0 { + next.origin.x += next.size.width + next.size.width = -next.size.width + effective = effective.mirroredHorizontally + } + if next.size.height < 0 { + next.origin.y += next.size.height + next.size.height = -next.size.height + effective = effective.mirroredVertically + } write(MediaEditGeometry.clampFrameRect( - next, frame: frame, ratio: aspect.ratio, anchor: anchor + next, frame: frame, ratio: aspect.ratio, anchor: effective ))Add the two mirrors to
CropAnchorinios/RxHive/Features/Media/Editor/MediaEdit.swift:extension CropAnchor { var mirroredHorizontally: CropAnchor { switch self { case .nw: return .ne case .ne: return .nw case .sw: return .se case .se: return .sw case .w: return .e case .e: return .w case .n, .s: return self } } var mirroredVertically: CropAnchor { switch self { case .nw: return .sw case .sw: return .nw case .ne: return .se case .se: return .ne case .n: return .s case .s: return .n case .w, .e: return self } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/RxHive/Features/Media/Editor/CropStageView.swift` around lines 291 - 298, Update the fold handling in the crop drag logic to mirror the anchor whenever the corresponding width or height span becomes negative before calling clampFrameRect. Add mirroredHorizontally and mirroredVertically to CropAnchor, preserving corner and edge mappings while leaving unaffected axes unchanged, then use the updated anchor for ratio-locked clamping.ios/RxHive/Features/Media/MediaAttachmentViews.swift-881-885 (1)
881-885: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the PDF preview to the top
AuthenticatedImageuses.scaledToFill(), so the centered frame can hide the title of a portrait page. Set the frame alignment to.topbefore clipping.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/RxHive/Features/Media/MediaAttachmentViews.swift` around lines 881 - 885, Update the PDF preview view around AuthenticatedImage so its fixed-size frame uses top alignment, preserving the existing previewSize dimensions and clipping behavior. Apply the alignment directly to the frame before .clipped() so portrait pages display their top content.frontend/src/services/livekitClient.js-612-614 (1)
612-614: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe stale-participant filter misses screen-share-only participants.
The filter keeps any entry whose
streamis null._syncParticipantexcludes the screen-share source fromstream, so a remote who publishes only a screen share hasstream === nullandscreenStreamset. If that person leaves while this client is disconnected, their tile is treated as a signalled-only placeholder and stays on the grid for the rest of the call.Test both media fields.
🐛 Proposed fix
store.remoteParticipants - .filter((p) => p.stream && !present.has(p.id)) + .filter((p) => (p.stream || p.screenStream) && !present.has(p.id)) .forEach((p) => store.removeRemoteParticipant(p.id));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/livekitClient.js` around lines 612 - 614, Update the stale-participant filter in the remote-participant cleanup flow to retain entries only when they have either a regular stream or a screenStream, while still excluding IDs present in present. This ensures screen-share-only participants are removed when no longer signalled.frontend/src/services/livekitClient.js-698-724 (1)
698-724: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDerive camera state from the publication after a queued failure.
previouscan be stale when queued toggles reject in sequence. ReadgetTrackPublication(Track.Source.Camera)incatch, setisCameraOntopublication ? !publication.isMuted : false, and return that value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/services/livekitClient.js` around lines 698 - 724, Update the catch block in setCameraEnabled to derive the rollback state from room.localParticipant.getTrackPublication(Track.Source.Camera) instead of the stale previous value; set isCameraOn to publication ? !publication.isMuted : false, keep localStream refreshed, and return the derived state.frontend/src/components/chat/AudioRecorderBar.jsx-132-141 (1)
132-141: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the message when the finished recording fails to load.
This branch shows one string for two different states. If
pausedis true, "Review is available once you finish recording." is correct. IfpreviewBrokenbecomes true after the recording is finished, the recording IS finished, and the text instructs the user to do what they already did.
previewBrokenis reachable in the finished state: the.ogg/.webaoutputs frompickAudioFormatdo not load in every engine, as the comment at Line 46 states. Send still works, so the impact is limited to wrong copy.✏️ Proposed fix to branch the copy
<AlertTriangle size={14} className="text-[`#F59E0B`] flex-shrink-0" /> <span className="text-xs text-[`#A3A3A3`] leading-tight"> - Review is available once you finish recording. + {paused + ? 'Review is available once you finish recording.' + : 'This recording cannot be played back here. You can still send it.'} </span>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/AudioRecorderBar.jsx` around lines 132 - 141, Update the unavailable-preview message in the previewBroken/paused branch of AudioRecorderBar so paused recordings retain “Review is available once you finish recording,” while a finished recording with previewBroken displays copy indicating the recording preview failed to load. Keep the existing layout and conditions unchanged.frontend/src/components/chat/VideoBubble.jsx-73-83 (1)
73-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA keyboard press on the nested retry control also opens the viewer.
The container carries
role="button",tabIndex={0}, and anonKeyDownhandler. The footer is rendered inside that container at Line 118. For a failed own send,MessageFooterrenders a<button data-testid="message-retry">inside the footer.
MessageFooterstops click propagation, so a mouse click on retry is contained. It does not stopkeydown. A keyboard user who focuses the retry control and presses Enter or Space triggers the retry and the container handler, so the fullscreen viewer opens on top of the retry. A nested interactive control insiderole="button"is also invalid ARIA.Restrict the container key handler to keys that target the container itself.
🐛 Proposed fix to scope the key handler
onKeyDown={(e) => { + // The footer's retry control is a nested button; its keydown bubbles here. + if (e.target !== e.currentTarget) return; if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); setViewerOpen(true); } }}Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/VideoBubble.jsx` around lines 73 - 83, Update the VideoBubble container’s onKeyDown handler to open the viewer only when the event target is the container itself, preventing Enter or Space on the nested MessageFooter retry button from opening the viewer. Preserve the existing keyboard activation behavior when the container is the focused target.frontend/src/components/chat/FullscreenImageViewer.jsx-164-170 (1)
164-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBackdrop click no longer closes the viewer.
The gesture container spans the whole
flex-1region and callse.stopPropagation()on every click. The overlay root at Line 121 holdsonClick={onClose}. A click on the dark area beside the photo therefore no longer closes the viewer. Before this change only the image itself stopped propagation.Close on a click that lands on the container itself, and keep propagation stopped for the image.
🐛 Proposed fix to restore backdrop close
<div ref={zoom.containerRef} className="flex-1 flex items-center justify-center px-16 py-4 overflow-hidden touch-none select-none" style={{ cursor: zoom.isZoomed ? 'grab' : 'default' }} - onClick={(e) => e.stopPropagation()} + // A click on the padding around the photo still closes the viewer; + // a click on the photo (below) does not. + onClick={(e) => { if (e.target === e.currentTarget && !zoom.isZoomed) onClose(); }} {...zoom.handlers} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/FullscreenImageViewer.jsx` around lines 164 - 170, Update the gesture container around zoom.containerRef so it closes the viewer when the click target is the container itself, while preserving propagation prevention for clicks on the image. Keep the overlay root’s onClick={onClose} behavior and ensure image interactions do not trigger backdrop closing.frontend/src/components/chat/ProfileDrawer.jsx-66-73 (1)
66-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe transcode does not guarantee a bounded upload size.
The comment states that the raw path "had no size check at all" and implies the transcode replaces one.
transcodeImagereturns the original file unchanged in four cases: the file is not an image, the type isimage/gif, the canvas pipeline throws (for example an undecodable HEIC), or the re-encoded blob is not smaller than the source. A 40 MB animated GIF or an undecodable HEIC is therefore still pushed to the server in full.The catch at Line 82 also discards
err, so a rejected oversize upload and a network failure both show "Failed to upload avatar".Add an explicit size check after the transcode, and surface the server message.
🛡️ Proposed fix to bound the upload and report the reason
const outgoing = await transcodeImage(file, 'standard'); + // transcodeImage returns the original for GIFs, undecodable files, and + // any re-encode that is not smaller — so the cap has to be checked here. + const MAX_AVATAR_BYTES = 5 * 1024 * 1024; + if (outgoing.size > MAX_AVATAR_BYTES) { + toast.error('Image is too large. Choose a file under 5 MB.'); + return; + } const formData = new FormData(); formData.append('file', outgoing);- } catch (err) { toast.error('Failed to upload avatar'); } + } catch (err) { toast.error(err.response?.data?.detail || 'Failed to upload avatar'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/ProfileDrawer.jsx` around lines 66 - 73, After transcodeImage in the avatar upload flow, explicitly reject outgoing files exceeding the existing avatar upload size limit before constructing or submitting FormData, including unchanged GIFs and unsupported images. Update the catch block to retain the error and display or log the server-provided message when available, while preserving a fallback for errors without a message.frontend/src/components/chat/MessageComposer.jsx-636-641 (1)
636-641: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
uploadingstill blocks the composer it was meant to free.The comment at Lines 627-629 states the composer stays usable during background uploads. Two things contradict that while
uploadingistruefor the whole batch:
- Line 1222 renders
<Loader2>on the main composer button instead of<Send>or<Mic>. TheonClickstill callshandleSend, so text sending works, but the button shows a spinner for the entire batch and the user cannot see which action it performs.sendingFilesRefblocks a secondhandleConfirmSendfor the whole batch, so the user cannot stage and send a further batch while the first uploads.The
uploadJobsrows already carry the in-flight state. Consider scopinguploadingto the tray only, and readinguploadJobs.lengthwhere a global indicator is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/MessageComposer.jsx` around lines 636 - 641, Update the upload state handling in MessageComposer so uploading does not disable or obscure the main composer during background transfers. Scope uploading to the upload tray, use uploadJobs.length for any global in-flight indicator, and remove the whole-batch blocking behavior from sendingFilesRef so handleConfirmSend can stage and send additional batches while prior jobs remain active; preserve the uploadJobs row state for tracking each transfer.frontend/src/utils/videoPoster.js-103-112 (1)
103-112: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not cache a total failure permanently.
The
finallyblock cachesresultunconditionally. A failed extraction stores{ posterUrl: null, duration: null }, and the guard at Line 45 then returns that stored failure for every later call with the same URL.The comment at Lines 28-30 states that mobile Safari refuses to load video data while the tab is backgrounded. That produces exactly this outcome: the 8-second timeout fires, the failure is cached, and the clip shows a grey box for the rest of the session even after the tab returns to the foreground.
Cache only when at least one of
posterUrlordurationwas obtained.🐛 Proposed fix
if (objectUrl) URL.revokeObjectURL(objectUrl); if (key) { - posterCache.set(key, result); inflight.delete(key); + // A total failure is not cached: a backgrounded tab or a transient + // network error would otherwise pin a grey tile for the session. + if (result.posterUrl || result.duration != null) posterCache.set(key, result); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/videoPoster.js` around lines 103 - 112, Update the finally block in the video extraction function so posterCache.set only runs when result.posterUrl or result.duration is available; always remove the inflight entry for keyed requests, but leave complete failures uncached so later calls can retry.frontend/src/components/chat/MessageComposer.jsx-333-334 (1)
333-334: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
signalto the message-create request as well.
sendMediaFileforwardssignalonly touploadFile. ThePOST /api/conversations/{id}/messagescall at Line 343 ignores it. If the user cancels after the upload completes but before the message is created, the optimistic bubble at Line 340 still appears and the message is still persisted. The cancel then has no visible effect.🐛 Proposed fix
const { data } = await client.post(`/api/conversations/${conversationId}/messages`, { content, type: msgType, temp_id: tempId, media_url: uploadResult.file_url, reply_to: replyId, duration, - }); + }, { signal });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/MessageComposer.jsx` around lines 333 - 334, Update sendMediaFile and its message-create request so the provided signal is passed through to the POST /api/conversations/{id}/messages call, preserving cancellation between upload completion and message persistence and preventing the optimistic bubble from being committed after cancellation.frontend/src/components/chat/MessageComposer.jsx-714-721 (1)
714-721: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA fully cancelled batch reports success.
stillFailedreceives only genuine failures. The abort branch at Line 699 usescontinue, so cancelled items are counted in neitherstillFailednor any other tally. If the user cancels every upload in the batch,stillFailed.lengthis0andsentequalsbatch.length. The code then shows"N files sent"for files that were never sent.Track cancellations and exclude them from
sent.🐛 Proposed fix
const stillFailed = []; + let cancelledCount = 0; try { for (let i = 0; i < batch.length; i += 1) { const item = batch[i]; const controller = uploadControllers.current.get(item.id); - if (!controller) { URL.revokeObjectURL(item.url); continue; } + if (!controller) { URL.revokeObjectURL(item.url); cancelledCount += 1; continue; }const aborted = err?.name === 'CanceledError' || err?.code === 'ERR_CANCELED' || controller.signal.aborted; - if (aborted) { URL.revokeObjectURL(item.url); continue; } + if (aborted) { URL.revokeObjectURL(item.url); cancelledCount += 1; continue; }- const sent = batch.length - stillFailed.length; + const sent = batch.length - stillFailed.length - cancelledCount; if (stillFailed.length > 0) { setStagedFiles((prev) => [...prev, ...stillFailed]); if (sent > 0) toast.error(`${stillFailed.length} of ${batch.length} failed — still attached`); - } else { + } else if (sent > 0) { toast.success(sent > 1 ? `${sent} files sent` : 'File sent'); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/MessageComposer.jsx` around lines 714 - 721, Update the batch accounting around the upload loop and the `sent` calculation so cancelled items are tracked separately from genuine failures. Exclude cancelled items when computing `sent`, and ensure a batch where every item is cancelled does not show a success toast; preserve reattachment and failure-toast behavior for items in `stillFailed`.frontend/src/utils/mediaQuality.js-68-105 (1)
68-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the bitmap in a
finally.
bitmap.close?.()runs at Line 87, afterctx.drawImage. IfdrawImagethrows, control jumps to the catch at Line 101 and the bitmap is never closed. AnImageBitmapfor a 12 MP photo holds tens of megabytes until garbage collection, andclose()is the deterministic release.
drawImagethrowing is most likely under memory pressure, which is exactly when the retained bitmap matters.🐛 Proposed fix
const tier = TIERS[tierKey] || TIERS.standard; + let bitmap = null; try { // imageOrientation: 'from-image' bakes the EXIF rotation into the pixels, // which is what kCGImageSourceCreateThumbnailWithTransform does on iOS. // Without it a portrait photo from a phone arrives on its side, because the // orientation tag is lost the moment it is drawn to a canvas. - const bitmap = await createImageBitmap(file, { imageOrientation: 'from-image' }); + bitmap = await createImageBitmap(file, { imageOrientation: 'from-image' }); const longEdge = Math.max(bitmap.width, bitmap.height); // Never upscale. const scale = Math.min(1, tier.maxEdge / longEdge); const width = Math.max(1, Math.round(bitmap.width * scale)); const height = Math.max(1, Math.round(bitmap.height * scale)); const canvas = typeof OffscreenCanvas !== 'undefined' ? new OffscreenCanvas(width, height) : Object.assign(document.createElement('canvas'), { width, height }); const ctx = canvas.getContext('2d'); - if (!ctx) { bitmap.close?.(); return file; } + if (!ctx) return file; ctx.drawImage(bitmap, 0, 0, width, height); - bitmap.close?.(); const blob = canvas.convertToBlob ? await canvas.convertToBlob({ type: 'image/jpeg', quality: tier.jpegQuality }) : await new Promise((r) => canvas.toBlob(r, 'image/jpeg', tier.jpegQuality)); if (!blob) return file; // Keep whichever is smaller. A JPEG that is already well compressed usually // grows when re-encoded, and sending the larger of the two would make the // whole feature counterproductive. if (blob.size >= file.size) return file; const base = (file.name || 'image').replace(/\.[^.]+$/, ''); return new File([blob], `${base}.jpg`, { type: 'image/jpeg', lastModified: Date.now() }); } catch { // HEIC in a browser that cannot decode it, a corrupt file, a tainted canvas. // Upload what the user picked. return file; + } finally { + bitmap?.close?.(); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/utils/mediaQuality.js` around lines 68 - 105, Update the bitmap lifecycle in the image-processing try block so the ImageBitmap created by createImageBitmap is always closed via a finally path, including when drawImage or later canvas operations throw. Remove reliance on the current inline bitmap.close call while preserving the existing fallback return behavior and successful image conversion flow.frontend/src/hooks/useAudioRecorder.js-123-133 (1)
123-133: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDebounce transient
MediaStreamTrackmuteeventsBrowser implementations can emit transient
mute/unmutepairs during device or Bluetooth route changes. This callback immediately pauses the recording and reportsdevice_lost, even when the track resumes. Delay the pause until the track remains muted, and cancel the pending loss onunmute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/hooks/useAudioRecorder.js` around lines 123 - 133, Update the watchTrack callback to debounce transient mute events: start a delayed loss notification when the track emits mute, cancel that pending notification on unmute, and preserve immediate handling for ended. Ensure cleanup in watchTrack clears the timer and removes all listeners.frontend/src/components/chat/DocumentBubble.jsx-107-112 (1)
107-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the retry control outside the download anchor.
MessageFooterrenders a retry<button>for failed own messages, including file uploads. This branch nests that button inside<a download>. The click handler only stops propagation, so the anchor's download action can still run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/components/chat/DocumentBubble.jsx` around lines 107 - 112, Update the download-link layout around the Download icon and trailingMeta so the retry button rendered by MessageFooter remains outside the <a download> element. Keep the download anchor limited to the file-download control while preserving the existing footer alignment and metadata behavior.
CodeRabbit review finding, verified against the code.
The rejoin loop cleared `rejoinTask` at two of its five exits. The three that
did not — the `callID == nil` guard, the mid-loop cancellation check, and the
post-loop guard — left the handle pointing at a task that had already finished.
`handleRoomGone` opens with `guard rejoinTask == nil else { return }`, so from
that moment every room loss was ignored and `onRoomLost` never fired again.
The reachable path is a hang-up during a rejoin. `leave()` deliberately skips
`rejoinTask?.cancel()` while `isRejoining` — cancelling there would abort the
very `room.connect` it is waiting on — and clears `callID`, so the next pass
through the loop returns at the `callID` guard without touching the handle.
Because CallStore holds one long-lived `LiveKitSession` for the app's lifetime,
the dead handle outlived the call that stranded it: SFU restarts, captive
portals and sleep/wake stopped being recoverable for the rest of the process,
silently, on every subsequent call.
Clearing it in a `defer` puts the handle's lifetime in one place. The defer is
generation-guarded rather than unconditional: `leave()` can cancel a sleeping
task and a new attempt can claim the handle before the cancelled one resumes,
and an unconditional `defer` would then have the old task clear the new task's
handle — orphaning a running loop and letting a second start beside it. The
generation makes a task clear the handle only while it still owns it.
Verified: xcodebuild test, 54 tests pass, no new warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`no-unused-vars` could not see JSX. eslint.config.js registered
eslint-plugin-react-hooks but not eslint-plugin-react, so react/jsx-uses-vars
never ran and every identifier used only in markup read as unused. A
`varsIgnorePattern: '^[A-Z_]'` had been added to paper over it, but that only
covers Capitalized names — lowercase `motion` still warned in 41 files, and a
destructured `icon: Icon` is an argument, so it warned too.
44 of the project's 77 warnings were therefore false, and dangerous: deleting
one of those imports type-checks, lints and builds clean, then throws
ReferenceError in the browser the moment the element renders. That is why all 77
were ignored, and why the ~30 real ones sat unread in the noise.
Registering the plugin and dropping the pattern escape hatch takes the count to
8 warnings and 0 errors:
- 41 false `motion` warnings gone
- 77 dead `import React` removed (React 19's automatic JSX transform does not
need them; verified no file retains a `React` code reference — the ones that
still contain the word have it in a comment or a menu label)
- 23 unused catch bindings folded into optional catch binding
- ~15 dead icon/component imports and one duplicate `createdPw` state removed
- genuinely unused Capitalized imports now surface instead of being exempted
react/jsx-no-undef is added as an ERROR, which is the guard that was missing.
Both fast gates pass an undefined JSX component today — `vite build` does not
resolve identifiers, core `no-undef` does not descend into JSX — so the only
thing standing between a bad merge and a blank pane behind the error boundary is
the Playwright suite: 3.6 minutes in, and only on pages a test happens to open.
Verified the rule catches exactly that case before committing.
The 8 remaining warnings are left deliberately. None are dead code; each is a
feature wired up but never finished — the `loading` and `jumpLoading` spinners
that are computed and never rendered, `onBack` and `isScreenSharing` props
accepted and ignored, `setCollapsed` never called. Deleting them would erase the
evidence; they want finishing, not removing.
Also drops frontend/dump.rdb, a Redis snapshot committed by accident holding one
stale local rate-limit counter, and gitignores dump.rdb so it cannot return.
Verified: npm run lint (0 errors), npm run build, and the full Playwright suite —
31/31 passing against a live API, covering chat, group calls, the media editor
and the admin portal.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
password randomness, and security headers nginx was discarding Four defects, each verified against running software rather than by reading. Both notification toggles in Settings were inert. Settings.jsx assigns a boolean to localStorage, so the stored value is the string "true" or "false" — but both readers in websocket.js compared against 'off', which nothing writes. Turning "Notification sound" or "Desktop notifications" off therefore changed nothing: the tone kept playing and a browser notification kept appearing for every message. Four readers had grown three dialects between them — callSounds compared against 'false', IncomingCallOverlay accepted either, websocket.js accepted neither — so the call sounds honoured the setting and the message ones did not. All four now go through utils/notificationPrefs.js, which accepts 'off' as well so a profile written by an older build keeps working. Message tones leaked an AudioContext each. _playNotificationSound constructed a new AudioContext per inbound message and never closed one. Browsers cap concurrent contexts — Chrome at six — so the constructor began throwing on the seventh message, the throw was swallowed by that method's own catch, and notification sounds silently stopped for the rest of the session while every abandoned context kept its audio thread. callSounds already owned exactly the right thing: one long-lived context, correctly gated. websocket.js now delegates to it. Its oscillator list is now pruned on `ended` as well, which it did not need when only call events drove it but does now that a message can. Generated passwords came from Math.random(). Two copies — generatePassword in admin/Users.jsx and genPassword in OrgAdmin/OrgAdminUsers.jsx — and this is the credential a new user is handed to sign in with. Math.random is a seeded PRNG whose internal state is recoverable from a modest run of outputs, so an admin who sees a few generated passwords can predict the ones minted next, including for accounts in organisations that are not theirs. Both now call one shared helper over crypto.getRandomValues. Bytes >= 201 are rejected rather than folded in with %, and 201 is exactly 67 x 3, so every character of the alphabet has exactly three byte values and the draw is uniform by construction. Checked empirically: 2.4M characters, chi-square 54.1 on 66 df against a 96.6 critical value, 20000 of 20000 sampled passwords distinct, alphabet and length unchanged. nginx was discarding every security header. add_header is inherited into a location only while that location declares no add_header of its own, and all three locations set Cache-Control — so the four headers in the server block reached nothing. Confirmed against nginx:alpine before touching it: `curl -I` on /, /index.html, /sw.js and /assets/* each returned Cache-Control and nothing else. The SPA document itself shipped with no CSP and no X-Frame-Options, so the app was framable and script-unrestricted while the config sat there looking correct. Moved to a snippet included by every block that sets a header, which is the only mechanism nginx offers. Re-verified from the actual built image: all four present on every path, Cache-Control intact, nginx -t clean. form-action 'self' added while restoring the CSP — without it the policy does not stop an injected form posting credentials off-origin, which is the gap that matters most on a login page. Verified: npm run lint (0 errors), npm run build, docker build, and the full Playwright suite — 31/31 against a live API. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/eslint.config.js`:
- Around line 16-19: Upgrade eslint-plugin-react-hooks from 5.2.0 to 6.1.0 or
newer in both dependency manifests and regenerate the lockfile so its resolved
version is updated consistently, while preserving the existing reactHooks
configuration in eslint.config.js.
In `@frontend/src/utils/generatePassword.js`:
- Around line 22-31: Update generatePassword to reserve and add at least one
letter and one digit before filling the remaining positions from the existing
character set. Then shuffle the completed out array using crypto.getRandomValues
so the required characters are not positionally predictable, while preserving
the requested length behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fe37c14a-c773-4194-b938-6dcbcff9516a
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (91)
.claude/launch.json.gitignorefrontend/Dockerfilefrontend/dump.rdbfrontend/eslint.config.jsfrontend/nginx-security-headers.conffrontend/nginx.conffrontend/package.jsonfrontend/src/App.jsxfrontend/src/components/calls/ActiveCallView.jsxfrontend/src/components/calls/AddParticipantsModal.jsxfrontend/src/components/calls/CallAudioSink.jsxfrontend/src/components/calls/CallsTab.jsxfrontend/src/components/calls/IncomingCallOverlay.jsxfrontend/src/components/calls/MinimizedCallBanner.jsxfrontend/src/components/calls/OngoingGroupCallBar.jsxfrontend/src/components/calls/OutgoingCallScreen.jsxfrontend/src/components/calls/StreamVideo.jsxfrontend/src/components/calls/VideoGrid.jsxfrontend/src/components/chat/AudioPlayer.jsxfrontend/src/components/chat/AudioRecorderBar.jsxfrontend/src/components/chat/CallButtonDropdown.jsxfrontend/src/components/chat/ChatHeaderMenu.jsxfrontend/src/components/chat/ChatPanel.jsxfrontend/src/components/chat/ChatSidebar.jsxfrontend/src/components/chat/ContactInfoPanel.jsxfrontend/src/components/chat/ConversationItem.jsxfrontend/src/components/chat/ConversationSearch.jsxfrontend/src/components/chat/CreateGroupModal.jsxfrontend/src/components/chat/DateSeparator.jsxfrontend/src/components/chat/DocumentBubble.jsxfrontend/src/components/chat/EmptyChat.jsxfrontend/src/components/chat/ForwardModal.jsxfrontend/src/components/chat/FullscreenImageViewer.jsxfrontend/src/components/chat/FullscreenVideoViewer.jsxfrontend/src/components/chat/GlobalSearchResults.jsxfrontend/src/components/chat/GroupInfoPanel.jsxfrontend/src/components/chat/ImageBubble.jsxfrontend/src/components/chat/MessageBubble.jsxfrontend/src/components/chat/MessageComposer.jsxfrontend/src/components/chat/MessageContextMenu.jsxfrontend/src/components/chat/MessageFooter.jsxfrontend/src/components/chat/MessageInfoModal.jsxfrontend/src/components/chat/NewChatModal.jsxfrontend/src/components/chat/PdfViewer.jsxfrontend/src/components/chat/ProfileDrawer.jsxfrontend/src/components/chat/ReactionPicker.jsxfrontend/src/components/chat/ReplyPreview.jsxfrontend/src/components/chat/StagedFilePreview.jsxfrontend/src/components/chat/TypingIndicator.jsxfrontend/src/components/chat/VideoBubble.jsxfrontend/src/components/chat/Waveform.jsxfrontend/src/components/chat/editor/AnnotateStage.jsxfrontend/src/components/chat/editor/CropStage.jsxfrontend/src/components/chat/editor/MediaEditor.jsxfrontend/src/components/chat/editor/editorKit.jsxfrontend/src/components/chat/info/EncryptionSection.jsxfrontend/src/components/chat/info/InfoPanelPrimitives.jsxfrontend/src/components/chat/info/InfoPanelShell.jsxfrontend/src/components/chat/info/MediaLinksDocsSection.jsxfrontend/src/components/chat/info/StarredSection.jsxfrontend/src/components/chat/menuKit.jsxfrontend/src/components/common/FloatingInput.jsxfrontend/src/components/common/PageTransition.jsxfrontend/src/components/layout/AdminLayout.jsxfrontend/src/components/layout/Sidebar.jsxfrontend/src/components/layout/TopBar.jsxfrontend/src/components/org-admin/OrgAdminLayout.jsxfrontend/src/components/shared/OfflineBanner.jsxfrontend/src/components/shared/WorkspaceSwitcher.jsxfrontend/src/contexts/AuthContext.jsxfrontend/src/pages/Chat.jsxfrontend/src/pages/Login.jsxfrontend/src/pages/NotFound.jsxfrontend/src/pages/OrgAdmin/OrgAdminDashboard.jsxfrontend/src/pages/OrgAdmin/OrgAdminDepartments.jsxfrontend/src/pages/OrgAdmin/OrgAdminSettings.jsxfrontend/src/pages/OrgAdmin/OrgAdminUsers.jsxfrontend/src/pages/Settings.jsxfrontend/src/pages/admin/CrossOrgGroups.jsxfrontend/src/pages/admin/Dashboard.jsxfrontend/src/pages/admin/Departments.jsxfrontend/src/pages/admin/Organizations.jsxfrontend/src/pages/admin/Settings.jsxfrontend/src/pages/admin/Users.jsxfrontend/src/services/callSounds.jsfrontend/src/services/livekitClient.jsfrontend/src/services/websocket.jsfrontend/src/utils/generatePassword.jsfrontend/src/utils/notificationPrefs.jsios/RxHive/Features/Calls/LiveKitSession.swift
💤 Files with no reviewable changes (13)
- frontend/src/components/chat/EmptyChat.jsx
- frontend/src/components/layout/TopBar.jsx
- frontend/src/components/chat/info/InfoPanelPrimitives.jsx
- frontend/src/pages/NotFound.jsx
- frontend/src/pages/admin/Settings.jsx
- frontend/src/components/shared/WorkspaceSwitcher.jsx
- frontend/src/components/common/PageTransition.jsx
- frontend/src/components/chat/TypingIndicator.jsx
- frontend/src/components/chat/info/EncryptionSection.jsx
- frontend/src/components/chat/DateSeparator.jsx
- frontend/src/components/layout/Sidebar.jsx
- frontend/src/components/chat/MessageFooter.jsx
- frontend/src/components/calls/OngoingGroupCallBar.jsx
🚧 Files skipped from review as they are similar to previous changes (28)
- frontend/src/components/chat/DocumentBubble.jsx
- frontend/src/contexts/AuthContext.jsx
- frontend/src/components/chat/ImageBubble.jsx
- frontend/src/components/calls/MinimizedCallBanner.jsx
- frontend/src/components/chat/ProfileDrawer.jsx
- frontend/src/pages/OrgAdmin/OrgAdminDepartments.jsx
- frontend/src/components/chat/AudioRecorderBar.jsx
- frontend/src/components/calls/VideoGrid.jsx
- frontend/src/components/chat/VideoBubble.jsx
- frontend/src/components/chat/Waveform.jsx
- frontend/src/components/chat/MessageBubble.jsx
- frontend/src/components/calls/AddParticipantsModal.jsx
- frontend/src/components/chat/StagedFilePreview.jsx
- frontend/src/App.jsx
- frontend/src/components/chat/editor/CropStage.jsx
- frontend/src/components/chat/ChatPanel.jsx
- frontend/src/components/chat/editor/AnnotateStage.jsx
- frontend/src/components/calls/OutgoingCallScreen.jsx
- frontend/src/components/chat/editor/editorKit.jsx
- frontend/src/components/chat/editor/MediaEditor.jsx
- frontend/src/services/livekitClient.js
- frontend/src/components/chat/AudioPlayer.jsx
- ios/RxHive/Features/Calls/LiveKitSession.swift
- frontend/src/components/chat/MessageComposer.jsx
- frontend/src/services/websocket.js
- frontend/src/pages/Login.jsx
- frontend/src/components/calls/ActiveCallView.jsx
- frontend/src/components/chat/FullscreenImageViewer.jsx
Clears the remaining 8 `no-unused-vars` warnings so `npm run lint` is clean, without suppressions, rule changes, or behaviour changes. - CallsTab: drop `missedCallCount` from the `useCallStore()` destructure; only `setMissedCallCount` is read here. Calling the hook without a selector already subscribes to the whole store, so the subscription is unchanged — the badge that does render the count reads it separately in ChatSidebar. - VideoGrid: remove the `isScreenSharing` prop. It went unread when the tile switched to the real `localScreenStream` instead of aliasing the camera as a share, and ActiveCallView's now-dead pass-through goes with it (the flag is still used for the share button and camera-cover logic there). - ChatPanel: `jumpLoading` was never rendered — keep the setter only, `const [, setJumpLoading]`. The two setter calls still run, so the render behaviour is identical. - ConversationSearch: same shape for `loading`; no spinner consumes it. - ChatSidebar: drop the `onBack` prop, which neither Chat.jsx call site passes, and the trailing `msgId` param of `onSelectMessage`, which only needs the conversation id to navigate. - MessageComposer: drop the unused event arg from the textarea `onFocus`; the handler only resets scroll position. - OrgAdminLayout: `setCollapsed` was never called, so `collapsed` was a constant `false` — keep the value alone. The collapsed-width branches are left in place for when a toggle is wired up. Verified: `npm run lint` reports 0 problems, `npm run build` succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
generatePassword drew all 12 characters uniformly from a flat 67-character alphabet, so nothing forced the character classes the backend requires. enforce_password_policy (backend/app/core/security.py) rejects any password without both a letter and a digit, and a uniform draw misses the 10 digits (57/67)^12 of the time — 14% of generated values. Measured over 200k samples: 14.363% rejected, against the theoretical 14.375%. So roughly one in seven times an admin used the auto-generate button, user creation failed with "Password must contain both letters and numbers" on a credential the UI had just produced for them. A missing letter is (15/67)^12, about one in 60 million, and never occurred in the sample. One letter and one digit are now reserved up front and the remaining len - 2 positions filled from the full alphabet, then the whole array is shuffled with Fisher-Yates off the same CSPRNG so the reserved pair is not pinned to positions 0 and 1 — otherwise every generated password visibly starts letter-then-digit, which narrows a guess to the remaining len - 2. The byte-level uniformity of the original is kept and generalised. The reject-above-the-largest-whole-multiple rule that made the 67-way draw exactly uniform now lives in a `uniform(n)` helper, so the narrower draws added here get it too — it matters more for them, since n = 10 would split 256 into 25 byte values for six digits and 26 for the other four. Bytes are still pulled in blocks rather than one syscall per character. `len` below 2 cannot express the guarantee and could not satisfy the policy's 10-character minimum anyway, so it now throws rather than silently returning a password that would be rejected. Every call site uses the default 12. Verified: 0 of 200k generated passwords rejected by the policy predicate (was 14.363%); class distribution flat across all 12 positions (max deviation 0.34pp from the expected 20.77% digit / 73.01% letter / 6.22% symbol); all 67 characters still reachable; digit frequencies within 1.8%; len=2, 10 and 64 valid. `npm run lint` clean, `npm run build` succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/utils/generatePassword.js`:
- Around line 67-83: Update generatePassword to reject lengths greater than 256
in its existing validation before byteReader and shuffling run, using the same
maximum password length enforced by the backend. Preserve the current integer
and minimum-length checks and ensure oversized values throw the established
RangeError instead of reaching uniform with an unsupported bound.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0809a174-580b-43a1-8397-86c2ad9ee816
📒 Files selected for processing (9)
frontend/src/components/calls/ActiveCallView.jsxfrontend/src/components/calls/CallsTab.jsxfrontend/src/components/calls/VideoGrid.jsxfrontend/src/components/chat/ChatPanel.jsxfrontend/src/components/chat/ChatSidebar.jsxfrontend/src/components/chat/ConversationSearch.jsxfrontend/src/components/chat/MessageComposer.jsxfrontend/src/components/org-admin/OrgAdminLayout.jsxfrontend/src/utils/generatePassword.js
💤 Files with no reviewable changes (2)
- frontend/src/components/calls/ActiveCallView.jsx
- frontend/src/components/calls/VideoGrid.jsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/components/calls/CallsTab.jsx
- frontend/src/components/chat/ChatPanel.jsx
- frontend/src/components/chat/ConversationSearch.jsx
- frontend/src/components/chat/MessageComposer.jsx
`reactHooks.configs.recommended` on eslint-plugin-react-hooks 5.2.0 is still the eslintrc-shaped object: its `plugins` key is an array of strings, which flat config rejects with a schema error. Only `.rules` was spread, so it worked — but silently, and only because of that one detail. Every other entry in this file spreads a config whose whole object is flat-safe, so the next person to follow the local pattern and spread this one gets a broken config for a reason nothing in the file explains. Switched to `configs['recommended-latest']`, the flat-native export, and recorded why in a comment so it does not get "simplified" back. Safe on the declared `^5.2.0` range: `recommended-latest` was added in 5.2.0 itself, and the range cannot resolve below that. No behaviour change, verified rather than assumed: `eslint --print-config` output is byte-for-byte identical before and after, all 66 resolved rules including `react-hooks/rules-of-hooks: 2` and the deliberately disabled `react-hooks/exhaustive-deps: 0`. Confirmed the rule is live and not merely listed by linting a component that calls useState conditionally, which still errors. `npm run lint` clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
uniform(nextByte, n) draws a byte and redraws while it lands at or above `256 - (256 % n)`. For n > 256 that limit is 0, because `256 % n` is 256, so every possible byte fails the test and the do/while never exits. The shuffle added in abf8eaa calls it as `uniform(nextByte, i + 1)` with i starting at len - 1, so the bound peaks at exactly len: any len above 256 spins the calling thread forever. Confirmed under a watchdog — 257, 300, 1000 and 100000 all had to be SIGKILLed, while 256 returns immediately (256 % 256 is 0, so the limit is 256 and no byte can reach it). Not reachable today, since every call site takes the default 12, but a length control in the admin UI is the obvious next caller and the failure mode is a frozen tab rather than a bad password. Capped at 72 rather than 256. 256 is where uniform breaks, but it is not where the value stops being usable: bcrypt ignores anything past 72 bytes, so enforce_password_policy refuses it outright (BCRYPT_MAX_PASSWORD_BYTES, backend/app/core/security.py). Stopping at 256 would trade the hang for the bug abf8eaa just fixed — handing an admin a generated credential the backend rejects. The alphabet is single-byte ASCII, so the character count is the byte count and one number covers both. Also guarded uniform() itself. The len cap makes n <= 72 by construction, so the guard is unreachable through the public API; it is there because the helper is shared and its n <= 256 contract was implicit, which is precisely how this bug arose. A future caller now gets a RangeError instead of a hung thread. Verified: 257 through 100000 throw RangeError instead of hanging; uniform rejects 257, 300, 1000, 0, -1, 2.5, NaN and Infinity, and stays in [0, n) for n = 1, 2, 10, 52, 67, 72, 255, 256; all lengths 2..72 produce the right length with a letter and a digit and no more than 72 bytes, over 142k samples; the default len=12 is unchanged at 0 of 200k policy rejections, digit rate flat to 0.17pp, all 67 characters reachable. Lint clean, build succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
Merged
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
1 participant
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.
Summary
This PR is the first batch of a production-quality audit of the Hive application.
Included
Validation
This is Batch 1 of a larger production audit. Additional batches will address the remaining findings.
Summary by CodeRabbit
New Features
Improvements
Documentation