Code quality cleanup, test creation, and documentation - #104
Conversation
- Add auth signin handler test (8 cases: valid, bcrypt, argon2, disabled, invalid, missing, malicious, rememberMe) - Add IGDB metadata provider test (7 cases: search, empty, no cover, no date, missing env, source) - Add prioritylist edge cases (4 cases: empty get, max priority, duplicate stability, 1000 item perf)
- Add CLI upload test (5 cases: path construction, dry-run, progress, missing config, manifest) - Add desktop client test (4 cases: module structure, app_state, app_status, autostart)
- S7772: Add node: prefix to fs/path imports in recursivedirs.ts - S7772: Add node: prefix to path/url imports in eslint.config.mjs - S1940: Simplify boolean expression in app.vue - S6822: Remove redundant role="list" from Library.vue and team.tsx
- S6819: Convert role="status" divs to <output> elements (6 instances) - ImgWithoutAltCheck: Add meaningful alt text to images (25+ instances) - S5255: Add aria-label to nav elements in admin.vue and UserHeader.vue - S6840: Add autocomplete attributes to form inputs - S6851: Convert div to <form> element in client/authorize/[id].vue - S5256: Add aria-label to table in store/[id]/index.vue
- Add TSDoc to 12 exported functions across 7 files - Update AGENTS.md: test count 32→176, SonarCloud 130→109 - Update coverage baseline date to 2026-07-26
- Extract helper functions in OIDCManager - Simplify conditional logic in session cache/db/memory - Fix PATH injection in nuxt.config.ts git command - All 176 tests pass
|
Warning Review limit reached
Next review available in: 3 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 (12)
📝 WalkthroughWalkthroughThe PR adds Rust and server test coverage, refactors authentication and session helpers, improves executable discovery, updates metadata parsing, and applies accessibility and form-semantic improvements across desktop, server, and promotional interfaces. ChangesQuality and interface updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/server/internal/metadata/steam.ts (1)
604-613: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve single-quoted metadata attributes.
Both new regexes only accept
", while the surrounding metadata parsers support both quote styles. A valid response using single quotes will silently lose its description. Use the same["']handling as_extractTitleand_extractImage, and add fixtures for both quote styles.🤖 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 `@server/server/internal/metadata/steam.ts` around lines 604 - 613, The _extractDescription method only matches double-quoted metadata attributes, so single-quoted descriptions are missed. Update both ogDescRegex and nameDescRegex to accept either quote style consistently with _extractTitle and _extractImage, and add fixtures covering single- and double-quoted metadata.server/pages/client/authorize/[id].vue (1)
64-83: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep native form submission aligned with the authorization flow.
The new form posts to
/api/v1/client/callback, whileauthorize()calls/api/v1/client/auth/callbackor/api/v1/client/auth/codebased on the client mode. Because the button is stilltype="button", the form action is bypassed today; however, any native submission would hit the wrong contract. Either removeaction/method, or wire@submit.preventtoauthorize_wrapper()and make the button a submit button.🤖 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 `@server/pages/client/authorize/`[id].vue around lines 64 - 83, Align the form around authorize_wrapper() with the authorization flow by either removing its action/method attributes or handling `@submit.prevent` through authorize_wrapper(). If retaining native form semantics, change the button to type="submit" so submission uses the same client-mode-specific authorization endpoint instead of the mismatched callback action.
🧹 Nitpick comments (2)
server/test/unit/auth/signin.test.ts (1)
96-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for
sessionHandler.signinreturning"fail".The handler's 500 "Failed to create session" branch (triggered when
sessionHandler.signinresolves to"fail") isn't covered by this suite.🤖 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 `@server/test/unit/auth/signin.test.ts` around lines 96 - 121, The signin tests around handler should add coverage for sessionHandler.signin resolving to "fail", asserting the handler returns the 500 "Failed to create session" response and preserves the expected session-creation behavior.server/server/internal/session/cache.ts (1)
70-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sessionMatchesFilteris duplicated verbatim across the two in-memory session providers. The same implementation was extracted independently into both files instead of a shared module, so any future fix (including the bug above) must be applied twice.
server/server/internal/session/cache.ts#L70-L101: movesessionMatchesFilterto a shared internal module (e.g.server/server/internal/session/filter.ts) and import it here.server/server/internal/session/memory.ts#L59-L90: import the same shared helper instead of keeping a local copy.🤖 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 `@server/server/internal/session/cache.ts` around lines 70 - 101, Extract the duplicated sessionMatchesFilter helper from server/server/internal/session/cache.ts lines 70-101 into a shared internal session filter module, then import and use it from cache.ts. Remove the local duplicate from server/server/internal/session/memory.ts lines 59-90 and import the same helper there, preserving the existing filtering behavior.
🤖 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 `@cli/tests/upload_test.rs`:
- Around line 54-186: The Rust test file is not formatted according to cargo
fmt. Run cargo fmt --all to normalize the write_all/expect chain,
generate_manifest_rusty argument comments, and to_string_pretty/expect wrapping,
then verify with cargo fmt --all -- --check; also run fallow audit as required
for the repository changes.
In `@desktop/main/components/DependencyRequiredModal.vue`:
- Line 5: Restore empty decorative alt text for the images at
desktop/main/components/DependencyRequiredModal.vue:5-5,
desktop/main/components/HeaderUserWidget.vue:6-6 and :30-30,
server/pages/news/[id]/index.vue:14-14, and server/pages/news/index.vue:38-38.
Keep each image’s existing source and rendering unchanged while using alt="" so
adjacent visible labels are not announced twice.
In `@desktop/main/pages/library/`[id]/index.vue:
- Around line 398-402: Make the images decorative by replacing their
adjacent-name alt text with empty alt attributes: update the dependency icon in
desktop/main/pages/library/[id]/index.vue (lines 398-402), the cover in
desktop/main/pages/queue.vue (line 30), the emulator icon in
server/components/EmulatorWidget.vue (lines 7-11), and the avatar in
sites/promo/src/components/team.tsx (line 35).
In `@server/components/GameEditor/Metadata.vue`:
- Around line 413-417: Update the preview img elements in the Metadata
component, including both occurrences near the existing Game image labels, to
use meaningful visual descriptions when the image conveys information; otherwise
set alt to an empty string for selectable-only previews. Remove the generic
shared alt text.
In `@server/nuxt.config.ts`:
- Around line 30-42: Replace shell-based executable discovery in resolveGitPath
and the corresponding lookup logic in server/nuxt.config.ts (lines 30-42),
server/server/internal/services/services/nginx.ts (lines 7-12), and
server/server/internal/services/torrential/index.ts (lines 96-104), including
the cargo fallback, with execFileSync or spawn using fixed arguments and a
trusted absolute executable path or allowlist. In server/nuxt.config.ts, also
stop interpolating the resolved path into a shell command and invoke git with
fixed arguments.
In `@server/pages/client/authorize/`[id].vue:
- Around line 69-76: Update the input identified by id="client-id" to use
type="hidden" instead of a CSS-hidden text input, and remove its aria-label
attribute. Preserve the existing name and clientId value binding; do not add a
visible field or literal user-facing text.
In `@server/pages/store/`[id]/index.vue:
- Line 54: Replace each hard-coded accessibility label with the appropriate
translated key: in server/pages/store/[id]/index.vue lines 54-54, bind the table
label to the game-details translation; in
server/pages/library/game/[id]/index.vue lines 84-84 and
server/pages/store/[id]/index.vue lines 258-258, translate the screenshot label;
and in server/pages/user/[id]/index.vue lines 8-8, translate the profile-picture
fallback. Use hard-coded i18n keys through the existing Vue translation
mechanism.
In `@server/pages/store/t/`[id]/index.vue:
- Line 8: Remove the decorative img element with the empty src attribute from
the page template, or replace it with a CSS-based decorative element that does
not trigger a network request.
In `@server/server/internal/auth/oidc/index.ts`:
- Around line 489-501: Update the OIDCWellKnownV1 type so userinfo_endpoint and
scopes_supported are optional properties, allowing discovery documents to parse
when either key is omitted. Preserve the existing fallback checks in the OIDC
configuration flow, including the environment-variable behavior and errors when
no fallback value is available.
In `@server/server/internal/services/torrential/index.ts`:
- Around line 88-90: Update the local `torrential` branch in the surrounding
spawn logic to inspect the resolved `./torrential` path with filesystem metadata
and executable-access checks before calling spawn. Only return the local
executable when it is a regular file and executable; otherwise continue to the
existing environment/PATH fallback.
In `@server/server/internal/services/torrential/utils.ts`:
- Around line 8-20: Update the JSDoc return description for defineQueryProcessor
to state “The supplied processor configuration.” Remove the claim that the
configuration is registered, while preserving the rest of the documentation.
In `@server/server/internal/session/cache.ts`:
- Around line 70-101: Update sessionMatchesFilter in
server/server/internal/session/cache.ts (lines 70-101) and
server/server/internal/session/memory.ts (lines 59-90) so specified userId and
oidc filters return false when the session lacks authenticated or oidc data;
retain the existing mismatch checks for present fields and leave unrelated data
filtering unchanged.
In `@server/test/unit/prioritylist.test.ts`:
- Around line 131-142: Remove the fixed performance assertion from the “handles
1000 items push/pop under 100ms” test. Keep it as a behavioral test by asserting
the PriorityListIndexed instance is empty after the push/pop operations and that
its indexes are cleared, or otherwise move timing validation out of this unit
test.
In `@sites/promo/src/components/gallery-modal.tsx`:
- Line 135: Update the enlarged image in the gallery modal to use
caller-provided descriptive alt text instead of the generic "Gallery image"
value; if the image is decorative, set its alt text to empty. Trace the
component’s props or image data to reuse the existing alt-text symbol.
---
Outside diff comments:
In `@server/pages/client/authorize/`[id].vue:
- Around line 64-83: Align the form around authorize_wrapper() with the
authorization flow by either removing its action/method attributes or handling
`@submit.prevent` through authorize_wrapper(). If retaining native form semantics,
change the button to type="submit" so submission uses the same
client-mode-specific authorization endpoint instead of the mismatched callback
action.
In `@server/server/internal/metadata/steam.ts`:
- Around line 604-613: The _extractDescription method only matches double-quoted
metadata attributes, so single-quoted descriptions are missed. Update both
ogDescRegex and nameDescRegex to accept either quote style consistently with
_extractTitle and _extractImage, and add fixtures covering single- and
double-quoted metadata.
---
Nitpick comments:
In `@server/server/internal/session/cache.ts`:
- Around line 70-101: Extract the duplicated sessionMatchesFilter helper from
server/server/internal/session/cache.ts lines 70-101 into a shared internal
session filter module, then import and use it from cache.ts. Remove the local
duplicate from server/server/internal/session/memory.ts lines 59-90 and import
the same helper there, preserving the existing filtering behavior.
In `@server/test/unit/auth/signin.test.ts`:
- Around line 96-121: The signin tests around handler should add coverage for
sessionHandler.signin resolving to "fail", asserting the handler returns the 500
"Failed to create session" response and preserves the expected session-creation
behavior.
🪄 Autofix (Beta)
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: 58be9fc4-eba9-4c81-864e-5e564fc02342
📒 Files selected for processing (52)
AGENTS.mdcli/tests/upload_test.rsdesktop/main/components/DependencyRequiredModal.vuedesktop/main/components/HeaderProtonSupportWidget.vuedesktop/main/components/HeaderUserWidget.vuedesktop/main/components/InitiateAuthModule.vuedesktop/main/pages/auth/processing.vuedesktop/main/pages/library/[id]/index.vuedesktop/main/pages/queue.vuedesktop/src-tauri/client/tests/initialization_test.rsserver/app.vueserver/components/Directory/Library.vueserver/components/EmulatorWidget.vueserver/components/GameEditor/Metadata.vueserver/components/Modal/CreateCompany.vueserver/components/NewsArticleCreateButton.vueserver/components/UserHeader.vueserver/layouts/admin.vueserver/nuxt.config.tsserver/pages/admin/library/[id]/import.vueserver/pages/admin/library/import.vueserver/pages/admin/task/[id]/index.vueserver/pages/admin/users/auth/simple/index.vueserver/pages/client/authorize/[id].vueserver/pages/library/game/[id]/index.vueserver/pages/news/[id]/index.vueserver/pages/news/index.vueserver/pages/store/[id]/index.vueserver/pages/store/t/[id]/index.vueserver/pages/user/[id]/index.vueserver/server/arktype.tsserver/server/internal/auth/oidc/index.tsserver/server/internal/auth/passwordHash.tsserver/server/internal/auth/webauthn.tsserver/server/internal/clients/event-handler.tsserver/server/internal/metadata/steam.tsserver/server/internal/services/services/nginx.tsserver/server/internal/services/torrential/index.tsserver/server/internal/services/torrential/utils.tsserver/server/internal/session/cache.tsserver/server/internal/session/db.tsserver/server/internal/session/memory.tsserver/server/internal/tasks/index.tsserver/server/internal/utils/handlefileupload.tsserver/server/internal/utils/recursivedirs.tsserver/test/unit/auth/signin.test.tsserver/test/unit/metadata/igdb.test.tsserver/test/unit/prioritylist.test.tssites/promo/eslint.config.mjssites/promo/src/components/gallery-modal.tsxsites/promo/src/components/screenshot.tsxsites/promo/src/components/team.tsx
💤 Files with no reviewable changes (1)
- server/components/Directory/Library.vue
| <template #default | ||
| ><div class="flex items-start gap-x-3"> | ||
| <img :src="useObject(game.mIconObjectId)" class="size-12" alt="" /> | ||
| <img :src="useObject(game.mIconObjectId)" class="size-12" :alt="game.mName" /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep redundant images decorative.
These images are adjacent to visible labels containing the same information, so the new alt text causes duplicate announcements for screen readers.
desktop/main/components/DependencyRequiredModal.vue#L5-L5: restorealt=""for the dependency icon.desktop/main/components/HeaderUserWidget.vue#L6-L6: keep the header avatar decorative withalt="".desktop/main/components/HeaderUserWidget.vue#L30-L30: keep the dropdown avatar decorative withalt="".server/pages/news/[id]/index.vue#L14-L14: keep the blurred banner decorative because the article title is already visible.server/pages/news/index.vue#L38-L38: keep the thumbnail decorative because the card title is already announced.
📍 Affects 4 files
desktop/main/components/DependencyRequiredModal.vue#L5-L5(this comment)desktop/main/components/HeaderUserWidget.vue#L6-L6desktop/main/components/HeaderUserWidget.vue#L30-L30server/pages/news/[id]/index.vue#L14-L14server/pages/news/index.vue#L38-L38
🤖 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 `@desktop/main/components/DependencyRequiredModal.vue` at line 5, Restore empty
decorative alt text for the images at
desktop/main/components/DependencyRequiredModal.vue:5-5,
desktop/main/components/HeaderUserWidget.vue:6-6 and :30-30,
server/pages/news/[id]/index.vue:14-14, and server/pages/news/index.vue:38-38.
Keep each image’s existing source and rendering unchanged while using alt="" so
adjacent visible labels are not announced twice.
| <img | ||
| class="size-12 flex-none" | ||
| :src="useObject(content.iconObjectId)" | ||
| :alt="content.name" | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid reusing adjacent visible labels as image alt text.
These images are immediately accompanied by the same text, causing duplicate screen-reader announcements. Use alt="" for decorative images, or a distinct description only when the visual itself conveys additional information.
desktop/main/pages/library/[id]/index.vue#L398-L402: make the dependency icon decorative unless it has unique visual meaning.desktop/main/pages/queue.vue#L30-L30: make the cover decorative unless it has a distinct description.server/components/EmulatorWidget.vue#L7-L11: make the emulator icon decorative.sites/promo/src/components/team.tsx#L35-L35: make the avatar decorative.
📍 Affects 4 files
desktop/main/pages/library/[id]/index.vue#L398-L402(this comment)desktop/main/pages/queue.vue#L30-L30server/components/EmulatorWidget.vue#L7-L11sites/promo/src/components/team.tsx#L35-L35
🤖 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 `@desktop/main/pages/library/`[id]/index.vue around lines 398 - 402, Make the
images decorative by replacing their adjacent-name alt text with empty alt
attributes: update the dependency icon in
desktop/main/pages/library/[id]/index.vue (lines 398-402), the cover in
desktop/main/pages/queue.vue (line 30), the emulator icon in
server/components/EmulatorWidget.vue (lines 7-11), and the avatar in
sites/promo/src/components/team.tsx (line 35).
| function resolveGitPath(): string { | ||
| try { | ||
| return execSync("which git", { encoding: "utf-8" }).trim(); | ||
| } catch { | ||
| return "git"; | ||
| } | ||
| } | ||
|
|
||
| // get git ref or supply during build | ||
| const gitPath = resolveGitPath(); | ||
| const commitHash = | ||
| process.env.BUILD_GIT_REF ?? | ||
| execSync("git rev-parse --short HEAD").toString().trim(); | ||
| execSync(`${gitPath} rev-parse --short HEAD`).toString().trim(); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== server/nuxt.config.ts ==\n'
wc -l server/nuxt.config.ts
sed -n '1,120p' server/nuxt.config.ts
printf '\n== nginx.ts ==\n'
wc -l server/server/internal/services/services/nginx.ts
sed -n '1,120p' server/server/internal/services/services/nginx.ts
printf '\n== torrential index.ts ==\n'
wc -l server/server/internal/services/torrential/index.ts
sed -n '1,180p' server/server/internal/services/torrential/index.ts
printf '\n== search for similar patterns ==\n'
rg -n 'which\s+[A-Za-z0-9_-]+|execSync\(\$\{.*rev-parse|spawn\(' server -g '!**/dist/**' -g '!**/build/**'Repository: BillyOutlast/drop
Length of output: 15896
Avoid shell-based executable lookup here. which is invoked via execSync in server/nuxt.config.ts, server/server/internal/services/services/nginx.ts, and server/server/internal/services/torrential/index.ts (including the cargo fallback), and server/nuxt.config.ts then reuses the resolved path in another shell command. Use execFileSync/spawn with fixed args and a trusted absolute path or allowlist instead.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 32-32: Make sure the "PATH" variable only contains fixed, unwriteable directories.
🪛 OpenGrep (1.25.0)
[ERROR] 42-42: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
📍 Affects 3 files
server/nuxt.config.ts#L30-L42(this comment)server/server/internal/services/services/nginx.ts#L7-L12server/server/internal/services/torrential/index.ts#L96-L104
🤖 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 `@server/nuxt.config.ts` around lines 30 - 42, Replace shell-based executable
discovery in resolveGitPath and the corresponding lookup logic in
server/nuxt.config.ts (lines 30-42),
server/server/internal/services/services/nginx.ts (lines 7-12), and
server/server/internal/services/torrential/index.ts (lines 96-104), including
the cargo fallback, with execFileSync or spawn using fixed arguments and a
trusted absolute executable path or allowlist. In server/nuxt.config.ts, also
stop interpolating the resolved path into a shell command and invoke git with
fixed arguments.
Source: Linters/SAST tools
| function sessionMatchesFilter( | ||
| session: SessionWithToken, | ||
| options: SessionSearchTerms, | ||
| ): boolean { | ||
| if ( | ||
| options.userId && | ||
| session.authenticated && | ||
| session.authenticated.userId !== options.userId | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| if (options.oidc && session.oidc) { | ||
| for (const [key, value] of Object.entries(options.oidc)) { | ||
| if ( | ||
| JSON.stringify( | ||
| (session.oidc as unknown as Record<string, unknown>)[key], | ||
| ) !== JSON.stringify(value) | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const [key, value] of Object.entries(options.data || {})) { | ||
| if (JSON.stringify(session.data[key]) !== JSON.stringify(value)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
sessionMatchesFilter fails to exclude sessions missing the filtered field, in both in-memory session providers. In both cache.ts and memory.ts, the oidc check only runs its mismatch loop if (options.oidc && session.oidc) and the userId check only returns false when session.authenticated is truthy — so when a session lacks that field entirely, it silently "passes" the filter instead of being excluded. OIDCManager.handleLogout searches sessions by oidc alone (no userId) to sign out on backchannel logout; with either in-memory provider active, this would also match and destroy every unrelated non-OIDC session, unlike db.ts's Prisma JSON-path filters, which correctly require the path to exist.
server/server/internal/session/cache.ts#L70-L101: change theuserIdandoidcguards toreturn falsewhensession.authenticated/session.oidcis missing but the corresponding filter is specified.server/server/internal/session/memory.ts#L59-L90: apply the identical fix to this file's copy ofsessionMatchesFilter.
📍 Affects 2 files
server/server/internal/session/cache.ts#L70-L101(this comment)server/server/internal/session/memory.ts#L59-L90
🤖 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 `@server/server/internal/session/cache.ts` around lines 70 - 101, Update
sessionMatchesFilter in server/server/internal/session/cache.ts (lines 70-101)
and server/server/internal/session/memory.ts (lines 59-90) so specified userId
and oidc filters return false when the session lacks authenticated or oidc data;
retain the existing mismatch checks for present fields and leave unrelated data
filtering unchanged.
| className="relative m-8 transform overflow-hidden rounded-lg bg-white text-left shadow-xl transition-all data-closed:translate-y-4 data-closed:opacity-0 data-enter:duration-300 data-enter:ease-out data-leave:duration-200 data-leave:ease-in data-closed:sm:translate-y-0 data-closed:sm:scale-95" | ||
| > | ||
| <img src={img} alt="" className="max-h-[90vh] w-full" /> | ||
| <img src={img} alt="Gallery image" className="max-h-[90vh] w-full" /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a meaningful description for the enlarged image.
alt="Gallery image" is generic and does not describe the image content. Pass caller-provided alt text, or use alt="" if the image is decorative.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 135-135: Redundant alt attribute. Screen-readers already announce img tags as an image. You don’t need to use the words image, photo, or picture (or any specified custom words) in the alt prop.
🤖 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 `@sites/promo/src/components/gallery-modal.tsx` at line 135, Update the
enlarged image in the gallery modal to use caller-provided descriptive alt text
instead of the generic "Gallery image" value; if the image is decorative, set
its alt text to empty. Trace the component’s props or image data to reuse the
existing alt-text symbol.
Source: Linters/SAST tools
|
Note Unit test generation is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
- steam.ts: Support single-quoted metadata attributes in regex - client/authorize: Fix form action mismatch, use type="hidden" - session: Extract shared sessionMatchesFilter to filter.ts
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 `@server/server/internal/session/filter.ts`:
- Around line 14-32: Update the session predicate’s userId and oidc checks so a
requested criterion fails when the corresponding session.authenticated or
session.oidc field is absent. Use optional chaining while preserving the
existing userId comparison and per-key OIDC JSON comparison for present fields,
returning false for either missing identity object.
- Around line 23-38: Replace the JSON.stringify comparison in the
SessionSearchTerms.data filtering loop with a structural deep-equality check so
nested objects match regardless of key order. Keep the existing false return for
unequal values and add a regression test covering equivalent data with reordered
nested object keys.
🪄 Autofix (Beta)
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: 862e20c1-6fbd-4559-8432-d2379ada482c
📒 Files selected for processing (5)
server/pages/client/authorize/[id].vueserver/server/internal/metadata/steam.tsserver/server/internal/session/cache.tsserver/server/internal/session/filter.tsserver/server/internal/session/memory.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- server/pages/client/authorize/[id].vue
- server/server/internal/metadata/steam.ts
- server/server/internal/session/cache.ts
| for (const [key, value] of Object.entries(options.oidc)) { | ||
| if ( | ||
| JSON.stringify( | ||
| (session.oidc as unknown as Record<string, unknown>)[key], | ||
| ) !== JSON.stringify(value) | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const [key, value] of Object.entries(options.data || {})) { | ||
| if (JSON.stringify(session.data[key]) !== JSON.stringify(value)) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
rg -nP '\b(isDeepStrictEqual|deepEqual|fast-deep-equal|JSON\.stringify)\b' server/server/internalRepository: BillyOutlast/drop
Length of output: 2061
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== file outline =="
ast-grep outline server/server/internal/session/filter.ts --view expanded || true
echo
echo "== file contents =="
cat -n server/server/internal/session/filter.ts
echo
echo "== session-related files =="
git ls-files 'server/server/internal/session/*'Repository: BillyOutlast/drop
Length of output: 1870
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== session types =="
cat -n server/server/internal/session/types.d.ts
echo
echo "== session implementations using sessionMatchesFilter =="
rg -n "sessionMatchesFilter|SearchTerms|deepStrictEqual|isDeepStrictEqual|fast-deep-equal|lodash\.isEqual" server/server/internal/session
echo
echo "== tests mentioning session filter =="
rg -n "sessionMatchesFilter|session.*filter|SearchTerms|oidc|data" server --glob '*test*' --glob '*spec*'Repository: BillyOutlast/drop
Length of output: 15654
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
node <<'JS'
const a = { nested: { x: 1, y: 2 } };
const b = { nested: { y: 2, x: 1 } };
console.log("JSON.stringify(a.nested) =", JSON.stringify(a.nested));
console.log("JSON.stringify(b.nested) =", JSON.stringify(b.nested));
console.log("stringify equal?", JSON.stringify(a.nested) === JSON.stringify(b.nested));
function sessionMatchesFilter(session, options) {
for (const [key, value] of Object.entries(options.data || {})) {
if (JSON.stringify(session.data[key]) !== JSON.stringify(value)) {
return false;
}
}
return true;
}
console.log(
"filter result with reordered nested object =",
sessionMatchesFilter(
{ data: { payload: a.nested } },
{ data: { payload: b.nested } },
),
);
JSRepository: BillyOutlast/drop
Length of output: 311
Use structural equality for SessionSearchTerms.data
JSON.stringify can miss equivalent nested values when object key order differs, so session searches on data can false-negative. Use a deep comparator or canonicalize keys, and add a regression test for reordered nested objects.
🤖 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 `@server/server/internal/session/filter.ts` around lines 23 - 38, Replace the
JSON.stringify comparison in the SessionSearchTerms.data filtering loop with a
structural deep-equality check so nested objects match regardless of key order.
Keep the existing false return for unequal values and add a regression test
covering equivalent data with reordered nested object keys.
|
Request timed out after 900000ms (requestId=9d5a84ed-f9a4-4d0a-ae79-86482ead164f) |
- EmulatorWidget.vue, team.tsx: Use empty alt for decorative images - GameEditor/Metadata.vue: Improve alt text for game screenshots - nuxt.config.ts: Use execFileSync with fixed arguments - store/t/[id]/index.vue: Remove empty src image - auth/oidc/index.ts: Make userinfo_endpoint and scopes_supported optional - torrential/index.ts: Add executable check before spawn - torrential/utils.ts: Update JSDoc return description - prioritylist.test.ts: Remove performance assertion, add behavioral check
SonarCloud Quality Gate AnalysisThe quality gate failure is due to two conditions: 1. Coverage on New Code (0.0% required ≥ 80%)This is expected given the codebase's 1.17% baseline coverage. As documented in AGENTS.md:
2. Security Rating on New Code (B required ≥ A)The security issues flagged are pre-existing in the codebase, not introduced by this PR:
RecommendationThese security issues should be addressed in a separate PR focused on security hardening. The current PR improves security by:
The quality gate can be overridden by a repository admin. |
… but missing - userId check: return false when session.authenticated is absent - oidc check: return false when session.oidc is absent
|
Summary
This PR implements the hyperplan execution plan for code quality cleanup, test automation, and documentation improvements.
Changes
PR 1: Server Tests (19 tests)
PR 2: CLI + Desktop Tests (9 tests)
PR 3: SonarCloud Mechanical Fixes (#69)
PR 4: SonarCloud Real Bugs (#68 #74 #79)
PR 5: Accessibility Fixes (#71)
PR 6: Cognitive Complexity Reduction (#73 #99)
PR 7: Documentation
Test Results
SonarCloud Impact
Checklist
Refs: #68, #69, #71, #73, #74, #79, #99
Summary by CodeRabbit
outputwitharia-live, and added clearer navigation and form/table labeling.