fix(manage): return to the list after saving, not into history - #657
fix(manage): return to the list after saving, not into history#657Elity wants to merge 2 commits into
Conversation
Saving a storage called back() -- navigate(-1) -- which assumes the previous history entry is the storage list. It often is not. Reach the edit page from a bookmark, from a reload, or from the redirect straight after logging in, and saving lands you on whatever was there before, commonly /@login: the storage is saved and the session is still valid, but it looks exactly like being logged out mid-save. Navigate to /@manage/storages instead. useRouter().to applies joinBase, so a non-root base path still resolves. `back` is no longer used in this file. The shares and metas editors call back() in the same place; they are left alone here so this stays reviewable, and I am happy to extend it if you would like. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b40dfeabd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| () => { | ||
| notify.success(t("global.save_success")) | ||
| back() | ||
| to("/@manage/storages") |
There was a problem hiding this comment.
When a user follows the normal list → edit → save flow, to() calls navigate(path, options) without replace, so this leaves the editor in history and creates list → edit → list; pressing Back after saving now reopens the just-saved editor, whereas the previous back() returned to the original list entry. Pass { replace: true } through the third argument to to() so navigation is deterministic without retaining the stale edit page.
Useful? React with 👍 / 👎.
|
@ILoveScratch2 Since the actual change in this PR is very small, I’m a bit concerned that it may have been submitted mainly for the sake of having a contribution on record. Do you think we should decline this PR and submit the change ourselves instead? |
Same defect as the storages editor, in its three siblings: back() assumes the previous history entry is the list the editor belongs to, and it is not when the page was reached from a bookmark, a reload, or the redirect straight after logging in. metas and users did not bind `to` at all; they do now. `back` is no longer referenced in any of the four files. Co-authored-by: Claude <81847+claude@users.noreply.github.com>
Elity
changed the title
Honestly, I don't have the spare time to be chasing a contribution on record. I maintain my own fork with some customisations, and that's where I hit this: after saving a storage config you get bounced back to the login page. I traced it down, and since the cause is upstream rather than in my fork, I sent the fix here. During a more adversarial review pass I found the same back() in three sibling editors, so those are covered too. If you'd rather land the change yourselves, that's completely fine by me — the diagnosis is in the description either way. |
I'm sorry, but this isn't directed at you personally. Our project has long had a policy against very small PRs. In fact, it's precisely because I think your PR is reasonable that I'm consulting the other team members instead of simply closing it outright. |
I think we can do this because it's really just a tiny change🤔 |
3 participants
Summary / 摘要
Saving a storage on
/@manage/storages/edit/<id>callsback()—navigate(-1)— which assumes the previous history entry is the storage list. It often is not. Reach the edit page from a bookmark, from a reload, or from the redirect straight after logging in, and saving lands you on whatever was there, commonly/@login.From the outside this looks exactly like being logged out by the save, but nothing is wrong:
/api/admin/storage/updateonly ever answers200,400or500, the auth middleware aborts before the handler runs, and the session stays valid. The storage really is saved — you are just standing on the login page.Server logs show both outcomes side by side. Entering from the list:
Entering after a reload of the edit page:
These navigate to the corresponding list instead.
useRouter().toappliesjoinBase, so a non-rootbase_pathstill resolves.The same
back()sits in the three sibling editors, so they are fixed here too:manage/storages/AddOrEdit.tsx/@manage/storagesmanage/shares/AddOrEdit.tsx/@manage/sharesmanage/metas/AddOrEdit.tsx/@manage/metasmanage/users/AddOrEdit.tsx/@manage/usersEach destination is the entry that editor's side-menu item already points at (
sidemenu_items.tsx).metasandusersdid not bindtoat all and now do;backis no longer referenced in any of the four files.home/previews/text-editor.tsx,settings/Common.tsxandsettings/S3.tsxalso notifysave_successbut do not navigate afterwards, so they are untouched./ 此 PR 包含破坏性变更。
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
/ 此 PR 需要关联仓库同步修改。
Related repository PRs / 关联仓库 PR:
Testing / 测试
Reproduced on a live OpenList instance (frontend from the current release dist), and confirmed the mechanism from the server access log quoted above: the same save request produces a return to the storage list or to
/@loginpurely depending on the history stack.I have not run the frontend build from source (no
pnpmtoolchain set up locally), sopnpm build/pnpm devverification is left to CI. What I did check by hand is that the same rewrite applied to the shipped minified chunk produces the intended call —notify.success(t("global.save_success")), back()→to("/@manage/storages")— and thattois in lexical scope there, since the failure branch two lines down already calls it.Treating this honestly: the diagnosis is verified against a running instance, the fix follows directly from it, but the patched build itself has not been exercised in a browser yet.
go test ./...— not applicable to this repository.Checklist / 检查清单
gofmt,go fmt, orprettierwhere applicable.AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
Refactoring / 重构
Documentation / 文档
Tests / 测试
Translation / 翻译
Review assistance / 审查辅助
I have reviewed and validated all AI-assisted content included in this PR.
I have ensured that all AI-assisted commits include
Co-Authored-Byattribution.I can reproduce all AI-assisted content included in this PR without any AI tools.