Skip to content

Fix posting navigation: handle successful responses in error handler #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 17, 2025
Merged

Fix posting navigation: handle successful responses in error handler #154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f32c1a7
Initial plan
Copilot Oct 16, 2025
ad02495
Fix posting navigation: handle successful topic creation in error han…
Copilot Oct 16, 2025
9474165
Apply same fix to AppendTopicActivity for consistency
Copilot Oct 16, 2025
cb7bc17
Guard topic success detection in append/create flows
graycreate Oct 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 36 additions & 13 deletions app/src/main/java/me/ghui/v2er/module/append/AppendTopicActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,26 +134,49 @@ public void handleError(GeneralError generalError) {
if (Check.isEmpty(response)) return;
Observable.just(response)
.compose(rx(null))
.map(s -> APIService.fruit().fromHtml(s, AppendTopicPageInfo.class))
.subscribe(new GeneralConsumer<AppendTopicPageInfo>() {
.map(s -> {
// First, check if this is actually a successful append
// (V2EX may return the topic page on success, which triggers error handler due to redirects)
TopicInfo topicInfo = APIService.fruit().fromHtml(s, TopicInfo.class);
if (isSuccessfulTopicResponse(topicInfo)) {
return topicInfo;
}
// If not a valid topic, try parsing as error page
return APIService.fruit().fromHtml(s, AppendTopicPageInfo.class);
})
Comment on lines +137 to +146
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The append flow now treats a TopicInfo parsed in the error path as success; add a test that feeds redirected topic HTML to confirm onAfterAppendTopic() is invoked and no error dialog appears.

Copilot generated this review using guidance from repository custom instructions.
.subscribe(new GeneralConsumer<BaseInfo>() {
@Override
public void onConsume(AppendTopicPageInfo pageInfo) {
AppendTopicPageInfo.Problem problem = pageInfo.getProblem();
if (problem != null) {
StringBuilder msg = new StringBuilder();
for (int i = 0; i < problem.getTips().size(); i++) {
msg.append(i + 1).append(". ").append(problem.getTips().get(i)).append("\n");
public void onConsume(BaseInfo baseInfo) {
if (baseInfo instanceof TopicInfo) {
// Actually a success! Treat it as such
onAfterAppendTopic((TopicInfo) baseInfo);
} else if (baseInfo instanceof AppendTopicPageInfo) {
AppendTopicPageInfo pageInfo = (AppendTopicPageInfo) baseInfo;
Comment on lines +153 to +154
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leverage pattern matching for instanceof to avoid the explicit cast and local variable reassignment: if (baseInfo instanceof TopicInfo topicInfo) { onAfterAppendTopic(topicInfo); } else if (baseInfo instanceof AppendTopicPageInfo pageInfo) { ... }.

Suggested change
} else if (baseInfo instanceof AppendTopicPageInfo) {
AppendTopicPageInfo pageInfo = (AppendTopicPageInfo) baseInfo;
} else if (baseInfo instanceof AppendTopicPageInfo pageInfo) {

Copilot uses AI. Check for mistakes.
AppendTopicPageInfo.Problem problem = pageInfo.getProblem();
if (problem != null) {
StringBuilder msg = new StringBuilder();
for (int i = 0; i < problem.getTips().size(); i++) {
msg.append(i + 1).append(". ").append(problem.getTips().get(i)).append("\n");
}
new ConfirmDialog.Builder(getActivity())
.title(problem.getTitle())
.msg(msg.toString())
.positiveText(R.string.ok)
.build().show();
}
new ConfirmDialog.Builder(getActivity())
.title(problem.getTitle())
.msg(msg.toString())
.positiveText(R.string.ok)
.build().show();
}
}
});
}

private boolean isSuccessfulTopicResponse(TopicInfo topicInfo) {
if (topicInfo == null || !topicInfo.isValid()) {
return false;
}
TopicInfo.Problem problem = topicInfo.getProblem();
return (problem == null || problem.isEmpty()) && Check.notEmpty(topicInfo.getTopicLink());
}
Comment on lines +172 to +178
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated success-detection logic; refactor into a shared helper to centralize the definition of a 'successful' TopicInfo response.

Copilot uses AI. Check for mistakes.

@Override
public void onAfterAppendTopic(TopicInfo topicInfo) {
Utils.toggleKeyboard(false, mContentET);
Expand Down
20 changes: 19 additions & 1 deletion app/src/main/java/me/ghui/v2er/module/create/CreateTopicActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,13 @@ public void handleError(GeneralError generalError) {
Observable.just(response)
.compose(rx(null))
.map(s -> {
// First, check if this is actually a successful topic creation
// (V2EX may return the topic page on success, which triggers error handler due to redirects)
TopicInfo topicInfo = APIService.fruit().fromHtml(s, TopicInfo.class);
if (isSuccessfulTopicResponse(topicInfo)) {
return topicInfo;
}
// If not a valid topic, try parsing as error pages
Comment on lines +206 to +212
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New success-detection logic is added but there is no accompanying test to assert that a 302 redirect followed by TopicInfo HTML hitting the error path results in onPostSuccess(). Please add a unit test (e.g., simulate redirected HTML passed to handleError) to ensure future changes do not regress this behavior.

Copilot generated this review using guidance from repository custom instructions.
BaseInfo resultInfo = APIService.fruit().fromHtml(s, CreateTopicPageInfo.class);
if (!resultInfo.isValid()) {
resultInfo = APIService.fruit().fromHtml(s, NewUserBannedCreateInfo.class);
Expand All @@ -212,7 +219,10 @@ public void handleError(GeneralError generalError) {
.subscribe(new GeneralConsumer<BaseInfo>(this) {
@Override
public void onConsume(BaseInfo baseInfo) {
if (baseInfo instanceof CreateTopicPageInfo) {
if (baseInfo instanceof TopicInfo) {
// Actually a success! Treat it as such
onPostSuccess((TopicInfo) baseInfo);
} else if (baseInfo instanceof CreateTopicPageInfo) {
onPostFailure((CreateTopicPageInfo) baseInfo);
} else {
onBannedCreateTopic((NewUserBannedCreateInfo) baseInfo);
Comment on lines +222 to 228
Copy link

Copilot AI Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify the instanceof + cast using Java 17 pattern matching to reduce duplication and improve readability: if (baseInfo instanceof TopicInfo topicInfo) { onPostSuccess(topicInfo); }.

Suggested change
if (baseInfo instanceof TopicInfo) {
// Actually a success! Treat it as such
onPostSuccess((TopicInfo) baseInfo);
} else if (baseInfo instanceof CreateTopicPageInfo) {
onPostFailure((CreateTopicPageInfo) baseInfo);
} else {
onBannedCreateTopic((NewUserBannedCreateInfo) baseInfo);
if (baseInfo instanceof TopicInfo topicInfo) {
// Actually a success! Treat it as such
onPostSuccess(topicInfo);
} else if (baseInfo instanceof CreateTopicPageInfo createTopicPageInfo) {
onPostFailure(createTopicPageInfo);
} else if (baseInfo instanceof NewUserBannedCreateInfo bannedCreateInfo) {
onBannedCreateTopic(bannedCreateInfo);

Copilot uses AI. Check for mistakes.
Expand All @@ -234,4 +244,12 @@ private void onBannedCreateTopic(NewUserBannedCreateInfo bannedCreateInfo) {
}).build().show();
}

private boolean isSuccessfulTopicResponse(TopicInfo topicInfo) {
if (topicInfo == null || !topicInfo.isValid()) {
return false;
}
TopicInfo.Problem problem = topicInfo.getProblem();
return (problem == null || problem.isEmpty()) && Check.notEmpty(topicInfo.getTopicLink());
}
Comment on lines +247 to +253
Copy link

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper method is duplicated in AppendTopicActivity; consider extracting it to a shared utility (e.g., TopicResponseUtils) or a base activity to avoid divergence if success criteria evolve.

Copilot uses AI. Check for mistakes.

}
Loading