refactor: always use short id in api calls#545
Merged
Conversation
Contributor
WalkthroughMigrates API route parameters and resource identifiers from Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Related Documentation Checked 10 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/modules/repositories/__tests__/repositories.controller.test.ts (1)
189-229:⚠️ Potential issue | 🟡 MinorPATCH tests correctly use
repository.shortId; add a positive integration test for GET/api/v1/repositories/:shortId.The test suite has no positive GET test that creates a real repository record and retrieves it by shortId. Currently only a 404 test exists for non-existent repositories. Adding a GET test that creates and retrieves a record would ensure this common read path is exercised.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/modules/repositories/__tests__/repositories.controller.test.ts` around lines 189 - 229, Add a positive integration test in repositories.controller.test.ts that uses createTestSession() and createRepositoryRecord(organizationId) to create a real repository, then call app.request(`/api/v1/repositories/${repository.shortId}`) with method "GET" and auth headers (getAuthHeaders(token)), assert res.status is 200, parse await res.json() and assert expected fields (e.g., shortId, organizationId, config or name) match the created repository values; place this alongside the existing GET 404 test so the common read path for the GET handler is exercised.
🧹 Nitpick comments (5)
app/client/modules/backups/components/schedule-summary.tsx (1)
64-69:summary.repositoryLabelis computed but never consumed.
summary.repositoryLabel(computed at line 67 usingschedule.repositoryId) is not referenced anywhere in the JSX — the "Repository" card at line 175 usesschedule.repository.namedirectly. Additionally,schedule.repositoryIdis a legacy non-short ID field that may no longer exist onBackupScheduleafter this migration, silently yielding"No repository selected". The entirerepositoryLabelentry in thesummaryobject can be dropped.🧹 Proposed cleanup
return { vol: schedule.volume.name, scheduleLabel, - repositoryLabel: schedule.repositoryId || "No repository selected", retentionLabel: retentionParts.length > 0 ? retentionParts.join(" • ") : "No retention policy", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/modules/backups/components/schedule-summary.tsx` around lines 64 - 69, Remove the unused repositoryLabel property from the summary object: in the ScheduleSummary component where you build the summary (the object that currently contains vol, scheduleLabel, repositoryLabel, retentionLabel), delete the repositoryLabel entry that uses schedule.repositoryId and any related fallback text; ensure no other code expects summary.repositoryLabel and keep using schedule.repository.name in the JSX (e.g., the "Repository" card) as before so behavior is unchanged.app/server/modules/repositories/repositories.service.ts (1)
185-222: Parameter names still sayid/ "Repository ID" but now accept shortIds — rename for clarity.Every public service function (
getRepository,deleteRepository,listSnapshots,listSnapshotFiles,restoreSnapshot,getSnapshotDetails,checkHealth,startDoctor,cancelDoctor,deleteSnapshot,deleteSnapshots,tagSnapshots,refreshSnapshots,updateRepository,unlockRepository,execResticCommand) usesidas the parameter name and the JSDoc at line 215 still reads@param id Repository ID. SincefindRepositorynow performs a shortId-only lookup, all of theseidparameters are semantically shortIds. Leaving them namedidwill mislead future contributors into passing UUIDs.Consider a follow-up rename to
shortId(or at minimum update the JSDoc) to keep the contract explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/modules/repositories/repositories.service.ts` around lines 185 - 222, The public service functions getRepository, deleteRepository, listSnapshots, listSnapshotFiles, restoreSnapshot, getSnapshotDetails, checkHealth, startDoctor, cancelDoctor, deleteSnapshot, deleteSnapshots, tagSnapshots, refreshSnapshots, updateRepository, unlockRepository, and execResticCommand currently accept a parameter named id but rely on findRepository which expects a shortId; rename the parameter from id to shortId (and update all internal references and callers) or at minimum update the JSDoc `@param` lines to indicate this is a shortId to avoid confusion; ensure function signatures, any exported types, and tests/call sites are updated consistently so the contract is explicit.app/client/modules/backups/routes/backup-details.tsx (1)
127-130: Minor:listSnapshotsQueryOptionsduplicates the options built on line 80.The query options object on lines 127–130 mirrors the inline options in
listSnapshotsOptions(...)on line 80. Consider extracting this once and reusing it in both places to avoid drift.Proposed deduplication
+ const listSnapshotsQueryOptions = { + path: { shortId: schedule.repository.shortId }, + query: { backupId: schedule.shortId }, + }; + const { data: snapshots, isLoading, failureReason, } = useQuery({ - ...listSnapshotsOptions({ path: { shortId: schedule.repository.shortId }, query: { backupId: schedule.shortId } }), + ...listSnapshotsOptions(listSnapshotsQueryOptions), });Also applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/modules/backups/routes/backup-details.tsx` around lines 127 - 130, The object literal for snapshot query options is duplicated; extract it to a single constant (e.g., const snapshotQueryOptions) and use that constant in both places where listSnapshotsOptions(...) and listSnapshotsQueryOptions are currently constructed so both calls reference the same object; reference the existing symbols schedule.repository.shortId and schedule.shortId when building snapshotQueryOptions and replace the inline literals in listSnapshotsOptions(...) and the listSnapshotsQueryOptions variable with this shared constant.app/client/modules/backups/components/schedule-mirrors-config.tsx (1)
27-33: Consider derivingscheduleIdfrom the query response instead of requiring both props.The component accepts both
scheduleId: numberandscheduleShortId: string— the numeric ID is only needed for SSE event matching (lines 110, 121). Since the schedule's numericidis available on the mirrors response or could be fetched alongside, you could potentially drop thescheduleIdprop and derive it internally, keeping the public surface shortId-only. Low priority since both values are readily available at the call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/client/modules/backups/components/schedule-mirrors-config.tsx` around lines 27 - 33, The component's Props currently require both scheduleId (number) and scheduleShortId (string) even though the numeric id exists in the mirrors response; update the component to stop requiring scheduleId in the Props and instead derive it from the GetScheduleMirrorsResponse (or fetch alongside) inside the ScheduleMirrorsConfig component, then use that derived id for SSE event matching where scheduleId was used; specifically remove scheduleId from the Props type, read the numeric id from the initialData (GetScheduleMirrorsResponse) or the mirrors payload, and replace usages tied to SSE matching so external callers only provide scheduleShortId.app/server/modules/backups/backups.service.ts (1)
333-362: Dead code: thetypeof identifier === "number"branch is unreachable.The DTO defines
scheduleShortIds: "string[]"(line 379 inbackups.dto.ts), and the controller passesbody.scheduleShortIds(alwaysstring[]). The numeric branch (lines 348–354) will never execute. The(string | number)[]parameter type is misleading.Consider narrowing the parameter to
string[]to match the actual contract, or drop the numeric-ID resolution path entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/modules/backups/backups.service.ts` around lines 333 - 362, The reorderSchedules function accepts (string | number)[] but the controller/DTO always passes body.scheduleShortIds (string[]), so the numeric branch in reorderSchedules is dead; update reorderSchedules signature to accept string[] (rename parameter to scheduleShortIds if helpful), remove the typeof identifier === "number" branch and associated validIds/map checks, and simplify the mapping to resolve each shortId via scheduleIdByShortId (throw NotFoundError when a shortId is missing); also adjust any callers to pass string[] and update related types in backups.dto.ts if needed to keep signatures consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/server/modules/backups/backups.service.ts`:
- Around line 194-207: The lookup in getScheduleForVolume uses
Number(volumeIdentifier) which can wrongly treat numeric-looking shortIds as
numeric IDs; change the query to only coerce to Number when the identifier is
actually numeric (e.g. typeof volumeIdentifier === 'number' or a strict
numeric-string test like /^\d+$/) and otherwise only compare against shortId, so
update the volumesTable.findFirst OR clause to conditionally include the { id:
Number(...) } branch only when the identifier is truly numeric and otherwise
only include { shortId: String(volumeIdentifier) } to avoid accidental matches.
- Around line 123-129: Introduce a type-aware resolver function (e.g.,
resolveSchedule) that accepts (identifier: string | number, organizationId:
string), builds the query WHERE based on typeof identifier (id for numbers,
shortId for strings), performs db.query.backupSchedulesTable.findFirst and
throws NotFoundError when missing; then replace the existing OR-based lookups in
updateSchedule, deleteSchedule, getScheduleForVolume, getMirrors, updateMirrors,
and getMirrorCompatibility to call resolveSchedule instead of using
Number(scheduleIdentifier) or the OR pattern to avoid numeric-string coercion
collisions.
In `@app/server/modules/repositories/repositories.service.ts`:
- Around line 32-38: findRepository was tightened to lookup only by shortId, but
restoreSnapshot passes the full DB UUID (repository.id) into getSnapshotDetails
which calls findRepository and therefore fails to find the repo; update
restoreSnapshot to pass the repository shortId (e.g., repository.shortId) into
getSnapshotDetails (instead of repository.id) so
getSnapshotDetails/findRepository continue to match on shortId, and verify
getSnapshotDetails, restoreSnapshot and any other callers expect and propagate
the shortId parameter consistently.
---
Outside diff comments:
In `@app/server/modules/repositories/__tests__/repositories.controller.test.ts`:
- Around line 189-229: Add a positive integration test in
repositories.controller.test.ts that uses createTestSession() and
createRepositoryRecord(organizationId) to create a real repository, then call
app.request(`/api/v1/repositories/${repository.shortId}`) with method "GET" and
auth headers (getAuthHeaders(token)), assert res.status is 200, parse await
res.json() and assert expected fields (e.g., shortId, organizationId, config or
name) match the created repository values; place this alongside the existing GET
404 test so the common read path for the GET handler is exercised.
---
Nitpick comments:
In `@app/client/modules/backups/components/schedule-mirrors-config.tsx`:
- Around line 27-33: The component's Props currently require both scheduleId
(number) and scheduleShortId (string) even though the numeric id exists in the
mirrors response; update the component to stop requiring scheduleId in the Props
and instead derive it from the GetScheduleMirrorsResponse (or fetch alongside)
inside the ScheduleMirrorsConfig component, then use that derived id for SSE
event matching where scheduleId was used; specifically remove scheduleId from
the Props type, read the numeric id from the initialData
(GetScheduleMirrorsResponse) or the mirrors payload, and replace usages tied to
SSE matching so external callers only provide scheduleShortId.
In `@app/client/modules/backups/components/schedule-summary.tsx`:
- Around line 64-69: Remove the unused repositoryLabel property from the summary
object: in the ScheduleSummary component where you build the summary (the object
that currently contains vol, scheduleLabel, repositoryLabel, retentionLabel),
delete the repositoryLabel entry that uses schedule.repositoryId and any related
fallback text; ensure no other code expects summary.repositoryLabel and keep
using schedule.repository.name in the JSX (e.g., the "Repository" card) as
before so behavior is unchanged.
In `@app/client/modules/backups/routes/backup-details.tsx`:
- Around line 127-130: The object literal for snapshot query options is
duplicated; extract it to a single constant (e.g., const snapshotQueryOptions)
and use that constant in both places where listSnapshotsOptions(...) and
listSnapshotsQueryOptions are currently constructed so both calls reference the
same object; reference the existing symbols schedule.repository.shortId and
schedule.shortId when building snapshotQueryOptions and replace the inline
literals in listSnapshotsOptions(...) and the listSnapshotsQueryOptions variable
with this shared constant.
In `@app/server/modules/backups/backups.service.ts`:
- Around line 333-362: The reorderSchedules function accepts (string | number)[]
but the controller/DTO always passes body.scheduleShortIds (string[]), so the
numeric branch in reorderSchedules is dead; update reorderSchedules signature to
accept string[] (rename parameter to scheduleShortIds if helpful), remove the
typeof identifier === "number" branch and associated validIds/map checks, and
simplify the mapping to resolve each shortId via scheduleIdByShortId (throw
NotFoundError when a shortId is missing); also adjust any callers to pass
string[] and update related types in backups.dto.ts if needed to keep signatures
consistent.
In `@app/server/modules/repositories/repositories.service.ts`:
- Around line 185-222: The public service functions getRepository,
deleteRepository, listSnapshots, listSnapshotFiles, restoreSnapshot,
getSnapshotDetails, checkHealth, startDoctor, cancelDoctor, deleteSnapshot,
deleteSnapshots, tagSnapshots, refreshSnapshots, updateRepository,
unlockRepository, and execResticCommand currently accept a parameter named id
but rely on findRepository which expects a shortId; rename the parameter from id
to shortId (and update all internal references and callers) or at minimum update
the JSDoc `@param` lines to indicate this is a shortId to avoid confusion; ensure
function signatures, any exported types, and tests/call sites are updated
consistently so the contract is explicit.
nicotsx
force-pushed
the
refactor/short-ids-api-contract
branch
from
be5d2ed to
7cb274f
Compare
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Feb 19, 2026
Closed
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 by CodeRabbit
Release Notes